fix(ui): prevent stacking banners during batch wallet identity search#735
Conversation
Add BackendTaskSuccessResult::Progress variant for progress updates
during long-running operations. Unlike Message, Progress does NOT
create global banners via MessageBanner::set_global — it only updates
the originating screen's existing banner handle in-place.
Root cause: load_user_identities_up_to_index() sent progress as
Message('Searching index X of Y...'), and app.rs's Message handler
calls MessageBanner::set_global() for every Message. Since each
progress text is unique, set_global() created a new banner each time,
causing the stacking visible in DET#713.
Fix: Change batch search progress to use the new Progress variant,
which bypasses global banner creation and only flows through
display_task_result() to update the screen's existing banner handle.
Closes dashpay#713
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds a structured Changes
Sequence DiagramsequenceDiagram
participant Backend as Backend Task
participant AppLoop as App Loop
participant UIScreen as Visible Screen
participant Banner as Refresh Banner
Backend->>AppLoop: emit BackendTaskSuccessResult::Progress{message,current,total}
AppLoop->>UIScreen: visible_screen_mut().display_task_result(Progress)
UIScreen->>Banner: update existing banner (set_message) or create banner (replace_with_elapsed)
Banner->>UIScreen: banner rendered with new progress message
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
There was a problem hiding this comment.
Pull request overview
Prevents UI banner stacking during batch wallet identity search by introducing a non-global “progress update” task result that updates the current screen’s banner in place.
Changes:
- Added
BackendTaskSuccessResult::Progress(String)for non-global progress updates. - Routed
Progressresults to screens without creating global banners. - Switched wallet identity search iteration updates from
MessagetoProgress.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| src/backend_task/mod.rs | Adds Progress(String) success variant and documents intended behavior. |
| src/app.rs | Handles Progress by forwarding to the visible screen without creating a global banner. |
| src/ui/identities/add_existing_identity_screen.rs | Updates existing banner handle in-place when receiving Progress. |
| src/backend_task/identity/load_identity_from_wallet.rs | Emits Progress during batch search iterations to avoid banner stacking. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /// Unlike `Message`, these do NOT create global banners — they only update | ||
| /// the originating screen's existing banner handle via `display_task_result`. |
There was a problem hiding this comment.
The latest commit updated the doc comment to say "By convention, the app routes Progress results to the visible screen's display_task_result without creating a global banner" — which reflects the actual enforcement mechanism.
| /// Progress updates during long-running operations (e.g. batch identity search). | ||
| /// Unlike `Message`, these do NOT create global banners — they only update | ||
| /// the originating screen's existing banner handle via `display_task_result`. | ||
| Progress(String), |
There was a problem hiding this comment.
claw please use structured progress type, with current / total. In future, we might get a dedicated progress component , it will be useful.
There was a problem hiding this comment.
Done — switched to a struct variant with current: u32 and total: u32 fields alongside the message: String. The call site passes 1-based values so a future progress bar component can just do current / total without offset math.
Progress {
/// Human-readable progress message
message: String,
/// Current step (1-based)
current: u32,
/// Total steps
total: u32,
}![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/backend_task/identity/load_identity_from_wallet.rs (1)
284-287: Make the progress banner text a complete, localizable sentence.
"Searching index {} of {}..."reads like an implementation detail and relies on positional placeholders. A full sentence with named placeholders will translate more cleanly and is easier to understand in the banner.✏️ Suggested tweak
- BackendTaskSuccessResult::Progress(format!( - "Searching index {} of {}...", - identity_index, max_identity_index - )), + BackendTaskSuccessResult::Progress(format!( + "Searching wallet identity index {current} of {last}.", + current = identity_index, + last = max_identity_index, + )),As per coding guidelines, "All user-facing strings (labels, messages, tooltips, errors) must be simple, complete sentences. Avoid concatenating fragments, positional assumptions, or grammar that breaks in other languages. Each string should be extractable as a single translation unit with named placeholders for dynamic values."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/backend_task/identity/load_identity_from_wallet.rs` around lines 284 - 287, The progress banner currently uses a fragment with positional placeholders in the BackendTaskSuccessResult::Progress format! call; change it to a complete, localizable sentence and use named placeholders instead (e.g., "Searching identity {current} of {total}."), replacing the format! invocation that references identity_index and max_identity_index so translators get a single extractable string and the placeholders are explicit (refer to the BackendTaskSuccessResult::Progress call around the format! that builds the banner).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/app.rs`:
- Around line 1115-1119: BackendTaskSuccessResult::Progress is being routed via
visible_screen_mut() (call sites: BackendTaskSuccessResult::Progress branch,
visible_screen_mut(), display_task_result()), which means progress updates
follow whatever screen is currently visible instead of the screen that started
the task (e.g., AddExistingIdentityScreen) and so refresh_banner stops updating
when the user navigates away. Fix this by adding task-to-screen affinity: when
starting the task, capture an identifier or direct banner handle from the
originating screen (e.g., store originating_screen_id or banner_handle on the
task context created by AddExistingIdentityScreen), then in the
BackendTaskSuccessResult::Progress handling path route the unboxed_message to
that stored identifier/handle instead of calling visible_screen_mut() so updates
go to the original screen's banner (or, if the original screen no longer exists,
gracefully drop the update). Ensure references use the existing symbols:
BackendTaskSuccessResult::Progress, display_task_result(), refresh_banner,
AddExistingIdentityScreen, visible_screen_mut() to locate and update the logic.
---
Nitpick comments:
In `@src/backend_task/identity/load_identity_from_wallet.rs`:
- Around line 284-287: The progress banner currently uses a fragment with
positional placeholders in the BackendTaskSuccessResult::Progress format! call;
change it to a complete, localizable sentence and use named placeholders instead
(e.g., "Searching identity {current} of {total}."), replacing the format!
invocation that references identity_index and max_identity_index so translators
get a single extractable string and the placeholders are explicit (refer to the
BackendTaskSuccessResult::Progress call around the format! that builds the
banner).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1a3d7339-7269-4d5f-9832-d66389cb0819
📒 Files selected for processing (4)
src/app.rssrc/backend_task/identity/load_identity_from_wallet.rssrc/backend_task/mod.rssrc/ui/identities/add_existing_identity_screen.rs
- Reword Progress doc comment to clarify behavior is a routing convention enforced by app.rs, not inherent to the enum variant (Copilot review) - Add fallback banner creation in Progress handler when refresh_banner is None, ensuring progress is always visible (Copilot review) - Use named placeholders in progress format string for localizability (CodeRabbit nitpick) - Add TODO about visible_screen_mut() task-to-screen affinity limitation (CodeRabbit major: architectural change deferred)
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Pull request overview
Prevents global banner stacking during batch wallet identity search by introducing a dedicated progress result type intended to update an existing in-screen banner in-place.
Changes:
- Added
BackendTaskSuccessResult::Progress(String)for non-global progress updates. - Routed
Progressresults directly to the current screen without creating global banners. - Updated wallet identity search to emit
Progressupdates during iteration.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/backend_task/mod.rs | Adds Progress(String) success result variant + docs describing routing conventions. |
| src/app.rs | Routes Progress results to the visible screen without invoking global banner creation. |
| src/ui/identities/add_existing_identity_screen.rs | Handles Progress by updating the existing banner handle; adds fallback banner creation when missing. |
| src/backend_task/identity/load_identity_from_wallet.rs | Emits Progress updates during batch identity search iterations. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| current = identity_index, | ||
| last = max_identity_index, |
There was a problem hiding this comment.
The progress message labels {last} but passes max_identity_index (a max index, not a count), which reads oddly when indices are 0-based (e.g., “index 0 of 10”). Consider changing the format parameters to be unambiguous (e.g., max_index = max_identity_index) or switching to 1-based “current/total” reporting (current = identity_index + 1, total = max_identity_index + 1) for clearer user-facing progress.
| current = identity_index, | |
| last = max_identity_index, | |
| current = identity_index + 1, | |
| last = max_identity_index + 1, |
There was a problem hiding this comment.
Good point — switched to 1-based indexing. Now shows "Searching wallet identity index 1 of 10" instead of "0 of 9".
…otal Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
thepastaclaw
force-pushed
the
fix/stacking-progress-banners
branch
from
9ae22ad to
994559b
Compare
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Clean, well-scoped fix. The Progress variant is properly routed in app.rs, the screen handler updates banners in-place, and the structured fields enable future progress bar rendering. No blocking issues — just a theoretical overflow edge case and a stale comment from the pre-Progress era.
Reviewed commit: 994559b
🟡 2 suggestions · ⚪ 1 nitpick
Review agents: claude-general, codex-general, claude-rust-quality, codex-rust-quality
Specialist: rust-quality (selected by LLM based on PR content)| total = max_identity_index + 1, | ||
| ), | ||
| current: identity_index + 1, | ||
| total: max_identity_index + 1, |
There was a problem hiding this comment.
suggestion (convergent: codex-general + codex-rust-quality, confidence: 0.80)
Progress counters can overflow at u32::MAX boundary
IdentityIndex is u32 and comes from user text input parsed at add_existing_identity_screen.rs:700. The expressions identity_index + 1 and max_identity_index + 1 overflow if the user enters 4294967295 — panic in debug, wrapping in release. In practice this is nearly impossible (default is 5), but saturating arithmetic is a trivial fix for correctness.
| total: max_identity_index + 1, | |
| message: format!( | |
| "Searching wallet identity index {current} of {total}.", | |
| current = identity_index.saturating_add(1), | |
| total = max_identity_index.saturating_add(1), | |
| ), | |
| current: identity_index.saturating_add(1), | |
| total: max_identity_index.saturating_add(1), |
| } | ||
| _ => {} | ||
| } | ||
| } |
There was a problem hiding this comment.
nitpick (convergent: codex-general + codex-rust-quality, confidence: 0.70)
No regression test covers the non-stacking progress banner path
The core fix — repeated Progress updates updating a banner in-place instead of stacking — has no automated test. This is understandable given the egui test harness limitations, but worth noting for future kittest coverage.
|
suggestion (convergent: claude-general + claude-rust-quality, confidence: 0.85) Stale comment and effectively dead The Consider removing the branch or updating the comment to reflect its actual role as a fallback for unexpected |
Fixes #713
Problem
When searching for wallet identities, each progress update (
Searching index X of Y...) was sent as aBackendTaskSuccessResult::Message, which creates a new global banner viaMessageBanner::set_global(). Since each message has different text, banners stacked up instead of replacing each other.Fix
Added a
BackendTaskSuccessResult::Progress(String)variant that:MessageBanner::set_global())display_task_result()Changed the batch identity search to use
Progressinstead ofMessagefor iteration updates.Changes
src/backend_task/mod.rs: AddedProgress(String)variant toBackendTaskSuccessResultsrc/app.rs: RouteProgressto screen without global bannersrc/ui/identities/add_existing_identity_screen.rs: HandleProgressby updating existing banner handle in-placesrc/backend_task/identity/load_identity_from_wallet.rs: UseProgressinstead ofMessagefor search iteration updates4 files, +18/-2 lines.
Summary by CodeRabbit
New Features
Bug Fixes