From c8105e83efb1e9d45a918220e1945252177c3751 Mon Sep 17 00:00:00 2001 From: Michael Telatynski <7t3chguy@gmail.com> Date: Wed, 6 Mar 2024 11:22:26 +0000 Subject: [PATCH 1/5] Reset power selector on API failure to prevent state mismatch Signed-off-by: Michael Telatynski <7t3chguy@gmail.com> --- .../views/elements/PowerSelector.tsx | 10 ++++++--- .../tabs/room/RolesRoomSettingsTab.tsx | 22 ++++++++++++++----- .../views/elements/PowerSelector-test.tsx | 22 +++++++++++++++++++ 3 files changed, 45 insertions(+), 9 deletions(-) diff --git a/src/components/views/elements/PowerSelector.tsx b/src/components/views/elements/PowerSelector.tsx index 36bb51e4bbe..c5681828457 100644 --- a/src/components/views/elements/PowerSelector.tsx +++ b/src/components/views/elements/PowerSelector.tsx @@ -39,7 +39,7 @@ interface Props { // 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): Promise; // Optional key to pass as the second argument to `onChange` powerLevelKey: K extends undefined ? void : K; @@ -112,8 +112,10 @@ export default class PowerSelector extends React.C this.setState({ custom: true }); } else { const powerLevel = parseInt(event.target.value); - this.props.onChange(powerLevel, this.props.powerLevelKey); this.setState({ selectValue: powerLevel }); + this.props.onChange(powerLevel, this.props.powerLevelKey).catch(() => { + this.initStateFromProps(); + }); } }; @@ -126,7 +128,9 @@ export default class PowerSelector extends React.C event.stopPropagation(); if (Number.isFinite(this.state.customValue)) { - this.props.onChange(this.state.customValue, this.props.powerLevelKey); + this.props.onChange(this.state.customValue, this.props.powerLevelKey).catch(() => { + 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 index 7bce2ccb17f..5f03e7f9505 100644 --- 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 { } } - private onPowerLevelsChanged = (value: number, powerLevelKey: string): void => { + private onPowerLevelsChanged = async (value: number, powerLevelKey: string): Promise => { 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 { 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 => { 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 { 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 index 4636aaafc79..bf300e197ff 100644 --- 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("", () => { expect(option.selected).toBeTruthy(); expect(fn).not.toHaveBeenCalled(); }); + + it("should reset when onChange promise rejects", async () => { + const deferred = defer(); + render( + 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); + }); }); From d70d677efd3b7572980d5399a93324aa947a2fb7 Mon Sep 17 00:00:00 2001 From: Michael Telatynski <7t3chguy@gmail.com> Date: Wed, 6 Mar 2024 11:45:05 +0000 Subject: [PATCH 2/5] Allow onChange to be sync or async Signed-off-by: Michael Telatynski <7t3chguy@gmail.com> --- .../views/elements/PowerSelector.tsx | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/src/components/views/elements/PowerSelector.tsx b/src/components/views/elements/PowerSelector.tsx index c5681828457..d437a363fb5 100644 --- a/src/components/views/elements/PowerSelector.tsx +++ b/src/components/views/elements/PowerSelector.tsx @@ -39,7 +39,7 @@ interface Props { // The name to annotate the selector with label?: string; - onChange(value: number, powerLevelKey: K extends undefined ? void : K): Promise; + onChange(value: number, powerLevelKey: K extends undefined ? void : K): void | Promise; // Optional key to pass as the second argument to `onChange` powerLevelKey: K extends undefined ? void : K; @@ -106,16 +106,18 @@ export default class PowerSelector extends React.C }); } - private onSelectChange = (event: React.ChangeEvent): void => { + private onSelectChange = async (event: React.ChangeEvent): Promise => { const isCustom = event.target.value === CUSTOM_VALUE; if (isCustom) { this.setState({ custom: true }); } else { const powerLevel = parseInt(event.target.value); this.setState({ selectValue: powerLevel }); - this.props.onChange(powerLevel, this.props.powerLevelKey).catch(() => { + try { + await this.props.onChange(powerLevel, this.props.powerLevelKey); + } catch { this.initStateFromProps(); - }); + } } }; @@ -123,14 +125,16 @@ export default class PowerSelector extends React.C this.setState({ customValue: parseInt(event.target.value) }); }; - private onCustomBlur = (event: React.FocusEvent): void => { + private onCustomBlur = async (event: React.FocusEvent): Promise => { event.preventDefault(); event.stopPropagation(); if (Number.isFinite(this.state.customValue)) { - this.props.onChange(this.state.customValue, this.props.powerLevelKey).catch(() => { + try { + await this.props.onChange(this.state.customValue, this.props.powerLevelKey); + } catch { this.initStateFromProps(); - }); + } } else { this.initStateFromProps(); // reset, invalid input } From 93979610f8b6296737801af6877831d5cc4223c3 Mon Sep 17 00:00:00 2001 From: Michael Telatynski <7t3chguy@gmail.com> Date: Fri, 8 Mar 2024 11:25:00 +0000 Subject: [PATCH 3/5] Add unmounted check Signed-off-by: Michael Telatynski <7t3chguy@gmail.com> --- src/components/views/elements/PowerSelector.tsx | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/components/views/elements/PowerSelector.tsx b/src/components/views/elements/PowerSelector.tsx index d437a363fb5..1c35424fe25 100644 --- a/src/components/views/elements/PowerSelector.tsx +++ b/src/components/views/elements/PowerSelector.tsx @@ -60,6 +60,7 @@ export default class PowerSelector extends React.C maxValue: Infinity, usersDefault: 0, }; + public unmounted = false; public constructor(props: Props) { super(props); @@ -84,6 +85,10 @@ export default class PowerSelector 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); @@ -116,6 +121,7 @@ export default class PowerSelector extends React.C try { await this.props.onChange(powerLevel, this.props.powerLevelKey); } catch { + if (this.unmounted) return; this.initStateFromProps(); } } @@ -133,6 +139,7 @@ export default class PowerSelector extends React.C try { await this.props.onChange(this.state.customValue, this.props.powerLevelKey); } catch { + if (this.unmounted) return; this.initStateFromProps(); } } else { From f3a69f70bb385134c0cb33a33fed665bb1b5fdc6 Mon Sep 17 00:00:00 2001 From: Michael Telatynski <7t3chguy@gmail.com> Date: Fri, 8 Mar 2024 11:34:56 +0000 Subject: [PATCH 4/5] Improve coverage Signed-off-by: Michael Telatynski <7t3chguy@gmail.com> --- .../tabs/room/RolesRoomSettingsTab-test.tsx | 38 ++++++++++++++++++- 1 file changed, 36 insertions(+), 2 deletions(-) diff --git a/test/components/views/settings/tabs/room/RolesRoomSettingsTab-test.tsx b/test/components/views/settings/tabs/room/RolesRoomSettingsTab-test.tsx index 0a9c8406f19..f121f6f045b 100644 --- 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(); + 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")); + }); }); From d312d5321e5b8b82172df8076137c20bce25c82b Mon Sep 17 00:00:00 2001 From: Michael Telatynski <7t3chguy@gmail.com> Date: Fri, 8 Mar 2024 11:35:43 +0000 Subject: [PATCH 5/5] Iterate Signed-off-by: Michael Telatynski <7t3chguy@gmail.com> --- src/components/views/elements/PowerSelector.tsx | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/components/views/elements/PowerSelector.tsx b/src/components/views/elements/PowerSelector.tsx index 1c35424fe25..7dcf3d78ecc 100644 --- a/src/components/views/elements/PowerSelector.tsx +++ b/src/components/views/elements/PowerSelector.tsx @@ -60,7 +60,7 @@ export default class PowerSelector extends React.C maxValue: Infinity, usersDefault: 0, }; - public unmounted = false; + private unmounted = false; public constructor(props: Props) { super(props); @@ -122,6 +122,7 @@ export default class PowerSelector extends React.C 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(); } } @@ -140,6 +141,7 @@ export default class PowerSelector extends React.C 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 {