feat(react-native): autocapture touches and clicks on React Native Web - #4643
Merged
Conversation
ioannisj
changed the title
Contributor
Contributor
|
Size Change: +3.05 kB (+0.02%) Total Size: 20.2 MB 📦 View Changed
ℹ️ View Unchanged
|
Contributor
🦔 ReviewHog reviewed this pull requestNothing worth raising this time, so here's a calming picture instead:
|
ioannisj
marked this pull request as ready for review
August 25, 2026 15:03
Contributor
Prompt To Fix All With AI### Issue 1
packages/react-native/src/PostHogProvider.tsx:195
**Document listener bypasses provider scope**
If a page has multiple `PostHogProvider` instances or another independently mounted React root, every enabled provider handles the click and accepts the target's fiber without checking ownership, causing out-of-tree events to be sent to the wrong client and single interactions to be reported multiple times.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Reviews (1): Last reviewed commit: "chore(react-native): regenerate public A..." | Re-trigger Greptile |
![veria-ai[bot]](https://avatars.githubusercontent.com/in/1723776?s=60&v=4){kind=small}
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
PR overviewThis pull request adds touch and click autocapture for React Native Web through a web-specific event and component traversal fallback. One issue has been addressed, but a privacy-control edge case remains open. In deeply nested component trees, traversal may stop before detecting an outer no-capture ancestor, allowing text or configured properties from a clicked control to be sent despite the exclusion. Exploitation depends on a specific nesting depth and component arrangement. Open issues (1)
Fixed/addressed: 1 · PR risk: 4/10 |
turnipdabeets
left a comment
turnipdabeets
left a comment
Contributor
There was a problem hiding this comment.
One thing with no line to hang off: #2167's thread mixes web and native asks — AdamDorwart's repro was example_expo_rn_web, but a few commenters are on native expo-router. Worth confirming native touch capture works there before Closes #2167 retires the whole issue.
| const handler = (e: any): void => { | ||
| autocaptureFromTouchEvent({ target: e.target, nativeEvent: e }, posthog, optionsRef.current, 'click') | ||
| } | ||
| doc.addEventListener('click', handler, true) |
Contributor
There was a problem hiding this comment.
[question] Two sibling providers sharing one client each register their own listener, so one click in one subtree enqueues two $autocapture events (measured in jsdom: expected 1, got 2 — native gives 1, since onTouchEndCapture is subtree-scoped). The autocapture spec has a scenario for exactly this — "Repeated setup does not install duplicate autocapture observers", exactly one event should be enqueued for that interaction (openspec/specs/autocapture/spec.md). Worth refcounting the listener per client, or is one-provider-per-app the assumption we're happy with?
Contributor
Author
There was a problem hiding this comment.
Good catch on the spec, I had this filed as a preference rather than a conformance gap.
Fixed in 8797151. The document listener is now refcounted per client in a module-level WeakMap, so however many providers mount against one client exactly one listener is installed, and a single interaction enqueues one $autocapture. Two tests came with it: two sibling providers sharing a client produce one event, and the listener only detaches when the last provider unmounts. Both fail against the previous commit, so they're real regression guards rather than passing by accident.
Worth being explicit that this fixes the duplication and not the scope. Clicks outside the provider's subtree are still captured, and with two different clients each still gets its own document listener, so the cross-client misrouting greptile raised is untouched. Picking that up separately since it needs a fiber ancestry check rather than a counter.
| if (!isWeb() || !captureTouches || !doc?.addEventListener) { | ||
| return | ||
| } | ||
| const handler = (e: any): void => { |
Contributor
There was a problem hiding this comment.
[question] Document scope means clicks outside the provider's subtree get captured too — a sibling button under a named app component comes back as elements: ["OutsideAppComponent"], $event_type: "click", where native captures nothing. Probably the right trade for Modal portals, but posthog.com currently says "touch events for children of PostHogProvider are tracked" — can we update that line in the same batch? (Portals work either way; React keeps the fiber .return chain intact through them.)
Contributor
Author
There was a problem hiding this comment.
Scoped in 16b0fca. The listener stays on the document so Modal still works, but capture now walks the fiber .return chain and drops anything whose ancestry doesn't pass through a mounted provider's root node. SO OutsideAppComponent case captures nothing now, and a click in one provider's subtree no longer reaches a second provider's client. This was verified in the browser.
Tbh I did weigh leaving it to a follow-up, multiple roots with distinct clients is rare I would think, but once the wide behaviour ships people depend on it and narrowing later becomes the breaking change
|
|
||
| export const autocaptureFromTouchEvent = (e: any, posthog: PostHog, options: PostHogAutocaptureOptions = {}): void => { | ||
| // react-native-web internals; skipped only where RNW puts them, so a same-named app component is kept. | ||
| const frameworkInternalLabels = ['LocaleProvider'] |
Contributor
There was a problem hiding this comment.
[nit] Could we note the RNW version this was verified against? I checked 0.20.0 — createElement/index.js only wraps in LocaleProvider when domProps.dir is set, so a nested <Text> inside a text ancestor never gets one. That conditional is easy to lose track of on the next RNW bump.
Contributor
Author
There was a problem hiding this comment.
Noted in d12d768. I checked 0.21.2 as well and the condition is unchanged, createElement only wraps in LocaleProvider when domProps.dir is set, so the comment now records both versions and says to recheck on a bump.
Comment on lines
+19
to
+20
| * On React Native Web this also captures `click` events from mouse and trackpad input, | ||
| * emitted with `$event_type: 'click'`, since browsers fire `touchend` only for touch input. |
Contributor
There was a problem hiding this comment.
[suggestion] click also fires for keyboard activation and programmatic .click(), not just pointers — worth saying so, since those produce autocapture events with no touch behind them.
Suggested change
| * On React Native Web this also captures `click` events from mouse and trackpad input, | |
| * emitted with `$event_type: 'click'`, since browsers fire `touchend` only for touch input. | |
| * On React Native Web this also captures `click` events — mouse, trackpad, keyboard | |
| * activation and programmatic clicks — emitted with `$event_type: 'click'`, since | |
| * browsers fire `touchend` only for touch input. |
Contributor
Author
There was a problem hiding this comment.
Applied verbatim in d12d768. You're right that keyboard activation and programmatic .click() both land here with no pointer behind them, and the old wording denied it.
| } catch (error) { | ||
| if (!warnedCaptureFailure) { | ||
| warnedCaptureFailure = true | ||
| console.warn('PostHog autocapture: capturing the interaction threw:', error) |
Contributor
There was a problem hiding this comment.
[nit] Not debug-gated, so this fires once in production for everyone. Fine given warn-once — just noting a systematic failure then goes fully silent, which is the case the comment above is worried about.
Contributor
Author
There was a problem hiding this comment.
Leaving this one as is. Bare console.warn is already the pattern across the package (storage.ts:92, 113, 128, 161 and PostHogProvider.tsx:137), so gating just this call site would make it the odd one out.
Routing it through posthog.logger isn't a free swap either. That ships a record to the logs product over the network on every failure, which is heavier than a console line and uncomfortably close to a loop when the thing failing is capture itself.
Agreed on the substance though, warn-once means a systematic failure surfaces once and then goes quiet for the life of the bundle. Worth revisiting if we ever grow an internal SDK-error channel that isn't the logs product.
Contributor
Author
Native was never broken, and I've confirmed it (Android but iOS should be the same). Ran |
|
|
||
| if (!e._targetInst) { | ||
| const nativeInst = e._targetInst | ||
| const targetInst: Element | undefined = nativeInst || getFallbackTargetInstance(e) |
There was a problem hiding this comment.
Low: No-capture ancestor can be skipped
The new web fallback feeds clicks into a traversal that terminates when elements.length reaches maxElementsCaptured (20 by default), so it never checks any remaining outer fibers for noCaptureProp. An end user can click a control nested under 20 labeled components inside a ph-no-capture ancestor and have its text or captured props sent; scan the complete return chain for exclusions before truncating the emitted element chain.
Contributor
Author
There was a problem hiding this comment.
Confirmed, and it isn't web-only. The ph-no-capture check at autocapture.tsx:170 sits inside the walk that stops at elements.length < maxElementsCaptured (line 161), so the same truncation drops the opt-out on the native touch path too. This PR widens the exposure rather than introducing it.
Splitting it into its own PR since it's a native bug as much as a web one. The fix is the same shape as the ownership walk added here, scan the full .return chain for the opt-out before the truncating element walk, so the cap governs what gets emitted and never what gets decided.
Contributor
Pull request risk assessment: CLEARI checked commit
All checks are clear. This pull request meets the automated low-risk rules. Review pointsNone. |
dustinbyrne
approved these changes
Aug 25, 2026
dustinbyrne
left a comment
dustinbyrne
left a comment
Contributor
There was a problem hiding this comment.
Risk assessment: CLEAR.
Details: #4643 (comment)
12 tasks
23 tasks
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
3 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
Touch autocapture produced no
$autocaptureevents on React Native Web, including expo-router apps running on web.autocaptureFromTouchEventreturned early whenevere._targetInstwas missing, and react-dom's synthetic events never set it, so every touch was dropped silently. The fiber was reachable the whole time, just under a randomised__reactFiber$*key one.targetrather than on the event.Native was never affected. Both
ReactFabric-prod.jsandReactNativeRenderer-prod.jsset_targetInstin theSyntheticEventconstructor and their own dispatch reads it back, so old architecture and Fabric already worked.Browsers also only fire
touchendfor touch input, so mouse and trackpad users on web produced nothing at all, even with the fallback in place.Closes #2167.
Changes
Falls back to walking up from
e.targetto the nearest node carrying a React fiber key (__reactFiber$*, or the legacy__reactInternalInstance$*) when_targetInstis absent. The element walk itself is untouched, since the fiber shape is identical either way.Wraps the capture path in try/catch so a throw can no longer escape into the host app's event dispatch, and warns once rather than on every interaction.
Registers a capture-phase
clicklistener ondocumenton web whencaptureTouchesis on, emitted with$event_type: 'click'. Both the capture phase and the document level are needed: react-native-web'sPressablecallsstopPropagation, and itsModalportals content outside the provider's subtree.onTouchEndCaptureis no longer wired on web, so a tap that fires bothtouchendand a synthesisedclickis still captured once.Skips react-native-web's own
LocaleProviderwrapper only where RNW puts it, at the head of the chain on the fallback path, so an app component of the same name keeps its place inelements.Tests cover both paths against the same fixture: the
_targetInstpath, the fiber fallback, the legacy key, the ancestor walk, a target whose property access throws,ph-no-capturethrough the fallback, and the click listener's phase, portal, re-render and unmount behaviour.Release info
Sub-libraries affected
Libraries affected
Checklist
If releasing new changes
pnpm changesetto generate a changeset file🤖 Agent context
DRI: @ioannisj
Autonomy: Human-driven (agent-assisted)