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 <m4air@m4airs-MacBook-Air.local>
This commit is contained in:
OrcaWin
2026-10-09 21:33:19 -07:00
committed by GitHub
co-authored by m4air
parent 7bd3928159
commit 274e2f2294
6 changed files with 182 additions and 18 deletions
@@ -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()
})
})
@@ -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.
@@ -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)
}
@@ -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<typeof createTestStore>
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([])
})
})
@@ -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,
@@ -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