feat: [SDK-4788] prototype iOS notification delegate proxy - #189
feat: [SDK-4788] prototype iOS notification delegate proxy#189fadi-george wants to merge 2 commits into
Conversation
377b756 to
6d478df
Compare
|
@claude review |
There was a problem hiding this comment.
No bugs found by the automated review, but this PR touches Objective-C runtime internals (manual IMP swizzling via method_getImplementation/method_setImplementation) and relies on private native OneSignal class/selector names, so I'd like a human to weigh in before merging.
What was reviewed:
- The delegate proxy's selector forwarding for both Apple and OneSignal-prefixed
UNUserNotificationCenterDelegateselectors. - The
RepairOneSignalDispatchIMP-swizzling flow and its failure path (throws inInitialize()if repair fails). - The MAUI demo's
FinishedLaunchingreordering to capture the third-party delegate before OneSignal initializes. - Post-init delegate-replacement detection via
DidBecomeActiveNotification.
Extended reasoning...
Overview
This PR adds an experimental iOS delegate proxy (iOSNotificationCenterDelegateProxy) that sits between OneSignal's native swizzled UNUserNotificationCenterDelegate and a third-party delegate (e.g. Plugin.LocalNotification), to avoid a .NET registrar crash when both installed delegates only implement the standard Apple selectors. It adds new binding definitions (OneSignalCoreHelper, OneSignalNotificationCenterDelegateProxy) exposing private OneSignal native selectors, wires the proxy into iOSOneSignal.Initialize, and updates the local-notification demo app and its README to reflect the new initialization ordering required on iOS.
Security risks
No injection/auth/credential exposure risk. The main risk is native-interop correctness: the code does raw Objective-C runtime IMP swapping via P/Invoke against class_getInstanceMethod/method_getImplementation/method_setImplementation. If OneSignal's native SDK renames/removes the targeted private class or selectors in a future release, this either silently no-ops (guarded, returns false) or triggers the explicit InvalidOperationException thrown in Initialize(), which would break app startup for any consumer relying on this path.
Level of scrutiny
This warrants above-average scrutiny. It is public-API-surfacing (new binding types), affects core notification dispatch on iOS, and depends on private/undocumented native selector and class names that are inherently fragile across OneSignal iOS SDK versions. The author explicitly frames this as an experimental prototype not yet production-ready, and calls out unresolved gaps (delegates replaced after Initialize are unprotected, Release/AOT validation for Clicked callbacks is incomplete). This is exactly the kind of judgment call — accepting a fragile-but-necessary interop workaround, and deciding whether throwing on repair failure is the right failure mode — that should have a human sign-off rather than an automated approval.
Other factors
Testing was manual (simulator only, per the PR description) with no new automated test coverage, which is reasonable for this kind of native-interop prototype but means there's no regression safety net. The PR is stacked on #188. No outstanding reviewer threads exist yet in the timeline besides the initial review request.
abdulraqeeb33
left a comment
There was a problem hiding this comment.
Thanks for the prototype — the overall approach makes sense for the Plugin.LocalNotification crash, and the README/demo ordering is clear about the preconditions.
I went through the proxy, init wiring, bindings, and the sample. Nothing that should block landing this as an experimental path, but a few things I'd want tightened (or at least commented) before we treat it as more than that:
Sample (MauiProgram)
- The
#if !IOSarm looks scary in isolation; iOS is actually covered viaFinishedLaunching. Worth a one-line comment, and resolvingappIdfrom DotEnv before registering that handler.
Proxy
- The no-op
completionHandlerfor OneSignal payloads needs a why-comment (or a pass-through if it isn't guaranteed safe after the IMP repair). - Hard-coding
OneSignalNotificationsUNUserNotificationCenteris the fragile bit we already call out — just confirming we're okay owning that across native SDK bumps.
Initialize
- Throwing after native init has already succeeded leaves a half-up SDK. Loud failure is fine for now; softer failure mode later.
- Delegate-replaced warning is Debug-only — fine for prototype, not for customers.
Bindings
OneSignalNotificationCenterDelegateProxy/OneSignalCoreHelpershow up as public binding types. If we can keep them less discoverable, great; if not, explicit "not a consumer API" note somewhere helps.
Manual simulator testing looks solid for a prototype. Would still want another pass on Release/AOT Clicked before calling this production-ready, as you already noted.
6d478df to
b5ac0a2
Compare
abdulraqeeb33
left a comment
There was a problem hiding this comment.
thank you for addressing the comments
nan-li
left a comment
There was a problem hiding this comment.
Thanks for the prototype and the clear write-up: I'm not sure about the PR as the right fix since it is a dotnet workaround around native swizzling and it fixes a narrow slice. Im also unsure about its categorization as a prototype and experimental (?) and it's not clear what comes next. Maybe we should look at something more robust. The solution is probably going to come from the native iOS SDK?
Description
One Line Summary
Prototypes an iOS managed delegate proxy that preserves OneSignal swizzling while forwarding third-party notification delegates safely.
Details
Motivation
OneSignal's prefixed swizzled selectors can crash the .NET registrar when
Plugin.LocalNotificationinstalls a managedUNUserNotificationCenterDelegate. The proxy supplies generated block-conversion metadata for both selector forms and prevents that registrar failure.Scope
FinishedLaunchingordering so the plugin delegate exists before OneSignal initializes.This is an experimental compatibility path. It relies on private native OneSignal class/selector names and does not protect delegates assigned after
OneSignal.Initialize. The generated binding helper/model types support the internal implementation and are not intended as consumer APIs.Stacked on #188.
Testing
Manual testing
response 27).response 7).ObjCRuntime.RuntimeException.FinishedLaunchingordering reproduces the native crash; restoring it resolves the crash.csharpier check .passed.Notification-open behavior was manually smoke-tested, but
Clickedcallback logging should receive additional Release/AOT validation before treating this prototype as production-ready.Affected code checklist
Checklist
Overview
Testing
Final pass
Made with Cursor