Skip to content

feat: [SDK-5170] register push before FCM credentials upload - #2735

Open
fadi-george wants to merge 4 commits into
firebase-installation-idsfrom
fadi/SDK-5170-register-before-service-account
Open

feat: [SDK-5170] register push before FCM credentials upload#2735
fadi-george wants to merge 4 commits into
firebase-installation-idsfrom
fadi/SDK-5170-register-before-service-account

Conversation

@fadi-george

Copy link
Copy Markdown
Contributor

Description

One Line Summary

Register Android push subscriptions from the host Firebase configuration before dashboard FCM credentials are uploaded.

Details

Motivation

A newly configured Android app currently receives no push token or Firebase Installation ID until its FCM service account is uploaded to OneSignal and the app launches again. This change derives the sender ID from the host default Firebase app when dashboard parameters do not provide one, allowing the identifier to be uploaded during the first app session.

Scope

  • Preserves the dashboard sender ID as the primary source when configured.
  • Falls back to the host default Firebase app sender ID when dashboard parameters omit it.
  • Supports both legacy FCM token and Firebase Installation ID registration.
  • Detects sender-ID mismatches between the host Firebase app and OneSignal configuration.
  • Keeps legacy dashboard-provided sender registration available when no default Firebase app exists.
  • Makes local demo Firebase configuration optional and ignored by Git.

Review focus

  • Whether legacy token registration should reject a host/dashboard sender mismatch even though it uses OneSignal's isolated Firebase app.
  • Correct behavior if the effective sender ID changes during the same process after remote parameter hydration.

Testing

Unit testing

Added coverage for sender-ID derivation, registration before a dashboard sender exists, legacy-token and FID paths, missing default Firebase apps, mismatches, reflective registration, and failures.

Ran:

  • Focused FCM registration unit tests
  • Full testDebugUnitTest
  • Spotless
  • Detekt
  • Release assembly

Manual testing

Built the GMS demo with a local google-services.json and confirmed the generated APK packages google_app_id and gcm_defaultSenderId. Final end-to-end validation of credential upload activating an existing subscription remains pending because the emulator disconnected before the rebuilt APK could be installed.

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
fadi-george requested a review from a team as a code owner September 4, 2026 01:28
@github-actions

github-actions Bot commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

📊 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

  • PushRegistratorAbstractGoogle.kt: 3/4 touched executable lines (75.0%) (5 touched lines in diff)
    • 1 uncovered touched lines in this file
  • PushRegistratorFCM.kt: 25/25 touched executable lines (100.0%) (42 touched lines in diff)

Overall (aggregate gate)

28/29 touched executable lines covered (96.6% — requires ≥ 80%)

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

  • PushRegistratorAbstractGoogle.kt: 75.0% (1 uncovered touched lines)

📥 View workflow run

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Multi-model review

Models: Claude Opus 5, GPT 5.6 Sol, Cursor Grok 4.6.

Act on

  • Legacy .token registration now throws on a host vs dashboard sender mismatch, even though the token is minted on the isolated ONESIGNAL_SDK_FCM_APP_NAME app (3/3). That is a regression for mixed-project apps, and it also blocks the host-fallback → dashboard-sender upgrade after credentials are uploaded. Keep validateSenderId on the FID path only.

Consider

  • initFirebaseApp caches by nullness, not sender (3/3). Once the mismatch throw is removed, an in-process sender change (start/host-fallback then HYDRATE, or appId switch) can keep serving the old project's token. Recreate the named app when sender/FCM params change.
  • Empty/"null" android_sender_id skips the host fallback (Opus); safeString only yields null when the key is absent.
  • Host-fallback still pairs the host sender with OneSignal shared FCM credentials on the named app (Grok). The FID path is consistent; the legacy first-session token may not be the identifier that later receives traffic.
  • Residual from the FID work: English substring detect of Firebase's "API disabled…register()" message, and no onRegistered/FID-rotation bridge (Opus/GPT).

Noted / dismissed

  • Consumer ProGuard register() keep, defaultSenderId derivation, and the Gradle require range look correct.
  • snd= census vs FirebaseOptions sender, and the "FID registration is enabled (…=not set)" error copy, are nits.
Open in Web View Automation 

Sent by Cursor Automation: PR Reviews

@abdulraqeeb33 abdulraqeeb33 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Answer to the review focus: do not reject a host vs dashboard sender mismatch on the legacy path. That path still mints from ONESIGNAL_SDK_FCM_APP_NAME. Mixed-project apps are supposed to keep working there. 2734 left that alone.

  1. Drop validateSenderId from getLegacyToken. Keep it on the FID path only. Invert the mixed-project legacy test.
  2. Same change: rebuild the named app when senderId changes. Host fallback plus HYDRATE can change sender in-process. Cache-by-nullness then uploads the old sender's token.

@fadi-george
fadi-george force-pushed the fadi/SDK-5170-register-before-service-account branch from a7d45a8 to d71828d Compare September 4, 2026 20:10
@fadi-george
fadi-george changed the base branch from main to firebase-installation-ids September 4, 2026 20:10
Co-authored-by: Cursor <cursoragent@cursor.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants