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

Jump to bottom

perf: remove getSearchText function #46409

Merged
merged 9 commits into from
Aug 5, 2024
Merged

perf: remove getSearchText function #46409

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
4981a04
remove usage of searchText for personal details
TMisiukiewicz Jul 29, 2024
dad7d56
remove usage of searchtext in current user option
TMisiukiewicz Jul 29, 2024
a5e171e
remove getSearchText function
TMisiukiewicz Jul 29, 2024
28aa5a4
remove unused search code
TMisiukiewicz Jul 29, 2024
5c333f3
migrate OptionListUtils search tests to filterOptions
TMisiukiewicz Jul 30, 2024
fc0599f
Merge remote-tracking branch 'upstream/main' into perf/getsearchtext
TMisiukiewicz Jul 30, 2024
b4997e2
link issue for email regex
TMisiukiewicz Jul 31, 2024
c3885ac
Merge remote-tracking branch 'upstream/main' into perf/getsearchtext
TMisiukiewicz Aug 5, 2024
23fb1e9
move normalizing search term out of loops
TMisiukiewicz Aug 5, 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
4 changes: 4 additions & 0 deletions src/CONST.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1118,6 +1118,10 @@ const CONST = {
// It's copied here so that the same regex pattern can be used in form validations to be consistent with the server.
VALIDATE_FOR_HTML_TAG_REGEX: /<([^>\s]+)(?:[^>]*?)>/g,

// The regex below is used to remove dots only from the local part of the user email (local-part@domain)
// so when we are using search, we can match emails that have dots without explicitly writing the dots (e.g: fistlast@domain will match first.last@domain)
EMAIL_SEARCH_REGEX: /\.(?=[^\s@]*@)/g,

VALIDATE_FOR_LEADINGSPACES_HTML_TAG_REGEX: /<([\s]+.+[\s]*)>/g,

WHITELISTED_TAGS: [/<>/, /< >/, /<->/, /<-->/, /<br>/, /<br\/>/],
Expand Down
105 changes: 16 additions & 89 deletions src/libs/OptionsListUtils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -512,50 +512,6 @@ function uniqFast(items: string[]): string[] {
return result;
}

/**
* Returns a string with all relevant search terms.
*
* This method must be incredibly performant. It was found to be a big performance bottleneck
* when dealing with accounts that have thousands of reports. For loops are more efficient than _.each
* Array.prototype.push.apply is faster than using the spread operator.
*/
function getSearchText(
report: OnyxInputOrEntry<Report>,
reportName: string,
personalDetailList: Array<Partial<PersonalDetails>>,
isChatRoomOrPolicyExpenseChat: boolean,
isThread: boolean,
): string {
const searchTerms: string[] = [];

for (const personalDetail of personalDetailList) {
if (personalDetail.login) {
// The regex below is used to remove dots only from the local part of the user email (local-part@domain)
// so that we can match emails that have dots without explicitly writing the dots (e.g: fistlast@domain will match first.last@domain)
// More info https://github.com/Expensify/App/issues/8007
Copy link
Contributor

Choose a reason for hiding this comment

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

no need to link this old issue anymore?

searchTerms.push(PersonalDetailsUtils.getDisplayNameOrDefault(personalDetail, '', false), personalDetail.login, personalDetail.login.replace(/\.(?=[^\s@]*@)/g, ''));
}
}

if (report) {
Array.prototype.push.apply(searchTerms, reportName.split(/[,\s]/));

if (isThread) {
const title = ReportUtils.getReportName(report);
const chatRoomSubtitle = ReportUtils.getChatRoomSubtitle(report);

Array.prototype.push.apply(searchTerms, title.split(/[,\s]/));
Array.prototype.push.apply(searchTerms, chatRoomSubtitle?.split(/[,\s]/) ?? ['']);
} else if (isChatRoomOrPolicyExpenseChat) {
const chatRoomSubtitle = ReportUtils.getChatRoomSubtitle(report);

Array.prototype.push.apply(searchTerms, chatRoomSubtitle?.split(/[,\s]/) ?? ['']);
}
Comment on lines -543 to -553
Copy link
Contributor

Choose a reason for hiding this comment

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

We had made threads searchable in #19117.
Please make sure that this removal doesn't cause regression.

}

return uniqFast(searchTerms).join(' ');
}

/**
* Get an object of error messages keyed by microtime by combining all error objects related to the report.
*/
Expand Down Expand Up @@ -788,7 +744,6 @@ function createOption(
phoneNumber: undefined,
hasDraftComment: false,
keyForList: undefined,
searchText: undefined,
isDefaultRoom: false,
isPinned: false,
isWaitingOnBankAccount: false,
Expand Down Expand Up @@ -885,9 +840,6 @@ function createOption(
}

result.text = reportName;
// Disabling this line for safeness as nullish coalescing works only if the value is undefined or null
// eslint-disable-next-line @typescript-eslint/prefer-nullish-coalescing
result.searchText = getSearchText(report, reportName, personalDetailList, !!result.isChatRoom || !!result.isPolicyExpenseChat, !!result.isThread);
result.icons = ReportUtils.getIcons(report, personalDetails, personalDetail?.avatar, personalDetail?.login, personalDetail?.accountID, null);
result.subtitle = subtitle;

Expand Down Expand Up @@ -2062,22 +2014,6 @@ function getOptions(
continue;
}

// Finally check to see if this option is a match for the provided search string if we have one
const {searchText, participantsList, isChatRoom} = reportOption;
const participantNames = getParticipantNames(participantsList);

if (searchValue) {
// Determine if the search is happening within a chat room and starts with the report ID
const isReportIdSearch = isChatRoom && Str.startsWith(reportOption.reportID ?? '-1', searchValue);

// Check if the search string matches the search text or participant names considering the type of the room
const isSearchMatch = isSearchStringMatch(searchValue, searchText, participantNames, isChatRoom);

if (!isReportIdSearch && !isSearchMatch) {
continue;
}
}

reportOption.isSelected = isReportSelected(reportOption, selectedOptions);

if (action === CONST.IOU.ACTION.CATEGORIZE) {
Expand All @@ -2102,19 +2038,11 @@ function getOptions(
if (personalDetailsOptionsToExclude.some((optionToExclude) => optionToExclude.login === personalDetailOption.login)) {
return;
}
const {searchText, participantsList, isChatRoom} = personalDetailOption;
const participantNames = getParticipantNames(participantsList);
if (searchValue && !isSearchStringMatch(searchValue, searchText, participantNames, isChatRoom)) {
return;
}

personalDetailsOptions.push(personalDetailOption);
});

let currentUserOption = allPersonalDetailsOptions.find((personalDetailsOption) => personalDetailsOption.login === currentUserLogin);
if (searchValue && currentUserOption && !isSearchStringMatch(searchValue, currentUserOption.searchText)) {
currentUserOption = undefined;
}
const currentUserOption = allPersonalDetailsOptions.find((personalDetailsOption) => personalDetailsOption.login === currentUserLogin);

let userToInvite: ReportUtils.OptionData | null = null;
if (
Expand Down Expand Up @@ -2451,7 +2379,7 @@ function formatSectionsFromSearchTerm(
// This will add them to the list of options, deduping them if they already exist in the other lists
const selectedParticipantsWithoutDetails = selectedOptions.filter((participant) => {
const accountID = participant.accountID ?? null;
const isPartOfSearchTerm = participant.searchText?.toLowerCase().includes(searchTerm.trim().toLowerCase());
const isPartOfSearchTerm = getPersonalDetailSearchTerms(participant).join(' ').toLowerCase().includes(searchTerm.trim().toLowerCase());
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we move searchTerm.trim().toLowerCase() to outside of the filter ? It seems redundant and we can potentially save some ms here.

Copy link
Contributor

Choose a reason for hiding this comment

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

yep nice catch! No need to transform it each time

const isReportInRecentReports = filteredRecentReports.some((report) => report.accountID === accountID);
const isReportInPersonalDetails = filteredPersonalDetails.some((personalDetail) => personalDetail.accountID === accountID);
return isPartOfSearchTerm && !isReportInRecentReports && !isReportInPersonalDetails;
Expand Down Expand Up @@ -2483,6 +2411,14 @@ function getFirstKeyForList(data?: Option[] | null) {

return firstNonEmptyDataObj.keyForList ? firstNonEmptyDataObj.keyForList : '';
}

function getPersonalDetailSearchTerms(item: Partial<ReportUtils.OptionData>) {
return [item.participantsList?.[0]?.displayName ?? '', item.login ?? '', item.login?.replace(CONST.EMAIL_SEARCH_REGEX, '') ?? ''];
}

function getCurrentUserSearchTerms(item: ReportUtils.OptionData) {
return [item.text ?? '', item.login ?? '', item.login?.replace(CONST.EMAIL_SEARCH_REGEX, '') ?? ''];
}
/**
* Filters options based on the search input value
*/
Expand All @@ -2504,10 +2440,6 @@ function filterOptions(options: Options, searchInputValue: string, config?: Filt
const searchValue = parsedPhoneNumber.possible && parsedPhoneNumber.number?.e164 ? parsedPhoneNumber.number.e164 : searchInputValue.toLowerCase();
const searchTerms = searchValue ? searchValue.split(' ') : [];

// The regex below is used to remove dots only from the local part of the user email (local-part@domain)
// so that we can match emails that have dots without explicitly writing the dots (e.g: fistlast@domain will match first.last@domain)
const emailRegex = /\.(?=[^\s@]*@)/g;

const optionsToExclude: Option[] = [{login: CONST.EMAIL.NOTIFICATIONS}];

excludeLogins.forEach((login) => {
Expand All @@ -2527,7 +2459,7 @@ function filterOptions(options: Options, searchInputValue: string, config?: Filt

if (login) {
keys.push(login);
keys.push(login.replace(emailRegex, ''));
keys.push(login.replace(CONST.EMAIL_SEARCH_REGEX, ''));
}
});
}
Expand All @@ -2543,7 +2475,7 @@ function filterOptions(options: Options, searchInputValue: string, config?: Filt

if (item.login) {
values.push(item.login);
values.push(item.login.replace(emailRegex, ''));
values.push(item.login.replace(CONST.EMAIL_SEARCH_REGEX, ''));
}

if (item.isThread) {
Expand All @@ -2569,15 +2501,9 @@ function filterOptions(options: Options, searchInputValue: string, config?: Filt

return uniqFast(values);
});
const personalDetails = filterArrayByMatch(items.personalDetails, term, (item) =>
uniqFast([item.participantsList?.[0]?.displayName ?? '', item.login ?? '', item.login?.replace(emailRegex, '') ?? '']),
);
const personalDetails = filterArrayByMatch(items.personalDetails, term, (item) => uniqFast(getPersonalDetailSearchTerms(item)));

const currentUserOptionSearchText = uniqFast([
items.currentUserOption?.text ?? '',
items.currentUserOption?.login ?? '',
items.currentUserOption?.login?.replace(emailRegex, '') ?? '',
]).join(' ');
const currentUserOptionSearchText = items.currentUserOption ? uniqFast(getCurrentUserSearchTerms(items.currentUserOption)).join(' ') : '';

const currentUserOption = isSearchStringMatch(term, currentUserOptionSearchText) ? items.currentUserOption : null;

Expand Down Expand Up @@ -2644,7 +2570,6 @@ export {
getSearchValueForPhoneOrEmail,
getPersonalDetailsForAccountIDs,
getIOUConfirmationOptionsFromPayeePersonalDetail,
getSearchText,
isSearchStringMatchUserDetails,
getAllReportErrors,
getPolicyExpenseReportOption,
Expand Down Expand Up @@ -2673,6 +2598,8 @@ export {
canCreateOptimisticPersonalDetailOption,
getUserToInviteOption,
shouldShowViolations,
getPersonalDetailSearchTerms,
getCurrentUserSearchTerms,
};

export type {MemberForList, CategorySection, CategoryTreeSection, Options, OptionList, SearchOption, PayeePersonalDetails, Category, Tax, TaxRatesOption, Option, OptionTree};
1 change: 0 additions & 1 deletion src/libs/SidebarUtils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -469,7 +469,6 @@ function getOptionData({
result.participantsList = participantPersonalDetailList;

result.icons = ReportUtils.getIcons(report, personalDetails, personalDetail?.avatar, personalDetail?.login, personalDetail?.accountID, policy);
result.searchText = OptionsListUtils.getSearchText(report, reportName, participantPersonalDetailList, result.isChatRoom || result.isPolicyExpenseChat, result.isThread);
result.displayNamesWithTooltips = displayNamesWithTooltips;

if (status) {
Expand Down
18 changes: 8 additions & 10 deletions src/pages/NewChatPage.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,16 +86,14 @@ function useOptions({isGroupChat}: NewChatPageProps) {
return filteredOptions;
}, [debouncedSearchTerm, defaultOptions, isGroupChat, selectedOptions]);

const headerMessage = useMemo(
() =>
OptionsListUtils.getHeaderMessage(
options.personalDetails.length + options.recentReports.length !== 0,
!!options.userToInvite,
debouncedSearchTerm.trim(),
selectedOptions.some((participant) => participant?.searchText?.toLowerCase?.().includes(debouncedSearchTerm.trim().toLowerCase())),
),
[debouncedSearchTerm, options.personalDetails.length, options.recentReports.length, options.userToInvite, selectedOptions],
);
const headerMessage = useMemo(() => {
return OptionsListUtils.getHeaderMessage(
options.personalDetails.length + options.recentReports.length !== 0,
!!options.userToInvite,
debouncedSearchTerm.trim(),
selectedOptions.some((participant) => OptionsListUtils.getPersonalDetailSearchTerms(participant).join(' ').toLowerCase?.().includes(debouncedSearchTerm.trim().toLowerCase())),
);
}, [debouncedSearchTerm, options.personalDetails.length, options.recentReports.length, options.userToInvite, selectedOptions]);

useEffect(() => {
if (!debouncedSearchTerm.length) {
Expand Down
2 changes: 1 addition & 1 deletion src/pages/iou/request/MoneyRequestParticipantsSelector.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -216,7 +216,7 @@ function MoneyRequestParticipantsSelector({participants = CONST.EMPTY_ARRAY, onF
(chatOptions.personalDetails ?? []).length + (chatOptions.recentReports ?? []).length !== 0,
!!chatOptions?.userToInvite,
debouncedSearchTerm.trim(),
participants.some((participant) => participant?.searchText?.toLowerCase().includes(debouncedSearchTerm.trim().toLowerCase())),
participants.some((participant) => OptionsListUtils.getPersonalDetailSearchTerms(participant).join(' ').toLowerCase().includes(debouncedSearchTerm.trim().toLowerCase())),
);

return [newSections, headerMessage];
Expand Down
Loading