Enforce mutual exclusivity of SPI navigation handlers#13
Merged
Conversation
`navigationHandler` and `modalNavigationHandler` docstrings on NanoViewController claim each is nil when the other is set, but the helpers only set one half and left the other untouched. If the same scene instance were ever re-subscribed through the inverse helper, a stale handler would silently outlive the contract. Each subscribe helper now clears the inverse hook. Adds two tests that exercise the re-subscribe path to lock the invariant in. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #13 +/- ##
=======================================
Coverage 97.43% 97.43%
=======================================
Files 39 39
Lines 1168 1168
=======================================
Hits 1138 1138
Misses 30 30
🚀 New features to boost your workflow:
|
Contributor
There was a problem hiding this comment.
Pull request overview
This PR aligns the @_spi(Testing) navigation handler hooks on NanoViewController with their documented contract by making the push-style and modal-style handlers mutually exclusive, and adds unit tests to lock in the re-subscribe behavior.
Changes:
- Clear
modalNavigationHandlerwhen subscribing push-style navigation, and clearnavigationHandlerwhen subscribing modal-style navigation. - Add two unit tests covering re-subscribing a scene through the opposite helper (push → modal, modal → push).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
Sources/NanoViewControllerController/Coordinating+NanoViewController+NavigationHelpers.swift |
Clears the inverse SPI hook when subscribing to enforce mutual exclusivity. |
Tests/NanoViewControllerControllerTests/NavigationHandlerSPITests.swift |
Adds tests asserting SPI hook mutual exclusivity on re-subscribe. |
Comments suppressed due to low confidence (1)
Tests/NanoViewControllerControllerTests/NavigationHandlerSPITests.swift:142
- Similar to the previous re-subscribe test: this only checks
navigationHandler/modalNavigationHandlernil-ness, but it doesn’t assert that a subsequent navigation emission routes through only the newly-installed subscription. Adding an assertion that a VM-triggered step after re-subscribe hits only the latest handler would prevent regressions where multiplesinks remain active.
func test_resubscribingPushAfterModal_clearsTheModalHandler() throws {
// Arrange — wire up via modal first.
let nav = ModalPresentCapturingNavigationController()
let coordinator = TestCoordinator(navigationController: nav)
let viewModel = SPITestViewModel()
coordinator.modallyPresent(scene: SPITestScene.self, viewModel: viewModel, animated: false) { _, _ in }
let presented = try XCTUnwrap(nav.presentedViewControllerCapture as? UINavigationController)
let scene = try XCTUnwrap(presented.viewControllers.first as? SPITestScene)
XCTAssertNotNil(scene.modalNavigationHandler)
XCTAssertNil(scene.navigationHandler)
// Act — re-subscribe the same instance through the push helper.
coordinator.subscribeToNavigation(of: scene) { _ in }
// Assert
XCTAssertNotNil(scene.navigationHandler)
XCTAssertNil(scene.modalNavigationHandler)
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
24
to
32
| func subscribeToNavigation<S: NanoViewController<V>, V: ContentView>( | ||
| of scene: S, | ||
| handler: @escaping (V.ViewModel.NavigationStep) -> Void | ||
| ) { | ||
| scene.navigationHandler = handler | ||
| scene.modalNavigationHandler = nil | ||
| scene.navigation | ||
| .sinkOnMain { handler($0) } | ||
| .store(in: &cancellables) |
Comment on lines
+106
to
+117
| func test_resubscribingModalAfterPush_clearsThePushHandler() throws { | ||
| // Arrange — wire up a scene through the push helper first. | ||
| let coordinator = TestCoordinator(navigationController: UINavigationController()) | ||
| let viewModel = SPITestViewModel() | ||
| coordinator.push(scene: SPITestScene.self, viewModel: viewModel, animated: false) { _ in } | ||
| let scene = try XCTUnwrap(coordinator.navigationController.viewControllers.last as? SPITestScene) | ||
| XCTAssertNotNil(scene.navigationHandler) | ||
| XCTAssertNil(scene.modalNavigationHandler) | ||
|
|
||
| // Act — re-subscribe the same instance through the modal helper. | ||
| coordinator.subscribeToModalNavigation(of: scene) { _, _ in } | ||
|
|
The previous commit nil-ed the inverse SPI property when re-subscribing a scene through the opposite helper, but left the previous Combine sink alive in the coordinator's bag. A subsequent `NavigationStep` emission would then route through *both* handler closures — the old subscription held the old closure by capture even after the SPI property went nil. Move the navigation cancellable onto the scene as `navigationSubscription`. Assigning a fresh `AnyCancellable` to the optional property releases (and cancels) the previous one, so re-subscribe genuinely yields "last subscriber wins" on the Combine route. Bonus: the subscription now dies with the scene instead of outliving it in the coordinator's bag. Strengthen the two re-subscribe tests to emit a step through the VM after re-subscribe and assert the previous handler never runs. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Replace bare `github.com/sajjon` parenthetical with explicit `https://github.com/sajjon` link across all 102 source/test/example files. Header-only change, no functional diff.
The macos-26 runner image dropped iOS 26.1 and Xcode 26.1.1; available sim runtimes are now 26.2/26.4/26.5. Pin both the workflow and the justfile default to 26.2 — the lowest version that exists on both the runner and a local machine that ran the previous 26.1 pin successfully. Local override still works: SIM_OS=26.x just test
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
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.
Summary
navigationHandlerandmodalNavigationHandlerdocstrings claim each is nil when the other is set, but the helpers only set one half. This change makes the implementation honor the documented contract.subscribe*helper now clears the inverse SPI hook, so the live handler is whichever helper last subscribed.test_resubscribingModalAfterPush_clearsThePushHandler,test_resubscribingPushAfterModal_clearsTheModalHandler) to lock the invariant in.Background: this fix was pushed to #12's branch as commit
28b3f35but did not land in the merge of #12 intomain.Test plan
just test— full suite, 30 controller tests pass (including the two new re-subscribe tests)🤖 Generated with Claude Code