diff --git a/apps/web/src/stores/RoomViewStore.tsx b/apps/web/src/stores/RoomViewStore.tsx index 407213d4d0..b2ff60fe09 100644 --- a/apps/web/src/stores/RoomViewStore.tsx +++ b/apps/web/src/stores/RoomViewStore.tsx @@ -148,7 +148,8 @@ export class RoomViewStore extends EventEmitter { // another RVS via INITIAL_STATE as they share the same underlying object. Mostly relevant for tests. private state = utils.deepCopy(INITIAL_STATE); - private dis?: MatrixDispatcher; + // this is defacto always assigned as `resetDispatcher` is called in the constructor. + private dis!: MatrixDispatcher; private dispatchToken?: string; public constructor( @@ -195,7 +196,7 @@ export class RoomViewStore extends EventEmitter { // Fired so we can reduce dependency on event emitters to this store, which is relatively // central to the application and can easily cause import cycles. - this.dis?.dispatch({ + this.dis.dispatch({ action: Action.ActiveRoomChanged, oldRoomId: lastRoomId, newRoomId: this.state.roomId, @@ -302,7 +303,7 @@ export class RoomViewStore extends EventEmitter { // if the room is displayed in a module, we don't want to change the room view if (roomId && this.isRoomDisplayedInModule(roomId)) return; - this.dis?.dispatch({ + this.dis.dispatch({ action: Action.ViewRoom, room_id: payload.event.getRoomId(), replyingToEvent: payload.event, @@ -415,7 +416,7 @@ export class RoomViewStore extends EventEmitter { await this.stores.slidingSyncManager.setRoomVisible(payload.room_id); // Re-fire the payload: we won't re-process it because the prev room ID == payload room ID now - this.dis?.dispatch({ + this.dis.dispatch({ ...payload, }); return; @@ -466,7 +467,7 @@ export class RoomViewStore extends EventEmitter { viaServers: payload.via_servers, }; } - this.dis?.dispatch(joinPayload); + this.dis.dispatch(joinPayload); } if (room) { @@ -505,7 +506,7 @@ export class RoomViewStore extends EventEmitter { viaServers = result.servers; } catch (err) { logger.error("RVS failed to get room id for alias: ", err); - this.dis?.dispatch({ + this.dis.dispatch({ action: Action.ViewRoomError, room_id: null, room_alias: payload.room_alias, @@ -516,7 +517,7 @@ export class RoomViewStore extends EventEmitter { } // Re-fire the payload with the newly found room_id - this.dis?.dispatch({ + this.dis.dispatch({ ...payload, room_id: roomId, via_servers: viaServers, @@ -545,9 +546,24 @@ export class RoomViewStore extends EventEmitter { }); // take a copy of roomAlias, roomId & viaServers as they may change by the time the join is complete - const { roomAlias, roomId = payload.roomId, viaServers = [] } = this.state; + const { roomAlias, viaServers = [] } = this.state; + // fall back to the payload's roomId explicitly since it is always the room we were asked to join + const roomId = this.state.roomId ?? payload.roomId; // prefer the room alias if we have one as it allows joining over federation even with no viaServers - const address = roomAlias || roomId!; + const address = roomAlias || roomId; + + if (!address) { + logger.error("Cannot join room: no room ID or alias to join", payload); + this.dis.dispatch({ + action: Action.JoinRoomError, + roomId, + err: new UserFriendlyError("room|error_join_unknown", { + cause: new Error("Cannot join room: no room ID or alias to join"), + }), + canAskToJoin: payload.canAskToJoin, + }); + return; + } const joinOpts: IJoinRoomOpts = { viaServers, @@ -568,14 +584,14 @@ export class RoomViewStore extends EventEmitter { // We do *not* clear the 'joining' flag because the Room object and/or our 'joined' member event may not // have come down the sync stream yet, and that's the point at which we'd consider the user joined to the // room. - this.dis?.dispatch({ + this.dis.dispatch({ action: Action.JoinRoomReady, - roomId: roomId!, + roomId, metricsTrigger: payload.metricsTrigger, }); } catch (err) { logger.error("Error thrown while handling joinRoom", err); - this.dis?.dispatch({ + this.dis.dispatch({ action: Action.JoinRoomError, roomId, err: err instanceof Error ? err : new UserFriendlyError("room|error_join_unknown", { cause: err }), @@ -583,7 +599,7 @@ export class RoomViewStore extends EventEmitter { }); if (payload.canAskToJoin && err instanceof MatrixError && err.httpStatus === 403) { - this.dis?.dispatch({ action: Action.PromptAskToJoin }); + this.dis.dispatch({ action: Action.PromptAskToJoin }); } } } @@ -665,7 +681,7 @@ export class RoomViewStore extends EventEmitter { */ public resetDispatcher(dis: MatrixDispatcher): void { if (this.dispatchToken) { - this.dis?.unregister(this.dispatchToken); + this.dis.unregister(this.dispatchToken); } this.dis = dis; if (dis) { diff --git a/apps/web/test/test-utils/utilities.ts b/apps/web/test/test-utils/utilities.ts index 6f9f1e68bb..15b0b9807f 100644 --- a/apps/web/test/test-utils/utilities.ts +++ b/apps/web/test/test-utils/utilities.ts @@ -31,13 +31,12 @@ export function untilDispatch( timeout = 1000, ): Promise { const callerLine = new Error().stack!.toString().split("\n")[2]; - if (typeof waitForAction === "string") { - const action = waitForAction; - waitForAction = (payload) => { - return payload.action === action; - }; - } - const callback = waitForAction as (payload: ActionPayload) => boolean; + const callback = + typeof waitForAction === "string" + ? (payload: ActionPayload) => { + return payload.action === waitForAction; + } + : waitForAction; return new Promise((resolve, reject) => { let fulfilled = false; let timeoutId: number; @@ -45,7 +44,11 @@ export function untilDispatch( if (timeout > 0) { timeoutId = window.setTimeout(() => { if (!fulfilled) { - reject(new Error(`untilDispatch: timed out at ${callerLine}`)); + reject( + new Error( + `untilDispatch: timed out (waiting for: ${typeof waitForAction === "function" ? "fn" : waitForAction}) at ${callerLine}`, + ), + ); fulfilled = true; } }, timeout); diff --git a/apps/web/test/unit-tests/stores/RoomViewStore-test.ts b/apps/web/test/unit-tests/stores/RoomViewStore-test.ts index d741fe3a21..95bf2e72fb 100644 --- a/apps/web/test/unit-tests/stores/RoomViewStore-test.ts +++ b/apps/web/test/unit-tests/stores/RoomViewStore-test.ts @@ -51,6 +51,7 @@ import { storeRoomAliasInCache } from "../../../src/RoomAliasCache.ts"; import { type Call, ConnectionState } from "../../../src/models/Call.ts"; import ActiveWidgetStore from "../../../src/stores/ActiveWidgetStore"; import { ModuleApi } from "../../../src/modules/Api"; +import { type JoinRoomPayload } from "../../../src/dispatcher/payloads/JoinRoomPayload.ts"; jest.mock("../../../src/Modal"); @@ -486,7 +487,6 @@ describe("RoomViewStore", function () { }); it("should display an error message when the room is unreachable via the roomId", async () => { - // When // View and wait for the room dis.dispatch({ action: Action.ViewRoom, room_id: roomId }); await untilDispatch(Action.ActiveRoomChanged, dis); @@ -497,7 +497,6 @@ describe("RoomViewStore", function () { // Check the modal props expect(mocked(Modal).createDialog.mock.calls[0][1]).toMatchSnapshot(); }); - // The server bob is on will affect the message we send. it.each(["server", "another-server"])( "should display an invite-specific error message when the room is unreachable", @@ -523,6 +522,12 @@ describe("RoomViewStore", function () { }, ); + it("should display an error message when the provided room is invalid", async () => { + dis.dispatch({ action: Action.JoinRoom, room_id: "" }); + const result = await untilDispatch(Action.JoinRoomError, dis); + expect(result.err.cause.message).toEqual("Cannot join room: no room ID or alias to join"); + }); + it("should display the generic error message when the roomId doesnt match", async () => { // When // Generate error to display the expected error message @@ -590,22 +595,34 @@ describe("RoomViewStore", function () { jest.spyOn(dis, "dispatch"); jest.spyOn(mockClient, "joinRoom").mockRejectedValueOnce(err); - dis.dispatch({ action: Action.JoinRoom, canAskToJoin: true }); + const roomId = "!hello:world"; + + dis.dispatch({ + action: Action.JoinRoom, + canAskToJoin: true, + roomId, + metricsTrigger: "RoomPreview", + }); await untilDispatch(Action.PromptAskToJoin, dis); - expect(mocked(dis.dispatch).mock.calls[0][0]).toEqual({ action: "join_room", canAskToJoin: true }); + expect(mocked(dis.dispatch).mock.calls[0][0]).toEqual({ + action: Action.JoinRoom, + canAskToJoin: true, + metricsTrigger: "RoomPreview", + roomId, + }); expect(mocked(dis.dispatch).mock.calls[1][0]).toEqual({ - action: "join_room_error", - roomId: null, + action: Action.JoinRoomError, + roomId, err, canAskToJoin: true, }); - expect(mocked(dis.dispatch).mock.calls[2][0]).toEqual({ action: "prompt_ask_to_join" }); + expect(mocked(dis.dispatch).mock.calls[2][0]).toEqual({ action: Action.PromptAskToJoin }); }); it("sets 'acceptSharedHistory'", async () => { - dis.dispatch({ action: Action.ViewRoom, room_id: roomId }); - dis.dispatch({ action: Action.JoinRoom }); + dis.dispatch({ action: Action.ViewRoom, room_id: roomId, metricsTrigger: "RoomList" }); + dis.dispatch({ action: Action.JoinRoom, roomId: roomId, metricsTrigger: "RoomPreview" }); await untilDispatch(Action.JoinRoomReady, dis); expect(mockClient.joinRoom).toHaveBeenCalledWith(roomId, { acceptSharedHistory: true, viaServers: [] }); });