XS✔ ◾ fix: stop update-ready reminder from re-popping and stacking within a session#972
Conversation
… 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
PR Metrics✔ Thanks for keeping your pull request small.
Metrics computed by PR Metrics. Add it to your Azure DevOps and GitHub PRs! |
github-actions
Bot
changed the title
There was a problem hiding this comment.
Pull request overview
This PR prevents the “Update Ready” reminder dialog from re-opening (and stacking) within the same app session when multiple autoUpdater update-downloaded events occur.
Changes:
- Added an
isUpdateDialogOpenre-entrancy guard around theupdate-downloadeddialog flow to ensure only one reminder dialog can be shown at a time. - Added regression tests that (1) verify no dialog stacking while a dialog is pending and (2) verify “Later” continues to suppress further reminders for the rest of the session.
- Enhanced unit-test mocks to capture and drive
autoUpdater.on(...)listeners and to controldialog.showMessageBox(...)resolution.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/backend/ipc/release-channel-handlers.ts | Adds an isUpdateDialogOpen guard and clears it in .finally() to prevent dialog stacking during repeated update-downloaded events. |
| src/backend/ipc/release-channel-handlers.test.ts | Adds regression coverage for dialog re-entrancy + session-level suppression; extends mocks to emit autoUpdater events and control showMessageBox behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // 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. |
|
🚀 Pre-release build is available for this PR: |
tomek-i
added
the
armada:reviewing
|
🔭 crows-nest: ready-PR pipeline started — review → address → re-validate → gated merge. |
tomek-i
left a comment
tomek-i
left a comment
There was a problem hiding this comment.
🔭 muster review — PR #972
Two independent lenses reviewed this PR: code-review (conventions/correctness) and a codex-rescue substitute (this environment has no codex plugin installed, so an independent general-purpose root-cause pass stood in for the canonical codex-rescue lens — noted for transparency, not treated as a degrade since both lenses ran and returned findings).
Summary: 0 blocking · 0 major · 2 minor · 2 nit
Both lenses independently converged on the same two logical gaps around the new isUpdateDialogOpen guard (see inline comments) — that convergence is a reasonably strong signal they're real, if low-severity for now. Nothing here blocks merge; flagging as minor/nit for awareness.
| } | ||
| this.isUpdateDialogOpen = true; | ||
|
|
||
| dialog |
There was a problem hiding this comment.
[minor] No timeout/safety-net if showMessageBox never settles (flagged by both lenses)
isUpdateDialogOpen is set true before calling dialog.showMessageBox(...) and only reset in .finally(). If that promise never settles (a documented Electron edge case — e.g. the parent window is destroyed while the dialog is up, or a native dialog-subsystem hang) or throws synchronously outside the promise chain, the guard stays true forever and silently suppresses every future update-ready reminder for the rest of the session — arguably worse than the stacking bug this PR fixes. Neither new test exercises a hung/never-resolving dialog. Consider a bounded timeout/watchdog that resets the guard as a fallback.
There was a problem hiding this comment.
Agree — implemented in ab0e2eb. Added a 5-minute bounded timeout that races against dialog.showMessageBox(); if it never settles, the guard is released (isUpdateDialogOpen = false) via the timeout branch without touching updateDialogDismissedInSession, so a future update-downloaded event can still offer the reminder. Added a regression test that pins the release-and-reuse behavior via this exact timeout path.
| .catch((err) => { | ||
| console.error("Error showing update dialog:", err); | ||
| }) | ||
| .finally(() => { |
There was a problem hiding this comment.
[minor] Guard resets before the actual quit fires on "Restart Now" (flagged by both lenses)
On the "Restart Now" branch, isUpdateDialogOpen resets to false synchronously in .finally(), but the real quit (autoUpdater.quitAndInstall) is deferred via setImmediate. In that brief window, updateDialogDismissedInSession is never set on this branch, so a second update-downloaded event landing in that gap could in principle stack a second dialog moments before the app quits — the same class of bug this PR closes, just narrowed rather than eliminated on this one path. Practically negligible (sub-millisecond window, needs a second full download to land in it) but worth a one-line note or a isQuitting short-circuit on the guard check.
There was a problem hiding this comment.
Agree — implemented in ab0e2eb. Added an isRestartingToInstall flag set synchronously in the "Restart Now" branch (before the deferred quitAndInstall() fires) and checked ahead of the isUpdateDialogOpen guard, so an event landing in that narrow window is short-circuited rather than stacking a dialog moments before quit. Added a regression test covering this exact gap.
| expect(showMessageBoxMock).toHaveBeenCalledTimes(1); | ||
|
|
||
| // Resolve the first dialog so the test doesn't leave a dangling promise. | ||
| resolveFirstDialog?.({ response: 1 }); |
There was a problem hiding this comment.
[nit] resolveFirstDialog?.({ response: 1 }) is called without awaiting the resulting .then/.finally chain on the handler side, so the guard's reset and any pending microtasks aren't guaranteed to flush before the test ends. Low risk (the next beforeEach clears mocks), but the second new test's use of vi.waitFor(...) is the more robust pattern — worth matching here too.
There was a problem hiding this comment.
Agree — implemented in ab0e2eb. Replaced the fire-and-forget resolve with await vi.waitFor(...) after resolving the dialog, matching the sibling test's pattern so the guard's reset and any pending microtasks are guaranteed to flush before the test ends.
| // Simulate the periodic check finding the same update again later in the session. | ||
| emitAutoUpdaterEvent("update-downloaded"); | ||
| emitAutoUpdaterEvent("update-downloaded"); | ||
|
|
There was a problem hiding this comment.
[nit] No test directly exercises the guard's release-and-reuse path (dialog resolves → guard clears → a later legitimate update-downloaded event is handled normally). Both new tests either overlap the guard while it's still open, or rely on updateDialogDismissedInSession short-circuiting first. Not a hard requirement given the call sites, but would directly pin down the .finally() reset behavior.
There was a problem hiding this comment.
Agree — implemented in ab0e2eb. Added "releases the guard after the dialog settles so a later legitimate event is handled normally", which directly pins the .finally() reset: it uses the new timeout fallback (a dialog that never settles) so the guard's own release is what's under test, isolated from updateDialogDismissedInSession/isRestartingToInstall short-circuits. Also added a second test for the Restart Now short-circuit path while I was in there.
…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).
Addressed muster reviewTriaged all 4 findings from the muster review — agreed with and implemented all 4 (2 minor, 2 nit). New head:
Re-validated: |
|
🚀 Pre-release build is available for this PR: |
tomek-i
removed
the
armada:reviewing
|
✅ crows-nest: reviewed, addressed (4 non-blocking findings triaged and fixed), re-validated (build/test/lint green, CI green) — awaiting human merge (auto-merge off). |
tomek-i
added
armada:reviewing
|
🔭 crows-nest: picked up by ARMADA — dispatching to muster review. |
| cancelId: 1, | ||
| }); | ||
|
|
||
| Promise.race([dialogPromise, timeout]) |
There was a problem hiding this comment.
[blocking] Timeout path reopens the dialog-stacking bug and silently drops the user's real click
Both review lenses (code-review + codex-rescue) independently converged on this as the same root-cause issue, from two angles:
-
Re-stacking after 5 minutes. When
setTimeoutwins thePromise.race(dialog unanswered for 5 minutes), the code logs a warning and lets.finally()resetisUpdateDialogOpen = false— but it never closes or otherwise deals with the actualdialog.showMessageBox()call, which is a native OS-modal that cannot be cancelled and is still on screen awaiting the user's click. Once the guard flips back tofalse, a thirdupdate-downloadedevent (very plausible: the periodic check re-fires every ~10 minutes, so a user stepping away for 15+ minutes will trigger this) callsdialog.showMessageBox()again, stacking a second native dialog on top of the still-open first one. This reproduces issue Remind me later: update reminder keeps popping up within same session #456's reported bug, merely delayed by 5 minutes instead of occurring on the immediate next event. -
Original dialog's real answer is silently discarded. The
.then/.catch/.finallychain is attached toPromise.race([dialogPromise, timeout]), not todialogPromiseitself. PerPromise.racesemantics, once the timeout promise wins and the race settles, the later resolution of the losingdialogPromisefires no callback at all. So when the user eventually clicks "Restart Now" or "Later" on the original (now stale) dialog after the timeout already fired, that click has zero effect:isRestartingToInstallnever gets set,quitAndInstall()is never called,updateDialogDismissedInSessionnever gets set — the user's explicit choice is silently dropped with no error or feedback, and (per point 1) they may now be looking at a second dialog that also doesn't behave as expected.
Suggested fix: either attach a handler directly to dialogPromise (not just the race) so a late user response is still honored even after the timeout warning fires, or track a generation/token per dialog invocation so a late resolution can be routed correctly instead of silently swallowed. The 5-minute timeout's own stated goal (don't leave isUpdateDialogOpen stuck forever) is a much rarer failure mode than the routine "user steps away for >5 minutes" case it reintroduces.
flagged by: code-review + codex-rescue (independent agreement)
There was a problem hiding this comment.
Agree — this was a real bug, thanks for catching it. Fixed in 1b3ff9f: the response handler is now attached directly to dialogPromise (not to Promise.race([dialogPromise, timeout])), so a late click on the original, still-open native dialog is always honored, however late it arrives. The 5-minute watchdog is now separate — it only logs a warning and frees isUpdateDialogOpen for future events; it never touches dialogPromise's own .then/.catch/.finally chain. Added a per-invocation requestId so a very-late resolution from an abandoned (timed-out) dialog can't clobber a newer dialog's guard state if a third event has since opened dialog 2. Added a regression test ("still honors the original dialog's real answer after the watchdog timeout releases the guard, without disturbing a newer dialog") that pins exactly this scenario.
| expect(showMessageBoxMock).toHaveBeenCalledTimes(2); | ||
| } finally { | ||
| vi.useRealTimers(); | ||
| } |
There was a problem hiding this comment.
[minor] No test covers a third event after timeout, nor the abandoned first dialog's late resolution
The "releases the guard after the dialog settles" test only verifies that after the 5-minute timeout, a second update-downloaded event causes showMessageBoxMock to be called again (total count 2). It never asserts anything about the ORIGINAL (never-settling) dialogPromise from the first call — e.g. resolving it after the timeout and checking whether that causes any unwanted side effects (ignored user response, or interaction with a second dialog now in flight). This is exactly the gap that let the blocking issue (see the inline comment on release-channel-handlers.ts, the Promise.race block) through untested.
Suggested addition: a test that (1) triggers dialog 1, (2) lets it hang past the 5-minute timeout so the guard resets, (3) triggers dialog 2 via a third event, (4) then resolves dialog 1's original promise with e.g. { response: 0 }, and asserts what actually happens — currently neither the "correct" nor the buggy behavior is pinned by any test.
flagged by: code-review + codex-rescue (independent agreement)
There was a problem hiding this comment.
Agree — added in 1b3ff9f. New test "still honors the original dialog's real answer after the watchdog timeout releases the guard, without disturbing a newer dialog (#456 blocking fix)": dialog 1 hangs past its watchdog (guard releases) -> a third event opens dialog 2 (still open, unanswered) -> dialog 1's original promise finally resolves ("Later", to keep this isolated from the separate isRestartingToInstall short-circuit). Asserts the late response is honored and that a fourth event is still correctly suppressed as a stack-on-top of dialog 2 — i.e. dialog 1's late .finally() did not incorrectly clobber dialog 2's guard state.
| // 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. | ||
| private isRestartingToInstall = false; |
There was a problem hiding this comment.
[minor] isRestartingToInstall is never reset, so a failed/blocked quit permanently suppresses all future reminders
isRestartingToInstall is set to true when the user picks "Restart Now" and is checked at the very top of the handler (before isUpdateDialogOpen), but nothing ever sets it back to false. This is fine on the expected path (app quits via quitAndInstall), but quitAndInstall is called inside a setImmediate and can in principle be deferred/blocked by other before-quit handlers, an OS-level prompt, or fail outright (installer error, permissions issue) without the process actually exiting. In that edge case the app keeps running but every future update-downloaded event is now permanently ignored for the rest of the session — with no code path that ever clears the flag. Same failure class ("flag never resets if the deferred follow-up action doesn't happen") as the main blocking issue in this PR.
flagged by: codex-rescue
There was a problem hiding this comment.
Discuss — agree this is a real gap, but implemented as documentation rather than a reset. There's no reliable signal to reset on: quitAndInstall() is fired via a fire-and-forget setImmediate() with no completion/failure callback, so we can't distinguish "quit is slow" from "quit is permanently blocked" without adding a second speculative watchdog/timeout guessing at how long a legitimate quit may take. Since the user has already explicitly committed to restarting, permanently suppressing further reminders for the rest of that (expected-to-exit-soon) session reads as the safer failure mode versus risking a reminder popping up while the app believes it's already quitting. Expanded the inline comment in 1b3ff9f to state this is intentional rather than an oversight. Happy to add a bounded reset watchdog (mirroring the dialog's own) if you'd rather have that — flagging as a possible follow-up rather than blocking this PR on it.
| // and silently suppress every future reminder for the rest of the session. | ||
| const timeout = new Promise<{ response: number; timedOut: true }>((resolve) => { | ||
| setTimeout( | ||
| () => resolve({ response: -1, timedOut: true }), |
There was a problem hiding this comment.
[nit] Timeout sentinel response value (-1) is unused/undocumented
The timeout promise resolves with { response: -1, timedOut: true }. Because the code branches on 'timedOut' in result before ever reading .response, the -1 value is never actually consumed anywhere — harmless, but it may read as a meaningful cancelId-like sentinel to a future reader. A brief comment noting the response field is a placeholder/unused in the timeout branch would avoid confusion.
flagged by: code-review
There was a problem hiding this comment.
Agree — moot now rather than just documented: the Promise.race/synthetic { response: -1, timedOut: true } result object it belonged to is gone entirely in 1b3ff9f (see the blocking-finding fix above). The watchdog is now a plain setTimeout that only logs a warning and flips the boolean guard — there's no sentinel response value left to be unused/undocumented.
muster review — PR #972Two independent lenses reviewed this diff in parallel: a conventions/correctness code-review pass and a codex-rescue root-cause second opinion. Both were run cold (no shared context) and their findings were consolidated and deduped below. Bottom line: 1 blocking, 2 minor, 1 nit — not ready to merge. Blocking (1)
Minor (2)
Nit (1)
Lens agreementThe two lenses agreed strongly on the core defect (independently, without seeing each other's output) — that's the strongest signal in this review. No genuine disagreement/dissent between lenses to report. Posted by ARMADA muster (dual-lens review: code-review + codex-rescue). This review does not approve, request changes, or merge — see inline comments for details. Addressing findings is a job for shipwright's address-review mode. |
) 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 pass (round 2) — new head
|
tomek-i
added
armada
|
🔭 crows-nest: muster found 1 blocking + 3 minor findings — shipwright addressed all of them (commit 1b3ff9f): fixed the Promise.race dialog-stacking/dropped-click bug by attaching response handling to the real |
|
🚀 Pre-release build is available for this PR: |
tomek-i
added
the
armada:reviewing
|
🔭 crows-nest: ready-PR pipeline started — review → address → re-validate → gated merge. |
| // winning branch's continuation ever runs). Attaching here instead means a late, real | ||
| // response is always honored, however late it arrives. | ||
| dialogPromise | ||
| .then((result) => { |
There was a problem hiding this comment.
[blocking] .then() result handler is missing the requestId guard, so a stale/abandoned dialog's late click can hijack a newer, currently-open dialog
The new updateDialogRequestId mechanism is meant to distinguish "my own dialog settled" from "a newer dialog has already superseded me", but it is only checked in .finally() (line 208: if (this.updateDialogRequestId === requestId) { this.isUpdateDialogOpen = false; }). The .then((result) => {...}) callback that actually acts on the user's choice — setting updateDialogDismissedInSession = true on "Later", or setting isRestartingToInstall = true and firing quitAndInstall() on "Restart Now" — has no such guard and runs unconditionally whenever the promise resolves, however late.
Concrete failure sequence:
- Dialog ✨ Native Linux Support - Enable YakShaver Desktop App to run natively on Linux #1 opens (
requestId=1). - The 5-minute watchdog fires because dialog ✨ Native Linux Support - Enable YakShaver Desktop App to run natively on Linux #1 is still on screen and unanswered, releases the guard, and warns.
- A subsequent
update-downloadedevent opens dialog ✨ Azure Trusted Signing - Implement Azure Trusted Signing for Windows Apps #2 (requestId=2), which the user is now actively looking at and hasn't answered. - The user finally clicks the stale, abandoned dialog ✨ Native Linux Support - Enable YakShaver Desktop App to run natively on Linux #1 (per the PR's own comment: "The original dialog is still on screen"):
- Clicking "Later" on dialog ✨ Native Linux Support - Enable YakShaver Desktop App to run natively on Linux #1 sets
updateDialogDismissedInSession = trueglobally, silently suppressing dialog ✨ Azure Trusted Signing - Implement Azure Trusted Signing for Windows Apps #2 (currently open, unanswered) and every future reminder for the rest of the session — even though the user never responded to dialog ✨ Azure Trusted Signing - Implement Azure Trusted Signing for Windows Apps #2. - Clicking "Restart Now" on dialog ✨ Native Linux Support - Enable YakShaver Desktop App to run natively on Linux #1 unconditionally calls
setIsQuitting(true)and schedulesautoUpdater.quitAndInstall(), force-quitting the app out from under the user while dialog ✨ Azure Trusted Signing - Implement Azure Trusted Signing for Windows Apps #2 is still open awaiting a real answer.
- Clicking "Later" on dialog ✨ Native Linux Support - Enable YakShaver Desktop App to run natively on Linux #1 sets
This undermines the PR's own stated design intent for handling a late response from an abandoned dialog — the .then() branch needed the same if (this.updateDialogRequestId === requestId) guard that .finally() received. Suggested fix: wrap the body of .then in the same requestId check used in .finally.
Related: the third new test ("still honors the original dialog's real answer after the watchdog timeout releases the guard, without disturbing a newer dialog (#456 blocking fix)") does not actually catch this — see the separate major finding on that test.
flagged by: codex-rescue
There was a problem hiding this comment.
Agree — this was a real bug, thanks for catching it (and for verifying it directly against the actual head commit rather than trusting my prior review-round comments). Fixed in 49c3204: the .then() handler now checks this.updateDialogRequestId === requestId before acting on the result — the same guard .finally() already had — so a stale/abandoned dialog's late click is now logged and ignored rather than silently dismissing the session or triggering quitAndInstall() out from under a newer, currently-open dialog.
| } | ||
| }); | ||
|
|
||
| it("still honors the original dialog's real answer after the watchdog timeout releases the guard, without disturbing a newer dialog (#456 blocking fix)", async () => { |
There was a problem hiding this comment.
[major] Third #456 regression test doesn't actually pin the requestId-guard behavior it claims to (both lenses independently flagged this)
In "still honors the original dialog's real answer after the watchdog timeout releases the guard, without disturbing a newer dialog (#456 blocking fix)", dialog 1 is deliberately resolved with response 1 ("Later") instead of 0 ("Restart Now") — per the test's own comment — specifically to avoid the isRestartingToInstall short-circuit confounding the final assertion.
However, resolving dialog 1 with "Later" sets this.updateDialogDismissedInSession = true (the else branch of .then). Since updateDialogDismissedInSession is checked before isRestartingToInstall/isUpdateDialogOpen in the update-downloaded handler, the 4th emitAutoUpdaterEvent in this test is actually suppressed by the dismissed-in-session flag — not by the isUpdateDialogOpen/requestId guard the test's comments say it is proving.
Concretely: this test would still pass even with the missing requestId guard on .then() (see the separate blocking finding on line 185) — the dismissed-in-session check short-circuits before that buggy code path is ever exercised by this test's final assertion. The test doesn't actually validate the fix it documents.
Suggested fix: restructure the scenario so dialog 1's late response doesn't also set updateDialogDismissedInSession before the assertion under test — e.g., assert on showMessageBoxMock call count for the newer dialog specifically before dialog 1 resolves, or add a dedicated test where dialog 1's late click is "Restart Now" is not usable either (confounds via isRestartingToInstall) — likely needs to inspect/exercise internal guard state more directly, or reorder so the guard-preservation assertion happens strictly before dialog 1 is ever resolved.
flagged by: code-review
There was a problem hiding this comment.
Agree — confirmed by reverting just the code fix and re-running: this test failed on the old (buggy) .then() before the fix, which is what it should have done all along. Rewrote it in 49c3204 to assert directly on updateDialogDismissedInSession staying false after the stale dialog's late click (rather than only inferring correctness from the newer dialog's suppression, which isUpdateDialogOpen alone could also explain) — this is now a genuine fail-before/pass-after regression test for the requestId guard.
| // its own dialog. The original dialogPromise keeps listening in the background; if the user | ||
| // eventually does click it, the .then handler above still runs and is still honored - it just | ||
| // won't re-release an already-released (or newer) guard, per the requestId check. | ||
| setTimeout(() => { |
There was a problem hiding this comment.
[minor] Watchdog setTimeout is never cleared on normal/fast dialog resolution, leaking a live timer for up to 5 minutes per dialog shown
The 5-minute watchdog setTimeout started for each dialog invocation is never cancelled via clearTimeout when the dialog resolves normally (the common case — most users respond within seconds), in .then()/.catch()/.finally(). It self-guards via the requestId check when it eventually fires, so this isn't a correctness bug, but every 'Update Ready' dialog shown leaves a live Node timer registered for up to 5 minutes after the user already responded — an avoidable dangling timer, especially relevant for the 'Later' path where the app keeps running.
Suggested fix: capture the timer id from setTimeout and call clearTimeout on it inside .finally() (guarded by the same requestId check).
flagged by: code-review + codex-rescue (independently)
There was a problem hiding this comment.
Agree — fixed in 49c3204. Captured the setTimeout handle into watchdogTimer and added clearTimeout(watchdogTimer) inside .finally() (guarded implicitly since .finally() only runs once the dialog itself has settled), so the watchdog no longer stays live for up to 5 minutes after a dialog the user already responded to.
| // 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; |
There was a problem hiding this comment.
[minor] isRestartingToInstall never resets and there's no teardown for the new guard state / watchdog timer, unlike the existing stopPeriodicUpdateChecks() pattern
By design (per the PR's own comment) isRestartingToInstall is set true on 'Restart Now' and never reset, because quitAndInstall() is fire-and-forget with no completion signal. That's a defensible failure mode for the common case, but if quitAndInstall() ever fails to actually terminate the process (blocked by another before-quit handler, an install/code-signing failure on some platform, or an exception thrown inside the deferred setImmediate callback), the app keeps running indefinitely with all future update-ready reminders permanently and silently suppressed for the rest of the session, with no user-visible indication why.
Separately: the class already has a stopPeriodicUpdateChecks() cleanup method for updateCheckInterval, but this PR adds new persistent state (isUpdateDialogOpen, isRestartingToInstall, updateDialogRequestId) and a watchdog setTimeout with no equivalent teardown hook. Low real-world risk today since ReleaseChannelIPCHandlers is a singleton and stopPeriodicUpdateChecks() itself isn't currently called from anywhere — flagging only as a maintainability gap for if/when a dispose path is wired up.
Non-blocking; consider logging when isRestartingToInstall is set (for diagnosability) as a low-cost improvement.
flagged by: codex-rescue + code-review (independently, related observations)
There was a problem hiding this comment.
Discuss — this is the same point raised and discussed in review round 2 (see the earlier thread on this line), where the team's call was to keep it as a documented, intentional fail-safe rather than add a speculative secondary watchdog, since there's no reliable 'quit failed' signal to reset on. Standing by that decision rather than re-opening it here — happy to add a bounded reset watchdog as a tracked follow-up if a human reviewer wants it, but not implementing it speculatively in this pass. The teardown-hook gap is real but low-risk today (the class is a singleton and stopPeriodicUpdateChecks() itself isn't currently called from anywhere) — noting it as a follow-up rather than expanding this PR's scope.
muster review — PR #972Two independent lenses reviewed this diff in parallel: code-review (conventions/correctness) and codex-rescue (root-cause second opinion). Both ran successfully — this is a full, non-degraded review. Bottom line1 blocking, 1 major, 2 minor — not ready to merge as-is. Findings (worst-first)
Lens agreementThe two lenses independently converged on the timer-leak (minor) and teardown-gap (minor) observations — independent agreement, strong signal. The blocking finding (missing Why this is blocking, not just advisoryThe PR's whole stated purpose is closing a re-entrancy/stacking gap between two dialogs. The Posted by ARMADA muster (crows-nest ready-PR pipeline). Findings are advisory for major/minor/nit; the blocking finding must be resolved before merge. |
…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>
Address-review pass (round 3) — new head
|
muster review — PR #972 (round 2, post-address)Two independent lenses re-reviewed the current head ( Bottom line0 blocking, 0 major, 0 minor, 0 nit — clean. Both lenses independently verified the round-1 blocking fix (the No new findings on a fresh pass over the full diff. This is round 2 of this pipeline invocation (maxReviewRounds cap) — clean, no further address-review pass needed. |
|
🚀 Pre-release build is available for this PR: |
tomek-i
removed
the
armada:reviewing
|
🔭 crows-nest: ✅ reviewed, addressed, green — awaiting human merge (auto-merge off). Round 1 found 1 blocking + 1 major + 2 minor findings, all fixed by shipwright; round 2 re-review came back clean; build/test/lint and CI all green. |
tomek-i
added
the
armada:reviewing
|
🔭 crows-nest: ready-PR pipeline started — review → address → re-validate → gated merge (auto-merge is off, so this will stop at ready-for-human if green). |
| // 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; |
There was a problem hiding this comment.
[minor] isRestartingToInstall is permanently sticky with no recovery path if quitAndInstall silently fails
If autoUpdater.quitAndInstall(false, true) fails silently or the quit is blocked for some unrelated reason, the app is left running indefinitely with all future update-ready reminders suppressed for the rest of the session, with no path back to healthy state short of an app restart. This is called out and accepted in the code's own comment as 'the safer failure mode,' which is a reasonable call for this Electron auto-updater edge case — not blocking. Worth noting there's no user-facing signal (log-only via console.warn) if this state is ever reached, so a support engineer debugging 'update reminders stopped working' would need to check the main-process console log rather than anything surfaced in the UI.
flagged by: codex-rescue
| // the promise never settles (e.g. the parent window is destroyed while the dialog is up, or a | ||
| // native dialog-subsystem hang), isUpdateDialogOpen must not get stuck true forever, which would | ||
| // silently suppress every future update-ready reminder for the rest of the session. | ||
| private static readonly UPDATE_DIALOG_TIMEOUT_MS = 5 * 60 * 1000; // 5 minutes |
There was a problem hiding this comment.
[nit] Watchdog's stated 'parent window destroyed' rationale doesn't quite apply — dialog.showMessageBox is called without a parent window
The comment justifying the 5-minute watchdog cites 'the parent window is destroyed while the dialog is up' as a concrete failure mode. However, dialog.showMessageBox() is called here with no BrowserWindow argument, so it's a standalone, un-parented modal — window destruction isn't actually a mechanism that could hang this particular call the way the comment implies. The watchdog itself is still good defensive practice (native dialog subsystem hangs, OS-level modal issues), so no code change needed — just consider tightening the comment to avoid citing a scenario that doesn't apply to this exact call site.
flagged by: codex-rescue
muster review — PR #972Two independent lenses reviewed this diff in parallel: code-review (conventions + correctness) and codex-rescue (root-cause / second-opinion, reviewing cold). Counts: 0 blocking, 0 major, 1 minor, 1 nit Lens agreement: The code-review lens returned no findings after tracing the Findings:
Bottom line: no blocking findings — review advisory only. |
tomek-i
removed
the
armada:reviewing
|
✅ reviewed, addressed, green — awaiting human merge (auto-merge off). muster ran both lenses (code-review + codex-rescue): zero blocking findings, only 1 minor + 1 nit posted advisory. |
|
🚀 Pre-release build is available for this PR: |
|
✅ Automated Release Created Successfully Release Details:
You can monitor the build progress in the Actions tab. |
Summary
After clicking "Remind me later" on the update-ready dialog, the periodic ~10-minute background update check could fire another
update-downloadedevent while the first dialog was still open, stacking a second reminder dialog on top of it. This PR closes that re-entrancy gap so at most one reminder dialog is ever shown at a time, and the existing per-session "Later" dismissal continues to suppress all further reminders for the rest of the session.Closes #456
What changed
src/backend/ipc/release-channel-handlers.tsisUpdateDialogOpenguard around theupdate-downloadedhandler so a subsequent event can't open a second "Update Ready" dialog while one is already awaiting a response. Cleared via.finally()once the dialog settles.src/backend/ipc/release-channel-handlers.test.tsupdate-downloadedevent while the first dialog is still pending must not open another dialog; (2) after "Later" is chosen, furtherupdate-downloadedevents this session must not show a dialog again. Also extended theelectron/electron-updatertest mocks to capture registeredautoUpdater.on(...)listeners and a controllabledialog.showMessageBoxmock so these events can be driven directly in tests.Decisions
isUpdateDialogOpen) rather than gating the periodic timer'scheckForUpdates()call itself.checkForUpdates()is shared with the user-initiated "Check for Updates" button and the PR-channel flow that can find a genuinely newer version; blocking the periodic call entirely once dismissed would also suppress checks for an unrelated, newer update later in the session. The issue only asks that the reminder not repeat/stack — not that background checking stop — so the fix targets exactly the dialog-presentation layer.setIntervalcallback onupdateDialogDismissedInSession— rejected because it changes checking behavior beyond what was reported and could mask a legitimately newer release.Note: the primary "dialog re-appears after clicking Later" symptom described in the issue was already fixed by PR #762 (
updateDialogDismissedInSession), which landed after this issue was filed. What remained unaddressed was the issue's second, explicit ask: "If the reminder dialog is open already, a subsequent update check should not open another reminder dialog on top of it" — that's the gap this PR closes.Acceptance criteria
updateDialogDismissedInSessionflag (from XS⚠️ ◾ Added tracking for update ready notification #762) plus a new regression test confirming it still holds across repeatedupdate-downloadedevents.isUpdateDialogOpenguard, with a regression test that fails without the fix.Testing
autoUpdaterevent/listener wiring)npm run build)npm rebuild better-sqlite3 --build-from-source && npx vitest run --exclude 'src/ui/**'— 693/693;npm --prefix src/ui test— 255/255)npm run lint— 0 errors; one pre-existing, unrelated info-level style note insrc/ui/src/components/cloud360/Cloud360LiveView.tsx, not touched by this change)Bug repro evidence
dialog.showMessageBox) opens on top of a still-open one when a secondautoUpdaterupdate-downloadedevent fires (e.g. the periodic ~10-minute background check landing while the user hasn't yet answered the first dialog).ReleaseChannelIPCHandlersupdate-downloadedlistener wiring via a mockedautoUpdater.on/dialog.showMessageBox, firing the event twice while the first dialog's promise is deliberately left unresolved.isUpdateDialogOpenguard removed, the test throws (TypeError: Cannot read properties of undefined (reading 'then')) becauseshowMessageBoxis invoked a second time — i.e. a second dialog attempt is made while the first is still open.showMessageBoxis called exactly once across both events; test passes.Follow-up items