expo-av to expo-video and expo-audio

[YoussefHenna]

No description provided.

[linear]

V2-2184

[gemini-code-assist]

Code Review

This pull request migrates the media playback components from expo-av to the newer expo-audio and expo-video libraries. Key changes include updating HeadlessAudioPlayer and VideoPlayer to use the new hooks and views, adapting MediaPlaybackWrapper to wrap the new player types, and updating Jest configurations and dependency files. The review feedback highlights several critical issues: stale closures in HeadlessAudioPlayer and VideoPlayer due to empty dependency arrays in event listeners, a race condition introduced by making normalizeBase64Source synchronous, and a missing player dependency in the togglePlayback callback within MediaPlaybackWrapper.

[gemini-code-assist]

The useEffect hook that registers the playbackStatusUpdate listener has an empty dependency array ([]). However, the listener closure references several dynamic values and props: player, onPlaybackStatusUpdateProp, isLooping, and onPlaybackFinish.

Because of the empty dependency array, these values will be captured as stale closures from the initial render, meaning any updates to isLooping or changes to the callback props will not be reflected in the listener.

To fix this, add these variables to the dependency array so the listener is correctly re-registered when they change.

Suggested change

	React.useEffect(() => {
	const subscription = player.addListener(
	"playbackStatusUpdate",
	(status) => {
	const mappedStatus = mapToMediaPlayerStatus(status);
	onPlaybackStatusUpdateProp?.(mappedStatus);
	if (status.isLoaded) {
	if (status.didJustFinish && !isLooping) {
	onPlaybackFinish?.();
	}
	setIsPlaying(status.playing);
	}
	}
	);
	return () => subscription.remove();
	}, []);
	React.useEffect(() => {
	const subscription = player.addListener(
	"playbackStatusUpdate",
	(status) => {
	const mappedStatus = mapToMediaPlayerStatus(status);
	onPlaybackStatusUpdateProp?.(mappedStatus);
	if (status.isLoaded) {
	if (status.didJustFinish && !isLooping) {
	onPlaybackFinish?.();
	}
	setIsPlaying(status.playing);
	}
	}
	);
	return () => subscription.remove();
	}, [player, onPlaybackStatusUpdateProp, isLooping, onPlaybackFinish]);

[gemini-code-assist]

The event listeners registered in the useEffect hook capture onPlaybackStatusUpdateProp and onPlaybackFinish from the initial render. Since the dependency array is empty ([]), any updates to these callback props from the parent component will not be reflected, leading to stale closures and potential bugs.

Since you are already using the ref pattern for shouldPlay and positionMillis to avoid re-registering listeners, you should extend this pattern to onPlaybackStatusUpdateProp and onPlaybackFinish as well.

    // Refs so statusChange can read latest shouldPlay/positionMillis/callbacks
    const shouldPlayRef = React.useRef(shouldPlay);
    const positionMillisRef = React.useRef(positionMillis);
    const onPlaybackStatusUpdateRef = React.useRef(onPlaybackStatusUpdateProp);
    const onPlaybackFinishRef = React.useRef(onPlaybackFinish);
    shouldPlayRef.current = shouldPlay;
    positionMillisRef.current = positionMillis;
    onPlaybackStatusUpdateRef.current = onPlaybackStatusUpdateProp;
    onPlaybackFinishRef.current = onPlaybackFinish;

    const hasAppliedInitialState = React.useRef(false);

    React.useEffect(() => {
      const timeUpdateSub = player.addListener("timeUpdate", (status) => {
        onPlaybackStatusUpdateRef.current?.(mapToMediaPlayerStatus(status, player));
      });

      const playingChangeSub = player.addListener(
        "playingChange",
        ({ isPlaying: playing }) => {
          setIsPlaying(playing);
          onPlaybackStatusUpdateRef.current?.(mapPlayerToMediaPlayerStatus(player));
        }
      );

      const playToEndSub = player.addListener("playToEnd", () => {
        onPlaybackFinishRef.current?.();
      });

      const statusChangeSub = player.addListener(
        "statusChange",
        ({ status, error }) => {
          if (status === "readyToPlay") {
            setShowPoster(false);
            if (!hasAppliedInitialState.current) {
              hasAppliedInitialState.current = true;
              if (positionMillisRef.current) {
                player.currentTime = positionMillisRef.current / 1000;
              }
              if (shouldPlayRef.current) {
                player.play();
              }
            }
          }
          const mappedStatus = mapPlayerToMediaPlayerStatus(player);
          onPlaybackStatusUpdateRef.current?.(
            status === "error" && error
              ? { ...mappedStatus, isError: true, error: error.message }
              : mappedStatus
          );
        }
      );

      return () => {
        timeUpdateSub.remove();
        playingChangeSub.remove();
        playToEndSub.remove();
        statusChangeSub.remove();
      };
    }, []);

[gemini-code-assist]

Changing normalizeBase64Source from an asynchronous function to a synchronous one introduces a critical correctness issue. expo-file-system operations (such as writing the base64 string to a local file) are inherently asynchronous. Making this function synchronous means any file writing operations inside it will execute asynchronously without being awaited.

This creates a race condition: normalizeBase64Source will immediately return the local file URI before the file has actually been written to disk. When the audio or video player attempts to load and play this URI, it will fail because the file does not exist yet.

To fix this, you should keep normalizeBase64Source asynchronous (returning a Promise) and handle the asynchronous resolution in the player components (e.g., by initializing the player with null or a placeholder, and calling player.replace(resolvedSource) once the promise resolves).

[gemini-code-assist]

The player dependency is missing from the dependency array of the togglePlayback callback. If the player instance changes or is initially undefined and then gets initialized, togglePlayback will use a stale reference. Adding player to the dependency array ensures it always uses the correct instance, consistent with the pause and play callbacks below.

Suggested change

	const togglePlayback = React.useCallback(() => {
	onTogglePlayback?.();
	if (isPlaying) {
	await media?.pauseAsync();
	player?.pause();
	} else {
	await media?.playAsync();
	player?.play();
	}
	}, [media, isPlaying, onTogglePlayback]);
	}, [isPlaying, onTogglePlayback]);
	const togglePlayback = React.useCallback(() => {
	onTogglePlayback?.();
	if (isPlaying) {
	player?.pause();
	} else {
	player?.play();
	}
	}, [player, isPlaying, onTogglePlayback]);

[github-actions]

Published version: @draftbit/ui@54.0.4-8202b6.2