-
Notifications
You must be signed in to change notification settings - Fork 13
Fix Android New Arch Issue #180
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -95,8 +95,8 @@ internal class RNUsercentricsModule( | |
| } | ||
|
|
||
| @ReactMethod | ||
| override fun setCMPId(id: Int) { | ||
| usercentricsProxy.instance.setCMPId(id) | ||
| override fun setCMPId(id: Double) { | ||
| usercentricsProxy.instance.setCMPId(id.toInt()) | ||
| } | ||
|
|
||
| @ReactMethod | ||
|
|
@@ -131,80 +131,80 @@ internal class RNUsercentricsModule( | |
| } | ||
|
|
||
| @ReactMethod | ||
| override fun acceptAllForTCF(fromLayer: Int, consentType: Int, promise: Promise) { | ||
| override fun acceptAllForTCF(fromLayer: Double, consentType: Double, promise: Promise) { | ||
| promise.resolve( | ||
| usercentricsProxy.instance.acceptAllForTCF( | ||
| TCFDecisionUILayer.values()[fromLayer], UsercentricsConsentType.values()[consentType] | ||
| TCFDecisionUILayer.values()[fromLayer.toInt()], UsercentricsConsentType.values()[consentType.toInt()] | ||
| ).toWritableArray() | ||
| ) | ||
| } | ||
|
|
||
| @ReactMethod | ||
| override fun acceptAll(consentType: Int, promise: Promise) { | ||
| override fun acceptAll(consentType: Double, promise: Promise) { | ||
| promise.resolve( | ||
| usercentricsProxy.instance.acceptAll( | ||
| UsercentricsConsentType.values()[consentType] | ||
| UsercentricsConsentType.values()[consentType.toInt()] | ||
| ).toWritableArray() | ||
| ) | ||
| } | ||
|
|
||
| @ReactMethod | ||
| override fun denyAllForTCF(fromLayer: Int, consentType: Int, unsavedPurposeLIDecisions: ReadableArray, promise: Promise) { | ||
| override fun denyAllForTCF(fromLayer: Double, consentType: Double, unsavedPurposeLIDecisions: ReadableArray, promise: Promise) { | ||
| promise.resolve( | ||
| usercentricsProxy.instance.denyAllForTCF( | ||
| TCFDecisionUILayer.values()[fromLayer], UsercentricsConsentType.values()[consentType], unsavedPurposeLIDecisions.deserializePurposeLIDecisionsMap() | ||
| TCFDecisionUILayer.values()[fromLayer.toInt()], UsercentricsConsentType.values()[consentType.toInt()], unsavedPurposeLIDecisions.deserializePurposeLIDecisionsMap() | ||
| ).toWritableArray() | ||
| ) | ||
| } | ||
|
|
||
| @ReactMethod | ||
| override fun denyAll(consentType: Int, promise: Promise) { | ||
| override fun denyAll(consentType: Double, promise: Promise) { | ||
| promise.resolve( | ||
| usercentricsProxy.instance.denyAll( | ||
| UsercentricsConsentType.values()[consentType] | ||
| UsercentricsConsentType.values()[consentType.toInt()] | ||
| ).toWritableArray() | ||
| ) | ||
| } | ||
|
|
||
| @ReactMethod | ||
| override fun saveDecisionsForTCF( | ||
| tcfDecisions: ReadableMap, | ||
| fromLayer: Int, | ||
| fromLayer: Double, | ||
| saveDecisions: ReadableArray, | ||
| consentType: Int, | ||
| consentType: Double, | ||
| promise: Promise | ||
| ) { | ||
| promise.resolve( | ||
| usercentricsProxy.instance.saveDecisionsForTCF( | ||
| tcfDecisions.deserializeTCFUserDecisions(), | ||
| TCFDecisionUILayer.values()[fromLayer], | ||
| TCFDecisionUILayer.values()[fromLayer.toInt()], | ||
| saveDecisions.deserializeUserDecision(), | ||
| UsercentricsConsentType.values()[consentType] | ||
| UsercentricsConsentType.values()[consentType.toInt()] | ||
| ).toWritableArray() | ||
| ) | ||
| } | ||
|
|
||
| @ReactMethod | ||
| override fun saveDecisions(decisions: ReadableArray, consentType: Int, promise: Promise) { | ||
| override fun saveDecisions(decisions: ReadableArray, consentType: Double, promise: Promise) { | ||
| promise.resolve( | ||
| usercentricsProxy.instance.saveDecisions( | ||
| decisions.deserializeUserDecision(), UsercentricsConsentType.values()[consentType] | ||
| decisions.deserializeUserDecision(), UsercentricsConsentType.values()[consentType.toInt()] | ||
| ).toWritableArray() | ||
| ) | ||
| } | ||
|
|
||
| @ReactMethod | ||
| override fun saveOptOutForCCPA(isOptedOut: Boolean, consentType: Int, promise: Promise) { | ||
| override fun saveOptOutForCCPA(isOptedOut: Boolean, consentType: Double, promise: Promise) { | ||
| promise.resolve( | ||
| usercentricsProxy.instance.saveOptOutForCCPA( | ||
| isOptedOut, UsercentricsConsentType.values()[consentType] | ||
| isOptedOut, UsercentricsConsentType.values()[consentType.toInt()] | ||
| ).toWritableArray() | ||
| ) | ||
| } | ||
|
|
||
| @ReactMethod | ||
| override fun track(event: Int) { | ||
| usercentricsProxy.instance.track(UsercentricsAnalyticsEventType.values()[event]) | ||
| override fun track(event: Double) { | ||
| usercentricsProxy.instance.track(UsercentricsAnalyticsEventType.values()[event.toInt()]) | ||
| } | ||
|
Comment on lines
+206
to
208
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same bounds-check concern applies here.
🤖 Prompt for AI Agents |
||
|
|
||
| @ReactMethod | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -55,7 +55,7 @@ abstract class RNUsercentricsModuleSpec internal constructor(context: ReactAppli | |||||
| abstract fun getABTestingVariant(promise: Promise) | ||||||
|
|
||||||
| @ReactMethod | ||||||
| abstract fun setCMPId(id: Int) | ||||||
| abstract fun setCMPId(id: Double) | ||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Suggestion: Non-nullable Severity Level: Critical 🚨- ❌ Native module call setCMPId may crash on null input.
- ⚠️ JS callers that omit id receive failed native invocation.
Suggested change
Steps of Reproduction ✅1. Build and run the Android app with this PR code so the TurboModule is registered (file:
android/src/main/java/com/usercentrics/reactnative/RNUsercentricsModuleSpec.kt,
declaration at line 58).
2. From JavaScript, call the native method exported by the TurboModule (e.g.,
RNUsercentricsModule.setCMPId(null) or omit the numeric argument so JS sends
null/undefined).
3. The React Native bridge invokes the generated/native implementation for the spec which
maps to the abstract signature at RNUsercentricsModuleSpec.kt:58; the bridge passes a
boxed Double which can be null.
4. Kotlin's non-nullable Double compiles to a primitive double and the runtime attempts to
unbox the null, causing a NullPointerException at the unboxing site and the native call to
fail.Prompt for AI Agent 🤖This is a comment left during a code review.
**Path:** android/src/main/java/com/usercentrics/reactnative/RNUsercentricsModuleSpec.kt
**Line:** 58:58
**Comment:**
*Null Pointer: Non-nullable `Double` parameter will be compiled to a primitive `double` and will throw a NullPointerException if the JS bridge passes null; make the parameter nullable (`Double?`) so it accepts boxed null values coming from JS interop.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. |
||||||
|
|
||||||
| @ReactMethod | ||||||
| abstract fun setABTestingVariant(variant: String) | ||||||
|
|
@@ -64,34 +64,34 @@ abstract class RNUsercentricsModuleSpec internal constructor(context: ReactAppli | |||||
| abstract fun changeLanguage(language: String, promise: Promise) | ||||||
|
|
||||||
| @ReactMethod | ||||||
| abstract fun acceptAll(consentType: Int, promise: Promise) | ||||||
| abstract fun acceptAll(consentType: Double, promise: Promise) | ||||||
|
|
||||||
| @ReactMethod | ||||||
| abstract fun acceptAllForTCF(fromLayer: Int, consentType: Int, promise: Promise) | ||||||
| abstract fun acceptAllForTCF(fromLayer: Double, consentType: Double, promise: Promise) | ||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Suggestion: Non-nullable Severity Level: Critical 🚨- ❌ acceptAllForTCF native calls may throw runtime exceptions.
- ⚠️ Consent flows relying on TCF acceptance break unpredictably.
Suggested change
Steps of Reproduction ✅1. Deploy the Android app containing RNUsercentricsModuleSpec (file path:
android/src/main/java/com/usercentrics/reactnative/RNUsercentricsModuleSpec.kt, method at
line 70).
2. From the React Native JS layer, invoke acceptAllForTCF with missing/null numeric args
(e.g., RNUsercentricsModule.acceptAllForTCF(null, null, promise)).
3. The TurboModule bridge calls into the native implementation bound to the abstract
signature at RNUsercentricsModuleSpec.kt:70 and forwards boxed Double values.
4. Because the signature requires non-nullable Double, the JVM will unbox the null and
throw an NPE, causing the native method to fail and the promise to reject unexpectedly.Prompt for AI Agent 🤖This is a comment left during a code review.
**Path:** android/src/main/java/com/usercentrics/reactnative/RNUsercentricsModuleSpec.kt
**Line:** 70:70
**Comment:**
*Null Pointer: Non-nullable `Double` parameters in the bridge method can cause unboxing NullPointerExceptions if JS sends null; declare `fromLayer` and `consentType` as nullable `Double?` so the module safely accepts boxed null values from the JS side.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. |
||||||
|
|
||||||
| @ReactMethod | ||||||
| abstract fun denyAll(consentType: Int, promise: Promise) | ||||||
| abstract fun denyAll(consentType: Double, promise: Promise) | ||||||
|
|
||||||
| @ReactMethod | ||||||
| abstract fun denyAllForTCF(fromLayer: Int, consentType: Int, unsavedPurposeLIDecisions: ReadableArray, promise: Promise) | ||||||
| abstract fun denyAllForTCF(fromLayer: Double, consentType: Double, unsavedPurposeLIDecisions: ReadableArray, promise: Promise) | ||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Suggestion: Method accepts a Severity Level: Critical 🚨- ❌ denyAllForTCF can crash when JS supplies null numerics.
- ⚠️ Purpose-level decision processing may be interrupted.
Suggested change
Steps of Reproduction ✅1. Install the PR version of the Android module so RNUsercentricsModuleSpec is used (see
file android/src/main/java/com/usercentrics/reactnative/RNUsercentricsModuleSpec.kt,
method at line 76).
2. From JS, call denyAllForTCF and pass null for numeric args while providing a
ReadableArray (e.g., RNUsercentricsModule.denyAllForTCF(null, null, someArray, promise)).
3. The TurboModule bridge forwards boxed numeric values to the native method bound to
RNUsercentricsModuleSpec.kt:76.
4. The non-nullable Double parameters force JVM unboxing of null, causing an NPE and
failing the native denyAllForTCF execution and promise resolution.Prompt for AI Agent 🤖This is a comment left during a code review.
**Path:** android/src/main/java/com/usercentrics/reactnative/RNUsercentricsModuleSpec.kt
**Line:** 76:76
**Comment:**
*Null Pointer: Method accepts a `ReadableArray` from JS alongside `Double` values; the `consentType` Double should be nullable (`Double?`) to avoid unboxing NullPointerException if JS passes null/undefined for numeric args.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. |
||||||
|
|
||||||
| @ReactMethod | ||||||
| abstract fun saveDecisions(decisions: ReadableArray, consentType: Int, promise: Promise) | ||||||
| abstract fun saveDecisions(decisions: ReadableArray, consentType: Double, promise: Promise) | ||||||
|
|
||||||
| @ReactMethod | ||||||
| abstract fun saveDecisionsForTCF( | ||||||
| tcfDecisions: ReadableMap, | ||||||
| fromLayer: Int, | ||||||
| fromLayer: Double, | ||||||
| saveDecisions: ReadableArray, | ||||||
| consentType: Int, | ||||||
| consentType: Double, | ||||||
| promise: Promise | ||||||
| ) | ||||||
|
|
||||||
| @ReactMethod | ||||||
| abstract fun saveOptOutForCCPA(isOptedOut: Boolean, consentType: Int, promise: Promise) | ||||||
| abstract fun saveOptOutForCCPA(isOptedOut: Boolean, consentType: Double, promise: Promise) | ||||||
|
|
||||||
| @ReactMethod | ||||||
| abstract fun track(event: Int) | ||||||
| abstract fun track(event: Double) | ||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Suggestion: The Severity Level: Critical 🚨- ❌ Analytics track calls may throw runtime exceptions.
- ⚠️ Event tracking reliability degraded on null inputs.
Suggested change
Steps of Reproduction ✅1. Run the Android app including this PR so the TurboModule and RNUsercentricsModuleSpec
are active (file:
android/src/main/java/com/usercentrics/reactnative/RNUsercentricsModuleSpec.kt, line 94).
2. From JS, call the tracking method with null or undefined event (e.g.,
RNUsercentricsModule.track(null) or omit event).
3. The React Native bridge invokes the native implementation tied to
RNUsercentricsModuleSpec.kt:94, passing a boxed Double that may be null.
4. The non-nullable Double requires unboxing; if null is received the JVM throws an NPE
and the track call fails silently or raises an exception on the native side.Prompt for AI Agent 🤖This is a comment left during a code review.
**Path:** android/src/main/java/com/usercentrics/reactnative/RNUsercentricsModuleSpec.kt
**Line:** 94:94
**Comment:**
*Null Pointer: The `track` method's `event` parameter is declared as non-nullable `Double` which will be compiled to a primitive and will NPE if JS sends null; change it to nullable `Double?` (or a boxed type) to safely accept bridge values.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. |
||||||
|
|
||||||
| companion object { | ||||||
| const val NAME = "RNUsercentricsModule" | ||||||
|
|
||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Potential
ArrayIndexOutOfBoundsExceptionfrom unchecked enum index access.Using
.values()[fromLayer.toInt()]and.values()[consentType.toInt()]without bounds validation can cause runtime crashes if JavaScript passes invalid values (negative numbers, values exceeding enum size, or fractional values that truncate unexpectedly).While this pattern existed before the
Int→Doublechange, the widerDoublerange and truncation behavior (e.g.,2.9→2) increase the surface for unexpected inputs.Consider adding defensive bounds checking, or at minimum, wrapping enum access in a try-catch to provide a meaningful error message back to the JS layer via
promise.reject().Example defensive approach
🤖 Prompt for AI Agents