Migrate InteractionManager - (Newly added usages) part 3#94946
Conversation
collectioneur
changed the title
collectioneur
changed the title
| InteractionManager.runAfterInteractions(() => { | ||
| htmlDivElementRef(submitContainerRef).current?.scrollIntoView?.({behavior: 'smooth', block: 'end'}); | ||
| }); | ||
| htmlDivElementRef(submitContainerRef).current?.scrollIntoView?.({behavior: 'smooth', block: 'end'}); |
There was a problem hiding this comment.
In the other cases, we added a transaction tracker, but here we only removed the Interaction Manager and aren't adding anything in its place. Is this definitely the expected behavior?
There was a problem hiding this comment.
I tried to reproduce the bug that was originally fixed by adding this InteractionManager instance, but I couldn't trigger it anymore. Since it's been a while since we first encountered it, I think it’s safe to remove it here
| callback: () => { | ||
| TransitionTracker.runAfterTransitions({ | ||
| waitForUpcomingTransition, | ||
| callback: () => { |
There was a problem hiding this comment.
I'm wondering if there might be a simpler way to do this. The double nesting of the transaction tracker and a callback returning another callback feel a bit complex at first glance.
There was a problem hiding this comment.
`function cleanupAfterExpenseCreate({draftTransactionIDs, linkedTrackedExpenseReportAction, waitForUpcomingTransition = false}: CleanupAfterExpenseCreateParams) {
const removeDrafts = () => removeDraftTransactionsByIDs(draftTransactionIDs);
if (waitForUpcomingTransition) {
// Navigation is about to fire up to two sequential transitions (fullscreen replace -> modal dismiss).
// Wait for the first, then wait again for a possible second, so drafts are cleared only after everything
// has settled and never mid-animation.
TransitionTracker.runAfterTransitions({
waitForUpcomingTransition: true,
callback: () => TransitionTracker.runAfterTransitions({waitForUpcomingTransition: true, callback: removeDrafts}),
});
} else {
// No navigation following: just defer past any in-flight transition (e.g. the modal dismiss).
TransitionTracker.runAfterTransitions({waitForUpcomingTransition: false, callback: removeDrafts});
}
if (linkedTrackedExpenseReportAction?.childReportID) {
const trackReport = Navigation.getReportRouteByID(linkedTrackedExpenseReportAction.childReportID);
if (trackReport?.key) {
Navigation.removeScreenByKey(trackReport.key);
}
}
}`
Maybe we could move in this direction instead. I'm not sure whether what I wrote would behave exactly the same, but my main concern is the deep nesting of callbacks and transaction trackers. This approach removes one level of nesting, and perhaps we could flatten it even further.
There was a problem hiding this comment.
I’ve added a more detailed comment in this section explaining why the nested TransitionTracker is needed. I believe we still have to keep this workaround for now.
| linkedTrackedExpenseReportAction?: OnyxEntry<ReportAction>; | ||
| /** Pass true when navigation will be dispatched right after this call (defers cleanup past all transitions). | ||
| * Pass false when there is no upcoming navigation (cleanup runs after current transitions or immediately). */ | ||
| waitForUpcomingTransition?: boolean; |
There was a problem hiding this comment.
If waitForUpcomingTransition is set to true, we call the transaction tracker, which then returns a callback that also waits for the transaction tracker. Doesn't that mean we're effectively waiting for two transitions? The name suggests we're only waiting for a single upcoming transition.
There was a problem hiding this comment.
Yep, I’ve also explained this in a comment in this file, let me know if this makes it clearer!
|
LGTM! |
melvin-bot
Bot
requested review from
Pujan92,
flaviadefaria and
ikevin127
and removed request for
a team
|
@Pujan92 @ikevin127 One of you needs to 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] |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c03dd1d4c3
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| waitForUpcomingTransition, | ||
| callback: () => { | ||
| TransitionTracker.runAfterTransitions({ | ||
| waitForUpcomingTransition, |
There was a problem hiding this comment.
When cleanupAndNavigateAfterExpenseCreate passes waitForUpcomingTransition: true, this inner wait always waits up to CONST.MAX_TRANSITION_START_WAIT_MS for a second transition even for a normal single-transition submit. During that extra second, the user can open a new expense; those flows reuse CONST.IOU.OPTIMISTIC_TRANSACTION_ID for the fresh draft, and the delayed removeDraftTransactionsByIDs(draftTransactionIDs) from the previous submit can then remove the new draft and wipe the newly entered fields. Please only wait for a second transition when one is actually scheduled, or clean up before a new draft can reuse the same ID.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Yep, good catch - the nested inner waitForUpcomingTransition was waiting up to 1s for a second transition that never comes on single dismiss. Removed it; cleanup now runs right after the one real transition. Manually tested inbox + workspaces on wide/narrow, including immediately starting a new expense after submit - new draft doesn't get wiped.
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppUploading Simulator Screen Recording - iPhone 15 Pro - 2026-07-04 at 19.17.02.mov… iOS: mWeb SafariSimulator.Screen.Recording.-.iPhone.15.Pro.-.2026-07-04.at.18.25.15.movMacOS: Chrome / SafariScreen.Recording.2026-07-04.at.19.34.49.movScreen.Recording.2026-07-04.at.19.22.20.mov |
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
| linkedTrackedExpenseReportAction?: OnyxEntry<ReportAction>; | ||
| /** Pass true when navigation will be dispatched right after this call (defers cleanup past the upcoming transition). | ||
| * Pass false when there is no upcoming navigation (cleanup runs after current transitions or immediately). */ | ||
| waitForUpcomingTransition?: boolean; |
There was a problem hiding this comment.
| waitForUpcomingTransition?: boolean; | |
| shouldWaitForUpcomingTransition?: boolean; |
There was a problem hiding this comment.
Updated, thanks!
|
🚧 roryabraham has triggered a test Expensify/App build. You can view the workflow run here. |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
🚀 Deployed to staging by https://github.com/roryabraham in version: 9.4.29-0 🚀
|
|
🤖 No help site changes required. I reviewed the changes in this PR against the help site content under Why: This is a purely internal timing refactor. It swaps Details
There are no new or renamed features, tabs, settings labels, workflows, or buttons — nothing that any help article describes. No draft PR was created because there is nothing to document. Since no docs changes are needed, I did not open a draft help site PR. |
|
🚀 Deployed to staging by https://github.com/roryabraham in version: 9.4.31-0 🚀
|
|
🤖 No help site changes required. I reviewed the changes in this PR against the help site content under WhyThis PR is a purely internal refactor with no user-facing or documented-behavior change:
I also searched No feature names, tab names, settings labels, or buttons are introduced or renamed, so there is nothing to verify against the live UI. |
|
🚀 Deployed to production by https://github.com/grgia in version: 9.4.31-0 🚀
Bundle Size Analysis (Sentry): |
7 participants
Explanation of Change
Replaces
InteractionManager.runAfterInteractionswithTransitionTracker.runAfterTransitionsincleanupAfterExpenseCreateandBaseLoginForm. Adds awaitForUpcomingTransitionflag to correctly defer draft-transaction cleanup past all sequential transitions (e.g. fullscreen replace + modal dismiss) that follow an expense submit.Fixed Issues
$ #71913
$ #83071
Tests
Offline tests
N/A
QA Steps
Same as tests
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)Avatar, 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
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
test 3:
Screen.Recording.2026-06-29.at.16.36.32.mov
Screen.Recording.2026-06-29.at.16.41.09.mov
MacOS: Chrome / Safari
test 1 and 2:
Screen.Recording.2026-06-29.at.17.42.29.mov