From 524cf578b8124b7abd77d367fc75b23e52cac757 Mon Sep 17 00:00:00 2001 From: 4gray Date: Sun, 9 Aug 2026 15:06:13 +0200 Subject: [PATCH] fix(settings): number close requests and abort installs only on error MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two review-round hardenings for the close guard's edge interleavings: - Close requests now carry a per-interception id and Stay's cancellation cites the one its dialog answered. A cancellation that lost the race against a fresh Cmd+Q is ignored as stale, so the newer quit intent survives and the follow-up dialog's Save quits instead of silently downgrading to a window close. - The install coordinator's push-based abort now fires only on an Error status: a benign push — like the 'checking' from an update check clicked while the updater quit winds up — no longer resurrects the beforeunload handler mid-install and strands the installation. Co-Authored-By: Claude Fable 5 --- .../src/app/api/main.preload.ts | 10 ++++-- .../window-close-guard.service.spec.ts | 34 ++++++++++++++++++- .../services/window-close-guard.service.ts | 24 ++++++++++--- .../app-update-install.service.spec.ts | 21 ++++++++++++ .../services/app-update-install.service.ts | 10 ++++-- .../settings-unload-guard.service.spec.ts | 31 ++++++++++------- .../settings/settings-unload-guard.service.ts | 12 +++++-- .../src/lib/electron-api.interface.ts | 8 +++-- 8 files changed, 121 insertions(+), 29 deletions(-) diff --git a/apps/electron-backend/src/app/api/main.preload.ts b/apps/electron-backend/src/app/api/main.preload.ts index b4602eba4..0f4298aec 100644 --- a/apps/electron-backend/src/app/api/main.preload.ts +++ b/apps/electron-backend/src/app/api/main.preload.ts @@ -425,9 +425,13 @@ const electronApi: ElectronBridgeApi = { setWindowCloseGuard: (active: boolean) => ipcRenderer.invoke(WINDOW_SET_CLOSE_GUARD, active), confirmWindowClose: () => ipcRenderer.invoke(WINDOW_CONFIRM_CLOSE), - cancelWindowClose: () => ipcRenderer.invoke(WINDOW_CANCEL_CLOSE), - onWindowCloseRequested: (callback: () => void) => { - const handler = () => callback(); + cancelWindowClose: (requestId?: number) => + ipcRenderer.invoke(WINDOW_CANCEL_CLOSE, requestId), + onWindowCloseRequested: (callback: (requestId: number) => void) => { + const handler = ( + _event: Electron.IpcRendererEvent, + requestId: number + ) => callback(requestId); ipcRenderer.on(WINDOW_CLOSE_REQUESTED, handler); return () => ipcRenderer.off(WINDOW_CLOSE_REQUESTED, handler); }, diff --git a/apps/electron-backend/src/app/services/window-close-guard.service.spec.ts b/apps/electron-backend/src/app/services/window-close-guard.service.spec.ts index ee5a43a2b..f679a5c20 100644 --- a/apps/electron-backend/src/app/services/window-close-guard.service.spec.ts +++ b/apps/electron-backend/src/app/services/window-close-guard.service.spec.ts @@ -145,7 +145,8 @@ describe('WindowCloseGuard', () => { expect(win.fireClose()).toBe(true); expect(win.webContents.send).toHaveBeenCalledWith( - WINDOW_CLOSE_REQUESTED + WINDOW_CLOSE_REQUESTED, + 1 ); expect(win.close).not.toHaveBeenCalled(); }); @@ -191,6 +192,37 @@ describe('WindowCloseGuard', () => { expect(app.quit).not.toHaveBeenCalled(); }); + it('ignores a cancellation citing an older interception', () => { + // Stay's cancel raced a fresh Cmd+Q: the late cancellation must not + // wipe the newer quit intent, or Save on the follow-up dialog would + // downgrade the quit to a plain window close. + const { app, guard, win } = createArmedGuard(); + + win.fireClose(); // request 1 + app.fireBeforeQuit(); + win.fireClose(); // request 2, quit + guard.cancelClose(1); + + guard.confirmClose(); + + expect(app.quit).toHaveBeenCalledTimes(1); + expect(win.close).not.toHaveBeenCalled(); + }); + + it('honors a cancellation citing the current interception', () => { + const { app, guard, win } = createArmedGuard(); + + app.fireBeforeQuit(); + win.fireClose(); // request 1, quit + guard.cancelClose(1); + + win.fireClose(); // request 2 + guard.confirmClose(); + + expect(win.close).toHaveBeenCalledTimes(1); + expect(app.quit).not.toHaveBeenCalled(); + }); + it('keeps a pending quit when close is clicked again before the user decides', () => { const { app, guard, win } = createArmedGuard(); diff --git a/apps/electron-backend/src/app/services/window-close-guard.service.ts b/apps/electron-backend/src/app/services/window-close-guard.service.ts index 3cf80b911..48fe53732 100644 --- a/apps/electron-backend/src/app/services/window-close-guard.service.ts +++ b/apps/electron-backend/src/app/services/window-close-guard.service.ts @@ -37,7 +37,7 @@ export interface CloseGuardWindow { on(event: string, listener: (...args: unknown[]) => void): unknown; webContents: { isDestroyed(): boolean; - send(channel: string): void; + send(channel: string, requestId?: number): void; on(event: string, listener: (...args: unknown[]) => void): unknown; }; } @@ -47,6 +47,8 @@ export class WindowCloseGuard { private bypassClose = false; private quitInProgress = false; private pendingIntent: CloseIntent | null = null; + /** Increments per interception; cancellations must cite the current one. */ + private requestSeq = 0; private window: CloseGuardWindow | null = null; constructor(private readonly electronApp: CloseGuardApp) {} @@ -160,8 +162,17 @@ export class WindowCloseGuard { * Renderer verdict: the user stays. Clears the pending intent so a later * close attempt starts fresh — without this, choosing Stay on a quit and * clicking the window's close button minutes later would quit the app. + * + * A cancellation citing an older request than the current interception + * is stale and ignored: a quit intercepted while the cancel was in + * flight must keep its intent, or Save on the follow-up dialog would + * downgrade the user's quit to a plain window close. */ - cancelClose(): void { + cancelClose(requestId?: number): void { + if (requestId !== undefined && requestId !== this.requestSeq) { + return; + } + this.pendingIntent = null; } @@ -198,7 +209,8 @@ export class WindowCloseGuard { } else { this.pendingIntent = this.pendingIntent ?? 'close'; } - win.webContents.send(WINDOW_CLOSE_REQUESTED); + this.requestSeq += 1; + win.webContents.send(WINDOW_CLOSE_REQUESTED, this.requestSeq); } } @@ -227,8 +239,10 @@ export function bootstrapWindowCloseGuard( ipcMain.handle(WINDOW_CONFIRM_CLOSE, () => { guard.confirmClose(); }); - ipcMain.handle(WINDOW_CANCEL_CLOSE, () => { - guard.cancelClose(); + ipcMain.handle(WINDOW_CANCEL_CLOSE, (_event, requestId?: number) => { + guard.cancelClose( + typeof requestId === 'number' ? requestId : undefined + ); }); return guard; diff --git a/apps/web/src/app/services/app-update-install.service.spec.ts b/apps/web/src/app/services/app-update-install.service.spec.ts index 44ba7da86..dd2639428 100644 --- a/apps/web/src/app/services/app-update-install.service.spec.ts +++ b/apps/web/src/app/services/app-update-install.service.spec.ts @@ -129,6 +129,27 @@ describe('AppUpdateInstallService', () => { expect(guard.resumeAfterAbortedAppQuit).toHaveBeenCalledTimes(1); }); + it('ignores benign pushes while the install quit is pending', async () => { + // A 'checking' push (update check clicked while the quit winds up) + // proves nothing about the quit; resuming the beforeunload handler + // on it would cancel the updater's window close and strand the + // install. + electronStub.installAppUpdate.mockResolvedValue({ + ...BASE_STATUS, + status: ELECTRON_BRIDGE_APP_UPDATE_STATUSES.Downloaded, + }); + const service = createService(); + service.registerUnloadGuard(guard); + await service.installAppUpdate(); + + pushStatus({ + ...BASE_STATUS, + status: ELECTRON_BRIDGE_APP_UPDATE_STATUSES.Checking, + }); + + expect(guard.resumeAfterAbortedAppQuit).not.toHaveBeenCalled(); + }); + it('survives an error push that beats the install reply', async () => { // IPC ordering between the invoke reply and status pushes is not // guaranteed; a stale 'downloaded' reply must not re-suspend the diff --git a/apps/web/src/app/services/app-update-install.service.ts b/apps/web/src/app/services/app-update-install.service.ts index 1916f11c4..cdc41ad70 100644 --- a/apps/web/src/app/services/app-update-install.service.ts +++ b/apps/web/src/app/services/app-update-install.service.ts @@ -34,12 +34,16 @@ export class AppUpdateInstallService { constructor() { // App-lifetime subscription on purpose: the failure push can arrive - // after the UI that requested the install is gone. + // after the UI that requested the install is gone. Only an Error + // push aborts — that is how electron-updater reports an install + // failure. Benign pushes (e.g. a 'checking' from an update-check + // clicked while the quit is still winding up) prove nothing about + // the quit and must not resurrect the beforeunload handler mid- + // install. window.electron?.onAppUpdateStatusChange?.((status) => { if ( this.installQuitPending && - status.status !== - ELECTRON_BRIDGE_APP_UPDATE_STATUSES.Downloaded + status.status === ELECTRON_BRIDGE_APP_UPDATE_STATUSES.Error ) { this.abortInstallQuit(); } diff --git a/apps/web/src/app/settings/settings-unload-guard.service.spec.ts b/apps/web/src/app/settings/settings-unload-guard.service.spec.ts index 28bc0e1ae..fe6185755 100644 --- a/apps/web/src/app/settings/settings-unload-guard.service.spec.ts +++ b/apps/web/src/app/settings/settings-unload-guard.service.spec.ts @@ -19,7 +19,7 @@ describe('SettingsUnloadGuardService', () => { let host: SettingsUnloadGuardHost & { confirmClose: jest.Mock }; const originalElectron = window.electron; - let closeRequestCallback: (() => void) | null; + let closeRequestCallback: ((requestId: number) => void) | null; let unsubscribeCloseRequests: jest.Mock; let electronStub: { setWindowCloseGuard: jest.Mock; @@ -55,10 +55,12 @@ describe('SettingsUnloadGuardService', () => { status: 'idle', supportedSelfUpdate: true, }), - onWindowCloseRequested: jest.fn((callback: () => void) => { - closeRequestCallback = callback; - return unsubscribeCloseRequests; - }), + onWindowCloseRequested: jest.fn( + (callback: (requestId: number) => void) => { + closeRequestCallback = callback; + return unsubscribeCloseRequests; + } + ), }; }); @@ -210,7 +212,7 @@ describe('SettingsUnloadGuardService', () => { activateInElectron(); form.markAsDirty(); - closeRequestCallback?.(); + closeRequestCallback?.(1); await flushAsyncWork(); expect(host.confirmClose).toHaveBeenCalledTimes(1); @@ -222,7 +224,7 @@ describe('SettingsUnloadGuardService', () => { activateInElectron(); form.markAsDirty(); - closeRequestCallback?.(); + closeRequestCallback?.(1); await flushAsyncWork(); expect(electronStub.confirmWindowClose).not.toHaveBeenCalled(); @@ -251,7 +253,7 @@ describe('SettingsUnloadGuardService', () => { expect(host.confirmClose).toHaveBeenCalledTimes(1); // ...then the user closes the window while it is on screen. - closeRequestCallback?.(); + closeRequestCallback?.(1); resolveConfirm?.(true); await flushAsyncWork(); @@ -273,7 +275,7 @@ describe('SettingsUnloadGuardService', () => { dispatchBeforeUnload(); await flushAsyncWork(); - closeRequestCallback?.(); + closeRequestCallback?.(1); resolveConfirm?.(false); await flushAsyncWork(); @@ -300,18 +302,21 @@ describe('SettingsUnloadGuardService', () => { activateInElectron(); form.markAsDirty(); - closeRequestCallback?.(); + closeRequestCallback?.(1); await flushAsyncWork(); expect(host.confirmClose).toHaveBeenCalledTimes(1); // Second close while the cancel acknowledgment is pending. - closeRequestCallback?.(); + closeRequestCallback?.(2); await flushAsyncWork(); expect(host.confirmClose).toHaveBeenCalledTimes(1); resolveCancel?.(); await flushAsyncWork(); + // The cancellation cited the request its dialog answered, so a + // newer interception's intent survives it in the main process. + expect(electronStub.cancelWindowClose).toHaveBeenCalledWith(1); expect(host.confirmClose).toHaveBeenCalledTimes(2); expect(electronStub.confirmWindowClose).toHaveBeenCalledTimes(1); }); @@ -327,8 +332,8 @@ describe('SettingsUnloadGuardService', () => { activateInElectron(); form.markAsDirty(); - closeRequestCallback?.(); - closeRequestCallback?.(); + closeRequestCallback?.(1); + closeRequestCallback?.(1); await flushAsyncWork(); expect(host.confirmClose).toHaveBeenCalledTimes(1); diff --git a/apps/web/src/app/settings/settings-unload-guard.service.ts b/apps/web/src/app/settings/settings-unload-guard.service.ts index f9f681022..c487f04a3 100644 --- a/apps/web/src/app/settings/settings-unload-guard.service.ts +++ b/apps/web/src/app/settings/settings-unload-guard.service.ts @@ -56,6 +56,11 @@ export class SettingsUnloadGuardService implements OnDestroy { private cancelling = false; /** A close requested during that window, re-asked once the cancel lands. */ private queuedCloseRequest = false; + /** + * Id of the latest intercepted request, cited in cancellations so a + * cancel arriving after a newer interception cannot wipe its intent. + */ + private latestCloseRequestId: number | undefined; /** Last guard state mirrored to the main process. */ private guardArmed = false; /** True while an updater-driven app quit must pass unchallenged. */ @@ -71,7 +76,8 @@ export class SettingsUnloadGuardService implements OnDestroy { window.addEventListener('beforeunload', this.beforeUnloadHandler); this.unsubscribeCloseRequests = - window.electron?.onWindowCloseRequested?.(() => { + window.electron?.onWindowCloseRequested?.((requestId) => { + this.latestCloseRequestId = requestId; this.zone.run(() => void this.handleCloseRequest('close')); }) ?? null; @@ -208,7 +214,9 @@ export class SettingsUnloadGuardService implements OnDestroy { this.cancelling = true; try { - await window.electron?.cancelWindowClose?.(); + await window.electron?.cancelWindowClose?.( + this.latestCloseRequestId + ); } finally { this.cancelling = false; } diff --git a/libs/shared/interfaces/src/lib/electron-api.interface.ts b/libs/shared/interfaces/src/lib/electron-api.interface.ts index 494e3faa4..33bba2ce6 100644 --- a/libs/shared/interfaces/src/lib/electron-api.interface.ts +++ b/libs/shared/interfaces/src/lib/electron-api.interface.ts @@ -632,9 +632,13 @@ export interface ElectronBridgeApi { /** * Abandons a close the guard intercepted (the user stays), so the * remembered close-vs-quit intent cannot leak into a later attempt. + * Carries the id of the request being abandoned: a cancellation that + * arrives after a NEWER interception must not wipe that newer intent. */ - cancelWindowClose: () => Promise; - onWindowCloseRequested: (callback: () => void) => () => void; + cancelWindowClose: (requestId?: number) => Promise; + onWindowCloseRequested: ( + callback: (requestId: number) => void + ) => () => void; fetchPlaylistByUrl: ( url: string, title?: string,