Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| package com.superwall.sdk.analytics.internal.trackable | ||
|
|
||
| import com.superwall.sdk.analytics.superwall.SuperwallEvent | ||
| import com.superwall.sdk.paywall.view.webview.messaging.PageViewData |
There was a problem hiding this comment.
PageViewData is inserted between two analytics.superwall imports, splitting what should be a contiguous alphabetical group. This will likely fail ktlint. The same issue occurs in SuperwallEvent.kt (lines 8-9), where paywall.view.webview.messaging.PageViewData precedes paywall.presentation.internal.* imports out of alphabetical order.
| package com.superwall.sdk.analytics.internal.trackable | |
| import com.superwall.sdk.analytics.superwall.SuperwallEvent | |
| import com.superwall.sdk.paywall.view.webview.messaging.PageViewData | |
| package com.superwall.sdk.analytics.internal.trackable | |
| import com.superwall.sdk.analytics.superwall.SuperwallEvent | |
| import com.superwall.sdk.analytics.superwall.TransactionProduct | |
| import com.superwall.sdk.paywall.view.webview.messaging.PageViewData |
In SuperwallEvent.kt, move the PageViewData import after the PaywallInfo import and before PaywallPresentationRequestStatus to maintain alphabetical ordering within the paywall.* group.
Prompt To Fix With AI
This is a comment left during a code review.
Path: superwall/src/main/java/com/superwall/sdk/analytics/internal/trackable/TrackableSuperwallEvent.kt
Line: 1-4
Comment:
**Import ordering breaks ktlint grouping**
`PageViewData` is inserted between two `analytics.superwall` imports, splitting what should be a contiguous alphabetical group. This will likely fail `ktlint`. The same issue occurs in `SuperwallEvent.kt` (lines 8-9), where `paywall.view.webview.messaging.PageViewData` precedes `paywall.presentation.internal.*` imports out of alphabetical order.
```suggestion
package com.superwall.sdk.analytics.internal.trackable
import com.superwall.sdk.analytics.superwall.SuperwallEvent
import com.superwall.sdk.analytics.superwall.TransactionProduct
import com.superwall.sdk.paywall.view.webview.messaging.PageViewData
```
In `SuperwallEvent.kt`, move the `PageViewData` import after the `PaywallInfo` import and before `PaywallPresentationRequestStatus` to maintain alphabetical ordering within the `paywall.*` group.
How can I resolve this? If you propose a fix, please make it concise.| navigationNodeId = json["navigation_node_id"]!!.jsonPrimitive.content, | ||
| previousPageNodeId = json["previous_page_node_id"]?.jsonPrimitive?.contentOrNull, | ||
| previousFlowPosition = json["previous_flow_position"]?.jsonPrimitive?.intOrNull, | ||
| navigationType = json["type"]!!.jsonPrimitive.content, |
There was a problem hiding this comment.
"type" is inconsistent with field name navigationType
Every other field in PageViewData maps directly from a snake_case JSON key that matches its Kotlin name (e.g. navigation_node_id → navigationNodeId), but navigationType maps from the ambiguous generic key "type". This deviates from the established convention and is easy to misread.
If the upstream JS contract really sends "type", consider documenting this explicitly, or (if the contract can be changed) align it to "navigation_type".
| navigationType = json["type"]!!.jsonPrimitive.content, | |
| navigationType = json["navigation_type"]!!.jsonPrimitive.content, |
Prompt To Fix With AI
This is a comment left during a code review.
Path: superwall/src/main/java/com/superwall/sdk/paywall/view/webview/messaging/PaywallMessage.kt
Line: 273
Comment:
**JSON key `"type"` is inconsistent with field name `navigationType`**
Every other field in `PageViewData` maps directly from a snake_case JSON key that matches its Kotlin name (e.g. `navigation_node_id` → `navigationNodeId`), but `navigationType` maps from the ambiguous generic key `"type"`. This deviates from the established convention and is easy to misread.
If the upstream JS contract really sends `"type"`, consider documenting this explicitly, or (if the contract can be changed) align it to `"navigation_type"`.
```suggestion
navigationType = json["navigation_type"]!!.jsonPrimitive.content,
```
How can I resolve this? If you propose a fix, please make it concise.| data class PaywallPageView( | ||
| val paywallInfo: PaywallInfo, | ||
| val data: PageViewData, | ||
| ) : SuperwallEvent() { | ||
| override val rawName: String | ||
| get() = SuperwallEvents.PaywallPageView.rawName | ||
| } |
There was a problem hiding this comment.
PageViewData is a public API type from an internal package
SuperwallEvent.PaywallPageView is part of the SDK's public surface, but its data parameter is typed as PageViewData from com.superwall.sdk.paywall.view.webview.messaging — an implementation-detail package. SDK consumers using this event via SuperwallDelegate will need to import from the webview messaging internals.
Consider moving PageViewData to a more appropriate location (e.g. alongside PaywallInfo in paywall.presentation) or to a dedicated model package so the public API doesn't expose an internal layer.
Prompt To Fix With AI
This is a comment left during a code review.
Path: superwall/src/main/java/com/superwall/sdk/analytics/superwall/SuperwallEvent.kt
Line: 143-149
Comment:
**`PageViewData` is a public API type from an internal package**
`SuperwallEvent.PaywallPageView` is part of the SDK's public surface, but its `data` parameter is typed as `PageViewData` from `com.superwall.sdk.paywall.view.webview.messaging` — an implementation-detail package. SDK consumers using this event via `SuperwallDelegate` will need to import from the webview messaging internals.
Consider moving `PageViewData` to a more appropriate location (e.g. alongside `PaywallInfo` in `paywall.presentation`) or to a dedicated model package so the public API doesn't expose an internal layer.
How can I resolve this? If you propose a fix, please make it concise.1 participant
Changes in this pull request
Checklist
CHANGELOG.mdfor any breaking changes, enhancements, or bug fixes.ktlintin the main directory and fixed any issues.Greptile Summary
This PR adds onboarding/multi-page paywall analytics support by introducing a new
paywall_page_viewevent fired whenever the user navigates between pages in a multi-step paywall. The implementation is end-to-end: a newPageViewDatamodel, apage_viewWebView message type, an internalPaywallPageViewtrackable event, and a publicSuperwallEvent.PaywallPageView— all backed by solid tests.Key changes:
PageViewData— new data class capturing page node IDs, flow position, navigation type, and optional timing infoPaywallMessage.PageView— parsed from the JS bridge; required fields are guarded with!!, optional ones use safe accessPaywallMessageHandler— dispatchesInternalSuperwallEvent.PaywallPageViewon the IO scope, consistent with existing event tracking patternsSuperwallEvent.PaywallPageView/SuperwallEvents.PaywallPageView— public and enum entries wired correctlyMinor concerns to address before merging:
TrackableSuperwallEvent.ktandSuperwallEvent.ktsplits alphabetical groups and will likely failktlintnavigationTypefield is mapped from the JSON key"type"rather than the expected"navigation_type", inconsistent with every other field in the same structPageViewDatais placed inpaywall.view.webview.messaging(an internal package) but is exposed via the publicSuperwallEvent.PaywallPageView; consider relocating it to a presentation/model packageConfidence Score: 5/5
Safe to merge; all remaining findings are P2 style/design suggestions with no correctness impact.
The implementation is correct, follows existing patterns, and is well-tested. The three open findings are all P2: import ordering (purely cosmetic/ktlint), the "type" vs "navigation_type" JSON key (works correctly if it matches the JS contract), and the PageViewData package placement (a design preference). None affect runtime behaviour.
Import ordering in TrackableSuperwallEvent.kt and SuperwallEvent.kt should be fixed before ktlint runs.
Important Files Changed
timeOnPreviousPageMsasInt?is fine for page-dwell durations.PageViewmessage type and parses it from JSON; required field handling is consistent with other messages, butnavigationTypeis mapped from the generic JSON key"type"rather than the expected"navigation_type".PageViewmessage by tracking anInternalSuperwallEvent.PaywallPageViewon the IO scope; follows existing patterns correctly.PaywallPageViewinternal event with correct parameter mapping; import ordering forPageViewDatabreaks the alphabetical grouping ofanalytics.superwallimports.PaywallPageViewevent; exposesPageViewDatafrom an internal webview package in the public API surface, and has an out-of-order import.PaywallPageView("paywall_page_view")enum entry in the correct position; no issues.Sequence Diagram
sequenceDiagram participant JS as Paywall JS (WebView) participant MH as PaywallMessageHandler participant MQ as parseWrappedPaywallMessages participant ISE as InternalSuperwallEvent.PaywallPageView participant T as track() participant SE as SuperwallEvent.PaywallPageView JS->>MH: postMessage(json with event_name="page_view") MH->>MQ: parseWrappedPaywallMessages(json) MQ-->>MH: Result containing PageView(PageViewData) MH->>MH: handle(PaywallMessage.PageView) MH->>MH: messageHandler?.state?.info (get PaywallInfo) MH->>ISE: PaywallPageView(paywallInfo, data) MH->>T: ioScope.launch { track(event) } T->>SE: SuperwallEvent.PaywallPageView emitted to delegatePrompt To Fix All With AI
Reviews (1): Last reviewed commit: "Add onboarding analytics support" | Re-trigger Greptile