-
Notifications
You must be signed in to change notification settings - Fork 37
Fix @mention search performance and refactor search functionality #614
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
@alanpoon & @tyreseluo, can you please give this a review before I dig in? Thanks. |
yep, i will. |
…tartup so CPU-bound work like member searches runs off the UI thread. - Reworked mention search to enqueue jobs on the CPU worker with cancellation tokens, unique search IDs, and better handling of loading state when the member list is still fetching. - Updated room loading and sync flow to fetch the full member list from the homeserver and deliver it through a new RoomMembersListFetched timeline update, removing the old MatrixRequest::SearchRoomMembers path. - Refactored the member search algorithm for batching, cancellation, richer match strategies, and added unit tests for the new helpers.
…tion bug after re-login
…ix into fixed-issue-452
|
Review suggestions: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Overall the performance of loading the mentionable list seems to be impacted.
- There's seem too many fields added MentionableTextInput.
- There's also bug when clicking the user from mentionable list, it does not apply into the text box.
- The search does not work after "comma".
5. There should not be "Notify the Room" for direct message.
| match timeline.room().members(RoomMemberships::JOIN).await { | ||
| Ok(members) => { | ||
| let count = members.len(); | ||
| log!("Fetched {count} members for room {room_id} after sync."); | ||
| if let Err(err) = sender.send(TimelineUpdate::RoomMembersListFetched { members }) { | ||
| warning!("Failed to send RoomMembersListFetched update for room {room_id}: {err:?}"); | ||
| } else { | ||
| SignalToUI::set_ui_signal(); | ||
| } | ||
| } | ||
| Err(err) => { | ||
| warning!("Failed to fetch room members from server for room {room_id}: {err:?}"); | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| // Note: This can be sent from either GetRoomMembers (local cache lookup) | ||
| // or SyncRoomMemberList (full server sync). We only clear the sync pending | ||
| // flag when we receive RoomMembersSynced (which is only sent after full sync). | ||
| let sort_data = precompute_member_sort(&members); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it make sense to move this processing operation to async function in sliding_sync rather than in UI thread?
| } | ||
|
|
||
| /// Search room members in background thread with streaming support (backward compatible) | ||
| pub fn search_room_members_streaming( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function is not used.
| pub sender: Sender<SearchResult>, | ||
| pub search_id: u64, | ||
| pub precomputed_sort: Option<Arc<PrecomputedMemberSort>>, | ||
| pub cancel_token: Option<Arc<AtomicBool>>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The PR is significantly slow enough when loading the mentionable list to test the cancel token when there is a new search input. The cancel token does not seem to work.
- Go to room with large number of members. (mentionable list is loading)
- While it is loading, type "k". I am expecting the mentionable list to load faster for shorter query.
- After it takes several seconds to load "k" result, remove "k" to go to empty string queries.
- I am expecting mentionable list to display "loading". But it shows the full results.
| cx.spawn_thread(move || match job { | ||
| CpuJob::SearchRoomMembers(params) => run_member_search(params), | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a way to close this CpuJob when closing the room screen tab?
| sender: &Sender<SearchResult>, | ||
| cancel_token: &Option<Arc<AtomicBool>>, | ||
| search_id: u64, | ||
| search_text: &Arc<String>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need for Arc
| if self.is_searching() { | ||
| if let Event::KeyUp(key_event) = event { | ||
| if key_event.key_code == KeyCode::Escape { | ||
| self.cancel_active_search(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After pressing KeyCode::Escaope, it is not cancelled. is_cancelled does not return true,
| // ESC key is now handled in the main event handler using KeyUp event | ||
| // This avoids conflicts with escaped() method being consumed by other components | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can be removed
| // Force a fresh search now that members are available | ||
| let search_text = self.cmd_text_input.search_text(); | ||
| self.last_search_text = None; | ||
| self.update_user_list(cx, &search_text, scope); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a cyclic dependencies. MentionableTextInputAction::RoomMembersLoaded -> "update_user_list" -> submit_async_request(MatrixRequest::GetRoomMembers) -> TimelineUpdate::RoomMembersListFetched -> MentionableTextInputAction::RoomMembersLoaded
| if is_complete { | ||
| self.search_results_pending = false; | ||
| // Search is complete - get results for final UI update | ||
| let final_results = if let MentionSearchState::Searching { | ||
| accumulated_results, | ||
| .. | ||
| } = &self.search_state | ||
| { | ||
| accumulated_results.clone() | ||
| } else { | ||
| Vec::new() | ||
| }; | ||
|
|
||
| if final_results.is_empty() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.


Fixed issue #506
This is a remastered version of the PR #550 .
cpu_workerthread that stays alive for the app lifetime and runsCPU-boundjobs (currently the room-member search);App::handle_startupinitializes it and widgets enqueue work viacpu_worker::spawn_cpu_job.MentionableTextInputinto an explicit state machine with search IDs, cancellation tokens, loading/empty UI states, and streaming updates that arrive over the background worker channel.MatrixRequest::SearchRoomMemberspath:sliding_syncnow fetches joined-member lists directly, emits RoomMembersListFetched, and RoomScreen caches those members plus precomputed sort data for downstream widgets.There are three related issues that still need to be fixed.