Merge pull request #4905 from matrix-org/travis/room-list/room-safety

Improve room safety in the new room list

Author: turt2live

src/stores/room-list/algorithms/Algorithm.ts

@@ -18,7 +18,7 @@ import { Room } from "matrix-js-sdk/src/models/room";
 import { isNullOrUndefined } from "matrix-js-sdk/src/utils";
 import DMRoomMap from "../../../utils/DMRoomMap";
 import { EventEmitter } from "events";
-import { arrayHasDiff, ArrayUtil } from "../../../utils/arrays";
+import { arrayDiff, arrayHasDiff, ArrayUtil } from "../../../utils/arrays";
 import { getEnumValues } from "../../../utils/enums";
 import { DefaultTagID, RoomUpdateCause, TagID } from "../models";
 import {
@@ -57,6 +57,7 @@ export class Algorithm extends EventEmitter {
     private _cachedStickyRooms: ITagMap = {}; // a clone of the _cachedRooms, with the sticky room
     private filteredRooms: ITagMap = {};
     private _stickyRoom: IStickyRoom = null;
+    private _lastStickyRoom: IStickyRoom = null; // only not-null when changing the sticky room
     private sortAlgorithms: ITagSortingMap;
     private listAlgorithms: IListOrderingMap;
     private algorithms: IOrderingAlgorithmMap;
@@ -162,9 +163,21 @@ export class Algorithm extends EventEmitter {
     }
 
     private async updateStickyRoom(val: Room) {
+        try {
+            return await this.doUpdateStickyRoom(val);
+        } finally {
+            this._lastStickyRoom = null; // clear to indicate we're done changing
+        }
+    }
+
+    private async doUpdateStickyRoom(val: Room) {
         // Note throughout: We need async so we can wait for handleRoomUpdate() to do its thing,
         // otherwise we risk duplicating rooms.
 
+        // Set the last sticky room to indicate that we're in a change. The code throughout the
+        // class can safely handle a null room, so this should be safe to do as a backup.
+        this._lastStickyRoom = this._stickyRoom || <IStickyRoom>{};
+
         // It's possible to have no selected room. In that case, clear the sticky room
         if (!val) {
             if (this._stickyRoom) {
@@ -179,7 +192,7 @@ export class Algorithm extends EventEmitter {
         }
 
         // When we do have a room though, we expect to be able to find it
-        const tag = this.roomIdsToTags[val.roomId][0];
+        let tag = this.roomIdsToTags[val.roomId][0];
         if (!tag) throw new Error(`${val.roomId} does not belong to a tag and cannot be sticky`);
 
         // We specifically do NOT use the ordered rooms set as it contains the sticky room, which
@@ -196,19 +209,41 @@ export class Algorithm extends EventEmitter {
         // the same thing it no-ops. After we're done calling the algorithm, we'll issue
         // a new update for ourselves.
         const lastStickyRoom = this._stickyRoom;
-        this._stickyRoom = null;
+        this._stickyRoom = null; // clear before we update the algorithm
         this.recalculateStickyRoom();
 
         // When we do have the room, re-add the old room (if needed) to the algorithm
         // and remove the sticky room from the algorithm. This is so the underlying
         // algorithm doesn't try and confuse itself with the sticky room concept.
-        if (lastStickyRoom) {
+        // We don't add the new room if the sticky room isn't changing because that's
+        // an easy way to cause duplication. We have to do room ID checks instead of
+        // referential checks as the references can differ through the lifecycle.
+        if (lastStickyRoom && lastStickyRoom.room && lastStickyRoom.room.roomId !== val.roomId) {
             // Lie to the algorithm and re-add the room to the algorithm
             await this.handleRoomUpdate(lastStickyRoom.room, RoomUpdateCause.NewRoom);
         }
         // Lie to the algorithm and remove the room from it's field of view
         await this.handleRoomUpdate(val, RoomUpdateCause.RoomRemoved);
 
+        // Check for tag & position changes while we're here. We also check the room to ensure
+        // it is still the same room.
+        if (this._stickyRoom) {
+            if (this._stickyRoom.room !== val) {
+                // Check the room IDs just in case
+                if (this._stickyRoom.room.roomId === val.roomId) {
+                    console.warn("Sticky room changed references");
+                } else {
+                    throw new Error("Sticky room changed while the sticky room was changing");
+                }
+            }
+
+            console.warn(`Sticky room changed tag & position from ${tag} / ${position} `
+                + `to ${this._stickyRoom.tag} / ${this._stickyRoom.position}`);
+
+            tag = this._stickyRoom.tag;
+            position = this._stickyRoom.position;
+        }
+
         // Now that we're done lying to the algorithm, we need to update our position
         // marker only if the user is moving further down the same list. If they're switching
         // lists, or moving upwards, the position marker will splice in just fine but if
@@ -560,7 +595,7 @@ export class Algorithm extends EventEmitter {
     /**
      * Updates the roomsToTags map
      */
-    protected updateTagsFromCache() {
+    private updateTagsFromCache() {
         const newMap = {};
 
         const tags = Object.keys(this.cachedRooms);
@@ -607,21 +642,94 @@ export class Algorithm extends EventEmitter {
      * processing.
      */
     public async handleRoomUpdate(room: Room, cause: RoomUpdateCause): Promise<boolean> {
+        // TODO: Remove debug: https://github.com/vector-im/riot-web/issues/14035
+        console.log(`Handle room update for ${room.roomId} called with cause ${cause}`);
         if (!this.algorithms) throw new Error("Not ready: no algorithms to determine tags from");
 
+        // Note: check the isSticky against the room ID just in case the reference is wrong
+        const isSticky = this._stickyRoom && this._stickyRoom.room && this._stickyRoom.room.roomId === room.roomId;
         if (cause === RoomUpdateCause.NewRoom) {
+            const isForLastSticky = this._lastStickyRoom && this._lastStickyRoom.room === room;
             const roomTags = this.roomIdsToTags[room.roomId];
-            if (roomTags && roomTags.length > 0) {
+            const hasTags = roomTags && roomTags.length > 0;
+
+            // Don't change the cause if the last sticky room is being re-added. If we fail to
+            // pass the cause through as NewRoom, we'll fail to lie to the algorithm and thus
+            // lose the room.
+            if (hasTags && !isForLastSticky) {
                 console.warn(`${room.roomId} is reportedly new but is already known - assuming TagChange instead`);
                 cause = RoomUpdateCause.PossibleTagChange;
             }
+
+            // If we have tags for a room and don't have the room referenced, the room reference
+            // probably changed. We need to swap out the problematic reference.
+            if (hasTags && !this.rooms.includes(room) && !isSticky) {
+                console.warn(`${room.roomId} is missing from room array but is known - trying to find duplicate`);
+                this.rooms = this.rooms.map(r => r.roomId === room.roomId ? room : r);
+
+                // Sanity check
+                if (!this.rooms.includes(room)) {
+                    throw new Error(`Failed to replace ${room.roomId} with an updated reference`);
+                }
+            }
+
+            // Like above, update the reference to the sticky room if we need to
+            if (hasTags && isSticky) {
+                // Go directly in and set the sticky room's new reference, being careful not
+                // to trigger a sticky room update ourselves.
+                this._stickyRoom.room = room;
+            }
         }
 
         if (cause === RoomUpdateCause.PossibleTagChange) {
-            // TODO: Be smarter and splice rather than regen the planet. https://github.com/vector-im/riot-web/issues/14035
-            // TODO: No-op if no change. https://github.com/vector-im/riot-web/issues/14035
-            await this.setKnownRooms(this.rooms);
-            return true;
+            let didTagChange = false;
+            const oldTags = this.roomIdsToTags[room.roomId] || [];
+            const newTags = this.getTagsForRoom(room);
+            const diff = arrayDiff(oldTags, newTags);
+            if (diff.removed.length > 0 || diff.added.length > 0) {
+                for (const rmTag of diff.removed) {
+                    // TODO: Remove debug: https://github.com/vector-im/riot-web/issues/14035
+                    console.log(`Removing ${room.roomId} from ${rmTag}`);
+                    const algorithm: OrderingAlgorithm = this.algorithms[rmTag];
+                    if (!algorithm) throw new Error(`No algorithm for ${rmTag}`);
+                    await algorithm.handleRoomUpdate(room, RoomUpdateCause.RoomRemoved);
+                }
+                for (const addTag of diff.added) {
+                    // TODO: Remove debug: https://github.com/vector-im/riot-web/issues/14035
+                    console.log(`Adding ${room.roomId} to ${addTag}`);
+                    const algorithm: OrderingAlgorithm = this.algorithms[addTag];
+                    if (!algorithm) throw new Error(`No algorithm for ${addTag}`);
+                    await algorithm.handleRoomUpdate(room, RoomUpdateCause.NewRoom);
+                }
+
+                // Update the tag map so we don't regen it in a moment
+                this.roomIdsToTags[room.roomId] = newTags;
+
+                // TODO: Remove debug: https://github.com/vector-im/riot-web/issues/14035
+                console.log(`Changing update cause for ${room.roomId} to Timeline to sort rooms`);
+                cause = RoomUpdateCause.Timeline;
+                didTagChange = true;
+            } else {
+                // TODO: Remove debug: https://github.com/vector-im/riot-web/issues/14035
+                console.warn(`Received no-op update for ${room.roomId} - changing to Timeline update`);
+                cause = RoomUpdateCause.Timeline;
+            }
+
+            if (didTagChange && isSticky) {
+                // Manually update the tag for the sticky room without triggering a sticky room
+                // update. The update will be handled implicitly by the sticky room handling and
+                // requires no changes on our part, if we're in the middle of a sticky room change.
+                if (this._lastStickyRoom) {
+                    this._stickyRoom = {
+                        room,
+                        tag: this.roomIdsToTags[room.roomId][0],
+                        position: 0, // right at the top as it changed tags
+                    };
+                } else {
+                    // We have to clear the lock as the sticky room change will trigger updates.
+                    await this.setStickyRoomAsync(room);
+                }
+            }
         }
 
         // If the update is for a room change which might be the sticky room, prevent it. We
@@ -635,8 +743,9 @@ export class Algorithm extends EventEmitter {
             }
         }
 
-        if (cause === RoomUpdateCause.NewRoom && !this.roomIdsToTags[room.roomId]) {
-            console.log(`[RoomListDebug] Updating tags for new room ${room.roomId} (${room.name})`);
+        if (!this.roomIdsToTags[room.roomId]) {
+            // TODO: Remove debug: https://github.com/vector-im/riot-web/issues/14035
+            console.log(`[RoomListDebug] Updating tags for room ${room.roomId} (${room.name})`);
 
             // Get the tags for the room and populate the cache
             const roomTags = this.getTagsForRoom(room).filter(t => !isNullOrUndefined(this.cachedRooms[t]));
@@ -646,9 +755,15 @@ export class Algorithm extends EventEmitter {
             if (!roomTags.length) throw new Error(`Tags cannot be determined for ${room.roomId}`);
 
             this.roomIdsToTags[room.roomId] = roomTags;
+
+            // TODO: Remove debug: https://github.com/vector-im/riot-web/issues/14035
+            console.log(`[RoomListDebug] Updated tags for ${room.roomId}:`, roomTags);
         }
 
-        let tags = this.roomIdsToTags[room.roomId];
+        // TODO: Remove debug: https://github.com/vector-im/riot-web/issues/14035
+        console.log(`[RoomListDebug] Reached algorithmic handling for ${room.roomId} and cause ${cause}`);
+
+        const tags = this.roomIdsToTags[room.roomId];
         if (!tags) {
             console.warn(`No tags known for "${room.name}" (${room.roomId})`);
             return false;
@@ -668,6 +783,8 @@ export class Algorithm extends EventEmitter {
             changed = true;
         }
 
-        return true;
+        // TODO: Remove debug: https://github.com/vector-im/riot-web/issues/14035
+        console.log(`[RoomListDebug] Finished handling ${room.roomId} with cause ${cause} (changed=${changed})`);
+        return changed;
     }
 }