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: Implement filtering in search page #37909

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
56 commits
Select commit Hold shift + click to select a range
4c017a6
incorporate match-based filtering into codebase
TMisiukiewicz Mar 4, 2024
3cda73b
create function responsible for filtering options
TMisiukiewicz Mar 4, 2024
c2e3157
use filtering on search page
TMisiukiewicz Mar 4, 2024
bd323a0
adapt alghoritm file to match project standards
TMisiukiewicz Mar 5, 2024
4a4ba24
update filtering method & refactor options filtering
TMisiukiewicz Mar 6, 2024
74a2ecd
remove comments
TMisiukiewicz Mar 6, 2024
d2b234c
update search page to filter options
TMisiukiewicz Mar 6, 2024
39b5036
add regex email to filtering results
TMisiukiewicz Mar 7, 2024
b04b4ae
remove sorting
TMisiukiewicz Mar 7, 2024
67e5dc8
update filter by match
TMisiukiewicz Mar 8, 2024
7c2c077
update filtered options state
TMisiukiewicz Mar 8, 2024
d5a8f97
Merge branch 'main' into perf/search-options-filtering
TMisiukiewicz Mar 8, 2024
450c5f0
reduce amount of rerenders
TMisiukiewicz Mar 8, 2024
b71dbea
update linting
TMisiukiewicz Mar 11, 2024
104b658
Merge branch 'main' into perf/search-options-filtering
TMisiukiewicz Mar 11, 2024
4dfeee1
Merge remote-tracking branch 'origin/main' into perf/search-options-f…
TMisiukiewicz Mar 12, 2024
8ea3f82
filter by phone number
TMisiukiewicz Mar 12, 2024
d9d0e84
update search options header
TMisiukiewicz Mar 12, 2024
32a7100
revert package lock
TMisiukiewicz Mar 12, 2024
4702bdc
Merge remote-tracking branch 'upstream/main' into perf/search-options…
TMisiukiewicz Mar 12, 2024
790a191
fix package-lock
TMisiukiewicz Mar 12, 2024
8099082
Merge branch 'main' into perf/search-options-filtering
TMisiukiewicz Mar 18, 2024
40a9715
wrap parsing phone number in a function
TMisiukiewicz Mar 18, 2024
3223b35
fix tests
TMisiukiewicz Mar 18, 2024
15ac693
rewrite filtering function
TMisiukiewicz Mar 18, 2024
5fbc428
Merge remote-tracking branch 'upstream/main' into perf/search-options…
TMisiukiewicz Mar 18, 2024
f370b02
update phone parsing function
TMisiukiewicz Mar 18, 2024
18e28c2
option list utils cleanup
TMisiukiewicz Mar 18, 2024
8d070e4
add tests for filtering
TMisiukiewicz Mar 19, 2024
6636270
add test checking lastVisibleActionCreated
TMisiukiewicz Mar 19, 2024
48fddbd
Merge branch 'main' into perf/search-options-filtering
TMisiukiewicz Mar 21, 2024
99a0cd7
Revert "update phone parsing function"
TMisiukiewicz Mar 21, 2024
671c0d8
Merge remote-tracking branch 'upstream/main' into perf/search-options…
TMisiukiewicz Mar 25, 2024
eb4c265
Merge branch 'main' into perf/search-options-filtering
TMisiukiewicz Apr 3, 2024
ae77968
code review updates
TMisiukiewicz Apr 3, 2024
dea2fb3
move getAcronym to string utils
TMisiukiewicz Apr 3, 2024
528ff86
remove createFilter function
TMisiukiewicz Apr 3, 2024
b0bd01d
update reduce with array function
TMisiukiewicz Apr 3, 2024
eab2574
remove getClosenessRanking function
TMisiukiewicz Apr 3, 2024
f81bbea
Merge remote-tracking branch 'upstream/main' into perf/search-options…
TMisiukiewicz Apr 3, 2024
765ae52
remove keys check
TMisiukiewicz Apr 3, 2024
f962092
update tests
TMisiukiewicz Apr 3, 2024
e3b3b97
change GetOptions type to Options
TMisiukiewicz Apr 5, 2024
5ea71db
remove dot-notation pattern
TMisiukiewicz Apr 5, 2024
51771d9
Merge remote-tracking branch 'upstream/main' into perf/search-options…
TMisiukiewicz Apr 5, 2024
a82c147
Merge branch 'main' into perf/search-options-filtering
TMisiukiewicz Apr 5, 2024
fae57e2
simplify the implementation
TMisiukiewicz Apr 5, 2024
78cb949
Merge branch 'main' into perf/search-options-filtering
TMisiukiewicz Apr 5, 2024
480f3be
Merge branch 'main' into perf/search-options-filtering
TMisiukiewicz Apr 8, 2024
850dd5b
Merge branch 'main' into perf/search-options-filtering
TMisiukiewicz Apr 9, 2024
0682936
fix typecheck
TMisiukiewicz Apr 9, 2024
efedd80
remove debounced state dependency
TMisiukiewicz Apr 9, 2024
f4fdacd
Merge branch 'main' into perf/search-options-filtering
TMisiukiewicz Apr 10, 2024
f94881e
update filtering method
TMisiukiewicz Apr 10, 2024
7620b35
show chatrooms where user is a participant
TMisiukiewicz Apr 10, 2024
5b42f38
fix tests
TMisiukiewicz Apr 10, 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
150 changes: 123 additions & 27 deletions src/libs/OptionsListUtils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ import times from '@src/utils/times';
import Timing from './actions/Timing';
import * as CollectionUtils from './CollectionUtils';
import * as ErrorUtils from './ErrorUtils';
import filterArrayByMatch from './filterArrayByMatch';
import localeCompare from './LocaleCompare';
import * as LocalePhoneNumber from './LocalePhoneNumber';
import * as Localize from './Localize';
Expand Down Expand Up @@ -179,7 +180,7 @@ type MemberForList = {
type SectionForSearchTerm = {
section: CategorySection;
};
type GetOptions = {
type Options = {
recentReports: ReportUtils.OptionData[];
personalDetails: ReportUtils.OptionData[];
userToInvite: ReportUtils.OptionData | null;
Expand Down Expand Up @@ -1497,6 +1498,35 @@ function createOptionFromReport(report: Report, personalDetails: OnyxEntry<Perso
};
}

/**
* Options need to be sorted in the specific order
* @param options - list of options to be sorted
* @param searchValue - search string
* @returns a sorted list of options
*/
function orderOptions(options: ReportUtils.OptionData[], searchValue: string | undefined) {
roryabraham marked this conversation as resolved.
Show resolved Hide resolved
return lodashOrderBy(
options,
[
(option) => {
if (!!option.isChatRoom || option.isArchivedRoom) {
return 3;
}
if (!option.login) {
return 2;
}
if (option.login.toLowerCase() !== searchValue?.toLowerCase()) {
return 1;
}

// When option.login is an exact match with the search value, returning 0 puts it at the top of the option list
return 0;
},
],
['asc'],
);
}

/**
* filter options based on specific conditions
*/
Expand Down Expand Up @@ -1539,7 +1569,7 @@ function getOptions(
policyReportFieldOptions = [],
recentlyUsedPolicyReportFieldOptions = [],
}: GetOptionsConfig,
): GetOptions {
): Options {
if (includeCategories) {
const categoryOptions = getCategoryListSections(categories, recentlyUsedCategories, selectedOptions as Category[], searchInputValue, maxRecentReportsToShow);

Expand Down Expand Up @@ -1597,7 +1627,7 @@ function getOptions(
}

const parsedPhoneNumber = PhoneNumber.parsePhoneNumber(LoginUtils.appendCountryCode(Str.removeSMSDomain(searchInputValue)));
const searchValue = parsedPhoneNumber.possible ? parsedPhoneNumber.number?.e164 : searchInputValue.toLowerCase();
const searchValue = parsedPhoneNumber.possible ? parsedPhoneNumber.number?.e164 ?? '' : searchInputValue.toLowerCase();
const topmostReportId = Navigation.getTopmostReportId() ?? '';

// Filter out all the reports that shouldn't be displayed
Expand Down Expand Up @@ -1847,26 +1877,7 @@ function getOptions(
// When sortByReportTypeInSearch is true, recentReports will be returned with all the reports including personalDetailsOptions in the correct Order.
recentReportOptions.push(...personalDetailsOptions);
personalDetailsOptions = [];
recentReportOptions = lodashOrderBy(
recentReportOptions,
[
(option) => {
if (!!option.isChatRoom || option.isArchivedRoom) {
return 3;
}
if (!option.login) {
return 2;
}
if (option.login.toLowerCase() !== searchValue?.toLowerCase()) {
return 1;
}

// When option.login is an exact match with the search value, returning 0 puts it at the top of the option list
return 0;
},
],
['asc'],
);
recentReportOptions = orderOptions(recentReportOptions, searchValue);
}

return {
Expand All @@ -1883,7 +1894,7 @@ function getOptions(
/**
* Build the options for the Search view
*/
function getSearchOptions(options: OptionList, searchValue = '', betas: Beta[] = []): GetOptions {
function getSearchOptions(options: OptionList, searchValue = '', betas: Beta[] = []): Options {
Timing.start(CONST.TIMING.LOAD_SEARCH_OPTIONS);
Performance.markStart(CONST.TIMING.LOAD_SEARCH_OPTIONS);
const optionList = getOptions(options, {
Expand All @@ -1908,7 +1919,7 @@ function getSearchOptions(options: OptionList, searchValue = '', betas: Beta[] =
return optionList;
}

function getShareLogOptions(options: OptionList, searchValue = '', betas: Beta[] = []): GetOptions {
function getShareLogOptions(options: OptionList, searchValue = '', betas: Beta[] = []): Options {
return getOptions(options, {
betas,
searchInputValue: searchValue.trim(),
Expand Down Expand Up @@ -2085,7 +2096,7 @@ function getMemberInviteOptions(
searchValue = '',
excludeLogins: string[] = [],
includeSelectedOptions = false,
): GetOptions {
): Options {
return getOptions(
{reports: [], personalDetails},
{
Expand Down Expand Up @@ -2204,6 +2215,90 @@ function formatSectionsFromSearchTerm(
};
}

/**
* Filters options based on the search input value
*/
function filterOptions(options: Options, searchInputValue: string): Options {
const searchValue = getSearchValueForPhoneOrEmail(searchInputValue);
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;
Copy link
Contributor

Choose a reason for hiding this comment

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

probably you can initialize it outside of the function

Copy link
Contributor Author

Choose a reason for hiding this comment

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

as it is not used anywhere else, I'd leave it here so it's clear where and why this regex belongs to

Copy link
Contributor

Choose a reason for hiding this comment

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

rather than using a regex to replace dots in emails, can we just have filterArrayByMatch ignore dots, semicolons, dashes, etc... by default?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hmm, I think it might negatively affect the performance, because we would have to call it for each value under a specified keys. With current approach, we can limit those checks only to the keys we need.


const getParticipantsLoginsArray = (item: ReportUtils.OptionData) => {
const keys: string[] = [];
const visibleChatMemberAccountIDs = item.participantsList ?? [];
if (allPersonalDetails) {
visibleChatMemberAccountIDs.forEach((participant) => {
const login = participant?.login;

if (participant?.displayName) {
keys.push(participant.displayName);
}

if (login) {
keys.push(login);
keys.push(login.replace(emailRegex, ''));
}
});
}

return keys;
};
const matchResults = searchTerms.reduceRight((items, term) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm a bit confused why we're doing a reduce here, is it intentional that we call filterArrayByMatch separately for every search term? doesn't think mean we're filtering over all the results iteratively, once for each search term? Wouldn't it be more efficient to filter all the results once, accounting for all the search terms in a single pass?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it might be hard to achieve, even current implementation loops through all the search terms.

By default filterArrayByMatch returns items where the properties matches the entire search term. For example, when searching for Foo Bar, it will return only those results where Foo Bar exists in a given keys. With reduce, we can match both Foo and Bar separ ately and return rows where two words are found not only in the same column, but also in different columns.

A good example from the app is searching for Tomasz Callstack. If we would look for the whole phrase, we would get 0 results. But with reduce, we get all the options that matches both terms. That's because Tomasz was found e.g. in a displayName and login properties, and Callstack was found as a part of an email address in a login property.

Additionally, note that with every next term, we are filtering the results narrowed to those matching the previous search terms.

const recentReports = filterArrayByMatch(items.recentReports, term, (item) => {
let values: string[] = [];
if (item.text) {
values.push(item.text);
}

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

if (item.isThread) {
if (item.alternateText) {
values.push(item.alternateText);
}
} else if (!!item.isChatRoom || !!item.isPolicyExpenseChat) {
if (item.subtitle) {
values.push(item.subtitle);
}
}
values = values.concat(getParticipantsLoginsArray(item));

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

return {
recentReports: recentReports ?? [],
personalDetails: personalDetails ?? [],
userToInvite: null,
currentUserOption: null,
categoryOptions: [],
tagOptions: [],
taxRatesOptions: [],
};
}, options);

const recentReports = matchResults.recentReports.concat(matchResults.personalDetails);

return {
personalDetails: [],
recentReports: orderOptions(recentReports, searchValue),
userToInvite: null,
currentUserOption: null,
categoryOptions: [],
tagOptions: [],
taxRatesOptions: [],
};
}

export {
getAvatarsForAccountIDs,
isCurrentUser,
Expand Down Expand Up @@ -2236,10 +2331,11 @@ export {
formatSectionsFromSearchTerm,
transformedTaxRates,
getShareLogOptions,
filterOptions,
createOptionList,
createOptionFromReport,
getReportOption,
getTaxRatesSection,
};

export type {MemberForList, CategorySection, CategoryTreeSection, GetOptions, OptionList, SearchOption, PayeePersonalDetails, Category, TaxRatesOption};
export type {MemberForList, CategorySection, CategoryTreeSection, Options, OptionList, SearchOption, PayeePersonalDetails, Category, TaxRatesOption};
19 changes: 18 additions & 1 deletion src/libs/StringUtils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -72,4 +72,21 @@ function normalizeCRLF(value?: string): string | undefined {
return value?.replace(/\r\n/g, '\n');
}

export default {sanitizeString, isEmptyString, removeInvisibleCharacters, normalizeCRLF};
/**
* Generates an acronym for a string.
* @param string the string for which to produce the acronym
* @returns the acronym
*/
function getAcronym(string: string): string {
let acronym = '';
const wordsInString = string.split(' ');
wordsInString.forEach((wordInString) => {
const splitByHyphenWords = wordInString.split('-');
splitByHyphenWords.forEach((splitByHyphenWord) => {
acronym += splitByHyphenWord.substring(0, 1);
});
});
return acronym;
}

export default {sanitizeString, isEmptyString, removeInvisibleCharacters, normalizeCRLF, getAcronym};
117 changes: 117 additions & 0 deletions src/libs/filterArrayByMatch.ts
Copy link
Contributor

Choose a reason for hiding this comment

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

would be nice to leave a comment that this is a slim version of what's available in match-sorter

Original file line number Diff line number Diff line change
@@ -0,0 +1,117 @@
/**
* This file is a slim version of match-sorter library (https://github.com/kentcdodds/match-sorter) adjusted to the needs.
Use `threshold` option with one of the rankings defined below to control the strictness of the match.
*/
import type {ValueOf} from 'type-fest';
import StringUtils from './StringUtils';

const MATCH_RANK = {
CASE_SENSITIVE_EQUAL: 7,
EQUAL: 6,
STARTS_WITH: 5,
WORD_STARTS_WITH: 4,
CONTAINS: 3,
ACRONYM: 2,
MATCHES: 1,
NO_MATCH: 0,
} as const;

type Ranking = ValueOf<typeof MATCH_RANK>;

/**
* Gives a rankings score based on how well the two strings match.
* @param testString - the string to test against
* @param stringToRank - the string to rank
* @returns the ranking for how well stringToRank matches testString
*/
function getMatchRanking(testString: string, stringToRank: string): Ranking {
// too long
if (stringToRank.length > testString.length) {
return MATCH_RANK.NO_MATCH;
}

// case sensitive equals
if (testString === stringToRank) {
return MATCH_RANK.CASE_SENSITIVE_EQUAL;
}

// Lower casing before further comparison
const lowercaseTestString = testString.toLowerCase();
const lowercaseStringToRank = stringToRank.toLowerCase();

// case insensitive equals
if (lowercaseTestString === lowercaseStringToRank) {
return MATCH_RANK.EQUAL;
}

// starts with
if (lowercaseTestString.startsWith(lowercaseStringToRank)) {
return MATCH_RANK.STARTS_WITH;
}

// word starts with
if (lowercaseTestString.includes(` ${lowercaseStringToRank}`)) {
return MATCH_RANK.WORD_STARTS_WITH;
}

// contains
if (lowercaseTestString.includes(lowercaseStringToRank)) {
return MATCH_RANK.CONTAINS;
}
if (lowercaseStringToRank.length === 1) {
return MATCH_RANK.NO_MATCH;
}

// acronym
if (StringUtils.getAcronym(lowercaseTestString).includes(lowercaseStringToRank)) {
return MATCH_RANK.ACRONYM;
}

// will return a number between rankings.MATCHES and rankings.MATCHES + 1 depending on how close of a match it is.
let matchingInOrderCharCount = 0;
let charNumber = 0;
for (const char of stringToRank) {
charNumber = lowercaseTestString.indexOf(char, charNumber) + 1;
if (!charNumber) {
return MATCH_RANK.NO_MATCH;
}
matchingInOrderCharCount++;
}

// Calculate ranking based on character sequence and spread
const spread = charNumber - lowercaseTestString.indexOf(stringToRank[0]);
const spreadPercentage = 1 / spread;
const inOrderPercentage = matchingInOrderCharCount / stringToRank.length;
const ranking = MATCH_RANK.MATCHES + inOrderPercentage * spreadPercentage;

return ranking as Ranking;
}

/**
* Takes an array of items and a value and returns a new array with the items that match the given value
* @param items - the items to filter
* @param searchValue - the value to use for ranking
* @param extractRankableValuesFromItem - an array of functions
* @returns the new filtered array
*/
function filterArrayByMatch<T = string>(items: readonly T[], searchValue: string, extractRankableValuesFromItem: (item: T) => string[]): T[] {
const filteredItems = [];
for (const item of items) {
const valuesToRank = extractRankableValuesFromItem(item);
let itemRank: Ranking = MATCH_RANK.NO_MATCH;
for (const value of valuesToRank) {
const rank = getMatchRanking(value, searchValue);
if (rank > itemRank) {
itemRank = rank;
}
}

if (itemRank >= MATCH_RANK.MATCHES + 1) {
filteredItems.push(item);
}
}
return filteredItems;
}

export default filterArrayByMatch;
export {MATCH_RANK};
Loading
Loading