Support for push notifications#68
Conversation
Add the pushNotificationIOS feature flag, UserDefaults keys for token / asked / per-content-type prefs (gated to non-Catalyst non-visionOS), and the aps-environment entitlement so the app target can register with APNs. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The manager owns APNs token capture, server registration, the rationale dialog + permission flow, and notification-tap routing through the existing newURLOpenedNotificationName pipeline. AppDelegate plumbs APNs callbacks and acts as the UNUserNotificationCenterDelegate. MainViewController kicks off setup once the news check completes (matching the Android sequence) and re-registers when an article ack updates lastNewsID. All push notification code is gated to iOS-only (no visionOS, no macCatalyst) and behind the pushNotificationIOS feature flag at every entry point. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Wire a SwiftUI screen with three toggles (weekly add-on, featured add-on, latest news) and a denied-state hint that links to the system Settings app. The screen pushes saved prefs to the server through a PushNotificationSettingContext attached after SettingsCoordinatorController construction (separate setter, since Swift doesn't allow #if inside parameter lists). Also switch the UNUserNotificationCenterDelegate methods to their async overloads with nonisolated, extracting the article-id / addon-id strings before the actor hop so we don't need to send the non-Sendable response across. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Replace the post-init setPushNotificationContext setter with a separate public initializer on SettingsCoordinatorController that's only available when push notifications can run (iOS-only, non-visionOS). The Catalyst init keeps its original signature. Add CelestiaNotificationService/NotificationService.swift, the source for the Notification Service Extension. It downloads image-url attachments before iOS displays the alert, falling back to the original payload on error or timeout. Wiring this up requires creating the matching target in Xcode (entitlements, code signing, NSExtension info.plist). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Drop the Catalyst guards so PushNotificationManager, PushNotificationSettingContext, the SwiftUI screen, and the notifications setting case all compile on iOS and Catalyst alike. visionOS continues to be excluded. Collapse the iOS-only SettingsCoordinatorController initializer back into the shared non-visionOS init by accepting an optional PushNotificationSettingContext (nil when the feature flag is off); runtime activation is gated by the server-managed pushNotificationIOS flag, which the server never grants for catalyst clients. MainViewController now stores the pushManager passed in by MainSceneDelegate instead of dynamically casting through AppDelegate each time, and its showSettings collapses to a single construction path. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Catalyst should report platform="catalyst" instead of "ios", matching the rest of the codebase's platform-string convention so server-side flags / queries see the same value. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The MobileCelestia target only builds for iOS and Catalyst, so the file-level and per-block #if !os(visionOS) guards on the push notification code path were unreachable. Drop them and switch the bits that should not run on Catalyst — UNUserNotificationCenterDelegate extension, delegate registration, didRegister/didFail callbacks — to #if !targetEnvironment(macCatalyst). Also make PushNotificationContentType + the UserDefaults pref helpers file-private (only the manager uses them) and require the pushNotificationContext parameter on the iOS-side SettingsCoordinatorController init since it's always passed now. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Every call site passes a real PushNotificationSettingContext now, so drop the Optional wrapping on the parameter, the stored property, and the saveHandler call site. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Replace AppURL.windowURL's universal Bool with a WindowURLSource enum (urlScheme / universalLink / pushNotification). When a guide URL is opened from a push, install a stronger actionHandler that sets lastNewsID and re-registers on ack — matching the in-app news flow — instead of writing lastNewsID synchronously when the notification is tapped. The dedicated source also makes future per-source behavior straightforward. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Drop the Xcode-template overrides on the NSE target that diverged from the rest of the project (IPHONEOS_DEPLOYMENT_TARGET = 26.4, SWIFT_VERSION 5.0, GENERATE_INFOPLIST_FILE alongside an existing Info.plist, etc.) and let the project-level settings take over. Use $(SHARED_BUILD_NUMBER) / $(SHARED_BUILD_VERSION) for versions, set SUPPORTS_MACCATALYST = NO and SUPPORTS_MAC_DESIGNED_FOR_IPHONE_IPAD = NO since push notifications are iOS-only here. Replace the template NotificationService body with the image-attachment-downloading implementation that reads "image-url" from the payload userInfo, attaches the downloaded file, and falls back to the original content on failure or timeout. Drop the earlier CelestiaNotificationService/ stub now that the real target lives at NotificationServiceExtension/. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Replace the inline MainActor.run { ... } with a dedicated @mainactor
method. The actor hop happens on the method call, so there's no
closure being sent across regions and Swift 6's region-based
isolation analysis is happy.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The Task { ... } closure was capturing the non-Sendable
UNMutableNotificationContent + the contentHandler closure, which Swift
6's region-based isolation flags as a possible data race. Use
URLSession.downloadTask with its plain completion handler instead —
non-Sendable, so no region warning, and matches the typical NSE
template.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
UNMutableNotificationContent and the (UNNotificationContent) -> Void contentHandler aren't marked Sendable in the SDK yet, so the @sendable URLSession completion handler can't capture them under strict concurrency. Treat the UserNotifications Sendable diagnostics as warnings — the captures are safe in practice (NSE has a single contentHandler invocation and runs in its own process). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Mark NotificationService @unchecked Sendable and have the URLSession.downloadTask completion handler hop through a [weak self] helper instead of capturing the local contentHandler closure directly. The non-Sendable contentHandler / bestAttemptContent live on self, so the @sendable closure only captures the (Sendable) class reference. Drop the @preconcurrency import since it's no longer needed. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Revert the @unchecked Sendable refactor and instead unsafeBitCast the contentHandler to a @sendable variant before using it inside the URLSession completion handler. The bestAttemptContent capture is covered by @preconcurrency import UserNotifications. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Without GENERATE_INFOPLIST_FILE, Xcode doesn't inject CFBundleIdentifier etc. for us, which is why the embedded binary's bundle identifier was null. Reference the build settings explicitly so the NSE bundle identifier ends up prefixed by the parent app's identifier. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Add a separate IOS_NSE_PROVISIONING_PROFILE_BASE64 secret, install the profile alongside the main one, and patch the per-target PROVISIONING_PROFILE_SPECIFIER in pbxproj via awk so the NSE configuration gets its own UUID instead of the parent app's. Also list the NSE bundle identifier in the iOS App Store export options plist. Setup outside this commit: - Apple Developer portal: register App ID space.celestia.MobileCelestia.NotificationServiceExtension and generate a matching iOS distribution provisioning profile. - GitHub repo: add IOS_NSE_PROVISIONING_PROFILE_BASE64 secret with the base64-encoded .mobileprovision. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The screen's @State status defaulted to .notDetermined and the first .task fetch from UNUserNotificationCenter would look like a fresh .notDetermined → .authorized transition for users who had already granted permission. That spuriously kicked off registerForRemoteNotifications + onSave on every screen open. Track hasObservedStatus and only react to actual transitions after the first observation. The "user returned from system Settings" re-register path still fires via the didBecomeActive observer. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
If the user opens the Notifications settings screen and the OS-level authorization is still .notDetermined (e.g. they dismissed the rationale dialog earlier without ever seeing the system prompt), surface the system prompt here so they don't have to bounce through system Settings. Re-register on grant. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
When the user finishes reading an article (either the in-app news flow or one opened from a push notification tap), find any delivered notifications whose userInfo's article-id matches and remove them via UNUserNotificationCenter.removeDeliveredNotifications. Mirrors the behaviour we wired up on Android. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Mark UNUserNotificationCenter / UNNotificationResponse / UNNotification as @unchecked @retroactive Sendable so they cross isolation cleanly. The delegate methods can then be @mainactor async and dispatch the tap through Task.detached on MainActor without needing the nonisolated + helper-method shuffle. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Replaces the @preconcurrency import + URLSession completion-handler detour: the retroactive @unchecked Sendable conformance lets the async Task capture bestAttemptContent without diagnostics, while still unsafeBitCasting the contentHandler to its @sendable variant. The body is back to async/await style. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Calling registerForRemoteNotifications eventually fires didRegisterForRemoteNotificationsWithDeviceToken on the AppDelegate, which routes through PushNotificationManager.didReceiveDeviceToken and already calls register(). The explicit onSave() after registerForRemoteNotifications was just duplicating that server request. The Save button still calls onSave() directly because that path doesn't go through APNs. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The local LAN URL (http://10.20.7.16:9000/...) was meant to stay in the working tree during testing but got bundled into 3f97b92. Restore the production URL.apiPrefixURL-based path on the branch. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The method is already @mainactor, so pushManager.handleTap can be called directly — the detached Task was a no-op trip through the scheduler. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Insert it just before the trailing celestiaPlusSettings + miscSettings entries instead of appending at the very end, so the order on screen matches: ... → Notifications → Celestia PLUS → Misc. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Convert the @mainactor let to func mainSetting(featureFlags:) so the notifications section can be inserted at the right slot — between advancedSettings and the trailing Celestia PLUS + misc sections — without the caller calculating settings.count - 2. The Catalyst / feature-flag gate now lives next to the insertion. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Adds iOS push notification support (including a Notification Service Extension for rich notifications) and exposes notification preferences in the app’s Settings, gated behind a feature flag.
Changes:
- Add a
UNNotificationServiceExtensionto download and attach remote images to notifications. - Introduce
PushNotificationManagerto handle authorization prompting, APNs token registration, tap handling, and server-side registration. - Add a Notifications settings entry + SwiftUI settings screen to configure content-type subscriptions (weekly add-on / featured add-on / latest news).
Reviewed changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| NotificationServiceExtension/NotificationService.swift | Implements rich notification attachment download from image-url. |
| NotificationServiceExtension/Info.plist | Declares NSExtension metadata for the notification service extension target. |
| MobileCelestia/Settings/SettingsModel.swift | Adds a Notifications section to Settings (feature-flagged). |
| MobileCelestia/PushNotification/PushNotificationManager.swift | Adds push registration, opt-in prompting, tap routing, and delivered-notification cleanup. |
| MobileCelestia/MainViewController.swift | Wires push manager into app flow; adds hooks to re-register when user acks news / saves settings. |
| MobileCelestia/MainSceneDelegate.swift | Injects PushNotificationManager into MainViewController. |
| MobileCelestia/Celestia.entitlements | Adds aps-environment entitlement. |
| MobileCelestia/AppDelegate.swift | Sets up notification center delegate + forwards APNs token callbacks to push manager. |
| MobileCelestia.xcodeproj/project.pbxproj | Adds the new Notification Service Extension target and embeds it in the iOS app. |
| ExportOptions-iphoneos-appStore.plist | Adds provisioning profile entry for the notification service extension bundle id. |
| CelestiaUI/Settings/SettingsModel.swift | Adds .notifications to OtherSettingType. |
| CelestiaUI/Settings/SettingsCoordinatorController.swift | Adds PushNotificationSettingContext and routes .notifications to the new settings screen. |
| CelestiaUI/Settings/PushNotificationSettingsScreen.swift | SwiftUI screen to request permission and manage content-type toggles. |
| CelestiaUI/Addon/Models/FeatureFlagsManager.swift | Adds pushNotificationIOS to remote feature flags. |
| CelestiaUI/Addon/Models/FeatureFlags.swift | Adds pushNotificationIOS field to FeatureFlags. |
| CelestiaFoundation/UserDefaults.swift | Adds new UserDefaults keys for push state/preferences. |
| CelestiaFoundation/URLHandler.swift | Replaces universal: Bool with source: WindowURLSource and adds .pushNotification source. |
| .github/workflows/build.yml | Adds CI handling for the extension provisioning profile and pbxproj provisioning injection. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| override func didReceive(_ request: UNNotificationRequest, withContentHandler contentHandler: @escaping (UNNotificationContent) -> Void) { | ||
| self.contentHandler = contentHandler | ||
| let bestAttemptContent = (request.content.mutableCopy() as? UNMutableNotificationContent) ?? UNMutableNotificationContent() | ||
| self.bestAttemptContent = bestAttemptContent | ||
|
|
||
| guard let urlString = bestAttemptContent.userInfo["image-url"] as? String, | ||
| let url = URL(string: urlString) else { | ||
| contentHandler(bestAttemptContent) | ||
| return | ||
| } | ||
|
|
||
| let sendableContentHandler = unsafeBitCast(contentHandler, to: (@Sendable (UNNotificationContent) -> Void).self) | ||
| Task { | ||
| do { | ||
| let (tempURL, response) = try await URLSession.shared.download(from: url) | ||
| let suggestedExtension = (response.suggestedFilename as NSString?)?.pathExtension | ||
| let pathExtension = (suggestedExtension?.isEmpty == false ? suggestedExtension : url.pathExtension) ?? "tmp" | ||
| let destination = tempURL.deletingLastPathComponent().appendingPathComponent("\(UUID().uuidString).\(pathExtension)") | ||
| try FileManager.default.moveItem(at: tempURL, to: destination) | ||
| let attachment = try UNNotificationAttachment(identifier: "", url: destination) | ||
| bestAttemptContent.attachments = [attachment] | ||
| } catch {} | ||
| sendableContentHandler(bestAttemptContent) | ||
| } | ||
| } | ||
|
|
||
| override func serviceExtensionTimeWillExpire() { | ||
| // Apple gives the extension a finite window (~30s) before falling back | ||
| // to the original payload — emit whatever we have. | ||
| if let contentHandler, let bestAttemptContent { | ||
| contentHandler(bestAttemptContent) | ||
| } |
There was a problem hiding this comment.
didReceive can end up invoking the notification contentHandler twice: once in serviceExtensionTimeWillExpire() and again when the async Task completes (or even when the early guard path calls contentHandler(bestAttemptContent)). Apple expects the handler to be called at most once; double-calling can lead to undefined behavior / dropped notifications. Consider nil-ing out contentHandler after the first invocation (and guarding in the Task before calling), and/or storing/canceling the download Task when serviceExtensionTimeWillExpire fires.
There was a problem hiding this comment.
Apple's NSE template (https://developer.apple.com/documentation/usernotifications/modifying-content-in-newly-delivered-notifications) does the same — stores contentHandler + bestAttemptContent as ivars and calls them from both paths without nil-ing after the first call. The system only invokes serviceExtensionTimeWillExpire() when the extension hasn't finished yet, so once we call contentHandler from the download path, timeWillExpire won't fire afterward.
| extension UNMutableNotificationContent: @retroactive @unchecked Sendable {} | ||
|
|
||
| class NotificationService: UNNotificationServiceExtension { | ||
| private var contentHandler: ((UNNotificationContent) -> Void)? | ||
| private var bestAttemptContent: UNMutableNotificationContent? | ||
|
|
||
| override func didReceive(_ request: UNNotificationRequest, withContentHandler contentHandler: @escaping (UNNotificationContent) -> Void) { | ||
| self.contentHandler = contentHandler | ||
| let bestAttemptContent = (request.content.mutableCopy() as? UNMutableNotificationContent) ?? UNMutableNotificationContent() | ||
| self.bestAttemptContent = bestAttemptContent | ||
|
|
||
| guard let urlString = bestAttemptContent.userInfo["image-url"] as? String, | ||
| let url = URL(string: urlString) else { | ||
| contentHandler(bestAttemptContent) | ||
| return | ||
| } | ||
|
|
||
| let sendableContentHandler = unsafeBitCast(contentHandler, to: (@Sendable (UNNotificationContent) -> Void).self) | ||
| Task { | ||
| do { |
There was a problem hiding this comment.
This uses unsafeBitCast to coerce contentHandler into a @Sendable closure and adds @retroactive @unchecked Sendable conformances to UNMutableNotificationContent. This bypasses Swift concurrency safety and can hide real data races. Prefer avoiding Task {} here (use URLSession completion-handler APIs) or restructure so no non-Sendable values/closures are captured by a @Sendable task operation.
There was a problem hiding this comment.
Acknowledged trade-off. We tried the URLSession.shared.downloadTask(with:completionHandler:) route earlier in this branch but it ran into the same @Sendable capture diagnostics on bestAttemptContent / contentHandler (Foundation marks the completion handler @Sendable). The current pattern — unsafeBitCast + retroactive @unchecked Sendable on UNMutableNotificationContent — is the smallest workaround that lets us keep the async/await body. We can revisit once Apple ships proper Sendable annotations on UserNotifications.
| guard let urlString = bestAttemptContent.userInfo["image-url"] as? String, | ||
| let url = URL(string: urlString) else { | ||
| contentHandler(bestAttemptContent) | ||
| return | ||
| } | ||
|
|
||
| let sendableContentHandler = unsafeBitCast(contentHandler, to: (@Sendable (UNNotificationContent) -> Void).self) | ||
| Task { | ||
| do { | ||
| let (tempURL, response) = try await URLSession.shared.download(from: url) | ||
| let suggestedExtension = (response.suggestedFilename as NSString?)?.pathExtension | ||
| let pathExtension = (suggestedExtension?.isEmpty == false ? suggestedExtension : url.pathExtension) ?? "tmp" | ||
| let destination = tempURL.deletingLastPathComponent().appendingPathComponent("\(UUID().uuidString).\(pathExtension)") | ||
| try FileManager.default.moveItem(at: tempURL, to: destination) | ||
| let attachment = try UNNotificationAttachment(identifier: "", url: destination) | ||
| bestAttemptContent.attachments = [attachment] |
There was a problem hiding this comment.
image-url is downloaded directly from the push payload without any validation (scheme/host) or resource limits. This allows your push sender (or a compromised sender) to trigger downloads of arbitrary/very large content, which can exceed the NSE time/memory budget or cause unwanted network access. Consider restricting to HTTPS, optionally whitelisting expected hosts, setting a short timeout, and rejecting responses over a reasonable size/content-type before creating the attachment.
There was a problem hiding this comment.
image-url originates from our own server (celestia.mobi) — the trust boundary is our APNs auth keys, not the payload string. If those leak, an attacker has bigger problems than triggering an oversized download in the NSE.
| <plist version="1.0"> | ||
| <dict> | ||
| <key>aps-environment</key> | ||
| <string>development</string> |
There was a problem hiding this comment.
aps-environment is hard-coded to development. This will prevent production (App Store/TestFlight) push notifications from working and can cause signing/export issues for distribution builds. Consider using a separate entitlements file per configuration (Debug=development, Release=production) or a build-setting-driven value so Release exports use production.
| <string>development</string> | |
| <string>$(APS_ENVIRONMENT)</string> |
There was a problem hiding this comment.
Xcode's distribution archive flow rewrites aps-environment to production automatically when signing with an App Store / Distribution provisioning profile, so source-controlling development is the standard pattern (matches Apple's default template). The CI archive picks the right value too.
| private extension UserDefaults { | ||
| func pushTypeEnabled(_ type: PushNotificationContentType) -> Bool { | ||
| // Defaults to true so newly-added types are enabled-by-default for opted-in users. | ||
| return (self[type.userDefaultsKey] as String?) != "false" | ||
| } | ||
|
|
||
| func enabledPushContentTypes() -> [String] { | ||
| return PushNotificationContentType.allCases | ||
| .filter { pushTypeEnabled($0) } | ||
| .map { $0.rawValue } | ||
| } |
There was a problem hiding this comment.
Push notification preferences are persisted as the strings "true"/"false" (e.g. (userDefaults[.pushWeeklyAddon] as String?) != "false"). Elsewhere in the app, boolean defaults are stored as Bool (e.g. userDefaults[.onboardMessageDisplayed] = true), and mixing representations makes these keys easy to misuse and hard to evolve. Consider storing these as Bool? and treating nil as enabled-by-default instead of encoding booleans as strings.
There was a problem hiding this comment.
Matches the existing convention in this codebase — other on/off prefs (e.g. the various enableXxx flags) are persisted as "true" / "false" strings too. Keeping consistent rather than introducing a second representation just for push prefs.
| init(userDefaults: UserDefaults, onSave: @escaping () -> Void, openSystemSettings: @escaping () -> Void) { | ||
| self.userDefaults = userDefaults | ||
| self.onSave = onSave | ||
| self.openSystemSettings = openSystemSettings | ||
| _weeklyAddon = State(initialValue: (userDefaults[.pushWeeklyAddon] as String?) != "false") | ||
| _latestNews = State(initialValue: (userDefaults[.pushLatestNews] as String?) != "false") | ||
| _featuredAddon = State(initialValue: (userDefaults[.pushFeaturedAddon] as String?) != "false") | ||
| } | ||
|
|
||
| var body: some View { | ||
| Form { | ||
| switch status { | ||
| case .denied: | ||
| Section { | ||
| Text(CelestiaString("Notifications are turned off for Celestia. Enable them in Settings to subscribe to updates.", comment: "Push notification denied state explanation")) | ||
| .foregroundStyle(.secondary) | ||
| Button(CelestiaString("Open System Settings", comment: "Push notification denied state action")) { | ||
| openSystemSettings() | ||
| } | ||
| } | ||
| case .authorized, .provisional, .ephemeral, .notDetermined: | ||
| Section { | ||
| Toggle(CelestiaString("Weekly Add-on", comment: "Push notification content type — weekly featured add-on"), isOn: $weeklyAddon) | ||
| Toggle(CelestiaString("Featured Add-on", comment: "Push notification content type — featured add-on"), isOn: $featuredAddon) | ||
| Toggle(CelestiaString("Latest News", comment: "Push notification content type — latest news"), isOn: $latestNews) | ||
| } | ||
| Section { | ||
| Button(CelestiaString("Save", comment: "Save push notification preferences")) { | ||
| userDefaults[.pushWeeklyAddon] = weeklyAddon ? "true" : "false" | ||
| userDefaults[.pushLatestNews] = latestNews ? "true" : "false" | ||
| userDefaults[.pushFeaturedAddon] = featuredAddon ? "true" : "false" | ||
| onSave() |
There was a problem hiding this comment.
This screen reads/writes push preference values as strings ("true"/"false") in UserDefaults. Given the codebase generally stores booleans as Bool, consider switching these keys to Bool? (and defaulting nil to true) to avoid representation drift and accidental type mismatches later.
There was a problem hiding this comment.
Same as the manager comment — the "true" / "false" string convention is consistent with the other on/off prefs in this codebase.
No description provided.