From 6b384fe9c1f28645601c55856d6e242900934086 Mon Sep 17 00:00:00 2001 From: Florian Duros Date: Fri, 13 Sep 2024 09:47:22 +0200 Subject: [PATCH] Fix huge usage bandwidth and performance issue of pinned message banner. (#37) * Return only the first 100 pinned messages * Execute pinned message 10 by 10 --- src/hooks/usePinnedEvents.ts | 20 ++++--- src/utils/promise.ts | 15 +++++ .../right_panel/PinnedMessagesCard-test.tsx | 15 +++++ .../PinnedMessagesCard-test.tsx.snap | 4 +- test/utils/promise-test.ts | 57 +++++++++++++++++++ 5 files changed, 100 insertions(+), 11 deletions(-) create mode 100644 test/utils/promise-test.ts diff --git a/src/hooks/usePinnedEvents.ts b/src/hooks/usePinnedEvents.ts index dd01ecb6ad..a29c65625c 100644 --- a/src/hooks/usePinnedEvents.ts +++ b/src/hooks/usePinnedEvents.ts @@ -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); } /** @@ -173,12 +176,11 @@ export function useFetchedPinnedEvents(room: Room, pinnedEventIds: string[]): Ar const cli = useMatrixClientContext(); return useAsyncMemo( - () => - Promise.all( - pinnedEventIds.map( - async (eventId): Promise => 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, ); diff --git a/src/utils/promise.ts b/src/utils/promise.ts index bceb2cc3cc..58dfdc8cd9 100644 --- a/src/utils/promise.ts +++ b/src/utils/promise.ts @@ -40,3 +40,18 @@ export async function retry( } 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(funcs: Array<() => Promise>, batchSize: number): Promise { + 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; +} diff --git a/test/components/views/right_panel/PinnedMessagesCard-test.tsx b/test/components/views/right_panel/PinnedMessagesCard-test.tsx index 9fd212b43c..8f8ffa3520 100644 --- a/test/components/views/right_panel/PinnedMessagesCard-test.tsx +++ b/test/components/views/right_panel/PinnedMessagesCard-test.tsx @@ -196,6 +196,21 @@ describe("", () => { 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([], []); diff --git a/test/components/views/right_panel/__snapshots__/PinnedMessagesCard-test.tsx.snap b/test/components/views/right_panel/__snapshots__/PinnedMessagesCard-test.tsx.snap index a055fdcca8..95573aa55e 100644 --- a/test/components/views/right_panel/__snapshots__/PinnedMessagesCard-test.tsx.snap +++ b/test/components/views/right_panel/__snapshots__/PinnedMessagesCard-test.tsx.snap @@ -358,7 +358,7 @@ exports[` 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" @@ -424,7 +424,7 @@ exports[` 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" diff --git a/test/utils/promise-test.ts b/test/utils/promise-test.ts new file mode 100644 index 0000000000..6733c2ae99 --- /dev/null +++ b/test/utils/promise-test.ts @@ -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((resolve) => { + promise1Called = true; + resolve(1); + }); + let promise2Called = false; + const promise2 = () => + new Promise((resolve) => { + promise2Called = true; + setTimeout(() => { + resolve(2); + }, 10); + }); + + let promise3Called = false; + const promise3 = () => + new Promise((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]); + }); + }); +});