Repository navigation
fix(#433): a channel is the same colour on the mobile live plot as it is everywhere else — and the third palette goes away - #435
Conversation
On the mobile Stream tab the trace colour IS the channel identifier — LivePlot labels each trace with the channel name and its latest value in that trace's colour. It was taken from a private 8-colour palette indexed by the channel's position in the SELECTED set, so: - the same channel was one colour while streaming and another in the mobile Channels tab and in the logged-session viewer, both of which show the channel's real ChannelColorBrush; - deselecting an earlier channel shifted every later channel's colour, so a channel changed colour between two runs with no colour action taken; - past the 8th channel the index wrapped and two traces shared a colour, on a 16-channel device, while 38 distinct colours were already available. Read the channel's own colour instead and delete the palette. Its comment claiming it held "the desktop channel colors" was false — none of its eight values appears anywhere else in the repo. No new palette and no colour-resolution abstraction: every other surface resolves this with a plain binding or a direct property read, and so does this now. Which palette should be canonical is #430's question and is untouched here — this change uses whatever colour the channel already has. Tests drive the shell's real StreamToggle over the real device wrapper and pin both repros. The shared ShellTestDevice double is lifted out of the watchdog tests rather than copied. Not verified at runtime on mobile: no adb and no device in this environment. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
/agentic_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 reviewTip of the day💡 Did you know, you can add REVIEW.md to your repo root and Qodo follows it on every PR Previous reviewsReview updated until commit 2bacbf3 Results up to commit 2bacbf3
|
PR Summary by QodoUse channel colors consistently in the mobile live plot
AI Description
Diagram
High-Level Assessment
Files changed (4)
|
|
Code review by qodo was updated up to the latest commit 2bacbf3 |
|
/agentic_review |
|
Code review by qodo was updated up to the latest commit 2bacbf3 |
|
Qodo-clean, CI green — ready for review. Head Two clean rounds, both settled, both surfaces checked.
Each round's Disclosure on depth. Both rounds graded CI green on this exact head, all six jobs: Desktop head on macOS, iOS head, Android head, Library + Desktop head + capture harness, CI script self-tests, Avalonia version lockstep. That capture-harness job is worth naming, because it independently closes a claim in the PR body: the body says this change cannot move a captured screen (the mobile sweep poses no device, so Still not verified, and not rounded up: mobile runtime behaviour. No |
|
/agentic_review |
|
Code review by qodo was updated up to the latest commit 2bacbf3 |
|
Shepherd verification — Qodo-clean, CI green, ready for review. Head The fourth file, which the change summary did not mention
Re-proof: the three new tests fail on unchanged
|
| trigger | summary updated_at |
anchored to | result | grade | |
|---|---|---|---|---|---|
| R3 (this one) | 00:01:24Z |
00:03:01Z |
2bacbf3 |
Bugs 0 / Rule violations 0 / Requirement gaps 0 | ⚖️ Balanced |
Timestamp recorded before posting. The advance was not taken as sufficient — the re-rendered summary was read as content and is a review naming this head with finding counts. Settled ≥3 min, then both surfaces re-checked: summary 0/0/0, and GraphQL reviewThreads with a startswith("qodo-code-review") author test gives 0 qodo threads, 0 unresolved (author leg validated against control PR #196 → 3 threads). R1 and R2 both graded 🚀 Fast and R2's own rationale read "No changes from previous review"; this round upgraded to ⚖️ Balanced on the same head and still found nothing, which is a stronger statement than either earlier round made.
One attribution to correct in the ready note above
The baseline capture check is not in the "Library + Desktop head + capture harness" job — that job only compiles AvaloniaCapture as a gate, and its only run step is HeadlessBench --scripted. The run.sh --determinism and --check-captured steps live in "Desktop head on macOS". The substance stands (CI did check the captures on this head, and they passed); only the job name was wrong.
Eleven-way merge — this subsumes the sweep
main + #418 9ad3811 + #419 b3c00d5 + #420 2ea658b + #422 a34c696 + #423 b55a6b6 + #425 081aaa1 + #426 f8966b0 + #428 453323e + #431 deb7bb7 + #432 9302f7a + #435 2bacbf3. Zero conflicts. Eleven-way tree 0a4e5b010eaf34526a2a8a5980ad60d92942ee74.
All six published intermediate trees reproduced byte-identically: 5-way 8599ae34, 6-way fd7c5e1b, 7-way 1369c47c, 8-way 2fb26b57, 9-way a0099579, 10-way 3584ccaa.
Suite on the merged tree: Passed: 1182, Failed: 0, Skipped: 0 — three numbers, whole suite. Reconciled from both ends:
- forward from CI's
main(1116):+5 −3 +24 +0 +0 +3 +10 +4 +20 +0=+63→1179(the ten-way),+3for fix(#433): a channel is the same colour on the mobile live plot as it is everywhere else — and the third palette goes away #435 →1182; - sideways through this PR's standalone total:
1119 − 1116 = +3(a delta, not the total),1179 + 3→1182. ✓
Three heads, all green on the merged tree (Daqifi.Avalonia library is the denominator I'm quoting, since it is the one comparable across heads):
| head | result |
|---|---|
| Desktop, Debug | succeeded, 0 errors — library 130 warnings (whole build 150, incl. 20 from third_party/oxyplot-avalonia) |
iOS, Release iossimulator-arm64 |
succeeded, 0 errors — library 130 + iOS head 1 = 131 |
Android, Debug (AndroidSdkDirectory + EmbedAssembliesIntoApk) |
succeeded, 0 errors — 7 warnings (Android head only; library cached) |
The 135 → 130 drop is fully accounted for, and it is not this PR. De-duplicated on (file, line, col, code), main's five surplus warnings are all CS8618 in ConnectionManager.cs at 237,13 / 1282,29 / 1284,29 / 1286,19 / 1288,19 (CI's macOS log prints each twice — 10 lines, 5 warnings). The only commit in the merge touching that file is #425 081aaa1, the DuplicateDeviceCheckResult → positional-record refactor, which is exactly the shape that removes them. 135 − 5 = 130. #435 itself contributes zero: no warning in any of the three merged-tree logs names MobileShellViewModel.cs (grep validated on the same logs against FirewallManager.cs, which yields 9 — a zero here is a real zero, not a failed search).
Capture gate on the merged tree: 39/39, and 39 is the number it should be. tools/parity-audit/baselines/macos-arm64.sha256 is 39 lines, the harness captured 39 PNGs, and 39 : OK lines came back — no screen moved and no baseline was re-recorded. Machine load was light (1.6) throughout, so this is not a green bought by a quiet machine. Consistent with the body's argument that this change cannot move a captured screen: the mobile sweep poses no device, so LivePlot never enters the visual tree. That argument, not the 39/39, is the evidence for this change — the gate is evidence for the other ten PRs in the tree.
packages.lock.json clean across every project after all restores and builds (git status --short -- '**/packages.lock.json' empty).
Still not verified, and not rounded up
Mobile runtime behaviour. adb does exist here (/opt/homebrew/share/android-commandlinetools/platform-tools/adb) — I ran adb devices rather than assuming, and it lists none. So the Stream tab has still never been rendered with a live stream on it. Three heads compile, the unit tests pin the colour mapping in both directions on the real streaming path, and Qodo has now passed it at Balanced. Nobody has looked at the running plot.
Not merged.
🤖 Generated with Claude Code
Three-way merge:
|
| PR | Ready-note SHA | Head now | CI green on head | CI merge base |
|---|---|---|---|---|
| #435 | 2bacbf3 |
2bacbf3 ✓ |
yes (run 35545404407) | 4716ce1, before #418 #419 #420 #422 #423 #425 #426 #428 #431 #432 |
| #436 | b36c347 |
b36c347 ✓ |
yes (run 35547335403) | 4716ce1, same 10 merges missed |
| #437 | this PR's note above | d50ef98 ✓ |
yes (run 35649810309) | 97be4df = current main |
So the greens for #435 and #436 come from before the last ten merges to main. This merged tree is the build that covers them against current main. It passes on desktop, iOS and tests, but not Android.
Collision audit (what changed across the three PRs):
- Removed or renamed:
- fix(#433): a channel is the same colour on the mobile live plot as it is everywhere else — and the third palette goes away #435 deletes
MobileShellViewModel.Palette, which is private. It also moves the nestedShellTestDeviceandAdoptout ofMobileShellSilentStreamWatchdogTests, into a top-levelinternal Daqifi.Avalonia.Tests.Views.ShellTestDevice.
- fix(#433): a channel is the same colour on the mobile live plot as it is everywhere else — and the third palette goes away #435 deletes
- Converted:
- test(abstraction-improver): use the SD-card importer seam instead of deleting it — 11 tests over the Import All loop #437 changes
DeviceLogsViewModel._messageBoxServicefromstaticto instance. It is private. - test(abstraction-improver): use the SD-card importer seam instead of deleting it — 11 tests over the Import All loop #437 adds a trailing optional parameter to the internal constructor. The only other test caller,
DeviceLogsSelectionTests, passes 3 positional arguments and still binds.
- test(abstraction-improver): use the SD-card importer seam instead of deleting it — 11 tests over the Import All loop #437 changes
- New top-level types:
ShellTestDevice,MobileShellSeriesColorTests,DeleteSessionFailureTests,DeviceLogsImportBatchTests. Each name exists exactly once in the merged tree. - Nested helpers:
RecordingHostappears 4 times,RecordingSessionImporterandRecordingMessageBoxonce each. They are allprivate sealedinside different outer classes, so none of them collide. - None of the three PRs touches a file another one touches.
- No collisions.
🤖 Generated with Claude Code
Sweep: five-way merge of
|
|
Sweep update: the six-way merge of |
Not merging — for review. Closes #433.
What the user sees
On the mobile Stream tab the trace colour is the channel identifier:
LivePlotdraws the legend as "channel name + latest value, in the trace's colour". That colour was taken from a private 8-colour array indexed by the channel's position in the selected set, so:#4FC3F7light blue on the plot; open the Channels tab and its chip is its real colour, e.g.#D32F2FRed700; open the logged session in Storage and it plots red again, because the viewer replays the colour that was stored. Live and replay of the same channel disagreed.The fix
The live series now takes the channel's own
ChannelColorBrush, the value assigned once at construction and shown by every other surface (desktop live-graph legend, desktop Channels pane, mobile Channels tab, and — via the copyDataSamplepersists from it — the mobile session viewer). The private palette is deleted.This is the shape the rest of the app already uses: a plain property read, no converter, no service, no colour-resolution abstraction.
ChannelSeriesstill takes auint,LivePlotstill caches pens by it, and nothing else moved.The comment above that array claimed it held "the desktop channel colors". It did not — its eight values are Material 300 shades and none of them appears anywhere else in the repo. That comment is gone with the array.
Scope: this does not touch #430
#430 asks which of the app's palettes should be canonical — the 38-colour auto-assign set or the 8-colour desktop picker. That is a product decision and it is untouched here. This change does not pick a palette; it makes the live plot use whatever colour the channel already has, which is what every other surface does today. Whichever way #430 goes, this plot follows it for free. The two issues together were "three mutually disjoint channel palettes"; this removes the third and leaves #430's two for #430.
Simplification
Production diff is +28 / −10, but comments dominate: 19 of the 28 added lines are the XML doc on the replacement, and 2 of the 10 removed lines were the false comment. Counting only code, it is +6 / −8 — net −2 lines, and one fewer palette.
Tests are +193 / −40 (two new files, plus the shared device double lifted out of the watchdog tests instead of copied).
Verification
The new tests compile against unchanged
origin/mainand genuinely fail there — they reference no new member, onlySeries,ColorArgbandChannelColorBrush, all of which exist onmain. Run with onlyDaqifi.Avalonia/Views/MobileShellViewModel.csreverted toorigin/main(sourcetouched, project rebuilt, so the assembly is not stale):Those are exactly the issue's Repro A and Repro B, and the wrap.
That first
Expectedvalue is run-dependent, and the test deliberately does not assert it as a constant.ChannelColorManageris a process-wide singleton whose_colorCountrotates over 38 brushes, so which entry AI0 is given depends on how many channels the process created before it. Reproduced independently as a whole-suite run against unchangedorigin/main(rather than a filtered one), the same three tests fail 3/3 with the sameActualvalues, but the firstExpectedreads4278356177(0xFF0288D1, LightBlue700) instead of4292030255. The other two numbers are stable because they are derived inside the test itself.ExpectedArgbreadsChannelColorBrushrather than hard-coding a colour, so the assertion is order-independent even though the number printed on failure is not — expect a different firstExpectedwhen you re-run this. Restoring the fix (sourcetouched again, rebuilt) turns all three green — the mutation control ran in both directions.The tests drive the shell's real
StreamTogglecommand over the real device wrapper and a real Core device, with channels created by Core's own status handling. Nothing about the colour path is faked, andExpectedArgbderives the expected value fromChannelColorBrushitself rather than calling the view-model's helper, so the assertion is not the shell agreeing with itself.Build gate (local, under a build lock):
iossimulator-arm64AndroidSdkDirectory+EmbedAssembliesIntoApk)dotnet test, whole suiteZero warnings name any changed file (positive control:
ConnectionManager.cs, which nothing here touches, contributes 5 in the same log). Warning totals are this worktree's own measurement of this head, not a delta against a quoted baseline.packages.lock.jsonis untouched in all five lock files.What I could NOT verify
adband no device or emulator in this environment, so the Stream tab was never rendered with a live stream on it. The Android build proves the head compiles; it proves nothing about what the screen looks like.mobile-portrait-1-streamandmobile-landscape-1-stream, but it boots with no device posed for the mobile sweep, soIsConnectedis false,ShowDeviceListis true andLivePlotis never in the visual tree. No captured screen can move, so no baseline was re-recorded and the harness was not run for this change.Palettewas private and never bound, andPlot.Series = _viewModel.Seriesis a compile-checked code-behind assignment, not a binding.The evidence for the fix is therefore the unit tests over the real streaming path, in both directions, plus three green heads — not observation of the running app.
🤖 Generated with Claude Code