Android fix exports #484
Android fix exports #484
Conversation
Use proper alert states for duplicate and successful wallet imports:
- Change from AppAlertState.General to AppAlertState.DuplicateWallet
for duplicate wallets (removes wallet ID from message)
- Change from AppAlertState.General to AppAlertState.ImportedSuccessfully
for successful imports
- Add navigation to wallet on OK press in ImportedSuccessfully handler
- Remove premature popRoute() calls that caused race conditions
This ensures consistent messaging ("Duplicate Wallet" / "This wallet has
already been imported!") and reliable navigation to the wallet after
pressing OK, matching iOS behavior.
Add popRoute() before setting alertState in QR scan screens so the scanner is dismissed before showing success/duplicate/error alerts, matching iOS behavior.
|
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 new AppAlertState variants (Loading, ImportedSuccessfully, DuplicateWallet, ErrorImportingHardwareWallet); updates alert emission and UI rendering (including a loading dialog); standardizes import flows to emit alerts and pop routes; adds transaction-sharing coroutine; and refactors FFI bindings from RustBuffer returns to Long handle + async future-based calls. Changes
Sequence Diagram(s)sequenceDiagram
participant Kotlin as Kotlin App
participant FFI as Uniffi Native (Rust)
participant Future as FFI Future Callback
participant KotlinLift as Kotlin Wrapper Lift
rect rgb(240,248,255)
Note over Kotlin,FFI: New async handle-based FFI flow
end
Kotlin->>FFI: uniffi_cove_fn_*(...) -> returns Long handle
FFI-->>Kotlin: handle (Long)
Kotlin->>Future: register future via ffi_cove_rust_future_* with handle
Future-->>Kotlin: callback(success: ptr / error)
alt success
Kotlin->>KotlinLift: FfiConverter.lift(handle) // wrap into high-level object
KotlinLift-->>Kotlin: domain object (e.g., Wallet, LabelManager)
else error
Kotlin-->>Kotlin: propagate as exception / set error alert
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested labels
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (6)
🚧 Files skipped from review as they are similar to previous changes (3)
🧰 Additional context used📓 Path-based instructions (1)android/app/src/main/java/org/bitcoinppl/cove/**/*.kt⚙️ CodeRabbit configuration file
Files:
⏰ 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)
🔇 Additional comments (7)
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 SummaryUnified export functionality and cold wallet import behavior across Android and iOS platforms. The changes introduce a reusable loading popup mechanism in Rust that displays feedback for operations taking longer than 50ms, significantly improving user experience during exports. Android cold wallet import flow now properly dismisses the QR scanner before showing alerts and consistently handles duplicate wallet scenarios, matching iOS behavior. Key improvements:
Confidence Score: 5/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant Android as Android UI
participant WalletManager as Rust WalletManager
participant LoadingPopup as Loading Popup Helper
participant Reconciler as App Reconciler
Note over User,Reconciler: Export Transactions/Labels Flow
User->>Android: Click Export Button
Android->>WalletManager: exportTransactionsCsv() / exportLabelsForShare()
WalletManager->>LoadingPopup: with_loading_popup(operation)
alt Operation takes > 50ms
LoadingPopup->>Reconciler: ShowLoadingPopup
Reconciler->>Android: Update alertState to Loading
Android->>User: Show "Working on it..." dialog
end
LoadingPopup->>WalletManager: Execute export operation
WalletManager-->>LoadingPopup: Return export result
alt Popup was shown
LoadingPopup->>LoadingPopup: Wait minimum 350ms display time
LoadingPopup->>Reconciler: HideLoadingPopup
Reconciler->>Android: Clear alertState
end
LoadingPopup-->>Android: Return export result
Android->>Android: Create share intent with file
Android->>User: Show system share sheet
Note over User,Reconciler: Cold Wallet Import Flow
User->>Android: Scan QR Code
Android->>Android: Parse MultiFormat.HardwareExport
alt Successful Import
Android->>WalletManager: Wallet.newFromExport()
WalletManager-->>Android: Return wallet ID
Android->>WalletManager: selectWallet(id)
Android->>Android: popRoute() to dismiss scanner
Android->>Android: alertState = ImportedSuccessfully
User->>Android: Click OK on alert
Android->>Android: Navigate to SelectedWallet screen
end
alt Duplicate Wallet
Android->>WalletManager: Wallet.newFromExport()
WalletManager-->>Android: WalletException.WalletAlreadyExists(walletId)
Android->>Android: popRoute() to dismiss scanner
Android->>Android: alertState = DuplicateWallet(walletId)
User->>Android: Click OK on alert
Android->>WalletManager: selectWallet(walletId)
Android->>Android: Navigate to existing wallet
end
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
android/app/src/main/java/org/bitcoinppl/cove/views/QrExportView.kt
Outdated
Show resolved
Hide resolved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
android/app/src/main/java/org/bitcoinppl/cove/flows/NewWalletFlow/NewWalletSelectScreen.kt (1)
112-140: Resource leak:Walletobject is not closed after use.The
Walletobject created at line 114 implementsAutoCloseableand must be closed to properly release resources. Per coding guidelines on FFI Resource Management, UniFFI objects must call.close()before being dereferenced.🔎 Proposed fix
fun importWallet(content: String) { try { val wallet = Wallet.newFromXpub(xpub = content.trim()) - val id = wallet.id() - android.util.Log.d("NewWalletSelectScreen", "Imported Wallet: $id") - - app.rust.selectWallet(id = id) - app.alertState = TaggedItem(AppAlertState.ImportedSuccessfully) + try { + val id = wallet.id() + android.util.Log.d("NewWalletSelectScreen", "Imported Wallet: $id") + + app.rust.selectWallet(id = id) + app.alertState = TaggedItem(AppAlertState.ImportedSuccessfully) + } finally { + wallet.close() + } } catch (e: WalletException.MultiFormat) {Based on coding guidelines: "Always verify that UniFFI objects implementing AutoCloseable call .close() before being nulled".
🧹 Nitpick comments (3)
android/app/src/main/java/org/bitcoinppl/cove/views/QrExportView.kt (2)
105-105: Consider small screen scenarios with the 500dp minimum.The minimum height constraint ensures adequate QR code visibility, but on small or landscape-oriented devices, this could cause layout issues. Verify that parent containers handle scrolling appropriately when the content exceeds available space.
165-187: LGTM! Responsive QR layout using BoxWithConstraints.The refactored layout correctly uses BoxWithConstraints to derive the QR size from available width, creating a properly sized square image. The combination of
size(qrSize)andContentScale.FillBoundsmaintains the square aspect ratio without distortion.Optional: Consider dynamic bitmap sizing for large screens
The QR bitmap is generated at a fixed 512px (line 178) but displayed at screen width, which could appear pixelated on very large displays. For premium quality on tablets or large phones, you could calculate the bitmap size based on screen density:
val qrBitmapSize = remember(qrSize) { (qrSize.value * LocalDensity.current.density).toInt().coerceAtLeast(512) } val bitmap = remember(qrString, qrBitmapSize) { QrCodeGenerator.generate(qrString, size = qrBitmapSize) }However, QR codes are binary patterns that typically scale acceptably, so the current implementation is likely sufficient for most use cases.
android/app/src/main/java/org/bitcoinppl/cove/MainActivity.kt (1)
562-578: Consider usingstate.title()for consistency.The Loading dialog hardcodes "Working on it..." while
AppAlertState.Loading.title()also returns this string. Usingstate.title()would ensure consistency if the title ever changes.🔎 Suggested change
CircularProgressIndicator() Spacer(modifier = Modifier.height(12.dp)) - Text("Working on it...") + Text(state.title()) }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
android/app/src/main/java/org/bitcoinppl/cove/AppAlertState.ktandroid/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/flows/NewWalletFlow/NewWalletSelectScreen.ktandroid/app/src/main/java/org/bitcoinppl/cove/flows/NewWalletFlow/cold_wallet/ColdWalletQrScanScreen.ktandroid/app/src/main/java/org/bitcoinppl/cove/flows/NewWalletFlow/cold_wallet/QrCodeImportScreen.ktandroid/app/src/main/java/org/bitcoinppl/cove/views/QrExportView.ktandroid/app/src/main/java/org/bitcoinppl/cove/wallet/WalletSheets.ktandroid/app/src/main/java/org/bitcoinppl/cove_core/cove.kt
🧰 Additional context used
📓 Path-based instructions (1)
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/AppAlertState.ktandroid/app/src/main/java/org/bitcoinppl/cove/AppManager.ktandroid/app/src/main/java/org/bitcoinppl/cove/flows/NewWalletFlow/NewWalletSelectScreen.ktandroid/app/src/main/java/org/bitcoinppl/cove/wallet/WalletSheets.ktandroid/app/src/main/java/org/bitcoinppl/cove/flows/NewWalletFlow/cold_wallet/QrCodeImportScreen.ktandroid/app/src/main/java/org/bitcoinppl/cove/MainActivity.ktandroid/app/src/main/java/org/bitcoinppl/cove/views/QrExportView.ktandroid/app/src/main/java/org/bitcoinppl/cove/flows/NewWalletFlow/cold_wallet/ColdWalletQrScanScreen.kt
🧬 Code graph analysis (1)
android/app/src/main/java/org/bitcoinppl/cove_core/cove.kt (2)
ios/CoveCore/Sources/CoveCore/generated/cove.swift (1)
uniffiRustCallAsync(27790-27822)android/app/src/main/java/org/bitcoinppl/cove_core/types/cove_types.kt (16)
callWithHandle(2391-2412)callWithHandle(2741-2762)callWithHandle(3025-3046)callWithHandle(3341-3362)callWithHandle(3644-3665)callWithHandle(4014-4035)callWithHandle(4296-4317)callWithHandle(4794-4815)callWithHandle(5069-5090)callWithHandle(5393-5414)callWithHandle(5738-5759)callWithHandle(6041-6062)callWithHandle(6415-6436)callWithHandle(6660-6681)callWithHandle(7008-7029)callWithHandle(7360-7381)
⏰ 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: test (ubuntu-latest)
- GitHub Check: test (macos-latest)
- GitHub Check: compile-android
- GitHub Check: compile-ios
- GitHub Check: ktlint
🔇 Additional comments (17)
android/app/src/main/java/org/bitcoinppl/cove/views/QrExportView.kt (4)
8-8: LGTM! Imports support the responsive layout improvements.The new imports (BoxWithConstraints, heightIn, ContentScale) are all properly utilized in the refactored QR display logic and enable better responsive sizing behavior.
Also applies to: 14-14, 40-40
110-110: LGTM! Typography improvements enhance visual hierarchy.The updated text styles (titleLarge and bodyMedium) improve readability and content prominence, following Material Design 3 guidelines.
Also applies to: 116-116
119-119: LGTM! Padding adjustments improve text layout.The updated padding values add horizontal spacing and adjust vertical spacing to better separate the subtitle from surrounding content.
154-162: LGTM! Responsive square aspect ratio for loading state.Replacing the fixed height with
aspectRatio(1f)makes the loading indicator responsive and matches the square dimensions of the actual QR code display.android/app/src/main/java/org/bitcoinppl/cove/AppManager.kt (1)
580-582: LGTM!The change to use
AppAlertState.Loadinginstead of a general alert with an empty message is a cleaner approach. This dedicated state allows for proper UI rendering with a progress indicator inGlobalAlertDialog.android/app/src/main/java/org/bitcoinppl/cove/AppAlertState.kt (2)
94-96: LGTM!The new
Loadingstate is correctly implemented as adata objectsince it doesn't carry any data. The title and message mappings are appropriate for a loading indicator.
136-136: Consistent handling intitle()andmessage()methods.The
Loadingstate correctly returns a user-friendly title and an empty message, which aligns with the UI rendering inGlobalAlertDialogwhere the text is shown alongside a progress indicator.Also applies to: 166-166
android/app/src/main/java/org/bitcoinppl/cove/MainActivity.kt (1)
580-594: LGTM!The
ImportedSuccessfullyhandler correctly:
- Dismisses the alert first
- Safely retrieves the selected wallet using
?.let- Navigates to the wallet route
This provides a smoother UX flow after wallet import.
android/app/src/main/java/org/bitcoinppl/cove/wallet/WalletSheets.kt (2)
90-97: LGTM!The transaction export flow now properly handles errors with a user-friendly snackbar message. The coroutine-based approach with try/catch is a clean pattern for async operations.
173-173: LGTM!The padding adjustment provides better horizontal spacing for the QR export view within the modal sheet.
android/app/src/main/java/org/bitcoinppl/cove/flows/NewWalletFlow/cold_wallet/ColdWalletQrScanScreen.kt (1)
77-98: LGTM!The wallet import handling is well-structured:
wallet.close()is correctly called after extracting the ID- Routes are consistently popped before setting alert states
- All branches use the appropriate
AppAlertStatevariantsandroid/app/src/main/java/org/bitcoinppl/cove/flows/NewWalletFlow/cold_wallet/QrCodeImportScreen.kt (1)
63-98: LGTM!The QR code import handling is consistent with
ColdWalletQrScanScreen.kt:
wallet.close()is properly called after use- Routes are popped before setting alert states
- All error branches provide meaningful feedback via the appropriate
AppAlertStatevariantsandroid/app/src/main/java/org/bitcoinppl/cove_core/cove.kt (5)
2152-2157: LGTM!The external function signatures correctly reflect the async refactor: returning
Longhandles instead ofRustBuffer.ByValueand removing inline error status parameters. This aligns with the handle-based async pattern where errors are propagated viauniffiRustCallAsyncexception handling.
3562-3570: LGTM!Checksum values updated to match the new async function signatures. This is standard UniFFI behavior to ensure API compatibility between Kotlin bindings and Rust implementation.
12788-12795: LGTM!Interface declarations correctly use
suspend funfor Kotlin coroutine support, enabling non-blocking async calls to the underlying Rust futures.
12928-12946: LGTM!The async implementation correctly uses
uniffiRustCallAsyncwith proper handle lifecycle management viacallWithHandle. The pattern aligns with the Swift equivalent shown in the codebase and follows UniFFI conventions for coroutine-based async calls.Note: The
@Suppress("ASSIGNED_BUT_NEVER_ACCESSED_VARIABLE")annotation appears to be a UniFFI code generation artifact (no local variable is assigned here), but this is harmless for generated code.
12965-12983: LGTM!The implementation correctly handles the
QrDensityparameter lowering viaFfiConverterTypeQrDensityand lifts the result toList<String>viaFfiConverterSequenceString. The async pattern is consistent with theexport()method above.
- Add fallback navigation when selectedWallet returns null in ImportedSuccessfully handler - Move startActivity() calls outside IO dispatcher in share functions - Use state.title() instead of hardcoded string in Loading dialog - Show error alert when wallet selection fails in DuplicateWallet handler - Add popRoute() before alerts in NewWalletSelectScreen for consistency with QR scanning views
praveenperera
force-pushed
the
android-fix-exports
branch
from
6765939 to
9b4a9d6
Compare
Summary by CodeRabbit
New Features
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.