Rust conditional loading popup #483
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds a loading-popup helper in Rust that conditionally shows a short-lived UI spinner around long-running exports, exposes new async label/transaction export APIs via the Rust wallet manager and FFI, updates reconcile messages for popup control, and simplifies mobile export/QR UI flows to use the new APIs. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as Mobile UI (Android / iOS)
participant FFI as Platform FFI (Kotlin / Swift)
participant Rust as Rust WalletManager
participant Updater as Updater/Reconciler
participant Op as Export Operation
UI->>FFI: user triggers export (share / QR / CSV)
FFI->>Rust: uniffi async call export_*
Rust->>Op: start export operation
Rust->>Rust: start short delay timer
par concurrent
Op->>Op: perform export work (blocking/async)
Rust->>Rust: await delay
end
alt operation finishes before delay
Op-->>Rust: return result (content, filename)
Rust-->>FFI: return result (no popup)
FFI-->>UI: present ShareSheet / QR
else operation exceeds delay
Rust->>Updater: send_update(ShowLoadingPopup)
Updater->>UI: show loading popup
Op-->>Rust: return result
Rust->>Rust: ensure minimum visible time
Rust->>Updater: send_update(HideLoadingPopup)
Updater->>UI: hide loading popup
Rust-->>FFI: return result
FFI-->>UI: present ShareSheet / QR
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~35 minutes
Possibly related PRs
Suggested labels
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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 centralizes loading popup logic into Rust, replacing platform-specific implementations with a unified conditional loading popup system. The new Key improvements:
All export flows simplified to single async call + error handling, delegating loading UX to Rust layer. Confidence Score: 5/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant UI as UI Layer (iOS/Android)
participant Rust as Rust Manager
participant LoadingPopup as with_loading_popup
participant LabelMgr as LabelManager
participant TaskPool as spawn_blocking
participant Frontend as Frontend (Updater)
Note over UI,Frontend: Export Labels for QR Code Flow
UI->>Rust: exportLabelsForQr(density)
Rust->>LoadingPopup: with_loading_popup(async { ... })
par Biased Select (Fast Path vs Delayed Popup)
LoadingPopup->>LabelMgr: export_to_bbqr_with_density()
LabelMgr->>LabelMgr: export() (async)
LabelMgr->>TaskPool: spawn_blocking(db.labels.all_labels())
TaskPool-->>LabelMgr: labels
LabelMgr->>TaskPool: spawn_blocking(BBQr encoding)
TaskPool-->>LabelMgr: QR strings
LabelMgr-->>LoadingPopup: Result<Vec<String>>
and Conditional Popup (After 125ms)
LoadingPopup->>LoadingPopup: sleep(125ms)
LoadingPopup->>Frontend: Updater::send_update(ShowLoadingPopup)
Frontend->>UI: Display loading popup
end
alt Popup was shown
LoadingPopup->>LoadingPopup: Ensure minimum 350ms display
LoadingPopup->>Frontend: Updater::send_update(HideLoadingPopup)
Frontend->>UI: Dismiss popup
end
LoadingPopup-->>Rust: Result<Vec<String>>
Rust-->>UI: QR code strings
UI->>UI: Render animated QR codes
Note over UI,Frontend: Export Transactions CSV Flow
UI->>Rust: exportTransactionsCsv()
Rust->>LoadingPopup: with_loading_popup(async { ... })
LoadingPopup->>Rust: actor.txns_with_prices()
Rust->>TaskPool: spawn_blocking(create CSV)
TaskPool-->>Rust: CSV content + filename
alt Operation > 125ms
LoadingPopup->>Frontend: ShowLoadingPopup
Frontend->>UI: Display loading
Note over LoadingPopup: Ensure 350ms minimum display
LoadingPopup->>Frontend: HideLoadingPopup
Frontend->>UI: Dismiss loading
end
Rust-->>UI: TransactionExportResult
UI->>UI: Present share sheet
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Additional Comments (2)
-
ios/Cove/Flows/SelectedWalletFlow/SelectedWalletScreen.swift, line 298-299 (link)logic: QR export not using new loading popup method - should call
manager.rust.exportLabelsForQr(density: density)instead oflabelManager.exportToBbqrWithDensity -
android/app/src/main/java/org/bitcoinppl/cove/wallet/WalletSheets.kt, line 166 (link)logic: QR export not using new loading popup method - should call
manager.rust.exportLabelsForQr(density)instead oflabelManager.exportToBbqrWithDensity
12 files reviewed, 3 comments
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (5)
ios/Cove/AppManager.swift (1)
278-282: Consider the scope of popup dismissal.The
HideLoadingPopupcase dismisses all popups viaPopupStack.dismissAllPopups(). If there are scenarios where multiple popups might be shown simultaneously, this could inadvertently dismiss unrelated popups.💡 Consider a more targeted dismissal approach
If the loading popup can be tracked, dismissing specifically that popup would be safer:
case .showLoadingPopup: Task { loadingPopup = await MiddlePopup(state: .loading).present() } case .hideLoadingPopup: Task { await loadingPopup?.dismiss() loadingPopup = nil }However, if the current approach is intentional and all popups should be cleared, this is fine.
android/app/src/main/java/org/bitcoinppl/cove/AppManager.kt (1)
580-591: Consider the scope and wording of the loading state.The implementation sets
alertStateto a general alert for loading and clears it on hide. Two considerations:
- Text wording: "Working on it..." is informal. Consider if this matches the app's tone.
- State clearing:
HideLoadingPopupsetsalertState = null, which will clear any alert, not just the loading one. This could inadvertently dismiss error alerts if timing overlaps.💡 Potential improvements
For more precise control, consider tracking the loading alert specifically:
is AppStateReconcileMessage.ShowLoadingPopup -> { val loadingAlert = TaggedItem( AppAlertState.General( title = "Please wait...", message = "", ), ) if (alertState == null) { // Only show if no other alert alertState = loadingAlert } } is AppStateReconcileMessage.HideLoadingPopup -> { // Only clear if it's the loading alert if (alertState?.item is AppAlertState.General) { alertState = null } }However, if the current approach is intentional and all alerts should be cleared, this is acceptable.
rust/src/loading_popup.rs (1)
23-34: Thebiasedselect correctly optimizes for fast operations.The comment on line 23 explains the behavior, but consider adding a note in the function documentation about this optimization to make it more discoverable.
📝 Enhanced documentation
/// Runs an async operation with conditional loading popup -/// Shows popup only if operation takes >125ms, keeps visible for min 400ms +/// Shows popup only if operation takes >125ms, keeps visible for min 400ms. +/// The biased select! prioritizes checking operation completion first, +/// which avoids showing the popup for operations that complete quickly. pub async fn with_loading_popup<F, T, E>(operation: F) -> Result<T, E>android/app/src/main/java/org/bitcoinppl/cove/wallet/MoreInfoPopover.kt (1)
100-134: Consider more specific error handling.The current error handling catches all exceptions generically. If the export methods can return null (as suggested by the
?.operator), that case is handled, but exceptions are broadly caught.Consider providing more specific feedback based on the failure mode:
when (currentExportType) { is ExportType.Transactions -> { val result = currentManager?.rust?.exportTransactionsCsv() if (result == null) { snackbarHostState.showSnackbar("No transactions to export") } else { result.content } } // similar for labels... }ios/Cove/Flows/SelectedWalletFlow/SelectedWalletScreen.swift (1)
158-160: Consider removing the trivial wrapper.The
showQrExport()function is a simple one-line wrapper around a state mutation. Unless this is needed for consistency with other patterns in the codebase, direct state mutation on line 233 would be clearer.💡 Simplification option
- func showQrExport() { - showLabelsQrExport = true - } - func shareLabelsFile() { // ... } // Later in the code: Button("QR Code") { - showQrExport() + showLabelsQrExport = true }However, if the wrapper provides a clear semantic intent or is part of a larger pattern, keeping it is fine.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
ios/CoveCore/Sources/CoveCore/generated/cove.swiftis excluded by!**/generated/**
📒 Files selected for processing (11)
android/app/src/main/java/org/bitcoinppl/cove/AppManager.kt(1 hunks)android/app/src/main/java/org/bitcoinppl/cove/wallet/MoreInfoPopover.kt(1 hunks)android/app/src/main/java/org/bitcoinppl/cove/wallet/WalletSheets.kt(2 hunks)android/app/src/main/java/org/bitcoinppl/cove_core/cove.kt(12 hunks)ios/Cove/AppManager.swift(2 hunks)ios/Cove/Flows/SelectedWalletFlow/MoreInfoPopover.swift(1 hunks)ios/Cove/Flows/SelectedWalletFlow/SelectedWalletScreen.swift(3 hunks)rust/src/app/reconcile.rs(1 hunks)rust/src/lib.rs(1 hunks)rust/src/loading_popup.rs(1 hunks)rust/src/manager/wallet_manager.rs(4 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
rust/**/*.rs
⚙️ CodeRabbit configuration file
rust/**/*.rs: - Verify correct functionality and logic consistency.
- Check for idiomatic Rust usage and performance best practices.
- Suggest improvements to error handling, unwraps, and Result handling.
- Check for any potential memory leaks or unsafe code.
- Check for any potential mutex deadlocks.
- Check for potential security issues, make sure Bitcoin wallets are handled securely.
- Identify spelling mistakes in comments, string literals, and documentation
Files:
rust/src/app/reconcile.rsrust/src/loading_popup.rsrust/src/lib.rsrust/src/manager/wallet_manager.rs
android/app/src/main/java/org/bitcoinppl/cove/**/*.kt
⚙️ CodeRabbit configuration file
android/app/src/main/java/org/bitcoinppl/cove/**/*.kt:⚠️ CRITICAL FFI/Threading Policy - READ FIRST:
NEVER suggest moving Rust FFI calls to background threads (withContext(Dispatchers.IO))
Rust FFI calls are SYNCHRONOUS and FAST - they complete in microseconds
The Rust core uses Tokio runtime internally and handles all async operations
Database operations (redb) are optimized and do NOT block the UI thread
ONLY suggest Dispatchers.IO with profiling evidence showing >16ms UI blocking
If you see a Rust FFI call on the main thread, DO NOT FLAG IT - this is correct
Ignore generated bindings code in android/app/src/main/java/org/bitcoinppl/cove_core/**
Verify correct functionality and logic consistency.
Check for idiomatic Kotlin usage and performance best practices.
FFI Resource Management:
- Always verify that UniFFI objects implementing AutoCloseable call .close() before being nulled
- Example: Mnemonic must call .close() to trigger zeroization of sensitive data
- Use DisposableEffect for cleanup in Compose, never just null references
Compose Best Practices:
- LaunchedEffect should be keyed to actual dependencies, not Unit
- Set loading states at the beginning of LaunchedEffect blocks
Files:
android/app/src/main/java/org/bitcoinppl/cove/wallet/MoreInfoPopover.ktandroid/app/src/main/java/org/bitcoinppl/cove/AppManager.ktandroid/app/src/main/java/org/bitcoinppl/cove/wallet/WalletSheets.kt
ios/Cove/**/*.swift
⚙️ CodeRabbit configuration file
ios/Cove/**/*.swift: - Review SwiftUI view code for proper layout, best practices
- Identify spelling mistakes in comments, string literals, and documentation
- Ignore generated bindings code in ios/CoveCore/Sources/CoveCore/generated/**
Files:
ios/Cove/AppManager.swiftios/Cove/Flows/SelectedWalletFlow/MoreInfoPopover.swiftios/Cove/Flows/SelectedWalletFlow/SelectedWalletScreen.swift
🧬 Code graph analysis (5)
rust/src/loading_popup.rs (3)
ios/Cove/AppManager.swift (1)
reconcile(213-285)android/app/src/main/java/org/bitcoinppl/cove/AppManager.kt (1)
reconcile(506-594)rust/src/app/reconcile.rs (2)
reconcile(69-69)send_update(61-63)
ios/Cove/AppManager.swift (2)
ios/Cove/Flows/SettingsFlow/SettingsScreen/NodeSelectionView.swift (1)
showLoadingPopup(42-49)ios/CoveCore/Sources/CoveCore/generated/cove.swift (2)
state(3056-3062)state(11097-11103)
rust/src/manager/wallet_manager.rs (2)
rust/src/loading_popup.rs (1)
with_loading_popup(14-47)rust/src/label_manager.rs (1)
export_default_file_name(88-95)
ios/Cove/Flows/SelectedWalletFlow/MoreInfoPopover.swift (1)
ios/Cove/ShareSheet.swift (2)
present(38-70)present(77-139)
ios/Cove/Flows/SelectedWalletFlow/SelectedWalletScreen.swift (2)
ios/CoveCore/Sources/CoveCore/generated/cove.swift (2)
exportLabelsForShare(7985-8000)data(10119-10125)ios/Cove/ShareSheet.swift (2)
present(38-70)present(77-139)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Greptile Review
- GitHub Check: test (ubuntu-latest)
- GitHub Check: compile-ios
- GitHub Check: test (macos-latest)
- GitHub Check: compile-android
🔇 Additional comments (15)
rust/src/lib.rs (1)
18-18: LGTM!The module declaration follows Rust naming conventions and is appropriately positioned in the module list.
ios/Cove/AppManager.swift (1)
1-1: LGTM!The import is necessary for the popup functionality added in this PR.
rust/src/app/reconcile.rs (1)
34-35: LGTM!The new enum variants are correctly defined and follow the existing pattern for reconcile messages.
android/app/src/main/java/org/bitcoinppl/cove/wallet/MoreInfoPopover.kt (1)
100-110: Excellent simplification - FFI calls correctly remain on the main thread.The export flow now delegates to the new Rust methods that handle loading popup timing internally. The code correctly keeps FFI calls (
exportTransactionsCsv(),exportLabelsForShare()) on the main thread and only moves file I/O toDispatchers.IO.Based on coding guidelines: Rust FFI calls are synchronous and fast, with async operations handled internally by the Tokio runtime.
ios/Cove/Flows/SelectedWalletFlow/SelectedWalletScreen.swift (1)
162-178: LGTM! Clean integration with the new export API.The refactored
shareLabelsFile()properly uses async/await to call the new Rust export method and includes error handling with user feedback.android/app/src/main/java/org/bitcoinppl/cove/wallet/WalletSheets.kt (2)
122-131: Proper resource cleanup before async operations.The
exportLabelManageris correctly closed and nulled before launching the async share operation, ensuring resources are released promptly.
237-262: LGTM! Correct FFI usage and I/O dispatching.The refactored function correctly:
- Calls the Rust FFI method on the main thread (as it should be)
- Moves only the file I/O operations to
Dispatchers.IO- Uses the simplified export API that returns a ready-to-use result
Based on coding guidelines: Rust FFI calls should remain on the main thread, with only actual I/O operations dispatched to background threads.
ios/Cove/Flows/SelectedWalletFlow/MoreInfoPopover.swift (1)
40-56: LGTM! Clean simplification of export flow.The refactored implementation correctly delegates the export logic to Rust (which handles the loading popup) and uses the standard ShareSheet pattern for sharing. Error handling is appropriate with clear user feedback.
rust/src/manager/wallet_manager.rs (3)
123-133: LGTM! Clean data structures for export results.The new
LabelExportResultandTransactionExportResultstructs are properly defined with uniffi annotations for FFI compatibility.
402-414: LGTM! Proper async implementation with loading popup integration.The method correctly:
- Clones necessary data before the async block
- Releases the RwLock immediately to avoid holding locks across await points
- Delegates to the
with_loading_popuphelper for consistent UX- Uses proper error propagation with the
?operator
416-425: LGTM! Clean implementation.Straightforward delegation to the label manager with proper loading popup integration.
android/app/src/main/java/org/bitcoinppl/cove_core/cove.kt (4)
1434-1439: LGTM: Standard UniFFI FFI declarations and checksum validation.The external function declarations and checksum validations follow the expected UniFFI patterns. The runtime checksum validation will catch any mismatches between the Kotlin bindings and Rust implementation.
Also applies to: 2380-2385, 3820-3828
17988-18001: LGTM: Clear interface definitions with proper documentation.The interface declarations are well-documented and the method signatures are appropriate for their export functionality.
24843-24877: LGTM: Well-structured data classes with proper FFI converters.Both
LabelExportResultandTransactionExportResultfollow the same clean structure and have properly implemented FFI converters for serialization/deserialization across the Rust-Kotlin boundary.Also applies to: 25423-25457
26421-26424: LGTM: Proper enum variant additions for loading popup state.The two new
AppStateReconcileMessagevariants (ShowLoadingPopupandHideLoadingPopup) are correctly implemented across all required FFI methods (destroy, read, allocationSize, write) following the established pattern for unit variants.Also applies to: 26509-26512, 26564-26565, 26660-26671, 26739-26746
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
ios/Cove/Flows/SelectedWalletFlow/SelectedWalletScreen.swift (1)
293-305: Consider using the new Rust API for consistency.Line 298 still directly calls
labelManager.exportToBbqrWithDensity(density: density), while the share flow (line 164) now uses the newmanager.rust.exportLabelsForShare()API.The Rust backend provides
exportLabelsForQr(density:)with loading popup support. Consider updating line 298 to use this new API for consistency and to benefit from the loading popup during QR generation:🔎 Optional refactor
generateBbqrStrings: { density in - try labelManager.exportToBbqrWithDensity(density: density) + try await manager.rust.exportLabelsForQr(density: density) },Note: This would require making the closure async, which may need adjustments to the
QrExportViewcomponent.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
ios/Cove/Flows/SelectedWalletFlow/SelectedWalletScreen.swift(2 hunks)rust/src/app/reconcile.rs(2 hunks)rust/src/manager/wallet_manager.rs(4 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
rust/**/*.rs
⚙️ CodeRabbit configuration file
rust/**/*.rs: - Verify correct functionality and logic consistency.
- Check for idiomatic Rust usage and performance best practices.
- Suggest improvements to error handling, unwraps, and Result handling.
- Check for any potential memory leaks or unsafe code.
- Check for any potential mutex deadlocks.
- Check for potential security issues, make sure Bitcoin wallets are handled securely.
- Identify spelling mistakes in comments, string literals, and documentation
Files:
rust/src/app/reconcile.rsrust/src/manager/wallet_manager.rs
ios/Cove/**/*.swift
⚙️ CodeRabbit configuration file
ios/Cove/**/*.swift: - Review SwiftUI view code for proper layout, best practices
- Identify spelling mistakes in comments, string literals, and documentation
- Ignore generated bindings code in ios/CoveCore/Sources/CoveCore/generated/**
Files:
ios/Cove/Flows/SelectedWalletFlow/SelectedWalletScreen.swift
🧬 Code graph analysis (3)
rust/src/app/reconcile.rs (1)
rust/src/app.rs (2)
global(121-123)global(261-263)
rust/src/manager/wallet_manager.rs (2)
rust/src/loading_popup.rs (1)
with_loading_popup(14-47)rust/src/label_manager.rs (1)
export_default_file_name(88-95)
ios/Cove/Flows/SelectedWalletFlow/SelectedWalletScreen.swift (3)
android/app/src/main/java/org/bitcoinppl/cove/wallet/WalletSheets.kt (1)
shareLabelsFile(237-263)ios/CoveCore/Sources/CoveCore/generated/cove.swift (2)
exportLabelsForShare(7985-8000)data(10119-10125)ios/Cove/ShareSheet.swift (2)
present(38-70)present(77-139)
🪛 GitHub Check: rustfmt
rust/src/manager/wallet_manager.rs
[warning] 446-446:
Diff in /home/runner/work/cove/cove/rust/src/manager/wallet_manager.rs
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: compile-ios
- GitHub Check: clippy
- GitHub Check: test (macos-latest)
- GitHub Check: test (ubuntu-latest)
- GitHub Check: ktlint
- GitHub Check: compile-android
🔇 Additional comments (8)
rust/src/app/reconcile.rs (2)
34-35: LGTM! Clean enum variants for loading popup control.The new
ShowLoadingPopupandHideLoadingPopupvariants integrate well with the loading popup mechanism and enable coordinated UI feedback across the frontend.
62-64: LGTM! Improved error handling for frontend disconnection.Logging the error instead of panicking is the correct approach when the frontend disconnects. This prevents the backend from crashing and maintains service availability.
rust/src/manager/wallet_manager.rs (4)
123-133: LGTM! Well-structured export result types.The
LabelExportResultandTransactionExportResultstructs are clean and properly annotated for FFI export, providing a clear interface for sharing export content and filenames across the language boundary.
402-414: LGTM! Clean implementation with loading popup support.The method correctly integrates the loading popup mechanism and exports labels with appropriate filename generation.
416-425: LGTM! Proper QR export with loading popup.The method appropriately wraps the QR export operation with the loading popup for better UX during potentially slow operations.
427-459: LGTM! Excellent improvements addressing previous review concerns.This implementation successfully addresses both past review comments:
Fixed error handling (lines 434-437): Replaced the unsafe double
.unwrap().unwrap()with proper error mapping usingmap_err, converting both the actor call failure and the inner result error into appropriate error types.Implemented filename sanitization (lines 444-453): The wallet name is now properly sanitized before use in the filename, removing invalid filesystem characters and providing a safe fallback, matching the pattern used in
label_manager.rs.The loading popup integration is correct, and the overall implementation is robust.
Note: There's a minor rustfmt formatting hint on line 446, but this will be automatically resolved by running
cargo fmt.ios/Cove/Flows/SelectedWalletFlow/SelectedWalletScreen.swift (2)
157-159: LGTM! Good refactoring for code reusability.The extracted
showQrExport()helper method improves code consistency and makes the intent clearer.
161-177: LGTM! Excellent integration with the new Rust export API.The updated implementation properly:
- Uses async/await for the new
exportLabelsForShare()API- Handles errors gracefully with user-friendly alerts
- Leverages the loading popup support from the Rust backend
- Simplifies the code by removing manual filename construction
praveenperera
force-pushed
the
rust-conditional-loading-popup
branch
from
04dedeb to
2687ac9
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
rust/src/manager/wallet_manager.rs (1)
429-456: Excellent error handling and filename sanitization.The implementation properly addresses previous review concerns:
- Lines 434-437: Two-level error handling correctly maps both actor communication errors and
txns_with_prices()errors to appropriateErrorvariants (no more double unwrap)- Lines 444-450: Robust filename sanitization replaces spaces with underscores, removes problematic characters while preserving underscores, and provides a safe fallback
The loading popup wrapper ensures good UX for this potentially long-running operation.
💡 Optional: Extract filename sanitization to shared helper
The sanitization logic at lines 444-450 differs slightly from the similar logic in
label_manager.rs(lines 87-94). Consider extracting this to a shared helper function to ensure consistency and reduce duplication:// In a shared utility module pub fn sanitize_for_filename(name: String, fallback: &str) -> String { let sanitized = name .replace(' ', "_") .replace(|c: char| !c.is_alphanumeric() && c != '_', "") .to_ascii_lowercase(); if sanitized.is_empty() { fallback.to_string() } else { sanitized } }Then both locations could use:
sanitize_for_filename(name, "wallet")orsanitize_for_filename(name, "labels").
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
android/app/src/main/java/org/bitcoinppl/cove/AppManager.kt(1 hunks)ios/Cove/Flows/SelectedWalletFlow/SelectedWalletScreen.swift(2 hunks)rust/src/app/reconcile.rs(2 hunks)rust/src/manager/wallet_manager.rs(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- android/app/src/main/java/org/bitcoinppl/cove/AppManager.kt
- rust/src/app/reconcile.rs
- ios/Cove/Flows/SelectedWalletFlow/SelectedWalletScreen.swift
🧰 Additional context used
📓 Path-based instructions (1)
rust/**/*.rs
⚙️ CodeRabbit configuration file
rust/**/*.rs: - Verify correct functionality and logic consistency.
- Check for idiomatic Rust usage and performance best practices.
- Suggest improvements to error handling, unwraps, and Result handling.
- Check for any potential memory leaks or unsafe code.
- Check for any potential mutex deadlocks.
- Check for potential security issues, make sure Bitcoin wallets are handled securely.
- Identify spelling mistakes in comments, string literals, and documentation
Files:
rust/src/manager/wallet_manager.rs
🧬 Code graph analysis (1)
rust/src/manager/wallet_manager.rs (3)
rust/src/loading_popup.rs (1)
with_loading_popup(14-47)rust/src/label_manager.rs (1)
export_default_file_name(88-95)rust/src/manager/wallet_manager/actor.rs (2)
txns_with_prices(810-832)new(160-176)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: compile-ios
- GitHub Check: test (ubuntu-latest)
- GitHub Check: ktlint
- GitHub Check: clippy
- GitHub Check: test (macos-latest)
- GitHub Check: compile-android
🔇 Additional comments (3)
rust/src/manager/wallet_manager.rs (3)
27-28: LGTM: Clean additions for export functionality.The new imports and data structures are well-organized:
LabelManagerandLabelManagerErrorenable label export operationswith_loading_popupprovides conditional loading UI feedbackLabelExportResultandTransactionExportResultproperly encapsulate export outcomes with content and filename- All types are correctly marked with
uniffiattributes for FFI boundaryAlso applies to: 50-50, 123-133
402-414: LGTM: Proper async pattern with loading popup.The implementation correctly:
- Clones data before moving into the async block (avoiding lock issues)
- Uses
with_loading_popupto show UI feedback only if the operation takes longer than the threshold- Propagates errors using
?operator- Delegates filename generation to the label manager's existing logic
418-425: LGTM: Clean QR export implementation.Follows the same solid pattern as the share export method with appropriate loading popup wrapper and proper error handling.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
android/app/src/main/java/org/bitcoinppl/cove/views/QrExportView.kt (1)
68-82: Set loading state at the beginning of LaunchedEffect.Per Compose best practices, clear
qrStringsat the start so users see "Loading..." during format/density changes instead of stale QR codes.🔎 Proposed fix
// generate QR codes on initial load and when format/density changes LaunchedEffect(selectedFormat, density) { + qrStrings = emptyList() try { qrStrings = when (selectedFormat) { QrExportFormat.BBQR -> generateBbqrStrings(density) QrExportFormat.UR -> generateUrStrings?.invoke(density) ?: generateBbqrStrings(density) } error = null currentIndex = 0 } catch (e: Exception) { error = e.message ?: "Unknown error" - qrStrings = emptyList() } }As per coding guidelines: "Set loading states at the beginning of LaunchedEffect blocks."
ios/Cove/Flows/SelectedWalletFlow/SelectedWalletScreen.swift (1)
157-159: Consider inlining the single-statement helper.The
showQrExport()method only sets a boolean flag. While it provides semantic clarity for the button action, you might consider inlining this directly in the button action for brevity, similar to howshowReceiveSheet()is used. However, keeping it as a separate function is also fine if you anticipate adding additional logic later.rust/src/manager/wallet_manager.rs (1)
391-400: Pre-existing issue: Double unwrap pattern.While not introduced by this PR, the
create_transactions_with_fiat_exportmethod at line 394 still uses.unwrap().unwrap()which can panic. Consider refactoring this in a follow-up to match the improved error handling pattern used in the newexport_transactions_csvmethod (lines 434-437).🔎 Suggested improvement
pub async fn create_transactions_with_fiat_export(&self) -> Result<String, Error> { let fiat_currency = Database::global().global_config.fiat_currency().unwrap_or_default(); - let txns_with_prices = call!(self.actor.txns_with_prices()).await.unwrap().unwrap(); + let txns_with_prices = call!(self.actor.txns_with_prices()) + .await + .map_err(|e| Error::TransactionsRetrievalError(e.to_string()))? + .map_err(|e| Error::GetHistoricalPricesError(e.to_string()))?; let report = HistoricalFiatPriceReport::new(fiat_currency, txns_with_prices); let csv = report.create_csv().map_err_str(Error::CsvCreationError)?; Ok(csv.into_string()) }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ 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/views/QrExportView.kt(2 hunks)ios/Cove/Flows/SelectedWalletFlow/SelectedWalletScreen.swift(3 hunks)ios/Cove/Views/QrExportView.swift(3 hunks)rust/src/label_manager.rs(2 hunks)rust/src/loading_popup.rs(1 hunks)rust/src/manager/wallet_manager.rs(4 hunks)rust/src/task.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- rust/src/loading_popup.rs
🧰 Additional context used
📓 Path-based instructions (3)
ios/Cove/**/*.swift
⚙️ CodeRabbit configuration file
ios/Cove/**/*.swift: - Review SwiftUI view code for proper layout, best practices
- Identify spelling mistakes in comments, string literals, and documentation
- Ignore generated bindings code in ios/CoveCore/Sources/CoveCore/generated/**
Files:
ios/Cove/Flows/SelectedWalletFlow/SelectedWalletScreen.swiftios/Cove/Views/QrExportView.swift
rust/**/*.rs
⚙️ CodeRabbit configuration file
rust/**/*.rs: - Verify correct functionality and logic consistency.
- Check for idiomatic Rust usage and performance best practices.
- Suggest improvements to error handling, unwraps, and Result handling.
- Check for any potential memory leaks or unsafe code.
- Check for any potential mutex deadlocks.
- Check for potential security issues, make sure Bitcoin wallets are handled securely.
- Identify spelling mistakes in comments, string literals, and documentation
Files:
rust/src/label_manager.rsrust/src/task.rsrust/src/manager/wallet_manager.rs
android/app/src/main/java/org/bitcoinppl/cove/**/*.kt
⚙️ CodeRabbit configuration file
android/app/src/main/java/org/bitcoinppl/cove/**/*.kt:⚠️ CRITICAL FFI/Threading Policy - READ FIRST:
NEVER suggest moving Rust FFI calls to background threads (withContext(Dispatchers.IO))
Rust FFI calls are SYNCHRONOUS and FAST - they complete in microseconds
The Rust core uses Tokio runtime internally and handles all async operations
Database operations (redb) are optimized and do NOT block the UI thread
ONLY suggest Dispatchers.IO with profiling evidence showing >16ms UI blocking
If you see a Rust FFI call on the main thread, DO NOT FLAG IT - this is correct
Ignore generated bindings code in android/app/src/main/java/org/bitcoinppl/cove_core/**
Verify correct functionality and logic consistency.
Check for idiomatic Kotlin usage and performance best practices.
FFI Resource Management:
- Always verify that UniFFI objects implementing AutoCloseable call .close() before being nulled
- Example: Mnemonic must call .close() to trigger zeroization of sensitive data
- Use DisposableEffect for cleanup in Compose, never just null references
Compose Best Practices:
- LaunchedEffect should be keyed to actual dependencies, not Unit
- Set loading states at the beginning of LaunchedEffect blocks
Files:
android/app/src/main/java/org/bitcoinppl/cove/views/QrExportView.kt
🧬 Code graph analysis (3)
ios/Cove/Flows/SelectedWalletFlow/SelectedWalletScreen.swift (3)
ios/CoveCore/Sources/CoveCore/generated/cove.swift (3)
exportLabelsForShare(8002-8017)data(10136-10142)exportToBbqrWithDensity(4781-4796)ios/Cove/ShareSheet.swift (2)
present(38-70)present(77-139)ios/CoveCore/Sources/CoveCore/generated/cove_nfc.swift (1)
data(1081-1087)
rust/src/label_manager.rs (1)
rust/src/task.rs (1)
spawn_blocking(50-56)
rust/src/manager/wallet_manager.rs (2)
rust/src/loading_popup.rs (1)
with_loading_popup(13-46)rust/src/label_manager.rs (1)
export_default_file_name(88-95)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: clippy
- GitHub Check: test (ubuntu-latest)
- GitHub Check: test (macos-latest)
- GitHub Check: compile-android
- GitHub Check: ktlint
- GitHub Check: compile-ios
🔇 Additional comments (14)
android/app/src/main/java/org/bitcoinppl/cove/views/QrExportView.kt (2)
55-56: LGTM!The signature change to suspend function types is well-designed, enabling callers to provide async QR generation while maintaining backward compatibility with synchronous lambdas.
361-367: LGTM!The convenience overload correctly passes FFI-backed lambdas to the suspend function parameters. Per coding guidelines, Rust FFI calls are synchronous and fast, so no threading changes are needed.
ios/Cove/Flows/SelectedWalletFlow/SelectedWalletScreen.swift (2)
161-177: LGTM!The async export flow properly awaits the Rust API, destructures the result, and handles errors by presenting an alert. The error message includes the localized description for user-friendly feedback.
293-305: LGTM!The QR export sheet correctly uses the updated async closure signature for
generateBbqrStrings, properly awaiting theexportToBbqrWithDensitycall.rust/src/task.rs (1)
50-56: LGTM!The
spawn_blockingwrapper correctly delegates to Tokio's blocking executor with appropriate trait bounds. Theexpect()panic on uninitialized runtime is consistent with the existingspawn()andblock_on()patterns in this module.ios/Cove/Views/QrExportView.swift (3)
66-74: LGTM!The async adoption is well-structured. Using
.taskfor initial generation and wrappingonChangehandlers inTask { await ... }blocks is the correct SwiftUI pattern for triggering async work from synchronous view lifecycle events.
213-223: LGTM!The convenience initializer correctly provides backwards compatibility. The closures contain synchronous throwing calls that are valid within an
async throwsclosure context since synchronous code executes normally in an async function.
188-208: LGTM!The
generateQrCodes()function properly handles async operations with appropriate error handling that updates UI state on failure.rust/src/label_manager.rs (2)
242-255: LGTM!The
export()method correctly offloads database I/O to a blocking task. Thedb.clone()before the move is necessary for the'staticclosure requirement, and the nested error handling properly maps both the innerResultand the outerJoinError.
258-293: LGTM!The async/blocking separation is well-designed: the async
export()runs first, then CPU-intensive BBQr encoding with Zlib compression is offloaded to a blocking task. Themax_versionextraction before the closure avoids borrow issues.rust/src/manager/wallet_manager.rs (4)
123-133: LGTM!The
LabelExportResultandTransactionExportResultstructs provide clean data containers for the export APIs, properly exposed viauniffi::Recordfor FFI.
402-414: LGTM!The
export_labels_for_sharemethod correctly clones necessary data before the async move, wraps the operation in a loading popup for UX, and properly propagates errors.
416-425: LGTM!Clean wrapper that adds loading popup behavior around the existing label manager method.
427-460: LGTM!The
export_transactions_csvmethod properly:
- Uses
with_loading_popupfor UX- Handles actor call errors with descriptive error types (addressing prior review)
- Offloads CPU-bound CSV generation to
spawn_blocking- Sanitizes the wallet name for safe filenames (addressing prior review)
- Falls back to "wallet" if sanitization produces an empty string
|
@greptile-apps review pr |
Greptile's behavior is changing!From now on, if a review finishes with no comments, we will not post an additional "statistics" comment to confirm that our review found nothing to comment on. However, you can confirm that we reviewed your changes in the status check section. This feature can be toggled off in your Code Review Settings by deselecting "Create a status check for each PR". |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
rust/src/loading_popup.rs (1)
13-46: Excellent async helper design with strong Rust practices.The implementation demonstrates idiomatic async Rust:
- Correct use of
tokio::pin!for polling the future multiple times withinselect!biased;prioritizes operation completion, avoiding unnecessary popup for fast operations- Proper minimum display duration prevents UI flicker
- Generic
Result<T, E>preserves the caller's error type- No unsafe code, unwraps, or memory concerns
Note: The concern in the previous review about
Updater::send_update()panicking on channel closure reflects a misunderstanding. Tokio's mpscsend()method returns aResulton closure—it doesn't panic. If there's a need to guarantee delivery of UI updates, verify that the channel is created with sufficient capacity and the receiver is never dropped unexpectedly.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
rust/src/loading_popup.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
rust/**/*.rs
⚙️ CodeRabbit configuration file
rust/**/*.rs: - Verify correct functionality and logic consistency.
- Check for idiomatic Rust usage and performance best practices.
- Suggest improvements to error handling, unwraps, and Result handling.
- Check for any potential memory leaks or unsafe code.
- Check for any potential mutex deadlocks.
- Check for potential security issues, make sure Bitcoin wallets are handled securely.
- Identify spelling mistakes in comments, string literals, and documentation
Files:
rust/src/loading_popup.rs
🔇 Additional comments (1)
rust/src/loading_popup.rs (1)
9-10: Well-chosen timing constants for smooth UX.The 50ms delay prevents popup flicker on fast operations, and the 350ms minimum display duration ensures the popup doesn't disappear too abruptly. These values provide good user experience without being overly conservative.
Summary by CodeRabbit
New Features
Refactor
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.