fix(serial): handle missing COM port gracefully and surface in dialog (#524)#525
Open
tylerkron wants to merge 1 commit into
Open
fix(serial): handle missing COM port gracefully and surface in dialog (#524)#525tylerkron wants to merge 1 commit into
tylerkron wants to merge 1 commit into
Conversation
…#524) Sentry was capturing FileNotFoundException from SerialPort.Open() as an error event whenever a user tried to connect to a COM port that did not exist (typo in Manual USB tab, device unplugged between discovery and click). It is a user/environmental condition, not an app bug. - Catch FileNotFoundException and UnauthorizedAccessException in SerialStreamingDevice.Connect and log via AppLogger.Warning instead of AppLogger.Error (the latter is the sole Sentry capture path). - Pre-validate the manually-entered port in ConnectManualSerialAsync against SerialPort.GetPortNames before attempting to connect, and surface a friendly inline error in the dialog when the port is absent. - After the connect attempt, check ConnectionManager.ConnectionStatus and keep the dialog open with an error message on failure (covers the "port held by another process" case that pre-validation cannot catch). - Auto-clear the validation error as soon as the user edits the field. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
Contributor
Review Summary by QodoHandle missing COM ports gracefully with pre-validation and error display
WalkthroughsDescription• Catch FileNotFoundException and UnauthorizedAccessException separately in SerialStreamingDevice.Connect, logging as warnings instead of errors to prevent Sentry capture of user/environmental conditions • Pre-validate manually-entered COM ports against SerialPort.GetPortNames() before connection attempt, displaying inline error in dialog • Check ConnectionStatus after connect attempt and keep dialog open with error message on failure, covering "port in use" scenarios • Auto-clear validation error when user edits the port name field • Add regression test for missing port handling and validation tests for dialog behavior Diagramflowchart LR
A["User enters COM port"] --> B["Pre-validate against GetPortNames"]
B -->|Port absent| C["Display inline error, keep dialog open"]
B -->|Port available| D["Attempt Connect"]
D -->|FileNotFoundException/UnauthorizedAccessException| E["Log as Warning, show error message"]
D -->|Success| F["Close dialog"]
C -->|User edits field| G["Clear error message"]
E -->|Keep dialog open| C
File Changes1. Daqifi.Desktop.Test/Device/DisplayIdentifierTests.cs
|
Contributor
Code Review by Qodo
Context used 1. Unit tests use real SerialPort
|
📊 Code Coverage ReportSummarySummary
CoverageDAQiFi - 19.4%
Daqifi.Desktop.Common - 30.8%
Daqifi.Desktop.IO - 100%
Coverage report generated by ReportGenerator • View full report in build artifacts |
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+93
to
+100
| var device = new SerialStreamingDevice("COM250"); | ||
|
|
||
| var result = device.Connect(); | ||
|
|
||
| Assert.IsFalse(result, | ||
| "Connect() against a nonexistent port should report failure, not throw."); | ||
| Assert.IsFalse(device.IsConnected, | ||
| "Device should not be marked connected when the underlying port could not be opened."); |
Contributor
There was a problem hiding this comment.
1. Unit tests use real serialport 📘 Rule violation ▣ Testability
New tests execute code paths that call SerialPort.Open() / SerialPort.GetPortNames(), which depend on the host environment and can be flaky on CI or non-Windows runners. This violates the requirement to mock external dependencies in unit tests.
Agent Prompt
## Issue description
New unit tests depend on real serial-port APIs (OS/hardware/environment). This can introduce flakiness and violates the requirement to mock external dependencies.
## Issue Context
- `SerialStreamingDevice_Connect_WithMissingPort_ReturnsFalseWithoutThrowing()` calls `SerialStreamingDevice.Connect()` which will ultimately attempt to open a real port.
- `ConnectManualSerialCommand_WithNonexistentPort_SetsManualPortError()` exercises `SerialPort.GetPortNames()` via the ViewModel validation.
## Fix Focus Areas
- Daqifi.Desktop.Test/Device/DisplayIdentifierTests.cs[93-101]
- Daqifi.Desktop.Test/ViewModels/ConnectionDialogViewModelCloseTests.cs[77-92]
- Daqifi.Desktop/ViewModels/ConnectionDialogViewModel.cs[273-288]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes #524.
Sentry was capturing
FileNotFoundExceptionfromSerialPort.Open()as an error-level event whenever a user tried to connect to a COM port that did not exist (typo in the Manual USB tab, device unplugged between discovery and click). That is a user/environmental condition, not an app bug.The fix has three parts:
SerialStreamingDevice.Connectnow catchesFileNotFoundExceptionandUnauthorizedAccessExceptionahead of the generic catch and logs viaAppLogger.Warninginstead ofAppLogger.Error(the latter is the soleSentrySdk.CaptureExceptionpath).ConnectManualSerialAsyncnow checks the entered port againstSerialPort.GetPortNames()before attempting to connect and surfaces a friendly inline error inside the dialog (StatusRedtext under the COM-PORT field) when the port is absent.Connect, we inspectConnectionManager.Instance.ConnectionStatus. OnErrorwe keep the dialog open and show an in-context message — this covers the "port exists but is held by another process" case that pre-validation cannot catch and that previously vanished into a silent dialog close.The validation message auto-clears as soon as the user edits the field.
Test plan
dotnet buildclean (build verified locally; tests run on Windows CI perbuild.yaml).ConnectManualSerialCommand_WithNonexistentPort_SetsManualPortError— error is set, dialog stays open.ManualPortError_ClearsWhenManualPortNameChanges— auto-clear hook fires.WithBlankPorttest tightened to assert no error surfaces for blank input.SerialStreamingDevice.Connectwith a missing port (uses"COM250"so theFileNotFoundExceptionpath is actually exercised on Windows CI).COMxyz) on the Manual USB tab → inline red error appears, dialog stays open, Sentry receives nothing.Notes / follow-ups (out of scope)
ConnectManualWifiAsync,ConnectAsync, andConnectSerialAsync. Worth a separate UX pass.NotNullToVisibilityConverteris now registered both globally (asNotNullToVisinApp.xaml) and locally inDeviceLogsView.xaml— easy consolidation later.🤖 Generated with Claude Code