Migration navigation from InteractionManager to TransitionTracker#82587
Conversation
Codecov Report❌ Looks like you've decreased code coverage for some files. Please write tests to increase, or at least maintain, the existing level of code coverage. See our documentation here for how to interpret this table.
|
…or improved readability and error handling
…for improved readability
…s and improve formatting
…w guidelines for using Navigation with waitForTransition. Update various files to reflect the new categorization
|
@blazejkustra That's about it in terms of code review ☝️ 🧪 Test Coverage %The PR introduces no new unit or integration tests - the changed surface area includes:
Estimated coverage added by author: Suggested tests to add: // TransitionTracker.test.ts
describe('runAfterTransitions', () => {
it('fires callback immediately when no active transitions', () => {
const cb = jest.fn();
TransitionTracker.runAfterTransitions(cb);
expect(cb).toHaveBeenCalledTimes(1);
});
it('defers callback until endTransition is called', () => {
TransitionTracker.startTransition();
const cb = jest.fn();
TransitionTracker.runAfterTransitions(cb);
expect(cb).not.toHaveBeenCalled();
TransitionTracker.endTransition();
expect(cb).toHaveBeenCalledTimes(1);
});
it('cancel() prevents deferred callback from firing', () => {
TransitionTracker.startTransition();
const cb = jest.fn();
const handle = TransitionTracker.runAfterTransitions(cb);
handle.cancel();
TransitionTracker.endTransition();
expect(cb).not.toHaveBeenCalled();
});
it('fires after safety timeout if endTransition never called', () => {
jest.useFakeTimers();
TransitionTracker.startTransition();
const cb = jest.fn();
TransitionTracker.runAfterTransitions(cb);
jest.advanceTimersByTime(1001);
expect(cb).toHaveBeenCalledTimes(1);
jest.useRealTimers();
});
});🔄 Will be moving forward to manual testing... |
|
@ikevin127 I’ll investigate the callback issues in |
|
🟢 Completed PR Reviewer Checklist, manual tests look good.
@collectioneur Please let me know when your changes are applied so I can take a final pass at the PR 🙌 🔥 Manual Edge Case / Destructive Regression Tests Report✅ 1. Double-tap dismiss: Rapidly double-tap "Close" on an RHP. Confirm only one DISMISS_MODAL is dispatched, and 📈 PR Strengths
|
…s before navigation transition
|
@ikevin127 I've addressed all the issues and included the necessary tests. It's ready for another review whenever you have a chance 👍 |
ikevin127
left a comment
There was a problem hiding this comment.
🟢 LGTM - Thanks for the changes!
roryabraham
left a comment
There was a problem hiding this comment.
looks great. Thanks @blazejkustra and everyone else who contributed to this!
|
🚧 @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.27-0 🚀
|
|
Deploy Blocker #83727 was identified to be related to this PR. |
|
Deploy Blocker #83755 was identified to be related to this PR. |
|
🚀 Deployed to production by https://github.com/blimpich in version: 9.3.27-8 🚀
|
Explanation of Change
This PR introduces the TransitionTracker and migrates the first batch of
InteractionManagerusages in navigation code to use it.TransitionTracker (
src/libs/Navigation/TransitionTracker.ts) is a new module that explicitly tracks active transitions (navigation, modal, keyboard, focus) using a counted map. It exposesstartTransition/endTransitionto mark transition boundaries, andrunAfterTransitionsto queue callbacks that fire once all transitions are idle. A safety timeout (1s) auto-ends transitions that are never explicitly closed.On top of TransitionTracker, existing APIs gain transition-aware options:
Navigation.navigate,goBack, anddismissModalnow acceptwaitForTransition(defer the navigation until ongoing transitions finish) andafterTransition(run a callback after the triggered transition completes). This replaces the old pattern of wrapping navigation calls inInteractionManager.runAfterInteractions.dismissModalWithReportusesafterTransitioninstead of the oldDeviceEventEmitter-basedMODAL_EVENTS.CLOSEDcallback pattern. TheMODAL_EVENTS.CLOSEDconstant and its associatedDeviceEventEmitteremissions inBaseModalandRightModalNavigatorare removed.KeyboardUtils.dismissnow accepts{ afterTransition, shouldSkipSafari }instead of a bare boolean, and tracks keyboard show/hide as a transition type.ScreenLayout(src/libs/Navigation/PlatformStackNavigation/ScreenLayout.tsx) is a new component wired into the web stack navigator viascreenLayoutprop. It listens totransitionStart/transitionEndevents to feed TransitionTracker.ReanimatedModalnow callsTransitionTracker.startTransition('modal')/endTransition('modal')alongside the existingInteractionManagerhandles.Concrete migration examples in this PR:
WorkspaceCategoriesSettingsPageandWorkspaceCreateTagPagereplaceKeyboard.dismiss()+InteractionManagerwithKeyboardUtils.dismiss({ afterTransition }).WorkspaceInviteMessageComponent,NewChatPage,TransactionReceiptModalContent, andReport/index.tsreplacedismissModal({ callback })withdismissModal({ afterTransition }).Fixed Issues
$ #71913
PROPOSAL:
Tests
Offline tests
N/A
QA Steps
Same as tests
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectioncanBeMissingparam foruseOnyxtoggleReportand 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
iOS: Native
Start new chat:
stat-new-chat-ios.mov
Workspace invite:
workspace-invite-ios.mov
Create group:
create-group-ios.mov
Replace receipt:
replace-receipt-ios.mov
Keyboard behaviour:
keyboard-ios.mov
MacOS: Chrome / Safari
Start new chat:
https://github.com/user-attachments/assets/8c36c37f-d457-41ea-8c0c-8b916e129e88
Workspace invite:
https://github.com/user-attachments/assets/ee5d5813-4546-4cef-8464-b4f85261bea6
Create group:
https://github.com/user-attachments/assets/9af3067b-d93b-42c0-97a2-1ab7d03e7cfa
Replace receipt:
https://github.com/user-attachments/assets/99405a86-deb1-4f3b-b229-9fcd23ebfcf4