[tylerkron]

Not merging — for review.

The simplification, in one line

ChannelsPaneViewModel.Rebuild ran two back-to-back foreach (var device in devices) passes over the same list; one loop now does both jobs.

Daqifi.Avalonia/Daqifi.Desktop/ViewModels/ChannelsPaneViewModel.cs   +0 / −3

That is the whole production diff, and it is the same figure with or without comments — no comment was touched, so there is no second number to report for it. The rest of the PR is the test file this routine required before the file could be touched at all: Daqifi.Avalonia.Tests/ViewModels/ChannelsPaneRebuildTests.cs, +376 raw / +229 excluding comments and blanks (147 lines of XML doc). Whole PR: +376/−3 raw, +229/−3 code-only.

So this diff is net-additive overall and I am not going to pretend otherwise: the logic shrank by three lines, and the reason the PR is nonetheless worth its size is the second half — Rebuild had no behavioural coverage at all before this, and it is the method that builds the entire Channels pane.

Why it is simpler, not just shorter

Before:

foreach (var device in devices)
{
    ConnectedDeviceNames.Add(device.Name);
}

foreach (var device in devices)
{
    foreach (var channel in device.DataChannels.OrderBy(...))
    { /* build and shelve the tile */ }
}

Nothing sat between the two loops, and the second pass read nothing the first produced. The tile constructor takes (channel, this, device.Name, HasMultipleDevices) — the device name comes off the device in hand, and HasMultipleDevices is assigned from devices.Count before either loop starts. So the split was accidental, not load-bearing: a reader had to check for a dependency between the passes to find out there wasn't one, and the list was walked twice to no end.

The merged loop states the actual rule directly — for each connected device: chip it, then shelve its channels — which is one sentence instead of two that have to be related by the reader.

Behaviour preservation

This is a behaviour-preserving change, so "tests that fail unfixed" is zero and says nothing. What backs it instead:

1. The tests landed first and passed against unmodified code. Commit 0b4ec35 (tests) precedes f8966b0 (the refactor) in this branch's history, and the 10 tests were green against production code byte-identical to origin/main.

2. Mutation controls on pristine origin/main — 10 mutations, 10 correct kills. Each mutation's anchor text was asserted present before rewriting, so a mutation that silently did nothing could not be misread as a surviving one; the file was restored from a backup and touched after every round so MSBuild never handed the runner a stale assembly.

mutation on origin/main	test killed
HasMultipleDevices assigned after the tile loop — the exact hazard merging the loops creates	Every_tile_labels_its_device_when_more_than_one_is_connected
.OrderBy(c => c.Name, ChannelNameComparer.Instance) dropped	Tiles_run_device_by_device_and_numerically_within_each
tile loop walks the devices in reverse	Tiles_run_device_by_device_and_numerically_within_each
chip loop walks the devices in reverse	The_pane_chips_every_connected_device_in_connection_order
_channelOwners[channel] = device deleted	The_settings_drawer_opens_on_the_device_that_owns_the_channel
PWM no longer shelves a digital channel as an output	Each_channel_shelves_by_type_direction_and_pwm
RecomputeCounts() dropped	The_header_counts_the_tiles_and_the_active_ones_separately
HasConnectedDevice = false	One_device_names_it… + Two_devices_report_multiple…
HasConnectedDevice = true	With_nothing_connected_the_pane_is_empty
HasMultipleDevices = devices.Count > 0	One_device_names_it… + A_single_device_leaves_the_tile_labels_off

The last two exist specifically because two tests assert a bool at its default (HasConnectedDevice == false on an empty pane, ShowDeviceLabel == false on a single device). An assertion on a default value passes when the assignment that should produce it is deleted, so each of those needed a mutation forcing the value the other way before it could be called a guard. All 10 tests are now demonstrably killable.

3. A double loop is a reordering risk, so the tests pin the sequence, not the contents. ConnectedDeviceNames, AnalogInputs, DigitalInputs and DigitalOutputs are all bound as ItemsSource (ChannelsPanePrototype.axaml, ChannelsMobileView.axaml), so their order is what the user reads. Tiles_run_device_by_device_and_numerically_within_each asserts the full device/channel sequence across two devices, and feeds both devices their channels in the byte-wise order (AI0, AI10, AI2) so a pane that stopped sorting would produce exactly the wrong sequence rather than accidentally the right one.

4. The rendered result is byte-identical. tools/parity-audit/run.sh --check-baseline → 39/39 screens matching the committed baselines/macos-arm64.sha256, which itself holds 39 lines. That includes all 9 desktop-min-* screens and all four that render this view-model directly (desktop-3-channels, desktop-min-3-channels, mobile-landscape-2-channels, mobile-portrait-2-channels).

5. Build + suite. Desktop build, iOS Release/iossimulator-arm64 build, and dotnet test all exit 0; suite 1126/1126 (1116 on main + the 10 added here). packages.lock.json untouched. .github/scripts/check_test_sqlite_pooling.py --glob passes over the new file.

Scope notes

• No view-model member was renamed or removed, so no .axaml binding is affected.
• No repo-wide guard or self-test was added, so nothing here puts a later-merging PR at risk in either order.
• No file touched by any other open PR (refactor(layering): the connect dialog resolved host names itself and hard-coded DAQiFi's wire ports #418, fix(#417): delete PauseForFlashAsync — an unreachable API describing a hand-off that would lock the flasher out #419, refactor(abstraction-improver): delete the firewall bootstrap's three unused injection seams and its duplicate elevation check #420, refactor(dead-code): delete four unreachable members — 38 lines out, 0 in #422, test(#421): three rows that reported Passed without running now report Skipped #423, refactor(logic-simplifier): the duplicate-device check answers with the match, not a flag beside it #425) is touched here.

Handed forward, deliberately not folded in

The orphaned doc block in DeviceLogsViewModel (lines 342–361) is still there and I left it alone: it belongs to a different view-model and a different feature, and pairing it with this would make a PR whose title cannot state one thing. One correction for whoever picks it up — it is a MOVE, not a deletion. The block accurately documents RaiseSdGateChanged (declared at line 437 with no docs of its own) and records a real upstream limitation (daqifi-core#394: a silent TCP death leaves IsConnected stale). It has merely drifted to sit above WatchSelectedDeviceConnectivity's own doc block. Deleting it would destroy accurate information.

🤖 Generated with Claude Code

[tylerkron]

Round 1. Head f8966b0. Production diff is +0/−3 in ChannelsPaneViewModel.Rebuild (two consecutive passes over the same device list merged into one); the rest is the guarding test file this routine required, with the mutation controls listed in the PR body.

[tylerkron]

/agentic_review

[qodo-code-review]

PR Summary by Qodo

Simplify Channels pane rebuild into a single device pass

✨ Enhancement 🧪 Tests 🕐 20-40 Minutes

AI Description

• Consolidates device chips and channel tile rebuilding into one device-list pass.
• Adds ten behavioral tests covering ordering, shelving, counts, labels, and ownership.

Diagram

graph TD
  CM["Connection Manager"] --> RB["Pane Rebuild"] --> DP["Single Device Pass"] --> CS["Sorted Channels"] --> PC["Pane Collections"] --> FS["Counts and Drawer"]

Loading

High-Level Assessment

The PR's single-pass approach is proportionate and preferable: the two former loops had no dependency requiring separation, so combining them removes redundant traversal while preserving ordering. Retaining separate passes or extracting a new projection abstraction would add indirection or scope without improving behavior, and the focused regression tests adequately protect the refactor.

Files changed (2) +376 / -3

Refactor (1) +0 / -3

ChannelsPaneViewModel.csRebuild device chips and channel tiles in one pass +0/-3

Rebuild device chips and channel tiles in one pass

• Merges two consecutive iterations over connected devices so each device name is added immediately before its sorted channels are processed. Existing pane ordering, ownership mapping, shelving, counts, and settings synchronization remain unchanged.

Daqifi.Avalonia/Daqifi.Desktop/ViewModels/ChannelsPaneViewModel.cs

Other (1) +376 / -0

ChannelsPaneRebuildTests.csAdd behavioral coverage for Channels pane rebuilding +376/-0

Add behavioral coverage for Channels pane rebuilding

• Adds ten tests that pin empty, single-device, and multi-device pane output; channel shelving and ordering; active and total counts; tile labels; and channel ownership resolution. Introduces lifecycle-safe helpers and a configurable channel test double while serializing access to the process-wide connection manager.

Daqifi.Avalonia.Tests/ViewModels/ChannelsPaneRebuildTests.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 turn on the rule miner and Qodo learns your standards from review history

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Previous reviews

Review updated until commit f8966b0 🚀 Fast

Results up to commit f8966b0 🚀 Fast

No changes from previous review

[tylerkron]

/agentic_review

[qodo-code-review]

Code review by qodo was updated up to the latest commit f8966b0

[tylerkron]

Shepherd round: firing an independent Qodo round on the unchanged head f8966b0. The two clean rounds on record are the builder's own; on this loop an independent unchanged-head round has raised a real finding on three of the six PRs it has been run on, so it is worth the round even with nothing changed. Both existing rounds are graded 🚀 Fast (read off the folded "Previous reviews" section, not the visible summary).

Also re-deriving the claims independently: the mutation controls, the reordering risk in collapsing the double foreach, and the seven-way merge build with #418/#419/#420/#422/#423/#425.

[tylerkron]

/agentic_review

[qodo-code-review]

Code review by qodo was updated up to the latest commit f8966b0

[tylerkron]

Qodo-clean, CI green — ready for review.

Head f8966b0073595079ac44b89d872f364c32ebe9ea — this is the current head and the exact head_sha of CI run 35533903659, conclusion success. I pushed nothing, so the verification behind this note is CI's on f8966b0, read from the API — not a local re-build of this PR.

Independent Qodo round (round 3, on the unchanged head). Trigger timestamp recorded before posting (20:10:55Z); summary updated_at advanced 20:01:14Z → 20:11:19Z, past it. I read the content, not just the timestamp: Bugs (0) / Rule violations (0) / Requirement gaps (0), no placeholder, and no review skipped string. Grade 🚀 Fast, read off the folded "Previous reviews" section. Settled 13.6 min and re-checked both surfaces: summary unchanged, and GraphQL reviewThreads gives 0 qodo threads / 0 unresolved (author leg validated against control PR #196 → 3 threads ignoring isResolved). One honest caveat: on an unchanged head the summary body is byte-identical to its pre-trigger state, so "it fired" rests on the updated_at advance plus Qodo's fresh commit notice, not on new text.

Re-proof of the claims. Every figure in the body re-derives: +0/−3 production; +376 raw / 229 code-only with 376−229 = 147 doc lines; suite 1126 on main+this (delta +10, not 1126 — main measures 1116 here); --check-baseline 39/39 where I read the manifest's own line count (39, 9 of them desktop-min-*) and counted 39 rendered PNGs with all 9 desktop-min-* present, so the denominator is the real one.

• Ordering. The two loops wrote to disjoint collections and HasMultipleDevices/HasConnectedDevice/DeviceName are all assigned before either one, so per-collection sequence is unchanged — only notification interleaving differs. Downstream is safe: both views bind these as plain ItemsSource with no comparer, no SortDescription, no index dependency, and there is no CollectionChanged subscriber on any of them anywhere in the repo.
• Mutations. Re-ran 3 of the 10 myself (backup-restore + touch after each, so no stale assembly): reversing device iteration killed 2 tests; dropping the OrderBy killed 1; forcing the tile ctor's HasMultipleDevices argument to false killed exactly Every_tile_labels_its_device_when_more_than_one_is_connected.
• The bool-at-default fix is real, and that third mutation is the proof. ShowDeviceLabel is get-only and set only in the ctor, so false is its default; the single-device test asserting False is half a test on its own, but the two-device test asserting True forces it away from the default — and that is the one the mutation kills.

Seven-way merge (main + #418 #419 #420 #422 #423 #425 #426): tree 1369c47cadab7bd1b63035a108f8ad85fb0b99b2, reproduced three ways (octopus, ascending, descending) — all agree, no conflicts in any order. Intermediates match the earlier sweep exactly: five-way 8599ae34, six-way fd7c5e1b. Desktop + iOS Release/iossimulator-arm64 build clean, packages.lock.json untouched, suite 1155 / 0 failed / 0 skipped, and --check-baseline 39/39 on the merged tree.

Counts reconcile against raw per-PR deltas I measured individually (main + each PR alone), signs preserved:

	suite	delta
main 4716ce1	1116	—
+#418	1121	+5
+#419	1113	−3
+#420	1140	+24
+#422	1116	+0
+#423	1116	+0
+#425	1119	+3
+#426	1126	+10

1116 +5 −3 +24 +0 +0 +3 +10 = 1155, matching the measured seven-way total. Quoting Passed; on macOS Skipped is 0 at every step, so Passed == Total and #423's Passed→Skipped conversion is a Windows-side effect invisible here — which is why its delta is +0 rather than negative.

Removal hazards (the #189 CS0115 shape) cleared: greped the merged tree across .cs and .axaml for every member #419/#422/#425 delete — PauseForFlashAsync, SendScpiMessage, ImportFromStreamAsync, EnsureAnyDeviceConnected, SwitchToNewInterface, IsDuplicate — and the only hit is a prose mention of PauseForFlashAsync inside an XML doc comment documenting its own removal. Searched the type name DuplicateDeviceCheckResult too, since target-typed new(...) hides a constructor. Duplicate added types are ruled out by the merged tree compiling clean.

Only #418 branched before current main (base ef7d18c); the other six share base 4716ce1.

Not merging.