feat(debug): preview paywalls in a specific state via deep link - #499
feat(debug): preview paywalls in a specific state via deep link#499konroj wants to merge 1 commit into
Conversation
|
PR author is not in the allowed authors list. |
There was a problem hiding this comment.
Important
attr_* turns an unauthenticated deep link into a disk-persisted, backend-tracked write on the real user's profile, and the generated Xcode project picked up unrelated toolchain churn that CI will discard. Both are worth a decision before merge.
Reviewed changes — full initial review of 0705e711, all 12 files.
DebugPaywallOverridesvalue type — parsestrial_state,appearance,locale,presentandattr_<key>query items off the debug deep link, tolerant of unknown values.SWDebugManagerLogic.QueryItemName— four new cases backing those params.- Override threading —
DebugManager.DeepLinkOutcomecarriesoverridesthroughlaunchDebugger→presentDebugger→ViewControllerFactory.makeDebugViewController(protocol signature change). DebugViewControllerapply/restore —viewDidLoadsnapshots locale + interface style then applies overrides;viewDidDisappearrestores all three;viewDidAppearandfinishLoadingPreviewboth drive a one-shotpresentAutomaticallyIfNeeded().- Preview honours
trial_state— the thumbnail request now passesPaywallRequest.Overrides(isFreeTrial:). - Tests — exhaustive parsing coverage in
DebugPaywallOverridesTests/DebugManagerTestswith exact-value assertions, plus fourgetQueryItemValuecases and a call-site fix inCheckDebuggerPresentationOperatorTests. - Generated project —
project.pbxprojand the shared scheme were regenerated with a newer toolchain.
⚠️ The Xcode project and scheme carry toolchain churn that CI will throw away
project.pbxproj jumps objectVersion 54 → 77 and gains preferredProjectObjectVersion / minimizedProjectReferenceProxies, and the shared scheme gains parallelizable = "NO". Both files are regenerated by xcodegen in scripts/build.sh, scripts/test.sh and every CI job, and project.yml does not declare parallelizable — so the scheme change is not reproducible and will vanish on the next regeneration, while the format bump raises the Xcode version needed to open the checked-in project for anyone who doesn't regenerate.
Technical details
# Generated Xcode project churn is unrelated to the feature and non-durable
## Affected sites
- `SuperwallKit.xcodeproj/project.pbxproj:6` — `objectVersion = 77` (was 54), plus `preferredProjectObjectVersion = 77`, `minimizedProjectReferenceProxies = 1`, removal of `compatibilityVersion = "Xcode 14.0"`, and reordering of the `Core Data` group children. All generator-version artifacts, not feature changes.
- `SuperwallKit.xcodeproj/xcshareddata/xcschemes/SuperwallKit.xcscheme:44` — `parallelizable = "NO"` on the `SuperwallKitTests` testable reference.
- `project.yml:11-15` — declares the `SuperwallKit` scheme with `testTargets: [SuperwallKitTests]` and no `parallelizable` key.
- `.github/workflows/tests.yml:32-33` — CI runs `xavierLowmiller/xcodegen-action@1.2.3` before `xcodebuild`, so the committed project is regenerated on every run.
## Required outcome
- The pbxproj/scheme diff should contain only what a `xcodegen` run at the team's pinned version produces for the two new source files, with no format-version bump riding along.
- If serial test execution is genuinely needed, it has to be expressed in `project.yml` so it survives regeneration; if it isn't needed, drop it.
## Open questions for the human
- Was `parallelizable = "NO"` deliberate (e.g. working around the known shared-`Superwall.shared` flakiness) or an incidental IDE/generator artifact? The new tests here are pure value-type parsing and don't touch global state.
- Is the project team ready to require the Xcode version that `objectVersion = 77` implies, or should the generator be pinned back?ℹ️ Nothing exercises the apply/restore of global SDK state
The parsing layer is covered thoroughly and the assertions are exact, but the behaviour that carries the risk — applyOverrides() mutating Superwall.shared and viewDidDisappear putting it back — has no test at all. A test that sets a pre-existing user attribute, applies an override for that key plus a key that did not exist, and asserts both are exactly restored (the second one removed, not left set) would pin the part most likely to regress, and would confirm the restoreAttributes.updateValue(current[key], …) explicit-nil trick behaves as intended end-to-end.
ℹ️ Nitpicks
presentacceptstrue/1/yes, whilesuperwall_debugon the same link is parsed with strictBool(_:)— two truthiness vocabularies in one URL. Worth aligning, or documenting the looser set as deliberate (it is convenient for Appium/XCUITest, which is presumably why it exists).- Per
CLAUDE.md, customer-facing changes get aCHANGELOG.mdentry. Five new documented deep-link parameters read as customer-facing even though the surface is debug-only; consider adding the entry rather than deferring it.
Claude Opus | 𝕏
| return | ||
| } | ||
| viewController.paywallDatabaseId = paywallDatabaseId | ||
| viewController.overrides = overrides |
There was a problem hiding this comment.
Assigning overrides here only half-works: applyOverrides() is viewDidLoad-gated, so on this reuse branch locale, appearance and attr_* are silently dropped while trial_state and present are still honoured via loadPreview() → presentAutomaticallyIfNeeded(). Today launchDebugger always nils the view controller through closeDebugger first so the branch is effectively unreachable, but the assignment implies otherwise — worth either applying the overrides here too or dropping the line.
0705e71 to
60a10b9
Compare
There was a problem hiding this comment.
Important
The new in-memory-only attribute path doesn't hold its guarantee. _userAttributes is the merge base for every persisting write in IdentityManager, so any ordinary setUserAttributes / attribution callback / identify() that fires while the debugger is open bakes the deep-link attr_* values into on-disk UserAttributes and reports them in the user_attributes event.
Reviewed changes — the delta since the prior pullfrog review of 0705e711; the branch was force-pushed to a single commit, 60a10b9f, touching DebugViewController and IdentityManager only.
- Moved
attr_*off the persisting path —applyOverrides()no longer touches user attributes; a newapplyAttributeOverridesIfNeeded()routes them through two new internalIdentityManagermethods that skipstorage.saveand theuser_attributestrack. - Deferred attribute application until the preview resolves — the call now sits in
finishLoadingPreview()afterpaywallRequestManager.getPaywall(...)succeeds, so an unusable debug token no longer applies anything. - Reworked the restore —
restoreAttributes: [String: Any?]replaced by a whole-dictionaryattributesSnapshot: [String: Any]?, put back onviewDidDisappearviareplaceAttributesInMemoryOnly(_:).
ℹ️ Nothing pins the in-memory-only guarantee
mergeAttributesInMemoryOnly / replaceAttributesInMemoryOnly are the load-bearing part of this revision and have no test at all, while the parsing layer that carries no risk is covered exhaustively. Tests/SuperwallKitTests/Identity/IdentityManagerTests.swift already builds a real IdentityManager over dependencyContainer.storage, so asserting the guarantee directly is a few lines — and the interesting assertion fails today.
Technical details
# The in-memory-only attribute lifecycle is untested
## Affected sites
- `Sources/SuperwallKit/Identity/IdentityManager.swift:437-448` — `mergeAttributesInMemoryOnly(_:)`, no test.
- `Sources/SuperwallKit/Identity/IdentityManager.swift:452-456` — `replaceAttributesInMemoryOnly(_:)`, no test.
- `Tests/SuperwallKitTests/Identity/IdentityManagerTests.swift` — existing suite; constructs a real `IdentityManager` with `dependencyContainer.storage` / `.deviceHelper` / `.configManager`, so no new fixture is needed.
## Required outcome
- A test asserts that `mergeAttributesInMemoryOnly` leaves `storage.get(UserAttributes.self)` byte-for-byte unchanged while `userAttributes` reflects the new value.
- A test asserts that a subsequent ordinary write (`mergeUserAttributes([...])`) does NOT carry the in-memory-only value into the dictionary that gets saved. This is the assertion that fails against the current implementation and the one worth having.
- A test asserts `replaceAttributesInMemoryOnly` restores exactly the snapshot, including removing a key that the merge introduced.
## Suggested approach (optional)
- Match the existing style in that file: build the manager, call the method, drain with `await MainActor.run {}` or a short `Task.sleep` as the neighbouring tests do, then read back through `storage.get(UserAttributes.self)`.Claude Opus | 𝕏
9af9d58 to
5c6226a
Compare
There was a problem hiding this comment.
ℹ️ No new issues in the added tests themselves — one note on what they claim to pin.
Reviewed changes — the delta since the prior pullfrog review of 60a10b9f. The branch was force-pushed to a single commit, 9af9d58e, whose only change against the previously reviewed tree is test-only: three tests appended to IdentityManagerTests.swift. No source file changed, so the open threads on DebugViewController and IdentityManager are untouched by this revision.
- Pinned the in-memory merge —
mergeAttributesInMemoryOnly_appliesInMemoryButNeverPersistsasserts the override is visible throughuserAttributesand absent fromstorage.get(UserAttributes.self). - Pinned the no-notify contract —
mergeAttributesInMemoryOnly_doesNotNotifyDelegateassertsnotifyUserChangeis never invoked for the in-memory path. - Pinned the restore —
replaceAttributesInMemoryOnly_restoresSnapshotWithoutPersistingasserts the snapshot restore removes the key the merge introduced, with storage untouched.
All three are genuinely falsifiable: Cache.write populates memCache synchronously before the async disk write and Cache.read consults memCache first, so a persisting implementation would be visible to the immediate storage.get. They also can't be perturbed by parallel execution over the process-wide Cache paths — they write nothing to disk and assert only on the suite-unique debugOnlyKey.
ℹ️ Nitpicks
- The
Task.sleepwaits atIdentityManagerTests.swift:109,:155and:160aren't load-bearing — each is followed by a read through theuserAttributesgetter, which isqueue.syncon the same serial queue the writes werequeue.async'd onto, so the ordering is already guaranteed. Only:135indoesNotNotifyDelegateneeds a drain, and_ = identityManager.userAttributeswould do it deterministically instead of costing 300 ms. It matches the file's existing style, so entirely optional.
Claude Opus | 𝕏
There was a problem hiding this comment.
ℹ️ The generated-project churn is gone — one consequence of how it was removed is worth a look.
Reviewed changes — the delta since my review of 9af9d58e. The branch was force-pushed to 5c6226a7, whose only change is to revert the two generated Xcode files; no source or test file differs from the previously reviewed tree.
- Dropped the generated-project churn —
project.pbxprojis back toobjectVersion = 54withpreferredProjectObjectVersion,minimizedProjectReferenceProxies,productRefGroupand theCore Datagroup reordering gone, and the hand-addedparallelizable = "NO"is gone from the shared scheme.
ℹ️ The committed Xcode project no longer knows about the three new files
Both generated files now match develop exactly, which means project.pbxproj carries no reference to DebugPaywallOverrides.swift, DebugPaywallOverridesTests.swift or DebugManagerTests.swift. Opening the checked-in project and building without first running xcodegen therefore fails, because DebugManager.swift references DebugPaywallOverrides. CI is unaffected, but this is the opposite corner from the churn: the ask was a regeneration at the team's pinned generator version, not omitting the file.
Technical details
# The reverted `project.pbxproj` omits the PR's new files
## Affected sites
- `SuperwallKit.xcodeproj/project.pbxproj` — identical to `develop`; zero occurrences of `DebugPaywallOverrides`. The SDK target's `Sources` phase does not compile `Sources/SuperwallKit/Debug/DebugPaywallOverrides.swift`, and the test target does not compile `DebugPaywallOverridesTests.swift` / `DebugManagerTests.swift`.
- `Sources/SuperwallKit/Debug/DebugManager.swift` — references `DebugPaywallOverrides` in `DeepLinkOutcome`, `launchDebugger` and `presentDebugger`, so a stale project fails to compile rather than silently skipping the feature.
- `scripts/pre-commit:3-4` — runs `xcodegen` then `git add SuperwallKit.xcodeproj`, and every recent commit on `develop` carries the regenerated file, so keeping the committed project in sync is the house convention.
- `.github/workflows/tests.yml:32-33`, `build-platforms.yml:27-28` — regenerate via `xavierLowmiller/xcodegen-action@1.2.3` before building, which is why CI stays green either way.
## Required outcome
- The committed project registers the three new files, with no generator-format churn riding along (`objectVersion` stays 54, no `preferredProjectObjectVersion` / `minimizedProjectReferenceProxies` / group reordering).
## Suggested approach (optional)
- Regenerate with the `xcodegen` version that `xavierLowmiller/xcodegen-action@1.2.3` installs (the one that produced the current `objectVersion = 54` file) rather than a locally newer toolchain; the resulting diff should be the handful of `PBXBuildFile` / `PBXFileReference` / group / `Sources` entries for the three files.
## Open questions for the human
- If the team's position is that the committed project is a pure build artifact and may lag, leaving it untouched is fine — but then the pre-commit hook regenerating it on the next commit will reintroduce whatever the contributor's local generator emits. Worth deciding which way this PR should land.Claude Opus | 𝕏
|
Thanks for the review — both points addressed (force-pushed).
Reworked so attribute overrides never reach durable state or analytics, and only apply once the token is validated:
Added three Generated-project churn Dropped the Full unit suite passes (925). |
Add debug deep link parameters so QA can open the paywall preview in a chosen state without changing device settings: trial_state (eligible/ineligible), appearance (light/dark/system), locale, attr_<key>=<value> to override user attributes, and present to open the paywall directly. The overrides apply only while the debugger is open and are reverted on exit.
5c6226a to
96a0abd
Compare
|
Update — I've pulled The in-memory approach I pushed earlier didn't actually hold: So this PR now covers Also addressed the two lifecycle notes from the last review: the in-flight preview load is now cancelled on dismiss, and the interface-style restore only runs when |
There was a problem hiding this comment.
Important
previewTask?.cancel() doesn't cancel anything on this path, so the dismiss-during-load hole it was added to close is still open — and with present=true it can now fire a paywall presentation through an already-dismissed debugger.
Reviewed changes — the delta since my review of 5c6226a7. The branch was force-pushed to a single commit, 96a0abdd, which is a scope reduction plus two fixes; the PR is down to 10 files and IdentityManager.swift / IdentityManagerTests.swift are no longer touched.
- Removed
attr_*entirely —DebugPaywallOverrides.attributes/attributePrefix/parseAttributes,DebugViewController.applyAttributeOverridesIfNeeded/attributesSnapshot, bothIdentityManagerin-memory-only methods and the threeIdentityManagerTestscases are gone, with no dangling references left. This retires the persistence, tracking and whole-dict-clobber concerns from the last three reviews. - Gated the interface-style restore —
viewDidDisappearnow only callsSuperwall.shared.setInterfaceStyle(to: initialInterfaceStyleOverride)whenoverrides.appearance != nil, so closing the debugger no longer emits a spuriousDeviceAttributesevent for sessions with no appearance override. - Tracked the preview load in a cancellable
Task—viewDidLoadstorespreviewTask = Task { await loadPreview() }andviewDidDisappearcallspreviewTask?.cancel()before the rest of the teardown.
Claude Opus | 𝕏
|
|
||
| override func viewDidDisappear(_ animated: Bool) { | ||
| super.viewDidDisappear(animated) | ||
| previewTask?.cancel() |
There was a problem hiding this comment.
cancel() only sets a flag: nothing in loadPreview() / finishLoadingPreview() checks Task.isCancelled, and both network hops suspend on an unstructured Task's .value — CustomURLSession.swift:93-100 (Task.retrying(...).value) and PaywallRequestManager.swift:78/:102 (activeTasks → await task.value) — which is immune to the caller's cancellation.
So exiting mid-load still runs the whole tail: addPaywallPreview() attaches a fresh PaywallViewController + WKWebView as a child of the dismissed controller after resetCache(), and with present=true presentAutomaticallyIfNeeded() still passes its guard (didAppear is never reset) and calls loadAndShowPaywall(presenter: self) on a detached presenter. The PR description's "the in-flight preview load is cancelled on dismiss" doesn't hold as written.
Technical details
# `previewTask?.cancel()` cannot interrupt the preview load
## Affected sites
- `Sources/SuperwallKit/Debug/DebugViewController.swift:587` — `previewTask?.cancel()`. Sets the flag and nothing observes it.
- `Sources/SuperwallKit/Debug/DebugViewController.swift:240-299` — `loadPreview()` / `finishLoadingPreview()`. Zero `Task.isCancelled` / `try Task.checkCancellation()` calls, including after the three awaits (`network.resolvePaywallIdentifier` at `:255`, `paywallRequestManager.getPaywall` at `:279`, `storeKitManager.getProductVariables` at `:281`).
- `Sources/SuperwallKit/Network/Custom URL Session/CustomURLSession.swift:93-100` — `try await Task.retrying(...).value`, where `Task+Retrying.swift:24` builds a detached `Task(priority:) { }`. Per the stdlib `Task.cancel()` contract, cancellation reaches only *structured* children, and `await task.value` does not throw on the awaiting task's own cancellation.
- `Sources/SuperwallKit/Paywall/Request/PaywallRequestManager.swift:78`, `:102` — same shape: the fetch lives in an unstructured `Task` stored in `activeTasks` and coalesced across callers, awaited via `.value`.
- `Sources/SuperwallKit/Debug/DebugViewController.swift:287-289` — `addPaywallPreview()` then `presentAutomaticallyIfNeeded()`, both reached unconditionally. The file contains no `removeFromParent()` anywhere, so the child controller added here is retained by the dismissed `DebugViewController` for its lifetime.
- `Sources/SuperwallKit/Debug/DebugViewController.swift:172-176`, `:187-195` — `didAppear` is set in `viewDidAppear` and never reset in `viewDidDisappear` (`:585-593`), so the `present=true` latch survives dismissal.
- `Sources/SuperwallKit/Paywall/Presentation/Internal/Operators/CheckDebuggerPresentation.swift:24-29` — guards on `request.presenter is DebugViewController` only, never on whether that instance is still attached, so the stale presentation is allowed through.
- `Sources/SuperwallKit/Debug/DebugViewController.swift:393`, `:437` — the picker and localization-picker reloads spawn `Task { await self?.loadPreview() }` without assigning `previewTask`, so even a working cancel would miss them.
## Required outcome
- Dismissing the debugger must prevent `addPaywallPreview()` and `presentAutomaticallyIfNeeded()` from running for a load that was in flight at dismissal, for every path that starts a preview load — not just the `viewDidLoad` one.
- Whatever the mechanism, `loadAndShowPaywall` must not be reachable with `self` detached from the window hierarchy.
## Suggested approach (optional)
- Since the network layer is deliberately unstructured, the cheapest honest fix is an explicit checkpoint rather than relying on task cancellation: guard the mutation tail in `finishLoadingPreview()` on `!Task.isCancelled` (or on `viewIfLoaded?.window != nil`), and add the same condition to `presentAutomaticallyIfNeeded()`'s guard alongside `didAppear`.
- Alternatively reset `didAppear = false` in `viewDidDisappear` — that alone closes the `present=true` half, though it leaves the orphaned child controller from `addPaywallPreview()`.
- If the intent is only to stop the auto-present and not the fetch, dropping `previewTask` and the `cancel()` in favour of the window/`didAppear` check would be less misleading than a cancel that has no effect.
## Open questions for the human
- Is `present=true` expected to be usable in an automated harness that can tear the debugger down mid-load (Appium/XCUITest), or is dismissal-during-load considered out of scope for the QA flow?
Summary
Adds debug deep link parameters so a paywall can be previewed in a specific state directly from the debugger, without changing device settings or App Store account — useful for manual QA and for automating real-device paywall testing (Appium/XCUITest). All parameters are optional, take effect only while the debugger is open, and are reverted on exit. They extend the existing
superwall_debug+tokendebug link; authorization is unchanged.Parameters
trial_state=eligible|ineligible— override free-trial eligibilityappearance=light|dark|system— override interface stylelocale=<code>— override paywall locale (e.g.de)present=true— open the paywall directly instead of stopping at the previewCombinable, e.g.
myapp://?superwall_debug=true&token=TOKEN&paywall_id=ID&trial_state=ineligible&appearance=dark&locale=de&present=trueImplementation
DebugPaywallOverridesvalue type parses the params (tolerant — unknown values ignored).DebugManager/DebugViewControllerapply overrides on launch and restore prior global state (locale, interface style) on dismiss. The in-flight preview load is cancelled on dismiss, and the interface-style restore only runs whenappearancewas overridden (so closing the debugger doesn't emit a stray device-attributes event).presentauto-opens once, after the debugger appears, using the resolved trial state; the preview thumbnail honorstrial_statetoo.Testing
DebugPaywallOverridesTests,DebugManagerTests, extendedSWDebugManagerLogicTests); full suite passes.Notes
attr_*) were intentionally left out of this PR. Applying them through the identity layer risked persisting deep-link values to disk via the shared merge base; doing it safely means injecting them only into the paywall's template render, which is a separate change and will follow as its own PR.device.interfaceStyletheming; paywalls without a dark design or extra localizations have nothing to switch to.handleDeepLink(_:)is unchanged and already listed under In-App Previews. The developer-facing write-up for these QA parameters belongs on the online docs (docs.superwall.com → In-App Previews) and is prepared separately.Checklist
CHANGELOG.md(intentionally omitted — see Notes).swiftlintin the main directory and fixed any issues.🤖 Generated with Claude Code