mirror of
https://github.com/stablyai/orca.git
synced 2026-10-22 08:03:37 +00:00
fix(space): route the ordinary Space delete to the row's host (STA-4343)
WorkspaceSpaceManagerPanel's force path already carried executionHostId; the ordinary Delete one line above it passed a bare worktree id, as did deleteSelected. The Space scan lists one row per host, so clicking Delete on the SSH row resolved through the id-keyed lookup and destroyed the LOCAL checkout — the exact defect this PR exists to fix, still reachable from a second surface. A Space row carries no instanceId, so a hand-built identity would be rejected as stale by the confirmed-target check and turn a wrong-host delete into a total refusal. The row is resolved on its own host first and the STORE row's identity is what travels. Selection keeps the rows, not just their ids, so a multi-select spanning two hosts no longer collapses to one. The delete-selection helpers move to their own module to stay under the 300-line cap (no max-lines disable). Also tightens the cleanup batch ambiguity gate: it keyed on the display identity, which defaults a hostless row to `local`, so a hostless row and a genuine local row for the same id shared an identity and the gate could not fire. It now keys on the strict resolver, where an unqualified row is its own bucket.
This commit is contained in:
@@ -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)}
|
||||
/>
|
||||
))
|
||||
|
||||
@@ -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])
|
||||
})
|
||||
})
|
||||
@@ -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<string>,
|
||||
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<string>,
|
||||
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)
|
||||
}
|
||||
@@ -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,
|
||||
|
||||
@@ -262,31 +262,6 @@ export function getWorkspaceSpaceGitStatusRefreshCandidates(
|
||||
)
|
||||
}
|
||||
|
||||
export function getSelectedDeletableWorkspaceIds(
|
||||
rows: readonly WorkspaceSpaceWorktree[],
|
||||
selectedIds: ReadonlySet<string>,
|
||||
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
|
||||
|
||||
@@ -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)
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user