-
-
Notifications
You must be signed in to change notification settings - Fork 135
fix(Mention): Maintain mention after nym change #2591
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: master
Are you sure you want to change the base?
Conversation
86071e6 to
4639417
Compare
4639417 to
95745c5
Compare
95745c5 to
363af8e
Compare
363af8e to
ea13f2e
Compare
ea13f2e to
44e4154
Compare
44e4154 to
0f58cd2
Compare
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.
Hi @brymut, sorry for the delay, we've been hard at work with flagship features.
As you may know, I'm working on the new editor, and everything kind of changed. Including mentions, so I wanted to review this in order to see how I can adapt it to the new paradigm and give you recognition for it.
This logic doesn't actually fix anything, it adds a way to get a User via Mention, but it still uses the name parsed from markdown. So if I change my nym, the mention breaks.1
Screen.Recording.2025-11-13.at.15.26.20.mp4
I don't understand how you made it work in your video, and probably your fix actually did work in previous commits, but it's hard to see them because of force-pushing.
Footnotes
-
in the video I'm using the rich_text branch, but I merged your PR to test it. ↩
api/resolvers/user.js
Outdated
| item: true | ||
| } | ||
| }) | ||
| const matchingMention = mentions.find(mention => mention.user?.name === name) |
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.
this breaks if the mentioned user changes nym.
|
Hi @Soxasora, let me re-familiarise myself, will rebase and update this PR and get back to you if that's ok. |
0f58cd2 to
4293c54
Compare
|
Hi @Soxasora, sorry for the force pushing again. Noticed my original solution hadn't properly factored in the order in which mentions are stored in the DB and multiple mentions in the same post. Updated the screen recording in the PR description with those test cases. Should be good to be used, let me know if you have any questions. |
Description
Changing nym should still point to the user mentioned instead of losing link.
fixes #2232
Tested scenarios with screen recording:
Screen.Recording.2025-11-26.at.18.43.02.mov
Screen.Recording.2025-11-26.at.18.44.44.mov
Screen.Recording.2025-11-26.at.19.00.00.mov
Checklist
Are your changes backward compatible? Please answer below:
Y
On a scale of 1-10 how well and how have you QA'd this change and any features it might affect? Please answer below:
10
For frontend changes: Tested on mobile, light and dark mode? Please answer below:
Y
Did you introduce any new environment variables? If so, call them out explicitly here:
N
Did you use AI for this? If so, how much did it assist you?
Y. Mostly gathering context on how mentions are stored in DB and helping me debug ordering problem of mentions.
Note
Resolve mentions to the correct user even after username changes by adding
userByMentionGraphQL query and wiring frontend mention/popover to use it withitemId.Query.userByMention(name: String!, itemId: ID): Userwith resolver that resolves a user fromMentionrecords for a givenitemId, falling back touser(name).components/text.js: PassitemIdintomentionrenderer andUserPopover; dynamically link mentions to/${user.name}when resolved. UpdateuseMemodeps to includeitemId.components/user-popover.js: QueryUSER_BY_MENTION(whenitemIdprovided) and fall back toUSER; expose resolved user to children; refine loading and display logic.USER_BY_MENTIONquery infragments/users.js.Written by Cursor Bugbot for commit 0f58cd2. This will update automatically on new commits. Configure here.