From 8e94efb14aa4ad272422a5265a96bf7e12ce8422 Mon Sep 17 00:00:00 2001 From: Arnei Date: Fri, 28 Oct 2022 15:19:14 +0200 Subject: [PATCH 1/2] Changed ListNotificationState to store room ids ListNotificationState stores a reference to a rooms array which is later used for comparing the stored array with new arrays. However, the comparison may fail since the stored array can be changed outside the class. This PR proposes to instead store only the room ids, which hopefully allows to avoid the issue by copying the room ids into a new array, while still being performant. Signed-off-by: Arne Wilken arnepokemon@yahoo.de --- .../notifications/ListNotificationState.ts | 33 ++++++++++++------- 1 file changed, 21 insertions(+), 12 deletions(-) diff --git a/src/stores/notifications/ListNotificationState.ts b/src/stores/notifications/ListNotificationState.ts index 56af3be178d..8a28b9aa867 100644 --- a/src/stores/notifications/ListNotificationState.ts +++ b/src/stores/notifications/ListNotificationState.ts @@ -20,11 +20,15 @@ import { NotificationColor } from "./NotificationColor"; import { arrayDiff } from "../../utils/arrays"; import { RoomNotificationState } from "./RoomNotificationState"; import { NotificationState, NotificationStateEvents } from "./NotificationState"; +import { MatrixClientPeg } from "../../MatrixClientPeg"; export type FetchRoomFn = (room: Room) => RoomNotificationState; export class ListNotificationState extends NotificationState { - private rooms: Room[] = []; + // Only store room ids instead of hole rooms. The only time we need more than the room + // ids is when calling the FetchRoomFn. It also allows to performantly copy a given rooms array + // instead of storing a reference to it, which avoids potential side effects. + private roomIds: string[] = []; private states: { [roomId: string]: RoomNotificationState } = {}; constructor(private byTileCount = false, private getRoomFn: FetchRoomFn) { @@ -38,24 +42,25 @@ export class ListNotificationState extends NotificationState { public setRooms(rooms: Room[]) { // If we're only concerned about the tile count, don't bother setting up listeners. if (this.byTileCount) { - this.rooms = rooms; + this.roomIds = this.getRoomIdsFromRooms(rooms); this.calculateTotalState(); return; } - const oldRooms = this.rooms; - const diff = arrayDiff(oldRooms, rooms); - this.rooms = rooms; - for (const oldRoom of diff.removed) { - const state = this.states[oldRoom.roomId]; + const oldRooms = this.roomIds; + const diff = arrayDiff(oldRooms, this.getRoomIdsFromRooms(rooms)); + this.roomIds = this.getRoomIdsFromRooms(rooms); + for (const oldRoomId of diff.removed) { + const state = this.states[oldRoomId]; if (!state) continue; // We likely just didn't have a badge (race condition) - delete this.states[oldRoom.roomId]; + delete this.states[oldRoomId]; state.off(NotificationStateEvents.Update, this.onRoomNotificationStateUpdate); } - for (const newRoom of diff.added) { - const state = this.getRoomFn(newRoom); + for (const newRoomId of diff.added) { + const client = MatrixClientPeg.get(); + const state = this.getRoomFn(client.getRoom(newRoomId)); state.on(NotificationStateEvents.Update, this.onRoomNotificationStateUpdate); - this.states[newRoom.roomId] = state; + this.states[newRoomId] = state; } this.calculateTotalState(); @@ -84,7 +89,7 @@ export class ListNotificationState extends NotificationState { if (this.byTileCount) { this._color = NotificationColor.Red; - this._count = this.rooms.length; + this._count = this.roomIds.length; } else { this._count = 0; this._color = NotificationColor.None; @@ -97,5 +102,9 @@ export class ListNotificationState extends NotificationState { // finally, publish an update if needed this.emitIfUpdated(snapshot); } + + private getRoomIdsFromRooms = (rooms: Room[]) => { + return rooms.map((room: Room) => room.roomId); + }; } From 8737a20eb488a6c1c653518e93cfcd68f2963e55 Mon Sep 17 00:00:00 2001 From: Arnei Date: Tue, 1 Nov 2022 09:45:52 +0100 Subject: [PATCH 2/2] Change ListNotificationState to shallow clone rooms Instead of using room ids like in the previous commit, shallow clone the rooms array instead. --- .../notifications/ListNotificationState.ts | 33 +++++++------------ 1 file changed, 12 insertions(+), 21 deletions(-) diff --git a/src/stores/notifications/ListNotificationState.ts b/src/stores/notifications/ListNotificationState.ts index 8a28b9aa867..8ff1824bd61 100644 --- a/src/stores/notifications/ListNotificationState.ts +++ b/src/stores/notifications/ListNotificationState.ts @@ -20,15 +20,11 @@ import { NotificationColor } from "./NotificationColor"; import { arrayDiff } from "../../utils/arrays"; import { RoomNotificationState } from "./RoomNotificationState"; import { NotificationState, NotificationStateEvents } from "./NotificationState"; -import { MatrixClientPeg } from "../../MatrixClientPeg"; export type FetchRoomFn = (room: Room) => RoomNotificationState; export class ListNotificationState extends NotificationState { - // Only store room ids instead of hole rooms. The only time we need more than the room - // ids is when calling the FetchRoomFn. It also allows to performantly copy a given rooms array - // instead of storing a reference to it, which avoids potential side effects. - private roomIds: string[] = []; + private rooms: Room[] = []; private states: { [roomId: string]: RoomNotificationState } = {}; constructor(private byTileCount = false, private getRoomFn: FetchRoomFn) { @@ -42,25 +38,24 @@ export class ListNotificationState extends NotificationState { public setRooms(rooms: Room[]) { // If we're only concerned about the tile count, don't bother setting up listeners. if (this.byTileCount) { - this.roomIds = this.getRoomIdsFromRooms(rooms); + this.rooms = rooms; this.calculateTotalState(); return; } - const oldRooms = this.roomIds; - const diff = arrayDiff(oldRooms, this.getRoomIdsFromRooms(rooms)); - this.roomIds = this.getRoomIdsFromRooms(rooms); - for (const oldRoomId of diff.removed) { - const state = this.states[oldRoomId]; + const oldRooms = this.rooms; + const diff = arrayDiff(oldRooms, rooms); + this.rooms = [...rooms]; + for (const oldRoom of diff.removed) { + const state = this.states[oldRoom.roomId]; if (!state) continue; // We likely just didn't have a badge (race condition) - delete this.states[oldRoomId]; + delete this.states[oldRoom.roomId]; state.off(NotificationStateEvents.Update, this.onRoomNotificationStateUpdate); } - for (const newRoomId of diff.added) { - const client = MatrixClientPeg.get(); - const state = this.getRoomFn(client.getRoom(newRoomId)); + for (const newRoom of diff.added) { + const state = this.getRoomFn(newRoom); state.on(NotificationStateEvents.Update, this.onRoomNotificationStateUpdate); - this.states[newRoomId] = state; + this.states[newRoom.roomId] = state; } this.calculateTotalState(); @@ -89,7 +84,7 @@ export class ListNotificationState extends NotificationState { if (this.byTileCount) { this._color = NotificationColor.Red; - this._count = this.roomIds.length; + this._count = this.rooms.length; } else { this._count = 0; this._color = NotificationColor.None; @@ -102,9 +97,5 @@ export class ListNotificationState extends NotificationState { // finally, publish an update if needed this.emitIfUpdated(snapshot); } - - private getRoomIdsFromRooms = (rooms: Room[]) => { - return rooms.map((room: Room) => room.roomId); - }; }