From 299d7baf8b40acace482932db2a176620d6d4824 Mon Sep 17 00:00:00 2001 From: Florian Duros Date: Thu, 30 Oct 2025 15:55:15 +0100 Subject: [PATCH] Avoid excessive re-render of room list and member list (#31131) * fix(list view): avoid re-create `onFocus` function at each render of the child items * fix(room list): update `onFocus` signature * fix(member list): update `onFocus` signature * fix(room list): avoid re-render at the beginning and end of the scroll * test(room list): remove scrolling test and props * test(member list): update member tile view tests * test(room list): update `ListView` focus test * test(member list): add `onFocus` test for member list tile --- src/components/utils/ListView.tsx | 33 ++++++----- .../views/rooms/MemberList/MemberListView.tsx | 4 +- .../MemberList/tiles/RoomMemberTileView.tsx | 9 ++- .../tiles/ThreePidInviteTileView.tsx | 9 ++- .../views/rooms/RoomListPanel/RoomList.tsx | 9 +-- .../rooms/RoomListPanel/RoomListItemView.tsx | 15 ++--- .../RoomListPanel/RoomListItemView-test.tsx | 23 -------- .../rooms/memberlist/MemberTileView-test.tsx | 55 ++++++++++++++++--- .../components/views/utils/ListView-test.tsx | 9 ++- 9 files changed, 97 insertions(+), 69 deletions(-) diff --git a/src/components/utils/ListView.tsx b/src/components/utils/ListView.tsx index f99b7ab944..ca45b703a3 100644 --- a/src/components/utils/ListView.tsx +++ b/src/components/utils/ListView.tsx @@ -42,7 +42,7 @@ export interface IListViewProps index: number, item: Item, context: ListContext, - onFocus: (e: React.FocusEvent) => void, + onFocus: (item: Item, e: React.FocusEvent) => void, ) => JSX.Element; /** @@ -230,19 +230,26 @@ export function ListView(props: IListViewProps): JSX.Element => { - const onFocus = (e: React.FocusEvent): void => { - // If one of the item components has been focused directly, set the focused and tabIndex state - // and stop propagation so the ListViews onFocus doesn't also handle it. - const key = getItemKey(item); - setIsFocused(true); - setTabIndexKey(key); - e.stopPropagation(); - }; - return getItemComponent(index, item, context, onFocus); + /** + * Focus handler passed to each item component. + * Don't declare inside getItemComponent to avoid re-creating on each render. + */ + const onFocusForGetItemComponent = useCallback( + (item: Item, e: React.FocusEvent) => { + // If one of the item components has been focused directly, set the focused and tabIndex state + // and stop propagation so the ListViews onFocus doesn't also handle it. + const key = getItemKey(item); + setIsFocused(true); + setTabIndexKey(key); + e.stopPropagation(); }, - [getItemComponent, getItemKey], + [getItemKey], + ); + + const getItemComponentInternal = useCallback( + (index: number, item: Item, context: ListContext): JSX.Element => + getItemComponent(index, item, context, onFocusForGetItemComponent), + [getItemComponent, onFocusForGetItemComponent], ); /** * Handles focus events on the list. diff --git a/src/components/views/rooms/MemberList/MemberListView.tsx b/src/components/views/rooms/MemberList/MemberListView.tsx index 1193e245c5..90c77fd70c 100644 --- a/src/components/views/rooms/MemberList/MemberListView.tsx +++ b/src/components/views/rooms/MemberList/MemberListView.tsx @@ -45,7 +45,7 @@ const MemberListView: React.FC = (props: IProps) => { index: number, item: MemberWithSeparator, context: ListContext, - onFocus: (e: React.FocusEvent) => void, + onFocus: (item: MemberWithSeparator, e: React.FocusEvent) => void, ): JSX.Element => { const itemKey = getItemKey(item); const isRovingItem = itemKey === context.tabIndexKey; @@ -55,6 +55,7 @@ const MemberListView: React.FC = (props: IProps) => { } else if (item.member) { return ( = (props: IProps) => { } else { return ( void; + onFocus: (item: MemberWithSeparator, e: React.FocusEvent) => void; } export function RoomMemberTileView(props: IProps): JSX.Element { @@ -60,7 +65,7 @@ export function RoomMemberTileView(props: IProps): JSX.Element { return ( props.onFocus(props.item, e)} avatarJsx={av} presenceJsx={presenceJSX} nameJsx={nameJSX} diff --git a/src/components/views/rooms/MemberList/tiles/ThreePidInviteTileView.tsx b/src/components/views/rooms/MemberList/tiles/ThreePidInviteTileView.tsx index e4f5d1dbaf..22be2fd4af 100644 --- a/src/components/views/rooms/MemberList/tiles/ThreePidInviteTileView.tsx +++ b/src/components/views/rooms/MemberList/tiles/ThreePidInviteTileView.tsx @@ -12,14 +12,19 @@ import { type ThreePIDInvite } from "../../../../../models/rooms/ThreePIDInvite" import BaseAvatar from "../../../avatars/BaseAvatar"; import { MemberTileView } from "./common/MemberTileView"; import { InvitedIconView } from "./common/InvitedIconView"; +import { type MemberWithSeparator } from "../../../../viewmodels/memberlist/MemberListViewModel"; interface Props { + /** + * Needed for `onFocus` + */ + item: MemberWithSeparator; threePidInvite: ThreePIDInvite; memberIndex: number; memberCount: number; focused?: boolean; tabIndex?: number; - onFocus: (e: React.FocusEvent) => void; + onFocus: (item: MemberWithSeparator, e: React.FocusEvent) => void; } export function ThreePidInviteTileView(props: Props): JSX.Element { @@ -40,7 +45,7 @@ export function ThreePidInviteTileView(props: Props): JSX.Element { iconJsx={iconJsx} focused={props.focused} tabIndex={props.tabIndex} - onFocus={props.onFocus} + onFocus={(e) => props.onFocus(props.item, e)} /> ); } diff --git a/src/components/views/rooms/RoomListPanel/RoomList.tsx b/src/components/views/rooms/RoomListPanel/RoomList.tsx index 4aae621d96..c1c081ca9c 100644 --- a/src/components/views/rooms/RoomListPanel/RoomList.tsx +++ b/src/components/views/rooms/RoomListPanel/RoomList.tsx @@ -5,7 +5,7 @@ * Please see LICENSE files in the repository root for full details. */ -import React, { useCallback, useRef, useState, type JSX } from "react"; +import React, { useCallback, useRef, type JSX } from "react"; import { type Room } from "matrix-js-sdk/src/matrix"; import { type ScrollIntoViewLocation } from "react-virtuoso"; import { isEqual } from "lodash"; @@ -44,7 +44,6 @@ export function RoomList({ vm: { roomsResult, activeIndex } }: RoomListProps): J const lastSpaceId = useRef(undefined); const lastFilterKeys = useRef(undefined); const roomCount = roomsResult.rooms.length; - const [isScrolling, setIsScrolling] = useState(false); const getItemComponent = useCallback( ( index: number, @@ -53,7 +52,7 @@ export function RoomList({ vm: { roomsResult, activeIndex } }: RoomListProps): J spaceId: string; filterKeys: FilterKey[] | undefined; }>, - onFocus: (e: React.FocusEvent) => void, + onFocus: (item: Room, e: React.FocusEvent) => void, ): JSX.Element => { const itemKey = item.roomId; const isRovingItem = itemKey === context.tabIndexKey; @@ -69,11 +68,10 @@ export function RoomList({ vm: { roomsResult, activeIndex } }: RoomListProps): J roomIndex={index} roomCount={roomCount} onFocus={onFocus} - listIsScrolling={isScrolling} /> ); }, - [activeIndex, roomCount, isScrolling], + [activeIndex, roomCount], ); const getItemKey = useCallback((item: Room): string => { @@ -129,7 +127,6 @@ export function RoomList({ vm: { roomsResult, activeIndex } }: RoomListProps): J getItemKey={getItemKey} isItemFocusable={() => true} onKeyDown={keyDownCallback} - isScrolling={setIsScrolling} increaseViewportBy={{ bottom: EXTENDED_VIEWPORT_HEIGHT, top: EXTENDED_VIEWPORT_HEIGHT, diff --git a/src/components/views/rooms/RoomListPanel/RoomListItemView.tsx b/src/components/views/rooms/RoomListPanel/RoomListItemView.tsx index a18cd337be..53c138eae4 100644 --- a/src/components/views/rooms/RoomListPanel/RoomListItemView.tsx +++ b/src/components/views/rooms/RoomListPanel/RoomListItemView.tsx @@ -16,7 +16,7 @@ import { NotificationDecoration } from "../NotificationDecoration"; import { RoomAvatarView } from "../../avatars/RoomAvatarView"; import { RoomListItemContextMenuView } from "./RoomListItemContextMenuView"; -interface RoomListItemViewProps extends React.HTMLAttributes { +interface RoomListItemViewProps extends Omit, "onFocus"> { /** * The room to display */ @@ -32,7 +32,7 @@ interface RoomListItemViewProps extends React.HTMLAttributes /** * A callback that indicates the item has received focus */ - onFocus: (e: React.FocusEvent) => void; + onFocus: (room: Room, e: React.FocusEvent) => void; /** * The index of the room in the list */ @@ -41,10 +41,6 @@ interface RoomListItemViewProps extends React.HTMLAttributes * The total number of rooms in the list */ roomCount: number; - /** - * Whether the list is currently scrolling - */ - listIsScrolling: boolean; } /** @@ -57,7 +53,6 @@ export const RoomListItemView = memo(function RoomListItemView({ onFocus, roomIndex: index, roomCount: count, - listIsScrolling, ...props }: RoomListItemViewProps): JSX.Element { const ref = useRef(null); @@ -100,7 +95,7 @@ export const RoomListItemView = memo(function RoomListItemView({ aria-selected={isSelected} aria-label={vm.a11yLabel} onClick={() => vm.openRoom()} - onFocus={onFocus} + onFocus={(e: React.FocusEvent) => onFocus(room, e)} onMouseOver={() => setHover(true)} onMouseOut={() => setHover(false)} onBlur={() => setHover(false)} @@ -148,9 +143,7 @@ export const RoomListItemView = memo(function RoomListItemView({ // Rendering multiple context menus can causes crashes in radix upstream, // See https://github.com/radix-ui/primitives/issues/2717. - // We also don't need the context menu while scrolling so can improve scroll performance - // by not rendering it. - if (!vm.showContextMenu || listIsScrolling) return content; + if (!vm.showContextMenu) return content; return ( ", () => { onFocus={jest.fn()} roomIndex={0} roomCount={1} - listIsScrolling={false} />, ); @@ -193,26 +192,4 @@ describe("", () => { await user.keyboard("{Escape}"); expect(screen.queryByRole("menu")).toBeNull(); }); - - test("should not render context menu when list is scrolling", async () => { - const user = userEvent.setup(); - - mocked(useRoomListItemViewModel).mockReturnValue({ - ...defaultValue, - showContextMenu: true, - }); - - renderRoomListItem({ - listIsScrolling: true, - }); - - const button = screen.getByRole("option", { name: `Open room ${room.name}` }); - await user.pointer([{ target: button }, { keys: "[MouseRight]", target: button }]); - - // Context menu should not appear when scrolling - expect(screen.queryByRole("menu")).toBeNull(); - - // But the room item itself should still be rendered - expect(button).toBeInTheDocument(); - }); }); diff --git a/test/unit-tests/components/views/rooms/memberlist/MemberTileView-test.tsx b/test/unit-tests/components/views/rooms/memberlist/MemberTileView-test.tsx index e501f24efe..b82caced89 100644 --- a/test/unit-tests/components/views/rooms/memberlist/MemberTileView-test.tsx +++ b/test/unit-tests/components/views/rooms/memberlist/MemberTileView-test.tsx @@ -17,13 +17,16 @@ import * as TestUtils from "../../../../../test-utils"; import { type RoomMember } from "../../../../../../src/models/rooms/RoomMember"; import { getPending3PidInvites, + type MemberWithSeparator, sdkRoomMemberToRoomMember, } from "../../../../../../src/components/viewmodels/memberlist/MemberListViewModel"; import { RoomMemberTileView } from "../../../../../../src/components/views/rooms/MemberList/tiles/RoomMemberTileView"; import { ThreePidInviteTileView } from "../../../../../../src/components/views/rooms/MemberList/tiles/ThreePidInviteTileView"; +import { type ThreePIDInvite } from "../../../../../../src/models/rooms/ThreePIDInvite"; describe("MemberTileView", () => { describe("RoomMemberTileView", () => { + const item = {} as { member: RoomMember }; let matrixClient: MatrixClient; let member: RoomMember; @@ -32,11 +35,12 @@ describe("MemberTileView", () => { mocked(matrixClient.isRoomEncrypted).mockReturnValue(true); const sdkMember = new SdkRoomMember("roomId", matrixClient.getUserId()!); member = sdkRoomMemberToRoomMember(sdkMember)!.member!; + item.member = member; }); it("should not display an E2EIcon when the e2E status = normal", () => { const { container } = render( - , + , ); const e2eIcon = container.querySelector(".mx_E2EIconView"); expect(e2eIcon).toBeNull(); @@ -50,7 +54,7 @@ describe("MemberTileView", () => { } as unknown as UserVerificationStatus); const { container } = render( - , + , ); await waitFor(async () => { await userEvent.hover(container.querySelector(".mx_E2EIcon")!); @@ -73,7 +77,7 @@ describe("MemberTileView", () => { } as DeviceVerificationStatus); const { container } = render( - , + , ); await waitFor(async () => { @@ -88,33 +92,46 @@ describe("MemberTileView", () => { it("renders user labels correctly", async () => { member.powerLevel = 50; const { container: container1 } = render( - , + , ); expect(container1).toHaveTextContent("Moderator"); member.powerLevel = 100; const { container: container2 } = render( - , + , ); expect(container2).toHaveTextContent("Admin"); member.powerLevel = Infinity; const { container: container3 } = render( - , + , ); expect(container3).toHaveTextContent("Owner"); member.isInvite = true; const { container: container4 } = render( - , + , ); expect(container4).toHaveTextContent("Invited"); }); + + it("should call onFocus handler when focused", async () => { + const user = userEvent.setup(); + const onFocus = jest.fn(); + render(); + + const button = screen.getByRole("option", { name: member.userId }); + await user.click(button); + + expect(onFocus).toHaveBeenCalledWith(item, expect.anything()); + }); }); describe("ThreePidInviteTileView", () => { + const member = {} as MemberWithSeparator; let cli: MatrixClient; let room: Room; + let threePidInvite: ThreePIDInvite; beforeEach(() => { cli = TestUtils.stubClient(); @@ -123,13 +140,14 @@ describe("MemberTileView", () => { TestUtils.mkThirdPartyInviteEvent(cli.getSafeUserId(), "Foobar", room.roomId), { toStartOfTimeline: false, addToState: true }, ); + threePidInvite = getPending3PidInvites(room)[0].threePidInvite!; }); it("renders ThreePidInvite correctly", async () => { - const [{ threePidInvite }] = getPending3PidInvites(room); const { container } = render( { ); expect(container).toMatchSnapshot(); }); + + it("should call onFocus handler when focused", async () => { + const user = userEvent.setup(); + const onFocus = jest.fn(); + render( + , + ); + + const button = screen.getByRole("option", { name: threePidInvite.event.getContent().display_name }); + await user.click(button); + + expect(onFocus).toHaveBeenCalledWith(member, expect.anything()); + }); }); }); diff --git a/test/unit-tests/components/views/utils/ListView-test.tsx b/test/unit-tests/components/views/utils/ListView-test.tsx index acf4fa7788..0cae681f73 100644 --- a/test/unit-tests/components/views/utils/ListView-test.tsx +++ b/test/unit-tests/components/views/utils/ListView-test.tsx @@ -363,7 +363,12 @@ describe("ListView", () => { const mockOnClick = jest.fn(); mockGetItemComponent.mockImplementation( - (index: number, item: TestItemWithSeparator, context: any, onFocus: (e: React.FocusEvent) => void) => { + ( + index: number, + item: TestItemWithSeparator, + context: any, + onFocus: (item: TestItemWithSeparator, e: React.FocusEvent) => void, + ) => { const itemKey = typeof item === "string" ? item : item.id; const isFocused = context.tabIndexKey === itemKey; return ( @@ -372,7 +377,7 @@ describe("ListView", () => { data-testid={`row-${index}`} tabIndex={isFocused ? 0 : -1} onClick={() => mockOnClick(item)} - onFocus={onFocus} + onFocus={(e) => onFocus(item, e)} > {item === SEPARATOR_ITEM ? "---" : (item as TestItem).name}