Repository navigation
iOS: park notification-tap deep links until the workspace can be navigated to - #5927
Conversation
The tap is delivered before the root view binds a store; today it is dropped and the user lands on the workspaces home screen. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…gated to A cold launch delivers the UNUserNotificationCenter tap before the root view binds the store (the tap was dropped with reason=no_store), and a warm-but-detached app has not loaded the tapped workspace yet. The coordinator now parks the tap and applies it through one path from bind(store:) and a new workspacesDidChange() hook the root view drives from workspace-list changes, with a 120s expiry so a stale tap cannot yank the user away later. Fixes tapping a beta notification landing on the workspaces home screen instead of the workspace. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
@codex review |
📝 WalkthroughWalkthroughPush notification taps are parked when the mobile store, workspace list, or terminal snapshot is unavailable; parked taps are retried during store binding and on workspace topology changes, and resolved via a one-shot deeplink request on the store with expiry handling. ChangesPending notification deeplink lifecycle
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error)
✅ Passed checks (20 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 68fb6e5b3e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // absent ID would not navigate and a later list load could not | ||
| // re-trigger the push. | ||
| guard store.workspaces.contains(where: { $0.id == target }) else { return } | ||
| store.selectedWorkspaceID = target |
There was a problem hiding this comment.
In the cold-launch iPhone/compact flow this still leaves the user on the workspace list: applying the parked tap only assigns selectedWorkspaceID, but WorkspaceShellView reacts to selection changes by calling pathForSelectionChange, whose first branch returns the existing empty compactNavigationPath unchanged. Because cold launches start with an empty path, the selected workspace is updated in the store but no NavigationStack destination is pushed, so the notification tap continues to open at the workspaces home screen instead of the tapped workspace. The new deep-link path needs to drive the same compact navigation path used by row taps/create, or expose a navigation intent the view can consume.
Useful? React with 👍 / 👎.
Greptile SummaryFixes cold-launch and attach-in-flight notification taps that silently dropped deep links and landed users on the workspaces home screen. The approach parks the tap in
Confidence Score: 4/5Safe to merge; the parking-and-retry path is well-bounded and the previously flagged surface-only bypass is addressed. The 120s window remains a known residual that can overlap with deliberate user navigation during slow attaches. The core logic is correct: membership gates prevent navigating to absent workspaces, the expiry prevents stale yanks, and the one-shot token prevents the compact stack from replaying a push on remount. No new blocking defects were found in this review. MobilePushCoordinator.swift — contains the entire parking/retry logic; the partial-resolve path (workspace arrives before terminal) is the most intricate branch and worth an extra read-through. Important Files Changed
Sequence DiagramsequenceDiagram
participant APNs as UNNotificationCenter
participant Coord as MobilePushCoordinator
participant Root as CMUXMobileRootView
participant Store as CMUXMobileShellStore
participant Shell as WorkspaceShellView
APNs->>Coord: handleTap(workspaceId, surfaceId)
Coord->>Coord: park as pendingDeeplink
Coord->>Coord: applyPendingDeeplinkIfReady - no store, return
Root->>Root: onAppear
Root->>Coord: bind(store:)
Coord->>Coord: applyPendingDeeplinkIfReady - store bound, workspaces empty, return
Note over Store: Mac attach completes, workspaces array set
Store->>Store: workspaces.didSet bumps workspaceTopologyVersion
Store-->>Root: onChange(workspaceTopologyVersion)
Root->>Coord: workspacesDidChange()
Coord->>Coord: applyPendingDeeplinkIfReady - workspace found
alt workspace present, terminal present
Coord->>Store: navigateToWorkspaceForDeeplink(id)
Store->>Store: "selectedWorkspaceID = id, deeplinkWorkspaceNavigationRequest set"
Coord->>Store: selectTerminal(surfaceId)
Coord->>Coord: "pendingDeeplink = nil"
Store-->>Shell: onChange(deeplinkWorkspaceNavigationRequest)
Shell->>Store: consumeDeeplinkWorkspaceNavigationRequest()
Shell->>Shell: push compactNavigationPath to id
else workspace present, terminal missing
Coord->>Store: navigateToWorkspaceForDeeplink(id)
Coord->>Coord: park surface-only pendingDeeplink, same expiry
else workspace absent
Coord->>Coord: return, still parked until 120s expiry
end
Reviews (3): Last reviewed commit: "Move workspaceID(forTerminalID:) to the ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| guard let store else { return } | ||
| if let workspaceId = pending.workspaceId { | ||
| let target = MobileWorkspacePreview.ID(rawValue: workspaceId) | ||
| // Wait for the attach to deliver the workspace; selecting an | ||
| // absent ID would not navigate and a later list load could not | ||
| // re-trigger the push. | ||
| guard store.workspaces.contains(where: { $0.id == target }) else { return } | ||
| store.selectedWorkspaceID = target | ||
| } | ||
| if let surfaceId { | ||
| if let surfaceId = pending.surfaceId { | ||
| store.selectTerminal(MobileTerminalPreview.ID(rawValue: surfaceId)) | ||
| } | ||
| pendingDeeplink = nil |
There was a problem hiding this comment.
When a notification carries workspaceId = nil but a non-nil surfaceId, the function skips the workspace-membership guard entirely and immediately calls store.selectTerminal() on the first applyPendingDeeplinkIfReady() after bind(store:). If the workspace list (and its terminals) hasn't loaded yet at bind time, selectTerminal likely silently ignores the unknown ID — and then pendingDeeplink is set to nil, permanently discarding the intent with no retry path. On a cold launch the store binds before any workspace or terminal data arrives, so this is the same "stranded at home screen" failure the PR set out to fix, for any notification that omits workspaceId.
The workspace-membership guard should extend to cover the surface-only path, or the terminal selection should also be deferred behind a similar membership check once the workspace list is available.
| @ObservationIgnored private var pendingDeeplink: PendingDeeplink? | ||
| /// Bounded so a tap from long ago cannot yank the user out of whatever | ||
| /// they navigated to in the meantime, but generous enough to cover cold | ||
| /// launch plus sign-in plus a slow attach. | ||
| private static let pendingDeeplinkLifetime: TimeInterval = 120 | ||
| @ObservationIgnored private let now: () -> Date |
There was a problem hiding this comment.
The 120-second window is generous enough to cover cold launch + sign-in + a slow attach, which also means it's generous enough to cover a user who taps a notification, waits for the app to start, then manually navigates to a different workspace before attach completes. When attach delivers the workspace list and workspacesDidChange() fires, applyPendingDeeplinkIfReady() will override that manual selection with the notification target — silently yanking the user — as long as fewer than 120 seconds have elapsed.
The PR description notes this as a residual and assumes store.workspaces is always empty before attach. That invariant lives entirely in the store; the coordinator has no check on it. A more targeted guard — for example, discarding the pending deeplink if store.selectedWorkspaceID has changed since the tap landed — would close this class of regression without relying on an external invariant or the wall-clock expiry.
Rule Used: Flag Swift fixes that patch symptoms while leaving... (source)
| .onChange(of: store.workspaces.map(\.id)) { _, _ in | ||
| pushCoordinator.workspacesDidChange() | ||
| } |
There was a problem hiding this comment.
store.workspaces.map(\.id) allocates a new array on every body evaluation
SwiftUI re-evaluates body whenever any tracked @Observable property changes. Each evaluation runs store.workspaces.map(\.id), allocating a fresh [ID] array which is then compared element-by-element against the stored old value to decide whether to fire onChange. For the workspace counts expected on a phone this is negligible, but if the workspace list is accessed from several places in body, the map runs on every render — not just workspace-count changes. A lighter trigger would be to observe only the count (.onChange(of: store.workspaces.count)) if an ID-level change is never needed here without a count change, or to expose a stable collection-version counter from the store.
Rule Used: Flag SwiftUI changes that can cause stale state, b... (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@Packages/CmuxMobileShellUI/Sources/CmuxMobileShellUI/MobilePushCoordinator.swift`:
- Around line 190-201: The deeplink handling clears pendingDeeplink even when
the terminal (surfaceId) may not be present; change MobilePushCoordinator so
that after setting selectedWorkspaceID it checks that the target terminal
(MobileTerminalPreview.ID(rawValue: surfaceId)) actually exists in the currently
selected workspace before calling store.selectTerminal(...) and before clearing
pendingDeeplink; if the terminal is missing, do not clear pendingDeeplink (or
clear only the workspace portion and keep surfaceId) so the deeplink can be
retried when terminal snapshots arrive, and update the retry trigger logic in
CMUXMobileRootView.swift to listen to terminal-list/terminal ID changes in
addition to workspace ID changes so pending terminal resolution is retried.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 10ee89e9-4e8c-40ac-9734-286d57fe2a97
📒 Files selected for processing (3)
Packages/CmuxMobileShellUI/Sources/CmuxMobileShellUI/CMUXMobileRootView.swiftPackages/CmuxMobileShellUI/Sources/CmuxMobileShellUI/MobilePushCoordinator.swiftios/cmuxPackage/Tests/cmuxFeatureTests/cmuxFeatureTests.swift
Parking alone was not enough on iPhone: the compact shell's NavigationStack deliberately ignores selection changes while its path is empty (so attach-time auto-selection cannot yank the user off the home list), so a parked tap that resolved to selectedWorkspaceID = target still left the user on the workspaces home screen. A resolved deep link now emits a one-shot DeeplinkWorkspaceNavigationRequest the shell consumes exactly once to push the stack; the split layout consumes and discards it since selection already presents the detail column. Also resolves surface-only taps to the terminal's owning workspace, and when the workspace arrives before its terminal snapshot, navigates to the workspace immediately while keeping just the surface part parked under the same expiry. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Packages/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swift`:
- Around line 2328-2330: The deeplinkWorkspaceNavigationRequest stored on
MobileShellComposite is long-lived and must be cleared during session teardown
to avoid replaying stale deep-links; update teardown paths by setting
deeplinkWorkspaceNavigationRequest = nil in signOut() and
clearRemoteConnectionContext() (and any other methods that fully reset
selection/session state) so the one-shot request cannot be replayed for a new
session.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: f819aed0-b4ef-43fc-b3d9-2cb3f8ccaa8a
📒 Files selected for processing (6)
Packages/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite+DeeplinkNavigation.swiftPackages/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swiftPackages/CmuxMobileShellUI/Sources/CmuxMobileShellUI/CMUXMobileRootView.swiftPackages/CmuxMobileShellUI/Sources/CmuxMobileShellUI/MobilePushCoordinator.swiftPackages/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceShellView.swiftios/cmuxPackage/Tests/cmuxFeatureTests/cmuxFeatureTests.swift
| /// One-shot "actually navigate" deep-link intent; API in | ||
| /// `MobileShellComposite+DeeplinkNavigation.swift` (storage must live here). | ||
| public internal(set) var deeplinkWorkspaceNavigationRequest: DeeplinkWorkspaceNavigationRequest? |
There was a problem hiding this comment.
Clear stale deeplink requests on session teardown.
deeplinkWorkspaceNavigationRequest is new long-lived store state, but the teardown paths that reset selection/session state (signOut(), clearRemoteConnectionContext()) never nil it out. If a notification tap sets this request and the user signs out or switches accounts before the compact shell consumes it, the next remount can replay a stale push against the wrong session.
🧩 Minimal fix sketch
public func signOut() {
+ deeplinkWorkspaceNavigationRequest = nil
// Reset analytics identity to anonymous on the signed-in→signed-out edge
if isSignedIn {
analytics.identify(userId: nil, alias: nil, properties: [:])
@@
private func clearRemoteConnectionContext() {
+ deeplinkWorkspaceNavigationRequest = nil
connectionGeneration = UUID()
cancelRemoteOperationTasks()
clearActiveConnectionContext()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Packages/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swift`
around lines 2328 - 2330, The deeplinkWorkspaceNavigationRequest stored on
MobileShellComposite is long-lived and must be cleared during session teardown
to avoid replaying stale deep-links; update teardown paths by setting
deeplinkWorkspaceNavigationRequest = nil in signOut() and
clearRemoteConnectionContext() (and any other methods that fully reset
selection/session state) so the one-shot request cannot be replayed for a new
session.
…ngth budget MobileShellComposite.swift went 10 lines over its tracked budget; the surface-resolution helper now lives with its deep-link callers. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b493b1bb20
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| /// was dropped (`reason: no_store`) and the user landed on the workspaces | ||
| /// home screen. | ||
| @Test @MainActor func notificationTapBeforeStoreBindsNavigatesOnceWorkspacesLoad() async throws { | ||
| let coordinator = MobilePushCoordinator(registration: InertPushRegistration()) |
There was a problem hiding this comment.
When this package is tested for its declared macOS platform (ios/cmuxPackage/Package.swift includes .macOS(.v14)), MobilePushCoordinator is not available because its source file is compiled only under #if os(iOS). These unguarded tests therefore break a normal macOS swift test build as soon as the test target reaches this reference; wrap the new deep-link test block in an iOS-only conditional or move it to an iOS-only test target.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
Packages/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swift (1)
2327-2329:⚠️ Potential issue | 🟠 Major | ⚡ Quick winClear one-shot deeplink state during teardown paths.
Line 2329 introduces long-lived deeplink intent state, but
signOut()andclearRemoteConnectionContext()do not reset it. That allows stale deep-link navigation to be consumed after session/context transitions.🧩 Minimal fix sketch
public func signOut() { + deeplinkWorkspaceNavigationRequest = nil // Reset analytics identity to anonymous on the signed-in→signed-out edge if isSignedIn { analytics.identify(userId: nil, alias: nil, properties: [:]) @@ private func clearRemoteConnectionContext() { + deeplinkWorkspaceNavigationRequest = nil connectionGeneration = UUID() cancelRemoteOperationTasks() clearActiveConnectionContext()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Packages/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swift` around lines 2327 - 2329, The deeplinkWorkspaceNavigationRequest property holds one-shot deep-link intent but is not cleared during teardown; update signOut() and clearRemoteConnectionContext() (and any other session/teardown paths in MobileShellComposite) to reset deeplinkWorkspaceNavigationRequest = nil so stale navigation cannot be consumed after session/context transitions, and ensure the reset occurs before/alongside other teardown state changes to avoid race conditions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@Packages/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swift`:
- Around line 2327-2329: The deeplinkWorkspaceNavigationRequest property holds
one-shot deep-link intent but is not cleared during teardown; update signOut()
and clearRemoteConnectionContext() (and any other session/teardown paths in
MobileShellComposite) to reset deeplinkWorkspaceNavigationRequest = nil so stale
navigation cannot be consumed after session/context transitions, and ensure the
reset occurs before/alongside other teardown state changes to avoid race
conditions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 796cb918-41eb-4ac2-980e-e7dc8068d4b3
📒 Files selected for processing (2)
Packages/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite+DeeplinkNavigation.swiftPackages/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swift
… links, #5927) Composite keeps both the branch's workspaceTopologyVersion didSet signal and dog's notifications/groups stores. Push coordinator unions dog's mute state with the branch's PendingDeeplink parking, and takes the branch's parked-tap apply path (navigateToWorkspaceForDeeplink) over dog's immediate requestOpenWorkspace tap handling.
The tip moved workspaceID(forTerminalID:) into the deeplink extension; drops both stale inline copies (replay already lives in the smooth- scroll TerminalReplay extension) and the re-duplicated subscription start task property. Shell package green (157 tests).
Carries unlanded: multi-Mac #5545, notif-sync #5568, foreground-repaint #5571, image-paste toast #5572, hidden-input #5596, groups (iOS side) #5625, scroll-hysteresis, dogfood pane, capabilities superset. Main's reviewed forms win: TerminalController decomposition (Control*Context), notif-tap-deeplink #5927, mobile.terminal.* routing.
…#5596/#5625/#5628) over current main Beta queue (#5876/#5872/#5869/#5875/#5927/#5912/#5726/#5776/#5916) is now on main; conflicts resolved by taking main as authoritative for the merged workspace-list/notifications/read-state/close surface, while preserving the carry-set: terminal.paste capability (#5572), hidden-input strings (#5596), smooth-scroll/scroll-to-bottom (#5628), and the live notifications feed (notificationsStore + mobile.notifications.list/mark_read dispatch). Dropped the superseded mute design. Capability flags unified onto main's computed supportedHostCapabilities set (added computed supportsTerminalPaste + DEBUG supportsDogfoodChecklist). xcstrings merged (HEAD-precedence union, mute keys dropped). pbxproj took HEAD consistently; budget regenerated. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This branch was successfully deployed
1 participant
Fixes Lawrence's beta report: tapping a push notification opened the app at the workspaces home screen instead of the workspace the notification was about.
Root cause. On a cold launch
UNUserNotificationCenter'sdidReceivedelivers the tap before the root view has mounted, soMobilePushCoordinator.handleTaphit itsguard let storeand silently dropped the deep link (ios_push_deeplink_failed reason=no_store).bind(store:)only happens in the root view's.onAppear. Warm-app taps mostly worked, which made the bug look intermittent. Secondary gap: a tap while the Mac attach is still in flight targeted a workspace not yet present instore.workspaces.Fix. The tap is parked in the coordinator and applied through one path:
bind(store:)and a newworkspacesDidChange()hook the root view drives from workspace-list changes. Application is membership-gated (never select an absent workspace) and bounded by a 120s expiry (injectednow()seam) so a stale tap cannot yank the user away from what they navigated to in the meantime.Red/green. Commit 1c10afc13 adds the failing cold-start test (tap before bind must navigate once the store binds with its list); commit 68fb6e5b3 adds the fix plus coverage for tap-during-attach and expiry. Tests live in cmuxFeatureTests (Swift Testing, runs in the ios-simulator CI job — the honest one post #5906; CmuxMobileShellUI cannot
swift teston macOS due to the pre-existing platform-floor mismatch).iOS arm64 simulator build green (
/tmp/cmux-ios-ndl). No user-facing strings changed, so no localization changes (audit: diff adds code and doc comments only).Residual: navigation while attach is in flight briefly shows the default workspace before the parked tap applies on list arrival (one onChange turn); if the tapped workspace was deleted on the Mac, the tap expires quietly after 120s.
🤖 Generated with Claude Code
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Note
Medium Risk
Touches mobile navigation and push routing across cold launch and attach timing; behavior is well-tested but wrong gating could still mis-route or drop taps.
Overview
Notification taps are parked and applied when navigation is actually possible, instead of failing when no store is bound or the workspace list is still empty.
MobilePushCoordinatorqueues each tap and retries frombind(store:)andworkspacesDidChange()(driven by a newworkspaceTopologyVersionon workspace list mutations). Resolution is membership-gated (workspace must exist; surface-only taps resolve via owning workspace), can split workspace vs terminal when the workspace snapshot arrives before its terminals, and expires after 120s so stale taps do not hijack navigation later.For iPhone compact layout, selection alone no longer suffices: the store emits a one-shot
DeeplinkWorkspaceNavigationRequestthatWorkspaceShellViewconsumes to push theNavigationStack, matching the existing behavior that ignores selection while the path is empty.New Swift Testing coverage in
cmuxFeatureTestsexercises cold launch before bind, pre-attach, surface-only taps, workspace-before-terminal, compact navigation intent, and expiry.Reviewed by Cursor Bugbot for commit b493b1b. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Fixes iOS push notification deep links so a tap opens the correct workspace/terminal across cold launch and iPhone’s compact layout. Taps are parked and applied when ready; deep links now push the compact stack via a one‑shot intent, and stale taps expire after 120s.
Bug Fixes
bind(store:)andworkspacesDidChange().WorkspaceShellViewconsumes to push the compactNavigationStack.workspaceTopologyVersionchange signal.Refactors
workspaceID(forTerminalID:)into the deep‑link extension to keepMobileShellCompositewithin its size budget.Written for commit b493b1b. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes
Tests