-
Notifications
You must be signed in to change notification settings - Fork 3.5k
Can't select an empty report from the Reports > Reports page #81036
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
base: main
Are you sure you want to change the base?
Changes from all commits
d4054c0
cabeca2
025c7d2
4339947
e98d167
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -48,7 +48,7 @@ import useWindowDimensions from '@hooks/useWindowDimensions'; | |
| import {turnOnMobileSelectionMode} from '@libs/actions/MobileSelectionMode'; | ||
| import DateUtils from '@libs/DateUtils'; | ||
| import navigationRef from '@libs/Navigation/navigationRef'; | ||
| import {getTableMinWidth} from '@libs/SearchUIUtils'; | ||
| import {getTableMinWidth, isTransactionReportGroupListItemType} from '@libs/SearchUIUtils'; | ||
| import variables from '@styles/variables'; | ||
| import type {TransactionPreviewData} from '@userActions/Search'; | ||
| import CONST from '@src/CONST'; | ||
|
|
@@ -239,16 +239,51 @@ function SearchList({ | |
| } | ||
| return data; | ||
| }, [data, groupBy, type]); | ||
| const flattenedItemsWithoutPendingDelete = useMemo(() => flattenedItems.filter((t) => t?.pendingAction !== CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE), [flattenedItems]); | ||
| const emptyReports = useMemo(() => { | ||
| if (type === CONST.SEARCH.DATA_TYPES.EXPENSE_REPORT && isTransactionGroupListItemArray(data)) { | ||
| return data.filter((item) => item.transactions.length === 0); | ||
| } | ||
| return []; | ||
| }, [data, type]); | ||
|
|
||
| const selectedItemsLength = useMemo(() => { | ||
| return flattenedItemsWithoutPendingDelete.reduce((acc, item) => { | ||
| if (item.keyForList && selectedTransactions[item.keyForList]?.isSelected) { | ||
| return acc + 1; | ||
| } | ||
| return acc; | ||
| const selectedTransactionsCount = flattenedItems.reduce((acc, item) => { | ||
| const isTransactionSelected = !!(item?.keyForList && selectedTransactions[item.keyForList]?.isSelected); | ||
| return acc + (isTransactionSelected ? 1 : 0); | ||
| }, 0); | ||
| }, [flattenedItemsWithoutPendingDelete, selectedTransactions]); | ||
|
|
||
| if (type === CONST.SEARCH.DATA_TYPES.EXPENSE_REPORT && isTransactionGroupListItemArray(data)) { | ||
| const selectedEmptyReports = emptyReports.reduce((acc, item) => { | ||
| const isEmptyReportSelected = !!(item.keyForList && selectedTransactions[item.keyForList]?.isSelected); | ||
| return acc + (isEmptyReportSelected ? 1 : 0); | ||
| }, 0); | ||
|
|
||
| return selectedEmptyReports + selectedTransactionsCount; | ||
| } | ||
|
|
||
| return selectedTransactionsCount; | ||
| }, [flattenedItems, type, data, emptyReports, selectedTransactions]); | ||
|
|
||
| const totalItems = useMemo(() => { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ❌ PERF-13 (docs)The Suggested fix: Hoist the type check and consolidate the filtering logic: const totalItems = useMemo(() => {
const isExpenseReportType = type === CONST.SEARCH.DATA_TYPES.EXPENSE_REPORT;
const isGroupedData = isTransactionGroupListItemArray(data);
const selectableTransactions = flattenedItems.filter((item) => {
if ('pendingAction' in item) {
return item.pendingAction !== CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE;
}
return true;
});
if (isExpenseReportType && isGroupedData) {
const selectableEmptyReports = emptyReports.filter((item) =>
item.pendingAction !== CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE
);
return selectableEmptyReports.length + selectableTransactions.length;
}
return selectableTransactions.length;
}, [data, type, flattenedItems, emptyReports]);Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency. |
||
| if (type === CONST.SEARCH.DATA_TYPES.EXPENSE_REPORT && isTransactionGroupListItemArray(data)) { | ||
| const selectableEmptyReports = emptyReports.filter((item) => item.pendingAction !== CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE); | ||
| const selectableTransactions = flattenedItems.filter((item) => { | ||
| if ('pendingAction' in item) { | ||
| return item.pendingAction !== CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE; | ||
| } | ||
| return true; | ||
| }); | ||
| return selectableEmptyReports.length + selectableTransactions.length; | ||
| } | ||
|
|
||
| const selectableTransactions = flattenedItems.filter((item) => { | ||
| if ('pendingAction' in item) { | ||
| return item.pendingAction !== CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE; | ||
| } | ||
| return true; | ||
| }); | ||
| return selectableTransactions.length; | ||
| }, [data, type, flattenedItems, emptyReports]); | ||
|
|
||
| const itemsWithSelection = useMemo(() => { | ||
| return data.map((item) => { | ||
|
|
@@ -259,10 +294,16 @@ function SearchList({ | |
| if (!canSelectMultiple) { | ||
| itemWithSelection = {...item, isSelected: false}; | ||
| } else { | ||
| const hasAnySelected = item.transactions.some((t) => t.keyForList && selectedTransactions[t.keyForList]?.isSelected); | ||
| const isEmptyReportSelected = | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ❌ PERF-13 (docs)The Suggested fix: Calculate the type check once per item to improve clarity: const itemsWithSelection = useMemo(() => {
return data.map((item) => {
let isSelected = false;
let itemWithSelection: SearchListItem = item;
if ('transactions' in item && item.transactions) {
if (\!canSelectMultiple) {
itemWithSelection = {...item, isSelected: false};
} else {
const isEmpty = item.transactions.length === 0;
const isReportType = isTransactionReportGroupListItemType(item);
const isEmptyReportSelected = isEmpty && isReportType && \!\!(item.keyForList && selectedTransactions[item.keyForList]?.isSelected);
const hasAnySelected = item.transactions.some((t) => t.keyForList && selectedTransactions[t.keyForList]?.isSelected) || isEmptyReportSelected;
if (\!hasAnySelected) {
itemWithSelection = {...item, isSelected: false};
} else if (isEmptyReportSelected) {
isSelected = true;
itemWithSelection = {...item, isSelected};
} else {
// ... rest of the logic
}
}
} else {
isSelected = \!\!(canSelectMultiple && item.keyForList && selectedTransactions[item.keyForList]?.isSelected);
itemWithSelection = {...item, isSelected};
}
return {originalItem: item, itemWithSelection, isSelected};
});
}, [data, canSelectMultiple, selectedTransactions]);Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency. |
||
| item.transactions.length === 0 && isTransactionReportGroupListItemType(item) && !!(item.keyForList && selectedTransactions[item.keyForList]?.isSelected); | ||
|
|
||
| const hasAnySelected = item.transactions.some((t) => t.keyForList && selectedTransactions[t.keyForList]?.isSelected) || isEmptyReportSelected; | ||
|
|
||
| if (!hasAnySelected) { | ||
| itemWithSelection = {...item, isSelected: false}; | ||
| } else if (isEmptyReportSelected) { | ||
| isSelected = true; | ||
| itemWithSelection = {...item, isSelected}; | ||
| } else { | ||
| let allNonDeletedSelected = true; | ||
| let hasNonDeletedTransactions = false; | ||
|
|
@@ -351,13 +392,10 @@ function SearchList({ | |
| } | ||
|
|
||
| // eslint-disable-next-line @typescript-eslint/prefer-nullish-coalescing | ||
| if (shouldPreventLongPressRow || !isSmallScreenWidth || item?.isDisabled || item?.isDisabledCheckbox) { | ||
| return; | ||
| } | ||
| // disable long press for empty expense reports | ||
| if ('transactions' in item && item.transactions.length === 0 && !groupBy) { | ||
| if (shouldPreventLongPressRow || !isSmallScreenWidth || item?.isDisabled || item?.isDisabledCheckbox || item.pendingAction === CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE) { | ||
| return; | ||
| } | ||
|
|
||
| if (isMobileSelectionModeEnabled) { | ||
| onCheckboxPress(item, itemTransactions); | ||
| return; | ||
|
|
@@ -366,7 +404,7 @@ function SearchList({ | |
| setLongPressedItemTransactions(itemTransactions); | ||
| setIsModalVisible(true); | ||
| }, | ||
| [groupBy, route.key, shouldPreventLongPressRow, isSmallScreenWidth, isMobileSelectionModeEnabled, onCheckboxPress], | ||
| [route.key, shouldPreventLongPressRow, isSmallScreenWidth, isMobileSelectionModeEnabled, onCheckboxPress], | ||
| ); | ||
|
|
||
| const turnOnSelectionMode = useCallback(() => { | ||
|
|
@@ -493,7 +531,7 @@ function SearchList({ | |
|
|
||
| const tableHeaderVisible = canSelectMultiple || !!SearchTableHeader; | ||
| const selectAllButtonVisible = canSelectMultiple && !SearchTableHeader; | ||
| const isSelectAllChecked = selectedItemsLength > 0 && selectedItemsLength === flattenedItemsWithoutPendingDelete.length && hasLoadedAllTransactions; | ||
| const isSelectAllChecked = selectedItemsLength > 0 && selectedItemsLength === totalItems && hasLoadedAllTransactions; | ||
|
|
||
| const content = ( | ||
| <View style={[styles.flex1, !isKeyboardShown && safeAreaPaddingBottomStyle, containerStyle]}> | ||
|
|
@@ -503,11 +541,11 @@ function SearchList({ | |
| <Checkbox | ||
| accessibilityLabel={translate('workspace.people.selectAll')} | ||
| isChecked={isSelectAllChecked} | ||
| isIndeterminate={selectedItemsLength > 0 && (selectedItemsLength !== flattenedItemsWithoutPendingDelete.length || !hasLoadedAllTransactions)} | ||
| isIndeterminate={selectedItemsLength > 0 && (selectedItemsLength !== totalItems || !hasLoadedAllTransactions)} | ||
| onPress={() => { | ||
| onAllCheckboxPress(); | ||
| }} | ||
| disabled={flattenedItems.length === 0} | ||
| disabled={totalItems === 0} | ||
| /> | ||
| )} | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❌ PERF-13 (docs)
The
isTransactionGroupListItemArray(data)type guard is called inside both theemptyReportsandselectedItemsLengthuseMemoblocks. This type check does not depend on the iterator and produces the same result on every iteration.Suggested fix: Hoist the type check outside the reduce/filter operations:
Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.