Skip to content

Commit ef5a9cf

Browse files
susnuxsusnux
committed
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>
1 parent cd3dc17 commit ef5a9cf

3 files changed

Lines changed: 122 additions & 61 deletions

File tree

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

Lines changed: 12 additions & 58 deletions
Original file line numberDiff line numberDiff line change
@@ -41,12 +41,9 @@
4141
import type { FileAction, Node } from '@nextcloud/files'
4242
import type { PropType } from 'vue'
4343
44-
import axios, { isAxiosError } from '@nextcloud/axios'
4544
import { showError, showSuccess } from '@nextcloud/dialogs'
46-
import { emit } from '@nextcloud/event-bus'
4745
import { FileType, NodeStatus } from '@nextcloud/files'
4846
import { translate as t } from '@nextcloud/l10n'
49-
import { dirname } from '@nextcloud/paths'
5047
import { defineComponent, inject } from 'vue'
5148
5249
import NcTextField from '@nextcloud/vue/dist/Components/NcTextField.js'
@@ -245,66 +242,23 @@ export default defineComponent({
245242
}
246243
247244
const oldName = this.source.basename
248-
const oldEncodedSource = this.source.encodedSource
249-
if (oldName === newName) {
250-
this.stopRenaming()
251-
return
252-
}
253-
254-
// Set loading state
255-
this.$set(this.source, 'status', NodeStatus.LOADING)
256245
257-
// Update node
258-
this.source.rename(newName)
259-
260-
logger.debug('Moving file to', { destination: this.source.encodedSource, oldEncodedSource })
261246
try {
262-
await axios({
263-
method: 'MOVE',
264-
url: oldEncodedSource,
265-
headers: {
266-
Destination: this.source.encodedSource,
267-
Overwrite: 'F',
268-
},
269-
})
270-
271-
// Success 🎉
272-
emit('files:node:updated', this.source)
273-
emit('files:node:renamed', this.source)
274-
emit('files:node:moved', {
275-
node: this.source,
276-
oldSource: `${dirname(this.source.source)}/${oldName}`,
277-
})
278-
showSuccess(t('files', 'Renamed "{oldName}" to "{newName}"', { oldName, newName }))
279-
280-
// Reset the renaming store
281-
this.stopRenaming()
282-
this.$nextTick(() => {
283-
const nameContainter = this.$refs.basename as HTMLElement | undefined
284-
nameContainter?.focus()
285-
})
247+
const status = await this.renamingStore.rename()
248+
if (status) {
249+
showSuccess(t('files', 'Renamed "{oldName}" to "{newName}"', { oldName, newName }))
250+
this.$nextTick(() => {
251+
const nameContainter = this.$refs.basename as HTMLElement | undefined
252+
nameContainter?.focus()
253+
})
254+
} else {
255+
// Was cancelled - meaning the renaming state is just reset
256+
}
286257
} catch (error) {
287-
logger.error('Error while renaming file', { error })
288-
// Rename back as it failed
289-
this.source.rename(oldName)
258+
logger.error(error as Error)
259+
showError((error as Error).message)
290260
// And ensure we reset to the renaming state
291261
this.startRenaming()
292-
293-
if (isAxiosError(error)) {
294-
// TODO: 409 means current folder does not exist, redirect ?
295-
if (error?.response?.status === 404) {
296-
showError(t('files', 'Could not rename "{oldName}", it does not exist any more', { oldName }))
297-
return
298-
} else if (error?.response?.status === 412) {
299-
showError(t('files', 'The name "{newName}" is already used in the folder "{dir}". Please choose a different name.', { newName, dir: this.directory }))
300-
return
301-
}
302-
}
303-
304-
// Unknown error
305-
showError(t('files', 'Could not rename "{oldName}"', { oldName }))
306-
} finally {
307-
this.$set(this.source, 'status', undefined)
308262
}
309263
},
310264

‎apps/files/src/store/renaming.ts‎

Lines changed: 81 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2,17 +2,96 @@
22
* SPDX-FileCopyrightText: 2023 Nextcloud GmbH and Nextcloud contributors
33
* SPDX-License-Identifier: AGPL-3.0-or-later
44
*/
5-
import { defineStore } from 'pinia'
6-
import { subscribe } from '@nextcloud/event-bus'
75
import type { Node } from '@nextcloud/files'
86
import type { RenamingStore } from '../types'
97

8+
import axios, { isAxiosError } from '@nextcloud/axios'
9+
import { emit, subscribe } from '@nextcloud/event-bus'
10+
import { NodeStatus } from '@nextcloud/files'
11+
import { t } from '@nextcloud/l10n'
12+
import { basename, dirname } from 'path'
13+
import { defineStore } from 'pinia'
14+
import logger from '../logger'
15+
import Vue from 'vue'
16+
1017
export const useRenamingStore = function(...args) {
1118
const store = defineStore('renaming', {
1219
state: () => ({
1320
renamingNode: undefined,
1421
newName: '',
1522
} as RenamingStore),
23+
24+
actions: {
25+
/**
26+
* Execute the renaming.
27+
* This will rename the node set as `renamingNode` to the configured new name `newName`.
28+
* @return true if success, false if skipped (e.g. new and old name are the same)
29+
* @throws Error if renaming fails, details are set in the error message
30+
*/
31+
async rename(): Promise<boolean> {
32+
if (this.renamingNode === undefined) {
33+
throw new Error('No node is currently being renamed')
34+
}
35+
36+
const newName = this.newName.trim?.() || ''
37+
const oldName = this.renamingNode.basename
38+
const oldEncodedSource = this.renamingNode.encodedSource
39+
if (oldName === newName) {
40+
return false
41+
}
42+
43+
const node = this.renamingNode
44+
Vue.set(node, 'status', NodeStatus.LOADING)
45+
46+
try {
47+
// rename the node
48+
this.renamingNode.rename(newName)
49+
logger.debug('Moving file to', { destination: this.renamingNode.encodedSource, oldEncodedSource })
50+
// create MOVE request
51+
await axios({
52+
method: 'MOVE',
53+
url: oldEncodedSource,
54+
headers: {
55+
Destination: this.renamingNode.encodedSource,
56+
Overwrite: 'F',
57+
},
58+
})
59+
60+
// Success 🎉
61+
emit('files:node:updated', this.renamingNode as Node)
62+
emit('files:node:renamed', this.renamingNode as Node)
63+
emit('files:node:moved', {
64+
node: this.renamingNode as Node,
65+
oldSource: `${dirname(this.renamingNode.source)}/${oldName}`,
66+
})
67+
this.$reset()
68+
return true
69+
} catch (error) {
70+
logger.error('Error while renaming file', { error })
71+
// Rename back as it failed
72+
this.renamingNode.rename(oldName)
73+
if (isAxiosError(error)) {
74+
// TODO: 409 means current folder does not exist, redirect ?
75+
if (error?.response?.status === 404) {
76+
throw new Error(t('files', 'Could not rename "{oldName}", it does not exist any more', { oldName }))
77+
} else if (error?.response?.status === 412) {
78+
throw new Error(t(
79+
'files',
80+
'The name "{newName}" is already used in the folder "{dir}". Please choose a different name.',
81+
{
82+
newName,
83+
dir: basename(this.renamingNode.dirname),
84+
},
85+
))
86+
}
87+
}
88+
// Unknown error
89+
throw new Error(t('files', 'Could not rename "{oldName}"', { oldName }))
90+
} finally {
91+
Vue.set(node, 'status', undefined)
92+
}
93+
},
94+
},
1695
})
1796

1897
const renamingStore = store(...args)

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

Lines changed: 29 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@
44
*/
55

66
import type { User } from '@nextcloud/cypress'
7-
import { getRowForFile, haveValidity, triggerActionForFile } from './FilesUtils'
7+
import { getRowForFile, haveValidity, renameFile, triggerActionForFile } from './FilesUtils'
88

99
describe('files: Rename nodes', { testIsolation: true }, () => {
1010
let user: User
@@ -115,4 +115,32 @@ describe('files: Rename nodes', { testIsolation: true }, () => {
115115
.findByRole('img', { name: 'File is loading' })
116116
.should('not.exist')
117117
})
118+
119+
/**
120+
* This is a regression test of: https://github.com/nextcloud/server/issues/47438
121+
* 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
122+
* 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).
123+
*/
124+
it('correctly resets renaming state', () => {
125+
for (let i = 1; i <= 20; i++) {
126+
cy.uploadContent(user, new Blob([]), 'text/plain', `/file${i}.txt`)
127+
}
128+
cy.viewport(1200, 500) // 500px is smaller then 20 * 50 which is the place that the files take up
129+
cy.login(user)
130+
cy.visit('/apps/files')
131+
132+
getRowForFile('file.txt').should('be.visible')
133+
// Z so it is shown last
134+
renameFile('file.txt', 'zzz.txt')
135+
// not visible any longer
136+
getRowForFile('zzz.txt').should('not.be.visible')
137+
// scroll file list to bottom
138+
cy.get('[data-cy-files-list]').scrollTo('bottom')
139+
cy.screenshot()
140+
// The file is no longer in rename state
141+
getRowForFile('zzz.txt')
142+
.should('be.visible')
143+
.findByRole('textbox', { name: 'Filename' })
144+
.should('not.exist')
145+
})
118146
})

0 commit comments

Comments
 (0)