Avoid reactivity bugs in how we track external state (#3316)

* Avoid reactivity bugs in how we track external state

Many of our hooks which attempt to bridge external state from an EventEmitter or EventTarget into React had subtle bugs which could cause them to fail to react to certain updates. The conditions necessary for triggering these bugs are explained by the tests that I've included.

In the majority of cases, I don't think we were triggering these bugs in practice. They could've become problems if we refactored our components in certain ways. The one concrete case I'm aware of in which we actually triggered such a bug was the race condition with the useRoomEncryptionSystem shared secret logic (addressed by a1110af6d5).

But, particularly with all the weird reactivity issues we're debugging this week, I think we need to eliminate the possibility that any of the bugs in these hooks are the cause of our current headaches.

* Reuse useTypedEventEmitterState in useLocalStorage

* Fix type error
This commit is contained in:
Robin
2025-06-05 07:54:57 -04:00
committed by GitHub
parent a62b0be831
commit b0587fcfb3
12 changed files with 283 additions and 137 deletions

View File

@@ -29,6 +29,7 @@ import { useAudioContext } from "../useAudioContext";
import { ActiveCall } from "./InCallView";
import {
flushPromises,
mockEmitter,
mockMatrixRoom,
mockMatrixRoomMember,
mockRtcMembership,
@@ -129,6 +130,7 @@ function createGroupCallView(
getMxcAvatarUrl: () => null,
getCanonicalAlias: () => null,
currentState: {
...mockEmitter(),
getJoinRule: () => JoinRule.Invite,
} as Partial<RoomState> as RoomState,
});

View File

@@ -9,11 +9,13 @@ import {
type MatrixRTCSession,
MatrixRTCSessionEvent,
} from "matrix-js-sdk/lib/matrixrtc";
import { useCallback, useEffect, useState } from "react";
import { useCallback, useEffect, useRef } from "react";
import { deepCompare } from "matrix-js-sdk/lib/utils";
import { logger } from "matrix-js-sdk/lib/logger";
import { type LivekitFocus, isLivekitFocus } from "matrix-js-sdk/lib/matrixrtc";
import { useTypedEventEmitterState } from "../useEvents";
/**
* Gets the currently active (livekit) focus for a MatrixRTC session
* This logic is specific to livekit foci where the whole call must use one
@@ -22,38 +24,27 @@ import { type LivekitFocus, isLivekitFocus } from "matrix-js-sdk/lib/matrixrtc";
export function useActiveLivekitFocus(
rtcSession: MatrixRTCSession,
): LivekitFocus | undefined {
const [activeFocus, setActiveFocus] = useState(() => {
const f = rtcSession.getActiveFocus();
// Only handle foci with type="livekit" for now.
return !!f && isLivekitFocus(f) ? f : undefined;
});
const activeFocus = useTypedEventEmitterState(
rtcSession,
MatrixRTCSessionEvent.MembershipsChanged,
useCallback(() => {
const f = rtcSession.getActiveFocus();
// Only handle foci with type="livekit" for now.
return !!f && isLivekitFocus(f) ? f : undefined;
}, [rtcSession]),
);
const onMembershipsChanged = useCallback(() => {
const newActiveFocus = rtcSession.getActiveFocus();
if (!!newActiveFocus && !isLivekitFocus(newActiveFocus)) return;
if (!deepCompare(activeFocus, newActiveFocus)) {
const prevActiveFocus = useRef(activeFocus);
useEffect(() => {
if (!deepCompare(activeFocus, prevActiveFocus.current)) {
const oldestMembership = rtcSession.getOldestMembership();
logger.warn(
`Got new active focus from membership: ${oldestMembership?.sender}/${oldestMembership?.deviceId}.
Updating focus (focus switch) from ${JSON.stringify(activeFocus)} to ${JSON.stringify(newActiveFocus)}`,
Updated focus (focus switch) from ${JSON.stringify(prevActiveFocus.current)} to ${JSON.stringify(activeFocus)}`,
);
setActiveFocus(newActiveFocus);
prevActiveFocus.current = activeFocus;
}
}, [activeFocus, rtcSession]);
useEffect(() => {
rtcSession.on(
MatrixRTCSessionEvent.MembershipsChanged,
onMembershipsChanged,
);
return (): void => {
rtcSession.off(
MatrixRTCSessionEvent.MembershipsChanged,
onMembershipsChanged,
);
};
});
return activeFocus;
}

View File

@@ -1,18 +1,19 @@
/*
Copyright 2023, 2024 New Vector Ltd.
Copyright 2025 New Vector Ltd.
SPDX-License-Identifier: AGPL-3.0-only OR LicenseRef-Element-Commercial
Please see LICENSE in the repository root for full details.
*/
import { type Room, RoomEvent } from "matrix-js-sdk";
import { useState } from "react";
import { useCallback } from "react";
import { useTypedEventEmitter } from "../useEvents";
import { useTypedEventEmitterState } from "../useEvents";
export function useRoomName(room: Room): string {
const [, setNumUpdates] = useState(0);
// Whenever the name changes, force an update
useTypedEventEmitter(room, RoomEvent.Name, () => setNumUpdates((n) => n + 1));
return room.name;
return useTypedEventEmitterState(
room,
RoomEvent.Name,
useCallback(() => room.name, [room]),
);
}

View File

@@ -5,10 +5,15 @@ SPDX-License-Identifier: AGPL-3.0-only OR LicenseRef-Element-Commercial
Please see LICENSE in the repository root for full details.
*/
import { useCallback, useMemo, useState } from "react";
import { type RoomState, RoomStateEvent, type Room } from "matrix-js-sdk";
import { useCallback } from "react";
import {
type RoomState,
RoomStateEvent,
type Room,
RoomEvent,
} from "matrix-js-sdk";
import { useTypedEventEmitter } from "../useEvents";
import { useTypedEventEmitterState } from "../useEvents";
/**
* A React hook for values computed from room state.
@@ -16,14 +21,17 @@ import { useTypedEventEmitter } from "../useEvents";
* @param f A mapping from the current room state to the computed value.
* @returns The computed value.
*/
export const useRoomState = <T>(room: Room, f: (state: RoomState) => T): T => {
const [numUpdates, setNumUpdates] = useState(0);
useTypedEventEmitter(
export function useRoomState<T>(room: Room, f: (state: RoomState) => T): T {
// TODO: matrix-js-sdk says that Room.currentState is deprecated, but it's not
// clear how to reactively track the current state of the room without it
const currentState = useTypedEventEmitterState(
room,
RoomStateEvent.Update,
useCallback(() => setNumUpdates((n) => n + 1), [setNumUpdates]),
RoomEvent.CurrentStateUpdated,
useCallback(() => room.currentState, [room]),
);
// We want any change to the update counter to trigger an update here
// eslint-disable-next-line react-hooks/exhaustive-deps
return useMemo(() => f(room.currentState), [room, f, numUpdates]);
};
return useTypedEventEmitterState(
currentState,
RoomStateEvent.Update,
useCallback(() => f(currentState), [f, currentState]),
);
}