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

Fix huge usage bandwidth and performance issue of pinned message banner. #37

Merged
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
20 changes: 11 additions & 9 deletions src/hooks/usePinnedEvents.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,19 +24,22 @@ import { ReadPinsEventId } from "../components/views/right_panel/types";
import { useMatrixClientContext } from "../contexts/MatrixClientContext";
import { useAsyncMemo } from "./useAsyncMemo";
import PinningUtils from "../utils/PinningUtils";
import { batch } from "../utils/promise.ts";

/**
* Get the pinned event IDs from a room.
* The number of pinned events is limited to 100.
* @param room
*/
function getPinnedEventIds(room?: Room): string[] {
return (
const eventIds: string[] =
room
?.getLiveTimeline()
.getState(EventTimeline.FORWARDS)
?.getStateEvents(EventType.RoomPinnedEvents, "")
?.getContent()?.pinned ?? []
);
?.getContent()?.pinned ?? [];
// Limit the number of pinned events to 100
return eventIds.slice(0, 100);
}

/**
Expand Down Expand Up @@ -173,12 +176,11 @@ export function useFetchedPinnedEvents(room: Room, pinnedEventIds: string[]): Ar
const cli = useMatrixClientContext();

return useAsyncMemo(
() =>
Promise.all(
pinnedEventIds.map(
async (eventId): Promise<MatrixEvent | null> => fetchPinnedEvent(room, eventId, cli),
),
),
() => {
const fetchPromises = pinnedEventIds.map((eventId) => () => fetchPinnedEvent(room, eventId, cli));
// Fetch the pinned events in batches of 10
return batch(fetchPromises, 10);
},
[cli, room, pinnedEventIds],
null,
);
Expand Down
15 changes: 15 additions & 0 deletions src/utils/promise.ts
Original file line number Diff line number Diff line change
Expand Up @@ -40,3 +40,18 @@ export async function retry<T, E extends Error>(
}
throw lastErr;
}

/**
* Batch promises into groups of a given size.
* Execute the promises in parallel, but wait for all promises in a batch to resolve before moving to the next batch.
* @param funcs - The promises to batch
* @param batchSize - The number of promises to execute in parallel
*/
export async function batch<T>(funcs: Array<() => Promise<T>>, batchSize: number): Promise<T[]> {
const results: T[] = [];
for (let i = 0; i < funcs.length; i += batchSize) {
const batch = funcs.slice(i, i + batchSize);
results.push(...(await Promise.all(batch.map((f) => f()))));
}
return results;
}
15 changes: 15 additions & 0 deletions test/components/views/right_panel/PinnedMessagesCard-test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -196,6 +196,21 @@ describe("<PinnedMessagesCard />", () => {
expect(asFragment()).toMatchSnapshot();
});

it("should not show more than 100 messages", async () => {
const events = Array.from({ length: 120 }, (_, i) =>
mkMessage({
event: true,
room: "!room:example.org",
user: "@alice:example.org",
msg: `The message ${i}`,
ts: i,
}),
);
await initPinnedMessagesCard(events, []);

expect(screen.queryAllByRole("listitem")).toHaveLength(100);
});

it("should updates when messages are pinned", async () => {
// Start with nothing pinned
const { addLocalPinEvent, addNonLocalPinEvent } = await initPinnedMessagesCard([], []);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -358,7 +358,7 @@ exports[`<PinnedMessagesCard /> unpin all should not allow to unpinall 1`] = `
aria-label="Open menu"
class="_icon-button_bh2qc_17"
data-state="closed"
id="radix-18"
id="radix-218"
role="button"
style="--cpd-icon-button-size: 24px;"
tabindex="0"
Expand Down Expand Up @@ -424,7 +424,7 @@ exports[`<PinnedMessagesCard /> unpin all should not allow to unpinall 1`] = `
aria-label="Open menu"
class="_icon-button_bh2qc_17"
data-state="closed"
id="radix-19"
id="radix-219"
role="button"
style="--cpd-icon-button-size: 24px;"
tabindex="0"
Expand Down
57 changes: 57 additions & 0 deletions test/utils/promise-test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
/*
* Copyright 2024 New Vector Ltd.
*
* SPDX-License-Identifier: AGPL-3.0-only OR GPL-3.0-only
* Please see LICENSE files in the repository root for full details.
*
*/

import { batch } from "../../src/utils/promise.ts";

describe("promise.ts", () => {
describe("batch", () => {
afterEach(() => jest.useRealTimers());

it("should batch promises into groups of a given size", async () => {
const promises = [() => Promise.resolve(1), () => Promise.resolve(2), () => Promise.resolve(3)];
const batchSize = 2;
const result = await batch(promises, batchSize);
expect(result).toEqual([1, 2, 3]);
});

it("should wait for the current batch to finish to request the next one", async () => {
jest.useFakeTimers();

let promise1Called = false;
const promise1 = () =>
new Promise<number>((resolve) => {
promise1Called = true;
resolve(1);
});
let promise2Called = false;
const promise2 = () =>
new Promise<number>((resolve) => {
promise2Called = true;
setTimeout(() => {
resolve(2);
}, 10);
});

let promise3Called = false;
const promise3 = () =>
new Promise<number>((resolve) => {
promise3Called = true;
resolve(3);
});
const batchSize = 2;
const batchPromise = batch([promise1, promise2, promise3], batchSize);

expect(promise1Called).toBe(true);
expect(promise2Called).toBe(true);
expect(promise3Called).toBe(false);

jest.advanceTimersByTime(11);
expect(await batchPromise).toEqual([1, 2, 3]);
});
});
});
Loading