fix(ui): fix message banner icons, dismiss button, text wrapping, and duplicate errors#654
Conversation
… duplicate errors - Change error icon from ❌ to ⛔ to avoid confusion with dismiss button - Use ❌ as styled dismiss button (clickable label, pointer cursor on hover) - Fix success icon (✅) and info icon (💬) to render correctly in Noto Sans - Wrap long banner text at word boundaries instead of overflowing window - Left-align text within the wrapping area - Remove duplicate error display in AddNewIdentityScreen — delegate to global MessageBanner - Route local validation errors through MessageBanner::set_global for consistent styling Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
📝 WalkthroughWalkthroughRefactors the message banner layout to use emoji icons and a clickable emoji dismiss control with dynamic text wrapping; integrates a global banner for error display by pushing errors from the Add New Identity screen; test expectations and Cargo.toml adjusted accordingly. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/components/message_banner.rs`:
- Around line 511-517: The fixed 100.0 minimum for text_width can cause the row
to overflow in very narrow windows; update the calculation used before
ui.add_sized so text_width never forces overflow by clamping it to the available
space: compute text_width from (available_width - dismiss_width - 30.0) and
clamp to a non-negative maximum (e.g. .clamp(0.0, available_width -
dismiss_width) or use .max(0.0) instead of .max(100.0)), then pass that
text_width to egui::vec2 in the ui.add_sized call (referencing text_width,
available_width, dismiss_width, ui.add_sized, egui::vec2).
In `@src/ui/identities/add_new_identity_screen/mod.rs`:
- Around line 24-25: The PR fails CI due to formatting; run rustfmt (cargo fmt
--all) to reformat src/ui/identities/add_new_identity_screen/mod.rs so the
imports and surrounding code (references: MessageBanner, MessageType,
ScreenLike) comply with rustfmt rules; after formatting, re-run cargo fmt --all
-- --check to verify and commit the updated file.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/ui/components/message_banner.rssrc/ui/identities/add_new_identity_screen/mod.rs
Prevents row overflow in very narrow windows by allowing text_width to shrink to zero rather than forcing a 100px floor. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Account for annotation width (e.g. "(5s)" countdown) when calculating text_width so it doesn't overlap long messages. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Update kittest assertions to match new dismiss button (❌ emoji label instead of "x" small_button) and new icon mappings (⛔ error, ✅ success, 💬 info). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR improves the global MessageBanner UX in the egui UI by adjusting icons, making the dismiss control a styled clickable label, ensuring banner text wraps cleanly, and removing duplicate/local error rendering in the “Add New Identity” flow.
Changes:
- Update banner icons (Error/Success/Info) and switch dismiss control to a clickable ❌ label.
- Improve banner layout so long text wraps at word boundaries instead of overflowing.
- Route
AddNewIdentityScreenerrors through the globalMessageBannerto avoid duplicate displays.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| tests/kittest/message_banner.rs | Updates UI tests to assert the new iconography and the ❌ dismiss label. |
| src/ui/identities/add_new_identity_screen/mod.rs | Removes local banner-like UI and forwards validation errors into the global banner system. |
| src/ui/components/message_banner.rs | Reworks banner rendering layout for wrapping text, updates icons, and implements the new dismiss interaction. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Fix comment: icon is not actually "fixed width" (#2850192960) - Measure annotation width dynamically instead of hard-coded 52px (#2850192972) - Prevent validation errors from re-pushing to global banner each frame by tracking last-sent message (#2850192953) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Lower per-character width multiplier from 0.55 to 0.4 to match actual rendered width of digit/paren annotation strings like "(5s)". Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/add_new_identity_screen/mod.rs`:
- Around line 1106-1116: When error_message != last_global_error, always clear
the previous global banner before setting a new one: in the block guarded by the
equality check, if let Some(old) = self.last_global_error.as_ref() call
MessageBanner::clear_global_message(ui.ctx(), old) first, then if let Some(new)
= self.error_message.as_ref() call MessageBanner::set_global(ui.ctx(), new,
MessageType::Error); finally update self.last_global_error =
self.error_message.clone(); this ensures orphaned banners are removed when
switching from Some("A") to Some("B") while using the existing symbols
error_message, last_global_error, MessageBanner::clear_global_message and
MessageBanner::set_global.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/ui/components/message_banner.rssrc/ui/identities/add_new_identity_screen/mod.rstests/kittest/message_banner.rs
When error_message transitions from Some("A") to Some("B"), the old
banner was never cleared because the clear branch was in an else-if.
This caused orphaned error banners to stack up in the global banner list.
Fix: always clear the old banner before setting a new one, using two
independent if-blocks instead of if/else-if.
Found by CodeRabbit review on dashpay#654.
Summary
small_button("x")AddNewIdentityScreen— errors now only show via the globalMessageBannerTest plan
🤖 Generated with Claude Code
🤖 Co-authored by Claudius the Magnificent AI Agent
Summary by CodeRabbit
Style
Refactor
Tests