From 9358096ac6ebf1fed4b6097658bc75b21c8fc366 Mon Sep 17 00:00:00 2001 From: Florian Duros Date: Fri, 20 Mar 2026 14:36:58 +0100 Subject: [PATCH] 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 --- .../viewmodels/room-list/RoomListViewModel.ts | 23 +++++-- .../room-list/RoomListViewModel-test.tsx | 62 +++++++++++++++++++ 2 files changed, 79 insertions(+), 6 deletions(-) diff --git a/apps/web/src/viewmodels/room-list/RoomListViewModel.ts b/apps/web/src/viewmodels/room-list/RoomListViewModel.ts index 5d5fecfefe..d3a59a6d22 100644 --- a/apps/web/src/viewmodels/room-list/RoomListViewModel.ts +++ b/apps/web/src/viewmodels/room-list/RoomListViewModel.ts @@ -52,6 +52,9 @@ export class RoomListViewModel // 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: RoomListViewModelProps) { @@ -142,11 +145,11 @@ export class RoomListViewModel }; /** - * 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 RoomListViewModel 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 RoomListViewModel // 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 RoomListViewModel // 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 RoomListViewModel return; } + this.updateRoomsMap(this.roomsResult); + // Normal room list update (not a space change) this.updateRoomListData(); }; diff --git a/apps/web/test/viewmodels/room-list/RoomListViewModel-test.tsx b/apps/web/test/viewmodels/room-list/RoomListViewModel-test.tsx index 2f95795f36..ce5c9ce6b4 100644 --- a/apps/web/test/viewmodels/room-list/RoomListViewModel-test.tsx +++ b/apps/web/test/viewmodels/room-list/RoomListViewModel-test.tsx @@ -180,6 +180,25 @@ describe("RoomListViewModel", () => { expect(disposeSpy1).toHaveBeenCalled(); expect(disposeSpy2).toHaveBeenCalled(); }); + + it("should clear roomsMap when space changes and repopulate with new rooms", () => { + viewModel = new RoomListViewModel({ 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("RoomListViewModel", () => { }).toThrow(); }); + it("should not throw when requesting view model for a room removed from the list but still in roomsMap", () => { + viewModel = new RoomListViewModel({ 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 RoomListViewModel({ 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 RoomListViewModel({ 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 RoomListViewModel({ client: matrixClient });