Connection State: Error Handling + Cleaning / refactoring / #3521

BillCarsonFr · 2025-10-01T12:23:23Z

Better reviewed commit per commit

…ction_states

Updated the test because now name will be the userId instead of default display name

robintown

Very nice work with fixing things up as you go. For the sake of getting our two branches merged into one as soon as possible, I went ahead and applied some changes directly for things which seemed hopefully uncontrovertial - if you like you can have a look through them. The rest of my comments are mostly about things from my branch that seem to have been lost in the previous merge conflicts.

I did also confirm that the app runs and connects to an SFU successfully.

robintown · 2025-10-08T20:30:45Z

src/livekit/MatrixAudioRenderer.tsx

   * This list needs to be composed based on the matrixRTC members so that we do not play audio from users
   * that are not expected to be in the rtc session.
   */
+  // TODO: Why do we have this structure? looks like we only need the valid/active participants (not the room member or id)?


Just laziness, really. The CallViewModel needs the ID and room member internally to create the media tiles, so I took the same data and made it public for the purpose of audio rendering without making the public type any stricter. I've remedied this in: c96e81b

robintown · 2025-10-08T20:33:10Z

src/main.tsx

 // dependency references.
 import "matrix-js-sdk/lib/browser-index";

-import { StrictMode } from "react";


Oops, I didn't mean to commit my temporary disabling of strict mode. e346c8c

robintown · 2025-10-08T20:44:16Z

src/state/MuteStates.ts

  ) {}
 }

+// TODO there is another MuteStates in src/room/MuteStates.tsx ?? why


Dead code, we can remove it. 5da780e

robintown · 2025-10-08T21:07:50Z

src/state/Async.ts

+import { catchError, from, map, type Observable, of, startWith } from "rxjs";

+// TODO where are all the comments? ::cry::
+// There used to be an unitialized state!, a state might not start in loading


IMO the "uninitialized" state doesn't belong here. In cases where it might be tempting to use "uninitialized", I'd like it if we explicitly add such a case into our types as needed (Async<T> → Async<T | undefined>) or keep an an eye out for better ways to model the problem. With just these three states, there's a very clear mapping between async values and Promises.

I added hopefully a decent amount of comments: b1d1437

robintown · 2025-10-08T21:29:51Z

src/state/Connection.ts

+  /**
+   * The focus server to connect to.
+   */
+  public readonly localTransport: LivekitTransport;


Why is this called localTransport and not transport? It's often remote.

robintown · 2025-10-08T22:32:08Z

src/state/Connection.ts

            // Pair with their associated LiveKit participant (if any)
-            .map((membership) => {
+            // Uses flatMap to filter out memberships with no associated rtc participant ([])
+            .flatMap((membership) => {
              const id = `${membership.sender}:${membership.deviceId}`;
              const participant = participants.find((p) => p.identity === id);
-              return { participant, membership };
+              return participant ? [{ participant, membership }] : [];
            }),


I think something went wrong here with the merge. If you take a look at 1820cac I was trying to make sure that even RTC memberships with no associated LiveKit participant are included in publishingParticipants$. (One could add them back in CallViewModel, but it seemed way easier and more efficient to just avoid filtering them out here.)

The type that I was aiming for was:

Connection.publishingParticipants$: Behavior<{ participant: RemoteParticipant | undefined; membership: CallMembership; }[]>

Once we get the CallViewModel tests back up and running we'll be back to a state where this can't regress without a red flag.

robintown · 2025-10-08T22:36:45Z

src/state/Connection.ts

+    scope
+      .behavior<ConnectionState>(connectionStateObserver(this.livekitRoom))
+      .subscribe((connectionState) => {
+        const current = this._focusedConnectionState$.value;
+        // Only update the state if we are already connected to the LiveKit room.
+        if (current.state === "ConnectedToLkRoom") {
+          this._focusedConnectionState$.next({
+            state: "ConnectedToLkRoom",
+            connectionState,
+            focus: current.focus,
+          });


Perhaps a more elegant way to do this would be to change the type to:

{ state: "ConnectedToLkRoom"; - connectionState: ConnectionState; + connectionState: Observable<ConnectionState>; focus: LivekitTransport; }

Then (on line 111) you can just write:

this._focusedConnectionState$.next({ state: "ConnectedToLkRoom", focus: this.localTransport, connectionState: connectionStateObserver(this.livekitRoom), });

and remove this line 212 code.

robintown · 2025-10-08T22:38:51Z

src/state/PublishConnection.ts

+    // Setup track processor syncing (blur)
+    this.observeTrackProcessors(scope, room, trackerProcessorState$);
+    // Observe mute state changes and update LiveKit microphone/camera states accordingly
+    this.observeMuteStates(scope);
+    // Observe media device changes and update LiveKit active devices accordingly
+    this.observeMediaDevices(scope, devices, controlledAudioDevices);
+
+    this.workaroundRestartAudioInputTrackChrome(devices, scope);


Nice, this cleaner constructor is also a welcome change

robintown · 2025-10-08T22:43:55Z

src/state/PublishConnection.ts

+    scope.onEnd(() => {
+      this.muteStates.audio.unsetHandler();
+      this.muteStates.video.unsetHandler();
+    });


In 86fb026 I moved these calls to unsetHandler to the stop method as scope.onEnd happens much later than what we need for SFU-hopping. I believe they should stay there in stop.

Actually, scratch that, we should just give the Connection class a proper ObservableScope of its own that ends at the right time (when CallViewModel is about to drop the Connection object).

I can work on that.

robintown · 2025-10-08T22:49:12Z

src/state/PublishConnection.ts

+    // TODO this can throw errors? It will also prompt for permissions if not already granted
+    const tracks = await this.livekitRoom.localParticipant.createTracks({
+      audio: this.muteStates.audio.enabled$.value,
+      video: this.muteStates.video.enabled$.value,
+    });
+    if (this.stopped) return;
+    for (const track of tracks) {
+      // TODO: handle errors? Needs the signaling connection to be up, but it has some retries internally
+      // with a timeout.
+      await this.livekitRoom.localParticipant.publishTrack(track);
+      if (this.stopped) return;
+      // TODO: check if the connection is still active? and break the loop if not?
+    }


We should restore this diff from 86fb026:

@@ -130,23 +131,35 @@ export class PublishConnection extends Connection { if (!this.stopped) await this.livekitRoom.connect(url, jwt); if (!this.stopped) { - const tracks = await this.livekitRoom.localParticipant.createTracks({ - audio: this.muteStates.audio.enabled$.value, - video: this.muteStates.video.enabled$.value, - }); - for (const track of tracks) { - await this.livekitRoom.localParticipant.publishTrack(track); + // TODO-MULTI-SFU: Prepublish a microphone track + const audio = this.muteStates.audio.enabled$.value; + const video = this.muteStates.video.enabled$.value; + // createTracks throws if called with audio=false and video=false + if (audio || video) { + const tracks = await this.livekitRoom.localParticipant.createTracks({ + audio, + video, + }); + for (const track of tracks) { + await this.livekitRoom.localParticipant.publishTrack(track); + } } } }

(by which I mean just the TODO and the audio || video guard.)

useCallViewKeyboardShortcuts() changed a param from `setMicrophoneMuted` to `setAudioEnabled`, the boolean arg of the callback is inverse tht it used to be

Signed-off-by: Timo K <[email protected]>

…ction_states

BillCarsonFr added 17 commits September 30, 2025 11:33

refactoring: prep work extract to file + documentation

edf68d1

refactor: Remote / Publish Connection and constructor

b00f7d5

Connection: add Connection state and handle error on start

879a1d4

Connection states tests

3d8639d

lint fixes

47c876f

extract common test setup

2290016

ConnectionState: test livekit connection states

6a1f7dd

tests: end scope tests

e8bf817

fix lint errors

dfaa6a3

tests: Add publisher observable tests

0502f66

test: Ensure scope for publishers observer

84f95be

refactor: PublishConnection extract from giant constructor

00401ca

tests: Publish connection states

91a366f

Merge branch 'voip-team/rebased-multiSFU' into valere/multi-sfu/conne…

597e678

…ction_states

Lint: fix all the lint errors

c3c0516

build: update lock file

c820ba3

lint: fix import order

7437961

github-actions bot deployed to Netlify October 7, 2025 14:15 View deployment

prettier !

529cb8a

github-actions bot deployed to Netlify October 7, 2025 14:25 View deployment

knip: remove dead code

18ba02c

github-actions bot deployed to Netlify October 7, 2025 14:30 View deployment

fixup MediaView tests

05e7b5a

github-actions bot deployed to Netlify October 7, 2025 15:40 View deployment

test: Fix mediaView test, ,member is not optional anymore

13fb466

Updated the test because now name will be the userId instead of default display name

github-actions bot deployed to Netlify October 8, 2025 06:52 View deployment

BillCarsonFr added 3 commits October 8, 2025 14:29

esLint fix

f5ea734

Remove un-necessary transport field, already accessible from connection

afe004c

test: Fix Audio render tests and added more

427a8dd

github-actions bot deployed to Netlify October 8, 2025 12:50 View deployment

robintown added 8 commits October 8, 2025 17:34

Fix doc comment typo

8778be8

Restore a hidden 'null' state for the local transport/connection

3691e71

Remove unused useIsEarpiece hook

dee06a4

Remove MockedObject from mockMediaDevices type signature

dcc3ab6

Remove local participant case (now enforced by types) from audio tests

00daf83

Fix focus connection state typo, simplify its initialization

5be3b91

Update outdated comment

64c2e59

Clean up subscriptions in Connection tests

2c576a7

robintown reviewed Oct 8, 2025

View reviewed changes

github-actions bot deployed to Netlify October 8, 2025 23:20 View deployment

Remove outdated comment

85ffe68

github-actions bot deployed to Netlify October 8, 2025 23:21 View deployment

fix: use correct TestEachFunction

4c6b960

github-actions bot deployed to Netlify October 9, 2025 09:02 View deployment

BillCarsonFr and others added 8 commits October 9, 2025 15:33

Simplify AudioRenderer and add more tests

7cbb1ec

test: Fix mute test, behavior change from setMuted to setAudioEnabled

a500915

useCallViewKeyboardShortcuts() changed a param from `setMicrophoneMuted` to `setAudioEnabled`, the boolean arg of the callback is inverse tht it used to be

Test: Fix mocking to fix failing tests

6710f4c

test: MISSING_MATRIX_RTC_FOCUS renamed as MISSING_MATRIX_RTC_TRANSPORT

1ab081d

Merge branch 'livekit' into voip-team/rebased-multiSFU

0fd4143

Signed-off-by: Timo K <[email protected]>

Merge branch 'voip-team/rebased-multiSFU' into valere/multi-sfu/conne…

4608d68

…ction_states

post merge lint fixes

4a8f5bc

test: fix additional test with proper mutestate fix

1e75f9a

robintown force-pushed the voip-team/rebased-multiSFU branch from 0fd4143 to de5f519 Compare October 10, 2025 13:52

BillCarsonFr added 2 commits October 10, 2025 16:51

Merge branch 'voip-team/rebased-multiSFU' into valere/multi-sfu/conne…

cca46bd

…ction_states

prettier fix

2fc7f11

BillCarsonFr marked this pull request as ready for review October 10, 2025 14:53

BillCarsonFr requested a review from a team as a code owner October 10, 2025 14:53

BillCarsonFr requested review from Half-Shot and removed request for a team October 10, 2025 14:53

post merge fixes (js-sdk changes)

fc2384e

Connection State: Error Handling + Cleaning / refactoring / #3521

Are you sure you want to change the base?

Connection State: Error Handling + Cleaning / refactoring / #3521

Conversation

BillCarsonFr commented Oct 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

robintown left a comment

Choose a reason for hiding this comment

Uh oh!

robintown Oct 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

robintown Oct 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

robintown Oct 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

robintown Oct 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

robintown Oct 8, 2025

Choose a reason for hiding this comment

Uh oh!

robintown Oct 8, 2025

Choose a reason for hiding this comment

Uh oh!

robintown Oct 8, 2025

Choose a reason for hiding this comment

Uh oh!

robintown Oct 8, 2025

Choose a reason for hiding this comment

Uh oh!

robintown Oct 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

robintown Oct 9, 2025

Choose a reason for hiding this comment

Uh oh!

robintown Oct 9, 2025

Choose a reason for hiding this comment

Uh oh!

robintown Oct 8, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

BillCarsonFr commented Oct 1, 2025 •

edited

Loading

robintown Oct 8, 2025 •

edited

Loading

robintown Oct 8, 2025 •

edited

Loading

robintown Oct 8, 2025 •

edited

Loading

robintown Oct 8, 2025 •

edited

Loading

robintown Oct 8, 2025 •

edited

Loading