From b0587fcfb33fbd5cd2b30449bf426cffce3ba017 Mon Sep 17 00:00:00 2001 From: Robin Date: Thu, 5 Jun 2025 07:54:57 -0400 Subject: [PATCH] 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 a1110af6d549e2e3b5cd33912e0a6389d7bdf326). 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 --- src/room/GroupCallView.test.tsx | 2 + src/room/useActiveFocus.ts | 43 ++++++------- src/room/useRoomName.ts | 15 ++--- src/room/useRoomState.ts | 32 ++++++---- src/useEvents.test.tsx | 88 +++++++++++++++++++++++++++ src/useEvents.ts | 40 +++++++++++- src/useLocalStorage.test.tsx | 51 ++++++++++++++++ src/useLocalStorage.ts | 48 +++++++-------- src/useMatrixRTCSessionJoinState.ts | 42 ++++++------- src/useMatrixRTCSessionMemberships.ts | 34 +++-------- src/useMediaQuery.ts | 23 +++---- src/utils/test.ts | 2 +- 12 files changed, 283 insertions(+), 137 deletions(-) create mode 100644 src/useEvents.test.tsx create mode 100644 src/useLocalStorage.test.tsx diff --git a/src/room/GroupCallView.test.tsx b/src/room/GroupCallView.test.tsx index 75961c72..04b1b62a 100644 --- a/src/room/GroupCallView.test.tsx +++ b/src/room/GroupCallView.test.tsx @@ -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 as RoomState, }); diff --git a/src/room/useActiveFocus.ts b/src/room/useActiveFocus.ts index a8dfa836..41b42341 100644 --- a/src/room/useActiveFocus.ts +++ b/src/room/useActiveFocus.ts @@ -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; } diff --git a/src/room/useRoomName.ts b/src/room/useRoomName.ts index 2b7459a3..83857857 100644 --- a/src/room/useRoomName.ts +++ b/src/room/useRoomName.ts @@ -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]), + ); } diff --git a/src/room/useRoomState.ts b/src/room/useRoomState.ts index 51a209ab..ad08f7a2 100644 --- a/src/room/useRoomState.ts +++ b/src/room/useRoomState.ts @@ -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 = (room: Room, f: (state: RoomState) => T): T => { - const [numUpdates, setNumUpdates] = useState(0); - useTypedEventEmitter( +export function useRoomState(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]), + ); +} diff --git a/src/useEvents.test.tsx b/src/useEvents.test.tsx new file mode 100644 index 00000000..5ad4c1d4 --- /dev/null +++ b/src/useEvents.test.tsx @@ -0,0 +1,88 @@ +/* +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 { test } from "vitest"; +import { render, screen } from "@testing-library/react"; +import { type FC, useEffect, useState } from "react"; +import userEvent from "@testing-library/user-event"; +import { TypedEventEmitter } from "matrix-js-sdk"; + +import { useTypedEventEmitterState } from "./useEvents"; + +class TestEmitter extends TypedEventEmitter<"change", { change: () => void }> { + private state = 1; + public readonly getState = (): number => this.state; + public readonly getNegativeState = (): number => -this.state; + public readonly setState = (value: number): void => { + this.state = value; + this.emit("change"); + }; +} + +test("useTypedEventEmitterState reacts to events", async () => { + const user = userEvent.setup(); + const emitter = new TestEmitter(); + + const Test: FC = () => { + const value = useTypedEventEmitterState( + emitter, + "change", + emitter.getState, + ); + return ( + <> + +
Value is {value}
+ + ); + }; + + render(); + screen.getByText("Value is 1"); + await user.click(screen.getByText("Change value")); + screen.getByText("Value is 2"); +}); + +test("useTypedEventEmitterState reacts to changes made by an effect mounted on the same render", () => { + const emitter = new TestEmitter(); + + const Test: FC = () => { + useEffect(() => emitter.setState(2), []); + const value = useTypedEventEmitterState( + emitter, + "change", + emitter.getState, + ); + return `Value is ${value}`; + }; + + render(); + screen.getByText("Value is 2"); +}); + +test("useTypedEventEmitterState reacts to changes in getState", async () => { + const user = userEvent.setup(); + const emitter = new TestEmitter(); + + const Test: FC = () => { + const [fn, setFn] = useState(() => emitter.getState); + const value = useTypedEventEmitterState(emitter, "change", fn); + return ( + <> + +
Value is {value}
+ + ); + }; + + render(); + screen.getByText("Value is 1"); + await user.click(screen.getByText("Change getState")); + screen.getByText("Value is -1"); +}); diff --git a/src/useEvents.ts b/src/useEvents.ts index c19145eb..3495cc57 100644 --- a/src/useEvents.ts +++ b/src/useEvents.ts @@ -5,7 +5,7 @@ SPDX-License-Identifier: AGPL-3.0-only OR LicenseRef-Element-Commercial Please see LICENSE in the repository root for full details. */ -import { useEffect } from "react"; +import { useCallback, useEffect, useSyncExternalStore } from "react"; import type { Listener, @@ -13,7 +13,9 @@ import type { TypedEventEmitter, } from "matrix-js-sdk/lib/models/typed-event-emitter"; -// Shortcut for registering a listener on an EventTarget +/** + * Shortcut for registering a listener on an EventTarget. + */ export function useEventTarget( target: EventTarget | null | undefined, eventType: string, @@ -33,7 +35,9 @@ export function useEventTarget( }, [target, eventType, listener, options]); } -// Shortcut for registering a listener on a TypedEventEmitter +/** + * Shortcut for registering a listener on a TypedEventEmitter. + */ export function useTypedEventEmitter< Events extends string, Arguments extends ListenerMap, @@ -50,3 +54,33 @@ export function useTypedEventEmitter< }; }, [emitter, eventType, listener]); } + +/** + * Reactively tracks a value which is recalculated whenever the provided event + * emitter emits an event. This is useful for bridging state from matrix-js-sdk + * into React. + */ +export function useTypedEventEmitterState< + Events extends string, + Arguments extends ListenerMap, + T extends Events, + State, +>( + emitter: TypedEventEmitter, + eventType: T, + getState: () => State, +): State { + const subscribe = useCallback( + (onChange: () => void) => { + emitter.on(eventType, onChange as Listener); + return (): void => { + emitter.off(eventType, onChange as Listener); + }; + }, + [emitter, eventType], + ); + // See the React docs for useSyncExternalStore; given that we're trying to + // bridge state from an external source into React, using this hook is exactly + // what React recommends. + return useSyncExternalStore(subscribe, getState); +} diff --git a/src/useLocalStorage.test.tsx b/src/useLocalStorage.test.tsx new file mode 100644 index 00000000..6e6c8c26 --- /dev/null +++ b/src/useLocalStorage.test.tsx @@ -0,0 +1,51 @@ +/* +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 { test } from "vitest"; +import { render, screen } from "@testing-library/react"; +import { type FC, useEffect, useState } from "react"; +import userEvent from "@testing-library/user-event"; + +import { + setLocalStorageItemReactive, + useLocalStorage, +} from "./useLocalStorage"; + +test("useLocalStorage reacts to changes made by an effect mounted on the same render", () => { + localStorage.clear(); + const Test: FC = () => { + useEffect(() => setLocalStorageItemReactive("my-value", "Hello!"), []); + const [myValue] = useLocalStorage("my-value"); + return myValue; + }; + render(); + screen.getByText("Hello!"); +}); + +test("useLocalStorage reacts to key changes", async () => { + localStorage.clear(); + localStorage.setItem("value-1", "1"); + localStorage.setItem("value-2", "2"); + + const Test: FC = () => { + const [key, setKey] = useState("value-1"); + const [value] = useLocalStorage(key); + if (key !== `value-${value}`) throw new Error("Value is out of sync"); + return ( + <> + +
Value is: {value}
+ + ); + }; + const user = userEvent.setup(); + render(); + + screen.getByText("Value is: 1"); + await user.click(screen.getByRole("button", { name: "Switch keys" })); + screen.getByText("Value is: 2"); +}); diff --git a/src/useLocalStorage.ts b/src/useLocalStorage.ts index 43e828bf..dcf00a42 100644 --- a/src/useLocalStorage.ts +++ b/src/useLocalStorage.ts @@ -5,50 +5,44 @@ SPDX-License-Identifier: AGPL-3.0-only OR LicenseRef-Element-Commercial Please see LICENSE in the repository root for full details. */ -import EventEmitter from "events"; -import { useCallback, useEffect, useState } from "react"; +import { useCallback } from "react"; +import { TypedEventEmitter } from "matrix-js-sdk"; + +import { useTypedEventEmitterState } from "./useEvents"; type LocalStorageItem = ReturnType; // Bus to notify other useLocalStorage consumers when an item is changed -export const localStorageBus = new EventEmitter(); +export const localStorageBus = new TypedEventEmitter< + string, + { [key: string]: () => void } +>(); /** * Like useState, but reads from and persists the value to localStorage * This hook will not update when we write to localStorage.setItem(key, value) directly. * For the hook to react either use the returned setter or `setLocalStorageItemReactive`. */ -export const useLocalStorage = ( +export function useLocalStorage( key: string, -): [LocalStorageItem, (value: string) => void] => { - const [value, setValue] = useState(() => - localStorage.getItem(key), +): [LocalStorageItem, (value: string) => void] { + const value = useTypedEventEmitterState( + localStorageBus, + key, + useCallback(() => localStorage.getItem(key), [key]), + ); + const setValue = useCallback( + (newValue: string) => setLocalStorageItemReactive(key, newValue), + [key], ); - useEffect(() => { - localStorageBus.on(key, setValue); - return (): void => { - localStorageBus.off(key, setValue); - }; - }, [key, setValue]); - - return [ - value, - useCallback( - (newValue: string) => { - setValue(newValue); - localStorage.setItem(key, newValue); - localStorageBus.emit(key, newValue); - }, - [key, setValue], - ), - ]; -}; + return [value, setValue]; +} export const setLocalStorageItemReactive = ( key: string, value: string, ): void => { localStorage.setItem(key, value); - localStorageBus.emit(key, value); + localStorageBus.emit(key); }; diff --git a/src/useMatrixRTCSessionJoinState.ts b/src/useMatrixRTCSessionJoinState.ts index 5e7ea110..2f6ccf25 100644 --- a/src/useMatrixRTCSessionJoinState.ts +++ b/src/useMatrixRTCSessionJoinState.ts @@ -10,33 +10,31 @@ import { type MatrixRTCSession, MatrixRTCSessionEvent, } from "matrix-js-sdk/lib/matrixrtc"; -import { useEffect, useState } from "react"; +import { TypedEventEmitter } from "matrix-js-sdk"; +import { useCallback, useEffect } from "react"; + +import { useTypedEventEmitterState } from "./useEvents"; + +const dummySession = new TypedEventEmitter(); export function useMatrixRTCSessionJoinState( rtcSession: MatrixRTCSession | undefined, ): boolean { - const [, setNumUpdates] = useState(0); + // React doesn't allow you to run a hook conditionally, so we have to plug in + // a dummy event emitter in case there is no rtcSession yet + const isJoined = useTypedEventEmitterState( + rtcSession ?? dummySession, + MatrixRTCSessionEvent.JoinStateChanged, + useCallback(() => rtcSession?.isJoined() ?? false, [rtcSession]), + ); useEffect(() => { - if (rtcSession !== undefined) { - const onJoinStateChanged = (isJoined: boolean): void => { - logger.info( - `Session in room ${rtcSession.room.roomId} changed to ${ - isJoined ? "joined" : "left" - }`, - ); - setNumUpdates((n) => n + 1); // Force an update - }; - rtcSession.on(MatrixRTCSessionEvent.JoinStateChanged, onJoinStateChanged); + logger.info( + `Session in room ${rtcSession?.room.roomId} changed to ${ + isJoined ? "joined" : "left" + }`, + ); + }, [rtcSession, isJoined]); - return (): void => { - rtcSession.off( - MatrixRTCSessionEvent.JoinStateChanged, - onJoinStateChanged, - ); - }; - } - }, [rtcSession]); - - return rtcSession?.isJoined() ?? false; + return isJoined; } diff --git a/src/useMatrixRTCSessionMemberships.ts b/src/useMatrixRTCSessionMemberships.ts index 25b790d2..0dba6b15 100644 --- a/src/useMatrixRTCSessionMemberships.ts +++ b/src/useMatrixRTCSessionMemberships.ts @@ -5,39 +5,21 @@ SPDX-License-Identifier: AGPL-3.0-only OR LicenseRef-Element-Commercial Please see LICENSE in the repository root for full details. */ -import { logger } from "matrix-js-sdk/lib/logger"; import { type CallMembership, type MatrixRTCSession, MatrixRTCSessionEvent, } from "matrix-js-sdk/lib/matrixrtc"; -import { useCallback, useEffect, useState } from "react"; +import { useCallback } from "react"; + +import { useTypedEventEmitterState } from "./useEvents"; export function useMatrixRTCSessionMemberships( rtcSession: MatrixRTCSession, ): CallMembership[] { - const [memberships, setMemberships] = useState(rtcSession.memberships); - - const onMembershipsChanged = useCallback(() => { - logger.info( - `Memberships changed for call in room ${rtcSession.room.roomId} (${rtcSession.memberships.length} members)`, - ); - setMemberships(rtcSession.memberships); - }, [rtcSession]); - - useEffect(() => { - rtcSession.on( - MatrixRTCSessionEvent.MembershipsChanged, - onMembershipsChanged, - ); - - return (): void => { - rtcSession.off( - MatrixRTCSessionEvent.MembershipsChanged, - onMembershipsChanged, - ); - }; - }, [rtcSession, onMembershipsChanged]); - - return memberships; + return useTypedEventEmitterState( + rtcSession, + MatrixRTCSessionEvent.MembershipsChanged, + useCallback(() => rtcSession.memberships, [rtcSession]), + ); } diff --git a/src/useMediaQuery.ts b/src/useMediaQuery.ts index ce73cb9c..1ea9196d 100644 --- a/src/useMediaQuery.ts +++ b/src/useMediaQuery.ts @@ -1,13 +1,11 @@ /* -Copyright 2023, 2024 New Vector Ltd. +Copyright 2023-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 { useCallback, useMemo, useState } from "react"; - -import { useEventTarget } from "./useEvents"; +import { useCallback, useMemo, useSyncExternalStore } from "react"; /** * React hook that tracks whether the given media query matches. @@ -15,14 +13,13 @@ import { useEventTarget } from "./useEvents"; export function useMediaQuery(query: string): boolean { const mediaQuery = useMemo(() => window.matchMedia(query), [query]); - const [numChanges, setNumChanges] = useState(0); - useEventTarget( - mediaQuery, - "change", - useCallback(() => setNumChanges((n) => n + 1), [setNumChanges]), + const subscribe = useCallback( + (onChange: () => void) => { + mediaQuery.addEventListener("change", onChange); + return (): void => mediaQuery.removeEventListener("change", onChange); + }, + [mediaQuery], ); - - // We want any change to the update counter to trigger an update here - // eslint-disable-next-line react-hooks/exhaustive-deps - return useMemo(() => mediaQuery.matches, [mediaQuery, numChanges]); + const getState = useCallback(() => mediaQuery.matches, [mediaQuery]); + return useSyncExternalStore(subscribe, getState); } diff --git a/src/utils/test.ts b/src/utils/test.ts index 51ed1ed2..f142b0d4 100644 --- a/src/utils/test.ts +++ b/src/utils/test.ts @@ -108,7 +108,7 @@ interface EmitterMock { removeListener: () => T; } -function mockEmitter(): EmitterMock { +export function mockEmitter(): EmitterMock { return { on(): T { return this as T;