Skip to content

refactor: route identifier reads through OneSignalIdentifiers#1667

Open
nan-li wants to merge 3 commits into
mainfrom
nan/identifier-accessors
Open

refactor: route identifier reads through OneSignalIdentifiers#1667
nan-li wants to merge 3 commits into
mainfrom
nan/identifier-accessors

Conversation

@nan-li
Copy link
Copy Markdown
Contributor

@nan-li nan-li commented May 30, 2026

Description

One Line Summary

Consolidate all OneSignal SDK identifier accessors (app_id, subscription_id) into a single Swift module so future fallback logic (cross-process / locked-state) can be added in one place.

Details

Motivation

The SDK currently reads OSUD_APP_ID and OSUD_PUSH_SUBSCRIPTION_ID from OneSignalUserDefaults at ~20 ad-hoc call sites across 8 modules, with three different in-memory caches (OneSignalConfigManager._appId, OSNotificationsManager._pushSubscriptionId, _user.pushSubscriptionModel.subscriptionId). This makes it difficult to add a coordinated fallback (e.g. for the iOS prewarm bug where UserDefaults is unreadable before first unlock). Consolidating the accessors into OneSignalIdentifiers (durable reads) and OneSignalConfig (readiness predicate) sets up a single place for that follow-up.

Scope

Three commits:

  1. 21ed8132 — Add OneSignalIdentifiers.storedAppId / subscriptionId in OneSignalOSCore. Migrate 6 durable-read sites. Link OneSignalOSCore into OneSignalNotifications and OneSignalExtension targets. Drop now-dead OSUD_APP_ID write to initStandard.

  2. 07891381 — Drop redundant OSNotificationsManager._pushSubscriptionId static cache + setter; the model's didSet already persists. Removes drift risk and one call in OSUserExecutor.

  3. eb1a4213 — Replace OneSignalConfigManager with OneSignalConfig + OneSignalIdentifiers.currentAppId (thread-safe via NSLock). Migrate ~30 call sites across 8 modules. Link OneSignalOSCore into OneSignalInAppMessages, OneSignalLocation, OneSignalOutcomes.

Behavior change to note: OneSignal.m's prevAppId reads previously hit initStandard, now hit initShared via the accessor. Behavior-preserving for any install on SDK 2.13.0+ (Dec 2019, commit 7240697d) — handleAppIdChange has dual-written OSUD_APP_ID to both stores ever since.

Not changed: any externally-observable behavior. No public API changes.

Testing

Unit testing

Existing tests updated for the renamed OneSignalConfigManager.setAppId(...)OneSignalIdentifiers.currentAppId = ....

Manual testing

Built and ran the dev app on a physical device with the SDK from source. Verified OneSignal.User.onesignalId and OneSignal.User.pushSubscription.id return the expected real values. Confirmed via temporary debug logs that OneSignalIdentifiers.storedAppId / subscriptionId return the persisted values at runtime.

Affected code checklist

  • Notifications
    • Display
    • Open
    • Push Processing
    • Confirm Deliveries (NSE-side reads now go through OneSignalIdentifiers)
  • Outcomes
  • Sessions
  • In-App Messaging
  • REST API requests
  • Public API changes

Checklist

Overview

  • I have filled out all REQUIRED sections above
  • PR does one thing
  • Any Public API changes are explained in the PR details and conform to existing APIs

Testing

  • I have included test coverage for these changes, or explained why they are not needed
  • All automated tests pass, or I explained why that is not possible
  • I have personally tested this on my device, or explained why that is not possible

Final pass

  • Code is as readable as possible.
  • I have reviewed this PR myself, ensuring it meets each checklist item

nan-li added 2 commits May 30, 2026 16:21
Add OneSignalIdentifiers in OneSignalOSCore with storedAppId and
storedSubscriptionId accessors backed by shared UserDefaults. Migrate
six call sites; drop the now-dead OSUD_APP_ID write to initStandard.
OSSubscriptionModel.subscriptionId.didSet already persists to UserDefaults
on every change, so the static _pushSubscriptionId cache and its setter
were duplicating state with drift risk. Route the one caller through
OneSignalIdentifiers.subscriptionId directly. Drop the now-unused
OSUserExecutor.setPushSubscriptionId call too — the model hydration on
the line above already triggers the persistence path.
Comment on lines 1982 to +1985
isa = PBXFrameworksBuildPhase;
buildActionMask = 2147483647;
files = (
3C5C70022FCB935000102E2C /* OneSignalOSCore.framework in Frameworks */,
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 This PR adds OneSignalOSCore.framework to the link phase of the OneSignalExtension and OneSignalNotifications targets (pbxproj lines 1985, 2070) and the implementations now reference OneSignalIdentifiers from those binaries, but none of the package manifests declare the new dependency. Customers using the standard NSE integration (CocoaPods OneSignalXCFramework/OneSignalExtension or SPM OneSignalExtension product) will not get OneSignalOSCore.framework embedded into their NSE target, so dyld will fail to load the binary when a push fires — silently breaking confirmed delivery and receive-receipts. Fix: add ss.dependency 'OneSignalXCFramework/OneSignalOSCore' to the OneSignalExtension and OneSignalNotifications subspecs in both OneSignalXCFramework.podspec and OneSignal.podspec, and add "OneSignalOSCore" to the OneSignalExtensionWrapper and OneSignalNotificationsWrapper target dependency arrays in Package.swift.

Extended reasoning...

What changed

This PR links OneSignalOSCore.framework into the OneSignalExtension target (pbxproj line 1985, build phase id 3C5C70022FCB935000102E2C) and the OneSignalNotifications target (pbxproj line 2070, build phase id 3C5C6FFD2FCB933100102E2C). The implementations in those frameworks now #import <OneSignalOSCore/OneSignalOSCore-Swift.h> and call OneSignalIdentifiers.subscriptionId / .storedAppId:

  • OneSignalExtension/OneSignalNotificationServiceExtensionHandler.m (NSE confirmed-delivery path, line 144-145)
  • OneSignalExtension/OneSignalReceiveReceiptsController.m (receive-receipts path, line 42-43)
  • OneSignalNotifications/OSNotificationsManager.m (submitNotificationOpened, line 776)

After this PR, the shipped OneSignalExtension.xcframework and OneSignalNotifications.xcframework binaries will have an LC_LOAD_DYLIB entry for @rpath/OneSignalOSCore.framework/OneSignalOSCore.

What is missing

The package manifests have not been updated to declare the new transitive dependency:

OneSignalXCFramework.podspec (lines 28-39):

s.subspec OneSignalExtension do |ss|
  ss.dependency OneSignalXCFramework/OneSignalCore
  ss.dependency OneSignalXCFramework/OneSignalOutcomes
  # MISSING: ss.dependency OneSignalXCFramework/OneSignalOSCore
end
s.subspec OneSignalNotifications do |ss|
  ss.dependency OneSignalXCFramework/OneSignalCore
  ss.dependency OneSignalXCFramework/OneSignalOutcomes
  ss.dependency OneSignalXCFramework/OneSignalExtension
  # MISSING: ss.dependency OneSignalXCFramework/OneSignalOSCore
end

OneSignal.podspec (lines 28-39): identical pattern, same omissions.

Package.swift (lines 72-90):

.target(name: "OneSignalNotificationsWrapper",
        dependencies: ["OneSignalNotifications", "OneSignalExtension", "OneSignalOutcomes", "OneSignalCore"]),  // MISSING "OneSignalOSCore"
.target(name: "OneSignalExtensionWrapper",
        dependencies: ["OneSignalExtension", "OneSignalOutcomes", "OneSignalCore"]),  // MISSING "OneSignalOSCore"

I verified there is no transitive pull-in: the OneSignalOutcomes subspec depends only on OneSignalCore; OneSignalExtension depends only on Core+Outcomes; OneSignalCore has no dependencies. So a consumer wiring only OneSignalXCFramework/OneSignalExtension (or the SPM OneSignalExtension product) into their NSE target will not receive OneSignalOSCore.framework.

Step-by-step proof of failure

  1. Customer follows the documented NSE integration: their Podfile contains pod OneSignalXCFramework/OneSignalExtension under their NSE target (or .product(name: "OneSignalExtension", package: "OneSignalFramework") for SPM).
  2. CocoaPods/SPM resolves dependencies. Per the manifests above, only OneSignalCore + OneSignalOutcomes + OneSignalExtension are embedded into the NSE bundle. OneSignalOSCore.framework is not copied or linked.
  3. A push notification with mutable-content: 1 arrives. iOS spins up the NSE process and loads OneSignalExtension.xcframework.
  4. dyld walks the load commands of OneSignalExtension. It encounters LC_LOAD_DYLIB @rpath/OneSignalOSCore.framework/OneSignalOSCore and cannot resolve it (the framework is not in the bundles Frameworks directory).
  5. dyld emits Library not loaded: @rpath/OneSignalOSCore.framework/OneSignalOSCore and aborts. The NSE process crashes before didReceiveNotificationExtensionRequest: is even invoked.
  6. iOS falls back to displaying the unmodified APNS payload (no media attachments, no confirmed delivery, no receive-receipt). The crash is silent to the end user but observable in Console as a DYLD, [0x4] termination.

This affects every push the NSE handles — confirmed-delivery and receive-receipts go to zero for the affected install.

Why this is not caught

  • The workspace build succeeds because the Xcode workspace embeds all internal frameworks side-by-side.
  • The main-app integration (OneSignal / OneSignalComplete subspec) already pulls OneSignalOSCore in transitively, so single-target apps are fine.
  • The break only manifests for the NSE subset integration, which is the documented standard pattern in OneSignals NSE setup guide.
  • The PRs own test plan explicitly leaves [ ] Confirmed delivery (NSE) and receipts flow exercised unchecked, so this regression has not been validated end-to-end.

Fix

Add the missing dependency declarations:

  1. OneSignalXCFramework.podspec: add ss.dependency OneSignalXCFramework/OneSignalOSCore to both the OneSignalExtension and OneSignalNotifications subspecs.
  2. OneSignal.podspec: add ss.dependency OneSignal/OneSignalOSCore to both the OneSignalExtension and OneSignalNotifications subspecs.
  3. Package.swift: add "OneSignalOSCore" to the dependencies array of both OneSignalExtensionWrapper and OneSignalNotificationsWrapper targets.

…gnalIdentifiers.currentAppId

OneSignalConfigManager held two distinct things in one ObjC class in
OneSignalCore: (1) the in-memory app_id state, and (2) the SDK-readiness
predicate `shouldAwaitAppIdAndLogMissingPrivacyConsent`. Split into two
Swift classes in OneSignalOSCore so all identifier accessors live in one
module:

- `OneSignalIdentifiers.currentAppId` — in-memory app_id, replaces
  `OneSignalConfigManager.getAppId/setAppId`. Sits next to the existing
  `storedAppId` (persisted) and `subscriptionId` (persisted) accessors.
- `OneSignalConfig.shouldAwaitAppIdAndLogMissingPrivacyConsent` — same
  semantics, now reads `OneSignalIdentifiers.currentAppId` internally.

Migrate ~30 call sites across OneSignalCore (umbrella), OneSignalOSCore,
OneSignalUser, OneSignalLiveActivities, OneSignalNotifications, OneSignal-
Location, OneSignalInAppMessages, OneSignalOutcomes, and the main OneSignal
target. Link OneSignalOSCore.framework into OneSignalInAppMessages, One-
SignalLocation, and OneSignalOutcomes targets (they didn't previously).
@nan-li nan-li force-pushed the nan/identifier-accessors branch from d99f0ad to eb1a421 Compare May 31, 2026 00:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant