Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| case testModeModalClose | ||
|
|
||
| /// When a user navigates to a page in a multi-page paywall. | ||
| case paywallPageView(paywallInfo: PaywallInfo) |
There was a problem hiding this comment.
The paywallPageView event only carries paywallInfo as its associated data. SDK consumers receiving this event via the delegate callback will know that a page view occurred, but won't be able to determine which page was viewed, what the navigation type was, or access any other page-specific details.
The internal tracking (lines 262–272 of PaywallMessageHandler.swift) captures all this context—pageNodeId, pageIndex, pageName, navigationType, previousPageNodeId, etc.—but none of it is surfaced in the public event.
Compare this to similar rich events like surveyResponse(survey:selectedOption:customResponse:paywallInfo:) or triggerFire(placementName:result:), which embed their contextual data directly. Consider whether page-specific fields should be included as associated values to make the event useful to SDK integrators:
case paywallPageView(
paywallInfo: PaywallInfo,
pageNodeId: String,
pageIndex: Int,
pageName: String,
navigationType: String
)Prompt To Fix With AI
This is a comment left during a code review.
Path: Sources/SuperwallKit/Analytics/Superwall Placement/SuperwallEvent.swift
Line: 285
Comment:
The `paywallPageView` event only carries `paywallInfo` as its associated data. SDK consumers receiving this event via the delegate callback will know that a page view occurred, but won't be able to determine which page was viewed, what the navigation type was, or access any other page-specific details.
The internal tracking (lines 262–272 of `PaywallMessageHandler.swift`) captures all this context—`pageNodeId`, `pageIndex`, `pageName`, `navigationType`, `previousPageNodeId`, etc.—but none of it is surfaced in the public event.
Compare this to similar rich events like `surveyResponse(survey:selectedOption:customResponse:paywallInfo:)` or `triggerFire(placementName:result:)`, which embed their contextual data directly. Consider whether page-specific fields should be included as associated values to make the event useful to SDK integrators:
```swift
case paywallPageView(
paywallInfo: PaywallInfo,
pageNodeId: String,
pageIndex: Int,
pageName: String,
navigationType: String
)
```
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| case .pageView: | ||
| let pageNodeId = try values.decode(String.self, forKey: .pageNodeId) | ||
| let pageIndex = try values.decode(Int.self, forKey: .pageIndex) | ||
| let pageName = try values.decode(String.self, forKey: .pageName) | ||
| let navigationNodeId = try values.decode(String.self, forKey: .navigationNodeId) | ||
| let previousPageNodeId = try? values.decode(String.self, forKey: .previousPageNodeId) | ||
| let previousPageIndex = try? values.decode(Int.self, forKey: .previousPageIndex) | ||
| let type = try values.decode(String.self, forKey: .type) | ||
| let timeOnPreviousPageMs = try? values.decode(Int.self, forKey: .timeOnPreviousPageMs) | ||
| self = .pageView( | ||
| pageNodeId: pageNodeId, | ||
| pageIndex: pageIndex, | ||
| pageName: pageName, | ||
| navigationNodeId: navigationNodeId, | ||
| previousPageNodeId: previousPageNodeId, | ||
| previousPageIndex: previousPageIndex, | ||
| type: type, | ||
| timeOnPreviousPageMs: timeOnPreviousPageMs | ||
| ) | ||
| return | ||
| } |
There was a problem hiding this comment.
The .pageView case diverges from the established error-handling pattern used by all other message types.
Lines 333–336 and 339 use non-optional try values.decode(...) for required fields (pageNodeId, pageIndex, pageName, navigationNodeId, type). If any of these fields are missing or malformed, this will throw a DecodingError.
Every other case in this decoder (.hapticFeedback, .requestCallback, .stripeCheckoutStart, etc.) uses try? values.decode(...) with an if-let guard. If decoding fails, the guard fails and execution falls through to the catch-all error at line 355, which throws PaywallMessageError.decoding(...).
This inconsistency means a malformed page_view message will propagate a DecodingError instead of the expected PaywallMessageError, which could cause unexpected behavior in callers that specifically catch PaywallMessageError.
For consistency, wrap the required decodes in try? / if-let guards matching the pattern used by .hapticFeedback, .requestCallback, and all other cases.
Prompt To Fix With AI
This is a comment left during a code review.
Path: Sources/SuperwallKit/Paywall/View Controller/Web View/Message Handling/PaywallMessage.swift
Line: 332-352
Comment:
The `.pageView` case diverges from the established error-handling pattern used by all other message types.
Lines 333–336 and 339 use non-optional `try values.decode(...)` for required fields (`pageNodeId`, `pageIndex`, `pageName`, `navigationNodeId`, `type`). If any of these fields are missing or malformed, this will throw a `DecodingError`.
Every other case in this decoder (`.hapticFeedback`, `.requestCallback`, `.stripeCheckoutStart`, etc.) uses `try? values.decode(...)` with an `if-let` guard. If decoding fails, the guard fails and execution falls through to the catch-all error at line 355, which throws `PaywallMessageError.decoding(...)`.
This inconsistency means a malformed `page_view` message will propagate a `DecodingError` instead of the expected `PaywallMessageError`, which could cause unexpected behavior in callers that specifically catch `PaywallMessageError`.
For consistency, wrap the required decodes in `try?` / `if-let` guards matching the pattern used by `.hapticFeedback`, `.requestCallback`, and all other cases.
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.swiftlintin the main directory and fixed any issues.Greptile Summary
This PR adds multi-page paywall navigation tracking by introducing a
page_viewweb message, aPaywallPageViewinternal event, and apresentationIdcorrelation ID that flows fromgetRawPaywallthroughPaywall→PaywallInfo→ analytics parameters. The changes follow established patterns for message handling and event tracking.Key changes:
pageViewcase onPaywallMessagewith 8 associated values (pageNodeId, pageName, pageIndex, navigationNodeId, navigationType, and optional previous-page fields)InternalSuperwallEvent.PaywallPageViewstruct that serialises all page fields into analytics paramsSuperwallEvent.paywallPageViewandSuperwallEventObjc.paywallPageViewcases surfacing the event to SDK consumerspresentationId: String?added toPaywallandPaywallInfo, assigned as a fresh UUID pergetRawPaywallcall, and included inplacementParams()as"presentation_id"Issues to address:
Error-handling inconsistency in PaywallMessage decoding: The
.pageViewcase uses non-optionaltryfor required fields, diverging from thetry?/if-letguard pattern used by every other message type. A malformedpage_viewpayload will throw aDecodingErrorrather thanPaywallMessageError.decoding, which callers may not expect.Limited public API surface for page context: The
paywallPageViewevent only carriespaywallInfo, so SDK consumers have no way to determine which page was navigated to, what the navigation type was, or access other page-specific data tracked internally. Compare this to similar rich events likesurveyResponseandtriggerFire, which embed their contextual data directly.Confidence Score: 3/5
presentationIdassignment is properly placed and analytics wiring is complete. However, these two issues represent technical debt and potential API gaps worth resolving.Sequence Diagram
sequenceDiagram participant Web as Paywall WebView participant MH as PaywallMessageHandler participant SW as Superwall.shared participant Delegate as SuperwallDelegate Web->>MH: page_view message<br/>(pageNodeId, pageIndex, pageName,<br/>navigationNodeId, type, ...) MH->>MH: decode into .pageView case<br/>(PaywallMessage.swift) MH->>MH: guard delegate, capture paywallInfo MH->>SW: Task { track(PaywallPageView event) } SW->>SW: getSuperwallParameters()<br/>adds page_node_id, page_index,<br/>page_name, navigation_type,<br/>presentation_id, etc. SW->>Delegate: handleSuperwallEvent(.paywallPageView(paywallInfo:)) Note over Web,Delegate: presentationId is assigned once in getRawPaywall()<br/>and flows through Paywall → PaywallInfo → placementParamsLast reviewed commit: 4149bce