Add wallet reordering to sidebar on iOS and Android#695
Add wallet reordering to sidebar on iOS and Android#695geeekyvishal wants to merge 15 commits intobitcoinppl:masterfrom
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 49 minutes and 27 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR adds drag-to-reorder UI on Android and iOS, a persistent Changes
Sequence DiagramssequenceDiagram
actor User
participant Android_UI as "Android UI\n(SidebarView)"
participant iOS_UI as "iOS UI\n(SidebarView)"
participant App_Layer as "App / Database API"
participant Rust_Backend as "Rust Backend\n(reorder_wallets)"
User->>Android_UI: Long-press & drag wallet
Android_UI->>Android_UI: update walletList order (live), apply graphicsLayer
User->>Android_UI: Release (drop)
Android_UI->>App_Layer: reorderWallets(orderedIds)
App_Layer->>Rust_Backend: uniffi call reorder_wallets(ordered_ids)
Rust_Backend->>Rust_Backend: validate permutation, update positions, persist
Rust_Backend-->>App_Layer: success / error
App_Layer-->>Android_UI: success / error (revert if error)
Note right of iOS_UI: iOS flow is analogous using\n.onDrag/.onDrop + SidebarWalletDropDelegate
sequenceDiagram
participant App_Layer as "App / Database API"
participant Rust_Backend as "Rust Backend"
App_Layer->>Rust_Backend: reorder_wallets(ordered_ids)
Rust_Backend->>Rust_Backend: validate ordered_ids is permutation
alt valid
Rust_Backend->>Rust_Backend: assign positions by order, persist
Rust_Backend-->>App_Layer: Ok
else invalid
Rust_Backend-->>App_Layer: Err(InvalidWalletReorder)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
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 adds drag-to-reorder wallet support to the sidebar on both iOS and Android. The Rust backend stores a Confidence Score: 5/5Safe to merge — all findings are non-blocking style/defensive-coding suggestions. All three comments are P2. The Rust backend is solid with thorough tests. The iOS missing drag-guard is a very low-probability edge case. No data loss or security concerns exist. ios/Cove/Views/SidebarView.swift — minor guard missing in onChange handler Important Files Changed
Sequence DiagramsequenceDiagram
participant U as User (drag gesture)
participant UI as iOS/Android Sidebar
participant Rust as WalletsTable (Rust FFI)
participant DB as redb Database
U->>UI: long-press + drag wallet row
UI->>UI: update local walletList order (optimistic)
U->>UI: release / drop
UI->>Rust: reorderWallets(orderedIds)
Rust->>Rust: validate_reorder_order()
Rust->>DB: save_all_wallets (positions updated)
Rust-->>UI: WalletsChanged update
UI->>UI: onChange(app.wallets) → sync walletList
Note over UI: iOS: no drag guard on onChange
Note over UI: Android: guards with draggedWalletId == null
Reviews (1): Last reviewed commit: "Add wallet reordering to mobile sidebars" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| onDragEnd = { | ||
| val draggedId = draggedWalletId | ||
| if (draggedId != null) { | ||
| val appOrder = app.wallets.map { it.id } | ||
| val localOrder = walletList.map { it.id } | ||
| if (localOrder != appOrder) { | ||
| runCatching { | ||
| app.database.wallets().reorderWallets(orderedIds = localOrder) | ||
| }.onFailure { | ||
| Log.e("SidebarView", "Failed to reorder wallets", it) | ||
| walletList = app.wallets | ||
| } | ||
| } |
There was a problem hiding this comment.
app.database.wallets().reorderWallets(...) is a synchronous Rust FFI call that writes to the redb database. Calling it directly in onDragEnd (which runs on the composition/main thread) can cause dropped frames or jank on slower devices, especially if the database write takes more than a single frame (~16 ms).
Consider launching the call in a coroutine scope (e.g., rememberCoroutineScope()) so it is dispatched off the main thread:
coroutineScope.launch(Dispatchers.IO) {
runCatching {
app.database.wallets().reorderWallets(orderedIds = localOrder)
}.onFailure {
Log.e("SidebarView", "Failed to reorder wallets", it)
withContext(Dispatchers.Main) { walletList = app.wallets }
}
}The iOS persistWalletOrder() has the same pattern and would benefit from an equivalent Task { await ... } wrapper.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (4)
rust/src/database/wallet.rs (2)
444-528: Good test coverage for the new helpers.Tests cover migration, sort stability, permutation validation (including duplicates, unknown ids, and partial lists), and the gap-aware
next_append_position. One suggestion: add an end-to-end test that opens aWalletsTable, inserts legacy rows with all-zero positions, callsget_all, and verifies the migration is persisted (not just computed in memory). That would catch the save-failure concern raised above.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/src/database/wallet.rs` around lines 444 - 528, Add an end-to-end test that verifies migration persistence: open a WalletsTable, insert rows with zeroed positions (legacy state), call WalletsTable::get_all (which should call migrate_legacy_positions_if_needed internally), and then reopen or re-query the table to assert that the wallets now have non-zero positions persisted to storage (not just in-memory). Ensure the test uses the same persistence backend setup as production tests, checks that migrate_legacy_positions_if_needed was applied and that subsequent reads reflect the updated positions (i.e., the save path in WalletsTable that writes migrated positions actually runs).
180-184: Nit: redundant rebindlet mut wallet = wallet;.♻️ Cleaner alternative
- fn save_new_wallet_metadata_with_backup_behavior( - &self, - wallet: WalletMetadata, - should_backup_to_cloud: bool, - ) -> Result<(), Error> { + fn save_new_wallet_metadata_with_backup_behavior( + &self, + mut wallet: WalletMetadata, + should_backup_to_cloud: bool, + ) -> Result<(), Error> { @@ let wallet_for_backup = should_backup_to_cloud.then(|| wallet.clone()); - let mut wallet = wallet; wallet.position = next_append_position(&wallets);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/src/database/wallet.rs` around lines 180 - 184, The code currently rebinding wallet with "let mut wallet = wallet;" which is redundant; instead make the original binding mutable or mutate through a mutable reference and remove that rebind. Concretely, change the original variable declaration that provides wallet to be mutable (or obtain a mutable reference) so you can set wallet.position = next_append_position(&wallets) directly, keep the existing wallet_for_backup = should_backup_to_cloud.then(|| wallet.clone()) logic in correct order, then push(wallet) and call self.save_all_wallets(network, mode, wallets)?; remove the redundant "let mut wallet = wallet;" line.android/app/src/main/java/org/bitcoinppl/cove/sidebar/SidebarView.kt (2)
349-359: Minor: thefromIndex == toIndexearly return at line 353 can be removed —coerceInalready makes this a no-op andremoveAt+addon equal indices preserves order.Not a bug, just dead code. Also worth noting that
fromIndexis trusted without validation; callers do checkfromIndex != -1before invoking, so current usage is safe.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@android/app/src/main/java/org/bitcoinppl/cove/sidebar/SidebarView.kt` around lines 349 - 359, Remove the redundant early-return check in the extension function List<WalletMetadata>.move: drop the if (fromIndex == toIndex) return this and let the existing logic (toMutableList(), removeAt(fromIndex), val safeTarget = toIndex.coerceIn(0, mutable.size), mutable.add(safeTarget, item), return mutable.toList()) handle no-op moves; keep using fromIndex and toIndex as-is since callers already guard fromIndex != -1.
69-81: UseWalletId?instead ofString?fordraggedWalletIdto improve type safety and consistency with iOS.
WalletIdis a public typealias available fromorg.bitcoinppl.cove_core.typesand is already used alongside other imported types in this file (e.g.,WalletMetadata,WalletColor). The currentString?declaration obscures the semantic meaning that this variable specifically holds wallet identifiers. Additionally, the.toString()call at line 151 becomes redundant when the type is explicitlyWalletId?rather thanString?.
🤖 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/sidebar/SidebarView.kt`:
- Around line 160-232: pointerInput is currently keyed on walletList which gets
reassigned during onDrag and cancels the active
detectDragGesturesAfterLongPress; change the key to a stable identifier (use
pointerInput(wallet.id) instead of pointerInput(wallet.id, walletList)) so the
gesture coroutine isn't recreated on list reassignments — keep using the
captured walletList state inside the lambda and leave
detectDragGesturesAfterLongPress, draggedWalletId, draggedDistance,
dragStartCenterY and the reorder logic unchanged.
In `@ios/Cove/Views/SidebarView.swift`:
- Around line 160-162: The onChange handler for app.wallets currently
unconditionally assigns walletList which can clobber the UI during an active
drag; modify the .onChange(of: app.wallets) closure to only set walletList =
updated when there is no active drag by checking the dragged wallet state (e.g.,
if draggedWalletId == nil or equivalent drag state property) so updates are
ignored while a drag is in progress; reference the onChange(of: app.wallets)
closure, the walletList property, and the draggedWalletId/drag state to
implement this guard.
- Around line 106-120: The drag state isn't cleared when a drag is cancelled so
draggedWalletId remains non-nil and blocks interactions; update the view that
sets draggedWalletId in .onDrag to also reset it to nil on
cancellation/transition by adding cleanup: call draggedWalletId = nil in
.onDisappear and in an .onChange(of: app.isSidebarVisible) handler (or replace
the current .onDrag with a DragGesture and use its .onEnded/.onCancelled to set
draggedWalletId = nil), and keep
SidebarWalletDropDelegate.performDrop/persistWalletOrder as the successful-drop
path.
In `@rust/src/database/wallet.rs`:
- Around line 399-408: migrate_legacy_positions_if_needed mutates the in-memory
WalletMetadata positions but callers (via get_all) may return migrated data even
if save_all_wallets fails, causing inconsistent in-memory vs persisted state and
repeated failed migrations; update the flow that calls
migrate_legacy_positions_if_needed (e.g., get_all) so that after setting
positions on WalletMetadata you attempt to persist via save_all_wallets and if
save fails you revert the in-memory changes (or return the original vec
unchanged) and log the save error at a higher level; refer to
migrate_legacy_positions_if_needed, get_all, save_all_wallets,
next_append_position, and WalletMetadata to locate and implement the
revert-on-save-failure plus logging behavior.
- Around line 233-245: get_all performs an implicit write by calling
migrate_legacy_positions_if_needed and save_all_wallets, which can emit
AppStateReconcileMessage::DatabaseUpdated and cause write contention from read
paths; change this by moving migration out of get_all into a one-time
startup/schema-upgrade step (or a single well-defined entrypoint) so get_all
remains read-only, or at minimum guard the in-place migration with a
process-wide flag (e.g., an AtomicBool) to ensure
migrate_legacy_positions_if_needed/save_all_wallets run only once and suppress
emitting DatabaseUpdated for that migration write; locate and modify the
get_all, migrate_legacy_positions_if_needed, and save_all_wallets call sites to
implement the one-shot startup migration or the guarded lazy-migration approach.
---
Nitpick comments:
In `@android/app/src/main/java/org/bitcoinppl/cove/sidebar/SidebarView.kt`:
- Around line 349-359: Remove the redundant early-return check in the extension
function List<WalletMetadata>.move: drop the if (fromIndex == toIndex) return
this and let the existing logic (toMutableList(), removeAt(fromIndex), val
safeTarget = toIndex.coerceIn(0, mutable.size), mutable.add(safeTarget, item),
return mutable.toList()) handle no-op moves; keep using fromIndex and toIndex
as-is since callers already guard fromIndex != -1.
In `@rust/src/database/wallet.rs`:
- Around line 444-528: Add an end-to-end test that verifies migration
persistence: open a WalletsTable, insert rows with zeroed positions (legacy
state), call WalletsTable::get_all (which should call
migrate_legacy_positions_if_needed internally), and then reopen or re-query the
table to assert that the wallets now have non-zero positions persisted to
storage (not just in-memory). Ensure the test uses the same persistence backend
setup as production tests, checks that migrate_legacy_positions_if_needed was
applied and that subsequent reads reflect the updated positions (i.e., the save
path in WalletsTable that writes migrated positions actually runs).
- Around line 180-184: The code currently rebinding wallet with "let mut wallet
= wallet;" which is redundant; instead make the original binding mutable or
mutate through a mutable reference and remove that rebind. Concretely, change
the original variable declaration that provides wallet to be mutable (or obtain
a mutable reference) so you can set wallet.position =
next_append_position(&wallets) directly, keep the existing wallet_for_backup =
should_backup_to_cloud.then(|| wallet.clone()) logic in correct order, then
push(wallet) and call self.save_all_wallets(network, mode, wallets)?; remove the
redundant "let mut wallet = wallet;" line.
🪄 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: 9216dd22-4789-449e-be11-43a918b1d901
⛔ Files ignored due to path filters (1)
ios/CoveCore/Sources/CoveCore/generated/cove.swiftis excluded by!**/generated/**
📒 Files selected for processing (6)
android/app/src/main/java/org/bitcoinppl/cove/sidebar/SidebarView.ktandroid/app/src/main/java/org/bitcoinppl/cove_core/cove.ktios/Cove/Views/SidebarView.swiftrust/src/backup/model.rsrust/src/database/wallet.rsrust/src/wallet/metadata.rs
| .onDrag { | ||
| draggedWalletId = wallet.id | ||
| UIImpactFeedbackGenerator(style: .light).impactOccurred() | ||
| return NSItemProvider(object: "\(wallet.id)" as NSString) | ||
| } | ||
| .onDrop( | ||
| of: [UTType.plainText], | ||
| delegate: SidebarWalletDropDelegate( | ||
| item: wallet, | ||
| wallets: $walletList, | ||
| draggedWalletId: $draggedWalletId, | ||
| onReorderCommitted: persistWalletOrder | ||
| ) | ||
| ) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd -t f "SidebarView.swift" ios/Repository: bitcoinppl/cove
Length of output: 91
🏁 Script executed:
wc -l ios/Cove/Views/SidebarView.swiftRepository: bitcoinppl/cove
Length of output: 95
🏁 Script executed:
sed -n '70,80p' ios/Cove/Views/SidebarView.swiftRepository: bitcoinppl/cove
Length of output: 560
🏁 Script executed:
sed -n '100,130p' ios/Cove/Views/SidebarView.swiftRepository: bitcoinppl/cove
Length of output: 1399
🏁 Script executed:
sed -n '1,50p' ios/Cove/Views/SidebarView.swiftRepository: bitcoinppl/cove
Length of output: 1447
🏁 Script executed:
sed -n '51,100p' ios/Cove/Views/SidebarView.swiftRepository: bitcoinppl/cove
Length of output: 2141
🏁 Script executed:
sed -n '130,180p' ios/Cove/Views/SidebarView.swiftRepository: bitcoinppl/cove
Length of output: 1704
🏁 Script executed:
sed -n '180,236p' ios/Cove/Views/SidebarView.swiftRepository: bitcoinppl/cove
Length of output: 1865
🏁 Script executed:
fd -t f "SidebarWalletDropDelegate" ios/Repository: bitcoinppl/cove
Length of output: 41
Reset draggedWalletId on cancelled drags or view transitions.
.onDrag sets draggedWalletId, but it's only reset in SidebarWalletDropDelegate.performDrop (line 232)—which never fires if the user releases outside a wallet row or cancels the drag. This leaves draggedWalletId non-nil and the guard draggedWalletId == nil else { return } at line 75 silently blocks all subsequent taps.
Add cleanup to onChange(of: app.isSidebarVisible) and/or .onDisappear to reset draggedWalletId, or use DragGesture with .onEnded for explicit cancellation handling.
🤖 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 106 - 120, The drag state
isn't cleared when a drag is cancelled so draggedWalletId remains non-nil and
blocks interactions; update the view that sets draggedWalletId in .onDrag to
also reset it to nil on cancellation/transition by adding cleanup: call
draggedWalletId = nil in .onDisappear and in an .onChange(of:
app.isSidebarVisible) handler (or replace the current .onDrag with a DragGesture
and use its .onEnded/.onCancelled to set draggedWalletId = nil), and keep
SidebarWalletDropDelegate.performDrop/persistWalletOrder as the successful-drop
path.
| /// Get all wallets for a network (sorted by [`WalletMetadata::position`], then id). | ||
| pub fn get_all( | ||
| &self, | ||
| network: Network, | ||
| mode: WalletMode, | ||
| ) -> Result<Vec<WalletMetadata>, Error> { | ||
| let mut wallets = self.load_wallets_raw(network, mode)?; | ||
| if migrate_legacy_positions_if_needed(&mut wallets) { | ||
| self.save_all_wallets(network, mode, wallets.clone())?; | ||
| } | ||
| sort_wallets_by_position(&mut wallets); | ||
| Ok(wallets) | ||
| } |
There was a problem hiding this comment.
get_all now performs writes and emits DatabaseUpdated on the read path — this has broader side effects than the name implies.
Every caller of get_all (e.g., len, is_empty, all, all_sorted_active, get, find_by_tap_signer_ident, update_wallet_metadata, delete, etc.) can now:
- Open a write transaction (via
save_all_wallets), potentially contending with other writers. - Emit
AppStateReconcileMessage::DatabaseUpdated(from insidesave_all_wallets) on what is effectively a read.
This can cause:
- UI refresh loops if a read triggered from a
DatabaseUpdatedobserver ends up re-emittingDatabaseUpdated. - Unexpected write contention in hot read paths (e.g.,
lenis called to checkis_empty). - Repeated migration attempts if the save path silently fails but the in-memory mutation succeeds.
Recommend moving the migration to a one-shot step at database open / schema upgrade time, or at minimum performing migration lazily only in a single well-defined entry point (not every read). If the in-place migration must stay here, guard it so it only runs once per process (e.g., an AtomicBool) and avoid emitting DatabaseUpdated for the migration write.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@rust/src/database/wallet.rs` around lines 233 - 245, get_all performs an
implicit write by calling migrate_legacy_positions_if_needed and
save_all_wallets, which can emit AppStateReconcileMessage::DatabaseUpdated and
cause write contention from read paths; change this by moving migration out of
get_all into a one-time startup/schema-upgrade step (or a single well-defined
entrypoint) so get_all remains read-only, or at minimum guard the in-place
migration with a process-wide flag (e.g., an AtomicBool) to ensure
migrate_legacy_positions_if_needed/save_all_wallets run only once and suppress
emitting DatabaseUpdated for that migration write; locate and modify the
get_all, migrate_legacy_positions_if_needed, and save_all_wallets call sites to
implement the one-shot startup migration or the guarded lazy-migration approach.
| fn migrate_legacy_positions_if_needed(wallets: &mut Vec<WalletMetadata>) -> bool { | ||
| if wallets.len() > 1 && wallets.iter().all(|w| w.position == 0) { | ||
| for (i, w) in wallets.iter_mut().enumerate() { | ||
| w.position = i as u32; | ||
| } | ||
| true | ||
| } else { | ||
| false | ||
| } | ||
| } |
There was a problem hiding this comment.
Migration heuristic is reasonable but has a subtle edge case worth noting.
The condition wallets.len() > 1 && wallets.iter().all(|w| w.position == 0) correctly captures the legacy case. However, if a user somehow ended up with exactly one wallet at position == 0 (the common case for a freshly created wallet that skipped the save-path override, or a user who deleted all but one wallet), subsequently adding a second wallet via next_append_position will return 1, so positions become [0, 1] — migration would not re-fire, which is correct.
One concern: if save_all_wallets fails inside get_all during migration, the in-memory wallets is still mutated and returned. Callers receive migrated positions that aren't persisted, and the migration will retry indefinitely on every subsequent get_all call. Consider logging the save failure at a higher level, or returning the raw (unmigrated) vec on save error.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@rust/src/database/wallet.rs` around lines 399 - 408,
migrate_legacy_positions_if_needed mutates the in-memory WalletMetadata
positions but callers (via get_all) may return migrated data even if
save_all_wallets fails, causing inconsistent in-memory vs persisted state and
repeated failed migrations; update the flow that calls
migrate_legacy_positions_if_needed (e.g., get_all) so that after setting
positions on WalletMetadata you attempt to persist via save_all_wallets and if
save fails you revert the in-memory changes (or return the original vec
unchanged) and log the save error at a higher level; refer to
migrate_legacy_positions_if_needed, get_all, save_all_wallets,
next_append_position, and WalletMetadata to locate and implement the
revert-on-save-failure plus logging behavior.
fix : onChange can reset an in-progress drag Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
…iew.kt fix : Misplaced import Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
android/app/src/main/java/org/bitcoinppl/cove/sidebar/SidebarView.kt (1)
166-166:⚠️ Potential issue | 🔴 CriticalCritical:
pointerInputkey still includeswalletList, cancelling the drag on every swap.
walletListis reassigned insideonDrag(line 202), and because it is part of thepointerInputkey, each reorder recreates the gesture coroutine and cancels the activedetectDragGesturesAfterLongPress. The first swap works, but subsequent drag deltas — andonDragEnd/onDragCancel— stop firing for the in-flight gesture. Key onwallet.idonly; thewalletListread inside the lambda stays current via the captured state.🛠️ Suggested fix
- }.pointerInput(wallet.id, walletList) { + }.pointerInput(wallet.id) { detectDragGesturesAfterLongPress(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@android/app/src/main/java/org/bitcoinppl/cove/sidebar/SidebarView.kt` at line 166, The pointerInput key currently includes walletList which causes the gesture coroutine to be recreated and cancels in-flight drags when walletList is reassigned; change the pointerInput call to key only on wallet.id (i.e., replace pointerInput(wallet.id, walletList) with pointerInput(wallet.id)) so the gesture coroutine persists across reorders, leaving the lambda to capture the current walletList state used by onDrag/onDragEnd/onDragCancel and keep detectDragGesturesAfterLongPress behavior intact.
🤖 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/sidebar/SidebarView.kt`:
- Around line 82-87: The LaunchedEffect tied to LaunchedEffect(app.wallets,
draggedWalletId) currently always assigns walletList = app.wallets when
draggedWalletId becomes null, causing a flicker because app.wallets may still be
in the old order; change the block so that when draggedWalletId == null you only
set walletList if app.wallets is actually different from the current walletList
(e.g., compare lists/IDs and only assign when not equal), referencing the
LaunchedEffect, draggedWalletId, walletList and app.wallets symbols; this
ensures you wait for the WalletsChanged update to differ before resyncing and
avoids the interim revert (alternatively, move the reorder to run on the main
thread so app.wallets is updated before onDragEnd clears draggedWalletId).
- Around line 215-229: The reorderWallets FFI call is incorrectly dispatched to
Dispatchers.IO causing unnecessary threading and potential race; change the
block that currently does scope.launch(Dispatchers.IO) { runCatching {
app.database.wallets().reorderWallets(orderedIds = localOrder) } ... } to invoke
app.database.wallets().reorderWallets(orderedIds = localOrder) directly on the
current (Main) thread inside the existing LaunchedEffect so it runs
synchronously; remove the surrounding scope.launch(Dispatchers.IO) and the
withContext(Dispatchers.Main) fallback (the onFailure should just log the
error), and then remove any now-unused coroutine imports and the
rememberCoroutineScope usage since they’re no longer needed; ensure walletList
and draggedWalletId behavior remains driven by the existing
LaunchedEffect/WalletsChanged flow.
---
Duplicate comments:
In `@android/app/src/main/java/org/bitcoinppl/cove/sidebar/SidebarView.kt`:
- Line 166: The pointerInput key currently includes walletList which causes the
gesture coroutine to be recreated and cancels in-flight drags when walletList is
reassigned; change the pointerInput call to key only on wallet.id (i.e., replace
pointerInput(wallet.id, walletList) with pointerInput(wallet.id)) so the gesture
coroutine persists across reorders, leaving the lambda to capture the current
walletList state used by onDrag/onDragEnd/onDragCancel and keep
detectDragGesturesAfterLongPress behavior intact.
🪄 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: af162c3d-5f48-40b1-b75c-58628700db75
⛔ Files ignored due to path filters (1)
ios/CoveCore/Sources/CoveCore/generated/cove.swiftis excluded by!**/generated/**
📒 Files selected for processing (3)
android/app/src/main/java/org/bitcoinppl/cove/sidebar/SidebarView.ktandroid/app/src/main/java/org/bitcoinppl/cove_core/cove.ktios/Cove/Views/SidebarView.swift
🚧 Files skipped from review as they are similar to previous changes (1)
- ios/Cove/Views/SidebarView.swift
| val appOrder = app.wallets.map { it.id } | ||
| val localOrder = walletList.map { it.id } | ||
| if (localOrder != appOrder) { | ||
| scope.launch(Dispatchers.IO) { | ||
| runCatching { | ||
| app.database.wallets().reorderWallets(orderedIds = localOrder) | ||
| }.onFailure { | ||
| Log.e("SidebarView", "Failed to reorder wallets", it) | ||
| withContext(Dispatchers.Main) { | ||
| walletList = app.wallets | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Don't dispatch the Rust FFI reorder call to Dispatchers.IO.
Per the repository FFI/Threading policy, Rust FFI calls are synchronous and fast (microseconds), Tokio is handled inside the core, and redb does not block the UI thread. scope.launch(Dispatchers.IO) { ... reorderWallets(...) } should only be used with profiling evidence showing >16ms UI blocking. Running on Main also removes the race described in the neighboring comment on the LaunchedEffect: app.wallets will be updated (via WalletsChanged) before draggedWalletId is cleared below, so walletList won't flicker back to the old order. The withContext(Dispatchers.Main) { walletList = app.wallets } inside onFailure is also redundant — clearing draggedWalletId a few lines later will re-trigger the LaunchedEffect and resync walletList from app.wallets on its own.
🛠️ Suggested simplification
- if (localOrder != appOrder) {
- scope.launch(Dispatchers.IO) {
- runCatching {
- app.database.wallets().reorderWallets(orderedIds = localOrder)
- }.onFailure {
- Log.e("SidebarView", "Failed to reorder wallets", it)
- withContext(Dispatchers.Main) {
- walletList = app.wallets
- }
- }
- }
- }
+ if (localOrder != appOrder) {
+ runCatching {
+ app.database.wallets().reorderWallets(orderedIds = localOrder)
+ }.onFailure {
+ Log.e("SidebarView", "Failed to reorder wallets", it)
+ walletList = app.wallets
+ }
+ }With this change, scope/rememberCoroutineScope and the kotlinx.coroutines.Dispatchers/launch/withContext imports on lines 39, 54–56 are no longer needed in this function.
As per coding guidelines: "NEVER suggest moving Rust FFI calls to background threads (withContext(Dispatchers.IO))" and "ONLY suggest Dispatchers.IO with profiling evidence showing >16ms UI blocking".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@android/app/src/main/java/org/bitcoinppl/cove/sidebar/SidebarView.kt` around
lines 215 - 229, The reorderWallets FFI call is incorrectly dispatched to
Dispatchers.IO causing unnecessary threading and potential race; change the
block that currently does scope.launch(Dispatchers.IO) { runCatching {
app.database.wallets().reorderWallets(orderedIds = localOrder) } ... } to invoke
app.database.wallets().reorderWallets(orderedIds = localOrder) directly on the
current (Main) thread inside the existing LaunchedEffect so it runs
synchronously; remove the surrounding scope.launch(Dispatchers.IO) and the
withContext(Dispatchers.Main) fallback (the onFailure should just log the
error), and then remove any now-unused coroutine imports and the
rememberCoroutineScope usage since they’re no longer needed; ensure walletList
and draggedWalletId behavior remains driven by the existing
LaunchedEffect/WalletsChanged flow.
…pointerInput keys
|
@geeekyvishal don't commit |
|
@geeekyvishal did you discuss with @diyaayay to decide who's going to finish this issue? |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
rust/src/database/wallet.rs (1)
175-182:⚠️ Potential issue | 🟠 MajorClone the backup metadata after assigning the persisted position.
Line 175 captures
wallet_for_backupbefore Line 176 assigns the append position, so cloud backup receives stalepositionmetadata while the database stores a different wallet order.Proposed fix
- let wallet_for_backup = should_backup_to_cloud.then(|| wallet.clone()); wallet.position = next_append_position(&wallets); + let wallet_for_backup = should_backup_to_cloud.then(|| wallet.clone()); wallets.push(wallet);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/src/database/wallet.rs` around lines 175 - 182, The backup captures wallet metadata before the persisted position is set, so the cloud receives stale position; move the clone to after assigning wallet.position and after save_all_wallets so wallet_for_backup = should_backup_to_cloud.then(|| wallet.clone()) is created only after calling wallet.position = next_append_position(&wallets) (and preferably after self.save_all_wallets(...)), then call CLOUD_BACKUP_MANAGER.backup_new_wallet(wallet_for_backup) as before; update references: wallet_for_backup, should_backup_to_cloud, wallet.position = next_append_position(&wallets), self.save_all_wallets(...), Updater::send_update, and CLOUD_BACKUP_MANAGER.backup_new_wallet to ensure the backup uses the final persisted wallet metadata.
♻️ Duplicate comments (2)
android/app/src/main/java/org/bitcoinppl/cove/sidebar/SidebarView.kt (1)
39-39:⚠️ Potential issue | 🟠 MajorCall the Rust reorder FFI synchronously on Main.
reorderWalletsis a Rust FFI call and should not be dispatched toDispatchers.IO; doing so also keeps the drag state cleanup racing theWalletsChangedrefresh.Proposed simplification
-import androidx.compose.runtime.rememberCoroutineScope import androidx.compose.runtime.remember import androidx.compose.runtime.setValue @@ -import kotlinx.coroutines.Dispatchers -import kotlinx.coroutines.launch -import kotlinx.coroutines.withContext @@ - val scope = rememberCoroutineScope() @@ val localOrder = walletList.map { it.id } if (localOrder != appOrder) { - scope.launch(Dispatchers.IO) { - runCatching { - app.database.wallets().reorderWallets(orderedIds = localOrder) - }.onSuccess { - withContext(Dispatchers.Main) { - draggedWalletId = null - draggedDistance = 0f - dragStartCenterY = 0f - haptic.performHapticFeedback(HapticFeedbackType.TextHandleMove) - } - }.onFailure { - Log.e("SidebarView", "Failed to reorder wallets", it) - withContext(Dispatchers.Main) { - walletList = app.wallets - draggedWalletId = null - draggedDistance = 0f - dragStartCenterY = 0f - haptic.performHapticFeedback(HapticFeedbackType.TextHandleMove) - } - } - } - return@detectDragGesturesAfterLongPress + runCatching { + app.database.wallets().reorderWallets(orderedIds = localOrder) + }.onFailure { + Log.e("SidebarView", "Failed to reorder wallets", it) + walletList = app.wallets + } } } draggedWalletId = nullAs per coding guidelines, "NEVER suggest moving Rust FFI calls to background threads (withContext(Dispatchers.IO))" and "ONLY suggest Dispatchers.IO with profiling evidence showing >16ms UI blocking".
Also applies to: 54-56, 80-80, 218-238
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@android/app/src/main/java/org/bitcoinppl/cove/sidebar/SidebarView.kt` at line 39, The call to the Rust FFI function reorderWallets must be invoked synchronously on the Main thread rather than dispatched to Dispatchers.IO; remove any withContext(Dispatchers.IO) wrappers around reorderWallets (and similar Rust FFI calls) and call reorderWallets directly from the coroutine launched on the Main thread (so the drag state cleanup and WalletsChanged refresh are not racing). Locate usages named reorderWallets and any surrounding coroutine blocks that switch to IO (also other occurrences flagged around the file) and ensure they execute on the Main/UI coroutine context (or no context switch) so the FFI call is synchronous and ordering with WalletsChanged is preserved.rust/src/database/wallet.rs (1)
227-236:⚠️ Potential issue | 🟠 MajorKeep
get_allread-only.This path can still perform a migration write via
save_all_wallets, which also emitsDatabaseUpdated; callers likelen,is_empty,get, and update helpers inherit write contention and observer side effects from a read-shaped API. Move this migration to a one-shot startup/schema step, or gate it so it cannot repeatedly run from ordinary reads.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/src/database/wallet.rs` around lines 227 - 236, get_all currently performs a write by calling save_all_wallets when migrate_legacy_positions_if_needed returns true, causing reads (len/is_empty/get) to incur write contention and emit DatabaseUpdated; remove that write from the read path and run the migration as a one-shot startup/schema step or gate it with a one-time flag. Concretely: stop calling save_all_wallets from get_all; instead implement an initialization function (e.g. ensure_positions_migrated or run_legacy_migration_once) that checks migrate_legacy_positions_if_needed and, if needed, calls save_all_wallets exactly once at DB open (use an AtomicBool/once_cell or persisted schema flag) so get_all, WalletStore::get_all, and helpers remain read-only and do not trigger DatabaseUpdated. Ensure references to migrate_legacy_positions_if_needed, save_all_wallets, and DatabaseUpdated are updated accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@rust/src/database/wallet.rs`:
- Around line 175-182: The backup captures wallet metadata before the persisted
position is set, so the cloud receives stale position; move the clone to after
assigning wallet.position and after save_all_wallets so wallet_for_backup =
should_backup_to_cloud.then(|| wallet.clone()) is created only after calling
wallet.position = next_append_position(&wallets) (and preferably after
self.save_all_wallets(...)), then call
CLOUD_BACKUP_MANAGER.backup_new_wallet(wallet_for_backup) as before; update
references: wallet_for_backup, should_backup_to_cloud, wallet.position =
next_append_position(&wallets), self.save_all_wallets(...),
Updater::send_update, and CLOUD_BACKUP_MANAGER.backup_new_wallet to ensure the
backup uses the final persisted wallet metadata.
---
Duplicate comments:
In `@android/app/src/main/java/org/bitcoinppl/cove/sidebar/SidebarView.kt`:
- Line 39: The call to the Rust FFI function reorderWallets must be invoked
synchronously on the Main thread rather than dispatched to Dispatchers.IO;
remove any withContext(Dispatchers.IO) wrappers around reorderWallets (and
similar Rust FFI calls) and call reorderWallets directly from the coroutine
launched on the Main thread (so the drag state cleanup and WalletsChanged
refresh are not racing). Locate usages named reorderWallets and any surrounding
coroutine blocks that switch to IO (also other occurrences flagged around the
file) and ensure they execute on the Main/UI coroutine context (or no context
switch) so the FFI call is synchronous and ordering with WalletsChanged is
preserved.
In `@rust/src/database/wallet.rs`:
- Around line 227-236: get_all currently performs a write by calling
save_all_wallets when migrate_legacy_positions_if_needed returns true, causing
reads (len/is_empty/get) to incur write contention and emit DatabaseUpdated;
remove that write from the read path and run the migration as a one-shot
startup/schema step or gate it with a one-time flag. Concretely: stop calling
save_all_wallets from get_all; instead implement an initialization function
(e.g. ensure_positions_migrated or run_legacy_migration_once) that checks
migrate_legacy_positions_if_needed and, if needed, calls save_all_wallets
exactly once at DB open (use an AtomicBool/once_cell or persisted schema flag)
so get_all, WalletStore::get_all, and helpers remain read-only and do not
trigger DatabaseUpdated. Ensure references to
migrate_legacy_positions_if_needed, save_all_wallets, and DatabaseUpdated are
updated accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 00c85c6f-64ff-4008-89f1-09ada97c3172
⛔ Files ignored due to path filters (4)
.DS_Storeis excluded by!**/.DS_Storeios/.DS_Storeis excluded by!**/.DS_Storeios/CoveCore/Sources/CoveCore/generated/cove.swiftis excluded by!**/generated/**rust/.DS_Storeis excluded by!**/.DS_Store
📒 Files selected for processing (3)
android/app/src/main/java/org/bitcoinppl/cove/sidebar/SidebarView.ktandroid/app/src/main/java/org/bitcoinppl/cove_core/cove.ktrust/src/database/wallet.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- android/app/src/main/java/org/bitcoinppl/cove_core/cove.kt
|
My apologies! I've removed the .DS_Store files and added it to .gitignore to avoid it for future |
Hi @praveenperera, |
|
ok @geeekyvishal can i close this PR then? @diyaayay if this PR has useful things to take it and we can add coauthored by @geeekyvishal in the final commit |
|
Hi @praveenperera and @diyaayay, sorry for the overlap! I was focused on the technical feedback and missed your discord reply. |
|
Alright @praveenperera . I'll close my PR in that case . Happy to co author if something was useful . @diyaayay pointerInput flicker fix and background threading logic on Android might be useful . |
|
sounds good thank you |
3 participants
Summary
Closes #493
Adds wallet reordering support to the sidebar on iOS and Android, with ordering logic persisted in Rust.
What changed
Rust backend
positiontoWalletMetadatawith backward-compatible serde defaultspositionreorder_wallets(ordered_ids)with validation for duplicate, missing, unknown, and partial listsiOS
WalletsChangedflow refreshes state after reorderAndroid
WalletsChangedflow refreshes state after reorderWhy
This implements the requested ability to reorder wallets directly from the sidebar while keeping ordering logic shared and consistent in Rust across platforms.
Testing
Commands run:
cargo check --manifest-path rust/Cargo.toml
just test
just build-ios
just build-android
Manual testing:
Test results:
Platform Coverage
Demo
Checklist
Summary by CodeRabbit
New Features
Bug Fixes
Data
Tests