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 index 1d7ce54e8a..84a7d348b2 100644 --- 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 index 99cb444ae4..7003a71999 100644 --- 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 { 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 { 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 { 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 { .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 { /** * 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 index 5de15eaa46..93c898ee21 100644 --- 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 { } /** - * 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 index 78e79ca44a..a6bde429c3 100644 --- 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 index 074979b262..671bfface8 100644 --- 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);