Dispel confusion about whether media is remote or local #3624
Conversation
They had loose types that were allowing them also output local members. They don't do this, it's just misleading.
By tagging participant behaviors with a type (local vs. remote) we can now tell what kind of participant it will be in a completely type-safe manner.
| this.scope, | ||
| this.id, | ||
| this.userId, | ||
| this.participant$ as Behavior<RemoteParticipant | null>, |
There was a problem hiding this comment.
This cast specifically was the problem.
BillCarsonFr
left a comment
BillCarsonFr
left a comment
There was a problem hiding this comment.
Thx for breaking down in several commits, and for the explaination.
Good point for the tests and nice to test all the modes
I just have some questions, but nothing blocking the approval
| connectionState$?: Behavior<ConnectionState>; | ||
| /** Optional behavior overriding the computed window size, mainly for testing purposes. */ | ||
| windowSize$?: Behavior<{ width: number; height: number }>; | ||
| /** Optional behavior overriding the MatrixRTC mode, mainly for testing purposes. */ |
There was a problem hiding this comment.
nit: I think it would be fine to make it mandatory
| ); | ||
| const localMatrixLivekitMember$ = scope.behavior<MatrixLivekitMember | null>( | ||
| localRtcMembership$.pipe( | ||
| generateItems( |
There was a problem hiding this comment.
so this is a collection of always one thing? there is only one possible key?
There was a problem hiding this comment.
I am struggling a bit why we need generateItems here. Or more precisely what part of generateItems do we need?
Since we are not using the scope we really just use it to return null if we dont have a membership.
My bias would be to use the least abstract functions of writing that code to not hide what its actual purpose is.
Right now I am not sure if there is sth else generateItems is doing that is super important but I dont see it.
BillCarsonFr
left a comment
BillCarsonFr
left a comment
There was a problem hiding this comment.
Wait, I need another look at that. I just looked at a PR based on this and maybe I got it wrong
3 participants
The UserMedia class contained some unsound casts of the participant behaviors. If for any reason your local MatrixRTC membership already existed while joining a call, we would create a UserMedia item for said membership, but the LiveKit participant would start out as
null, and so UserMedia would mistakenly assume that the media item is for a remote participant.This confusion is immediately apparent if we run the CallViewModel tests in Matrix 2.0 mode; they fail due to broken layouts. So, I propose we run these tests against all possible MatrixRTC modes.
The fix is to add some more explicit type hints to the data (
type: "local" | "remote") so we no longer need to guess at what anullparticipant means. This makes the tests pass again.You can review this commit-by-commit :)
Closes #3609