Skip to content
This repository has been archived by the owner on Sep 11, 2024. It is now read-only.

Commit

Permalink
Fix closing all modals
Browse files Browse the repository at this point in the history
We used `Modal.closeCurrentModal()` in a bunch of places, in all cases
(as far as I can see: it wasn't commented) we meant to close all open
modals. This swaps that function for one that closes all open modals.

Also types the close reason which claimed to be something in a comment,
of course, was wrong because a load of places passed their own random
string which was never used.
  • Loading branch information
dbkr committed Jul 4, 2024
1 parent de12d69 commit d208bc6
Show file tree
Hide file tree
Showing 6 changed files with 33 additions and 40 deletions.
22 changes: 9 additions & 13 deletions src/Modal.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -47,8 +47,8 @@ export interface IModal<C extends ComponentType> {
elem: React.ReactNode;
className?: string;
beforeClosePromise?: Promise<boolean>;
closeReason?: string;
onBeforeClose?(reason?: string): Promise<boolean>;
closeReason?: ModalCloseReason;
onBeforeClose?(reason?: ModalCloseReason): Promise<boolean>;
onFinished: ComponentProps<C>["onFinished"];
close(...args: Parameters<ComponentProps<C>["onFinished"]>): void;
hidden?: boolean;
Expand All @@ -73,6 +73,8 @@ type HandlerMap = {
[ModalManagerEvent.Closed]: () => void;
};

type ModalCloseReason = "backgroundClick";

export class ModalManager extends TypedEventEmitter<ModalManagerEvent, HandlerMap> {
private counter = 0;
// The modal to prioritise over all others. If this is set, only show
Expand Down Expand Up @@ -147,18 +149,12 @@ export class ModalManager extends TypedEventEmitter<ModalManagerEvent, HandlerMa
return this.appendDialogAsync<C>(Promise.resolve(Element), props, className);
}

/**
* @param reason either "backgroundClick" or undefined
* @return whether a modal was closed
*/
public closeCurrentModal(reason?: string): boolean {
const modal = this.getCurrentModal();
if (!modal) {
return false;
public closeAllModals(reason?: ModalCloseReason): void {
let modal: IModal<any>;
while ((modal = this.getCurrentModal())) {
modal.closeReason = reason;
modal.close();
}
modal.closeReason = reason;
modal.close();
return true;
}

private buildModal<C extends ComponentType>(
Expand Down
2 changes: 1 addition & 1 deletion src/components/structures/LoggedInView.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -491,7 +491,7 @@ class LoggedInView extends React.Component<IProps, IState> {
dis.dispatch({
action: Action.ViewHomePage,
});
Modal.closeCurrentModal("homeKeyboardShortcut");
Modal.closeAllModals();
handled = true;
break;
case KeyBindingAction.ToggleSpacePanel:
Expand Down
2 changes: 1 addition & 1 deletion src/components/structures/MatrixChat.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -1544,7 +1544,7 @@ export default class MatrixChat extends React.PureComponent<IProps, IState> {
if (Lifecycle.isLoggingOut()) return;

// A modal might have been open when we were logged out by the server
Modal.closeCurrentModal("Session.logged_out");
Modal.closeAllModals();

if (errObj.httpStatus === 401 && errObj.data && errObj.data["soft_logout"]) {
logger.warn("Soft logout issued by server - avoiding data deletion");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ import { mkMessage, stubClient } from "../../../test-utils/test-utils";
import { shouldShowComponent } from "../../../../src/customisations/helpers/UIComponents";
import { UIComponent } from "../../../../src/settings/UIFeature";
import SettingsStore from "../../../../src/settings/SettingsStore";
import Modal from "../../../../src/Modal";
import { clearAllModals } from "../../../test-utils";

jest.mock("../../../../src/customisations/helpers/UIComponents", () => ({
shouldShowComponent: jest.fn(),
Expand Down Expand Up @@ -90,8 +90,8 @@ describe("RoomGeneralContextMenu", () => {
onFinished = jest.fn();
});

afterEach(() => {
Modal.closeCurrentModal("force");
afterEach(async () => {
await clearAllModals();
});

it("renders an empty context menu for archived rooms", async () => {
Expand Down
6 changes: 3 additions & 3 deletions test/components/views/dialogs/InviteDialog-test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import { mocked, Mocked } from "jest-mock";
import InviteDialog from "../../../../src/components/views/dialogs/InviteDialog";
import { InviteKind } from "../../../../src/components/views/dialogs/InviteDialogTypes";
import {
clearAllModals,
filterConsole,
flushPromises,
getMockClientWithEventEmitter,
Expand All @@ -40,7 +41,6 @@ import { SdkContextClass } from "../../../../src/contexts/SDKContext";
import { IProfileInfo } from "../../../../src/hooks/useProfileInfo";
import { DirectoryMember, startDmOnFirstMessage } from "../../../../src/utils/direct-messages";
import SettingsStore from "../../../../src/settings/SettingsStore";
import Modal from "../../../../src/Modal";

const mockGetAccessToken = jest.fn().mockResolvedValue("getAccessToken");
jest.mock("../../../../src/IdentityAuthClient", () =>
Expand Down Expand Up @@ -178,8 +178,8 @@ describe("InviteDialog", () => {
SdkContextClass.instance.client = mockClient;
});

afterEach(() => {
Modal.closeCurrentModal();
afterEach(async () => {
await clearAllModals();
SdkContextClass.instance.onLoggedOut();
SdkContextClass.instance.client = undefined;
});
Expand Down
35 changes: 16 additions & 19 deletions test/test-utils/utilities.ts
Original file line number Diff line number Diff line change
Expand Up @@ -202,27 +202,24 @@ export const waitEnoughCyclesForModal = async ({
*/
export const clearAllModals = async (): Promise<void> => {
// Prevent modals from leaking and polluting other tests
let keepClosingModals = true;
while (keepClosingModals) {
keepClosingModals = Modal.closeCurrentModal("End of test (clean-up)");
Modal.closeAllModals();

// Then wait for the screen to update (probably React rerender and async/await).
// Important for tests using Jest fake timers to not get into an infinite loop
// of removing the same modal because the promises don't flush otherwise.
//
// XXX: Maybe in the future with Jest 29.5.0+, we could use `runAllTimersAsync` instead.
// Then wait for the screen to update (probably React rerender and async/await).
// Important for tests using Jest fake timers to not get into an infinite loop
// of removing the same modal because the promises don't flush otherwise.
//
// XXX: Maybe in the future with Jest 29.5.0+, we could use `runAllTimersAsync` instead.

// this is called in some places where timers are not faked
// which causes a lot of noise in the console
// to make a hack even hackier check if timers are faked using a weird trick from github
// then call the appropriate promise flusher
// https://github.com/facebook/jest/issues/10555#issuecomment-1136466942
const jestTimersFaked = setTimeout.name === "setTimeout";
if (jestTimersFaked) {
await flushPromisesWithFakeTimers();
} else {
await flushPromises();
}
// this is called in some places where timers are not faked
// which causes a lot of noise in the console
// to make a hack even hackier check if timers are faked using a weird trick from github
// then call the appropriate promise flusher
// https://github.com/facebook/jest/issues/10555#issuecomment-1136466942
const jestTimersFaked = setTimeout.name === "setTimeout";
if (jestTimersFaked) {
await flushPromisesWithFakeTimers();
} else {
await flushPromises();
}
};

Expand Down

0 comments on commit d208bc6

Please sign in to comment.