Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 5 additions & 7 deletions .github/actions/setup-demo/action.yml
Original file line number Diff line number Diff line change
Expand Up @@ -20,9 +20,6 @@ runs:
cache: true
run-install: true

- name: Set up Bun
uses: oven-sh/setup-bun@v2

- name: Cache bun dependencies
uses: actions/cache@v5
with:
Expand All @@ -34,8 +31,8 @@ runs:
shell: bash
working-directory: examples/demo
run: |
bun run setup
bun install
vp run setup
vp install

- name: Cache CocoaPods
if: inputs.install-pods == 'true'
Expand All @@ -48,8 +45,8 @@ runs:
- name: Update CocoaPods
if: inputs.install-pods == 'true'
shell: bash
working-directory: examples/demo
run: bun run update:pods
working-directory: examples/demo/ios
run: pod update OneSignalXCFramework --no-repo-update

- name: Create demo .env
shell: bash
Expand All @@ -58,3 +55,4 @@ runs:
echo "ONESIGNAL_APP_ID=${{ inputs.onesignal-app-id }}" > .env
echo "ONESIGNAL_API_KEY=${{ inputs.onesignal-api-key }}" >> .env
echo "E2E_MODE=true" >> .env
echo "ONESIGNAL_ANDROID_CHANNEL_ID=7ec2ece9-c538-4656-9516-1316f48a005c" >> .env
63 changes: 56 additions & 7 deletions .github/workflows/create-release-pr.yml
Original file line number Diff line number Diff line change
Expand Up @@ -92,8 +92,8 @@

# Get versions from target branch (not the release branch)
CURRENT_VERSION=$(git show origin/${{ inputs.target_branch }}:package.json | jq -r .version)
ANDROID_VERSION=$(git show origin/${{ inputs.target_branch }}:android/build.gradle | grep "com.onesignal:OneSignal:" | sed -E "s/.*OneSignal:([0-9.]+).*/\1/")
IOS_VERSION=$(git show origin/${{ inputs.target_branch }}:react-native-onesignal.podspec | grep "OneSignalXCFramework" | sed -E "s/.*'([0-9.]+)'.*/\1/")
ANDROID_VERSION=$(git show origin/${{ inputs.target_branch }}:android/build.gradle | grep "com.onesignal:OneSignal:" | sed -E "s/.*OneSignal:([^\"']+).*/\1/")
IOS_VERSION=$(git show origin/${{ inputs.target_branch }}:react-native-onesignal.podspec | grep "OneSignalXCFramework" | sed -E "s/.*'([^']+)'.*/\1/")

echo "rn_from=$CURRENT_VERSION" >> $GITHUB_OUTPUT
echo "android_from=$ANDROID_VERSION" >> $GITHUB_OUTPUT
Expand All @@ -105,20 +105,23 @@
VERSION="${{ inputs.android_version }}"

# Validate version exists on GitHub
RELEASE=$(curl -s -H "Authorization: token ${{ github.token }}" \
"https://api.github.com/repos/OneSignal/OneSignal-Android-SDK/releases/tags/${VERSION}")
# -sf: silent + fail on HTTP >= 400 so RELEASE stays empty
# on 404, otherwise GitHub's JSON error body would defeat the
# `[ -z ]` guard below.
RELEASE=$(curl -sf -H "Authorization: token ${{ github.token }}" \
"https://api.github.com/repos/OneSignal/OneSignal-Android-SDK/releases/tags/${VERSION}" || true)


if [ -z "$RELEASE" ]; then
echo "✗ Android SDK version ${VERSION} not found"
exit 1
fi

# Update Android SDK version in build.gradle (handles both api '...' and api('...') syntax)
sed -i '' -E "s/(com\.onesignal:OneSignal:)[0-9.]+/\1$VERSION/" android/build.gradle
sed -i '' -E "s/(com\.onesignal:OneSignal:)[^\"']+/\1$VERSION/" android/build.gradle
echo "✓ Updated android/build.gradle with Android SDK ${VERSION}"

# Only commit if there are changes

Check warning on line 124 in .github/workflows/create-release-pr.yml

View check run for this annotation

Claude / Claude Code Review

Parallel Android/iOS SDK seds lack the post-substitution verification just added to wrapper version

Forward-looking nit: this PR added `grep -q` post-substitution verification to the wrapper-version sed in `Update sdk version` (lines 170-173, 178-181), but the parallel `Update Android SDK version` (line 121, which this PR just touched to broaden `[0-9.]+` → `[^"']+`) and `Update iOS SDK version` (line 146) seds still rely on the unverified `sed → echo "✓ Updated" → git diff --staged --quiet && exit 0` pattern. BSD `sed -i` exits 0 on zero matches, so a future reformat of `android/build.gradle`
Comment on lines 119 to 124
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Forward-looking nit: this PR added grep -q post-substitution verification to the wrapper-version sed in Update sdk version (lines 170-173, 178-181), but the parallel Update Android SDK version (line 121, which this PR just touched to broaden [0-9.]+[^"']+) and Update iOS SDK version (line 146) seds still rely on the unverified sed → echo "✓ Updated" → git diff --staged --quiet && exit 0 pattern. BSD sed -i exits 0 on zero matches, so a future reformat of android/build.gradle (e.g. catalog/multi-line) or the podspec (e.g. s.dependency('OneSignalXCFramework', "5.5.1")) would silently no-op while the log lies — and since neither step has a prior mutation, the staged diff guard fires exit 0 cleanly, leaving the release with a bumped package.json but a stale native pin. Current files match losslessly so no immediate impact; mirroring the existing wrapper-version grep -qF pattern to both earlier seds (two lines per step) closes the same class of bug symmetrically.

Extended reasoning...

What is asymmetric

This PR closed a silent-sed-miss gap in the wrapper-version block at .github/workflows/create-release-pr.yml:168-181 by adding post-substitution grep -q checks:

sed -i '' -E "s/(OneSignalWrapper\.setSdkVersion\(\")[0-9]+(\"\))/\1${PADDED_VERSION}\2/" "$ANDROID_FILE"
if ! grep -q "OneSignalWrapper.setSdkVersion(\"${PADDED_VERSION}\")" "$ANDROID_FILE"; then
  echo "::error::Failed to update wrapper version in ${ANDROID_FILE} to ${PADDED_VERSION}"
  exit 1
fi

But the structurally-identical seds in the two earlier steps in the same job — both of which this PR actively touched (broadened the regexes from [0-9.]+ to [^"']+ / [^']+) — got no analogous verification:

  • Line 121 (Update Android SDK version): sed -i '' -E "s/(com\.onesignal:OneSignal:)[^\"']+/\1$VERSION/" android/build.gradle followed only by an unconditional echo "✓ Updated …" and git add -A; git diff --staged --quiet && exit 0.
  • Line 146 (Update iOS SDK version): sed -i '' "s/s\.dependency 'OneSignalXCFramework', '[^']*'/.../" react-native-onesignal.podspec with the same unverified pattern.

Why the existing guard does not save these

BSD sed -i '' (job runs on macos-latest) exits 0 even when the pattern matches zero lines. The echo "✓ Updated …" is unconditional so the log lies. And critically, unlike the wrapper-version step, neither of these earlier steps has a prior mutation like npm pkg set version. So when the sed misses, the staged diff is genuinely empty and git diff --staged --quiet && exit 0 exits the step cleanly with success status — no commit, no error, no signal that anything went wrong.

Failure mode (Android — most exposed)

  1. Future contributor reformats android/build.gradle — e.g. moves the dep into a libs.versions.toml catalog and references it as api libs.onesignal, or splits the literal across multiple lines.
  2. Operator triggers the workflow with android_version: 5.9.0. The new curl -sf validates that release exists. Good.
  3. sed runs against the reformatted build.gradle. The com.onesignal:OneSignal:[^"']+ pattern matches zero lines. BSD sed -i exits 0.
  4. echo "✓ Updated android/build.gradle with Android SDK 5.9.0" — workflow log claims success.
  5. git add -A; git diff --staged --quiet && exit 0 — staged diff empty, step exits 0.
  6. Workflow proceeds. Update sdk version bumps package.json + wrapper literal. Refresh demo Podfile.lock regenerates the lock against the still-stale gradle line. Final commits pushed.
  7. rel/5.9.0 ships with package.json at 5.9.0 but android/build.gradle still pinning the prior Android SDK — exactly the failure mode the wrapper-version verification was added to prevent.

The iOS branch is structurally more brittle: the regex requires exact s.dependency + space + single-quoted args + comma-space syntax. A future podspec refactor to parens (s.dependency('OneSignalXCFramework', '5.5.1')) or double-quoted version produces zero matches with the same silent-exit-0 outcome. The downstream Refresh demo Podfile.lock step does not catch it either — pod install resolves against the stale podspec and produces a self-consistent lock at the old version.

Why nothing else catches it

  • curl -sf validates the GitHub release tag exists; it has no opinion on whether sed matched.
  • The downstream pod install resolves whatever the podspec says, so a missed iOS sed produces a self-consistent (but stale) lock.
  • e2e on rel/** may pass if the previously-pinned native SDK is still live on Maven Central / CocoaPods trunk.

Step-by-step proof — Android branch

  1. Reformat the Android dep declaration (any of: catalog reference, multi-line interpolation, removed quotes around version) so the literal com.onesignal:OneSignal:5.8.0 no longer appears on a single line in android/build.gradle.
  2. Workflow runs with android_version: 5.9.0.
  3. sed -i '' -E "s/(com\.onesignal:OneSignal:)[^\"']+/\15.9.0/" android/build.gradle — pattern matches zero lines. BSD sed exits 0.
  4. echo "✓ Updated android/build.gradle with Android SDK 5.9.0" — log says success.
  5. git add -A — nothing new to stage.
  6. git diff --staged --quiet && exit 0 — empty diff, exits 0.
  7. Step status: success. No human-visible signal of the miss.
  8. Subsequent steps run normally and commit/push the bumped package.json and wrapper literal.

How to fix

Mirror the wrapper-version pattern (~4 lines per step):

sed -i '' -E "s/(com\.onesignal:OneSignal:)[^\"']+/\1$VERSION/" android/build.gradle
if ! grep -qF "com.onesignal:OneSignal:${VERSION}" android/build.gradle; then
  echo "::error::Failed to update android/build.gradle to OneSignal SDK ${VERSION}"
  exit 1
fi
echo "✓ Updated android/build.gradle with Android SDK ${VERSION}"

and analogously for the iOS podspec sed. grep -qF (fixed-string) is sufficient since we are matching the exact value just substituted in.

Severity

nit — current pins (Android 5.8.0, iOS 5.5.1) match the regexes losslessly so there is no immediate impact. Worth flagging because (a) this PR has been twice-revised to harden this very script (curl -sf, wrapper-version grep -q), and (b) the parallel gap in the two earlier seds leaves the same class of silent-drift bug open. Mirroring the new pattern symmetrically while it is fresh costs 4 lines per step.

git add -A
git diff --staged --quiet && exit 0
git commit -m "Update Android SDK to ${VERSION}" && git push
Expand All @@ -129,8 +132,11 @@
VERSION="${{ inputs.ios_version }}"

# Validate version exists on GitHub
RELEASE=$(curl -s -H "Authorization: token ${{ github.token }}" \
"https://api.github.com/repos/OneSignal/OneSignal-iOS-SDK/releases/tags/${VERSION}")
# -sf: silent + fail on HTTP >= 400 so RELEASE stays empty
# on 404, otherwise GitHub's JSON error body would defeat the
# `[ -z ]` guard below.
RELEASE=$(curl -sf -H "Authorization: token ${{ github.token }}" \
"https://api.github.com/repos/OneSignal/OneSignal-iOS-SDK/releases/tags/${VERSION}" || true)

if [ -z "$RELEASE" ]; then
echo "✗ iOS SDK version ${VERSION} not found"
Expand All @@ -152,11 +158,54 @@
# Update package.json version
npm pkg set version="$NEW_VERSION"

# Update the wrapper version literal reported to OneSignal's backend.
# Format is MMmmpp (zero-padded major/minor/patch); strip any pre-release suffix.
CORE_VERSION=${NEW_VERSION%%-*}
CORE_VERSION=${CORE_VERSION%%+*}
IFS='.' read -r MAJOR MINOR PATCH <<< "$CORE_VERSION"
PADDED_VERSION=$(printf "%02d%02d%02d" "$MAJOR" "$MINOR" "$PATCH")

ANDROID_FILE=android/src/main/java/com/onesignal/rnonesignalandroid/RNOneSignal.java
sed -i '' -E "s/(OneSignalWrapper\.setSdkVersion\(\")[0-9]+(\"\))/\1${PADDED_VERSION}\2/" "$ANDROID_FILE"
if ! grep -q "OneSignalWrapper.setSdkVersion(\"${PADDED_VERSION}\")" "$ANDROID_FILE"; then
echo "::error::Failed to update wrapper version in ${ANDROID_FILE} to ${PADDED_VERSION}"
exit 1
fi
echo "✓ Updated RNOneSignal.java wrapper version to ${PADDED_VERSION}"

IOS_FILE=ios/RCTOneSignal/RCTOneSignal.mm
sed -i '' -E "s/(OneSignalWrapper\.sdkVersion = @\")[0-9]+(\";)/\1${PADDED_VERSION}\2/" "$IOS_FILE"
if ! grep -q "OneSignalWrapper.sdkVersion = @\"${PADDED_VERSION}\";" "$IOS_FILE"; then
echo "::error::Failed to update wrapper version in ${IOS_FILE} to ${PADDED_VERSION}"
exit 1
fi
echo "✓ Updated RCTOneSignal.mm wrapper version to ${PADDED_VERSION}"

Comment thread
fadi-george marked this conversation as resolved.
# Only commit if there are changes
git add -A
git diff --staged --quiet && exit 0
git commit -m "Release $NEW_VERSION" && git push

- name: Refresh demo Podfile.lock
run: |
# Runs after all version edits (package.json + podspec) so the
# repacked SDK tarball and OneSignalXCFramework pin both reflect
# the new release. pod install regenerates Podfile.lock entries
# for the path-based react-native-onesignal pod and any changed
# native pins in a single resolve.
(
cd examples/demo
vp run setup
cd ios
pod install
)
echo "✓ Refreshed examples/demo/ios/Podfile.lock"

# Only commit if there are changes
git add -A

Check warning on line 205 in .github/workflows/create-release-pr.yml

View check run for this annotation

Claude / Claude Code Review

Refresh demo Podfile.lock can race CocoaPods CDN propagation

Forward-looking nit: the new "Refresh demo Podfile.lock" step at create-release-pr.yml:189-207 runs `pod install` (no `--repo-update`, no preceding wait gate) seconds after the prior "Update iOS SDK version" step bumped the OneSignalXCFramework pin via sed. If a future iOS SDK release is `pod trunk push`-ed minutes before this workflow runs and the CocoaPods CDN has not yet indexed it, `pod install` fails with "Unable to find a specification for OneSignalXCFramework (= X.Y.Z)" — but only after t
Comment on lines +196 to +205
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Forward-looking nit: the new "Refresh demo Podfile.lock" step at create-release-pr.yml:189-207 runs pod install (no --repo-update, no preceding wait gate) seconds after the prior "Update iOS SDK version" step bumped the OneSignalXCFramework pin via sed. If a future iOS SDK release is pod trunk push-ed minutes before this workflow runs and the CocoaPods CDN has not yet indexed it, pod install fails with "Unable to find a specification for OneSignalXCFramework (= X.Y.Z)" — but only after the prior Update Android SDK version / Update iOS SDK version / Update sdk version steps have already committed and pushed package.json + podspec + wrapper-version literals to rel/X.Y.Z, leaving the release branch in a half-finished state requiring manual recovery. No immediate impact for 5.4.4 (5.5.1 has long since propagated through trunk) — but the new step is the symmetric iOS counterpart of the wait-for-maven-artifact gate this PR added on the Android side, and it would benefit from a wait-for-pod-trunk step (or equivalent retry loop) for the same reason. Note --repo-update is largely a no-op for CDN-backed trunk on CocoaPods 1.7+, so a wait/retry is the cleaner fix.

Extended reasoning...

What is wrong

The new "Refresh demo Podfile.lock" step at .github/workflows/create-release-pr.yml:189-207 runs pod install immediately after the preceding "Update iOS SDK version" step (lines 130-156) sed-bumps the OneSignalXCFramework pin in react-native-onesignal.podspec. There is no --repo-update, no pod repo update, and no wait-for-pod-trunk gate between the bump and the install. The only validation upstream is the curl -sf GitHub-release-tag check at line 136 — but that endpoint has no opinion on whether the pod has propagated through the CocoaPods CDN.

This is the same class of race the verifier flagged for e2e.yml:60 (the asymmetric Android Maven Central wait vs. no iOS wait), but at a different file (this PR's newly introduced step) and a different (tighter) race window. The previous comment was scoped to e2e.yml; the fix for that file does not automatically cover this one.

Code path that triggers it

  1. Operator triggers create-release-pr.yml with ios_version: X.Y.Z shortly after the iOS SDK team pod trunk push-ed OneSignalXCFramework X.Y.Z.
  2. "Update Android SDK version" step (if applicable) commits and pushes the Android bump to rel/X.Y.Z.
  3. "Update iOS SDK version" step validates the GitHub release tag exists (succeeds — the tag was created at pod trunk push time), seds the podspec to s.dependency 'OneSignalXCFramework', 'X.Y.Z', commits and pushes.
  4. "Update sdk version" step bumps package.json, the wrapper-version literals in RNOneSignal.java and RCTOneSignal.mm, commits and pushes.
  5. "Refresh demo Podfile.lock" step runs vp run setup then pod install against the freshly-sed-ed podspec.
  6. CocoaPods asks the CDN for OneSignalXCFramework/X.Y.Z.podspec.json. If CDN propagation has not completed (the spec was just registered with trunk), CocoaPods errors with Unable to find a specification for OneSignalXCFramework (= X.Y.Z) and pod install exits non-zero. Under set -e the run-block aborts.
  7. git add -A; git diff --staged --quiet && exit 0; git commit ... never runs, so this step exits without committing the lock — but the prior three steps have already pushed package.json + podspec + wrapper-version commits to rel/X.Y.Z. The release branch is now in a half-finished state.

Why nothing prevents it

  • The GitHub-tag check at line 136 (curl -sf .../releases/tags/${VERSION}) validates only that the GitHub release exists — orthogonal to CDN indexing.
  • pod install with no flags uses the CocoaPods 1.7+ CDN by default (see examples/demo/ios/Podfile declares no source). --repo-update is largely a no-op for CDN-backed trunk because there is no local spec repo to refresh — this is why a retry/wait is the substantive fix.
  • The "Refresh demo Podfile.lock" step is new in this PR; no equivalent backstop existed before.
  • The Android side already has a symmetric protection: this PR adds wait-for-maven-artifact in e2e.yml. The iOS side now has the same kind of race exposure in create-release-pr.yml with no analogous gate.

Why this is genuinely a nit (not a blocker) and not a duplicate

  • Duplicate refutation: The synthesis agent has already considered the prior e2e.yml:60 comment — the substance there was about a different file (e2e.yml, on push: rel/**) and a different pod install callsite (inside setup-demo). This bug is about a new step in create-release-pr.yml introduced by this PR. The fix is localized to each callsite; addressing the e2e.yml comment does not automatically harden this step.
  • Proposed fix being partially wrong: The refutation rightly notes that --repo-update is mostly a no-op for CDN-backed trunk. That observation is correct and informs the recommended fix: a wait-for-pod-trunk step (mirroring the new wait-for-maven-artifact step) or a pod search/retry loop is the substantive remedy, not --repo-update per se.
  • Operationally narrow: CocoaPods CDN propagation is typically sub-minute, and OneSignal iOS SDK 5.5.1 has long since propagated, so this PR's 5.4.4 cut will not flake. The race exists for future iOS bumps where the wrapper PR is triggered close to the iOS SDK's pod trunk push.
  • Recoverable: A failure halts before create-pr runs. The operator can re-run; a second attempt succeeds once CDN propagation completes. Not release-blocking — hence nit.

How to fix

Add a wait-for-pod-trunk step before pod install, mirroring the wait-for-maven-artifact step this PR added on the Android side, e.g.:

- name: Resolve OneSignal iOS SDK version
  id: ios-sdk-version
  run: |
    VERSION=$(grep "OneSignalXCFramework" react-native-onesignal.podspec | sed -E "s/.*'([^']+)'.*/\1/")
    echo "version=${VERSION}" >> "$GITHUB_OUTPUT"

- name: Wait for OneSignalXCFramework on CocoaPods trunk
  uses: OneSignal/sdk-shared/.github/actions/wait-for-pod-trunk@main
  with:
    pod: OneSignalXCFramework
    version: ${{ steps.ios-sdk-version.outputs.version }}

If no wait-for-pod-trunk action exists in OneSignal/sdk-shared, an inline retry loop works equally well:

for i in 1 2 3 4 5; do
  if pod install; then break; fi
  echo "pod install failed (attempt $i); waiting 30s for CDN propagation"
  sleep 30
done

Step-by-step proof

Suppose the iOS SDK team pod trunk push-es OneSignalXCFramework 5.6.0 at T+0. The wrapper releaser triggers create-release-pr.yml with ios_version: 5.6.0 at T+45s.

  1. T+45s — "Update iOS SDK version" step runs. curl -sf .../releases/tags/5.6.0 returns 200 (GitHub tag was created at trunk push time). sed rewrites the podspec to '5.6.0'. git commit -m "Update iOS SDK to 5.6.0" && git pushrel/X.Y.Z now has the podspec bump committed.
  2. T+50s — "Update sdk version" step bumps package.json to the new RN version, seds the wrapper-version literals in RNOneSignal.java and RCTOneSignal.mm, commits and pushes.
  3. T+55s — "Refresh demo Podfile.lock" step runs vp run setup (re-packs the SDK tarball with the new wrapper version) then cd ios; pod install.
  4. CocoaPods queries https://cdn.cocoapods.org/all_pods_versions_*.txt and https://cdn.cocoapods.org/Specs/.../OneSignalXCFramework/5.6.0/OneSignalXCFramework.podspec.json. The version-shard file has not finished propagating. CocoaPods reports Unable to find a specification for OneSignalXCFramework (= 5.6.0)``. Exit non-zero.
  5. Under set -e, the run-block aborts. git diff --staged --quiet && exit 0; git commit ... && git push never executes, so the Podfile.lock refresh is not committed. But the prior three steps have already pushed package.json + podspec + wrapper-version literal commits to rel/X.Y.Z.
  6. The release branch is now half-finished: podspec at 5.6.0, wrapper version bumped, but examples/demo/ios/Podfile.lock still pinned to the previous OneSignalXCFramework version. create-pr (the next job) does not run because update_version failed. Operator must either re-trigger the workflow (will succeed after CDN propagation completes) or manually push the missing lock commit.

Severity

nit — narrow, forward-looking race window. CocoaPods trunk indexing is typically fast and 5.5.1 has long since propagated, so no immediate impact for this PR. But the new step is the symmetric iOS counterpart of the wait-for-maven-artifact Android gate this PR introduces, and the same defensive treatment makes the asymmetry intentional rather than accidental.

🔬 also observed by previous_4

git diff --staged --quiet && exit 0
git commit -m "Update demo Podfile.lock" && git push

create-pr:
needs: [prep, update_version]
uses: OneSignal/sdk-shared/.github/workflows/create-release.yml@main
Expand Down
20 changes: 17 additions & 3 deletions .github/workflows/e2e.yml
Original file line number Diff line number Diff line change
Expand Up @@ -46,15 +46,29 @@ jobs:
onesignal-app-id: ${{ vars.APPIUM_ONESIGNAL_APP_ID }}
onesignal-api-key: ${{ secrets.APPIUM_ONESIGNAL_API_KEY }}

- name: Resolve OneSignal Android SDK version
id: android-sdk-version
run: |
VERSION=$(grep "com.onesignal:OneSignal:" android/build.gradle | sed -E "s/.*OneSignal:([^\"']+).*/\1/")
echo "version=${VERSION}" >> "$GITHUB_OUTPUT"
Comment thread
fadi-george marked this conversation as resolved.

- name: Wait for OneSignal Android SDK on Maven Central
uses: OneSignal/sdk-shared/.github/actions/wait-for-maven-artifact@main
with:
version: ${{ steps.android-sdk-version.outputs.version }}

- name: Build release APK
Comment thread
fadi-george marked this conversation as resolved.
working-directory: examples/demo/android
run: ./gradlew assembleRelease --quiet --console=plain --warning-mode=summary
# -PciSingleAbi → arm64-v8a-only APK (~10 MB) instead of the
# ~30 MB universal APK. BrowserStack devices we test against
# are all arm64-v8a.
run: ./gradlew assembleRelease -PciSingleAbi --quiet --console=plain --warning-mode=summary

- name: Upload APK
uses: actions/upload-artifact@v7
with:
name: demo-apk
path: examples/demo/android/app/build/outputs/apk/release/app-release.apk
path: examples/demo/android/app/build/outputs/apk/release/app-arm64-v8a-release.apk
retention-days: 1
compression-level: 0

Expand Down Expand Up @@ -139,7 +153,7 @@ jobs:
with:
platform: android
app-artifact: demo-apk
app-filename: app-release.apk
app-filename: app-arm64-v8a-release.apk
sdk-type: react-native
build-name: react-native-android-${{ github.ref_name }}-${{ github.run_number }}

Expand Down
4 changes: 2 additions & 2 deletions android/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -36,10 +36,10 @@ dependencies {
// api is used instead of implementation so the parent :app project can access any of the OneSignal Java
// classes if needed. Such as com.onesignal.NotificationExtenderService
//
// Exclude OkHttp from OneSignal's transitive deps: the 5.7.x otel module pulls in OkHttp 5.x
// Exclude OkHttp from OneSignal's transitive deps: the otel module pulls in OkHttp 5.x
// (via opentelemetry-exporter-sender-okhttp) which is binary-incompatible with React Native's
// networking stack (okhttp3.internal.Util removed in 5.x). React Native already provides OkHttp 4.x.
api('com.onesignal:OneSignal:5.7.7') {
api('com.onesignal:OneSignal:5.8.0') {
exclude group: 'com.squareup.okhttp3', module: 'okhttp'
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -230,7 +230,7 @@ public void invalidate() {
@Override
public void initialize(String appId) {
OneSignalWrapper.setSdkType("reactnative");
OneSignalWrapper.setSdkVersion("050213");
OneSignalWrapper.setSdkVersion("050404");

if (oneSignalInitDone) {
Logging.debug("Already initialized the OneSignal React-Native SDK", null);
Expand Down
4 changes: 4 additions & 0 deletions examples/demo/.env.example
Original file line number Diff line number Diff line change
Expand Up @@ -2,3 +2,7 @@
ONESIGNAL_APP_ID=your-onesignal-app-id
ONESIGNAL_API_KEY=your-onesignal-api-key
E2E_MODE=false

# Optional: Android Notification Channel ID for the WITH SOUND test notification.
# Create one in your OneSignal dashboard under Settings > Android Notification Categories.
ONESIGNAL_ANDROID_CHANNEL_ID=

Check notice on line 8 in examples/demo/.env.example

View check run for this annotation

Claude / Claude Code Review

.env.example placeholder defeats documented default-app-id fallback

Pre-existing nit (lines 1–2 aren't in the diff, but this PR adds an adjacent line that pairs cleanly with the correct pattern, so it's a natural fold-in). `.env.example:1` documents "Default App ID (used if ONESIGNAL_APP_ID is empty): 77e32082-…" but `.env.example:2` ships `ONESIGNAL_APP_ID=your-onesignal-app-id` (non-empty), and the fallback at `useOneSignal.ts:32` is `ONESIGNAL_APP_ID?.trim() || DEFAULT_APP_ID` — only triggers on empty/whitespace, not on the literal placeholder. So a developer
Comment on lines +5 to +8
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟣 Pre-existing nit (lines 1–2 aren't in the diff, but this PR adds an adjacent line that pairs cleanly with the correct pattern, so it's a natural fold-in). .env.example:1 documents "Default App ID (used if ONESIGNAL_APP_ID is empty): 77e32082-…" but .env.example:2 ships ONESIGNAL_APP_ID=your-onesignal-app-id (non-empty), and the fallback at useOneSignal.ts:32 is ONESIGNAL_APP_ID?.trim() || DEFAULT_APP_ID — only triggers on empty/whitespace, not on the literal placeholder. So a developer who runs cp .env.example .env verbatim ends up calling OneSignal.initialize("your-onesignal-app-id") (which OneSignal rejects) instead of getting the documented default. Fix: ship ONESIGNAL_APP_ID= (matching the new ONESIGNAL_ANDROID_CHANNEL_ID= on line 8) so the fallback actually kicks in, or drop the "used if empty" comment. CI is unaffected (setup-demo writes a real APP_ID); only local-dev onboarding bites.

Extended reasoning...

What is wrong

Three pieces of code disagree about whether ONESIGNAL_APP_ID is a "must-be-replaced placeholder" or a "may-be-empty fallback":

  1. examples/demo/.env.example:1# Default App ID (used if ONESIGNAL_APP_ID is empty): 77e32082-ea27-42e3-a898-c72e141824ef. Comment says the fallback kicks in when the value is empty.
  2. examples/demo/.env.example:2ONESIGNAL_APP_ID=your-onesignal-app-id. Non-empty placeholder.
  3. examples/demo/src/hooks/useOneSignal.ts:32return ONESIGNAL_APP_ID?.trim() || DEFAULT_APP_ID;. The || only short-circuits on empty/whitespace, not on the literal placeholder string.

The result: a developer who follows the documented onboarding flow (cp .env.example .env and run) does not get the default UUID — they get the literal string your-onesignal-app-id passed straight into OneSignal.initialize(), which the API rejects.

Addressing the refutation

The refuting verifier argues the file deliberately mixes two conventions: placeholder-must-replace for required values (ONESIGNAL_APP_ID, ONESIGNAL_API_KEY) and empty-with-default for optional values (ONESIGNAL_ANDROID_CHANNEL_ID). That framing is reasonable on its face, but it does not fit ONESIGNAL_APP_ID specifically, because line 1 of this very file explicitly documents that the value has an empty-triggered fallback (used if ONESIGNAL_APP_ID is empty: 77e32082-…). If ONESIGNAL_APP_ID were truly required, that comment would be wrong; if the comment is right, line 2 should be empty. They cannot both be correct, which is the contradiction.

Why this PR is the natural place to fix it

Lines 1–2 are pre-existing — not in the diff. But this PR adds ONESIGNAL_ANDROID_CHANNEL_ID= on line 8, which is the correct defined-but-empty pattern that pairs cleanly with ONESIGNAL_ANDROID_CHANNEL_ID?.trim() || ... at OneSignalApiService.ts:53 (also added by this PR). So the same pattern is now used both ways within the same 8-line file: line 8 does it right, line 2 does not. That makes the line 2 inconsistency more visible than it was before this PR, and a one-character fold-in (ONESIGNAL_APP_ID=) aligns the two.

Impact

  • CI: unaffected. .github/actions/setup-demo/action.yml writes a non-empty ONESIGNAL_APP_ID from a workflow secret, so e2e never reads .env.example verbatim.
  • End users: unaffected. They install via npm; examples/demo is internal.
  • Local-dev onboarding: the documented cp .env.example .env flow does not produce the documented default. New contributors hit OneSignal.initialize rejection and have to debug why "the default is supposed to kick in".

How to fix

Either:

-# Default App ID (used if ONESIGNAL_APP_ID is empty): 77e32082-ea27-42e3-a898-c72e141824ef
-ONESIGNAL_APP_ID=your-onesignal-app-id
+# Default App ID (used if ONESIGNAL_APP_ID is empty): 77e32082-ea27-42e3-a898-c72e141824ef
+ONESIGNAL_APP_ID=

(matches line 8 ONESIGNAL_ANDROID_CHANNEL_ID= pattern), or drop the "used if empty" comment entirely and tell the developer to replace the placeholder.

Step-by-step proof

  1. Developer clones the repo and runs cp examples/demo/.env.example examples/demo/.env (the documented onboarding step).
  2. .env now contains ONESIGNAL_APP_ID=your-onesignal-app-id.
  3. babel.config.js (react-native-dotenv) inlines ONESIGNAL_APP_ID as the literal string "your-onesignal-app-id" at every callsite.
  4. useOneSignal.ts:32 evaluates "your-onesignal-app-id"?.trim() || DEFAULT_APP_ID. .trim() returns "your-onesignal-app-id" (truthy), so || short-circuits and DEFAULT_APP_ID is not used.
  5. OneSignal.initialize("your-onesignal-app-id") runs. OneSignals API rejects the malformed (non-UUID) app ID.
  6. Developer is confused because line 1 of .env.example told them the documented default would be used "if ONESIGNAL_APP_ID is empty" — and from their point of view, they did not set it.

Severity

pre_existing — line 2 is unchanged by this PR, but the PR adds an adjacent line that uses the correct empty-fallback pattern, so this is the natural moment to align the older line. Trivial fix (one character) and only affects local-dev onboarding.

17 changes: 16 additions & 1 deletion examples/demo/android/app/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -56,8 +56,9 @@ react {

/**
* Set this to true to Run Proguard on Release builds to minify the Java bytecode.
* Enabled for the demo to keep the BrowserStack/e2e APK small (~28 MB → ~13 MB).
*/
def enableProguardInReleaseBuilds = false
def enableProguardInReleaseBuilds = true

/**
* The preferred build flavor of JavaScriptCore (JSC)
Expand Down Expand Up @@ -105,6 +106,20 @@ android {
proguardFiles getDefaultProguardFile("proguard-android.txt"), "proguard-rules.pro"
}
}

// CI-only: build a single arm64-v8a APK (~10 MB) instead of the
// universal ~30 MB fat APK. Enable with `-PciSingleAbi`. BrowserStack
// devices we run e2e against are all arm64-v8a.
if (project.hasProperty('ciSingleAbi')) {
splits {
abi {
enable true
reset()
include 'arm64-v8a'
universalApk false
}
}
}
}

apply from: "../../node_modules/react-native-vector-icons/fonts.gradle"
Expand Down
4 changes: 2 additions & 2 deletions examples/demo/bun.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion examples/demo/ios/ExportOptions.plist
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
<plist version="1.0">
<dict>
<key>method</key>
<string>development</string>
<string>debugging</string>
<key>teamID</key>
<string>99SW8E36CT</string>
<key>signingStyle</key>
Expand Down
34 changes: 17 additions & 17 deletions examples/demo/ios/Podfile.lock
Original file line number Diff line number Diff line change
Expand Up @@ -3,48 +3,48 @@ PODS:
- hermes-engine (250829098.0.7):
- hermes-engine/Pre-built (= 250829098.0.7)
- hermes-engine/Pre-built (250829098.0.7)
- OneSignalXCFramework (5.5.0):
- OneSignalXCFramework/OneSignalComplete (= 5.5.0)
- OneSignalXCFramework/OneSignal (5.5.0):
- OneSignalXCFramework (5.5.1):
- OneSignalXCFramework/OneSignalComplete (= 5.5.1)
- OneSignalXCFramework/OneSignal (5.5.1):
- OneSignalXCFramework/OneSignalCore
- OneSignalXCFramework/OneSignalExtension
- OneSignalXCFramework/OneSignalLiveActivities
- OneSignalXCFramework/OneSignalNotifications
- OneSignalXCFramework/OneSignalOSCore
- OneSignalXCFramework/OneSignalOutcomes
- OneSignalXCFramework/OneSignalUser
- OneSignalXCFramework/OneSignalComplete (5.5.0):
- OneSignalXCFramework/OneSignalComplete (5.5.1):
- OneSignalXCFramework/OneSignal
- OneSignalXCFramework/OneSignalInAppMessages
- OneSignalXCFramework/OneSignalLocation
- OneSignalXCFramework/OneSignalCore (5.5.0)
- OneSignalXCFramework/OneSignalExtension (5.5.0):
- OneSignalXCFramework/OneSignalCore (5.5.1)
- OneSignalXCFramework/OneSignalExtension (5.5.1):
- OneSignalXCFramework/OneSignalCore
- OneSignalXCFramework/OneSignalOutcomes
- OneSignalXCFramework/OneSignalInAppMessages (5.5.0):
- OneSignalXCFramework/OneSignalInAppMessages (5.5.1):
- OneSignalXCFramework/OneSignalCore
- OneSignalXCFramework/OneSignalNotifications
- OneSignalXCFramework/OneSignalOSCore
- OneSignalXCFramework/OneSignalOutcomes
- OneSignalXCFramework/OneSignalUser
- OneSignalXCFramework/OneSignalLiveActivities (5.5.0):
- OneSignalXCFramework/OneSignalLiveActivities (5.5.1):
- OneSignalXCFramework/OneSignalCore
- OneSignalXCFramework/OneSignalOSCore
- OneSignalXCFramework/OneSignalUser
- OneSignalXCFramework/OneSignalLocation (5.5.0):
- OneSignalXCFramework/OneSignalLocation (5.5.1):
- OneSignalXCFramework/OneSignalCore
- OneSignalXCFramework/OneSignalNotifications
- OneSignalXCFramework/OneSignalOSCore
- OneSignalXCFramework/OneSignalUser
- OneSignalXCFramework/OneSignalNotifications (5.5.0):
- OneSignalXCFramework/OneSignalNotifications (5.5.1):
- OneSignalXCFramework/OneSignalCore
- OneSignalXCFramework/OneSignalExtension
- OneSignalXCFramework/OneSignalOutcomes
- OneSignalXCFramework/OneSignalOSCore (5.5.0):
- OneSignalXCFramework/OneSignalOSCore (5.5.1):
- OneSignalXCFramework/OneSignalCore
- OneSignalXCFramework/OneSignalOutcomes (5.5.0):
- OneSignalXCFramework/OneSignalOutcomes (5.5.1):
- OneSignalXCFramework/OneSignalCore
- OneSignalXCFramework/OneSignalUser (5.5.0):
- OneSignalXCFramework/OneSignalUser (5.5.1):
- OneSignalXCFramework/OneSignalCore
- OneSignalXCFramework/OneSignalNotifications
- OneSignalXCFramework/OneSignalOSCore
Expand Down Expand Up @@ -1446,9 +1446,9 @@ PODS:
- React-RCTFBReactNativeSpec
- ReactCommon/turbomodule/core
- ReactNativeDependencies
- react-native-onesignal (5.4.3):
- react-native-onesignal (5.4.4):
- hermes-engine
- OneSignalXCFramework (= 5.5.0)
- OneSignalXCFramework (= 5.5.1)
- RCTRequired
- RCTTypeSafety
- React-Core
Expand Down Expand Up @@ -2312,7 +2312,7 @@ EXTERNAL SOURCES:
SPEC CHECKSUMS:
FBLazyVector: c12d2108050e27952983d565a232f6f7b1ad5e69
hermes-engine: 177322198264d50dc281084c48e1ed80ea14884c
OneSignalXCFramework: 943852e7d70d719f73e9669d48620aeec1b93022
OneSignalXCFramework: 2b46c36b38528b65dce33ed9d83375f8c98bf40c
RCTDeprecation: 3280799c14232a56e5a44f92981a8ee33bc69fd9
RCTRequired: 9854a51b0f65ccf43ea0b744df4d70fce339db32
RCTSwiftUI: 96986e49a4fdc2c2103929dee2641e1b57edf33d
Expand Down Expand Up @@ -2349,7 +2349,7 @@ SPEC CHECKSUMS:
React-logger: 9e51e01455f15cb3ef87a09a1ec773cdb22d56c1
React-Mapbuffer: 92b99e450e8ff598b27d6e4db3a75e04fd45e9a9
React-microtasksnativemodule: 2fe0f2bd2840dedbd66c0ac249c64f977f39cc18
react-native-onesignal: dc0212667c92798d90a72316aec304f9df6dddc4
react-native-onesignal: bc00d2a1100522842e5215026178212c560004eb
react-native-safe-area-context: ae7587b95fb580d1800c5b0b2a7bd48c2868e67a
React-NativeModulesApple: 44a9474594566cd03659f92e38f42599c6b9dee4
React-networking: db73d91466cb134fcbdaaa579fb2de14e2c2ea01
Expand Down
Loading
Loading