feat(ui): redesign wallet screen information architecture

[lklimek]

Summary

Redesigns the wallet screen layout from a vertically-stacked ComboBox-driven architecture to a tab-based information architecture with progressive disclosure.

User Story

Imagine you are a Dash user opening the Wallets screen. Instead of a dropdown to switch between account types, you now see clear tabs — Dash Core, Platform, Shielded — letting you instantly navigate between your different balance types. Your transaction history is neatly collapsible, and if you're a developer, a Dev Tools button gives you quick access to test faucets, mining, and refresh controls without cluttering the main interface.

Changes

• Account tabs replace ComboBox dropdown — one tab per account type (Dash Core, Platform, Shielded; dev mode adds Identity, CoinJoin, Provider, Legacy)
• "Main Account" renamed to "Dash Core", "Platform Account" to "Platform"
• Balance breakdown is now collapsible (collapsed by default in normal mode, expanded in dev mode)
• Dev Tools expandable button consolidates Get Test Dash (opens faucet URL), Mine, and Refresh Mode selector
• Transaction history is collapsible and visible in all modes (was dev-only); Fee column shown only in dev mode
• Asset Locks restricted to Dash Core tab only
• Shielded moved from top-level tab to account tab
• Progressive disclosure: default mode shows 3 tabs, dev mode reveals all account types with addresses
• No theme changes — colors, fonts, spacing untouched

Files Changed

• src/ui/wallets/account_summary.rs — renamed labels, added tab_label(), is_visible_in_default_mode()
• src/ui/wallets/wallets_screen/mod.rs — restructured rendering flow, tab system, collapsible sections

Test plan

• Open wallet screen — verify tab strip shows Dash Core | Platform | Shielded
• Switch between tabs — verify correct addresses display per tab
• Toggle developer mode — verify additional account tabs appear/disappear
• Collapse/expand balance breakdown in both modes
• Click Dev Tools — verify Get Test Dash, Mine, Refresh Mode appear
• Collapse/expand transaction history
• Verify asset locks only appear on Dash Core tab
• Verify Send, Receive, Refresh buttons work from all tabs
• Test with single-key wallet — should be unchanged

_{🤖 Co-authored by Claudius the Magnificent AI Agent}

Summary by CodeRabbit

• New Features

  • Shielded wallet auto-initializes on unlock and syncs in background; shielded addresses table shows all diversified addresses with per-row actions.
  • Account tabs redesigned into Dash Core / Platform / Shielded with consolidated balances and balance breakdown.

• Bug Fixes

  • Prevented duplicate shielded notes during persistence.
  • RPC errors now show actual error text and clear on success.

• Improvements

  • Clearer address validation messages and unified truncation display.
  • Address input resets now also clear validated destinations.
  • Network password input always updates (falls back to default).

• Tests

  • Added end-to-end test asserting transaction "is_ours" flag behavior.

• Documentation

  • Added new Wallet Management user stories.

[coderabbitai]

Actionable comments posted: 5

♻️ Duplicate comments (4)

src/model/address.rs (1)

103-105: ⚠️ Potential issue | 🟠 Major

Add 9 to the Core-prefix whitelist.

Dash testnet P2SH addresses can also start with 9. Those inputs currently go through the identity-first branch, so this disambiguation is still wrong for one valid Core prefix.

Prefix fix

         let core_prefix = matches!(
             trimmed.as_bytes().first(),
-            Some(b'X' | b'Y' | b'y' | b'8' | b'7')
+            Some(b'X' | b'Y' | b'y' | b'8' | b'9' | b'7')
         );

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/model/address.rs` around lines 103 - 105, The core_prefix check in
address parsing (the matches! on trimmed.as_bytes().first() used to set
core_prefix) omits '9', causing valid Dash testnet P2SH addresses to be
misclassified; update that matches! pattern to include b'9' alongside
b'X'|b'Y'|b'y'|b'8'|b'7' so trimmed (and the core_prefix boolean) correctly
recognizes '9' as a Core prefix in the logic that governs identity-first vs core
address parsing.

src/ui/wallets/wallets_screen/mod.rs (3)

1798-1818: ⚠️ Potential issue | 🟡 Minor

Refresh mode labels still don't match the dispatched work.

create_refresh_action_for_mode() always appends shielded sync, so “Core Only” and “Platform Only” still start extra work. The same global self.refreshing flag then paints the Platform row as syncing even when only Core/shielded work is running, so the UI still overstates what is being refreshed.

Also applies to: 2121-2159

🤖 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 1798 - 1818, The UI shows
platform syncing whenever the global self.refreshing is true because
create_refresh_action_for_mode() always appends shielded/platform work; update
create_refresh_action_for_mode() to only include shielded/platform steps when
the selected RefreshMode requires them (do not unconditionally append shielded
sync), and introduce per-target flags (e.g., self.refreshing_core,
self.refreshing_platform, self.refreshing_shielded) or derive booleans from the
dispatched Action to represent which subsystems are actually running; then
change the platform row rendering (the addr_color/selecting spinner logic in
wallets_screen mod.rs and the analogous code at lines ~2121-2159) to consult the
platform-specific flag (e.g., self.refreshing_platform) instead of the global
self.refreshing so the Platform row only shows syncing when platform work is
dispatched.

---

1084-1137: ⚠️ Potential issue | 🟡 Minor

Dev Tools are still always expanded in developer mode.

This block renders Get Test Dash, Mine, and the refresh-mode control inline whenever developer mode is enabled. The PR objective/test plan calls for those controls to live behind an expandable Dev Tools affordance, so the progressive-disclosure behavior is still missing.

🤖 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 1084 - 1137, The Dev Tools
controls are always shown when is_developer_mode() is true; make them hidden
behind an expandable affordance by adding a toggled panel (e.g., a boolean field
like self.dev_tools_expanded) or using egui's collapsing header and render the
existing allocate_ui_with_layout block only when that expanded flag is true;
keep the header/button (label "Dev Tools" or similar) visible when
self.app_context.is_developer_mode() and toggle self.dev_tools_expanded on
click, then move the Get Test Dash, Mine (open_mine_dialog()) and Refresh mode
(self.refresh_mode.next()) UI code inside the expanded conditional so they are
only shown when the Dev Tools panel is opened.

---

1719-1786: ⚠️ Potential issue | 🟠 Major

Use ConnectionStatus for the SPV health row.

This branch still reads spv_manager().status() directly for SPV state, peers, and phase text. That can drift from the rest of the app’s health reporting and breaks the single-source-of-truth contract for connection status. As per coding guidelines, "For connection health (status, peer counts, errors, overall state), always read from ConnectionStatus, not directly from SpvManager or other subsystems. ConnectionStatus is the single source of truth."

🤖 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 1719 - 1786, In the
CoreBackendMode::Spv branch, don't call self.app_context.spv_manager().status()
directly; instead read the SPV health (state, peer count, sync progress/phase,
error) from the single source-of-truth ConnectionStatus (via
self.app_context.connection_status()) and use those values to drive the UI
labels and spinner logic; replace uses of snapshot, snapshot.status,
snapshot.connected_peers, and snapshot.sync_progress with the corresponding
properties on ConnectionStatus, still using spv_phase_summary to format any
progress phase text, and keep the same UI branches (Idle/Stopped, Starting,
Syncing, Running, Stopping, Error) and colors.

🧹 Nitpick comments (1)

src/model/address.rs (1)

353-396: Prefer fixed fixtures over randomness in these unit tests.

Both tests depend on Identifier::random(), and the second one can pass without ever exercising its assertion path. Fixed Base58 fixtures would make CI deterministic and cover these Core-prefix branches directly.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/model/address.rs` around lines 353 - 396, Replace the randomized
identifier generation in the tests detect_identity_base58 and
detect_identity_with_core_prefix_still_works_when_not_valid_core with fixed
Base58 fixtures: prepare a small Vec<&str> of hard-coded Base58 identifier
strings that cover (1) identifiers without Core prefixes (for
detect_identity_base58) and (2) identifiers that start with Core prefix
characters both valid and invalid as Core addresses (for
detect_identity_with_core_prefix_still_works_when_not_valid_core); then iterate
those fixtures instead of calling Identifier::random(), call
AddressKind::detect(&id_str) and use id_str.parse::<Address<NetworkUnchecked>>()
to classify the Core-parse-fail case and assert the expected
AddressKind::Identity outcomes. Ensure you update the test names/comments to
reflect deterministic fixtures and keep references to Identifier::random removed
so CI is deterministic.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/backend_task/shielded/sync.rs`:
- Around line 126-136: The current branch that detects a decrypted-value
mismatch for an existing dn.position only logs a warning and continues, leaving
stale data in existing_notes and allowing shielded_state.recalculate_balance()
to use wrong values; change this to treat divergence as a sync failure or
actively replace and persist the note. Specifically, in the block handling
existing_notes.get(&dn.position) where you compare let new_value =
dn.note.value().inner(), either return an error (e.g.,
Err(SyncError::NoteDivergence { position: dn.position, expected: existing_value,
found: new_value })) to abort sync, or update the persisted store and the
in-memory map: overwrite the DB entry for dn.position with dn.note, update
existing_notes.insert(dn.position, new_value), and ensure any in-memory
shielded_state structures that feed recalculate_balance() are updated before
calling shielded_state.recalculate_balance(). Ensure the chosen approach is
applied where existing_notes, dn.position, dn.note.value(), and
shielded_state.recalculate_balance() are referenced.

In `@src/model/address.rs`:
- Around line 243-245: The truncation check in the function that builds the
shortened address uses min_useful = prefix_len + suffix_len + 3 and only skips
truncation when addr.len() < min_useful, which still truncates when addr.len()
== min_useful and buys no space; change the condition to if addr.len() <=
min_useful (or otherwise use <= when comparing addr.len() against min_useful) so
that when the formatted result would be exactly the same length as the original
the function returns addr.to_string() unchanged (update the check around
min_useful/prefix_len/suffix_len in the same function).
- Around line 15-18: The current code in is_platform_address_string() (called by
AddressKind::detect()) slices s using s[..hrp.len()] which can panic on
arbitrary UTF-8 input; change to use boundary-checked access: call
s.get(..hrp.len()) and if it returns Some(prefix) perform
prefix.eq_ignore_ascii_case(hrp), and replace the raw index access
s.as_bytes()[hrp.len()] with s.as_bytes().get(hrp.len()) and compare the
Option<u8> to b'1' (or check Some(&b'1')). This ensures no slicing or byte
indexing panics for invalid UTF-8/short strings while preserving the same logic
in is_platform_address_string().

In `@src/ui/wallets/wallets_screen/mod.rs`:
- Around line 1145-1166: build_account_tabs currently appends every summary
whose category is "visible" which in normal mode produces multiple Bip44 or
Platform tabs; change the logic so when developer_mode is false you only add a
single tab per primary category (e.g., one AccountCategory::Bip44 (prefer index
0), one Platform/PlatformPayment, one Shielded) by tracking which primary
categories have already been added (a small HashSet or bools) before pushing
AccountTab::Category(summary.category, summary.index); keep existing behavior
when developer_mode is true; still ensure the Bip44 index 0 tab exists by
inserting AccountTab::Category(AccountCategory::Bip44, Some(0)) if not present.
- Around line 1826-1835: The code currently collapses a poisoned mutex into None
via .lock().ok()? which makes a lock failure indistinguishable from "no shielded
state"; update the shielded_states lock handling in the closure that computes
shielded_info (and the analogous block around lines 2100-2108) to explicitly
detect PoisonError and propagate or surface it instead of returning None:
replace .lock().ok()? with a match (or map_err/and_then) on
self.app_context.shielded_states.lock() so that on Err(poison) you either return
an Err variant or set a distinct error/status flag (and log the poison using
processLogger/app_context logger) so the UI shows an error state rather than
"never synced"; keep the remainder that reads state.get(&hash) unchanged.

---

Duplicate comments:
In `@src/model/address.rs`:
- Around line 103-105: The core_prefix check in address parsing (the matches! on
trimmed.as_bytes().first() used to set core_prefix) omits '9', causing valid
Dash testnet P2SH addresses to be misclassified; update that matches! pattern to
include b'9' alongside b'X'|b'Y'|b'y'|b'8'|b'7' so trimmed (and the core_prefix
boolean) correctly recognizes '9' as a Core prefix in the logic that governs
identity-first vs core address parsing.

In `@src/ui/wallets/wallets_screen/mod.rs`:
- Around line 1798-1818: The UI shows platform syncing whenever the global
self.refreshing is true because create_refresh_action_for_mode() always appends
shielded/platform work; update create_refresh_action_for_mode() to only include
shielded/platform steps when the selected RefreshMode requires them (do not
unconditionally append shielded sync), and introduce per-target flags (e.g.,
self.refreshing_core, self.refreshing_platform, self.refreshing_shielded) or
derive booleans from the dispatched Action to represent which subsystems are
actually running; then change the platform row rendering (the
addr_color/selecting spinner logic in wallets_screen mod.rs and the analogous
code at lines ~2121-2159) to consult the platform-specific flag (e.g.,
self.refreshing_platform) instead of the global self.refreshing so the Platform
row only shows syncing when platform work is dispatched.
- Around line 1084-1137: The Dev Tools controls are always shown when
is_developer_mode() is true; make them hidden behind an expandable affordance by
adding a toggled panel (e.g., a boolean field like self.dev_tools_expanded) or
using egui's collapsing header and render the existing allocate_ui_with_layout
block only when that expanded flag is true; keep the header/button (label "Dev
Tools" or similar) visible when self.app_context.is_developer_mode() and toggle
self.dev_tools_expanded on click, then move the Get Test Dash, Mine
(open_mine_dialog()) and Refresh mode (self.refresh_mode.next()) UI code inside
the expanded conditional so they are only shown when the Dev Tools panel is
opened.
- Around line 1719-1786: In the CoreBackendMode::Spv branch, don't call
self.app_context.spv_manager().status() directly; instead read the SPV health
(state, peer count, sync progress/phase, error) from the single source-of-truth
ConnectionStatus (via self.app_context.connection_status()) and use those values
to drive the UI labels and spinner logic; replace uses of snapshot,
snapshot.status, snapshot.connected_peers, and snapshot.sync_progress with the
corresponding properties on ConnectionStatus, still using spv_phase_summary to
format any progress phase text, and keep the same UI branches (Idle/Stopped,
Starting, Syncing, Running, Stopping, Error) and colors.

---

Nitpick comments:
In `@src/model/address.rs`:
- Around line 353-396: Replace the randomized identifier generation in the tests
detect_identity_base58 and
detect_identity_with_core_prefix_still_works_when_not_valid_core with fixed
Base58 fixtures: prepare a small Vec<&str> of hard-coded Base58 identifier
strings that cover (1) identifiers without Core prefixes (for
detect_identity_base58) and (2) identifiers that start with Core prefix
characters both valid and invalid as Core addresses (for
detect_identity_with_core_prefix_still_works_when_not_valid_core); then iterate
those fixtures instead of calling Identifier::random(), call
AddressKind::detect(&id_str) and use id_str.parse::<Address<NetworkUnchecked>>()
to classify the Core-parse-fail case and assert the expected
AddressKind::Identity outcomes. Ensure you update the test names/comments to
reflect deterministic fixtures and keep references to Identifier::random removed
so CI is deterministic.

🪄 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: 1032f4c4-c3f9-4314-b00e-98072f5ebf21

📥 Commits

Reviewing files that changed from the base of the PR and between 3c89c3a and 8bab96b.

📒 Files selected for processing (8)

• src/backend_task/shielded/sync.rs
• src/context/wallet_lifecycle.rs
• src/model/address.rs
• src/ui/components/address_input.rs
• src/ui/wallets/account_summary.rs
• src/ui/wallets/shielded_tab.rs
• src/ui/wallets/wallets_screen/mod.rs
• tests/backend-e2e/tx_is_ours.rs

✅ Files skipped from review due to trivial changes (1)

• tests/backend-e2e/tx_is_ours.rs

🚧 Files skipped from review as they are similar to previous changes (4)

• src/context/wallet_lifecycle.rs
• src/ui/components/address_input.rs
• src/ui/wallets/account_summary.rs
• src/ui/wallets/shielded_tab.rs

[coderabbitai]

Actionable comments posted: 8

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)

src/ui/components/address_input.rs (2)

219-223: ⚠️ Potential issue | 🟡 Minor

Make with_exclude_change() order-independent.

with_wallets() eagerly fills all_entries, so calling with_exclude_change(true) later is a silent no-op. That makes the new flag easy to miss in builder chains and leaves change addresses visible in the very flow this option is meant to fix.

♻️ Minimal fix

 pub fn with_exclude_change(mut self, exclude: bool) -> Self {
     self.exclude_change = exclude;
+    if exclude {
+        self.all_entries
+            .retain(|e| e.address_kind != AddressKind::Core || !e.is_change);
+    }
     self
 }

Also applies to: 261-264

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/ui/components/address_input.rs` around lines 219 - 223, with_wallets
currently populates all_entries eagerly which makes calling with_exclude_change
afterwards a no-op; fix by making with_exclude_change order-independent: (1)
have with_wallets consult the existing self.exclude_change flag when calling
extract_wallet_entries and skip adding change addresses if true, and (2) have
with_exclude_change set the exclude flag and also filter/remove any
already-populated entries in self.all_entries (use the same change-detection
logic as extract_wallet_entries) so toggling the flag after with_wallets updates
existing state; reference methods: with_wallets, with_exclude_change,
extract_wallet_entries, and the all_entries collection.

---

625-633: ⚠️ Potential issue | 🟡 Minor

Normalize case before the prefix check in validate_shielded().

validate_shielded() uses a case-sensitive starts_with(expected_prefix) check before parsing, so an all-uppercase shielded address on the right network gets rejected with "different network." The BIP-350 bech32m spec allows all-uppercase addresses. Follow the pattern in validate_platform(): check for valid case (all-upper or all-lower, not mixed), normalize to lowercase, apply the prefix check to the canonical form, and pass the canonical string to OrchardAddress::from_bech32m_string().

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/ui/components/address_input.rs` around lines 625 - 633, In
validate_shielded(), update the prefix check to operate on a canonical lowercase
form like validate_platform() does: first verify the address case is valid
(either all-lower or all-upper, reject mixed-case), then normalize the input to
lowercase, perform the starts_with(expected_prefix) check against that canonical
lowercase string, and pass the canonical string to
OrchardAddress::from_bech32m_string(); ensure error messages remain the same but
are emitted after the canonicalization and prefix check.

♻️ Duplicate comments (1)

src/ui/components/address_input.rs (1)

383-398: ⚠️ Potential issue | 🟠 Major

Still unresolved: default autocomplete scales with every derived address.

This still materializes known_addresses into all_entries, while filtered_entries() scans and sorts the whole list on each focused frame. Including zero-balance derived addresses by default makes opening the dropdown progressively slower on larger wallets.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/ui/components/address_input.rs` around lines 383 - 398, The UI is
materializing all derived addresses into all_entries and filtered_entries()
rescans/sorts the full set each focus, causing slow autocomplete; modify the
logic that builds the dropdown so it does not iterate/collect every entry from
known_addresses but instead applies filters incrementally: skip system addresses
(system_addresses), skip change addresses when self.exclude_change and
derivation_path.is_bip44_change(self.network) is true, and skip zero-balance
addresses by default unless with_balance_range(1..) is explicitly requested (use
address_balances.get(address).copied().unwrap_or(0) to check balance); ensure
filtered_entries() no longer sorts/scans the entire known_addresses on each
focus but uses the already-filtered iterator or caches the filtered list to
avoid per-frame full traversal.

🤖 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/user-stories.md`:
- Around line 217-224: Update the WAL-024 user story text to reflect the
implemented UI: state that the "Balance breakdown" section (not the
address/asset-lock sections) is collapsible and is expanded by default only when
developer mode is enabled, while the "Address table" and "Asset locks" sections
each have their own collapsible headers and are expanded by default for users;
adjust the acceptance checklist and wording under the WAL-024 header so QA has
the correct criteria and examples for testing.

In `@src/context/wallet_lifecycle.rs`:
- Around line 161-166: register_wallet() currently only triggers shielded setup
via handle_wallet_unlocked() in the SPV branch, so newly-registered RPC wallets
skip shielded init; refactor by extracting shielded initialization into a
standalone call and ensure queue_shielded_init_and_sync(seed_hash) (or a new
helper like initialize_shielded_for_wallet(seed_hash)) is invoked for every
successful wallet registration path (both SPV and RPC) instead of being tied
only to handle_wallet_unlocked(); remove or consolidate any duplicate calls so
shielded init runs exactly once per registration.
- Around line 181-245: The unlock-time shielded init/sync
(queue_shielded_init_and_sync → run_shielded_sync →
sync_shielded_notes/check_nullifiers_task) can race with user-triggered
refreshes and remove state from the Mutex, causing TaskError::WalletNotFound;
serialize these by adding a per-seed lock (e.g., a shared
Arc<Mutex<HashMap<WalletSeedHash, Arc<Mutex<()>>>>) and acquire the
seed-specific mutex at the start of queue_shielded_init_and_sync (and likewise
when handling UI-triggered SyncNotes/CheckNullifiers) so both paths hold the
same per-seed guard while calling initialize_shielded_wallet,
sync_shielded_notes, and check_nullifiers_task; alternatively, dispatch the
unlock-time sync through the existing BackendTask queue used by UI refreshes
(use the same task type/queue) so both flows are serialized.

In `@src/ui/components/address_input.rs`:
- Around line 370-381: The AddressInput component currently unconditionally
builds system_addresses by filtering guard.watched_addresses via
AccountCategory::from_reference and hides non-Bip44/PlatformPayment addresses
everywhere; make this filtering opt-in by adding a private builder-config flag
(e.g., hide_system_addresses: bool) to AddressInput's private fields with a
builder method (e.g., .hide_system_addresses(bool)) and only apply the
AccountCategory-based filtering when that flag is true; ensure the component
follows the pattern by keeping fields private, exposing builder methods for
configuration, and keeping/adding a Response struct that implements
ComponentResponse so callers can opt into the send-autocomplete behavior (caller
screens should pass developer_mode or set the builder flag as needed).

In `@src/ui/wallets/wallets_screen/mod.rs`:
- Around line 1059-1067: The current shielded_balance_duffs collapses a poisoned
mutex into a zero balance; change its signature from fn
shielded_balance_duffs(&self, seed_hash: &WalletSeedHash) -> u64 to ->
Option<u64> and stop using lock().ok(). Instead call
self.app_context.shielded_states.lock() and match the Result: on Ok(states)
return states.get(seed_hash).map(|s| s.shielded_balance / CREDITS_PER_DUFF), and
on Err(poison_err) log the error (including poison_err) and return None so
callers can render an error state instead of silently showing 0; update callers
of shielded_balance_duffs to handle None (e.g., show error/placeholder or skip
badge).
- Around line 1172-1204: build_account_tabs currently backfills Dash Core
(AccountCategory::Bip44) but not Platform, causing wallets without a
PlatformPayment summary to lack the Platform tab and preventing
render_bottom_options from being invoked to create the first Platform address;
fix by adding the same backfill logic used for Dash Core to ensure an
AccountTab::Category(AccountCategory::Platform, Some(0)) is inserted (e.g.,
right after the Bip44 insertion) when no Platform summary exists, keep Shielded
and System insertion logic unchanged, and remove/avoid any early-return or
empty-tab short-circuit in the caller so render_bottom_options is still called
when only the backfilled Platform tab exists (refer to build_account_tabs,
AccountTab::Category, AccountCategory::Platform, and render_bottom_options to
locate the spots to change).
- Around line 161-162: The generating_core_address flag (field
generating_core_address in wallets_screen) must be reset when the Wallets screen
regains focus so a stale in-flight task doesn't keep the "+ New Receive Address"
button disabled; add a focus handler (or extend the existing on_focus /
set_visible / on_show logic for the Wallets screen) to set
self.generating_core_address = false when the screen becomes visible, and ensure
this ties into the main update() routing (display_task_result()) behavior so any
pending TaskResult routed elsewhere won't leave the flag set when returning to
the Wallets screen.
- Around line 2226-2238: The shielded_tab_view.tick() is still called when
switching from an HD wallet to a single-key wallet, draining pending_task for
the old HD seed; update the guard so we only call tick() when the shielded view
corresponds to the currently-selected account (not just when
selected_account_tab != AccountTab::Shielded). Concretely, before calling
self.shielded_tab_view.as_mut().map(|v| v.tick()), check that the
shielded_tab_view’s associated seed/seed_hash or account identifier matches the
currently selected account (or that the selected account is an HD/seed-backed
account); if it doesn’t match, return AppAction::None. Use existing identifiers
like shielded_tab_view, tick(), selected_account_tab and the shielded view’s
seed/seed_hash or account id to implement the extra guard.

---

Outside diff comments:
In `@src/ui/components/address_input.rs`:
- Around line 219-223: with_wallets currently populates all_entries eagerly
which makes calling with_exclude_change afterwards a no-op; fix by making
with_exclude_change order-independent: (1) have with_wallets consult the
existing self.exclude_change flag when calling extract_wallet_entries and skip
adding change addresses if true, and (2) have with_exclude_change set the
exclude flag and also filter/remove any already-populated entries in
self.all_entries (use the same change-detection logic as extract_wallet_entries)
so toggling the flag after with_wallets updates existing state; reference
methods: with_wallets, with_exclude_change, extract_wallet_entries, and the
all_entries collection.
- Around line 625-633: In validate_shielded(), update the prefix check to
operate on a canonical lowercase form like validate_platform() does: first
verify the address case is valid (either all-lower or all-upper, reject
mixed-case), then normalize the input to lowercase, perform the
starts_with(expected_prefix) check against that canonical lowercase string, and
pass the canonical string to OrchardAddress::from_bech32m_string(); ensure error
messages remain the same but are emitted after the canonicalization and prefix
check.

---

Duplicate comments:
In `@src/ui/components/address_input.rs`:
- Around line 383-398: The UI is materializing all derived addresses into
all_entries and filtered_entries() rescans/sorts the full set each focus,
causing slow autocomplete; modify the logic that builds the dropdown so it does
not iterate/collect every entry from known_addresses but instead applies filters
incrementally: skip system addresses (system_addresses), skip change addresses
when self.exclude_change and derivation_path.is_bip44_change(self.network) is
true, and skip zero-balance addresses by default unless with_balance_range(1..)
is explicitly requested (use address_balances.get(address).copied().unwrap_or(0)
to check balance); ensure filtered_entries() no longer sorts/scans the entire
known_addresses on each focus but uses the already-filtered iterator or caches
the filtered list to avoid per-frame full traversal.

🪄 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: f183cbe1-ff64-4118-b78f-a7e512d9f8a0

📥 Commits

Reviewing files that changed from the base of the PR and between 8bab96b and 4889de7.

📒 Files selected for processing (6)

• docs/user-stories.md
• src/context/wallet_lifecycle.rs
• src/ui/components/address_input.rs
• src/ui/wallets/send_screen.rs
• src/ui/wallets/wallets_screen/address_table.rs
• src/ui/wallets/wallets_screen/mod.rs

🚧 Files skipped from review as they are similar to previous changes (1)

• src/ui/wallets/send_screen.rs

[coderabbitai]

Actionable comments posted: 1

♻️ Duplicate comments (1)

src/ui/components/address_input.rs (1)

370-381: ⚠️ Potential issue | 🟠 Major

Keep system-address filtering opt-in in this shared component.

Line 378 is reusing the wallet screen’s default-mode visibility split, and Lines 390-397 then drop those addresses unconditionally. That means dev-only/tooling addresses never reach autocomplete anywhere AddressInput loads wallets, because developer_mode is not consulted in this path. Please move this behind a caller-controlled builder flag, or let callers pass the allowed categories, instead of hard-coding is_system_category() here.

As per coding guidelines, src/ui/components/**/*.rs: "Provide private fields only, builder methods for configuration, Response struct with ComponentResponse trait."

Also applies to: 390-397

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/ui/components/address_input.rs` around lines 370 - 381, The AddressInput
component is unconditionally filtering out system addresses by using
AccountCategory::from_reference(info.path_reference).is_system_category() on
guard.watched_addresses; make this behavior opt-in by adding a private
configuration field on the AddressInput builder (e.g., allowed_categories or
allow_system_addresses boolean, defaulting to current safe behavior) and a
builder method to set it; then change the logic that builds system_addresses
(the map/filter that uses AccountCategory::from_reference and the subsequent
drop logic at the later block) to consult that new field—only exclude
system-category addresses when the builder flag indicates they should be
filtered, otherwise include them so callers can control visibility.

🧹 Nitpick comments (2)

src/backend_task/error.rs (1)

1019-1025: Add regression tests for the new nonce classification branch.

The new AddressInvalidNonceError mapping has no dedicated test coverage. Please add tests for:

1. shielded_broadcast_error() → TaskError::ShieldedNonceMismatch, and
2. display text includes actionable retry guidance.

This keeps the new behavior from silently regressing.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/backend_task/error.rs` around lines 1019 - 1025, Add unit tests that
cover the new nonce-classification branch: write tests that call
shielded_broadcast_error() (or the public wrapper that produces the TaskError)
with a simulated
ConsensusError::StateError(StateError::AddressInvalidNonceError(...)) and assert
it returns TaskError::ShieldedNonceMismatch (checking the variant). Also add an
assertion that the Display (or to_string()) of TaskError::ShieldedNonceMismatch
contains actionable retry guidance text (e.g., mentions "retry" or "increment
nonce"); use the same constructors/types (ConsensusError::StateError,
StateError::AddressInvalidNonceError, TaskError::ShieldedNonceMismatch) to build
the inputs and verify both variant and user-facing message to prevent
regressions.

src/ui/components/address_input.rs (1)

400-405: Build the dropdown label with one localizable format.

Line 400 appends " (change)", and Line 885 then wraps the finished text again with "{} ({})". Composing user-facing labels from suffix fragments makes them hard to translate or reorder cleanly. Please funnel the final row text through a single formatter/helper instead.

As per coding guidelines, **/*.rs: "All user-facing strings (labels, messages, tooltips, errors) must be simple, complete sentences. Avoid concatenating fragments, positional assumptions, or grammar that breaks in other languages. Each string should be extractable as a single translation unit with named placeholders for dynamic values and no logic in the text itself."

Also applies to: 885-887

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/ui/components/address_input.rs` around lines 400 - 405, The code
currently builds the dropdown label by concatenating fragments (change_suffix +
truncated/full address) in multiple places (see change_suffix, display,
truncate_address, and later wrapping with "{} ({})"); replace these fragment
concatenations with a single helper/formatter (e.g., format_address_label) that
takes named parameters like prefix, address, and is_change (or full_addresses
flag) and returns one fully formatted, localizable string; update both the
current display construction and the later wrapping (the code that uses "{}
({})") to call this helper so the entire user-facing label is produced by one
localization call with named placeholders.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/backend_task/error.rs`:
- Around line 833-841: The ShieldedNonceMismatch error claims automatic retry
but with_anchor_retry only retries on ShieldedAnchorMismatch; update
with_anchor_retry to also handle ShieldedNonceMismatch by invoking the
wallet/nonce refresh path (the same refresh used for AddressInvalidNonceError
handling) and performing a single retry of the broadcast attempt before
returning the error; ensure you reference ShieldedNonceMismatch in the match
arm, call whatever nonce-refresh/rehydration routine used elsewhere (or reuse
shielded_broadcast_error mapping logic) to update the cached nonce, then retry
the operation once and return the eventual result or the error if it still
fails.

---

Duplicate comments:
In `@src/ui/components/address_input.rs`:
- Around line 370-381: The AddressInput component is unconditionally filtering
out system addresses by using
AccountCategory::from_reference(info.path_reference).is_system_category() on
guard.watched_addresses; make this behavior opt-in by adding a private
configuration field on the AddressInput builder (e.g., allowed_categories or
allow_system_addresses boolean, defaulting to current safe behavior) and a
builder method to set it; then change the logic that builds system_addresses
(the map/filter that uses AccountCategory::from_reference and the subsequent
drop logic at the later block) to consult that new field—only exclude
system-category addresses when the builder flag indicates they should be
filtered, otherwise include them so callers can control visibility.

---

Nitpick comments:
In `@src/backend_task/error.rs`:
- Around line 1019-1025: Add unit tests that cover the new nonce-classification
branch: write tests that call shielded_broadcast_error() (or the public wrapper
that produces the TaskError) with a simulated
ConsensusError::StateError(StateError::AddressInvalidNonceError(...)) and assert
it returns TaskError::ShieldedNonceMismatch (checking the variant). Also add an
assertion that the Display (or to_string()) of TaskError::ShieldedNonceMismatch
contains actionable retry guidance text (e.g., mentions "retry" or "increment
nonce"); use the same constructors/types (ConsensusError::StateError,
StateError::AddressInvalidNonceError, TaskError::ShieldedNonceMismatch) to build
the inputs and verify both variant and user-facing message to prevent
regressions.

In `@src/ui/components/address_input.rs`:
- Around line 400-405: The code currently builds the dropdown label by
concatenating fragments (change_suffix + truncated/full address) in multiple
places (see change_suffix, display, truncate_address, and later wrapping with
"{} ({})"); replace these fragment concatenations with a single helper/formatter
(e.g., format_address_label) that takes named parameters like prefix, address,
and is_change (or full_addresses flag) and returns one fully formatted,
localizable string; update both the current display construction and the later
wrapping (the code that uses "{} ({})") to call this helper so the entire
user-facing label is produced by one localization call with named placeholders.

🪄 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: e90e7f38-07f5-47ab-9d01-86fac0cb8ff6

📥 Commits

Reviewing files that changed from the base of the PR and between 4889de7 and fabcb73.

📒 Files selected for processing (2)

• src/backend_task/error.rs
• src/ui/components/address_input.rs

[Copilot]

Pull request overview

Copilot reviewed 23 out of 23 changed files in this pull request and generated 6 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.