From c51a1617a798aa2ce4cb0fd6f0bcb23ab10a560a Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Fri, 14 Aug 2026 18:40:50 -0700 Subject: [PATCH] fix(cleanup): disambiguate hosts and scope removal lookup --- src/main/runtime/orca-runtime.test.ts | 1 + src/main/runtime/orca-runtime.ts | 21 +-- .../repo-worktree-row-resolution.test.ts | 161 ++++++++++++++++++ .../runtime/repo-worktree-row-resolution.ts | 15 +- .../workspace-cleanup-candidate-row.test.tsx | 52 +++++- .../workspace-cleanup-candidate-row.tsx | 22 ++- .../workspace-cleanup-confirm-remove.test.tsx | 42 +++++ .../workspace-cleanup-confirm-remove.tsx | 21 ++- .../workspace-cleanup-host-label.ts | 22 +++ 9 files changed, 328 insertions(+), 29 deletions(-) create mode 100644 src/main/runtime/repo-worktree-row-resolution.test.ts create mode 100644 src/renderer/src/components/workspace-cleanup/workspace-cleanup-host-label.ts diff --git a/src/main/runtime/orca-runtime.test.ts b/src/main/runtime/orca-runtime.test.ts index 97f166bfa6f..50b13d9b08b 100644 --- a/src/main/runtime/orca-runtime.test.ts +++ b/src/main/runtime/orca-runtime.test.ts @@ -2417,6 +2417,7 @@ describe('OrcaRuntimeService', () => { await expect( internals.resolveWorktreeRemovalTarget(TEST_WORKTREE_ID, 'runtime:env-b') ).rejects.toThrow('selector_not_found') + expect(internals.listResolvedWorktrees).not.toHaveBeenCalled() }) it('claims the first window as authoritative and ignores later windows', () => { diff --git a/src/main/runtime/orca-runtime.ts b/src/main/runtime/orca-runtime.ts index 63b0222e175..0f432caa78b 100644 --- a/src/main/runtime/orca-runtime.ts +++ b/src/main/runtime/orca-runtime.ts @@ -24582,25 +24582,13 @@ export class OrcaRuntimeService { private async resolveWorktreeRemovalTarget( worktreeSelector: string, - requiredHostId?: string + requiredHostId?: ExecutionHostId ): Promise { try { const exactTarget = parseExactWorktreeIdSelector(worktreeSelector) const worktree = exactTarget && requiredHostId - ? ((await this.listResolvedWorktrees()).find((candidate) => { - if (candidate.id !== exactTarget.id) { - return false - } - const repo = this.store - ?.getRepos() - .find( - (entry) => - entry.id === candidate.repoId && - getRepoExecutionHostId(entry) === requiredHostId - ) - return getWorktreeExecutionHostId(candidate, repo) === requiredHostId - }) ?? + ? ((await this.resolveExplicitWorktreeIdScoped(exactTarget.id, requiredHostId)) ?? (() => { throw new Error('selector_not_found') })()) @@ -29827,12 +29815,13 @@ export class OrcaRuntimeService { } private async resolveExplicitWorktreeIdScoped( - worktreeId: string + worktreeId: string, + requiredHostId?: ExecutionHostId ): Promise { if (!this.store) { return null } - return await resolveScopedWorktreeIdRow(this.repoWorktreeRowDeps(), worktreeId) + return await resolveScopedWorktreeIdRow(this.repoWorktreeRowDeps(), worktreeId, requiredHostId) } private async listRepoWorktreesForResolution( diff --git a/src/main/runtime/repo-worktree-row-resolution.test.ts b/src/main/runtime/repo-worktree-row-resolution.test.ts new file mode 100644 index 00000000000..6c8eb2834b6 --- /dev/null +++ b/src/main/runtime/repo-worktree-row-resolution.test.ts @@ -0,0 +1,161 @@ +import { describe, expect, it, vi } from 'vitest' +import type { ExecutionHostId } from '../../shared/execution-host' +import type { Repo } from '../../shared/repo-types' +import type { WorktreeMeta } from '../../shared/worktree/meta-types' +import type { GitWorktreeInfo, Worktree } from '../../shared/worktree/types' +import type { Store } from '../persistence' +import { + resolveScopedWorktreeIdRow, + type RepoWorktreeRowDeps +} from './repo-worktree-row-resolution' + +function repo( + id: string, + path: string, + options: Pick = {} +): Repo { + return { + id, + path, + displayName: id, + badgeColor: 'blue', + addedAt: 1, + ...options + } +} + +function gitWorktree(path: string): GitWorktreeInfo { + return { + path, + head: 'abc123', + branch: 'refs/heads/feature', + isBare: false, + isMainWorktree: false + } +} + +function createDeps(repos: Repo[]): RepoWorktreeRowDeps & { + scanRepo: ReturnType> + listFolderWorkspaces: ReturnType> +} { + const metaById: Record = {} + const store = { + getRepos: () => repos, + getAllWorktreeMeta: () => metaById, + getAllWorktreeLineage: () => ({}), + getProjects: () => [], + getSettings: () => ({}), + setWorktreeMeta: (worktreeId: string, updates: Partial) => { + const next = { ...metaById[worktreeId], ...updates } as WorktreeMeta + metaById[worktreeId] = next + return next + } + } as unknown as Store + const scanRepo = vi.fn(async (owner) => ({ + ok: true, + worktrees: [gitWorktree(owner.id === 'unrelated' ? '/unrelated/worktree' : '/same/worktree')] + })) + const listFolderWorkspaces = vi.fn(() => []) + return { store, scanRepo, listFolderWorkspaces } +} + +describe('host-qualified scoped worktree resolution', () => { + it('scans only the selected local or SSH owner when repo ids and paths collide', async () => { + const owners = [ + repo('shared', '/local/repo', { executionHostId: 'local' }), + repo('shared', '/remote/repo', { + connectionId: 'builder', + executionHostId: 'ssh:builder' + }), + repo('unrelated', '/unrelated/repo', { connectionId: 'slow-box' }) + ] + const deps = createDeps(owners) + const worktreeId = 'shared::/same/worktree' + + await expect( + resolveScopedWorktreeIdRow(deps, worktreeId, 'ssh:builder') + ).resolves.toMatchObject({ id: worktreeId, hostId: 'ssh:builder' }) + expect(deps.scanRepo.mock.calls.map(([owner]) => owner)).toEqual([owners[1]]) + + deps.scanRepo.mockClear() + await expect(resolveScopedWorktreeIdRow(deps, worktreeId, 'local')).resolves.toMatchObject({ + id: worktreeId, + hostId: 'local' + }) + expect(deps.scanRepo.mock.calls.map(([owner]) => owner)).toEqual([owners[0]]) + }) + + it.each([ + ['runtime:windows', String.raw`C:\Users\dev\orca worktree`], + ['local', '/mnt/c/Users/dev/orca worktree'] + ] satisfies [ExecutionHostId, string][])( + 'keeps %s path resolution scoped', + async (hostId, path) => { + const target = repo('shared', path, { executionHostId: hostId }) + const deps = createDeps([ + target, + repo('shared', '/other-host/repo', { executionHostId: 'ssh:unrelated' }), + repo('unrelated', '/unrelated/repo') + ]) + deps.scanRepo.mockImplementation(async () => ({ ok: true, worktrees: [gitWorktree(path)] })) + const worktreeId = `shared::${path}` + + await expect(resolveScopedWorktreeIdRow(deps, worktreeId, hostId)).resolves.toMatchObject({ + id: worktreeId, + hostId, + path + }) + expect(deps.scanRepo.mock.calls.map(([owner]) => owner)).toEqual([target]) + } + ) + + it('resolves only the selected folder-workspace owner without scanning Git providers', async () => { + const local = repo('folders', '/local/folders', { + kind: 'folder', + executionHostId: 'local' + }) + const remote = repo('folders', '/remote/folders', { + kind: 'folder', + connectionId: 'builder', + executionHostId: 'ssh:builder' + }) + const deps = createDeps([local, remote, repo('unrelated', '/unrelated/repo')]) + const worktreeId = 'folders::/shared/folder' + deps.listFolderWorkspaces.mockImplementation((owner) => + owner === remote + ? ([ + { + id: worktreeId, + repoId: owner.id, + path: '/shared/folder', + head: '', + branch: '', + isBare: false, + isMainWorktree: false, + displayName: 'folder', + comment: '', + hostId: 'ssh:builder' + } as unknown as Worktree + ] as Worktree[]) + : [] + ) + + await expect( + resolveScopedWorktreeIdRow(deps, worktreeId, 'ssh:builder') + ).resolves.toMatchObject({ id: worktreeId, hostId: 'ssh:builder' }) + expect(deps.listFolderWorkspaces).toHaveBeenCalledExactlyOnceWith(remote) + expect(deps.scanRepo).not.toHaveBeenCalled() + }) + + it('does not scan when the requested host does not own the repo id', async () => { + const deps = createDeps([ + repo('shared', '/local/repo', { executionHostId: 'local' }), + repo('unrelated', '/unrelated/repo', { executionHostId: 'runtime:other' }) + ]) + + await expect( + resolveScopedWorktreeIdRow(deps, 'shared::/same/worktree', 'ssh:builder') + ).resolves.toBeNull() + expect(deps.scanRepo).not.toHaveBeenCalled() + }) +}) diff --git a/src/main/runtime/repo-worktree-row-resolution.ts b/src/main/runtime/repo-worktree-row-resolution.ts index 24ca1c36293..50ed16ed539 100644 --- a/src/main/runtime/repo-worktree-row-resolution.ts +++ b/src/main/runtime/repo-worktree-row-resolution.ts @@ -1,5 +1,5 @@ import { splitWorktreeId, splitWorktreeIdForFilesystem } from '../../shared/worktree/id' -import { getRepoExecutionHostId } from '../../shared/execution-host' +import { getRepoExecutionHostId, type ExecutionHostId } from '../../shared/execution-host' import { isFolderRepo } from '../../shared/repo-kind' import { projectResolvedWorktreeLineage } from '../../shared/resolved-worktree-lineage' import { withTimeout } from '../../shared/promise-timeout-fallback' @@ -157,16 +157,23 @@ export async function resolveRepoWorktreeRows( */ export async function resolveScopedWorktreeIdRow( deps: RepoWorktreeRowDeps, - worktreeId: string + worktreeId: string, + requiredHostId?: ExecutionHostId ): Promise { const { store } = deps const parsed = splitWorktreeIdForFilesystem(worktreeId) if (!parsed?.repoId || !parsed.worktreePath) { return null } - const owners = store.getRepos().filter((repo) => repo.id === parsed.repoId) + const owners = store + .getRepos() + .filter( + (repo) => + repo.id === parsed.repoId && + (requiredHostId === undefined || getRepoExecutionHostId(repo) === requiredHostId) + ) // Why: one repo id can be registered on several execution hosts, and only the fleet scan decides - // between their rows. Hand those back to the unscoped path rather than guessing. + // between unqualified rows. A host qualifier narrows the same-id set without scanning other owners. if (owners.length !== 1) { return null } diff --git a/src/renderer/src/components/workspace-cleanup/workspace-cleanup-candidate-row.test.tsx b/src/renderer/src/components/workspace-cleanup/workspace-cleanup-candidate-row.test.tsx index f7cd1ac46f9..d9deb16e8e9 100644 --- a/src/renderer/src/components/workspace-cleanup/workspace-cleanup-candidate-row.test.tsx +++ b/src/renderer/src/components/workspace-cleanup/workspace-cleanup-candidate-row.test.tsx @@ -5,6 +5,7 @@ import { createRoot, type Root } from 'react-dom/client' import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' import { getWorkspaceCleanupCandidateIdentity } from './workspace-cleanup-host-identity' import { CandidateRow } from './workspace-cleanup-candidate-row' +import { getWorkspaceCleanupCandidateAccessibleName } from './workspace-cleanup-host-label' import { makeCandidate } from './workspace-cleanup-presentation-fixtures' vi.mock('@/components/ui/tooltip', () => ({ @@ -128,7 +129,9 @@ describe('CandidateRow', () => { ) }) - const openButton = container?.querySelector(`[aria-label="Open ${candidate.displayName}"]`) + const openButton = container?.querySelector( + `[aria-label="Open ${getWorkspaceCleanupCandidateAccessibleName(candidate)}"]` + ) expect(openButton?.querySelector('.lucide-external-link')).not.toBeNull() }) @@ -161,8 +164,9 @@ describe('CandidateRow', () => { ) }) - expect(container?.querySelector(`[aria-label="Select ${candidate.displayName}"]`)).toBeNull() - expect(container?.querySelector(`[aria-label="Remove ${candidate.displayName}"]`)).toBeNull() + const accessibleName = getWorkspaceCleanupCandidateAccessibleName(candidate) + expect(container?.querySelector(`[aria-label="Select ${accessibleName}"]`)).toBeNull() + expect(container?.querySelector(`[aria-label="Remove ${accessibleName}"]`)).toBeNull() }) it.each([ @@ -201,4 +205,46 @@ describe('CandidateRow', () => { expect(container?.textContent).toContain(label) expect(container?.textContent).not.toContain('Ready') }) + + it('visibly and accessibly distinguishes colliding local and SSH rows', () => { + const local = makeCandidate({ executionHostId: 'local' }) + const remote = makeCandidate({ connectionId: 'builder', executionHostId: 'ssh:builder' }) + + act(() => { + root?.render( + <> + {[local, remote].map((candidate) => ( + + ))} + + ) + }) + + const localName = getWorkspaceCleanupCandidateAccessibleName(local) + const remoteName = getWorkspaceCleanupCandidateAccessibleName(remote) + expect(localName).not.toBe(remoteName) + expect(container?.querySelector(`[aria-label="Select ${localName}"]`)).not.toBeNull() + expect(container?.querySelector(`[aria-label="Select ${remoteName}"]`)).not.toBeNull() + expect(container?.querySelector(`[aria-label="Host: builder"]`)).not.toBeNull() + }) }) diff --git a/src/renderer/src/components/workspace-cleanup/workspace-cleanup-candidate-row.tsx b/src/renderer/src/components/workspace-cleanup/workspace-cleanup-candidate-row.tsx index 4beedf3bdd3..d5a7416914e 100644 --- a/src/renderer/src/components/workspace-cleanup/workspace-cleanup-candidate-row.tsx +++ b/src/renderer/src/components/workspace-cleanup/workspace-cleanup-candidate-row.tsx @@ -12,6 +12,7 @@ import { GitPullRequest, HardDrive, Loader2, + Monitor, SquareTerminal, Trash2 } from 'lucide-react' @@ -41,6 +42,10 @@ import { } from './workspace-cleanup-candidate-row-data' import { StatusPill } from './workspace-cleanup-status-pill' import { WorkspaceCleanupMetadataChip } from './workspace-cleanup-metadata-chip' +import { + getWorkspaceCleanupCandidateAccessibleName, + getWorkspaceCleanupCandidateHostLabel +} from './workspace-cleanup-host-label' export type WorkspaceCleanupDeletionPhase = 'deleting' | 'queued' @@ -107,6 +112,8 @@ export const CandidateRow = React.memo(function CandidateRow({ const gitLabel = getWorkspaceCleanupGitLabel(candidate) const showGitMetadataChip = shouldShowGitMetadataChip(candidate) const contextCount = getContextCount(candidate) + const candidateAccessibleName = getWorkspaceCleanupCandidateAccessibleName(candidate) + const hostLabel = getWorkspaceCleanupCandidateHostLabel(candidate) const sizeValue = sizeLabel ?? translate('components.workspace.cleanup.browse.notMeasured', 'Not measured') const hasExpandableDetails = @@ -134,7 +141,7 @@ export const CandidateRow = React.memo(function CandidateRow({ aria-label={translate( 'auto.components.workspace.cleanup.WorkspaceCleanupDialog.bbb1ab6a6f', 'Select {{value0}}', - { value0: candidate.displayName } + { value0: candidateAccessibleName } )} onClick={() => onToggleSelected(identity)} className="mt-0.5 flex size-4 shrink-0 items-center justify-center rounded border border-border bg-background text-primary hover:bg-accent focus-visible:outline-none focus-visible:ring-1 focus-visible:ring-ring" @@ -152,6 +159,13 @@ export const CandidateRow = React.memo(function CandidateRow({ {candidate.displayName} + {deletionPhase ? ( {deletionPhase === 'queued' @@ -303,7 +317,7 @@ export const CandidateRow = React.memo(function CandidateRow({ aria-label={translate( 'components.workspace.cleanup.browse.openWorkspaceNamed', 'Open {{value0}}', - { value0: candidate.displayName } + { value0: candidateAccessibleName } )} onClick={() => onView(candidate)} > @@ -323,7 +337,7 @@ export const CandidateRow = React.memo(function CandidateRow({ aria-label={translate( 'auto.components.workspace.cleanup.WorkspaceCleanupDialog.a9957007eb', 'Ignore {{value0}}', - { value0: candidate.displayName } + { value0: candidateAccessibleName } )} onClick={() => onIgnore(candidate)} > @@ -347,7 +361,7 @@ export const CandidateRow = React.memo(function CandidateRow({ aria-label={translate( 'auto.components.workspace.cleanup.WorkspaceCleanupDialog.3828408538', 'Remove {{value0}}', - { value0: candidate.displayName } + { value0: candidateAccessibleName } )} className="text-destructive hover:text-destructive" onClick={() => onRemove(candidate)} diff --git a/src/renderer/src/components/workspace-cleanup/workspace-cleanup-confirm-remove.test.tsx b/src/renderer/src/components/workspace-cleanup/workspace-cleanup-confirm-remove.test.tsx index f93672103c8..0a7014cf1b2 100644 --- a/src/renderer/src/components/workspace-cleanup/workspace-cleanup-confirm-remove.test.tsx +++ b/src/renderer/src/components/workspace-cleanup/workspace-cleanup-confirm-remove.test.tsx @@ -4,6 +4,10 @@ import { createRoot, type Root } from 'react-dom/client' import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' import { makeFacetCandidate } from './workspace-cleanup-facet.test.fixture' import { WorkspaceCleanupConfirmRemove } from './workspace-cleanup-confirm-remove' +import { + getWorkspaceCleanupCandidateAccessibleName, + getWorkspaceCleanupCandidateHostLabel +} from './workspace-cleanup-host-label' const listProbe = vi.hoisted(() => vi.fn()) @@ -31,6 +35,12 @@ vi.mock('@/components/ui/scroll-area', () => ({ ScrollArea: ({ children }: { children: ReactNode }) =>
{children}
})) +vi.mock('@/components/ui/tooltip', () => ({ + Tooltip: ({ children }: { children: ReactNode }) => <>{children}, + TooltipContent: ({ children }: { children: ReactNode }) => <>{children}, + TooltipTrigger: ({ children }: { children: ReactNode }) => <>{children} +})) + let container: HTMLDivElement let root: Root @@ -73,4 +83,36 @@ describe('WorkspaceCleanupConfirmRemove', () => { expect(listProbe).toHaveBeenCalledWith(candidates.length) expect(container.querySelectorAll('span.truncate.text-sm.font-medium')).toHaveLength(5) }) + + it('visibly and accessibly identifies each host for colliding confirmations', () => { + const local = makeFacetCandidate({ executionHostId: 'local' }) + const remote = makeFacetCandidate({ + connectionId: 'builder', + executionHostId: 'ssh:builder' + }) + + act(() => { + root.render( + + ) + }) + + expect(getWorkspaceCleanupCandidateAccessibleName(local)).not.toBe( + getWorkspaceCleanupCandidateAccessibleName(remote) + ) + for (const candidate of [local, remote]) { + const name = getWorkspaceCleanupCandidateAccessibleName(candidate) + const hostLabel = getWorkspaceCleanupCandidateHostLabel(candidate) + expect(container.querySelector(`[role="group"][aria-label="${name}"]`)).not.toBeNull() + expect(container.querySelector(`[aria-label="Host: ${hostLabel}"]`)).not.toBeNull() + } + }) }) diff --git a/src/renderer/src/components/workspace-cleanup/workspace-cleanup-confirm-remove.tsx b/src/renderer/src/components/workspace-cleanup/workspace-cleanup-confirm-remove.tsx index 7f49197efce..7c0cb8d9ef1 100644 --- a/src/renderer/src/components/workspace-cleanup/workspace-cleanup-confirm-remove.tsx +++ b/src/renderer/src/components/workspace-cleanup/workspace-cleanup-confirm-remove.tsx @@ -1,5 +1,5 @@ import React from 'react' -import { AlertTriangle, Loader2, Trash2, X } from 'lucide-react' +import { AlertTriangle, Loader2, Monitor, Trash2, X } from 'lucide-react' import { Button } from '@/components/ui/button' import { DialogDescription, DialogFooter, DialogHeader, DialogTitle } from '@/components/ui/dialog' import { Progress } from '@/components/ui/progress' @@ -24,6 +24,11 @@ import { getWorkspaceCleanupCandidateIdentity, getWorkspaceCleanupHostIdentity } from './workspace-cleanup-host-identity' +import { + getWorkspaceCleanupCandidateAccessibleName, + getWorkspaceCleanupCandidateHostLabel +} from './workspace-cleanup-host-label' +import { WorkspaceCleanupMetadataChip } from './workspace-cleanup-metadata-chip' const CONFIRM_REMOVE_ROW_ESTIMATE_PX = 76 @@ -201,10 +206,22 @@ function ConfirmRemoveRow({ const contextPillLabel = getContextPillLabel(candidate) const showGitMetadataChip = shouldShowGitMetadataChip(candidate) const factStatus = getCandidateFactStatus(candidate) + const hostLabel = getWorkspaceCleanupCandidateHostLabel(candidate) return ( -
+
{candidate.displayName} + {translate( 'auto.components.workspace.cleanup.WorkspaceCleanupDialog.352f15d6fc', diff --git a/src/renderer/src/components/workspace-cleanup/workspace-cleanup-host-label.ts b/src/renderer/src/components/workspace-cleanup/workspace-cleanup-host-label.ts new file mode 100644 index 00000000000..9c3d4bcb5b9 --- /dev/null +++ b/src/renderer/src/components/workspace-cleanup/workspace-cleanup-host-label.ts @@ -0,0 +1,22 @@ +import { translate } from '@/i18n/i18n' +import { getExecutionHostLabel } from '../../../../shared/execution-host' +import type { WorkspaceCleanupCandidate } from '../../../../shared/workspace-cleanup' +import { resolveWorkspaceCleanupRemovalHostId } from './workspace-cleanup-host-identity' + +export function getWorkspaceCleanupCandidateHostLabel( + candidate: WorkspaceCleanupCandidate +): string { + const hostId = resolveWorkspaceCleanupRemovalHostId(candidate) + return hostId + ? getExecutionHostLabel(hostId) + : translate('components.workspace.cleanup.host.unknown', 'Unknown host') +} + +export function getWorkspaceCleanupCandidateAccessibleName( + candidate: WorkspaceCleanupCandidate +): string { + return translate('components.workspace.cleanup.host.candidateName', '{{value0}} on {{value1}}', { + value0: candidate.displayName, + value1: getWorkspaceCleanupCandidateHostLabel(candidate) + }) +}