[tylerkron]

Closes #412. Not merging — for review.

What was wrong

If an SD card log held even one NaN reading, the whole import failed with Import Failed. Microsoft.Data.Sqlite refuses to bind a NaN (Cannot store 'NaN' values.), so the 1,000-sample batch holding it failed, along with everything the parser had not reached yet. A NaN in the first batch meant nothing was imported. A later NaN left an ImportFailed fragment with the rest of the file dropped. Either way the failure was logged at Error, which makes it a Sentry event. Core's parsers pass NaN through: the CSV parser accepts NaN/nan, and the .bin float leg takes the float as-is.

The fix

SdCardSessionImporter.WriteSamplesAsync now applies the live path's rule from #294 (SessionSampleWriter.Add): it skips a NaN reading, counts it, and imports everything else.

• NaN only, not IsFinite. I measured this instead of assuming it: Infinities_are_stored_not_dropped imports 1e400 and -Infinity through the real CSV parser and reads +∞/-∞ back from the database. The database refuses only NaN, and fix(logging): one unstorable reading threw away the rest of the session — #247's characterization, replaced #294's guard is NaN-only for the same reason. The issue suggested IsFinite, following fix(#295): one bad reading made the Summary Average read NaN for the rest of the session — refuse it where it enters the app #298, but fix(#295): one bad reading made the Summary Average read NaN for the rest of the session — refuse it where it enters the app #298 guards ActiveSample (the running statistics), not storage. So an imported session can still contain ±∞. That is unchanged, and it is a deliberate choice, not an oversight.

• The user is told. OutcomeGuidance gains the sentence "Some readings in the log were NaN (they had no numeric value) and were left out; every other reading was imported." All three import surfaces already show OutcomeGuidance:

  • DaqifiViewModel import-from-disk, :1523
  • DeviceLogsViewModel.ImportFile, :562
  • Import All, through RecordNotice → BuildImportAllSummary, :670

  Like the other guidance sentences it names no count and no file, so Import All shows it once however many files are affected.

• When every reading is NaN, the dialog no longer says "The log contained no samples". It says every reading was NaN, so nothing could be stored and no session was created.

• Logging: one Warning per import with the count. There is no Error, so no Sentry event, and no line per sample.

• No shared helper with fix(logging): one unstorable reading threw away the rest of the session — #247's characterization, replaced #294. The duplicated part is double.IsNaN(value). The reporting has a different shape on each side: fix(logging): one unstorable reading threw away the rest of the session — #247's characterization, replaced #294 is rate-limited per stream on a long-running writer, while this is one summary for a finite import. A helper would add a layer without removing any code.

• Test seam: the existing internal constructor takes an optional IAppLogger (default AppLogger.Instance), so a test can check which level an outcome was logged at. The production constructor is unchanged.

Diffstat: +61/−12 in SdCardSessionImporter.cs, plus a new 257-line test file.

Verification

• 8 new tests in SdCardImportNaNTests. I ran them against the unchanged code, with only the logger seam added. 6 of 8 fail, all with InvalidOperationException: Cannot store 'NaN' values.:

  • NaN in the first batch
  • NaN in a later batch (past the first 1,000-sample commit)
  • user-facing guidance
  • Warning-not-Error
  • every reading NaN
  • a firmware CSV with lower-case nan through ImportFromFileAsync

  The other 2 pass on main, as they should: a clean log produces no guidance, and infinities store. They pin behaviour that must stay the same.

• Mutation control: I changed the NaN summary from Warning to Error. That killed 3 tests, all on the Assert.Empty(_log.Errors) line, so the no-Sentry assertion does catch a regression.

• Build gate green locally under a build.N lock: desktop, iOS restore, iOS Release iossimulator-arm64. Full suite 1065/1065 passed. Lock files unchanged. Android is left to CI.

• No board attached. This change can't be observed on the device anyway: the import path runs from a file, and the tests go through the real parser.

• Warnings, taken from CI's own logs at both ends (Library + Desktop head job): merge-base 858d143 166, this head 789d566 166. The importer has the same 3 pre-existing warnings on both sides (CS8625 plus two CS8601), each shifted +48 lines. The new test file adds none. Tests went from 1057 to 1065 (+8).

🤖 Generated with Claude Code

[tylerkron]

/agentic_review

[qodo-code-review]

PR Summary by Qodo

Skip NaN readings during SD card imports

🐞 Bug fix 🧪 Tests 🕐 40+ Minutes

AI Description

• Skip NaN SD readings so valid samples continue importing.
• Report omissions through user guidance and one warning, without Sentry errors.
• Cover batch boundaries, all-NaN logs, CSV parsing, logging, and infinity persistence.

Diagram

graph TD
  P["SD Log Parser"] --> I["Session Importer"] --> N{"Reading NaN?"}
  N -- No --> B["Sample Batch"] --> D["SQLite Session"]
  N -- Yes --> C["Dropped Counter"] --> R["Import Outcome"]
  D --> R

Loading

High-Level Assessment

The focused NaN-only guard is the best approach because it mirrors the established live-sample behavior and filters exactly what Microsoft.Data.Sqlite rejects. Using IsFinite would unnecessarily discard supported infinities, while extracting a shared helper for a single double.IsNaN check would add indirection without unifying the distinct reporting lifecycles.

Files changed (2) +318 / -12

Bug fix (1) +61 / -12

SdCardSessionImporter.csDrop and report NaN readings during SD imports +61/-12

Drop and report NaN readings during SD imports

• Filters NaN analog readings before SQLite batching, counts omissions, and emits one warning per import. Adds user-facing guidance for partial and all-NaN imports and introduces injectable logging through the internal test constructor.

Daqifi.Avalonia/Daqifi.Desktop/Loggers/SdCardSessionImporter.cs

Tests (1) +257 / -0

SdCardImportNaNTests.csAdd end-to-end coverage for NaN-tolerant SD imports +257/-0

Add end-to-end coverage for NaN-tolerant SD imports

• Adds database-backed tests proving NaN readings are omitted without losing valid samples across batch boundaries. Covers user guidance, warning-level logging, all-NaN logs, real CSV parsing, and preservation of positive and negative infinity.

Daqifi.Avalonia.Tests/Loggers/SdCardImportNaNTests.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 789d566 ⚖️ Balanced

Results up to commit 789d566 ⚖️ Balanced

No changes from previous review

[tylerkron]

/agentic_review

[qodo-code-review]

Code review by qodo was updated up to the latest commit 789d566

[tylerkron]

Qodo-clean, CI green — ready for review. Two clean rounds on head 789d566, the second graded Balanced (fired 21:00:57Z, summary updated 21:03:45Z), both settled with 0 unresolved qodo threads. CI passed all heads on that same SHA, including 1065/1065 tests. 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: #413 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:08:15Z names 789d566 = current head, no push since
CI green on that SHA	✅ run 35468880064 "Build", head_sha 789d566, 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:46Z — 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, #414 and #415; only #416's run postdates the merges.

Note also that the ready note quotes 1065/1065, measured on the older base. On today's main the six-way tree runs 1116/1116, and this PR's own contribution re-derives from its diff as +8 (SdCardImportNaNTests, eight [Fact]).

Specific to this PR

#413 edits SdCardSessionImporter.cs, a production file inside the scope of the repo-wide BindingFacts source scanner that #405 tightened. That guard was run explicitly on the merged tree (--filter "FullyQualifiedName~Binding" → 175 passed, 0 failed). The new type SdCardImportNaNTests does not exist on main and is not introduced by any other PR in the set.

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.