fix: [SDK-4900] expose Android notification SetExtender - #193
Conversation
abdulraqeeb33
left a comment
There was a problem hiding this comment.
Multi-model adversarial review
Verdict: Approve with changes — the binding fix is correct; the teaching sample needs fixes before merge.
Reviewed with packaging (Opus), bindings/API (GPT), example quality (Sonnet), Bugbot, and Security. Scoped to this PR’s 11-file diff.
What held up
javapconfirmsIMutableNotification.setExtender(NotificationCompat.Extender)incore-release.aar.- Placing
Xamarin.AndroidX.Core1.12.0.1 onCore.Binding+ the Android nuspec group is the right fix. - AppCompat’s Core floor is
[1.8.0.1,)— 1.12.0.1 resolves cleanly (no NU1107/NU1605). - Android-only binding exposure (no cross-platform wrapper) is appropriate for #92.
- Security: launcher
Exported = trueis required on API 31+; extension is meta-data only; sample embeds public App ID only. No medium+ security findings. - Bugbot: no bugs.
Blocking
-
Manifest permissions (
examples/android-service-ext/Properties/AndroidManifest.xml)
Only theNotificationServiceExtensionmeta-data is declared. Sibling examples declareINTERNET,ACCESS_NETWORK_STATE,POST_NOTIFICATIONS, etc. Debug may injectINTERNET; Release and API 33+POST_NOTIFICATIONSwill not. Align with sibling manifests (at leastPOST_NOTIFICATIONS+ network). -
Linker / trim preserve for JNI-registered type (
NotificationServiceExtension.cs+ README)
The class is only reached via manifest string + native reflection. No[Preserve], trim root, or ILLink descriptor. README tells integrators to copy this into MAUI apps that ship trimmed Release builds — silent strip → extender never runs, no compile error. Add preserve guidance next to the copy instructions. -
Title-overwrite footgun (
NotificationServiceExtension.cs)
SetContentTitle("[Extended] Test")teaches destroying campaign titles. Prefer color-only (or prefixing the existing title) so the sample provesSetExtenderwithout a destructive default.
Should-fix before merge
- Await / surface errors from
RequestPermissionAsynclikedemo/demo-no-location(avoid fire-and-forget_ =). - In the PR / #92 discussion: state that the duplicate-delivery / deferred
PreventDefaulttimeout half of #92 is out of scope so this isn’t read as a full close. - Split or drop
AGENTS.md— csharpier policy is unrelated to SDK-4900.
Non-blocking
- Mirror
Xamarin.AndroidX.CoreinOneSignalSDK.DotNet.Android.csprojor document the intentional exception (every other AndroidX nuspec dep is mirrored there; Core is only on binding + nuspec). - CI currently only runs csharpier; a Release build of the binding + sample would catch SetExtender generation and trim issues. Sample
ProjectReferencealso never validates the nuspec dependency graph.
Summary table
| Severity | Location | Finding |
|---|---|---|
| High | AndroidManifest.xml |
Missing permissions vs sibling examples; API 33+ / Release footgun |
| High | NotificationServiceExtension.cs + README |
Reflective JNI type with no preserve; copy-paste into trimmed apps |
| Medium | NotificationServiceExtension.cs |
SetContentTitle overwrite is a teaching footgun |
| Medium | ci.yml / sample csproj |
No CI/build proof of binding generation or nuspec graph |
| Medium | PR body / #92 | Only half of #92 addressed |
| Low | Android.csproj vs nuspec |
Convention drift on Core PackageReference |
| Low | MainActivity.cs |
Fire-and-forget RequestPermissionAsync |
| Low | AGENTS.md |
Scope creep |
Happy to re-review once the sample blockers are addressed — the dependency change itself looks merge-ready.
Co-authored-by: Cursor <cursoragent@cursor.com>
|
Thanks for the detailed review. I pushed permission error handling and clarified that the duplicate/timeout behavior is outside this PR's code changes. The Release merged manifest already contains INTERNET, ACCESS_NETWORK_STATE, and POST_NOTIFICATIONS from dependency manifests. The registered extension types are retained in the Release output; Android.Runtime.Preserve is obsolete in modern .NET Android, so I did not add it. I also kept the title override and AGENTS.md intentionally. The Release sample build passes with 0 warnings and 0 errors. |
|
Addressed:
Not changing:
|
abdulraqeeb33
left a comment
There was a problem hiding this comment.
Re-review
Thanks for the follow-up — looks good to approve.
Addressed
RequestPermissionAsyncis now awaited with a surfaced failure toast.OnDestroyonly unsubscribes when subscribed.- Scope note on duplicate delivery / deferred
PreventDefaulttimeout is clear in the comment thread. - Binding + nuspec
Xamarin.AndroidX.Coreplacement remains correct.
Accepting as intentional / locally validated
- Manifest permissions via merged Release dependencies (not duplicated in the sample manifest).
- No
[Preserve]given obsolete API + Release retention of registered peers. - Hardcoded
[Extended] Testtitle as an obvious demo signal. AGENTS.md, noAndroid.csprojCore mirror, and no CI expansion for this PR.
Approve — the SetExtender exposure is the important change and the sample is in good enough shape to land.
Description
One Line Summary
Expose Android notification extenders in the .NET binding and add a runnable service-extension example.
Details
Motivation
The native Android SDK exposes
IMutableNotification.setExtender, but the .NET binding generator omitted it becauseNotificationCompat.Extendercould not be resolved. This prevented .NET apps from customizing notifications through an Android notification service extension. Addresses #92.The duplicate-delivery and deferred
PreventDefaulttimeout behavior also discussed in #92 is not changed by this PR. Current bundled Android SDKs exposePreventDefault(true)to permanently discard a notification; that existing behavior was validated while testing this change.Scope
SetExtender(IExtender)is generated and consumable.examples/android-service-extapp showing stable JNI registration, manifest configuration, notification customization, push subscription display,.envsetup, permission error handling, and device launch tooling.PreventDefaultbehavior, or cross-platform notification API.Testing
Unit testing
No unit tests were added because the API surface is generated from the native AAR. The runnable sample provides compile-time coverage for the generated
SetExtendermethod.Manual testing
SetExtendercompiler error. Restored the dependency and confirmed the build passes.[Extended] Testand applies the blue accent while preserving rich media and actions.PreventDefault(true)suppresses display without restoration or the 30-second timeout; generation and receive-receipt workers completed successfully.Affected code checklist
Checklist
Overview
Testing
Final pass
Made with Cursor