refactor(ui-voip): own the media session lifecycle from an effect - #41691
refactor(ui-voip): own the media session lifecycle from an effect#41691ggazzo wants to merge 3 commits into
Conversation
The session was exposed through useSyncExternalStore, which meant the store had to be fed by three separate effects (send signal fn, webrtc processor factory, instance creation) before it could hand out an instance, and any ICE setting change replaced the factory. Now the hook creates the session in an effect and keeps it in state: - iceServers/iceGatheringTimeout are read through a stable getter on every processor creation, so setting changes reach new calls without recreating the session; - getInstance is idempotent per userId, so a re-render (StrictMode, discarded render) no longer ends a live session; - getOldSessionId ignores an id this tab created itself, so a recreated instance doesn't try to resume the session it just replaced; - the store no longer emits 'change': with the instance in React state there were no subscribers left.
|
Looks like this PR is not ready to merge, because of the following issues:
Please fix the issues and try again If you have any trouble, please check the PR guidelines |
|
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #41691 +/- ##
===========================================
+ Coverage 68.61% 68.63% +0.02%
===========================================
Files 4162 4162
Lines 158824 158824
Branches 28174 28156 -18
===========================================
+ Hits 108976 109012 +36
+ Misses 44665 44631 -34
+ Partials 5183 5181 -2
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
1 participant
Proposed changes (including videos or screenshots)
useMediaSessionInstanceexposed the session throughuseSyncExternalStore, soMediaSessionStorecould only build an instance after three separate effects had fed it (setSendSignalFn,setWebRTCProcessorFactory, then instance creation on the next snapshot). Every change toVoIP_TeamCollab_Ice_Gathering_Timeoutor to the ICE server list replaced the processor factory, and instance creation was driven by an impuregetSnapshot.The hook now creates the session in an effect and keeps it in state. Concretely:
makeInstancetakes agetWebRTCConfig()getter (auseStableCallback, stable identity + latest values) which the webrtc processor factory calls on every processor creation. New calls pick up new settings; the live session is left alone.getInstanceis idempotent peruserId. Creating the session is a side effect; without a guard, a re-render (StrictMode double render, a discarded render) calledendSession()on the live session — which ignores every known call — and built a new one.getOldSessionIdignores ids this tab created. The session id is written tosessionStorageon creation, so a recreated instance read back the id of the instance it had just replaced and asked the server to resume it. Session recovery after a real page refresh still works, sincelastSessionIdis in-memory only.MediaSignalingSessionconstructor already sends the register signal, sosendSignalFnhas to be in place first — that ordering is now explicit instead of depending on effect order.notify-user/<uid>/media-signalis subscribed with the instance in the dependency list and delivers toinstance.processSignal, which removes the store-leveluserIdcomparison that existed only because the store was a singleton.changeevent, sochange()/onChange()/ thechangeentry in the event map are gone, along withsetSendSignalFn(its returned unsubscribe had no callers) and three unused imports.No behavior change intended other than the session no longer being recreated.
Issue(s)
Steps to test or reproduce
sessionIdinsessionStorage).VoIP_TeamCollab_Ice_Gathering_Timeoutin admin: the ongoing call is unaffected; the next call uses the new value.sessionId.Further comments
The remaining rough edge is that
MediaSignalingSessiondoes its side effects in the constructor (sendsregister, starts the state report interval). Splitting that intostart()/stop()in@rocket.chat/media-signalingwould make construction pure and let the effect own the lifecycle outright, removing the need for the idempotency guard here. Left out of this PR to keep it scoped to the hook.