Improve performance of inbox page loading#76012
Conversation
So far we have a 10% gain compared to the main version, and I’m continuing to work on other things that could be improved. |
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.
|
|
After comparing data in the Applause account, I can see that it’s possible to achieve an inbox-open gain of around 75% of the time. However, this requires combining two approaches: using useMemo and screen preloading. As shown in the attached screenshot, this delivers the best results. 
|
…of_inbox # Conflicts: # src/libs/Navigation/AppNavigator/usePreloadFullScreenNavigators.ts
|
I managed to get the compiler running in reportSvcrean, add memoization to several components, and activate preload, which produced the following results. WEB: 
IOS: This kind of render-time reduction is even more noticeable for the user in real usage, thanks to the benefits of preload. These improvements are visible in practice but require computations that extend the measured render time. This gives us roughly a 70% gain on the web and about a 60% gain on native platforms. All measurements were taken on the Aplause account. Now I need to check whether there are any bugs, and I’ll try to fix them if they appear. But we’re on a very good path toward a massive performance improvement on this screen |
|
It looks like the bugs that caused the previous preload-inbox rollbacks no longer occur. I’m now working on recording the videos and reviewing everything to make sure there isn’t a case that would still break it. |
sumo-slonik
force-pushed
the
feature/kuba-nowakowski/improve_performance_of_inbox
branch
from
179e0d9 to
5ac39ed
Compare
melvin-bot
bot
requested review from
situchan and
trjExpensify
and removed request for
a team and
trjExpensify
| const isFocused = useIsFocused(); | ||
| const prevIsFocused = usePrevious(isFocused); | ||
| const firstRenderRef = useRef(true); | ||
| const [firstRender, setFirstRender] = useState(true); |
There was a problem hiding this comment.
Can you explain the reason why use state instead of ref?
There was a problem hiding this comment.
Thanks to replacing useRef with useState, we were able to enable the React Compiler. The benefit we previously got from useRef was minimal anyway, because we were already using setState below, which triggered a re-render. Now that ref is no longer used inside the effect, the React Compiler can properly memoize the component, resulting in better performance.
There was a problem hiding this comment.
Though this ref was introduced 2 years ago, last usage was 10 months ago.
We need to make sure that this removal doesn't reintroduce #55251
There was a problem hiding this comment.
This bug no longer occurs because newly created expenses are now displayed in the new report view, and in that view the issue does not appear when removing expense.
Screen.Recording.2025-12-04.at.10.20.34.mov
There was a problem hiding this comment.
Screen.Recording.2025-12-04.at.10.48.17.mov
| // Currently the Inbox, Workspaces and Account tabs are preloaded, while Search is not preloaded due to its potential complexity. | ||
| const TABS_TO_PRELOAD = [NAVIGATION_TABS.HOME, NAVIGATION_TABS.SETTINGS, NAVIGATION_TABS.WORKSPACES]; |
There was a problem hiding this comment.
| // Currently the Inbox, Workspaces and Account tabs are preloaded, while Search is not preloaded due to its potential complexity. | |
| const TABS_TO_PRELOAD = [NAVIGATION_TABS.HOME, NAVIGATION_TABS.SETTINGS, NAVIGATION_TABS.WORKSPACES]; | |
| const TABS_TO_PRELOAD = [NAVIGATION_TABS.HOME, NAVIGATION_TABS.WORKSPACES, NAVIGATION_TABS.SETTINGS]; |
to be consistent with visual order
| const [isBannerVisible, setIsBannerVisible] = useState(true); | ||
| const [scrollPosition, setScrollPosition] = useState<ScrollPosition>({}); | ||
|
|
||
| const wasReportAccessibleRef = useRef(false); |
There was a problem hiding this comment.
I'm already checking whether this doesn't reproduce. Thanks for finding it, because I have a feeling it might show up, since I think I had checked different cases.
There was a problem hiding this comment.
I’m not sure what the correct behavior should be here. If we try to navigate to a deleted report, should we see the “Oops” screen or not?
Currently, on my branch, the screen is displayed (previously it wasn’t), but I saw an issue where “works as designed” was marked, stating that it should appear.
The message on the page also seems appropriate, since it informs us that we’re trying to navigate to an expense that has already been deleted.
Screen.Recording.2025-12-04.at.10.50.51.mov
It seems to me that it would be best if the @Expensify/design team spoke up about what the correct flow should be here
There was a problem hiding this comment.
If the @Expensify/design team decides it should be an upss page, I’ll keep it. If not, I’ll add navigation wherever they think it’s appropriate in that situation.
There was a problem hiding this comment.
I'm not sure exactly where I would expect to land when I went back to the inbox, but it wouldn't be a not found screen. @dubielzyk-expensify @Expensify/product-pr do y'all have any opinions on where a user should land here?
There was a problem hiding this comment.
From a technical point of view, getting information about whether or not an expense is deleted is the first step to both navigating and showing information that an expense has been deleted. So I can do what you see fit.
There was a problem hiding this comment.
My take would be that and basically just swap the text and illustration to this:
But curious to hear if @dannymcclain agrees. And @Expensify/product-pr 's take too
There was a problem hiding this comment.
I think that text and illustration is much better than a generic one when we know the absolute reason. The cloud guy is in a ton of places, and it's somewhat not as helpful when it's ambiguously positioned (i.e "it could be this, or that, maybe even this!").
That said, I generally don't really like the pattern of returning to an error screen if it can be avoided by removing the deleted report from the stack. So I think my preference would be something that accommodates that, really.
There was a problem hiding this comment.
I'm into that empty state because like Tom mentioned it's much more clear. But I definitely see his point about us purposely landing someone on an "error" page. I'm just having a hard time thinking of a sensible place to land someone.
There was a problem hiding this comment.
I currently have a higher-priority task related to SAML. As soon as I’m done with it (most likely on Tuesday), I’ll take care of the fixes for the new empty state
| const isFocused = useIsFocused(); | ||
| const prevIsFocused = usePrevious(isFocused); | ||
| const firstRenderRef = useRef(true); | ||
| const [firstRender, setFirstRender] = useState(true); |
There was a problem hiding this comment.
Though this ref was introduced 2 years ago, last usage was 10 months ago.
We need to make sure that this removal doesn't reintroduce #55251
|
Codex Review: Something went wrong. Try again later by commenting “@codex review”. ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
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". |
| // eslint-disable-next-line react-hooks/set-state-in-effect | ||
| setWasDeleted(false); |
There was a problem hiding this comment.
Please add comment why setting state here is safe.
| // eslint-disable-next-line react-hooks/set-state-in-effect | ||
| setWasDeleted(true); |
| setWasDeleted(true); | ||
| }, [currentReportID, isOptimisticDelete, userLeavingStatus]); | ||
|
|
||
| return useMemo(() => ({wasDeleted, parentReportID}), [wasDeleted, parentReportID]); |
There was a problem hiding this comment.
Do we need useMemo here? React compiler doesn't already memoize it?
There was a problem hiding this comment.
Unfortunately, we use this in the report screen, which in the current version of the code is not compiled, so useMemo is justified here.
^ |
|
@codex review |
| * Also stores the parentReportID from when the report was accessible, so it can be used for navigation | ||
| * after the report is deleted. | ||
| */ | ||
| function useReportWasDeleted( |
There was a problem hiding this comment.
Why do we need this hook? It seems like we use it to redirect the user to the parent report/Concierge when a report is deleted, but based on this comment, we should just show the Not found page.
There was a problem hiding this comment.
I see, thanks for the context.
|
Codex Review: Something went wrong. Try again later by commenting “@codex review”. ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
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". |
|
@codex review |
|
Codex Review: Something went wrong. Try again later by commenting “@codex review”. ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
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". |
|
@codex review |
|
Codex Review: Something went wrong. Try again later by commenting “@codex review”. ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
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". |
|
Codex isn't willing to review this PR 😅 |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
It seems to me that it’s ready |
|
✋ 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/luacmartins in version: 9.3.8-0 🚀
|
|
🚀 Deployed to production by https://github.com/roryabraham in version: 9.3.8-3 🚀
|
10 participants
Managed to activate the compiler in the Reports screen by stopping the existing useRef and useState that were already there, simplifying the logic. Additionally, I was able to implement memoization in other places and fix the preload of the inbox tab, which results in a 70% performance gain on web loading and 60% on mobile native for a heavy account.
Additionally, the redirect from a non-existent report has been removed, and we are now showing the "not found" state instead.
Explanation of Change
Fixed Issues
$ #71587
$ #74613
$ #73993 <- this caused the previous revert
$ #73691
PROPOSAL:
Tests
Open any report.
Skeleton loader should be visible while messages are being fetched.
Skeleton loader should disappear once data is loaded and be replaced with report actions.
If no data is fetched, there should be an empty state component with "No activity yet" information.
Check whether the inbox loads correctly when navigating from each tab.
Test to verify the bug does not reproduce:
Expected result:
There is no infinite loading of skeletons.
Offline tests
unesesary
QA Steps
same as test
// TODO: These must be filled out, or the issue title must include "[No QA]."
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
Android: Native
Screen.Recording.2025-12-03.at.13.36.52.mov
Android: mWeb Chrome
Screen.Recording.2025-12-03.at.13.43.07.mov
iOS: Native
Screen.Recording.2025-12-03.at.13.48.47.mov
iOS: mWeb Safari
Screen.Recording.2025-12-03.at.11.48.37.mov
MacOS: Chrome / Safari
Screen.Recording.2025-12-03.at.11.44.30.mov