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: improve chat switch performance - fixed #38255

Merged
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
11 changes: 6 additions & 5 deletions src/components/FlatList/index.android.tsx
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import {useFocusEffect} from '@react-navigation/native';
import type {ForwardedRef} from 'react';
import React, {forwardRef, useCallback, useContext} from 'react';
import type {FlatListProps} from 'react-native';
import type {FlatListProps, NativeScrollEvent, NativeSyntheticEvent} from 'react-native';
import {FlatList} from 'react-native';
import {ActionListContext} from '@pages/home/ReportScreenContext';

Expand All @@ -22,6 +22,9 @@ function CustomFlatList<T>(props: FlatListProps<T>, ref: ForwardedRef<FlatList>)
}
}, [scrollPosition?.offset, ref]);

// eslint-disable-next-line react-hooks/exhaustive-deps
const onMomentumScrollEnd = useCallback((event: NativeSyntheticEvent<NativeScrollEvent>) => setScrollPosition({offset: event.nativeEvent.contentOffset.y}), []);

useFocusEffect(
useCallback(() => {
onScreenFocus();
Expand All @@ -32,10 +35,8 @@ function CustomFlatList<T>(props: FlatListProps<T>, ref: ForwardedRef<FlatList>)
<FlatList<T>
// eslint-disable-next-line react/jsx-props-no-spreading
{...props}
onScroll={(event) => props.onScroll?.(event)}
onMomentumScrollEnd={(event) => {
setScrollPosition({offset: event.nativeEvent.contentOffset.y});
}}
onScroll={props.onScroll}
onMomentumScrollEnd={onMomentumScrollEnd}
ref={ref}
/>
);
Expand Down
7 changes: 2 additions & 5 deletions src/components/HeaderWithBackButton/types.ts
Original file line number Diff line number Diff line change
@@ -1,10 +1,10 @@
import type {ReactNode} from 'react';
import type {StyleProp, ViewStyle} from 'react-native';
import type {OnyxCollection, OnyxEntry} from 'react-native-onyx';
import type {OnyxEntry} from 'react-native-onyx';
import type {Action} from '@hooks/useSingleExecution';
import type {StepCounterParams} from '@src/languages/types';
import type {AnchorPosition} from '@src/styles';
import type {PersonalDetails, Policy, Report} from '@src/types/onyx';
import type {Policy, Report} from '@src/types/onyx';
import type {Icon} from '@src/types/onyx/OnyxCommon';
import type ChildrenProps from '@src/types/utils/ChildrenProps';
import type IconAsset from '@src/types/utils/IconAsset';
Expand Down Expand Up @@ -103,9 +103,6 @@ type HeaderWithBackButtonProps = Partial<ChildrenProps> & {
/** The report's policy, if we're showing the details for a report and need info about it for AvatarWithDisplay */
policy?: OnyxEntry<Policy>;

/** Policies, if we're showing the details for a report and need participant details for AvatarWithDisplay */
personalDetails?: OnyxCollection<PersonalDetails>;

/** Single execution function to prevent concurrent navigation actions */
singleExecution?: <T extends unknown[]>(action: Action<T>) => Action<T>;

Expand Down
10 changes: 6 additions & 4 deletions src/components/InvertedFlatList/BaseInvertedFlatList.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -6,17 +6,19 @@ import FlatList from '@components/FlatList';
const WINDOW_SIZE = 15;
const AUTOSCROLL_TO_TOP_THRESHOLD = 128;

const maintainVisibleContentPosition = {
minIndexForVisible: 0,
autoscrollToTopThreshold: AUTOSCROLL_TO_TOP_THRESHOLD,
};

function BaseInvertedFlatList<T>(props: FlatListProps<T>, ref: ForwardedRef<FlatList>) {
return (
<FlatList
// eslint-disable-next-line react/jsx-props-no-spreading
{...props}
ref={ref}
windowSize={WINDOW_SIZE}
maintainVisibleContentPosition={{
minIndexForVisible: 0,
autoscrollToTopThreshold: AUTOSCROLL_TO_TOP_THRESHOLD,
}}
maintainVisibleContentPosition={maintainVisibleContentPosition}
inverted
/>
);
Expand Down
65 changes: 31 additions & 34 deletions src/components/LHNOptionsList/LHNOptionsList.tsx
Original file line number Diff line number Diff line change
@@ -1,9 +1,8 @@
import {FlashList} from '@shopify/flash-list';
Copy link
Contributor

Choose a reason for hiding this comment

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

@jbroma the changes in this file seems to have caused the issue below, only when a request is created offline

Screen.Recording.2024-03-20.at.20.22.43.mov

Copy link
Contributor

Choose a reason for hiding this comment

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

I think specifically the removal of withCurrentReportID HOC

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually it's the removal of currentReportID from extraData that's passed to FlashList. But I get it's necessary so the whole LHN doesn't re-render when we switch focus from a report, right? so how do we fix this, any ideas?

Copy link
Contributor

@allgandalf allgandalf Mar 20, 2024

Choose a reason for hiding this comment

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

what if we pass currentReportID in extraData ? 🤔 it won't rerender then right?

const extraData = useMemo(() => 
[reportActions, reports, policy, personalDetails, currentReportID], 
[reportActions, reports, policy, personalDetails]);

I guess we can satisfy typescript dependency error by entering exception there right?

Copy link
Contributor

@youssef-lr youssef-lr Mar 20, 2024

Choose a reason for hiding this comment

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

but if we leave out from the dependencies it will remain stale, it won't be updated when we switch to another report, no?

Copy link
Contributor

Choose a reason for hiding this comment

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

The useMemo gets triggered everytime we move from reports right?

Nope, it doesn't, only when those dependencies change and they don't when we're simply switching between reports.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'll console.log itemFullReport just to help you bebug more over here

const renderItem = useCallback(
({item: reportID}: RenderItemProps): ReactElement => {
const itemFullReport = reports?.[`${ONYXKEYS.COLLECTION.REPORT}${reportID}`] ?? null;

Copy link
Contributor

Choose a reason for hiding this comment

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

umm makes more sense now:
(Unedited main branch) Added console log

simplescreenrecorder-2024-03-21_03.43.37.mp4

Copy link
Contributor

Choose a reason for hiding this comment

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

I can't reproduce this anymore for some reason @GandalfGwaihir, can you?

Copy link
Contributor

Choose a reason for hiding this comment

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

nvm I just reproduced it again

import type {ReactElement} from 'react';
import React, {memo, useCallback} from 'react';
import React, {memo, useCallback, useMemo} from 'react';
import {StyleSheet, View} from 'react-native';
import {withOnyx} from 'react-native-onyx';
import withCurrentReportID from '@components/withCurrentReportID';
import usePermissions from '@hooks/usePermissions';
import useThemeStyles from '@hooks/useThemeStyles';
import * as ReportActionsUtils from '@libs/ReportActionsUtils';
Expand All @@ -28,7 +27,6 @@ function LHNOptionsList({
preferredLocale = CONST.LOCALES.DEFAULT,
personalDetails = {},
transactions = {},
currentReportID = '',
draftComments = {},
transactionViolations = {},
onFirstItemRendered = () => {},
Expand Down Expand Up @@ -84,7 +82,7 @@ function LHNOptionsList({
lastReportActionTransaction={lastReportActionTransaction}
receiptTransactions={transactions}
viewMode={optionMode}
isFocused={!shouldDisableFocusOptions && reportID === currentReportID}
isFocused={!shouldDisableFocusOptions}
onSelectRow={onSelectRow}
preferredLocale={preferredLocale}
comment={itemComment}
Expand All @@ -95,7 +93,6 @@ function LHNOptionsList({
);
},
[
currentReportID,
draftComments,
onSelectRow,
optionMode,
Expand All @@ -112,6 +109,8 @@ function LHNOptionsList({
],
);

const extraData = useMemo(() => [reportActions, reports, policy, personalDetails], [reportActions, reports, policy, personalDetails]);

return (
<View style={style ?? styles.flex1}>
<FlashList
Expand All @@ -123,7 +122,7 @@ function LHNOptionsList({
keyExtractor={keyExtractor}
renderItem={renderItem}
estimatedItemSize={optionMode === CONST.OPTION_MODE.COMPACT ? variables.optionRowHeightCompact : variables.optionRowHeight}
extraData={[currentReportID]}
extraData={extraData}
showsVerticalScrollIndicator={false}
/>
</View>
Expand All @@ -132,33 +131,31 @@ function LHNOptionsList({

LHNOptionsList.displayName = 'LHNOptionsList';

export default withCurrentReportID(
withOnyx<LHNOptionsListProps, LHNOptionsListOnyxProps>({
reports: {
key: ONYXKEYS.COLLECTION.REPORT,
},
reportActions: {
key: ONYXKEYS.COLLECTION.REPORT_ACTIONS,
},
policy: {
key: ONYXKEYS.COLLECTION.POLICY,
},
preferredLocale: {
key: ONYXKEYS.NVP_PREFERRED_LOCALE,
},
personalDetails: {
key: ONYXKEYS.PERSONAL_DETAILS_LIST,
},
transactions: {
key: ONYXKEYS.COLLECTION.TRANSACTION,
},
draftComments: {
key: ONYXKEYS.COLLECTION.REPORT_DRAFT_COMMENT,
},
transactionViolations: {
key: ONYXKEYS.COLLECTION.TRANSACTION_VIOLATIONS,
},
})(memo(LHNOptionsList)),
);
export default withOnyx<LHNOptionsListProps, LHNOptionsListOnyxProps>({
reports: {
key: ONYXKEYS.COLLECTION.REPORT,
},
reportActions: {
key: ONYXKEYS.COLLECTION.REPORT_ACTIONS,
},
policy: {
key: ONYXKEYS.COLLECTION.POLICY,
},
preferredLocale: {
key: ONYXKEYS.NVP_PREFERRED_LOCALE,
},
personalDetails: {
key: ONYXKEYS.PERSONAL_DETAILS_LIST,
},
transactions: {
key: ONYXKEYS.COLLECTION.TRANSACTION,
},
draftComments: {
key: ONYXKEYS.COLLECTION.REPORT_DRAFT_COMMENT,
},
transactionViolations: {
key: ONYXKEYS.COLLECTION.TRANSACTION_VIOLATIONS,
},
})(memo(LHNOptionsList));

export type {LHNOptionsListProps};
5 changes: 4 additions & 1 deletion src/components/LHNOptionsList/OptionRowLHNData.tsx
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import {deepEqual} from 'fast-equals';
import React, {useEffect, useMemo, useRef} from 'react';
import useCurrentReportID from '@hooks/useCurrentReportID';
import * as ReportUtils from '@libs/ReportUtils';
import SidebarUtils from '@libs/SidebarUtils';
import * as Report from '@userActions/Report';
Expand Down Expand Up @@ -31,6 +32,8 @@ function OptionRowLHNData({
...propsToForward
}: OptionRowLHNDataProps) {
const reportID = propsToForward.reportID;
const currentReportIDValue = useCurrentReportID();
const isReportFocused = isFocused && currentReportIDValue?.currentReportID === reportID;

const optionItemRef = useRef<OptionData>();

Expand Down Expand Up @@ -83,7 +86,7 @@ function OptionRowLHNData({
<OptionRowLHN
// eslint-disable-next-line react/jsx-props-no-spreading
{...propsToForward}
isFocused={isFocused}
isFocused={isReportFocused}
optionItem={optionItem}
/>
);
Expand Down
3 changes: 1 addition & 2 deletions src/components/LHNOptionsList/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ import type {RefObject} from 'react';
import type {LayoutChangeEvent, StyleProp, TextStyle, View, ViewStyle} from 'react-native';
import type {OnyxCollection, OnyxEntry} from 'react-native-onyx';
import type {ValueOf} from 'type-fest';
import type {CurrentReportIDContextValue} from '@components/withCurrentReportID';
import type CONST from '@src/CONST';
import type {OptionData} from '@src/libs/ReportUtils';
import type {Locale, PersonalDetailsList, Policy, Report, ReportAction, ReportActions, Transaction, TransactionViolation} from '@src/types/onyx';
Expand Down Expand Up @@ -60,7 +59,7 @@ type CustomLHNOptionsListProps = {
onFirstItemRendered: () => void;
};

type LHNOptionsListProps = CustomLHNOptionsListProps & CurrentReportIDContextValue & LHNOptionsListOnyxProps;
type LHNOptionsListProps = CustomLHNOptionsListProps & LHNOptionsListOnyxProps;

type OptionRowLHNDataProps = {
/** Whether row should be focused */
Expand Down
3 changes: 0 additions & 3 deletions src/components/MoneyReportHeader.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@ import ConfirmModal from './ConfirmModal';
import HeaderWithBackButton from './HeaderWithBackButton';
import * as Expensicons from './Icon/Expensicons';
import MoneyReportHeaderStatusBar from './MoneyReportHeaderStatusBar';
import {usePersonalDetails} from './OnyxProvider';
import SettlementButton from './SettlementButton';

type PaymentType = DeepValueOf<typeof CONST.IOU.PAYMENT_TYPE>;
Expand All @@ -45,7 +44,6 @@ type MoneyReportHeaderProps = MoneyReportHeaderOnyxProps & {
};

function MoneyReportHeader({session, policy, chatReport, nextStep, report: moneyRequestReport}: MoneyReportHeaderProps) {
const personalDetails = usePersonalDetails() || CONST.EMPTY_OBJECT;
const styles = useThemeStyles();
const {translate} = useLocalize();
const {windowWidth, isSmallScreenWidth} = useWindowDimensions();
Expand Down Expand Up @@ -102,7 +100,6 @@ function MoneyReportHeader({session, policy, chatReport, nextStep, report: money
shouldShowPinButton={false}
report={moneyRequestReport}
policy={policy}
personalDetails={personalDetails}
shouldShowBackButton={isSmallScreenWidth}
onBackButtonPress={() => Navigation.goBack(undefined, false, true)}
// Shows border if no buttons or next steps are showing below the header
Expand Down
3 changes: 0 additions & 3 deletions src/components/MoneyRequestHeader.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@ import HeaderWithBackButton from './HeaderWithBackButton';
import HoldBanner from './HoldBanner';
import * as Expensicons from './Icon/Expensicons';
import MoneyRequestHeaderStatusBar from './MoneyRequestHeaderStatusBar';
import {usePersonalDetails} from './OnyxProvider';
import ProcessMoneyRequestHoldMenu from './ProcessMoneyRequestHoldMenu';

type MoneyRequestHeaderOnyxProps = {
Expand Down Expand Up @@ -54,7 +53,6 @@ type MoneyRequestHeaderProps = MoneyRequestHeaderOnyxProps & {
};

function MoneyRequestHeader({session, parentReport, report, parentReportAction, transaction, shownHoldUseExplanation = false, policy}: MoneyRequestHeaderProps) {
const personalDetails = usePersonalDetails() || CONST.EMPTY_OBJECT;
const styles = useThemeStyles();
const {translate} = useLocalize();
const [isDeleteModalVisible, setIsDeleteModalVisible] = useState(false);
Expand Down Expand Up @@ -173,7 +171,6 @@ function MoneyRequestHeader({session, parentReport, report, parentReportAction,
ownerAccountID: parentReport?.ownerAccountID,
}}
policy={policy}
personalDetails={personalDetails}
shouldShowBackButton={isSmallScreenWidth}
onBackButtonPress={() => Navigation.goBack(undefined, false, true)}
/>
Expand Down
81 changes: 42 additions & 39 deletions src/libs/Pusher/pusher.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import isObject from 'lodash/isObject';
import type {Channel, ChannelAuthorizerGenerator, Options} from 'pusher-js/with-encryption';
import {InteractionManager} from 'react-native';
import Onyx from 'react-native-onyx';
import type {LiteralUnion, ValueOf} from 'type-fest';
import Log from '@libs/Log';
Expand Down Expand Up @@ -226,48 +227,50 @@ function subscribe<EventName extends PusherEventName>(
onResubscribe = () => {},
): Promise<void> {
return new Promise((resolve, reject) => {
// We cannot call subscribe() before init(). Prevent any attempt to do this on dev.
if (!socket) {
throw new Error(`[Pusher] instance not found. Pusher.subscribe()
InteractionManager.runAfterInteractions(() => {
// We cannot call subscribe() before init(). Prevent any attempt to do this on dev.
if (!socket) {
throw new Error(`[Pusher] instance not found. Pusher.subscribe()
most likely has been called before Pusher.init()`);
}
}

Log.info('[Pusher] Attempting to subscribe to channel', false, {channelName, eventName});
let channel = getChannel(channelName);

if (!channel || !channel.subscribed) {
channel = socket.subscribe(channelName);
let isBound = false;
channel.bind('pusher:subscription_succeeded', () => {
// Check so that we do not bind another event with each reconnect attempt
if (!isBound) {
bindEventToChannel(channel, eventName, eventCallback);
resolve();
isBound = true;
return;
}

// When subscribing for the first time we register a success callback that can be
// called multiple times when the subscription succeeds again in the future
// e.g. as a result of Pusher disconnecting and reconnecting. This callback does
// not fire on the first subscription_succeeded event.
onResubscribe();
});

channel.bind('pusher:subscription_error', (data: PusherSubscribtionErrorData = {}) => {
const {type, error, status} = data;
Log.hmmm('[Pusher] Issue authenticating with Pusher during subscribe attempt.', {
channelName,
status,
type,
error,
Log.info('[Pusher] Attempting to subscribe to channel', false, {channelName, eventName});
let channel = getChannel(channelName);

if (!channel || !channel.subscribed) {
channel = socket.subscribe(channelName);
let isBound = false;
channel.bind('pusher:subscription_succeeded', () => {
// Check so that we do not bind another event with each reconnect attempt
if (!isBound) {
bindEventToChannel(channel, eventName, eventCallback);
resolve();
isBound = true;
return;
}

// When subscribing for the first time we register a success callback that can be
// called multiple times when the subscription succeeds again in the future
// e.g. as a result of Pusher disconnecting and reconnecting. This callback does
// not fire on the first subscription_succeeded event.
onResubscribe();
});
reject(error);
});
} else {
bindEventToChannel(channel, eventName, eventCallback);
resolve();
}

channel.bind('pusher:subscription_error', (data: PusherSubscribtionErrorData = {}) => {
const {type, error, status} = data;
Log.hmmm('[Pusher] Issue authenticating with Pusher during subscribe attempt.', {
channelName,
status,
type,
error,
});
reject(error);
});
} else {
bindEventToChannel(channel, eventName, eventCallback);
resolve();
}
});
});
}

Expand Down
Loading
Loading