New Room List: Prevent old tombstoned rooms from appearing in the list (#29881)

MidhunSureshR · web-flow · commit 6ba21dafa7a7 · 2025-05-06T15:57:13.000Z
* Write failing playwright test

Basically when someone changes their name, any old tombstoned rooms that
were previously hidden would suddenly show up in the list.

* Split addRoom into two methods

- `reInsertRoom` that re-inserts a room that is already known by the skiplist.
- `addNewRoom` to add new rooms

The idea is that sometimes you only want to re-insert to noop, eg: when
you get an event in an old room that was upgraded.

* Use new methods in the RLS

Only use `addNewRoom` when absolutely necessary. Most events should
instead use `reInsertRoom` which will noop when the room isn't already
known by the skiplist.

* Fix broken tests

* Add new test

* Fix playwright test
diff --git a/playwright/e2e/left-panel/room-list-panel/room-list-filter-sort.spec.ts b/playwright/e2e/left-panel/room-list-panel/room-list-filter-sort.spec.ts
@@ -5,8 +5,11 @@
  * Please see LICENSE files in the repository root for full details.
  */
 
+import { type Visibility } from "matrix-js-sdk/src/matrix";
+import { type Locator, type Page } from "@playwright/test";
+
 import { expect, test } from "../../../element-web-test";
-import type { Locator, Page } from "@playwright/test";
+import { SettingLevel } from "../../../../src/settings/SettingLevel";
 
 test.describe("Room list filters and sort", () => {
     test.use({
@@ -39,6 +42,65 @@ test.describe("Room list filters and sort", () => {
         await app.closeNotificationToast();
     });
 
+    test("Tombstoned rooms are not shown even when they receive updates", async ({ page, app, bot }) => {
+        // This bug shows up with this setting turned on
+        await app.settings.setValue("Spaces.allRoomsInHome", null, SettingLevel.DEVICE, true);
+
+        /*
+        We will first create a room named 'Old Room' and will invite the bot user to this room.
+        We will also send a simple message in this room.
+        */
+        const oldRoomId = await app.client.createRoom({ name: "Old Room" });
+        await app.client.inviteUser(oldRoomId, bot.credentials.userId);
+        await bot.joinRoom(oldRoomId);
+        const response = await app.client.sendMessage(oldRoomId, "Hello!");
+
+        /*
+        At this point, we haven't done anything interesting.
+        So we expect 'Old Room' to show up in the room list.
+        */
+        const roomListView = getRoomList(page);
+        const oldRoomTile = roomListView.getByRole("gridcell", { name: "Open room Old Room" });
+        await expect(oldRoomTile).toBeVisible();
+
+        /*
+        Now let's tombstone 'Old Room'.
+        First we create a new room ('New Room') with the predecessor set to the old room..
+        */
+        const newRoomId = await bot.createRoom({
+            name: "New Room",
+            creation_content: {
+                predecessor: {
+                    event_id: response.event_id,
+                    room_id: oldRoomId,
+                },
+            },
+            visibility: "public" as Visibility,
+        });
+
+        /*
+        Now we can send the tombstone event itself to the 'Old Room'.
+        */
+        await app.client.sendStateEvent(oldRoomId, "m.room.tombstone", {
+            body: "This room has been replaced",
+            replacement_room: newRoomId,
+        });
+
+        // Let's join the replaced room.
+        await app.client.joinRoom(newRoomId);
+
+        // We expect 'Old Room' to be hidden from the room list.
+        await expect(oldRoomTile).not.toBeVisible();
+
+        /*
+        Let's say some user in the 'Old Room' changes their display name.
+        This will send events to the all the rooms including 'Old Room'.
+        Nevertheless, the replaced room should not be shown in the room list.
+        */
+        await bot.setDisplayName("MyNewName");
+        await expect(oldRoomTile).not.toBeVisible();
+    });
+
     test.describe("Scroll behaviour", () => {
         test("should scroll to the top of list when filter is applied and active room is not in filtered list", async ({
             page,
diff --git a/src/stores/room-list-v3/RoomListStoreV3.ts b/src/stores/room-list-v3/RoomListStoreV3.ts
@@ -211,23 +211,28 @@ export class RoomListStoreV3Class extends AsyncStoreWithClient<EmptyObject> {
                 const oldMembership = getEffectiveMembership(payload.oldMembership);
                 const newMembership = getEffectiveMembershipTag(payload.room, payload.membership);
 
+                // If the user is kicked, re-insert the room and do nothing more.
                 const ownUserId = this.matrixClient.getSafeUserId();
                 const isKicked = (payload.room as Room).getMember(ownUserId)?.isKicked();
-                const shouldRemove =
-                    !isKicked &&
+                if (isKicked) {
+                    this.addRoomAndEmit(payload.room);
+                    return;
+                }
+
+                // If the user has left this room, remove it from the skiplist.
+                if (
                     (payload.oldMembership === KnownMembership.Invite ||
                         payload.oldMembership === KnownMembership.Join) &&
-                    payload.membership === KnownMembership.Leave;
-
-                if (shouldRemove) {
+                    payload.membership === KnownMembership.Leave
+                ) {
                     this.roomSkipList.removeRoom(payload.room);
                     this.emit(LISTS_UPDATE_EVENT);
                     return;
                 }
 
+                // If we're joining an upgraded room, we'll want to make sure we don't proliferate
+                // the dead room in the list.
                 if (oldMembership !== EffectiveMembership.Join && newMembership === EffectiveMembership.Join) {
-                    // If we're joining an upgraded room, we'll want to make sure we don't proliferate
-                    // the dead room in the list.
                     const roomState: RoomState = payload.room.currentState;
                     const predecessor = roomState.findPredecessor(this.msc3946ProcessDynamicPredecessor);
                     if (predecessor) {
@@ -236,7 +241,8 @@ export class RoomListStoreV3Class extends AsyncStoreWithClient<EmptyObject> {
                         else logger.warn(`Unable to find predecessor room with id ${predecessor.roomId}`);
                     }
                 }
-                this.addRoomAndEmit(payload.room);
+
+                this.addRoomAndEmit(payload.room, true);
                 break;
             }
         }
@@ -260,7 +266,7 @@ export class RoomListStoreV3Class extends AsyncStoreWithClient<EmptyObject> {
                             logger.warn(`${roomId} was found in DMs but the room is not in the store`);
                             continue;
                         }
-                        this.roomSkipList!.addRoom(room);
+                        this.roomSkipList!.reInsertRoom(room);
                         needsEmit = true;
                     }
                 }
@@ -274,7 +280,7 @@ export class RoomListStoreV3Class extends AsyncStoreWithClient<EmptyObject> {
                     .map((id) => this.matrixClient?.getRoom(id))
                     .filter((room) => !!room);
                 for (const room of rooms) {
-                    this.roomSkipList!.addRoom(room);
+                    this.roomSkipList!.reInsertRoom(room);
                     needsEmit = true;
                 }
                 break;
@@ -303,10 +309,12 @@ export class RoomListStoreV3Class extends AsyncStoreWithClient<EmptyObject> {
     /**
      * Add a room to the skiplist and emit an update.
      * @param room The room to add to the skiplist
+     * @param isNewRoom Set this to true if this a new room that the isn't already in the skiplist
      */
-    private addRoomAndEmit(room: Room): void {
+    private addRoomAndEmit(room: Room, isNewRoom = false): void {
         if (!this.roomSkipList) throw new Error("roomSkipList hasn't been created yet!");
-        this.roomSkipList.addRoom(room);
+        if (isNewRoom) this.roomSkipList.addNewRoom(room);
+        else this.roomSkipList.reInsertRoom(room);
         this.emit(LISTS_UPDATE_EVENT);
     }
 
diff --git a/src/stores/room-list-v3/skip-list/RoomSkipList.ts b/src/stores/room-list-v3/skip-list/RoomSkipList.ts
@@ -90,15 +90,34 @@ export class RoomSkipList implements Iterable<Room> {
     }
 
     /**
-     * Adds a given room to the correct sorted position in the list.
-     * If the room is already present in the list, it is first removed.
+     * Re-inserts a room that is already in the skiplist.
+     * This method does nothing if the room isn't already in the skiplist.
+     * @param room the room to add
      */
-    public addRoom(room: Room): void {
-        /**
-         * Remove this room from the skip list if necessary.
-         */
+    public reInsertRoom(room: Room): void {
+        if (!this.roomNodeMap.has(room.roomId)) {
+            return;
+        }
         this.removeRoom(room);
+        this.addNewRoom(room);
+    }
 
+    /**
+     * Adds a new room to the skiplist.
+     * This method will throw an error if the room is already in the skiplist.
+     * @param room the room to add
+     */
+    public addNewRoom(room: Room): void {
+        if (this.roomNodeMap.has(room.roomId)) {
+            throw new Error(`Can't add room to skiplist: ${room.roomId} is already in the skiplist!`);
+        }
+        this.insertRoom(room);
+    }
+
+    /**
+     * Adds a given room to the correct sorted position in the list.
+     */
+    private insertRoom(room: Room): void {
         const newNode = new RoomNode(room);
         newNode.checkIfRoomBelongsToActiveSpace();
         newNode.applyFilters(this.filters);
diff --git a/test/unit-tests/stores/room-list-v3/RoomListStoreV3-test.ts b/test/unit-tests/stores/room-list-v3/RoomListStoreV3-test.ts
@@ -13,7 +13,7 @@ import type { RoomNotificationState } from "../../../../src/stores/notifications
 import { LISTS_UPDATE_EVENT, RoomListStoreV3Class } from "../../../../src/stores/room-list-v3/RoomListStoreV3";
 import { AsyncStoreWithClient } from "../../../../src/stores/AsyncStoreWithClient";
 import { RecencySorter } from "../../../../src/stores/room-list-v3/skip-list/sorters/RecencySorter";
-import { mkEvent, mkMessage, mkSpace, stubClient, upsertRoomStateEvents } from "../../../test-utils";
+import { mkEvent, mkMessage, mkSpace, mkStubRoom, stubClient, upsertRoomStateEvents } from "../../../test-utils";
 import { getMockedRooms } from "./skip-list/getMockedRooms";
 import { AlphabeticSorter } from "../../../../src/stores/room-list-v3/skip-list/sorters/AlphabeticSorter";
 import dispatcher from "../../../../src/dispatcher/dispatcher";
@@ -205,14 +205,17 @@ describe("RoomListStoreV3", () => {
             expect(roomIds).toContain(newRoom.roomId);
         });
 
-        it("Rooms are inserted on m.direct event", async () => {
-            const { store, dispatcher } = await getRoomListStore();
+        it("Rooms are re-inserted on m.direct event", async () => {
+            const { store, dispatcher, client } = await getRoomListStore();
+
+            // Let's mock the client to return new rooms with the name "My DM Room"
+            client.getRoom = (roomId: string) => mkStubRoom(roomId, "My DM Room", client);
 
             // Let's create a m.direct event that we can dispatch
             const content = {
-                "@bar1:matrix.org": ["!newroom1:matrix.org", "!newroom2:matrix.org"],
-                "@bar2:matrix.org": ["!newroom3:matrix.org", "!newroom4:matrix.org"],
-                "@bar3:matrix.org": ["!newroom5:matrix.org"],
+                "@bar1:matrix.org": ["!foo1:matrix.org", "!foo2:matrix.org"],
+                "@bar2:matrix.org": ["!foo3:matrix.org", "!foo4:matrix.org"],
+                "@bar3:matrix.org": ["!foo5:matrix.org"],
             };
             const event = mkEvent({
                 event: true,
@@ -223,6 +226,8 @@ describe("RoomListStoreV3", () => {
 
             const fn = jest.fn();
             store.on(LISTS_UPDATE_EVENT, fn);
+
+            // Do the actual dispatch
             dispatcher.dispatch(
                 {
                     action: "MatrixActions.accountData",
@@ -235,17 +240,21 @@ describe("RoomListStoreV3", () => {
             // Ensure only one emit occurs
             expect(fn).toHaveBeenCalledTimes(1);
 
-            // Each of these rooms should now appear in the store
-            // We don't need to mock the rooms themselves since our mocked
-            // client will create the rooms on getRoom() call.
-            const roomIds = store.getSortedRooms().map((r) => r.roomId);
-            [
-                "!newroom1:matrix.org",
-                "!newroom2:matrix.org",
-                "!newroom3:matrix.org",
-                "!newroom4:matrix.org",
-                "!newroom5:matrix.org",
-            ].forEach((id) => expect(roomIds).toContain(id));
+            /*
+             When the dispatched event is processed by the room-list, the associated
+             rooms will be fetched via client.getRoom and will be re-inserted into the
+             skip list. We can confirm that this happened by checking if all the dm rooms
+             have the same name ("My DM Room") since we've mocked the client to return such rooms.
+             */
+            const ids = [
+                "!foo1:matrix.org",
+                "!foo2:matrix.org",
+                "!foo3:matrix.org",
+                "!foo4:matrix.org",
+                "!foo5:matrix.org",
+            ];
+            const rooms = store.getSortedRooms().filter((r) => ids.includes(r.roomId));
+            rooms.forEach((room) => expect(room.name).toBe("My DM Room"));
         });
 
         it("Room is re-inserted on tag change", async () => {
diff --git a/test/unit-tests/stores/room-list-v3/skip-list/RoomSkipList-test.ts b/test/unit-tests/stores/room-list-v3/skip-list/RoomSkipList-test.ts
@@ -62,7 +62,7 @@ describe("RoomSkipList", () => {
         for (const room of toInsert) {
             // Insert this room 10 times
             for (let i = 0; i < 10; ++i) {
-                skipList.addRoom(room);
+                skipList.reInsertRoom(room);
             }
         }
         // Sorting order should be the same as before
@@ -84,7 +84,7 @@ describe("RoomSkipList", () => {
                 event: true,
             });
             room.timeline.push(event);
-            skipList.addRoom(room);
+            skipList.reInsertRoom(room);
             expect(skipList.size).toEqual(rooms.length);
         }
         const sortedRooms = [...skipList];
@@ -93,6 +93,12 @@ describe("RoomSkipList", () => {
         }
     });
 
+    it("Throws error when same room is added via addNewRoom", () => {
+        const { skipList, rooms } = generateSkipList();
+        const room = rooms[5];
+        expect(() => skipList.addNewRoom(room)).toThrow("Can't add room to skiplist");
+    });
+
     it("Re-sort works when sorter is swapped", () => {
         const { skipList, rooms, sorter } = generateSkipList();
         const sortedByRecency = [...rooms].sort((a, b) => sorter.comparator(a, b));
@@ -120,7 +126,7 @@ describe("RoomSkipList", () => {
 
             // Shuffle and insert the rooms
             for (const room of shuffle(rooms)) {
-                roomSkipList.addRoom(room);
+                roomSkipList.addNewRoom(room);
             }
 
             expect(roomSkipList.size).toEqual(totalRooms);
