Skip to content

Refactor components related to mentioning users. Support @room mentions.#496

Merged
kevinaboos merged 55 commits intoproject-robius:mainfrom
ZhangHanDong:mentionable-refact
Jul 8, 2025
Merged

Refactor components related to mentioning users. Support @room mentions.#496
kevinaboos merged 55 commits intoproject-robius:mainfrom
ZhangHanDong:mentionable-refact

Conversation

@ZhangHanDong
Copy link
Contributor

@ZhangHanDong ZhangHanDong commented May 27, 2025

This PR is a rewrite of PR #463.

Fixes #456.
Fixes #464.
Fixes #481.

Main improvements:

  1. Refactored components such as ‎⁠mentionable_text_input⁠ so that ‎⁠RoomScreen⁠ now holds the ‎⁠room_members⁠ information and passes it to child components like ‎⁠mentionable_text_input⁠ via props. At the same time, ‎⁠RoomMembersManager⁠ is used to manage and share ‎⁠room_members⁠ to reduce memory copying.
  2. Added the ‎⁠@room⁠ feature.
  3. Performance optimization: resolved UI lag when loading rooms with more than 2,000 users.
  4. Loading room members animation. When there are too many users in a room, user data isn’t available immediately after typing “@”. To address this, I added a “Loading room members” animation. Once the room members are loaded, they’re automatically rendered in the user list.

loading

  1. added search by user ID.
Screenshot 2025-05-29 at 18 11 26 Screenshot 2025-05-29 at 18 11 35

Details of Performance Optimization:

Problem: When a room has more than 2,000 users, rendering the mention popup causes the app to lag and show a loading spinner.

Performance Bottlenecks:

  • Unrestricted iteration over all 2,000+ members
  • Widget instances are created for all matching members, even those not visible
  • No virtualization or limiting mechanism

Solution:

  • Early Termination: Limit the number of matching members to MAX_VISIBLE_ITEMS * 2 (30 items)
  • Smart Search: Two-stage search—first match by prefix, then by substring—to provide a better user experience
  • Virtualization: Only create Widget instances for actually visible items (‎⁠take(MAX_VISIBLE_ITEMS)⁠)

Performance Improvements:

  • Reduces processing from over 2,000 items to a maximum of 30 items
  • Reduces Widget creation from over 2,000 to at most 15 visible Widgets
  • Significantly decreases string operations and Widget creation overhead

@ZhangHanDong ZhangHanDong marked this pull request as ready for review May 28, 2025 16:32
@ZhangHanDong ZhangHanDong added the waiting-on-review This issue is waiting to be reviewed label May 28, 2025
Copy link
Member

@kevinaboos kevinaboos left a comment

Choose a reason for hiding this comment

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

Thanks Alex!

It looks like there may have been some merge conflicts that were resolved incorrectly. Please ensure that any new changes i made as part of #480 are kept in the code. I identified a few places where it looks like that happened, but I might've missed some others.


The logic in the MentionableTextInput is just wayyyy too complicated. We can't be doing that many complex checks upon each inputted character. See my comments there for ways that it can be massively simplified.


I think I may have underestimated how many pervasive changes are required to pass room member info down through all of the various functions --- i was thinking that it's just the event handler and draw routines, but I forgot about all of the functions that populate widget data, like Avatar methods and the other functions in the RoomScreen.

I wonder if we should rethink the way that the room member list is stored, or if we should keep it as it currently is in this PR, in which we just pass it to every function that needs it.

For now, can you kindly remove all of the usages of the room_members_opt that aren't absolutely mandatory? All of these extra changes where you use the room members list instead of the user profile cache are not strictly necessary, and it makes it hard for me to review this. For example, don't use it in the avatar.rs or room_read_receipt.rs files yet --- we can add that later. For now, only use the room member list in the code components related to mentioning users. That way, it will be very clear where we actually need the room members list to be passed into a function.

@ZhangHanDong 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 May 29, 2025
@ZhangHanDong 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 May 29, 2025
@ZhangHanDong ZhangHanDong requested a review from kevinaboos May 29, 2025 10:03
Copy link
Member

@kevinaboos kevinaboos left a comment

Choose a reason for hiding this comment

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

Thanks, this looks much better now. I still think things can be simplified a lot more, and I left some detailed comments where I think that is the case.

  1. First, one request: can you please disable your IDE's code auto-formatter when working on Robrix (for all future PRs)? A large majority of the changes here are just formatting changes, which makes it very hard on me as the reviewer to determine what semantics actually changed.

  2. Second: when using scope with code like scope.props.get::<RoomScreenProps>(), we actually want to unwrap() that in order to cause a panic, as it should be considered a developer bug if a function expects something to be passed down via Scope but it actually isn't there.

    • Please check all the places you access scope by downcasting it to a specific type (e.g., RoomScreenProps) and ensure they're using unwrap() (or expect() if you want).
  3. Third, a general question: there are tons of new log comments everywhere in this PR, most of which seem like they're just there as leftovers from your development work. Please remove all of the logs that aren't useful enough to be seen by all future developers & users, in order to keep the Robrix log brief enough to be easily readable.

@kevinaboos 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 May 29, 2025
@kevinaboos kevinaboos added the waiting-on-author This issue is waiting on the original author for a response label Jul 2, 2025
@ZhangHanDong
Copy link
Contributor Author

This PR Design Discussion Points

  1. Component Refactoring and Data Flow Optimization

    • Unified management of room_members information by RoomScreen, passing to child components (such as mentionable_text_input) via props, and using RoomMembersManager for unified management to reduce memory copying.
    • Pass room_members list to all functions that need it, avoiding unnecessary data passing.
    • Significant simplification of MentionableTextInput, avoiding complex checks and unnecessary operations on every input.
  2. @room Feature Support

    • Added @room mention functionality, allowing one-time mention of all room members.
  3. Performance Optimization

    • Solved UI lag issues when loading large rooms (2000+ users), using room_members data from timeline expansion to avoid additional remote requests.
    • Moved checking whether room users need updates to async threads.
    • Pass room information through RoomScreenProps to avoid multiple calls to get_client and get_room.
    • Other optimization points include:
      • Limit matching member count (maximum 30).
      • Two-stage search (prefix first, then substring).
      • Virtualization, only rendering visible items (maximum 15 widgets).
      • Significantly reduced string operations and widget creation, improving response speed.
  4. User Experience Improvements

    • Added "loading room members" animation to enhance interaction experience when loading large rooms.
    • Support searching members by user ID.
  5. Code Readability Refactoring

    • Using .get(idx) is better than using [idx]
    • Eliminated some if let nesting
  6. Bug Fixes

    • Prohibit mentioning oneself.
    • Fixed issue where some user avatars weren't displaying.
    • Optimized handling logic for already mentioned users, simplified to boolean judgment.
    • Fixed popup header appearing when backspacing.
  7. Formatting and Logging

    • Disabled IDE auto-formatting to avoid meaningless format changes affecting review.
    • Removed useless logs left over from development process, keeping logs concise.
  8. Error Handling

    • When getting props through scope, recommend directly unwrap/expect; if not passed, should directly panic for developers to discover issues.
  9. Compatibility and Format Handling (deleted, awaiting future TextFlow component)

    • When editing messages, support extracting mentioned user information from original messages.
    • Fixed HTML/Markdown format conversion issues when editing messages from different clients (such as element).
  10. Follow-up

    • PR size is already large, should minimize scope and avoid introducing too many new features.
    • Recommend splitting complex features into subsequent PRs, keeping this PR focused on mention functionality refactoring and performance optimization.
    • Until we convert room members to hash maps (in background threads) for fast constant-time lookups in main thread UI, we cannot handle room members in the UI main thread.

I reviewed the code again using this checklist, and there shouldn’t be any issues now. @kevinaboos

@ZhangHanDong ZhangHanDong requested a review from kevinaboos July 4, 2025 09:31
@ZhangHanDong ZhangHanDong added waiting-on-review This issue is waiting to be reviewed waiting-on-author This issue is waiting on the original author for a response and removed waiting-on-author This issue is waiting on the original author for a response waiting-on-review This issue is waiting to be reviewed labels Jul 4, 2025
@ZhangHanDong
Copy link
Contributor Author

I’ll remove the room member subscription first and review it later. @kevinaboos

@kevinaboos
Copy link
Member

I’ll remove the room member subscription first and review it later. @kevinaboos

ok yea, sounds good, thanks for remembering that. I'll wait for your notice to review it.

@ZhangHanDong
Copy link
Contributor Author

@kevinaboos
The subscription management for room members has been completely removed. Now, room members are managed solely by the Matrix SDK. In the room screen, the room members cache from the Matrix SDK is reused. When a room tab is closed (specifically, after save_state), the room members data will be cleared. This is done to prevent Robrix’s memory usage from growing indefinitely as more rooms are opened.

@ZhangHanDong ZhangHanDong requested a review from kevinaboos July 8, 2025 02:39
@ZhangHanDong 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 Jul 8, 2025
Copy link
Member

@kevinaboos kevinaboos left a comment

Choose a reason for hiding this comment

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

Thanks Alex! Looks good and it works well, with a few small caveats. I'll merge this in now, but kindly fix these small issues in another PR as soon as possible.

When there are no matching users, we should not close the mentionable popup, because that makes it seem like the popup has unexpectedly crashed (from the user's perspective).
Previously, the mentionable popup would still be shown if there were no matching users, which is much better UX because it clearly conveys to the user that there are no matches.

A side effect of this incorrect behavior is that if you make a typo when searching, or search for a name that doesn't exist, the mentionable popup will remain permanently closed until you remove all characters after the @, at which point the popup will be shown again.

We must keep the popup open until the user either selects a mention or deletes the original @ trigger character. This will enable the user to correct their search string if no matching users are found.

@kevinaboos kevinaboos removed the waiting-on-review This issue is waiting to be reviewed label Jul 8, 2025
@kevinaboos kevinaboos merged commit b17c495 into project-robius:main Jul 8, 2025
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

3 participants