feat: add ResponseExt trait to enforce tooltip cursor policy#738
Merged
Conversation
Introduce `ResponseExt` extension trait on `egui::Response` with three methods — `info_tooltip` (Help cursor), `clickable_tooltip` (PointingHand), and `disabled_tooltip` (NotAllowed) — that combine tooltip text with the correct cursor icon in a single call. Migrate all 56 `.on_hover_text()` callsites across 31 files to use the new helpers, ensuring consistent cursor behavior project-wide. Update UX design patterns doc to reference the new API. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
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 (5)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds a public Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (10)
src/ui/tokens/set_token_price_screen.rs (1)
1043-1051:⚠️ Potential issue | 🟡 MinorAvoid using the Help-cursor tooltip on an editable input.
This
TextEditis interactive, soinfo_tooltip()gives it the wrong affordance. Moving the tooltip to the label would keep the cursor policy consistent without making the input look passive.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/tokens/set_token_price_screen.rs` around lines 1043 - 1051, The TextEdit for public_note is using info_tooltip(), which gives an incorrect Help-cursor affordance on an editable input; move the tooltip from the TextEdit to the label instead: update the ui.horizontal block that constructs the label and TextEdit (the ui.label("Public note (optional):") and the subsequent ui.text_edit_singleline(&mut txt) using self.public_note) so the label receives the info_tooltip(...) call and the TextEdit is rendered without info_tooltip(), preserving an editable cursor on the input while still exposing the explanatory tooltip on the label.src/ui/identities/keys/add_key_screen.rs (1)
559-569:⚠️ Potential issue | 🟡 MinorUse the disabled-tooltip variant on this locked selector.
This hover target represents an unavailable control, so
info_tooltip()gives it the wrong cursor.disabled_tooltip()would preserve the not-allowed affordance the new policy expects.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/identities/keys/add_key_screen.rs` around lines 559 - 569, The hover target for the locked security-level selector currently calls hover_response.info_tooltip(...), which gives the wrong cursor; change it to use hover_response.disabled_tooltip(...) so the control preserves the "not-allowed" affordance. Locate the block that checks has_multiple_security_levels and creates hover_response via ui.interact (using inner_response.response.rect and egui::Id::new("security_level_tooltip")), and replace the call to info_tooltip(format!("{:?} purpose requires {:?} security level", self.purpose, self.security_level)) with a disabled_tooltip call that passes the same formatted message.src/ui/tokens/mint_tokens_screen.rs (1)
615-623:⚠️ Potential issue | 🟡 MinorThis tooltip should not live on the text input itself.
info_tooltip()makes the public-noteTextEditadvertise the Help cursor instead of a text-entry cursor. Prefer attaching the tooltip to the label, so the field still reads as editable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/tokens/mint_tokens_screen.rs` around lines 615 - 623, The public-note tooltip is currently applied to the TextEdit (via text_edit_singleline(...).info_tooltip(...)) which forces a Help cursor and prevents normal text-entry cursor; move the tooltip to the label instead so the field remains editable: remove the info_tooltip call from the TextEdit for self.public_note and attach the explanatory tooltip to the label created by ui.label("Public note (optional):") (e.g., use the label's tooltip/hover API), keeping the rest of the ui.horizontal block and the mutable txt handling unchanged.src/ui/tokens/claim_tokens_screen.rs (1)
458-466:⚠️ Potential issue | 🟡 MinorKeep the tooltip off the editable text box.
info_tooltip()puts the Help cursor on aTextEdit, which makes an editable control look non-editable. Attach the tooltip to the label instead, or use a helper that preserves the text-entry cursor for input fields.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/tokens/claim_tokens_screen.rs` around lines 458 - 466, The tooltip is being attached to the editable TextEdit via text_edit_singleline(&mut txt).info_tooltip(...), which makes the input show a Help cursor; move the tooltip to the label instead so the editable field retains the text-entry cursor: remove the .info_tooltip(...) call on text_edit_singleline and attach the same tooltip string to the label created by ui.label("Public note (optional):") (e.g., use label.info_tooltip(...) or equivalent), leaving the TextEdit call as-is so public_note editing behaves normally.src/ui/tokens/destroy_frozen_funds_screen.rs (1)
550-558:⚠️ Potential issue | 🟡 MinorDon’t turn the note editor into a Help-cursor target.
info_tooltip()is fine for passive labels, but onTextEditit hides the normal text-entry affordance. This field should keep the I-beam cursor; move the tooltip to the label.💡 Proposed fix
ui.horizontal(|ui| { - ui.label("Public note (optional):"); + ui.label("Public note (optional):").info_tooltip( + "A note about the transaction that can be seen by the public.", + ); ui.add_space(10.0); let mut txt = self.public_note.clone().unwrap_or_default(); if ui - .text_edit_singleline(&mut txt) - .info_tooltip( - "A note about the transaction that can be seen by the public.", - ) + .text_edit_singleline(&mut txt) .changed() { self.public_note = if !txt.is_empty() { Some(txt) } else { None };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/tokens/destroy_frozen_funds_screen.rs` around lines 550 - 558, The TextEdit is currently wrapped with info_tooltip which forces a Help cursor and hides the I-beam; change the tooltip to the label instead: in the ui.horizontal block locate the label creation and move the .info_tooltip(...) call from the TextEdit invocation (text_edit_singleline on the local variable txt) to the label call that displays "Public note (optional):", leaving text_edit_singleline(&mut txt) without info_tooltip so the I-beam/text-entry affordance is preserved for the public_note field.src/ui/tokens/unfreeze_tokens_screen.rs (1)
537-545:⚠️ Potential issue | 🟡 MinorKeep the I-beam cursor on the note input.
Using
info_tooltip()on theTextEditswaps its hover affordance from “editable text” to Help. That is a UX regression for a field the user is supposed to type into. Put the tooltip on the label instead.💡 Proposed fix
ui.horizontal(|ui| { - ui.label("Public note (optional):"); + ui.label("Public note (optional):").info_tooltip( + "A note about the transaction that can be seen by the public.", + ); ui.add_space(10.0); let mut txt = self.public_note.clone().unwrap_or_default(); if ui - .text_edit_singleline(&mut txt) - .info_tooltip( - "A note about the transaction that can be seen by the public.", - ) + .text_edit_singleline(&mut txt) .changed() { self.public_note = if !txt.is_empty() { Some(txt) } else { None };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/tokens/unfreeze_tokens_screen.rs` around lines 537 - 545, The TextEdit's info_tooltip is causing the I-beam hover to be lost; remove the .info_tooltip(...) call from the text_edit_singleline(&mut txt) invocation and instead attach the tooltip to the label widget (e.g. the ui.label("Public note (optional):") call) using the label's tooltip API (on_hover_text / info_tooltip equivalent) so the label shows help while the text input retains its editable cursor; keep the rest of the logic around self.public_note, mutable txt, and ui.horizontal unchanged.src/ui/tokens/transfer_tokens_screen.rs (1)
555-563:⚠️ Potential issue | 🟡 MinorKeep the text cursor on this editable field.
info_tooltip()is the Help-cursor helper, so putting it onTextEditmakes this input look non-editable while hovered. That regresses the cursor semantics this PR is standardizing. Attach the tooltip to the adjacent label instead, or keep a plain hover tooltip on the editor.💡 Proposed fix
ui.horizontal(|ui| { - ui.label("Public note (optional):"); + ui.label("Public note (optional):").info_tooltip( + "A note about the transaction that can be seen by the public.", + ); ui.add_space(10.0); let mut txt = self.public_note.clone().unwrap_or_default(); if ui - .text_edit_singleline(&mut txt) - .info_tooltip( - "A note about the transaction that can be seen by the public.", - ) + .text_edit_singleline(&mut txt) .changed() { self.public_note = Some(txt);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/tokens/transfer_tokens_screen.rs` around lines 555 - 563, The text edit's info_tooltip is making the field appear non-editable; remove the .info_tooltip call from the ui.text_edit_singleline(&mut txt) and instead attach the explanatory tooltip to the label widget (the ui.label("Public note (optional):") call) using a label hover tooltip (e.g., label.on_hover_text(...)) or keep a plain hover tooltip on the editor; update references around self.public_note, txt, and ui.text_edit_singleline accordingly so the tooltip sits on the label and not the TextEdit.src/ui/components/top_panel.rs (1)
170-177:⚠️ Potential issue | 🟡 MinorMatch the indicator cursor to its clickable state.
This response can launch Dash-Qt in the disconnected RPC case, but
info_tooltip()always forces Help semantics. That means the one actionable state advertises the wrong cursor. Useclickable_tooltip()only when the click path is live, and fall back toinfo_tooltip()otherwise.💡 Proposed fix
- let tip = status.tooltip_text(app_context); - let resp = resp.info_tooltip(tip); + let can_start_dash_qt = overall == OverallConnectionState::Disconnected + && backend_mode == CoreBackendMode::Rpc + && !status.rpc_online(); + let tip = status.tooltip_text(app_context); + let resp = if can_start_dash_qt { + resp.clickable_tooltip(tip) + } else { + resp.info_tooltip(tip) + }; - if resp.clicked() - && overall == OverallConnectionState::Disconnected - && backend_mode == CoreBackendMode::Rpc - && !status.rpc_online() - { + if resp.clicked() && can_start_dash_qt {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/components/top_panel.rs` around lines 170 - 177, The tooltip currently always uses info_tooltip(), which forces help semantics/cursor even when the widget is actionable; change the tooltip selection to use clickable_tooltip() when the widget is actually clickable and keep info_tooltip() otherwise: compute the same clickable condition used for the click-path (overall == OverallConnectionState::Disconnected && backend_mode == CoreBackendMode::Rpc && !status.rpc_online()) and call resp = resp.clickable_tooltip(tip) in that branch, otherwise call resp = resp.info_tooltip(tip); keep the existing click handling logic unchanged.src/ui/dashpay/profile_screen.rs (1)
982-997:⚠️ Potential issue | 🟡 MinorUse the disabled helper for the non-saveable state.
Line 995 applies
clickable_tooltip()even whencan_saveis false, so the disabled Save button can still get the clickable cursor policy. That undercuts the new tooltip contract for unavailable actions. Split the response handling so enabled usesclickable_tooltip()and disabled usesdisabled_tooltip().Suggested fix
- if ui - .add_enabled(can_save, save_button) - .clickable_tooltip(&hover_text) - .on_disabled_hover_text(&hover_text) - .clicked() - { - action |= self.save_profile(); - } + let save_response = + ui.add_enabled(can_save, save_button); + + let save_response = if can_save { + save_response.clickable_tooltip(&hover_text) + } else { + save_response.disabled_tooltip(&hover_text) + }; + + if save_response.clicked() { + action |= self.save_profile(); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/dashpay/profile_screen.rs` around lines 982 - 997, The Save button currently always applies clickable_tooltip() even when can_save is false so the disabled button gets a clickable cursor; fix by splitting the widget creation/response handling: build the button widget with ui.add_enabled(can_save, save_button) but if can_save is true call .clickable_tooltip(&hover_text).on_disabled_hover_text(&hover_text).clicked() to handle clicks, otherwise call .disabled_tooltip(&hover_text) (and do not treat it as clickable). Refer to hover_text, can_save, save_button, ui.add_enabled(...) and self.is_valid() to keep the same hover text/conditions when switching the branches.src/ui/identities/identities_screen.rs (1)
611-671:⚠️ Potential issue | 🟡 MinorUse
disabled_tooltip()on the controls that can be disabled.Line 611 is inside
ui.add_enabled_ui(is_active, ...), and Lines 635 and 671 are inside branches that callui.disable(), but all three still useclickable_tooltip(). That gives disabled controls a pointing-hand affordance and breaks the cursor policy this PR is introducing. Split the enabled/disabled branches and usedisabled_tooltip()whenever the button cannot be activated.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/identities/identities_screen.rs` around lines 611 - 671, The popup currently uses clickable_tooltip() for buttons that may be disabled (the Withdraw and Transfer buttons inside the actions_popup created from actions_response), which leaves a pointing-hand cursor and breaks the new cursor policy; split the code paths so enabled and disabled UI are added separately (or use ui.add_enabled_ui/is_active) and call clickable_tooltip(...) only in the enabled branch while calling disabled_tooltip(...) in the branches where ui.disable() is currently used (affecting the Withdraw button that pushes Screen::WithdrawalScreen via WithdrawalScreen::new and the Transfer button), and similarly replace clickable_tooltip on the Top up / other disabled controls with disabled_tooltip when their can_* checks are false.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/ux-design-patterns.md`:
- Around line 184-185: The docs currently point readers to StyledButton for
helpers like add_primary_button and related tooltip policy, but those helpers
live on ComponentStyles; update the text to reference ComponentStyles (and
change the import suggestion from ResponseExt/StyledButton to ComponentStyles)
and clarify that tooltip policy should be applied via the ComponentStyles helper
methods (e.g., add_primary_button), not on StyledButton.
In `@src/ui/identities/register_dpns_name_screen.rs`:
- Around line 549-552: The tooltip is using clickable_tooltip() even when the
control is disabled; change the branch that builds the UI for
add_enabled(button_enabled, button) so that when button_enabled is false you
call disabled_tooltip(...) instead of clickable_tooltip(...). Update the code
path around add_enabled, clickable_tooltip, and on_disabled_hover_text to use
disabled_tooltip on the disabled branch (using the same hover_text) so the
disabled button does not present a click affordance.
In `@src/ui/identities/transfer_screen.rs`:
- Around line 801-804: The UI currently always calls clickable_tooltip(...) on
the result of ui.add_enabled(ready, button), which causes a clickable cursor to
be shown even when the widget is disabled; change the flow to inspect the
Response from ui.add_enabled(ready, button) and branch: if the Response is
enabled use .clickable_tooltip(hover_text).clicked() as before, otherwise call
the disabled-tooltip path (e.g. use the disabled tooltip method or attach the
hover_text via the disabled response) so the disabled state does not advertise
clickability; look for add_enabled, clickable_tooltip, clicked, ready, button
and hover_text to update the handling accordingly.
In `@src/ui/theme.rs`:
- Around line 950-963: The disabled_tooltip implementation in the ResponseExt
impl should use egui's disabled-hover API so tooltips show for disabled widgets;
update the disabled_tooltip(self, text: impl Into<egui::WidgetText>) method to
call on_disabled_hover_text(text) (and still chain
.on_hover_cursor(CursorIcon::NotAllowed)) instead of on_hover_text(text) so
disabled responses created via ui.add_enabled(false, ...) display the tooltip
correctly.
In `@src/ui/tokens/burn_tokens_screen.rs`:
- Around line 575-578: Remove the .info_tooltip(...) call from the text input
chain so the text field created by .text_edit_singleline(&mut txt) keeps its
normal text-edit cursor; instead attach the tooltip to the label or surrounding
widget (e.g., call .info_tooltip(...) on the label or a separate ui.label(...)
before/above the .text_edit_singleline) in burn_tokens_screen.rs so the help
cursor appears on the label, not the editable field.
In `@src/ui/tokens/freeze_tokens_screen.rs`:
- Around line 539-542: The tooltip is currently attached to the editable widget
via .text_edit_singleline(&mut txt).info_tooltip(...), which gives the input an
unintended Help hover; move the .info_tooltip(...) off the text_edit_singleline
call and attach it to the field label or an adjacent info/icon widget instead
(e.g., place the info_tooltip on the label rendering code for the public-note
field or the label/icon widget that wraps this input). Ensure
.text_edit_singleline(&mut txt) remains a plain editable field and the tooltip
is only present on the non-editable label/info affordance.
---
Outside diff comments:
In `@src/ui/components/top_panel.rs`:
- Around line 170-177: The tooltip currently always uses info_tooltip(), which
forces help semantics/cursor even when the widget is actionable; change the
tooltip selection to use clickable_tooltip() when the widget is actually
clickable and keep info_tooltip() otherwise: compute the same clickable
condition used for the click-path (overall ==
OverallConnectionState::Disconnected && backend_mode == CoreBackendMode::Rpc &&
!status.rpc_online()) and call resp = resp.clickable_tooltip(tip) in that
branch, otherwise call resp = resp.info_tooltip(tip); keep the existing click
handling logic unchanged.
In `@src/ui/dashpay/profile_screen.rs`:
- Around line 982-997: The Save button currently always applies
clickable_tooltip() even when can_save is false so the disabled button gets a
clickable cursor; fix by splitting the widget creation/response handling: build
the button widget with ui.add_enabled(can_save, save_button) but if can_save is
true call
.clickable_tooltip(&hover_text).on_disabled_hover_text(&hover_text).clicked() to
handle clicks, otherwise call .disabled_tooltip(&hover_text) (and do not treat
it as clickable). Refer to hover_text, can_save, save_button,
ui.add_enabled(...) and self.is_valid() to keep the same hover text/conditions
when switching the branches.
In `@src/ui/identities/identities_screen.rs`:
- Around line 611-671: The popup currently uses clickable_tooltip() for buttons
that may be disabled (the Withdraw and Transfer buttons inside the actions_popup
created from actions_response), which leaves a pointing-hand cursor and breaks
the new cursor policy; split the code paths so enabled and disabled UI are added
separately (or use ui.add_enabled_ui/is_active) and call clickable_tooltip(...)
only in the enabled branch while calling disabled_tooltip(...) in the branches
where ui.disable() is currently used (affecting the Withdraw button that pushes
Screen::WithdrawalScreen via WithdrawalScreen::new and the Transfer button), and
similarly replace clickable_tooltip on the Top up / other disabled controls with
disabled_tooltip when their can_* checks are false.
In `@src/ui/identities/keys/add_key_screen.rs`:
- Around line 559-569: The hover target for the locked security-level selector
currently calls hover_response.info_tooltip(...), which gives the wrong cursor;
change it to use hover_response.disabled_tooltip(...) so the control preserves
the "not-allowed" affordance. Locate the block that checks
has_multiple_security_levels and creates hover_response via ui.interact (using
inner_response.response.rect and egui::Id::new("security_level_tooltip")), and
replace the call to info_tooltip(format!("{:?} purpose requires {:?} security
level", self.purpose, self.security_level)) with a disabled_tooltip call that
passes the same formatted message.
In `@src/ui/tokens/claim_tokens_screen.rs`:
- Around line 458-466: The tooltip is being attached to the editable TextEdit
via text_edit_singleline(&mut txt).info_tooltip(...), which makes the input show
a Help cursor; move the tooltip to the label instead so the editable field
retains the text-entry cursor: remove the .info_tooltip(...) call on
text_edit_singleline and attach the same tooltip string to the label created by
ui.label("Public note (optional):") (e.g., use label.info_tooltip(...) or
equivalent), leaving the TextEdit call as-is so public_note editing behaves
normally.
In `@src/ui/tokens/destroy_frozen_funds_screen.rs`:
- Around line 550-558: The TextEdit is currently wrapped with info_tooltip which
forces a Help cursor and hides the I-beam; change the tooltip to the label
instead: in the ui.horizontal block locate the label creation and move the
.info_tooltip(...) call from the TextEdit invocation (text_edit_singleline on
the local variable txt) to the label call that displays "Public note
(optional):", leaving text_edit_singleline(&mut txt) without info_tooltip so the
I-beam/text-entry affordance is preserved for the public_note field.
In `@src/ui/tokens/mint_tokens_screen.rs`:
- Around line 615-623: The public-note tooltip is currently applied to the
TextEdit (via text_edit_singleline(...).info_tooltip(...)) which forces a Help
cursor and prevents normal text-entry cursor; move the tooltip to the label
instead so the field remains editable: remove the info_tooltip call from the
TextEdit for self.public_note and attach the explanatory tooltip to the label
created by ui.label("Public note (optional):") (e.g., use the label's
tooltip/hover API), keeping the rest of the ui.horizontal block and the mutable
txt handling unchanged.
In `@src/ui/tokens/set_token_price_screen.rs`:
- Around line 1043-1051: The TextEdit for public_note is using info_tooltip(),
which gives an incorrect Help-cursor affordance on an editable input; move the
tooltip from the TextEdit to the label instead: update the ui.horizontal block
that constructs the label and TextEdit (the ui.label("Public note (optional):")
and the subsequent ui.text_edit_singleline(&mut txt) using self.public_note) so
the label receives the info_tooltip(...) call and the TextEdit is rendered
without info_tooltip(), preserving an editable cursor on the input while still
exposing the explanatory tooltip on the label.
In `@src/ui/tokens/transfer_tokens_screen.rs`:
- Around line 555-563: The text edit's info_tooltip is making the field appear
non-editable; remove the .info_tooltip call from the
ui.text_edit_singleline(&mut txt) and instead attach the explanatory tooltip to
the label widget (the ui.label("Public note (optional):") call) using a label
hover tooltip (e.g., label.on_hover_text(...)) or keep a plain hover tooltip on
the editor; update references around self.public_note, txt, and
ui.text_edit_singleline accordingly so the tooltip sits on the label and not the
TextEdit.
In `@src/ui/tokens/unfreeze_tokens_screen.rs`:
- Around line 537-545: The TextEdit's info_tooltip is causing the I-beam hover
to be lost; remove the .info_tooltip(...) call from the
text_edit_singleline(&mut txt) invocation and instead attach the tooltip to the
label widget (e.g. the ui.label("Public note (optional):") call) using the
label's tooltip API (on_hover_text / info_tooltip equivalent) so the label shows
help while the text input retains its editable cursor; keep the rest of the
logic around self.public_note, mutable txt, and ui.horizontal unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 62860992-a1fb-43df-a2f1-23555f552997
📒 Files selected for processing (32)
docs/ux-design-patterns.mdsrc/ui/components/left_panel.rssrc/ui/components/password_input.rssrc/ui/components/top_panel.rssrc/ui/dashpay/profile_screen.rssrc/ui/dpns/dpns_contested_names_screen.rssrc/ui/helpers.rssrc/ui/identities/identities_screen.rssrc/ui/identities/keys/add_key_screen.rssrc/ui/identities/register_dpns_name_screen.rssrc/ui/identities/transfer_screen.rssrc/ui/network_chooser_screen.rssrc/ui/theme.rssrc/ui/tokens/burn_tokens_screen.rssrc/ui/tokens/claim_tokens_screen.rssrc/ui/tokens/destroy_frozen_funds_screen.rssrc/ui/tokens/direct_token_purchase_screen.rssrc/ui/tokens/freeze_tokens_screen.rssrc/ui/tokens/mint_tokens_screen.rssrc/ui/tokens/pause_tokens_screen.rssrc/ui/tokens/resume_tokens_screen.rssrc/ui/tokens/set_token_price_screen.rssrc/ui/tokens/tokens_screen/mod.rssrc/ui/tokens/tokens_screen/my_tokens.rssrc/ui/tokens/transfer_tokens_screen.rssrc/ui/tokens/unfreeze_tokens_screen.rssrc/ui/tokens/update_token_config.rssrc/ui/tools/masternode_list_diff_screen.rssrc/ui/tools/proof_log_screen.rssrc/ui/tools/transition_visualizer_screen.rssrc/ui/wallets/wallets_screen/asset_locks.rssrc/ui/wallets/wallets_screen/mod.rs
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Summary — PR #738
Ah, a tooltip cursor policy trait. Claudius approves of the concept — enforcing consistency through the type system rather than trusting developers to remember cursor conventions. How delightfully pessimistic about human reliability.
Overall: Clean, well-structured mechanical refactoring. The ResponseExt trait is well-designed, the 56-callsite migration is mechanically correct (no tooltip text accidentally altered), and the documentation is clear. Import organization across 30 files is pristine.
Findings
| ID | Severity | Title |
|---|---|---|
| RUST-001 | 🟡 MEDIUM | disabled_tooltip uses on_hover_text instead of on_disabled_hover_text |
One inline comment posted. Two LOW-severity observations (not posted): text input fields showing Help cursor instead of IBeam, and clickable_tooltip on add_enabled buttons showing PointingHand when disabled. These are edge cases worth considering but not blocking.
Verdict: Fix RUST-001 (the disabled_tooltip implementation), then this is ready to merge. The remaining on_disabled_hover_text migration can be done in this PR or a follow-up.
Full report available as a workflow artifact.
- Fix disabled_tooltip() to use on_disabled_hover_text() so tooltips actually appear on disabled widgets - Split add_enabled() callsites to use clickable_tooltip when enabled and disabled_tooltip when disabled (4 files) - Move info_tooltip from editable TextEdit fields to their labels to preserve the I-beam cursor on inputs (8 token screens) - Use contextual tooltip on connection indicator (clickable only when Dash-Qt can be launched) - Change locked security level selector from info_tooltip to disabled_tooltip - Fix docs referencing StyledButton instead of ComponentStyles Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/ui/components/top_panel.rs (1)
171-194:⚠️ Potential issue | 🟡 MinorDon't advertise Dash-Qt startup when no executable path is configured.
can_start_dash_qtonly checks connection state, but Lines 181-194 still no-op whensettings.dash_qt_pathisNone. That leaves the new clickable cursor/tooltip active for a path that cannot actually start Dash-Qt. Fold the path check intocan_start_dash_qt, or switch to a non-clickable/disabled tooltip that explains the missing path.Suggested adjustment
- let tip = status.tooltip_text(app_context); - let can_start_dash_qt = overall == OverallConnectionState::Disconnected - && backend_mode == CoreBackendMode::Rpc - && !status.rpc_online(); + let tip = status.tooltip_text(app_context); + let settings = app_context.get_settings().ok().flatten(); + let can_start_dash_qt = overall == OverallConnectionState::Disconnected + && backend_mode == CoreBackendMode::Rpc + && !status.rpc_online() + && settings + .as_ref() + .and_then(|s| s.dash_qt_path.as_ref()) + .is_some(); let resp = if can_start_dash_qt { resp.clickable_tooltip(tip) } else { resp.info_tooltip(tip) }; if resp.clicked() && can_start_dash_qt { - let settings = app_context.get_settings().ok().flatten(); - let (custom_path, overwrite) = settings .map(|s| (s.dash_qt_path, s.overwrite_dash_conf)) .unwrap_or((None, true));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/components/top_panel.rs` around lines 171 - 194, can_start_dash_qt currently only checks connection/backend state but not whether a Dash-Qt executable is configured, causing a clickable tooltip that no-ops; change the logic so the UI is only clickable when a path exists: fetch settings via app_context.get_settings(), compute has_dash_qt_path = settings.map(|s| s.dash_qt_path.is_some()).unwrap_or(false), and include that in the can_start_dash_qt boolean (or use it to choose resp.clickable_tooltip vs resp.info_tooltip), leaving the existing StartDashQT call and the tracing::debug branch unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/ui/identities/identities_screen.rs`:
- Line 611: The Actions button uses clickable_tooltip() inside the
add_enabled_ui(is_active, ...) block so when the identity is inactive the
response is disabled but still gets an interactive tooltip; change the tooltip
call to disabled_tooltip() for the disabled state so inactive identities show
the non-interactive explanation. Locate where actions_response is created from
ui.add(actions_button) inside add_enabled_ui and replace clickable_tooltip(...)
with disabled_tooltip(...) (or conditionally call clickable_tooltip when
is_active is true and disabled_tooltip when false) so the tooltip behavior
matches the enabled/disabled state.
---
Outside diff comments:
In `@src/ui/components/top_panel.rs`:
- Around line 171-194: can_start_dash_qt currently only checks
connection/backend state but not whether a Dash-Qt executable is configured,
causing a clickable tooltip that no-ops; change the logic so the UI is only
clickable when a path exists: fetch settings via app_context.get_settings(),
compute has_dash_qt_path = settings.map(|s|
s.dash_qt_path.is_some()).unwrap_or(false), and include that in the
can_start_dash_qt boolean (or use it to choose resp.clickable_tooltip vs
resp.info_tooltip), leaving the existing StartDashQT call and the tracing::debug
branch unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 10fb2a88-7033-4aa2-9b67-ee99646c91ab
📒 Files selected for processing (16)
docs/ux-design-patterns.mdsrc/ui/components/top_panel.rssrc/ui/dashpay/profile_screen.rssrc/ui/identities/identities_screen.rssrc/ui/identities/keys/add_key_screen.rssrc/ui/identities/register_dpns_name_screen.rssrc/ui/identities/transfer_screen.rssrc/ui/theme.rssrc/ui/tokens/burn_tokens_screen.rssrc/ui/tokens/claim_tokens_screen.rssrc/ui/tokens/destroy_frozen_funds_screen.rssrc/ui/tokens/freeze_tokens_screen.rssrc/ui/tokens/mint_tokens_screen.rssrc/ui/tokens/set_token_price_screen.rssrc/ui/tokens/transfer_tokens_screen.rssrc/ui/tokens/unfreeze_tokens_screen.rs
🚧 Files skipped from review as they are similar to previous changes (8)
- src/ui/tokens/unfreeze_tokens_screen.rs
- src/ui/tokens/mint_tokens_screen.rs
- src/ui/tokens/transfer_tokens_screen.rs
- src/ui/tokens/destroy_frozen_funds_screen.rs
- src/ui/identities/transfer_screen.rs
- src/ui/tokens/burn_tokens_screen.rs
- src/ui/tokens/freeze_tokens_screen.rs
- src/ui/tokens/claim_tokens_screen.rs
clickable_tooltip now only applies when widget is enabled,
disabled_tooltip only when disabled. This enables clean chaining:
ui.add_enabled(ready, button)
.clickable_tooltip("Transfer")
.disabled_tooltip("Fill fields first")
.clicked()
Simplifies 4 callsites that previously needed if/else branching.
Adds comprehensive rustdocs with chaining contract for future
implementors.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/ui/identities/identities_screen.rs (1)
611-611:⚠️ Potential issue | 🟡 MinorAdd the disabled-tooltip path for inactive identities’ Actions button.
This button is still created inside
add_enabled_ui(is_active, ...), so whenis_activeis falseclickable_tooltip()becomes a no-op and the user loses the “why is this unavailable?” hover text. Chaindisabled_tooltip(...)here as well.Suggested fix
- let actions_response = ui.add(actions_button).clickable_tooltip("Manage identity credits"); + let actions_response = ui + .add(actions_button) + .clickable_tooltip("Manage identity credits") + .disabled_tooltip( + "Identity actions are unavailable until this identity becomes active", + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/identities/identities_screen.rs` at line 611, The Actions button created inside add_enabled_ui currently only chains clickable_tooltip("Manage identity credits") so inactive identities lose hover feedback; update the chain on the actions_response (the value from ui.add(actions_button) in add_enabled_ui) to also call disabled_tooltip("Manage identity credits") so the tooltip appears when is_active is false while keeping clickable_tooltip for the active path; modify the code that creates actions_response in add_enabled_ui to append .disabled_tooltip("Manage identity credits") alongside the existing .clickable_tooltip(...) call.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/ui/theme.rs`:
- Around line 949-951: The doc comment for info_tooltip() promises
state-agnostic tooltips but the implementation currently uses on_hover_text(),
which doesn't show for disabled widgets; update the info_tooltip()
implementation to use on_disabled_hover_text() (or call both on_hover_text() and
on_disabled_hover_text() appropriately) so informational tooltips appear for
disabled widgets as well, ensuring the function name info_tooltip() and the
helper calls on_hover_text() / on_disabled_hover_text() are adjusted
accordingly.
---
Duplicate comments:
In `@src/ui/identities/identities_screen.rs`:
- Line 611: The Actions button created inside add_enabled_ui currently only
chains clickable_tooltip("Manage identity credits") so inactive identities lose
hover feedback; update the chain on the actions_response (the value from
ui.add(actions_button) in add_enabled_ui) to also call disabled_tooltip("Manage
identity credits") so the tooltip appears when is_active is false while keeping
clickable_tooltip for the active path; modify the code that creates
actions_response in add_enabled_ui to append .disabled_tooltip("Manage identity
credits") alongside the existing .clickable_tooltip(...) call.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7b9e3599-6d74-4c22-8344-3b737cbeefef
📒 Files selected for processing (5)
src/ui/dashpay/profile_screen.rssrc/ui/identities/identities_screen.rssrc/ui/identities/register_dpns_name_screen.rssrc/ui/identities/transfer_screen.rssrc/ui/theme.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- src/ui/dashpay/profile_screen.rs
- src/ui/identities/register_dpns_name_screen.rs
There was a problem hiding this comment.
Claudius Review — PR #738
Well-designed PR. The ResponseExt trait with state-aware chaining is elegant — it eliminates a whole class of cursor/state mismatch bugs in one stroke. The migration across 30+ files is consistently correct and the ux-design-patterns docs are clean.
2 findings posted (MEDIUM):
| ID | Severity | File | Issue |
|---|---|---|---|
RUST-001 |
🟡 MEDIUM | add_key_screen.rs:566-569 |
disabled_tooltip on always-enabled hover_response — tooltip silently lost (regression) |
RUST-002 |
🟡 MEDIUM | theme.rs:985-987 |
info_tooltip doc promises state-agnostic but on_hover_text ignores disabled widgets |
Additional LOW findings (not posted inline):
- 4 unmigrated
on_disabled_hover_textcallsites remain (withdraw_screen.rs,transfer_tokens_screen.rs,set_token_price_screen.rs,network_chooser_screen.rs) - Redundant manual
set_cursor_iconinpassword_input.rs:201-203(now handled byclickable_tooltip) add_primary_button_enabledsets PointingHand unconditionally even when disabled- ux-design-patterns.md "no tooltip helper needed" phrasing could be clearer
2 unresolved CodeRabbit threads also remain on this PR (identities_screen.rs Actions button, info_tooltip disabled behavior).
Full report: report.json + report.html in CI artifacts.
- Make info_tooltip truly state-agnostic by calling both on_hover_text and on_disabled_hover_text (fixes doc/impl mismatch) - Change add_key_screen locked selector back to info_tooltip — the hover overlay from ui.interact() is always enabled, so state-aware disabled_tooltip was a silent no-op - Chain disabled_tooltip on Actions button in identities_screen so inactive identities show explanation instead of click affordance Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Approved ✅ — Excellent trait design, thorough migration, well-documented policy.
The ResponseExt trait's state-aware chaining pattern (where clickable_tooltip silently skips on disabled widgets and vice versa) is genuinely clever — it makes the enabled/disabled tooltip pattern composable and eliminates a whole class of cursor-policy bugs. The migration across ~56 callsites in 30+ files is consistent and the method choices are correct throughout.
One MEDIUM finding: 4 bare on_disabled_hover_text() calls survive in files that already import ResponseExt — a quick cleanup to fully deliver on the "never use bare" policy this PR establishes. See inline comment.
Minor suggestions (LOW):
- 3 token screens (
pause,resume,update_token_config) keptinfo_tooltipon thetext_editinstead of the label — inconsistent with the 7 screens that moved it password_input.rs:201-203has a now-redundant manualset_cursor_icon(PointingHand)alongsideclickable_tooltip()network_chooser_screen.rs:418-421discards thedisabled_tooltipreturn value — assign to_for clarity
None of these are merge-blocking. Ship it! 🚀
🤖 Claudius the Magnificent · Full report · 2 agents, 7 findings (1 MEDIUM, 4 LOW, 2 INFO), redundancy ratio 43%
| | `.disabled_tooltip("text")` | `NotAllowed` | Disabled elements explaining why the action is unavailable | | ||
|
|
||
| - Import: `use crate::ui::theme::ResponseExt;` | ||
| - `ComponentStyles` button constructors (`add_primary_button`, etc.) set `PointingHand` automatically -- no tooltip helper needed |
There was a problem hiding this comment.
[MEDIUM] The policy is sound, but it's contradicted by 4 remaining bare on_disabled_hover_text() calls in files this PR already touches:
src/ui/tokens/set_token_price_screen.rs:1132src/ui/tokens/transfer_tokens_screen.rs:613src/ui/network_chooser_screen.rs:1640src/ui/identities/withdraw_screen.rs:627(not in this PR's changeset)
The first three files were modified by this PR and already import ResponseExt — migrating them is a one-line change each. These callsites also miss the NotAllowed cursor that disabled_tooltip() provides.
Separately, the phrasing "no tooltip helper needed" on line 185 could be read as "no ResponseExt call ever needed on ComponentStyles buttons." Consider: "ComponentStyles button constructors set PointingHand automatically — no clickable_tooltip needed for the cursor. You still need disabled_tooltip when the button can be disabled."
🤖 Claudius the Magnificent · RUST-001
Addresses the last unresolved PR review comment: 4 callsites still used bare on_disabled_hover_text() instead of the ResponseExt policy, and the docs understated when disabled_tooltip is still needed. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Introduces a
ResponseExttrait foregui::Responsethat enforces a consistent tooltip cursor policy across the UI. Three semantic methods replace raw.on_hover_text()calls:.info_tooltip().clickable_tooltip().disabled_tooltip()docs/ux-design-patterns.mddisabled_tooltip()useson_disabled_hover_text()so tooltips appear correctly on disabled widgetsadd_enabled()split tooltip path:clickable_tooltipwhen enabled,disabled_tooltipwhen disabledinfo_tooltip()on editableTextEditfields moved to their labels to preserve the I-beam cursorTest plan
🤖 Generated with Claude Code
🤖 Co-authored by Claudius the Magnificent AI Agent
Summary by CodeRabbit
New Features
Refactor
Documentation