fix(files): improve handling of copy-move action

1. only show 1 loading toast instead of N for N files in batch
   operation.
2. Reuse more code to reduce duplicated logic.
3. Show the conflict picker once for all files instead of opening a new
   conflict picker for every file to copy / move.

Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>
This commit is contained in:
Ferdinand Thiessen 2026-02-02 13:23:06 +01:00
parent 9cfaf9b3b3
commit 904b946611
No known key found for this signature in database
GPG key ID: 7E849AE05218500F
4 changed files with 234 additions and 270 deletions

View file

@ -11,18 +11,214 @@ import type { MoveCopyResult } from './moveOrCopyActionUtils.ts'
import FolderMoveSvg from '@mdi/svg/svg/folder-move-outline.svg?raw'
import CopyIconSvg from '@mdi/svg/svg/folder-multiple-outline.svg?raw'
import { isAxiosError } from '@nextcloud/axios'
import { FilePickerClosed, getFilePickerBuilder, showError, showInfo, TOAST_PERMANENT_TIMEOUT } from '@nextcloud/dialogs'
import { FilePickerClosed, getFilePickerBuilder, openConflictPicker, showError, showLoading } from '@nextcloud/dialogs'
import { emit } from '@nextcloud/event-bus'
import { FileAction, FileType, getUniqueName, NodeStatus, Permission } from '@nextcloud/files'
import { defaultRootPath, getClient, getDefaultPropfind, resultToNode } from '@nextcloud/files/dav'
import { t } from '@nextcloud/l10n'
import { hasConflict, openConflictPicker } from '@nextcloud/upload'
import { getConflicts } from '@nextcloud/upload'
import { basename, join } from 'path'
import Vue from 'vue'
import logger from '../logger.ts'
import { getContents } from '../services/Files.ts'
import { canCopy, canMove, getQueue, MoveCopyAction } from './moveOrCopyActionUtils.ts'
/**
* Exception to hint the user about something.
* The message is intended to be shown to the user.
*/
export class HintException extends Error {}
export const ACTION_COPY_MOVE = 'move-copy'
export const action = new FileAction({
id: ACTION_COPY_MOVE,
order: 15,
displayName({ nodes }) {
switch (getActionForNodes(nodes)) {
case MoveCopyAction.MOVE:
return t('files', 'Move')
case MoveCopyAction.COPY:
return t('files', 'Copy')
case MoveCopyAction.MOVE_OR_COPY:
return t('files', 'Move or copy')
}
},
iconSvgInline: () => FolderMoveSvg,
enabled({ nodes, view }): boolean {
// We can not copy or move in single file shares
if (view.id === 'public-file-share') {
return false
}
// We only support moving/copying files within the user folder
if (!nodes.every((node) => node.root?.startsWith('/files/'))) {
return false
}
return nodes.length > 0 && (canMove(nodes) || canCopy(nodes))
},
async exec(context) {
return this.execBatch!(context)[0]
},
async execBatch({ nodes, folder }) {
const action = getActionForNodes(nodes)
const target = await openFilePickerForAction(action, folder.path, nodes)
// Handle cancellation silently
if (target === false) {
return nodes.map(() => null)
}
try {
const result = await Array.fromAsync(handleCopyMoveNodesTo(nodes, target.destination, target.action))
return result.map(() => true)
} catch (error) {
logger.error(`Failed to ${target.action} node`, { nodes, error })
if (error instanceof HintException && !!error.message) {
showError(error.message)
// Silent action as we handle the toast
return nodes.map(() => null)
}
// We need to keep the selection on error!
// So we do not return null, and for batch action
return nodes.map(() => false)
}
},
})
/**
* Handle the copy/move of a node to a destination
* This can be imported and used by other scripts/components on server
*
* @param nodes The nodes to copy/move
* @param destination The destination to copy/move the nodes to
* @param method The method to use for the copy/move
* @param overwrite Whether to overwrite the destination if it exists
* @yields {AsyncGenerator<void, void, never>} A promise that resolves when the copy/move is done
*/
export async function* handleCopyMoveNodesTo(nodes: INode[], destination: IFolder, method: MoveCopyAction.COPY | MoveCopyAction.MOVE, overwrite = false): AsyncGenerator<void, void, never> {
if (!destination) {
return
}
if (destination.type !== FileType.Folder) {
throw new Error(t('files', 'Destination is not a folder'))
}
// Do not allow to MOVE a node to the same folder it is already located
if (method === MoveCopyAction.MOVE && nodes.some((node) => node.dirname === destination.path)) {
throw new Error(t('files', 'This file/folder is already in that directory'))
}
/**
* Example:
* - node: /foo/bar/file.txt -> path = /foo/bar/file.txt, destination: /foo
* Allow move of /foo does not start with /foo/bar/file.txt so allow
* - node: /foo , destination: /foo/bar
* Do not allow as it would copy foo within itself
* - node: /foo/bar.txt, destination: /foo
* Allow copy a file to the same directory
* - node: "/foo/bar", destination: "/foo/bar 1"
* Allow to move or copy but we need to check with trailing / otherwise it would report false positive
*/
if (nodes.some((node) => `${destination.path}/`.startsWith(`${node.path}/`))) {
throw new Error(t('files', 'You cannot move a file/folder onto itself or into a subfolder of itself'))
}
const nameMapping = new Map<string, string>()
// Check for conflicts if we do not want to overwrite
if (!overwrite) {
const otherNodes = (await getContents(destination.path)).contents
const conflicts = getConflicts(nodes, otherNodes) as unknown as INode[]
const nodesToRename: INode[] = []
if (conflicts.length > 0) {
if (method === MoveCopyAction.MOVE) {
// Let the user choose what to do with the conflicting files
const content = otherNodes.filter((n) => conflicts.some((c) => c.basename === n.basename))
const result = await openConflictPicker(destination.path, conflicts, content)
if (!result) {
// User cancelled
return
}
nodes = nodes.filter((n) => !result.skipped.includes(n as never))
nodesToRename.push(...(result.renamed as unknown as INode[]))
} else {
// for COPY we always rename conflicting files
nodesToRename.push(...conflicts)
}
const usedNames = [...otherNodes, ...nodes.filter((n) => !conflicts.includes(n))].map((n) => n.basename)
for (const node of nodesToRename) {
const newName = getUniqueName(node.basename, usedNames, { ignoreFileExtension: node.type === FileType.Folder })
nameMapping.set(node.source, newName)
usedNames.push(newName) // add the new name to avoid duplicates for following re-namimgs
}
}
}
const actionFinished = createLoadingNotification(method, nodes.map((node) => node.basename), destination.path)
const queue = getQueue()
try {
for (const node of nodes) {
// Set loading state
Vue.set(node, 'status', NodeStatus.LOADING)
yield queue.add(async () => {
try {
const client = getClient()
const currentPath = join(defaultRootPath, node.path)
const destinationPath = join(defaultRootPath, destination.path, nameMapping.get(node.source) ?? node.basename)
if (method === MoveCopyAction.COPY) {
await client.copyFile(currentPath, destinationPath)
// If the node is copied into current directory the view needs to be updated
if (node.dirname === destination.path) {
const { data } = await client.stat(
destinationPath,
{
details: true,
data: getDefaultPropfind(),
},
) as ResponseDataDetailed<FileStat>
emit('files:node:created', resultToNode(data))
}
} else {
await client.moveFile(currentPath, destinationPath)
// Delete the node as it will be fetched again
// when navigating to the destination folder
emit('files:node:deleted', node)
}
} catch (error) {
logger.debug(`Error while trying to ${method === MoveCopyAction.COPY ? 'copy' : 'move'} node`, { node, error })
if (isAxiosError(error)) {
if (error.response?.status === 412) {
throw new HintException(t('files', 'A file or folder with that name already exists in this folder'))
} else if (error.response?.status === 423) {
throw new HintException(t('files', 'The files are locked'))
} else if (error.response?.status === 404) {
throw new HintException(t('files', 'The file does not exist anymore'))
} else if ('response' in error && error.response) {
const parser = new DOMParser()
const text = await (error as WebDAVClientError).response!.text()
const message = parser.parseFromString(text ?? '', 'text/xml')
.querySelector('message')?.textContent
if (message) {
throw new HintException(message)
}
}
}
throw error
} finally {
Vue.set(node, 'status', undefined)
}
})
}
} finally {
actionFinished()
}
}
/**
* Return the action that is possible for the given nodes
*
@ -45,168 +241,25 @@ function getActionForNodes(nodes: INode[]): MoveCopyAction {
* Create a loading notification toast
*
* @param mode The move or copy mode
* @param source Name of the node that is copied / moved
* @param sources Names of the nodes that are copied / moved
* @param destination Destination path
* @return Function to hide the notification
*/
function createLoadingNotification(mode: MoveCopyAction, source: string, destination: string): () => void {
const text = mode === MoveCopyAction.MOVE ? t('files', 'Moving "{source}" to "{destination}" …', { source, destination }) : t('files', 'Copying "{source}" to "{destination}" …', { source, destination })
function createLoadingNotification(mode: MoveCopyAction, sources: string[], destination: string): () => void {
const text = mode === MoveCopyAction.MOVE
? (sources.length === 1
? t('files', 'Moving "{source}" to "{destination}" …', { source: sources[0], destination })
: t('files', 'Moving {count} files to "{destination}" …', { count: sources.length, destination })
)
: (sources.length === 1
? t('files', 'Copying "{source}" to "{destination}" …', { source: sources[0], destination })
: t('files', 'Copying {count} files to "{destination}" …', { count: sources.length, destination })
)
let toast: ReturnType<typeof showInfo> | undefined
toast = showInfo(
`<span class="icon icon-loading-small toast-loading-icon"></span> ${text}`,
{
isHTML: true,
timeout: TOAST_PERMANENT_TIMEOUT,
onRemove() {
toast?.hideToast()
toast = undefined
},
},
)
const toast = showLoading(text)
return () => toast && toast.hideToast()
}
/**
* Handle the copy/move of a node to a destination
* This can be imported and used by other scripts/components on server
*
* @param node The node to copy/move
* @param destination The destination to copy/move the node to
* @param method The method to use for the copy/move
* @param overwrite Whether to overwrite the destination if it exists
* @return A promise that resolves when the copy/move is done
*/
export async function handleCopyMoveNodeTo(node: INode, destination: IFolder, method: MoveCopyAction.COPY | MoveCopyAction.MOVE, overwrite = false) {
if (!destination) {
return
}
if (destination.type !== FileType.Folder) {
throw new Error(t('files', 'Destination is not a folder'))
}
// Do not allow to MOVE a node to the same folder it is already located
if (method === MoveCopyAction.MOVE && node.dirname === destination.path) {
throw new Error(t('files', 'This file/folder is already in that directory'))
}
/**
* Example:
* - node: /foo/bar/file.txt -> path = /foo/bar/file.txt, destination: /foo
* Allow move of /foo does not start with /foo/bar/file.txt so allow
* - node: /foo , destination: /foo/bar
* Do not allow as it would copy foo within itself
* - node: /foo/bar.txt, destination: /foo
* Allow copy a file to the same directory
* - node: "/foo/bar", destination: "/foo/bar 1"
* Allow to move or copy but we need to check with trailing / otherwise it would report false positive
*/
if (`${destination.path}/`.startsWith(`${node.path}/`)) {
throw new Error(t('files', 'You cannot move a file/folder onto itself or into a subfolder of itself'))
}
// Set loading state
Vue.set(node, 'status', NodeStatus.LOADING)
const actionFinished = createLoadingNotification(method, node.basename, destination.path)
const queue = getQueue()
return await queue.add(async () => {
const copySuffix = (index: number) => {
if (index === 1) {
return t('files', '(copy)') // TRANSLATORS: Mark a file as a copy of another file
}
return t('files', '(copy %n)', undefined, index) // TRANSLATORS: Meaning it is the n'th copy of a file
}
try {
const client = getClient()
const currentPath = join(defaultRootPath, node.path)
const destinationPath = join(defaultRootPath, destination.path)
if (method === MoveCopyAction.COPY) {
let target = node.basename
// If we do not allow overwriting then find an unique name
if (!overwrite) {
const otherNodes = await client.getDirectoryContents(destinationPath) as FileStat[]
target = getUniqueName(
node.basename,
otherNodes.map((n) => n.basename),
{
suffix: copySuffix,
ignoreFileExtension: node.type === FileType.Folder,
},
)
}
await client.copyFile(currentPath, join(destinationPath, target))
// If the node is copied into current directory the view needs to be updated
if (node.dirname === destination.path) {
const { data } = await client.stat(
join(destinationPath, target),
{
details: true,
data: getDefaultPropfind(),
},
) as ResponseDataDetailed<FileStat>
emit('files:node:created', resultToNode(data))
}
} else {
// show conflict file popup if we do not allow overwriting
if (!overwrite) {
const otherNodes = await getContents(destination.path)
if (hasConflict([node], otherNodes.contents)) {
try {
// Let the user choose what to do with the conflicting files
const { selected, renamed } = await openConflictPicker(destination.path, [node], otherNodes.contents)
// two empty arrays: either only old files or conflict skipped -> no action required
if (!selected.length && !renamed.length) {
return
}
} catch {
// User cancelled
return
}
}
}
// getting here means either no conflict, file was renamed to keep both files
// in a conflict, or the selected file was chosen to be kept during the conflict
try {
await client.moveFile(currentPath, join(destinationPath, node.basename))
} catch (error) {
const parser = new DOMParser()
const text = await (error as WebDAVClientError).response?.text()
const message = parser.parseFromString(text ?? '', 'text/xml')
.querySelector('message')?.textContent
if (message) {
showError(message)
}
throw error
}
// Delete the node as it will be fetched again
// when navigating to the destination folder
emit('files:node:deleted', node)
}
} catch (error) {
if (isAxiosError(error)) {
if (error.response?.status === 412) {
throw new Error(t('files', 'A file or folder with that name already exists in this folder'))
} else if (error.response?.status === 423) {
throw new Error(t('files', 'The files are locked'))
} else if (error.response?.status === 404) {
throw new Error(t('files', 'The file does not exist anymore'))
} else if (error.message) {
throw new Error(error.message)
}
}
logger.debug(error as Error)
throw new Error()
} finally {
Vue.set(node, 'status', '')
actionFinished()
}
})
}
/**
* Open a file picker for the given action
*
@ -302,82 +355,3 @@ async function openFilePickerForAction(
return promise
}
export const ACTION_COPY_MOVE = 'move-copy'
export const action = new FileAction({
id: ACTION_COPY_MOVE,
displayName({ nodes }) {
switch (getActionForNodes(nodes)) {
case MoveCopyAction.MOVE:
return t('files', 'Move')
case MoveCopyAction.COPY:
return t('files', 'Copy')
case MoveCopyAction.MOVE_OR_COPY:
return t('files', 'Move or copy')
}
},
iconSvgInline: () => FolderMoveSvg,
enabled({ nodes, view }): boolean {
// We can not copy or move in single file shares
if (view.id === 'public-file-share') {
return false
}
// We only support moving/copying files within the user folder
if (!nodes.every((node) => node.root?.startsWith('/files/'))) {
return false
}
return nodes.length > 0 && (canMove(nodes) || canCopy(nodes))
},
async exec({ nodes, folder }) {
const action = getActionForNodes([nodes[0]])
let result
try {
result = await openFilePickerForAction(action, folder.path, [nodes[0]])
} catch (e) {
logger.error(e as Error)
return false
}
if (result === false) {
return null
}
try {
await handleCopyMoveNodeTo(nodes[0], result.destination, result.action)
return true
} catch (error) {
if (error instanceof Error && !!error.message) {
showError(error.message)
// Silent action as we handle the toast
return null
}
return false
}
},
async execBatch({ nodes, folder }) {
const action = getActionForNodes(nodes)
const result = await openFilePickerForAction(action, folder.path, nodes)
// Handle cancellation silently
if (result === false) {
return nodes.map(() => null)
}
const promises = nodes.map(async (node) => {
try {
await handleCopyMoveNodeTo(node, result.destination, result.action)
return true
} catch (error) {
logger.error(`Failed to ${result.action} node`, { node, error })
return false
}
})
// We need to keep the selection on error!
// So we do not return null, and for batch action
// we let the front handle the error.
return await Promise.all(promises)
},
order: 15,
})

View file

@ -8,12 +8,10 @@ import type { Upload } from '@nextcloud/upload'
import type { RootDirectory } from './DropServiceUtils.ts'
import { showError, showInfo, showSuccess, showWarning } from '@nextcloud/dialogs'
import { NodeStatus } from '@nextcloud/files'
import { t } from '@nextcloud/l10n'
import { join } from '@nextcloud/paths'
import { getUploader, hasConflict } from '@nextcloud/upload'
import Vue from 'vue'
import { handleCopyMoveNodeTo } from '../actions/moveOrCopyAction.ts'
import { handleCopyMoveNodesTo, HintException } from '../actions/moveOrCopyAction.ts'
import { MoveCopyAction } from '../actions/moveOrCopyActionUtils.ts'
import logger from '../logger.ts'
import { createDirectoryIfNotExists, Directory, resolveConflict, traverseTree } from './DropServiceUtils.ts'
@ -178,8 +176,6 @@ export async function onDropExternalFiles(root: RootDirectory, destination: IFol
* @param isCopy - Whether the operation is a copy
*/
export async function onDropInternalFiles(nodes: INode[], destination: IFolder, contents: INode[], isCopy = false) {
const queue = [] as Promise<void>[]
// Check for conflicts on root elements
if (await hasConflict(nodes, contents)) {
nodes = await resolveConflict(nodes, destination, contents)
@ -191,23 +187,17 @@ export async function onDropInternalFiles(nodes: INode[], destination: IFolder,
return
}
for (const node of nodes) {
Vue.set(node, 'status', NodeStatus.LOADING)
queue.push(handleCopyMoveNodeTo(node, destination, isCopy ? MoveCopyAction.COPY : MoveCopyAction.MOVE, true))
try {
const promises = Array.fromAsync(handleCopyMoveNodesTo(nodes, destination, isCopy ? MoveCopyAction.COPY : MoveCopyAction.MOVE))
await promises
logger.debug('Files copy/move successful')
showSuccess(isCopy ? t('files', 'Files copied successfully') : t('files', 'Files moved successfully'))
} catch (error) {
logger.error('Error while processing dropped files', { error })
if (error instanceof HintException) {
showError(error.message)
} else {
showError(isCopy ? t('files', 'Some files could not be copied') : t('files', 'Some files could not be moved'))
}
}
// Wait for all promises to settle
const results = await Promise.allSettled(queue)
nodes.forEach((node) => Vue.set(node, 'status', undefined))
// Check for errors
const errors = results.filter((result) => result.status === 'rejected')
if (errors.length > 0) {
logger.error('Error while copying or moving files', { errors })
showError(isCopy ? t('files', 'Some files could not be copied') : t('files', 'Some files could not be moved'))
return
}
logger.debug('Files copy/move successful')
showSuccess(isCopy ? t('files', 'Files copied successfully') : t('files', 'Files moved successfully'))
}

View file

@ -107,23 +107,23 @@ describe('Files: Move or copy files', { testIsolation: true }, () => {
copyFile('original.txt', '.')
getRowForFile('original.txt').should('be.visible')
getRowForFile('original (copy).txt').should('be.visible')
getRowForFile('original (1).txt').should('be.visible')
})
it('Can copy a file multiple times to same folder', () => {
cy.uploadContent(currentUser, new Blob(), 'text/plain', '/original.txt')
cy.uploadContent(currentUser, new Blob(), 'text/plain', '/original (copy).txt')
cy.uploadContent(currentUser, new Blob(), 'text/plain', '/original (1).txt')
cy.login(currentUser)
cy.visit('/apps/files')
copyFile('original.txt', '.')
getRowForFile('original.txt').should('be.visible')
getRowForFile('original (copy 2).txt').should('be.visible')
getRowForFile('original (2).txt').should('be.visible')
})
/**
* Test that a copied folder with a dot will be renamed correctly ('foo.bar' -> 'foo.bar (copy)')
* Test that a copied folder with a dot will be renamed correctly ('foo.bar' -> 'foo.bar (1)')
* Test for: https://github.com/nextcloud/server/issues/43843
*/
it('Can copy a folder to same folder', () => {
@ -134,7 +134,7 @@ describe('Files: Move or copy files', { testIsolation: true }, () => {
copyFile('foo.bar', '.')
getRowForFile('foo.bar').should('be.visible')
getRowForFile('foo.bar (copy)').should('be.visible')
getRowForFile('foo.bar (1)').should('be.visible')
})
/** Test for https://github.com/nextcloud/server/issues/43329 */

View file

@ -56,8 +56,8 @@ describe('Files: Live photos', { testIsolation: true }, () => {
getRowForFile(`${randomFileName}.jpg`).should('have.length', 1)
getRowForFile(`${randomFileName}.mov`).should('have.length', 1)
getRowForFile(`${randomFileName} (copy).jpg`).should('have.length', 1)
getRowForFile(`${randomFileName} (copy).mov`).should('have.length', 1)
getRowForFile(`${randomFileName} (1).jpg`).should('have.length', 1)
getRowForFile(`${randomFileName} (1).mov`).should('have.length', 1)
})
it('Copies both files when copying the .mov', () => {
@ -65,15 +65,15 @@ describe('Files: Live photos', { testIsolation: true }, () => {
reloadCurrentFolder()
getRowForFile(`${randomFileName}.mov`).should('have.length', 1)
getRowForFile(`${randomFileName} (copy).jpg`).should('have.length', 1)
getRowForFile(`${randomFileName} (copy).mov`).should('have.length', 1)
getRowForFile(`${randomFileName} (1).jpg`).should('have.length', 1)
getRowForFile(`${randomFileName} (1).mov`).should('have.length', 1)
})
it('Keeps live photo link when copying folder', () => {
createFolder('folder')
moveFile(`${randomFileName}.jpg`, 'folder')
copyFile('folder', '.')
navigateToFolder('folder (copy)')
navigateToFolder('folder (1)')
getRowForFile(`${randomFileName}.jpg`).should('have.length', 1)
getRowForFile(`${randomFileName}.mov`).should('have.length', 1)
@ -95,7 +95,7 @@ describe('Files: Live photos', { testIsolation: true }, () => {
cy.get('[data-cy-files-list-row-fileid]').should('have.length', 1)
getRowForFile(`${randomFileName}.mov`).should('have.length', 1)
getRowForFile(`${randomFileName}.jpg`).should('have.length', 0)
getRowForFile(`${randomFileName} (copy).jpg`).should('have.length', 0)
getRowForFile(`${randomFileName} (1).jpg`).should('have.length', 0)
})
it('Moves files when moving the .jpg', () => {