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 all commits
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
10 changes: 6 additions & 4 deletions src/spv/manager.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1160,14 +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);

// Truncate error before formatting to avoid
// large transient allocations from adversarial peers.
let limit = error.floor_char_boundary(100);
let msg = format!("Sync manager {} failed: {}", manager, &error[..limit]);
if let Ok(mut err_guard) = last_error.write() {
if err_guard.is_none() {
*err_guard = Some(msg);
} else {
tracing::warn!("SPV last_error already set, ignoring subsequent: {}", msg);
tracing::warn!(%manager, error, "SPV last_error already set, ignoring subsequent: {}", 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
132 changes: 75 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 All @@ -228,13 +224,32 @@ impl DocumentActionScreen {
ui.heading("1. Select a contract and document type:");
ui.add_space(10.0);

let prev_contract_id = self.selected_contract.as_ref().map(|c| c.contract.id());
let prev_doc_type = self
.selected_document_type
.as_ref()
.map(|d| d.name().to_owned());

add_contract_doc_type_chooser_with_filtering(
ui,
&mut self.contract_search,
&self.app_context,
&mut self.selected_contract,
&mut self.selected_document_type,
);

let contract_changed =
prev_contract_id != self.selected_contract.as_ref().map(|c| c.contract.id());
let doc_type_changed = prev_doc_type
!= self
.selected_document_type
.as_ref()
.map(|d| d.name().to_owned());
if contract_changed || doc_type_changed {
self.no_documents_found = false;
self.fetched_documents.clear();
}

ui.add_space(10.0);
}

Expand Down Expand Up @@ -265,6 +280,8 @@ impl DocumentActionScreen {

// Handle identity change - auto-select key and update wallet
if response.changed() {
self.no_documents_found = false;
self.fetched_documents.clear();
if let Some(identity) = &self.selected_identity {
// Auto-select a suitable key for document actions
// Note: MASTER keys cannot be used for document operations,
Expand Down Expand Up @@ -467,9 +484,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.
Expand Down Expand Up @@ -520,7 +535,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 +595,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 +933,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 +994,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 +1093,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 +1632,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 +1714,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 +1799,12 @@ 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(),
"Unable to open wallet. Please unlock it and try again.",
crate::ui::MessageType::Error,
)
.with_details(e);
}
self.wallet_open_attempted = true;
}
Expand All @@ -1790,26 +1828,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