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 9 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
122 changes: 102 additions & 20 deletions src/libs/OptionsListUtils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,8 @@ 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 type {KeyOption} from './filterArrayByMatch';
import localeCompare from './LocaleCompare';
import * as LocalePhoneNumber from './LocalePhoneNumber';
import * as Localize from './Localize';
Expand Down Expand Up @@ -149,6 +151,8 @@ type GetOptions = {

type PreviewConfig = {showChatPreviewLine?: boolean; forcePolicyNamePreview?: boolean};

type ReportTypesOptionData = Record<string, ReportUtils.OptionData[]>;

/**
* OptionsListUtils is used to build a list options passed to the OptionsList component. Several different UI views can
* be configured to display different results based on the options passed to the private getOptions() method. Public
Expand Down Expand Up @@ -1306,6 +1310,35 @@ function isReportSelected(reportOption: ReportUtils.OptionData, selectedOptions:
return selectedOptions.some((option) => (option.accountID && option.accountID === reportOption.accountID) || (option.reportID && option.reportID === reportOption.reportID));
}

/**
* 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'],
);
}

/**
* Build the options
*/
Expand Down Expand Up @@ -1656,26 +1689,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 Down Expand Up @@ -1997,6 +2011,73 @@ function formatSectionsFromSearchTerm(
};
}

function filterOptions(options: GetOptions, searchValue: string): ReportUtils.OptionData[] {
const searchTerms = searchValue.split(' ');

const reportsByType = options.recentReports.reduce<ReportTypesOptionData>(
(acc, option) => {
if (!!option.isChatRoom || !!option.isPolicyExpenseChat) {
acc.chatRoomsAndPolicyExpenseChats.push(option);
} else {
acc.reports.push(option);
}

return acc;
},
{
chatRoomsAndPolicyExpenseChats: [],
reports: [],
personalDetails: options.personalDetails,
},
);
const createFilter = (items: ReportUtils.OptionData[], keys: ReadonlyArray<KeyOption<ReportUtils.OptionData>>, term: string) =>
Copy link
Contributor

Choose a reason for hiding this comment

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

This createFilter function appears prettymuch useless to me, and it's adding some complexity as an additional layer of abstraction. Let's remove it

filterArrayByMatch(items, term, {
keys,
strict: true,
});

// 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 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 personalDetails = createFilter(
items.personalDetails,
[
'text',
'login',
(item) => (item.login ? item.login.replace(emailRegex, '') : []),
'participantsList.0.displayName',
'participantsList.0.firstName',
'participantsList.0.lastName',
],
term,
);
const chatRoomsAndPolicyExpenseChats = createFilter(items.chatRoomsAndPolicyExpenseChats, ['text', 'alternateText'], term);
const reports = createFilter(
items.reports,
[
'text',
'participantsList.0.login',
(item) => (item.participantsList ? item.participantsList.filter(({login}) => login).map((i) => (i.login ? i.login.replace(emailRegex, '') : '')) : []),
'participantsList.0.displayName',
'participantsList.0.firstName',
'participantsList.0.lastName',
],
term,
);

return {
reports,
personalDetails,
chatRoomsAndPolicyExpenseChats,
};
}, reportsByType);

const filteredOptions = Object.values(matchResults).flat();
return orderOptions(filteredOptions, searchValue);
}

export {
getAvatarsForAccountIDs,
isCurrentUser,
Expand Down Expand Up @@ -2027,6 +2108,7 @@ export {
formatSectionsFromSearchTerm,
transformedTaxRates,
getShareLogOptions,
filterOptions,
};

export type {MemberForList, CategorySection, GetOptions};
Loading
Loading