From b2eb7f11204c58befb42c90a8b44542bd31ed6a5 Mon Sep 17 00:00:00 2001
From: Lance Chant <13349722+lancechant@users.noreply.github.com>
Date: Thu, 25 Jun 2026 09:06:06 +0200
Subject: [PATCH] addressing coderabbit comments

Signed-off-by: Lance Chant <13349722+lancechant@users.noreply.github.com>
---
 .../expo/modules/mpvplayer/MpvPlayerView.kt   | 26 +++++++++++++++
 modules/mpv-player/ios/MpvPlayerView.swift    | 32 +++++++++++++++++++
 2 files changed, 58 insertions(+)

diff --git a/modules/mpv-player/android/src/main/java/expo/modules/mpvplayer/MpvPlayerView.kt b/modules/mpv-player/android/src/main/java/expo/modules/mpvplayer/MpvPlayerView.kt
index 0b4d5204..7cb322d9 100644
--- a/modules/mpv-player/android/src/main/java/expo/modules/mpvplayer/MpvPlayerView.kt
+++ b/modules/mpv-player/android/src/main/java/expo/modules/mpvplayer/MpvPlayerView.kt
@@ -155,6 +155,10 @@ class MpvPlayerView(context: Context, appContext: AppContext) : ExpoView(context
         surfaceReady = true
 
         if (rendererStarted) {
+            // Release the previous wrapper Surface before losing the only
+            // reference to it. cleanup() only runs on detach, so without this
+            // repeated PiP/background/resize cycles leak native surface objects.
+            activeSurface?.release()
             activeSurface = surface
             renderer?.attachSurface(surface)
         } else {
@@ -268,6 +272,28 @@ class MpvPlayerView(context: Context, appContext: AppContext) : ExpoView(context
      */
     fun destroy() {
         renderer?.stop()
+
+        // Reset view-level state so a subsequent loadVideo() on the SAME view
+        // instance re-creates the mpv handle and re-attaches the still-live
+        // TextureView surface. Without this, rendererStarted stays true and
+        // ensureRendererStarted() early-returns, so renderer.start() is never
+        // called again — but stop() already nulled the renderer's mpv handle.
+        // The next loadVideo() then runs loadVideoInternal() -> renderer.load()
+        // against mpv == null, where every mpv?.command() (including the
+        // "stop" and load commands) silently no-ops, leaving a black frame.
+        //
+        // This path is hit by direct-player.tsx's goToNextItem()/stop(),
+        // which call destroy() immediately before router.replace() to the
+        // same route — Expo Router reuses the same MpvPlayerView instance,
+        // so the next source load happens on this view without a remount.
+        rendererStarted = false
+        currentUrl = null
+        // Move the active surface back to pending so ensureRendererStarted()
+        // re-attaches it to the freshly created mpv instance on next load.
+        // The Surface itself is still valid — onSurfaceTextureDestroyed has
+        // not fired because the TextureView is not being unmounted.
+        activeSurface?.let { pendingSurface = it }
+        activeSurface = null
     }
 
     fun seekTo(position: Double) {
diff --git a/modules/mpv-player/ios/MpvPlayerView.swift b/modules/mpv-player/ios/MpvPlayerView.swift
index 743afbd4..9a4e9612 100644
--- a/modules/mpv-player/ios/MpvPlayerView.swift
+++ b/modules/mpv-player/ios/MpvPlayerView.swift
@@ -298,6 +298,38 @@ class MpvPlayerView: ExpoView {
 	 */
 	func destroy() {
 		renderer?.stop()
+
+		// Reset view state and re-create the mpv handle so a subsequent
+		// loadVideo() on the SAME view instance can actually load.
+		// Without this, stop() leaves renderer.mpv == nil, and the next
+		// loadVideo(config:) calls renderer.load() which early-returns
+		// at `guard let handle = self.mpv else { return }` — but only
+		// after flipping isLoading = true and dispatching the loading
+		// delegate callback, so the JS layer is stuck in a perpetual
+		// "loading" state with no actual playback.
+		//
+		// This path is hit by direct-player.tsx's goToNextItem()/stop(),
+		// which call destroy() immediately before router.replace() to
+		// the same route — Expo Router reuses the same MpvPlayerView
+		// instance, so the next `source` prop update arrives on this
+		// view without a remount. setupView() is otherwise the only
+		// place start() is called, so without re-starting here the
+		// renderer stays dead until the whole view is unmounted and
+		// recreated.
+		//
+		// start() is idempotent (`guard !isRunning else { return }`)
+		// and stop() has already nulled mpv synchronously before
+		// dispatching the async mpv_terminate_destroy, so creating a
+		// fresh handle here is safe even while the old handle's
+		// teardown is still in flight on a background queue (libmpv
+		// handles are independent).
+		currentURL = nil
+		intendedPlayState = false
+		do {
+			try renderer?.start()
+		} catch {
+			onError(["error": "Failed to restart renderer after destroy: \(error.localizedDescription)"])
+		}
 	}
 
 	func seekTo(position: Double) {