Skip to content

fix(replay): drop masks for a screen hidden behind a full-screen cover - #812

Merged
marandaneto merged 13 commits into
mainfrom
posthog-self-driving/fixreplay-ios-ignore-masking-reporters-fcc05a
Sep 15, 2026
Merged

marandaneto merged 13 commits into
mainfrom
posthog-self-driving/fixreplay-ios-ignore-masking-reporters-fcc05a

Conversation

@posthog

@posthog posthog Bot commented Sep 10, 2026

Copy link
Copy Markdown
Contributor

💡 Motivation and Context

  • A postHogMask() region kept being redacted after its screen went behind a SwiftUI fullScreenCover, so replay viewers saw grey boxes painted over unrelated content on the covering screen — reported against 3.72.0 on iOS 26 with screenshot mode.
  • Root cause: a mask reporter registers while attached to a window and reports its rect live. PostHogSessionReplayMaskRegistry.maskedRects(in:) filtered on the window plus the reporter's own isHidden/alpha only. A covered screen is neither hidden on the reporter itself nor detached on newer OS versions, where UIKit keeps the presenter's view in the window.
  • The heuristic walk shares the root cause: it descends from the window, so a covered UITextField could leak a stale rect the same way.

Changes

Both rect sources are now pruned by one rule, computed once per frame in collectMaskedRegions:

Condition Effect
Reporter must reach the screen No hidden or fully transparent view anywhere between it and the window — one walk up the ancestry, replacing the reporter-only check
Frontmost full-window cover When a presented view holds the whole window opaquely, only its subtree is on screen: reporters outside it are skipped and the heuristic walk starts at the cover instead of the window

The cover test fails closed — it keeps everything behind the presentation masked unless every condition holds:

opaque:    .fullScreen style, or an opaque background colour
           (UIView.isOpaque is a drawing hint, true even on a see-through view)
covering:  presentation-tree rect contains the whole window
topmost:   last subview at every step up to the window

So a sheet, a see-through cover, a partial cover, a transition still animating in, and any banner drawn over the cover all keep the content behind them redacted.

Note

A reporter behind a cover no longer vetoes the frame when it has not laid out yet, since it has no rect to fail closed on.

💚 How did you test it?

New suite PostHogTests/PostHogMaskPresentationTest.swift:

  • a SwiftUI fullScreenCover over a masked view drops its mask, and the mask returns on dismissal
  • an opaque cover that provably keeps the presenter attached drops the masks behind it
  • a see-through cover keeps them
  • a hidden ancestor drops a reporter's mask
Not verified in this environment

No macOS toolchain or simulator was available in the sandbox, so nothing was compiled, make test/make lint were not run, and the visual result was not confirmed on a device. Every claim above rests on code reading and the tests as written. A reviewer should run the suite on the iOS 26 simulator and eyeball a recording of the reported flow before merging.

📝 Checklist

  • I reviewed the submitted code.
  • I added tests to verify the changes.
  • I updated the docs if needed.
  • No breaking change or entry added to the changelog.

If releasing new changes

  • Ran pnpm changeset to generate a changeset file (added by hand: .changeset/replay-mask-behind-cover.md, patch)

🤖 Agent context

Autonomy: Fully autonomous

  • Written by Claude Opus 5 in PostHog Desktop from the inbox report linked below. No human drove the work, so the PR is left unassigned for the owning team.
  • Rejected: keying the fix on modalPresentationStyle alone (misses .overFullScreen, and the styles UIKit detaches for never had the bug); trusting UIView.isOpaque (defaults to true on transparent views, would unmask live content); a general sibling-occlusion sweep of the whole hierarchy (more under-masking risk than the reported defect is worth).
  • Kept deliberately narrow: masks fail closed everywhere the cover cannot be proven opaque, full-window and topmost. The failure mode of a wrong guess is a stale box, never exposed content.
  • A /simplify pass moved the cover filter down into collectMaskedRegions so the heuristic path inherits it, fused the two per-reporter ancestor walks into one, dropped a child-controller recursion that ran on every capture, and hoisted the test host harness in PostHogMaskScenarioTest.swift to file scope instead of copying it.

Created with PostHog Desktop from this inbox report.

A `postHogMask()` reporter registers itself while it is attached to a window and
reports its rect live at capture time. The registry filtered only on the window,
the reporter's own `isHidden` and its alpha, so a screen that went off screen but
stayed attached kept reporting. SwiftUI's `fullScreenCover` does exactly that on
newer OS versions, where UIKit no longer takes the presenter's view out of the
window, and the covered screen's mask boxes were then painted over the cover.

Adds two conditions, both applied where the two rect sources meet:

- A reporter must reach the screen: no hidden or fully transparent view anywhere
  between it and the window, in one walk up the ancestry.
- When a presented view holds the whole window opaquely and nothing is drawn over
  it, only its own subtree is on screen. The heuristic walk starts there too, so
  a covered `UITextField` no longer leaks a stale rect either.

The cover test fails closed: a sheet, a see-through cover, a partial cover, a
transition still on its way in, or anything drawn above the cover all keep the
content behind it masked.

Generated-By: PostHog Desktop
Task-Id: 8a7caf3c-7cee-428d-8ca3-6ffedda19a2e
@posthog

posthog Bot commented Sep 10, 2026 •

Copy link
Copy Markdown
Contributor Author

🦔 PostHog Review reviewed this pull request

Found 0 must fix, 1 should fix, 1 consider.

Published 2 findings (view the review).

@github-actions

github-actions Bot commented Sep 10, 2026 •

Copy link
Copy Markdown
Contributor

posthog-ios Compliance Report

Date: 2026-09-15 11:12:45 UTC
Duration: 204709ms

⚠️ Some Tests Failed

44/45 tests passed, 1 failed


Capture Tests

⚠️ 28/29 tests passed, 1 failed

View Details
Test Status Duration
Format Validation.Event Has Required Fields ✅ 3044ms
Format Validation.Event Has Uuid ✅ 2784ms
Format Validation.Event Has Lib Properties ✅ 303ms
Format Validation.Distinct Id Is String ✅ 451ms
Format Validation.Token Is Present ✅ 425ms
Format Validation.Custom Properties Preserved ✅ 304ms
Format Validation.Event Has Timestamp ✅ 2726ms
Retry Behavior.Retries On 503 ✅ 11767ms
Retry Behavior.Does Not Retry On 400 ✅ 2388ms
Retry Behavior.Does Not Retry On 401 ✅ 4825ms
Retry Behavior.Respects Retry After Header ✅ 7864ms
Retry Behavior.Implements Backoff ✅ 15511ms
Retry Behavior.Retries On 500 ✅ 8874ms
Retry Behavior.Retries On 502 ✅ 9151ms
Retry Behavior.Retries On 504 ✅ 9245ms
Retry Behavior.Max Retries Respected ❌ 15344ms
Deduplication.Generates Unique Uuids ✅ 3069ms
Deduplication.Preserves Uuid On Retry ✅ 8871ms
Deduplication.Preserves Uuid And Timestamp On Retry ✅ 16195ms
Deduplication.Preserves Uuid And Timestamp On Batch Retry ✅ 8012ms
Deduplication.No Duplicate Events In Batch ✅ 2888ms
Deduplication.Different Events Have Different Uuids ✅ 2864ms
Compression.Sends Gzip When Enabled ✅ 2718ms
Batch Format.Uses Proper Batch Structure ✅ 417ms
Batch Format.Flush With No Events Sends Nothing ✅ 296ms
Batch Format.Multiple Events Batched Together ✅ 2946ms
Error Handling.Does Not Retry On 403 ✅ 4745ms
Error Handling.Does Not Retry On 413 ✅ 4789ms
Error Handling.Retries On 408 ✅ 5453ms

Failures

retry_behavior.max_retries_respected

Expected 4 requests, got 5

Feature_Flags Tests

✅ 16/16 tests passed

View Details
Test Status Duration
Request Payload.Request With Person Properties Device Id ✅ 2829ms
Request Payload.Flags Request Uses V2 Query Param ✅ 2836ms
Request Payload.Flags Request Hits Flags Path Not Decide ✅ 2842ms
Request Payload.Flags Request Omits Authorization Header ✅ 2859ms
Request Payload.Token In Flags Body Matches Init ✅ 2887ms
Request Payload.Groups Round Trip ✅ 2730ms
Request Payload.Groups Default To Empty Object ✅ 2864ms
Request Payload.Person Properties Distinct Id Auto Populated When Caller Omits It ✅ 2760ms
Request Payload.Disable Geoip False Propagates As Geoip Disable False ✅ 3009ms
Request Payload.Disable Geoip Omitted Defaults To False ✅ 2770ms
Request Payload.Flag Keys To Evaluate Contains Only Requested Key ✅ 2789ms
Request Lifecycle.No Flags Request On Init Alone ✅ 72ms
Request Lifecycle.No Flags Request On Normal Capture ✅ 2870ms
Request Lifecycle.Two Flag Calls Produce Two Remote Requests ✅ 5493ms
Request Lifecycle.Mock Response Value Is Returned To Caller ✅ 2830ms
Side Effect Events.Get Feature Flag Captures Feature Flag Called Event ✅ 2859ms

@posthog

posthog Bot commented Sep 10, 2026

Copy link
Copy Markdown
Contributor Author

PostHog Review alpha 🦔 If you find any issues helpful - please reply "valid", "invalid", etc., for evaluation purposes 🙏

@posthog posthog Bot left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PostHog Review

Found 2 must fix, 1 should fix, 2 consider.

Comment thread PostHog/Replay/PostHogReplayIntegration.swift
Comment thread PostHog/Replay/UIView+Util.swift
Comment thread PostHog/Replay/PostHogPresentationCover.swift Outdated
Comment thread PostHog/Replay/PostHogPresentationCover.swift Outdated
Comment thread PostHog/Replay/PostHogPresentationCover.swift Outdated
posthog Bot added 4 commits September 10, 2026 06:22
PostHog.xcodeproj lists every source explicitly, so the `PostHog` framework
target could not see `PostHogPresentationCover.swift` and the iOS build broke
at the call site in `PostHogReplayIntegration.swift`. Register it in the Replay
group and the framework's Sources phase, and register
`PostHogMaskPresentationTest.swift` in the PostHogTests target so the new suite
actually runs on the simulator job.

Generated-By: PostHog Desktop
Task-Id: a0b8fea5-a4d8-4ad2-a74e-c3ea70dec3de
The new ancestor walk read `UIView.alpha`, the model value, which parks at the
destination on the first run loop pass of a `UIView.animate` fade. During a
fade-out it reads 0 for the whole animation while the presentation layer still
renders the content, so a `postHogMask()` reporter was dropped while its pixels
were still legible — the opposite of the fail-closed behaviour this change set
claims for itself. No other guard covers it: `isAnimatingTransition` only sees
`UIViewControllerTransitionCoordinator`, which a plain view animation never
sets.

Treat a view as transparent only when the presentation tree agrees, falling back
to the model value when no presentation layer exists and nothing is therefore in
flight. This mirrors `toPresentationRect`, which already prefers the
presentation tree for mask geometry for the same reason. The new predicate is a
strict subset of the old one, so it can only keep masks the old code dropped,
never the reverse.

Generated-By: PostHog Desktop
Task-Id: a0b8fea5-a4d8-4ad2-a74e-c3ea70dec3de
A background colour only fills the layer's own outline, so it does not prove the
view's output covers its bounds. A rounded `.overFullScreen` cover leaves the
presenter showing through the corner arcs, and a mask layer can cut any hole it
likes — `toPresentationRect` reports the plain bounds in both cases, so the
full-window test cannot notice and masks behind the cover were dropped while
that content was still visible.

Require the inferred branch to be unshaped: no corner radius, no mask layer. The
`.fullScreen` fast path is untouched, since it rests on UIKit's presentation
contract rather than on what the view paints. The added conditions can only
reject covers the old code accepted, so this can only keep masks, never drop
one.

Adds a regression case that flips a single cover from accepted to rejected by
rounding it, so the corner radius is provably what decides it.

Generated-By: PostHog Desktop
Task-Id: a0b8fea5-a4d8-4ad2-a74e-c3ea70dec3de
Sibling layers composite by `zPosition` and fall back to subview order only when
those values tie, so `subviews.last` alone does not tell which view is drawn last.
An app's banner kept above later presentations by a raised `zPosition` sits earlier
in the array, passed the old check, and let the cover be trusted — dropping the
masks of the banner's own content while it was still on screen, which is the exact
case the check was written to catch.

Compare both keys at every ancestor level: a sibling wins on a higher `zPosition`,
or on a later index when the values are equal. With no `zPosition` set anywhere the
comparison reduces to the previous index test, so untouched hierarchies behave as
before, and any new rejection only keeps masks.

Adds a regression case that inserts a banner behind the presentation, checks the
cover still holds, then raises only its `zPosition` and expects the masks back.


Generated-By: PostHog Desktop
Task-Id: b35d85f9-01d1-47b1-9292-7fc9185fb196
@dustinbyrne
dustinbyrne requested a review from a team September 11, 2026 21:11
@marandaneto marandaneto self-assigned this Sep 13, 2026
@marandaneto
marandaneto marked this pull request as ready for review September 13, 2026 08:20
…vers

# Conflicts:
#	PostHog/Replay/PostHogReplayIntegration.swift
#	PostHog/Replay/UIView+Util.swift
#	PostHog/SwiftUI/PostHogMaskViewModifier.swift
@marandaneto

Copy link
Copy Markdown
Member

Local fix, main merge, and test results

Pushed the privacy correction in 393bfc3 and merged main (2a40ee24a) in 41a28be. Resolved the three replay conflicts by retaining main's per-subtree masking state and hasRenderedOpacity helper alongside the cover/ancestor filtering.

The cover optimization now requires opaque model and rendered backgrounds, checks opacity/shape/clipping throughout the ancestry, and fails closed for rotation, shear, perspective, masks, and uncertain coverage. It no longer assumes .fullScreen itself proves opacity.

Results on the merged commit

Command / scope Result
make testPresentationMasks, iOS 26.5 20 tests passed (original stale-mask + expanded privacy suites)
make testPresentationMasks, iOS 26.4.1 20 tests passed
make testOniOSSimulator, iOS 26.5 193 XCTest + 944 Swift Testing tests passed, no crash/restart
make test, macOS 172 XCTest + 799 Swift Testing tests passed
make format, make lint, git diff --check Passed; lint has existing warnings, no serious violations

Privacy coverage includes sensitive content inside UIKit/SwiftUI covers, automatic text/input/image masking, secure fields, sheets, partial/nested covers, dismissal/slide animations, fade-in/fade-out, animated background/rotation/corners/ancestor opacity, and masked ancestors. The original reported stale-mask reproduction still passes. The expanded privacy suite was RED on the uncorrected PR and GREEN with this correction.

About the two observed test-app crashes

During publication cleanup I initially made the entire iOS suite app-hosted. That was incorrect: existing suites rely on the hostless xctest identity/entitlements. The crash reports pointed to a push-notification test's placeholder response reaching a live subscriber, and PostHogStorageTest.swift:30 force-unwrapping a nil app-group directory—not the cover helper. There were also host-identity assertion failures.

This is now separated properly: only make testPresentationMasks uses the example app as a host, and the normal suite remains hostless. CI runs both targets. The regular full iOS suite and the dedicated presentation suite both pass in that arrangement, without crashes. Normal example-app behavior is preserved outside tests.

Before the main merge, all SDK platform builds passed. Full local make build reached the external-SDK client example and stopped at a worktree package-identity mismatch (posthog-ios-repro-812 versus posthog-ios); that is not being reported as a full-build pass.

These are collection/painting and regression-test results, not an exhaustive privacy guarantee or an end-to-end recording-upload/player test. CI for 41a28be is still running.

@posthog posthog Bot left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PostHog Review

Found 1 should fix, 1 consider.

Comment thread PostHog/Replay/PostHogReplayIntegration.swift Outdated
Comment thread PostHog/Replay/PostHogPresentationCover.swift
marandaneto and others added 5 commits September 13, 2026 10:45
The heuristic walk now starts at the cover, so it no longer passes the views between the cover and the window. Those views own the two rules that reach a whole subtree - `ph-no-capture` masks everything below it, `ph-no-mask` drops every heuristic mask below it - and the cover sits inside that subtree. A window marked `ph-no-capture` therefore stopped redacting the screen shown over it.

Replay both rules along the path from the window down to the cover before the walk starts below them. The sensitive-type checks stay out: those describe an ancestor's own pixels, which the cover hides.

Generated-By: PostHog Desktop
Task-Id: 59a53674-e1cc-4a4b-804b-499ccdb313ec
…thing

`drawsLast` read only `zPosition` and the subview index, so any later or raised sibling vetoed the cover - a hidden one included. An overlay host parked in the window between uses, which is where a raised `zPosition` usually comes from, therefore kept the covered screen's masks painting over the cover: the defect this PR fixes stayed for those apps.

Skip a sibling that paints nothing. The rule drops masks, so it demands agreement from both trees: the model flag and the rendered layer, the same caution the cover and its ancestors already get. A sibling whose hidden flag or alpha only sits at its animation destination still vetoes.

Generated-By: PostHog Desktop
Task-Id: 59a53674-e1cc-4a4b-804b-499ccdb313ec
Keep fading siblings masked until both layer trees report invisibility. Validate inherited masking rules and move the cover traversal helper into a private extension to retain the existing lint limit.
…os-ignore-masking-reporters-fcc05a' into fix/pr-812-private-covers

# Conflicts:
#	PostHog/Replay/PostHogPresentationCover.swift
@marandaneto

Copy link
Copy Markdown
Member

Follow-up validation and reconciliation — 9888a74

Merged the bot's ancestor-masking fix (51a4aa0) and invisible-sibling fix (93f912e), retained the additional regression tests from 9761597, and pushed the reconciled result in 9888a74. No history was rewritten.

  • 26 presentation/privacy tests pass on iOS 26.5 and 26.4.1 with make testPresentationMasks.
  • make format, make lint, and git diff --check pass. The bot's ancestor helper was moved unchanged into a private extension to fix the class-length lint error without suppressing the rule.
  • Immediately before reconciliation, the full iOS suite passed 193 XCTest + 944 Swift Testing tests, and macOS passed 172 XCTest + 799 Swift Testing tests. The exact merge was then checked with both hosted simulator runs above.

Review evidence

  • Ancestor inheritance (thread): both ph-no-capture and ph-no-mask tests fail with the pre-bot implementation and pass with the bot's correction. Explicit reporters still remain masked under a heuristic ph-no-mask ancestor.
  • Invisible siblings (thread): reproduced stale masks with hidden and alpha-zero overlays in both later-insertion and raised-zPosition configurations. The fade-out test also failed after the banner became fully transparent. All five cases now pass, while explicit and automatic sensitive content in a visible/fading banner stays masked. The merge keeps the bot's conservative drawsNothing helper rather than duplicating the two implementations.
  • Transparent SwiftUI cover (thread): e75f343 added the exact fullScreenCover + .presentationBackground(.clear) test. It passes on both runtimes. Important correction to the original review: on both tested runtimes this API uses .overFullScreen (raw value 5) and nil model/rendered background colors. This exact case also passed with the old cover helper on iOS 26.5, so it is not claimed as a newly reproduced leak. The style-only opacity shortcut is removed, and the original opaque-cover stale-mask fix still passes. Older runtimes were not tested.

GitHub's inline-reply API has repeatedly returned internal/gateway errors (including request ID 1086:29C078:113A36E3:10BB15EA:6AA665B1). This comment records the evidence without claiming an inline reply or resolution succeeded. CI for the new head is pending; these are local test results, not an exhaustive privacy guarantee.

@marandaneto

Copy link
Copy Markdown
Member

CI is now green for 9888a74: all 46 checks passed. The only failing job was Swift CodeQL; its build and analysis completed, but the original attempt failed during SARIF upload without a diagnostic. Rerunning only the failed job succeeded on attempt 2 with the same commit. No SDK or workflow changes were needed. Run: https://github.com/PostHog/posthog-ios/actions/runs/34749109811

@dustinbyrne dustinbyrne left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An animated sensitive sibling can remain visibly above the opaque cover while its model zPosition is lower. This comparison then removes its mask. Consider using presentation-layer ordering, or conservatively retaining the mask when ordering is uncertain. A capture during an explicit zPosition animation would cover this case.

@marandaneto
marandaneto requested review from a team and dustinbyrne September 15, 2026 08:52
@marandaneto

Copy link
Copy Markdown
Member

Addressed @dustinbyrne’s animated zPosition review in 566d28f.

drawsLast now checks both model and presentation-layer ordering, preserving masks if either tree puts a visible sibling above the cover. This also handles an animating cover, not just an animating sibling.

Regression evidence:

  • Added animatedZPosition(animateCover:) with two cases using a paused, explicit CABasicAnimation. Each verifies the sensitive sibling is above the cover in the presentation tree but below it in the model tree.
  • Before the fix, mask collection returned no masks for all four sensitive regions (explicit SwiftUI reporter, label, text field, image) in both cases: eight assertion failures. The tests completed, although Xcode hung during result finalization.
  • After the fix, both cases pass. Removing the animation still drops stale masks behind the settled opaque cover.
  • make testPresentationMasks: all 27 tests pass on iOS 26.5 and 26.4.1.
  • make test: 172 XCTest + 799 Swift Testing pass.
  • Full iOS suite (temporary copy of the Makefile with a pinned simulator destination): logged 193 XCTest + 944 Swift Testing passes, but Xcode hung during result finalization, so this was not a clean command exit.
  • make format, make lint, and git diff --check pass (lint reports existing warnings).

Local toolchain limitations: the installed Xcode 27 rejects the repository’s legacy deployment targets. Presentation/iOS tests used a temporary external xcconfig setting iOS 15 only for local validation; no project deployment targets were changed. With local compatibility overrides, the build passed iOS, macOS, Catalyst and tvOS, then stopped at Xcode 27 rejecting the legacy watchOS deployment target/armv7k architecture. Remaining build targets were not reached. CI will validate the committed configuration.

This regression verifies mask collection during rendered z-order divergence, not recording upload/web-player output. Max-retries work remains untouched and out of scope, as requested.

@marandaneto
marandaneto merged commit f5ec8cb into main Sep 15, 2026
46 checks passed
@marandaneto
marandaneto deleted the posthog-self-driving/fixreplay-ios-ignore-masking-reporters-fcc05a branch September 15, 2026 11:33

@posthog posthog Bot left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PostHog Review

Found 1 should fix, 1 consider.

Comment thread Makefile
Comment on lines +116 to +124
xcrun xcodebuild test -project PostHog.xcodeproj -scheme PostHog \
-destination "$${PRESENTATION_TEST_DESTINATION:-platform=iOS Simulator,name=$$device}" \
-parallel-testing-enabled NO \
POSTHOG_PRESENTATION_TEST_HOST='$$(BUILT_PRODUCTS_DIR)/PostHogExample.app/PostHogExample' \
SWIFT_ACTIVE_COMPILATION_CONDITIONS='$$(inherited) TEST_PRESENTATION_MASKS' \
-only-testing:PostHogTests/PostHogMaskPresentationTest \
-only-testing:PostHogTests/PostHogMaskPresentationPrivacyTest \
2>&1 | tee presentation-masks.log | xcpretty
@grep -q 'Suite "Replay masking behind a cover" passed' presentation-masks.log

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Timing-sensitive presentation tests run without retries

should_fix testing

Issue description

These suites drive UIKit presentations and real animations. The existing simulator target retries timing failures, but this target runs each test once. A busy runner can therefore fail a healthy change.

Why we think it's a valid issue
  • Checked: the new testPresentationMasks recipe (Makefile:112-126), the sibling testOniOSSimulator recipe (Makefile:93-106), scripts/check-ios-test-result.sh, the flake-report step in .github/workflows/test.yml:70-90, and every real-time assertion in the two suites this target runs.
  • Found: the suites sample live animations and gate on the sampled value with #require, which fails the test outright. Four sites start a two-second linear animation, sleep 300 ms, then require a mid-animation rendered value: PostHogTests/PostHogMaskPresentationPrivacyTest.swift:366-369, :384-387, :428-440, and :589-592. animatedCoverState is parameterised over four arguments (:409-411), so these four sites produce seven test cases. slideTransitions adds an eighth: it polls a real UIKit transition every 20 ms and requires at least one sample, #require(samples > 0, "Must sample exposed PII during real transitions") (:356).
  • Found: the timing margin is bounded and small. The tightest gates require a rendered opacity or background alpha below 0.8 on a two-second linear ramp (:386, :438-441), so the sample must land before 1600 ms. The sleep targets 300 ms, which leaves about 1.3 s of stall tolerance. A stall past that point reads a finished animation and fails the require on correct code.
  • Found: the repository already records this exact failure mode for its own CI. The comment at Makefile:93-95 states that tests asserting real-time behaviour "can't be made deterministic; on slow, load-variable CI runners those windows occasionally slip", and Makefile:104 answers it with -retry-tests-on-failure -test-iterations 3.
  • Found: the new target gets none of that handling. Makefile:116-123 runs xcodebuild test once, with no retry flags, and pipes straight to tee/xcpretty without calling scripts/check-ios-test-result.sh. That script is what converts "failed, then passed on retry" into a success and still hard-fails a test that never passes.
  • Found: the two suites compile only under the TEST_PRESENTATION_MASKS condition (PostHogTests/PostHogMaskPresentationPrivacyTest.swift:1), which Makefile:120 sets only in this target. They therefore never run under the retry-enabled testOniOSSimulator target, so no other job covers them.
  • Found: the flake reporter reads only xcodebuild-ios.log (.github/workflows/test.yml:77), so presentation-masks.log is outside it.
  • Impact: a healthy pull request can get a red merge-blocking check. The step runs in the test-ios-simulator job on every non-markdown pull request, and eight timing-gated cases each need a sample inside a window of about 1.3 s. A transient runner stall fails the job, and the author must re-run to clear it. Adding the retry flags and routing the exit status plus presentation-masks.log through scripts/check-ios-test-result.sh matches the handling the sibling target already uses, and keeps a real repeated failure red.
Suggested fix

Add -retry-tests-on-failure -test-iterations 3. Pass the pipeline status and presentation-masks.log to scripts/check-ios-test-result.sh. Include this log in flake reporting.

Prompt to fix with AI (copy-paste)
## Context
@Makefile#L116-124

<issue_description>
These suites drive UIKit presentations and real animations. The existing simulator target retries timing failures, but this target runs each test once. A busy runner can therefore fail a healthy change.
</issue_description>

<issue_validation>
- **Checked:** the new `testPresentationMasks` recipe (`Makefile:112-126`), the sibling `testOniOSSimulator` recipe (`Makefile:93-106`), `scripts/check-ios-test-result.sh`, the flake-report step in `.github/workflows/test.yml:70-90`, and every real-time assertion in the two suites this target runs.
- **Found:** the suites sample live animations and gate on the sampled value with `#require`, which fails the test outright. Four sites start a two-second linear animation, sleep 300 ms, then require a mid-animation rendered value: `PostHogTests/PostHogMaskPresentationPrivacyTest.swift:366-369`, `:384-387`, `:428-440`, and `:589-592`. `animatedCoverState` is parameterised over four arguments (`:409-411`), so these four sites produce seven test cases. `slideTransitions` adds an eighth: it polls a real UIKit transition every 20 ms and requires at least one sample, `#require(samples > 0, "Must sample exposed PII during real transitions")` (`:356`).
- **Found:** the timing margin is bounded and small. The tightest gates require a rendered opacity or background alpha below 0.8 on a two-second linear ramp (`:386`, `:438-441`), so the sample must land before 1600 ms. The sleep targets 300 ms, which leaves about 1.3 s of stall tolerance. A stall past that point reads a finished animation and fails the require on correct code.
- **Found:** the repository already records this exact failure mode for its own CI. The comment at `Makefile:93-95` states that tests asserting real-time behaviour "can't be made deterministic; on slow, load-variable CI runners those windows occasionally slip", and `Makefile:104` answers it with `-retry-tests-on-failure -test-iterations 3`.
- **Found:** the new target gets none of that handling. `Makefile:116-123` runs `xcodebuild test` once, with no retry flags, and pipes straight to `tee`/`xcpretty` without calling `scripts/check-ios-test-result.sh`. That script is what converts "failed, then passed on retry" into a success and still hard-fails a test that never passes.
- **Found:** the two suites compile only under the `TEST_PRESENTATION_MASKS` condition (`PostHogTests/PostHogMaskPresentationPrivacyTest.swift:1`), which `Makefile:120` sets only in this target. They therefore never run under the retry-enabled `testOniOSSimulator` target, so no other job covers them.
- **Found:** the flake reporter reads only `xcodebuild-ios.log` (`.github/workflows/test.yml:77`), so `presentation-masks.log` is outside it.
- **Impact:** a healthy pull request can get a red merge-blocking check. The step runs in the `test-ios-simulator` job on every non-markdown pull request, and eight timing-gated cases each need a sample inside a window of about 1.3 s. A transient runner stall fails the job, and the author must re-run to clear it. Adding the retry flags and routing the exit status plus `presentation-masks.log` through `scripts/check-ios-test-result.sh` matches the handling the sibling target already uses, and keeps a real repeated failure red.
</issue_validation>

## Task
Investigate the issue and solve it

<potential_solution>
Add `-retry-tests-on-failure -test-iterations 3`. Pass the pipeline status and `presentation-masks.log` to `scripts/check-ios-test-result.sh`. Include this log in flake reporting.
</potential_solution>

Comment thread Makefile
Comment on lines +116 to +121
xcrun xcodebuild test -project PostHog.xcodeproj -scheme PostHog \
-destination "$${PRESENTATION_TEST_DESTINATION:-platform=iOS Simulator,name=$$device}" \
-parallel-testing-enabled NO \
POSTHOG_PRESENTATION_TEST_HOST='$$(BUILT_PRODUCTS_DIR)/PostHogExample.app/PostHogExample' \
SWIFT_ACTIVE_COMPILATION_CONDITIONS='$$(inherited) TEST_PRESENTATION_MASKS' \
-only-testing:PostHogTests/PostHogMaskPresentationTest \

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The global test flag forces a second SDK build

consider performance

Issue description

The command-line SWIFT_ACTIVE_COMPILATION_CONDITIONS applies to every target. It changes the framework compile signature after make testOniOSSimulator. -only-testing limits execution, not compilation, so Xcode recompiles all Swift sources.

Why we think it's a valid issue
  • Checked: the two make steps in the test-ios-simulator job (.github/workflows/test.yml:50, :67, :70), both recipes (Makefile:99-106 and Makefile:112-126), the PostHog scheme's test action, the PostHogTests target settings in PostHog.xcodeproj/project.pbxproj, and the existing snapshot recipes at Makefile:157-170.
  • Found: the premise holds. Makefile:120 passes SWIFT_ACTIVE_COMPILATION_CONDITIONS as a command-line override, which applies to every target in the build, so the framework's swiftc invocation gains -DTEST_PRESENTATION_MASKS. The flag appears in no SDK source: grep finds it only at Makefile:120, PostHogTests/PostHogMaskPresentationTest.swift:9, and PostHogTests/PostHogMaskPresentationPrivacyTest.swift:1. The recompile therefore buys nothing in the framework, and -only-testing filters execution, so the whole test action still builds.
  • Found: both steps share one build directory, so the invalidation is real, not theoretical. Makefile:104 and Makefile:116 use the same PostHog scheme, the same project, and the default derived-data path, and both resolve the same simulator destination. PostHog.xcodeproj/xcshareddata/xcschemes/PostHog.xcscheme runs the test action in the Testing configuration for both. The step at .github/workflows/test.yml:70 runs straight after :67 on the same runner, so it recompiles what the previous step just built. PostHog holds 156 Swift files and about 30,700 lines.
  • Found: the cited precedent does not carry the same cost. maskSnapshots and recordMaskSnapshots use the identical command-line pattern with TEST_MASK_SNAPSHOTS (Makefile:160, :169), but they run in the separate mask-snapshots job on a clean runner, where no earlier build exists to invalidate. This new step is the first to add a project-wide condition after another build in the same job.
  • Found: a developer who alternates make testOniOSSimulator and make testPresentationMasks locally pays the same recompile in both directions, because the condition set differs each time.
  • Found: the pull request already demonstrates a lighter technique for the same goal. PostHog.xcodeproj/project.pbxproj:4563-4564 sets TEST_HOST[sdk=iphonesimulator*] = $(POSTHOG_PRESENTATION_TEST_HOST) on the PostHogTests target only, and the Makefile fills the variable. The same indirection on SWIFT_ACTIVE_COMPILATION_CONDITIONS for that target would confine the flag to the test bundle, so the framework signature stays stable without a new target.
  • Impact: every run of the test-ios-simulator job compiles the SDK twice, and the second compile produces identical code. The cost is continuous-integration and local build time. Nothing about correctness, masking behaviour, or shipped code changes.
  • Priority: lowered to consider. The defect is real and verified, but it costs build minutes rather than correctness, and the proposed remedy is heavier than the problem: a new hosted test target needs its own membership for the test sources and PostHogTests/TestUtils, and every later test utility must then be added to two targets.
Suggested fix

Create a small hosted test target for these suites. Define TEST_PRESENTATION_MASKS only on that target. Keep the PostHog framework settings identical between both simulator steps.

Prompt to fix with AI (copy-paste)
## Context
@Makefile#L116-121

<issue_description>
The command-line `SWIFT_ACTIVE_COMPILATION_CONDITIONS` applies to every target. It changes the framework compile signature after `make testOniOSSimulator`. `-only-testing` limits execution, not compilation, so Xcode recompiles all Swift sources.
</issue_description>

<issue_validation>
- **Checked:** the two `make` steps in the `test-ios-simulator` job (`.github/workflows/test.yml:50`, `:67`, `:70`), both recipes (`Makefile:99-106` and `Makefile:112-126`), the `PostHog` scheme's test action, the `PostHogTests` target settings in `PostHog.xcodeproj/project.pbxproj`, and the existing snapshot recipes at `Makefile:157-170`.
- **Found:** the premise holds. `Makefile:120` passes `SWIFT_ACTIVE_COMPILATION_CONDITIONS` as a command-line override, which applies to every target in the build, so the framework's `swiftc` invocation gains `-DTEST_PRESENTATION_MASKS`. The flag appears in no SDK source: `grep` finds it only at `Makefile:120`, `PostHogTests/PostHogMaskPresentationTest.swift:9`, and `PostHogTests/PostHogMaskPresentationPrivacyTest.swift:1`. The recompile therefore buys nothing in the framework, and `-only-testing` filters execution, so the whole test action still builds.
- **Found:** both steps share one build directory, so the invalidation is real, not theoretical. `Makefile:104` and `Makefile:116` use the same `PostHog` scheme, the same project, and the default derived-data path, and both resolve the same simulator destination. `PostHog.xcodeproj/xcshareddata/xcschemes/PostHog.xcscheme` runs the test action in the `Testing` configuration for both. The step at `.github/workflows/test.yml:70` runs straight after `:67` on the same runner, so it recompiles what the previous step just built. `PostHog` holds 156 Swift files and about 30,700 lines.
- **Found:** the cited precedent does not carry the same cost. `maskSnapshots` and `recordMaskSnapshots` use the identical command-line pattern with `TEST_MASK_SNAPSHOTS` (`Makefile:160`, `:169`), but they run in the separate `mask-snapshots` job on a clean runner, where no earlier build exists to invalidate. This new step is the first to add a project-wide condition after another build in the same job.
- **Found:** a developer who alternates `make testOniOSSimulator` and `make testPresentationMasks` locally pays the same recompile in both directions, because the condition set differs each time.
- **Found:** the pull request already demonstrates a lighter technique for the same goal. `PostHog.xcodeproj/project.pbxproj:4563-4564` sets `TEST_HOST[sdk=iphonesimulator*] = $(POSTHOG_PRESENTATION_TEST_HOST)` on the `PostHogTests` target only, and the Makefile fills the variable. The same indirection on `SWIFT_ACTIVE_COMPILATION_CONDITIONS` for that target would confine the flag to the test bundle, so the framework signature stays stable without a new target.
- **Impact:** every run of the `test-ios-simulator` job compiles the SDK twice, and the second compile produces identical code. The cost is continuous-integration and local build time. Nothing about correctness, masking behaviour, or shipped code changes.
- **Priority:** lowered to `consider`. The defect is real and verified, but it costs build minutes rather than correctness, and the proposed remedy is heavier than the problem: a new hosted test target needs its own membership for the test sources and `PostHogTests/TestUtils`, and every later test utility must then be added to two targets.
</issue_validation>

## Task
Investigate the issue and solve it

<potential_solution>
Create a small hosted test target for these suites. Define `TEST_PRESENTATION_MASKS` only on that target. Keep the PostHog framework settings identical between both simulator steps.
</potential_solution>

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants