feat(ui): redesign welcome-screen experience-level selector as cards - #903
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
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe Welcome screen experience-level picker now uses three selectable cards instead of radio buttons. Each card displays a role-specific icon, label, and description, while adaptive layouts, accessibility coverage, and onboarding action-card rendering were updated. ChangesWelcome picker redesign
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant WelcomeScreen
participant UserRole
participant AppState
User->>WelcomeScreen: Click role card
WelcomeScreen->>UserRole: Read role icon and details
UserRole-->>WelcomeScreen: Return role presentation values
WelcomeScreen->>AppState: Persist changed role
WelcomeScreen-->>User: Render selected card state
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
|
✅ Final review complete — no blockers (commit cd20b20) |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The role-card redesign preserves selection and persistence behavior, but it replaces native radio buttons with a custom clickable frame that does not expose a radio role, accessible name, or selected state. This is an in-scope accessibility regression for assistive-technology users and should be fixed before merging.
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 (completed),gpt-5.6-sol— rust-quality (failed),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/ui/welcome_screen.rs`:
- [BLOCKING] src/ui/welcome_screen.rs:202-206: Restore radio-button accessibility semantics on role cards
The previous `ui.radio_value` controls registered each experience-level option as a labeled `RadioButton` and exposed its selected state. The replacement only adds click sensing to the frame response; egui therefore creates an unnamed node with click handling but no radio role or selected-state metadata, while the visible title remains a separate noninteractive label. Assistive-technology users cannot identify these cards as a mutually exclusive selector or determine which role is selected. The existing kittest does not catch this because `get_by_label(...).click()` targets the child label's screen coordinates, and the overlapping frame receives the resulting pointer click without requiring correct accessibility semantics.
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>
lklimek
left a comment
lklimek
left a comment
There was a problem hiding this comment.
Automated grumpy-review — consolidated findings
Reviewed at 7beec7be (re-anchored from the originally-reviewed 6811617e after the accessibility fix landed).
What's good: cargo test --test kittest and cargo clippy --all-features -D warnings are clean against this head. The redesign reuses the existing label()/description() model accessors rather than inventing UI-local strings, so the i18n-ready-strings rule holds. role_icon() follows the same exhaustive-match idiom as the enum's sibling accessors and is unit-tested. No security-relevant paths touched: no new unwrap/expect, no new unsafe, no I/O, no user-controlled input (role_icon() returns &'static str off a closed enum). No function signature was modified or removed, so no call-tree risk. Keeping render_role_card private to welcome_screen.rs correctly follows the component-placement policy.
Findings: 8 posted — 2 worth fixing before merge (🟡), 6 minor follow-ups (🟢).
Adjusted after re-review of 7beec7be:
- The AccessKit/
WidgetInfo::selectedhalf of the accessibility finding is already fixed byfix(ui): restore radio-button accessibility semantics on role cards, which also matches the existing (now outdated) thread from @thepastaclaw — deferring to that thread, not re-raising. The keyboard-focus-indicator half is a distinct gap that commit did not address, so it is posted separately. - The selected-state half of the test-coverage finding is now covered by the new
welcome_role_cards_expose_radio_accessibility_statetest; that finding is narrowed to the still-unasserted description text.
This is an advisory review, not a merge decision — hence COMMENT rather than request-changes.
🤖 Co-authored by Claudius the Magnificent AI Agent — automated grumpy-review (Claude trio-equivalent + Codex Sol)
| egui::Stroke::new(2.0, DashColors::DASH_BLUE) | ||
| } else { | ||
| egui::Stroke::new(1.0, DashColors::border_light(dark_mode)) | ||
| }; |
There was a problem hiding this comment.
🟡 Role cards paint no keyboard-focus indicator (WCAG / ux-design-patterns.md §10)
Note: the AccessKit/
WidgetInfo::selectedhalf of this finding is already fixed by7beec7be(and was raised in @thepastaclaw's now-outdated thread). This comment covers only the remaining, separate gap: the visual focus indicator.
docs/ux-design-patterns.md §10 states as project policy (line 170): "Focus indicator: BORDER_WIDTH_THICK, WCAG 3:1 contrast ratio" for all interactive elements.
The fill/stroke above are computed purely from the selected: bool parameter — neither reads response.has_focus() nor goes through ui.style().interact(). The only interaction feedback painted is a PointingHand cursor on .hovered().
This is a regression from the removed ui.radio_value(): verified in egui 0.35.0 source (widgets/radio_button.rs:81, let visuals = *ui.style().interact(&prepared.response);) that the old widget derived its colors from Style::interact(), which returns the highlighted WidgetVisuals when response.has_focus() is true — tabbing to a radio button visibly highlighted it. The cards remain keyboard-operable (Space/Enter fire the Sense::click() response), but a keyboard-only user tabbing through the welcome screen now gets zero visual cue of which card has focus.
Recommendation: read response.has_focus() — or use ui.style().interact_selectable(&response, selected) — when choosing fill/stroke, so a distinct focus ring paints independently of the selected state (a focused-but-unselected card must be visually distinguishable from an unfocused one). Add a kittest assertion covering Tab-focus.
🤖 Co-authored by Claudius the Magnificent AI Agent — automated grumpy-review (Claude trio-equivalent + Codex Sol)
There was a problem hiding this comment.
Added a distinct keyboard-focus ring to each role card while retaining the existing AccessKit radio-selected state. Fixed in cd20b20c.
🤖 Co-authored by Claudius the Magnificent AI Agent
| let card_visual_width = 170.0 + (Spacing::MD * 2.0) + 2.0; | ||
| let total_width = (card_visual_width * 3.0) + (card_spacing * 2.0); | ||
|
|
||
| ui.allocate_ui(Vec2::new(total_width, 140.0), |ui| { |
There was a problem hiding this comment.
🟡 Fixed-width card row is clipped at narrow window widths, with no scroll fallback
The selector always requests a fixed ~644px row (card_visual_width = 170.0 + Spacing::MD*2.0 + 2.0; total_width = card_visual_width*3.0 + card_spacing*2.0, reserved via ui.allocate_ui), while the welcome screen wraps its content in ScrollArea::vertical() only (line 38) and the native viewport sets no minimum width — src/main.rs's ViewportBuilder::default() has no with_min_inner_size(...), yet persist_window: true is set.
Because window dimensions persist across launches, resizing below roughly 700px — or simply reopening a window that was previously resized narrow — leaves one or more role cards partially or fully clipped, with no horizontal scroll to reach them. On a first-run onboarding screen, that's a broken journey, not a speculative edge case.
Secondary: the selected card's 2px stroke vs. the unselected 1px stroke isn't accounted for by the flat +2.0 in the width formula, so the reserved width is already slightly off from the actual painted geometry.
This conflicts with docs/ux-design-patterns.md §12: "Use ui.available_width() for adaptive layouts".
Recommendation: derive the row layout from ui.available_width() (horizontal wrap, or stack vertically below a threshold) instead of a fixed pixel total, and keep selected/unselected cards at identical outer geometry regardless of stroke width. Add a narrow-window kittest case asserting all three roles stay visible and clickable.
🤖 Co-authored by Claudius the Magnificent AI Agent — automated grumpy-review (Claude trio-equivalent + Codex Sol)
There was a problem hiding this comment.
Both card rows are now adaptive at narrow window widths instead of clipping at a fixed ~644px. Fixed in cd20b20c.
🤖 Co-authored by Claudius the Magnificent AI Agent
| } | ||
| let card_spacing = 16.0; | ||
| let card_visual_width = 170.0 + (Spacing::MD * 2.0) + 2.0; | ||
| let total_width = (card_visual_width * 3.0) + (card_spacing * 2.0); |
There was a problem hiding this comment.
🟢 Card-row width math is hardcoded to exactly 3 cards, decoupled from the iterated role list
The literal 3.0/2.0 card-count constants here aren't derived from the iterated array's length or any shared constant — unlike role_icon()/label()/description(), whose exhaustive match arms force a compile error the moment a 4th UserRole variant is added. Nothing forces total_width's 3.0 to track the loop's actual element count, so a future 4th role would compile cleanly but silently reserve too little width.
The identical 3-line formula already exists in the pre-existing render_getting_started_section for its fixed 3 action cards; this PR duplicates it verbatim into a second location, so the same magic numbers (170.0, 16.0, 2.0) are now copy-pasted twice.
Recommendation: extract a shared width-calculation helper parameterized by card count — or better, derive it from ui.available_width()/a wrapping layout so no hardcoded count is needed at all (which would also resolve the clipping finding) — and reuse it from both call sites.
🤖 Co-authored by Claudius the Magnificent AI Agent — automated grumpy-review (Claude trio-equivalent + Codex Sol)
There was a problem hiding this comment.
Layout counts are now derived from the card arrays via a shared helper instead of hardcoded 3-card literals. Fixed in cd20b20c.
🤖 Co-authored by Claudius the Magnificent AI Agent
| let response = egui::Frame::new() | ||
| .fill(fill) | ||
| .stroke(stroke) | ||
| .corner_radius(Shape::RADIUS_LG) |
There was a problem hiding this comment.
🟢 New role cards copy the pre-existing RADIUS_LG-for-cards mismatch instead of correcting it
docs/ux-design-patterns.md's Border (Shape) table (lines 49-50) maps RADIUS_MD → "Buttons, cards, inputs" and RADIUS_LG → "Island panels" specifically.
This uses Shape::RADIUS_LG, matching the pre-existing — and already non-conforming — render_action_card (unmodified by this PR). Adding a second card-drawing function was an opportunity to align with the documented token table; instead the divergence now has a second call site, widening the doc/code mismatch rather than narrowing it.
Recommendation: either fix both card renderers to use Shape::RADIUS_MD per the documented table, or update the Border table in docs/ux-design-patterns.md if RADIUS_LG is genuinely the intended token for onboarding cards. Either way, doc and code should agree.
🤖 Co-authored by Claudius the Magnificent AI Agent — automated grumpy-review (Claude trio-equivalent + Codex Sol)
There was a problem hiding this comment.
Both card types now use RADIUS_MD, matching ux-design-patterns.md's Border table instead of perpetuating the RADIUS_LG mismatch. Fixed in cd20b20c.
🤖 Co-authored by Claudius the Magnificent AI Agent
| .shadow(Shadow::small()) | ||
| .inner_margin(Spacing::MD) | ||
| .show(ui, |ui| { | ||
| ui.set_min_size(Vec2::new(170.0, 100.0)); |
There was a problem hiding this comment.
🟢 Card styling bypasses design tokens with pixel literals
The new card implementation introduces hard-coded dimensions (170.0, 100.0), typography sizes (24.0, 14.0, 11.0), spacing (5.0, 6.0), and stroke widths rather than the semantic Spacing/Typography/Shape tokens that docs/ux-design-patterns.md calls for — and repeats the card-width formula already used by render_getting_started_section.
The duplicated, approximate border-width assumption baked into that formula has already drifted from the selected card's actual (thicker) stroke — see the clipping finding above for the concrete consequence of that drift.
Recommendation: define shared welcome-card geometry constants (or reuse existing tokens), replace the pixel literals with Spacing/Typography/Shape tokens, and share the row-width/layout helper between the role and action card groups.
🤖 Co-authored by Claudius the Magnificent AI Agent — automated grumpy-review (Claude trio-equivalent + Codex Sol)
There was a problem hiding this comment.
Replaced the pixel-literal card styling with design tokens and named geometry. Fixed in cd20b20c.
🤖 Co-authored by Claudius the Magnificent AI Agent
| ui.spacing_mut().item_spacing.x = card_spacing; | ||
|
|
||
| for option in [UserRole::Everyday, UserRole::Power, UserRole::Developer] { | ||
| if self.render_role_card(ui, dark_mode, option, role == option) { |
There was a problem hiding this comment.
🟢 Selecting a new role paints that card's selected-highlight one frame late
if self.render_role_card(ui, dark_mode, option, role == option) { role = option; } — the selected argument passed into render_role_card for the clicked iteration is evaluated from the pre-click role value, so the just-clicked card paints with its old (unselected) fill/stroke for the frame in which the click registers. The highlighted border only appears on the next repaint.
In practice that repaint is requested immediately via ui.ctx().request_repaint(), so the delay is a single, imperceptible frame at normal frame rates. Flagging it only because it is a genuine behavior difference from the removed ui.radio_value(), whose internal checked state is set and read back within the same paint call.
Recommendation: low priority given the imperceptible delay. If this code is ever revisited, resolve clicks first and render selected from the post-click role for exact same-frame parity with the old widget.
🤖 Co-authored by Claudius the Magnificent AI Agent — automated grumpy-review (Claude trio-equivalent + Codex Sol)
There was a problem hiding this comment.
Selection painting is now deferred so the highlight updates in the same click frame instead of one frame late. Fixed in cd20b20c.
🤖 Co-authored by Claudius the Magnificent AI Agent
| use egui_kittest::kittest::{NodeT, Queryable}; | ||
|
|
||
| #[test] | ||
| fn welcome_role_cards_expose_radio_accessibility_state() { |
There was a problem hiding this comment.
🟢 No test locks in the new always-visible three-description layout
Credit where due: this new test (added in 7beec7be) now pins the radio role and selected/Toggled state for all three cards simultaneously — which covers the selected-highlight half of what was originally flagged here.
What remains unasserted is the description text. This PR changed the role selector from "show only the selected role's description below the row" to "show all three roles' descriptions on their own cards, always" — but no test queries role.description() for any card. The PR description's "each card shows its own description inline" claim (and the CHANGELOG's "easier to compare at a glance") is therefore verified by nothing; a future regression that hides the descriptions again would sail through the suite green.
Recommendation: extend this test (or add a sibling) with an AccessKit-level assertion — e.g. harness.get_by_label(role.description()) exists for all three roles simultaneously — to pin down the always-visible-description behavior the CHANGELOG advertises.
🤖 Co-authored by Claudius the Magnificent AI Agent — automated grumpy-review (Claude trio-equivalent + Codex Sol)
There was a problem hiding this comment.
Added kittest coverage for focus state, the narrow-layout fallback, the description text, and same-frame selection. Fixed in cd20b20c.
🤖 Co-authored by Claudius the Magnificent AI Agent
|
|
||
| - **Welcome screen's experience-level picker is now three cards**: it matches | ||
| the Create Wallet / Import Wallet / Just Explore cards below it, with an | ||
| icon and a short description on each, and a highlighted border on the one |
There was a problem hiding this comment.
🟢 Wording implies the sibling action cards have icons, but they don't
The entry reads: "it matches the Create Wallet / Import Wallet / Just Explore cards below it, with an icon and a short description on each".
Verified by reading render_action_card (unmodified by this PR): those three cards render only a title and a description — no icon glyph. Read literally, the sentence groups "matches the ... cards below it" with "with an icon and a short description on each", which a reader can reasonably parse as claiming the referenced cards also carry icons. The two rows match in general chrome (Frame/border/shadow/padding), not in content composition.
Recommendation: reword to something unambiguous, e.g.: "now three cards, matching the visual style of the Create Wallet / Import Wallet / Just Explore cards below; each role card adds an icon and a short description, and the one you're on gets a highlighted border."
🤖 Co-authored by Claudius the Magnificent AI Agent — automated grumpy-review (Claude trio-equivalent + Codex Sol)
There was a problem hiding this comment.
Corrected the CHANGELOG wording — it no longer implies the sibling action cards have icons. Fixed in cd20b20c.
🤖 Co-authored by Claudius the Magnificent AI Agent
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Codex + Sonnet
At exact head 7beec7b, the latest commit correctly restores radio-button accessibility semantics on the welcome-screen experience-level cards. src/ui/welcome_screen.rs:202-208 now calls response.widget_info(|| WidgetInfo::selected(WidgetType::RadioButton, true, selected, role.label())) after .interact(egui::Sense::click()), which I independently confirmed against the vendored egui 0.35.0 source matches WidgetInfo::selected's real signature (typ, enabled, selected, label) at egui-0.35.0/src/data/output.rs:643, and WidgetType::RadioButton maps to accesskit Role::RadioButton at egui-0.35.0/src/response.rs:924. The new kittest welcome_role_cards_expose_radio_accessibility_state queries all three cards by Role::RadioButton and asserts correct Toggled state (Default view=False, Expert view=True matching the app's default UserRole, Developer view=False), and the pre-existing persistence test now activates the control via get_by_role_and_label(Role::RadioButton, ...) instead of a plain label query, which only proves the a11y role wiring is real. I ran cargo test --test kittest welcome --all-features and all 3 welcome_screen tests pass; cargo clippy --all-features --all-targets -- -D warnings and a full cargo build also pass cleanly. No cumulative concerns remain in the current-PR range (3b32bed..7beec7b); the one remaining reviewer note (duplicated card-sizing magic numbers) is a low-confidence suggestion/nitpick, not a defect, and does not block.
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:
claude-sonnet-5— final-verifier - Sonnet reviewers:
claude-sonnet-5— general (failed),claude-sonnet-5— rust-quality (failed),claude-sonnet-5— general (failed),claude-sonnet-5— rust-quality (failed),claude-sonnet-5— general (completed),claude-sonnet-5— rust-quality (completed),claude-sonnet-5— general (completed),claude-sonnet-5— rust-quality (completed)
Adds visible keyboard-focus rings (retaining AccessKit radio state), makes both card rows adaptive at narrow widths instead of clipping, derives layout counts from the card arrays instead of hardcoded constants, unifies card corner radius on RADIUS_MD, replaces pixel literals with design tokens/named geometry, defers painting so selection updates land in the same click frame, and adds focus/narrow-layout/description/same-frame kittest coverage. Also corrects a CHANGELOG line that implied action cards carry icons they don't. Addresses PR #903 review comments (human reviewer). Co-Authored-By: Codex Sol <noreply@openai.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Codex + Sonnet
Independent verification at head cd20b20 confirms all nine prior findings are genuinely fixed: role cards now expose RadioButton widget_info, keyboard focus paints a distinct stroke, layout adapts to narrow widths via a generic render_adaptive_card_rows helper driven by cards.len() (no hardcoded 3), styling uses Shape/Spacing/Typography tokens, selection paints in the same click frame, all three descriptions are always visible, and the CHANGELOG no longer implies action cards have icons. All six welcome_screen kittest tests pass locally. No new in-scope defects found; the only remaining gap (action-card accessibility semantics) predates this PR and is correctly tracked out of scope.
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:
claude-sonnet-5— final-verifier - Sonnet reviewers:
claude-sonnet-5— general (failed),claude-sonnet-5— rust-quality (failed),claude-sonnet-5— general (completed),claude-sonnet-5— rust-quality (completed)
…#903, dashpay#904) Pulled in 4 new upstream commits since the last sync (3b32bed): - dashpay#903 feat(ui): redesign welcome-screen experience-level selector as cards, with radio-button accessibility semantics. Only real conflict in this merge: tests/kittest/welcome_screen.rs, where both sides added tests — kept our create_wallet_path_shows_and_clears_onboarding_stepper test and pulled in upstream's 4 new card-accessibility tests (rebranded dash_evo_tool:: -> orchardpay:: in the process). - dashpay#904 fix(wallet): surface DAPI endpoint exhaustion — pins the exact Sdk generation a backend task ran against for error classification, fixing errors misattributed to a since-reloaded SDK. Touches backend_task/mod.rs, which our own pub mod orchardpay; addition also lives in; merged cleanly with no conflict. - dashpay#898 ci: allow manual test/clippy runs, cover rust-toolchain.toml. - dashpay#899 docs: record the local-vs-CI split in CLAUDE.md. Also corrects local fmt guidance from `cargo +nightly fmt` to plain `cargo fmt --all` (CI checks against the rust-toolchain.toml-pinned compiler, not nightly) — worth adopting going forward. One rebrand miss caught in the merged prose (`--bin dash-evo-tool` -> `--bin orchardpay`). All other files (CLAUDE.md, src/app.rs, src/backend_task/error.rs, src/model/user_role.rs, src/ui/dpns/dpns_contested_names_screen.rs, src/ui/welcome_screen.rs) auto-merged cleanly — none needed manual reconciliation beyond the one branding fix above. Verified: cargo build (both --all-features and default features), clippy -D warnings, cargo fmt --all -- --check, and the full test suite all green. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
3 participants
Why this PR exists
What was done
Framestyling tokens as the action cards below (corner radius, shadow, border, spacing, 170px width) — the two rows now read as one visual family.UserRole::role_icon()accessor: 👤 Default view, 🛠 Expert view, 💻 Developer view.role != previousdiff, sameset_and_persist_user_rolecall, same error-banner handling.Testing
cargo fmt --all,cargo +nightly fmt --allcargo clippy --all-features --all-targets -- -D warnings— cleancargo build --bin dash-evo-tool— cleancargo test --test kittest welcome_role_selector_sets_and_persists_role— pass (test file unmodified; still queries "Default view" by accessible label)cargo test --test kittest just_explore_lands_on_identities_hub— pass (unmodified)role_icons_match_each_experience_level— passBreaking changes
None.
Checklist
CHANGELOG.mdupdated (Unreleased → Changed)docs/user-stories.md— not touched; this redesigns the presentation of an existing selector, not a new feature (per repo convention, cosmetic-only changes skip user-story updates)Attribution
🤖 Co-authored by Claudius the Magnificent AI Agent
Summary by CodeRabbit