From f71ef7ee03042b28cdbc5352351ebedda6d0e6f2 Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Tue, 18 Aug 2026 14:08:25 -0700 Subject: [PATCH] fix(sidebar): clear the delete state when a removal is refused (#15187) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(sidebar): clear the delete state when a removal is refused Callers mark rows deleting up front for immediate sidebar feedback, but a refusal in beginHostQualifiedRemoval returns before removeWorktree's try/catch — the only other place that clears the flag. The failure toast auto-dismisses after 10s, so the workspace was left on a 'Deleting…' spinner indefinitely with no explanation still on screen. Also removes workspace-cleanup-removal-host-guard.ts: #14731 refused a colliding cleanup removal, #15013 replaced refusing with routing to the confirmed host, and nothing imports the module any more — not even its own tests. * test(sidebar): pin the two states that actually reach the removal refusal The existing case used an empty store, which proved the clear but not that a user can land there. Cover a stale folder-workspace id and a hostless row under an ambiguous legacy runtime — both resolve to no route from populated state. --------- Co-authored-by: QA --- .../workspace-cleanup-removal-host-guard.ts | 43 ------- .../host-qualified-removal-refusal.test.ts | 110 ++++++++++++++++++ .../host-qualified-worktree-removal.ts | 7 ++ 3 files changed, 117 insertions(+), 43 deletions(-) delete mode 100644 src/renderer/src/store/slices/workspace-cleanup-removal-host-guard.ts create mode 100644 src/renderer/src/store/slices/worktrees/teardown/host-qualified-removal-refusal.test.ts diff --git a/src/renderer/src/store/slices/workspace-cleanup-removal-host-guard.ts b/src/renderer/src/store/slices/workspace-cleanup-removal-host-guard.ts deleted file mode 100644 index 30d55d66d6b..00000000000 --- a/src/renderer/src/store/slices/workspace-cleanup-removal-host-guard.ts +++ /dev/null @@ -1,43 +0,0 @@ -/** - * STA-4343 fail-closed guard: cleanup must never issue a destructive removal - * against a host other than the one whose row the user confirmed. - * - * A cleanup row's `worktreeId` is `repoId::path`, which two execution hosts can - * both own, while selection, confirmation and preflight all key on `worktreeId` - * alone and removal routing prefers the ACTIVE workspace's host. This is the - * minimal safety property: refuse whenever the confirmed owner is unknown, is - * not the only owner the refreshed scan reports, or is not where the removal - * would actually land. Routing a colliding row to its right host is #14606. - */ -import type { ExecutionHostId } from '../../../../shared/execution-host' -import { resolveWorkspaceCleanupRemovalHostId } from '../../../../shared/workspace-cleanup-host-identity' -import type { WorkspaceCleanupCandidate } from '../../../../shared/workspace-cleanup' - -/** True only when the removal is provably about to land on the confirmed row's own host. */ -export function isWorkspaceCleanupRemovalHostCertain(args: { - /** The row the user confirmed; internal callers without one fall back to the scanned row. */ - confirmedCandidate: WorkspaceCleanupCandidate | undefined - scannedCandidate: WorkspaceCleanupCandidate - /** Every owner the refreshed scan reported for this `worktreeId`, null where a row carries none. */ - scannedHostIds: readonly (ExecutionHostId | null)[] - /** - * The host `removeWorktree` would route the destructive IPC to. Null is not a - * refusal here: `removeWorktree` already fails an unroutable removal closed - * with WORKTREE_REMOVAL_AMBIGUOUS_ERROR before it touches any transport. - */ - routeHostId: ExecutionHostId | null -}): boolean { - const confirmedHostId = resolveWorkspaceCleanupRemovalHostId( - args.confirmedCandidate ?? args.scannedCandidate - ) - if (!confirmedHostId) { - return false - } - // Why: one id owned by two hosts cannot say which row was confirmed, and a - // confirmed owner the refreshed scan no longer reports cannot be verified either. - const scannedHostIds = new Set(args.scannedHostIds) - if (scannedHostIds.size !== 1 || !scannedHostIds.has(confirmedHostId)) { - return false - } - return args.routeHostId === null || args.routeHostId === confirmedHostId -} diff --git a/src/renderer/src/store/slices/worktrees/teardown/host-qualified-removal-refusal.test.ts b/src/renderer/src/store/slices/worktrees/teardown/host-qualified-removal-refusal.test.ts new file mode 100644 index 00000000000..ae2d3c31871 --- /dev/null +++ b/src/renderer/src/store/slices/worktrees/teardown/host-qualified-removal-refusal.test.ts @@ -0,0 +1,110 @@ +import { describe, expect, it, vi } from 'vitest' +import { beginHostQualifiedRemoval } from './host-qualified-worktree-removal' +import { folderWorkspaceKey } from '../../../../../../shared/workspace-scope' + +const WORKTREE_ID = 'repo1::/shared/workspace/path' + +/** + * Callers mark rows deleting up front for immediate sidebar feedback + * (worktree-delete-execution.ts). These refusals return BEFORE removeWorktree's + * try/catch, which is the only other place that clears the flag — so without an + * explicit clear the workspace keeps its "Deleting…" spinner indefinitely, long + * after the 10s failure toast has gone. + */ +// Note: with a confirmed host the route resolves from the host id alone, so this refusal is +// reached on the UNQUALIFIED path — a caller that names no host and has no resolvable owner. +describe('beginHostQualifiedRemoval refusals clear the delete state', () => { + function makeGet(clearWorktreeDeleteState: ReturnType) { + // Minimal store surface: no worktrees, so no route can resolve for MISSING_HOST. + return () => + ({ + clearWorktreeDeleteState, + allWorktrees: () => [], + worktreesByRepo: {}, + repos: [], + detectedWorktreesByRepo: {}, + settings: {}, + sshConnectionStates: new Map(), + sshTargetLabels: new Map(), + workspaceCleanupScan: null + }) as never + } + + it('clears when no route resolves and no host was confirmed', () => { + const clearWorktreeDeleteState = vi.fn() + const start = beginHostQualifiedRemoval( + makeGet(clearWorktreeDeleteState), + WORKTREE_ID, + null, + false + ) + + expect(start.ok).toBe(false) + expect(clearWorktreeDeleteState).toHaveBeenCalledWith(WORKTREE_ID) + }) + + // The store above is deliberately empty, which proves the clear but not that a real user can + // land here. These two cases route through populated state a user actually has, and still + // resolve to no route — the states that leave the row spinning in the product. + function makeRoutedGet( + clearWorktreeDeleteState: ReturnType, + overrides: Record + ) { + return () => + ({ + clearWorktreeDeleteState, + allWorktrees: () => [], + worktreesByRepo: {}, + repos: [], + detectedWorktreesByRepo: {}, + settings: {}, + runtimeEnvironments: [], + folderWorkspaces: [], + projectGroups: [], + sshConnectionStates: new Map(), + sshTargetLabels: new Map(), + workspaceCleanupScan: null, + ...overrides + }) as never + } + + // A hostless row (folder-workspace meta never sets hostId, and runtime rows omit the field + // when the repo is unresolved) plus more than one saved runtime environment trips the legacy + // single-runtime gate in resolveWorktreeOperationRouteResult, which returns `missing`. + it('clears when a known worktree has no host and the legacy runtime is ambiguous', () => { + const clearWorktreeDeleteState = vi.fn() + const start = beginHostQualifiedRemoval( + makeRoutedGet(clearWorktreeDeleteState, { + repos: [{ id: 'repo1', connectionId: null, executionHostId: undefined }], + worktreesByRepo: { repo1: [{ id: WORKTREE_ID, repoId: 'repo1' }] }, + settings: { activeRuntimeEnvironmentId: 'env-a' }, + runtimeEnvironments: [{ id: 'env-a' }, { id: 'env-b' }] + }), + WORKTREE_ID, + null, + false + ) + + expect(start.ok).toBe(false) + expect(clearWorktreeDeleteState).toHaveBeenCalledWith(WORKTREE_ID) + }) + + // Folder workspaces fail closed on a stale id by design, so a row whose folder record is gone + // refuses instead of routing. + it('clears when a folder workspace id no longer has an owner', () => { + const clearWorktreeDeleteState = vi.fn() + const folderId = folderWorkspaceKey('fw-removed') + const start = beginHostQualifiedRemoval( + makeRoutedGet(clearWorktreeDeleteState, { + repos: [{ id: 'repo1', connectionId: null, executionHostId: 'local' }], + folderWorkspaces: [] + }), + folderId, + null, + false + ) + + expect(start.ok).toBe(false) + expect(clearWorktreeDeleteState).toHaveBeenCalledWith(folderId) + }) +}) diff --git a/src/renderer/src/store/slices/worktrees/teardown/host-qualified-worktree-removal.ts b/src/renderer/src/store/slices/worktrees/teardown/host-qualified-worktree-removal.ts index 86bc2d62f80..7f7eb8d4158 100644 --- a/src/renderer/src/store/slices/worktrees/teardown/host-qualified-worktree-removal.ts +++ b/src/renderer/src/store/slices/worktrees/teardown/host-qualified-worktree-removal.ts @@ -78,6 +78,12 @@ export function beginHostQualifiedRemoval( resolveHostQualifiedRemovalRoute(get, worktreeId, requiredExecutionHostId) const removalRoute = resolveRemovalRoute() if (!removalRoute && (!forgetLocalOnly || !requiredExecutionHostId)) { + // Why: callers mark rows deleting up front for immediate sidebar feedback + // (worktree-delete-execution.ts), and a refusal returns before the try/catch that + // would otherwise clear it. The failure toast auto-dismisses after 10s, so without + // this the workspace sits on a "Deleting…" spinner forever with no explanation left + // on screen. + get().clearWorktreeDeleteState(worktreeId) return { ok: false, error: WORKTREE_REMOVAL_AMBIGUOUS_ERROR } } // Fail closed rather than delete on a host the caller never confirmed. @@ -86,6 +92,7 @@ export function beginHostQualifiedRemoval( removalRoute && removalRoute.executionHostId !== requiredExecutionHostId ) { + get().clearWorktreeDeleteState(worktreeId) return { ok: false, error: WORKTREE_REMOVAL_HOST_CHANGED_ERROR } } const sameIdSurvivingHostId = resolveSameIdSurvivingHostId(