refactor: Upgrade to React Native Gesture Handler v3#499
Merged
Conversation
- v3 was released as stable today, but we won't install it immediately due to our PNPM rules of waiting a day after a package has been released before installing it.
- Gestures are now created with hooks. - `onStart` & `onEnd` were renamed. - We can pass SharedValue to the config properties.
- Since to apply our "fake gaps", we add a "mb-2" style basically to every rendered item, this would result in a bigger gap at the bottom of the list. To counter this, we had a `-mb-2` style on the draglist.
- We get have worklet warnings. - One of the annoying ones is "Tried to modify key `current` of an object which has been already passed to a worklet". - From this, we found our that our `Swipeable` component broke, which was due to the value we assigned to `ref.current` not being available in the current callback.
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
mobile/src/components/Swipeable.tsx (1)
147-151:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winMissing
.start()call on reset animation.The
Animated.timing()is created but never started. The comment above indicates this animation should resetdragXto prevent recycled items from staying swiped, but without calling.start(), it has no effect.🐛 Proposed fix
// Reset to prevent the recycled item being stuck in the swiped state. // - Use `Animated.timing()` instead of `setValue()` due to the animation // resolving before the UI removes the item on the New Architecture. Animated.timing(dragX, { duration: 0, toValue: 0, useNativeDriver: true, - }); + }).start();🤖 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 `@mobile/src/components/Swipeable.tsx` around lines 147 - 151, The reset animation for dragX in the Swipeable component is created but never started; update the code that constructs Animated.timing(dragX, { duration: 0, toValue: 0, useNativeDriver: true }) so it is invoked (e.g., call .start() on the returned animation) to actually reset dragX when recycling items, ensuring the timing animation runs and clears any residual swipe state.
🤖 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 `@mobile/src/components/NScrollbar.tsx`:
- Around line 157-164: The onFinalize callback currently calls onEnd
unconditionally which can fire when a pan never became ACTIVE; modify the
NScrollbar gesture handlers to track whether a real drag occurred (e.g.,
introduce a didDrag boolean set to true inside onActivate or onUpdate on first
movement and reset in onDeactivate/dismissScrollbar), and change onFinalize to
only call onEnd when didDrag is true; ensure you still reset didDrag alongside
nextScrollPosition.set(-1) and prevY.set(-1) in onDeactivate so subsequent
gestures are clean.
---
Outside diff comments:
In `@mobile/src/components/Swipeable.tsx`:
- Around line 147-151: The reset animation for dragX in the Swipeable component
is created but never started; update the code that constructs
Animated.timing(dragX, { duration: 0, toValue: 0, useNativeDriver: true }) so it
is invoked (e.g., call .start() on the returned animation) to actually reset
dragX when recycling items, ensuring the timing animation runs and clears any
residual swipe state.
🪄 Autofix (Beta)
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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 516402e0-0937-49ee-9134-2076f4c24dd1
⛔ Files ignored due to path filters (1)
mobile/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (12)
mobile/modules/toast/src/components/Toast.tsxmobile/package.jsonmobile/src/components/DragList.tsxmobile/src/components/Form/Input.tsxmobile/src/components/Form/Slider.tsxmobile/src/components/NScrollbar.tsxmobile/src/components/Swipeable.tsxmobile/src/modules/customization/theme/components/ColorPicker.tsxmobile/src/navigation/components/MiniPlayer.tsxmobile/src/navigation/screens/now-playing/helpers/useVinylSeekbar.tsmobile/src/navigation/screens/settings/sheets/TabOrderSheet.tsxmobile/src/resources/licenses.json
💤 Files with no reviewable changes (1)
- mobile/src/components/Form/Input.tsx
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
1 participant
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.
Why
The migration to the hook-based API was mostly smooth, with some hiccups. Some notable changes include:
onStart->onActivate&onEnd->onDeactivate.SharedValuedirectly in the Gesture configs.~0.35 MB.One thing that did break during this migration were the swipe gestures on our miniplayer, which was handled by our
Swipeablecomponent. Due to switching to the hook-based API, if we update the value of a ref inside one of the callbacks and later accessed it in the same callback, we would see that the ref doesn't contain the latest value. This might be due to the cloning that happens under the hood in the hook.Besides that, we get a lot more worklet warnings.
Other Changes / Notes
DragListcomponent.onCleanupfunction in theDragListcomponent to a worklet function due to the timing changes resulting in a worse "jumping" effect after dropping the item.References:
Checklist
pnpm sync:licenses.pnpm android:prod.Summary by CodeRabbit
Bug Fixes
Chores