Fix navigationStackCoordinator getting torn down when being reset on … (

#613) * Fix navigationStackCoordinator getting torn down when being reset on the navigationSplitCoordinator, added unit tests for it * Guard against using the same coordinator more than once
element-hq · Feb 23, 2023 · ad06e81 · ad06e81
1 parent 99d6cae
commit ad06e81
Show file tree

Hide file tree

Showing 4 changed files with 39 additions and 3 deletions.
diff --git a/ElementX/Sources/Application/Navigation/NavigationCoordinators.swift b/ElementX/Sources/Application/Navigation/NavigationCoordinators.swift
@@ -138,6 +138,10 @@ class NavigationSplitCoordinator: CoordinatorProtocol, ObservableObject, CustomS
             return
         }
 
+        if sidebarModule?.coordinator === coordinator {
+            fatalError("Cannot use the same coordinator more than once")
+        }
+
         sidebarModule = NavigationModule(coordinator, dismissalCallback: dismissalCallback)
     }
 
@@ -151,6 +155,10 @@ class NavigationSplitCoordinator: CoordinatorProtocol, ObservableObject, CustomS
             return
         }
 
+        if detailModule?.coordinator === coordinator {
+            fatalError("Cannot use the same coordinator more than once")
+        }
+
         detailModule = NavigationModule(coordinator, dismissalCallback: dismissalCallback)
     }
 
@@ -164,6 +172,10 @@ class NavigationSplitCoordinator: CoordinatorProtocol, ObservableObject, CustomS
             return
         }
 
+        if sheetModule?.coordinator === coordinator {
+            fatalError("Cannot use the same coordinator more than once")
+        }
+
         sheetModule = NavigationModule(coordinator, dismissalCallback: dismissalCallback)
     }
 
@@ -177,6 +189,10 @@ class NavigationSplitCoordinator: CoordinatorProtocol, ObservableObject, CustomS
             return
         }
 
+        if fullScreenCoverModule?.coordinator === coordinator {
+            fatalError("Cannot use the same coordinator more than once")
+        }
+
         fullScreenCoverModule = NavigationModule(coordinator, dismissalCallback: dismissalCallback)
     }
 
@@ -509,6 +525,10 @@ class NavigationStackCoordinator: ObservableObject, CoordinatorProtocol, CustomS
             return
         }
 
+        if rootModule?.coordinator === coordinator {
+            fatalError("Cannot use the same coordinator more than once")
+        }
+
         popToRoot(animated: false)
 
         rootModule = NavigationModule(coordinator, dismissalCallback: dismissalCallback)
@@ -565,6 +585,10 @@ class NavigationStackCoordinator: ObservableObject, CoordinatorProtocol, CustomS
             return
         }
 
+        if sheetModule?.coordinator === coordinator {
+            fatalError("Cannot use the same coordinator more than once")
+        }
+
         sheetModule = NavigationModule(coordinator, dismissalCallback: dismissalCallback)
     }
 
@@ -584,6 +608,10 @@ class NavigationStackCoordinator: ObservableObject, CoordinatorProtocol, CustomS
             return
         }
 
+        if fullScreenCoverModule?.coordinator === coordinator {
+            fatalError("Cannot use the same coordinator more than once")
+        }
+
         fullScreenCoverModule = NavigationModule(coordinator, dismissalCallback: dismissalCallback)
     }
 

diff --git a/ElementX/Sources/Services/UserSession/UserSessionFlowCoordinator.swift b/ElementX/Sources/Services/UserSession/UserSessionFlowCoordinator.swift
@@ -168,8 +168,7 @@ class UserSessionFlowCoordinator: CoordinatorProtocol {
                                                              emojiProvider: emojiProvider)
             let coordinator = RoomScreenCoordinator(parameters: parameters)
 
-            detailNavigationStackCoordinator.setRootCoordinator(coordinator)
-            navigationSplitCoordinator.setDetailCoordinator(detailNavigationStackCoordinator) { [weak self, roomIdentifier] in
+            detailNavigationStackCoordinator.setRootCoordinator(coordinator) { [weak self, roomIdentifier] in
                 guard let self else { return }
 
                 // Move the state machine to no room selected if the room currently being dimissed
@@ -180,6 +179,10 @@ class UserSessionFlowCoordinator: CoordinatorProtocol {
                     self.detailNavigationStackCoordinator.setRootCoordinator(nil)
                 }
             }
+
+            if navigationSplitCoordinator.detailCoordinator == nil {
+                navigationSplitCoordinator.setDetailCoordinator(detailNavigationStackCoordinator)
+            }
         }
     }
 

diff --git a/UnitTests/Sources/NavigationSplitCoordinatorTests.swift b/UnitTests/Sources/NavigationSplitCoordinatorTests.swift
@@ -259,9 +259,13 @@ class NavigationSplitCoordinatorTests: XCTestCase {
     // MARK: - Private
 
     private func assertCoordinatorsEqual(_ lhs: CoordinatorProtocol?, _ rhs: CoordinatorProtocol?) {
+        if lhs == nil, rhs == nil {
+            return
+        }
+
         guard let lhs = lhs as? SomeTestCoordinator,
               let rhs = rhs as? SomeTestCoordinator else {
-            XCTFail("Coordinators are not the same")
+            XCTFail("Coordinators are not the same: \(String(describing: lhs)) != \(String(describing: rhs))")
             return
         }
 

diff --git a/changelog.d/pr-613.bugfix b/changelog.d/pr-613.bugfix
@@ -0,0 +1 @@
+Fix broken split layout room navigation