feat(overlay): blocking progress overlay + SPV-sync hard-block

[lklimek]

Why this PR exists

• Problem: Long, unsafe operations (signing, broadcasting a state transition, identity registration, key import, migration) and the initial SPV chain sync run with the whole UI live. There was no component to block interaction behind a clear "please wait" plane, and nothing wired it to the get-connected sync.
• What breaks without it: A user clicks Broadcast, sees no block, clicks again → a second conflicting state transition (double broadcast). Or, on startup/Connect, they start using the app mid-sync and act on stale/empty state. These are the hazards this PR fences off.
• Blocking relationship: Stacked atop feat: platform-wallet backend rewrite (spec + implementation) #860 (platform-wallet migration). Built on the docs/platform-wallet-migration-design branch and targets it; merge after feat: platform-wallet backend rewrite (spec + implementation) #860.

Imagine you launch the app or press Connect. The screen dims, a spinner with "Syncing with the Dash network." and a "Step 2 of 5" indicator appears, and you can't act on half-synced data — but a "Continue in the background" button is always there if you'd rather not wait.

What was done

1. ProgressOverlay component (src/ui/components/progress_overlay.rs)

• Full-window blocking modal — indeterminate spinner (no fake ETA), optional "Step N of M" counter, scrollable description, optional primary/secondary buttons. Implements the project Component trait alongside a global ctx.data render path, mirroring MessageBanner.
• Renders at AppState::update level (claim_input at frame start + render_global after panels) at Order::Foreground, so it covers all panels; the secret-prompt modal stays above and fully typeable (keyboard gated off while a prompt is active).
• Generic button facility, no built-in Cancel: with_button / with_secondary_button; the caller owns semantics. Clicks are keyed per overlay entry and delivered to the owner via OverlayHandle::take_actions; the app loop only sweeps orphan actions.
• Safety model: no fake cancellation; a caller contract (clear on every terminal path; button-less blocks cover only bounded ops) + a 30 s reassurance line and 120 s no-progress watchdog. Total input block (text + Space/Enter/Tab/arrows, focus released from widgets beneath).

2. First adopter — SPV-sync hard-block (src/app.rs)

• Hard-blocks the UI during the user-initiated get-connected sync only: startup auto-start and the Connect button. An spv_block_armed gate scopes it to that episode and disarms on Synced/Error/network-switch — so ambient mid-session reconnects and per-block catch-up never re-block a working user.
• Plain-language copy ("Connecting to the Dash network." / "Syncing with the Dash network.") + a jargon-free "Step N of 5" phase counter. Always-visible "Continue in the background" escape — SPV sync is unbounded (offline ⇒ never completes), so the escape guarantees the user is never trapped.
• Suppresses the redundant Connecting/Syncing text banner while blocked; keeps the ambient top-panel indicator.

Testing

• cargo test --lib --all-features → 919 passed, 0 failed, 1 ignored.
• cargo test --test kittest --all-features → 135 passed, 0 failed, 0 ignored.
• cargo +nightly fmt --all -- --check and cargo clippy --all-features --all-targets -- -D warnings → clean.
• Coverage incl. teeth-y regression guards: keyboard/text blocked beneath a button-less overlay; passphrase prompt typeable over an overlay (real AppState gate); backdrop/Esc/Space do not dismiss/activate; keyed action FIFO + orphan sweep; 30 s/120 s watchdog via clock seam; and the SPV arming-gate test proving ambient reconnect does NOT block (fails if the gate is removed).

Breaking changes

None.

Checklist

• fmt + clippy -D warnings clean
• Tests green (919 lib + 135 kittest)
• Design docs + docs/user-stories.md updated (UX-001 + new UX-002 SPV-block story)
• Wired the overlay to a first real operation (SPV sync) under the caller contract

Attribution

_{🤖 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: 8865e2ba-d9a7-4dee-86ee-85d910cdd46b

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/blocking-progress-overlay

---

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]

✅ Review complete (commit 8fefb7e)

[Copilot]

Pull request overview

Adds a new global, full-window blocking progress overlay component and wires it into the app frame loop, with a first concrete adopter that hard-blocks UI interaction during user-initiated SPV “get connected” sync (startup/Connect) while still providing a safe “Continue in the background” escape.

Changes:

• Introduces ProgressOverlay as a reusable UI component (global ctx.data path + Component instance path), including input-claiming at frame start and keyed action delivery via OverlayHandle::take_actions.
• Integrates the overlay into AppState::update and adds an SPV-sync-specific blocking policy (armed episode, disarm on terminal, escape hatch).
• Adds extensive kittest coverage plus supporting test seams (testing feature) and updates documentation/catalogs.

Reviewed changes

Copilot reviewed 14 out of 14 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
src/ui/components/progress_overlay.rs	Implements the blocking overlay (rendering, input claiming, action queue semantics, watchdog/escalation).
src/app.rs	Integrates overlay into the frame loop and adds SPV-sync blocking overlay policy + test seams.
src/context/connection_status.rs	Adds test seam to force overall connection state and a helper mapping SPV phases to “Step N of 5”.
src/ui/components/passphrase_modal.rs	Ensures secret prompt window renders above the overlay via Order::Foreground.
src/ui/components/secret_prompt_host.rs	Adds a testing-only stub active prompt for AppState-level overlay/prompt gate tests.
src/ui/components/mod.rs	Exposes progress_overlay module and re-exports overlay types.
src/ui/components/README.md	Documents ProgressOverlay in the reusable component catalog.
tests/kittest/progress_overlay.rs	Adds comprehensive kittest suite covering overlay behavior, input blocking, z-order, and SPV overlay wiring.
tests/kittest/main.rs	Registers the new progress overlay kittests module.
docs/user-stories.md	Adds UX stories for the blocking overlay and SPV-sync blocking adopter.
docs/ai-design/2026-06-17-blocking-progress-overlay/01-requirements-ux.md	Requirements/UX spec for the overlay (with supersession notes).
docs/ai-design/2026-06-17-blocking-progress-overlay/02-test-spec.md	Detailed acceptance test specification for the overlay.
docs/ai-design/2026-06-17-blocking-progress-overlay/03-dev-plan.md	Development plan and architecture decisions (incl. post-outage redesign notes).
docs/ai-design/2026-06-17-blocking-progress-overlay/04-design-addendum.md	QA-wave design addendum (watchdog, input-claim, keyed action dispatch, SPV exception).

---

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

[thepastaclaw]

Code Review

Generally well-factored overlay + SPV hard-block wiring with solid test coverage. Verified concerns: (1) frame ordering leaves one frame of interactive UI after Connect because update_spv_overlay runs after claim_overlay_input and the screen's ui(); (2) the 120s no-progress watchdog will spuriously fire on legitimately slow SPV phases because (description, step) doesn't change while heights advance; (3) the doc comment on claim_overlay_input is a copy/paste residue describing the orphan-sweeper; (4) a kittest header still cites Order::Middle though the renderer moved to Order::Foreground for SEC-002. No in-scope blocking defects.

🟡 2 suggestion(s) | 💬 2 nitpick(s)

🤖 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`:
- [SUGGESTION] src/app.rs:1791-1830: Connect leaves one frame of interactive UI before the SPV block is raised
  `update_spv_overlay` runs at line 1824, after `claim_overlay_input` (1794), the visible screen's `ui()` (1803), and `ProgressOverlay::render_global` (1810). `StartSpv` is then processed at line 1882 in the action loop, setting `spv_block_armed = true`. On the next frame, `claim_overlay_input` and the screen's `ui()` execute before `update_spv_overlay` raises the overlay, so a full frame passes with no overlay in `ctx.data`, no input claim, and a fully interactive screen. The overlay only becomes visible and effective on frame N+2. For a feature explicitly framed as a hard-block on user-initiated sync (PR title + F-SPV-A in the doc comment), the one-frame interactive gap contradicts the safety goal. Fix is local: drive `update_spv_overlay` before `claim_overlay_input` so a newly armed episode raises the overlay in time for the same frame's input claim and global render.
- [SUGGESTION] src/app.rs:1158-1189: 120s no-progress watchdog will spuriously escalate during slow SPV phases
  `update_spv_overlay` writes the same `description` (`SPV_SYNCING_DESCRIPTION` or `SPV_CONNECTING_DESCRIPTION`) and the same step number (1–5 from `spv_phase_step`) for the entire duration of a phase — the underlying `SpvSyncProgress` heights are not threaded into overlay content. `log_overlay_state` (progress_overlay.rs:819) only resets `last_progress_at` when `(description, step)` actually changes, so any phase that runs past 120s (typical for Headers on slower links) trips `watchdog_tripped`, swaps the copy to `STUCK_WATCHDOG_REASSURANCE` ("This is taking much longer than expected…") AND fires a `tracing::error!` (progress_overlay.rs:677-684) claiming a leaked handle or un-bounded operation. That dev-error is exactly the signal the PR's C2 escape was designed to suppress for SPV. Thread a monotonic progress signal into `set_description` (e.g. include a height/percent fragment for the active phase) so content ticks across frames while real sync work happens; or special-case the SPV adopter to skip the escalated copy when the underlying sync state is still advancing.

[thepastaclaw]

Code Review

Incremental reconciliation: prior findings #1 (Connect one-frame interactive leak) and #2 (no-progress watchdog spurious escalation during slow SPV phases) remain STILL VALID at b726871 and are carried forward; prior nitpicks #3 (claim_overlay_input doc) and #4 (Order::Middle kittest comment) are FIXED in the latest delta. The latest delta (phase-count constant, claim_input keyboard hardening, deterministic elapsed test, doc cleanup) is sound and introduces no new regressions. One new agent finding (SPV unbounded block lacks keyboard-accessible escape) is already acknowledged in-code via TODO(RUST-006) pending product decision and is therefore intentionally deferred.

🟡 1 suggestion(s)

1 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.

🤖 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`:
- [SUGGESTION] src/app.rs:1160-1195: No-progress watchdog still misfires during slow but healthy SPV phases
  Carried-forward (STILL VALID at current head). `update_spv_overlay` still writes only `(description, step)` to the overlay — one of two fixed `&'static str` descriptions (`SPV_CONNECTING_DESCRIPTION` / `SPV_SYNCING_DESCRIPTION`) plus the phase step from `spv_phase_step`, which is constant for the entire duration of a single phase (Headers, Masternodes, Filter Headers, Filters, Blocks). `log_overlay_state` in `src/ui/components/progress_overlay.rs:849-869` only bumps `last_progress_at` when `(description, step)` changes, so after `STUCK_OVERLAY_WATCHDOG_THRESHOLD` (120s) the watchdog at `progress_overlay.rs:702-714` will log a one-shot `tracing::error!("… likely a leaked handle or an un-bounded operation")` and the overlay flips to the “taking much longer than expected” copy — even when the underlying `SpvSyncProgress` heights are visibly advancing on a slow Filters or Blocks phase. The new `TODO(SEC-001)` comment acknowledges the watchdog isn't actionable but doesn't address the false-positive itself. Either (a) thread a monotonic progress signal (e.g. current/target height per phase, or even a phase-tick counter) into the overlay state so the content tuple changes as the chain advances, or (b) extend the watchdog with a caller-supplied progress monotone (`bump_progress()` on `OverlayHandle`) that the SPV adopter calls whenever any phase's `current_height` advances. As is, the watchdog log will fire on healthy testnet/mainnet syncs.

[thepastaclaw]

Code Review

Both prior findings (one-frame interactive gap; watchdog false-stall on slow phases) are FIXED at 20723d2 — update_spv_overlay now runs before input claim and screen UI, and a hidden monotonic spv_progress_token resets the watchdog on intra-phase height progress. New delta findings: (1) try_auto_start_spv on post-onboarding auto-start launches SPV without arming spv_block_armed, so the first sync after onboarding is not hard-blocked; (2) claim_input still TODOs pointer claiming, so a mouse click queued at the start of an overlay's raise frame can reach widgets beneath before the foreground sink paints. Both are suggestion-grade — the boot-time and Connect-button arming paths are correctly covered.

🟡 3 suggestion(s)

1 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`:
- [SUGGESTION] src/app.rs:1469-1483: Post-onboarding auto-start launches SPV without arming the hard block
  `try_auto_start_spv` (called from `AppAction::OnboardingComplete` at app.rs:1960) spawns `ensure_wallet_backend_and_start_spv` but never sets `spv_block_armed = true` or clears `spv_overlay_dismissed`. The only arming sites are the boot constructor (app.rs:760, gated on `boot_auto_start_spv`) and the manual `StartSpv` action (app.rs:1916). When a user completes onboarding with auto-start enabled, the initial SPV sync episode kicks off but `update_spv_overlay` observes `armed == false` and returns `Idle`, leaving the UI fully interactive against an unsynced chain — the exact scenario the hard-block PR exists to prevent. The fix is symmetrical with the boot path: set `spv_block_armed = true; spv_overlay_dismissed = false;` before spawning. The method needs to take `&mut self` for that.

In `src/ui/components/progress_overlay.rs`:
- [SUGGESTION] src/ui/components/progress_overlay.rs:791-793: Pointer events can still click through on the frame an overlay is raised
  `claim_input` is now the frame-start ownership boundary for blocking overlays, but it only strips keyboard/text events — the in-code TODO acknowledges pointer claiming is still missing. In `AppState::update`, `update_spv_overlay` can raise the overlay, `claim_overlay_input` runs, then the visible screen's `ui(ctx)` runs, then `ProgressOverlay::render_global` finally installs the full-window `Sense::click_and_drag` foreground sink. On the first frame an overlay is newly raised, no sink existed last frame, so a mouse press queued at the start of this frame is evaluated against widgets beneath before the sink exists. For SPV-sync today this is mitigated because arming usually happens via `AppAction::StartSpv` in the prior frame (not synchronous with a mouse-down), but it leaves the same class of double-click hazard the PR closes for keyboard input. Mirror the QA-001 frame-start key claim for `PointerButton::Primary` press/release/click/drag events and add a kittest with a synthetic pointer event over a button under the overlay.
- [SUGGESTION] src/ui/components/progress_overlay.rs:737-767: Opt-in keyboard escape lets Enter/Space remain on `ctx.input` for the screen ui pass
  When `escape_confirmed`, `claim_input` retains Enter/Space key-press events on `ctx.input` so the focused overlay button can activate. The focus check (`escape_focus_id` recorded by last frame + `ctx.memory().focused()` match) correctly denies the raise frame and prevents focused widgets beneath from consuming the event — but `AppState::update` still runs `visible_screen_mut().ui(ctx)` after `claim_overlay_input` and before `ProgressOverlay::render_global`. Any screen or component that reads raw `ctx.input(|i| i.key_pressed(Key::Enter))` (global shortcuts, list-row activation by Enter, etc.) will observe the same event before the overlay button processes it on the later render pass. A safer pattern is to consume Enter/Space at frame start and enqueue the escape action directly for the overlay entry, rather than retaining the event on the shared input stream. Worth auditing existing screens for raw key-press reads before treating this as benign.

[thepastaclaw]

🟡 Suggestion: Pointer events can still click through on the frame an overlay is raised

claim_input is now the frame-start ownership boundary for blocking overlays, but it only strips keyboard/text events — the in-code TODO acknowledges pointer claiming is still missing. In AppState::update, update_spv_overlay can raise the overlay, claim_overlay_input runs, then the visible screen's ui(ctx) runs, then ProgressOverlay::render_global finally installs the full-window Sense::click_and_drag foreground sink. On the first frame an overlay is newly raised, no sink existed last frame, so a mouse press queued at the start of this frame is evaluated against widgets beneath before the sink exists. For SPV-sync today this is mitigated because arming usually happens via AppAction::StartSpv in the prior frame (not synchronous with a mouse-down), but it leaves the same class of double-click hazard the PR closes for keyboard input. Mirror the QA-001 frame-start key claim for PointerButton::Primary press/release/click/drag events and add a kittest with a synthetic pointer event over a button under the overlay.

_{source: ['claude', 'codex']}

[thepastaclaw]

Resolved in f50cbca — Pointer events can still click through on the frame an overlay is raised no longer present.

_{Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.}

[thepastaclaw]

Correction: the reconciliation reply above was posted in error. Re-validating the current head 4ad29505, the pointer-event click-through issue is still valid and was carried forward in review #863 (review).

Please ignore the resolved note above; this thread should remain open until pointer events are claimed on the overlay-raising frame.

[lklimek]

Confirmed still open and intentional. Frame-start pointer-event claiming is deferred follow-up, tracked in-source as TODO(SEC-002-pointer) (progress_overlay.rs) and in our backlog alongside its keyboard sibling.

For the originally-reported symptom (escape button unclickable during SPV sync), the shipped fix at 1f45f519 — ctx.set_sublayer(sink_layer, card_layer) — pins the content card above the dim sink, so the button receives clicks reliably even after a backdrop press. Full frame-start pointer claiming (defense-in-depth against the one-frame raise gap) remains a distinct hardening task. Leaving this thread open to track it.

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

[thepastaclaw]

Code Review

Incremental review against 4ad2950. Carried-forward findings from 20723d2: the post-onboarding auto-start path still does not arm the SPV hard block (try_auto_start_spv at app.rs:1480 takes &self and never sets spv_block_armed), and pointer events still click through on the overlay's raising frame (claim_input strips only keyboard/text — TODO(SEC-002-pointer) acknowledged at progress_overlay.rs:789). The prior Enter/Space passthrough finding is FIXED by f50cbca — claim_input now enqueues the designated escape action at frame start and strips the key unconditionally, with a dedicated test. New in this delta: the focus-independent escape activation removes the implicit "button-id must match" check that previously enforced the documented no-op contract for unmatched escape ids (with_keyboard_escape doc at line 290-291 vs. claim_input at line 736).

🟡 2 suggestion(s)

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

1 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.

🤖 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`:
- [SUGGESTION] src/app.rs:1480-1494: Post-onboarding auto-start launches SPV without arming the hard block
  `try_auto_start_spv` (called from `AppAction::OnboardingComplete` at app.rs:1973) calls `ensure_wallet_backend_and_start_spv` but never sets `spv_block_armed = true` or clears `spv_overlay_dismissed`. The two other initial-sync entrypoints arm the block symmetrically with the spawn: the boot constructor at app.rs:760 (`spv_block_armed: boot_auto_start_spv`) and the manual `StartSpv` action at app.rs:1929. When a user finishes onboarding with `auto_start_spv` enabled, the SPV episode kicks off but `update_spv_overlay` observes `armed == false` and resolves to Idle, leaving the UI fully interactive against an unsynced chain — the exact double-broadcast hazard this PR exists to prevent. The cheapest fix is to change the signature to `&mut self` and set `self.spv_block_armed = true; self.spv_overlay_dismissed = false;` before spawning, mirroring the boot/manual paths. A kittest driving `AppAction::OnboardingComplete` with `auto_start_spv = true` would lock in the invariant.

In `src/ui/components/progress_overlay.rs`:
- [SUGGESTION] src/ui/components/progress_overlay.rs:736-737: Keyboard escape can enqueue an action with no rendered owner button
  The new frame-start escape path at line 736 clones `top.keyboard_escape_action` and enqueues it on Enter/Space without checking whether any `OverlayButton` in `top.buttons` carries that action id. That breaks the documented contract on `with_keyboard_escape` at line 290-291 ("Designating an action id that no button carries is a no-op") which was previously enforced implicitly: under the prior focus-based activation, focus could only be granted to a button whose id matched the escape, so an unmatched id could never activate via Enter. The SEC-001/SEC-002 refactor moved activation to frame-start and dropped that implicit check. Consequence: a caller that typos the escape id wires a live, invisible keyboard action the user cannot see or click — and a screen drain that's not expecting it. Filter `escape_action` against `top.buttons` (`top.buttons.iter().any(|b| b.action_id == *id)`) or update the doc to reflect the new "always-on" semantics. The codex-suggested filter at line 736 is the minimal fix.

[thepastaclaw]

🟡 Suggestion: Keyboard escape can enqueue an action with no rendered owner button

The new frame-start escape path at line 736 clones top.keyboard_escape_action and enqueues it on Enter/Space without checking whether any OverlayButton in top.buttons carries that action id. That breaks the documented contract on with_keyboard_escape at line 290-291 ("Designating an action id that no button carries is a no-op") which was previously enforced implicitly: under the prior focus-based activation, focus could only be granted to a button whose id matched the escape, so an unmatched id could never activate via Enter. The SEC-001/SEC-002 refactor moved activation to frame-start and dropped that implicit check. Consequence: a caller that typos the escape id wires a live, invisible keyboard action the user cannot see or click — and a screen drain that's not expecting it. Filter escape_action against top.buttons (top.buttons.iter().any(|b| b.action_id == *id)) or update the doc to reflect the new "always-on" semantics. The codex-suggested filter at line 736 is the minimal fix.

_{source: ['codex-rust-quality']}

[lklimek]

Valid observation — with_keyboard_escape documents that an action id no button carries is a no-op, but claim_input enqueues the escape action regardless of whether a matching button exists. Low severity in practice: the only production opt-in (the SPV Continue in the background escape) always carries a matching secondary-action button, so no live path violates the contract.

We're tracking this and will reconcile doc and behavior — either guard the enqueue against state.buttons or change the doc to document always-on semantics. Leaving open until that lands.

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

[thepastaclaw]

Correction: this auto-resolve reply was produced by the reconciler in error. The finding is still valid at 8fefb7e4: claim_input still enqueues keyboard_escape_action on Enter/Space without first checking that a rendered OverlayButton owns that action id. The current review summary intentionally carries this forward as unresolved.

[thepastaclaw]

Code Review

Incremental review against d057027. The latest delta is a merge of origin/docs/platform-wallet-migration-design (mnlist-inspector removal, watch-only identity funding fix, SPV shutdown fix) that does not touch the overlay/SPV-blocking wiring. No new in-scope defects introduced by the delta. Carried-forward from the prior review at 4ad2950: all three findings (post-onboarding auto-start does not arm the SPV hard block; pointer events click through on the overlay's raising frame; keyboard escape can enqueue an action no rendered button carries) verified STILL VALID at head and re-emitted as suggestions.

🟡 1 suggestion(s)

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

2 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.

🤖 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`:
- [SUGGESTION] src/app.rs:1473-1487: Post-onboarding auto-start launches SPV without arming the hard block
  Carried forward — verified still valid at d0570277. `try_auto_start_spv` (app.rs:1473) is called from `AppAction::OnboardingComplete` (app.rs:1966) but only spawns `ensure_wallet_backend_and_start_spv` — it never sets `spv_block_armed = true` or clears `spv_overlay_dismissed`. The other two initial-sync entrypoints arm the block symmetrically: the boot constructor at app.rs:753 (`spv_block_armed: boot_auto_start_spv`) and the manual `StartSpv` action at app.rs:1922 (`self.spv_block_armed = true; self.spv_overlay_dismissed = false;` before spawning). On the onboarding path `update_spv_overlay` observes `armed == false` and resolves to Idle, leaving the UI fully interactive during exactly the initial-sync episode this PR is meant to hard-block. Change the signature to `&mut self` and set `self.spv_block_armed = true; self.spv_overlay_dismissed = false;` before `spawn_sync`, mirroring the boot/manual paths. A kittest driving `AppAction::OnboardingComplete` with `auto_start_spv = true` would lock the invariant in.

[lklimek]

Fix: SPV escape button was unclickable with the mouse (1f45f519)

Symptom: while the SPV-sync hard block was up, clicking "Continue in the background" did nothing — force-quit was the only way out.

Root cause: a pre-existing egui z-order trap (not the keyboard-escape rework in f50cbcad). The overlay paints its full-window dim/pointer sink and its content card as two peer Order::Foreground areas. egui auto-raises any interactable area to the top of its order on a pointer press (area.rs bring-to-front), so a single click on the dim backdrop floated the click-absorbing sink above the card and permanently buried the button beneath it.

Fix (one line of logic): ctx.set_sublayer(sink_layer, card_layer) — egui places a sublayer directly above its parent after the per-frame order sort, so card-above-sink now holds by construction, immune to the bring-to-front race. The sink still blocks every widget beneath; the passphrase prompt still wins above the overlay.

Why review/tests missed it: existing button-click tests click straight on the button and never press the backdrop first. Added TC-OVL-054 (tc_ovl_054_escape_clickable_after_backdrop_press): presses the backdrop, then clicks the escape and asserts the action enqueues — fails pre-fix, passes post-fix.

Gates: fmt clean · clippy --all-features --all-targets -- -D warnings 0 warnings · kittest 145 passed (incl. new TC-OVL-054, and regression guards TC-OVL-028 sink-still-blocks / TC-OVL-048 secret-prompt gate) · lib progress_overlay 26 passed.

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

[Copilot]

Pull request overview

Copilot reviewed 14 out of 14 changed files in this pull request and generated 2 comments.

[thepastaclaw]

Code Review

Incremental review at 1f45f51. The delta itself is a focused, well-tested fix: pinning the card layer above the sink via ctx.set_sublayer to defeat egui's bring-to-front on backdrop press, with a clean kittest regression (TC-OVL-054). No new defects in the delta. All three prior findings (post-onboarding hard-block arming, frame-start pointer claim, keyboard-escape unvalidated id) remain STILL VALID at head and are carried forward — none are blocking, but they materially weaken the overlay's input-blocking and SPV hard-block contracts that this PR exists to enforce.

🟡 1 suggestion(s)

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

2 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.

🤖 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`:
- [SUGGESTION] src/app.rs:1473-1487: Carried-forward: post-onboarding SPV auto-start does not arm the hard block
  Verified at 1f45f519. `try_auto_start_spv` (app.rs:1473, signature is `&self`) is the third initial-sync entrypoint, alongside the boot constructor at app.rs:753 (`spv_block_armed: boot_auto_start_spv`) and `AppAction::StartSpv` at app.rs:1922-1923 (`self.spv_block_armed = true; self.spv_overlay_dismissed = false;`). Unlike those two, it spawns `ensure_wallet_backend_and_start_spv` without setting `spv_block_armed` or clearing `spv_overlay_dismissed`. Its sole caller is `AppAction::OnboardingComplete` at app.rs:1966 — exactly the initial-sync episode the hard block was designed to cover. With `armed == false`, `spv_block_step` resolves to Idle and the UI stays fully interactive during the freshly-onboarded user's first sync, when stale/empty state is most dangerous. The current `&self` signature also has to change to support the fix. The asymmetry across the three entrypoints is itself a maintainability hazard — the arming side-effect should live next to `spawn_sync` to prevent future entrypoints from reintroducing the same gap.

[thepastaclaw]

Code Review

Incremental review at 327b603. The PR-scoped files src/app.rs and src/ui/components/progress_overlay.rs are byte-identical to 1f45f51, so all three prior findings are carried forward as STILL VALID. The latest delta (merge of platform-wallet-migration-design) adds a tokio::sync::watch SPV-status mirror, a persister-release barrier in stop_spv, and backend-e2e tests; verification surfaced one new split-state concern: begin_spv_stop bypasses the watch sender, leaving subscribers stale during disconnect.

Reviewed commit: 327b6033 (PR head advanced after this queue item was claimed, so this body-only review is pinned to the assigned SHA.)

4 🟡 suggestion

Carried-forward prior findings

suggestion: Carried-forward: post-onboarding SPV auto-start does not arm the hard block

src/app.rs (lines 1473-1487)

Verified STILL VALID at 327b603 — src/app.rs:1473 still has fn try_auto_start_spv(&self) and spawns ensure_wallet_backend_and_start_spv without setting spv_block_armed = true or clearing spv_overlay_dismissed. The other two initial-sync entrypoints arm the block symmetrically: the boot constructor sets spv_block_armed: boot_auto_start_spv, and AppAction::StartSpv at src/app.rs:1922-1923 sets both fields before spawning. The sole caller of try_auto_start_spv is AppAction::OnboardingComplete at src/app.rs:1966 — exactly the initial-sync episode the hard block was designed to cover. With armed == false, update_spv_overlay resolves to Idle and the UI stays fully interactive during a freshly-onboarded user's first sync, when stale/empty state is most dangerous. The asymmetry across the three entrypoints is itself a maintainability hazard; the arming side-effect belongs next to spawn_sync so future entrypoints cannot reintroduce the same gap. Fix is mechanical: change to &mut self and set the two fields before spawning. A kittest driving AppAction::OnboardingComplete with auto_start_spv = true would lock the invariant in.

    fn try_auto_start_spv(&mut self) {
        let ctx = self.current_app_context().clone();
        let auto_start = ctx.get_app_settings().auto_start_spv;
        if auto_start && FeatureGate::SpvBackend.is_available(&ctx) {
            self.spv_block_armed = true;
            self.spv_overlay_dismissed = false;
            let sender = self.task_result_sender.clone();
            self.subtasks.spawn_sync("spv_auto_start", async move {
                if let Err(e) = ctx.ensure_wallet_backend_and_start_spv(sender).await {
                    tracing::warn!("Failed to auto-start SPV sync: {e}");
                } else {
                    tracing::info!("SPV sync started automatically for {:?}", ctx.network);
                }
            });
        }
    }

suggestion: Carried-forward: pointer events can click through on the overlay raise frame

src/ui/components/progress_overlay.rs (lines 789-791)

Verified STILL VALID at 327b603 — the in-source TODO(SEC-002-pointer) at progress_overlay.rs:789-791 still acknowledges the gap. claim_input is the frame-start ownership boundary that strips keyboard/text/clipboard events (lines 739-783) and frame-start-enqueues the designated keyboard escape (lines 786-788), but egui::Event::PointerButton press/release/click/drag events are never claimed. In AppState::update the sequence is: update_spv_overlay raises an overlay → claim_overlay_input (keyboard-only) → the visible screen's ui(ctx) consumes input → ProgressOverlay::render_global installs the full-window Sense::click_and_drag foreground sink. On the frame an overlay is newly raised, no sink existed last frame, so a queued primary pointer press is hit-tested against widgets beneath before the sink exists — the pointer analogue of the keyboard hazard SEC-001/002 already closed (distinct from the recent TC-OVL-054 backdrop z-order fix). For SPV this leaves a fast second Connect click possible; for the future broadcast/signing adopters this overlay is being built to fence off, it reopens the original double-broadcast hazard. Mirror the frame-start key claim for Event::PointerButton { button: Primary, pressed: true, .. } (and matching release/drag stripping) inside claim_input, and add a kittest with a synthetic pointer press over a button beneath a just-raised overlay.

suggestion: Carried-forward: keyboard escape enqueues an action with no rendered owner button

src/ui/components/progress_overlay.rs (lines 736-788)

Verified STILL VALID at 327b603. The docstring on with_keyboard_escape (progress_overlay.rs:290-291) explicitly states: "Designating an action id that no button carries is a no-op." The claim_input path at line 736 clones top.keyboard_escape_action and enqueues it via push_overlay_action on Enter/Space (lines 758-760, 786-787) based only on escape_action.is_some() and !*repeat, without checking whether any OverlayButton in top.buttons carries that action id. Under the prior focus-based activation this contract was enforced implicitly — focus could only be granted to a rendered button whose id matched the escape. The SEC-001/002 refactor moved activation to frame-start and dropped that implicit guard. Consequence: a caller that typos the escape id, removes the matching button while leaving the designation, or builds the entry conditionally silently dispatches an invisible keyboard-only action that no button advertises — and an owner screen drains an action it never offered. Either filter escape_action against the rendered button set at the same point it is cloned, or update the docstring to reflect the new "always-on" semantics. The filter is a one-line, behaviour-preserving change.

        let escape_action = top
            .keyboard_escape_action
            .as_ref()
            .filter(|action_id| {
                top.buttons
                    .iter()
                    .any(|button| button.action_id.as_str() == action_id.as_str())
            })
            .cloned();
        let key = top.key;
        let mut activate_escape = false;

New finding in latest delta

suggestion: begin_spv_stop bypasses the SPV status watch channel

src/context/connection_status.rs (lines 268-284)

Verified at 327b603. The latest delta introduces spv_status_tx: watch::Sender<SpvStatus> (connection_status.rs:60) as an event-driven mirror of the atomic, consumed by mcp::resolve::ensure_spv_synced (mcp/resolve.rs:147-163) via subscribe_spv_status().borrow_and_update(). set_spv_status (lines 223-235) keeps the atomic and the watch coherent via send_if_modified, but begin_spv_stop (lines 268-284) writes the atomic directly with compare_exchange and returns without updating the watch. There is a window where spv_status() returns Stopping but a freshly-subscribed receiver's first borrow_and_update() still sees the previous Running value. The next set_spv_status call (e.g. from a teardown event handler) closes the gap, but a wallet-facing MCP call that subscribes during that window passes the sync gate while teardown has already been claimed. The two state-stores need a single coherent mutation path: mirror the transition into the watch the same way set_spv_status does.

    pub fn begin_spv_stop(&self) -> bool {
        for current in [SpvStatus::Starting, SpvStatus::Syncing, SpvStatus::Running] {
            if self
                .spv_status
                .compare_exchange(
                    current as u8,
                    SpvStatus::Stopping as u8,
                    Ordering::AcqRel,
                    Ordering::Relaxed,
                )
                .is_ok()
            {
                let _ = self.spv_status_tx.send_if_modified(|cur| {
                    if *cur != SpvStatus::Stopping {
                        *cur = SpvStatus::Stopping;
                        true
                    } else {
                        false
                    }
                });
                return true;
            }
        }
        false
    }

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

- [SUGGESTION] In `src/app.rs`:1473-1487: Carried-forward: post-onboarding SPV auto-start does not arm the hard block
  Verified STILL VALID at 327b6033 — src/app.rs:1473 still has `fn try_auto_start_spv(&self)` and spawns `ensure_wallet_backend_and_start_spv` without setting `spv_block_armed = true` or clearing `spv_overlay_dismissed`. The other two initial-sync entrypoints arm the block symmetrically: the boot constructor sets `spv_block_armed: boot_auto_start_spv`, and `AppAction::StartSpv` at src/app.rs:1922-1923 sets both fields before spawning. The sole caller of `try_auto_start_spv` is `AppAction::OnboardingComplete` at src/app.rs:1966 — exactly the initial-sync episode the hard block was designed to cover. With `armed == false`, `update_spv_overlay` resolves to Idle and the UI stays fully interactive during a freshly-onboarded user's first sync, when stale/empty state is most dangerous. The asymmetry across the three entrypoints is itself a maintainability hazard; the arming side-effect belongs next to `spawn_sync` so future entrypoints cannot reintroduce the same gap. Fix is mechanical: change to `&mut self` and set the two fields before spawning. A kittest driving `AppAction::OnboardingComplete` with `auto_start_spv = true` would lock the invariant in.
- [SUGGESTION] In `src/ui/components/progress_overlay.rs`:789-791: Carried-forward: pointer events can click through on the overlay raise frame
  Verified STILL VALID at 327b6033 — the in-source `TODO(SEC-002-pointer)` at progress_overlay.rs:789-791 still acknowledges the gap. `claim_input` is the frame-start ownership boundary that strips keyboard/text/clipboard events (lines 739-783) and frame-start-enqueues the designated keyboard escape (lines 786-788), but `egui::Event::PointerButton` press/release/click/drag events are never claimed. In `AppState::update` the sequence is: `update_spv_overlay` raises an overlay → `claim_overlay_input` (keyboard-only) → the visible screen's `ui(ctx)` consumes input → `ProgressOverlay::render_global` installs the full-window `Sense::click_and_drag` foreground sink. On the frame an overlay is newly raised, no sink existed last frame, so a queued primary pointer press is hit-tested against widgets beneath before the sink exists — the pointer analogue of the keyboard hazard SEC-001/002 already closed (distinct from the recent TC-OVL-054 backdrop z-order fix). For SPV this leaves a fast second Connect click possible; for the future broadcast/signing adopters this overlay is being built to fence off, it reopens the original double-broadcast hazard. Mirror the frame-start key claim for `Event::PointerButton { button: Primary, pressed: true, .. }` (and matching release/drag stripping) inside `claim_input`, and add a kittest with a synthetic pointer press over a button beneath a just-raised overlay.
- [SUGGESTION] In `src/ui/components/progress_overlay.rs`:736-788: Carried-forward: keyboard escape enqueues an action with no rendered owner button
  Verified STILL VALID at 327b6033. The docstring on `with_keyboard_escape` (progress_overlay.rs:290-291) explicitly states: "Designating an action id that no button carries is a no-op." The `claim_input` path at line 736 clones `top.keyboard_escape_action` and enqueues it via `push_overlay_action` on Enter/Space (lines 758-760, 786-787) based only on `escape_action.is_some()` and `!*repeat`, without checking whether any `OverlayButton` in `top.buttons` carries that action id. Under the prior focus-based activation this contract was enforced implicitly — focus could only be granted to a rendered button whose id matched the escape. The SEC-001/002 refactor moved activation to frame-start and dropped that implicit guard. Consequence: a caller that typos the escape id, removes the matching button while leaving the designation, or builds the entry conditionally silently dispatches an invisible keyboard-only action that no button advertises — and an owner screen drains an action it never offered. Either filter `escape_action` against the rendered button set at the same point it is cloned, or update the docstring to reflect the new "always-on" semantics. The filter is a one-line, behaviour-preserving change.
- [SUGGESTION] In `src/context/connection_status.rs`:268-284: begin_spv_stop bypasses the SPV status watch channel
  Verified at 327b6033. The latest delta introduces `spv_status_tx: watch::Sender<SpvStatus>` (connection_status.rs:60) as an event-driven mirror of the atomic, consumed by `mcp::resolve::ensure_spv_synced` (mcp/resolve.rs:147-163) via `subscribe_spv_status().borrow_and_update()`. `set_spv_status` (lines 223-235) keeps the atomic and the watch coherent via `send_if_modified`, but `begin_spv_stop` (lines 268-284) writes the atomic directly with `compare_exchange` and returns without updating the watch. There is a window where `spv_status()` returns `Stopping` but a freshly-subscribed receiver's first `borrow_and_update()` still sees the previous `Running` value. The next `set_spv_status` call (e.g. from a teardown event handler) closes the gap, but a wallet-facing MCP call that subscribes during that window passes the sync gate while teardown has already been claimed. The two state-stores need a single coherent mutation path: mirror the transition into the watch the same way `set_spv_status` does.

Inline posting was rejected by GitHub with HTTP 422 after the PR advanced, so I posted the same verified findings as a body-only review for the assigned commit.

[thepastaclaw]

Code Review

Latest head 8fefb7e fixes the prior post-onboarding SPV auto-start arming gap: try_auto_start_spv now takes &mut self and sets spv_block_armed = true / spv_overlay_dismissed = false before spawning, with a dedicated kittest (fspv_a_onboarding_auto_start_arms_spv_block) locking the real path. Two prior overlay findings remain carried-forward and unaddressed in source: the TODO(SEC-002-pointer) at progress_overlay.rs:789-791 confirms pointer events are still not claimed at frame start (the set_sublayer change addresses z-order, not raise-frame pointer stripping), and claim_input still enqueues keyboard_escape_action on Enter/Space without validating against top.buttons, violating the documented no-op contract for the generic API. The new persister-release barrier and watch-channel ensure_spv_synced waiter in the latest delta look correct and adequately tested. One low-priority latest-delta nitpick remains: the persister-release barrier still hardcodes platform-wallet.sqlite separately from WalletBackend::new, so the comment's “no hardcoded path, no drift” claim is not quite true; a single helper for the full persister path would avoid future silent drift.

💬 1 nitpick(s)

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

2 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.