[stable29] fix(files): Ensure renaming state is correctly reset

apps/files/src/components/FileEntry/FileEntryCheckbox.vue

@@ -22,7 +22,7 @@
 <template>
 	<td class="files-list__row-checkbox"
 		@keyup.esc.exact="resetSelection">
-		<NcLoadingIcon v-if="isLoading" />
+		<NcLoadingIcon v-if="isLoading" :name="loadingLabel" />
 		<NcCheckboxRadioSwitch v-else
 			:aria-label="ariaLabel"
 			:checked="isSelected"
@@ -92,6 +92,11 @@ export default defineComponent({
 		isFile() {
 			return this.source.type === FileType.File
 		},
+		loadingLabel() {
+			return this.isFile
+				? t('files', 'File is loading')
+				: t('files', 'Folder is loading')
+		},
 		ariaLabel() {
 			return this.isFile
 				? t('files', 'Toggle selection for file "{displayName}"', { displayName: this.source.basename })

apps/files/src/components/FileEntry/FileEntryName.vue

@@ -57,10 +57,8 @@
 import type { FileAction, Node } from '@nextcloud/files'
 import type { PropType } from 'vue'
 
-import axios from '@nextcloud/axios'
 import { showError, showSuccess } from '@nextcloud/dialogs'
-import { emit } from '@nextcloud/event-bus'
-import { FileType, NodeStatus } from '@nextcloud/files'
+import { FileType } from '@nextcloud/files'
 import { translate as t } from '@nextcloud/l10n'
 import { defineComponent, inject } from 'vue'
 
@@ -260,64 +258,23 @@ export default defineComponent({
 			}
 
 			const oldName = this.source.basename
-			const oldEncodedSource = this.source.encodedSource
-			if (oldName === newName) {
-				this.stopRenaming()
-				return
-			}
-
-			// Set loading state
-			this.loading = 'renaming'
-			this.$set(this.source, 'status', NodeStatus.LOADING)
 
-			// Update node
-			this.source.rename(newName)
-
-			logger.debug('Moving file to', { destination: this.source.encodedSource, oldEncodedSource })
 			try {
-				await axios({
-					method: 'MOVE',
-					url: oldEncodedSource,
-					headers: {
-						Destination: this.source.encodedSource,
-						Overwrite: 'F',
-					},
-				})
-
-				// Success 🎉
-				emit('files:node:updated', this.source)
-				emit('files:node:renamed', this.source)
-				showSuccess(t('files', 'Renamed "{oldName}" to "{newName}"', { oldName, newName }))
-
-				// Reset the renaming store
-				this.stopRenaming()
-				this.$nextTick(() => {
-					const nameContainter = this.$refs.basename as HTMLElement | undefined
-					nameContainter?.focus()
-				})
+				const status = await this.renamingStore.rename()
+				if (status) {
+					showSuccess(t('files', 'Renamed "{oldName}" to "{newName}"', { oldName, newName }))
+					this.$nextTick(() => {
+						const nameContainter = this.$refs.basename as HTMLElement | undefined
+						nameContainter?.focus()
+					})
+				} else {
+					// Was cancelled - meaning the renaming state is just reset
+				}
 			} catch (error) {
-				logger.error('Error while renaming file', { error })
-				// Rename back as it failed
-				this.source.rename(oldName)
+				logger.error(error as Error)
+				showError((error as Error).message)
 				// And ensure we reset to the renaming state
 				this.startRenaming()
-
-				if (isAxiosError(error)) {
-					// TODO: 409 means current folder does not exist, redirect ?
-					if (error?.response?.status === 404) {
-						showError(t('files', 'Could not rename "{oldName}", it does not exist any more', { oldName }))
-						return
-					} else if (error?.response?.status === 412) {
-						showError(t('files', 'The name "{newName}" is already used in the folder "{dir}". Please choose a different name.', { newName, dir: this.directory }))
-						return
-					}
-				}
-
-				// Unknown error
-				showError(t('files', 'Could not rename "{oldName}"', { oldName }))
-			} finally {
-				this.loading = false
-				this.$set(this.source, 'status', undefined)
 			}
 		},
 

apps/files/src/store/renaming.ts

@@ -19,17 +19,96 @@
  * along with this program. If not, see <http://www.gnu.org/licenses/>.
  *
  */
-import { defineStore } from 'pinia'
-import { subscribe } from '@nextcloud/event-bus'
 import type { Node } from '@nextcloud/files'
 import type { RenamingStore } from '../types'
 
+import axios, { isAxiosError } from '@nextcloud/axios'
+import { emit, subscribe } from '@nextcloud/event-bus'
+import { NodeStatus } from '@nextcloud/files'
+import { t } from '@nextcloud/l10n'
+import { basename, dirname } from 'path'
+import { defineStore } from 'pinia'
+import logger from '../logger'
+import Vue from 'vue'
+
 export const useRenamingStore = function(...args) {
 	const store = defineStore('renaming', {
 		state: () => ({
 			renamingNode: undefined,
 			newName: '',
 		} as RenamingStore),
+
+		actions: {
+			/**
+			 * Execute the renaming.
+			 * This will rename the node set as `renamingNode` to the configured new name `newName`.
+			 * @return true if success, false if skipped (e.g. new and old name are the same)
+			 * @throws Error if renaming fails, details are set in the error message
+			 */
+			async rename(): Promise<boolean> {
+				if (this.renamingNode === undefined) {
+					throw new Error('No node is currently being renamed')
+				}
+
+				const newName = this.newName.trim?.() || ''
+				const oldName = this.renamingNode.basename
+				const oldEncodedSource = this.renamingNode.encodedSource
+				if (oldName === newName) {
+					return false
+				}
+
+				const node = this.renamingNode
+				Vue.set(node, 'status', NodeStatus.LOADING)
+
+				try {
+					// rename the node
+					this.renamingNode.rename(newName)
+					logger.debug('Moving file to', { destination: this.renamingNode.encodedSource, oldEncodedSource })
+					// create MOVE request
+					await axios({
+						method: 'MOVE',
+						url: oldEncodedSource,
+						headers: {
+							Destination: this.renamingNode.encodedSource,
+							Overwrite: 'F',
+						},
+					})
+
+					// Success 🎉
+					emit('files:node:updated', this.renamingNode as Node)
+					emit('files:node:renamed', this.renamingNode as Node)
+					emit('files:node:moved', {
+						node: this.renamingNode as Node,
+						oldSource: `${dirname(this.renamingNode.source)}/${oldName}`,
+					})
+					this.$reset()
+					return true
+				} catch (error) {
+					logger.error('Error while renaming file', { error })
+					// Rename back as it failed
+					this.renamingNode.rename(oldName)
+					if (isAxiosError(error)) {
+						// TODO: 409 means current folder does not exist, redirect ?
+						if (error?.response?.status === 404) {
+							throw new Error(t('files', 'Could not rename "{oldName}", it does not exist any more', { oldName }))
+						} else if (error?.response?.status === 412) {
+							throw new Error(t(
+								'files',
+								'The name "{newName}" is already used in the folder "{dir}". Please choose a different name.',
+								{
+									newName,
+									dir: basename(this.renamingNode.dirname),
+								},
+							))
+						}
+					}
+					// Unknown error
+					throw new Error(t('files', 'Could not rename "{oldName}"', { oldName }))
+				} finally {
+					Vue.set(node, 'status', undefined)
+				}
+			},
+		},
 	})
 
 	const renamingStore = store(...args)

cypress/e2e/files/files-renaming.cy.ts

@@ -4,7 +4,8 @@
  */
 
 import type { User } from '@nextcloud/cypress'
-import { getRowForFile, triggerActionForFile } from './FilesUtils'
+import { getRowForFile, renameFile, triggerActionForFile } from './FilesUtils'
+import { assertNotExistOrNotVisible } from '../settings/usersUtils'
 
 const haveValidity = (validity: string | RegExp) => {
 	if (typeof validity === 'string') {
@@ -74,4 +75,84 @@ describe('files: Rename nodes', { testIsolation: true }, () => {
 			// See validity
 			.should(haveValidity(/reserved name/i))
 	})
+
+	it('shows accessible loading information', () => {
+		const { resolve, promise } = Promise.withResolvers()
+
+		getRowForFile('file.txt').should('be.visible')
+
+		// intercept the rename (MOVE)
+		// the callback will wait until the promise resolve (so we have time to check the loading state)
+		cy.intercept(
+			'MOVE',
+			/\/remote.php\/dav\/files\//,
+			(request) => {
+				// we need to wait in the onResponse handler as the intercept handler times out otherwise
+				request.on('response', async () => { await promise })
+			},
+		).as('moveFile')
+
+		// Start the renaming
+		triggerActionForFile('file.txt', 'rename')
+		getRowForFile('file.txt')
+			.findByRole('textbox', { name: 'Filename' })
+			.should('be.visible')
+			.type('{selectAll}new-name.txt{enter}')
+
+		// Loading state is visible
+		getRowForFile('new-name.txt')
+			.findByRole('img', { name: 'File is loading' })
+			.should('be.visible')
+		// checkbox is not visible
+		getRowForFile('new-name.txt')
+			.findByRole('checkbox', { name: /^Toggle selection/ })
+			.should('not.exist')
+
+		cy.log('Resolve promise to preoceed with MOVE request')
+			.then(() => resolve(null))
+
+		// Ensure the request is done (file renamed)
+		cy.wait('@moveFile')
+
+		// checkbox visible again
+		getRowForFile('new-name.txt')
+			.findByRole('checkbox', { name: /^Toggle selection/ })
+			.should('exist')
+		// see the loading state is gone
+		getRowForFile('new-name.txt')
+			.findByRole('img', { name: 'File is loading' })
+			.should('not.exist')
+	})
+
+	/**
+	 * This is a regression test of: https://github.com/nextcloud/server/issues/47438
+	 * The issue was that the renaming state was not reset when the new name moved the file out of the view of the current files list
+	 * due to virtual scrolling the renaming state was not changed then by the UI events (as the component was taken out of DOM before any event handling).
+	 */
+	it('correctly resets renaming state', () => {
+		for (let i = 1; i <= 20; i++) {
+			cy.uploadContent(user, new Blob([]), 'text/plain', `/file${i}.txt`)
+		}
+		cy.viewport(1200, 500) // 500px is smaller then 20 * 50 which is the place that the files take up
+		cy.login(user)
+		cy.visit('/apps/files')
+
+		getRowForFile('file.txt').should('be.visible')
+		// Z so it is shown last
+		renameFile('file.txt', 'zzz.txt')
+
+		// not visible any longer
+		getRowForFile('zzz.txt')
+			.should(assertNotExistOrNotVisible)
+
+		// scroll file list to bottom
+		cy.get('[data-cy-files-list]').scrollTo('bottom')
+		cy.screenshot()
+
+		// The file is no longer in rename state
+		getRowForFile('zzz.txt')
+			.should('be.visible')
+			.findByRole('textbox', { name: 'Filename' })
+			.should('not.exist')
+	})
 })

cypress/support/commands.ts

@@ -30,6 +30,7 @@ import { basename } from 'path'
 import '@testing-library/cypress/add-commands'
 import 'cypress-if'
 import 'cypress-wait-until'
+
 addCommands()
 
 // Register this file's custom commands types

cypress/support/component.ts

@@ -19,6 +19,11 @@
  * along with this program. If not, see <http://www.gnu.org/licenses/>.
  *
  */
+
+// TODO: Drop once we use Node 22
+import 'core-js/stable/index.js'
+
+// Commands
 import 'cypress-axe'
 
 /* eslint-disable */

cypress/support/e2e.ts

@@ -19,6 +19,11 @@
  * along with this program. If not, see <http://www.gnu.org/licenses/>.
  *
  */
+
+// TODO: Drop once we use Node 22
+import 'core-js/stable/index.js'
+
+// Commands
 import 'cypress-axe'
 import './commands.ts'