Repository navigation
Notify iOS while using the app - #6654
austinywang wants to merge 30 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds end-to-end iOS notification settings synchronization with the Mac. New ChangesiOS Notification Settings Sync
Sequence Diagram(s)sequenceDiagram
actor User
participant MobileSettingsView
participant MobilePushCoordinator
participant MobileShellComposite
participant MobileCoreRPCClient
participant TerminalController as TerminalController (Mac)
rect rgba(100, 149, 237, 0.5)
Note over MobileSettingsView,TerminalController: Enable notifications + initial sync
User->>MobileSettingsView: toggle notifications on
MobileSettingsView->>MobilePushCoordinator: enable()
MobilePushCoordinator->>MobilePushCoordinator: persist MobileNotificationPreferences to UserDefaults
MobilePushCoordinator->>MobileShellComposite: syncNotificationPreferencesToMac(preferences)
MobileShellComposite->>MobileCoreRPCClient: notification.settings.set {enabled, mode, hide_content, client_id}
MobileCoreRPCClient->>TerminalController: notification.settings.set RPC
TerminalController->>TerminalController: write enabled/mode/hide_content to UserDefaults
TerminalController-->>MobileCoreRPCClient: {enabled, mode, hide_content} response
MobileCoreRPCClient-->>MobileShellComposite: MobileNotificationSettingsResponse
MobileShellComposite-->>MobilePushCoordinator: MobileNotificationPreferences (Mac-echoed)
MobilePushCoordinator->>MobilePushCoordinator: persist Mac-returned preferences to UserDefaults
MobilePushCoordinator-->>MobileSettingsView: updated MobileNotificationPreferences
MobileSettingsView->>MobileSettingsView: loadNotificationPreferences(prefs)
end
rect rgba(144, 238, 144, 0.5)
Note over MobileSettingsView,TerminalController: Change forwarding mode
User->>MobileSettingsView: pick forwarding mode
MobileSettingsView->>MobilePushCoordinator: setForwardingMode(mode)
MobilePushCoordinator->>MobileShellComposite: syncNotificationPreferencesToMac(preferences)
MobileShellComposite->>MobileCoreRPCClient: notification.settings.set {mode, ...}
MobileCoreRPCClient->>TerminalController: notification.settings.set RPC
TerminalController-->>MobileCoreRPCClient: updated payload
MobileCoreRPCClient-->>MobileShellComposite: MobileNotificationSettingsResponse
MobileShellComposite-->>MobilePushCoordinator: MobileNotificationPreferences
MobilePushCoordinator-->>MobileSettingsView: updated preferences
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (2 errors, 1 warning)
✅ Passed checks (20 passed)
✨ Finishing Touches🧪 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 |
Greptile SummaryThis PR adds iOS notification settings (enable/disable, Always vs Only When Away, Hide Content) synced to the Mac over new
Confidence Score: 5/5Safe to merge; the generation-based concurrency model and pending-sync retry mechanism are sound, @mainactor isolation is correct throughout, and all new user-facing strings are localized in both English and Japanese. The core write path (enable → write defaults → Mac RPC set → echo back → persist) is correctly guarded by the generation counter, always mutated synchronously before the first await on the @MainActor-isolated coordinator. The pending-sync mechanism survives disconnects, reconnects to different Macs, and Mac-side opt-outs. The two issues found are edge-case gaps: reconcileTask?.cancel() not stopping an in-flight reconcile (the generation mechanism is the real guard), and a pending-sync flag that can linger when the phone is locally disabled via a public API path that bypasses disable(). Both are bounded to settings sync state and self-correct on the next explicit user action. MobilePushCoordinator.swift — the reconcileTask cancellation semantics and the pending-sync-not-cleared-when-disabled branch. Important Files Changed
Reviews (13): Last reviewed commit: "Scope pending iOS notification settings ..." | Re-trigger Greptile |
| .onChange(of: notificationMode) { mode in | ||
| guard !notificationSettingsSyncing else { return } | ||
| updateNotificationMode(mode) | ||
| } | ||
| .accessibilityIdentifier("MobileSettingsNotificationsMode") | ||
| .disabled(notificationSettingsSyncing) | ||
|
|
||
| Toggle(isOn: $hideNotificationContent) { | ||
| Text(L10n.string( | ||
| "mobile.notifications.hideContent", | ||
| defaultValue: "Hide Notification Content" | ||
| )) | ||
| } | ||
| .onChange(of: hideNotificationContent) { hidesContent in | ||
| guard !notificationSettingsSyncing else { return } | ||
| updateNotificationHideContent(hidesContent) | ||
| } |
There was a problem hiding this comment.
Both
.onChange(of:) callbacks use the single-argument closure form that was deprecated in iOS 17. Since MobilePushCoordinator is @Observable (iOS 17+), the minimum deployment target makes these deprecated API calls, and Xcode will emit warnings. The fix is the two-argument { _, new in } form.
| .onChange(of: notificationMode) { mode in | |
| guard !notificationSettingsSyncing else { return } | |
| updateNotificationMode(mode) | |
| } | |
| .accessibilityIdentifier("MobileSettingsNotificationsMode") | |
| .disabled(notificationSettingsSyncing) | |
| Toggle(isOn: $hideNotificationContent) { | |
| Text(L10n.string( | |
| "mobile.notifications.hideContent", | |
| defaultValue: "Hide Notification Content" | |
| )) | |
| } | |
| .onChange(of: hideNotificationContent) { hidesContent in | |
| guard !notificationSettingsSyncing else { return } | |
| updateNotificationHideContent(hidesContent) | |
| } | |
| .onChange(of: notificationMode) { _, mode in | |
| guard !notificationSettingsSyncing else { return } | |
| updateNotificationMode(mode) | |
| } | |
| .accessibilityIdentifier("MobileSettingsNotificationsMode") | |
| .disabled(notificationSettingsSyncing) | |
| Toggle(isOn: $hideNotificationContent) { | |
| Text(L10n.string( | |
| "mobile.notifications.hideContent", | |
| defaultValue: "Hide Notification Content" | |
| )) | |
| } | |
| .onChange(of: hideNotificationContent) { _, hidesContent in | |
| guard !notificationSettingsSyncing else { return } | |
| updateNotificationHideContent(hidesContent) | |
| } |
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!
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 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 `@cmuxTests/PhonePushPresenceGateTests.swift`:
- Around line 325-335: The PhonePushPresenceGateTests struct modifies shared
global state (UserDefaults.standard and TerminalController.shared) without
serialization, which can cause race conditions when tests run in parallel. Add
the .serialized trait to the PhonePushPresenceGateTests struct declaration to
ensure tests in this suite execute serially and prevent interleaved mutations of
shared state.
In
`@Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite`+NotificationSettingsSync.swift:
- Line 43: The notificationSettingsLog.error calls at lines 43 and 67 are
logging error descriptions with `.public` privacy redaction, which can expose
sensitive authentication or runtime details in production logs. Change the
privacy parameter from `.public` to `.private` for the String(describing: error)
portion in both notificationSettingsLog.error calls to properly redact sensitive
error information according to coding guidelines.
- Around line 6-9: The file-scoped logger instance notificationSettingsLog needs
to be declared as nonisolated to avoid unnecessary MainActor coupling in Swift 6
isolation mode. Add the nonisolated keyword before the private let declaration
of notificationSettingsLog so it reads nonisolated private let
notificationSettingsLog instead of just private let notificationSettingsLog.
This explicitly marks the logger as not being actor-isolated, allowing it to be
safely accessed from both actor and non-actor contexts.
In
`@Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/MobilePushCoordinator.swift`:
- Around line 214-217: The issue is that the
reconcileNotificationPreferencesWithMac() method uses a single
hasStoredNotificationPreference boolean flag to determine if local preferences
should be synced to Mac, but this doesn't account for partial preference data
where some fields may be missing or only have default values. On upgrade paths
with incomplete stored data, missing mode/hide fields get defaulted and pushed
to Mac, overwriting its authoritative settings. Instead of relying solely on the
hasStoredNotificationPreference flag, implement explicit validation to check
that all required notification preference fields (enabled, mode, hide_content,
forwarding behavior) are fully present and valid before syncing to Mac. If any
required fields are missing or only have defaults, treat the local cache as
stale and skip the sync, allowing the Mac settings to remain authoritative. This
aligns with the cache-substitution-correctness requirements for explicit
cold/stale cache handling.
- Around line 109-114: The Task created in the bind method for
reconcileNotificationPreferencesWithMac is a fire-and-forget task that is not
tracked or managed, which can cause overlapping task execution if bind is called
multiple times before the previous task completes. Store the reconciliation task
as a property of the MobilePushCoordinator class, and in the bind method, cancel
any existing stored task before starting the new reconciliation task. This
ensures that only one reconciliation task runs at a time and prevents stale
preferences from being persisted due to out-of-order task completion.
In
`@Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/MobileSettingsView.swift`:
- Around line 405-447: The notificationSettingsSyncing flag is being set to true
inside the Task block rather than before it, which creates a race condition
where rapid events can queue multiple async tasks before the gate is
established. Move the notificationSettingsSyncing = true statement outside and
before each Task block initialization in the four methods:
refreshNotificationPreferencesFromMac, toggleNotifications,
updateNotificationMode, and updateNotificationHideContent. This ensures the
synchronization flag is set immediately when the method is called, preventing
overlapping writes and out-of-order preference application.
In `@Sources/TerminalController`+MobileNotificationSync.swift:
- Around line 20-43: The code is persisting values to UserDefaults as each
parameter is validated individually, which means if an early parameter is valid
but a later one is invalid, the earlier value is already persisted when the
error is returned. Refactor the logic to first validate all parameters (enabled
via v2Bool, mode via v2OptionalTrimmedRawString and PhoneForwardingMode
initialization, and hide_content via v2Bool) and collect the valid values, then
only after all validations pass, persist all values to UserDefaults using the
keys PhonePushSettings.forwardEnabledKey, PhonePushSettings.forwardModeKey, and
PhonePushSettings.hideContentKey. This ensures atomicity—either all changes are
persisted or none are.
🪄 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: 87b5d38d-36c4-4015-82c9-73acd912b936
📒 Files selected for processing (17)
Packages/iOS/CmuxMobileRPC/Sources/CmuxMobileRPC/MobileCoreRPCClient.swiftPackages/iOS/CmuxMobileRPC/Sources/CmuxMobileRPC/MobileNotificationSettingsResponse.swiftPackages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite+NotificationSettingsSync.swiftPackages/iOS/CmuxMobileShellModel/Sources/CmuxMobileShellModel/MobileNotificationForwardingMode.swiftPackages/iOS/CmuxMobileShellModel/Sources/CmuxMobileShellModel/MobileNotificationPreferences.swiftPackages/iOS/CmuxMobileShellModel/Tests/CmuxMobileShellModelTests/MobileNotificationPreferencesTests.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/MobilePushCoordinator.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/MobileSettingsView.swiftSources/Cloud/PhoneForwardingMode.swiftSources/Cloud/PhonePushClient.swiftSources/Mobile/MobileHostService+Capabilities.swiftSources/Mobile/MobileHostService.swiftSources/TerminalController+MobileNotificationSync.swiftSources/TerminalController.swiftcmuxTests/PhonePushPresenceGateTests.swiftios/cmux/Resources/Localizable.xcstringsios/cmuxPackage/Tests/cmuxFeatureTests/cmuxFeatureTests.swift
There was a problem hiding this comment.
♻️ Duplicate comments (1)
Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/MobilePushCoordinator.swift (1)
230-233: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winPersist reconciled Mac values in the cold-cache branch.
This branch returns Mac-derived
forwardingMode/hidesContentbut does not persist them, so subsequent reads fromnotificationPreferencescan still serve stale defaults after reconciliation. Persist the reconciled non-opt-in fields before returning to make cold-start reconciliation durable.As per path instructions, cache substitution paths must explicitly handle cold/stale cache behavior rather than leaving authoritative reads ephemeral.
🤖 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/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/MobilePushCoordinator.swift` around lines 230 - 233, The localPreferences variable is modified with Mac-derived values for forwardingMode and hidesContent but is not persisted before being returned. This causes subsequent reads from notificationPreferences to serve stale defaults. After assigning the reconciled Mac values to localPreferences (both forwardingMode and hidesContent), persist these changes to the underlying cache or storage mechanism before returning localPreferences so that cold-start reconciliation is durable and future reads reflect the reconciled state.Source: Path instructions
🤖 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/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/MobilePushCoordinator.swift`:
- Around line 230-233: The localPreferences variable is modified with
Mac-derived values for forwardingMode and hidesContent but is not persisted
before being returned. This causes subsequent reads from notificationPreferences
to serve stale defaults. After assigning the reconciled Mac values to
localPreferences (both forwardingMode and hidesContent), persist these changes
to the underlying cache or storage mechanism before returning localPreferences
so that cold-start reconciliation is durable and future reads reflect the
reconciled state.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 47782889-0144-4ef2-a9e4-b923a5f4c1da
📒 Files selected for processing (7)
Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/MobilePushCoordinator.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/MobileSettingsView.swiftSources/Mobile/MobileHostService.swiftSources/TerminalController+MobileNotificationSync.swiftSources/TerminalController.swiftcmuxTests/PhonePushPresenceGateTests.swiftios/cmuxPackage/Tests/cmuxFeatureTests/cmuxFeatureTests.swift
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/MobilePushCoordinator.swift (1)
240-246: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winPersist the Mac settings in the cold-cache reconciliation path.
When no local opt-in key exists, this fetches Mac settings but leaves
isEnabledas the local default and never writes the fetched values todefaults; thebind(store:)caller ignores the return value, so first-run devices can drop the authoritative Mac settings and later overwrite them from local defaults.💡 Suggested fix
guard let macPreferences = await store?.fetchNotificationPreferencesFromMac() else { return notificationPreferences } - var localPreferences = notificationPreferences - localPreferences.forwardingMode = macPreferences.forwardingMode - localPreferences.hidesContent = macPreferences.hidesContent - return localPreferences + macPreferences.persist(to: defaults) + return macPreferencesAs per path instructions,
.github/review-bot-rules/cache-substitution-correctness.mdrequires explicit cold-cache handling when substituting cached/local values for authoritative settings.🤖 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/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/MobilePushCoordinator.swift` around lines 240 - 246, In the cold-cache reconciliation path where fetchNotificationPreferencesFromMac() is called, the code currently only copies forwardingMode and hidesContent from macPreferences to localPreferences while leaving isEnabled as the local default, and never persists the merged values back to defaults. This causes first-run devices to lose the authoritative Mac settings. Fix this by ensuring all properties from macPreferences (including the missing isEnabled property) are copied to localPreferences, and then persist the complete merged localPreferences back to the defaults cache before returning, so the authoritative Mac settings are preserved and not overwritten by local defaults on subsequent app launches.Source: Path instructions
🤖 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/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/MobilePushCoordinator.swift`:
- Around line 387-397: The three helper properties hasStoredNotificationOptIn,
hasStoredForwardingModePreference, and hasStoredHideContentPreference only check
for key presence in defaults without validating the actual stored values. This
allows stale or invalid values to be treated as authoritative, causing invalid
fallback values in notificationPreferences to potentially sync back to the Mac
and overwrite valid Mac settings. Modify each helper to validate not only that
the key exists but also that the stored value is of the correct type and
represents a valid state before returning true, ensuring stale or malformed
cached values are rejected.
---
Outside diff comments:
In
`@Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/MobilePushCoordinator.swift`:
- Around line 240-246: In the cold-cache reconciliation path where
fetchNotificationPreferencesFromMac() is called, the code currently only copies
forwardingMode and hidesContent from macPreferences to localPreferences while
leaving isEnabled as the local default, and never persists the merged values
back to defaults. This causes first-run devices to lose the authoritative Mac
settings. Fix this by ensuring all properties from macPreferences (including the
missing isEnabled property) are copied to localPreferences, and then persist the
complete merged localPreferences back to the defaults cache before returning, so
the authoritative Mac settings are preserved and not overwritten by local
defaults on subsequent app launches.
🪄 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: 249fdb23-008f-450d-879b-9358e2bdeecf
📒 Files selected for processing (2)
Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/MobilePushCoordinator.swiftios/cmuxPackage/Tests/cmuxFeatureTests/cmuxFeatureTests.swift
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/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/MobileSettingsView.swift`:
- Line 235: The toggle at line 235 is being disabled whenever
supportsNotificationSettings is false, and the handler at line 432 returns early
in the same condition, which prevents users from disabling notifications on
older Macs. The restriction should only apply to enabling or syncing settings,
not disabling. Modify the disabled condition at line 235 to only restrict when
the notification toggle would be switching from off to on (or when sync is
needed), and update the early return at line 432 to only apply when the user is
trying to enable notifications with pushCoordinator.enable(), allowing the
pushCoordinator.disable() path to proceed unconditionally since it only updates
local state and does not require Mac settings sync support.
🪄 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: 86d21070-3aa8-4131-9f21-b2906623b119
📒 Files selected for processing (6)
Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite+NotificationSettingsSync.swiftPackages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swiftPackages/iOS/CmuxMobileShell/Tests/CmuxMobileShellTests/NotificationSettingsCapabilityTests.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/MobileSettingsView.swiftios/cmux/Resources/Localizable.xcstringsios/cmuxPackage/Tests/cmuxFeatureTests/cmuxFeatureTests.swift
…-while-using-app # Conflicts: # .github/swift-file-length-budget.tsv
…-while-using-app # Conflicts: # .github/swift-file-length-budget.tsv
…-while-using-app # Conflicts: # .github/swift-file-length-budget.tsv # ios/cmux/Resources/Localizable.xcstrings
Summary
notification.settings.get/setmobile RPCs backed by the same MacPhonePushSettingsdefaults that power the Mac Notifications page.Behavior
When the user enables agent notifications on iOS, the phone persists the local APNs opt-in and syncs the forwarding preference to the paired Mac. The iOS default mode is Always, so completed-workspace notifications are forwarded to the phone even if the Mac was awake or recently used. Users can still choose Only When Away from Mac from the iOS settings, and the settings view explains that this mode can be suppressed by Mac presence.
The Mac Notifications page and iOS settings share the Mac forwarding mode and hide-content preference. The iOS local APNs opt-in is stored separately from the Mac forwarding-enabled mirror: passive iOS reconciliation is read-only toward the Mac, so a later Mac-side forwarding opt-out or privacy change is not overwritten by an old phone opt-in, and it does not silently unregister the phone. Explicit iOS enable/mode/privacy actions remain the write path, and failed explicit writes are marked pending so reconnect can retry. Turning notifications off on one phone remains device-scoped and does not disable Mac forwarding or another device.
Foreground presentation remains intentionally narrow: iOS suppresses a banner only for the exact workspace/terminal currently on screen, and still presents foreground banners for other completed workspaces/terminals. Background delivery continues through APNs; real device/TestFlight validation is required for end-to-end phone-push delivery.
Tests
notification.settings.get/set, capability gating, passive read-only reconcile, Mac-side opt-out handling, and pending explicit-sync retry.git diff --check,jq empty ios/cmux/Resources/Localizable.xcstrings, and localization key audit for English/Japanese notification strings.Closes #6636