fix(android): commit the portal fragment only once its container view is attached

.github/workflows/ci.yml

@@ -103,6 +103,44 @@ jobs:
         run: |
           yarn turbo run build:android --cache-dir="${{ env.TURBO_CACHE_DIR }}"
 
+  test-android:
+    runs-on: ubuntu-latest
+    steps:
+      - name: Checkout
+        uses: actions/checkout@v5
+
+      - name: Setup
+        uses: ./.github/actions/setup
+
+      - name: Install JDK
+        uses: actions/setup-java@v4
+        with:
+          distribution: 'zulu'
+          java-version: '17'
+
+      - name: Finalize Android SDK
+        run: |
+          /bin/bash -c "yes | $ANDROID_HOME/cmdline-tools/latest/bin/sdkmanager --licenses > /dev/null"
+
+      - name: Cache Gradle
+        uses: actions/cache@v4
+        with:
+          path: |
+            ~/.gradle/wrapper
+            ~/.gradle/caches
+          key: ${{ runner.os }}-gradle-${{ hashFiles('example/android/gradle/wrapper/gradle-wrapper.properties') }}
+          restore-keys: |
+            ${{ runner.os }}-gradle-
+
+      - name: Run Android unit tests
+        run: |
+          cd example/android
+          ./gradlew :ionic_portals-react-native:testDebugUnitTest --no-daemon --console=plain
+
+      - name: Summarize unit test results
+        if: always()
+        run: grep -h "<testsuite " android/build/test-results/testDebugUnitTest/*.xml
+
   build-ios:
     runs-on: macos-14
     env:

android/build.gradle

@@ -80,6 +80,12 @@ android {
     sourceCompatibility JavaVersion.VERSION_1_8
     targetCompatibility JavaVersion.VERSION_1_8
   }
+
+  testOptions {
+    unitTests {
+      includeAndroidResources = true
+    }
+  }
 }
 
 repositories {
@@ -97,5 +103,8 @@ dependencies {
   implementation "org.jetbrains.kotlin:kotlin-stdlib:$kotlin_version"
   implementation "io.ionic:portals:0.13.0-rn.1"
   implementation "io.ionic:liveupdates:0.5.+"
+
+  testImplementation "junit:junit:4.13.2"
+  testImplementation "org.robolectric:robolectric:4.17"
 }
 

android/src/main/java/io/ionic/portals/reactnative/PortalView.kt

@@ -113,7 +113,6 @@ internal class PortalViewManager(private val context: ReactApplicationContext) :
 
         val portalFragment = PortalFragment(portal)
         viewState.initialContext?.let(portalFragment::setInitialContext)
-        viewState.fragment = portalFragment
 
         portalFragment.lifecycle.addObserver(object : LifecycleEventObserver {
             override fun onStateChanged(source: LifecycleOwner, event: Lifecycle.Event) {
@@ -130,28 +129,44 @@ internal class PortalViewManager(private val context: ReactApplicationContext) :
             }
         })
 
-        val activity = context.currentActivity as? FragmentActivity ?: return
-        activity.supportFragmentManager
-            .beginTransaction()
-            .replace(viewId, portalFragment, "$viewId")
-            .commit()
+        // The fragment transaction must not be committed asynchronously here:
+        // FragmentManager resolves the container view by id when the
+        // transaction executes (next main-loop iteration), and throws
+        // "No view found for id" if the container is not part of the
+        // activity's window at that moment. That happens when React drops the
+        // view before the transaction runs (mount and unmount in one batch),
+        // or when the view exists but is not attached yet / any more (e.g. a
+        // react-native-screens screen still animating in, or detached while
+        // React keeps the subtree mounted).
+        //
+        // Instead, commit synchronously once the view is attached and still ours
+        // (see runWhenAttached for why it is posted on the view).
+        runWhenAttached(parentView, isCurrent = { fragmentMap[viewId] === viewState }) {
+            val activity = context.currentActivity as? FragmentActivity ?: return@runWhenAttached
+            try {
+                activity.supportFragmentManager
+                    .beginTransaction()
+                    .replace(viewId, portalFragment, "$viewId")
+                    .commitNowAllowingStateLoss()
+                viewState.fragment = portalFragment
+            } catch (e: IllegalStateException) {
+                // Host destroyed or FragmentManager unavailable.
+                Log.i("io.ionic.portals.rn", "Fragment manager not available", e)
+            }
+        }
     }
 
     override fun onDropViewInstance(view: FrameLayout) {
         super.onDropViewInstance(view)
-        val viewState = fragmentMap[view.id] ?: return
-
-        try {
-            viewState.fragment
-                ?.parentFragmentManager
-                ?.beginTransaction()
-                ?.remove(viewState.fragment!!)
-                ?.commit()
-        } catch (e: IllegalStateException) {
-            Log.i("io.ionic.portals.rn", "Parent fragment manager not available")
-        }
-
-        fragmentMap.remove(view.id)
+        val viewState = fragmentMap.remove(view.id) ?: return
+        // Only set once the fragment was actually added, so parentFragmentManager
+        // is available here. A drop before the add ran is handled by the
+        // isCurrent check in runWhenAttached.
+        val fragment = viewState.fragment ?: return
+        fragment.parentFragmentManager
+            .beginTransaction()
+            .remove(fragment)
+            .commitAllowingStateLoss()
     }
 
     private fun setupLayout(view: ViewGroup) {

android/src/main/java/io/ionic/portals/reactnative/RunWhenAttached.kt

@@ -0,0 +1,27 @@
+package io.ionic.portals.reactnative
+
+import android.view.View
+
+/**
+ * Runs [action] on the main thread once [view] is attached to a window, provided
+ * [isCurrent] still returns true at that point.
+ *
+ * The runnable is posted on the view: [View.post] queues it until the view is
+ * attached and always dispatches it through the main handler, so [action] never
+ * runs nested inside another FragmentManager transaction that may be executing
+ * further up the stack. If the view is not attached when the runnable runs
+ * (detached again in between), it re-posts itself and waits for the next
+ * attach. If [isCurrent] returns false (the view was dropped), nothing runs.
+ */
+internal fun runWhenAttached(view: View, isCurrent: () -> Boolean, action: () -> Unit) {
+    view.post(object : Runnable {
+        override fun run() {
+            if (!isCurrent()) return
+            if (!view.isAttachedToWindow) {
+                view.post(this)
+                return
+            }
+            action()
+        }
+    })
+}

android/src/test/java/io/ionic/portals/reactnative/RunWhenAttachedTest.kt

@@ -0,0 +1,136 @@
+package io.ionic.portals.reactnative
+
+import android.os.Looper
+import android.view.View
+import android.view.ViewGroup
+import android.widget.FrameLayout
+import androidx.fragment.app.Fragment
+import androidx.fragment.app.FragmentActivity
+import androidx.fragment.app.FragmentManager
+import org.junit.Assert.assertEquals
+import org.junit.Assert.assertNotNull
+import org.junit.Assert.assertNull
+import org.junit.Assert.assertTrue
+import org.junit.Before
+import org.junit.Test
+import org.junit.runner.RunWith
+import org.robolectric.Robolectric
+import org.robolectric.RobolectricTestRunner
+import org.robolectric.Shadows.shadowOf
+import org.robolectric.annotation.Config
+
+/**
+ * Regression tests for the container race behind
+ * `IllegalArgumentException: No view found for id … for fragment PortalFragment`.
+ *
+ * They drive the real androidx [FragmentManager] under Robolectric with plain
+ * [Fragment]s, which is enough: `FragmentStateManager.createView` resolves the
+ * container by id before it asks the fragment for a view. `PortalFragment`
+ * itself is out of scope here, it would boot a Capacitor bridge and a WebView.
+ */
+@RunWith(RobolectricTestRunner::class)
+@Config(sdk = [34])
+class RunWhenAttachedTest {
+    private lateinit var activity: FragmentActivity
+    private lateinit var container: FrameLayout
+
+    private val fragmentManager: FragmentManager
+        get() = activity.supportFragmentManager
+
+    @Before
+    fun setUp() {
+        activity = Robolectric.buildActivity(FragmentActivity::class.java).setup().get()
+        container = FrameLayout(activity).apply { id = View.generateViewId() }
+    }
+
+    private fun attachContainer() = activity.setContentView(container)
+
+    private fun detachContainer() = (container.parent as ViewGroup).removeView(container)
+
+    private fun idleMainLooper() = shadowOf(Looper.getMainLooper()).idle()
+
+    private fun commitFragmentNow() {
+        fragmentManager
+            .beginTransaction()
+            .replace(container.id, Fragment())
+            .commitNowAllowingStateLoss()
+    }
+
+    private fun fragmentInContainer(): Fragment? = fragmentManager.findFragmentById(container.id)
+
+    /**
+     * Control: this is the pattern the view manager used before the fix and the
+     * production crash. It proves the harness reproduces the failure, so the
+     * tests below mean something.
+     */
+    @Test
+    fun asyncCommitThrowsWhenTheContainerWasRemovedBeforeItRan() {
+        attachContainer()
+        fragmentManager.beginTransaction().replace(container.id, Fragment()).commit()
+        detachContainer()
+
+        val thrown = runCatching { idleMainLooper() }.exceptionOrNull()
+
+        assertNotNull("expected the pending add to throw", thrown)
+        val messages = generateSequence(thrown) { it.cause }
+            .mapNotNull { it.message }
+            .joinToString(" | ")
+        assertTrue(messages, messages.contains("No view found for id"))
+    }
+
+    @Test
+    fun commitsOnceTheLooperRunsWhenTheContainerIsAttached() {
+        attachContainer()
+
+        runWhenAttached(container, isCurrent = { true }, action = ::commitFragmentNow)
+        idleMainLooper()
+
+        assertTrue(fragmentInContainer()?.isAdded == true)
+    }
+
+    @Test
+    fun skipsTheCommitWhenTheViewWasDroppedBeforeItRan() {
+        attachContainer()
+        var actionRuns = 0
+
+        runWhenAttached(container, isCurrent = { false }) {
+            actionRuns++
+            commitFragmentNow()
+        }
+        detachContainer()
+        idleMainLooper()
+
+        assertEquals(0, actionRuns)
+        assertNull(fragmentInContainer())
+    }
+
+    @Test
+    fun waitsForTheContainerToAttachBeforeCommitting() {
+        // Not attached yet: mirrors a screen that is still animating in.
+        runWhenAttached(container, isCurrent = { true }, action = ::commitFragmentNow)
+        idleMainLooper()
+        assertNull("must not commit before the container is in the window", fragmentInContainer())
+
+        attachContainer()
+        idleMainLooper()
+
+        assertTrue(fragmentInContainer()?.isAdded == true)
+    }
+
+    @Test
+    fun doesNotThrowWhenTheContainerIsDetachedWhileStillCurrent() {
+        // Mirrors a screen detached natively while React still has the view mounted.
+        attachContainer()
+        runWhenAttached(container, isCurrent = { true }, action = ::commitFragmentNow)
+        detachContainer()
+
+        idleMainLooper()
+
+        assertNull(fragmentInContainer())
+
+        // ...and the commit still happens once the container comes back.
+        attachContainer()
+        idleMainLooper()
+        assertTrue(fragmentInContainer()?.isAdded == true)
+    }
+}