fix(analytics, android, ios): cast item INDEX param to integer#8964
fix(analytics, android, ios): cast item INDEX param to integer#8964brojor wants to merge 1 commit intoinvertase:mainfrom
Conversation
The React Native bridge converts all JS numbers to double, but the native Firebase SDK expects FirebaseAnalytics.Param.INDEX as an integer/long. When a double is passed, Firebase does not recognise it as the standard ecommerce parameter and item_list_index ends up as (not set) in GA4 / BigQuery. Mirror the existing QUANTITY conversion for INDEX in both native modules so that JS numbers are coerced to integer before being forwarded to Firebase.
|
@brojor is attempting to deploy a commit to the Invertase Team on Vercel. A member of the Team first needs to authorize it. |
|
|
![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 adds support for the INDEX parameter in Firebase Analytics for both Android and iOS platforms. On Android, the parameter is converted from a double to a long, while on iOS, it is converted to an integer. A review comment suggests improving variable naming for better readability and notes a potential inconsistency in how numeric parameters are typed compared to the Firebase SDK expectations.
| if (itemBundle.containsKey(FirebaseAnalytics.Param.INDEX)) { | ||
| double number = itemBundle.getDouble(FirebaseAnalytics.Param.INDEX); | ||
| itemBundle.putLong(FirebaseAnalytics.Param.INDEX, (long) number); | ||
| } |
There was a problem hiding this comment.
There's an inconsistency in how numeric parameters are handled. The existing code for QUANTITY uses putInt, while this change uses putLong for INDEX. According to the Firebase SDK documentation, both parameters are expected to be of type long. For correctness and consistency, QUANTITY should also be handled as a long.
Additionally, using a more descriptive variable name instead of number would improve readability.
| if (itemBundle.containsKey(FirebaseAnalytics.Param.INDEX)) { | |
| double number = itemBundle.getDouble(FirebaseAnalytics.Param.INDEX); | |
| itemBundle.putLong(FirebaseAnalytics.Param.INDEX, (long) number); | |
| } | |
| if (itemBundle.containsKey(FirebaseAnalytics.Param.INDEX)) { | |
| double index = itemBundle.getDouble(FirebaseAnalytics.Param.INDEX); | |
| itemBundle.putLong(FirebaseAnalytics.Param.INDEX, (long) index); | |
| } |
There was a problem hiding this comment.
Regarding the QUANTITY inconsistency — I'd prefer to keep that out of this PR to stay focused on the INDEX fix. If QUANTITY should indeed be a long per the Firebase SDK, that's a pre-existing issue and deserves its own PR so it can be reviewed and reverted independently.
For the variable naming, I intentionally kept number to stay consistent with the existing QUANTITY block right above, which uses the same pattern. Renaming only the new lines would introduce inconsistency within the same method.
2 participants
Description
The React Native bridge converts all JS numbers to
double, but the native Firebase SDK expectsFirebaseAnalytics.Param.INDEX(which maps toitem_list_index) as an integer/long. When adoubleis forwarded, Firebase does not recognise it as the standard ecommerce parameter —item_list_indexends up as(not set)in GA4 / BigQuery, breaking attribution of clicks/views to list positions.The library already handles this exact problem for
Param.QUANTITYin both native modules. This PR mirrors that conversion forParam.INDEX:ReactNativeFirebaseAnalyticsModule.java):putLong(FirebaseAnalytics.Param.INDEX, (long) number)RNFBAnalyticsModule.m):item[kFIRParameterIndex] = @([item[kFIRParameterIndex] integerValue])No JS / TS changes — the public API already accepts
index: number, only the native coercion was missing.Related issues
None filed upstream; happy to open one if preferred.
Release Summary
Fix
item_list_indexshowing as(not set)in GA4 / BigQuery whenindexis provided onitems[].Checklist
AndroidiOSOther(macOS, web)e2etests added or updated inpackages/**/e2ejesttests added or updated inpackages/**/__tests__No tests added — the existing
QUANTITYconversion (the precedent this PR follows) is also not covered by jest/e2e tests, since the coercion happens in the native modules below the JS bridge and there is no native test infra in the analytics package. Happy to add coverage if maintainers point me at the right place.Test Plan
Verified in production via `patch-package` against `@react-native-firebase/analytics@22.3.0` in a live e-commerce app:
Note
Low Risk
Small, localized change to native parameter coercion for analytics events; low risk aside from potential truncation if callers pass non-integer
indexvalues.Overview
Ensures Firebase ecommerce
items[]event params sendINDEX/item_list_indexas an integer type instead of a JS-bridgedouble.On Android,
ReactNativeFirebaseAnalyticsModule.toBundlenow castsFirebaseAnalytics.Param.INDEXtolong; on iOS,cleanJavascriptParamsnow coerceskFIRParameterIndexto anintegerValue, mirroring existingQUANTITYhandling.Reviewed by Cursor Bugbot for commit 77c898a. Bugbot is set up for automated code reviews on this repo. Configure here.
🔥