fix(replay): correct platform view mask geometry and capture-failure fallback - #547
Closed
posthog[bot] wants to merge 2 commits into
Closed
fix(replay): correct platform view mask geometry and capture-failure fallback#547posthog[bot] wants to merge 2 commits into
posthog[bot] wants to merge 2 commits into
Conversation
…fallback Three defects in the platform view path of screenshot_capturer.dart: - A revealed view (maskAllPlatformViews = false) turned black whenever the Android native capture returned null, so the disabled mask still hid it. Keep the Flutter pixels and log the failure instead. - The mask rect used unclipped paintBounds, so it spilled past the view onto the widgets below. Intersect it with the ancestor clip chain. - The platform view rects were collected after toImage() and the raw-RGBA await, so a mask could land a frame late. Collect them beside the widget mask walk, before any await. Generated-By: PostHog Desktop Task-Id: b9d8412a-9522-4446-a859-62f7f9fa0118
Contributor
Author
🦔 ReviewHog reviewed this pull requestFound 0 must fix, 1 should fix, 0 consider. Published 1 finding (view the review). Resolved comments: 1 left for you |
Contributor
posthog-flutter Compliance ReportDate: 2026-08-24 15:37:52 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
|
ReviewHog 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
+303
to
+308
| final rect = clippedPaintBounds(ro, ancestor); | ||
| // A view clipped away by an ancestor covers nothing, so it gets no rect. | ||
| if (rect.isEmpty) return; | ||
| final transform = ro.getTransformTo(ancestor); | ||
| final data = ElementData( | ||
| rect: ro.paintBounds, | ||
| rect: rect, |
Contributor
Author
There was a problem hiding this comment.
Clipped capture rect prevents native platform view capture
Why we think it's a valid issue
- Checked: The full request path from the new clipped rect to both native implementations:
_addIfNewatposthog_flutter/lib/src/replay/screenshot/screenshot_capturer.dart:303-311storesclippedPaintBounds(ro, ancestor)inElementData.rect;_viewSpecatscreenshot_capturer.dart:322-332builds thex/y/width/heightpayload from that samerect; line 712-717 sends it tocaptureNativeScreenshots. So the clipped rect, not the full view rect, is what the native side receives. - Found: iOS requires containment.
captureOneNativelooks up the target withfindWKWebView(in: window, containedBy: cropRect)(posthog_flutter/darwin/posthog_flutter/Sources/posthog_flutter/PosthogFlutterPlugin.swift:704), and the predicate isrect.insetBy(dx: -1, dy: -1).contains(frameInWindow)(PosthogFlutterPlugin.swift:786). One point of slack only. A crop rect trimmed by an ancestor clip is smaller than the WKWebView frame, so no web view matches and the function falls through toonResult(nil)atPosthogFlutterPlugin.swift:723. - Found: Android requires containment too.
compositeSurfaceViewsOntoskips any SurfaceView that extends past the destination bitmap:destX + svLogW > destBitmap.width + tolerance || destY + svLogH > destBitmap.height + tolerancewithtolerance = 8(posthog_flutter/android/src/main/kotlin/com/posthog/flutter/PosthogFlutterPlugin.kt:1430-1434).destBitmapis sized from the requestedwidth/height(PosthogFlutterPlugin.kt:1258-1263), so a clip that removes more than 8 logical pixels makes the platform-view SurfaceView fail the test and get dropped from the composite. - Found: The new failure paths in
_compositeRevealedView(screenshot_capturer.dart:346-356) return early on a null or undecodable capture and keep the Flutter pixels. The Flutter pixels do not contain the native view content on these composition modes, which is the reason the native capture exists. - Impact: A revealed platform view that an ancestor clips loses its native content. Concrete trigger: a WKWebView or a SurfaceView-backed view (camera preview, map with
useAndroidViewSurface) inside aClipRect, or partly scrolled inside a viewport, withmaskAllPlatformViews = falseor a per-viewcapturepolicy. Result: iOS getsnilbytes, Android gets a composite without the SurfaceView, and the replay frame shows a blank region where the view is. Before this change the unclipped rect satisfied both containment checks and the content was captured (it then spilled past the clip, which is the bug this PR set out to fix). The two changes in this PR interact: the clipped request rect causes the capture failure, and the new fallback then silently keeps blank Flutter pixels instead of the old black mask, so the breakage is quiet. - Priority: Lowered to
should_fix.maskAllPlatformViewsdefaults totrue(posthog_flutter/lib/src/posthog_config.dart:728), so the affected configuration is opt-in, and the outcome is lost replay fidelity, not a privacy leak, data loss, or a crash. It is still a real regression in the exact scenario the PR targets and should be resolved before the reveal path is advertised as fixed.
Issue description
The code uses the clipped rect for the native capture request. The native implementations require the request rect to contain the full platform view. A clipped WebView or SurfaceView fails this check. The replay then keeps the Flutter pixels, which do not contain the native view on affected composition modes. Revealed clipped views therefore remain blank or stale.
Suggested fix
Keep the full platform view bounds and the visible clipped bounds. Use the full bounds to identify the native view. Capture only the visible intersection, or capture the identified view and crop it safely. Composite the result into the clipped destination rect. Add capture tests for a revealed WebView and SurfaceView inside a ClipRect.
Prompt to fix with AI (copy-paste)
## Context
@posthog_flutter/lib/src/replay/screenshot/screenshot_capturer.dart#L303-308
@posthog_flutter/lib/src/replay/screenshot/screenshot_capturer.dart#L833-859
<issue_description>
The code uses the clipped rect for the native capture request. The native implementations require the request rect to contain the full platform view. A clipped WebView or SurfaceView fails this check. The replay then keeps the Flutter pixels, which do not contain the native view on affected composition modes. Revealed clipped views therefore remain blank or stale.
</issue_description>
<issue_validation>
- **Checked:** The full request path from the new clipped rect to both native implementations: `_addIfNew` at `posthog_flutter/lib/src/replay/screenshot/screenshot_capturer.dart:303-311` stores `clippedPaintBounds(ro, ancestor)` in `ElementData.rect`; `_viewSpec` at `screenshot_capturer.dart:322-332` builds the `x/y/width/height` payload from that same `rect`; line 712-717 sends it to `captureNativeScreenshots`. So the clipped rect, not the full view rect, is what the native side receives.
- **Found:** iOS requires containment. `captureOneNative` looks up the target with `findWKWebView(in: window, containedBy: cropRect)` (`posthog_flutter/darwin/posthog_flutter/Sources/posthog_flutter/PosthogFlutterPlugin.swift:704`), and the predicate is `rect.insetBy(dx: -1, dy: -1).contains(frameInWindow)` (`PosthogFlutterPlugin.swift:786`). One point of slack only. A crop rect trimmed by an ancestor clip is smaller than the WKWebView frame, so no web view matches and the function falls through to `onResult(nil)` at `PosthogFlutterPlugin.swift:723`.
- **Found:** Android requires containment too. `compositeSurfaceViewsOnto` skips any SurfaceView that extends past the destination bitmap: `destX + svLogW > destBitmap.width + tolerance || destY + svLogH > destBitmap.height + tolerance` with `tolerance = 8` (`posthog_flutter/android/src/main/kotlin/com/posthog/flutter/PosthogFlutterPlugin.kt:1430-1434`). `destBitmap` is sized from the requested `width`/`height` (`PosthogFlutterPlugin.kt:1258-1263`), so a clip that removes more than 8 logical pixels makes the platform-view SurfaceView fail the test and get dropped from the composite.
- **Found:** The new failure paths in `_compositeRevealedView` (`screenshot_capturer.dart:346-356`) return early on a null or undecodable capture and keep the Flutter pixels. The Flutter pixels do not contain the native view content on these composition modes, which is the reason the native capture exists.
- **Impact:** A revealed platform view that an ancestor clips loses its native content. Concrete trigger: a WKWebView or a SurfaceView-backed view (camera preview, map with `useAndroidViewSurface`) inside a `ClipRect`, or partly scrolled inside a viewport, with `maskAllPlatformViews = false` or a per-view `capture` policy. Result: iOS gets `nil` bytes, Android gets a composite without the SurfaceView, and the replay frame shows a blank region where the view is. Before this change the unclipped rect satisfied both containment checks and the content was captured (it then spilled past the clip, which is the bug this PR set out to fix). The two changes in this PR interact: the clipped request rect causes the capture failure, and the new fallback then silently keeps blank Flutter pixels instead of the old black mask, so the breakage is quiet.
- **Priority:** Lowered to `should_fix`. `maskAllPlatformViews` defaults to `true` (`posthog_flutter/lib/src/posthog_config.dart:728`), so the affected configuration is opt-in, and the outcome is lost replay fidelity, not a privacy leak, data loss, or a crash. It is still a real regression in the exact scenario the PR targets and should be resolved before the reveal path is advertised as fixed.
</issue_validation>
## Task
Investigate the issue and solve it
<potential_solution>
Keep the full platform view bounds and the visible clipped bounds. Use the full bounds to identify the native view. Capture only the visible intersection, or capture the identified view and crop it safely. Composite the result into the clipped destination rect. Add capture tests for a revealed WebView and SurfaceView inside a ClipRect.
</potential_solution>
Contributor
Author
There was a problem hiding this comment.
Confirmed: this is a real regression this PR introduces, and I'm escalating it for a human rather than fixing it unattended.
The capture path sends the clipped rect to native. _addIfNew stores clippedPaintBounds in ElementData.rect, and revealed (captured) views build their native request from that same rect (_viewSpec → captureNativeScreenshots). But both native sides deliberately require the request rect to contain the full platform view:
- iOS
findWKWebView(in:containedBy:)matches only whencropRect.insetBy(-1,-1)contains the WKWebView frame (1pt slack) — a clip-trimmed rect is smaller than the frame, so nothing matches and it returns nil. This containment rule is intentional: it stops a neighboring masked web view from being snapshotted and leaked. - Android drops any SurfaceView that extends past the destination bitmap (sized from the requested width/height) by more than 8px.
So a revealed platform view inside a ClipRect (or partly scrolled in a viewport) with maskAllPlatformViews = false fails the native capture, and the new capture-failure fallback then keeps the blank Flutter pixels — a quiet loss of the exact content the reveal path exists to show.
What a human needs to decide and verify on-device:
- Carry both rects on
ElementData— the full (unclipped) paint bounds for the native request/identification, and the clipped bounds for masking and for the composite destination. - Pick the capture strategy the suggested fix leaves open — capture only the visible intersection, or capture the identified full view and crop it — and get the source→clipped-destination mapping in
_compositeRevealedViewright so the revealed content lands correctly and still doesn't spill past the clip (the original bug this PR fixed). - Confirm the widened request rect doesn't re-open the leak the native containment check guards against.
Why I'm not doing it here: this environment has no Dart/Flutter toolchain (both flutter and dart are missing), so I can't run flutter analyze or the tests, and the fix's correctness is fundamentally native + visual — WKWebView snapshot geometry, Android SurfaceView compositing, and the on-canvas crop — which can only be validated by running on a real iOS and Android device. It needs the capture tests the reviewer asked for (revealed WebView and SurfaceView inside a ClipRect), exercised in CI/on-device.
The unclipped-view case forced the view to the ancestor's tight size, so it measured 300x300 instead of 200x200. Loosen it with a Center. Also wrap the render-object lookups to satisfy dart format. Generated-By: PostHog Desktop Task-Id: b9d8412a-9522-4446-a859-62f7f9fa0118
Comment on lines
+605
to
+614
| // Collect the platform view rects in the same frame as the widget mask | ||
| // rects, before any await. Collecting them after toImage() lets the UI | ||
| // move first, so a mask lands a frame late over the wrong pixels. | ||
| final defaultPolicy = replayConfig.maskAllPlatformViews | ||
| ? PostHogPlatformViewPrivacy.mask | ||
| : PostHogPlatformViewPrivacy.capture; | ||
| final pvRects = _collectPlatformViewRects(defaultPolicy); | ||
| final hasCapturedViews = pvRects.captured.isNotEmpty; | ||
| hasCapturedPlatformViews = hasCapturedViews; | ||
|
|
Contributor
Author
There was a problem hiding this comment.
Early rect collection makes native capture requests stale
Why we think it's a valid issue
- Checked: the diff between the merge base and PR head
7706ebe, the full await sequence in the capture body ofscreenshot_capturer.dart,_viewSpec,ElementData, and both native capture handlers. - Found: before this PR,
_collectPlatformViewRectsran after_getImageBytes, and noawaitsat between it andcaptureNativeScreenshots. The work in between is synchronous (_computeImageHash,canvas.drawImage,_imageMaskPainter.drawMaskedImage). Dart runs on one thread, so no new frame could run in that window. - Found: the PR moves the collection to
posthog_flutter/lib/src/replay/screenshot/screenshot_capturer.dart:611, ahead ofawait renderObject.toImage(...)at:615andawait _getImageBytes(...)at:643.captureNativeScreenshots(specs)now runs at:717, two awaits later. A raw-RGBAtoByteDataof a full-screen image costs many milliseconds, so the scheduler produces one or more frames in that window. The gap is new, and this diff creates it. - Found:
_viewSpecatscreenshot_capturer.dart:319-329builds the native request only from the cachedElementData.rectandtransform.ElementData(posthog_flutter/lib/src/replay/element_parsers/element_data.dart:4) holds no RenderObject, so the spec cannot be refreshed at request time. - Impact (iOS):
findWKWebView(in:containedBy:)atposthog_flutter/darwin/posthog_flutter/Sources/posthog_flutter/PosthogFlutterPlugin.swift:782-786needs the live frame of the web view to fit inside the crop rect, with only 1 pt of slack. A scroll during the two awaits breaks that test, so the plugin returns nil. The other change in this PR then keeps the Flutter pixels for the revealed view, and a hybrid-composition web view contributes no Flutter pixels. The region shows blank. - Impact (Android):
captureOneNativecrops the live surface at the stale coordinates (posthog_flutter/android/src/main/kotlin/com/posthog/flutter/PosthogFlutterPlugin.kt:1256-1288). The crop shifts by the scroll delta of one to three frames. The composite runs atscreenshot_capturer.dart:727, after the mask pass, so it draws last. A stale crop can therefore paint unmasked neighbour pixels over a mask that the earlier pass applied. - Impact (reach): only the revealed path is affected (
maskAllPlatformViewsdefaults to true atposthog_flutter/lib/src/posthog_config.dart:728). That path is opt-in, but it is the exact path this PR sets out to repair, and scrolling a map or a web view is the normal way to use it.
Issue description
The code collects captured platform view rects before toImage() and the raw RGBA conversion. The native capture runs after both asynchronous operations. An animation or scroll can move the native view during this interval. The request then uses the old position. iOS can reject the request because the old rect no longer contains the view. Android can capture the old screen region. The final replay can show blank, stale, or unrelated native content.
Suggested fix
Separate mask geometry from native capture geometry. Keep the frame-aligned rects for masking and final placement. Refresh each native view's screen bounds immediately before captureNativeScreenshots. Map each result back to its frame-aligned destination. Drop the frame if the view identity or geometry cannot be matched safely. Add a test that moves a revealed platform view while capture is pending.
Prompt to fix with AI (copy-paste)
## Context
@posthog_flutter/lib/src/replay/screenshot/screenshot_capturer.dart#L605-614
<issue_description>
The code collects captured platform view rects before `toImage()` and the raw RGBA conversion. The native capture runs after both asynchronous operations. An animation or scroll can move the native view during this interval. The request then uses the old position. iOS can reject the request because the old rect no longer contains the view. Android can capture the old screen region. The final replay can show blank, stale, or unrelated native content.
</issue_description>
<issue_validation>
- **Checked:** the diff between the merge base and PR head `7706ebe`, the full await sequence in the capture body of `screenshot_capturer.dart`, `_viewSpec`, `ElementData`, and both native capture handlers.
- **Found:** before this PR, `_collectPlatformViewRects` ran after `_getImageBytes`, and no `await` sat between it and `captureNativeScreenshots`. The work in between is synchronous (`_computeImageHash`, `canvas.drawImage`, `_imageMaskPainter.drawMaskedImage`). Dart runs on one thread, so no new frame could run in that window.
- **Found:** the PR moves the collection to `posthog_flutter/lib/src/replay/screenshot/screenshot_capturer.dart:611`, ahead of `await renderObject.toImage(...)` at `:615` and `await _getImageBytes(...)` at `:643`. `captureNativeScreenshots(specs)` now runs at `:717`, two awaits later. A raw-RGBA `toByteData` of a full-screen image costs many milliseconds, so the scheduler produces one or more frames in that window. The gap is new, and this diff creates it.
- **Found:** `_viewSpec` at `screenshot_capturer.dart:319-329` builds the native request only from the cached `ElementData.rect` and `transform`. `ElementData` (`posthog_flutter/lib/src/replay/element_parsers/element_data.dart:4`) holds no RenderObject, so the spec cannot be refreshed at request time.
- **Impact (iOS):** `findWKWebView(in:containedBy:)` at `posthog_flutter/darwin/posthog_flutter/Sources/posthog_flutter/PosthogFlutterPlugin.swift:782-786` needs the live frame of the web view to fit inside the crop rect, with only 1 pt of slack. A scroll during the two awaits breaks that test, so the plugin returns nil. The other change in this PR then keeps the Flutter pixels for the revealed view, and a hybrid-composition web view contributes no Flutter pixels. The region shows blank.
- **Impact (Android):** `captureOneNative` crops the live surface at the stale coordinates (`posthog_flutter/android/src/main/kotlin/com/posthog/flutter/PosthogFlutterPlugin.kt:1256-1288`). The crop shifts by the scroll delta of one to three frames. The composite runs at `screenshot_capturer.dart:727`, after the mask pass, so it draws last. A stale crop can therefore paint unmasked neighbour pixels over a mask that the earlier pass applied.
- **Impact (reach):** only the revealed path is affected (`maskAllPlatformViews` defaults to true at `posthog_flutter/lib/src/posthog_config.dart:728`). That path is opt-in, but it is the exact path this PR sets out to repair, and scrolling a map or a web view is the normal way to use it.
</issue_validation>
## Task
Investigate the issue and solve it
<potential_solution>
Separate mask geometry from native capture geometry. Keep the frame-aligned rects for masking and final placement. Refresh each native view's screen bounds immediately before `captureNativeScreenshots`. Map each result back to its frame-aligned destination. Drop the frame if the view identity or geometry cannot be matched safely. Add a test that moves a revealed platform view while capture is pending.
</potential_solution>
Contributor
Author
There was a problem hiding this comment.
This is a real tradeoff introduced by the frame-alignment fix, and I'm escalating it rather than fixing it unattended.
Confirmed on the current head: the revealed platform-view rects are collected before toImage() and the raw-RGBA conversion, but the native capture request is issued two awaits later, so a scroll/animation in that window makes the native crop coordinates stale — which matches the iOS (crop-rejection → blank region on a hybrid-composition web view) and Android (offset crop painting over the mask) failure modes you describe.
The reason this isn't a safe autonomous fix:
- The mask/placement rects must stay frame-aligned (they composite onto the frame-N Flutter image already drawn on the canvas), so you can't simply collect late — that would reintroduce the masking-frame-late privacy leak this PR is fixing. The fix therefore has to split geometry: frame-aligned rects for masking and final placement, plus a freshly re-queried native-capture geometry taken right before
captureNativeScreenshots, with each native result mapped back to its placement rect and the frame dropped if identity/geometry can't be matched._viewSpecandElementData(which carries no RenderObject today) can't express that without a structural change. - The correctness of that change is native-timing behavior on real iOS/Android platform views under scroll — it can't be proven by reading code or the existing Dart tests in this run, and this PR couldn't run the Flutter toolchain at all.
What a human needs to decide: whether to take on the dual-geometry refresh (retain RenderObjects / re-walk to re-query bounds at capture time, match results back to placement rects, drop on mismatch), and to validate it on-device against a scrolling map/WebView — the exact E2E this autonomous run can't perform.
Contributor
|
closing in favor of #553 |
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
1 participant
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
Session replay masking is broken on any Flutter screen that embeds a platform view — a map, WebView, or camera preview. Customers cannot record these screens and there is no workaround (turning off every mask flag,
PostHogPlatformView, andClipRectall leave the geometry untouched).The original privacy leak (masked content legible on a paused fast scroll) shipped in 5.36.4. This PR fixes the three remaining defects in the platform view path of
screenshot_capturer.dart:maskAllPlatformViews = false) painted a solid black mask whenever the Android native capture returned null — so the mask the user turned off still hid the view.paintBoundswith no ancestor-clip intersection, so it overran the view and covered unrelated UI below it.toImage()and the raw-RGBA await, so they could come from a later frame than the pixels.💚 How did you test it?
platform_view_clip_test.dartcoveringclippedPaintBounds: an unclipped view keeps its full paint bounds, and aClipRectancestor trims the view to its visible region.flutter analyzewere not executed locally — they need to run in CI. Fixes 1 and 3 (capture-failure fallback, collection ordering) were verified by reading the code, not exercised at runtime.📝 Checklist
If releasing new changes
pnpm changesetto generate a changeset file🤖 Agent context
Autonomy: Fully autonomous
clippedPaintBoundswalks from the view up to the screenshot container, and for each clipping ancestor maps itsdescribeApproximatePaintClipinto the view's frame and intersects. This mirrors how Flutter's own paint pipeline reasons about clips, rather than reimplementing per-clip-type geometry._viewSpecnow derives the native capture size from the clipped rect, so the native screenshot request matches the visible region.flutter test/flutter analyze— no toolchain in the environment. Left to CI.Created with PostHog Desktop from this inbox report.