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

[Search v1] Add sorting #42248

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions assets/images/arrow-down-long.svg
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
1 change: 1 addition & 0 deletions assets/images/arrow-up-long.svg
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
6 changes: 6 additions & 0 deletions src/CONST.ts
Original file line number Diff line number Diff line change
Expand Up @@ -931,6 +931,7 @@ const CONST = {
RECEIPT: 'receipt',
DATE: 'date',
MERCHANT: 'merchant',
DESCRIPTION: 'description',
FROM: 'from',
TO: 'to',
CATEGORY: 'category',
Expand Down Expand Up @@ -4776,6 +4777,11 @@ const CONST = {
REFERRER: {
NOTIFICATION: 'notification',
},

SORT_ORDER: {
ASC: 'asc',
DESC: 'desc',
},
} as const;

type Country = keyof typeof CONST.ALL_COUNTRIES;
Expand Down
12 changes: 11 additions & 1 deletion src/ROUTES.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@ import type {ValueOf} from 'type-fest';
import type CONST from './CONST';
import type {IOUAction, IOUType} from './CONST';
import type {IOURequestType} from './libs/actions/IOU';
import type {CentralPaneNavigatorParamList} from './libs/Navigation/types';
import type {SearchQuery} from './types/onyx/SearchResults';
import type AssertTypesNotEqual from './types/utils/AssertTypesNotEqual';

// This is a file containing constants for all the routes we want to be able to go to
Expand All @@ -25,7 +27,15 @@ const ROUTES = {

SEARCH: {
route: '/search/:query',
getRoute: (query: string) => `search/${query}` as const,
getRoute: (searchQuery: SearchQuery, queryParams?: CentralPaneNavigatorParamList['Search_Central_Pane']) => {
const {sortBy, sortOrder} = queryParams ?? {};

if (!sortBy && !sortOrder) {
return `search/${searchQuery}` as const;
}

return `search/${searchQuery}?sortBy=${sortBy}&sortOrder=${sortOrder}` as const;
},
},

SEARCH_REPORT: {
Expand Down
4 changes: 4 additions & 0 deletions src/components/Icon/Expensicons.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,10 @@ import AddReaction from '@assets/images/add-reaction.svg';
import All from '@assets/images/all.svg';
import Android from '@assets/images/android.svg';
import Apple from '@assets/images/apple.svg';
import ArrowDownLong from '@assets/images/arrow-down-long.svg';
import ArrowRightLong from '@assets/images/arrow-right-long.svg';
import ArrowRight from '@assets/images/arrow-right.svg';
import ArrowUpLong from '@assets/images/arrow-up-long.svg';
import UpArrow from '@assets/images/arrow-up.svg';
import ArrowsUpDown from '@assets/images/arrows-updown.svg';
import AdminRoomAvatar from '@assets/images/avatars/admin-room.svg';
Expand Down Expand Up @@ -182,6 +184,8 @@ export {
ArrowRight,
ArrowRightLong,
ArrowsUpDown,
ArrowUpLong,
ArrowDownLong,
Wrench,
BackArrow,
Bank,
Expand Down
62 changes: 48 additions & 14 deletions src/components/Search.tsx
Original file line number Diff line number Diff line change
@@ -1,16 +1,23 @@
import React, {useEffect} from 'react';
import {useNavigation} from '@react-navigation/native';
import type {StackNavigationProp} from '@react-navigation/stack';
import React, {useEffect, useRef} from 'react';
import type {OnyxEntry} from 'react-native-onyx';
import {useOnyx} from 'react-native-onyx';
import useNetwork from '@hooks/useNetwork';
import useThemeStyles from '@hooks/useThemeStyles';
import * as SearchActions from '@libs/actions/Search';
import * as DeviceCapabilities from '@libs/DeviceCapabilities';
import Log from '@libs/Log';
import * as SearchUtils from '@libs/SearchUtils';
import type {SearchColumnType, SortOrder} from '@libs/SearchUtils';
import Navigation from '@navigation/Navigation';
import type {CentralPaneNavigatorParamList} from '@navigation/types';
import EmptySearchView from '@pages/Search/EmptySearchView';
import CONST from '@src/CONST';
import ONYXKEYS from '@src/ONYXKEYS';
import ROUTES from '@src/ROUTES';
import type {SearchQuery} from '@src/types/onyx/SearchResults';
import type SearchResults from '@src/types/onyx/SearchResults';
import {isEmptyObject} from '@src/types/utils/EmptyObject';
import isLoadingOnyxValue from '@src/types/utils/isLoadingOnyxValue';
import SelectionList from './SelectionList';
Expand All @@ -19,36 +26,47 @@ import type {ReportListItemType, TransactionListItemType} from './SelectionList/
import TableListItemSkeleton from './Skeletons/TableListItemSkeleton';

type SearchProps = {
query: string;
query: SearchQuery;
policyIDs?: string;
sortBy?: SearchColumnType;
sortOrder?: SortOrder;
};

function isReportListItemType(item: TransactionListItemType | ReportListItemType): item is ReportListItemType {
const reportListItem = item as ReportListItemType;
return reportListItem.transactions !== undefined;
}

function Search({query, policyIDs}: SearchProps) {
function Search({query, policyIDs, sortBy, sortOrder}: SearchProps) {
const {isOffline} = useNetwork();
const styles = useThemeStyles();
const navigation = useNavigation<StackNavigationProp<CentralPaneNavigatorParamList>>();
const lastSearchResultsRef = useRef<OnyxEntry<SearchResults>>();

const hash = SearchUtils.getQueryHash(query, policyIDs);
const [searchResults, searchResultsMeta] = useOnyx(`${ONYXKEYS.COLLECTION.SNAPSHOT}${hash}`);
const hash = SearchUtils.getQueryHash(query, policyIDs, sortBy, sortOrder);
const [currentSearchResults, searchResultsMeta] = useOnyx(`${ONYXKEYS.COLLECTION.SNAPSHOT}${hash}`);

// save last non-empty search results to avoid ugly flash of loading screen when hash changes and onyx returns empty data
if (currentSearchResults?.data && currentSearchResults !== lastSearchResultsRef.current) {
lastSearchResultsRef.current = currentSearchResults;
}

const searchResults = currentSearchResults?.data ? currentSearchResults : lastSearchResultsRef.current;

useEffect(() => {
if (isOffline) {
return;
}

SearchActions.search(hash, query, 0, policyIDs);
SearchActions.search({hash, query, policyIDs, offset: 0, sortBy, sortOrder});
// eslint-disable-next-line react-hooks/exhaustive-deps
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if we should be computing a new hash for each set of sorting param since the search results would be different than what we currently have. I think we should at least reset the offset, since the following seems to be possible:

  1. Open the search page - the default sort for date desc applies
  2. Scroll down to load more results, offset will be updated to 50
  3. Sort by a different column, e.g. category
  4. We'll fetch the results from the API sorted by category, but we'll get the 2nd "page" since the offset is 50.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe one optimization would be when hasMoreResults = false since we know we have all available data and wouldn't need to fetch anything else to properly sort things in the client.

Copy link
Contributor Author

@Kicu Kicu May 22, 2024

Choose a reason for hiding this comment

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

I wonder if we should be computing a new hash for each set of sorting param

I agree with this and I will add sortBy and sortOrder to hash generation.

However I think the scenario you described would not happen because in this specific line offset is set always to 0 inside useEffect.
So in your scenario in point 3 if you sort by different column or change order, then this useEffect block will be in place and a fresh call will be made with offset: 0. If you keep scrolling then another call will be made with offset 50 or whatever, but the one with the 0 was already there.

Perhaps another solution is to scroll list to top if you switch sort/order?
I now believe that adding sortBy + sortOrder into hash solves all our problems because onyx key changes.
Please verify this yourself

}, [hash, isOffline]);

const isLoadingInitialItems = (!isOffline && isLoadingOnyxValue(searchResultsMeta)) || searchResults?.data === undefined;
const isLoadingMoreItems = !isLoadingInitialItems && searchResults?.search?.isLoading;
const shouldShowEmptyState = !isLoadingInitialItems && isEmptyObject(searchResults?.data);
const isLoadingItems = (!isOffline && isLoadingOnyxValue(searchResultsMeta)) || searchResults?.data === undefined;
const isLoadingMoreItems = !isLoadingItems && searchResults?.search?.isLoading;
const shouldShowEmptyState = !isLoadingItems && isEmptyObject(searchResults?.data);

if (isLoadingInitialItems) {
if (isLoadingItems) {
return <TableListItemSkeleton shouldAnimate />;
}

Expand All @@ -65,11 +83,11 @@ function Search({query, policyIDs}: SearchProps) {
};

const fetchMoreResults = () => {
if (!searchResults?.search?.hasMoreResults || isLoadingInitialItems || isLoadingMoreItems) {
if (!searchResults?.search?.hasMoreResults || isLoadingItems || isLoadingMoreItems) {
return;
}
const currentOffset = searchResults?.search?.offset ?? 0;
SearchActions.search(hash, query, currentOffset + CONST.SEARCH_RESULTS_PAGE_SIZE);
SearchActions.search({hash, query, offset: currentOffset + CONST.SEARCH_RESULTS_PAGE_SIZE, sortBy, sortOrder});
};

const type = SearchUtils.getSearchType(searchResults?.search);
Expand All @@ -83,9 +101,25 @@ function Search({query, policyIDs}: SearchProps) {

const data = SearchUtils.getSections(searchResults?.data ?? {}, type);

const onSortPress = (column: SearchColumnType, order: SortOrder) => {
navigation.setParams({
sortBy: column,
sortOrder: order,
});
};

const sortedData = SearchUtils.getSortedSections(type, data, sortBy, sortOrder);

return (
<SelectionList<ReportListItemType | TransactionListItemType>
customListHeader={<SearchTableHeader data={searchResults?.data} />}
customListHeader={
<SearchTableHeader
data={searchResults?.data}
onSortPress={onSortPress}
sortOrder={sortOrder}
sortBy={sortBy}
/>
}
// To enhance the smoothness of scrolling and minimize the risk of encountering blank spaces during scrolling,
// we have configured a larger windowSize and a longer delay between batch renders.
// The windowSize determines the number of items rendered before and after the currently visible items.
Expand All @@ -98,7 +132,7 @@ function Search({query, policyIDs}: SearchProps) {
windowSize={111}
updateCellsBatchingPeriod={200}
ListItem={ListItem}
sections={[{data, isDisabled: false}]}
sections={[{data: sortedData, isDisabled: false}]}
onSelectRow={(item) => {
const reportID = isReportListItemType(item) ? item.reportID : item.transactionThreadReportID;

Expand Down
14 changes: 11 additions & 3 deletions src/components/SelectionList/Search/ExpenseItemHeaderNarrow.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -13,11 +13,13 @@ import UserInfoCell from './UserInfoCell';
type ExpenseItemHeaderNarrowProps = {
participantFrom: SearchAccountDetails;
participantTo: SearchAccountDetails;
participantFromDisplayName: string;
participantToDisplayName: string;
buttonText: string;
onButtonPress: () => void;
};

function ExpenseItemHeaderNarrow({participantFrom, participantTo, buttonText, onButtonPress}: ExpenseItemHeaderNarrowProps) {
function ExpenseItemHeaderNarrow({participantFrom, participantFromDisplayName, participantTo, participantToDisplayName, buttonText, onButtonPress}: ExpenseItemHeaderNarrowProps) {
const styles = useThemeStyles();
const StyleUtils = useStyleUtils();
const theme = useTheme();
Expand All @@ -26,7 +28,10 @@ function ExpenseItemHeaderNarrow({participantFrom, participantTo, buttonText, on
<View style={[styles.flex1, styles.flexRow, styles.alignItemsCenter, styles.justifyContentBetween, styles.mb2, styles.gap2]}>
<View style={[styles.flexRow, styles.alignItemsCenter, styles.gap1, styles.flex1]}>
<View style={[styles.mw50]}>
<UserInfoCell participant={participantFrom} />
<UserInfoCell
participant={participantFrom}
displayName={participantFromDisplayName}
/>
</View>
<Icon
src={Expensicons.ArrowRightLong}
Expand All @@ -35,7 +40,10 @@ function ExpenseItemHeaderNarrow({participantFrom, participantTo, buttonText, on
fill={theme.icon}
/>
<View style={[styles.flex1, styles.mw50]}>
<UserInfoCell participant={participantTo} />
<UserInfoCell
participant={participantTo}
displayName={participantToDisplayName}
/>
</View>
</View>
<View style={[StyleUtils.getWidthStyle(variables.w80)]}>
Expand Down
7 changes: 7 additions & 0 deletions src/components/SelectionList/Search/ReportListItem.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,11 @@ function ReportListItem<TItem extends ListItem>({
const participantFrom = reportItem.transactions[0].from;
const participantTo = reportItem.transactions[0].to;

// These values should come as part of the item via SearchUtils.getSections() but ReportListItem is not yet 100% handled
// This will be simplified in future once sorting of ReportListItem is done
const participantFromDisplayName = participantFrom?.name ?? participantFrom?.displayName ?? participantFrom?.login ?? '';
const participantToDisplayName = participantTo?.name ?? participantTo?.displayName ?? participantTo?.login ?? '';

if (reportItem.transactions.length === 1) {
const transactionItem = reportItem.transactions[0];

Expand Down Expand Up @@ -118,7 +123,9 @@ function ReportListItem<TItem extends ListItem>({
{!isLargeScreenWidth && (
<ExpenseItemHeaderNarrow
participantFrom={participantFrom}
participantFromDisplayName={participantFromDisplayName}
participantTo={participantTo}
participantToDisplayName={participantToDisplayName}
buttonText={translate('common.view')}
onButtonPress={handleOnButtonPress}
/>
Expand Down
Loading
Loading