Fix duplicate remote worktree delete failures (#11623)

* Fix duplicate worktree delete attempts

* Complete delete flow selector mock

* Normalize duplicate delete selections
This commit is contained in:
Brennan Benson 2026-07-30 20:32:40 -07:00 committed by GitHub
parent b5abab8b09
commit 8fef6db292
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
5 changed files with 55 additions and 14 deletions

View File

@ -42,6 +42,10 @@ vi.mock('@/store', () => ({
)
}))
vi.mock('@/store/selectors', () => ({
useAllWorktrees: () => mocks.state.allWorktrees()
}))
vi.mock('@/components/ui/dialog', () => ({
Dialog: ({ open, children }: { open: boolean; children: ReactNode }) =>
open ? <div>{children}</div> : null,

View File

@ -7,6 +7,7 @@ import {
DialogTitle
} from '@/components/ui/dialog'
import { useAppStore } from '@/store'
import { useAllWorktrees } from '@/store/selectors'
import { toast } from 'sonner'
import { getConnectionId } from '@/lib/connection-context'
import { getRuntimeGitStatus } from '@/runtime/runtime-git-client'
@ -36,7 +37,7 @@ const DeleteWorktreeDialog = React.memo(function DeleteWorktreeDialog() {
const closeModal = useAppStore((s) => s.closeModal)
const removeWorktree = useAppStore((s) => s.removeWorktree)
const clearWorktreeDeleteState = useAppStore((s) => s.clearWorktreeDeleteState)
const allWorktrees = useAppStore((s) => s.allWorktrees)
const allWorktrees = useAllWorktrees()
const repos = useAppStore((s) => s.repos)
const worktreeLineageById = useAppStore((s) => s.worktreeLineageById)
const updateSettings = useAppStore((s) => s.updateSettings)
@ -62,7 +63,7 @@ const DeleteWorktreeDialog = React.memo(function DeleteWorktreeDialog() {
? (modalData.onDeleted as (worktreeIds: string[]) => void)
: null
const worktree = useMemo(
() => (worktreeId ? (allWorktrees().find((item) => item.id === worktreeId) ?? null) : null),
() => (worktreeId ? (allWorktrees.find((item) => item.id === worktreeId) ?? null) : null),
[allWorktrees, worktreeId]
)
const worktrees = useMemo(() => {
@ -70,7 +71,7 @@ const DeleteWorktreeDialog = React.memo(function DeleteWorktreeDialog() {
return []
}
const selected = new Set(worktreeIds)
return allWorktrees().filter((item) => selected.has(item.id))
return allWorktrees.filter((item) => selected.has(item.id))
}, [allWorktrees, worktreeIds])
const repoMap = useMemo(() => new Map(repos.map((repo) => [repo.id, repo])), [repos])
const isBatchDelete = worktreeIds.length > 1
@ -90,7 +91,7 @@ const DeleteWorktreeDialog = React.memo(function DeleteWorktreeDialog() {
const lineageDelete = useMemo(
() =>
!isBatchDelete && worktree
? getWorkspaceDeleteLineage(worktree, allWorktrees(), worktreeLineageById)
? getWorkspaceDeleteLineage(worktree, allWorktrees, worktreeLineageById)
: { descendants: [], deleteAllTargets: [] },
[allWorktrees, isBatchDelete, worktree, worktreeLineageById]
)

View File

@ -53,6 +53,7 @@ vi.mock('@/store', () => ({
}))
vi.mock('@/store/selectors', () => ({
getAllWorktreesFromState: () => Array.from(mocks.state.worktreeMap.values()),
getWorktreeMapFromState: () => mocks.state.worktreeMap
}))
@ -141,6 +142,16 @@ describe('runWorktreeBatchDelete', () => {
expect(mocks.state.openModal).toHaveBeenCalledWith('delete-worktree', { worktreeId: 'wt-1' })
})
it('treats duplicate selected ids as one delete target', () => {
setWorktrees([{ id: 'wt-1' }])
const started = runWorktreeBatchDelete(['wt-1', 'wt-1'])
expect(started).toBe(true)
expect(mocks.state.clearWorktreeDeleteState).toHaveBeenCalledTimes(1)
expect(mocks.state.openModal).toHaveBeenCalledWith('delete-worktree', { worktreeId: 'wt-1' })
})
it('keeps batch deletes behind confirmation when confirmation is skipped', () => {
mocks.state.settings = { skipDeleteWorktreeConfirm: true }
setWorktrees([

View File

@ -1,6 +1,6 @@
import { toast } from 'sonner'
import { useAppStore } from '@/store'
import { getWorktreeMapFromState } from '@/store/selectors'
import { getAllWorktreesFromState, getWorktreeMapFromState } from '@/store/selectors'
import { findRepoForHost } from '@/store/slices/repo-host-identity'
import { activateAndRevealWorktree } from '@/lib/worktree-activation'
import { prepareActiveWorktreeFocusAfterDelete } from './active-worktree-focus-after-delete'
@ -46,16 +46,18 @@ export async function runWorktreeDeletesInParallel(
targets: readonly Pick<Worktree, 'id' | 'displayName' | 'repoId' | 'path'>[],
options: WorktreeDeleteWithToastOptions = {}
): Promise<string[]> {
// Why: refresh races can leave duplicate rows, but a destructive command must run once per identity.
const uniqueTargets = Array.from(new Map(targets.map((target) => [target.id, target])).values())
// Why: capture the viewed workspace before any delete so we can focus one survivor after the batch settles, not per delete.
const activeWorktreeIdBefore = useAppStore.getState().activeWorktreeId
const commitBatchFocus = activeWorktreeIdBefore
? prepareActiveWorktreeFocusAfterDelete(activeWorktreeIdBefore)
: null
// Why: mark every target deleting up front for immediate in-flight feedback, even though deletes serialize per repo.
useAppStore.getState().markWorktreesDeleting(targets.map((target) => target.id))
useAppStore.getState().markWorktreesDeleting(uniqueTargets.map((target) => target.id))
// Why: worktree remove/prune/branch -D race on shared ref locks; group by repoId to serialize per repo (cross-repo stays parallel).
const groups = new Map<string, (typeof targets)[number][]>()
for (const target of targets) {
const groups = new Map<string, (typeof uniqueTargets)[number][]>()
for (const target of uniqueTargets) {
const group = groups.get(target.repoId)
if (group) {
group.push(target)
@ -95,7 +97,7 @@ export async function runWorktreeDeletesInParallel(
if (activeWorktreeIdBefore && deletedSet.has(activeWorktreeIdBefore)) {
commitBatchFocus?.()
}
return targets.filter((target) => deletedSet.has(target.id)).map((target) => target.id)
return uniqueTargets.filter((target) => deletedSet.has(target.id)).map((target) => target.id)
}
/**
@ -249,8 +251,8 @@ export function runWorktreeDelete(worktreeId: string): void {
}
const hasLineageChildren =
getWorkspaceDeleteLineage(target, state.allWorktrees(), state.worktreeLineageById).descendants
.length > 0
getWorkspaceDeleteLineage(target, getAllWorktreesFromState(state), state.worktreeLineageById)
.descendants.length > 0
const skipConfirm = state.settings?.skipDeleteWorktreeConfirm ?? false
if (skipConfirm && !hasLineageChildren) {
void runWorktreeDeleteWithToast(worktreeId, target.displayName)
@ -268,7 +270,7 @@ export function runWorktreeBatchDelete(
): boolean {
const state = useAppStore.getState()
const worktreeMap = getWorktreeMapFromState(state)
const targets = worktreeIds
const targets = Array.from(new Set(worktreeIds))
.map((id) => worktreeMap.get(id) ?? null)
.filter((worktree): worktree is Worktree => worktree != null && !worktree.isMainWorktree)
@ -295,8 +297,11 @@ export function runWorktreeBatchDelete(
// Why: bulk cleanup can destroy many directories at once, so batch/Space deletes keep an explicit confirmation step.
const singleTargetHasLineageChildren =
targets.length === 1 &&
getWorkspaceDeleteLineage(targets[0], state.allWorktrees(), state.worktreeLineageById)
.descendants.length > 0
getWorkspaceDeleteLineage(
targets[0],
getAllWorktreesFromState(state),
state.worktreeLineageById
).descendants.length > 0
const skipConfirm =
!options.forceConfirm &&
targets.length === 1 &&

View File

@ -31,6 +31,7 @@ vi.mock('@/store', () => ({
}))
vi.mock('@/store/selectors', () => ({
getAllWorktreesFromState: () => Array.from(mocks.state.worktreeMap.values()),
getWorktreeMapFromState: () => mocks.state.worktreeMap
}))
@ -145,6 +146,25 @@ describe('runWorktreeDeletesInParallel', () => {
expect(mocks.state.removeWorktree).toHaveBeenNthCalledWith(2, 'wt-2', true)
})
it('deletes a duplicated target identity only once', async () => {
const target = {
id: 'wt-1',
displayName: 'one',
repoId: 'repo-a',
path: '/workspaces/one'
}
mocks.state.removeWorktree
.mockResolvedValueOnce({ ok: true })
.mockResolvedValueOnce({ ok: false, error: 'selector_not_found' })
await expect(runWorktreeDeletesInParallel([target, target])).resolves.toEqual(['wt-1'])
expect(mocks.state.markWorktreesDeleting).toHaveBeenCalledWith(['wt-1'])
expect(mocks.state.removeWorktree).toHaveBeenCalledTimes(1)
expect(mocks.state.removeWorktree).toHaveBeenCalledWith('wt-1', false)
expect(toast.error).not.toHaveBeenCalled()
})
it('clears a pending ancestor when a nested descendant delete fails', async () => {
mocks.state.removeWorktree.mockImplementationOnce(async (worktreeId: string) => {
mocks.state.deleteStateByWorktreeId[worktreeId] = {