Conversation
|
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 (1)
Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 7 remain after this review. 📝 SummarySummary by CodeRabbit
WalkthroughThe web tap handler’s default inter-tap delay changes from 500 ms to 200 ms. Related tap gesture documentation now states a 200 ms default. ChangesTap Gesture Delay
Suggested reviewers: Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to Web multi-taps now use a 200 ms default interval instead of 500 ms; callers needing a longer interval can configure it explicitly. The related documentation matches this intended change, so no actionable merge risk remains. Security Architecture ReviewSecurity architecture risk: ⚪ Minimal · up to The change shortens the default waiting period without expanding access or authority. Explicit timeout settings remain effective, and the existing failure and cleanup paths are preserved. No material security risk was identified in the changed behavior. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
🚥 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.
Copilot review overview
🟢 Approval recommended
The focused change includes regression coverage, preserves explicit overrides, and has no identified blocking issues.
Review effort: Balanced
Findings: None
What changed in this PR
Aligns web Tap’s default inter-tap delay with Android and Apple at 200 ms, while preserving explicit overrides.
Changes:
- Changes the web default from 500 ms to 200 ms.
- Adds regression tests for timing, overrides, and configuration resets.
- Updates current documentation and API comments; versioned docs remain unchanged.
| File | Description |
|---|---|
| packages/react-native-gesture-handler/src/web/handlers/TapGestureHandler.ts | Sets the default delay to 200 ms. |
| packages/react-native-gesture-handler/src/web/handlers/__tests__/TapGestureHandler.test.ts | Tests default timing, overrides, and resets. |
| packages/react-native-gesture-handler/src/v3/hooks/gestures/tap/TapTypes.ts | Corrects the v3 default-value comment. |
| packages/react-native-gesture-handler/src/handlers/TapGestureHandler.ts | Corrects the legacy API default-value comment. |
| packages/docs-gesture-handler/docs/gestures/use-tap-gesture.mdx | Documents the 200 ms default. |
💡 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
There was a problem hiding this comment.
Thank you for this PR! ❤️
I don't think the test file is necessary, other than that looks good!
|
Thanks for reviewing the PR! 🙏❤️ You're right, changing one constant doesn't need a whole new test file, so I removed it. I also updated one more stale "default is 500" comment in the v2 builder ( |
Description
The default for Tap's
maxDelay(how long the handler waits for the next tap whennumberOfTaps > 1) was 500 ms on web but 200 ms on Android and Apple, so the sameuseTapGesture({ numberOfTaps: 2 })behaved differently per platform:src/web/handlers/TapGestureHandler.ts:10android/.../core/TapGestureHandler.kt:190apple/Handlers/RNTapHandler.m:51As discussed in #4555, 200 ms is the intended default, since it is the value chosen for mobile. This PR changes web to 200 ms and updates the places that stated 500 ms: the
useTapGesturedocs page and themaxDelaycomments inTapTypes.ts, the v1TapGestureHandler.tsand the v2tapGesture.ts. The 1.x and 2.x versioned docs are untouched.Behaviour change: on web, a multi-tap now fails if the next tap takes longer than 200 ms, unless
maxDelayis set explicitly. Setting it keeps working as before.Discussion: #4555
Test plan
Ran in
packages/react-native-gesture-handler:yarn test: 21 suites, 178 tests passedyarn ts-check: exit 0yarn lint:js: exit 0, 0 errorsI also checked the new default with a throwaway Jest test (two-tap config, first tap, 300 ms passes), which failed at 500 and passed at 200. It is not included in the PR.
The change is web and docs only, so I did not run Android or iOS builds.