Repository navigation
Conversation
|
|
There was a problem hiding this comment.
Why do we want to use LegacyComposedGesture? Shouldn't we migrate to the new API?
There was a problem hiding this comment.
My initial thought is to split the bump and the migration into two PRs to prevent a lot of regressions at once. This PR just bumps the gesture handler, and the next one will migrate the old components to the new ones. WDYT?
There was a problem hiding this comment.
Cool, I'm fine with it 👍 We can unblock RN 0.88 and then focus on migration, nice
There was a problem hiding this comment.
Same but @MelvinBot can you create new issue for this migration to the new API please? Assign @collectioneur @QichenZhu and @mountiny
There was a problem hiding this comment.
I created the follow-up issue: [HOLD App#102678] Migrate react-native-gesture-handler builder API to the 3.x hook API
- Assignees:
collectioneur,QichenZhu,mountiny - Labels:
Weekly,Task - Scope: the 24 files with grandfathered
no-deprecatedseatbelt entries from this PR, grouped by feature area. It's done when theLegacy*types and those seatbelt entries are gone.
The issue stays on hold until this PR merges.
There was a problem hiding this comment.
Why do we need so many new entries here? 👀
There was a problem hiding this comment.
The entire builder API in RNGH 3 is marked as deprecated in favor of the new hook API, that’s why the linter is throwing errors. The 3.x builder API still works, but moving to hooks requires rewriting gesture logic, so I think it would be better to split this bump into two or more PRs
There was a problem hiding this comment.
@collectioneur Should we do that in a follow up? Happy to create new issues for that
There was a problem hiding this comment.
Yes, I think a new issue would be more suitable for that, thanks!
|
@QichenZhu Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
|
🚧 mountiny has triggered a test Expensify/App build. You can view the workflow run here. |
|
@MelvinBot can you create payment issue for @QichenZhu for $175 for their work here? |
This comment has been minimized.
This comment has been minimized.
|
I created the payment issue: Payment issue for E/App PR#102678 - $175 for @QichenZhu
This PR hasn't merged yet. After it deploys to production, add |
mountiny
left a comment
mountiny
left a comment
There was a problem hiding this comment.
Looks good, gonna create issues for the migration now
JmillsExpensify
left a comment
JmillsExpensify
left a comment
There was a problem hiding this comment.
No product review needed on this one.
QichenZhu
left a comment
QichenZhu
left a comment
There was a problem hiding this comment.
Code looks good except for minor comments. Next I'll do more testing.
| useComposedEventHandler: jest.fn(() => () => {}), | ||
| useHandler: jest.fn(() => ({context: {}, doDependenciesDiffer: false})), | ||
| isSharedValue: jest.fn((value: unknown) => typeof value === 'object' && value !== null && 'value' in value && 'get' in value && typeof value.get === 'function'), |
There was a problem hiding this comment.
Hm, good catch. I guess let's leave my mock, then, if the PR with the gesture handler bump gets merged before the reanimated one, I'll remove these lines in the reanimated bump. If not, I'll remove them here
|
@MelvinBot please review the changes for accessibility issues. |
|
I found no confirmed accessibility regressions. The App changes are type renames only, so the risk comes from runtime changes inside Gesture Handler 3.3.0. One area needs a screen-reader test before merge. Please test: TalkBack and VoiceOver on the composer Send button. Lower-risk changes and what I checkedLower-risk changes worth a quick check
Checked and found fine
Not verified: I didn't run a screen reader on any platform. Video slider and Growl dismiss behavior with assistive tech is also untested. |
|
I ran into a few weird issues, but I don't think they are blockers.
|
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppandroid-native-1.mp4android-native-2.mp4android-native-3.mp4android-native-5.mp4android-native-6.mp4android-native-7.mp4android-native-8.mp4Android: mWeb Chromeandroid-web-1.mp4android-web-2.mp4android-web-3.mp4android-web-5.mp4android-web-6.mp4android-web-7.mp4android-web-8.mp4iOS: HybridAppios-native-1.movios-native-2.movios-native-3.movios-native-4.movios-native-5.movios-native-6.movios-native-7.movios-native-8.moviOS: mWeb Safariios-web-1.movios-web-2.movios-web-3.movios-web-5.movios-web-6.movios-web-7.movios-web-8.movMacOS: Chrome / Safarimac-web-1.movmac-web-2.movmac-web-3.movmac-web-5.movmac-web-6.movmac-web-7.movmac-web-8.mov |
|
Yeah, I also think they're just edge cases. And since they are the same in production, I don't think it was caused by the bump. Alternatively, we could create another issue to investigate these edge cases |
|
@collectioneur please resolve the conflicts, then I'll complete the checklist soon. |
|
NAB:
nab-4.mp4 |
|
🚧 mountiny has triggered a test Expensify/App build. You can view the workflow run here. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
🚀 Deployed to staging by https://github.com/mountiny in version: 9.5.2-0 🚀
|
|
No help site update is needed. This PR upgrades |
|
🚀 Deployed to production by https://github.com/Julesssss in version: 9.5.2-4 🚀
Bundle Size Analysis (Sentry): |
7 participants
Explanation of Change
Bumps
react-native-gesture-handlerfrom 2.32.0 to 3.3.0. This is one of the library upgrades that has to land before the RN 0.88 / Expo SDK 58 bump (#101427). Gesture Handler declaresreact-native: *, so it lands on the RN 0.86 / Expo SDK 57 stack we run today.The issue targets
~3.2.1, the version Expo SDK 58 bundles today. 3.3.0 is the latest 3.x release and has the same API as 3.2.x, so the RN 0.88 / Expo 58 PR can keep it or align it with whatever SDK 58 ships at release.What changes in this PR:
package.json/package-lock.json:react-native-gesture-handler2.32.0 → 3.3.0. Gesture Handler 3 no longer depends on@egjs/hammerjsandhoist-non-react-statics, so both (and@types/hammerjs) drop out of the lockfile.MultiGestureCanvas/usePanGesture.ts,usePinchGesture.ts,useTapGestures.ts,AvatarCropModal/ImageCropView.tsx,AttachmentCarouselView/index.tsx): in 3.x the builder API types are exported asLegacyPanGesture,LegacyPinchGesture,LegacyTapGestureandLegacyComposedGesture, because the old names now belong to the new hook-based API. No runtime change,Gesture.Pan()& co. still return the same objects.config/eslint/eslint.seatbelt.tsv: 3.x marks the whole builder API (Gesture.*,GestureType,Legacy*types,PanGestureChangeEventPayload) as@deprecatedin favor of the hook-based API (usePanGesture,useTapGesture, ...). The 82 resulting@typescript-eslint/no-deprecated/<Symbol>violations in 24 files are grandfathered in the seatbelt baseline so this PR stays a version bump. Migrating those files to the hook API is a larger change to the gesture code (attachment carousel,MultiGestureCanvas, crop views, charts, modals) and will follow in a separate PR.ScrollView/FlatList/TextInputexports now resolve to the new 3.x components (the old wrappers moved toLegacy*and are deprecated). The 6 call sites (onboarding purpose and personal track goal pages, Travel terms step, autocomplete suggestions,DraggableListandTimePickerref types) typecheck unchanged and are covered by the tests below.ios/Podfile.lock(standalone NewDot) and the HybridAppPodfile.lockin the companion Mobile-Expensify PR:RNGestureHandler3.3.0, its newRNWorklets (>= 0.8.0)dependency (the 3.x podspec builds againstreact-native-workletswhen it is installed) and the spec checksum.None of the APIs removed in 3.0 (
gestureHandlerRootHOC,RNGestureHandlerEnabledRootView, the*GestureHandlercomponents) are used in App, and there are no patches for the library.Where we use the library:
GestureHandlerRootViewwraps the app (App.tsx) and the crop / feature training views.Gesture+GestureDetectordrive the attachment carousel andMultiGestureCanvas(pinch, pan, double tap), the avatar and receipt crop views, the video player progress bar and volume slider, modal swipe-to-dismiss (ReanimatedModal), Growl fling-to-dismiss, the composer send button, camera tap-to-focus on the Scan step, the GPS trip edit slider, chart tap / hover, and horizontal HTML table scrolling in chat.Jest mocks in
jest/setup.tsTwo Gesture Handler 3 changes broke 83 tests in 11 Jest suites and the
ReportActionComposeReassure test, sojest/setup.tsgets two mocks:GestureHandlerRootViewContextalways reports a root view. In 3.xGestureDetectorthrows in__DEV__when there is noGestureHandlerRootViewabove it (2.x only logged a warning). Our tests render screens on their own, outsideApp.tsx, so every test that renders a gesture failed withGestureDetector must be used as a descendant of GestureHandlerRootView. The mock targetsreact-native-gesture-handler/lib/module/GestureHandlerRootViewContextbecause Jest resolves the package throughmain(lib/module); thesrcpaths in the stack traces come from source maps.useComposedEventHandler,useHandlerandisSharedValueare added to the Reanimated mock. The new RNGHScrollView/FlatListwrappers (onboarding, Travel terms, distance waypoints) render a detector that calls these three functions, andreact-native-reanimated/mockdoes not provide them. They are hand-written stand-ins:useComposedEventHandlerreturns a no-op like the mockeduseEvent,useHandlerreturns the same{context, doDependenciesDiffer}shape as the real hook, andisSharedValuerecognizes the shared values created by the mockeduseSharedValue.Fixed Issues
$ #101442
PROPOSAL: N/A
MOBILE-EXPENSIFY: https://github.com/Expensify/Mobile-Expensify/pull/14147
Tests
@and then:smto open the mention and emoji suggestions. Scroll the suggestion list and pick an item. Verify the list scrolls and the selected item is inserted.type:expense group-by:category view:bar,type:expense group-by:merchant view:pie,type:expense group-by:month view:line). Tap a bar / slice / point and verify it opens Spend filtered by that category / merchant / month. Tap an empty area of the chart and verify nothing happens. Verify the page still scrolls vertically over the chart. On web, hover the bars / slices and verify the tooltip follows the cursor.Offline tests
N/A — dependency version bump with no behavioral changes. Gesture handling does not depend on the network.
QA Steps
Same as tests
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectionAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Screen.Recording.2026-10-01.at.13.31.16.compressed.mp4
Android: mWeb Chrome
iOS: Native
Screen.Recording.2026-10-01.at.12.59.41.compressed.mp4
iOS: mWeb Safari
MacOS: Chrome / Safari
Screen.Recording.2026-10-01.at.13.41.24.compressed.mp4