Never skip lobby for huddles (#3508) (#3509)

* Never skip lobby for huddles



---------

Signed-off-by: Timo K <toger5@hotmail.de>

.github/workflows/publish-embedded-packages.yaml

@@ -44,6 +44,8 @@ jobs:
         run: |
           if [[ "${VERSION}" =~ ^v[0-9]+\.[0-9]+\.[0-9]+$ ]]; then
             echo "TAG=latest" >> "$GITHUB_OUTPUT"
+          elif [[ "${VERSION}" =~ ^v[0-9]+\.[0-9]+\.[0-9]+\-rc\.[0-9]+$ ]]; then
+            echo "TAG=rc" >> "$GITHUB_OUTPUT"
           else
             echo "TAG=other" >> "$GITHUB_OUTPUT"
           fi
@@ -163,6 +165,8 @@ jobs:
         run: |
           if [[ "${{ needs.versioning.outputs.TAG }}" == "latest" ]]; then
             echo "ARTIFACT_VERSION=${{ needs.versioning.outputs.UNPREFIXED_VERSION }}" >> "$GITHUB_ENV"
+          elif [[ "${{ needs.versioning.outputs.TAG }}" == "rc" ]]; then
+            echo "ARTIFACT_VERSION=${{ needs.versioning.outputs.UNPREFIXED_VERSION }}" >> "$GITHUB_ENV"
           else
             echo "ARTIFACT_VERSION=${{ needs.versioning.outputs.UNPREFIXED_VERSION }}-SNAPSHOT" >> "$GITHUB_ENV"
           fi

src/UrlParams.test.ts

@@ -5,12 +5,15 @@ SPDX-License-Identifier: AGPL-3.0-only OR LicenseRef-Element-Commercial
 Please see LICENSE in the repository root for full details.
 */
 
-import { describe, expect, it } from "vitest";
+import { describe, expect, it, onTestFinished, vi } from "vitest";
+import { logger } from "matrix-js-sdk/lib/logger";
 
+import * as PlatformMod from "../src/Platform";
 import {
   getRoomIdentifierFromUrl,
-  getUrlParams,
+  computeUrlParams,
   HeaderStyle,
+  getUrlParams,
 } from "../src/UrlParams";
 
 const ROOM_NAME = "roomNameHere";
@@ -103,16 +106,16 @@ describe("UrlParams", () => {
 
   describe("preload", () => {
     it("defaults to false", () => {
-      expect(getUrlParams().preload).toBe(false);
+      expect(computeUrlParams().preload).toBe(false);
     });
 
     it("ignored in SPA mode", () => {
-      expect(getUrlParams("?preload=true").preload).toBe(false);
+      expect(computeUrlParams("?preload=true").preload).toBe(false);
     });
 
     it("respected in widget mode", () => {
       expect(
-        getUrlParams(
+        computeUrlParams(
           "?preload=true&widgetId=12345&parentUrl=https%3A%2F%2Flocalhost%2Ffoo",
         ).preload,
       ).toBe(true);
@@ -121,19 +124,20 @@ describe("UrlParams", () => {
 
   describe("returnToLobby", () => {
     it("is false in SPA mode", () => {
-      expect(getUrlParams("?returnToLobby=true").returnToLobby).toBe(false);
+      expect(computeUrlParams("?returnToLobby=true").returnToLobby).toBe(false);
     });
 
     it("defaults to false in widget mode", () => {
       expect(
-        getUrlParams("?widgetId=12345&parentUrl=https%3A%2F%2Flocalhost%2Ffoo")
-          .returnToLobby,
+        computeUrlParams(
+          "?widgetId=12345&parentUrl=https%3A%2F%2Flocalhost%2Ffoo",
+        ).returnToLobby,
       ).toBe(false);
     });
 
     it("respected in widget mode", () => {
       expect(
-        getUrlParams(
+        computeUrlParams(
           "?returnToLobby=true&widgetId=12345&parentUrl=https%3A%2F%2Flocalhost%2Ffoo",
         ).returnToLobby,
       ).toBe(true);
@@ -142,12 +146,12 @@ describe("UrlParams", () => {
 
   describe("userId", () => {
     it("is ignored in SPA mode", () => {
-      expect(getUrlParams("?userId=asd").userId).toBe(null);
+      expect(computeUrlParams("?userId=asd").userId).toBe(null);
     });
 
     it("is parsed in widget mode", () => {
       expect(
-        getUrlParams(
+        computeUrlParams(
           "?userId=asd&widgetId=12345&parentUrl=https%3A%2F%2Flocalhost%2Ffoo",
         ).userId,
       ).toBe("asd");
@@ -156,12 +160,12 @@ describe("UrlParams", () => {
 
   describe("deviceId", () => {
     it("is ignored in SPA mode", () => {
-      expect(getUrlParams("?deviceId=asd").deviceId).toBe(null);
+      expect(computeUrlParams("?deviceId=asd").deviceId).toBe(null);
     });
 
     it("is parsed in widget mode", () => {
       expect(
-        getUrlParams(
+        computeUrlParams(
           "?deviceId=asd&widgetId=12345&parentUrl=https%3A%2F%2Flocalhost%2Ffoo",
         ).deviceId,
       ).toBe("asd");
@@ -170,12 +174,12 @@ describe("UrlParams", () => {
 
   describe("baseUrl", () => {
     it("is ignored in SPA mode", () => {
-      expect(getUrlParams("?baseUrl=asd").baseUrl).toBe(null);
+      expect(computeUrlParams("?baseUrl=asd").baseUrl).toBe(null);
     });
 
     it("is parsed in widget mode", () => {
       expect(
-        getUrlParams(
+        computeUrlParams(
           "?baseUrl=asd&widgetId=12345&parentUrl=https%3A%2F%2Flocalhost%2Ffoo",
         ).baseUrl,
       ).toBe("asd");
@@ -185,28 +189,28 @@ describe("UrlParams", () => {
   describe("viaServers", () => {
     it("is ignored in widget mode", () => {
       expect(
-        getUrlParams(
+        computeUrlParams(
           "?viaServers=asd&widgetId=12345&parentUrl=https%3A%2F%2Flocalhost%2Ffoo",
         ).viaServers,
       ).toBe(null);
     });
 
     it("is parsed in SPA mode", () => {
-      expect(getUrlParams("?viaServers=asd").viaServers).toBe("asd");
+      expect(computeUrlParams("?viaServers=asd").viaServers).toBe("asd");
     });
   });
 
   describe("homeserver", () => {
     it("is ignored in widget mode", () => {
       expect(
-        getUrlParams(
+        computeUrlParams(
           "?homeserver=asd&widgetId=12345&parentUrl=https%3A%2F%2Flocalhost%2Ffoo",
         ).homeserver,
       ).toBe(null);
     });
 
     it("is parsed in SPA mode", () => {
-      expect(getUrlParams("?homeserver=asd").homeserver).toBe("asd");
+      expect(computeUrlParams("?homeserver=asd").homeserver).toBe("asd");
     });
   });
 
@@ -237,7 +241,7 @@ describe("UrlParams", () => {
       controlledAudioDevices: platform === "desktop" ? false : true,
       skipLobby: true,
       returnToLobby: false,
-      sendNotificationType: "notification",
+      sendNotificationType: platform === "desktop" ? "notification" : "ring",
     });
     const joinExistingCallDefaults = (platform: string): object => ({
       confineToRoom: true,
@@ -252,24 +256,55 @@ describe("UrlParams", () => {
       skipLobby: false,
       returnToLobby: false,
       sendNotificationType: "notification",
+      defaultAudioEnabled: true,
+      defaultVideoEnabled: true,
     });
     it("use no-intent-defaults with unknown intent", () => {
-      expect(getUrlParams()).toMatchObject(noIntentDefaults);
+      expect(computeUrlParams()).toMatchObject(noIntentDefaults);
     });
 
     it("ignores intent if it is not a valid value", () => {
-      expect(getUrlParams("?intent=foo")).toMatchObject(noIntentDefaults);
+      expect(computeUrlParams("?intent=foo")).toMatchObject(noIntentDefaults);
     });
 
     it("accepts start_call", () => {
       expect(
-        getUrlParams("?intent=start_call&widgetId=1234&parentUrl=parent.org"),
-      ).toMatchObject(startNewCallDefaults("desktop"));
+        computeUrlParams(
+          "?intent=start_call&widgetId=1234&parentUrl=parent.org",
+        ),
+      ).toMatchObject({ ...startNewCallDefaults("desktop"), skipLobby: false });
+    });
+
+    it("accepts start_call_dm mobile", () => {
+      vi.spyOn(PlatformMod, "platform", "get").mockReturnValue("android");
+      onTestFinished(() => {
+        vi.spyOn(PlatformMod, "platform", "get").mockReturnValue("desktop");
+      });
+      expect(
+        computeUrlParams(
+          "?intent=start_call_dm&widgetId=1234&parentUrl=parent.org",
+        ),
+      ).toMatchObject(startNewCallDefaults("android"));
+    });
+
+    it("accepts start_call_dm mobile and prioritizes overwritten params", () => {
+      vi.spyOn(PlatformMod, "platform", "get").mockReturnValue("android");
+      onTestFinished(() => {
+        vi.spyOn(PlatformMod, "platform", "get").mockReturnValue("desktop");
+      });
+      expect(
+        computeUrlParams(
+          "?intent=start_call_dm&widgetId=1234&parentUrl=parent.org&sendNotificationType=notification",
+        ),
+      ).toMatchObject({
+        ...startNewCallDefaults("android"),
+        sendNotificationType: "notification",
+      });
     });
 
     it("accepts join_existing", () => {
       expect(
-        getUrlParams(
+        computeUrlParams(
           "?intent=join_existing&widgetId=1234&parentUrl=parent.org",
         ),
       ).toMatchObject(joinExistingCallDefaults("desktop"));
@@ -278,31 +313,55 @@ describe("UrlParams", () => {
 
   describe("skipLobby", () => {
     it("defaults to false", () => {
-      expect(getUrlParams().skipLobby).toBe(false);
+      expect(computeUrlParams().skipLobby).toBe(false);
     });
 
     it("defaults to false if intent is start_call in SPA mode", () => {
-      expect(getUrlParams("?intent=start_call").skipLobby).toBe(false);
+      expect(computeUrlParams("?intent=start_call").skipLobby).toBe(false);
     });
 
-    it("defaults to true if intent is start_call in widget mode", () => {
+    it("defaults to false if intent is start_call in widget mode", () => {
       expect(
-        getUrlParams(
+        computeUrlParams(
           "?intent=start_call&widgetId=12345&parentUrl=https%3A%2F%2Flocalhost%2Ffoo",
         ).skipLobby,
-      ).toBe(true);
+      ).toBe(false);
     });
 
     it("default to false if intent is join_existing", () => {
-      expect(getUrlParams("?intent=join_existing").skipLobby).toBe(false);
+      expect(computeUrlParams("?intent=join_existing").skipLobby).toBe(false);
     });
   });
   describe("header", () => {
     it("uses header if provided", () => {
-      expect(getUrlParams("?header=app_bar&hideHeader=true").header).toBe(
+      expect(computeUrlParams("?header=app_bar&hideHeader=true").header).toBe(
         "app_bar",
       );
-      expect(getUrlParams("?header=none&hideHeader=false").header).toBe("none");
+      expect(computeUrlParams("?header=none&hideHeader=false").header).toBe(
+        "none",
+      );
+    });
+  });
+  describe("getUrlParams", () => {
+    it("uses cached values", () => {
+      const spy = vi.spyOn(logger, "info");
+      // call get once
+      const params = getUrlParams("?header=app_bar&hideHeader=true", "");
+      // call get twice
+      expect(getUrlParams("?header=app_bar&hideHeader=true", "")).toBe(params);
+      // expect compute to only be called once
+      // it will only log when it is computing the values
+      expect(spy).toHaveBeenCalledExactlyOnceWith(
+        "UrlParams: final set of url params\n",
+        "intent:",
+        "unknown",
+        "\nproperties:",
+        expect.any(Object),
+        "configuration:",
+        expect.any(Object),
+        "intentAndPlatformDerivedConfiguration:",
+        {},
+      );
     });
   });
 });

src/UrlParams.ts

@@ -322,8 +322,9 @@ let urlParamCache: {
   hash?: string;
   params?: UrlParams;
 } = {};
+
 /**
- * Gets the app parameters for the current URL.
+ * Gets the url params and loads them from a cache if already computed.
  * @param search The URL search string
  * @param hash The URL hash
  * @returns The app parameters encoded in the URL
@@ -331,18 +332,27 @@ let urlParamCache: {
 export const getUrlParams = (
   search = window.location.search,
   hash = window.location.hash,
-  /** Skipping the cache might be needed in tests, to allow recomputing based on mocked platform changes. */
-  skipCache = false,
 ): UrlParams => {
-  // Only run the param configuration if we do not yet have it cached for this url.
   if (
     urlParamCache.search === search &&
     urlParamCache.hash === hash &&
-    urlParamCache.params &&
-    !skipCache
+    urlParamCache.params
   ) {
     return urlParamCache.params;
   }
+  const params = computeUrlParams(search, hash);
+  urlParamCache = { search, hash, params };
+
+  return params;
+};
+
+/**
+ * Gets the app parameters for the current URL.
+ * @param search The URL search string
+ * @param hash The URL hash
+ * @returns The app parameters encoded in the URL
+ */
+export const computeUrlParams = (search = "", hash = ""): UrlParams => {
   const parser = new ParamParser(search, hash);
 
   const fontScale = parseFloat(parser.getParam("fontScale") ?? "");
@@ -378,13 +388,13 @@ export const getUrlParams = (
     controlledAudioDevices: platform === "desktop" ? false : true,
     skipLobby: true,
     returnToLobby: false,
-    sendNotificationType: "notification" as RTCNotificationType,
+    sendNotificationType: "notification",
     autoLeaveWhenOthersLeft: false,
     waitForCallPickup: false,
   };
   switch (intent) {
     case UserIntent.StartNewCall:
-      intentPreset.skipLobby = true;
+      intentPreset.skipLobby = false;
       break;
     case UserIntent.JoinExistingCall:
       // On desktop this will be overridden based on which button was used to join the call
@@ -392,6 +402,7 @@ export const getUrlParams = (
       break;
     case UserIntent.StartNewCallDM:
       intentPreset.skipLobby = true;
+      intentPreset.sendNotificationType = "ring";
       intentPreset.autoLeaveWhenOthersLeft = true;
       intentPreset.waitForCallPickup = true;
 
@@ -505,15 +516,12 @@ export const getUrlParams = (
     intentAndPlatformDerivedConfiguration,
   );
 
-  const params = {
+  return {
     ...properties,
     ...intentPreset,
     ...pickBy(configuration, (v?: unknown) => v !== undefined),
     ...intentAndPlatformDerivedConfiguration,
   };
-  urlParamCache = { search, hash, params };
-
-  return params;
 };
 
 /**

src/state/CallViewModel.test.ts

@@ -266,7 +266,7 @@ const mockLegacyRingEvent = {} as { event_id: string } & ICallNotifyContent;
 interface CallViewModelInputs {
   remoteParticipants$: Behavior<RemoteParticipant[]>;
   rtcMembers$: Behavior<Partial<CallMembership>[]>;
-  connectionState$: Observable<ECConnectionState>;
+  livekitConnectionState$: Observable<ECConnectionState>;
   speaking: Map<Participant, Observable<boolean>>;
   mediaDevices: MediaDevices;
   initialSyncState: SyncState;
@@ -276,7 +276,7 @@ function withCallViewModel(
   {
     remoteParticipants$ = constant([]),
     rtcMembers$ = constant([localRtcMember]),
-    connectionState$ = of(ConnectionState.Connected),
+    livekitConnectionState$: connectionState$ = of(ConnectionState.Connected),
     speaking = new Map(),
     mediaDevices = mockMediaDevices({}),
     initialSyncState = SyncState.Syncing,
@@ -384,7 +384,7 @@ test("participants are retained during a focus switch", () => {
           b: [],
         }),
         rtcMembers$: constant([localRtcMember, aliceRtcMember, bobRtcMember]),
-        connectionState$: behavior(connectionInputMarbles, {
+        livekitConnectionState$: behavior(connectionInputMarbles, {
           c: ConnectionState.Connected,
           s: ECAddonConnectionState.ECSwitchingFocus,
         }),
@@ -1251,6 +1251,41 @@ describe("waitForCallPickup$", () => {
     });
   });
 
+  test("regression test: does stop ringing in case livekitConnectionState$ emits after didSendCallNotification$ has already emitted", () => {
+    withTestScheduler(({ schedule, expectObservable, behavior }) => {
+      withCallViewModel(
+        {
+          livekitConnectionState$: behavior("d 9ms c", {
+            d: ConnectionState.Disconnected,
+            c: ConnectionState.Connected,
+          }),
+        },
+        (vm, rtcSession) => {
+          // Fire a call notification IMMEDIATELY (its important for this test, that this happens before the livekitConnectionState$ emits)
+          schedule("n", {
+            n: () => {
+              rtcSession.emit(
+                MatrixRTCSessionEvent.DidSendCallNotification,
+                mockRingEvent("$notif1", 30),
+                mockLegacyRingEvent,
+              );
+            },
+          });
+
+          expectObservable(vm.callPickupState$).toBe("a 9ms b 29ms c", {
+            a: "unknown",
+            b: "ringing",
+            c: "timeout",
+          });
+        },
+        {
+          waitForCallPickup: true,
+          encryptionSystem: { kind: E2eeType.PER_PARTICIPANT },
+        },
+      );
+    });
+  });
+
   test("ringing -> success if someone joins before timeout", () => {
     withTestScheduler(({ behavior, schedule, expectObservable }) => {
       // Someone joins at 20ms (both LiveKit participant and MatrixRTC member)
@@ -1305,7 +1340,7 @@ describe("waitForCallPickup$", () => {
             a: [localRtcMember],
             b: [localRtcMember, aliceRtcMember],
           }),
-          connectionState$,
+          livekitConnectionState$: connectionState$,
         },
         (vm, rtcSession) => {
           // Notify at 5ms so we enter ringing, then get disconnected 5ms later

src/state/CallViewModel.ts

@@ -880,60 +880,68 @@ export class CallViewModel extends ViewModel {
     ? this.allOthersLeft$
     : NEVER;
 
+  private readonly didSendCallNotification$ = fromEvent(
+    this.matrixRTCSession,
+    MatrixRTCSessionEvent.DidSendCallNotification,
+  ) as Observable<
+    Parameters<
+      MatrixRTCSessionEventHandlerMap[MatrixRTCSessionEvent.DidSendCallNotification]
+    >
+  >;
   /**
    * Whenever the RTC session tells us that it intends to ring the remote
    * participant's devices, this emits an Observable tracking the current state of
    * that ringing process.
    */
-  private readonly ring$: Observable<
-    Observable<"ringing" | "timeout" | "decline">
-  > = (
-    fromEvent(
-      this.matrixRTCSession,
-      MatrixRTCSessionEvent.DidSendCallNotification,
-    ) as Observable<
-      Parameters<
-        MatrixRTCSessionEventHandlerMap[MatrixRTCSessionEvent.DidSendCallNotification]
-      >
-    >
-  ).pipe(
-    filter(
-      ([notificationEvent]) => notificationEvent.notification_type === "ring",
-    ),
-    map(([notificationEvent]) => {
-      const lifetimeMs = notificationEvent?.lifetime ?? 0;
-      return concat(
-        lifetimeMs === 0
-          ? // If no lifetime, skip the ring state
-            EMPTY
-          : // Ring until lifetime ms have passed
-            timer(lifetimeMs).pipe(
-              ignoreElements(),
-              startWith("ringing" as const),
-            ),
-        // The notification lifetime has timed out, meaning ringing has likely
-        // stopped on all receiving clients.
-        of("timeout" as const),
-        NEVER,
-      ).pipe(
-        takeUntil(
-          (
-            fromEvent(this.matrixRoom, RoomEvent.Timeline) as Observable<
-              Parameters<EventTimelineSetHandlerMap[RoomEvent.Timeline]>
-            >
-          ).pipe(
-            filter(
-              ([event]) =>
-                event.getType() === EventType.RTCDecline &&
-                event.getRelation()?.rel_type === "m.reference" &&
-                event.getRelation()?.event_id === notificationEvent.event_id &&
-                event.getSender() !== this.userId,
+  // This is a behavior since we need to store the latest state for when we subscribe to this after `didSendCallNotification$`
+  // has already emitted but we still need the latest observable with a timeout timer that only gets created on after receiving `notificationEvent`.
+  // A behavior will emit the latest observable with the running timer to new subscribers.
+  // see also: callPickupState$ and in particular the line: `return this.ring$.pipe(mergeAll());` here we otherwise might get an EMPTY observable if
+  // `ring$` would not be a behavior.
+  private readonly ring$: Behavior<
+    Observable<"ringing" | "timeout" | "decline"> | Observable<never>
+  > = this.scope.behavior(
+    this.didSendCallNotification$.pipe(
+      filter(
+        ([notificationEvent]) => notificationEvent.notification_type === "ring",
+      ),
+      map(([notificationEvent]) => {
+        const lifetimeMs = notificationEvent?.lifetime ?? 0;
+        return concat(
+          lifetimeMs === 0
+            ? // If no lifetime, skip the ring state
+              EMPTY
+            : // Ring until lifetime ms have passed
+              timer(lifetimeMs).pipe(
+                ignoreElements(),
+                startWith("ringing" as const),
+              ),
+          // The notification lifetime has timed out, meaning ringing has likely
+          // stopped on all receiving clients.
+          of("timeout" as const),
+          NEVER,
+        ).pipe(
+          takeUntil(
+            (
+              fromEvent(this.matrixRoom, RoomEvent.Timeline) as Observable<
+                Parameters<EventTimelineSetHandlerMap[RoomEvent.Timeline]>
+              >
+            ).pipe(
+              filter(
+                ([event]) =>
+                  event.getType() === EventType.RTCDecline &&
+                  event.getRelation()?.rel_type === "m.reference" &&
+                  event.getRelation()?.event_id ===
+                    notificationEvent.event_id &&
+                  event.getSender() !== this.userId,
+              ),
             ),
           ),
-        ),
-        endWith("decline" as const),
-      );
-    }),
+          endWith("decline" as const),
+        );
+      }),
+    ),
+    EMPTY,
   );
 
   /**
@@ -972,6 +980,8 @@ export class CallViewModel extends ViewModel {
               return of("success" as const);
             }
             // Show the ringing state of the most recent ringing attempt.
+            // ring$ is a behavior so it will emit the latest observable which very well might already have a running timer.
+            // this is important in case livekitConnectionState$ after didSendCallNotification$ has already emitted.
             return this.ring$.pipe(switchAll());
           }),
           // The state starts as 'unknown' because we don't know if the RTC