fix(ui): suppress disconnected banner during onboarding - #907
Conversation
Replace the plain radio-button row for Default/Expert/Developer view with square cards matching the Create Wallet / Import Wallet / Just Explore cards below, each carrying an icon, title, and description. The selected role gets a Dash Blue border and highlighted fill. Navigation and role-persistence logic are unchanged. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018LpMLZCnc6KpUA2AeMvE7h
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018LpMLZCnc6KpUA2AeMvE7h
Welcome-screen experience-level cards lost their radio-button a11y metadata when redesigned from plain radio buttons into cards. Each card now reports AccessKit RadioButton role + selected state via WidgetInfo::selected, and the kittest suite queries by role instead of label to catch regressions. Co-Authored-By: Codex Sol <noreply@openai.com>
On a cold start the Welcome/onboarding screen showed a red "Disconnected — check your internet connection" banner before the user did anything. `OverallConnectionState::Disconnected` is `ConnectionStatus`'s default atomic state, so on first launch — no wallet, no sync attempted — the reconciler read "hasn't started yet" as "can't connect". Thread `onboarding_active` (the existing `show_welcome_screen` signal) into `ConnectionBanner::update` and suppress the `Disconnected` banner while the Welcome screen is showing, mirroring the existing `spv_overlaying` branch: clear any handle, still advance `previous_state`, and return without setting a global banner. Once onboarding completes and SPV starts, the state leaves `Disconnected` and the normal machine resumes — every other state arm is untouched. Adds a kittest regression asserting the banner is absent on a fresh cold start. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 55 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the 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 configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
|
🕓 Ready for review — 26 ahead in queue (commit 9e65baf) |
The onboarding-suppression branch in ConnectionBanner::update advanced previous_state to the suppressed Disconnected value before returning, mirroring the pre-existing spv_overlaying branch. Unlike spv_overlaying (always transient — the SPV block resolves in bounded time), a Disconnected connection can persist indefinitely (auto_start_spv off, or a genuinely offline machine). Once previous_state was pinned to Disconnected during suppression, the very next frame's state_changed check went false and hit the early-return fast path before the onboarding check was even reached — permanently hiding the real Disconnected banner for the rest of the session, directly contradicting this PR's own CHANGELOG promise that "real connection problems are still reported afterwards." Fix: don't advance previous_state while suppressing for onboarding. Disconnected isn't a transient state like Connecting/Syncing, so leaving it unrecorded means state_changed stays true until a real state transition or the onboarding flag itself changes, letting the banner correctly reappear once onboarding ends while still disconnected. Found by the QA reviewer (MEDIUM) during PR #907 grumpy-review; independently flagged LOW by the security reviewer for the same root cause. Added a reconciler-level regression test driving three frames (suppressed, still-suppressed, onboarding-ended-still-disconnected) that failed before this fix. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
@/tmp/pr907-comment.md |
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The PR correctly suppresses the misleading cold-start Disconnected banner and fixes the prior bug that hid a persistent Disconnected state after onboarding. However, the suppression branch leaves a previously recorded non-Disconnected state intact, so a Syncing → Disconnected → Syncing transition during onboarding clears the Syncing banner and prevents it from returning. A focused verifier regression reproduced the failure, and resetting previous_state to None fixed both this sequence and the PR's existing post-onboarding regression test.
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 (failed),gpt-5.6-sol— rust-quality (failed),gpt-5.6-sol— general (failed),gpt-5.6-sol— rust-quality (failed),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)
🔴 1 blocking
🤖 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/reconcilers.rs`:
- [BLOCKING] src/app/reconcilers.rs:303-307: Clear stale previous state when suppressing Disconnected
This branch clears the active banner without clearing previous_state. If a non-Disconnected state was previously reconciled, a temporary DAPI outage can produce Disconnected during onboarding, clear that state's banner here, and then return to the recorded state. The unchanged-state fast path at lines 271-273 then returns without restoring the banner. This is reachable when the feature-gated MCP server starts SPV through the AppContext shared with the GUI while onboarding is still open; ConnectionStatus::refresh_state also explicitly maps temporary DAPI unavailability to Disconnected. A focused verifier test reproduced Syncing → Disconnected → Syncing leaving handle as None. Set previous_state to None during suppression so every state after the suppressed interval is reconciled; this also preserves the new behavior that shows a persistent Disconnected state immediately after onboarding ends.
- Reset previous_state when suppressing Disconnected during onboarding so a recurring Error/Connecting state after suppression is restored instead of silently swallowed by the unchanged-state fast path. - Bound onboarding Disconnected suppression to 120s from banner construction so a stuck onboarding flag (e.g. a failed update_onboarding_completed persist) cannot hide real connectivity problems indefinitely. Addresses PR #907's two blocking review findings (CODE-001, CODE-002) and a matching GitHub bot (thepastaclaw) review thread. Co-Authored-By: Codex Sol <noreply@openai.com>
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The latest delta fixes prior finding 5b07736d88d2 by invalidating previous_state during onboarding suppression and adding a regression test for recurring states. However, the new 120-second limit reintroduces the PR's original misleading Disconnected banner while the Welcome screen is still legitimately active, contradicting both the authoritative onboarding signal and the changelog promise that suppression lasts until onboarding finishes. No other in-scope findings were confirmed.
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 (failed),gpt-5.6-sol— rust-quality (failed),gpt-5.6-sol— general (failed),gpt-5.6-sol— rust-quality (failed),gpt-5.6-sol— general (failed),gpt-5.6-sol— rust-quality (failed),gpt-5.6-sol— general (failed),gpt-5.6-sol— rust-quality (failed),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)
🔴 1 blocking
🤖 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/reconcilers.rs`:
- [BLOCKING] src/app/reconcilers.rs:298-300: Suppress Disconnected for the entire onboarding session
The elapsed-time condition stops suppression 120 seconds after ConnectionBanner is constructed even when onboarding_active remains true. The Welcome screen has no automatic timeout and remains active until the user selects an onboarding path, so a user who spends more than two minutes reading or deciding will see the red "Disconnected — check your internet connection" banner while the connection state is still its untouched cold-start default and no sync has been attempted. This directly restores the behavior the PR is intended to remove and contradicts the changelog statement that the banner stays hidden until onboarding finishes. A persistence failure does not justify this timer: selecting a path sets show_welcome_screen to false synchronously through AppAction::OnboardingComplete even if update_onboarding_completed fails, with any persistence problem affecting only a later launch. Use onboarding_active as the suppression lifetime and remove the timer field, constant, and expiry test.
# Conflicts: # CHANGELOG.md # src/ui/welcome_screen.rs # tests/kittest/welcome_screen.rs
…lution `git checkout --theirs tests/kittest/welcome_screen.rs` during the origin/v1.0-dev merge replaced the whole file instead of just the conflicted hunk, silently dropping welcome_screen_suppresses_disconnected_banner (added on this branch, not present upstream). Re-add it after the adopted upstream role-card test suite. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
At exact HEAD a5f374d, the prior blocking finding remains valid: the hard-coded 120-second limit can expose the misleading Disconnected banner while onboarding is still legitimately active. The latest delta leaves the reconciler unchanged and restores only immediate-start kittest coverage, while the current unit test explicitly codifies the incorrect post-timeout behavior; no additional in-scope findings were confirmed.
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 (failed),gpt-5.6-sol— rust-quality (failed),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)
🔴 1 blocking
1 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
Carried forward: the prior blocking finding remains valid because the 120-second limit can expose the misleading Disconnected banner while onboarding is still active. Latest delta: the v1.0-dev merge does not modify the reconciler or fix this issue, and no new findings were confirmed.
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 (failed),gpt-5.6-sol— rust-quality (failed),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)
🔴 1 blocking
1 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
Suppress the disconnected banner for the full Welcome flow so the UI matches the documented onboarding lifecycle. Reconcile it immediately when onboarding ends. Co-Authored-By: Codex GPT-5 <noreply@openai.com>
|
Pushed a fix for the onboarding-suppression/CHANGELOG contradiction: the disconnected banner is now suppressed for the full Welcome/onboarding flow (no more 120s cap), matching the documented behavior, and lifts immediately once onboarding ends. Commit: 🤖 Co-authored by Claudius the Magnificent AI Agent |
Brings in: shutdown fix (dashpay#905), duplicate-DPNS-name error message (dashpay#915), startup banner clearing (dashpay#916), nav pointer cursor + tooltips and wallet-less masternode indication (dashpay#917), onboarding disconnected- banner suppression (dashpay#907), masternode dialog/nav/passphrase fixes (dashpay#913), DAPI auto-refresh during pre-1.0 migration (dashpay#908), "Add Receiving Address" wiring + its test hardening (dashpay#914, dashpay#920), and a CI timeout bump (dashpay#912). dashpay#906 (shielded re-enable) was already pulled in individually last session, so its squashed commit merged as a no-op. Conflicts (6 files) were rebrand-naming overlaps (dash_evo_tool:: vs orchardpay:: imports) plus one real merge in left_panel.rs, where OrchardPay's green-icon tint had to combine with upstream's new nav tooltip. Also fixed 5 files upstream's auto-merged (non-conflicting) additions left un-rebranded: a stray DASH_EVO_DATA_DIR_LOCK/env-var name in a new app.rs test, and dash_evo_tool:: references in three kittest test files. Added tooltip strings for OrchardPay's own nav entries (OrchardPay, DashPay) so the new every_nav_entry_has_a_tooltip test covers them — upstream's version only knows its own nav items. Fixed the new nav_label_hover_shows_pointer_cursor kittest test: OrchardPay's nav rail carries two more always-visible entries than upstream's, pushing "Settings" below the scrollable list's default-size visible viewport; scroll it into view first, matching what a real user would do. Verified: cargo check (both feature modes), cargo clippy --all-features --all-targets -- -D warnings, cargo fmt --all, cargo test --all-features --workspace (2066 lib + 257 kittest + doc tests, 0 failed), all clean. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Why this PR exists
OverallConnectionState::DisconnectedisConnectionStatus's default atomic state. On cold start there is no wallet and SPV has not been asked to connect, so the state simply hasn't left its initial value yet — the reconciler reads "hasn't started" as "can't connect" and raises an alarming, actionable-looking error banner the user cannot do anything about (there's nothing to retry — nothing was ever attempted).What was done
onboarding_active: boolparam intoConnectionBanner::update(src/app/reconcilers.rs), fed from the existingAppState.show_welcome_screensignal at the call site insrc/app.rs.onboarding_active && current_state == OverallConnectionState::Disconnected, the banner is suppressed: any existing handle is cleared,previous_statestill advances (so state-change bookkeeping never desyncs), and no banner is set — mirrored on the existingspv_overlayingsuppression branch just above it.src/ui/welcome_screen.rsrendering/layout was not touched — this is a global-banner-reconciler fix, not a screen-local one.Testing
welcome_screen_suppresses_disconnected_banner: builds a freshAppState(cold start naturally beginsDisconnected), asserts the welcome screen is showing and the "check your internet connection" text is absent. Confirmed RED before the fix (banner present, test failed) and GREEN after.cargo clippy --all-features --all-targets -- -D warnings— clean.cargo +nightly fmt --all,cargo fmt --all— clean.Breaking changes
None.
Checklist
CHANGELOG.mdupdated (Unreleased → Fixed)docs/user-stories.md— not touched; this is a bug fix to banner display, not a new feature (consistent with fix(wallet): surface DAPI endpoint exhaustion #904's precedent for this repo's convention)Attribution
🤖 Co-authored by Claudius the Magnificent AI Agent