feat: let the user turn off the startup version check, and throttle it to once a day - #1724
Merged
Conversation
…t to once a day Closes #1663. AboutViewModel's public constructor hardcoded autoCheck: true, and the About VM is resolved eagerly at startup precisely so that fires — so every launch made TWO calls to api.github.com (latest release, plus the last ten for the history list) with no setting, no UI, and no record of the previous check. Two consequences, both real: The README states network access happens only "for features you explicitly use". The startup check was not a feature the user chose — it ran before any interaction, on every launch, with no switch. That is the one place the product's strongest claim was contestable, and it is now accurate rather than arguable. Second, UpdateService documents GitHub's anonymous limit as 60 requests/hour/IP. Nothing recorded when the last check ran, so restarting the app repeatedly could exhaust it and leave About showing an error for no real reason. Now: a persisted preference gates the startup path, plus a 24h throttle. The manual "Check for updates" and "Retry" buttons deliberately bypass both — the user is never blocked from asking. A checkbox on the About tab carries the switch, worded and shaped like Bulk Installer's existing "Load app icons from the web" toggle (plain language, tooltip naming exactly what does and does not leave the machine) with AutomationProperties.Name per Gate-UX. Two deliberate departures from the issue's proposal: - It suggested copying ThemeService's persistence. ThemeService uses static readonly paths, which is the pattern that made ResourceHistoryService untestable. This follows the modern seam instead — an optional configDir, as in ClosePreferenceService and the seven other persistence services — so the throttle is tested against a temp directory and never touches the developer's own file. - Malformed input falls back to ENABLED, not to off. Defaulting to off would silently close the only channel that tells a user about a fix, and they would never notice. ShouldCheckAtStartup is a pure static taking the clock as a parameter, so the window is tested without sleeping. A future-dated timestamp — a clock moved back, or a file copied from another machine — is treated as stale rather than trusted, so a bad clock cannot suppress checks indefinitely. RecordCheck runs only AFTER the calls actually go out, so a failed check does not start the 24h clock and strand the user for a day. Registered in ProfileService's catalog so the preference survives export/import. Verified that does not break the existing count assertion: AvailableSections only includes files that exist, so a two-file profile still yields two sections — checked against the real service, plus a round-trip proving the new section restores. Docs: the README's network paragraph now states the version check separately and honestly instead of listing it among things the user opted into. ARCHITECTURE gains the service and no longer claims the check happens unconditionally "at startup". SECURITY.md's supported-version table was stale at 1.56.x — updated to 1.58.x. Tests: 20 on the service (defaults, the switch, throttle boundaries at 23/24/72h, future-dated clock, malformed input, directory isolation) and 5 on the view-model. UpdateService is sealed with no interface so the request itself cannot be counted; the VM tests assert what IS observable — with the check off, no version is fetched, no error is shown, and the status explains why. Confirmed the constructor only READS the preference, so tests using the public ctor cannot write to the real profile. All four projects rebuild with 0 warnings.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
|
||
| // Suppresses saving while the constructor applies the loaded value, so restoring the | ||
| // preference does not immediately rewrite the same file. | ||
| private bool _loadingPreference; |
laurentiu021
added a commit
that referenced
this pull request
Aug 18, 2026
CodeQL raised cs/xmldoc/missing-summary #1724 on AudioSessionRowViewModel's constructor: batch 89 added a <param> block for the new reportFailure callback without a <summary> above it, so the whole doc block documented an argument of a member with no stated purpose. Swept the source for the shape rather than fixing the one alert, and found FOUR constructors documented with <param> only — CodeQL had reported just two of them (#1724 and the pre-existing #1715 on DashboardViewModel). The other two are mine too: EtaCalculator (batch 81's TimeProvider seam) and EtwBandwidthSource (batch 86's). All four now lead with a summary, matching BandwidthMonitorViewModel, whose constructors already carry one. Each summary states something the reader cannot get from the signature and that I verified against the body: the audio row seeds volume/mute/name/peak through the backing fields so the change handlers do not echo straight back to the service; EtaCalculator starts with no samples so Remaining stays null; EtwBandwidthSource opens no ETW session until Start(), which is why holding one costs nothing and needs no elevation; DashboardViewModel starts InitAsync fire-and-forget and reads elevation once because it cannot change without a relaunch. Comments only — no behaviour change, no version bump, no release. Co-authored-by: laurentiu021 <laurentiu021@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #1663.
The problem
AboutViewModel's public constructor hardcodedautoCheck: true, and the About VM is resolved eagerly at startup precisely so that fires. So every launch made two calls toapi.github.com— the latest release, plus the last ten for the history list — with no setting, no UI, and no record of the previous check.Two consequences, both real:
1. The privacy claim was contestable. The README states network access happens only "for features you explicitly use". The startup check was not a feature the user chose — it ran before any interaction, on every launch, with no switch. That's the one place the product's strongest claim didn't hold, and it's now accurate rather than arguable.
2. The rate limit.
UpdateServicedocuments GitHub's anonymous limit as 60 req/hour/IP. Nothing recorded when the last check ran, so restarting repeatedly could exhaust it and leave About showing an error for no real reason.The fix
A persisted preference gates the startup path, plus a 24h throttle. The manual Check for updates and Retry buttons deliberately bypass both — the user is never blocked from asking.
The checkbox on the About tab is worded and shaped like Bulk Installer's existing "Load app icons from the web" toggle — plain language, tooltip naming exactly what does and does not leave the machine,
AutomationProperties.Nameper Gate-UX:Two deliberate departures from the proposal
ThemeService's persistence.ThemeServiceusesstatic readonlypaths — the pattern that madeResourceHistoryServiceuntestable (fixed in fix: serialize the Resource History reloads and handle an unreadable history file #1713). This follows the modern seam instead: an optionalconfigDir, as inClosePreferenceServiceand the seven other persistence services. That's what lets the throttle be tested against a temp directory without touching the developer's own file.ClosePreferenceService'sAsk. Defaulting to off would silently close the only channel that tells a user about a fix — a failure they'd never notice.Design details worth flagging
ShouldCheckAtStartupis a pure static taking the clock as a parameter, so the window is tested without sleeping.RecordCheckruns only after the calls actually go out, so a failed check doesn't start the 24h clock and strand the user with stale information for a day.Regression checks
Registered in
ProfileService's catalog so the preference survives export/import. Its tests assert a section count, so I verified against the real service rather than reasoning about it:AvailableSectionsonly includes files that exist, so a two-file profile still yields two sections — plus a round-trip proving the new section restores. 5/5.Confirmed the constructor only reads the preference (the save handler is suppressed during load), so the existing tests that use the public ctor cannot write to the real profile — and verified no
update-check.jsonappeared there.Docs (Gate-DOCS)
Tests
20 on the service — defaults, the switch, throttle boundaries at 23/24/72h, the future-dated clock, malformed input, directory isolation — and 5 on the view-model.
UpdateServiceis sealed with no interface, so the request itself can't be counted. The VM tests assert what is observable: with the check off, no version is fetched, no error is shown, and the status explains why. Harness output:28/28 on the combined harness. All four projects rebuild
--no-incrementalwith 0 errors, 0 warnings.Not in this PR
#1653 (disclose the call in a Welcome overlay) stays open — it's disclosure only, explicitly says not to add a consent gate, and names this toggle as the natural follow-up. It also depends on a Welcome overlay that doesn't exist yet.
The on-screen appearance of the checkbox needs the app running, so that check belongs to the secondary workstation.