Refactor and navigation fix#720
Merged
praveenperera merged 19 commits intomasterfrom Apr 29, 2026
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughNavigation is now generation-aware: AppManager coordinates navigation with a monotonic generation token to gate/cancel delayed sidebar and load-and-reset navigations. Many call sites were changed to use AppManager wallet-selection helpers that throw on failure. Rust cloud-backup and passkey handling received substantial refactors and safer error mapping. Changes
Sequence DiagramsequenceDiagram
participant UI as UI Component
participant AppMgr as AppManager
participant Nav as Router
participant Rust as Rust Backend
UI->>AppMgr: selectWallet(id) / resetRoute(...)
activate AppMgr
AppMgr->>AppMgr: beginNavigationIntent() (inc generation)
AppMgr->>Nav: record navigation intent / update router
AppMgr->>Rust: call FFI select/reset
deactivate AppMgr
UI->>AppMgr: closeSidebarAndNavigate(route)
activate AppMgr
AppMgr->>AppMgr: cancel pendingSidebarNavigationTask
AppMgr->>AppMgr: capture generation g0
AppMgr->>AppMgr: sleep 200ms / loadingTimeMs
AppMgr->>AppMgr: if current generation == g0 then
alt generation matches
AppMgr->>Rust: resetAfterLoading(nextRoutes)
AppMgr->>Nav: navigate to nextRoutes
else generation advanced
AppMgr->>AppMgr: skip (stale)
end
deactivate AppMgr
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (2 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 |
Greptile SummaryThis PR introduces a navigation-generation counter on both iOS and Android to prevent stale delayed sidebar navigations from executing after a newer navigation event has already been dispatched. On the Rust side, wallet selection is unified behind Confidence Score: 5/5Safe to merge — all findings are P2 dead-code clarity nits with no runtime impact. No P0 or P1 issues found. The navigation generation pattern is correctly implemented on both platforms, error propagation from dispatch is handled at every call site, and the multi-namespace restore path has corresponding tests. Only two unreachable code branches were identified, both harmless. rust/src/manager/cloud_backup_manager/ops.rs (unreachable else branch in enable flow) Important Files Changed
Sequence DiagramsequenceDiagram
participant UI as SidebarView
participant AM as AppManager
participant Rust as FfiApp (Rust)
UI->>AM: closeSidebarAndSelectWallet(id)
AM->>AM: cancel pendingSidebarNavigationJob
AM->>AM: advanceNavigationGeneration() → gen=N
AM->>AM: isSidebarVisible = false
AM->>AM: launch coroutine (delay 250ms)
alt Another navigation fires before delay expires
UI->>AM: pushRoute / popRoute / etc.
AM->>AM: advanceNavigationGeneration() → gen=N+1
Note over AM: delayed job wakes up
AM->>AM: isNavigationGenerationCurrent(N)? → false → return
else No intervening navigation
Note over AM: delayed job wakes up
AM->>AM: isNavigationGenerationCurrent(N)? → true
AM->>Rust: dispatch(AppAction.SelectWallet(id))
Rust-->>AM: Ok(()) or Err → logged
end
Reviews (2): Last reviewed commit: "Handle unexpected wallet creation errors" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 (3)
ios/Cove/Flows/NewWalletFlow/NewWalletSelectScreen.swift (1)
169-177:⚠️ Potential issue | 🟡 MinorSet the success alert only after selection succeeds.
Line 174 queues
.importedSuccessfullybefore the throwing call on Line 175. If selection fails, the local error alert is shown but the app-wide success alert is still pending, so users can get a contradictory success dialog afterward.💡 Suggested change
- app.alertState = TaggedItem(.importedSuccessfully) try app.selectWalletOrThrow(id) + app.alertState = TaggedItem(.importedSuccessfully)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ios/Cove/Flows/NewWalletFlow/NewWalletSelectScreen.swift` around lines 169 - 177, In newWalletFromXpub, the code sets app.alertState = TaggedItem(.importedSuccessfully) before calling try app.selectWalletOrThrow(id), which can leave a success alert queued even if selection fails; move the app.alertState assignment so it runs only after try app.selectWalletOrThrow(id) completes without throwing (i.e., call Wallet.newFromXpub(xpub:), get id, call try app.selectWalletOrThrow(id), then set app.alertState = TaggedItem(.importedSuccessfully)); keep the catch block unchanged to present the local error AlertItem on failure.android/app/src/main/java/org/bitcoinppl/cove/MainActivity.kt (1)
821-828:⚠️ Potential issue | 🟡 MinorSurface TapSigner wallet-selection failures to the user.
This branch dismisses the dialog before
selectWalletOrThrow(...)and only logs on failure, so the user gets no feedback if selection fails. The duplicate-wallet path already raisesUnableToSelectWallet; this one should stay consistent.💡 Suggested change
} catch (e: Exception) { Log.e("GlobalAlert", "Failed to select wallet", e) + app.alertState = TaggedItem(AppAlertState.UnableToSelectWallet) }🤖 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/MainActivity.kt` around lines 821 - 828, Move the onDismiss() call to after a successful wallet selection and propagate failures to the UI instead of only logging: call app.selectWalletOrThrow(state.walletId) inside the try block before calling onDismiss() and app.resetRoute(Route.SelectedWallet(state.walletId)), and in the catch block rethrow or surface the same UnableToSelectWallet error (or invoke the same user-visible error path used by the duplicate-wallet branch) instead of only Log.e in the TextButton onClick handler so the user sees the selection failure.ios/Cove/Flows/NewWalletFlow/ColdWallet/QrCodeImportScreen.swift (1)
154-169:⚠️ Potential issue | 🟡 MinorDon't queue
.importedSuccessfullybefore the throwing select.Line 159 sets the app-wide success alert before Line 160 can fail. If selection throws, the local error alert is shown, but the global success alert is still queued and can appear afterward on a later screen.
💡 Suggested change
- app.alertState = TaggedItem(.importedSuccessfully) try app.selectWalletOrThrow(id) + app.alertState = TaggedItem(.importedSuccessfully)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ios/Cove/Flows/NewWalletFlow/ColdWallet/QrCodeImportScreen.swift` around lines 154 - 169, The code sets app.alertState = TaggedItem(.importedSuccessfully) before calling app.selectWalletOrThrow(id), which can throw and leave the success alert queued; change the flow in the import completion block so you first compute let id = wallet.id(), then if let onImported { onImported(id) } else try app.selectWalletOrThrow(id) and only after a successful select set app.alertState = TaggedItem(.importedSuccessfully); keep existing catch handling (WalletError.MultiFormat, WalletError.WalletAlreadyExists -> handleWalletAlreadyExists(id), and the generic catch that sets alert) unchanged so the success alert is never queued if selectWalletOrThrow throws.
🧹 Nitpick comments (3)
rust/src/manager/cloud_backup_manager/verify/session.rs (1)
113-115: Add a regression test for this new retry branch.This new fallback path is important; a focused test would prevent future regressions back to panic-style handling.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/src/manager/cloud_backup_manager/verify/session.rs` around lines 113 - 115, Add a focused regression test that exercises the new MasterKeyResolution::NeedsWrapperRepair branch to ensure it returns the retry_result instead of panicking; specifically, create a test in the same module that sets up a VerifySession (or the function that calls the match on MasterKeyResolution in session.rs) and injects/mocks the master key resolution to return MasterKeyResolution::NeedsWrapperRepair, then call the verify path and assert the returned value equals the result of self.retry_result("cloud master key wrapper still needs repair") (or that it is a Retry outcome) and that no panic occurs; refer to the MasterKeyResolution enum, the retry_result method, and the verify/session entrypoint to locate where to hook the mock.android/app/src/main/java/org/bitcoinppl/cove/flows/SelectedWalletFlow/TransactionsCardView.kt (1)
301-325: Extract shared BTC/FIAT amount formatting to one helper.Both branches now carry near-identical formatting code; centralizing it will reduce drift and make future display changes safer.
Refactor sketch
+private fun WalletManager.formatTxnAmounts( + fiatOrBtc: FiatOrBtc, + sentAndReceived: SentAndReceived, + fiatAmount: FiatAmount?, +): Pair<String, String> { + val direction = sentAndReceived.direction() + val btc = rust.displaySentAndReceivedAmount(sentAndReceived) + val fiat = fiatAmount?.let { rust.displayFiatAmountWithDirection(it.amount, direction) } ?: "---" + return if (fiatOrBtc == FiatOrBtc.BTC) btc to fiat else fiat to btc +}Also applies to: 363-387
🤖 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/SelectedWalletFlow/TransactionsCardView.kt` around lines 301 - 325, The BTC/FIAT formatting logic duplicated between formattedAmount and secondaryAmount should be moved into a single helper to avoid drift; create a private function (e.g., formatAmountFor(fiatOrBtc: FiatOrBtc, txn: TransactionWrapper, direction: Direction, manager: Manager)) that encapsulates the when-switch and the null-check for txn.v1.fiatAmount(), calling manager.rust.displaySentAndReceivedAmount(txn.v1.sentAndReceived()) for BTC and manager.rust.displayFiatAmountWithDirection(fiatAmount.amount, direction) or "---" when fiatAmount is null for FIAT, then replace both computed properties formattedAmount and secondaryAmount to call this helper (also apply the same refactor to the similar block referenced at lines 363-387).android/app/src/main/java/org/bitcoinppl/cove/RouteView.kt (1)
77-98: Derive the reset payload fromrouteinside the container.Now that
LoadAndResetContaineralready takes the fullRoute.LoadAndReset, passingnextRoutesandloadingTimeMsseparately gives this code two sources of truth for the same state. Computing them inside the container will make future edits harder to desync.♻️ Suggested simplification
is Route.LoadAndReset -> { LoadAndResetContainer( app = app, - route = route, - nextRoutes = route.resetTo.map { it.route() }, - loadingTimeMs = route.afterMillis.toLong(), + route = route, ) } @@ private fun LoadAndResetContainer( app: AppManager, route: Route.LoadAndReset, - nextRoutes: List<Route>, - loadingTimeMs: Long, ) { + val nextRoutes = route.resetTo.map { it.route() } + val loadingTimeMs = route.afterMillis.toLong() + // show loading indicator Box(modifier = Modifier.fillMaxSize(), contentAlignment = Alignment.Center) { CircularProgressIndicator() }🤖 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/RouteView.kt` around lines 77 - 98, The container currently accepts nextRoutes and loadingTimeMs separately which duplicates state from the Route.LoadAndReset; change LoadAndResetContainer to derive those values from the passed-in route instead: inside LoadAndResetContainer compute nextRoutes = route.resetTo.map { it.route() } and loadingTimeMs = route.afterMillis.toLong(), remove the nextRoutes and loadingTimeMs parameters from the LoadAndResetContainer signature, and update the caller in the Route.LoadAndReset branch to only pass app and route (remove nextRoutes and loadingTimeMs there as well).
🤖 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/flows/NewWalletFlow/hot_wallet/HotWalletImportScreen.kt`:
- Around line 277-282: The code currently calls
app.selectWalletOrThrow(walletMetadata.id) before checking onImported and then
again in the fallback branch, causing duplicated side effects; change the flow
so selection happens exactly once: remove the pre-branch app.selectWalletOrThrow
call, invoke onImported?.invoke(walletMetadata.id) if present, otherwise call
app.resetRoute(Route.SelectedWallet(walletMetadata.id)), and then call
app.selectWalletOrThrow(walletMetadata.id) once after the branching (or place
the single select call into both branches identically) to match the
duplicate-wallet handling and avoid double selection; update references around
manager.importWallet, onImported, app.resetRoute, app.clearWalletManager
accordingly.
In `@ios/Cove/AppManager.swift`:
- Around line 261-279: closeSidebarAndNavigate spawns a delayed Task stored in
pendingSidebarNavigationTask which can run after reset(); ensure reset()
cancels/invalidates this delayed navigation by calling
pendingSidebarNavigationTask?.cancel() and setting pendingSidebarNavigationTask
= nil (or otherwise invalidating the navigation generation) inside the reset()
implementation. Update the AppManager.reset() method to perform that
cancellation and/or bump the navigation generation
(beginNavigationIntent/isNavigationGenerationCurrent) so any queued sidebar
navigation cannot resurrect stale routing after a full reset.
In `@ios/Cove/Flows/NewWalletFlow/HotWallet/HotWalletImportScreen.swift`:
- Around line 516-517: The code is swallowing selection errors by using `try?`
before calling `app.resetRoute(to: .selectedWallet(walletId))`; replace this
with proper error handling so the route is only reset when
`app.selectWalletOrThrow(walletId)` succeeds — for example, wrap the call in a
do-catch (or propagate the error) and in the do-block call `try
app.selectWalletOrThrow(walletId)` followed by `app.resetRoute(to:
.selectedWallet(walletId))`, and in the catch block handle or surface the error
instead of proceeding with navigation.
In `@ios/Cove/LoadAndResetContainer.swift`:
- Around line 17-25: Change the unkeyed SwiftUI task in LoadAndResetContainer to
a keyed task so it cancels when the route changes: replace the current .task
usage (which calls app.captureLoadAndResetGeneration() and later await
app.resetAfterLoadingIfCurrent(generation:route:nextRoute:)) with .task(id:
route) so the task is tied to the route value and will be cancelled/restarted on
route changes, preventing concurrent task instances for different routes.
In `@rust/src/qr_scanner.rs`:
- Line 349: The early return in the Self::Complete arm loses the cached state
because std::mem::replace(self, Self::Uninitialized) was already performed; fix
by restoring the scanner state before returning: after extracting result from
the match arm (the Self::Complete(result) branch), assign *self =
Self::Complete(result) (or avoid replacing until after matching) so the cached
completion is preserved instead of leaving self as Self::Uninitialized.
---
Outside diff comments:
In `@android/app/src/main/java/org/bitcoinppl/cove/MainActivity.kt`:
- Around line 821-828: Move the onDismiss() call to after a successful wallet
selection and propagate failures to the UI instead of only logging: call
app.selectWalletOrThrow(state.walletId) inside the try block before calling
onDismiss() and app.resetRoute(Route.SelectedWallet(state.walletId)), and in the
catch block rethrow or surface the same UnableToSelectWallet error (or invoke
the same user-visible error path used by the duplicate-wallet branch) instead of
only Log.e in the TextButton onClick handler so the user sees the selection
failure.
In `@ios/Cove/Flows/NewWalletFlow/ColdWallet/QrCodeImportScreen.swift`:
- Around line 154-169: The code sets app.alertState =
TaggedItem(.importedSuccessfully) before calling app.selectWalletOrThrow(id),
which can throw and leave the success alert queued; change the flow in the
import completion block so you first compute let id = wallet.id(), then if let
onImported { onImported(id) } else try app.selectWalletOrThrow(id) and only
after a successful select set app.alertState =
TaggedItem(.importedSuccessfully); keep existing catch handling
(WalletError.MultiFormat, WalletError.WalletAlreadyExists ->
handleWalletAlreadyExists(id), and the generic catch that sets alert) unchanged
so the success alert is never queued if selectWalletOrThrow throws.
In `@ios/Cove/Flows/NewWalletFlow/NewWalletSelectScreen.swift`:
- Around line 169-177: In newWalletFromXpub, the code sets app.alertState =
TaggedItem(.importedSuccessfully) before calling try
app.selectWalletOrThrow(id), which can leave a success alert queued even if
selection fails; move the app.alertState assignment so it runs only after try
app.selectWalletOrThrow(id) completes without throwing (i.e., call
Wallet.newFromXpub(xpub:), get id, call try app.selectWalletOrThrow(id), then
set app.alertState = TaggedItem(.importedSuccessfully)); keep the catch block
unchanged to present the local error AlertItem on failure.
---
Nitpick comments:
In
`@android/app/src/main/java/org/bitcoinppl/cove/flows/SelectedWalletFlow/TransactionsCardView.kt`:
- Around line 301-325: The BTC/FIAT formatting logic duplicated between
formattedAmount and secondaryAmount should be moved into a single helper to
avoid drift; create a private function (e.g., formatAmountFor(fiatOrBtc:
FiatOrBtc, txn: TransactionWrapper, direction: Direction, manager: Manager))
that encapsulates the when-switch and the null-check for txn.v1.fiatAmount(),
calling manager.rust.displaySentAndReceivedAmount(txn.v1.sentAndReceived()) for
BTC and manager.rust.displayFiatAmountWithDirection(fiatAmount.amount,
direction) or "---" when fiatAmount is null for FIAT, then replace both computed
properties formattedAmount and secondaryAmount to call this helper (also apply
the same refactor to the similar block referenced at lines 363-387).
In `@android/app/src/main/java/org/bitcoinppl/cove/RouteView.kt`:
- Around line 77-98: The container currently accepts nextRoutes and
loadingTimeMs separately which duplicates state from the Route.LoadAndReset;
change LoadAndResetContainer to derive those values from the passed-in route
instead: inside LoadAndResetContainer compute nextRoutes = route.resetTo.map {
it.route() } and loadingTimeMs = route.afterMillis.toLong(), remove the
nextRoutes and loadingTimeMs parameters from the LoadAndResetContainer
signature, and update the caller in the Route.LoadAndReset branch to only pass
app and route (remove nextRoutes and loadingTimeMs there as well).
In `@rust/src/manager/cloud_backup_manager/verify/session.rs`:
- Around line 113-115: Add a focused regression test that exercises the new
MasterKeyResolution::NeedsWrapperRepair branch to ensure it returns the
retry_result instead of panicking; specifically, create a test in the same
module that sets up a VerifySession (or the function that calls the match on
MasterKeyResolution in session.rs) and injects/mocks the master key resolution
to return MasterKeyResolution::NeedsWrapperRepair, then call the verify path and
assert the returned value equals the result of self.retry_result("cloud master
key wrapper still needs repair") (or that it is a Retry outcome) and that no
panic occurs; refer to the MasterKeyResolution enum, the retry_result method,
and the verify/session entrypoint to locate where to hook the mock.
🪄 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: 04010720-2b12-43e2-82ac-a726708a764e
📒 Files selected for processing (40)
android/app/src/main/java/org/bitcoinppl/cove/AppManager.ktandroid/app/src/main/java/org/bitcoinppl/cove/AuthManager.ktandroid/app/src/main/java/org/bitcoinppl/cove/MainActivity.ktandroid/app/src/main/java/org/bitcoinppl/cove/RouteView.ktandroid/app/src/main/java/org/bitcoinppl/cove/ScanManager.ktandroid/app/src/main/java/org/bitcoinppl/cove/flows/NewWalletFlow/NewWalletSelectScreen.ktandroid/app/src/main/java/org/bitcoinppl/cove/flows/NewWalletFlow/cold_wallet/ColdWalletQrScanScreen.ktandroid/app/src/main/java/org/bitcoinppl/cove/flows/NewWalletFlow/cold_wallet/QrCodeImportScreen.ktandroid/app/src/main/java/org/bitcoinppl/cove/flows/NewWalletFlow/hot_wallet/HotWalletImportScreen.ktandroid/app/src/main/java/org/bitcoinppl/cove/flows/SelectedWalletFlow/SelectedWalletContainer.ktandroid/app/src/main/java/org/bitcoinppl/cove/flows/SelectedWalletFlow/SelectedWalletScreen.ktandroid/app/src/main/java/org/bitcoinppl/cove/flows/SelectedWalletFlow/TransactionDetails/TransactionDetailsScreen.ktandroid/app/src/main/java/org/bitcoinppl/cove/flows/SelectedWalletFlow/TransactionsCardView.ktandroid/app/src/main/java/org/bitcoinppl/cove/flows/SendFlow/SendFlowHardwareScreen.ktandroid/app/src/main/java/org/bitcoinppl/cove/flows/SettingsFlow/MainSettingsScreen.ktandroid/app/src/main/java/org/bitcoinppl/cove/flows/SettingsFlow/NetworkSettingsScreen.ktandroid/app/src/main/java/org/bitcoinppl/cove/navigation/CoveNavDisplay.ktandroid/app/src/main/java/org/bitcoinppl/cove/sidebar/SidebarView.ktios/Cove/AppManager.swiftios/Cove/AuthManager.swiftios/Cove/CoveMainView.swiftios/Cove/Flows/NewWalletFlow/ColdWallet/QrCodeImportScreen.swiftios/Cove/Flows/NewWalletFlow/HotWallet/HotWalletImportScreen.swiftios/Cove/Flows/NewWalletFlow/NewWalletSelectScreen.swiftios/Cove/Flows/SelectedWalletFlow/SelectedWalletContainer.swiftios/Cove/Flows/SelectedWalletFlow/TransactionDetails/TransactionsDetailScreen.swiftios/Cove/Flows/SendFlow/SendFlowContainer.swiftios/Cove/Flows/SettingsFlow/MainSettingsScreen.swiftios/Cove/Flows/SettingsFlow/SettingsContainer.swiftios/Cove/Flows/SettingsFlow/WalletSettings/WalletSettingsContainer.swiftios/Cove/HomeScreens/ListWalletsScreen.swiftios/Cove/LoadAndResetContainer.swiftios/Cove/RouteView.swiftios/Cove/ScanManager.swiftios/Cove/Views/SidebarView.swiftrust/src/keys.rsrust/src/manager/cloud_backup_manager/pending/queue_processor.rsrust/src/manager/cloud_backup_manager/verify/session.rsrust/src/manager/pending_wallet_manager.rsrust/src/qr_scanner.rs
Avoid selecting the imported wallet when onboarding supplies its own import callback. The callback owns the next onboarding route, while the no-callback path still selects and opens the imported wallet.
Record additional repository guidance for readability and comment placement in Rust, Swift, and Kotlin edits.
Consolidate passkey material handling and authentication behind explicit helper types, and add explicit master key version parsing so callers share clearer, stable error behavior.
Refine verification prompts and pending cloud upload state transitions for clearer behavior around authorization failures, retries, and startup integrity refresh conditions.
Align wallet import/reset side effects and route reset timing in Android and iOS so selection, navigation, and alerts update deterministically.
Improve pending-wallet import diagnostics and preserve completed QR scan state for reliable scanner consumers.
Move cloud backup recovery, verification, and cloud-only action handling into a single cloud_backup_manager detail module. This removes a separate module boundary that was only delegating manager control flow, reducing indirection and making all related state transitions co-located. Imports and enum references were updated to preserve behavior while keeping the public API surface unchanged.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
ios/Cove/AppManager.swift (1)
158-164:⚠️ Potential issue | 🟠 MajorRe-register reconcile listener after rebuilding
FfiAppinreset().Line 158 creates a new
FfiApp, but unlike initialization (Line 97),reset()does not callrust.listenForUpdates(updater: self). After reset, app-state reconcile messages can stop flowing.🔧 Proposed fix
public func reset() { pendingSidebarNavigationTask?.cancel() pendingSidebarNavigationTask = nil beginNavigationIntent() rust = FfiApp() + rust.listenForUpdates(updater: self) database = Database() walletManager = nil let state = rust.state() router = state.router }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ios/Cove/AppManager.swift` around lines 158 - 164, The reset() implementation creates a new FfiApp instance (rust = FfiApp()) but does not re-register the reconcile listener, so state updates stop; after rebuilding FfiApp in reset(), call rust.listenForUpdates(updater: self) (the same call used during initialization where rust.listenForUpdates(updater: self) is invoked) to reattach the app as the update listener so reconcile messages continue flowing.rust/src/manager/cloud_backup_manager/verify/pending_completion.rs (1)
87-104:⚠️ Potential issue | 🔴 CriticalConfirm failed uploads have a guaranteed terminal-failure path or revise
is_pending_upload_confirmedto prevent pending loops.Line 87 treats
PersistedCloudBlobState::Failed(_)as confirmed without verifying remote blob presence. However, inverify_pending_wallet_backup(line 175-189), ifreader.summary()returnsNotFound, the function returnsPendingrather than marking the upload as failed. SincePendingdoes not clearpending_verification_completion(line 48 only clears on success or error),finalize_pending_verification_if_ready()can loop indefinitely:
- Upload fails before or during cloud write →
Failedstate storedis_pending_upload_confirmed()returnstrue(line 87, no remote check)verify_pending_wallet_backup()finds no remote blob and returnsPending- Completion not cleared, retry loops on next call
Failed uploads with
revision_hash: Noneor failed uploads where the cloud write never completed can trigger this path. Either ensure failed uploads always have remote blobs, add a retry limit to Pending states, or verify remote presence before confirming failed uploads inis_pending_upload_confirmed().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/src/manager/cloud_backup_manager/verify/pending_completion.rs` around lines 87 - 104, The code treats PersistedCloudBlobState::Failed(_) as immediately confirmed in is_pending_upload_confirmed, which can cause an infinite pending loop when the remote blob doesn't exist; modify is_pending_upload_confirmed to check remote presence for the Failed(_) variant (same pattern used for UploadedPendingConfirmation(_)) by calling CloudStorage::global_silent_client().download_wallet_backup with completion.namespace_id() and upload.record_id(), mapping CloudStorageError::NotFound(_) to Ok(false) and other errors to Err, and returning false on unwrap_or(false); ensure you update the branch handling PersistedCloudBlobState::Failed(_) instead of returning true unconditionally so verify_pending_wallet_backup can transition out of Pending when remote is absent.
🧹 Nitpick comments (4)
rust/src/manager/cloud_backup_manager/pending/detail.rs (1)
91-96: Add a blank line between thesematcharms.Please separate the multi-line
UploadedPendingConfirmationarm from the fallback arm with a blank line for consistency.♻️ Proposed formatting-only diff
let pending_state = match &state.state { PersistedCloudBlobState::UploadedPendingConfirmation(pending_state) => { pending_state } + _ => continue, };As per coding guidelines,
**/*.{rs,swift,kt,kts}requires blank lines between multi-linematch/switcharms.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/src/manager/cloud_backup_manager/pending/detail.rs` around lines 91 - 96, The match expression extracting pending_state from state.state should have a blank line between the multi-line arm for PersistedCloudBlobState::UploadedPendingConfirmation and the fallback arm; update the match in the function where pending_state is assigned (matching PersistedCloudBlobState::UploadedPendingConfirmation(pending_state) => { pending_state } _ => continue) to insert a single empty line between those two arms to satisfy the Rust formatting guideline.rust/crates/cove-device/src/keychain.rs (1)
221-228: Add unit coverage for malformed credential hex behavior.This method intentionally treats decode failures as
None; please add a test that stores invalid hex and assertsNone(and valid hex still round-trips) to keep this contract stable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/crates/cove-device/src/keychain.rs` around lines 221 - 228, Add a unit test for load_cspp_credential_id that verifies decode failures return None and valid hex round-trips: write a test which uses the same key name constant CSPP_CREDENTIAL_ID_KEY to insert an invalid hex string via the keychain backing (exercise Keychain::get/put or the test helper used elsewhere), assert load_cspp_credential_id() returns None, then store a valid hex (e.g. hex::encode of a byte vec) and assert load_cspp_credential_id() returns the original bytes; reference the load_cspp_credential_id method and ensure the test covers the hex::decode error path and successful path so the contract remains stable.rust/src/manager/cloud_backup_manager/verify/wrapper_repair.rs (1)
209-216: Avoid redundantcredential_idclones in credential construction.You can move
credential_iddirectly intoWrapperRepairCredentialsin both branches and skip the extra allocations.Proposed clone removal
Ok(WrapperRepairCredentials { prf_key: new_prf.prf_key, prf_salt: new_prf.prf_salt, - credential_id: new_prf.credential_id.clone(), + credential_id: new_prf.credential_id, }) @@ Ok(WrapperRepairCredentials { prf_key: passkey.prf_key, prf_salt: passkey.prf_salt, - credential_id: passkey.credential_id.clone(), + credential_id: passkey.credential_id, })Also applies to: 220-229
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/src/manager/cloud_backup_manager/verify/wrapper_repair.rs` around lines 209 - 216, The code is cloning credential_id unnecessarily when building WrapperRepairCredentials; instead take ownership of the credential_id from the created struct to avoid the extra allocation. In the branches that call PasskeyMaterialAcquirer::new(&self.passkey).create_for_wrapper_repair().await (and the other branch around lines 220-229), move new_prf.credential_id into WrapperRepairCredentials (e.g., use the owned value rather than new_prf.credential_id.clone()) and adjust any pattern/matching to consume new_prf so the credential_id can be moved.rust/src/manager/cloud_backup_manager/wallets/passkey.rs (1)
192-195: Use the typed backup-version parser here too.This is the one remaining raw
version != 1gate in the new flow. CallingEncryptedMasterKeyBackup::backup_version()keeps namespace matching aligned with the version handling you just centralized elsewhere.♻️ Suggested cleanup
- if encrypted.version != 1 { + if encrypted.backup_version().is_err() { had_unsupported_versions = true; continue; }As per coding guidelines:
rust/**/*.rs: Verify correct functionality and logic consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/src/manager/cloud_backup_manager/wallets/passkey.rs` around lines 192 - 195, Replace the raw numeric version check on the EncryptedMasterKeyBackup struct (the `if encrypted.version != 1 { ... }` gate) with the typed parser call EncryptedMasterKeyBackup::backup_version() so namespace/version handling is centralized; i.e., call EncryptedMasterKeyBackup::backup_version() on `encrypted`, branch when it returns an unsupported value to set had_unsupported_versions and continue, keeping the existing behavior but using the typed API for consistency.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@rust/src/manager/cloud_backup_manager/verify.rs`:
- Around line 261-268: Change the error classification in the
encrypted.backup_version() match: instead of returning
CloudBackupError::Internal when the Err(unsupported) case occurs, return the
compatibility-specific variant (e.g., CloudBackupError::Compatibility or the
project’s forward‑compatibility error type) with the same descriptive message
about the unsupported master key backup version; update the Err(unsupported)
branch that currently references MasterKeyBackupVersion::V1 and
CloudBackupError::Internal to use the compatibility error variant so unsupported
backup versions are treated as forward‑compatibility cases.
In `@rust/src/manager/cloud_backup_manager/verify/passkey_auth.rs`:
- Around line 39-52: The PasskeyAuthenticator type and its associated methods
use pub(super) which violates the repo visibility rule; update the visibility to
pub(crate) for the struct declaration and all impl items (e.g., change
`pub(super) struct PasskeyAuthenticator`, `pub(super) fn new`, and `pub(super)
async fn authenticate_with_policy` to use `pub(crate)`) so the type and its API
follow the crate-level visibility guideline.
In `@rust/src/manager/cloud_backup_manager/wallets/passkey.rs`:
- Around line 221-225: The namespace-matching match that binds `discovered` maps
all `PasskeyError` variants except `UserCancelled` and `NoCredentialFound` into
a generic `CloudBackupError::Passkey(...)`, losing the special-case
`PrfUnsupportedProvider`; update that match (the block binding `discovered` and
the similar block later around the other namespace-checking match) to handle
`Err(PasskeyError::PrfUnsupportedProvider)` explicitly by returning
`Err(CloudBackupError::UnsupportedPasskeyProvider)` (the same mapping used in
the acquirer path), leaving the existing `UserCancelled` ->
`NamespaceMatchOutcome::UserDeclined` and `NoCredentialFound` ->
`NamespaceMatchOutcome::NoMatch` arms unchanged.
---
Outside diff comments:
In `@ios/Cove/AppManager.swift`:
- Around line 158-164: The reset() implementation creates a new FfiApp instance
(rust = FfiApp()) but does not re-register the reconcile listener, so state
updates stop; after rebuilding FfiApp in reset(), call
rust.listenForUpdates(updater: self) (the same call used during initialization
where rust.listenForUpdates(updater: self) is invoked) to reattach the app as
the update listener so reconcile messages continue flowing.
In `@rust/src/manager/cloud_backup_manager/verify/pending_completion.rs`:
- Around line 87-104: The code treats PersistedCloudBlobState::Failed(_) as
immediately confirmed in is_pending_upload_confirmed, which can cause an
infinite pending loop when the remote blob doesn't exist; modify
is_pending_upload_confirmed to check remote presence for the Failed(_) variant
(same pattern used for UploadedPendingConfirmation(_)) by calling
CloudStorage::global_silent_client().download_wallet_backup with
completion.namespace_id() and upload.record_id(), mapping
CloudStorageError::NotFound(_) to Ok(false) and other errors to Err, and
returning false on unwrap_or(false); ensure you update the branch handling
PersistedCloudBlobState::Failed(_) instead of returning true unconditionally so
verify_pending_wallet_backup can transition out of Pending when remote is
absent.
---
Nitpick comments:
In `@rust/crates/cove-device/src/keychain.rs`:
- Around line 221-228: Add a unit test for load_cspp_credential_id that verifies
decode failures return None and valid hex round-trips: write a test which uses
the same key name constant CSPP_CREDENTIAL_ID_KEY to insert an invalid hex
string via the keychain backing (exercise Keychain::get/put or the test helper
used elsewhere), assert load_cspp_credential_id() returns None, then store a
valid hex (e.g. hex::encode of a byte vec) and assert load_cspp_credential_id()
returns the original bytes; reference the load_cspp_credential_id method and
ensure the test covers the hex::decode error path and successful path so the
contract remains stable.
In `@rust/src/manager/cloud_backup_manager/pending/detail.rs`:
- Around line 91-96: The match expression extracting pending_state from
state.state should have a blank line between the multi-line arm for
PersistedCloudBlobState::UploadedPendingConfirmation and the fallback arm;
update the match in the function where pending_state is assigned (matching
PersistedCloudBlobState::UploadedPendingConfirmation(pending_state) => {
pending_state } _ => continue) to insert a single empty line between those two
arms to satisfy the Rust formatting guideline.
In `@rust/src/manager/cloud_backup_manager/verify/wrapper_repair.rs`:
- Around line 209-216: The code is cloning credential_id unnecessarily when
building WrapperRepairCredentials; instead take ownership of the credential_id
from the created struct to avoid the extra allocation. In the branches that call
PasskeyMaterialAcquirer::new(&self.passkey).create_for_wrapper_repair().await
(and the other branch around lines 220-229), move new_prf.credential_id into
WrapperRepairCredentials (e.g., use the owned value rather than
new_prf.credential_id.clone()) and adjust any pattern/matching to consume
new_prf so the credential_id can be moved.
In `@rust/src/manager/cloud_backup_manager/wallets/passkey.rs`:
- Around line 192-195: Replace the raw numeric version check on the
EncryptedMasterKeyBackup struct (the `if encrypted.version != 1 { ... }` gate)
with the typed parser call EncryptedMasterKeyBackup::backup_version() so
namespace/version handling is centralized; i.e., call
EncryptedMasterKeyBackup::backup_version() on `encrypted`, branch when it
returns an unsupported value to set had_unsupported_versions and continue,
keeping the existing behavior but using the typed API for consistency.
🪄 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: c7b12918-075c-4a2f-96ce-3c48fdc5e6be
📒 Files selected for processing (33)
AGENTS.mdandroid/app/src/main/java/org/bitcoinppl/cove/AppManager.ktandroid/app/src/main/java/org/bitcoinppl/cove/MainActivity.ktandroid/app/src/main/java/org/bitcoinppl/cove/RouteView.ktandroid/app/src/main/java/org/bitcoinppl/cove/flows/NewWalletFlow/hot_wallet/HotWalletImportScreen.ktandroid/app/src/main/java/org/bitcoinppl/cove/flows/SelectedWalletFlow/TransactionsCardView.ktandroid/app/src/main/java/org/bitcoinppl/cove/navigation/CoveNavDisplay.ktios/Cove/AppManager.swiftios/Cove/Flows/NewWalletFlow/ColdWallet/QrCodeImportScreen.swiftios/Cove/Flows/NewWalletFlow/HotWallet/HotWalletImportScreen.swiftios/Cove/Flows/NewWalletFlow/NewWalletSelectScreen.swiftios/Cove/LoadAndResetContainer.swiftrust/crates/cove-cspp/src/backup_data.rsrust/crates/cove-cspp/src/master_key_crypto.rsrust/crates/cove-device/src/keychain.rsrust/src/manager.rsrust/src/manager/cloud_backup_manager.rsrust/src/manager/cloud_backup_manager/detail.rsrust/src/manager/cloud_backup_manager/ops.rsrust/src/manager/cloud_backup_manager/pending/detail.rsrust/src/manager/cloud_backup_manager/pending/queue_processor.rsrust/src/manager/cloud_backup_manager/prompt.rsrust/src/manager/cloud_backup_manager/runtime_actor.rsrust/src/manager/cloud_backup_manager/verify.rsrust/src/manager/cloud_backup_manager/verify/integrity.rsrust/src/manager/cloud_backup_manager/verify/passkey_auth.rsrust/src/manager/cloud_backup_manager/verify/pending_completion.rsrust/src/manager/cloud_backup_manager/verify/session.rsrust/src/manager/cloud_backup_manager/verify/wrapper_repair.rsrust/src/manager/cloud_backup_manager/wallets.rsrust/src/manager/cloud_backup_manager/wallets/passkey.rsrust/src/manager/pending_wallet_manager.rsrust/src/qr_scanner.rs
💤 Files with no reviewable changes (1)
- rust/src/manager.rs
✅ Files skipped from review due to trivial changes (3)
- AGENTS.md
- rust/src/manager/cloud_backup_manager/runtime_actor.rs
- rust/src/manager/cloud_backup_manager/wallets.rs
🚧 Files skipped from review as they are similar to previous changes (4)
- ios/Cove/Flows/NewWalletFlow/NewWalletSelectScreen.swift
- rust/src/manager/pending_wallet_manager.rs
- rust/src/manager/cloud_backup_manager/verify/session.rs
- rust/src/manager/cloud_backup_manager/pending/queue_processor.rs
praveenperera
force-pushed
the
refactor-and-navigation-fix
branch
from
35ae586 to
e5df590
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
ios/Cove/AppManager.swift (2)
260-264: Add a blank line between mutation and return inbeginNavigationIntent().This function currently mutates state and returns immediately without the required visual separation.
As per coding guidelines "`**/*.{rs,swift,kt}`: Add blank lines between logical steps in function bodies ... between state mutations and final returns".🎨 Minimal formatting fix
`@discardableResult` private func beginNavigationIntent() -> UInt64 { navigationGeneration &+= 1 + return navigationGeneration }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ios/Cove/AppManager.swift` around lines 260 - 264, In beginNavigationIntent(), add a blank line after the state mutation (the navigationGeneration &+= 1 statement) and before the return of navigationGeneration so the mutation and the return are visually separated; update the body of the beginNavigationIntent() method to include that blank line while leaving the existing logic and `@discardableResult` annotation unchanged.
194-203: Consider routing these user intents throughdispatch(action:)for manager consistency.
selectWalletOrThrowandselectLatestOrNewWalletare user-intent entry points; aligning them withdispatch(action:)(where equivalent actions exist) would keep AppManager intent handling consistent and easier to audit.Based on learnings: "For long-lived UI-facing managers, prefer
dispatch(action:)for user intents and keep named methods for reads, bootstrap/lifecycle hooks, and special service-style operations; usestate()for the initial snapshot only; send typed delta reconcile messages for ongoing UI updates instead of re-sending the whole state after every mutation".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ios/Cove/AppManager.swift` around lines 194 - 203, Replace direct calls from UI intents with dispatched actions: stop calling beginNavigationIntent(); rust.selectWallet(id:); rust.selectLatestOrNewWallet() directly in selectWalletOrThrow and selectLatestOrNewWallet. Instead define/ reuse typed actions (e.g., SelectWallet(id: WalletId) and SelectLatestOrNewWallet) that the AppManager reducer/handler will run (calling rust.selectWallet / rust.selectLatestOrNewWallet, handle throwing errors for SelectWallet, and set isSidebarVisible = false there). Then have selectWalletOrThrow and selectLatestOrNewWallet call beginNavigationIntent() and dispatch(action: /* corresponding action */) so all intent handling flows through dispatch(action:).rust/src/manager/cloud_backup_manager/verify/pending_completion.rs (1)
87-93: Log remote-check failures before falling back tofalse.Current
unwrap_or(false)keeps verification pending (good) but silently drops non-NotFoundcloud errors, which makes stalls hard to diagnose.Suggested refactor
- Some(PersistedCloudBlobState::Failed(_)) => { - Self::remote_pending_upload_exists(completion, upload).await.unwrap_or(false) - } + Some(PersistedCloudBlobState::Failed(_)) => { + Self::remote_pending_upload_exists(completion, upload).await + } Some(PersistedCloudBlobState::UploadedPendingConfirmation(_)) => { - Self::remote_pending_upload_exists(completion, upload).await.unwrap_or(false) + Self::remote_pending_upload_exists(completion, upload).await } @@ - ) -> Result<bool, CloudStorageError> { - CloudStorage::global_silent_client() + ) -> bool { + match CloudStorage::global_silent_client() .download_wallet_backup( completion.namespace_id().to_string(), upload.record_id().to_string(), ) .await - .map(|_| true) - .or_else(|error| match error { - CloudStorageError::NotFound(_) => Ok(false), - other => Err(other), - }) + { + Ok(_) => true, + Err(CloudStorageError::NotFound(_)) => false, + Err(error) => { + warn!( + "Pending verification: remote upload existence check failed for record_id={}: {error}", + upload.record_id(), + ); + false + } + } }Also applies to: 99-114
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/src/manager/cloud_backup_manager/verify/pending_completion.rs` around lines 87 - 93, The code currently calls Self::remote_pending_upload_exists(...).await.unwrap_or(false) in the PersistedCloudBlobState::Failed and ::UploadedPendingConfirmation arms (and similar places) which hides cloud errors; change those unwrap_or(false) calls to explicitly match the Result, log the Err with context (include the completion and upload identifiers or other unique context) and then return false on error. Specifically, replace the unwrap_or(false) uses around remote_pending_upload_exists in pending_completion.rs with a Result match (or .await.map_or_else) that on Ok(v) returns v and on Err(e) calls the logger (e.g., error!("remote_pending_upload_exists failed for completion={} upload={} : {:?}", completion.id, upload.id, e)) then returns false.rust/src/manager/cloud_backup_manager/ops.rs (1)
559-568: Use an enable-specific discover/create entrypoint here.
do_enable_cloud_backup_create_newnow goes throughdiscover_or_create_for_wrapper_repair(), so normal enable cancellations and logs inherit wrapper-repair wording/context. The no-discovery enable path already uses the enable-specificcreate_new(...)API, so these two enable flows now diverge operationally. I’d extract a dedicateddiscover_or_create_for_enablewrapper (or parameterize the context) to keep enable telemetry and error paths consistent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/src/manager/cloud_backup_manager/ops.rs` around lines 559 - 568, The current enable flow calls PasskeyMaterialAcquirer::discover_or_create_for_wrapper_repair (via do_enable_cloud_backup_create_new), which causes enable-specific cancellations/logs to inherit "wrapper-repair" wording; change the enable path to use an enable-specific discovery/create entrypoint: add a new wrapper (e.g., PasskeyMaterialAcquirer::discover_or_create_for_enable or a helper discover_or_create_for_enable) that forwards to the appropriate internal logic but uses enable-specific context/messages, then update the acquire_enable_passkey call site to invoke this new discover_or_create_for_enable closure instead of discover_or_create_for_wrapper_repair so do_enable_cloud_backup_create_new and the no-discovery create_new path share consistent enable telemetry and error wording.
🤖 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`:
- Around line 225-228: popRoute() currently calls router.routes.removeLast()
which can crash if router.routes is empty; update popRoute() to check the routes
array before mutating it (e.g., guard !router.routes.isEmpty else { return } or
log and return) so removeLast() is only called when safe; keep the call to
beginNavigationIntent() only if appropriate (either before or after the
emptiness check depending on intended behavior) and reference popRoute(),
router.routes, removeLast(), beginNavigationIntent(), and currentRoute when
making the change so callers relying on empty-route behavior remain supported.
In `@rust/src/manager/cloud_backup_manager/verify/passkey_auth.rs`:
- Around line 83-86: The StoredPasskeyAuthOutcome::Failed arm in
verify/passkey_auth.rs currently converts all failures (e.g., in the match that
returns Ok(PasskeyAuthOutcome::NoCredentialFound)) which hides
PasskeyError::PrfUnsupportedProvider; change the failure handling to detect and
propagate PasskeyError::PrfUnsupportedProvider up as
CloudBackupError::UnsupportedPasskeyProvider (instead of mapping to
NoCredentialFound), and update the map_discovery_error usage to preserve /
convert PrfUnsupportedProvider into the specific UnsupportedPasskeyProvider
variant rather than a generic Passkey(...) error; apply the same change to the
other identical handlers referenced (the ones around
StoredPasskeyAuthOutcome::Failed at the other two locations) so matcher/acquirer
and verification paths behave consistently.
---
Nitpick comments:
In `@ios/Cove/AppManager.swift`:
- Around line 260-264: In beginNavigationIntent(), add a blank line after the
state mutation (the navigationGeneration &+= 1 statement) and before the return
of navigationGeneration so the mutation and the return are visually separated;
update the body of the beginNavigationIntent() method to include that blank line
while leaving the existing logic and `@discardableResult` annotation unchanged.
- Around line 194-203: Replace direct calls from UI intents with dispatched
actions: stop calling beginNavigationIntent(); rust.selectWallet(id:);
rust.selectLatestOrNewWallet() directly in selectWalletOrThrow and
selectLatestOrNewWallet. Instead define/ reuse typed actions (e.g.,
SelectWallet(id: WalletId) and SelectLatestOrNewWallet) that the AppManager
reducer/handler will run (calling rust.selectWallet /
rust.selectLatestOrNewWallet, handle throwing errors for SelectWallet, and set
isSidebarVisible = false there). Then have selectWalletOrThrow and
selectLatestOrNewWallet call beginNavigationIntent() and dispatch(action: /*
corresponding action */) so all intent handling flows through dispatch(action:).
In `@rust/src/manager/cloud_backup_manager/ops.rs`:
- Around line 559-568: The current enable flow calls
PasskeyMaterialAcquirer::discover_or_create_for_wrapper_repair (via
do_enable_cloud_backup_create_new), which causes enable-specific
cancellations/logs to inherit "wrapper-repair" wording; change the enable path
to use an enable-specific discovery/create entrypoint: add a new wrapper (e.g.,
PasskeyMaterialAcquirer::discover_or_create_for_enable or a helper
discover_or_create_for_enable) that forwards to the appropriate internal logic
but uses enable-specific context/messages, then update the
acquire_enable_passkey call site to invoke this new
discover_or_create_for_enable closure instead of
discover_or_create_for_wrapper_repair so do_enable_cloud_backup_create_new and
the no-discovery create_new path share consistent enable telemetry and error
wording.
In `@rust/src/manager/cloud_backup_manager/verify/pending_completion.rs`:
- Around line 87-93: The code currently calls
Self::remote_pending_upload_exists(...).await.unwrap_or(false) in the
PersistedCloudBlobState::Failed and ::UploadedPendingConfirmation arms (and
similar places) which hides cloud errors; change those unwrap_or(false) calls to
explicitly match the Result, log the Err with context (include the completion
and upload identifiers or other unique context) and then return false on error.
Specifically, replace the unwrap_or(false) uses around
remote_pending_upload_exists in pending_completion.rs with a Result match (or
.await.map_or_else) that on Ok(v) returns v and on Err(e) calls the logger
(e.g., error!("remote_pending_upload_exists failed for completion={} upload={} :
{:?}", completion.id, upload.id, e)) then returns false.
🪄 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: b767bd08-830f-46ce-a908-e836b1ffa677
⛔ 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_core/cove.ktios/Cove/AppManager.swiftrust/crates/cove-device/src/keychain.rsrust/src/manager/cloud_backup_manager.rsrust/src/manager/cloud_backup_manager/ops.rsrust/src/manager/cloud_backup_manager/pending/detail.rsrust/src/manager/cloud_backup_manager/verify.rsrust/src/manager/cloud_backup_manager/verify/passkey_auth.rsrust/src/manager/cloud_backup_manager/verify/pending_completion.rsrust/src/manager/cloud_backup_manager/verify/wrapper_repair.rsrust/src/manager/cloud_backup_manager/wallets.rsrust/src/manager/cloud_backup_manager/wallets/passkey.rsrust/src/manager/cloud_backup_manager/wallets/upload.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- rust/src/manager/cloud_backup_manager/wallets.rs
- rust/src/manager/cloud_backup_manager/pending/detail.rs
- rust/src/manager/cloud_backup_manager/verify/wrapper_repair.rs
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
rust/src/app.rs (1)
242-245: Usemap_err_strinstead of manual stringification.Replace
.map_err(|error| AppError::WalletSelection(error.to_string()))with.map_err_str(AppError::WalletSelection)to align with the established pattern across the codebase per coding guidelines.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/src/app.rs` around lines 242 - 245, In the AppAction::SelectWallet match arm, change the manual map_err closure on FfiApp::global().select_wallet(id, None) to use the codebase helper by replacing `.map_err(|error| AppError::WalletSelection(error.to_string()))` with `.map_err_str(AppError::WalletSelection)` so the select_wallet call uses map_err_str with AppError::WalletSelection.
🤖 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`:
- Around line 225-229: popRoute currently returns early when router.routes is
empty, skipping beginNavigationIntent and leaving a previously queued deferred
sidebar navigation valid; change popRoute so beginNavigationIntent() is always
invoked before the guard/early-return (or move the guard to only protect
router.routes.removeLast), e.g., ensure beginNavigationIntent() is called
unconditionally inside popRoute and only conditionally call
router.routes.removeLast() when router.routes.isEmpty is false.
In `@rust/src/app.rs`:
- Around line 248-250: The handler for AppAction::SelectLatestOrNewWallet
blindly treats any failure inside FfiApp::global().select_latest_or_new_wallet()
as "no wallets" because select_latest_or_new_wallet() currently falls back to
Route::NewWallet for every select_latest_wallet() error; change
select_latest_or_new_wallet() so it distinguishes the "no wallets found" case
from data-access/query errors and returns a Result<Route, AppError> (or
otherwise propagates AppError::WalletSelection on DB/query failures), then
update the call site in AppAction::SelectLatestOrNewWallet to handle the
Err(AppError::WalletSelection) path (propagate/log/return the error) rather than
treating it as Route::NewWallet; keep Route::NewWallet only for the explicit
empty-wallet case from select_latest_wallet().
In `@rust/src/manager/cloud_backup_manager/wallets/passkey.rs`:
- Around line 83-98: The current match_namespaces() implementation returns on
the first successful decrypt, allowing a single credential_id to be reused
across namespaces and causing nondeterministic/incorrect restores; change
match_namespaces() to continue scanning all candidate namespaces after the first
successful decrypt, collect all successful matches, and then either (a) surface
an explicit ambiguity error (e.g., AmbiguousCredentialForNamespaces) to callers
(used by discover_or_create_for_enable() and the restore flow in ops.rs) or (b)
apply a deterministic tie-breaker based on real metadata before returning a
single namespace; update discover_or_create_for_enable() and its callers in
rust/src/manager/cloud_backup_manager/ops.rs to handle the new ambiguity result,
and add a regression test that creates two namespaces encrypted with the same
credential to ensure ambiguity is detected and handled.
---
Nitpick comments:
In `@rust/src/app.rs`:
- Around line 242-245: In the AppAction::SelectWallet match arm, change the
manual map_err closure on FfiApp::global().select_wallet(id, None) to use the
codebase helper by replacing `.map_err(|error|
AppError::WalletSelection(error.to_string()))` with
`.map_err_str(AppError::WalletSelection)` so the select_wallet call uses
map_err_str with AppError::WalletSelection.
🪄 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: 1222bd45-1843-4048-88fb-695e3ed59060
⛔ Files ignored due to path filters (1)
ios/CoveCore/Sources/CoveCore/generated/cove.swiftis excluded by!**/generated/**
📒 Files selected for processing (7)
android/app/src/main/java/org/bitcoinppl/cove_core/cove.ktios/Cove/AppManager.swiftrust/src/app.rsrust/src/manager/cloud_backup_manager/ops.rsrust/src/manager/cloud_backup_manager/verify/passkey_auth.rsrust/src/manager/cloud_backup_manager/verify/pending_completion.rsrust/src/manager/cloud_backup_manager/wallets/passkey.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- rust/src/manager/cloud_backup_manager/verify/pending_completion.rs
Selection now returns an error when no wallets exist, and callers handle fallback behavior instead of assuming success. Native Kotlin and Swift bindings were regenerated for the updated error- returning API and mobile startup/navigation paths were adjusted to avoid unnecessary crashes during wallet bootstrap.,
praveenperera
force-pushed
the
refactor-and-navigation-fix
branch
from
205c5a4 to
874da81
Compare
Restore now keeps all namespaces matched by one passkey and restores wallets from every match in order. This includes aggregate progress reporting for the full download set, namespace-scoped error reporting, and updated matching behavior to return multiple candidates instead of a single namespace result.,
praveenperera
force-pushed
the
refactor-and-navigation-fix
branch
from
874da81 to
363094c
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
rust/src/manager/cloud_backup_manager/ops.rs (1)
629-631: 🛠️ Refactor suggestion | 🟠 MajorUse
pub(crate)here instead ofpub(super).This new entrypoint violates the repo's Rust visibility rule.
Suggested fix
- pub(super) async fn do_enable_cloud_backup_no_discovery(&self) -> Result<(), CloudBackupError> { + pub(crate) async fn do_enable_cloud_backup_no_discovery(&self) -> Result<(), CloudBackupError> {As per coding guidelines "
**/*.rs: Never usepub(in ...)orpub(super)visibility modifiers; usepub(crate)orpubif non-private visibility is needed".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/src/manager/cloud_backup_manager/ops.rs` around lines 629 - 631, The function visibility for do_enable_cloud_backup_no_discovery is declared as pub(super) which violates the repo rule; change its visibility to pub(crate) so the signature becomes pub(crate) async fn do_enable_cloud_backup_no_discovery(&self) -> Result<(), CloudBackupError> (locate this method in cloud_backup_manager::ops.rs and update the visibility modifier only).
♻️ Duplicate comments (1)
rust/src/manager/cloud_backup_manager/ops.rs (1)
469-485:⚠️ Potential issue | 🟠 MajorDon't silently bind recovery to the first matched namespace.
When one passkey matches multiple namespaces, these branches still persist only
matches[0]. That choice is effectively arbitrary, but the rest of this file usescurrent_namespace_id()for future sync/delete/detail flows, so wallets restored from the other matched namespaces can later be treated as out-of-sync and re-uploaded into the selected namespace. Please surface this as an explicit ambiguity/user-choice instead of auto-picking the first match.Also applies to: 731-759
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/src/manager/cloud_backup_manager/ops.rs` around lines 469 - 485, The code currently auto-picks the first namespace from NamespaceMatchOutcome::Matched by calling matches.into_iter().next() and then calls complete_recovery with that arbitrary choice; change this to detect when matched_count > 1 and surface an explicit ambiguity flow (e.g., prompt user to choose which namespace to restore or fail with a clear ambiguity error) instead of silently binding to matches[0]; after the user selects a namespace (or aborts), pass that selected namespace to complete_recovery(keychain, &cloud, &cspp, selected_namespace) and ensure the same ambiguity-handling change is applied to the duplicate block around the other occurrence (the similar logic at lines 731-759) so future current_namespace_id() uses reflect the user’s selection rather than an implicit first element.
🧹 Nitpick comments (2)
rust/src/app.rs (1)
678-689: Usemap_err_prefixfor static-context error wrapping.Line 678 and Line 685 build static-prefix error strings manually. This is better expressed with
map_err_prefixfor consistency with the repository standard.Suggested refactor
- let wallets = database.wallets().all_sorted_active().map_err_str(|error| { - SelectLatestWalletError::WalletSelection(AppError::WalletSelection(format!( - "unable to get sorted wallets: {error}" - ))) - })?; + let wallets = database + .wallets() + .all_sorted_active() + .map_err_prefix("unable to get sorted wallets: ") + .map_err(AppError::WalletSelection) + .map_err(SelectLatestWalletError::WalletSelection)?; let latest_wallet = wallets.first().ok_or(SelectLatestWalletError::NoWalletsFound)?; - self.select_wallet(latest_wallet.id.clone(), None).map_err_str(|error| { - SelectLatestWalletError::WalletSelection(AppError::WalletSelection(format!( - "unable to select latest wallet: {error}" - ))) - })?; + self.select_wallet(latest_wallet.id.clone(), None) + .map_err_prefix("unable to select latest wallet: ") + .map_err(AppError::WalletSelection) + .map_err(SelectLatestWalletError::WalletSelection)?;As per coding guidelines
**/*.rs: Usecove_util::ResultExt::map_err_prefixinstead of.map_err(|e| Error::Variant(format!("context: {e}")))when the prefix is a static string.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/src/app.rs` around lines 678 - 689, Replace the manual error wrapping that uses map_err_str when fetching and selecting the latest wallet with the repository standard map_err_prefix: change the wallets retrieval call that currently uses .map_err_str(|error| SelectLatestWalletError::WalletSelection(AppError::WalletSelection(format!("unable to get sorted wallets: {error}")))) to use .map_err_prefix("unable to get sorted wallets", SelectLatestWalletError::WalletSelection) (or equivalent map_err_prefix signature), and likewise change the self.select_wallet(...) .map_err_str(...) wrapping that produces SelectLatestWalletError::WalletSelection(AppError::WalletSelection(...)) to use map_err_prefix("unable to select latest wallet", SelectLatestWalletError::WalletSelection) so the static-context prefix is used consistently instead of formatting the error string manually; keep the same error variants (SelectLatestWalletError::WalletSelection and AppError::WalletSelection) as the target wrappers.android/app/src/main/java/org/bitcoinppl/cove_core/cove.kt (1)
9975-9978: Avoid exposing wallet selection through multiple public APIs with different failure semantics.
SelectWallet/SelectLatestOrNewWalletare now part ofAppAction, but wallet selection is also exposed through dedicated methods and a separate throwing dispatch path. That makes it easy for future callers to pick non-throwingdispatch()and accidentally loseAppExceptionpropagation for the same user intent. I’d pick one canonical source-level API for wallet selection, then regenerate this binding.Based on learnings,
For long-lived UI-facing managers, prefer dispatch(action:) for user intents and keep named methods for reads, bootstrap/lifecycle hooks, and special service-style operations, use state() for the initial snapshot only, and send typed delta reconcile messages for ongoing UI updates instead of re-sending the whole state after every mutation, andNever manually edit generated files.Also applies to: 10597-10605, 30334-30343
🤖 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_core/cove.kt` around lines 9975 - 9978, The code exposes wallet selection both via AppAction (SelectWallet/SelectLatestOrNewWallet) and via dedicated public methods plus a throwing dispatch path (`dispatchThrowing`), which risks callers using non-throwing `dispatch` and dropping AppException propagation; consolidate by choosing one canonical API for wallet selection (prefer dispatch(action: AppAction) for UI intents), remove or make private the dedicated wallet-selection methods and/or `dispatchThrowing`, update callsites to use `dispatch(SelectWallet...)`/`dispatch(SelectLatestOrNewWallet...)`, then regenerate the binding so the generated file reflects the single API; retain named public methods only for reads/bootstrap/service operations and keep `state()` as initial snapshot access.
🤖 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_core/cove.kt`:
- Around line 9975-9978: The interface method dispatchThrowing(action:
AppAction) is missing a Kotlin checked-exception annotation: add
`@Throws`(AppException::class) to the method declaration in the UniFFI/source
definition for FfiAppInterface (not by editing the generated cove.kt), then
regenerate the bindings so FfiAppInterface.dispatchThrowing and the generated
override both carry `@Throws`(AppException::class) and Java callers will see the
checked exception in the interface signature.
In `@ios/Cove/AppManager.swift`:
- Around line 198-201: selectLatestOrNewWallet currently calls the non-throwing
rust.dispatch which swallows actionable failures; change selectLatestOrNewWallet
to propagate dispatch errors by making it a throwing function and call the
throwing variant (e.g., use try rust.dispatch(action:
.selectLatestOrNewWallet)); keep beginNavigationIntent() before dispatch, and
let the error propagate (or rethrow) so callers can react (retry/alert/fallback
UI) when AppAction.selectLatestOrNewWallet fails.
---
Outside diff comments:
In `@rust/src/manager/cloud_backup_manager/ops.rs`:
- Around line 629-631: The function visibility for
do_enable_cloud_backup_no_discovery is declared as pub(super) which violates the
repo rule; change its visibility to pub(crate) so the signature becomes
pub(crate) async fn do_enable_cloud_backup_no_discovery(&self) -> Result<(),
CloudBackupError> (locate this method in cloud_backup_manager::ops.rs and update
the visibility modifier only).
---
Duplicate comments:
In `@rust/src/manager/cloud_backup_manager/ops.rs`:
- Around line 469-485: The code currently auto-picks the first namespace from
NamespaceMatchOutcome::Matched by calling matches.into_iter().next() and then
calls complete_recovery with that arbitrary choice; change this to detect when
matched_count > 1 and surface an explicit ambiguity flow (e.g., prompt user to
choose which namespace to restore or fail with a clear ambiguity error) instead
of silently binding to matches[0]; after the user selects a namespace (or
aborts), pass that selected namespace to complete_recovery(keychain, &cloud,
&cspp, selected_namespace) and ensure the same ambiguity-handling change is
applied to the duplicate block around the other occurrence (the similar logic at
lines 731-759) so future current_namespace_id() uses reflect the user’s
selection rather than an implicit first element.
---
Nitpick comments:
In `@android/app/src/main/java/org/bitcoinppl/cove_core/cove.kt`:
- Around line 9975-9978: The code exposes wallet selection both via AppAction
(SelectWallet/SelectLatestOrNewWallet) and via dedicated public methods plus a
throwing dispatch path (`dispatchThrowing`), which risks callers using
non-throwing `dispatch` and dropping AppException propagation; consolidate by
choosing one canonical API for wallet selection (prefer dispatch(action:
AppAction) for UI intents), remove or make private the dedicated
wallet-selection methods and/or `dispatchThrowing`, update callsites to use
`dispatch(SelectWallet...)`/`dispatch(SelectLatestOrNewWallet...)`, then
regenerate the binding so the generated file reflects the single API; retain
named public methods only for reads/bootstrap/service operations and keep
`state()` as initial snapshot access.
In `@rust/src/app.rs`:
- Around line 678-689: Replace the manual error wrapping that uses map_err_str
when fetching and selecting the latest wallet with the repository standard
map_err_prefix: change the wallets retrieval call that currently uses
.map_err_str(|error|
SelectLatestWalletError::WalletSelection(AppError::WalletSelection(format!("unable
to get sorted wallets: {error}")))) to use .map_err_prefix("unable to get sorted
wallets", SelectLatestWalletError::WalletSelection) (or equivalent
map_err_prefix signature), and likewise change the self.select_wallet(...)
.map_err_str(...) wrapping that produces
SelectLatestWalletError::WalletSelection(AppError::WalletSelection(...)) to use
map_err_prefix("unable to select latest wallet",
SelectLatestWalletError::WalletSelection) so the static-context prefix is used
consistently instead of formatting the error string manually; keep the same
error variants (SelectLatestWalletError::WalletSelection and
AppError::WalletSelection) as the target wrappers.
🪄 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: 3b47bd6f-e6ff-4c66-b050-c0212e1bad8d
⛔ Files ignored due to path filters (1)
ios/CoveCore/Sources/CoveCore/generated/cove.swiftis excluded by!**/generated/**
📒 Files selected for processing (9)
android/app/src/main/java/org/bitcoinppl/cove/AppManager.ktandroid/app/src/main/java/org/bitcoinppl/cove_core/cove.ktios/Cove/AppManager.swiftrust/src/app.rsrust/src/manager/cloud_backup_manager/ops.rsrust/src/manager/cloud_backup_manager/verify/passkey_auth.rsrust/src/manager/cloud_backup_manager/verify/pending_completion.rsrust/src/manager/cloud_backup_manager/wallets/passkey.rsrust/src/manager/onboarding_manager.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- android/app/src/main/java/org/bitcoinppl/cove/AppManager.kt
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
ios/Cove/AppManager.swift (1)
198-205:⚠️ Potential issue | 🟠 Major
selectLatestOrNewWalletstill swallows actionable dispatch errors.Line 201–204 logs failure but does not let callers react (retry/alert/fallback). This weakens the new error-propagating wallet-selection path.
🛠️ Suggested fix
+ func selectLatestOrNewWalletOrThrow() throws { + beginNavigationIntent() + try rust.dispatch(action: .selectLatestOrNewWallet) + isSidebarVisible = false + } + func selectLatestOrNewWallet() { - beginNavigationIntent() do { - try rust.dispatch(action: .selectLatestOrNewWallet) + try selectLatestOrNewWalletOrThrow() } catch { Log.error("Unable to select latest wallet, error: \(error)") } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ios/Cove/AppManager.swift` around lines 198 - 205, The selectLatestOrNewWallet function currently swallows dispatch errors by catching and only logging them; change it to propagate failures so callers can react—either make selectLatestOrNewWallet a throwing method that calls try rust.dispatch(action: .selectLatestOrNewWallet) without catching (or rethrow the caught error), or add a completion/Result callback parameter to return success/failure; ensure beginNavigationIntent() remains invoked and preserve the rust.dispatch(action: .selectLatestOrNewWallet) reference while removing the silent catch so callers can retry/alert/fallback.
🧹 Nitpick comments (1)
android/app/src/main/java/org/bitcoinppl/cove_core/cove.kt (1)
30399-30418: Optional: document the AppAction ordinal contract inAGENTS.md.Given the tight Rust/Kotlin ordinal coupling, adding a short “do not reorder without regen + checksum update” note would reduce future integration mistakes.
Based on learnings: Document common mistakes and confusion points in the AGENTS.md file to alert future developers and agents of surprises encountered in the project.
🤖 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_core/cove.kt` around lines 30399 - 30418, Add a short note to AGENTS.md documenting the tight ordinal contract between Kotlin and Rust for AppAction (the enum/variant ordering used in the switch/when that maps ordinals like 4 -> AppAction.SelectWallet, 5 -> AppAction.SelectLatestOrNewWallet, 6 -> AppAction.ChangeNetwork, etc.), warning developers not to reorder, insert, or remove variants without regenerating bindings and updating the checksum; briefly list the common pitfalls (reordering, missing regen, checksum update) and where to update (reference AppAction and the FfiConverterType* readers) so future maintainers know the exact steps to keep Kotlin/Rust serialization in sync.
🤖 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/AppManager.kt`:
- Around line 244-251: The method selectLatestOrNewWallet currently catches
exceptions from rust.dispatch(AppAction.SelectLatestOrNewWallet) and only logs
them, preventing callers from reacting to failures; remove the
swallow-or-log-only behavior by letting the exception propagate (either remove
the try/catch or rethrow the caught Exception after logging) so callers can
handle selection failures, keeping references to beginNavigationIntent() and
rust.dispatch(AppAction.SelectLatestOrNewWallet) when locating the code.
In `@rust/src/manager/cloud_backup_manager/ops.rs`:
- Around line 889-908: The code currently persists namespace_wallets.first()
which may be lexicographically first rather than the namespace that actually
restored wallets; change the logic to pick the namespace associated with the
first successful restore (or first namespace with downloadable/restored wallets)
returned by restore_via_passkey_matching() instead of namespace_wallets.first().
Locate where restore results are collected and add a variable like
first_success_namespace (or find the first element in the restore results with
non-empty downloads/success), then use that namespace when calling
cspp.save_master_key(&...master_key) and
keychain.save_cspp_passkey_and_namespace(... &active.namespace_id) (also apply
the same change to the analogous block referenced around the 1062-1069 region)
so persisted master key/passkey/namespace matches the namespace that actually
restored data.
---
Duplicate comments:
In `@ios/Cove/AppManager.swift`:
- Around line 198-205: The selectLatestOrNewWallet function currently swallows
dispatch errors by catching and only logging them; change it to propagate
failures so callers can react—either make selectLatestOrNewWallet a throwing
method that calls try rust.dispatch(action: .selectLatestOrNewWallet) without
catching (or rethrow the caught error), or add a completion/Result callback
parameter to return success/failure; ensure beginNavigationIntent() remains
invoked and preserve the rust.dispatch(action: .selectLatestOrNewWallet)
reference while removing the silent catch so callers can retry/alert/fallback.
---
Nitpick comments:
In `@android/app/src/main/java/org/bitcoinppl/cove_core/cove.kt`:
- Around line 30399-30418: Add a short note to AGENTS.md documenting the tight
ordinal contract between Kotlin and Rust for AppAction (the enum/variant
ordering used in the switch/when that maps ordinals like 4 ->
AppAction.SelectWallet, 5 -> AppAction.SelectLatestOrNewWallet, 6 ->
AppAction.ChangeNetwork, etc.), warning developers not to reorder, insert, or
remove variants without regenerating bindings and updating the checksum; briefly
list the common pitfalls (reordering, missing regen, checksum update) and where
to update (reference AppAction and the FfiConverterType* readers) so future
maintainers know the exact steps to keep Kotlin/Rust serialization in sync.
🪄 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: 39affd77-239e-4fff-a918-69e4c6c16242
⛔ Files ignored due to path filters (1)
ios/CoveCore/Sources/CoveCore/generated/cove.swiftis excluded by!**/generated/**
📒 Files selected for processing (9)
android/app/src/main/java/org/bitcoinppl/cove/AppManager.ktandroid/app/src/main/java/org/bitcoinppl/cove_core/cove.ktios/Cove/AppManager.swiftrust/src/app.rsrust/src/manager/cloud_backup_manager/ops.rsrust/src/manager/cloud_backup_manager/verify/passkey_auth.rsrust/src/manager/cloud_backup_manager/verify/pending_completion.rsrust/src/manager/cloud_backup_manager/wallets/passkey.rsrust/src/manager/onboarding_manager.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- rust/src/manager/cloud_backup_manager/verify/pending_completion.rs
Split sidebar navigation triggers from low-level route updates and add safe wallet-selection entrypoints across Android and iOS.
Add a dedicated unexpected wallet creation error and propagate it to generated Kotlin and Swift UniFFI bindings. Update restore flow ordering so the active cloud namespace tracks the first successful restore wallet.
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 by CodeRabbit
New Features
Bug Fixes
Refactor
Documentation