fix(ui): apply code review triage fixes for MessageBanner migration#675
Merged
Conversation
Address 7 findings from the unified message display review: - SEC-001: Truncate SPV error strings to 512 chars (UTF-8 safe via floor_char_boundary) to prevent unbounded memory from adversarial peers - PROJ-001: Remove DocumentActionScreen inline error rendering, replace backend_message with MessageBanner + no_documents_found bool - PROJ-003: Standardize take_and_clear() across 7 files (tokens_screen, keyword_search, register/update_contract_screen) - PROJ-004: Remove dead error_message fields from register_contract_screen and update_contract_screen - RUST-006: Migrate claim_tokens_screen to MessageBanner pattern — simplify status enum, remove inline error/progress rendering - RUST-007: Store BannerHandle in WalletsBalancesScreen for asset lock search progress with elapsed timer - SEC-007: Add INTENTIONAL marker for accepted risk on or_show_error raw Display text Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
📝 WalkthroughWalkthroughConsolidates per-screen error state into a global MessageBanner with an OptionBannerExt::take_and_clear() helper, removes several local error_message fields, adds banner handle lifecycles, and truncates SPV manager error strings before logging and storing last_error. Documentation note added for ResultBannerExt::or_show_error. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
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)
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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/ui/tokens/tokens_screen/keyword_search.rs (1)
85-102:⚠️ Potential issue | 🟠 Major
Clearaction should also dismiss the active operation banner.If the user clicks Clear while a search is in-flight, the elapsed “Searching contracts...” banner can remain visible. That misses the acceptance criterion for clearing keyword search state.
💡 Proposed fix
if ui.button("Clear").clicked() { + // Dismiss any active progress banner for keyword search + self.operation_banner.take_and_clear(); + // Clear the search input self.token_search_query = Some("".to_string()); // Clear the search results self.search_results.lock().unwrap().clear();As per coding guidelines: "Screens that run backend tasks should store a
refresh_banner: Option<BannerHandle>field... Indisplay_message(), dismiss the progress banner viaself.refresh_banner.take_and_clear()(fromOptionBannerExt)."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/tokens/tokens_screen/keyword_search.rs` around lines 85 - 102, The Clear button handler doesn't dismiss an in-flight progress banner; update the handler in keyword_search.rs (inside the if ui.button("Clear").clicked() block) to call self.refresh_banner.take_and_clear() so any active BannerHandle stored in the screen's refresh_banner Option is removed; ensure the struct has a refresh_banner: Option<BannerHandle> field and that OptionBannerExt is in scope so take_and_clear() is available (this mirrors how display_message() dismisses the progress banner).
🤖 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/spv/manager.rs`:
- Around line 1168-1172: The truncation appends '…' after cutting at 512 bytes
which can push the stored message past the SEC-001 512-byte cap; to fix, compute
the suffix as a byte string (e.g., let suffix = "…"), compute allowed =
512.saturating_sub(suffix.len()), then use msg.floor_char_boundary(allowed) to
get the boundary, msg.truncate(boundary) and finally append the suffix with
msg.push_str(suffix); update the code that now calls
msg.floor_char_boundary(512), msg.truncate(...), and msg.push('…') to use these
new variables and handle the edge-case where suffix.len() >= 512 by skipping
truncation/append.
In `@src/ui/contracts_documents/document_action_screen.rs`:
- Around line 466-469: The stale "No owned documents found." message happens
because self.no_documents_found is only updated after fetch; reset this flag
immediately when the selection context changes. In each method that changes
selection state (e.g., any setters or handlers that update
self.selected_identity, self.selected_contract, or self.selected_doc_type — or
methods named like select_identity/select_contract/select_type or the UI
handlers that call them), set self.no_documents_found = false (and optionally
clear any temporary document list) before triggering
fetch_documents/update_documents so the UI won't display the old "No owned
documents found." message while the new fetch is in progress.
- Around line 1781-1785: Replace the direct exposure of the raw error `e` in the
call to MessageBanner::set_global by showing a generic, user-friendly message
via MessageBanner::set_global(self.app_context.egui_ctx(), "<friendly message>",
crate::ui::MessageType::Error) and attach the technical details using
BannerHandle::with_details(...) or the equivalent banner handle method; locate
the code around MessageBanner::set_global(self.app_context.egui_ctx(), e,
crate::ui::MessageType::Error) and change it to pass a safe message (e.g.,
"Failed to open wallet") while providing `e` only as details to
BannerHandle::with_details so technical information is available in the banner
handle but not shown raw to users.
In `@src/ui/tokens/claim_tokens_screen.rs`:
- Around line 403-407: Replace the call that passes the raw error `e` into
MessageBanner::set_global—do not display backend errors directly to users;
instead create a user-friendly banner message (e.g., "Failed to open wallet")
and attach the technical error `e` as details using BannerHandle::with_details()
while still calling MessageBanner::set_global(self.app_context.egui_ctx(),
banner_handle, MessageType::Error); locate the usage around
MessageBanner::set_global and update it to build the banner via
BannerHandle::with_details() so the UI shows the safe message and keeps `e` as
non-displayed details.
---
Outside diff comments:
In `@src/ui/tokens/tokens_screen/keyword_search.rs`:
- Around line 85-102: The Clear button handler doesn't dismiss an in-flight
progress banner; update the handler in keyword_search.rs (inside the if
ui.button("Clear").clicked() block) to call self.refresh_banner.take_and_clear()
so any active BannerHandle stored in the screen's refresh_banner Option is
removed; ensure the struct has a refresh_banner: Option<BannerHandle> field and
that OptionBannerExt is in scope so take_and_clear() is available (this mirrors
how display_message() dismisses the progress banner).
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
src/spv/manager.rssrc/ui/components/message_banner.rssrc/ui/contracts_documents/document_action_screen.rssrc/ui/contracts_documents/register_contract_screen.rssrc/ui/contracts_documents/update_contract_screen.rssrc/ui/tokens/claim_tokens_screen.rssrc/ui/tokens/tokens_screen/keyword_search.rssrc/ui/tokens/tokens_screen/mod.rssrc/ui/wallets/wallets_screen/mod.rs
Comment on lines
+1168
to
+1172
| if msg.len() > 512 { | ||
| let boundary = msg.floor_char_boundary(512); | ||
| msg.truncate(boundary); | ||
| msg.push('…'); | ||
| } |
Contributor
There was a problem hiding this comment.
Ellipsis currently breaks the 512-byte cap.
After truncating at 512 bytes, appending '…' can grow the cached error to 515 bytes. If SEC-001 expects a strict 512-byte maximum, reserve space for the suffix before truncation.
Proposed fix
- if msg.len() > 512 {
- let boundary = msg.floor_char_boundary(512);
- msg.truncate(boundary);
- msg.push('…');
- }
+ const MAX_ERROR_BYTES: usize = 512;
+ let ellipsis = '…';
+ if msg.len() > MAX_ERROR_BYTES {
+ let keep = MAX_ERROR_BYTES.saturating_sub(ellipsis.len_utf8());
+ let boundary = msg.floor_char_boundary(keep);
+ msg.truncate(boundary);
+ msg.push(ellipsis);
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if msg.len() > 512 { | |
| let boundary = msg.floor_char_boundary(512); | |
| msg.truncate(boundary); | |
| msg.push('…'); | |
| } | |
| let max_error_bytes = 512; | |
| let ellipsis = '…'; | |
| if msg.len() > max_error_bytes { | |
| let keep = max_error_bytes.saturating_sub(ellipsis.len_utf8()); | |
| let boundary = msg.floor_char_boundary(keep); | |
| msg.truncate(boundary); | |
| msg.push(ellipsis); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/spv/manager.rs` around lines 1168 - 1172, The truncation appends '…'
after cutting at 512 bytes which can push the stored message past the SEC-001
512-byte cap; to fix, compute the suffix as a byte string (e.g., let suffix =
"…"), compute allowed = 512.saturating_sub(suffix.len()), then use
msg.floor_char_boundary(allowed) to get the boundary, msg.truncate(boundary) and
finally append the suffix with msg.push_str(suffix); update the code that now
calls msg.floor_char_boundary(512), msg.truncate(...), and msg.push('…') to use
these new variables and handle the edge-case where suffix.len() >= 512 by
skipping truncation/append.
Contributor
There was a problem hiding this comment.
Pull request overview
Follow-up cleanup to the unified MessageBanner migration, focusing on consistent banner lifecycle handling across UI screens, improved progress/error UX, and mitigation of oversized SPV error strings.
Changes:
- Standardize banner lifecycle management via
OptionBannerExt::take_and_clear()and store/clearBannerHandles for long-running operations. - Migrate remaining inline/field-based error/progress rendering (e.g., document actions, token claiming) to global
MessageBanner. - Add SPV sync error string truncation and document accepted risk around showing raw
Displaytext viaor_show_error.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
src/ui/wallets/wallets_screen/mod.rs |
Tracks a banner handle for asset lock search progress and clears it on completion/error. |
src/ui/tokens/tokens_screen/mod.rs |
Replaces manual banner clearing with take_and_clear() for operation banners. |
src/ui/tokens/tokens_screen/keyword_search.rs |
Uses take_and_clear() to prevent operation banner leaks during keyword search. |
src/ui/tokens/claim_tokens_screen.rs |
Migrates claim flow status/error/progress UI to global MessageBanner with elapsed timer. |
src/ui/contracts_documents/update_contract_screen.rs |
Removes unused error_message field and standardizes banner clearing via take_and_clear(). |
src/ui/contracts_documents/register_contract_screen.rs |
Removes unused error_message field and standardizes banner clearing via take_and_clear(). |
src/ui/contracts_documents/document_action_screen.rs |
Removes inline backend message rendering, uses global banners, and adds no_documents_found state. |
src/ui/components/message_banner.rs |
Documents intentional risk for ResultBannerExt::or_show_error showing raw Display text. |
src/spv/manager.rs |
Truncates stored SPV manager error strings with UTF-8-safe boundary handling. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- CMT-002/CMT-003: Wrap raw wallet-open errors with user-friendly message and attach technical details via BannerHandle::with_details() - CMT-004: Reset no_documents_found flag when identity/contract/doc type selection changes to prevent stale "No owned documents found." message - SPV: Truncate error before formatting to avoid large transient allocations Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/ui/tokens/claim_tokens_screen.rs (1)
199-206: Consider using the banner handle for consistent state management.When setting the error banner for "Identity not available" and "No signing key selected" (lines 200-206 and 226-232), the
BannerHandlereturned byMessageBanner::set_global()is discarded. While this works because error banners don't need explicit dismissal in these flows, for consistency with therefresh_bannerpattern used elsewhere in this screen, you might consider storing these handles if future requirements need to dismiss these error banners programmatically.This is a minor observation and the current implementation is functionally correct.
🤖 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 199 - 206, The calls to MessageBanner::set_global(...) in the identity and signing-key error branches discard the returned BannerHandle, which is inconsistent with the refresh_banner pattern used elsewhere; update those branches (the block that checks self.identity and the block that checks selected signing key) to capture and store the returned BannerHandle (e.g., assign to the same field used by refresh_banner or a banner handle field on the struct) instead of discarding it, while leaving the status set to ClaimTokensStatus::Error and returning AppAction::None.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/ui/tokens/claim_tokens_screen.rs`:
- Around line 199-206: The calls to MessageBanner::set_global(...) in the
identity and signing-key error branches discard the returned BannerHandle, which
is inconsistent with the refresh_banner pattern used elsewhere; update those
branches (the block that checks self.identity and the block that checks selected
signing key) to capture and store the returned BannerHandle (e.g., assign to the
same field used by refresh_banner or a banner handle field on the struct)
instead of discarding it, while leaving the status set to
ClaimTokensStatus::Error and returning AppAction::None.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/spv/manager.rssrc/ui/contracts_documents/document_action_screen.rssrc/ui/tokens/claim_tokens_screen.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/spv/manager.rs
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
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
Follow-up fixes from a systematic code review of the unified MessageBanner migration (#604). Addresses 7 triaged findings across security, project consistency, and code quality:
floor_char_boundary()to prevent unbounded memory from adversarial peers (CWE-400)DocumentActionScreeninline error rendering — replacebackend_messagefield withMessageBanner::set_global()+no_documents_foundbool for state detectiontake_and_clear()fromOptionBannerExtacross 7 files, replacing manualtake()+clear()patterns that risk banner leakserror_messagefields fromRegisterContractScreenandUpdateContractScreenClaimTokensScreento MessageBanner — simplifyClaimTokensStatusenum (removeErrorMessage(String)andWaitingForResult(u64)payloads), remove inline error/progress renderingBannerHandleinWalletsBalancesScreenfor asset lock search progress with elapsed timerINTENTIONALmarker for accepted risk onor_show_errorrawDisplaytextTest plan
error_messagefield regressions🤖 Generated with Claude Code
🤖 Co-authored by Claudius the Magnificent AI Agent
Summary by CodeRabbit
New Features
Improvements
Documentation