diff --git a/src/renderer/src/lib/worktree-agent-activation-gate.test.ts b/src/renderer/src/lib/worktree-agent-activation-gate.test.ts index 5209d9b134d..cc319992b71 100644 --- a/src/renderer/src/lib/worktree-agent-activation-gate.test.ts +++ b/src/renderer/src/lib/worktree-agent-activation-gate.test.ts @@ -554,7 +554,9 @@ describe('worktree agent activation gate', () => { expect(deps.listSurfaceOwners).not.toHaveBeenCalled() }) - it('does not mint a second surface for a PTY recorded only on the tab row', async () => { + it('lets the execution host decide for a PTY recorded only on the tab row', async () => { + // The row is no longer an ownership tier, so it is not evidence of a surface. Only the host + // can prove a live PTY has none, and an authoritative empty census says exactly that. const livePtyId = `${WORKTREE_ID}@@live-agent` const { deps, createTab } = testDeps({ sessions: [listed(livePtyId)] }) const store = deps.getState() @@ -563,7 +565,13 @@ describe('worktree agent activation gate', () => { await expect(runWorktreeAgentActivationGate(WORKTREE_ID, deps)).resolves.toBe('adopted') - expect(createTab).not.toHaveBeenCalled() + expect(deps.listSurfaceOwners).toHaveBeenCalled() + expect(createTab).toHaveBeenCalledWith( + WORKTREE_ID, + undefined, + undefined, + expect.objectContaining({ initialPtyId: livePtyId }) + ) }) it('restores the host-owned surface when the renderer projection lost every binding', async () => { diff --git a/src/renderer/src/lib/worktree-agent-cross-worktree-surface-adoption.test.ts b/src/renderer/src/lib/worktree-agent-cross-worktree-surface-adoption.test.ts new file mode 100644 index 00000000000..887c4e845c2 --- /dev/null +++ b/src/renderer/src/lib/worktree-agent-cross-worktree-surface-adoption.test.ts @@ -0,0 +1,75 @@ +// Ownership is tab-keyed, so this sweep's "is the PTY already shown?" question no longer has a +// worktree in it. A pane filed under another key counts; an orphan layout with no row does not. +import { describe, expect, it, vi } from 'vitest' +import type { TerminalLayoutSnapshot, TerminalTab } from '../../../shared/terminal-tab-types' +import { + adoptLiveWorkspacePtySurfaces, + type LiveSurfaceAdoptionStore +} from './worktree-agent-live-surface-adoption' + +const WORKTREE_ID = 'repo::/worktree' +const OTHER_WORKTREE_ID = 'repo::/other' +const LEAF_ID = '11111111-1111-4111-8111-111111111111' +const LIVE_PTY_ID = 'repo::/worktree@@live-agent' + +function boundLayout(): TerminalLayoutSnapshot { + return { + root: { type: 'leaf', leafId: LEAF_ID }, + activeLeafId: LEAF_ID, + expandedLeafId: null, + ptyIdsByLeafId: { [LEAF_ID]: LIVE_PTY_ID } + } +} + +function row(worktreeId: string): TerminalTab { + return { + id: 'tab-live', + ptyId: null, + worktreeId, + title: 'Codex', + customTitle: null, + color: null, + sortOrder: 0, + createdAt: 1 + } +} + +function store(tabsByWorktree: Record): LiveSurfaceAdoptionStore { + return { + createTab: vi.fn(() => ({ id: 'tab-minted' })), + ptyIdsByTabId: {}, + setTabLayout: vi.fn(), + tabsByWorktree, + terminalLayoutsByTabId: { 'tab-live': boundLayout() }, + updateTabPtyId: vi.fn(), + replaceTerminalLayoutPanePtyId: vi.fn() + } as unknown as LiveSurfaceAdoptionStore +} + +describe('live pty surface adoption across worktree keys', () => { + it('treats a pane filed under another worktree key as already surfaced', async () => { + // Surfacing one PTY twice is the STA-7961 failure; a foreign key is not a second chance. + const state = store({ [OTHER_WORKTREE_ID]: [row(OTHER_WORKTREE_ID)] }) + const listSurfaceOwners = vi.fn(async () => new Map()) + + const result = await adoptLiveWorkspacePtySurfaces( + () => state, + WORKTREE_ID, + [LIVE_PTY_ID], + listSurfaceOwners + ) + + expect(result).toEqual({ surfaced: true, declinedPtyIds: [] }) + expect(listSurfaceOwners).not.toHaveBeenCalled() + expect(state.createTab).not.toHaveBeenCalled() + }) + + it('does not count a layout whose row is gone, since it surfaces nothing', async () => { + const state = store({}) + const listSurfaceOwners = vi.fn(async () => new Map()) + + await adoptLiveWorkspacePtySurfaces(() => state, WORKTREE_ID, [LIVE_PTY_ID], listSurfaceOwners) + + expect(listSurfaceOwners).toHaveBeenCalledWith(WORKTREE_ID) + }) +}) diff --git a/src/renderer/src/lib/worktree-agent-live-surface-adoption.ts b/src/renderer/src/lib/worktree-agent-live-surface-adoption.ts index 1fa7ba86806..ac154a18ec3 100644 --- a/src/renderer/src/lib/worktree-agent-live-surface-adoption.ts +++ b/src/renderer/src/lib/worktree-agent-live-surface-adoption.ts @@ -1,7 +1,8 @@ import { parsePaneKey } from '../../../shared/stable-pane-id' import { worktreeIdsEqual } from '../../../shared/worktree/id' import type { useAppStore } from '@/store' -import { resolveTerminalPtyPaneOwnership } from './terminal-pty-pane-owner' +import { listTerminalPtyPaneOwners } from './terminal-pty-pane-owner' +import { findTerminalTabRow } from './terminal-reveal-tab-adoption' import type { LiveTerminalSurfaceOwner, LiveTerminalSurfaceOwnerIndex @@ -124,18 +125,24 @@ function adoptHostOwnedSurface( * seeded pane, because failing closed must not also fail silent. `declinedPtyIds` names * the live PTYs the sweep left without one, so a decline is diagnosable and not mute. */ +/** + * Whether some pane in this renderer already shows the PTY. Ownership is tab-keyed, so a row + * filed under any other worktree key still counts — a PTY already surfaced must not be adopted + * twice. A layout whose row is gone surfaces nothing, so it counts for neither. + */ +function isPtyAlreadySurfaced(store: LiveSurfaceAdoptionStore, ptyId: string): boolean { + return listTerminalPtyPaneOwners(store, ptyId).some( + (owner) => findTerminalTabRow(store, owner.tabId) !== null + ) +} + export async function adoptLiveWorkspacePtySurfaces( getState: () => LiveSurfaceAdoptionStore, worktreeId: string, livePtyIds: readonly string[], listSurfaceOwners: (worktreeId: string) => Promise ): Promise<{ surfaced: boolean; declinedPtyIds: string[] }> { - // Why: ptyIdsByTabId holds only panes this renderer mounted, so a tab bound solely in the - // persisted layout used to read as unbound — under any worktree key, since a PTY already - // surfaced elsewhere must not be adopted a second time. - const unbound = livePtyIds.filter( - (ptyId) => resolveTerminalPtyPaneOwnership(getState(), ptyId).kind === 'none' - ) + const unbound = livePtyIds.filter((ptyId) => !isPtyAlreadySurfaced(getState(), ptyId)) let surfaced = unbound.length < livePtyIds.length const declinedPtyIds: string[] = [] if (unbound.length === 0) { @@ -151,7 +158,7 @@ export async function adoptLiveWorkspacePtySurfaces( for (const ptyId of unbound) { // Why: a pane can mount while the census is in flight, so the pre-RPC // verdict is stale by the time it would authorize a mint. - if (resolveTerminalPtyPaneOwnership(getState(), ptyId).kind !== 'none') { + if (isPtyAlreadySurfaced(getState(), ptyId)) { surfaced = true continue } diff --git a/src/renderer/src/store/slices/store-session-terminal-reconnect.test.ts b/src/renderer/src/store/slices/store-session-terminal-reconnect.test.ts index c0f05124eda..f64ebe296c8 100644 --- a/src/renderer/src/store/slices/store-session-terminal-reconnect.test.ts +++ b/src/renderer/src/store/slices/store-session-terminal-reconnect.test.ts @@ -383,8 +383,14 @@ describe('reconnectPersistedTerminals', () => { const s = store.getState() // Why: deferred reattach doesn't call spawn — connectPanePty handles it expect((mockApi.pty as Record).spawn).not.toHaveBeenCalled() - // Why: reconnect restores the tab-level ptyId so getWorktreeStatus() shows active (green dot) before the terminal mounts. - expect(s.tabsByWorktree[wt1][0].ptyId).toBe('daemon-session-B') + // Why: reconnect restores the tab-level ptyId so getWorktreeStatus() shows active (green dot) + // before the terminal mounts. The layout is the binding, so the anchor is the active leaf's + // session, not whichever pane the persisted row happened to name. + const activeLeafId = s.terminalLayoutsByTabId['tab1'].activeLeafId + expect(s.tabsByWorktree[wt1][0].ptyId).toBe( + s.terminalLayoutsByTabId['tab1'].ptyIdsByLeafId?.[activeLeafId!] + ) + expect(s.tabsByWorktree[wt1][0].ptyId).toBe('daemon-session-A') // ptyIdsByLeafId preserved for connectPanePty; legacy pane:* leaves reminted to durable UUID leaves at hydration. const layout = s.terminalLayoutsByTabId['tab1'] const bindings = layout.ptyIdsByLeafId ?? {} diff --git a/src/renderer/src/store/terminals/workspace-terminal-duplicate-binding-reconnect.test.ts b/src/renderer/src/store/terminals/workspace-terminal-duplicate-binding-reconnect.test.ts new file mode 100644 index 00000000000..5121158d664 --- /dev/null +++ b/src/renderer/src/store/terminals/workspace-terminal-duplicate-binding-reconnect.test.ts @@ -0,0 +1,172 @@ +// A losing tab has a second route back to the PTY its layout just gave up: its own row `ptyId`, +// which reconnect copies into pendingReconnectPtyIdByTabId and the pane then takes as a fallback. +// This drives the real hydration entry with the STA-7961 pair to pin that route shut. +import { describe, expect, it, vi } from 'vitest' +import type { WorkspaceSessionState } from '../../../../shared/workspace-session-state-types' + +vi.mock('sonner', () => ({ toast: { info: vi.fn(), success: vi.fn(), error: vi.fn() } })) +vi.mock('@/runtime/sync-runtime-graph', () => ({ scheduleRuntimeGraphSync: vi.fn() })) +vi.mock('@/components/terminal-pane/pty-transport', () => ({ + registerEagerPtyBuffer: vi.fn(), + ensurePtyDispatcher: vi.fn() +})) + +const apiProxy = (): unknown => + new Proxy(() => undefined, { + get: (_target, prop) => (prop === 'then' ? undefined : apiProxy()), + apply: () => Promise.resolve(null) + }) + +// @ts-expect-error -- mocked browser preload API +globalThis.window = { api: apiProxy() } + +import { createTestStore, makeTab, makeWorktree, seedStore } from '../slices/store-test-helpers' + +const WORKTREE_ID = 'repo1::/wt-1' +const SPLIT_TAB_ID = 'eba00a9a-17df-4152-8258-42381b48890a' +const SINGLE_TAB_ID = '881a9ee2-7143-46c8-98ac-8ffbb9cf4b2c' +const SHARED_LEAF_ID = '10cb5648-8a54-41c0-a6a4-ef0028d93599' +const OWN_LEAF_ID = 'df8913c9-fd8a-420a-a7d6-17daf0ed30f0' +const SHARED_PTY_ID = 'repo1::/wt-1@@289ed0f2' +const SPLIT_OWN_PTY_ID = 'repo1::/wt-1@@eaff6e99' +const LEGACY_TAB_ID = '4b0f3f05-5e9e-4a2e-9f3a-6f1f3a4b7c21' +const LEGACY_LEAF_ID = '7c1a8d2e-2f44-4a7b-93b6-10c6a1f9d0b4' +const LEGACY_PTY_ID = 'repo1::/wt-1@@legacy01' + +function baseSession(): WorkspaceSessionState { + return { + activeRepoId: 'repo1', + activeWorktreeId: WORKTREE_ID, + activeTabId: SINGLE_TAB_ID, + activeWorktreeIdsOnShutdown: [WORKTREE_ID], + tabsByWorktree: { [WORKTREE_ID]: [] }, + terminalLayoutsByTabId: {} + } +} + +/** The persisted pair from the report: both tabs bind the same leaf to the same PTY. */ +function duplicateLeafSession(): WorkspaceSessionState { + return { + ...baseSession(), + tabsByWorktree: { + [WORKTREE_ID]: [ + makeTab({ + id: SPLIT_TAB_ID, + worktreeId: WORKTREE_ID, + ptyId: SPLIT_OWN_PTY_ID, + sortOrder: 0, + createdAt: 1_789_867_969_623 + }), + makeTab({ + id: SINGLE_TAB_ID, + worktreeId: WORKTREE_ID, + ptyId: SHARED_PTY_ID, + sortOrder: 1, + createdAt: 1_789_867_969_624 + }) + ] + }, + terminalLayoutsByTabId: { + [SPLIT_TAB_ID]: { + root: { + type: 'split', + direction: 'vertical', + first: { type: 'leaf', leafId: OWN_LEAF_ID }, + second: { type: 'leaf', leafId: SHARED_LEAF_ID } + }, + activeLeafId: SHARED_LEAF_ID, + expandedLeafId: null, + ptyIdsByLeafId: { [OWN_LEAF_ID]: SPLIT_OWN_PTY_ID, [SHARED_LEAF_ID]: SHARED_PTY_ID } + }, + [SINGLE_TAB_ID]: { + root: { type: 'leaf', leafId: SHARED_LEAF_ID }, + activeLeafId: SHARED_LEAF_ID, + expandedLeafId: null, + ptyIdsByLeafId: { [SHARED_LEAF_ID]: SHARED_PTY_ID } + } + } + } +} + +/** A profile written before per-leaf bindings existed: a tree, and no bindings map at all. */ +function legacyLayoutSession(): WorkspaceSessionState { + return { + ...baseSession(), + activeTabId: LEGACY_TAB_ID, + tabsByWorktree: { + [WORKTREE_ID]: [makeTab({ id: LEGACY_TAB_ID, worktreeId: WORKTREE_ID, ptyId: LEGACY_PTY_ID })] + }, + terminalLayoutsByTabId: { + [LEGACY_TAB_ID]: { + root: { type: 'leaf', leafId: LEGACY_LEAF_ID }, + activeLeafId: LEGACY_LEAF_ID, + expandedLeafId: null + } + } + } +} + +function hydrate( + session: WorkspaceSessionState +): ReturnType['getState']> { + const store = createTestStore() + seedStore(store, { + worktreesByRepo: { + repo1: [makeWorktree({ id: WORKTREE_ID, repoId: 'repo1', path: '/wt-1' })] + } + }) + store.getState().hydrateWorkspaceSession(session) + return store.getState() +} + +describe('hydrating the STA-7961 duplicate binding', () => { + it('leaves the shared pty bound to one tab only', () => { + const state = hydrate(duplicateLeafSession()) + + expect(state.terminalLayoutsByTabId[SPLIT_TAB_ID]?.ptyIdsByLeafId?.[SHARED_LEAF_ID]).toBe( + SHARED_PTY_ID + ) + expect( + Object.values(state.terminalLayoutsByTabId[SINGLE_TAB_ID]?.ptyIdsByLeafId ?? {}) + ).not.toContain(SHARED_PTY_ID) + }) + + it('keeps the losing row from taking the pty back through its tab-level id', () => { + const state = hydrate(duplicateLeafSession()) + + expect(state.pendingReconnectPtyIdByTabId[SINGLE_TAB_ID]).toBeUndefined() + }) + + it('anchors the winner to what its healed layout binds, not to its row', () => { + // The winner's row still names its other pane's pty; the anchor follows the active leaf, + // which is the shared session it just won. + const state = hydrate(duplicateLeafSession()) + + expect(state.pendingReconnectPtyIdByTabId[SPLIT_TAB_ID]).toBe(SHARED_PTY_ID) + expect(state.terminalLayoutsByTabId[SPLIT_TAB_ID]?.ptyIdsByLeafId?.[OWN_LEAF_ID]).toBe( + SPLIT_OWN_PTY_ID + ) + }) + + it('keeps both rows and the losing tab keeps its pane', () => { + const state = hydrate(duplicateLeafSession()) + + expect(state.tabsByWorktree[WORKTREE_ID]?.map((tab) => tab.id)).toEqual([ + SPLIT_TAB_ID, + SINGLE_TAB_ID + ]) + expect(state.terminalLayoutsByTabId[SINGLE_TAB_ID]?.root).toEqual({ + type: 'leaf', + leafId: SHARED_LEAF_ID + }) + }) + + it('a legacy layout with a tree but no bindings map still reconnects through the row', () => { + // Nothing migrates tab.ptyId into ptyIdsByLeafId, so this shape survives on disk and the + // row is the only thing that names its session. An absent map, not an empty one, marks it. + const state = hydrate(legacyLayoutSession()) + + expect(state.terminalLayoutsByTabId[LEGACY_TAB_ID]?.ptyIdsByLeafId).toBeUndefined() + expect(state.pendingReconnectPtyIdByTabId[LEGACY_TAB_ID]).toBe(LEGACY_PTY_ID) + }) +}) diff --git a/src/renderer/src/store/terminals/workspace-terminal-hydration.ts b/src/renderer/src/store/terminals/workspace-terminal-hydration.ts index db278e05d92..42be2366182 100644 --- a/src/renderer/src/store/terminals/workspace-terminal-hydration.ts +++ b/src/renderer/src/store/terminals/workspace-terminal-hydration.ts @@ -109,11 +109,26 @@ export function createWorkspaceTerminalHydrationActions( runtimeSessionPlaceholders.repos.some((repo) => repo.id === session.activeRepoId) ? session.activeRepoId : null + // Why indexed here: the layout plan looks up a tab per persisted layout, and re-flattening + // tabsByWorktree per entry is O(tabs x layouts). + const allTabs = Object.values(tabsByWorktree).flat() + const tabById = buildByIdIndex(allTabs) + // Why the layout plan first: it is what heals a duplicated leaf or pty binding, and the + // reconnect plan below reads the healed layout to decide what a row may reattach to. + const { layoutsByTabId } = buildWorkspaceTerminalLayoutPlan({ + ownershipTransfersByTabId, + ownershipTransferTabIds, + releasedPtyIdsByTabId, + session, + tabById, + validTabIds + }) const { pendingReconnectPtyIdByTabId, pendingReconnectTabByWorktree, pendingReconnectWorktreeIds } = buildWorkspaceTerminalReconnectPlan({ + layoutsByTabId, reconnectPtyIdByRetainedTabId, releasedPtyIdsByTabId, repos: runtimeSessionPlaceholders.repos, @@ -159,10 +174,6 @@ export function createWorkspaceTerminalHydrationActions( if (activeWorktreeId) { nextEverActivated.add(activeWorktreeId) } - // Why indexed: the layout map below looks up a tab per persisted layout, and - // re-flattening tabsByWorktree per entry is O(tabs x layouts). - const allTabs = Object.values(tabsByWorktree).flat() - const tabById = buildByIdIndex(allTabs) const hydrated: WorkspaceHydrationPatch = { activeRepoId, activeWorktreeId, @@ -210,14 +221,7 @@ export function createWorkspaceTerminalHydrationActions( worktreeNavHistory: activeWorktreeId ? [activeWorktreeId] : [], worktreeNavHistoryIndex: activeWorktreeId ? 0 : -1, ptyIdsByTabId: Object.fromEntries(allTabs.map((tab) => [tab.id, []] as const)), - terminalLayoutsByTabId: buildWorkspaceTerminalLayoutPlan({ - ownershipTransfersByTabId, - ownershipTransferTabIds, - releasedPtyIdsByTabId, - session, - tabById, - validTabIds - }), + terminalLayoutsByTabId: layoutsByTabId, localOnlyScrollbackByTabId: Object.fromEntries( Object.entries(session.localOnlyScrollbackByTabId ?? {}).filter(([tabId]) => validTabIds.has(tabId) diff --git a/src/renderer/src/store/terminals/workspace-terminal-layout-duplicate-bindings.test.ts b/src/renderer/src/store/terminals/workspace-terminal-layout-duplicate-bindings.test.ts new file mode 100644 index 00000000000..36689dd64d5 --- /dev/null +++ b/src/renderer/src/store/terminals/workspace-terminal-layout-duplicate-bindings.test.ts @@ -0,0 +1,226 @@ +// One leaf id belongs to one tab; one PTY id is bound by one tab. Two tabs holding the same +// leaf is what let a reveal mint a second pane over a live session (STA-7961), and the loser +// must keep its row and its pane — deleting rows is what #13060 did and #13098 reverted. +import { describe, expect, it } from 'vitest' +import { resolveDuplicateTerminalLayoutBindings } from './workspace-terminal-layout-duplicate-bindings' +import type { TerminalLayoutSnapshot, TerminalTab } from '../../../../shared/terminal-tab-types' + +const SPLIT_TAB = 'tab-split' +const SINGLE_TAB = 'tab-single' +// Why real UUIDs: layout normalization re-mints any leaf id that is not a stable pane id. +const SHARED_LEAF = '10cb5648-8a54-41c0-a6a4-ef0028d93599' +const OTHER_LEAF = 'df8913c9-fd8a-420a-a7d6-17daf0ed30f0' +const ELSEWHERE_LEAF = '96cdf7ea-9c83-4ba5-a41b-c425955e6606' +const SHARED_PTY = 'wt-1@@shared' + +function tab(id: string, sortOrder: number, createdAt = 1_000 + sortOrder): TerminalTab { + return { + id, + ptyId: null, + worktreeId: 'wt-1', + title: 'Terminal', + customTitle: null, + color: null, + sortOrder, + createdAt + } +} + +function heal( + layoutsByTabId: Record, + tabs: TerminalTab[], + canonicalTabIds: string[] = [] +): Record { + return resolveDuplicateTerminalLayoutBindings({ + canonicalTabIds: new Set(canonicalTabIds), + layoutsByTabId, + tabById: new Map(tabs.map((row) => [row.id, row])) + }) +} + +function splitLayout(): TerminalLayoutSnapshot { + return { + root: { + type: 'split', + direction: 'vertical', + first: { type: 'leaf', leafId: OTHER_LEAF }, + second: { type: 'leaf', leafId: SHARED_LEAF } + }, + activeLeafId: SHARED_LEAF, + expandedLeafId: null, + ptyIdsByLeafId: { [OTHER_LEAF]: 'wt-1@@other', [SHARED_LEAF]: SHARED_PTY } + } +} + +function singleLeafLayout(leafId: string, ptyId: string): TerminalLayoutSnapshot { + return { + root: { type: 'leaf', leafId }, + activeLeafId: leafId, + expandedLeafId: null, + ptyIdsByLeafId: { [leafId]: ptyId } + } +} + +describe('resolveDuplicateTerminalLayoutBindings', () => { + it('leaves the single-leaf loser its pane and takes only the pty binding', () => { + const healed = heal( + { + [SPLIT_TAB]: splitLayout(), + [SINGLE_TAB]: singleLeafLayout(SHARED_LEAF, SHARED_PTY) + }, + [tab(SPLIT_TAB, 0), tab(SINGLE_TAB, 1)] + ) + + expect(healed[SPLIT_TAB]!.ptyIdsByLeafId?.[SHARED_LEAF]).toBe(SHARED_PTY) + expect(healed[SINGLE_TAB]!.root).toEqual({ type: 'leaf', leafId: SHARED_LEAF }) + expect(healed[SINGLE_TAB]!.activeLeafId).toBe(SHARED_LEAF) + }) + + it('leaves the loser an explicit, empty bindings map rather than none at all', () => { + // An absent map is the legacy shape reconnect still reads `tab.ptyId` for, which would + // hand the loser straight back the PTY its layout just surrendered. + const healed = heal( + { + [SPLIT_TAB]: splitLayout(), + [SINGLE_TAB]: singleLeafLayout(SHARED_LEAF, SHARED_PTY) + }, + [tab(SPLIT_TAB, 0), tab(SINGLE_TAB, 1)] + ) + + expect(healed[SINGLE_TAB]!.ptyIdsByLeafId).toEqual({}) + }) + + it('removes the duplicated leaf from a split loser and keeps its other panes', () => { + const healed = heal( + { + [SPLIT_TAB]: splitLayout(), + [SINGLE_TAB]: singleLeafLayout(SHARED_LEAF, SHARED_PTY) + }, + [tab(SINGLE_TAB, 0), tab(SPLIT_TAB, 1)] + ) + + expect(healed[SPLIT_TAB]!.root).toEqual({ type: 'leaf', leafId: OTHER_LEAF }) + expect(healed[SPLIT_TAB]!.ptyIdsByLeafId).toEqual({ [OTHER_LEAF]: 'wt-1@@other' }) + expect(healed[SPLIT_TAB]!.activeLeafId).toBe(OTHER_LEAF) + expect(healed[SINGLE_TAB]!.ptyIdsByLeafId?.[SHARED_LEAF]).toBe(SHARED_PTY) + }) + + it('unbinds a pty shared under two leaf ids and leaves both panes standing', () => { + const healed = heal( + { + [SPLIT_TAB]: splitLayout(), + [SINGLE_TAB]: singleLeafLayout(ELSEWHERE_LEAF, SHARED_PTY) + }, + [tab(SINGLE_TAB, 0), tab(SPLIT_TAB, 1)] + ) + + const owners = Object.entries(healed) + .filter(([, layout]) => Object.values(layout.ptyIdsByLeafId ?? {}).includes(SHARED_PTY)) + .map(([tabId]) => tabId) + expect(owners).toEqual([SINGLE_TAB]) + // The losing pane is real — only its mount collided — so the split keeps both leaves. + expect(healed[SPLIT_TAB]!.root).toEqual(splitLayout().root) + expect(healed[SPLIT_TAB]!.ptyIdsByLeafId).toEqual({ [OTHER_LEAF]: 'wt-1@@other' }) + expect(healed[SPLIT_TAB]!.activeLeafId).toBe(OTHER_LEAF) + }) + + it('lets a canonical unified tab outrank a lower sort order', () => { + const healed = heal( + { + [SPLIT_TAB]: splitLayout(), + [SINGLE_TAB]: singleLeafLayout(SHARED_LEAF, SHARED_PTY) + }, + [tab(SPLIT_TAB, 0), tab(SINGLE_TAB, 1)], + [SINGLE_TAB] + ) + + expect(healed[SINGLE_TAB]!.ptyIdsByLeafId?.[SHARED_LEAF]).toBe(SHARED_PTY) + expect(healed[SPLIT_TAB]!.root).toEqual({ type: 'leaf', leafId: OTHER_LEAF }) + }) + + it('prefers the older row when sort order ties', () => { + const healed = heal( + { + 'tab-new': singleLeafLayout(SHARED_LEAF, SHARED_PTY), + 'tab-old': singleLeafLayout(SHARED_LEAF, SHARED_PTY) + }, + [tab('tab-new', 0, 2_000), tab('tab-old', 0, 1_000)] + ) + + expect(healed['tab-old']!.ptyIdsByLeafId?.[SHARED_LEAF]).toBe(SHARED_PTY) + expect(healed['tab-new']!.ptyIdsByLeafId).toEqual({}) + }) + + it('breaks an all-equal tie on tab id, whichever order the layouts arrive in', () => { + const tabs = [tab('tab-b', 0, 1_000), tab('tab-a', 0, 1_000)] + const forward = heal( + { + 'tab-b': singleLeafLayout(SHARED_LEAF, SHARED_PTY), + 'tab-a': singleLeafLayout(SHARED_LEAF, SHARED_PTY) + }, + tabs + ) + const reverse = heal( + { + 'tab-a': singleLeafLayout(SHARED_LEAF, SHARED_PTY), + 'tab-b': singleLeafLayout(SHARED_LEAF, SHARED_PTY) + }, + tabs + ) + + for (const healed of [forward, reverse]) { + expect(healed['tab-a']!.ptyIdsByLeafId?.[SHARED_LEAF]).toBe(SHARED_PTY) + expect(healed['tab-b']!.ptyIdsByLeafId).toEqual({}) + } + }) + + it('ranks a tab id with no surviving row behind one that has one', () => { + const healed = heal( + { + 'tab-rowless': singleLeafLayout(SHARED_LEAF, SHARED_PTY), + [SINGLE_TAB]: singleLeafLayout(SHARED_LEAF, SHARED_PTY) + }, + [tab(SINGLE_TAB, 99, 9_000)] + ) + + expect(healed[SINGLE_TAB]!.ptyIdsByLeafId?.[SHARED_LEAF]).toBe(SHARED_PTY) + expect(healed['tab-rowless']!.ptyIdsByLeafId).toEqual({}) + }) + + it('ignores a binding whose pane already left the tree', () => { + const ghost: TerminalLayoutSnapshot = { + root: null, + activeLeafId: null, + expandedLeafId: null, + ptyIdsByLeafId: { [ELSEWHERE_LEAF]: SHARED_PTY } + } + const healed = heal({ [SINGLE_TAB]: ghost, [SPLIT_TAB]: splitLayout() }, [ + tab(SINGLE_TAB, 0), + tab(SPLIT_TAB, 1) + ]) + + // The ghost reattaches nothing, so it must not outrank the pane that still mounts the pty. + expect(healed[SPLIT_TAB]!.ptyIdsByLeafId?.[SHARED_LEAF]).toBe(SHARED_PTY) + }) + + it('returns the same object when no tab collides', () => { + const layoutsByTabId = { + [SPLIT_TAB]: splitLayout(), + [SINGLE_TAB]: singleLeafLayout('08d0d524-0d46-410b-ba08-b43c97a2b4e4', 'wt-1@@own') + } + + expect(heal(layoutsByTabId, [tab(SPLIT_TAB, 0), tab(SINGLE_TAB, 1)])).toBe(layoutsByTabId) + }) + + it('keeps every tab id it was given, healed or not', () => { + const healed = heal( + { + [SPLIT_TAB]: splitLayout(), + [SINGLE_TAB]: singleLeafLayout(SHARED_LEAF, SHARED_PTY) + }, + [tab(SPLIT_TAB, 0), tab(SINGLE_TAB, 1)] + ) + + expect(Object.keys(healed).sort()).toEqual([SINGLE_TAB, SPLIT_TAB].sort()) + }) +}) diff --git a/src/renderer/src/store/terminals/workspace-terminal-layout-duplicate-bindings.ts b/src/renderer/src/store/terminals/workspace-terminal-layout-duplicate-bindings.ts new file mode 100644 index 00000000000..fd1547551f0 --- /dev/null +++ b/src/renderer/src/store/terminals/workspace-terminal-layout-duplicate-bindings.ts @@ -0,0 +1,162 @@ +import type { TerminalLayoutSnapshot, TerminalTab } from '../../../../shared/terminal-tab-types' +import type { WorkspaceSessionState } from '../../../../shared/workspace-session-state-types' +import { detachTerminalLayoutLeaf } from '@/components/terminal-pane/terminal-layout-leaf-detach' +import { + collectLeafIdsInOrder, + resolvePtyBoundActiveLeafId +} from '@/components/terminal-pane/terminal-layout-leaf-ids' + +type TerminalLayoutOwnerRanking = { + canonicalTabIds: ReadonlySet + tabById: ReadonlyMap +} + +/** Terminal rows the unified tab model still lists, across every workspace key. */ +export function readCanonicalTerminalTabIds(session: WorkspaceSessionState): Set { + const tabIds = new Set() + for (const tabs of Object.values(session.unifiedTabs ?? {})) { + for (const tab of tabs) { + if (tab.contentType === 'terminal') { + tabIds.add(tab.entityId) + } + } + } + return tabIds +} + +// Why MAX_SAFE_INTEGER: a tab id with no surviving row ranks behind every row that has one. +const MISSING_ROW_RANK = Number.MAX_SAFE_INTEGER + +/** Canonical row, then the order the user put them in, then the older row, then the id. */ +function compareOwnerTabIds(a: string, b: string, ranking: TerminalLayoutOwnerRanking): number { + const canonical = + Number(!ranking.canonicalTabIds.has(a)) - Number(!ranking.canonicalTabIds.has(b)) + if (canonical !== 0) { + return canonical + } + const aTab = ranking.tabById.get(a) + const bTab = ranking.tabById.get(b) + const sortOrder = (aTab?.sortOrder ?? MISSING_ROW_RANK) - (bTab?.sortOrder ?? MISSING_ROW_RANK) + if (sortOrder !== 0) { + return sortOrder + } + const createdAt = (aTab?.createdAt ?? MISSING_ROW_RANK) - (bTab?.createdAt ?? MISSING_ROW_RANK) + if (createdAt !== 0) { + return createdAt + } + return a < b ? -1 : a > b ? 1 : 0 +} + +/** Leaf ids this layout claims as pane identities. A binding off the tree reattaches nothing. */ +function collectHeldLeafIds(layout: TerminalLayoutSnapshot): string[] { + return [...new Set(collectLeafIdsInOrder(layout.root))] +} + +/** PTY ids this layout can actually reattach: bound to a leaf its own tree still mounts (#13098). */ +function collectMountedPtyIds(layout: TerminalLayoutSnapshot): string[] { + const leafIdsInTree = new Set(collectLeafIdsInOrder(layout.root)) + return [ + ...new Set( + Object.entries(layout.ptyIdsByLeafId ?? {}) + .filter(([leafId]) => leafIdsInTree.has(leafId)) + .map(([, ptyId]) => ptyId) + ) + ] +} + +/** + * Why an explicit map, even when it ends up empty: an absent one reads as a profile written + * before leaf bindings existed, and reconnect hands such a row its own `ptyId` straight back. + */ +function withSurrenderedBindings( + layout: TerminalLayoutSnapshot, + ptyIdsByLeafId: Record +): TerminalLayoutSnapshot { + return { + ...layout, + ptyIdsByLeafId, + activeLeafId: resolvePtyBoundActiveLeafId({ + root: layout.root, + activeLeafId: layout.activeLeafId, + ptyIdsByLeafId + }) + } +} + +function unbindLeafId(layout: TerminalLayoutSnapshot, leafId: string): TerminalLayoutSnapshot { + const { [leafId]: _surrendered, ...ptyIdsByLeafId } = layout.ptyIdsByLeafId ?? {} + return withSurrenderedBindings(layout, ptyIdsByLeafId) +} + +function unbindPtyId(layout: TerminalLayoutSnapshot, ptyId: string): TerminalLayoutSnapshot { + return withSurrenderedBindings( + layout, + Object.fromEntries( + Object.entries(layout.ptyIdsByLeafId ?? {}).filter(([, bound]) => bound !== ptyId) + ) + ) +} + +/** + * A duplicated leaf id is an identity collision, so the losing pane leaves the tree entirely. + * A single-leaf tree has nothing to detach into, and a tab with no pane at all is worse than + * one that cold-starts a shell, so that loser only gives up the binding. + */ +function surrenderLeafId(layout: TerminalLayoutSnapshot, leafId: string): TerminalLayoutSnapshot { + const detached = detachTerminalLayoutLeaf(layout, leafId) + return detached + ? withSurrenderedBindings(detached.sourceLayout, detached.sourceLayout.ptyIdsByLeafId ?? {}) + : unbindLeafId(layout, leafId) +} + +/** One owner per duplicated key; every loser keeps its row, and nothing is deleted (#13060). */ +function resolveDuplicateHolders( + layoutsByTabId: Record, + ranking: TerminalLayoutOwnerRanking, + readHeldKeys: (layout: TerminalLayoutSnapshot) => string[], + surrender: (layout: TerminalLayoutSnapshot, key: string) => TerminalLayoutSnapshot +): Record { + const holderTabIdsByKey = new Map() + for (const [tabId, layout] of Object.entries(layoutsByTabId)) { + for (const key of readHeldKeys(layout)) { + const holders = holderTabIdsByKey.get(key) + if (holders) { + holders.push(tabId) + } else { + holderTabIdsByKey.set(key, [tabId]) + } + } + } + let healed: Record | null = null + for (const [key, holderTabIds] of holderTabIdsByKey) { + if (holderTabIds.length < 2) { + continue + } + // Why sort over reduce: the id tie-break is deterministic only if every holder is compared. + const [, ...losers] = [...holderTabIds].sort((a, b) => compareOwnerTabIds(a, b, ranking)) + healed ??= { ...layoutsByTabId } + for (const tabId of losers) { + healed[tabId] = surrender(healed[tabId]!, key) + } + } + return healed ?? layoutsByTabId +} + +/** + * The hydration invariant: one leaf id belongs to one tab, and one PTY id is bound by one tab. + * Leaf ids first, then PTY ids over the healed layouts, so a leaf the first pass surrendered is + * not counted twice. Returns the argument itself when nothing collides (STA-7961). + */ +export function resolveDuplicateTerminalLayoutBindings(args: { + canonicalTabIds: ReadonlySet + layoutsByTabId: Record + tabById: ReadonlyMap +}): Record { + const ranking = { canonicalTabIds: args.canonicalTabIds, tabById: args.tabById } + return resolveDuplicateHolders( + resolveDuplicateHolders(args.layoutsByTabId, ranking, collectHeldLeafIds, surrenderLeafId), + ranking, + collectMountedPtyIds, + unbindPtyId + ) +} diff --git a/src/renderer/src/store/terminals/workspace-terminal-layout-plan-duplicate-leaf.test.ts b/src/renderer/src/store/terminals/workspace-terminal-layout-plan-duplicate-leaf.test.ts new file mode 100644 index 00000000000..e39574235f1 --- /dev/null +++ b/src/renderer/src/store/terminals/workspace-terminal-layout-plan-duplicate-leaf.test.ts @@ -0,0 +1,110 @@ +import { describe, expect, it } from 'vitest' +import { buildWorkspaceTerminalLayoutPlan } from './workspace-terminal-layout-plan' +import type { TerminalTab } from '../../../../shared/terminal-tab-types' +import type { WorkspaceSessionState } from '../../../../shared/workspace-session-state-types' + +const WORKTREE_ID = 'wt-1' +// Ids and shapes copied verbatim from the STA-7961 orca-data.json excerpt. +const SHARED_LEAF_ID = '10cb5648-8a54-41c0-a6a4-ef0028d93599' +const SHARED_PTY_ID = 'wt-1@@289ed0f2' +const SPLIT_TAB_ID = 'eba00a9a-17df-4152-8258-42381b48890a' +const SINGLE_TAB_ID = '881a9ee2-7143-46c8-98ac-8ffbb9cf4b2c' + +function tab(id: string, sortOrder: number): TerminalTab { + return { + id, + ptyId: null, + worktreeId: WORKTREE_ID, + title: 'OpenCode', + customTitle: null, + color: null, + sortOrder, + createdAt: 1_789_867_969_623 + sortOrder + } +} + +/** The persisted pair from the report: a nested split and a single leaf, same leaf id and pty. */ +function duplicateLeafSession(): WorkspaceSessionState { + return { + activeRepoId: 'repo-1', + activeWorktreeId: WORKTREE_ID, + activeTabId: SINGLE_TAB_ID, + tabsByWorktree: { [WORKTREE_ID]: [tab(SPLIT_TAB_ID, 0), tab(SINGLE_TAB_ID, 1)] }, + terminalLayoutsByTabId: { + [SPLIT_TAB_ID]: { + root: { + type: 'split', + direction: 'vertical', + first: { + type: 'split', + direction: 'horizontal', + first: { type: 'leaf', leafId: 'df8913c9-fd8a-420a-a7d6-17daf0ed30f0' }, + second: { + type: 'split', + direction: 'horizontal', + first: { type: 'leaf', leafId: SHARED_LEAF_ID }, + second: { type: 'leaf', leafId: '08d0d524-0d46-410b-ba08-b43c97a2b4e4' } + } + }, + second: { type: 'leaf', leafId: '96cdf7ea-9c83-4ba5-a41b-c425955e6606' } + }, + activeLeafId: SHARED_LEAF_ID, + expandedLeafId: null, + ptyIdsByLeafId: { + 'df8913c9-fd8a-420a-a7d6-17daf0ed30f0': 'wt-1@@eaff6e99', + '96cdf7ea-9c83-4ba5-a41b-c425955e6606': 'wt-1@@e905e74a', + [SHARED_LEAF_ID]: SHARED_PTY_ID, + '08d0d524-0d46-410b-ba08-b43c97a2b4e4': 'wt-1@@8123ec24' + } + }, + [SINGLE_TAB_ID]: { + root: { type: 'leaf', leafId: SHARED_LEAF_ID }, + activeLeafId: SHARED_LEAF_ID, + expandedLeafId: null, + ptyIdsByLeafId: { [SHARED_LEAF_ID]: SHARED_PTY_ID } + } + } + } +} + +function planFor(session: WorkspaceSessionState) { + const tabs = session.tabsByWorktree[WORKTREE_ID] ?? [] + return buildWorkspaceTerminalLayoutPlan({ + ownershipTransfersByTabId: new Map(), + ownershipTransferTabIds: null, + releasedPtyIdsByTabId: new Map(), + session, + tabById: new Map(tabs.map((row) => [row.id, row])), + validTabIds: new Set(tabs.map((row) => row.id)) + }) +} + +describe('workspace terminal layout hydration (STA-7961)', () => { + it('binds a persisted leaf id to exactly one tab', () => { + const { layoutsByTabId } = planFor(duplicateLeafSession()) + + const tabsBindingSharedLeaf = Object.entries(layoutsByTabId) + .filter(([, layout]) => SHARED_LEAF_ID in (layout.ptyIdsByLeafId ?? {})) + .map(([tabId]) => tabId) + expect(tabsBindingSharedLeaf).toHaveLength(1) + }) + + it('hands the shared pty to exactly one tab', () => { + const { layoutsByTabId } = planFor(duplicateLeafSession()) + + const tabsBindingSharedPty = Object.entries(layoutsByTabId) + .filter(([, layout]) => Object.values(layout.ptyIdsByLeafId ?? {}).includes(SHARED_PTY_ID)) + .map(([tabId]) => tabId) + expect(tabsBindingSharedPty).toHaveLength(1) + }) + + it('keeps both layouts, and the loser keeps its pane', () => { + const { layoutsByTabId } = planFor(duplicateLeafSession()) + + expect(Object.keys(layoutsByTabId).sort()).toEqual([SINGLE_TAB_ID, SPLIT_TAB_ID].sort()) + expect(layoutsByTabId[SINGLE_TAB_ID]?.root).toEqual({ + type: 'leaf', + leafId: SHARED_LEAF_ID + }) + }) +}) diff --git a/src/renderer/src/store/terminals/workspace-terminal-layout-plan.ts b/src/renderer/src/store/terminals/workspace-terminal-layout-plan.ts index a97af259580..4f1967685ae 100644 --- a/src/renderer/src/store/terminals/workspace-terminal-layout-plan.ts +++ b/src/renderer/src/store/terminals/workspace-terminal-layout-plan.ts @@ -8,6 +8,14 @@ import { import { resolveTerminalLayoutPtyOwnershipTransfers } from '@/components/terminal-pane/terminal-layout-pty-ownership' import { sanitizeTerminalLayoutPaneTitles } from '@/lib/terminal-pane-title-sanitization' import type { TerminalLayoutPtyOwnershipTransfer } from './workspace-terminal-hydration-patch' +import { + readCanonicalTerminalTabIds, + resolveDuplicateTerminalLayoutBindings +} from './workspace-terminal-layout-duplicate-bindings' + +export type WorkspaceTerminalLayoutPlan = { + layoutsByTabId: Record +} export function buildWorkspaceTerminalLayoutPlan({ ownershipTransfersByTabId, @@ -23,8 +31,8 @@ export function buildWorkspaceTerminalLayoutPlan({ session: WorkspaceSessionState tabById: ReadonlyMap validTabIds: ReadonlySet -}): Record { - return Object.fromEntries( +}): WorkspaceTerminalLayoutPlan { + const layoutsByTabId = Object.fromEntries( Object.entries(session.terminalLayoutsByTabId) .filter(([tabId]) => validTabIds.has(tabId)) .map(([tabId, persisted]) => { @@ -55,4 +63,13 @@ export function buildWorkspaceTerminalLayoutPlan({ return [tabId, { ...sanitized, activeLeafId }] }) ) + // Why after per-tab normalization: a duplicated leaf or pty id is only visible once every + // layout has been normalized and had its canonical-row releases applied. + return { + layoutsByTabId: resolveDuplicateTerminalLayoutBindings({ + canonicalTabIds: readCanonicalTerminalTabIds(session), + layoutsByTabId, + tabById + }) + } } diff --git a/src/renderer/src/store/terminals/workspace-terminal-reconnect-plan.ts b/src/renderer/src/store/terminals/workspace-terminal-reconnect-plan.ts index 86a7d9b6b31..877ad74241f 100644 --- a/src/renderer/src/store/terminals/workspace-terminal-reconnect-plan.ts +++ b/src/renderer/src/store/terminals/workspace-terminal-reconnect-plan.ts @@ -1,7 +1,9 @@ import type { Repo } from '../../../../shared/repo-types' +import type { TerminalLayoutSnapshot } from '../../../../shared/terminal-tab-types' import type { Worktree } from '../../../../shared/worktree/types' import type { WorkspaceSessionState } from '../../../../shared/workspace-session-state-types' import { buildByIdIndex, buildWorktreeByIdIndex } from '../slices/worktree-by-id-index' +import { resolvePrimaryLayoutPtyId } from './terminal-pty-identities' export type WorkspaceTerminalReconnectPlan = { pendingReconnectPtyIdByTabId: Record @@ -10,6 +12,7 @@ export type WorkspaceTerminalReconnectPlan = { } export function buildWorkspaceTerminalReconnectPlan({ + layoutsByTabId, reconnectPtyIdByRetainedTabId, releasedPtyIdsByTabId, repos, @@ -18,6 +21,7 @@ export function buildWorkspaceTerminalReconnectPlan({ validWorktreeIds, worktreesByRepo }: { + layoutsByTabId: Record reconnectPtyIdByRetainedTabId: ReadonlyMap releasedPtyIdsByTabId: ReadonlyMap> repos: readonly Repo[] @@ -59,12 +63,18 @@ export function buildWorkspaceTerminalReconnectPlan({ continue } for (const tab of session.tabsByWorktree[worktreeId] ?? []) { + // Why: the layout is the binding, so a row whose layout surrendered its PTY has nothing to + // reattach. A layout with no bindings map at all predates leaf bindings; only that shape + // still answers from the row. + const layout = layoutsByTabId[tab.id] + const reconnectPtyId = + layout?.ptyIdsByLeafId === undefined ? tab.ptyId : resolvePrimaryLayoutPtyId(layout) if ( - tab.ptyId && + reconnectPtyId && validTabIds.has(tab.id) && - !releasedPtyIdsByTabId.get(tab.id)?.has(tab.ptyId) + !releasedPtyIdsByTabId.get(tab.id)?.has(reconnectPtyId) ) { - pendingReconnectPtyIdByTabId[tab.id] = tab.ptyId + pendingReconnectPtyIdByTabId[tab.id] = reconnectPtyId } } }