fix(ui): prevent 'No users found' flash during profile search#736
fix(ui): prevent 'No users found' flash during profile search#736thepastaclaw wants to merge 4 commits into
Conversation
…pay#713) Add BackendTaskSuccessResult::Progress variant for progress messages that should update the existing banner instead of creating new global banners. Change load_user_identities_up_to_index to send Progress instead of Message for "Searching index X of Y..." updates. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
When searching profiles, 'No users found' would briefly flash before results loaded. This happened because display_message() set loading=false (for error banner side-effects) before display_task_result() delivered actual results, creating a frame where has_searched=true && !loading && results.is_empty(). Fix: Add search_completed flag that's only set in display_task_result(), ensuring the empty-results message only appears after the backend has actually responded. Fixes dashpay#684
📝 WalkthroughWalkthroughAdds a Changes
Sequence Diagram(s)sequenceDiagram
rect rgba(200,220,255,0.5)
participant User
participant UI as "UI Screen\n(ProfileSearch / AddExisting)"
end
rect rgba(200,255,200,0.5)
participant App
end
rect rgba(255,220,200,0.5)
participant Backend as "Backend Task\n(load identities / search)"
end
User->>UI: initiate search / start action
UI->>App: dispatch task request
App->>Backend: start backend task
Backend-->>App: BackendTaskSuccessResult::Progress(msg)
App-->>UI: display_task_result(progress msg)
UI->>User: update progress banner / indicator
Backend-->>App: BackendTaskSuccessResult::Message|Result(end)
App-->>UI: display_task_result(final result)
UI->>User: show results or "No users found" (after search_completed)
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 |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/ui/dashpay/profile_search.rs (1)
324-326: Consider guardingsearch_completedassignment.Setting
search_completed = trueunconditionally at the start ofdisplay_task_resultmeans any backend result (including unrelated ones caught by the wildcard arm) would mark the search as completed. While this works correctly now since profile search only receivesDashPayProfileSearchResults, a defensive approach would be to set it only for the expected result type.🔧 Suggested refinement
fn display_task_result(&mut self, result: BackendTaskSuccessResult) { self.loading = false; - self.search_completed = true; match result { BackendTaskSuccessResult::DashPayProfileSearchResults(results) => { + self.search_completed = true; self.search_results.clear(); // ... rest of the handler } BackendTaskSuccessResult::Message(_msg) => { // Message display is handled globally by AppState } _ => { // Ignore other results } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/dashpay/profile_search.rs` around lines 324 - 326, The method display_task_result currently sets self.search_completed = true unconditionally; change it so search_completed is only set to true when the backend result is the expected DashPayProfileSearchResults variant (i.e., inside the match arm that handles BackendTaskSuccessResult::DashPayProfileSearchResults). Keep self.loading = false as appropriate, but move the search_completed assignment into the specific match arm for DashPayProfileSearchResults (and do not set it in the wildcard/unrelated arms) to avoid marking searches completed for unrelated backend results.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/ui/dashpay/profile_search.rs`:
- Around line 324-326: The method display_task_result currently sets
self.search_completed = true unconditionally; change it so search_completed is
only set to true when the backend result is the expected
DashPayProfileSearchResults variant (i.e., inside the match arm that handles
BackendTaskSuccessResult::DashPayProfileSearchResults). Keep self.loading =
false as appropriate, but move the search_completed assignment into the specific
match arm for DashPayProfileSearchResults (and do not set it in the
wildcard/unrelated arms) to avoid marking searches completed for unrelated
backend results.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: bdea49ce-8008-4d75-8216-ca8921668df8
📒 Files selected for processing (5)
src/app.rssrc/backend_task/identity/load_identity_from_wallet.rssrc/backend_task/mod.rssrc/ui/dashpay/profile_search.rssrc/ui/identities/add_existing_identity_screen.rs
…s arm Move self.loading = false and self.search_completed = true inside the DashPayProfileSearchResults match arm instead of setting them unconditionally at the top of display_task_result. Without this fix, any unrelated BackendTaskSuccessResult (e.g. Progress messages from a concurrent identity-load task) routed to the visible screen could prematurely clear the loading spinner and set search_completed = true, causing the 'No users found' flash that this PR set out to fix. Addresses CodeRabbit review comment.
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/ui/dashpay/profile_search.rs (1)
256-259:⚠️ Potential issue | 🟠 MajorDo not clear loading from generic
display_message()callbacks.Line 258 still sets
self.loading = falsefor any global message callback, which can hide the spinner beforeDashPayProfileSearchResultsarrives (e.g., unrelated concurrent banner events).Suggested fix
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; + // Keep loading state tied to the profile-search task lifecycle in `display_task_result()`. }If failure-path cleanup is needed, handle that in
display_task_error()for this screen/task instead of in generic banner callbacks.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/dashpay/profile_search.rs` around lines 256 - 259, The generic callback display_message currently clears self.loading unconditionally; remove the assignment to self.loading = false from the display_message method so banner events won't hide the spinner for unrelated messages, and move any failure-path cleanup to the screen-specific error handler display_task_error (operate on self.loading there) or wherever DashPayProfileSearchResults handles task completion/failure so loading is only cleared for this task's terminal states.
🤖 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/ui/dashpay/profile_search.rs`:
- Around line 256-259: The generic callback display_message currently clears
self.loading unconditionally; remove the assignment to self.loading = false from
the display_message method so banner events won't hide the spinner for unrelated
messages, and move any failure-path cleanup to the screen-specific error handler
display_task_error (operate on self.loading there) or wherever
DashPayProfileSearchResults handles task completion/failure so loading is only
cleared for this task's terminal states.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 75f23417-b539-4216-af9d-14bf69447bf6
📒 Files selected for processing (1)
src/ui/dashpay/profile_search.rs
Address CodeRabbit review: remove self.loading = false from the generic display_message() callback, which fires for ANY completed backend task and could hide the spinner before search results arrive. Loading is now only cleared via: - display_task_result: inside the DashPayProfileSearchResults arm - display_task_error (new override): on task failure
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/ui/dashpay/profile_search.rs (1)
257-264: Align backend-task banner lifecycle with screen guidelines.This no-op
display_message()path leaves out the expectedrefresh_bannerhandle lifecycle (set_globalon dispatch,take_and_clear()on completion/error).As per coding guidelines "Screens with backend tasks should store a
refresh_banner: Option<BannerHandle>field; on task dispatch set it viaMessageBanner::set_global()with info/progress, then indisplay_message()dismiss it viatake_and_clear()(not just setting toNone)."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/dashpay/profile_search.rs` around lines 257 - 264, The screen currently leaves display_message() as a no-op and therefore skips properly dismissing the banner lifecycle; add a refresh_banner: Option<BannerHandle> field on the profile-search screen (if not present), ensure the task dispatch path uses MessageBanner::set_global(...) to set refresh_banner on dispatch, and modify display_message(&mut self, _message: &str, _message_type: MessageType) to call self.refresh_banner.take_and_clear() (or call take_and_clear() on the stored BannerHandle) to dismiss the banner on completion/error rather than just setting the field to None so the spinner/banner lifecycle aligns with the guidelines.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/ui/dashpay/profile_search.rs`:
- Around line 257-264: The screen currently leaves display_message() as a no-op
and therefore skips properly dismissing the banner lifecycle; add a
refresh_banner: Option<BannerHandle> field on the profile-search screen (if not
present), ensure the task dispatch path uses MessageBanner::set_global(...) to
set refresh_banner on dispatch, and modify display_message(&mut self, _message:
&str, _message_type: MessageType) to call self.refresh_banner.take_and_clear()
(or call take_and_clear() on the stored BannerHandle) to dismiss the banner on
completion/error rather than just setting the field to None so the
spinner/banner lifecycle aligns with the guidelines.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1df019e0-d991-4f14-928f-10226bcb3fce
📒 Files selected for processing (1)
src/ui/dashpay/profile_search.rs
|
Closing as duplicate of #748, which has the more complete fix. Apologies for the noise — this was caused by a heartbeat loop that didn't check for existing PRs before re-investigating the same issue. |
1 participant
Issue
Fixes #684 — Search Public Profiles always shows "No users found" briefly before results load.
Root Cause
When a search starts,
has_searchedis set totrueand results are cleared. Thedisplay_message()method (called for banner side-effects) setsloading = falsebeforedisplay_task_result()delivers actual results. This creates a frame wherehas_searched && !loading && results.is_empty()is true, causing the "No users found" message to flash.Fix
Added a
search_completedflag that's only set indisplay_task_result(), and changed the empty-results condition to checksearch_completedinstead ofhas_searched. This ensures the "No users found" message only appears after the backend has actually returned (empty) results.Testing
Summary by CodeRabbit
New Features
Bug Fixes