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) Debounce search in OptionList #32962

Merged
merged 8 commits into from
Dec 18, 2023
Merged
2 changes: 1 addition & 1 deletion src/CONST.ts
Original file line number Diff line number Diff line change
Expand Up @@ -704,7 +704,7 @@ const CONST = {
TOOLTIP_SENSE: 1000,
TRIE_INITIALIZATION: 'trie_initialization',
COMMENT_LENGTH_DEBOUNCE_TIME: 500,
SEARCH_FOR_REPORTS_DEBOUNCE_TIME: 300,
SEARCH_OPTION_LIST_DEBOUNCE_TIME: 300,
},
PRIORITY_MODE: {
GSD: 'gsd',
Expand Down
1 change: 0 additions & 1 deletion src/components/CategoryPicker/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,6 @@ function CategoryPicker({selectedCategory, policyCategories, policyRecentlyUsedC
sectionHeaderStyle={styles.mt5}
sections={sections}
selectedOptions={selectedOptions}
value={searchValue}
// Focus the first option when searching
focusedIndex={0}
// Focus the selected option on first load
Expand Down
1 change: 0 additions & 1 deletion src/components/MoneyRequestConfirmationList.js
Original file line number Diff line number Diff line change
Expand Up @@ -565,7 +565,6 @@ function MoneyRequestConfirmationList(props) {
return (
<OptionsSelector
sections={optionSelectorSections}
value=""
onSelectRow={canModifyParticipants ? selectParticipant : navigateToReportOrUserDetail}
onAddToSelection={selectParticipant}
onConfirmSelection={confirm}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -565,7 +565,6 @@ function MoneyTemporaryForRefactorRequestConfirmationList({
return (
<OptionsSelector
sections={optionSelectorSections}
value=""
onSelectRow={userCanModifyParticipants.current ? selectParticipant : navigateToReportOrUserDetail}
onAddToSelection={selectParticipant}
onConfirmSelection={confirm}
Expand Down
2 changes: 1 addition & 1 deletion src/components/OptionRow.js
Original file line number Diff line number Diff line change
Expand Up @@ -323,7 +323,7 @@ export default React.memo(
prevProps.showSelectedState === nextProps.showSelectedState &&
prevProps.highlightSelected === nextProps.highlightSelected &&
prevProps.showTitleTooltip === nextProps.showTitleTooltip &&
!_.isEqual(prevProps.option.icons, nextProps.option.icons) &&
_.isEqual(prevProps.option.icons, nextProps.option.icons) &&
Copy link
Contributor

Choose a reason for hiding this comment

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

What is this change about? This must have been causing some bug if it was incorrect 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was breaking the memoisation of the OptionRow - it was rerendering the whole OptionRow when typing in the SearchInput, but in fact the icons array and its values remained the same

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks!

prevProps.optionIsFocused === nextProps.optionIsFocused &&
prevProps.option.text === nextProps.option.text &&
prevProps.option.alternateText === nextProps.option.alternateText &&
Expand Down
12 changes: 7 additions & 5 deletions src/components/OptionsSelector/BaseOptionsSelector.js
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,7 @@ class BaseOptionsSelector extends Component {
this.incrementPage = this.incrementPage.bind(this);
this.sliceSections = this.sliceSections.bind(this);
this.calculateAllVisibleOptionsCount = this.calculateAllVisibleOptionsCount.bind(this);
this.debouncedUpdateSearchValue = _.debounce(this.updateSearchValue, CONST.TIMING.SEARCH_OPTION_LIST_DEBOUNCE_TIME);
this.relatedTarget = null;

const allOptions = this.flattenSections();
Expand All @@ -94,6 +95,7 @@ class BaseOptionsSelector extends Component {
shouldShowReferralModal: false,
errorMessage: '',
paginationPage: 1,
value: '',
};
}

Expand Down Expand Up @@ -161,7 +163,7 @@ class BaseOptionsSelector extends Component {
},
() => {
// If we just toggled an option on a multi-selection page or cleared the search input, scroll to top
if (this.props.selectedOptions.length !== prevProps.selectedOptions.length || (!!prevProps.value && !this.props.value)) {
if (this.props.selectedOptions.length !== prevProps.selectedOptions.length || (!!prevState.value && !this.state.value)) {
this.scrollToIndex(0);
return;
}
Expand Down Expand Up @@ -247,6 +249,7 @@ class BaseOptionsSelector extends Component {
this.setState({
paginationPage: 1,
errorMessage: value.length > this.props.maxLength ? this.props.translate('common.error.characterLimitExceedCounter', {length: value.length, limit: this.props.maxLength}) : '',
value,
});

this.props.onChangeText(value);
Expand Down Expand Up @@ -415,7 +418,7 @@ class BaseOptionsSelector extends Component {
this.relatedTarget = null;
}
if (this.textInput.isFocused()) {
setSelection(this.textInput, 0, this.props.value.length);
setSelection(this.textInput, 0, this.state.value.length);
}
}
const selectedOption = this.props.onSelectRow(option);
Expand All @@ -440,7 +443,7 @@ class BaseOptionsSelector extends Component {
if (this.props.shouldShowTextInput && this.props.shouldPreventDefaultFocusOnSelectRow) {
this.textInput.focus();
if (this.textInput.isFocused()) {
setSelection(this.textInput, 0, this.props.value.length);
setSelection(this.textInput, 0, this.state.value.length);
}
}
this.props.onAddToSelection(option);
Expand Down Expand Up @@ -468,11 +471,10 @@ class BaseOptionsSelector extends Component {
const textInput = (
<TextInput
ref={(el) => (this.textInput = el)}
value={this.props.value}
label={this.props.textInputLabel}
accessibilityLabel={this.props.textInputLabel}
role={CONST.ROLE.PRESENTATION}
onChangeText={this.updateSearchValue}
onChangeText={this.debouncedUpdateSearchValue}
errorText={this.state.errorMessage}
onSubmitEditing={this.selectFocusedOption}
placeholder={this.props.placeholderText}
Expand Down
3 changes: 0 additions & 3 deletions src/components/OptionsSelector/optionsSelectorPropTypes.js
Original file line number Diff line number Diff line change
Expand Up @@ -27,9 +27,6 @@ const propTypes = {
}),
).isRequired,

/** Value in the search input field */
value: PropTypes.string.isRequired,

TMisiukiewicz marked this conversation as resolved.
Show resolved Hide resolved
/** Callback fired when text changes */
onChangeText: PropTypes.func,

Expand Down
1 change: 0 additions & 1 deletion src/components/TagPicker/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,6 @@ function TagPicker({selectedTag, tag, policyTags, policyRecentlyUsedTags, should
highlightSelectedOptions
isRowMultilineSupported
shouldShowTextInput={shouldShowTextInput}
value={searchValue}
// Focus the first option when searching
focusedIndex={0}
// Focus the selected option on first load
Expand Down
5 changes: 1 addition & 4 deletions src/libs/actions/Report.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
import {format as timezoneFormat, utcToZonedTime} from 'date-fns-tz';
import ExpensiMark from 'expensify-common/lib/ExpensiMark';
import Str from 'expensify-common/lib/str';
import lodashDebounce from 'lodash/debounce';
import isEmpty from 'lodash/isEmpty';
import {DeviceEventEmitter, InteractionManager} from 'react-native';
import Onyx, {OnyxCollection, OnyxEntry, OnyxUpdate} from 'react-native-onyx';
Expand Down Expand Up @@ -2499,8 +2498,6 @@ function searchForReports(searchInput: string) {
API.read('SearchForReports', parameters, {successData, failureData});
}

const debouncedSearchInServer = lodashDebounce(searchForReports, CONST.TIMING.SEARCH_FOR_REPORTS_DEBOUNCE_TIME, {leading: false});

function searchInServer(searchInput: string) {
if (isNetworkOffline || !searchInput.trim().length) {
Onyx.set(ONYXKEYS.IS_SEARCHING_FOR_REPORTS, false);
Expand All @@ -2511,7 +2508,7 @@ function searchInServer(searchInput: string) {
// we want to show the loading state right away. Otherwise, we will see a flashing UI where the client options are sorted and
// tell the user there are no options, then we start searching, and tell them there are no options again.
Onyx.set(ONYXKEYS.IS_SEARCHING_FOR_REPORTS, true);
debouncedSearchInServer(searchInput);
searchForReports(searchInput);
}

function clearNewRoomFormError() {
Expand Down
1 change: 0 additions & 1 deletion src/pages/NewChatPage.js
Original file line number Diff line number Diff line change
Expand Up @@ -244,7 +244,6 @@ function NewChatPage({betas, isGroupChat, personalDetails, reports, translate, i
onAddToSelection={(option) => toggleOption(option)}
sections={sections}
selectedOptions={selectedOptions}
value={searchTerm}
onSelectRow={(option) => createChat(option)}
onChangeText={setSearchTermAndSearchInServer}
headerMessage={headerMessage}
Expand Down
13 changes: 2 additions & 11 deletions src/pages/SearchPage.js
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,6 @@ class SearchPage extends Component {
this.selectReport = this.selectReport.bind(this);
this.onChangeText = this.onChangeText.bind(this);
this.updateOptions = this.updateOptions.bind(this);
this.debouncedUpdateOptions = _.debounce(this.updateOptions.bind(this), 75);
this.state = {
searchValue: '',
recentReports: {},
Expand All @@ -85,7 +84,7 @@ class SearchPage extends Component {

onChangeText(searchValue = '') {
Report.searchInServer(searchValue);
this.setState({searchValue}, this.debouncedUpdateOptions);
this.setState({searchValue}, this.updateOptions);
}

/**
Expand Down Expand Up @@ -156,14 +155,7 @@ class SearchPage extends Component {
}

if (option.reportID) {
this.setState(
{
searchValue: '',
},
() => {
Navigation.dismissModal(option.reportID);
},
);
Navigation.dismissModal(option.reportID);
} else {
Report.navigateToAndOpenReport([option.login]);
}
Expand All @@ -190,7 +182,6 @@ class SearchPage extends Component {
<View style={[this.props.themeStyles.flex1, this.props.themeStyles.w100, this.props.themeStyles.pRelative]}>
<OptionsSelector
sections={sections}
value={this.state.searchValue}
TMisiukiewicz marked this conversation as resolved.
Show resolved Hide resolved
onSelectRow={this.selectReport}
onChangeText={this.onChangeText}
headerMessage={headerMessage}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -307,7 +307,6 @@ function MoneyTemporaryForRefactorRequestParticipantsSelector({
onAddToSelection={addParticipantToSelection}
sections={sections}
selectedOptions={participants}
value={searchTerm}
onSelectRow={addSingleParticipant}
onChangeText={setSearchTermAndSearchInServer}
ref={forwardedRef}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import {withOnyx} from 'react-native-onyx';
import _ from 'underscore';
import Button from '@components/Button';
import FormHelpMessage from '@components/FormHelpMessage';
import {usePersonalDetails} from '@components/OnyxProvider';
import OptionsSelector from '@components/OptionsSelector';
import refPropTypes from '@components/refPropTypes';
import withLocalize, {withLocalizePropTypes} from '@components/withLocalize';
Expand All @@ -16,7 +17,6 @@ import * as Browser from '@libs/Browser';
import compose from '@libs/compose';
import * as OptionsListUtils from '@libs/OptionsListUtils';
import * as ReportUtils from '@libs/ReportUtils';
import personalDetailsPropType from '@pages/personalDetailsPropType';
import reportPropTypes from '@pages/reportPropTypes';
import CONST from '@src/CONST';
import ONYXKEYS from '@src/ONYXKEYS';
Expand Down Expand Up @@ -48,9 +48,6 @@ const propTypes = {
}),
),

/** All of the personal details for everyone */
personalDetails: PropTypes.objectOf(personalDetailsPropType),

/** All reports shared with the user */
reports: PropTypes.objectOf(reportPropTypes),

Expand All @@ -73,7 +70,6 @@ const defaultProps = {
participants: [],
forwardedRef: undefined,
safeAreaPaddingBottomStyle: {},
personalDetails: {},
reports: {},
betas: [],
isDistanceRequest: false,
Expand All @@ -84,7 +80,6 @@ function MoneyRequestParticipantsSelector({
forwardedRef,
betas,
participants,
personalDetails,
reports,
translate,
navigateToRequest,
Expand All @@ -103,7 +98,7 @@ function MoneyRequestParticipantsSelector({
userToInvite: null,
});
const {isOffline} = useNetwork();

const personalDetails = usePersonalDetails();
const maxParticipantsReached = participants.length === CONST.REPORT.MAXIMUM_PARTICIPANTS;

/**
Expand Down Expand Up @@ -160,20 +155,32 @@ function MoneyRequestParticipantsSelector({
}

return newSections;
}, [maxParticipantsReached, newChatOptions, participants, personalDetails, translate, searchTerm]);
}, [maxParticipantsReached, newChatOptions.personalDetails, newChatOptions.recentReports, newChatOptions.userToInvite, participants, personalDetails, searchTerm, translate]);

/**
* Adds a single participant to the request
*
* @param {Object} option
*/
const addSingleParticipant = (option) => {
onAddParticipants(
[{accountID: option.accountID, login: option.login, isPolicyExpenseChat: option.isPolicyExpenseChat, reportID: option.reportID, selected: true, searchText: option.searchText}],
false,
);
navigateToRequest();
};
const addSingleParticipant = useCallback(
(option) => {
onAddParticipants(
[
{
accountID: option.accountID,
login: option.login,
isPolicyExpenseChat: option.isPolicyExpenseChat,
reportID: option.reportID,
selected: true,
searchText: option.searchText,
},
],
false,
);
navigateToRequest();
},
[onAddParticipants, navigateToRequest],
);

/**
* Removes a selected option from list if already selected. If not already selected add this option to the list.
Expand Down Expand Up @@ -215,12 +222,16 @@ function MoneyRequestParticipantsSelector({
[participants, onAddParticipants],
);

const headerMessage = OptionsListUtils.getHeaderMessage(
newChatOptions.personalDetails.length + newChatOptions.recentReports.length !== 0,
Boolean(newChatOptions.userToInvite),
searchTerm.trim(),
maxParticipantsReached,
_.some(participants, (participant) => participant.searchText.toLowerCase().includes(searchTerm.trim().toLowerCase())),
const headerMessage = useMemo(
() =>
OptionsListUtils.getHeaderMessage(
newChatOptions.personalDetails.length + newChatOptions.recentReports.length !== 0,
Boolean(newChatOptions.userToInvite),
searchTerm.trim(),
maxParticipantsReached,
_.some(participants, (participant) => participant.searchText.toLowerCase().includes(searchTerm.trim().toLowerCase())),
),
[maxParticipantsReached, newChatOptions.personalDetails.length, newChatOptions.recentReports.length, newChatOptions.userToInvite, participants, searchTerm],
);
const isOptionsDataReady = ReportUtils.isReportDataReady() && OptionsListUtils.isPersonalDetailsReady(personalDetails);

Expand Down Expand Up @@ -278,23 +289,27 @@ function MoneyRequestParticipantsSelector({
navigateToSplit();
}, [shouldShowSplitBillErrorMessage, navigateToSplit]);

const footerContent = (
<View>
{shouldShowSplitBillErrorMessage && (
<FormHelpMessage
style={[styles.ph1, styles.mb2]}
isError
message="iou.error.splitBillMultipleParticipantsErrorMessage"
const footerContent = useMemo(
() => (
<View>
{shouldShowSplitBillErrorMessage && (
<FormHelpMessage
style={[styles.ph1, styles.mb2]}
isError
message="iou.error.splitBillMultipleParticipantsErrorMessage"
/>
)}
<Button
success
text={translate('iou.addToSplit')}
onPress={handleConfirmSelection}
pressOnEnter
isDisabled={shouldShowSplitBillErrorMessage}
/>
)}
<Button
success
text={translate('iou.addToSplit')}
onPress={handleConfirmSelection}
pressOnEnter
isDisabled={shouldShowSplitBillErrorMessage}
/>
</View>
</View>
),
// eslint-disable-next-line react-hooks/exhaustive-deps
[handleConfirmSelection, shouldShowSplitBillErrorMessage, translate],
);

return (
Expand All @@ -306,7 +321,6 @@ function MoneyRequestParticipantsSelector({
onAddToSelection={addParticipantToSelection}
sections={sections}
selectedOptions={participants}
value={searchTerm}
onSelectRow={addSingleParticipant}
onChangeText={setSearchTermAndSearchInServer}
ref={forwardedRef}
Expand Down Expand Up @@ -346,9 +360,6 @@ MoneyRequestParticipantsSelectorWithRef.displayName = 'MoneyRequestParticipantsS
export default compose(
withLocalize,
withOnyx({
personalDetails: {
key: ONYXKEYS.PERSONAL_DETAILS_LIST,
},
reports: {
key: ONYXKEYS.COLLECTION.REPORT,
},
Expand Down
Loading
Loading