[tylerkron]

Closes #445. Not merging. This is for review.

What was wrong

Four production types took constructor parameters that were documented as existing "for tests". No test, no tool and no production code ever passed a value for them, so each one was a x ?? productionDefault that always used the right-hand side. Three of the doc comments described tests that do not exist:

• FirmwareUpdateCoordinator: "tests inject a fake to assert WiFi sequencing"
• HidBootloaderDiscovery: "tests inject a fake"
• FirmwareDialogViewModel: "(tests pass mocks via the constructor args)"

A comment like that gives the next reader a reason to leave the code alone without checking whether anything still uses it. This PR deletes the seams and the false comments. It has the same shape as #420.

What changed

The change only removes code. There is no behaviour change and no test change.

type	removed	what the code does now
FirmwareUpdateCoordinator	wifiFirmwareUpdateServiceFactory parameter, its field, its ?? line, 5 doc lines	calls CreateWifiFirmwareUpdateService(...) directly. The local is still typed IFirmwareUpdateService, so the calls go through the same interface as before.
DaqifiViewModel	the same parameter, its 3 doc lines, the forwarding argument	none of the other parameters are affected.
HidBootloaderDiscovery	pollInterval and finderFactory parameters, both fields, both ?? lines, 5 doc lines	Start() builds new HidDeviceFinder() with DefaultPollInterval. It still creates a fresh finder on every Start, as the old factory did.
FirmwareDialogViewModel	firmwareUpdateService and watcher parameters, their ?? legs and docs	resolves both from DI, which was already the only path any caller used.
AppLoggerLoggerProvider	the appLogger parameter and the constructor	the field now initialises from AppLogger.Instance, at the same point in construction.

Two parameters are kept because callers pass real values to them:

• FirmwareDialogViewModel's firmwareDownloadService. AvaloniaCapture passes an offline stub. Its doc now says that instead of "for tests".
• targetDevicePath. The connection dialog passes it.

Diffstat (git diff --numstat origin/main...HEAD): raw +12 / −53 in 5 production files. Blank lines excluded and split by kind:

• code: +8 / −27. All 8 added lines rewrite a line that was removed.
• comments: +4 / −25

Proof that nothing passes these parameters (on origin/main @ 97be4df)

• Case-insensitive base-name scan of every tracked file (.cs, .axaml, tools/, tests, docs) for wifiFirmwareUpdateServiceFactory, finderFactory, pollInterval, HidBootloaderDiscovery and AppLoggerLoggerProvider. Every hit is either the declaration or a single production construction: App.cs:149 passes no arguments, and App.cs:184 passes only the logger.
  • Positive control: the same scan did find SerialPortPollingDeviceWatcher's pollInterval, which tests do pass.
• Constructions of each type. I searched for new T(, and separately for target-typed new( through a declaration or expression-bodied helper whose declared type is T.
  • Positive control: the helper regex found all three test CreateCoordinator() => new(...) helpers for FirmwareUpdateCoordinator. All three pass six positional arguments and then wifiUpdateModeSettleDelay: and canFlashWifiModule: by name. None passes the factory.
  • All 14 test constructions of DaqifiViewModel are new DaqifiViewModel(new NullDialogService()). The : this(...) chain passes four positional arguments and then loggingContextFactory: by name.
  • FirmwareDialogViewModel has exactly two call sites: ConnectionDialogViewModel.cs:868 (two positional arguments) and AvaloniaCapture/Program.cs:1068 (hidDeviceName: and firmwareDownloadService:).
• Reflection and DI. No Activator.CreateInstance, GetConstructor or DI registration names any of the four types. The only Activator sites are exception factories in tests, FirewallManager's COM types and DialogService's parameterless Windows.
• Positional arguments. No caller passes a positional argument at or after any removed position, so every call site compiles unchanged, as the build confirms.
• Open PRs 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, fix(#434): a logged session whose delete fails now tells the user, instead of silently staying in the list #436, test(abstraction-improver): use the SD-card importer seam instead of deleting it — 11 tests over the Import All loop #437, chore(deps): Bump the minor-and-patch group with 18 updates #439 and refactor(logic-simplifier): the connection dialog's "no devices" flags were each list's Count, stored twice #443 touch none of these five files. refactor(logic-simplifier): the connection dialog's "no devices" flags were each list's Count, stored twice #443 edits AvaloniaCapture/Program.cs, but not the FirmwareDialogViewModel construction.

Verification

• Build gate, run under a build.1 lock:
  • desktop head: dotnet build, green (0 errors)
  • iOS restore without -r
  • iOS Release -r iossimulator-arm64 --no-restore: green (exit 0)
• dotnet test Daqifi.Avalonia.Tests: 1179 passed, 0 failed, 0 skipped (macOS, full suite)
• Warnings: no change, read from CI's own logs on both ends (main Build run 35552132041 at 97be4df against this PR's run 35653845541). I compared the distinct first-party warnings, keyed on file + code + message (line numbers left out because the touched files shift). The sets are identical in all four jobs: Android 109/109, Desktop-on-macOS 107/107, Library+Desktop 108/108, iOS 107/107. Those are distinct counts, not the printed totals.
• packages.lock.json is unchanged.
• No device was needed. None of these paths is observable on the board without a flash, and flashing is on the denylist.

Coverage, stated plainly:

• FirmwareUpdateCoordinator is covered by WifiFlashPlatformGateTests, FirmwareUpdateCancellationTests and FirmwareUploadAffordanceTests. None of them constructs it with the factory, so they run exactly the path this PR keeps.
• HidBootloaderDiscovery and AppLoggerLoggerProvider have no covering tests. The change there only touches the constructor signature: each removed ?? always took its right-hand side, which is now written directly. The evidence is a green build plus the unchanged call sites in App.cs.

Qodo

• R1 on 40bfef0 was graded ⚖️ Balanced and came back clean: 0 bugs, 0 rule violations, 0 requirement gaps.
• R2 ran on the same head. It also came back clean, but its rationale reads "No changes from previous review". So only R1 actually reviewed the code, and R2 does not count as an independent second pass.
• No inline threads were opened on either round. I re-checked both surfaces after a settle wait of at least 3 minutes each time.
• CI is green on every job, including the Android head.

Out of scope

• SdCardSessionImporter's internal downloadStallTimeout "test seam" has the same invariant shape: its one caller passes the production constant. It is not removed here. The stall watchdog has no test at all, and the right fix is to write that test using the seam. That is tracked separately (see SD import: ImportOptions has three settings and no production code path sets any of them — ~85 lines handle values the app never passes #444).
• The other exclusions listed in Five constructor seams documented as "for tests" that no test passes — three of the comments describe tests that do not exist #445 stay excluded:
  • VersionNotification, which fix(update): the version check queries daqifi-desktop, reads the wrong assembly, and Sentry-errors on any prerelease tag #345 rewrites
  • ConnectionDialogViewModel and DeviceLogsViewModel, which open PRs are editing and whose seams tests use

🤖 Generated with Claude Code

[tylerkron]

/agentic_review

[qodo-code-review]

PR Summary by Qodo

Remove unused firmware, HID, and logging constructor seams

✨ Enhancement 🕐 10-20 Minutes

AI Description

• Remove unused constructor overrides across firmware, HID discovery, and logging components.
• Use established production defaults and DI paths directly without changing runtime behavior.
• Correct comments that falsely described nonexistent test overrides.

Diagram

graph TD
  C["Update coordinator"] -->|"constructs directly"| W["WiFi updater"]
  H["HID discovery"] -->|"creates on Start"| F["HID finder"]
  D["Firmware dialog"] -->|"resolves services"| DI["App service provider"]
  L["Logger provider"] -->|"uses directly"| A["AppLogger singleton"]

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Require explicit dependency injection

• ➕ Keeps dependencies visible in constructor contracts
• ➕ Preserves straightforward substitution for future unit tests
• ➖ Requires composition-root and caller changes
• ➖ Retains abstractions with no current test or production consumer
• ➖ Expands scope without delivering current behavioral value

Recommendation: Keep the PR's direct-production-path approach. Required constructor injection would be preferable if substitution were actively needed, but repository-wide caller evidence shows these optional seams are unused; removing them reduces misleading API surface while preserving behavior.

Files changed (5) +12 / -53

Refactor (5) +12 / -53

FirmwareUpdateCoordinator.csConstruct the WiFi firmware updater directly +2/-10

Construct the WiFi firmware updater directly

• Removes the unused WiFi update service factory parameter and field. WiFi flashing now calls the existing production factory method directly while retaining the interface-typed local and existing update flow.

Daqifi.Avalonia/Daqifi.Desktop/Device/Firmware/FirmwareUpdateCoordinator.cs

HidBootloaderDiscovery.csUse fixed production HID discovery dependencies +3/-15

Use fixed production HID discovery dependencies

• Removes optional polling interval and finder factory overrides. Each Start call still creates a fresh HidDeviceFinder, now using DefaultPollInterval directly.

Daqifi.Avalonia/Daqifi.Desktop/Device/Firmware/HidBootloaderDiscovery.cs

AppLoggerLoggerProvider.csInitialize the logger provider from the singleton +1/-12

Initialize the logger provider from the singleton

• Removes the unused optional constructor and initializes the backing logger directly from AppLogger.Instance.

Daqifi.Avalonia/Daqifi.Desktop/Loggers/AppLoggerLoggerProvider.cs

DaqifiViewModel.csStop forwarding the unused WiFi updater factory +0/-5

Stop forwarding the unused WiFi updater factory

• Removes the optional WiFi service factory from the view-model constructor and its FirmwareUpdateCoordinator call. Other firmware dependencies and defaults remain unchanged.

Daqifi.Avalonia/Daqifi.Desktop/ViewModels/DaqifiViewModel.cs

FirmwareDialogViewModel.csResolve updater and watcher exclusively through DI +6/-11

Resolve updater and watcher exclusively through DI

• Removes unused constructor overrides for the firmware updater and bootloader watcher, resolving both through the application service provider. Retains the firmware download override used by the capture harness and clarifies its documentation.

Daqifi.Avalonia/Daqifi.Desktop/ViewModels/FirmwareDialogViewModel.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 add REVIEW.md to your repo root and Qodo follows it on every PR

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Previous reviews

Review updated until commit 40bfef0 ⚖️ Balanced

Results up to commit 40bfef0 ⚖️ Balanced

No changes from previous review

[tylerkron]

/agentic_review

[qodo-code-review]

Code review by qodo was updated up to the latest commit 40bfef0

[tylerkron]

Qodo-clean, CI green — ready for review. R1 (⚖️ Balanced) is the only round that reviewed the code; R2 on the same head says "No changes from previous review". Not merging.

[tylerkron]

Sweep: five-way merge of main + #435 + #436 + #437 + #443 + #446 is clean

I built all five PRs merged onto origin/main 97be4df, using the heads 2bacbf3, b36c347, d50ef98, 9e29bb4 and 40bfef0. Nothing was pushed and nothing was merged.

• Merge: there were no conflicts, and no file is touched by more than one of the five PRs. The result is tree 27e0b21cec350c79f4f7c9f4cf4898619b459191. I got the same tree two ways: merging one PR at a time in the order 435→446, and as a single octopus merge in the reverse order.
• Earlier three-way result checked: partway through the ordered merge, the tree after 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, fix(#434): a logged session whose delete fails now tells the user, instead of silently staying in the list #436 and test(abstraction-improver): use the SD-card importer seam instead of deleting it — 11 tests over the Import All loop #437 was bd5c5110. That matches the three-way tree reported earlier.
• Build and tests: the desktop build, iOS restore, iOS Release -r iossimulator-arm64 --no-restore and HeadlessBench (--no-incremental) all exited 0. The full test suite gave 1208 passed / 0 failed / 0 skipped (macOS). No packages.lock.json changed.
• Test count reconciles, starting from main's CI total of 1179: 1179 +3 (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) +6 (fix(#434): a logged session whose delete fails now tells the user, instead of silently staying in the list #436) +11 (test(abstraction-improver): use the SD-card importer seam instead of deleting it — 11 tests over the Import All loop #437) +9 (refactor(logic-simplifier): the connection dialog's "no devices" flags were each list's Count, stored twice #443) +0 (refactor(#445): delete five "for tests" constructor seams no caller ever passes — production −41 lines #446) = 1208. refactor(logic-simplifier): the connection dialog's "no devices" flags were each list's Count, stored twice #443 is +9, not +11: it adds 11 tests and removes the two HasNoWiFiDevices/HasNoSerialDevices InlineData rows. Each PR's delta comes from its own CI total against its own base.
• Parity capture: --check-baseline passed 39/39. The manifest has 39 lines, so every screen was checked. refactor(logic-simplifier): the connection dialog's "no devices" flags were each list's Count, stored twice #443 changes the capture harness. The run logged one non-failing [WARN] between screens: a closed drawer pane was still 2 px wide. No saved screen was affected.
• Warnings: no change. For test(abstraction-improver): use the SD-card importer seam instead of deleting it — 11 tests over the Import All loop #437, refactor(logic-simplifier): the connection dialog's "no devices" flags were each list's Count, stored twice #443 and refactor(#445): delete five "for tests" constructor seams no caller ever passes — production −41 lines #446, CI's first-party set (the Library + Desktop + capture harness job, keyed on file + code + message) is identical to main's 100. 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 and fix(#434): a logged session whose delete fails now tells the user, instead of silently staying in the list #436 were last run by CI on the older base 4716ce1. On that base their warning sets are identical to each other, and the two PRs touch different files, so neither adds or removes a warning. On the merged tree, the local desktop build shows 99 first-party warnings, the same as main. The missing one is the capture harness's own warning, which the desktop build does not compile.
• Changes that could break another PR: refactor(#445): delete five "for tests" constructor seams no caller ever passes — production −41 lines #446 removes constructor parameters from five types, and test(abstraction-improver): use the SD-card importer seam instead of deleting it — 11 tests over the Import All loop #437 changes the DeviceLogsViewModel constructor and its test double. No PR in this set calls a constructor that another PR changes. Every new DaqifiViewModel(...) in the tests passes only the dialog service. The capture harness calls FirmwareDialogViewModel with named arguments. A search for the members refactor(logic-simplifier): the connection dialog's "no devices" flags were each list's Count, stored twice #443 removes returns nothing in the merged tree (it finds 26 on main, so the search works). The five new top-level types are each defined once. The repo-wide SQLite pooling guard passes on the merged tree (120 files).
• Checks on each PR: each head matches its ready note. refactor(logic-simplifier): the connection dialog's "no devices" flags were each list's Count, stored twice #443's ready statement is in its PR body, not in a comment. Every PR is MERGEABLE/CLEAN with 0 unresolved qodo threads; the thread filter finds 3 on control PR fix(logging): a half-finished Delete All left a table-less database, and the next logging toggle killed the app #196. CI is green on every head.
• Out-of-date green on 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 and fix(#434): a logged session whose delete fails now tells the user, instead of silently staying in the list #436: their CI runs (2026-09-20T23:41Z and 2026-09-21T00:19Z) merged into 4716ce1, before the ten commits that took main to 97be4df. No workflow file changed in between. For those two PRs, this local build is the only evidence against current main. I did not build the Android head locally.

[tylerkron]

Sweep update: the six-way merge of main 97be4df + #435 + #436 + #437 + #443 + #446 + #449 is clean. The tree is b5f3d4d7, and the five-way 27e0b21c reproduced exactly. The full gate, HeadlessBench --no-incremental, 1208/1208 tests and parity 39/39 all pass, and this PR's head is unchanged. Details are on #449.