[tylerkron]

Closes #410. Not merging — for review.

What was wrong

In Logged Data → DEVICE LOGS, with two or more devices connected, a connect or disconnect of any other device moved the DEVICE combo back to the first device. That device's SD card was listed in place of the one the user picked, and IMPORT ALL then targeted it. Nothing told the user the selection had moved. And with a single device selected, every other device connecting still sent that device a fresh SD listing.

The cause was DeviceLogsViewModel.UpdateConnectedDevices. It rebuilt ConnectedDevices with Clear() followed by re-adding every device. When the combo's items emptied, its two-way SelectedItem binding wrote null into SelectedDevice. The SelectedDevice == null fallback then picked First(), and each of those two writes ran OnSelectedDeviceChanged, which started an SD listing.

The fix

UpdateConnectedDevices now updates the list in place instead of rebuilding it. It removes only the devices that left and appends only the ones that arrived. A selected device that is still connected never leaves the combo's items, so nothing writes to the selection and no SD listing is sent.

When the selected device itself leaves, the pane falls back to the first device still connected and lists that device's card, as before. If no device is left, the selection is cleared. The view-model now makes that check itself (is the selection still in the list?) instead of only checking for null, so it no longer depends on the combo to clear the selection first.

Mobile shares this code. DeviceLogsMobileView.axaml binds the same DeviceLogsViewModel with the same two-way SelectedItem="{Binding SelectedDevice}" (it is created in StorageMobileView.OnDeviceLogs), so this one change fixes both views.

Considered and left alone: equality. The update uses List.Contains, which follows Equals, and the WiFi DaqifiStreamingDevice overrides Equals to compare Name, IP and MAC. The pane's list and the registry could only disagree about which of two value-equal instances is present if the registry swapped one for the other without a notification in between. That cannot happen: RegisterConnectedDevice refuses a value-equal duplicate, and every registry change raises "ConnectedDevices" synchronously, so a reconnect is always a removal update followed by an arrival update. ConnectionManager itself uses the same value equality.

Also a test seam: the existing internal test constructor takes an optional UI-thread marshal (Action<Action>). This is the same pattern DatabaseLogger and ConnectionDialogViewModel already use. Outside a running app, Dispatcher.UIThread binds to the first thread that touches it and is never pumped, so the constructor's own update would hang any test running on another thread. Production passes null and keeps the Dispatcher.UIThread path.

The UI is unchanged; this is a behaviour fix, so the tests are the evidence. The issue's screenshots show the "before".

Verification

DeviceLogsSelectionTests has 9 new tests. They register real notifications through ConnectionManager.Instance and use RecordingStreamingDevice, which now counts SD listings. This test project builds no Avalonia controls, so a small stand-in plays the combo: when the selected item is no longer in the items, it writes null into SelectedDevice. That is the behaviour the issue recorded against the real control ([null, AAAA0001]).

7 of 9 fail against the old update logic. I ran them twice, with the old Clear()+re-add body restored and the seam kept so the tests could run:

• selection kept when another device connects → old: AAAA0001
• selection kept when another device drops → old: AAAA0001 (the test re-picks B after the connect, so it measures the drop alone)
• no SD listing to any device when another connects → old: A listed again (1 → 2)
• the first device's card is not re-listed when a second device connects → old: 1 → 2
• the selection is never written → old: [null, "AAAA0001"], which is exactly the issue's trace
• fallback with no view attached → old: kept the departed BBBB0002
• the last device leaving clears the selection → old: kept the departed AAAA0001 (this only fails when no view is attached; with a combo, the old code also ended at null)

The 2 that pass on the old code are guards on behaviour that must not change: the fallback to the first remaining device with the combo attached (including its one SD listing), and the list following connects and drops in order.

• Build gate: Desktop ✅, iOS restore ✅, iOS Release iossimulator-arm64 --no-restore ✅. Android is not built on this Mac; CI covers it.
• dotnet test: 1066 passed, 0 failed (locally).
• Warnings, from CI's own "Library + Desktop head" logs at both ends (merge-base 858d143 run 35463500927 vs this head ed04818 run 35468884235): 148 distinct first-party warnings on both sides, 166 all-project on both. Ignoring line numbers, 0 were added and 0 removed. The only three entries that differ are the existing DeviceLogsViewModel constructor CS8618s (_busyMessage, _selectedDevice, _deviceFiles), which moved down by the seam's lines.
• CI is green on every job (Library+Desktop, Desktop macOS, iOS, Android, lockstep) on ed04818.
• No board attached, so no bench run. The change does not touch the device protocol; it stops unrequested RefreshSdCardFiles calls.

Diffstat

Production code is DeviceLogsViewModel.cs +44/−10; most of the added lines are comments, plus the seam. Tests: DeviceLogsSelectionTests.cs +324 (new) and RecordingStreamingDevice.cs +9/−1. The diff is net-additive because it is a bug fix with its regression tests.

🤖 Generated with Claude Code

[tylerkron]

/agentic_review

[qodo-code-review]

PR Summary by Qodo

Preserve Device Logs selection across connection changes

🐞 Bug fix 🧪 Tests 🕐 20-40 Minutes

AI Description

• Reconciles connected devices in place, preserving selections during unrelated connection changes.
• Falls back only when the selected device disconnects, preventing redundant SD listings.
• Adds regression coverage for selection, fallback, ordering, and listing behavior.

Diagram

graph TD
  CM["Connection Manager"] --> UI["UI marshal"] --> REC["In-place reconcile"] --> VALID{"Selection retained?"}
  VALID -- Yes --> KEEP["Keep selection"] --> NONE["No SD listing"]
  VALID -- No --> FALL["Select fallback"] --> LIST["List SD card"]

Loading

High-Level Assessment

In-place reconciliation is the appropriate approach because it preserves the selected item’s collection membership and prevents two-way binding from generating transient selection changes. Saving and restoring the selection around Clear() or replacing the collection were considered, but both retain unnecessary UI churn and risk triggering selection side effects and redundant SD listings.

Files changed (3) +377 / -11

Bug fix (1) +44 / -10

DeviceLogsViewModel.csPreserve selected device during connection changes +44/-10

Preserve selected device during connection changes

• Reconciles ConnectedDevices in place instead of clearing and rebuilding it, preserving valid selections and preventing redundant SD refreshes. Invalid selections now fall back explicitly, and an injectable UI-thread marshal enables deterministic headless tests while production retains Avalonia dispatching.

Daqifi.Avalonia/Daqifi.Desktop/ViewModels/DeviceLogsViewModel.cs

Tests (2) +333 / -1

RecordingStreamingDevice.csTrack SD card listing requests safely +9/-1

Track SD card listing requests safely

• Adds a thread-safe counter for SD card refresh requests, allowing tests to verify that connection changes do not trigger redundant listings.

Daqifi.Avalonia.Tests/Device/RecordingStreamingDevice.cs

DeviceLogsSelectionTests.csCover Device Logs selection reconciliation +324/-0

Cover Device Logs selection reconciliation

• Adds nine regression tests covering unrelated connects and drops, selected-device fallback, final-device removal, collection ordering, selection writes, and SD listing counts. The tests emulate ComboBox selection behavior while exercising real ConnectionManager notifications.

Daqifi.Avalonia.Tests/ViewModels/DeviceLogsSelectionTests.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 describe a rule in plain language on the Rules page and Qodo drafts it for you

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Previous reviews

Review updated until commit ed04818 ⚖️ Balanced

Results up to commit ed04818 ⚖️ Balanced

No changes from previous review

[tylerkron]

/agentic_review

[qodo-code-review]

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

[tylerkron]

Qodo-clean, CI green — ready for review. Two graded rounds on head ed04818 (the second graded Balanced), both 0 findings and 0 threads, each re-checked after a 3+ minute settle. CI green on every job on ed04818. Not merging.

[tylerkron]

7-way sweep — main 858d143 + #405 + #406 + #407 + #408 + #411 + #413 + #414 built together (2026-09-19 21:20Z).

• Merge: clean, no conflicts, in two orders — ascending octopus (405→414) and descending sequential (414→405). Both produce the identical tree fb1188996398ab979f2db0720dbac1edb0cc29c5. The only textual overlap, Daqifi.Avalonia.Tests/Services/SerialPortPollingDeviceWatcherTests.cs between test(coverage-gap): release the held read on every path of the Stop/Start race test (follow-up to #404, Qodo Medium) #407 and test: the last two private IAppLogger fakes fold into RecordingAppLogger — one logger double in the suite (−44) #408, auto-merged with no markers in both directions.
• Gate, all under one build.1 lock and judged by process exit code (not a Build succeeded grep, which -tl:on breaks): desktop 0; iOS restore 0; iOS Release iossimulator-arm64 --no-restore 0 (167 warnings); tools/system-test/HeadlessBench restore 0 and Release build 0 — re-run with --no-incremental after the first pass was a no-op, 167 warnings on the real compile.
• Tests: 1096 passed / 0 failed, and the count reconciles exactly against main's 1057: +19 test(#388): the dialog view-model guard requires the initializer to END at the construction — new VM(...).ToString() no longer passes #405 (2 [Theory], 6+13 [InlineData]), +0 fix(#397): Devices, Channels and Profiles panes take their view-model on attach, not Loaded — no more frame with the drawer forced open #406, +0 test(coverage-gap): release the held read on every path of the Stop/Start race test (follow-up to #404, Qodo Medium) #407, +0 test: the last two private IAppLogger fakes fold into RecordingAppLogger — one logger double in the suite (−44) #408 (fold is net zero), +3 test(#409): the flash probe's cancellation token and the no-document warning are now pinned through app code, not only Core #411, +8 fix(#412): one NaN reading no longer fails an SD import — dropped and reported, as the live path does (#294) #413, +9 fix(#410): Device Logs keeps the device you picked when another one connects or drops, and stops re-listing its SD card #414. The reported "+8"/"+9" for fix(#412): one NaN reading no longer fails an SD import — dropped and reported, as the live path does (#294) #413/fix(#410): Device Logs keeps the device you picked when another one connects or drops, and stops re-listing its SD card #414 were re-derived from the diffs, not taken on trust.
• Per-PR status: each head matches the SHA in its ready note (b4af3c6 / 9530bff / faa85e9 / b6e1055 / 33d3906 / 789d566 / ed04818), all MERGEABLE/CLEAN, all CI jobs green. Every CI run started after main@858d143 (18:13:46Z) and the newest .github/ change on main is 2026-09-08, so no stale-pipeline case and no guard merged after any PR's green.
• Qodo: 0 unresolved qodo threads on all seven, matching authors by startswith("qodo-code-review") over GraphQL reviewThreads (the login carries no [bot] suffix there). Author-leg control fix(logging): a half-finished Delete All left a table-less database, and the next logging toggle killed the app #196 = 3 qodo-authored threads, 0 unresolved, as expected.
• Hazards checked. (a) test(#388): the dialog view-model guard requires the initializer to END at the construction — new VM(...).ToString() no longer passes #405's repo-wide scan over Daqifi.Avalonia/**/*.cs: fix(#412): one NaN reading no longer fails an SD import — dropped and reported, as the live path does (#294) #413's SdCardSessionImporter.cs and fix(#410): Device Logs keeps the device you picked when another one connects or drops, and stops re-listing its SD card #414's DeviceLogsViewModel.cs introduce no ShowDialogAsync<…> or DataContext = token, and test(#388): the dialog view-model guard requires the initializer to END at the construction — new VM(...).ToString() no longer passes #405's guard tests pass on the merged tree — the tree being identical in both orders, the guard's verdict is order-independent. (b) Added top-level types are all distinct (BindingFactsConstructionTests, TheNoFreshDocumentWarning/DiscardingWorker/SentryHubCollection, WireHarness, SdCardImportNaNTests, DeviceLogsSelectionTests) — no two PRs add the same extracted helper. (c) Removed members: test: the last two private IAppLogger fakes fold into RecordingAppLogger — one logger double in the suite (−44) #408's two private nested RecordingLogger fakes (referenced nowhere else) and fix(#410): Device Logs keeps the device you picked when another one connects or drops, and stops re-listing its SD card #414's 2-arg internal DeviceLogsViewModel ctor, which is widened by an optional parameter and so still binds for existing callers. (d) RecordingAppLogger.Warnings/.Errors, which fix(#412): one NaN reading no longer fails an SD import — dropped and reported, as the live path does (#294) #413's new tests consume, survive test: the last two private IAppLogger fakes fold into RecordingAppLogger — one logger double in the suite (−44) #408's fold — that pairing compiles and passes here.
• No packages.lock.json drift; working tree clean. Throwaway branches deleted. Nothing merged, nothing pushed to any PR branch.

[tylerkron]

Sweep: the combination now includes #415 — 8-way merge verified. Nothing merged, nothing pushed.

The earlier sweep built #405 #406 #407 #408 #411 #413 #414 onto main 858d143 → tree fb11889, 1096/1096. #415 opened afterwards and had been built with nothing. This run re-derived the state and rebuilt the set with #415 in it, so this PR is now verified against the full combination the user is about to merge.

State re-derived

origin/main is still 858d143 — unchanged. Re-running the seven-way octopus in a fresh worktree reproduced tree fb11889 byte-for-byte, which independently confirms both that main has not moved and that the earlier result stands. All seven heads unchanged; this PR is still MERGEABLE/CLEAN with CI green on its ready-note SHA and 0 unresolved qodo threads (author filter validated against the #196 control = 3 threads).

#415 on its own

92db2b6 — ready note names the current head, CI run 35470171059 green on that SHA, MERGEABLE/CLEAN, 0 qodo threads, and its green does not predate main's head or the newest .github/ change (94fcac3, 2026-09-08). It adds no top-level type and removes no member: the whole diff is nullable annotations (DataSample?, Expression?, CancellationTokenSource?, LoggingSession?, string?) across four production files, +8/−8.

8-way merge

Two orders — octopus main ← 405…415, and sequential 415, 414, 413, 411, 407, 408, 406, 405 — both giving tree c15f6937439e15485867aac96a4c6c7e8f4d98f9. And git diff fb11889 c15f693 is exactly #415's four files: the 8-way tree is the already-verified 7-way tree plus eight lines, so nothing about this PR's position in the set changed.

Full gate under one build.1 lock, judged by exit code:

leg	result
desktop --no-incremental	✅ exit 0, 155 warning(s)
iOS restore (no -r)	✅ exit 0
iOS Release -r iossimulator-arm64 --no-restore	✅ exit 0, 156 warning(s), 112.0 s
HeadlessBench Release --no-incremental	✅ exit 0, 156 warning(s), 3.5 s — it emitted Program.cs(2060,13) CS0618, so it really compiled #406's Program.cs (the previous sweep caught this leg "succeeding" in 0.77 s against a stale assembly)
dotnet test	✅ 1096 / 1096, 0 failed, 0 skipped

git status clean, no packages.lock.json drift.

Warnings improve, and nothing is added

Both ends re-measured here with --no-incremental, not carried over: desktop 166 → 155, iOS Release 167 → 156. The set diff is identical on both heads — 11 removed, 0 added, no line-number shift (6 × CS8625 + 5 × CS8618, all #415's); 8 of the 10 other warning codes bit-identical. The reconciliation identity closes at all four ends.

The one cross-PR hazard checked, and cleared

#415 annotates production members nullable while #407/#408/#413 own the IAppLogger test fakes, and #415's body says two Error(null, …) sites were left because of those PRs. In the merged tree those sites are still exactly two (DatabaseMigrator.cs:739, SdCardSessionImporter.cs:751 — #413 shifted the latter but neither added nor removed one), no test type implements IChannel, and #414's vm.SelectedDevice?.DeviceSerialNo is IStreamingDevice's member, not the Notifications one #415 touches. The decisive check is that the test project compiles at all: it runs TreatWarningsAsErrors, so a CS8602/CS8604/CS8767 cascade into the merged test files would have failed the build rather than passed quietly.

Test count reconciles

1057 (main) + 19 (#405) + 0 (#406) + 0 (#407) + 0 (#408) + 3 (#411) + 8 (#413) + 9 (#414) + 0 (#415) = 1096 ✅. #415's +0 was re-derived from its diff — it touches no file under Daqifi.Avalonia.Tests/.

Not covered

#363 (dependabot, BLOCKED) has never been built with any of these eight. Android was not built locally (workload absent on this Mac); CI covers it. No visual re-capture — none of the eight touches tools/parity-audit/baselines/macos-arm64.sha256, so the combination's visual state is unchanged from the 7-way's, but the combination itself was not re-captured.

Throwaway branches deleted. Nothing merged, nothing pushed.

[tylerkron]

Sweep: #414 re-verified and built with the other five, onto today's main

Result: green. The previous sweep built this PR onto main 858d143; main has since moved to d8ef0f9 (#405, #406, #407 merged), so that verification needed redoing.

check	result
ready-note SHA = head	✅ note at 21:09:11Z names ed04818 = current head, no push since
CI green on that SHA	✅ run 35468884235 "Build", head_sha ed04818, success
mergeable	✅ MERGEABLE / CLEAN
qodo threads	✅ 0 threads, 0 unresolved (author-leg control #196 returns 3, so the filter is live)
stale pipeline	✅ newest .github/ change on main is 2026-09-08
stale base	⚠️ yes — see below

⚠️ This PR's green predates all three merges

#405 merged 04:22:46Z, #406 04:25:27Z, #407 05:00:43Z (2026-09-20). This PR's newest Build run was created 2026-09-19T20:55:51Z — before all three. The badge is displayed against a main that no longer exists (strict: false), so the local build below is the only current evidence for this PR. Same is true of #408, #411, #413 and #415; only #416's run postdates the merges.

Specific to this PR — one false positive worth naming

A removed-member scan of this diff (the #189 heuristic: any removed public/abstract/virtual member needs a compile, not a status query) flags:

-    public void RefreshSdCardFiles() { }

That is not a #189 shape. It is a no-op stub inside the RecordingStreamingDevice test double, replaced in the same diff by => Interlocked.Increment(ref _sdCardListings). The production member is untouched — AbstractStreamingDevice.cs:1662 and IStreamingDevice.cs:63 both still declare it, DeviceLogsViewModel.cs:483 still calls it, and DeviceLogsView.axaml:243 still binds RefreshSdCardFilesButton. Recording it because the heuristic cannot distinguish a production member from a test double's stub of the same name, and this one reads exactly like the dangerous case.

The internal DeviceLogsViewModel(IAppLogger?, ISdCardSessionImporter?) ctor change is .cs-only and therefore fully covered by the compile.

This PR also edits DeviceLogsViewModel.cs, inside the scope of the repo-wide BindingFacts source scanner that #405 tightened, and shares the shared-test-double surface with #408 (different files: Device/RecordingStreamingDevice.cs vs RecordingAppLogger.cs). Both cleared — see below. Test delta re-derived from the diff: +9.

The 6-way merge

origin/main d8ef0f9 ← #408 + #411 + #413 + #414 + #415 + #416, built in two orders
(octopus, and sequential 416→415→414→413→411→408). Both land on the same tree
ebe16170827e2f2c2cdb745a2ec41885a8921f40. One auto-merge, SerialPortPollingDeviceWatcherTests.cs.

git diff c15f693 ebe1617 — against the tree the 8-way sweep verified — is one file, +650/−0: BootloaderHoldServiceTests.cs. So #405/#406/#407 merged as squashes into exactly the content that sweep predicted, and #416 is textually orthogonal to the other five.

Full gate on ebe1617 — judged by exit code, not by grepping for Build succeeded

leg	result
desktop --no-incremental	exit 0, 155 warning(s)
iOS restore (no -r)	exit 0
iOS Release -r iossimulator-arm64 --no-restore --no-incremental	exit 0, 156 warning(s)
HeadlessBench restore	exit 0
HeadlessBench Release --no-incremental	exit 0, 156 warning(s)
dotnet test	1116 passed / 0 failed / 0 skipped

Working tree clean, no packages.lock.json drift. The HeadlessBench leg demonstrably compiled — its output carries Program.cs(2060,13): warning CS0618, not a stale-assembly no-op.

Test reconciliation — each delta re-derived from that PR's own diff

main d8ef0f9 = 1057 (at 858d143) + 19 (#405/#406/#407 together) = 1076.

#408	#411	#413	#414	#415	#416
+0	+3	+8	+9	+0	+20

1076 + 0 + 3 + 8 + 9 + 0 + 20 = 1116 ✅

Hazards checked by compilation, not by argument

• chore(warning-zero): CS8625 ×6 — the members that are documented as null-when-empty are now declared nullable (8 lines, 4 files) #415's nullable annotations × the test-side PRs. Daqifi.Avalonia.Tests.csproj has <Nullable>enable</Nullable> + <TreatWarningsAsErrors>true</TreatWarningsAsErrors>, so any CS8602/CS8604/CS8767 cascade into test: the last two private IAppLogger fakes fold into RecordingAppLogger — one logger double in the suite (−44) #408's folded logger, test(#409): the flash probe's cancellation token and the no-document warning are now pinned through app code, not only Core #411's harnesses, fix(#412): one NaN reading no longer fails an SD import — dropped and reported, as the live path does (#294) #413's/fix(#410): Device Logs keeps the device you picked when another one connects or drops, and stops re-listing its SD card #414's/test(coverage-gap): pin BootloaderHoldService — the hold that decides whether a firmware flash can proceed #416's new tests would have failed the build. It compiled. Cleared.
• test: the last two private IAppLogger fakes fold into RecordingAppLogger — one logger double in the suite (−44) #408 and fix(#410): Device Logs keeps the device you picked when another one connects or drops, and stops re-listing its SD card #414 both alter shared test doubles — different files, zero overlap (RecordingAppLogger.cs vs Device/RecordingStreamingDevice.cs). The suite-wide consumer risk is what the compile + 1116 tests rule out. Cleared.
• Duplicate top-level types (fix(notifications): the flyout names its empty state instead of rendering a blank panel #258 shape). test(coverage-gap): pin BootloaderHoldService — the hold that decides whether a firmware flash can proceed #416's new file declares exactly one top-level type; FakeHidTransport/ConnectCall are nested. No name introduced by any of the six exists on main or in another PR. Cleared.
• test(#388): the dialog view-model guard requires the initializer to END at the construction — new VM(...).ToString() no longer passes #405's repo-wide guard. BindingFacts.cs:270 scans Daqifi.Avalonia/**/*.cs — production, so fix(#410): Device Logs keeps the device you picked when another one connects or drops, and stops re-listing its SD card #414's and chore(warning-zero): CS8625 ×6 — the members that are documented as null-when-empty are now declared nullable (8 lines, 4 files) #415's edits are in scope. Run explicitly on the merged tree: --filter "FullyQualifiedName~Binding" → 175 passed, 0 failed, including BindingFactsConstructionTests and the dialog/DataContext guards. Cleared.

Not claimed

#363 (dependabot, UNKNOWN) was not built with any of these. Android not built here — CI covers it. No visual re-run and none needed: the merged diff is 15 files, all .cs; no .axaml and no baseline manifest. No bench run — none of the six is device-observable. main's own warning baseline was not re-measured, so no warning delta is claimed.

Full log: ~/.claude-daqifi/avalonia-loop/logs/20260920T053803Z-sweep-6way.md.
Nothing merged, nothing pushed.