-
Notifications
You must be signed in to change notification settings - Fork 41
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?
Changes from all commits
2fcd9ed
c6df225
3c9ec1a
a009763
e93fd1e
1caf4ef
22bec53
777967d
5938646
2d0abd0
048000b
d31c12a
bb916a0
8d29838
89decb1
ac5414d
480a52f
75dc668
be1321a
bb5e83e
df504c2
fccce82
a73dce1
0ae4f95
f80f26d
3853537
f4305b4
270d6d1
524e1a3
8b537fd
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,4 +6,8 @@ | |
| .vscode | ||
| .DS_Store | ||
| CLAUDE.md | ||
| AGENTS.md | ||
| proxychains.conf | ||
| /specs | ||
| ai-docs/ | ||
| .claude | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,14 @@ | ||
| [files] | ||
| # Extend default configuration | ||
| extend-exclude = [ | ||
| "*/tests/*", | ||
| "*/test_*.rs", | ||
| "*_test.rs", | ||
| ] | ||
|
|
||
| [default.extend-words] | ||
| # Add any custom words that should not be flagged as typos | ||
| # Test strings used in member_search.rs tests | ||
| hel = "hel" | ||
| caf = "caf" | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -247,12 +247,13 @@ impl MatchEvent for App { | |
| } | ||
|
|
||
| if let Some(LogoutAction::ClearAppState { on_clear_appstate }) = action.downcast_ref() { | ||
| // Clear user profile cache, invited_rooms timeline states | ||
| // Clear user profile cache, invited_rooms timeline states | ||
| clear_all_app_state(cx); | ||
| // Reset all app state to its default. | ||
| self.app_state = Default::default(); | ||
| on_clear_appstate.notify_one(); | ||
| continue; | ||
| // Don't continue here - let the action propagate to child widgets (e.g., RoomScreen) | ||
| // so they can reset their state as well | ||
|
Comment on lines
-255
to
+256
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. that's not what The only thing that Thus, you can safely restore the |
||
| } | ||
|
|
||
| if let Some(LoginAction::LoginSuccess) = action.downcast_ref() { | ||
|
|
@@ -436,12 +437,14 @@ impl MatchEvent for App { | |
| } | ||
|
|
||
| /// Clears all thread-local UI caches (user profiles, invited rooms, and timeline states). | ||
| /// The `cx` parameter ensures that these thread-local caches are cleared on the main UI thread, | ||
| /// The `cx` parameter ensures that these thread-local caches are cleared on the main UI thread, | ||
| fn clear_all_app_state(cx: &mut Cx) { | ||
| clear_user_profile_cache(cx); | ||
| clear_all_invited_rooms(cx); | ||
| clear_timeline_states(cx); | ||
| clear_avatar_cache(cx); | ||
| // Clear room members digests to ensure members are re-fetched after re-login | ||
| crate::sliding_sync::clear_room_members_digests(); | ||
|
Comment on lines
+446
to
+447
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. naming question: what is a room member digest? The only definition of digest I can think of is "a summary of something", so i'm not sure how that's relevant here. |
||
| } | ||
|
|
||
| impl AppMain for App { | ||
|
|
@@ -609,6 +612,23 @@ pub struct AppState { | |
| } | ||
|
|
||
| /// A snapshot of the main dock: all state needed to restore the dock tabs/layout. | ||
| /// | ||
| /// # Cross-Version Hash Drift Warning | ||
| /// | ||
| /// The `LiveId` keys in `dock_items` and `open_rooms` are derived from `LiveId::from_str(room_id)`. | ||
| /// However, the hash value may change when upgrading Makepad or the Rust toolchain, causing | ||
| /// persisted `LiveId`s to no longer match freshly computed ones. This leads to duplicate tabs | ||
| /// when restoring state across versions. | ||
| /// | ||
| /// **Current mitigation**: `MainDesktopUI::find_open_room_live_id` performs a reverse-lookup | ||
| /// by `room_id` to find the actual stored `LiveId`, avoiding hash mismatch issues at runtime. | ||
| /// | ||
| // TODO: A more thorough fix would be to use `room_id` (String) as the persistence key instead | ||
| // of `LiveId`, and derive `LiveId` at runtime when needed. This would eliminate cross-version | ||
| // hash drift entirely and make the persisted format stable across Makepad/toolchain upgrades. | ||
| // | ||
| // TODO: Consider using `OwnedRoomId` (String) as the key instead of `LiveId` to avoid | ||
| // cross-version hash drift. See struct-level documentation for details. | ||
|
Comment on lines
+615
to
+631
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. please don't paste this huge message everywhere throughout the codebase. We already have an issue #639 to track this. |
||
| #[derive(Clone, Default, Debug, Serialize, Deserialize)] | ||
| pub struct SavedDockState { | ||
| /// All items contained in the dock, keyed by their room or space ID. | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,63 @@ | ||
| //! Lightweight wrapper for CPU-bound tasks. | ||
| //! | ||
| //! Currently each job is handled by spawning a detached native thread via | ||
| //! Makepad's `cx.spawn_thread`. This keeps the implementation simple while | ||
| //! still moving CPU-heavy work off the UI thread. | ||
| //! | ||
| //! ## Future TODOs | ||
| //! - TODO: Add task queue with priority and deduplication | ||
| //! - TODO: Limit max concurrent tasks (e.g., 2-4 workers) | ||
| //! - TODO: Add platform-specific thread pool (desktop only, via #[cfg]) | ||
| //! - TODO: Support task cancellation and timeout | ||
| //! - TODO: Add progress callbacks for long-running tasks | ||
|
Comment on lines
+7
to
+12
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Aside from task cancelation (which we really should include right now), i don't think any of this will ever be necessary, to be honest. We certainly don't want to limit the max number of tasks or make things desktop-only, since Robrix forbids platform-specific code. |
||
| use makepad_widgets::{Cx, CxOsApi}; | ||
| use std::sync::{atomic::AtomicBool, mpsc::Sender, Arc}; | ||
| use crate::{ | ||
| room::member_search::{search_room_members_streaming_with_sort, PrecomputedMemberSort}, | ||
| shared::mentionable_text_input::SearchResult, | ||
| }; | ||
| use matrix_sdk::room::RoomMember; | ||
|
|
||
| pub enum CpuJob { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: "CpuJob" is a bit vague. Let's at least use the term "background" somewhere, maybe "BackgroundSyncJob" to distinguish from all the other async workers? |
||
| SearchRoomMembers(SearchRoomMembersJob), | ||
| } | ||
|
|
||
| pub struct SearchRoomMembersJob { | ||
| pub members: Arc<Vec<RoomMember>>, | ||
| pub search_text: String, | ||
| pub max_results: usize, | ||
| pub sender: Sender<SearchResult>, | ||
| pub search_id: u64, | ||
| pub precomputed_sort: Option<Arc<PrecomputedMemberSort>>, | ||
| pub cancel_token: Option<Arc<AtomicBool>>, | ||
alanpoon marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| } | ||
|
|
||
| fn run_member_search(params: SearchRoomMembersJob) { | ||
| let SearchRoomMembersJob { | ||
| members, | ||
| search_text, | ||
| max_results, | ||
| sender, | ||
| search_id, | ||
| precomputed_sort, | ||
| cancel_token, | ||
| } = params; | ||
|
|
||
| search_room_members_streaming_with_sort( | ||
| members, | ||
| search_text, | ||
| max_results, | ||
| sender, | ||
| search_id, | ||
| precomputed_sort, | ||
| cancel_token, | ||
| ); | ||
| } | ||
|
Comment on lines
+36
to
+56
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what's the point of this fn? Why not just directly call |
||
|
|
||
| /// Spawns a CPU-bound job on a detached native thread. | ||
| pub fn spawn_cpu_job(cx: &mut Cx, job: CpuJob) { | ||
| cx.spawn_thread(move || match job { | ||
| CpuJob::SearchRoomMembers(params) => run_member_search(params), | ||
| }); | ||
alanpoon marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -135,22 +135,49 @@ impl Widget for MainDesktopUI { | |
|
|
||
| impl MainDesktopUI { | ||
| /// Focuses on a room if it is already open, otherwise creates a new tab for the room. | ||
| /// | ||
| /// # Duplicate Tab Prevention (Cross-Version Hash Drift) | ||
| /// | ||
| /// This method uses [`Self::find_open_room_live_id`] to check if the room is already open, | ||
| /// rather than directly comparing `LiveId::from_str(room_id)`. This is necessary because | ||
| /// persisted `LiveId`s may differ from freshly computed ones due to **cross-version hash drift**. | ||
| /// | ||
| /// ## Root Cause | ||
| /// | ||
| /// The 64-bit value of `LiveId::from_str` depends on Makepad's hash implementation (and | ||
| /// potentially compiler/stdlib hash seeds). When upgrading Makepad or the Rust toolchain, | ||
| /// the hash algorithm or seed may change. Persisted data (in `open_rooms`/`dock_items`) | ||
| /// contains "old hash values," while the new runtime computes "new hash values." This | ||
| /// causes `contains_key`/`select_tab` lookups to fail, and the room is incorrectly | ||
| /// treated as "not open," resulting in duplicate tabs. | ||
| /// | ||
| /// ## Current Fix | ||
| /// | ||
| /// By reverse-looking up the actual stored `LiveId` via `room_id` comparison (using | ||
| /// [`Self::find_open_room_live_id`]), we correctly identify already-open rooms regardless | ||
| /// of hash drift between versions. | ||
| /// | ||
| // TODO: A more thorough fix would be to use `room_id` (String) as the persistence key | ||
| // instead of `LiveId`, and derive `LiveId` at runtime. This would eliminate cross-version | ||
| // hash drift entirely. See `SavedDockState` in `src/app.rs`. | ||
|
Comment on lines
+138
to
+162
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. please don't paste this huge message everywhere throughout the codebase. We already have an issue #639 to track this. |
||
| fn focus_or_create_tab(&mut self, cx: &mut Cx, room: SelectedRoom) { | ||
| // Do nothing if the room to select is already created and focused. | ||
| if self.most_recently_selected_room.as_ref().is_some_and(|r| r == &room) { | ||
| return; | ||
| } | ||
|
|
||
| // If the room is already open, select (jump to) its existing tab. | ||
| // We use `find_open_room_live_id` to look up by room_id, because the dock | ||
| // may store LiveIds with a prefix that differs from `LiveId::from_str(room_id)`. | ||
| let dock = self.view.dock(ids!(dock)); | ||
|
|
||
| // If the room is already open, select (jump to) its existing tab | ||
| let room_id_as_live_id = LiveId::from_str(room.room_id().as_str()); | ||
| if self.open_rooms.contains_key(&room_id_as_live_id) { | ||
| dock.select_tab(cx, room_id_as_live_id); | ||
| if let Some(existing_live_id) = self.find_open_room_live_id(room.room_id()) { | ||
| dock.select_tab(cx, existing_live_id); | ||
| self.most_recently_selected_room = Some(room); | ||
| return; | ||
| } | ||
|
|
||
| let room_id_as_live_id = LiveId::from_str(room.room_id().as_str()); | ||
|
|
||
| // Create a new tab for the room | ||
| let (tab_bar, _pos) = dock.find_tab_bar_of_tab(id!(home_tab)).unwrap(); | ||
| let (kind, name) = match &room { | ||
|
|
@@ -201,13 +228,28 @@ impl MainDesktopUI { | |
| ); | ||
| } | ||
| } | ||
| // Only update open_rooms after successful tab creation to avoid orphan entries | ||
| self.open_rooms.insert(room_id_as_live_id, room.clone()); | ||
| self.most_recently_selected_room = Some(room); | ||
| cx.action(MainDesktopUiAction::SaveDockIntoAppState); | ||
| } else { | ||
| error!("BUG: failed to create tab for {room:?}"); | ||
| } | ||
| } | ||
|
|
||
| self.open_rooms.insert(room_id_as_live_id, room.clone()); | ||
| self.most_recently_selected_room = Some(room); | ||
| /// Finds the `LiveId` of an already-open room by its `room_id`. | ||
| /// | ||
| /// This reverse-lookup is necessary to handle **cross-version hash drift**: when Makepad | ||
| /// or the toolchain is upgraded, `LiveId::from_str(room_id)` may compute a different hash | ||
| /// than what was persisted. By matching on the stable `room_id` value instead of the | ||
| /// potentially-drifted `LiveId`, we correctly identify rooms regardless of version changes. | ||
| /// | ||
| /// See [`Self::focus_or_create_tab`] for more details on the root cause. | ||
|
Comment on lines
+246
to
+247
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. remove these 2 lines. The above lines are more than sufficient to justify this function. |
||
| fn find_open_room_live_id(&self, room_id: &OwnedRoomId) -> Option<LiveId> { | ||
| self.open_rooms | ||
| .iter() | ||
| .find(|(_, selected_room)| selected_room.room_id() == room_id) | ||
| .map(|(live_id, _)| *live_id) | ||
| } | ||
|
|
||
| /// Closes a tab in the dock and focuses on the latest open room. | ||
|
|
||
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.
in general, we do want these words to be flagged as mis-spelled. Can you just allow them in the one specific place where they should not be considered typos?