Skip to content

New room list: fix incorrect decoration #29770

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Apr 15, 2025
Merged

New room list: fix incorrect decoration #29770

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
f085854
fix(call): reset call value when the roomId changes
florianduros Apr 15, 2025
901bf37
fix(call): reset presence indicator when the room changes
florianduros Apr 15, 2025
d3e2ced
refactor: use existing `usePresence`
florianduros Apr 15, 2025
433e769
test: fix room avatar view test
florianduros Apr 15, 2025
a0f229d
test: update snapshots
florianduros Apr 15, 2025
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
75 changes: 5 additions & 70 deletions src/components/viewmodels/avatars/RoomAvatarViewModel.tsx
View file
Open in desktop
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While you're in the area: useDMPresence doesn't seem to work quite right, getDMUser is not stable and can change, so should react to changes to m.direct - maybe use useDmMember?

Copy link
Member Author

@florianduros florianduros Apr 15, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I used useDmMember instead

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like WithPresenceIndicator also has a usePresence which could likely replace the majority of the rest of the below hook

Original file line number Diff line number Diff line change
Expand Up @@ -5,32 +5,11 @@
* Please see LICENSE files in the repository root for full details.
*/

import {
EventType,
JoinRule,
type MatrixEvent,
type Room,
RoomEvent,
type User,
UserEvent,
} from "matrix-js-sdk/src/matrix";
import { EventType, JoinRule, type MatrixEvent, type Room, RoomEvent } from "matrix-js-sdk/src/matrix";
import { useEffect, useState } from "react";

import { useTypedEventEmitter } from "../../../hooks/useEventEmitter";
import DMRoomMap from "../../../utils/DMRoomMap";
import { getJoinedNonFunctionalMembers } from "../../../utils/room/getJoinedNonFunctionalMembers";
import { BUSY_PRESENCE_NAME } from "../../views/rooms/PresenceLabel";
import { isPresenceEnabled } from "../../../utils/presence";

/**
* The presence of a user in a DM room.
* - "online": The user is online.
* - "offline": The user is offline.
* - "busy": The user is busy.
* - "unavailable": the presence is unavailable.
* - null: the user is not in a DM room or presence is not enabled.
*/
export type Presence = "online" | "offline" | "busy" | "unavailable" | null;
import { useDmMember, usePresence, type Presence } from "../../views/avatars/WithPresenceIndicator";

export interface RoomAvatarViewState {
/**
Expand All @@ -50,7 +29,7 @@ export interface RoomAvatarViewState {
* The presence of the user in the DM room.
* If null, the user is not in a DM room or presence is not enabled.
*/
presence: Presence;
presence: Presence | null;
}

/**
Expand All @@ -59,7 +38,8 @@ export interface RoomAvatarViewState {
*/
export function useRoomAvatarViewModel(room: Room): RoomAvatarViewState {
const isVideoRoom = room.isElementVideoRoom() || room.isCallRoom();
const presence = useDMPresence(room);
const roomMember = useDmMember(room);
const presence = usePresence(room, roomMember);
const isPublic = useIsPublic(room);

const hasDecoration = isPublic || isVideoRoom || presence !== null;
Expand Down Expand Up @@ -97,48 +77,3 @@ function useIsPublic(room: Room): boolean {
function isRoomPublic(room: Room): boolean {
return room.getJoinRule() === JoinRule.Public;
}

/**
* Hook listening to the presence of the DM user.
* @param room
*/
function useDMPresence(room: Room): Presence {
const dmUser = getDMUser(room);
const [presence, setPresence] = useState<Presence>(getPresence(dmUser));
useTypedEventEmitter(dmUser, UserEvent.Presence, () => setPresence(getPresence(dmUser)));
useTypedEventEmitter(dmUser, UserEvent.CurrentlyActive, () => setPresence(getPresence(dmUser)));

return presence;
}

/**
* Get the DM user of the room.
* Return undefined if the room is not a DM room, if we can't find the user or if the presence is not enabled.
* @param room
* @returns found user
*/
function getDMUser(room: Room): User | undefined {
const otherUserId = DMRoomMap.shared().getUserIdForRoomId(room.roomId);
if (!otherUserId) return;
if (getJoinedNonFunctionalMembers(room).length !== 2) return;
if (!isPresenceEnabled(room.client)) return;

return room.client.getUser(otherUserId) || undefined;
}

/**
* Get the presence of the DM user.
* @param dmUser
*/
function getPresence(dmUser: User | undefined): Presence {
if (!dmUser) return null;
if (BUSY_PRESENCE_NAME.matches(dmUser.presence)) return "busy";

const isOnline = dmUser.currentlyActive || dmUser.presence === "online";
if (isOnline) return "online";

if (dmUser.presence === "offline") return "offline";
if (dmUser.presence === "unavailable") return "unavailable";

return null;
}
11 changes: 6 additions & 5 deletions src/components/views/avatars/RoomAvatarView.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,9 @@ import BusyIcon from "@vector-im/compound-design-tokens/assets/web/icons/presenc
import classNames from "classnames";

import RoomAvatar from "./RoomAvatar";
import { useRoomAvatarViewModel, type Presence } from "../../viewmodels/avatars/RoomAvatarViewModel";
import { useRoomAvatarViewModel } from "../../viewmodels/avatars/RoomAvatarViewModel";
import { _t } from "../../../languageHandler";
import { Presence } from "./WithPresenceIndicator";

interface RoomAvatarViewProps {
/**
Expand Down Expand Up @@ -83,7 +84,7 @@ type PresenceDecorationProps = {
*/
function PresenceDecoration({ presence }: PresenceDecorationProps): JSX.Element {
switch (presence) {
case "online":
case Presence.Online:
return (
<OnlineOrUnavailableIcon
width="8px"
Expand All @@ -93,7 +94,7 @@ function PresenceDecoration({ presence }: PresenceDecorationProps): JSX.Element
aria-label={_t("presence|online")}
/>
);
case "unavailable":
case Presence.Away:
return (
<OnlineOrUnavailableIcon
width="8px"
Expand All @@ -103,7 +104,7 @@ function PresenceDecoration({ presence }: PresenceDecorationProps): JSX.Element
aria-label={_t("presence|away")}
/>
);
case "offline":
case Presence.Offline:
return (
<OfflineIcon
width="8px"
Expand All @@ -113,7 +114,7 @@ function PresenceDecoration({ presence }: PresenceDecorationProps): JSX.Element
aria-label={_t("presence|offline")}
/>
);
case "busy":
case Presence.Busy:
return (
<BusyIcon
width="8px"
Expand Down
4 changes: 2 additions & 2 deletions src/components/views/avatars/WithPresenceIndicator.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ interface Props {
children: ReactNode;
}

enum Presence {
export enum Presence {
// Note: the names here are used in CSS class names
Online = "ONLINE",
Away = "AWAY",
Expand Down Expand Up @@ -86,7 +86,7 @@ function getPresence(member: RoomMember | null): Presence | null {
return null;
}

const usePresence = (room: Room, member: RoomMember | null): Presence | null => {
export const usePresence = (room: Room, member: RoomMember | null): Presence | null => {
const [presence, setPresence] = useState<Presence | null>(getPresence(member));
const updatePresence = (): void => {
setPresence(getPresence(member));
Expand Down
8 changes: 7 additions & 1 deletion src/hooks/useCall.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ SPDX-License-Identifier: AGPL-3.0-only OR GPL-3.0-only OR LicenseRef-Element-Com
Please see LICENSE files in the repository root for full details.
*/

import { useState, useCallback, useMemo } from "react";
import { useState, useCallback, useMemo, useEffect } from "react";

import type { RoomMember } from "matrix-js-sdk/src/matrix";
import { type Call, ConnectionState, CallEvent } from "../models/Call";
Expand All @@ -20,6 +20,12 @@ export const useCall = (roomId: string): Call | null => {
useEventEmitter(CallStore.instance, CallStoreEvent.Call, (call: Call | null, forRoomId: string) => {
if (forRoomId === roomId) setCall(call);
});

// Reset the value when the roomId changes
useEffect(() => {
setCall(CallStore.instance.getCall(roomId));
}, [roomId]);

return call;
};

Expand Down
87 changes: 14 additions & 73 deletions test/unit-tests/components/viewmodels/avatars/RoomAvatarViewModel-test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,32 +5,18 @@
* Please see LICENSE files in the repository root for full details.
*/

import { renderHook, waitFor, act } from "jest-matrix-react";
import {
JoinRule,
type MatrixClient,
MatrixEvent,
type Room,
type RoomMember,
User,
UserEvent,
} from "matrix-js-sdk/src/matrix";
import { mocked } from "jest-mock";
import { renderHook, waitFor } from "jest-matrix-react";
import { JoinRule, type MatrixClient, type Room, RoomMember, User } from "matrix-js-sdk/src/matrix";

import { useRoomAvatarViewModel } from "../../../../../src/components/viewmodels/avatars/RoomAvatarViewModel";
import { createTestClient, mkStubRoom } from "../../../../test-utils";
import DMRoomMap from "../../../../../src/utils/DMRoomMap";
import { getJoinedNonFunctionalMembers } from "../../../../../src/utils/room/getJoinedNonFunctionalMembers";
import { isPresenceEnabled } from "../../../../../src/utils/presence";
import * as PresenceIndicatorModule from "../../../../../src/components/views/avatars/WithPresenceIndicator";

jest.mock("../../../../../src/utils/room/getJoinedNonFunctionalMembers", () => ({
getJoinedNonFunctionalMembers: jest.fn().mockReturnValue([]),
}));

jest.mock("../../../../../src/utils/presence", () => ({
isPresenceEnabled: jest.fn().mockReturnValue(false),
}));

describe("RoomAvatarViewModel", () => {
let matrixClient: MatrixClient;
let room: Room;
Expand All @@ -41,6 +27,9 @@ describe("RoomAvatarViewModel", () => {

DMRoomMap.makeShared(matrixClient);
jest.spyOn(DMRoomMap.shared(), "getUserIdForRoomId").mockReturnValue(null);

jest.spyOn(PresenceIndicatorModule, "useDmMember").mockReturnValue(null);
jest.spyOn(PresenceIndicatorModule, "usePresence").mockReturnValue(null);
});

it("should has hasDecoration to false", async () => {
Expand Down Expand Up @@ -74,62 +63,14 @@ describe("RoomAvatarViewModel", () => {
await waitFor(() => expect(vm.current.isPublic).toBe(true));
});

describe("presence", () => {
let user: User;

beforeEach(() => {
jest.spyOn(DMRoomMap.shared(), "getUserIdForRoomId").mockReturnValue("userId");
mocked(getJoinedNonFunctionalMembers).mockReturnValue([{}, {}] as RoomMember[]);
mocked(isPresenceEnabled).mockReturnValue(true);

user = User.createUser("userId", matrixClient);
jest.spyOn(matrixClient, "getUser").mockReturnValue(user);
});

it("should has presence set to null", () => {
jest.spyOn(DMRoomMap.shared(), "getUserIdForRoomId").mockReturnValue(null);

const { result: vm } = renderHook(() => useRoomAvatarViewModel(room));
expect(vm.current.presence).toBe(null);
});

it("should has online presence", async () => {
const { result: vm } = renderHook(() => useRoomAvatarViewModel(room));
expect(vm.current.presence).toBe("offline");
it("should return presence", async () => {
const user = User.createUser("userId", matrixClient);
const roomMember = new RoomMember(room.roomId, "userId");
roomMember.user = user;
jest.spyOn(PresenceIndicatorModule, "useDmMember").mockReturnValue(roomMember);
jest.spyOn(PresenceIndicatorModule, "usePresence").mockReturnValue(PresenceIndicatorModule.Presence.Online);

user.presence = "online";

await act(() => user.emit(UserEvent.Presence, new MatrixEvent(), user));
await waitFor(() => expect(vm.current.presence).toBe("online"));

user.currentlyActive = true;
user.presence = "offline";

await act(() => user.emit(UserEvent.CurrentlyActive, new MatrixEvent(), user));
await waitFor(() => expect(vm.current.presence).toBe("online"));
});

it("should has busy presence", async () => {
user.presence = "busy";
const { result: vm } = renderHook(() => useRoomAvatarViewModel(room));
expect(vm.current.presence).toBe("busy");
});

it("should has offline presence", async () => {
user.presence = "offline";
const { result: vm } = renderHook(() => useRoomAvatarViewModel(room));
expect(vm.current.presence).toBe("offline");
});

it("should has unavailable presence", async () => {
user.presence = "unavailable";
const { result: vm } = renderHook(() => useRoomAvatarViewModel(room));
expect(vm.current.presence).toBe("unavailable");
});

it("should has hasDecoration to true", async () => {
const { result: vm } = renderHook(() => useRoomAvatarViewModel(room));
expect(vm.current.hasDecoration).toBe(true);
});
const { result: vm } = renderHook(() => useRoomAvatarViewModel(room));
expect(vm.current.presence).toBe(PresenceIndicatorModule.Presence.Online);
});
});
10 changes: 5 additions & 5 deletions test/unit-tests/components/views/avatars/RoomAvatarView-test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,11 +12,11 @@ import { mocked } from "jest-mock";
import { RoomAvatarView } from "../../../../../src/components/views/avatars/RoomAvatarView";
import { mkStubRoom, stubClient } from "../../../../test-utils";
import {
type Presence,
type RoomAvatarViewState,
useRoomAvatarViewModel,
} from "../../../../../src/components/viewmodels/avatars/RoomAvatarViewModel";
import DMRoomMap from "../../../../../src/utils/DMRoomMap";
import { Presence } from "../../../../../src/components/views/avatars/WithPresenceIndicator";

jest.mock("../../../../../src/components/viewmodels/avatars/RoomAvatarViewModel", () => ({
useRoomAvatarViewModel: jest.fn(),
Expand Down Expand Up @@ -83,10 +83,10 @@ describe("<RoomAvatarView />", () => {
});

it.each([
{ presence: "online" as Presence, label: "Online" },
{ presence: "offline" as Presence, label: "Offline" },
{ presence: "busy" as Presence, label: "Busy" },
{ presence: "unavailable" as Presence, label: "Away" },
{ presence: Presence.Online, label: "Online" },
{ presence: Presence.Offline, label: "Offline" },
{ presence: Presence.Busy, label: "Busy" },
{ presence: Presence.Away, label: "Away" },
])("should render the $presence presence", ({ presence, label }) => {
mocked(useRoomAvatarViewModel).mockReturnValue({
...defaultValue,
Expand Down
Loading
Loading