Skip to content
This repository has been archived by the owner on Sep 11, 2024. It is now read-only.

Fix pin/unpin slowness and non refresh from the message action bar #12934

Merged
merged 5 commits into from
Aug 28, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 17 additions & 0 deletions src/components/views/messages/MessageActionBar.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand Down Expand Up @@ -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 => {
Expand All @@ -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();
Expand Down
4 changes: 0 additions & 4 deletions src/utils/EventUtils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
19 changes: 13 additions & 6 deletions src/utils/PinningUtils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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";

Expand All @@ -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.
*/
Expand Down Expand Up @@ -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
*/
Expand All @@ -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),
);
}

Expand All @@ -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,
]);
}
}
26 changes: 25 additions & 1 deletion test/components/views/messages/MessageActionBar-test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -26,6 +26,7 @@ import {
FeatureSupport,
Thread,
EventTimeline,
RoomStateEvent,
} from "matrix-js-sdk/src/matrix";

import MessageActionBar from "../../../../src/components/views/messages/MessageActionBar";
Expand All @@ -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");

Expand Down Expand Up @@ -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(
Expand Down Expand Up @@ -476,6 +479,7 @@ describe("<MessageActionBar />", () => {
beforeEach(() => {
// enable pin button
jest.spyOn(SettingsStore, "getValue").mockReturnValue(true);
jest.spyOn(PinningUtils, "isPinned").mockReturnValue(false);
});

afterEach(() => {
Expand All @@ -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());
});
});
});
7 changes: 2 additions & 5 deletions test/utils/PinningUtils-test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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", () => {
Expand All @@ -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;
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);
});
Expand Down
Loading