feat(file-explorer): confirm multi-select deletes with a single batch prompt (#7459)
* 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 <help@stably.ai>
---------
Co-authored-by: Tom de Bres <tomdebres@users.noreply.github.com>
Co-authored-by: Jinjing <6427696+AmethystLiang@users.noreply.github.com>
Co-authored-by: Orca <help@stably.ai>
This commit is contained in:
parent
745b164b94
commit
80164d6863
|
|
@ -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]])
|
||||
})
|
||||
})
|
||||
|
|
@ -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<boolean>
|
||||
deleteNode: (node: TreeNode) => Promise<boolean>
|
||||
}
|
||||
|
||||
// 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<TreeNode[] | null> {
|
||||
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
|
||||
}
|
||||
|
|
@ -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<Set<string>>(new Set())
|
||||
|
||||
const runDelete = useCallback(
|
||||
async (node: TreeNode): Promise<boolean> => {
|
||||
async (node: TreeNode, options?: { skipConfirmation?: boolean }): Promise<boolean> => {
|
||||
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(
|
||||
|
|
|
|||
|
|
@ -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"
|
||||
|
|
|
|||
|
|
@ -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"
|
||||
|
|
|
|||
|
|
@ -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": "シンボリックリンクターゲットを開けません"
|
||||
|
|
|
|||
|
|
@ -9871,7 +9871,9 @@
|
|||
"92276aceb7": "삭제",
|
||||
"7fb9435c86": "원격 호스트의 디렉터리와 그 안의 내용을 영구적으로 삭제합니다. 이 작업은 되돌릴 수 없습니다.",
|
||||
"23e98f192f": "원격 호스트의 파일을 영구적으로 삭제합니다. 이 작업은 되돌릴 수 없습니다.",
|
||||
"8b8ee9d22f": "이 파일을 소유한 호스트를 확인할 수 없습니다. 워크스페이스 연결을 확인한 후 다시 시도하세요."
|
||||
"8b8ee9d22f": "이 파일을 소유한 호스트를 확인할 수 없습니다. 워크스페이스 연결을 확인한 후 다시 시도하세요.",
|
||||
"af1270b90d": "{{count}}개 항목을 영구적으로 삭제하시겠습니까?",
|
||||
"dd029aa5cd": "원격 호스트의 선택한 항목과 디렉터리 내용을 영구적으로 삭제합니다. 이 작업은 되돌릴 수 없습니다."
|
||||
},
|
||||
"useFileExplorerHandlers": {
|
||||
"32cd9fd991": "심볼릭 링크 대상을 열 수 없습니다"
|
||||
|
|
|
|||
|
|
@ -9871,7 +9871,9 @@
|
|||
"92276aceb7": "删除",
|
||||
"7fb9435c86": "这会永久删除远程主机上的目录及其内容。此操作无法撤销。",
|
||||
"23e98f192f": "这会永久删除远程主机上的文件。此操作无法撤销。",
|
||||
"8b8ee9d22f": "无法确定哪个主机拥有此文件。请检查工作区连接后重试。"
|
||||
"8b8ee9d22f": "无法确定哪个主机拥有此文件。请检查工作区连接后重试。",
|
||||
"af1270b90d": "永久删除 {{count}} 项?",
|
||||
"dd029aa5cd": "这会永久删除远程主机上所选的各项及任何目录内容。此操作无法撤销。"
|
||||
},
|
||||
"useFileExplorerHandlers": {
|
||||
"32cd9fd991": "无法打开符号链接目标"
|
||||
|
|
|
|||
|
|
@ -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,
|
||||
|
|
|
|||
Loading…
Reference in New Issue