Merge pull request #4940 from matrix-org/travis/room-list/sticky-rooms

Improve safety of new rooms in the room list

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;
@@ -666,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) {
@@ -690,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) {
@@ -704,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
@@ -711,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
@@ -755,6 +763,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})`);
 

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<boolean> {
         try {
             await this.updateLock.acquireAsync();

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);
+                const idx = this.getRoomIndex(room);
-                if (idx >= 0) this.cachedOrderedRooms.splice(idx, 1);
+                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

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<boolean>;
+
+    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;
+    }
 }