Merged
Conversation
The old logic added +1.1 sat/vb offsets between tiers that cascaded, turning API values of (0.1, 0.5, 1.0) into displayed rates of (1.2, 2.3, 3.4). Replace with a 1.0 sat/vb floor and 0.1 minimum tier gaps so low fees correctly show (1.0, 1.1, 1.2) while high fees pass through unchanged.
Use the API-provided minimum_fee as a floor (maxed with MIN_FEE_RATE) when computing slow/derived fee tiers so rates never fall below the API minimum. Update assertions to use to_sat_per_kwu() conversions and add unit tests covering: minimum_fee above the hardcoded floor and minimum_fee lifting the slow tier above the raw midpoint.
* Fix descriptor export for Sparrow compatibility
Export descriptors as a single BIP-389 multipath descriptor with <0;1>
notation and h-hardened notation, matching Coldcard's default format.
BDK's miniscript uses ' (apostrophe) for hardened steps, but Sparrow
normalizes to h before validating the checksum, which causes a mismatch.
The new to_normalized_string() and to_multipath_string() methods on
DescriptorExt produce h-notation strings with correctly recomputed
checksums.
* Add test for descriptor Display omitting checksum
Add a unit test that verifies the alternate Display format ({:#}) for Descriptor omits the checksum. The test parses a wpkh descriptor, compares normal and alternate formatting, and asserts that recomputing the checksum from the alternate body reproduces the normal output. This guards against upstream changes to miniscript/display behavior.
Add IPHONEOS_DEPLOYMENT_TARGET = "18.0" to rust/.cargo/config.toml to set the minimum iOS deployment target. Change reqwest feature from "rustls" to "rustls-no-provider" in rust/Cargo.toml (both direct and workspace entries) and update Cargo.lock accordingly.
Adapt cove-util to the updated rand API by importing rand::RngExt and calling rand::rng().fill(&mut chain_code) directly (removing the intermediate rng binding). Cargo.lock was also updated/regen'd to reflect dependency changes.
Replace rand::Rng with rand::RngExt for methods like random_range, random_bool, random, and fill. Replace rand::RngCore with rand::Rng for next_u32 in deterministic_random_draw.
…leanup - Use loadAndReset instead of pushRoute for import word buttons to prevent infinite alert loop on back press - Swap "Use as Watch Only" and "Use with Hardware Wallet" button order to match iOS - Remove close() call in clearWalletManager to avoid cancelling in-flight reconcile messages, matching iOS behavior - Remove dead code in iOS alertButtons catch-all branch - Regenerate UniFFI Swift bindings
Add rustls dependency to Cargo.toml (including workspace entry) and enable the ring feature so a crypto backend is available for rustls. Initialize the ring provider at startup by calling rustls::crypto::ring::default_provider().install_default().ok() in init_on_start to satisfy reqwest builds that use rustls-no-provider.
Release version bump to 1.2.2 across platforms: Android app versionCode incremented 15->16 and versionName 1.2.1->1.2.2 (android/app/build.gradle.kts); iOS MARKETING_VERSION set to 1.2.2 and CURRENT_PROJECT_VERSION incremented 62->63 (ios/Cove.xcodeproj/project.pbxproj); Rust crate version updated to 1.2.2 in Cargo.toml and reflected in Cargo.lock (rust/). This prepares the codebase for the 1.2.2 release.
Fixes Android crash where rustls-platform-verifier panics without JNI init, poisoning the FIAT_CLIENT/FEE_CLIENT LazyLock singletons. Build reqwest clients with a pre-configured rustls ClientConfig using webpki-roots for cert verification. Also move ring provider install to App::new() so it runs before any LazyLock client initialization.
Replace onScrollGeometryChange (which depends on contentInsets.top that fluctuates during iOS 26 Liquid Glass transitions) with onGeometryChange on the header view, measuring position in stable global coordinates. Add 10pt hysteresis dead zone to prevent threshold oscillation.
The simultaneousGesture on SidebarView could set isDragging=true from a slight finger movement during a button tap, causing the onChange handler to skip the close animation. Also guard onDragEnded against stale gesture data reopening a programmatically closed sidebar.
SwiftUI has a bug where ToolbarPlacementEnvironment.updateValue() enters an infinite loop during _UINavigationParallaxTransition when a .principal toolbar item exists at accessibilityExtraExtraLarge dynamic type size. Bypass .principal entirely by hosting the SwiftUI title content inside a UIHostingController assigned to UIKit's navigationItem.titleView. This preserves the centered title appearance while avoiding the SwiftUI toolbar placement system.
Use .navigationTitle with .toolbarColorScheme(.dark) for screens with
plain static titles on dark backgrounds. Add .navigationTitleView { }
View extension for screens needing custom title content (shield icon,
context menu). Document the iOS 26 .principal toolbar bug in CLAUDE.md.
Switch ML Kit barcode scanning from the play-services thin client to the bundled variant (com.google.mlkit:barcode-scanning) which embeds the ML model in the APK and has zero GMS runtime dependency. Add StrongBox fallback in KeychainAccessor: check device capability via PackageManager before requesting StrongBox, and retry with TEE-only key if StrongBox-backed initialization fails. If StrongBox was not used, the exception propagates unchanged (fail-closed).
Increment CURRENT_PROJECT_VERSION from 65 to 66 in ios/Cove.xcodeproj/project.pbxproj for the affected build configurations to update the app build number.
|
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:
📝 WalkthroughWalkthroughAdds an iOS workaround for an iOS 26 SwiftUI Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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 release PR (v1.2.2) delivers a focused set of bug fixes addressing iOS navigation rendering issues at large accessibility font sizes, a sidebar drag gesture regression, and wallet load-state initialization inconsistencies, alongside an Android StrongBox fallback fix and a barcode-scanning dependency swap. Key changes:
Confidence Score: 5/5Safe to merge — all changes are well-targeted bug fixes with no critical logic errors identified. All findings are P2 or below. The iOS 26 UIKit workaround is clearly documented and correctly implemented. The wallet load-state logic is symmetric between iOS and Android. The customToColor division-by-255 fix corrects a definite rendering bug. The HTTP client lazy-init pattern is sound given the OnceCell guard ensuring single initialization. No files require special attention — NavigationTitleView.swift is worth a quick read as the most complex new addition, but it is well-commented. Important Files Changed
Reviews (3): Last reviewed commit: "Fix validated review findings" | Re-trigger Greptile |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
rust/src/wallet/metadata.rs (1)
51-51:⚠️ Potential issue | 🟡 MinorTypo in comment.
Minor typo: "transactions i the transaction list" should be "transactions in the transaction list".
📝 Suggested fix
- /// Show labels for transactions i the transaction list + /// Show labels for transactions in the transaction listAs per coding guidelines:
rust/**/*.rs: Identify spelling mistakes in comments, string literals, and documentation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/src/wallet/metadata.rs` at line 51, Fix the typo in the doc comment in metadata.rs: change the comment string "Show labels for transactions i the transaction list" to "Show labels for transactions in the transaction list" so the documentation/readme for the related item (the doc comment above the wallet metadata declaration) reads correctly.rust/src/keys.rs (1)
337-344:⚠️ Potential issue | 🔴 CriticalFix moved-value usage in
Fromconversion match arm—proposed fix is incorrect.At Line 343,
erroris matched by value, making the subsequenterror.to_string()call invalid. The proposed fixE::NotMatchingPair.to_string()also won't compile sinceNotMatchingPairis a unit variant without associated data.Use one of these approaches instead:
- Match by reference: Change
match errortomatch &errorto borrow rather than consume.- Hardcode the message: Since
NotMatchingPairis a unit variant with a fixed error message, use:E::NotMatchingPair => Self::UnsupportedDescriptor("descriptors are not a matching external/internal pair".to_string()),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/src/keys.rs` around lines 337 - 344, The From conversion currently consumes `error` and then tries to reuse it; change `match error` to `match &error` to borrow the enum and avoid moved-value issues, adjust the arms to clone the borrowed strings for `E::UnsupportedDescriptor(s)` and `E::UnsupportedDescriptorType(s)` (e.g. `Self::UnsupportedDescriptor(s.clone())`), keep `E::NoOrigin => Self::NoOrigin`, and for `E::NotMatchingPair` return a fixed message (e.g. `Self::UnsupportedDescriptor("descriptors are not a matching external/internal pair".to_string())`) instead of calling `to_string()` on a moved unit variant; these changes should be applied inside the `fn from(error: cove_bdk::descriptor_ext::Error) -> Self` match block referencing `cove_bdk::descriptor_ext::Error` and its variants.
🧹 Nitpick comments (3)
ios/Cove/Flows/SelectedWalletFlow/SecretWordsScreen.swift (1)
110-112: Avoid showing the same screen title twice.The body already renders a prominent
Recovery Wordsheading, so Lines 110-112 repeat the same label in the navigation bar and burn vertical space on an already dense view.🤖 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 110 - 112, Remove the duplicate navigation bar title by deleting the .navigationTitle("Recovery Words") and .navigationBarTitleDisplayMode(.inline) modifiers in SecretWordsScreen so the prominent in-body "Recovery Words" heading remains the primary label; you may keep .toolbarColorScheme(.dark, for: .navigationBar) if you still want a dark nav appearance, but do not render the same string in both the view body and the navigation bar.ios/Cove/Flows/NewWalletFlow/NewWalletSelectScreen.swift (1)
115-115: Drop the duplicated inline title-display modifier.
navigationBarTitleDisplayMode(.inline)appears twice in the same chain; keeping one is clearer.Suggested cleanup
- .navigationBarTitleDisplayMode(.inline) .fileImporter( isPresented: $isImporting, allowedContentTypes: [.plainText, .json] @@ .background(Color.midnightBlue) .navigationTitle("Add New Wallet") .navigationBarTitleDisplayMode(.inline) .toolbarColorScheme(.dark, for: .navigationBar)As per coding guidelines, "Review SwiftUI view code for proper layout, best practices".
Also applies to: 150-152
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ios/Cove/Flows/NewWalletFlow/NewWalletSelectScreen.swift` at line 115, Remove the duplicated navigationBarTitleDisplayMode(.inline) modifier in the SwiftUI view chain for NewWalletSelectScreen (the body of struct NewWalletSelectScreen) — leave a single .navigationBarTitleDisplayMode(.inline) and delete the extra occurrence (also remove the duplicate at the other occurrence around lines noted in the review), ensuring the view modifier chain contains only one inline title-display modifier.ios/Cove/Flows/NewWalletFlow/HotWallet/HotWalletSelectScreen.swift (1)
95-95: Remove duplicatenavigationBarTitleDisplayModemodifier.
.navigationBarTitleDisplayMode(.inline)is applied twice; keeping one improves clarity with no behavior change.Suggested cleanup
- .navigationBarTitleDisplayMode(.inline) .frame(maxHeight: .infinity) .background( Image(.newWalletPattern) @@ .background(Color.midnightBlue) .navigationTitle("Add New Wallet") .navigationBarTitleDisplayMode(.inline) .toolbarColorScheme(.dark, for: .navigationBar)As per coding guidelines, "Review SwiftUI view code for proper layout, best practices".
Also applies to: 106-108
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ios/Cove/Flows/NewWalletFlow/HotWallet/HotWalletSelectScreen.swift` at line 95, HotWalletSelectScreen contains a duplicated SwiftUI modifier navigationBarTitleDisplayMode(.inline); remove the redundant occurrence so the view only applies navigationBarTitleDisplayMode once (keep the single modifier on the HotWalletSelectScreen view body), ensuring you update the HotWalletSelectScreen view to eliminate the duplicate instance (also remove the duplicate around the block referenced in the 106-108 area) to improve clarity without changing behavior.
🤖 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/Security.kt`:
- Around line 20-28: The current catch block around createEncryptedPrefs is too
broad and should only handle security-related failures from StrongBox; replace
the catch of Exception with catching GeneralSecurityException (which covers
StrongBoxUnavailableException) in the code path that uses hasStrongBox and
createEncryptedPrefs so only crypto/hardware unavailability triggers the TEE
fallback; keep rethrowing other exceptions when useStrongBox is false and remove
any message-based detection logic so failures unrelated to StrongBox aren’t
silently masked (refer to hasStrongBox, createEncryptedPrefs,
sharedPreferences/KeychainAccessor to locate the block).
In `@android/app/src/main/java/org/bitcoinppl/cove/WalletManager.kt`:
- Around line 105-111: The constructor currently subscribes to updates
(listenForUpdates(this)) before setting WalletManager.loadState from
rustManager.initialLoadState(), allowing reconcile updates to overwrite fresher
state; fix by initializing loadState from rustManager.initialLoadState() before
calling listenForUpdates(this) (or otherwise deferring subscription until after
the when-block that maps org.bitcoinppl.cove_core.WalletLoadState to
WalletLoadState), ensuring the mapping logic that sets
WalletLoadState.LOADING/SCANNING/LOADED runs prior to any reconcile callbacks.
In `@ios/Cove/Flows/SelectedWalletFlow/SelectedWalletScreen.swift`:
- Around line 510-512: The onAppear handler in SelectedWalletScreen.swift
currently sets shouldShowNavBar = false but does not reset the global header
flag app.isPastHeader, causing a transient toolbar mismatch; update the
.onAppear block (where shouldShowNavBar is changed) to also set app.isPastHeader
= false so the global header state is synchronized with the local navbar state
(i.e., set both shouldShowNavBar = false and app.isPastHeader = false in the
same onAppear).
In `@ios/Cove/Views/NavigationTitleView.swift`:
- Around line 61-65: The dismantleUIViewController implementation
unconditionally calls vc.clearTitle(), which can remove a title view installed
by a newer helper; change dismantleUIViewController in NavigationTitleView.swift
so it only clears the navigation title if this helper still owns it—e.g., have
NavigationTitleViewController/clearTitle check ownership by comparing the stored
titleView reference (or an ownership token/id kept on the controller) against
the navigationItem.titleView (or token) and only remove/clear when they match;
update any place that sets titleView to also set that ownership token so
dismantleUIViewController safely no-ops when another helper replaced the title.
In `@rust/src/app.rs`:
- Line 87: The call to
rustls::crypto::ring::default_provider().install_default() currently uses .ok(),
which drops errors silently; change this to call .expect("failed to install
default crypto provider") so the program panics with a clear message on
failure—update the expression where install_default() is invoked to use expect
with that message to surface configuration errors.
In `@rust/src/http_client.rs`:
- Around line 10-13: The HTTP client initializer currently panics via .expect()
and lacks timeouts; change new_client() to return Result<reqwest::Client,
Box<dyn std::error::Error>> (or an appropriate error type) instead of calling
.expect(), and configure explicit timeouts on the builder (e.g., connect_timeout
and overall timeout) before build; update all call sites that construct the
client (the constructors/factories for FeeClient and FiatClient) to propagate or
handle the Result (return Result from those constructors or handle errors) so
the code no longer panics on builder failure and will time out on
slow/unresponsive servers. Ensure you still use
tls_backend_preconfigured(tls_config) on reqwest::ClientBuilder and
map/propagate build errors rather than unwrapping.
---
Outside diff comments:
In `@rust/src/keys.rs`:
- Around line 337-344: The From conversion currently consumes `error` and then
tries to reuse it; change `match error` to `match &error` to borrow the enum and
avoid moved-value issues, adjust the arms to clone the borrowed strings for
`E::UnsupportedDescriptor(s)` and `E::UnsupportedDescriptorType(s)` (e.g.
`Self::UnsupportedDescriptor(s.clone())`), keep `E::NoOrigin => Self::NoOrigin`,
and for `E::NotMatchingPair` return a fixed message (e.g.
`Self::UnsupportedDescriptor("descriptors are not a matching external/internal
pair".to_string())`) instead of calling `to_string()` on a moved unit variant;
these changes should be applied inside the `fn from(error:
cove_bdk::descriptor_ext::Error) -> Self` match block referencing
`cove_bdk::descriptor_ext::Error` and its variants.
In `@rust/src/wallet/metadata.rs`:
- Line 51: Fix the typo in the doc comment in metadata.rs: change the comment
string "Show labels for transactions i the transaction list" to "Show labels for
transactions in the transaction list" so the documentation/readme for the
related item (the doc comment above the wallet metadata declaration) reads
correctly.
---
Nitpick comments:
In `@ios/Cove/Flows/NewWalletFlow/HotWallet/HotWalletSelectScreen.swift`:
- Line 95: HotWalletSelectScreen contains a duplicated SwiftUI modifier
navigationBarTitleDisplayMode(.inline); remove the redundant occurrence so the
view only applies navigationBarTitleDisplayMode once (keep the single modifier
on the HotWalletSelectScreen view body), ensuring you update the
HotWalletSelectScreen view to eliminate the duplicate instance (also remove the
duplicate around the block referenced in the 106-108 area) to improve clarity
without changing behavior.
In `@ios/Cove/Flows/NewWalletFlow/NewWalletSelectScreen.swift`:
- Line 115: Remove the duplicated navigationBarTitleDisplayMode(.inline)
modifier in the SwiftUI view chain for NewWalletSelectScreen (the body of struct
NewWalletSelectScreen) — leave a single .navigationBarTitleDisplayMode(.inline)
and delete the extra occurrence (also remove the duplicate at the other
occurrence around lines noted in the review), ensuring the view modifier chain
contains only one inline title-display modifier.
In `@ios/Cove/Flows/SelectedWalletFlow/SecretWordsScreen.swift`:
- Around line 110-112: Remove the duplicate navigation bar title by deleting the
.navigationTitle("Recovery Words") and .navigationBarTitleDisplayMode(.inline)
modifiers in SecretWordsScreen so the prominent in-body "Recovery Words" heading
remains the primary label; you may keep .toolbarColorScheme(.dark, for:
.navigationBar) if you still want a dark nav appearance, but do not render the
same string in both the view body and the navigation bar.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a53771d0-dab7-4639-8da9-2811f40a1ef0
⛔ Files ignored due to path filters (3)
ios/CoveCore/Sources/CoveCore/generated/cove.swiftis excluded by!**/generated/**ios/CoveCore/Sources/CoveCore/generated/cove_device.swiftis excluded by!**/generated/**rust/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (42)
ARCHITECTURE.mdandroid/app/build.gradle.ktsandroid/app/proguard-rules.proandroid/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/Security.ktandroid/app/src/main/java/org/bitcoinppl/cove/WalletManager.ktandroid/app/src/main/java/org/bitcoinppl/cove_core/cove.ktios/Cove.xcodeproj/project.pbxprojios/Cove/CoveApp.swiftios/Cove/Extention/WalletColor+Ext.swiftios/Cove/Flows/NewWalletFlow/HotWallet/HotWalletCreateScreen.swiftios/Cove/Flows/NewWalletFlow/HotWallet/HotWalletImportScreen.swiftios/Cove/Flows/NewWalletFlow/HotWallet/HotWalletSelectScreen.swiftios/Cove/Flows/NewWalletFlow/HotWallet/VerifyWords/VerifyWordsScreen.swiftios/Cove/Flows/NewWalletFlow/NewWalletSelectScreen.swiftios/Cove/Flows/SelectedWalletFlow/MoreInfoPopover.swiftios/Cove/Flows/SelectedWalletFlow/SecretWordsScreen.swiftios/Cove/Flows/SelectedWalletFlow/SelectedWalletScreen.swiftios/Cove/Flows/SelectedWalletFlow/WalletBalanceHeaderView.swiftios/Cove/SidebarContainer.swiftios/Cove/Views/NavigationTitleView.swiftios/Cove/WalletManager.swiftrust/.cargo/config.tomlrust/Cargo.tomlrust/crates/cove-bdk/src/coin_selection/deterministic_random_draw.rsrust/crates/cove-bdk/src/descriptor_ext.rsrust/crates/cove-bip39/src/lib.rsrust/crates/cove-types/src/transaction/sent_and_received.rsrust/crates/cove-util/src/lib.rsrust/src/app.rsrust/src/database/historical_price/record.rsrust/src/fee_client.rsrust/src/fiat/client.rsrust/src/http_client.rsrust/src/keys.rsrust/src/lib.rsrust/src/manager/wallet_manager.rsrust/src/manager/wallet_manager/actor.rsrust/src/mnemonic/number_of_bip39_words.rsrust/src/transaction/ffi.rsrust/src/wallet/metadata.rs
💤 Files with no reviewable changes (1)
- android/app/src/main/java/org/bitcoinppl/cove/AppManager.kt
There was a problem hiding this comment.
🧹 Nitpick comments (1)
ios/Cove/CoveMainView.swift (1)
584-586: Guard deferredidreset against stale route state.Line 585 schedules
idreset for the next run loop; if routes are repopulated before the closure runs, the view can still be force-reset unnecessarily. Gate the reset on current route state inside the async block.Proposed change
if !old.isEmpty, new.isEmpty { - DispatchQueue.main.async { id = UUID() } + DispatchQueue.main.async { + guard app.router.routes.isEmpty else { return } + id = UUID() + } }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/CoveMainView.swift` around lines 584 - 586, The deferred id reset schedules a UUID change without verifying the route state at execution time, which can cause an unnecessary reset if routes are repopulated before the closure runs; inside the DispatchQueue.main.async closure, re-check the current route collection (e.g., the view's routes/currentRoutes property or the viewModel's routes) and only set id = UUID() if that collection is still empty, and capture self weakly if the closure references self or a view model to avoid retain cycles.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@ios/Cove/CoveMainView.swift`:
- Around line 584-586: The deferred id reset schedules a UUID change without
verifying the route state at execution time, which can cause an unnecessary
reset if routes are repopulated before the closure runs; inside the
DispatchQueue.main.async closure, re-check the current route collection (e.g.,
the view's routes/currentRoutes property or the viewModel's routes) and only set
id = UUID() if that collection is still empty, and capture self weakly if the
closure references self or a view model to avoid retain cycles.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4338a942-3bf6-4304-8b06-a261e23cf9c4
⛔ Files ignored due to path filters (7)
ios/CoveCore/Sources/CoveCore/generated/cove.swiftis excluded by!**/generated/**ios/CoveCore/Sources/CoveCore/generated/cove_device.swiftis excluded by!**/generated/**ios/CoveCore/Sources/CoveCore/generated/cove_nfc.swiftis excluded by!**/generated/**ios/CoveCore/Sources/CoveCore/generated/cove_tap_card.swiftis excluded by!**/generated/**ios/CoveCore/Sources/CoveCore/generated/cove_types.swiftis excluded by!**/generated/**ios/CoveCore/Sources/CoveCore/generated/cove_ur.swiftis excluded by!**/generated/**rust/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (11)
ARCHITECTURE.mdandroid/app/build.gradle.ktsandroid/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_core/cove.ktios/Cove/CoveMainView.swiftios/Cove/Flows/SelectedWalletFlow/MoreInfoPopover.swiftios/Cove/WalletManager.swiftrust/src/app.rsrust/src/manager/wallet_manager.rsrust/xtask/src/ios.rs
💤 Files with no reviewable changes (1)
- android/app/src/main/java/org/bitcoinppl/cove/AppManager.kt
✅ Files skipped from review due to trivial changes (3)
- ios/Cove/Flows/SelectedWalletFlow/MoreInfoPopover.swift
- rust/xtask/src/ios.rs
- ARCHITECTURE.md
🚧 Files skipped from review as they are similar to previous changes (3)
- android/app/src/main/java/org/bitcoinppl/cove/MainActivity.kt
- rust/src/manager/wallet_manager.rs
- ios/Cove/WalletManager.swift
Contributor
Author
|
@greptile-apps re-review |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
rust/src/manager/wallet_manager.rs (1)
161-162: Consider making the startup snapshot one-shot.
initial_load_statecan hold the full cached transaction list, while the same history is already retained insideWalletActor. Keeping another owned copy onRustWalletManagerfor its full lifetime adds avoidable memory pressure, and because the object derivesClone, this is also the field that can deep-copy that history on manager clones. If native code only reads this once, anOption<WalletLoadState>plus atake_initial_load_state()accessor would let you release it immediately after bridging.As per coding guidelines,
rust/**/*.rs: Check for idiomatic Rust usage and performance best practices.Also applies to: 318-320
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/src/manager/wallet_manager.rs` around lines 161 - 162, Change RustWalletManager.initial_load_state from owning WalletLoadState to an Option<WalletLoadState> so startup snapshot can be consumed and dropped immediately; add a take_initial_load_state() method on RustWalletManager that returns Option<WalletLoadState> (taking ownership and leaving None) and update any places that read initial_load_state to call that accessor once (e.g., bridge to native or initialize WalletActor), then remove the retained copy so clones of RustWalletManager no longer deep-copy the history and memory pressure is reduced.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@rust/src/manager/wallet_manager.rs`:
- Around line 161-162: Change RustWalletManager.initial_load_state from owning
WalletLoadState to an Option<WalletLoadState> so startup snapshot can be
consumed and dropped immediately; add a take_initial_load_state() method on
RustWalletManager that returns Option<WalletLoadState> (taking ownership and
leaving None) and update any places that read initial_load_state to call that
accessor once (e.g., bridge to native or initialize WalletActor), then remove
the retained copy so clones of RustWalletManager no longer deep-copy the history
and memory pressure is reduced.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4345bfd8-607b-4b04-8b8d-195fd9a9571e
📒 Files selected for processing (14)
android/app/src/main/java/org/bitcoinppl/cove/Security.ktandroid/app/src/main/java/org/bitcoinppl/cove/WalletManager.ktios/Cove/Flows/NewWalletFlow/HotWallet/HotWalletSelectScreen.swiftios/Cove/Flows/NewWalletFlow/NewWalletSelectScreen.swiftios/Cove/Flows/SelectedWalletFlow/SecretWordsScreen.swiftios/Cove/Flows/SelectedWalletFlow/SelectedWalletScreen.swiftios/Cove/Views/NavigationTitleView.swiftrust/crates/cove-http/src/lib.rsrust/src/app.rsrust/src/fee_client.rsrust/src/fiat/client.rsrust/src/keys.rsrust/src/manager/wallet_manager.rsrust/src/wallet/metadata.rs
✅ Files skipped from review due to trivial changes (2)
- rust/src/wallet/metadata.rs
- android/app/src/main/java/org/bitcoinppl/cove/WalletManager.kt
🚧 Files skipped from review as they are similar to previous changes (5)
- ios/Cove/Flows/SelectedWalletFlow/SecretWordsScreen.swift
- ios/Cove/Flows/NewWalletFlow/HotWallet/HotWalletSelectScreen.swift
- rust/src/app.rs
- ios/Cove/Flows/NewWalletFlow/NewWalletSelectScreen.swift
- ios/Cove/Flows/SelectedWalletFlow/SelectedWalletScreen.swift
There was a problem hiding this comment.
🧹 Nitpick comments (1)
ios/Cove/AppManager.swift (1)
8-8: Extract the bootstrap gate into a shared helper.
ios/Cove/AppManager.swiftandios/Cove/AuthManager.swiftnow carry the same preview bypass and.completecheck. Centralizing that logic will keep both singletons on the same startup invariant and avoid drift later.Possible cleanup
+enum BootstrapGuard { + static func requireComplete(for managerName: String) { + if ProcessInfo.processInfo.environment["XCODE_RUNNING_FOR_PREVIEWS"] == "1" { + return + } + + let step = bootstrapProgress() + guard step == .complete else { + fatalError("\(managerName) initialized before bootstrap completed: \(step)") + } + } +} + `@Observable` final class AppManager: FfiReconcile { static let shared = makeShared() ... private static func makeShared() -> AppManager { - requireBootstrapComplete() + BootstrapGuard.requireComplete(for: "AppManager") return AppManager() }Also applies to: 64-78
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ios/Cove/AppManager.swift` at line 8, Extract the shared “bootstrap gate” logic (the preview bypass and the .complete check) into a single helper (e.g., BootstrapGate or AppBootstrap) and have both AppManager.makeShared() and the equivalent initializer in AuthManager call that helper rather than duplicating the logic; move the preview bypass and completion-checking code into a single method on the helper (e.g., BootstrapGate.waitIfNeeded() or BootstrapGate.isComplete()) and replace the duplicated blocks in AppManager and AuthManager with calls to that method so both singletons share the same startup invariant.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@ios/Cove/AppManager.swift`:
- Line 8: Extract the shared “bootstrap gate” logic (the preview bypass and the
.complete check) into a single helper (e.g., BootstrapGate or AppBootstrap) and
have both AppManager.makeShared() and the equivalent initializer in AuthManager
call that helper rather than duplicating the logic; move the preview bypass and
completion-checking code into a single method on the helper (e.g.,
BootstrapGate.waitIfNeeded() or BootstrapGate.isComplete()) and replace the
duplicated blocks in AppManager and AuthManager with calls to that method so
both singletons share the same startup invariant.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: bcb16d6d-ddaa-41a4-a30d-75ed2f0708a6
📒 Files selected for processing (6)
android/app/src/main/java/org/bitcoinppl/cove/AppManager.ktandroid/app/src/main/java/org/bitcoinppl/cove/AuthManager.ktios/Cove/AppManager.swiftios/Cove/AuthManager.swiftrust/src/app.rsrust/src/bootstrap.rs
💤 Files with no reviewable changes (1)
- rust/src/app.rs
This was referenced Apr 9, 2026
Closed
7 tasks
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
Improvements
Chores