From ea3c5cf7870f66949605850f0be3f18f8385e1ea Mon Sep 17 00:00:00 2001 From: Florian Duros Date: Wed, 28 Aug 2024 10:56:46 +0200 Subject: [PATCH] Fix pin/unpin slowness and non refresh from the message action bar (#12934) * Improve PinningUtils.ts doc and use common methods to check pin or unpin. Removed unused methods. * Send room account data and state event in parallel * Rerender MessageActionBar.tsx if there is a room pinned event * Update pinning util tests * Add test for room pinned events in MessageActionBar-test.tsx --- .../views/messages/MessageActionBar.tsx | 17 ++++++++++++ src/utils/EventUtils.ts | 4 --- src/utils/PinningUtils.ts | 19 +++++++++----- .../views/messages/MessageActionBar-test.tsx | 26 ++++++++++++++++++- test/utils/PinningUtils-test.ts | 7 ++--- 5 files changed, 57 insertions(+), 16 deletions(-) diff --git a/src/components/views/messages/MessageActionBar.tsx b/src/components/views/messages/MessageActionBar.tsx index eebcb96b1e..9130183e84 100644 --- a/src/components/views/messages/MessageActionBar.tsx +++ b/src/components/views/messages/MessageActionBar.tsx @@ -24,6 +24,9 @@ import { MsgType, RelationType, M_BEACON_INFO, + EventTimeline, + RoomStateEvent, + EventType, } from "matrix-js-sdk/src/matrix"; import classNames from "classnames"; import { Icon as PinIcon } from "@vector-im/compound-design-tokens/icons/pin.svg"; @@ -278,12 +281,20 @@ export default class MessageActionBar extends React.PureComponent { @@ -297,6 +308,12 @@ export default class MessageActionBar extends React.PureComponent { + // If the event is pinned or unpinned, rerender the component. + if (!event || event.getType() !== EventType.RoomPinnedEvents) return; + this.forceUpdate(); + }; + private onSent = (): void => { // When an event is sent and echoed the possible actions change. this.forceUpdate(); diff --git a/src/utils/EventUtils.ts b/src/utils/EventUtils.ts index 373bcf49a7..dc5c89379b 100644 --- a/src/utils/EventUtils.ts +++ b/src/utils/EventUtils.ts @@ -286,10 +286,6 @@ export function hasThreadSummary(event: MatrixEvent): boolean { return event.isThreadRoot && !!event.getThread()?.length && !!event.getThread()!.replyToEvent; } -export function canPinEvent(event: MatrixEvent): boolean { - return !M_BEACON_INFO.matches(event.getType()); -} - export const highlightEvent = (roomId: string, eventId: string): void => { defaultDispatcher.dispatch({ action: Action.ViewRoom, diff --git a/src/utils/PinningUtils.ts b/src/utils/PinningUtils.ts index 0d3323d50b..22db64a6f1 100644 --- a/src/utils/PinningUtils.ts +++ b/src/utils/PinningUtils.ts @@ -16,7 +16,7 @@ limitations under the License. import { MatrixEvent, EventType, M_POLL_START, MatrixClient, EventTimeline } from "matrix-js-sdk/src/matrix"; -import { canPinEvent, isContentActionable } from "./EventUtils"; +import { isContentActionable } from "./EventUtils"; import SettingsStore from "../settings/SettingsStore"; import { ReadPinsEventId } from "../components/views/right_panel/types"; @@ -31,7 +31,8 @@ export default class PinningUtils { ]; /** - * Determines if the given event may be pinned. + * Determines if the given event can be pinned. + * This is a simple check to see if the event is of a type that can be pinned. * @param {MatrixEvent} event The event to check. * @return {boolean} True if the event may be pinned, false otherwise. */ @@ -62,7 +63,8 @@ export default class PinningUtils { } /** - * Determines if the given event may be pinned or unpinned. + * Determines if the given event may be pinned or unpinned by the current user. + * This checks if the user has the necessary permissions to pin or unpin the event, and if the event is pinnable. * @param matrixClient * @param mxEvent */ @@ -77,7 +79,7 @@ export default class PinningUtils { room .getLiveTimeline() .getState(EventTimeline.FORWARDS) - ?.mayClientSendStateEvent(EventType.RoomPinnedEvents, matrixClient) && canPinEvent(mxEvent), + ?.mayClientSendStateEvent(EventType.RoomPinnedEvents, matrixClient) && PinningUtils.isPinnable(mxEvent), ); } @@ -101,16 +103,21 @@ export default class PinningUtils { ?.getStateEvents(EventType.RoomPinnedEvents, "") ?.getContent().pinned || []; + let roomAccountDataPromise: Promise<{} | void> = Promise.resolve(); // If the event is already pinned, unpin it if (pinnedIds.includes(eventId)) { pinnedIds.splice(pinnedIds.indexOf(eventId), 1); } else { // Otherwise, pin it pinnedIds.push(eventId); - await matrixClient.setRoomAccountData(room.roomId, ReadPinsEventId, { + // We don't want to wait for the roomAccountDataPromise to resolve before sending the state event + roomAccountDataPromise = matrixClient.setRoomAccountData(room.roomId, ReadPinsEventId, { event_ids: [...(room.getAccountData(ReadPinsEventId)?.getContent()?.event_ids || []), eventId], }); } - await matrixClient.sendStateEvent(room.roomId, EventType.RoomPinnedEvents, { pinned: pinnedIds }, ""); + await Promise.all([ + matrixClient.sendStateEvent(room.roomId, EventType.RoomPinnedEvents, { pinned: pinnedIds }, ""), + roomAccountDataPromise, + ]); } } diff --git a/test/components/views/messages/MessageActionBar-test.tsx b/test/components/views/messages/MessageActionBar-test.tsx index ad184314a2..9b5da31802 100644 --- a/test/components/views/messages/MessageActionBar-test.tsx +++ b/test/components/views/messages/MessageActionBar-test.tsx @@ -15,7 +15,7 @@ limitations under the License. */ import React from "react"; -import { act, render, fireEvent } from "@testing-library/react"; +import { act, render, fireEvent, screen, waitFor } from "@testing-library/react"; import { EventType, EventStatus, @@ -26,6 +26,7 @@ import { FeatureSupport, Thread, EventTimeline, + RoomStateEvent, } from "matrix-js-sdk/src/matrix"; import MessageActionBar from "../../../../src/components/views/messages/MessageActionBar"; @@ -41,6 +42,7 @@ import { IRoomState } from "../../../../src/components/structures/RoomView"; import dispatcher from "../../../../src/dispatcher/dispatcher"; import SettingsStore from "../../../../src/settings/SettingsStore"; import { Action } from "../../../../src/dispatcher/actions"; +import PinningUtils from "../../../../src/utils/PinningUtils"; jest.mock("../../../../src/dispatcher/dispatcher"); @@ -119,6 +121,7 @@ describe("", () => { timelineRenderingType: TimelineRenderingType.Room, canSendMessages: true, canReact: true, + room, } as unknown as IRoomState; const getComponent = (props = {}, roomContext: Partial = {}) => render( @@ -476,6 +479,7 @@ describe("", () => { beforeEach(() => { // enable pin button jest.spyOn(SettingsStore, "getValue").mockReturnValue(true); + jest.spyOn(PinningUtils, "isPinned").mockReturnValue(false); }); afterEach(() => { @@ -499,5 +503,25 @@ describe("", () => { const { queryByLabelText } = getComponent({ mxEvent: alicesMessageEvent }); expect(queryByLabelText("Pin")).toBeTruthy(); }); + + it("should listen to room pinned events", async () => { + getComponent({ mxEvent: alicesMessageEvent }); + expect(screen.getByLabelText("Pin")).toBeInTheDocument(); + + // Event is considered pinned + jest.spyOn(PinningUtils, "isPinned").mockReturnValue(true); + // Emit that the room pinned events have changed + const roomState = room.getLiveTimeline().getState(EventTimeline.FORWARDS)!; + roomState.emit( + RoomStateEvent.Events, + { + getType: () => EventType.RoomPinnedEvents, + } as MatrixEvent, + roomState, + null, + ); + + await waitFor(() => expect(screen.getByLabelText("Unpin")).toBeInTheDocument()); + }); }); }); diff --git a/test/utils/PinningUtils-test.ts b/test/utils/PinningUtils-test.ts index 07b26adb8a..47434c4fca 100644 --- a/test/utils/PinningUtils-test.ts +++ b/test/utils/PinningUtils-test.ts @@ -20,7 +20,7 @@ import { mocked } from "jest-mock"; import { createTestClient } from "../test-utils"; import PinningUtils from "../../src/utils/PinningUtils"; import SettingsStore from "../../src/settings/SettingsStore"; -import { canPinEvent, isContentActionable } from "../../src/utils/EventUtils"; +import { isContentActionable } from "../../src/utils/EventUtils"; import { ReadPinsEventId } from "../../src/components/views/right_panel/types"; jest.mock("../../src/utils/EventUtils", () => { @@ -35,7 +35,6 @@ describe("PinningUtils", () => { const userId = "@alice:example.org"; const mockedIsContentActionable = mocked(isContentActionable); - const mockedCanPinEvent = mocked(canPinEvent); let matrixClient: MatrixClient; let room: Room; @@ -63,7 +62,6 @@ describe("PinningUtils", () => { // Enable feature pinning jest.spyOn(SettingsStore, "getValue").mockReturnValue(true); mockedIsContentActionable.mockImplementation(() => true); - mockedCanPinEvent.mockImplementation(() => true); matrixClient = createTestClient(); room = new Room(roomId, matrixClient, userId); @@ -171,8 +169,7 @@ describe("PinningUtils", () => { }); test("should return false if event is not pinnable", () => { - mockedCanPinEvent.mockReturnValue(false); - const event = makePinEvent(); + const event = makePinEvent({ type: EventType.RoomCreate }); expect(PinningUtils.canPinOrUnpin(matrixClient, event)).toBe(false); });