Repository navigation
fix(connection): a failed USB connect closed the dialog silently and told the user nothing - #209
Conversation
…told the user nothing
Picking a discovered device out of the USB tab and pressing Connect ran
ConnectSerialAsync, which connected each selected device and then called
RaiseCloseRequested() unconditionally. A failed connect was therefore
indistinguishable from a successful one: the dialog dismissed itself, the device
never appeared in the device list, and nothing on screen said why. The user's
only signal was the absence of the thing they asked for.
Not a rare path. A device left streaming over WiFi answers the switch-to-USB
with a SCPI error; a port held by another application fails the open. Both land
here.
The Manual USB tab in the same file already does the right thing fifteen lines
below, and its comment says why — "the dialog would close silently and the user
would have no idea the connection failed". The discovered-device path, which is
the common one, never got it. The view model's own CloseRequested doc comment
already claimed the behaviour ("Not raised on empty input or failed connect");
the serial path did not honour it.
Port the capability from daqifi-desktop: a SerialConnectError property, a status
check after each device, and a red TextBlock bound to it in the USB tab. The
check is per-device rather than once after the loop because ConnectionStatus is
a single shared field on the ConnectionManager singleton and the list is
multi-select, so a later device's success would otherwise overwrite an earlier
device's failure and the dialog would close reporting nothing. On failure,
discovery restarts so the list keeps refreshing.
Left behind deliberately: upstream logs nothing extra here and neither does
this, because ConnectionManager.Connect already logs every path that sets Error.
The tests drive the real ConnectSerialAsync without opening a COM port, by
setting DeviceBeingUpdated so Connect refuses USB connects and reports Error
before any I/O — the same gate also holds the failure path's discovery restart
down, so no finder is ever created. Verified non-vacuous: dropping the early
return turns both dialog-closing assertions red.
That made a second class a writer of ConnectionManager.Instance, which xUnit
would run in parallel with ConnectionDialogFirmwarePauseTests — one clearing the
firmware gate the other relies on to keep discovery off the bus, which reached a
real UDP finder once. Both classes now share a non-parallel collection.
ConnectionManagerTeardownTests builds its own ConnectionManager and is
deliberately not a member.
closes #207
|
/agentic_review |
PR Summary by QodoSurface failed USB connections without closing the dialog
AI Description
Diagram
High-Level Assessment
Files changed (5)
|
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Qodo read the previous comment ("keeps finding devices") as promising a refreshed
list, and it does not deliver one: StartSerialDiscovery recreates the finder but
never clears AvailableSerialDevices, unlike StartWiFiDiscovery.
Not clearing is the right behaviour on this path — the device that just failed is
the one the user is most likely to retry, and blanking the list back to
"Scanning..." would make them wait for rediscovery first; upstream does not clear
here either. So the comment was what was wrong, not the call. The underlying
serial/WiFi asymmetry is pre-existing at three other call sites and is filed as
#212 for a deliberate decision.
|
Both findings are correct descriptions of real weaknesses. Neither is introduced by this PR, and the remediation each asks for is a change to shared connection/discovery behaviour rather than to this PR's parity target — so both are filed as #212 and one comment here has been corrected. Detail: 1. Shared status misattributes connects — High — valid, pre-existing, filedAccurate. What the finding does not say is that this is the established pattern of the file, at two call sites that predate this PR:
So the choice here is not "correct result vs. shared field"; it is "no failure check at all vs. the same check the other two paths already use". Before this PR the discovered-device path read nothing and closed unconditionally, which is strictly worse than a check that can be misattributed under a rare interleaving. The suggested fix — an immutable per-call result from
Taking that recommendation. Filed as #212 §1 with all three call sites. 2. Restart retains stale devices — Medium — valid asymmetry, but clearing here would be wrongThe asymmetry is real: But clearing on this path specifically would make the feature worse, not better. The whole point of the PR is that the user can retry after a failure without reopening the dialog, and the device most likely to be retried is the one that just failed. Blanking the list back to "Scanning for USB devices…" would force them to wait for rediscovery before they can try again. Upstream What was wrong is my comment, which said "keeps finding devices" and so read as promising a refresh it does not perform. Fixed in No behavioural change in that commit; 341 tests still pass and desktop + iOS still build. |
|
/agentic_review |
|
Code review by qodo was updated up to the latest commit 52823c9 |
The dialog has now been rendered and looked atThis PR shipped with an explicit disclosure that the connect dialog had never been photographed, Method. I temporarily extended the capture harness to build the real The error rendersAttached below, in order: the failed-connect state, then the same tab with no error for contrast. Red, wrapped over two lines, sitting above the separator, with the failed device still in the list Above the pixels, the harness also read the element straight out of the visual tree, because a So all three things a green build could not tell us are confirmed:
The second image is the contrast, and it earns its placeWith Also re-verified on this head (
|
| result | |
|---|---|
| desktop build | green |
iOS restore + -c Release -r iossimulator-arm64 --no-restore |
green |
dotnet test |
341/341, 0 failed, 0 skipped |
| capture harness | all 20 screens captured, none skipped |
packages.lock.json |
untouched |
main moved? |
no — merge-base is origin/main (cd44a11), so there is nothing to test-merge |
| CI | all 6 checks green, Android and iOS heads included |
The harness's inability to photograph any dialog is a standing hole in the visual gate rather than
a problem with this PR, so it is filed separately as #213 — including what had to be done by
hand here, and why the indeterminate "Scanning…" progress bar makes naive dialog capture fail. Not
fixed here: #204 is mid-flight in that directory.
|
Qodo-clean, CI green — ready for review. The connect dialog was rendered and looked at (screenshots above), so the "did not render the dialog" disclosure is closed by observation rather than repeated; the harness gap that forced it is filed as #213. |
Not merging — for review.
closes #207
Before
Pick a device out of the USB tab's discovered list, press Connect, and if the connect fails: the dialog closes, the device never appears in the device list, and nothing tells you why.
ConnectSerialAsyncconnected each selected device and then calledRaiseCloseRequested()unconditionally, so failure and success looked identical. The only signal was the absence of the thing you asked for.Not a corner case. A device left streaming over WiFi answers the switch-to-USB with a SCPI error; a port held by another application fails the open. Both land here.
After
The dialog stays open and says which device failed:
Discovery resumes — it was drained before the connect — so the list is not left frozen and you can try again without reopening the dialog. The list is deliberately not cleared: the device that just failed is the one you are most likely to retry, and blanking it back to "Scanning…" would make you wait for rediscovery first.
Why this was a parity gap and not just a bug
daqifi-desktophas done this for years —SerialConnectError, a per-device status check, and a redTextBlockin the USB tab. The port has neither the property nor the element;SerialConnectErrorappeared nowhere inDaqifi.Avalonia.Two things made it easy to miss. The Manual USB tab in the same file already does it correctly fifteen lines below, with a comment saying exactly why — "the dialog would close silently and the user would have no idea the connection failed" — so the file looks like it handles this. And the view model's own
CloseRequesteddoc comment already claimed the behaviour ("Not raised on empty input or failed connect"); the serial path simply did not honour its own contract.The per-device check is load-bearing
The status is read after each device, not once after the loop.
ConnectionStatusis a single shared field on theConnectionManagersingleton and the USB list isSelectionMode="Multiple", so with several devices selected a later device's success would overwrite an earlier one's failure and the dialog would close reporting nothing. Upstream's comment records the same reasoning.What I deliberately did not port
ConnectionManager.Connectalready logs every path that setsError. A second log line would be duplicate noise.ConnectAsynchas the same unconditional close, in both apps. That is an upstream bug, not a parity gap, so fixing it here would be scope creep and would put the two apps out of step. Worth filing separately.Verification
dotnet builddesktopdotnet restore+build -c Release -r iossimulator-arm64 --no-restoreiOSdotnet testpackages.lock.jsonAndroid cannot be built on this Mac (missing workload), so I did not build it — but CI did, and every check passed on this branch, Android and iOS heads included.
The new tests are non-vacuous. Removing the early
returnon the failure path turns both dialog-closing assertions red with their own messages; I ran that deliberately before restoring it. They drive the realConnectSerialAsyncwithout opening a COM port, by settingDeviceBeingUpdatedsoConnectrefuses USB connects and reportsErrorbefore any I/O — the same gate holds the failure path's discovery restart down, so no finder is ever created.The dialog has been rendered — see this comment for the screenshots. The original disclosure here said it had not been, because
tools/parity-audit/AvaloniaCaptureconstructs no dialogs at all. A shepherd pass closed that by temporarily extending the harness to build the realConnectionDialogwith a real view model, seed one discovered device, select the USB tab and capture it with and withoutSerialConnectErrorset — same headless Skia + HarfBuzz path and settle loop as every other capture. The harness edit was reverted and nothing was pushed;tools/is untouched on this branch.The message renders: red, wrapped over two lines, above the separator, with the failed device still listed and selectable for a retry. Read straight out of the visual tree rather than inferred from pixels — a bound-but-blank
TextBlocklooks the same in a screenshot as one that never rendered — the element reportsIsVisible=True,Bounds=0,338,528,27,Foreground=#fff43f5e(the sameStatusRedroseManualPortErroruses), and the full expected text. So the reflection-resolved binding, theNotNullToVisconverter and theStatusRedresource are all confirmed, none of which a green build could establish. With the error null the new row collapses to zero height, leaving the separator and Connect button exactly where they were.Still not verified: nobody has driven this dialog interactively in a windowed app, and it has not been exercised against a device that genuinely fails to connect. The harness's inability to photograph any dialog is filed as #213.
No bench validation — reproducing this needs a device that fails to connect, and the board was not required for anything else here.
A pre-existing flake found on the way, filed not fixed
Stabilising this surfaced a rare
ObjectDisposedException: SQLitePCL.sqlite3in an unrelated test — once in 32 full-suite runs on this branch, zero in 20 on the merge-base. Cause: five test classes call the process-globalSqliteConnection.ClearAllPools()while xUnit runs test classes in parallel. All five call sites are onmain; this branch only perturbs scheduling. Filed as #210 rather than fixed here, to keep this PR to its target and stay out of concurrent work in the test project.I am not claiming it is proven unrelated — 20 clean base runs do not rule out a ~1-in-32 event. What is established is the mechanism, which is entirely on
mainand independent of anything here.Review
Qodo raised two findings, both accurate descriptions of pre-existing weaknesses rather than anything this PR introduces, and both now tracked in #212:
ConnectionStatus. True — and it is the third instance of that pattern, joiningConnectManualSerialAsync(line 646) andConnectManualWifiAsync(line 731), both onmain. The alternative this PR replaces was no failure check at all. The proposed remedy — a typed per-call result fromConnectionManager.Connect, all callers updated — is the right direction and is what Qodo's own High-Level Assessment recommends deferring to a separate connection API redesign.AvailableSerialDevices, unlike the WiFi path. The asymmetry is real and pre-existing at three call sites. Clearing on the failure path would be actively worse, though: the device that just failed is the one the user wants to retry, and blanking the list to "Scanning…" makes them wait for rediscovery. Upstream does not clear either. What was wrong was my comment promising a refresh it does not perform — corrected in52823c9.Round 2 on head
52823c9: 0 bugs, 0 rule violations, both threads answered and resolved.Diffstat
+298 / -2across 5 files — of which production code is+43 / -2across 2 files; the remaining+255is tests.About that test-collection line
Adding a second class that writes
ConnectionManager.Instancecreated a real race withConnectionDialogFirmwarePauseTests: xUnit runs distinct classes in parallel, one clears the firmware gate the other relies on to keep discovery off the bus, and it reached a real UDP finder once during development. Both classes now share a non-parallel collection.ConnectionManagerTeardownTestsconstructs its ownConnectionManager, shares no state, and is deliberately not a member — the collection's doc says so, so nobody adds it later by pattern-matching.