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.

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

perf: Reduce heavy operations in Request Money flow #39063

Merged
merged 4 commits into from
Mar 28, 2024
+27 −13
Merged

perf: Reduce heavy operations in Request Money flow #39063

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
23def11
Merge remote-tracking branch 'upstream/main' into perf/request-money-…
TMisiukiewicz Mar 26, 2024
21ccc39
Merge branch 'main' into perf/request-money-workspace
TMisiukiewicz Mar 27, 2024
7adff95
decouple search in server from update search value and memoize component
TMisiukiewicz Mar 27, 2024
74ff9d1
add memo check for betas
TMisiukiewicz Mar 28, 2024
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
40 changes: 27 additions & 13 deletions src/pages/iou/request/MoneyTemporaryForRefactorRequestParticipantsSelector.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import lodashGet from 'lodash/get';
import PropTypes from 'prop-types';
import React, {useCallback, useMemo, useState} from 'react';
import React, {memo, useCallback, useEffect, useMemo} from 'react';
import {View} from 'react-native';
import {withOnyx} from 'react-native-onyx';
import _ from 'underscore';
Expand All @@ -12,15 +12,16 @@ import ReferralProgramCTA from '@components/ReferralProgramCTA';
import SelectCircle from '@components/SelectCircle';
import SelectionList from '@components/SelectionList';
import UserListItem from '@components/SelectionList/UserListItem';
import useDebouncedState from '@hooks/useDebouncedState';
import useLocalize from '@hooks/useLocalize';
import useNetwork from '@hooks/useNetwork';
import usePermissions from '@hooks/usePermissions';
import useSearchTermAndSearch from '@hooks/useSearchTermAndSearch';
import useThemeStyles from '@hooks/useThemeStyles';
import * as DeviceCapabilities from '@libs/DeviceCapabilities';
import * as OptionsListUtils from '@libs/OptionsListUtils';
import * as ReportUtils from '@libs/ReportUtils';
import reportPropTypes from '@pages/reportPropTypes';
import * as Report from '@userActions/Report';
import CONST from '@src/CONST';
import ONYXKEYS from '@src/ONYXKEYS';

Expand Down Expand Up @@ -87,7 +88,7 @@ function MoneyTemporaryForRefactorRequestParticipantsSelector({
}) {
const {translate} = useLocalize();
const styles = useThemeStyles();
const [searchTerm, setSearchTerm] = useState('');
const [searchTerm, debouncedSearchTerm, setSearchTerm] = useDebouncedState('');
const referralContentType = iouType === CONST.IOU.TYPE.SEND ? CONST.REFERRAL_PROGRAM.CONTENT_TYPES.SEND_MONEY : CONST.REFERRAL_PROGRAM.CONTENT_TYPES.MONEY_REQUEST;
const {isOffline} = useNetwork();
const personalDetails = usePersonalDetails();
Expand All @@ -96,7 +97,10 @@ function MoneyTemporaryForRefactorRequestParticipantsSelector({
const offlineMessage = isOffline ? [`${translate('common.youAppearToBeOffline')} ${translate('search.resultsAreLimited')}`, {isTranslated: true}] : '';

const maxParticipantsReached = participants.length === CONST.REPORT.MAXIMUM_PARTICIPANTS;
const setSearchTermAndSearchInServer = useSearchTermAndSearch(setSearchTerm, maxParticipantsReached);

useEffect(() => {
Report.searchInServer(debouncedSearchTerm.trim());
}, [debouncedSearchTerm]);

/**
* Returns the sections needed for the OptionsSelector
Expand All @@ -109,12 +113,11 @@ function MoneyTemporaryForRefactorRequestParticipantsSelector({
return [newSections, {}];
}
let indexOffset = 0;

const chatOptions = OptionsListUtils.getFilteredOptions(
reports,
personalDetails,
betas,
searchTerm,
debouncedSearchTerm,
participants,
CONST.EXPENSIFY_EMAILS,

Expand All @@ -134,7 +137,7 @@ function MoneyTemporaryForRefactorRequestParticipantsSelector({
);

const formatResults = OptionsListUtils.formatSectionsFromSearchTerm(
searchTerm,
debouncedSearchTerm,
participants,
chatOptions.recentReports,
chatOptions.personalDetails,
Expand Down Expand Up @@ -180,7 +183,7 @@ function MoneyTemporaryForRefactorRequestParticipantsSelector({
}

return [newSections, chatOptions];
}, [didScreenTransitionEnd, reports, personalDetails, betas, searchTerm, participants, iouType, iouRequestType, maxParticipantsReached, canUseP2PDistanceRequests, translate]);
}, [didScreenTransitionEnd, reports, personalDetails, betas, debouncedSearchTerm, participants, iouType, canUseP2PDistanceRequests, iouRequestType, maxParticipantsReached, translate]);

/**
* Adds a single participant to the request
Expand Down Expand Up @@ -246,11 +249,11 @@ function MoneyTemporaryForRefactorRequestParticipantsSelector({
OptionsListUtils.getHeaderMessage(
_.get(newChatOptions, 'personalDetails', []).length + _.get(newChatOptions, 'recentReports', []).length !== 0,
Boolean(newChatOptions.userToInvite),
searchTerm.trim(),
debouncedSearchTerm.trim(),
maxParticipantsReached,
_.some(participants, (participant) => participant.searchText.toLowerCase().includes(searchTerm.trim().toLowerCase())),
_.some(participants, (participant) => participant.searchText.toLowerCase().includes(debouncedSearchTerm.trim().toLowerCase())),
),
[maxParticipantsReached, newChatOptions, participants, searchTerm],
[maxParticipantsReached, newChatOptions, participants, debouncedSearchTerm],
);

// Right now you can't split a request with a workspace and other additional participants
Expand Down Expand Up @@ -354,7 +357,7 @@ function MoneyTemporaryForRefactorRequestParticipantsSelector({
textInputValue={searchTerm}
textInputLabel={translate('optionsSelector.nameEmailOrPhoneNumber')}
textInputHint={offlineMessage}
onChangeText={setSearchTermAndSearchInServer}
onChangeText={setSearchTerm}
shouldPreventDefaultFocusOnSelectRow={!DeviceCapabilities.canUseTouchScreen()}
onSelectRow={addSingleParticipant}
footerContent={footerContent}
Expand All @@ -380,4 +383,15 @@ export default withOnyx({
betas: {
key: ONYXKEYS.BETAS,
},
})(MoneyTemporaryForRefactorRequestParticipantsSelector);
})(
memo(
MoneyTemporaryForRefactorRequestParticipantsSelector,
(prevProps, nextProps) =>
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we sure we dont have to add more checks for this memo?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated the memo with additional check for betas. The only non-checked props now are

  1. callback functions
  2. Onyx prop: reports

I think check for reports would need a deep equality comparision, which might be a costy operation

_.isEqual(prevProps.participants, nextProps.participants) &&
prevProps.didScreenTransitionEnd === nextProps.didScreenTransitionEnd &&
Copy link
Contributor

@akinwale akinwale Mar 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@TMisiukiewicz Is there a particular reason we're checking didScreenTransitionEnd here, especially as the event never fires on Android?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

in this page, didScreenTransitionEnd is used to prevent generating options unless the transition ended, to avoid doing heavy calculations while animation is happening. Without checking it, we'd infinitely get stuck on the skeleton

_.isEqual(prevProps.dismissedReferralBanners, nextProps.dismissedReferralBanners) &&
prevProps.iouRequestType === nextProps.iouRequestType &&
prevProps.iouType === nextProps.iouType &&
_.isEqual(prevProps.betas, nextProps.betas),
),
);
Loading