Repository navigation
fix(replay): mask full-window ph-no-capture views - #970
Merged
Merged
Conversation
A view tagged ph-no-capture now always masks its own rect. Previously a tagged view that filled the window was skipped and only its descendants were masked, so its own drawing reached the screenshot. Co-Authored-By: Claude Opus 5.5 <[email protected]>
Contributor
There was a problem hiding this comment.
PostHog Review · Standard · reviewed 4b085fc: 1 Must fix.
See this review and its findings in PostHog Review. Comment @posthog review --deep for a Deep review. A Deep review takes longer and looks deeper than a Standard review.
Contributor
posthog-ios Compliance ReportDate: 2026-10-27T22:24:43.478149+00:00
|
| Test | Status | Duration |
|---|---|---|
| Format Validation.Event Has Required Fields | ✅ | 2770ms |
| Format Validation.Event Has Uuid | ✅ | 2785ms |
| Format Validation.Event Has Lib Properties | ✅ | 2782ms |
| Format Validation.Distinct Id Is String | ✅ | 2665ms |
| Format Validation.Token Is Present | ✅ | 2770ms |
| Format Validation.Custom Properties Preserved | ✅ | 2677ms |
| Format Validation.Event Has Timestamp | ✅ | 2817ms |
| Format Validation.Non Utc Event Timestamp Is Converted To Utc | ✅ | 2763ms |
| Retry Behavior.Retries On 503 | ✅ | 11795ms |
| Retry Behavior.Does Not Retry On 400 | ✅ | 2415ms |
| Retry Behavior.Does Not Retry On 401 | ✅ | 4668ms |
| Retry Behavior.Respects Retry After Header | ✅ | 7702ms |
| Retry Behavior.Implements Backoff | ✅ | 21741ms |
| Retry Behavior.Retries On 500 | ✅ | 8759ms |
| Retry Behavior.Retries On 502 | ✅ | 9233ms |
| Retry Behavior.Retries On 504 | ✅ | 5251ms |
| Retry Behavior.Max Retries Respected | ❌ | 21249ms |
| Deduplication.Generates Unique Uuids | ✅ | 2861ms |
| Deduplication.Preserves Uuid On Retry | ✅ | 9274ms |
| Deduplication.Preserves Uuid And Timestamp On Retry | ✅ | 16735ms |
| Deduplication.Preserves Uuid And Timestamp On Batch Retry | ✅ | 7704ms |
| Deduplication.No Duplicate Events In Batch | ✅ | 2800ms |
| Deduplication.Different Events Have Different Uuids | ✅ | 2759ms |
| Compression.Sends Gzip When Enabled | ✅ | 2708ms |
| Batch Format.Uses Proper Batch Structure | ✅ | 625ms |
| Batch Format.Flush With No Events Sends Nothing | ✅ | 312ms |
| Batch Format.Multiple Events Batched Together | ✅ | 2793ms |
| Error Handling.Does Not Retry On 403 | ✅ | 4788ms |
| Error Handling.Does Not Retry On 413 | ✅ | 4667ms |
| Error Handling.Retries On 408 | ✅ | 8714ms |
Failures
retry_behavior.max_retries_respected
Expected 4 requests, got 6
Feature_Flags Tests
✅ 17/17 tests passed
View Details
| Test | Status | Duration |
|---|---|---|
| Request Payload.Request With Person Properties Device Id | ✅ | 2809ms |
| Request Payload.Flags Request Uses V2 Query Param | ✅ | 2719ms |
| Request Payload.Flags Request Hits Flags Path Not Decide | ✅ | 2784ms |
| Request Payload.Flags Request Omits Authorization Header | ✅ | 2813ms |
| Request Payload.Token In Flags Body Matches Init | ✅ | 2734ms |
| Request Payload.Groups Round Trip | ✅ | 2766ms |
| Request Payload.Groups Default To Empty Object | ✅ | 2715ms |
| Request Payload.Disable Geoip False Propagates As Geoip Disable False | ✅ | 2794ms |
| Request Payload.Disable Geoip Omitted Defaults To False | ✅ | 2758ms |
| Request Payload.Flag Keys To Evaluate Contains Only Requested Key | ✅ | 2816ms |
| Request Lifecycle.No Flags Request On Init Alone | ✅ | 60ms |
| Request Lifecycle.No Flags Request On Normal Capture | ✅ | 2722ms |
| Request Lifecycle.Two Flag Calls Produce Two Remote Requests | ✅ | 5386ms |
| Request Lifecycle.Mock Response Value Is Returned To Caller | ✅ | 2748ms |
| Retry Behavior.Retries Flags On 502 | ✅ | 2770ms |
| Retry Behavior.Retries Flags On 504 | ✅ | 2754ms |
| Side Effect Events.Get Feature Flag Captures Feature Flag Called Event | ✅ | 2953ms |
A tagged view's mask is its presentation rect, so an unclipped descendant drawing outside it (for example while the view's bounds animate) leaked. A tagged view now masks its own rect and every descendant, whatever its size, and the tests that pin descendant masking are restored. Co-Authored-By: Claude Opus 5.5 <[email protected]>
dustinbyrne
approved these changes
Oct 27, 2026
dustinbyrne
left a comment
Contributor
There was a problem hiding this comment.
The previous descendant-masking issue is resolved. One optional mixed UIKit/SwiftUI mask-source overlap is noted inline.
AI-assisted review.
The native walk and the mask registry both collected it, and the repeated owner made the settle check read a still frame as motion. Co-Authored-By: Claude Opus 5.5 <[email protected]>
turnipdabeets
enabled auto-merge (squash)
October 27, 2026 22:25
turnipdabeets
disabled auto-merge
October 27, 2026 22:33
turnipdabeets
enabled auto-merge (squash)
October 27, 2026 22:33
4 of 5 tasks
turnipdabeets
added a commit
that referenced
this pull request
Oct 27, 2026
… guide (#980) #970 masks a full-window ph-no-capture view and its descendants, so the exception no longer applies. Co-authored-by: Claude Opus 5.5 <[email protected]>
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
A view tagged
ph-no-capturethat fills the window was not masked itself. Only its descendants were. A full-window custom-drawn view that draws a secret and has one small subview therefore uploaded the secret in the screenshot. This was raised by the bots on #873 (thread 1, thread 2) and deferred to a fix onmain.Decision: an explicit
ph-no-capturealways masks the view's own rect and its descendantsfindMaskableWidgetsnow masks a tagged view's rect whatever its size, and the full-window exemption is removed. Every tagged view also passes the inheritedmaskChildrenflag down, which used to happen only for full-window ones. The mask uses the view's presentation rect, so an unclipped descendant can draw outside it, for example while the view's bounds animate. The bot review on this PR caught that case. The same rule applies when an ancestor of a presented cover is tagged.Why the exemption is safe to remove:
RCTViewandRCTViewComponentViewansweraccessibilityLabelwith their descendants' labels joined (RCTRecursiveAccessibilityLabel). A full-screen RN root holding aPostHogMaskViewanywhere therefore reportedph-no-captureand would have blacked out the screen.containsAccessibilityTokenreadssuper.accessibilityLabel. That skips the RCTView override and sees only an explicitly assigned label.PostHogAccessibilityTokenCarrierTestalready pins this, and a new RN test pins the end-to-end case: a full-window container whose derived label contains the token is not masked, only thePostHogMaskView-style child is.posthog-androidmasks aph-no-captureview's visible rect with no size exemption (PostHogReplayIntegration.kt,view.isNoCapture() -> view.addGlobalVisibleRect(walk)).Behavior changes (both stricter, both intended — please confirm)
ph-no-captureview now records as a fully black frame. That includes a taggedUIWindow, a full-screen tagged root view, and an RN app that wraps its whole screen inPostHogMaskViewor setsaccessibilityLabel="ph-no-capture"on a full-screen container. Before, only that view's non-full-window descendants were masked, which mostly blacked out the screen anyway.ph-no-maskview inside a full-windowph-no-captureview is now covered by the parent's rect. This is already what happened for any smaller tagged parent, and it matches Android.ph-no-captureview now also masks each descendant, as full-window ones always did. Inside the view's rect this changes nothing visible. It only changes unclipped descendants that draw outside the rect, which were uploaded before. Android doesn't do this: it masks the tagged view's rect and stops walking.v4 compatibility
chore/v4-remove-deprecatedchanges the same line to|| view.subviews.isEmpty, so a full-window leaf is masked. This PR covers that case too. When merging into v4, take this side of the hunk. Also take this PR's expectation forinheritedMaskRespectsScopeAndNoMask, which now includes the parent rect.💚 How did you test it?
On an iOS 26.4 simulator (iPhone 17):
PostHogMaskFailClosedTestregression tests:window.bounds. Onorigin/main, only the subview rect was masked.PostHogReactNativeMaskingTestcase: a full-window RCTView-like container with a derived label isn't masked whole.make testPresentationMaskspasses (31 tests). That includesnoCaptureAncestorAboveCover, which covers the cover-ancestor path.make lintandmake buildSdkpass.What I didn't run:
make maskSnapshots), because the pinned iOS 26.2 runtime isn't installed locally. No golden scenario usesph-no-capture, so I don't expect them to change, and CI will confirm.📝 Checklist
If releasing new changes
pnpm changesetto generate a changeset file🤖 Agent context
Autonomy: Human-driven (agent-assisted)
RCTRecursiveAccessibilityLabelin RN 0.79 (Paper and Fabric), the sdk-specs replay privacy spec, posthog-android's masking walk, and the posthog.com iOS/RN privacy docs. The docs already say aph-no-captureview is redacted, so they need no change.🤖 Generated with Claude Code