Introduce Navigating protocol to make decision handlers testable#246
Merged
Conversation
Add a public `Navigating` protocol that abstracts the navigation surface that route and web view policy decision handlers rely on, so handlers can be exercised against a lightweight test double instead of a concrete `Navigator`. It refines the existing `NavigationHandler` (route-only) with pop/clearAll/reload, the root/modal/active navigation controllers, the active web view, and a `currentVisitableURL` convenience whose default implementation reads the active navigation controller's top visitable.
`Navigator` already implements every member, so the conformance is empty. `currentVisitableURL` is satisfied by the protocol's default implementation.
Change the `navigator` parameter from the concrete `Navigator` to the `Navigating` protocol in `RouteDecisionHandler` and `WebViewPolicyDecisionHandler`, the `Router`/`WebViewPolicyManager` that drive them, and all of the built-in handlers. Handler bodies are unchanged -- they only use members the protocol exposes.
Rewrite `NavigationSpy` as a standalone `Navigating` conformer that records route/reload calls and exposes a settable `currentVisitableURL`, instead of subclassing `Navigator` and creating real `Session` and `WKWebView` instances. Update the in-test decision handler spies to the new `Navigating` signature.
The router, web view policy manager, and individual route decision handler tests only pass the navigator through to the code under test; they never rely on real navigation. Swap the concrete `Navigator` for the lightweight `NavigationSpy`, which avoids creating real `Session` and `WKWebView` instances. The Navigator-behavior tests (NavigationHierarchyController*, NavigationDelegateTests) keep the real `Navigator`, since they exercise its actual navigation behavior.
Neither the built-in decision handlers nor the bc3-ios/hey-ios route and web view policy handlers ever read the navigator's web view; the only navigator-level web view/session access in those apps lives in coordinators that hold a concrete `Navigator`. Keeping `activeWebView` on the protocol forced every conformer (and test double) to fabricate a `WKWebView` for no benefit, so remove it. `Navigator` keeps its own public `activeWebView` for concrete callers.
`NavigationHandler` is the "trigger a visit" contract, so all of the `route` overloads belong together there. Move the full-control `route(_:options:parameters:)` down from `Navigating`, and provide `route(_:)` as a default implementation in terms of it. Conformers now only implement `route(_:options:parameters:)` and `route(_ proposal:)`: `Navigator` satisfies both with its own methods (empty conformance), and `HotwireTabBarController` forwards the full-control overload to its active navigator and drops its hand-written `route(_:)`. `Navigating` still exposes the overloads through protocol inheritance, so nothing changes for decision handlers.
svara
marked this pull request as ready for review
June 30, 2026 11:26
zoejessica
approved these changes
Jul 1, 2026
zoejessica
left a comment
zoejessica
left a comment
Contributor
There was a problem hiding this comment.
Nicely done @svara, I'm really happy to see this one get merged in! 👏🚀
* main: (21 commits) Restore navigation simulator leak fix lost in merge Test unsafe-scheme handling. Cancel external navigation to unsafe URL schemes Handle taps inside iframes that request a new window Revert "Handle requests for new windows by proposing a new visit" Add tests for always-on visit retry handler Fix inaccurate log messages in Navigator session inspection Log HTTP status code on cold boot visit failures Add logging to JSFetch recovery handler and error paths Retry failed visit directly when no topmost visitable Remove isRetryable logic and always provide clients with a retry handler Update tests Rename to JSFetchRecoveryHandler Update to allow retries for offline and timeout errors Additional logging Lower logging level for WebViewPolicyManager Allow injection of a custom logger Handle requests for new windows by proposing a new visit Place menu popover on the element that triggered it Use latest method on iOS 16+; refactor tests ...
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
Route and web view policy decision handlers receive a concrete
Navigator, so unit-testing a custom handler means subclassingNavigatorand dragging in realSessionandWKWebViewinstances. Handlers actually touch only a thin slice ofNavigator(route,reload,activeNavigationController).This introduces a
Navigatingprotocol so handlers depend on an abstraction and can be tested against a lightweight double.What
Navigatingprotocol that refines the existingNavigationHandler, exposing the navigation surface handlers actually use: therouteoverloads,pop/clearAll/reload, the root/modal/active navigation controllers, and a newcurrentVisitableURLconvenience (default implementation reads the active navigation controller's top visitable).Navigatorconforms with no additional code.RouteDecisionHandler/WebViewPolicyDecisionHandler(and theRouter/WebViewPolicyManagerthat drive them) now takeNavigatinginstead of the concreteNavigator.NavigationSpyis now a lightweight standaloneNavigatingconformer (noSession/WKWebView), and the decision handler tests use it instead of a realNavigator.Design notes
session/modalSession,webkitUIDelegate,delegate, andstart()are intentionally not on the protocol — they're implementation details and would force test doubles to fabricate heavyweight collaborators.routeoverloads live onNavigationHandler. The full-controlroute(_:options:parameters:)is the requirement;route(_:)is a default implementation in terms of it, so conformers (incl.HotwireTabBarController) implement only the primitives.Breaking changes (source)
Both are minor and apply only to code that implements these protocols, not code that calls them:
RouteDecisionHandler/WebViewPolicyDecisionHandlerimplementations must change theirhandle(...)navigatorparameter fromNavigatortoNavigating(one-line signature change; bodies unchanged). This is the intended enabler for testing them.route(_:options:parameters:)as aNavigationHandlerrequirement affects any external type conforming toNavigationHandlerdirectly (a consume-not-implement protocol; no known external conformers).Testing
The routing handler suites also got noticeably faster now that they no longer spin up real sessions/web views.