fix(files): Ensure renaming state is correctly reset

Problem: Is a node is renamed and the new name is out of the current
visible list of nodes the component will be recycled, this means
the props will change, so when the `onRename` functions is about to reset
the state the `this.source` will point to a different node.

To fix this, but also to separate business logic from visual representation,
the logic is moved into the renaming store and the component is only
responsible for rendering.

Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>

Author: susnux

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,7 @@
  */
 
 import type { User } from '@nextcloud/cypress'
-import { getRowForFile, triggerActionForFile } from './FilesUtils'
+import { getRowForFile, renameFile, triggerActionForFile } from './FilesUtils'
 
 const haveValidity = (validity: string | RegExp) => {
 	if (typeof validity === 'string') {
@@ -74,4 +74,80 @@ 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('not.be.visible')
+		// 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