feat: [SDK-5032] enable Mac Catalyst demo builds - #1716
Conversation
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Multimodal adversarial review (interrogate)
Verdict: Do not merge for Catalyst E2E validation until the App Group identity mismatch is fixed. The storage createDirectory change treats a misconfigured group name as a missing directory.
Intent
Enable the SwiftUI demo (and extensions) to build/run under Mac Catalyst so the SDK’s KMP Catalyst slice and platform behavior can be validated end-to-end: Catalyst project settings, Live Activities stubs/exclusions, widget placeholder, and resilient storage creating a missing container before atomic writes. Stacked on #1715.
Reviewers
- A:
claude-fable-5-thinking-xhigh— 6 findings - B:
gpt-5.6-sol-xhigh— 2 findings - C:
cursor-grok-4.5-high-fast— 4 findings - D:
claude-opus-5-thinking-high— 8 findings
Act On
DERIVE_MACCATALYST_PRODUCT_BUNDLE_IDENTIFIERbreaks App Group naming (A/B/C/D — critical). Demo has noOneSignal_app_groups_key. SDK falls back togroup.<bundleId>.onesignal. Catalyst bundle becomesmaccatalyst.com.onesignal.example→ computed groupgroup.maccatalyst.com.onesignal.example.onesignal, but entitlements only grantgroup.com.onesignal.example.onesignal. Shared storage / NSE co-access / the PR’s own App Group prompt note all follow from this. Fix: pinOneSignal_app_groups_keyon App+NSE, or disable derive so the bundle id stayscom.onesignal.example.- App Group prepare failure silently rehomes to Application Support (A/C/D). New
do/catchfalls through after a non-nil container URL. That severs the app↔NSE single-file contract and can make Catalyst “validation” look green on private storage. Keep using the group URL / fail closed once a container URL was selected. - Demo Live Activities UI not actually stubbed (A/B/C/D).
LiveActivitySectionstill mounts;startno-ops with no feedback;update/endstill POST REST for activities that cannot start locally. Hide/disable the section on Catalyst (or gate all controller ops + show unavailable copy).
Consider
OneSignal.mCatalyst#ifis largely redundant with the LiveActivities module already blanking sources; stub still logs “add the module” on Catalyst (A/D).- Widget placeholder vs excluding the widget target from Catalyst (A).
- New test mostly asserts Foundation
createDirectory+atomic write; misses prepare-failure / fallback behavior (A/C/D). createDirectoryon everyfileURL()read; directory lacksNSFileProtectionNonewhile the file is carefully unprotected (D).- No CI
macOS,variant=Mac Catalystbuild of the demo (D).
Noted
- Inconsistent
#ifaroundimport OneSignalLiveActivitiesvs unguarded import inApp.swift(D). - TEMP_* UUID churn in pbxproj from xcodegen (noise).
Dismissed
- Dropping
createDirectoryentirely once group naming is fixed — still useful for a legitimately missing entitled container; keep it, but stop using it to paper over the wrong group / silent private fallback. - Claiming LiveActivities framework cannot link on Catalyst — module already compiles empty under
targetEnvironment(macCatalyst); linking is intentional empty.
Agreement Map
All four models independently hit the derive→App Group mismatch and incomplete demo LA stubbing. Three flagged silent Application Support fallback and weak test coverage. Divergence is mostly on whether OneSignal.m guards / widget placeholder / file-protection / CI belong in Act On vs Consider — lead kept Act On to the three merge-blocking items above.
Sent by Cursor Automation: Automatic PR Review
| SUPPORTS_MACCATALYST: YES | ||
| DERIVE_MACCATALYST_PRODUCT_BUNDLE_IDENTIFIER: YES |
There was a problem hiding this comment.
Act on (A/B/C/D): With DERIVE_MACCATALYST_PRODUCT_BUNDLE_IDENTIFIER = YES and no OneSignal_app_groups_key in Info.plist, OneSignalUserDefaults.appGroupName() becomes group.maccatalyst.com.onesignal.example.onesignal, but entitlements only declare group.com.onesignal.example.onesignal.
That breaks the shared App Group / NSE path this PR is meant to validate, and matches the “macOS App Group authorization prompt” note in the PR body.
Pin OneSignal_app_groups_key on App + NSE, or set derive to NO so the Catalyst bundle id stays com.onesignal.example.
| if let container = fileManager.containerURL(forSecurityApplicationGroupIdentifier: groupName) { | ||
| do { | ||
| return try preparedFileURL(in: container, fileManager: fileManager) | ||
| } catch { | ||
| OneSignalLog.onesignalLog( | ||
| .LL_WARN, | ||
| message: "OSResilientStorage could not prepare the App Group container: \(error)" | ||
| ) | ||
| } |
There was a problem hiding this comment.
Act on (A/C/D): After a non-nil App Group containerURL, prepare failure now warns and falls through to Application Support.
Previously the group URL was always returned and a write failure stayed on that path. Silent private rehoming breaks the documented app↔NSE single-file contract and can make Catalyst storage checks look green without touching the entitled group.
Once a container URL is selected, fail closed (return nil / keep the group URL) instead of switching backing stores mid-flight.
| @@ -98,6 +103,7 @@ enum LiveActivityController { | |||
| attributes: attributes, | |||
| content: content | |||
| ) | |||
| #endif | |||
There was a problem hiding this comment.
Act on (A/B/C/D): Only setup/start are compiled out. LiveActivitySection still mounts on Catalyst (ContentView), Start silently no-ops, and update/end below still hit the Live Activities REST API for an activity that cannot exist locally.
Hide/disable the section on Catalyst (mirror the widget’s “unavailable” copy), and gate all controller operations consistently.
| #if TARGET_OS_MACCATALYST | ||
| return [OSStubLiveActivities liveActivities]; |
There was a problem hiding this comment.
Consider (A/D): OneSignalLiveActivities sources are already blanked under #if targetEnvironment(macCatalyst), so NSClassFromString already misses and the pre-existing else path already returned the stub. These #ifs mainly suppress the facade logs; OSStubLiveActivities still ERROR-logs “module must be added,” which is the wrong diagnosis on Catalyst.
Prefer one platform-aware stub/message owned by the LiveActivities module rather than a third dispatch path in OneSignal.m.
| func testPreparedFileURL_createsMissingContainerForAtomicWrites() throws { | ||
| let root = FileManager.default.temporaryDirectory | ||
| .appendingPathComponent(UUID().uuidString, isDirectory: true) | ||
| let container = root.appendingPathComponent("group.example", isDirectory: true) | ||
| defer { try? FileManager.default.removeItem(at: root) } | ||
|
|
||
| let fileURL = try OSResilientStorage.preparedFileURL(in: container) | ||
| try Data("value".utf8).write(to: fileURL, options: .atomic) | ||
|
|
||
| XCTAssertTrue(FileManager.default.fileExists(atPath: fileURL.path)) |
There was a problem hiding this comment.
Consider (A/C/D): This asserts FileManager.createDirectory + atomic Data.write on a temp path. It does not cover the new risky fileURL() behavior (prepare-failure fallthrough / group vs Application Support choice), which is what motivated the production change.
Cover prepare-failure semantics (no private fallback once a group URL was selected), or drop the helper-only assertion.
1 participant
Description
One Line Summary
Enables the SwiftUI demo app and its extensions to build and run with Mac Catalyst.
Details
Motivation
A runnable Catalyst host is needed to validate the SDK's KMP Catalyst framework slice and platform behavior end to end.
Scope
Adds Catalyst project support, excludes unsupported Live Activity behavior, provides a Catalyst widget placeholder, and makes resilient storage tolerate a missing App Group directory.
Other
Stacked on #1715.
Local development builds may show the macOS App Group authorization prompt when their provisioning profile does not authorize the Catalyst group. Properly provisioned App Store and TestFlight builds are unaffected.
Testing
Unit testing
Added resilient-storage coverage for creating a missing container before an atomic write.
Manual testing
Built the demo and SDK frameworks for both iOS Simulator and Mac Catalyst. Verified the Catalyst demo launch, crash upload flow, storage behavior, and Live Activities no-op behavior.
Affected code checklist
Checklist
Overview
Testing
Final pass
Made with Cursor