fix(replay): drop masks for a screen hidden behind a full-screen cover - #812
Draft
posthog[bot] wants to merge 5 commits into
Draft
fix(replay): drop masks for a screen hidden behind a full-screen cover#812posthog[bot] wants to merge 5 commits into
posthog[bot] wants to merge 5 commits into
Conversation
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
Contributor
Author
🦔 PostHog Review reviewed this pull requestFound 2 must fix, 1 should fix, 2 consider. Published 5 findings (view the review). Resolved comments: 1 fixed, 1 already settled, 1 left for you |
Contributor
posthog-ios Compliance ReportDate: 2026-09-10 06:49:03 UTC ✅ All Tests Passed!45/45 tests passed Capture Tests✅ 29/29 tests passed View Details
Feature_Flags Tests✅ 16/16 tests passed View Details
|
Contributor
Author
|
PostHog Review alpha 🦔 If you find any issues helpful - please reply "valid", "invalid", etc., for evaluation purposes 🙏 |
![posthog[bot]](https://avatars.githubusercontent.com/in/1203049?s=60&v=4){kind=small}
Comment on lines
+67
to
+68
| if controller.modalPresentationStyle == .fullScreen { | ||
| return true |
Contributor
Author
There was a problem hiding this comment.
Do not treat every full-screen presentation as opaque
Why we think it's a valid issue
- Checked: the style short-circuit at PostHog/Replay/PostHogPresentationCover.swift:66-68 against the justification comment at lines 60-64, the callers at PostHogReplayIntegration.swift:1126 and :1133, the ancestry walk at PostHog/Replay/UIView+Util.swift:25-38, and the four cases in PostHogTests/PostHogMaskPresentationTest.swift.
- Found: The branch contradicts the PR's own root cause. The comment at lines 60-62 justifies trusting
.fullScreenbecause "it is the style for which UIKit may drop the presenter's view from the window." The PR exists because UIKit does not drop it — the commit message states that on newer OS versions "UIKit no longer takes the presenter's view out of the hierarchy" forfullScreenCover, which uses exactly this style. So the property that made the inference safe is the property the PR reports as absent. - Found: The two cases are indistinguishable to this code. A
UIHostingControllerview presented byfullScreenCovernormally carries a nilbackgroundColor, so both an opaque cover and a transparent one reach line 67 with the same style and the same nil background. The short-circuit returns true for both. The test at PostHogMaskPresentationTest.swift:74-87 depends on this: its cover paints white in SwiftUI content (ZStack { Color.white... }, line 38), not on the hosting view'sbackgroundColor. - Found: A transparent full-screen cover is a supported, first-party pattern, not a contrivance.
.presentationBackground(.clear)applies to afullScreenCoverpresentation from iOS 16.4, and the olderUIViewControllerRepresentableworkaround reaches the same result. Both keepmodalPresentationStyle == .fullScreen. The presented view still spans the window and stays topmost, so all three tests at lines 35-38 pass. - Found: The suite gives false confidence here. The only see-through case, at line 105-110, presents with
.overFullScreenand a.clearbackground, so it exercises the background branch at line 69 and never reaches the style short-circuit. No test covers a transparent.fullScreenpresentation. - Impact: Every mask behind the cover is dropped while the content behind it is plainly visible through the clear presentation. The blast radius is the whole screen, not a sliver, and it is a regression: before this diff the registry kept those masks. The failure is silent — the SDK captures the sensitive pixels and nothing signals it.
- Impact: The premise cannot fail safely for the author. If UIKit really keeps the presenter attached for
.fullScreen, this hole is live. If it removes the presenter as Apple documents, then the reporters behind have no window,isVisibleToWindowalready returns false at UIView+Util.swift:37, and the cover logic was not needed for the reported bug in the first place. - Impact: Note the cost of the suggested remedy. Removing the style short-circuit without a replacement opacity signal would likely break the PR's primary fix, because the hosting view's
backgroundColoris nil for an opaquefullScreenCovertoo. The branch needs a different opacity signal, not simple deletion.
Issue description
The .fullScreen style does not guarantee an opaque background. SwiftUI supports translucent presentation backgrounds. The target OS can keep the presenter attached, so this branch can remove masks from presenter pixels that remain visible.
Suggested fix
Do not use modalPresentationStyle as opacity proof. Require a proven opaque rendered cover when the presenter remains attached. Otherwise, scan the full window. Add a transparent fullScreenCover regression test.
Prompt to fix with AI (copy-paste)
## Context
@PostHog/Replay/PostHogPresentationCover.swift#L67-68
<issue_description>
The .fullScreen style does not guarantee an opaque background. SwiftUI supports translucent presentation backgrounds. The target OS can keep the presenter attached, so this branch can remove masks from presenter pixels that remain visible.
</issue_description>
<issue_validation>
- **Checked:** the style short-circuit at PostHog/Replay/PostHogPresentationCover.swift:66-68 against the justification comment at lines 60-64, the callers at PostHogReplayIntegration.swift:1126 and :1133, the ancestry walk at PostHog/Replay/UIView+Util.swift:25-38, and the four cases in PostHogTests/PostHogMaskPresentationTest.swift.
- **Found:** The branch contradicts the PR's own root cause. The comment at lines 60-62 justifies trusting `.fullScreen` because "it is the style for which UIKit **may** drop the presenter's view from the window." The PR exists because UIKit does not drop it — the commit message states that on newer OS versions "UIKit no longer takes the presenter's view out of the hierarchy" for `fullScreenCover`, which uses exactly this style. So the property that made the inference safe is the property the PR reports as absent.
- **Found:** The two cases are indistinguishable to this code. A `UIHostingController` view presented by `fullScreenCover` normally carries a nil `backgroundColor`, so both an opaque cover and a transparent one reach line 67 with the same style and the same nil background. The short-circuit returns true for both. The test at PostHogMaskPresentationTest.swift:74-87 depends on this: its cover paints white in SwiftUI content (`ZStack { Color.white... }`, line 38), not on the hosting view's `backgroundColor`.
- **Found:** A transparent full-screen cover is a supported, first-party pattern, not a contrivance. `.presentationBackground(.clear)` applies to a `fullScreenCover` presentation from iOS 16.4, and the older `UIViewControllerRepresentable` workaround reaches the same result. Both keep `modalPresentationStyle == .fullScreen`. The presented view still spans the window and stays topmost, so all three tests at lines 35-38 pass.
- **Found:** The suite gives false confidence here. The only see-through case, at line 105-110, presents with `.overFullScreen` and a `.clear` background, so it exercises the background branch at line 69 and never reaches the style short-circuit. No test covers a transparent `.fullScreen` presentation.
- **Impact:** Every mask behind the cover is dropped while the content behind it is plainly visible through the clear presentation. The blast radius is the whole screen, not a sliver, and it is a regression: before this diff the registry kept those masks. The failure is silent — the SDK captures the sensitive pixels and nothing signals it.
- **Impact:** The premise cannot fail safely for the author. If UIKit really keeps the presenter attached for `.fullScreen`, this hole is live. If it removes the presenter as Apple documents, then the reporters behind have no window, `isVisibleToWindow` already returns false at UIView+Util.swift:37, and the cover logic was not needed for the reported bug in the first place.
- **Impact:** Note the cost of the suggested remedy. Removing the style short-circuit without a replacement opacity signal would likely break the PR's primary fix, because the hosting view's `backgroundColor` is nil for an opaque `fullScreenCover` too. The branch needs a different opacity signal, not simple deletion.
</issue_validation>
## Task
Investigate the issue and solve it
<potential_solution>
Do not use modalPresentationStyle as opacity proof. Require a proven opaque rendered cover when the presenter remains attached. Otherwise, scan the full window. Add a transparent fullScreenCover regression test.
</potential_solution>
Contributor
Author
There was a problem hiding this comment.
Escalating this one to a human rather than guessing at it — the analysis holds, and it is the only branch in this file that can unmask live content, but every candidate fix turns on a runtime fact that cannot be checked without a simulator, and this environment has no Swift toolchain at all.
What holds: the .fullScreen short-circuit is still there and unchanged by the two fixes landed on this branch (the shaped-cover guard in 38fbd2b and the z-order fix in 7cbafde). Its stated justification is that .fullScreen is the style for which UIKit may take the presenter's view out of the window — but this PR exists precisely because newer OS versions keep it in. A transparent full-screen presentation (.presentationBackground(.clear), iOS 16.4+) keeps that same style, spans the window and stays topmost, so it passes all three conditions and drops every mask behind it. No test covers it: the only see-through case in the suite presents .overFullScreen, which goes down the background-colour branch instead.
Why it is not being fixed unattended — two questions have to be answered on a device or simulator first, and they decide which fix is even correct:
- Does an opaque SwiftUI
fullScreenCoverexpose any opaque signal on the presented view? PrintpresentedViewController.view.backgroundColorfor a plainfullScreenCoveron iOS 16.4 through 26. If it is opaque, the short-circuit can simply be deleted and the existing background-colour branch carries the PR's fix — the smallest and strictly fail-closed change. If it is nil or clear (as this review reports), deletion silently reverts the PR's headline behaviour and makes thefullScreenCovertest fail; that test must not be weakened to paper over it. - How does a transparent presentation actually surface? If
.presentationBackground(.clear)sets a clearbackgroundColoron that same view, a one-line narrowing closes the hole. If it plumbs the transparency somewhere else, that narrowing would look like a fix while changing nothing — the worst outcome on a privacy path.
If both answers point the wrong way, the remaining options are a subtree scan for a proven opaque backdrop (a new per-frame heuristic on the unmask path), narrowing this PR to non-.fullScreen covers only, or reconsidering the mechanism altogether — because the third question this thread implies is a genuine design call: if UIKit really does detach the presenter for .fullScreen, then those reporters have no window, the existing visibility walk already drops them, and the cover logic was not needed for the reported bug in the first place.
Until that is settled by someone who can run the app, the honest state of this PR is that the two fail-closed gaps are fixed and this fail-open one is open. Please treat it as blocking for merge.
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
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
💡 Motivation and Context
postHogMask()region kept being redacted after its screen went behind a SwiftUIfullScreenCover, 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.PostHogSessionReplayMaskRegistry.maskedRects(in:)filtered on the window plus the reporter's ownisHidden/alphaonly. 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.UITextFieldcould leak a stale rect the same way.Changes
Both rect sources are now pruned by one rule, computed once per frame in
collectMaskedRegions:The cover test fails closed — it keeps everything behind the presentation masked unless every condition holds:
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:fullScreenCoverover a masked view drops its mask, and the mask returns on dismissalNot verified in this environment
No macOS toolchain or simulator was available in the sandbox, so nothing was compiled,
make test/make lintwere 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
If releasing new changes
pnpm changesetto generate a changeset file (added by hand:.changeset/replay-mask-behind-cover.md, patch)🤖 Agent context
Autonomy: Fully autonomous
modalPresentationStylealone (misses.overFullScreen, and the styles UIKit detaches for never had the bug); trustingUIView.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)./simplifypass moved the cover filter down intocollectMaskedRegionsso 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 inPostHogMaskScenarioTest.swiftto file scope instead of copying it.Created with PostHog Desktop from this inbox report.