Refactor livekit disconnect to use an effect hook. #1925
Conversation
Signed-off-by: Timo K <toger5@hotmail.de>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## livekit #1925 +/- ##
========================================
Coverage 24.67% 24.67%
========================================
Files 48 48
Lines 2383 2383
Branches 438 438
========================================
Hits 588 588
Misses 1744 1744
Partials 51 51
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Signed-off-by: Timo K <toger5@hotmail.de>
src/room/InCallView.tsx
Outdated
| const cleanup = useCallback(() => { | ||
| if (connState === ConnectionState.Connected) { | ||
| livekitRoom?.disconnect(); | ||
| } | ||
| }, [livekitRoom, connState]); | ||
|
|
||
| useEffect(() => { | ||
| return cleanup; | ||
| }, [cleanup, livekitRoom]); |
There was a problem hiding this comment.
Note that:
- This clean-up function may be called multiple times.
- The connection state could also be
ReconnectingorConnectingin which case thedisconnect()will not be called (meaning that we will connect to the room despite unmounting theActiveCallleading to an unexpected bug for this edge-case).
I think the cleanest way to fix the problem is to do a clean effect that just disconnects the room on unmount without any further actions and checks (I don't think we need connState here), similarly to how LiveKit does it. I checked the LiveKit Meet example and how they do it. They use the LiveKitRoom component that relies on LkRoomContext. Here is how the clean-up is done: link which is just a typical React idiom for these sort of cases. Are there any strong reasons why we don't want to do it this way? 🙂
There was a problem hiding this comment.
I had this initially. But was seeing it beeing called in the beginning, thats why I added: if (connState === ConnectionState.Connected) {. But that was because i let copilot + tooling add the dependencies...
There was a problem hiding this comment.
I now use exactly what you propose.
Signed-off-by: Timo K <toger5@hotmail.de>
(we had an error because onLeave gets called with the button) Signed-off-by: Timo K <toger5@hotmail.de>
Signed-off-by: Timo K <toger5@hotmail.de>
No description provided.