Skip to content

[No QA] Show skeleton in BaseSidebarScreen to avoid mounting heavy SidebarLinks during initial load #84153

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
JS00001 merged 7 commits into from
Mar 6, 2026
+38 −10
Merged

[No QA] Show skeleton in BaseSidebarScreen to avoid mounting heavy SidebarLinks during initial load #84153

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
8326a55
Show skeleton in BaseSidebarScreen to avoid mounting heavy SidebarLin…
BartekObudzinski Mar 4, 2026
4b1a69d
Set IS_LOADING_APP to false in UI tests so the LHN renders instead of…
BartekObudzinski Mar 5, 2026
7506083
Merge remote-tracking branch 'upstream/main' into perf/early-return-s…
BartekObudzinski Mar 5, 2026
32977a6
perf: call useConfirmReadyToOpenApp unconditionally to prevent deadlock
BartekObudzinski Mar 5, 2026
80bb9d1
perf: expand comment explaining why connectWithoutView is used over a…
BartekObudzinski Mar 5, 2026
1e2f871
test: set IS_LOADING_APP to false in SessionTest openApp mock
BartekObudzinski Mar 5, 2026
c4f0cdf
perf: restore skeleton fallback in SidebarLinks to prevent BlockingVi…
BartekObudzinski Mar 6, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 29 additions & 3 deletions src/pages/inbox/sidebar/BaseSidebarScreen.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,21 +1,49 @@
import React from 'react';
import {View} from 'react-native';
import Onyx from 'react-native-onyx';
import NavigationTabBar from '@components/Navigation/NavigationTabBar';
import NAVIGATION_TABS from '@components/Navigation/NavigationTabBar/NAVIGATION_TABS';
import TopBar from '@components/Navigation/TopBar';
import OptionsListSkeletonView from '@components/OptionsListSkeletonView';
import ScreenWrapper from '@components/ScreenWrapper';
import useConfirmReadyToOpenApp from '@hooks/useConfirmReadyToOpenApp';
import useLocalize from '@hooks/useLocalize';
import useOnyx from '@hooks/useOnyx';
import useResponsiveLayout from '@hooks/useResponsiveLayout';
import useThemeStyles from '@hooks/useThemeStyles';
import {isMobile} from '@libs/Browser';
import ONYXKEYS from '@src/ONYXKEYS';
import SidebarLinksData from './SidebarLinksData';

// 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).
// This uses a module-level variable + connectWithoutView instead of a ref because
// a ref resets on unmount, so the skeleton would flash again when the component
// remounts (e.g. navigating between tabs).
let hasEverFinishedLoading = false;
Copy link
Contributor

@JS00001 JS00001 Mar 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why are we doing this?

Copy link
Contributor Author

@BartekObudzinski BartekObudzinski Mar 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@JS00001 JS00001 Mar 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are we doing this subscription outside of the view? Could we use a ref for this?

Copy link
Contributor Author

@BartekObudzinski BartekObudzinski Mar 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@JS00001 JS00001 Mar 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

@BartekObudzinski BartekObudzinski Mar 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 80bb9d1 — expanded the comment to explain why we use a module-level variable + connectWithoutView instead of a ref.

Onyx.connectWithoutView({
key: ONYXKEYS.IS_LOADING_APP,
callback: (value) => {
if (value !== false) {
return;
}
hasEverFinishedLoading = true;
},
});

function BaseSidebarScreen() {
const styles = useThemeStyles();
const {translate} = useLocalize();
const {shouldUseNarrowLayout} = useResponsiveLayout();
const shouldDisplayLHB = !shouldUseNarrowLayout;

const [isLoadingApp = true] = useOnyx(ONYXKEYS.IS_LOADING_APP);
const shouldShowSkeleton = isLoadingApp && !hasEverFinishedLoading;

// Must be called unconditionally so openApp() can proceed even when
// the skeleton is shown and SidebarLinksData has not mounted yet.
useConfirmReadyToOpenApp();

return (
<ScreenWrapper
shouldEnableKeyboardAvoidingView={false}
Expand All @@ -30,9 +58,7 @@ function BaseSidebarScreen() {
shouldDisplaySearch={shouldUseNarrowLayout}
shouldDisplayHelpButton={shouldUseNarrowLayout}
/>
<View style={[styles.flex1]}>
<SidebarLinksData insets={insets} />
</View>
<View style={[styles.flex1]}>{shouldShowSkeleton ? <OptionsListSkeletonView shouldAnimate /> : <SidebarLinksData insets={insets} />}</View>
Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot Mar 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Mount sidebar data while app readiness is unresolved

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 👍 / 👎.

Copy link
Contributor Author

@BartekObudzinski BartekObudzinski Mar 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

{shouldDisplayLHB && <NavigationTabBar selectedTab={NAVIGATION_TABS.INBOX} />}
</>
)}
Expand Down
10 changes: 5 additions & 5 deletions src/pages/inbox/sidebar/SidebarLinks.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import type {ValueOf} from 'type-fest';
import LHNOptionsList from '@components/LHNOptionsList/LHNOptionsList';
import OptionsListSkeletonView from '@components/OptionsListSkeletonView';
import useConfirmReadyToOpenApp from '@hooks/useConfirmReadyToOpenApp';
import useOnyx from '@hooks/useOnyx';
import useResponsiveLayout from '@hooks/useResponsiveLayout';
import useStyleUtils from '@hooks/useStyleUtils';
import useThemeStyles from '@hooks/useThemeStyles';
Expand All @@ -14,6 +15,7 @@ import Navigation from '@libs/Navigation/Navigation';
import {cancelSpan} from '@libs/telemetry/activeSpans';
import * as ReportActionContextMenu from '@pages/inbox/report/ContextMenu/ReportActionContextMenu';
import CONST from '@src/CONST';
import ONYXKEYS from '@src/ONYXKEYS';
import ROUTES from '@src/ROUTES';
import type {Report} from '@src/types/onyx';

Expand All @@ -24,20 +26,18 @@ type SidebarLinksProps = {
/** List of options to display */
optionListItems: Report[];

/** Whether the reports are loading. When false it means they are ready to be used. */
isLoading: OnyxEntry<boolean>;

/** The chat priority mode */
priorityMode?: OnyxEntry<ValueOf<typeof CONST.PRIORITY_MODE>>;

/** Method to change currently active report */
isActiveReport: (reportID: string) => boolean;
};

function SidebarLinks({insets, optionListItems, isLoading, priorityMode = CONST.PRIORITY_MODE.DEFAULT, isActiveReport}: SidebarLinksProps) {
function SidebarLinks({insets, optionListItems, priorityMode = CONST.PRIORITY_MODE.DEFAULT, isActiveReport}: SidebarLinksProps) {
const styles = useThemeStyles();
const StyleUtils = useStyleUtils();
const {shouldUseNarrowLayout} = useResponsiveLayout();
const [isLoadingReportData = true] = useOnyx(ONYXKEYS.IS_LOADING_REPORT_DATA);

useConfirmReadyToOpenApp();

Expand Down Expand Up @@ -90,7 +90,7 @@ function SidebarLinks({insets, optionListItems, isLoading, priorityMode = CONST.
optionMode={viewMode}
onFirstItemRendered={setSidebarLoaded}
/>
{!!isLoading && optionListItems?.length === 0 && (
Copy link
Contributor

@Pujan92 Pujan92 Mar 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think optionListItems length is used here to avoid showing BlockingView(shouldShowEmptyLHN)

Screen.Recording.2026-03-05.at.21.35.27.mov

Copy link
Contributor Author

@BartekObudzinski BartekObudzinski Mar 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

{!!isLoadingReportData && optionListItems?.length === 0 && (
<View style={[StyleSheet.absoluteFillObject, styles.appBG, styles.mt3]}>
<OptionsListSkeletonView shouldAnimate />
</View>
Expand Down
2 changes: 0 additions & 2 deletions src/pages/inbox/sidebar/SidebarLinksData.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@ function SidebarLinksData({insets}: SidebarLinksDataProps) {
const isFocused = useIsFocused();
const styles = useThemeStyles();
const {translate} = useLocalize();
const [isLoadingApp = true] = useOnyx(ONYXKEYS.IS_LOADING_APP);
const [priorityMode = CONST.PRIORITY_MODE.DEFAULT] = useOnyx(ONYXKEYS.NVP_PRIORITY_MODE);

const {orderedReports, currentReportID} = useSidebarOrderedReportsState('SidebarLinksData');
Expand Down Expand Up @@ -62,7 +61,6 @@ function SidebarLinksData({insets}: SidebarLinksDataProps) {
priorityMode={priorityMode ?? CONST.PRIORITY_MODE.DEFAULT}
// Data props:
isActiveReport={isActiveReport}
isLoading={isLoadingApp ?? false}
optionListItems={orderedReports}
/>
</View>
Expand Down
1 change: 1 addition & 0 deletions tests/ui/GroupChatNameTests.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -197,6 +197,7 @@ function signInAndGetApp(reportName = '', participantAccountIDs?: number[]): Pro
// Simulate setting an unread report and personal details
await act(async () => {
await Promise.all([
Onyx.merge(ONYXKEYS.IS_LOADING_APP, false),
Onyx.merge(`${ONYXKEYS.COLLECTION.REPORT}${REPORT_ID}`, {
reportID: REPORT_ID,
reportName,
Expand Down
1 change: 1 addition & 0 deletions tests/ui/PaginationTest.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -232,6 +232,7 @@ async function signInAndGetApp(): Promise<void> {

await act(async () => {
await Promise.all([
Onyx.merge(ONYXKEYS.IS_LOADING_APP, false),
// Simulate setting an unread report and personal details
Onyx.merge(`${ONYXKEYS.COLLECTION.REPORT}${REPORT_ID}`, {
reportID: REPORT_ID,
Expand Down
1 change: 1 addition & 0 deletions tests/ui/SessionTest.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,7 @@ describe('Deep linking', () => {
jest.spyOn(AppActions, 'openApp').mockImplementation(async () => {
const originalResult = await originalOpenApp();
await Onyx.set(`${ONYXKEYS.COLLECTION.REPORT}${report.reportID}`, report);
await Onyx.merge(ONYXKEYS.IS_LOADING_APP, false);
return originalResult;
});
});
Expand Down
1 change: 1 addition & 0 deletions tests/ui/UnreadIndicatorsTest.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -215,6 +215,7 @@ async function signInAndGetAppWithUnreadChat(): Promise<void> {
};

await Promise.all([
Onyx.merge(ONYXKEYS.IS_LOADING_APP, false),
Onyx.merge(ONYXKEYS.PERSONAL_DETAILS_LIST, personalDetails),
Onyx.merge(`${ONYXKEYS.COLLECTION.REPORT}${REPORT_ID}`, report),
Onyx.merge(`${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${REPORT_ID}`, reportActions),
Expand Down
Loading