Skip to content

Searching messages in a room (not encrypted) #483

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

Open
wants to merge 22 commits into
base: main
Choose a base branch
from

Conversation

alanpoon
Copy link
Contributor

@alanpoon alanpoon commented May 16, 2025

@alanpoon alanpoon requested a review from aaravlu May 16, 2025 04:31
@aaravlu aaravlu requested a review from Copilot May 16, 2025 04:54
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

Adds support for searching messages via the Matrix Search API and integrates UI hooks for initiating and clearing searches.

  • Extends MatrixRequest with SearchMessages and implements async handling, pagination, and UI signaling
  • Updates the room filter input bar to emit click events and clears the filter when switching rooms
  • Introduces a new room_search_result module and wires it into live_design alongside related minor refactors and generics updates

Reviewed Changes

Copilot reviewed 12 out of 12 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
src/sliding_sync.rs Implements SearchMessages request handling and task management
src/shared/room_filter_input_bar.rs Emits Click action and handles Clear in the filter bar
src/home/rooms_list.rs Adds is_room_encrypted flag and getters
src/home/room_read_receipt.rs Generalizes populate_read_receipts with Eventable
src/home/new_message_context_menu.rs Makes from_user_power_and_event generic
src/home/mod.rs Registers the new room_search_result module
src/home/main_desktop_ui.rs Clears filter input on room tab close
src/event_preview.rs Updates text_preview_of_other_state signature
Cargo.toml Pins ruma dependency for search API
Comments suppressed due to low confidence (2)

src/home/new_message_context_menu.rs:249

  • [nitpick] The _generic suffix on the function name is redundant and may be confusing. Consider renaming it back to from_user_power_and_event now that it’s generic.
pub fn from_user_power_and_event_generic<T: Eventable, M: MsgTypeAble>(

src/sliding_sync.rs:1115

  • [nitpick] There are no tests covering the new search workflow (including pagination and error paths). Consider adding unit tests or integration tests for SearchMessages handling.
MatrixRequest::SearchMessages { room_id, search_term, include_all_rooms, next_batch, abort_previous_search }

}
}
});
register_core_task(CoreTask::Search, handle.abort_handle());
Copy link
Preview

Copilot AI May 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

handle is a JoinHandle, which does not expose an abort_handle() method. You need to either store the JoinHandle itself and call abort(), or wrap the future in a cancelable AbortHandle.

Suggested change
register_core_task(CoreTask::Search, handle.abort_handle());
}, abort_registration);
let handle = Handle::current().spawn(abortable_future);
register_core_task(CoreTask::Search, abort_handle);

Copilot uses AI. Check for mistakes.

@@ -210,6 +209,7 @@ impl MainDesktopUI {
dock.close_tab(cx, tab_id);
self.tab_to_close = None;
self.open_rooms.remove(&tab_id);
cx.widget_action(self.widget_uid(), &Scope::empty().path, RoomFilterAction::Clear);
Copy link
Preview

Copilot AI May 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] Using Scope::empty().path may not target the filter widget correctly. You may need to reference the actual scope path where the RoomFilterInputBar lives.

Suggested change
cx.widget_action(self.widget_uid(), &Scope::empty().path, RoomFilterAction::Clear);
cx.widget_action(self.widget_uid(), &scope.path, RoomFilterAction::Clear);

Copilot uses AI. Check for mistakes.

Copy link
Contributor

@aaravlu aaravlu May 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

May be there is better to use action() if we dont want an actual Scope rather than widget_action()?

@alanpoon alanpoon changed the title Added functionality for searching messages in a room Searching messages in a room (not encrypted) May 16, 2025
@alanpoon alanpoon force-pushed the search_messages_3#122 branch from d5763a4 to ca2ca78 Compare May 19, 2025 09:31
@alanpoon alanpoon force-pushed the search_messages_3#122 branch from a88c44b to 4a7d12c Compare May 21, 2025 11:24
@alanpoon alanpoon marked this pull request as ready for review May 21, 2025 12:05
@smarizvi110
Copy link
Contributor

smarizvi110 commented May 22, 2025

MUTED_2025-05-22.18-27-24.mp4

Hi @alanpoon! This PR looks wonderful! I checked it out after your request to do so on #395 and found a few issues. The attached video walks through them (please skip to 0:19)

Firstly, the initial start up sequence looks very different from what the program looks like when cloned from the original repo's main branch. Secondly, several icons seem to be missing too. I checked out the commits ahead of your branch on project-robius/robrix:main and it doesn't seem like there was any file changed that would lead to this issue, but I could be wrong on this.

Third, the search feature itself seems to work! The only problem is that when you attempt to search all rooms while initiating a search in an encrypted room, it does not seem to search in any available unencrypted rooms either and simply tells me that searching unencrypted rooms is currently not supported. Furthermore, the little notification that tells me this seems to never go away, and just stays there in the top right corner. Perhaps that should be tweaked to disappear in a few seconds?

One last thing is that the mermaid diagram you attached to the PR here makes sense, but I don't quite understand why there are 4 edges coming out of the Check Room Encryption node. Shouldn't there just be 2, one for encrypted room searching flow and one for unencrypted room search flow? If I'm misunderstanding something, please do let me know!

For reference, the second video shows what Robrix looks like currently on project-robius/robrix:main where the icons are not missing and the startup screen looks a little more user-friendly.

Please note that in both cases, I gave Robrix my credentials through the CLI when using cargo run.

2025-05-22.18-30-28.mp4

Thank you!

@alanpoon alanpoon force-pushed the search_messages_3#122 branch from dfb5651 to 747cb44 Compare May 23, 2025 06:15
@alanpoon
Copy link
Contributor Author

MUTED_2025-05-22.18-27-24.mp4

Hi @alanpoon! This PR looks wonderful! I checked it out after your request to do so on #395 and found a few issues. The attached video walks through them (please skip to 0:19)

Firstly, the initial start up sequence looks very different from what the program looks like when cloned from the original repo's main branch. Secondly, several icons seem to be missing too. I checked out the commits ahead of your branch on project-robius/robrix:main and it doesn't seem like there was any file changed that would lead to this issue, but I could be wrong on this.

Third, the search feature itself seems to work! The only problem is that when you attempt to search all rooms while initiating a search in an encrypted room, it does not seem to search in any available unencrypted rooms either and simply tells me that searching unencrypted rooms is currently not supported. Furthermore, the little notification that tells me this seems to never go away, and just stays there in the top right corner. Perhaps that should be tweaked to disappear in a few seconds?

One last thing is that the mermaid diagram you attached to the PR here makes sense, but I don't quite understand why there are 4 edges coming out of the Check Room Encryption node. Shouldn't there just be 2, one for encrypted room searching flow and one for unencrypted room search flow? If I'm misunderstanding something, please do let me know!

For reference, the second video shows what Robrix looks like currently on project-robius/robrix:main where the icons are not missing and the startup screen looks a little more user-friendly.

Please note that in both cases, I gave Robrix my credentials through the CLI when using cargo run.

2025-05-22.18-30-28.mp4

Thank you!

Thank you for testing.
Unfortunately, I cannot replicate the missing icon issues even after cloning my branch. I have fixed the issue of not be able to search all rooms while initiating a search in an encrypted room. There's supposed to be a close button on the top right of the popup which could be missing due to missing icon issues.
I have also fixed the mermaid diagram.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants