Fix @mention search performance and refactor search functionality #550

ZhangHanDong · 2025-07-11T16:06:25Z

Fixed issue #452.

The @mention search feature was iterating over the entire members list on the main UI thread, causing significant performance issues and UI freezes, especially in rooms with many members.

Changes

Moved search to background thread

Implemented async search using tokio::task::spawn_blocking
Added streaming/partial results support for better responsiveness
First match is sent immediately for instant feedback

Refactored search logic into separate module

Created new src/room/member_search.rs module
Moved search functions from sliding_sync.rs for better code organization
Improved separation of concerns between protocol handling and business logic

Performance Improvements

Search no longer blocks the UI thread
Results stream in batches for smoother experience
Loading animations remain responsive during search

…responsiveness

kevinaboos

Thanks Alex, the code looks quite good in general!

I have only one major suggestion, which addresses the problem that there is no way to cancel a member search that's ongoing in the background.

Instead of using Makepad actions to send the results from the background task to the UI thread (which requires a search_id), we should just use a direct channel to communicate between them. This design has a few benefits:

No need for a search_id: just create the channel in the MentionableTextInput code and include the sender side of the channel in the MatrixRequest:: SearchRoomMembers request.
- The MentionableTextInput will hold on to the receiver side of the channel and check it for updates. You could certainly use a dedicated action to tell the MentionableTextInput widget to check its channel for updates; that would be better than using Event::Signal like we typically do.
This allows a SearchRoomMembers request to be aborted/canceled very easily. We only have to drop the receiver side in the MentionableTextInput, which will then cause any sender.send() operation in the background thread to fail.
- When send() fails, we can stop searching and return from the search_room_members_streaming function, as that would indicate that the UI side is no longer listening for updates.
Reduces the quantity and the size of actions that are sent through Makepad's event system, which itself reduces the amount of memory allocations needed to store those larger action types.

src/sliding_sync.rs

…ng/robrix into optimize-member-search

ZhangHanDong · 2025-08-01T05:34:19Z

Further optimizations have been made:

Used channel communication instead of makepad actions.
Selected the appropriate data structure, BinaryHeap, to prioritize the best matching results for user mentions.
Utilized indices to avoid copying RoomMember objects in the search results.

alanpoon

ZhangHanDong#1

I have created a PR request after some amendments.

alanpoon · 2025-08-01T10:11:21Z

src/shared/mentionable_text_input.rs

+            submit_async_request(MatrixRequest::SearchRoomMembers {
+                room_id: room_props.room_id.clone(),
+                search_text: search_text.to_string(),
+                sender,
+                max_results: max_visible_items * SEARCH_BUFFER_MULTIPLIER,
+                cached_members: cached_members.clone(),
+            });


You can use cx.spawn, instead of using submit_async_request. Also there are two such calls duplicated in check_search_channel, user_update_list.

That’s strange—it seems like you’re looking at the old code.

Why use cx.spawn? Does this method exist?

"cx.spawn" seems to be changed to cx.spawn_thread.

There is no matrix Request to be made for submit_async_request(MatrixRequest::SearchRoomMembers).

It is also not using room's timelineUpdate sender.

"search_room_members_streaming" is not an async function anyway. Hence there is no need for tokio.

It is a just a thread that does data processing. " cx.spawn_thread" should work.

Yes. you're right. Fixed it.

However, directly using cx.spawn_local here is not the best solution; relying on tokio is necessary.

Directly using cx.spawn_local is not the optimal approach in this context. It’s still necessary to rely on tokio, because tokio manages spawn_blocking threads and task cancellation more effectively than Makepad’s built-in threading. Of course, I won’t continue using submit_async_request(MatrixRequest::SearchRoomMembers) here, to avoid introducing logical ambiguity.

ZhangHanDong · 2025-08-01T14:19:41Z

ZhangHanDong#1

I have created a PR request after some amendments.

The code in your PR seems to be outdated.None of them are based on my latest code.The issue you mentioned does not exist in the current code.

ZhangHanDong · 2025-08-01T14:21:13Z

May I ask what issue you encountered during testing? Please comment here directly instead of submitting a PR, as the code will conflict. @alanpoon

src/shared/mentionable_text_input.rs

alanpoon · 2025-08-01T16:32:11Z

src/shared/mentionable_text_input.rs

+            submit_async_request(MatrixRequest::SearchRoomMembers {
+                room_id: room_props.room_id.clone(),
+                search_text: search_text.to_string(),
+                sender,
+                max_results: max_visible_items * SEARCH_BUFFER_MULTIPLIER,
+                cached_members: cached_members.clone(),
+            });


"cx.spawn" seems to be changed to cx.spawn_thread.

There is no matrix Request to be made for submit_async_request(MatrixRequest::SearchRoomMembers).

It is also not using room's timelineUpdate sender.

"search_room_members_streaming" is not an async function anyway. Hence there is no need for tokio.

It is a just a thread that does data processing. " cx.spawn_thread" should work.

ZhangHanDong · 2025-08-02T07:54:38Z

Further optimizations:

Changed ‎SearchResult to ‎Vec<usize> to reduce ‎String cloning.
Added a ‎BackgroundRequest enum to unify ‎MatrixRequest and ‎AsyncJob. This is because there are currently local async tasks that don’t need to interact with the matrix server, but we still want to reuse the tokio runtime for better thread management instead of using ‎cx.spawn_thread. The new ‎AsyncJob variant handles these local async tasks.

alanpoon · 2025-08-04T08:03:22Z

Further optimizations:

Changed ‎SearchResult to ‎Vec<usize> to reduce ‎String cloning.

Added a ‎BackgroundRequest enum to unify ‎MatrixRequest and ‎AsyncJob. This is because there are currently local async tasks that don’t need to interact with the matrix server, but we still want to reuse the tokio runtime for better thread management instead of using ‎cx.spawn_thread. The new ‎AsyncJob variant handles these local async tasks.

Given that the Alvin's logout PR will do a Tokio::runtime::shutdown_background. So if there is a possible panic when there is any tokio thread running, searching for member and when the user logouts at the same time.
What is the better thread management in tokio?

alanpoon

Please check with Kevin on Element before making big changes to submit_async_request.

alanpoon · 2025-08-04T07:03:34Z

src/shared/mentionable_text_input.rs

+                    if self.is_searching {
+                        let search_text = self.cmd_text_input.search_text();
+                        self.update_user_list(cx, &search_text, scope);
+                    }
                } else if self.cmd_text_input.view(id!(popup)).visible() {


Should handle the actions that hide or show the popup instead of checking the visibility of the popup on any actions.

Need to CommandTextInput's public functions, rather than querying its inner widgets. Open a discussion on why the existing CommandTextInput's public functions are insufficient.

Currently, the CommandTextInput component doesn’t have any relevant public methods. I can add a TODO comment here, but I don’t want this PR to be blocked by Makepad.

alanpoon · 2025-08-04T07:40:17Z

src/shared/mentionable_text_input.rs

+        if should_update_ui && !self.accumulated_results.is_empty() {
+            // Received search results, updating UI...
+            // Hide loading and show results immediately
+            self.members_loading = false;


It is weird to set self.members_loading to be false, based on the non-empty accumulated_results. self.show_loading_indicator(cx), never seems to be executed. I would imagine comparing room_props.room_members.len with room.active_members_count() to determine self.members_loading to be true / false.

alanpoon · 2025-08-04T07:45:25Z

src/shared/mentionable_text_input.rs

+                    if self.accumulated_results.is_empty() {
+                        // Clear results before showing empty state
+                        self.accumulated_results.clear();


Why do you want to clear an empty array?

Oh, it looks like some redundant code wasn’t completely removed.

alanpoon · 2025-08-04T08:13:55Z

src/shared/mentionable_text_input.rs

-            self.cmd_text_input.text_input_ref().set_key_focus(cx);
+            if self.members_loading {
+                // Show loading indicator while fetching members
+                self.show_loading_indicator(cx);


"self.members_loading" seems to be always false. self.show_loading_indicator(cx), does not seem to be executed.

alanpoon · 2025-08-04T09:22:41Z

src/room/member_search.rs

+        let mut all_results = Vec::new();
+        let mut sent_count = 0;
+
+        for (index, member) in members.iter().enumerate() {


"members" is Arc<Vec>. This means that length of room member list is dynamic, mutated by matrix sdk. This function search_room_members_streaming is called once. Which means that the mentionable user list only contain the room members existed at that point of time.

I am expecting a while loop with perhaps a time interval / room member sync event to access the Arc<Vec> until the accumulated length == room.active_member_count. Call CommandTextInput.add_item when room members are ready and fit the search criteria.

Also use slice member[i..j] instead of enumerate.

The current approach uses a member cache fetched from the Matrix server during room screen initialization, which is sufficient for most use cases and avoids the complexity of real-time synchronization. This PR focuses on delivering MVP functionality; further optimizations can be addressed later to avoid premature optimization. For now, when a user reopens a room, the latest member list is fetched from the server.

alanpoon · 2025-08-04T09:39:36Z

src/shared/mentionable_text_input.rs

+                popup.set_visible(cx, true);
+                self.cmd_text_input.text_input_ref().set_key_focus(cx);
+            } else {
+                // Show "no matches" if we're searching and not loading
+                self.show_no_matches_indicator(cx);
+                popup.set_visible(cx, true);
+                self.cmd_text_input.text_input_ref().set_key_focus(cx);
+            }


Use CommandTextInput's public functions instead of manually accessing child widgets.

Let’s leave it for a future PR—I’ve already added a TODO.

alanpoon · 2025-08-04T09:47:49Z

src/shared/mentionable_text_input.rs

+                self.search_receiver = None;
+
+                // If we still have loading indicator, clear it
+                if self.members_loading {


Confusing checks before state reset. Move the following code to separate functions.

Refactored.

alanpoon · 2025-08-04T09:55:22Z

src/shared/mentionable_text_input.rs

+        // Clone the indices to avoid borrowing issues
+        let member_indices = self.accumulated_results.clone();
+        let user_items_added = self.add_user_mention_items(cx, &member_indices, user_items_limit, is_desktop, room_props);


Why not just access self.accumulated_results inside add_user_mention_items? It is not multiple threaded. It is also not Arc<Mutex<Vec>>, where you need lock to avoid data race. If that is what you mean by borrowing issue.

Because there was a compilation error before.This section of the code has now been refactored.

alanpoon · 2025-08-04T10:00:41Z

src/shared/mentionable_text_input.rs

+        self.accumulated_results.clear();
+
+        // Check if we have cached members
+        let has_members = matches!(&room_props.room_members, Some(members) if !members.is_empty());


Again, has_members may not be enough, room_members is a Arc<Vec>. May not be fully synced at this point of time.

Refer to the previous explanation.

alanpoon · 2025-08-04T10:01:10Z

src/shared/mentionable_text_input.rs

+            self.members_loading = false;
+
+            let popup = self.cmd_text_input.view(id!(popup));
+            let header_view = self.cmd_text_input.view(id!(popup.header_view));
+            header_view.set_visible(cx, true);
+            popup.set_visible(cx, true);
+            self.cmd_text_input.text_input_ref().set_key_focus(cx);


Use CommandTextInput's public functions.

Refer to the previous explanation.

ZhangHanDong · 2025-08-05T05:28:33Z

Please check with Kevin on Element before making big changes to submit_async_request.

This isn’t a major change—it just reuses submit_async_request under a new name to handle background tasks, avoiding any semantic confusion that it’s sending messages to Matrix. There are no changes to submit_async_request itself.

kevinaboos · 2025-08-05T20:07:37Z

Please check with Kevin on Element before making big changes to submit_async_request.

This isn’t a major change—it just reuses submit_async_request under a new name to handle background tasks, avoiding any semantic confusion that it’s sending messages to Matrix. There are no changes to submit_async_request itself.

I don't disagree with the name change, but let's do it in a separate PR. There are various considerations here, and I don't think there's much of a compelling reason to differentiate between Matrix vs. non-Matrix background requests, especially if they're handled by the same worker.

For now, you can just move the SearchRoomMembers variant into the existing MatrixRequest enum, which is especially convenient since there is only one type of AsyncJob right now. We can refactor the large MatrixRequest enum later when it is actually required and/or beneficial to do so.

In my TSP support PR (#557), I've created a completely separate worker handler and request sender channel for this purpose, so it's probably best for us to consolidate these later, such that we have one Matrix-specific async worker, and one general async worker for everything else. But doing it in a separate PR will make everything easier to review — there's no need for 1500 lines of unrelated minor changes to sliding_sync in this PR.

implement streaming or batching of results to the UI for even better …

a6d5613

…responsiveness

ZhangHanDong requested a review from kevinaboos July 11, 2025 16:06

fixed bug

b5be2de

kevinaboos added the waiting-on-review This issue is waiting to be reviewed label Jul 11, 2025

ZhangHanDong added 2 commits July 12, 2025 11:28

remove unused field

de47523

Merge branch 'main' into optimize-member-search

c1261d7

kevinaboos requested changes Jul 22, 2025

View reviewed changes

src/sliding_sync.rs Outdated Show resolved Hide resolved

kevinaboos added waiting-on-author This issue is waiting on the original author for a response and removed waiting-on-review This issue is waiting to be reviewed labels Jul 22, 2025

ZhangHanDong added 9 commits July 29, 2025 16:25

Use channel communication instead of sending actions

145f353

Modify the type of cached_members

50ae6d6

modify gitignore

10e4079

Merge branch 'optimize-member-search' of github.com-robrix:ZhangHanDo…

d72ddc9

…ng/robrix into optimize-member-search

Merge branch 'main' into optimize-member-search

79558f8

fixed conflict

30eeb42

fixed clippy warning

5f412d5

modify search user match rule

46c522a

Further optimize the user mention search

e0d68c6

ZhangHanDong added 2 commits August 1, 2025 13:34

clean code

e1f9f6b

clean code

bcebd53

ZhangHanDong requested a review from kevinaboos August 1, 2025 05:53

ZhangHanDong added waiting-on-review This issue is waiting to be reviewed and removed waiting-on-author This issue is waiting on the original author for a response labels Aug 1, 2025

alanpoon reviewed Aug 1, 2025

View reviewed changes

ZhangHanDong added 2 commits August 2, 2025 15:51

Further optimize according to Allen’s review suggestions

eda667b

clean doc

10a1c41

fixed clippy warning

7ce4814

ZhangHanDong requested a review from alanpoon August 2, 2025 08:02

alanpoon reviewed Aug 4, 2025

View reviewed changes

ZhangHanDong added waiting-on-author This issue is waiting on the original author for a response and removed waiting-on-review This issue is waiting to be reviewed labels Aug 5, 2025

kevinaboos removed their request for review August 5, 2025 20:04

ZhangHanDong added 6 commits August 6, 2025 15:27

refactor code

8f42762

refactor: Optimize the search state

798c73e

clean code

7c8000d

refactor

fe9a10d

refactor

9cf5fbb

refactor

8b4dd9d

Fix @mention search performance and refactor search functionality #550

Are you sure you want to change the base?

Fix @mention search performance and refactor search functionality #550

Conversation

ZhangHanDong commented Jul 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kevinaboos left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ZhangHanDong commented Aug 1, 2025

Uh oh!

alanpoon left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ZhangHanDong Aug 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ZhangHanDong commented Aug 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ZhangHanDong commented Aug 1, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ZhangHanDong commented Aug 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alanpoon commented Aug 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alanpoon left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ZhangHanDong Aug 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ZhangHanDong commented Jul 11, 2025 •

edited

Loading

ZhangHanDong Aug 2, 2025 •

edited

Loading

ZhangHanDong commented Aug 1, 2025 •

edited

Loading

ZhangHanDong commented Aug 2, 2025 •

edited

Loading

alanpoon commented Aug 4, 2025 •

edited

Loading

ZhangHanDong Aug 5, 2025 •

edited

Loading

ZhangHanDong commented Aug 5, 2025 •

edited

Loading