From 08c47ac47343aeee68eb43b52b303cf839fdb87f Mon Sep 17 00:00:00 2001 From: Michael Telatynski <7t3chguy@gmail.com> Date: Tue, 1 Mar 2022 08:33:29 +0000 Subject: [PATCH] Fix changing space sometimes bouncing to the wrong space (#7910) --- src/stores/room-list/SpaceWatcher.ts | 2 +- src/stores/spaces/SpaceStore.ts | 21 +++++---- test/stores/SpaceStore-test.ts | 64 +++++++++++++++++++++++++++- test/test-utils/test-utils.ts | 1 + 4 files changed, 77 insertions(+), 11 deletions(-) diff --git a/src/stores/room-list/SpaceWatcher.ts b/src/stores/room-list/SpaceWatcher.ts index d9aa032c4b..b0fd3c9478 100644 --- a/src/stores/room-list/SpaceWatcher.ts +++ b/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); }; diff --git a/src/stores/spaces/SpaceStore.ts b/src/stores/spaces/SpaceStore.ts index aca68a2339..6d2703d447 100644 --- a/src/stores/spaces/SpaceStore.ts +++ b/src/stores/spaces/SpaceStore.ts @@ -235,13 +235,9 @@ export class SpaceStoreClass extends AsyncStoreWithClient { 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 { room_id: roomId, context_switch: true, metricsTrigger: "WebSpaceContextSwitch", - }); + }, true); } else if (cliSpace) { defaultDispatcher.dispatch({ action: Action.ViewRoom, room_id: space, context_switch: true, metricsTrigger: "WebSpaceContextSwitch", - }); + }, true); } else { defaultDispatcher.dispatch({ 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); diff --git a/test/stores/SpaceStore-test.ts b/test/stores/SpaceStore-test.ts index c6f834a896..d285f22f0b 100644 --- a/test/stores/SpaceStore-test.ts +++ b/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(); + (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; diff --git a/test/test-utils/test-utils.ts b/test/test-utils/test-utils.ts index 7fb434bf29..b0c7ae3e47 100644 --- a/test/test-utils/test-utils.ts +++ b/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; }