Repository navigation
fix(logged-data): the empty plot region is a black void — the axes and the card were both there, both gated off - #259
Conversation
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ndering as a void Closes #251. Re-records desktop-2-loggeddata.png only. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
/agentic_review |
PR Summary by QodoShow Logged Data plot frame and guidance without a selected session
AI Description
Diagram
High-Level Assessment
Files changed (4)
|
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
…ssible action Qodo round 1: with no sessions the card's primary line still read "Select a session to view data", directly above a list reporting NO SESSIONS. Now "No session selected" — descriptive, true in both states, and the same naming/instructing split LiveGraphPane's card uses. Re-records desktop-2-loggeddata.png. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
/agentic_review |
|
Code review by qodo was updated up to the latest commit 7a6043f |
Shepherd verification — head
|
| merge | result |
|---|---|
| #259 × #256 (both orders) | clean — identical tree either way |
| #259 × #258 (both orders) | clean |
| #256 × #258 | CONFLICT — tools/parity-audit/README.md, one hunk |
| #255 + #256 + #258 + #259, two orders | clean once that is resolved, and bit-identical tree in both orders |
The conflict, precisely. Both #256 and #258 insert their re-recording paragraph immediately after the baselines/<os>-<arch>.sha256 records the SHA-256 of all N Avalonia screens… paragraph, i.e. at the top of the Baselines list, before the desktop-9-summary-flyout.png entry. Two adds at one anchor.
Resolution: keep both paragraphs. They describe different recordings (#256 adds dialog-firmware-uploading.png; #258 re-records desktop-7-notifications-flyout.png), neither supersedes the other, and their relative order is a style choice — the tree is bit-identical either way once both are present. Nothing else in that file conflicts.
#259 is clear of both. Its paragraph sits one entry lower, below the #228 entry, which is far enough from the shared anchor that git's context windows do not overlap. The deliberate placement claimed in the PR body is real and it works — but note it was aimed at #258 only; it happens to clear #256 as well.
#256 also carries the 24 → 25 edits at lines ~102 and ~370 of that README, and #256 and #258 both edit DaqifiViewModel.cs. Neither is a conflict; both auto-merge.
2. The three baseline mutations really do coexist — proved by running the gate, not by reading file names
On the five-way merged tree (main + #255 + #256 + #258 + #259):
baselines/macos-arm64.sha256auto-merges to exactly the right 25 lines — line 2 rewritten (fix(logged-data): the empty plot region is a black void — the axes and the card were both there, both gated off #259), line 7 rewritten (fix(notifications): the flyout names its empty state instead of rendering a blank panel #258), line 16 inserted (fix(firmware): a running upload can be cancelled, and it says what it is doing #256).- Desktop head: succeeded. iOS restore + Release
iossimulator-arm64: succeeded. Nopackages.lock.jsonchurn. dotnet test: 578 passed, 0 failed (mainis 530; this PR alone is 535).run.sh --determinism: 25/25 screens byte-identical across 5 runs,all 25 expected screens captured.run.sh --check-capturedagainst the merged manifest: all 25 OK, including all three mutated entries.
No #201 edge-column flake appeared on desktop-7/-8/-9.
3. The state the PR body says is not photographed — now photographed
The body correctly discloses that only the zero-session state is captured, and that "sessions exist, none selected" is reasoned from the code. I rendered it, via a temporary extra screen in AvaloniaCapture (reverted; nothing committed), on main and on this branch.
On main (b924261) — the card was already visible here (HasLoggingSessions is true), but the plot region is still a void, and the primary line asks for something the user has already been given:
On this branch — labelled Analog (V) / Digital / Time (ms) axes with gridlines, the card reading No session selected, and the Choose a session from the list below second line correctly present because sessions exist:
This is the picture that makes the double-gate story concrete: the card's second condition is why a fresh install got nothing, and the PlotView's gate is why neither state ever had axes.
Mid-load is covered by something else and is safe. LoggingSessionListViewModel.DisplaySessionAsync sets IsLoggedDataBusy = true for the whole load, and LoggedDataPanePrototype.axaml puts a full-pane #CC000000 scrim + progress bar at ZIndex="20" over everything while that flag is set. So the card is not what the user sees while a session loads, before or after this change.
Subtraction did not make a hidden state reachable. The other three HasSessionData gates in that file — the session header strip (which also carries the plot toolbar), the minimap, and the legend panel — are untouched and still collapse with no session. Only the main PlotView was un-gated.
4. Every number in the body re-derived
| claim | re-derived |
|---|---|
| net −5 markup lines, ignoring comments | ✅ +4 / −9 non-comment lines in the .axaml (total diff is +27/−14; comments are +18) |
| 530 → 535 tests | ✅ main 530, head 535, 0 failed — 5 cases in 3 methods |
| pre-fix capture matched the committed manifest | ✅ my own pre-fix desktop-2-loggeddata.png hashes to c893092e…, which is exactly what main has committed — the void is what ships |
| the new hash | ✅ my own post-fix capture hashes to b8d9c8d4…, exactly the value this PR records |
| blast radius 1 of 24 | ✅ pre-fix vs post-fix captures differ on desktop-2-loggeddata.png and nothing else |
| desktop 191 warnings | ✅ 191 all-project MSBuild total, on a full -t:Rebuild |
| first-party 171/171, 0 added | ✅ 171 first-party each side against my own merge-base b924261, compared as a set: 0 added, 0 removed. The 20-warning gap is third_party OxyPlot |
Two body claims were wrong and I corrected them in the body text only — no commit, so CI's green and Qodo's settled review still cover 7a6043f8:
- "No source file is touched by more than one of the three" — false; fix(firmware): a running upload can be cancelled, and it says what it is doing #256 and fix(notifications): the flyout names its empty state instead of rendering a blank panel #258 both touch
DaqifiViewModel.cs. This PR is disjoint from both, which is the claim that was meant. - "All seven CI checks pass" — six pass;
Claimed bumps are actually appliedis skipped, which is not a pass.
5. One pre-existing nit, not a regression, not worth a commit here
A session that loads with zero samples (initial.IsEmpty in DatabaseLogger.DisplayLoggingSession) leaves HasSessionData false, so the card shows No session selected while a session is selected. main was equally wrong in that state (Select a session to view data), the card was already visible there, and this PR changes neither its visibility nor its reachability — so it is pre-existing. Filed separately rather than pushed into a settled review.
6. Status
- Qodo: 2 rounds. Round 2's
Code Review by Qodocomment says in its own words that it was "updated up to the latest commit7a6043f8", and reportsBugs (0) / Rule violations (0) / Skill insights (0). GraphQLreviewThreadsshows 1 qodo thread, resolved, 0 unresolved (author matched bystartswith("qodo-code-review")— the GraphQL login has no[bot]suffix; validated against PR fix(logging): a half-finished Delete All left a table-less database, and the next logging toggle killed the app #196, which yields 3 qodo threads and 0 unresolved as expected). - CI: green on
7a6043f8itself, not an inherited green — run33995300439, all six real jobssuccessagainsthead_sha 7a6043f8. - Not covered: the Android head was not built here (workload absent on this Mac) — CI's
Android headjob covers it. No bench/device validation; an empty-state render is not device-observable.
|
Qodo-clean, CI green — ready for review at Uncovered: the Android head was not built here (workload absent on this Mac); no device/bench validation, since an empty-state render is not device-observable; and there is no automated assertion on the XAML itself, because these views carry no |
Closes #251. Not merging — for review.
On a fresh install the Logged Data pane's upper ~57% rendered as a featureless black
rectangle: no axes, no gridlines, no copy, while the list below it correctly said
NO SESSIONS. Live Graph handles the identical "nothing to plot yet" state well, drawinglabelled axes plus a small card. This makes Logged Data do the same thing — by deleting the
two conditions that were suppressing the empty state it already had.
The cause was narrower than "nobody built one"
The pane already had both halves of a proper empty state. Each was gated off in exactly
this case:
mainoxy:PlotView(main plot)IsVisible="{Binding DbLogger.HasSessionData}"MultiBinding:!HasSessionDataANDHasLoggingSessionsThe second condition is the interesting one: the card was shown only when sessions exist but
none is picked, so it was silenced in the one state that needed it most.
DatabaseLogger's constructor already assignsPlotModel = _plotModelFactory.CreateMainPlotModel()unconditionally, and that model isbuilt with the same three titled axes Live Graph shows —
Analog (V),Digital,Time (ms). Nothing adds a series untilDisplayLoggingSessionruns. So the axes werethere the whole time; the view was just hiding them.
The fix is subtraction
Delete the
PlotView's gate, and delete the second condition from the card's. Thatsatisfies both remedies the issue offers at once, and leaves the pane structurally
identical to
LiveGraphPane.axaml's empty state: plot always drawn, card overlaid.Ignoring comments, the markup is net −5 lines — a 7-line
StackPanel.IsVisibleMultiBinding block and one
IsVisibleattribute removed, two plainIsVisibleattributesadded, and one
Textstring swapped 1-for-1. No new style, converter, resource or panellevel. Two converter references (
BoolAndToVis,BooleanToInverse) drop out of this file;both stay registered and in use by
LiveGraphPane.axaml, so nothing is orphaned.The card's copy, which took two passes to get right
Making the card visible in the zero-session state exposed a wording problem it never had
before, and the fix is now split the way
LiveGraphPane's card is — a line that namesthe state and a line that instructs:
No session selectedChoose a session from the list belowHasLoggingSessionsonlyThe secondary line is gated because with no sessions it would point at an empty list and
repeat guidance the pane already gives 300px lower. The primary line was
Select a session to view datauntil Qodo round 1 correctly pointed out that this still requested animpossible action on a fresh install — conceded and changed.
HasLoggingSessionsthereforesurvives as a gate, but on the one element that actually depends on a non-empty list rather
than on the whole card.
Evidence
A layout/empty-state fix is not meaningfully unit-testable, so the claim rests on these
images, not on an assertion. Saying that plainly rather than rounding it up to "verified".
Before —
origin/main@b924261, unmodified.--check-baselinematched the committedmanifest on all 24 screens, so this is what ships today, not a harness artifact:
After — the same capture site on this branch:
--check-capturedagainst the manifest with onlydesktop-2-loggeddata.pngupdated reports the other 23 OK — byte-for-byte identical towhat
mainhas recorded.--determinismran 5 passes,24/24 screens byte-identical across 5 runs.No edge-column delta appeared. (Re-run from scratch after the round-1 copy change.)
desktop-2-loggeddata.png→b8d9c8d4ca0af2b4fbc2a270bded2beda5f350383d3731b34fbe50afa8b812e3intools/parity-audit/baselines/macos-arm64.sha256. The updated manifest then verified cleanover
determinism/r1, which is the same chain CI runs. Harness README updated with there-recording note the Baselines section expects.
Desktop head on macOSis green on this PR, and thatjob runs its own five-pass
--determinismon themacos-latestrunner and then verifiesdeterminism/r1against this manifest — so the new hash reproduces on a second machine, notonly on the Mac that recorded it. (Not added to the README note: the review had settled by
the time CI confirmed it, and this is not worth another round-trip. Worth folding in if the
file is touched again.)
What this evidence does NOT cover
The committed harness only renders the zero-session state. The "sessions exist but none
selected" state also changes — it gains the empty axes behind the card, and its primary line
is reworded — and is not part of the committed capture set; in this PR's own evidence that
behaviour is reasoned from the code, not photographed. There is no automated assertion that
the pane renders correctly, because views here carry no
x:DataTypeand their bindingsresolve by reflection at run time.
Update: the shepherd has since photographed that state on
mainand on this branch, usinga temporary extra screen in
AvaloniaCapturethat was reverted rather than committed — seethe shepherd verification comment.
It renders as reasoned. The same comment covers the mid-load state, which turns out to be
hidden behind the pane's existing
IsLoggedDataBusyscrim in both the old and the newbehaviour.
Tests
5 new cases in 3 methods (
EmptyLoggedPlotFrameTests); suite goes 530 → 535, 0 failures.They pin the one genuinely new assumption this fix introduces: that the model handed to the
now-always-visible
PlotViewis a complete labelled empty frame — three titled axes at theright positions, gridlines on analog and time and deliberately none on digital, zero series.
If those axes were ever moved out of construction and built on first data instead, the pane
would silently revert to the void this issue is about, both heads would still build green,
and nothing else would notice.
Scoped deliberately to
PlotModelFactory, which is pure by design — no app host, no DB, noNullDialogService— so it cannot collide with #250/#258's test-file refactor.I did not add view-model tests, because unlike #258 this diff adds no view-model surface at
all. Both members it binds were already bound by this same view before the change, and both
were verified to exist and change-notify:
DbLogger.HasSessionDatais[ObservableProperty],and
HasLoggingSessionsis re-raised byNotifyLoggingSessionsChanged.Coordination with the other two baseline-mutating PRs
Three open PRs move the capture baseline at once. Checked with
gh pr diff <N> --name-only:DevicesPanePrototype.axaml,FirmwareDialog.axaml,DaqifiViewModel.cs,AvaloniaCapture/Program.csNotificationsFlyout.axaml,DaqifiViewModel.cs, 2 test filesdesktop-7-notifications-flyout.pngLoggedDataPanePrototype.axaml, 1 new test filedesktop-2-loggeddata.pngThis PR is disjoint from both. No source file it touches is touched by either of the other
two. (Correction to an earlier, stronger version of this claim: the three are not pairwise
disjoint — #256 and #258 both edit
DaqifiViewModel.cs. They auto-merge there, but that istheir overlap, not this PR's.) All three edit
baselines/macos-arm64.sha256, on differentlines (2, 7, and #256's insertion at 16), which merges cleanly. I did not touch the gate's
expected=count — that is #256's to own.The one conflict surface is
tools/parity-audit/README.md. #256 and #258 both insert theirre-recording paragraph immediately after the
baselines/<os>-<arch>.sha256 records …paragraph — the top of the Baselines list — so those two collide with each other there. This
PR's paragraph sits one entry lower, below the #228 entry, so it lands in a different hunk from
both; each paragraph self-dates, and the README says the placement was deliberate.
Shepherd-verified by actually merging.
MERGEABLEis computed pairwise againstmainandcan never see a PR-to-PR conflict, so all three read clean while two of them collide:
tools/parity-audit/README.md, one hunk. Resolution: keep both paragraphs (their order is a style choice)On that five-way tree: desktop head green, iOS Release
iossimulator-arm64green, 578 testspass, 0 fail, the manifest auto-merges to 25 correct lines,
--determinismis 25/25 across5 runs, and
--check-capturedagainst the merged manifest reports all 25 screens OK — sothe three baseline mutations genuinely coexist.
Build gate
Re-run in full after the round-1 copy change:
iossimulator-arm64dotnet testpackages.lock.jsonchurnWarnings measured against my own merge-base, not a quoted constant:
origin/main@b924261and this branch both report 191 on the desktop head — an all-project MSBuildtotal, 20 of which are
third_party— and the first-party warning set is identical: 171on each side, 0 added, 0 removed (compared as a set, not just a count). The delta was
measured on the desktop head, which is the build that compiles the changed
.axaml; the iOShead built green but its total was not measured cleanly (incremental).
The Android head does not build on this Mac (workload absent) — I left that to CI rather than
claiming it, and CI's
Android headjob is green.CI at
7a6043f(run33995300439): six jobs green — Library + Desktop head + captureharness, Desktop head on macOS, Android head, iOS head, Avalonia version lockstep, CI script
self-tests. The seventh,
Claimed bumps are actually applied, is skipped, not passed — itdoes not apply to this diff.
Qodo
Two rounds. Round 1 raised one Medium correctness finding (the impossible zero-session
instruction) — conceded and fixed, replied and resolved. Round 2 is clean on both surfaces at
this head SHA:
Bugs (0) / Rule violations (0) / Skill insights (0), 0 unresolved qodothreads, re-checked after a settle wait.
No bench validation: an empty-state rendering fix with no device attached is not
device-observable, so no board lock was taken.