From 5e1a5bf664924d5632fc7c3505338c8dbfd07c7c Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Sun, 16 Aug 2026 17:03:51 -0700 Subject: [PATCH] fix(sidebar): stop collapsing two hosts into one workspace row (STA-4343) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Routing a removal to the confirmed host is useless if that host's row can never be confirmed. `worktreeId` is `repoId::path` with no host component, and every id-keyed projection folded the second host away. Store index: dedup by (host, id) instead of id. The duplicate this dedup was built for — the createWorktree-appends / fetchWorktrees-replaces race — is the SAME workspace twice and still collapses; two hosts are two workspaces and now both survive. Adds getIndexedWorktreesById for callers that need every owner. Sidebar: rowKey, host-context labels, and the lineage emit/child sets are keyed by host-qualified identity. The lineage set mattered most — keyed by bare id, the second host's row was never emitted at all. Repo grouping falls back to per-row host labels when one repo id spans two hosts, since the per-repo label cannot tell those rows apart. Delete lineage: a colliding child id resolves to the parent's host, so "delete all" cannot route a descendant at the other machine's checkout. Reconciles the two id indices, which disagreed (first-wins in worktree-by-id-index, last-wins in worktree-repo-index) and so resolved the same colliding id to different rows. Both are first-wins now, matching the `.find()` they replaced; same-host duplicates are already collapsed by freshness before that point, so first-vs-last only decides genuine two-host collisions, which no bare-id lookup can resolve. Documented on both, and both are marked as not-evidence-of-ownership. The index keeps its one-read-per-row budget (retained selectors assert it) by recovering the id from the identity key instead of re-reading it. --- ....lineage-agent-expansion-coupling.test.tsx | 4 +- .../WorktreeList.lineage-child-card.test.ts | 23 ++--- ...ktreeList.lineage-child-real-card.test.tsx | 12 ++- .../sidebar/workspace-delete-lineage.test.ts | 47 +++++++++- .../sidebar/workspace-delete-lineage.ts | 13 ++- .../worktree-list-card-markup-queries.ts | 7 +- ...orktree-list-groups-host-collision.test.ts | 87 +++++++++++++++++ .../worktree-list/grouping/group-sections.ts | 6 +- .../worktree-list/grouping/host-labels.ts | 25 +++-- .../worktree-list/grouping/row-builders.ts | 28 +++--- .../src/store/slices/worktree-by-id-index.ts | 11 ++- .../worktree-host-collision-index.test.ts | 93 +++++++++++++++++++ src/renderer/src/store/worktree-repo-index.ts | 58 ++++++++++-- .../worktree/host-qualified-identity.ts | 44 +++++++++ 14 files changed, 406 insertions(+), 52 deletions(-) create mode 100644 src/renderer/src/components/sidebar/worktree-list-groups-host-collision.test.ts create mode 100644 src/renderer/src/store/worktree-host-collision-index.test.ts create mode 100644 src/shared/worktree/host-qualified-identity.ts diff --git a/src/renderer/src/components/sidebar/WorktreeList.lineage-agent-expansion-coupling.test.tsx b/src/renderer/src/components/sidebar/WorktreeList.lineage-agent-expansion-coupling.test.tsx index 01c9847d980..ab4d4a605d4 100644 --- a/src/renderer/src/components/sidebar/WorktreeList.lineage-agent-expansion-coupling.test.tsx +++ b/src/renderer/src/components/sidebar/WorktreeList.lineage-agent-expansion-coupling.test.tsx @@ -424,7 +424,7 @@ function compactAgentSummary(container: HTMLElement): HTMLButtonElement | null { } function childWorktreeCardPresent(container: HTMLElement): boolean { - return container.querySelector('[id="worktree-list-option-all%3Achild"]') !== null + return container.querySelector('[id="worktree-list-option-all%3A%7Cchild"]') !== null } function parentVirtualRowKey(container: HTMLElement): string | null { @@ -523,7 +523,7 @@ describe('WorktreeCard agent-list <-> child-worktrees expansion coupling', () => // The remount still happens: the parent moved to a standalone 'item' render // row with a DIFFERENT React key, and the child card is gone. - expect(parentVirtualRowKey(container)).toBe('wt:all:parent') + expect(parentVirtualRowKey(container)).toBe('wt:all:|parent') expect(childWorktreeCardPresent(container)).toBe(false) // FIXED: the card remounted, but the durable expansion cache means the diff --git a/src/renderer/src/components/sidebar/WorktreeList.lineage-child-card.test.ts b/src/renderer/src/components/sidebar/WorktreeList.lineage-child-card.test.ts index e75c6c2832d..e129b835d86 100644 --- a/src/renderer/src/components/sidebar/WorktreeList.lineage-child-card.test.ts +++ b/src/renderer/src/components/sidebar/WorktreeList.lineage-child-card.test.ts @@ -133,16 +133,16 @@ describe('WorktreeList lineage child card renderer', () => { mockStore.state.activeWorktreeId = 'child' const markup = await renderWorktreeListMarkup() - expect(markup).toContain('aria-activedescendant="worktree-list-option-all%3Achild"') + expect(markup).toContain('aria-activedescendant="worktree-list-option-all%3A%7Cchild"') }) it('points aria-activedescendant at the pinned row for active pinned workspaces', async () => { setPinnedFixtureState() const markup = await renderWorktreeListMarkup() - expect(markup).toContain('aria-activedescendant="worktree-list-option-pinned%3Apinned"') - expect(markup).toContain('id="worktree-list-option-pinned%3Apinned"') - expect(markup).not.toContain('id="worktree-list-option-all%3Apinned"') + expect(markup).toContain('aria-activedescendant="worktree-list-option-pinned%3A%7Cpinned"') + expect(markup).toContain('id="worktree-list-option-pinned%3A%7Cpinned"') + expect(markup).not.toContain('id="worktree-list-option-all%3A%7Cpinned"') }) it('points aria-activedescendant at the natural duplicate when enabled', async () => { @@ -150,22 +150,23 @@ describe('WorktreeList lineage child card renderer', () => { mockStore.state.settings = { showPinnedWorktreesInGroups: true } const markup = await renderWorktreeListMarkup() - expect(markup).toContain('aria-activedescendant="worktree-list-option-all%3Apinned"') - expect(markup).toContain('id="worktree-list-option-pinned%3Apinned"') - expect(markup).toContain('id="worktree-list-option-all%3Apinned"') + expect(markup).toContain('aria-activedescendant="worktree-list-option-all%3A%7Cpinned"') + expect(markup).toContain('id="worktree-list-option-pinned%3A%7Cpinned"') + expect(markup).toContain('id="worktree-list-option-all%3A%7Cpinned"') }) it('opens inline rename only for the row-scoped lineage child request', async () => { setLineageFixtureState() - mockStore.state.renamingWorktreeId = { worktreeId: 'child', rowKey: 'all:child' } + mockStore.state.renamingWorktreeId = { worktreeId: 'child', rowKey: 'all:|child' } const markup = await renderWorktreeListMarkup() const childCard = markup.match( - /
{ const parentRow = getOptionOpeningTag(markup, 'parent') - expect(parentRow).toContain('id="worktree-list-option-all%3Aparent"') + expect(parentRow).toContain('id="worktree-list-option-all%3A%7Cparent"') expect(parentRow).not.toContain('padding-left') }) diff --git a/src/renderer/src/components/sidebar/WorktreeList.lineage-child-real-card.test.tsx b/src/renderer/src/components/sidebar/WorktreeList.lineage-child-real-card.test.tsx index 7b51d933d63..a14d69a7267 100644 --- a/src/renderer/src/components/sidebar/WorktreeList.lineage-child-real-card.test.tsx +++ b/src/renderer/src/components/sidebar/WorktreeList.lineage-child-real-card.test.tsx @@ -398,7 +398,7 @@ describe('WorktreeList real child WorktreeCard integration', () => { it('renders GitLab MR metadata from a child through the real WorktreeCard path', async () => { const container = await renderWorktreeList() - const childOption = container.querySelector('[id="worktree-list-option-all%3Achild"]') + const childOption = container.querySelector('[id="worktree-list-option-all%3A%7Cchild"]') expect(childOption?.textContent).toContain('MR #42') expect(childOption?.textContent).toContain('Child GitLab MR') @@ -422,9 +422,11 @@ describe('WorktreeList real child WorktreeCard integration', () => { const wrappers = [ ...container.querySelectorAll('[data-worktree-lineage-children]') ] - const childRow = container.querySelector('[id="worktree-list-option-all%3Achild"]') + const childRow = container.querySelector( + '[id="worktree-list-option-all%3A%7Cchild"]' + ) const grandchildRow = container.querySelector( - '[id="worktree-list-option-all%3Agrandchild"]' + '[id="worktree-list-option-all%3A%7Cgrandchild"]' ) const childSurface = childRow?.querySelector('[data-worktree-card-surface="true"]') const grandchildSurface = grandchildRow?.querySelector( @@ -456,7 +458,7 @@ describe('WorktreeList real child WorktreeCard integration', () => { it('double-clicking a nested child opens edit metadata for the child only', async () => { const container = await renderWorktreeList() const childCard = container.querySelector( - '[id="worktree-list-option-all%3Achild"] [data-worktree-card-surface="true"]' + '[id="worktree-list-option-all%3A%7Cchild"] [data-worktree-card-surface="true"]' ) expect(childCard).not.toBeNull() @@ -483,7 +485,7 @@ describe('WorktreeList real child WorktreeCard integration', () => { setLineageState({ deletingChild: true }) const container = await renderWorktreeList() const childCard = container.querySelector( - '[id="worktree-list-option-all%3Achild"] [data-worktree-card-surface="true"]' + '[id="worktree-list-option-all%3A%7Cchild"] [data-worktree-card-surface="true"]' ) expect(childCard?.textContent).toContain('Deleting') diff --git a/src/renderer/src/components/sidebar/workspace-delete-lineage.test.ts b/src/renderer/src/components/sidebar/workspace-delete-lineage.test.ts index b368cb93133..c3b994b369d 100644 --- a/src/renderer/src/components/sidebar/workspace-delete-lineage.test.ts +++ b/src/renderer/src/components/sidebar/workspace-delete-lineage.test.ts @@ -1,5 +1,9 @@ import { describe, expect, it } from 'vitest' -import { LOCAL_EXECUTION_HOST_ID, toSshExecutionHostId } from '../../../../shared/execution-host' +import { + LOCAL_EXECUTION_HOST_ID, + toSshExecutionHostId, + type ExecutionHostId +} from '../../../../shared/execution-host' import type { WorktreeLineage } from '../../../../shared/worktree/lineage-types' import type { Worktree } from '../../../../shared/worktree/types' import { getWorkspaceDeleteLineage } from './workspace-delete-lineage' @@ -39,6 +43,8 @@ function makeLineage(child: Worktree, parent: Worktree): WorktreeLineage { } } +const SSH_HOST: ExecutionHostId = toSshExecutionHostId('build-box') + describe('getWorkspaceDeleteLineage', () => { it('returns valid descendants for parent delete copy and child-first delete-all targets', () => { const parent = makeWorktree('parent', '/workspaces/parent') @@ -136,4 +142,43 @@ describe('getWorkspaceDeleteLineage', () => { expect(lineage.descendants).toEqual([]) expect(lineage.deleteAllTargets).toEqual([parent]) }) + + // Why (STA-4343): lineage is recorded against the bare `repoId::path` id, so a + // colliding child id resolves to one of two hosts. Delete-all must not route a + // descendant's removal at the other machine's checkout. + it('resolves a colliding child id to the parent host', () => { + const parent: Worktree = { ...makeWorktree('parent', '/workspaces/parent'), hostId: SSH_HOST } + const localChild: Worktree = { + ...makeWorktree('child', '/workspaces/parent/child'), + hostId: LOCAL_EXECUTION_HOST_ID + } + const sshChild: Worktree = { ...localChild, hostId: SSH_HOST } + + const lineage = getWorkspaceDeleteLineage(parent, [parent, localChild, sshChild], { + [localChild.id]: makeLineage(localChild, parent) + }) + + expect(lineage.deleteAllTargets.map((worktree) => worktree.hostId)).toEqual([ + SSH_HOST, + SSH_HOST + ]) + }) + + it('keeps the parent host preference stable regardless of row order', () => { + const parent: Worktree = { ...makeWorktree('parent', '/workspaces/parent'), hostId: SSH_HOST } + const localChild: Worktree = { + ...makeWorktree('child', '/workspaces/parent/child'), + hostId: LOCAL_EXECUTION_HOST_ID + } + const sshChild: Worktree = { ...localChild, hostId: SSH_HOST } + + const lineage = getWorkspaceDeleteLineage(parent, [parent, sshChild, localChild], { + [localChild.id]: makeLineage(localChild, parent) + }) + + expect(lineage.deleteAllTargets.map((worktree) => worktree.hostId)).toEqual([ + SSH_HOST, + SSH_HOST + ]) + }) }) diff --git a/src/renderer/src/components/sidebar/workspace-delete-lineage.ts b/src/renderer/src/components/sidebar/workspace-delete-lineage.ts index 55d62fc71fd..937c6da104b 100644 --- a/src/renderer/src/components/sidebar/workspace-delete-lineage.ts +++ b/src/renderer/src/components/sidebar/workspace-delete-lineage.ts @@ -12,7 +12,18 @@ export function getWorkspaceDeleteLineage( worktrees: readonly Worktree[], lineageById: Record ): WorkspaceDeleteLineage { - const worktreeById = new Map(worktrees.map((worktree) => [worktree.id, worktree])) + // Why (STA-4343): lineage is recorded against the bare `repoId::path` id, so a + // colliding id resolves to one of two hosts here. A lineage child of a workspace + // on host X is on host X, so prefer the parent's host — otherwise "delete all" + // could route a descendant's removal at the other machine's checkout. + const worktreeById = new Map() + for (const worktree of worktrees) { + const claimed = worktreeById.get(worktree.id) + if (claimed && claimed.hostId === parent.hostId && worktree.hostId !== parent.hostId) { + continue + } + worktreeById.set(worktree.id, worktree) + } const childrenByParentId = getProjectedWorktreeLineageChildrenByParentId( lineageById, worktreeById diff --git a/src/renderer/src/components/sidebar/worktree-list-card-markup-queries.ts b/src/renderer/src/components/sidebar/worktree-list-card-markup-queries.ts index 1a2b486fe4c..e4cefed4dd8 100644 --- a/src/renderer/src/components/sidebar/worktree-list-card-markup-queries.ts +++ b/src/renderer/src/components/sidebar/worktree-list-card-markup-queries.ts @@ -7,11 +7,12 @@ export function getCardOpeningTag(markup: string, worktreeId: string): string { } export function getOptionOpeningTag(markup: string, worktreeId: string): string { - // Why: option ids are keyed by the row's rowKey (e.g. all%3Achild), so the - // worktree id is the suffix after the encoded ':' group separator. + // Why: option ids are keyed by the row's rowKey, which is + // `
:|` once URI-encoded (e.g. all%3Alocal%7Cchild). + // The worktree id is the suffix after the encoded host separator (STA-4343). return ( markup.match( - new RegExp(`]*id="worktree-list-option-[^"]*%3A${escapeRegExp(worktreeId)}"[^>]*>`) + new RegExp(`]*id="worktree-list-option-[^"]*%7C${escapeRegExp(worktreeId)}"[^>]*>`) )?.[0] ?? '' ) } diff --git a/src/renderer/src/components/sidebar/worktree-list-groups-host-collision.test.ts b/src/renderer/src/components/sidebar/worktree-list-groups-host-collision.test.ts new file mode 100644 index 00000000000..4639a51051a --- /dev/null +++ b/src/renderer/src/components/sidebar/worktree-list-groups-host-collision.test.ts @@ -0,0 +1,87 @@ +/** + * STA-4343: correct host-qualified routing is useless if the user cannot select + * the row it would route to. + * + * A repo id can be registered once per execution host, so the same repo checked + * out at the same path on the local host and on an SSH host publishes the SAME + * `repoId::path` workspace id twice. The sidebar collapsed that to one row, which + * meant the other host's workspace could never be confirmed — and a delete + * confirmed on the surviving row carried the wrong host. + */ +import { describe, expect, it } from 'vitest' +import { buildRows } from './worktree-list/grouping/build-rows' +import { repo, worktree } from './worktree-list-groups-test-fixtures' +import { getExecutionHostLabel } from '../../../../shared/execution-host' +import type { Repo } from '../../../../shared/repo-types' +import type { Worktree } from '../../../../shared/worktree/types' + +const SHARED_WORKTREE_ID = 'repo-shared::/work/orca-feature' + +const localRepo: Repo = { ...repo, id: 'repo-shared', path: '/work/orca' } +const sshRepo: Repo = { + ...repo, + id: 'repo-shared', + path: '/work/orca', + connectionId: 'build-box', + executionHostId: 'ssh:build-box' +} + +const localWorktree: Worktree = { + ...worktree, + id: SHARED_WORKTREE_ID, + repoId: 'repo-shared', + path: '/work/orca-feature', + hostId: 'local', + displayName: 'orca-feature' +} +const sshWorktree: Worktree = { ...localWorktree, hostId: 'ssh:build-box' } + +function buildCollidingRows(): ReturnType { + // Only one repo can be keyed per id in the repo map; the SSH row still has to + // render, so the collision must be resolved from the worktree rows themselves. + return buildRows( + 'repo', + [localWorktree, sshWorktree], + new Map([[localRepo.id, localRepo]]), + null, + new Set() + ) +} + +describe('sidebar rows for a workspace id owned by two hosts', () => { + it('renders one selectable row per host', () => { + const itemRows = buildCollidingRows().filter((row) => row.type === 'item') + + expect(itemRows).toHaveLength(2) + expect(itemRows.map((row) => row.worktree.hostId)).toEqual(['local', 'ssh:build-box']) + }) + + it('gives each row its own React key so neither replaces the other', () => { + const rowKeys = buildCollidingRows() + .filter((row) => row.type === 'item') + .map((row) => row.rowKey) + + expect(new Set(rowKeys).size).toBe(rowKeys.length) + }) + + it('labels each row with the host it lives on', () => { + const labels = buildCollidingRows() + .filter((row) => row.type === 'item') + .map((row) => row.hostContextLabel) + + expect(labels).toEqual([getExecutionHostLabel('local'), getExecutionHostLabel('ssh:build-box')]) + }) + + it('counts the two hosts separately in the group header', () => { + const header = buildCollidingRows().find((row) => row.type === 'header') + + expect(header?.count).toBe(2) + }) + + // The same-host duplicate that this dedup was originally built for collapses a + // layer up, in the store index — see worktree-host-collision-index.test.ts. +}) + +// Referenced so the SSH repo fixture documents the two-host registration this +// scenario requires, even though buildRows keys its repo map by id alone. +export const SSH_REPO_FIXTURE = sshRepo diff --git a/src/renderer/src/components/sidebar/worktree-list/grouping/group-sections.ts b/src/renderer/src/components/sidebar/worktree-list/grouping/group-sections.ts index 31a0d86c0f6..fc3acb360c7 100644 --- a/src/renderer/src/components/sidebar/worktree-list/grouping/group-sections.ts +++ b/src/renderer/src/components/sidebar/worktree-list/grouping/group-sections.ts @@ -156,8 +156,12 @@ export function appendOrderedGroups( groupBy === 'repo' ? getMixedHostContextLabels(group, repoMap, projectIndex, hostLabelById) : undefined + // Why (STA-4343): repo grouping normally labels by repo, but one repo id can + // be registered on two hosts — then every row in the group shares a repo id + // and the per-repo label cannot tell them apart. Fall back to the per-row + // host labels, which are keyed by host-qualified identity. const hostContextLabelByWorktreeId = - groupBy === 'repo' ? undefined : mixedWorktreeHostContextLabels + groupBy === 'repo' && hostContextLabelByRepoId ? undefined : mixedWorktreeHostContextLabels appendWorktreeRows(result, items, repoMap, lineageById, worktreeMap, { nestLineage, collapsedGroups, diff --git a/src/renderer/src/components/sidebar/worktree-list/grouping/host-labels.ts b/src/renderer/src/components/sidebar/worktree-list/grouping/host-labels.ts index 16f5280ad63..6b077e0a07c 100644 --- a/src/renderer/src/components/sidebar/worktree-list/grouping/host-labels.ts +++ b/src/renderer/src/components/sidebar/worktree-list/grouping/host-labels.ts @@ -6,6 +6,7 @@ import { getWorktreeExecutionHostId } from '../../../../../../shared/execution-host' import type { ExecutionHostId } from '../../../../../../shared/execution-host' +import { getWorktreeHostIdentity } from '../../../../../../shared/worktree/host-qualified-identity' import type { ProjectGroupingIndex, WorktreeGroupEntry } from './project-grouping' function getRepoHostLabel( @@ -45,20 +46,24 @@ export function getMixedHostContextLabels( return uniqueLabels.size > 1 ? labelsByRepoId : undefined } +/** Keyed by host-qualified identity: two hosts sharing an id need two labels. */ export function getMixedWorktreeHostContextLabels( worktrees: readonly Worktree[], repoMap: Map, hostLabelById: ReadonlyMap | undefined, defaultHostId: ExecutionHostId ): Map | undefined { - const labelsByWorktreeId = new Map() + const labelsByIdentity = new Map() const uniqueHostIds = new Set() for (const worktree of worktrees) { const hostId = getWorktreeExecutionHostId(worktree, repoMap.get(worktree.repoId), defaultHostId) uniqueHostIds.add(hostId) - labelsByWorktreeId.set(worktree.id, hostLabelById?.get(hostId) ?? getExecutionHostLabel(hostId)) + labelsByIdentity.set( + getWorktreeHostIdentity(worktree), + hostLabelById?.get(hostId) ?? getExecutionHostLabel(hostId) + ) } - return uniqueHostIds.size > 1 ? labelsByWorktreeId : undefined + return uniqueHostIds.size > 1 ? labelsByIdentity : undefined } export function getHostWorktreeCounts( @@ -70,12 +75,14 @@ export function getHostWorktreeCounts( return undefined } const counts = new Map() - const seenWorktreeIds = new Set() + // Dedup by host, not by bare id: the same id on two hosts is two workspaces + // and has to be counted under each of them (STA-4343). + const seenIdentities = new Set() for (const worktree of worktrees) { - if (seenWorktreeIds.has(worktree.id)) { + if (seenIdentities.has(getWorktreeHostIdentity(worktree))) { continue } - seenWorktreeIds.add(worktree.id) + seenIdentities.add(getWorktreeHostIdentity(worktree)) const hostId = getWorktreeExecutionHostId(worktree, repoMap.get(worktree.repoId), defaultHostId) counts.set(hostId, (counts.get(hostId) ?? 0) + 1) } @@ -91,12 +98,12 @@ export function getHostWorktreeIds( return undefined } const idsByHost = new Map() - const seenWorktreeIds = new Set() + const seenIdentities = new Set() for (const worktree of worktrees) { - if (seenWorktreeIds.has(worktree.id)) { + if (seenIdentities.has(getWorktreeHostIdentity(worktree))) { continue } - seenWorktreeIds.add(worktree.id) + seenIdentities.add(getWorktreeHostIdentity(worktree)) const hostId = getWorktreeExecutionHostId(worktree, repoMap.get(worktree.repoId), defaultHostId) const ids = idsByHost.get(hostId) ?? [] ids.push(worktree.id) diff --git a/src/renderer/src/components/sidebar/worktree-list/grouping/row-builders.ts b/src/renderer/src/components/sidebar/worktree-list/grouping/row-builders.ts index 3f9617174ce..bada875fda4 100644 --- a/src/renderer/src/components/sidebar/worktree-list/grouping/row-builders.ts +++ b/src/renderer/src/components/sidebar/worktree-list/grouping/row-builders.ts @@ -3,6 +3,7 @@ import type { WorktreeLineage } from '../../../../../../shared/worktree/lineage- import type { Worktree } from '../../../../../../shared/worktree/types' import { getWorktreeExecutionHostId } from '../../../../../../shared/execution-host' import type { ExecutionHostId } from '../../../../../../shared/execution-host' +import { getWorktreeHostIdentity } from '../../../../../../shared/worktree/host-qualified-identity' import { getLineageRenderInfo } from '../../worktree-lineage-projection' import { PINNED_GROUP_KEY, PINNED_GROUP_META, getLineageGroupKey } from './group-keys' import type { @@ -82,7 +83,7 @@ export function emitPinnedGroup( for (const [index, worktree] of pinned.entries()) { result.push( buildWorktreeRow(worktree, repoMap, { - rowKey: `${PINNED_GROUP_KEY}:${worktree.id}`, + rowKey: `${PINNED_GROUP_KEY}:${getWorktreeHostIdentity(worktree)}`, sectionKey: PINNED_GROUP_KEY, depth: 0, groupDepth: 0, @@ -190,7 +191,7 @@ export function appendWorktreeRows( for (const worktree of worktrees) { result.push( buildWorktreeRow(worktree, repoMap, { - rowKey: `${sectionKey}:${worktree.id}`, + rowKey: `${sectionKey}:${getWorktreeHostIdentity(worktree)}`, sectionKey, depth: 0, groupDepth, @@ -199,7 +200,7 @@ export function appendWorktreeRows( lineageChildCount: 0, lineageCollapsed: false, hostContextLabel: - hostContextLabelByWorktreeId?.get(worktree.id) ?? + hostContextLabelByWorktreeId?.get(getWorktreeHostIdentity(worktree)) ?? hostContextLabelByRepoId?.get(worktree.repoId) }) ) @@ -209,13 +210,16 @@ export function appendWorktreeRows( const visibleIds = new Set(worktrees.map((worktree) => worktree.id)) const childrenByParentId = new Map() - const childIds = new Set() + // Why (STA-4343): lineage itself is keyed by the bare id, so parent/child + // lookups stay id-based — but membership must be host-qualified or the second + // host's row for a colliding id is silently never emitted. + const childIdentities = new Set() for (const worktree of worktrees) { const lineage = getLineageRenderInfo(worktree, lineageById, worktreeMap, cyclicLineageIds) if (lineage.state !== 'valid' || !visibleIds.has(lineage.parent.id)) { continue } - childIds.add(worktree.id) + childIdentities.add(getWorktreeHostIdentity(worktree)) const children = childrenByParentId.get(lineage.parent.id) ?? [] children.push(worktree) childrenByParentId.set(lineage.parent.id, children) @@ -228,16 +232,16 @@ export function appendWorktreeRows( lineageTrail: boolean[], isLastChild: boolean ): void => { - if (emitted.has(worktree.id)) { + if (emitted.has(getWorktreeHostIdentity(worktree))) { return } const children = childrenByParentId.get(worktree.id) ?? [] const lineageGroupKey = getLineageGroupKey(worktree.id) const lineageCollapsed = collapsedGroups.has(lineageGroupKey) - emitted.add(worktree.id) + emitted.add(getWorktreeHostIdentity(worktree)) result.push( buildWorktreeRow(worktree, repoMap, { - rowKey: `${sectionKey}:${worktree.id}`, + rowKey: `${sectionKey}:${getWorktreeHostIdentity(worktree)}`, sectionKey, depth, groupDepth, @@ -246,7 +250,7 @@ export function appendWorktreeRows( lineageChildCount: children.length, lineageCollapsed, hostContextLabel: - hostContextLabelByWorktreeId?.get(worktree.id) ?? + hostContextLabelByWorktreeId?.get(getWorktreeHostIdentity(worktree)) ?? hostContextLabelByRepoId?.get(worktree.repoId) }) ) @@ -263,13 +267,15 @@ export function appendWorktreeRows( }) } - const roots = worktrees.filter((worktree) => !childIds.has(worktree.id)) + const roots = worktrees.filter( + (worktree) => !childIdentities.has(getWorktreeHostIdentity(worktree)) + ) for (const [index, worktree] of roots.entries()) { emit(worktree, 0, [], index === roots.length - 1) } if (roots.length === 0) { for (const worktree of worktrees) { - if (!emitted.has(worktree.id)) { + if (!emitted.has(getWorktreeHostIdentity(worktree))) { // Why: malformed cyclic lineage should not hide every participant. // Render any leftovers as roots rather than recursing forever. emit(worktree, 0, [], true) diff --git a/src/renderer/src/store/slices/worktree-by-id-index.ts b/src/renderer/src/store/slices/worktree-by-id-index.ts index 3607ce0f339..eb118513cce 100644 --- a/src/renderer/src/store/slices/worktree-by-id-index.ts +++ b/src/renderer/src/store/slices/worktree-by-id-index.ts @@ -5,8 +5,15 @@ import type { Worktree } from '../../../../shared/worktree/types' * * Why: session hydration and terminal reconnect loop over pending worktree ids and * previously re-flattened + linear-searched the whole map per iteration, which is - * O(worktrees x ids) for O(worktrees + ids) distinct work. First entry wins, matching - * the `.find()` it replaces. + * O(worktrees x ids) for O(worktrees + ids) distinct work. + * + * FIRST entry wins, matching the `.find()` it replaces — and matching + * `getIndexedWorktreeMap`, which used to disagree (STA-4343). Both flatten the + * same `worktreesByRepo`, so on an id owned by two hosts one caller resolved to + * the local row while the other resolved to the SSH row. Neither answer is "the" + * workspace: an id with two hosts is two workspaces and a bare-id lookup cannot + * say which. Callers that must distinguish them use `getIndexedWorktreesById`; + * destructive callers carry an explicit host instead. */ export function buildWorktreeByIdIndex( worktreesByRepo: Record diff --git a/src/renderer/src/store/worktree-host-collision-index.test.ts b/src/renderer/src/store/worktree-host-collision-index.test.ts new file mode 100644 index 00000000000..5546f34203f --- /dev/null +++ b/src/renderer/src/store/worktree-host-collision-index.test.ts @@ -0,0 +1,93 @@ +/** + * STA-4343: the same repo at the same path on two hosts is TWO workspaces. + * + * `worktreeId` is `repoId::path` with no host component, so a repo registered on + * both the local host and an SSH host publishes the SAME id twice. Every id-keyed + * projection therefore has to decide what a second row means. Collapsing them + * leaves the user one row for two workspaces — and no way to confirm the other + * one, which makes correct host-qualified routing unreachable. + * + * The dedup these indices were built for is a different thing: a race between + * createWorktree (which appends) and fetchWorktrees (which replaces) can leave + * two IDENTICAL entries in one repo array. That still collapses. + */ +import { describe, expect, it } from 'vitest' +import type { Worktree } from '../../../shared/worktree/types' +import type { AppState } from './types' +import { + getIndexedAllWorktrees, + getIndexedWorktreeMap, + getIndexedWorktreesById +} from './worktree-repo-index' +import { buildWorktreeByIdIndex } from './slices/worktree-by-id-index' + +const SHARED_ID = 'repo-1::/work/orca' + +const baseWorktree: Worktree = { + id: SHARED_ID, + repoId: 'repo-1', + path: '/work/orca', + branch: 'refs/heads/feature', + head: 'abc123', + isBare: false, + isMainWorktree: false, + linkedIssue: null, + linkedPR: null, + linkedLinearIssue: null, + isArchived: false, + comment: '', + isUnread: false, + isPinned: false +} as Worktree + +const localRow: Worktree = { ...baseWorktree, hostId: 'local', displayName: 'local orca' } +const sshRow: Worktree = { ...baseWorktree, hostId: 'ssh:build-box', displayName: 'ssh orca' } + +function byRepo(...worktrees: Worktree[]): AppState['worktreesByRepo'] { + return { 'repo-1': worktrees } +} + +describe('id-keyed worktree projections keep distinct hosts distinct', () => { + it('lists a colliding id once per host', () => { + const all = getIndexedAllWorktrees(byRepo(localRow, sshRow)) + + expect(all).toHaveLength(2) + expect(all.map((entry) => entry.hostId)).toEqual(['local', 'ssh:build-box']) + }) + + it('still collapses the createWorktree/fetchWorktrees duplicate on one host', () => { + // Same id AND same host: this is the append/replace race the dedup exists for. + const stale: Worktree = { ...localRow, head: 'stale' } + const fresh: Worktree = { ...localRow, head: 'fresh' } + + const all = getIndexedAllWorktrees(byRepo(stale, fresh)) + + expect(all).toHaveLength(1) + // Last wins: fetchWorktrees replaces, so the later entry is the current one. + expect(all[0]?.head).toBe('fresh') + }) + + it('treats an unqualified row as its own bucket rather than inventing a host', () => { + const unqualified: Worktree = { ...baseWorktree, displayName: 'unqualified orca' } + + const all = getIndexedAllWorktrees(byRepo(unqualified, sshRow)) + + expect(all).toHaveLength(2) + }) + + it('enumerates every host that publishes an id', () => { + const rows = getIndexedWorktreesById(byRepo(localRow, sshRow), SHARED_ID) + + expect(rows.map((entry) => entry.hostId)).toEqual(['local', 'ssh:build-box']) + }) + + it('agrees with the other id index on which row a bare id resolves to', () => { + const worktreesByRepo = byRepo(localRow, sshRow) + + // Both indices flatten the same data; disagreeing meant one caller saw the + // local row and another the SSH row for the same lookup. + expect(getIndexedWorktreeMap(worktreesByRepo).get(SHARED_ID)).toBe( + buildWorktreeByIdIndex(worktreesByRepo).get(SHARED_ID) + ) + }) +}) diff --git a/src/renderer/src/store/worktree-repo-index.ts b/src/renderer/src/store/worktree-repo-index.ts index 670e6b7ceec..a6108c77d1a 100644 --- a/src/renderer/src/store/worktree-repo-index.ts +++ b/src/renderer/src/store/worktree-repo-index.ts @@ -1,10 +1,15 @@ import type { Repo } from '../../../shared/repo-types' import type { Worktree } from '../../../shared/worktree/types' +import { + composeWorktreeHostIdentity, + getWorktreeIdFromHostIdentity +} from '../../../shared/worktree/host-qualified-identity' import type { AppState } from './types' type WorktreeSnapshot = { allWorktrees: Worktree[] worktreeMap: Map + worktreesById: Map } // Why: Zustand reruns selectors on every write, so identity projections need @@ -19,17 +24,43 @@ function getWorktreeSnapshot(worktreesByRepo: AppState['worktreesByRepo']): Work } // Why: a race between createWorktree (which appends) and fetchWorktrees - // (which replaces) can produce duplicate entries within one repo array. - const worktreeMap = new Map() + // (which replaces) can produce duplicate entries within one repo array. That + // duplicate is the SAME workspace twice, so it collapses — but the key has to + // include the host (STA-4343). `worktreeId` is `repoId::path` with no host + // component, so the local host and an SSH host can publish the same id for + // two genuinely different workspaces, and collapsing those hides one of them + // from the sidebar entirely. Last wins within a host: fetchWorktrees replaces, + // so the later entry is the current one. + const byHostAndId = new Map() for (const worktrees of Object.values(worktreesByRepo)) { for (const worktree of worktrees) { - worktreeMap.set(worktree.id, worktree) + byHostAndId.set(composeWorktreeHostIdentity(worktree.hostId, worktree.id), worktree) } } - const snapshot = { - allWorktrees: Array.from(worktreeMap.values()), - worktreeMap + const allWorktrees: Worktree[] = [] + const worktreeMap = new Map() + const worktreesById = new Map() + // Why the identity round-trip: `worktree.id` is a getter on some snapshots and + // retained selectors assert it is read exactly once per row, so recover the id + // from the key instead of reading the property again. + for (const [identity, worktree] of byHostAndId) { + const worktreeId = getWorktreeIdFromHostIdentity(identity) + allWorktrees.push(worktree) + // FIRST wins across hosts, matching `buildWorktreeByIdIndex` and the `.find()` + // both replaced. Freshness is not the question here: the same-host duplicate + // was already collapsed above, so what is left is a genuine two-host collision + // that no bare-id lookup can resolve. + if (!worktreeMap.has(worktreeId)) { + worktreeMap.set(worktreeId, worktree) + } + const rows = worktreesById.get(worktreeId) + if (rows) { + rows.push(worktree) + } else { + worktreesById.set(worktreeId, [worktree]) + } } + const snapshot = { allWorktrees, worktreeMap, worktreesById } worktreeSnapshotCache.set(worktreesByRepo, snapshot) return snapshot } @@ -38,12 +69,27 @@ export function getIndexedAllWorktrees(worktreesByRepo: AppState['worktreesByRep return getWorktreeSnapshot(worktreesByRepo).allWorktrees } +/** + * One row per id — a LEGACY convenience for the many id-only lookups. + * + * It cannot represent a collision, so it is not evidence of ownership: a + * destructive path must take an explicit host (see `WorktreeRemovalTarget`) and + * use `getIndexedWorktreesById` when it needs to see every owner. + */ export function getIndexedWorktreeMap( worktreesByRepo: AppState['worktreesByRepo'] ): Map { return getWorktreeSnapshot(worktreesByRepo).worktreeMap } +/** Every host's row for one id, in `worktreesByRepo` order. */ +export function getIndexedWorktreesById( + worktreesByRepo: AppState['worktreesByRepo'], + worktreeId: string +): Worktree[] { + return getWorktreeSnapshot(worktreesByRepo).worktreesById.get(worktreeId) ?? [] +} + export function getIndexedWorktreeById( worktreesByRepo: AppState['worktreesByRepo'], worktreeId: string diff --git a/src/shared/worktree/host-qualified-identity.ts b/src/shared/worktree/host-qualified-identity.ts new file mode 100644 index 00000000000..434bacbae10 --- /dev/null +++ b/src/shared/worktree/host-qualified-identity.ts @@ -0,0 +1,44 @@ +import type { ExecutionHostId } from '../execution-host' +import type { Worktree } from './types' + +/** + * Separator between the host and the workspace id. + * + * Printable on purpose: these identities become React keys and DOM attribute + * values, and HTML attribute parsing replaces U+0000 with U+FFFD — a NUL + * separator would not survive the round trip an anchor comparison depends on. + * The host comes first so the variable-length id absorbs any `|` of its own. + */ +const HOST_SEPARATOR = '|' + +/** + * Stable key for one workspace on one host (STA-4343). + * + * `worktreeId` is `repoId::path` with no host component, so a repo registered on + * two execution hosts publishes the same id twice for two different workspaces. + * Any map, set or React key that must keep them apart keys on this instead. + * + * An unqualified row gets its own bucket rather than being folded into a host: + * it may well BE one of them, but nothing here can prove which. + */ +export function getWorktreeHostIdentity(worktree: Pick): string { + return composeWorktreeHostIdentity(worktree.hostId, worktree.id) +} + +export function composeWorktreeHostIdentity( + hostId: ExecutionHostId | undefined, + worktreeId: string +): string { + return `${hostId ?? ''}${HOST_SEPARATOR}${worktreeId}` +} + +/** + * The workspace id back out of an identity. + * + * Host-first ordering makes this exact: the host cannot contain the separator, + * so everything after the first one is the id — which lets an index recover the + * id without re-reading it off the row. + */ +export function getWorktreeIdFromHostIdentity(identity: string): string { + return identity.slice(identity.indexOf(HOST_SEPARATOR) + 1) +}