Disambiguate displaynames (#2918)

* Disambigute displaynames

* Add test

* fixup test functions

* prettier

* lint

* Split displayname utils into own file and add tests.

* Split out fixtures

* Add more testcases for displayname calculation.

* lint

* Also listen for displayname changes. (I stand corrected!)

* fix missing media tiles on missing member

src/utils/displayname.test.ts

@@ -0,0 +1,118 @@
+/*
+Copyright 2025 New Vector Ltd.
+
+SPDX-License-Identifier: AGPL-3.0-only
+Please see LICENSE in the repository root for full details.
+*/
+
+import { describe, expect, test } from "vitest";
+
+import { calculateDisplayName, shouldDisambiguate } from "./displayname";
+import {
+  alice,
+  aliceDoppelganger,
+  aliceDoppelgangerRtcMember,
+  aliceRtcMember,
+  bob,
+  bobRtcMember,
+  bobZeroWidthSpace,
+  bobZeroWidthSpaceRtcMember,
+  daveRTL,
+} from "./test-fixtures";
+import { mockMatrixRoom } from "./test";
+
+describe("shouldDisambiguate", () => {
+  test("should not disambiguate a solo member", () => {
+    const room = mockMatrixRoom({});
+    expect(shouldDisambiguate(alice, [], room)).toEqual(false);
+  });
+  test("should not disambiguate a member with an empty displayname", () => {
+    const room = mockMatrixRoom({
+      getMember: (u) =>
+        [alice, aliceDoppelganger].find((m) => m.userId === u) ?? null,
+    });
+    expect(
+      shouldDisambiguate(
+        { rawDisplayName: "", userId: alice.userId },
+        [aliceRtcMember, aliceDoppelgangerRtcMember],
+        room,
+      ),
+    ).toEqual(false);
+  });
+  test("should disambiguate a member with RTL characters", () => {
+    const room = mockMatrixRoom({});
+    expect(shouldDisambiguate(daveRTL, [], room)).toEqual(true);
+  });
+  test("should disambiguate a member with a matching displayname", () => {
+    const room = mockMatrixRoom({
+      getMember: (u) =>
+        [alice, aliceDoppelganger].find((m) => m.userId === u) ?? null,
+    });
+    expect(
+      shouldDisambiguate(
+        alice,
+        [aliceRtcMember, aliceDoppelgangerRtcMember],
+        room,
+      ),
+    ).toEqual(true);
+    expect(
+      shouldDisambiguate(
+        aliceDoppelganger,
+        [aliceRtcMember, aliceDoppelgangerRtcMember],
+        room,
+      ),
+    ).toEqual(true);
+  });
+  test("should disambiguate a member with a matching displayname with hidden spaces", () => {
+    const room = mockMatrixRoom({
+      getMember: (u) =>
+        [bob, bobZeroWidthSpace].find((m) => m.userId === u) ?? null,
+    });
+    expect(
+      shouldDisambiguate(bob, [bobRtcMember, bobZeroWidthSpaceRtcMember], room),
+    ).toEqual(true);
+    expect(
+      shouldDisambiguate(
+        bobZeroWidthSpace,
+        [bobRtcMember, bobZeroWidthSpaceRtcMember],
+        room,
+      ),
+    ).toEqual(true);
+  });
+  test.for(["Alice @foo:bar", "@foo:b", "A@foo:lice", "A @f oo: ba r"])(
+    "should disambiguate a member with a displayname containing a mxid-like string '%s'",
+    (rawDisplayName) => {
+      const room = mockMatrixRoom({
+        getMember: (u) =>
+          [alice, aliceDoppelganger].find((m) => m.userId === u) ?? null,
+      });
+      expect(
+        shouldDisambiguate({ rawDisplayName, userId: alice.userId }, [], room),
+      ).toEqual(true);
+    },
+  );
+});
+
+describe("calculateDisplayName", () => {
+  test.for<[{ rawDisplayName?: string; userId: string }, boolean, string]>([
+    [alice, false, alice.rawDisplayName],
+    [alice, true, `${alice.rawDisplayName} (${alice.userId})`],
+    [alice, false, alice.rawDisplayName],
+    [{ rawDisplayName: "", userId: alice.userId }, false, alice.userId],
+    [
+      { rawDisplayName: alice.userId, userId: alice.userId },
+      false,
+      alice.userId,
+    ],
+    [bobZeroWidthSpace, false, "Bob"],
+    [
+      { rawDisplayName: "\u200b\u200b\u200b", userId: alice.userId },
+      false,
+      alice.userId,
+    ],
+    [daveRTL, false, "evaD"],
+    [daveRTL, true, `evaD (${daveRTL.userId})`],
+  ])("correctly calculates displayname", ([member, disambiguate, result]) =>
+    expect(calculateDisplayName(member, disambiguate)).toEqual(result),
+  );
+});

src/utils/displayname.ts

@@ -0,0 +1,81 @@
+/*
+Copyright 2025 New Vector Ltd.
+
+SPDX-License-Identifier: AGPL-3.0-only
+Please see LICENSE in the repository root for full details.
+*/
+
+import {
+  removeDirectionOverrideChars,
+  removeHiddenChars,
+} from "matrix-js-sdk/src/utils";
+
+import type { Room } from "matrix-js-sdk/src/matrix";
+import type { CallMembership } from "matrix-js-sdk/src/matrixrtc";
+
+// Borrowed from https://github.com/matrix-org/matrix-js-sdk/blob/f10deb5ef2e8f061ff005af0476034382ea128ca/src/models/room-member.ts#L409
+export function shouldDisambiguate(
+  member: { rawDisplayName?: string; userId: string },
+  memberships: CallMembership[],
+  room: Room,
+): boolean {
+  const { rawDisplayName: displayName, userId } = member;
+  if (!displayName || displayName === userId) return false;
+
+  // First check if the displayname is something we consider truthy
+  // after stripping it of zero width characters and padding spaces
+  const strippedDisplayName = removeHiddenChars(displayName);
+  if (!strippedDisplayName) return false;
+
+  // Next check if the name contains something that look like a mxid
+  // If it does, it may be someone trying to impersonate someone else
+  // Show full mxid in this case
+  if (/@.+:.+/.test(displayName)) return true;
+
+  // Also show mxid if the display name contains any LTR/RTL characters as these
+  // make it very difficult for us to find similar *looking* display names
+  // E.g "Mark" could be cloned by writing "kraM" but in RTL.
+  if (/[\u200E\u200F\u202A-\u202F]/.test(displayName)) return true;
+
+  // Also show mxid if there are other people with the same or similar
+  // displayname, after hidden character removal.
+  return (
+    memberships
+      .map((m) => m.sender && room.getMember(m.sender))
+      // NOTE: We *should* have a room member for everyone.
+      .filter((m) => !!m)
+      .filter((m) => m.userId !== userId)
+      .some((m) => calculateDisplayName(m, false) === strippedDisplayName)
+  );
+}
+
+export function calculateDisplayName(
+  member: { rawDisplayName?: string; userId: string },
+  disambiguate: boolean,
+): string {
+  const { rawDisplayName: displayName, userId } = member;
+  if (!displayName || displayName === userId) return userId;
+
+  const resultDisplayname = removeDirectionOverrideChars(
+    removeHiddenChars(displayName),
+  );
+
+  if (disambiguate) return resultDisplayname + " (" + userId + ")";
+
+  // First check if the displayname is something we consider truthy
+  // after stripping it of zero width characters and padding spaces
+  if (!removeHiddenChars(displayName)) return userId;
+
+  // We always strip the direction override characters (LRO and RLO).
+  // These override the text direction for all subsequent characters
+  // in the paragraph so if display names contained these, they'd
+  // need to be wrapped in something to prevent this from leaking out
+  // (which we can do in HTML but not text) or we'd need to add
+  // control characters to the string to reset any overrides (eg.
+  // adding PDF characters at the end). As far as we can see,
+  // there should be no reason these would be necessary - rtl display
+  // names should flip into the correct direction automatically based on
+  // the characters, and you can still embed rtl in ltr or vice versa
+  // with the embed chars or marker chars.
+  return resultDisplayname;
+}

src/utils/test-fixtures.ts

@@ -12,13 +12,50 @@ import {
   mockLocalParticipant,
 } from "./test";
 
-export const aliceRtcMember = mockRtcMembership("@alice:example.org", "AAAA");
-export const alice = mockMatrixRoomMember(aliceRtcMember);
-export const aliceId = `${alice.userId}:${aliceRtcMember.deviceId}`;
-export const aliceParticipant = mockRemoteParticipant({ identity: aliceId });
-
 export const localRtcMember = mockRtcMembership("@carol:example.org", "CCCC");
 export const local = mockMatrixRoomMember(localRtcMember);
 export const localParticipant = mockLocalParticipant({ identity: "" });
+export const localId = `${local.userId}:${localRtcMember.deviceId}`;
+
+export const aliceRtcMember = mockRtcMembership("@alice:example.org", "AAAA");
+export const alice = mockMatrixRoomMember(aliceRtcMember, {
+  rawDisplayName: "Alice",
+});
+export const aliceId = `${alice.userId}:${aliceRtcMember.deviceId}`;
+export const aliceParticipant = mockRemoteParticipant({ identity: aliceId });
+
+export const aliceDoppelgangerRtcMember = mockRtcMembership(
+  "@alice2:example.org",
+  "AAAA",
+);
+export const aliceDoppelganger = mockMatrixRoomMember(
+  aliceDoppelgangerRtcMember,
+  {
+    rawDisplayName: "Alice",
+  },
+);
+export const aliceDoppelgangerId = `${aliceDoppelganger.userId}:${aliceDoppelgangerRtcMember.deviceId}`;
 
 export const bobRtcMember = mockRtcMembership("@bob:example.org", "BBBB");
+export const bob = mockMatrixRoomMember(bobRtcMember, {
+  rawDisplayName: "Bob",
+});
+export const bobId = `${bob.userId}:${bobRtcMember.deviceId}`;
+
+export const bobZeroWidthSpaceRtcMember = mockRtcMembership(
+  "@bob2:example.org",
+  "BBBB",
+);
+export const bobZeroWidthSpace = mockMatrixRoomMember(
+  bobZeroWidthSpaceRtcMember,
+  {
+    rawDisplayName: "Bo\u200bb",
+  },
+);
+export const bobZeroWidthSpaceId = `${bobZeroWidthSpace.userId}:${bobZeroWidthSpaceRtcMember.deviceId}`;
+
+export const daveRTLRtcMember = mockRtcMembership("@dave2:example.org", "DDDD");
+export const daveRTL = mockMatrixRoomMember(daveRTLRtcMember, {
+  rawDisplayName: "\u200fevaD",
+});
+export const daveRTLId = `${daveRTL.userId}:${daveRTLRtcMember.deviceId}`;

src/utils/test.ts

@@ -205,6 +205,7 @@ export async function withLocalMedia(
       kind: E2eeType.PER_PARTICIPANT,
     },
     mockLivekitRoom({ localParticipant }),
+    of(roomMember.rawDisplayName ?? "nodisplayname"),
     of(null),
     of(null),
   );
@@ -243,6 +244,7 @@ export async function withRemoteMedia(
       kind: E2eeType.PER_PARTICIPANT,
     },
     mockLivekitRoom({}, { remoteParticipants$: of([remoteParticipant]) }),
+    of(roomMember.rawDisplayName ?? "nodisplayname"),
     of(null),
     of(null),
   );