feat(ui): unified AddressInput component with autocomplete

[lklimek]

Summary

• New AddressInput component replacing scattered address handling across screens
• Supports all 4 address types (Core, Platform, Shielded, Identity) with auto-detection
• Autocomplete dropdown from wallet data with balance display per address
• Optional type filter dropdown and balance range filtering
• Selection-only mode for source address picking
• Integrated into 3 proof-of-concept sites: send screen (simple mode destination), send screen (advanced mode types), unshield credits screen

Details

New files

• src/model/address.rs — AddressKind enum and ValidatedAddress domain enum
• src/ui/components/address_input.rs — Full component (~1200 lines) with builder API, Component/ComponentResponse traits

Migrated screens

• UnshieldCreditsScreen — full migration, removed local Destination enum
• WalletSendScreen simple mode — full migration, removed inline AddressType/detect_address_type()/is_shielded_address()
• WalletSendScreen advanced mode — minimal migration (types updated, source inputs unchanged)

Component features

• Address type detection (Shielded > Platform > Core > Identity priority)
• Network-aware validation with user-friendly error messages
• Eager wallet data extraction (no per-frame lock acquisition)
• Keyboard navigation (Arrow Up/Down, Enter, Escape)
• Developer mode (shows credits alongside DASH)
• 33 unit tests

Test plan

• Verify cargo build succeeds
• Verify cargo clippy --all-features --all-targets -- -D warnings passes
• Verify all 33 address/component unit tests pass
• Manual: send screen — paste Core address, verify detection and validation
• Manual: send screen — paste Platform address, verify detection
• Manual: send screen — paste Shielded address, verify detection
• Manual: send screen — type 3+ chars, verify autocomplete appears with balances
• Manual: send screen — select from autocomplete, verify field populates
• Manual: send screen — enter wrong-network address, verify error on blur
• Manual: unshield screen — enter Platform address, verify unshield flow
• Manual: unshield screen — enter Core address, verify withdrawal flow
• Manual: unshield screen — enter invalid address, verify error

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

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

🗂️ Base branches to auto review (2)

• master
• v1.0-dev

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 5ba70891-3662-4624-83cd-529b4ad45dc9

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch feat/unified-address-input

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[Copilot]

Pull request overview

This PR introduces a unified AddressInput UI component and accompanying domain types to consolidate address detection/validation and provide wallet-backed autocomplete, then migrates the send and unshield flows to use it.

Changes:

• Added AddressKind/ValidatedAddress domain types for representing detected + validated addresses.
• Added a new AddressInput component (builder-style API) with validation, autocomplete, and optional filtering.
• Migrated WalletSendScreen (simple mode) and UnshieldCreditsScreen to use AddressInput instead of ad-hoc parsing/detection.

Reviewed changes

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

Show a summary per file

File	Description
src/model/address.rs	New domain enums for address classification and validated typed payloads.
src/model/mod.rs	Exposes the new model::address module.
src/ui/components/address_input.rs	New unified address input component with autocomplete and validation.
src/ui/components/mod.rs	Exposes the new ui::components::address_input module.
src/ui/wallets/send_screen.rs	Replaces simple-mode destination handling with AddressInput + ValidatedAddress.
src/ui/wallets/unshield_credits_screen.rs	Migrates destination input/parsing to AddressInput + ValidatedAddress.

---

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

[claude]

Claudius the Magnificent's Review — PR #787

The AddressInput component is genuinely excellent work — clean builder API, proper lazy init pattern, comprehensive 35+ test suite, user-friendly validation messages following Everyday User conventions. The address model's detection/validation split is architecturally sound. Well done.

3 MEDIUM findings posted as inline comments:

#	Finding	File
1	Shielded address validation accepts arbitrary content after prefix+length check — no Bech32m parsing unlike all other address types	address_input.rs:515-536
2	truncate_address() uses byte-offset slicing — panics on multi-byte UTF-8 from DPNS names (DoS vector)	address_input.rs:916-922
3	AddressInput not re-initialized when source selection changes — allows invalid source/destination combinations	send_screen.rs:1479-1489

None of these are blockers, but all three affect the end-user experience in a wallet application. The shielded validation gap (#1) is the most important — it's the only address type where "validated" doesn't mean "actually parsed."

_{🤖 Co-authored by Claudius the Magnificent AI Agent}
📊 View full HTML review report

[Copilot]

Pull request overview

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

---

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

[claude]

Claudius the Magnificent's Review — PR #787 (Round 2)

Three specialist agents (Smythe/security, Adams/project, Bilby/Rust) reviewed 6 files (~1885 lines). After deduplication of 40 raw findings → 18 unique findings: 1 HIGH, 5 MEDIUM, 6 LOW, 6 INFO.

Previous comments addressed ✅

6 of 17 prior review threads are now fixed and resolved:

• ✅ Shielded validation now uses OrchardAddress::from_bech32m_string() (was prefix+length only)
• ✅ truncate_address() now uses .chars() for UTF-8 safety
• ✅ AddressInput re-initialized on source selection change
• ✅ ValidatedAddress::Platform now stores canonical bech32m string
• ✅ Identity kind now scoped per source selection
• ✅ Send screen properly restricts allowed destination types

Key findings this round

#	Sev	Finding	Status
RUST-001	🔴 HIGH	changed flag never set to true — blur-triggered validation never propagates to screens. Manual address entry is broken.	Existing thread
RUST-002	🔸 MED	selected_from_autocomplete never resets — has_changed() stuck true every frame after selection	Existing thread
RUST-003	🔸 MED	validate_platform() silently accepts mixed-case bech32m (BIP-173/350 violation)	Existing thread
PROJ-001	🔸 MED	ComboBox id collision with fixed "address_type_filter" string	Existing thread
PROJ-002	🔸 MED	"Type 17"/"Type 19" jargon shown to Everyday Users	New comment

Most MEDIUM+ findings already have unresolved copilot threads from previous rounds. Only the jargon finding (PROJ-002) is posted as a new inline comment.

The HIGH finding (RUST-001) is the blocker: users who type/paste an address and tab away will see the ✅ validation label, but the send/confirm button stays disabled because validated_destination is never populated. Only autocomplete selection works. This needs to be fixed before merge.

_{🤖 Co-authored by Claudius the Magnificent AI Agent}
📊 View full HTML review report

[thepastaclaw]

Code Review

Verified 4 agent findings as valid, dropped 3 as false positives/non-issues. One confirmed blocking bug: with_anchor_retry leaks shielded state on sync_notes failure, leaving the wallet permanently unusable until app restart. All 3 prior review findings are resolved in this increment.

Reviewed commit: 811c098

🔴 1 blocking | 🟡 3 suggestion(s) | 💬 1 nitpick(s)

5 additional findings

🔴 blocking: with_anchor_retry leaks shielded state when sync_notes fails after anchor mismatch

src/context/shielded.rs (lines 432-469)

State is removed from the map at line 434. If the first operation hits ShieldedAnchorMismatch and the subsequent sync_notes call fails (line 453), the ? operator returns early. Lines 464-467 that re-insert state into the map are never reached. The wallet's shielded state is permanently lost — every subsequent shielded operation returns WalletNotFound until the app restarts.

All other error paths correctly fall through to the re-insertion block (non-anchor-mismatch errors at line 437, retry operation failure at line 455). Only the sync_notes failure path is broken.

Fix: use a scope guard or restructure so state is always re-inserted, e.g. wrap the whole body in a closure that re-inserts state in a finally-like pattern, or replace ? with explicit error handling that re-inserts before returning.

💡 Suggested change

Replace the `?` on line 453 with explicit error handling:
```rust
.await {
    Ok(()) => {},
    Err(e) => {
        tracing::warn!("Note sync after anchor mismatch failed: {e}");
        let mut states = self.shielded_states.lock().unwrap();
        states.insert(*seed_hash, state);
        return Err(e);
    }
};

Or better, use a drop guard that ensures state is always re-inserted.

</details>

</details>
<details>
<summary>🟡 <b>suggestion:</b> Failed config.save() warning claims session update that never happens</summary>

`src/ui/network_chooser_screen.rs (lines 494-500)`

On `config.save()` failure (line 494), the warning says "Your changes will apply for this session only." But the in-memory config update (`app_context.config.write()` at line 520-522) is exclusively in the `else` branch — the successful-save path. So on save failure, changes do NOT apply for this session either. The message should say changes were not applied at all, or the in-memory update should be done regardless of save success.

<details>
<summary>💡 Suggested change</summary>

```suggestion
Either fix the message to "Could not save the configuration file. Your changes were not applied." or move the in-memory update before the save attempt so the message is truthful.

🟡 suggestion: Forced resync clears shared commitment tree tables for all wallets

src/context/shielded.rs (lines 246-262)

clear_commitment_tree_tables() (database/shielded.rs:171-178) runs unscoped DELETE FROM commitment_tree_* statements with no WHERE clause. The commitment tree tables are shared across all wallets in the same DB. If wallet A triggers a forced resync (no notes in DB), it wipes tree data that wallet B depends on for its note witnesses.

Practical impact depends on whether multiple shielded wallets are active simultaneously — the shielded_states map keyed by WalletSeedHash suggests this is architecturally supported.

💡 Suggested change

If the commitment tree is truly global (one per chain), this may be acceptable since all wallets would need the same tree. But if so, add a comment explaining why clearing is safe. If wallets can have independent trees, scope the delete.

🟡 suggestion: Confusing brace structure in config save handler

src/ui/network_chooser_screen.rs (lines 493-554)

The if let Err(e) = config.save() / else block has misleading indentation — the else body (lines 509-552) is not visually indented under the else, and the closing } on line 553 has a comment // else: config.save() succeeded that is easy to misread. This contributed to the misleading-message bug above — the structure makes it hard to see which code runs in which branch.

💬 nitpick: Unnecessary outer clone of to_core_address

src/context/shielded.rs (lines 402-412)

to_core_address is taken by value (already owned). The outer clone on line 402 (let to_core_address_clone = to_core_address.clone()) is redundant — the owned value could be moved directly into the closure. The inner clone on line 412 is still needed since the closure may be called twice (retry).

💡 Suggested change

Remove line 402 and capture `to_core_address` directly in the closure, cloning it inside.

🤖 Prompt for all review comments with AI agents

These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.

In `src/context/shielded.rs`:
- [BLOCKING] lines 432-469: with_anchor_retry leaks shielded state when sync_notes fails after anchor mismatch
  State is removed from the map at line 434. If the first operation hits `ShieldedAnchorMismatch` and the subsequent `sync_notes` call fails (line 453), the `?` operator returns early. Lines 464-467 that re-insert state into the map are never reached. The wallet's shielded state is permanently lost — every subsequent shielded operation returns `WalletNotFound` until the app restarts.

All other error paths correctly fall through to the re-insertion block (non-anchor-mismatch errors at line 437, retry operation failure at line 455). Only the `sync_notes` failure path is broken.

Fix: use a scope guard or restructure so state is always re-inserted, e.g. wrap the whole body in a closure that re-inserts state in a `finally`-like pattern, or replace `?` with explicit error handling that re-inserts before returning.
- [SUGGESTION] lines 246-262: Forced resync clears shared commitment tree tables for all wallets
  `clear_commitment_tree_tables()` (database/shielded.rs:171-178) runs unscoped `DELETE FROM commitment_tree_*` statements with no WHERE clause. The commitment tree tables are shared across all wallets in the same DB. If wallet A triggers a forced resync (no notes in DB), it wipes tree data that wallet B depends on for its note witnesses.

Practical impact depends on whether multiple shielded wallets are active simultaneously — the `shielded_states` map keyed by `WalletSeedHash` suggests this is architecturally supported.

In `src/ui/network_chooser_screen.rs`:
- [SUGGESTION] lines 494-500: Failed config.save() warning claims session update that never happens
  On `config.save()` failure (line 494), the warning says "Your changes will apply for this session only." But the in-memory config update (`app_context.config.write()` at line 520-522) is exclusively in the `else` branch — the successful-save path. So on save failure, changes do NOT apply for this session either. The message should say changes were not applied at all, or the in-memory update should be done regardless of save success.
- [SUGGESTION] lines 493-554: Confusing brace structure in config save handler
  The `if let Err(e) = config.save()` / `else` block has misleading indentation — the `else` body (lines 509-552) is not visually indented under the else, and the closing `}` on line 553 has a comment `// else: config.save() succeeded` that is easy to misread. This contributed to the misleading-message bug above — the structure makes it hard to see which code runs in which branch.