Repository navigation
Migrate InteractionManager - batch 1 - #88603
roryabraham merged 27 commits into
Conversation
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
… with transitiontracker directly
| if (pendingCloseRef.current) { | ||
| pendingCloseRef.current = false; | ||
| pendingCloseModalAction(); | ||
| } |
There was a problem hiding this comment.
Do we need this ref here? Why can't we trigger the action when shouldCloseOnConfirm is true? 🤔
There was a problem hiding this comment.
This ref is there so that we can check whether the modal was closed using the closeModal callback.
shouldCloseOnConfirm only indicates whether modal should be closed after pressing submit, while the action should trigger anytime the modal closes via the closeModal (which is called either by pressing submit with shouldCloseOnConfirm = true or by user closing the modal e.g. with a backdrop press).
There are two cases which don't use closeModal and should not trigger the action - when the modal is closed by switching the isModalDisabled flag and by pressing help button with shouldCallOnHelpWhenModalHidden = true.
In both cases, the value of shouldCloseOnConfirm does not affect the outcome.
| InteractionManager.runAfterInteractions(() => { | ||
| validateCodeFormRef.current?.focus?.(); | ||
| }); | ||
| validateCodeFormRef.current?.focus?.(); |
There was a problem hiding this comment.
Is it safe to remove this? from what I see in the ScreenWrapper code, it's wrapped in setTimeout, not our TransitionTracker :/
There was a problem hiding this comment.
the ScreenWrapper waits for the transitionEnd of the entry transition, the setTimeout is only a fallback for iOS where the transitionEnd doesn't trigger sometimes (according to the comment). So from what I understand the difference is that transition tracker guards against all events, while ScreenWrapper only waits for the entry transition end. However, I just noticed that this change is not a part of this batch, but the batch 8 (Input Focus usages), so I'll revert it for now.
There was a problem hiding this comment.
Does this change belong to the current batch? Also, are you sure requestAnimationFrame can be used instead of InteractionManager here?
There was a problem hiding this comment.
Yeah, WorkspacesListPage.tsx belongs to the current batch (it's on the list), but I'm also not sure if requestAnimationFrame works correctly here 🤔
There was a problem hiding this comment.
Right, the requestAnimationFrame doesn't work here as I thought 😅 - I'll change it to transitiontracker directly
| // eslint-disable-next-line @typescript-eslint/no-deprecated | ||
| InteractionManager.runAfterInteractions(() => { | ||
| onClose?.(); | ||
| }); | ||
| onClose?.(); |
There was a problem hiding this comment.
Are you sure it won't break anything? It was added here to fix RHP error
There was a problem hiding this comment.
I tested it and could not reproduce the referenced error - the current flow of editing photo differs as well from the one that had the rhp error, the flow is now split between two modals (cropping and saving) which I believe is also the reason why the bug is not present anymore
collectioneur
left a comment
There was a problem hiding this comment.
I second @war-in comments. Otherwise, great job on this 😄
There was a problem hiding this comment.
Yeah, WorkspacesListPage.tsx belongs to the current batch (it's on the list), but I'm also not sure if requestAnimationFrame works correctly here 🤔
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppSimulator.Screen.Recording.-.iPhone.15.Pro.-.2026-05-14.at.21.43.14.movSimulator.Screen.Recording.-.iPhone.15.Pro.-.2026-05-14.at.21.47.47.movSimulator.Screen.Recording.-.iPhone.15.Pro.-.2026-05-14.at.21.49.15.movSimulator.Screen.Recording.-.iPhone.15.Pro.-.2026-05-14.at.21.52.52.movSimulator.Screen.Recording.-.iPhone.15.Pro.-.2026-05-14.at.21.54.01.movSimulator.Screen.Recording.-.iPhone.15.Pro.-.2026-05-14.at.21.56.26.movSimulator.Screen.Recording.-.iPhone.15.Pro.-.2026-05-14.at.21.55.56.moviOS: mWeb SafariSimulator.Screen.Recording.-.iPhone.15.Pro.-.2026-05-14.at.22.11.44.movSimulator.Screen.Recording.-.iPhone.15.Pro.-.2026-05-14.at.22.13.34.movMacOS: Chrome / SafariScreen.Recording.2026-05-13.at.23.10.48.movScreen.Recording.2026-05-13.at.23.56.51.movScreen.Recording.2026-05-14.at.21.37.43.movScreen.Recording.2026-05-14.at.21.38.39.movScreen.Recording.2026-05-14.at.21.39.15.mov |
| InteractionManager.runAfterInteractions(() => { | ||
| onClose?.(); | ||
| }); | ||
| onClose?.(); |
There was a problem hiding this comment.
Don't we need to use this inside TransitionTracker.runAfterTransitions callback?
There was a problem hiding this comment.
It was previously added to fix an error with RHP that @war-in linked above. The current flow differs from the one shown in the issue (it's now split between two modals), which I believe is what fixed the bug, as I've tested and couldn't reproduce it.
|
@mrejdak Plz fix the conflicts |
|
@mrejdak lint is failing and conflicts again 😢 |
|
@roryabraham can you plz trigger adhoc build here |
Done! |
| "../../src/components/AutoCompleteSuggestions/AutoCompleteSuggestionsPortal/TransparentOverlay/TransparentOverlay.tsx" "no-restricted-syntax" 1 | ||
| "../../src/components/AutoCompleteSuggestions/AutoCompleteSuggestionsPortal/TransparentOverlay/TransparentOverlay.tsx" "react-hooks/refs" 1 | ||
| "../../src/components/AutoCompleteSuggestions/index.tsx" "react-hooks/set-state-in-effect" 1 | ||
| "../../src/components/AutoSubmitModal.tsx" "@typescript-eslint/no-deprecated/InteractionManager.runAfterInteractions" 1 |
There was a problem hiding this comment.
@mrejdak in future, you should not need to manually tighten eslint.seatbelt.tsv. It will happen automatically on merge, and if you let it happen that way then you'll avoid having to resolve conflicts in the ratchet file.
There was a problem hiding this comment.
Will keep that in mind, thanks!
|
🚧 @roryabraham 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! 🧪🧪
|
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to staging by https://github.com/roryabraham in version: 9.3.74-7 🚀
Bundle Size Analysis (Sentry): |
|
After reviewing the changes in this PR, no help site updates are required. This PR is a purely internal code refactoring (Batch 1 of No new features were added, no existing feature names or settings were changed, and no user-facing flows were modified. |
|
🚀 Deployed to production by https://github.com/Beamanator in version: 9.3.74-7 🚀
|
|
Deploy Blocker #90866 was identified to be related to this PR. |
Explanation of Change
Batch 1 of InteractionManager migration
Fixed Issues
$ #71913
$ #83061
PROPOSAL:
Tests
WorkspaceCompanyCardsSettingsPage.tsxSettings->Remove card feedDeleteWorkspaceExpensifyCardDetailsPage.tsxDeactivate cardDeactivateAvatarCropModal.tsxProfileand press the edit avatar iconEditand select "Upload photo"Upload->Saveand verify that both modals closed correctlyAutoSubmitModal.tsxGot itand verify that it closes correctlyFeatureTrainingModal.tsxGot itand verify that it closes correctlyuseFilesValidation.ts+button and selectCreate expenseChoose fileCloseand verify that it closes correctlyPopoverReportActionContextMenu.tsx...on web)Deletefrom the options displayedDeleteand verify that it closes correctly and that the message is removed after the modal disappearsIOURequestStepDescription.tsx(Web)DescriptionDiscard changesmodal opens, pressCanceland verify that the input focuses back correctly.IOURequestStepMerchant.tsx(Web)MerchantDiscard changesmodal opens, pressCanceland verify that the input focuses back correctly.AttachmentModalContainer/index.native.tsx(mobile)2 Tap on attachment preview to open larger preview
ContactMethodDetailsPage.tsxProfileand thenContact methods...button and select "Remove"Canceland verify that the modal closes first and that the input cell focuses back correctlyYes, continueand verify that the modal closes before navigating backWorkspaceCategoriesPage.tsxCategoriesDelete categoriesDeleteand verify that the modal closes correctly, the categories are removed and there that are no categories selectedPolicyDistanceRatesPage.tsxDelete ratesDeleteand verify that the modal closes correctly, the distance rates are removed and there that are no distance rates selectedWorkspacePerDiemPage.tsxPer Diemtab and select at least two rowsDelete rates->DeleteReportFieldsListValuesPage.tsxDelete values->DeleteWorkspaceTagsPage.tsxTagsand select at least two tags (but not all of them)Delete tags->DeleteWorkspaceTaxesPage.tsxDelete rates->DeleteWorkspaceWorkflowsApprovalsEditPage.tsxDelete->DeleteWorkspaceMembersPage.tsxMembersand select at least twoRemove membersand thenRemoveWorkspacesListPage.tsx...on the last one and selectDuplicate workspaceNextand verify that the list correctly scrolls to the new workspaceReportParticipantsPage.tsxRemove membersand thenRemoveRoomMembersPage.tsx#roomwith >12 people (so that search bar appears)Offline tests
N/A
QA Steps
Same as Tests
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, 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.ScrollViewcomponent to make it scrollable when more elements are added to the page.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
Android: mWeb Chrome
iOS: Native
WorkspaceCompanyCardsSettingsPage.tsxbatch1_remove_card_feed.mov
WorkspaceExpensifyCardDetailsPage.tsxbatch1_deactivate_card.mov
AvatarCropModal.tsxbatch1_avatar_crop.mov
useFilesValidation.tsbatch1_file_validation.mov
PopoverReportActionContextMenu.tsxbatch1_popover_remove_comment.mov
AttachmentModalContainer/index.native.tsx(mobile)batch1_attachment_modal_container.mov
ContactMethodDetailsPage.tsxbatch1_contact_details.mov
WorkspaceCategoriesPage.tsxbatch1_workspace_categories.mov
PolicyDistanceRatesPage.tsxbatch1_distance_rates.mov
WorkspacePerDiemPage.tsxbatch1_per_diem.mov
ReportFieldsListValuesPage.tsxbatch1_report_field_list.mov
WorkspaceTagsPage.tsxbatch1_tags.mov
WorkspaceTaxesPage.tsxbatch1_taxes.mov
WorkspaceWorkflowsApprovalsEditPage.tsxbatch1_approver_workflow.mov
batch1_approver_workflow_del.mov
WorkspaceMembersPage.tsxbatch1_members.mov
WorkspacesListPage.tsxbatch1_workspace_list.mov
ReportParticipantsPage.tsxbatch1_group_chat.mov
RoomMembersPage.tsxbatch1_room_members.mov
iOS: mWeb Safari
MacOS: Chrome / Safari
AvatarCropModal.tsxbatch1_avatar_crop_web.mov
AutoSubmitModal.tsxbatch1_auto_submit.mov
FeatureTrainingModal.tsxbatch1_track_training.mov
useFilesValidation.tsbatch1_file_validation__web.mov
PopoverReportActionContextMenu.tsxbatch1_popover_remove_comment_web.mov
IOURequestStepDescription.tsx(Web)batch1_IOU_description_web.mov
IOURequestStepMerchant.tsx(Web)batch1_IOU_merchant_web.mov
ContactMethodDetailsPage.tsxbatch1_contract_details_web.mov
WorkspaceCategoriesPage.tsxbatch1_workspace_categories_web.mov
PolicyDistanceRatesPage.tsxbatch1_distance_rates_web.mov
WorkspacePerDiemPage.tsxbatch1_per_diem_web.mov
ReportFieldsListValuesPage.tsxbatch1_report_field_list_web.mov
WorkspaceTagsPage.tsxbatch1_tags_web.mov
WorkspaceTaxesPage.tsxbatch1_taxes_web.mov
WorkspaceWorkflowsApprovalsEditPage.tsxbatch1_approver_workflow_web.mov
batch1_approver_workflow_del_web.mov
WorkspaceMembersPage.tsxbatch1_members_web.mov
WorkspacesListPage.tsxbatch1_workspace_list_web.mov
ReportParticipantsPage.tsxbatch1_group_chat_web.mov
RoomMembersPage.tsxbatch1_room_members_web.mov