Skip to content

Migrate watch navigation to a value-based NavigationStack - #4804

Draft
kean wants to merge 1 commit into
trunkfrom
kean/watch-navigation-stack
Draft

Migrate watch navigation to a value-based NavigationStack#4804
kean wants to merge 1 commit into
trunkfrom
kean/watch-navigation-stack

Conversation

@kean

@kean kean commented Jul 21, 2026

Copy link
Copy Markdown
Contributor

The watch app still navigated through NavigationLink(destination:tag:selection:), deprecated since watchOS 9. Every link was driven by a selection binding, and NavigationManager tracked the pushed screen as an Int index left over from the old WatchKit page-based API.

This moves the whole watch app to a value-based stack:

  • NavigationManager owns the stack as @Published var path: [WatchRoute]. navigateTo replaces the stack (matching the old tag/selection behavior, where only one link could be active at a time); the new push appends, for screens opened from within Now Playing.
  • New WatchRoute enum plus WatchRouteView, which maps a route to its screen and is registered once via .navigationDestination(for:) on the root stack, so any screen can push any route.
  • All rows are now NavigationLink(value:).
  • SourceInterfaceNavigationView uses NavigationStack(path:) instead of the deprecated NavigationView.
  • NowPlayingContainerView loses the ZStack of hidden zero-size NavigationLinks that existed only so buttons inside the paged TabView could push. Those buttons now call navigationModel.push(_:), which also removes the presentView binding threaded through NowPlayingOptions and NowPlayingControls.
  • NowPlayingRow contained its own NavigationLink, so in InterfaceView it was nested inside another link. The link moved out to the two call sites (InterfaceView, UpNextView).
  • Removed WatchInterfaceType.interfacePosition / .indexPosition and the now-unused NavigationManager.navigateToMainMenu() / topMostController(), which drops the last WKInterfaceController usage from the navigation layer.

.unknown, .interface, and .filter never had a destination, so WatchRoute.init? returns nil for them and they stay no-ops rather than pushing a blank screen.

No intended behavior change. The app still launches into the current source's interface list, which the old code achieved by pre-seeding the selection binding and this preserves by seeding path.

To test

Requires a paired Apple Watch or the watch simulator.

  1. Launch the watch app. It should open directly on the current source's list (Phone or Watch), with the source picker one back-swipe away.
  2. From the source picker, tap Phone and then Watch (Plus accounts). Each should push the matching list, and the source switch side effects should still run — the active-source indicator moves and a sync/refresh is triggered when the now playing episodes differ between the two.
  3. From the interface list, open each row in turn: Now Playing, Up Next, Podcasts, Playlists, Downloads, Files. Each should push its screen, and back-swipe should return.
  4. Open Now Playing, swipe left to the options page and tap Episode Details. The episode should push on top of Now Playing.
  5. On the Now Playing controls page, tap the effects icon and then the Up Next icon. Both should push on top of Now Playing, not replace it.
  6. In Up Next, tap the Now Playing row at the top — it should push Now Playing once, not twice.
  7. Background the app from a screen other than the root, then relaunch. State restoration should land you back on that screen.
  8. With the watch app closed, start playback on the phone so watchOS auto-launches the app. It should open on Now Playing for the phone source.

Checklist

  • I have considered if this change warrants user-facing release notes and have added them to CHANGELOG.md if necessary.
  • I have considered adding unit tests for my changes.
  • I have updated (or requested that someone edit) the Event Horizon schema to reflect any new or changed analytics.

Replaces the deprecated NavigationLink(destination:tag:selection:) links
with NavigationLink(value:) and a single navigationDestination(for:)
registered on the root stack.

NavigationManager now owns the stack as [WatchRoute] instead of tracking
a selected interface index, so taps and programmatic navigation share one
source of truth.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0124af39BcDTwppPNMJJfSqn
@kean kean added this to the 8.18 milestone Jul 21, 2026
@kean kean added [Area] watchOS Issus related to watchOS refactoring Refactoring related issues labels Jul 21, 2026
@claude

claude Bot commented Jul 21, 2026

Copy link
Copy Markdown

Claude finished @kean's task in 3m 43s —— View job


Code Review

Nice refactor — moving off NavigationLink(destination:tag:selection:) and the Int-index model to a value-based NavigationStack is a clear improvement, and the WatchRoute/WatchRouteView split (destination registered once at the root so any deep screen can push any route) is exactly the right shape. Removing the hidden zero-size NavigationLink ZStack and the last WKInterfaceController usage is a good cleanup. I traced the launch, source-switch, restoration, and now-playing push paths against origin/trunk and the behavior looks preserved.

A few observations, all minor:

1. Dead re-entrancy guard + unused parameter in navigateToNowPlayingNavigationManager.swift:34-44

private var navigatingToNowPlaying = false
func navigateToNowPlaying(source: Source, fromLaunchEvent: Bool) {
    if navigatingToNowPlaying { return }
    navigatingToNowPlaying = true
    ...
    path = [.source(source), .interface(.nowPlaying)]
    navigatingToNowPlaying = false
}

Now that the body is fully synchronous, the flag is set and cleared within the same call, so it can never actually block re-entry — it's effectively dead. fromLaunchEvent is also unused inside the function (both callers pass true). Worth removing both to avoid implying a guarantee that isn't there. Fix this →

2. onChange(of: path.first) now also fires on programmatic path replacement — SourceInterfaceNavigationView.swift:147-156
The old side-effect trigger (onChange(of: activeSource)) only fired from a user tapping a source row. The new one keys off path.first, so navigateToNowPlaying(source:) — which replaces the whole path — will also fire phoneTapped()/watchTapped() whenever it changes the leading .source(...). I worked through the auto-launch case (handleRemoteNowPlayingActivitynavigateToNowPlaying(source: .phone) when the persisted source was .watch): it's benign because navigateToNowPlaying calls setSource(.phone) first, so by the time phoneTapped() runs, isWatch() is already false and the sync/notify branch is skipped. So no double side-effect today — but it's a slightly wider trigger surface than before and worth a comment noting the invariant, since a future change to phoneTapped/watchTapped that isn't idempotent would regress silently.

3. push(.episodeDetails) can push a blank destination — WatchRoute.swift:48-51 / NowPlayingOptions.swift:19
The old episode-details NavigationLink was only built if let episode = viewModel.episode. Now push(.episodeDetails) appends the route unconditionally, and WatchRouteView resolves the episode via PlaySourceHelper.playSourceViewModel.nowPlayingEpisode; if that's nil the if let produces an empty destination and you push a blank pushed screen. In practice this reads the same underlying nowPlayingEpisode that backs NowPlayingViewModel.episode, and the options page only appears when an episode is playing, so it's essentially unreachable — flagging only for robustness. Consider not appending the route when there's no episode.

4. @StateObject private var navigationModel = NavigationManager.sharedSourceInterfaceNavigationView.swift:57
Holding an externally-owned singleton in @StateObject is slightly unusual (@StateObject is meant to create/own the object). It's harmless here since the singleton lives forever and @StateObject avoids re-subscription churn — just noting it reads a bit against the grain; @ObservedObject is the more conventional fit for shared state.

Tests: NavigationManager is now pure path manipulation (navigateTo replaces, push appends, navigateToRestorable maps names→routes, WatchRoute.init? returns nil for .unknown/.interface/.filter). That's very unit-testable now and would lock in the "replace vs push" and no-op-route semantics cheaply — worth a small test given the checklist item is unchecked.

None of these are blockers. Restoration mapping (including .interface → no-op, staying on the seeded source list) and the double-push fix in NowPlayingRow/UpNextView both check out.
kean/watch-navigation-stack

@dangermattic

Copy link
Copy Markdown
Collaborator
1 Warning
⚠️ View files have been modified, but no screenshot or video is included in the pull request. Consider adding some for clarity.
1 Message
📖 This PR is still a Draft: some checks will be skipped.

Generated by 🚫 Danger

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

[Area] watchOS Issus related to watchOS refactoring Refactoring related issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants