From 2c769a8e09b569e973402db881bf45574b773fd0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Denis=20S=CC=8Cvara?= Date: Tue, 30 Jun 2026 15:34:59 +0200 Subject: [PATCH 1/2] Stamp routed location on view controllers for navigation identity Navigation identity checks (visitingSamePage/visitingPreviousPage) derived a screen's URL by casting to Visitable/VisitableViewController and fell back to view-controller type equality otherwise. Two instances of the same custom view-controller class at different URLs were then indistinguishable, causing an incorrect pop or replace instead of a push. Stamp the routed URL onto every routed controller at route time and read it back for identity comparisons, preferring a Visitable's own initialVisitableURL when present. This mirrors the Android navigator, which stores the location in the back-stack entry's arguments rather than deriving it from the destination's type, and works for any custom view controller without opt-in. --- .../UIViewController+RoutedLocation.swift | 13 ++++ .../NavigationHierarchyController.swift | 48 ++++++++---- .../NavigationHierarchyControllerTests.swift | 78 ++++++++++++++++++- 3 files changed, 121 insertions(+), 18 deletions(-) create mode 100644 Source/Turbo/Navigator/Extensions/UIViewController+RoutedLocation.swift diff --git a/Source/Turbo/Navigator/Extensions/UIViewController+RoutedLocation.swift b/Source/Turbo/Navigator/Extensions/UIViewController+RoutedLocation.swift new file mode 100644 index 00000000..f3102c2c --- /dev/null +++ b/Source/Turbo/Navigator/Extensions/UIViewController+RoutedLocation.swift @@ -0,0 +1,13 @@ +import UIKit + +private let routedLocationKey = malloc(1)! + +extension UIViewController { + /// The location the navigator routed this view controller to, stamped at route time by + /// `NavigationHierarchyController`. Navigation identity checks read this so two instances + /// of the same custom view-controller class at different URLs are distinguishable. + var routedLocation: URL? { + get { objc_getAssociatedObject(self, routedLocationKey) as? URL } + set { objc_setAssociatedObject(self, routedLocationKey, newValue, .OBJC_ASSOCIATION_RETAIN_NONATOMIC) } + } +} diff --git a/Source/Turbo/Navigator/NavigationHierarchyController.swift b/Source/Turbo/Navigator/NavigationHierarchyController.swift index b69b50f6..a467de44 100644 --- a/Source/Turbo/Navigator/NavigationHierarchyController.swift +++ b/Source/Turbo/Navigator/NavigationHierarchyController.swift @@ -37,6 +37,8 @@ class NavigationHierarchyController { if let alert = controller as? UIAlertController { presentAlert(alert, via: proposal) } else { + controller.routedLocation = proposal.url + if let visitable = controller as? Visitable { visitable.visitableView.allowsPullToRefresh = proposal.pullToRefreshEnabled } @@ -134,9 +136,9 @@ class NavigationHierarchyController { with controller: UIViewController, via proposal: VisitProposal, didReplaceModalContext: Bool = false) { - if visitingSamePage(on: navigationController, with: controller, via: proposal) { + if visitingSamePage(on: navigationController, via: proposal) { navigationController.replaceLastViewController(with: controller) - } else if visitingPreviousPage(on: navigationController, with: controller, via: proposal) { + } else if visitingPreviousPage(on: navigationController, via: proposal) { navigationController.popViewController(animated: proposal.animated) } else if proposal.options.action == .advance || didReplaceModalContext { navigationController.pushViewController(controller, animated: proposal.animated) @@ -145,27 +147,41 @@ class NavigationHierarchyController { } } - private func visitingSamePage(on navigationController: UINavigationController, - with controller: UIViewController, - via proposal: VisitProposal) -> Bool { - if let visitable = navigationController.topViewController as? Visitable { - return visitable.initialVisitableURL.isSameLocation(as: proposal.url, pathProperties: proposal.properties) - } else if let topViewController = navigationController.topViewController { - return topViewController.isMember(of: type(of: controller)) + private func visitingSamePage( + on navigationController: UINavigationController, + via proposal: VisitProposal + ) -> Bool { + guard let topViewController = navigationController.topViewController else { return false } + + guard let location = topViewController.routedLocation else { + assertionFailure("Top view controller \(topViewController) has no routed location. " + + "Controllers placed on the navigator's stack must be routed through it; treating the visit as a new page.") + return false } - return false + + return location.isSameLocation( + as: proposal.url, + pathProperties: proposal.properties + ) } - private func visitingPreviousPage(on navigationController: UINavigationController, - with controller: UIViewController, - via proposal: VisitProposal) -> Bool { + private func visitingPreviousPage( + on navigationController: UINavigationController, + via proposal: VisitProposal + ) -> Bool { guard navigationController.viewControllers.count >= 2 else { return false } let previousController = navigationController.viewControllers[navigationController.viewControllers.count - 2] - if let previousVisitable = previousController as? VisitableViewController { - return previousVisitable.initialVisitableURL.isSameLocation(as: proposal.url, pathProperties: proposal.properties) + guard let location = previousController.routedLocation else { + assertionFailure("Previous view controller \(previousController) has no routed location. " + + "Controllers placed on the navigator's stack must be routed through it; treating the visit as a new page.") + return false } - return type(of: previousController) == type(of: controller) + + return location.isSameLocation( + as: proposal.url, + pathProperties: proposal.properties + ) } private func replace(with controller: UIViewController, via proposal: VisitProposal) { diff --git a/Tests/Turbo/Navigator/NavigationHierarchyControllerTests.swift b/Tests/Turbo/Navigator/NavigationHierarchyControllerTests.swift index 500b0a2d..ebd4cb2b 100644 --- a/Tests/Turbo/Navigator/NavigationHierarchyControllerTests.swift +++ b/Tests/Turbo/Navigator/NavigationHierarchyControllerTests.swift @@ -83,6 +83,41 @@ final class NavigationHierarchyControllerTests: XCTestCase { assertVisited(url: oneURL, on: .main) } + func test_customViewController_visitingDifferentURL_pushesOnMainStack() { + let navigator = makeCustomViewControllerNavigator() + + navigator.route(oneURL) + XCTAssertEqual(navigationController.viewControllers.count, 1) + XCTAssert(navigationController.topViewController is CustomViewController) + + navigator.route(twoURL) + XCTAssertEqual(navigationController.viewControllers.count, 2) + XCTAssert(navigationController.topViewController is CustomViewController) + } + + func test_customViewController_visitingSameURL_replacesOnMainStack() { + let navigator = makeCustomViewControllerNavigator() + + navigator.route(oneURL) + XCTAssertEqual(navigationController.viewControllers.count, 1) + + navigator.route(oneURL) + XCTAssertEqual(navigationController.viewControllers.count, 1) + XCTAssert(navigationController.topViewController is CustomViewController) + } + + func test_customViewController_visitingPreviousURL_popsOnMainStack() { + let navigator = makeCustomViewControllerNavigator() + + navigator.route(oneURL) + navigator.route(twoURL) + XCTAssertEqual(navigationController.viewControllers.count, 2) + + navigator.route(oneURL) + XCTAssertEqual(navigationController.viewControllers.count, 1) + XCTAssert(navigationController.topViewController is CustomViewController) + } + func test_default_default_default_replaceAction_replacesOnMainStack() { let proposal = VisitProposal(action: .replace) navigator.route(proposal) @@ -188,7 +223,7 @@ final class NavigationHierarchyControllerTests: XCTestCase { } func test_modal_default_default_dismissesModalThenPushesOnMainStack() { - navigationController.pushViewController(UIViewController(), animated: false) + navigationController.pushViewController(routedViewController(url: baseURL.appendingPathComponent("/existing")), animated: false) XCTAssertEqual(navigationController.viewControllers.count, 1) navigator.route(VisitProposal(context: .modal)) @@ -327,7 +362,7 @@ final class NavigationHierarchyControllerTests: XCTestCase { } func test_default_any_pop_popsOffMainStack() { - navigationController.pushViewController(UIViewController(), animated: false) + navigationController.pushViewController(routedViewController(url: baseURL.appendingPathComponent("/existing")), animated: false) XCTAssertEqual(navigationController.viewControllers.count, 1) navigator.route(VisitProposal()) @@ -482,12 +517,39 @@ final class NavigationHierarchyControllerTests: XCTestCase { private var navigator: Navigator! private let alertControllerDelegate = AlertControllerDelegate() + // `Navigator.delegate` is weak, so the test must hold a strong reference. + private let customViewControllerDelegate = CustomViewControllerDelegate() private var hierarchyController: NavigationHierarchyController! private var navigationController: TestableNavigationController! private var modalNavigationController: TestableNavigationController! private let window = UIWindow() + // A navigator whose delegate routes custom, non-`Visitable` view controllers, wired to the + // same navigation controllers the test asserts against. + private func makeCustomViewControllerNavigator() -> Navigator { + let navigator = Navigator( + session: session, + modalSession: modalSession, + delegate: customViewControllerDelegate, + configuration: .init(name: "Test", startLocation: oneURL) + ) + navigator.hierarchyController = NavigationHierarchyController( + delegate: navigator, + navigationController: navigationController, + modalNavigationController: modalNavigationController + ) + return navigator + } + + // A view controller stamped with a routed location, standing in for a screen the navigator + // has already routed. Identity checks require every stack member to have a location. + private func routedViewController(url: URL) -> UIViewController { + let viewController = UIViewController() + viewController.routedLocation = url + return viewController + } + // Simulate a "real" app so presenting view controllers works under test. private func loadNavigationControllerInWindow() { window.rootViewController = navigationController @@ -548,3 +610,15 @@ private class AlertControllerDelegate: NavigatorDelegate { return .accept } } + +// MARK: - CustomViewControllerDelegate + +/// A non-`Visitable` custom view controller. Two instances at different URLs share a type, so +/// the navigator must rely on the stamped routed location — not type equality — to tell them apart. +private final class CustomViewController: UIViewController {} + +private class CustomViewControllerDelegate: NavigatorDelegate { + func handle(proposal: VisitProposal, from navigator: Navigator) -> ProposalResult { + .acceptCustom(CustomViewController()) + } +} From 0ca4cfec92b932ef181a8e3c3e0604b4efe0dcde Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Denis=20S=CC=8Cvara?= Date: Thu, 2 Jul 2026 16:12:21 +0200 Subject: [PATCH 2/2] Downgrade unrouted-location signal from assertion to warning log --- .../NavigationHierarchyController.swift | 8 +++--- .../NavigationHierarchyControllerTests.swift | 26 ++++++++++++------- 2 files changed, 20 insertions(+), 14 deletions(-) diff --git a/Source/Turbo/Navigator/NavigationHierarchyController.swift b/Source/Turbo/Navigator/NavigationHierarchyController.swift index a467de44..490c4a9c 100644 --- a/Source/Turbo/Navigator/NavigationHierarchyController.swift +++ b/Source/Turbo/Navigator/NavigationHierarchyController.swift @@ -154,8 +154,8 @@ class NavigationHierarchyController { guard let topViewController = navigationController.topViewController else { return false } guard let location = topViewController.routedLocation else { - assertionFailure("Top view controller \(topViewController) has no routed location. " + - "Controllers placed on the navigator's stack must be routed through it; treating the visit as a new page.") + logger.warning("Top view controller \(topViewController) has no routed location; " + + "treating the visit as a new page. Expected for controllers pushed onto the stack outside the navigator.") return false } @@ -173,8 +173,8 @@ class NavigationHierarchyController { let previousController = navigationController.viewControllers[navigationController.viewControllers.count - 2] guard let location = previousController.routedLocation else { - assertionFailure("Previous view controller \(previousController) has no routed location. " + - "Controllers placed on the navigator's stack must be routed through it; treating the visit as a new page.") + logger.warning("Previous view controller \(previousController) has no routed location; " + + "treating the visit as a new page. Expected for controllers pushed onto the stack outside the navigator.") return false } diff --git a/Tests/Turbo/Navigator/NavigationHierarchyControllerTests.swift b/Tests/Turbo/Navigator/NavigationHierarchyControllerTests.swift index ebd4cb2b..20ec3802 100644 --- a/Tests/Turbo/Navigator/NavigationHierarchyControllerTests.swift +++ b/Tests/Turbo/Navigator/NavigationHierarchyControllerTests.swift @@ -118,6 +118,20 @@ final class NavigationHierarchyControllerTests: XCTestCase { XCTAssert(navigationController.topViewController is CustomViewController) } + func test_unroutedViewController_onStack_visitPushesOnMainStack() { + // A view controller pushed onto the stack outside the navigator (e.g. mixed native + // navigation) has no routed location. A subsequent visit must treat it as a new page + // and push — never crash or incorrectly replace/pop. + navigationController.pushViewController(UIViewController(), animated: false) + XCTAssertEqual(navigationController.viewControllers.count, 1) + + navigator.route(oneURL) + + XCTAssertEqual(navigationController.viewControllers.count, 2) + XCTAssert(navigationController.topViewController is VisitableViewController) + assertVisited(url: oneURL, on: .main) + } + func test_default_default_default_replaceAction_replacesOnMainStack() { let proposal = VisitProposal(action: .replace) navigator.route(proposal) @@ -223,7 +237,7 @@ final class NavigationHierarchyControllerTests: XCTestCase { } func test_modal_default_default_dismissesModalThenPushesOnMainStack() { - navigationController.pushViewController(routedViewController(url: baseURL.appendingPathComponent("/existing")), animated: false) + navigationController.pushViewController(UIViewController(), animated: false) XCTAssertEqual(navigationController.viewControllers.count, 1) navigator.route(VisitProposal(context: .modal)) @@ -362,7 +376,7 @@ final class NavigationHierarchyControllerTests: XCTestCase { } func test_default_any_pop_popsOffMainStack() { - navigationController.pushViewController(routedViewController(url: baseURL.appendingPathComponent("/existing")), animated: false) + navigationController.pushViewController(UIViewController(), animated: false) XCTAssertEqual(navigationController.viewControllers.count, 1) navigator.route(VisitProposal()) @@ -542,14 +556,6 @@ final class NavigationHierarchyControllerTests: XCTestCase { return navigator } - // A view controller stamped with a routed location, standing in for a screen the navigator - // has already routed. Identity checks require every stack member to have a location. - private func routedViewController(url: URL) -> UIViewController { - let viewController = UIViewController() - viewController.routedLocation = url - return viewController - } - // Simulate a "real" app so presenting view controllers works under test. private func loadNavigationControllerInWindow() { window.rootViewController = navigationController