-
Notifications
You must be signed in to change notification settings - Fork 16
feat(MSDK-3779): add consent-or-pay login/subscribe callbacks #242
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 |
|---|---|---|
|
|
@@ -14,7 +14,13 @@ interface UsercentricsProxy { | |
| fun initialize(context: Context, options: UsercentricsOptions) | ||
| fun isReady(onSuccess: (UsercentricsReadyStatus) -> Unit, onFailure: (UsercentricsError) -> Unit) | ||
|
|
||
| fun showFirstLayer(activity: Activity, bannerSettings: BannerSettings?, promise: Promise) | ||
| fun showFirstLayer( | ||
| activity: Activity, | ||
| bannerSettings: BannerSettings?, | ||
| onLoginClicked: (String?) -> Unit, | ||
| onSubscribeClicked: (String?) -> Unit, | ||
| promise: Promise, | ||
| ) | ||
|
Comment on lines
+17
to
+23
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 Android test fake still implements the old Assessment: 🟠 Prompt for AI Agent 🤖This is a comment left during a code review.
**Path:** android/src/main/java/com/usercentrics/reactnative/api/UsercentricsProxy.kt
**Line:** 17:23
**Comment:**
*Api Mismatch: The Android test fake still implements the old `showFirstLayer` signature, so Android test compilation fails because it no longer satisfies `UsercentricsProxy`.
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.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
Comment on lines
+17
to
+23
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. 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win 🔎 Supported by static analysis🏁 Script executed: #!/bin/bash
set -euo pipefail
rg -n -C 5 \
'UsercentricsProxy|override fun showFirstLayer' \
android --glob '*.kt'Repository: Usercentrics/react-native-sdk Length of output: 50385 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '--- declarations and overrides ---'
rg -n '^(internal )?(class|interface) .*UsercentricsProxy|: UsercentricsProxy|override fun showFirstLayer' \
android --glob '*.kt'
printf '%s\n' '--- production interface and implementation ---'
sed -n '10,65p' android/src/main/java/com/usercentrics/reactnative/api/UsercentricsProxy.kt
printf '%s\n' '--- test double ---'
sed -n '1,65p' android/src/androidTest/java/com/usercentrics/reactnative/api/FakeUsercentricsProxy.kt
printf '%s\n' '--- module call site ---'
sed -n '35,75p' android/src/main/java/com/usercentrics/reactnative/RNUsercentricsModule.ktRepository: Usercentrics/react-native-sdk Length of output: 6484 🤖 get_repo_knowledge executed:
Length of output: 33306 Update The test double still uses the old signature and omits 🤖 Prompt for AI Agents |
||
| fun showSecondLayer(activity: Activity, bannerSettings: BannerSettings?, promise: Promise) | ||
| } | ||
|
|
||
|
|
@@ -39,11 +45,15 @@ internal class UsercentricsProxyImpl : UsercentricsProxy { | |
| override fun showFirstLayer( | ||
| activity: Activity, | ||
| bannerSettings: BannerSettings?, | ||
| onLoginClicked: (String?) -> Unit, | ||
| onSubscribeClicked: (String?) -> Unit, | ||
| promise: Promise, | ||
| ) { | ||
| UsercentricsBanner(activity, bannerSettings).showFirstLayer { | ||
| promise.resolve(it?.toWritableMap()) | ||
| } | ||
| UsercentricsBanner(activity, bannerSettings).showFirstLayer( | ||
| callback = { promise.resolve(it?.toWritableMap()) }, | ||
| onLoginClicked = onLoginClicked, | ||
| onSubscribeClicked = onSubscribeClicked, | ||
| ) | ||
| } | ||
|
|
||
| override fun showSecondLayer( | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -29,6 +29,13 @@ NS_ASSUME_NONNULL_BEGIN | |
| - (void)clearUserSession:(RCTPromiseResolveBlock)resolve | ||
| reject:(RCTPromiseRejectBlock)reject; | ||
|
|
||
| // Consent or Pay | ||
| - (void)notifyLoginSuccess:(RCTPromiseResolveBlock)resolve | ||
| reject:(RCTPromiseRejectBlock)reject; | ||
|
|
||
| - (void)notifySubscribeSuccess:(RCTPromiseResolveBlock)resolve | ||
| reject:(RCTPromiseRejectBlock)reject; | ||
|
|
||
|
Comment on lines
+32
to
+38
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. 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win Do not edit the generated Codegen header. Keep these method declarations in the TypeScript TurboModule spec and regenerate As per coding guidelines, 🤖 Prompt for AI AgentsSource: Coding guidelines |
||
| // Data Retrieval | ||
| - (void)getConsents:(RCTPromiseResolveBlock)resolve | ||
| reject:(RCTPromiseRejectBlock)reject; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -216,10 +216,20 @@ final class FakeUsercentricsManager: UsercentricsManager { | |
| } | ||
|
|
||
| var showFirstLayerBannerSettings: BannerSettings? | ||
| var loginClickedUrl: String? | ||
| var subscribeClickedUrl: String? | ||
|
|
||
| func showFirstLayer(bannerSettings: BannerSettings?, | ||
| onLoginClicked: @escaping (String?) -> Void, | ||
| onSubscribeClicked: @escaping (String?) -> Void, | ||
| dismissViewHandler: @escaping (UsercentricsConsentUserResponse) -> Void) { | ||
| self.showFirstLayerBannerSettings = bannerSettings | ||
| if let loginClickedUrl = loginClickedUrl { | ||
| onLoginClicked(loginClickedUrl) | ||
| } | ||
| if let subscribeClickedUrl = subscribeClickedUrl { | ||
| onSubscribeClicked(subscribeClickedUrl) | ||
| } | ||
| dismissViewHandler(UsercentricsConsentUserResponse(consents: [], controllerId: "", userInteraction: .acceptAll)) | ||
|
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. 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win Keep the fake first layer open after Consent-or-Pay clicks. When 🤖 Prompt for AI Agents |
||
| } | ||
|
|
||
|
|
@@ -244,4 +254,22 @@ final class FakeUsercentricsManager: UsercentricsManager { | |
| onError(clearUserSessionError) | ||
| } | ||
| } | ||
|
|
||
| var notifyLoginSuccessError: Error? | ||
| func notifyLoginSuccess(onSuccess: @escaping (() -> Void), onError: @escaping ((Error) -> Void)) { | ||
| if let notifyLoginSuccessError = notifyLoginSuccessError { | ||
| onError(notifyLoginSuccessError) | ||
| return | ||
| } | ||
| onSuccess() | ||
| } | ||
|
|
||
| var notifySubscribeSuccessError: Error? | ||
| func notifySubscribeSuccess(onSuccess: @escaping (() -> Void), onError: @escaping ((Error) -> Void)) { | ||
| if let notifySubscribeSuccessError = notifySubscribeSuccessError { | ||
| onError(notifySubscribeSuccessError) | ||
| return | ||
| } | ||
| onSuccess() | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,5 @@ | ||
| import React from 'react'; | ||
| import { Button, StyleSheet, View } from 'react-native'; | ||
| import { Alert, Button, StyleSheet, View } from 'react-native'; | ||
| import { | ||
| BannerSettings, | ||
| Usercentrics, | ||
|
|
@@ -42,6 +42,37 @@ export const HomeScreen = ({ navigation }: { navigation: any }) => { | |
| .catch(e => console.error('[Usercentrics] status failed:', e)); | ||
| }, [showFirstLayer]); | ||
|
|
||
| React.useEffect(() => { | ||
| const loginSubscription = Usercentrics.onLoginClicked(async (url) => { | ||
| console.log('[Usercentrics] onLoginClicked:', url); | ||
| Alert.alert('onLoginClicked', `url: ${url}`); | ||
| try { | ||
| await Usercentrics.notifyLoginSuccess(); | ||
|
Comment on lines
+46
to
+50
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: Both handlers notify success immediately on a tap, before login or subscription is confirmed, so stored TCF consent can be cleared after an unsuccessful attempt. [logic error] Assessment: 🟠 Prompt for AI Agent 🤖This is a comment left during a code review.
**Path:** sample/src/screens/Home.tsx
**Line:** 46:50
**Comment:**
*Logic Error: Both handlers notify success immediately on a tap, before login or subscription is confirmed, so stored TCF consent can be cleared after an unsuccessful attempt.
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.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fixThere 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. 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win Report success only after the operation succeeds. The click listeners do not start or await login or subscription. They call the success methods immediately, which clears stored TCF data. Start the corresponding flow first, then call its notification after success. Explicitly dismiss the banner afterward because these notifications do not dismiss it automatically. 🤖 Prompt for AI Agents |
||
| console.log('[Usercentrics] notifyLoginSuccess done'); | ||
| Alert.alert('notifyLoginSuccess', 'TCF storage cleared'); | ||
|
Comment on lines
+49
to
+52
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. 3. Banner taps erase consent prematurely HomeScreen calls notifyLoginSuccess and notifySubscribeSuccess directly from the click callbacks without opening the supplied URL or awaiting an authentication or subscription result. Any tap therefore clears stored consent even when the user abandons or fails the corresponding flow, and copied sample integrations will reproduce that behavior. Agent Prompt
|
||
| } catch (e) { | ||
| console.error('[Usercentrics] notifyLoginSuccess failed:', e); | ||
| Alert.alert('notifyLoginSuccess failed', String(e)); | ||
| } | ||
| }); | ||
| const subscribeSubscription = Usercentrics.onSubscribeClicked(async (url) => { | ||
| console.log('[Usercentrics] onSubscribeClicked:', url); | ||
| Alert.alert('onSubscribeClicked', `url: ${url}`); | ||
| try { | ||
| await Usercentrics.notifySubscribeSuccess(); | ||
| console.log('[Usercentrics] notifySubscribeSuccess done'); | ||
| Alert.alert('notifySubscribeSuccess', 'TCF storage cleared'); | ||
| } catch (e) { | ||
| console.error('[Usercentrics] notifySubscribeSuccess failed:', e); | ||
| Alert.alert('notifySubscribeSuccess failed', String(e)); | ||
| } | ||
| }); | ||
| return () => { | ||
| loginSubscription.remove(); | ||
| subscribeSubscription.remove(); | ||
| }; | ||
| }, []); | ||
|
Comment on lines
+45
to
+74
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. [NITPICK] The sample's onLoginClicked/onSubscribeClicked handlers call notifyLoginSuccess/notifySubscribeSuccess immediately. The SDK docs you added say the host app should call notify* once login/subscription is confirmed. Consider clarifying in the sample (or delaying notify* until a simulated confirmation) so the sample doesn't encourage calling notify* immediately before actual login/subscription success. // Inside HomeScreen, replace the immediate notify* calls with a simulated
// async confirmation so the sample matches the docs' guidance.
React.useEffect(() => {
const loginSubscription = Usercentrics.onLoginClicked(async (url) => {
console.log('[Usercentrics] onLoginClicked:', url);
Alert.alert('onLoginClicked', `url: ${url}`);
// Simulate host-app login flow completing before notifying success
const confirmed = await new Promise<boolean>((resolve) => {
Alert.alert(
'Simulate login',
'Pretend the user has logged in successfully?',
[
{ text: 'Cancel', style: 'cancel', onPress: () => resolve(false) },
{ text: 'OK', onPress: () => resolve(true) },
],
);
});
if (!confirmed) {
return;
}
try {
await Usercentrics.notifyLoginSuccess();
console.log('[Usercentrics] notifyLoginSuccess done');
Alert.alert('notifyLoginSuccess', 'TCF storage cleared');
} catch (e) {
console.error('[Usercentrics] notifyLoginSuccess failed:', e);
Alert.alert('notifyLoginSuccess failed', String(e));
}
});
const subscribeSubscription = Usercentrics.onSubscribeClicked(async (url) => {
console.log('[Usercentrics] onSubscribeClicked:', url);
Alert.alert('onSubscribeClicked', `url: ${url}`);
// Simulate host-app subscription flow completing before notifying success
const confirmed = await new Promise<boolean>((resolve) => {
Alert.alert(
'Simulate subscription',
'Pretend the user has subscribed successfully?',
[
{ text: 'Cancel', style: 'cancel', onPress: () => resolve(false) },
{ text: 'OK', onPress: () => resolve(true) },
],
);
});
if (!confirmed) {
return;
}
try {
await Usercentrics.notifySubscribeSuccess();
console.log('[Usercentrics] notifySubscribeSuccess done');
Alert.alert('notifySubscribeSuccess', 'TCF storage cleared');
} catch (e) {
console.error('[Usercentrics] notifySubscribeSuccess failed:', e);
Alert.alert('notifySubscribeSuccess failed', String(e));
}
});
return () => {
loginSubscription.remove();
subscribeSubscription.remove();
};
}, []); |
||
|
|
||
| async function showSecondLayer() { | ||
| try { | ||
| const response = await Usercentrics.showSecondLayer({ | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -171,4 +171,30 @@ export const Usercentrics = { | |
| onGppSectionChange: (callback: (payload: GppSectionChangePayload) => void): EmitterSubscription => { | ||
| return eventEmitter.addListener("onGppSectionChange", callback); | ||
| }, | ||
|
|
||
| // Fires when the user taps the Consent-or-Pay 1st-layer subscriber-login link. The banner is not | ||
| // dismissed automatically — call notifyLoginSuccess once the host app confirms login, then dismiss | ||
| // the banner yourself. | ||
|
Comment on lines
+175
to
+177
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. 2. Apps cannot dismiss the banner The new onLoginClicked and onSubscribeClicked contracts require React Native callers to dismiss the banner after notifying success, but the bridge retains neither the banner instance nor a public dismissal method. Both native implementations construct the banner inside showFirstLayer, so an app completing login or subscription through these callbacks has no JavaScript path to perform the documented dismissal. Agent Prompt
|
||
| onLoginClicked: (callback: (url: string | null) => void): EmitterSubscription => { | ||
| return eventEmitter.addListener("onLoginClicked", callback); | ||
| }, | ||
|
Comment on lines
+178
to
+180
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: Adding these subscriptions triggers native listener removal, but Android ignores their event names and can dispose an active GPP subscription when a login or subscribe listener is removed. [api mismatch] Assessment: 🟠 Prompt for AI Agent 🤖This is a comment left during a code review.
**Path:** src/Usercentrics.tsx
**Line:** 178:180
**Comment:**
*Api Mismatch: Adding these subscriptions triggers native listener removal, but Android ignores their event names and can dispose an active GPP subscription when a login or subscribe listener is removed.
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.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
Comment on lines
+178
to
+180
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. 1. Removing a banner listener stops updates Android addListener increments gppSectionChangeListenersCount only for the section-change event, while the shared removeListeners method decrements that count when either new banner subscription is removed. If a section-change listener remains when a login or subscription listener is removed, its native subscription can be disposed and subsequent changes no longer reach JavaScript. Agent Prompt
|
||
|
|
||
| // Fires when the user taps the Consent-or-Pay 1st-layer Reject & Subscribe button. The banner is not | ||
| // dismissed automatically — call notifySubscribeSuccess once the host app confirms the subscription, | ||
| // then dismiss the banner yourself. | ||
| onSubscribeClicked: (callback: (url: string | null) => void): EmitterSubscription => { | ||
| return eventEmitter.addListener("onSubscribeClicked", callback); | ||
| }, | ||
|
Comment on lines
+182
to
+187
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 banner remains open after the click, but this public API exposes no dismissal operation, leaving the host unable to perform the documented final step. [incomplete implementation] Assessment: 🟠 Prompt for AI Agent 🤖This is a comment left during a code review.
**Path:** src/Usercentrics.tsx
**Line:** 182:187
**Comment:**
*Incomplete Implementation: The banner remains open after the click, but this public API exposes no dismissal operation, leaving the host unable to perform the documented final step.
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.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix |
||
|
|
||
| // Clears stored TCF consent data after a successful Consent-or-Pay login. | ||
| notifyLoginSuccess: async (): Promise<void> => { | ||
| await RNUsercentricsModule.isReady(); | ||
| return RNUsercentricsModule.notifyLoginSuccess(); | ||
| }, | ||
|
|
||
| // Clears stored TCF consent data after a successful Consent-or-Pay subscription. | ||
| notifySubscribeSuccess: async (): Promise<void> => { | ||
| await RNUsercentricsModule.isReady(); | ||
| return RNUsercentricsModule.notifySubscribeSuccess(); | ||
| }, | ||
| } | ||
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.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
Repository: Usercentrics/react-native-sdk
Length of output: 346
🏁 Script executed:
Repository: Usercentrics/react-native-sdk
Length of output: 5793
🤖 get_repo_knowledge executed:
get_repo_knowledge Usercentrics/react-native-sdk /tmp/coderabbit-repo-knowledge/usercentrics-react-native-sdk-8bb5a603/conventions /tmp/coderabbit-repo-knowledge/usercentrics-react-native-sdk-8bb5a603/architectureLength of output: 35876
Pass
promiseas a named argument.The
usercentricsProxy.showFirstLayercall passespromisepositionally after named arguments. Kotlin does not allow this, so the file will not compile. Change the final argument topromise = promise.Proposed fix
usercentricsProxy.showFirstLayer( activity, bannerSettings, onLoginClicked = { url -> emitEvent(ON_LOGIN_CLICKED_EVENT, url) }, onSubscribeClicked = { url -> emitEvent(ON_SUBSCRIBE_CLICKED_EVENT, url) }, - promise, + promise = promise, )📝 Committable suggestion
🤖 Prompt for AI Agents