Replace generateKeyed$ with a redesigned generateItems operator

And use it to clean up a number of code smells, fix some reactivity bugs, and avoid some resource leaks.
2025-11-07 17:36:16 -05:00
parent 1f386a1d57
commit b4c17ed26d
18 changed files with 610 additions and 441 deletions
--- a/src/state/CallViewModel/CallViewModel.ts
+++ b/src/state/CallViewModel/CallViewModel.ts
@@ -52,7 +52,7 @@ import {
  ScreenShareViewModel,
  type UserMediaViewModel,
 } from "../MediaViewModel";
-import { accumulate, generateKeyed$, pauseWhen } from "../../utils/observable";
+import { accumulate, generateItems, pauseWhen } from "../../utils/observable";
 import {
  duplicateTiles,
  MatrixRTCMode,
@@ -75,7 +75,7 @@ import {
 } from "../../reactions";
 import { shallowEquals } from "../../utils/array";
 import { type MediaDevices } from "../MediaDevices";
-import { type Behavior, constant } from "../Behavior";
+import { type Behavior } from "../Behavior";
 import { E2eeType } from "../../e2ee/e2eeType";
 import { MatrixKeyProvider } from "../../e2ee/matrixKeyProvider";
 import { type MuteStates } from "../MuteStates";
@@ -370,103 +370,101 @@ export class CallViewModel {
  );

  /**
-   * List of MediaItems that we want to have tiles for.
+   * List of user media (camera feeds) that we want tiles for.
   */
-  // TODO KEEP THIS!! and adapt it to what our membershipManger returns
  // TODO this also needs the local participant to be added.
-  private readonly mediaItems$ = this.scope.behavior<MediaItem[]>(
-    generateKeyed$<
-      [typeof this.matrixLivekitMembers$.value, number],
-      MediaItem,
-      MediaItem[]
-    >(
+  private readonly userMedia$ = this.scope.behavior<UserMedia[]>(
+    combineLatest([this.matrixLivekitMembers$, duplicateTiles.value$]).pipe(
      // Generate a collection of MediaItems from the list of expected (whether
      // present or missing) LiveKit participants.
-      combineLatest([this.matrixLivekitMembers$, duplicateTiles.value$]),
-      ([{ value: matrixLivekitMembers }, duplicateTiles], createOrGet) => {
-        const items: MediaItem[] = [];
-
-        for (const {
-          connection,
-          participant,
-          member,
-          displayName,
+      generateItems(
+        function* ([{ value: matrixLivekitMembers }, duplicateTiles]) {
+          for (const {
+            participantId,
+            userId,
+            participant$,
+            connection$,
+            displayName$,
+            mxcAvatarUrl$,
+          } of matrixLivekitMembers)
+            for (let dup = 0; dup < 1 + duplicateTiles; dup++)
+              yield {
+                keys: [
+                  dup,
+                  participantId,
+                  userId,
+                  participant$,
+                  connection$,
+                  displayName$,
+                  mxcAvatarUrl$,
+                ],
+                data: undefined,
+              };
+        },
+        (
+          scope,
+          _data$,
+          dup,
          participantId,
-        } of matrixLivekitMembers) {
-          if (connection === undefined) {
-            logger.warn("connection is not yet initialised.");
-            continue;
-          }
-          for (let i = 0; i < 1 + duplicateTiles; i++) {
-            const mediaId = `${participantId}:${i}`;
-            const lkRoom = connection?.livekitRoom;
-            const url = connection?.transport.livekit_service_url;
+          userId,
+          participant$,
+          connection$,
+          displayName$,
+          mxcAvatarUrl$,
+        ) => {
+          const livekitRoom$ = scope.behavior(
+            connection$.pipe(map((c) => c?.livekitRoom)),
+          );
+          const focusUrl$ = scope.behavior(
+            connection$.pipe(map((c) => c?.transport.livekit_service_url)),
+          );

-            const item = createOrGet(
-              mediaId,
-              (scope) =>
-                // We create UserMedia with or without a participant.
-                // This will be the initial value of a BehaviourSubject.
-                // Once a participant appears we will update the BehaviourSubject. (see below)
-                new UserMedia(
-                  scope,
-                  mediaId,
-                  member,
-                  participant,
-                  this.options.encryptionSystem,
-                  lkRoom,
-                  url,
-                  this.mediaDevices,
-                  this.pretendToBeDisconnected$,
-                  constant(displayName ?? "[👻]"),
-                  this.handsRaised$.pipe(
-                    map((v) => v[participantId]?.time ?? null),
-                  ),
-                  this.reactions$.pipe(
-                    map((v) => v[participantId] ?? undefined),
-                  ),
-                ),
-            );
-            items.push(item);
-            (item as UserMedia).updateParticipant(participant);
-
-            if (participant?.isScreenShareEnabled) {
-              const screenShareId = `${mediaId}:screen-share`;
-              items.push(
-                createOrGet(
-                  screenShareId,
-                  (scope) =>
-                    new ScreenShare(
-                      scope,
-                      screenShareId,
-                      member,
-                      participant,
-                      this.options.encryptionSystem,
-                      lkRoom,
-                      url,
-                      this.pretendToBeDisconnected$,
-                      constant(displayName ?? "[👻]"),
-                    ),
-                ),
-              );
-            }
-          }
-        }
-        return items;
-      },
+          return new UserMedia(
+            scope,
+            `${participantId}:${dup}`,
+            userId,
+            participant$,
+            this.options.encryptionSystem,
+            livekitRoom$,
+            focusUrl$,
+            this.mediaDevices,
+            this.pretendToBeDisconnected$,
+            displayName$,
+            mxcAvatarUrl$,
+            this.handsRaised$.pipe(map((v) => v[participantId]?.time ?? null)),
+            this.reactions$.pipe(map((v) => v[participantId] ?? undefined)),
+          );
+        },
+      ),
    ),
  );

  /**
-   * List of MediaItems that we want to display, that are of type UserMedia
+   * List of all media items (user media and screen share media) that we want
+   * tiles for.
   */
-  private readonly userMedia$ = this.scope.behavior<UserMedia[]>(
-    this.mediaItems$.pipe(
-      map((mediaItems) =>
-        mediaItems.filter((m): m is UserMedia => m instanceof UserMedia),
+  private readonly mediaItems$ = this.scope.behavior<MediaItem[]>(
+    this.userMedia$.pipe(
+      switchMap((userMedia) =>
+        userMedia.length === 0
+          ? of([])
+          : combineLatest(
+              userMedia.map((m) => m.screenShares$),
+              (...screenShares) => [...userMedia, ...screenShares.flat(1)],
+            ),
+      ),
+    ),
+  );
+
+  /**
+   * List of MediaItems that we want to display, that are of type ScreenShare
+   */
+  private readonly screenShares$ = this.scope.behavior<ScreenShare[]>(
+    this.mediaItems$.pipe(
+      map((mediaItems) =>
+        mediaItems.filter((m): m is ScreenShare => m instanceof ScreenShare),
      ),
    ),
-    [],
  );

  public readonly joinSoundEffect$ = this.userMedia$.pipe(
@@ -544,17 +542,6 @@ export class CallViewModel {
    tap((reason) => this.leaveHoisted$.next(reason)),
  );

-  /**
-   * List of MediaItems that we want to display, that are of type ScreenShare
-   */
-  private readonly screenShares$ = this.scope.behavior<ScreenShare[]>(
-    this.mediaItems$.pipe(
-      map((mediaItems) =>
-        mediaItems.filter((m): m is ScreenShare => m instanceof ScreenShare),
-      ),
-    ),
-  );
-
  private readonly spotlightSpeaker$ =
    this.scope.behavior<UserMediaViewModel | null>(
      this.userMedia$.pipe(
--- a/src/state/CallViewModel/localMember/LocalMembership.ts
+++ b/src/state/CallViewModel/localMember/LocalMembership.ts
@@ -5,7 +5,13 @@ SPDX-License-IdFentifier: AGPL-3.0-only OR LicenseRef-Element-Commercial
 Please see LICENSE in the repository root for full details.
 */

-import { type LocalTrack, type E2EEOptions } from "livekit-client";
+import {
+  type LocalTrack,
+  type E2EEOptions,
+  type Participant,
+  ParticipantEvent,
+} from "livekit-client";
+import { observeParticipantEvents } from "@livekit/components-core";
 import {
  type LivekitTransport,
  type MatrixRTCSession,
@@ -26,11 +32,9 @@ import {
  switchMap,
  take,
  takeWhile,
-  tap,
 } from "rxjs";
 import { logger } from "matrix-js-sdk/lib/logger";

-import { sharingScreen$ as observeSharingScreen$ } from "../../UserMedia.ts";
 import { type Behavior } from "../../Behavior";
 import { type IConnectionManager } from "../remoteMembers/ConnectionManager";
 import { ObservableScope } from "../../ObservableScope";
@@ -521,3 +525,13 @@ export const createLocalMembership$ = ({
    toggleScreenSharing,
  };
 };
+
+export function observeSharingScreen$(p: Participant): Observable<boolean> {
+  return observeParticipantEvents(
+    p,
+    ParticipantEvent.TrackPublished,
+    ParticipantEvent.TrackUnpublished,
+    ParticipantEvent.LocalTrackPublished,
+    ParticipantEvent.LocalTrackUnpublished,
+  ).pipe(map((p) => p.isScreenShareEnabled));
+}
--- a/src/state/CallViewModel/remoteMembers/ConnectionManager.ts
+++ b/src/state/CallViewModel/remoteMembers/ConnectionManager.ts
@@ -20,7 +20,7 @@ import { type LocalParticipant, type RemoteParticipant } from "livekit-client";
 import { type Behavior } from "../../Behavior.ts";
 import { type Connection } from "./Connection.ts";
 import { Epoch, type ObservableScope } from "../../ObservableScope.ts";
-import { generateKeyed$ } from "../../../utils/observable.ts";
+import { generateItemsWithEpoch } from "../../../utils/observable.ts";
 import { areLivekitTransportsEqual } from "./MatrixLivekitMembers.ts";
 import { type ConnectionFactory } from "./ConnectionFactory.ts";

@@ -144,34 +144,32 @@ export function createConnectionManager$({
   * Connections for each transport in use by one or more session members.
   */
  const connections$ = scope.behavior(
-    generateKeyed$<Epoch<LivekitTransport[]>, Connection, Epoch<Connection[]>>(
-      transports$,
-      (transports, createOrGet) => {
-        const createConnection =
-          (
-            transport: LivekitTransport,
-          ): ((scope: ObservableScope) => Connection) =>
-          (scope) => {
-            const connection = connectionFactory.createConnection(
-              transport,
-              scope,
-              logger,
-            );
-            // Start the connection immediately
-            // Use connection state to track connection progress
-            void connection.start();
-            // TODO subscribe to connection state to retry or log issues?
-            return connection;
-          };
-
-        return transports.mapInner((transports) => {
-          return transports.map((transport) => {
-            const key =
-              transport.livekit_service_url + "|" + transport.livekit_alias;
-            return createOrGet(key, createConnection(transport));
-          });
-        });
-      },
+    transports$.pipe(
+      generateItemsWithEpoch(
+        function* (transports) {
+          for (const transport of transports)
+            yield {
+              keys: [transport.livekit_service_url, transport.livekit_alias],
+              data: undefined,
+            };
+        },
+        (scope, _data$, serviceUrl, alias) => {
+          const connection = connectionFactory.createConnection(
+            {
+              type: "livekit",
+              livekit_service_url: serviceUrl,
+              livekit_alias: alias,
+            },
+            scope,
+            logger,
+          );
+          // Start the connection immediately
+          // Use connection state to track connection progress
+          void connection.start();
+          // TODO subscribe to connection state to retry or log issues?
+          return connection;
+        },
+      ),
    ),
  );

--- a/src/state/CallViewModel/remoteMembers/MatrixLivekitMembers.ts
+++ b/src/state/CallViewModel/remoteMembers/MatrixLivekitMembers.ts
@@ -16,32 +16,31 @@ import {
 import { combineLatest, filter, map } from "rxjs";
 // eslint-disable-next-line rxjs/no-internal
 import { type NodeStyleEventEmitter } from "rxjs/internal/observable/fromEvent";
-import { type Room as MatrixRoom, type RoomMember } from "matrix-js-sdk";
+import { type Room as MatrixRoom } from "matrix-js-sdk";
+import { logger } from "matrix-js-sdk/lib/logger";

 import { type Behavior } from "../../Behavior";
 import { type IConnectionManager } from "./ConnectionManager";
 import { Epoch, mapEpoch, type ObservableScope } from "../../ObservableScope";
-import { getRoomMemberFromRtcMember, memberDisplaynames$ } from "./displayname";
+import { memberDisplaynames$ } from "./displayname";
 import { type Connection } from "./Connection";
+import { generateItemsWithEpoch } from "../../../utils/observable";

 /**
- * Represent a matrix call member and his associated livekit participation.
+ * Represents a Matrix call member and their associated LiveKit participation.
 * `livekitParticipant` can be undefined if the member is not yet connected to the livekit room
 * or if it has no livekit transport at all.
 */
 export interface MatrixLivekitMember {
-  membership: CallMembership;
-  displayName?: string;
-  participant?: LocalLivekitParticipant | RemoteLivekitParticipant;
-  connection?: Connection;
-  /**
-   * TODO Try to remove this! Its waaay to much information.
-   * Just get the member's avatar
-   * @deprecated
-   */
-  member: RoomMember;
-  mxcAvatarUrl?: string;
  participantId: string;
+  userId: string;
+  membership$: Behavior<CallMembership>;
+  participant$: Behavior<
+    LocalLivekitParticipant | RemoteLivekitParticipant | null
+  >;
+  connection$: Behavior<Connection | undefined>;
+  displayName$: Behavior<string>;
+  mxcAvatarUrl$: Behavior<string | undefined>;
 }

 interface Props {
@@ -100,44 +99,54 @@ export function createMatrixLivekitMembers$({
          { value: membershipsWithTransports, epoch },
          { value: managerData },
          { value: displaynames },
-        ]) => {
-          const items: MatrixLivekitMember[] = membershipsWithTransports.map(
-            ({ membership, transport }) => {
-              // TODO! cannot use membership.membershipID yet, Currently its hardcoded by the jwt service to
-              const participantId = /*membership.membershipID*/ `${membership.userId}:${membership.deviceId}`;
+        ]) =>
+          new Epoch(
+            [membershipsWithTransports, managerData, displaynames] as const,
+            epoch,
+          ),
+      ),
+      generateItemsWithEpoch(
+        function* ([membershipsWithTransports, managerData, displaynames]) {
+          for (const { membership, transport } of membershipsWithTransports) {
+            // TODO! cannot use membership.membershipID yet, Currently its hardcoded by the jwt service to
+            const participantId = /*membership.membershipID*/ `${membership.userId}:${membership.deviceId}`;

-              const participants = transport
-                ? managerData.getParticipantForTransport(transport)
-                : [];
-              const participant = participants.find(
-                (p) => p.identity == participantId,
-              );
-              const member = getRoomMemberFromRtcMember(
+            const participants = transport
+              ? managerData.getParticipantForTransport(transport)
+              : [];
+            const participant =
+              participants.find((p) => p.identity == participantId) ?? null;
+            // This makes sense to add to the js-sdk callMembership (we only need the avatar so probably the call memberhsip just should aquire the avatar)
+            const member = matrixRoom.getMember(membership.userId);
+            const connection = transport
+              ? managerData.getConnectionForTransport(transport)
+              : undefined;
+
+            let displayName = displaynames.get(membership.userId);
+            if (displayName === undefined) {
+              logger.warn(`No display name for user ${membership.userId}`);
+              displayName = "";
+            }
+
+            yield {
+              keys: [participantId, membership.userId],
+              data: {
                membership,
-                matrixRoom,
-              )?.member;
-              const connection = transport
-                ? managerData.getConnectionForTransport(transport)
-                : undefined;
-              const displayName = displaynames.get(participantId);
-              return {
                participant,
-                membership,
                connection,
-                // This makes sense to add to the js-sdk callMembership (we only need the avatar so probably the call memberhsip just should aquire the avatar)
-                // TODO Ugh this is hidign that it might be undefined!! best we remove the member entirely.
-                member: member as RoomMember,
                displayName,
                mxcAvatarUrl: member?.getMxcAvatarUrl(),
-                participantId,
-              };
-            },
-          );
-          return new Epoch(items, epoch);
+              },
+            };
+          }
        },
+        (scope, data$, participantId, userId) => ({
+          participantId,
+          userId,
+          ...scope.splitBehavior(data$),
+        }),
      ),
    ),
-    // new Epoch([]),
  );
 }

--- a/src/state/CallViewModel/remoteMembers/displayname.test.ts
+++ b/src/state/CallViewModel/remoteMembers/displayname.test.ts
@@ -97,7 +97,7 @@ test.skip("should always have our own user", () => {

    expectObservable(dn$.pipe(map((e) => e.value))).toBe("a", {
      a: new Map<string, string>([
-        ["@local:example.com:DEVICE000", "@local:example.com"],
+        ["@local:example.com", "@local:example.com"],
      ]),
    });
  });
@@ -130,9 +130,9 @@ test("should get displayName for users", () => {

    expectObservable(dn$.pipe(map((e) => e.value))).toBe("a", {
      a: new Map<string, string>([
-        // ["@local:example.com:DEVICE000", "it's a me"],
-        ["@alice:example.com:DEVICE1", "Alice"],
-        ["@bob:example.com:DEVICE1", "Bob"],
+        // ["@local:example.com", "it's a me"],
+        ["@alice:example.com", "Alice"],
+        ["@bob:example.com", "Bob"],
      ]),
    });
  });
@@ -152,8 +152,8 @@ test("should use userId if no display name", () => {

    expectObservable(dn$.pipe(map((e) => e.value))).toBe("a", {
      a: new Map<string, string>([
-        // ["@local:example.com:DEVICE000", "it's a me"],
-        ["@no-name:foo.bar:D000", "@no-name:foo.bar"],
+        // ["@local:example.com", "it's a me"],
+        ["@no-name:foo.bar", "@no-name:foo.bar"],
      ]),
    });
  });
@@ -179,12 +179,12 @@ test("should disambiguate users with same display name", () => {

    expectObservable(dn$.pipe(map((e) => e.value))).toBe("a", {
      a: new Map<string, string>([
-        // ["@local:example.com:DEVICE000", "it's a me"],
-        ["@bob:example.com:DEVICE1", "Bob (@bob:example.com)"],
-        ["@bob:example.com:DEVICE2", "Bob (@bob:example.com)"],
-        ["@bob:foo.bar:BOB000", "Bob (@bob:foo.bar)"],
-        ["@carl:example.com:C000", "Carl (@carl:example.com)"],
-        ["@evil:example.com:E000", "Carl (@evil:example.com)"],
+        // ["@local:example.com", "it's a me"],
+        ["@bob:example.com", "Bob (@bob:example.com)"],
+        ["@bob:example.com", "Bob (@bob:example.com)"],
+        ["@bob:foo.bar", "Bob (@bob:foo.bar)"],
+        ["@carl:example.com", "Carl (@carl:example.com)"],
+        ["@evil:example.com", "Carl (@evil:example.com)"],
      ]),
    });
  });
@@ -208,13 +208,13 @@ test("should disambiguate when needed", () => {

    expectObservable(dn$.pipe(map((e) => e.value))).toBe("ab", {
      a: new Map<string, string>([
-        // ["@local:example.com:DEVICE000", "it's a me"],
-        ["@bob:example.com:DEVICE1", "Bob"],
+        // ["@local:example.com", "it's a me"],
+        ["@bob:example.com", "Bob"],
      ]),
      b: new Map<string, string>([
-        // ["@local:example.com:DEVICE000", "it's a me"],
-        ["@bob:example.com:DEVICE1", "Bob (@bob:example.com)"],
-        ["@bob:foo.bar:BOB000", "Bob (@bob:foo.bar)"],
+        // ["@local:example.com", "it's a me"],
+        ["@bob:example.com", "Bob (@bob:example.com)"],
+        ["@bob:foo.bar", "Bob (@bob:foo.bar)"],
      ]),
    });
  });
@@ -238,13 +238,13 @@ test.skip("should keep disambiguated name when other leave", () => {

    expectObservable(dn$.pipe(map((e) => e.value))).toBe("ab", {
      a: new Map<string, string>([
-        // ["@local:example.com:DEVICE000", "it's a me"],
-        ["@bob:example.com:DEVICE1", "Bob (@bob:example.com)"],
-        ["@bob:foo.bar:BOB000", "Bob (@bob:foo.bar)"],
+        // ["@local:example.com", "it's a me"],
+        ["@bob:example.com", "Bob (@bob:example.com)"],
+        ["@bob:foo.bar", "Bob (@bob:foo.bar)"],
      ]),
      b: new Map<string, string>([
-        // ["@local:example.com:DEVICE000", "it's a me"],
-        ["@bob:example.com:DEVICE1", "Bob (@bob:example.com)"],
+        // ["@local:example.com", "it's a me"],
+        ["@bob:example.com", "Bob (@bob:example.com)"],
      ]),
    });
  });
@@ -273,14 +273,14 @@ test("should disambiguate on name change", () => {

    expectObservable(dn$.pipe(map((e) => e.value))).toBe("ab", {
      a: new Map<string, string>([
-        // ["@local:example.com:DEVICE000", "it's a me"],
-        ["@bob:example.com:B000", "Bob"],
-        ["@carl:example.com:C000", "Carl"],
+        // ["@local:example.com", "it's a me"],
+        ["@bob:example.com", "Bob"],
+        ["@carl:example.com", "Carl"],
      ]),
      b: new Map<string, string>([
-        // ["@local:example.com:DEVICE000", "it's a me"],
-        ["@bob:example.com:B000", "Bob (@bob:example.com)"],
-        ["@carl:example.com:C000", "Bob (@carl:example.com)"],
+        // ["@local:example.com", "it's a me"],
+        ["@bob:example.com", "Bob (@bob:example.com)"],
+        ["@carl:example.com", "Bob (@carl:example.com)"],
      ]),
    });
  });
--- a/src/state/CallViewModel/remoteMembers/displayname.ts
+++ b/src/state/CallViewModel/remoteMembers/displayname.ts
@@ -42,7 +42,7 @@ export function createRoomMembers$(
 * any displayname that clashes with another member. Only members
 * joined to the call are considered here.
 *
- * @returns Map<member.id, displayname> uses the rtc member idenitfier as the key.
+ * @returns Map<userId, displayname> uses the Matrix user ID as the key.
 */
 // don't do this work more times than we need to. This is achieved by converting to a behavior:
 export const memberDisplaynames$ = (
@@ -66,19 +66,14 @@ export const memberDisplaynames$ = (

        // We only consider RTC members for disambiguation as they are the only visible members.
        for (const rtcMember of memberships) {
-          // TODO a hard-coded participant ID ? should use rtcMember.membershipID instead?
-          const matrixIdentifier = `${rtcMember.userId}:${rtcMember.deviceId}`;
-          const { member } = getRoomMemberFromRtcMember(rtcMember, room);
-          if (!member) {
-            logger.error(
-              "Could not find member for participant id:",
-              matrixIdentifier,
-            );
+          const member = room.getMember(rtcMember.userId);
+          if (member === null) {
+            logger.error(`Could not find member for user ${rtcMember.userId}`);
            continue;
          }
          const disambiguate = shouldDisambiguate(member, memberships, room);
          displaynameMap.set(
-            matrixIdentifier,
+            rtcMember.userId,
            calculateDisplayName(member, disambiguate),
          );
        }
@@ -87,13 +82,3 @@ export const memberDisplaynames$ = (
    ),
    new Epoch(new Map<string, string>()),
  );
-
-export function getRoomMemberFromRtcMember(
-  rtcMember: CallMembership,
-  room: Pick<MatrixRoom, "getMember">,
-): { id: string; member: RoomMember | undefined } {
-  return {
-    id: rtcMember.userId + ":" + rtcMember.deviceId,
-    member: room.getMember(rtcMember.userId) ?? undefined,
-  };
-}