Fix joinRoom failing when the roomviewstore state changes. (#34180)
* Fix joinRoom failing when the roomviewstore state changes. * drive by type fix * Help debugging failed dispatches * Use proper types and include roomId in RoomViewStore tests * remove focus * Update apps/web/src/stores/RoomViewStore.tsx Co-authored-by: R Midhun Suresh <hi@midhun.dev> * fmt * Add a test for coverage --------- Co-authored-by: R Midhun Suresh <hi@midhun.dev>
This commit is contained in:
Will Hunt
co-authored by
R Midhun Suresh
R Midhun Suresh
parent
f2cc9353ac
commit
ea38041c87
@@ -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<ActiveRoomChangedPayload>({
|
||||
this.dis.dispatch<ActiveRoomChangedPayload>({
|
||||
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<ViewRoomPayload>({
|
||||
this.dis.dispatch<ViewRoomPayload>({
|
||||
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<JoinRoomPayload>(joinPayload);
|
||||
this.dis.dispatch<JoinRoomPayload>(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<ViewRoomErrorPayload>({
|
||||
this.dis.dispatch<ViewRoomErrorPayload>({
|
||||
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<JoinRoomErrorPayload>({
|
||||
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<JoinRoomReadyPayload>({
|
||||
this.dis.dispatch<JoinRoomReadyPayload>({
|
||||
action: Action.JoinRoomReady,
|
||||
roomId: roomId!,
|
||||
roomId,
|
||||
metricsTrigger: payload.metricsTrigger,
|
||||
});
|
||||
} catch (err) {
|
||||
logger.error("Error thrown while handling joinRoom", err);
|
||||
this.dis?.dispatch<JoinRoomErrorPayload>({
|
||||
this.dis.dispatch<JoinRoomErrorPayload>({
|
||||
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) {
|
||||
|
||||
@@ -31,13 +31,12 @@ export function untilDispatch(
|
||||
timeout = 1000,
|
||||
): Promise<ActionPayload> {
|
||||
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);
|
||||
|
||||
@@ -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<JoinRoomPayload>({
|
||||
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<ViewRoomPayload>({ action: Action.ViewRoom, room_id: roomId, metricsTrigger: "RoomList" });
|
||||
dis.dispatch<JoinRoomPayload>({ action: Action.JoinRoom, roomId: roomId, metricsTrigger: "RoomPreview" });
|
||||
await untilDispatch(Action.JoinRoomReady, dis);
|
||||
expect(mockClient.joinRoom).toHaveBeenCalledWith(roomId, { acceptSharedHistory: true, viaServers: [] });
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user