Move hitSlop normalization to JS - #4388
Open
coado wants to merge 9 commits into
Open
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
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 (2)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe change normalizes hit-slop inputs into a canonical six-element array. Legacy, v3, web, Android, and Apple paths now consume this format. Tests cover normalization, validation, shared values, wiring, and nullish behavior. ChangesHit-slop normalization
Sequence Diagram(s)sequenceDiagram
participant GestureConfig
participant normalizeHitSlop
participant NativeConfig
participant GestureHandler
GestureConfig->>normalizeHitSlop: raw hitSlop value
normalizeHitSlop-->>GestureConfig: canonical six-element array
GestureConfig->>NativeConfig: normalized hitSlop
NativeConfig->>GestureHandler: apply hit-slop tuple
GestureHandler-->>GestureConfig: updated gesture bounds
Suggested reviewers: 🚥 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 |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR centralizes hitSlop parsing/validation in a shared JS module and changes the native/web consumers to accept a single canonical shape ([left, top, right, bottom, width, height] with null for “unset”), reducing platform-specific parsing logic.
Changes:
- Introduces
normalizeHitSlop+CanonicalHitSlopin JS and updates config producers (v1/v2filterConfig, v3 native config prep, v3 shared-value UI-thread updates, and the web button wrapper) to emit the canonical array. - Simplifies native (Android/iOS)
hitSlopparsing to fixed-index reads, plus DIP→px conversion on Android. - Updates web handler types and hit-testing logic to consume the canonical array and adds targeted Jest tests to lock the wire contract.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/react-native-gesture-handler/src/web/interfaces.ts | Switches web config types from object HitSlop to CanonicalHitSlop. |
| packages/react-native-gesture-handler/src/web/handlers/GestureHandler.ts | Updates web handler to store/consume canonical hitSlop arrays; removes local validation. |
| packages/react-native-gesture-handler/src/v3/hooks/utils/reanimatedUtils.ts | Normalizes hitSlop when pushed via UI-thread shared value updates. |
| packages/react-native-gesture-handler/src/v3/hooks/utils/configUtils.ts | Normalizes hitSlop in v3 config preparation (including shared values). |
| packages/react-native-gesture-handler/src/utils.ts | Adds isWorkletRuntime helper for worklet-vs-RN runtime detection. |
| packages/react-native-gesture-handler/src/handlers/utils.ts | Normalizes hitSlop in v1/v2 filterConfig. |
| packages/react-native-gesture-handler/src/handlers/hitSlop.ts | Adds shared canonical hitSlop type + normalization/validation logic (worklet-safe). |
| packages/react-native-gesture-handler/src/components/GestureHandlerButton.web.tsx | Normalizes button hitSlop before passing to the web module. |
| packages/react-native-gesture-handler/src/tests/hitSlopWiring.test.ts | Adds wiring tests for normalization at multiple producers. |
| packages/react-native-gesture-handler/src/tests/hitSlopSharedValue.test.ts | Adds test ensuring UI-thread shared-value path normalizes hitSlop. |
| packages/react-native-gesture-handler/src/tests/hitSlop.test.ts | Adds unit tests for normalizeHitSlop, including worklet-runtime behavior. |
| packages/react-native-gesture-handler/apple/RNGestureHandlerButtonComponentView.mm | Updates button hitSlop config emission to the canonical array shape. |
| packages/react-native-gesture-handler/apple/RNGestureHandler.mm | Updates iOS handler hitSlop parsing to read canonical array indices. |
| packages/react-native-gesture-handler/android/src/main/java/com/swmansion/gesturehandler/core/GestureHandler.kt | Updates Android hitSlop parsing to read canonical array indices and convert DIP→px. |
Suppressed comments (1)
packages/react-native-gesture-handler/src/tests/hitSlop.test.ts:205
- These assignments also rely on
globalThis.__RUNTIME_KIND/_WORKLETtypings. Once a localgis introduced, use it consistently here to avoid TS errors.
test('reports instead of throwing', () => {
// 2 is the UI runtime.
globalThis.__RUNTIME_KIND = 2;
expectReported();
});
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/react-native-gesture-handler/src/v3/hooks/utils/configUtils.ts (1)
110-117: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse
maybeUnpackValuefor the shared-value unwrapping.Line 102 in the same function already unwraps through
maybeUnpackValue. The inlineReanimated?.isSharedValue(value) ? value.value : valueduplicates that rule. Use the helper so both call sites stay in sync.♻️ Proposed refactor
- const unpackedValue = Reanimated?.isSharedValue(value) - ? value.value - : value; + const unpackedValue = maybeUnpackValue(value); (filteredConfig as Record<string, unknown>)[key] = key === 'hitSlop' ? normalizeHitSlop(unpackedValue as HitSlop) : unpackedValue;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/react-native-gesture-handler/src/v3/hooks/utils/configUtils.ts` around lines 110 - 117, In the config filtering logic, replace the inline Reanimated shared-value check and value access before the hitSlop handling with the existing maybeUnpackValue helper already used earlier in the same function. Keep the subsequent normalizeHitSlop and filteredConfig assignment behavior unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@packages/react-native-gesture-handler/android/src/main/java/com/swmansion/gesturehandler/core/GestureHandler.kt`:
- Around line 988-1003: Update the hitSlop reader around the local edge function
and handler.setHitSlop call to validate each tuple entry before accessing it:
treat out-of-bounds or non-numeric values as GestureHandler.HIT_SLOP_NONE,
matching Apple behavior for malformed arrays. Use ReadableArray bounds/type
checks before getDouble so malformed hitSlop configuration cannot crash native
updates.
In `@packages/react-native-gesture-handler/src/web/handlers/GestureHandler.ts`:
- Around line 879-915: Add web hit-slop coverage in GestureHandler.test.ts for
zero-valued edges, explicitly null edges, and combinations of
left/right/top/bottom with width/height sizing. Ensure the tests validate the
bounds computed by the hit-slop logic in GestureHandler, then run yarn lint:js,
yarn format:js, yarn ts-check, and yarn test GestureHandler.test.ts with
dependencies installed.
---
Nitpick comments:
In `@packages/react-native-gesture-handler/src/v3/hooks/utils/configUtils.ts`:
- Around line 110-117: In the config filtering logic, replace the inline
Reanimated shared-value check and value access before the hitSlop handling with
the existing maybeUnpackValue helper already used earlier in the same function.
Keep the subsequent normalizeHitSlop and filteredConfig assignment behavior
unchanged.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 68c6aa8c-5054-4c13-bc42-508a04972fe0
📒 Files selected for processing (14)
packages/react-native-gesture-handler/android/src/main/java/com/swmansion/gesturehandler/core/GestureHandler.ktpackages/react-native-gesture-handler/apple/RNGestureHandler.mmpackages/react-native-gesture-handler/apple/RNGestureHandlerButtonComponentView.mmpackages/react-native-gesture-handler/src/__tests__/hitSlop.test.tspackages/react-native-gesture-handler/src/__tests__/hitSlopSharedValue.test.tspackages/react-native-gesture-handler/src/__tests__/hitSlopWiring.test.tspackages/react-native-gesture-handler/src/components/GestureHandlerButton.web.tsxpackages/react-native-gesture-handler/src/handlers/hitSlop.tspackages/react-native-gesture-handler/src/handlers/utils.tspackages/react-native-gesture-handler/src/utils.tspackages/react-native-gesture-handler/src/v3/hooks/utils/configUtils.tspackages/react-native-gesture-handler/src/v3/hooks/utils/reanimatedUtils.tspackages/react-native-gesture-handler/src/web/handlers/GestureHandler.tspackages/react-native-gesture-handler/src/web/interfaces.ts
coado
marked this pull request as ready for review
August 7, 2026 12:02
m-bert
reviewed
Aug 7, 2026
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/react-native-gesture-handler/src/v3/types/ConfigTypes.ts`:
- Line 77: Resolve the failing Jest cases in hitSlop.test.ts and
hitSlopSharedValue.test.ts by tracing the hitSlop type and runtime handling from
ConfigTypes.ts, then update the implementation or tests so both regular and
shared-value hitSlop behavior passes. Verify the package test suite succeeds
without regressing the existing lint, formatting, or TypeScript checks.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: efb67868-57be-440a-be3e-8b1de90b58f9
📒 Files selected for processing (5)
packages/react-native-gesture-handler/apple/RNGestureHandler.mmpackages/react-native-gesture-handler/src/handlers/hitSlop.tspackages/react-native-gesture-handler/src/v3/hooks/utils/configUtils.tspackages/react-native-gesture-handler/src/v3/hooks/utils/reanimatedUtils.tspackages/react-native-gesture-handler/src/v3/types/ConfigTypes.ts
💤 Files with no reviewable changes (1)
- packages/react-native-gesture-handler/apple/RNGestureHandler.mm
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/react-native-gesture-handler/src/v3/hooks/utils/configUtils.ts
- packages/react-native-gesture-handler/src/v3/hooks/utils/reanimatedUtils.ts
- packages/react-native-gesture-handler/src/handlers/hitSlop.ts
| enabled?: boolean | undefined; | ||
| shouldCancelWhenOutside?: boolean | undefined; | ||
| hitSlop?: HitSlop | undefined; | ||
| hitSlop?: HitSlop | CanonicalHitSlop | undefined; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
yarn lint:js
yarn format:js
yarn ts-check
(cd packages/react-native-gesture-handler && yarn test)Repository: software-mansion/react-native-gesture-handler
Length of output: 50401
🏁 Script executed:
#!/usr/bin/env bash
set -u
printf '%s\n' '--- root scripts ---'
node - <<'JS'
const p = require('./package.json');
for (const [k, v] of Object.entries(p.scripts || {})) {
if (/^(lint:js|format:js|ts-check|test)/.test(k)) console.log(`${k}: ${v}`);
}
JS
printf '%s\n' '--- package scripts ---'
node - <<'JS'
const p = require('./packages/react-native-gesture-handler/package.json');
for (const [k, v] of Object.entries(p.scripts || {})) {
if (/^(lint:js|format:js|ts-check|test)/.test(k)) console.log(`${k}: ${v}`);
}
JS
printf '%s\n' '--- combined-command tail ---'
log=/tmp/coderabbit-shell-logs/shell-output-WkHJhD
if [ -f "$log" ]; then
tail -n 120 "$log"
else
printf '%s\n' 'Log not available'
fi
printf '%s\n' '--- repository status summary ---'
git status --short
git diff --statRepository: software-mansion/react-native-gesture-handler
Length of output: 10604
🏁 Script executed:
#!/usr/bin/env bash
set -u
printf '%s\n' '--- root scripts and workspace configuration ---'
node - <<'JS'
const p = require('./package.json');
console.log(JSON.stringify({
scripts: p.scripts || {},
packageManager: p.packageManager,
workspaces: p.workspaces
}, null, 2));
JS
printf '%s\n' '--- target type and related symbols ---'
cat -n packages/react-native-gesture-handler/src/v3/types/ConfigTypes.ts | sed -n '1,105p'
rg -n "normalizeHitSlop|expectReported|bindSharedValues|mockUpdateGestureHandlerConfig|HitSlop" \
packages/react-native-gesture-handler/src/__tests__/hitSlop.test.ts \
packages/react-native-gesture-handler/src/__tests__/hitSlopSharedValue.test.ts \
packages/react-native-gesture-handler/src/handlers/hitSlop.ts
printf '%s\n' '--- recent commit summary ---'
git log --oneline -3
git show --stat --oneline HEAD
git diff HEAD^ HEAD -- packages/react-native-gesture-handler/src/v3/types/ConfigTypes.ts \
packages/react-native-gesture-handler/src/handlers/hitSlop.ts \
packages/react-native-gesture-handler/src/__tests__/hitSlop.test.ts \
packages/react-native-gesture-handler/src/__tests__/hitSlopSharedValue.test.ts
printf '%s\n' '--- validation-stage markers from the prior log ---'
log=/tmp/coderabbit-shell-logs/shell-output-WkHJhD
if [ -f "$log" ]; then
rg -n "tsc|ts-check|Test Suites:|Tests:|FAIL |PASS |Done in|error TS|error Command|Exit code|Command failed" "$log" | tail -n 100
fiRepository: software-mansion/react-native-gesture-handler
Length of output: 13910
Resolve the failing Jest tests before merge.
yarn lint:js, yarn format:js, and yarn ts-check pass. Package yarn test fails in hitSlop.test.ts and hitSlopSharedValue.test.ts.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/react-native-gesture-handler/src/v3/types/ConfigTypes.ts` at line
77, Resolve the failing Jest cases in hitSlop.test.ts and
hitSlopSharedValue.test.ts by tracing the hitSlop type and runtime handling from
ConfigTypes.ts, then update the implementation or tests so both regular and
shared-value hitSlop behavior passes. Verify the package test suite succeeds
without regressing the existing lint, formatting, or TypeScript checks.
Source: Coding guidelines
m-bert
requested changes
Aug 10, 2026
j-piasecki
reviewed
Aug 12, 2026
|
|
||
| prop = config[@"hitSlop"]; | ||
| if ([prop isKindOfClass:[NSNumber class]]) { | ||
| _hitSlop.left = _hitSlop.right = _hitSlop.top = _hitSlop.bottom = [prop doubleValue]; |
Member
There was a problem hiding this comment.
Can we leave the single-number path in place on both platforms? Handling this is a matter of building a rect using the number, and passing it is much more efficient than the wrappers RN is using for objects (at least on Android).
| if (!isnan(_hitSlop.height) && !isnan(_hitSlop.top) && !isnan(_hitSlop.bottom)) { | ||
| RCTLogError(@"Cannot have all of top, bottom and height defined"); | ||
| } | ||
| // An explicit `null` clears the hit slop; a missing key leaves the previous value alone. |
Member
There was a problem hiding this comment.
Won't explicit null be translated to nil?
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
This moves the parsing and validation of
hitSlopinto one shared JS module and reduces the platforms to reading a fixed shape.JS now sends a normalized [left, top, right, bottom, width, height] array, where null marks an unspecified edge. That layout matches what the platforms already stored internally (FloatArray(6) on Android, RNGHHitSlop on Apple), so the native parsers shrink to mapping null → NaN, plus the DIP→px conversion on Android.
Applied at all four producers: filterConfig (v1 and v2), prepareConfigForNativeSide (v3), bindSharedValues (v3's UI-thread path) and the button's web wrapper.
Test plan
Tested on Android, iOS and Web