[PF-248] [1 of 5] Extract shared deep-link router out of AppDelegateViewModel - #2989
stevestreza-ksr wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
🟢 Approval recommended
No unresolved issues were identified that would block approval.
Pull request overview
Extracts deep-link routing from AppDelegateViewModel into a reusable router for future SceneDelegate integration.
Changes:
- Added
DeepLinkNavigationRouterwith shared routing signals and helpers. - Updated
AppDelegateViewModelto consume router outputs. - Added direct router tests.
File summaries
| File | Description |
|---|---|
| Kickstarter-Framework/Sources/Kickstarter-Framework/Kickstarter-iOS/DeepLinkNavigationRouter.swift | Updated as part of this pull request. |
| Kickstarter-Framework/Sources/Kickstarter-Framework/Kickstarter-iOS/AppDelegateViewModel.swift | Updated as part of this pull request. |
| Kickstarter-Framework/Sources/Kickstarter-Framework-iOSTests/Kickstarter-iOS/DeepLinkNavigationRouterTests.swift | Updated as part of this pull request. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
d27aea3 to
3935801
Compare
Generated by 🚫 Danger |
AppDelegateViewModel resolves a merged `Navigation` stream (from push notifications, Braze, opened URLs, continued user activities, and shortcut items) into concrete UI actions — which tab to switch to, what to present, which login intent to use, and so on. Pull that resolution logic into its own `DeepLinkNavigationRouter`, so it can be reused once URL/user-activity/shortcut-item handling moves to a scene delegate. This is a pure refactor: `AppDelegateViewModel` still owns every input and output it did before, still merges the same four deep-link sources, and simply hands the merged stream to the router instead of resolving it inline. `AppDelegateViewModelTests` is unchanged and passes as-is against the new implementation, which is the check that this doesn't alter behavior. Also adds `DeepLinkNavigationRouterTests`, covering the router directly: tab routes, both login intents, message threads, presented project stacks, the notification-settings user update, and that an unrouted `Navigation` emits nothing. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
3935801 to
a0c19b8
Compare
amy-at-kickstarter
left a comment
There was a problem hiding this comment.
LGTM. I skimmed the moved code, just to verify that it looks unchanged, but I'm largely relying on the fact that AppDelegateViewModelTests is unchanged and should catch any regressions.
Most of my suggestions are around improving the clarity and coverage of the new DeepLinkNavigationRouterTests.
| self.goToProfile = goToProfile | ||
| self.goToSearch = goToSearch | ||
| self.presentViewController = presentViewController | ||
| self.updateCurrentUserInEnvironment = updatedUserNotificationSettings |
There was a problem hiding this comment.
Nit: I don't see an advantage to defining these all at the bottom of the method when they could be defined inline, e.g.
self.goToActivity = deepLink.filter { ...| self.goToSearch.assertValueCount(0) | ||
| self.goToProfile.assertValueCount(0) |
There was a problem hiding this comment.
Nit: This tests negative cases for only two outputs, instead of all the other possibilities.
To cover all the possible signals, this test could have a combined observer like
private let goToAnyPage = TestObserver<(), Never>()
//...
let allSignals = Signal.merge(outputs.goToActivity, outputs.goToDiscovery, //etc
allSignals.observe(self.goToAnyPage.observer)and then in the test
self.goToAnyPage.assertValueCount(1)| self.goToProfile.assertValueCount(0) | ||
| self.goToSearch.assertValueCount(0) | ||
| self.presentViewControllerCount.assertValues([]) | ||
| self.updateCurrentUserInEnvironment.assertDidNotEmitValue() |
There was a problem hiding this comment.
Same nit here, this might make more sense as a combined observer.
| } | ||
| } | ||
|
|
||
| // MARK: - Notification settings |
There was a problem hiding this comment.
Nit: missing a test case for the Pledge Manager web view (which is also output as part of outputs.presentViewController
📲 What
This PR refactors out several signals for routing deep links into a standalone object.
🤔 Why
AppDelegate has a bunch of behavioral logic for routing that we need to reuse in both AppDelegate and SceneDelegate.
🛠 How
Claude was prompted to extract logic from the AppDelegateViewModel relating to routing, and move this into a standalone object. Existing usages were changed to use this new object, and existing tests were moved as part of Claude's migration.
🤖 Claude-Generated Summary
AppDelegateViewModel resolves a merged
Navigationstream (from push notifications, Braze, opened URLs, continued user activities, and shortcut items) into concrete UI actions — which tab to switch to, what to present, which login intent to use, and so on. Pull that resolution logic into its ownDeepLinkNavigationRouter, so it can be reused once URL/user-activity/shortcut-item handling moves to a scene delegate.This is a pure refactor:
AppDelegateViewModelstill owns every input and output it did before, still merges the same four deep-link sources, and simply hands the merged stream to the router instead of resolving it inline.AppDelegateViewModelTestsis unchanged and passes as-is against the new implementation, which is the check that this doesn't alter behavior.Also adds
DeepLinkNavigationRouterTests, covering the router directly: tab routes, both login intents, message threads, presented project stacks, the notification-settings user update, and that an unroutedNavigationemits nothing.