From 6eb77f7f8967b8cd1af39ee07f11b9e2baea8f8a Mon Sep 17 00:00:00 2001 From: brennanb2025 Date: Sun, 31 May 2026 21:12:28 -0700 Subject: [PATCH] Fix agent popover stale pane races Co-authored-by: Orca --- .../sidebar/WorktreeCardAgents.test.tsx | 55 ++++++++++++++++++- .../components/sidebar/WorktreeCardAgents.tsx | 25 ++++++++- .../components/terminal-pane/TerminalPane.tsx | 24 +++++--- .../terminal-pane/pty-connection-types.ts | 1 + .../terminal-pane/pty-connection.test.ts | 51 +++++++++++++++++ .../terminal-pane/pty-connection.ts | 14 +++++ .../components/terminal-pane/pty-transport.ts | 3 + .../remote-runtime-pty-transport.ts | 6 ++ .../use-terminal-pane-lifecycle.ts | 3 + 9 files changed, 172 insertions(+), 10 deletions(-) diff --git a/src/renderer/src/components/sidebar/WorktreeCardAgents.test.tsx b/src/renderer/src/components/sidebar/WorktreeCardAgents.test.tsx index 00c0a993b30..9e0c22e2013 100644 --- a/src/renderer/src/components/sidebar/WorktreeCardAgents.test.tsx +++ b/src/renderer/src/components/sidebar/WorktreeCardAgents.test.tsx @@ -57,6 +57,15 @@ let mockFocusedAgentPaneKey: string | null = null let mockAgentActivityDisplayMode: 'compact' | 'full' | undefined let mockExperimentalAgentTerminalPopover = false let mockLiveTabIds = ['tab-1'] +let mockTerminalLayoutsByTabId: Record = {} + +function mockLayoutForLeaf(leafId: string): unknown { + return { + root: { type: 'leaf', leafId }, + activeLeafId: leafId, + expandedLeafId: null + } +} vi.mock('@/store', () => ({ useAppStore: (selector: (state: unknown) => unknown) => @@ -70,7 +79,7 @@ vi.mock('@/store', () => ({ agentSendPopoverTargetMode: null, agentStatusByPaneKey: {}, tabsByWorktree: { 'wt-1': mockLiveTabIds.map((id) => ({ id })) }, - terminalLayoutsByTabId: {}, + terminalLayoutsByTabId: mockTerminalLayoutsByTabId, sendPromptToSidebarAgentTarget: vi.fn() }) })) @@ -158,6 +167,7 @@ describe('WorktreeCardAgents', () => { mockAgentActivityDisplayMode = undefined mockExperimentalAgentTerminalPopover = false mockLiveTabIds = ['tab-1'] + mockTerminalLayoutsByTabId = {} }) it('renders ordinary rows in full mode without a child disclosure', async () => { @@ -287,6 +297,9 @@ describe('WorktreeCardAgents', () => { mockAgentActivityDisplayMode = 'full' mockExperimentalAgentTerminalPopover = true mockLiveTabIds = ['tab-1', 'tab-other', 'legacy'] + mockTerminalLayoutsByTabId = { + 'tab-1': mockLayoutForLeaf(leafId) + } mockAgents = [ mockAgent({ paneKey: `tab-1:${leafId}`, tabId: 'tab-1' }), mockAgent({ paneKey: 'tab-2:1', tabId: 'tab-other' }), @@ -303,10 +316,38 @@ describe('WorktreeCardAgents', () => { expect(markup).toContain('data-pane-key="tab-stale:1" data-popover="false"') }) + it('does not wire terminal popovers for missing split leaves in a live tab', async () => { + const liveLeafId = '55555555-5555-4555-8555-555555555555' + const removedLeafId = '66666666-6666-4666-8666-666666666666' + mockAgentActivityDisplayMode = 'full' + mockExperimentalAgentTerminalPopover = true + mockLiveTabIds = ['tab-1'] + mockTerminalLayoutsByTabId = { + 'tab-1': { + root: { type: 'leaf', leafId: liveLeafId }, + activeLeafId: liveLeafId, + expandedLeafId: null + } + } + mockAgents = [ + mockAgent({ paneKey: `tab-1:${liveLeafId}`, tabId: 'tab-1' }), + mockAgent({ paneKey: `tab-1:${removedLeafId}`, tabId: 'tab-1', state: 'done' }) + ] + const { default: WorktreeCardAgents } = await import('./WorktreeCardAgents') + + const markup = renderToStaticMarkup() + + expect(markup).toContain(`data-pane-key="tab-1:${liveLeafId}" data-popover="true"`) + expect(markup).toContain(`data-pane-key="tab-1:${removedLeafId}" data-popover="false"`) + }) + it('wires terminal popovers to compact agent rows when compact mode is active', async () => { const leafId = '55555555-5555-4555-8555-555555555555' mockAgentActivityDisplayMode = 'compact' mockExperimentalAgentTerminalPopover = true + mockTerminalLayoutsByTabId = { + 'tab-1': mockLayoutForLeaf(leafId) + } mockAgents = [ mockAgent({ paneKey: `tab-1:${leafId}`, @@ -327,6 +368,18 @@ describe('WorktreeCardAgents', () => { it('keeps compact rows visible instead of an aggregate summary when terminal popovers are enabled', async () => { mockAgentActivityDisplayMode = 'compact' mockExperimentalAgentTerminalPopover = true + mockTerminalLayoutsByTabId = { + 'tab-1': { + root: { + type: 'split', + direction: 'horizontal', + first: { type: 'leaf', leafId: '66666666-6666-4666-8666-666666666666' }, + second: { type: 'leaf', leafId: '77777777-7777-4777-8777-777777777777' } + }, + activeLeafId: '66666666-6666-4666-8666-666666666666', + expandedLeafId: null + } + } mockAgents = [ mockAgent({ paneKey: 'tab-1:66666666-6666-4666-8666-666666666666', diff --git a/src/renderer/src/components/sidebar/WorktreeCardAgents.tsx b/src/renderer/src/components/sidebar/WorktreeCardAgents.tsx index 155a0d99388..7b429c13e2d 100644 --- a/src/renderer/src/components/sidebar/WorktreeCardAgents.tsx +++ b/src/renderer/src/components/sidebar/WorktreeCardAgents.tsx @@ -21,6 +21,7 @@ import { } from './worktree-card-compact-agents' import { DEFAULT_AGENT_ACTIVITY_DISPLAY_MODE } from '../../../../shared/constants' import { revealElementInScrollContainer } from './worktree-sidebar-reveal' +import type { TerminalPaneLayoutNode } from '../../../../shared/types' export const SUPPRESS_WORKTREE_LIST_SCROLL_ADJUSTMENT_EVENT = 'orca-suppress-worktree-list-scroll-adjustment' @@ -38,6 +39,19 @@ function revealCompactAgentCard(agentListRoot: HTMLElement | null): void { revealElementInScrollContainer(sidebarElement, worktreeOptionElement, 'auto') } +function layoutContainsLeaf( + node: TerminalPaneLayoutNode | null | undefined, + leafId: string +): boolean { + if (!node) { + return false + } + if (node.type === 'leaf') { + return node.leafId === leafId + } + return layoutContainsLeaf(node.first, leafId) || layoutContainsLeaf(node.second, leafId) +} + // Why: the terminal popover is experimental and default-off. Keep its Radix / // portal dependencies off the normal inline-agent render path until enabled. const LazyAgentTerminalPopover = React.lazy(async () => { @@ -347,11 +361,18 @@ const WorktreeCardAgentsBody = React.memo(function WorktreeCardAgentsBody({ return agentTerminalPopoverEnabled && !isAgentSendTargetModeActive && liveTerminalTabIdSet.has(agent.tab.id) && - parsedPaneKey?.tabId === agent.tab.id + parsedPaneKey?.tabId === agent.tab.id && + layoutContainsLeaf(terminalLayoutsByTabId[agent.tab.id]?.root, parsedPaneKey.leafId) ? `agent-popover:${worktreeId}:${agent.tab.id}:${agent.paneKey}` : null }, - [agentTerminalPopoverEnabled, isAgentSendTargetModeActive, liveTerminalTabIdSet, worktreeId] + [ + agentTerminalPopoverEnabled, + isAgentSendTargetModeActive, + liveTerminalTabIdSet, + terminalLayoutsByTabId, + worktreeId + ] ) const renderTerminalPopoverForAgent = useCallback( diff --git a/src/renderer/src/components/terminal-pane/TerminalPane.tsx b/src/renderer/src/components/terminal-pane/TerminalPane.tsx index 5b3aa2c5d0c..c45cfc84bdf 100644 --- a/src/renderer/src/components/terminal-pane/TerminalPane.tsx +++ b/src/renderer/src/components/terminal-pane/TerminalPane.tsx @@ -614,16 +614,12 @@ export default function TerminalPane({ persistLayoutSnapshot() }, [paneCount, paneTitles, persistLayoutSnapshot, terminalTab]) - const syncPanePtyLayoutBinding = useCallback( - (paneId: number, ptyId: string | null): void => { + const syncPanePtyLayoutBindingByLeafId = useCallback( + (leafId: string, ptyId: string | null): void => { const existingLayout = useAppStore.getState().terminalLayoutsByTabId[tabId] ?? EMPTY_LAYOUT const { ptyIdsByLeafId: _existingPtyIdsByLeafId, ...layoutWithoutPtyBindings } = existingLayout const existingBindings = existingLayout.ptyIdsByLeafId ?? {} - const leafId = managerRef.current?.getLeafId(paneId) - if (!leafId) { - return - } if (ptyId) { setTabLayout(tabId, { @@ -650,6 +646,17 @@ export default function TerminalPane({ [setTabLayout, tabId] ) + const syncPanePtyLayoutBinding = useCallback( + (paneId: number, ptyId: string | null): void => { + const leafId = managerRef.current?.getLeafId(paneId) + if (!leafId) { + return + } + syncPanePtyLayoutBindingByLeafId(leafId, ptyId) + }, + [managerRef, syncPanePtyLayoutBindingByLeafId] + ) + const { setExpandedPane, restoreExpandedLayout, @@ -787,6 +794,7 @@ export default function TerminalPane({ dispatchNotification, setCacheTimerStartedAt, syncPanePtyLayoutBinding, + syncPanePtyLayoutBindingByLeafId, setTabPaneExpanded, setTabCanExpandPane, setExpandedPane, @@ -992,7 +1000,8 @@ export default function TerminalPane({ clearTerminalPaneUnread, dispatchNotification, setCacheTimerStartedAt, - syncPanePtyLayoutBinding + syncPanePtyLayoutBinding, + syncPanePtyLayoutBindingByLeafId }) panePtyBindingsRef.current.set(paneId, newPaneBinding) manager.setActivePane(paneId, { focus: true }) @@ -1014,6 +1023,7 @@ export default function TerminalPane({ setRuntimePaneTitle, suppressPtyExit, syncPanePtyLayoutBinding, + syncPanePtyLayoutBindingByLeafId, tabId, updateTabPtyId, updateTabTitle, diff --git a/src/renderer/src/components/terminal-pane/pty-connection-types.ts b/src/renderer/src/components/terminal-pane/pty-connection-types.ts index d7703f8d382..966085e479c 100644 --- a/src/renderer/src/components/terminal-pane/pty-connection-types.ts +++ b/src/renderer/src/components/terminal-pane/pty-connection-types.ts @@ -50,4 +50,5 @@ export type PtyConnectionDeps = { }) => void setCacheTimerStartedAt: (key: string, ts: number | null) => void syncPanePtyLayoutBinding: (paneId: number, ptyId: string | null) => void + syncPanePtyLayoutBindingByLeafId: (leafId: string, ptyId: string | null) => void } diff --git a/src/renderer/src/components/terminal-pane/pty-connection.test.ts b/src/renderer/src/components/terminal-pane/pty-connection.test.ts index c2c2a767537..8903511532d 100644 --- a/src/renderer/src/components/terminal-pane/pty-connection.test.ts +++ b/src/renderer/src/components/terminal-pane/pty-connection.test.ts @@ -100,6 +100,7 @@ type MockTransport = { sendInputAccepted?: ReturnType resize: ReturnType getPtyId: ReturnType + destroy: ReturnType } const scheduleRuntimeGraphSync = vi.fn() @@ -215,6 +216,9 @@ function createMockTransport(initialPtyId: string | null = null): MockTransport } return ptyId }), + destroy: vi.fn(() => { + ptyId = null + }), sendInput: vi.fn(() => true), resize: vi.fn(() => true), getPtyId: vi.fn(() => ptyId) @@ -333,6 +337,7 @@ function createDeps(overrides: Record = {}) { dispatchNotification: vi.fn(), setCacheTimerStartedAt: vi.fn(), syncPanePtyLayoutBinding: vi.fn(), + syncPanePtyLayoutBindingByLeafId: vi.fn(), ...overrides } } @@ -699,6 +704,52 @@ describe('connectPanePty', () => { ) }) + it('persists fresh spawned PTYs by stable leaf id', async () => { + const { connectPanePty } = await import('./pty-connection') + const transport = createMockTransport('pty-fresh') + transportFactoryQueue.push(transport) + const pane = createPane(2) + const manager = createManager(2) + const deps = createDeps() + + connectPanePty(pane as never, manager as never, deps as never) + await flushAsyncTicks() + const onPtySpawn = createdTransportOptions[0]?.onPtySpawn as + | ((ptyId: string) => void) + | undefined + expect(onPtySpawn).toBeTypeOf('function') + onPtySpawn?.('pty-fresh') + + expect(deps.syncPanePtyLayoutBinding).toHaveBeenCalledWith(2, 'pty-fresh') + expect(deps.syncPanePtyLayoutBindingByLeafId).toHaveBeenCalledWith(LEAF_2, 'pty-fresh') + }) + + it('tears down fresh spawned PTYs when the tab disappeared before spawn resolution', async () => { + const { connectPanePty } = await import('./pty-connection') + const transport = createMockTransport('pty-late') + transportFactoryQueue.push(transport) + mockStoreState = { + ...mockStoreState, + tabsByWorktree: { 'wt-1': [] } + } + const pane = createPane(1) + const manager = createManager(1) + const deps = createDeps() + + connectPanePty(pane as never, manager as never, deps as never) + await flushAsyncTicks() + const onPtySpawn = createdTransportOptions[0]?.onPtySpawn as + | ((ptyId: string) => void) + | undefined + expect(onPtySpawn).toBeTypeOf('function') + onPtySpawn?.('pty-late') + + expect(transport.destroy).toHaveBeenCalled() + expect(deps.syncPanePtyLayoutBinding).not.toHaveBeenCalled() + expect(deps.syncPanePtyLayoutBindingByLeafId).not.toHaveBeenCalled() + expect(deps.updateTabPtyId).not.toHaveBeenCalled() + }) + it('seeds a working status from Command Code thinking output without a startup prompt', async () => { const { connectPanePty } = await import('./pty-connection') const capturedDataCallback: { current: ((data: string) => void) | null } = { current: null } diff --git a/src/renderer/src/components/terminal-pane/pty-connection.ts b/src/renderer/src/components/terminal-pane/pty-connection.ts index 2e6b235b326..6940ad7e29c 100644 --- a/src/renderer/src/components/terminal-pane/pty-connection.ts +++ b/src/renderer/src/components/terminal-pane/pty-connection.ts @@ -850,8 +850,21 @@ export function connectPanePty( const observeTerminalGitHubPRLink = createTerminalGitHubPRLinkDetector() const onPtySpawn = (ptyId: string): void => { + const tabStillExists = (useAppStore.getState().tabsByWorktree[deps.worktreeId] ?? []).some( + (candidate) => candidate.id === deps.tabId + ) + if (!tabStillExists) { + // Why: a detached pending spawn can resolve after its tab was closed. + // Tear down the fresh PTY before it can resurrect stale tab/layout state. + transport.destroy?.() + return + } setPanePtyFitBinding(ptyId) deps.syncPanePtyLayoutBinding(pane.id, ptyId) + // Why: detached pending spawns can resolve after their old PaneManager is + // gone. Persist by stable leaf id too, so the remount can reattach the + // spawned PTY to the original split leaf instead of falling back to tab PTY. + deps.syncPanePtyLayoutBindingByLeafId(pane.leafId, ptyId) deps.updateTabPtyId(deps.tabId, ptyId) // Why: Command Code has no prompt-start hook. Seed the visible working row // once the PTY exists, then let real hook events refine or complete it. @@ -2603,6 +2616,7 @@ export function connectPanePty( // Persist the binding here so tab-level PTY ownership stays correct // even if no later spawn event or layout snapshot runs. deps.syncPanePtyLayoutBinding(pane.id, spawnedPtyId) + deps.syncPanePtyLayoutBindingByLeafId(pane.leafId, spawnedPtyId) deps.updateTabPtyId(deps.tabId, spawnedPtyId) transport.attach({ existingPtyId: spawnedPtyId, diff --git a/src/renderer/src/components/terminal-pane/pty-transport.ts b/src/renderer/src/components/terminal-pane/pty-transport.ts index ed840e9686b..5f669657726 100644 --- a/src/renderer/src/components/terminal-pane/pty-transport.ts +++ b/src/renderer/src/components/terminal-pane/pty-transport.ts @@ -562,6 +562,9 @@ export function createIpcPtyTransport(opts: IpcPtyTransportOptions = {}): PtyTra // it would reset lastActivityAt and destroy the recency sort order. if (!spawnResult.isReattach && !spawnResult.coldRestore) { onPtySpawn?.(spawnResult.id) + if (destroyed) { + return + } } registerPtyDataHandler(spawnResult.id) diff --git a/src/renderer/src/components/terminal-pane/remote-runtime-pty-transport.ts b/src/renderer/src/components/terminal-pane/remote-runtime-pty-transport.ts index ff2962d5783..0c043098cdc 100644 --- a/src/renderer/src/components/terminal-pane/remote-runtime-pty-transport.ts +++ b/src/renderer/src/components/terminal-pane/remote-runtime-pty-transport.ts @@ -173,6 +173,9 @@ export function createRemoteRuntimePtyTransport( rows: options.rows ?? 24 } onPtySpawn?.(remotePtyId) + if (destroyed || !connected || !remotePtyId) { + return undefined + } await subscribeToHandle() if (destroyed || !connected || !remotePtyId) { @@ -395,6 +398,9 @@ export function createRemoteRuntimePtyTransport( rows: options.rows ?? 24 } onPtySpawn?.(remotePtyId) + if (destroyed || !connected || !remotePtyId) { + return + } await subscribeToHandle() if (destroyed || !connected || !remotePtyId) { diff --git a/src/renderer/src/components/terminal-pane/use-terminal-pane-lifecycle.ts b/src/renderer/src/components/terminal-pane/use-terminal-pane-lifecycle.ts index 0e6fbf45842..f51e6150c58 100644 --- a/src/renderer/src/components/terminal-pane/use-terminal-pane-lifecycle.ts +++ b/src/renderer/src/components/terminal-pane/use-terminal-pane-lifecycle.ts @@ -148,6 +148,7 @@ type UseTerminalPaneLifecycleDeps = { }) => void setCacheTimerStartedAt: (key: string, ts: number | null) => void syncPanePtyLayoutBinding: (paneId: number, ptyId: string | null) => void + syncPanePtyLayoutBindingByLeafId: (leafId: string, ptyId: string | null) => void setTabPaneExpanded: (tabId: string, expanded: boolean) => void setTabCanExpandPane: (tabId: string, canExpand: boolean) => void setExpandedPane: (paneId: number | null) => void @@ -292,6 +293,7 @@ export function useTerminalPaneLifecycle({ dispatchNotification, setCacheTimerStartedAt, syncPanePtyLayoutBinding, + syncPanePtyLayoutBindingByLeafId, setTabPaneExpanded, setTabCanExpandPane, setExpandedPane, @@ -469,6 +471,7 @@ export function useTerminalPaneLifecycle({ dispatchNotification, setCacheTimerStartedAt, syncPanePtyLayoutBinding, + syncPanePtyLayoutBindingByLeafId, restoredPtyIdByLeafId: initialLayoutRef.current.ptyIdsByLeafId ?? {} }