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
This commit is contained in:
@@ -52,6 +52,9 @@ export class RoomListViewModel
|
|||||||
|
|
||||||
// Child view model management
|
// Child view model management
|
||||||
private roomItemViewModels = new Map<string, RoomListItemViewModel>();
|
private roomItemViewModels = new Map<string, RoomListItemViewModel>();
|
||||||
|
// 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<string, Room>();
|
private roomsMap = new Map<string, Room>();
|
||||||
|
|
||||||
public constructor(props: RoomListViewModelProps) {
|
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.
|
* This maintains a quick lookup for room objects.
|
||||||
*/
|
*/
|
||||||
private updateRoomsMap(roomsResult: RoomsResult): void {
|
private updateRoomsMap(roomsResult: RoomsResult): void {
|
||||||
this.roomsMap.clear();
|
|
||||||
for (const room of roomsResult.rooms) {
|
for (const room of roomsResult.rooms) {
|
||||||
this.roomsMap.set(room.roomId, room);
|
this.roomsMap.set(room.roomId, room);
|
||||||
}
|
}
|
||||||
@@ -181,11 +184,15 @@ export class RoomListViewModel
|
|||||||
let viewModel = this.roomItemViewModels.get(roomId);
|
let viewModel = this.roomItemViewModels.get(roomId);
|
||||||
|
|
||||||
if (!viewModel) {
|
if (!viewModel) {
|
||||||
const room = this.roomsMap.get(roomId);
|
let room = this.roomsMap.get(roomId);
|
||||||
if (!room) {
|
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
|
// Create new view model
|
||||||
viewModel = new RoomListItemViewModel({
|
viewModel = new RoomListItemViewModel({
|
||||||
room,
|
room,
|
||||||
@@ -298,8 +305,6 @@ export class RoomListViewModel
|
|||||||
|
|
||||||
// Refresh room data from store
|
// Refresh room data from store
|
||||||
this.roomsResult = RoomListStoreV3.instance.getSortedRoomsInActiveSpace(filterKeys);
|
this.roomsResult = RoomListStoreV3.instance.getSortedRoomsInActiveSpace(filterKeys);
|
||||||
this.updateRoomsMap(this.roomsResult);
|
|
||||||
|
|
||||||
const newSpaceId = this.roomsResult.spaceId;
|
const newSpaceId = this.roomsResult.spaceId;
|
||||||
|
|
||||||
// Detect space change
|
// 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
|
// 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)
|
// The view models are disposed when scrolling out of view (handled by updateVisibleRooms)
|
||||||
this.clearViewModels();
|
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
|
// Space changed - get the last selected room for the new space to prevent flicker
|
||||||
const lastSelectedRoom = SpaceStore.instance.getLastSelectedRoomIdForSpace(newSpaceId);
|
const lastSelectedRoom = SpaceStore.instance.getLastSelectedRoomIdForSpace(newSpaceId);
|
||||||
@@ -316,6 +325,8 @@ export class RoomListViewModel
|
|||||||
return;
|
return;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
this.updateRoomsMap(this.roomsResult);
|
||||||
|
|
||||||
// Normal room list update (not a space change)
|
// Normal room list update (not a space change)
|
||||||
this.updateRoomListData();
|
this.updateRoomListData();
|
||||||
};
|
};
|
||||||
|
|||||||
@@ -180,6 +180,25 @@ describe("RoomListViewModel", () => {
|
|||||||
expect(disposeSpy1).toHaveBeenCalled();
|
expect(disposeSpy1).toHaveBeenCalled();
|
||||||
expect(disposeSpy2).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", () => {
|
describe("Active room tracking", () => {
|
||||||
@@ -342,6 +361,49 @@ describe("RoomListViewModel", () => {
|
|||||||
}).toThrow();
|
}).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", () => {
|
it("should dispose view models for rooms no longer visible", () => {
|
||||||
viewModel = new RoomListViewModel({ client: matrixClient });
|
viewModel = new RoomListViewModel({ client: matrixClient });
|
||||||
|
|
||||||
|
|||||||
Reference in New Issue
Block a user