Fix reactivity based on localTransport updates (Cleanup for LocalMembership.ts) #3591
Conversation
toger5
force-pushed
the
toger5/fix-local-transport-update
branch
from
6ab0ecd to
d22d746
Compare
toger5
force-pushed
the
toger5/fix-local-transport-update
branch
from
c16c22d to
46f8fe4
Compare
BillCarsonFr
left a comment
BillCarsonFr
left a comment
There was a problem hiding this comment.
Great work thanks. The quality gate is 🤩
We did some live pair session on it already.
I'd like just that we add more debug logs on the LocalMembership/publisher. things about the lifecycle:
start tracks requested
Creating traks
Request disconnect
Looks like we only have logging on error. I'd like that we add a lot more logs, and eventually revert later if too verbose or impacting perfs
| Connecting = "connecting", | ||
| Connected = "connected", | ||
| Error = "error", | ||
| /** Not even a transport is available to the LocalMembership */ |
There was a problem hiding this comment.
nit: maybe we could name this RTCBackendState? rather than LivekitState? Because it could be confused to the actual Livekit Room connection state?
It would also better match the name we use in the EC readme main schema?
| matrixRTCSession, | ||
| }: Props): { | ||
| requestConnect: () => LocalMemberConnectionState; | ||
| requestConnect: () => void; |
There was a problem hiding this comment.
nit: Maybe add some quick comments on what all these does?
| : of(false), | ||
| ), | ||
| localConnectionState$.pipe( | ||
| map((state) => (state ? state.state === "ConnectedToLkRoom" : false)), |
There was a problem hiding this comment.
When state.state === "ConnectedToLkRoom" should we then look at the inner livekitConnectionState? to look for Disconnected / SignalReconnecting?
| {
state: "ConnectedToLkRoom";
livekitConnectionState$: Observable<LivekitConenctionState>;
}| ); | ||
| const publishing$ = scope.behavior( | ||
| publisher$.pipe( | ||
| switchMap((p) => (p?.publishing$ ? p.publishing$ : constant(false))), |
There was a problem hiding this comment.
nit: I am not a fan of the condensed/ternary notation. Can I at least get a comment of what is doing and why?
|
|
||
| // While this loop runs it will process the latest from `value$` until it caught up with the updates. | ||
| // It might skip updates from `value$` and only process the newest value after callback has resolved. | ||
| const reconcileLoop = async (): Promise<void> => { |
There was a problem hiding this comment.
we might want to merge develop in to have the reconcile test #3597
This PR refactors LocalMembership.ts to use reconcile for cleaning up tracks. It also enables track and publisher creation to be reactive to the local transport and the connection.