Reset power selector on API failure to prevent state mismatch (#12319)

* Reset power selector on API failure to prevent state mismatch Signed-off-by: Michael Telatynski <7t3chguy@gmail.com> * Allow onChange to be sync or async Signed-off-by: Michael Telatynski <7t3chguy@gmail.com> * Add unmounted check Signed-off-by: Michael Telatynski <7t3chguy@gmail.com> * Improve coverage 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>
matrix-org · Mar 8, 2024 · 42ac873 · 42ac873
1 parent ddbc643
commit 42ac873
Show file tree

Hide file tree

Showing 4 changed files with 96 additions and 13 deletions.
diff --git a/src/components/views/elements/PowerSelector.tsx b/src/components/views/elements/PowerSelector.tsx
@@ -39,7 +39,7 @@ interface Props<K extends undefined | string> {
     // The name to annotate the selector with
     label?: string;
 
-    onChange(value: number, powerLevelKey: K extends undefined ? void : K): void;
+    onChange(value: number, powerLevelKey: K extends undefined ? void : K): void | Promise<void>;
 
     // Optional key to pass as the second argument to `onChange`
     powerLevelKey: K extends undefined ? void : K;
@@ -60,6 +60,7 @@ export default class PowerSelector<K extends undefined | string> extends React.C
         maxValue: Infinity,
         usersDefault: 0,
     };
+    private unmounted = false;
 
     public constructor(props: Props<K>) {
         super(props);
@@ -84,6 +85,10 @@ export default class PowerSelector<K extends undefined | string> extends React.C
         }
     }
 
+    public componentWillUnmount(): void {
+        this.unmounted = true;
+    }
+
     private initStateFromProps(): void {
         // This needs to be done now because levelRoleMap has translated strings
         const levelRoleMap = Roles.levelRoleMap(this.props.usersDefault);
@@ -106,27 +111,39 @@ export default class PowerSelector<K extends undefined | string> extends React.C
         });
     }
 
-    private onSelectChange = (event: React.ChangeEvent<HTMLSelectElement>): void => {
+    private onSelectChange = async (event: React.ChangeEvent<HTMLSelectElement>): Promise<void> => {
         const isCustom = event.target.value === CUSTOM_VALUE;
         if (isCustom) {
             this.setState({ custom: true });
         } else {
             const powerLevel = parseInt(event.target.value);
-            this.props.onChange(powerLevel, this.props.powerLevelKey);
             this.setState({ selectValue: powerLevel });
+            try {
+                await this.props.onChange(powerLevel, this.props.powerLevelKey);
+            } catch {
+                if (this.unmounted) return;
+                // If the request failed, roll back the state of the selector.
+                this.initStateFromProps();
+            }
         }
     };
 
     private onCustomChange = (event: React.ChangeEvent<HTMLInputElement>): void => {
         this.setState({ customValue: parseInt(event.target.value) });
     };
 
-    private onCustomBlur = (event: React.FocusEvent): void => {
+    private onCustomBlur = async (event: React.FocusEvent): Promise<void> => {
         event.preventDefault();
         event.stopPropagation();
 
         if (Number.isFinite(this.state.customValue)) {
-            this.props.onChange(this.state.customValue, this.props.powerLevelKey);
+            try {
+                await this.props.onChange(this.state.customValue, this.props.powerLevelKey);
+            } catch {
+                if (this.unmounted) return;
+                // If the request failed, roll back the state of the selector.
+                this.initStateFromProps();
+            }
         } else {
             this.initStateFromProps(); // reset, invalid input
         }

diff --git a/src/components/views/settings/tabs/room/RolesRoomSettingsTab.tsx b/src/components/views/settings/tabs/room/RolesRoomSettingsTab.tsx
@@ -174,7 +174,7 @@ export default class RolesRoomSettingsTab extends React.Component<IProps> {
         }
     }
 
-    private onPowerLevelsChanged = (value: number, powerLevelKey: string): void => {
+    private onPowerLevelsChanged = async (value: number, powerLevelKey: string): Promise<void> => {
         const client = this.context;
         const room = this.props.room;
         const plEvent = room.currentState.getStateEvents(EventType.RoomPowerLevels, "");
@@ -203,17 +203,22 @@ export default class RolesRoomSettingsTab extends React.Component<IProps> {
             parentObj[keyPath[keyPath.length - 1]] = value;
         }
 
-        client.sendStateEvent(this.props.room.roomId, EventType.RoomPowerLevels, plContent).catch((e) => {
+        try {
+            await client.sendStateEvent(this.props.room.roomId, EventType.RoomPowerLevels, plContent);
+        } catch (e) {
             logger.error(e);
 
             Modal.createDialog(ErrorDialog, {
                 title: _t("room_settings|permissions|error_changing_pl_reqs_title"),
                 description: _t("room_settings|permissions|error_changing_pl_reqs_description"),
             });
-        });
+
+            // Rethrow so that the PowerSelector can roll back
+            throw e;
+        }
     };
 
-    private onUserPowerLevelChanged = (value: number, powerLevelKey: string): void => {
+    private onUserPowerLevelChanged = async (value: number, powerLevelKey: string): Promise<void> => {
         const client = this.context;
         const room = this.props.room;
         const plEvent = room.currentState.getStateEvents(EventType.RoomPowerLevels, "");
@@ -226,14 +231,19 @@ export default class RolesRoomSettingsTab extends React.Component<IProps> {
         if (!plContent["users"]) plContent["users"] = {};
         plContent["users"][powerLevelKey] = value;
 
-        client.sendStateEvent(this.props.room.roomId, EventType.RoomPowerLevels, plContent).catch((e) => {
+        try {
+            await client.sendStateEvent(this.props.room.roomId, EventType.RoomPowerLevels, plContent);
+        } catch (e) {
             logger.error(e);
 
             Modal.createDialog(ErrorDialog, {
                 title: _t("room_settings|permissions|error_changing_pl_title"),
                 description: _t("room_settings|permissions|error_changing_pl_description"),
             });
-        });
+
+            // Rethrow so that the PowerSelector can roll back
+            throw e;
+        }
     };
 
     public render(): React.ReactNode {

diff --git a/test/components/views/elements/PowerSelector-test.tsx b/test/components/views/elements/PowerSelector-test.tsx
@@ -16,6 +16,7 @@ limitations under the License.
 
 import React from "react";
 import { fireEvent, render, screen } from "@testing-library/react";
+import { defer } from "matrix-js-sdk/src/utils";
 
 import PowerSelector from "../../../../src/components/views/elements/PowerSelector";
 
@@ -75,4 +76,25 @@ describe("<PowerSelector />", () => {
         expect(option.selected).toBeTruthy();
         expect(fn).not.toHaveBeenCalled();
     });
+
+    it("should reset when onChange promise rejects", async () => {
+        const deferred = defer<void>();
+        render(
+            <PowerSelector
+                value={25}
+                maxValue={100}
+                usersDefault={0}
+                onChange={() => deferred.promise}
+                powerLevelKey="key"
+            />,
+        );
+
+        const input = screen.getByLabelText("Power level");
+        fireEvent.change(input, { target: { value: 40 } });
+        fireEvent.blur(input);
+
+        await screen.findByDisplayValue(40);
+        deferred.reject("Some error");
+        await screen.findByDisplayValue(25);
+    });
 });
diff --git a/test/components/views/settings/tabs/room/RolesRoomSettingsTab-test.tsx b/test/components/views/settings/tabs/room/RolesRoomSettingsTab-test.tsx
@@ -15,9 +15,10 @@ limitations under the License.
 */
 
 import React from "react";
-import { fireEvent, render, RenderResult, screen } from "@testing-library/react";
-import { MatrixClient, EventType, MatrixEvent, Room, RoomMember } from "matrix-js-sdk/src/matrix";
+import { fireEvent, render, RenderResult, screen, waitFor } from "@testing-library/react";
+import { MatrixClient, EventType, MatrixEvent, Room, RoomMember, ISendEventResponse } from "matrix-js-sdk/src/matrix";
 import { mocked } from "jest-mock";
+import { defer } from "matrix-js-sdk/src/utils";
 
 import RolesRoomSettingsTab from "../../../../../../src/components/views/settings/tabs/room/RolesRoomSettingsTab";
 import { mkStubRoom, withClientContextRenderOptions, stubClient } from "../../../../../test-utils";
@@ -231,4 +232,37 @@ describe("RolesRoomSettingsTab", () => {
             expect(screen.getByTitle("Banned by Alice")).toBeInTheDocument();
         });
     });
+
+    it("should roll back power level change on error", async () => {
+        const deferred = defer<ISendEventResponse>();
+        mocked(cli.sendStateEvent).mockReturnValue(deferred.promise);
+        mocked(cli.getRoom).mockReturnValue(room);
+        // @ts-ignore - mocked doesn't support overloads properly
+        mocked(room.currentState.getStateEvents).mockImplementation((type, key) => {
+            if (key === undefined) return [] as MatrixEvent[];
+            if (type === "m.room.power_levels") {
+                return new MatrixEvent({
+                    sender: "@sender:server",
+                    room_id: roomId,
+                    type: "m.room.power_levels",
+                    state_key: "",
+                    content: {
+                        users: {
+                            [cli.getUserId()!]: 100,
+                        },
+                    },
+                });
+            }
+            return null;
+        });
+        mocked(room.currentState.mayClientSendStateEvent).mockReturnValue(true);
+        const { container } = renderTab();
+
+        const selector = container.querySelector(`[placeholder="${cli.getUserId()}"]`)!;
+        fireEvent.change(selector, { target: { value: "50" } });
+        expect(selector).toHaveValue("50");
+
+        deferred.reject("Error");
+        await waitFor(() => expect(selector).toHaveValue("100"));
+    });
 });