Merge pull request #294 from wealthfront/lazy-set-synchronous-nav

Prevent crash when navigating between navigables synchronously
wealthfront · Dec 18, 2023 · 9c72d57 · 9c72d57
2 parents 035316a + 6497fba
commit 9c72d57
Show file tree

Hide file tree

Showing 2 changed files with 40 additions and 1 deletion.
diff --git a/magellan-library/src/main/java/com/wealthfront/magellan/navigation/LazySetNavigator.kt b/magellan-library/src/main/java/com/wealthfront/magellan/navigation/LazySetNavigator.kt
@@ -112,7 +112,15 @@ public open class LazySetNavigator(
     navigationPropagator.onNavigatedTo(currentNavigable)
     when (currentState) {
       is LifecycleState.Shown, is LifecycleState.Resumed -> {
-        containerView!!.addView(currentNavigable.view!!, 0)
+        val currentView = currentNavigable.view!!
+        val currentViewParent = currentView.parent
+        if (currentViewParent == null) {
+          containerView!!.addView(currentView, 0)
+        } else if (currentViewParent != containerView) {
+          throw IllegalStateException(
+            "currentNavigable ${currentNavigable.javaClass.simpleName} has view already attached to a parent"
+          )
+        }
       }
       is LifecycleState.Destroyed, is LifecycleState.Created -> {
       }

diff --git a/magellan-library/src/test/java/com/wealthfront/magellan/navigation/LazySetNavigatorTest.kt b/magellan-library/src/test/java/com/wealthfront/magellan/navigation/LazySetNavigatorTest.kt
@@ -106,4 +106,35 @@ class LazySetNavigatorTest {
     verify { navigableListener.onNavigatedTo(step2) }
     verify { navigableListener.afterNavigation() }
   }
+
+  @Test
+  fun replaceTwice() {
+    navigator.addNavigables(setOf(step1, step2))
+    navigator.transitionToState(LifecycleState.Resumed(activityController.get()))
+
+    navigator.replace(step1, CrossfadeTransition())
+    step1.view!!.viewTreeObserver.dispatchOnPreDraw()
+    shadowOf(Looper.getMainLooper()).idle()
+    assertThat(navigator.containerView!!.childCount).isEqualTo(1)
+    assertThat(navigator.containerView!!.getChildAt(0)).isEqualTo(step1.view)
+    assertThat(step1.currentState).isInstanceOf(LifecycleState.Resumed::class.java)
+    assertThat(step2.currentState).isInstanceOf(LifecycleState.Created::class.java)
+
+    verify { navigableListener.beforeNavigation() }
+    verify(exactly = 0) { navigableListener.onNavigatedFrom(any()) }
+    verify { navigableListener.onNavigatedTo(step1) }
+    verify { navigableListener.afterNavigation() }
+    clearMocks(navigableListener)
+    initMocks()
+
+    navigator.replace(step2, CrossfadeTransition())
+    navigator.replace(step1, CrossfadeTransition())
+
+    step1.view!!.viewTreeObserver.dispatchOnPreDraw()
+    shadowOf(Looper.getMainLooper()).idle()
+    assertThat(navigator.containerView!!.childCount).isEqualTo(1)
+    assertThat(navigator.containerView!!.getChildAt(0)).isEqualTo(step1.view)
+    assertThat(step1.currentState).isInstanceOf(LifecycleState.Resumed::class.java)
+    assertThat(step2.currentState).isInstanceOf(LifecycleState.Shown::class.java)
+  }
 }