Repository navigation
chore!: remove wireframe session replay capture - #908
turnipdabeets wants to merge 4 commits into
Conversation
Session replay always records masked screenshots, the mode `screenshot = true` already used. Remove `PostHogSessionReplayConfig.screenshot`, and `drawableConverter` / `PostHogDrawableConverter`, which only the wireframe builder read. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
![posthog[bot]](https://avatars.githubusercontent.com/in/1203049?s=60&v=4){kind=small}
There was a problem hiding this comment.
PostHog Review · Standard · reviewed 697f9c0: 1 Should fix.
This is the only automatic review while this pull request is a draft. Comment @posthog review to review new changes sooner. Otherwise, a review of the new changes runs when you mark it ready for review.
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.
![veria-ai[bot]](https://avatars.githubusercontent.com/in/1723776?s=60&v=4){kind=small}
PR overviewAll previously flagged issues have been addressed. No open security concerns remain on this pull request. Security reviewNo open security issues remain on this pull request. Fixed/addressed: 1 · PR risk: 0/10 |
With wireframe capture gone, each window sends a single screenshot node, so the tree fields, RRStyle and the child-flattening diff are unused. Screenshot frames no longer carry an empty `style`, matching iOS 4.0. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
PostHog Review · Standard · reviewed 37df5a2: 1 Must fix.
This is review round 3 on this pull request. When findings keep coming back in the same code, the approach is usually the problem. Before the next fix, check whether a different design would remove the whole class of findings.
| val wireframe = | ||
| if (useScreenshot) { | ||
| view.toScreenshotWireframe( | ||
| window, | ||
| status.drawState, | ||
| ) ?: return false | ||
| } else { | ||
| view.toWireframe() ?: return false | ||
| } | ||
|
|
||
| // if the decorView has no backgroundColor, we use the theme color | ||
| // no need to do this if we are capturing a screenshot | ||
| if (wireframe.style?.backgroundColor == null && !useScreenshot) { | ||
| context.theme?.toRGBColor()?.let { | ||
| wireframe.style?.backgroundColor = it | ||
| } | ||
| } | ||
| view.toScreenshotWireframe( | ||
| window, | ||
| status.drawState, | ||
| ) ?: return false |
There was a problem hiding this comment.
P1 · Mask Compose images without accessibility descriptions before capturing pixels
With default replay settings, Compose Image(contentDescription = null) now uploads its pixels despite maskAllImages = true. Compose omits semantics for these images, and findMaskableComposeWidgets identifies images only through ContentDescription; wireframes previously omitted their pixels. Mask these images or conservatively redact their Compose region.
There was a problem hiding this comment.
[P1] Mask Compose images independently of accessibility descriptions
Relationship: Existing screenshot defect exposed by default Compose pixel capture
Underlying cause: PostHogReplayIntegration.kt:1278 and 1304–1305
With replay enabled and maskAllImages = true, an ordinary bitmap-backed Compose image is emitted without a mask:
Image(
bitmap = bitmap.asImageBitmap(),
contentDescription = null,
modifier = Modifier.size(100.dp),
)Expected: mask the image or conservatively suppress unsafe capture—not treat absent accessibility metadata as an unmask instruction.
Image discovery relies solely on SemanticsProperties.ContentDescription. The actual null-description image in the regression supplies neither that property nor Role. Discovery succeeds without poisoning but omits the measured image rectangle. A Role.Image check alone would therefore not fix this case.
Executed regression: Real Compose activity, actual decorated-window capture, Compose 1.11.4 and Robolectric API 35:
| Case | Base default | Reviewed head |
|---|---|---|
| Null description, image masking enabled | PASS; no image acquisition | FAIL; image-region marker remains unmasked |
| Non-null description, masking enabled | PASS | PASS; masked |
| Non-null description, masking disabled | PASS | PASS; intentionally unmasked |
Null description plus postHogMask() |
PASS | PASS; masked |
The image bounds were (0,56)–(100,156). Walks completed without poison. The three controls isolate image discovery rather than general Compose setup or masking failure.
This contradicts the local maskAllImages option and canonical privacy Behavior 8. Android’s semantics documentation explains that semantics describes meaning rather than drawing and may omit composables. Exact version behavior was verified through the executed public Image API.
Consider reliable image discovery or a conservative masking fallback for Compose regions whose image content cannot be enumerated. Retain all four controls. Explicit postHogMask() is a supported, tested workaround, not a replacement for automatic image masking.
The base default wireframe path did not acquire Compose image pixels. Screenshot-only capture therefore exposes content existing default replay did not upload; this is not attributed to the latest masking fixes or DTO trim.
Validation limits: Controlled PixelCopy acquisition exercised production mask discovery, masking, encoding and fake-core capture against the actual window.decorView. This does not establish physical-device acquisition, literal secret transmission, real HTTP/backend delivery or player rendering.
There was a problem hiding this comment.
Fixing on main in #898: with maskAllImages, the Compose mask pass walks the layout tree and masks painter-backed nodes, so it doesn't rely on ContentDescription or Role. It covers the contentDescription = null case and keeps the vector, postHogUnmask() and masking-off cases unmasked. It reaches 4.0 when main is merged into the 4.0 branches.
dustinbyrne
left a comment
dustinbyrne
left a comment
There was a problem hiding this comment.
The animated-view and idle surface-scan fixes are verified. Two default screenshot privacy gaps remain: overflowing descendants of masked containers and Compose images without descriptions. Details are inline and in the existing Compose-image discussion.
AI-assisted review.
| wireframe.style?.backgroundColor = it | ||
| } | ||
| } | ||
| view.toScreenshotWireframe( |
There was a problem hiding this comment.
[P1] Preserve descendant masking beneath an unclipped masked container
Relationship: Existing screenshot defect exposed by the default-mode transition
Underlying cause: PostHogReplayIntegration.kt:1075–1077
With replay enabled and default maskAllTextInputs = true, adding ph-no-capture to an unclipped container can expose its overflowing child text. Expected: retain the container mask and cover the matching descendants.
A supported example:
val child = TextView(context).apply { text = "secret" }
val wrapper = FrameLayout(context).apply {
tag = "ph-no-capture"
clipChildren = false
clipToPadding = false
addView(child)
}
// Attached hierarchy, with the child visibly outside its parent:
wrapper.layout(0, 0, 100, 20)
child.layout(0, 30, 100, 50)findMaskableWidgets() records only the tagged container’s rectangle and stops descending. The child consequently loses the automatic text mask it receives without the parent tag.
Executed regression:
- Native Canvas verifies that the child actually paints outside the wrapper.
- Layout is settled and ancestors are attached/visible.
- Wrapper rectangle:
(0,56)–(100,76). - Child rectangle:
(0,86)–(100,106). - Base default serializes
"******", not"secret". - Head fails in both alignment-verification modes: the child-region marker remains unmasked.
- Removing only the wrapper tag makes the head control pass.
This is neither an animation race nor unrecognized custom drawing. The new poison rejection does not help because the stable tagged parent prevents descendant discovery.
The canonical privacy contract explicitly requires full matched-subtree discovery and the wrapper rectangle alongside matching descendant rectangles: Behavior 4 and the multiple-descendant scenario.
Consider retaining the wrapper mask while continuing descendant coverage, including visible overflow and inherited masking where necessary, while preserving supported unmask precedence. Retain the tagged, verified-tagged and untagged controls.
The screenshot limitation predates this PR, but the base default protected this concrete case. Making screenshots mandatory exposes it to existing replay-enabled applications.
Validation limits: Executed under Robolectric API 26 against the actual window.decorView, with controlled PixelCopy acquisition and production masking, encoding and fake-core capture. Native Canvas independently verifies the overflow rendering. This does not establish physical-device acquisition, literal secret transmission, real HTTP/backend delivery or player rendering.
There was a problem hiding this comment.
Fixing on main in #914: a ph-no-capture view and its visible descendants are all masked now, and ph-no-mask descendants still take precedence. It reaches 4.0 when main is merged into the 4.0 branches.
💡 Motivation and Context
Part of the 4.0 stack, based on #871 (
chore/v4-remove-deprecated). Ioannis asked to drop wireframe on Android too, matching posthog-ios 4.0 (PostHog/posthog-ios#907, then PostHog/posthog-ios#973 removedscreenshotModeoutright).Android session replay has two capture modes. Wireframe is the default (
screenshot = false). It serializes the View tree to JSON. Screenshot captures masked images. This PR removes wireframe, so replay always captures masked screenshots.Why:
ph-no-capturecontainers. sdk-specscompliance/posthog-android.mdn26 marks this:toWireframe()never checksisNoCapture()for a tagged container, so its children's text is still serialized in the default mode. The screenshot path masks a tagged view as one rect.ComposeViewwas one box with a background color. The screenshot path masks Compose through semantics.What changes
childWireframes,style, text masked with*)screenshotwireframe with maskedbase64, the same payloadscreenshot = truealready sentPostHogSessionReplayConfig.screenshotdrawableConverter/PostHogDrawableConverter$sdk_debug_replay_capture_modewireframe/screenshotscreenshotThe full snapshot and meta events are the same as what screenshot mode sends today, and the meta
href(window title) is unchanged. The player already handles both formats, so the server doesn't need any change.Public API: removing the option, not deprecating it as a no-op, follows PostHog/posthog-ios#973. An app that set
screenshot = falseto keep pixels out of recordings now gets a compile error instead of silently uploading screenshots, so it has to choose: mask what's sensitive (ph-no-capture,Modifier.postHogMask(),maskAllImages/maskAllTextInputs) or turn replay off.Core
RRWireframetrimmed (e28cf3d4): it keeps onlyid,x,y,width,height,typeandbase64, matching iOS 4.0, andRRStyleis deleted. Each window sends one screenshot node, so the child-flattening tree diff becomes a single-node compare (update when the image or size changes). Screenshot frames no longer send an emptystyle: {}, the same shape iOS 4.0 sends. posthog-flutter'sSnapshotSenderpassedstyle = RRStyle(), so it needs PostHog/posthog-flutter#684.Deleted as orphaned:
toWireframe()and its helpers (Drawable.base64/toRGBColor/toBitmap/copy,String.mask(), the theme background lookup) and theforceScreenshotparameter ongenerateSnapshot.For standard widgets the masking decision is unchanged: both modes used the same
shouldMask*/isTextInputSensitivehelpers. What changes is everything else:onDraw, maps, video, charts). Screenshots render the pixels, and masking only covers recognized widgets plusph-no-capture/Modifier.postHogMask().Downstream PRs
SessionRecordingConfig.screenshot(stacked on chore!: adopt posthog-android 4.0, core 7.0 and posthog-ios 4.0 posthog-kmp#108). It no longer compiles against this branch without that change.sessionReplayConfig.screenshot = true(stacked on chore(react-native-plugin)!: require posthog-android 4.0 posthog-js#5476).screenshot, and its Android replay already reportsscreenshot. The Gradle plugin doesn't touch replay.Follow-ups (not in this PR)
posthog-react-native-session-replayrepo also setsscreenshot = true, but it's still on posthog-android 3.x.💚 How did you test it?
:posthog-android:testDebugUnitTestandtestWithoutCompose: pass.PostHogReplayIntegrationTestis 133/133.screenshot = truenow get the same thing from the default config.debugPropertiestests are now one test, which checks that the default config reportsscreenshot.batch-request.jsonevent-shape snapshot changes only on that one property.spotlessCheck,apiCheckand the sample app build pass.posthog7.0.0 /posthog-android4.0.0 from this branch to a local Maven repo and built KMP's downstream PR against it: config tests pass, the sample builds.📝 Checklist
If releasing new changes
pnpm changesetto generate a changeset file (added a bullet to the existing.changeset/android-v4-major.md)🤖 Agent context
Autonomy: Human-driven (agent-assisted)
drawableConverter, which was never deprecated, because without wireframe it does nothing. Easy to put back as a deprecated no-op if we'd rather.RRWireframe/RRStyleunchanged so posthog-flutter keeps compiling.🤖 Generated with Claude Code