Feat/messaging request response#9866
Feat/messaging request response#9866zwu52 wants to merge 4 commits intofeat/messaging-api-seriesfrom
Conversation
zwu52
commented
Apr 20, 2026
zwu52
commented
- add sdk version & origin in the request
- fix response parsing
- Persist the last successful FID registration time in IndexedDB and refresh backend registration on a 7-day cadence even when the FID is unchanged - Fall back to opening DB v1 when opening/upgrading to v2 fails, so existing token data remains readable (guarded by a unit test) - Extend register() tests to cover weekly refresh without re-firing onRegistered Made-with: Cursor
…sponse Made-with: Cursor
Add top-level origin to registration API payloads (host from SW scope URL, or app name fallback). Export getRegistrationOrigin for tests. Clarify CreateRegistration name field in register-fid comment. Made-with: Cursor
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the FCM messaging package to include the SDK version and origin in registration requests and adds support for parsing the Firebase Installation ID (FID) from both legacy plain strings and new resource name formats. Key changes include the implementation of getRegistrationOrigin to derive the client's host from the service worker scope and updates to the request body interfaces. Feedback highlights an improvement opportunity to avoid circular dependencies in tests by using hardcoded expected values and suggests using a more robust regex-based approach for parsing FIDs from resource names.
I am having trouble creating individual review comments. Click here to see my feedback.
packages/messaging/src/internals/requests.test.ts (72-75)
The test uses the implementation function getRegistrationOrigin to define the expected value for the origin field. This creates a circular dependency in the test logic, meaning that if getRegistrationOrigin has a bug, the test might still pass because both the code and the test use the same faulty logic. It is better to use a hardcoded string (e.g., 'example.org') that represents the expected output for the specific input used in the test.
origin: 'example.org',packages/messaging/src/internals/requests.test.ts (272-275)
Similar to the getToken test, this test uses the implementation function getRegistrationOrigin to define the expected value. To ensure the test is robust and independent of the implementation logic, please use a hardcoded expected value.
origin: 'example.org',packages/messaging/src/internals/requests.ts (204-210)
The logic for extracting the FID from the resource name using indexOf and slice is somewhat fragile. It assumes that the FID is always the remainder of the string after the first occurrence of /registrations/. A more robust approach would be to use a regular expression that specifically matches the expected resource name pattern (e.g., projects/*/registrations/*) and extracts the final segment. This would better handle potential edge cases or future changes in the resource name format.
| * - Use the FIS auth token produced by the installations instance (implicitly associated with FID). | ||
| * - CreateRegistration must echo a non-empty FID in `name`; it must match `expectedFid` from | ||
| * - CreateRegistration must echo the installation in `name` (e.g. | ||
| * `projects/{projectId}/registrations/{fid}`) or a legacy plain FID; it must match `expectedFid` from |
There was a problem hiding this comment.
I'm not sure about what legacy plain FID means. I think it's either name or token for backend. However for this method, it should always expect name from backend.
| ): ApiRequestBody { | ||
| const body: ApiRequestBody = { | ||
| // eslint-disable-next-line camelcase | ||
| fcm_sdk_version: fcmSdkVersion, |
There was a problem hiding this comment.
If we set fcm_sdk_version here, does it guarantee that the token creation is not affected? Since backend will use this parameter to differentiate whether to create a FID-based or a token-based registration, so we should only pass fcm_sdk_version for FID registration requests.
2 participants
