[tylerkron]

Not merging — for review. warning-zero routine, one coherent class.

The class

MVVMTK0034 — "field annotated with [ObservableProperty] should not be directly referenced (use the generated property instead)". Five instances, which is every instance in the repo, all in Daqifi.Avalonia/Daqifi.Desktop/Loggers/LoggedSeriesLegendItem.cs. After this the code is absent from the build entirely rather than reduced.

All five are reads, not writes:

site	expression
37,40 · 38,17 · 39,11	TruncatedSerialNo reads _deviceSerialNo three times
58,65 · 58,82	the minimap-sync callback inside IsVisible's setter passes _deviceSerialNo and _channelName to DatabaseLogger.SetMinimapSeriesVisibility

Why this is behaviour-preserving

The generated getter is get => _deviceSerialNo; — a field read with no side effect — so each substitution is value-identical by construction.

No write is touched, which is the part that would have mattered. MVVMTK0034 on a write is a behaviour change: the generated setter raises PropertyChanged where a direct field write does not. There are no flagged writes here — the analyzer exempts the declaring type's own constructor, which is why the four direct field assignments on lines 102–105 carry no warning. Those are deliberately left alone; routing them through the generated setters would start raising notifications during construction, which is a real change and not what this routine is for.

Nothing else moved:

• no [NotifyPropertyChangedFor] was added — that would change when TruncatedSerialNo re-renders;
• the _isVisible reads on lines 53 and 58 stay field reads, because _isVisible backs a hand-written property, not an [ObservableProperty];
• no suppression, NoWarn or #pragma appears anywhere in the diff.

Tests

TruncatedSerialNo had no coverage at all, so the substitution was unwitnessed. Ten test cases were added to the existing LegendFormattingTests — the file that already characterises the other two legend formatters, for exactly this reason.

They have teeth. Mutating the production property to >= 4 and dropping the ?? string.Empty guard fails exactly 2 of the 29 tests in that class — A_serial_of_four_characters_or_fewer_is_shown_whole(serial: "4104"), the boundary, and A_missing_serial_renders_as_an_empty_string — and the other 27 pass. That control was run after the final state of the fix, not only before it.

The null case is not hypothetical: PlotModelFactory.cs:173 carries a live CS8604 for passing a possibly-null deviceSerialNo into this constructor, so the property's two null guards are load-bearing and are now pinned.

The measurement

Derived from CI's own logs at both ends. That avoids the incremental-rebuild trap (CI builds cold, so nothing is skipped as up to date) and needs no third_party heuristic (the totals are all-project).

• base: merge-base 74308c5, run 35161937326
• head: 0b77c4f, run 35192368199

Warning lines are de-duplicated on (file, line, col, code, message). MSBuild prints every warning twice — once inline and once in the trailing summary block — so the raw matched-line count is exactly 2× the truth (348 → 174 at the base, 338 → 169 on this branch).

Reconciliation, so the extraction is checkable rather than asserted:

	base 74308c5	head 0b77c4f
first-party distinct	154	149
vendored third_party/oxyplot-avalonia	20	20
distinct total	174	169
un-located AVLN3001 (carries no line/col)	1	1
= MSBuild's own "N Warning(s)", Desktop	175	170
iOS Release (+1 SDK RuntimeIdentifier override warning)	176	171
Android	181	176

Every head moves by the same −5.

The vendored block is the known 20 — 13 AVP1002 plus 4 CS0108, 2 CS0169 and 1 CS0618 — identical on both sides. (Filtering it on AVP1002 alone would come up 7 short, and the AVLN3001 line carries no (line,col), so a parser that requires one silently loses it and reports 174 against MSBuild's 175. Both were caught by the identity above before any figure was written here.)

Delta: −5 on the first-party set, and the set diff is a PURE DELETION — 5 removed, all five MVVMTK0034, 0 added, no line-number shift. All 149 first-party warnings on this branch appear in the base's 154 at the same file, line, column, code and message; the edit replaces identifiers in place and changes no line count. MVVMTK0034 goes 5 → 0.

Denominator named: these are the all-project de-duplicated figures for the whole build, not the desktop head project alone.

Diffstat

 Daqifi.Avalonia.Tests/Loggers/LegendFormattingTests.cs           | 118 +++++++++++++++++++++-
 Daqifi.Avalonia/Daqifi.Desktop/Loggers/LoggedSeriesLegendItem.cs |  8 +-

The production change is +4/−4 across two hunks — five identifiers, no lines added or removed, no logic. The other 118 added lines are the tests and their documentation.

Gate

• Daqifi.Avalonia.Desktop — succeeded
• Daqifi.Avalonia.iOS Release iossimulator-arm64 (restore without -r, build with it under --no-restore) — succeeded; packages.lock.json unchanged afterwards, confirmed clean
• dotnet test — 875 passed, 0 failed
• The Android head cannot be built on the machine this was written on (only the ios workload is installed), so no local claim is made about it — but CI's Android head job is green on this commit, which is where the 181 → 176 figure above comes from.

All six CI jobs are green on 0b77c4f.

Qodo

Two rounds, both on this branch's own head SHAs, both Bugs (0) / Rule violations (0) / Requirement gaps (0) with zero inline review threads on either surface.

Grade disclosed: both rounds graded 🚀 Fast, not Deep — which is what a delta this small earns, and it is the reason the behaviour-preservation argument above is backed by a positive control and a pinning test rather than by the review outcome. Round 1's grade was not rendered at the time; it only became visible once round 2 created the "Previous reviews" block, which now shows Fast for both.

Round 2 exists because the hand-review found that this PR's central premise — that a generated getter is a side-effect-free field read — was asserted in prose and nowhere tested. It is now tested.

Scope

Deliberately clear of every open PR: AppLogger.cs (#367), MainWindow.axaml and the binding tests (#338), SdCardFailureClassifier and its tests (#365), and the Dependabot branch (#363) — checked by diffing each. This PR touches two files, neither of which appears in any of them.

---

🤖 Generated with Claude Code

[tylerkron]

Requesting a Qodo round on 073be81. Worth an adversarial look at two things in particular: (1) whether reading the generated [ObservableProperty] getter is genuinely interchangeable with reading its backing field at all five sites, including inside IsVisible's setter where the surrounding SetProperty has already run; and (2) whether the nine new characterisation tests actually constrain TruncatedSerialNo rather than restating it — the positive control (mutating > to >= and dropping the ?? string.Empty) fails exactly two of them and was re-run after the final state of the fix, not only before it.

[tylerkron]

/agentic_review

[qodo-code-review]

PR Summary by Qodo

Eliminate MVVMTK0034 warnings in logged-series legend items

✨ Enhancement 🧪 Tests 🕐 10-20 Minutes

AI Description

• Replace five backing-field reads with generated properties, eliminating all MVVMTK0034 warnings.
• Preserve constructor assignments and visibility behavior without introducing additional change
 notifications.
• Add characterization tests for serial formatting, null handling, and property identity.

Diagram

graph TD
  BF["Observable fields"] --> GP["Generated properties"] --> SF["Serial formatter"] --> LT["Legend text"]
  GP --> VS["Visibility setter"] --> MS["Minimap sync"]
  FT["Formatting tests"] --> SF
  FT --> GP

Loading

High-Level Assessment

Using the generated getters is the narrowest correct resolution because it satisfies MVVMTK0034 while retaining value-identical read behavior. Warning suppression would leave discouraged access in place, while routing constructor initialization through generated setters could introduce change notifications during construction.

Files changed (2) +88 / -8

Refactor (1) +4 / -4

LoggedSeriesLegendItem.csRead observable values through generated properties +4/-4

Read observable values through generated properties

• Replaces all five flagged reads of observable backing fields with DeviceSerialNo and ChannelName property reads. Direct constructor assignments and the hand-written visibility backing field remain unchanged to preserve notification behavior.

Daqifi.Avalonia/Daqifi.Desktop/Loggers/LoggedSeriesLegendItem.cs

Tests (1) +84 / -4

LegendFormattingTests.csCharacterize logged-series serial formatting and generated getters +84/-4

Characterize logged-series serial formatting and generated getters

• Adds nine test cases covering serial truncation boundaries, short and empty values, null fallback behavior, and reference identity for generated serial and channel properties. Extends the formatter test suite documentation to include LoggedSeriesLegendItem.

Daqifi.Avalonia.Tests/Loggers/LegendFormattingTests.cs

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0)

Great, no issues found!

Qodo reviewed your code and found no material issues that require review

Tip of the day

💡 Did you know, you can enable the Remediation agent and Qodo fixes findings in a dedicated fix PR

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Previous reviews

Review updated until commit 0b77c4f 🚀 Fast

Results up to commit 073be81 🚀 Fast

No changes from previous review

Results up to commit 0b77c4f 🚀 Fast

No changes from previous review

[tylerkron]

Pushed 0b77c4f, which adds one test and changes no production code (LoggedSeriesLegendItem.cs is byte-identical to 073be81).

Round 1 on 073be81 came back Bugs (0) / Rule violations (0) / Requirement gaps (0) with no inline threads. Note for the record that that comment rendered no Review mode line, where other PRs in this repo show one (e.g. #365 shows 🚀 Fast) — so I am not able to report the grade for round 1, and I hand-reviewed the delta myself rather than resting on it.

The new commit exists because of that hand-review. The PR's entire behaviour-preservation argument rested on one unasserted premise — that reading a generated [ObservableProperty] getter is a plain field read with no side effect — which was prose, not a test. Reading_a_generated_property_is_a_plain_field_read now pins it, and carries its own control: a WRITE through the generated setter must raise PropertyChanged, or the six silent reads would be proving nothing about a live subscription.

Positive control re-run against this final state: mutating the property to >= 4 and dropping ?? string.Empty fails exactly 2 of the 29 tests in the class and passes 27. dotnet test on the reverted tree: 875/875.

[tylerkron]

/agentic_review

[qodo-code-review]

Code review by qodo was updated up to the latest commit 0b77c4f

[tylerkron]

Shepherd round on 0b77c4f (unchanged head — no new commits since round 2).

Re-derived every claim in the body against the final tree rather than taking it on trust, and all of it holds:

• All five sites are reads. DeviceSerialNo?.Length > 4, DeviceSerialNo[^4..], DeviceSerialNo ?? string.Empty, and the two arguments at LoggedSeriesLegendItem.cs:58. DatabaseLogger.SetMinimapSeriesVisibility(string, string, bool) takes all three by value — no out, no ref, no in, so no site is a write or a by-reference capture.
• The generated getter really is a bare field read here. Checked the emitted source rather than the general rule (-p:EmitCompilerGeneratedFiles=true): ObservablePropertyGenerator/Daqifi.Desktop.Logger.LoggedSeriesLegendItem.g.cs has get => _channelName; and get => _deviceSerialNo;. Every partial hook the generator emitted for this type is On…Changing/On…Changed — all setter-side; there is no getter hook. Only one partial class LoggedSeriesLegendItem declaration exists, so no second part can add one.
• Warning arithmetic closes at both ends, measured independently. Cold local -t:Rebuild of the Desktop head at base and head gives 175 → 170, reconciling as first-party + vendored = MSBuild total (155+20=175, 150+20=170) with the vendored block exactly 13 AVP1002 + 4 CS0108 + 2 CS0169 + 1 CS0618. CI agrees: Desktop 175→170, iOS 176→171, Android 181→176. The set difference is a pure deletion — 5 removed at (37,40) (38,17) (39,11) (58,65) (58,82), all MVVMTK0034, 0 added, no line shift.
  (Both parser traps the body names are real: my first pass returned exactly 2× because the inline copy carries a 2> node prefix the summary copy lacks, and it dropped the un-located AVLN3001, which is formatted … : Avalonia warning AVLN3001: with no (line,col).)
• Mutation control re-run locally on the final 29-test tree, not inherited: >= 4 plus dropping ?? string.Empty fails exactly 2 of 29 — A_serial_of_four_characters_or_fewer_is_shown_whole(serial: "4104") and A_missing_serial_renders_as_an_empty_string. Restored and re-ran to 875/875 (which also proves the restore was not measured against a stale assembly).

Worth an adversarial look at the one thing I cannot fully discharge by inspection: the test block is +118 against a +4/−4 production change. I read it as earning its keep — The_generated_properties_hand_back_exactly_what_the_constructor_stored defeats string interning with new string(char[]) so Assert.Same is meaningful, and Reading_a_generated_property_is_a_plain_field_read carries its own self-control (a write must raise PropertyChanged), which is what stops Assert.Empty(raised) being a test that cannot fail. If any of it still reads as tautological, say so.

[tylerkron]

/agentic_review

[qodo-code-review]

Code review by qodo was updated up to the latest commit 0b77c4f

[tylerkron]

Qodo-clean, CI green — ready for review. Three rounds now, all Bugs (0) / Rule violations (0) / Requirement gaps (0) with reviewThreads.totalCount = 0; all three graded 🚀 Fast and the grade is observable — the "Previous reviews" block lists 073be81, 0b77c4f and this one. My shepherd round on the unchanged head fired rather than being refused (review updated_at 07:01:33Z → 07:16:31Z, past my 07:16:09Z request, summary naming 0b77c4f), and both surfaces were re-checked clean after a >3 min settle against that same SHA. CI green on 0b77c4f, which is the current head — run 35192368199, all six jobs, a pull_request run of Merge 0b77c4f into main created after main’s tip 74308c5 (which is also this PR’s merge base), so the green is not stale.

I re-ran the mutation control locally rather than relying on CI (took build.2, released; build.1 was held by another agent throughout and untouched): on the final 29-test tree, >= 4 + dropping ?? string.Empty fails exactly 2 of 29 — the "4104" boundary and the null case — then restored, touched past the mtime trap, and re-ran to 875/875. I also verified the equivalence premise from the emitted generator output, not the general rule: get => _channelName; / get => _deviceSerialNo;, with every emitted hook On…Changing/On…Changed (setter-side only, no getter hook), and all five call sites confirmed reads passed by value. Warning arithmetic closes independently at both ends — local cold rebuild 175 → 170 reconciling as first-party + vendored, CI 175→170 / 176→171 / 181→176, MVVMTK0034 5 → 0 on every head, set difference a pure deletion with 0 added.

One thing for whoever sequences the merges: #368 has been built with none of #338 / #365 / #367 — the sweep that built 74308c5 ← #338 ← #365 ← #367 did not include it. No file overlap with any of them (nor with the new #370, which adds only new files), it adds no new top-level type and removes/renames no accessible member — but it has never been compiled alongside them, so it is worth a sweep rather than assuming. Not merging.