From 502cc91dfee611699518e4d2c4708ef624ddc0e6 Mon Sep 17 00:00:00 2001 From: Michael Telatynski <7t3chguy@gmail.com> Date: Mon, 4 Nov 2024 11:34:00 +0000 Subject: [PATCH] Switch ModalManager to the React 18 createRoot API (#28336) * Remove boilerplate around dispatcher and settings watchers Signed-off-by: Michael Telatynski <7t3chguy@gmail.com> * Iterate Signed-off-by: Michael Telatynski <7t3chguy@gmail.com> * Move state update listeners from constructor to componentDidMount Signed-off-by: Michael Telatynski <7t3chguy@gmail.com> * Iterate Signed-off-by: Michael Telatynski <7t3chguy@gmail.com> * Switch ModalManager to the React 18 createRoot API Signed-off-by: Michael Telatynski <7t3chguy@gmail.com> * Iterate Signed-off-by: Michael Telatynski <7t3chguy@gmail.com> * Iterate Signed-off-by: Michael Telatynski <7t3chguy@gmail.com> --------- Signed-off-by: Michael Telatynski <7t3chguy@gmail.com> --- src/Modal.tsx | 61 +++++++++---------- .../components/structures/MatrixChat-test.tsx | 6 +- .../structures/auth/ForgotPassword-test.tsx | 36 +++++------ .../dialogs/ConfirmRedactDialog-test.tsx | 6 +- .../views/messages/DateSeparator-test.tsx | 6 +- .../wysiwyg_composer/utils/message-test.ts | 2 +- .../views/settings/JoinRuleSettings-test.tsx | 4 +- .../tabs/user/SessionManagerTab-test.tsx | 1 + .../media/requestMediaPermissions-test.tsx | 4 +- 9 files changed, 66 insertions(+), 60 deletions(-) diff --git a/src/Modal.tsx b/src/Modal.tsx index 9e6a7672be..a2919bdc5f 100644 --- a/src/Modal.tsx +++ b/src/Modal.tsx @@ -8,9 +8,9 @@ Please see LICENSE files in the repository root for full details. */ import React, { StrictMode } from "react"; -import ReactDOM from "react-dom"; +import { createRoot, Root } from "react-dom/client"; import classNames from "classnames"; -import { IDeferred, defer, sleep } from "matrix-js-sdk/src/utils"; +import { IDeferred, defer } from "matrix-js-sdk/src/utils"; import { TypedEventEmitter } from "matrix-js-sdk/src/matrix"; import { Glass, TooltipProvider } from "@vector-im/compound-web"; @@ -69,6 +69,16 @@ type HandlerMap = { type ModalCloseReason = "backgroundClick"; +function getOrCreateContainer(id: string): HTMLDivElement { + let container = document.getElementById(id) as HTMLDivElement | null; + if (!container) { + container = document.createElement("div"); + container.id = id; + document.body.appendChild(container); + } + return container; +} + export class ModalManager extends TypedEventEmitter { private counter = 0; // The modal to prioritise over all others. If this is set, only show @@ -83,28 +93,22 @@ export class ModalManager extends TypedEventEmitter[] = []; - private static getOrCreateContainer(): HTMLElement { - let container = document.getElementById(DIALOG_CONTAINER_ID); - - if (!container) { - container = document.createElement("div"); - container.id = DIALOG_CONTAINER_ID; - document.body.appendChild(container); + private static root?: Root; + private static getOrCreateRoot(): Root { + if (!ModalManager.root) { + const container = getOrCreateContainer(DIALOG_CONTAINER_ID); + ModalManager.root = createRoot(container); } - - return container; + return ModalManager.root; } - private static getOrCreateStaticContainer(): HTMLElement { - let container = document.getElementById(STATIC_DIALOG_CONTAINER_ID); - - if (!container) { - container = document.createElement("div"); - container.id = STATIC_DIALOG_CONTAINER_ID; - document.body.appendChild(container); + private static staticRoot?: Root; + private static getOrCreateStaticRoot(): Root { + if (!ModalManager.staticRoot) { + const container = getOrCreateContainer(STATIC_DIALOG_CONTAINER_ID); + ModalManager.staticRoot = createRoot(container); } - - return container; + return ModalManager.staticRoot; } public constructor() { @@ -389,19 +393,14 @@ export class ModalManager extends TypedEventEmitter { - // TODO: We should figure out how to remove this weird sleep. It also makes testing harder - // - // await next tick because sometimes ReactDOM can race with itself and cause the modal to wrongly stick around - await sleep(0); - if (this.modals.length === 0 && !this.priorityModal && !this.staticModal) { // If there is no modal to render, make all of Element available // to screen reader users again dis.dispatch({ action: "aria_unhide_main_app", }); - ReactDOM.unmountComponentAtNode(ModalManager.getOrCreateContainer()); - ReactDOM.unmountComponentAtNode(ModalManager.getOrCreateStaticContainer()); + ModalManager.getOrCreateRoot().render(<>); + ModalManager.getOrCreateStaticRoot().render(<>); return; } @@ -432,10 +431,10 @@ export class ModalManager extends TypedEventEmitter ); - ReactDOM.render(staticDialog, ModalManager.getOrCreateStaticContainer()); + ModalManager.getOrCreateStaticRoot().render(staticDialog); } else { // This is safe to call repeatedly if we happen to do that - ReactDOM.unmountComponentAtNode(ModalManager.getOrCreateStaticContainer()); + ModalManager.getOrCreateStaticRoot().render(<>); } const modal = this.getCurrentModal(); @@ -461,10 +460,10 @@ export class ModalManager extends TypedEventEmitter ); - setTimeout(() => ReactDOM.render(dialog, ModalManager.getOrCreateContainer()), 0); + ModalManager.getOrCreateRoot().render(dialog); } else { // This is safe to call repeatedly if we happen to do that - ReactDOM.unmountComponentAtNode(ModalManager.getOrCreateContainer()); + ModalManager.getOrCreateRoot().render(<>); } } } diff --git a/test/unit-tests/components/structures/MatrixChat-test.tsx b/test/unit-tests/components/structures/MatrixChat-test.tsx index 7f565d682f..4e04025f55 100644 --- a/test/unit-tests/components/structures/MatrixChat-test.tsx +++ b/test/unit-tests/components/structures/MatrixChat-test.tsx @@ -62,6 +62,7 @@ import { DRAFT_LAST_CLEANUP_KEY } from "../../../../src/DraftCleaner"; import { UIFeature } from "../../../../src/settings/UIFeature"; import AutoDiscoveryUtils from "../../../../src/utils/AutoDiscoveryUtils"; import { ValidatedServerConfig } from "../../../../src/utils/ValidatedServerConfig"; +import Modal from "../../../../src/Modal.tsx"; jest.mock("matrix-js-sdk/src/oidc/authorize", () => ({ completeAuthorizationCodeGrant: jest.fn(), @@ -1514,7 +1515,9 @@ describe("", () => { describe("when key backup failed", () => { it("should show the new recovery method dialog", async () => { + const spy = jest.spyOn(Modal, "createDialogAsync"); jest.mock("../../../../src/async-components/views/dialogs/security/NewRecoveryMethodDialog", () => ({ + __test: true, __esModule: true, default: () => mocked dialog, })); @@ -1526,7 +1529,8 @@ describe("", () => { }); await flushPromises(); mockClient.emit(CryptoEvent.KeyBackupFailed, "error code"); - await waitFor(() => expect(screen.getByText("mocked dialog")).toBeInTheDocument()); + await waitFor(() => expect(spy).toHaveBeenCalledTimes(1)); + expect(await spy.mock.lastCall![0]).toEqual(expect.objectContaining({ __test: true })); }); }); }); diff --git a/test/unit-tests/components/structures/auth/ForgotPassword-test.tsx b/test/unit-tests/components/structures/auth/ForgotPassword-test.tsx index f439605319..db6ce005c0 100644 --- a/test/unit-tests/components/structures/auth/ForgotPassword-test.tsx +++ b/test/unit-tests/components/structures/auth/ForgotPassword-test.tsx @@ -8,7 +8,7 @@ Please see LICENSE files in the repository root for full details. import React from "react"; import { mocked } from "jest-mock"; -import { act, render, RenderResult, screen } from "jest-matrix-react"; +import { act, render, RenderResult, screen, waitFor } from "jest-matrix-react"; import userEvent from "@testing-library/user-event"; import { MatrixClient, createClient } from "matrix-js-sdk/src/matrix"; @@ -47,14 +47,12 @@ describe("", () => { }; const click = async (element: Element): Promise => { - await act(async () => { - await userEvent.click(element, { delay: null }); - }); + await userEvent.click(element, { delay: null }); }; const itShouldCloseTheDialogAndShowThePasswordInput = (): void => { - it("should close the dialog and show the password input", () => { - expect(screen.queryByText("Verify your email to continue")).not.toBeInTheDocument(); + it("should close the dialog and show the password input", async () => { + await waitFor(() => expect(screen.queryByText("Verify your email to continue")).not.toBeInTheDocument()); expect(screen.getByText("Reset your password")).toBeInTheDocument(); }); }; @@ -314,7 +312,7 @@ describe("", () => { }); }); - it("should send the new password and show the click validation link dialog", () => { + it("should send the new password and show the click validation link dialog", async () => { expect(client.setPassword).toHaveBeenCalledWith( { type: "m.login.email.identity", @@ -326,15 +324,15 @@ describe("", () => { testPassword, false, ); - expect(screen.getByText("Verify your email to continue")).toBeInTheDocument(); + await expect( + screen.findByText("Verify your email to continue"), + ).resolves.toBeInTheDocument(); expect(screen.getByText(testEmail)).toBeInTheDocument(); }); describe("and dismissing the dialog by clicking the background", () => { beforeEach(async () => { - await act(async () => { - await userEvent.click(screen.getByTestId("dialog-background"), { delay: null }); - }); + await userEvent.click(await screen.findByTestId("dialog-background"), { delay: null }); await waitEnoughCyclesForModal({ useFakeTimers: true, }); @@ -345,7 +343,7 @@ describe("", () => { describe("and dismissing the dialog", () => { beforeEach(async () => { - await click(screen.getByLabelText("Close dialog")); + await click(await screen.findByLabelText("Close dialog")); await waitEnoughCyclesForModal({ useFakeTimers: true, }); @@ -356,14 +354,16 @@ describe("", () => { describe("and clicking »Re-enter email address«", () => { beforeEach(async () => { - await click(screen.getByText("Re-enter email address")); + await click(await screen.findByText("Re-enter email address")); await waitEnoughCyclesForModal({ useFakeTimers: true, }); }); - it("should close the dialog and go back to the email input", () => { - expect(screen.queryByText("Verify your email to continue")).not.toBeInTheDocument(); + it("should close the dialog and go back to the email input", async () => { + await waitFor(() => + expect(screen.queryByText("Verify your email to continue")).not.toBeInTheDocument(), + ); expect(screen.queryByText("Enter your email to reset password")).toBeInTheDocument(); }); }); @@ -397,11 +397,11 @@ describe("", () => { }); it("should show the sign out warning dialog", async () => { - expect( - screen.getByText( + await expect( + screen.findByText( "Signing out your devices will delete the message encryption keys stored on them, making encrypted chat history unreadable.", ), - ).toBeInTheDocument(); + ).resolves.toBeInTheDocument(); // confirm dialog await click(screen.getByText("Continue")); diff --git a/test/unit-tests/components/views/dialogs/ConfirmRedactDialog-test.tsx b/test/unit-tests/components/views/dialogs/ConfirmRedactDialog-test.tsx index b6c894c52b..c648f416f9 100644 --- a/test/unit-tests/components/views/dialogs/ConfirmRedactDialog-test.tsx +++ b/test/unit-tests/components/views/dialogs/ConfirmRedactDialog-test.tsx @@ -8,7 +8,7 @@ Please see LICENSE files in the repository root for full details. import { Feature, ServerSupport } from "matrix-js-sdk/src/feature"; import { MatrixClient, MatrixEvent, RelationType } from "matrix-js-sdk/src/matrix"; -import { screen } from "jest-matrix-react"; +import { screen, act } from "jest-matrix-react"; import userEvent from "@testing-library/user-event"; import { flushPromises, mkEvent, stubClient } from "../../../../test-utils"; @@ -31,12 +31,12 @@ describe("ConfirmRedactDialog", () => { }; const confirmDeleteVoiceBroadcastStartedEvent = async () => { - createRedactEventDialog({ mxEvent }); + act(() => createRedactEventDialog({ mxEvent })); // double-flush promises required for the dialog to show up await flushPromises(); await flushPromises(); - await userEvent.click(screen.getByTestId("dialog-primary-button")); + await userEvent.click(await screen.findByTestId("dialog-primary-button")); }; beforeEach(() => { diff --git a/test/unit-tests/components/views/messages/DateSeparator-test.tsx b/test/unit-tests/components/views/messages/DateSeparator-test.tsx index 4bdd66b227..25b89e05f2 100644 --- a/test/unit-tests/components/views/messages/DateSeparator-test.tsx +++ b/test/unit-tests/components/views/messages/DateSeparator-test.tsx @@ -8,7 +8,7 @@ Please see LICENSE files in the repository root for full details. import React from "react"; import { mocked } from "jest-mock"; -import { fireEvent, render, screen } from "jest-matrix-react"; +import { fireEvent, render, screen, waitFor } from "jest-matrix-react"; import { TimestampToEventResponse, ConnectionError, HTTPError, MatrixError } from "matrix-js-sdk/src/matrix"; import dispatcher from "../../../../../src/dispatcher/dispatcher"; @@ -291,7 +291,9 @@ describe("DateSeparator", () => { // The submit debug logs option should *NOT* be shown for network errors. // // We have to use `queryBy` so that it can return `null` for something that does not exist. - expect(screen.queryByTestId("jump-to-date-error-submit-debug-logs-button")).not.toBeInTheDocument(); + await waitFor(() => + expect(screen.queryByTestId("jump-to-date-error-submit-debug-logs-button")).not.toBeInTheDocument(), + ); }); }); }); diff --git a/test/unit-tests/components/views/rooms/wysiwyg_composer/utils/message-test.ts b/test/unit-tests/components/views/rooms/wysiwyg_composer/utils/message-test.ts index 4cabd6f666..7c77912206 100644 --- a/test/unit-tests/components/views/rooms/wysiwyg_composer/utils/message-test.ts +++ b/test/unit-tests/components/views/rooms/wysiwyg_composer/utils/message-test.ts @@ -417,7 +417,7 @@ describe("message", () => { expect(mockClient.sendMessage).toHaveBeenCalledTimes(0); expect(mockClient.cancelPendingEvent).toHaveBeenCalledTimes(1); expect(mockCreateRedactEventDialog).toHaveBeenCalledTimes(1); - expect(spyDispatcher).toHaveBeenCalledTimes(0); + expect(spyDispatcher).toHaveBeenCalledTimes(1); }); it("Should do nothing if the content is unmodified", async () => { diff --git a/test/unit-tests/components/views/settings/JoinRuleSettings-test.tsx b/test/unit-tests/components/views/settings/JoinRuleSettings-test.tsx index 55a179b1d1..36dd664ac6 100644 --- a/test/unit-tests/components/views/settings/JoinRuleSettings-test.tsx +++ b/test/unit-tests/components/views/settings/JoinRuleSettings-test.tsx @@ -202,7 +202,7 @@ describe("", () => { await flushPromises(); - expect(within(dialog).getByText("Loading new room")).toBeInTheDocument(); + await expect(within(dialog).findByText("Loading new room")).resolves.toBeInTheDocument(); // "create" our new room, have it come thru sync client.getRoom.mockImplementation((id) => { @@ -250,7 +250,7 @@ describe("", () => { await flushPromises(); - expect(within(dialog).getByText("Loading new room")).toBeInTheDocument(); + await expect(within(dialog).findByText("Loading new room")).resolves.toBeInTheDocument(); // "create" our new room, have it come thru sync client.getRoom.mockImplementation((id) => { diff --git a/test/unit-tests/components/views/settings/tabs/user/SessionManagerTab-test.tsx b/test/unit-tests/components/views/settings/tabs/user/SessionManagerTab-test.tsx index 32497cb0c8..52c9d3aaa9 100644 --- a/test/unit-tests/components/views/settings/tabs/user/SessionManagerTab-test.tsx +++ b/test/unit-tests/components/views/settings/tabs/user/SessionManagerTab-test.tsx @@ -119,6 +119,7 @@ describe("", () => { const mockVerificationRequest = { cancel: jest.fn(), on: jest.fn(), + off: jest.fn(), } as unknown as VerificationRequest; const mockCrypto = mocked({ diff --git a/test/unit-tests/utils/media/requestMediaPermissions-test.tsx b/test/unit-tests/utils/media/requestMediaPermissions-test.tsx index a968b51fab..14dfa15505 100644 --- a/test/unit-tests/utils/media/requestMediaPermissions-test.tsx +++ b/test/unit-tests/utils/media/requestMediaPermissions-test.tsx @@ -19,9 +19,9 @@ describe("requestMediaPermissions", () => { const audioStream = {} as MediaStream; const itShouldLogTheErrorAndShowTheNoMediaPermissionsModal = () => { - it("should log the error and show the »No media permissions« modal", () => { + it("should log the error and show the »No media permissions« modal", async () => { expect(logger.log).toHaveBeenCalledWith("Failed to list userMedia devices", error); - screen.getByText("No media permissions"); + await screen.findByText("No media permissions"); }); };