diff --git a/src/renderer/src/components/status-bar/WorkspaceSpaceManagerPanel.tsx b/src/renderer/src/components/status-bar/WorkspaceSpaceManagerPanel.tsx index aa7a8526829..7831c6c48e7 100644 --- a/src/renderer/src/components/status-bar/WorkspaceSpaceManagerPanel.tsx +++ b/src/renderer/src/components/status-bar/WorkspaceSpaceManagerPanel.tsx @@ -46,7 +46,14 @@ import { getRepoMapFromState, getWorktreeMapFromState } from '../../store/select import { getHostedReviewCacheKey } from '../../store/slices/hosted-review' import { issueCacheKey as getIssueCacheKey } from '../../store/slices/github' import { refreshGitStatusForWorktree } from '../right-sidebar/git-status-refresh' +import { + getSelectedDeletableWorkspaceRows, + getVisibleDeletableWorkspaceIds +} from './workspace-space-delete-selection' import { runWorktreeBatchDelete } from '../sidebar/delete-worktree-flow' +import { toWorktreeDeleteIdentities } from '../sidebar/worktree-delete-request' +import { showWorkspaceListChangedToast } from '../sidebar/stale-workspace-list-toast' +import { getWorktreeOnHostFromState } from '../../store/selectors' import { prepareActiveWorktreeFocusAfterDelete } from '../sidebar/active-worktree-focus-after-delete' import { branchDisplayName } from '../sidebar/WorktreeCardHelpers' import { Badge } from '../ui/badge' @@ -75,8 +82,6 @@ import { countWorkspaceSpaceActiveAgents, getLargestWorkspaceSpaceItemSize, getLargestWorkspaceSpaceRowSize, - getSelectedDeletableWorkspaceIds, - getVisibleDeletableWorkspaceIds, getWorkspaceSpaceGitStatusRefreshCandidates, isWorkspaceSpaceRowReadyToDelete, pruneWorkspaceSpaceSelectedIds, @@ -1491,10 +1496,14 @@ export function WorkspaceSpaceManagerPanel(): React.JSX.Element { sourceRows.find((row) => row.worktreeId === nextTreemapZoomWorktreeId && row.status === 'ok') ?? null const maxSize = getLargestWorkspaceSpaceRowSize(rows) - const selectedDeletableIds = useMemo( - () => getSelectedDeletableWorkspaceIds(rows, nextSelectedIds, isWorktreeUnavailableForDelete), + const selectedDeletableRows = useMemo( + () => getSelectedDeletableWorkspaceRows(rows, nextSelectedIds, isWorktreeUnavailableForDelete), [isWorktreeUnavailableForDelete, nextSelectedIds, rows] ) + const selectedDeletableIds = useMemo( + () => selectedDeletableRows.map((row) => row.worktreeId), + [selectedDeletableRows] + ) const selectedDeletableIdSet = useMemo( () => new Set(selectedDeletableIds), [selectedDeletableIds] @@ -1606,11 +1615,31 @@ export function WorkspaceSpaceManagerPanel(): React.JSX.Element { ) const deleteWorktrees = useCallback( - (worktreeIds: readonly string[]): void => { - if (worktreeIds.length === 0) { + (targets: readonly WorkspaceSpaceWorktree[]): void => { + if (targets.length === 0) { return } - runWorktreeBatchDelete(worktreeIds, { + // Why (STA-4343): the Space scan lists one row per host, so a bare id would + // route the delete at whichever host the id-keyed lookup happens to hold. + // Resolve each row on ITS host and hand over the store row's own identity — + // a Space row carries no instanceId, so a hand-built one would be rejected + // as stale by the confirmed-target check. + const state = useAppStore.getState() + const identities = toWorktreeDeleteIdentities( + targets.flatMap((target) => { + const row = getWorktreeOnHostFromState( + state, + target.worktreeId, + target.executionHostId ?? undefined + ) + return row ? [row] : [] + }) + ) + if (identities.length !== targets.length) { + showWorkspaceListChangedToast() + return + } + runWorktreeBatchDelete(identities, { forceConfirm: true, onDeleted: handleDeletedWorktrees }) @@ -1663,10 +1692,10 @@ export function WorkspaceSpaceManagerPanel(): React.JSX.Element { ) const deleteSelected = (): void => { - if (selectedDeletableIds.length === 0) { + if (selectedDeletableRows.length === 0) { return } - deleteWorktrees(selectedDeletableIds) + deleteWorktrees(selectedDeletableRows) } return ( @@ -2082,7 +2111,7 @@ export function WorkspaceSpaceManagerPanel(): React.JSX.Element { onToggleSelected={() => toggleSelection(worktree.worktreeId)} onInspect={() => setInspectedWorktreeId(worktree.worktreeId)} onOpenWorkspace={() => activateAndRevealWorktree(worktree.worktreeId)} - onDelete={() => deleteWorktrees([worktree.worktreeId])} + onDelete={() => deleteWorktrees([worktree])} onForceDelete={() => forceDeleteWorktree(worktree)} /> )) diff --git a/src/renderer/src/components/status-bar/workspace-space-delete-host-routing.test.ts b/src/renderer/src/components/status-bar/workspace-space-delete-host-routing.test.ts new file mode 100644 index 00000000000..a3cd1f46865 --- /dev/null +++ b/src/renderer/src/components/status-bar/workspace-space-delete-host-routing.test.ts @@ -0,0 +1,117 @@ +/** + * STA-4343: the Space Manager's ORDINARY delete must route to the row's host. + * + * The Space scan lists one row per execution host, so two rows can share a + * `repoId::path` worktree id. The force-delete path carried `executionHostId`; + * the ordinary Delete one line above it passed a bare id, which resolves through + * the id-keyed lookup and lands on whichever host happens to be first. Clicking + * Delete on the SSH row therefore deleted the LOCAL checkout. + * + * A Space row carries no `instanceId` (see WorkspaceSpaceWorktree), so the fix + * cannot hand-build an identity — it has to resolve the store row on that host + * and pass the store row's own identity, or the confirmed-target check rejects + * every Space delete as stale. + */ +import { describe, expect, it } from 'vitest' +import type { ExecutionHostId } from '../../../../shared/execution-host' +import type { WorkspaceSpaceWorktree } from '../../../../shared/workspace-space-types' +import type { Worktree } from '../../../../shared/worktree/types' +import { getSelectedDeletableWorkspaceRows } from './workspace-space-delete-selection' +import { + resolveWorktreeBatchDeleteTargets, + toWorktreeDeleteIdentities +} from '../sidebar/worktree-delete-request' + +const SHARED_ID = 'repo-1::/work/orca' +const LOCAL: ExecutionHostId = 'local' +const SSH: ExecutionHostId = 'ssh:build-box' + +function spaceRow(executionHostId: ExecutionHostId): WorkspaceSpaceWorktree { + return { + worktreeId: SHARED_ID, + executionHostId, + displayName: 'orca', + path: '/work/orca', + status: 'ok', + canDelete: true, + sizeBytes: 1 + } as WorkspaceSpaceWorktree +} + +function storeRow(hostId: ExecutionHostId): Worktree { + return { + id: SHARED_ID, + // A Space row has no instanceId; the store row does, and that is the one + // the confirmed-target check compares against. + instanceId: `instance-${hostId}`, + repoId: 'repo-1', + path: '/work/orca', + isMainWorktree: false, + hostId + } as Worktree +} + +const localStoreRow = storeRow(LOCAL) +const sshStoreRow = storeRow(SSH) + +/** Stands in for getWorktreeOnHostFromState: resolve on the named host. */ +function lookupOnHost(worktreeId: string, hostId: ExecutionHostId | undefined) { + const matches = [localStoreRow, sshStoreRow].filter((row) => row.id === worktreeId) + return hostId ? matches.find((row) => row.hostId === hostId) : matches[0] +} + +/** The panel's delete path: Space row -> store row on ITS host -> identity. */ +function identitiesForSpaceDelete(targets: readonly WorkspaceSpaceWorktree[]) { + return toWorktreeDeleteIdentities( + targets.flatMap((target) => { + const row = lookupOnHost(target.worktreeId, target.executionHostId ?? undefined) + return row ? [row] : [] + }) + ) +} + +describe('Space Manager delete routes to the row host', () => { + it('deletes the SSH row when the SSH row is the one clicked', () => { + const targets = resolveWorktreeBatchDeleteTargets( + identitiesForSpaceDelete([spaceRow(SSH)]), + lookupOnHost + ) + + // The local row is listed first, so a bare-id delete would resolve to it. + expect(targets?.map((row) => row.hostId)).toEqual([SSH]) + }) + + it('deletes the local row when the local row is the one clicked', () => { + const targets = resolveWorktreeBatchDeleteTargets( + identitiesForSpaceDelete([spaceRow(LOCAL)]), + lookupOnHost + ) + + expect(targets?.map((row) => row.hostId)).toEqual([LOCAL]) + }) + + it('carries the store row instanceId so the confirmed-target check accepts it', () => { + // Regression guard: hand-building {id, hostId} with no instanceId would make + // resolveWorktreeBatchDeleteTargets refuse every Space delete instead. + const identities = identitiesForSpaceDelete([spaceRow(SSH)]) + + expect(identities).toEqual([{ id: SHARED_ID, instanceId: `instance-${SSH}`, hostId: SSH }]) + expect(resolveWorktreeBatchDeleteTargets(identities, lookupOnHost)).not.toBeNull() + }) + + it('keeps each selected row on its own host in a multi-select delete', () => { + const selected = getSelectedDeletableWorkspaceRows( + [spaceRow(LOCAL), spaceRow(SSH)], + new Set([SHARED_ID]) + ) + // Both rows share the id, so a selection keyed by id alone would collapse them. + expect(selected).toHaveLength(2) + + const targets = resolveWorktreeBatchDeleteTargets( + identitiesForSpaceDelete(selected), + lookupOnHost + ) + + expect(targets?.map((row) => row.hostId)).toEqual([LOCAL, SSH]) + }) +}) diff --git a/src/renderer/src/components/status-bar/workspace-space-delete-selection.ts b/src/renderer/src/components/status-bar/workspace-space-delete-selection.ts new file mode 100644 index 00000000000..3ba84aebc5b --- /dev/null +++ b/src/renderer/src/components/status-bar/workspace-space-delete-selection.ts @@ -0,0 +1,44 @@ +import type { WorkspaceSpaceWorktree } from '../../../../shared/workspace-space-types' + +/** + * Which Space rows a delete action applies to. + * + * Rows are host-partitioned: two rows can share a `repoId::path` worktree id + * when the same repo is registered on two execution hosts. Anything feeding a + * destructive action therefore hands back ROWS, not ids — an id alone cannot say + * which host's checkout the user pointed at (STA-4343). + */ + +/** The selected rows themselves — each keeps its own host (STA-4343). */ +export function getSelectedDeletableWorkspaceRows( + rows: readonly WorkspaceSpaceWorktree[], + selectedIds: ReadonlySet, + isWorktreeDeleting: (worktreeId: string) => boolean = () => false +): WorkspaceSpaceWorktree[] { + return rows.filter( + (row) => + row.canDelete && + row.status === 'ok' && + selectedIds.has(row.worktreeId) && + !isWorktreeDeleting(row.worktreeId) + ) +} + +export function getSelectedDeletableWorkspaceIds( + rows: readonly WorkspaceSpaceWorktree[], + selectedIds: ReadonlySet, + isWorktreeDeleting: (worktreeId: string) => boolean = () => false +): string[] { + return getSelectedDeletableWorkspaceRows(rows, selectedIds, isWorktreeDeleting).map( + (row) => row.worktreeId + ) +} + +export function getVisibleDeletableWorkspaceIds( + rows: readonly WorkspaceSpaceWorktree[], + isWorktreeDeleting: (worktreeId: string) => boolean = () => false +): string[] { + return rows + .filter((row) => row.canDelete && row.status === 'ok' && !isWorktreeDeleting(row.worktreeId)) + .map((row) => row.worktreeId) +} diff --git a/src/renderer/src/components/status-bar/workspace-space-presentation.test.ts b/src/renderer/src/components/status-bar/workspace-space-presentation.test.ts index 1d600faed2a..e643749ff37 100644 --- a/src/renderer/src/components/status-bar/workspace-space-presentation.test.ts +++ b/src/renderer/src/components/status-bar/workspace-space-presentation.test.ts @@ -1,3 +1,7 @@ +import { + getSelectedDeletableWorkspaceIds, + getVisibleDeletableWorkspaceIds +} from './workspace-space-delete-selection' import { describe, expect, it } from 'vitest' import type { WorkspaceSpaceWorktree } from '../../../../shared/workspace-space-types' import { @@ -6,8 +10,6 @@ import { filterWorkspaceSpaceRows, getLargestWorkspaceSpaceItemSize, getLargestWorkspaceSpaceRowSize, - getSelectedDeletableWorkspaceIds, - getVisibleDeletableWorkspaceIds, getWorkspaceSpaceGitStatusRefreshCandidates, isWorkspaceSpaceFilterQueryTooLarge, isWorkspaceSpaceRowReadyToDelete, diff --git a/src/renderer/src/components/status-bar/workspace-space-presentation.ts b/src/renderer/src/components/status-bar/workspace-space-presentation.ts index 5aab2dcefc5..2b6d5847569 100644 --- a/src/renderer/src/components/status-bar/workspace-space-presentation.ts +++ b/src/renderer/src/components/status-bar/workspace-space-presentation.ts @@ -262,31 +262,6 @@ export function getWorkspaceSpaceGitStatusRefreshCandidates( ) } -export function getSelectedDeletableWorkspaceIds( - rows: readonly WorkspaceSpaceWorktree[], - selectedIds: ReadonlySet, - isWorktreeDeleting: (worktreeId: string) => boolean = () => false -): string[] { - return rows - .filter( - (row) => - row.canDelete && - row.status === 'ok' && - selectedIds.has(row.worktreeId) && - !isWorktreeDeleting(row.worktreeId) - ) - .map((row) => row.worktreeId) -} - -export function getVisibleDeletableWorkspaceIds( - rows: readonly WorkspaceSpaceWorktree[], - isWorktreeDeleting: (worktreeId: string) => boolean = () => false -): string[] { - return rows - .filter((row) => row.canDelete && row.status === 'ok' && !isWorktreeDeleting(row.worktreeId)) - .map((row) => row.worktreeId) -} - export function resolveWorkspaceSpaceInspectedWorktreeId( rows: readonly WorkspaceSpaceWorktree[], currentWorktreeId: string | null diff --git a/src/renderer/src/store/slices/workspace-cleanup-removal-targets.ts b/src/renderer/src/store/slices/workspace-cleanup-removal-targets.ts index 726341eb65e..c04d5c737f9 100644 --- a/src/renderer/src/store/slices/workspace-cleanup-removal-targets.ts +++ b/src/renderer/src/store/slices/workspace-cleanup-removal-targets.ts @@ -26,6 +26,9 @@ import { getWorktreeOperationOwnerHostIds } from '@/lib/worktree-operation-route import { translate } from '@/i18n/i18n' import type { WorkspaceCleanupFailure } from './workspace-cleanup' +/** Distinct from every ExecutionHostId, so a hostless row cannot alias one. */ +const UNQUALIFIED_HOST_BUCKET = Symbol('unqualified-cleanup-host') + export type WorkspaceCleanupRemovalTarget = { kind: 'target' worktreeId: string @@ -90,8 +93,17 @@ export function resolveWorkspaceCleanupRemovalTargets( const displayName = approved[0]?.displayName ?? worktreeId // Why: one id confirmed for two hosts in a single batch cannot say which // row the user meant, and picking either one may delete the other's work. + // + // Why the STRICT resolver: the display identity defaults a hostless row to + // `local`, so a hostless row and a genuine local row for the same id share an + // identity and this gate would not fire. Destructive code may not guess a + // host — an unqualified row is its own bucket here. if ( - new Set(approved.map((candidate) => getWorkspaceCleanupCandidateIdentity(candidate))).size > 1 + new Set( + approved.map( + (candidate) => resolveWorkspaceCleanupRemovalHostId(candidate) ?? UNQUALIFIED_HOST_BUCKET + ) + ).size > 1 ) { return ambiguousHostFailure(worktreeId, displayName) }