test(terminal): pin the workbench projection against under-selecting

Losing a surface unmounts live terminals, which is worse than the
duplicate mount STA-4846 fixes, so cover every catalog shape that reaches
the workbench: local-only rows that name no host, an unqualified row
colliding with a host-qualified one, two SSH hosts on one id, folder rows
across three hosts, folder ids alongside git worktree ids, and a
whole-catalog assertion that the emitted id set equals the distinct input
id set. Also pin the `useAllWorktrees` -> `useWorktreeMap` swap: both read
the same WeakMap-cached snapshot, so the zustand compare is unchanged.

Harden the folder tie-break to require the row to name its own host.
`getCatalogOwnerHostId` defaults an unstamped row to `local`, which would
let a row that never named a host win the `local` tie and mount another
host's path; it now keeps first-wins instead of guessing.
This commit is contained in:
Neil
2026-08-31 00:52:42 -07:00
parent 15494ee69d
commit ba7535effe
2 changed files with 164 additions and 4 deletions
@@ -11,7 +11,7 @@ import { readFileSync } from 'node:fs'
import { join } from 'node:path'
import { describe, expect, it } from 'vitest'
import { projectWorkspaceSurfaces } from './workspace-surface-projection'
import { getIndexedWorktreeMap } from '../store/worktree-repo-index'
import { getIndexedAllWorktrees, getIndexedWorktreeMap } from '../store/worktree-repo-index'
import type { ExecutionHostId } from '../../../shared/execution-host'
import type { FolderWorkspace } from '../../../shared/folder-workspace-types'
import type { Worktree } from '../../../shared/worktree/types'
@@ -163,6 +163,148 @@ describe('projectWorkspaceSurfaces', () => {
})
})
// The collapse only ever removes a duplicate id. Losing a surface the user owns
// unmounts live terminals, which is strictly worse than the duplicate mount this
// fixes, so every shape that reaches the workbench is pinned against dropping one.
describe('projectWorkspaceSurfaces never under-selects', () => {
const unqualifiedWorktree: Worktree = { ...localWorktree, hostId: undefined }
const secondSshWorktree: Worktree = { ...localWorktree, hostId: 'ssh:ci-box' }
const localOnlyFolder: FolderWorkspace = {
...localFolder,
id: 'folder-local-only',
executionHostId: undefined
}
it('emits every distinct id from a mixed local, SSH, runtime and folder catalog', () => {
const distinctWorktree: Worktree = {
...sshWorktree,
id: 'repo-shared::/work/orca-ssh-only',
path: '/work/orca-ssh-only'
}
const distinctFolder: FolderWorkspace = { ...runtimeFolder, id: 'folder-runtime-only' }
const worktrees = [localWorktree, sshWorktree, secondSshWorktree, distinctWorktree]
const folderWorkspaces = [localFolder, runtimeFolder, localOnlyFolder, distinctFolder]
const surfaceIds = project({ worktrees, folderWorkspaces }).map((surface) => surface.id)
const expectedIds = new Set([
...worktrees.map((worktree) => worktree.id),
...folderWorkspaces.map((workspace) => `folder:${workspace.id}`)
])
expect(new Set(surfaceIds)).toEqual(expectedIds)
expect(surfaceIds).toHaveLength(expectedIds.size)
})
it('mounts a local-only catalog whose rows never name a host', () => {
const otherUnqualified: Worktree = {
...unqualifiedWorktree,
id: 'repo-shared::/work/orca-other',
path: '/work/orca-other'
}
expect(
project({
worktrees: [unqualifiedWorktree, otherUnqualified],
folderWorkspaces: [localOnlyFolder]
})
).toEqual([
{ id: SHARED_WORKTREE_ID, path: '/work/orca-feature' },
{ id: 'repo-shared::/work/orca-other', path: '/work/orca-other' },
{ id: 'folder:folder-local-only', path: '/work/orca-local' }
])
})
it('keeps the id when an unqualified row collides with a host-qualified one', () => {
// `composeWorktreeHostIdentity` gives an unqualified row its own bucket, so
// the pair survives the host-qualified index and must still collapse to one id.
expect(project({ worktrees: [unqualifiedWorktree, localWorktree] })).toEqual([
{ id: SHARED_WORKTREE_ID, path: '/work/orca-feature' }
])
expect(project({ worktrees: [localWorktree, unqualifiedWorktree] })).toEqual([
{ id: SHARED_WORKTREE_ID, path: '/work/orca-feature' }
])
})
it('collapses two SSH hosts publishing one worktree id, with no local row present', () => {
expect(project({ worktrees: [sshWorktree, secondSshWorktree] })).toEqual([
{ id: SHARED_WORKTREE_ID, path: '/work/orca-feature' }
])
})
it('keeps a git worktree and a folder workspace apart even on the same id text', () => {
// `folderWorkspaceKey` prefixes `folder:`; a worktree id is `repoId::path`.
const surfaces = project({
worktrees: [{ ...localWorktree, id: 'folder-shared', path: '/work/collide' }],
folderWorkspaces: [localFolder]
})
expect(surfaces).toEqual([
{ id: 'folder-shared', path: '/work/collide' },
{ id: 'folder:folder-shared', path: '/work/orca-local' }
])
})
it('collapses folder rows across three hosts to one surface without losing the id', () => {
const sshFolder: FolderWorkspace = {
...localFolder,
folderPath: '/ssh/orca',
executionHostId: undefined,
connectionId: 'build-box'
}
const surfaces = project({
folderWorkspaces: [localFolder, runtimeFolder, sshFolder],
activeWorkspaceId: 'folder:folder-shared',
activeWorkspaceResolvedHostId: 'ssh:build-box'
})
expect(surfaces).toEqual([{ id: 'folder:folder-shared', path: '/ssh/orca' }])
})
it('does not let a folder row that names no host win the local tie-break', () => {
// `getCatalogOwnerHostId` defaults an unstamped row to `local`; honouring that
// would mount the unstamped row's path over the row that really is local.
const unstampedPeer: FolderWorkspace = {
...localFolder,
folderPath: '/unknown/orca',
executionHostId: undefined,
connectionId: undefined
}
expect(
project({
folderWorkspaces: [localFolder, unstampedPeer],
activeWorkspaceId: 'folder:folder-shared',
activeWorkspaceResolvedHostId: 'local'
})
).toEqual([{ id: 'folder:folder-shared', path: '/work/orca-local' }])
})
it('emits nothing extra and nothing missing for an empty catalog', () => {
expect(project({})).toEqual([])
})
})
// The feed swapped `useAllWorktrees` for `useWorktreeMap`. Both read the same
// WeakMap-cached snapshot of `worktreesByRepo`, so the zustand `Object.is` compare
// still re-renders on exactly the writes that replace the slice — no dropped update.
describe('worktree surface feed subscription identity', () => {
const worktreesByRepo = { 'repo-shared': [localWorktree, sshWorktree] }
it('returns a stable map for an unchanged slice and a fresh one after a replace', () => {
expect(getIndexedWorktreeMap(worktreesByRepo)).toBe(getIndexedWorktreeMap(worktreesByRepo))
expect(getIndexedWorktreeMap({ ...worktreesByRepo })).not.toBe(
getIndexedWorktreeMap(worktreesByRepo)
)
})
it('exposes the same id set the host-qualified array does', () => {
expect(new Set(getIndexedWorktreeMap(worktreesByRepo).keys())).toEqual(
new Set(getIndexedAllWorktrees(worktreesByRepo).map((worktree) => worktree.id))
)
})
})
// Why source text: the per-id collapse lives in the store index, so the module
// tests above stay green even if Terminal.tsx goes back to flattening the
// host-qualified array itself. The feed is the half that has to be ratcheted.
@@ -1,4 +1,4 @@
import type { ExecutionHostId } from '../../../shared/execution-host'
import { parseExecutionHostId, type ExecutionHostId } from '../../../shared/execution-host'
import type { FolderWorkspace } from '../../../shared/folder-workspace-types'
import type { Worktree } from '../../../shared/worktree/types'
import { folderWorkspaceKey } from '../../../shared/workspace-scope'
@@ -11,6 +11,23 @@ type FolderWorkspaceSurfaceRow = Pick<
'id' | 'folderPath' | 'connectionId' | 'executionHostId'
>
/**
* The row's own host, or null when the row names none.
*
* `getCatalogOwnerHostId` defaults an unstamped row to `local`, which is the
* right answer for an owner lookup but the wrong one for a tie-break: it would
* let a row that never named a host win the `local` tie and mount another
* host's path. Only a row that names its own host may claim the collision.
*/
function getStampedFolderWorkspaceHostId(
workspace: FolderWorkspaceSurfaceRow
): ExecutionHostId | null {
const namesOwnHost = Boolean(
parseExecutionHostId(workspace.executionHostId) ?? workspace.connectionId?.trim()
)
return namesOwnHost ? getCatalogOwnerHostId(workspace) : null
}
/**
* The terminal workbench's mount set: exactly one surface per workspace id.
*
@@ -53,11 +70,12 @@ export function projectWorkspaceSurfaces({
// Why: a folder-workspace id is opaque, not path-derived, so colliding hosts
// disagree on the path; only the active workspace's resolved host breaks the tie.
// Deriving that host from the row alone is sufficient because every stored row is
// stamped with an explicit `executionHostId` by `folderWorkspaceWithFetchedOwner`.
// stamped with an explicit `executionHostId` by `folderWorkspaceWithFetchedOwner`;
// an unstamped row keeps first-wins rather than guessing.
if (
activeWorkspaceResolvedHostId &&
id === activeWorkspaceId &&
getCatalogOwnerHostId(workspace) === activeWorkspaceResolvedHostId
getStampedFolderWorkspaceHostId(workspace) === activeWorkspaceResolvedHostId
) {
surfaces[existingIndex] = surface
}