mirror of
https://github.com/stablyai/orca.git
synced 2026-10-22 08:03:37 +00:00
fix(sidebar): stop collapsing two hosts into one workspace row (STA-4343)
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.
This commit is contained in:
+2
-2
@@ -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
|
||||
|
||||
@@ -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(
|
||||
/<div id="worktree-list-option-all%3Achild"[\s\S]*?lineage child with agent/
|
||||
/<div id="worktree-list-option-all%3A%7Cchild"[\s\S]*?lineage child with agent/
|
||||
)?.[0] ?? ''
|
||||
const parentCard =
|
||||
markup.match(/<div id="worktree-list-option-all%3Aparent"[\s\S]*?lineage parent/)?.[0] ?? ''
|
||||
markup.match(/<div id="worktree-list-option-all%3A%7Cparent"[\s\S]*?lineage parent/)?.[0] ??
|
||||
''
|
||||
|
||||
expect(childCard).toContain('data-begin-editing="true"')
|
||||
expect(parentCard).not.toContain('data-begin-editing="true"')
|
||||
@@ -177,7 +178,7 @@ describe('WorktreeList lineage child card renderer', () => {
|
||||
|
||||
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')
|
||||
})
|
||||
|
||||
|
||||
@@ -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<HTMLElement>('[data-worktree-lineage-children]')
|
||||
]
|
||||
const childRow = container.querySelector<HTMLElement>('[id="worktree-list-option-all%3Achild"]')
|
||||
const childRow = container.querySelector<HTMLElement>(
|
||||
'[id="worktree-list-option-all%3A%7Cchild"]'
|
||||
)
|
||||
const grandchildRow = container.querySelector<HTMLElement>(
|
||||
'[id="worktree-list-option-all%3Agrandchild"]'
|
||||
'[id="worktree-list-option-all%3A%7Cgrandchild"]'
|
||||
)
|
||||
const childSurface = childRow?.querySelector<HTMLElement>('[data-worktree-card-surface="true"]')
|
||||
const grandchildSurface = grandchildRow?.querySelector<HTMLElement>(
|
||||
@@ -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<HTMLElement>(
|
||||
'[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<HTMLElement>(
|
||||
'[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')
|
||||
|
||||
@@ -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
|
||||
])
|
||||
})
|
||||
})
|
||||
|
||||
@@ -12,7 +12,18 @@ export function getWorkspaceDeleteLineage(
|
||||
worktrees: readonly Worktree[],
|
||||
lineageById: Record<string, WorktreeLineage>
|
||||
): 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<string, Worktree>()
|
||||
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
|
||||
|
||||
@@ -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
|
||||
// `<section>:<host>|<worktreeId>` 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(`<div[^>]*id="worktree-list-option-[^"]*%3A${escapeRegExp(worktreeId)}"[^>]*>`)
|
||||
new RegExp(`<div[^>]*id="worktree-list-option-[^"]*%7C${escapeRegExp(worktreeId)}"[^>]*>`)
|
||||
)?.[0] ?? ''
|
||||
)
|
||||
}
|
||||
|
||||
@@ -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<typeof buildRows> {
|
||||
// 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
|
||||
@@ -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,
|
||||
|
||||
@@ -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<string, Repo>,
|
||||
hostLabelById: ReadonlyMap<string, string> | undefined,
|
||||
defaultHostId: ExecutionHostId
|
||||
): Map<string, string> | undefined {
|
||||
const labelsByWorktreeId = new Map<string, string>()
|
||||
const labelsByIdentity = new Map<string, string>()
|
||||
const uniqueHostIds = new Set<ExecutionHostId>()
|
||||
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<ExecutionHostId, number>()
|
||||
const seenWorktreeIds = new Set<string>()
|
||||
// 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<string>()
|
||||
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<ExecutionHostId, string[]>()
|
||||
const seenWorktreeIds = new Set<string>()
|
||||
const seenIdentities = new Set<string>()
|
||||
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)
|
||||
|
||||
@@ -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<string, Worktree[]>()
|
||||
const childIds = new Set<string>()
|
||||
// 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<string>()
|
||||
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)
|
||||
|
||||
@@ -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<string, Worktree[]>
|
||||
|
||||
@@ -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)
|
||||
)
|
||||
})
|
||||
})
|
||||
@@ -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<string, Worktree>
|
||||
worktreesById: Map<string, Worktree[]>
|
||||
}
|
||||
|
||||
// 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<string, Worktree>()
|
||||
// (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<string, Worktree>()
|
||||
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<string, Worktree>()
|
||||
const worktreesById = new Map<string, Worktree[]>()
|
||||
// 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<string, Worktree> {
|
||||
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
|
||||
|
||||
@@ -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<Worktree, 'id' | 'hostId'>): 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)
|
||||
}
|
||||
Reference in New Issue
Block a user