Ensure no gesture is used across multiple detectors#4285
Open
m-bert wants to merge 2 commits into
Open
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Adds a dev-time JavaScript guard to prevent attaching the same v3 gesture instance to more than one detector simultaneously, addressing cross-platform inconsistencies (native last-detector-wins vs web multi-detector behavior).
Changes:
- Introduces
useDetectorAttachmentGuardthat trackshandlerTagownership per mounted detector and throws in__DEV__on conflicts. - Wires the guard into v3 detectors (
NativeDetector,VirtualDetector,InterceptingGestureDetector) using computedhandlerTags. - Adds Jest coverage for the “shared gesture across detectors” error case (plus a reattach scenario).
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/react-native-gesture-handler/src/v3/detectors/VirtualDetector/VirtualDetector.tsx | Computes handlerTags via useMemo and invokes the new attachment guard. |
| packages/react-native-gesture-handler/src/v3/detectors/VirtualDetector/InterceptingGestureDetector.tsx | Invokes the new attachment guard for intercepting detector handlerTags. |
| packages/react-native-gesture-handler/src/v3/detectors/useDetectorAttachmentGuard.ts | New hook implementing the dev-only “single detector owns a handlerTag” invariant. |
| packages/react-native-gesture-handler/src/v3/detectors/NativeDetector.tsx | Invokes the new attachment guard for native detector handlerTags. |
| packages/react-native-gesture-handler/src/tests/Errors.test.tsx | Adds tests validating throws on shared gestures and a reattachment path. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
36
to
+43
| const handlerTags = useMemo(() => { | ||
| return isComposedGesture(gesture) | ||
| ? gesture.handlerTags | ||
| : [gesture.handlerTag]; | ||
| }, [gesture]); | ||
|
|
||
| useDetectorAttachmentGuard(handlerTags); | ||
|
|
Comment on lines
+109
to
+126
| test('does not throw when the same gesture is reattached to another detector', () => { | ||
| // Changing the key unmounts the previous detector and mounts a new one with | ||
| // the same gesture - the old detector must release the gesture before the | ||
| // new one claims it. | ||
| function ReattachedGesture({ detectorKey }: { detectorKey: string }) { | ||
| const tap = useTapGesture({}); | ||
| return ( | ||
| <GestureHandlerRootView> | ||
| <GestureDetector key={detectorKey} gesture={tap}> | ||
| <View /> | ||
| </GestureDetector> | ||
| </GestureHandlerRootView> | ||
| ); | ||
| } | ||
|
|
||
| const { rerender } = render(<ReattachedGesture detectorKey="a" />); | ||
| expect(() => rerender(<ReattachedGesture detectorKey="b" />)).not.toThrow(); | ||
| }); |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
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
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.
Description
Currently there's no error when the same gesture is used in multiple detectors. This may results in a bug, e.g. on Android and iOS only last detector captures gestures, on web all of them. This PR adds JS side check whether instance of a gesture was passed into more than one detector.
Note
The example below crashes on web, in reattaching scenario. However, this seems to be unrelated to this PR, so I left that for a follow-up.
Test plan
Tested on the following code: