Merge pull request #47905 from nextcloud/fix/files-duplicated-nodes

fix(files): Ensure children are removed from folder and not duplicated

apps/files/src/store/paths.spec.ts

@@ -0,0 +1,130 @@
+/**
+ * SPDX-FileCopyrightText: 2024 Nextcloud GmbH and Nextcloud contributors
+ * SPDX-License-Identifier: AGPL-3.0-or-later
+ */
+
+import { describe, beforeEach, test, expect } from 'vitest'
+import { setActivePinia, createPinia } from 'pinia'
+import { usePathsStore } from './paths.ts'
+import { emit } from '@nextcloud/event-bus'
+import { File, Folder } from '@nextcloud/files'
+import { useFilesStore } from './files.ts'
+
+describe('Path store', () => {
+
+	let store: ReturnType<typeof usePathsStore>
+	let files: ReturnType<typeof useFilesStore>
+	let root: Folder & { _children?: string[] }
+
+	beforeEach(() => {
+		setActivePinia(createPinia())
+
+		root = new Folder({ owner: 'test', source: 'http://example.com/remote.php/dav/files/test/', id: 1 })
+		files = useFilesStore()
+		files.setRoot({ service: 'files', root })
+
+		store = usePathsStore()
+	})
+
+	test('Folder is created', () => {
+		// no defined paths
+		expect(store.paths).toEqual({})
+
+		// create the folder
+		const node = new Folder({ owner: 'test', source: 'http://example.com/remote.php/dav/files/test/folder', id: 2 })
+		emit('files:node:created', node)
+
+		// see that the path is added
+		expect(store.paths).toEqual({ files: { [node.path]: node.source } })
+
+		// see that the node is added
+		expect(root._children).toEqual([node.source])
+	})
+
+	test('File is created', () => {
+		// no defined paths
+		expect(store.paths).toEqual({})
+
+		// create the file
+		const node = new File({ owner: 'test', source: 'http://example.com/remote.php/dav/files/test/file.txt', id: 2, mime: 'text/plain' })
+		emit('files:node:created', node)
+
+		// see that there are still no paths
+		expect(store.paths).toEqual({})
+
+		// see that the node is added
+		expect(root._children).toEqual([node.source])
+	})
+
+	test('Existing file is created', () => {
+		// no defined paths
+		expect(store.paths).toEqual({})
+
+		// create the file
+		const node1 = new File({ owner: 'test', source: 'http://example.com/remote.php/dav/files/test/file.txt', id: 2, mime: 'text/plain' })
+		emit('files:node:created', node1)
+
+		// see that there are still no paths
+		expect(store.paths).toEqual({})
+
+		// see that the node is added
+		expect(root._children).toEqual([node1.source])
+
+		// create the same named file again
+		const node2 = new File({ owner: 'test', source: 'http://example.com/remote.php/dav/files/test/file.txt', id: 2, mime: 'text/plain' })
+		emit('files:node:created', node2)
+
+		// see that there are still no paths and the children are not duplicated
+		expect(store.paths).toEqual({})
+		expect(root._children).toEqual([node1.source])
+
+	})
+
+	test('Existing folder is created', () => {
+		// no defined paths
+		expect(store.paths).toEqual({})
+
+		// create the file
+		const node1 = new Folder({ owner: 'test', source: 'http://example.com/remote.php/dav/files/test/folder', id: 2 })
+		emit('files:node:created', node1)
+
+		// see the path is added
+		expect(store.paths).toEqual({ files: { [node1.path]: node1.source } })
+
+		// see that the node is added
+		expect(root._children).toEqual([node1.source])
+
+		// create the same named file again
+		const node2 = new Folder({ owner: 'test', source: 'http://example.com/remote.php/dav/files/test/folder', id: 2 })
+		emit('files:node:created', node2)
+
+		// see that there is still only one paths and the children are not duplicated
+		expect(store.paths).toEqual({ files: { [node1.path]: node1.source } })
+		expect(root._children).toEqual([node1.source])
+	})
+
+	test('Folder is deleted', () => {
+		const node = new Folder({ owner: 'test', source: 'http://example.com/remote.php/dav/files/test/folder', id: 2 })
+		emit('files:node:created', node)
+		// see that the path is added and the children are set-up
+		expect(store.paths).toEqual({ files: { [node.path]: node.source } })
+		expect(root._children).toEqual([node.source])
+
+		emit('files:node:deleted', node)
+		// See the path is removed
+		expect(store.paths).toEqual({ files: {} })
+		// See the child is removed
+		expect(root._children).toEqual([])
+	})
+
+	test('File is deleted', () => {
+		const node = new File({ owner: 'test', source: 'http://example.com/remote.php/dav/files/test/file.txt', id: 2, mime: 'text/plain' })
+		emit('files:node:created', node)
+		// see that the children are set-up
+		expect(root._children).toEqual([node.source])
+
+		emit('files:node:deleted', node)
+		// See the child is removed
+		expect(root._children).toEqual([])
+	})
+})

apps/files/src/store/paths.ts

@@ -2,7 +2,7 @@
  * SPDX-FileCopyrightText: 2023 Nextcloud GmbH and Nextcloud contributors
  * SPDX-License-Identifier: AGPL-3.0-or-later
  */
-import type { FileSource, PathsStore, PathOptions, ServicesState } from '../types'
+import type { FileSource, PathsStore, PathOptions, ServicesState, Service } from '../types'
 import { defineStore } from 'pinia'
 import { FileType, Folder, Node, getNavigation } from '@nextcloud/files'
 import { subscribe } from '@nextcloud/event-bus'
@@ -41,6 +41,57 @@ export const usePathsStore = function(...args) {
 				Vue.set(this.paths[payload.service], payload.path, payload.source)
 			},
 
+			deletePath(service: Service, path: string) {
+				// skip if service does not exist
+				if (!this.paths[service]) {
+					return
+				}
+
+				Vue.delete(this.paths[service], path)
+			},
+
+			onDeletedNode(node: Node) {
+				const service = getNavigation()?.active?.id || 'files'
+
+				if (node.type === FileType.Folder) {
+					// Delete the path
+					this.deletePath(
+						service,
+						node.path,
+					)
+				}
+
+				// Remove node from children
+				if (node.dirname === '/') {
+					const root = files.getRoot(service) as Folder & { _children?: string[] }
+					// ensure sources are unique
+					const children = new Set(root._children ?? [])
+					children.delete(node.source)
+					Vue.set(root, '_children', [...children.values()])
+					return
+				}
+
+				if (this.paths[service][node.dirname]) {
+					const parentSource = this.paths[service][node.dirname]
+					const parentFolder = files.getNode(parentSource) as Folder & { _children?: string[] }
+
+					if (!parentFolder) {
+						logger.error('Parent folder not found', { parentSource })
+						return
+					}
+
+					logger.debug('Path exists, removing from children', { parentFolder, node })
+
+					// ensure sources are unique
+					const children = new Set(parentFolder._children ?? [])
+					children.delete(node.source)
+					Vue.set(parentFolder, '_children', [...children.values()])
+					return
+				}
+
+				logger.debug('Parent path does not exists, skipping children update', { node })
+			},
+
 			onCreatedNode(node: Node) {
 				const service = getNavigation()?.active?.id || 'files'
 				if (!node.fileid) {
@@ -60,30 +111,30 @@ export const usePathsStore = function(...args) {
 				// Update parent folder children if exists
 				// If the folder is the root, get it and update it
 				if (node.dirname === '/') {
-					const root = files.getRoot(service)
-					if (!root._children) {
-						Vue.set(root, '_children', [])
-					}
-					root._children.push(node.source)
+					const root = files.getRoot(service) as Folder & { _children?: string[] }
+					// ensure sources are unique
+					const children = new Set(root._children ?? [])
+					children.add(node.source)
+					Vue.set(root, '_children', [...children.values()])
 					return
 				}
 
 				// If the folder doesn't exists yet, it will be
 				// fetched later and its children updated anyway.
 				if (this.paths[service][node.dirname]) {
 					const parentSource = this.paths[service][node.dirname]
-					const parentFolder = files.getNode(parentSource) as Folder
+					const parentFolder = files.getNode(parentSource) as Folder & { _children?: string[] }
 					logger.debug('Path already exists, updating children', { parentFolder, node })
 
 					if (!parentFolder) {
 						logger.error('Parent folder not found', { parentSource })
 						return
 					}
 
-					if (!parentFolder._children) {
-						Vue.set(parentFolder, '_children', [])
-					}
-					parentFolder._children.push(node.source)
+					// ensure sources are unique
+					const children = new Set(parentFolder._children ?? [])
+					children.add(node.source)
+					Vue.set(parentFolder, '_children', [...children.values()])
 					return
 				}
 
@@ -97,7 +148,7 @@ export const usePathsStore = function(...args) {
 	if (!pathsStore._initialized) {
 		// TODO: watch folders to update paths?
 		subscribe('files:node:created', pathsStore.onCreatedNode)
-		// subscribe('files:node:deleted', pathsStore.onDeletedNode)
+		subscribe('files:node:deleted', pathsStore.onDeletedNode)
 		// subscribe('files:node:moved', pathsStore.onMovedNode)
 
 		pathsStore._initialized = true

apps/files/src/views/FilesList.vue

@@ -658,7 +658,7 @@ export default defineComponent({
 		},
 
 		filterDirContent() {
-			let nodes = this.dirContents
+			let nodes: INode[] = this.dirContents
 			for (const filter of this.filtersStore.sortedFilters) {
 				nodes = filter.filter(nodes)
 			}

cypress/e2e/files/duplicated-node-regression.cy.ts

@@ -0,0 +1,33 @@
+/**
+ * SPDX-FileCopyrightText: 2024 Nextcloud GmbH and Nextcloud contributors
+ * SPDX-License-Identifier: AGPL-3.0-or-later
+ */
+
+import { createFolder, getRowForFile, triggerActionForFile } from './FilesUtils.ts'
+
+before(() => {
+	cy.createRandomUser()
+		.then((user) => {
+			cy.mkdir(user, '/only once')
+			cy.login(user)
+			cy.visit('/apps/files')
+		})
+})
+
+/**
+ * Regression test for https://github.com/nextcloud/server/issues/47904
+ */
+it('Ensure nodes are not duplicated in the file list', () => {
+	// See the folder
+	getRowForFile('only once').should('be.visible')
+	// Delete the folder
+	cy.intercept('DELETE', '**/remote.php/dav/**').as('deleteFolder')
+	triggerActionForFile('only once', 'delete')
+	cy.wait('@deleteFolder')
+	getRowForFile('only once').should('not.exist')
+	// Create the folder again
+	createFolder('only once')
+	// See folder exists only once
+	getRowForFile('only once')
+		.should('have.length', 1)
+})