2.8.4

[ianrumac]

Changes in this pull request

Fixes

• Fix bug where experiment ID would be stale among paywalls sharing experiments
• Fix trial reminder notifications being scheduled after a paywall purchase that did not start a free trial (for example a user who already used their trial and was charged immediately). Reminders are now only scheduled when a trial actually started, and the paywall receives the freeTrial_start message for every started trial, even when it has no trial reminders configured.

Checklist

• All unit tests pass.
• All UI tests pass.
• Demo project builds and runs.
• I added/updated tests or detailed why my change isn't tested.
• I added an entry to the CHANGELOG.md for any breaking changes, enhancements, or bug fixes.
• I have run ktlint in the main directory and fixed any issues.
• I have updated the SDK documentation as well as the online docs.
• I have reviewed the contributing guide

[pullfrog]

ℹ️ No blocking issues — two minor points inline plus one scope question.

Reviewed changes — the 2.8.4 release branch: two bug fixes, their tests, and the version/CHANGELOG/badge bump.

• Experiment bound at request time, not fetch time — MergePaywall no longer copies experiment/presentationSourceType off the freshly fetched Paywall; SetRequest now writes them (plus presentationId) atomically with the request, so two campaigns sharing a cached PaywallView stop overwriting each other's metadata at fetch time.
• experiment threaded through the presentation pipeline — new required parameter on PaywallComponents, presentPaywallView, presentPaywallViewSync, PaywallView.present and PaywallView.set, sourced from (outcome.triggerResult as? InternalTriggerResult.Paywall)?.experiment.
• Fresh presentationId per re-binding — SetRequest keeps the fetch-time id only for a view's first binding and mints a new UUID for every subsequent one.
• Trial reminders gated on an actual trial — new TrialReminderLogic.fallbackTrialNotifications returns nothing unless didStartFreeTrial; TransactionManager nulls trialEnd in the same case and passes the flag through notifyOfTransactionComplete.
• freeTrial_start decoupled from reminder config — the PaywallMessage.TrialStarted webview message moved out of the trialNotifications.isNotEmpty() block into its own if (didStartFreeTrial), so a paywall with no reminders configured still receives it.
• Tests — new PaywallManagerExperimentIsolationTest, three new PaywallViewStateTest cases, two new TransactionManagerTest cases, and a new TrialReminderLogicTest; existing call sites updated for the new parameters.
• Release chores — version.env 2.8.3 → 2.8.4, CHANGELOG entry, coverage badge.

I checked the paths that could have regressed from dropping experiment out of MergePaywall and found none: preloading skips the merge (isPreloading), the debugger preview builds its own view outside the cache, getPresentationResult never calls set(), and the webview experiment template is rebuilt in presentationWillBegin() which runs after present() → set(). The (outcome.triggerResult as? …)?.experiment cast can't be null on a real presentation because getExperiment() throws for every other trigger result before a view is produced.

ℹ️ The shared cached view still resolves last-writer-wins across concurrent consumers

The fix moves the metadata write from fetch time to bind time, which closes the reported window, but the cached PaywallView still has exactly one metadata slot shared by every campaign that uses the paywall. PaywallManagerExperimentIsolationTest encodes this as intended behavior: after B binds, the shared view reports B even though A bound first. If A is the presentation actually on screen, its paywall_close and transaction events will report B's experiment. Worth confirming that's the intended long-term contract rather than a second stage of the same fix.

Technical details

# Concurrent consumers of one cached PaywallView

## Affected sites
- `superwall/src/main/java/com/superwall/sdk/paywall/view/PaywallViewState.kt:126-149` — `SetRequest` writes `experiment` / `presentationSourceType` / `presentationId` into the single shared `state.paywall`.
- `superwall/src/main/java/com/superwall/sdk/paywall/manager/PaywallManager.kt:79-98` — the cache-hit branch hands the same instance to every non-preloading caller, including one whose view is currently presented.
- `superwall/src/test/java/com/superwall/sdk/paywall/manager/PaywallManagerExperimentIsolationTest.kt:237-242` — asserts the later binder wins.

## Open questions for the human
- Is "last binder wins on a shared view" the intended long-term contract, or is per-presentation isolation (a view instance or metadata slot per in-flight request) a follow-up?
- Should the embedded `getPaywall()` path refuse to re-bind a view whose `state.isPresented` is true, the way `register()` is protected by `checkNoPaywallAlreadyPresented`?

ℹ️ Nitpicks

• PaywallView.present, Superwall.presentPaywallView, Superwall.presentPaywallViewSync and the PaywallComponents constructor are all public and each gained a required parameter, which is source- and binary-incompatible for external callers on a patch version bump. No in-repo caller breaks, so this may well be an acceptable trade — just worth being a deliberate choice rather than a side effect.
• The headline CHANGELOG claim — "the paywall receives the freeTrial_start message for every started trial, even when it has no trial reminders configured" — lives in the DependencyContainer.notifyOfTransactionComplete lambda, which has no test. TrialReminderLogicTest covers only the extracted pure function, so the if (didStartFreeTrial) branch that actually sends PaywallMessage.TrialStarted is unverified.

  ^{｜ Fix all ➔ ｜ Fix 👍s ➔ ｜ View workflow run ｜ Using Claude Opus ｜ 𝕏}

[pullfrog]

Important

The new state.isPresented guard fixes the reported id churn but latches on for host-managed embeds, so distinct presentations of an embedded paywall now share one presentation_id.

Reviewed changes — the delta since the prior pullfrog review at 265ec9a5: one behavioral commit plus a CI badge refresh.

• Kept presentationId stable while a view is presented — SetRequest's predicate widened from state.request == null to state.request == null || state.isPresented, so a re-binding of a view that is already on screen no longer splits paywall_open from the paywall_close/transaction_* events that follow it. This is the fix for the prior review's inline thread, which I've resolved.
• New regression test — PaywallViewStateTest.setRequest_keepsPresentationIdWhileTheViewIsPresented walks fresh bind → cached re-bind → SetPresentedAndFinished → mid-presentation re-bind → CleanupAfterDestroy → re-bind, asserting the id is held across the live re-bind and minted afresh after teardown. It fails pre-fix, so it is real coverage.
• CHANGELOG entry for the new fix, and a coverage badge bump (branches 38.3 → 38.4%, jacoco 47.1 → 47.2%) from the [skip ci] automation.

I traced the isPresented lifecycle across the full-screen Activity teardown (finishing and non-finishing), the billing-sheet onStop window, the Compose wrapper and the raw getPaywall() embed path. Compose is safe — PaywallComposable's onRelease calls destroyed(forceCleanup = true). The full-screen path is safe — finishing onDestroy reaches CleanupAfterDestroy, and a resume early-returns in onViewCreated() without touching SetRequest. The raw embed path is not, which is the first comment below.

⚠️ Presentation boundaries are inferred from view state rather than known at the seam

Both inline findings come from the same root: SetRequest has to guess whether a re-binding is "the live presentation being repainted" or "the next presentation starting", and isPresented is a proxy that is wrong in both directions. InternalGetPaywall.getPaywall still has no checkNoPaywallAlreadyPresented equivalent (InternalPresentation.internallyPresent does), so the seam that creates the ambiguity is the one place that could resolve it definitively. The previous review raised rejecting a re-bind on a live view as the alternative to widening the predicate; this commit took the predicate route, which is worth confirming as the deliberate long-term contract rather than the cheaper of the two.

Technical details

# `getPaywall()` can rebind a live, shared paywall view and nothing at the seam says so

## Affected sites
- `superwall/src/main/java/com/superwall/sdk/paywall/presentation/get_paywall/InternalGetPaywall.kt:36-51` — calls `view.set(...)` on whatever `PaywallManager` returns, with no liveness check.
- `superwall/src/main/java/com/superwall/sdk/paywall/presentation/internal/InternalPresentation.kt:29` — the `register()` path *is* guarded by `checkNoPaywallAlreadyPresented`.
- `superwall/src/main/java/com/superwall/sdk/paywall/manager/PaywallManager.kt:78-98` — the cache-hit branch hands the same instance to every caller and knows a new presentation is starting, but only resets transient spinner/prepare state.
- `superwall/src/main/java/com/superwall/sdk/paywall/view/PaywallViewState.kt:139-141` — the inference site.

## Open questions for the human
- Is "a `getPaywall()` call may repaint a paywall that is currently on screen" a supported pattern, or should that seam reject the rebind the way `register()` does?
- If it stays supported, should the decision move to `PaywallManager`'s cache-hit branch (which already distinguishes a new presentation from a resume) instead of being re-derived from `isPresented` inside `SetRequest`?

  ^{｜ Fix all ➔ ｜ Fix 👍s ➔ ｜ View workflow run ｜ Using Claude Opus ｜ 𝕏}

[pullfrog]

ℹ️ No blocking issues — the prior review's latched-isPresented finding is properly fixed; two minor points inline.

Reviewed changes — the delta since the prior pullfrog review at 77c13c4f: one behavioral commit that rewrites how presentation_id rotates.

• Moved id ownership off SetRequest onto a presentation boundary — the state.request == null || state.isPresented predicate is gone; SetRequest no longer touches presentationId at all. A new Updates.BeginPresentation mints one iff the current id has already been consumed by a paywall_open or was never set.
• Added presentationIdOpened to PaywallViewState — written only by SetLastOpen, deliberately surviving CleanupAfterDestroy and ResetPresentationPreparations so a consumed id stays consumed across a teardown.
• Wired two mint sites in PaywallView — presentationWillBegin() (guarded by !state.isPresented) and onViewCreated() (guarded by the newly hoisted isNewPresentation = !state.presentationDidFinishPrepare, computed before the completion so PaywallState.Presented(info) carries the new id). BeginPresentation is idempotent, so the second call is a no-op.
• Rewrote the id tests — PaywallViewStateTest now has presentationId_isKeptFromFetchThroughTheFirstOpen, presentationId_isMintedPerPresentationOfACachedView (including an "embedded view re-presented without ever being torn down" sub-case) and presentationId_isStableWhileAPresentationIsLive; PaywallViewTest gains onViewCreated_mintsPresentationIdOnlyForANewPresentation, which drives the real view through the cache-hit reset.
• CHANGELOG entry for the id fix expanded to state the rotation semantics.

This does resolve the prior review's thread: keying on presentationIdOpened rather than isPresented means a host-managed embed that never calls destroyed(forceCleanup = true) now gets a distinct id per presentation, and the new PaywallViewStateTest sub-case pins exactly that case. I re-traced the full-screen Activity lifecycle (finishing teardown, non-finishing background stop, plain resume) and the Compose wrapper against the new predicate and found them consistent — a backgrounded live paywall keeps presentationDidFinishPrepare = true, so presentationWillBegin() early-returns and onViewCreated() takes the resume branch without minting.

The PaywallViewState.kt:145-147 thread from the earlier review (one presentation_id reporting two experiments when a live view is re-bound) is untouched by this commit, so I've left it open.

  ^{｜ Fix all ➔ ｜ Fix 👍s ➔ ｜ View workflow run ｜ Using Claude Opus ｜ 𝕏}

[pullfrog]

Important

The single mint predicate and the will/did/open id fix look right, but the new cache-hit guard has a second effect: a cached view that is on screen for one presentation now hands itself back to a different presentation request with stale prepare flags and a stale purchase spinner.

Reviewed changes — the delta since the prior pullfrog review at 6947840d: one behavioral commit that collapses the two presentation_id mint sites onto a single predicate and moves the "don't disturb a live presentation" decision from presentationWillBegin() into the cache-hit reset.

• Collapsed the mint onto one predicate — the !state.isPresented guard is gone from presentationWillBegin(), so the outer :418 guard (!presentationWillPrepare || presentationDidFinishPrepare) is the only gate and willPresentPaywall, didPresentPaywall and the following paywall_open all report the same id.
• Made the cache-hit reset refuse a live, attached view — resetTransientPresentationState() early-returns when state.isPresented && isAttachedToWindow, so the mint site can only fire on a view whose prepare flags were deliberately reset.
• Documented the trade at the seam — the PaywallManager cache-hit comment now states that the view itself refuses the reset while presented and attached.
• Stabilised the racy precondition — onViewCreated_mintsPresentationIdOnlyForANewPresentation awaits the first paywall_open via waitUntil and asserts presentationIdOpened before relying on it.
• Added two PaywallViewTest cases — rePresentingADetachedCachedView_reportsOneNewPresentationIdToWillDidAndOpen drives beforeViewCreated() and pins will/did/open to one new id; reBindingALivePresentedView_doesNotStartANewPresentation pins the new guard using a Robolectric-attached view.

Both prior review threads are properly addressed and I've resolved them. I re-traced the mint predicate against every writer of presentationDidFinishPrepare: ResetPresentationPreparations is the only one that clears it, reachable only from the cache-hit reset or from destroyed(forceCleanup = true) (which also clears isPresented and detaches the view via cleanup()), so the unconditional mint in presentationWillBegin() is sound for the full-screen lifecycle, the billing-sheet/background stop and plain resume. register() → register() also cannot reach the new guard: checkNoPaywallAlreadyPresented blocks it while activePaywallVcKey is set, and that key is cleared in the same non-suspending block as CleanupAfterDestroy. The embed seam is the gap, and it's the one comment below.

  ^{｜ Fix all ➔ ｜ Fix 👍s ➔ ｜ View workflow run ｜ Using Claude Opus ｜ 𝕏}

[pullfrog]

ℹ️ No blocking issues — the prior review's thread is properly resolved; one minor point inline and one scope question below.

Reviewed changes — the delta since the prior pullfrog review at 28349b95: one behavioral commit that reverses the "leave a live presented view alone" decision, plus a CI badge refresh.

• Made the cache-hit reset unconditional again — the state.isPresented && isAttachedToWindow early return is gone from resetTransientPresentationState(), so a cached view handed back for a new presentation always gets its spinner and prepare flags cleared, even while it is still on screen.
• Adopted takeover semantics for getPaywall() on a presented paywall — the new comments in PaywallManager.getPaywallView and resetTransientPresentationState() state the contract: such a call takes the view over (prepareToDisplay() detaches it from its current parent), so it is a new presentation with a new id and a fresh willPresentPaywall/didPresentPaywall/paywall_open triple.
• Rewrote the guard's test into its inverse — reBindingALivePresentedView_doesNotStartANewPresentation became handingALiveAttachedViewToANewPresentation_resetsItAndMintsANewId, which puts a Robolectric-attached, presented view into LoadingPurchase and pins the spinner reset, the cleared prepare flag, and one new id reported to willPresentPaywall, didPresentPaywall and the second paywall_open.

I verified the load-bearing claim in the new comment. register() genuinely cannot reach the cache-hit branch while a paywall is on screen: internallyPresent runs checkNoPaywallAlreadyPresented first, and Superwall.isPaywallPresented resolves to cache.activePaywallVcKey != null, which prepareToDisplay() sets for embedded views too. Every public getPaywall entry point (PublicGetPaywall.kt:39 and :69, which PaywallBuilder and PaywallComposable both go through) does call prepareToDisplay(), so "takes the view over" is accurate rather than aspirational. PaywallPreload still passes isPreloading = true and GetPaywallVC still maps the result-only request types to isForPresentation = false, so neither reaches the reset. The new test fails against 28349b95 — its first Then asserts exactly the two things the removed guard used to skip — so it is real coverage, and it also replaces the previous test's Thread.sleep(300) with latch waits.

ℹ️ The superseded presentation ends with no paywall_close, and that is not in the CHANGELOG

The takeover contract means a getPaywall() for a paywall that is currently on screen silently terminates the presentation that was there: no paywall_close, no didDismissPaywall, and the full-screen host is left alive with an empty container (SuperwallPaywallActivity.paywallView() resolves via findViewWithTag, which returns null once the view is detached, so its onStop/onDestroy teardown no-ops). Downstream funnel analytics that pair paywall_open with paywall_close will now see an unmatched open where before this commit they saw no second open at all. The CHANGELOG bullet describes the id rotation but not this consequence.

Technical details

# Takeover semantics drop the superseded presentation's close event

## Affected sites
- `superwall/src/main/java/com/superwall/sdk/paywall/view/PaywallView.kt:538-539` — the comment states "The presentation being taken over ends here without a `paywall_close` of its own" as a settled trade, with no note of the analytics consequence.
- `superwall/src/main/java/com/superwall/sdk/paywall/view/SuperwallPaywallActivity.kt:211` — `paywallView()` is `contentView?.findViewWithTag(ACTIVE_PAYWALL_TAG)`, so after `prepareToDisplay()` detaches the view, `onStop`/`onDestroy` (`:813-821`, `:831-864`) no-op and the orphaned Activity never finishes itself.
- `superwall-compose/src/main/java/com/superwall/sdk/compose/PaywallComposable.kt:108-115` — the opposite failure for embeds: the *original* host's `onRelease` still runs `destroyed(forceCleanup = true)` + `cleanup()` on the now-shared view, so the close, the `PaywallState.Dismissed` emission and the teardown all land on the *successor* presentation.
- `CHANGELOG.md:9` — describes id rotation only.

## Required outcome
- The new behavior for "`getPaywall()` for a paywall that is already on screen" is a deliberate, documented product decision rather than a side effect of resolving the previous review thread.
- Customers reading the 2.8.4 CHANGELOG can tell that a takeover produces an extra `paywall_open` with no matching `paywall_close` for the presentation it replaced.

## Open questions for the human
- Should the takeover dismiss the superseded presentation properly (track `paywall_close`, fire `didDismissPaywall`, finish the orphaned `SuperwallPaywallActivity`) rather than dropping it, now that this path is endorsed instead of avoided?
- Is the Compose case — host A's `onRelease` tearing down host B's live presentation on the shared view — in scope for this release, or a separate follow-up? It is pre-existing, but the takeover contract makes overlapping host lifetimes a supported pattern rather than an accident.

  ^{｜ Fix all ➔ ｜ Fix 👍s ➔ ｜ View workflow run ｜ Using Claude Opus ｜ 𝕏}