Fix reactive display name disambiguation#5135
Conversation
When a room member changes their display name, recalculate the disambiguation flag for all other members who share (or previously shared) that display name. This ensures that the 'disambiguate' flag is updated reactively when display name conflicts appear or are resolved. Fixes element-hq/element-web#468 Fixes element-hq/element-web#4795 Fixes element-hq/element-web#31551 Signed-off-by: aditya-cherukuru <cherukuru.aditya01@gmail.com>
990e80f to
29521c5
Compare
|
If you believe this is a security fix, this should have been submitted privately as per https://github.com/matrix-org/matrix-js-sdk?tab=security-ov-file - had this been a real security issue, you would have just disclosed it publicly, potentially causing a major security incident (even though the original bug was public, you gave specific steps on how to exploit the bug). However, your repro steps describe Alice (the real Alice) sending a message and her display name not showing with disambiguation. This might be a little confusing but Charlie (the imposter Alice) would still be shown with the disambiguation showing he wasn't really Alice. So, if I understand correctly I think your AI might have got a little confused. You've also marked this as fixing 31551 but then said that it's only a partial fix and there's follow-up work coming. This special syntax will cause the bug to be closed when the PR is merged so if it doesn't completely fix it, please just say "For" or something instead of "Fixes", or create a sub-issue and mark it as fixing that. |
|
Thanks for the review @dbkr! apologies for the confusion. I was echoing the "security implications" mention from the original issue but yeah, framing it as a full security bug here definitely sends the wrong signal about disclosure. Still learning the ropes with OSS contributions, so I appreciate the heads up. I'll tone it down to a UX/confusion issue which is what it really is. Updating the description now. Let me know if the actual logic in |
changelog.d/5135.bugfix
Outdated
| @@ -0,0 +1 @@ | |||
| Fix reactive display name disambiguation to update immediately when room members change their display names. | |||
There was a problem hiding this comment.
We don't use towncrier so you don't need this file.
src/models/room-state.ts
Outdated
| private updateDisplayNameCache(userId: string, displayName: string): void { | ||
| const oldName = this.userIdsToDisplayNames[userId]; | ||
|
|
||
| // Track which display names are affected by this change so we can |
There was a problem hiding this comment.
I think doing this in a function called, updateDisplayNameCache is confusing as this function is now doing more than just what its name implies. It might make more sense to do it a level above, perhaps? This might also allow it be be done differently as I'm finding the recalculateDisambiguationForDisplayNames to be quite specific and more like just a chunk of one large function moved out to a separate function rather than a reusable unit of functionality.
src/models/room-state.ts
Outdated
| // Recalculate disambiguation by re-setting the membership event. | ||
| // This will call shouldDisambiguate() with the updated room state. | ||
| const oldName = member.name; | ||
| member.setMembershipEvent(member.events.member, this); |
There was a problem hiding this comment.
I don't love doing this by setting the same membership event again. Feels like if we want to update the disambiguation at other times, we should restructure to call it directly rather than faking things changing on that member.
b2f8f9d to
8f14251
Compare
- Added updateDisambiguation() method to RoomMember for direct disambiguation recalculation - Moved affected display name tracking to setStateEvents() instead of updateDisplayNameCache() - Removed setMembershipEvent() hack, now calls updateDisambiguation() directly Signed-off-by: aditya-cherukuru <cherukuru.aditya01@gmail.com>
8f14251 to
440a24c
Compare
|
Thanks for the guidance @dbkr. Your comments really made me see the code structure in a different way, I learnt a lot from this review. I've addressed the feedback and cleaned up the architecture as suggested. Will continue to get better! |
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
This is looking much nicer, thanks. One thing though: your previous logic excluded the user whose name was being changed. Do we need to do that here too to avoid duplicate events?
|
you're right! since the main loop already processes the user changing their name, my disambiguation loop would be doing redundant work for them. I'll add a check to track which userIds were processed in the main batch and exclude them from the disambiguation pass. This ensures we only emit extra events for the other affected members, not duplicates. Pushing the fix now. |
Signed-off-by: aditya-cherukuru <cherukuru.aditya01@gmail.com>
There was a problem hiding this comment.
Pull request overview
This PR fixes a bug where display name disambiguation (showing user ID suffix like Alice (@alice:server)) did not update reactively when room members changed their display names. The fix ensures that when one member changes their name to match (or stop matching) another member's name, all affected members' disambiguation flags are recalculated and UI events are emitted immediately.
Changes:
- Added tracking of affected display names during membership event processing
- Implemented recalculation of disambiguation flags for all members with affected display names
- Added new
recalculateDisambiguatedNamemethod to RoomMember class for on-demand disambiguation updates
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/models/room-state.ts | Added logic to track display name changes and recalculate disambiguation for affected members after processing membership events |
| src/models/room-member.ts | Added new recalculateDisambiguatedName method to recalculate disambiguation flag and emit Name event when needed |
| spec/unit/room-state.spec.ts | Added three comprehensive tests covering disambiguation addition, removal, and event emission |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Ah, the test failure here is legitimate and is this exact change being reflected in a screenshot of the member list in Element Web. You'll need to make a corresponding element-web PR to update the screenshot. If you use the same branch name it might pick up your fixed js-sdk so the tests pass there, although I'm not sure this will work from a fork. If not, we'll have to override the checks. |
This PR updates the Playwright screenshot tests to reflect the changes from matrix-js-sdk PR element-hq#5135, which fixes reactive display name disambiguation. The screenshot will be updated based on CI test results to show the correct disambiguation behavior when multiple room members share the same display name. Related: matrix-org/matrix-js-sdk#5135
|
Hi @dbkr, I've opened the companion Element Web PR to update the visual snapshots: element-hq/element-web#32431 I used the same branch name (fix/reactive-display-name-disambiguation) so the CI should pick up the SDK changes automatically. Currently waiting for the CI to run so I can grab the Linux-generated snapshots and push them to that branch. |
79d5df1 to
b8f9d34
Compare
|
hi @dbkr The screenshot updates there confirm the fix is working as expected. This should be ready to merge whenever you are! |
* Update screenshot for reactive display name disambiguation This PR updates the Playwright screenshot tests to reflect the changes from matrix-js-sdk PR #5135, which fixes reactive display name disambiguation. The screenshot will be updated based on CI test results to show the correct disambiguation behavior when multiple room members share the same display name. Related: matrix-org/matrix-js-sdk#5135 * Update member list screenshot for reactive disambiguation * Retry CI for flaky MatrixChat timeout
* Update screenshot for reactive display name disambiguation This PR updates the Playwright screenshot tests to reflect the changes from matrix-js-sdk PR #5135, which fixes reactive display name disambiguation. The screenshot will be updated based on CI test results to show the correct disambiguation behavior when multiple room members share the same display name. Related: matrix-org/matrix-js-sdk#5135 * Update member list screenshot for reactive disambiguation * Retry CI for flaky MatrixChat timeout
* Update screenshot for reactive display name disambiguation This PR updates the Playwright screenshot tests to reflect the changes from matrix-js-sdk PR #5135, which fixes reactive display name disambiguation. The screenshot will be updated based on CI test results to show the correct disambiguation behavior when multiple room members share the same display name. Related: matrix-org/matrix-js-sdk#5135 * Update member list screenshot for reactive disambiguation * Retry CI for flaky MatrixChat timeout
|
Bypassing checks as the complement crypto tests are broken with pnpm and CI is broken without this. |
Problem
Display name disambiguation (showing user ID suffix like
Alice (@alice:server)) was not updating reactively when room members changed their display names.Steps to reproduce:
Root Cause:
updateDisplayNameCache()inroom-state.tscorrectly updates the display name → userId mapping, but did NOT recalculate thedisambiguateflag for other members who share (or previously shared) that display name.Solution
recalculateDisambiguationForDisplayNames()RoomState.Membersevent so UI updates immediatelyTesting
Added 3 unit tests covering the fix:
Run with:
yarn test -- --testPathPattern=room-state.spec.ts -t "reactive"What this fixes
Remaining work (separate PR)
This PR addresses reactive updates for name changes. A separate issue exists for parted users (element-hq/element-meta#2850):
Related to element-hq/element-web#31551
Related issues:
Checklist
Signed-off-by: @aditya-cherukuru cherukuru.aditya01@gmail.com