Remove runAfterInteractions on handlePreexistingReport#74370
Remove runAfterInteractions on handlePreexistingReport#74370s77rt wants to merge 4 commits intoExpensify:mainfrom
Conversation
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
mountiny
left a comment
There was a problem hiding this comment.
Thanks for that, @TMisiukiewicz Is there a way we could try to measure a diff in the performance here maybe using the applause tester account compared to main/ staging?
|
🚧 @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, Desktop, and Web. Happy testing! 🧪🧪
|
|
@s77rt Can you please fix conflicts? |
|
Resolved |
|
@suneox can you please do the checklist here? |
|
PR doesn’t need product input as a code clean-up PR. Unassigning and unsubscribing myself. |
TMisiukiewicz
left a comment
There was a problem hiding this comment.
Thank you for handling this!
|
|
||
| // eslint-disable-next-line @typescript-eslint/no-deprecated | ||
| InteractionManager.runAfterInteractions(() => { | ||
| if (report.preexistingReportID) { |
There was a problem hiding this comment.
[nitpick] We already have this check in handlePreexistingReport so I'm not sure if we should do that here
There was a problem hiding this comment.
True, just because this is done in a loop I thought it may be better to do this early check here to avoid the function call? Not sure how effective in terms of performance this is
There was a problem hiding this comment.
probably doesn't matter, but you're right we can leave that 👌
There was a problem hiding this comment.
Could you add a comment to make it clear why we do it here too?
| InteractionManager.runAfterInteractions(() => { | ||
| navigate(reportRoute); | ||
| }); | ||
| navigate(reportRoute); |
There was a problem hiding this comment.
I think this change is too much for us at the scope of this PR 🤔 dismissModalWithReport is used in many places so this removal should be tested much better on slower devices too (or with CPU throotling on)
There was a problem hiding this comment.
@s77rt is this change necessary to fix the issue?
There was a problem hiding this comment.
Yes it's necessary, see #74364 (comment) and other comments.
TL:DR: This code was used to fix a bug that's no longer reproducible. It's necessary to remove it because that added delay is what had us add this delay in the first place (chain effect)
There was a problem hiding this comment.
Ok, I can confirm the removal of that InteractionManager doesn't break the current functionality, it works faster but looks a bit differently
4x CPU throttling
- with InteractionManager
Screen.Recording.2025-11-06.at.13.14.46.mov
- without InteractionManager
Screen.Recording.2025-11-06.at.13.12.20.mov
There was a problem hiding this comment.
It looks much better without throttling though
- with InteractionManager
Screen.Recording.2025-11-06.at.13.21.29.mov
- withoutInteractionManager
Screen.Recording.2025-11-06.at.13.20.07.mov
There was a problem hiding this comment.
@mountiny I think we need your (or maybe design team) blessing on this change
There was a problem hiding this comment.
How does it look on narrow/ mobile view? cc @Expensify/design
There was a problem hiding this comment.
The flow is web-only. We have this condition for narrow layout few lines above
There was a problem hiding this comment.
I do think it is better and more in-line with out style that it first animates to close the RHP before we navigate away. How can we keep that without introducing the perf regression
There was a problem hiding this comment.
Discussed with @war-in That this is not that simple to get around to make sure the navigation events happen in the proper order. So I tend to accept the state without waiting for the animation now given the performance gains are really noticeable and work on ways to achieve the nice ux animations in a fast follow up.
So that would mean we will have this experiencing when dismissing the modal and navigating in wide web or desktop
https://github.com/user-attachments/assets/569714f5-ea55-4658-a4e3-6d4a99ef2c5d
|
Closing in favor #74567 |
Explanation of Change
Fixed Issues
$ #74364
PROPOSAL:
Tests
NonechatReportwithnullApp/src/libs/actions/IOU.ts
Line 3965 in 74c3c35
Screen.Recording.2025-11-05.at.7.43.29.PM.mov
Offline tests
n/a
QA Steps
NonePR 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
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
MacOS: Desktop