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

Author: florianduros

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<IMessageAction
             this.props.mxEvent.once(MatrixEventEvent.Decrypted, this.onDecrypted);
         }
         this.props.mxEvent.on(MatrixEventEvent.BeforeRedaction, this.onBeforeRedaction);
+        this.context.room
+            ?.getLiveTimeline()
+            .getState(EventTimeline.FORWARDS)
+            ?.on(RoomStateEvent.Events, this.onRoomEvent);
     }
 
     public componentWillUnmount(): void {
         this.props.mxEvent.off(MatrixEventEvent.Status, this.onSent);
         this.props.mxEvent.off(MatrixEventEvent.Decrypted, this.onDecrypted);
         this.props.mxEvent.off(MatrixEventEvent.BeforeRedaction, this.onBeforeRedaction);
+        this.context.room
+            ?.getLiveTimeline()
+            .getState(EventTimeline.FORWARDS)
+            ?.off(RoomStateEvent.Events, this.onRoomEvent);
     }
 
     private onDecrypted = (): void => {
@@ -297,6 +308,12 @@ export default class MessageActionBar extends React.PureComponent<IMessageAction
         this.forceUpdate();
     };
 
+    private onRoomEvent = (event?: MatrixEvent): void => {
+        // 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();

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<ViewRoomPayload>({
         action: Action.ViewRoom,

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,
+        ]);
     }
 }

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("<MessageActionBar />", () => {
         timelineRenderingType: TimelineRenderingType.Room,
         canSendMessages: true,
         canReact: true,
+        room,
     } as unknown as IRoomState;
     const getComponent = (props = {}, roomContext: Partial<IRoomState> = {}) =>
         render(
@@ -476,6 +479,7 @@ describe("<MessageActionBar />", () => {
         beforeEach(() => {
             // enable pin button
             jest.spyOn(SettingsStore, "getValue").mockReturnValue(true);
+            jest.spyOn(PinningUtils, "isPinned").mockReturnValue(false);
         });
 
         afterEach(() => {
@@ -499,5 +503,25 @@ describe("<MessageActionBar />", () => {
             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());
+        });
     });
 });

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);
         });