Skip to content

fix(ui): apply code review triage fixes for MessageBanner migration #675

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
lklimek merged 3 commits into from
Mar 3, 2026
Merged

fix(ui): apply code review triage fixes for MessageBanner migration #675

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
43b768f
fix(ui): apply code review triage fixes for MessageBanner migration
lklimek Mar 3, 2026
a223ff0
fix: truncate string
lklimek Mar 3, 2026
bc030ca
fix(ui): address PR review comments on MessageBanner migration
lklimek Mar 3, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 10 additions & 3 deletions src/spv/manager.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1160,9 +1160,16 @@ impl SpvManager {
*guard = SpvStatus::Error;
drop(guard); // Maintain lock ordering: status → release → last_error
}
// TODO: truncate error string to ~512 chars to prevent
// unbounded memory from adversarial peer errors (CWE-400).
let msg = format!("Sync manager {} failed: {}", manager, error);
let mut msg = format!("Sync manager {} failed: {}", manager, error);
// Truncate to prevent unbounded memory from
// adversarial peer errors (CWE-400).
// Use floor_char_boundary to avoid panicking
// on multi-byte UTF-8 sequences.
if msg.len() > 512 {
let boundary = msg.floor_char_boundary(512);
msg.truncate(boundary);
msg.push('…');
}

@coderabbitai coderabbitai Bot Mar 3, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

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.

Comment thread
lklimek marked this conversation as resolved.
Resolved
if let Ok(mut err_guard) = last_error.write() {
if err_guard.is_none() {
*err_guard = Some(msg);
Expand Down
3 changes: 3 additions & 0 deletions src/ui/components/message_banner.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -730,6 +730,9 @@ fn icon_for_type(message_type: MessageType) -> &'static str {
pub trait ResultBannerExt<T, E> {
/// If `Err`, displays a global error banner with the error's `Display` text.
/// Returns `self` unchanged — this is a side-effect-only method.
///
/// INTENTIONAL(SEC-007): Raw `Display` text is shown directly. Callers must
/// ensure error types have user-friendly Display implementations.
fn or_show_error(self, ctx: &egui::Context) -> Self;
}

Expand Down
110 changes: 53 additions & 57 deletions src/ui/contracts_documents/document_action_screen.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ use crate::ui::components::top_panel::add_top_panel;
use crate::ui::components::wallet_unlock_popup::{
WalletUnlockPopup, WalletUnlockResult, try_open_wallet_no_password, wallet_needs_unlock,
};
use crate::ui::components::{BannerHandle, MessageBanner, ResultBannerExt};
use crate::ui::components::{BannerHandle, MessageBanner, OptionBannerExt, ResultBannerExt};
use crate::ui::helpers::{
TransactionType, add_contract_doc_type_chooser_with_filtering, add_key_chooser_with_doc_type,
show_success_screen_with_info,
Expand Down Expand Up @@ -90,7 +90,7 @@ pub struct DocumentActionScreen {
pub action_type: DocumentActionType,

// Common fields
pub backend_message: Option<String>,
no_documents_found: bool,
pub selected_identity: Option<QualifiedIdentity>,
selected_identity_string: String,
pub selected_key: Option<IdentityPublicKey>,
Expand Down Expand Up @@ -163,7 +163,7 @@ impl DocumentActionScreen {
Self {
app_context,
action_type,
backend_message: None,
no_documents_found: false,
selected_identity,
selected_identity_string,
selected_key: None,
Expand All @@ -190,19 +190,15 @@ impl DocumentActionScreen {
}

fn set_fetching_banner(&mut self, ctx: &egui::Context, text: &str) {
if let Some(handle) = self.refresh_banner.take() {
handle.clear();
}
self.refresh_banner.take_and_clear();
let handle = MessageBanner::set_global(ctx, text, crate::ui::MessageType::Info);
handle.with_elapsed();
self.refresh_banner = Some(handle);
}

fn reset_screen(&mut self) {
if let Some(handle) = self.refresh_banner.take() {
handle.clear();
}
self.backend_message = None;
self.refresh_banner.take_and_clear();
self.no_documents_found = false;
self.selected_identity = None;
self.selected_identity_string = String::new();
self.selected_key = None;
Expand Down Expand Up @@ -467,9 +463,7 @@ impl DocumentActionScreen {
}
}

if let Some(backend_message) = &self.backend_message
&& backend_message.contains("No owned documents found")
{
if self.no_documents_found {
ui.add_space(10.0);
ui.label("No owned documents found.");
}
Comment thread
lklimek marked this conversation as resolved.
Resolved
Expand Down Expand Up @@ -520,7 +514,11 @@ impl DocumentActionScreen {
)));
}
} else {
self.backend_message = Some("Invalid Document ID format".to_string());
MessageBanner::set_global(
self.app_context.egui_ctx(),
"Invalid Document ID format",
crate::ui::MessageType::Error,
);
}
}

Expand Down Expand Up @@ -576,7 +574,11 @@ impl DocumentActionScreen {
)));
}
} else {
self.backend_message = Some("Invalid Document ID format".to_string());
MessageBanner::set_global(
self.app_context.egui_ctx(),
"Invalid Document ID format",
crate::ui::MessageType::Error,
);
}
}
});
Expand Down Expand Up @@ -910,7 +912,6 @@ impl DocumentActionScreen {
.min_size(egui::vec2(100.0, 30.0));

if ui.add(button).clicked() && self.can_broadcast() {
self.backend_message = None;
let task = self.create_document_action();
if task != BackendTask::None {
self.broadcast_status = BroadcastStatus::Broadcasting;
Expand Down Expand Up @@ -972,7 +973,11 @@ impl DocumentActionScreen {
)))
}
Err(e) => {
self.backend_message = Some(format!("Failed to build document: {}", e));
MessageBanner::set_global(
self.app_context.egui_ctx(),
format!("Failed to build document: {}", e),
crate::ui::MessageType::Error,
);
BackendTask::None
}
}
Expand Down Expand Up @@ -1067,7 +1072,11 @@ impl DocumentActionScreen {
)))
}
Err(e) => {
self.backend_message = Some(format!("Failed to build updated document: {}", e));
MessageBanner::set_global(
self.app_context.egui_ctx(),
format!("Failed to build updated document: {}", e),
crate::ui::MessageType::Error,
);
BackendTask::None
}
}
Expand Down Expand Up @@ -1602,23 +1611,20 @@ impl ScreenLike for DocumentActionScreen {
// Backend messages are handled via display_message
}

fn display_message(&mut self, message: &str, message_type: crate::ui::MessageType) {
fn display_message(&mut self, _message: &str, message_type: crate::ui::MessageType) {
if matches!(
message_type,
crate::ui::MessageType::Error | crate::ui::MessageType::Warning
) && let Some(handle) = self.refresh_banner.take()
{
handle.clear();
) {
self.refresh_banner.take_and_clear();
}
self.backend_message = Some(message.to_string());
// Banner display is handled globally by AppState; this is only for side-effects.
self.broadcast_status = BroadcastStatus::NotBroadcasted;
}

fn display_task_result(&mut self, result: crate::ui::BackendTaskSuccessResult) {
// Clear the progress banner on any completed task
if let Some(handle) = self.refresh_banner.take() {
handle.clear();
}
self.refresh_banner.take_and_clear();
match result {
BackendTaskSuccessResult::BroadcastedDocument(_) => {
self.broadcast_status = BroadcastStatus::Broadcasted;
Expand Down Expand Up @@ -1687,28 +1693,34 @@ impl ScreenLike for DocumentActionScreen {
self.fetched_price = Some(price);
}
Ok(None) => {
self.backend_message =
Some("Document has no price set".to_string());
MessageBanner::set_global(
self.app_context.egui_ctx(),
"Document has no price set",
crate::ui::MessageType::Error,
);
self.fetched_price = None;
}
Err(_) => {
self.backend_message =
Some("Failed to get document price".to_string());
MessageBanner::set_global(
self.app_context.egui_ctx(),
"Failed to get document price",
crate::ui::MessageType::Error,
);
self.fetched_price = None;
}
}
} else {
self.backend_message = Some("No document found".to_string());
MessageBanner::set_global(
self.app_context.egui_ctx(),
"No document found",
crate::ui::MessageType::Error,
);
self.fetched_price = None;
}
}
DocumentActionType::Delete => {
// For delete, store the fetched documents
if documents.is_empty() {
self.backend_message = Some("No owned documents found".to_string());
} else {
self.backend_message = None;
}
self.no_documents_found = documents.is_empty();
self.fetched_documents = documents;
}
_ => {}
Expand Down Expand Up @@ -1766,7 +1778,11 @@ impl DocumentActionScreen {
if let Some(wallet) = &self.wallet {
if !self.wallet_open_attempted {
if let Err(e) = try_open_wallet_no_password(wallet) {
self.backend_message = Some(e);
MessageBanner::set_global(
self.app_context.egui_ctx(),
e,
crate::ui::MessageType::Error,
);
Comment thread
lklimek marked this conversation as resolved.
Resolved
}
self.wallet_open_attempted = true;
}
Expand All @@ -1790,26 +1806,6 @@ impl DocumentActionScreen {
_ => self.render_action_specific_inputs(ui),
};

if let Some(ref msg) = self.backend_message {
ui.add_space(10.0);
let error_color = DashColors::error_color(ui.visuals().dark_mode);
let msg = msg.clone();
Frame::new()
.fill(error_color.gamma_multiply(0.1))
.inner_margin(Margin::symmetric(10, 8))
.corner_radius(5.0)
.stroke(egui::Stroke::new(1.0, error_color))
.show(ui, |ui| {
ui.horizontal(|ui| {
ui.label(RichText::new(&msg).color(error_color));
ui.add_space(10.0);
if ui.small_button("Dismiss").clicked() {
self.backend_message = None;
}
});
});
}

action
})
.inner
Expand Down
24 changes: 7 additions & 17 deletions src/ui/contracts_documents/register_contract_screen.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ use crate::ui::components::top_panel::add_top_panel;
use crate::ui::components::wallet_unlock_popup::{
WalletUnlockPopup, WalletUnlockResult, try_open_wallet_no_password, wallet_needs_unlock,
};
use crate::ui::components::{BannerHandle, MessageBanner, ResultBannerExt};
use crate::ui::components::{BannerHandle, MessageBanner, OptionBannerExt, ResultBannerExt};
use crate::ui::helpers::{TransactionType, add_key_chooser};
use crate::ui::identities::get_selected_wallet;
use crate::ui::theme::DashColors;
Expand Down Expand Up @@ -54,7 +54,6 @@ pub struct RegisterDataContractScreen {
pub selected_wallet: Option<Arc<RwLock<Wallet>>>,
wallet_open_attempted: bool,
wallet_unlock_popup: WalletUnlockPopup,
error_message: Option<String>,
completed_fee_result: Option<FeeResult>,
refresh_banner: Option<BannerHandle>,
}
Expand Down Expand Up @@ -110,7 +109,6 @@ impl RegisterDataContractScreen {
selected_wallet,
wallet_open_attempted: false,
wallet_unlock_popup: WalletUnlockPopup::new(),
error_message: None,
completed_fee_result: None,
refresh_banner: None,
}
Expand Down Expand Up @@ -285,9 +283,7 @@ impl RegisterDataContractScreen {
&& let ContractTask::RegisterDataContract(_, _, _, _) = **contract_task
{
self.broadcast_status = BroadcastStatus::Broadcasting;
if let Some(handle) = self.refresh_banner.take() {
handle.clear();
}
self.refresh_banner.take_and_clear();
let handle =
MessageBanner::set_global(ui.ctx(), "Broadcasting contract...", MessageType::Info);
handle.with_elapsed();
Expand Down Expand Up @@ -331,14 +327,11 @@ impl RegisterDataContractScreen {

impl ScreenLike for RegisterDataContractScreen {
fn display_message(&mut self, message: &str, message_type: MessageType) {
if matches!(message_type, MessageType::Error | MessageType::Warning)
&& let Some(handle) = self.refresh_banner.take()
{
handle.clear();
if matches!(message_type, MessageType::Error | MessageType::Warning) {
self.refresh_banner.take_and_clear();
}
if message_type == MessageType::Error {
if message.contains("proof error logged, contract inserted into the database") {
self.error_message = Some(message.to_string());
self.broadcast_status = BroadcastStatus::Done;
} else {
self.broadcast_status = BroadcastStatus::BroadcastError(message.to_string());
Expand All @@ -352,9 +345,7 @@ impl ScreenLike for RegisterDataContractScreen {
self.broadcast_status = BroadcastStatus::Broadcasting;
}
BackendTaskSuccessResult::RegisteredContract(fee_result) => {
if let Some(handle) = self.refresh_banner.take() {
handle.clear();
}
self.refresh_banner.take_and_clear();
self.completed_fee_result = Some(fee_result);
self.broadcast_status = BroadcastStatus::Done;
}
Expand Down Expand Up @@ -515,9 +506,8 @@ impl ScreenLike for RegisterDataContractScreen {
// Render wallet unlock if needed
if let Some(wallet) = &self.selected_wallet {
if !self.wallet_open_attempted {
if let Err(e) = try_open_wallet_no_password(wallet) {
self.error_message = Some(e);
}
let _ = try_open_wallet_no_password(wallet)
.or_show_error(ui.ctx());
self.wallet_open_attempted = true;
}
if wallet_needs_unlock(wallet) {
Expand Down
Loading
Loading