fix(updater): don't auto-install Desktop updates when the setting is off (#1104)#1127
fix(updater): don't auto-install Desktop updates when the setting is off (#1104)#1127Kosinkadink wants to merge 3 commits into
Conversation
…off (#1104) The "Automatically install Desktop updates" setting (autoInstallUpdates) gated nothing on the install side: a downloaded update still installed via the Windows startup-install path or electron-updater's install-on-quit, regardless of the setting. (ToDesktop's checkForUpdates always downloads, so the download itself stays - only the install is now gated.) - evaluateStartupInstall(): skip with new `auto_install_disabled` reason when auto-install is off, so a staged update is never applied at startup. - syncInstallOnQuitPolicy(): single source of truth for autoInstallOnAppQuit - off when the startup-install path owns the install OR auto-install is disabled; armed only on non-Windows with auto-install on. Used by register() and re-applied on the autoInstallUpdates toggle so it takes effect without a restart. With the setting off, a downloaded update now waits for an explicit "Desktop Update Ready" pill click (installUpdate) instead of installing automatically on next launch or on close. Amp-Thread-ID: https://ampcode.com/threads/T-019ece99-2c11-75ca-bcdf-3ad07bde982b Co-authored-by: Amp <amp@ampcode.com>
|
Warning Review limit reached
More reviews will be available in 11 minutes and 22 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds a new exported ChangesAuto-install-off policy enforcement
Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
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 |
…view) Code review caught a race: syncInstallOnQuitPolicy() (now re-run on the autoInstallUpdates toggle) could re-arm autoInstallOnAppQuit after the OS session-end guard suppressInstallOnQuit() had disabled it, reintroducing the mid-write install corruption during shutdown. Latch the suppression for the life of the process: suppressInstallOnQuit() sets a flag that syncInstallOnQuitPolicy() honors, so a settings toggle mid-shutdown can never re-arm install-on-quit. Tests: never re-arms after session-end suppression; macOS disables install-on-quit when auto-install is off. Amp-Thread-ID: https://ampcode.com/threads/T-019ece99-2c11-75ca-bcdf-3ad07bde982b Co-authored-by: Amp <amp@ampcode.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/main/lib/updater.test.ts`:
- Around line 487-490: The test for the autoInstallUpdates regression is not
testing a real toggle scenario since the setting is already truthy by default.
Before the line that sets settingsStore['autoInstallUpdates'] = true, first set
it to false, then set it to true to create an actual mid-shutdown toggle. This
demonstrates that a real toggle from false to true still cannot re-arm the
install-on-quit feature. Additionally, condense the comment on the preceding
line to a single concise sentence that explains the regression test's purpose,
following the coding guideline to avoid multi-paragraph justifications for small
changes.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 5f2baf6c-5913-49d0-b784-879de967168d
📒 Files selected for processing (2)
src/main/lib/updater.test.tssrc/main/lib/updater.ts
…tch test (#1104 review) Amp-Thread-ID: https://ampcode.com/threads/T-019ece99-2c11-75ca-bcdf-3ad07bde982b Co-authored-by: Amp <amp@ampcode.com>
Summary
Fixes #1104 - "New updates still install even if the setting to automatically install Desktop updates is disabled."
The "Desktop Settings -> Updates -> Automatically install Desktop updates" setting (
autoInstallUpdates) gated nothing on the install side. Even with it off, a downloaded update was still installed automatically via:evaluateStartupInstallkeyed only offinstallUpdatesOnStartup, neverautoInstallUpdates), andautoInstallOnAppQuit, which was only suppressed for the startup-install path / OS shutdown, never for this setting).So a user who disabled auto-install still got updates installed on next launch or on close.
Why the download still happens
ToDesktop's
autoUpdater.checkForUpdates()always downloads - itsUpdaterAgent.checkAndDownload()is a single atomic check-and-download with no check-only mode in the public API. Rather than bypass ToDesktop's proven detection path (which carries a real risk of missing updates), this change keeps the background download and gates only the install. The setting's label ("Automatically install...") is now literally accurate.Changes
evaluateStartupInstall()- newauto_install_disabledskip reason; a staged update is never applied at startup when the setting is off. (Skip stays silent in telemetry - it's an intentional user choice, not an anomaly.)syncInstallOnQuitPolicy()- single source of truth forautoInstallOnAppQuit: disabled when the startup-install path owns the install or auto-install is off; armed only on non-Windows with auto-install on. Used byregister()and re-applied fromnotifyAutoUpdateChanged()so toggling the setting takes effect without a restart.Resulting behavior (setting OFF)
The update still downloads in the background, then waits for the user to click the "Desktop Update Ready" title-bar pill, which triggers
installUpdate()(the existing user-gated confirm -> restart-and-install flow). No install on next launch, no install on close.Tests
Added to
src/main/lib/updater.test.ts:register()disables install-on-quit when auto-install is off (opted out of startup install)installUpdate()(pill-confirm path) still installs when auto-install is offValidation
pnpm run typecheck,pnpm run lint,pnpm run build, andpnpm run test(2223 tests) all pass.