Fix agent popover stale pane races

Co-authored-by: Orca <help@stably.ai>

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<string, unknown> = {}
+
+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(<WorktreeCardAgents worktreeId="wt-1" />)
+
+    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',

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(

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,

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
 }

src/renderer/src/components/terminal-pane/pty-connection.test.ts

@@ -100,6 +100,7 @@ type MockTransport = {
   sendInputAccepted?: ReturnType<typeof vi.fn>
   resize: ReturnType<typeof vi.fn>
   getPtyId: ReturnType<typeof vi.fn>
+  destroy: ReturnType<typeof vi.fn>
 }
 
 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<string, unknown> = {}) {
     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 }

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,

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)

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) {

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 ?? {}
     }