Skip to content

Commit 028357f

Browse files
aditya-cherukuruaditya-cherukuru
authored
Fix reactive display name disambiguation (#5135)
* Fix reactive display name disambiguation 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> * Refactor: move disambiguation logic per review feedback - 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> * Exclude processed members from disambiguation loop Signed-off-by: aditya-cherukuru <cherukuru.aditya01@gmail.com> --------- Signed-off-by: aditya-cherukuru <cherukuru.aditya01@gmail.com>
1 parent 872ec67 commit 028357f

File tree

3 files changed

+225
-1
lines changed

3 files changed

+225
-1
lines changed

‎spec/unit/room-state.spec.ts‎

Lines changed: 140 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1308,4 +1308,144 @@ describe("RoomState", function () {
13081308
).toBeFalsy();
13091309
});
13101310
});
1311+
1312+
describe("reactive display name disambiguation", function () {
1313+
it("should disambiguate existing member when another member changes to the same name", function () {
1314+
// Create a fresh state
1315+
const testState = new RoomState(roomId);
1316+
1317+
// Alice joins with display name "Alice"
1318+
const aliceJoinEvent = utils.mkMembership({
1319+
user: userA,
1320+
mship: KnownMembership.Join,
1321+
room: roomId,
1322+
event: true,
1323+
name: "Alice",
1324+
});
1325+
1326+
// Bob joins with display name "Bob"
1327+
const bobJoinEvent = utils.mkMembership({
1328+
user: userB,
1329+
mship: KnownMembership.Join,
1330+
room: roomId,
1331+
event: true,
1332+
name: "Bob",
1333+
});
1334+
1335+
testState.setStateEvents([aliceJoinEvent, bobJoinEvent]);
1336+
1337+
// Verify no disambiguation needed initially
1338+
const aliceBefore = testState.getMember(userA);
1339+
const bobBefore = testState.getMember(userB);
1340+
expect(aliceBefore?.disambiguate).toBe(false);
1341+
expect(bobBefore?.disambiguate).toBe(false);
1342+
expect(aliceBefore?.name).toBe("Alice");
1343+
expect(bobBefore?.name).toBe("Bob");
1344+
1345+
// Bob changes display name to "Alice"
1346+
const bobRenameEvent = utils.mkMembership({
1347+
user: userB,
1348+
mship: KnownMembership.Join,
1349+
room: roomId,
1350+
event: true,
1351+
name: "Alice",
1352+
});
1353+
1354+
testState.setStateEvents([bobRenameEvent]);
1355+
1356+
// Now both should be disambiguated
1357+
const aliceAfter = testState.getMember(userA);
1358+
const bobAfter = testState.getMember(userB);
1359+
expect(aliceAfter?.disambiguate).toBe(true);
1360+
expect(bobAfter?.disambiguate).toBe(true);
1361+
expect(aliceAfter?.name).toContain(userA);
1362+
expect(bobAfter?.name).toContain(userB);
1363+
});
1364+
1365+
it("should un-disambiguate member when conflicting member changes to different name", function () {
1366+
// Create a fresh state
1367+
const testState = new RoomState(roomId);
1368+
1369+
// Both Alice and Bob join with display name "Alice"
1370+
const aliceJoinEvent = utils.mkMembership({
1371+
user: userA,
1372+
mship: KnownMembership.Join,
1373+
room: roomId,
1374+
event: true,
1375+
name: "Alice",
1376+
});
1377+
1378+
const bobJoinEvent = utils.mkMembership({
1379+
user: userB,
1380+
mship: KnownMembership.Join,
1381+
room: roomId,
1382+
event: true,
1383+
name: "Alice",
1384+
});
1385+
1386+
testState.setStateEvents([aliceJoinEvent, bobJoinEvent]);
1387+
1388+
// Verify both are disambiguated
1389+
const aliceBefore = testState.getMember(userA);
1390+
const bobBefore = testState.getMember(userB);
1391+
expect(aliceBefore?.disambiguate).toBe(true);
1392+
expect(bobBefore?.disambiguate).toBe(true);
1393+
1394+
// Bob changes display name to "Bob"
1395+
const bobRenameEvent = utils.mkMembership({
1396+
user: userB,
1397+
mship: KnownMembership.Join,
1398+
room: roomId,
1399+
event: true,
1400+
name: "Bob",
1401+
});
1402+
1403+
testState.setStateEvents([bobRenameEvent]);
1404+
1405+
// Alice should no longer be disambiguated, Bob should not be either
1406+
const aliceAfter = testState.getMember(userA);
1407+
const bobAfter = testState.getMember(userB);
1408+
expect(aliceAfter?.disambiguate).toBe(false);
1409+
expect(bobAfter?.disambiguate).toBe(false);
1410+
expect(aliceAfter?.name).toBe("Alice");
1411+
expect(bobAfter?.name).toBe("Bob");
1412+
});
1413+
1414+
it("should emit RoomState.members for affected members when disambiguation changes", function () {
1415+
// Create a fresh state
1416+
const testState = new RoomState(roomId);
1417+
1418+
// Alice joins with display name "Alice"
1419+
const aliceJoinEvent = utils.mkMembership({
1420+
user: userA,
1421+
mship: KnownMembership.Join,
1422+
room: roomId,
1423+
event: true,
1424+
name: "Alice",
1425+
});
1426+
1427+
testState.setStateEvents([aliceJoinEvent]);
1428+
1429+
// Set up listener for Members event
1430+
const membersEmitted: string[] = [];
1431+
testState.on(RoomStateEvent.Members, (_ev, _state, member) => {
1432+
membersEmitted.push(member.userId);
1433+
});
1434+
1435+
// Bob joins with display name "Alice" - should trigger disambiguation for Alice
1436+
const bobJoinEvent = utils.mkMembership({
1437+
user: userB,
1438+
mship: KnownMembership.Join,
1439+
room: roomId,
1440+
event: true,
1441+
name: "Alice",
1442+
});
1443+
1444+
testState.setStateEvents([bobJoinEvent]);
1445+
1446+
// Both Alice and Bob should have emitted Members events
1447+
expect(membersEmitted).toContain(userA);
1448+
expect(membersEmitted).toContain(userB);
1449+
});
1450+
});
13111451
});

‎src/models/room-member.ts‎

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -227,6 +227,42 @@ export class RoomMember extends TypedEventEmitter<RoomMemberEvent, RoomMemberEve
227227
}
228228
}
229229

230+
/**
231+
* Recalculate the disambiguation flag for this member based on current room state.
232+
* This should be called when another member's display name changes and may affect
233+
* whether this member needs disambiguation.
234+
*
235+
* @param roomState - The current room state to use for disambiguation check
236+
* @returns true if the member's name changed as a result of the disambiguation update
237+
*
238+
* @remarks
239+
* Fires {@link RoomMemberEvent.Name}
240+
*/
241+
public recalculateDisambiguatedName(roomState: RoomState): boolean {
242+
if (!this.events.member) {
243+
return false;
244+
}
245+
246+
const displayName = this.events.member.getDirectionalContent().displayname ?? "";
247+
const newDisambiguate = shouldDisambiguate(this.userId, displayName, roomState);
248+
249+
if (newDisambiguate === this.disambiguate) {
250+
return false;
251+
}
252+
253+
this.disambiguate = newDisambiguate;
254+
const oldName = this.name;
255+
this.name = calculateDisplayName(this.userId, displayName, this.disambiguate);
256+
257+
if (oldName !== this.name) {
258+
this.updateModifiedTime();
259+
this.emit(RoomMemberEvent.Name, this.events.member, this, oldName);
260+
return true;
261+
}
262+
263+
return false;
264+
}
265+
230266
/**
231267
* Update this room member's power level event. Will fire
232268
* "RoomMember.powerLevel" if the new power level is different

‎src/models/room-state.ts‎

Lines changed: 49 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -438,6 +438,11 @@ export class RoomState extends TypedEventEmitter<EmittedEvents, EventHandlerMap>
438438
this.updateModifiedTime();
439439

440440
// update the core event dict
441+
// Track display names that change so we can recalculate disambiguation
442+
const affectedDisplayNames = new Set<string>();
443+
// Track userIds whose membership events we process so we don't emit duplicate events
444+
const processedMemberUserIds = new Set<string>();
445+
441446
stateEvents.forEach((event) => {
442447
if (event.getRoomId() !== this.roomId || !event.isState()) return;
443448

@@ -448,7 +453,22 @@ export class RoomState extends TypedEventEmitter<EmittedEvents, EventHandlerMap>
448453
const lastStateEvent = this.getStateEventMatching(event);
449454
this.setStateEvent(event);
450455
if (event.getType() === EventType.RoomMember) {
451-
this.updateDisplayNameCache(event.getStateKey()!, event.getContent().displayname ?? "");
456+
const userId = event.getStateKey()!;
457+
processedMemberUserIds.add(userId);
458+
const newDisplayName = event.getContent().displayname ?? "";
459+
const oldDisplayName = this.userIdsToDisplayNames[userId];
460+
461+
// Track both old and new display names for disambiguation recalculation
462+
if (oldDisplayName) {
463+
const strippedOld = removeHiddenChars(oldDisplayName);
464+
if (strippedOld) affectedDisplayNames.add(strippedOld);
465+
}
466+
if (newDisplayName) {
467+
const strippedNew = removeHiddenChars(newDisplayName);
468+
if (strippedNew) affectedDisplayNames.add(strippedNew);
469+
}
470+
471+
this.updateDisplayNameCache(userId, newDisplayName);
452472
this.updateThirdPartyTokenCache(event);
453473
}
454474
this.emit(RoomStateEvent.Events, event, this, lastStateEvent);
@@ -514,6 +534,33 @@ export class RoomState extends TypedEventEmitter<EmittedEvents, EventHandlerMap>
514534
}
515535
});
516536

537+
// Recalculate disambiguation for all members whose display names were affected.
538+
// This ensures that when a user changes their name to match (or stop matching)
539+
// another user, all affected users' disambiguation flags are updated correctly.
540+
if (affectedDisplayNames.size > 0) {
541+
// Collect all affected user IDs first to avoid duplicate processing
542+
const affectedUserIds = new Set<string>();
543+
for (const displayName of affectedDisplayNames) {
544+
const userIds = this.displayNameToUserIds.get(displayName) ?? [];
545+
userIds.forEach((id) => affectedUserIds.add(id));
546+
}
547+
548+
// Process each affected member once, excluding those whose membership
549+
// events were already processed (they already got their events emitted)
550+
for (const userId of affectedUserIds) {
551+
if (processedMemberUserIds.has(userId)) {
552+
continue;
553+
}
554+
const member = this.members[userId];
555+
if (member?.events.member) {
556+
const nameChanged = member.recalculateDisambiguatedName(this);
557+
if (nameChanged) {
558+
this.emit(RoomStateEvent.Members, member.events.member, this, member);
559+
}
560+
}
561+
}
562+
}
563+
517564
this.emit(RoomStateEvent.Update, this);
518565
}
519566

@@ -1110,6 +1157,7 @@ export class RoomState extends TypedEventEmitter<EmittedEvents, EventHandlerMap>
11101157

11111158
private updateDisplayNameCache(userId: string, displayName: string): void {
11121159
const oldName = this.userIdsToDisplayNames[userId];
1160+
11131161
delete this.userIdsToDisplayNames[userId];
11141162
if (oldName) {
11151163
// Remove the old name from the cache.

0 commit comments

Comments
 (0)