chore: [SDK-4689] refresh demo app for E2E parity#2651
Conversation
fadi-george
changed the title
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
@claude review |
📊 Diff Coverage ReportDiff Coverage Report (Changed Lines Only)Gate: aggregate coverage on changed executable lines must be ≥ 80% (JaCoCo line data for lines touched in the diff). Changed Files Coverage
Overall (aggregate gate)13/33 touched executable lines covered (39.4% — requires ≥ 80%) Per-file detail (informational; gate is aggregate above):
❌ Coverage Check FailedAggregate coverage on touched lines is 39.4% (minimum 80%). |
There was a problem hiding this comment.
LGTM — the two critical findings from my prior review (stale-fetch logout race + observer leak in onCleared) were both addressed in 0f8ca94.
Extended reasoning...
Overview
Demo app refresh for E2E parity: package rename from com.onesignal.sdktest → com.onesignal.example (with matching updates to google-services.json / agconnect-services.json / manifest), full Compose UI rewrite into modular SectionCard + ToggleRow + ActionButton primitives with stable testTag ids, new local.properties-driven BuildConfig flow for ONESIGNAL_APP_ID / ONESIGNAL_ANDROID_CHANNEL_ID / E2E_MODE, replacement of the in-app LogView/LogManager with a forwarder to android.util.Log under tag OneSignalSDK, and a new shared build.md that points at the cross-platform shared guide. The only SDK touch is in OneSignalImp.kt and is purely ktlint-style formatting (brace style on when branches, multi-line ternaries, line-break of the supertype list) with no behavior change.
Security risks
None I see. No auth/crypto/permissions surface is altered; OneSignal.consentRequired / consentGiven plumbing in MainApplication is preserved (still set before initWithContext). The REST client (OneSignalService) was already shipping without an Authorization header for the public /users/by/... endpoint; this PR's logging cleanup doesn't change the request shape. The package-rename touch to the ADM permission name (com.onesignal.example.permission.RECEIVE_ADM_MESSAGE) and intent category line up consistently.
Level of scrutiny
Light — this is a demo-app refresh, not production SDK code. The published SDK diff is formatting-only. The fact that the author moved ++fetchRequestSequence to the top of fetchUserDataFromApi and added the missing removeObserver calls to onCleared in response to prior feedback indicates active engagement with review comments.
Other factors
Remaining inline bugs in this review pass (fetchUser URL encoding of aliasValue; stale GettingStarted.md doc drift) are both pre-existing and nit-level — flagged inline for awareness but not blockers. The WITH SOUND channel-id coupling concern from my prior review is also nit-level and unaddressed, but the value is documented in local.properties.example and surfaces a Log.e on failure. Net effect: critical findings closed, residual items are documentation/UX polish appropriate for follow-up, scope is contained to demo-app + SDK formatting.
- MainViewModel: route toasts through a buffered Channel<String> instead of LiveData so identical messages aren't deduped by Compose's structural equality and so background-thread callers (IUserJwtInvalidatedListener fires on OneSignalDispatchers.launchOnDefault) don't trip LiveData's @mainthread assertion that JwtTokenStore silently swallows. - MainViewModel: gate the auto-prompt with hasAutoPrompted so config changes (rotation, theme, font-scale) don't re-fire OneSignal's permission prompt or settings prepompt. - MainScreen: add testTag pair to the Update User JWT PairInputDialog so the inputs are addressable by Appium id= selectors like every other PairInputDialog. - MainScreen: collect MainViewModel.toastMessages and call viewModel.autoPromptPushOnce() in place of the old LiveData bridge. - OneSignalService.postNotification: re-throw CancellationException so viewModelScope teardown during the delay-between-retries actually propagates instead of being swallowed as a generic send failure. - OneSignalService.fetchUser: URL-encode the alias value so reserved chars in external_id don't silently 404. - GettingStarted.md / build.md: drop stale references to editing strings.xml, the in-app Log Viewer, and runtime App ID configuration now that local.properties + BuildConfig.ONESIGNAL_APP_ID is the contract; describe the Channel-backed snackbar and one-shot auto-prompt. Co-authored-by: Cursor <cursoragent@cursor.com>
The previous commit added `catch (e: CancellationException) { throw e }`
inside `postNotification`, but the re-thrown cancellation was caught
again one frame up by `sendNotification` / `sendCustomNotification`'s
outer `catch (e: Exception)`, silently undoing the fix. The same
anti-pattern lived in `OneSignalService.fetchUser` (wrapping
`withContext(Dispatchers.IO)` + `connection.responseCode`) and
`MainViewModel.fetchUserDataFromApi` (wrapping `repository.fetchUser`,
the inner `withContext(Dispatchers.Main)`, and `delay(100)`).
Add a `CancellationException` re-throw clause ahead of each generic
`Exception` catch so structured-concurrency cancellation propagates all
the way up to the cancelled scope instead of being swallowed as a
misleading I/O error.
Co-authored-by: Cursor <cursoragent@cursor.com>
fadi-george
force-pushed
the
fadi/sdk-4689
branch
from
e98b3fa to
d378e67
Compare
| /** @deprecated Use [OsPrimary] */ | ||
| val OneSignalRed = OsPrimary | ||
|
|
||
| /** @deprecated Use [OsSuccess] */ | ||
| val OneSignalGreen = OsSuccess | ||
|
|
||
| /** @deprecated Use [OsLightBackground] */ | ||
| val LightBackground = OsLightBackground | ||
|
|
||
| /** @deprecated Use [OsCardBackground] */ | ||
| val CardBackground = OsCardBackground | ||
|
|
||
| /** @deprecated Use [OsDivider] */ | ||
| val DividerColor = OsDivider | ||
|
|
||
| /** @deprecated Use [OsWarningBackground] */ | ||
| val WarningBackground = OsWarningBackground |
There was a problem hiding this comment.
🟡 Dead code: the six @deprecated aliases in Theme.kt:28-44 (OneSignalRed, OneSignalGreen, LightBackground, CardBackground, DividerColor, WarningBackground) have no consumers in the codebase after this PR — grep -rn across examples/demo/app/src/ shows them only at their own declaration sites. Per CLAUDE.md ("If you are certain that something is unused, you can delete it completely"), delete lines 28-44.
Extended reasoning...
What the bug is
Theme.kt:28-44 declares six @deprecated aliases pointing at the new Os* tokens:
/** @deprecated Use [OsPrimary] */
val OneSignalRed = OsPrimary
/** @deprecated Use [OsSuccess] */
val OneSignalGreen = OsSuccess
/** @deprecated Use [OsLightBackground] */
val LightBackground = OsLightBackground
/** @deprecated Use [OsCardBackground] */
val CardBackground = OsCardBackground
/** @deprecated Use [OsDivider] */
val DividerColor = OsDivider
/** @deprecated Use [OsWarningBackground] */
val WarningBackground = OsWarningBackgroundStep-by-step proof
grep -rn '\\b(OneSignalRed|OneSignalGreen|LightBackground|CardBackground|DividerColor|WarningBackground)\\b' examples/demo/app/src/ (run against the working tree) returns exactly six hits, all in Theme.kt at lines 29, 32, 35, 38, 41, 44 — the declaration sites themselves. Zero call sites anywhere else in the demo app.
The cross-checks confirm this:
MainActivity.kt:16importsOsLightBackgrounddirectly.SecondaryActivity.kt:29importsOsLightBackgrounddirectly.Sections.kt:49importsOsWarningBackgrounddirectly.- Every other consumer in the diff (
Dialogs.kt,SectionCard.kt,ActionButton.kt,ListComponents.kt,CardKvRow.kt,ToggleRow.kt,DemoAppBar.kt) imports the newOs*tokens directly.
Why the deprecation window is meaningless here
The PR also renames the package from com.onesignal.sdktest to com.onesignal.example. That rename is itself a hard break — any hypothetical external consumer referencing the old fully-qualified names (com.onesignal.sdktest.ui.theme.OneSignalRed) is already broken by the package move. Keeping deprecated aliases at the new package path serves no migration purpose: nothing external could ever have referenced them.
The demo app is also a standalone Android application, not a published library module — there is no API surface that downstream consumers depend on.
Impact
Pure code hygiene; no functional impact. Worth flagging because CLAUDE.md explicitly instructs contributors:
Avoid backwards-compatibility hacks like renaming unused _vars, re-exporting types, adding // removed comments for removed code, etc. If you are certain that something is unused, you can delete it completely.
The PR is a clean rename + UI refactor that already breaks every external import path — the deprecation aliases just add noise without serving the migration purpose the @deprecated marker implies.
Fix
Delete lines 28-44 of examples/demo/app/src/main/java/com/onesignal/example/ui/theme/Theme.kt. 17 lines removed, zero call sites to update.
| setContent { | ||
| OneSignalTheme { | ||
| Scaffold( | ||
| topBar = { | ||
| CenterAlignedTopAppBar( | ||
| DemoAppBar( | ||
| title = { Text("Secondary Screen", color = Color.White) }, | ||
| navigationIcon = { | ||
| IconButton(onClick = { finish() }) { | ||
| Icon( | ||
| imageVector = Icons.AutoMirrored.Filled.ArrowBack, | ||
| contentDescription = "Back", | ||
| tint = Color.White | ||
| tint = Color.White, | ||
| ) | ||
| } | ||
| }, | ||
| colors = TopAppBarDefaults.topAppBarColors( | ||
| containerColor = OneSignalRed | ||
| ) | ||
| ) | ||
| }, |
There was a problem hiding this comment.
🟡 SecondaryActivity diverges from MainActivity in two ways that break the CRASH/ANR flow GettingStarted.md line 141 markets for E2E: the DestructiveButton calls at SecondaryActivity.kt:77-87 pass no testTag (unlike every other interactive button in the demo), and the setContent root at line 44 is missing Modifier.semantics { testTagsAsResourceId = true } that MainActivity.kt:41-43 applies with a comment explicitly explaining why it is needed for Appium id= selectors. Two-line fix: add testTag = "crash_button" / testTag = "anr_button" to the two buttons and apply the semantics modifier to the Scaffold (or wrap it in a Surface like MainActivity does) so the new tags are addressable.
Extended reasoning...
What the bug is
SecondaryActivity.kt is in this PR's diff (package rename sdktest → example, DemoAppBar substitution, enableEdgeToEdge), but it still has two gaps that prevent the CRASH/ANR buttons from being addressable by Appium id= selectors — exactly the kind of automation-hook stability the PR description markets as its motivation.
Gap 1: testTag is missing on the two buttons
SecondaryActivity.kt:77-87:
DestructiveButton(
text = "CRASH",
onClick = { triggerCrash() }
)
Spacer(modifier = Modifier.height(16.dp))
DestructiveButton(
text = "SIMULATE ANR (10s block)",
onClick = { triggerAnr() }
)Neither call passes a testTag parameter. DestructiveButton declares testTag: String? = null in ActionButton.kt and forwards it through to OutlineButton, where the local Modifier.buttonLayout(testTag) applies Modifier.testTag(...) only when the tag is non-null. So both rendered buttons carry no Modifier.testTag at all — unlike every other button in the demo (e.g. login_user_button, prompt_push_button, send_simple_button, send_image_button, send_sound_button, send_custom_button, next_screen_button, clear_all_button, all the add_*_button / remove_*_button calls, etc.).
Gap 2: testTagsAsResourceId is missing on the root
SecondaryActivity.kt:44-91 wraps its content as:
setContent {
OneSignalTheme {
Scaffold(
topBar = { DemoAppBar(...) },
containerColor = OsLightBackground,
) { paddingValues -> Column(...) { ... } }
}
}There is no Surface wrapper and no Modifier.semantics { testTagsAsResourceId = true } modifier anywhere in this tree. By contrast MainActivity.kt:32-49 applies exactly that modifier to a root Surface, with an in-code comment at lines 35-40:
testTagsAsResourceId=true exposes every Modifier.testTag(...) in the tree as an Android resource-id, which is what UiAutomator2 / Appium's
id=fooselector resolves against. Without this, Compose only surfaces testTag as a Semantics property invisible to UiAutomator, and Appium E2E specs that look up elements viaid=time out.
Each Compose Activity has its own root ViewRootImpl, so the semantics flag set on MainActivity's Surface does not propagate to SecondaryActivity. This is the same reason Dialogs.kt exposes its own Modifier.exposeTestTagsAsResourceId() helper — Compose dialogs render into a separate window and need to re-apply the flag.
Step-by-step proof
- Appium driver navigates to the secondary screen via the
next_screen_buttonid (this works — MainActivity's root hastestTagsAsResourceId = trueandnext_screen_buttonis set atMainScreen.kt). - The driver attempts to find an element with
id=crash_button(the natural name following the existing demo convention, e.g.clear_triggers_button). DestructiveButton(text = "CRASH", onClick = { triggerCrash() })rendered noModifier.testTagat all (gap 1), so no semantics property exists — match fails.- Even if the author added
testTag = "crash_button"to that call, the SecondaryActivity's root tree has notestTagsAsResourceId = true(gap 2). Compose would surfacecrash_buttonas a Semantics property but UiAutomator2 would still not see it as an Android resource-id, soid=crash_buttonwould still time out. - The Appium spec has to fall back to a text-based selector keyed on
"CRASH"/"SIMULATE ANR (10s block)", which is brittle to copy changes and inconsistent with the rest of the suite.
Impact
Demo-only and recoverable via text-based selectors, so practical impact is bounded. Worth flagging because:
- The PR's stated motivation is "clearer organization, stable automation hooks, and test-friendly UI behavior so it can be used more reliably for E2E coverage".
examples/demo/GettingStarted.mdexplicitly markets the CRASH/ANR flow at line 141.SecondaryActivity.ktis already in this PR's diff (package rename, app-bar swap,enableEdgeToEdgeadoption), so the consistency drift is in PR scope rather than purely pre-existing.- The fix is ~3 lines and directly mirrors the pattern MainActivity already establishes with an explanatory comment.
How to fix
Three small additions:
setContent {
OneSignalTheme {
Surface(
modifier = Modifier
.fillMaxSize()
.semantics { testTagsAsResourceId = true },
color = OsLightBackground,
) {
Scaffold(...) { paddingValues ->
Column(...) {
Text("Secondary Screen", ...)
Spacer(modifier = Modifier.height(32.dp))
DestructiveButton(
text = "CRASH",
onClick = { triggerCrash() },
testTag = "crash_button",
)
Spacer(modifier = Modifier.height(16.dp))
DestructiveButton(
text = "SIMULATE ANR (10s block)",
onClick = { triggerAnr() },
testTag = "anr_button",
)
}
}
}
}
}Alternative: skip the Surface and apply Modifier.semantics { testTagsAsResourceId = true } directly to the Scaffold's modifier — either works.
| Displays the OneSignal **App ID** (set at build time via `local.properties` / `BuildConfig.ONESIGNAL_APP_ID`), toggles **privacy consent required**, and grants/revokes **privacy consent**. | ||
|
|
||
| ### User | ||
| **Login** with an external user ID or **Logout** to switch between identified and anonymous users. Displays the current OneSignal ID and external user ID. |
There was a problem hiding this comment.
🟡 GettingStarted.md was partially refreshed (App ID instructions + Log Viewer removal) in a813284 but the Features section still drifts from this PR's UI changes: line 90 claims the User section displays OneSignal ID (no such row exists), Send Push (lines 96-99) is missing WITH SOUND and CLEAR ALL, the Track Event heading at line 134 should be Custom Events (Sections.kt:499), Location (line 138) is missing CHECK LOCATION, and the User section never mentions the new Identity Verification toggle or UPDATE USER JWT button (both called out in build.md's Android Additions Beyond the Shared Guide). Doc-only nit fixable with a short edit.
Extended reasoning...
What the bug is
The partial refresh in a813284 closed the strings.xml / Log Viewer drift previously reported, but left the Features section out of sync with the UI changes this same PR shipped. Five concrete drifts remain in examples/demo/GettingStarted.md:
-
Line 90 (User) — "Displays the current OneSignal ID and external user ID" but
UserSection(Sections.kt:151-163) only renders Status and External ID rows. There is no OneSignal ID row. The PR added_oneSignalIdLiveData toMainViewModeland writes to it inonUserStateChange+refreshPushSubscription, but nothing observes it — so the doc claim is wrong either way (and is the dead-state previously flagged in bug Option to disable sound and vibrate from api #11 of the verify pipeline). -
Lines 95-99 (Send Push) — lists only Simple / With Image / Custom. The PR added two new buttons in
SendPushSection(Sections.kt:243-247): WITH SOUND (send_sound_button) and CLEAR ALL (clear_all_button). Neither is mentioned. -
Line 134 (Track Event) — heading is still
### Track Eventbut the UI section was renamed to Custom Events (CustomEventsSectionat Sections.kt:493,title = "Custom Events"at line 499). Real heading drift. -
Lines 137-138 (Location) — "Toggle location sharing on or off and prompt for location permission."
LocationSection(Sections.kt:529-530) now exposes both PROMPT LOCATION and CHECK LOCATION (check_location_buttoncallingviewModel.checkLocationShared()). CHECK LOCATION is undocumented. -
User section omissions —
UserSection(Sections.kt:142-149) includes an Identity Verification toggle (identity_verification_toggle), and at lines 184-188 an UPDATE USER JWT button (update_user_jwt_button). Both are PR-introduced, both are explicitly highlighted in the new build.md under Android Additions Beyond the Shared Guide, but neither appears in GettingStarted.md.
Step-by-step proof
- A cross-platform contributor opens
examples/demo/after this PR merges; bothbuild.md(rewritten by this PR) andGettingStarted.md(partially refreshed) live side by side. - They open GettingStarted.md and reach the User section at line 90, which promises a OneSignal ID display. They launch the demo and see only Status + External ID rows — no OneSignal ID. They wonder whether the SDK is failing to populate it or whether the row was moved.
- They scroll to Send Push at lines 95-99, see Simple / With Image / Custom documented. The actual UI has five buttons — the two new ones (WITH SOUND, CLEAR ALL) are unexplained.
- They look for Custom Events (per build.md's Android Additions list) at line 134 of GettingStarted.md and find
### Track Eventinstead. - They look for CHECK LOCATION (visible right next to PROMPT LOCATION in the UI) in the Location section at lines 137-138 and find no mention.
- They look for the Identity Verification toggle or UPDATE USER JWT button (per build.md) in the User section and find no mention.
Why existing code doesn't prevent it
The partial refresh in a813284 only touched the Quick Start / App ID parts of the doc (lines 74 / 84 / 95 in the diff) and the Logs section. The Features bullets the original verify pipeline never reported are untouched. The PR's own build.md explicitly enumerates the Android additions (Identity Verification toggle, UPDATE USER JWT button) and the section rename (Custom Events) — so the source of truth for what the demo does now is build.md, not GettingStarted.md.
Impact
Doc-only. No code path is broken; a contributor reading build.md will get the right picture. But GettingStarted.md is in this PR's diff and the PR's stated motivation is clearer organization and better support for E2E testing — leaving the Features bullets stale is an inconsistent partial refresh.
How to fix
A short edit to examples/demo/GettingStarted.md:
- Replace "Displays the current OneSignal ID and external user ID" with "Displays the current external user ID, identity-verification status, and login/logout / SWITCH USER / UPDATE USER JWT controls." (or wire up the dead
_oneSignalIdLiveData intoUserSectionif the original intent was to surface it). - Add bullets for With Sound and Clear All under Send Push.
- Rename
### Track Event→### Custom Events. - Add CHECK LOCATION to the Location bullet.
- Mention Identity Verification toggle + UPDATE USER JWT in the User entry.
Severity nit — doc-only, demo-app-only, recoverable by reading build.md. Flagged because the file is in this PR's diff, the author already updated the upper half of it in response to the earlier report, and the listed drifts trace directly to PR-introduced UI changes.
2 participants
Description
One Line Summary
Refresh the demo app structure and UI to better support E2E testing and manual validation.
Details
Motivation
The demo app needs clearer organization, stable automation hooks, and test-friendly UI behavior so it can be used more reliably for E2E coverage and SDK validation.
Scope
This updates the demo app package naming, UI components, masked value display, dialogs, notification controls, build documentation, and example configuration. SDK runtime changes are limited to formatting cleanup in
OneSignalImp.kt; no public API changes are intended.Testing
Unit testing
Not run locally while creating this PR. Changes are primarily demo app UI, docs, and testability updates.
Manual testing
Not run locally while creating this PR.
Affected code checklist
Checklist
Overview
Testing
Final pass
Made with Cursor