Skip to content

Refactor ring$ observable#3504

Merged
toger5 merged 3 commits intolivekitfrom
toger5/refactor-ringing-call-view-model
Sep 19, 2025
Merged

Refactor ring$ observable#3504
toger5 merged 3 commits intolivekitfrom
toger5/refactor-ringing-call-view-model

Conversation

@toger5
Copy link
Copy Markdown
Contributor

@toger5 toger5 commented Sep 19, 2025

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

@toger5 toger5 requested a review from a team as a code owner September 19, 2025 14:08
@toger5 toger5 requested review from robintown and removed request for a team September 19, 2025 14:08
@toger5 toger5 force-pushed the toger5/refactor-ringing-call-view-model branch from fc8f6be to 1c95dd1 Compare September 19, 2025 14:09
@toger5 toger5 changed the base branch from toger5/beackport-fix-never-stop-ringing to toger5/fix-ring-never-stops September 19, 2025 14:10
@toger5 toger5 changed the base branch from toger5/fix-ring-never-stops to livekit September 19, 2025 14:43
Signed-off-by: Timo K <toger5@hotmail.de>
Signed-off-by: Timo K <toger5@hotmail.de>
Signed-off-by: Timo K <toger5@hotmail.de>
});

expectObservable(vm.callPickupState$).toBe("a 9ms b 29ms c", {
expectObservable(vm.callPickupState$).toBe("a 9ms b 19ms c", {
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was actually wrong before the refactor.

Before the timer was only ever started once connected. But it might have sent the notification event a couple of seconds before (if the connection to the sfu is very poor)
The test has passed previously because we only were running ring$.pipe(...) once we were connected. Now we subscribe to ring$ right in the combineLatest so we start the timer instantly.
Basically even though the observable with the timer was already created it was only subscribed later. (the inner observable at least. the other one was subscribed by the behavior scope immediately that is why the didSendCallNotification$ was still caught.

I hope this makes sense. The more direct justification for this change is:

 // 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,
              );
            },
          });

Says we fire the event immediately and it rings for 30ms -> it should stop after 30ms -> c=timeout should happen after 30ms
before: "a 9ms b 29ms c" 1ms+9ms+1ms+29ms = 40ms is just wrong
now: "a 9ms b 19ms c" 1ms+9ms+1ms+19ms = 30ms is what we would expect with a 30ms notificaiton event

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That makes sense to me 👍 , good catch.

@fkwp fkwp self-requested a review September 19, 2025 15:43
@toger5 toger5 merged commit 9b9c08e into livekit Sep 19, 2025
19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants