From 00fc34924cffa7f83948f1c2151b050368c46c4e Mon Sep 17 00:00:00 2001 From: Travis Ralston Date: Thu, 9 Jul 2020 11:34:34 -0600 Subject: [PATCH 1/2] Fix rooms disappearing that were created by the user Fixes https://github.com/vector-im/riot-web/issues/14388 We were receiving a read receipt before a room object, leading to the algorithm to assume the room is archived (no membership), which was causing later index issues when the room tried to get moved from archived to untagged. To prevent this, we just ignore nonsensical updates. --- src/stores/room-list/algorithms/Algorithm.ts | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/src/stores/room-list/algorithms/Algorithm.ts b/src/stores/room-list/algorithms/Algorithm.ts index eee8e60b86..f70eb69e91 100644 --- a/src/stores/room-list/algorithms/Algorithm.ts +++ b/src/stores/room-list/algorithms/Algorithm.ts @@ -41,6 +41,17 @@ import { getListAlgorithmInstance } from "./list-ordering"; */ export const LIST_UPDATED_EVENT = "list_updated_event"; +// These are the causes which require a room to be known in order for us to handle them. If +// a cause in this list is raised and we don't know about the room, we don't handle the update. +// +// Note: these typically happen when a new room is coming in, such as the user creating or +// joining the room. For these cases, we need to know about the room prior to handling it otherwise +// we'll make bad assumptions. +const CAUSES_REQUIRING_ROOM = [ + RoomUpdateCause.Timeline, + RoomUpdateCause.ReadReceipt, +]; + interface IStickyRoom { room: Room; position: number; @@ -755,6 +766,11 @@ export class Algorithm extends EventEmitter { } if (!this.roomIdsToTags[room.roomId]) { + if (CAUSES_REQUIRING_ROOM.includes(cause)) { + console.warn(`Skipping tag update for ${room.roomId} because we don't know about the room`); + return false; + } + // TODO: Remove debug: https://github.com/vector-im/riot-web/issues/14035 console.log(`[RoomListDebug] Updating tags for room ${room.roomId} (${room.name})`); From f8e1996e2fb8823efb3808d98a41ec19866a24f1 Mon Sep 17 00:00:00 2001 From: Travis Ralston Date: Thu, 9 Jul 2020 12:08:40 -0600 Subject: [PATCH 2/2] Handle NewRoom and PossibleTagChange updates correctly For new rooms, we need to append to our list of known rooms. For tag changes, we need to be sure to update our cache when the tag can reasonably be assumed to have changed. Fixes https://github.com/vector-im/riot-web/issues/14389 --- src/stores/room-list/algorithms/Algorithm.ts | 21 ++++++++----------- .../list-ordering/ImportanceAlgorithm.ts | 14 ++++--------- .../list-ordering/NaturalAlgorithm.ts | 8 +++++-- .../list-ordering/OrderingAlgorithm.ts | 9 ++++++++ 4 files changed, 28 insertions(+), 24 deletions(-) diff --git a/src/stores/room-list/algorithms/Algorithm.ts b/src/stores/room-list/algorithms/Algorithm.ts index f70eb69e91..35511a461d 100644 --- a/src/stores/room-list/algorithms/Algorithm.ts +++ b/src/stores/room-list/algorithms/Algorithm.ts @@ -677,18 +677,6 @@ export class Algorithm extends EventEmitter { } } - if (hasTags && isForLastSticky && !knownRoomRef) { - // we have a fairly good chance at losing a room right now. Under some circumstances, - // we can end up with a room which transitions references and tag changes, then gets - // lost when the sticky room changes. To counter this, we try and add the room to the - // list manually as the condition below to update the reference will fail. - // - // Other conditions *should* result in the room being sorted into the right place. - console.warn(`${room.roomId} was about to be lost - inserting at end of room list`); - this.rooms.push(room); - knownRoomRef = true; - } - // If we have tags for a room and don't have the room referenced, something went horribly // wrong - the reference should have been updated above. if (hasTags && !knownRoomRef && !isSticky) { @@ -701,6 +689,13 @@ export class Algorithm extends EventEmitter { // to trigger a sticky room update ourselves. this._stickyRoom.room = room; } + + // If after all that we're still a NewRoom update, add the room if applicable. + // We don't do this for the sticky room (because it causes duplication issues) + // or if we know about the reference (as it should be replaced). + if (cause === RoomUpdateCause.NewRoom && !isSticky && !knownRoomRef) { + this.rooms.push(room); + } } if (cause === RoomUpdateCause.PossibleTagChange) { @@ -715,6 +710,7 @@ export class Algorithm extends EventEmitter { const algorithm: OrderingAlgorithm = this.algorithms[rmTag]; if (!algorithm) throw new Error(`No algorithm for ${rmTag}`); await algorithm.handleRoomUpdate(room, RoomUpdateCause.RoomRemoved); + this.cachedRooms[rmTag] = algorithm.orderedRooms; } for (const addTag of diff.added) { // TODO: Remove debug: https://github.com/vector-im/riot-web/issues/14035 @@ -722,6 +718,7 @@ export class Algorithm extends EventEmitter { const algorithm: OrderingAlgorithm = this.algorithms[addTag]; if (!algorithm) throw new Error(`No algorithm for ${addTag}`); await algorithm.handleRoomUpdate(room, RoomUpdateCause.NewRoom); + this.cachedRooms[addTag] = algorithm.orderedRooms; } // Update the tag map so we don't regen it in a moment diff --git a/src/stores/room-list/algorithms/list-ordering/ImportanceAlgorithm.ts b/src/stores/room-list/algorithms/list-ordering/ImportanceAlgorithm.ts index e95f92f985..3acd9f924e 100644 --- a/src/stores/room-list/algorithms/list-ordering/ImportanceAlgorithm.ts +++ b/src/stores/room-list/algorithms/list-ordering/ImportanceAlgorithm.ts @@ -160,7 +160,10 @@ export class ImportanceAlgorithm extends OrderingAlgorithm { this.cachedOrderedRooms.splice(this.indices[category], 0, room); // splice in the new room (pre-adjusted) } else if (cause === RoomUpdateCause.RoomRemoved) { const roomIdx = this.getRoomIndex(room); - if (roomIdx === -1) return false; // no change + if (roomIdx === -1) { + console.warn(`Tried to remove unknown room from ${this.tagId}: ${room.roomId}`); + return false; // no change + } const oldCategory = this.getCategoryFromIndices(roomIdx, this.indices); this.alterCategoryPositionBy(oldCategory, -1, this.indices); this.cachedOrderedRooms.splice(roomIdx, 1); // remove the room @@ -169,15 +172,6 @@ export class ImportanceAlgorithm extends OrderingAlgorithm { } } - private getRoomIndex(room: Room): number { - let roomIdx = this.cachedOrderedRooms.indexOf(room); - if (roomIdx === -1) { // can only happen if the js-sdk's store goes sideways. - console.warn(`Degrading performance to find missing room in "${this.tagId}": ${room.roomId}`); - roomIdx = this.cachedOrderedRooms.findIndex(r => r.roomId === room.roomId); - } - return roomIdx; - } - public async handleRoomUpdate(room: Room, cause: RoomUpdateCause): Promise { try { await this.updateLock.acquireAsync(); diff --git a/src/stores/room-list/algorithms/list-ordering/NaturalAlgorithm.ts b/src/stores/room-list/algorithms/list-ordering/NaturalAlgorithm.ts index f74329cb4d..849c8a2877 100644 --- a/src/stores/room-list/algorithms/list-ordering/NaturalAlgorithm.ts +++ b/src/stores/room-list/algorithms/list-ordering/NaturalAlgorithm.ts @@ -50,8 +50,12 @@ export class NaturalAlgorithm extends OrderingAlgorithm { if (cause === RoomUpdateCause.NewRoom) { this.cachedOrderedRooms.push(room); } else if (cause === RoomUpdateCause.RoomRemoved) { - const idx = this.cachedOrderedRooms.indexOf(room); - if (idx >= 0) this.cachedOrderedRooms.splice(idx, 1); + const idx = this.getRoomIndex(room); + if (idx >= 0) { + this.cachedOrderedRooms.splice(idx, 1); + } else { + console.warn(`Tried to remove unknown room from ${this.tagId}: ${room.roomId}`); + } } // TODO: Optimize this to avoid useless operations: https://github.com/vector-im/riot-web/issues/14035 diff --git a/src/stores/room-list/algorithms/list-ordering/OrderingAlgorithm.ts b/src/stores/room-list/algorithms/list-ordering/OrderingAlgorithm.ts index 4ab7650367..c47a35523c 100644 --- a/src/stores/room-list/algorithms/list-ordering/OrderingAlgorithm.ts +++ b/src/stores/room-list/algorithms/list-ordering/OrderingAlgorithm.ts @@ -70,4 +70,13 @@ export abstract class OrderingAlgorithm { * @returns True if the update requires the Algorithm to update the presentation layers. */ public abstract handleRoomUpdate(room: Room, cause: RoomUpdateCause): Promise; + + protected getRoomIndex(room: Room): number { + let roomIdx = this.cachedOrderedRooms.indexOf(room); + if (roomIdx === -1) { // can only happen if the js-sdk's store goes sideways. + console.warn(`Degrading performance to find missing room in "${this.tagId}": ${room.roomId}`); + roomIdx = this.cachedOrderedRooms.findIndex(r => r.roomId === room.roomId); + } + return roomIdx; + } }