Fix changing space sometimes bouncing to the wrong space (#7910)

Author: t3chguy

src/stores/room-list/SpaceWatcher.ts

@@ -69,7 +69,7 @@ export class SpaceWatcher {
         if (!isMetaSpace(this.activeSpace)) {
             SpaceStore.instance.traverseSpace(this.activeSpace, roomId => {
                 this.store.matrixClient?.getRoom(roomId)?.loadMembersIfNeeded();
-            });
+            }, false);
         }
         this.filter.updateSpace(this.activeSpace);
     };

src/stores/spaces/SpaceStore.ts

@@ -235,13 +235,9 @@ export class SpaceStoreClass extends AsyncStoreWithClient<IState> {
             return;
         }
 
-        this._activeSpace = space;
-        this.emit(UPDATE_SELECTED_SPACE, this.activeSpace);
-        this.emit(UPDATE_SUGGESTED_ROOMS, this._suggestedRooms = []);
-
         if (contextSwitch) {
             // view last selected room from space
-            const roomId = window.localStorage.getItem(getSpaceContextKey(this.activeSpace));
+            const roomId = window.localStorage.getItem(getSpaceContextKey(space));
 
             // if the space being selected is an invite then always view that invite
             // else if the last viewed room in this space is joined then view that
@@ -255,22 +251,31 @@ export class SpaceStoreClass extends AsyncStoreWithClient<IState> {
                     room_id: roomId,
                     context_switch: true,
                     metricsTrigger: "WebSpaceContextSwitch",
-                });
+                }, true);
             } else if (cliSpace) {
                 defaultDispatcher.dispatch<ViewRoomPayload>({
                     action: Action.ViewRoom,
                     room_id: space,
                     context_switch: true,
                     metricsTrigger: "WebSpaceContextSwitch",
-                });
+                }, true);
             } else {
                 defaultDispatcher.dispatch<ViewHomePagePayload>({
                     action: Action.ViewHomePage,
                     context_switch: true,
-                });
+                }, true);
             }
         }
 
+        // We can set the space after context switching as the dispatch handler which stores the last viewed room
+        // specifically no-ops on context_switch=true.
+        this._activeSpace = space;
+        // Emit after a synchronous dispatch for context switching to prevent racing with SpaceWatcher calling
+        // Room::loadMembersIfNeeded which could (via onMemberUpdate) call upon switchSpaceIfNeeded causing the
+        // space to wrongly bounce.
+        this.emit(UPDATE_SELECTED_SPACE, this.activeSpace);
+        this.emit(UPDATE_SUGGESTED_ROOMS, this._suggestedRooms = []);
+
         // persist space selected
         window.localStorage.setItem(ACTIVE_SPACE_LS_KEY, space);
 

test/stores/SpaceStore-test.ts

@@ -16,9 +16,10 @@ limitations under the License.
 
 import { EventType } from "matrix-js-sdk/src/@types/event";
 import { RoomMember } from "matrix-js-sdk/src/models/room-member";
+import { RoomStateEvent } from "matrix-js-sdk/src/models/room-state";
+import { defer } from "matrix-js-sdk/src/utils";
 
 import "../skinned-sdk"; // Must be first for skinning to work
-
 import SpaceStore from "../../src/stores/spaces/SpaceStore";
 import {
     MetaSpace,
@@ -30,11 +31,11 @@ import {
 import * as testUtils from "../test-utils";
 import { mkEvent, stubClient } from "../test-utils";
 import DMRoomMap from "../../src/utils/DMRoomMap";
-import { MatrixClientPeg } from "../../src/MatrixClientPeg";
 import defaultDispatcher from "../../src/dispatcher/dispatcher";
 import SettingsStore from "../../src/settings/SettingsStore";
 import { SettingLevel } from "../../src/settings/SettingLevel";
 import { Action } from "../../src/dispatcher/actions";
+import { MatrixClientPeg } from "../../src/MatrixClientPeg";
 
 jest.useFakeTimers();
 
@@ -92,6 +93,8 @@ describe("SpaceStore", () => {
     const store = SpaceStore.instance;
     const client = MatrixClientPeg.get();
 
+    const spyDispatcher = jest.spyOn(defaultDispatcher, "dispatch");
+
     let rooms = [];
     const mkRoom = (roomId: string) => testUtils.mkRoom(client, roomId, rooms);
     const mkSpace = (spaceId: string, children: string[] = []) => testUtils.mkSpace(client, spaceId, rooms, children);
@@ -122,6 +125,8 @@ describe("SpaceStore", () => {
             [MetaSpace.People]: true,
             [MetaSpace.Orphans]: true,
         });
+
+        spyDispatcher.mockClear();
     });
 
     afterEach(async () => {
@@ -842,6 +847,61 @@ describe("SpaceStore", () => {
         });
     });
 
+    it("does not race with lazy loading", async () => {
+        store.setActiveSpace(MetaSpace.Home);
+
+        mkRoom(room1);
+        const space = mkSpace(space1, [room1]);
+        // seed the context for space1 to be room1
+        window.localStorage.setItem(`mx_space_context_${space1}`, room1);
+
+        await run();
+
+        const deferred = defer<void>();
+        (space.loadMembersIfNeeded as jest.Mock).mockImplementation(() => {
+            const event = mkEvent({
+                event: true,
+                type: EventType.RoomMember,
+                content: { membership: "join" },
+                skey: dm1Partner.userId,
+                user: dm1Partner.userId,
+                room: space1,
+            });
+
+            client.emit(RoomStateEvent.Members, event, null, null);
+            deferred.resolve();
+        });
+
+        spyDispatcher.mockClear();
+        const getCurrentRoom = () => {
+            for (let i = spyDispatcher.mock.calls.length - 1; i >= 0; i--) {
+                if (spyDispatcher.mock.calls[i][0].action === Action.ViewRoom) {
+                    return spyDispatcher.mock.calls[i][0]["room_id"];
+                }
+            }
+        };
+
+        // set up space with LL where loadMembersIfNeeded emits membership events which trip switchSpaceIfNeeded
+        expect(space.loadMembersIfNeeded).not.toHaveBeenCalled();
+
+        store.setActiveSpace(space1, true);
+        // traverse the space and call loadMembersIfNeeded, similarly to SpaceWatcher's behaviour
+        store.traverseSpace(space1, roomId => {
+            client.getRoom(roomId)?.loadMembersIfNeeded();
+        }, false);
+
+        expect(store.activeSpace).toBe(space1);
+        expect(getCurrentRoom()).toBe(room1);
+
+        await deferred.promise;
+        expect(store.activeSpace).toBe(space1);
+        expect(getCurrentRoom()).toBe(room1);
+
+        jest.runAllTimers();
+        expect(store.activeSpace).toBe(space1);
+        expect(getCurrentRoom()).toBe(room1);
+    });
+
     describe("context switching tests", () => {
         let dispatcherRef;
         let currentRoom = null;

test/test-utils/test-utils.ts

@@ -349,6 +349,7 @@ export function mkStubRoom(roomId = null, name: string, client: MatrixClient): R
         getAltAliases: jest.fn().mockReturnValue([]),
         timeline: [],
         getJoinRule: jest.fn().mockReturnValue("invite"),
+        loadMembersIfNeeded: jest.fn(),
         client,
     } as unknown as Room;
 }