Add wallet reordering to sidebar (iOS + Android)#688
Add wallet reordering to sidebar (iOS + Android)#688diyaayay wants to merge 12 commits intobitcoinppl:masterfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds in‑view wallet reordering on Android and iOS, persists ordering via new UniFFI Rust APIs, introduces a persistent Changes
Sequence DiagramsequenceDiagram
participant User
participant UI as SidebarView (Android / iOS)
participant App as App State
participant FFI as UniFFI Rust FFI
participant DB as Database
User->>UI: Initiate reorder (long-press drag / enter edit + drag)
UI->>App: Apply optimistic local reorder (walletList / app.wallets)
App->>UI: Render updated order and drag visuals
User->>UI: Continue drag / drop to target
UI->>App: Update visual swaps and track offsets
User->>UI: Release / end drag
UI->>FFI: Call moveWallet(toPosition) or reorderWallets(orderedIds)
FFI->>DB: Validate, mutate positions, persist
DB-->>FFI: Success or Error
alt Success
FFI-->>UI: Return updated metadata list
UI->>App: Confirm persisted order, clear drag/edit state
else Failure
FFI-->>UI: Return error
UI->>App: Roll back to pre-drag snapshot, clear drag/edit state
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 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 |
diyaayay
force-pushed
the
cove-sort-wallets-hamburger-menu
branch
from
d39ec60 to
a077c6e
Compare
diyaayay
force-pushed
the
cove-sort-wallets-hamburger-menu
branch
from
107c171 to
ce03c4a
Compare
Greptile SummaryAdds drag-to-reorder wallet ordering via a new A few correctness issues in the Rust layer are worth addressing before shipping:
Confidence Score: 4/5Safe to merge with awareness of three P2 position-consistency issues in the Rust layer that could produce subtle ordering regressions in specific user journeys. All four findings are P2 — no data loss, no crashes, no security issues. The stable sort and the self-correcting nature of rust/src/database/wallet.rs — position lifecycle (delete compaction, update preservation, migration heuristic) Important Files Changed
Sequence DiagramsequenceDiagram
participant UI as iOS/Android UI
participant App as AppManager (wallets state)
participant FFI as WalletsTable (Rust FFI)
participant DB as redb Database
UI->>App: read app.wallets (snapshot = previous)
UI->>App: optimistic update app.wallets = reordered
UI->>UI: haptic feedback (drag start)
UI->>FFI: reorderWallets(orderedIds) [background thread]
FFI->>DB: get_all() → sort_by_key(position)
FFI->>FFI: validate ordered_ids is a permutation
FFI->>DB: save_all_wallets() with new positions
FFI-->>UI: send WalletsChanged update
App->>App: app.wallets refreshed from DB
alt persist succeeds
UI->>UI: haptic success
else persist fails
UI->>App: rollback app.wallets = previous
end
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
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)
210-220:⚠️ Potential issue | 🟠 MajorNormalize positions before assigning the new wallet’s position.
wallets.len()only appends correctly when existing positions are contiguous. After deleting middle wallets, positions can have gaps; a new wallet can then receive a lower position than an existing wallet and sort above it instead of appearing at the end.Proposed fix
let mut wallet = wallet; + for (i, existing_wallet) in wallets.iter_mut().enumerate() { + existing_wallet.position = i as u32; + } wallet.position = wallets.len() as u32;🤖 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 210 - 220, The code currently sets wallet.position = wallets.len() which fails when positions have gaps; modify the insertion logic in the function using get_all and wallets to first normalize/reindex existing wallets' positions (e.g., sort wallets by position and assign positions 0..N-1) or compute the current maximum position and set the new wallet's position to max_position + 1, then proceed with should_backup_to_cloud cloning and wallets.push; ensure any normalization updates the persisted records or is reflected before using wallets elsewhere so ordering remains correct.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@rust/src/database.rs`:
- Around line 141-143: Database::init currently swallows failures from
wallets.migrate_positions() by logging and continuing; change it to propagate
the error so init fails on migration errors. Replace the if-let that calls
wallets.migrate_positions() so that any Err(err) is returned (converted into the
function's Error/Result type) instead of only logging; reference the
Database::init function and the wallets.migrate_positions() call to locate the
change and ensure the returned error preserves the original err message/context.
In `@rust/src/database/wallet.rs`:
- Around line 136-195: reorder_wallets and move_wallet currently read wallets in
a read transaction then later call save_all_wallets with a cloned vector, which
can overwrite newer metadata written between read and save; change both
functions to perform the read→modify→write inside a single write transaction (or
implement and call a position-only updater that re-reads the latest wallets and
patchs only the position fields) so that you either use the DB write transaction
around get_all + modifications + save_all_wallets, or re-fetch the current
wallet list just before persisting and only update positions (referencing
reorder_wallets, move_wallet, and save_all_wallets/position update helper).
---
Outside diff comments:
In `@rust/src/database/wallet.rs`:
- Around line 210-220: The code currently sets wallet.position = wallets.len()
which fails when positions have gaps; modify the insertion logic in the function
using get_all and wallets to first normalize/reindex existing wallets' positions
(e.g., sort wallets by position and assign positions 0..N-1) or compute the
current maximum position and set the new wallet's position to max_position + 1,
then proceed with should_backup_to_cloud cloning and wallets.push; ensure any
normalization updates the persisted records or is reflected before using wallets
elsewhere so ordering remains correct.
🪄 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: 33b4b351-bf12-4605-b31c-5d60487caf7c
⛔ 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/sidebar/SidebarView.ktandroid/app/src/main/java/org/bitcoinppl/cove_core/cove.ktios/Cove/Views/SidebarView.swiftrust/src/backup/model.rsrust/src/database.rsrust/src/database/wallet.rsrust/src/wallet/metadata.rs
diyaayay
force-pushed
the
cove-sort-wallets-hamburger-menu
branch
from
a8e441c to
5e380b7
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ios/Cove/Views/SidebarView.swift`:
- Around line 178-203: The persistWalletOrder flow is vulnerable to out-of-order
completions: update persistWalletOrder and handleMove so writes are serialized
or single-flight and success/rollback only apply if the current UI still matches
that request’s optimistic order; specifically, in persistWalletOrder use a
request token or compare appRef.wallets.map(\.id) to the orderedIds before doing
await MainActor.run updates (both success haptic and rollback), and on success
consume the returned [WalletMetadata] from
database.wallets().reorderWallets(...) to update appRef.wallets (positions)
instead of discarding it; also introduce a simple in-flight guard (e.g., a
single-flight Bool or serial Task queue) to prevent starting a new detach while
one is pending or to enqueue subsequent moves so persistence is serialized.
🪄 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: 2645647e-39b7-4ecb-bb19-60c927441e1b
⛔ 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/sidebar/SidebarView.ktandroid/app/src/main/java/org/bitcoinppl/cove_core/cove.ktios/Cove/Views/SidebarView.swiftrust/src/backup/model.rsrust/src/database.rsrust/src/database/wallet.rsrust/src/wallet/metadata.rs
✅ Files skipped from review due to trivial changes (2)
- rust/src/backup/model.rs
- rust/src/database.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- android/app/src/main/java/org/bitcoinppl/cove/sidebar/SidebarView.kt
- rust/src/database/wallet.rs
diyaayay
force-pushed
the
cove-sort-wallets-hamburger-menu
branch
from
5e380b7 to
e225e62
Compare
diyaayay
changed the title
diyaayay
force-pushed
the
cove-sort-wallets-hamburger-menu
branch
from
81a3428 to
c1ba778
Compare
praveenperera
left a comment
praveenperera
left a comment
There was a problem hiding this comment.
Serialize Android reorder writes so older drags can't win
If a user performs another reorder before the previous reorderWallets(...) call finishes, each drag launches an independent coroutine here and whichever write completes last becomes the persisted order. That means an older drag can overwrite the user's newer order even though the UI already showed the later arrangement. iOS already avoids this by chaining reorderTask, and Android needs the same serialization or version check.
|
Hey @diyaayay i have just a thought maybe i am wrong you can correct me but can we do the commit history bit more clean? like for example some commits are patching earlier commits in the same pr and the binding regeneration commits are floating separately when they should be squashed into the feature commits that triggered them. can we make the repo clean using 3 4 logical commits. What do you think? |
|
@Sandipmandal25 its alright, i'm just going to do a squash merge, I do prefer focused logical commits, but doing a squash merge is easier than trying to enforce this on each PR. Also having these messy commits make it easier to see each change that was made after a review. |
makes sense thanks! |
diyaayay
force-pushed
the
cove-sort-wallets-hamburger-menu
branch
from
c1ba778 to
54cf710
Compare
Persists are now serialized under a |
There was a problem hiding this comment.
Actionable comments posted: 3
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)
210-222:⚠️ Potential issue | 🟡 MinorRace: concurrent new-wallet inserts can mint duplicate
positionvalues.
get_allruns in a read transaction andsave_all_walletsopens its own write transaction, so two concurrentsave_new_wallet_metadata_with_backup_behaviorcalls (or a concurrent scan/metadata update that adds a wallet) can both observe the samemaxand both writeposition = max + 1.migrate_positions()will heal this on the next startup, but in the current sessionget_all's stable sort will leave the tie in insertion order, which can still produce surprising UI ordering until restart.Consider moving the read + max-computation + write into a single write transaction (a small
insert_new_wallet_txnhelper mirroringmutate_positions_txn) sopositionuniqueness is guaranteed per insert.🤖 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 210 - 222, The current flow in save_new_wallet_metadata_with_backup_behavior uses get_all (read transaction) to compute max position and then calls save_all_wallets (write transaction), which allows race conditions producing duplicate wallet.position; move the read+max+insert into a single write transaction by creating an insert_new_wallet_txn helper (similar to mutate_positions_txn) that opens a write txn, reads existing wallets, computes max position, assigns wallet.position = max+1, checks for duplicate id, appends the new wallet, and saves all wallets in that same txn; update save_new_wallet_metadata_with_backup_behavior to call insert_new_wallet_txn (and keep migrate_positions/mutate_positions_txn untouched).
♻️ Duplicate comments (1)
ios/Cove/Views/SidebarView.swift (1)
192-218:⚠️ Potential issue | 🟠 MajorRollback can still land on an unpersisted optimistic order.
Queued writes are serialized, but
previousis not guaranteed to be the last persisted state. If an earlier queued reorder fails as stale and a later one also fails, the later catch can restore the earlier optimistic order even though it never reached the database. Keep a confirmed authoritative baseline, refetch on failure, or make reorder persistence single-flight; also trigger success haptics only inside the same “still current” guard.🤖 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 192 - 218, persistWalletOrder currently restores the optimistic `previous` state on any failure which can reapply an order that was never persisted; change the error-path to re-sync with the authoritative persisted state instead of blindly setting `appRef.wallets = previous`: in the catch for Task in persistWalletOrder call the persistence layer (database.wallets().list or similar) to fetch the current persisted wallet order and assign that to appRef.wallets only if appRef.wallets.map(\.id) == orderedIds (i.e. still matches the optimistic order), and move HapticFeedback.success.trigger() inside the same MainActor guard that checks the appRef.wallets ids before assigning `persisted` so haptics only fire when the persisted result is still current; alternatively make reorderTask single-flight so only the latest reorder is applied.
🧹 Nitpick comments (2)
rust/src/database/wallet.rs (2)
320-328: Nit: inconsistent empty-vec fallback.
get_alluses.unwrap_or(vec![])(line 324) while the newmutate_positions_txnuses.unwrap_or_default()(line 458). Preferunwrap_or_default()in both for consistency and to avoid the small eager allocation on the hot path.♻️ Proposed tweak
- .unwrap_or(vec![]); + .unwrap_or_default();Also applies to: 454-458
🤖 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 320 - 328, In get_all and mutate_positions_txn in wallet.rs replace the eager fallback .unwrap_or(vec![]) with .unwrap_or_default() so the empty Vec<WalletMetadata> uses the default allocation-free constructor; locate the .get(key.as_str()).map_err_str(WalletTableError::ReadError)?...map(|value| value.value()).unwrap_or(vec![]) in the get_all implementation and the analogous unwrap in mutate_positions_txn and switch both to .unwrap_or_default().
239-276:migrate_positionsonly fires on duplicate positions, not on gaps.The duplicate-only trigger is a reasonable scope for this migration, but note that pre-existing non-contiguous positions (e.g.,
[0, 2, 5]from a partially applied older build) won't be normalized. Functionally fine becauseget_allsorts byposition, and the insertion path usesmax + 1so gaps don't cause collisions — just flagging so it's an intentional decision. If you'd rather normalize aggressively, checkwallets.iter().enumerate().any(|(i, w)| w.position as usize != i)instead of just duplicates.🤖 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 239 - 276, The migration currently only runs when duplicates are found; to also normalize non-contiguous positions (gaps) update the trigger in migrate_positions to detect any wallet whose position does not match its sorted index: replace the duplicate check that uses seen.insert on wallets.iter() with an enumerate-based check like wallets.iter().enumerate().any(|(i, w)| w.position as usize != i) and then proceed to rebuild migrated from wallets and call self.save_all_wallets(network, mode, migrated) as before; keep using get_all(network, mode) which returns wallets sorted by position.
🤖 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 190-194: The rollback restores the already-mutated optimistic
order because `previous` is captured after `walletList` is changed; fix by
capturing the pre-drag snapshot of the wallet order when the drag begins (e.g.,
in the `onDragStart` handler) and store it in a dedicated variable (like
`previousWalletOrder` or reuse `previous`) before any reordering occurs, then
use that snapshot in `onRollback` to restore the true pre-drag state; update
references to `walletList`, `onDragStart`, `draggedWalletId`, `draggedOffsetY`,
and `onRollback` accordingly so the captured order is taken prior to mutation.
- Around line 58-62: The Rust FFI call reorderWallets() in SidebarView.kt is
synchronous and should not be wrapped in withContext(Dispatchers.IO); remove the
withContext(Dispatchers.IO) wrapper around reorderWallets() in the coroutine
blocks (the launch + withContext sequence that currently calls reorderWallets())
and call reorderWallets() directly inside the existing coroutine or protected
Mutex.withLock block (same change for the second occurrence referenced around
the file). Ensure you keep the surrounding coroutine scope, Mutex locking
(Mutex.withLock), and error handling intact while eliminating the extra
Dispatchers.IO context switch.
- Around line 337-340: The reorderWallets success path currently calls
onAdoptAuthoritative which only mutates the local walletList, leaving
app.wallets stale until a Rust-originated WalletsChanged reconciliation arrives;
to avoid temporary ordering inconsistencies, after
onAdoptAuthoritative(authoritative) succeeds (inside the result.onSuccess block
where haptics.performHapticFeedback is called), either invoke app.loadWallets()
to immediately refresh the shared app.wallets state or ensure reorderWallets
emits a WalletsChanged event that is processed before any sidebar
recreation—update the code in SidebarView.kt to call app.loadWallets() after
onAdoptAuthoritative or add confirmation that WalletsChanged is synchronously
triggered and handled.
---
Outside diff comments:
In `@rust/src/database/wallet.rs`:
- Around line 210-222: The current flow in
save_new_wallet_metadata_with_backup_behavior uses get_all (read transaction) to
compute max position and then calls save_all_wallets (write transaction), which
allows race conditions producing duplicate wallet.position; move the
read+max+insert into a single write transaction by creating an
insert_new_wallet_txn helper (similar to mutate_positions_txn) that opens a
write txn, reads existing wallets, computes max position, assigns
wallet.position = max+1, checks for duplicate id, appends the new wallet, and
saves all wallets in that same txn; update
save_new_wallet_metadata_with_backup_behavior to call insert_new_wallet_txn (and
keep migrate_positions/mutate_positions_txn untouched).
---
Duplicate comments:
In `@ios/Cove/Views/SidebarView.swift`:
- Around line 192-218: persistWalletOrder currently restores the optimistic
`previous` state on any failure which can reapply an order that was never
persisted; change the error-path to re-sync with the authoritative persisted
state instead of blindly setting `appRef.wallets = previous`: in the catch for
Task in persistWalletOrder call the persistence layer (database.wallets().list
or similar) to fetch the current persisted wallet order and assign that to
appRef.wallets only if appRef.wallets.map(\.id) == orderedIds (i.e. still
matches the optimistic order), and move HapticFeedback.success.trigger() inside
the same MainActor guard that checks the appRef.wallets ids before assigning
`persisted` so haptics only fire when the persisted result is still current;
alternatively make reorderTask single-flight so only the latest reorder is
applied.
---
Nitpick comments:
In `@rust/src/database/wallet.rs`:
- Around line 320-328: In get_all and mutate_positions_txn in wallet.rs replace
the eager fallback .unwrap_or(vec![]) with .unwrap_or_default() so the empty
Vec<WalletMetadata> uses the default allocation-free constructor; locate the
.get(key.as_str()).map_err_str(WalletTableError::ReadError)?...map(|value|
value.value()).unwrap_or(vec![]) in the get_all implementation and the analogous
unwrap in mutate_positions_txn and switch both to .unwrap_or_default().
- Around line 239-276: The migration currently only runs when duplicates are
found; to also normalize non-contiguous positions (gaps) update the trigger in
migrate_positions to detect any wallet whose position does not match its sorted
index: replace the duplicate check that uses seen.insert on wallets.iter() with
an enumerate-based check like wallets.iter().enumerate().any(|(i, w)| w.position
as usize != i) and then proceed to rebuild migrated from wallets and call
self.save_all_wallets(network, mode, migrated) as before; keep using
get_all(network, mode) which returns wallets sorted by position.
🪄 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: 8867c58f-dbaa-4e34-ae92-97204b8fc038
⛔ 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/sidebar/SidebarView.ktandroid/app/src/main/java/org/bitcoinppl/cove_core/cove.ktios/Cove/Views/SidebarView.swiftrust/src/backup/model.rsrust/src/database.rsrust/src/database/wallet.rsrust/src/wallet/metadata.rs
✅ Files skipped from review due to trivial changes (1)
- rust/src/backup/model.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- rust/src/database.rs
diyaayay
force-pushed
the
cove-sort-wallets-hamburger-menu
branch
2 times, most recently
from
4304349 to
082d8ca
Compare
praveenperera
left a comment
praveenperera
left a comment
There was a problem hiding this comment.
I think the Mutex is functionally correct, but I’d prefer sequencing these writes by chaining off the previous reorder Job instead. What we’re protecting here isn’t really shared mutable state, it’s the ordering of side effects: we need reorder persists to run strictly in drag order so an older drag can’t finish later and write a stale wallet order back over a newer one.
Using a pendingReorderJob?.join() style flow would make that intent more explicit, and it would also bring Android in line with iOS, which is already doing the equivalent by awaiting the previous Task before starting the next persist. Keeping both platforms on the same serialization pattern will make this code easier to reason about, easier to review, and less likely to drift into subtle platform-specific behavior the next time we touch wallet reordering.
diyaayay
force-pushed
the
cove-sort-wallets-hamburger-menu
branch
from
082d8ca to
1eb4756
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
ios/Cove/Views/SidebarView.swift (1)
192-219: Reorder task chain is never cleared.
reorderTaskis reassigned each call viareorderTask = Task { await pending?.value … }, but after the final task completes it's retained indefinitely. Not a functional bug (subsequentawait pending?.valuereturns immediately on a finished Task), but you may want toself.reorderTask = nilafter the work completes so the task reference can be released, particularly since each task capturesappRefanddatabase.Optional cleanup
- private func persistWalletOrder(orderedIds: [WalletId]) { - let database = app.database - let appRef = app - let pending = reorderTask - reorderTask = Task { - await pending?.value - - do { + private func persistWalletOrder(orderedIds: [WalletId]) { + let database = app.database + let appRef = app + let pending = reorderTask + let task = Task { + await pending?.value + do { let persisted = try await Task.detached { try database.wallets().reorderWallets(orderedIds: orderedIds) }.value - await MainActor.run { if appRef.wallets.map(\.id) == orderedIds { appRef.wallets = persisted HapticFeedback.success.trigger() } } } catch { Log.error("Failed to reorder wallets: \(error)") await MainActor.run { if appRef.wallets.map(\.id) == orderedIds { appRef.loadWallets() } } } } + reorderTask = task + Task { `@MainActor` in + await task.value + if reorderTask === task { reorderTask = nil } + } }🤖 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 192 - 219, The persistWalletOrder Task stored in reorderTask is never cleared, causing retained captures of appRef and database; update persistWalletOrder so that after the Task's work (both success and error paths) completes you set self.reorderTask = nil (ensure this is done on the correct actor/MainActor as needed) — locate the Task creation in persistWalletOrder (where reorderTask = Task { await pending?.value ... }) and add logic at the end of the Task body (or in finally-style handling) to clear reorderTask so the reference is released.android/app/src/main/java/org/bitcoinppl/cove/sidebar/SidebarView.kt (2)
232-240: Redundant DB read in the success path.
onAdoptAuthoritativealready receives the authoritativeList<WalletMetadata>returned byreorderWallets, but the callback then callsapp.loadWallets(), which re-readsdatabase.wallets().all()from Main. You can avoid the extra synchronous DB hit by assigning the list directly, e.g.app.wallets = authoritative(or expose a small setter onAppManager). Same idea applies toonFailureResync— if you already didapp.loadWallets(), assigningwalletList = app.walletson the next line is only guaranteed to match ifloadWalletsran synchronously, which is the case today but ties you to that implementation detail.Suggested simplification
- onAdoptAuthoritative = { authoritative -> - walletList = authoritative - app.loadWallets() - }, + onAdoptAuthoritative = { authoritative -> + app.wallets = authoritative + walletList = authoritative + },🤖 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 232 - 240, The callbacks are causing redundant DB reads: in onAdoptAuthoritative you receive the authoritative List<WalletMetadata> from reorderWallets but then call app.loadWallets() which re-reads the DB; change the handler to assign that list directly (e.g., set app.wallets or a new setter on AppManager) and set walletList = authoritative instead of calling app.loadWallets(); likewise in onFailureResync avoid the pattern app.loadWallets(); walletList = app.wallets — either call loadWallets() alone or, if you already have the updated list from reorderWallets/failure path, assign it directly to app.wallets and walletList to avoid relying on synchronous loadWallets() behavior.
95-95:pendingReorderJobdoesn't need to bemutableStateOf.Nothing in the composition reads
pendingReorderJob; it's only captured by theonDragEndclosure for chaining. Wrapping it inmutableStateOfcauses an unnecessary recomposition ofSidebarViewevery time a reorder starts/completes. Using a plainremember-held holder avoids that:Suggested change
- var pendingReorderJob by remember { mutableStateOf<Job?>(null) } + val pendingReorderJobRef = remember { arrayOfNulls<Job>(1) } @@ - onDragEnd = { - val orderedIds = walletList.map { it.id } - pendingReorderJob = - persistReorder( - … - previousJob = pendingReorderJob, - … - ) + onDragEnd = { + val orderedIds = walletList.map { it.id } + pendingReorderJobRef[0] = + persistReorder( + … + previousJob = pendingReorderJobRef[0], + … + )🤖 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 95, The variable pendingReorderJob in SidebarView is declared with mutableStateOf causing unnecessary recompositions; change it to a plain remember-held mutable holder (e.g., use remember { arrayOfNulls<Job?>(1) } or an AtomicReference) and update all reads/writes to use that holder (replace uses of pendingReorderJob with holder[0] or ref.get()/set()) so onDragEnd can still access and chain the Job but the composition won't recompose on changes.
🤖 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`:
- Line 93: The shared mutable state itemHeightPx used by WalletItem leads to
incorrect threshold math (step = itemHeightPx + spacingPx) when rows differ;
change to track height per-wallet (e.g., use a mutableStateMapOf<WalletId,
Float>() keyed by the wallet's id) or store/measure height for the currently
dragged item only, update reads/writes to use that per-id height instead of
itemHeightPx, and adjust the threshold calculation (step) to reference the
per-wallet height for the dragged item's id so swaps use the correct row height.
---
Nitpick comments:
In `@android/app/src/main/java/org/bitcoinppl/cove/sidebar/SidebarView.kt`:
- Around line 232-240: The callbacks are causing redundant DB reads: in
onAdoptAuthoritative you receive the authoritative List<WalletMetadata> from
reorderWallets but then call app.loadWallets() which re-reads the DB; change the
handler to assign that list directly (e.g., set app.wallets or a new setter on
AppManager) and set walletList = authoritative instead of calling
app.loadWallets(); likewise in onFailureResync avoid the pattern
app.loadWallets(); walletList = app.wallets — either call loadWallets() alone
or, if you already have the updated list from reorderWallets/failure path,
assign it directly to app.wallets and walletList to avoid relying on synchronous
loadWallets() behavior.
- Line 95: The variable pendingReorderJob in SidebarView is declared with
mutableStateOf causing unnecessary recompositions; change it to a plain
remember-held mutable holder (e.g., use remember { arrayOfNulls<Job?>(1) } or an
AtomicReference) and update all reads/writes to use that holder (replace uses of
pendingReorderJob with holder[0] or ref.get()/set()) so onDragEnd can still
access and chain the Job but the composition won't recompose on changes.
In `@ios/Cove/Views/SidebarView.swift`:
- Around line 192-219: The persistWalletOrder Task stored in reorderTask is
never cleared, causing retained captures of appRef and database; update
persistWalletOrder so that after the Task's work (both success and error paths)
completes you set self.reorderTask = nil (ensure this is done on the correct
actor/MainActor as needed) — locate the Task creation in persistWalletOrder
(where reorderTask = Task { await pending?.value ... }) and add logic at the end
of the Task body (or in finally-style handling) to clear reorderTask so the
reference is released.
🪄 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: 076ebc96-95a4-40c1-85aa-ceb6cd870eca
📒 Files selected for processing (2)
android/app/src/main/java/org/bitcoinppl/cove/sidebar/SidebarView.ktios/Cove/Views/SidebarView.swift
@praveenperera Switched to |
|
thanks @diyaayay this looks good, but I need to do some manual testing on device, will do that and merge soon. Thanks for your contribution. |
praveenperera
force-pushed
the
cove-sort-wallets-hamburger-menu
branch
from
1eb4756 to
7aeafad
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
rust/src/database.rs (1)
141-143:⚠️ Potential issue | 🟠 MajorDon't continue startup after
migrate_positions()fails.This migration repairs the persisted wallet-order invariant. Logging and returning
Okhere can leave duplicate/brokenpositiondata in place, so the app comes up with ordering semantics partially enabled and can show the wrong order after restart. Failinit()instead of treating this as best-effort.🐛 Proposed fix
- if let Err(err) = wallets.migrate_positions() { - error!("wallet position migration failed: {err}"); - } + wallets.migrate_positions()?;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/src/database.rs` around lines 141 - 143, The current code swallows errors from wallets.migrate_positions() and continues startup; instead propagate failure so init() returns Err and prevents the app from starting with broken position state. Replace the if-let logging branch in init() that calls wallets.migrate_positions() so that any error is returned (e.g., use ? or map the error into init()'s error type) rather than logging and returning Ok, ensuring wallets.migrate_positions() failures abort initialization and bubble up to the caller.rust/src/database/wallet.rs (1)
239-272:⚠️ Potential issue | 🟠 MajorMake
migrate_positionsatomic to avoid stale-clone overwrite.Line 248 reads via
get_all, then Line 271 writes viasave_all_walletsin a separate transaction. A concurrent metadata write between those calls can still be clobbered by the migration save.Suggested fix (single write txn per (network, mode) migration pass)
pub fn migrate_positions(&self) -> Result<(), Error> { use std::collections::HashSet; use strum::IntoEnumIterator; for network in Network::iter() { for mode in WalletMode::iter() { - let wallets = self.get_all(network, mode)?; - - if wallets.len() <= 1 { - continue; - } - - let mut seen = HashSet::with_capacity(wallets.len()); - let has_duplicates = wallets.iter().any(|w| !seen.insert(w.position)); - if !has_duplicates { - continue; - } - - debug!("migrating wallet positions for {network}/{mode}"); - - let migrated: Vec<WalletMetadata> = wallets - .into_iter() - .enumerate() - .map(|(i, mut w)| { - w.position = i as u32; - w - }) - .collect(); - - self.save_all_wallets(network, mode, migrated)?; + let write_txn = self.db.begin_write()?; + let key = WalletKey::from((network, mode)).to_string(); + + let changed = { + let mut table = write_txn.open_table(TABLE)?; + let mut wallets: Vec<WalletMetadata> = table + .get(key.as_str()) + .map_err_str(WalletTableError::ReadError)? + .map(|value| value.value()) + .unwrap_or_default(); + + wallets.sort_by_key(|w| w.position); + if wallets.len() <= 1 { + false + } else { + let mut seen = HashSet::with_capacity(wallets.len()); + let has_duplicates = wallets.iter().any(|w| !seen.insert(w.position)); + if has_duplicates { + debug!("migrating wallet positions for {network}/{mode}"); + for (i, w) in wallets.iter_mut().enumerate() { + w.position = i as u32; + } + table + .insert(&*key, wallets) + .map_err_str(WalletTableError::SaveError)?; + } + has_duplicates + } + }; + + if changed { + write_txn + .commit() + .map_err_str(WalletTableError::SaveError)?; + } } } Ok(()) }🤖 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 239 - 272, migrate_positions currently reads wallets with get_all then later calls save_all_wallets which runs a separate write txn, allowing concurrent writes to be lost; make the migration atomic by performing the read-and-update inside a single write transaction: for each Network::iter()/WalletMode::iter() pair open one write txn, fetch wallets using a txn-aware read (or call a new get_all_with_txn variant), detect duplicates and adjust positions in-place, then write them back with a txn-aware save_all_wallets (or a new save_all_wallets_with_txn) and commit the txn; update or add txn-accepting overloads for get_all and save_all_wallets and use those from migrate_positions so the read and write happen inside the same transaction.
🤖 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 222-240: onDragEnd currently always calls persistReorder even when
the order didn't change; fix by capturing the pre-drag order (e.g.,
originalOrderedIds = walletList.map { it.id } in the drag start handler) and in
onDragEnd compare that saved originalOrderedIds to the current walletList.map {
it.id } and only call persistReorder (and set pendingReorderJob) when they
differ; update references to onDragEnd, persistReorder, pendingReorderJob and
walletList so unchanged drags are skipped and no write/haptic is triggered.
---
Duplicate comments:
In `@rust/src/database.rs`:
- Around line 141-143: The current code swallows errors from
wallets.migrate_positions() and continues startup; instead propagate failure so
init() returns Err and prevents the app from starting with broken position
state. Replace the if-let logging branch in init() that calls
wallets.migrate_positions() so that any error is returned (e.g., use ? or map
the error into init()'s error type) rather than logging and returning Ok,
ensuring wallets.migrate_positions() failures abort initialization and bubble up
to the caller.
In `@rust/src/database/wallet.rs`:
- Around line 239-272: migrate_positions currently reads wallets with get_all
then later calls save_all_wallets which runs a separate write txn, allowing
concurrent writes to be lost; make the migration atomic by performing the
read-and-update inside a single write transaction: for each
Network::iter()/WalletMode::iter() pair open one write txn, fetch wallets using
a txn-aware read (or call a new get_all_with_txn variant), detect duplicates and
adjust positions in-place, then write them back with a txn-aware
save_all_wallets (or a new save_all_wallets_with_txn) and commit the txn; update
or add txn-accepting overloads for get_all and save_all_wallets and use those
from migrate_positions so the read and write happen inside the same transaction.
🪄 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: 9af96881-bb00-4fea-964b-a93f872073b2
⛔ 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/sidebar/SidebarView.ktandroid/app/src/main/java/org/bitcoinppl/cove_core/cove.ktios/Cove/Views/SidebarView.swiftrust/src/backup/model.rsrust/src/database.rsrust/src/database/wallet.rsrust/src/wallet/metadata.rs
✅ Files skipped from review due to trivial changes (2)
- rust/src/backup/model.rs
- rust/src/wallet/metadata.rs
| onDragEnd = { | ||
| val orderedIds = walletList.map { it.id } | ||
| pendingReorderJob = | ||
| persistReorder( | ||
| scope = scope, | ||
| app = app, | ||
| previousJob = pendingReorderJob, | ||
| orderedIds = orderedIds, | ||
| haptics = haptics, | ||
| currentOptimisticIds = { walletList.map { it.id } }, | ||
| onAdoptAuthoritative = { authoritative -> | ||
| walletList = authoritative | ||
| app.loadWallets() | ||
| }, | ||
| onFailureResync = { | ||
| app.loadWallets() | ||
| walletList = app.wallets | ||
| }, | ||
| ) |
There was a problem hiding this comment.
Skip persistence when the drag didn’t actually reorder anything.
onDragEnd always queues reorderWallets(). A long-press/release that never crosses a swap threshold still does a write transaction and later emits the success haptic, even though the order is unchanged.
💡 Proposed fix
onDragEnd = {
val orderedIds = walletList.map { it.id }
- pendingReorderJob =
- persistReorder(
- scope = scope,
- app = app,
- previousJob = pendingReorderJob,
- orderedIds = orderedIds,
- haptics = haptics,
- currentOptimisticIds = { walletList.map { it.id } },
- onAdoptAuthoritative = { authoritative ->
- walletList = authoritative
- app.loadWallets()
- },
- onFailureResync = {
- app.loadWallets()
- walletList = app.wallets
- },
- )
+ if (orderedIds != preDragSnapshot.map { it.id }) {
+ pendingReorderJob =
+ persistReorder(
+ scope = scope,
+ app = app,
+ previousJob = pendingReorderJob,
+ orderedIds = orderedIds,
+ haptics = haptics,
+ currentOptimisticIds = { walletList.map { it.id } },
+ onAdoptAuthoritative = { authoritative ->
+ walletList = authoritative
+ app.loadWallets()
+ },
+ onFailureResync = {
+ app.loadWallets()
+ walletList = app.wallets
+ },
+ )
+ }
draggedWalletId = null
draggedOffsetY = 0f
},🤖 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 222 - 240, onDragEnd currently always calls persistReorder even when the
order didn't change; fix by capturing the pre-drag order (e.g.,
originalOrderedIds = walletList.map { it.id } in the drag start handler) and in
onDragEnd compare that saved originalOrderedIds to the current walletList.map {
it.id } and only call persistReorder (and set pendingReorderJob) when they
differ; update references to onDragEnd, persistReorder, pendingReorderJob and
walletList so unchanged drags are skipped and no write/haptic is triggered.
7aeafad to
81c0c42
Compare
…plicate aware migration
praveenperera
force-pushed
the
cove-sort-wallets-hamburger-menu
branch
from
81c0c42 to
a918d7a
Compare
3 participants
Summary
Adds wallet reordering in the hamburger menu (closes #493).
position: u32onWalletMetadata, stable-sortedget_all(), one-timemigrate_positions()backfill on startup.reorderWallets(orderedIds)andmoveWallet(walletId, toPosition); newWalletTableError::{WalletNotFound, ReorderMismatch}.SidebarViewusingList+.onMove+EditMode; haptics on drag start and on successful persist; optimistic update with rollback on FFI error; edit-mode resets when the sidebar closes.SidebarView.kt; haptics; visual elevation (scale + shadow + zIndex); smoothanimateItem()for neighbors; FFI persisted off the UI thread; optimistic update with rollback on error.Testing
cargo fmt --check— passcargo clippy --all-features -- -D warnings— passcargo test --package cove --lib— passes aside from pre-existingdatabase::migration::bdk/database::encrypted_backendflakiness (reproduced onmaster; unrelated to this PR)just ba— Android build + Kotlin bindings regenerated successfullyjust bi— iOS build + Swift bindings regenerated successfullyManual: reordered wallets on an Android device (Pixel) and on the iOS simulator; killed and reopened the app — wallet order persisted in the new position on both.
Platform Coverage
Checklist
Summary by CodeRabbit
New Features
Improvements
Bug Fixes