Conversation
`shouldUpdateDetector` resolved every relation entry through `transformIntoHandlerTags` on each gesture mount. That call runs `toArray`, `map` and `filter`, so each mount allocated three arrays per relation, for three relations, for every attached gesture of every mounted detector. Opening a sheet with ~50 pressables blocked the JS thread for ~800ms on a Pixel 7 Pro with several hundred detectors mounted. Most of that work could never match. Only refs can start pointing at a different handler after the detector attached, because `current` is filled in when the gesture they point at mounts. Gestures and numeric tags already carry their tag by then, and `transformIntoHandlerTags` mapped them to -1 and filtered them out anyway. Walk the relation array directly and compare `current.handlerTag`, skipping the entries that cannot resolve late. No allocations, and the impossible comparisons are gone. This also fixes late-mounted relations on web, where they never re-attached: the web branch of `transformIntoHandlerTags` returns handler objects, which the caller compared against a numeric `handlerTag`, so the check was always false. Comparing tags works on both platforms. Fixes software-mansion#4540
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review. 📝 SummarySummary by CodeRabbit
Walkthrough
ChangesMount reaction relation checks
Priority: ➖ Normal Severity of issue fixed: Medium Merge Risk: ⚪ Minimal · up to The relation check now matches mounted handlers through their refs, with tests covering matching and non-matching cases. No material merge-blocking risk is evident; normal checks remain appropriate. 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Full details: Linked Issues checkExplanation For
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
One or more issues must be addressed before approval.
Review effort: Lite
Findings: None
What changed in this PR
Optimizes detector mount relation checks for issue #4540 by scanning refs directly without allocating intermediate arrays, while preserving relation behavior across platforms.
Changes:
- Replaced
transformIntoHandlerTagscalls with allocation-free ref scanning. - Added web-focused tests for relation matching and lifecycle cases.
| File | Description |
|---|---|
| packages/react-native-gesture-handler/src/handlers/gestures/GestureDetector/useMountReactions.ts | Updated as part of this pull request. |
| packages/react-native-gesture-handler/src/__tests__/useMountReactions.test.ts | Updated as part of this pull request. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
Thanks for flagging the string-ID case — I looked into it specifically, and I don't think it reaches this path.
More decisively, the attach-time resolver for these same three arrays — if (typeof ref === 'number') return ref;
else if (ref instanceof BaseGesture) return ref.handlerTag;
else return ref.current?.handlerTag ?? -1;There is no The So the new scan deliberately mirrors |
Description
Fixes #4540
shouldUpdateDetectorresolved every relation entry throughtransformIntoHandlerTagson each gesture mount. That call runstoArray,mapandfilter, so each mount allocated three arrays per relation, for three relations, for every attached gesture of every mounted detector. The issue reports ~800ms of JS thread blocking when opening a sheet with ~50 pressables on a Pixel 7 Pro, with several hundred detectors mounted app-wide.Most of that work could never match. Only refs can start pointing at a different handler after the detector attached, because
currentis filled in when the gesture they point at mounts. Gestures and numeric tags already carry their tag by then — andtransformIntoHandlerTagsmapped them to-1and filtered them out anyway.This walks the relation array directly and compares
current.handlerTag, skipping entries that cannot resolve late. No allocations, and the impossible comparisons are gone.It also fixes late-mounted relations on web, the secondary issue in the report: the web branch of
transformIntoHandlerTagsreturns handler objects, which the caller compared against a numerichandlerTag, so the check was always false and those relations never re-attached. Comparing tags works on both platforms.Behaviour for gesture objects and numeric tags is unchanged — they were already filtered out before reaching the comparison.
Test plan
New
src/__tests__/useMountReactions.test.ts, run withPlatform.OSmocked towebso it covers the branch that was broken:blocksHandlers,requireToFailorsimultaneousWithresolving on mount updates the detector — these three fail without the change, since the web path never matchedcurrent: null) does not update the detectorVerified in
packages/react-native-gesture-handler:yarn jest185 passing across 22 suites,yarn ts-checkclean,yarn lint:jsclean on the changed files.Not verified on a device or simulator — this environment has neither, so the Android and iOS builds and the example apps were not exercised.