perf-scan-page/Add preloading for FAB icons#85964
perf-scan-page/Add preloading for FAB icons#85964cristipaval merged 4 commits intoExpensify:mainfrom
Conversation
|
@ikevin127 Please 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] |
melvin-bot
bot
requested review from
trjExpensify
and removed request for
a team
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
|
||
| useEffect(() => { | ||
| loadIllustrationsChunk(); | ||
| loadExpensifyIconsChunk(); |
There was a problem hiding this comment.
❌ CONSISTENCY-6 (docs)
loadIllustrationsChunk() and loadExpensifyIconsChunk() both return promises that re-throw errors from their internal .catch() handlers. Calling them fire-and-forget without a .catch() means any chunk loading failure (e.g., network error) will produce an unhandled promise rejection, which can cause yellow/red box errors on React Native or crash the app in production.
Since these are best-effort preload calls, add a no-op .catch() to suppress unhandled rejections:
useEffect(() => {
loadIllustrationsChunk().catch(() => {});
loadExpensifyIconsChunk().catch(() => {});
}, []);Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
There was a problem hiding this comment.
@rinej This is a valid concern, suggested solution seems accurate ✅
|
PR doesn’t need product input as a performance PR. Unassigning and unsubscribing myself. |
![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: ddafa65c6a
ℹ️ 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".
| useEffect(() => { | ||
| loadIllustrationsChunk(); | ||
| loadExpensifyIconsChunk(); |
There was a problem hiding this comment.
loadIllustrationsChunk() and loadExpensifyIconsChunk() both rethrow on import errors (src/components/Icon/IllustrationLoader.ts:34-36, src/components/Icon/ExpensifyIconLoader.ts:36-38). Calling them fire-and-forget here means a stale client or transient chunk/network failure will now raise an unhandledrejection as soon as the tab bar mounts, even though the normal lazy-asset hooks already catch these failures and fall back safely. Please swallow or log the preload failure in this effect instead of letting it escape.
Useful? React with 👍 / 👎.
| const {translate} = useLocalize(); | ||
|
|
||
| useEffect(() => { | ||
| loadIllustrationsChunk(); |
There was a problem hiding this comment.
BaseFloatingCameraButton is mounted from NavigationTabBar (src/components/Navigation/NavigationTabBar/index.tsx:280-287), so this line now eagerly imports the shared illustrations bundle for every shell render, not just when the user opens Scan. That chunk (src/components/Icon/chunks/illustrations.chunk.ts) contains hundreds of unrelated SVGs, so users who never tap the FAB still pay the extra download/parse cost. In practice this undoes the lazy-loading added by useMemoizedLazyIllustrations and makes the initial narrow-layout experience heavier.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
@rinej This is a valid concern:
- Reconsider the entire eager preloading strategy: The cost (loading hundreds of unused assets for every user) outweighs the benefit (342ms improvement only when FAB is tapped)
If preloading is truly needed, load only specific icons required for scan page, not entire chunks.
Reviewer Checklist
Screenshots/VideosScreen.Recording.2026-03-23.at.15.54.12.mov |
ikevin127
left a comment
ikevin127
left a comment
There was a problem hiding this comment.
🟢 LGTM - The implementation is idiomatic and follows established patterns.
Except the 2 valid AI reviewer comments from above, the lazy-loaded hooks are designed for "load when needed", but the BaseFloatingCameraButton is "warming the cache" before navigation - this is a legitimate optimization pattern ✅
|
Thanks for the review! For the 1st comment it's a good catch, I added the catch to both calls For the "load only specific icons" thread: I see the concern here. Technically we could create separate scan-specific webpack chunks containing only the ~12 icons the scan page actually needs (4 illustrations + 8 expensify icons), and preload those instead. But that would require creating new chunk files, new loaders, new hooks in useLazyAsset, updating all 5 scan page components to use them, it's a significant amount of new logic for a relatively small gain over the current approach. Worth noting that these chunks are already lazily loaded elsewhere in the app (any screen using useMemoizedLazyIllustrations or useMemoizedLazyExpensifyIcons triggers them), so in practice most users will already have them cached before they ever tap the FAB. The preload here just ensures they're warm specifically for the scan page flow, and since it's a low-priority async fetch it won't block rendering or compete with critical resources. Please let me know if we want to go with the scan-specific chunk route if you feel strongly about it - just want to flag it adds complexity and new potential maintenance issues for what's essentially the same perf win. |
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
🚧 @cristipaval 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/cristipaval in version: 9.3.44-0 🚀
Bundle Size Analysis (Sentry): |
|
🚀 Deployed to staging by https://github.com/cristipaval in version: 9.3.47-0 🚀
Bundle Size Analysis (Sentry): |
|
🚀 Deployed to production by https://github.com/grgia in version: 9.3.48-2 🚀
|
…unty for pr Expensify#85964 Signed-off-by: hinzwilliam52-ship-it <hinzwilliam52@gmail.com>
5 participants
Explanation of Change
When the Camera FAB mounts, we now preload the lazy-loaded icon and illustration chunks (
loadExpensifyIconsChunkandloadIllustrationsChunk) via auseEffect. Since the FAB is visible for several seconds before the user taps it, the chunks are already cached by the time the scan page mounts.Fixed Issues
$ #83502
PROPOSAL: #83502 (comment)
Tests
Offline tests
QA Steps
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
iOS: mWeb Safari
MacOS: Chrome / Safari