fix(ui-spv): address PR #837 review feedback

[thepastaclaw]

Summary

• Replace the ad-hoc single-key SPV warnings with a shared inline banner
  helper that keeps MessageBanner warning styling and centralized copy.
• Dim disabled single-key Send button labels so SPV-disabled actions no
  longer look enabled.
• Simplify the touched backend-e2e single-key tests to the SPV-only path
  the harness actually runs.
• Log explicit cleanup failures when transactions_waiting_for_finality
  cannot be locked during shielded asset-lock cleanup.

Context

This is a helper PR against
#837
(fix/spv-rpc-path-audit) to address the concrete review comments from
Lukasz and Copilot.

Validation

• cargo fmt --check ✅
• cargo check --all-features ✅
• cargo clippy --workspace --all-targets -- -D warnings ⚠️ fails in
  pre-existing backend-e2e test-harness code on this branch/worktree
  (missing feature-gated helpers/accessors such as
  AppContext::wallets() / db() / sdk() and
  database::test_helpers).
• cargo test --test backend-e2e --all-features -- --ignored --nocapture test_tc003_refresh_single_key_wallet_info test_tc009_send_single_key_wallet_payment ⚠️ blocked by missing
  required E2E_WALLET_MNEMONIC.

Summary by CodeRabbit

Release Notes

• New Features

  • Implemented unified warning message and banner for single-key wallet operations requiring Dash Core.

• Improvements

  • Enhanced visual feedback for disabled buttons with dimmed text styling.
  • Improved error and warning logging for transaction handling during broadcast failures and timeouts.

[coderabbitai]

Important

Review skipped

Draft detected.

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: 409ec922-a6b1-4712-b209-239f9a637e8a

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

📝 Walkthrough

Walkthrough

This pull request consolidates single-key wallet warning messaging into shared components, improves error logging in transaction cleanup logic with explicit handling of mutex acquisition failures, and simplifies backend test cases by removing RPC mode branching logic.

Changes

Cohort / File(s)	Summary
Backend Error Handling src/backend_task/shielded/bundle.rs	Replaced if let Ok(...) with explicit match statement for mutex cleanup in shield_from_asset_lock. Adds tracing::error! logging on broadcast-failure cleanup failures and tracing::warn! on timeout-path cleanup failures.
UI Wallet Messaging src/ui/wallets/mod.rs	Extracted shared warning message constant SINGLE_KEY_REQUIRES_CORE_MESSAGE and added new public function render_single_key_requires_core_banner(ui) to render consistent inline warning banners across wallet UI components.
Single-Key UI Refactoring src/ui/wallets/single_key_send_screen.rs, src/ui/wallets/wallets_screen/single_key_view.rs	Updated components to import and use the shared warning message constant and banner rendering function. Modified "Send" button styling to use computed button_enabled state for consistent dimming of disabled button text. Replaced local tooltip constant with shared message.
Backend Test Simplification tests/backend-e2e/core_tasks.rs	Removed conditional RPC-mode branching in test_tc003_refresh_single_key_wallet_info and test_tc009_send_single_key_wallet_payment. Tests now unconditionally assert that single-key wallet operations fail with OperationRequiresDashCore error, eliminating previously successful RPC execution paths.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐰 Messages once scattered, now share a home,
Errors speak loudly, no silence to roam!
Tests cleaned of branches, wallets refine,
A tidy, sharp PR—our code looks divine! ✨

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title concisely describes the main change: centralizing SPV/single-key wallet warnings and addressing prior review feedback from PR #837 across multiple files.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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

[thepastaclaw]

@coderabbitai review

[coderabbitai]

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[thepastaclaw]

⏳ Review in progress (commit 9283f74)

[coderabbitai]

🧹 Nitpick comments (2)

tests/backend-e2e/core_tasks.rs (1)

234-315: Consider dropping the funding step now that the send path is unreachable.

TC-009 still funds the single-key wallet via SendWalletPayment + a 5s sleep before asserting RefreshSingleKeyWalletInfo fails with OperationRequiresDashCore. Since the refresh short-circuits before any UTXO is consumed, the funding transfer and sleep add ~5s and real on-chain activity without affecting the assertion. The comment calls this "parity with the RPC scenario," which is reasonable, but you could tighten the test to just construct the SKW and assert the typed error — matching TC-003's shape and cutting harness runtime/flakiness.

If the funding is intentionally retained as a smoke check that SendWalletPayment itself still works end-to-end in SPV mode, consider calling that out explicitly in the comment so the intent is obvious to future readers.

🤖 Prompt for AI Agents

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

In `@tests/backend-e2e/core_tasks.rs` around lines 234 - 315, The test
test_tc009_send_single_key_wallet_payment performs an unnecessary funding step
(the run_task call with BackendTask::CoreTask(CoreTask::SendWalletPayment { ..
}) and the subsequent tokio::time::sleep) even though RefreshSingleKeyWalletInfo
is expected to short-circuit with TaskError::OperationRequiresDashCore; remove
the funding run_task and the 5s sleep so the test simply constructs the
SingleKeyWallet (skw/skw_arc) and immediately calls run_task(app_context,
BackendTask::CoreTask(CoreTask::RefreshSingleKeyWalletInfo(skw_arc))).await
expecting an error, or if you want to keep the funding as an explicit smoke
check, update the test comment to state that intent and keep the funding block
guarded by an explanatory comment.

src/backend_task/shielded/bundle.rs (1)

502-567: Minor inconsistency: blocking lock() on broadcast-failure path vs try_lock() on timeout path.

The broadcast-failure cleanup (Line 502) uses blocking lock(), while the timeout cleanup (Line 555) uses try_lock(). The sibling implementation in src/context/transaction_processing.rs (see broadcast_asset_lock_transaction_internal around line 140) uses try_lock() on the broadcast-failure path for fire-and-forget cleanup. Since the only failure mode for lock() here is a poisoned mutex (contention with the SPV finality listener is brief), this is not a correctness issue, but aligning both paths to try_lock() would make the semantics and logging symmetric and avoid any chance of this cleanup step itself blocking on a poisoned lock while we're already on an error path.

Also consider including %tx_id / structured fields rather than embedding into the message, but that's purely stylistic.

🤖 Prompt for AI Agents

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

In `@src/backend_task/shielded/bundle.rs` around lines 502 - 567, In the
broadcast-failure cleanup path replace the blocking
transactions_waiting_for_finality.lock() call with
transactions_waiting_for_finality.try_lock() (same pattern as the timeout loop)
and update the Err branch to use tracing::warn with the same message structure
used in the timeout path (including tx_id and lock_err); keep the
proofs.remove(&tx_id) on the Ok path and ensure error handling returns Err(e) as
before so behavior is symmetric with broadcast_asset_lock_transaction_internal.

🤖 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/backend_task/shielded/bundle.rs`:
- Around line 502-567: In the broadcast-failure cleanup path replace the
blocking transactions_waiting_for_finality.lock() call with
transactions_waiting_for_finality.try_lock() (same pattern as the timeout loop)
and update the Err branch to use tracing::warn with the same message structure
used in the timeout path (including tx_id and lock_err); keep the
proofs.remove(&tx_id) on the Ok path and ensure error handling returns Err(e) as
before so behavior is symmetric with broadcast_asset_lock_transaction_internal.

In `@tests/backend-e2e/core_tasks.rs`:
- Around line 234-315: The test test_tc009_send_single_key_wallet_payment
performs an unnecessary funding step (the run_task call with
BackendTask::CoreTask(CoreTask::SendWalletPayment { .. }) and the subsequent
tokio::time::sleep) even though RefreshSingleKeyWalletInfo is expected to
short-circuit with TaskError::OperationRequiresDashCore; remove the funding
run_task and the 5s sleep so the test simply constructs the SingleKeyWallet
(skw/skw_arc) and immediately calls run_task(app_context,
BackendTask::CoreTask(CoreTask::RefreshSingleKeyWalletInfo(skw_arc))).await
expecting an error, or if you want to keep the funding as an explicit smoke
check, update the test comment to state that intent and keep the funding block
guarded by an explanatory comment.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: ebd3f984-ecee-44a7-942d-e891bb372674

📥 Commits

Reviewing files that changed from the base of the PR and between d14d4fb and 123d383.

📒 Files selected for processing (5)

• src/backend_task/shielded/bundle.rs
• src/ui/wallets/mod.rs
• src/ui/wallets/single_key_send_screen.rs
• src/ui/wallets/wallets_screen/single_key_view.rs
• tests/backend-e2e/core_tasks.rs

[thepastaclaw]

Following up on the CodeRabbit review here: I’m not taking further changes on #838. PR #837 already merged with the relevant fixes, and this helper PR is now closed/superseded. So the remaining suggestions on this draft helper branch are obsolete against the merged branch state rather than something I’m carrying in a follow-up.