Repository navigation
One badly-shaped value still costs every Lite setting after it, and the message can't say which key #2444
Description
Activity
- added 8 commits that reference this issue
on Aug 21, 2026 Fixed on
devby #2453. Both judgement calls went the way the issue leaned — a read helper, and reporting the whole set rather than the first — and they turned out to be the same fix, since the old code threw on the first bad key and never reached the rest.The extractor trap was handled by not springing it.
SettingsReader.TryGetPropertycarries that name deliberately, so call sites still readread.TryGetProperty("alert_cpu_threshold", out v)andSettingsSampleTests's regex matches unchanged — the extractor needed no edit at all. That is the right answer, and it creates a dependency nothing compiles-checks, so the guard was strengthened rather than left implicit: both call shapes are proven seen over a source the test writes, the realUndocumentedKeyscomparison is proven to still bite, the trap itself is measured (the #2428ReadInt(root, "key", …)shape run through the same extraction yields nothing), and the method name is asserted directly so a rename fails there with the reason attached.A nice detail: the first draft put
TryGetProperty("inside two comments, which would have injected two junk keys into the extracted set. Caught before it shipped — the extractor cannot tell a comment from code, which is the same reason #2423's sample-file comment had to avoid spelling its own call out.Two latent bugs fixed on the way, neither in the issue: a
(int)Math.Max(0, GetInt64())floor-then-narrow overflow, and a second throw site atelem.GetString()on a non-string array element.The review finding worth recording is a regression introduced mid-branch: choosing the clamp bound by sign turned
analysis_timeout_seconds: 30.0into 600 silently, becauseTryGetInt64fails on fractional tokens as well as on strings. Both new cases were run against the prior commit and are red there — which is the standard that catches this class rather than reasoning about it.Split out as #2456: the Viewer's dialog has the same complaint one level up but a different mechanism — it round-trips a whole object, so the deserialize fails before any property exists and there is no per-property seam to reuse.
- added a commit that references this issue
on Aug 24, 2026
Named by #2428 and again by #2441, unfixed in both because it is its own change rather than a residue of either.
App.LoadAlertSettingswrapsJsonDocument.Parseand all 88TryGetPropertyreads in onetry. #2428 closed the parse half — a malformed document is now reported and quarantined rather than silently resetting everything. What remains is the value half: a single key of the wrong shape (a string where an int is expected, a number where a bool is) throws on its ownGet*call and abandons every read after it.So the settings that survive depend on where the bad key sits in the file. One wrong value near the top costs almost everything; the same value near the bottom costs almost nothing. Nothing in that behaviour is visible to the user, and the ordering it depends on is an implementation detail of the loader.
This is milder than the parse case, which is why it has been deferred twice and reasonably so: #2428 made it loud, so a user in this state now gets a startup message telling them the file could not be read fully. What they do not get is which key, and without that the message sends them to proofread an 88-key document.
Why it is not just "wrap each read"
88 try blocks would work and would be awful to read and maintain. The shapes worth considering first:
A read helper that carries the key name.
TryRead(root, "alert_cpu_threshold", ref CpuThreshold)swallowing and recording per key gets the naming for free and collapses to one line per setting rather than five. It also creates a place to put the clamp, which today is repeated inline at every call site.Report the whole set, not the first. A user with one hand-edited file probably has one mistake, but a user who pasted a block has several. Failing on the first and stopping means they fix, restart, and discover the next one — several times.
Note the constraint from #2418:
SettingsSampleTestsextractsTryGetPropertykey literals by regex out ofApp.xaml.csand requires each to be documented insettings.sample.json. A helper that hides the literal behind a method call will make those keys vanish from the extracted set and turn the guard red — #2428 hit exactly this and had to read back throughJsonDocument. Whatever shape this takes has to keep the key literals greppable, or the extractor has to learn the new shape deliberately rather than by accident.The viewer has the same gap, one level up
Split from #2434: the viewer's startup dialog names the file and the parse position but cannot say which settings were lost, because it round-trips a whole object and the deserialize failed before any property existed. Same question, different mechanism — and the same answer would need per-property reads there too.