UI events for paywall component interactions#3287
UI events for paywall component interactions#3287MonikaMateska wants to merge 8 commits intomainfrom
Conversation
| LaunchedEffect(pagerState, pageCount, style.componentName, controlInteractionTracker) { | ||
| var previousPage = pagerState.currentPage | ||
| snapshotFlow { pagerState.currentPage }.collect { page -> | ||
| if (page != previousPage) { | ||
| if (skipProgrammaticPageTracking.getAndSet(false)) { | ||
| // Auto-advance scroll; do not emit control interaction (parity with iOS). | ||
| } else { | ||
| val logicalPage = page % pageCount | ||
| controlInteractionTracker.track( | ||
| componentType = PaywallControlType.CAROUSEL, | ||
| componentName = style.componentName, | ||
| componentValue = logicalPage.toString(), | ||
| componentUrl = null, | ||
| ) | ||
| } | ||
| previousPage = page |
There was a problem hiding this comment.
We had a bug in non-looping carousels with auto-advance turned on.
The code always computed the next page as (current + 1) mod page count, which is correct when the carousel loops but wrong when it does not. On the last page it would wrap back to the first page and keep animating forever instead of staying put.
Single-page carousels had the same kind of problem: modulo math still produced a “next” page when there was nowhere real to go. The fix is to only schedule the next auto-advance page when there actually is one, for non-loop mode that means stop once you are on the last page (and bail out for empty or single-page carousels).
Looping behavior is unchanged; we still just increment the page index there.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| */ | ||
| @JvmSynthetic | ||
| internal fun tierSelectorControlInteractionValue(tier: TemplateConfiguration.TierInfo): String = | ||
| tier.name.takeUnless { it.isBlank() } ?: "" |
There was a problem hiding this comment.
Tier selector fallback uses empty string instead of tier ID
Medium Severity
tierSelectorControlInteractionValue falls back to an empty string "" when the tier name is blank, but the PR specification states it should fall back to tier.id. The expression tier.name.takeUnless { it.isBlank() } ?: "" discards the tier identifier entirely. The corresponding tests also assert the incorrect empty-string behavior.
Additional Locations (1)
There was a problem hiding this comment.
We could consider fullbacking to the tier id here. Would appreciate any thoughts.
tonidero
left a comment
tonidero
left a comment
There was a problem hiding this comment.
Looking really great! Left a few initial comments but I think it's close!
| } | ||
|
|
||
| @InternalRevenueCatAPI | ||
| internal fun PaywallControlType.toWireString(): String = when (this) { |
There was a problem hiding this comment.
Hmm since PaywallControlType is serializable, should we instead just serialize the enum value? (Alternatively, not sure if we just want to make the type not serializable and keep this. Just keeping both means we need to keep serialization logic in both places.)
| myActionInProgress = true | ||
| state.update(actionInProgress = true) |
There was a problem hiding this comment.
Very nitpicky... but should we set this to true before doing anything else to minimize chances of 2 successive clicks making it through?
| ) | ||
|
|
||
| val coroutineScope = rememberCoroutineScope() | ||
| val controlInteractionTracker = LocalPaywallControlInteractionTracker.current |
There was a problem hiding this comment.
Hmm so we've been trying to minimize usages of this pattern, mostly because implicit dependencies like this are more error prone when considering how to keep scope in consideration and that you need to remember to mock it the values as well... (This might be something we should add to the AGENTS.md 😅)
I believe the alternative would be to pass the data we want to track up the action handlers when interacting, which is indeed a lot more plumbing... (though maybe in true MVVM, we should be handling all these interactions from the view model but that's trickier 😅)
So I guess I'm not too opposed to keep it like this. Thoughts @JayShortway @vegaro?
There was a problem hiding this comment.
Yea we should avoid this as much as possible imo, as it is error prone and can lead to crashes if not provided. In fact, detekt failed on this:
CompositionLocals are implicit dependencies and creating new ones should be avoided.
There was a problem hiding this comment.
definitely a good idea to add to AGENTS.md!
| is ButtonComponentStyle.Action.WebProductSelection, | ||
| is ButtonComponentStyle.Action.CustomWebCheckout, | ||
| -> true | ||
| is ButtonComponentStyle.Action.RestorePurchases, |
There was a problem hiding this comment.
Hmm should Restore Purchases emit paywall_control_interaction? I guess it's kinda purchase related (though it doesn't perform a purchase, just restores them...). Just doubting because, if what we want to know is whether a restore purchases happened, wouldn't we also want to know it for purchase related items like starting to purchase a package, or navigating to a web checkout? (I'm not familiar with the specs so I might be missing something 😅)
| } | ||
| } | ||
|
|
||
| val skipProgrammaticPageTracking = remember { AtomicBoolean(false) } |
There was a problem hiding this comment.
Just to check, is this reset to false if the user manually moves to previous pages in the carousel?
Also worth checking what happens (if you haven't already) with slow manual scrolling and back, like moving > 50% to the next page but moving back before releasing.
| pagerState: PagerState, | ||
| shouldLoop: Boolean, | ||
| pageCount: Int, | ||
| skipProgrammaticPageTracking: AtomicBoolean, |
There was a problem hiding this comment.
Hmm so this is using AtomicBoolean as a way to pass a shared mutable state... Would it be worth to pass a mutable state directly? Though I guess it's not updating UI based on this value... So it's probably ok as is.
There was a problem hiding this comment.
Why is it atomic though? Also, isn't a regular boolean all the "mutable" state we need? Compose will recompose the thing if the boolean changed.
| checked = checked, | ||
| onCheckedChange = { state.update(selectedTabIndex = if (it) 1 else 0) }, | ||
| onCheckedChange = { | ||
| state.update(selectedTabIndex = if (it) 1 else 0) |
There was a problem hiding this comment.
Similar nitpick as before, I would wait to update the state until after the tracking
| textAlign = textState.textAlign, | ||
| style = textStyle, | ||
| ) | ||
| } |
There was a problem hiding this comment.
Would be good to add some UI tests that create a dummy component of these types and make sure the LocalPaywallControlInteractionTracker or viewModel method is called with the correct params. I think that's the kinda thing that's easy to break 😅 . For example, for this one, it could be a test in TextComponentViewTests I think.
MonikaMateska
changed the title
| @@ -1,4 +1,5 @@ | |||
| @file:JvmSynthetic | |||
| @file:OptIn(com.revenuecat.purchases.InternalRevenueCatAPI::class) | |||
There was a problem hiding this comment.
FQN in @file:OptIn instead of using imports
Low Severity
Several new/modified files use @file:OptIn(com.revenuecat.purchases.InternalRevenueCatAPI::class) with a fully-qualified reference instead of importing InternalRevenueCatAPI and using @file:OptIn(InternalRevenueCatAPI::class). The existing codebase (e.g. VideoView.kt) already follows the import pattern. This violates the rule requiring FQN imports over inline fully-qualified references.
Additional Locations (2)
Triggered by learned rule: Use FQN imports, not inline fully-qualified references
Reviewed by Cursor Bugbot for commit bdd1a74. Configure here.
Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 3 total unresolved issues (including 2 from previous reviews).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 022e6b5. Configure here.
| componentName = style.componentName, | ||
| componentValue = if (it) "on" else "off", | ||
| ), | ||
| ) |
There was a problem hiding this comment.
Toggle tracks interaction after state update, unlike other components
Low Severity
In TabControlToggleView, state.update(selectedTabIndex = ...) is called before componentInteractionTracker.track(...). Every other component in this PR (TabControlButtonView, PackageComponentView, ButtonComponentView) tracks the interaction before updating state. This inconsistency means the UI state has already mutated by the time the tracking call fires, which is the opposite of the intended order. The PR reviewer explicitly flagged this: "I would wait to update the state until after the tracking."
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 022e6b5. Configure here.
4 participants
Checklist
purchases-iosand hybridsMotivation
This PR is part of the “Posting UI Events to Integrations” initiative and focuses on enabling
paywall_component_interactionon Android so UI behavior events can flow through the existing paywall event pipeline and be forwarded to integrations without app-side callback wiring.It addresses the current gap where UI events exist on other platforms but are not consistently represented in Android’s paywall event payloads—including package selection, package-selection sheet lifecycle, and extended interaction metadata aligned with iOS. This is especially important for the upcoming Campaigns/Workflows/Checkpoints direction and for high-integration customers (e.g. Leadtech).
Resolves: PWENG-15
Description
This PR adds Android support for
paywall_component_interactionand wires control interaction metadata through purchases + RevenueCatUI, aligned with the iOS wire format and semantics (same backend field names as iOS’s paywall component interaction map, e.g.origin_package_id,current_package_id, etc.).What was added
paywall_component_interaction(unchanged type string on Android; payload extended to match iOS).component_type,component_name,component_value,component_url(optional)ComponentInteractionData/ backend map):origin_index,destination_index,default_indexorigin_context_name,destination_context_nameorigin_package_id,destination_package_id,default_package_idorigin_product_id,destination_product_id,default_product_idcurrent_package_id,resulting_package_id,current_product_id,resulting_product_idtab,switch,carousel,button,textpackage(selectable package row)package_selection_sheet(bottom sheet open/close lifecycle)PaywallControlInteractionDataexpanded;PaywallEvent/PaywallStoredEventround-tripBackendEvent.Paywalls+BackendStoredEvent/PaywallStoredEventmapping viaPaywallControlInteractionData.toBackendControlFields()→BackendPaywallControlFieldsPaywallViewModel.trackControlInteraction(PaywallControlInteractionData)as the primary entry point; 4-parameter overload builds a minimalPaywallControlInteractionDatafor legacy call sitesPurchases.track(PaywallEvent)unchanged as the sinkLocalPaywallControlInteractionTrackernow takes a singlePaywallControlInteractionDataargumentpaywallPackageSelectionSheetOpen/Close,paywallPackageRowSelection,paywallTierSelection)nameonPackageComponent→PackageComponentStyle.componentNameforcomponent_nameon package rows when the dashboard provides itUI coverage included
V2 (components paywall)
on/off)navigate_to_url+ URL)package):component_value= destination package id; origin/destination/default package + product ids when applicablepackage_selection_sheet):component_value=open;current_*= root selection at open timecomponent_value=close;current_*= selection while sheet was open;resulting_*= root afterresetToDefaultPackage()V1 (template / legacy paywall)
Footeris not shown)Semantics used
component_namenamewhen available (including package components when JSON includesname)all_plans_button,restore_button,terms_link,privacy_link,tier_selector)component_valuerestore_purchases,navigate_to_terms,navigate_to_privacy_policy,toggle_all_plans,navigate_to_url,open/closefor package sheet, etc.)on/offcurrent/resultingpair on dismiss)Note
Medium Risk
Adds a new
paywall_component_interactionevent type and threads many new optional fields through paywall event serialization, storage, and flush to backend; mistakes could drop or mis-shape analytics payloads. UI wiring spans multiple components (tabs/carousel/buttons/text/package selection), increasing the chance of noisy or missing event emission.Overview
Adds first-class support for paywall component interaction analytics on Android by introducing
PaywallEventType.COMPONENT_INTERACTIONwith a typedPaywallComponentInteractionDatapayload (including extended index/context/package/product metadata and legacy JSON aliasing via@JsonNames("controlInteraction")).Extends
BackendEvent.Paywallsand both stored-event conversion paths (PaywallEvent.toBackendStoredEvent,PaywallStoredEvent.toBackendEvent) to flatten and send these component fields (component_*,origin/destination/default_*,current/resulting_*).Wires RevenueCatUI to emit these interactions for V2 components (non-purchase buttons, carousel page changes excluding auto-advance, tab buttons/toggles, markdown link taps, package row selection, package-selection sheet open/close) and for legacy templates (footer actions + Template 7 tier selection), using a new composition-local
PaywallComponentInteractionTrackerand helper factories; adds optional dashboardnamepropagation into component models/styles and updates/expands serialization/unit tests accordingly.Reviewed by Cursor Bugbot for commit 022e6b5. Bugbot is set up for automated code reviews on this repo. Configure here.