Stamp routed location on view controllers for navigation identity#248
Conversation
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.
zoejessica
left a comment
There was a problem hiding this comment.
Sometimes the old ways are the best @svara 👏
No issues with this, apart from a question on the assertion failure. UIKit does let users manually push unrouted view controllers on the stack whether in production or in testing, so I'm thinking we should assume that lots of users do. I can imagine it being really annoying to have to fork just to remove the assertion in development. Maybe we should provide a setting, or downgrade it to a log?
My intent with the assertion was to guard against unintended/uncommon use of the library. But you are right that we can't distinguish that from legitimate mixed navigation. This is something we should aim for. I've downgraded it to a |
Summary
Navigation identity checks —
visitingSamePage(→ replace) andvisitingPreviousPage(→ pop) inNavigationHierarchyController— decided a screen's URL by casting the on-stack controller toVisitable/VisitableViewControllerand readinginitialVisitableURL, falling back to view-controller type equality when the cast failed.That fallback is wrong for custom native view controllers: two instances of the same custom class at different URLs are indistinguishable, so the navigator incorrectly pops or replaces instead of pushing.
This PR makes the routed location the single source of truth for navigation identity. The navigator stamps the proposal URL onto every routed controller at route time; the identity checks compare that stamped location and no longer look at the controller's type at all.
This is a direct port of how Hotwire Native Android already solves the same problem (see Parity with Android). It is an alternative to the opt-in protocol approach in #237.
The change
UIViewController.routedLocation: URL?— an associated-object property.NavigationHierarchyController.route(controller:proposal:)stampscontroller.routedLocation = proposal.urlat the single routing choke point (covers the main and modal stacks, push/replace/replaceRoot).visitingSamePage/visitingPreviousPagecompare the stamped location viaisSameLocation(as:pathProperties:). The candidate-controllertype(of:)comparison is gone.assertionFailurein debug and returnfalsein release, so the visit is treated as a new page and pushed (safe: never a wrong pop/replace).Parity with Android
Android has always used exactly this model, and this PR converges iOS onto it:
NavigatorRulereadscontroller.currentBackStackEntry.location/previousBackStackEntry.location, where.locationisarguments?.getString(ARG_LOCATION)— the location stored in the back-stack entry's arguments, stamped at navigation time inNavigatorRule.withNavArguments(). It never derives identity from the fragment or its type.locationsAreSamereturnsfalsewhen either side is null → falls through to push (the same push-on-miss behavior).pushViewController(_:)an arbitrary controller, so the unrouted-controller bug is reachable on iOS. TheassertionFailureis the iOS-specific guard for a gap Android closes by construction.Alternatives considered — identity source
How should the navigator know a screen's URL?
UIHostingController, third-party, plainUIViewController); single source of truth; matches AndroidPathIdentifiableprotocol (#237)extension UIViewController: PathIdentifiableinitialVisitableURLis non-optional, so a plain VC must return a sentinel URL — and once every VC "conforms," two of them collide on that sentinel and get wrongly treated as the same page (worse than the original bug). Also, a non-@objcproperty declared in a class extension cannot be overridden by subclasses (verified:error: non-'@objc' property 'initialVisitableURL' is declared in extension of 'UIViewController' and cannot be overridden), soVisitableViewControllercould no longer report its real URL. The@objc dynamicescape hatch dragsVisitableinto@objcland and still doesn't fix the sentinel problem.acceptCustom(UIViewController & PathIdentifiable)acceptCustomcurrently carriesUIAlertController(and could carry a share sheet), which can't sanely bePathIdentifiable, forcing an API split (pages vs. presentations);UIHostingControllercan't conform without a subclass; shifts a correctness responsibility onto adopters (a VC can report a URL that disagrees with the proposal that routed it)UIHostingController,UITableViewController, third-party VCs, and apps with their own base class can't all subclass it; undermines the whole point ofacceptCustom(UIViewController); still opt-in (remember to subclass)We concluded the navigator (which generated the URL) should own it, rather than requiring every custom VC to re-declare a URL the framework already handed it in
handle(proposal:).Alternatives considered — storage mechanism
Given that we stamp the location onto the controller, how should it be stored? Extensions can't add stored properties to
UIViewController, so:objc_setAssociatedObject(this PR)popToViewController); auto-freed ondealloc— no lifecycle bookkeeping; faithful analog of Android's per-entry arguments bag; a mainstream, Apple-documented idiom[URL]stack in the navigatorDictionary<ObjectIdentifier, URL>in the navigatorObjectIdentifieris reused afterdealloc, so a stale entry can match a different controller — correctness landmineNSMapTable(weak keys) in the navigatorUIViewControllerWhy
objc_setAssociatedObjectis the right choice hereObjectIdentifiermap risks matching a reused identity.objc_getAssociatedObject/objc_setAssociatedObjectis the canonical, Apple-documented mechanism for attaching state to a class you don't own. It's used across the ecosystem.The
malloc(1)!key is deliberate: it's a stable, unique, Swift-6-concurrency-clean key that avoids the&staticVarwarning. The whole thing is isolated to one smallinternalextension, so the runtime call doesn't leak into the rest of the codebase.Design decisions within the chosen approach
Visitable.initialVisitableURL ?? routedLocationpreference). For every controller the framework routes,initialVisitableURL == routedLocation == proposal.url, so the preference was redundant on the happy path. Its only effect was to silently exempt an unroutedVisitablefrom the "everything on the stack is routed" invariant — but an unroutedVisitablebypasseddelegate.visit()and is itself a bug, so it should trip the assertion like anything else.assertionFailure+ push, not a hardprecondition. A hard trap would crash apps that mix manual UIKit navigation with Hotwire (and would break tests that place a pre-existing screen on the stack).assertionFailureflags the bug loudly in development while degrading safely (push) in release.Testing
Visitablecontrollers of the same class and assert push / replace / pop by URL. Verified they fail without the production change and pass with it (the different-URL and previous-URL cases; the same-URL case guards the collapse invariant).routedViewController(url:)helper), reflecting that every stack member is routed.