XS✔ ◾ fix: stop update-ready reminder from re-popping and stacking within a session (#972)

* fix: stop update-ready reminder from re-popping and stacking within a session (#456)

Clicking "Remind me later" on the update-ready dialog only guarded the
dialog's own "Later" state, not re-entrancy: if the periodic ~10-minute
update check fired another "update-downloaded" event while the first
dialog was still open, a second dialog would stack on top of it. Add an
isUpdateDialogOpen guard so only one reminder dialog can be shown at a
time, cleared once the user responds.

Closes #456

* address review: bound the update dialog + short-circuit post-restart events (#456)

- Add a 5-minute timeout fallback so a never-settling showMessageBox()
  can't leave isUpdateDialogOpen stuck true forever, silently
  suppressing all future update-ready reminders (minor finding).
- Add an isRestartingToInstall short-circuit so an update-downloaded
  event landing in the gap between the isUpdateDialogOpen reset and
  the deferred quitAndInstall() can't stack a second dialog moments
  before quit (minor finding).
- Test: await the guard's reset/microtask flush via vi.waitFor instead
  of firing-and-forgetting the resolved dialog promise (nit).
- Test: add coverage for the guard's release-and-reuse path via the
  new timeout fallback, and for the Restart Now short-circuit (nit).

* address review: honor the real dialog response instead of the race (#456)

The 5-minute Promise.race timeout previously reset isUpdateDialogOpen
and had .then/.catch/.finally attached to the race, not to the real
dialog.showMessageBox() promise. Once the timeout "won", a later
update-downloaded event could stack a second native dialog on top of
the still-open first one, and the user's eventual real click on the
original dialog fired no callback at all (Promise.race only resolves
the winning branch's continuation).

Fix: attach the response handler directly to dialogPromise so a late
click is always honored, and use a separate watchdog that only logs a
warning and frees the guard for future events after 5 minutes -  it
never touches dialogPromise's own chain. A per-invocation requestId
stops an abandoned dialog's very-late resolution from clobbering a
newer dialog's guard state.

Also documents (rather than "fixes") that isRestartingToInstall is
intentionally never reset - quitAndInstall() is a fire-and-forget
setImmediate() call with no failure signal, so permanently suppressing
reminders once the user has committed to restarting is the safer
failure mode. Removes the now-unused -1 timeout sentinel entirely
(the Promise.race/synthetic-result approach it belonged to is gone).

Adds a regression test pinning the blocking fix: dialog 1 times out,
guard releases, dialog 2 opens for a new event, then dialog 1's
original promise resolves late - asserting the late response is still
honored and does not clobber dialog 2's guard state.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

* address review: guard .then() with requestId so a stale dialog can't clobber a newer one (#456)

muster round 3 found that .finally() checked updateDialogRequestId before
releasing the guard, but .then() had no such check — a late click on an
abandoned, superseded dialog could still flip updateDialogDismissedInSession
or trigger quitAndInstall() out from under a different, currently-open
dialog. Added the same requestId check to .then(), clear the watchdog
timer once a dialog settles on its own, and rewrote the round-2 regression
test (which was inadvertently confounded by updateDialogDismissedInSession
and didn't actually exercise the buggy path) to correctly pin the new,
request-scoped behavior.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

* fix: keep update dialog guard until dialog closes

---------

Co-authored-by: armada-lookout <armada-lookout@users.noreply.github.com>
Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
Co-authored-by: Willow Lyu <iqay_lyu@hotmail.com>

Author: 4 people

src/backend/ipc/release-channel-handlers.test.ts

@@ -1,21 +1,37 @@
 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",
     getVersion: () => "1.2.3",
     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<string, Array<(...args: unknown[]) => 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));
+  });
+});

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;
         });
     });
   }