- Notifications
You must be signed in to change notification settings - Fork5
feat: support disabling the built-in updater#219
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation
095bf83 tof34e415CompareUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Pull Request Overview
This PR adds support for disabling the built-in updater via a UserDefaults setting that can be controlled by MDM policies. WhendisableUpdater is set to true, the updater won't initialize and related UI elements will be hidden.
- Adds
disabledproperty to UpdaterService that checks UserDefaults for updater disable setting - Conditionally initializes the Sparkle updater only when not disabled
- Updates GeneralTab UI to hide updater controls and show explanatory message when disabled
- Refactors some@published properties to private(set) for better encapsulation
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| UpdaterService.swift | Core logic for conditionally disabling updater initialization based on UserDefaults |
| GeneralTab.swift | UI changes to hide updater controls and show disabled message |
| FilePicker.swift | Access control improvements for@published properties |
| VPNService.swift | Access control improvements for@published properties |
| // The auto-updater can be entirely disabled by setting the | ||
| // `disableUpdater` UserDefaults key to `true`. This is designed for use in | ||
| // MDM configurations, where the value can be set to `true` permanently. | ||
| letdisabled:Bool=UserDefaults.standard.bool(forKey:Keys.disableUpdater) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The UserDefaults.bool(forKey:) method returns false when the key doesn't exist, which means the updater will be enabled by default. However, the comment suggests the intent is to disable the updater when the key is set to true. This logic appears inverted - consider using!UserDefaults.standard.bool(forKey: Keys.disableUpdater) if you want the updater enabled by default.
Copilot uses AI. Check for mistakes.
| Button("Check for updates"){ updater.checkForUpdates()}.disabled(!updater.canCheckForUpdates) | ||
| }else{ | ||
| Section{ | ||
| Text("The app updater has been disabled by a device management policy.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
[nitpick] The error message assumes the updater was disabled by device management policy, but the UserDefaults key could be set by other means. Consider making the message more generic like 'The app updater has been disabled.' or 'Automatic updates are not available.'
| Text("The app updater has been disabled by a device management policy.") | |
| Text("The app updater has been disabled.") |
Copilot uses AI. Check for mistakes.
ethanndickson commentedAug 7, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Merge activity
|
830d147 intomainUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Closes#182.
UserDefaults can be forcibly set by MDM admins.
When the
disableUpdaterbool in UserDefaults is set tofalse, the updater won't be initialized on launch, and the UI elements for the updater in settings will be hidden.Related to#220.