diff --git a/.ai/contexts/viewer-panel.md b/.ai/contexts/viewer-panel.md index 856c865e8..c9ced07e5 100644 --- a/.ai/contexts/viewer-panel.md +++ b/.ai/contexts/viewer-panel.md @@ -164,7 +164,7 @@ Each of these reads and clears `pendingTouchedOpen` before `destroyCurrentTab`, Nothing ends the window with unsaved file-panel edits without the user saying so. The scope is the Touched editor in the slot (a tab with an `absolutePath`) and every `touchedStashes` entry, in every session of `filePanelState`; the Memory and Work Files panels, MCP diff tabs and Changes buffers are not covered. -- **Main** (`unsaved-guard.js`, one guard, `attach(win)` per window, `beforeQuit(event, win)` for the app). `main.js`'s `before-quit` handler calls `beforeQuit` first: it prevents the quit, asks the renderer, and calls `app.quit()` again on a yes, so the cleanup that follows (PTYs killed, MCP servers, watchers) runs only once the quit is confirmed and a Cancel leaves the app intact. This covers every `app.quit()` caller (☰ Quit, the last window closing). `updater-install` asks first (`confirmQuit`), because electron-updater's `quitAndInstall()` starts the installer before it calls `app.quit()`: a Cancel must leave the installer unstarted, and a yes pre-approves the quit that follows. A Windows `session-end` approves the quit, so logoff and shutdown never wait on the dialog; `query-session-end` approves it for 60 s (`LOGOFF_CANCEL_MS`), so a logoff that is cancelled does not skip the question on every later quit. A window close, a quit and an install share one question while it is open; one that joins a `'reload'` question sends `unsaved-check-reason` (`id`, `'quit'`), and the renderer acts on the reason it holds when the user answers; a reload it has already approved then writes the app state at once, and main skips the reload of a question a quit joined. The window's own `close` event is held the same way for a plain window close. The question is `unsaved-check` (`id`, `'quit'` or `'reload'`); the renderer's `unsaved-check-result` (`id`, `proceed`) approves it. A `will-prevent-unload` (the renderer's `beforeunload` veto, which a reload hits) asks with `'reload'`; on a yes the next unload is allowed once (`allowNextUnload`, answered with `preventDefault()`, which in Electron means "unload anyway") and `webContents.reload()` is called. Every close asks the renderer, even with nothing dirty; a main-side dirty flag would save that round trip and is not built. +- **Main** (`unsaved-guard.js`, one guard, `attach(win)` per window, `beforeQuit(event, win)` for the app). `main.js`'s `before-quit` handler calls `beforeQuit` first: it prevents the quit, asks the renderer, and calls `app.quit()` again on a yes, so the cleanup that follows (PTYs killed, MCP servers, watchers) runs only once the quit is confirmed and a Cancel leaves the app intact. This covers every `app.quit()` caller (☰ Quit, the last window closing). `updater-install` asks first (`confirmQuit`), because electron-updater's `quitAndInstall()` starts the installer before it calls `app.quit()`: a Cancel must leave the installer unstarted, and a yes pre-approves the quit that follows. A Windows `session-end` approves the quit, so logoff and shutdown never wait on the dialog; `query-session-end` approves it for 60 s (`LOGOFF_CANCEL_MS`), so a logoff that is cancelled does not skip the question on every later quit. A window close, a quit and an install share one question while it is open; one that joins a `'reload'` question sends `unsaved-check-reason` (`id`, `'quit'`), and the renderer acts on the reason it holds when the user answers; a reload it has already approved then writes the app state at once. The renderer answers every upgrade with `unsaved-check-reason-done` (`id`): at once, unless it is writing for an approved reload, then once that write ends; main holds the joined quit until it, bounded by the check timeout and released at once if the renderer process is gone (`render-process-gone`, `destroyed`), since the answer may have left before the upgrade arrived. Until then the joined question stays the one in flight, so a close, quit or install arriving meanwhile joins it instead of asking again. Main skips the reload of a question a quit joined. The window's own `close` event is held the same way for a plain window close. The question is `unsaved-check` (`id`, `'quit'` or `'reload'`); the renderer's `unsaved-check-result` (`id`, `proceed`) approves it. A `will-prevent-unload` (the renderer's `beforeunload` veto, which a reload hits) asks with `'reload'`; on a yes the next unload is allowed once (`allowNextUnload`, answered with `preventDefault()`, which in Electron means "unload anyway") and `webContents.reload()` is called. Every close asks the renderer, even with nothing dirty; a main-side dirty flag would save that round trip and is not built. - **Bounded.** The renderer acknowledges (`unsaved-check-ack`) as soon as it receives the check, before any dialog. The 2.5 s bound (`DEFAULT_TIMEOUT_MS`) covers only send to ack: no ack answers yes, so a hung renderer never keeps the app from quitting. The window is then closed the usual way (`win.close()`, or `app.quit()` for a quit or an update install), so a page that is only slow (still starting, or busy) closes as before, without the question, once it runs its `beforeunload`. A hung page never runs it, and that close would never finish: the guard destroys such a window when Electron reports it `unresponsive`, or when it is closed again, the guard's own `close()` or `app.quit()` aside. Electron reports a page `unresponsive` about 5 s after a close it has not completed, and cannot tell a hung page from a busy one: a page still busy about 5 s after that close, about 7.5 s after the click counting the 2.5 s bound, is destroyed, and its pending work is lost (the debounced working-set save among it). Only a page that recovers before then closes normally; a stall of 8–10 s that starts before the click (#526) does not. `e2e/hung-window.spec.js` measures it: the app exits about 7.5 s after the close it schedules. A page reported unresponsive earlier, and neither `responsive` nor acknowledging a check since, is destroyed when that close reaches it: right after `win.close()` for a window close, and for a quit or an update install only once the quit closes the window (its `close` event), so a quit still held by another `before-quit` handler (the ActivityWatch flush) is not cut short. A check the page acknowledged never destroys the window, however long the user takes. After the ack the guard waits for the answer without a limit, so a slow user or a slow save loses nothing, and answers yes only if the renderer process is gone (`render-process-gone`, `destroyed`) or a send fails. A late answer is ignored. - **Renderer** (`file-panel.js`). `askAboutUnsavedEdits()` lists the dirty tabs (`collectUnsavedFileTabs`) in the `#unsaved-edits-dialog` dialog, built on the add-project dialog's classes (already in the frameless no-drag list). Save writes every dirty entry: the editor in the slot through `handleChangesSave`, a stashed one through `saveFileForPanel(filePath, content, savedContent, {git, version})`, removed from its session once written. A save that fails (including a disk that moved) keeps the dialog open with the reason, and nothing is answered. Discard answers yes. Cancel and Escape answer no. A second request while the dialog is open gets the same dialog's answer. - **`beforeunload`.** The renderer vetoes an unload while an entry is dirty, unless `unloadApproved`, set for 10 s after the user answered yes. That is what stops a reload from the keyboard or devtools from slipping past, and what keeps the veto from asking a second time after an approved close. diff --git a/CHANGELOG.md b/CHANGELOG.md index 862763f77..d17cf1f1c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,7 @@ What changes for you in each release of Switchboard. How to write an entry: [doc ### Fixed - The New Session dialog now opens with focus on the selected permission mode, and Enter starts the session with that mode. Cancelling returns focus to the + button that opened the dialog; starting moves it to the new terminal. (#583) - Tab and Shift+Tab now close the New session (+) menu and move on from its + button, instead of leaving the menu open with no way to close it from the keyboard. (#582) +- A quit that arrives just as a reload of the window is confirmed now waits for the open sessions to be saved before Switchboard closes, so the next start restores them. (#479) ## v0.0.95 — 2026-10-12 diff --git a/docs/session-restore.md b/docs/session-restore.md index fdf2f7bfe..f0b4c8a9e 100644 --- a/docs/session-restore.md +++ b/docs/session-restore.md @@ -57,8 +57,11 @@ not an exit and does neither. A quit or close that arrives while a reload's question is still open joins it, and main tells the renderer the question is now an exit (`unsaved-check-reason`), so the answer still waits for the write, and the page is not reloaded before the quit. If the page had already answered -the reload when the upgrade reaches it, it writes the set then, best effort as -for a logoff below. +the reload when the upgrade reaches it, it writes the set then, and main waits +for it to report the write done (`unsaved-check-reason-done`, bounded to +2.5 s, and not at all once the page has crashed) before quitting; the page +sends that report at once when it has nothing to write then. A close or quit +that arrives during that wait joins it rather than asking again. The write keeps the saved sessions that are not open yet: those the index has not reached, those offered by the Restore prompt and not answered, and those diff --git a/preload.js b/preload.js index 82d46c043..fb62fa74e 100644 --- a/preload.js +++ b/preload.js @@ -168,6 +168,7 @@ contextBridge.exposeInMainWorld('api', { }, unsavedCheckAck: (id) => ipcRenderer.send('unsaved-check-ack', id), unsavedCheckResult: (id, proceed) => ipcRenderer.send('unsaved-check-result', id, proceed), + unsavedCheckReasonDone: (id) => ipcRenderer.send('unsaved-check-reason-done', id), onExitFlush: (callback) => { ipcRenderer.on('exit-flush', () => callback()); }, diff --git a/public/file-panel.js b/public/file-panel.js index a91a9de00..d06604e9f 100644 --- a/public/file-panel.js +++ b/public/file-panel.js @@ -154,10 +154,13 @@ function initFilePanel() { const answeredReloads = new Set(); if (window.api.onUnsavedCheckReason) { window.api.onUnsavedCheckReason((id, reason) => { + const done = () => { if (window.api.unsavedCheckReasonDone) window.api.unsavedCheckReasonDone(id); }; if (unsavedCheckReasons.has(id)) unsavedCheckReasons.set(id, reason); else if (answeredReloads.delete(id) && typeof window.flushStateForExit === 'function') { - window.flushStateForExit().catch((err) => console.error('[exit-flush]', err)); + window.flushStateForExit().catch((err) => console.error('[exit-flush]', err)).finally(done); + return; } + done(); }); } window.api.onUnsavedCheck(async (id, reason) => { diff --git a/test/dom-file-panel-unsaved-guard.test.js b/test/dom-file-panel-unsaved-guard.test.js index e1ad0d09f..398b4419b 100644 --- a/test/dom-file-panel-unsaved-guard.test.js +++ b/test/dom-file-panel-unsaved-guard.test.js @@ -43,7 +43,7 @@ function setup() { const dom = new JSDOM(INDEX_HTML, { url: 'http://localhost/', runScripts: 'outside-only', pretendToBeVisual: true }); const { window } = dom; const disk = new Map(); - const calls = { openFile: null, check: null, acks: [], answers: [], saves: [], confirms: [] }; + const calls = { openFile: null, check: null, acks: [], answers: [], saves: [], confirms: [], reasonDone: [] }; let editor = null; window.api = new Proxy({ @@ -52,6 +52,7 @@ function setup() { onUnsavedCheckReason: (cb) => { calls.reason = cb; }, unsavedCheckAck: (id) => { calls.acks.push({ id, at: calls.answers.length, dialog: !!window.document.getElementById('unsaved-edits-dialog') }); }, unsavedCheckResult: (id, proceed) => { calls.answers.push({ id, proceed }); }, + unsavedCheckReasonDone: (id) => { calls.reasonDone.push(id); }, watchFile: () => Promise.resolve({ ok: true }), unwatchFile: () => Promise.resolve({ ok: true }), readFileForPanel: (p) => Promise.resolve(disk.has(p) ? { ok: true, git: false, original: disk.get(p), current: disk.get(p) } : { ok: false, code: 'ENOENT', error: 'ENOENT' }), @@ -313,6 +314,43 @@ test('a quit that joins a reload check the page has already answered still write } finally { ctx.destroy(); } }); +test('the page tells main when the save a late upgrade asked for is done, and at once when it has none to do', async () => { + const ctx = setup(); + try { + let finish = null; + let fail = false; + ctx.window.flushStateForExit = () => new Promise((resolve, reject) => { finish = () => (fail ? reject(new Error('disk')) : resolve()); }); + ctx.calls.check(6, 'reload'); + await flush(); + ctx.calls.reason(6, 'quit'); + await flush(); + assert.deepEqual(ctx.calls.reasonDone, [], 'not before the save lands'); + finish(); + await flush(); + assert.deepEqual(ctx.calls.reasonDone, [6]); + + ctx.calls.check(7, 'reload'); + await flush(); + fail = true; + ctx.calls.reason(7, 'quit'); + await flush(); + finish(); + await flush(); + assert.deepEqual(ctx.calls.reasonDone, [6, 7], 'a failed save still lets the quit go'); + + ctx.calls.reason(99, 'quit'); + await flush(); + assert.deepEqual(ctx.calls.reasonDone, [6, 7, 99], 'an unknown check has nothing to wait for'); + + await dirtyTab(ctx, 's1', A); + ctx.calls.check(8, 'reload'); + await flush(); + ctx.calls.reason(8, 'quit'); + await flush(); + assert.deepEqual(ctx.calls.reasonDone, [6, 7, 99, 8], 'a check still asking saves before it answers, so main need not wait'); + } finally { ctx.destroy(); } +}); + test('a cancelled close does not flush', async () => { const ctx = setup(); try { diff --git a/test/unsaved-guard.test.js b/test/unsaved-guard.test.js index fbf65a17b..b26822cb1 100644 --- a/test/unsaved-guard.test.js +++ b/test/unsaved-guard.test.js @@ -39,7 +39,8 @@ function setup({ timeoutMs = 1000, quit, closeFinishes = false } = {}) { const closeEvent = () => ({ prevented: false, preventDefault() { this.prevented = true; } }); const answer = (id, proceed) => ipcMain.emit('unsaved-check-result', {}, id, proceed); const ack = (id) => ipcMain.emit('unsaved-check-ack', {}, id); - return { win, wc, sent, timers, closeEvent, answer, ack, ipcMain, guard }; + const reasonDone = (id) => ipcMain.emit('unsaved-check-reason-done', {}, id); + return { win, wc, sent, timers, closeEvent, answer, ack, reasonDone, ipcMain, guard }; } const tick = () => new Promise((r) => setImmediate(r)); @@ -279,6 +280,7 @@ test('a quit that joins an unanswered reload check tells the renderer it is now assert.deepEqual(t.sent.map((m) => m.channel), ['unsaved-check', 'unsaved-check-reason'], 'one upgrade, no second question'); assert.deepEqual(t.sent[1].args, [id, 'quit']); t.answer(id, true); + t.reasonDone(id); await tick(); assert.equal(quits.length, 1); }); @@ -290,11 +292,124 @@ test('a reload check that a quit joined quits without reloading first', async () const id = t.sent[0].args[0]; t.guard.beforeQuit(t.closeEvent(), t.win); t.answer(id, true); + t.reasonDone(id); await tick(); assert.equal(t.wc.reloads, 0); assert.equal(quits.length, 1); }); +test('a quit that joins a reload check waits for the page to finish the save that upgrade asks for, but not for ever', async () => { + const quits = []; + const t = setup({ quit: () => quits.push(1) }); + t.wc.emit('will-prevent-unload', t.closeEvent()); + const id = t.sent[0].args[0]; + t.guard.beforeQuit(t.closeEvent(), t.win); + t.answer(id, true); + await tick(); + assert.equal(quits.length, 0, 'the page may have answered before it saw the upgrade, and is still saving'); + t.reasonDone(id); + await tick(); + assert.equal(quits.length, 1); + + const silent = setup({ quit: () => quits.push(2) }); + silent.wc.emit('will-prevent-unload', silent.closeEvent()); + const sid = silent.sent[0].args[0]; + silent.guard.beforeQuit(silent.closeEvent(), silent.win); + silent.answer(sid, true); + await tick(); + assert.deepEqual(quits, [1]); + const bound = silent.timers.filter((x) => !x.cleared && x.ms === 1000); + assert.equal(bound.length, 1, 'one bounded wait'); + bound[0].fn(); + await tick(); + assert.deepEqual(quits, [1, 2], 'a page that never says it is done does not hold the quit'); + + const declined = setup({ quit: () => quits.push(3) }); + declined.wc.emit('will-prevent-unload', declined.closeEvent()); + const did = declined.sent[0].args[0]; + declined.guard.beforeQuit(declined.closeEvent(), declined.win); + declined.answer(did, false); + await tick(); + assert.equal(declined.timers.filter((x) => !x.cleared && x.ms === 1000).length, 0, 'a declined check leaves no wait behind'); + + const hung = setup({ quit: () => quits.push(4) }); + hung.wc.emit('will-prevent-unload', hung.closeEvent()); + hung.guard.beforeQuit(hung.closeEvent(), hung.win); + const checkBound = hung.timers.filter((x) => !x.cleared && x.ms === 1000)[0]; + checkBound.fn(); + await tick(); + assert.deepEqual(quits, [1, 2, 4], 'a page that never answered the check is not waited for a second time'); + assert.equal(hung.timers.filter((x) => !x.cleared && x.ms === 1000).length, 0); +}); + +test('a page that reports the upgrade done before it answers lets the joined quit go on the answer', async () => { + const quits = []; + const t = setup({ quit: () => quits.push(1) }); + t.wc.emit('will-prevent-unload', t.closeEvent()); + const id = t.sent[0].args[0]; + t.guard.beforeQuit(t.closeEvent(), t.win); + t.reasonDone(id); + await tick(); + assert.equal(quits.length, 0, 'the question is still open'); + t.answer(id, true); + await tick(); + assert.equal(quits.length, 1, 'no wait for a report already received'); + assert.equal(t.timers.filter((x) => !x.cleared && x.ms === 1000).length, 0, 'no bounded wait left running'); +}); + +test('a close or quit arriving while a joined quit waits for the late save joins it instead of asking again', async () => { + const quits = []; + const t = setup({ quit: () => quits.push(1) }); + t.wc.emit('will-prevent-unload', t.closeEvent()); + const id = t.sent[0].args[0]; + t.guard.beforeQuit(t.closeEvent(), t.win); + t.answer(id, true); + await tick(); + const close = t.closeEvent(); + t.win.emit('close', close); + const install = t.guard.confirmQuit(t.win); + await tick(); + assert.equal(close.prevented, true); + assert.deepEqual(t.sent.map((m) => m.channel), ['unsaved-check', 'unsaved-check-reason'], 'no second question while the save is pending'); + assert.equal(quits.length, 0); + t.reasonDone(id); + await tick(); + assert.equal(quits.length, 1); + assert.equal(await install, true); + assert.equal(t.win.closes, 1, 'the held close goes through once the save is done'); +}); + +test('a joined quit the page declined leaves nothing for a later close to join', async () => { + const t = setup(); + t.wc.emit('will-prevent-unload', t.closeEvent()); + t.guard.beforeQuit(t.closeEvent(), t.win); + t.answer(t.sent[0].args[0], false); + await tick(); + t.win.emit('close', t.closeEvent()); + const checks = t.sent.filter((m) => m.channel === 'unsaved-check'); + assert.equal(checks.length, 2, 'the later close is asked about'); + t.answer(checks[1].args[0], true); + await tick(); + assert.equal(t.win.closes, 1); +}); + +test('a page that dies while a joined quit waits for its late save releases the quit at once', async () => { + for (const event of ['render-process-gone', 'destroyed']) { + const quits = []; + const t = setup({ quit: () => quits.push(1) }); + t.wc.emit('will-prevent-unload', t.closeEvent()); + const id = t.sent[0].args[0]; + t.guard.beforeQuit(t.closeEvent(), t.win); + t.answer(id, true); + await tick(); + assert.equal(quits.length, 0); + t.wc.emit(event); + await tick(); + assert.equal(quits.length, 1, `released on ${event}`); + assert.equal(t.timers.filter((x) => !x.cleared && x.ms === 1000).length, 0, 'no bounded wait left running'); + } +}); + test('a reload joining a quit check leaves it a quit', () => { const t = setup(); t.win.emit('close', t.closeEvent()); diff --git a/unsaved-guard.js b/unsaved-guard.js index 639ec7416..55c4d62ad 100644 --- a/unsaved-guard.js +++ b/unsaved-guard.js @@ -31,11 +31,50 @@ function createUnsavedGuard({ ipcMain, timeoutMs = DEFAULT_TIMEOUT_MS, logoffCan entry.finish(proceed === true); }); + const reasonWaits = new Map(); + ipcMain.on('unsaved-check-reason-done', (_event, id) => { + const done = reasonWaits.get(id); + if (done) done(); + }); + + function waitReasonDone(id, wc) { + let release = null; + const done = new Promise((resolve) => { + const timer = setTimeoutFn(() => release(), timeoutMs); + const onGone = () => release(); + release = () => { + reasonWaits.delete(id); + clearTimeoutFn(timer); + wc.removeListener('render-process-gone', onGone); + wc.removeListener('destroyed', onGone); + resolve(); + }; + reasonWaits.set(id, release); + wc.on('render-process-gone', onGone); + wc.on('destroyed', onGone); + }); + return { done, release }; + } + function askFull(win, reason) { if (inflight) { if (inflight.reason === 'reload') { - inflight.reason = reason; - try { win.webContents.send('unsaved-check-reason', inflight.id, reason); } catch {} + const joined = inflight; + joined.reason = reason; + joined.held = true; + const { id } = joined; + const wait = waitReasonDone(id, win.webContents); + try { win.webContents.send('unsaved-check-reason', id, reason); } catch { wait.release(); } + joined.asked = joined.asked.then((result) => { + if (!result.proceed || result.unanswered) { + wait.release(); + return result; + } + return wait.done.then(() => result); + }).then((result) => { + if (inflight === joined) inflight = null; + return result; + }); } return inflight.asked; } @@ -53,7 +92,7 @@ function createUnsavedGuard({ ipcMain, timeoutMs = DEFAULT_TIMEOUT_MS, logoffCan wc.removeListener('render-process-gone', onGone); wc.removeListener('destroyed', onGone); settled = true; - inflight = null; + if (inflight && inflight.id === id && !inflight.held) inflight = null; resolve({ proceed, unanswered }); }; entry.timer = setTimeoutFn(() => entry.finish(true, true), timeoutMs);