fix: Enhance error handling for connection issues in TapSigner flow NFC interactions#689
fix: Enhance error handling for connection issues in TapSigner flow NFC interactions#689kartikangiras wants to merge 1 commit intobitcoinppl:masterfrom
Conversation
Greptile SummaryThis PR replaces string-based connection error detection with type-safe FFI methods (
Confidence Score: 3/5Not safe to merge: the iOS layer has a likely compile error and a PIN-scrambling overflow bug that must be resolved first. Two P1 findings exist: (1) a type mismatch in TapSignerNFC.swift that will prevent iOS compilation, and (2) a logic bug in the confirm-PIN overflow guard that silently replaces the user's input with the old PIN. iOS was not tested on device or simulator per the checklist. ios/Cove/TapSignerNFC.swift (compile error on line 400) and ios/Cove/Flows/TapSignerFlow/TapSignerConfirmPinView.swift (overflow guard bug on line 186). Important Files Changed
Sequence DiagramsequenceDiagram
participant UI as iOS/Android UI
participant NFC as TapCardNFC / TapCardNfcManager
participant Transport as TapCardTransport
participant Rust as Rust (UniFFI)
UI->>NFC: scan NFC tag
NFC->>Transport: transmitApdu(commandApdu)
alt Tag connection lost
Transport-->>Rust: throw TransportError.ConnectionError(msg)
else Other APDU error
Transport-->>Rust: throw TransportError.UnknownError(msg)
end
Rust-->>NFC: TapSignerReaderError.tapSignerError(.connectionError)
NFC->>NFC: error.isConnectionError() → true
NFC-->>UI: show "Connection Lost" alert
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
praveenperera
force-pushed
the
errorhandling
branch
from
eddc003 to
487e6de
Compare
📝 WalkthroughWalkthroughType-safe NFC connection-error handling was added across Rust, UniFFI, Android, and iOS. A new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI as Mobile UI
participant Transport as Platform Transport
participant Rust as Rust FFI
participant NFC as System NFC
User->>UI: Initiate PIN operation (derive/backup/sign)
UI->>Transport: send APDU/transceive
Transport->>NFC: transmit command
NFC-->>Transport: transceive fails (tag moved / connection lost)
Transport->>Rust: convert/error -> TransportError
Rust-->>Transport: TransportError::ConnectionError
Transport->>Rust: call isConnectionError() (FFI)
Rust-->>Transport: true
Transport->>UI: surface ConnectionException / error
UI->>UI: detect connection error, show "Tag connection lost, please hold your phone still"
UI-->>User: Connection Lost alert
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
doesnt compile |
kartikangiras
force-pushed
the
errorhandling
branch
2 times, most recently
from
1c6455b to
5550361
Compare
|
Hey @kartikangiras your ci tests are failing coz you haven't pushed the latest bindings. You can generate new bindings and push them to get ci passing |
kartikangiras
force-pushed
the
errorhandling
branch
from
5550361 to
c3ee0e1
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
ios/Cove/TapSignerNFC.swift (1)
398-407:⚠️ Potential issue | 🔴 CriticalCompile error:
TapSignerReaderErrorhas no direct.ConnectionErrorcase.
tapSignerErroris typedTapSignerReaderError?. TheTapSignerReaderErrorenum cases are.TapSignerError(TransportError),.PsbtSignError,.ExtractTxError,.UnknownCardType,.NoCommand,.InvalidPinLength,.NonNumericPin,.SetupAlreadyComplete,.InvalidChainCodeLength, and.Unknown— there is no top-level.ConnectionError. TheConnectionErrorvariant lives onTransportError(line 29349), so line 400 must wrap it:Proposed fix
case .readerTransceiveErrorTagConnectionLost: - tapSignerError = .ConnectionError("Tag connection lost, please hold your phone still") + tapSignerError = .TapSignerError( + .ConnectionError("Tag connection lost, please hold your phone still") + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ios/Cove/TapSignerNFC.swift` around lines 398 - 407, The code assigns tapSignerError using a non-existent top-level case `.ConnectionError`; change assignments to wrap the transport-level error inside the TapSignerReaderError by constructing a TransportError.ConnectionError and passing it into TapSignerReaderError.TapSignerError — e.g. set tapSignerError = .TapSignerError(.ConnectionError("Tag connection lost, please hold your phone still")) for the readerTransceiveErrorTagConnectionLost branch (and likewise for the default branch use .TapSignerError(.ConnectionError("Unable to read NFC tag, try again"))), leaving the session.invalidate(...) calls as-is.rust/src/tap_card.rs (1)
105-118:⚠️ Potential issue | 🔴 Critical
TransportError::ConnectionErroris silently lost when round-tripped throughApduError.When
TapcardTransportProtocol::transmit_apdureturnsTransportError::ConnectionError(_), theCkTransport::transmit_apduimpl on Line 86-89 propagates it via?, which goes through theFrom<TransportError> for ApduErrorconversion at Line 113-114 →ApduError::CkTap(CkTapError::BadArguments). Higher-level reader code (e.g.backup,change,deriveintap_signer_reader.rs) then does.map_err(TransportError::from), which converts back via theFrom<ApduError> for TransportErrorimpl at Line 96 →TransportError::CkTap(CkTapError::BadArguments).Net effect: by the time the iOS/Android UI calls
error.is_connection_error(), the variant is no longerConnectionError— it has becomeCkTap(BadArguments), andis_connection_error()returnsfalse. The PR's whole purpose is defeated for any error that originates inside the FFI transport callback.Either add a corresponding variant (e.g.
ConnectionLost) torust_cktap::apdu::Errorupstream and round-trip through it, or bypass theApduErrorboundary for transport-callback errors by preserving the originalTransportErrorvia context passing or short-circuiting inCkTransport::transmit_apdu.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/src/tap_card.rs` around lines 105 - 118, The From<TransportError> for ApduError impl is collapsing TransportError::ConnectionError into CkTap(BadArguments), losing the original connection error when CkTransport::transmit_apdu uses `?`; update the conversion so connection-level transport errors round-trip intact: either add a new ApduError variant (e.g. ConnectionLost or TransportConnectionError) in rust_cktap::apdu::Error and map TransportError::ConnectionError(_) -> ApduError::ConnectionLost (and adjust the reverse From<ApduError> -> TransportError to recreate TransportError::ConnectionError), or short-circuit in CkTransport::transmit_apdu to propagate TransportError directly for transport-callback errors (bypass the ApduError boundary) so callers like backup/change/derive preserve is_connection_error() semantics. Ensure you update the matching arms in From<TransportError> for ApduError and the corresponding From<ApduError> for TransportError to maintain a true round-trip for connection errors.
🧹 Nitpick comments (2)
android/app/src/main/java/org/bitcoinppl/cove/flows/TapSignerFlow/TapSignerEnterPinView.kt (1)
255-260: Optional: extract the duplicated connection-error branch.The same three lines (
Log.w+errorMessage = "Tag connection lost, please hold your phone still") are repeated inderiveAction,backupAction, andsignAction. Consider a small helper that takes the caughtExceptionand returns the resolved error message (mirroring the existingisAuthError/isConnectionErrorpattern) to keep the three failure handlers in lockstep going forward.Also note: the actual triggering of this branch depends on the Rust
TransportError::ConnectionErrorvariant surviving theApduErrorround-trip — see the issue raised onrust/src/tap_card.rs.Also applies to: 323-328, 383-388
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@android/app/src/main/java/org/bitcoinppl/cove/flows/TapSignerFlow/TapSignerEnterPinView.kt` around lines 255 - 260, The three catch branches in deriveAction, backupAction, and signAction duplicate the same connection-error handling (Log.w("TapSignerEnterPin", "TapSigner connection lost") and manager.errorMessage = "Tag connection lost, please hold your phone still"); extract this into a small helper function (e.g., resolveTapSignerError(Exception e): String or handleTapSignerError(Exception e, Manager manager)) that uses the existing isAuthError/isConnectionError predicates and returns or applies the appropriate error message (and performs the Log.w for connection errors); then replace the duplicated blocks in deriveAction, backupAction, and signAction with calls to that helper so all three handlers stay in sync.ios/Cove/Flows/TapSignerFlow/TapSignerConfirmPinView.swift (1)
84-84: Optional: extract the duplicated "Connection Lost" alert.The same
(title: "Connection Lost", message: "Tag connection lost, please hold your phone still")literal appears here and three more times inTapSignerEnterPinView.swift(derive/backup/sign branches). Consider a dedicatedAppAlertStatecase (e.g..tapSignerConnectionLost) or a shared constant so future copy/format changes are made in one place — same pattern already used by.tapSignerInvalidAuthand.tapSignerNoBackupjust above.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ios/Cove/Flows/TapSignerFlow/TapSignerConfirmPinView.swift` at line 84, Extract the duplicated alert literal into a single shared AppAlertState case (e.g. .tapSignerConnectionLost) or a shared constant and replace the repeated inline tuple in the error.isConnectionError() branches with that new case/constant; specifically update the branches inside TapSignerConfirmPinView (where error.isConnectionError() is used) and the three occurrences in TapSignerEnterPinView (derive/backup/sign paths) to return/apply .tapSignerConnectionLost instead of duplicating (title: "Connection Lost", message: "Tag connection lost, please hold your phone still").
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@android/app/src/main/java/org/bitcoinppl/cove/nfc/TapCardNfcManager.kt`:
- Around line 284-287: The catch-all in TapCardNfcManager that currently does
"catch (e: Exception)" and maps every error to
TransportException.ConnectionException should be narrowed: handle NFC
connection-specific failures (e.g., catch android.nfc.TagLostException and/or
IOException that indicates tag lost from the transceive call) and throw
TransportException.ConnectionException for those, while for all other exceptions
(SecurityException, IllegalStateException, malformed APDU bugs, etc.) log the
original error message and rethrow a TransportException.UnknownException
preserving e as the cause; update the Log.e call to include e.message and the
exception (e) for better debugging. Ensure these changes are implemented in the
catch block around the transceive call in TapCardNfcManager so only genuine
tag-connection errors map to ConnectionException and all other exceptions
propagate as UnknownException with the original exception attached.
---
Outside diff comments:
In `@ios/Cove/TapSignerNFC.swift`:
- Around line 398-407: The code assigns tapSignerError using a non-existent
top-level case `.ConnectionError`; change assignments to wrap the
transport-level error inside the TapSignerReaderError by constructing a
TransportError.ConnectionError and passing it into
TapSignerReaderError.TapSignerError — e.g. set tapSignerError =
.TapSignerError(.ConnectionError("Tag connection lost, please hold your phone
still")) for the readerTransceiveErrorTagConnectionLost branch (and likewise for
the default branch use .TapSignerError(.ConnectionError("Unable to read NFC tag,
try again"))), leaving the session.invalidate(...) calls as-is.
In `@rust/src/tap_card.rs`:
- Around line 105-118: The From<TransportError> for ApduError impl is collapsing
TransportError::ConnectionError into CkTap(BadArguments), losing the original
connection error when CkTransport::transmit_apdu uses `?`; update the conversion
so connection-level transport errors round-trip intact: either add a new
ApduError variant (e.g. ConnectionLost or TransportConnectionError) in
rust_cktap::apdu::Error and map TransportError::ConnectionError(_) ->
ApduError::ConnectionLost (and adjust the reverse From<ApduError> ->
TransportError to recreate TransportError::ConnectionError), or short-circuit in
CkTransport::transmit_apdu to propagate TransportError directly for
transport-callback errors (bypass the ApduError boundary) so callers like
backup/change/derive preserve is_connection_error() semantics. Ensure you update
the matching arms in From<TransportError> for ApduError and the corresponding
From<ApduError> for TransportError to maintain a true round-trip for connection
errors.
---
Nitpick comments:
In
`@android/app/src/main/java/org/bitcoinppl/cove/flows/TapSignerFlow/TapSignerEnterPinView.kt`:
- Around line 255-260: The three catch branches in deriveAction, backupAction,
and signAction duplicate the same connection-error handling
(Log.w("TapSignerEnterPin", "TapSigner connection lost") and
manager.errorMessage = "Tag connection lost, please hold your phone still");
extract this into a small helper function (e.g., resolveTapSignerError(Exception
e): String or handleTapSignerError(Exception e, Manager manager)) that uses the
existing isAuthError/isConnectionError predicates and returns or applies the
appropriate error message (and performs the Log.w for connection errors); then
replace the duplicated blocks in deriveAction, backupAction, and signAction with
calls to that helper so all three handlers stay in sync.
In `@ios/Cove/Flows/TapSignerFlow/TapSignerConfirmPinView.swift`:
- Line 84: Extract the duplicated alert literal into a single shared
AppAlertState case (e.g. .tapSignerConnectionLost) or a shared constant and
replace the repeated inline tuple in the error.isConnectionError() branches with
that new case/constant; specifically update the branches inside
TapSignerConfirmPinView (where error.isConnectionError() is used) and the three
occurrences in TapSignerEnterPinView (derive/backup/sign paths) to return/apply
.tapSignerConnectionLost instead of duplicating (title: "Connection Lost",
message: "Tag connection lost, please hold your phone still").
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 597c8a24-4e3b-455a-8f12-5f0db0ddac14
⛔ Files ignored due to path filters (1)
ios/CoveCore/Sources/CoveCore/generated/cove.swiftis excluded by!**/generated/**
📒 Files selected for processing (8)
android/app/src/main/java/org/bitcoinppl/cove/flows/TapSignerFlow/TapSignerEnterPinView.ktandroid/app/src/main/java/org/bitcoinppl/cove/nfc/TapCardNfcManager.ktandroid/app/src/main/java/org/bitcoinppl/cove_core/cove.ktios/Cove/Flows/TapSignerFlow/TapSignerConfirmPinView.swiftios/Cove/Flows/TapSignerFlow/TapSignerEnterPinView.swiftios/Cove/TapSignerNFC.swiftrust/src/tap_card.rsrust/src/tap_card/tap_signer_reader.rs
kartikangiras
force-pushed
the
errorhandling
branch
from
c3ee0e1 to
785de38
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
ios/Cove/Flows/TapSignerFlow/TapSignerEnterPinView.swift (1)
131-142:⚠️ Potential issue | 🟡 MinorMinor inconsistency in
signAction:app.sheetStateis not cleared on connection-lost.In the existing generic failure branch (Line 141)
app.sheetState = .noneis set after presenting the alert, but the newisConnectionError()branch (Lines 135-136) does not do the same. If the intent is to dismiss the NFC sheet on a fatal sign failure, the connection branch should mirror it; if the intent is to keep the sheet open for retry on connection loss, then this is fine — but the asymmetry is worth a deliberate decision.Also (across all three actions in this file), the same
.general(title: "Connection Lost", message: …)literal is duplicated and re-duplicated again inTapSignerConfirmPinView.swift. Consider centralizing as e.g.AppAlertState.tapSignerConnectionLost, mirroring.tapSignerInvalidAuth/.tapSignerNoBackup.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ios/Cove/Flows/TapSignerFlow/TapSignerEnterPinView.swift` around lines 131 - 142, The connection-error branch in the sign failure handler in TapSignerEnterPinView.swift is inconsistent: add app.sheetState = .none inside the else-if error.isConnectionError() branch (the same place where the generic failure sets sheetState = .none) so the NFC sheet is dismissed on fatal sign failures; then remove the duplicated literal by introducing a reusable alert case (e.g. AppAlertState.tapSignerConnectionLost) and replace the hard-coded .general(title: "Connection Lost", message: ...) occurrences in TapSignerEnterPinView and TapSignerConfirmPinView with that new AppAlertState constant to centralize the message.rust/src/tap_card.rs (1)
105-118:⚠️ Potential issue | 🔴 CriticalCritical:
TransportError::ConnectionErroris lost in the round-trip conversion —is_connection_error()will never detect connection errors from the transport callback.When the Swift callback returns
TransportError::ConnectionError, the error flow throughCkTransport::transmit_apdu(line 87) invokes the lossy conversion at line 114, mapping it toApduError::CkTap(BadArguments). When this error is later converted back toTransportErrorvia theFrom<ApduError>implementation (lines 93–103),ApduError::CkTapbecomesTransportError::CkTap, notConnectionError. The original variant is discarded.Since
is_connection_error()(line 544 intap_signer_reader.rs) matches specifically onTransportError::ConnectionError(_), it will always returnfalsefor any connection error that originates from the platform callback, defeating the purpose of detecting network disconnection.To fix:
- Add a
ConnectionErrorvariant to the internalApduErrortype (or equivalent bridge type) so the round-trip preserves the variant, OR- Handle
TransportError::ConnectionErrorat the callback boundary inTapcardTransport::transmit_apdubefore it's converted toApduError, and preserve it via a separate channel (e.g., reader state or thread-local), OR- Remove the
ConnectionErrorvariant fromTransportErrorand theis_connection_error()check if platform-side connection handling is preferred.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/src/tap_card.rs` around lines 105 - 118, The TransportError::ConnectionError variant is being lost when converted into ApduError in the From<TransportError> for ApduError impl (currently mapping ConnectionError to ApduError::CkTap(BadArguments)), so is_connection_error() can never detect platform connection failures; fix by adding a ConnectionError variant to the internal ApduError enum (or equivalent bridge type) and update the From<TransportError> for ApduError conversion to map TransportError::ConnectionError(_) -> ApduError::ConnectionError(...), and ensure the reverse conversion (From<ApduError> for TransportError or the path in TapcardTransport::transmit_apdu) maps ApduError::ConnectionError back to TransportError::ConnectionError so the round-trip preserves the error; update any matching sites like is_connection_error() to match the new ApduError::ConnectionError where appropriate.
🧹 Nitpick comments (4)
android/app/src/main/java/org/bitcoinppl/cove/flows/TapSignerFlow/TapSignerEnterPinView.kt (1)
252-260: Nit: identical 5-line error-classification blocks repeated three times.Lines 252-260, 320-328, and 380-388 are essentially the same
if isAuthError / else if isConnectionError / elsechain with only the action-context differing. Consider extracting a small helper such as:private fun classifyTapSignerError(e: Exception): String = when { isAuthError(e) -> "Wrong PIN, please try again" isConnectionError(e) -> "Tag connection lost, please hold your phone still" else -> "Connection failed, please try again" }and using it in each
catch(combined with the existingLog.wcalls). Keeps wording consistent and makes future error categories one-line additions.Note: with the current broad
catch (e: Exception) → ConnectionExceptionmapping inTapCardNfcManager.transmitApdu,isConnectionError(e)will returntruefor almost every non-auth failure, so the “Connection failed, please try again” else-branch is effectively unreachable until that mapping is narrowed (see comment onTapCardNfcManager.kt).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@android/app/src/main/java/org/bitcoinppl/cove/flows/TapSignerFlow/TapSignerEnterPinView.kt` around lines 252 - 260, Extract the repeated error-classification logic in TapSignerEnterPinView into a small helper (e.g., private fun classifyTapSignerError(e: Exception): String) that returns the appropriate message based on isAuthError(e) / isConnectionError(e) / else, then replace each catch-site’s manager.errorMessage assignment with manager.errorMessage = classifyTapSignerError(e) while keeping the existing Log.w calls (so logs remain context-specific); reference isAuthError, isConnectionError, manager.errorMessage, and the TapSignerEnterPinView catch blocks when making the change.ios/Cove/Flows/TapSignerFlow/TapSignerConfirmPinView.swift (1)
84-84: Consider extracting the duplicated "Connection Lost" alert into a helper /AppAlertStatecase.The exact same
(title: "Connection Lost", message: "Tag connection lost, please hold your phone still")literal is repeated here and three times inTapSignerEnterPinView.swift. Centralizing as e.g.AppAlertState.tapSignerConnectionLost(mirroring.tapSignerInvalidAuth/.tapSignerNoBackup) keeps wording consistent, simplifies future localization, and avoids drift if the message is updated.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ios/Cove/Flows/TapSignerFlow/TapSignerConfirmPinView.swift` at line 84, Extract the repeated alert literal into a single AppAlertState case (e.g. AppAlertState.tapSignerConnectionLost) and replace the inline initializers in TapSignerConfirmPinView (the if branch using error.isConnectionError()) and the three occurrences in TapSignerEnterPinView.swift with that new case; make sure the new AppAlertState case mirrors the pattern used by .tapSignerInvalidAuth/.tapSignerNoBackup so existing alert consumers (alertState = .init(...)) continue to work and localization strings are centralized.rust/src/tap_card.rs (1)
28-29: Stylistic: prefer a more descriptive payload than a free-formString.
ConnectionError(String)mirrorsUnknownError(String)but, likeCkTap(CkTapError), this is a closed concept — most call sites only ever pass the same hard-coded"Tag connection lost, please hold your phone still". Consider either a unit variant (ConnectionError) or an enum payload describing concrete sub-cases (TagLost,Timeout,Io(String)) so the message can live entirely in UI/localization, not in the transport error.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/src/tap_card.rs` around lines 28 - 29, The ConnectionError(String) enum variant should use a more structured payload: either convert it to a unit variant ConnectionError or replace its String payload with a small enum (e.g., ConnectionIssue with variants TagLost, Timeout, Io(String)) so callers can pass a discrete case instead of free-form text; update the Display/#[error(...)] formatting for the enum (and any match arms creating ConnectionError) to construct the new variant and adjust places that currently pass the hard-coded "Tag connection lost, please hold your phone still" message to use ConnectionError::TagLost (or the unit ConnectionError) and leave the human-readable message to the UI/localization layer; references: the enum that contains ConnectionError, the existing UnknownError(String) and CkTap(CkTapError) variants to mirror their patterns.ios/Cove/AppManager.swift (1)
41-42: Drop redundant per‑member@MainActorannotations.Once the class is annotated
@MainActor(line 7), all stored properties and instance methods inherit that isolation. The explicit@MainActoronisLoading(41),getWalletManager(98),getSendFlowManager(113),resetRoute(to:)(220) andloadAndReset(to:)(230) are now no‑ops and just add noise.Proposed cleanup
- `@MainActor` var isLoading = false- `@MainActor` public func getWalletManager(id: WalletId) throws -> WalletManager {- `@MainActor` public func getSendFlowManager(_ wm: WalletManager, presenter: SendFlowPresenter) -> SendFlowManager {- `@MainActor` func resetRoute(to routes: [Route]) {- `@MainActor` func loadAndReset(to route: Route) {Also applies to: 98-99, 113-114, 220-221, 230-231
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ios/Cove/AppManager.swift` around lines 41 - 42, The class is already annotated `@MainActor`, so remove the redundant per-member `@MainActor` annotations on the stored property and methods to reduce noise: delete the `@MainActor` before the isLoading property and the `@MainActor` annotations on the methods getWalletManager, getSendFlowManager, resetRoute(to:), and loadAndReset(to:), leaving the class-level `@MainActor` intact so all members remain main-isolated.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ios/Cove/AppManager.swift`:
- Line 8: Remove the trailing whitespace after the attribute declaration so
`@Observable ` becomes `@Observable`; edit the `AppManager.swift` file to delete
the extra space following the `@Observable` attribute (ensure there are no other
trailing spaces on that line) and save/run swiftformat to confirm the warning is
cleared.
- Around line 7-9: AppManager is annotated `@MainActor` which makes its methods
actor-isolated, but FfiReconcile's callbacks (reconcile(message:) and
dispatch(action:)) are invoked from non-Swift/FFI contexts; mark both methods as
nonisolated so they can be called from any thread and keep the existing explicit
MainActor dispatch inside their bodies (e.g., continue using
DispatchQueue.main.async or Task { `@MainActor` ... } within reconcile(message:)
and dispatch(action:) to run UI/main-thread work). Locate the implementations of
reconcile(message:) and dispatch(action:) in AppManager and add the nonisolated
modifier to each method signature while preserving the current main-thread
dispatch logic.
In `@ios/Cove/ImportWalletManager.swift`:
- Line 4: Remove the trailing whitespace after the `@Observable` attribute in
ImportWalletManager.swift so that the annotation is exactly "@Observable" with
no extra spaces; update the declaration that uses the `@Observable` attribute (the
annotation line in the ImportWalletManager class/file) to eliminate the trailing
space so swiftformat no longer flags it.
In `@ios/Cove/WalletManager.swift`:
- Line 6: Remove the trailing whitespace after the `@Observable` attribute in
WalletManager.swift: edit the line containing "@Observable " to be "@Observable"
(no trailing space), save and re-run swiftformat/lint to confirm the warning is
cleared; ensure no other lines in the file have trailing spaces.
- Around line 5-7: WalletManager is `@MainActor` but must implement nonisolated
reconcile and reconcileMany from AnyReconciler/WalletManagerReconciler; mark the
instance methods reconcile(...) and reconcileMany(...) as nonisolated, remove
the redundant DispatchQueue.main.async wrappers inside those methods (since
they'll be called from background threads), and ensure the `@Observable`
annotation has no trailing whitespace on the class declaration line.
---
Outside diff comments:
In `@ios/Cove/Flows/TapSignerFlow/TapSignerEnterPinView.swift`:
- Around line 131-142: The connection-error branch in the sign failure handler
in TapSignerEnterPinView.swift is inconsistent: add app.sheetState = .none
inside the else-if error.isConnectionError() branch (the same place where the
generic failure sets sheetState = .none) so the NFC sheet is dismissed on fatal
sign failures; then remove the duplicated literal by introducing a reusable
alert case (e.g. AppAlertState.tapSignerConnectionLost) and replace the
hard-coded .general(title: "Connection Lost", message: ...) occurrences in
TapSignerEnterPinView and TapSignerConfirmPinView with that new AppAlertState
constant to centralize the message.
In `@rust/src/tap_card.rs`:
- Around line 105-118: The TransportError::ConnectionError variant is being lost
when converted into ApduError in the From<TransportError> for ApduError impl
(currently mapping ConnectionError to ApduError::CkTap(BadArguments)), so
is_connection_error() can never detect platform connection failures; fix by
adding a ConnectionError variant to the internal ApduError enum (or equivalent
bridge type) and update the From<TransportError> for ApduError conversion to map
TransportError::ConnectionError(_) -> ApduError::ConnectionError(...), and
ensure the reverse conversion (From<ApduError> for TransportError or the path in
TapcardTransport::transmit_apdu) maps ApduError::ConnectionError back to
TransportError::ConnectionError so the round-trip preserves the error; update
any matching sites like is_connection_error() to match the new
ApduError::ConnectionError where appropriate.
---
Nitpick comments:
In
`@android/app/src/main/java/org/bitcoinppl/cove/flows/TapSignerFlow/TapSignerEnterPinView.kt`:
- Around line 252-260: Extract the repeated error-classification logic in
TapSignerEnterPinView into a small helper (e.g., private fun
classifyTapSignerError(e: Exception): String) that returns the appropriate
message based on isAuthError(e) / isConnectionError(e) / else, then replace each
catch-site’s manager.errorMessage assignment with manager.errorMessage =
classifyTapSignerError(e) while keeping the existing Log.w calls (so logs remain
context-specific); reference isAuthError, isConnectionError,
manager.errorMessage, and the TapSignerEnterPinView catch blocks when making the
change.
In `@ios/Cove/AppManager.swift`:
- Around line 41-42: The class is already annotated `@MainActor`, so remove the
redundant per-member `@MainActor` annotations on the stored property and methods
to reduce noise: delete the `@MainActor` before the isLoading property and the
`@MainActor` annotations on the methods getWalletManager, getSendFlowManager,
resetRoute(to:), and loadAndReset(to:), leaving the class-level `@MainActor`
intact so all members remain main-isolated.
In `@ios/Cove/Flows/TapSignerFlow/TapSignerConfirmPinView.swift`:
- Line 84: Extract the repeated alert literal into a single AppAlertState case
(e.g. AppAlertState.tapSignerConnectionLost) and replace the inline initializers
in TapSignerConfirmPinView (the if branch using error.isConnectionError()) and
the three occurrences in TapSignerEnterPinView.swift with that new case; make
sure the new AppAlertState case mirrors the pattern used by
.tapSignerInvalidAuth/.tapSignerNoBackup so existing alert consumers (alertState
= .init(...)) continue to work and localization strings are centralized.
In `@rust/src/tap_card.rs`:
- Around line 28-29: The ConnectionError(String) enum variant should use a more
structured payload: either convert it to a unit variant ConnectionError or
replace its String payload with a small enum (e.g., ConnectionIssue with
variants TagLost, Timeout, Io(String)) so callers can pass a discrete case
instead of free-form text; update the Display/#[error(...)] formatting for the
enum (and any match arms creating ConnectionError) to construct the new variant
and adjust places that currently pass the hard-coded "Tag connection lost,
please hold your phone still" message to use ConnectionError::TagLost (or the
unit ConnectionError) and leave the human-readable message to the
UI/localization layer; references: the enum that contains ConnectionError, the
existing UnknownError(String) and CkTap(CkTapError) variants to mirror their
patterns.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4061e850-50ae-4a1d-81d6-d4e3a601d8c8
⛔ Files ignored due to path filters (1)
ios/CoveCore/Sources/CoveCore/generated/cove.swiftis excluded by!**/generated/**
📒 Files selected for processing (13)
android/app/src/main/java/org/bitcoinppl/cove/flows/TapSignerFlow/TapSignerEnterPinView.ktandroid/app/src/main/java/org/bitcoinppl/cove/nfc/TapCardNfcManager.ktandroid/app/src/main/java/org/bitcoinppl/cove_core/cove.ktios/Cove/AppManager.swiftios/Cove/Flows/TapSignerFlow/TapSignerConfirmPinView.swiftios/Cove/Flows/TapSignerFlow/TapSignerEnterPinView.swiftios/Cove/ImportWalletManager.swiftios/Cove/Security.swiftios/Cove/ShareSheet.swiftios/Cove/TapSignerNFC.swiftios/Cove/WalletManager.swiftrust/src/tap_card.rsrust/src/tap_card/tap_signer_reader.rs
✅ Files skipped from review due to trivial changes (1)
- ios/Cove/ShareSheet.swift
🚧 Files skipped from review as they are similar to previous changes (2)
- rust/src/tap_card/tap_signer_reader.rs
- ios/Cove/TapSignerNFC.swift
| @MainActor | ||
| @Observable | ||
| final class AppManager: FfiReconcile { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find FfiReconcile protocol declaration and call sites that may now cross actor boundaries.
rg -nP '\bprotocol\s+FfiReconcile\b' --type=swift -C5
rg -nP '\bAppManager\.shared\.(dispatch|pushRoute|popRoute|setRoute|resetRoute|reset|reloadWallets|selectWallet|loadWallets|toggleSidebar)\b' --type=swift -C2
rg -nP '\b\.reconcile\s*\(' --type=swift -C2Repository: bitcoinppl/cove
Length of output: 6326
🏁 Script executed:
# Get the full FfiReconcile protocol definition with isolation attributes
rg -A 20 'public protocol FfiReconcile' --type=swift ios/CoveCore/Sources/CoveCore/generated/cove.swiftRepository: bitcoinppl/cove
Length of output: 827
🏁 Script executed:
# Get AppManager.reconcile() and dispatch(action:) implementations
cat -n ios/Cove/AppManager.swift | head -350 | tail -150Repository: bitcoinppl/cove
Length of output: 5335
🏁 Script executed:
# Check the context of AuthManager.reset() call
cat -n ios/Cove/AuthManager.swift | sed -n '115,130p'Repository: bitcoinppl/cove
Length of output: 443
🏁 Script executed:
# Get broader context around AuthManager.reset() call
cat -n ios/Cove/AuthManager.swift | sed -n '100,135p'Repository: bitcoinppl/cove
Length of output: 1205
🏁 Script executed:
# Find all dispatch(action:) calls to check their context
rg -n 'dispatch\(action:' --type=swift -B 3 -A 1Repository: bitcoinppl/cove
Length of output: 50371
🏁 Script executed:
# Check for any Swift compilation errors in the project
find . -name "*.swift" -type f | head -5Repository: bitcoinppl/cove
Length of output: 276
🏁 Script executed:
# Search for nonisolated declarations in AppManager or similar patterns
rg -n 'nonisolated func (reconcile|dispatch)' --type=swiftRepository: bitcoinppl/cove
Length of output: 41
🏁 Script executed:
# Check if there are any `@unchecked` Sendable or actor isolation suppression patterns
rg -n '@unchecked|nonisolated|-suppress' --type=swift ios/Cove/AppManager.swiftRepository: bitcoinppl/cove
Length of output: 41
🏁 Script executed:
# Check AppManager.swift around the reconcile and dispatch methods for any actor isolation issues
cat -n ios/Cove/AppManager.swift | sed -n '238,245p'Repository: bitcoinppl/cove
Length of output: 348
🏁 Script executed:
# Look for any build configuration or Swift settings that enable strict concurrency
fd -i 'build.settings|swiftsettings|xcconfig' --type f | head -10Repository: bitcoinppl/cove
Length of output: 41
🏁 Script executed:
# Check if there are any generated error/warning indicators in the codebase
rg -i 'error:|warning:|fixme' ios/Cove/AppManager.swift | head -20Repository: bitcoinppl/cove
Length of output: 131
Mark reconcile(message:) and dispatch(action:) as nonisolated.
The class-level @MainActor annotation makes both methods main-actor-isolated, but FfiReconcile protocol declares reconcile(message:) with no isolation attributes. Since Rust FFI callbacks invoke this method from non-Swift contexts (potentially from background threads), a main-actor-isolated implementation would violate actor boundaries at runtime. The existing DispatchQueue.main.async wrapper in the reconcile() implementation confirms this was a known concern. Both protocol-conforming methods (reconcile and dispatch) should be marked nonisolated to allow FFI entry from any context, with the main actor dispatch handled explicitly in the method body as already done.
🧰 Tools
🪛 GitHub Check: swiftformat
[warning] 8-8:
Remove trailing space at end of a line. (trailingSpace)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ios/Cove/AppManager.swift` around lines 7 - 9, AppManager is annotated
`@MainActor` which makes its methods actor-isolated, but FfiReconcile's callbacks
(reconcile(message:) and dispatch(action:)) are invoked from non-Swift/FFI
contexts; mark both methods as nonisolated so they can be called from any thread
and keep the existing explicit MainActor dispatch inside their bodies (e.g.,
continue using DispatchQueue.main.async or Task { `@MainActor` ... } within
reconcile(message:) and dispatch(action:) to run UI/main-thread work). Locate
the implementations of reconcile(message:) and dispatch(action:) in AppManager
and add the nonisolated modifier to each method signature while preserving the
current main-thread dispatch logic.
|
|
||
| @Observable final class AppManager: FfiReconcile { | ||
| @MainActor | ||
| @Observable |
There was a problem hiding this comment.
Trim trailing whitespace.
swiftformat is flagging a trailing space after @Observable.
Proposed fix
-@Observable
+@Observable📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @Observable | |
| `@Observable` |
🧰 Tools
🪛 GitHub Check: swiftformat
[warning] 8-8:
Remove trailing space at end of a line. (trailingSpace)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ios/Cove/AppManager.swift` at line 8, Remove the trailing whitespace after
the attribute declaration so `@Observable ` becomes `@Observable`; edit the
`AppManager.swift` file to delete the extra space following the `@Observable`
attribute (ensure there are no other trailing spaces on that line) and save/run
swiftformat to confirm the warning is cleared.
|
|
||
| @Observable class ImportWalletManager: ImportWalletManagerReconciler { | ||
| @MainActor | ||
| @Observable |
There was a problem hiding this comment.
Trim trailing whitespace.
swiftformat is flagging a trailing space after @Observable.
Proposed fix
-@Observable
+@Observable📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @Observable | |
| `@Observable` |
🧰 Tools
🪛 GitHub Check: swiftformat
[warning] 4-4:
Remove trailing space at end of a line. (trailingSpace)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ios/Cove/ImportWalletManager.swift` at line 4, Remove the trailing whitespace
after the `@Observable` attribute in ImportWalletManager.swift so that the
annotation is exactly "@Observable" with no extra spaces; update the declaration
that uses the `@Observable` attribute (the annotation line in the
ImportWalletManager class/file) to eliminate the trailing space so swiftformat
no longer flags it.
| @MainActor | ||
| @Observable | ||
| final class WalletManager: AnyReconciler, WalletManagerReconciler { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect generated UniFFI bindings & related protocols for actor isolation expectations.
fd -t f 'WalletManagerReconciler|wallet_manager' ios/CoveCore | head -50
rg -nP '\b(protocol|func)\s+(WalletManagerReconciler|reconcile|reconcileMany|dispatch)\b' --type=swift -C2
rg -nP '\bAnyReconciler\b' --type=swift -C3
rg -nP '\bWeakReconciler\b' --type=swift -C3Repository: bitcoinppl/cove
Length of output: 34306
🏁 Script executed:
cat -n ios/Cove/WalletManager.swift | head -280 | tail -250Repository: bitcoinppl/cove
Length of output: 9241
🏁 Script executed:
# Check for trailing whitespace on line 6 specifically
sed -n '6p' ios/Cove/WalletManager.swift | od -c | grep -E '(space|tab)$' && echo "Trailing whitespace found" || echo "No trailing whitespace"Repository: bitcoinppl/cove
Length of output: 141
🏁 Script executed:
# Check if reconcile/reconcileMany methods have nonisolated marker
rg -A 5 'func reconcile\(message:|func reconcileMany\(messages:' ios/Cove/WalletManager.swiftRepository: bitcoinppl/cove
Length of output: 542
🏁 Script executed:
# Check line 6 directly with hexdump alternative
sed -n '6p' ios/Cove/WalletManager.swift | cat -ARepository: bitcoinppl/cove
Length of output: 73
🏁 Script executed:
# Search for any recent fixes or build-related comments
rg -i 'mainactor|nonisolated|compilation|doesnt compile|actor isolation' ios/Cove/WalletManager.swiftRepository: bitcoinppl/cove
Length of output: 100
🏁 Script executed:
# Check if there are any other managers with `@MainActor` to compare pattern
rg '@MainActor' ios/Cove/ --type swift | grep -E 'class|struct'Repository: bitcoinppl/cove
Length of output: 41
🏁 Script executed:
# Look at other reconciler implementations to see if they use nonisolated
rg -B2 -A8 'func reconcile\(message:' ios/Cove/*.swift | grep -E '(nonisolated|@MainActor|class )' -A1 -B1Repository: bitcoinppl/cove
Length of output: 41
🏁 Script executed:
# Look for any PR or branch information, recent commits
git log --oneline -20 ios/Cove/WalletManager.swiftRepository: bitcoinppl/cove
Length of output: 148
🏁 Script executed:
# Check if `@MainActor` is recently added (compare with git)
git show HEAD:ios/Cove/WalletManager.swift | head -10Repository: bitcoinppl/cove
Length of output: 353
🏁 Script executed:
# Verify current full signature of reconcile to be absolutely certain
sed -n '238,244p' ios/Cove/WalletManager.swiftRepository: bitcoinppl/cove
Length of output: 288
🏁 Script executed:
# Check generated protocol definition more carefully to see isolation context
rg -A 10 'public protocol WalletManagerReconciler' ios/CoveCore/Sources/CoveCore/generated/cove.swift | head -20Repository: bitcoinppl/cove
Length of output: 418
🏁 Script executed:
# Final check: does WalletManager or other managers actually have nonisolated on reconcile?
rg 'nonisolated.*func reconcile' ios/Cove/ --type swiftRepository: bitcoinppl/cove
Length of output: 41
🏁 Script executed:
# Check other managers to see if they have `@MainActor`
rg -B2 'class.*AnyReconciler.*Reconciler {' ios/Cove/ --type swift | grep '@MainActor' | wc -lRepository: bitcoinppl/cove
Length of output: 211
🏁 Script executed:
# Verify the exact line 6 content with hex
sed -n '6p' ios/Cove/WalletManager.swift | xxdRepository: bitcoinppl/cove
Length of output: 103
🏁 Script executed:
# Check if the repo has recent actor-related changes
git log --oneline --all -30 | grep -i 'actor\|mainactor\|isolation'Repository: bitcoinppl/cove
Length of output: 41
🏁 Script executed:
# Verify trailing space on line 6 using sed and wc
sed -n '6p' ios/Cove/WalletManager.swift | wc -c
# Compare with line 5 (should have no trailing space)
sed -n '5p' ios/Cove/WalletManager.swift | wc -cRepository: bitcoinppl/cove
Length of output: 65
🏁 Script executed:
# Check what line 6 actually contains by looking at raw bytes
sed -n '6p' ios/Cove/WalletManager.swift | od -An -tx1Repository: bitcoinppl/cove
Length of output: 102
Add nonisolated to reconcile/reconcileMany methods to satisfy protocol conformance under @MainActor isolation.
WalletManager is marked @MainActor, making all instance methods main-actor isolated. However, both AnyReconciler and the generated WalletManagerReconciler protocol declare reconcile and reconcileMany as non-isolated. The class conformance won't compile unless these specific methods are marked nonisolated.
The internal DispatchQueue.main.async wrapping becomes redundant once methods are nonisolated, since they're called from Rust on a background thread; removing it and marking the methods nonisolated is the idiomatic pattern.
Also fix trailing whitespace on line 6 (@Observable).
🧰 Tools
🪛 GitHub Check: swiftformat
[warning] 6-6:
Remove trailing space at end of a line. (trailingSpace)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ios/Cove/WalletManager.swift` around lines 5 - 7, WalletManager is `@MainActor`
but must implement nonisolated reconcile and reconcileMany from
AnyReconciler/WalletManagerReconciler; mark the instance methods reconcile(...)
and reconcileMany(...) as nonisolated, remove the redundant
DispatchQueue.main.async wrappers inside those methods (since they'll be called
from background threads), and ensure the `@Observable` annotation has no trailing
whitespace on the class declaration line.
|
|
||
| @Observable final class WalletManager: AnyReconciler, WalletManagerReconciler { | ||
| @MainActor | ||
| @Observable |
There was a problem hiding this comment.
Trim trailing whitespace.
swiftformat is flagging a trailing space after @Observable.
Proposed fix
-@Observable
+@Observable📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @Observable | |
| `@Observable` |
🧰 Tools
🪛 GitHub Check: swiftformat
[warning] 6-6:
Remove trailing space at end of a line. (trailingSpace)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ios/Cove/WalletManager.swift` at line 6, Remove the trailing whitespace after
the `@Observable` attribute in WalletManager.swift: edit the line containing
"@Observable " to be "@Observable" (no trailing space), save and re-run
swiftformat/lint to confirm the warning is cleared; ensure no other lines in the
file have trailing spaces.
3 participants
Summary
Replace string-based connection error checking with type-safe error handling across Rust, iOS, and Android platforms.
fixes #439
Testing
Platform Coverage
Checklist
Summary by CodeRabbit