New connections should fail if there's an already open connection #40
Conversation
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
There was a problem hiding this comment.
Pull Request Overview
This PR changes the connection behavior to prevent multiple simultaneous connections by throwing a ConnectionError.busy error when attempting to establish a new connection while one already exists. Previously, the system would automatically close existing connections and replace them with new ones.
- Adds a new
ConnectionError.busycase to indicate an active connection exists - Updates connection managers to check for existing connections and throw
.busyinstead of replacing them - Modifies the SmartCard connection cleanup to be handled asynchronously when connections close
- Updates tests from XCTest to the Swift Testing framework with proper connection lifecycle management
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| Connection.swift | Adds new busy error case and Sendable conformance |
| SmartCardConnection.swift | Replaces automatic connection replacement with busy error and adds async cleanup |
| NFCConnection.swift | Changes from invalidating existing connections to throwing busy error |
| LightningConnection.swift | Removes concurrent connection task handling in favor of busy error |
| ConnectionFullStackTests.swift | Migrates tests to Swift Testing framework and adds proper connection lifecycle testing |
jdmoreira
commented
Aug 6, 2025
| public enum ConnectionError: Error { | ||
| public enum ConnectionError: Error, Sendable { | ||
| /// There is an active connection. | ||
| case busy |
There was a problem hiding this comment.
I'm open to some other naming here!
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This Pull Request changes the behaviour around Connections creation.
Before this Pull Request when a new connection was requested the previous one was closed.
Now when a new connection is requested, and there is already a connection,
ConnectionError.busyis thrown.ConnectionFullStackTestshad to be changed to test for the new behaviour instead of the old one and I also took the opportunity to migrate them from the old XCTest framework to the new Swift Testing framework.I've run the Connection tests for the three different kinds of connections and made sure they passed both on iOS and macOS. (actually
sendManually()fails for iOS with NFC but that's a entitlements issue that was already failing before and I will fix in a different pull request).