Skip to content
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.

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

perf: fix significant memory consumption on web and desktop #41898

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
18 changes: 11 additions & 7 deletions src/components/LHNOptionsList/OptionRowLHN.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -51,13 +51,22 @@ function OptionRowLHN({reportID, isFocused = false, onSelectRow = () => {}, opti
}, []),
);

const isInFocusMode = viewMode === CONST.OPTION_MODE.COMPACT;
const sidebarInnerRowStyle = StyleSheet.flatten<ViewStyle>(
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;
// 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 <View style={sidebarInnerRowStyle} />;
hurali97 marked this conversation as resolved.
Show resolved Hide resolved
}

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];
Expand All @@ -66,11 +75,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<ViewStyle>(
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;

Expand Down
11 changes: 3 additions & 8 deletions src/libs/Navigation/FreezeWrapper.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import {useIsFocused, useNavigation, useRoute} from '@react-navigation/native';
import React, {useEffect, useRef, useState} from 'react';
import {Freeze} from 'react-freeze';
import type ChildrenProps from '@src/types/utils/ChildrenProps';
import shouldSetScreenBlurred from './shouldSetScreenBlurred';

type FreezeWrapperProps = ChildrenProps & {
/** Prop to disable freeze */
Expand All @@ -24,14 +25,8 @@ 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
if ((navigation.getState()?.index ?? 0) - (screenIndexRef.current ?? 0) > 1) {
setIsScreenBlurred(true);
} else {
setIsScreenBlurred(false);
}
const navigationIndex = (navigation.getState()?.index ?? 0) - (screenIndexRef.current ?? 0);
setIsScreenBlurred(shouldSetScreenBlurred(navigationIndex));
});
return () => unsubscribe();
}, [isFocused, isScreenBlurred, navigation]);
Expand Down
12 changes: 12 additions & 0 deletions src/libs/Navigation/shouldSetScreenBlurred/index.native.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
/**
* @param navigationIndex
*
* 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;

export default shouldSetScreenBlurred;
13 changes: 13 additions & 0 deletions src/libs/Navigation/shouldSetScreenBlurred/index.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
/**
* @param navigationIndex
*
* 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.
Comment on lines +6 to +9
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this assuming then that web/desktop is wide layout and native is narrow layout?

Its not a best practice and we should avoid that if that is the case as we are working on allowing wide layout on native ipad too and then this would not work that well. Not a blocker though

*/
const shouldSetScreenBlurred = (navigationIndex: number) => navigationIndex >= 1;

export default shouldSetScreenBlurred;
Loading