Add wallet recovery word status tracking and error handling#665
Add wallet recovery word status tracking and error handling#665kartikangiras wants to merge 1 commit intobitcoinppl:masterfrom
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 25 minutes and 8 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughEnhanced error handling in SecretWordsScreen by replacing generic Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~15 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 tracks whether hot wallet recovery words are accessible on the device and surfaces a warning in the sidebar and on the
Confidence Score: 5/5Safe to merge; all findings are P2 style/polish suggestions with no blocking correctness or security issues. All three comments are P2 (style, UX polish, and a minor visibility concern). No data loss, broken logic, or security issues were found. The core feature — detecting missing recovery words via MnemonicError.NotAvailable and surfacing the warning in the sidebar — is implemented correctly and follows the existing AppManager reconcile pattern. ios/Cove/AppManager.swift (synchronous keychain reads on main thread) and ios/Cove/Flows/SelectedWalletFlow/SecretWordsScreen.swift (loading state rendered with error styling). Important Files Changed
Sequence DiagramsequenceDiagram
participant App as App Startup / Event
participant AM as AppManager
participant KC as Keychain (Mnemonic FFI)
participant SB as SidebarView
participant SW as SecretWordsScreen
App->>AM: init() / reconcile(.walletsChanged)
AM->>AM: refreshWalletRecoveryWordStatus()
loop each hot wallet
AM->>KC: Mnemonic(id:) [blocking, main thread]
alt NotAvailable
KC-->>AM: throws MnemonicError.NotAvailable
AM->>AM: missingWalletIds.insert(wallet.id)
else success
KC-->>AM: Mnemonic object (discarded)
end
end
AM->>AM: walletsMissingRecoveryWords = missingWalletIds
SB->>AM: walletMissingRecoveryWords(wallet)
AM-->>SB: Bool
alt missing
SB->>SB: show red bg + orange warning label
end
SW->>SW: onAppear → auth.lock()
SW->>KC: Mnemonic(id:) [blocking, main thread]
alt success
KC-->>SW: words set → show word grid + QR button
else MnemonicError.NotAvailable
KC-->>SW: loadError = .NotAvailable → show red error box
else MnemonicError.GetWalletKeychain
KC-->>SW: loadError = .GetWalletKeychain → show red error box
else other
KC-->>SW: fallbackErrorMessage → show red error box
end
Reviews (1): Last reviewed commit: "Add wallet recovery word status tracking..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| } else { | ||
| Text(errorMessage ?? "Loading...") | ||
| Text(recoveryWordsErrorMessage) | ||
| .multilineTextAlignment(.leading) | ||
| .font(.callout) | ||
| .foregroundStyle(.white.opacity(0.95)) | ||
| .padding(16) | ||
| .frame(maxWidth: screenWidth * 0.9, alignment: .leading) | ||
| .background(Color.red.opacity(0.16)) | ||
| .overlay { | ||
| RoundedRectangle(cornerRadius: 10) | ||
| .stroke(Color.red.opacity(0.30), lineWidth: 1) | ||
| } | ||
| .cornerRadius(10) | ||
| } |
There was a problem hiding this comment.
The else branch is entered whenever words == nil, which includes the initial render frame before onAppear has had a chance to populate either words or loadError. At that point recoveryWordsErrorMessage returns "Loading…", so users briefly see the red-border error box with "Loading…" — indistinguishable from an actual error. On slower devices or if keychain access is delayed, this persists visibly.
Consider separating the loading state from the error state:
} else if loadError != nil || fallbackErrorMessage != nil {
Text(recoveryWordsErrorMessage)
.multilineTextAlignment(.leading)
.font(.callout)
.foregroundStyle(.white.opacity(0.95))
.padding(16)
.frame(maxWidth: screenWidth * 0.9, alignment: .leading)
.background(Color.red.opacity(0.16))
.overlay {
RoundedRectangle(cornerRadius: 10)
.stroke(Color.red.opacity(0.30), lineWidth: 1)
}
.cornerRadius(10)
} else {
ProgressView()
.tint(.white)
}| var router: Router | ||
| var database: Database | ||
| var wallets: [WalletMetadata] = [] | ||
| var walletsMissingRecoveryWords: Set<WalletId> = [] |
There was a problem hiding this comment.
walletsMissingRecoveryWords should be private
Since walletMissingRecoveryWords(_ wallet:) is already the intended public read interface, the backing Set can be made private to prevent external callers from observing or mutating it directly.
| var walletsMissingRecoveryWords: Set<WalletId> = [] | |
| private var walletsMissingRecoveryWords: Set<WalletId> = [] |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| } | ||
|
|
||
| var recoveryWordsErrorMessage: String { | ||
| if let loadError { |
There was a problem hiding this comment.
combine if let with switch just match directly in switch
| var router: Router | ||
| var database: Database | ||
| var wallets: [WalletMetadata] = [] | ||
| var walletsMissingRecoveryWords: Set<WalletId> = [] |
There was a problem hiding this comment.
why do we need this and refreshing recovery words status on each change to wallets in AppManager? Just check if theres an error in the SecretWordsScreen
kartikangiras
force-pushed
the
seedwords
branch
from
d3a2ac1 to
8f54680
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: 2
🧹 Nitpick comments (1)
ios/Cove/Views/SidebarView.swift (1)
79-87: Avoid competing flexible width in the wallet row.
Text(wallet.name)already expands with.frame(maxWidth: .infinity, alignment: .leading), so the addedSpacer(minLength: 0)is redundant and can make long wallet names truncate/scale earlier. Keep one flexible layout mechanism.♻️ Proposed cleanup
Text(wallet.name) .font(.footnote) .fontWeight(.medium) .foregroundStyle(.white) .lineLimit(1) .minimumScaleFactor(0.80) .frame(maxWidth: .infinity, alignment: .leading) - - Spacer(minLength: 0)As per coding guidelines,
ios/Cove/**/*.swift: “Review SwiftUI view code for proper layout, best practices”.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ios/Cove/Views/SidebarView.swift` around lines 79 - 87, The wallet row uses two competing flexible width mechanisms—Text(wallet.name).frame(maxWidth: .infinity, alignment: .leading) and Spacer(minLength: 0)—which causes premature truncation/scaling; remove the redundant Spacer(minLength: 0) (or alternatively remove the .frame(maxWidth: .infinity) and keep the Spacer) so only one flexible layout mechanism remains, updating the wallet row around Text(wallet.name) to rely on a single approach.
🤖 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/Flows/SelectedWalletFlow/SecretWordsScreen.swift`:
- Around line 30-40: The computed property recoveryWordsErrorMessage treats the
loading state as an error ("Loading...") so the UI callout always shows; change
it to return an optional String (or return nil) for the loading case and only
provide actual error strings for real loadError cases (.NotAvailable,
.GetWalletKeychain, .UnknownWord) or fallbackErrorMessage when present; then
update the view that renders the red error callout (where
recoveryWordsErrorMessage is consumed) to only show the callout when
recoveryWordsErrorMessage is non-nil/non-empty (i.e., when a real error exists)
so normal loading remains neutral.
In `@ios/Cove/Views/SidebarView.swift`:
- Around line 93-96: The overlay using RoundedRectangle(cornerRadius:
10).stroke(Color.clear, lineWidth: 1) inside SidebarView is a no-op and should
either be removed or replaced with a visible warning that is conditionally shown
based on the wallet’s recovery-word availability; update the SidebarView (or the
specific view that contains the .overlay call) to check the wallet/recoveryWords
state and, when recovery words are missing, replace the clear stroke with a
visible badge/overlay (e.g., .stroke(Color.red, lineWidth: 2) or a small
Text/Image warning inside a ZStack) and ensure the UI binds to the wallet model
so the warning appears and disappears correctly.
---
Nitpick comments:
In `@ios/Cove/Views/SidebarView.swift`:
- Around line 79-87: The wallet row uses two competing flexible width
mechanisms—Text(wallet.name).frame(maxWidth: .infinity, alignment: .leading) and
Spacer(minLength: 0)—which causes premature truncation/scaling; remove the
redundant Spacer(minLength: 0) (or alternatively remove the .frame(maxWidth:
.infinity) and keep the Spacer) so only one flexible layout mechanism remains,
updating the wallet row around Text(wallet.name) to rely on a single approach.
🪄 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: 5cc76d53-8aa9-4f92-9160-5ecf8e35b7a2
📒 Files selected for processing (2)
ios/Cove/Flows/SelectedWalletFlow/SecretWordsScreen.swiftios/Cove/Views/SidebarView.swift
| var recoveryWordsErrorMessage: String { | ||
| switch loadError { | ||
| case .some(.NotAvailable): | ||
| "Recovery words are unavailable on this device for this wallet. This can happen after restoring to a new iPhone because Secure Enclave data does not transfer. Restore this wallet from your saved recovery words before sending more BTC to it." | ||
| case .some(.GetWalletKeychain): | ||
| "Cove could not access this wallet's recovery words in the keychain right now." | ||
| case .some(.UnknownWord): | ||
| "Cove could not decode this wallet's recovery words." | ||
| case .none: | ||
| fallbackErrorMessage ?? "Loading..." | ||
| } |
There was a problem hiding this comment.
Don’t render the loading state as an error.
Line 39 returns "Loading...", but lines 75-86 always display it in the red error callout. This can flash a false warning during normal load; keep loading neutral and reserve the callout for real errors.
Proposed adjustment
- var recoveryWordsErrorMessage: String {
+ var recoveryWordsErrorMessage: String? {
switch loadError {
case .some(.NotAvailable):
"Recovery words are unavailable on this device for this wallet. This can happen after restoring to a new iPhone because Secure Enclave data does not transfer. Restore this wallet from your saved recovery words before sending more BTC to it."
case .some(.GetWalletKeychain):
"Cove could not access this wallet's recovery words in the keychain right now."
case .some(.UnknownWord):
"Cove could not decode this wallet's recovery words."
case .none:
- fallbackErrorMessage ?? "Loading..."
+ fallbackErrorMessage
}
} } else {
- Text(recoveryWordsErrorMessage)
- .multilineTextAlignment(.leading)
- .font(.callout)
- .foregroundStyle(.white.opacity(0.95))
- .padding(16)
- .frame(maxWidth: screenWidth * 0.9, alignment: .leading)
- .background(Color.red.opacity(0.16))
- .overlay {
- RoundedRectangle(cornerRadius: 10)
- .stroke(Color.red.opacity(0.30), lineWidth: 1)
- }
- .cornerRadius(10)
+ if let recoveryWordsErrorMessage {
+ Text(recoveryWordsErrorMessage)
+ .multilineTextAlignment(.leading)
+ .font(.callout)
+ .foregroundStyle(.white.opacity(0.95))
+ .padding(16)
+ .frame(maxWidth: screenWidth * 0.9, alignment: .leading)
+ .background(Color.red.opacity(0.16))
+ .overlay {
+ RoundedRectangle(cornerRadius: 10)
+ .stroke(Color.red.opacity(0.30), lineWidth: 1)
+ }
+ .cornerRadius(10)
+ } else {
+ Text("Loading...")
+ .font(.callout)
+ .foregroundStyle(.white.opacity(0.75))
+ }
}Also applies to: 75-86
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ios/Cove/Flows/SelectedWalletFlow/SecretWordsScreen.swift` around lines 30 -
40, The computed property recoveryWordsErrorMessage treats the loading state as
an error ("Loading...") so the UI callout always shows; change it to return an
optional String (or return nil) for the loading case and only provide actual
error strings for real loadError cases (.NotAvailable, .GetWalletKeychain,
.UnknownWord) or fallbackErrorMessage when present; then update the view that
renders the red error callout (where recoveryWordsErrorMessage is consumed) to
only show the callout when recoveryWordsErrorMessage is non-nil/non-empty (i.e.,
when a real error exists) so normal loading remains neutral.
| .overlay { | ||
| RoundedRectangle(cornerRadius: 10) | ||
| .stroke(Color.clear, lineWidth: 1) | ||
| } |
There was a problem hiding this comment.
Make the recovery-words warning visible, or remove this no-op overlay.
This Color.clear stroke is invisible and does not affect layout, so it won’t provide the “My Wallets” warning required by the PR objective. If this is the intended warning hook, wire it to the wallet’s recovery-word availability and use visible styling/content.
🛠️ Example direction
.background(Color.coveLightGray.opacity(0.06))
-.overlay {
- RoundedRectangle(cornerRadius: 10)
- .stroke(Color.clear, lineWidth: 1)
-}
+.overlay {
+ if wallet.recoveryWordsUnavailable {
+ RoundedRectangle(cornerRadius: 10)
+ .stroke(Color.red, lineWidth: 1)
+ }
+}As per coding guidelines, ios/Cove/**/*.swift: “Review SwiftUI view code for proper layout, best practices”.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ios/Cove/Views/SidebarView.swift` around lines 93 - 96, The overlay using
RoundedRectangle(cornerRadius: 10).stroke(Color.clear, lineWidth: 1) inside
SidebarView is a no-op and should either be removed or replaced with a visible
warning that is conditionally shown based on the wallet’s recovery-word
availability; update the SidebarView (or the specific view that contains the
.overlay call) to check the wallet/recoveryWords state and, when recovery words
are missing, replace the clear stroke with a visible badge/overlay (e.g.,
.stroke(Color.red, lineWidth: 2) or a small Text/Image warning inside a ZStack)
and ensure the UI binds to the wallet model so the warning appears and
disappears correctly.
praveenperera
force-pushed
the
seedwords
branch
from
8f54680 to
5a4331f
Compare
| @State private var loadError: MnemonicError? | ||
| @State private var fallbackErrorMessage: String? |
There was a problem hiding this comment.
can you explain why we have 2 error messages here? can we use an enum? and we need to bring the change to android too.
There was a problem hiding this comment.
my initial thought was to have one err check for rust and other the normal try catch, but it was a redundant approach so i shifted to using enum and also applied similar changes to android as well.
kartikangiras
force-pushed
the
seedwords
branch
from
5a4331f to
40dae3c
Compare
|
@kartikangiras can you respond to my comments inline, and resolve or respond to the bots too thanks |
2 participants
Summary
Display a clear warning in My Wallets when a hot wallet's recovery words are unavailable on device. This addresses the issue where users restore to a new iPhone and don't realize their seed words are missing from the Secure Enclave which doesn't transfer between devices.
Fixes #316
Testing
Platform Coverage
Recovery words warning only appears when keychain entry is missing which happens in device restore. Could not simulate this in simulator without manual keychain deletion.
Checklist
Summary by CodeRabbit