feat(MSDK-3261): add storage info and sdks field to ConsentDisclosure…#182
feat(MSDK-3261): add storage info and sdks field to ConsentDisclosure…#182uc-brunosilva merged 2 commits intomasterfrom
Conversation
…Object - Add storageInfo and sdks fields to ConsentDisclosureObject model - Update ConsentDisclosureSerializer (Android) to map new fields - Update UsercentricsCMPData+Dict (iOS) to include new fields - Update mocks (Android/iOS) and TS test mocks to reflect new fields Co-authored-by: Cursor <cursoragent@cursor.com>
|
CodeAnt AI is reviewing your PR. Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
Review Summary by QodoAdd storage info and SDKs field to ConsentDisclosureObject
WalkthroughsDescription• Add storageInfo and sdks fields to ConsentDisclosureObject model • Restructure deviceStorage from flat list to nested object with disclosures and sdks • Update serialization logic across Android, iOS, and TypeScript implementations • Update all mock data to reflect new model structure File Changes1. android/src/androidTest/java/com/usercentrics/reactnative/mock/GetCMPDataMock.kt
|
|
No actionable comments were generated in the recent review. 🎉 📝 WalkthroughWalkthroughConsentDisclosureObject gains a new sdks field across Android, iOS, and web. Serialization and mock data are updated to include both "disclosures" and "sdks" (empty lists where applicable), and a ConsentDisclosureSDK type/class and serializer were added. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
CI Feedback 🧐A test triggered by this PR failed. Here is an AI-generated analysis of the failure:
|
codeant-ai
bot
added
the
size:M
|
PR Summary: Add sdks field to ConsentDisclosureObject and turn deviceStorage from a list into an object {disclosures, sdks} across Android, iOS, and JS/TS layers.
|
Nitpicks 🔍
|
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
| sdks: [ConsentDisclosureSDK] | ||
|
|
There was a problem hiding this comment.
Suggestion: The sdks property and constructor parameter are typed as a single-element tuple ([ConsentDisclosureSDK]), but both the constructor default (sdks ?? []) and the test mocks assign an empty array, which is not compatible with a non-empty tuple type and also misrepresents the intended "zero or more SDKs" contract; this can lead to type errors and incorrect modeling of the data. [type error]
Severity Level: Critical 🚨
- ❌ TypeScript compilation fails at src/__tests__/mocks.ts:270.
- ⚠️ Cannot represent ConsentDisclosureObject with zero SDKs.
- ⚠️ TS model diverges from native list-based sdks field.| sdks: [ConsentDisclosureSDK] | |
| sdks: ConsentDisclosureSDK[] | |
| constructor(disclosures: [ConsentDisclosure], sdks?: ConsentDisclosureSDK[]) { |
Steps of Reproduction ✅
1. Run the TypeScript type checker or test suite for this repo (e.g., `tsc` or `yarn
test`) so that all files under `src/` are type-checked.
2. During type-checking, `src/models/ConsentDisclosureObject.tsx:11-20` is loaded, where
`ConsentDisclosureObject` defines `sdks: [ConsentDisclosureSDK]` and its constructor
parameter as `sdks?: [ConsentDisclosureSDK]`, and assigns `this.sdks = sdks ?? []`.
3. The compiler then loads `src/__tests__/mocks.ts:259-273`, where `const
consentDisclosureObject: ConsentDisclosureObject = { disclosures: [disclosure], sdks: [],
}` attempts to assign an empty array `[]` to the `sdks` property.
4. TypeScript reports a type error because `sdks` is declared as a single-element tuple
`[ConsentDisclosureSDK]` (non-empty) while both `this.sdks = sdks ?? []` in
`ConsentDisclosureObject` and `sdks: []` in `mocks.ts` provide `[]`, which is not
assignable to `[ConsentDisclosureSDK]`, causing the build/type-check to fail.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/models/ConsentDisclosureObject.tsx
**Line:** 14:15
**Comment:**
*Type Error: The `sdks` property and constructor parameter are typed as a single-element tuple (`[ConsentDisclosureSDK]`), but both the constructor default (`sdks ?? []`) and the test mocks assign an empty array, which is not compatible with a non-empty tuple type and also misrepresents the intended "zero or more SDKs" contract; this can lead to type errors and incorrect modeling of the data.
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.
|
CodeAnt AI finished reviewing your PR. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
example/ios/exampleTests/Mock/CMPData+Mock.swift (1)
234-238: Consider adding aConsentDisclosureSDK.mock()helper to mirror the existingConsentDisclosure.mock().The
sdks: []initialisation is correct and consistent with thesample/counterpart. However, the file already provides a fully-populatedConsentDisclosure.mock()(lines 241–253) to enable tests to work with real-looking disclosure data. There is no equivalentConsentDisclosureSDK.mock()extension here, which means any future test that needs to assert on SDK-level fields will have to construct instances inline or silently run against an empty list.✨ Suggested addition (optional)
extension ConsentDisclosureSDK { static func mock() -> ConsentDisclosureSDK { return .init(/* populate with representative values once the type's initializer is known */) } }And update the
ConsentDisclosureObjectmock to use it:- return .init(disclosures: [.mock()], sdks: []) + return .init(disclosures: [.mock()], sdks: [.mock()])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@example/ios/exampleTests/Mock/CMPData`+Mock.swift around lines 234 - 238, Add a ConsentDisclosureSDK.mock() helper and use it from ConsentDisclosureObject.mock(): implement an extension on ConsentDisclosureSDK with a static func mock() -> ConsentDisclosureSDK that returns a representative, fully-populated instance (fill fields with realistic test values), then change ConsentDisclosureObject.mock() to initialize sdks using [.mock()] (or an appropriate array of mocks) instead of the empty array so tests can assert SDK-level fields.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/models/ConsentDisclosureObject.tsx`:
- Around line 14-18: The tuple types like sdks: [ConsentDisclosureSDK],
disclosures: [ConsentDisclosure], and purposes: [number] are incorrect — change
them to array types ConsentDisclosureSDK[], ConsentDisclosure[], and number[]
respectively; update the ConsentDisclosureObject constructor
(constructor(disclosures: [ConsentDisclosure], sdks?: [ConsentDisclosureSDK]))
to accept disclosures: ConsentDisclosure[] and sdks?: ConsentDisclosureSDK[] and
set this.sdks = sdks ?? [] (and similarly ensure this.disclosures and any
purposes defaults use [] where appropriate) so empty arrays are allowed and the
types match runtime usage.
---
Nitpick comments:
In `@example/ios/exampleTests/Mock/CMPData`+Mock.swift:
- Around line 234-238: Add a ConsentDisclosureSDK.mock() helper and use it from
ConsentDisclosureObject.mock(): implement an extension on ConsentDisclosureSDK
with a static func mock() -> ConsentDisclosureSDK that returns a representative,
fully-populated instance (fill fields with realistic test values), then change
ConsentDisclosureObject.mock() to initialize sdks using [.mock()] (or an
appropriate array of mocks) instead of the empty array so tests can assert
SDK-level fields.
| sdks: [ConsentDisclosureSDK] | ||
|
|
||
| constructor(disclosures: [ConsentDisclosure]) { | ||
| constructor(disclosures: [ConsentDisclosure], sdks?: [ConsentDisclosureSDK]) { | ||
| this.disclosures = disclosures | ||
| this.sdks = sdks ?? [] |
There was a problem hiding this comment.
[ConsentDisclosureSDK] is a tuple (exactly 1 element), not an array — this is the root cause of the pipeline failure.
In TypeScript, [T] defines a tuple with exactly one element of type T. Assigning [] to it fails because an empty array doesn't satisfy the single-element requirement. The field type should be ConsentDisclosureSDK[] (a variable-length array).
Note: the same tuple-vs-array issue exists for pre-existing fields (disclosures: [ConsentDisclosure], purposes: [number]), which happen not to break only because they're never assigned an empty array. Consider fixing those as well.
🐛 Proposed fix for the type and constructor
- sdks: [ConsentDisclosureSDK]
+ sdks: ConsentDisclosureSDK[]
- constructor(disclosures: [ConsentDisclosure], sdks?: [ConsentDisclosureSDK]) {
+ constructor(disclosures: [ConsentDisclosure], sdks?: ConsentDisclosureSDK[]) {Ideally also fix the pre-existing tuple types:
- disclosures: [ConsentDisclosure]
+ disclosures: ConsentDisclosure[]
- purposes: [number]
+ purposes: number[]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| sdks: [ConsentDisclosureSDK] | |
| constructor(disclosures: [ConsentDisclosure]) { | |
| constructor(disclosures: [ConsentDisclosure], sdks?: [ConsentDisclosureSDK]) { | |
| this.disclosures = disclosures | |
| this.sdks = sdks ?? [] | |
| sdks: ConsentDisclosureSDK[] | |
| constructor(disclosures: [ConsentDisclosure], sdks?: ConsentDisclosureSDK[]) { | |
| this.disclosures = disclosures | |
| this.sdks = sdks ?? [] |
🧰 Tools
🪛 GitHub Actions: CI/CD
[error] 18-18: TS2322: Type '[ConsentDisclosureSDK] | []' is not assignable to type '[ConsentDisclosureSDK]'.
🪛 GitHub Check: test-rn
[failure] 18-18:
Type '[ConsentDisclosureSDK] | []' is not assignable to type '[ConsentDisclosureSDK]'.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/models/ConsentDisclosureObject.tsx` around lines 14 - 18, The tuple types
like sdks: [ConsentDisclosureSDK], disclosures: [ConsentDisclosure], and
purposes: [number] are incorrect — change them to array types
ConsentDisclosureSDK[], ConsentDisclosure[], and number[] respectively; update
the ConsentDisclosureObject constructor (constructor(disclosures:
[ConsentDisclosure], sdks?: [ConsentDisclosureSDK])) to accept disclosures:
ConsentDisclosure[] and sdks?: ConsentDisclosureSDK[] and set this.sdks = sdks
?? [] (and similarly ensure this.disclosures and any purposes defaults use []
where appropriate) so empty arrays are allowed and the types match runtime
usage.
![pantoaibot[bot]](https://avatars.githubusercontent.com/in/934356?s=60&v=4){kind=small}
| export class ConsentDisclosureSDK { | ||
| name: string | ||
| use: string | ||
|
|
||
| constructor(name: string, use: string) { | ||
| this.name = name | ||
| this.use = use | ||
| } | ||
| } | ||
|
|
||
| export class ConsentDisclosureObject { | ||
|
|
||
| disclosures: [ConsentDisclosure] | ||
| sdks: [ConsentDisclosureSDK] | ||
|
|
||
| constructor(disclosures: [ConsentDisclosure]) { | ||
| constructor(disclosures: [ConsentDisclosure], sdks?: [ConsentDisclosureSDK]) { | ||
| this.disclosures = disclosures | ||
| this.sdks = sdks ?? [] | ||
| } | ||
| } | ||
|
|
||
| export class ConsentDisclosure { |
There was a problem hiding this comment.
[CRITICAL_BUG] The new TypeScript types use tuple syntax ([T]) instead of array syntax (T[]). This will cause incorrect typing/compile errors and unexpected runtime shapes. Change: - disclosures: [ConsentDisclosure] -> disclosures: ConsentDisclosure[] - sdks: [ConsentDisclosureSDK] -> sdks: ConsentDisclosureSDK[] - purposes: [number] -> purposes: number[] Also update constructor signature to accept sdks?: ConsentDisclosureSDK[] and purposes: number[]. Example corrections: constructor(disclosures: ConsentDisclosure[], sdks?: ConsentDisclosureSDK[]) { ... } and class fields disclosures: ConsentDisclosure[]; sdks: ConsentDisclosureSDK[]; purposes: number[].
export class ConsentDisclosureSDK {
name: string
use: string
constructor(name: string, use: string) {
this.name = name
this.use = use
}
}
export class ConsentDisclosureObject {
disclosures: ConsentDisclosure[]
sdks: ConsentDisclosureSDK[]
constructor(disclosures: ConsentDisclosure[], sdks?: ConsentDisclosureSDK[]) {
this.disclosures = disclosures
this.sdks = sdks ?? []
}
}
export class ConsentDisclosure {
identifier?: string
type?: ConsentDisclosureType
name?: string
maxAgeSeconds?: number
cookieRefresh: boolean
purposes: number[]
domain?: string
description?: string
constructor(
cookieRefresh: boolean,
purposes: number[],
identifier?: string,
type?: ConsentDisclosureType,
name?: string,
maxAgeSeconds?: number,
domain?: string,
description?: string,
) {
this.identifier = identifier
this.type = type
this.name = name
this.maxAgeSeconds = maxAgeSeconds
this.cookieRefresh = cookieRefresh
this.purposes = purposes
}
}
| return mapOf( | ||
| "disclosures" to disclosures.map { it.serialize() }, | ||
| "sdks" to sdks.map { it.serialize() } | ||
| ) | ||
| } | ||
|
|
||
| private fun ConsentDisclosureSDK.serialize(): Map<String, Any> { | ||
| return mapOf( | ||
| "name" to name, | ||
| "use" to use | ||
| ) |
There was a problem hiding this comment.
[VALIDATION] serialize() now serializes sdks unguarded: sdks.map { it.serialize() }. If ConsentDisclosureObject.sdks can be null, this will throw an NPE. Make this null-safe and preserve an empty list if absent: "sdks" to (sdks?.map { it.serialize() } ?: emptyList()). Also consider whether the nested structures need conversion to WritableMap at this level or are handled by the caller.
import com.usercentrics.sdk.v2.settings.data.ConsentDisclosure
import com.usercentrics.sdk.v2.settings.data.ConsentDisclosureObject
import com.usercentrics.sdk.v2.settings.data.ConsentDisclosureSDK
internal fun ConsentDisclosureObject?.serialize(): Any? {
if (this == null) return null
return mapOf(
"disclosures" to disclosures.map { it.serialize() },
"sdks" to (sdks?.map { it.serialize() } ?: emptyList())
)
}
private fun ConsentDisclosureSDK.serialize(): Map<String, Any> {
return mapOf(
"name" to name,
"use" to use
)
}
| return [ | ||
| "disclosures": self.disclosures.map { $0.toDictionary() }, | ||
| "sdks": self.sdks.map { ["name": $0.name, "use": $0.use] } | ||
| ] |
There was a problem hiding this comment.
[CRITICAL_BUG] You changed ConsentDisclosureObject.toDictionary() from returning an array of disclosures to returning a Dictionary with keys "disclosures" and "sdks". This is a breaking shape change for any consumers that previously expected an array. Either: 1) Ensure all call sites / JS/native bridge logic were updated to expect an object with keys {disclosures, sdks}; or 2) keep backward compatible shape (e.g. return the array for old callers and add a separate field or version). Additionally guard sdks for nil to avoid crashes: "sdks": self.sdks?.map { ["name": $0.name, "use": $0.use] } ?? []
extension ConsentDisclosureObject {
func toDictionary() -> Any {
return [
"disclosures": self.disclosures.map { $0.toDictionary() },
"sdks": (self.sdks ?? []).map { [
"name": $0.name as Any,
"use": $0.use as Any
]]
]
}
}
| ), | ||
| sdks = listOf() |
There was a problem hiding this comment.
[VALIDATION] Mocks were updated to add sdks = listOf() inside ConsentDisclosureObject and the expected map for "deviceStorage" changed from a list to a map with keys disclosures/sdks. Ensure all test assertions and any other mock consumers are updated to the new structure. Also verify the real SDK's ConsentDisclosureObject constructor supports the new sdks parameter; otherwise tests will not compile.
| "deviceStorage" to mapOf( | ||
| "disclosures" to listOf( | ||
| mapOf( | ||
| "identifier" to "identifier", | ||
| "type" to 2, | ||
| "name" to "name", | ||
| "maxAgeSeconds" to 123123L, | ||
| "cookieRefresh" to true, | ||
| "purposes" to listOf(1, 2, 3), | ||
| "domain" to "domain", | ||
| "description" to "description", | ||
| ) | ||
| ), | ||
| "sdks" to emptyList<Map<String, Any>>() | ||
| ), |
There was a problem hiding this comment.
[REFACTORING] In the expected map you changed "deviceStorage" from a list of maps to a single map with keys "disclosures" and "sdks". Confirm the code that consumes this expected map (test verification code) is updated accordingly. If many tests expect the old list shape, consider adding a small migration helper in tests to transform the new object into the older shape for backward-compatible assertions while the rest of the codebase is updated.
| disclosures: [disclosure], | ||
| sdks: [], | ||
| } |
There was a problem hiding this comment.
[REFACTORING] You added sdks: [] to the test ConsentDisclosureObject fixture. After fixing the model types (see src/models/ConsentDisclosureObject.tsx), update this fixture's typing (if typings are enforced) to match ConsentDisclosureSDK[] and, if useful, add a minimal SDK entry to exercise serialization paths in tests (e.g. { name: 'sdkName', use: 'purpose' }).
const consentDisclosureObject: ConsentDisclosureObject = {
disclosures: [disclosure],
sdks: [
{
name: "sdkName",
use: "purpose",
},
],
}
|
Reviewed up to commit:516fa6fe2f954d9f4d18777b5f7b43f46ef1d64b |
Code Review by Qodo
✅ 1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| export class ConsentDisclosureObject { | ||
|
|
||
| disclosures: [ConsentDisclosure] | ||
| sdks: [ConsentDisclosureSDK] | ||
|
|
||
| constructor(disclosures: [ConsentDisclosure]) { | ||
| constructor(disclosures: [ConsentDisclosure], sdks?: [ConsentDisclosureSDK]) { | ||
| this.disclosures = disclosures | ||
| this.sdks = sdks ?? [] | ||
| } |
There was a problem hiding this comment.
1. Ts sdks typed as tuple 🐞 Bug ✓ Correctness
ConsentDisclosureObject.sdks is typed as a 1-element tuple ([ConsentDisclosureSDK]) even though the native payload is a variable-length list (including empty). This makes the published TypeScript API incorrect and can break consumers that have 0 or multiple SDK entries.
Agent Prompt
### Issue description
`ConsentDisclosureObject.sdks` is declared as a tuple type (`[ConsentDisclosureSDK]`), but native returns a variable-length array (including empty). This makes the TS API contract incorrect.
### Issue Context
Android/iOS both serialize `sdks` via `.map { ... }` and Android test mocks include an empty `sdks` list.
### Fix Focus Areas
- src/models/ConsentDisclosureObject.tsx[11-19]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| internal fun ConsentDisclosure.serialize(): Any { | ||
| return mapOf( | ||
| "identifier" to identifier, | ||
| "type" to type?.ordinal, | ||
| "name" to name, | ||
| "maxAgeSeconds" to maxAgeSeconds, | ||
| "cookieRefresh" to cookieRefresh, | ||
| "purposes" to purposes, | ||
| "domain" to domain, | ||
| "description" to description, | ||
| ) |
There was a problem hiding this comment.
2. Android drops long fields 🐞 Bug ✓ Correctness
Android’s Map/List -> WritableMap/WritableArray conversion doesn’t handle Long values. Since ConsentDisclosure.serialize() includes maxAgeSeconds and tests expect it as a Long, this field can be silently omitted from the JS payload.
Agent Prompt
### Issue description
Android conversion from Kotlin `Map`/`List` to React Native `WritableMap`/`WritableArray` ignores `Long` values, which can silently drop fields like `maxAgeSeconds` from the JS payload.
### Issue Context
`ConsentDisclosure.serialize()` includes `maxAgeSeconds`, and Android tests expect it to be a `Long` (`123123L`). The conversion layer currently handles `Int`/`Double` but not `Long`.
### Fix Focus Areas
- android/src/main/java/com/usercentrics/reactnative/extensions/ReadableMapExtensions.kt[32-72]
- android/src/main/java/com/usercentrics/reactnative/extensions/ReadableMapExtensions.kt[74-116]
- android/src/main/java/com/usercentrics/reactnative/extensions/ConsentDisclosureSerializer.kt[24-35]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Co-authored-by: Cursor <cursoragent@cursor.com>
2 participants
CodeAnt-AI Description
Add SDK entries to device storage disclosures and make deviceStorage an object with disclosures and sdks
What Changed
Impact
✅ Clearer SDK disclosures in CMP data✅ Consistent deviceStorage shape across Android, iOS and web✅ Fewer missing fields in CMP mocks and tests💡 Usage Guide
Checking Your Pull Request
Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.
Talking to CodeAnt AI
Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Example
Preserve Org Learnings with CodeAnt
You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:
This helps CodeAnt AI learn and adapt to your team's coding style and standards.
Example
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
Check Your Repository Health
To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.
Summary by CodeRabbit
New Features
Tests