Repository navigation
fix(identity): let users load an identity from the Identities page - #1042
Conversation
Bring over the generic toolbar dropdown infrastructure from #1028 (ToolbarMenuItem and DesiredAppAction::Menu, rendered as a popup by the top panel) so other screens can group actions behind one button. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Users with identities already loaded had no way to reach "Load an existing identity" from the picker: the add card promised create or load but only opened creation, and the identity pill's dropdown needs a selected identity. Add an "Add" dropdown to the Identities top bar (create, load, and the Power-user test-identities entry, matching the pill's dropdown) and make the "Add a new identity" card open the same choices. Both route through the hub's existing breadcrumb add effects, so creation keeps the selected wallet preselected. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 15 minutes. View limit detailsLimit details: You’ve used the included review currently available. Review configuration: ⚙️ Run configurationConfiguration used: Repository: dashpay/dash-evo-tool/.coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (14)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: dashpay/dash-evo-tool/.coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (3)
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe Identities page now offers create and load actions in both the top-bar Add menu and the add card. A shared popup helper renders toolbar menus, and the hub routes the selected identity action. ChangesIdentity creation and loading
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Bug fix Sequence Diagram(s)sequenceDiagram
participant User
participant TopPanel
participant IdentityPicker
participant IdentityHubScreen
participant IdentityScreen
User->>TopPanel: select a create or load menu item
TopPanel->>IdentityHubScreen: dispatch selected action
User->>IdentityPicker: click the add card and select a menu item
IdentityPicker->>IdentityHubScreen: return the selected command
IdentityHubScreen->>IdentityScreen: route to the matching identity screen
Suggested reviewers: Merge Risk: ⚪ Minimal · up to This change adds create and load identity menus to the Identities page. No merge-blocking risk was identified in the reviewed changes. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The new menus open existing identity workflows rather than performing privileged operations directly. Wallet and input checks remain in those workflows. No introduced security concern was identified, but downstream coverage is not complete. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
🚥 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 9e7f6fb) · triage: normal · Phase 2 only (no Phase 1 for this repository) |
No bulk-creation screen exists, so the entry only opened the single identity creation screen. Remove it from the Add menu and the identity pill dropdown until IDH-005 is implemented. Also route the picker scroll test through the Add card's new create/load menu. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…t/identity-add-menu
There was a problem hiding this comment.
Verdict: approve — no blocking findings; a clean identity-picker/Add-menu refactor with the call-tree fully traced and nothing broken, just two MEDIUM DRY/doc-accuracy nits and some LOW hygiene worth a follow-up.
Deferral candidates (flagged out_of_scope_follow_up, acceptable to leave but not filed anywhere, so naming them here): RUST-004 (pre-existing Screen::AddNewIdentityScreen → ScreenType::AddExistingIdentity copy-paste bug, untouched by this PR) and RUST-006 (the disabled-menu-item test checks is_disabled() but never clicks it to prove the click is a no-op).
2 finding(s) posted: 2 inline, 0 in this body.
🤖 Co-authored by Claudius the Magnificent AI Agent
📊 View full HTML review report
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Phase 2 only (no Phase 1 for this repository)
The implementation correctly exposes create and load actions through the identity hub and picker card, and the supplied CI checks pass. One in-scope test-coverage gap remains: the regression test verifies only screen variants and does not prove that either new Create entry preserves a non-first selected wallet.
🟡 1 suggestion(s)
Review provenance
Source: reviewer 1: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
normalbygpt-6.1-sol(effort low) — This is a cross-cutting UI behavior change spanning navigation, shared menu infrastructure, identity picker routing, documentation, and integration tests, but it does not affect critical surfaces such as funds, cryptography, networking, or storage. - Phase 1 reviewers: not run (disabled for this repository)
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort high); agentphase2-reviewer
🤖 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 `tests/kittest/identity_hub_switcher.rs`:
- [SUGGESTION] tests/kittest/identity_hub_switcher.rs:1169-1178: Protect selected-wallet preselection through both new Create entries
This test seeds identities without exercising wallet selection, tests only the toolbar menu, and asserts only the pushed screen variant. That cannot distinguish the intended `AddNewIdentityScreen::new_with_wallet` route from the default constructor path that selects the first available wallet. The selected-wallet preservation is the reason for the hub's custom command routing, and the picker card uses that same routing. Add a regression with two HD wallets, select the wallet that is not `first_hd()`, and exercise Create from both the toolbar menu and the picker-card menu; assert that the creation screen's wallet chooser displays the selected wallet.
|
Your move: thepastaclaw left review threads unresolved; resolve them. |
Clarify the identity menu documentation, consolidate four popup renderers, and assert selected-wallet preservation through toolbar and picker creation. Co-Authored-By: OpenAI Codex <noreply@openai.com> <sub>🤖 Co-authored by [Claudius the Magnificent](https://github.com/lklimek/claudius) AI Agent</sub>
There was a problem hiding this comment.
Clean refactor, no blockers — one MEDIUM cosmetic regression (shared menu button lost its full-width sizing) plus a handful of LOW nits on API shape, menu-definition duplication, and a couple of docs that didn't get the memo. Ship it, then tidy up.
1 finding(s) posted: 0 inline, 1 in this body.
Findings outside the diff or deferred
- RUST-001 · MEDIUM — Extracting show_action_menu() dropped the full-width min_size on dropdown buttons —
src/ui/components/action_menu.rs:100-104
Nice extraction, shame about the ruler. show_action_menu() consolidated three copy-pasted menu builders into one — genuinely good — but it also quietly dropped the .min_size(ui.available_width(), 0.0) call the old Documents/Contracts buttons had. Now every menu that flows through this component (including the two it wasn't even trying to change) renders ragged, label-width buttons in a still-150px popup instead of full-width rows. Not dangerous, just a real, visible step backward in two menus nobody asked you to touch. Put the sizing back on the shared button and nobody will notice you were ever here.
🤖 Co-authored by Claudius the Magnificent AI Agent
📊 View full HTML review report
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 2 only (no Phase 1 for this repository)
The PR correctly exposes identity creation and loading from both the Identities toolbar and picker card, preserves selected-wallet preselection, and adds focused coverage for these flows. The shared action-menu extraction introduced a usability regression: menu buttons no longer fill the popup width, so only their text-sized area is clickable.
🟡 1 suggestion(s)
Review provenance
Source: reviewer 1: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
normalbygpt-6.1-sol(effort low) — This is a cross-cutting UI change spanning toolbar components, identity navigation, app action routing, documentation, and integration tests, but it does not modify critical surfaces such as funds movement, cryptography, consensus, networking, or storage. - Phase 1 reviewers: not run (disabled for this repository)
- Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— general (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort high); agentphase2-reviewer
🤖 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/components/action_menu.rs`:
- [SUGGESTION] src/ui/components/action_menu.rs:21-24: Preserve full-width hit targets in shared action menus
The previous document and contract popup implementations gave each button a minimum size equal to the popup's available width. The shared renderer now uses only the button's intrinsic size, while `ui.set_min_width(150.0)` affects the popup layout but does not expand the button's interaction rectangle. As a result, the existing Documents and Contracts menus, as well as the new Add menu, have a substantially smaller clickable area than before. Preserve the full-width button hit target when rendering each menu item.
|
Your move: thepastaclaw left review threads unresolved; resolve them. |
There was a problem hiding this comment.
No security issues, no broken callers, no blockers — just a tidy identity-picker refactor with a couple of loose threads (an unconditional Add menu duplicating Onboarding's CTAs, and a silently-removed dev shortcut) worth a follow-up, plus some documentation drift. Approving with comments.
2 finding(s) posted: 1 inline, 1 in this body.
Findings outside the diff or deferred
- RUST-001 · MEDIUM — Power-user "Create multiple test identities" entry removed from the identity-pill dropdown with no changelog entry —
src/ui/components/global_nav_switcher.rs:153-163
Removing the Power-role "Create multiple test identities" shortcut is a perfectly reasonable cleanup now that the Add menu covers create/load explicitly — but the changelog entry only brags about what you added, not what quietly vanished. A Power-role user reaching for a muscle-memory menu item deserves one line telling them where it went.
🤖 Co-authored by Claudius the Magnificent AI Agent
📊 View full HTML review report
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 2 only (no Phase 1 for this repository)
Independently reviewed the complete PR diff at 9e7f6fb and found no actionable in-scope issues. Both new entry points reuse the existing create/load routing and preserve selected-wallet preselection; the menu hit-target issue is fixed, and the previously requested wallet regression coverage already existed. Validation was static only: the supplied CI snapshot at 2026-10-02T10:23:07Z reports Test Suite, Clippy, migration, and det-cli build checks passing, with PR Hygiene pending.
🔴 0 blocking | 🟡 0 suggestion(s) | 💬 0 nitpick(s)
Review provenance
Source: reviewer 1: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
normalbygpt-6.1-sol(effort low) — The change adds shared toolbar and card menus, identity navigation routing, theme adjustments, and UI tests across multiple files, but does not modify backend logic or any critical surface. - Phase 1 reviewers: not run (disabled for this repository)
- Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— general (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort high); agentphase2-reviewer
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify the current code and confirm that no unresolved issues remain.
No unresolved findings remain from the prior review on this head.
|
Bots are done — your move: address claudbot left a review thread unresolved, then post |
|
/self-reviewed |
TL;DR
The Identities page now has an Add ▾ button with "Create a new identity" and "Load an existing identity". The "Add a new identity" card offers the same two choices. Before this, a user who already had identities had no visible way to load another one, for example to search a restored wallet for its identities.
User story
As a user who already has identities in the app, I want to load an existing identity from the Identities page, so that I can bring back identities that belong to my wallet without hunting for a hidden menu.
Scenario
Steps to reproduce
Actual behavior
The "Add a new identity" card says "Create a new identity or load one you already own." but only opens identity creation. The only "Load an existing identity" entry is in the identity dropdown in the top navigation bar, which does not open while no identity is selected.
Expected behavior
The top bar has an Add ▾ menu, and the card opens a small menu. Both offer creating a new identity and loading an existing one.
Detailed discussion
feat(ui): add toolbar dropdown menu support) brings in the generic toolbar dropdown infrastructure from fix(wallet): surface unresolved Platform funding transfers #1028 (commit fbc2762):ToolbarMenuItem,DesiredAppAction::Menuinsrc/app.rs, and its rendering plus test insrc/ui/components/top_panel.rs. Applied byte-identical, so fix(wallet): surface unresolved Platform funding transfers #1028 should merge over it without conflicts. The wallet-specific parts of fix(wallet): surface unresolved Platform funding transfers #1028 are not included.breadcrumb_switcher::add_identity_menu_items(role)is the single item list, shared by the toolbar and the card. It matches the identity pill's dropdown.AppAction::Customcommands. The hub maps them back to the existingBreadcrumbEffectadd actions, so all three entry points use one routing path.Customis used instead ofAddScreenTypebecause the hub's create path preselects the currently selected wallet.ScreenType::AddNewIdentitywould pick the first one.IdentityPickerAddCardResponsegains arectfield, used to place the card's popup.docs/user-stories.mdgains an acceptance criterion saying load is reachable from the Add menu and the card.Testing
add_menu_offers_create_and_load,add_menu_actions_map_to_hub_effects,add_card_offers_create_and_load_in_both_themes.advanced_menu_opens_dispatches_and_closes_in_both_themes.picker_top_bar_add_menu_creates_or_loads_an_identity. With two identities seeded, Add ▾ → Load opens the load screen and Add ▾ → Create opens the create screen.cargo test --test kittest --all-featurespasses (366).ui_polish_many_identities_picker_scroll_reaches_add_cardnow goes through the card's create/load menu.cargo clippy --all-features --all-targets -- -D warningsis clean.🤖 Generated with Claude Code
🤖 Co-authored by Claudius the Magnificent AI Agent
PR Hygiene ·
9e7f6fbWhen every box is checked the
PR Hygienecheck passes and this can merge.Summary by CodeRabbit