fix: move list_core_wallets() RPC off the UI thread#719
Merged
Conversation
Move synchronous list_core_wallets() RPC calls off the UI thread to prevent UI freezes when Dash Core is slow or unreachable. - Add CoreTask::ListCoreWallets backend task variant - Add BackendTaskSuccessResult::CoreWalletsList result variant - Convert AddNewWalletScreen constructor to async fetch pattern - Convert ImportMnemonicScreen constructor to async fetch pattern - Convert WalletsBalancesScreen::display_task_error to async fetch - Add error recovery (retry on failure) and network-switch reset - Remove TODO comments CMT-004, CMT-007, CMT-008 Closes #700 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
📝 WalkthroughWalkthroughAdds an asynchronous Core wallet listing flow: new backend task variant Changes
Sequence DiagramsequenceDiagram
participant UI as UI Screen
participant Context as App Context
participant Backend as Backend Task Queue
participant Core as Core Wallets API
UI->>UI: render() detects cache empty & not loading
UI->>Context: dispatch CoreTask::ListCoreWallets
Context->>Backend: enqueue async task
UI->>UI: set loading flag / pending state
Backend->>Core: list_core_wallets()
Core->>Backend: Vec<String> result
Backend->>Context: deliver BackendTaskSuccessResult::CoreWalletsList
Context->>UI: deliver task result
alt Single Wallet
UI->>UI: auto-select stored wallet & refresh
else Multiple Wallets
UI->>UI: open selection dialog with wallets
else No Wallets
UI->>UI: display error banner
end
UI->>UI: clear loading / pending flags
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly Related Issues
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claudius the Magnificent has inspected your offering and finds it... acceptable. Clean, well-structured async migration. All three call sites correctly follow the app task system pattern. No MEDIUM+ findings. The manual test scenarios doc is thorough - 16 scenarios covering happy paths, edge cases, error recovery, and network switches.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
src/ui/wallets/wallets_screen/mod.rs (1)
2102-2133: Consider early return when no wallet is selected.If both
selected_walletandselected_single_key_walletareNone(or their locks are poisoned),wallet_hashwill beNone. TheListCoreWalletstask will still be dispatched, but indisplay_task_result, the auto-selection path (line 2293) requiresSome(hash), and the dialog path storesNoneinpending_core_wallet_seed_hash, causing the selection confirmation to silently fail.Proposed improvement
self.pending_list_core_wallets = true; self.pending_list_wallet_hash = wallet_hash; self.pending_list_is_single_key = is_single_key; + + if wallet_hash.is_none() { + // No wallet selected - show error instead of dispatching useless task + MessageBanner::set_global( + self.app_context.egui_ctx(), + "Core wallet not configured. Please select a wallet first.", + MessageType::Error, + ); + self.pending_list_core_wallets = false; + } true // Suppress generic error banner🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/wallets/wallets_screen/mod.rs` around lines 2102 - 2133, In display_task_error, if neither selected_wallet nor selected_single_key_wallet yields a hash then don't schedule the ListCoreWallets flow: detect wallet_hash.is_none() after computing (wallet_hash, is_single_key) and early-return false instead of setting pending_list_core_wallets/pending_list_wallet_hash/pending_list_is_single_key; this prevents dispatching a task with None that later breaks display_task_result's auto-selection and dialog paths (refer to selected_wallet, selected_single_key_wallet, pending_list_core_wallets, pending_list_wallet_hash, pending_list_is_single_key).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/ai-design/2026-03-10-async-list-core-wallets/manual-test-scenarios.md`:
- Around line 98-102: The save logic reads core_wallets[0] even when the UI
ComboBox is hidden, causing an implicit core wallet to be persisted; update
AddNewWalletScreen::save_wallet() and ImportMnemonicScreen::save_*() to check
the UI visibility/state (or a flag indicating the ComboBox was shown/selected)
and only assign core_wallet_name when the user actually selected a core wallet;
when the ComboBox is hidden or no selection was made, explicitly set
core_wallet_name = NULL (or equivalent) before writing to SQLite so the saved
record matches the hidden-ComboBox behavior described in ACW-003.
In `@src/backend_task/mod.rs`:
- Around line 275-276: The CoreWalletsList enum variant currently carries only
Vec<String>, which allows stale Mainnet results to clobber Testnet UI; change
the CoreWalletsList variant to include origin metadata (e.g., a Network enum
plus the Vec<String> or a small struct like CoreWalletsPayload { network:
Network, wallets: Vec<String> }) so consumers can detect origin; then update
consumers such as AddNewWalletScreen and ImportMnemonicScreen to ignore
CoreWalletsList results whose payload.network does not match app_context.network
(also adjust any code around change_context() that awaits or dispatches
CoreWalletsList accordingly).
In `@src/ui/wallets/add_new_wallet_screen.rs`:
- Around line 730-733: When handling BackendTaskSuccessResult::CoreWalletsList
in display_task_result, clamp self.selected_core_wallet_index to the new wallets
list length after assigning self.core_wallets to avoid out-of-bounds access when
the refreshed list is shorter; locate the display_task_result method and after
setting self.core_wallets = Some(wallets) update selected_core_wallet_index (or
set to None/0 as appropriate) using min(current_index,
wallets.len().saturating_sub(1)) or equivalent bounds-checking so subsequent
indexing (e.g., where the ComboBox reads
self.core_wallets[self.selected_core_wallet_index]) cannot panic.
In `@src/ui/wallets/import_mnemonic_screen.rs`:
- Around line 536-539: When handling BackendTaskSuccessResult::CoreWalletsList
in display_task_result, clamp self.selected_core_wallet_index to the new list
length before storing the wallet list to avoid out-of-bounds indexing later;
after you set self.core_wallets = Some(wallets) (or immediately before), compute
let max_index = wallets.len().saturating_sub(1) and set
self.selected_core_wallet_index = std::cmp::min(self.selected_core_wallet_index,
max_index) (and if wallets.is_empty(), set selected_core_wallet_index = 0) so
subsequent indexing of core_wallets[self.selected_core_wallet_index] is safe.
In `@src/ui/wallets/wallets_screen/mod.rs`:
- Around line 2022-2026: There’s a race where multiple CoreWalletNotConfigured
errors can overwrite pending_list_wallet_hash while a ListCoreWallets task is
already in flight; add an in-flight guard field (e.g.,
pending_list_core_wallet_inflight: bool) to the struct and use it when
setting/clearing pending_list_core_wallet_hash and when dispatching
AppAction::BackendTask(BackendTask::CoreTask(CoreTask::ListCoreWallets)) so you
only set pending_list_wallet_hash if no in-flight task exists, set
pending_list_core_wallet_inflight = true when you dispatch the ListCoreWallets
task (where pending_list_core_wallets is currently toggled), and clear
pending_list_core_wallet_inflight when display_task_result for the
ListCoreWallets response completes; ensure CoreWalletNotConfigured handling
checks the in-flight flag before overwriting pending_list_wallet_hash.
---
Nitpick comments:
In `@src/ui/wallets/wallets_screen/mod.rs`:
- Around line 2102-2133: In display_task_error, if neither selected_wallet nor
selected_single_key_wallet yields a hash then don't schedule the ListCoreWallets
flow: detect wallet_hash.is_none() after computing (wallet_hash, is_single_key)
and early-return false instead of setting
pending_list_core_wallets/pending_list_wallet_hash/pending_list_is_single_key;
this prevents dispatching a task with None that later breaks
display_task_result's auto-selection and dialog paths (refer to selected_wallet,
selected_single_key_wallet, pending_list_core_wallets, pending_list_wallet_hash,
pending_list_is_single_key).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 57c55125-41f9-4448-96a0-ae2c620d4cfe
📒 Files selected for processing (7)
docs/ai-design/2026-03-10-async-list-core-wallets/manual-test-scenarios.mdsrc/backend_task/core/mod.rssrc/backend_task/mod.rssrc/ui/mod.rssrc/ui/wallets/add_new_wallet_screen.rssrc/ui/wallets/import_mnemonic_screen.rssrc/ui/wallets/wallets_screen/mod.rs
3 tasks
Prevent potential out-of-bounds panic when display_task_result receives a shorter wallet list than the currently selected index. Also corrects the ACW-003 test scenario to match actual save behavior (auto-assigns sole wallet rather than NULL). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/ui/wallets/add_new_wallet_screen.rs (1)
739-742: Consider filtering error types before resetting loading state.
display_task_errorresetscore_wallets_loadingfor any backend error, not just errors fromListCoreWallets. If a different backend task fails while the wallet list fetch is in flight, this could incorrectly reset the loading flag and trigger a duplicate dispatch.However, given the current architecture where task results are routed to the visible screen without task correlation (per context snippet 1), this is likely acceptable as a pragmatic simplification — the worst case is an extra
listwalletsRPC call.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/wallets/add_new_wallet_screen.rs` around lines 739 - 742, display_task_error currently clears self.core_wallets_loading for every TaskError, causing unrelated task failures to reset the wallet-list loading state; update display_task_error in add_new_wallet_screen.rs to only clear core_wallets_loading when the error corresponds to the wallet-list task (e.g., match the TaskError variant or backend error name associated with ListCoreWallets or the specific task id used to fetch core wallets) and otherwise leave core_wallets_loading untouched; implement a match on the TaskError enum (or inspect BackendError::ListCoreWallets) and only set self.core_wallets_loading = false and return false in that branch, returning the existing behavior for other errors.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/ai-design/2026-03-10-async-list-core-wallets/manual-test-scenarios.md`:
- Around line 324-355: Update ACW-012 and ACW-013 to remove the incorrect
assertion that Create Wallet and Import Wallet persist as root screens and
receive context updates on network switch; explicitly state in preconditions
that these screens are modal/detail screens pushed on screen_stack (not
RootScreenType/main_screens), and in expected results replace the claim that
reset_core_wallets_cache() is invoked via change_context() with the correct
behavior: change_network() only calls change_context() for main_screens
(RootScreenType), so modal screens on screen_stack will retain stale
core_wallets until dismissed, and only after returning to a root screen or
reopening the modal will reset_core_wallets_cache() and ListCoreWallets run for
the new network.
---
Nitpick comments:
In `@src/ui/wallets/add_new_wallet_screen.rs`:
- Around line 739-742: display_task_error currently clears
self.core_wallets_loading for every TaskError, causing unrelated task failures
to reset the wallet-list loading state; update display_task_error in
add_new_wallet_screen.rs to only clear core_wallets_loading when the error
corresponds to the wallet-list task (e.g., match the TaskError variant or
backend error name associated with ListCoreWallets or the specific task id used
to fetch core wallets) and otherwise leave core_wallets_loading untouched;
implement a match on the TaskError enum (or inspect
BackendError::ListCoreWallets) and only set self.core_wallets_loading = false
and return false in that branch, returning the existing behavior for other
errors.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 47864fa7-d87e-48e2-90b7-4b52b5e10071
📒 Files selected for processing (3)
docs/ai-design/2026-03-10-async-list-core-wallets/manual-test-scenarios.mdsrc/ui/wallets/add_new_wallet_screen.rssrc/ui/wallets/import_mnemonic_screen.rs
- SEC-001: Set core_wallets = Some(vec![]) on fetch error to break 60fps retry loop in AddNewWalletScreen and ImportMnemonicScreen - SEC-002: Clear pending_list_* state in WalletsBalancesScreen on network switch to prevent stale wallet hash from wrong network - ACW-012/013: Correct test scenarios — modal screens on screen_stack don't receive change_context() from change_network() Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/ui/wallets/import_mnemonic_screen.rs (1)
536-543: Consider resettingcore_wallets_loadingon success for consistency.The error path (line 546) resets
core_wallets_loading = false, but the success path does not. While this isn't a bug (the dispatch guard checkscore_wallets.is_none()first), resetting it on success would make the state transitions symmetric and avoid confusion if the logic is refactored later.♻️ Optional defensive fix
fn display_task_result(&mut self, backend_task_success_result: BackendTaskSuccessResult) { if let BackendTaskSuccessResult::CoreWalletsList(wallets) = backend_task_success_result { + self.core_wallets_loading = false; self.selected_core_wallet_index = self .selected_core_wallet_index .min(wallets.len().saturating_sub(1)); self.core_wallets = Some(wallets); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/wallets/import_mnemonic_screen.rs` around lines 536 - 543, The success branch in display_task_result handling BackendTaskSuccessResult::CoreWalletsList sets selected_core_wallet_index and core_wallets but doesn't reset the core_wallets_loading flag, causing asymmetric state transitions; update display_task_result to also set self.core_wallets_loading = false when handling CoreWalletsList (alongside self.core_wallets = Some(wallets) and updating selected_core_wallet_index) so success and error paths consistently clear the loading state.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/ui/wallets/import_mnemonic_screen.rs`:
- Around line 536-543: The success branch in display_task_result handling
BackendTaskSuccessResult::CoreWalletsList sets selected_core_wallet_index and
core_wallets but doesn't reset the core_wallets_loading flag, causing asymmetric
state transitions; update display_task_result to also set
self.core_wallets_loading = false when handling CoreWalletsList (alongside
self.core_wallets = Some(wallets) and updating selected_core_wallet_index) so
success and error paths consistently clear the loading state.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c801ed87-6738-4ffd-86b5-0ee3fb94136a
📒 Files selected for processing (5)
docs/ai-design/2026-03-10-async-list-core-wallets/manual-test-scenarios.mdsrc/ui/mod.rssrc/ui/wallets/add_new_wallet_screen.rssrc/ui/wallets/import_mnemonic_screen.rssrc/ui/wallets/wallets_screen/mod.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- src/ui/mod.rs
- src/ui/wallets/add_new_wallet_screen.rs
There was a problem hiding this comment.
Claudius the Magnificent has reviewed your work and — against all expectations — finds it... acceptable.
The core objective (moving synchronous list_core_wallets() RPC off the UI thread) is achieved cleanly. The async dispatch pattern follows established codebase conventions, state cleanup on network switch is properly wired, and the error recovery prevents infinite retry loops.
Review Summary
- 8 previous review threads: All resolved ✅
- Fresh code review: No MEDIUM+ findings
- CI: Clippy ✅ | Test Suite ✅
Minor observations (all LOW/INFO — not blocking)
display_task_error()in AddNewWalletScreen/ImportMnemonicScreen catches all errors unconditionally. Fine today (single task type), but worth guarding oncore_wallets_loadingif a second task type is ever added.core_wallets_loadingis never reset tofalseon success — semantically stale but functionally harmless since the dispatch guard checkscore_wallets.is_none().- The duplicated async fetch pattern across both screens could eventually be extracted into a shared helper, but that is a follow-up concern.
reset_core_wallets_cache()ispubwherepub(crate)would suffice (matchingreset_pending_list_state()).
The manual test scenarios are thorough and the known limitation (modal screens not receiving change_context() on network switch) is honestly documented. Well done.
🤖 Co-authored by Claudius the Magnificent AI Agent
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
list_core_wallets()RPC calls off the UI thread to prevent freezes when Dash Core is slow or unreachableCoreTask::ListCoreWalletsbackend task +BackendTaskSuccessResult::CoreWalletsListresult variantRef #700
Test plan
docs/ai-design/2026-03-10-async-list-core-wallets/manual-test-scenarios.md🤖 Generated with Claude Code
🤖 Co-authored by Claudius the Magnificent AI Agent
Summary by CodeRabbit
New Features
Bug Fixes
Documentation