Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| objcDelegate: nil | ||
| ) | ||
|
|
||
| let product = StoreProduct( |
There was a problem hiding this comment.
The 100 ms sleep is used to wait for the async dismissView() delegate call to complete. In slow CI environments or under heavy load this window can be missed, causing the assertion to see 0 results and fail spuriously. Consider structuring the test so the delegate call is awaited directly via a withCheckedContinuation or an AsyncStream-based expectation instead of a fixed sleep.
The same concern applies to the restore test at line 207.
Prompt To Fix With AI
This is a comment left during a code review.
Path: Tests/SuperwallKitTests/Paywall/View Controller/PaywallViewControllerTests.swift
Line: 177-180
Comment:
**Time-based async assertion is fragile**
The 100 ms sleep is used to wait for the async `dismissView()` delegate call to complete. In slow CI environments or under heavy load this window can be missed, causing the assertion to see 0 results and fail spuriously. Consider structuring the test so the delegate call is awaited directly via a `withCheckedContinuation` or an `AsyncStream`-based expectation instead of a fixed sleep.
The same concern applies to the restore test at line 207.
How can I resolve this? If you propose a fix, please make it concise.| nonisolated(unsafe) var results: [PaywallResult] = [] | ||
| func paywall( | ||
| _ paywall: PaywallViewController, |
There was a problem hiding this comment.
nonisolated(unsafe) bypasses Swift concurrency checks
nonisolated(unsafe) var results disables actor-isolation enforcement on RecordingDelegate. If the delegate is ever called off the main actor the mutation would be a data race. Since the enclosing test suite is @MainActor and PaywallViewController also runs on the main actor the access is safe in practice, but marking RecordingDelegate itself @MainActor would be cleaner and restore compiler-enforced isolation.
Prompt To Fix With AI
This is a comment left during a code review.
Path: Tests/SuperwallKitTests/Paywall/View Controller/PaywallViewControllerTests.swift
Line: 134-136
Comment:
**`nonisolated(unsafe)` bypasses Swift concurrency checks**
`nonisolated(unsafe) var results` disables actor-isolation enforcement on `RecordingDelegate`. If the delegate is ever called off the main actor the mutation would be a data race. Since the enclosing test suite is `@MainActor` and `PaywallViewController` also runs on the main actor the access is safe in practice, but marking `RecordingDelegate` itself `@MainActor` would be cleaner and restore compiler-enforced isolation.
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
Adds an idempotency guard to
PaywallViewController.dismiss(result:closeReason:completion:)that short-circuits any subsequent dismiss call whenpaywallResultis already.purchasedor.restored, preventing a system-close event from overwriting a terminal purchase/restore state. Two new@MainActortests cover both the purchase and restore scenarios; the test file is also renamed fromPaywallViewControllerDrawerTeststoPaywallViewControllerTests.Confidence Score: 5/5
Safe to merge — the guard is logically correct and all remaining findings are P2 style suggestions.
The fix synchronously guards on the already-set
paywallResultproperty before any async work begins. SincePaywallViewControlleris@MainActor, reads and writes ofpaywallResultare serialized, making the guard race-free by construction. All open findings are P2.Tests/SuperwallKitTests/Paywall/View Controller/PaywallViewControllerTests.swift — minor test quality concerns (sleep-based assertions, nonisolated(unsafe)).
Important Files Changed
dismiss(result:closeReason:completion:)that ignores any dismiss call whenpaywallResultis already.purchasedor.restored, fixing the post-purchase race condition.@MainActortests verifying idempotency; minor concerns around time-based async assertions andnonisolated(unsafe)usage.Sequence Diagram
sequenceDiagram participant TM as TransactionManager participant PVC as PaywallViewController participant Sys as System (viewDidDisappear) participant Del as Delegate TM->>PVC: dismiss(result: .purchased, closeReason: .systemLogic) Note over PVC: paywallResult = nil → guard passes<br/>paywallResult set to .purchased PVC->>Del: didFinish(result: .purchased, shouldDismiss: true) Note over PVC: UIKit dismiss animation starts Sys->>PVC: dismiss(result: .declined, closeReason: .manualClose) Note over PVC: paywallResult == .purchased → guard triggers<br/>completion?() called, return early PVC-->>Sys: (completion called, no delegate call)Prompt To Fix All With AI
Reviews (1): Last reviewed commit: "Fix dismiss post purchase race" | Re-trigger Greptile