[No QA] Show skeleton in BaseSidebarScreen to avoid mounting heavy SidebarLinks during initial load#84153
Conversation
…ks during initial load
BartekObudzinski
changed the title
BartekObudzinski
changed the title
melvin-bot
bot
requested review from
JmillsExpensify and
abdulrahuman5196
and removed request for
a team
|
@abdulrahuman5196 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
a team,
JS00001 and
Pujan92
and removed request for
a team
![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: 750608312c
ℹ️ 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".
| <View style={[styles.flex1]}> | ||
| <SidebarLinksData insets={insets} /> | ||
| </View> | ||
| <View style={[styles.flex1]}>{shouldShowSkeleton ? <OptionsListSkeletonView shouldAnimate /> : <SidebarLinksData insets={insets} />}</View> |
There was a problem hiding this comment.
This conditional can deadlock app initialization on inbox-first routes: when isLoadingApp is true, SidebarLinksData (and therefore SidebarLinks) never mounts, but SidebarLinks is where useConfirmReadyToOpenApp() is called to resolve the isReadyToOpenApp gate that openApp() waits for before it can finish loading. On startup paths that land in the reports split (for example /inbox or /r/:id) before HomePage mounts, openApp() waits for readiness while this screen keeps rendering only the skeleton, so the sidebar can remain stuck in loading indefinitely.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch — addressed in 32977a6. useConfirmReadyToOpenApp() is now called unconditionally in BaseSidebarScreen so that openApp() can proceed even when the skeleton is shown and SidebarLinksData has not mounted yet. The hook is idempotent, so the duplicate call once SidebarLinks mounts is harmless.
|
@JS00001 I don't know how but two C+ are assigned here by Melvin. Shall I proceed with the review or let @abdulrahuman5196 to do it? |
|
@Pujan92 go for it! |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / SafariScreen.Recording.2026-03-06.at.16.40.50.mov |
|
|
||
| // Once the app finishes loading for the first time, we never show the skeleton again | ||
| // (even if isLoadingApp briefly flips back to true during a reconnect). | ||
| let hasEverFinishedLoading = false; |
There was a problem hiding this comment.
isLoadingApp can briefly flip back to true during a reconnect (e.g. network loss → reconnection). Without this latch, users would see the skeleton flash over an already-loaded LHN, which is a UX regression. hasEverFinishedLoading ensures the skeleton only appears on the very first app load and never again after that.
There was a problem hiding this comment.
Why are we doing this subscription outside of the view? Could we use a ref for this?
There was a problem hiding this comment.
A ref resets to its initial value when the component unmounts. If BaseSidebarScreen unmounts and remounts (e.g. navigating between tabs), a ref-based flag would go back to false and the skeleton could flash again even though the app already finished loading. A module-level variable survives independently of the component lifecycle, so it acts as a true one-time latch for the entire app session.
There was a problem hiding this comment.
That makes sense, I know that we typically try to avoid using connectWithoutView so could you add a similar comment to what you said above this so that people know why it was added?
There was a problem hiding this comment.
Done in 80bb9d1 — expanded the comment to explain why we use a module-level variable + connectWithoutView instead of a ref.
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.
|
| optionMode={viewMode} | ||
| onFirstItemRendered={setSidebarLoaded} | ||
| /> | ||
| {!!isLoading && optionListItems?.length === 0 && ( |
There was a problem hiding this comment.
I think optionListItems length is used here to avoid showing BlockingView(shouldShowEmptyLHN)
Screen.Recording.2026-03-05.at.21.35.27.mov
There was a problem hiding this comment.
Good catch. Restored the skeleton overlay in SidebarLinks using IS_LOADING_REPORT_DATA instead of the old IS_LOADING_APP (which is now handled by BaseSidebarScreen). This prevents BlockingView from flashing when SidebarLinks mounts but reports have not populated yet.
JmillsExpensify
left a comment
JmillsExpensify
left a comment
There was a problem hiding this comment.
No product review required.
|
🚧 @JS00001 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/JS00001 in version: 9.3.33-0 🚀
|
6 participants
Explanation of Change
During initial app load,
SidebarLinks(and its entire subtree:LHNOptionsList,FlashList, allOptionRowLHNitems and their hooks) was mounting immediately and rendering an empty list, with a skeleton overlay placed on top via anabsoluteFillObjectview. This meant the heavy component tree was paying full mount cost just to be hidden behind a skeleton.This change moves the skeleton decision up to
BaseSidebarScreen. A module-levelOnyx.connectWithoutViewsubscription sets ahasEverFinishedLoadingflag the first timeIS_LOADING_APPtransitions tofalse. WhileisLoadingAppistrueand the app has never finished loading,BaseSidebarScreenrendersOptionsListSkeletonViewdirectly in place ofSidebarLinksData. The heavy component tree does not mount at all until data is ready. The module-level flag ensures the skeleton never reappears during subsequent reconnects (whereIS_LOADING_APPcould briefly flip back totrue).isLoadingprop removed fromSidebarLinksandSidebarLinksDataas it is no longer needed.Fixed Issues
$ #77175
PROPOSAL:
Tests
Offline tests
N/A
QA Steps
N/A
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