diff --git a/src/renderer/src/components/workspace-cleanup/WorkspaceCleanupDialog.tsx b/src/renderer/src/components/workspace-cleanup/WorkspaceCleanupDialog.tsx index 91a552735ef..d5ec61ca3f3 100644 --- a/src/renderer/src/components/workspace-cleanup/WorkspaceCleanupDialog.tsx +++ b/src/renderer/src/components/workspace-cleanup/WorkspaceCleanupDialog.tsx @@ -307,7 +307,10 @@ export default function WorkspaceCleanupDialog(): React.JSX.Element { setSelectedIds(new Set()) } } - if (!loading && !autoScanAttemptedForOpenRef.current) { + // Why: reopening mid-batch keeps the deletion progress view; a broad scan + // started here would be discarded by the removal's scan invalidation, so + // skip it while a removal batch is running (matches the reset guard above). + if (!loading && !autoScanAttemptedForOpenRef.current && !removalInFlightRef.current) { autoScanAttemptedForOpenRef.current = true startWorkspaceCleanupScan({ notifyWhenReady: true }) } @@ -523,6 +526,19 @@ export default function WorkspaceCleanupDialog(): React.JSX.Element { setConfirmCandidates([]) }, [closeModal, removalProgress]) + const clearQueuedDeleteState = useCallback( + (worktreeId: string) => { + const deleteState = useAppStore.getState().deleteStateByWorktreeId[worktreeId] + // Why: candidates that fail before removal starts would otherwise stay + // marked "Queued for deletion" in the sidebar; rows already in the + // 'deleting' phase or failed with an error keep their own state. + if (deleteState?.isDeleting && deleteState.error === null && deleteState.phase === 'queued') { + clearWorktreeDeleteState(worktreeId) + } + }, + [clearWorktreeDeleteState] + ) + const confirmRemove = useCallback(() => { if (confirmCandidates.length === 0 || removalInFlightRef.current) { return @@ -547,20 +563,14 @@ export default function WorkspaceCleanupDialog(): React.JSX.Element { setRemovalProgress(progress) } }, + onRowFailed: (failure) => { + clearQueuedDeleteState(failure.worktreeId) + }, onResult: (result) => { const nextFailures: Record = {} for (const failure of result.failures) { nextFailures[failure.worktreeId] = failure.message - const deleteState = useAppStore.getState().deleteStateByWorktreeId[failure.worktreeId] - if ( - deleteState?.isDeleting && - deleteState.error === null && - deleteState.phase === 'queued' - ) { - // Why: candidates that fail before removal starts would otherwise - // stay marked "Queued for deletion" in the sidebar indefinitely. - clearWorktreeDeleteState(failure.worktreeId) - } + clearQueuedDeleteState(failure.worktreeId) } if (mountedRef.current) { setRowFailures(nextFailures) @@ -592,6 +602,7 @@ export default function WorkspaceCleanupDialog(): React.JSX.Element { } }) }, [ + clearQueuedDeleteState, clearWorktreeDeleteState, confirmCandidates, markWorktreesQueuedForDeletion, diff --git a/src/renderer/src/components/workspace-cleanup/workspace-cleanup-background-removal.test.ts b/src/renderer/src/components/workspace-cleanup/workspace-cleanup-background-removal.test.ts index ec3a4114a97..abe1c5ab396 100644 --- a/src/renderer/src/components/workspace-cleanup/workspace-cleanup-background-removal.test.ts +++ b/src/renderer/src/components/workspace-cleanup/workspace-cleanup-background-removal.test.ts @@ -214,6 +214,41 @@ describe('startWorkspaceCleanupBackgroundRemoval', () => { }) }) + it('reports each failure as it happens so queued rows can clear before the batch ends', async () => { + const parent = makeCandidate({ + worktreeId: 'repo-1::/repo/parent', + displayName: 'parent', + branch: 'parent', + path: '/repo/parent' + }) + const child = makeCandidate({ + worktreeId: 'repo-1::/repo/parent/child', + displayName: 'child', + branch: 'child', + path: '/repo/parent/child' + }) + const removeCandidates = vi.fn().mockResolvedValueOnce({ + removedIds: [], + failures: [{ worktreeId: child.worktreeId, displayName: child.displayName, message: 'busy' }] + }) + const onRowFailed = vi.fn() + + startWorkspaceCleanupBackgroundRemoval({ + candidates: [parent, child], + removeCandidates, + onProgress: vi.fn(), + onRowFailed + }) + await settleBackgroundRemoval() + + // Child fails at removal (per-row result), then parent is skipped as its + // ancestor — both reported incrementally, not only in the final result. + expect(onRowFailed.mock.calls.map(([failure]) => failure.worktreeId)).toEqual([ + child.worktreeId, + parent.worktreeId + ]) + }) + it('does not skip same-path ancestors from another connection after a nested failure', async () => { const failedChild = makeCandidate({ worktreeId: 'repo-1::/repo/parent/child', diff --git a/src/renderer/src/components/workspace-cleanup/workspace-cleanup-background-removal.ts b/src/renderer/src/components/workspace-cleanup/workspace-cleanup-background-removal.ts index a8d14923cba..9823aa04033 100644 --- a/src/renderer/src/components/workspace-cleanup/workspace-cleanup-background-removal.ts +++ b/src/renderer/src/components/workspace-cleanup/workspace-cleanup-background-removal.ts @@ -29,6 +29,10 @@ export type WorkspaceCleanupBackgroundRemovalArgs = { onProgress: (progress: WorkspaceCleanupRemovalProgress) => void onResult?: (result: WorkspaceCleanupRemoveResult) => void onError?: (error: unknown) => void + // Why: a row can fail before its removal starts (preflight failure or a + // skipped nested workspace); report it now so its queued overlay can clear + // instead of waiting for the whole batch to settle. + onRowFailed?: (failure: WorkspaceCleanupFailure) => void removalTimeoutMs?: number } @@ -38,6 +42,7 @@ export function startWorkspaceCleanupBackgroundRemoval({ onProgress, onResult, onError, + onRowFailed, removalTimeoutMs = DEFAULT_WORKSPACE_CLEANUP_REMOVAL_TIMEOUT_MS }: WorkspaceCleanupBackgroundRemovalArgs): void { if (candidates.length === 0) { @@ -64,6 +69,17 @@ export function startWorkspaceCleanupBackgroundRemoval({ }) } + const reportFailures = (rowFailures: readonly WorkspaceCleanupFailure[]): void => { + for (const failure of rowFailures) { + failures.push(failure) + try { + onRowFailed?.(failure) + } catch (callbackError) { + console.error('Workspace cleanup row failure callback failed', callbackError) + } + } + } + emitProgress() // Why: keep the store's nested-worktree delete invariant even though progress @@ -78,14 +94,16 @@ export function startWorkspaceCleanupBackgroundRemoval({ ) ) { failedCandidates.push(candidate) - failures.push({ - worktreeId: candidate.worktreeId, - displayName: candidate.displayName, - message: translate( - 'auto.components.workspace.cleanup.backgroundRemoval.skippedAncestor', - 'Skipped because a nested workspace could not be removed.' - ) - }) + reportFailures([ + { + worktreeId: candidate.worktreeId, + displayName: candidate.displayName, + message: translate( + 'auto.components.workspace.cleanup.backgroundRemoval.skippedAncestor', + 'Skipped because a nested workspace could not be removed.' + ) + } + ]) processedCount += 1 emitProgress() continue @@ -97,17 +115,19 @@ export function startWorkspaceCleanupBackgroundRemoval({ removalTimeoutMs ) removedIds.push(...result.removedIds) - failures.push(...result.failures) + reportFailures(result.failures) if (result.failures.length > 0) { failedCandidates.push(candidate) } } catch (error: unknown) { failedCandidates.push(candidate) - failures.push({ - worktreeId: candidate.worktreeId, - displayName: candidate.displayName, - message: error instanceof Error ? error.message : String(error) - }) + reportFailures([ + { + worktreeId: candidate.worktreeId, + displayName: candidate.displayName, + message: error instanceof Error ? error.message : String(error) + } + ]) } finally { processedCount += 1 emitProgress() diff --git a/src/renderer/src/store/slices/workspace-cleanup-removal-preflight.test.ts b/src/renderer/src/store/slices/workspace-cleanup-removal-preflight.test.ts index 745e1a32298..cae7d7c8fce 100644 --- a/src/renderer/src/store/slices/workspace-cleanup-removal-preflight.test.ts +++ b/src/renderer/src/store/slices/workspace-cleanup-removal-preflight.test.ts @@ -369,6 +369,43 @@ describe('workspace cleanup removal and protection', () => { }) }) + it('fails a removal that reveals concrete git risk after an unverified force approval', async () => { + const approvedCandidate = makeCandidate({ + tier: 'review', + blockers: ['git-status-error'], + git: { clean: null, upstreamAhead: null, upstreamBehind: null, checkedAt: null } + }) + const nowRevealsUnpushed = makeCandidate({ + tier: 'review', + blockers: ['unpushed-commits'], + git: { clean: true, upstreamAhead: 3, upstreamBehind: 0, checkedAt: NOW } + }) + const scan = vi.fn().mockResolvedValue({ + scannedAt: NOW, + candidates: [nowRevealsUnpushed], + errors: [] + } satisfies WorkspaceCleanupScanResult) + installWorkspaceCleanupApi(scan) + const removeWorktree = vi.fn().mockResolvedValue({ ok: true }) + const store = createCleanupTestStore(removeWorktree) + + await expect( + store.getState().removeWorkspaceCleanupCandidates([WORKTREE_ID], { + approvedCandidates: [approvedCandidate] + }) + ).resolves.toEqual({ + removedIds: [], + failures: [ + { + worktreeId: WORKTREE_ID, + displayName: 'old-workspace', + message: 'Workspace changed after confirmation. Refresh to review it before removing.' + } + ] + }) + expect(removeWorktree).not.toHaveBeenCalled() + }) + it('protects old workspaces when an agent process is still foregrounded', async () => { ;(globalThis as { window: unknown }).window = { api: { diff --git a/src/renderer/src/store/slices/workspace-cleanup.ts b/src/renderer/src/store/slices/workspace-cleanup.ts index 52106424121..ed2e12d49be 100644 --- a/src/renderer/src/store/slices/workspace-cleanup.ts +++ b/src/renderer/src/store/slices/workspace-cleanup.ts @@ -79,6 +79,10 @@ type WorkspaceCleanupEnrichmentCacheEntry = { const RECENT_VISIBLE_CONTEXT_MS = 24 * 60 * 60 * 1000 const VIEWED_FROM_CLEANUP_MS = 2 * 60 * 60 * 1000 const WORKSPACE_CLEANUP_PREFLIGHT_CONCURRENCY = 4 +// Why: dirty-files/unpushed-commits are concrete known work at risk; unknown-base +// and git-status-error only mean "couldn't verify". A row approved while +// unverifiable must still fail if real work becomes visible before removal. +const WORKSPACE_CLEANUP_CONCRETE_RISK_BLOCKERS = ['dirty-files', 'unpushed-commits'] as const let inFlightWorkspaceCleanupScan: { key: string @@ -857,20 +861,27 @@ async function preflightWorkspaceCleanupCandidate( // Why: this row may be removed minutes after the confirm click. If it now // needs a force removal the user never approved (new dirt, unpushed work, // or a git error since confirmation), fail it instead of force-deleting. - if ( - approvedCandidate && - shouldForceWorkspaceCleanupRemoval(candidate) && - !shouldForceWorkspaceCleanupRemoval(approvedCandidate) - ) { - return { - ok: false, - failure: { - worktreeId, - displayName: candidate.displayName, - message: translate( - 'auto.store.slices.workspace.cleanup.changedSinceConfirmation', - 'Workspace changed after confirmation. Refresh to review it before removing.' - ) + if (approvedCandidate) { + const escalatedToForce = + shouldForceWorkspaceCleanupRemoval(candidate) && + !shouldForceWorkspaceCleanupRemoval(approvedCandidate) + // Why: an approved row that was already force-flagged for an unverifiable + // reason must still fail when real dirt/unpushed work is now visible. + const revealedConcreteRisk = WORKSPACE_CLEANUP_CONCRETE_RISK_BLOCKERS.some( + (blocker) => + candidate.blockers.includes(blocker) && !approvedCandidate.blockers.includes(blocker) + ) + if (escalatedToForce || revealedConcreteRisk) { + return { + ok: false, + failure: { + worktreeId, + displayName: candidate.displayName, + message: translate( + 'auto.store.slices.workspace.cleanup.changedSinceConfirmation', + 'Workspace changed after confirmation. Refresh to review it before removing.' + ) + } } } }