From 8feb63901db1c1612ace83e4c592a91c4eacf153 Mon Sep 17 00:00:00 2001 From: Jinwoo Hong <73622457+Jinwoo-H@users.noreply.github.com> Date: Sun, 7 Jun 2026 19:08:41 -0400 Subject: [PATCH] Address CodeRabbit follow-ups from #4825 (#4831) Co-authored-by: Cursor --- .../src/components/GitHubItemDialog.tsx | 1 + .../terminal/terminal-tab-actions.test.ts | 56 ++++++++++++++++++- .../terminal/terminal-tab-actions.ts | 31 +++++++--- .../src/hooks/useAutomationDispatchEvents.ts | 10 ++-- .../src/lib/launch-agent-in-new-tab.test.ts | 21 ++++++- .../src/lib/launch-agent-in-new-tab.ts | 21 ++++--- .../web-runtime-wake-terminal-respawn.test.ts | 13 ++++- .../web-runtime-wake-terminal-respawn.ts | 20 ++++--- .../src/runtime/web-session-tabs-sync.test.ts | 39 +++++++++++++ .../src/runtime/web-session-tabs-sync.ts | 33 +++++++++-- 10 files changed, 209 insertions(+), 36 deletions(-) diff --git a/src/renderer/src/components/GitHubItemDialog.tsx b/src/renderer/src/components/GitHubItemDialog.tsx index 0e2985de6..7b891ba00 100644 --- a/src/renderer/src/components/GitHubItemDialog.tsx +++ b/src/renderer/src/components/GitHubItemDialog.tsx @@ -3502,6 +3502,7 @@ function ChecksTab({ toast.error('Could not build the agent launch command.') return } + // Why: host-backed web launches can succeed without a local tab id. if (result.tabId) { focusTerminalTabSurface(result.tabId) } diff --git a/src/renderer/src/components/terminal/terminal-tab-actions.test.ts b/src/renderer/src/components/terminal/terminal-tab-actions.test.ts index 23c5351a1..6ae542860 100644 --- a/src/renderer/src/components/terminal/terminal-tab-actions.test.ts +++ b/src/renderer/src/components/terminal/terminal-tab-actions.test.ts @@ -126,7 +126,7 @@ describe('closeTerminalTab', () => { expect(closeTab).toHaveBeenCalledWith('local-tab-1') expect(closeWebRuntimeSessionTabMock).toHaveBeenCalledWith({ worktreeId: 'wt-1', - tabId: 'local-tab-1', + tabId: 'host-tab-1', environmentId: 'web-runtime' }) }) @@ -169,6 +169,60 @@ describe('closeTerminalTab', () => { expect(closeUnifiedTab).toHaveBeenCalledWith('unified-tab-1') }) + it('activates the next unified terminal tab when closing the active unified-only tab', () => { + const closeUnifiedTab = vi.fn() + const setActiveTab = vi.fn() + getStateMock.mockReturnValue({ + settings: { activeRuntimeEnvironmentId: null }, + tabsByWorktree: {}, + unifiedTabsByWorktree: { + 'wt-1': [ + { + id: 'unified-tab-1', + entityId: 'terminal-entity-1', + contentType: 'terminal', + groupId: 'group-1', + worktreeId: 'wt-1', + label: 'Claude', + customLabel: null, + color: null, + sortOrder: 0, + createdAt: 0, + isPreview: false, + isPinned: false + }, + { + id: 'unified-tab-2', + entityId: 'terminal-entity-2', + contentType: 'terminal', + groupId: 'group-1', + worktreeId: 'wt-1', + label: 'Terminal', + customLabel: null, + color: null, + sortOrder: 1, + createdAt: 0, + isPreview: false, + isPinned: false + } + ] + }, + activeWorktreeId: 'wt-1', + activeTabId: 'terminal-entity-1', + openFiles: [], + browserTabsByWorktree: {}, + closeTab: vi.fn(), + closeUnifiedTab, + setActiveTab, + setActiveWorktree: vi.fn() + }) + + closeTerminalTab('terminal-entity-1') + + expect(setActiveTab).toHaveBeenCalledWith('terminal-entity-2') + expect(closeUnifiedTab).toHaveBeenCalledWith('unified-tab-1') + }) + it('closes local-only agent tabs locally when they have no host session binding', () => { const closeTab = vi.fn() isWebRuntimeSessionActiveMock.mockReturnValue(true) diff --git a/src/renderer/src/components/terminal/terminal-tab-actions.ts b/src/renderer/src/components/terminal/terminal-tab-actions.ts index 14b583032..fc2f780c5 100644 --- a/src/renderer/src/components/terminal/terminal-tab-actions.ts +++ b/src/renderer/src/components/terminal/terminal-tab-actions.ts @@ -42,6 +42,22 @@ function resolveCloseTerminalTabTarget( return null } +// Why: host-backed terminals may only exist in unifiedTabsByWorktree as +// terminal entities, so close/sibling selection must merge tabsByWorktree and +// unified terminal entityIds into one deduped list per worktree. +function getWorktreeTerminalTabIds(state: TerminalTabActionState, worktreeId: string): string[] { + const ids = new Set() + for (const tab of state.tabsByWorktree[worktreeId] ?? []) { + ids.add(tab.id) + } + for (const tab of state.unifiedTabsByWorktree?.[worktreeId] ?? []) { + if (tab.contentType === 'terminal') { + ids.add(tab.entityId) + } + } + return [...ids] +} + function closeLocalTerminalTabState(terminalTabId: string): void { const state = useAppStore.getState() if ( @@ -144,7 +160,7 @@ export function closeTerminalTab(tabId: string): void { closeLocalTerminalTabState(terminalTabId) void closeWebRuntimeSessionTab({ worktreeId: owningWorktreeId, - tabId: terminalTabId, + tabId: hostBackedTabId, environmentId: runtimeEnvironmentId }) return @@ -153,8 +169,8 @@ export function closeTerminalTab(tabId: string): void { // have no host session binding and must still close locally. } - const currentTabs = state.tabsByWorktree[owningWorktreeId] ?? [] - if (currentTabs.length <= 1) { + const currentTerminalTabIds = getWorktreeTerminalTabIds(state, owningWorktreeId) + if (currentTerminalTabIds.length <= 1) { closeLocalTerminalTabState(terminalTabId) if (state.activeWorktreeId === owningWorktreeId) { // Why: only deactivate the worktree when no tabs of any kind remain. @@ -178,10 +194,11 @@ export function closeTerminalTab(tabId: string): void { } if (state.activeWorktreeId === owningWorktreeId && terminalTabId === state.activeTabId) { - const currentIndex = currentTabs.findIndex((tab) => tab.id === terminalTabId) - const nextTab = currentTabs[currentIndex + 1] ?? currentTabs[currentIndex - 1] - if (nextTab) { - state.setActiveTab(nextTab.id) + const currentIndex = currentTerminalTabIds.indexOf(terminalTabId) + const nextTabId = + currentTerminalTabIds[currentIndex + 1] ?? currentTerminalTabIds[currentIndex - 1] + if (nextTabId) { + state.setActiveTab(nextTabId) } } diff --git a/src/renderer/src/hooks/useAutomationDispatchEvents.ts b/src/renderer/src/hooks/useAutomationDispatchEvents.ts index d0a948bc6..7d3622a2a 100644 --- a/src/renderer/src/hooks/useAutomationDispatchEvents.ts +++ b/src/renderer/src/hooks/useAutomationDispatchEvents.ts @@ -378,11 +378,13 @@ export function useAutomationDispatchEvents(): void { if (!result) { throw new Error('Unable to build an agent launch plan.') } - if (!result.tabId) { - throw new Error('Host-backed agent tabs are not yet trackable for automation dispatch.') - } const launchedTabId = result.tabId - observeAgentStatus(launchedTabId, dispatchStartedAt) + // Why: host-backed automation terminals may lack a local tab id; skip + // pane-key status observation while background session output still + // tracks completion. + if (launchedTabId) { + observeAgentStatus(launchedTabId, dispatchStartedAt) + } try { await markDispatchResult({ runId: run.id, diff --git a/src/renderer/src/lib/launch-agent-in-new-tab.test.ts b/src/renderer/src/lib/launch-agent-in-new-tab.test.ts index a42a900a6..153597132 100644 --- a/src/renderer/src/lib/launch-agent-in-new-tab.test.ts +++ b/src/renderer/src/lib/launch-agent-in-new-tab.test.ts @@ -33,8 +33,10 @@ vi.mock('@/store', () => ({ } })) +const mockToastError = vi.fn() + vi.mock('sonner', () => ({ - toast: { message: vi.fn() } + toast: { message: vi.fn(), error: mockToastError } })) vi.mock('@/components/tab-bar/reconcile-order', () => ({ @@ -125,10 +127,27 @@ describe('launchAgentInNewTab', () => { }) expect(mockCreateTab).not.toHaveBeenCalled() expect(mockQueueTabStartupCommand).not.toHaveBeenCalled() + await Promise.resolve() expect(mockSetActiveTabType).toHaveBeenCalledWith('terminal') expect(store.closeTab).toHaveBeenCalledWith('stale-agent-tab') }) + it('surfaces a toast when host agent launch fails in paired web clients', async () => { + mockIsWebRuntimeSessionActive.mockReturnValue(true) + mockCreateWebRuntimeSessionTerminal.mockResolvedValue(false) + store.settings = { agentCmdOverrides: {}, activeRuntimeEnvironmentId: 'web-runtime' } + const { launchAgentInNewTab } = await import('./launch-agent-in-new-tab') + + launchAgentInNewTab({ + agent: 'claude', + worktreeId: 'wt-1' + }) + + await Promise.resolve() + expect(mockToastError).toHaveBeenCalledWith('Could not launch claude in a new terminal.') + expect(mockSetActiveTabType).not.toHaveBeenCalled() + }) + it('queues initial working status for Command Code argv prompt launches', async () => { const { launchAgentInNewTab } = await import('./launch-agent-in-new-tab') diff --git a/src/renderer/src/lib/launch-agent-in-new-tab.ts b/src/renderer/src/lib/launch-agent-in-new-tab.ts index ea925ec21..58e43e3ae 100644 --- a/src/renderer/src/lib/launch-agent-in-new-tab.ts +++ b/src/renderer/src/lib/launch-agent-in-new-tab.ts @@ -205,8 +205,9 @@ export function launchAgentInNewTab(args: LaunchAgentInNewTabArgs): LaunchAgentI const runtimeEnvironmentId = store.settings?.activeRuntimeEnvironmentId?.trim() if (isWebRuntimeSessionActive(runtimeEnvironmentId) && pasteDraftAfterLaunch === null) { - // Why: paired web tabs are host-owned. Local-only agent tabs cannot be - // closed because close routes through session.tabs.close on the host. + // Why: paired web tabs are host-owned and return tabId: null on success. + // Local-only agent tabs cannot be closed because close routes through + // session.tabs.close on the host, so prune them before the host snapshot. removeStaleLocalAgentTabsForWebHostLaunch(worktreeId) void createWebRuntimeSessionTerminal({ worktreeId, @@ -214,13 +215,19 @@ export function launchAgentInNewTab(args: LaunchAgentInNewTabArgs): LaunchAgentI targetGroupId: groupId, activate: true, ...(hasPrompt ? { command: startupPlan.launchCommand } : { agent }) - }).then(() => { + }).then((created) => { + // Why: created means the host accepted the launch, not that a local tab + // exists; keep pruning stale local rows until the snapshot mirrors. removeStaleLocalAgentTabsForWebHostLaunch(worktreeId) + if (!created) { + toast.error(`Could not launch ${agent} in a new terminal.`) + return + } + store.setActiveTabType('terminal') + if (hasPrompt) { + onPromptDelivered?.() + } }) - store.setActiveTabType('terminal') - if (hasPrompt) { - onPromptDelivered?.() - } return { tabId: null, startupPlan, pasteDraftAfterLaunch: false } } diff --git a/src/renderer/src/runtime/web-runtime-wake-terminal-respawn.test.ts b/src/renderer/src/runtime/web-runtime-wake-terminal-respawn.test.ts index bd17392a6..6e55e47e6 100644 --- a/src/renderer/src/runtime/web-runtime-wake-terminal-respawn.test.ts +++ b/src/renderer/src/runtime/web-runtime-wake-terminal-respawn.test.ts @@ -1,6 +1,7 @@ import { beforeEach, describe, expect, it } from 'vitest' import { beginWebRuntimeWakeTerminalRespawn, + clearWebRuntimeWakeTerminalRespawnForWorktree, endWebRuntimeWakeTerminalRespawn, resetWebRuntimeWakeTerminalRespawnForTests, shouldSkipWebRuntimeWakeTerminalRespawn @@ -11,11 +12,19 @@ describe('web-runtime-wake-terminal-respawn', () => { resetWebRuntimeWakeTerminalRespawnForTests() }) - it('dedupes wake respawn requests for the same worktree', () => { + it('dedupes concurrent wake respawn requests for the same worktree', () => { expect(beginWebRuntimeWakeTerminalRespawn('wt-1')).toBe(true) expect(shouldSkipWebRuntimeWakeTerminalRespawn('wt-1')).toBe(true) expect(beginWebRuntimeWakeTerminalRespawn('wt-1')).toBe(false) endWebRuntimeWakeTerminalRespawn('wt-1') - expect(shouldSkipWebRuntimeWakeTerminalRespawn('wt-1')).toBe(true) + expect(shouldSkipWebRuntimeWakeTerminalRespawn('wt-1')).toBe(false) + expect(beginWebRuntimeWakeTerminalRespawn('wt-1')).toBe(true) + }) + + it('clears wake respawn tracking for a removed worktree', () => { + beginWebRuntimeWakeTerminalRespawn('wt-1') + clearWebRuntimeWakeTerminalRespawnForWorktree('wt-1') + expect(shouldSkipWebRuntimeWakeTerminalRespawn('wt-1')).toBe(false) + expect(beginWebRuntimeWakeTerminalRespawn('wt-1')).toBe(true) }) }) diff --git a/src/renderer/src/runtime/web-runtime-wake-terminal-respawn.ts b/src/renderer/src/runtime/web-runtime-wake-terminal-respawn.ts index 55a419d12..7e1dcc510 100644 --- a/src/renderer/src/runtime/web-runtime-wake-terminal-respawn.ts +++ b/src/renderer/src/runtime/web-runtime-wake-terminal-respawn.ts @@ -1,18 +1,13 @@ -const wakeTerminalRespawnRequestedByWorktree = new Set() const wakeTerminalRespawnInFlightByWorktree = new Set() export function shouldSkipWebRuntimeWakeTerminalRespawn(worktreeId: string): boolean { - return ( - wakeTerminalRespawnRequestedByWorktree.has(worktreeId) || - wakeTerminalRespawnInFlightByWorktree.has(worktreeId) - ) + return wakeTerminalRespawnInFlightByWorktree.has(worktreeId) } export function beginWebRuntimeWakeTerminalRespawn(worktreeId: string): boolean { - if (shouldSkipWebRuntimeWakeTerminalRespawn(worktreeId)) { + if (wakeTerminalRespawnInFlightByWorktree.has(worktreeId)) { return false } - wakeTerminalRespawnRequestedByWorktree.add(worktreeId) wakeTerminalRespawnInFlightByWorktree.add(worktreeId) return true } @@ -21,7 +16,14 @@ export function endWebRuntimeWakeTerminalRespawn(worktreeId: string): void { wakeTerminalRespawnInFlightByWorktree.delete(worktreeId) } -export function resetWebRuntimeWakeTerminalRespawnForTests(): void { - wakeTerminalRespawnRequestedByWorktree.clear() +export function clearWebRuntimeWakeTerminalRespawnForWorktree(worktreeId: string): void { + wakeTerminalRespawnInFlightByWorktree.delete(worktreeId) +} + +export function clearAllWebRuntimeWakeTerminalRespawn(): void { wakeTerminalRespawnInFlightByWorktree.clear() } + +export function resetWebRuntimeWakeTerminalRespawnForTests(): void { + clearAllWebRuntimeWakeTerminalRespawn() +} diff --git a/src/renderer/src/runtime/web-session-tabs-sync.test.ts b/src/renderer/src/runtime/web-session-tabs-sync.test.ts index 494137333..69cc19a25 100644 --- a/src/renderer/src/runtime/web-session-tabs-sync.test.ts +++ b/src/renderer/src/runtime/web-session-tabs-sync.test.ts @@ -439,6 +439,45 @@ describe('applyWebSessionTabsSnapshot', () => { expect(patch.groupsByWorktree?.[WT]?.[0]?.tabOrder).toEqual([mirroredId]) }) + it('keeps stale local agent tabs when the host mirror is for a different agent', () => { + const staleLocalClaudeTab: TerminalTab = { + id: 'local-claude-tab', + ptyId: null, + worktreeId: WT, + title: 'Claude', + defaultTitle: 'Claude', + customTitle: null, + color: null, + sortOrder: 0, + createdAt: NOW, + launchAgent: 'claude' + } + + const patch = applyWebSessionTabsSnapshot( + makeState({ + tabsByWorktree: { [WT]: [staleLocalClaudeTab] } + }), + makeSnapshot([ + { + type: 'terminal', + id: HOST_SURFACE_ID, + title: 'Codex', + parentTabId: 'host-tab-1', + leafId: LEAF_ID, + isActive: true, + launchAgent: 'codex', + status: 'ready', + terminal: 'terminal-1' + } + ]), + ENV, + NOW + ) as Partial + + expect(patch.tabsByWorktree?.[WT]).toHaveLength(2) + expect(patch.tabsByWorktree?.[WT]?.some((tab) => tab.id === 'local-claude-tab')).toBe(true) + }) + it('hydrates ready host terminal surfaces as remote runtime terminal tabs', () => { const patch = applyWebSessionTabsSnapshot( makeState(), diff --git a/src/renderer/src/runtime/web-session-tabs-sync.ts b/src/renderer/src/runtime/web-session-tabs-sync.ts index 4d570bfba..90423efe1 100644 --- a/src/renderer/src/runtime/web-session-tabs-sync.ts +++ b/src/renderer/src/runtime/web-session-tabs-sync.ts @@ -25,7 +25,8 @@ import type { TabGroupLayoutNode, TerminalLayoutSnapshot, TerminalPaneLayoutNode, - TerminalTab + TerminalTab, + TuiAgent } from '../../../shared/types' import type { OpenFile } from '../store/slices/editor' import { isTerminalLeafId, makePaneKey, parsePaneKey } from '../../../shared/stable-pane-id' @@ -41,6 +42,8 @@ import { import { toRuntimeWorktreeSelector } from './runtime-worktree-selector' import { beginWebRuntimeWakeTerminalRespawn, + clearAllWebRuntimeWakeTerminalRespawn, + clearWebRuntimeWakeTerminalRespawnForWorktree, endWebRuntimeWakeTerminalRespawn, shouldSkipWebRuntimeWakeTerminalRespawn } from './web-runtime-wake-terminal-respawn' @@ -245,6 +248,7 @@ function clearWebSessionTabsTrackingForWorktree(environmentId: string, worktreeI const key = sessionTabsFreshnessKey(environmentId, worktreeId) latestSessionTabsSnapshotByWorktree.delete(key) lastHostTerminalTabCountByWorktree.delete(key) + clearWebRuntimeWakeTerminalRespawnForWorktree(worktreeId) const keyPrefix = `${environmentId}:${worktreeId}:` for (const key of hostSessionTabIdByLocalKey.keys()) { if (key.startsWith(keyPrefix)) { @@ -274,6 +278,7 @@ export function clearWebSessionTabsTrackingForEnvironment(environmentId: string) hostSessionTabIdByLocalKey.delete(key) } } + clearAllWebRuntimeWakeTerminalRespawn() } function hostSessionTabMappingKey(args: { @@ -419,11 +424,16 @@ function shouldReplaceTerminalTab( tab: TerminalTab, environmentId: string, nextRemotePtyIds: ReadonlySet, - nextMirroredTerminalIds: ReadonlySet + nextMirroredTerminalIds: ReadonlySet, + nextMirroredLaunchAgents: ReadonlySet ): boolean { - if (tab.launchAgent && !isMirroredTerminalSurfaceId(tab.id) && nextMirroredTerminalIds.size > 0) { + if ( + tab.launchAgent && + !isMirroredTerminalSurfaceId(tab.id) && + nextMirroredLaunchAgents.has(tab.launchAgent) + ) { // Why: paired web agent quick-launch used to create local-only tabs before - // the host snapshot landed. Once host mirrors exist, retire the stale row. + // the host snapshot landed. Retire only the matching agent's stale row. return true } if (isMirroredTerminalSurfaceId(tab.id)) { @@ -1438,9 +1448,20 @@ export function applyWebSessionTabsSnapshot( const nextMirroredTerminalIds = new Set( terminalSurfaceTabs.map((tab) => toWebTerminalSurfaceTabId(tab.parentTabId)) ) + const nextMirroredLaunchAgents = new Set( + terminalSurfaceTabs + .map((tab) => tab.launchAgent) + .filter((agent): agent is TuiAgent => Boolean(agent)) + ) const retainedTerminalTabs = currentTerminalTabs.filter( (tab) => - !shouldReplaceTerminalTab(tab, environmentId, nextRemotePtyIds, nextMirroredTerminalIds) + !shouldReplaceTerminalTab( + tab, + environmentId, + nextRemotePtyIds, + nextMirroredTerminalIds, + nextMirroredLaunchAgents + ) ) const mirroredTerminalTabs = buildMirroredTerminalTabs( snapshot, @@ -2300,6 +2321,8 @@ export function useWebSessionTabsSync(): void { beginWebRuntimeWakeTerminalRespawn(activeWorktreeId) ) { requestedRespawnAfterWake = true + // Why: wake recovery must recreate the terminal without changing + // selected worktree to avoid re-triggering activation churn. void createWebRuntimeSessionTerminal({ worktreeId: activeWorktreeId, environmentId,