Dance AI: stop playback when opening modal

[sanchitmalhotra126]

A couple of interrelated changes for bugs we've seen when opening the AI modal mid-playback. We decided for performance and usage reason that it would be best to just stop playback when the modal opens, so some of these changes aren't strictly necessary, but they do help alleviate possible bugs in case they surface in other ways, or if we do decide to change our approach and keep the song playing while the modal is open. These changes include

• In ProgramExecutor, instead of stopping all audio on reset (which is what Dance.js does), we only try to stop the song that had been previously started by the executor specifically, if any. This was causing an issue because Dance.js has logic to switch the run/reset buttons when playback stops that we were unintentionally hitting while calling reset() within the ProgramExecutor while previewing in the modal.
• Wrap callbacks to AiBlockPreview in useCallback to prevent duplicate sets of blocks from being rendered. We'd occasionally see this because the useEffect hook would fire multiple times due to the callback dependencies not being memoized.
• Also call workspace.dispose() on the block preview workspace whenever dismounting to be safe.
• I also removed the done state in AiBlockPreview since it seemed like wasn't being used to trigger any rendering

Links

https://trello.com/c/Mau4dYYV

Testing story

Tested locally on Dance AI levels

[breville]

Great changes!

[bencodeorg]

Thanks for doing this! I was looking at this last week and wondering if we should only create the workspace once (maybe in a separate useEffect that only responds to changes in the container ref)?

[sanchitmalhotra126]

Ah good question - though I think this allows us to be more strict about the workspace lifecycle, since we don't want to be rendering additional blocks even if we have a single workspace? This way the workspace is only created once (and shouldn't be recreated unnecessarily due to the useCallback wrapping generateBlocksFromResult) and we can also ensure that we call workspace.dispose() when unmounting, or when dependencies do happen to change.

[bencodeorg]

Ok that makes sense -- I guess I was wondering what happens if this were to get called multiple times (acknowledging with the changes you've made to generateBlocksFromResult that it shouldn't), like what happens if a second workspace is generated within the same ref? Anyway probably doesn't matter

[sanchitmalhotra126]

Yep yep I think still in that case we'd be okay since we clean up with workspace.dispose() - so if the useEffect ever runs multiple times, we'll call dispose() before each run which should remove any existing blocks before rendering the new workspace

[bencodeorg]

Oh snap, I think I misunderstood react docs -- the cleanup callback from useEffect runs not only on unmount but before the subsequent render. Nice!

[fisher-alice]

Thanks for the discussion and shared reference! TIL about cleanup functions in useEffect. 😃