diff --git a/src/renderer/src/App.tsx b/src/renderer/src/App.tsx index b9765d914..81f72be49 100644 --- a/src/renderer/src/App.tsx +++ b/src/renderer/src/App.tsx @@ -207,6 +207,7 @@ import { showTerminalShortcutCaptureNotification } from '@/lib/terminal-shortcut import { resolveMountedLazyModalIds, type LazyModalId } from './lazy-modal-mount-state' import { translate } from '@/i18n/i18n' import PinnedTabCloseDialog from './components/terminal-pane/PinnedTabCloseDialog' +import RunningTerminalCloseDialog from './components/terminal-pane/RunningTerminalCloseDialog' import WorktreeBaseFallbackDialog from './components/WorktreeBaseFallbackDialog' import { useOsc52ClipboardDefaultOnNotice } from './components/terminal-pane/osc52-clipboard-default-on-notice' import { @@ -2789,6 +2790,7 @@ function App(): React.JSX.Element { + {/* Why: Electron's drag-region hit-test is DOM-order-based (ignores z-index); render last so WindowControls stay clickable. */} {hasCustomTitleBar && } diff --git a/src/renderer/src/components/Terminal.tsx b/src/renderer/src/components/Terminal.tsx index 6eaf0afc7..dc39c3a06 100644 --- a/src/renderer/src/components/Terminal.tsx +++ b/src/renderer/src/components/Terminal.tsx @@ -1797,7 +1797,8 @@ function Terminal(): React.JSX.Element | null { ) { if (unifiedTab.contentType === 'terminal') { // Why: paired-host bulk close must revoke renderer resume and hook authority, not just remove the host session tab. - closeTerminalTab(unifiedTab.entityId) + // No running-process prompt: "Close Others" over N busy tabs would be a modal storm. + closeTerminalTab(unifiedTab.entityId, { skipRunningProcessConfirm: true }) } else { void closeWebRuntimeSessionTab({ worktreeId: activeWorktreeId, diff --git a/src/renderer/src/components/tab-group/useTabGroupWorkspaceModel.ts b/src/renderer/src/components/tab-group/useTabGroupWorkspaceModel.ts index 4bb234b97..f493f2e58 100644 --- a/src/renderer/src/components/tab-group/useTabGroupWorkspaceModel.ts +++ b/src/renderer/src/components/tab-group/useTabGroupWorkspaceModel.ts @@ -235,10 +235,12 @@ export function useTabGroupWorkspaceModel({ worktreeId ) if (item.contentType === 'terminal') { - closeTerminalTab(item.entityId) - if (!opts?.skipEmptyCheck) { - leaveWorktreeIfEmpty() - } + // Why: closeTerminalTab can defer behind a pin / running-process dialog, so the + // empty check has to run on the actual close — never on cancel. + closeTerminalTab( + item.entityId, + opts?.skipEmptyCheck ? undefined : { onClosed: leaveWorktreeIfEmpty } + ) return } if (item.contentType === 'browser') { @@ -296,7 +298,8 @@ export function useTabGroupWorkspaceModel({ ) if (item.contentType === 'terminal' && isWebRuntimeSessionActive(runtimeEnvironmentId)) { // Why: revoke local resume + hook authority before the host removes its canonical tab. - closeTerminalTab(item.entityId) + // No running-process prompt: a bulk close of N busy tabs would be a modal storm. + closeTerminalTab(item.entityId, { skipRunningProcessConfirm: true }) continue } if (item.contentType === 'browser') { diff --git a/src/renderer/src/components/terminal-pane/CloseTerminalDialog.tsx b/src/renderer/src/components/terminal-pane/CloseTerminalDialog.tsx index 771d5e15e..c4da52444 100644 --- a/src/renderer/src/components/terminal-pane/CloseTerminalDialog.tsx +++ b/src/renderer/src/components/terminal-pane/CloseTerminalDialog.tsx @@ -17,17 +17,26 @@ export type CloseTerminalDialogCopyKind = 'command' | 'agent' export default function CloseTerminalDialog({ open, copyKind = 'command', + tabLabel, + subjectKey, onCancel, onConfirm }: { open: boolean copyKind?: CloseTerminalDialogCopyKind + /** Names the tab when the prompt can target a tab the user is not looking at + * (tab-strip X, middle-click). Omitted for the focused-pane keyboard path. */ + tabLabel?: string + /** Identifies what is being closed, for hosts that reuse one open dialog across a queue + * of confirmations. Changing it clears the previous subject's "don't ask again" tick. */ + subjectKey?: string onCancel: () => void onConfirm: (dontAskAgain: boolean) => void }): React.JSX.Element { const checkboxId = useId() const [dontAskAgain, setDontAskAgain] = useState(false) const [previousOpen, setPreviousOpen] = useState(open) + const [previousSubjectKey, setPreviousSubjectKey] = useState(subjectKey) // Why: each reopen represents a fresh confirmation, so clear the old choice // during render rather than briefly painting it while the dialog opens. @@ -38,7 +47,18 @@ export default function CloseTerminalDialog({ } } + // Why: a queued confirmation swaps the subject without ever closing the dialog, so the + // reopen reset above never fires. Ignore the swap to undefined as the dialog closes — + // clearing the tick mid-exit-animation would be visible for no reason. + if (subjectKey !== previousSubjectKey) { + setPreviousSubjectKey(subjectKey) + if (subjectKey !== undefined) { + setDontAskAgain(false) + } + } + const isAgent = copyKind === 'agent' + const trimmedTabLabel = tabLabel?.trim() return ( + {trimmedTabLabel ? ( +

+ {trimmedTabLabel} +

+ ) : null}
& { onConfirm: () => void }, + updateSettings: AppState['updateSettings'] +): Promise { + useAppStore.setState({ updateSettings }) + useRunningTerminalCloseConfirmStore.getState().requestRunningTerminalCloseConfirm({ + terminalTabId: 'tab-1', + tabLabel: 'dev server', + copyKind: 'command', + ...request + }) + + const container = document.createElement('div') + document.body.appendChild(container) + const root = createRoot(container) + mountedRoots.push(root) + + await act(async () => { + root.render() + }) +} + +function getButton(label: string): HTMLButtonElement { + const button = [...document.body.querySelectorAll('button')].find( + (candidate) => candidate.textContent === label + ) + if (!button) { + throw new Error(`Button not found: ${label}`) + } + return button +} + +function getCheckbox(): HTMLButtonElement { + const checkbox = document.body.querySelector('[role="checkbox"]') + if (!checkbox) { + throw new Error('Checkbox not found') + } + return checkbox +} + +describe('RunningTerminalCloseDialog', () => { + beforeEach(() => { + useAppStore.setState(initialState, true) + // Monotonic across tests: the store is a singleton, so winding the clock back would + // leave a previous test's guard deadline in the future and block every action. + clock += 10_000 + vi.spyOn(Date, 'now').mockImplementation(() => clock) + }) + + afterEach(async () => { + while (useRunningTerminalCloseConfirmStore.getState().runningTerminalCloseConfirm !== null) { + advancePastGuard() + useRunningTerminalCloseConfirmStore.getState().dismissRunningTerminalClose() + } + vi.mocked(Date.now).mockRestore() + await act(async () => { + for (const root of mountedRoots.splice(0)) { + root.unmount() + } + }) + document.body.innerHTML = '' + useAppStore.setState(initialState, true) + }) + + it('names the tab so a background close is not ambiguous', async () => { + const onConfirm = vi.fn() + const updateSettings = vi.fn().mockResolvedValue(undefined) + + await renderDialog({ onConfirm }, updateSettings) + + expect(document.body.textContent).toContain('Stop running command?') + expect(document.body.textContent).toContain('dev server') + + await act(async () => { + getButton('Stop and Close').click() + }) + + expect(updateSettings).not.toHaveBeenCalled() + expect(onConfirm).toHaveBeenCalledTimes(1) + }) + + it('uses the agent copy for an agent pane', async () => { + const updateSettings = vi.fn().mockResolvedValue(undefined) + + await renderDialog({ onConfirm: vi.fn(), copyKind: 'agent' }, updateSettings) + + expect(document.body.textContent).toContain('Stop this agent?') + expect(getButton('Stop Agent')).toBeTruthy() + }) + + it('persists the opt-out when "don\'t ask again" is checked', async () => { + const onConfirm = vi.fn() + const updateSettings = vi.fn().mockResolvedValue(undefined) + + await renderDialog({ onConfirm }, updateSettings) + + await act(async () => { + getCheckbox().click() + }) + await act(async () => { + getButton('Stop and Close').click() + }) + + expect(updateSettings).toHaveBeenCalledWith({ + skipCloseTerminalWithRunningProcessConfirm: true + }) + expect(onConfirm).toHaveBeenCalledTimes(1) + }) + + // Why: this queue opens after an async probe while the pinned queue opens synchronously, + // so both can be pending at once. Two modal overlays + focus traps is the bug. + it('waits for a visible pinned confirmation instead of stacking a second modal', async () => { + const updateSettings = vi.fn().mockResolvedValue(undefined) + useAppStore.setState({ + pinnedTabCloseConfirm: { tabLabel: 'pinned tab', onConfirm: vi.fn() } + }) + + await renderDialog({ onConfirm: vi.fn() }, updateSettings) + + expect(document.body.textContent).not.toContain('Stop running command?') + + await act(async () => { + useAppStore.setState({ pinnedTabCloseConfirm: null }) + }) + + expect(document.body.textContent).toContain('Stop running command?') + }) + + it('does not carry the opt-out tick over to the next queued tab', async () => { + const updateSettings = vi.fn().mockResolvedValue(undefined) + const nextOnConfirm = vi.fn() + + await renderDialog({ onConfirm: vi.fn(), onCancel: vi.fn() }, updateSettings) + await act(async () => { + useRunningTerminalCloseConfirmStore.getState().requestRunningTerminalCloseConfirm({ + terminalTabId: 'tab-2', + tabLabel: 'build watcher', + copyKind: 'command', + onConfirm: nextOnConfirm + }) + }) + + await act(async () => { + getCheckbox().click() + }) + await act(async () => { + getButton('Cancel').click() + }) + + expect(document.body.textContent).toContain('build watcher') + expect(getCheckbox().getAttribute('data-state')).toBe('unchecked') + + advancePastGuard() + await act(async () => { + getButton('Stop and Close').click() + }) + + expect(updateSettings).not.toHaveBeenCalled() + expect(nextOnConfirm).toHaveBeenCalledTimes(1) + }) + + it('drops a queued prompt once the user opts out of asking again', async () => { + const updateSettings = vi.fn().mockResolvedValue(undefined) + const onConfirm = vi.fn() + const nextOnConfirm = vi.fn() + + await renderDialog({ onConfirm }, updateSettings) + await act(async () => { + useRunningTerminalCloseConfirmStore.getState().requestRunningTerminalCloseConfirm({ + terminalTabId: 'tab-2', + tabLabel: 'build watcher', + copyKind: 'command', + onConfirm: nextOnConfirm + }) + }) + + await act(async () => { + getCheckbox().click() + }) + await act(async () => { + getButton('Stop and Close').click() + }) + + expect(updateSettings).toHaveBeenCalledWith({ + skipCloseTerminalWithRunningProcessConfirm: true + }) + expect(onConfirm).toHaveBeenCalledTimes(1) + expect(nextOnConfirm).toHaveBeenCalledTimes(1) + expect(document.body.textContent).not.toContain('build watcher') + }) + + it('cancels without closing and shows the next queued tab', async () => { + const onConfirm = vi.fn() + const onCancel = vi.fn() + const nextOnConfirm = vi.fn() + const updateSettings = vi.fn().mockResolvedValue(undefined) + + await renderDialog({ onConfirm, onCancel }, updateSettings) + await act(async () => { + useRunningTerminalCloseConfirmStore.getState().requestRunningTerminalCloseConfirm({ + terminalTabId: 'tab-2', + tabLabel: 'build watcher', + copyKind: 'command', + onConfirm: nextOnConfirm + }) + }) + + await act(async () => { + getButton('Cancel').click() + }) + + expect(onCancel).toHaveBeenCalledTimes(1) + expect(onConfirm).not.toHaveBeenCalled() + expect(document.body.textContent).toContain('build watcher') + + advancePastGuard() + await act(async () => { + getButton('Stop and Close').click() + }) + + expect(nextOnConfirm).toHaveBeenCalledTimes(1) + }) +}) diff --git a/src/renderer/src/components/terminal-pane/RunningTerminalCloseDialog.tsx b/src/renderer/src/components/terminal-pane/RunningTerminalCloseDialog.tsx new file mode 100644 index 000000000..cf93fa772 --- /dev/null +++ b/src/renderer/src/components/terminal-pane/RunningTerminalCloseDialog.tsx @@ -0,0 +1,45 @@ +import { useAppStore } from '@/store' +import { useRunningTerminalCloseConfirmStore } from '@/store/running-terminal-close-confirm' +import CloseTerminalDialog from './CloseTerminalDialog' + +/** Hosts the running-process close confirmation for tab-level closes (tab-strip X, + * middle-click, tab menu, tab groups, floating panel) so they share the prompt Cmd+W + * already raised. Store-driven, like PinnedTabCloseDialog, because those closes run + * outside any pane's React tree. */ +export default function RunningTerminalCloseDialog(): React.JSX.Element { + const request = useRunningTerminalCloseConfirmStore((state) => state.runningTerminalCloseConfirm) + const confirmClose = useRunningTerminalCloseConfirmStore( + (state) => state.confirmRunningTerminalClose + ) + const confirmAllCloses = useRunningTerminalCloseConfirmStore( + (state) => state.confirmAllRunningTerminalCloses + ) + const dismissClose = useRunningTerminalCloseConfirmStore( + (state) => state.dismissRunningTerminalClose + ) + const updateSettings = useAppStore((state) => state.updateSettings) + // Why: this queue is async (it opens after a probe) while the pinned queue is synchronous, + // so both can be pending at once. Wait rather than stack two modal overlays and focus traps. + const pinnedRequest = useAppStore((state) => state.pinnedTabCloseConfirm) + + return ( + { + if (dontAskAgain) { + void updateSettings({ skipCloseTerminalWithRunningProcessConfirm: true }) + // Why: the user just opted out of this prompt; a queued one must not still appear. + confirmAllCloses() + return + } + confirmClose() + }} + /> + ) +} diff --git a/src/renderer/src/components/terminal-pane/TerminalPane.tsx b/src/renderer/src/components/terminal-pane/TerminalPane.tsx index 9666ae98e..c2b2149ba 100644 --- a/src/renderer/src/components/terminal-pane/TerminalPane.tsx +++ b/src/renderer/src/components/terminal-pane/TerminalPane.tsx @@ -14,7 +14,6 @@ import { createPortal } from 'react-dom' import type { CSSProperties } from 'react' import type { IDisposable } from '@xterm/xterm' import { useAppStore } from '../../store' -import { isUnifiedTabPinned } from '@/store/pinned-tab-close-guard' import { useLinkRoutingPreferenceDialog } from '@/components/link-routing-preference-dialog' import { DaemonActionDialog, useDaemonActions } from '@/components/shared/useDaemonActions' import { @@ -64,6 +63,8 @@ import type { MacOptionAsAlt } from './terminal-shortcut-policy' import { useEffectiveMacOptionAsAlt } from '@/lib/keyboard-layout/use-effective-mac-option-as-alt' import { useTerminalFontZoom } from './useTerminalFontZoom' import CloseTerminalDialog, { type CloseTerminalDialogCopyKind } from './CloseTerminalDialog' +import { resolveLeafCloseCopyKind } from '../terminal/terminal-close-copy-kind' +import { RUNNING_CLOSE_PROBE_TIMEOUT_MS } from '../terminal/running-terminal-close-guard' import CodexRestartChip from '../CodexRestartChip' import { MobileDriverOverlay } from './MobileDriverOverlay' import { stripSshReconnectOwnedErrorLines, TerminalErrorToast } from './TerminalErrorToast' @@ -1288,30 +1289,21 @@ function TerminalPane( ) // Cmd+W confirms before killing a shell with a running child (e.g. npm run dev); idle prompts close immediately, and Ctrl+D bypasses by design. + // Why: the agent-vs-command rule is shared with the tab-strip prompt so the two close paths cannot word the same close differently (#10142). const getCloseDialogCopyKind = useCallback( - (paneId: number): CloseTerminalDialogCopyKind => { - const leafId = managerRef.current?.getLeafId(paneId) - if (!leafId) { - return 'command' - } - const agentType = - useAppStore.getState().agentStatusByPaneKey[makePaneKey(tabId, leafId)]?.agentType - return agentType && agentType !== 'unknown' ? 'agent' : 'command' - }, + (paneId: number): CloseTerminalDialogCopyKind => + resolveLeafCloseCopyKind(tabId, managerRef.current?.getLeafId(paneId)), [tabId] ) const handleRequestClosePane = useCallback( (paneId: number) => { - // Why: closing the last pane of a pinned tab prefers the pin dialog over the running-process prompt; non-pinned tabs keep the process prompt. - const isLastPane = (managerRef.current?.getPanes().length ?? 0) <= 1 - if (isLastPane) { - const state = useAppStore.getState() - const confirmPinned = state.settings?.confirmClosePinnedTab ?? true - if (confirmPinned && isUnifiedTabPinned(state, worktreeId, tabId)) { - executeClosePane(paneId) - return - } + // Why: the last pane closes the whole tab, and closeTerminalTab owns both the pinned + // and running-process guards. Probing here too would double-prompt, and its nullable + // transport ptyId would silently skip the prompt the mouse paths now get (#10142). + if ((managerRef.current?.getPanes().length ?? 0) <= 1) { + executeClosePane(paneId) + return } const transport = paneTransportsRef.current.get(paneId) const ptyId = transport?.getPtyId() @@ -1320,18 +1312,40 @@ function TerminalPane( return } const settings = useAppStore.getState().settings + // Why: same bound as the whole-tab guard, so a wedged remote probe never leaves Cmd+W + // looking dead for the full 15s RPC timeout; unanswered means ask, not close (#10142). + let decided = false + const decide = (act: () => void): void => { + if (decided) { + return + } + decided = true + act() + } + const confirmClose = (): void => + setPendingCloseConfirmation({ paneId, copyKind: getCloseDialogCopyKind(paneId) }) + const probeTimeout = setTimeout(() => decide(confirmClose), RUNNING_CLOSE_PROBE_TIMEOUT_MS) void inspectRuntimeTerminalProcess(settings, ptyId) .then((process) => { - if (!process.hasChildProcesses || settings?.skipCloseTerminalWithRunningProcessConfirm) { - executeClosePane(paneId) - } else { - setPendingCloseConfirmation({ paneId, copyKind: getCloseDialogCopyKind(paneId) }) - } + clearTimeout(probeTimeout) + decide(() => { + if ( + !process.hasChildProcesses || + settings?.skipCloseTerminalWithRunningProcessConfirm + ) { + executeClosePane(paneId) + } else { + confirmClose() + } + }) }) // Why: if the child-process probe rejects (wedged IPC, legacy provider), close anyway — Cmd+W doing nothing is worse than closing a pane with a child. - .catch(() => executeClosePane(paneId)) + .catch(() => { + clearTimeout(probeTimeout) + decide(() => executeClosePane(paneId)) + }) }, - [executeClosePane, tabId, worktreeId, getCloseDialogCopyKind] + [executeClosePane, getCloseDialogCopyKind] ) useImperativeHandle( 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 23a7475f5..bc230ecd4 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 @@ -1664,7 +1664,8 @@ export function useTerminalPaneLifecycle({ manager: mgr, getPtyIdForLeaf: (leafId) => useAppStore.getState().terminalLayoutsByTabId[tabId]?.ptyIdsByLeafId?.[leafId], - closeTab: () => closeTerminalTab(tabId), + // Why: CLI-driven pane close; its caller is answered immediately and cannot wait on a modal. + closeTab: () => closeTerminalTab(tabId, { skipRunningProcessConfirm: true }), closeTabPreservingPty: () => { const store = useAppStore.getState() if (detail.retireSurface && detail.leafId) { diff --git a/src/renderer/src/components/terminal/running-terminal-close-guard.test.ts b/src/renderer/src/components/terminal/running-terminal-close-guard.test.ts new file mode 100644 index 000000000..3dc808e6a --- /dev/null +++ b/src/renderer/src/components/terminal/running-terminal-close-guard.test.ts @@ -0,0 +1,409 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' + +const { getStateMock, inspectRuntimeTerminalProcessMock } = vi.hoisted(() => ({ + getStateMock: vi.fn(), + inspectRuntimeTerminalProcessMock: vi.fn() +})) + +vi.mock('@/store', () => ({ + useAppStore: { getState: getStateMock } +})) + +vi.mock('@/runtime/runtime-terminal-inspection', () => ({ + inspectRuntimeTerminalProcess: inspectRuntimeTerminalProcessMock +})) + +import { useRunningTerminalCloseConfirmStore } from '@/store/running-terminal-close-confirm' +import { + guardRunningTerminalClose, + shouldConfirmRunningTerminalClose, + RUNNING_CLOSE_PROBE_TIMEOUT_MS +} from './running-terminal-close-guard' + +const LEAF_A = '11111111-1111-4111-8111-111111111111' +const LEAF_B = '22222222-2222-4222-8222-222222222222' + +function setState(overrides: Record = {}): void { + getStateMock.mockReturnValue({ + settings: { activeRuntimeEnvironmentId: null }, + ptyIdsByTabId: { 'tab-1': ['pty-a'] }, + terminalLayoutsByTabId: { 'tab-1': { ptyIdsByLeafId: { [LEAF_A]: 'pty-a' } } }, + agentStatusByPaneKey: {}, + ...overrides + }) +} + +function guard(onClose = vi.fn(), onCancel?: () => void): void { + guardRunningTerminalClose({ + terminalTabId: 'tab-1', + tabLabel: 'npm run dev', + onClose, + ...(onCancel ? { onCancel } : {}) + }) +} + +function visibleRequest() { + return useRunningTerminalCloseConfirmStore.getState().runningTerminalCloseConfirm +} + +async function settleProbe(): Promise { + await Promise.resolve() + await Promise.resolve() + await Promise.resolve() +} + +describe('shouldConfirmRunningTerminalClose', () => { + it('confirms a plain interactive close', () => { + expect(shouldConfirmRunningTerminalClose(undefined)).toBe(true) + expect(shouldConfirmRunningTerminalClose({})).toBe(true) + expect(shouldConfirmRunningTerminalClose({ reason: 'user' })).toBe(true) + expect(shouldConfirmRunningTerminalClose({ hostCloseReason: 'user' })).toBe(true) + }) + + it.each([ + ['post-confirmation re-entry', { force: true }], + ['non-interactive CLI reject', { rejectPinned: true }], + ['bulk/CLI opt-out', { skipRunningProcessConfirm: true }], + ['pty lifecycle echo', { reason: 'pty-exit' as const }], + ['cleanup teardown', { reason: 'cleanup' as const }], + ['host-only pty lifecycle echo', { hostCloseReason: 'pty-exit' as const }], + ['pty-scoped lifecycle close', { lifecyclePtyId: 'pty-a' }] + ])('never prompts for a %s', (_label, options) => { + expect(shouldConfirmRunningTerminalClose(options)).toBe(false) + }) +}) + +describe('guardRunningTerminalClose', () => { + beforeEach(() => { + vi.clearAllMocks() + setState() + inspectRuntimeTerminalProcessMock.mockResolvedValue({ + foregroundProcess: 'sleep', + hasChildProcesses: true + }) + }) + + afterEach(() => { + const store = useRunningTerminalCloseConfirmStore.getState() + while (visibleRequest()) { + store.dismissRunningTerminalClose() + } + }) + + it('closes synchronously and never probes when the tab has no live pty', () => { + setState({ ptyIdsByTabId: {}, terminalLayoutsByTabId: {} }) + const onClose = vi.fn() + + guard(onClose) + + expect(onClose).toHaveBeenCalledTimes(1) + expect(inspectRuntimeTerminalProcessMock).not.toHaveBeenCalled() + }) + + // Why: a mounting pane is bound into the layout before ptyIdsByTabId catches up. Reading + // only the liveness map would let a close slip through that window with no prompt. + it('prompts for a pane the layout has bound but the liveness map has not', async () => { + setState({ ptyIdsByTabId: { 'tab-1': [] } }) + const onClose = vi.fn() + + guard(onClose) + await settleProbe() + + expect(inspectRuntimeTerminalProcessMock).toHaveBeenCalledWith(expect.anything(), 'pty-a') + expect(onClose).not.toHaveBeenCalled() + expect(visibleRequest()).toMatchObject({ terminalTabId: 'tab-1' }) + }) + + it('probes each pty once when the map and the layout name the same one', async () => { + guard() + await settleProbe() + + expect(inspectRuntimeTerminalProcessMock).toHaveBeenCalledTimes(1) + }) + + it('closes without probing when the user turned the prompt off', () => { + setState({ + settings: { + activeRuntimeEnvironmentId: null, + skipCloseTerminalWithRunningProcessConfirm: true + } + }) + const onClose = vi.fn() + + guard(onClose) + + expect(onClose).toHaveBeenCalledTimes(1) + expect(inspectRuntimeTerminalProcessMock).not.toHaveBeenCalled() + expect(visibleRequest()).toBeNull() + }) + + it('closes an idle terminal without a prompt', async () => { + inspectRuntimeTerminalProcessMock.mockResolvedValue({ + foregroundProcess: 'zsh', + hasChildProcesses: false + }) + const onClose = vi.fn() + + guard(onClose) + await settleProbe() + + expect(onClose).toHaveBeenCalledTimes(1) + expect(visibleRequest()).toBeNull() + }) + + it('defers a busy terminal behind a confirmation that carries the tab label', async () => { + const onClose = vi.fn() + + guard(onClose) + expect(onClose).not.toHaveBeenCalled() + await settleProbe() + + expect(onClose).not.toHaveBeenCalled() + expect(visibleRequest()).toMatchObject({ + terminalTabId: 'tab-1', + tabLabel: 'npm run dev', + copyKind: 'command' + }) + + useRunningTerminalCloseConfirmStore.getState().confirmRunningTerminalClose() + expect(onClose).toHaveBeenCalledTimes(1) + }) + + it('runs onCancel and keeps the tab when the confirmation is dismissed', async () => { + const onClose = vi.fn() + const onCancel = vi.fn() + + guard(onClose, onCancel) + await settleProbe() + useRunningTerminalCloseConfirmStore.getState().dismissRunningTerminalClose() + + expect(onCancel).toHaveBeenCalledTimes(1) + expect(onClose).not.toHaveBeenCalled() + }) + + it('fails open and closes when the probe rejects (wedged relay / legacy provider)', async () => { + inspectRuntimeTerminalProcessMock.mockRejectedValue(new Error('rpc_timeout')) + const onClose = vi.fn() + + guard(onClose) + await settleProbe() + + expect(onClose).toHaveBeenCalledTimes(1) + expect(visibleRequest()).toBeNull() + }) + + it('fails open when a remote handle reports the inspection as unavailable', async () => { + inspectRuntimeTerminalProcessMock.mockResolvedValue({ + foregroundProcess: null, + hasChildProcesses: true, + unavailable: true + }) + const onClose = vi.fn() + + guard(onClose) + await settleProbe() + + expect(onClose).toHaveBeenCalledTimes(1) + expect(visibleRequest()).toBeNull() + }) + + it('prompts once for a split tab where only the second pane is busy', async () => { + setState({ + ptyIdsByTabId: { 'tab-1': ['pty-a', 'pty-b'] }, + terminalLayoutsByTabId: { + 'tab-1': { ptyIdsByLeafId: { [LEAF_A]: 'pty-a', [LEAF_B]: 'pty-b' } } + }, + agentStatusByPaneKey: { [`tab-1:${LEAF_B}`]: { agentType: 'claude' } } + }) + inspectRuntimeTerminalProcessMock.mockImplementation(async (_settings, ptyId: string) => ({ + foregroundProcess: ptyId === 'pty-b' ? 'claude' : 'zsh', + hasChildProcesses: ptyId === 'pty-b' + })) + const onClose = vi.fn() + + guard(onClose) + await settleProbe() + + expect(inspectRuntimeTerminalProcessMock).toHaveBeenCalledTimes(2) + expect(visibleRequest()).toMatchObject({ terminalTabId: 'tab-1', copyKind: 'agent' }) + expect(onClose).not.toHaveBeenCalled() + }) + + it('does not use an idle sibling pane to pick the agent copy', async () => { + setState({ + ptyIdsByTabId: { 'tab-1': ['pty-a', 'pty-b'] }, + terminalLayoutsByTabId: { + 'tab-1': { ptyIdsByLeafId: { [LEAF_A]: 'pty-a', [LEAF_B]: 'pty-b' } } + }, + agentStatusByPaneKey: { [`tab-1:${LEAF_B}`]: { agentType: 'claude' } } + }) + inspectRuntimeTerminalProcessMock.mockImplementation(async (_settings, ptyId: string) => ({ + foregroundProcess: ptyId === 'pty-a' ? 'npm' : 'zsh', + hasChildProcesses: ptyId === 'pty-a' + })) + + guard() + await settleProbe() + + expect(visibleRequest()?.copyKind).toBe('command') + }) + + it('prefers the agent copy regardless of pty spawn order when both panes are busy', async () => { + setState({ + ptyIdsByTabId: { 'tab-1': ['pty-a', 'pty-b'] }, + terminalLayoutsByTabId: { + 'tab-1': { ptyIdsByLeafId: { [LEAF_A]: 'pty-a', [LEAF_B]: 'pty-b' } } + }, + agentStatusByPaneKey: { [`tab-1:${LEAF_B}`]: { agentType: 'codex' } } + }) + + guard() + await settleProbe() + + expect(visibleRequest()?.copyKind).toBe('agent') + }) + + it('ignores a pane whose agent status is unknown', async () => { + setState({ agentStatusByPaneKey: { [`tab-1:${LEAF_A}`]: { agentType: 'unknown' } } }) + + guard() + await settleProbe() + + expect(visibleRequest()?.copyKind).toBe('command') + }) + + it('survives legacy layout leaf ids that are not stable pane uuids', async () => { + setState({ terminalLayoutsByTabId: { 'tab-1': { ptyIdsByLeafId: { leaf: 'pty-a' } } } }) + const onClose = vi.fn() + + guard(onClose) + await settleProbe() + + expect(visibleRequest()).toMatchObject({ copyKind: 'command' }) + expect(onClose).not.toHaveBeenCalled() + }) + + // Why: makePaneKey throws on a tab id containing ':'; the dialog must never be the + // reason a close silently stops happening. + it('closes rather than wedging when the copy-kind lookup throws', async () => { + getStateMock.mockReturnValue({ + settings: { activeRuntimeEnvironmentId: null }, + ptyIdsByTabId: { 'tab:1': ['pty-a'] }, + terminalLayoutsByTabId: { 'tab:1': { ptyIdsByLeafId: { [LEAF_A]: 'pty-a' } } }, + agentStatusByPaneKey: {} + }) + const onClose = vi.fn() + + guardRunningTerminalClose({ terminalTabId: 'tab:1', tabLabel: 'weird', onClose }) + await settleProbe() + + expect(visibleRequest()).toMatchObject({ terminalTabId: 'tab:1', copyKind: 'command' }) + expect(onClose).not.toHaveBeenCalled() + }) + + it('closes rather than wedging when raising the confirmation throws', async () => { + const requestSpy = vi + .spyOn(useRunningTerminalCloseConfirmStore.getState(), 'requestRunningTerminalCloseConfirm') + .mockImplementation(() => { + throw new Error('subscriber blew up') + }) + const onClose = vi.fn() + + guard(onClose) + await settleProbe() + requestSpy.mockRestore() + + expect(onClose).toHaveBeenCalledTimes(1) + }) + + // Why: a remote inspect can take its full 15s RPC timeout. Closing at 4s would kill a + // running remote command with no prompt — the exact failure this guard exists to stop. + it('prompts instead of closing when a wedged remote probe never settles', async () => { + vi.useFakeTimers() + inspectRuntimeTerminalProcessMock.mockReturnValue(new Promise(() => {})) + const onClose = vi.fn() + + guard(onClose) + expect(onClose).not.toHaveBeenCalled() + + vi.advanceTimersByTime(RUNNING_CLOSE_PROBE_TIMEOUT_MS) + vi.useRealTimers() + + expect(onClose).not.toHaveBeenCalled() + expect(visibleRequest()).toMatchObject({ terminalTabId: 'tab-1', tabLabel: 'npm run dev' }) + + useRunningTerminalCloseConfirmStore.getState().confirmRunningTerminalClose() + expect(onClose).toHaveBeenCalledTimes(1) + }) + + it('treats every pane as a candidate when picking the copy for a timed-out probe', async () => { + setState({ + ptyIdsByTabId: { 'tab-1': ['pty-a', 'pty-b'] }, + terminalLayoutsByTabId: { + 'tab-1': { ptyIdsByLeafId: { [LEAF_A]: 'pty-a', [LEAF_B]: 'pty-b' } } + }, + agentStatusByPaneKey: { [`tab-1:${LEAF_B}`]: { agentType: 'claude' } } + }) + vi.useFakeTimers() + inspectRuntimeTerminalProcessMock.mockReturnValue(new Promise(() => {})) + + guard() + vi.advanceTimersByTime(RUNNING_CLOSE_PROBE_TIMEOUT_MS) + vi.useRealTimers() + + expect(visibleRequest()?.copyKind).toBe('agent') + }) + + it('closes rather than wedging when the timed-out prompt throws', async () => { + const requestSpy = vi + .spyOn(useRunningTerminalCloseConfirmStore.getState(), 'requestRunningTerminalCloseConfirm') + .mockImplementation(() => { + throw new Error('subscriber blew up') + }) + vi.useFakeTimers() + inspectRuntimeTerminalProcessMock.mockReturnValue(new Promise(() => {})) + const onClose = vi.fn() + + guard(onClose) + vi.advanceTimersByTime(RUNNING_CLOSE_PROBE_TIMEOUT_MS) + vi.useRealTimers() + requestSpy.mockRestore() + + expect(onClose).toHaveBeenCalledTimes(1) + }) + + it('ignores a slow probe that resolves after the timeout already prompted', async () => { + vi.useFakeTimers() + inspectRuntimeTerminalProcessMock.mockResolvedValue({ + foregroundProcess: 'sleep', + hasChildProcesses: true + }) + const onClose = vi.fn() + + guard(onClose) + vi.advanceTimersByTime(RUNNING_CLOSE_PROBE_TIMEOUT_MS) + vi.useRealTimers() + await settleProbe() + + expect(onClose).not.toHaveBeenCalled() + useRunningTerminalCloseConfirmStore.getState().confirmRunningTerminalClose() + expect(onClose).toHaveBeenCalledTimes(1) + expect(visibleRequest()).toBeNull() + }) + + // Why: an SSH drop zeroes ptyIdsByTabId while the layout still names the pane. The stale + // binding is probed, that probe fails on the dead link, and the close falls open — so a + // reconnecting tab stays closable instead of being blocked behind a prompt for a pty + // nobody can reach. Documented so the behavior is a decision, not an accident. + it('closes a reconnecting ssh tab whose pty ids were already zeroed', async () => { + setState({ ptyIdsByTabId: { 'tab-1': [] } }) + inspectRuntimeTerminalProcessMock.mockRejectedValue(new Error('ssh_disconnected')) + const onClose = vi.fn() + + guard(onClose) + await settleProbe() + + expect(onClose).toHaveBeenCalledTimes(1) + expect(visibleRequest()).toBeNull() + }) +}) diff --git a/src/renderer/src/components/terminal/running-terminal-close-guard.ts b/src/renderer/src/components/terminal/running-terminal-close-guard.ts new file mode 100644 index 000000000..73f63fc85 --- /dev/null +++ b/src/renderer/src/components/terminal/running-terminal-close-guard.ts @@ -0,0 +1,154 @@ +import { useAppStore } from '@/store' +import { inspectRuntimeTerminalProcess } from '@/runtime/runtime-terminal-inspection' +import { useRunningTerminalCloseConfirmStore } from '@/store/running-terminal-close-confirm' +import type { TerminalTabCloseReason } from '@/store/slices/terminal-tab-retirement' +import type { AppState } from '@/store/types' +import { resolveBusyPtyCloseCopyKind } from './terminal-close-copy-kind' + +export type RunningTerminalCloseGuardOptions = { + force?: boolean + rejectPinned?: boolean + reason?: TerminalTabCloseReason + hostCloseReason?: TerminalTabCloseReason + lifecyclePtyId?: string + skipRunningProcessConfirm?: boolean +} + +/** Upper bound on how long a close may wait on the probe before it asks instead. A remote + * inspect RPC can hang for its full 15s timeout, and an X button that looks dead for 15s + * is the same class of bug as one that never asks — but an unanswered probe is not + * evidence of an idle shell, so the timeout raises the prompt rather than killing a + * possibly-running remote command (#10142). */ +export const RUNNING_CLOSE_PROBE_TIMEOUT_MS = 4_000 + +/** Whether this close is an interactive user action that should stop and ask before + * killing a live child process. Lifecycle echoes, bulk closes, CLI/RPC closes and the + * post-confirmation re-entry are all excluded. */ +export function shouldConfirmRunningTerminalClose( + options?: RunningTerminalCloseGuardOptions +): boolean { + if (options?.force === true || options?.rejectPinned === true) { + return false + } + if (options?.skipRunningProcessConfirm === true || options?.lifecyclePtyId !== undefined) { + return false + } + const isUserReason = (reason: TerminalTabCloseReason | undefined): boolean => + reason === undefined || reason === 'user' + return isUserReason(options?.reason) && isUserReason(options?.hostCloseReason) +} + +/** Every PTY the tab could still own. `ptyIdsByTabId` is the liveness map the rest of the + * app reads, but a mounting pane is bound into the layout before the map catches up, and + * the store's own teardown collector unions both for exactly that reason — reading only + * the map would let a close slip through the window with no prompt. A stale id costs + * nothing: its probe fails and the guard falls open. */ +function collectTabPtyIds( + state: Pick, + terminalTabId: string +): string[] { + const ptyIds = new Set() + for (const ptyId of state.ptyIdsByTabId?.[terminalTabId] ?? []) { + if (ptyId) { + ptyIds.add(ptyId) + } + } + const ptyIdsByLeafId = state.terminalLayoutsByTabId?.[terminalTabId]?.ptyIdsByLeafId ?? {} + for (const ptyId of Object.values(ptyIdsByLeafId)) { + if (typeof ptyId === 'string' && ptyId) { + ptyIds.add(ptyId) + } + } + return [...ptyIds] +} + +/** + * Routes an interactive terminal-tab close through the running-process confirmation. + * Closes immediately when nothing is running, so idle tabs keep today's behavior. + */ +export function guardRunningTerminalClose(params: { + terminalTabId: string + tabLabel: string + onClose: () => void + onCancel?: () => void +}): void { + const { terminalTabId, tabLabel, onClose, onCancel } = params + const state = useAppStore.getState() + const settings = state.settings + const ptyIds = collectTabPtyIds(state, terminalTabId) + // Why: no PTY at all means there is nothing to probe (parked/hibernated tab, or a + // teardown that already cleared both maps), and the opt-out setting means the answer is + // already known. Both keep the close fully synchronous. + if (ptyIds.length === 0 || settings?.skipCloseTerminalWithRunningProcessConfirm === true) { + onClose() + return + } + + // Why: the timeout, the probe result and the error path race to decide this close, so the + // first one to land owns it instead of trusting those races to stay mutually exclusive. + let decided = false + const closeNow = (): void => { + if (decided) { + return + } + decided = true + onClose() + } + const confirmClose = (busyPtyIds: readonly string[]): void => { + if (decided) { + return + } + const copyKind = resolveBusyPtyCloseCopyKind(terminalTabId, busyPtyIds) + useRunningTerminalCloseConfirmStore.getState().requestRunningTerminalCloseConfirm({ + terminalTabId, + tabLabel, + copyKind, + onConfirm: onClose, + ...(onCancel ? { onCancel } : {}) + }) + // Why: only once the prompt is actually up — if either call above throws, the close must + // still be free to fall through and happen. + decided = true + } + + const probeTimeout = setTimeout(() => { + try { + // Why: a probe that has not answered yet is unknown, not idle. Ask, treating every pty + // as a candidate, so a degraded relay costs a click instead of a killed remote command. + confirmClose(ptyIds) + } catch { + closeNow() + } + }, RUNNING_CLOSE_PROBE_TIMEOUT_MS) + + void Promise.allSettled(ptyIds.map((ptyId) => inspectRuntimeTerminalProcess(settings, ptyId))) + .then((results) => { + clearTimeout(probeTimeout) + if (decided) { + return + } + // Why: fail open on an *answered* probe, matching the Cmd+W pane path — a rejection + // (wedged relay, legacy provider) or a stale remote handle is not evidence of a live + // child, and a close button that silently does nothing is worse than closing a busy tab. + const busyPtyIds = ptyIds.filter((_, index) => { + const result = results[index] + return ( + result?.status === 'fulfilled' && + result.value.hasChildProcesses && + result.value.unavailable !== true + ) + }) + if (busyPtyIds.length === 0) { + closeNow() + return + } + confirmClose(busyPtyIds) + }) + // Why: allSettled never rejects, so this only fires when the decision above throws (a + // copy-kind lookup, a store subscriber). Without it the tab would silently never close + // and the user would get no feedback at all; the pane path it replaced had this catch. + .catch(() => { + clearTimeout(probeTimeout) + closeNow() + }) +} diff --git a/src/renderer/src/components/terminal/terminal-close-confirm-keyboard-vs-mouse.test.ts b/src/renderer/src/components/terminal/terminal-close-confirm-keyboard-vs-mouse.test.ts new file mode 100644 index 000000000..3ad4229e2 --- /dev/null +++ b/src/renderer/src/components/terminal/terminal-close-confirm-keyboard-vs-mouse.test.ts @@ -0,0 +1,131 @@ +/** + * Regression for #10142: the tab X button / middle-click used to bypass the + * running-process close confirmation that Cmd+W enforces. Assertions unchanged + * from the adjudicated repro. + * + * Mouse close path: SortableTab (X onClick / onAuxClick button===1) -> onClose + * -> Terminal.tsx handleCloseTab -> closeTerminalTab() -> running-process guard. + * Keyboard path: Cmd+W -> TerminalPane.handleRequestClosePane -> inspectRuntimeTerminalProcess + * (split panes) or closeTerminalTab's guard (last pane) -> CloseTerminalDialog. + */ +import { readFileSync } from 'node:fs' +import { join } from 'node:path' +import { beforeEach, describe, expect, it, vi } from 'vitest' + +const { + requestPinnedTabCloseConfirmMock, + getStateMock, + inspectRuntimeTerminalProcessMock, + isWebRuntimeSessionActiveMock, + resolveHostSessionTabIdForWebSessionTabMock +} = vi.hoisted(() => ({ + requestPinnedTabCloseConfirmMock: vi.fn(), + getStateMock: vi.fn(), + inspectRuntimeTerminalProcessMock: vi.fn(), + isWebRuntimeSessionActiveMock: vi.fn(() => false), + resolveHostSessionTabIdForWebSessionTabMock: vi.fn<() => string | null>(() => null) +})) + +vi.mock('@/store', () => ({ + useAppStore: { getState: getStateMock } +})) + +vi.mock('@/runtime/web-runtime-session', () => ({ + activateWebRuntimeSessionTab: vi.fn(), + closeWebRuntimeSessionTab: vi.fn(), + createWebRuntimeSessionTerminal: vi.fn(), + isWebRuntimeSessionActive: isWebRuntimeSessionActiveMock, + isWebTerminalSurfaceTabId: vi.fn(() => false), + toHostSessionTabId: vi.fn((tabId: string) => tabId) +})) + +vi.mock('@/runtime/web-session-tabs-sync', () => ({ + getLatestWebSessionTabsPublicationEpoch: vi.fn(() => 'epoch-1'), + resolveHostSessionTabIdForWebSessionTab: resolveHostSessionTabIdForWebSessionTabMock +})) + +vi.mock('@/runtime/runtime-terminal-inspection', () => ({ + inspectRuntimeTerminalProcess: inspectRuntimeTerminalProcessMock +})) + +import { closeTerminalTab } from './terminal-tab-actions' + +// A non-pinned terminal tab whose PTY has a live child process (e.g. `sleep 300`). +function stateWithBusyTerminalTab(closeTab: () => void): Record { + return { + settings: { activeRuntimeEnvironmentId: null, confirmClosePinnedTab: true }, + tabsByWorktree: { 'wt-1': [{ id: 'tab-busy' }, { id: 'tab-other' }] }, + unifiedTabsByWorktree: { + 'wt-1': [ + { id: 'tab-busy', entityId: 'tab-busy', contentType: 'terminal', isPinned: false }, + { id: 'tab-other', entityId: 'tab-other', contentType: 'terminal', isPinned: false } + ] + }, + ptyIdsByTabId: { 'tab-busy': ['pty-busy'] }, + terminalLayoutsByTabId: { 'tab-busy': { ptyIdsByLeafId: { leaf: 'pty-busy' } } }, + agentStatusByPaneKey: {}, + openFiles: [], + browserTabsByWorktree: {}, + activeWorktreeId: 'wt-1', + activeTabId: 'tab-busy', + closeTab, + requestPinnedTabCloseConfirm: requestPinnedTabCloseConfirmMock, + setActiveTab: vi.fn(), + setActiveFile: vi.fn(), + setActiveTabType: vi.fn(), + setActiveBrowserTab: vi.fn(), + setActiveWorktree: vi.fn() + } +} + +describe('#10142 close confirmation policy is the same for keyboard and mouse', () => { + const closeTab = vi.fn() + + beforeEach(() => { + vi.clearAllMocks() + getStateMock.mockReturnValue(stateWithBusyTerminalTab(closeTab)) + isWebRuntimeSessionActiveMock.mockReturnValue(false) + resolveHostSessionTabIdForWebSessionTabMock.mockReturnValue(null) + inspectRuntimeTerminalProcessMock.mockResolvedValue({ + foregroundProcess: 'sleep', + hasChildProcesses: true + }) + }) + + // Control: the keyboard entry point does probe for running children. + it('keyboard Cmd+W path probes for running child processes before closing', () => { + const source = readFileSync(join(__dirname, '../terminal-pane/TerminalPane.tsx'), 'utf8') + const handler = source.slice(source.indexOf('const handleRequestClosePane')) + expect(handler.slice(0, handler.indexOf('useImperativeHandle'))).toContain( + 'inspectRuntimeTerminalProcess' + ) + }) + + // Control: the harness does observe a guard when one exists — pinning blocks the same mouse close. + it('mouse close routes a pinned tab through its confirmation guard', () => { + const state = stateWithBusyTerminalTab(closeTab) + ;(state.unifiedTabsByWorktree as Record)[ + 'wt-1' + ]![0]!.isPinned = true + getStateMock.mockReturnValue(state) + + closeTerminalTab('tab-busy') + + expect(requestPinnedTabCloseConfirmMock).toHaveBeenCalled() + expect(closeTab).not.toHaveBeenCalled() + }) + + it('mouse close (X button / middle-click) consults the running-process probe', async () => { + closeTerminalTab('tab-busy') + await Promise.resolve() + + expect(inspectRuntimeTerminalProcessMock).toHaveBeenCalled() + }) + + it('mouse close (X button / middle-click) does not drop a busy tab without confirmation', async () => { + closeTerminalTab('tab-busy') + await Promise.resolve() + + expect(closeTab).not.toHaveBeenCalled() + }) +}) diff --git a/src/renderer/src/components/terminal/terminal-close-copy-kind.test.ts b/src/renderer/src/components/terminal/terminal-close-copy-kind.test.ts new file mode 100644 index 000000000..960eaaa8c --- /dev/null +++ b/src/renderer/src/components/terminal/terminal-close-copy-kind.test.ts @@ -0,0 +1,67 @@ +/** + * The keyboard (pane-scoped) and tab-strip (pty-scoped) close prompts must word the same + * close the same way, so both resolve their copy through this module (#10142). + */ +import { beforeEach, describe, expect, it, vi } from 'vitest' + +const { getStateMock } = vi.hoisted(() => ({ getStateMock: vi.fn() })) + +vi.mock('@/store', () => ({ useAppStore: { getState: getStateMock } })) + +import { resolveBusyPtyCloseCopyKind, resolveLeafCloseCopyKind } from './terminal-close-copy-kind' + +const LEAF_A = '11111111-1111-4111-8111-111111111111' +const LEAF_B = '22222222-2222-4222-8222-222222222222' + +function setState(overrides: Record = {}): void { + getStateMock.mockReturnValue({ + terminalLayoutsByTabId: { + 'tab-1': { ptyIdsByLeafId: { [LEAF_A]: 'pty-a', [LEAF_B]: 'pty-b' } } + }, + agentStatusByPaneKey: { [`tab-1:${LEAF_B}`]: { agentType: 'claude' } }, + ...overrides + }) +} + +describe('terminal close copy kind', () => { + beforeEach(() => { + vi.clearAllMocks() + setState() + }) + + it('agrees between the pane lookup and the busy-pty lookup for the same pane', () => { + expect(resolveLeafCloseCopyKind('tab-1', LEAF_B)).toBe('agent') + expect(resolveBusyPtyCloseCopyKind('tab-1', ['pty-b'])).toBe('agent') + + expect(resolveLeafCloseCopyKind('tab-1', LEAF_A)).toBe('command') + expect(resolveBusyPtyCloseCopyKind('tab-1', ['pty-a'])).toBe('command') + }) + + it('prefers the agent copy when a split has both busy', () => { + expect(resolveBusyPtyCloseCopyKind('tab-1', ['pty-a', 'pty-b'])).toBe('agent') + }) + + it.each([ + ['a missing leaf id', undefined], + ['a null leaf id', null], + ['a legacy non-uuid leaf id', 'leaf'] + ])('falls back to the command copy for %s', (_label, leafId) => { + expect(resolveLeafCloseCopyKind('tab-1', leafId)).toBe('command') + }) + + it('never throws on a tab id that makePaneKey would reject', () => { + setState({ + terminalLayoutsByTabId: { 'tab:1': { ptyIdsByLeafId: { [LEAF_B]: 'pty-b' } } } + }) + + expect(resolveLeafCloseCopyKind('tab:1', LEAF_B)).toBe('command') + expect(resolveBusyPtyCloseCopyKind('tab:1', ['pty-b'])).toBe('command') + }) + + it('ignores an unknown agent type', () => { + setState({ agentStatusByPaneKey: { [`tab-1:${LEAF_B}`]: { agentType: 'unknown' } } }) + + expect(resolveLeafCloseCopyKind('tab-1', LEAF_B)).toBe('command') + expect(resolveBusyPtyCloseCopyKind('tab-1', ['pty-b'])).toBe('command') + }) +}) diff --git a/src/renderer/src/components/terminal/terminal-close-copy-kind.ts b/src/renderer/src/components/terminal/terminal-close-copy-kind.ts new file mode 100644 index 000000000..e012987ba --- /dev/null +++ b/src/renderer/src/components/terminal/terminal-close-copy-kind.ts @@ -0,0 +1,38 @@ +import { useAppStore } from '@/store' +import type { CloseTerminalDialogCopyKind } from '../terminal-pane/CloseTerminalDialog' +import { isTerminalLeafId, makePaneKey } from '../../../../shared/stable-pane-id' + +/** + * Single source of truth for "is this pane an agent?" in the close confirmation, so the + * keyboard (pane-scoped) and tab-strip (pty-scoped) prompts cannot word the same close + * differently. Each caller resolves its own leaf id; only the policy is shared. + */ +export function resolveLeafCloseCopyKind( + tabId: string, + leafId: string | null | undefined +): CloseTerminalDialogCopyKind { + // Why: legacy layouts and mid-attach panes carry non-UUID or missing leaf ids, and + // makePaneKey throws on those — a dialog must never be the thing that breaks a close. + if (!leafId || !isTerminalLeafId(leafId) || !tabId || tabId.includes(':')) { + return 'command' + } + const agentStatusByPaneKey = useAppStore.getState().agentStatusByPaneKey ?? {} + const agentType = agentStatusByPaneKey[makePaneKey(tabId, leafId)]?.agentType + return agentType && agentType !== 'unknown' ? 'agent' : 'command' +} + +/** Copy for a whole-tab close, given the PTYs that reported a live child. Agent panes win + * in a mixed split: stopping an agent mid-task is the costlier surprise. */ +export function resolveBusyPtyCloseCopyKind( + tabId: string, + busyPtyIds: readonly string[] +): CloseTerminalDialogCopyKind { + const ptyIdsByLeafId = + useAppStore.getState().terminalLayoutsByTabId?.[tabId]?.ptyIdsByLeafId ?? {} + for (const [leafId, ptyId] of Object.entries(ptyIdsByLeafId)) { + if (busyPtyIds.includes(ptyId) && resolveLeafCloseCopyKind(tabId, leafId) === 'agent') { + return 'agent' + } + } + return 'command' +} diff --git a/src/renderer/src/components/terminal/terminal-tab-actions.ts b/src/renderer/src/components/terminal/terminal-tab-actions.ts index 3dd2762f7..55934e7ae 100644 --- a/src/renderer/src/components/terminal/terminal-tab-actions.ts +++ b/src/renderer/src/components/terminal/terminal-tab-actions.ts @@ -9,11 +9,20 @@ import { resolveHostSessionTabIdForWebSessionTab } from '@/runtime/web-session-tabs-sync' import { resolveTerminalWorktreeRoute } from '@/lib/terminal-worktree-route' -import { guardPinnedTabClose, resolvePinnedTabLabel } from '@/store/pinned-tab-close-guard' +import { + guardPinnedTabClose, + isUnifiedTabPinned, + resolvePinnedTabLabel, + shouldConfirmPinnedTabClose +} from '@/store/pinned-tab-close-guard' import type { TerminalTabCloseReason, TerminalTabRetirementPlan } from '@/store/slices/terminal-tab-retirement' +import { + guardRunningTerminalClose, + shouldConfirmRunningTerminalClose +} from './running-terminal-close-guard' import { closeLocalTerminalTabState } from './close-local-terminal-tab-state' import { getTerminalIncarnationHandle } from './terminal-close-incarnation' import { @@ -25,20 +34,6 @@ import { export type { PrecomputedTerminalCloseState } from './terminal-close-target' export { closeOtherTerminalTabs, closeTerminalTabsToRight } from './terminal-tab-bulk-actions' -type TerminalTabActionState = ReturnType - -function isPinnedVisibleTab( - state: TerminalTabActionState, - worktreeId: string, - visibleId: string -): boolean { - return ( - (state.unifiedTabsByWorktree?.[worktreeId] ?? []).some( - (tab) => (tab.id === visibleId || tab.entityId === visibleId) && tab.isPinned - ) ?? false - ) -} - export function closeTerminalTab( tabId: string, options?: { @@ -51,6 +46,9 @@ export function closeTerminalTab( hostCloseReason?: TerminalTabCloseReason /** PTY whose lifecycle event initiated the host close. */ lifecyclePtyId?: string + /** Set by callers that must never raise a modal (bulk closes, CLI/RPC, lifecycle + * fallbacks) and by the re-entry that runs once the user confirmed. */ + skipRunningProcessConfirm?: boolean captureRecentlyClosed?: boolean localPtyTeardownOwnedExternally?: boolean precomputedRetirementPlan?: TerminalTabRetirementPlan @@ -82,7 +80,7 @@ export function closeTerminalTab( if ( options?.reason !== 'pty-exit' && !options?.force && - isPinnedVisibleTab(state, owningWorktreeId, terminalTabId) + isUnifiedTabPinned(state, owningWorktreeId, terminalTabId) ) { // Why: background lifecycle callers cannot safely wait on a modal whose // owner may be unattended; reject pinned tabs without bypassing the guard. @@ -90,10 +88,30 @@ export function closeTerminalTab( options.onCancel?.() return } - guardPinnedTabClose({ - isPinned: true, + // Why: the pin prompt supersedes the running-process one only when it actually + // appears. With `confirmClosePinnedTab` off it says nothing, so fall through and let + // a busy pinned tab still get asked — Cmd+W did exactly that before #10142. + if (shouldConfirmPinnedTabClose(state)) { + guardPinnedTabClose({ + isPinned: true, + tabLabel: resolvePinnedTabLabel(state, owningWorktreeId, terminalTabId), + onClose: () => closeTerminalTab(tabId, { ...options, force: true }), + ...(options?.onCancel ? { onCancel: options.onCancel } : {}) + }) + return + } + } + + // Why: the X button, middle-click and the tab menu used to skip the running-process + // prompt that Cmd+W enforced (#10142). Guarding here — above the web-runtime branch so + // host-backed tabs are covered too — gives every close path one shared policy. + if (shouldConfirmRunningTerminalClose(options)) { + guardRunningTerminalClose({ + terminalTabId, tabLabel: resolvePinnedTabLabel(state, owningWorktreeId, terminalTabId), - onClose: () => closeTerminalTab(tabId, { ...options, force: true }), + // Why: re-enter instead of continuing inline so pinned/route/precomputed state is + // re-validated against fresh state after an arbitrarily long dialog. + onClose: () => closeTerminalTab(tabId, { ...options, skipRunningProcessConfirm: true }), ...(options?.onCancel ? { onCancel: options.onCancel } : {}) }) return diff --git a/src/renderer/src/components/terminal/terminal-tab-close-running-confirm.test.ts b/src/renderer/src/components/terminal/terminal-tab-close-running-confirm.test.ts new file mode 100644 index 000000000..066c5795e --- /dev/null +++ b/src/renderer/src/components/terminal/terminal-tab-close-running-confirm.test.ts @@ -0,0 +1,263 @@ +/** + * Which `closeTerminalTab` callers reach the running-process confirmation (#10142). + * Kept out of terminal-tab-actions.test.ts, which is already at its max-lines budget. + */ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' + +const { + closeWebRuntimeSessionTabMock, + getStateMock, + inspectRuntimeTerminalProcessMock, + isWebRuntimeSessionActiveMock, + requestPinnedTabCloseConfirmMock, + resolveHostSessionTabIdForWebSessionTabMock +} = vi.hoisted(() => ({ + closeWebRuntimeSessionTabMock: vi.fn(), + getStateMock: vi.fn(), + inspectRuntimeTerminalProcessMock: vi.fn(), + isWebRuntimeSessionActiveMock: vi.fn(() => false), + requestPinnedTabCloseConfirmMock: vi.fn(), + resolveHostSessionTabIdForWebSessionTabMock: vi.fn<() => string | null>(() => null) +})) + +vi.mock('@/store', () => ({ + useAppStore: { getState: getStateMock } +})) + +vi.mock('@/runtime/web-runtime-session', () => ({ + activateWebRuntimeSessionTab: vi.fn(), + closeWebRuntimeSessionTab: closeWebRuntimeSessionTabMock, + createWebRuntimeSessionTerminal: vi.fn(), + isWebRuntimeSessionActive: isWebRuntimeSessionActiveMock, + isWebTerminalSurfaceTabId: vi.fn(() => false), + toHostSessionTabId: vi.fn((tabId: string) => tabId) +})) + +vi.mock('@/runtime/web-session-tabs-sync', () => ({ + getLatestWebSessionTabsPublicationEpoch: vi.fn(() => 'epoch-1'), + resolveHostSessionTabIdForWebSessionTab: resolveHostSessionTabIdForWebSessionTabMock +})) + +vi.mock('@/runtime/runtime-terminal-inspection', () => ({ + inspectRuntimeTerminalProcess: inspectRuntimeTerminalProcessMock +})) + +import { useRunningTerminalCloseConfirmStore } from '@/store/running-terminal-close-confirm' +import { closeTerminalTab } from './terminal-tab-actions' + +const LEAF = '33333333-3333-4333-8333-333333333333' + +const closeTab = vi.fn() + +function busyTabState(overrides: Record = {}): Record { + return { + settings: { activeRuntimeEnvironmentId: null, confirmClosePinnedTab: true }, + tabsByWorktree: { 'wt-1': [{ id: 'tab-busy' }, { id: 'tab-other' }] }, + unifiedTabsByWorktree: { + 'wt-1': [ + { + id: 'tab-busy', + entityId: 'tab-busy', + contentType: 'terminal', + isPinned: false, + label: 'dev server' + }, + { id: 'tab-other', entityId: 'tab-other', contentType: 'terminal', isPinned: false } + ] + }, + ptyIdsByTabId: { 'tab-busy': ['pty-busy'] }, + terminalLayoutsByTabId: { 'tab-busy': { ptyIdsByLeafId: { [LEAF]: 'pty-busy' } } }, + agentStatusByPaneKey: {}, + openFiles: [], + browserTabsByWorktree: {}, + activeWorktreeId: 'wt-1', + activeTabId: 'tab-busy', + closeTab, + requestPinnedTabCloseConfirm: requestPinnedTabCloseConfirmMock, + setActiveTab: vi.fn(), + setActiveFile: vi.fn(), + setActiveTabType: vi.fn(), + setActiveBrowserTab: vi.fn(), + setActiveWorktree: vi.fn(), + ...overrides + } +} + +function visibleRequest() { + return useRunningTerminalCloseConfirmStore.getState().runningTerminalCloseConfirm +} + +async function settleProbe(): Promise { + await Promise.resolve() + await Promise.resolve() + await Promise.resolve() +} + +describe('closeTerminalTab running-process confirmation', () => { + beforeEach(() => { + vi.clearAllMocks() + getStateMock.mockReturnValue(busyTabState()) + isWebRuntimeSessionActiveMock.mockReturnValue(false) + resolveHostSessionTabIdForWebSessionTabMock.mockReturnValue(null) + inspectRuntimeTerminalProcessMock.mockResolvedValue({ + foregroundProcess: 'npm', + hasChildProcesses: true + }) + }) + + afterEach(() => { + const store = useRunningTerminalCloseConfirmStore.getState() + while (visibleRequest()) { + store.dismissRunningTerminalClose() + } + }) + + it('closes a busy tab only after the confirmation is accepted', async () => { + closeTerminalTab('tab-busy') + await settleProbe() + + expect(closeTab).not.toHaveBeenCalled() + expect(visibleRequest()).toMatchObject({ + terminalTabId: 'tab-busy', + tabLabel: 'dev server', + copyKind: 'command' + }) + + useRunningTerminalCloseConfirmStore.getState().confirmRunningTerminalClose() + + expect(closeTab).toHaveBeenCalledWith('tab-busy') + }) + + it('keeps the tab and runs onCancel when the confirmation is dismissed', async () => { + const onCancel = vi.fn() + const onClosed = vi.fn() + + closeTerminalTab('tab-busy', { onCancel, onClosed }) + await settleProbe() + useRunningTerminalCloseConfirmStore.getState().dismissRunningTerminalClose() + + expect(onCancel).toHaveBeenCalledTimes(1) + expect(onClosed).not.toHaveBeenCalled() + expect(closeTab).not.toHaveBeenCalled() + }) + + it('still reports the close through onClosed after the confirmation', async () => { + const onClosed = vi.fn() + + closeTerminalTab('tab-busy', { onClosed }) + await settleProbe() + useRunningTerminalCloseConfirmStore.getState().confirmRunningTerminalClose() + + expect(onClosed).toHaveBeenCalledTimes(1) + }) + + it('stays fully synchronous for a tab with no live pty', () => { + getStateMock.mockReturnValue(busyTabState({ ptyIdsByTabId: {}, terminalLayoutsByTabId: {} })) + + closeTerminalTab('tab-busy') + + expect(inspectRuntimeTerminalProcessMock).not.toHaveBeenCalled() + expect(closeTab).toHaveBeenCalledWith('tab-busy') + }) + + it.each([ + ['force (post-confirmation re-entry)', { force: true }], + ['rejectPinned (CLI tab close request)', { rejectPinned: true }], + ['skipRunningProcessConfirm (bulk / CLI)', { skipRunningProcessConfirm: true }], + ['reason pty-exit', { reason: 'pty-exit' as const }], + ['reason cleanup', { reason: 'cleanup' as const }], + ['hostCloseReason pty-exit', { hostCloseReason: 'pty-exit' as const }], + ['lifecyclePtyId', { lifecyclePtyId: 'pty-busy' }] + ])('never probes for a close carrying %s', (_label, options) => { + closeTerminalTab('tab-busy', options) + + expect(inspectRuntimeTerminalProcessMock).not.toHaveBeenCalled() + }) + + it('shows only the pinned dialog for a pinned busy tab, and its confirm does not probe', () => { + const state = busyTabState() + ;(state.unifiedTabsByWorktree as Record)[ + 'wt-1' + ]![0]!.isPinned = true + getStateMock.mockReturnValue(state) + + closeTerminalTab('tab-busy') + + expect(requestPinnedTabCloseConfirmMock).toHaveBeenCalledTimes(1) + expect(inspectRuntimeTerminalProcessMock).not.toHaveBeenCalled() + + const onConfirm = requestPinnedTabCloseConfirmMock.mock.calls[0]![0].onConfirm as () => void + onConfirm() + + expect(inspectRuntimeTerminalProcessMock).not.toHaveBeenCalled() + expect(closeTab).toHaveBeenCalledWith('tab-busy') + }) + + // Why: the pin prompt supersedes this one only when it actually appears. With the pin + // confirmation off it says nothing, and the running command must still be announced. + it('still asks about a running command on a pinned tab when pin confirmation is off', async () => { + const state = busyTabState({ + settings: { activeRuntimeEnvironmentId: null, confirmClosePinnedTab: false } + }) + ;(state.unifiedTabsByWorktree as Record)[ + 'wt-1' + ]![0]!.isPinned = true + getStateMock.mockReturnValue(state) + + closeTerminalTab('tab-busy') + await settleProbe() + + expect(requestPinnedTabCloseConfirmMock).not.toHaveBeenCalled() + expect(closeTab).not.toHaveBeenCalled() + expect(visibleRequest()).toMatchObject({ terminalTabId: 'tab-busy' }) + + useRunningTerminalCloseConfirmStore.getState().confirmRunningTerminalClose() + + expect(closeTab).toHaveBeenCalledWith('tab-busy') + }) + + it('rejects a pinned tab for a CLI close even when pin confirmation is off', () => { + const state = busyTabState({ + settings: { activeRuntimeEnvironmentId: null, confirmClosePinnedTab: false } + }) + ;(state.unifiedTabsByWorktree as Record)[ + 'wt-1' + ]![0]!.isPinned = true + getStateMock.mockReturnValue(state) + const onCancel = vi.fn() + + closeTerminalTab('tab-busy', { rejectPinned: true, onCancel }) + + expect(onCancel).toHaveBeenCalledTimes(1) + expect(inspectRuntimeTerminalProcessMock).not.toHaveBeenCalled() + expect(closeTab).not.toHaveBeenCalled() + }) + + it('confirms before telling a paired host to close its busy tab', async () => { + isWebRuntimeSessionActiveMock.mockReturnValue(true) + getStateMock.mockReturnValue( + busyTabState({ settings: { activeRuntimeEnvironmentId: 'web-runtime' } }) + ) + + closeTerminalTab('tab-busy') + await settleProbe() + + expect(closeWebRuntimeSessionTabMock).not.toHaveBeenCalled() + expect(closeTab).not.toHaveBeenCalled() + + useRunningTerminalCloseConfirmStore.getState().confirmRunningTerminalClose() + + expect(closeWebRuntimeSessionTabMock).toHaveBeenCalledTimes(1) + }) + + it('picks the agent copy for a busy agent pane', async () => { + getStateMock.mockReturnValue( + busyTabState({ agentStatusByPaneKey: { [`tab-busy:${LEAF}`]: { agentType: 'claude' } } }) + ) + + closeTerminalTab('tab-busy') + await settleProbe() + + expect(visibleRequest()?.copyKind).toBe('agent') + }) +}) diff --git a/src/renderer/src/hooks/useIpcEvents.test.ts b/src/renderer/src/hooks/useIpcEvents.test.ts index e59b6db6b..c13cf2005 100644 --- a/src/renderer/src/hooks/useIpcEvents.test.ts +++ b/src/renderer/src/hooks/useIpcEvents.test.ts @@ -3382,7 +3382,10 @@ describe('useIpcEvents browser tab close routing', () => { closeTerminalListenerRef.current?.({ tabId: 'terminal-1' }) - expect(closeTerminalTabMock).toHaveBeenCalledWith('terminal-1') + // The CLI/RPC caller is answered immediately, so this close must never raise a modal. + expect(closeTerminalTabMock).toHaveBeenCalledWith('terminal-1', { + skipRunningProcessConfirm: true + }) }) it('acknowledges whole-tab close only after the fresh session is durably persisted', async () => { diff --git a/src/renderer/src/hooks/useIpcEvents.ts b/src/renderer/src/hooks/useIpcEvents.ts index be6a5e4b2..35c48344e 100644 --- a/src/renderer/src/hooks/useIpcEvents.ts +++ b/src/renderer/src/hooks/useIpcEvents.ts @@ -1965,7 +1965,8 @@ export function useIpcEvents(): void { const detail: CloseTerminalPaneDetail = { tabId, paneRuntimeId } window.dispatchEvent(new CustomEvent(CLOSE_TERMINAL_PANE_EVENT, { detail })) } else { - closeTerminalTab(tabId) + // Why: the CLI/RPC caller is answered immediately, so it cannot wait on a modal. + closeTerminalTab(tabId, { skipRunningProcessConfirm: true }) } }) ) diff --git a/src/renderer/src/store/pinned-tab-close-guard.ts b/src/renderer/src/store/pinned-tab-close-guard.ts index 9603abd60..75a7db4fc 100644 --- a/src/renderer/src/store/pinned-tab-close-guard.ts +++ b/src/renderer/src/store/pinned-tab-close-guard.ts @@ -23,6 +23,13 @@ export function isUnifiedTabPinned(state: AppState, worktreeId: string, tabId: s ) } +/** Whether a pinned close will actually raise the pin dialog. Callers that let the pin + * prompt supersede another confirmation must know this: with the setting off the pin + * has nothing to say, so it must not swallow the other prompt (#10142). */ +export function shouldConfirmPinnedTabClose(state: AppState): boolean { + return state.settings?.confirmClosePinnedTab ?? true +} + /** Routes a pinned-tab close attempt through the confirmation dialog when the * setting is on. Non-pinned tabs (and pinned tabs when the setting is off) * close immediately. Keeping every close path behind this single helper is why @@ -40,8 +47,7 @@ export function guardPinnedTabClose(params: { } const state = useAppStore.getState() - const shouldConfirm = state.settings?.confirmClosePinnedTab ?? true - if (!shouldConfirm) { + if (!shouldConfirmPinnedTabClose(state)) { onClose() return } diff --git a/src/renderer/src/store/running-terminal-close-confirm.test.ts b/src/renderer/src/store/running-terminal-close-confirm.test.ts new file mode 100644 index 000000000..b597923a1 --- /dev/null +++ b/src/renderer/src/store/running-terminal-close-confirm.test.ts @@ -0,0 +1,226 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import { useRunningTerminalCloseConfirmStore } from './running-terminal-close-confirm' + +// The store rejects a second action within 350 ms of revealing a queued request, so these +// tests drive a clock rather than letting wall time decide whether an action lands. +let clock = 1_000 + +function advancePastGuard(): void { + clock += 400 +} + +function drainRequests(): void { + const store = useRunningTerminalCloseConfirmStore.getState() + while (useRunningTerminalCloseConfirmStore.getState().runningTerminalCloseConfirm) { + advancePastGuard() + store.dismissRunningTerminalClose() + } +} + +function request(terminalTabId: string, onConfirm: () => void = vi.fn(), onCancel?: () => void) { + return { + terminalTabId, + tabLabel: terminalTabId, + copyKind: 'command' as const, + onConfirm, + ...(onCancel ? { onCancel } : {}) + } +} + +describe('running terminal close confirmation store', () => { + beforeEach(() => { + // Monotonic across tests: the store is a singleton, so winding the clock back would + // leave a previous test's guard deadline in the future and block every action. + clock += 10_000 + vi.spyOn(Date, 'now').mockImplementation(() => clock) + }) + + afterEach(() => { + drainRequests() + vi.mocked(Date.now).mockRestore() + }) + + it('shows the first request and queues the next one', () => { + const first = vi.fn() + const second = vi.fn() + const store = useRunningTerminalCloseConfirmStore.getState() + + store.requestRunningTerminalCloseConfirm(request('tab-1', first)) + store.requestRunningTerminalCloseConfirm(request('tab-2', second)) + + expect( + useRunningTerminalCloseConfirmStore.getState().runningTerminalCloseConfirm?.terminalTabId + ).toBe('tab-1') + + store.confirmRunningTerminalClose() + + expect(first).toHaveBeenCalledTimes(1) + expect(second).not.toHaveBeenCalled() + expect( + useRunningTerminalCloseConfirmStore.getState().runningTerminalCloseConfirm?.terminalTabId + ).toBe('tab-2') + }) + + it('shows one prompt for a repeat request but still resolves both closes', () => { + const first = vi.fn() + const duplicate = vi.fn() + const store = useRunningTerminalCloseConfirmStore.getState() + + store.requestRunningTerminalCloseConfirm(request('tab-1', first)) + store.requestRunningTerminalCloseConfirm(request('tab-1', duplicate)) + store.confirmRunningTerminalClose() + + expect(first).toHaveBeenCalledTimes(1) + expect(duplicate).toHaveBeenCalledTimes(1) + expect(useRunningTerminalCloseConfirmStore.getState().runningTerminalCloseConfirm).toBeNull() + }) + + it('cancels both callers when a folded repeat request is dismissed', () => { + const store = useRunningTerminalCloseConfirmStore.getState() + const firstCancel = vi.fn() + const duplicateCancel = vi.fn() + + store.requestRunningTerminalCloseConfirm(request('tab-1', vi.fn(), firstCancel)) + store.requestRunningTerminalCloseConfirm(request('tab-1', vi.fn(), duplicateCancel)) + store.dismissRunningTerminalClose() + + expect(firstCancel).toHaveBeenCalledTimes(1) + expect(duplicateCancel).toHaveBeenCalledTimes(1) + }) + + it('folds a repeat request into the one already waiting in the queue', () => { + const store = useRunningTerminalCloseConfirmStore.getState() + const queued = vi.fn() + const duplicate = vi.fn() + + store.requestRunningTerminalCloseConfirm(request('tab-1')) + store.requestRunningTerminalCloseConfirm(request('tab-2', queued)) + store.requestRunningTerminalCloseConfirm(request('tab-2', duplicate)) + + store.confirmRunningTerminalClose() + advancePastGuard() + store.confirmRunningTerminalClose() + + expect(queued).toHaveBeenCalledTimes(1) + expect(duplicate).toHaveBeenCalledTimes(1) + expect(useRunningTerminalCloseConfirmStore.getState().runningTerminalCloseConfirm).toBeNull() + }) + + it('confirms every pending request once the user opts out of the prompt', () => { + const store = useRunningTerminalCloseConfirmStore.getState() + const visible = vi.fn() + const queued = vi.fn() + + store.requestRunningTerminalCloseConfirm(request('tab-1', visible)) + store.requestRunningTerminalCloseConfirm(request('tab-2', queued)) + store.confirmAllRunningTerminalCloses() + + expect(visible).toHaveBeenCalledTimes(1) + expect(queued).toHaveBeenCalledTimes(1) + expect(useRunningTerminalCloseConfirmStore.getState().runningTerminalCloseConfirm).toBeNull() + }) + + it('runs onCancel on dismiss and never the close', () => { + const onConfirm = vi.fn() + const onCancel = vi.fn() + const store = useRunningTerminalCloseConfirmStore.getState() + + store.requestRunningTerminalCloseConfirm(request('tab-1', onConfirm, onCancel)) + store.dismissRunningTerminalClose() + + expect(onCancel).toHaveBeenCalledTimes(1) + expect(onConfirm).not.toHaveBeenCalled() + expect(useRunningTerminalCloseConfirmStore.getState().runningTerminalCloseConfirm).toBeNull() + }) + + it('is inert when there is nothing to confirm', () => { + const store = useRunningTerminalCloseConfirmStore.getState() + + expect(() => store.confirmRunningTerminalClose()).not.toThrow() + expect(() => store.dismissRunningTerminalClose()).not.toThrow() + }) + + it('lets a re-entrant close from onConfirm queue behind the next request', () => { + const store = useRunningTerminalCloseConfirmStore.getState() + const reentrant = vi.fn() + const second = vi.fn() + + store.requestRunningTerminalCloseConfirm( + request('tab-1', () => { + // Why: the real onConfirm re-enters closeTerminalTab, which may request again. + store.requestRunningTerminalCloseConfirm(request('tab-3', reentrant)) + }) + ) + store.requestRunningTerminalCloseConfirm(request('tab-2', second)) + store.confirmRunningTerminalClose() + + expect( + useRunningTerminalCloseConfirmStore.getState().runningTerminalCloseConfirm?.terminalTabId + ).toBe('tab-2') + + advancePastGuard() + store.confirmRunningTerminalClose() + expect(second).toHaveBeenCalledTimes(1) + expect( + useRunningTerminalCloseConfirmStore.getState().runningTerminalCloseConfirm?.terminalTabId + ).toBe('tab-3') + + advancePastGuard() + store.confirmRunningTerminalClose() + expect(reentrant).toHaveBeenCalledTimes(1) + }) + + // Why: a queued request replaces the visible one in place. Without this window, the second + // click of a double-click aimed at one tab lands on the next tab's prompt and kills a + // running process the user never saw asked about — the bug class this PR exists to close. + it('ignores a second action that lands on the freshly revealed request', () => { + const store = useRunningTerminalCloseConfirmStore.getState() + const first = vi.fn() + const second = vi.fn() + + store.requestRunningTerminalCloseConfirm(request('tab-1', first)) + store.requestRunningTerminalCloseConfirm(request('tab-2', second)) + + store.confirmRunningTerminalClose() + store.confirmRunningTerminalClose() + + expect(first).toHaveBeenCalledTimes(1) + expect(second).not.toHaveBeenCalled() + expect( + useRunningTerminalCloseConfirmStore.getState().runningTerminalCloseConfirm?.terminalTabId + ).toBe('tab-2') + + advancePastGuard() + store.confirmRunningTerminalClose() + expect(second).toHaveBeenCalledTimes(1) + }) + + it('also holds off a dismiss that lands on the freshly revealed request', () => { + const store = useRunningTerminalCloseConfirmStore.getState() + const secondCancel = vi.fn() + + store.requestRunningTerminalCloseConfirm(request('tab-1')) + store.requestRunningTerminalCloseConfirm(request('tab-2', vi.fn(), secondCancel)) + + store.confirmRunningTerminalClose() + store.dismissRunningTerminalClose() + + expect(secondCancel).not.toHaveBeenCalled() + expect( + useRunningTerminalCloseConfirmStore.getState().runningTerminalCloseConfirm?.terminalTabId + ).toBe('tab-2') + }) + + // Why: emptying the queue leaves nothing to mis-click, so the opt-out must not be delayed. + it('arms no guard when the last request leaves the queue empty', () => { + const store = useRunningTerminalCloseConfirmStore.getState() + const onConfirm = vi.fn() + + store.requestRunningTerminalCloseConfirm(request('tab-1')) + store.confirmRunningTerminalClose() + store.requestRunningTerminalCloseConfirm(request('tab-2', onConfirm)) + store.confirmRunningTerminalClose() + + expect(onConfirm).toHaveBeenCalledTimes(1) + }) +}) diff --git a/src/renderer/src/store/running-terminal-close-confirm.ts b/src/renderer/src/store/running-terminal-close-confirm.ts new file mode 100644 index 000000000..618d30c98 --- /dev/null +++ b/src/renderer/src/store/running-terminal-close-confirm.ts @@ -0,0 +1,132 @@ +import { create } from 'zustand' +import type { CloseTerminalDialogCopyKind } from '@/components/terminal-pane/CloseTerminalDialog' + +/** A pending confirmation for closing a terminal tab whose shell still has a + * running child process. `onConfirm` performs the original close. */ +export type RunningTerminalCloseConfirmRequest = { + terminalTabId: string + tabLabel: string + copyKind: CloseTerminalDialogCopyKind + onConfirm: () => void + onCancel?: () => void +} + +export type RunningTerminalCloseConfirmState = { + runningTerminalCloseConfirm: RunningTerminalCloseConfirmRequest | null + requestRunningTerminalCloseConfirm: (request: RunningTerminalCloseConfirmRequest) => void + confirmRunningTerminalClose: () => void + /** Accepts the visible request and every queued one. Used when the user ticks "don't ask + * again": a prompt they just opted out of must not still be waiting behind this one. */ + confirmAllRunningTerminalCloses: () => void + dismissRunningTerminalClose: () => void +} + +/** Folds a duplicate request for the same tab into the pending one instead of dropping it, + * so two surfaces closing the same tab both get their callback. */ +function mergeRequests( + pending: RunningTerminalCloseConfirmRequest, + duplicate: RunningTerminalCloseConfirmRequest +): RunningTerminalCloseConfirmRequest { + return { + ...pending, + onConfirm: () => { + pending.onConfirm() + duplicate.onConfirm() + }, + onCancel: () => { + pending.onCancel?.() + duplicate.onCancel?.() + } + } +} + +// Why a standalone store instead of an AppState slice (which is what the sibling +// pinned-tab confirmation uses): the request is raised from closeTerminalTab, a plain +// module whose unit fixtures build partial app-state objects, so dispatching through +// useAppStore.getState() would throw there. Nothing outside the dialog reads this state, +// so the AppState coupling would buy nothing. +export const useRunningTerminalCloseConfirmStore = create()(( + set, + get +) => { + const queuedRequests: RunningTerminalCloseConfirmRequest[] = [] + // Why: a queued request replaces the visible one in place, so a double-click or held + // Enter meant for the tab the user was looking at would land on the next tab's prompt and + // kill a second running process unseen. Matches the sibling pinned-tab confirmation. + const INTER_REQUEST_ACTION_GUARD_MS = 350 + let nextRequestActionAllowedAt = 0 + + /** Reveals the next queued request, and reports whether one took the visible slot. */ + const advanceRequest = (): boolean => { + const next = queuedRequests.shift() ?? null + set({ runningTerminalCloseConfirm: next }) + return next !== null + } + + const guardNextAction = (revealedNextRequest: boolean): void => { + if (revealedNextRequest) { + nextRequestActionAllowedAt = Date.now() + INTER_REQUEST_ACTION_GUARD_MS + } + } + + return { + runningTerminalCloseConfirm: null, + + requestRunningTerminalCloseConfirm: (request) => { + const visible = get().runningTerminalCloseConfirm + // Why: the probe is async, so a second click on the same tab arrives before the + // dialog opens. One prompt, but both closes still resolve. + if (visible?.terminalTabId === request.terminalTabId) { + set({ runningTerminalCloseConfirm: mergeRequests(visible, request) }) + return + } + const queuedIndex = queuedRequests.findIndex( + (queued) => queued.terminalTabId === request.terminalTabId + ) + if (queuedIndex >= 0) { + queuedRequests[queuedIndex] = mergeRequests(queuedRequests[queuedIndex]!, request) + return + } + if (visible) { + // Why: closing two busy tabs in quick succession must not strand the second + // tab's close callback behind a replaced request. + queuedRequests.push(request) + return + } + set({ runningTerminalCloseConfirm: request }) + }, + + confirmRunningTerminalClose: () => { + const request = get().runningTerminalCloseConfirm + if (!request || Date.now() < nextRequestActionAllowedAt) { + return + } + // Why: advance before running onConfirm so a re-entrant close queues behind the + // next real request instead of seeing the stale one. + guardNextAction(advanceRequest()) + request.onConfirm() + }, + + confirmAllRunningTerminalCloses: () => { + if (Date.now() < nextRequestActionAllowedAt) { + return + } + const pending = [get().runningTerminalCloseConfirm, ...queuedRequests.splice(0)] + set({ runningTerminalCloseConfirm: null }) + // No guard to arm: the queue is empty, so there is no next prompt to mis-click. + for (const request of pending) { + request?.onConfirm() + } + }, + + dismissRunningTerminalClose: () => { + const request = get().runningTerminalCloseConfirm + if (!request || Date.now() < nextRequestActionAllowedAt) { + return + } + guardNextAction(advanceRequest()) + // Why: callers such as the tab-group model resume their own cleanup on cancel. + request.onCancel?.() + } + } +}) diff --git a/tests/e2e/terminal-tab-close-running-confirm-mouse.spec.ts b/tests/e2e/terminal-tab-close-running-confirm-mouse.spec.ts new file mode 100644 index 000000000..c854c8ba7 --- /dev/null +++ b/tests/e2e/terminal-tab-close-running-confirm-mouse.spec.ts @@ -0,0 +1,101 @@ +/** + * #10142 follow-ups to the X-button regression: middle-click prompts too, confirming + * actually closes, and Cmd+W raises exactly one dialog (the pane path delegates the + * last-pane close to closeTerminalTab instead of probing a second time). + */ +import { test, expect } from './helpers/orca-app' +import type { Page } from '@stablyai/playwright-test' +import { + waitForSessionReady, + waitForActiveWorktree, + getActiveTabId, + ensureTerminalVisible +} from './helpers/store' +import { + execInTerminal, + focusActiveTerminalInput, + waitForActivePanePtyId, + waitForActiveTerminalManager, + waitForPaneCount, + waitForTerminalOutput +} from './helpers/terminal' + +const SORTABLE_TAB = '[data-testid="sortable-tab"]' + +function closeDialogTitle(page: Page) { + return page.getByText(/Stop running command\?|Stop this agent\?/) +} + +async function startBusyTerminal(page: Page): Promise { + await waitForSessionReady(page) + await waitForActiveWorktree(page) + await ensureTerminalVisible(page) + const hasPaneManager = await waitForActiveTerminalManager(page, 30_000) + .then(() => true) + .catch(() => false) + test.skip(!hasPaneManager, 'Electron automation never mounted the live TerminalPane manager.') + await waitForPaneCount(page, 1, 30_000) + + const ptyId = await waitForActivePanePtyId(page) + await execInTerminal(page, ptyId, 'echo close-confirm-ready') + await waitForTerminalOutput(page, 'close-confirm-ready', 20_000) + await execInTerminal(page, ptyId, 'sleep 300') + // Why: `hasChildProcesses` is already true while macOS's `login` wrapper starts the + // shell, so wait for `sleep` itself or the close legitimately sees an idle terminal. + await expect + .poll( + async () => + (await page.evaluate((id) => window.api.pty.inspectProcess(id), ptyId)).foregroundProcess, + { timeout: 20_000, message: 'sleep 300 never became the foreground process' } + ) + .toBe('sleep') + return (await getActiveTabId(page))! +} + +test.describe.configure({ mode: 'serial' }) + +test('middle-clicking a busy tab prompts, and cancelling keeps the tab', async ({ orcaPage }) => { + test.setTimeout(120_000) + const busyTabId = await startBusyTerminal(orcaPage) + const busyTab = orcaPage.locator(`${SORTABLE_TAB}[data-tab-id="${busyTabId}"]`).first() + const tabsBefore = await orcaPage.locator(SORTABLE_TAB).count() + + await busyTab.click({ button: 'middle' }) + + await expect(closeDialogTitle(orcaPage)).toBeVisible({ timeout: 15_000 }) + await orcaPage.getByRole('button', { name: /^Cancel$/ }).click() + await expect(closeDialogTitle(orcaPage)).toBeHidden() + await expect(busyTab).toBeVisible() + expect(await orcaPage.locator(SORTABLE_TAB).count()).toBe(tabsBefore) +}) + +test('confirming the X-button prompt closes the busy tab', async ({ orcaPage }) => { + test.setTimeout(120_000) + const busyTabId = await startBusyTerminal(orcaPage) + const busyTab = orcaPage.locator(`${SORTABLE_TAB}[data-tab-id="${busyTabId}"]`).first() + + await busyTab.hover() + await busyTab.getByRole('button', { name: /^Close tab /i }).click() + await expect(closeDialogTitle(orcaPage)).toBeVisible({ timeout: 15_000 }) + await orcaPage.getByRole('button', { name: /^Stop and Close$/ }).click() + + await expect(busyTab).toHaveCount(0, { timeout: 15_000 }) + await expect(closeDialogTitle(orcaPage)).toBeHidden() +}) + +test('Cmd+W on a busy single-pane tab raises exactly one dialog', async ({ orcaPage }) => { + test.setTimeout(120_000) + const busyTabId = await startBusyTerminal(orcaPage) + const busyTab = orcaPage.locator(`${SORTABLE_TAB}[data-tab-id="${busyTabId}"]`).first() + + await focusActiveTerminalInput(orcaPage) + await orcaPage.keyboard.press(process.platform === 'darwin' ? 'Meta+w' : 'Control+w') + await expect(closeDialogTitle(orcaPage)).toBeVisible({ timeout: 15_000 }) + await orcaPage.getByRole('button', { name: /^Stop and Close$/ }).click() + + await expect(busyTab).toHaveCount(0, { timeout: 15_000 }) + // Why: the pane used to probe and prompt on its own before delegating to + // closeTerminalTab, which now prompts too — a second dialog would mean a double prompt. + await orcaPage.waitForTimeout(1_500) + await expect(closeDialogTitle(orcaPage)).toBeHidden() +}) diff --git a/tests/e2e/terminal-tab-close-running-confirm.spec.ts b/tests/e2e/terminal-tab-close-running-confirm.spec.ts new file mode 100644 index 000000000..68182eb47 --- /dev/null +++ b/tests/e2e/terminal-tab-close-running-confirm.spec.ts @@ -0,0 +1,90 @@ +/** + * Regression for #10142: keyboard and mouse enforce the same running-process close + * confirmation. Both halves run against one tab with a live `sleep 300` child: + * 1. Cmd/Ctrl+W -> "Stop running command?" dialog (cancelled, tab survives). + * 2. X click -> the same dialog, and the tab is still there behind it. + */ +import { test, expect } from './helpers/orca-app' +import type { Page } from '@stablyai/playwright-test' +import { + waitForSessionReady, + waitForActiveWorktree, + getActiveTabId, + ensureTerminalVisible +} from './helpers/store' +import { + execInTerminal, + focusActiveTerminalInput, + waitForActivePanePtyId, + waitForActiveTerminalManager, + waitForPaneCount, + waitForTerminalOutput +} from './helpers/terminal' + +const SORTABLE_TAB = '[data-testid="sortable-tab"]' + +function countRenderedTabs(page: Page): Promise { + return page.locator(SORTABLE_TAB).count() +} + +function closeDialogTitle(page: Page) { + return page.getByText(/Stop running command\?|Stop this agent\?/) +} + +test.describe.configure({ mode: 'serial' }) + +test('the tab X button applies the same running-process confirmation as Cmd+W', async ({ + orcaPage +}) => { + test.setTimeout(120_000) + await waitForSessionReady(orcaPage) + await waitForActiveWorktree(orcaPage) + await ensureTerminalVisible(orcaPage) + const hasPaneManager = await waitForActiveTerminalManager(orcaPage, 30_000) + .then(() => true) + .catch(() => false) + test.skip(!hasPaneManager, 'Electron automation never mounted the live TerminalPane manager.') + await waitForPaneCount(orcaPage, 1, 30_000) + + const ptyId = await waitForActivePanePtyId(orcaPage) + await execInTerminal(orcaPage, ptyId, 'echo repro-10142-ready') + await waitForTerminalOutput(orcaPage, 'repro-10142-ready', 20_000) + await execInTerminal(orcaPage, ptyId, 'sleep 300') + // Only press close once `sleep` is the foreground process; otherwise the probe + // legitimately sees an idle shell and closing is correct. `hasChildProcesses` alone is + // not enough: macOS spawns the shell under `login`, so a still-initialising terminal + // reports a child before `sleep 300` has run. + await expect + .poll( + async () => + (await orcaPage.evaluate((id) => window.api.pty.inspectProcess(id), ptyId)) + .foregroundProcess, + { timeout: 20_000, message: 'sleep 300 never became the foreground process' } + ) + .toBe('sleep') + + const busyTabId = (await getActiveTabId(orcaPage))! + const busyTab = orcaPage.locator(`${SORTABLE_TAB}[data-tab-id="${busyTabId}"]`).first() + + // 1. Keyboard close prompts. + await focusActiveTerminalInput(orcaPage) + await orcaPage.keyboard.press(process.platform === 'darwin' ? 'Meta+w' : 'Control+w') + await expect(closeDialogTitle(orcaPage)).toBeVisible({ timeout: 15_000 }) + await orcaPage.getByRole('button', { name: /^Cancel$/ }).click() + await expect(closeDialogTitle(orcaPage)).toBeHidden() + await expect(busyTab).toBeVisible() + const tabsBefore = await countRenderedTabs(orcaPage) + + // 2. Same tab, same running child, mouse close. + await busyTab.hover() + await busyTab.getByRole('button', { name: /^Close tab /i }).click() + await orcaPage.waitForTimeout(1_500) + + expect( + { + confirmDialogVisible: await closeDialogTitle(orcaPage).isVisible(), + tabStillPresent: (await countRenderedTabs(orcaPage)) === tabsBefore + }, + 'X-button close must apply the same running-process confirmation as Cmd+W' + ).toEqual({ confirmDialogVisible: true, tabStillPresent: true }) +})