chore: [SDK-4689] refresh demo app for E2E parity

[fadi-george]

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

• Notifications
  • Display
  • Open
  • Push Processing
  • Confirm Deliveries
• Outcomes
• Sessions
• In-App Messaging
• REST API requests
• Public API changes

Checklist

Overview

• I have filled out all REQUIRED sections above
• PR does one thing
• Any Public API changes are explained in the PR details and conform to existing APIs

Testing

• I have included test coverage for these changes, or explained why they are not needed
• All automated tests pass, or I explained why that is not possible
• I have personally tested this on my device, or explained why that is not possible

Final pass

• Code is as readable as possible.
• I have reviewed this PR myself, ensuring it meets each checklist item

Made with Cursor

[fadi-george]

@claude review

[github-actions]

📊 Diff Coverage Report

Diff 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

• OneSignalImp.kt: 13/33 touched executable lines (39.4%) (60 touched lines in diff)
  • 20 uncovered touched lines in this file

Overall (aggregate gate)

13/33 touched executable lines covered (39.4% — requires ≥ 80%)

Per-file detail (informational; gate is aggregate above):

• OneSignalImp.kt: 39.4% (20 uncovered touched lines)

❌ Coverage Check Failed

Aggregate coverage on touched lines is 39.4% (minimum 80%).

📥 View workflow run

[claude]

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.

[claude]

🟡 Dead state introduced by this PR: MainViewModel adds _oneSignalId / oneSignalId (lines 40-41) and writes to it from onUserStateChange (line 140) and refreshPushSubscription (line 264), but nothing observes it — grep -rn 'oneSignalId\b' examples/demo/app/src/main/java/ returns only those 4 lines. Either wire viewModel.oneSignalId into UserSection (e.g. a card row with testTag = "user_onesignal_id_value" matching the existing External ID row), or drop the field and its two writes.

Extended reasoning...

What the bug is

This PR adds a new MutableLiveData<String?> for the OneSignal ID on MainViewModel:

// OneSignal ID
private val _oneSignalId = MutableLiveData<String?>()
val oneSignalId: LiveData<String?> = _oneSignalId

(MainViewModel.kt:40-41 — both lines are + lines in the diff.)

It writes to that LiveData in two places, both also added by this PR:

• Line 140 inside onUserStateChange: _oneSignalId.postValue(state.current.onesignalId) (this whole line is a + in the diff hunk for onUserStateChange).
• Line 264 inside refreshPushSubscription: _oneSignalId.value = repository.getOneSignalId() (also a + line in the diff hunk).

But the exposed LiveData is never observed.

Step-by-step proof

1. grep -rn 'oneSignalId\b' examples/demo/app/src/main/java/ (run against the working tree) returns exactly 4 hits, all in MainViewModel.kt — the declaration at lines 40-41 and the two writes at lines 140 and 264. No viewModel.oneSignalId access anywhere.
2. MainScreen.kt does observeAsState on every LiveData the UI consumes (appId, pushSubscriptionId, pushEnabled, externalUserId, etc.); it never references viewModel.oneSignalId.
3. Sections.kt renders UserSection with three rows (Identity Verification toggle, Status, External ID) — there is no OneSignal ID row. UserSection's parameter list also has no oneSignalId parameter.
4. No other consumer exists in the app: nothing in MainActivity.kt, SecondaryActivity.kt, MainApplication.kt, or anywhere else looks up viewModel.oneSignalId. The onesignal_id string elsewhere in the codebase (e.g. OneSignalService's alias label for the fetchUser endpoint) is a separate string literal, not a reference to this LiveData.

So the public oneSignalId LiveData is dead state — its two write sites compute a value that no one reads.

Why the existing code doesn't prevent it

The PR's stated goal is E2E parity with the cross-platform Appium suite. The existing UserSection already exposes user_external_id_value (a stable testTag on the External ID row in Sections.kt) — the natural intent of the new LiveData is to add a parallel user_onesignal_id_value row so specs can read the OneSignal ID without needing logcat. But the binding/Card row was never added, leaving the field unreferenced.

Impact

No functional impact — no crash, no leak, no incorrect behavior. The two write sites simply do nothing useful, and the public oneSignalId property is a no-op getter. This is purely unused code added by the PR.

How to fix

Two cheap options:

1. Surface it in the UI (likely the original intent for E2E parity). In Sections.kt's UserSection, add a third CardKvRow between Status and External ID:

   CardKvRow(
       label = "OneSignal ID",
       value = oneSignalId ?: "—",
       valueTestTag = "user_onesignal_id_value",
   )
   HorizontalDivider(color = OsDivider, modifier = Modifier.padding(vertical = DemoLayout.gap))

   …then thread oneSignalId through UserSection and observe it in MainScreen.kt (val oneSignalId by viewModel.oneSignalId.observeAsState()).

2. Remove the dead state. Delete lines 40-41, line 140's postValue, and line 264's .value assignment. Roughly a 4-line cleanup.

Either resolves the issue. Option 1 is consistent with the PR's E2E parity motivation; option 2 is the smallest possible change.

[claude]

🟡 The PR added catch (e: CancellationException) { throw e } inside postNotification (lines 170-173) to fix structured-concurrency cancellation, but the re-thrown CancellationException is caught again one frame up by sendNotification's outer catch (e: Exception) at line 79 and sendCustomNotification's outer catch (e: Exception) at line 113 — since CancellationException extends RuntimeException → Exception on the JVM, the fix is silently undone. The same anti-pattern also lives in MainViewModel.fetchUserDataFromApi (catch at MainViewModel.kt:237) which wraps three suspension points (repository.fetchUser, the inner withContext(Dispatchers.Main), and delay(100)). Two-line fix at each site: catch (e: CancellationException) { throw e } ahead of the generic catch (e: Exception).

Extended reasoning...

What is broken

The PR added the canonical structured-concurrency re-throw inside postNotification at OneSignalService.kt:170-173:

} catch (e: CancellationException) {
    throw e
} catch (e: Exception) {
    Log.e(TAG, "Send $label error: ${e.message}")
    return false
}

But the callers — sendNotification (catch at lines 79-82) and sendCustomNotification (catch at lines 113-116) — wrap their call to postNotification(...) in a bare catch (e: Exception). On the JVM, kotlinx.coroutines.CancellationException is typealiased to java.util.concurrent.CancellationException, which extends IllegalStateException → RuntimeException → Exception. So the re-thrown cancellation propagates one stack frame up and is caught again — logging a misleading "Error sending [custom] notification" and returning false. The PR-added throw e accomplishes nothing visible at the caller layer.

The exact same anti-pattern lives in MainViewModel.fetchUserDataFromApi (MainViewModel.kt:237). The try block at line 203 contains three independent cancellation points: repository.fetchUser(...) at line 204, the inner withContext(Dispatchers.Main) { ... } context switch at line 205, and kotlinx.coroutines.delay(100) at line 232. The bare catch (e: Exception) at line 237 catches CancellationException from any of them, emits Log.e("MainViewModel", "Error fetching user data", e) with a misleading JobCancellationException stack trace, then attempts another withContext(Dispatchers.Main) { ... } on the already-cancelled scope (which itself throws CancellationException silently swallowed by viewModelScope.launch).

Step-by-step proof (sendNotification)

1. User taps Send Push. viewModelScope.launch(IO) { repository.sendNotification(SIMPLE) } invokes OneSignalService.sendNotification.
2. sendNotification builds the JSON and calls postNotification(notificationJson, "notification").
3. Attempt gradle? #1 returns HTTP 200 with {"errors":{"invalid_player_ids":[...]}}. isTransientSendFailure returns true, so the loop calls delay(2_000) and suspends.
4. User rotates the device → MainViewModel.onCleared() fires → viewModelScope is cancelled.
5. delay throws CancellationException.
6. postNotification's catch (CancellationException) { throw e } at line 170-173 correctly re-throws it.
7. The cancellation propagates up into sendNotification's catch (e: Exception) at line 79.
8. Line 80 fires: Log.e(TAG, "Error sending notification", e) — misleading log, since cancellation is not an error.
9. Line 81 returns false. The caller (MainViewModel.sendNotification) sees a normal failure return.

Same trace applies to sendCustomNotification via the CUSTOM button.

Why existing code doesn't prevent it

CancellationException is a regular Exception subclass at the JVM type-system level, so there's no language-level way for a catch (e: Exception) to skip it. The only fix is an explicit catch (e: CancellationException) { throw e } ordered ahead. The PR demonstrates that the author knows this pattern — they used it inside postNotification — they just missed the outer wrappers and the parallel call site in MainViewModel.

Impact

• Logcat noise. Every cancellation during a Send Push retry sleep or an in-flight user fetch fires a Log.e line that looks like a real error.
• Misleading JobCancellationException stack traces in the fetchUserDataFromApi catch — the kind that send developers chasing a bug that isn't there.
• Defeats the PR's stated fix. The PR description for the structured-concurrency change (and the comment at lines 170-173) explicitly says the goal is to not swallow cancellation; the outer wrapper cancels that out.
• This is the canonical wrong pattern in coroutines code, and the demo is the template other wrapper-SDK demos copy.

Fix

Two lines at each of the three sites. For example, in sendNotification:

return@withContext postNotification(notificationJson, "notification")
} catch (e: CancellationException) {
    throw e
} catch (e: Exception) {
    Log.e(TAG, "Error sending notification", e)
    return@withContext false
}

Same shape for sendCustomNotification at line 113 and for MainViewModel.fetchUserDataFromApi at line 237 (importing kotlinx.coroutines.CancellationException in MainViewModel.kt).

Severity rationale

nit — matches every verifier's recommendation. Demo-only code, narrow trigger window (cancellation during a retry sleep or in-flight fetch), and the user-visible artifact is one or two misleading log lines per cancellation event with no app-level state corruption (viewModelScope.launch silently absorbs the re-asserted cancellation downstream). Worth flagging because the PR explicitly committed to this fix pattern inside postNotification and missed the wrappers, and because the demo is the canonical template for cross-platform Appium parity that other wrapper SDK demos copy.