From 219c91c9970b96e2bfe1d956eaa4296c148d6a90 Mon Sep 17 00:00:00 2001 From: Jinwoo-H Date: Sun, 20 Sep 2026 19:17:17 -0400 Subject: [PATCH] refactor(terminal): make the layout the only PTY ownership source for reveal Ownership becomes tab-keyed and layout-sourced: resolveTerminalPtyPaneOwnership reads no worktree key and does not treat tab.ptyId as a tier, so a pane whose row is filed under another workspace key is still found. A reveal whose PTY is unowned but whose leaf id a layout already holds now adopts that tab instead of minting a second pane with the same identity (STA-7961), and every surfacing site follows the owner's worktree key so verifyTerminalRevealIdentity can attest the binding. --- .../src/hooks/ipc-events-test-harness.ts | 16 +- .../terminal-presentation-ipc-bridge.ts | 53 ++--- ...obile-terminal-reveal-tab-adoption.test.ts | 9 +- ...erminal-reveal-duplicate-leaf-mint.test.ts | 106 +++++++++ ...al-reveal-owner-worktree-surfacing.test.ts | 94 ++++++++ ...al-reveal-split-owner-worktree-key.test.ts | 76 +++++++ .../src/lib/mobile-terminal-tab-mount.test.ts | 35 ++- .../src/lib/mobile-terminal-tab-mount.ts | 32 ++- .../src/lib/terminal-pty-pane-owner.test.ts | 157 +++++++++++++ .../src/lib/terminal-pty-pane-owner.ts | 98 +++++++++ .../terminal-reveal-leaf-owner-lookup.test.ts | 65 ++++++ .../src/lib/terminal-reveal-tab-adoption.ts | 139 ++++++++++++ .../src/lib/terminal-tab-for-pty-id.test.ts | 208 ------------------ .../src/lib/terminal-tab-for-pty-id.ts | 74 ------- .../worktree-agent-live-surface-adoption.ts | 11 +- 15 files changed, 834 insertions(+), 339 deletions(-) create mode 100644 src/renderer/src/hooks/terminal-reveal-duplicate-leaf-mint.test.ts create mode 100644 src/renderer/src/hooks/terminal-reveal-owner-worktree-surfacing.test.ts create mode 100644 src/renderer/src/hooks/terminal-reveal-split-owner-worktree-key.test.ts create mode 100644 src/renderer/src/lib/terminal-pty-pane-owner.test.ts create mode 100644 src/renderer/src/lib/terminal-pty-pane-owner.ts create mode 100644 src/renderer/src/lib/terminal-reveal-leaf-owner-lookup.test.ts create mode 100644 src/renderer/src/lib/terminal-reveal-tab-adoption.ts delete mode 100644 src/renderer/src/lib/terminal-tab-for-pty-id.test.ts delete mode 100644 src/renderer/src/lib/terminal-tab-for-pty-id.ts diff --git a/src/renderer/src/hooks/ipc-events-test-harness.ts b/src/renderer/src/hooks/ipc-events-test-harness.ts index 69ab58bf9d8..1c3325e8610 100644 --- a/src/renderer/src/hooks/ipc-events-test-harness.ts +++ b/src/renderer/src/hooks/ipc-events-test-harness.ts @@ -62,6 +62,10 @@ export type IpcEventsHarness = { settleClientHostedBrowserRowsSnapshot: () => Promise /** Standard (non-palette) target of a workspace digit chord. */ activateAndRevealWorkspace: ReturnType + /** Runtime-surface focus attempt; its third argument is the worktree key a reveal surfaces under. */ + focusRuntimeTerminalSurface: ReturnType + /** Window CustomEvents the bridge emits, e.g. split-pane and background-mount requests. */ + dispatchEvent: ReturnType } export type IpcEventsHarnessOptions = { @@ -83,6 +87,8 @@ export async function loadIpcEventsHarness( ): Promise { const replyTerminalCreate = vi.fn() const activateAndRevealWorkspace = vi.fn() + const focusRuntimeTerminalSurface = vi.fn(() => false) + const dispatchEvent = vi.fn() let createTerminalListener: ((request: CreateTerminalRequest) => void) | null = null let requestTerminalCreateListener: ((request: RequestTerminalCreateRequest) => void) | null = null let focusEditorTabListener: ((request: { tabId: string; worktreeId: string }) => void) | null = @@ -134,13 +140,11 @@ export async function loadIpcEventsHarness( isWebRuntimeSessionActive: vi.fn(() => false) })) vi.doMock('@/lib/focus-terminal-tab-surface', () => ({ focusTerminalTabSurface: vi.fn() })) - vi.doMock('@/runtime/sync-runtime-graph', () => ({ - focusRuntimeTerminalSurface: vi.fn(() => false) - })) + vi.doMock('@/runtime/sync-runtime-graph', () => ({ focusRuntimeTerminalSurface })) vi.doMock('@/lib/activate-tab-and-focus-pane', () => ({ activateTabAndFocusPane: vi.fn() })) vi.stubGlobal('window', { - dispatchEvent: vi.fn(), + dispatchEvent, api: new Proxy( { runtimeEnvironments: createApiNamespaceStub({ @@ -291,7 +295,9 @@ export async function loadIpcEventsHarness( await Promise.resolve() await Promise.resolve() }, - activateAndRevealWorkspace + activateAndRevealWorkspace, + focusRuntimeTerminalSurface, + dispatchEvent } } diff --git a/src/renderer/src/hooks/ipc-events/terminal-presentation-ipc-bridge.ts b/src/renderer/src/hooks/ipc-events/terminal-presentation-ipc-bridge.ts index 47835ab529d..3f4fcc58b6c 100644 --- a/src/renderer/src/hooks/ipc-events/terminal-presentation-ipc-bridge.ts +++ b/src/renderer/src/hooks/ipc-events/terminal-presentation-ipc-bridge.ts @@ -1,7 +1,7 @@ import { requestBackgroundTerminalWorktreeMount } from '@/components/terminal/background-terminal-worktree-mount' import { hasRegisteredRuntimeTerminalTab } from '@/runtime/sync-runtime-graph' import { planMobileTerminalTabMount } from '@/lib/mobile-terminal-tab-mount' -import { resolveTerminalTabPtyOwnership } from '@/lib/terminal-tab-for-pty-id' +import { resolveTerminalRevealTarget } from '@/lib/terminal-reveal-tab-adoption' import { SPLIT_TERMINAL_PANE_EVENT } from '@/constants/terminal' import type { SplitTerminalPaneDetail } from '@/constants/terminal' import { singlePaneLayoutSnapshot } from '@/store/slices/terminal-helpers' @@ -54,32 +54,22 @@ export function registerTerminalPresentationIpcBridge(unsubs: (() => void)[]): v }) const shouldActivate = terminalPresentation === 'focused' const shouldSurfaceOwner = terminalPresentation !== 'background' && surfaceOwner !== false + // Why: a split pane revealed from mobile is only bound in the persisted layout until + // its pane mounts, and its row can sit under another worktree key (#10486, STA-7961). + const revealTarget = resolveTerminalRevealTarget(store, { + worktreeId, + ...(ptyId ? { ptyId } : {}), + ...(tabId !== undefined ? { tabId } : {}), + ...(leafId ? { leafId } : {}), + ...(splitFromLeafId ? { splitFromLeafId } : {}) + }) + // Why: every surfacing site below must use the owner's key, not the event's, or + // verifyTerminalRevealIdentity looks the tab up under a key that does not hold it. + const ownerWorktreeId = revealTarget.ownerWorktreeId if (shouldActivate) { - activateTerminalInitiatedWorktree(store, worktreeId) + activateTerminalInitiatedWorktree(store, ownerWorktreeId) } - const worktreeTabs = store.tabsByWorktree[worktreeId] ?? [] - // Why: a split pane revealed from mobile is only bound in the persisted - // layout until its pane mounts; missing it minted a duplicate tab (#10486). - const ownership = ptyId - ? resolveTerminalTabPtyOwnership( - store, - worktreeId, - ptyId, - tabId !== undefined ? { preferTabId: tabId } : {} - ) - : { kind: 'none' as const } - const existingTab = - ownership.kind === 'owned' - ? worktreeTabs.find((candidate) => candidate.id === ownership.tabId) - : undefined - const isSplitReveal = Boolean(ptyId && tabId && leafId && splitFromLeafId) - const splitTargetTab = isSplitReveal - ? worktreeTabs.find((candidate) => candidate.id === tabId) - : undefined - if (isSplitReveal && !splitTargetTab) { - throw new Error(`Terminal tab ${tabId} not found`) - } - const reusedTab = existingTab ?? splitTargetTab + const reusedTab = revealTarget.tab const tab = reusedTab ?? (ptyId @@ -130,8 +120,8 @@ export function registerTerminalPresentationIpcBridge(unsubs: (() => void)[]): v store.setActiveTab(tab.id) } if (shouldSurfaceOwner) { - store.revealWorktreeInSidebar(worktreeId) - focusTerminalInitiatedTab(tab.id, leafId, worktreeId) + store.revealWorktreeInSidebar(ownerWorktreeId) + focusTerminalInitiatedTab(tab.id, leafId, ownerWorktreeId) } // Why: only stamp the runtime title on fresh tabs; reused tabs may have a user customTitle it would overwrite on focus. if (title && !reusedTab) { @@ -172,7 +162,7 @@ export function registerTerminalPresentationIpcBridge(unsubs: (() => void)[]): v new CustomEvent(SPLIT_TERMINAL_PANE_EVENT, { detail: { tabId: tab.id, - worktreeId, + worktreeId: ownerWorktreeId, paneRuntimeId: -1, direction: splitDirection ?? 'horizontal', sourceLeafId: splitFromLeafId, @@ -212,14 +202,17 @@ export function registerTerminalPresentationIpcBridge(unsubs: (() => void)[]): v }) } if (ptyId && terminalPresentation === 'background') { - requestBackgroundTerminalWorktreeMount({ worktreeId, tabIds: [tab.id] }) + requestBackgroundTerminalWorktreeMount({ + worktreeId: ownerWorktreeId, + tabIds: [tab.id] + }) } if (requestId) { // Why: attest the actual binding; recovery callers compare it with their expected identity. const identity = ptyId && tabId && leafId ? verifyTerminalRevealIdentity(useAppStore.getState(), { - worktreeId, + worktreeId: ownerWorktreeId, tabId: tab.id, leafId, ptyId diff --git a/src/renderer/src/hooks/mobile-terminal-reveal-tab-adoption.test.ts b/src/renderer/src/hooks/mobile-terminal-reveal-tab-adoption.test.ts index 4f5afa434fd..0ce50eb201e 100644 --- a/src/renderer/src/hooks/mobile-terminal-reveal-tab-adoption.test.ts +++ b/src/renderer/src/hooks/mobile-terminal-reveal-tab-adoption.test.ts @@ -211,9 +211,10 @@ describe('mobile terminal reveal tab adoption', () => { }) }) - it('replies without an error when two recorded bindings both claim the pty', async () => { - // Ambiguity is unresolvable, so the reveal still creates a tab — but it must - // not reject, because the mobile focus path awaits it with no catch. + it('adopts the sole layout claimant now that the tab row is not an ownership tier', async () => { + // tab-stale-a holds the pty only through its row, which no longer binds anything, so + // tab-stale-b is the one claimant. The reply must still not reject: the mobile focus + // path awaits it with no catch. const storeState: HarnessStoreState = createHarnessStoreState({ tabsByWorktree: { [WORKTREE_ID]: [ @@ -238,7 +239,7 @@ describe('mobile terminal reveal tab adoption', () => { expect(harness.replyTerminalCreate).toHaveBeenCalledWith({ requestId: 'mobile-reveal', - tabId: 'tab-minted', + tabId: 'tab-stale-b', title: 'codex' }) }) diff --git a/src/renderer/src/hooks/terminal-reveal-duplicate-leaf-mint.test.ts b/src/renderer/src/hooks/terminal-reveal-duplicate-leaf-mint.test.ts new file mode 100644 index 00000000000..5c095ca0ab5 --- /dev/null +++ b/src/renderer/src/hooks/terminal-reveal-duplicate-leaf-mint.test.ts @@ -0,0 +1,106 @@ +// A reveal that mints a tab for a leaf id some layout already holds hands two panes one pane +// identity, and the renderer delivers PTY data to one handler slot per pty id — so one pane +// starves and both fight over the grid size. That is STA-7961. +import { describe, expect, it } from 'vitest' +import { + createHarnessStoreState, + loadIpcEventsHarness, + type HarnessStoreState +} from './ipc-events-test-harness' + +const WORKTREE_ID = 'wt-1' +const SHARED_LEAF_ID = 'leaf-shared' +const SHARED_PTY_ID = 'pty-shared' + +/** Tabs whose layout binds SHARED_LEAF_ID, whatever pty it points at. */ +function tabsBindingSharedLeaf(state: HarnessStoreState): string[] { + return Object.entries(state.terminalLayoutsByTabId) + .filter(([, layout]) => SHARED_LEAF_ID in (layout.ptyIdsByLeafId ?? {})) + .map(([tabId]) => tabId) +} + +function revealUnownedPty(harness: { + createTerminal: (request: { + requestId?: string + worktreeId: string + ptyId?: string + leafId?: string + presentation?: 'background' | 'focused' + title?: string + }) => void +}): void { + harness.createTerminal({ + requestId: 'reveal', + worktreeId: WORKTREE_ID, + ptyId: SHARED_PTY_ID, + leafId: SHARED_LEAF_ID, + presentation: 'focused', + title: 'OpenCode' + }) +} + +describe('terminal reveal must not mint a tab that reuses a bound leaf id (STA-7961)', () => { + it('leaves the leaf id bound to one tab when the owning row is filed under another workspace key', async () => { + // terminalLayoutsByTabId is keyed by tab id alone, so it still records tab-a's + // split leaf while the reveal's worktree key lists entirely different rows. + const storeState: HarnessStoreState = createHarnessStoreState({ + tabsByWorktree: { + [WORKTREE_ID]: [{ id: 'tab-other', ptyId: 'pty-other', title: 'Terminal 3' }], + 'wt-other': [{ id: 'tab-a', ptyId: 'pty-a', title: 'Terminal 1' }] + }, + ptyIdsByTabId: {}, + terminalLayoutsByTabId: { + 'tab-a': { ptyIdsByLeafId: { 'leaf-a': 'pty-a', [SHARED_LEAF_ID]: SHARED_PTY_ID } } + } + }) + const harness = await loadIpcEventsHarness(storeState) + harness.useIpcEvents() + + revealUnownedPty(harness) + + expect(storeState.createTab).not.toHaveBeenCalled() + expect(tabsBindingSharedLeaf(storeState)).toEqual(['tab-a']) + }) + + it('refuses to bind a leaf id a sibling tab in the same worktree already owns', async () => { + // The pty is unowned, so ownership resolves to none; the leaf id is not, and + // re-minting it hands two tabs the same pane identity. + const storeState: HarnessStoreState = createHarnessStoreState({ + tabsByWorktree: { + [WORKTREE_ID]: [{ id: 'tab-a', ptyId: 'pty-a', title: 'Terminal 1' }] + }, + ptyIdsByTabId: {}, + terminalLayoutsByTabId: { + 'tab-a': { ptyIdsByLeafId: { 'leaf-a': 'pty-a', [SHARED_LEAF_ID]: 'pty-stale' } } + } + }) + const harness = await loadIpcEventsHarness(storeState) + harness.useIpcEvents() + + revealUnownedPty(harness) + + expect(storeState.createTab).not.toHaveBeenCalled() + expect(tabsBindingSharedLeaf(storeState)).toEqual(['tab-a']) + }) + + it('adopts the recorded owner instead of minting when the layout binds the pty but no pane is mounted', async () => { + // Control for the hibernated-pane hypothesis: a recorded layout row alone is + // enough for the resolver, so this path never reaches the bound-leaf branch. + const storeState: HarnessStoreState = createHarnessStoreState({ + tabsByWorktree: { + [WORKTREE_ID]: [{ id: 'tab-a', ptyId: 'pty-a', title: 'Terminal 1' }] + }, + ptyIdsByTabId: { 'tab-a': [] }, + terminalLayoutsByTabId: { + 'tab-a': { ptyIdsByLeafId: { 'leaf-a': 'pty-a', [SHARED_LEAF_ID]: SHARED_PTY_ID } } + } + }) + const harness = await loadIpcEventsHarness(storeState) + harness.useIpcEvents() + + revealUnownedPty(harness) + + expect(storeState.createTab).not.toHaveBeenCalled() + expect(tabsBindingSharedLeaf(storeState)).toEqual(['tab-a']) + }) +}) diff --git a/src/renderer/src/hooks/terminal-reveal-owner-worktree-surfacing.test.ts b/src/renderer/src/hooks/terminal-reveal-owner-worktree-surfacing.test.ts new file mode 100644 index 00000000000..6b4b5666c80 --- /dev/null +++ b/src/renderer/src/hooks/terminal-reveal-owner-worktree-surfacing.test.ts @@ -0,0 +1,94 @@ +// Ownership is tab-keyed, so the row a reveal adopts can be filed under a worktree key other +// than the event's. Every surfacing site must follow the owner's key: verifyTerminalRevealIdentity +// looks the tab up under the key it is handed, and throws when that key does not hold it. +import { describe, expect, it } from 'vitest' +import { BACKGROUND_MOUNT_TERMINAL_WORKTREE_EVENT } from '@/constants/terminal' +import { + createHarnessStoreState, + loadIpcEventsHarness, + type HarnessStoreState, + type IpcEventsHarness +} from './ipc-events-test-harness' + +const EVENT_WORKTREE_ID = 'wt-1' +const OWNER_WORKTREE_ID = 'wt-other' + +function storeWithOwnerFiledElsewhere(): HarnessStoreState { + return createHarnessStoreState({ + tabsByWorktree: { + [EVENT_WORKTREE_ID]: [{ id: 'tab-other', ptyId: 'pty-other', title: 'Terminal 3' }], + [OWNER_WORKTREE_ID]: [{ id: 'tab-a', ptyId: 'pty-a', title: 'Terminal 1' }] + }, + ptyIdsByTabId: {}, + terminalLayoutsByTabId: { + 'tab-a': { + root: { type: 'leaf', leafId: 'leaf-a' }, + ptyIdsByLeafId: { 'leaf-a': 'pty-a' } + } + } + }) +} + +function revealOwnedPane(harness: IpcEventsHarness, presentation: 'background' | 'focused'): void { + harness.createTerminal({ + requestId: 'reveal', + worktreeId: EVENT_WORKTREE_ID, + ptyId: 'pty-a', + tabId: 'tab-a', + leafId: 'leaf-a', + presentation + }) +} + +describe('a reveal surfaces its owner under the owner’s worktree key', () => { + it('activates, reveals and focuses the owner workspace, not the event’s', async () => { + const storeState = storeWithOwnerFiledElsewhere() + const harness = await loadIpcEventsHarness(storeState) + harness.useIpcEvents() + + revealOwnedPane(harness, 'focused') + + expect(storeState.setActiveWorktree).toHaveBeenCalledWith(OWNER_WORKTREE_ID) + expect(storeState.revealWorktreeInSidebar).toHaveBeenCalledWith(OWNER_WORKTREE_ID) + expect(harness.focusRuntimeTerminalSurface).toHaveBeenCalledWith( + 'tab-a', + 'leaf-a', + OWNER_WORKTREE_ID + ) + }) + + it('attests the identity under the owner key, so the reply carries one instead of an error', async () => { + const storeState = storeWithOwnerFiledElsewhere() + const harness = await loadIpcEventsHarness(storeState) + harness.useIpcEvents() + + revealOwnedPane(harness, 'focused') + + expect(harness.replyTerminalCreate).toHaveBeenCalledWith({ + requestId: 'reveal', + tabId: 'tab-a', + title: 'Terminal 1', + identity: { + worktreeId: OWNER_WORKTREE_ID, + tabId: 'tab-a', + leafId: 'leaf-a', + ptyId: 'pty-a' + } + }) + }) + + it('background-mounts the owner workspace rather than the event’s', async () => { + const storeState = storeWithOwnerFiledElsewhere() + const harness = await loadIpcEventsHarness(storeState) + harness.useIpcEvents() + + revealOwnedPane(harness, 'background') + + const mountEvents = harness.dispatchEvent.mock.calls + .map(([event]) => event as CustomEvent<{ worktreeId: string; tabIds?: string[] }>) + .filter((event) => event.type === BACKGROUND_MOUNT_TERMINAL_WORKTREE_EVENT) + expect(mountEvents.map((event) => event.detail)).toEqual([ + { worktreeId: OWNER_WORKTREE_ID, tabIds: ['tab-a'] } + ]) + }) +}) diff --git a/src/renderer/src/hooks/terminal-reveal-split-owner-worktree-key.test.ts b/src/renderer/src/hooks/terminal-reveal-split-owner-worktree-key.test.ts new file mode 100644 index 00000000000..f182f08aca1 --- /dev/null +++ b/src/renderer/src/hooks/terminal-reveal-split-owner-worktree-key.test.ts @@ -0,0 +1,76 @@ +// A split reveal names its parent tab by id, but that row can sit under a worktree key other +// than the event's (STA-7961). Looking the hint up in the event worktree's list alone failed the +// whole reveal, so the split pane never appeared. +import { describe, expect, it } from 'vitest' +import { collectLeafIdsInOrder } from '@/components/terminal-pane/layout-serialization' +import { + createHarnessStoreState, + loadIpcEventsHarness, + type HarnessStoreState +} from './ipc-events-test-harness' + +const EVENT_WORKTREE_ID = 'wt-1' +const OWNER_WORKTREE_ID = 'wt-other' + +function storeWithOwnerFiledElsewhere(): HarnessStoreState { + return createHarnessStoreState({ + tabsByWorktree: { + [EVENT_WORKTREE_ID]: [{ id: 'tab-other', ptyId: 'pty-other', title: 'Terminal 3' }], + [OWNER_WORKTREE_ID]: [{ id: 'tab-a', ptyId: 'pty-a', title: 'Terminal 1' }] + }, + ptyIdsByTabId: { 'tab-a': ['pty-a'] }, + terminalLayoutsByTabId: { + 'tab-a': { + root: { type: 'leaf', leafId: 'leaf-a' }, + ptyIdsByLeafId: { 'leaf-a': 'pty-a' } + } + } + }) +} + +describe('split reveal whose target tab is filed under another worktree key', () => { + it('splits the owner tab instead of failing the reveal', async () => { + const storeState = storeWithOwnerFiledElsewhere() + const harness = await loadIpcEventsHarness(storeState) + harness.useIpcEvents() + + harness.createTerminal({ + requestId: 'reveal', + worktreeId: EVENT_WORKTREE_ID, + ptyId: 'pty-split', + tabId: 'tab-a', + leafId: 'leaf-split', + splitFromLeafId: 'leaf-a', + presentation: 'focused' + }) + + const reply = harness.replyTerminalCreate.mock.calls[0]?.[0] + expect(reply).toMatchObject({ requestId: 'reveal', tabId: 'tab-a' }) + expect(reply.error).toBeUndefined() + expect(storeState.createTab).not.toHaveBeenCalled() + const layout = storeState.terminalLayoutsByTabId['tab-a'] + expect(collectLeafIdsInOrder(layout.root ?? null)).toEqual(['leaf-a', 'leaf-split']) + expect(layout.ptyIdsByLeafId).toMatchObject({ 'leaf-split': 'pty-split' }) + }) + + it('still fails a split reveal whose parent row exists under no worktree key', async () => { + const storeState = storeWithOwnerFiledElsewhere() + const harness = await loadIpcEventsHarness(storeState) + harness.useIpcEvents() + + harness.createTerminal({ + requestId: 'reveal', + worktreeId: EVENT_WORKTREE_ID, + ptyId: 'pty-split', + tabId: 'tab-gone', + leafId: 'leaf-split', + splitFromLeafId: 'leaf-a', + presentation: 'focused' + }) + + expect(harness.replyTerminalCreate).toHaveBeenCalledWith({ + requestId: 'reveal', + error: 'Terminal tab tab-gone not found' + }) + }) +}) diff --git a/src/renderer/src/lib/mobile-terminal-tab-mount.test.ts b/src/renderer/src/lib/mobile-terminal-tab-mount.test.ts index 01da9791eb0..0e05c989cc2 100644 --- a/src/renderer/src/lib/mobile-terminal-tab-mount.test.ts +++ b/src/renderer/src/lib/mobile-terminal-tab-mount.test.ts @@ -1,17 +1,30 @@ +// Ownership itself is worktree-agnostic; this planner is where the worktree scope lives, because +// it mounts the tab under the requested worktree and a row filed elsewhere cannot be mounted there. import { describe, expect, it, vi } from 'vitest' import type { AppState } from '@/store/types' -import { planMobileTerminalTabMount } from './mobile-terminal-tab-mount' -import type { TerminalTabPtyOwnershipState } from './terminal-tab-for-pty-id' +import { + planMobileTerminalTabMount, + type MobileTerminalTabMountState +} from './mobile-terminal-tab-mount' -function state(tabCount = 1): TerminalTabPtyOwnershipState { +/** `tabCount` rows in `wt`, each with its own single-leaf layout bound to `wt@@`. */ +function state(tabCount = 1): MobileTerminalTabMountState { + const indices = Array.from({ length: tabCount }, (_, index) => index) return { tabsByWorktree: { - wt: Array.from({ length: tabCount }, (_, index) => ({ - id: `tab-${index}`, - ptyId: `wt@@${index}` - })) + wt: indices.map((index) => ({ id: `tab-${index}`, ptyId: null })) } as unknown as AppState['tabsByWorktree'], - terminalLayoutsByTabId: {}, + terminalLayoutsByTabId: Object.fromEntries( + indices.map((index) => [ + `tab-${index}`, + { + root: { type: 'leaf', leafId: `leaf-${index}` }, + activeLeafId: `leaf-${index}`, + expandedLeafId: null, + ptyIdsByLeafId: { [`leaf-${index}`]: `wt@@${index}` } + } + ]) + ) as unknown as AppState['terminalLayoutsByTabId'], ptyIdsByTabId: {} } } @@ -65,6 +78,12 @@ describe('planMobileTerminalTabMount', () => { }) }) + it('refuses a pty whose owning row is filed under another worktree key', () => { + // Absorbed from the deleted terminal-tab-for-pty-id suite: the lookup no longer scopes by + // worktree, so the planner must, or a stale handle mounts a hidden workspace (#8597). + expect(planMobileTerminalTabMount(state(), { worktreeId: 'other', ptyId: 'wt@@0' })).toBeNull() + }) + it('does not mount a hidden worktree for a stale direct tab id', () => { const isTabMounted = vi.fn() diff --git a/src/renderer/src/lib/mobile-terminal-tab-mount.ts b/src/renderer/src/lib/mobile-terminal-tab-mount.ts index 60206684200..5f5f245a06a 100644 --- a/src/renderer/src/lib/mobile-terminal-tab-mount.ts +++ b/src/renderer/src/lib/mobile-terminal-tab-mount.ts @@ -1,8 +1,13 @@ import type { BackgroundMountTerminalWorktreeDetail } from '@/constants/terminal' +import type { AppState } from '@/store/types' import { - resolveTerminalTabIdForPtyId, - type TerminalTabPtyOwnershipState -} from './terminal-tab-for-pty-id' + resolveTerminalPtyPaneOwnership, + type TerminalPtyPaneOwnerState +} from './terminal-pty-pane-owner' +import { findTerminalTabRow } from './terminal-reveal-tab-adoption' + +export type MobileTerminalTabMountState = TerminalPtyPaneOwnerState & + Pick export type MobileTerminalTabMountRequest = { worktreeId: string @@ -14,9 +19,26 @@ type MobileTerminalTabMountOptions = { isTabMounted?: (tabId: string, worktreeId?: string) => boolean } +/** + * Why scoped here and not in the lookup: ownership is worktree-agnostic, but this caller mounts + * the tab under the requested worktree, so a row filed elsewhere is not a usable answer (#8597). + */ +function resolvePtyOwnerTabIdInWorktree( + state: MobileTerminalTabMountState, + worktreeId: string, + ptyId: string +): string | null { + const ownership = resolveTerminalPtyPaneOwnership(state, ptyId) + if (ownership.kind !== 'owned') { + return null + } + const row = findTerminalTabRow(state, ownership.owner.tabId) + return row?.worktreeId === worktreeId ? ownership.owner.tabId : null +} + /** Why: exact-tab planning prevents a stale ptyId from mounting every saved xterm (#8597). */ export function planMobileTerminalTabMount( - state: TerminalTabPtyOwnershipState, + state: MobileTerminalTabMountState, request: MobileTerminalTabMountRequest, options: MobileTerminalTabMountOptions = {} ): BackgroundMountTerminalWorktreeDetail | null { @@ -33,7 +55,7 @@ export function planMobileTerminalTabMount( ? request.tabId : null : request.ptyId - ? resolveTerminalTabIdForPtyId(state, request.worktreeId, request.ptyId) + ? resolvePtyOwnerTabIdInWorktree(state, request.worktreeId, request.ptyId) : null // Why: replaying the background-mount event for a live pane restarts its // three-second hidden measurement window on every mobile reconnect. diff --git a/src/renderer/src/lib/terminal-pty-pane-owner.test.ts b/src/renderer/src/lib/terminal-pty-pane-owner.test.ts new file mode 100644 index 00000000000..946745bc2be --- /dev/null +++ b/src/renderer/src/lib/terminal-pty-pane-owner.test.ts @@ -0,0 +1,157 @@ +// Ownership is tab-keyed and layout-sourced: no worktree key, and the tab row's own `ptyId` +// is not a tier. Two panes holding one PTY is the STA-7961 symptom this resolver must refuse. +import { describe, expect, it } from 'vitest' +import { + listTerminalPtyPaneOwners, + resolveTerminalPtyPaneOwnership, + type TerminalPtyPaneOwnerState +} from './terminal-pty-pane-owner' +import type { AppState } from '@/store/types' +import type { TerminalPaneLayoutNode } from '../../../shared/terminal-tab-types' + +const PTY_ID = 'wt@@1' + +function state(partial: { + layouts?: Record< + string, + { root?: TerminalPaneLayoutNode | null; ptyIdsByLeafId?: Record } + > + livePtyIds?: Record +}): TerminalPtyPaneOwnerState { + return { + terminalLayoutsByTabId: (partial.layouts ?? + {}) as unknown as AppState['terminalLayoutsByTabId'], + ptyIdsByTabId: partial.livePtyIds ?? {} + } +} + +function leaf(leafId: string): TerminalPaneLayoutNode { + return { type: 'leaf', leafId } +} + +describe('resolveTerminalPtyPaneOwnership', () => { + it('reports no owner when nothing binds the pty', () => { + const s = state({ layouts: { 'tab-a': { root: leaf('leaf-a'), ptyIdsByLeafId: {} } } }) + expect(resolveTerminalPtyPaneOwnership(s, PTY_ID)).toEqual({ kind: 'none' }) + }) + + it('names the bound leaf of the one recorded owner', () => { + const s = state({ + layouts: { 'tab-a': { root: leaf('leaf-a'), ptyIdsByLeafId: { 'leaf-a': PTY_ID } } } + }) + expect(resolveTerminalPtyPaneOwnership(s, PTY_ID)).toEqual({ + kind: 'owned', + owner: { tabId: 'tab-a', leafId: 'leaf-a', tier: 'recorded' } + }) + }) + + it('counts a rootless layout, which binds its sole pane off-tree', () => { + const s = state({ layouts: { 'tab-a': { ptyIdsByLeafId: { 'leaf-a': PTY_ID } } } }) + expect(resolveTerminalPtyPaneOwnership(s, PTY_ID)).toEqual({ + kind: 'owned', + owner: { tabId: 'tab-a', leafId: 'leaf-a', tier: 'recorded' } + }) + }) + + it('skips a stranded binding whose leaf already left the tree', () => { + // The pane was detached; the map entry it left behind reattaches nothing (#13098). + const s = state({ + layouts: { + 'tab-ghost': { root: leaf('leaf-other'), ptyIdsByLeafId: { 'leaf-a': PTY_ID } }, + 'tab-live': { root: leaf('leaf-a'), ptyIdsByLeafId: { 'leaf-a': PTY_ID } } + } + }) + expect(resolveTerminalPtyPaneOwnership(s, PTY_ID)).toEqual({ + kind: 'owned', + owner: { tabId: 'tab-live', leafId: 'leaf-a', tier: 'recorded' } + }) + }) + + it('lets a mounted pane decide over a recorded one, with no leaf of its own', () => { + const s = state({ + layouts: { 'tab-stale': { root: leaf('leaf-x'), ptyIdsByLeafId: { 'leaf-x': PTY_ID } } }, + livePtyIds: { 'tab-mounted': [PTY_ID] } + }) + expect(resolveTerminalPtyPaneOwnership(s, PTY_ID)).toEqual({ + kind: 'owned', + owner: { tabId: 'tab-mounted', leafId: null, tier: 'mounted' } + }) + }) + + it('reports ambiguity when two recorded bindings claim the pty', () => { + const s = state({ + layouts: { + 'tab-b': { root: leaf('leaf-b'), ptyIdsByLeafId: { 'leaf-b': PTY_ID } }, + 'tab-a': { root: leaf('leaf-a'), ptyIdsByLeafId: { 'leaf-a': PTY_ID } } + } + }) + const ownership = resolveTerminalPtyPaneOwnership(s, PTY_ID) + expect(ownership.kind).toBe('ambiguous') + // Why ordered: persistence order must not decide which claimant the reveal adopts. + expect(ownership.kind === 'ambiguous' && ownership.owners.map((o) => o.tabId)).toEqual([ + 'tab-a', + 'tab-b' + ]) + }) + + it('reports ambiguity when two mounted panes claim the pty', () => { + const s = state({ livePtyIds: { 'tab-a': [PTY_ID], 'tab-b': [PTY_ID] } }) + expect(resolveTerminalPtyPaneOwnership(s, PTY_ID).kind).toBe('ambiguous') + }) + + it('breaks a same-tier conflict with the pre-minted tab id', () => { + const s = state({ + layouts: { + 'tab-a': { root: leaf('leaf-a'), ptyIdsByLeafId: { 'leaf-a': PTY_ID } }, + 'tab-b': { root: leaf('leaf-b'), ptyIdsByLeafId: { 'leaf-b': PTY_ID } } + } + }) + expect(resolveTerminalPtyPaneOwnership(s, PTY_ID, { preferTabId: 'tab-b' })).toEqual({ + kind: 'owned', + owner: { tabId: 'tab-b', leafId: 'leaf-b', tier: 'recorded' } + }) + }) + + it('keeps a sole owner over a tab id the pty outgrew', () => { + // A pane dragged to another tab keeps its leaf binding; the id baked into the PTY env does not move. + const s = state({ + layouts: { + 'tab-detached-to': { root: leaf('leaf-a'), ptyIdsByLeafId: { 'leaf-a': PTY_ID } } + } + }) + expect(resolveTerminalPtyPaneOwnership(s, PTY_ID, { preferTabId: 'tab-minted-in' })).toEqual({ + kind: 'owned', + owner: { tabId: 'tab-detached-to', leafId: 'leaf-a', tier: 'recorded' } + }) + }) + + it('falls back to the pre-minted tab id when nothing records the pty (#10486)', () => { + expect( + resolveTerminalPtyPaneOwnership(state({}), PTY_ID, { preferTabId: 'tab-hinted' }) + ).toEqual({ kind: 'owned', owner: { tabId: 'tab-hinted', leafId: null, tier: 'recorded' } }) + }) + + it('owns a pty whose only holder is filed under a foreign worktree key', () => { + // The resolver never reads tabsByWorktree, so a row filed elsewhere is still an owner. + const s = state({ + layouts: { 'tab-elsewhere': { root: leaf('leaf-a'), ptyIdsByLeafId: { 'leaf-a': PTY_ID } } } + }) + expect(resolveTerminalPtyPaneOwnership(s, PTY_ID)).toMatchObject({ + kind: 'owned', + owner: { tabId: 'tab-elsewhere' } + }) + }) +}) + +describe('listTerminalPtyPaneOwners', () => { + it('lists every claimant, mounted tier first', () => { + const s = state({ + layouts: { 'tab-a': { root: leaf('leaf-a'), ptyIdsByLeafId: { 'leaf-a': PTY_ID } } }, + livePtyIds: { 'tab-z': [PTY_ID] } + }) + expect(listTerminalPtyPaneOwners(s, PTY_ID)).toEqual([ + { tabId: 'tab-z', leafId: null, tier: 'mounted' }, + { tabId: 'tab-a', leafId: 'leaf-a', tier: 'recorded' } + ]) + }) +}) diff --git a/src/renderer/src/lib/terminal-pty-pane-owner.ts b/src/renderer/src/lib/terminal-pty-pane-owner.ts new file mode 100644 index 00000000000..4a71a3e7dd1 --- /dev/null +++ b/src/renderer/src/lib/terminal-pty-pane-owner.ts @@ -0,0 +1,98 @@ +import { collectLeafIdsInOrder } from '@/components/terminal-pane/terminal-layout-leaf-ids' +import type { AppState } from '@/store/types' + +/** No `tabsByWorktree`: ownership is tab-keyed, so no worktree key participates. */ +export type TerminalPtyPaneOwnerState = Pick + +export type TerminalPtyPaneOwner = { + tabId: string + /** The leaf the layout binds; null when only the live map proves the mount. */ + leafId: string | null + tier: 'mounted' | 'recorded' +} + +export type TerminalPtyPaneOwnership = + | { kind: 'owned'; owner: TerminalPtyPaneOwner } + | { kind: 'ambiguous'; owners: TerminalPtyPaneOwner[] } + | { kind: 'none' } + +export type TerminalPtyPaneOwnerOptions = { + /** Tab id baked into the PTY's env; a tie-break and a last resort, never a binding. */ + preferTabId?: string +} + +/** + * The leaf a tab's layout binds to `ptyId`, or null. + * + * A binding whose leaf has left the tree reattaches nothing, so it must not outrank a live + * pane (#13098). Rootless layouts bind their sole pane off-tree and keep counting. + */ +function findLayoutBoundLeafId( + state: TerminalPtyPaneOwnerState, + tabId: string, + ptyId: string +): string | null { + const layout = state.terminalLayoutsByTabId[tabId] + if (!layout?.ptyIdsByLeafId) { + return null + } + const leafIdsInTree = layout.root ? new Set(collectLeafIdsInOrder(layout.root)) : null + for (const [leafId, boundPtyId] of Object.entries(layout.ptyIdsByLeafId)) { + if (boundPtyId === ptyId && (!leafIdsInTree || leafIdsInTree.has(leafId))) { + return leafId + } + } + return null +} + +/** Every candidate, strongest tier first; the raw input to the verdict below. */ +export function listTerminalPtyPaneOwners( + state: TerminalPtyPaneOwnerState, + ptyId: string +): TerminalPtyPaneOwner[] { + const mounted: TerminalPtyPaneOwner[] = [] + const recorded: TerminalPtyPaneOwner[] = [] + const tabIds = new Set([ + ...Object.keys(state.ptyIdsByTabId), + ...Object.keys(state.terminalLayoutsByTabId) + ]) + for (const tabId of tabIds) { + const leafId = findLayoutBoundLeafId(state, tabId, ptyId) + if (state.ptyIdsByTabId[tabId]?.includes(ptyId)) { + mounted.push({ tabId, leafId, tier: 'mounted' }) + } else if (leafId !== null) { + recorded.push({ tabId, leafId, tier: 'recorded' }) + } + } + // Why: object key order is persistence order, so sort to keep the verdict reproducible. + const byTabId = (a: TerminalPtyPaneOwner, b: TerminalPtyPaneOwner): number => + a.tabId < b.tabId ? -1 : a.tabId > b.tabId ? 1 : 0 + return [...mounted.sort(byTabId), ...recorded.sort(byTabId)] +} + +/** + * Which pane owns a ptyId. The tab row's own `ptyId` is deliberately not a tier: the layout + * is the binding, and a row that disagrees with it is what hands two panes one PTY (STA-7961). + */ +export function resolveTerminalPtyPaneOwnership( + state: TerminalPtyPaneOwnerState, + ptyId: string, + options: TerminalPtyPaneOwnerOptions = {} +): TerminalPtyPaneOwnership { + const owners = listTerminalPtyPaneOwners(state, ptyId) + const mounted = owners.filter((owner) => owner.tier === 'mounted') + const deciding = mounted.length > 0 ? mounted : owners + if (deciding.length === 1) { + return { kind: 'owned', owner: deciding[0]! } + } + if (deciding.length > 1) { + // Why: stale duplicate ownership must not attach whichever hidden tab persisted order lists first. + const preferred = deciding.find((owner) => owner.tabId === options.preferTabId) + return preferred ? { kind: 'owned', owner: preferred } : { kind: 'ambiguous', owners: deciding } + } + // Why: nothing records the PTY yet, so the tab it was minted against is the only thing left + // that keeps paneKey hook attribution intact (#10486). + return options.preferTabId !== undefined + ? { kind: 'owned', owner: { tabId: options.preferTabId, leafId: null, tier: 'recorded' } } + : { kind: 'none' } +} diff --git a/src/renderer/src/lib/terminal-reveal-leaf-owner-lookup.test.ts b/src/renderer/src/lib/terminal-reveal-leaf-owner-lookup.test.ts new file mode 100644 index 00000000000..dbf9ae677f5 --- /dev/null +++ b/src/renderer/src/lib/terminal-reveal-leaf-owner-lookup.test.ts @@ -0,0 +1,65 @@ +// Which tab owns a leaf id decides whether a reveal adopts a pane or mints a second one +// (STA-7961). Two layouts can name the same leaf: one that still mounts it, and one left +// holding the id by a detach or by the hydration self-heal. +import { describe, expect, it } from 'vitest' +import { findTerminalTabIdBindingLeafId } from './terminal-reveal-tab-adoption' +import type { AppState } from '@/store/types' +import type { + TerminalLayoutSnapshot, + TerminalPaneLayoutNode +} from '../../../shared/terminal-tab-types' + +const SHARED_LEAF_ID = 'leaf-shared' + +function layout( + root: TerminalPaneLayoutNode | null, + ptyIdsByLeafId?: Record +): TerminalLayoutSnapshot { + return { + root, + activeLeafId: null, + expandedLeafId: null, + ...(ptyIdsByLeafId ? { ptyIdsByLeafId } : {}) + } +} + +function leaf(leafId: string): TerminalPaneLayoutNode { + return { type: 'leaf', leafId } +} + +function lookup(layoutsByTabId: AppState['terminalLayoutsByTabId']): string | null { + return findTerminalTabIdBindingLeafId({ terminalLayoutsByTabId: layoutsByTabId }, SHARED_LEAF_ID) +} + +describe('findTerminalTabIdBindingLeafId', () => { + it('skips a stranded binding whose own tree no longer holds the leaf', () => { + // The detached pane left tab-ghost's map entry behind; only tab-live can mount the leaf. + expect( + lookup({ + 'tab-ghost': layout(leaf('leaf-other'), { [SHARED_LEAF_ID]: 'pty-stale' }), + 'tab-live': layout(leaf(SHARED_LEAF_ID), { [SHARED_LEAF_ID]: 'pty-live' }) + }) + ).toBe('tab-live') + }) + + it('keeps answering a layout that records no tree at all', () => { + expect(lookup({ 'tab-a': layout(null, { [SHARED_LEAF_ID]: 'pty-a' }) })).toBe('tab-a') + }) + + it('prefers the tab that binds the leaf over one that only carries it unbound', () => { + // The hydration self-heal unbinds the losing single-leaf tab but leaves the id in its tree. + const bound = layout(leaf(SHARED_LEAF_ID), { [SHARED_LEAF_ID]: 'pty-a' }) + const unbound = layout(leaf(SHARED_LEAF_ID)) + + expect(lookup({ 'tab-bound': bound, 'tab-unbound': unbound })).toBe('tab-bound') + expect(lookup({ 'tab-unbound': unbound, 'tab-bound': bound })).toBe('tab-bound') + }) + + it('falls back to an unbound carrier when no tab binds the leaf', () => { + expect(lookup({ 'tab-unbound': layout(leaf(SHARED_LEAF_ID)) })).toBe('tab-unbound') + }) + + it('returns null when no layout names the leaf', () => { + expect(lookup({ 'tab-a': layout(leaf('leaf-other'), { 'leaf-other': 'pty-a' }) })).toBeNull() + }) +}) diff --git a/src/renderer/src/lib/terminal-reveal-tab-adoption.ts b/src/renderer/src/lib/terminal-reveal-tab-adoption.ts new file mode 100644 index 00000000000..411f6c686dd --- /dev/null +++ b/src/renderer/src/lib/terminal-reveal-tab-adoption.ts @@ -0,0 +1,139 @@ +import { collectLeafIdsInOrder } from '@/components/terminal-pane/terminal-layout-leaf-ids' +import type { AppState } from '@/store/types' +import type { TerminalTab } from '../../../shared/terminal-tab-types' +import { + resolveTerminalPtyPaneOwnership, + type TerminalPtyPaneOwnerState +} from './terminal-pty-pane-owner' + +export type TerminalRevealAdoptionState = TerminalPtyPaneOwnerState & + Pick + +export type TerminalRevealTabAdoption = + | { kind: 'adopt'; tabId: string; via: 'pty-owner' | 'bound-leaf' | 'ambiguity-tiebreak' } + | { kind: 'mint' } + +/** + * The tab whose layout owns a leaf id. Bound-and-in-tree beats in-tree-unbound, because the + * hydration self-heal leaves a losing single-leaf tab carrying its leaf with no session to adopt. + * Every layout is scanned, including ones whose row is gone: a leaf id is a pane identity for its + * lifetime, and re-minting one an orphan layout still holds is how the STA-7961 pair was created. + */ +export function findTerminalTabIdBindingLeafId( + state: Pick, + leafId: string +): string | null { + let unboundCarrierTabId: string | null = null + for (const [tabId, layout] of Object.entries(state.terminalLayoutsByTabId)) { + const carriesLeaf = layout.root ? collectLeafIdsInOrder(layout.root).includes(leafId) : null + if (layout.ptyIdsByLeafId?.[leafId] !== undefined && carriesLeaf !== false) { + return tabId + } + if (carriesLeaf && unboundCarrierTabId === null) { + unboundCarrierTabId = tabId + } + } + return unboundCarrierTabId +} + +/** Locate a tab row and the worktree key it is filed under, across every key. */ +export function findTerminalTabRow( + state: Pick, + tabId: string +): { tab: TerminalTab; worktreeId: string } | null { + for (const [worktreeId, tabs] of Object.entries(state.tabsByWorktree)) { + const tab = tabs.find((candidate) => candidate.id === tabId) + if (tab) { + return { tab, worktreeId } + } + } + return null +} + +/** + * Whether a reveal should adopt an existing tab or mint one. Adoption never rejects and never + * mints for a PTY or a leaf id some layout still holds — a second tab bound to a live PTY starves + * a pane, while a slightly wrong tab does not. + */ +export function resolveTerminalRevealTabAdoption( + state: TerminalRevealAdoptionState, + request: { ptyId: string; leafId?: string; hintTabId?: string } +): TerminalRevealTabAdoption { + // Why: a hint naming no row is a stale baked-in paneKey, not a binding. + const preferTabId = + request.hintTabId !== undefined && findTerminalTabRow(state, request.hintTabId) + ? { preferTabId: request.hintTabId } + : {} + const ownership = resolveTerminalPtyPaneOwnership(state, request.ptyId, preferTabId) + if (ownership.kind === 'owned') { + return { kind: 'adopt', tabId: ownership.owner.tabId, via: 'pty-owner' } + } + // STA-7961: the PTY is unowned here, but the leaf id may already be someone's pane. + const leafOwnerTabId = request.leafId + ? findTerminalTabIdBindingLeafId(state, request.leafId) + : null + if (leafOwnerTabId !== null) { + return { kind: 'adopt', tabId: leafOwnerTabId, via: 'bound-leaf' } + } + if (ownership.kind === 'ambiguous') { + console.warn( + `[terminal-reveal] ptyId ${request.ptyId} is claimed by ${ownership.owners + .map((owner) => `${owner.tabId}(${owner.tier})`) + .join(', ')}; adopting the first` + ) + return { kind: 'adopt', tabId: ownership.owners[0]!.tabId, via: 'ambiguity-tiebreak' } + } + console.warn( + `[terminal-reveal] no pane owns ptyId ${request.ptyId} (tabId hint ${request.hintTabId ?? 'none'}, leafId ${request.leafId ?? 'none'}); minting a tab` + ) + return { kind: 'mint' } +} + +export type TerminalRevealTargetRequest = { + worktreeId: string + ptyId?: string + tabId?: string + leafId?: string + splitFromLeafId?: string +} + +export type TerminalRevealTarget = { + /** The row to reuse: the PTY or leaf owner, else a split reveal's parent row. */ + tab: TerminalTab | undefined + /** The worktree key the reused row is filed under; the event's key when minting. */ + ownerWorktreeId: string +} + +/** + * The tab a reveal should land on, and the worktree key to surface it under. Ownership is + * tab-keyed, so the owning row can sit under a worktree key other than the event's — surfacing + * under the event's key then fails `verifyTerminalRevealIdentity` (STA-7961). + */ +export function resolveTerminalRevealTarget( + state: TerminalRevealAdoptionState, + request: TerminalRevealTargetRequest +): TerminalRevealTarget { + const adoption = request.ptyId + ? resolveTerminalRevealTabAdoption(state, { + ptyId: request.ptyId, + ...(request.leafId ? { leafId: request.leafId } : {}), + ...(request.tabId !== undefined ? { hintTabId: request.tabId } : {}) + }) + : ({ kind: 'mint' } as const) + const adoptedRow = adoption.kind === 'adopt' ? findTerminalTabRow(state, adoption.tabId) : null + if (adoption.kind === 'adopt' && !adoptedRow) { + // Why: minting instead would re-bind a leaf id the orphan layout still holds. + throw new Error(`terminal_reveal_owner_row_missing: tab ${adoption.tabId}`) + } + const isSplitReveal = Boolean( + request.ptyId && request.tabId && request.leafId && request.splitFromLeafId + ) + // Why: a split of a new PTY has no owner to adopt, and its parent row can sit under another key. + const splitTargetRow = + isSplitReveal && request.tabId !== undefined ? findTerminalTabRow(state, request.tabId) : null + if (isSplitReveal && !adoptedRow && !splitTargetRow) { + throw new Error(`Terminal tab ${request.tabId} not found`) + } + const ownerRow = adoptedRow ?? splitTargetRow + return { tab: ownerRow?.tab, ownerWorktreeId: ownerRow?.worktreeId ?? request.worktreeId } +} diff --git a/src/renderer/src/lib/terminal-tab-for-pty-id.test.ts b/src/renderer/src/lib/terminal-tab-for-pty-id.test.ts deleted file mode 100644 index ed3f651a2d4..00000000000 --- a/src/renderer/src/lib/terminal-tab-for-pty-id.test.ts +++ /dev/null @@ -1,208 +0,0 @@ -import { describe, expect, it } from 'vitest' -import { - resolveTerminalTabIdForPtyId, - resolveTerminalTabPtyOwnership, - type TerminalTabPtyOwnershipState -} from './terminal-tab-for-pty-id' -import type { AppState } from '@/store/types' - -function state(partial: { - tabs?: Record - layouts?: Record }> - livePtyIds?: Record -}): TerminalTabPtyOwnershipState { - return { - tabsByWorktree: (partial.tabs ?? {}) as unknown as AppState['tabsByWorktree'], - terminalLayoutsByTabId: (partial.layouts ?? - {}) as unknown as AppState['terminalLayoutsByTabId'], - ptyIdsByTabId: partial.livePtyIds ?? {} - } -} - -describe('resolveTerminalTabIdForPtyId', () => { - it('matches a tab by its own ptyId', () => { - const s = state({ - tabs: { - wt: [ - { id: 'tab-a', ptyId: 'wt@@1' }, - { id: 'tab-b', ptyId: 'wt@@2' } - ] - } - }) - expect(resolveTerminalTabIdForPtyId(s, 'wt', 'wt@@2')).toBe('tab-b') - }) - - it('matches a tab by a split leaf ptyId in its saved layout', () => { - const s = state({ - tabs: { wt: [{ id: 'tab-a', ptyId: null }] }, - layouts: { 'tab-a': { ptyIdsByLeafId: { leaf1: 'wt@@1', leaf2: 'wt@@9' } } } - }) - expect(resolveTerminalTabIdForPtyId(s, 'wt', 'wt@@9')).toBe('tab-a') - }) - - it('matches a tab by a live pty binding with no saved layout yet', () => { - const s = state({ - tabs: { wt: [{ id: 'tab-a', ptyId: null }] }, - livePtyIds: { 'tab-a': ['wt@@9'] } - }) - expect(resolveTerminalTabIdForPtyId(s, 'wt', 'wt@@9')).toBe('tab-a') - }) - - it('returns null when no tab owns the ptyId', () => { - const s = state({ tabs: { wt: [{ id: 'tab-a', ptyId: 'wt@@1' }] } }) - expect(resolveTerminalTabIdForPtyId(s, 'wt', 'wt@@nope')).toBeNull() - }) - - it('returns null when stale persistence binds the ptyId to multiple tabs', () => { - const s = state({ - tabs: { - wt: [ - { id: 'tab-a', ptyId: 'wt@@1' }, - { id: 'tab-b', ptyId: null } - ] - }, - layouts: { 'tab-b': { ptyIdsByLeafId: { leaf2: 'wt@@1' } } } - }) - expect(resolveTerminalTabIdForPtyId(s, 'wt', 'wt@@1')).toBeNull() - }) - - it('returns null for an unknown worktree', () => { - const s = state({ tabs: { wt: [{ id: 'tab-a', ptyId: 'wt@@1' }] } }) - expect(resolveTerminalTabIdForPtyId(s, 'other', 'wt@@1')).toBeNull() - }) -}) - -describe('resolveTerminalTabPtyOwnership', () => { - const staleLayoutBesideMountedTab = state({ - tabs: { - wt: [ - { id: 'tab-stale', ptyId: null }, - { id: 'tab-mounted', ptyId: null } - ] - }, - layouts: { 'tab-stale': { ptyIdsByLeafId: { leaf1: 'wt@@1' } } }, - livePtyIds: { 'tab-mounted': ['wt@@1'] } - }) - - it('prefers a mounted pane over a stale layout row in another tab', () => { - expect(resolveTerminalTabPtyOwnership(staleLayoutBesideMountedTab, 'wt', 'wt@@1')).toEqual({ - kind: 'owned', - tabId: 'tab-mounted' - }) - }) - - it('keeps the mounted pane even when the hint names the stale tab', () => { - expect( - resolveTerminalTabPtyOwnership(staleLayoutBesideMountedTab, 'wt', 'wt@@1', { - preferTabId: 'tab-stale' - }) - ).toEqual({ kind: 'owned', tabId: 'tab-mounted' }) - }) - - it('keeps a sole wake hint over a tab id the PTY outgrew', () => { - // Why: a pane dragged to another tab moves tab.ptyId with it, but the id - // baked into the PTY env is written once at spawn and never rewritten. - const s = state({ - tabs: { - wt: [ - { id: 'tab-minted-in', ptyId: null }, - { id: 'tab-detached-to', ptyId: 'wt@@1' } - ] - } - }) - expect( - resolveTerminalTabPtyOwnership(s, 'wt', 'wt@@1', { preferTabId: 'tab-minted-in' }) - ).toEqual({ kind: 'owned', tabId: 'tab-detached-to' }) - }) - - it('keeps a sole layout row over a tab id the PTY outgrew', () => { - const s = state({ - tabs: { - wt: [ - { id: 'tab-minted-in', ptyId: null }, - { id: 'tab-detached-to', ptyId: null } - ] - }, - layouts: { 'tab-detached-to': { ptyIdsByLeafId: { leaf1: 'wt@@1' } } } - }) - expect( - resolveTerminalTabPtyOwnership(s, 'wt', 'wt@@1', { preferTabId: 'tab-minted-in' }) - ).toEqual({ kind: 'owned', tabId: 'tab-detached-to' }) - }) - - it('falls back to the pre-minted tab id when nothing records the ptyId', () => { - const s = state({ - tabs: { - wt: [ - { id: 'tab-other', ptyId: 'wt@@2' }, - { id: 'tab-hinted', ptyId: null } - ] - } - }) - expect(resolveTerminalTabPtyOwnership(s, 'wt', 'wt@@1', { preferTabId: 'tab-hinted' })).toEqual( - { kind: 'owned', tabId: 'tab-hinted' } - ) - }) - - it('ignores a hint that names no existing tab', () => { - const s = state({ - tabs: { wt: [{ id: 'tab-a', ptyId: null }] }, - layouts: { 'tab-a': { ptyIdsByLeafId: { leaf1: 'wt@@1' } } } - }) - expect(resolveTerminalTabPtyOwnership(s, 'wt', 'wt@@1', { preferTabId: 'tab-gone' })).toEqual({ - kind: 'owned', - tabId: 'tab-a' - }) - }) - - it('reports ambiguity when only recorded bindings claim the ptyId', () => { - const s = state({ - tabs: { - wt: [ - { id: 'tab-a', ptyId: 'wt@@1' }, - { id: 'tab-b', ptyId: null } - ] - }, - layouts: { 'tab-b': { ptyIdsByLeafId: { leaf2: 'wt@@1' } } } - }) - expect(resolveTerminalTabPtyOwnership(s, 'wt', 'wt@@1')).toEqual({ kind: 'ambiguous' }) - }) - - it('breaks a recorded ownership conflict with the pre-minted tab id', () => { - const s = state({ - tabs: { - wt: [ - { id: 'tab-a', ptyId: 'wt@@1' }, - { id: 'tab-b', ptyId: null } - ] - }, - layouts: { 'tab-b': { ptyIdsByLeafId: { leaf2: 'wt@@1' } } } - }) - expect(resolveTerminalTabPtyOwnership(s, 'wt', 'wt@@1', { preferTabId: 'tab-b' })).toEqual({ - kind: 'owned', - tabId: 'tab-b' - }) - }) - - it('breaks a mounted ownership conflict with the pre-minted tab id', () => { - const s = state({ - tabs: { - wt: [ - { id: 'tab-a', ptyId: null }, - { id: 'tab-b', ptyId: null } - ] - }, - livePtyIds: { 'tab-a': ['wt@@1'], 'tab-b': ['wt@@1'] } - }) - expect(resolveTerminalTabPtyOwnership(s, 'wt', 'wt@@1')).toEqual({ kind: 'ambiguous' }) - expect(resolveTerminalTabPtyOwnership(s, 'wt', 'wt@@1', { preferTabId: 'tab-b' })).toEqual({ - kind: 'owned', - tabId: 'tab-b' - }) - }) - - it('reports no owner when nothing binds the ptyId', () => { - const s = state({ tabs: { wt: [{ id: 'tab-a', ptyId: 'wt@@2' }] } }) - expect(resolveTerminalTabPtyOwnership(s, 'wt', 'wt@@1')).toEqual({ kind: 'none' }) - }) -}) diff --git a/src/renderer/src/lib/terminal-tab-for-pty-id.ts b/src/renderer/src/lib/terminal-tab-for-pty-id.ts deleted file mode 100644 index 1c247e6b55e..00000000000 --- a/src/renderer/src/lib/terminal-tab-for-pty-id.ts +++ /dev/null @@ -1,74 +0,0 @@ -import type { AppState } from '@/store/types' - -export type TerminalTabPtyOwnershipState = Pick< - AppState, - 'tabsByWorktree' | 'terminalLayoutsByTabId' | 'ptyIdsByTabId' -> - -export type TerminalTabPtyOwnership = - | { kind: 'owned'; tabId: string } - | { kind: 'ambiguous' } - | { kind: 'none' } - -type TerminalTabPtyOwnershipOptions = { - /** Tab id baked into the PTY's env; a fallback and tie-break, not a binding. */ - preferTabId?: string -} - -/** - * Resolve which tab owns a ptyId. Every binding below is an exact match on the - * id, so any of them beats the caller's tab hint: that hint is written once when - * the PTY spawns and goes stale as soon as a pane moves between tabs. A mounted - * pane outranks a recorded one, and same-tier conflicts stay ambiguous. - */ -export function resolveTerminalTabPtyOwnership( - state: TerminalTabPtyOwnershipState, - worktreeId: string, - ptyId: string, - options: TerminalTabPtyOwnershipOptions = {} -): TerminalTabPtyOwnership { - const tabs = state.tabsByWorktree[worktreeId] ?? [] - const mountedOwners: string[] = [] - const recordedOwners: string[] = [] - for (const tab of tabs) { - if ((state.ptyIdsByTabId[tab.id] ?? []).includes(ptyId)) { - mountedOwners.push(tab.id) - continue - } - const ptyIdsByLeafId = state.terminalLayoutsByTabId[tab.id]?.ptyIdsByLeafId - if ( - tab.ptyId === ptyId || - (ptyIdsByLeafId !== undefined && Object.values(ptyIdsByLeafId).includes(ptyId)) - ) { - recordedOwners.push(tab.id) - } - } - const preferredTabId = - options.preferTabId !== undefined && tabs.some((tab) => tab.id === options.preferTabId) - ? options.preferTabId - : undefined - const owners = mountedOwners.length > 0 ? mountedOwners : recordedOwners - if (owners.length === 1) { - return { kind: 'owned', tabId: owners[0]! } - } - if (owners.length > 1) { - // Why: stale duplicate ownership must not attach whichever hidden tab - // happens to appear first in persisted order. - return preferredTabId !== undefined && owners.includes(preferredTabId) - ? { kind: 'owned', tabId: preferredTabId } - : { kind: 'ambiguous' } - } - // Why: nothing records the PTY yet, so the tab it was minted against is the - // only thing left that keeps paneKey hook attribution intact (#10486). - return preferredTabId !== undefined ? { kind: 'owned', tabId: preferredTabId } : { kind: 'none' } -} - -/** Resolve a synthetic mobile handle's ptyId; null when unowned or ambiguous. */ -export function resolveTerminalTabIdForPtyId( - state: TerminalTabPtyOwnershipState, - worktreeId: string, - ptyId: string -): string | null { - const ownership = resolveTerminalTabPtyOwnership(state, worktreeId, ptyId) - return ownership.kind === 'owned' ? ownership.tabId : null -} 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 3675b7575da..1fa7ba86806 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,7 @@ import { parsePaneKey } from '../../../shared/stable-pane-id' import { worktreeIdsEqual } from '../../../shared/worktree/id' import type { useAppStore } from '@/store' -import { resolveTerminalTabPtyOwnership } from './terminal-tab-for-pty-id' +import { resolveTerminalPtyPaneOwnership } from './terminal-pty-pane-owner' import type { LiveTerminalSurfaceOwner, LiveTerminalSurfaceOwnerIndex @@ -130,10 +130,11 @@ export async function adoptLiveWorkspacePtySurfaces( 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 tab.ptyId or the persisted layout used to read as unbound. + // 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) => resolveTerminalTabPtyOwnership(getState(), worktreeId, ptyId).kind === 'none' + (ptyId) => resolveTerminalPtyPaneOwnership(getState(), ptyId).kind === 'none' ) let surfaced = unbound.length < livePtyIds.length const declinedPtyIds: string[] = [] @@ -150,7 +151,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 (resolveTerminalTabPtyOwnership(getState(), worktreeId, ptyId).kind !== 'none') { + if (resolveTerminalPtyPaneOwnership(getState(), ptyId).kind !== 'none') { surfaced = true continue }