From 14c14f61ac41fb472f516dd18abd795646196218 Mon Sep 17 00:00:00 2001 From: hurali97 Date: Thu, 9 May 2024 17:12:42 +0500 Subject: [PATCH 1/5] perf: avoid rendering null as FlashList render item which causes the FlashList to render all its children and consume signficant memory --- src/components/LHNOptionsList/OptionRowLHN.tsx | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/src/components/LHNOptionsList/OptionRowLHN.tsx b/src/components/LHNOptionsList/OptionRowLHN.tsx index b9bc46f56ae5..8435fa77b804 100644 --- a/src/components/LHNOptionsList/OptionRowLHN.tsx +++ b/src/components/LHNOptionsList/OptionRowLHN.tsx @@ -51,13 +51,19 @@ function OptionRowLHN({reportID, isFocused = false, onSelectRow = () => {}, opti }, []), ); + const isInFocusMode = viewMode === CONST.OPTION_MODE.COMPACT; + const sidebarInnerRowStyle = StyleSheet.flatten( + isInFocusMode + ? [styles.chatLinkRowPressable, styles.flexGrow1, styles.optionItemAvatarNameWrapper, styles.optionRowCompact, styles.justifyContentCenter] + : [styles.chatLinkRowPressable, styles.flexGrow1, styles.optionItemAvatarNameWrapper, styles.optionRow, styles.justifyContentCenter], + ); + if (!optionItem) { - return null; + return ; } const hasBrickError = optionItem.brickRoadIndicator === CONST.BRICK_ROAD_INDICATOR_STATUS.ERROR; const shouldShowGreenDotIndicator = !hasBrickError && ReportUtils.requiresAttentionFromCurrentUser(optionItem, optionItem.parentReportAction); - const isInFocusMode = viewMode === CONST.OPTION_MODE.COMPACT; const textStyle = isFocused ? styles.sidebarLinkActiveText : styles.sidebarLinkText; const textUnreadStyle = optionItem?.isUnread && optionItem.notificationPreference !== CONST.REPORT.NOTIFICATION_PREFERENCE.MUTE ? [textStyle, styles.sidebarLinkTextBold] : [textStyle]; const displayNameStyle = [styles.optionDisplayName, styles.optionDisplayNameCompact, styles.pre, textUnreadStyle, style]; @@ -66,11 +72,6 @@ function OptionRowLHN({reportID, isFocused = false, onSelectRow = () => {}, opti : [textStyle, styles.optionAlternateText, styles.textLabelSupporting, style]; const contentContainerStyles = isInFocusMode ? [styles.flex1, styles.flexRow, styles.overflowHidden, StyleUtils.getCompactContentContainerStyles()] : [styles.flex1]; - const sidebarInnerRowStyle = StyleSheet.flatten( - isInFocusMode - ? [styles.chatLinkRowPressable, styles.flexGrow1, styles.optionItemAvatarNameWrapper, styles.optionRowCompact, styles.justifyContentCenter] - : [styles.chatLinkRowPressable, styles.flexGrow1, styles.optionItemAvatarNameWrapper, styles.optionRow, styles.justifyContentCenter], - ); const hoveredBackgroundColor = !!styles.sidebarLinkHover && 'backgroundColor' in styles.sidebarLinkHover ? styles.sidebarLinkHover.backgroundColor : theme.sidebar; const focusedBackgroundColor = styles.sidebarLinkActive.backgroundColor; From 81f1f5a607c492156870a17fc7d5d78d4ae424c2 Mon Sep 17 00:00:00 2001 From: hurali97 Date: Thu, 9 May 2024 17:14:10 +0500 Subject: [PATCH 2/5] perf: allow freezing the first screen on web and desktop. Without this FlashList renders all its children when we switch from LHN to settings --- src/libs/Navigation/FreezeWrapper.tsx | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/src/libs/Navigation/FreezeWrapper.tsx b/src/libs/Navigation/FreezeWrapper.tsx index bb6ab107373b..d0f1d357cf25 100644 --- a/src/libs/Navigation/FreezeWrapper.tsx +++ b/src/libs/Navigation/FreezeWrapper.tsx @@ -1,6 +1,7 @@ import {useIsFocused, useNavigation, useRoute} from '@react-navigation/native'; import React, {useEffect, useRef, useState} from 'react'; import {Freeze} from 'react-freeze'; +import {Platform} from 'react-native'; import type ChildrenProps from '@src/types/utils/ChildrenProps'; type FreezeWrapperProps = ChildrenProps & { @@ -27,11 +28,9 @@ function FreezeWrapper({keepVisible = false, children}: FreezeWrapperProps) { // if the screen is more than 1 screen away from the current screen, freeze it, // we don't want to freeze the screen if it's the previous screen because the freeze placeholder // would be visible at the beginning of the back animation then - if ((navigation.getState()?.index ?? 0) - (screenIndexRef.current ?? 0) > 1) { - setIsScreenBlurred(true); - } else { - setIsScreenBlurred(false); - } + const navigationIndex = (navigation.getState()?.index ?? 0) - (screenIndexRef.current ?? 0); + const shouldSetScreenBlurred = Platform.OS === 'android' || Platform.OS === 'ios' ? navigationIndex > 1 : navigationIndex >= 1; + setIsScreenBlurred(shouldSetScreenBlurred); }); return () => unsubscribe(); }, [isFocused, isScreenBlurred, navigation]); From 140a02ef68c0383fe7576d3066a73ab3ca4f9e1d Mon Sep 17 00:00:00 2001 From: hurali97 Date: Fri, 10 May 2024 12:04:57 +0500 Subject: [PATCH 3/5] refactor: use dedicated platform files for platform dependent code --- src/libs/Navigation/FreezeWrapper.tsx | 5 ++--- .../Navigation/shouldSetScreenBlurred/index.native.tsx | 8 ++++++++ src/libs/Navigation/shouldSetScreenBlurred/index.tsx | 8 ++++++++ 3 files changed, 18 insertions(+), 3 deletions(-) create mode 100644 src/libs/Navigation/shouldSetScreenBlurred/index.native.tsx create mode 100644 src/libs/Navigation/shouldSetScreenBlurred/index.tsx diff --git a/src/libs/Navigation/FreezeWrapper.tsx b/src/libs/Navigation/FreezeWrapper.tsx index d0f1d357cf25..de54f2dddffc 100644 --- a/src/libs/Navigation/FreezeWrapper.tsx +++ b/src/libs/Navigation/FreezeWrapper.tsx @@ -1,8 +1,8 @@ import {useIsFocused, useNavigation, useRoute} from '@react-navigation/native'; import React, {useEffect, useRef, useState} from 'react'; import {Freeze} from 'react-freeze'; -import {Platform} from 'react-native'; import type ChildrenProps from '@src/types/utils/ChildrenProps'; +import shouldSetScreenBlurred from './shouldSetScreenBlurred'; type FreezeWrapperProps = ChildrenProps & { /** Prop to disable freeze */ @@ -29,8 +29,7 @@ function FreezeWrapper({keepVisible = false, children}: FreezeWrapperProps) { // we don't want to freeze the screen if it's the previous screen because the freeze placeholder // would be visible at the beginning of the back animation then const navigationIndex = (navigation.getState()?.index ?? 0) - (screenIndexRef.current ?? 0); - const shouldSetScreenBlurred = Platform.OS === 'android' || Platform.OS === 'ios' ? navigationIndex > 1 : navigationIndex >= 1; - setIsScreenBlurred(shouldSetScreenBlurred); + setIsScreenBlurred(shouldSetScreenBlurred(navigationIndex)); }); return () => unsubscribe(); }, [isFocused, isScreenBlurred, navigation]); diff --git a/src/libs/Navigation/shouldSetScreenBlurred/index.native.tsx b/src/libs/Navigation/shouldSetScreenBlurred/index.native.tsx new file mode 100644 index 000000000000..9af38f789ed0 --- /dev/null +++ b/src/libs/Navigation/shouldSetScreenBlurred/index.native.tsx @@ -0,0 +1,8 @@ +/** + * @param navigationIndex + * + * Decides whether to set screen to blurred state + */ +const shouldSetScreenBlurred = (navigationIndex: number) => navigationIndex > 1; + +export default shouldSetScreenBlurred; diff --git a/src/libs/Navigation/shouldSetScreenBlurred/index.tsx b/src/libs/Navigation/shouldSetScreenBlurred/index.tsx new file mode 100644 index 000000000000..d52c556807cc --- /dev/null +++ b/src/libs/Navigation/shouldSetScreenBlurred/index.tsx @@ -0,0 +1,8 @@ +/** + * @param navigationIndex + * + * Decides whether to set screen to blurred state + */ +const shouldSetScreenBlurred = (navigationIndex: number) => navigationIndex >= 1; + +export default shouldSetScreenBlurred; From 3695e886b46734cddced0c6939c39d8faaf3e8c3 Mon Sep 17 00:00:00 2001 From: hurali97 Date: Fri, 10 May 2024 12:08:22 +0500 Subject: [PATCH 4/5] feat: add comments for why we need to render a placeholder view --- src/components/LHNOptionsList/OptionRowLHN.tsx | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/components/LHNOptionsList/OptionRowLHN.tsx b/src/components/LHNOptionsList/OptionRowLHN.tsx index 8435fa77b804..b665c305f5cf 100644 --- a/src/components/LHNOptionsList/OptionRowLHN.tsx +++ b/src/components/LHNOptionsList/OptionRowLHN.tsx @@ -59,6 +59,9 @@ function OptionRowLHN({reportID, isFocused = false, onSelectRow = () => {}, opti ); if (!optionItem) { + // rendering null as a render item causes the FlashList to render all + // its children and consume signficant memory. We can avoid this by + // rendering a placeholder view instead. return ; } From 456fe957b7c9ca9dcaf3aaa2d2c1bdf740434fe4 Mon Sep 17 00:00:00 2001 From: hurali97 Date: Fri, 10 May 2024 14:06:48 +0500 Subject: [PATCH 5/5] feat: add descriptive comments for shouldSetScreenBlurred function --- src/libs/Navigation/FreezeWrapper.tsx | 3 --- .../Navigation/shouldSetScreenBlurred/index.native.tsx | 6 +++++- src/libs/Navigation/shouldSetScreenBlurred/index.tsx | 7 ++++++- 3 files changed, 11 insertions(+), 5 deletions(-) diff --git a/src/libs/Navigation/FreezeWrapper.tsx b/src/libs/Navigation/FreezeWrapper.tsx index de54f2dddffc..23513dd87431 100644 --- a/src/libs/Navigation/FreezeWrapper.tsx +++ b/src/libs/Navigation/FreezeWrapper.tsx @@ -25,9 +25,6 @@ function FreezeWrapper({keepVisible = false, children}: FreezeWrapperProps) { useEffect(() => { const unsubscribe = navigation.addListener('state', () => { - // if the screen is more than 1 screen away from the current screen, freeze it, - // we don't want to freeze the screen if it's the previous screen because the freeze placeholder - // would be visible at the beginning of the back animation then const navigationIndex = (navigation.getState()?.index ?? 0) - (screenIndexRef.current ?? 0); setIsScreenBlurred(shouldSetScreenBlurred(navigationIndex)); }); diff --git a/src/libs/Navigation/shouldSetScreenBlurred/index.native.tsx b/src/libs/Navigation/shouldSetScreenBlurred/index.native.tsx index 9af38f789ed0..4043fddb7372 100644 --- a/src/libs/Navigation/shouldSetScreenBlurred/index.native.tsx +++ b/src/libs/Navigation/shouldSetScreenBlurred/index.native.tsx @@ -1,7 +1,11 @@ /** * @param navigationIndex * - * Decides whether to set screen to blurred state + * Decides whether to set screen to blurred state. + * + * If the screen is more than 1 screen away from the current screen, freeze it, + * we don't want to freeze the screen if it's the previous screen because the freeze placeholder + * would be visible at the beginning of the back animation then */ const shouldSetScreenBlurred = (navigationIndex: number) => navigationIndex > 1; diff --git a/src/libs/Navigation/shouldSetScreenBlurred/index.tsx b/src/libs/Navigation/shouldSetScreenBlurred/index.tsx index d52c556807cc..14b45921bdb2 100644 --- a/src/libs/Navigation/shouldSetScreenBlurred/index.tsx +++ b/src/libs/Navigation/shouldSetScreenBlurred/index.tsx @@ -1,7 +1,12 @@ /** * @param navigationIndex * - * Decides whether to set screen to blurred state + * Decides whether to set screen to blurred state. + * + * Allow freezing the first screen and more in the stack only on + * web and desktop platforms. The reason is that in the case of + * LHN, we have FlashList rendering in the back while we are on + * Settings screen. */ const shouldSetScreenBlurred = (navigationIndex: number) => navigationIndex >= 1;