feat: [SDK-4788] add iOS manual notification forwarding#190
feat: [SDK-4788] add iOS manual notification forwarding#190fadi-george wants to merge 3 commits into
Conversation
|
@claude review |
|
@claude review once |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| public void DidRegisterForRemoteNotifications(UIApplication application, NSData deviceToken) | ||
| { | ||
| NotificationsManualIntegration.DidRegisterForRemoteNotifications(deviceToken); | ||
| } | ||
|
|
||
| [Export("application:didFailToRegisterForRemoteNotificationsWithError:")] | ||
| public void DidFailToRegisterForRemoteNotifications(UIApplication application, NSError error) | ||
| { | ||
| NotificationsManualIntegration.DidFailToRegisterForRemoteNotifications(error); | ||
| } | ||
|
|
||
| [Export("application:didReceiveRemoteNotification:fetchCompletionHandler:")] | ||
| public void DidReceiveRemoteNotification( | ||
| UIApplication application, | ||
| NSDictionary userInfo, | ||
| Action<UIBackgroundFetchResult> completionHandler | ||
| ) | ||
| { | ||
| NotificationsManualIntegration.DidReceiveRemoteNotification(userInfo, completionHandler); | ||
| } |
There was a problem hiding this comment.
🟡 The three new AppDelegate notification callbacks are declared without override, so they don't join MauiUIApplicationDelegate's virtual dispatch chain — they merely claim the same Objective-C selectors via [Export]. In particular, DidReceiveRemoteNotification shares the exact name and signature of the inherited virtual, which triggers a CS0114 warning that contradicts the PR's 'zero warnings' build claim, and silently bypasses MAUI's ConfigureLifecycleEvents hooks for these three callbacks for any code (this app's future features or other packages) that relies on them. Fix: declare these as override, drop the redundant [Export] attributes, and call base in addition to forwarding to NotificationsManualIntegration.
Extended reasoning...
The bug: AppDelegate (examples/plugin-local-notif/Platforms/iOS/AppDelegate.cs:15-34) adds three methods decorated with [Export] that reuse selectors already bound to virtual methods on UIApplicationDelegate/MauiUIApplicationDelegate, but declares them as plain (non-override) methods:
DidRegisterForRemoteNotifications(UIApplication, NSData)— selectorapplication:didRegisterForRemoteNotificationsWithDeviceToken:, whose bound virtual isRegisteredForRemoteNotifications.DidFailToRegisterForRemoteNotifications(UIApplication, NSError)— selectorapplication:didFailToRegisterForRemoteNotificationsWithError:, bound virtualFailedToRegisterForRemoteNotifications.DidReceiveRemoteNotification(UIApplication, NSDictionary, Action<UIBackgroundFetchResult>)— this one matches the inherited virtual's C# name and signature exactly, so it hides rather than shadows-by-different-name.
Why this matters: MauiUIApplicationDelegate implements these three callbacks as overrides specifically so it can dispatch to the documented ConfigureLifecycleEvents(...).AddiOS(ios => ios.RegisteredForRemoteNotifications(...)/.FailedToRegisterForRemoteNotifications(...)/.DidReceiveRemoteNotification(...)) hooks. Because the new methods aren't declared override, they don't join that virtual dispatch chain — they're separate methods that happen to claim the same selector via [Export]. Since only one native IMP can exist per selector, the .NET-for-iOS registrar binds it to the most-derived declaration (AppDelegate's new method), so the inherited MauiUIApplicationDelegate override — and therefore the lifecycle-event dispatch — never fires for this app.
Compiler-observable proof (CS0114): For the third method specifically, C# requires override or new whenever a member exactly matches an inherited virtual's name and signature; omitting both is a deterministic CS0114 ('hides inherited member — did you intend to override?') warning, not something that depends on runtime behavior or environment. I confirmed examples/plugin-local-notif/plugin-local-notif.csproj has no NoWarn for CS0114, and the repo's shared Directory.Build.props only suppresses NETSDK1202 — nothing suppresses CS0114 for this project. (Only OneSignalSDK.DotNet.iOS.Binding.csproj, an unrelated binding project, suppresses CS0114.) So an iOS build of this example necessarily emits at least this one warning, which contradicts the PR description's explicit claim: 'Built iOS Debug for iossimulator-arm64 with zero warnings' / 'Built and launched iOS Release/AOT ... with zero warnings.' The other two methods use different C# names than their base virtuals, so only this third one triggers CS0114 — the underlying shadow-registrar issue nonetheless applies to all three.
Addressing the counterargument: One reviewer argued this is intentional interception with no functional impact, since the example doesn't wire up ConfigureLifecycleEvents for these three events — it only subscribes to FinishedLaunching, and OneSignal itself doesn't depend on MAUI lifecycle events to receive the forwarded calls. That's correct as far as it goes, and is exactly why this is a nit rather than a normal-severity finding: there is no observable runtime failure in this specific demo today. However, that argument doesn't refute the CS0114 sub-claim — CS0114 is a deterministic consequence of C#'s hiding rules for an exact name/signature match, independent of anything observed at runtime, and I've verified no suppression exists in the relevant project or its shared build props. A 'zero warnings' claim in the PR body is therefore factually inaccurate for this file, and — because this file is meant to be a canonical reference pattern that developers will copy into real apps — the shadowing footgun is worth flagging even though it's harmless in the sample itself.
Step-by-step proof of the shadow mechanism:
UIApplicationDelegate(base ofMauiUIApplicationDelegate) exposes a virtualDidReceiveRemoteNotification(UIApplication, NSDictionary, Action<UIBackgroundFetchResult>)bound to selectorapplication:didReceiveRemoteNotification:fetchCompletionHandler:.MauiUIApplicationDelegateoverrides that virtual to dispatch toiOSLifecycle.DidReceiveRemoteNotificationsubscribers registered viaConfigureLifecycleEvents.AppDelegate(this PR) declares a plain method with the identical name, signature, and[Export]selector string, but withoutoverride.- At Objective-C class-generation time, only one native IMP can back a given selector; the .NET-for-iOS registrar binds it to the most-derived declaration —
AppDelegate's new method — notMauiUIApplicationDelegate's override. - Consequently, if any code (a future feature in this app, or another NuGet package) calls
ConfigureLifecycleEvents(...).AddiOS(ios => ios.DidReceiveRemoteNotification(...)), its handler is never invoked, silently, with no compiler error. - Separately, step 3 also produces CS0114 at compile time because the C# name/signature exactly matches the inherited virtual — verified against the project's csproj and the repo's Directory.Build.props, neither of which suppress it.
Fix: declare all three methods with override (dropping the now-redundant [Export] attributes, since the base virtuals already carry them) and call base.RegisteredForRemoteNotifications(...)/base.FailedToRegisterForRemoteNotifications(...)/base.DidReceiveRemoteNotification(...) in addition to forwarding to NotificationsManualIntegration, preserving MAUI's lifecycle-event dispatch for any future consumer.
There was a problem hiding this comment.
I checked the actual Microsoft.Maui.Core 10.0.41 assembly used by this example. In that version, MauiUIApplicationDelegate inherits UIResponder and implements IUIApplicationDelegate; it does not define virtual RegisteredForRemoteNotifications, FailedToRegisterForRemoteNotifications, or DidReceiveRemoteNotification methods. Its iOSLifecycle API also has no registration or remote-notification lifecycle delegates. Therefore there is no applicable base virtual to override or base implementation to call, and the Export attributes are required to expose these optional Objective-C selectors. The iOS Debug and Release/AOT builds both completed with 0 warnings, so CS0114 is not emitted here. We are keeping the explicit Export-based forwarding pattern.
Co-authored-by: Cursor <cursoragent@cursor.com>
abdulraqeeb33
left a comment
abdulraqeeb33
left a comment
There was a problem hiding this comment.
Thanks for landing the lower-risk manual-forwarding path — this is clearly the right shape versus the proxy approach. The bindings look solid against the packaged headers (isOneSignalPayload:, the five OSNotifications manual APIs, display-response nullability), InternalsVisibleTo is wired correctly, and keeping this opt-in behind OneSignal_disable_swizzling means default consumers are unaffected.
I’m leaving the AppDelegate [Export] / override discussion alone — Fadi already checked MAUI 10.0.41 and that thread is settled for this sample.
What I’d still like fixed before merge are the completion-handler footgun on the shared TryHandle* path, and the example’s “send a OneSignal push” story, which doesn’t match the committed code.
|
|
||
| var payload = notification.Request.Content.UserInfo; | ||
| if (!IsOneSignalNotification(payload)) | ||
| return false; |
There was a problem hiding this comment.
Both TryHandle* methods call EnsureEnabled() before the IsOneSignalNotification gate. EnsureEnabled throws when the plist flag is missing, so we never get to return false and the coordinator never falls through to the plugin delegate.
In the example that means a missing OneSignal_disable_swizzling key doesn’t quietly skip OneSignal — it aborts the whole WillPresent / DidReceiveResponse path, Apple’s completion never runs, and local notifications hang too. That’s a much worse failure mode than “OneSignal just doesn’t get the callback.”
I’d either:
- Check the payload first and only call
EnsureEnabledonce we know it’s ours, or - Treat “not enabled” as
return false(log + fall through) instead of throwing on these two entry points.
The throw still makes sense on the unconditional AppDelegate forwards (DidRegister…, DidReceiveRemoteNotification, etc.), where there’s no alternate owner to hand off to.
| ? (UNNotificationPresentationOptions)0 | ||
| : presentationOptions | ||
| ) | ||
| ); |
There was a problem hiding this comment.
Small asymmetry with TryHandleNotificationResponse below: the click path wraps the native call in try/finally so Apple’s completion always fires once. Here the only way completionHandler runs is inside OneSignal’s display callback.
Native willPresentNotificationWithPayload:completion: does appear to invoke the block on the happy/error paths I checked, so this isn’t a guaranteed bug — but a sync failure at the P/Invoke boundary would leave willPresent unfinished, and we already chose the safer pattern for clicks. Worth matching that here (try/finally with a once-guard, or equivalent).
| var response = await client.PostAsync( | ||
| "https://onesignal.com/api/v1/notifications", | ||
| new StringContent(json, Encoding.UTF8, "application/json") | ||
| ); |
There was a problem hiding this comment.
This POST has no Authorization header, so OneSignal will 401 it every time. That conflicts with the README / PR testing claim that the in-app “Show OneSignal Notification” action was used to verify foreground display and clicks.
Two related concerns:
- Evidence — either this was tested against a local change that isn’t in the PR, or the push came from the dashboard and got attributed to this button. Worth correcting the testing write-up either way.
- Footgun —
.envis already aMauiAsset(appenv). The obvious “fix” is stuffing a REST API key into.env, which then ships inside the ipa/apk.
I’d either drop the button and document “send a test push from the dashboard to the subscription ID shown above,” or route send through something that never embeds a REST key in the client. Matching the main demo’s auth-less pattern doesn’t really help here because this sample is specifically selling that button as the verification path.
| notificationCenter.Delegate | ||
| ?? throw new InvalidOperationException( | ||
| "Plugin.LocalNotification must install its iOS delegate before OneSignal." | ||
| ); |
There was a problem hiding this comment.
Throwing from FinishedLaunching when UNUserNotificationCenter.Current.Delegate is still null is harsh for a reference pattern. Today it depends on UseLocalNotification() having already installed its delegate by the time our lifecycle hook runs — true with the current plugin, but not something we control, and the failure mode is a launch crash with a stack that doesn’t obviously point at init ordering.
A short comment above the throw spelling out the required order (plugin first, wrap, then OneSignal.Initialize) would help copiers a lot. Even better: log + skip install (or retry once) instead of taking down startup, since the AppDelegate forwards can still work without the coordinator.
2 participants
Description
One Line Summary
Adds a supported, opt-in iOS manual notification-forwarding API for apps that own
UNUserNotificationCenterDelegate.This is lower risk than the automatic proxy approach in #189 because it uses only the native SDK’s documented manual-forwarding APIs. It does not depend on private native class or selector names, patch Objective-C method implementations at runtime, or change behavior unless an app explicitly opts in.
Details
Motivation
Setting
OneSignal_disable_swizzlingcurrently disables OneSignal's automatic notification callback handling, but the .NET SDK does not expose the native manual-forwarding APIs. As a result, setting the flag alone leaves token registration, foreground display, clicks, and background notification processing without a supported forwarding path.This PR makes disabling swizzling functional for .NET consumers. It exposes the native SDK's documented manual-forwarding path so apps can explicitly coordinate callbacks when another library, such as
Plugin.LocalNotification, ownsUNUserNotificationCenterDelegate. This also avoids the managed registrar crashes caused when native swizzling invokes dynamically injected selectors on managed delegates.Scope
NotificationsManualIntegrationpublic API:IsEnabledandIsOneSignalNotificationDidRegisterForRemoteNotificationsDidFailToRegisterForRemoteNotificationsDidReceiveRemoteNotificationTryHandleWillPresentNotificationTryHandleNotificationResponseSetBadgeCountOneSignal_disable_swizzlingand retain ownership of the notification delegate.Testing
Unit testing
No unit tests were added because these APIs bridge UIKit/UserNotifications callbacks into the packaged native iOS framework and require native runtime behavior. The binding, AOT compilation, and callback paths were exercised directly.
Manual testing
From
examples/plugin-local-notif, I ran:I requested both OneSignal notification permission and local notification permission, then verified:
All four paths completed without a managed registrar crash. The OneSignal
WillDisplayandClickedlisteners also fired for their respective notification paths.Additional verification:
iossimulator-arm64with zero warnings.iossimulator-arm64with zero warnings.WillDisplay, banner presentation, and a single Apple completion response.csharpier check .andgit diff --check.Affected code checklist
Checklist
Overview
Testing
Final pass