diff --git a/apps/electron-backend/src/app/services/app-update.service.spec.ts b/apps/electron-backend/src/app/services/app-update.service.spec.ts index 07a5b9b24..ae266b8b6 100644 --- a/apps/electron-backend/src/app/services/app-update.service.spec.ts +++ b/apps/electron-backend/src/app/services/app-update.service.spec.ts @@ -506,4 +506,32 @@ describe('AppUpdateService', () => { expect(cancelPreparedQuit).toHaveBeenCalledTimes(1); expect(status.status).toBe(ELECTRON_BRIDGE_APP_UPDATE_STATUSES.Error); }); + + it('takes the bypass back when the updater emits the failure as an error event', () => { + // electron-updater's BaseUpdater catches synchronous install + // failures internally and emits 'error' instead of throwing, and + // MacUpdater can fail after quitAndInstall already returned — the + // revocation must ride the error path. + const cancelPreparedQuit = jest.fn(); + const { service } = createService({ cancelPreparedQuit }); + service.handleUpdateAvailable({ version: '0.23.0' }); + service.handleUpdateDownloaded({ version: '0.23.0' }); + + service.installUpdate(); + expect(cancelPreparedQuit).not.toHaveBeenCalled(); + + // What attachUpdaterEvents forwards from the updater 'error' event. + service.handleError(new Error('ShipIt failed')); + + expect(cancelPreparedQuit).toHaveBeenCalledTimes(1); + }); + + it('does not revoke a bypass for errors unrelated to an install', () => { + const cancelPreparedQuit = jest.fn(); + const { service } = createService({ cancelPreparedQuit }); + + service.handleError(new Error('check failed')); + + expect(cancelPreparedQuit).not.toHaveBeenCalled(); + }); }); diff --git a/apps/electron-backend/src/app/services/app-update.service.ts b/apps/electron-backend/src/app/services/app-update.service.ts index 78b5fb0ac..6fe2d9cd7 100644 --- a/apps/electron-backend/src/app/services/app-update.service.ts +++ b/apps/electron-backend/src/app/services/app-update.service.ts @@ -253,6 +253,8 @@ export class AppUpdateService { private checkForUpdatesPromise: Promise | null = null; private status: ElectronBridgeAppUpdateStatus; + /** True between prepareQuit() and the quit — or the error that voids it. */ + private quitPreparationPending = false; constructor(private readonly options: AppUpdateServiceOptions) { this.currentVersion = resolveCurrentVersion( @@ -414,15 +416,17 @@ export class AppUpdateService { this.status.status === ELECTRON_BRIDGE_APP_UPDATE_STATUSES.Downloaded ) { + // Consumed by handleError: electron-updater's BaseUpdater + // catches its own synchronous install failures and emits + // 'error' instead of throwing, and MacUpdater can return here + // and fail asynchronously — the revocation must ride the error + // path, not a try/catch around this call. + this.quitPreparationPending = true; this.options.prepareQuit?.(); try { this.updater.quitAndInstall(); } catch (error) { - // No quit is coming: take back the close-guard bypass and - // report the failure as a status the renderer can read — - // non-Downloaded also tells it to restore its own guard. - this.options.cancelPreparedQuit?.(); this.handleError(error); } } @@ -471,6 +475,14 @@ export class AppUpdateService { } handleError(error: unknown): void { + // An error after a prepared quit means no quit is coming: take back + // the one-shot close-guard bypass, whether the failure was thrown + // synchronously or emitted later as an updater 'error' event. + if (this.quitPreparationPending) { + this.quitPreparationPending = false; + this.options.cancelPreparedQuit?.(); + } + this.setStatus({ error: normalizeError(error), status: ELECTRON_BRIDGE_APP_UPDATE_STATUSES.Error, diff --git a/apps/web/src/app/settings/settings-app-update.facade.spec.ts b/apps/web/src/app/settings/settings-app-update.facade.spec.ts index 801d20748..3ce5afea3 100644 --- a/apps/web/src/app/settings/settings-app-update.facade.spec.ts +++ b/apps/web/src/app/settings/settings-app-update.facade.spec.ts @@ -195,6 +195,41 @@ describe('SettingsAppUpdateFacade', () => { ); }); + it('restores the unload guard when an error push follows a quitting install', async () => { + // electron-updater reports install failures as a later 'error' + // status event, not as a rejection of the install IPC — the app is + // then still running with the protection suspended. + const unloadGuard = TestBed.inject(SettingsUnloadGuardService); + (window.electron.installAppUpdate as jest.Mock).mockResolvedValue({ + ...DEFAULT_APP_UPDATE_STATUS, + status: ELECTRON_BRIDGE_APP_UPDATE_STATUSES.Downloaded, + }); + facade.init(); + const pushStatus = ( + window.electron.onAppUpdateStatusChange as jest.Mock + ).mock.calls[0][0] as ( + status: ElectronBridgeAppUpdateStatus + ) => void; + + await facade.installAppUpdate(); + expect(unloadGuard.resumeAfterAbortedAppQuit).not.toHaveBeenCalled(); + + pushStatus({ + ...DEFAULT_APP_UPDATE_STATUS, + status: ELECTRON_BRIDGE_APP_UPDATE_STATUSES.Error, + }); + + expect(unloadGuard.resumeAfterAbortedAppQuit).toHaveBeenCalledTimes( + 1 + ); + + // The resume is one-shot: later unrelated pushes change nothing. + pushStatus(DEFAULT_APP_UPDATE_STATUS); + expect(unloadGuard.resumeAfterAbortedAppQuit).toHaveBeenCalledTimes( + 1 + ); + }); + it('opens the manual release URL from unsupported update status', () => { const openSpy = jest.spyOn(window, 'open').mockReturnValue(null); facade.status.set({ diff --git a/apps/web/src/app/settings/settings-app-update.facade.ts b/apps/web/src/app/settings/settings-app-update.facade.ts index 1e1a216b5..81e09f27e 100644 --- a/apps/web/src/app/settings/settings-app-update.facade.ts +++ b/apps/web/src/app/settings/settings-app-update.facade.ts @@ -37,6 +37,8 @@ export class SettingsAppUpdateFacade { readonly updateMessage = signal(''); private unsubscribeStatus: (() => void) | null = null; + /** True after an install reply promised a quit that has not happened. */ + private installQuitPending = false; /** Subscribes to status pushes and kicks off the initial status load */ init(): void { @@ -83,9 +85,15 @@ export class SettingsAppUpdateFacade { this.status.set(status); if ( - status?.status !== + status?.status === ELECTRON_BRIDGE_APP_UPDATE_STATUSES.Downloaded ) { + // quitAndInstall ran, but electron-updater reports its + // failures as a later 'error' status push rather than here — + // the status stream (bindStatusEvents) resumes the guard if + // that happens instead of a quit. + this.installQuitPending = true; + } else { this.unloadGuard.resumeAfterAbortedAppQuit(); } } catch (error) { @@ -189,6 +197,18 @@ export class SettingsAppUpdateFacade { this.unsubscribeStatus = window.electron.onAppUpdateStatusChange( (status) => { this.status.set(status); + + // Any post-install status other than the quitting install's + // own proves the app is still running — the promised quit is + // not coming, so the unload protection returns. + if ( + this.installQuitPending && + status.status !== + ELECTRON_BRIDGE_APP_UPDATE_STATUSES.Downloaded + ) { + this.installQuitPending = false; + this.unloadGuard.resumeAfterAbortedAppQuit(); + } } ); } 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 b3f671c3c..6c11fbff2 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 @@ -227,6 +227,56 @@ describe('SettingsUnloadGuardService', () => { expect(electronStub.cancelWindowClose).toHaveBeenCalledTimes(1); }); + it('escalates an open reload confirmation into the requested close', async () => { + let resolveConfirm: ((value: boolean) => void) | null = null; + host.confirmClose.mockImplementation( + () => + new Promise((resolve) => { + resolveConfirm = resolve; + }) + ); + const reloadPage = jest.fn(); + activateInElectron(); + service.reloadPage = reloadPage; + form.markAsDirty(); + + // Reload confirmation opens first... + dispatchBeforeUnload(); + await flushAsyncWork(); + expect(host.confirmClose).toHaveBeenCalledTimes(1); + + // ...then the user closes the window while it is on screen. + closeRequestCallback?.(); + resolveConfirm?.(true); + await flushAsyncWork(); + + // Save/Discard completes the close — not the stale reload. + expect(electronStub.confirmWindowClose).toHaveBeenCalledTimes(1); + expect(reloadPage).not.toHaveBeenCalled(); + }); + + it('cancels the escalated close when the user stays', async () => { + let resolveConfirm: ((value: boolean) => void) | null = null; + host.confirmClose.mockImplementation( + () => + new Promise((resolve) => { + resolveConfirm = resolve; + }) + ); + activateInElectron(); + form.markAsDirty(); + + dispatchBeforeUnload(); + await flushAsyncWork(); + closeRequestCallback?.(); + resolveConfirm?.(false); + await flushAsyncWork(); + + // The main process remembered a close; staying must clear it. + expect(electronStub.cancelWindowClose).toHaveBeenCalledTimes(1); + expect(electronStub.confirmWindowClose).not.toHaveBeenCalled(); + }); + it('ignores repeated close requests while a dialog is open', async () => { let resolveConfirm: ((value: boolean) => void) | null = null; host.confirmClose.mockImplementation( 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 5b686065b..1ca67b89f 100644 --- a/apps/web/src/app/settings/settings-unload-guard.service.ts +++ b/apps/web/src/app/settings/settings-unload-guard.service.ts @@ -41,6 +41,13 @@ export class SettingsUnloadGuardService implements OnDestroy { private dirtySubscription: Subscription | null = null; private unsubscribeCloseRequests: (() => void) | null = null; private confirmationPending = false; + /** + * Intent of the confirmation currently on screen. A close request + * arriving while a reload confirmation is open escalates this to + * 'close' — the dialog is the same, only the continuation differs, and + * the user's most recent ask must be the one that completes. + */ + private activeIntent: 'close' | 'reload' | null = null; /** Last guard state mirrored to the main process. */ private guardArmed = false; /** True while an updater-driven app quit must pass unchallenged. */ @@ -144,15 +151,31 @@ export class SettingsUnloadGuardService implements OnDestroy { ): Promise { const host = this.host; - if (!host || this.confirmationPending) { + if (!host) { + return; + } + + if (this.confirmationPending) { + // A close outranks a reload, never the other way around: the + // open dialog stays, but Save/Discard must complete the close + // the user just asked for, not the earlier reload. + if (intent === 'close') { + this.activeIntent = 'close'; + } return; } this.confirmationPending = true; + this.activeIntent = intent; try { - if (!(await host.confirmClose())) { - if (intent === 'close') { + const proceed = await host.confirmClose(); + // Re-read after the dialog: a cross-intent request may have + // escalated it while the user was deciding. + const finalIntent = this.activeIntent ?? intent; + + if (!proceed) { + if (finalIntent === 'close') { // Staying must clear the intent the main process // remembered, or a later close attempt would replay a // stale quit. @@ -161,13 +184,14 @@ export class SettingsUnloadGuardService implements OnDestroy { return; } - if (intent === 'close') { + if (finalIntent === 'close') { await window.electron?.confirmWindowClose?.(); } else { this.reloadPage(); } } finally { this.confirmationPending = false; + this.activeIntent = null; } }