From bf33f56bd066891636ab35a5c80ce033f5d19a5d Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Tue, 28 Jul 2026 17:59:29 -0700 Subject: [PATCH] fix(mobile): stop a mobile reveal from minting a duplicate terminal tab Revealing a live PTY from a paired phone passes the tab id baked into its env, but the create-terminal handler only recognised ownership via tab.ptyId and the live ptyIdsByTabId map. With the worktree closed on the desktop no pane is mounted, so ptyIdsByTabId is empty and tab.ptyId holds at most one leaf's PTY -- a split pane's second PTY lives only in the persisted layout. Ownership missed, and createTab ran with an id that already existed; it mints a fresh uuid on collision, so a second tab appeared on the same session (#10486). Resolve ownership through the persisted layout as well. Every binding consulted is an exact match on the ptyId, so all of them outrank the reveal's tab id hint: that hint is written once when the PTY spawns and is never rewritten, so it goes stale as soon as a pane is dragged to another tab. A mounted pane outranks a recorded one, same-tier conflicts stay ambiguous so the mobile mount planner keeps failing closed, and the hint is what resolves a reveal that nothing records yet -- which is also what keeps paneKey hook attribution intact. --- .../src/hooks/ipc-events-test-harness.ts | 216 ++++++++++++++++ ...obile-terminal-reveal-tab-adoption.test.ts | 237 ++++++++++++++++++ src/renderer/src/hooks/useIpcEvents.ts | 33 ++- .../src/lib/mobile-terminal-tab-mount.test.ts | 24 +- .../src/lib/mobile-terminal-tab-mount.ts | 8 +- .../src/lib/terminal-tab-for-pty-id.test.ts | 158 +++++++++++- .../src/lib/terminal-tab-for-pty-id.ts | 87 +++++-- 7 files changed, 713 insertions(+), 50 deletions(-) create mode 100644 src/renderer/src/hooks/ipc-events-test-harness.ts create mode 100644 src/renderer/src/hooks/mobile-terminal-reveal-tab-adoption.test.ts diff --git a/src/renderer/src/hooks/ipc-events-test-harness.ts b/src/renderer/src/hooks/ipc-events-test-harness.ts new file mode 100644 index 00000000000..1ce81d5e404 --- /dev/null +++ b/src/renderer/src/hooks/ipc-events-test-harness.ts @@ -0,0 +1,216 @@ +import { vi } from 'vitest' +import type * as ReactModule from 'react' +import type { TerminalPaneLayoutNode } from '../../../shared/types' + +export type CreateTerminalRequest = { + requestId?: string + worktreeId: string + command?: string + title?: string + ptyId?: string + presentation?: 'background' | 'focused' + tabId?: string + leafId?: string + splitFromLeafId?: string +} + +export type HarnessTab = { id: string; ptyId?: string | null; title?: string } + +export type HarnessStoreState = { + tabsByWorktree: Record + ptyIdsByTabId: Record + terminalLayoutsByTabId: Record< + string, + { root?: TerminalPaneLayoutNode; ptyIdsByLeafId?: Record } + > + [key: string]: unknown +} + +/** Store surface useIpcEvents touches at mount plus the terminal-reveal path. */ +export function createHarnessStoreState( + overrides: Partial & Pick +): HarnessStoreState { + return { + createTab: vi.fn(() => ({ id: 'tab-minted' })), + setActiveView: vi.fn(), + setActiveWorktree: vi.fn(), + markWorktreeVisited: vi.fn(), + recordWorktreeVisit: vi.fn(), + isNavigatingHistory: false, + setActiveTabType: vi.fn(), + setActiveTab: vi.fn(), + revealWorktreeInSidebar: vi.fn(), + setTabCustomTitle: vi.fn(), + queueTabStartupCommand: vi.fn(), + registerAgentLaunchConfig: vi.fn(), + clearAgentLaunchConfig: vi.fn(), + updateTabPtyId: vi.fn(), + setTabLayout: vi.fn(), + setTabBarOrder: vi.fn(), + clearTabPtyId: vi.fn(), + setUpdateStatus: vi.fn(), + fetchRepos: vi.fn(), + fetchWorktrees: vi.fn(), + closeModal: vi.fn(), + openModal: vi.fn(), + setActiveRepo: vi.fn(), + setIsFullScreen: vi.fn(), + updateBrowserPageState: vi.fn(), + setEditorFontZoomLevel: vi.fn(), + setRateLimitsFromPush: vi.fn(), + setSshConnectionState: vi.fn(), + setSshTargetLabels: vi.fn(), + setPortForwards: vi.fn(), + clearPortForwards: vi.fn(), + setDetectedPorts: vi.fn(), + enqueueSshCredentialRequest: vi.fn(), + removeSshCredentialRequest: vi.fn(), + ptyIdsByTabId: {}, + terminalLayoutsByTabId: {}, + folderWorkspaces: [], + projectGroups: [], + repos: [{ id: 'repo-1', connectionId: null, executionHostId: 'local' }], + worktreesByRepo: { 'repo-1': [{ id: 'wt-1', repoId: 'repo-1' }] }, + openFiles: [], + browserTabsByWorktree: {}, + tabBarOrderByWorktree: {}, + activeModal: null, + activeWorktreeId: 'wt-1', + activeView: 'terminal', + activeTabType: 'terminal', + editorFontZoomLevel: 0, + settings: { + terminalFontSize: 13, + experimentalNativeChat: false, + openAgentTabsInChatByDefault: false, + activeRuntimeEnvironmentId: undefined + }, + ...overrides + } +} + +/** Subscription no-ops for every listener useIpcEvents attaches beyond the ones under test. */ +function createApiNamespaceStub(overrides: Record = {}): Record { + return new Proxy(overrides, { + get: (target, prop: string) => (prop in target ? target[prop] : () => () => {}) + }) +} + +export type IpcEventsHarness = { + /** Call inside the test body: useIpcEvents runs its effects eagerly here. */ + useIpcEvents: () => void + createTerminal: (request: CreateTerminalRequest) => void + replyTerminalCreate: ReturnType +} + +/** + * Loads useIpcEvents against a stubbed preload API and returns a driver for the + * create-terminal IPC, so reveal/adoption behavior is asserted through the hook. + */ +export async function loadIpcEventsHarness( + storeState: HarnessStoreState +): Promise { + const replyTerminalCreate = vi.fn() + let createTerminalListener: ((request: CreateTerminalRequest) => void) | null = null + + vi.resetModules() + vi.unstubAllGlobals() + + vi.doMock('react', async () => { + const actual = await vi.importActual('react') + return { ...actual, useEffect: (effect: () => void | (() => void)) => void effect() } + }) + vi.doMock('../store', () => ({ + useAppStore: { subscribe: vi.fn(() => () => {}), getState: () => storeState } + })) + vi.doMock('@/lib/ui-zoom', () => ({ applyUIZoom: vi.fn() })) + vi.doMock('@/lib/worktree-activation', () => ({ + activateAndRevealWorktree: vi.fn(), + ensureWorktreeHasInitialTerminal: vi.fn() + })) + vi.doMock('@/components/sidebar/visible-worktrees', () => ({ getVisibleWorktreeIds: () => [] })) + vi.doMock('@/lib/floating-workspace-terminal-actions', () => ({ + createFloatingWorkspaceTerminalTab: vi.fn(), + isEmptyFloatingWorkspacePanelVisible: () => false, + isFloatingWorkspacePanelFocused: () => false + })) + vi.doMock('@/runtime/web-runtime-session', () => ({ + activateWebRuntimeSessionTab: vi.fn(), + closeWebRuntimeSessionTab: vi.fn(), + createWebRuntimeSessionBrowserTab: vi.fn().mockResolvedValue(false), + createWebRuntimeSessionTerminal: vi.fn().mockResolvedValue({ status: 'failed', message: '' }), + 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('@/lib/activate-tab-and-focus-pane', () => ({ activateTabAndFocusPane: vi.fn() })) + + vi.stubGlobal('window', { + dispatchEvent: vi.fn(), + api: new Proxy( + { + ui: createApiNamespaceStub({ + getZoomLevel: () => 0, + consumePendingOpenSettings: () => Promise.resolve(false), + set: vi.fn(), + replyTabCreate: vi.fn(), + replyTabClose: vi.fn(), + replyTabSetProfile: vi.fn(), + replyTerminalCreate, + onCreateTerminal: (listener: (request: CreateTerminalRequest) => void) => { + createTerminalListener = listener + return () => {} + } + }), + rateLimits: { + get: () => Promise.resolve({ limits: {}, lastUpdatedAt: 0 }), + onUpdate: () => () => {} + }, + runtime: { + getTerminalFitOverrides: () => Promise.resolve([]), + getTerminalDrivers: () => Promise.resolve([]), + getBrowserDrivers: () => Promise.resolve([]), + onTerminalFitOverrideChanged: () => () => {}, + onTerminalDriverChanged: () => () => {}, + onBrowserDriverChanged: () => () => {} + }, + ssh: { + listTargets: () => Promise.resolve([]), + listPortForwards: () => Promise.resolve([]), + listDetectedPorts: () => Promise.resolve([]), + listRemovedTargetLabels: () => Promise.resolve([]), + getState: () => Promise.resolve(null), + onStateChanged: () => () => {}, + onCredentialRequest: () => () => {}, + onCredentialResolved: () => () => {}, + onPortForwardsChanged: () => () => {}, + onDetectedPortsChanged: () => () => {} + }, + updater: { + getStatus: () => Promise.resolve({ state: 'idle' }), + onStatus: () => () => {}, + onClearDismissal: () => () => {} + }, + mobile: createApiNamespaceStub({ + consumePendingUnpairedDeviceAuthFailure: () => Promise.resolve(false) + }), + remoteWorkspace: createApiNamespaceStub({ clientId: () => Promise.resolve(null) }) + } as Record, + { get: (target, prop: string) => target[prop] ?? createApiNamespaceStub() } + ) + }) + + const { useIpcEvents } = await import('./useIpcEvents') + return { + useIpcEvents, + createTerminal: (request) => { + if (typeof createTerminalListener !== 'function') { + throw new Error('Expected the create-terminal listener to be registered') + } + createTerminalListener(request) + }, + replyTerminalCreate + } +} 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 new file mode 100644 index 00000000000..30130211c62 --- /dev/null +++ b/src/renderer/src/hooks/mobile-terminal-reveal-tab-adoption.test.ts @@ -0,0 +1,237 @@ +import { describe, expect, it } from 'vitest' +import { + createHarnessStoreState, + loadIpcEventsHarness, + type HarnessStoreState +} from './ipc-events-test-harness' + +const WORKTREE_ID = 'wt-1' + +function revealSplitPaneFromMobile(harness: { + createTerminal: (request: { + requestId?: string + worktreeId: string + ptyId?: string + tabId?: string + leafId?: string + presentation?: 'background' | 'focused' + title?: string + }) => void +}): void { + harness.createTerminal({ + requestId: 'mobile-reveal', + worktreeId: WORKTREE_ID, + ptyId: 'pty-b', + tabId: 'tab-split', + leafId: 'leaf-b', + presentation: 'focused', + title: 'codex' + }) +} + +describe('mobile terminal reveal tab adoption', () => { + it('adopts the owning tab when a split pane is revealed before its panes mount (#10486)', async () => { + // Desktop has the worktree closed: no pane is mounted, so the live pty map is + // empty and only the persisted layout still records the second leaf's pty. + const storeState: HarnessStoreState = createHarnessStoreState({ + tabsByWorktree: { [WORKTREE_ID]: [{ id: 'tab-split', ptyId: 'pty-a', title: 'codex' }] }, + ptyIdsByTabId: {}, + terminalLayoutsByTabId: { + 'tab-split': { ptyIdsByLeafId: { 'leaf-a': 'pty-a', 'leaf-b': 'pty-b' } } + } + }) + const harness = await loadIpcEventsHarness(storeState) + harness.useIpcEvents() + + revealSplitPaneFromMobile(harness) + + expect(storeState.createTab).not.toHaveBeenCalled() + expect(harness.replyTerminalCreate).toHaveBeenCalledWith({ + requestId: 'mobile-reveal', + tabId: 'tab-split', + title: 'codex' + }) + }) + + it('adopts the owning tab when neither the live map nor the layout is hydrated', async () => { + // Same reveal, but layout hydration has not landed either; the pre-minted + // tabId hint is the PTY's baked-in pane key and must still be honoured. + const storeState: HarnessStoreState = createHarnessStoreState({ + tabsByWorktree: { [WORKTREE_ID]: [{ id: 'tab-split', ptyId: 'pty-a', title: 'codex' }] }, + ptyIdsByTabId: { 'tab-split': ['pty-a'] }, + terminalLayoutsByTabId: {} + }) + const harness = await loadIpcEventsHarness(storeState) + harness.useIpcEvents() + + revealSplitPaneFromMobile(harness) + + expect(storeState.createTab).not.toHaveBeenCalled() + expect(harness.replyTerminalCreate).toHaveBeenCalledWith({ + requestId: 'mobile-reveal', + tabId: 'tab-split', + title: 'codex' + }) + }) + + it('adopts the tab that records the pty when the reveal hint disagrees', async () => { + // The pane was dragged out of tab-split since the PTY's env was baked, so + // the reveal still names tab-split while tab-detached is where it now lives. + const storeState: HarnessStoreState = createHarnessStoreState({ + tabsByWorktree: { + [WORKTREE_ID]: [ + { id: 'tab-detached', ptyId: null, title: 'Terminal 1' }, + { id: 'tab-split', ptyId: 'pty-a', title: 'codex' } + ] + }, + ptyIdsByTabId: { 'tab-split': ['pty-a'] }, + terminalLayoutsByTabId: { + 'tab-detached': { ptyIdsByLeafId: { 'leaf-b': 'pty-b' } }, + 'tab-split': { ptyIdsByLeafId: { 'leaf-a': 'pty-a' } } + } + }) + const harness = await loadIpcEventsHarness(storeState) + harness.useIpcEvents() + + revealSplitPaneFromMobile(harness) + + expect(storeState.createTab).not.toHaveBeenCalled() + expect(harness.replyTerminalCreate).toHaveBeenCalledWith({ + requestId: 'mobile-reveal', + tabId: 'tab-detached', + title: 'codex' + }) + }) + + it('adopts through the persisted layout when the PTY carries no tab id', async () => { + const storeState: HarnessStoreState = createHarnessStoreState({ + tabsByWorktree: { [WORKTREE_ID]: [{ id: 'tab-split', ptyId: 'pty-a', title: 'codex' }] }, + ptyIdsByTabId: {}, + terminalLayoutsByTabId: { + 'tab-split': { ptyIdsByLeafId: { 'leaf-a': 'pty-a', 'leaf-b': 'pty-b' } } + } + }) + const harness = await loadIpcEventsHarness(storeState) + harness.useIpcEvents() + + harness.createTerminal({ + requestId: 'mobile-reveal', + worktreeId: WORKTREE_ID, + ptyId: 'pty-b', + leafId: 'leaf-b', + presentation: 'focused', + title: 'codex' + }) + + expect(storeState.createTab).not.toHaveBeenCalled() + expect(harness.replyTerminalCreate).toHaveBeenCalledWith({ + requestId: 'mobile-reveal', + tabId: 'tab-split', + title: 'codex' + }) + }) + + it('keeps a live binding authoritative over a stale layout row', async () => { + // No tabId hint: the live pty map still pins the session to its real tab. + const storeState: HarnessStoreState = createHarnessStoreState({ + tabsByWorktree: { + [WORKTREE_ID]: [ + { id: 'tab-stale', ptyId: null, title: 'Terminal 1' }, + { id: 'tab-live', ptyId: null, title: 'codex' } + ] + }, + ptyIdsByTabId: { 'tab-live': ['pty-b'] }, + terminalLayoutsByTabId: { 'tab-stale': { ptyIdsByLeafId: { 'leaf-x': 'pty-b' } } } + }) + const harness = await loadIpcEventsHarness(storeState) + harness.useIpcEvents() + + harness.createTerminal({ + requestId: 'mobile-reveal', + worktreeId: WORKTREE_ID, + ptyId: 'pty-b', + leafId: 'leaf-b', + presentation: 'focused', + title: 'codex' + }) + + expect(storeState.createTab).not.toHaveBeenCalled() + expect(harness.replyTerminalCreate).toHaveBeenCalledWith({ + requestId: 'mobile-reveal', + tabId: 'tab-live', + title: 'codex' + }) + }) + + it('leaves the hinted tab split intact when the pty lives in another tab', async () => { + // Adopting the hinted tab here would rewrite its layout with a single-pane + // snapshot, silently collapsing the user's split onto the revealed pty. + const storeState: HarnessStoreState = createHarnessStoreState({ + tabsByWorktree: { + [WORKTREE_ID]: [ + { id: 'tab-split', ptyId: null, title: 'codex' }, + { id: 'tab-detached', ptyId: null, title: 'Terminal 2' } + ] + }, + ptyIdsByTabId: {}, + terminalLayoutsByTabId: { + 'tab-split': { + root: { + type: 'split', + direction: 'horizontal', + first: { type: 'leaf', leafId: 'leaf-1' }, + second: { type: 'leaf', leafId: 'leaf-2' } + }, + ptyIdsByLeafId: { 'leaf-1': 'pty-1', 'leaf-2': 'pty-2' } + }, + 'tab-detached': { ptyIdsByLeafId: { 'leaf-b': 'pty-b' } } + } + }) + + const harness = await loadIpcEventsHarness(storeState) + harness.useIpcEvents() + + revealSplitPaneFromMobile(harness) + + expect(storeState.createTab).not.toHaveBeenCalled() + const layoutWrites = (storeState.setTabLayout as { mock: { calls: unknown[][] } }).mock.calls + expect(layoutWrites.map((call) => call[0])).not.toContain('tab-split') + expect(harness.replyTerminalCreate).toHaveBeenCalledWith({ + requestId: 'mobile-reveal', + tabId: 'tab-detached', + title: 'codex' + }) + }) + + 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. + const storeState: HarnessStoreState = createHarnessStoreState({ + tabsByWorktree: { + [WORKTREE_ID]: [ + { id: 'tab-stale-a', ptyId: 'pty-b', title: 'Terminal 1' }, + { id: 'tab-stale-b', ptyId: null, title: 'Terminal 2' } + ] + }, + ptyIdsByTabId: {}, + terminalLayoutsByTabId: { 'tab-stale-b': { ptyIdsByLeafId: { 'leaf-y': 'pty-b' } } } + }) + const harness = await loadIpcEventsHarness(storeState) + harness.useIpcEvents() + + harness.createTerminal({ + requestId: 'mobile-reveal', + worktreeId: WORKTREE_ID, + ptyId: 'pty-b', + leafId: 'leaf-b', + presentation: 'focused', + title: 'codex' + }) + + expect(harness.replyTerminalCreate).toHaveBeenCalledWith({ + requestId: 'mobile-reveal', + tabId: 'tab-minted', + title: 'codex' + }) + }) +}) diff --git a/src/renderer/src/hooks/useIpcEvents.ts b/src/renderer/src/hooks/useIpcEvents.ts index 68a94915300..65119eaa24e 100644 --- a/src/renderer/src/hooks/useIpcEvents.ts +++ b/src/renderer/src/hooks/useIpcEvents.ts @@ -15,6 +15,7 @@ import { OPEN_WORKSPACE_BOARD_EVENT } from '@/components/sidebar/useWorkspaceBoa import { SPLIT_TERMINAL_PANE_EVENT, CLOSE_TERMINAL_PANE_EVENT } from '@/constants/terminal' import { requestBackgroundTerminalWorktreeMount } from '@/components/terminal/background-terminal-worktree-mount' import { planMobileTerminalTabMount } from '@/lib/mobile-terminal-tab-mount' +import { resolveTerminalTabPtyOwnership } from '@/lib/terminal-tab-for-pty-id' import { hasRegisteredRuntimeTerminalTab, focusRuntimeTerminalSurface @@ -1411,13 +1412,20 @@ export function useIpcEvents(): void { activateTerminalInitiatedWorktree(store, worktreeId) } const worktreeTabs = store.tabsByWorktree[worktreeId] ?? [] - const existingTab = ptyId - ? worktreeTabs.find( - (candidate) => - candidate.ptyId === ptyId || - (store.ptyIdsByTabId[candidate.id] ?? []).includes(ptyId) + // 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 } : {} ) - : undefined + : { 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) @@ -1425,18 +1433,7 @@ export function useIpcEvents(): void { if (isSplitReveal && !splitTargetTab) { throw new Error(`Terminal tab ${tabId} not found`) } - const hintedPendingTab = - ptyId && tabId && !isSplitReveal - ? worktreeTabs.find((candidate) => { - if (candidate.id !== tabId) { - return false - } - const candidatePtyIds = store.ptyIdsByTabId[candidate.id] ?? [] - return candidate.ptyId == null && candidatePtyIds.length === 0 - }) - : undefined - // Why: runtime fallback reveals a PTY for a renderer-created pending tab; adopt only when the hinted tab has no PTY yet. - const reusedTab = existingTab ?? splitTargetTab ?? hintedPendingTab + const reusedTab = existingTab ?? splitTargetTab const tab = reusedTab ?? (ptyId 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 6529ebca17f..35bef7fec53 100644 --- a/src/renderer/src/lib/mobile-terminal-tab-mount.test.ts +++ b/src/renderer/src/lib/mobile-terminal-tab-mount.test.ts @@ -1,10 +1,9 @@ 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' -type PlannerState = Pick - -function state(tabCount = 1): PlannerState { +function state(tabCount = 1): TerminalTabPtyOwnershipState { return { tabsByWorktree: { wt: Array.from({ length: tabCount }, (_, index) => ({ @@ -12,7 +11,8 @@ function state(tabCount = 1): PlannerState { ptyId: `wt@@${index}` })) } as unknown as AppState['tabsByWorktree'], - terminalLayoutsByTabId: {} + terminalLayoutsByTabId: {}, + ptyIdsByTabId: {} } } @@ -49,6 +49,22 @@ describe('planMobileTerminalTabMount', () => { expect(planMobileTerminalTabMount(s, { worktreeId: 'wt', ptyId: 'wt@@173' })).toBeNull() }) + it('mounts the tab whose pane is mounted when a stale layout row also claims the pty', () => { + const s = state(200) + s.ptyIdsByTabId['tab-173'] = ['wt@@173'] + s.terminalLayoutsByTabId['tab-199'] = { + root: null, + activeLeafId: null, + expandedLeafId: null, + ptyIdsByLeafId: { leaf: 'wt@@173' } + } + + expect(planMobileTerminalTabMount(s, { worktreeId: 'wt', ptyId: 'wt@@173' })).toEqual({ + worktreeId: 'wt', + tabIds: ['tab-173'] + }) + }) + 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 28431cc5ad6..e4e034db28b 100644 --- a/src/renderer/src/lib/mobile-terminal-tab-mount.ts +++ b/src/renderer/src/lib/mobile-terminal-tab-mount.ts @@ -1,6 +1,8 @@ -import type { AppState } from '@/store/types' import type { BackgroundMountTerminalWorktreeDetail } from '@/constants/terminal' -import { resolveTerminalTabIdForPtyId } from './terminal-tab-for-pty-id' +import { + resolveTerminalTabIdForPtyId, + type TerminalTabPtyOwnershipState +} from './terminal-tab-for-pty-id' export type MobileTerminalTabMountRequest = { worktreeId: string @@ -14,7 +16,7 @@ type MobileTerminalTabMountOptions = { /** Why: exact-tab planning prevents a stale ptyId from mounting every saved xterm (#8597). */ export function planMobileTerminalTabMount( - state: Pick, + state: TerminalTabPtyOwnershipState, request: MobileTerminalTabMountRequest, options: MobileTerminalTabMountOptions = {} ): BackgroundMountTerminalWorktreeDetail | null { 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 index 4fdc698e79e..ed3f651a2d4 100644 --- a/src/renderer/src/lib/terminal-tab-for-pty-id.test.ts +++ b/src/renderer/src/lib/terminal-tab-for-pty-id.test.ts @@ -1,16 +1,21 @@ import { describe, expect, it } from 'vitest' -import { resolveTerminalTabIdForPtyId } from './terminal-tab-for-pty-id' +import { + resolveTerminalTabIdForPtyId, + resolveTerminalTabPtyOwnership, + type TerminalTabPtyOwnershipState +} from './terminal-tab-for-pty-id' import type { AppState } from '@/store/types' -type ResolverState = Pick - function state(partial: { tabs?: Record layouts?: Record }> -}): ResolverState { + livePtyIds?: Record +}): TerminalTabPtyOwnershipState { return { tabsByWorktree: (partial.tabs ?? {}) as unknown as AppState['tabsByWorktree'], - terminalLayoutsByTabId: (partial.layouts ?? {}) as unknown as AppState['terminalLayoutsByTabId'] + terminalLayoutsByTabId: (partial.layouts ?? + {}) as unknown as AppState['terminalLayoutsByTabId'], + ptyIdsByTabId: partial.livePtyIds ?? {} } } @@ -35,6 +40,14 @@ describe('resolveTerminalTabIdForPtyId', () => { 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() @@ -58,3 +71,138 @@ describe('resolveTerminalTabIdForPtyId', () => { 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 index 77ee3c29706..1c247e6b55e 100644 --- a/src/renderer/src/lib/terminal-tab-for-pty-id.ts +++ b/src/renderer/src/lib/terminal-tab-for-pty-id.ts @@ -1,27 +1,74 @@ import type { AppState } from '@/store/types' -/** Resolve a synthetic mobile handle's ptyId through persisted tab and split bindings. */ +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: Pick, + state: TerminalTabPtyOwnershipState, worktreeId: string, ptyId: string ): string | null { - const tabs = state.tabsByWorktree[worktreeId] ?? [] - let resolvedTabId: string | null = null - for (const tab of tabs) { - const ptyIdsByLeafId = state.terminalLayoutsByTabId[tab.id]?.ptyIdsByLeafId - const ownsPty = - tab.ptyId === ptyId || - (ptyIdsByLeafId !== undefined && Object.values(ptyIdsByLeafId).includes(ptyId)) - if (!ownsPty) { - continue - } - if (resolvedTabId && resolvedTabId !== tab.id) { - // Why: stale duplicate ownership must not attach whichever hidden tab - // happens to appear first in persisted order. - return null - } - resolvedTabId = tab.id - } - return resolvedTabId + const ownership = resolveTerminalTabPtyOwnership(state, worktreeId, ptyId) + return ownership.kind === 'owned' ? ownership.tabId : null }