fix(updates): install macOS updates through electron-updater (#955) - #3431
fix(updates): install macOS updates through electron-updater (#955)#3431jeanfbrito wants to merge 2 commits into
Conversation
The darwin branch of the `update-downloaded` handler destroyed every window
and then drove Electron's native `autoUpdater` directly:
nativeUpdater.checkForUpdates();
nativeUpdater.on('update-downloaded', nativeUpdateDownloadedCallback);
electron-updater's `MacUpdater` already owns that same native `autoUpdater`
singleton. It serves the downloaded zip from a local authenticated proxy
server, calls `setFeedURL` against it, and installs via its own
`quitAndInstall()` — the only path that consults `squirrelDownloadedUpdate`
and reaches `handleUpdateDownloaded()`. Calling `checkForUpdates()` on the
singleton bypassed that bookkeeping, so Squirrel never installed: the app
quit with no windows left and offered the same update on the next launch.
The listener was also registered after the call meant to trigger it, and the
`try/catch` wrapped only the `setImmediate` scheduling rather than its body,
so a throw from the later tick escaped and never dispatched
`UPDATES_ERROR_THROWN` — leaving the failure silent.
Drop the darwin special case and call `autoUpdater.quitAndInstall(true, true)`
on every platform, restoring the behaviour that predated 972f141, and move
the `try/catch` inside the callback so install failures are reported.
Not verified against a live update feed: that needs a packaged, signed macOS
build. `tsc --noEmit`, `eslint`, and the full test suite pass.
WalkthroughChangesmacOS update installation
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/updates/main.ts (2)
288-303: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd test coverage for the deferred install-failure path.
This code block is the direct fix for issue
#955. Add a test in src/updates/main.spec.ts that simulatesautoUpdater.quitAndInstallthrowing inside thesetImmediatecallback and asserts thatUPDATES_ERROR_THROWNis dispatched. This locks in the regression fix and guards against the try/catch placement mistake described in the inline comment reoccurring.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/updates/main.ts` around lines 288 - 303, Add a regression test in the update tests for the deferred install path: mock autoUpdater.quitAndInstall to throw inside the setImmediate callback, then assert dispatch receives an UPDATES_ERROR_THROWN action with the captured error details. Ensure the test flushes the deferred callback and validates the try/catch behavior in the surrounding update flow.
285-304: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the repeated error-dispatch block into a shared helper.
The
error instanceof Error && dispatch({type: UPDATES_ERROR_THROWN, payload: {message, stack, name}})block appears four times in this file (here and at lines 322-330, 340-348, and 366-374). A small helper, for examplereportUpdateError(error: unknown): void, removes the duplication and gives one place to fix the non-Errorhandling gap noted above.const reportUpdateError = (error: unknown): void => { const normalizedError = error instanceof Error ? error : new Error(String(error)); dispatch({ type: UPDATES_ERROR_THROWN, payload: { message: normalizedError.message, stack: normalizedError.stack, name: normalizedError.name, }, }); };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/updates/main.ts` around lines 285 - 304, Extract the repeated UPDATES_ERROR_THROWN dispatch logic into a shared reportUpdateError(error: unknown) helper in src/updates/main.ts. Have it normalize non-Error values into Error instances and dispatch message, stack, and name; then replace the four duplicated error instanceof Error blocks, including the setImmediate catch, with calls to this helper.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/updates/main.ts`:
- Around line 292-301: Update the catch block around quitAndInstall to normalize
every thrown value into an Error before dispatching UPDATES_ERROR_THROWN. Remove
the error instanceof Error guard, preserve existing Error instances, and derive
message, stack, and name from the normalized error so non-Error throws are
reported as well.
- Around line 288-303: Update the setImmediate update-install flow around
autoUpdater.quitAndInstall so a thrown installation error restores the
window-all-closed listener(s) removed by app.removeAllListeners. Perform the
restoration in the catch path before or alongside dispatching
UPDATES_ERROR_THROWN, preserving the existing error payload behavior.
---
Nitpick comments:
In `@src/updates/main.ts`:
- Around line 288-303: Add a regression test in the update tests for the
deferred install path: mock autoUpdater.quitAndInstall to throw inside the
setImmediate callback, then assert dispatch receives an UPDATES_ERROR_THROWN
action with the captured error details. Ensure the test flushes the deferred
callback and validates the try/catch behavior in the surrounding update flow.
- Around line 285-304: Extract the repeated UPDATES_ERROR_THROWN dispatch logic
into a shared reportUpdateError(error: unknown) helper in src/updates/main.ts.
Have it normalize non-Error values into Error instances and dispatch message,
stack, and name; then replace the four duplicated error instanceof Error blocks,
including the setImmediate catch, with calls to this helper.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 031bdcaa-2cbf-49c0-98f7-4ed1400ee893
📒 Files selected for processing (1)
src/updates/main.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (3)
- GitHub Check: check (ubuntu-latest)
- GitHub Check: check (windows-latest)
- GitHub Check: Analyze (javascript)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use TypeScript for new code unless explicitly told otherwise.
Use Fuselage components from@rocket.chat/fuselagefor UI work unless the design requires something Fuselage does not provide.
CheckTheme.d.tsfor valid color tokens before using Fuselage colors.
Verify library props, APIs, and tokens against official docs or local.d.tsfiles instead of assuming.
Use React functional components with hooks.
Redux actions follow FSA shape.
Use camelCase for file names and PascalCase for components.
Prefer clear names over unnecessary comments.
Prefer editing existing files over creating new abstractions unless the new abstraction removes real complexity or matches an existing pattern.
**/*.{ts,tsx}: Use TypeScript for all new code unless explicitly told otherwise.
Use Fuselage components for all UI work; create custom components only when Fuselage lacks the required functionality.
Import Fuselage components from@rocket.chat/fuselage.
Use only valid color tokens documented byTheme.d.ts.
Use optional chaining with fallbacks for platform-specific APIs, especially Linux-only process APIs such asprocess.getuid(),getgid(),geteuid(), andgetegid().
Use TypeScript strict mode.
Redux actions must follow the Flux Standard Action pattern.
Use camelCase for file names and PascalCase for component names.
Avoid unnecessary comments; prefer self-documenting code through clear naming.
Do not commit or push without explicit user permission.
Verify library APIs, props, tokens, and types against official documentation and.d.tsfiles instead of assuming they are valid.
Files:
src/updates/main.ts
🔇 Additional comments (1)
src/updates/main.ts (1)
4-4: LGTM!
…tall
Addresses two review findings on the install path:
- `app.removeAllListeners('window-all-closed')` detaches the app's quit
handler process-wide. The removal is needed so the pending quit isn't
cancelled, but when the install throws we never quit, leaving those
listeners detached for the rest of the session — closing every window would
then no longer reach the handler in src/app/main/app.ts. Capture the
listeners first and reattach them in the error path.
- `error instanceof Error && dispatch(...)` silently dropped non-Error thrown
values, so a thrown string or object produced no error action at all. Since
this block exists specifically to surface install failures that used to go
unreported, normalize any thrown value into a serializable
{message, stack, name} payload before dispatching.
|
Holding this for the release after 4.16.0 rather than merging into the current cycle. The reasoning is the validation gap called out above: nothing in CI executes this code path, and the change can only be meaningfully verified on a packaged, signed macOS build against a real update feed. Since it touches the update mechanism itself — and a regression there costs users the ability to update in-app at all, including to a fix — it should not ride along on a release that is already carrying the navigation rework. Deferring alongside #3427, which reworks the same update surface (its No changes needed on the PR itself: both CodeRabbit findings are addressed and confirmed, |
1 participant
Summary
Fixes the long-standing macOS update loop reported in #955: click install, the app quits, relaunch, and the same update is offered again — forever.
Root cause
The
update-downloadedhandler had a darwin-specific branch that destroyed every window and then drove Electron's nativeautoUpdaterdirectly:But
electron-updater'sMacUpdateralready owns that same nativeautoUpdatersingleton:On macOS it installs by starting a local authenticated HTTP proxy server that serves the already-downloaded zip, pointing Squirrel at it with
setFeedURL, and then going through its ownquitAndInstall()— which is the only path that consultssquirrelDownloadedUpdateand reacheshandleUpdateDownloaded():Calling
checkForUpdates()on the singleton reached around the library and skipped that bookkeeping entirely. Squirrel never installed, and since all windows had just been destroyed there was no UI left and no way back — so the update reappeared on the next launch.Two faults compounded it:
update-downloadedlistener was registered after the call meant to trigger it, so a fast resolution had nothing listening.try/catchwrapped only thesetImmediatescheduling, not its body. A throw from the later tick escaped it, soUPDATES_ERROR_THROWNwas never dispatched and the failure was silent — which is why this went undiagnosed for years.All three came from 972f141 ("change updater to test", Apr 2023), an experiment that shipped.
The fix
autoUpdater.quitAndInstall(true, true)on all platforms and letMacUpdaterdrive Squirrel through its own proxy. This restores the behaviour that predated 972f141.try/catchinside thesetImmediatecallback so install failures are actually reported.BrowserWindowandnativeUpdaterimports and the deadnativeUpdateDownloadedCallback.autoRunAppAfterInstallandautoInstallOnAppQuitboth default totrue, and the only override in this file isautoDownload = false, so the defaultsMacUpdaterrelies on are intact.Testing
tsc --noEmit,eslint, and the fullyarn testsuite (155 suites, 1662 passing, 2 skipped) all pass.Important
These checks prove nothing about the actual fix — they only show nothing else regressed. None of them execute the install path:
setupUpdateshas zero test coverage (src/updates/main.spec.tsonly tests the puremergeConfigurations), which is how this survived eight years.This needs validation on a packaged, signed macOS build against a live update feed before merging. It is the mechanism by which users receive every other fix, and there is no in-app fallback if it misbehaves.
I deliberately did not add a unit test: meaningfully covering this means mocking the
MacUpdater/Squirrel proxy interaction, and a test asserting "we calledquitAndInstall" would just restate the diff while implying coverage of behaviour it cannot reach.Note for #3427
#3427 extracts this same block verbatim into
installDownloadedUpdate(), so it carries the bug forward. Landing this first means that PR inherits the fix on rebase instead of re-shipping it.Closes #955
Summary by CodeRabbit