diff --git a/src/backend/ipc/release-channel-handlers.test.ts b/src/backend/ipc/release-channel-handlers.test.ts index 0465993c..7ff03ce7 100644 --- a/src/backend/ipc/release-channel-handlers.test.ts +++ b/src/backend/ipc/release-channel-handlers.test.ts @@ -1,8 +1,10 @@ import { afterEach, beforeEach, describe, expect, it, type Mock, vi } from "vitest"; // Keep Electron, the real autoUpdater, and encrypted storage out of this unit test — we're -// exercising the token-health gating logic in ReleaseChannelIPCHandlers (#919), not Electron -// itself. +// exercising both the token-health gating logic (#919) and the update-ready reminder dialog's +// anti-stacking guard behavior (#456) in ReleaseChannelIPCHandlers, driven via mocked +// Electron/autoUpdater listeners rather than the real Electron runtime. +const showMessageBoxMock = vi.fn().mockResolvedValue({ response: 1 }); vi.mock("electron", () => ({ app: { getName: () => "YakShaver", @@ -10,12 +12,26 @@ vi.mock("electron", () => ({ isPackaged: true, }, BrowserWindow: { getAllWindows: () => [] }, - dialog: { showMessageBox: vi.fn().mockResolvedValue({ response: 1 }) }, + dialog: { showMessageBox: (...args: unknown[]) => showMessageBoxMock(...args) }, ipcMain: { handle: vi.fn() }, })); const checkForUpdatesMock = vi.fn(); const setFeedURLMock = vi.fn(); +const quitAndInstallMock = vi.fn(); +// Capture registered autoUpdater listeners (keyed by event name) so tests can fire events like +// "update-downloaded" directly, the same way the real electron-updater instance would (#456). +const autoUpdaterListeners = new Map void>>(); +const autoUpdaterOnMock = vi.fn((event: string, listener: (...args: unknown[]) => void) => { + const listeners = autoUpdaterListeners.get(event) ?? []; + listeners.push(listener); + autoUpdaterListeners.set(event, listeners); +}); +function emitAutoUpdaterEvent(event: string, ...args: unknown[]) { + for (const listener of autoUpdaterListeners.get(event) ?? []) { + listener(...args); + } +} vi.mock("electron-updater", () => ({ autoUpdater: { autoDownload: false, @@ -24,9 +40,10 @@ vi.mock("electron-updater", () => ({ allowPrerelease: false, allowDowngrade: false, requestHeaders: {}, - on: vi.fn(), + on: (...args: [string, (...a: unknown[]) => void]) => autoUpdaterOnMock(...args), setFeedURL: (...args: unknown[]) => setFeedURLMock(...args), checkForUpdates: (...args: unknown[]) => checkForUpdatesMock(...args), + quitAndInstall: (...args: unknown[]) => quitAndInstallMock(...args), }, })); @@ -373,3 +390,118 @@ describe("ReleaseChannelIPCHandlers — PR releases require a healthy GitHub tok expect(result).toEqual({ available: false, currentVersion: "1.2.3" }); }); }); + +describe("ReleaseChannelIPCHandlers — update-ready reminder dialog (#456)", () => { + beforeEach(() => { + vi.clearAllMocks(); + autoUpdaterListeners.clear(); + showMessageBoxMock.mockReset(); + }); + + it("does not stack a second reminder dialog while the first is still awaiting a response", async () => { + // The reported bug's second half: "If the reminder dialog is open already, a subsequent + // update check should not open another reminder dialog on top of it." Simulate the first + // dialog's promise never resolving (user hasn't answered yet) and fire a second + // "update-downloaded" event (e.g. the periodic background check landing mid-dialog) — only one + // dialog must ever be shown. + let resolveFirstDialog: ((result: { response: number }) => void) | undefined; + showMessageBoxMock.mockImplementationOnce( + () => + new Promise((resolve) => { + resolveFirstDialog = resolve; + }), + ); + + new ReleaseChannelIPCHandlers(); + + emitAutoUpdaterEvent("update-downloaded"); + emitAutoUpdaterEvent("update-downloaded"); + + expect(showMessageBoxMock).toHaveBeenCalledTimes(1); + + // Resolve the first dialog and wait for the handler's .then/.finally chain to flush before + // the test ends, matching the sibling test's vi.waitFor pattern rather than firing-and-forgetting. + resolveFirstDialog?.({ response: 1 }); + await vi.waitFor(() => expect(showMessageBoxMock).toHaveBeenCalledTimes(1)); + }); + + it("keeps suppressing duplicate reminders while a long-running dialog remains open", async () => { + vi.useFakeTimers(); + try { + showMessageBoxMock.mockImplementation(() => new Promise(() => {})); // never settles + + new ReleaseChannelIPCHandlers(); + + emitAutoUpdaterEvent("update-downloaded"); + expect(showMessageBoxMock).toHaveBeenCalledTimes(1); + + // Advance beyond both the removed five-minute watchdog and the ten-minute update interval. + await vi.advanceTimersByTimeAsync(11 * 60 * 1000); + + // The original native dialog is still visible, so another event must remain suppressed. + emitAutoUpdaterEvent("update-downloaded"); + expect(showMessageBoxMock).toHaveBeenCalledTimes(1); + } finally { + vi.useRealTimers(); + } + }); + + it("releases the guard after a dialog error so a later event can retry", async () => { + const errorSpy = vi.spyOn(console, "error").mockImplementation(() => {}); + showMessageBoxMock + .mockRejectedValueOnce(new Error("dialog failed")) + .mockImplementationOnce(() => new Promise(() => {})); + + try { + new ReleaseChannelIPCHandlers(); + + emitAutoUpdaterEvent("update-downloaded"); + await vi.waitFor(() => expect(errorSpy).toHaveBeenCalledTimes(1)); + + emitAutoUpdaterEvent("update-downloaded"); + expect(showMessageBoxMock).toHaveBeenCalledTimes(2); + } finally { + errorSpy.mockRestore(); + } + }); + + it("does not show a reminder again this session after the user clicks Later — the originally reported bug", async () => { + // The reported bug's first half: clicking "Remind me later" must stop further reminders for + // the rest of the current session, even when the periodic ~10-minute check fires again. + showMessageBoxMock.mockResolvedValue({ response: 1 }); // 1 = "Later" + + new ReleaseChannelIPCHandlers(); + + emitAutoUpdaterEvent("update-downloaded"); + await vi.waitFor(() => expect(showMessageBoxMock).toHaveBeenCalledTimes(1)); + + // Simulate the periodic check finding the same update again later in the session. + emitAutoUpdaterEvent("update-downloaded"); + emitAutoUpdaterEvent("update-downloaded"); + + expect(showMessageBoxMock).toHaveBeenCalledTimes(1); + }); + + it("does not show another reminder after the user clicks Restart Now, even before quitAndInstall fires", async () => { + // Regression coverage (review on #456): isUpdateDialogOpen resets in .finally() synchronously, + // but the real quit (autoUpdater.quitAndInstall) is deferred via setImmediate. A second + // "update-downloaded" event landing in that gap must not stack a dialog moments before the app + // quits — pinned here via the isRestartingToInstall short-circuit. + showMessageBoxMock.mockResolvedValue({ response: 0 }); // 0 = "Restart Now" + + new ReleaseChannelIPCHandlers(); + + emitAutoUpdaterEvent("update-downloaded"); + await vi.waitFor(() => expect(showMessageBoxMock).toHaveBeenCalledTimes(1)); + + // A second event landing after the dialog resolved (isUpdateDialogOpen already released) but + // before the deferred quitAndInstall() fires must still be suppressed. + emitAutoUpdaterEvent("update-downloaded"); + + expect(showMessageBoxMock).toHaveBeenCalledTimes(1); + + // Let the deferred quitAndInstall() flush before the test ends, so it doesn't fire during a + // later test. + await vi.waitFor(() => expect(quitAndInstallMock).toHaveBeenCalledTimes(1)); + }); +}); diff --git a/src/backend/ipc/release-channel-handlers.ts b/src/backend/ipc/release-channel-handlers.ts index 6ce4ad70..e750c9ec 100644 --- a/src/backend/ipc/release-channel-handlers.ts +++ b/src/backend/ipc/release-channel-handlers.ts @@ -75,6 +75,23 @@ export class ReleaseChannelIPCHandlers { private updateCheckInterval: NodeJS.Timeout | null = null; private readonly UPDATE_CHECK_INTERVAL = 10 * 60 * 1000; // Check every 10 minutes private updateDialogDismissedInSession = false; // Track if user dismissed update dialog this session + // Track whether the "Update Ready" dialog is currently open (#456) - guards against a second + // autoUpdater "update-downloaded" event (e.g. a periodic re-check landing while the first + // dialog is still awaiting a response) from stacking a duplicate dialog on top of it. + private isUpdateDialogOpen = false; + // Once the user picks "Restart Now" the app is on its way out (review on #456) - short-circuit + // any further "update-downloaded" events immediately rather than relying solely on the + // isUpdateDialogOpen guard, which resets in .finally() before the deferred quitAndInstall() call + // actually fires and would otherwise leave a narrow window where a second event could stack a + // dialog moments before quit. + // + // Never reset back to false (review on #456): quitAndInstall() is fired via a fire-and-forget + // setImmediate() with no completion signal, so there is no reliable point at which we could learn + // the quit was blocked/failed and it's safe to resume showing reminders. Once the user has + // committed to restarting, permanently suppressing further reminders for the rest of this + // (soon-to-exit) session is the safer failure mode versus risking a reminder popping up while the + // app believes it is already on its way out. + private isRestartingToInstall = false; constructor() { ipcMain.handle(IPC_CHANNELS.RELEASE_CHANNEL_GET, () => this.getChannel()); @@ -111,11 +128,27 @@ export class ReleaseChannelIPCHandlers { }); autoUpdater.on("update-downloaded", () => { - // Skip showing dialog if user already dismissed it this session + // Skip showing dialog if user already dismissed it this session (#456) - once "Later" is + // clicked, no further reminder should appear for the rest of the current session. if (this.updateDialogDismissedInSession) { return; } + // Skip if the app is already on its way out from a prior "Restart Now" (review on #456) - + // checked before isUpdateDialogOpen since that guard resets in .finally() slightly before + // the deferred quitAndInstall() call actually fires. + if (this.isRestartingToInstall) { + return; + } + + // Skip if a reminder dialog is already open (#456) - a subsequent "update-downloaded" event + // (e.g. the periodic background check firing again) must not stack another dialog on top of + // one the user hasn't responded to yet. + if (this.isUpdateDialogOpen) { + return; + } + this.isUpdateDialogOpen = true; + dialog .showMessageBox({ type: "info", @@ -127,6 +160,9 @@ export class ReleaseChannelIPCHandlers { }) .then((result) => { if (result.response === 0) { + // The app is quitting to install - short-circuit any further events immediately, + // ahead of the isUpdateDialogOpen reset in .finally() below. + this.isRestartingToInstall = true; // Set isQuitting to true so that the before-quit handler allows the app to quit setIsQuitting(true); // Force immediate quit and install @@ -135,12 +171,19 @@ export class ReleaseChannelIPCHandlers { autoUpdater.quitAndInstall(false, true); }); } else { - // User chose "Later" - remember this for the current session + // User chose "Later" - remember this for the current session so no further reminder + // is shown, whether from another "update-downloaded" event or the periodic check. this.updateDialogDismissedInSession = true; } }) .catch((err) => { console.error("Error showing update dialog:", err); + }) + .finally(() => { + // Keep the guard for the full lifetime of the native dialog. This dialog has no reliable + // cross-platform cancellation configured, so releasing the guard on a timeout would + // allow another dialog to stack on top while the original is still visible. + this.isUpdateDialogOpen = false; }); }); }