From 274e2f229405f6156536cd996b5a918b7a7e56bb Mon Sep 17 00:00:00 2001 From: OrcaWin Date: Fri, 9 Oct 2026 21:33:19 -0700 Subject: [PATCH] fix(terminal): closing a tab is never a silent no-op (#27132) Clicking x did nothing when the worktree catalog had not hydrated yet or the tab's group record was missing. The close now routes by the host the tab's own PTY ids (or unified tab) name, and with no host at all still prunes locally and reaches main's close record; per-PTY teardown already fails closed for an unproven host. closeUnifiedTab closes a tab whose group record is gone instead of returning null. Fixes #11308 Co-authored-by: m4air --- ...erminal-tab-actions-unrouted-close.test.ts | 85 +++++++++++++++++++ .../terminal/terminal-tab-actions.ts | 12 ++- src/renderer/src/lib/terminal-close-route.ts | 45 ++++++++++ .../slices/tabs-close-dangling-group.test.ts | 36 ++++++++ .../store/slices/tabs/tabs-close-actions.ts | 20 ++--- .../store/slices/terminal-tab-retirement.ts | 2 +- 6 files changed, 182 insertions(+), 18 deletions(-) create mode 100644 src/renderer/src/components/terminal/terminal-tab-actions-unrouted-close.test.ts create mode 100644 src/renderer/src/lib/terminal-close-route.ts create mode 100644 src/renderer/src/store/slices/tabs-close-dangling-group.test.ts diff --git a/src/renderer/src/components/terminal/terminal-tab-actions-unrouted-close.test.ts b/src/renderer/src/components/terminal/terminal-tab-actions-unrouted-close.test.ts new file mode 100644 index 00000000000..1c0b4b07801 --- /dev/null +++ b/src/renderer/src/components/terminal/terminal-tab-actions-unrouted-close.test.ts @@ -0,0 +1,85 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest' +import { toRemoteRuntimePtyId } from '../../../../shared/remote-runtime-pty-id' + +const { closeWebRuntimeSessionTabMock, getStateMock, isWebRuntimeSessionActiveMock } = vi.hoisted( + () => ({ + closeWebRuntimeSessionTabMock: vi.fn(), + getStateMock: vi.fn(), + isWebRuntimeSessionActiveMock: vi.fn() + }) +) + +vi.mock('@/store', () => ({ useAppStore: { getState: getStateMock } })) + +vi.mock('@/runtime/web-runtime-session', () => ({ + closeWebRuntimeSessionTab: closeWebRuntimeSessionTabMock, + isWebRuntimeSessionActive: isWebRuntimeSessionActiveMock, + toHostSessionTabId: (tabId: string) => tabId +})) + +vi.mock('@/runtime/web-session-tabs-sync', () => ({ + getLatestWebSessionTabsPublicationEpoch: () => null, + resolveHostSessionTabIdForWebSessionTab: () => null +})) + +import { closeTerminalTab } from './terminal-tab-actions' + +const WORKTREE_ID = 'repo-1::/srv/app' + +/** The worktree catalog has not hydrated yet, so the worktree itself names no host (#11308). */ +function unhydratedState(ptyId: string | null, closeTab = vi.fn()) { + return { + settings: { activeRuntimeEnvironmentId: null }, + repos: [], + worktreesByRepo: {}, + detectedWorktreesByRepo: {}, + tabsByWorktree: { [WORKTREE_ID]: [{ id: 'tab-1', ptyId, worktreeId: WORKTREE_ID }] }, + unifiedTabsByWorktree: {}, + ptyIdsByTabId: {}, + terminalLayoutsByTabId: {}, + lastKnownRelayPtyIdByTabId: {}, + deferredSshSessionIdsByTabId: {}, + pendingReconnectPtyIdByTabId: {}, + activeWorktreeId: null, + activeTabId: null, + openFiles: [], + closeTab, + setActiveTab: vi.fn() + } +} + +describe('closeTerminalTab before the worktree catalog names a host', () => { + beforeEach(() => { + vi.clearAllMocks() + }) + + it('sends the close to the host its PTY id names', () => { + isWebRuntimeSessionActiveMock.mockImplementation((id: string | null) => id === 'env-1') + const closeTab = vi.fn() + getStateMock.mockReturnValue(unhydratedState(toRemoteRuntimePtyId('h-1', 'env-1'), closeTab)) + const onCancel = vi.fn() + + closeTerminalTab('tab-1', { onCancel }) + + expect(onCancel).not.toHaveBeenCalled() + expect(closeWebRuntimeSessionTabMock).toHaveBeenCalledWith({ + worktreeId: WORKTREE_ID, + tabId: 'tab-1', + environmentId: 'env-1', + reason: 'user' + }) + }) + + it('still closes the tab locally when nothing names its host', () => { + isWebRuntimeSessionActiveMock.mockReturnValue(false) + const closeTab = vi.fn() + getStateMock.mockReturnValue(unhydratedState(null, closeTab)) + const onClosed = vi.fn() + + closeTerminalTab('tab-1', { onClosed }) + + expect(closeTab).toHaveBeenCalledWith('tab-1') + expect(onClosed).toHaveBeenCalled() + expect(closeWebRuntimeSessionTabMock).not.toHaveBeenCalled() + }) +}) diff --git a/src/renderer/src/components/terminal/terminal-tab-actions.ts b/src/renderer/src/components/terminal/terminal-tab-actions.ts index 3013b921f70..489bbf01178 100644 --- a/src/renderer/src/components/terminal/terminal-tab-actions.ts +++ b/src/renderer/src/components/terminal/terminal-tab-actions.ts @@ -8,7 +8,7 @@ import { getLatestWebSessionTabsPublicationEpoch, resolveHostSessionTabIdForWebSessionTab } from '@/runtime/web-session-tabs-sync' -import { resolveTerminalWorktreeRoute } from '@/lib/terminal-worktree-route' +import { resolveTerminalCloseRoute } from '@/lib/terminal-close-route' import { guardPinnedTabClose, isUnifiedTabPinned, @@ -82,11 +82,9 @@ export function closeTerminalTab( return } const { worktreeId: owningWorktreeId, terminalTabId } = target - const worktreeRoute = resolveTerminalWorktreeRoute(state, owningWorktreeId) - if (!worktreeRoute) { - options?.onCancel?.() - return - } + // Why no early return: an unrouted close still prunes locally and reaches main's close record; + // teardown of a PTY whose host is unproven fails closed on its own. + const worktreeRoute = resolveTerminalCloseRoute(state, owningWorktreeId, terminalTabId) // Why: a pinned tab routes through the confirmation guard instead of closing // outright. `force` is the post-confirmation re-entry, which skips the guard. @@ -130,7 +128,7 @@ export function closeTerminalTab( return } - const runtimeEnvironmentId = worktreeRoute.runtimeEnvironmentId + const runtimeEnvironmentId = worktreeRoute?.runtimeEnvironmentId ?? null if (runtimeEnvironmentId && isWebRuntimeSessionActive(runtimeEnvironmentId)) { if (options?.reason === 'pty-exit') { // Why: stream exit is not host-tab closure; the HUB snapshot decides whether reconnect restores or removes this tab. diff --git a/src/renderer/src/lib/terminal-close-route.ts b/src/renderer/src/lib/terminal-close-route.ts new file mode 100644 index 00000000000..0232951a823 --- /dev/null +++ b/src/renderer/src/lib/terminal-close-route.ts @@ -0,0 +1,45 @@ +import { parseExecutionHostId } from '../../../shared/execution-host' +import { getPtyExecutionHost } from '../../../shared/terminal-execution-host' +import type { AppState } from '@/store/types' +import { collectPtyIdsForTab } from '@/store/slices/terminal-tab-retirement' +import { resolveTerminalWorktreeRoute, type TerminalWorktreeRoute } from './terminal-worktree-route' + +function routeForHost(hostId: string | null | undefined): TerminalWorktreeRoute | null { + const host = parseExecutionHostId(hostId) + if (host?.kind === 'runtime') { + return { runtimeEnvironmentId: host.environmentId } + } + return host ? { runtimeEnvironmentId: null } : null +} + +/** + * Which host a terminal tab's close goes to: the worktree catalog's route, else the one host the + * tab's own PTY ids name, else the host recorded on its unified tab. Null when nothing names one, + * e.g. before the catalog hydrates (#11308); per-PTY teardown still fails closed then. + */ +export function resolveTerminalCloseRoute( + state: AppState, + worktreeId: string, + terminalTabId: string +): TerminalWorktreeRoute | null { + const catalogRoute = resolveTerminalWorktreeRoute(state, worktreeId) + if (catalogRoute) { + return catalogRoute + } + const row = state.tabsByWorktree?.[worktreeId]?.find((tab) => tab.id === terminalTabId) + const ptyHosts = new Set( + collectPtyIdsForTab(state, terminalTabId, row?.ptyId ?? null) + .map(getPtyExecutionHost) + .filter((host) => host !== null) + ) + if (ptyHosts.size > 0) { + // Why: an id that names no host ('foreign') or two hosts proves only that it is not local. + const [host] = ptyHosts + return ptyHosts.size === 1 && host !== 'foreign' ? routeForHost(host) : null + } + const unified = state.unifiedTabsByWorktree?.[worktreeId]?.find( + (tab) => + tab.contentType === 'terminal' && (tab.entityId === terminalTabId || tab.id === terminalTabId) + ) + return routeForHost(unified?.executionHostId) +} diff --git a/src/renderer/src/store/slices/tabs-close-dangling-group.test.ts b/src/renderer/src/store/slices/tabs-close-dangling-group.test.ts new file mode 100644 index 00000000000..626b090ffd7 --- /dev/null +++ b/src/renderer/src/store/slices/tabs-close-dangling-group.test.ts @@ -0,0 +1,36 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest' +import { createTabsSliceMockApi } from './tabs-slice-test-harness' +import { createTestStore } from './store-test-helpers' + +vi.mock('sonner', () => ({ toast: { info: vi.fn(), success: vi.fn(), error: vi.fn() } })) + +createTabsSliceMockApi() + +const WT = 'repo1::/tmp/feature' + +describe('closeUnifiedTab with a missing group record (#11308)', () => { + let store: ReturnType + + beforeEach(() => { + store = createTestStore() + }) + + it.each(['terminal', 'editor'] as const)('still removes a %s tab', (contentType) => { + const kept = store.getState().createUnifiedTab(WT, contentType, { id: `${contentType}-kept` }) + const closing = store.getState().createUnifiedTab(WT, contentType, { id: `${contentType}-x` }) + store.setState({ groupsByWorktree: { [WT]: [] } }) + + const result = store.getState().closeUnifiedTab(closing.id) + + expect(result).toEqual({ closedTabId: closing.id, wasLastTab: false, worktreeId: WT }) + expect(store.getState().unifiedTabsByWorktree[WT]?.map((tab) => tab.id)).toEqual([kept.id]) + }) + + it('reports the last tab when the worktree has no other tab', () => { + const only = store.getState().createUnifiedTab(WT, 'editor', { id: 'editor-only' }) + store.setState({ groupsByWorktree: { [WT]: [] } }) + + expect(store.getState().closeUnifiedTab(only.id)?.wasLastTab).toBe(true) + expect(store.getState().unifiedTabsByWorktree[WT]).toEqual([]) + }) +}) diff --git a/src/renderer/src/store/slices/tabs/tabs-close-actions.ts b/src/renderer/src/store/slices/tabs/tabs-close-actions.ts index 6af491e7a37..56ed2fb6623 100644 --- a/src/renderer/src/store/slices/tabs/tabs-close-actions.ts +++ b/src/renderer/src/store/slices/tabs/tabs-close-actions.ts @@ -34,12 +34,12 @@ export function createTabsCloseActions( } const { tab, worktreeId } = found const group = findGroupForTab(state.groupsByWorktree, worktreeId, tab.groupId) - if (!group) { - return null - } + // Why: a tab whose group record vanished must still close; its order is the worktree's tabs. + const groupOrder = + group?.tabOrder ?? (state.unifiedTabsByWorktree[worktreeId] ?? []).map((item) => item.id) if (tab.contentType === 'terminal' && !opts?.terminalRetirementHandled) { - const dedupedGroupOrder = dedupeTabOrder(group.tabOrder) + const dedupedGroupOrder = dedupeTabOrder(groupOrder) const wasLastTab = dedupeTabOrder(dedupedGroupOrder.filter((id) => id !== tabId)).length === 0 // Why: unified-only hydrated tabs still own provider sessions without a legacy row, so retire every terminal close by entity id. @@ -47,7 +47,7 @@ export function createTabsCloseActions( return { closedTabId: tabId, wasLastTab, worktreeId } } - const dedupedGroupOrder = dedupeTabOrder(group.tabOrder) + const dedupedGroupOrder = dedupeTabOrder(groupOrder) const remainingOrder = dedupeTabOrder(dedupedGroupOrder.filter((id) => id !== tabId)) const wasLastTab = remainingOrder.length === 0 if (tab.contentType === 'agent-session') { @@ -78,13 +78,13 @@ export function createTabsCloseActions( } // Why: on closing the active tab, walk the MRU stack to the previously-active tab; pickNextActiveTab falls back to the neighbor. const nextActiveTabId = - group.activeTabId === tabId + group?.activeTabId === tabId ? wasLastTab ? null : pickNextActiveTab(dedupedGroupOrder, group.recentTabIds, tabId) - : group.activeTabId + : (group?.activeTabId ?? null) const nextRecentTabIds = sanitizeRecentTabIds( - (group.recentTabIds ?? []).filter((id) => id !== tabId), + (group?.recentTabIds ?? []).filter((id) => id !== tabId), remainingOrder ) const terminalEntityId = tab.contentType === 'terminal' ? tab.entityId : null @@ -100,7 +100,7 @@ export function createTabsCloseActions( delete nextUnreadTerminalTabs[terminalEntityId] } let nextGroups = (current.groupsByWorktree[worktreeId] ?? []).map((candidate) => - candidate.id === group.id + candidate.id === group?.id ? { ...candidate, activeTabId: nextActiveTabId, @@ -111,7 +111,7 @@ export function createTabsCloseActions( ) let nextLayoutByWorktree = current.layoutByWorktree let nextActiveGroupIdByWorktree = current.activeGroupIdByWorktree - if (wasLastTab && current.layoutByWorktree[worktreeId] && nextGroups.length > 1) { + if (group && wasLastTab && current.layoutByWorktree[worktreeId] && nextGroups.length > 1) { nextGroups = nextGroups.filter((candidate) => candidate.id !== group.id) const collapsedState = collapseGroupLayout( current.layoutByWorktree, diff --git a/src/renderer/src/store/slices/terminal-tab-retirement.ts b/src/renderer/src/store/slices/terminal-tab-retirement.ts index 0276541b7da..0310c65ba7d 100644 --- a/src/renderer/src/store/slices/terminal-tab-retirement.ts +++ b/src/renderer/src/store/slices/terminal-tab-retirement.ts @@ -63,7 +63,7 @@ export function getTerminalPtyOwnershipIdentity( return JSON.stringify(['runtime', environmentId, remote.handle]) } -function collectPtyIdsForTab( +export function collectPtyIdsForTab( state: TerminalTabRetirementState, tabId: string, rowPtyId: string | null | undefined