feat(replay): PostHogMaskWidget enables web canvas masking on its own - #501
feat(replay): PostHogMaskWidget enables web canvas masking on its own#501turnipdabeets wants to merge 10 commits into
Conversation
posthog-flutter Compliance ReportDate: 2026-07-28 19:24:00 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
|
5c00fc8 to
93e3d0d
Compare
688652a to
035e3f1
Compare
93e3d0d to
8ba79e4
Compare
035e3f1 to
2010638
Compare
8ba79e4 to
393cdd5
Compare
2010638 to
01e3715
Compare
Canvas masking only registered with posthog-js when the app declared session_recording.captureCanvas.canvasMaskRegionsFn in its posthog.init call, so a developer who wrapped sensitive UI in PostHogMaskWidget and never touched web/index.html got no masking at all, silently — while the same widget needs no setup on iOS and Android. The first PostHogMaskWidget to mount now opts the app in: the mount is routed through a conditional import (no-op off web) to WebCanvasMaskProvider, which registers the mask-region provider if it has not already. Registration stays idempotent, so any number of mask widgets produce at most one set_config and one recording restart. The gate is kept for everyone else: apps with neither a PostHogMaskWidget nor the init declaration still see no set_config, no blockSelector and no restart. Registering mid-session restarts an in-flight recording (posthog-js reads canvas capture options only when recording starts) and does not cover frames captured before the first mount — declaring canvasMaskRegionsFn in posthog.init remains the only way to cover the pre-boot window. Also drops a stale caveat from the example app: test 15's title claimed PostHogMaskWidget with multiple children needs maskAllTexts or maskAllImages. It does not on either platform — getMaskElements calls extractMaskWidgetRects() unconditionally on web, and screenshot_capturer calls getPostHogWidgetWrapperElements() outside the flags branch on mobile. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HPiKky15SKqNg9PT2wHeaw
393cdd5 to
fc3a116
Compare
…register-fixes # Conflicts: # posthog_flutter/lib/src/replay/web/web_canvas_mask_provider.dart # posthog_flutter/test/web_canvas_mask_provider_test.dart
… document both opt-in paths A throw while applying the mount opt-in now schedules a retry chain instead of permanently consuming the opt-in. The not-opted-in warnings mention mounting a PostHogMaskWidget as the easier fix, both changesets describe the two opt-in paths (and that one mask widget enables the whole masking config, maskAllTexts/maskAllImages included), and tests cover cross-path idempotency plus the failed-mount retry. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…g.canvasCapture.maskRegionsFn Also renames the old path in this branch's own additions (mount opt-in warnings, dartdoc, changeset, tests). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…vas stills Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Resolves the provider conflict by keeping the mount state machine alongside the version gate, and adds a test that the warning also fires on the mount-triggered apply path. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
🦔 ReviewHog reviewed this pull requestFound 0 must fix, 0 should fix, 1 consider. Published 1 finding (view the review). |
set_config landing but the restart throwing left _applied latched, so no later pump retried the restart and blockSelector never took effect for the in-flight recording.
|
ReviewHog Alpha 🦔 If you find any issues helpful - please reply "valid", "invalid", etc., for evaluation purposes 🙏 |
There was a problem hiding this comment.
ReviewHog Report
Changes
Issues: 1 issue
Files (7)
.changeset/canvas-masking-web.md.changeset/mask-widget-web-canvas.mdexample/lib/masking_tests_screen.dartposthog_flutter/lib/src/replay/mask/canvas_mask_registration_io.dartposthog_flutter/lib/src/replay/mask/canvas_mask_registration_web.dartposthog_flutter/lib/src/replay/mask/posthog_mask_widget.dartposthog_flutter/lib/src/replay/web/web_canvas_mask_provider.dart
Other findings (outside the changed lines)
Valid issues on this PR's files that sit on lines GitHub won't let us comment on inline.
Retry loop never backs off when _apply() keeps throwing, unlike the posthog-not-ready path
Priority: consider | File: posthog_flutter/lib/src/replay/web/web_canvas_mask_provider.dart:161-178 | Category: performance
Why we think it's a valid issue
- Checked: Traced
_scheduleRetry(L164-181) and every_apply()throw source (set_configL248,stopSessionRecording/startSessionRecordingL255-256), plus the paths that enter the chain with an exception in flight (_onMaskWidgetMountedcatch L140-141,registercatch L120-121). - Found: The code claim is accurate:
nextis seeded todelay(L166) and only ramped inside thetryon aposthogNotReadyresult (L172-175); a thrown_apply()skips tocatch(L176-178) leavingnext == delay, so_scheduleRetry(next)(L179) reschedules at the same interval. TheposthogNotReadypath deliberately backs off to a 4s ceiling (comment L161-163: 'polls forever once backed off to 4s'); the exception path does not. - Found (reachability): A true infinite fixed-250ms loop requires
set_configorstopSessionRecordingto throw on every tick — if onlystartSessionRecordingthrows, the next tick seessessionRecordingStarted()==false(L254), skips the restart, sets_applied=true(L260), and the chain terminates in ~2 ticks. - Impact: Confirmed but bounded: under a persistent
set_config/stopSessionRecordingthrow the chain re-runs the apply at 4Hz for the page's life, producingprintIfDebugspam (L177) and wasted interop calls; recording is left running in both loop cases (no crash/hang/data-loss), and it only bites in an already-degenerate posthog-js state. - Priority: Real, verified inconsistency with a one-line fix, but a rare/degenerate trigger and bounded (log-spam / wasted-cycles) impact — real-but-minor, so down-ranked from should_fix to consider rather than surfaced at higher urgency.
Issue description
This chunk adds a new, expected way to enter the shared _scheduleRetry chain with a genuine exception in flight: _onMaskWidgetMounted()'s catch block explicitly schedules a retry (_polling = true; _scheduleRetry(const Duration(milliseconds: 250));) when _apply() throws mid-way (e.g. set_config or stopSessionRecording/startSessionRecording failing). But _scheduleRetry's Timer callback only advances the backoff delay on the success/not-ready branch: next is initialized to the current delay and is only reassigned to doubled/capped-at-4s inside the try block, right before the early return. When _apply() throws again on that retry tick, execution jumps straight to catch (e) { printIfDebug(...); }, next is never touched, and _scheduleRetry(next) reschedules at the exact same delay it started with. Contrast this with the posthogNotReady path, which the adjacent comment explicitly says 'polls forever once backed off to 4s' to avoid hammering anything while waiting for a slow app boot — that reasoning was never extended to the exception path this PR newly makes reachable. If an app hits a persistent failure after a PostHogMaskWidget mounts (e.g. stopSessionRecording()/startSessionRecording() throwing every time due to a broken posthog-js state, a hostile monkey-patch, or a CSP/extension interference that's not transient), the SDK will retry the full _apply() — including set_config and a stopSessionRecording/startSessionRecording pair — every 250ms for the rest of the page session, since nothing ever raises the delay past its starting value. None of the existing tests ('an exception during one retry tick does not kill the chain' and 'retries the full apply when the restart throws on first register') exercise a repeated/persistent throw — both stub the failure to occur exactly once and succeed on the second attempt, so this fixed-interval-forever behavior has no test coverage and was not caught by any prior pass.
Suggested fix
Compute the backed-off next delay unconditionally (before or independent of the try/catch), so a repeated exception still ramps the interval up toward the same 4s ceiling used for the not-ready case, e.g. move the doubled/next calculation above the try, or wrap only the _apply() call itself in the try and always fall through to the backoff computation afterward.
| } | ||
| _maskWidgetMounted = true; | ||
| // a retry chain still in flight picks the flag up on its next tick | ||
| if (_applied || _polling) { |
There was a problem hiding this comment.
When this apply throws, the retry is scheduled without setting _polling = true. If a PostHogMaskWidget mounts before that timer fires, it passes the _polling guard and starts a second retry chain. Since only the latest timer is retained, a later setup cancels one chain while the sibling can survive and reapply the old provider/config. I reproduced this sequence locally. Could we mark polling active here and enforce at most one outstanding retry timer?
| @override | ||
| void initState() { | ||
| super.initState(); | ||
| notifyMaskWidgetMounted(); |
There was a problem hiding this comment.
This globally opts in without verifying that the mounted mask widget belongs to the PostHogWidget subtree used by PostHogMaskController. If a different PostHogWidget exists as a sibling, the walk succeeds on that other subtree and returns an empty/incomplete region list instead of failing closed, leaving this explicitly masked widget exposed. I reproduced the sibling-tree case locally. Could we validate the triggering widget's ancestry or otherwise fail closed when it is outside the tracked tree?
| return true; | ||
| // only after the restart: a throw above must leave the apply retryable, | ||
| // or the selector never takes effect for this recording | ||
| _applied = true; |
There was a problem hiding this comment.
If stopSessionRecording() succeeds but startSessionRecording() throws, the retry sees recording as stopped, skips this block, and marks the provider applied, leaving recording permanently stopped. The current test only throws from stopSessionRecording() and its stub does not model recording-state transitions. Could we retain a pending-restart state and retry the start independently?
💡 Motivation and Context
#499 gives Flutter web canvas masking, but it only registers the mask-region provider with posthog-js when the app already declared
session_recording.canvasCapture.maskRegionsFnin itsposthog.initcall.That leaves a footgun: a developer wraps sensitive UI in
PostHogMaskWidget, does not also editweb/index.html, and gets zero masking on web, silently.PostHogMaskWidgetis the most explicit possible statement of "never record this", and it needs no setup at all on iOS and Android — so web quietly diverging is the worst possible failure mode.The gate itself is not wrong. Registering does two things beyond attaching a function: it adds
blockSelector: 'flt-semantics-host'(so Flutter's accessibility DOM is not recorded in the clear), and it restarts an in-flight recording, because posthog-js only reads those options when rrweb'srecord()starts. Imposing that on every Flutter web app — including ones that never asked for masking — is not acceptable.So this PR adds a second way to opt in rather than removing the gate: a mounted
PostHogMaskWidgetis itself the opt-in.🔨 What changed
PostHogMaskWidgetState.initStatecallsnotifyMaskWidgetMounted(), resolved through a conditional import (canvas_mask_registration_io.dart/..._web.dart). Off web it is an empty function body, so no web code reaches mobile builds and nothing changes on iOS/Android/macOS.WebCanvasMaskProvider.notifyMaskWidgetMounted(). Deferring matters: registration restarts the recording, and rrweb can take a canvas snapshot from inside that call — which would walk a widget tree that is still being built.WebCanvasMaskProviderkeeps the same "did the app declare it?" gate, but now also applies when a mask widget has mounted. The retry/apply path became a small state machine (applied/notOptedIn/posthogNotReady) so that "posthog-js is up but the app did not opt in" parks instead of terminating — aPostHogMaskWidgetmounting later re-enters it. Registration is idempotent: at most oneset_configand at most one recording restart, however many mask widgets mount, and whether posthog-js loads before or after Flutter.Apps with no
PostHogMaskWidgetand no declaration are still completely untouched: noset_config, noblockSelector, no restart.Also drops a stale caveat from the example app: test 15's section title claimed
PostHogMaskWidgetwith multiple children "needs maskAllTexts or maskAllImages". It does not, on either platform —getMaskElementscallsextractMaskWidgetRects()unconditionally on web, andscreenshot_capturercallsgetPostHogWidgetWrapperElements()outside the flags branch on mobile (post-#500). A browser check against the built app onfeat/web-canvas-maskingwithmaskAllTexts: false, maskAllImages: falseconfirmed it: the provider returned a full-width rect covering the whole Row, amber box included.These are documented in the CHANGELOG and in the
PostHogMaskWidgetdartdoc:blockSelectoris a start-time option.maskRegionsFn: () => nullinposthog.initis still the only thing that covers the window between page load and Flutter booting (those frames are skipped instead), and it also avoids the restart. The HTML setup is now an optimization, not a requirement.PostHogWidgetis still required. An app that mounts aPostHogMaskWidgetbut is not wrapped inPostHogWidgetnow fails closed — canvas frames are skipped rather than recorded unmasked, with a console warning explaining the fix. That is the correct behavior for something that explicitly asked for masking, but it is a behavior change for that (misconfigured) shape of app.By design: registration does not wait on
recordCanvasRegistration opts in without first checking that canvas capture is actually enabled. That is deliberate, for two reasons:
recordCanvascan arrive from remote config afterposthog.init, so aph.configcheck at registration time can read "canvas off" for a session that goes on to record the canvas — masking would silently never attach.blockSelector: 'flt-semantics-host', and that exclusion is independent of the canvas: with accessibility active, Flutter mirrors widget text intoflt-semanticsDOM nodes, which rrweb records as plaintext during ordinary DOM recording. So a canvas-off app that mounts aPostHogMaskWidgetstill gains a real thing — the a11y text side-channel is closed.The restart is gated on
ph.sessionRecordingStarted()regardless, so an app that is not recording pays nothing at all.💚 How did you test it?
Three new tests in
test/web_canvas_mask_provider_test.dart(already listed in the "Test (web)" CI job, so no workflow change needed):PostHogMaskWidgetopts the app in whenposthog.initdeclared nothing — provider attached,blockSelectorset, recording restarted once;set_configand one restart across two pumps with three and then five mask widgets;A follow-up commit (review fixes, merged with the updated #499 base) adds two more: cross-path idempotency (app opted in via the init declaration, then a mask widget mounts — still exactly one
set_configand one restart), and aset_configthrow during the mount-triggered apply being retried and applied on a later tick instead of permanently consuming the opt-in.The pre-existing
leaves posthog-js untouched when the app declares no mask providertest is unchanged and still passing, which is the assertion that apps opting into nothing stay untouched.Ran locally, all green:
flutter test— 242 passingflutter test --platform chrome test/posthog_flutter_web_handler_test.dart test/web_canvas_mask_provider_test.dart— 23 passingflutter analyze— no issuesdart format --output=none --set-exit-if-changed ./— exit 0The registration change itself was not manually verified in a browser against a real posthog-js build; the posthog-js side of the contract is exercised by the stub, as in #499. The test 15 measurement quoted above was taken by hand on the base branch, not by the agent.
📝 Checklist
If releasing new changes
## Nextupdated alongside feat(replay): mask canvas session replay recordings on Flutter web #499's entry (that entry's claim thatPostHogMaskWidgethas no effect on web is now false and has been corrected). No separate changeset, matching feat(replay): mask canvas session replay recordings on Flutter web #499.🤖 Agent context
Autonomy: Human-driven (agent-assisted)
Written with Claude Code (Opus 5). The design question was how to let shared widget code trigger web-only registration without leaking
dart:js_interopinto mobile builds; the repo's existing_io.dart/_web.dartconditional-import convention (seeorigin.dart,chunk_ids.dart) handles it, so the mount hook is a one-line no-op function on every non-web platform.Two alternatives were rejected: removing the gate outright (would restart recording and drop the semantics tree for every Flutter web app, including ones that never asked for masking), and gating registration on
recordCanvasbeing visibly enabled inph.config(fails open when canvas capture arrives via remote config, and would forfeit the semantics exclusion for canvas-off apps — see "By design" above).Deferring the hook to a post-frame callback was a deliberate change after the first working version called it synchronously from
initState— registering synchronously means posthog-js may re-enter the mask-region provider mid-build, which walks the render tree.