Skip to content

content: Add GroupMention to render user group mentions dynamically #2119

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.

Sign up for GitHub

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

Jump to bottom
Open
Ruhdee wants to merge 6 commits into
base: main
Choose a base branch
from
Open

content: Add GroupMention to render user group mentions dynamically #2119

Ruhdee wants to merge 6 commits into from
+162 −71

Conversation

@Ruhdee
Copy link
Contributor

@Ruhdee Ruhdee commented Feb 4, 2026 •
edited
Loading

Fixes #1259.

Before After
old_light new_light
old_dark new_dark

Changes Made:

  1. Added sealed class 'MentionNode' and refactored variables, cleaned comments accordingly.
  2. Added 'GroupMentionNode' class
  3. Added 'GroupMention' Widget. Created this separate widget instead of keeping the singular UserMention widget to introduce changes in the future (content: Distinguish direct and wildcard mentions #646 will introduce separate styling, maybe in the future, user group mentions might become tappable, etc.). It is also in line with the sealed class implementation.
  4. Updated and added tests

Doubts:

  1. Between the 2nd and 3rd commits, I have made a temporary implementation of GroupMentionNode case and added a TODO comment in the content widget file to separate the commits. Is this practice correct?
  2. In 1st commit, I have fixed one old comment (description of TODO issue 646) in the content widget file. Is that the way to change old comments that might be incorrect?

@gnprice gnprice added the maintainer review PR ready for review by Zulip maintainers label Feb 5, 2026
@rajveermalviya rajveermalviya self-assigned this Feb 6, 2026
rajveermalviya
rajveermalviya requested changes Feb 6, 2026
View reviewed changes
Copy link
Member

@rajveermalviya rajveermalviya left a comment

Choose a reason for hiding this comment

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

Thanks for working on this @Ruhdee! Overall looks good, some small comments below.

@Ruhdee Ruhdee force-pushed the issue-1259 branch 7 times, most recently from 71538c1 to b2408e8 Compare February 6, 2026 18:22
@Ruhdee
Copy link
Contributor Author

Ruhdee commented Feb 6, 2026

@rajveermalviya Thank you for the review! I have addressed the feedback. PTAL.

@Ruhdee Ruhdee requested a review from rajveermalviya February 6, 2026 18:25
rajveermalviya
rajveermalviya approved these changes Feb 8, 2026
View reviewed changes
Copy link
Member

@rajveermalviya rajveermalviya left a comment

Choose a reason for hiding this comment

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

Thanks for the revision @Ruhdee! All looks good, modulo one nit.

Moving over to Greg's review.

@rajveermalviya rajveermalviya added integration review Added by maintainers when PR may be ready for integration and removed maintainer review PR ready for review by Zulip maintainers labels Feb 8, 2026
@Ruhdee
Copy link
Contributor Author

Ruhdee commented Feb 8, 2026

I have made the latest requested change.

gnprice
gnprice reviewed Feb 9, 2026
View reviewed changes
Copy link
Member

@gnprice gnprice left a comment

Choose a reason for hiding this comment

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

Thanks @Ruhdee for taking care of this, and thanks @rajveermalviya for the previous reviews!

Generally this looks great. Various small comments below, mostly nits.

lib/model/content.dart Outdated
final userId = switch (element.attributes['data-user-id']) {
// For legacy or wildcard mentions.
null || '*' => null,
final userIdString => int.tryParse(userIdString),
Copy link
Member

@gnprice gnprice Feb 9, 2026

Choose a reason for hiding this comment

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

Ah, and I see this existing call is missing radix: 10. So that would be good to fix as a separate prep commit.

Copy link
Contributor Author

@Ruhdee Ruhdee Feb 10, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

Since this change affects the behaviour of the function in case 'data-user-id' is a hexadecimal, this change technically counts as a functional change, right?

@Ruhdee Ruhdee force-pushed the issue-1259 branch 5 times, most recently from a3ebdd1 to 77f3ce7 Compare February 10, 2026 15:48
@Ruhdee
content [nfc]: Fix TODO comment in UserMention widget code
b028bd4
@Ruhdee
content: Fix usage of int.tryParse by adding radix: 10
7ad7226
@Ruhdee
content [nfc]: Add sealed base class MentionNode
f1be91e 
Deleted enum 'UserMentionType' and outdated comments. Refactored
'parseUserMention' and changed it's return type to 'MentionNode'.
Renamed '_userMentionClassNameRegexp'.
@Ruhdee
content [nfc]: Rename UserMention widget to Mention
c0d5f70
@Ruhdee
content: Add UserGroupMentionNode and update parseMention
ce7ee0e
@Ruhdee
content: Update Mention widget to render user group mentions dynami…
66a7186 
…cally

Fixes zulip#1259.

Look up current user group name from PerAccountStore when rendering
mentions, while respecting "isSilent" flag. This ensures mentions
display correctly even if the user group name has been changed since
the message was sent. Falls back to original HTML text when user
group not found. Added 'user group mention dynamic name resolution'
tests group.
@Ruhdee Ruhdee requested a review from gnprice February 10, 2026 16:30
@Ruhdee
Copy link
Contributor Author

Ruhdee commented Feb 10, 2026

I have addressed all the feedback. @gnprice PTAL.
Why CI is failing: #2129 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@rajveermalviya rajveermalviya rajveermalviya approved these changes

@gnprice gnprice Awaiting requested review from gnprice

Assignees

@gnprice gnprice

Labels

integration review Added by maintainers when PR may be ready for integration

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Handle user-group renames in rendering @-mentions

3 participants

@Ruhdee @gnprice @rajveermalviya