Skip to content

Commit 299d7ba

Browse files
floriandurosflorianduros
authored
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
1 parent f297282 commit 299d7ba

File tree

9 files changed

+97
-69
lines changed

9 files changed

+97
-69
lines changed

‎src/components/utils/ListView.tsx‎

Lines changed: 20 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -42,7 +42,7 @@ export interface IListViewProps<Item, Context>
4242
index: number,
4343
item: Item,
4444
context: ListContext<Context>,
45-
onFocus: (e: React.FocusEvent) => void,
45+
onFocus: (item: Item, e: React.FocusEvent) => void,
4646
) => JSX.Element;
4747

4848
/**
@@ -230,19 +230,26 @@ export function ListView<Item, Context = any>(props: IListViewProps<Item, Contex
230230
virtuosoDomRef.current = element;
231231
}, []);
232232

233-
const getItemComponentInternal = useCallback(
234-
(index: number, item: Item, context: ListContext<Context>): JSX.Element => {
235-
const onFocus = (e: React.FocusEvent): void => {
236-
// If one of the item components has been focused directly, set the focused and tabIndex state
237-
// and stop propagation so the ListViews onFocus doesn't also handle it.
238-
const key = getItemKey(item);
239-
setIsFocused(true);
240-
setTabIndexKey(key);
241-
e.stopPropagation();
242-
};
243-
return getItemComponent(index, item, context, onFocus);
233+
/**
234+
* Focus handler passed to each item component.
235+
* Don't declare inside getItemComponent to avoid re-creating on each render.
236+
*/
237+
const onFocusForGetItemComponent = useCallback(
238+
(item: Item, e: React.FocusEvent) => {
239+
// If one of the item components has been focused directly, set the focused and tabIndex state
240+
// and stop propagation so the ListViews onFocus doesn't also handle it.
241+
const key = getItemKey(item);
242+
setIsFocused(true);
243+
setTabIndexKey(key);
244+
e.stopPropagation();
244245
},
245-
[getItemComponent, getItemKey],
246+
[getItemKey],
247+
);
248+
249+
const getItemComponentInternal = useCallback(
250+
(index: number, item: Item, context: ListContext<Context>): JSX.Element =>
251+
getItemComponent(index, item, context, onFocusForGetItemComponent),
252+
[getItemComponent, onFocusForGetItemComponent],
246253
);
247254
/**
248255
* Handles focus events on the list.

‎src/components/views/rooms/MemberList/MemberListView.tsx‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -45,7 +45,7 @@ const MemberListView: React.FC<IProps> = (props: IProps) => {
4545
index: number,
4646
item: MemberWithSeparator,
4747
context: ListContext<any>,
48-
onFocus: (e: React.FocusEvent) => void,
48+
onFocus: (item: MemberWithSeparator, e: React.FocusEvent) => void,
4949
): JSX.Element => {
5050
const itemKey = getItemKey(item);
5151
const isRovingItem = itemKey === context.tabIndexKey;
@@ -55,6 +55,7 @@ const MemberListView: React.FC<IProps> = (props: IProps) => {
5555
} else if (item.member) {
5656
return (
5757
<RoomMemberTileView
58+
item={item}
5859
member={item.member}
5960
showPresence={isPresenceEnabled}
6061
focused={focused}
@@ -67,6 +68,7 @@ const MemberListView: React.FC<IProps> = (props: IProps) => {
6768
} else {
6869
return (
6970
<ThreePidInviteTileView
71+
item={item}
7072
threePidInvite={item.threePidInvite}
7173
focused={focused}
7274
tabIndex={isRovingItem ? 0 : -1}

‎src/components/views/rooms/MemberList/tiles/RoomMemberTileView.tsx‎

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -16,15 +16,20 @@ import BaseAvatar from "../../../avatars/BaseAvatar";
1616
import { _t } from "../../../../../languageHandler";
1717
import { MemberTileView } from "./common/MemberTileView";
1818
import { InvitedIconView } from "./common/InvitedIconView";
19+
import { type MemberWithSeparator } from "../../../../viewmodels/memberlist/MemberListViewModel";
1920

2021
interface IProps {
22+
/**
23+
* Needed for `onFocus`
24+
*/
25+
item: MemberWithSeparator;
2126
member: RoomMember;
2227
index: number;
2328
memberCount: number;
2429
showPresence?: boolean;
2530
focused?: boolean;
2631
tabIndex?: number;
27-
onFocus: (e: React.FocusEvent) => void;
32+
onFocus: (item: MemberWithSeparator, e: React.FocusEvent) => void;
2833
}
2934

3035
export function RoomMemberTileView(props: IProps): JSX.Element {
@@ -60,7 +65,7 @@ export function RoomMemberTileView(props: IProps): JSX.Element {
6065
return (
6166
<MemberTileView
6267
onClick={vm.onClick}
63-
onFocus={props.onFocus}
68+
onFocus={(e) => props.onFocus(props.item, e)}
6469
avatarJsx={av}
6570
presenceJsx={presenceJSX}
6671
nameJsx={nameJSX}

‎src/components/views/rooms/MemberList/tiles/ThreePidInviteTileView.tsx‎

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -12,14 +12,19 @@ import { type ThreePIDInvite } from "../../../../../models/rooms/ThreePIDInvite"
1212
import BaseAvatar from "../../../avatars/BaseAvatar";
1313
import { MemberTileView } from "./common/MemberTileView";
1414
import { InvitedIconView } from "./common/InvitedIconView";
15+
import { type MemberWithSeparator } from "../../../../viewmodels/memberlist/MemberListViewModel";
1516

1617
interface Props {
18+
/**
19+
* Needed for `onFocus`
20+
*/
21+
item: MemberWithSeparator;
1722
threePidInvite: ThreePIDInvite;
1823
memberIndex: number;
1924
memberCount: number;
2025
focused?: boolean;
2126
tabIndex?: number;
22-
onFocus: (e: React.FocusEvent) => void;
27+
onFocus: (item: MemberWithSeparator, e: React.FocusEvent) => void;
2328
}
2429

2530
export function ThreePidInviteTileView(props: Props): JSX.Element {
@@ -40,7 +45,7 @@ export function ThreePidInviteTileView(props: Props): JSX.Element {
4045
iconJsx={iconJsx}
4146
focused={props.focused}
4247
tabIndex={props.tabIndex}
43-
onFocus={props.onFocus}
48+
onFocus={(e) => props.onFocus(props.item, e)}
4449
/>
4550
);
4651
}

‎src/components/views/rooms/RoomListPanel/RoomList.tsx‎

Lines changed: 3 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@
55
* Please see LICENSE files in the repository root for full details.
66
*/
77

8-
import React, { useCallback, useRef, useState, type JSX } from "react";
8+
import React, { useCallback, useRef, type JSX } from "react";
99
import { type Room } from "matrix-js-sdk/src/matrix";
1010
import { type ScrollIntoViewLocation } from "react-virtuoso";
1111
import { isEqual } from "lodash";
@@ -44,7 +44,6 @@ export function RoomList({ vm: { roomsResult, activeIndex } }: RoomListProps): J
4444
const lastSpaceId = useRef<string | undefined>(undefined);
4545
const lastFilterKeys = useRef<FilterKey[] | undefined>(undefined);
4646
const roomCount = roomsResult.rooms.length;
47-
const [isScrolling, setIsScrolling] = useState(false);
4847
const getItemComponent = useCallback(
4948
(
5049
index: number,
@@ -53,7 +52,7 @@ export function RoomList({ vm: { roomsResult, activeIndex } }: RoomListProps): J
5352
spaceId: string;
5453
filterKeys: FilterKey[] | undefined;
5554
}>,
56-
onFocus: (e: React.FocusEvent) => void,
55+
onFocus: (item: Room, e: React.FocusEvent) => void,
5756
): JSX.Element => {
5857
const itemKey = item.roomId;
5958
const isRovingItem = itemKey === context.tabIndexKey;
@@ -69,11 +68,10 @@ export function RoomList({ vm: { roomsResult, activeIndex } }: RoomListProps): J
6968
roomIndex={index}
7069
roomCount={roomCount}
7170
onFocus={onFocus}
72-
listIsScrolling={isScrolling}
7371
/>
7472
);
7573
},
76-
[activeIndex, roomCount, isScrolling],
74+
[activeIndex, roomCount],
7775
);
7876

7977
const getItemKey = useCallback((item: Room): string => {
@@ -129,7 +127,6 @@ export function RoomList({ vm: { roomsResult, activeIndex } }: RoomListProps): J
129127
getItemKey={getItemKey}
130128
isItemFocusable={() => true}
131129
onKeyDown={keyDownCallback}
132-
isScrolling={setIsScrolling}
133130
increaseViewportBy={{
134131
bottom: EXTENDED_VIEWPORT_HEIGHT,
135132
top: EXTENDED_VIEWPORT_HEIGHT,

‎src/components/views/rooms/RoomListPanel/RoomListItemView.tsx‎

Lines changed: 4 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,7 @@ import { NotificationDecoration } from "../NotificationDecoration";
1616
import { RoomAvatarView } from "../../avatars/RoomAvatarView";
1717
import { RoomListItemContextMenuView } from "./RoomListItemContextMenuView";
1818

19-
interface RoomListItemViewProps extends React.HTMLAttributes<HTMLButtonElement> {
19+
interface RoomListItemViewProps extends Omit<React.HTMLAttributes<HTMLButtonElement>, "onFocus"> {
2020
/**
2121
* The room to display
2222
*/
@@ -32,7 +32,7 @@ interface RoomListItemViewProps extends React.HTMLAttributes<HTMLButtonElement>
3232
/**
3333
* A callback that indicates the item has received focus
3434
*/
35-
onFocus: (e: React.FocusEvent) => void;
35+
onFocus: (room: Room, e: React.FocusEvent) => void;
3636
/**
3737
* The index of the room in the list
3838
*/
@@ -41,10 +41,6 @@ interface RoomListItemViewProps extends React.HTMLAttributes<HTMLButtonElement>
4141
* The total number of rooms in the list
4242
*/
4343
roomCount: number;
44-
/**
45-
* Whether the list is currently scrolling
46-
*/
47-
listIsScrolling: boolean;
4844
}
4945

5046
/**
@@ -57,7 +53,6 @@ export const RoomListItemView = memo(function RoomListItemView({
5753
onFocus,
5854
roomIndex: index,
5955
roomCount: count,
60-
listIsScrolling,
6156
...props
6257
}: RoomListItemViewProps): JSX.Element {
6358
const ref = useRef<HTMLButtonElement>(null);
@@ -100,7 +95,7 @@ export const RoomListItemView = memo(function RoomListItemView({
10095
aria-selected={isSelected}
10196
aria-label={vm.a11yLabel}
10297
onClick={() => vm.openRoom()}
103-
onFocus={onFocus}
98+
onFocus={(e: React.FocusEvent<HTMLButtonElement>) => onFocus(room, e)}
10499
onMouseOver={() => setHover(true)}
105100
onMouseOut={() => setHover(false)}
106101
onBlur={() => setHover(false)}
@@ -148,9 +143,7 @@ export const RoomListItemView = memo(function RoomListItemView({
148143

149144
// Rendering multiple context menus can causes crashes in radix upstream,
150145
// See https://github.com/radix-ui/primitives/issues/2717.
151-
// We also don't need the context menu while scrolling so can improve scroll performance
152-
// by not rendering it.
153-
if (!vm.showContextMenu || listIsScrolling) return content;
146+
if (!vm.showContextMenu) return content;
154147

155148
return (
156149
<RoomListItemContextMenuView

‎test/unit-tests/components/views/rooms/RoomListPanel/RoomListItemView-test.tsx‎

Lines changed: 0 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -129,7 +129,6 @@ describe("<RoomListItemView />", () => {
129129
onFocus={jest.fn()}
130130
roomIndex={0}
131131
roomCount={1}
132-
listIsScrolling={false}
133132
/>,
134133
);
135134

@@ -193,26 +192,4 @@ describe("<RoomListItemView />", () => {
193192
await user.keyboard("{Escape}");
194193
expect(screen.queryByRole("menu")).toBeNull();
195194
});
196-
197-
test("should not render context menu when list is scrolling", async () => {
198-
const user = userEvent.setup();
199-
200-
mocked(useRoomListItemViewModel).mockReturnValue({
201-
...defaultValue,
202-
showContextMenu: true,
203-
});
204-
205-
renderRoomListItem({
206-
listIsScrolling: true,
207-
});
208-
209-
const button = screen.getByRole("option", { name: `Open room ${room.name}` });
210-
await user.pointer([{ target: button }, { keys: "[MouseRight]", target: button }]);
211-
212-
// Context menu should not appear when scrolling
213-
expect(screen.queryByRole("menu")).toBeNull();
214-
215-
// But the room item itself should still be rendered
216-
expect(button).toBeInTheDocument();
217-
});
218195
});

0 commit comments

Comments
 (0)