fix(wallets): move alias rename into a backend task, off the UI thread

[Claudius-Maginificent]

TL;DR: Renaming a wallet or imported key now saves in the background like every other wallet action. A storage hiccup while saving no longer silently erases the wallet's other saved details, two overlapping renames of the same wallet can no longer clobber each other, renaming can't resurrect a wallet you just removed, and if a rename fails you get your typed name back instead of having to retype it.

User story

As a Dash Evo Tool user, I want renaming a wallet to be safe and reliable — never losing other saved settings, never showing me a stale name, and never losing what I typed if something goes wrong — so I can trust the name I see is the name that's actually saved.

Scenario

Base flow

A user opens the Wallets screen, picks a wallet (HD or an imported single key), opens the rename dialog, types a new name, and saves.

Actual behavior

The rename dialog saved the new name by reading and writing the wallet's saved details directly on the UI thread. If that read ever failed, the app treated the failure the same as "no details saved yet" and wrote a blank record with just the new name — silently dropping the wallet's other saved details instead of telling the user something went wrong.

A first pass moved this work to the background, off the UI thread — but that on its own introduced new gaps: switching to a different wallet before a rename finished could leave the wallet's displayed name stale until the app was restarted; renaming the same wallet twice in quick succession, or renaming a wallet while removing it, could interleave and produce an inconsistent result; and closing the dialog immediately on save meant a failure lost the name the user had just typed.

Expected behavior

Saving a new wallet name runs as a background action, the same way every other wallet operation does. If reading the wallet's existing saved details fails, the rename is aborted and reported as an error instead of overwriting those details with a blank record. Switching wallets mid-rename no longer leaves a stale name behind. Two renames of the same wallet, or a rename racing a removal, always resolve to one consistent, correct outcome. The rename dialog stays open (and disabled) while saving and, on failure, reopens with the name the user typed still there to retry.

Detailed discussion

What was done

• Added a fallible WalletMetaView::try_get (on the generic SidecarView) that distinguishes a genuinely absent row (Ok(None)) from an unreadable one (Err) — the existing get collapsed both cases to None, which is what let a read failure through as if there were nothing to preserve.
• Added two typed WalletTask variants, RenameHdWallet and RenameSingleKeyWallet, that perform the rename off the UI thread and return WalletAliasRenamed / SingleKeyAliasRenamed. The HD path reads through try_get, so a read fault now aborts the rename instead of dropping the wallet's other saved fields.
• wallets_screen/mod.rs now dispatches the rename as a background task on save and updates the in-memory label only from that task's success result, for both wallet kinds. The two ad hoc error banners at the old callsites are gone — failures now surface through the existing centralized TaskError-driven banner display.
• No new TaskError variants — existing typed variants (KvSidecarStorage, WalletNotFound, SingleKeyMetaStorage, ImportedKeyNotFound, InvalidWalletAliasLength) already covered every failure path.

Follow-up hardening (independent review + fix pass)

A follow-up review of the background-task change above found it had introduced its own new correctness gaps in the exact code it touched — all newly possible only because the rename write is no longer serialized on the single-threaded UI frame loop:

• Stale alias on selection change: the in-memory alias update was gated on the renamed wallet still being the currently selected one when the background result arrived. Switching wallets mid-rename silently dropped the update, and it didn't self-correct until app restart. Fixed by applying the update to the wallet by its own identity (seed_hash/address) instead.
• Concurrent renames of the same wallet: two overlapping rename calls for the same wallet had no synchronization, so their reads and writes could interleave. Added a per-wallet rename lock, and for the HD path, the wallet-store lookup guard is now held across the whole read-modify-write so a rename can't interleave with a concurrent removal of the same wallet either — one always fully completes before the other proceeds. The single-key path now holds its in-memory index lock through persistence for the same reason.
• Rename racing removal: an in-flight rename could resurrect a wallet-meta row that a concurrent removal had already deleted. Closed by the same lock-scope change above.
• Lost input on failure: the dialog closed and cleared its input the moment save was confirmed, before the background result was known — a failure (rare: storage fault, or the wallet having just been removed) forced a full retype. The dialog now stays open, disabled, and pre-filled while saving, and reopens editable with the same typed name if the save fails. The Rename button itself is now disabled for that same span too, so a save can't be interrupted by starting another one.
• Added deterministic (Condvar-gated, not sleep-based) regression tests for the two concurrency scenarios, and kittest coverage for the full confirm → dispatch → result cycle (success, failure, and the selection-race) for both wallet kinds.

Testing

• rename_hd_read_failure_surfaces_and_does_not_clobber reproduces the original bug: with the old get(...).unwrap_or_default() behavior this test failed by returning a successful (clobbering) rename; with try_get()? it now correctly returns a typed storage error and leaves the stored data untouched.
• New regression tests for the follow-up hardening were each confirmed RED against the unfixed code, then GREEN after the fix (concurrency races via a Condvar-gated fake KV store, not timing-dependent sleeps).
• cargo test backend_task::wallet::rename_wallet::tests --all-features — 8 passed.
• cargo test wallet_backend::single_key::tests --all-features — 24 passed.
• cargo test context::wallet_lifecycle::tests --all-features — 63 passed (regression check for the lock-scope change).
• cargo test --test kittest --all-features — 278 passed.
• cargo fmt --all — clean.
• cargo clippy --all-features --lib --tests -- -D warnings — clean.
• All of the above independently re-run and confirmed by a separate adversarial QA pass, which also proved the two concurrency tests are load-bearing (not tautological) by reverting each fix in isolation and confirming the corresponding test failed.

Breaking changes

None.

Checklist

• Tests updated where behavior changed
• cargo fmt --all
• Scoped cargo clippy clean
• Full CI suite (runs automatically on this PR)

Prior work

This finding originated as a CodeRabbit review comment on #927 (fee-reserve hygiene / validation centralization), which this repo decided to extend to cover both wallet kinds rather than just the HD path CodeRabbit flagged. #927 was squash-merged before this follow-up commit was ready, so it's carried forward here as its own PR against the current v1.0-dev tip rather than reopening the closed #927. The follow-up hardening commit responds to an independent multi-agent code review of this PR's first commit.

Attribution

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

[coderabbitai]

Warning

Review limit reached

@lklimek, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 7 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: d10d22bb-fdbe-4665-825f-ed6ad98beb7d

📥 Commits

Reviewing files that changed from the base of the PR and between 6c20946 and cfe7d45.

📒 Files selected for processing (10)

• CHANGELOG.md
• src/backend_task/mod.rs
• src/backend_task/wallet/mod.rs
• src/backend_task/wallet/rename_wallet.rs
• src/context/mod.rs
• src/ui/wallets/wallets_screen/mod.rs
• src/wallet_backend/sidecar.rs
• src/wallet_backend/single_key.rs
• src/wallet_backend/wallet_meta.rs
• tests/kittest/wallets_screen.rs

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/wallet-alias-rename-backend-task

---

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.}

[thepastaclaw]

⛔ Blockers found — Sonnet deferred (commit cfe7d45)
Canonical validated blockers: 2

[thepastaclaw]

Preliminary review — Codex only

The PR correctly moves alias persistence off the UI thread and improves metadata-read and concurrent-rename handling. Two blocking gaps remain: rename completions lose their originating screen and network affinity, and imported-key removal is not serialized with alias persistence, allowing deleted metadata to be recreated. HD-wallet removal can also block the UI while waiting for a pending rename's storage operation.

Validated blockers were found in the Codex precheck. Sonnet is deferred until a fresh Codex revalidation clears the blocker gate.

Review provenance

• Codex reviewers: gpt-5.6-sol — general (completed), gpt-5.6-sol — rust-quality (completed)
• Verifier: gpt-5.6-sol — verifier
• Sonnet: not run (deferred by blocker gate)

🔴 2 blocking | 🟡 1 suggestion(s)

3 additional finding(s) omitted (not in diff).

🤖 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/app.rs`:
- [BLOCKING] src/app.rs:2680-2684: Route rename completions to their originating wallet screen and network
  These new rename results fall through the generic arm and are delivered only to `visible_screen_mut()`. The rename window does not block root or pushed-screen navigation, so leaving Wallets before completion sends the callback to an unrelated screen. The persistent Wallets screen then retains `pending_rename_context`, leaving the dialog permanently disabled when the user returns, and its wallet Arc never receives the new alias. A network switch introduces a second failure: the backend task persists through the captured old `AppContext`, but every screen is rebound to the new context before the result arrives. Since the result carries no network provenance, the callback can update a same-seed wallet in the new network while leaving the originating context stale. Route completion and failure callbacks explicitly to the persistent Wallets root and preserve the originating `AppContext` or network when applying the in-memory update.

In `src/wallet_backend/single_key.rs`:
- [BLOCKING] src/wallet_backend/single_key.rs:375-387: Acquire the single-key index lock before removing persisted state
  `set_alias` now retains the index write guard across its sidecar write, but `forget` deletes the secret and sidecar before acquiring that guard. If a rename is paused in `kv.put`, `forget` can delete both persisted rows and then wait for the index lock; the rename subsequently recreates the metadata row before `forget` removes only the index entry. The final state has no secret or index entry but contains resurrected metadata for the removed key, contradicting the PR's removal-race guarantee. Hold the index guard for the complete delete-and-remove operation and add a Condvar-gated rename-versus-forget regression test.

In `src/ui/wallets/wallets_screen/mod.rs`:
- [SUGGESTION] src/ui/wallets/wallets_screen/mod.rs:841-843: Disable wallet removal while a rename is pending
  The Remove button remains active while an alias save is in progress. `rename_hd_wallet` intentionally holds the global wallets read guard across sidecar I/O, while the synchronous UI removal path needs the corresponding write guard. Confirming removal during a slow or stalled rename therefore blocks the egui frame until the background storage operation finishes, partially reintroducing the UI stall this PR is intended to eliminate. Disable removal while a rename is pending, or move removal into the backend task system.