From 09f56ed3b6d45b620fade8130c672c54ab669d00 Mon Sep 17 00:00:00 2001 From: ElementRobot Date: Fri, 20 Mar 2026 16:44:40 +0100 Subject: [PATCH] [Backport staging] Fix soft crash of room list when trying to open a room (#32872) * Fix soft crash of room list when trying to open a room (#32864) * fix: soft crash of room list trying to get item vm * test: add test to check roomMap recovery and cleared when needed (cherry picked from commit 9358096ac6ebf1fed4b6097658bc75b21c8fc366) * test: fix room list vm usage in test https://github.com/element-hq/element-web/pull/32819 has renamed the vm but this PR isn't in the base branch of the backport --------- Co-authored-by: Florian Duros Co-authored-by: Florian Duros --- .../room-list/RoomListViewViewModel.ts | 23 +++++-- .../room-list/RoomListViewViewModel-test.tsx | 62 +++++++++++++++++++ 2 files changed, 79 insertions(+), 6 deletions(-) diff --git a/apps/web/src/viewmodels/room-list/RoomListViewViewModel.ts b/apps/web/src/viewmodels/room-list/RoomListViewViewModel.ts index 2eaea3d246..3f241c584f 100644 --- a/apps/web/src/viewmodels/room-list/RoomListViewViewModel.ts +++ b/apps/web/src/viewmodels/room-list/RoomListViewViewModel.ts @@ -52,6 +52,9 @@ export class RoomListViewViewModel // Child view model management private roomItemViewModels = new Map(); + // This map is intentionally additive (never cleared except on space changes) to avoid a race condition: + // a list update can refresh roomsResult and roomsMap before the view re-renders, so the view may still + // request a view model for a room that was removed from the latest list. Keeping old entries prevents a crash. private roomsMap = new Map(); public constructor(props: RoomListViewViewModelProps) { @@ -142,11 +145,11 @@ export class RoomListViewViewModel }; /** - * Rebuild roomsMap when roomsResult changes. + * Add rooms from the RoomsResult to the roomsMap for quick lookup. + * This does not clear the roomsMap. * This maintains a quick lookup for room objects. */ private updateRoomsMap(roomsResult: RoomsResult): void { - this.roomsMap.clear(); for (const room of roomsResult.rooms) { this.roomsMap.set(room.roomId, room); } @@ -181,11 +184,15 @@ export class RoomListViewViewModel let viewModel = this.roomItemViewModels.get(roomId); if (!viewModel) { - const room = this.roomsMap.get(roomId); + let room = this.roomsMap.get(roomId); if (!room) { - throw new Error(`Room ${roomId} not found in roomsMap`); + // Maybe the roomsMap is out of date due to a recent roomsResult change that hasn't been applied yet (race condition) + this.updateRoomsMap(this.roomsResult); + room = this.roomsMap.get(roomId); } + if (!room) throw new Error(`Room ${roomId} not found in roomsMap`); + // Create new view model viewModel = new RoomListItemViewModel({ room, @@ -298,8 +305,6 @@ export class RoomListViewViewModel // Refresh room data from store this.roomsResult = RoomListStoreV3.instance.getSortedRoomsInActiveSpace(filterKeys); - this.updateRoomsMap(this.roomsResult); - const newSpaceId = this.roomsResult.spaceId; // Detect space change @@ -308,6 +313,10 @@ export class RoomListViewViewModel // We only want to do this on space changes, not on regular list updates, to preserve view models when possible // The view models are disposed when scrolling out of view (handled by updateVisibleRooms) this.clearViewModels(); + // Clear roomsMap to prevent stale room data - it will be repopulated with the new roomsResult + this.roomsMap.clear(); + + this.updateRoomsMap(this.roomsResult); // Space changed - get the last selected room for the new space to prevent flicker const lastSelectedRoom = SpaceStore.instance.getLastSelectedRoomIdForSpace(newSpaceId); @@ -316,6 +325,8 @@ export class RoomListViewViewModel return; } + this.updateRoomsMap(this.roomsResult); + // Normal room list update (not a space change) this.updateRoomListData(); }; diff --git a/apps/web/test/viewmodels/room-list/RoomListViewViewModel-test.tsx b/apps/web/test/viewmodels/room-list/RoomListViewViewModel-test.tsx index ceeb940669..4aaa370122 100644 --- a/apps/web/test/viewmodels/room-list/RoomListViewViewModel-test.tsx +++ b/apps/web/test/viewmodels/room-list/RoomListViewViewModel-test.tsx @@ -180,6 +180,25 @@ describe("RoomListViewViewModel", () => { expect(disposeSpy1).toHaveBeenCalled(); expect(disposeSpy2).toHaveBeenCalled(); }); + + it("should clear roomsMap when space changes and repopulate with new rooms", () => { + viewModel = new RoomListViewViewModel({ client: matrixClient }); + + const newSpaceRoom = mkStubRoom("!spaceroom:server", "Space Room", matrixClient); + + jest.spyOn(RoomListStoreV3.instance, "getSortedRoomsInActiveSpace").mockReturnValue({ + spaceId: "!space:server", + rooms: [newSpaceRoom], + }); + jest.spyOn(SpaceStore.instance, "getLastSelectedRoomIdForSpace").mockReturnValue(null); + + RoomListStoreV3.instance.emit(RoomListStoreV3Event.ListsUpdate); + + // New space room should be accessible + expect(() => viewModel.getRoomItemViewModel("!spaceroom:server")).not.toThrow(); + // Old rooms from the home space should not be accessible + expect(() => viewModel.getRoomItemViewModel("!room1:server")).toThrow(); + }); }); describe("Active room tracking", () => { @@ -342,6 +361,49 @@ describe("RoomListViewViewModel", () => { }).toThrow(); }); + it("should not throw when requesting view model for a room removed from the list but still in roomsMap", () => { + viewModel = new RoomListViewViewModel({ client: matrixClient }); + + // Normal list update removes room2 from the list + jest.spyOn(RoomListStoreV3.instance, "getSortedRoomsInActiveSpace").mockReturnValue({ + spaceId: "home", + rooms: [room1, room3], + }); + + RoomListStoreV3.instance.emit(RoomListStoreV3Event.ListsUpdate); + + expect(() => viewModel.getRoomItemViewModel("!room2:server")).not.toThrow(); + }); + + it("should throw when requesting view model for a room from old space after space change", () => { + viewModel = new RoomListViewViewModel({ client: matrixClient }); + + const spaceRoom = mkStubRoom("!newroom:server", "New Room", matrixClient); + + // Space change: new space only has spaceRoom + jest.spyOn(RoomListStoreV3.instance, "getSortedRoomsInActiveSpace").mockReturnValue({ + spaceId: "!space:server", + rooms: [spaceRoom], + }); + jest.spyOn(SpaceStore.instance, "getLastSelectedRoomIdForSpace").mockReturnValue(null); + + RoomListStoreV3.instance.emit(RoomListStoreV3Event.ListsUpdate); + + expect(() => viewModel.getRoomItemViewModel("!room1:server")).toThrow( + "Room !room1:server not found in roomsMap", + ); + }); + + it("should recover when roomsMap is stale but roomsResult has the room", () => { + viewModel = new RoomListViewViewModel({ client: matrixClient }); + + // Manually clear roomsMap to simulate stale cache, but keep roomsResult intact + (viewModel as any).roomsMap.clear(); + + // getRoomItemViewModel should retry by re-populating roomsMap from roomsResult + expect(() => viewModel.getRoomItemViewModel("!room1:server")).not.toThrow(); + }); + it("should dispose view models for rooms no longer visible", () => { viewModel = new RoomListViewViewModel({ client: matrixClient });