Leave issue refactor (#3302)

* Simplify key local storage management. * Refactor useLivekit to only ever connect to one room. This change also tries to make the code more explicit so that we only do the things we really need to do and rely less on react updating everything correctly. It also surfaces, that we are currently implementing useLivekit in a way, so that we can change the encryption system on the fly and recreate the room. I am not sure this is a case we need to support? * simplify the useLivekit hook even more This is possible because we concluded that we do not need to be able to hot reload the e2ee system. * review * linter * Update src/room/InCallView.tsx Co-authored-by: Robin <robin@robin.town> --------- Co-authored-by: Robin <robin@robin.town>
2026-02-14 04:37:03 +00:00 · 2025-06-04 22:51:13 +02:00
parent ef41ef85eb
commit 2f3e0b419d
6 changed files with 141 additions and 129 deletions
--- a/src/e2ee/sharedKeyManagement.ts
+++ b/src/e2ee/sharedKeyManagement.ts
@@ -6,58 +6,70 @@ Please see LICENSE in the repository root for full details.
 */

 import { useEffect, useMemo } from "react";
+import { logger } from "matrix-js-sdk/lib/logger";

-import { setLocalStorageItem, useLocalStorage } from "../useLocalStorage";
-import { type UrlParams, getUrlParams, useUrlParams } from "../UrlParams";
+import {
+  setLocalStorageItemReactive,
+  useLocalStorage,
+} from "../useLocalStorage";
+import { getUrlParams } from "../UrlParams";
 import { E2eeType } from "./e2eeType";
 import { useClient } from "../ClientContext";

+/**
+ * This setter will update the state for all `useRoomSharedKey` hooks
+ * if the password is different from the one in local storage or if its not yet in the local storage.
+ */
 export function saveKeyForRoom(roomId: string, password: string): void {
-  setLocalStorageItem(getRoomSharedKeyLocalStorageKey(roomId), password);
+  if (
+    localStorage.getItem(getRoomSharedKeyLocalStorageKey(roomId)) !== password
+  )
+    setLocalStorageItemReactive(
+      getRoomSharedKeyLocalStorageKey(roomId),
+      password,
+    );
 }

 const getRoomSharedKeyLocalStorageKey = (roomId: string): string =>
  `room-shared-key-${roomId}`;

-const useInternalRoomSharedKey = (roomId: string): string | null => {
-  const key = getRoomSharedKeyLocalStorageKey(roomId);
-  const [roomSharedKey] = useLocalStorage(key);
+/**
+ * An upto-date shared key for the room. Either from local storage or the value from `setInitialValue`.
+ * @param roomId
+ * @param setInitialValue The value we get from the URL. The hook will overwrite the local storage value with this.
+ * @returns [roomSharedKey, setRoomSharedKey] like a react useState hook.
+ */
+const useRoomSharedKey = (
+  roomId: string,
+  setInitialValue?: string,
+): [string | null, setKey: (key: string) => void] => {
+  const [roomSharedKey, setRoomSharedKey] = useLocalStorage(
+    getRoomSharedKeyLocalStorageKey(roomId),
+  );
+  useEffect(() => {
+    // If setInitialValue is available, update the local storage (usually the password from the url).
+    // This will update roomSharedKey but wont update the returned value since
+    // that already defaults to setInitialValue.
+    if (setInitialValue) setRoomSharedKey(setInitialValue);
+  }, [setInitialValue, setRoomSharedKey]);

-  return roomSharedKey;
+  // make sure we never return the initial null value from `useLocalStorage`
+  return [setInitialValue ?? roomSharedKey, setRoomSharedKey];
 };

 export function getKeyForRoom(roomId: string): string | null {
-  saveKeyFromUrlParams(getUrlParams());
-  const key = getRoomSharedKeyLocalStorageKey(roomId);
-  return localStorage.getItem(key);
+  const { roomId: urlRoomId, password } = getUrlParams();
+  if (roomId !== urlRoomId)
+    logger.warn(
+      "requested key for a roomId which is not the current call room id (from the URL)",
+      roomId,
+      urlRoomId,
+    );
+  return (
+    password ?? localStorage.getItem(getRoomSharedKeyLocalStorageKey(roomId))
+  );
 }

-function saveKeyFromUrlParams(urlParams: UrlParams): void {
-  if (!urlParams.password || !urlParams.roomId) return;
-
-  // Take the key from the URL and save it.
-  // It's important to always use the room ID specified in the URL
-  // when saving keys rather than whatever the current room ID might be,
-  // in case we've moved to a different room but the URL hasn't changed.
-  saveKeyForRoom(urlParams.roomId, urlParams.password);
-}
-
-/**
- * Extracts the room password from the URL if one is present, saving it in localstorage
- * and returning it in a tuple with the corresponding room ID from the URL.
- * @returns A tuple of the roomId and password from the URL if the URL has both,
- *          otherwise [undefined, undefined]
- */
-const useKeyFromUrl = (): [string, string] | [undefined, undefined] => {
-  const urlParams = useUrlParams();
-
-  useEffect(() => saveKeyFromUrlParams(urlParams), [urlParams]);
-
-  return urlParams.roomId && urlParams.password
-    ? [urlParams.roomId, urlParams.password]
-    : [undefined, undefined];
-};
-
 export type Unencrypted = { kind: E2eeType.NONE };
 export type SharedSecret = { kind: E2eeType.SHARED_KEY; secret: string };
 export type PerParticipantE2EE = { kind: E2eeType.PER_PARTICIPANT };
@@ -66,12 +78,11 @@ export type EncryptionSystem = Unencrypted | SharedSecret | PerParticipantE2EE;
 export function useRoomEncryptionSystem(roomId: string): EncryptionSystem {
  const { client } = useClient();

-  // make sure we've extracted the key from the URL first
-  // (and we still need to take the value it returns because
-  // the effect won't run in time for it to save to localstorage in
-  // time for us to read it out again).
-  const [urlRoomId, passwordFromUrl] = useKeyFromUrl();
-  const storedPassword = useInternalRoomSharedKey(roomId);
+  const [storedPassword] = useRoomSharedKey(
+    getRoomSharedKeyLocalStorageKey(roomId),
+    getKeyForRoom(roomId) ?? undefined,
+  );
+
  const room = client?.getRoom(roomId);
  const e2eeSystem = <EncryptionSystem>useMemo(() => {
    if (!room) return { kind: E2eeType.NONE };
@@ -80,15 +91,10 @@ export function useRoomEncryptionSystem(roomId: string): EncryptionSystem {
        kind: E2eeType.SHARED_KEY,
        secret: storedPassword,
      };
-    if (urlRoomId === roomId)
-      return {
-        kind: E2eeType.SHARED_KEY,
-        secret: passwordFromUrl,
-      };
    if (room.hasEncryptionStateEvent()) {
      return { kind: E2eeType.PER_PARTICIPANT };
    }
    return { kind: E2eeType.NONE };
-  }, [passwordFromUrl, room, roomId, storedPassword, urlRoomId]);
+  }, [room, storedPassword]);
  return e2eeSystem;
 }
--- a/src/home/useGroupCallRooms.ts
+++ b/src/home/useGroupCallRooms.ts
@@ -81,8 +81,9 @@ function sortRooms(client: MatrixClient, rooms: Room[]): Room[] {
 }

 const roomIsJoinable = (room: Room): boolean => {
-  if (!room.hasEncryptionStateEvent() && !getKeyForRoom(room.roomId)) {
-    // if we have an non encrypted room (no encryption state event) we need a locally stored shared key.
+  const password = getKeyForRoom(room.roomId);
+  if (!room.hasEncryptionStateEvent() && !password) {
+    // if we have a non encrypted room (no encryption state event) we need a locally stored shared key.
    // in case this key also does not exists we cannot join the room.
    return false;
  }
--- a/src/livekit/useLivekit.ts
+++ b/src/livekit/useLivekit.ts
@@ -14,7 +14,7 @@ import {
  type RoomOptions,
  Track,
 } from "livekit-client";
-import { useEffect, useMemo, useRef } from "react";
+import { useEffect, useRef } from "react";
 import E2EEWorker from "livekit-client/e2ee-worker?worker";
 import { logger } from "matrix-js-sdk/lib/logger";
 import { type MatrixRTCSession } from "matrix-js-sdk/lib/matrixrtc";
@@ -24,11 +24,7 @@ import { map } from "rxjs";
 import { defaultLiveKitOptions } from "./options";
 import { type SFUConfig } from "./openIDSFU";
 import { type MuteStates } from "../room/MuteStates";
-import {
-  type MediaDeviceHandle,
-  type MediaDevices,
-  useMediaDevices,
-} from "./MediaDevicesContext";
+import { type MediaDeviceHandle, useMediaDevices } from "./MediaDevicesContext";
 import {
  type ECConnectionState,
  useECConnectionState,
@@ -40,9 +36,9 @@ import {
  useTrackProcessor,
  useTrackProcessorSync,
 } from "./TrackProcessorContext";
-import { useInitial } from "../useInitial";
 import { observeTrackReference$ } from "../state/MediaViewModel";
 import { useUrlParams } from "../UrlParams";
+import { useInitial } from "../useInitial";

 interface UseLivekitResult {
  livekitRoom?: Room;
@@ -57,27 +53,78 @@ export function useLivekit(
 ): UseLivekitResult {
  const { controlledAudioDevices } = useUrlParams();

-  const e2eeOptions = useMemo((): E2EEManagerOptions | undefined => {
-    if (e2eeSystem.kind === E2eeType.NONE) return undefined;
+  const initialMuteStates = useInitial(() => muteStates);

+  const devices = useMediaDevices();
+  const initialDevices = useInitial(() => devices);
+
+  // Store if audio/video are currently updating. If to prohibit unnecessary calls
+  // to setMicrophoneEnabled/setCameraEnabled
+  const audioMuteUpdating = useRef(false);
+  const videoMuteUpdating = useRef(false);
+  // Store the current button mute state that gets passed to this hook via props.
+  // We need to store it for awaited code that relies on the current value.
+  const buttonEnabled = useRef({
+    audio: initialMuteStates.audio.enabled,
+    video: initialMuteStates.video.enabled,
+  });
+
+  const { processor } = useTrackProcessor();
+
+  // Only ever create the room once via useInitial.
+  const room = useInitial(() => {
+    logger.info("[LivekitRoom] Create LiveKit room");
+
+    let e2ee: E2EEManagerOptions | undefined;
    if (e2eeSystem.kind === E2eeType.PER_PARTICIPANT) {
      logger.info("Created MatrixKeyProvider (per participant)");
-      return {
+      e2ee = {
        keyProvider: new MatrixKeyProvider(),
        worker: new E2EEWorker(),
      };
    } else if (e2eeSystem.kind === E2eeType.SHARED_KEY && e2eeSystem.secret) {
      logger.info("Created ExternalE2EEKeyProvider (shared key)");
-
-      return {
+      e2ee = {
        keyProvider: new ExternalE2EEKeyProvider(),
        worker: new E2EEWorker(),
      };
    }
-  }, [e2eeSystem]);

+    const roomOptions: RoomOptions = {
+      ...defaultLiveKitOptions,
+      videoCaptureDefaults: {
+        ...defaultLiveKitOptions.videoCaptureDefaults,
+        deviceId: initialDevices.videoInput.selectedId,
+        processor,
+      },
+      audioCaptureDefaults: {
+        ...defaultLiveKitOptions.audioCaptureDefaults,
+        deviceId: initialDevices.audioInput.selectedId,
+      },
+      audioOutput: {
+        deviceId: initialDevices.audioOutput.selectedId,
+      },
+      e2ee,
+    };
+    // We have to create the room manually here due to a bug inside
+    // @livekit/components-react. JSON.stringify() is used in deps of a
+    // useEffect() with an argument that references itself, if E2EE is enabled
+    const room = new Room(roomOptions);
+    room.setE2EEEnabled(e2eeSystem.kind !== E2eeType.NONE).catch((e) => {
+      logger.error("Failed to set E2EE enabled on room", e);
+    });
+
+    return room;
+  });
+
+  // Setup and update the keyProvider which was create by `createRoom`
  useEffect(() => {
-    if (e2eeSystem.kind === E2eeType.NONE || !e2eeOptions) return;
+    const e2eeOptions = room.options.e2ee;
+    if (
+      e2eeSystem.kind === E2eeType.NONE ||
+      !(e2eeOptions && "keyProvider" in e2eeOptions)
+    )
+      return;

    if (e2eeSystem.kind === E2eeType.PER_PARTICIPANT) {
      (e2eeOptions.keyProvider as MatrixKeyProvider).setRTCSession(rtcSession);
@@ -88,56 +135,7 @@ export function useLivekit(
          logger.error("Failed to set shared key for E2EE", e);
        });
    }
-  }, [e2eeOptions, e2eeSystem, rtcSession]);
-
-  const initialMuteStates = useRef<MuteStates>(muteStates);
-  const devices = useMediaDevices();
-  const initialDevices = useRef<MediaDevices>(devices);
-
-  const { processor } = useTrackProcessor();
-  const initialProcessor = useInitial(() => processor);
-  const roomOptions = useMemo(
-    (): RoomOptions => ({
-      ...defaultLiveKitOptions,
-      videoCaptureDefaults: {
-        ...defaultLiveKitOptions.videoCaptureDefaults,
-        deviceId: initialDevices.current.videoInput.selectedId,
-        processor: initialProcessor,
-      },
-      audioCaptureDefaults: {
-        ...defaultLiveKitOptions.audioCaptureDefaults,
-        deviceId: initialDevices.current.audioInput.selectedId,
-      },
-      audioOutput: {
-        deviceId: initialDevices.current.audioOutput.selectedId,
-      },
-      e2ee: e2eeOptions,
-    }),
-    [e2eeOptions, initialProcessor],
-  );
-
-  // Store if audio/video are currently updating. If to prohibit unnecessary calls
-  // to setMicrophoneEnabled/setCameraEnabled
-  const audioMuteUpdating = useRef(false);
-  const videoMuteUpdating = useRef(false);
-  // Store the current button mute state that gets passed to this hook via props.
-  // We need to store it for awaited code that relies on the current value.
-  const buttonEnabled = useRef({
-    audio: initialMuteStates.current.audio.enabled,
-    video: initialMuteStates.current.video.enabled,
-  });
-
-  // We have to create the room manually here due to a bug inside
-  // @livekit/components-react. JSON.stringify() is used in deps of a
-  // useEffect() with an argument that references itself, if E2EE is enabled
-  const room = useMemo(() => {
-    logger.info("[LivekitRooms] Create LiveKit room with options", roomOptions);
-    const r = new Room(roomOptions);
-    r.setE2EEEnabled(e2eeSystem.kind !== E2eeType.NONE).catch((e) => {
-      logger.error("Failed to set E2EE enabled on room", e);
-    });
-    return r;
-  }, [roomOptions, e2eeSystem]);
+  }, [room.options.e2ee, e2eeSystem, rtcSession]);

  // Sync the requested track processors with LiveKit
  useTrackProcessorSync(
@@ -159,8 +157,8 @@ export function useLivekit(
  );

  const connectionState = useECConnectionState(
-    initialDevices.current.audioInput.selectedId,
-    initialMuteStates.current.audio.enabled,
+    initialDevices.audioInput.selectedId,
+    initialMuteStates.audio.enabled,
    room,
    sfuConfig,
  );
--- a/src/room/GroupCallView.tsx
+++ b/src/room/GroupCallView.tsx
@@ -43,7 +43,10 @@ import { MUTE_PARTICIPANT_COUNT, type MuteStates } from "./MuteStates";
 import { useMediaDevices } from "../livekit/MediaDevicesContext";
 import { useMatrixRTCSessionMemberships } from "../useMatrixRTCSessionMemberships";
 import { enterRTCSession, leaveRTCSession } from "../rtcSessionHelpers";
-import { useRoomEncryptionSystem } from "../e2ee/sharedKeyManagement";
+import {
+  saveKeyForRoom,
+  useRoomEncryptionSystem,
+} from "../e2ee/sharedKeyManagement";
 import { useRoomAvatar } from "./useRoomAvatar";
 import { useRoomName } from "./useRoomName";
 import { useJoinRule } from "./useJoinRule";
@@ -167,6 +170,12 @@ export const GroupCallView: FC<Props> = ({
    useExperimentalToDeviceTransportSetting,
  );

+  // Save the password once we start the groupCallView
+  const { password: passwordFromUrl } = useUrlParams();
+  useEffect(() => {
+    if (passwordFromUrl) saveKeyForRoom(room.roomId, passwordFromUrl);
+  }, [passwordFromUrl, room.roomId]);
+
  usePageTitle(roomName);

  const matrixInfo = useMemo((): MatrixInfo => {
--- a/src/room/InCallView.tsx
+++ b/src/room/InCallView.tsx
@@ -147,8 +147,7 @@ export const ActiveCall: FC<ActiveCallProps> = (props) => {
          logger.error("[Lifecycle] Failed to disconnect from livekit room", e);
        });
    };
-    // eslint-disable-next-line react-hooks/exhaustive-deps
-  }, []);
+  }, [livekitRoom]);

  useEffect(() => {
    if (livekitRoom !== undefined) {
--- a/src/useLocalStorage.ts
+++ b/src/useLocalStorage.ts
@@ -13,7 +13,11 @@ type LocalStorageItem = ReturnType<typeof localStorage.getItem>;
 // Bus to notify other useLocalStorage consumers when an item is changed
 export const localStorageBus = new EventEmitter();

-// Like useState, but reads from and persists the value to localStorage
+/**
+ * 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 = (
  key: string,
 ): [LocalStorageItem, (value: string) => void] => {
@@ -41,15 +45,10 @@ export const useLocalStorage = (
  ];
 };

-export const setLocalStorageItem = (key: string, value: string): void => {
-  // Avoid unnecessary updates. Not avoiding them so can cause unexpected state updates across hooks.
-  // For instance:
-  // - In call view uses useRoomEncryptionSystem
-  // - This will set the key again.
-  // - All other instances of useRoomEncryptionSystem will now do a useMemo update of the e2eeSystem
-  //   - because the dependency `storedPassword = useInternalRoomSharedKey(roomId);` would change.
-  if (localStorage.getItem(key) === value) return;
-
+export const setLocalStorageItemReactive = (
+  key: string,
+  value: string,
+): void => {
  localStorage.setItem(key, value);
  localStorageBus.emit(key, value);
 };