feat: unified message banner for all screens#604
Merged
Conversation
Add UX specification, technical architecture, and HTML mockup for the MessageBanner component that will replace the ~50 ad-hoc error/message display implementations across screens with a single reusable component. Key design decisions: - Per-screen MessageBanner with show()/set_message() API - All colors via DashColors (zero hardcoded Color32 values) - 4 severity levels: Error, Warning, Success, Info - Auto-dismiss for Success/Info (5s), persistent for Error/Warning - Follows Component Design Pattern conventions (private fields, builder, show) - No changes to BackendTask/TaskResult/AppState architecture Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughCentralizes per-screen messaging into a global MessageBanner API with BannerHandle lifecycle; migrates screens from local message/error fields and timestamped status payloads to banner-driven status; integrates connection-state banners in AppState; refactors SPV client handling and updates backend fee/logging utilities. Changes
Sequence Diagram(s)sequenceDiagram
participant User as UI / Screen
participant App as AppState
participant Backend as BackendTask
participant Banner as MessageBanner
rect rgba(200,200,255,0.5)
User->>App: Trigger action (start backend task)
App->>Backend: Dispatch task (with context)
Backend-->>App: Return Success / Error / Message
App->>Banner: MessageBanner::set_global(text, type)
Banner-->>App: BannerHandle
App->>User: Update screens (store BannerHandle / state)
User->>Banner: BannerHandle.with_elapsed() / take_and_clear()
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
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 |
lklimek
changed the base branch from
v1.0-dev
to
design/unified-message-display
![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: 13
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (15)
src/ui/dashpay/qr_scanner.rs (1)
323-369:⚠️ Potential issue | 🟡 MinorImplement missing ScreenLike lifecycle methods.
This impl lacks
refresh/refresh_on_arrivalandchange_context, which are required for screens insrc/ui/**/*.rs. Please add the methods (even as no-ops) per the trait signature.
As per coding guidelines:src/ui/**/*.rs: All screens must implement theScreenLiketrait with methods:ui(),display_task_result(),display_message(),refresh()/refresh_on_arrival(), andchange_context().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/dashpay/qr_scanner.rs` around lines 323 - 369, The QRScannerScreen impl of ScreenLike is missing the lifecycle methods required by the trait; add no-op implementations (or appropriate behavior) for refresh() or refresh_on_arrival() (whichever the trait defines) and change_context(&mut self, ctx: AppContext) to the impl block for QRScannerScreen so the trait is fully implemented; keep them minimal (empty bodies or a small update) matching the trait signatures used across src/ui/**/*.rs and reference the ScreenLike impl for QRScannerScreen so the compiler recognizes methods like refresh()/refresh_on_arrival() and change_context().src/ui/tokens/view_token_claims_screen.rs (1)
70-97:⚠️ Potential issue | 🟡 MinorImplement missing ScreenLike lifecycle methods.
This impl lacks
refresh/refresh_on_arrivalandchange_context, which are required for screens insrc/ui/**/*.rs. Please add the methods (even as no-ops) per the trait signature.
As per coding guidelines:src/ui/**/*.rs: All screens must implement theScreenLiketrait with methods:ui(),display_task_result(),display_message(),refresh()/refresh_on_arrival(), andchange_context().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/tokens/view_token_claims_screen.rs` around lines 70 - 97, The ScreenLike impl for ViewTokenClaimsScreen is missing required lifecycle methods; add the missing methods refresh (or refresh_on_arrival, depending on the trait variant) and change_context to the impl so the struct fully implements ScreenLike. Locate the impl ScreenLike for ViewTokenClaimsScreen and add fn refresh(&mut self) { /* no-op */ } or fn refresh_on_arrival(&mut self) { /* no-op */ } as appropriate to your ScreenLike trait, plus fn change_context(&mut self, ctx: AppContext) { /* update/ignore ctx as needed */ } using the exact method names and signatures from the ScreenLike trait so the file compiles and behavior remains unchanged.src/ui/contracts_documents/register_contract_screen.rs (2)
341-377:⚠️ Potential issue | 🟡 MinorImplement missing ScreenLike lifecycle methods.
This impl lacks
refresh/refresh_on_arrivalandchange_context, which are required for screens insrc/ui/**/*.rs. Please add the methods (even as no-ops) per the trait signature.
As per coding guidelines:src/ui/**/*.rs: All screens must implement theScreenLiketrait with methods:ui(),display_task_result(),display_message(),refresh()/refresh_on_arrival(), andchange_context().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/contracts_documents/register_contract_screen.rs` around lines 341 - 377, Add the missing ScreenLike lifecycle methods to the impl for RegisterDataContractScreen: implement refresh(&mut self, ctx: &egui::Context) and refresh_on_arrival(&mut self, ctx: &egui::Context) (they can be no-ops) and implement change_context(&mut self, ctx: ScreenContext) to satisfy the ScreenLike trait; place them alongside the existing display_message and display_task_result methods and follow the trait signatures used across other screens so the compiler recognizes the impl of ScreenLike for RegisterDataContractScreen.
67-73:⚠️ Potential issue | 🟡 MinorSurface wallet-selection errors during initialization.
get_selected_walletcan populateerror_message, but it’s currently ignored innew(). After removing per-screen error state, this makes initial wallet failures silent. Consider surfacing via the global banner.Proposed fix
let selected_wallet = if let Some(ref identity) = selected_qualified_identity { get_selected_wallet(identity, Some(app_context), None, &mut error_message) } else { None }; + if let Some(err) = error_message { + MessageBanner::set_global(app_context.egui_ctx(), &err, MessageType::Error); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/contracts_documents/register_contract_screen.rs` around lines 67 - 73, The initialization in new() calls get_selected_wallet(...) which can set error_message but currently ignores it; update new() to check error_message after the call to get_selected_wallet and, if Some(msg), surface it to the global/banner UI via the app_context (e.g., call the app_context or global banner enqueue/display method) so initial wallet-selection failures are visible; ensure you still return the selected_wallet as before but also forward the error string to the global banner handling function.src/ui/tools/grovestark_screen.rs (1)
984-1003:⚠️ Potential issue | 🟡 MinorAdd missing
change_contextimplementation.This screen already implements
refreshandrefresh_on_arrival, butchange_contextis still missing. Please add it per the ScreenLike trait contract.
As per coding guidelines:src/ui/**/*.rs: All screens must implement theScreenLiketrait with methods:ui(),display_task_result(),display_message(),refresh()/refresh_on_arrival(), andchange_context().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/tools/grovestark_screen.rs` around lines 984 - 1003, Add the missing change_context method to GroveSTARKScreen to satisfy the ScreenLike trait: implement fn change_context(&mut self, new_context: AppContext) (or the exact signature used in ScreenLike) to update the screen's stored app_context (and any dependent state) and trigger necessary updates (e.g., call self.refresh() or self.refresh_on_arrival() as appropriate); locate GroveSTARKScreen and implement change_context to replace/clone the existing app_context field and then call the existing refresh/refresh_identities(&app_context)/refresh_contracts(&app_context) flow so the screen reflects the new context.src/ui/tools/platform_info_screen.rs (1)
141-225:⚠️ Potential issue | 🟡 MinorAdd missing
change_contextimplementation.This screen already implements
refreshandrefresh_on_arrival, butchange_contextis still missing. Please add it per the ScreenLike trait contract.
As per coding guidelines:src/ui/**/*.rs: All screens must implement theScreenLiketrait with methods:ui(),display_task_result(),display_message(),refresh()/refresh_on_arrival(), andchange_context().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/tools/platform_info_screen.rs` around lines 141 - 225, Add the missing change_context implementation in the impl ScreenLike for PlatformInfoScreen: implement fn change_context(&mut self, app_context: Arc<AppContext>) so it replaces the screen's AppContext (self.app_context = app_context), updates dependent cached fields (e.g. self.network = self.app_context.network), and clears or resets transient UI state such as self.platform_version, self.core_chain_lock_height, self.current_result, self.current_result_title and self.active_tasks; ensure the method signature and ownership match the ScreenLike trait definition used elsewhere.src/ui/tokens/direct_token_purchase_screen.rs (1)
346-387:⚠️ Potential issue | 🟡 MinorAdd missing
change_contextimplementation.This screen implements
refresh, butchange_contextis still missing. Please add it per the ScreenLike trait contract.
As per coding guidelines:src/ui/**/*.rs: All screens must implement theScreenLiketrait with methods:ui(),display_task_result(),display_message(),refresh()/refresh_on_arrival(), andchange_context().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/tokens/direct_token_purchase_screen.rs` around lines 346 - 387, Implement the missing ScreenLike::change_context(&mut self, ctx: &AppContext) for PurchaseTokenScreen: add a method named change_context that updates the screen's stored app context reference/handle (the same field used elsewhere, e.g., self.app_context), clears or resets transient UI state such as self.refresh_banner and any error status, and then triggers the same refresh behavior used on arrival (call the existing refresh() or refresh_on_arrival() path used by other screens). Follow the pattern from other screens implementing ScreenLike so the screen's fields (fetched_pricing_schedule, pricing_fetch_attempted, status, refresh_banner) are correctly reinitialized when context changes.src/ui/mod.rs (1)
837-846: 🛠️ Refactor suggestion | 🟠 MajorAdd
change_context()method to ScreenLike trait.The trait is missing
change_context()required by the coding guidelines. Add it as a default no-op and delegate in theScreenimpl to match the pattern used for other optional methods.Suggested changes
pub trait ScreenLike { fn refresh(&mut self) {} fn refresh_on_arrival(&mut self) { self.refresh() } fn ui(&mut self, ctx: &Context) -> AppAction; fn display_message(&mut self, _message: &str, _message_type: MessageType) {} fn display_task_result(&mut self, _backend_task_success_result: BackendTaskSuccessResult) {} + fn change_context(&mut self, _app_context: Arc<AppContext>) {} fn pop_on_success(&mut self) {} }impl ScreenLike for Screen { + fn change_context(&mut self, app_context: Arc<AppContext>) { + Screen::change_context(self, app_context); + } fn refresh(&mut self) { match self { Screen::IdentitiesScreen(screen) => screen.refresh(), // ... }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/mod.rs` around lines 837 - 846, The ScreenLike trait needs a new optional method change_context() following the same pattern as the other no-op methods: add fn change_context(&mut self) {} to the ScreenLike trait as a default no-op, and in the Screen impl add a corresponding pub fn change_context(&mut self) { self.screen.change_context(); } (or delegate from whatever wrapper method name you use) so the Screen wrapper simply forwards change_context to the inner ScreenLike implementation.src/ui/wallets/send_screen.rs (1)
2580-2723:⚠️ Potential issue | 🟡 MinorAdd change_context to ScreenLike impl.
This ScreenLike impl still lacks
change_context(). Please add it to meet the screen contract.As per coding guidelines,
src/ui/**/*.rs: All screens must implement theScreenLiketrait with methods:ui(),display_task_result(),display_message(),refresh()/refresh_on_arrival(), andchange_context().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/wallets/send_screen.rs` around lines 2580 - 2723, The ScreenLike implementation for WalletSendScreen is missing the required change_context() method; add a public impl method fn change_context(&mut self, ctx: &AppContext) { ... } (or matching your trait signature) to the impl ScreenLike for WalletSendScreen so the type fulfills the ScreenLike contract. Locate the impl block containing ui(), display_message(), display_task_result(), refresh_on_arrival(), and refresh(), and add change_context() alongside those methods, updating any internal state (e.g., self.app_context or selected_wallet) as your trait expects.src/ui/tools/transition_visualizer_screen.rs (1)
386-431:⚠️ Potential issue | 🟡 MinorAdd change_context to ScreenLike impl.
This ScreenLike impl still lacks
change_context(). Please add it to meet the screen contract.As per coding guidelines,
src/ui/**/*.rs: All screens must implement theScreenLiketrait with methods:ui(),display_task_result(),display_message(),refresh()/refresh_on_arrival(), andchange_context().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/tools/transition_visualizer_screen.rs` around lines 386 - 431, The impl ScreenLike for TransitionVisualizerScreen is missing the required change_context method; add a method named change_context to this impl with the exact signature defined by the ScreenLike trait (e.g., fn change_context(&mut self, ctx: &mut AppContext) or the crate::ui equivalent) and implement it to update the screen's internal state from the provided context (similar to how ui()/refresh()/display_message()/display_task_result() operate), placing it inside the impl block for TransitionVisualizerScreen so the type fully satisfies the ScreenLike contract.src/ui/dashpay/send_payment.rs (1)
379-449:⚠️ Potential issue | 🟡 MinorAdd change_context to ScreenLike impl.
This ScreenLike impl still lacks
change_context(). Please add it to meet the screen contract.As per coding guidelines,
src/ui/**/*.rs: All screens must implement theScreenLiketrait with methods:ui(),display_task_result(),display_message(),refresh()/refresh_on_arrival(), andchange_context().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/dashpay/send_payment.rs` around lines 379 - 449, The impl for ScreenLike on SendPaymentScreen is missing the required change_context method; add a change_context implementation matching the ScreenLike trait signature (same params/borrow patterns as other screens) inside the impl for SendPaymentScreen, update the internal app context/state (self.app_context or related fields) from the new context, and call any necessary refresh logic like self.load_contact_info() or self.refresh() so the screen updates when the app context changes; reference: implement change_context for SendPaymentScreen within the existing impl ScreenLike block so it mirrors other screens' behavior.src/ui/tokens/resume_tokens_screen.rs (1)
278-307:⚠️ Potential issue | 🟡 MinorAdd change_context to ScreenLike impl.
This ScreenLike impl doesn’t provide
change_context(). Please add it to align with the screen contract.As per coding guidelines,
src/ui/**/*.rs: All screens must implement theScreenLiketrait with methods:ui(),display_task_result(),display_message(),refresh()/refresh_on_arrival(), andchange_context().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/tokens/resume_tokens_screen.rs` around lines 278 - 307, Add the missing ScreenLike::change_context implementation for ResumeTokensScreen: implement fn change_context(&mut self, ctx: ScreenContext) (matching the ScreenLike trait) that updates the screen's stored context (assign ctx to the screen's context field, e.g., self.context) and then trigger the appropriate refresh behavior (call refresh_on_arrival() if present else refresh()) so the screen state is updated when context changes; ensure the method lives in the same impl block for ResumeTokensScreen alongside display_message/display_task_result/refresh.src/ui/contracts_documents/update_contract_screen.rs (2)
362-398:⚠️ Potential issue | 🟡 MinorAdd change_context to ScreenLike impl.
This ScreenLike impl still lacks
change_context(). Please add it to meet the screen contract.As per coding guidelines,
src/ui/**/*.rs: All screens must implement theScreenLiketrait with methods:ui(),display_task_result(),display_message(),refresh()/refresh_on_arrival(), andchange_context().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/contracts_documents/update_contract_screen.rs` around lines 362 - 398, The impl for ScreenLike on UpdateDataContractScreen is missing the required change_context method; add a change_context implementation matching the ScreenLike trait signature (change_context(...)) inside the impl block for UpdateDataContractScreen, and have it update the screen's context or call the existing refresh/refresh_on_arrival logic (e.g., update any context-related field on UpdateDataContractScreen and trigger refresh behavior) so the type satisfies the ScreenLike contract.
35-44:⚠️ Potential issue | 🟠 MajorBroadcastError blocks retry because the parsed contract is discarded.
After an error,
BroadcastStatus::BroadcastErroris set and theValidContractpayload is lost, so the Update button never reappears unless the user edits the JSON to re-parse. Consider rehydrating the parsed contract (or resetting to a retryable state) when a broadcast error occurs.✅ Suggested fix (rehydrate for retry)
- } else { - self.broadcast_status = BroadcastStatus::BroadcastError; - } + } else { + // Re-parse to restore ValidContract so the user can retry without editing JSON. + self.parse_contract(); + }Also applies to: 186-222, 364-370
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/contracts_documents/update_contract_screen.rs` around lines 35 - 44, The BroadcastError path currently drops the parsed contract (BroadcastStatus::BroadcastError) so retries are blocked; preserve the parsed contract by changing the enum (in src/ui/contracts_documents/update_contract_screen.rs) to carry the contract (e.g., BroadcastError(Box<DataContract>, Option<String>) or add a new variant RetryableBroadcastError(Box<DataContract>, Option<String>)), update all places that set BroadcastStatus::BroadcastError (and any match arms handling it) to supply the parsed contract and error message, and update the UI logic that decides whether to show the Update button to treat the new/updated BroadcastError variant as retryable (equivalent to ValidContract) so the user can retry without re-parsing.src/ui/identities/keys/add_key_screen.rs (1)
410-413:⚠️ Potential issue | 🟡 MinorDuplicate unreachable
Completecheck.Lines 402-405 already check
AddKeyStatus::Completeand return early. This second check at lines 410-413 is dead code.🧹 Remove duplicate check
ui.heading("Add New Key"); ui.add_space(10.0); - if self.add_key_status == AddKeyStatus::Complete { - inner_action |= self.show_success(ui); - return inner_action; - } - if self.selected_wallet.is_some()🤖 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 410 - 413, Remove the duplicate unreachable check for AddKeyStatus::Complete: the code block that tests `if self.add_key_status == AddKeyStatus::Complete { inner_action |= self.show_success(ui); return inner_action; }` is redundant because an earlier return already handles `AddKeyStatus::Complete`; delete this second check (the duplicate `Complete` branch) so `self.add_key_status` is only handled once and flow continues to the intended subsequent logic (leave `show_success` and `inner_action` usage only in the original place where it's returned).
🟡 Minor comments (11)
src/ui/identities/transfer_screen.rs-502-510 (1)
502-510:⚠️ Potential issue | 🟡 MinorAdd missing
change_contextimplementation.This screen implements
refresh, butchange_contextis still missing. Please add it per the ScreenLike trait contract.
As per coding guidelines:src/ui/**/*.rs: All screens must implement theScreenLiketrait with methods:ui(),display_task_result(),display_message(),refresh()/refresh_on_arrival(), andchange_context().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/identities/transfer_screen.rs` around lines 502 - 510, The TransferScreen impl for ScreenLike is missing the required change_context(&mut self, ctx: ScreenContext) method; add a change_context implementation inside impl ScreenLike for TransferScreen that matches the ScreenLike trait signature (change_context), update TransferScreen's internal context/state as appropriate (e.g., store the incoming ScreenContext or update fields used by ui()/refresh()/refresh_on_arrival()), and trigger any necessary side-effects such as scheduling a refresh or clearing/setting refresh_banner similar to display_message/refresh behavior so the screen reacts to context changes consistently.src/ui/tokens/update_token_config.rs-918-934 (1)
918-934:⚠️ Potential issue | 🟡 MinorImplement missing ScreenLike lifecycle methods.
This impl lacks
refresh/refresh_on_arrivalandchange_context, which are required for screens insrc/ui/**/*.rs. Please add the methods (even as no-ops) per the trait signature.
As per coding guidelines:src/ui/**/*.rs: All screens must implement theScreenLiketrait with methods:ui(),display_task_result(),display_message(),refresh()/refresh_on_arrival(), andchange_context().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/tokens/update_token_config.rs` around lines 918 - 934, The impl for UpdateTokenConfigScreen is missing the ScreenLike lifecycle methods refresh/refresh_on_arrival and change_context; add no-op implementations that match the ScreenLike trait signatures (i.e., implement refresh() and/or refresh_on_arrival() and change_context() for UpdateTokenConfigScreen) alongside the existing display_message and display_task_result methods so the type fully implements ScreenLike. Ensure the method names and signatures exactly match the trait used by other screens (use those files as reference) and keep bodies empty or performing no-ops consistent with coding guidelines.src/ui/dashpay/qr_scanner.rs-50-58 (1)
50-58:⚠️ Potential issue | 🟡 MinorClear stale parsed QR data when input is empty.
If the user clears the input and clicks “Parse QR Code,”
parsed_qr_dataremains from a previous parse, so “Add Contact” can send outdated data. Reset it before returning.Proposed fix
if self.qr_data_input.is_empty() { + self.parsed_qr_data = None; crate::ui::components::MessageBanner::set_global( self.app_context.egui_ctx(), "Please enter QR code data", MessageType::Error, ); return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/dashpay/qr_scanner.rs` around lines 50 - 58, In parse_qr_code, when self.qr_data_input is empty you currently show an error and return but do not clear stale parsed_qr_data; update parse_qr_code to reset self.parsed_qr_data to None (or the empty/default value used by the struct) before returning so the previous parsed QR payload cannot be reused by the "Add Contact" flow; locate the parsed_qr_data field on the same struct and clear it at the top of parse_qr_code right before the MessageBanner and return.docs/ai-design/2026-02-17-unified-messages/message-banner-ux-spec.md-31-35 (1)
31-35:⚠️ Potential issue | 🟡 MinorAdd language identifiers to fenced code blocks (MD040).
This resolves markdownlint warnings and improves formatting consistency.💡 Suggested fix
-``` +```text +-----------------------------------------------------------------------+ | [Icon] Message text here [5s] [x] | +-----------------------------------------------------------------------+ -``` +``` -``` +```text +--------------------------------------------------+ | Top Panel (header / navigation) | +--------------------------------------------------+ | Left Panel | [ Banner 1 ] | | | [ Banner 2 ] | | | +----- Screen Content -----+ | | | | ... | | | | +--------------------------+ | +--------------------------------------------------+ -``` +``` </details> Also applies to: 81-91 <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@docs/ai-design/2026-02-17-unified-messages/message-banner-ux-spec.mdaround
lines 31 - 35, Add language identifiers ("text") to the fenced code blocks for
the ASCII UI art examples so markdownlint MD040 is satisfied; update the three
backtick fences shown in the snippet (the banner box block and the larger layout
block—the same pattern also appears later around the second example) to use
text instead ofand ensure each opening fence has a matching closing
fence with no extra characters removed or added.</details> </blockquote></details> <details> <summary>docs/ai-design/2026-02-17-unified-messages/architecture.md-127-139 (1)</summary><blockquote> `127-139`: _⚠️ Potential issue_ | _🟡 Minor_ **Add language identifiers to fenced code blocks (MD040).** This clears markdownlint warnings and improves syntax highlighting. <details> <summary>💡 Suggested fix</summary> ```diff -``` +```rust render_banner(ui, text, message_type, annotation: Option<&str>) -> bool (dismissed?) ``` -``` +```text +-----------------------------------------------------------------------+ | [Icon] Message text here [5s] [x] | +-----------------------------------------------------------------------+ -``` +``` -``` +```text TaskResult::Error(message) → MessageBanner::set_global(ctx, &message, MessageType::Error) → screen.display_message(&message, MessageType::Error) // for side-effects ``` </details> Also applies to: 156-164 <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@docs/ai-design/2026-02-17-unified-messages/architecture.mdaround lines 127
- 139, Add explicit language identifiers to the fenced code blocks in the doc
snippets to satisfy MD040: update the first function block to userust for the render_banner signature (render_banner/ui/process_banner references), change the ASCII art and flow blocks totext (the visual structure box and the
TaskResult::Error → MessageBanner::set_global → screen.display_message
sequence), and ensure the other similar block at lines 156-164 also has a
language identifier; no code changes required, only annotate the three fenced
blocks with appropriate language tags.</details> </blockquote></details> <details> <summary>src/ui/tokens/tokens_screen/keyword_search.rs-63-70 (1)</summary><blockquote> `63-70`: _⚠️ Potential issue_ | _🟡 Minor_ **Elapsed timer can carry over across searches.** `MessageBanner::set_global` deduplicates by text, so repeating the same “Searching contracts...” banner can reuse the old `created_at` and the elapsed counter won’t reset on a new search. Consider clearing/replacing the prior banner handle before starting a new search. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@src/ui/tokens/tokens_screen/keyword_search.rs` around lines 63 - 70, The elapsed timer can persist because MessageBanner::set_global deduplicates by text; before creating a new banner in keyword_search (where you call MessageBanner::set_global and assign to self.operation_banner), clear or remove any existing banner handle stored in self.operation_banner (e.g., if let Some(prev) = self.operation_banner.take() { prev.close() } or call the banner's removal API) so the old banner is removed and the new handle gets a fresh created_at; then call MessageBanner::set_global and store the new handle in self.operation_banner as you already do. ``` </details> </blockquote></details> <details> <summary>src/ui/tokens/pause_tokens_screen.rs-91-117 (1)</summary><blockquote> `91-117`: _⚠️ Potential issue_ | _🟡 Minor_ **Fix “Burning” wording in pause authorization errors.** These messages are shown on the Pause screen and should say “pause,” not “burn.” <details> <summary>Suggested text fix</summary> ```diff - set_error_banner("Burning is not allowed on this token"); + set_error_banner("Pausing is not allowed on this token"); ... - "You are not allowed to burn this token. Only the contract owner is.", + "You are not allowed to pause this token. Only the contract owner is.", ... - set_error_banner("You are not allowed to burn this token"); + set_error_banner("You are not allowed to pause this token"); ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@src/ui/tokens/pause_tokens_screen.rs` around lines 91 - 117, Update the user-facing error strings in the pause authorization checks to refer to "pause" instead of "burn": modify the messages passed to set_error_banner in the AuthorizedActionTakers match arms (the "Burning is not allowed on this token", "You are not allowed to burn this token", and "You are not allowed to burn this token. Only the contract owner is.") so they read e.g. "Pausing is not allowed on this token", "You are not allowed to pause this token", and "You are not allowed to pause this token. Only the contract owner is." Reference set_error_banner, identity_token_info, and the AuthorizedActionTakers::NoOne/ContractOwner/Identity branches to locate and change the strings. ``` </details> </blockquote></details> <details> <summary>src/ui/tokens/resume_tokens_screen.rs-279-286 (1)</summary><blockquote> `279-286`: _⚠️ Potential issue_ | _🟡 Minor_ **Handle both Error and Warning messages to prevent stuck "resuming" state.** The `display_message` method currently only clears `refresh_banner` and sets error state for `Error` messages. However, `Warning` messages are also emitted in the UI (they don't auto-dismiss in the banner), and should be treated identically to prevent the "Resuming tokens..." banner from continuing indefinitely. This pattern is already established across multiple similar screen implementations (send_screen, add_contracts_screen, add_token_by_id_screen, etc.) which all handle both `Error` and `Warning` variants together. <details> <summary>✅ Suggested change</summary> ```diff - if let MessageType::Error = message_type { + if matches!(message_type, MessageType::Error | MessageType::Warning) { if let Some(h) = self.refresh_banner.take() { h.clear(); } self.status = ResumeTokensStatus::Error; } ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@src/ui/tokens/resume_tokens_screen.rs` around lines 279 - 286, The display_message method only handles MessageType::Error; update it to treat MessageType::Warning the same way so warning banners also clear and the UI doesn't stay in "Resuming tokens..." state—locate the display_message function and change the conditional on MessageType (e.g., use a match or matches!() to check for MessageType::Error or MessageType::Warning), then keep the existing behavior: take and clear self.refresh_banner (if Some) and set self.status = ResumeTokensStatus::Error. ``` </details> </blockquote></details> <details> <summary>src/ui/contracts_documents/contracts_documents_screen.rs-213-218 (1)</summary><blockquote> `213-218`: _⚠️ Potential issue_ | _🟡 Minor_ **Clear any active query banner on parse errors.** If a user submits an invalid query while a previous “Querying documents…” banner is active, the stale banner remains visible. Clearing it here avoids misleading UI state. <details> <summary>🐛 Suggested fix</summary> ```diff Err(e) => { + if let Some(h) = self.query_banner.take() { + h.clear(); + } self.document_query_status = DocumentQueryStatus::Error; MessageBanner::set_global( ui.ctx(), &format!("Failed to parse query properly: {}", e), MessageType::Error, ); } ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@src/ui/contracts_documents/contracts_documents_screen.rs` around lines 213 - 218, On parse error update, clear any existing global banner before setting the error banner to avoid leaving the prior "Querying documents..." message visible: in the block where you set self.document_query_status = DocumentQueryStatus::Error and call MessageBanner::set_global(ui.ctx(), ...), first call the MessageBanner method that removes/clears the global banner (e.g., MessageBanner::clear_global(ui.ctx()) or the project's equivalent) and then set the new error banner via MessageBanner::set_global; keep the same context ui.ctx() and the formatted error text. ``` </details> </blockquote></details> <details> <summary>src/ui/dashpay/contacts_list.rs-910-913 (1)</summary><blockquote> `910-913`: _⚠️ Potential issue_ | _🟡 Minor_ **Show a user-facing error banner when hide/unhide fails.** Right now failures only log, so the user gets no feedback. Consider showing a global error banner in addition to logging. <details> <summary>🐛 Suggested fix</summary> ```diff +use crate::ui::components::MessageBanner; ``` ```diff if let Some(identity) = &self.selected_identity { let owner_id = identity.identity.id(); if let Err(e) = self.app_context.db.set_contact_hidden( &owner_id, &contact.identity_id, new_hidden, ) { + MessageBanner::set_global( + ui.ctx(), + &format!("Failed to update contact: {}", e), + MessageType::Error, + ); tracing::error!( "Failed to update contact: {}", e ); } else { ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@src/ui/dashpay/contacts_list.rs` around lines 910 - 913, The error handler that currently only calls tracing::error!("Failed to update contact: {}", e) should also surface a user-facing global error banner: keep the tracing::error log, then call or create a UI helper (e.g. show_error_banner or AppState::show_global_error) to display a concise message like "Failed to hide/unhide contact" plus the error details or a short code; do this in the same error branch where the variable e is available so the user sees feedback when the hide/unhide operation fails. ``` </details> </blockquote></details> <details> <summary>src/ui/tokens/unfreeze_tokens_screen.rs-115-132 (1)</summary><blockquote> `115-132`: _⚠️ Potential issue_ | _🟡 Minor_ **Update banner copy to match “unfreeze” semantics.** These messages still reference “burning,” which is misleading in the unfreeze flow. <details> <summary>Proposed wording fix</summary> ```diff - set_error_banner("Burning is not allowed on this token"); + set_error_banner("Unfreezing is not allowed on this token"); ``` ```diff - "You are not allowed to burn this token. Only the contract owner is.", + "You are not allowed to unfreeze this token. Only the contract owner is.", ``` ```diff - set_error_banner("You are not allowed to burn this token"); + set_error_banner("You are not allowed to unfreeze this token"); ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@src/ui/tokens/unfreeze_tokens_screen.rs` around lines 115 - 132, The error banners in the AuthorizedActionTakers match arms (variants NoOne, ContractOwner, Identity) use "burn" language but this flow is for unfreezing; update calls to set_error_banner to use unfreeze wording (e.g., "Unfreezing is not allowed on this token", "You are not allowed to unfreeze this token", and for ContractOwner: "You are not allowed to unfreeze this token. Only the contract owner is.") while keeping the same guard checks against identity_token_info and without changing control flow in those branches. ``` </details> </blockquote></details> </blockquote></details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
* refactor(context): replace RwLock<Sdk> with ArcSwap<Sdk> Sdk is internally thread-safe (Arc, ArcSwapOption, atomics) and all methods take &self. The RwLock was adding unnecessary contention across backend tasks. Using ArcSwap instead of plain Sdk because reinit_core_client_and_sdk() needs to atomically swap the entire Sdk instance when config changes. ArcSwap provides lock-free reads with atomic swap for the rare write. Suggested-by: lklimek * fix: address CodeRabbit review findings for ArcSwap migration - Fix import ordering: move arc_swap::ArcSwap before crossbeam_channel - Remove redundant SDK loads in load_identity_from_wallet, register_dpns_name, and load_identity — use the sdk parameter already passed to these functions - Fix stale TODO referencing removed sdk.read().unwrap() API - Rename sdk_guard → sdk in transfer, withdraw_from_identity, and refresh_loaded_identities_dpns_names (no longer lock guards) - Pass &sdk to run_platform_info_task from dispatch site instead of reloading internally - Fix leftover sdk.write() call in context_provider.rs (RwLock remnant) - Add missing Color32 import in wallets dialogs Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * refactor: address remaining CodeRabbit review feedback on ArcSwap migration - Move SDK load outside for loop in refresh_loaded_identities_dpns_names.rs so it's loaded once for the batch instead of on each iteration - Update stale TODO comment in default_platform_version() to reflect that this is a free function with no sdk access Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * refactor: consolidate double read-lock on spv_context_provider Clone the SPV provider in a single lock acquisition, then bind app context on the clone instead of locking twice. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> --------- Co-authored-by: PastaClaw <thepastaclaw@users.noreply.github.com> Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
* refactor: remove unused Insight API references The `insight_api_url` field in `NetworkConfig` and its associated `insight_api_uri()` method were never used in production code (both marked `#[allow(dead_code)]`). Remove the field, method, config entries, env example lines, and related tests. https://claude.ai/code/session_01HWPmCJHT8KTZGP9bFiksjn * refactor: remove unused `show_in_ui` field from NetworkConfig The `show_in_ui` field was defined on `NetworkConfig` and serialized in `save()`, but never read by any production code to control network visibility. Remove the field, its serialization, env example lines, and test references. https://claude.ai/code/session_01HWPmCJHT8KTZGP9bFiksjn * fix: add missing `Color32` import in wallet dialogs https://claude.ai/code/session_01HWPmCJHT8KTZGP9bFiksjn --------- Co-authored-by: Claude <noreply@anthropic.com>
* build: remove snap version * build: add Flatpak packaging and CI workflow Add Flatpak build manifest, desktop entry, AppStream metadata, and GitHub Actions workflow for building and distributing Flatpak bundles. Uses freedesktop 25.08 runtime with rust-stable and llvm21 extensions. No application source code changes required - works in SPV mode by default. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * build: address review findings for Flatpak packaging - Pin GitHub Actions to commit SHAs for supply chain security - Upgrade softprops/action-gh-release from v1 to v2.2.2 - Remove redundant --socket=x11 (fallback-x11 suffices) - Remove duplicate tag trigger preventing double builds on release - Remove duplicate env vars inherited from top-level build-options - Add Flatpak build artifacts to .gitignore - Add bugtracker URL to AppStream metainfo - Remove deprecated <categories> from metainfo (use .desktop instead) - Add Terminal=false and Keywords to desktop entry - Add disk space check after SDK install in CI - Rename artifact to include architecture suffix Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * build: simplify CI workflows for Linux-only releases - Remove "Free disk space" step from flatpak and release workflows - Remove Windows and macOS builds from release workflow - Use "ubuntu" runner image instead of pinned versions - Clean up unused matrix.ext references Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * build: attach to existing releases instead of creating new ones - Replace release-creating job with attach-to-release (only on release event) - Add 14-day retention for build artifacts - On tag push or workflow_dispatch, only upload artifacts (no release) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * revert: restore release.yml to original v1.0-dev version The release workflow changes were out of scope for the Flatpak PR. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: address CodeRabbit review comments - Fix CRLF line endings in Flatpak manifest (convert to LF) - Set app_id on ViewportBuilder to match desktop StartupWMClass - Use --locked flag for reproducible cargo builds in Flatpak - Rename --repo=repo to --repo=flatpak-repo to match .gitignore - Add architecture note for protoc x86_64 binary Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * docs: add Flatpak install instructions to README Add a dedicated section for installing via Flatpak on Linux, clarify that prerequisites are only needed for building from source, and rename "Installation" to "Build from Source" for clarity. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: match StartupWMClass to Flatpak app_id Use reverse-DNS format org.dash.DashEvoTool to match the Wayland app_id set via ViewportBuilder::with_app_id(). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: use ** glob for branch trigger to match feat/flatpak Single * doesn't match path separators in GitHub Actions branch filters. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * feat: add aarch64 Flatpak build and caching to CI - Add matrix strategy for parallel x86_64 and aarch64 builds - Patch protoc URL/sha256 per architecture at build time - Cache .flatpak-builder directory keyed on arch + manifest + lockfile - Pin actions/cache to SHA Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: convert desktop and metainfo files to LF line endings Flatpak builder validates desktop files and rejects CRLF line endings. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * build: cancel in-progress Flatpak builds on new push Add concurrency group keyed on git ref so a new push cancels any running build for the same branch or release. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: address PR review findings for Flatpak packaging - Remove unnecessary --filesystem=xdg-config/dash-evo-tool:create (Flatpak already redirects XDG_CONFIG_HOME to sandbox) - Add categories and keywords to AppStream metainfo for discoverability - Update README with both x86_64/aarch64 install commands, uninstall instructions, and Flatpak data path note - Clarify aarch64 comment in manifest to reference CI sed patching Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * chore: workflow timeout and perms * fix: move permissions to job level in Flatpak workflow Step-level permissions are not valid in GitHub Actions. Move contents: write to the job level where it is needed for release attachment. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * build: cache Cargo registry and target in Flatpak CI Bind-mount host-side cargo-cache and cargo-target directories into the Flatpak build sandbox so CARGO_HOME and target/ persist across builds. Uses split restore/save with cleanup of incremental and registry/src (similar to Swatinem/rust-cache). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: scope cargo cache bind-mount sed to build-args only The previous sed matched --share=network in both finish-args and build-args, corrupting finish-args. Use a sed range to only target the build-args section. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * Apply suggestions from code review --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
lklimek
force-pushed
the
design/unified-message-display-applied
branch
from
75c8e63 to
95a1809
Compare
Aligns elapsed display with the countdown timer which already adds 1 to avoid showing "0s" immediately. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
lklimek
force-pushed
the
design/unified-message-display-applied
branch
from
95a1809 to
e0f0ce9
Compare
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…sage-display-applied
Resolves conflict in mint_tokens_screen.rs by combining both Warning handling from the base branch and refresh_banner clearing from HEAD. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Merge v1.0-dev (d45b3ee) into the unified message display branch, resolving 45 conflicted files across the codebase. Resolution strategy: - Keep the unified MessageBanner approach (design branch) for all screen-level message display, replacing v1.0-dev's inline colored labels and per-screen message fields - Keep simplified status enums without timestamp parameters, since MessageBanner handles elapsed-time display via BannerHandle - Merge v1.0-dev's MessageBanner enhancements: details/suggestion fields, collapsible details section, tracing-based logging, BannerState::new()/reset_to() constructors, distinct error icon - Merge v1.0-dev's batch refresh counting in IdentitiesScreen (pending_refresh_count, total_refresh_count, pluralized messages) - Merge v1.0-dev's ContactDetailsScreen database loading and Platform persistence via backend tasks - Merge v1.0-dev's improved UX text in ContactRequests (informative label instead of non-functional Cancel button) - Fix post-merge issues: remove references to deleted `message` field in ContactDetailsScreen, fix infinite recursion in display_message https://claude.ai/code/session_015EEVFee5cXpgaGASfZcAN3
Guard side effects with Error|Warning match, use take_and_clear(), and remove redundant MessageBanner::set_global() call in 4 screens. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ssage-display-applied # Conflicts: # src/app.rs # src/ui/components/message_banner.rs
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
src/context/connection_status.rs (1)
351-377:⚠️ Potential issue | 🟠 MajorHandle TaskResult::Error in chainlock polling to prevent stale connection state.
When
GetBestChainLocksfails (e.g., Config::load fails at line 169 ofsrc/backend_task/core/mod.rs), it sendsTaskResult::Error, whichhandle_task_resultignores entirely. This leavesrpc_onlineunchanged from its previous value, so a prior success can leave the state incorrectly showing "connected" even when polling has failed. The error appears only as a message banner, not reflected in the connection status indicator.Reset
rpc_onlineto false when handlingTaskResult::Errorfor chainlock-related tasks, or distinguish chainlock errors in the task result metadata to update state appropriately.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/context/connection_status.rs` around lines 351 - 377, handle_task_result currently only updates connection state on TaskResult::Success for chainlock messages, so when GetBestChainLocks returns TaskResult::Error the rpc_online flag can remain true; modify handle_task_result to also handle TaskResult::Error for chainlock-related tasks by calling self.set_rpc_online(false) (and self.refresh_state() if appropriate) when the failing task corresponds to GetBestChainLocks / CoreItem::ChainLock(s). Locate the match on TaskResult in handle_task_result and add an arm or branch that detects TaskResult::Error (or inspects task metadata indicating CoreItem::ChainLock/GetBestChainLocks) and clears rpc_online to prevent stale "connected" state. Ensure you reference and use set_rpc_online(false) and refresh_state() from the same context where update_from_chainlocks and the existing CoreItem::ChainLock handling live.src/ui/tools/platform_info_screen.rs (1)
271-293:⚠️ Potential issue | 🟠 MajorSingle-flight task tracking is inconsistent with current result handling.
Line 282 infers the completed task by scanning
active_tasks, and Line 292 clears all active tasks. If multiple requests are triggered, results can be mislabeled and loading can stop prematurely.💡 Proposed fix (enforce single in-flight request)
fn trigger_task( &mut self, task_type: PlatformInfoTaskRequestType, task_name: &str, ) -> AppAction { - if !self.active_tasks.contains(task_name) { + if self.active_tasks.is_empty() && !self.active_tasks.contains(task_name) { self.active_tasks.insert(task_name.to_string()); let task = BackendTask::PlatformInfo(task_type); return AppAction::BackendTask(task); } AppAction::None }for (task_id, button_text, task_type) in button_tasks { - let is_loading = self.active_tasks.contains(task_id); + let is_loading = !self.active_tasks.is_empty();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/tools/platform_info_screen.rs` around lines 271 - 293, The code infers which task completed by scanning self.active_tasks and then clears all active tasks, which can mislabel results if multiple requests are in-flight; to fix, enforce a single in-flight request or match completions to a tracked task id: introduce/use a single tracked identifier (e.g., replace or supplement self.active_tasks with a self.current_task_id or ensure only one entry is pushed) when dispatching tasks, on completion only set self.current_result and self.current_result_title if the completed task id equals self.current_task_id (or if active_tasks.len() == 1), and only clear that specific task (or reset self.current_task_id) instead of calling self.active_tasks.clear(); update the logic around the task_names loop, the place that inserts into self.active_tasks, and where current_result/current_result_title are assigned to perform this exact-match check.src/ui/wallets/send_screen.rs (1)
1116-1124:⚠️ Potential issue | 🟠 MajorError state lacks context and recovery guidance.
On Line 1116, the error branch only renders a bare “Dismiss” button. If the banner has already dismissed, users lose context for what failed.
💡 Suggested improvement
SendStatus::Error => { - // Error message is displayed by the global MessageBanner. - // Show a dismiss/retry option. + // Error details are in the global MessageBanner. + // Keep local context so recovery is obvious even after banner timeout. ui.add_space(10.0); - if ui.button("Dismiss").clicked() { + ui.label( + RichText::new("Send failed. See the banner above for details.") + .color(DashColors::ERROR), + ); + ui.add_space(6.0); + if ui.button("Back to form").clicked() { self.send_status = SendStatus::NotStarted; } ui.add_space(10.0); None }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/wallets/send_screen.rs` around lines 1116 - 1124, The SendStatus::Error branch currently only shows a "Dismiss" button and loses error context if the global MessageBanner is gone; update the SendStatus::Error handling in send_screen.rs to surface the error and recovery actions by displaying a concise error summary (pulling from the same source MessageBanner uses or from a stored error field on the struct) and adding both a "Retry" button that re-attempts the send (calling the existing send method or state transition) and a "Dismiss" that clears self.send_status back to SendStatus::NotStarted; ensure the UI also offers a "Details" toggle or tooltip to reveal the full error message for advanced users so they don't lose context when the banner is closed.src/ui/dpns/dpns_contested_names_screen.rs (1)
1829-1845:⚠️ Potential issue | 🟠 MajorError path clears banners but leaves operation states stuck.
On Lines 1831-1834, error/warning handling does not reset
refreshing_statusor vote workflow status. This can block refresh retries and leave bulk voting in an in-progress state after failures.🩹 Suggested state-reset fix
fn display_message(&mut self, message: &str, message_type: MessageType) { // Banner display is handled globally by AppState; this is only for side-effects. if matches!(message_type, MessageType::Error | MessageType::Warning) { self.refresh_banner.take_and_clear(); self.vote_banner.take_and_clear(); + self.refreshing_status = RefreshingStatus::NotRefreshing; + if matches!( + self.bulk_vote_handling_status, + VoteHandlingStatus::CastingVotes | VoteHandlingStatus::SchedulingVotes + ) { + self.bulk_vote_handling_status = VoteHandlingStatus::Failed(message.to_string()); + } } if message.contains("Error casting scheduled vote") { self.scheduled_vote_cast_in_progress = false;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/dpns/dpns_contested_names_screen.rs` around lines 1829 - 1845, In display_message, after clearing banners via refresh_banner.take_and_clear() and vote_banner.take_and_clear(), also reset the view and workflow state so retries and vote flows aren't left "in-progress": set self.refreshing_status back to the idle/ready variant your RefreshingStatus enum uses, set self.scheduled_vote_cast_in_progress = false (already done for the specific error) and also clear any global vote workflow flags (e.g., self.bulk_vote_in_progress = false) and iterate any in-progress entries in self.scheduled_votes and any vote_workflows collections to mark them as Failed (similar to how you set ScheduledVoteCastingStatus::Failed) so no operation remains stuck.src/ui/identities/add_existing_identity_screen.rs (1)
670-702:⚠️ Potential issue | 🟠 MajorClear the loading banner when local index validation fails.
On Line 673 a loading banner starts before parsing, but when parsing fails (Line 695), the
refresh_bannerhandle is never cleared. This leaves a stale “Loading identity...” banner despite no backend task being started.🛠️ Suggested fix
if ui.add(button).clicked() { - self.add_identity_status = AddIdentityStatus::WaitingForResult; self.success_message = None; - let handle = MessageBanner::set_global( - self.app_context.egui_ctx(), - "Loading identity...", - MessageType::Info, - ); - handle.with_elapsed(); - self.refresh_banner = Some(handle); // Parse identity index input if let Ok(identity_index) = self.identity_index_input.trim().parse::<u32>() { + self.add_identity_status = AddIdentityStatus::WaitingForResult; + let handle = MessageBanner::set_global( + self.app_context.egui_ctx(), + "Loading identity...", + MessageType::Info, + ); + handle.with_elapsed(); + self.refresh_banner = Some(handle); + let wallet_ref = self.selected_wallet.as_ref().unwrap().clone().into(); action = AppAction::BackendTask(BackendTask::IdentityTask( match self.wallet_search_mode { WalletIdentitySearchMode::SpecificIndex => { IdentityTask::SearchIdentityFromWallet(wallet_ref, identity_index) @@ } else { // Handle invalid index input + self.refresh_banner.take_and_clear(); self.add_identity_status = AddIdentityStatus::NotStarted; MessageBanner::set_global( self.app_context.egui_ctx(), "Invalid identity index", MessageType::Error, ); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/identities/add_existing_identity_screen.rs` around lines 670 - 702, When parsing identity_index_input fails you must clear the loading banner so it doesn't remain visible; in the else branch after setting add_identity_status back to NotStarted, remove/close the existing MessageBanner handle stored in self.refresh_banner (e.g. take the Option, call the handle's close/clear method or call a MessageBanner::clear_global helper) and set self.refresh_banner to None before showing the "Invalid identity index" error via MessageBanner::set_global; update code touching self.refresh_banner and the else branch around identity_index_input parsing to do this.
♻️ Duplicate comments (1)
src/ui/wallets/create_asset_lock_screen.rs (1)
673-675:⚠️ Potential issue | 🟠 Major
display_messageno-op can leave the flow stuck after backend errors.When an error arrives during
WalletFundedScreenStep::WaitingForAssetLock, Line 673 currently does nothing, so the screen can remain in a perpetual waiting state.💡 Proposed fix (preserve global banner, restore local state)
fn display_message(&mut self, _message: &str, _message_type: MessageType) { - // Error/success display is handled by the global MessageBanner. + // Error/success display is handled by the global MessageBanner. + // Keep side-effects to prevent stale waiting states. + if matches!(_message_type, MessageType::Error | MessageType::Warning) { + self.is_creating = false; + if let Ok(mut step) = self.step.write() + && *step == WalletFundedScreenStep::WaitingForAssetLock + { + *step = WalletFundedScreenStep::FundsReceived; + } + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/wallets/create_asset_lock_screen.rs` around lines 673 - 675, display_message currently no-ops causing the UI to stay stuck during WalletFundedScreenStep::WaitingForAssetLock errors; modify display_message to forward the message to the global MessageBanner (preserve existing banner behavior) and also update the local screen state (e.g., clear the WaitingForAssetLock flag or set the screen step to an error/idle state) so the local flow can proceed or show retry UI; locate and update the display_message method in create_asset_lock_screen.rs and adjust state fields used by WalletFundedScreenStep::WaitingForAssetLock (or the local waiting flag) accordingly.
🟠 Major comments (20)
src/ui/dashpay/qr_scanner.rs-260-260 (1)
260-260:⚠️ Potential issue | 🟠 MajorDo not surface raw wallet errors directly to users.
This currently displays backend/internal error text as-is. Please sanitize to a stable user-facing message.
Proposed fix
- if let Err(e) = try_open_wallet_no_password(wallet) { - MessageBanner::set_global(ui.ctx(), &e, MessageType::Error); + if let Err(_e) = try_open_wallet_no_password(wallet) { + MessageBanner::set_global( + ui.ctx(), + "Unable to access the selected wallet. Please unlock it and try again.", + MessageType::Error, + ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/dashpay/qr_scanner.rs` at line 260, The code is passing the raw backend error variable `e` into `MessageBanner::set_global(ui.ctx(), &e, MessageType::Error)` which surfaces internal error text to users; change this to show a stable, sanitized user-facing message (e.g. "Unable to access wallet. Please try again or contact support.") while logging the original `e` to the application logs for diagnostics (use your existing logger/tracing facility). Update the call site in `qr_scanner.rs` (the `MessageBanner::set_global` invocation) to pass the sanitized string and add a separate log statement that records `e` at an appropriate level.src/ui/identities/keys/key_info_screen.rs-513-514 (1)
513-514:⚠️ Potential issue | 🟠 MajorDon’t surface raw internal errors directly in user banners.
These banners currently expose raw error strings from wallet/storage/validation paths. Prefer a user-safe message and log full details separately.
🔒 Suggested hardening
- MessageBanner::set_global(ui.ctx(), &e, MessageType::Error); + tracing::warn!("try_open_wallet_no_password failed: {}", e); + MessageBanner::set_global( + ui.ctx(), + "Unable to access wallet right now. Please try again.", + MessageType::Error, + );- MessageBanner::set_global( - self.app_context.egui_ctx(), - format!("Issue saving: {}", e), - MessageType::Error, - ); + tracing::warn!("update_local_qualified_identity failed: {}", e); + MessageBanner::set_global( + self.app_context.egui_ctx(), + "Failed to save changes.", + MessageType::Error, + );Also applies to: 635-639, 651-655, 801-805
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/identities/keys/key_info_screen.rs` around lines 513 - 514, Several MessageBanner::set_global(...) calls are currently passing raw internal error strings to the UI; replace those raw errors with a concise, user-safe message like "An unexpected error occurred" (or context-specific friendly text) and send the full error details to your logging system instead (e.g., tracing::error! or log::error!) so internals aren't exposed. Locate each usage of MessageBanner::set_global in key_info_screen.rs (the calls noted around the current occurrences) and change the banner text to a sanitized message while calling the logger with the original error value for diagnostics. Ensure any error formatting is not displayed to users and reuse the same pattern for the other similar blocks in this file.src/ui/identities/keys/key_info_screen.rs-642-656 (1)
642-656:⚠️ Potential issue | 🟠 MajorPersist-first (or rollback) to avoid in-memory/persisted state divergence.
Both add/remove flows mutate
self.identity/self.private_key_databefore persistence. Ifupdate_local_qualified_identityfails, the UI state no longer reflects stored state.💡 Suggested fix (transactional update pattern)
- self.private_key_data = Some((PrivateKeyData::Clear(private_key_bytes), None)); - self.identity.private_keys.insert_non_encrypted( + let mut updated_identity = self.identity.clone(); + updated_identity.private_keys.insert_non_encrypted( (self.key.purpose().into(), self.key.id()), (self.key.clone().into(), private_key_bytes), ); - if let Err(e) = self.app_context.update_local_qualified_identity(&self.identity) { + if let Err(e) = self.app_context.update_local_qualified_identity(&updated_identity) { MessageBanner::set_global( self.app_context.egui_ctx(), - format!("Issue saving: {}", e), + "Failed to save private key changes.", MessageType::Error, ); + return; } + self.identity = updated_identity; + self.private_key_data = Some((PrivateKeyData::Clear(private_key_bytes), None));- self.private_key_data = None; - self.identity.private_keys.private_keys.remove(&(self.key.purpose().into(), self.key.id())); - if let Err(e) = self.app_context.update_local_qualified_identity(&self.identity) { + let mut updated_identity = self.identity.clone(); + updated_identity + .private_keys + .private_keys + .remove(&(self.key.purpose().into(), self.key.id())); + if let Err(e) = self.app_context.update_local_qualified_identity(&updated_identity) { MessageBanner::set_global( ui.ctx(), - format!("Issue saving: {}", e), + "Failed to remove private key.", MessageType::Error, ); + return; } + self.identity = updated_identity; + self.private_key_data = None;Also applies to: 792-807
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/identities/keys/key_info_screen.rs` around lines 642 - 656, The code mutates in-memory state (self.private_key_data and self.identity via identity.private_keys.insert_non_encrypted) before calling update_local_qualified_identity, causing UI/state divergence if persistence fails; change to a transactional pattern: perform the persistence call update_local_qualified_identity first using the new key data (or construct the updated identity/keys to pass), and only on success update self.private_key_data and self.identity in memory (or if you must mutate first, catch errors and rollback by removing the inserted key from identity.private_keys and resetting self.private_key_data); refer to symbols identity.private_keys.insert_non_encrypted, self.private_key_data, and update_local_qualified_identity to locate and apply the fix (also apply same fix to the analogous block around lines 792-807).src/ui/components/message_banner.rs-309-331 (1)
309-331:⚠️ Potential issue | 🟠 MajorText-based dedup can alias independent operations to the same handle.
Line 316 returns the existing banner key for matching text. Then Line 775 clears by key, so one caller can clear another caller’s in-flight banner if both used identical text.
Also applies to: 772-776
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/components/message_banner.rs` around lines 309 - 331, The dedup logic in set_global (using get_banners / set_banners) returns an existing banner's key when text matches, allowing one caller to clear another caller's banner via BannerHandle.key; change this so text-only matches do not reuse the existing.key: when a banner with the same text is found, do not return its key—either update its message_type in-place but create a new Banner entry with a freshly generated key (or otherwise generate a new unique key for the caller) and push it to banners, then call set_banners(ctx, banners) and return a BannerHandle with the new key; reference the set_global function, existing.key, BannerHandle and get_banners/set_banners when locating where to implement the change.src/ui/tokens/freeze_tokens_screen.rs-333-338 (1)
333-338:⚠️ Potential issue | 🟠 MajorScope
display_messageside-effects to active freeze operations only.Lines 335-338 set
FreezeTokensStatus::Errorfor any warning/error. Unrelated global messages can incorrectly change this screen’s state.Suggested fix
fn display_message(&mut self, _message: &str, message_type: MessageType) { // Banner display is handled globally by AppState; this is only for side-effects. if matches!(message_type, MessageType::Error | MessageType::Warning) { - self.refresh_banner.take_and_clear(); - self.status = FreezeTokensStatus::Error; + if matches!(self.status, FreezeTokensStatus::WaitingForResult) { + self.refresh_banner.take_and_clear(); + self.status = FreezeTokensStatus::Error; + } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/tokens/freeze_tokens_screen.rs` around lines 333 - 338, display_message currently clears refresh_banner and sets status to FreezeTokensStatus::Error for any Warning/Error; restrict these side-effects to only when the screen has an active freeze operation. Update display_message (the method shown) to first check that this screen is handling an active freeze (e.g. verify a pending refresh via self.refresh_banner.is_some() or an existing active/freezing flag on the struct) and only then call self.refresh_banner.take_and_clear() and set self.status = FreezeTokensStatus::Error; otherwise ignore unrelated global messages. Ensure you reference the existing symbols display_message, self.refresh_banner, and FreezeTokensStatus::Error when making the change.src/ui/dashpay/qr_code_generator.rs-125-129 (1)
125-129:⚠️ Potential issue | 🟠 MajorSanitize backend error details before showing QR generation failures.
On Line 127, the raw error is shown directly to users. This can leak internal details and unstable diagnostics.
Suggested fix
Err(e) => { - MessageBanner::set_global( - self.app_context.egui_ctx(), - format!("Failed to generate QR code: {}", e), - MessageType::Error, - ); + tracing::error!("Failed to generate QR code: {e}"); + MessageBanner::set_global( + self.app_context.egui_ctx(), + "Failed to generate QR code. Please try again.", + MessageType::Error, + ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/dashpay/qr_code_generator.rs` around lines 125 - 129, The code currently passes the raw backend error into MessageBanner::set_global (call using self.app_context.egui_ctx() and format!("Failed to generate QR code: {}", e)), which can leak internal details; change it to show a generic, sanitized user-facing message like "Failed to generate QR code. Please try again or contact support." and move the detailed error into a developer log (e.g., use log::error! or processLogger.error) so MessageBanner only displays the generic text while the original error `e` is recorded in logs for debugging.src/ui/tools/transition_visualizer_screen.rs-394-399 (1)
394-399:⚠️ Potential issue | 🟠 MajorDo not reset broadcast state on unrelated warning/error messages.
Lines 394-397 currently clear
submit_bannerand reset toNotStartedfor any warning/error. This can interrupt active flow and re-enable submit prematurely.Suggested fix
- fn display_message(&mut self, _message: &str, message_type: MessageType) { + fn display_message(&mut self, message: &str, message_type: MessageType) { // Banner display is handled globally by AppState; this is only for side-effects. match message_type { MessageType::Success => { if matches!(self.broadcast_status, TransitionBroadcastStatus::Submitting) { self.submit_banner.take_and_clear(); self.broadcast_status = TransitionBroadcastStatus::Complete(Instant::now()); } } MessageType::Error | MessageType::Warning => { - self.submit_banner.take_and_clear(); - self.broadcast_status = TransitionBroadcastStatus::NotStarted; + if matches!(self.broadcast_status, TransitionBroadcastStatus::Submitting) { + self.submit_banner.take_and_clear(); + self.broadcast_status = + TransitionBroadcastStatus::Error(message.to_string(), Instant::now()); + } } MessageType::Info => {} } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/tools/transition_visualizer_screen.rs` around lines 394 - 399, The current match arm for MessageType::Error | MessageType::Warning unconditionally calls self.submit_banner.take_and_clear() and resets self.broadcast_status to TransitionBroadcastStatus::NotStarted, which disrupts active broadcasts; change this to only clear the banner and reset broadcast_status when the warning/error is actually related to the transition broadcast. Concretely, in the MessageType::Error | MessageType::Warning branch add a guard that verifies the submit_banner corresponds to a broadcast failure (e.g. check a new or existing metadata field on self.submit_banner, or inspect the banner message text via a helper like submit_banner.is_broadcast_error()) and only then call self.submit_banner.take_and_clear() and set self.broadcast_status = TransitionBroadcastStatus::NotStarted; otherwise leave the banner and broadcast_status untouched. Ensure you update or add the minimal helper on the submit_banner type if necessary to identify broadcast-related messages.src/ui/contracts_documents/document_action_screen.rs-512-516 (1)
512-516:⚠️ Potential issue | 🟠 MajorReset action state when document ID parsing fails.
Line 512 and Line 581 only show a banner; they don’t clear previously fetched state. That leaves stale
fetched_price/original_docpaths active and can enable unintended broadcasts after invalid input.Suggested fix
} else { MessageBanner::set_global( ui.ctx(), "Invalid Document ID format", MessageType::Error, ); + self.fetched_price = None; + self.broadcast_status = BroadcastStatus::NotBroadcasted; }} else { MessageBanner::set_global( ui.ctx(), "Invalid Document ID format", MessageType::Error, ); + self.original_doc = None; + self.field_inputs.clear(); + self.broadcast_status = BroadcastStatus::NotBroadcasted; }Also applies to: 581-585
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/contracts_documents/document_action_screen.rs` around lines 512 - 516, When parsing the Document ID fails (where MessageBanner::set_global("Invalid Document ID format", MessageType::Error) is called) also reset the action state to avoid stale data: clear or set to None the fetched_price and original_doc fields and reset any broadcast/enabled flags or paths that depend on a valid document (e.g., disable/clear broadcast_path or set can_broadcast = false). Apply the same reset logic at the other parse-failure site where the banner is shown so both code paths clear fetched_price, original_doc and any broadcast-related state in the DocumentActionScreen struct before returning.src/ui/tokens/mod.rs-24-30 (1)
24-30:⚠️ Potential issue | 🟠 MajorDo not downgrade identity-load failures to an empty list.
Line 29 (
unwrap_or_default) masks load/corruption errors and allows token flows to proceed with silently missing identities. This should return/propagate the error so callers can bail explicitly.Suggested direction
-pub fn load_identities_with_banner(app_context: &AppContext) -> Vec<QualifiedIdentity> { +pub fn load_identities_with_banner( + app_context: &AppContext, +) -> Result<Vec<QualifiedIdentity>, String> { use crate::ui::components::ResultBannerExt; app_context .load_local_qualified_identities() .or_show_error(app_context.egui_ctx()) - .unwrap_or_default() }Then make callers early-return on
Err.Based on learnings: In
src/database/identities.rs, identity-loading paths intentionally abort on the first corrupted blob because skipping/masking corruption can lead to fund loss.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/tokens/mod.rs` around lines 24 - 30, In load_identities_with_banner replace the current unwrap_or_default behavior so it returns a Result<Vec<QualifiedIdentity>, E> (propagating the error from app_context.load_local_qualified_identities().or_show_error(...)) instead of silently returning an empty list; update the function signature and callers to handle Err (early-return where identities are required) and preserve the use of or_show_error/app_context.egui_ctx() when converting or displaying the error from load_local_qualified_identities.src/app.rs-1030-1037 (1)
1030-1037:⚠️ Potential issue | 🟠 MajorDo not force all
BackendTaskSuccessResult::Messagepayloads into a success banner.Line 1035 always shows a success banner, but these message payloads are also interpreted as errors by some screens, causing contradictory UX (success flash followed by error). Route message severity explicitly instead of hardcoding
MessageType::Success.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app.rs` around lines 1030 - 1037, Currently BackendTaskSuccessResult::Message always calls MessageBanner::set_global with MessageType::Success, which forces every message into a success banner; instead, determine and pass the correct severity for the message (either by adding a severity field to BackendTaskSuccessResult::Message or by extracting severity from the existing unboxed_message) and call MessageBanner::set_global with that computed MessageType rather than MessageType::Success; update the BackendTaskSuccessResult::Message variant and the handling code (the match arm that calls MessageBanner::set_global and visible_screen_mut().display_task_result) so the banner reflects the message's explicit severity.src/ui/tools/masternode_list_diff_screen.rs-4149-4153 (1)
4149-4153:⚠️ Potential issue | 🟠 MajorRemove local error rendering side-effect from
display_messageto avoid duplicate banners.On Line 4152, setting
self.ui_state.errorstill triggers the inline error banner, so users can see both local and global errors for the same event. Keep only non-visual side effects here (e.g., clearing pending state).Suggested minimal fix
fn display_message(&mut self, message: &str, message_type: MessageType) { // Banner display is handled globally by AppState; this is only for side-effects. if matches!(message_type, MessageType::Error | MessageType::Warning) { self.task.pending = None; - self.ui_state.error = Some(message.to_string()); } }As per coding guidelines: "Use
MessageBannerfor user-facing messages and letisland_central_panel()render global banners centrally".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/tools/masternode_list_diff_screen.rs` around lines 4149 - 4153, In display_message (the block matching message_type against MessageType::Error | MessageType::Warning) remove the visual side-effect that sets self.ui_state.error and only perform non-visual updates: keep self.task.pending = None but delete the assignment self.ui_state.error = Some(message.to_string()); let global banner rendering remain handled by AppState/MessageBanner and island_central_panel().src/ui/dashpay/add_contact_screen.rs-623-625 (1)
623-625:⚠️ Potential issue | 🟠 MajorDo not rely on fragile substring matching for backend error classification.
If backend wording changes, Line 626’s heuristic can miss errors and leave the screen stuck in
Sending. Add a fallback that always transitions out of sending state when a message arrives during an active send flow.Suggested guard to avoid sticky Sending state
BackendTaskSuccessResult::Message(message) => { // TODO(RUST-004): Replace string-based error matching with structured // error types through the task result system. This is fragile — if // upstream error wording changes, classification silently breaks. if message.contains("Error") || message.contains("Failed") || message.contains("does not have") { // Try to parse structured error, fallback to generic let error = if message.contains("ENCRYPTION key") { DashPayError::MissingEncryptionKey } else if message.contains("DECRYPTION key") { DashPayError::MissingDecryptionKey } else if message.contains("not found") && message.contains("username") { DashPayError::UsernameResolutionFailed { username: self.username_or_id.clone(), } } else if message.contains("Identity not found") { DashPayError::IdentityNotFound { identity_id: dash_sdk::platform::Identifier::from_string( &self.username_or_id, dash_sdk::dpp::platform_value::string_encoding::Encoding::Base58, ) .unwrap_or_else(|_| dash_sdk::platform::Identifier::random()), } } else if message.contains("Network") || message.contains("connection") { DashPayError::NetworkError { reason: message.clone(), } } else { DashPayError::Internal { message: message.clone(), } }; self.status = ContactRequestStatus::Error(error); + } else if matches!(self.status, ContactRequestStatus::Sending) { + self.status = ContactRequestStatus::Error(DashPayError::Internal { + message: message.clone(), + }); } // Ignore other messages - they're not for this screen }Also applies to: 626-659
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/dashpay/add_contact_screen.rs` around lines 623 - 625, The current backend-error classification in add_contact_screen.rs relies on fragile substring matching of the "Sending" flow (see TODO(RUST-004)); modify the message-handling code (the branch that checks for backend error strings between lines ~626-659) to always clear the sending state as a fallback: whenever any backend message arrives while the UI is in the Sending state, set the UI to a non-sending state (e.g., mark sending=false or transition to Idle/Error/Success as appropriate) so the screen cannot become permanently stuck; keep the planned migration to structured task results (TODO(RUST-004>) but implement this unconditional fallback in the function that processes backend messages/ task results (the handler around the current substring check) and add a debug log for unexpected messages to aid future refactors.src/ui/contracts_documents/group_actions_screen.rs-262-268 (1)
262-268:⚠️ Potential issue | 🟠 MajorAvoid dropping fetched results on row-level action errors.
Line [262] and Line [275] set
fetch_group_actions_status = FetchGroupActionsStatus::Errorwhen preparing a single “Take Action” flow fails. That hides the already-fetched table and forces a refetch, even though fetch itself succeeded.Proposed fix
- self.fetch_group_actions_status = - FetchGroupActionsStatus::Error; MessageBanner::set_global( ui.ctx(), "No identity token balance found", MessageType::Error, ); @@ - self.fetch_group_actions_status = - FetchGroupActionsStatus::Error; MessageBanner::set_global( ui.ctx(), format!("Failed to get identity token info: {}", e), MessageType::Error, );Also applies to: 275-281
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/contracts_documents/group_actions_screen.rs` around lines 262 - 268, The code currently sets self.fetch_group_actions_status = FetchGroupActionsStatus::Error in the row-level "Take Action" failure branches (around the blocks that call MessageBanner::set_global with "No identity token balance found"), which incorrectly hides the already-fetched table and forces a refetch; instead, remove the assignment to fetch_group_actions_status in those error paths and only emit the user-facing error via MessageBanner::set_global(ui.ctx(), ...). If you need to represent per-row action state, set or introduce a separate field (e.g., self.take_group_action_status or an ActionRowStatus enum) and update that instead of FetchGroupActionsStatus, leaving fetch_group_actions_status untouched when fetch succeeded. Ensure you update both error blocks mentioned (the one around lines 262 and the one around lines 275-281) and keep MessageBanner usage and ui.ctx() as-is.src/ui/dashpay/contact_info_editor.rs-288-290 (1)
288-290:⚠️ Potential issue | 🟠 MajorReset
savingon error/warning messages to avoid stuck spinner.After Line [97] sets
self.saving = true, backend failures typically arrive viadisplay_message. With Line [288] as a no-op, the screen can stay in “Saving...” indefinitely.Proposed fix
- pub fn display_message(&mut self, _message: &str, _message_type: MessageType) { + pub fn display_message(&mut self, _message: &str, message_type: MessageType) { // Banner display is handled globally by AppState; this is only for side-effects. + if matches!(message_type, MessageType::Error | MessageType::Warning) { + self.saving = false; + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/dashpay/contact_info_editor.rs` around lines 288 - 290, The display_message method currently does nothing, causing self.saving to remain true after backend failures; update fn display_message(&mut self, _message: &str, message_type: MessageType) to clear the saving flag on non-success outcomes by setting self.saving = false when message_type indicates Error or Warning (leave Success/Info behaviors unchanged), and keep any existing note about global banner handling for AppState; reference the display_message method and the struct field saving to locate where to add this change.src/ui/dashpay/send_payment.rs-786-788 (1)
786-788:⚠️ Potential issue | 🟠 Major
PaymentHistorycan remain stuck in loading state on backend errors.Line [576] sets
loading = true, but Line [786] no longer clears it when failures arrive through the message path.Proposed fix
- pub fn display_message(&mut self, _message: &str, _message_type: MessageType) { + pub fn display_message(&mut self, _message: &str, _message_type: MessageType) { // Banner display is handled globally by AppState; this is only for side-effects. + self.loading = false; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/dashpay/send_payment.rs` around lines 786 - 788, The PaymentHistory display_message implementation currently does nothing, causing self.loading to stay true when backend errors are delivered via messages; update the PaymentHistory::display_message method in src/ui/dashpay/send_payment.rs to clear the loading flag (self.loading = false) and set an appropriate error state when a failure message arrives—inspect the MessageType enum (or the message text) to detect error/failure cases and ensure you update self.error or similar fields and reset any loading indicator so the UI unblocks.src/ui/contracts_documents/group_actions_screen.rs-460-466 (1)
460-466:⚠️ Potential issue | 🟠 MajorScope error side-effects to active fetch state.
Line [463] currently flips to
Errorfor any warning/error delivered to this screen. Unrelated global errors can clear a validComplete(...)result view.Proposed fix
- fn display_message(&mut self, _message: &str, message_type: MessageType) { + fn display_message(&mut self, _message: &str, message_type: MessageType) { // Banner display is handled globally by AppState; this is only for side-effects. - match message_type { - MessageType::Error | MessageType::Warning => { - self.fetch_banner.take_and_clear(); - self.fetch_group_actions_status = FetchGroupActionsStatus::Error; - } - MessageType::Success | MessageType::Info => {} + if matches!(self.fetch_group_actions_status, FetchGroupActionsStatus::WaitingForResult) + && matches!(message_type, MessageType::Error | MessageType::Warning) + { + self.fetch_banner.take_and_clear(); + self.fetch_group_actions_status = FetchGroupActionsStatus::Error; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/contracts_documents/group_actions_screen.rs` around lines 460 - 466, In display_message, avoid flipping fetch_group_actions_status to Error for unrelated global warnings/errors; instead, only perform the side-effects (self.fetch_banner.take_and_clear() and set fetch_group_actions_status = FetchGroupActionsStatus::Error) when the screen is actively fetching or in a non-final state—i.e., guard the MessageType::Error | MessageType::Warning arm with a check against the current self.fetch_group_actions_status (allow mutation only for FetchGroupActionsStatus variants representing in-progress/pending states, not Complete), so existing Complete results are not cleared by unrelated errors.src/ui/contracts_documents/contracts_documents_screen.rs-543-550 (1)
543-550:⚠️ Potential issue | 🟠 MajorDon’t key query failure state on message text.
Line [545] relies on
"Error fetching documents"substring matching. If backend wording changes, this screen can remainWaitingForResultwith stale elapsed banner and no state transition.Proposed fix
- fn display_message(&mut self, message: &str, message_type: MessageType) { + fn display_message(&mut self, _message: &str, message_type: MessageType) { // Banner display is handled globally by AppState; this is only for side-effects. - if message.contains("Error fetching documents") - && matches!(message_type, MessageType::Error | MessageType::Warning) - { + if matches!(self.document_query_status, DocumentQueryStatus::WaitingForResult) + && matches!(message_type, MessageType::Error | MessageType::Warning) + { self.query_banner.take_and_clear(); self.document_query_status = DocumentQueryStatus::Error; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/contracts_documents/contracts_documents_screen.rs` around lines 543 - 550, display_message currently keys query failure logic on a string match of "Error fetching documents", which is brittle; update the handling to rely on a structured signal instead: add a dedicated variant like MessageType::QueryFailed (or MessageType::DocumentQueryError) to the MessageType enum and change display_message to check for that variant (matches!(message_type, MessageType::QueryFailed)) when deciding to call self.query_banner.take_and_clear() and set self.document_query_status = DocumentQueryStatus::Error; this ensures the screen transitions correctly without depending on backend wording.src/ui/dashpay/contacts_list.rs-1036-1040 (1)
1036-1040:⚠️ Potential issue | 🟠 MajorStop swallowing database write failures in contact sync paths.
These
let _ = ...calls suppress persistence errors and can leave cache/state silently inconsistent after a fetch or profile update.🛠️ Suggested fix pattern
- let _ = self - .app_context - .db - .clear_dashpay_contacts(&owner_id, &network_str); + if let Err(e) = self + .app_context + .db + .clear_dashpay_contacts(&owner_id, &network_str) + { + tracing::error!("Failed to clear cached contacts for {}: {}", owner_id, e); + MessageBanner::set_global( + self.app_context.egui_ctx(), + "Failed to refresh contacts cache", + MessageType::Error, + ); + return; + } - let _ = self.app_context.db.save_dashpay_contact( + if let Err(e) = self.app_context.db.save_dashpay_contact( &owner_id, &contact_data.identity_id, &network_str, contact_data.username.as_deref(), contact_data.display_name.as_deref(), contact_data.avatar_url.as_deref(), None, "accepted", - ); + ) { + tracing::error!("Failed to save cached contact {}: {}", contact_data.identity_id, e); + }Also applies to: 1065-1074, 1078-1085, 1154-1163
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/dashpay/contacts_list.rs` around lines 1036 - 1040, The code is currently swallowing persistence errors by using "let _ = self.app_context.db.clear_dashpay_contacts(&owner_id, &network_str);" which can leave cache/state inconsistent; change calls to clear_dashpay_contacts (and other DB write calls in the contact sync paths) to handle the Result: check for Err, log the error with context (include owner_id and network_str) via the existing logger, and propagate or return the error upstream (or at minimum mark sync as failed) instead of discarding it; apply the same pattern to the other occurrences around the contact sync (the calls at lines referenced in the review: the similar DB writes at 1065-1074, 1078-1085, 1154-1163).src/ui/dashpay/contacts_list.rs-1077-1085 (1)
1077-1085:⚠️ Potential issue | 🟠 MajorPersist private contact data even when nickname is empty.
Saving private info only when
nicknameis present dropsnote/is_hiddenupdates for contacts without a nickname.🛠️ Proposed fix
- if let Some(nickname) = &contact_data.nickname { - let _ = self.app_context.db.save_contact_private_info( - &owner_id, - &contact_data.identity_id, - nickname, - &contact_data.note.unwrap_or_default(), - contact_data.is_hidden, - ); - } + if contact_data.nickname.is_some() || contact_data.note.is_some() || contact_data.is_hidden { + let nickname = contact_data.nickname.as_deref().unwrap_or(""); + let note = contact_data.note.as_deref().unwrap_or(""); + if let Err(e) = self.app_context.db.save_contact_private_info( + &owner_id, + &contact_data.identity_id, + nickname, + note, + contact_data.is_hidden, + ) { + tracing::error!( + "Failed to save private contact info for {}: {}", + contact_data.identity_id, + e + ); + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/dashpay/contacts_list.rs` around lines 1077 - 1085, The code only calls self.app_context.db.save_contact_private_info when contact_data.nickname is Some, which drops updates to note/is_hidden for contacts without nicknames; change it to always call save_contact_private_info using the contact_data.nickname defaulted to an empty string (e.g. via contact_data.nickname.as_deref().unwrap_or_default() or contact_data.nickname.unwrap_or_default()), passing owner_id, contact_data.identity_id, the computed nickname, &contact_data.note.unwrap_or_default(), and contact_data.is_hidden so note/is_hidden are persisted even when nickname is empty.src/ui/tokens/claim_tokens_screen.rs-85-97 (1)
85-97:⚠️ Potential issue | 🟠 MajorDo not fallback to an arbitrary identity when target identity is missing.
Line 96 falls back to the first local identity after an error. That can route a claim through the wrong identity/key set and produce unintended token operations.
🛠️ Suggested fix
- let identity = known_identities - .iter() - .find(|id| id.identity.id() == identity_token_basic_info.identity_id) - .cloned() - .or_else(|| { - MessageBanner::set_global( - app_context.egui_ctx(), - "Identity not found in local store", - MessageType::Error, - ); - // Fallback to first available identity for degraded state. - known_identities.first().cloned() - }); + let identity = known_identities + .iter() + .find(|id| id.identity.id() == identity_token_basic_info.identity_id) + .cloned(); + + if identity.is_none() { + MessageBanner::set_global( + app_context.egui_ctx(), + "Identity not found in local store", + MessageType::Error, + ); + }🤖 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 85 - 97, The code currently falls back to known_identities.first().cloned() when the identity lookup (using known_identities.iter().find(...) against identity_token_basic_info.identity_id) fails, which can route actions through the wrong key; remove that fallback and ensure the lookup returns None while still showing the error banner via MessageBanner::set_global(app_context.egui_ctx(), ...). Concretely: stop using or_else(...) that returns first().cloned(); instead perform the .find(...).cloned(), check if the result is None, call MessageBanner::set_global(...) to surface the error, and propagate/handle the None identity so no implicit fallback identity is used (update any downstream code to handle the Option accordingly).
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (76)
CLAUDE.mddocs/ai-design/2026-02-27-banner-review-fixes/pr-604-review-guide.mdsrc/app.rssrc/backend_task/core/mod.rssrc/context/connection_status.rssrc/ui/components/message_banner.rssrc/ui/components/mod.rssrc/ui/components/wallet_unlock.rssrc/ui/contracts_documents/add_contracts_screen.rssrc/ui/contracts_documents/contracts_documents_screen.rssrc/ui/contracts_documents/document_action_screen.rssrc/ui/contracts_documents/group_actions_screen.rssrc/ui/contracts_documents/register_contract_screen.rssrc/ui/contracts_documents/update_contract_screen.rssrc/ui/dashpay/add_contact_screen.rssrc/ui/dashpay/contact_details.rssrc/ui/dashpay/contact_info_editor.rssrc/ui/dashpay/contact_profile_viewer.rssrc/ui/dashpay/contact_requests.rssrc/ui/dashpay/contacts_list.rssrc/ui/dashpay/profile_screen.rssrc/ui/dashpay/profile_search.rssrc/ui/dashpay/qr_code_generator.rssrc/ui/dashpay/qr_scanner.rssrc/ui/dashpay/send_payment.rssrc/ui/dpns/dpns_contested_names_screen.rssrc/ui/identities/add_existing_identity_screen.rssrc/ui/identities/add_new_identity_screen/by_platform_address.rssrc/ui/identities/add_new_identity_screen/by_using_unused_asset_lock.rssrc/ui/identities/add_new_identity_screen/by_using_unused_balance.rssrc/ui/identities/add_new_identity_screen/by_wallet_qr_code.rssrc/ui/identities/add_new_identity_screen/mod.rssrc/ui/identities/identities_screen.rssrc/ui/identities/keys/add_key_screen.rssrc/ui/identities/keys/key_info_screen.rssrc/ui/identities/mod.rssrc/ui/identities/register_dpns_name_screen.rssrc/ui/identities/transfer_screen.rssrc/ui/identities/withdraw_screen.rssrc/ui/mod.rssrc/ui/network_chooser_screen.rssrc/ui/tokens/add_token_by_id_screen.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/mod.rssrc/ui/tokens/pause_tokens_screen.rssrc/ui/tokens/resume_tokens_screen.rssrc/ui/tokens/set_token_price_screen.rssrc/ui/tokens/tokens_screen/contract_details.rssrc/ui/tokens/tokens_screen/keyword_search.rssrc/ui/tokens/tokens_screen/mod.rssrc/ui/tokens/tokens_screen/my_tokens.rssrc/ui/tokens/tokens_screen/structs.rssrc/ui/tokens/tokens_screen/token_creator.rssrc/ui/tokens/transfer_tokens_screen.rssrc/ui/tokens/unfreeze_tokens_screen.rssrc/ui/tokens/update_token_config.rssrc/ui/tokens/view_token_claims_screen.rssrc/ui/tools/address_balance_screen.rssrc/ui/tools/contract_visualizer_screen.rssrc/ui/tools/document_visualizer_screen.rssrc/ui/tools/grovestark_screen.rssrc/ui/tools/masternode_list_diff_screen.rssrc/ui/tools/platform_info_screen.rssrc/ui/tools/transition_visualizer_screen.rssrc/ui/wallets/asset_lock_detail_screen.rssrc/ui/wallets/create_asset_lock_screen.rssrc/ui/wallets/send_screen.rssrc/ui/wallets/single_key_send_screen.rssrc/ui/wallets/wallets_screen/address_table.rssrc/ui/wallets/wallets_screen/dialogs.rssrc/ui/wallets/wallets_screen/mod.rs
🚧 Files skipped from review as they are similar to previous changes (8)
- src/ui/wallets/single_key_send_screen.rs
- src/ui/identities/add_new_identity_screen/by_using_unused_asset_lock.rs
- src/ui/tokens/tokens_screen/contract_details.rs
- src/ui/identities/add_new_identity_screen/by_platform_address.rs
- src/ui/wallets/wallets_screen/dialogs.rs
- src/ui/dashpay/contact_requests.rs
- src/ui/tools/document_visualizer_screen.rs
- src/ui/tokens/tokens_screen/mod.rs
Contributor
There was a problem hiding this comment.
Pull request overview
This PR implements a unified MessageBanner system for all screens, replacing dozens of per-screen inline status messages, elapsed-time labels, and error bubbles with a single centrally-rendered banner component.
Changes:
- Introduces
MessageBannercomponent with a global API (set_global,replace_global,clear_global_message),BannerHandlefor lifecycle management, and auto-dismiss/elapsed-timer support - Migrates ~50 screens from inline error/success labels and
WaitingForResult(timestamp)status variants to the unified banner system - Refactors
get_selected_walletto returnResult<Option<...>, String>instead of writing to an&mut Option<String>error parameter
Reviewed changes
Copilot reviewed 76 out of 76 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
src/ui/components/mod.rs |
Re-exports new banner extension traits |
src/ui/components/wallet_unlock.rs |
Removes set_error_message/error_message from trait; routes errors to global banner |
src/ui/mod.rs |
Changes display_task_result default to a no-op; adds doc comments to ScreenLike |
src/ui/identities/mod.rs |
Converts get_selected_wallet to Result-returning signature |
src/app.rs |
Routes task success/error to global banner; adds connection status banner management |
src/backend_task/core/mod.rs |
Removes early-return error for all-None chainlock result |
src/context/connection_status.rs |
Removes error branch from handle_task_result |
src/ui/tokens/mod.rs |
Adds shared helpers: load_identities_with_banner, set_error_banner, validate_signing_key |
src/ui/tokens/tokens_screen/structs.rs |
Simplifies can_transfer check |
| All screen files | Migrates inline messages/elapsed labels to global MessageBanner API |
CLAUDE.md |
Documents new banner architecture and BannerHandle lifecycle |
Comments suppressed due to low confidence (5)
src/ui/wallets/send_screen.rs:1
handle.with_elapsed()returns a value (likely&BannerHandleor a builder) but the return value is discarded. Ifwith_elapsed()is a builder method that must be stored to take effect, the elapsed timer will never start. The pattern used elsewhere in the codebase (e.g. intoken_creator.rs) callshandle.with_elapsed()and then storesself.operation_banner = Some(handle), which is what this function does — however, ifwith_elapsedreturns a new handle or mutates through a returned reference and the originalhandleis what gets stored, this is correct. But ifwith_elapsedis a consuming builder, the stored handle would be missing the elapsed configuration. Please verify the return type ofwith_elapsed()and confirm that storing the pre-callhandlecaptures the elapsed timer state.
src/ui/wallets/single_key_send_screen.rs:1- The
returnstatement was removed from this branch. Previously the function returned early after showing the fee confirmation dialog, preventing the message from being stored. Now execution continues to fall through to the end of the function, which no longer stores a message. While no message is stored (the storage was removed), thesendingstate reset logic above this block (lines 936–940) may now incorrectly execute for relay-fee errors —self.sending = falsewould be set even for relay-fee errors that should keep the sending state alive. The original comment said "Keep sending state true until user confirms or cancels", but removingreturnmeans thesendingstate check at lines 936–940 runs first and may clear it.
src/ui/tokens/tokens_screen/keyword_search.rs:1 - This manual
take-and-clearpattern is used in several other files viaself.operation_banner.take_and_clear()(using theOptionBannerExttrait). For consistency with the rest of the codebase, this should useself.operation_banner.take_and_clear()instead.
src/ui/wallets/single_key_send_screen.rs:1 self.fee_dialog.pending_request = Nonewas added here indisplay_task_resultbut the originaldisplay_messagecall (now replaced with a directset_global) was the mechanism that also cleared the fee dialog state in the original code. If a success result comes in while the fee dialog is open,pending_requestis cleared. However if the banner is set viaAppStateanddisplay_task_resultis called, there may be a double-clear or ordering issue depending on the call sequence. This is likely correct, but worth verifying thatpending_requestdoesn't need to be cleared indisplay_messageas well (for the error path).
src/ui/tools/transition_visualizer_screen.rs:1- Previously, when an error occurred the broadcast status was set to
TransitionBroadcastStatus::Error(message.to_string(), Instant::now()), which preserved the error message and timestamp for display. Now it resets toNotStarted, discarding the error info entirely. TheError(String, Instant)variant still exists in the enum (line 29 in the diff context) but is never set anymore. The fade-effect rendering for the error state (which was noted in the PR description as being intentionally preserved) would now never trigger. Please verify this is intentional or restore theErrorvariant assignment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Adapt to breaking changes in rust-dashcore (a05d256f → 2824e52a): - Replace removed FeeLevel enum with FeeRate::normal() direct calls - Replace removed WalletManager::create_unsigned_payment_transaction() with TransactionBuilder + WalletManager::get_change_address() - Replace removed DashSpvClientInterface/DashSpvClientCommand with direct Arc<SpvClient> for quorum lookups via get_quorum_at_height() - Replace removed start()+monitor_network() with client.run(token) - Add .await to now-async subscribe_sync/network/progress methods - Replace removed SyncState::Initializing with SyncState::WaitForEvents Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Fix QR scanner form reset matching wrong result type (CMT-001) - Remove dangerous identity fallback in token transfer screen (CMT-002) - Add fee-aware validation before credit transfers (CMT-003) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…reference The SPV client reference only needs atomic set/clear (on start/stop) and wait-free reads (quorum lookups). ArcSwapOption is a better fit than AsyncRwLock<Option<Arc<...>>> — no lock contention, no async in blocking context, and consistent with how AppContext already uses ArcSwap for the SDK. Also fixes stale doc comment referencing removed DashSpvClientInterface. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/tokens/transfer_tokens_screen.rs`:
- Around line 85-92: The code currently emits two conflicting banners when a
selected identity lookup fails: one via
MessageBanner::set_global(app_context.egui_ctx(), "Identity not found in local
store", ...) and later another saying "No identities loaded...". Change the
later branch so it only shows "No identities loaded..." when the identities
collection is actually empty (e.g., check identities.is_empty()), otherwise do
not emit that second banner; keep the initial "Identity not found in local
store" for the missing-selected-identity case. Ensure MessageBanner::set_global
is only called once per failure path.
- Around line 340-343: Replace the user-facing banners that include raw error
text in the MessageBanner::set_global calls (the uses that pass format!("Failed
to load local identities: {e}") and the similar one around lines referencing the
transfer token load) with a generic, non-sensitive message such as "Failed to
load local identities" or "Failed to load tokens" and move the full error
details into a developer log call (e.g., tracing::error! or log::error!) right
before or after the banner; keep the calls to self.app_context.egui_ctx() and
MessageType::Error unchanged so only the banner text changes and ensure both
occurrences are updated.
- Around line 361-365: The refresh logic sets self.max_amount by finding any
balance with the same identity_id, which picks an arbitrary token when an
identity owns multiple tokens; update the filter on token_balances to match both
the identity and the specific token/contract represented by this screen (e.g.,
compare balance.contract_id or balance.token_id to the screen's selected
contract/token identifier such as self.contract_id or self.selected_token)
before mapping to Amount::from so self.max_amount always reflects the correct
token's balance.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/backend_task/core/recover_asset_locks.rssrc/ui/dashpay/qr_scanner.rssrc/ui/identities/transfer_screen.rssrc/ui/tokens/transfer_tokens_screen.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- src/backend_task/core/recover_asset_locks.rs
- src/ui/identities/transfer_screen.rs
…nto design/unified-message-display-applied
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/backend_task/core/mod.rs (1)
533-566:⚠️ Potential issue | 🟠 MajorUse typed error propagation instead of converting
WalletErrortoString.The function returns
Result<Transaction, String>but should returnResult<Transaction, WalletError>(or a properTaskError). At line 541,WalletError::InsufficientFundsis correctly matched, but immediately converted back to a String at line 560, forcing callers (like lines 660–663) to parse the error message instead of pattern-matching on types. This violates the backend task guideline: "Backend tasks should returnResult<T, TaskError>with typed error variants for domain errors rather than converting toString."Update the function signature and error returns to propagate
WalletErrordirectly or convert to an appropriateTaskErrorvariant.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/backend_task/core/mod.rs` around lines 533 - 566, The code is converting WalletError into String; change the function to return Result<Transaction, WalletError> (or Result<Transaction, TaskError>) and stop formatting errors to strings: update the function signature and replace the Err(format!(...)) arm in the match over Self::build_unsigned_payment_tx with a direct propagation (e.g., use ? or return Err(err) or map WalletError into the chosen TaskError variant). Ensure WalletError::InsufficientFunds handling remains the same (it should still match the variant), and convert other failure paths (e.g., the build_unsigned_payment_tx error arm and any calls to estimate_fallback_amount) to propagate typed errors instead of strings; keep references to symbols like build_unsigned_payment_tx, WalletError::InsufficientFunds, estimate_fallback_amount, DEFAULT_BIP44_ACCOUNT_INDEX, scale_factor, and FALLBACK_STEP to locate the spots to change.
🧹 Nitpick comments (4)
src/spv/manager.rs (2)
555-556: Clearspv_clientdefensively inclear_data_dir().Relying only on async shutdown cleanup is a bit brittle. Clearing the ArcSwap slot here too makes data-dir reset more robust after abnormal task exits.
Small defensive cleanup
- // spv_client is cleared asynchronously when the client stops; no action needed here. + // Defensive clear in case prior shutdown path was interrupted. + self.spv_client.store(None);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/spv/manager.rs` around lines 555 - 556, The spv_client ArcSwap slot should be defensively cleared inside clear_data_dir() so abnormal task exits don't leave a stale client; locate the clear_data_dir function and add a step that stores/sets the ArcSwap-backed spv_client slot to None (or the equivalent empty value for the ArcSwapOption/ArcSwap type used) before returning, ensuring you use the same API (e.g., store or swap) that safely updates the ArcSwap slot for spv_client.
633-642: Consider usingHandle::try_current()instead of panicking on missing runtime context.Lines 638–639 use
tokio::runtime::Handle::current().block_on()directly. While the app's Tokio runtime is initialized inmain(), this pattern will panic ifget_quorum_public_key()is ever called from outside runtime context (e.g., from a different executor or thread pool in future refactors).Use
Handle::try_current()to gracefully return an error instead:Suggested improvement
- tokio::task::block_in_place(|| { - tokio::runtime::Handle::current().block_on(async { + let handle = tokio::runtime::Handle::try_current() + .map_err(|_| "SPV quorum lookup requires an active Tokio runtime".to_string())?; + tokio::task::block_in_place(|| { + handle.block_on(async { client .get_quorum_at_height(core_chain_locked_height, llmq_type, qh) .await .map(|q| {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/spv/manager.rs` around lines 633 - 642, The call that uses tokio::runtime::Handle::current().block_on() inside the block_in_place in the function that calls client.get_quorum_at_height should be changed to use tokio::runtime::Handle::try_current() and return a descriptive error if no runtime handle is available instead of panicking; locate the code around the invocation of get_quorum_at_height on self.spv_client in the method that loads the SPV client, call Handle::try_current() before block_on, map the Err case into the function's error path (e.g., propagate a String or appropriate Error) and only call block_on when try_current() yields Ok(handle). Ensure the error message clearly states that no Tokio runtime handle is available for get_quorum_public_key/get_quorum_at_height.src/backend_task/core/mod.rs (2)
190-196: All-Nonechainlock success is ambiguous for consumers.Returning
CoreItem::ChainLocks(None, None, None, None)as plain success makes it hard for the UI to distinguish “temporarily unavailable” vs “valid empty state.” Consider adding explicit status/warning metadata for this case.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/backend_task/core/mod.rs` around lines 190 - 196, The current success return uses BackendTaskSuccessResult::CoreItem(CoreItem::ChainLocks(...)) which can be all None and is ambiguous; modify the logic in mod.rs where CoreItem::ChainLocks is constructed to detect the all-None case and return an explicit status-bearing result instead (e.g., create/return a new status variant like BackendTaskSuccessResult::CoreItemWithStatus or extend CoreItem::ChainLocks to include a Status/Warning field) so consumers can distinguish "temporarily unavailable" vs "valid empty state"; update any call sites that pattern-match on BackendTaskSuccessResult::CoreItem and CoreItem::ChainLocks to handle the new status metadata.
608-668: Add inline unit tests for the new transaction builder helper.This helper now covers core payment construction behavior; please add
#[test]cases for no-UTXO, insufficient-funds mapping, and multi-recipient output construction to lock down regressions.As per coding guidelines:
**/*.rs: Place unit tests inline in source files with#[test]attribute.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/backend_task/core/mod.rs` around lines 608 - 668, Add three inline #[test]s in the same mod.rs testing build_unsigned_payment_tx: (1) no-UTXO case — prepare a WalletManager/ManagedWalletInfo whose account.utxos is empty and assert Err(WalletError::InsufficientFunds) from build_unsigned_payment_tx; (2) insufficient-funds mapping — prepare account with UTXOs too-small for recipients so select_inputs returns the insufficient error and assert the function returns WalletError::InsufficientFunds (exercise the map_err branch on select_inputs); (3) multi-recipient output construction — prepare account with sufficient UTXOs and recipients list of multiple (Address,u64) and assert Ok(Transaction) is returned and that the transaction contains outputs matching each recipient amount/address; use the same helper objects/types in the file (WalletManager, ManagedWalletInfo, WalletId, TransactionBuilder, Transaction, WalletError, FeeRate) to construct minimal wallets and addresses for each test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/backend_task/core/mod.rs`:
- Around line 533-566: The code is converting WalletError into String; change
the function to return Result<Transaction, WalletError> (or Result<Transaction,
TaskError>) and stop formatting errors to strings: update the function signature
and replace the Err(format!(...)) arm in the match over
Self::build_unsigned_payment_tx with a direct propagation (e.g., use ? or return
Err(err) or map WalletError into the chosen TaskError variant). Ensure
WalletError::InsufficientFunds handling remains the same (it should still match
the variant), and convert other failure paths (e.g., the
build_unsigned_payment_tx error arm and any calls to estimate_fallback_amount)
to propagate typed errors instead of strings; keep references to symbols like
build_unsigned_payment_tx, WalletError::InsufficientFunds,
estimate_fallback_amount, DEFAULT_BIP44_ACCOUNT_INDEX, scale_factor, and
FALLBACK_STEP to locate the spots to change.
---
Nitpick comments:
In `@src/backend_task/core/mod.rs`:
- Around line 190-196: The current success return uses
BackendTaskSuccessResult::CoreItem(CoreItem::ChainLocks(...)) which can be all
None and is ambiguous; modify the logic in mod.rs where CoreItem::ChainLocks is
constructed to detect the all-None case and return an explicit status-bearing
result instead (e.g., create/return a new status variant like
BackendTaskSuccessResult::CoreItemWithStatus or extend CoreItem::ChainLocks to
include a Status/Warning field) so consumers can distinguish "temporarily
unavailable" vs "valid empty state"; update any call sites that pattern-match on
BackendTaskSuccessResult::CoreItem and CoreItem::ChainLocks to handle the new
status metadata.
- Around line 608-668: Add three inline #[test]s in the same mod.rs testing
build_unsigned_payment_tx: (1) no-UTXO case — prepare a
WalletManager/ManagedWalletInfo whose account.utxos is empty and assert
Err(WalletError::InsufficientFunds) from build_unsigned_payment_tx; (2)
insufficient-funds mapping — prepare account with UTXOs too-small for recipients
so select_inputs returns the insufficient error and assert the function returns
WalletError::InsufficientFunds (exercise the map_err branch on select_inputs);
(3) multi-recipient output construction — prepare account with sufficient UTXOs
and recipients list of multiple (Address,u64) and assert Ok(Transaction) is
returned and that the transaction contains outputs matching each recipient
amount/address; use the same helper objects/types in the file (WalletManager,
ManagedWalletInfo, WalletId, TransactionBuilder, Transaction, WalletError,
FeeRate) to construct minimal wallets and addresses for each test.
In `@src/spv/manager.rs`:
- Around line 555-556: The spv_client ArcSwap slot should be defensively cleared
inside clear_data_dir() so abnormal task exits don't leave a stale client;
locate the clear_data_dir function and add a step that stores/sets the
ArcSwap-backed spv_client slot to None (or the equivalent empty value for the
ArcSwapOption/ArcSwap type used) before returning, ensuring you use the same API
(e.g., store or swap) that safely updates the ArcSwap slot for spv_client.
- Around line 633-642: The call that uses
tokio::runtime::Handle::current().block_on() inside the block_in_place in the
function that calls client.get_quorum_at_height should be changed to use
tokio::runtime::Handle::try_current() and return a descriptive error if no
runtime handle is available instead of panicking; locate the code around the
invocation of get_quorum_at_height on self.spv_client in the method that loads
the SPV client, call Handle::try_current() before block_on, map the Err case
into the function's error path (e.g., propagate a String or appropriate Error)
and only call block_on when try_current() yields Ok(handle). Ensure the error
message clearly states that no Tokio runtime handle is available for
get_quorum_public_key/get_quorum_at_height.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
Cargo.tomlsrc/backend_task/core/mod.rssrc/backend_task/core/send_single_key_wallet_payment.rssrc/spv/manager.rssrc/ui/network_chooser_screen.rssrc/ui/wallets/single_key_send_screen.rs
…_screen) - Remove duplicate conflicting banner on missing identity (CMT-003) - Use generic banner messages with with_details() for errors (CMT-002) - Fix refresh to match specific token by contract+position (CMT-001) - Document error banner pattern in CLAUDE.md Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Address findings from code review triage (SEC-002, RUST-003, RUST-012, PROJ-005): preserve error context in payment tx builder, add INTENTIONAL comment for Debug-formatting accepted risk, remove dead import in SPV manager, fix CLAUDE.md error banner documentation, and add OptionBannerExt replace/replace_with_elapsed convenience methods. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
RUST-010: Add wallet_open_attempted guard to ~30 screens preventing per-frame try_open_wallet_no_password calls. Guard resets on wallet or identity selection change. PROJ-001: Migrate register/update contract and document action screens from BroadcastStatus::Broadcasting(u64) to refresh_banner with elapsed time display. RUST-001: Surface database errors via .or_show_error() in contacts_list instead of silently swallowing with let _ =. RUST-005: tokens_screen display_message only clears operation_banner on Error/Warning, preserving it on Success/Info. RUST-007: Remove dead Error(String, Instant) variant from TransitionBroadcastStatus in transition_visualizer_screen. RUST-009: Make identity Optional in claim_tokens_screen with degraded state handling when identity not found locally. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ssage-display-applied
Cover the new Error variant added by #650 (SPV sync error state) in the connection banner match arm. Shows an error banner directing users to the connection status tooltip for details. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 20
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (10)
src/context/connection_status.rs (1)
374-399:⚠️ Potential issue | 🟠 MajorIgnoring error results here can leave connection state stale.
At Line 374, the
if let TaskResult::Success(...)gate drops allTaskResult::Errorpaths. If chainlock polling fails,rpc_onlineis never driven offline here, sooverall_statecan remain incorrectly connected until another success updates it.Please add explicit error handling for relevant core polling failures (and call
refresh_state()), rather than only processing success variants.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/context/connection_status.rs` around lines 374 - 399, The code only handles TaskResult::Success variants for BackendTaskSuccessResult::CoreItem chainlock messages, ignoring TaskResult::Error which can leave rpc_online/states stale; add explicit handling for TaskResult::Error (and other non-success cases related to CoreItem::ChainLocks and CoreItem::ChainLock) so that when a chainlock poll fails you call self.set_rpc_online(false) and self.refresh_state(), and keep existing success handling in update_from_chainlocks and set_rpc_online(true); update the match on TaskResult (or add an else arm) to detect errors for the same tasks and drive the connection state offline and refresh_state() accordingly.src/spv/manager.rs (2)
545-556:⚠️ Potential issue | 🟠 MajorHarden
clear_data_dirwith a runtime-liveness gate and defensive client clear.Line 547 checks only
status.is_active(). SinceSpvStatus::Erroris “inactive,” this path can run before teardown is fully complete. With Line 555 skippingspv_clientclear, stale handles can survive into directory deletion.🔧 Suggested fix
pub fn clear_data_dir(&self) -> Result<(), String> { let status = self.read_status().map_err(|e| e.to_string())?; - if status.is_active() { + let runtime_running = self + .stop_token + .lock() + .map_err(|_| "SPV stop_token lock poisoned".to_string())? + .is_some(); + if status.is_active() || runtime_running { return Err("Stop the SPV client before clearing its data".to_string()); } @@ - // spv_client is cleared asynchronously when the client stops; no action needed here. + // Defensive clear in case status transitioned to Error before full teardown completed. + self.spv_client.store(None);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/spv/manager.rs` around lines 545 - 556, The clear_data_dir path currently only checks status.is_active() which allows SpvStatus::Error (an "inactive" but not-fully-teardown state) to proceed; update clear_data_dir to gate on a stronger runtime liveness check (e.g., require the status to be explicitly stopped/terminated rather than merely not active — treat SpvStatus::Error as unsafe) by using read_status()/status and refusing to proceed if the status is Active or in an Error/terminating state, and additionally defensively clear any lingering client handle by locking and setting self.spv_client (or the spv_client mutex) to None before clearing storage to avoid stale handles surviving directory deletion; keep references to clear_data_dir, read_status, storage, spv_client and SpvStatus::Error when making the change.
633-663:⚠️ Potential issue | 🟠 MajorPrevent panic in synchronous quorum lookup bridge.
Handle::current()panics when called outside a Tokio runtime, andblock_in_place()panics on current-thread runtimes. Sinceget_quorum_public_key()is a synchronous method that can be called from various contexts, both scenarios are possible.Use
Handle::try_current()with a runtime flavor check to handle these cases gracefully:🔧 Suggested fix
- tokio::task::block_in_place(|| { - tokio::runtime::Handle::current().block_on(async { + let handle = tokio::runtime::Handle::try_current() + .map_err(|_| "No Tokio runtime available for quorum lookup".to_string())?; + if matches!(handle.runtime_flavor(), tokio::runtime::RuntimeFlavor::CurrentThread) { + return Err("Quorum lookup requires a multi-thread Tokio runtime".to_string()); + } + + tokio::task::block_in_place(|| { + handle.block_on(async { client .get_quorum_at_height(core_chain_locked_height, llmq_type, qh) .await @@ - }) + }) })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/spv/manager.rs` around lines 633 - 663, The synchronous bridge currently calls tokio::task::block_in_place() with tokio::runtime::Handle::current() which can panic; replace that with a safe runtime detection: use tokio::runtime::Handle::try_current() and inspect the runtime flavor, and branch so that when a compatible runtime exists (Some(handle) and handle.runtime_flavor() != RuntimeFlavor::CurrentThread) you keep the block_in_place + handle.block_on(...) path, but when try_current() is None or the runtime is CurrentThread create a temporary tokio::runtime::Runtime::new() and use its block_on(...) (or otherwise run the async client call on a blocking thread) to execute client.get_quorum_at_height(...) safely; update the code around spv_client.load_full(), tokio::task::block_in_place, tokio::runtime::Handle::current(), and client.get_quorum_at_height to implement this branching so no panic occurs.src/backend_task/core/mod.rs (1)
528-531:⚠️ Potential issue | 🟠 MajorGuard against zero-value outputs during fee scaling.
Line 530 truncates scaled amounts with
as u64. For small transfers, one or more recipients can become0, causing invalid-output build failures and unclear UX instead of a deterministic “amount too small after fees” error.Proposed fix
for _ in 0..MAX_FEE_ITERATIONS { let scaled_recipients: Vec<(Address, u64)> = recipients .iter() .map(|(addr, amt)| (addr.clone(), (*amt as f64 * scale_factor) as u64)) .collect(); + + if scaled_recipients.iter().any(|(_, amt)| *amt == 0) { + return Err( + "Amount too small after fee adjustment; increase amount to cover fees" + .to_string(), + ); + }Based on learnings: "ensure the transfer amount considers estimated transaction fees; set a practical minimum based on fee requirements so that a transfer doesn't fail due to insufficient credits."
Also applies to: 542-563
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/backend_task/core/mod.rs` around lines 528 - 531, Scaled amounts are currently truncated to zero in scaled_recipients (created from recipients and scale_factor), which can produce invalid zero-valued outputs; detect when any scaled amount becomes 0 and return a deterministic error (e.g., an AmountTooSmallAfterFees / TransferAmountTooSmall variant) instead of building outputs, or alternatively enforce a minimum of 1 and adjust logic consistently; update the code that maps recipients -> scaled_recipients (and the similar logic around lines 542-563) to check for any zero entries after scaling and fail early with that error (or apply the chosen minimum) so the caller sees a clear "amount too small after fees" response.src/ui/wallets/send_screen.rs (1)
2663-2708:⚠️ Potential issue | 🟠 MajorOnly clear
send_bannerfor send-related task results.Line 2663 clears
send_bannerbefore checking which result arrived. If an unrelatedBackendTaskSuccessResultis processed while a send is in flight, the progress banner is removed early.💡 Proposed fix
- self.send_banner.take_and_clear(); match backend_task_success_result { crate::backend_task::BackendTaskSuccessResult::WalletPayment { txid: _, recipients, total_amount, } => { + self.send_banner.take_and_clear(); let msg = if recipients.len() == 1 { @@ crate::backend_task::BackendTaskSuccessResult::TransferredCredits(fee_result) => { + self.send_banner.take_and_clear(); let fee_info = format!( @@ crate::backend_task::BackendTaskSuccessResult::PlatformAddressFunded { .. } => { + self.send_banner.take_and_clear(); self.send_status = SendStatus::Complete("Platform address funded successfully!".to_string()); } crate::backend_task::BackendTaskSuccessResult::PlatformAddressWithdrawal { .. } => { + self.send_banner.take_and_clear(); self.send_status = SendStatus::Complete("Withdrawal initiated successfully!\n\nNote: It may take a few minutes for funds to appear on the Core chain.".to_string()); } crate::backend_task::BackendTaskSuccessResult::PlatformCreditsTransferred { .. } => { + self.send_banner.take_and_clear(); self.send_status = SendStatus::Complete("Platform credits transferred successfully!".to_string()); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/wallets/send_screen.rs` around lines 2663 - 2708, The code currently calls self.send_banner.take_and_clear() before matching on crate::backend_task::BackendTaskSuccessResult, which prematurely clears the send progress banner for unrelated results; remove that unconditional call and instead invoke self.send_banner.take_and_clear() only inside the match arms that correspond to send-related results (e.g., BackendTaskSuccessResult::WalletPayment, ::TransferredCredits, ::PlatformAddressFunded, ::PlatformAddressWithdrawal, ::PlatformCreditsTransferred) immediately before updating self.send_status (SendStatus::Complete) so unrelated task results do not hide an in-flight send banner.src/ui/tokens/tokens_screen/mod.rs (1)
2242-2254:⚠️ Potential issue | 🟠 MajorInvalid perpetual recipient IDs are silently coerced to
Identifier::default().At Line 2244,
unwrap_or_default()allows invalid base58 input to continue with a zero/default identity instead of failing fast.Suggested fix
TokenDistributionRecipientUI::Identity => { if let Some(id) = self.perpetual_dist_recipient_identity_input.as_ref() { - let id_res = Identifier::from_string(id, Encoding::Base58); - TokenDistributionRecipient::Identity(id_res.unwrap_or_default()) + let parsed_id = Identifier::from_string(id, Encoding::Base58).map_err(|_| { + let msg = "Invalid base58 identifier for perpetual distribution recipient"; + MessageBanner::set_global(self.app_context.egui_ctx(), msg, MessageType::Error); + msg.to_string() + })?; + TokenDistributionRecipient::Identity(parsed_id) } else { MessageBanner::set_global( self.app_context.egui_ctx(), "Invalid base58 identifier for perpetual distribution recipient", MessageType::Error, ); return Err( "Invalid base58 identifier for perpetual distribution recipient" .to_string(), ); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/tokens/tokens_screen/mod.rs` around lines 2242 - 2254, The current code calls Identifier::from_string(...) and then unwrap_or_default(), which silently converts invalid base58 input into Identifier::default(); change this to explicitly handle the Result from Identifier::from_string for perpetual_dist_recipient_identity_input: match or if let Err(e) => call MessageBanner::set_global(self.app_context.egui_ctx(), "Invalid base58 identifier for perpetual distribution recipient", MessageType::Error) and return Err with a descriptive message (including e if useful), and only construct TokenDistributionRecipient::Identity(id) when Identifier::from_string returns Ok(id).src/ui/tokens/resume_tokens_screen.rs (1)
99-155:⚠️ Potential issue | 🟠 MajorAuthorization failures in
new()notify, but do not block submission.The screen still starts in
ResumeTokensStatus::NotStartedafter these failures, so users can continue and submit a guaranteed-failing resume request.Suggested fix
pub fn new(identity_token_info: IdentityTokenInfo, app_context: &Arc<AppContext>) -> Self { + let mut initial_status = ResumeTokensStatus::NotStarted; let set_error_banner = |msg: &str| super::set_error_banner(app_context, msg); let group = match identity_token_info .token_config .emergency_action_rules() .authorized_to_make_change_action_takers() { AuthorizedActionTakers::NoOne => { set_error_banner("Resuming is not allowed on this token"); + initial_status = ResumeTokensStatus::Error; None } AuthorizedActionTakers::ContractOwner => { if identity_token_info.data_contract.contract.owner_id() != identity_token_info.identity.identity.id() { set_error_banner( "You are not allowed to resume this token. Only the contract owner is.", ); + initial_status = ResumeTokensStatus::Error; } None } AuthorizedActionTakers::Identity(identifier) => { if identifier != &identity_token_info.identity.identity.id() { set_error_banner("You are not allowed to resume this token"); + initial_status = ResumeTokensStatus::Error; } None } // ... }; Self { // ... - status: ResumeTokensStatus::NotStarted, + status: initial_status, // ... } }src/app.rs (1)
1084-1095:⚠️ Potential issue | 🟠 MajorAvoid showing raw
TaskErrortext directly to non-developer users.
err.to_string()is surfaced as user-facing text, which can leak internal backend/SDK details.Suggested patch
- let msg = err.to_string(); - let handle = MessageBanner::set_global(ctx, &msg, MessageType::Error); - if self.current_app_context().is_developer_mode() { + let is_dev = self.current_app_context().is_developer_mode(); + let user_msg = if is_dev { + err.to_string() + } else { + "Operation failed. Please retry or check logs for details.".to_string() + }; + let handle = MessageBanner::set_global(ctx, &user_msg, MessageType::Error); + if is_dev { // INTENTIONAL(SEC-003): TaskError Debug output is shown to users // in developer mode. This is a local UI app — no third parties // see this output. Ensure inner error types don't expose secrets // (see `#667`). handle.with_details(&err); } self.visible_screen_mut() - .display_message(&msg, MessageType::Error); + .display_message(&user_msg, MessageType::Error);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app.rs` around lines 1084 - 1095, Replace the direct use of err.to_string() for user-facing text with a sanitized user message for non-developers: call something like let user_msg = if self.current_app_context().is_developer_mode() { err.to_string() } else { sanitized or generic message ("An error occurred while performing the task.") } then pass user_msg into MessageBanner::set_global(ctx, &user_msg, MessageType::Error) and visible_screen_mut().display_message(&user_msg, MessageType::Error); keep handle.with_details(&err) only inside the is_developer_mode() branch so raw TaskError details are shown only to developers.src/ui/identities/add_existing_identity_screen.rs (2)
781-803:⚠️ Potential issue | 🟡 MinorReset
wallet_open_attemptedwhen DPNS wallet selection changes.Unlike other selectors, this block updates
selected_walletwithout resettingwallet_open_attempted. After mode switches, that can skip auto-open attempts for the newly selected wallet.Suggested fix
if ui .selectable_label( self.selected_wallet.is_none(), "All unlocked wallets", ) .clicked() { self.selected_wallet = None; + self.wallet_open_attempted = false; } for (alias, wallet) in &wallets_snapshot { let is_selected = self .selected_wallet .as_ref() .is_some_and(|selected| Arc::ptr_eq(selected, wallet)); if ui.selectable_label(is_selected, alias).clicked() { self.selected_wallet = Some(wallet.clone()); + self.wallet_open_attempted = false; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/identities/add_existing_identity_screen.rs` around lines 781 - 803, When handling DPNS wallet selection in ComboBox::from_id_salt("dpns_wallet_selector") ensure you reset the flag wallet_open_attempted whenever selected_wallet is changed; specifically, set wallet_open_attempted = false both in the branch that sets self.selected_wallet = None (the "All unlocked wallets" selectable) and in the branch inside the for (alias, wallet) in &wallets_snapshot loop where you set self.selected_wallet = Some(wallet.clone()) so the new selection can trigger auto-open attempts like other selectors.
683-714:⚠️ Potential issue | 🟡 MinorClear loading banner when wallet-index validation fails.
Line 685 creates a
"Loading identity..."banner before parsing. On parse failure (Line 708), state resets butrefresh_banneris left active, so stale progress UI can linger.Suggested fix
if ui.add(button).clicked() { - self.add_identity_status = AddIdentityStatus::WaitingForResult; - self.success_message = None; - let handle = MessageBanner::set_global( - self.app_context.egui_ctx(), - "Loading identity...", - MessageType::Info, - ); - handle.with_elapsed(); - self.refresh_banner = Some(handle); - // Parse identity index input if let Ok(identity_index) = self.identity_index_input.trim().parse::<u32>() { + self.add_identity_status = AddIdentityStatus::WaitingForResult; + self.success_message = None; + let handle = MessageBanner::set_global( + self.app_context.egui_ctx(), + "Loading identity...", + MessageType::Info, + ); + handle.with_elapsed(); + self.refresh_banner = Some(handle); let wallet_ref = self.selected_wallet.as_ref().unwrap().clone().into(); action = AppAction::BackendTask(BackendTask::IdentityTask( match self.wallet_search_mode { WalletIdentitySearchMode::SpecificIndex => { IdentityTask::SearchIdentityFromWallet(wallet_ref, identity_index) } WalletIdentitySearchMode::UpToIndex => { IdentityTask::SearchIdentitiesUpToIndex(wallet_ref, identity_index) } }, )); } else { // Handle invalid index input + self.refresh_banner.take_and_clear(); self.add_identity_status = AddIdentityStatus::NotStarted; MessageBanner::set_global( self.app_context.egui_ctx(), "Invalid identity index", MessageType::Error, ); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/identities/add_existing_identity_screen.rs` around lines 683 - 714, The loading banner is created before parsing identity_index_input but not cleared on parse failure, leaving refresh_banner active; update the error branch so it takes and clears the existing banner handle (e.g., self.refresh_banner.take().map(|h| h.clear()) or the equivalent dismiss/close method on the MessageBanner handle) before setting add_identity_status back to NotStarted and showing the "Invalid identity index" error via MessageBanner::set_global, ensuring no stale progress UI remains; locate this logic around add_identity_status, refresh_banner, identity_index_input parsing, and MessageBanner::set_global in the AddExistingIdentityScreen flow.
♻️ Duplicate comments (2)
src/ui/identities/register_dpns_name_screen.rs (1)
301-308: 🛠️ Refactor suggestion | 🟠 MajorComplete the
ScreenLikemethod set on this screen.This impl block still omits the repo-required methods (
refresh,refresh_on_arrival,change_context) forScreenLike.As per coding guidelines:
src/ui/**/*.rs: ImplementScreenLiketrait with methods:ui(),display_task_result(),display_message(),refresh(),refresh_on_arrival(), andchange_context().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/identities/register_dpns_name_screen.rs` around lines 301 - 308, The impl for ScreenLike on RegisterDpnsNameScreen is missing the required methods refresh, refresh_on_arrival, and change_context; add implementations for ScreenLike::refresh(&mut self) to update any cached state and mark view dirty (use existing refresh_banner or related fields), ScreenLike::refresh_on_arrival(&mut self) to perform the same actions when the screen becomes active (e.g., set refresh_banner.take_and_clear() or trigger a full state reload), and ScreenLike::change_context(&mut self, ctx: Context) to update the screen's context field and clear/initialize any context-dependent state (like resetting register_dpns_name_status or clearing inputs). Ensure the method signatures exactly match the ScreenLike trait and that these methods coexist with the existing ui(), display_task_result(), and display_message() implementations on RegisterDpnsNameScreen.src/ui/dashpay/contacts_list.rs (1)
1006-1232: 🛠️ Refactor suggestion | 🟠 MajorImplement
change_context()in thisScreenLikeimpl.
impl ScreenLike for ContactsListstill omitschange_context(), which risks stale state on network/context switch paths.As per coding guidelines:
src/ui/**/*.rs: ImplementScreenLiketrait with methods:ui(),display_task_result(),display_message(),refresh(),refresh_on_arrival(), andchange_context().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/dashpay/contacts_list.rs` around lines 1006 - 1232, Add a change_context() implementation on the ScreenLike impl for ContactsList that updates the stored context and resets state to avoid stale data: set self.app_context to the new context, clear or reset self.selected_identity, self.contacts, self.message, self.loading = false and self.has_loaded = false, and then call self.load_contacts_from_database() (or self.refresh()) to repopulate contacts for the new context; ensure you reference the ContactsList methods/fields (change_context, load_contacts_from_database, selected_identity, contacts, message, loading, has_loaded, app_context) when making the change.
🟡 Minor comments (2)
src/ui/tokens/resume_tokens_screen.rs-173-178 (1)
173-178:⚠️ Potential issue | 🟡 MinorWallet lookup emits an error banner in expected no-key scenarios.
When
possible_keyisNone,get_selected_wallet(..., None)returns an error and immediately shows a banner, even though the UI already handles “no key” normally.Suggested fix
-let selected_wallet = - get_selected_wallet(&identity_token_info.identity, None, possible_key.as_ref()) - .unwrap_or_else(|e| { - set_error_banner(&e); - None - }); +let selected_wallet = if let Some(key) = possible_key.as_ref() { + get_selected_wallet(&identity_token_info.identity, None, Some(key)).unwrap_or_else(|e| { + set_error_banner(&e); + None + }) +} else { + None +};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/tokens/resume_tokens_screen.rs` around lines 173 - 178, The code calls get_selected_wallet(...) even when possible_key is None, causing set_error_banner to be shown for an expected "no key" case; change the logic to only call get_selected_wallet when possible_key.is_some() (using a match or if let Some(key) = possible_key.as_ref()) and otherwise set selected_wallet = None without calling set_error_banner, or if you must call get_selected_wallet, match its Err and suppress the banner for the "no-key" error variant; reference get_selected_wallet, set_error_banner, possible_key, and identity_token_info.identity to locate and update the code.src/app.rs-911-913 (1)
911-913:⚠️ Potential issue | 🟡 MinorConnection status banner can vanish after eviction and never return until state changes.
For non-
Connectingstates, unchanged state returns early. With bounded global banners, the connection banner can be evicted and not reasserted.Suggested patch
- if !state_changed && current_state != OverallConnectionState::Connecting { + if !state_changed && current_state == OverallConnectionState::Synced { return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app.rs` around lines 911 - 913, The early return when !state_changed && current_state != OverallConnectionState::Connecting allows a previously-evicted connection banner to never be reasserted; modify the logic so that before returning you either verify the connection banner is currently displayed or explicitly reassert it. Concretely, change the condition around state_changed/current_state to also check banner presence (e.g., call or inline a check like is_connection_banner_visible()) and if the banner is missing call the routine that shows the connection banner (e.g., show_connection_banner() or reassert_connection_banner()) instead of returning; this ensures the banner is restored even when the overall connection state hasn't changed.`
🧹 Nitpick comments (3)
src/ui/dashpay/contact_requests.rs (1)
1041-1052: String-based error classification is fragile but tracked.The
message.contains()approach for detecting encryption/decryption key errors could break if error message formats change. The TODO referencing issue#660appropriately tracks this for future improvement. Consider using typed error variants propagated through the task result system to make this more robust.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/dashpay/contact_requests.rs` around lines 1041 - 1052, The display_message function currently uses fragile string matching (message.contains) to set DashPayError variants for encryption/decryption key issues; update the flow so display_message(&mut self, message: &str, message_type: MessageType) consumes a typed error instead of parsing strings: change callers that invoke display_message to pass a Result/enum error (or an Option<DashPayError>) so you can match on concrete variants (DashPayError::MissingEncryptionKey, DashPayError::MissingDecryptionKey) rather than testing message contents, and remove the message.contains branches (leave the TODO referencing `#660`). Keep MessageType handling for banner logic but ensure the task result types propagate the DashPayError up to where display_message is called.src/ui/contracts_documents/document_action_screen.rs (1)
478-481: Progress is rendered twice (global banner + inline spinner).These spinner branches duplicate the new global elapsed/banner progress and create inconsistent UX.
As per coding guidelines "Use
MessageBannerfor user-facing messages and letisland_central_panel()render global banners centrally".Also applies to: 528-531, 585-588, 924-927
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/contracts_documents/document_action_screen.rs` around lines 478 - 481, The inline spinner branches that check self.broadcast_status == BroadcastStatus::Fetching and call ui.spinner() should be removed and replaced by relying on the centralized MessageBanner rendered via island_central_panel(); update the code locations around the BroadcastStatus checks (e.g., the blocks that call ui.spinner() at the spots referencing self.broadcast_status and BroadcastStatus::Fetching) to not render an inline spinner and ensure any user-facing progress uses MessageBanner/island_central_panel() instead so the global banner is the sole progress indicator.src/app.rs (1)
903-903: Place&AppContextfirst afterselfinupdate_connection_banner.This signature currently puts
ctxbefore app context, which breaks the repo-wide Rust signature rule.Suggested patch
- fn update_connection_banner(&mut self, ctx: &egui::Context, app_context: &Arc<AppContext>) { + fn update_connection_banner(&mut self, app_context: &AppContext, ctx: &egui::Context) { let connection_status = app_context.connection_status(); let current_state = connection_status.overall_state(); let state_changed = self.previous_connection_state != Some(current_state); ... } - self.update_connection_banner(ctx, &active_context); + self.update_connection_banner(active_context.as_ref(), ctx);As per coding guidelines: "
**/*.rs: Place&AppContext(orOption<&AppContext>) as the first parameter afterselfin method signatures."Also applies to: 1264-1264
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app.rs` at line 903, Update the method signature of update_connection_banner to place &AppContext (or Option<&AppContext>) immediately after self (e.g., fn update_connection_banner(&mut self, app_context: &AppContext, ctx: &egui::Context)) and make the same swap for any other methods flagged (the occurrence around line ~1264); update all callers to pass the AppContext argument first so the repo-wide Rust signature rule is preserved.
🤖 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/backend_task/core/mod.rs`:
- Around line 665-668: The code is brittle because it inspects e.to_string() in
the map_err closure after TransactionBuilder::select_inputs to decide between
WalletError::InsufficientFunds and WalletError::TransactionBuild; instead,
change the error handling to match on a typed error variant from the dash-sdk
(e.g. the error enum returned by TransactionBuilder::select_inputs) and map
those variants to WalletError::InsufficientFunds or
WalletError::TransactionBuild accordingly; if the SDK currently only exposes
string errors, replace the string-match with a clear TODO + doc-comment and
return a TaskError/WalletError::TransactionBuild containing the original error
(preserving the error type) and add a follow-up issue/request to the upstream
dash-sdk to expose a typed error enum so we can remove string parsing in the
closure that currently maps errors from TransactionBuilder::select_inputs.
In `@src/ui/components/message_banner.rs`:
- Around line 800-814: The replace and replace_with_elapsed methods currently
always store Some(handle) returned by MessageBanner::set_global even when
set_global may return a no-op handle for empty text; update both methods
(replace and replace_with_elapsed) to only set self = Some(handle) if the
returned BannerHandle indicates it actually inserted/active (e.g., check a
method like handle.is_active() / handle.is_inserted() on the BannerHandle), and
only call handle.with_elapsed() when the handle is active; use the BannerHandle
API to detect active vs no-op handles instead of unconditionally storing them.
- Around line 388-395: In replace_global, avoid creating duplicate banners when
old_text and new_text both exist: before calling b.reset_to inside the
banners.iter_mut().find(|b| b.text == old_text) branch, search for an existing
banner with new_text (e.g., banners.iter().find(|e| e.text == new_text)) and if
found reuse its key (set key = existing.key) and remove or skip the old banner
instead of resetting it; only call b.reset_to(new_text, message_type) when no
other banner already holds new_text. Use the identifiers replace_global,
banners, old_text, new_text, b.reset_to, existing, and key to locate and
implement the check-and-reuse logic.
In `@src/ui/contracts_documents/document_action_screen.rs`:
- Around line 1605-1615: The display_message function is incorrectly resetting
broadcast_status for every message; change it so only error/warning messages
(i.e., when message_type is MessageType::Error or MessageType::Warning) clear
refresh_banner and set broadcast_status = BroadcastStatus::NotBroadcasted, while
ordinary info/status messages should only update backend_message (and rely on
MessageBanner/island_central_panel() to render global banners). Update the logic
in display_message (reference: display_message, refresh_banner, backend_message,
broadcast_status, BroadcastStatus::NotBroadcasted) to avoid mutating broadcast
state for non-error messages.
In `@src/ui/contracts_documents/register_contract_screen.rs`:
- Around line 69-71: The current initialization swallows errors by calling
unwrap_or(None) on get_selected_wallet in the selected_wallet assignment; change
this to handle the Result properly by matching on
get_selected_wallet(selected_qualified_identity, Some(app_context), None) (or
using ? to propagate) so that failures are surfaced instead of converted to
None—either propagate the error from the enclosing function (using the ?
operator) or explicitly log/return the error where appropriate; update
references to selected_qualified_identity, get_selected_wallet, and
selected_wallet accordingly.
- Around line 518-520: The wallet auto-open error is being stored in
self.error_message but the UI displays errors from broadcast_status/global
banners; change the error handling in the try_open_wallet_no_password(...) Err
branch to publish the error to the global banner field (broadcast_status)
instead of self.error_message — e.g., replace setting self.error_message =
Some(e) with assigning/setting self.broadcast_status = Some(e.to_string()) or
calling the existing helper that posts to broadcast_status so the error is
visible to users.
In `@src/ui/contracts_documents/update_contract_screen.rs`:
- Around line 73-75: The code hides errors from get_selected_wallet by calling
unwrap_or(None) on its Result; change the selected_wallet assignment to handle
the Result explicitly instead of silencing failures—call
get_selected_wallet(selected_qualified_identity, Some(app_context), None) and
either propagate the error with the ? operator (if the caller returns a Result)
or match the Result and return/log the error, converting only Ok(value) into the
Option for selected_wallet; reference the get_selected_wallet call and the
selected_wallet binding so the change replaces unwrap_or(None) with proper
Result handling.
- Around line 531-533: The wallet auto-open error is only stored in
self.error_message but the UI shows errors via the banner/broadcast state;
replace the assignment to self.error_message in the
try_open_wallet_no_password(error) Err branch with a call that publishes the
error to the active banner/broadcast mechanism used by this screen (e.g., call
the component's banner or broadcast helper to set a visible error message using
the error `e`), and keep or remove self.error_message only if you also
synchronize it with the banner state so users see the failure via the standard
UI path.
In `@src/ui/dashpay/contact_info_editor.rs`:
- Around line 293-295: display_message currently does nothing so when
save_contact_info sets self.saving = true and the backend returns an error or
warning path the spinner never resets; update display_message (in
ContactInfoEditor) to set self.saving = false for non-success message types
(e.g., MessageType::Error or MessageType::Warning) and leave success behavior
unchanged so that the saving flag is cleared when an error/warning is received.
In `@src/ui/dashpay/contacts_list.rs`:
- Around line 1069-1073: The code calls
self.app_context.db.clear_dashpay_contacts(...).or_show_error(self.app_context.egui_ctx()).ok()
which swallows DB errors and lets in-memory self.contacts be updated even when
persistence fails; change this to check the Result returned from
clear_dashpay_contacts (using the or_show_error path) and if it is an Err, abort
the sync/return early instead of continuing to mutate self.contacts. Apply the
same pattern to the save loop (the calls wrapped with .or_show_error(...) in the
block around lines 1105–1132): check each save result (e.g.,
save_dashpay_contact) and on failure stop further processing and do not update
self.contacts, optionally returning the error to the caller or showing it via
or_show_error before returning.
In `@src/ui/dashpay/qr_scanner.rs`:
- Around line 373-375: display_message is a no-op so backend send failures never
clear the QR scanner's sending flag and the UI can remain stuck; update fn
display_message(&mut self, _message: &str, message_type: MessageType) in
qr_scanner.rs to clear self.sending (e.g., self.sending = false) when called for
error or completion messages, and ensure it routes the message to the global
banner/AppState if needed — reference display_message, MessageType, and the
self.sending field so the send flow (the send task result handling that calls
display_message) will unblock the UI on failures as well as successes.
In `@src/ui/identities/keys/key_info_screen.rs`:
- Around line 656-660: The code is displaying raw backend/database error strings
via MessageBanner::set_global using format!("Issue saving: {}", e); change this
to show a generic, user-friendly message (e.g., "Unable to save changes. Please
try again.") and stop interpolating the raw error into the banner; instead
record the full technical error to a secure debug sink (e.g., app logger or
telemetry) and surface technical details only via a separate expandable/details
UI or a "Show details" dialog/button. Update both occurrences (the call with
self.app_context.egui_ctx() and the other similar call around lines 806-810) to
use the sanitized banner text and route e (the original error object/string) to
the logger or the details UI rather than directly into
MessageBanner::set_global.
In `@src/ui/tokens/claim_tokens_screen.rs`:
- Around line 281-288: The refresh logic currently only runs when self.identity
is Some, preventing recovery if identity starts missing; change refresh() to
always call app_context.load_local_qualified_identities() (remove the initial
`if let Some(current) = &self.identity &&` guard), then: if
load_local_qualified_identities() returns Ok(all) and self.identity is Some,
find the matching identity by comparing id() and update self.identity to that
updated value; if self.identity is None and `all` is non-empty, set
self.identity to the first/appropriate entry from `all` (e.g.,
all.into_iter().next()) so the UI can recover when an identity appears. Ensure
you reference and update the existing symbols: self.identity, refresh(), and
app_context.load_local_qualified_identities().
- Around line 385-388: The code sets self.error_message when
try_open_wallet_no_password(wallet) fails but does not flip the screen-visible
flag, so the error never appears; update the block around
try_open_wallet_no_password to set self.wallet_open_attempted = true whenever an
open attempt occurs and, on Err(e), assign self.error_message = Some(e) and
ensure any render-triggering flag/state (here self.wallet_open_attempted) is set
so the screen’s active rendering path will display the error (i.e., move the
wallet_open_attempted = true into both the success and error paths so the UI
shows self.error_message).
In `@src/ui/tokens/mint_tokens_screen.rs`:
- Around line 290-304: The code currently always parses
self.recipient_identity_id via Identifier::from_string_try_encodings and returns
an error path, which prevents optional/empty recipient flows; change the logic
to only attempt parsing when self.recipient_identity_id is non-empty (e.g.,
trim().is_empty() check), set an Option<Identifier> (None when empty,
Some(parsed_id) when present), and on parse failure set MintTokensStatus::Error
and call MessageBanner::set_global as before; then update the call site that
currently sends Some(receiver_id) (around the mint/send invocation) to pass the
Option<Identifier> you created instead of forcing Some(receiver_id). Ensure you
reference and update the parsing block with
Identifier::from_string_try_encodings and the place that constructs the
mint/send arguments to accept the optional receiver.
In `@src/ui/tokens/set_token_price_screen.rs`:
- Around line 1064-1069: The code is calling MessageBanner::set_global(ui.ctx(),
...) each render which makes the error banner non-dismissible; change the logic
so the banner is only set once when the error condition transitions to true (or
in response to the user/action) instead of every frame: introduce or reuse a
component-level state flag (e.g., show_set_price_error or
last_set_price_error_state) and call MessageBanner::set_global only when that
flag changes from false to true (or when the user attempts the forbidden
action), or clear the flag when the condition no longer applies; update the
branch around MessageBanner::set_global(ui.ctx(), "Only group members can set
price on this token", MessageType::Error) to check and flip that state rather
than unconditionally calling set_global during rendering.
In `@src/ui/tokens/tokens_screen/mod.rs`:
- Around line 3097-3102: The code currently clears self.operation_banner
unconditionally in display_task_result, which drops elapsed-progress for
intermediate-success steps; change the logic to only clear (and call h.clear())
when the BackendTaskSuccessResult represents a final/terminal success for the
overall flow (e.g., final completion flag or success variant), otherwise leave
self.operation_banner intact so progress/elapsed remains visible; locate
display_task_result and modify the conditional to check a final/completion
indicator on backend_task_success_result (e.g., is_final(), .complete, or
matching a Completed variant) before taking and clearing operation_banner.
- Around line 3085-3091: The branch that matches messages containing "Added
token" | "Token already added" | "Saved token to db" clears local state
(self.adding_token_start_time and self.adding_token_name) but does not emit
deterministic success feedback; after clearing those fields, explicitly emit a
success notification (for example call the existing global/banner notification
API or dispatch a UI message) with a deterministic message that includes the
token name (use self.adding_token_name before clearing) or a fixed success
string if name is None; update the handler in tokens_screen/mod.rs where those
matches occur so the UI always receives an explicit success event (e.g., call
show_global_banner or enqueue an AddTokenSuccess event) immediately after
clearing state.
In `@src/ui/tokens/tokens_screen/token_creator.rs`:
- Around line 924-925: Don’t display the raw error string `e` in the UI: replace
the `MessageBanner::set_global(context, format!("Error building contract V1:
{e}"), ...)` call with a user-friendly, non-sensitive message like "Failed to
create contract. Please try again or contact support.", and move the full
technical details to a developer-facing sink (e.g., call the app logger or emit
to a debug/diagnostic channel) by logging `e` (e.g., `log::error!` or the
existing logger) so the `token_creator` code reports the real error for
developers but only the sanitized message is shown to users; ensure the unique
symbols affected are `MessageBanner::set_global` and the error variable `e`.
In `@src/ui/tools/transition_visualizer_screen.rs`:
- Around line 93-96: Avoid resetting self.broadcast_status to
TransitionBroadcastStatus::NotStarted on every input change; instead, only clear
it when no broadcast is in flight. Modify the code that currently does
self.broadcast_status = TransitionBroadcastStatus::NotStarted so it first checks
the current status (e.g., if self.broadcast_status !=
TransitionBroadcastStatus::Submitting) and only then set NotStarted; this
preserves the Submitting state while a broadcast task is running and prevents
concurrent dispatches.
---
Outside diff comments:
In `@src/app.rs`:
- Around line 1084-1095: Replace the direct use of err.to_string() for
user-facing text with a sanitized user message for non-developers: call
something like let user_msg = if self.current_app_context().is_developer_mode()
{ err.to_string() } else { sanitized or generic message ("An error occurred
while performing the task.") } then pass user_msg into
MessageBanner::set_global(ctx, &user_msg, MessageType::Error) and
visible_screen_mut().display_message(&user_msg, MessageType::Error); keep
handle.with_details(&err) only inside the is_developer_mode() branch so raw
TaskError details are shown only to developers.
In `@src/backend_task/core/mod.rs`:
- Around line 528-531: Scaled amounts are currently truncated to zero in
scaled_recipients (created from recipients and scale_factor), which can produce
invalid zero-valued outputs; detect when any scaled amount becomes 0 and return
a deterministic error (e.g., an AmountTooSmallAfterFees / TransferAmountTooSmall
variant) instead of building outputs, or alternatively enforce a minimum of 1
and adjust logic consistently; update the code that maps recipients ->
scaled_recipients (and the similar logic around lines 542-563) to check for any
zero entries after scaling and fail early with that error (or apply the chosen
minimum) so the caller sees a clear "amount too small after fees" response.
In `@src/context/connection_status.rs`:
- Around line 374-399: The code only handles TaskResult::Success variants for
BackendTaskSuccessResult::CoreItem chainlock messages, ignoring
TaskResult::Error which can leave rpc_online/states stale; add explicit handling
for TaskResult::Error (and other non-success cases related to
CoreItem::ChainLocks and CoreItem::ChainLock) so that when a chainlock poll
fails you call self.set_rpc_online(false) and self.refresh_state(), and keep
existing success handling in update_from_chainlocks and set_rpc_online(true);
update the match on TaskResult (or add an else arm) to detect errors for the
same tasks and drive the connection state offline and refresh_state()
accordingly.
In `@src/spv/manager.rs`:
- Around line 545-556: The clear_data_dir path currently only checks
status.is_active() which allows SpvStatus::Error (an "inactive" but
not-fully-teardown state) to proceed; update clear_data_dir to gate on a
stronger runtime liveness check (e.g., require the status to be explicitly
stopped/terminated rather than merely not active — treat SpvStatus::Error as
unsafe) by using read_status()/status and refusing to proceed if the status is
Active or in an Error/terminating state, and additionally defensively clear any
lingering client handle by locking and setting self.spv_client (or the
spv_client mutex) to None before clearing storage to avoid stale handles
surviving directory deletion; keep references to clear_data_dir, read_status,
storage, spv_client and SpvStatus::Error when making the change.
- Around line 633-663: The synchronous bridge currently calls
tokio::task::block_in_place() with tokio::runtime::Handle::current() which can
panic; replace that with a safe runtime detection: use
tokio::runtime::Handle::try_current() and inspect the runtime flavor, and branch
so that when a compatible runtime exists (Some(handle) and
handle.runtime_flavor() != RuntimeFlavor::CurrentThread) you keep the
block_in_place + handle.block_on(...) path, but when try_current() is None or
the runtime is CurrentThread create a temporary tokio::runtime::Runtime::new()
and use its block_on(...) (or otherwise run the async client call on a blocking
thread) to execute client.get_quorum_at_height(...) safely; update the code
around spv_client.load_full(), tokio::task::block_in_place,
tokio::runtime::Handle::current(), and client.get_quorum_at_height to implement
this branching so no panic occurs.
In `@src/ui/identities/add_existing_identity_screen.rs`:
- Around line 781-803: When handling DPNS wallet selection in
ComboBox::from_id_salt("dpns_wallet_selector") ensure you reset the flag
wallet_open_attempted whenever selected_wallet is changed; specifically, set
wallet_open_attempted = false both in the branch that sets self.selected_wallet
= None (the "All unlocked wallets" selectable) and in the branch inside the for
(alias, wallet) in &wallets_snapshot loop where you set self.selected_wallet =
Some(wallet.clone()) so the new selection can trigger auto-open attempts like
other selectors.
- Around line 683-714: The loading banner is created before parsing
identity_index_input but not cleared on parse failure, leaving refresh_banner
active; update the error branch so it takes and clears the existing banner
handle (e.g., self.refresh_banner.take().map(|h| h.clear()) or the equivalent
dismiss/close method on the MessageBanner handle) before setting
add_identity_status back to NotStarted and showing the "Invalid identity index"
error via MessageBanner::set_global, ensuring no stale progress UI remains;
locate this logic around add_identity_status, refresh_banner,
identity_index_input parsing, and MessageBanner::set_global in the
AddExistingIdentityScreen flow.
In `@src/ui/tokens/tokens_screen/mod.rs`:
- Around line 2242-2254: The current code calls Identifier::from_string(...) and
then unwrap_or_default(), which silently converts invalid base58 input into
Identifier::default(); change this to explicitly handle the Result from
Identifier::from_string for perpetual_dist_recipient_identity_input: match or if
let Err(e) => call MessageBanner::set_global(self.app_context.egui_ctx(),
"Invalid base58 identifier for perpetual distribution recipient",
MessageType::Error) and return Err with a descriptive message (including e if
useful), and only construct TokenDistributionRecipient::Identity(id) when
Identifier::from_string returns Ok(id).
In `@src/ui/wallets/send_screen.rs`:
- Around line 2663-2708: The code currently calls
self.send_banner.take_and_clear() before matching on
crate::backend_task::BackendTaskSuccessResult, which prematurely clears the send
progress banner for unrelated results; remove that unconditional call and
instead invoke self.send_banner.take_and_clear() only inside the match arms that
correspond to send-related results (e.g.,
BackendTaskSuccessResult::WalletPayment, ::TransferredCredits,
::PlatformAddressFunded, ::PlatformAddressWithdrawal,
::PlatformCreditsTransferred) immediately before updating self.send_status
(SendStatus::Complete) so unrelated task results do not hide an in-flight send
banner.
---
Minor comments:
In `@src/app.rs`:
- Around line 911-913: The early return when !state_changed && current_state !=
OverallConnectionState::Connecting allows a previously-evicted connection banner
to never be reasserted; modify the logic so that before returning you either
verify the connection banner is currently displayed or explicitly reassert it.
Concretely, change the condition around state_changed/current_state to also
check banner presence (e.g., call or inline a check like
is_connection_banner_visible()) and if the banner is missing call the routine
that shows the connection banner (e.g., show_connection_banner() or
reassert_connection_banner()) instead of returning; this ensures the banner is
restored even when the overall connection state hasn't changed.`
In `@src/ui/tokens/resume_tokens_screen.rs`:
- Around line 173-178: The code calls get_selected_wallet(...) even when
possible_key is None, causing set_error_banner to be shown for an expected "no
key" case; change the logic to only call get_selected_wallet when
possible_key.is_some() (using a match or if let Some(key) =
possible_key.as_ref()) and otherwise set selected_wallet = None without calling
set_error_banner, or if you must call get_selected_wallet, match its Err and
suppress the banner for the "no-key" error variant; reference
get_selected_wallet, set_error_banner, possible_key, and
identity_token_info.identity to locate and update the code.
---
Duplicate comments:
In `@src/ui/dashpay/contacts_list.rs`:
- Around line 1006-1232: Add a change_context() implementation on the ScreenLike
impl for ContactsList that updates the stored context and resets state to avoid
stale data: set self.app_context to the new context, clear or reset
self.selected_identity, self.contacts, self.message, self.loading = false and
self.has_loaded = false, and then call self.load_contacts_from_database() (or
self.refresh()) to repopulate contacts for the new context; ensure you reference
the ContactsList methods/fields (change_context, load_contacts_from_database,
selected_identity, contacts, message, loading, has_loaded, app_context) when
making the change.
In `@src/ui/identities/register_dpns_name_screen.rs`:
- Around line 301-308: The impl for ScreenLike on RegisterDpnsNameScreen is
missing the required methods refresh, refresh_on_arrival, and change_context;
add implementations for ScreenLike::refresh(&mut self) to update any cached
state and mark view dirty (use existing refresh_banner or related fields),
ScreenLike::refresh_on_arrival(&mut self) to perform the same actions when the
screen becomes active (e.g., set refresh_banner.take_and_clear() or trigger a
full state reload), and ScreenLike::change_context(&mut self, ctx: Context) to
update the screen's context field and clear/initialize any context-dependent
state (like resetting register_dpns_name_status or clearing inputs). Ensure the
method signatures exactly match the ScreenLike trait and that these methods
coexist with the existing ui(), display_task_result(), and display_message()
implementations on RegisterDpnsNameScreen.
---
Nitpick comments:
In `@src/app.rs`:
- Line 903: Update the method signature of update_connection_banner to place
&AppContext (or Option<&AppContext>) immediately after self (e.g., fn
update_connection_banner(&mut self, app_context: &AppContext, ctx:
&egui::Context)) and make the same swap for any other methods flagged (the
occurrence around line ~1264); update all callers to pass the AppContext
argument first so the repo-wide Rust signature rule is preserved.
In `@src/ui/contracts_documents/document_action_screen.rs`:
- Around line 478-481: The inline spinner branches that check
self.broadcast_status == BroadcastStatus::Fetching and call ui.spinner() should
be removed and replaced by relying on the centralized MessageBanner rendered via
island_central_panel(); update the code locations around the BroadcastStatus
checks (e.g., the blocks that call ui.spinner() at the spots referencing
self.broadcast_status and BroadcastStatus::Fetching) to not render an inline
spinner and ensure any user-facing progress uses
MessageBanner/island_central_panel() instead so the global banner is the sole
progress indicator.
In `@src/ui/dashpay/contact_requests.rs`:
- Around line 1041-1052: The display_message function currently uses fragile
string matching (message.contains) to set DashPayError variants for
encryption/decryption key issues; update the flow so display_message(&mut self,
message: &str, message_type: MessageType) consumes a typed error instead of
parsing strings: change callers that invoke display_message to pass a
Result/enum error (or an Option<DashPayError>) so you can match on concrete
variants (DashPayError::MissingEncryptionKey,
DashPayError::MissingDecryptionKey) rather than testing message contents, and
remove the message.contains branches (leave the TODO referencing `#660`). Keep
MessageType handling for banner logic but ensure the task result types propagate
the DashPayError up to where display_message is called.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (43)
CLAUDE.mddocs/ai-design/2026-02-27-banner-review-fixes/pr-604-review-guide.mdsrc/app.rssrc/backend_task/core/mod.rssrc/context/connection_status.rssrc/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/dashpay/add_contact_screen.rssrc/ui/dashpay/contact_info_editor.rssrc/ui/dashpay/contact_requests.rssrc/ui/dashpay/contacts_list.rssrc/ui/dashpay/profile_screen.rssrc/ui/dashpay/qr_code_generator.rssrc/ui/dashpay/qr_scanner.rssrc/ui/dashpay/send_payment.rssrc/ui/identities/add_existing_identity_screen.rssrc/ui/identities/add_new_identity_screen/mod.rssrc/ui/identities/identities_screen.rssrc/ui/identities/keys/add_key_screen.rssrc/ui/identities/keys/key_info_screen.rssrc/ui/identities/register_dpns_name_screen.rssrc/ui/identities/top_up_identity_screen/mod.rssrc/ui/identities/transfer_screen.rssrc/ui/identities/withdraw_screen.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/token_creator.rssrc/ui/tokens/transfer_tokens_screen.rssrc/ui/tokens/unfreeze_tokens_screen.rssrc/ui/tokens/update_token_config.rssrc/ui/tools/transition_visualizer_screen.rssrc/ui/wallets/send_screen.rs
✅ Files skipped from review due to trivial changes (1)
- docs/ai-design/2026-02-27-banner-review-fixes/pr-604-review-guide.md
Comment on lines
+665
to
+668
| .map_err(|e| match e.to_string() { | ||
| msg if msg.contains("Insufficient") => WalletError::InsufficientFunds, | ||
| msg => WalletError::TransactionBuild(msg), | ||
| })?; |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd src && head -680 backend_task/core/mod.rs | tail -30Repository: dashpay/dash-evo-tool
Length of output: 1120
🏁 Script executed:
head -30 src/backend_task/core/mod.rs && echo "---" && sed -n '608,675p' src/backend_task/core/mod.rsRepository: dashpay/dash-evo-tool
Length of output: 4216
🏁 Script executed:
fd -e rs | xargs grep -l "TransactionBuilder" | head -5Repository: dashpay/dash-evo-tool
Length of output: 93
🏁 Script executed:
cat Cargo.toml | grep -A 5 "dash-sdk"Repository: dashpay/dash-evo-tool
Length of output: 314
🏁 Script executed:
rg "TransactionBuilder" -A 5 -B 2 | head -100Repository: dashpay/dash-evo-tool
Length of output: 3041
🏁 Script executed:
rg "select_inputs|SelectionStrategy" -A 3 -B 1Repository: dashpay/dash-evo-tool
Length of output: 1772
🏁 Script executed:
rg "WalletError|TaskError" -A 2 | head -80Repository: dashpay/dash-evo-tool
Length of output: 4198
🏁 Script executed:
rg "enum WalletError|struct WalletError" -A 20Repository: dashpay/dash-evo-tool
Length of output: 1192
🏁 Script executed:
rg "select_inputs" -B 5 -A 10Repository: dashpay/dash-evo-tool
Length of output: 1203
🏁 Script executed:
rg "TransactionBuilder|transaction_builder" -A 8 -B 2 | head -120Repository: dashpay/dash-evo-tool
Length of output: 4184
🏁 Script executed:
fd -e md | xargs grep -l "660\|RUST-002\|String-based error" 2>/dev/nullRepository: dashpay/dash-evo-tool
Length of output: 49
🌐 Web query:
dash-sdk TransactionBuilder select_inputs error types Rust
💡 Result:
dash_sdk (the Dash Platform Rust SDK) does not appear to expose a TransactionBuilder::select_inputs API in its published Rust docs; its public error surface is the crate-wide dash_sdk::Error (SDK methods return this error type) rather than per-transaction-builder error enums. [1]
If you’re actually looking at transaction building + UTXO selection (i.e., something that would plausibly have a select_inputs), that functionality lives in the Dash payment-chain Rust stack (dashpay/rust-dashcore), which explicitly advertises “high-level wallet management with transaction building and UTXO management” (separate from the Platform SDK). [2]
What I could not confirm from public docs pages:
- The exact Rust signature of
select_inputsand its concrete error type (GitHub code search requires login, and the Platform docs aren’t published on docs.rs yet). [3]
Practical way to get the exact error type (authoritative):
- Pin the repo revision you use (or the crate git revision in your
Cargo.lock). - Run:
rg "fn select_inputs|select_inputs\\(" -ninside that checkout. - Read the function signature and follow the returned
Result<_, E>to the defining error enum/type.
Sources:
[1] dash_sdk crate docs (error handling / Error re-export) (dashpay.github.io)
[2] dashpay/rust-dashcore README (wallet management includes tx building + UTXO management) (github.com)
[3] Dash Platform Rust SDK quick start note: docs.rs availability / SDK published-doc status (docs.dash.org)
Citations:
- 1: https://dashpay.github.io/docs-platform/dash_sdk/?utm_source=openai
- 2: https://github.com/dashpay/rust-dashcore
- 3: https://docs.dash.org/projects/platform/en/stable/docs/sdk-rs/quick-start.html?utm_source=openai
🏁 Script executed:
git log --all --oneline --grep="RUST-002\|660\|error.*classification" 2>/dev/null | head -20Repository: dashpay/dash-evo-tool
Length of output: 137
🏁 Script executed:
git log --oneline -30Repository: dashpay/dash-evo-tool
Length of output: 97
🏁 Script executed:
git show 7fa30ed --stat | head -40Repository: dashpay/dash-evo-tool
Length of output: 1895
🏁 Script executed:
rg "RUST-002|#660" -B 2 -A 2Repository: dashpay/dash-evo-tool
Length of output: 1860
🏁 Script executed:
cat src/backend_task/error.rsRepository: dashpay/dash-evo-tool
Length of output: 1404
Avoid string-matching builder errors for control flow; use typed variants if available.
Line 665 uses msg.contains("Insufficient") to classify errors, which is brittle—if SDK error messages change, retry logic may silently fail. The code should match a typed error variant from the builder/selection API instead of parsing to_string().
Note: This is already flagged as TODO(RUST-002) in the code. If the dash-sdk's TransactionBuilder::select_inputs does not yet expose a typed error enum (only to_string() for error classification), document the limitation or request the typed variant from the upstream SDK.
As per coding guidelines: "Backend tasks should return Result<T, TaskError> with typed error variants for domain errors rather than converting to String."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/backend_task/core/mod.rs` around lines 665 - 668, The code is brittle
because it inspects e.to_string() in the map_err closure after
TransactionBuilder::select_inputs to decide between
WalletError::InsufficientFunds and WalletError::TransactionBuild; instead,
change the error handling to match on a typed error variant from the dash-sdk
(e.g. the error enum returned by TransactionBuilder::select_inputs) and map
those variants to WalletError::InsufficientFunds or
WalletError::TransactionBuild accordingly; if the SDK currently only exposes
string errors, replace the string-match with a clear TODO + doc-comment and
return a TaskError/WalletError::TransactionBuild containing the original error
(preserving the error type) and add a follow-up issue/request to the upstream
dash-sdk to expose a typed error enum so we can remove string parsing in the
closure that currently maps errors from TransactionBuilder::select_inputs.
Comment on lines
388
to
395
| if let Some(b) = banners.iter_mut().find(|b| b.text == old_text) { | ||
| key = b.key; | ||
| b.reset_to(new_text.to_string(), message_type); | ||
| } else if let Some(existing) = banners.iter_mut().find(|b| b.text == new_text) { | ||
| b.reset_to(new_text, message_type); | ||
| } else if let Some(existing) = banners.iter().find(|b| b.text == new_text) { | ||
| // Idempotent: if new_text already displayed, return handle without | ||
| // resetting (consistent with set_global behavior). | ||
| key = existing.key; | ||
| existing.reset_to(new_text.to_string(), message_type); | ||
| } else { |
Contributor
There was a problem hiding this comment.
replace_global can violate dedup by creating duplicate text banners.
If old_text exists and new_text already exists in another banner, Line 390 resets old_text to new_text instead of reusing the existing banner key.
Suggested fix
- if let Some(b) = banners.iter_mut().find(|b| b.text == old_text) {
- key = b.key;
- b.reset_to(new_text, message_type);
+ if let Some(old_idx) = banners.iter().position(|b| b.text == old_text) {
+ let old_key = banners[old_idx].key;
+ if let Some(existing_key) = banners
+ .iter()
+ .find(|b| b.text == new_text && b.key != old_key)
+ .map(|b| b.key)
+ {
+ banners.retain(|b| b.key != old_key);
+ set_banners(ctx, banners);
+ return BannerHandle { ctx: ctx.clone(), key: existing_key };
+ }
+ key = old_key;
+ banners[old_idx].reset_to(new_text, message_type);
} else if let Some(existing) = banners.iter().find(|b| b.text == new_text) {📝 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 let Some(b) = banners.iter_mut().find(|b| b.text == old_text) { | |
| key = b.key; | |
| b.reset_to(new_text.to_string(), message_type); | |
| } else if let Some(existing) = banners.iter_mut().find(|b| b.text == new_text) { | |
| b.reset_to(new_text, message_type); | |
| } else if let Some(existing) = banners.iter().find(|b| b.text == new_text) { | |
| // Idempotent: if new_text already displayed, return handle without | |
| // resetting (consistent with set_global behavior). | |
| key = existing.key; | |
| existing.reset_to(new_text.to_string(), message_type); | |
| } else { | |
| if let Some(old_idx) = banners.iter().position(|b| b.text == old_text) { | |
| let old_key = banners[old_idx].key; | |
| if let Some(existing_key) = banners | |
| .iter() | |
| .find(|b| b.text == new_text && b.key != old_key) | |
| .map(|b| b.key) | |
| { | |
| banners.retain(|b| b.key != old_key); | |
| set_banners(ctx, banners); | |
| return BannerHandle { ctx: ctx.clone(), key: existing_key }; | |
| } | |
| key = old_key; | |
| banners[old_idx].reset_to(new_text, message_type); | |
| } else if let Some(existing) = banners.iter().find(|b| b.text == new_text) { | |
| // Idempotent: if new_text already displayed, return handle without | |
| // resetting (consistent with set_global behavior). | |
| key = existing.key; | |
| } else { |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/ui/components/message_banner.rs` around lines 388 - 395, In
replace_global, avoid creating duplicate banners when old_text and new_text both
exist: before calling b.reset_to inside the banners.iter_mut().find(|b| b.text
== old_text) branch, search for an existing banner with new_text (e.g.,
banners.iter().find(|e| e.text == new_text)) and if found reuse its key (set key
= existing.key) and remove or skip the old banner instead of resetting it; only
call b.reset_to(new_text, message_type) when no other banner already holds
new_text. Use the identifiers replace_global, banners, old_text, new_text,
b.reset_to, existing, and key to locate and implement the check-and-reuse logic.
Comment on lines
+800
to
+814
| fn replace(&mut self, ctx: &egui::Context, msg: impl fmt::Display, msg_type: MessageType) { | ||
| self.take_and_clear(); | ||
| *self = Some(MessageBanner::set_global(ctx, msg.to_string(), msg_type)); | ||
| } | ||
|
|
||
| fn replace_with_elapsed( | ||
| &mut self, | ||
| ctx: &egui::Context, | ||
| msg: impl fmt::Display, | ||
| msg_type: MessageType, | ||
| ) { | ||
| self.take_and_clear(); | ||
| let handle = MessageBanner::set_global(ctx, msg.to_string(), msg_type); | ||
| handle.with_elapsed(); | ||
| *self = Some(handle); |
Contributor
There was a problem hiding this comment.
Avoid storing Some(BannerHandle) when message text is empty.
set_global may return a handle without inserting a banner for empty text. At Lines 802/812, that handle is still stored, leaving stale Some(...) state.
Suggested fix
fn replace(&mut self, ctx: &egui::Context, msg: impl fmt::Display, msg_type: MessageType) {
self.take_and_clear();
- *self = Some(MessageBanner::set_global(ctx, msg.to_string(), msg_type));
+ let msg = msg.to_string();
+ if msg.is_empty() {
+ *self = None;
+ return;
+ }
+ *self = Some(MessageBanner::set_global(ctx, msg, msg_type));
}
fn replace_with_elapsed(
&mut self,
ctx: &egui::Context,
msg: impl fmt::Display,
msg_type: MessageType,
) {
self.take_and_clear();
- let handle = MessageBanner::set_global(ctx, msg.to_string(), msg_type);
+ let msg = msg.to_string();
+ if msg.is_empty() {
+ *self = None;
+ return;
+ }
+ let handle = MessageBanner::set_global(ctx, msg, msg_type);
handle.with_elapsed();
*self = Some(handle);
}📝 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
| fn replace(&mut self, ctx: &egui::Context, msg: impl fmt::Display, msg_type: MessageType) { | |
| self.take_and_clear(); | |
| *self = Some(MessageBanner::set_global(ctx, msg.to_string(), msg_type)); | |
| } | |
| fn replace_with_elapsed( | |
| &mut self, | |
| ctx: &egui::Context, | |
| msg: impl fmt::Display, | |
| msg_type: MessageType, | |
| ) { | |
| self.take_and_clear(); | |
| let handle = MessageBanner::set_global(ctx, msg.to_string(), msg_type); | |
| handle.with_elapsed(); | |
| *self = Some(handle); | |
| fn replace(&mut self, ctx: &egui::Context, msg: impl fmt::Display, msg_type: MessageType) { | |
| self.take_and_clear(); | |
| let msg = msg.to_string(); | |
| if msg.is_empty() { | |
| *self = None; | |
| return; | |
| } | |
| *self = Some(MessageBanner::set_global(ctx, msg, msg_type)); | |
| } | |
| fn replace_with_elapsed( | |
| &mut self, | |
| ctx: &egui::Context, | |
| msg: impl fmt::Display, | |
| msg_type: MessageType, | |
| ) { | |
| self.take_and_clear(); | |
| let msg = msg.to_string(); | |
| if msg.is_empty() { | |
| *self = None; | |
| return; | |
| } | |
| let handle = MessageBanner::set_global(ctx, msg, msg_type); | |
| handle.with_elapsed(); | |
| *self = Some(handle); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/ui/components/message_banner.rs` around lines 800 - 814, The replace and
replace_with_elapsed methods currently always store Some(handle) returned by
MessageBanner::set_global even when set_global may return a no-op handle for
empty text; update both methods (replace and replace_with_elapsed) to only set
self = Some(handle) if the returned BannerHandle indicates it actually
inserted/active (e.g., check a method like handle.is_active() /
handle.is_inserted() on the BannerHandle), and only call handle.with_elapsed()
when the handle is active; use the BannerHandle API to detect active vs no-op
handles instead of unconditionally storing them.
Comment on lines
+1605
to
1615
| 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.backend_message = Some(message.to_string()); | ||
| self.broadcast_status = BroadcastStatus::NotBroadcasted; | ||
| } |
Contributor
There was a problem hiding this comment.
display_message() still mutates screen state and can overwrite completed flows.
At Line 1614, every message resets broadcast_status to NotBroadcasted, including non-error messages (e.g., global info/status banners). This can hide a just-completed success state and conflicts with the “side-effects only” intent.
Suggested fix
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.backend_message = Some(message.to_string());
- self.broadcast_status = BroadcastStatus::NotBroadcasted;
+ if matches!(
+ message_type,
+ crate::ui::MessageType::Error | crate::ui::MessageType::Warning
+ ) {
+ if let Some(handle) = self.refresh_banner.take() {
+ handle.clear();
+ }
+ self.backend_message = Some(message.to_string());
+ self.broadcast_status = BroadcastStatus::NotBroadcasted;
+ }
}As per coding guidelines "Use MessageBanner for user-facing messages and let island_central_panel() render global banners centrally".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/ui/contracts_documents/document_action_screen.rs` around lines 1605 -
1615, The display_message function is incorrectly resetting broadcast_status for
every message; change it so only error/warning messages (i.e., when message_type
is MessageType::Error or MessageType::Warning) clear refresh_banner and set
broadcast_status = BroadcastStatus::NotBroadcasted, while ordinary info/status
messages should only update backend_message (and rely on
MessageBanner/island_central_panel() to render global banners). Update the logic
in display_message (reference: display_message, refresh_banner, backend_message,
broadcast_status, BroadcastStatus::NotBroadcasted) to avoid mutating broadcast
state for non-error messages.
Comment on lines
69
to
71
| let selected_wallet = if let Some(ref identity) = selected_qualified_identity { | ||
| get_selected_wallet(identity, Some(app_context), None, &mut error_message) | ||
| get_selected_wallet(identity, Some(app_context), None).unwrap_or(None) | ||
| } else { |
Contributor
There was a problem hiding this comment.
Surface initial wallet lookup failures instead of swallowing them.
At Line 70, unwrap_or(None) hides get_selected_wallet errors during initialization, so startup failures are silent.
Suggested fix
- let selected_wallet = if let Some(ref identity) = selected_qualified_identity {
- get_selected_wallet(identity, Some(app_context), None).unwrap_or(None)
+ let selected_wallet = if let Some(ref identity) = selected_qualified_identity {
+ get_selected_wallet(identity, Some(app_context), None)
+ .or_show_error(app_context.egui_ctx())
+ .unwrap_or(None)
} else {
None
};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/ui/contracts_documents/register_contract_screen.rs` around lines 69 - 71,
The current initialization swallows errors by calling unwrap_or(None) on
get_selected_wallet in the selected_wallet assignment; change this to handle the
Result properly by matching on get_selected_wallet(selected_qualified_identity,
Some(app_context), None) (or using ? to propagate) so that failures are surfaced
instead of converted to None—either propagate the error from the enclosing
function (using the ? operator) or explicitly log/return the error where
appropriate; update references to selected_qualified_identity,
get_selected_wallet, and selected_wallet accordingly.
Comment on lines
+1064
to
1069
| MessageBanner::set_global( | ||
| ui.ctx(), | ||
| "Only group members can set price on this token", | ||
| MessageType::Error, | ||
| ); | ||
| } |
Contributor
There was a problem hiding this comment.
Don’t emit this global error banner every frame.
At Line 1064, this branch runs during rendering. Re-calling MessageBanner::set_global(...) continuously makes the error effectively non-dismissible while the condition holds.
🤖 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 1064 - 1069, The code
is calling MessageBanner::set_global(ui.ctx(), ...) each render which makes the
error banner non-dismissible; change the logic so the banner is only set once
when the error condition transitions to true (or in response to the user/action)
instead of every frame: introduce or reuse a component-level state flag (e.g.,
show_set_price_error or last_set_price_error_state) and call
MessageBanner::set_global only when that flag changes from false to true (or
when the user attempts the forbidden action), or clear the flag when the
condition no longer applies; update the branch around
MessageBanner::set_global(ui.ctx(), "Only group members can set price on this
token", MessageType::Error) to check and flip that state rather than
unconditionally calling set_global during rendering.
Comment on lines
3085
to
3091
| } else if msg.contains("Added token") | ||
| | msg.contains("Token already added") | ||
| | msg.contains("Saved token to db") | ||
| { | ||
| // Clear adding status and show success message | ||
| // Clear adding status (success message shown by global banner) | ||
| self.adding_token_start_time = None; | ||
| self.adding_token_name = None; |
Contributor
There was a problem hiding this comment.
Add-token completion clears local state but does not emit deterministic success feedback.
This branch assumes success messaging is handled elsewhere; from this file’s logic, completion can end with no explicit “added” confirmation.
Suggested fix
// inside BackendTaskSuccessResult::FetchedTokenBalances arm
+if let Some(token_name) = self.adding_token_name.take() {
+ self.adding_token_start_time = None;
+ MessageBanner::set_global(
+ self.app_context.egui_ctx(),
+ format!("Token \"{}\" added successfully!", token_name),
+ MessageType::Success,
+ );
+}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/ui/tokens/tokens_screen/mod.rs` around lines 3085 - 3091, The branch that
matches messages containing "Added token" | "Token already added" | "Saved token
to db" clears local state (self.adding_token_start_time and
self.adding_token_name) but does not emit deterministic success feedback; after
clearing those fields, explicitly emit a success notification (for example call
the existing global/banner notification API or dispatch a UI message) with a
deterministic message that includes the token name (use self.adding_token_name
before clearing) or a fixed success string if name is None; update the handler
in tokens_screen/mod.rs where those matches occur so the UI always receives an
explicit success event (e.g., call show_global_banner or enqueue an
AddTokenSuccess event) immediately after clearing state.
Comment on lines
3097
to
+3102
| fn display_task_result(&mut self, backend_task_success_result: BackendTaskSuccessResult) { | ||
| // Clear any active operation banner | ||
| if let Some(h) = self.operation_banner.take() { | ||
| h.clear(); | ||
| } | ||
|
|
Contributor
There was a problem hiding this comment.
operation_banner is cleared on every success result, including intermediate ones.
This removes elapsed progress too early for multi-step flows (e.g., add-token sequential tasks), so users lose in-progress feedback before completion.
Suggested fix
fn display_task_result(&mut self, backend_task_success_result: BackendTaskSuccessResult) {
- // Clear any active operation banner
- if let Some(h) = self.operation_banner.take() {
- h.clear();
- }
-
match backend_task_success_result {
BackendTaskSuccessResult::DescriptionsByKeyword(descriptions, next_cursor) => {
+ self.operation_banner.take_and_clear();
let mut sr = self.search_results.lock().unwrap();
*sr = descriptions;
// ...
}
// ...
BackendTaskSuccessResult::FetchedTokenBalances => {
+ self.operation_banner.take_and_clear();
self.my_tokens = my_tokens(
&self.app_context,
&self.identities,
&self.all_known_tokens,
&self.token_pricing_data,
);
self.refreshing_status = RefreshingStatus::NotRefreshing;
}
BackendTaskSuccessResult::RegisteredTokenContract => {
+ self.operation_banner.take_and_clear();
self.token_creator_status = TokenCreatorStatus::Complete;
}
_ => {}
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/ui/tokens/tokens_screen/mod.rs` around lines 3097 - 3102, The code
currently clears self.operation_banner unconditionally in display_task_result,
which drops elapsed-progress for intermediate-success steps; change the logic to
only clear (and call h.clear()) when the BackendTaskSuccessResult represents a
final/terminal success for the overall flow (e.g., final completion flag or
success variant), otherwise leave self.operation_banner intact so
progress/elapsed remains visible; locate display_task_result and modify the
conditional to check a final/completion indicator on backend_task_success_result
(e.g., is_final(), .complete, or matching a Completed variant) before taking and
clearing operation_banner.
Comment on lines
+924
to
925
| MessageBanner::set_global(context, format!("Error building contract V1: {e}"), MessageType::Error); | ||
| return; |
Contributor
There was a problem hiding this comment.
Avoid showing raw contract-build errors directly to users.
At Line 924, the banner includes the full internal error string. Prefer a user-safe message and attach technical details separately.
Suggested adjustment
- MessageBanner::set_global(context, format!("Error building contract V1: {e}"), MessageType::Error);
+ MessageBanner::set_global(context, "Failed to build token contract", MessageType::Error)
+ .with_details(e);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/ui/tokens/tokens_screen/token_creator.rs` around lines 924 - 925, Don’t
display the raw error string `e` in the UI: replace the
`MessageBanner::set_global(context, format!("Error building contract V1: {e}"),
...)` call with a user-friendly, non-sensitive message like "Failed to create
contract. Please try again or contact support.", and move the full technical
details to a developer-facing sink (e.g., call the app logger or emit to a
debug/diagnostic channel) by logging `e` (e.g., `log::error!` or the existing
logger) so the `token_creator` code reports the real error for developers but
only the sanitized message is shown to users; ensure the unique symbols affected
are `MessageBanner::set_global` and the error variable `e`.
Comment on lines
+93
to
96
| // Reset the broadcast status so we no longer show old states | ||
| // from a previous parse/broadcast. | ||
| self.broadcast_status = TransitionBroadcastStatus::NotStarted; | ||
|
|
Contributor
There was a problem hiding this comment.
Keep Submitting state stable while a broadcast task is still in flight.
Resetting broadcast_status to NotStarted on each input change re-enables submit and can dispatch concurrent broadcasts before the prior one completes.
🛠️ Proposed fix
- // Reset the broadcast status so we no longer show old states
- // from a previous parse/broadcast.
- self.broadcast_status = TransitionBroadcastStatus::NotStarted;
+ // Don't reset while a broadcast is in-flight.
+ if !matches!(self.broadcast_status, TransitionBroadcastStatus::Submitting) {
+ self.submit_banner.take_and_clear();
+ self.broadcast_status = TransitionBroadcastStatus::NotStarted;
+ }📝 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
| // Reset the broadcast status so we no longer show old states | |
| // from a previous parse/broadcast. | |
| self.broadcast_status = TransitionBroadcastStatus::NotStarted; | |
| // Don't reset while a broadcast is in-flight. | |
| if !matches!(self.broadcast_status, TransitionBroadcastStatus::Submitting) { | |
| self.submit_banner.take_and_clear(); | |
| self.broadcast_status = TransitionBroadcastStatus::NotStarted; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/ui/tools/transition_visualizer_screen.rs` around lines 93 - 96, Avoid
resetting self.broadcast_status to TransitionBroadcastStatus::NotStarted on
every input change; instead, only clear it when no broadcast is in flight.
Modify the code that currently does self.broadcast_status =
TransitionBroadcastStatus::NotStarted so it first checks the current status
(e.g., if self.broadcast_status != TransitionBroadcastStatus::Submitting) and
only then set NotStarted; this preserves the Submitting state while a broadcast
task is running and prevents concurrent dispatches.
shumkov
approved these changes
Mar 3, 2026
Contributor
shumkov
left a comment
shumkov
left a comment
There was a problem hiding this comment.
Conceptually, I like the design. Didn't review in detail.
std's backtrace symbolizer (gimli) silently fails to resolve symbols when parsing ~790MB of DWARF debug sections in our 1.1GB debug binary, producing backtraces full of <unknown> frames. Set `debug = "line-tables-only"` for dependency crates while keeping full debuginfo for our own code. This cuts DWARF from 790MB to ~354MB and binary size from 1.1GB to 600MB, letting gimli actually resolve panic backtraces. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
6 tasks
lklimek
added a commit
that referenced
this pull request
Mar 18, 2026
* docs: add unified message display design documents Add UX specification, technical architecture, and HTML mockup for the MessageBanner component that will replace the ~50 ad-hoc error/message display implementations across screens with a single reusable component. Key design decisions: - Per-screen MessageBanner with show()/set_message() API - All colors via DashColors (zero hardcoded Color32 values) - 4 severity levels: Error, Warning, Success, Info - Auto-dismiss for Success/Info (5s), persistent for Error/Warning - Follows Component Design Pattern conventions (private fields, builder, show) - No changes to BackendTask/TaskResult/AppState architecture Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * feat: add MessageType::Warning * chore: initial implementation * chore: docs * chore: self review * refactor(context): replace RwLock<Sdk> with ArcSwap<Sdk> (#600) * refactor(context): replace RwLock<Sdk> with ArcSwap<Sdk> Sdk is internally thread-safe (Arc, ArcSwapOption, atomics) and all methods take &self. The RwLock was adding unnecessary contention across backend tasks. Using ArcSwap instead of plain Sdk because reinit_core_client_and_sdk() needs to atomically swap the entire Sdk instance when config changes. ArcSwap provides lock-free reads with atomic swap for the rare write. Suggested-by: lklimek * fix: address CodeRabbit review findings for ArcSwap migration - Fix import ordering: move arc_swap::ArcSwap before crossbeam_channel - Remove redundant SDK loads in load_identity_from_wallet, register_dpns_name, and load_identity — use the sdk parameter already passed to these functions - Fix stale TODO referencing removed sdk.read().unwrap() API - Rename sdk_guard → sdk in transfer, withdraw_from_identity, and refresh_loaded_identities_dpns_names (no longer lock guards) - Pass &sdk to run_platform_info_task from dispatch site instead of reloading internally - Fix leftover sdk.write() call in context_provider.rs (RwLock remnant) - Add missing Color32 import in wallets dialogs Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * refactor: address remaining CodeRabbit review feedback on ArcSwap migration - Move SDK load outside for loop in refresh_loaded_identities_dpns_names.rs so it's loaded once for the batch instead of on each iteration - Update stale TODO comment in default_platform_version() to reflect that this is a free function with no sdk access Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * refactor: consolidate double read-lock on spv_context_provider Clone the SPV provider in a single lock acquisition, then bind app context on the clone instead of locking twice. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> --------- Co-authored-by: PastaClaw <thepastaclaw@users.noreply.github.com> Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com> * refactor: remove unused Insight API and show_in_ui config fields (#597) * refactor: remove unused Insight API references The `insight_api_url` field in `NetworkConfig` and its associated `insight_api_uri()` method were never used in production code (both marked `#[allow(dead_code)]`). Remove the field, method, config entries, env example lines, and related tests. https://claude.ai/code/session_01HWPmCJHT8KTZGP9bFiksjn * refactor: remove unused `show_in_ui` field from NetworkConfig The `show_in_ui` field was defined on `NetworkConfig` and serialized in `save()`, but never read by any production code to control network visibility. Remove the field, its serialization, env example lines, and test references. https://claude.ai/code/session_01HWPmCJHT8KTZGP9bFiksjn * fix: add missing `Color32` import in wallet dialogs https://claude.ai/code/session_01HWPmCJHT8KTZGP9bFiksjn --------- Co-authored-by: Claude <noreply@anthropic.com> * build: add Flatpak packaging and CI workflow (#589) * build: remove snap version * build: add Flatpak packaging and CI workflow Add Flatpak build manifest, desktop entry, AppStream metadata, and GitHub Actions workflow for building and distributing Flatpak bundles. Uses freedesktop 25.08 runtime with rust-stable and llvm21 extensions. No application source code changes required - works in SPV mode by default. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * build: address review findings for Flatpak packaging - Pin GitHub Actions to commit SHAs for supply chain security - Upgrade softprops/action-gh-release from v1 to v2.2.2 - Remove redundant --socket=x11 (fallback-x11 suffices) - Remove duplicate tag trigger preventing double builds on release - Remove duplicate env vars inherited from top-level build-options - Add Flatpak build artifacts to .gitignore - Add bugtracker URL to AppStream metainfo - Remove deprecated <categories> from metainfo (use .desktop instead) - Add Terminal=false and Keywords to desktop entry - Add disk space check after SDK install in CI - Rename artifact to include architecture suffix Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * build: simplify CI workflows for Linux-only releases - Remove "Free disk space" step from flatpak and release workflows - Remove Windows and macOS builds from release workflow - Use "ubuntu" runner image instead of pinned versions - Clean up unused matrix.ext references Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * build: attach to existing releases instead of creating new ones - Replace release-creating job with attach-to-release (only on release event) - Add 14-day retention for build artifacts - On tag push or workflow_dispatch, only upload artifacts (no release) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * revert: restore release.yml to original v1.0-dev version The release workflow changes were out of scope for the Flatpak PR. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: address CodeRabbit review comments - Fix CRLF line endings in Flatpak manifest (convert to LF) - Set app_id on ViewportBuilder to match desktop StartupWMClass - Use --locked flag for reproducible cargo builds in Flatpak - Rename --repo=repo to --repo=flatpak-repo to match .gitignore - Add architecture note for protoc x86_64 binary Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * docs: add Flatpak install instructions to README Add a dedicated section for installing via Flatpak on Linux, clarify that prerequisites are only needed for building from source, and rename "Installation" to "Build from Source" for clarity. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: match StartupWMClass to Flatpak app_id Use reverse-DNS format org.dash.DashEvoTool to match the Wayland app_id set via ViewportBuilder::with_app_id(). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: use ** glob for branch trigger to match feat/flatpak Single * doesn't match path separators in GitHub Actions branch filters. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * feat: add aarch64 Flatpak build and caching to CI - Add matrix strategy for parallel x86_64 and aarch64 builds - Patch protoc URL/sha256 per architecture at build time - Cache .flatpak-builder directory keyed on arch + manifest + lockfile - Pin actions/cache to SHA Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: convert desktop and metainfo files to LF line endings Flatpak builder validates desktop files and rejects CRLF line endings. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * build: cancel in-progress Flatpak builds on new push Add concurrency group keyed on git ref so a new push cancels any running build for the same branch or release. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: address PR review findings for Flatpak packaging - Remove unnecessary --filesystem=xdg-config/dash-evo-tool:create (Flatpak already redirects XDG_CONFIG_HOME to sandbox) - Add categories and keywords to AppStream metainfo for discoverability - Update README with both x86_64/aarch64 install commands, uninstall instructions, and Flatpak data path note - Clarify aarch64 comment in manifest to reference CI sed patching Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * chore: workflow timeout and perms * fix: move permissions to job level in Flatpak workflow Step-level permissions are not valid in GitHub Actions. Move contents: write to the job level where it is needed for release attachment. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * build: cache Cargo registry and target in Flatpak CI Bind-mount host-side cargo-cache and cargo-target directories into the Flatpak build sandbox so CARGO_HOME and target/ persist across builds. Uses split restore/save with cleanup of incremental and registry/src (similar to Swatinem/rust-cache). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: scope cargo cache bind-mount sed to build-args only The previous sed matched --share=network in both finish-args and build-args, corrupting finish-args. Use a sed range to only target the build-args section. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * Apply suggestions from code review --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com> * chore: fix build * chore: use new error handling everywhere - not self reviewed * chore: use message banner to show progress * fix: start elapsed counter at 1s instead of 0s Aligns elapsed display with the countdown timer which already adds 1 to avoid showing "0s" immediately. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * chore: rabbit review * Update src/app.rs Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * chore: peer review * chore: fix build errors * fix(ui): make MessageBanner::set_global truly idempotent set_global() no longer resets timestamps, auto-dismiss timer, or logged flag when a banner with identical text already exists. This makes it safe to call every frame without log spam or timer restarts. Cherry-picked from origin/fix/spv-peer-timeout (08e3b3b). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(ui): address code review findings R01, R03, R06, R09, R10 - R01: Replace expect("No key selected") with graceful match + error banner in 11 token screens to prevent panics on missing signing key - R03: Remove dead backend_message field from AddExistingIdentityScreen - R06: Replace is_some() + unwrap() with idiomatic if-let-Some pattern in 10 token screens; use is_some_and() in structs.rs - R09: Add use imports for MessageBanner in 5 dashpay screens, replacing 22 fully-qualified crate::ui::components::MessageBanner:: calls - R10: Replace custom_dash_qt_error_message inline rendering with MessageBanner::set_global in network_chooser_screen Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(ui): address review findings SEC-08, SEC-09, RUST-015, SEC-05, SEC-07 - SEC-08: Restore safe if-let-Some pattern in WithdrawalScreen::refresh() to prevent double unwrap() panic on DB error or deleted identity - SEC-09: Restore original DB lookup in SendPaymentScreen::load_contact_info() replacing hardcoded "alice.dash" mock data - RUST-015: Revert unimplemented!() back to ui.label() in update_token_config MarketplaceTradeMode arm - SEC-05: Add success banners for contact request accept/reject in ContactRequests::display_task_result - SEC-07: Add MessageBanner::clear_all_global() and call it from AppState::change_network() to prevent stale banners leaking across network switches Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * docs: update coding conventions and message display guidance Add fallible constructor rule (Result<Self, ...> when they can fail), rename section to "General rules", and document MessageBanner idempotency (no guard needed for set_global). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(ui): replace expect/panic with graceful error handling (SEC-10) Replace all expect() calls in token screen constructors and confirmation handlers with MessageBanner error display. Constructors handle errors internally and return Self with degraded state, keeping create_screen() clean. refresh() methods now show errors via MessageBanner instead of tracing-only logging. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * refactor(ui): accept impl Display/Debug in MessageBanner API Change MessageBanner public methods to accept `impl Display` for message text and `impl Debug` for details, instead of `&str`. Remove needless `&format!(...)` borrows across 27 call sites. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(ui): remove error Failed to get best chain lock for mainnet, testnet, devnet, and local Fixes #633 * feat(ui): add automatic connection status banners Display persistent MessageBanner notifications based on network connection state transitions. Mode-aware messages guide users toward the right recovery action (RPC vs SPV). - Disconnected (RPC): "Disconnected — check that Dash Core is running" - Disconnected (SPV): "Disconnected — check your internet connection" - Syncing (RPC): "Syncing with Dash Core…" - Syncing (SPV): "SPV sync in progress…" - Synced: banner cleared Uses Option<OverallConnectionState> for change detection, with None as initial/post-network-switch sentinel to force first evaluation. Closes #667 (partial — action links deferred to follow-up) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(ui): pass TaskError directly to with_details() to avoid double-formatting The previous code used `format!("{err:?}")` which produced a String, then `with_details()` applied `{:#?}` again — wrapping the output in quotes and escaping inner characters. Passing `&err` directly lets Debug format once. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(ui): correct copy-paste error messages in token screens Replace "Burning" error messages that were copy-pasted from burn screen into freeze, destroy, and resume token screens with contextually correct messages. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(ui): restore lost success/error messages in 5 screens Replace display_message() calls with MessageBanner::set_global() in screens where display_message() is now a side-effect-only handler and no longer displays messages directly. Affected screens: create_asset_lock_screen, wallets_screen (MineBlocks), address_table (export error), profile_screen (validation), contact_details. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(ui): replace unwrap/expect with graceful error handling Replace double unwrap in transfer_screen refresh() with unwrap_or_else + MessageBanner error display, matching the pattern from withdraw_screen. SEC-002 tokens_screen skipped: the .expect() calls are only on compile-time embedded image data (include_bytes!) which is safe. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * refactor(ui): migrate masternode_list_diff_screen to global MessageBanner Replace ~15 local ui_state.message assignments and custom render_message_banner() with MessageBanner::set_global() via the display_message() trait method. Remove the message field from UiState and the unused Color32 import. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(ui): improve banner eviction logging and atomics - Upgrade BANNER_KEY_COUNTER from Relaxed to SeqCst ordering for future-proofing against multi-threaded usage - Log evicted banners at warn level in set_global() and replace_global() - Add comment explaining why show_global() always writes back Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * chore: remove resolved TODO.md All items tracked in the unified message display TODO have been addressed or moved to the review findings. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * docs(ui): add INTENTIONAL markers and API documentation - Document why with_details() accepts Debug (not Display): structured error context is more useful in diagnostic details panes - Document replace_global() fallback-to-add behavior as intentional - Add INTENTIONAL(SEC-003) marker for developer mode error details - Add INTENTIONAL(SEC-004) marker for BannerHandle Send+Sync safety Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * refactor(ui): extract shared token validation and fix ordering - Add validate_signing_key() helper in tokens/mod.rs to eliminate duplicated signing key validation across 12 token screens - Move signing key validation BEFORE WaitingForResult state transition so users see immediate errors instead of loading spinner then error - Replace is_err()/unwrap() anti-pattern with idiomatic let-else blocks in freeze, mint, transfer, destroy_frozen_funds, unfreeze screens Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * refactor(ui): return Result from get_selected_wallet Replace &mut Option<String> error out-parameter with idiomatic Result<Option<Arc<RwLock<Wallet>>>, String>. Update 26+ callsites across identity, token, DashPay, and contract screens. Callsite patterns: unwrap_or_else with MessageBanner for user-visible errors, unwrap_or(None) where errors were previously silently ignored. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(ui): resolve duplicate imports and clippy warnings Remove duplicate MessageBanner imports in create_asset_lock_screen and wallets_screen/mod. Fix needless_borrows_for_generic_args clippy lints in profile_screen, transfer_screen, and wallets_screen. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * style: reorder imports in masternode_list_diff_screen Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(ui): address review findings from MessageBanner migration audit Apply 13 triaged review fixes plus 1 bug fix across 22 files: - Remove dead error state fields (backend_message, error_message, Error variant) - Replace .expect() panics with graceful fallback + MessageBanner in token screens - Fix missing MessageBanner::show_global() on contracts documents screen - Migrate DocumentActionScreen inline errors to MessageBanner - Replace unwrap_or(None) with error-reporting fallback in DashPay screens - Fix replace_global idempotency and use relaxed atomic ordering in banner - Extract shared set_error_banner helper for 8 token screens - Restore correct Some(0) wallet index semantics - Document BannerHandle lifecycle in CLAUDE.md Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: validate token description length before sending to Platform (#530) * fix: validate token description length before sending to Platform Descriptions must be either empty or 3-100 characters long. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * fix(ui): validate token description by char count, not byte length String::len() counts UTF-8 bytes, causing multi-byte characters (CJK, emoji) to be miscounted against the 3–100 limit. Switch to chars().count() and update all UI labels to surface the minimum. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.5 <noreply@anthropic.com> Co-authored-by: Lukasz Klimek <842586+lklimek@users.noreply.github.com> * refactor(ui): consolidate banner extension traits into message_banner Move BannerHandleExt and ResultBannerExt from banner_ext.rs into message_banner.rs where they belong. Merge take_and_clear() into OptionBannerExt trait (impl for Option<BannerHandle>) alongside or_show_error() for Option. Remove the separate banner_ext module and Clearable helper trait for simplicity. Apply review findings: DRY patterns (take_and_clear, or_show_error, load_identities_with_banner), fix .expect() panics in constructors, restore known_identities in refresh(), narrow pub field visibility, add ScreenLike doc comments, and update CLAUDE.md conventions. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * doc(tmp): review guide for pr 604 * fix(ui): address review findings from grumpy-review iteration 1 - Replace .expect() panics in TransferTokensScreen and ClaimTokensScreen constructors with graceful degradation via Option<QualifiedIdentity> and MessageBanner error display (PROJ-001 HIGH) - Fix CLAUDE.md referencing non-existent BannerHandleExt trait name, corrected to OptionBannerExt (PROJ-002 MEDIUM) - Update set_global to preserve message_type when same text appears with different severity (RUST-001 MEDIUM) - Standardize display_message to handle both Error and Warning across all 11 token screens (RUST-002 MEDIUM) - Replace 21 manual take().clear() patterns with take_and_clear() across 6 files (RUST-003 MEDIUM) - Remove unused OptionBannerExt::or_show_error method (RUST-004 MEDIUM) - Migrate update_token_config from old error_message pattern to set_error_banner closure (RUST-005 MEDIUM) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * perf(ui): replace per-frame QualifiedIdentity clone with borrow Use .as_ref() instead of .clone() in the ui() identity guard of TransferTokensScreen and ClaimTokensScreen. QualifiedIdentity (Identity + KeyStorage + BTreeMap + Vec) was being cloned 60x/sec; now only borrowed for display, with clones deferred to button-click paths that actually need ownership. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * feat(ui): add OptionResultExt::or_show_error for Option<T> Mirrors ResultBannerExt::or_show_error but for Option<T>: if None, displays a global error banner with the given message. Enables concise patterns like: identities.first().cloned().or_show_error(ctx, "No identities loaded") Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(ui): address review findings from grumpy-review iteration 2 - Standardize display_message to handle both Error and Warning across 13 non-token screens that were missed in iteration 1 (PROJ-001 MEDIUM) - Replace .expect() panic in AddKeyScreen::refresh() with graceful or_show_error() + unwrap_or_default() (PROJ-002 MEDIUM) - Rename OptionResultExt to OptionBannerShowExt to avoid confusion with ResultBannerExt (RUST-001 MEDIUM) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(ui): handle Warning in add_new_identity_screen display_message Missed in the previous sweep — standardize display_message to handle both Error and Warning, matching all other screens. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(ui): standardize display_message side-effect patterns across screens Guard side effects with Error|Warning match, use take_and_clear(), and remove redundant MessageBanner::set_global() call in 4 screens. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * chore(deps): update dashpay/platform to rev 570e3af0 Adapt to breaking changes in rust-dashcore (a05d256f → 2824e52a): - Replace removed FeeLevel enum with FeeRate::normal() direct calls - Replace removed WalletManager::create_unsigned_payment_transaction() with TransactionBuilder + WalletManager::get_change_address() - Replace removed DashSpvClientInterface/DashSpvClientCommand with direct Arc<SpvClient> for quorum lookups via get_quorum_at_height() - Replace removed start()+monitor_network() with client.run(token) - Add .await to now-async subscribe_sync/network/progress methods - Replace removed SyncState::Initializing with SyncState::WaitForEvents Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * chore: log backtrace on panic * fix: panic in asset locks * fix(ui): address PR #604 review comments (CMT-001, CMT-002, CMT-003) - Fix QR scanner form reset matching wrong result type (CMT-001) - Remove dangerous identity fallback in token transfer screen (CMT-002) - Add fee-aware validation before credit transfers (CMT-003) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * refactor(spv): replace AsyncRwLock with ArcSwapOption for SPV client reference The SPV client reference only needs atomic set/clear (on start/stop) and wait-free reads (quorum lookups). ArcSwapOption is a better fit than AsyncRwLock<Option<Arc<...>>> — no lock contention, no async in blocking context, and consistent with how AppContext already uses ArcSwap for the SDK. Also fixes stale doc comment referencing removed DashSpvClientInterface. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(ui): address PR #604 review comments iteration 2 (transfer_tokens_screen) - Remove duplicate conflicting banner on missing identity (CMT-003) - Use generic banner messages with with_details() for errors (CMT-002) - Fix refresh to match specific token by contract+position (CMT-001) - Document error banner pattern in CLAUDE.md Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * [claudesquad] update from 'testing research' on 02 Mar 26 13:25 CET (paused) * test: add backend E2E test harness and SPV wallet test Add test-only accessors (db(), wallets()) on AppContext gated behind cfg(test/testing), fix compilation errors in the backend-e2e test (private field access, unused import), and apply nightly rustfmt. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * feat: add DASH_EVO_DATA_DIR env var to override app data directory Allows tests and CI to redirect all app data (database, SPV chain state, .env config) to a temp directory. The backend-e2e test harness now uses this to achieve full data isolation. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * test: add backend E2E test framework with shared state and funded wallets Evolve the prototype backend E2E test harness into a reusable framework: - LazyLock shared BackendTestContext with persistent workdir, SPV, and framework wallet (funded via E2E_WALLET_MNEMONIC or testnet faucet) - Task runner wrapper, polling wait helpers, faucet HTTP client - Identity key derivation helpers for wallet-funded registration - Six test scenarios: SPV wallet, identity create, identity withdraw, send/receive funds, fetch contracts, register DPNS name - Move default_identity_key_specs() from UI to backend_task::identity (domain logic, not UI concern) and make IdentityKeys fields pub - Add dashpay_contract_id() test accessor to AppContext Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(test): use tokio::sync::OnceCell instead of LazyLock for async init LazyLock triggers synchronously inside the #[tokio::test] runtime, causing "Cannot start a runtime from within a runtime" when the init function calls block_on(). Switch to tokio::sync::OnceCell with an async init() method so shared state initialization runs cooperatively within the existing tokio runtime. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(test): handle persistent DB and SPV balance sync in E2E harness - Wait for SPV to sync existing wallet balance before checking if faucet funding is needed (pre-funded wallets need time to discover on-chain UTXOs) - Handle "already imported" error gracefully when framework wallet exists in persistent DB from a previous run Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(test): wait for spendable balance and retry sends in E2E harness The SPV wallet reports total balance (including unconfirmed) but only confirmed/IS-locked UTXOs are available for transaction building. This caused "Insufficient funds" errors when tests tried to spend immediately after receiving funds. - Add wait_for_spendable_balance() that checks confirmed_balance_duffs() and triggers reconcile_spv_wallets() on each poll iteration - Add retry logic (5 attempts, 10s backoff) to create_funded_test_wallet() for sends that fail with InsufficientFunds - Wait for framework wallet change output to become spendable after each send so subsequent calls don't fail - Add wait_for_spendable_balance() before identity registration in all identity/DPNS tests - Add send_with_retry() helper in send_funds test - Add developer-facing README.md for the test framework Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(test): use tokio-shared-rt for shared runtime and sweep orphaned wallets Replace per-test tokio runtimes with tokio-shared-rt's global shared runtime to prevent SPV background tasks from dying between test functions. Add automatic orphaned wallet fund recovery during setup — wallets persist in DB, so on next run the harness sweeps funds back to the framework wallet. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: handle OverallConnectionState::Error in connection banner Semantic merge conflict from v1.0-dev: PR #650 added an Error variant to OverallConnectionState, which our connection banner match didn't cover. Show an error banner when SPV sync enters the error state. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * docs(test): improve backend-e2e documentation and funding UX - Add backend E2E section to CLAUDE.md pointing to the full README - Document .env file handling (project root vs workdir) and precedence - Fix test attribute in README: tokio::test → tokio_shared_rt::test(shared) - Update init sequence to reflect current code (spendable wait, orphan sweep) - Document automatic cleanup-on-init of orphaned test wallets - Raise minimum balance threshold from 1 to 10 tDASH - Always panic with receive address when faucet fails and balance is below minimum (previously only panicked on zero balance) - Show receive address in spendable-balance timeout warning Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: patch SPV UTXO spendability flags before coin selection Upstream key-wallet-manager (rust-dashcore) never sets is_confirmed or is_instantlocked on UTXOs, but CoinSelector requires one of them. This caused "No UTXOs available for selection" errors despite having balance. Workaround infers status from block inclusion (height > 0 → confirmed, height == 0 → IS-locked). Ref: dashpay/rust-dashcore#514 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * test: harden backend E2E tests with retry logic and calibrated amounts - Increase funding amounts to avoid insufficient-funds flakes - Add 3-attempt retry for identity registration (chain height sync) - Retry on "No UTXOs" in send_with_retry alongside "Insufficient" - Wait for spendable balance in create_funded_test_wallet before return - Add CI workflow for backend E2E tests Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * Update test command in backend E2E workflow * ci: merge backend-e2e workflow into tests workflow (#727) * Initial plan * ci: merge backend-e2e workflow into tests workflow as an additional step Co-authored-by: lklimek <842586+lklimek@users.noreply.github.com> * test: add cleanup_only noop test to sweep orphaned wallets Add a standalone test that triggers BackendTestContext initialization, which runs cleanup_test_wallets() as its final step. This can be run as a dedicated CI step after the E2E suite to sweep orphaned wallet funds back to the framework wallet. Run with: cargo test --test backend-e2e --all-features -- --ignored --nocapture cleanup_only Co-authored-by: lklimek <lklimek@users.noreply.github.com> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * Add cleanup step for E2E test wallets Added a cleanup step for E2E test wallets in the workflow. * Simplify E2E test workflow conditions Removed conditional checks for E2E_WALLET_MNEMONIC in test steps. --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: lklimek <842586+lklimek@users.noreply.github.com> Co-authored-by: claude[bot] <41898282+claude[bot]@users.noreply.github.com> Co-authored-by: lklimek <lklimek@users.noreply.github.com> Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com> * fix: address PR #673 review comments from triage Production code: - Extract patch_utxo_spendability_flags() helper to deduplicate workaround in estimate_fallback_amount and build_unsigned_payment_tx - Add IdentityKeys::new() constructor - Make wallet+address persistence atomic via store_wallet_with_addresses() - Add pending_wallet_selection after wallet creation Test code: - Add DPNS registration retry for identity propagation delay - Use u64 hex for DPNS name uniqueness (CMT-023) - Calibrate test funding amounts per reviewer feedback - Add fragility note on string-match wallet detection (CMT-006) - Add TODO for identity fund withdrawal in cleanup (CMT-032) - Add INTENTIONAL comment for bounded channel design (CMT-017) - Add balance assertion for return leg in send_funds test - Log reconcile_spv_wallets errors in wait helpers - Use is_ok_and for clarity in spv_wallet test Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * refactor: remove UTXO spendability workaround, update platform dep Remove `patch_utxo_spendability_flags()` that faked IS-locked status on mempool UTXOs. Wait for upstream fix (dashpay/rust-dashcore#514) to properly set is_confirmed/is_instantlocked flags on UTXOs. Also: - Update dashpay/platform rev to aa86b74f7e2 - Adapt to upstream API: FeeLevel→FeeRate, remove NetworkExt import - Fix retry to catch "No UTXOs" errors in test harness Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * refactor: deduplicate default_identity_key_specs Move the single canonical copy to backend_task::identity::mod and have the UI screen import it, eliminating ~240 lines of duplicated function and tests. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * ci: temporarily disable backend E2E tests in CI The backend E2E tests need updates after the TaskError migration (#739) changed AppContext field visibility. Commenting out the CI steps until the tests are adapted. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * refactor: type-safe wallet creation and identity key visibility - Restrict IdentityKeys fields to pub(crate) to prevent private key exposure outside the crate - Change register_wallet() to return TaskError instead of String, using proper rusqlite error matching via is_unique_constraint_violation() and a new WalletAlreadyImported variant - Change Wallet::new_from_seed() to accept Option<&Secret> for password instead of Option<&str>, keeping sensitive data in the Secret wrapper - Change Wallet::new_from_seed() to return TaskError instead of String, with a new WalletKeyDerivationFailed variant for derivation errors - Move build_identity_registration() and get_receive_address() from test helpers to production code in src/backend_task/identity/mod.rs - Extract is_unique_constraint_violation() to src/database/mod.rs as a shared pub(crate) utility, removing the duplicate in import_mnemonic_screen - Update all callers in add_new_wallet_screen and import_mnemonic_screen Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * test: address PR #673 review comments for E2E test framework - Move framework modules (harness, task_runner, wait, funding, cleanup, identity_helpers) into tests/backend-e2e/framework/ subdirectory - Make E2E_WALLET_MNEMONIC required (panic with instructions if unset) - Remove auto-faucet from initialization flow (keep as helper) - Remove retry loops in identity_create and identity_withdraw tests - Remove unnecessary wait_for_spendable_balance calls (already done by create_funded_test_wallet) - Replace all println!/eprintln! with tracing macros - Initialize tracing subscriber in harness init - Add "No spendable funds" and "spendable" to send retry conditions - Remove stale "other agent" NOTE comments from identity_helpers - Consolidate funding logic (harness delegates to funding module) - Update README for required mnemonic and new directory structure Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: reconcile production and test code after type-safe refactor Adapt test framework to production API changes: - Use IdentityKeys::new() constructor (fields now pub(crate)) - Match TaskError::WalletAlreadyImported variant instead of string - Allow dead_code on faucet helpers (kept but not auto-called) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * refactor: thread data_dir as explicit parameter to eliminate env var dependency Add `data_dir: PathBuf` field to `AppContext` and thread it through `Config::load_from()`, `Config::save()`, `SpvManager::new()`, `start_dash_qt()`, and `create_dash_core_config_if_not_exists()`. This enables E2E tests to specify their data directory without mutating process-wide environment variables, making parallel test execution safe. The `DASH_EVO_DATA_DIR` env var is still checked in production via `app_user_data_dir_path()`, but the resolved path is threaded through as a value rather than re-read from env on every call. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(test): simplify funded wallet creation and make spendable balance check reliable - Remove retry loop from create_funded_test_wallet; wait for full amount_duffs instead of 1 duff in spendable balance check - Add Wallet::spv_confirmed_balance() that returns None when SPV hasn't synced yet (no max_balance fallback) - Use spv_confirmed_balance() in wait_for_spendable_balance so the wait never gets false positives from the fallback - Remove --test-threads=1 requirement from README (unsafe set_var was the only reason; data_dir is now threaded explicitly) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: remove send retry loop, handle PRIMARY KEY constraints, isolate SPV test dirs - Remove send_with_retry() from send_funds.rs; use wait_for_spendable_balance before each send instead - Add SQLITE_CONSTRAINT_PRIMARYKEY (1555) to uniqueness check alongside SQLITE_CONSTRAINT_UNIQUE (2067) - Use tempfile::TempDir in SPV tests instead of fixed /tmp/spv-test path to prevent state leaks and support concurrent test runs Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com> Co-authored-by: Pasta Lil Claw <pasta+claw@dashboost.org> Co-authored-by: PastaClaw <thepastaclaw@users.noreply.github.com> Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Co-authored-by: Paul DeLucia <69597248+pauldelucia@users.noreply.github.com> Co-authored-by: Copilot <198982749+Copilot@users.noreply.github.com> Co-authored-by: claude[bot] <41898282+claude[bot]@users.noreply.github.com> Co-authored-by: lklimek <lklimek@users.noreply.github.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
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
Unified message display system using global
MessageBannercomponent, replacing per-screen inline error handling across the entire codebase.BannerHandlelifecycle, auto-dismiss, elapsed time, collapsible technical detailsResultBannerExt,OptionBannerShowExt,OptionBannerExt(withreplace/replace_with_elapsed)island_central_panel(); screens only overridedisplay_message()for side-effectsTaskErrortyped envelope,get_selected_walletreturnsResult, graceful degradation instead ofunwrap/expectwallet_open_attemptedflag on ~30 screens prevents per-frametry_open_wallet_no_passwordcallsrefresh_bannerwith elapsed time instead ofBroadcastStatus::Broadcasting(u64)Review guide
See
docs/ai-design/2026-02-27-banner-review-fixes/pr-604-review-guide.md— separates architectural, behavioral, and mechanical changes.Test plan
cargo clippy --all-features --all-targets -- -D warnings— cleancargo +nightly fmt --all -- --check— cleancargo test --all-features --workspace— 42 passed, 0 faileddocs/ai-design/2026-02-27-banner-review-fixes/manual-test-scenarios.md🤖 Generated with Claude Code
🤖 Co-authored by Claudius the Magnificent AI Agent
Summary by CodeRabbit
New Features
Improvements
Documentation