fix: preserve terminals on manual quit (#524)

This commit is contained in:
Jinjing 2026-04-11 23:50:58 -07:00 committed by GitHub
parent a9b3ae6de4
commit 206eb50ab2
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
7 changed files with 144 additions and 23 deletions

View File

@ -27,6 +27,10 @@ import { createMainWindow } from './window/createMainWindow'
import { CodexAccountService } from './codex-accounts/service'
let mainWindow: BrowserWindow | null = null
/** Whether a manual app.quit() (Cmd+Q, etc.) is in progress. Shared with the
* window close handler so it can tell the renderer to skip the running-process
* confirmation dialog and proceed directly to buffer capture + close. */
let isQuitting = false
let store: Store | null = null
let stats: StatsCollector | null = null
let claudeUsage: ClaudeUsageStore | null = null
@ -75,7 +79,12 @@ function openMainWindow(): BrowserWindow {
throw new Error('Codex account service must be initialized before opening the main window')
}
const window = createMainWindow(store)
const window = createMainWindow(store, {
getIsQuitting: () => isQuitting,
onQuitAborted: () => {
isQuitting = false
}
})
registerCoreHandlers(
store,
runtime,
@ -169,12 +178,21 @@ app.whenReady().then(async () => {
})
app.on('before-quit', () => {
isQuitting = true
// Why: PTY cleanup is deferred to will-quit so the renderer has a chance to
// capture terminal scrollback buffers before PTY exit events race in and
// unmount TerminalPane components (removing their capture callbacks).
// The window close handler passes isQuitting to the renderer so it skips the
// child-process confirmation dialog and proceeds directly to buffer capture.
rateLimits?.stop()
})
app.on('will-quit', () => {
// Why: stats.flush() must run before killAllPty() so it can read the
// live agent state and emit synthetic agent_stop events for agents that
// are still running. killAllPty() does not call runtime.onPtyExit(),
// so without this ordering, running agents would produce orphaned
// agent_start events with no matching stops.
rateLimits?.stop()
stats?.flush()
killAllPty()
void closeAllWatchers()
@ -187,7 +205,12 @@ app.on('before-quit', () => {
})
app.on('window-all-closed', () => {
if (process.platform !== 'darwin') {
// Why: on macOS, closing all windows normally keeps the app alive (dock
// stays active). But when a quit is in progress (Cmd+Q), the window close
// handler defers to the renderer for buffer capture, which cancels the
// original quit sequence. Re-trigger quit here so the app actually exits
// instead of requiring a second Cmd+Q.
if (process.platform !== 'darwin' || isQuitting) {
app.quit()
}
})

View File

@ -318,8 +318,24 @@ describe('createMainWindow', () => {
const isDarwin = process.platform === 'darwin'
for (const input of [
{ type: 'keyDown', code: 'KeyJ', key: 'j', meta: isDarwin, control: !isDarwin, alt: false, shift: !isDarwin },
{ type: 'keyDown', code: 'KeyJ', key: '', meta: isDarwin, control: !isDarwin, alt: false, shift: !isDarwin }
{
type: 'keyDown',
code: 'KeyJ',
key: 'j',
meta: isDarwin,
control: !isDarwin,
alt: false,
shift: !isDarwin
},
{
type: 'keyDown',
code: 'KeyJ',
key: '',
meta: isDarwin,
control: !isDarwin,
alt: false,
shift: !isDarwin
}
]) {
const preventDefault = vi.fn()
windowHandlers['before-input-event']({ preventDefault } as never, input as never)
@ -377,4 +393,47 @@ describe('createMainWindow', () => {
expect(webContents.openDevTools).toHaveBeenCalledWith({ mode: 'undocked' })
expect(webContents.closeDevTools).not.toHaveBeenCalled()
})
it('clears the quit latch when the renderer prevents unload', () => {
const windowHandlers: Record<string, (...args: any[]) => void> = {}
const webContents = {
on: vi.fn((event, handler) => {
windowHandlers[event] = handler
}),
setZoomLevel: vi.fn(),
setBackgroundThrottling: vi.fn(),
invalidate: vi.fn(),
setWindowOpenHandler: vi.fn(),
send: vi.fn()
}
const browserWindowInstance = {
webContents,
on: vi.fn((event, handler) => {
windowHandlers[event] = handler
}),
isDestroyed: vi.fn(() => false),
isMaximized: vi.fn(() => true),
isFullScreen: vi.fn(() => false),
getSize: vi.fn(() => [1200, 800]),
setSize: vi.fn(),
maximize: vi.fn(),
show: vi.fn(),
loadFile: vi.fn(),
loadURL: vi.fn()
}
const onQuitAborted = vi.fn()
browserWindowMock.mockImplementation(function () {
return browserWindowInstance
})
createMainWindow(null, { getIsQuitting: () => true, onQuitAborted })
const preventDefault = vi.fn()
windowHandlers.close({ preventDefault } as never)
expect(preventDefault).toHaveBeenCalledTimes(1)
expect(webContents.send).toHaveBeenCalledWith('window:close-requested', { isQuitting: true })
windowHandlers['will-prevent-unload']()
expect(onQuitAborted).toHaveBeenCalledTimes(1)
})
})

View File

@ -45,7 +45,22 @@ function syncTrafficLightPosition(win: BrowserWindow, zoomFactor: number): void
win.setWindowButtonPosition({ x: TRAFFIC_LIGHT_X, y })
}
export function createMainWindow(store: Store | null): BrowserWindow {
type CreateMainWindowOptions = {
/** Returns true when a manual app.quit() (Cmd+Q) is in progress. The close
* handler sends this to the renderer so it can skip the running-process
* confirmation dialog and proceed directly to buffer capture + close. */
getIsQuitting?: () => boolean
/** Notifies the caller when the renderer vetoes unload. Why: a prevented
* beforeunload cancels the in-flight app.quit(), so the app-level quit
* latch must be cleared or later window closes will be misclassified as
* quit attempts. */
onQuitAborted?: () => void
}
export function createMainWindow(
store: Store | null,
opts?: CreateMainWindowOptions
): BrowserWindow {
const mainWindow = new BrowserWindow({
width: 1200,
height: 800,
@ -258,7 +273,12 @@ export function createMainWindow(store: Store | null): BrowserWindow {
return
}
e.preventDefault()
mainWindow.webContents.send('window:close-requested')
mainWindow.webContents.send('window:close-requested', {
isQuitting: opts?.getIsQuitting?.() ?? false
})
})
mainWindow.webContents.on('will-prevent-unload', () => {
opts?.onQuitAborted?.()
})
const onConfirmClose = (): void => {

View File

@ -376,7 +376,7 @@ export type PreloadApi = {
setZoomLevel: (level: number) => void
syncTrafficLights: (zoomFactor: number) => void
onFullscreenChanged: (callback: (isFullScreen: boolean) => void) => () => void
onWindowCloseRequested: (callback: () => void) => () => void
onWindowCloseRequested: (callback: (data: { isQuitting: boolean }) => void) => () => void
confirmWindowClose: () => void
}
runtime: {

View File

@ -672,9 +672,12 @@ const api = {
},
/** Fired by the main process when the user tries to close the window
* (X button, Cmd+Q, etc.). Renderer should show a confirmation dialog
* if terminals are still running, then call confirmWindowClose(). */
onWindowCloseRequested: (callback: () => void): (() => void) => {
const listener = () => callback()
* if terminals are still running, then call confirmWindowClose().
* When isQuitting is true, the close was initiated by app.quit() (Cmd+Q)
* and the renderer should skip the running-process dialog. */
onWindowCloseRequested: (callback: (data: { isQuitting: boolean }) => void): (() => void) => {
const listener = (_event: Electron.IpcRendererEvent, data: { isQuitting: boolean }) =>
callback(data ?? { isQuitting: false })
ipcRenderer.on('window:close-requested', listener)
return () => ipcRenderer.removeListener('window:close-requested', listener)
},

View File

@ -252,7 +252,18 @@ function App(): React.JSX.Element {
// On shutdown, capture terminal scrollback buffers and flush to disk.
// Runs synchronously in beforeunload: capture → Zustand set → sendSync → flush.
useEffect(() => {
// Why: beforeunload fires twice during a manual quit — once from the
// synthetic dispatch in the onWindowCloseRequested handler (captures
// good data while TerminalPanes are still mounted), and again from the
// native window close triggered by confirmWindowClose(). Between these
// two firings, PTY exit events can arrive and unmount TerminalPanes,
// emptying shutdownBufferCaptures. The guard prevents the second call
// from overwriting the good session data with an empty snapshot.
let shutdownBuffersCaptured = false
const captureAndFlush = (): void => {
if (shutdownBuffersCaptured) {
return
}
if (!useAppStore.getState().workspaceSessionReady) {
return
}
@ -265,17 +276,16 @@ function App(): React.JSX.Element {
}
const state = useAppStore.getState()
window.api.session.setSync(buildWorkspaceSessionPayload(state))
shutdownBuffersCaptured = true
}
window.addEventListener('beforeunload', captureAndFlush)
return () => window.removeEventListener('beforeunload', captureAndFlush)
}, [])
// Periodically capture terminal scrollback buffers and persist to disk.
// Why: the normal shutdown path races with PTY exit events — before-quit
// kills all PTYs, whose async exit handlers close tabs and unmount
// TerminalPane components (removing their capture callbacks) before the
// renderer's beforeunload can run. Periodic saves ensure scrollback is
// available on restart even when the shutdown capture is lost.
// Why: the shutdown path captures buffers in beforeunload, but periodic
// saves provide a safety net so scrollback is available on restart even
// if an unexpected exit (crash, force-kill) bypasses normal shutdown.
useEffect(() => {
const PERIODIC_SAVE_INTERVAL_MS = 3 * 60_000
const timer = window.setInterval(() => {

View File

@ -608,18 +608,24 @@ export default function Terminal(): React.JSX.Element | null {
// Listen for main-process window close requests. When any terminal has a
// child process running (not just an idle shell), show a confirmation dialog.
useEffect(() => {
return window.api.ui.onWindowCloseRequested(() => {
return window.api.ui.onWindowCloseRequested(({ isQuitting }) => {
if (isUpdaterQuitAndInstallInProgress()) {
window.api.ui.confirmWindowClose()
return
}
// Why: before-quit (main process) kills all PTYs before this handler
// runs. The async PTY exit events close tabs and unmount TerminalPane
// components, removing their buffer capture callbacks. Dispatching
// beforeunload here — while components are still mounted — captures
// scrollback buffers before the race can discard them. This mirrors
// the auto-updater path (preload/index.ts) which does the same thing.
// Why: capture terminal scrollback buffers while TerminalPane components
// are still mounted. Dispatching beforeunload triggers the App.tsx
// captureAndFlush handler which serializes each pane's xterm buffer
// and writes the session to disk via synchronous IPC.
window.dispatchEvent(new Event('beforeunload'))
// Why: during a quit (Cmd+Q), PTYs are still alive (cleanup is deferred
// to will-quit so buffers can be captured first). Skip the child-process
// confirmation dialog and proceed directly — the user's intent to quit
// is unambiguous.
if (isQuitting) {
window.api.ui.confirmWindowClose()
return
}
const state = useAppStore.getState()
const allPtyIds = Object.values(state.ptyIdsByTabId).flat()
if (allPtyIds.length === 0) {