From 80164d686366a95cc424d807d054bd7dc1107f03 Mon Sep 17 00:00:00 2001 From: Tom Date: Sun, 12 Jul 2026 00:07:16 +0100 Subject: [PATCH] feat(file-explorer): confirm multi-select deletes with a single batch prompt (#7459) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * feat(file-explorer): confirm multi-select deletes with a single batch prompt Deleting a multi-file selection on a remote host prompted once per file: requestDeleteAll awaited runDelete per root, and the remote-delete confirmation lived inside runDelete. Hoist the confirmation for batches — one 'Permanently delete {{count}} items?' dialog up front, then per-node deletes with skipConfirmation. Single-file and local (Trash) deletes are unchanged. Root filtering and the batch loop move to file-explorer-batch-deletion.ts with unit coverage. Fixes #7457 * fix(file-explorer): count the full selection in the batch delete prompt Address review: roots.length undercounts when a selected directory's children are also selected (could even read 'delete 1 items?'). Use the visible selection size instead. Also reword the zh strings so 项目 cannot read as 'project' in a file-delete flow. * review: skip batch delete confirm for unresolved-owner selections Narrow the batch-confirm gate to a resolvable remote route so an unresolved-owner multi-select no longer pops a destructive prompt for deletes that fail closed anyway — mirroring the single-delete path. Co-authored-by: Orca --------- Co-authored-by: Tom de Bres Co-authored-by: Jinjing <6427696+AmethystLiang@users.noreply.github.com> Co-authored-by: Orca --- .../file-explorer-batch-deletion.test.ts | 106 ++++++++++++++++++ .../file-explorer-batch-deletion.ts | 45 ++++++++ .../right-sidebar/useFileDeletion.ts | 73 +++++++----- src/renderer/src/i18n/locales/en.json | 4 +- src/renderer/src/i18n/locales/es.json | 4 +- src/renderer/src/i18n/locales/ja.json | 4 +- src/renderer/src/i18n/locales/ko.json | 4 +- src/renderer/src/i18n/locales/zh.json | 4 +- ...e-explorer-delete-owner-provenance.test.ts | 37 ++++++ 9 files changed, 250 insertions(+), 31 deletions(-) create mode 100644 src/renderer/src/components/right-sidebar/file-explorer-batch-deletion.test.ts create mode 100644 src/renderer/src/components/right-sidebar/file-explorer-batch-deletion.ts diff --git a/src/renderer/src/components/right-sidebar/file-explorer-batch-deletion.test.ts b/src/renderer/src/components/right-sidebar/file-explorer-batch-deletion.test.ts new file mode 100644 index 000000000..e93928bf4 --- /dev/null +++ b/src/renderer/src/components/right-sidebar/file-explorer-batch-deletion.test.ts @@ -0,0 +1,106 @@ +import { describe, expect, it, vi } from 'vitest' +import { runBatchDeletion, selectDeletionRoots } from './file-explorer-batch-deletion' +import type { TreeNode } from './file-explorer-types' + +function node(path: string, isDirectory = false): TreeNode { + return { + name: path.split('/').pop() ?? path, + path, + relativePath: path.replace(/^\//, ''), + isDirectory, + depth: 0 + } +} + +describe('selectDeletionRoots', () => { + it('keeps unrelated files and directories', () => { + const nodes = [node('/repo/a.ts'), node('/repo/b.ts'), node('/repo/docs', true)] + expect(selectDeletionRoots(nodes)).toEqual(nodes) + }) + + it('drops children of a selected directory', () => { + const dir = node('/repo/docs', true) + const child = node('/repo/docs/readme.md') + const nested = node('/repo/docs/guides/intro.md') + const outside = node('/repo/a.ts') + expect(selectDeletionRoots([dir, child, nested, outside])).toEqual([dir, outside]) + }) + + it('keeps siblings whose paths merely share a prefix', () => { + const dir = node('/repo/docs', true) + const lookalike = node('/repo/docs-old/readme.md') + expect(selectDeletionRoots([dir, lookalike])).toEqual([dir, lookalike]) + }) + + it('does not treat selected files as containers', () => { + const file = node('/repo/a.ts') + const other = node('/repo/a.ts/impossible-child') + expect(selectDeletionRoots([file, other])).toEqual([file, other]) + }) +}) + +describe('runBatchDeletion', () => { + const roots = [node('/repo/a.ts'), node('/repo/b.ts'), node('/repo/c.ts')] + + it('asks for confirmation once and deletes every root in order', async () => { + const confirmBatch = vi.fn().mockResolvedValue(true) + const order: string[] = [] + const deleteNode = vi.fn(async (n: TreeNode) => { + order.push(n.path) + return true + }) + + const deleted = await runBatchDeletion({ + roots, + needsConfirmation: true, + confirmBatch, + deleteNode + }) + + expect(confirmBatch).toHaveBeenCalledTimes(1) + expect(order).toEqual(['/repo/a.ts', '/repo/b.ts', '/repo/c.ts']) + expect(deleted).toEqual(roots) + }) + + it('deletes nothing when the batch confirmation is declined', async () => { + const deleteNode = vi.fn() + + const deleted = await runBatchDeletion({ + roots, + needsConfirmation: true, + confirmBatch: vi.fn().mockResolvedValue(false), + deleteNode + }) + + expect(deleted).toBeNull() + expect(deleteNode).not.toHaveBeenCalled() + }) + + it('skips confirmation when none is needed', async () => { + const confirmBatch = vi.fn() + + const deleted = await runBatchDeletion({ + roots, + needsConfirmation: false, + confirmBatch, + deleteNode: vi.fn(async () => true) + }) + + expect(confirmBatch).not.toHaveBeenCalled() + expect(deleted).toEqual(roots) + }) + + it('excludes failed deletes from the result but continues the batch', async () => { + const deleteNode = vi.fn(async (n: TreeNode) => n.path !== '/repo/b.ts') + + const deleted = await runBatchDeletion({ + roots, + needsConfirmation: false, + confirmBatch: vi.fn(), + deleteNode + }) + + expect(deleteNode).toHaveBeenCalledTimes(3) + expect(deleted).toEqual([roots[0], roots[2]]) + }) +}) diff --git a/src/renderer/src/components/right-sidebar/file-explorer-batch-deletion.ts b/src/renderer/src/components/right-sidebar/file-explorer-batch-deletion.ts new file mode 100644 index 000000000..63a5d0297 --- /dev/null +++ b/src/renderer/src/components/right-sidebar/file-explorer-batch-deletion.ts @@ -0,0 +1,45 @@ +import { isPathEqualOrDescendant } from './file-explorer-paths' +import type { TreeNode } from './file-explorer-types' + +// Why: skip descendants of other selected directories — deleting a parent +// already removes the child, and issuing both requests races on the +// now-missing path and produces spurious errors. +export function selectDeletionRoots(nodes: TreeNode[]): TreeNode[] { + return nodes.filter( + (n) => + !nodes.some( + (other) => other !== n && other.isDirectory && isPathEqualOrDescendant(n.path, other.path) + ) + ) +} + +type RunBatchDeletionParams = { + roots: TreeNode[] + needsConfirmation: boolean + confirmBatch: () => Promise + deleteNode: (node: TreeNode) => Promise +} + +// Why: confirm the whole batch once up front — per-node confirmation inside +// the delete path would prompt once per selected item. Returns the deleted +// roots, or null when the user cancels the batch. +export async function runBatchDeletion({ + roots, + needsConfirmation, + confirmBatch, + deleteNode +}: RunBatchDeletionParams): Promise { + if (needsConfirmation && !(await confirmBatch())) { + return null + } + // Why: process sequentially in the caller's tree order so each delete + // fully settles before the next begins — this avoids concurrent writes + // to the same parent directory and makes failure toasts deterministic. + const deleted: TreeNode[] = [] + for (const node of roots) { + if (await deleteNode(node)) { + deleted.push(node) + } + } + return deleted +} diff --git a/src/renderer/src/components/right-sidebar/useFileDeletion.ts b/src/renderer/src/components/right-sidebar/useFileDeletion.ts index 4d868b7bf..72034023e 100644 --- a/src/renderer/src/components/right-sidebar/useFileDeletion.ts +++ b/src/renderer/src/components/right-sidebar/useFileDeletion.ts @@ -5,6 +5,7 @@ import { useConfirmationDialog } from '@/components/confirmation-dialog' import { dirname } from '@/lib/path' import { useShortcutLabel } from '@/hooks/useShortcutLabel' import { isPathEqualOrDescendant } from './file-explorer-paths' +import { runBatchDeletion, selectDeletionRoots } from './file-explorer-batch-deletion' import type { TreeNode } from './file-explorer-types' import { getFileExplorerOperationRoute } from './file-explorer-operation-owner' import { @@ -38,6 +39,15 @@ type UseFileDeletionResult = { requestDeleteAll: (nodes: TreeNode[]) => void } +// Why: gate the batch prompt on the same condition runDelete uses to actually +// show its per-node confirm — a non-local owner with a resolvable route. +// Unresolved owners throw before prompting, so a batch of them must not pop a +// destructive dialog for deletes that provably cannot proceed. +function needsRemoteDeleteConfirmation(node: TreeNode): boolean { + const operationOwner = node.operationOwner ?? { kind: 'unresolved' as const } + return operationOwner.kind !== 'local' && getFileExplorerOperationRoute(operationOwner) !== null +} + export function useFileDeletion({ activeWorktreeId, openFiles, @@ -57,7 +67,7 @@ export function useFileDeletion({ const inFlightRef = useRef>(new Set()) const runDelete = useCallback( - async (node: TreeNode): Promise => { + async (node: TreeNode, options?: { skipConfirmation?: boolean }): Promise => { if (inFlightRef.current.has(node.path)) { return false } @@ -86,8 +96,9 @@ export function useFileDeletion({ connectionId } // Why: remote deletes bypass OS Trash, and undo cannot recover - // directories or unreadable files. - if (isRemote) { + // directories or unreadable files. Batch deletes confirm once up + // front instead, so they skip the per-node prompt. + if (isRemote && !options?.skipConfirmation) { const confirmed = await confirm({ title: translate( 'auto.components.right.sidebar.useFileDeletion.d979a4fbb5', @@ -247,29 +258,39 @@ export function useFileDeletion({ requestDelete(nodes[0]) return } - // Why: skip descendants of other selected directories — deleting a parent - // already removes the child, and issuing both requests races on the - // now-missing path and produces spurious errors. - const roots = nodes.filter( - (n) => - !nodes.some( - (other) => - other !== n && other.isDirectory && isPathEqualOrDescendant(n.path, other.path) - ) - ) - // Why: process sequentially in the caller's tree order so each delete - // fully settles before the next begins — this avoids concurrent writes - // to the same parent directory and makes failure toasts deterministic. - // Selection is cleared once after the entire batch settles rather than - // per-node, so no concurrent completion can restore a partial stale set. + const roots = selectDeletionRoots(nodes) + // Why: selection is cleared once after the entire batch settles rather + // than per-node, so no concurrent completion can restore a partial + // stale set. void (async () => { - const deletedRoots: TreeNode[] = [] - for (const node of roots) { - if (await runDelete(node)) { - deletedRoots.push(node) - } - } - if (deletedRoots.length === 0) { + const deletedRoots = await runBatchDeletion({ + roots, + // Why: only remote deletes confirm at all — local deletes go to the + // OS Trash and stay prompt-free in batches too. + needsConfirmation: roots.some(needsRemoteDeleteConfirmation), + confirmBatch: () => + confirm({ + // Why: count the full selection, not the filtered roots — + // deleting a folder still deletes the selected children inside + // it, and the prompt should match what the user sees selected. + title: translate( + 'auto.components.right.sidebar.useFileDeletion.af1270b90d', + 'Permanently delete {{count}} items?', + { count: nodes.length } + ), + description: translate( + 'auto.components.right.sidebar.useFileDeletion.dd029aa5cd', + 'This permanently deletes the selected items and any directory contents on the remote host. This cannot be undone.' + ), + confirmLabel: translate( + 'auto.components.right.sidebar.useFileDeletion.92276aceb7', + 'Delete' + ), + confirmVariant: 'destructive' + }), + deleteNode: (node) => runDelete(node, { skipConfirmation: true }) + }) + if (deletedRoots === null || deletedRoots.length === 0) { return } setSelectedPaths( @@ -284,7 +305,7 @@ export function useFileDeletion({ ) })() }, - [runDelete, requestDelete, setSelectedPaths] + [confirm, runDelete, requestDelete, setSelectedPaths] ) return useMemo( diff --git a/src/renderer/src/i18n/locales/en.json b/src/renderer/src/i18n/locales/en.json index 05574797b..22e81f486 100644 --- a/src/renderer/src/i18n/locales/en.json +++ b/src/renderer/src/i18n/locales/en.json @@ -9871,7 +9871,9 @@ "92276aceb7": "Delete", "7fb9435c86": "This permanently deletes the directory and its contents on the remote host. This cannot be undone.", "23e98f192f": "This permanently deletes the file on the remote host. This cannot be undone.", - "8b8ee9d22f": "Couldn't determine which host owns this file. Check the workspace connection and try again." + "8b8ee9d22f": "Couldn't determine which host owns this file. Check the workspace connection and try again.", + "af1270b90d": "Permanently delete {{count}} items?", + "dd029aa5cd": "This permanently deletes the selected items and any directory contents on the remote host. This cannot be undone." }, "useFileExplorerHandlers": { "32cd9fd991": "Cannot open symlink target" diff --git a/src/renderer/src/i18n/locales/es.json b/src/renderer/src/i18n/locales/es.json index 5b5f73cf6..4fd95f6c3 100644 --- a/src/renderer/src/i18n/locales/es.json +++ b/src/renderer/src/i18n/locales/es.json @@ -9871,7 +9871,9 @@ "92276aceb7": "Eliminar", "7fb9435c86": "Esto elimina permanentemente el directorio y su contenido en el host remoto. Esto no se puede deshacer.", "23e98f192f": "Esto elimina permanentemente el archivo en el host remoto. Esto no se puede deshacer.", - "8b8ee9d22f": "No se pudo determinar qué host posee este archivo. Comprueba la conexión del espacio de trabajo e inténtalo de nuevo." + "8b8ee9d22f": "No se pudo determinar qué host posee este archivo. Comprueba la conexión del espacio de trabajo e inténtalo de nuevo.", + "af1270b90d": "¿Eliminar {{count}} elementos permanentemente?", + "dd029aa5cd": "Esto elimina permanentemente los elementos seleccionados y el contenido de cualquier directorio en el host remoto. Esto no se puede deshacer." }, "useFileExplorerHandlers": { "32cd9fd991": "No se puede abrir el destino del enlace simbólico" diff --git a/src/renderer/src/i18n/locales/ja.json b/src/renderer/src/i18n/locales/ja.json index a1c295e55..b5092c297 100644 --- a/src/renderer/src/i18n/locales/ja.json +++ b/src/renderer/src/i18n/locales/ja.json @@ -9871,7 +9871,9 @@ "92276aceb7": "削除", "7fb9435c86": "This permanently deletes the directory and its contents on the remote host. This cannot be undone.", "23e98f192f": "This permanently deletes the file on the remote host. This cannot be undone.", - "8b8ee9d22f": "このファイルを所有しているホストを特定できませんでした。ワークスペースの接続を確認して、もう一度お試しください。" + "8b8ee9d22f": "このファイルを所有しているホストを特定できませんでした。ワークスペースの接続を確認して、もう一度お試しください。", + "af1270b90d": "{{count}}個の項目を完全に削除しますか?", + "dd029aa5cd": "選択した項目とディレクトリの内容がリモートホスト上で完全に削除されます。この操作は元に戻せません。" }, "useFileExplorerHandlers": { "32cd9fd991": "シンボリックリンクターゲットを開けません" diff --git a/src/renderer/src/i18n/locales/ko.json b/src/renderer/src/i18n/locales/ko.json index 93a3b8a52..2d7922995 100644 --- a/src/renderer/src/i18n/locales/ko.json +++ b/src/renderer/src/i18n/locales/ko.json @@ -9871,7 +9871,9 @@ "92276aceb7": "삭제", "7fb9435c86": "원격 호스트의 디렉터리와 그 안의 내용을 영구적으로 삭제합니다. 이 작업은 되돌릴 수 없습니다.", "23e98f192f": "원격 호스트의 파일을 영구적으로 삭제합니다. 이 작업은 되돌릴 수 없습니다.", - "8b8ee9d22f": "이 파일을 소유한 호스트를 확인할 수 없습니다. 워크스페이스 연결을 확인한 후 다시 시도하세요." + "8b8ee9d22f": "이 파일을 소유한 호스트를 확인할 수 없습니다. 워크스페이스 연결을 확인한 후 다시 시도하세요.", + "af1270b90d": "{{count}}개 항목을 영구적으로 삭제하시겠습니까?", + "dd029aa5cd": "원격 호스트의 선택한 항목과 디렉터리 내용을 영구적으로 삭제합니다. 이 작업은 되돌릴 수 없습니다." }, "useFileExplorerHandlers": { "32cd9fd991": "심볼릭 링크 대상을 열 수 없습니다" diff --git a/src/renderer/src/i18n/locales/zh.json b/src/renderer/src/i18n/locales/zh.json index 97398b88c..aa8b6c23f 100644 --- a/src/renderer/src/i18n/locales/zh.json +++ b/src/renderer/src/i18n/locales/zh.json @@ -9871,7 +9871,9 @@ "92276aceb7": "删除", "7fb9435c86": "这会永久删除远程主机上的目录及其内容。此操作无法撤销。", "23e98f192f": "这会永久删除远程主机上的文件。此操作无法撤销。", - "8b8ee9d22f": "无法确定哪个主机拥有此文件。请检查工作区连接后重试。" + "8b8ee9d22f": "无法确定哪个主机拥有此文件。请检查工作区连接后重试。", + "af1270b90d": "永久删除 {{count}} 项？", + "dd029aa5cd": "这会永久删除远程主机上所选的各项及任何目录内容。此操作无法撤销。" }, "useFileExplorerHandlers": { "32cd9fd991": "无法打开符号链接目标" diff --git a/src/renderer/src/runtime/file-explorer-delete-owner-provenance.test.ts b/src/renderer/src/runtime/file-explorer-delete-owner-provenance.test.ts index 75e2bfa6b..80a42f1d8 100644 --- a/src/renderer/src/runtime/file-explorer-delete-owner-provenance.test.ts +++ b/src/renderer/src/runtime/file-explorer-delete-owner-provenance.test.ts @@ -211,6 +211,43 @@ describe('file explorer deletion owner provenance', () => { expect(runtimeEnvironmentCall).not.toHaveBeenCalled() }) + it('does not pop the batch confirm for an unresolved-owner multi-select', async () => { + useAppStore.setState({ + repos: duplicateHostRepos(), + worktreesByRepo: { + [LOCAL_REPO_ID]: [makeWorktree('local'), makeWorktree(`ssh:${SSH_ID}`)] + } + }) + const owner = getFileExplorerOperationOwner(LOCAL_WORKTREE_ID) + expect(owner).toEqual({ kind: 'unresolved' }) + + const { result } = renderDelete(LOCAL_WORKTREE_ID) + const nodeA: TreeNode = { + ...localNode, + name: 'a.ts', + path: '/tmp/project/src/a.ts', + relativePath: 'src/a.ts', + operationOwner: owner + } + const nodeB: TreeNode = { + ...localNode, + name: 'b.ts', + path: '/tmp/project/src/b.ts', + relativePath: 'src/b.ts', + operationOwner: owner + } + await act(async () => { + result.current.requestDeleteAll([nodeA, nodeB]) + }) + + await vi.waitFor(() => expect(toastError).toHaveBeenCalled()) + // Why: unresolved deletes throw before any confirm, so the batch must not + // pop a destructive prompt for operations that provably cannot succeed. + expect(confirm).not.toHaveBeenCalled() + expect(fsDeletePath).not.toHaveBeenCalled() + expect(runtimeEnvironmentCall).not.toHaveBeenCalled() + }) + it('keeps an explicit local worktree local when duplicate repo IDs include SSH', async () => { useAppStore.setState({ settings: { activeRuntimeEnvironmentId: 'focused-env' } as never,