New room list: avoid extra render for room list item (#29752)

* fix: avoid extra render in the new room list

* fix: listen to room name changes

* fix: trigger render when notification state change

* test: fix room list item tests

* chore: fix typo `RoomNotificationState.isUnsentMessage`

* refactor: move `isNotificationDecorationVisible` into `useRoomListItemViewModel`

* refactor: recalculate notification values on notification state changes

* refactor: rename `isNotificationDecorationVisible` to `showNotificationDecoration`

* test: add test for room list item view

* test: add notification tests in room list item vm

* fix: listen to notification updates in `NotificationDecoration`

* test: update notification decoration tests

* refactor: display notification decoration according to vm

* test: update room list item view tests

* fix: a11y label computation after room name change

* refactor: improve notification handling
This commit is contained in:
Florian Duros 2025-04-16 23:40:36 +02:00 committed by GitHub
parent 6767e4d6ad
commit fd455179f7
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
9 changed files with 290 additions and 55 deletions

View File

@ -5,7 +5,7 @@
* Please see LICENSE files in the repository root for full details.
*/
import { useCallback, useMemo } from "react";
import { useCallback, useEffect, useMemo, useState } from "react";
import { type Room, RoomEvent } from "matrix-js-sdk/src/matrix";
import dispatcher from "../../../dispatcher/dispatcher";
@ -16,12 +16,17 @@ import { _t } from "../../../languageHandler";
import { type RoomNotificationState } from "../../../stores/notifications/RoomNotificationState";
import { RoomNotificationStateStore } from "../../../stores/notifications/RoomNotificationStateStore";
import { useMatrixClientContext } from "../../../contexts/MatrixClientContext";
import { useEventEmitterState } from "../../../hooks/useEventEmitter";
import { useEventEmitterState, useTypedEventEmitter } from "../../../hooks/useEventEmitter";
import { DefaultTagID } from "../../../stores/room-list/models";
import { useCall, useConnectionState, useParticipantCount } from "../../../hooks/useCall";
import { type ConnectionState } from "../../../models/Call";
import { NotificationStateEvents } from "../../../stores/notifications/NotificationState";
export interface RoomListItemViewState {
/**
* The name of the room.
*/
name: string;
/**
* Whether the hover menu should be shown.
*/
@ -55,6 +60,10 @@ export interface RoomListItemViewState {
* Whether there are participants in the call.
*/
hasParticipantInCall: boolean;
/**
* Whether the notification decoration should be shown.
*/
showNotificationDecoration: boolean;
}
/**
@ -65,11 +74,23 @@ export function useRoomListItemViewModel(room: Room): RoomListItemViewState {
const matrixClient = useMatrixClientContext();
const roomTags = useEventEmitterState(room, RoomEvent.Tags, () => room.tags);
const isArchived = Boolean(roomTags[DefaultTagID.Archived]);
const name = useEventEmitterState(room, RoomEvent.Name, () => room.name);
const notificationState = useMemo(() => RoomNotificationStateStore.instance.getRoomState(room), [room]);
const invited = notificationState.invited;
const a11yLabel = getA11yLabel(room, notificationState);
const isBold = notificationState.hasAnyNotificationOrActivity;
const [a11yLabel, setA11yLabel] = useState(getA11yLabel(name, notificationState));
const [{ isBold, invited, hasVisibleNotification }, setNotificationValues] = useState(
getNotificationValues(notificationState),
);
useEffect(() => {
setA11yLabel(getA11yLabel(name, notificationState));
}, [name, notificationState]);
// Listen to changes in the notification state and update the values
useTypedEventEmitter(notificationState, NotificationStateEvents.Update, () => {
setA11yLabel(getA11yLabel(name, notificationState));
setNotificationValues(getNotificationValues(notificationState));
});
// We don't want to show the hover menu if
// - there is an invitation for this room
@ -86,6 +107,8 @@ export function useRoomListItemViewModel(room: Room): RoomListItemViewState {
const hasParticipantInCall = useParticipantCount(call) > 0;
const callConnectionState = call ? connectionState : null;
const showNotificationDecoration = hasVisibleNotification || hasParticipantInCall;
// Actions
const openRoom = useCallback((): void => {
@ -97,6 +120,7 @@ export function useRoomListItemViewModel(room: Room): RoomListItemViewState {
}, [room]);
return {
name,
notificationState,
showHoverMenu,
openRoom,
@ -105,34 +129,59 @@ export function useRoomListItemViewModel(room: Room): RoomListItemViewState {
isVideoRoom,
callConnectionState,
hasParticipantInCall,
showNotificationDecoration,
};
}
/**
* Calculate the values from the notification state
* @param notificationState
*/
function getNotificationValues(notificationState: RoomNotificationState): {
computeA11yLabel: (name: string) => string;
isBold: boolean;
invited: boolean;
hasVisibleNotification: boolean;
} {
const invited = notificationState.invited;
const computeA11yLabel = (name: string): string => getA11yLabel(name, notificationState);
const isBold = notificationState.hasAnyNotificationOrActivity;
const hasVisibleNotification = notificationState.hasAnyNotificationOrActivity || notificationState.muted;
return {
computeA11yLabel,
isBold,
invited,
hasVisibleNotification,
};
}
/**
* Get the a11y label for the room list item
* @param room
* @param roomName
* @param notificationState
*/
function getA11yLabel(room: Room, notificationState: RoomNotificationState): string {
if (notificationState.isUnsetMessage) {
function getA11yLabel(roomName: string, notificationState: RoomNotificationState): string {
if (notificationState.isUnsentMessage) {
return _t("a11y|room_messsage_not_sent", {
roomName: room.name,
roomName,
});
} else if (notificationState.invited) {
return _t("a11y|room_n_unread_invite", {
roomName: room.name,
roomName,
});
} else if (notificationState.isMention) {
return _t("a11y|room_n_unread_messages_mentions", {
roomName: room.name,
roomName,
count: notificationState.count,
});
} else if (notificationState.hasUnreadCount) {
return _t("a11y|room_n_unread_messages", {
roomName: room.name,
roomName,
count: notificationState.count,
});
} else {
return _t("room_list|room|open_room", { roomName: room.name });
return _t("room_list|room|open_room", { roomName });
}
}

View File

@ -15,6 +15,8 @@ import { UnreadCounter, Unread } from "@vector-im/compound-web";
import { Flex } from "../../utils/Flex";
import { type RoomNotificationState } from "../../../stores/notifications/RoomNotificationState";
import { useTypedEventEmitterState } from "../../../hooks/useEventEmitter";
import { NotificationStateEvents } from "../../../stores/notifications/NotificationState";
interface NotificationDecorationProps extends HTMLProps<HTMLDivElement> {
/**
@ -35,16 +37,27 @@ export function NotificationDecoration({
hasVideoCall,
...props
}: NotificationDecorationProps): JSX.Element | null {
// Listen to the notification state and update the component when it changes
const {
hasAnyNotificationOrActivity,
isUnsetMessage,
isUnsentMessage,
invited,
isMention,
isActivityNotification,
isNotification,
count,
muted,
} = notificationState;
} = useTypedEventEmitterState(notificationState, NotificationStateEvents.Update, () => ({
hasAnyNotificationOrActivity: notificationState.hasAnyNotificationOrActivity,
isUnsentMessage: notificationState.isUnsentMessage,
invited: notificationState.invited,
isMention: notificationState.isMention,
isActivityNotification: notificationState.isActivityNotification,
isNotification: notificationState.isNotification,
count: notificationState.count,
muted: notificationState.muted,
}));
if (!hasAnyNotificationOrActivity && !muted && !hasVideoCall) return null;
return (
@ -55,7 +68,7 @@ export function NotificationDecoration({
{...props}
data-testid="notification-decoration"
>
{isUnsetMessage && <ErrorIcon width="20px" height="20px" fill="var(--cpd-color-icon-critical-primary)" />}
{isUnsentMessage && <ErrorIcon width="20px" height="20px" fill="var(--cpd-color-icon-critical-primary)" />}
{hasVideoCall && <VideoCallIcon width="20px" height="20px" fill="var(--cpd-color-icon-accent-primary)" />}
{invited && <EmailIcon width="20px" height="20px" fill="var(--cpd-color-icon-accent-primary)" />}
{isMention && <MentionIcon width="20px" height="20px" fill="var(--cpd-color-icon-accent-primary)" />}

View File

@ -5,7 +5,7 @@
* Please see LICENSE files in the repository root for full details.
*/
import React, { type JSX, useState } from "react";
import React, { type JSX, memo, useState } from "react";
import { type Room } from "matrix-js-sdk/src/matrix";
import classNames from "classnames";
@ -29,7 +29,11 @@ interface RoomListItemViewPropsProps extends React.HTMLAttributes<HTMLButtonElem
/**
* An item in the room list
*/
export function RoomListItemView({ room, isSelected, ...props }: RoomListItemViewPropsProps): JSX.Element {
export const RoomListItemView = memo(function RoomListItemView({
room,
isSelected,
...props
}: RoomListItemViewPropsProps): JSX.Element {
const vm = useRoomListItemViewModel(room);
const [isHover, setIsHover] = useState(false);
@ -38,9 +42,7 @@ export function RoomListItemView({ room, isSelected, ...props }: RoomListItemVie
// Using display: none; and then display:flex when hovered in CSS causes the menu to be misaligned
const showHoverDecoration = (isMenuOpen || isHover) && vm.showHoverMenu;
const isNotificationDecorationVisible =
!showHoverDecoration &&
(vm.notificationState.hasAnyNotificationOrActivity || vm.notificationState.muted || vm.hasParticipantInCall);
const isNotificationDecorationVisible = !showHoverDecoration && vm.showNotificationDecoration;
return (
<button
@ -71,8 +73,8 @@ export function RoomListItemView({ room, isSelected, ...props }: RoomListItemVie
justify="space-between"
>
{/* We truncate the room name when too long. Title here is to show the full name on hover */}
<span className="mx_RoomListItemView_roomName" title={room.name}>
{room.name}
<span className="mx_RoomListItemView_roomName" title={vm.name}>
{vm.name}
</span>
{showHoverDecoration ? (
<RoomListItemMenuView
@ -86,15 +88,17 @@ export function RoomListItemView({ room, isSelected, ...props }: RoomListItemVie
) : (
<>
{/* aria-hidden because we summarise the unread count/notification status in a11yLabel variable */}
<NotificationDecoration
notificationState={vm.notificationState}
aria-hidden={true}
hasVideoCall={vm.hasParticipantInCall}
/>
{vm.showNotificationDecoration && (
<NotificationDecoration
notificationState={vm.notificationState}
aria-hidden={true}
hasVideoCall={vm.hasParticipantInCall}
/>
)}
</>
)}
</Flex>
</Flex>
</button>
);
}
});

View File

@ -62,9 +62,9 @@ export class RoomNotificationState extends NotificationState implements IDestroy
}
/**
* True if the notification is an unset message.
* True if the notification is an unsent message.
*/
public get isUnsetMessage(): boolean {
public get isUnsentMessage(): boolean {
return this.level === NotificationLevel.Unsent;
}

View File

@ -19,6 +19,7 @@ import {
} from "../../../../../src/components/viewmodels/roomlist/utils";
import { RoomNotificationState } from "../../../../../src/stores/notifications/RoomNotificationState";
import { RoomNotificationStateStore } from "../../../../../src/stores/notifications/RoomNotificationStateStore";
import * as UseCallModule from "../../../../../src/hooks/useCall";
jest.mock("../../../../../src/components/viewmodels/roomlist/utils", () => ({
hasAccessToOptionsMenu: jest.fn().mockReturnValue(false),
@ -86,6 +87,49 @@ describe("RoomListItemViewModel", () => {
expect(vm.current.showHoverMenu).toBe(true);
});
describe("notification", () => {
let notificationState: RoomNotificationState;
beforeEach(() => {
notificationState = new RoomNotificationState(room, false);
jest.spyOn(RoomNotificationStateStore.instance, "getRoomState").mockReturnValue(notificationState);
});
it("should show notification decoration if there is call has participant", () => {
jest.spyOn(UseCallModule, "useParticipantCount").mockReturnValue(1);
const { result: vm } = renderHook(
() => useRoomListItemViewModel(room),
withClientContextRenderOptions(room.client),
);
expect(vm.current.showNotificationDecoration).toBe(true);
});
it.each([
{
label: "hasAnyNotificationOrActivity",
mock: () => jest.spyOn(notificationState, "hasAnyNotificationOrActivity", "get").mockReturnValue(true),
},
{ label: "muted", mock: () => jest.spyOn(notificationState, "muted", "get").mockReturnValue(true) },
])("should show notification decoration if $label=true", ({ mock }) => {
mock();
const { result: vm } = renderHook(
() => useRoomListItemViewModel(room),
withClientContextRenderOptions(room.client),
);
expect(vm.current.showNotificationDecoration).toBe(true);
});
it("should be bold if there is a notification", () => {
jest.spyOn(notificationState, "hasAnyNotificationOrActivity", "get").mockReturnValue(true);
const { result: vm } = renderHook(
() => useRoomListItemViewModel(room),
withClientContextRenderOptions(room.client),
);
expect(vm.current.isBold).toBe(true);
});
});
describe("a11yLabel", () => {
let notificationState: RoomNotificationState;
beforeEach(() => {
@ -96,7 +140,7 @@ describe("RoomListItemViewModel", () => {
it.each([
{
label: "unsent message",
mock: () => jest.spyOn(notificationState, "isUnsetMessage", "get").mockReturnValue(true),
mock: () => jest.spyOn(notificationState, "isUnsentMessage", "get").mockReturnValue(true),
expected: "Open room roomName with an unsent message.",
},
{

View File

@ -8,60 +8,94 @@
import React from "react";
import { render, screen } from "jest-matrix-react";
import { type RoomNotificationState } from "../../../../../src/stores/notifications/RoomNotificationState";
import { RoomNotificationState } from "../../../../../src/stores/notifications/RoomNotificationState";
import { NotificationDecoration } from "../../../../../src/components/views/rooms/NotificationDecoration";
import { createTestClient, mkStubRoom } from "../../../../test-utils";
describe("<NotificationDecoration />", () => {
it("should not render if RoomNotificationState.isSilent=true", () => {
const state = { hasAnyNotificationOrActivity: false } as RoomNotificationState;
render(<NotificationDecoration notificationState={state} hasVideoCall={false} />);
let roomNotificationState: RoomNotificationState;
beforeEach(() => {
const matrixClient = createTestClient();
const room = mkStubRoom("roomId", "roomName", matrixClient);
roomNotificationState = new RoomNotificationState(room, false);
});
it("should not render if RoomNotificationState.hasAnyNotificationOrActivity=true", () => {
jest.spyOn(roomNotificationState, "hasAnyNotificationOrActivity", "get").mockReturnValue(false);
render(<NotificationDecoration notificationState={roomNotificationState} hasVideoCall={false} />);
expect(screen.queryByTestId("notification-decoration")).toBeNull();
});
it("should render the unset message decoration", () => {
const state = { hasAnyNotificationOrActivity: true, isUnsetMessage: true } as RoomNotificationState;
const { asFragment } = render(<NotificationDecoration notificationState={state} hasVideoCall={false} />);
jest.spyOn(roomNotificationState, "hasAnyNotificationOrActivity", "get").mockReturnValue(true);
jest.spyOn(roomNotificationState, "isUnsentMessage", "get").mockReturnValue(true);
const { asFragment } = render(
<NotificationDecoration notificationState={roomNotificationState} hasVideoCall={false} />,
);
expect(asFragment()).toMatchSnapshot();
});
it("should render the invitation decoration", () => {
const state = { hasAnyNotificationOrActivity: true, invited: true } as RoomNotificationState;
const { asFragment } = render(<NotificationDecoration notificationState={state} hasVideoCall={false} />);
jest.spyOn(roomNotificationState, "hasAnyNotificationOrActivity", "get").mockReturnValue(true);
jest.spyOn(roomNotificationState, "invited", "get").mockReturnValue(true);
const { asFragment } = render(
<NotificationDecoration notificationState={roomNotificationState} hasVideoCall={false} />,
);
expect(asFragment()).toMatchSnapshot();
});
it("should render the mention decoration", () => {
const state = { hasAnyNotificationOrActivity: true, isMention: true, count: 1 } as RoomNotificationState;
const { asFragment } = render(<NotificationDecoration notificationState={state} hasVideoCall={false} />);
jest.spyOn(roomNotificationState, "hasAnyNotificationOrActivity", "get").mockReturnValue(true);
jest.spyOn(roomNotificationState, "isMention", "get").mockReturnValue(true);
jest.spyOn(roomNotificationState, "count", "get").mockReturnValue(1);
const { asFragment } = render(
<NotificationDecoration notificationState={roomNotificationState} hasVideoCall={false} />,
);
expect(asFragment()).toMatchSnapshot();
});
it("should render the notification decoration", () => {
const state = { hasAnyNotificationOrActivity: true, isNotification: true, count: 1 } as RoomNotificationState;
const { asFragment } = render(<NotificationDecoration notificationState={state} hasVideoCall={false} />);
jest.spyOn(roomNotificationState, "hasAnyNotificationOrActivity", "get").mockReturnValue(true);
jest.spyOn(roomNotificationState, "isNotification", "get").mockReturnValue(true);
jest.spyOn(roomNotificationState, "count", "get").mockReturnValue(1);
const { asFragment } = render(
<NotificationDecoration notificationState={roomNotificationState} hasVideoCall={false} />,
);
expect(asFragment()).toMatchSnapshot();
});
it("should render the notification decoration without count", () => {
const state = { hasAnyNotificationOrActivity: true, isNotification: true, count: 0 } as RoomNotificationState;
const { asFragment } = render(<NotificationDecoration notificationState={state} hasVideoCall={false} />);
jest.spyOn(roomNotificationState, "hasAnyNotificationOrActivity", "get").mockReturnValue(true);
jest.spyOn(roomNotificationState, "isNotification", "get").mockReturnValue(true);
jest.spyOn(roomNotificationState, "count", "get").mockReturnValue(0);
const { asFragment } = render(
<NotificationDecoration notificationState={roomNotificationState} hasVideoCall={false} />,
);
expect(asFragment()).toMatchSnapshot();
});
it("should render the activity decoration", () => {
const state = { hasAnyNotificationOrActivity: true, isActivityNotification: true } as RoomNotificationState;
const { asFragment } = render(<NotificationDecoration notificationState={state} hasVideoCall={false} />);
jest.spyOn(roomNotificationState, "hasAnyNotificationOrActivity", "get").mockReturnValue(true);
jest.spyOn(roomNotificationState, "isActivityNotification", "get").mockReturnValue(true);
const { asFragment } = render(
<NotificationDecoration notificationState={roomNotificationState} hasVideoCall={false} />,
);
expect(asFragment()).toMatchSnapshot();
});
it("should render the muted decoration", () => {
const state = { hasAnyNotificationOrActivity: true, muted: true } as RoomNotificationState;
const { asFragment } = render(<NotificationDecoration notificationState={state} hasVideoCall={false} />);
jest.spyOn(roomNotificationState, "hasAnyNotificationOrActivity", "get").mockReturnValue(true);
jest.spyOn(roomNotificationState, "muted", "get").mockReturnValue(true);
const { asFragment } = render(
<NotificationDecoration notificationState={roomNotificationState} hasVideoCall={false} />,
);
expect(asFragment()).toMatchSnapshot();
});
it("should render the video decoration", () => {
const state = { hasAnyNotificationOrActivity: false } as RoomNotificationState;
const { asFragment } = render(<NotificationDecoration notificationState={state} hasVideoCall={true} />);
jest.spyOn(roomNotificationState, "hasAnyNotificationOrActivity", "get").mockReturnValue(false);
const { asFragment } = render(
<NotificationDecoration notificationState={roomNotificationState} hasVideoCall={true} />,
);
expect(asFragment()).toMatchSnapshot();
});
});

View File

@ -28,7 +28,6 @@ describe("<RoomListItemView />", () => {
let defaultValue: RoomListItemViewState;
let matrixClient: MatrixClient;
let room: Room;
beforeEach(() => {
matrixClient = stubClient();
room = mkRoom(matrixClient, "room1");
@ -36,15 +35,22 @@ describe("<RoomListItemView />", () => {
DMRoomMap.makeShared(matrixClient);
jest.spyOn(DMRoomMap.shared(), "getUserIdForRoomId").mockReturnValue(null);
const notificationState = new RoomNotificationState(room, false);
jest.spyOn(notificationState, "hasAnyNotificationOrActivity", "get").mockReturnValue(true);
jest.spyOn(notificationState, "isNotification", "get").mockReturnValue(true);
jest.spyOn(notificationState, "count", "get").mockReturnValue(1);
defaultValue = {
openRoom: jest.fn(),
showHoverMenu: false,
notificationState: new RoomNotificationState(room, false),
notificationState,
a11yLabel: "Open room room1",
isBold: false,
isVideoRoom: false,
callConnectionState: null,
hasParticipantInCall: false,
name: room.name,
showNotificationDecoration: false,
};
mocked(useRoomListItemViewModel).mockReturnValue(defaultValue);
@ -84,4 +90,30 @@ describe("<RoomListItemView />", () => {
);
expect(asFragment()).toMatchSnapshot();
});
test("should display notification decoration", async () => {
mocked(useRoomListItemViewModel).mockReturnValue({
...defaultValue,
showNotificationDecoration: true,
});
const { asFragment } = render(<RoomListItemView room={room} isSelected={false} />);
expect(screen.getByTestId("notification-decoration")).toBeInTheDocument();
expect(asFragment()).toMatchSnapshot();
});
test("should not display notification decoration when hovered", async () => {
const user = userEvent.setup();
mocked(useRoomListItemViewModel).mockReturnValue({
...defaultValue,
showNotificationDecoration: true,
});
render(<RoomListItemView room={room} isSelected={false} />);
const listItem = screen.getByRole("button", { name: `Open room ${room.name}` });
await user.hover(listItem);
expect(screen.queryByRole("notification-decoration")).toBeNull();
});
});

View File

@ -47,6 +47,65 @@ exports[`<RoomListItemView /> should be selected if isSelected=true 1`] = `
</DocumentFragment>
`;
exports[`<RoomListItemView /> should display notification decoration 1`] = `
<DocumentFragment>
<button
aria-label="Open room room1"
aria-selected="false"
class="mx_RoomListItemView mx_RoomListItemView_notification_decoration"
type="button"
>
<div
class="mx_Flex mx_RoomListItemView_container"
style="--mx-flex-display: flex; --mx-flex-direction: row; --mx-flex-align: center; --mx-flex-justify: start; --mx-flex-gap: var(--cpd-space-3x); --mx-flex-wrap: nowrap;"
>
<span
aria-label="Avatar"
class="_avatar_1qbcf_8 mx_BaseAvatar"
data-color="3"
data-testid="avatar-img"
data-type="round"
style="--cpd-avatar-size: 32px;"
>
<img
alt=""
class="_image_1qbcf_41"
data-type="round"
height="32px"
loading="lazy"
referrerpolicy="no-referrer"
src="http://this.is.a.url/avatar.url/room.png"
width="32px"
/>
</span>
<div
class="mx_Flex mx_RoomListItemView_content"
style="--mx-flex-display: flex; --mx-flex-direction: row; --mx-flex-align: center; --mx-flex-justify: space-between; --mx-flex-gap: var(--cpd-space-3x); --mx-flex-wrap: nowrap;"
>
<span
class="mx_RoomListItemView_roomName"
title="room1"
>
room1
</span>
<div
aria-hidden="true"
class="mx_Flex"
data-testid="notification-decoration"
style="--mx-flex-display: flex; --mx-flex-direction: row; --mx-flex-align: center; --mx-flex-justify: center; --mx-flex-gap: var(--cpd-space-1-5x); --mx-flex-wrap: nowrap;"
>
<span
class="_unread-counter_9mg0k_8"
>
1
</span>
</div>
</div>
</div>
</button>
</DocumentFragment>
`;
exports[`<RoomListItemView /> should render a room item 1`] = `
<DocumentFragment>
<button

View File

@ -223,7 +223,7 @@ describe("RoomNotificationState", () => {
it("should has isUnsetMessage at true", () => {
jest.spyOn(RoomStatusBarModule, "getUnsentMessages").mockReturnValue([{} as MatrixEvent]);
const roomNotifState = new RoomNotificationState(room, false);
expect(roomNotifState.isUnsetMessage).toBe(true);
expect(roomNotifState.isUnsentMessage).toBe(true);
});
it("should has isMention at false if the notification is invitation, an unset message or a knock", () => {