fix(react-native): respect ph-no-capture above maxElementsCaptured - #4649
Merged
Conversation
12 tasks
maxElementsCaptured now caps only the emitted payload; the walk continues so a ph-no-capture ancestor above the cap is still honoured, with a fail-closed bound on chain length.
Contributor
Author
Contributor
Author
|
Size Change: +435 B (0%) Total Size: 20.2 MB 📦 View Changed
ℹ️ View Unchanged
|
turnipdabeets
changed the title
turnipdabeets
marked this pull request as ready for review
August 25, 2026 23:30
# Conflicts: # packages/react-native/src/autocapture.tsx
…ric maxElementsCaptured
ioannisj
approved these changes
Aug 26, 2026
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
2 participants
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.
Problem
Fixes #4648.
ph-no-capturewas silently ignored when the opting-out ancestor sat above the element-collection cap. One loop both decided the opt-out and built the emitted payload, and it was bounded byelements.length < maxElementsCaptured(default 20). Once 20 labelled elements had been collected the loop exited, so aph-no-capturehigher up was never seen and the event went out carrying the element text and props from inside the opted-out subtree.This is a privacy failure, and a silent one — nothing in the payload marks that an opt-out was missed, so an app can look correctly instrumented while leaking exactly the subtree it excluded.
The depth needed is ordinary, not exotic. This repo's own recorded fixture (
packages/react-native/test/data/autocapture-event.json) is a 129-fiber chain with 58 label-bearing elements for a single trivial example screen — nearly 3× the default cap.The canonical spec agrees this is a bug:
autocaptureBehavior §2 Filter ineligible events requires respecting no-capture markers "on the target or ancestor chain", and puts maximum hierarchy sizes in §3 Build element metadata — the cap is a payload bound, never an opt-out decision.Changes
One bounded ancestor pass in
autocaptureFromTouchEvent:maxElementsCapturednow gates only payload collection. The walkcontinues past it so a higherph-no-captureancestor is still seen.maxAncestorsTraversed = 1000bound makes the walk fail closed on an abnormally long chain, mirroring the browser SDK'sshouldCaptureElement(MAX_DOM_ANCESTOR_DEPTH = 1000, also fail-closed) inpackages/browser-common/src/utils/autocapture-utils.ts.data-ph-capture-attribute-*properties,ph-labelpromotion andignoreLabelsare unchanged. The cap check sits above the prop scan, so continued walking costs ~4 property reads per extra ancestor.No public API change —
src/autocaptureis not re-exported fromsrc/index.ts, andpackage.jsonexportsmaps only.,./metro,./expo.Behavior changes (both
patch, both disclosed in the changeset)ph-no-captureancestor above the cap no longer emit$autocaptureph-no-capturehigh in a deep tree — this is the fix; those events were the leakVerification
Unit —
packages/react-native/test/autocapture.spec.ts, 5 tests pinning both sides of both bounds. Full RN suite: 42 suites / 618 tests pass; eslint and prettier clean.Each new test was mutation-checked rather than assumed:
++x >=)should still capture at exactly the traversal bound>vs>=)should fail closed when the ancestor chain exceeds the traversal boundshould fail closed…(mixed labelled/unlabelled chain)should still capture on an ordinarily deep component treeOn device — Android emulator (Pixel_9, Android 17),
examples/example-expo-57built from this branch, with the installed SDK verified to be the patched build. Four cases, each tap confirmed by its own console marker before reading the result:main$autocapture(20 els)$autocapture(20 els)$autocapture(20 els)$autocapture(20 els)ph-no-captureph-no-capture$autocapture, 20 els leakedThe C-vs-D asymmetry on
mainis the bug: the opt-out is honoured inside the cap and silently ignored outside it. Onmain, case D leakedText → ThemedText → View → Pressable → NestedDepth → View → …from inside the opted-out subtree. A and B are identical across both builds, confirming the new traversal bound does not over-fire and suppress legitimate deep captures.Conflicts with #4643
This touches the same hunk as #4643, which keeps the
elements.length < maxElementsCapturedloop condition this PR replaces. Whichever lands second must keep the single-pass walk —should ignore a no-capture ancestor beyond maxElementsCapturedgoes red if the restructure is dropped during the merge, so CI will catch a bad hand-merge.Separately, and as a note for that PR rather than this one:
findOwningNode'smaxOwnerAncestors = 100bounds the same.returnfiber chain. Given the 129-fiber fixture above, a click target more than 100 fibers below the provider would get no owner and no event.Release info Sub-libraries affected
Libraries affected
Checklist
If releasing new changes
pnpm changesetto generate a changeset file🤖 Agent context
Autonomy: Human-driven (agent-assisted)
Opened autonomously by posthog-watcher-action, then reviewed and reworked under human direction using Claude Code.
The original fix scanned the whole chain in a separate pre-pass before the capped loop. That worked, but left the loop's own opt-out check unreachable (jest coverage flagged the
returnas uncovered) and walked the chain twice. It was replaced with a single pass that guards only the collection body, and a fail-closed traversal bound was added to match the browser SDK's precedent.Reviewers rejected two tempting options: lowering the bound to 100 for consistency with #4643's neighbouring constants (100 fibers is reachable — the repo's own fixture is 129), and asserting the constant's exact value in a test (a change-detector). The 400-ancestor floor test was chosen instead, so lowering the bound to anything under 400 fails CI while the exact value stays free to tune.