From feaddf187e25b5d0af8bc16fa872d59b1d667f30 Mon Sep 17 00:00:00 2001 From: alpha Date: Thu, 27 Aug 2026 23:13:45 -0400 Subject: [PATCH] fix: close two holes left by the update-install bypass `quitAndInstall` reports nothing when an install fails - it just does not quit - and the handler was reporting success either way, so one failed update left `closeConfirmed` set for the rest of the session and the next close took the interview with it silently. That is the failure the guard exists to stop, so `rearmCloseGuardIfStillRunning()` puts it back unless `before-quit` has fired. Keyed on `quitting` rather than a window count: re-arming a quit that is under way would veto the close it just approved. And `UpdateNotification` was reading `hasHistory` through `useSaveHistoryGuard`, which subscribes to the app state - a new object several times a second during an interview. The component re-rendered and re-armed its status effect on every ASR partial to answer a question it only asks on a click. It reads the flag over `appState.get()` at that click instead, which is also where the flag is derived rather than one broadcast behind. Co-Authored-By: Claude Opus 5 --- CLAUDE.md | 18 +++++-- src/main/ipc/auto-updater.ts | 19 ++++++-- src/main/window-close-guard.ts | 21 +++++++++ .../components/custom/update-notification.tsx | 47 +++++++++++-------- test/save-history.test.mjs | 26 +++++++--- 5 files changed, 96 insertions(+), 35 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 614beed..570891f 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -181,10 +181,20 @@ an app that refuses to exit, which on Windows ends with the installer killing it lost anyway, and the prompt asking about it was on screen for a second. So the updater IPC handler calls `allowNextClose()` before it installs, and `rearmCloseGuard()` if nothing was launched (`quitAndInstall` returns whether it committed to quitting, which the macOS "no downloaded file" -path does not). The question is asked one layer up instead, in `update-notification.tsx`, in front -of the install rather than behind it. `confirmDiscard` is held in a ref there: it closes over the -app state, so it is a new function on every broadcast from main, and naming it as a dependency -would re-run the update-status effect several times a second during an interview. +path does not). + +**A failed install reports nothing - it simply does not quit** - so `rearmCloseGuardIfStillRunning()` +puts the guard back unless `before-quit` has fired by then. Without it one failed update disarms +the guard for the rest of the session and the next close takes the interview with it, which is +precisely what the guard exists to stop. The test is `quitting` rather than a window count, +because re-arming a quit that *is* under way turns the guard into a veto of the close it just +approved. + +The question is asked one layer up, in `update-notification.tsx`, in front of the install rather +than behind it. It reads `hasHistory` over `appState.get()` on the click instead of through +`useSaveHistoryGuard`: that hook subscribes to the app state, which during an interview is a new +object several times a second, and this component would then re-render and re-arm its status +effect on every ASR partial to answer a question it only asks when a button is pressed. ### Interview language diff --git a/src/main/ipc/auto-updater.ts b/src/main/ipc/auto-updater.ts index 71d427a..371e9fd 100644 --- a/src/main/ipc/auto-updater.ts +++ b/src/main/ipc/auto-updater.ts @@ -1,7 +1,11 @@ import { ipcMain } from 'electron'; import { autoUpdaterService } from '../services/auto-updater.service.js'; -import { allowNextClose, rearmCloseGuard } from '../window-close-guard.js'; +import { + allowNextClose, + rearmCloseGuard, + rearmCloseGuardIfStillRunning, +} from '../window-close-guard.js'; export function registerAutoUpdaterHandlers(): void { ipcMain.handle('auto-updater:check-for-updates', async () => { @@ -25,9 +29,16 @@ export function registerAutoUpdaterHandlers(): void { allowNextClose(); try { const quitting = await autoUpdaterService.quitAndInstall(); - // Nothing was launched, so the app is staying and the guard goes back on. - if (!quitting) rearmCloseGuard(); - return { success: quitting }; + if (!quitting) { + // Nothing was launched, so the app is staying and the guard goes back on. + rearmCloseGuard(); + return { success: false }; + } + + // The install was started, but electron-updater says nothing when one fails - it just + // does not quit. So the guard is put back unless the quit really begins. + rearmCloseGuardIfStillRunning(); + return { success: true }; } catch (error) { console.error('[IPC] Failed to quit and install:', error); rearmCloseGuard(); diff --git a/src/main/window-close-guard.ts b/src/main/window-close-guard.ts index 21e7758..81c3f8a 100644 --- a/src/main/window-close-guard.ts +++ b/src/main/window-close-guard.ts @@ -76,6 +76,27 @@ export function rearmCloseGuard(): void { closeConfirmed = false; } +// Long enough for `app.quit()` to have emitted `before-quit`, which happens in the tick after +// the installer is spawned. Nothing waits on this, so being generous costs nothing. +const QUIT_GRACE_MS = 5_000; + +/** + * Re-arm unless the quit actually started. + * + * `autoUpdater.quitAndInstall()` reports nothing back when the install fails - it simply does + * not quit - so this is the only way to notice that the app is still here. Left unchecked, one + * failed update would disarm the guard for the rest of the session and the next close would + * take the interview with it, silently, which is the whole thing this file exists to stop. + * + * `quitting` is the test rather than a window count, because re-arming a quit that *is* under + * way would turn the guard into a veto of the close it had just approved. + */ +export function rearmCloseGuardIfStillRunning(): void { + setTimeout(() => { + if (!quitting) closeConfirmed = false; + }, QUIT_GRACE_MS).unref(); +} + export function registerCloseGuardHandlers(): void { app.on('before-quit', () => { quitting = true; diff --git a/src/renderer/components/custom/update-notification.tsx b/src/renderer/components/custom/update-notification.tsx index fdaaf26..62607c1 100644 --- a/src/renderer/components/custom/update-notification.tsx +++ b/src/renderer/components/custom/update-notification.tsx @@ -1,23 +1,37 @@ -import { useEffect, useRef } from 'react'; +import { useCallback, useEffect, useRef } from 'react'; import { toast } from 'sonner'; import { UpdateStatus, useAutoUpdater } from '@/hooks/use-auto-updater'; -import { useSaveHistoryGuard } from '@/hooks/use-save-history-guard'; +import { useSaveHistoryPrompt } from '@/hooks/use-save-history-guard'; +import { getElectron } from '@/lib/utils'; export function UpdateNotification() { const { updateStatus, quitAndInstall } = useAutoUpdater(); - const { confirmDiscard } = useSaveHistoryGuard(); const lastStatusRef = useRef(null); const downloadToastIdRef = useRef(null); - // Held in a ref rather than listed as a dependency. `confirmDiscard` closes over the current - // app state, so it is a new function on every broadcast from main - several a second during - // an interview - and naming it in the effect below would re-run the whole status machine - // that often. - const confirmDiscardRef = useRef(confirmDiscard); - useEffect(() => { - confirmDiscardRef.current = confirmDiscard; - }); + /** + * Installing takes the app down, so it loses the interview exactly as closing does - and + * unlike a close it cannot be vetoed once started, because the installer is launched before + * the quit is requested. So the question is asked here, in front of it. + * + * `hasHistory` is read from main on the click rather than through `useSaveHistoryGuard`. + * That hook subscribes to the app state, which during an interview is a new object several + * times a second, and this component would then re-render - and re-arm its status effect - + * on every ASR partial to answer a question it only asks when a button is pressed. One round + * trip on the click also reads the flag where it is derived rather than a broadcast behind. + */ + const confirmThenInstall = useCallback(async () => { + const electron = getElectron(); + const state = await electron?.appState.get(); + + if (state?.hasHistory) { + const proceed = await useSaveHistoryPrompt.getState().prompt('update'); + if (!proceed) return; + } + + await quitAndInstall(); + }, [quitAndInstall]); useEffect(() => { if (!updateStatus) return; @@ -76,14 +90,7 @@ export function UpdateNotification() { duration: Infinity, action: { label: isMac ? 'Open Installer' : 'Restart Now', - // Installing takes the app down, so it loses the interview exactly as closing - // does - and the install cannot be vetoed once started, because the installer is - // launched before the quit. So the question is asked here, in front of it. - onClick: () => { - void confirmDiscardRef.current('update').then((proceed) => { - if (proceed) void quitAndInstall(); - }); - }, + onClick: () => void confirmThenInstall(), }, }); } @@ -97,7 +104,7 @@ export function UpdateNotification() { console.error('[UpdateNotification] Update error:', error); break; } - }, [updateStatus, quitAndInstall]); + }, [updateStatus, confirmThenInstall]); return null; } diff --git a/test/save-history.test.mjs b/test/save-history.test.mjs index 8e41dfb..3b3baaa 100644 --- a/test/save-history.test.mjs +++ b/test/save-history.test.mjs @@ -7,7 +7,7 @@ * writes rather than the array lengths, and that an export refuses the placeholder - which the * length check let through, producing a billed summary of "Transcripts will be here". */ -import { createChecker, loadMain, readSource } from './helpers.mjs'; +import { codeOnly, createChecker, loadMain, readSource } from './helpers.mjs'; export async function run() { const { check, failures } = createChecker('save-history'); @@ -134,9 +134,18 @@ export async function run() { ); check( 'and re-arms it when nothing was launched', - updaterIpc.includes('if (!quitting) rearmCloseGuard();') && - updaterIpc.includes('catch') && - updaterIpc.split('rearmCloseGuard()').length === 3 + updaterIpc.includes('rearmCloseGuard();') && updaterIpc.includes('return { success: false };') + ); + // electron-updater says nothing when an install fails - it simply does not quit - so without + // this one failed update disarms the guard for the rest of the session and the next close + // takes the interview with it. + check( + 'and re-arms it when the install was started but never quit', + updaterIpc.includes('rearmCloseGuardIfStillRunning();') + ); + check( + 'the delayed re-arm stands down for a quit that did start', + guard.includes('if (!quitting) closeConfirmed = false;') ); const updateToast = readSource( @@ -144,11 +153,14 @@ export async function run() { ); check( 'the install asks about an unsaved interview first', - updateToast.includes("confirmDiscardRef.current('update')") + updateToast.includes("useSaveHistoryPrompt.getState().prompt('update')") ); + check('and only installs when the answer is yes', updateToast.includes('if (!proceed) return;')); + // Reading it off the subscribed app state would re-render this component - and re-arm its + // status effect - on every ASR partial, to answer a question it only asks on a click. check( - 'and only installs when the answer is yes', - updateToast.includes('if (proceed) void quitAndInstall();') + 'and does not subscribe to the app state to find that out', + !codeOnly(updateToast).includes('useSaveHistoryGuard') && updateToast.includes('appState.get()') ); return failures;