Reimplement WebRTC controls as a hook #1023
Conversation
zarvox
force-pushed
the
zarvox-use-call-state
branch
from
6a0ba27 to
fb9d6c2
Compare
zarvox
force-pushed
the
zarvox-use-call-state
branch
from
fb9d6c2 to
5e5cf59
Compare
| @@ -262,7 +262,6 @@ const AudioConfig = () => { | |||
| <audio | |||
| ref={audioRef} | |||
| autoPlay | |||
| playsInline | |||
There was a problem hiding this comment.
I slipped this (and the matching change in CallSection) into this PR since I saw it's what's causing #1029
to fail presubmit checks at https://github.com/deathandmayhem/jolly-roger/runs/8193203773?check_suite_focus=true and this change rewrites the line in CallSection anyway.
The backstory is kinda fun -- this is a vestige of the distant past from when I was bringing up the mesh WebRTC implementation with video because I didn't yet have anything like a spectrogram and I was sick of hearing feedback when the connection was successfully established. Once I had the implementation working reliably, I switched the video element to an audio one, without realizing this property isn't valid for audio.
| useEffect(() => { | ||
| // If huntId, puzzleId, or tabId change (but mostly puzzleId), reset | ||
| // call state. | ||
| return () => { | ||
| log('huntId/puzzleId/tabId changed, resetting call state'); | ||
| dispatch({ type: 'reset' }); | ||
| }; | ||
| }, [huntId, puzzleId, tabId]); |
There was a problem hiding this comment.
This makes me wonder if we should add a beforeunload handler to prompt people before accidentally switching away from a call.
There was a problem hiding this comment.
I think it's a good idea, but the obvious implementation doesn't appear to do anything because we need to hook into react-router instead, since it's not a browser-level navigation that any of our links trigger.
This was something that was included in react-router v5, but is absent in v6: https://reactrouter.com/en/main/upgrading/v5#prompt-is-not-currently-supported . Given that this looks like it'll be a bit more work to sort out, I filed #1037 for future followup.
imports/client/hooks/useCallState.ts
Outdated
| if (state.audioState?.mediaSource) { | ||
| state.audioState?.mediaSource.getTracks().forEach((track) => { | ||
| track.stop(); | ||
| }); | ||
| } |
There was a problem hiding this comment.
This will fire on the new tracks, right? Do you want to capture a reference to the old mediaSource in the useEffect before the unwind callback?
There was a problem hiding this comment.
Ah, that's probably wise, thanks. In practice, I think the way we've written the rest of our code means mediaSource never transitions from truthy to truthy, so we never hit this edge case, but it's good to be resilient.
imports/client/hooks/useCallState.ts
Outdated
| // force side effects from onProduce | ||
| const [producerParamsGeneration, setProducerParamsGeneration] = useState<number>(0); | ||
| // force side-effects when producer is ready | ||
| const [producerGeneration, setProducerGeneration] = useState<number>(0); |
There was a problem hiding this comment.
I think I understand it after reading the rest of the file, but I think I'd appreciate a comment here about what these generation numbers trigger
There was a problem hiding this comment.
Wrote up a bit more about how the producer flow works, including what fields these generation counters are a proxy for changing, and which effects read those fields (and thus, need to rerun when the generation counter changes).
There was a problem hiding this comment.
Sorry for not getting to this sooner. I'm a little sad that we can't rely on React to manage more of the lifecycle for us, but given that they explicitly seem to not want to do that, this seems like it will be a better long-term structure.
I left a few comments. Feel free to address them (or not, if I misunderstood something). I'll approve now so you don't need to wait for another review from me.
This replaces the hierarchy of components that managed our WebRTC interactions with a new `useCallState` hook which encapsulates all of the call state management functionality. This provides a key benefit: we can hoist the actual state storage higher in the component tree, which means that we won't lose call state when switching layouts between desktop and mobile screen widths. As a bonus, this change also corrects a bunch of things from the audio calls implementation that would have gone awry under `StrictMode` in React 18 due to doing side-effectful actions outside of `useEffect` blocks. Fixes #594.
zarvox
force-pushed
the
zarvox-use-call-state
branch
from
5e5cf59 to
b598a1c
Compare
This replaces the hierarchy of components that managed our WebRTC
interactions with a new
useCallStatehook which encapsulates all ofthe call state management functionality. This provides a key benefit:
we can hoist the actual state storage higher in the component tree,
which means that we won't lose call state when switching layouts between
desktop and mobile screen widths.
As a bonus, this change also corrects a bunch of things from the audio
calls implementation that would have gone awry under
StrictModeinReact 18 due to doing side-effectful actions outside of
useEffectblocks.
Fixes #594.