One Save, one write, one answer (#2433) by erikdarlingdata · Pull Request #2441 · erikdarlingdata/PerformanceMonitor

erikdarlingdata · 2026-08-21T14:02:42Z

Lite's Settings window said "Settings saved." whether or not a byte reached disk, and the reason was structural: the ten writers behind that one button each opened, parsed and rewrote the whole of settings.json for itself — ten read/parse/write cycles for one click — behind its own catch, and none of them was in a position to tell the caller what had happened.

The premise, checked, with the counts corrected

The issue's shape is right and its arithmetic is off in both directions, so here is what is actually there.

SaveButton_Click calls ten writers, not six or four:

var (mcpChanged, mcpValid) = await SaveMcpSettingsAsync();
SaveDefaultTimeRange();  SaveConnectionTimeout();  SaveCsvSeparator();
SaveColorTheme();        SaveTimeDisplayMode();    SaveCheckForUpdates();
bool alertsValid = SaveAlertSettings();
SaveSmtpSettings();
bool webhooksValid = SaveWebhookSettings();

Seven return void, not four. The four the brief named — SaveColorTheme, SaveTimeDisplayMode, SaveCheckForUpdates, SaveSmtpSettings — are all void, and so are SaveDefaultTimeRange, SaveConnectionTimeout and SaveCsvSeparator, which the brief did not list. Six of the seven route through App.WriteSetting, which reads and rewrites the document exactly as the others do; only SaveSmtpSettings rolls its own.

The brief's correction to the issue is right and worth keeping: the three that return something ARE honoured. if (!alertsValid || !mcpValid || !webhooksValid) return; really does suppress the toast, so the issue's "every one of them lets the button claim success" overstates it. What those three return is whether the BOXES validated, which is a different question from whether the save happened, and no value of any kind existed for the second question.

The other correction is to the issue's own list of remedies. Its option (2) says to fold the four whole-document rewrites into one pass — but the four whole-document rewrites (Mcp, Alert, Smtp, Webhook) and the four void writers it identifies are different sets, overlapping only at SaveSmtpSettings. Consolidating only those four would have left the six WriteSetting knobs rewriting the file separately, so the click would still have made seven writes and the button still would not have had one answer. What is implemented here is option (2) taken to its actual conclusion: all ten.

Option 2, and why

SaveButton_Click now opens the document once, hands it to all ten writers, and writes it once:

JsonNode root = App.SettingsRootForWrite();   // one read, in a try that reports
...
bool written = App.WriteSettingsDocument(root, "settings");   // one write, that answers
switch (SettingsSaveReport.Classify(written, mcpChanged, alertsValid, mcpValid, webhooksValid))

The writers no longer read or write anything; they mutate what they are handed. That does more than give the button a bool: it deletes the state that made a single honest sentence impossible. "SMTP saved, alerts not" was reachable while there were ten writes and is unreachable now that there is one, which is why this beats the other two options rather than merely costing more than them. SaveSmtpSettings stays void and is honestly so now — the problem was never that void was the wrong signature, it was that the method could fail at something it had no way to mention, and it cannot fail any more.

The narrow residue #2433 was actually filed for is now reported. App.SettingsRootForWrite throws only when settings.json cannot be parsed AND cannot be copied aside (#2425) — a directory that will not accept a File.Copy, a locked file, a full disk. That throw used to land in ten separate catches that each logged and let the toast claim success. It now stops the save before any writer runs, and says so.

SaveMcpSettingsAsync's catch was the instance the review bot flagged. With the read and the write both gone from it, the only thing left inside is the port bind probe, so Valid = true is no longer even tempting: if the probe threw we do not know whether the port is free, the MCP keys are left as they were, and the user is told.

The wiring guard, restated rather than patched

#2425's SettingsWriterQuarantineWiringTests counted File.WriteAllText(settingsPath against SettingsRootForWrite() per file, and this change legitimately moves that anchor — SettingsWindow.xaml.cs now has zero of the first and one of the second. Rather than adjust the counting, the invariant is restated stronger and simpler: settings.json has exactly ONE writer in the whole of Lite, and that writer takes the quarantining read. A second writer brings back both defects at once — its own read can replace an unparseable file without copying it aside (#2425), and a save split across several writes has no single honest answer to report (#2433).

Both scans now run with comments stripped, and that is not tidiness — the first run of the new all-of-Lite scan went red on SettingsFileGuard.cs, because the one file that explains why JsonNode.Parse(json) ?? new JsonObject() is banned has to quote it in order to explain it. Same trap as #2418's TryGetProperty key extractor, one file over.

Verification

Lite.Tests targets net10.0-windows and cannot run on macOS, so the real test files were compiled into a throwaway net10.0 harness with a minimal xUnit shim, against the real SettingsFileGuard.cs and the real SettingsSaveReport.cs, and run against both branches. dev has no classifier, so it got a faithful transcription of what dev's SaveButton_Click actually decides — including the fact that whether anything was written is not an input it has, because no such value existed for it to consult:

if (!alertsValid || !mcpValid || !webhooksValid) return SettingsSaveOutcome.WrittenWithObjections;
return mcpChanged ? SavedAndMcpNeedsRestart : Saved;

	dev's behavior	this branch
`SettingsSaveReportTests` + `SettingsSaveButtonHonestyTests` + `SettingsWriterQuarantineWiringTests` + the existing `SettingsFileGuardTests`	16 passed, 10 failed	26 passed, 0 failed

Three more arms came out of review, all of them the same defect wearing different clothes. The theme selector applies its choice LIVE, and _saved is what stops the close handlers reverting that preview — set unconditionally on the click, a failed write showed the new "nothing was saved" dialog while the unpersisted theme stayed applied for the rest of the run and quietly reverted at the next launch. _saved now tracks the write, and TheLiveThemePreview_OnlySurvivesASaveThatWrote asserts both halves: the assignment must be the write's, and the close handlers must still be reading it, or the assertion describes a gate that has moved.

McpSettingsChanged was the same shape and a louder claim. MainWindow reads it after ShowDialog and, when set, stops and restarts the MCP server — dropping every connected client — then reloads the port from settings.json on disk, not from the document the window just built. A failed write therefore produced a disruptive restart back onto the OLD configuration, moments after the app had said nothing was saved. Same gate now.

And the mutators were outside the guard. Before consolidation each writer carried its own try, so an exception thrown while BUILDING a value — not only on the disk I/O — was caught, logged, and the remaining writers still ran. Guarding only the read and the write left the nine mutators between them able to escape into App's generic "An error occurred" dispatcher dialog, which is precisely the class of silence this change exists to remove. The read and every mutator sit under one catch now, funnelling into the same "Nothing was saved" the I/O failure gets — equally true there, and the reason the diff shows one try rather than two.

The headline failure is the one sentence this issue is about — AFailedWrite_IsNeverReportedAsSaved: expected <NothingWritten> actual <Saved>. The other nine are its three theory rows, the theme-preview, MCP-restart and mutator-guard pins, both source-wiring guards on SaveButton_Click, and the one-writer pin, which reports dev exactly: "5 whole-document rewrite(s) of settings.json across 2 file(s): App.xaml.cs (1), SettingsWindow.xaml.cs (4)".

The nine passing SettingsFileGuardTests and two of the three wiring tests pass on both sides, which is the point of including them: they are #2425's guarantees and this change must not have moved any of them.

One note on the merge

#2435 landed on dev while this was open and added a block to SaveButton_Click: when the window opened over an unreadable settings.json, re-read McpSettings after saving and refresh the status line, because the save may have fixed the file and this window stays open afterwards. It conflicts textually with a change that replaced the whole method.

Resolved by keeping both ideas rather than either version. dev's copy of the pre-consolidation body goes; its re-read now sits after the single written, where the ten independent attempts it was written against used to be. Its reasoning survives the move and gets sharper — "a write that failed leaves the file exactly as unreadable as it was" was a hypothetical when nothing could report a failed write, and is now a value the method holds.

Deliberately not in scope

App.WriteSetting survives for the single-value knobs OUTSIDE this window — MainWindow's Overview sort selector is the live caller — because those really are one read and one write on their own, with no sibling writer to be inconsistent with and no toast to be wrong. It now routes through WriteSettingsDocument, so its failure is named under the setting the operator changed rather than under a filename, but its swallow-and-log contract is unchanged and it is still the shape #2425 blessed.

The eighty-eight alert reads still share one try in App.LoadAlertSettings, so one value of the wrong shape still costs every setting after it. Named out loud since #2428 and still filed rather than folded in.

McpSettings.Load's bare catch, which silently disables the MCP server on an unreadable file, is untouched here — it is a load, not a save, and this diff is about what the Save button is entitled to say.

Fixes #2433

Lite has five methods that each end in File.WriteAllText(settingsPath, ...) inside their own catch. None of them can tell a caller whether the write happened, which is why the Settings window says "Settings saved." over a save that wrote nothing -- the log is the only place the truth exists, and nobody reads a log after a dialog says it worked. WriteSettingsDocument is the one write, and it answers. WriteSetting keeps its shape for the single-value knobs outside the Settings window, where one read and one write really is the whole operation, and now routes through it so the failure it used to swallow gets named under the setting the operator was changing rather than under a filename. SettingsSaveReport is the rule the toast follows, kept pure so it can be pinned without a UI thread. The ordering worth writing down is that a failed write outranks a validation objection: a writer that rejected a value has already raised its own dialog about that value, while nothing anywhere tells the user that none of it was saved. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

The Settings window's Save button calls ten writers, and each one opened, parsed and rewrote the whole of settings.json for itself -- ten read/parse/ write cycles for one click -- behind its own catch. Seven of them return void and so cannot report a write failure by construction. The three that return something return whether the BOXES validated, which is a different question, and the caller does honour those. So the toast was shown whenever nothing objected, and "nothing objected" was never about whether a byte reached disk. The issue offered three options and this is the second, because it makes the question go away rather than answering it. SaveButton_Click opens the document once, hands it to all ten writers, and writes it once. The writers no longer read or write anything; they mutate what they are given. That does not merely give the button a bool to report -- it deletes the state that made a single sentence impossible in the first place, because "SMTP saved, alerts not" was reachable while there were ten writes and is unreachable now that there is one. The residue the issue named is now reported instead of swallowed. When settings.json cannot be parsed AND cannot be copied aside, SettingsRootForWrite throws; that throw used to land in ten separate catches that each logged and let the toast claim success, and it now stops the save before a writer runs and says so. SaveMcpSettingsAsync's catch was the instance the review bot found: with the read and the write both gone from it, all that is left in there is the port bind probe, so returning Valid = true is no longer even tempting -- if the probe threw we do not know the port is free, the MCP keys stay as they were, and the user is told. SaveSmtpSettings is still void, and honestly so now. It was not that void was the wrong signature; it was that the method could fail at something it had no way to mention. It cannot fail any more. The #2425 wiring guard's anchor legitimately moves with this, so it is restated stronger rather than patched: instead of counting rewrites against guarded reads file by file, it pins that settings.json has exactly ONE writer in the whole of Lite and that the writer takes the quarantining read. A second writer would bring back both defects at once. Both scans now run with comments stripped, because the file that explains why `JsonNode.Parse(json) ?? new JsonObject()` is banned has to quote it to explain it -- the same trap #2418's key extractor has, one file over. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

claude · 2026-08-21T14:06:51Z

+
+        bool written = App.WriteSettingsDocument(root, "settings");

        _saved = true;


_saved = true is set unconditionally here, before written is known to the switch below. When App.WriteSettingsDocument fails, the NothingWritten branch tells the user "nothing was saved and the changes on this page will be gone when the window closes" — but _saved is already true, so CloseButton_Click/OnClosing will not revert the live theme preview (if (!_saved) ThemeManager.Apply(_originalTheme);). A theme picked in this session but never persisted to settings.json stays applied for the rest of the run, directly contradicting the dialog text this PR just added.

Since this PR is specifically about making the save outcome honest, _saved should probably track written (e.g. _saved = written;) rather than being set unconditionally before the classification.

claude · 2026-08-21T14:06:59Z

Reviewed. This is a Lite-only C# change (no T-SQL, no collector changes), consolidating the Settings window's ten separate settings.json read/mutate/write cycles into a single read → mutate → write, with a SettingsSaveReport.Classify pure function pinning the reporting rule. The design (single writer, NothingWritten outranks validation objections) is sound and well tested.

Parity check: Darling's SettingsWindow.xaml.cs doesn't share this defect — its viewer-local prefs go through _appSettingsStore.Save, and the monitoring-settings side already reports write failures individually via try/catch around UpsertAlertSettingsAsync/UpdateServiceFlagsAsync with its own ViewerReadOnlyException/ViewerSchemaSkewException handling. No parity drift introduced by this PR.

One correctness issue found (left as an inline comment on SettingsWindow.xaml.cs:261): _saved = true is still set unconditionally before the write-outcome switch, so when the write actually fails and the new NothingWritten dialog tells the user "nothing was saved... will be gone when the window closes," the live theme preview isn't reverted on close — contradicting that new message.

No SQL-side or security concerns; didn't flag missing-index-style DMV recommendations per instructions (n/a here anyway).

The review bot caught the window contradicting itself, and it is a fair catch on a PR whose whole subject is the window's honesty. The theme selector applies its choice LIVE, and _saved is what stops CloseButton_Click and OnClosing reverting that preview. Setting it unconditionally on the click meant a failed write showed the new "nothing was saved" dialog while the unpersisted theme stayed applied for the rest of the run -- and came back to the old one at the next launch, with nothing in between to explain it. _saved now tracks the write. The test asserts both halves, because the assertion is worthless if the gate it describes has moved: the assignment must be the write's, and the close handlers must still be reading it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

claude · 2026-08-21T14:18:33Z

+           the window contradict itself: the dialog below says nothing was saved while the unpersisted theme
+           stays applied for the rest of the run and comes back to its old value on the next launch. */
+        _saved = written;
        if (mcpChanged) McpSettingsChanged = true;


McpSettingsChanged is still set unconditionally on mcpChanged, unlike _saved right above it, which was just fixed to track written for exactly this reason.

If App.WriteSettingsDocument fails (line 259, written = false), execution falls through to the NothingWritten case and returns — but McpSettingsChanged has already been set to true here, before the switch runs. SettingsButton_Click in MainWindow.xaml.cs (~line 1381) reads that property after ShowDialog() returns and unconditionally does:

if (window.McpSettingsChanged) { await StopMcpServerAsync(); await StartMcpServerAsync(); }

StartMcpServerAsync reloads via McpSettings.Load(App.ConfigDirectory), which reads settings.json from disk — not the in-memory root. So on a failed write, MainWindow still stops and restarts the MCP server (dropping any connected MCP clients), and it comes back up with the old on-disk port/enabled state, i.e. a disruptive no-op restart for a save that the app just told the user did not happen.

Suggest gating this the same way _saved is:

if (mcpChanged && written) McpSettingsChanged = true;

claude · 2026-08-21T14:18:39Z

+        var (mcpChanged, mcpValid) = await SaveMcpSettingsAsync(root);
+        SaveDefaultTimeRange(root);
+        SaveConnectionTimeout(root);
+        SaveCsvSeparator(root);
+        SaveColorTheme(root);
+        SaveTimeDisplayMode(root);
+        SaveCheckForUpdates(root);
+        bool alertsValid = SaveAlertSettings(root);
+        SaveSmtpSettings(root);
+        bool webhooksValid = SaveWebhookSettings(root);
+
+        bool written = App.WriteSettingsDocument(root, "settings");


Minor robustness note: before this change, each Save* writer's mutation and its File.WriteAllText shared one try/catch (via WriteSetting/its own inline try), so an unexpected exception thrown while building a value (not just the disk I/O) was caught, logged under that one setting, and the remaining writers still ran.

Now only App.SettingsRootForWrite() (top of the method) and the write itself (inside WriteSettingsDocument) are guarded. If any of the nine synchronous mutators between them throws for a reason unrelated to disk I/O (e.g. an unexpected JsonNode value-type mismatch, or a null entry surfacing from App.AlertExcludedDatabases in SaveAlertSettings), it now propagates out of SaveButton_Click entirely: none of the later mutators run, the document is never written, and the user sees App's generic DispatcherUnhandledException dialog ("An error occurred... The application will attempt to continue") instead of the settings-specific "Nothing was saved" messaging this PR just added for the I/O case.

This is probably an acceptable trade-off given the "one document, one write, no partial state" design goal, but it's a behavior change beyond what's described in the PR (which frames the remaining exception surface as just the SettingsRootForWrite() parse/quarantine failure). Worth a conscious call — and maybe worth wrapping the mutator sequence in one more try/catch that funnels into the same "Nothing was saved" dialog, so unexpected exceptions get the same honest treatment as I/O failures do now.

claude · 2026-08-21T14:18:53Z

Review summary

This is a well-scoped, well-tested fix: collapsing ten independent read/mutate/write cycles into one shared JsonNode mutated in place and written once genuinely eliminates the "SMTP saved, alerts not" partial-save state, not just describes it. The wiring guard restated as "settings.json has exactly one writer in all of Lite" is a good simplification of the old per-file counting guard, and the new SettingsSaveReport.Classify pure-function ordering rule (failed write outranks a validation objection) is sound and well covered by SettingsSaveReportTests.

Left two inline comments on Lite/Windows/SettingsWindow.xaml.cs:

Bug: McpSettingsChanged (line 266) is set unconditionally on mcpChanged, unlike _saved right above it which now correctly tracks written. On a failed write, MainWindow will still stop/restart the MCP server based on stale on-disk settings — a disruptive no-op restart for a save that was just reported as not having happened.
Minor robustness note: the nine synchronous Save*(root) mutators are no longer individually try/caught (only the top-level read and the final write are), so a non-I/O exception during mutation now aborts the whole save and surfaces as the generic dispatcher-level error dialog rather than the settings-specific "Nothing was saved" messaging. Likely an acceptable trade-off given the design goal, but worth a conscious call-out since the PR description doesn't mention this widened exception surface.

Lite/Darling parity note (not part of this diff, but directly relevant given what this PR fixes): Darling/PerformanceMonitor.Darling.Viewer/SettingsWindow.xaml.cs has the same bug this PR just fixed for Lite's _saved/theme-preview flag, in its own _themeSaved flag. SaveColorTheme() (line 679) sets _themeSaved = true unconditionally as soon as it's called (line 1508, early in SaveButton_Click), well before the async control-plane writes at lines 1540–1566 that can fail with ViewerReadOnlyException, ViewerSchemaSkewException, or a generic Exception (each of which returns without throwing further). So a save that fails at the control-plane write still leaves _themeSaved = true, and CloseButton_Click/OnClosing won't revert the previewed-but-unpersisted theme — exactly the "theme preview survives a failed save" defect this PR just closed in Lite. Worth a follow-up issue/PR to bring Darling in line, mirroring the _saved = written-style fix here (e.g. _themeSaved should track whether the store write actually succeeded, not just that SaveColorTheme() ran).

No SQL/T-SQL surface in this PR (pure C#/WPF), so the T-SQL style conventions don't apply here.

#2435 landed while this was open and added a block to SaveButton_Click: when the window opened over an unreadable settings.json, re-read McpSettings after saving and refresh the status line, because the save may have fixed the file and the window stays open afterwards. It conflicts textually with this branch, which replaced that whole method. Resolved by keeping both ideas rather than either version. dev's copy of the pre-consolidation body goes; its re-read moves to sit after the single `written`, where the ten independent attempts it was written against used to be. The reasoning survives the move intact and gets sharper: "a write that failed leaves the file exactly as unreadable as it was" was a hypothetical when nothing could report a failed write, and is now a value the method holds. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

Both from review, and both the same defect as the toast wearing different clothes. McpSettingsChanged was still set on the click. MainWindow reads it after ShowDialog and, when it is set, stops and restarts the MCP server -- dropping every connected client -- and then reloads the port from settings.json on DISK rather than from the document the window just built. So a failed write produced a disruptive restart back onto the OLD configuration, moments after the app had told the user nothing was saved. It is a louder claim than the toast is, and it now takes the same gate. And the mutators were outside the guard. Before the consolidation each writer carried its own try, so an exception thrown while BUILDING a value -- not only on the disk I/O -- was caught, logged, and the remaining writers still ran. Guarding just the read and the write left the nine mutators between them able to escape into App's generic "An error occurred" dispatcher dialog, which is exactly the kind of silence this change exists to remove. The read and every mutator are under one catch now, and it funnels into the same "Nothing was saved" the I/O failure gets, because that is equally true there. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

claude · 2026-08-21T14:31:41Z

+    private static string WithoutComments(string source)
+    {
+        var withoutBlocks = Regex.Replace(source, @"/\*.*?\*/", "", RegexOptions.Singleline);
+        return Regex.Replace(withoutBlocks, @"//[^\r\n]*", "");


WithoutComments strips //[^\r\n]* naively, which also fires on the // inside any http:// (or https://) string literal — not just real line comments. That's not hypothetical here: Lite/Windows/SettingsWindow.xaml.cs:201 already has

McpStatusText.Text = $"Status: Running on http://localhost:{_mcpService.Port}";

Everything on that physical line after http: gets silently deleted by this "comment stripper" before the regex scan runs. Today nothing load-bearing follows on that same line, so SettingsJson_HasExactlyOneWriterInAllOfLite and NoWriter_FallsBackToAFreshDocumentOnAFailedParse still pass — but the failure mode is a silent false negative: if a future edit ever puts a real File.WriteAllText(settingsPath...) or a banned JsonNode.Parse(...) ?? new JsonObject() on the same line as (or after) an http:// literal, this guard will no longer see it and will pass when it should fail. That's exactly the class of bug these tests exist to catch (per the comment right above this method, referencing #2418's same trap) — worth using a proper C#-comment-aware strip (e.g. skip ////* */ that occur inside string/interpolation literals) rather than a bare regex.

claude · 2026-08-21T14:32:02Z

Reviewed the diff (Lite.Tests/SettingsFileGuardTests.cs, Lite.Tests/SettingsSaveReportTests.cs, Lite/App.xaml.cs, Lite/Services/SettingsSaveReport.cs, Lite/Windows/SettingsWindow.xaml.cs).

Overall: the consolidation itself is sound. Traced through SaveButton_Click's new one-read/ten-mutators/one-write flow, SettingsSaveReport.Classify's ordering, the _saved/McpSettingsChanged write-gating, and the SaveMcpSettingsAsync catch no longer returning Valid = true on an unknown port state — all consistent with the PR description and each other. The definite-assignment pattern (root/mcpChanged/etc. assigned only inside try, with catch unconditionally returning) is valid C# and compiles fine. Left two inline comments on real-but-non-blocking issues:

Lite.Tests/SettingsFileGuardTests.cs — the new WithoutComments helper's //[^\r\n]* strip is not string-literal-aware, so it silently truncates any line containing http:///https:// (already present in SettingsWindow.xaml.cs:201). Doesn't break today's scan, but it's a latent false-negative in a guard whose whole job is to prevent a second settings.json writer from creeping back in.
Lite/App.xaml.cs (outside the diff hunk, so noted here instead of inline) — SettingsRootForWrite()'s doc comment still says it's called by "the four Save* methods in SettingsWindow," which was true pre-Lite's Settings window says "Settings saved." on a save that wrote nothing #2433 but is no longer true now that those methods take a shared root parameter and the only caller is SaveButton_Click, once. Worth a touch-up given this PR's whole point is nailing "exactly one writer" into the codebase's self-documentation.

Parity check (Lite/Darling): this PR only touches Lite. I checked Darling/PerformanceMonitor.Darling.Viewer/SettingsWindow.xaml.cs for the analogous defect — its SaveButton_Click is architecturally different (DB-backed _dataService calls with per-call try/catch that already report specific failures, and no unconditional "Settings saved" toast to lie about), so it doesn't have the same false-success bug this PR fixes and doesn't need the same fix. Worth knowing for later, though: ViewerAppSettingsStore.Save() (ViewerAppSettings.cs:310) calls File.WriteAllText with no try/catch at all, and SaveButton_Click invokes it (_appSettingsStore.Save(_appSettings)) unguarded — a failed write there throws out of an async void handler instead of being reported. Not a regression from this PR and not in scope, but the same underlying question ("did the local settings file actually get written?") is currently unanswered on the Darling side too, just via a different failure mode (crash/unhandled exception vs. false success).

No correctness issues found in the core save/report/gate logic, no missing-index-DMV concerns (N/A, no T-SQL here), and no obvious secret-handling or injection issues — the webhook cleartext-HTTP confirmation and Credential Manager routing for secrets are unchanged from before.

The viewer's startup dialog could name the file and the parse position and nothing else. That is not a wording problem: it round-trips a whole object, so when JsonSerializer.Deserialize threw there was no property in existence to report -- and every setting in the file reverted, not just the bad one. Per-property reads, the shape erikdarlingdata#2444 gave Lite, are the wrong answer here, and the reason is not the eighty-odd lines of typing the issue weighed. Lite's settings.json is built key by key on BOTH sides -- erikdarlingdata#2441 made every writer mutate one JsonObject -- so a per-key read is symmetric with a per-key write. These files are a whole-object round trip on both sides. Converting only the read half would break that symmetry: a property added to ViewerAppSettings afterwards would still be serialized on save and would silently never be read back. That is a worse defect than the one being fixed, and an invisible one. So the fix keeps the round trip and works on the exception instead. Measured rather than assumed, against the shipped BCL: JsonException.Path is "$" for a document fault and "$.AlertCpuThreshold" for a member's value, every time and for every fault class -- string-where-int, string-where-bool, number-where-string, number-where-list, a bad element inside a list, an overflow, a null. So the split between "the document is broken" and "one setting is the wrong shape" is a fact the reader already had in hand. SettingsFileGuard.Describe was throwing it away, deliberately: WithoutPathSuffix cuts " Path: ..." off the message because it duplicates the line and position, and Path is the one part that does not. ReadObject now names the member the deserializer stopped on, drops it, and runs the SAME deserialize again. Two consequences, and the second is the issue's title: a badly-shaped setting costs exactly its own setting, and the read can report the whole set rather than the first one -- which the path alone cannot, because a file with three bad members reports only whichever the reader met first. It is one judge, not two. erikdarlingdata#2213 spent a review round on a two-pass classify whose second pass judged by a different standard from the first; here every attempt is JsonSerializer.Deserialize<T> with the caller's own options over the caller's own type, and the only thing that changes between attempts is that one member is gone. Only a top-level member of a root JSON OBJECT is ever dropped, and that restriction is the load-bearing half. The viewer's server registry is a root ARRAY: "drop the element that would not deserialize" there means silently deleting a server the operator added -- the data loss erikdarlingdata#2434 exists to prevent, wearing a repair's clothes. The registry keeps its all-or-nothing behaviour, ViewerServerStore says so where the next reader will find it, and there is a control test for exactly that mistake because it was one line away. The state stays Unreadable for a partially recovered file rather than becoming some third thing, so PermitReplace still copies the original aside before the next save replaces it. Relaxing that would have destroyed the one setting the user actually got wrong, with no copy of it anywhere -- strictly worse than dev, where nothing was recovered and everything was preserved. The dialog gets two paragraphs, because there are two facts. A file that could not be read at all costs every setting in it; a file read after dropping named members costs only those. One sentence covering both would have to overstate one of them. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

erikdarlingdata and others added 2 commits August 21, 2026 15:01

claude Bot reviewed Aug 21, 2026

View reviewed changes

erikdarlingdata and others added 2 commits August 21, 2026 15:22

claude Bot reviewed Aug 21, 2026

View reviewed changes

erikdarlingdata merged commit 7284da6 into dev Aug 21, 2026
7 checks passed

erikdarlingdata deleted the fix/2433-settings-saved-honesty branch September 12, 2026 20:29

Navigation Menu

One Save, one write, one answer (#2433) - #2441

One Save, one write, one answer (#2433)#2441
erikdarlingdata merged 5 commits into
devfrom
fix/2433-settings-saved-honesty

erikdarlingdata commented Aug 21, 2026 •

edited

Loading

Uh oh!

claude Bot Aug 21, 2026

Uh oh!

claude Bot commented Aug 21, 2026

Uh oh!

claude Bot Aug 21, 2026

Uh oh!

claude Bot Aug 21, 2026

Uh oh!

claude Bot commented Aug 21, 2026

Uh oh!

claude Bot Aug 21, 2026

Uh oh!

claude Bot commented Aug 21, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant


		bool written = App.WriteSettingsDocument(root, "settings");

		_saved = true;

Conversation

erikdarlingdata commented Aug 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

The premise, checked, with the counts corrected

Option 2, and why

The wiring guard, restated rather than patched

Verification

One note on the merge

Deliberately not in scope

Uh oh!

claude Bot Aug 21, 2026

Choose a reason for hiding this comment

Uh oh!

claude Bot commented Aug 21, 2026

Uh oh!

claude Bot Aug 21, 2026

Choose a reason for hiding this comment

Uh oh!

claude Bot Aug 21, 2026

Choose a reason for hiding this comment

Uh oh!

claude Bot commented Aug 21, 2026

Review summary

Uh oh!

claude Bot Aug 21, 2026

Choose a reason for hiding this comment

Uh oh!

claude Bot commented Aug 21, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

erikdarlingdata commented Aug 21, 2026 •

edited

Loading