Add wallet recovery word status tracking and error handling#665
Add wallet recovery word status tracking and error handling#665kartikangiras wants to merge 2 commits into
Conversation
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ 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 (2)
📝 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}
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
praveenperera
force-pushed
the
seedwords
branch
from
8f54680 to
5a4331f
Compare
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 |
kartikangiras
force-pushed
the
seedwords
branch
from
40dae3c to
b5ca4b0
Compare
|
@praveenperera I have address all the inline review comments and from bots as well. |
| Text( | ||
| text = errorMessage ?: stringResource(R.string.label_loading), | ||
| color = Color.White.copy(alpha = 0.7f), | ||
| text = loadError?.message ?: stringResource(R.string.label_loading), |
There was a problem hiding this comment.
loadError?.message ?: stringResource(R.string.label_loading) is rendered inside the same red error-styled Text, so the normal initial loading state still appears as an error on Android while Mnemonic(id = walletId) is running. Split this branch like the iOS version: show a neutral loading view when loadError == null, and only apply the red background/padding to actual RecoveryWordsLoadError messages.
|
Closing this for now after maintainer triage. The branch is conflicting, CI is not green, and the PR scope/title no longer matches the current diff. Please reopen or send a fresh PR after rebasing, fixing the Android loading/error state, and aligning the scope with the implementation. |
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