chore: [SDK-5083] add an inert-by-default notification service extension to the demo - #2726
chore: [SDK-5083] add an inert-by-default notification service extension to the demo#2726nan-li wants to merge 6 commits into
Conversation
…on to the demo Nothing in the repo implemented INotificationServiceExtension, so reproducing an NSE bug meant writing one from scratch and no compiled sample guarded the interface against a breaking change. Building the demo inside OneSignalSDK's :app project now turns that into a CI failure, and the release build exercises the -keep rule in onesignal/notifications/consumer-rules.pro end to end. Six switches drive it, all off, folded behind a Show options row. It reads them from SharedPreferences rather than MainViewModel because it runs whether or not the app is open, and it sets an extender only when a switch needs one, since an extender makes the SDK display a data-only push carrying no alert. The channel readout uses NotificationCompat.getChannelId inside the extender, the only place an extension sees the SDK's choice. A restored notification lands on restored_OS_notifications whatever the payload asked for. Logging restoring next to it waits on SDK-5011.
Demo output and forwarded SDK output sat side by side in logcat under tags that gave no hint which was which, which made reading a notification repro slower than it needed to be. DemoLog stamps both the tag and the message, so `logcat -s` still filters on the tag and a line stays recognizable when only the message column is in view. Callers pass the plain class name and DemoLog adds the prefix, keeping [Demo] in one place. All 127 demo call sites go through it. The five forwarding calls in MainApplication keep using android.util.Log and stay unmarked. Those lines are the SDK's, and marking them would bury the demo's own output whenever you grep [Demo].
📊 Diff Coverage Report✓ Coverage check passed (no source files changed) |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Multi-model review (Opus 5 / GPT 5.6 Sol / Grok 4.6) of the demo NSE work. Demo-only; no SDK source issues.
Act on
- Delay Display can drop the notification. Raw
Thread+Thread.sleepwith notry/finally. If the thread is interrupted or dies,display()never runs and the SDK’s 30s waiter treats it aswantsToDisplay = false.MainApplication’s existing delay path already catchesInterruptedExceptionand still callsdisplay(). - Documented extender / data-only-push behavior is wrong for 5.x.
processHandlerResponsecheckscanDisplay(nonemptyalert) beforeshouldDisplayNotification. An extender cannot make a bodyless push display. Conditional extender is still reasonable; the rationale in the class andbuild.mdis not.
Consider
- Foreground Delay Display stacks with
MainApplication’s 2s lifecycle delay (~7s observed). [Demo]OneSignalRepositoryis 25 chars; tags over 23 throw on API 21–23 (minSdk21).- Hidden Discard/Delay stay live after collapse/rotation with no visible indicator.
Noted / dismissed
- Preference backup restoring NSE switches, title-prefix clobbering the app-name default, leftover high-importance channel, and
build.mdstaleLog.*mentions: lone-model or low-impact demo nits.
Sent by Cursor Automation: PR Reviews
The comment claimed an extender makes the SDK display a data-only push, so installing a no-op one would not be inert. That is not how 5.x behaves. processHandlerResponse gates on canDisplay, a non-empty notification body, before it reaches shouldDisplayNotification, so hasExtender() is never read for a bodyless push and an extender cannot rescue one. The code stays as it is. Setting an extender only when a switch needs one is still right, just for the duller reason that nothing asked for it otherwise. Left uncorrected, a customer reading the demo could design around SDK behavior that does not exist.
|
I would check this doesnt break the e2e tests suit, you can test with sdk-shared run-local |
fadi-george
left a comment
fadi-george
left a comment
There was a problem hiding this comment.
Ideally we dont modify the current demo and just maybe make another example e.g. examples/demo-nse
The NSE is disabled by default, do you think we really need another demo app just for the NSE? 
|
Done locally, all passed. Note that I don't think the e2e workflow itself added in #2652 has ever ran |
The demo app is supposed to almost the same for all the wrappers. Ideally we don't add stuff to it willy-nilly. I still think this could be a separate example i.e. examples/demo-nse it can just have the section you've made and maybe some test notification if it needs it. |
nan-li
changed the title
We have the NSE in our ios demo ( granted it is more crucial for ios for core features like received receipts), and it used to be included in the prev Android example (see old example) that never ported over to the new sample. I would argue this keeps parity with previous version of the sample app and our current iOS sample rather than an extra feature that warrants to be in a separate example. But if you still feel strongly I can make a separate demo, it's just when you start making more demos people are rarely going to run the other ones in their day-to-day, compared to other demos we have that exclude location or don't use pods. Those are demos that rarely run and seem to be for build verification. |
The shared demo is meant to look almost the same across every wrapper, so a section with six switches and a fold row was more surface than this earns. Enable Extension is the only control now, and the section reads like the In-App Messaging card next to it. The five behavior switches are unchanged and still wired end to end. They just have no UI: flip the defaults in SharedPreferenceUtil.getNotificationExtensionOptions and rebuild when reproducing something.
|
Removed all the option toggles from the UI per JonF's feedback today, since the NSE is quite customizable, we don't want to lead users into only considering limited options, or taking our options as best practices. |
|
yeah we definitely shouldnt have another demo app. |
| onInfoClick = { showTooltipDialog = "sendPushNotification" } | ||
| ) | ||
|
|
||
| NotificationExtensionSection( |
There was a problem hiding this comment.
Maybe render this section last as to not interfere with appium tests.
Potentially could also move to secondary screen.
There was a problem hiding this comment.
yeah i think this is a better solution, lets move functionality that are meant for one platform into another screen. - Secondary screen. That way we dont break appium tests.
There was a problem hiding this comment.
It's at the bottom of the push section where it makes sense to be: after push ID, enabled toggle, "send push notification" section.
lets move functionality that are meant for one platform into another screen
Can you clarify the Appium issue that is breaking? Because they passed locally for me. I did notice that the e2e workflow added in #2652 doesn't look like it has ever ran? I may be wrong and looking in the wrong places on this.
There was a problem hiding this comment.
Ok, since we are omitting mention of Android NSE for wrappers, I'll remove the UI and just make it code only (for when testing this)
There was a problem hiding this comment.
what are you planning for the code-only change?
There was a problem hiding this comment.
added the gradle property to show it for testing
|
Otherwise maybe we can gate rendering the whole section by a gradle property e.g. onesignal.showNSESection ? |
The shared demo is meant to render the same screen across every wrapper, and this PR was adding a section to it. Gating the UI keeps that screen byte identical by default while leaving the section available to whoever is actually debugging an extension. MainScreen wraps the section in `if (BuildConfig.SHOW_NSE_SECTION)`, fed by the existing demoOverride() helper, so it resolves -P, then local.properties, then false. Build with -PSHOW_NSE_SECTION=true to get it. Only the UI is gated. The extension stays registered in the manifest, so the class still compiles in the demo-build job and still exercises the -keep rule in onesignal/notifications/consumer-rules.pro, which is the part that guards the interface. The flag is a compile-time constant, so R8 drops the section from a default release build rather than shipping unreachable Compose. This reverts the trim in 0717e9a. All six switches are back, since gating answers the uniformity concern that motivated cutting them.
|
dont need the collapse toggle (hide/show options) |
|
do we need a toggle for log details, maybe it should be controlled by enable extension toggle? |
Two review notes from Fadi. The fold was protecting a screen that no longer renders by default, so the five switches sit flat now. And a separate Log Details switch meant enabling the extension produced nothing observable; logging follows the master toggle instead, so turning it on always writes [Demo]NSE lines. That makes the extender unconditional, since the resolved channel can only be read from inside extend(builder) and that readout is part of the log line. Nothing is lost: an extender cannot change what displays, because processHandlerResponse gates on canDisplay before shouldDisplayNotification ever reads hasExtender().
3 participants
Description
One Line Summary
Adds a notification service extension to the Android demo, hidden unless you build with
-PSHOW_NSE_SECTION=true.Details
Motivation
Add NSE example to debug NSE issues.
Scope
examples/demoonly. No SDK source changes, no public API changes.The demo screen is unchanged in a default build.
MainScreenwraps the section inif (BuildConfig.SHOW_NSE_SECTION), resolved through the existingdemoOverride()helper (-P→local.properties→false). Only the UI is gated: the extension stays registered, so the class still compiles indemo-buildand still exercises the-keeprule inonesignal/notifications/consumer-rules.pro. The flag is a compile-time constant, so R8 drops the section from a default release build.Build with
-PSHOW_NSE_SECTION=trueto get the section. Every switch defaults to off.[Demo]NSE.NotificationCompat.Extender.IMPORTANCE_HIGHchannel.preventDefault(), thendisplay()five seconds later.preventDefault(true). Takes precedence over the others.Switches persist through
SharedPreferenceUtil, which the extension reads directly since it runs whether or not the app is open.Design notes
The channel readout uses
NotificationCompat.getChannelId(builder.build())inside the extender, the only place an extension sees the SDK's choice. ReadingchnloffrawPayloadwould report what the payload asked for, not what the SDK picked, and the two diverge on a restore. That readout is why the extender is set unconditionally, which costs nothing: an extender cannot change what displays, becauseprocessHandlerResponsegates oncanDisplaybeforeshouldDisplayNotificationever readshasExtender().event.restoringis not onINotificationReceivedEventyet, so the log line omits it. #2723 adds it, and a TODO marks the spot.A separate commit routes the demo's 127 log calls through
DemoLog, which marks both tag and message sologcat -sstill filters and a line stays recognizable in the message column. The five SDK-forwarding calls inMainApplicationstay unmarked.Testing
Unit testing
None added. Demo code with no test target, and the compile is the guard:
ci.yml'sdemo-buildbuilds:appfrom insideOneSignalSDK/, wheresettings.gradlesubstitutes the published dependency with local source.On a default release build I confirmed
DemoNotificationServiceExtensionsurvives R8 with its class-level mapping entry whileNotificationExtensionSectionis stripped to zero entries.Manual testing
Pixel 7 emulator, API 34, Google Play image,
gmsdebug against local SDK source, section shown.Each switch changes behavior: extender prefixes the title, forced channel posts on
demo_nse_high_importanceatmImportance=4, delay displays five seconds later, discard posts nothing. With the master switch off nothing is logged and notifications post normally.Two beyond the matrix. Killing the process with
adb shell am killand sending a push showed the extension firing in a fresh pid, reading its switches from SharedPreferences with noMainViewModelalive. And afteradb rebootthe same notifications came back loggingchannel=restored_OS_notificationsatimportance=2, againstfcm_fallback_notification_channelatimportance=3on first delivery, which is why the channel is read from the builder.The shared Appium suite passes on a default build, 32 passing with the 2 iOS-only Live Activities skips. It does not pass with
-PSHOW_NSE_SECTION=true: the taller screen pushesiam_info_iconpast theheight * 0.82line that triggersnudgeAboveBottomOverlayinsdk-shared'sappium/tests/helpers/app.ts, and that path throwsgetElementRectwith an undefinedelementIdpast its owncatch. The demo is fine, tapping the icon opens the right tooltip. Placement is not the cause; moving the section to the bottom of the screen fails identically. Noted inbuild.md. CI ande2e.ymlboth build the default configuration.spotlessCheck,detekt,assembleGmsRelease, andcompileHuaweiDebugKotlinall pass.Affected code checklist
Ticked because the demo extension exercises those paths. No SDK behavior changes.
Checklist
Overview
Testing
Final pass