Pass reanimatedEventHandler only when the Reanimated detector is used - #4429
Conversation
The handler is built whenever `disableReanimated` is unset, but `shouldUseReanimatedDetector` additionally requires worklet callbacks. A gesture with none renders the plain host component, which forwards props verbatim, so it received Reanimated's event handler object under `onGestureHandlerReanimatedEvent`, a codegen DirectEventHandler prop. React then throws out of `getListener` instead of dispatching.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: Your plan includes up to 8 reviews per rolling hour; 7 remain after this review. 📝 WalkthroughSummary by CodeRabbit
WalkthroughChangesNative detector event handling
Merge Risk: ⚪ Minimal · up to The change limits the Reanimated event handler to the detector that uses it, preventing an invalid handler object from being passed to plain gesture detectors. No actionable merge-blocking risk remains beyond normal checks and review. 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
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.
Pull request overview
Fixes a New Architecture crash where NativeDetector could forward Reanimated’s { workletEventHandler } object into a codegen DirectEventHandler prop (onGestureHandlerReanimatedEvent) when the Reanimated detector is not in use, causing React to throw when resolving listeners.
Changes:
- Gate
onGestureHandlerReanimatedEventso it’s only provided whengesture.config.shouldUseReanimatedDetectoris true (native path), otherwise passundefinedto avoid an invalid listener type. - Add an in-file comment documenting why the guard is necessary and how the mismatch occurs.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
m-bert
left a comment
m-bert
left a comment
There was a problem hiding this comment.
Hi @antFrancon! Thank you for submitting this PR! Could you please also add the same guard for the web part? I know that it should be safe since we have forReanimated, but I'd like to keep these in sync 😅
| // `reanimatedEventHandler` is built whenever `disableReanimated` is unset, but | ||
| // `shouldUseReanimatedDetector` additionally requires worklet callbacks. When it is | ||
| // false we render the plain host component, which forwards props verbatim, so passing | ||
| // the handler would put a non-function on a codegen `DirectEventHandler` prop. |
There was a problem hiding this comment.
I don't think we need this
| // `reanimatedEventHandler` is built whenever `disableReanimated` is unset, but | |
| // `shouldUseReanimatedDetector` additionally requires worklet callbacks. When it is | |
| // false we render the plain host component, which forwards props verbatim, so passing | |
| // the handler would put a non-function on a codegen `DirectEventHandler` prop. |
Web is safe today because the handler only emits Reanimated events when `forReanimated` is set, which itself comes from `dispatchesReanimatedEvents` and so already requires `shouldUseReanimatedDetector`. Hoisting the guard keeps both branches in sync rather than relying on that.
|
Done, pushed. I hoisted the guard into a single And you are right that web is already safe:
|
Description
NativeDetectoralways passesreanimatedEventHandlerto the native component, even when the gesture does not use the Reanimated detector. Two different conditions decide whether the handler exists and whether the Reanimated detector is used.useGestureCallbacksbuilds the handler wheneverdisableReanimatedis unset:shouldUseReanimatedDetectoradditionally requires worklet callbacks:So a gesture with no worklet callbacks gets
shouldUseReanimatedDetector === false, renders the plainHostGestureDetector, which forwards props verbatim, and receives Reanimated's{ workletEventHandler }object underonGestureHandlerReanimatedEvent. That prop is a codegenDirectEventHandler, so React throws out ofgetListenerinstead of dispatching:It stays latent most of the time, because a handler on a plain detector gets
dispatchesReanimatedEvents: shouldUseReanimatedDetector && !runOnJS, which is false, so it never emits the event. It surfaces once a handler ends up attached to a detector that is not its own. The prop is invalid either way.This passes the handler only when the Reanimated detector is actually used. The event prop is part of the static view config, so
undefinedonly means there is no JS listener, and native still emits the event.I left the web branch untouched, since I have not tested whether the same mismatch applies there. Happy to extend it if you think it does.
Fixes #4428
Test plan
yarn ts-check,yarn lint:jsandyarn testinpackages/react-native-gesture-handler.NativeDetectorpicks and thetypeofof the handler it receives.useNativeGesture(),usePanGesture()anduseTapGesture({ onActivate })all renderHostGestureDetectorwithtypeof reanimatedEventHandler === 'object'.undefined, and the worklet control still gets the handler onReanimatedNativeDetector.