fix(desktop): guard installUpdate against repeat clicks on macOS#3549
fix(desktop): guard installUpdate against repeat clicks on macOS#3549
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughAdds an Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
apps/desktop/src/main/lib/auto-updater.test.ts (2)
42-52: Nit:setupAutoUpdater()is re-invoked everybeforeEach.Each call re-registers all
autoUpdater.on(...)listeners and schedules a freshsetInterval. TheremoveAllListeners()above handles the listener duplication, andinterval.unref()keeps the process from hanging, but you're still accumulating timers for the lifetime of the test run. CallingsetupAutoUpdater()once (e.g. in abeforeAll) and only resetting state (removeAllListeners+ re-register, or just re-emit the error to clearisInstalling) inbeforeEachwould be cleaner. Not blocking.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/lib/auto-updater.test.ts` around lines 42 - 52, The test currently calls setupAutoUpdater() inside beforeEach which re-registers autoUpdater.on listeners and creates new setInterval timers each run; instead, move the setupAutoUpdater() invocation to a beforeAll so the module-level listeners and interval are created once, and keep beforeEach to reset state by calling fakeAutoUpdater.removeAllListeners(), fakeAutoUpdater.quitAndInstall.mockClear(), fakeAutoUpdater.checkForUpdates.mockClear(), fakeAutoUpdater.setFeedURL.mockClear(), and re-emitting the reset error with fakeAutoUpdater.emit("error", new Error("reset")) to clear isInstalling; ensure tests still call any per-test re-registration if needed after removeAllListeners().
16-40: Consider mockingmain/env.mainto make the tests hermetic.
auto-updater.tsearly-returns frominstallUpdate()whenenv.NODE_ENV === "development". The suite relies on whatevermain/env.mainresolves to at test time (presumably not"development"), so if that module ever readsprocess.env.NODE_ENVdirectly — or a future change setsNODE_ENV=developmentin the test runner — the "collapses repeat install clicks" test would silently pass-through the dev-mode branch andquitAndInstallwould never be called, masking a regression (or worse, turning green when it shouldn't).Explicitly mocking it removes that coupling:
🧪 Suggested addition
mock.module("main/index", () => ({ setSkipQuitConfirmation: mock(() => {}), })); + +mock.module("main/env.main", () => ({ + env: { NODE_ENV: "production" }, +}));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/lib/auto-updater.test.ts` around lines 16 - 40, The test must explicitly mock the runtime env module so installUpdate() can't early-return in dev mode; before importing "./auto-updater" add a mock for the "main/env.main" module that exports an env (or NODE_ENV) value that is not "development" (e.g., "test" or "production"), ensuring installUpdate and quitAndInstall branches are exercised; update the test setup where mock.module(...) calls are made so the mock for main/env.main appears before the import of installUpdate and AUTO_UPDATE_STATUS.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/desktop/src/main/lib/auto-updater.test.ts`:
- Around line 42-52: The test currently calls setupAutoUpdater() inside
beforeEach which re-registers autoUpdater.on listeners and creates new
setInterval timers each run; instead, move the setupAutoUpdater() invocation to
a beforeAll so the module-level listeners and interval are created once, and
keep beforeEach to reset state by calling fakeAutoUpdater.removeAllListeners(),
fakeAutoUpdater.quitAndInstall.mockClear(),
fakeAutoUpdater.checkForUpdates.mockClear(),
fakeAutoUpdater.setFeedURL.mockClear(), and re-emitting the reset error with
fakeAutoUpdater.emit("error", new Error("reset")) to clear isInstalling; ensure
tests still call any per-test re-registration if needed after
removeAllListeners().
- Around line 16-40: The test must explicitly mock the runtime env module so
installUpdate() can't early-return in dev mode; before importing
"./auto-updater" add a mock for the "main/env.main" module that exports an env
(or NODE_ENV) value that is not "development" (e.g., "test" or "production"),
ensuring installUpdate and quitAndInstall branches are exercised; update the
test setup where mock.module(...) calls are made so the mock for main/env.main
appears before the import of installUpdate and AUTO_UPDATE_STATUS.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ebb40bc5-4957-40c8-9b57-d08eb637901a
📒 Files selected for processing (2)
apps/desktop/src/main/lib/auto-updater.test.tsapps/desktop/src/main/lib/auto-updater.ts
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
MacUpdater.quitAndInstall() registers a fresh native-updater `update-downloaded` listener each call when Squirrel.Mac hasn't finished staging. Repeat clicks on the update button stacked listeners, then fanned out into parallel nativeUpdater.quitAndInstall() calls once Squirrel fired — racing to swap the binary and leaving the app on the old version. Matches the reporter's symptom (app quits, reopens on same version). Add an `isInstalling` guard + `status === READY` precondition so repeat clicks collapse to a single quitAndInstall, and clear the flag in the error handler so the user can retry if Squirrel surfaces an error instead of actually quitting. Closes #3507
Kitenite
force-pushed
the
fix/3507-auto-update
branch
from
9d8f6d7 to
b51271a
Compare
Greptile SummaryThis PR fixes a macOS-specific race condition where repeated clicks on the "Update" button closed the app but left it running on the old version. The root cause was that The fix adds two guards to Key changes:
Confidence Score: 4/5Safe to merge — the implementation fix is correct and minimal; one P1 test portability issue should be resolved before expanding CI to Windows. The production fix in auto-updater.ts is clean, well-commented, and directly addresses the root cause. The guards are ordered correctly and the error-handler reset enables retries. The only issue is in the test file: unguarded platform and env dependencies mean the test suite can silently or loudly fail on Windows runners. Since the bug is macOS-specific and current CI appears to be macOS/Linux, this is non-blocking today but should be fixed before adding any cross-platform CI coverage. apps/desktop/src/main/lib/auto-updater.test.ts — needs mocks for shared/constants (PLATFORM) and optionally main/env.main to be platform-portable.
|
| Filename | Overview |
|---|---|
| apps/desktop/src/main/lib/auto-updater.ts | Adds isInstalling guard and status === READY precondition to installUpdate(), resetting the flag in the error handler — cleanly fixes the macOS parallel-quitAndInstall race. |
| apps/desktop/src/main/lib/auto-updater.test.ts | New test file covering three key scenarios; correct assertions, but tests implicitly require a non-Windows platform due to unguarded IS_AUTO_UPDATE_PLATFORM usage in setupAutoUpdater(). |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[installUpdate called] --> B{NODE_ENV === development?}
B -- Yes --> C[Log: skipped in dev mode\nemitStatus IDLE\nreturn]
B -- No --> D{isInstalling === true?}
D -- Yes --> E[Log: duplicate ignored\nreturn]
D -- No --> F{currentStatus === READY?}
F -- No --> G[Log: update not ready\nreturn]
F -- Yes --> H[isInstalling = true]
H --> I[setSkipQuitConfirmation]
I --> J[autoUpdater.quitAndInstall false, true]
J --> K{Squirrel outcome}
K -- App quits --> L[Process exits — isInstalling moot]
K -- Error fires --> M[error handler:\nisInstalling = false\nemitStatus ERROR]
M --> N[User may retry]
Prompt To Fix All With AI
This is a comment left during a code review.
Path: apps/desktop/src/main/lib/auto-updater.test.ts
Line: 42-52
Comment:
**Tests silently depend on the runtime platform**
`setupAutoUpdater()` early-returns if `IS_AUTO_UPDATE_PLATFORM` is false (i.e. `process.platform !== 'darwin' && process.platform !== 'linux'`). `shared/constants` is not mocked in the test file, so `PLATFORM` is resolved from the actual host OS at module-load time.
On a Windows CI runner:
- `setupAutoUpdater()` exits before registering the `update-downloaded` and `error` handlers.
- `fakeAutoUpdater.emit("error", new Error("reset"))` becomes a no-op — `isInstalling` is never reset.
- `fakeAutoUpdater.emit("update-downloaded", ...)` becomes a no-op — `currentStatus` never reaches `READY`.
- Test 2 ("collapses repeat install clicks") asserts `quitAndInstall` was called once, but it's called zero times → **failure**.
- Test 3 ("clears the in-flight guard") has the same problem.
To make the tests portable, mock the platform dependency. One approach is to mock `shared/constants`:
```ts
mock.module("shared/constants", () => ({
PLATFORM: { IS_MAC: true, IS_LINUX: false, IS_WINDOWS: false },
}));
```
Alternatively, mock `main/env.main` so `NODE_ENV` is always `"test"` regardless of the environment, since that early-return path also affects `installUpdate()` correctness in the tests.
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "fix(desktop): guard installUpdate agains..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| describe("installUpdate", () => { | ||
| beforeEach(() => { | ||
| fakeAutoUpdater.removeAllListeners(); | ||
| fakeAutoUpdater.quitAndInstall.mockClear(); | ||
| fakeAutoUpdater.checkForUpdates.mockClear(); | ||
| fakeAutoUpdater.setFeedURL.mockClear(); | ||
| autoUpdater.setupAutoUpdater(); | ||
| // The module is a singleton; emit an error to reset isInstalling | ||
| // between tests (the error handler clears it). | ||
| fakeAutoUpdater.emit("error", new Error("reset")); | ||
| }); |
There was a problem hiding this comment.
setupAutoUpdater() early-returns if IS_AUTO_UPDATE_PLATFORM is false (i.e. process.platform !== 'darwin' && process.platform !== 'linux'). shared/constants is not mocked in the test file, so PLATFORM is resolved from the actual host OS at module-load time.
On a Windows CI runner:
setupAutoUpdater()exits before registering theupdate-downloadedanderrorhandlers.fakeAutoUpdater.emit("error", new Error("reset"))becomes a no-op —isInstallingis never reset.fakeAutoUpdater.emit("update-downloaded", ...)becomes a no-op —currentStatusnever reachesREADY.- Test 2 ("collapses repeat install clicks") asserts
quitAndInstallwas called once, but it's called zero times → failure. - Test 3 ("clears the in-flight guard") has the same problem.
To make the tests portable, mock the platform dependency. One approach is to mock shared/constants:
mock.module("shared/constants", () => ({
PLATFORM: { IS_MAC: true, IS_LINUX: false, IS_WINDOWS: false },
}));Alternatively, mock main/env.main so NODE_ENV is always "test" regardless of the environment, since that early-return path also affects installUpdate() correctness in the tests.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/main/lib/auto-updater.test.ts
Line: 42-52
Comment:
**Tests silently depend on the runtime platform**
`setupAutoUpdater()` early-returns if `IS_AUTO_UPDATE_PLATFORM` is false (i.e. `process.platform !== 'darwin' && process.platform !== 'linux'`). `shared/constants` is not mocked in the test file, so `PLATFORM` is resolved from the actual host OS at module-load time.
On a Windows CI runner:
- `setupAutoUpdater()` exits before registering the `update-downloaded` and `error` handlers.
- `fakeAutoUpdater.emit("error", new Error("reset"))` becomes a no-op — `isInstalling` is never reset.
- `fakeAutoUpdater.emit("update-downloaded", ...)` becomes a no-op — `currentStatus` never reaches `READY`.
- Test 2 ("collapses repeat install clicks") asserts `quitAndInstall` was called once, but it's called zero times → **failure**.
- Test 3 ("clears the in-flight guard") has the same problem.
To make the tests portable, mock the platform dependency. One approach is to mock `shared/constants`:
```ts
mock.module("shared/constants", () => ({
PLATFORM: { IS_MAC: true, IS_LINUX: false, IS_WINDOWS: false },
}));
```
Alternatively, mock `main/env.main` so `NODE_ENV` is always `"test"` regardless of the environment, since that early-return path also affects `installUpdate()` correctness in the tests.
How can I resolve this? If you propose a fix, please make it concise.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/desktop/src/main/lib/auto-updater.test.ts`:
- Around line 49-51: The test currently resets the singleton by calling
fakeAutoUpdater.emit("error", new Error("reset")), which triggers the production
error handler (setting status to ERROR and calling clearCachedUpdate); instead,
add and use a non-destructive test reset: either (A) expose a guarded test-only
helper on the AutoUpdater module (e.g., AutoUpdater._testReset or a
resetForTests() function in auto-updater.ts) that clears isInstalling and any
test-only state without invoking the real error handler, or (B) emit a synthetic
error shaped like a recoverable network error that isNetworkError(...) will
treat as non-fatal so the handler maps status back to IDLE; update the test to
call that helper or emit the synthetic network error rather than emitting a
plain Error("reset").
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 70405fdc-3343-4dfc-a07c-7e44ae734ada
📒 Files selected for processing (2)
apps/desktop/src/main/lib/auto-updater.test.tsapps/desktop/src/main/lib/auto-updater.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/desktop/src/main/lib/auto-updater.ts
Greptile flagged that setupAutoUpdater() short-circuits on non-mac/linux hosts, so the suite would silently fail on a Windows CI runner (handlers never register, guard never resets). Mock shared/constants to pin the platform. CodeRabbit flagged that the beforeEach reset emitted a non-network error, triggering the real ERROR path (which also clears the cached update). Use a network-shaped error so the handler maps back to IDLE without the extra side effect.
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
1 participant
Summary
MacUpdater.quitAndInstall()registers a freshupdate-downloadedlistener on the native autoUpdater every call while Squirrel.Mac is still staging. Each extra click stacked another listener; when Squirrel finally fired, N parallelnativeUpdater.quitAndInstall()calls raced to swap the binary.isInstallingguard +status === READYprecondition ininstallUpdate, and clear the guard in theerrorhandler so retries are possible.Verified against
electron-updater@6.8.3source (MacUpdater.jslines 236–252) and cross-checked with t3code's equivalentupdateInstallInFlightpattern.Supersedes draft PR #3508 (same root-cause analysis, tightened comments to project conventions).
Closes #3507
Test plan
bun test src/main/lib/auto-updater.test.ts— 3 new tests pass (ignores when not READY, collapses repeats, resets guard on error)bun run typecheckcleanbun run lintclean on changed filesquitAndInstallfires and the app relaunches on the new versionSummary by cubic
Fixes a macOS auto-update bug where repeat Update clicks quit the app but relaunch on the old version. We now gate install requests and ignore duplicates until the install completes.
installUpdate()withisInstallingand requirestatus === READYto prevent parallelquitAndInstallcalls fromelectron-updateron macOS.errorso users can retry if the install fails.shared/constantsto pin macOS and use a network-shaped error in reset to avoid clearing the cached update.Written for commit f589326. Summary will update on new commits.
Summary by CodeRabbit
Tests
Bug Fixes