From 4668b8a5db06c864c61bb3f0d543d039a50b69c3 Mon Sep 17 00:00:00 2001 From: Kevin Brian Bader Date: Fri, 13 Sep 2024 14:23:22 -0700 Subject: [PATCH 01/13] Search - Highlight newly added expense --- src/components/Search/index.tsx | 73 +++++++++++++++++-- src/components/SelectionList/BaseListItem.tsx | 11 +++ .../Search/TransactionListItem.tsx | 12 ++- src/components/SelectionList/types.ts | 4 + src/hooks/useAnimatedHighlightStyle/index.ts | 17 ++++- 5 files changed, 105 insertions(+), 12 deletions(-) diff --git a/src/components/Search/index.tsx b/src/components/Search/index.tsx index 4e23c0883f84..3a7e8fad255b 100644 --- a/src/components/Search/index.tsx +++ b/src/components/Search/index.tsx @@ -1,6 +1,7 @@ import {useNavigation} from '@react-navigation/native'; import type {StackNavigationProp} from '@react-navigation/stack'; -import React, {useCallback, useEffect, useRef, useState} from 'react'; +import React, {useCallback, useEffect, useMemo, useRef, useState} from 'react'; +import {InteractionManager} from 'react-native'; import type {OnyxEntry} from 'react-native-onyx'; import {useOnyx} from 'react-native-onyx'; import FullPageOfflineBlockingView from '@components/BlockingViews/FullPageOfflineBlockingView'; @@ -51,19 +52,26 @@ function mapTransactionItemToSelectedEntry(item: TransactionListItemType): [stri return [item.keyForList, {isSelected: true, canDelete: item.canDelete, canHold: item.canHold, canUnhold: item.canUnhold, action: item.action}]; } -function mapToTransactionItemWithSelectionInfo(item: TransactionListItemType, selectedTransactions: SelectedTransactions, canSelectMultiple: boolean) { - return {...item, isSelected: selectedTransactions[item.keyForList]?.isSelected && canSelectMultiple}; +function mapToTransactionItemWithSelectionInfo(item: TransactionListItemType, selectedTransactions: SelectedTransactions, canSelectMultiple: boolean, shouldAnimateInHighlight: boolean) { + return {...item, shouldAnimateInHighlight, isSelected: selectedTransactions[item.keyForList]?.isSelected && canSelectMultiple}; } -function mapToItemWithSelectionInfo(item: TransactionListItemType | ReportListItemType | ReportActionListItemType, selectedTransactions: SelectedTransactions, canSelectMultiple: boolean) { +function mapToItemWithSelectionInfo( + item: TransactionListItemType | ReportListItemType | ReportActionListItemType, + selectedTransactions: SelectedTransactions, + canSelectMultiple: boolean, + shouldAnimateInHighlight: boolean, +) { if (SearchUtils.isReportActionListItemType(item)) { return item; } + return SearchUtils.isTransactionListItemType(item) - ? mapToTransactionItemWithSelectionInfo(item, selectedTransactions, canSelectMultiple) + ? mapToTransactionItemWithSelectionInfo(item, selectedTransactions, canSelectMultiple, shouldAnimateInHighlight) : { ...item, - transactions: item.transactions?.map((transaction) => mapToTransactionItemWithSelectionInfo(transaction, selectedTransactions, canSelectMultiple)), + shouldAnimateInHighlight, + transactions: item.transactions?.map((transaction) => mapToTransactionItemWithSelectionInfo(transaction, selectedTransactions, canSelectMultiple, shouldAnimateInHighlight)), isSelected: item.transactions.every((transaction) => selectedTransactions[transaction.keyForList]?.isSelected && canSelectMultiple), }; } @@ -96,6 +104,8 @@ function Search({queryJSON}: SearchProps) { const {type, status, sortBy, sortOrder, hash} = queryJSON; const [currentSearchResults] = useOnyx(`${ONYXKEYS.COLLECTION.SNAPSHOT}${hash}`); + const [transactions] = useOnyx(ONYXKEYS.COLLECTION.TRANSACTION); + const previousTransactions = usePrevious(transactions); const canSelectMultiple = isSmallScreenWidth ? !!selectionMode?.isEnabled : true; @@ -123,9 +133,41 @@ function Search({queryJSON}: SearchProps) { } SearchActions.search({queryJSON, offset}); - // eslint-disable-next-line react-compiler/react-compiler, react-hooks/exhaustive-deps }, [isOffline, offset, queryJSON]); + const searchTriggeredRef = useRef(false); + + useEffect(() => { + const previousTransactionsLength = previousTransactions && Object.keys(previousTransactions).length; + const transactionsLength = transactions && Object.keys(transactions).length; + + if (!previousTransactionsLength || !transactionsLength || previousTransactionsLength === transactionsLength) { + return; + } + + // Check if the search block was already triggered + if (!searchTriggeredRef.current && transactionsLength > previousTransactionsLength) { + // A transaction was added, trigger the action again + SearchActions.search({queryJSON, offset}); + searchTriggeredRef.current = true; // Set the flag to true to prevent further triggers + } + }, [transactions, previousTransactions, queryJSON, offset]); + + // Reset the flag using InteractionManager after the search action + useEffect(() => { + if (!searchTriggeredRef.current) { + return; + } + + // Schedule a task to reset the flag after all current interactions have completed + const interactionHandle = InteractionManager.runAfterInteractions(() => { + searchTriggeredRef.current = false; // Reset the flag after interactions + }); + + // Cleanup the interaction handle if the component unmounts or effect reruns + return () => interactionHandle.cancel(); + }, [transactions]); + const handleOnCancelConfirmModal = () => { setDeleteExpensesConfirmModalVisible(false); }; @@ -182,6 +224,18 @@ function Search({queryJSON}: SearchProps) { } const searchResults = currentSearchResults?.data ? currentSearchResults : lastSearchResultsRef.current; + const previousSearchResults = usePrevious(searchResults?.data); + + const newSearchResultKey = useMemo(() => { + if (!previousSearchResults || !searchResults?.data) { + return null; + } + + const previousKeys = Object.keys(previousSearchResults); + const currentKeys = Object.keys(searchResults.data); + + return currentKeys.find((key) => !previousKeys.includes(key)); + }, [searchResults, previousSearchResults]); // There's a race condition in Onyx which makes it return data from the previous Search, so in addition to checking that the data is loaded // we also need to check that the searchResults matches the type and status of the current search @@ -229,7 +283,10 @@ function Search({queryJSON}: SearchProps) { const ListItem = SearchUtils.getListItem(type, status); const data = SearchUtils.getSections(type, status, searchResults.data, searchResults.search); const sortedData = SearchUtils.getSortedSections(type, status, data, sortBy, sortOrder); - const sortedSelectedData = sortedData.map((item) => mapToItemWithSelectionInfo(item, selectedTransactions, canSelectMultiple)); + const sortedSelectedData = sortedData.map((item) => { + const shouldAnimateInHighlight = `${ONYXKEYS.COLLECTION.TRANSACTION}${(item as TransactionListItemType).transactionID}` === newSearchResultKey; + return mapToItemWithSelectionInfo(item, selectedTransactions, canSelectMultiple, shouldAnimateInHighlight); + }); const shouldShowEmptyState = !isDataLoaded || data.length === 0; diff --git a/src/components/SelectionList/BaseListItem.tsx b/src/components/SelectionList/BaseListItem.tsx index a78944b50112..e8052b7dcedb 100644 --- a/src/components/SelectionList/BaseListItem.tsx +++ b/src/components/SelectionList/BaseListItem.tsx @@ -4,11 +4,13 @@ import Icon from '@components/Icon'; import * as Expensicons from '@components/Icon/Expensicons'; import OfflineWithFeedback from '@components/OfflineWithFeedback'; import PressableWithFeedback from '@components/Pressable/PressableWithFeedback'; +import useAnimatedHighlightStyle from '@hooks/useAnimatedHighlightStyle'; import useHover from '@hooks/useHover'; import {useMouseContext} from '@hooks/useMouseContext'; import useSyncFocus from '@hooks/useSyncFocus'; import useTheme from '@hooks/useTheme'; import useThemeStyles from '@hooks/useThemeStyles'; +import variables from '@styles/variables'; import CONST from '@src/CONST'; import type {BaseListItemProps, ListItem} from './types'; @@ -33,6 +35,7 @@ function BaseListItem({ onFocus = () => {}, hoverStyle, onLongPressRow, + shouldAnimateInHighlight = false, }: BaseListItemProps) { const theme = useTheme(); const styles = useThemeStyles(); @@ -60,6 +63,13 @@ function BaseListItem({ return rightHandSideComponent; }; + const animatedHighlightStyle = useAnimatedHighlightStyle({ + borderRadius: variables.componentBorderRadius, + shouldHighlight: item?.shouldAnimateInHighlight ?? false, + highlightColor: theme.messageHighlightBG, + backgroundColor: theme.highlightBG, + }); + return ( onDismissError(item)} @@ -98,6 +108,7 @@ function BaseListItem({ onFocus={onFocus} onMouseLeave={handleMouseLeave} tabIndex={item.tabIndex} + wrapperStyle={shouldAnimateInHighlight ? [styles.mh5, animatedHighlightStyle] : []} > {typeof children === 'function' ? children(hovered) : children} diff --git a/src/components/SelectionList/Search/TransactionListItem.tsx b/src/components/SelectionList/Search/TransactionListItem.tsx index 5043d6f8f562..9e8daf83929a 100644 --- a/src/components/SelectionList/Search/TransactionListItem.tsx +++ b/src/components/SelectionList/Search/TransactionListItem.tsx @@ -23,7 +23,16 @@ function TransactionListItem({ const {isLargeScreenWidth} = useResponsiveLayout(); - const listItemPressableStyle = [styles.selectionListPressableItemWrapper, styles.pv3, styles.ph3, item.isSelected && styles.activeComponentBG, isFocused && styles.sidebarLinkActive]; + const listItemPressableStyle = [ + styles.selectionListPressableItemWrapper, + styles.pv3, + styles.ph3, + item.isSelected && styles.activeComponentBG, + isFocused && styles.sidebarLinkActive, + // Removing some of the styles because they are added to the parent OpacityView via animatedHighlightStyle + {backgroundColor: 'unset'}, + styles.mh0, + ]; const listItemWrapperStyle = [ styles.flex1, @@ -50,6 +59,7 @@ function TransactionListItem({ onLongPressRow={onLongPressRow} shouldSyncFocus={shouldSyncFocus} hoverStyle={item.isSelected && styles.activeComponentBG} + shouldAnimateInHighlight > = CommonListItemProps & { children?: ReactElement> | ((hovered: boolean) => ReactElement>); shouldSyncFocus?: boolean; hoverStyle?: StyleProp; + shouldAnimateInHighlight: boolean; }; type UserListItemProps = ListItemProps & { diff --git a/src/hooks/useAnimatedHighlightStyle/index.ts b/src/hooks/useAnimatedHighlightStyle/index.ts index e474a9455d4d..951cc90d1682 100644 --- a/src/hooks/useAnimatedHighlightStyle/index.ts +++ b/src/hooks/useAnimatedHighlightStyle/index.ts @@ -10,7 +10,7 @@ type Props = { borderRadius: number; /** Height of the item that is to be faded */ - height: number; + height?: number; /** Delay before the highlighted item enters */ itemEnterDelay?: number; @@ -32,6 +32,15 @@ type Props = { /** Whether the item should be highlighted */ shouldHighlight: boolean; + + /** The base backgroundColor used for the highlight animation, defaults to theme.appBG + * @default theme.appBG + */ + backgroundColor?: string; + /** The base highlightColor used for the highlight animation, defaults to theme.border + * @default theme.border + */ + highlightColor?: string; }; /** @@ -47,6 +56,8 @@ export default function useAnimatedHighlightStyle({ highlightEndDelay = CONST.ANIMATED_HIGHLIGHT_END_DELAY, highlightEndDuration = CONST.ANIMATED_HIGHLIGHT_END_DURATION, height, + highlightColor, + backgroundColor, }: Props) { const [startHighlight, setStartHighlight] = useState(false); const repeatableProgress = useSharedValue(0); @@ -55,8 +66,8 @@ export default function useAnimatedHighlightStyle({ const theme = useTheme(); const highlightBackgroundStyle = useAnimatedStyle(() => ({ - backgroundColor: interpolateColor(repeatableProgress.value, [0, 1], [theme.appBG, theme.border]), - height: interpolate(nonRepeatableProgress.value, [0, 1], [0, height]), + backgroundColor: interpolateColor(repeatableProgress.value, [0, 1], [backgroundColor ?? theme.appBG, highlightColor ?? theme.border]), + height: height ? interpolate(nonRepeatableProgress.value, [0, 1], [0, height]) : 'auto', opacity: interpolate(nonRepeatableProgress.value, [0, 1], [0, 1]), borderRadius, })); From 14818c8008c2e84b8b4174db818952349685f9ee Mon Sep 17 00:00:00 2001 From: Kevin Brian Bader Date: Fri, 13 Sep 2024 16:34:59 -0700 Subject: [PATCH 02/13] fixed type issues --- src/components/SelectionList/BaseListItem.tsx | 4 ++-- src/components/SelectionList/Search/TransactionListItem.tsx | 2 +- src/components/SelectionList/types.ts | 3 ++- 3 files changed, 5 insertions(+), 4 deletions(-) diff --git a/src/components/SelectionList/BaseListItem.tsx b/src/components/SelectionList/BaseListItem.tsx index e8052b7dcedb..403ca921cf90 100644 --- a/src/components/SelectionList/BaseListItem.tsx +++ b/src/components/SelectionList/BaseListItem.tsx @@ -35,7 +35,7 @@ function BaseListItem({ onFocus = () => {}, hoverStyle, onLongPressRow, - shouldAnimateInHighlight = false, + hasAnimateInHighlightStyle = false, }: BaseListItemProps) { const theme = useTheme(); const styles = useThemeStyles(); @@ -108,7 +108,7 @@ function BaseListItem({ onFocus={onFocus} onMouseLeave={handleMouseLeave} tabIndex={item.tabIndex} - wrapperStyle={shouldAnimateInHighlight ? [styles.mh5, animatedHighlightStyle] : []} + wrapperStyle={hasAnimateInHighlightStyle ? [styles.mh5, animatedHighlightStyle] : []} > {typeof children === 'function' ? children(hovered) : children} diff --git a/src/components/SelectionList/Search/TransactionListItem.tsx b/src/components/SelectionList/Search/TransactionListItem.tsx index 9e8daf83929a..39172711516e 100644 --- a/src/components/SelectionList/Search/TransactionListItem.tsx +++ b/src/components/SelectionList/Search/TransactionListItem.tsx @@ -59,7 +59,7 @@ function TransactionListItem({ onLongPressRow={onLongPressRow} shouldSyncFocus={shouldSyncFocus} hoverStyle={item.isSelected && styles.activeComponentBG} - shouldAnimateInHighlight + hasAnimateInHighlightStyle > = CommonListItemProps & { children?: ReactElement> | ((hovered: boolean) => ReactElement>); shouldSyncFocus?: boolean; hoverStyle?: StyleProp; - shouldAnimateInHighlight: boolean; + /** Errors that this user may contain */ + hasAnimateInHighlightStyle?: boolean; }; type UserListItemProps = ListItemProps & { From b38ba48e3451388da21a654330d356ac530fd126 Mon Sep 17 00:00:00 2001 From: Kevin Brian Bader Date: Mon, 16 Sep 2024 13:27:14 -0700 Subject: [PATCH 03/13] adjusted logic for new account (empty Search transaction list) --- src/components/Search/index.tsx | 40 ++++++++++++++++----------------- 1 file changed, 19 insertions(+), 21 deletions(-) diff --git a/src/components/Search/index.tsx b/src/components/Search/index.tsx index 3a7e8fad255b..7247269e85b6 100644 --- a/src/components/Search/index.tsx +++ b/src/components/Search/index.tsx @@ -1,7 +1,6 @@ import {useNavigation} from '@react-navigation/native'; import type {StackNavigationProp} from '@react-navigation/stack'; import React, {useCallback, useEffect, useMemo, useRef, useState} from 'react'; -import {InteractionManager} from 'react-native'; import type {OnyxEntry} from 'react-native-onyx'; import {useOnyx} from 'react-native-onyx'; import FullPageOfflineBlockingView from '@components/BlockingViews/FullPageOfflineBlockingView'; @@ -141,32 +140,25 @@ function Search({queryJSON}: SearchProps) { const previousTransactionsLength = previousTransactions && Object.keys(previousTransactions).length; const transactionsLength = transactions && Object.keys(transactions).length; - if (!previousTransactionsLength || !transactionsLength || previousTransactionsLength === transactionsLength) { + // Return early if search block was already triggered or there's no change in transactions length + if (searchTriggeredRef.current || previousTransactionsLength === transactionsLength) { return; } - // Check if the search block was already triggered - if (!searchTriggeredRef.current && transactionsLength > previousTransactionsLength) { - // A transaction was added, trigger the action again + // Checking if length exists (we check if previousTransactionsLength is number because + // if we check for existance and initially it's 0 then the check will fail) + if (transactionsLength && typeof previousTransactionsLength === 'number' && transactionsLength > previousTransactionsLength) { + // A new transaction was added, trigger the action SearchActions.search({queryJSON, offset}); - searchTriggeredRef.current = true; // Set the flag to true to prevent further triggers - } - }, [transactions, previousTransactions, queryJSON, offset]); - - // Reset the flag using InteractionManager after the search action - useEffect(() => { - if (!searchTriggeredRef.current) { - return; + // Set the flag to true to prevent further triggers + searchTriggeredRef.current = true; } - // Schedule a task to reset the flag after all current interactions have completed - const interactionHandle = InteractionManager.runAfterInteractions(() => { - searchTriggeredRef.current = false; // Reset the flag after interactions - }); - - // Cleanup the interaction handle if the component unmounts or effect reruns - return () => interactionHandle.cancel(); - }, [transactions]); + // Reset the flag when transactions are updated + return () => { + searchTriggeredRef.current = false; + }; + }, [transactions, previousTransactions, queryJSON, offset]); const handleOnCancelConfirmModal = () => { setDeleteExpensesConfirmModalVisible(false); @@ -234,6 +226,12 @@ function Search({queryJSON}: SearchProps) { const previousKeys = Object.keys(previousSearchResults); const currentKeys = Object.keys(searchResults.data); + const isTransactionKey = (key: string) => key.startsWith(ONYXKEYS.COLLECTION.TRANSACTION); + // When previousKeys is empty (no transactions), find the first transaction key in currentKeys + if (previousKeys.length === 0) { + return currentKeys.find(isTransactionKey); + } + return currentKeys.find((key) => !previousKeys.includes(key)); }, [searchResults, previousSearchResults]); From 4595e26091f69718adbc2974acbc7d65a4bdf912 Mon Sep 17 00:00:00 2001 From: Kevin Brian Bader Date: Thu, 19 Sep 2024 14:33:12 -0700 Subject: [PATCH 04/13] added useScreenWrapperTransitionStatus mock to fix failing tests --- jest/setup.ts | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/jest/setup.ts b/jest/setup.ts index 51385ad19e45..ea5953be2d85 100644 --- a/jest/setup.ts +++ b/jest/setup.ts @@ -73,6 +73,13 @@ jest.mock('react-native-reanimated', () => ({ useReducedMotion: jest.fn, })); +// This mock is required as it's part of the `useAnimatedHighlightStyle` hook +jest.mock('@hooks/useScreenWrapperTransitionStatus', () => ({ + default: () => ({ + didScreenTransitionEnd: true, + }), +})); + jest.mock('react-native-keyboard-controller', () => require('react-native-keyboard-controller/jest')); jest.mock('@src/libs/actions/Timing', () => ({ From 24c9e8fa17a8ba735ac738f3044e227d4ddfde4d Mon Sep 17 00:00:00 2001 From: Kevin Brian Bader Date: Thu, 19 Sep 2024 15:11:11 -0700 Subject: [PATCH 05/13] added BootSplash mock and adjusted previously added mock --- jest/setup.ts | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/jest/setup.ts b/jest/setup.ts index ea5953be2d85..c9f18cb79398 100644 --- a/jest/setup.ts +++ b/jest/setup.ts @@ -75,9 +75,11 @@ jest.mock('react-native-reanimated', () => ({ // This mock is required as it's part of the `useAnimatedHighlightStyle` hook jest.mock('@hooks/useScreenWrapperTransitionStatus', () => ({ - default: () => ({ - didScreenTransitionEnd: true, - }), + didScreenTransitionEnd: true, +})); + +jest.mock('@libs/BootSplash', () => ({ + hide: jest.fn().mockResolvedValue(undefined), })); jest.mock('react-native-keyboard-controller', () => require('react-native-keyboard-controller/jest')); From aa9317b62f23989c5f1344b9c6f45b459d9a276d Mon Sep 17 00:00:00 2001 From: Kevin Brian Bader Date: Thu, 19 Sep 2024 16:28:37 -0700 Subject: [PATCH 06/13] targeted useScreenWrapperTransitionStatus mock to failing test files --- jest/setup.ts | 9 --------- tests/actions/OnyxUpdateManagerTest.ts | 6 ++++++ tests/actions/ReportTest.ts | 6 ++++++ tests/unit/OnyxUpdateManagerTest.ts | 6 ++++++ 4 files changed, 18 insertions(+), 9 deletions(-) diff --git a/jest/setup.ts b/jest/setup.ts index c9f18cb79398..51385ad19e45 100644 --- a/jest/setup.ts +++ b/jest/setup.ts @@ -73,15 +73,6 @@ jest.mock('react-native-reanimated', () => ({ useReducedMotion: jest.fn, })); -// This mock is required as it's part of the `useAnimatedHighlightStyle` hook -jest.mock('@hooks/useScreenWrapperTransitionStatus', () => ({ - didScreenTransitionEnd: true, -})); - -jest.mock('@libs/BootSplash', () => ({ - hide: jest.fn().mockResolvedValue(undefined), -})); - jest.mock('react-native-keyboard-controller', () => require('react-native-keyboard-controller/jest')); jest.mock('@src/libs/actions/Timing', () => ({ diff --git a/tests/actions/OnyxUpdateManagerTest.ts b/tests/actions/OnyxUpdateManagerTest.ts index 3a4ff0779217..e1008064163b 100644 --- a/tests/actions/OnyxUpdateManagerTest.ts +++ b/tests/actions/OnyxUpdateManagerTest.ts @@ -27,6 +27,12 @@ jest.mock('@libs/actions/OnyxUpdateManager/utils/applyUpdates', () => { }; }); +jest.mock('@hooks/useScreenWrapperTransitionStatus', () => ({ + default: () => ({ + didScreenTransitionEnd: true, + }), +})); + const App = AppImport as AppActionsMock; const ApplyUpdates = ApplyUpdatesImport as ApplyUpdatesMock; const OnyxUpdateManagerUtils = OnyxUpdateManagerUtilsImport as OnyxUpdateManagerUtilsMock; diff --git a/tests/actions/ReportTest.ts b/tests/actions/ReportTest.ts index a099348257f0..03e844332c58 100644 --- a/tests/actions/ReportTest.ts +++ b/tests/actions/ReportTest.ts @@ -30,6 +30,12 @@ jest.mock('@src/libs/actions/Report', () => { }; }); +jest.mock('@hooks/useScreenWrapperTransitionStatus', () => ({ + default: () => ({ + didScreenTransitionEnd: true, + }), +})); + OnyxUpdateManager(); describe('actions/Report', () => { beforeAll(() => { diff --git a/tests/unit/OnyxUpdateManagerTest.ts b/tests/unit/OnyxUpdateManagerTest.ts index 2cfd1eec10c7..7b0446641458 100644 --- a/tests/unit/OnyxUpdateManagerTest.ts +++ b/tests/unit/OnyxUpdateManagerTest.ts @@ -13,6 +13,12 @@ jest.mock('@libs/actions/App'); jest.mock('@libs/actions/OnyxUpdateManager/utils'); jest.mock('@libs/actions/OnyxUpdateManager/utils/applyUpdates'); +jest.mock('@hooks/useScreenWrapperTransitionStatus', () => ({ + default: () => ({ + didScreenTransitionEnd: true, + }), +})); + const App = AppImport as AppActionsMock; const ApplyUpdates = ApplyUpdatesImport as ApplyUpdatesMock; const OnyxUpdateManagerUtils = OnyxUpdateManagerUtilsImport as OnyxUpdateManagerUtilsMock; From babb58ba496e3121f3a3f2f6ef8fc15143679ca0 Mon Sep 17 00:00:00 2001 From: Kevin Brian Bader Date: Thu, 19 Sep 2024 18:32:07 -0700 Subject: [PATCH 07/13] mocked useScreenWrapperTransitionStatus in failing perf test --- tests/perf-test/SelectionList.perf-test.tsx | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/tests/perf-test/SelectionList.perf-test.tsx b/tests/perf-test/SelectionList.perf-test.tsx index 07a9a4d6ce22..835c296f46e8 100644 --- a/tests/perf-test/SelectionList.perf-test.tsx +++ b/tests/perf-test/SelectionList.perf-test.tsx @@ -76,6 +76,14 @@ jest.mock('../../src/hooks/useKeyboardState', () => ({ })), })); +jest.mock('../../src/hooks/useScreenWrapperTransitionStatus', () => ({ + // eslint-disable-next-line @typescript-eslint/naming-convention + __esModule: true, // Required in order to ensures ES6 module compatibility + default: jest.fn(() => ({ + didScreenTransitionEnd: true, + })), +})); + function SelectionListWrapper({canSelectMultiple}: SelectionListWrapperProps) { const [selectedIds, setSelectedIds] = useState([]); From 72fd61228863d59c5d8475b69a85d554f5a15f03 Mon Sep 17 00:00:00 2001 From: Kevin Brian Bader Date: Thu, 19 Sep 2024 20:35:01 -0700 Subject: [PATCH 08/13] lint fix --- tests/perf-test/SelectionList.perf-test.tsx | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/tests/perf-test/SelectionList.perf-test.tsx b/tests/perf-test/SelectionList.perf-test.tsx index 835c296f46e8..7552f9347862 100644 --- a/tests/perf-test/SelectionList.perf-test.tsx +++ b/tests/perf-test/SelectionList.perf-test.tsx @@ -2,7 +2,7 @@ import {fireEvent} from '@testing-library/react-native'; import type {RenderResult} from '@testing-library/react-native'; import React, {useState} from 'react'; import type {ComponentType} from 'react'; -import {measurePerformance} from 'reassure'; +import {measureRenders} from 'reassure'; import SelectionList from '@components/SelectionList'; import RadioListItem from '@components/SelectionList/RadioListItem'; import type {ListItem} from '@components/SelectionList/types'; @@ -78,7 +78,7 @@ jest.mock('../../src/hooks/useKeyboardState', () => ({ jest.mock('../../src/hooks/useScreenWrapperTransitionStatus', () => ({ // eslint-disable-next-line @typescript-eslint/naming-convention - __esModule: true, // Required in order to ensures ES6 module compatibility + __esModule: true, default: jest.fn(() => ({ didScreenTransitionEnd: true, })), @@ -127,7 +127,7 @@ function SelectionListWrapper({canSelectMultiple}: SelectionListWrapperProps) { } test('[SelectionList] should render 1 section and a thousand items', () => { - measurePerformance(); + measureRenders(); }); test('[SelectionList] should press a list item', () => { @@ -136,7 +136,7 @@ test('[SelectionList] should press a list item', () => { fireEvent.press(screen.getByText('Item 5')); }; - measurePerformance(, {scenario}); + measureRenders(, {scenario}); }); test('[SelectionList] should render multiple selection and select 3 items', () => { @@ -147,7 +147,7 @@ test('[SelectionList] should render multiple selection and select 3 items', () = fireEvent.press(screen.getByText('Item 3')); }; - measurePerformance(, {scenario}); + measureRenders(, {scenario}); }); test('[SelectionList] should scroll and select a few items', () => { @@ -179,5 +179,5 @@ test('[SelectionList] should scroll and select a few items', () => { fireEvent.press(screen.getByText('Item 15')); }; - measurePerformance(, {scenario}); + measureRenders(, {scenario}); }); From 9e43cbcba42219c0fbcc2896f0f40e47163d2ed5 Mon Sep 17 00:00:00 2001 From: Kevin Brian Bader Date: Mon, 23 Sep 2024 18:03:57 -0700 Subject: [PATCH 09/13] scroll to newly added expense functionality --- src/components/Search/index.tsx | 18 +++++++++++++++++- .../SelectionList/BaseSelectionList.tsx | 2 +- src/components/SelectionList/types.ts | 1 + 3 files changed, 19 insertions(+), 2 deletions(-) diff --git a/src/components/Search/index.tsx b/src/components/Search/index.tsx index 7247269e85b6..d3b3c72604a8 100644 --- a/src/components/Search/index.tsx +++ b/src/components/Search/index.tsx @@ -7,7 +7,7 @@ import FullPageOfflineBlockingView from '@components/BlockingViews/FullPageOffli import ConfirmModal from '@components/ConfirmModal'; import DecisionModal from '@components/DecisionModal'; import SearchTableHeader from '@components/SelectionList/SearchTableHeader'; -import type {ReportActionListItemType, ReportListItemType, TransactionListItemType} from '@components/SelectionList/types'; +import type {ReportActionListItemType, ReportListItemType, SelectionListHandle, TransactionListItemType} from '@components/SelectionList/types'; import SelectionListWithModal from '@components/SelectionListWithModal'; import SearchRowSkeleton from '@components/Skeletons/SearchRowSkeleton'; import SearchStatusSkeleton from '@components/Skeletons/SearchStatusSkeleton'; @@ -235,6 +235,21 @@ function Search({queryJSON}: SearchProps) { return currentKeys.find((key) => !previousKeys.includes(key)); }, [searchResults, previousSearchResults]); + const handleSelectionListScroll = useCallback( + (data: Array) => (ref: SelectionListHandle | null) => { + const indexOfNewTransaction = data?.findIndex( + (transaction) => `${ONYXKEYS.COLLECTION.TRANSACTION}${(transaction as TransactionListItemType)?.transactionID}` === newSearchResultKey, + ); + + if (!ref || indexOfNewTransaction < 0) { + return; + } + + ref?.scrollToIndex(indexOfNewTransaction); + }, + [newSearchResultKey], + ); + // There's a race condition in Onyx which makes it return data from the previous Search, so in addition to checking that the data is loaded // we also need to check that the searchResults matches the type and status of the current search const isDataLoaded = searchResults?.data !== undefined && searchResults?.search?.type === type && searchResults?.search?.status === status; @@ -408,6 +423,7 @@ function Search({queryJSON}: SearchProps) { resetOffset={resetOffset} /> + ref={handleSelectionListScroll(sortedSelectedData)} sections={[{data: sortedSelectedData, isDisabled: false}]} turnOnSelectionModeOnLongPress={type !== CONST.SEARCH.DATA_TYPES.CHAT} onTurnOnSelectionMode={(item) => item && toggleTransaction(item)} diff --git a/src/components/SelectionList/BaseSelectionList.tsx b/src/components/SelectionList/BaseSelectionList.tsx index 9ee264bc9bf9..f45cbf5bd5e2 100644 --- a/src/components/SelectionList/BaseSelectionList.tsx +++ b/src/components/SelectionList/BaseSelectionList.tsx @@ -616,7 +616,7 @@ function BaseSelectionList( [flattenedSections.allOptions, setFocusedIndex, updateAndScrollToFocusedIndex], ); - useImperativeHandle(ref, () => ({scrollAndHighlightItem, clearInputAfterSelect}), [scrollAndHighlightItem, clearInputAfterSelect]); + useImperativeHandle(ref, () => ({scrollAndHighlightItem, clearInputAfterSelect, scrollToIndex}), [scrollAndHighlightItem, clearInputAfterSelect, scrollToIndex]); /** Selects row when pressing Enter */ useKeyboardShortcut(CONST.KEYBOARD_SHORTCUTS.ENTER, selectFocusedOption, { diff --git a/src/components/SelectionList/types.ts b/src/components/SelectionList/types.ts index 12c68a988d70..27590a4522aa 100644 --- a/src/components/SelectionList/types.ts +++ b/src/components/SelectionList/types.ts @@ -549,6 +549,7 @@ type BaseSelectionListProps = Partial & { type SelectionListHandle = { scrollAndHighlightItem?: (items: string[], timeout: number) => void; clearInputAfterSelect?: () => void; + scrollToIndex: (index: number, animated?: boolean) => void; }; type ItemLayout = { From 0f1bb6074556eee41ca696dc87c08847a96fc0d3 Mon Sep 17 00:00:00 2001 From: Kevin Brian Bader Date: Wed, 25 Sep 2024 15:55:00 -0700 Subject: [PATCH 10/13] hooks refactoring, fixed highlight animation issues --- src/components/Search/index.tsx | 59 +++--------- .../SelectionList/Search/ReportListItem.tsx | 4 + src/hooks/useNewSearchResultKey.ts | 94 +++++++++++++++++++ .../useTriggerSearchOnTransactionIncrease.ts | 51 ++++++++++ 4 files changed, 164 insertions(+), 44 deletions(-) create mode 100644 src/hooks/useNewSearchResultKey.ts create mode 100644 src/hooks/useTriggerSearchOnTransactionIncrease.ts diff --git a/src/components/Search/index.tsx b/src/components/Search/index.tsx index d3b3c72604a8..673cad929366 100644 --- a/src/components/Search/index.tsx +++ b/src/components/Search/index.tsx @@ -36,6 +36,8 @@ import {useSearchContext} from './SearchContext'; import SearchPageHeader from './SearchPageHeader'; import SearchStatusBar from './SearchStatusBar'; import type {SearchColumnType, SearchQueryJSON, SearchStatus, SelectedTransactionInfo, SelectedTransactions, SortOrder} from './types'; +import useTriggerSearchOnNewTransaction from '@hooks/useTriggerSearchOnTransactionIncrease'; +import useNewSearchResultKey from '@hooks/useNewSearchResultKey'; type SearchProps = { queryJSON: SearchQueryJSON; @@ -134,31 +136,7 @@ function Search({queryJSON}: SearchProps) { SearchActions.search({queryJSON, offset}); }, [isOffline, offset, queryJSON]); - const searchTriggeredRef = useRef(false); - - useEffect(() => { - const previousTransactionsLength = previousTransactions && Object.keys(previousTransactions).length; - const transactionsLength = transactions && Object.keys(transactions).length; - - // Return early if search block was already triggered or there's no change in transactions length - if (searchTriggeredRef.current || previousTransactionsLength === transactionsLength) { - return; - } - - // Checking if length exists (we check if previousTransactionsLength is number because - // if we check for existance and initially it's 0 then the check will fail) - if (transactionsLength && typeof previousTransactionsLength === 'number' && transactionsLength > previousTransactionsLength) { - // A new transaction was added, trigger the action - SearchActions.search({queryJSON, offset}); - // Set the flag to true to prevent further triggers - searchTriggeredRef.current = true; - } - - // Reset the flag when transactions are updated - return () => { - searchTriggeredRef.current = false; - }; - }, [transactions, previousTransactions, queryJSON, offset]); + useTriggerSearchOnNewTransaction({transactions, previousTransactions, queryJSON, offset}); const handleOnCancelConfirmModal = () => { setDeleteExpensesConfirmModalVisible(false); @@ -216,24 +194,7 @@ function Search({queryJSON}: SearchProps) { } const searchResults = currentSearchResults?.data ? currentSearchResults : lastSearchResultsRef.current; - const previousSearchResults = usePrevious(searchResults?.data); - - const newSearchResultKey = useMemo(() => { - if (!previousSearchResults || !searchResults?.data) { - return null; - } - - const previousKeys = Object.keys(previousSearchResults); - const currentKeys = Object.keys(searchResults.data); - - const isTransactionKey = (key: string) => key.startsWith(ONYXKEYS.COLLECTION.TRANSACTION); - // When previousKeys is empty (no transactions), find the first transaction key in currentKeys - if (previousKeys.length === 0) { - return currentKeys.find(isTransactionKey); - } - - return currentKeys.find((key) => !previousKeys.includes(key)); - }, [searchResults, previousSearchResults]); + const newSearchResultKey = useNewSearchResultKey(searchResults); const handleSelectionListScroll = useCallback( (data: Array) => (ref: SelectionListHandle | null) => { @@ -297,7 +258,17 @@ function Search({queryJSON}: SearchProps) { const data = SearchUtils.getSections(type, status, searchResults.data, searchResults.search); const sortedData = SearchUtils.getSortedSections(type, status, data, sortBy, sortOrder); const sortedSelectedData = sortedData.map((item) => { - const shouldAnimateInHighlight = `${ONYXKEYS.COLLECTION.TRANSACTION}${(item as TransactionListItemType).transactionID}` === newSearchResultKey; + const baseKey = `${ONYXKEYS.COLLECTION.TRANSACTION}${(item as TransactionListItemType).transactionID}`; + // Check if the base key matches the newSearchResultKey (TransactionListItemType) + const isBaseKeyMatch = baseKey === newSearchResultKey; + // Check if any transaction within the transactions array (ReportListItemType) matches the newSearchResultKey + const isAnyTransactionMatch = (item as ReportListItemType)?.transactions?.some((transaction) => { + const transactionKey = `${ONYXKEYS.COLLECTION.TRANSACTION}${transaction.transactionID}`; + return transactionKey === newSearchResultKey; + }); + // Determine if either the base key or any transaction key matches + const shouldAnimateInHighlight = isBaseKeyMatch || isAnyTransactionMatch; + return mapToItemWithSelectionInfo(item, selectedTransactions, canSelectMultiple, shouldAnimateInHighlight); }); diff --git a/src/components/SelectionList/Search/ReportListItem.tsx b/src/components/SelectionList/Search/ReportListItem.tsx index a0b96547bcd8..fb5c30c1946d 100644 --- a/src/components/SelectionList/Search/ReportListItem.tsx +++ b/src/components/SelectionList/Search/ReportListItem.tsx @@ -77,6 +77,9 @@ function ReportListItem({ styles.overflowHidden, item.isSelected && styles.activeComponentBG, isFocused && styles.sidebarLinkActive, + // Removing some of the styles because they are added to the parent OpacityView via animatedHighlightStyle + {backgroundColor: 'unset'}, + styles.mh0, ]; const handleOnButtonPress = () => { @@ -138,6 +141,7 @@ function ReportListItem({ onFocus={onFocus} shouldSyncFocus={shouldSyncFocus} hoverStyle={item.isSelected && styles.activeComponentBG} + hasAnimateInHighlightStyle > {!isLargeScreenWidth && ( diff --git a/src/hooks/useNewSearchResultKey.ts b/src/hooks/useNewSearchResultKey.ts new file mode 100644 index 000000000000..fc442744557c --- /dev/null +++ b/src/hooks/useNewSearchResultKey.ts @@ -0,0 +1,94 @@ +import { useEffect, useMemo, useRef, useState } from 'react'; +import usePrevious from './usePrevious'; // Assuming you have this hook +import ONYXKEYS from '@src/ONYXKEYS'; +import { SearchResults } from '@src/types/onyx'; +import { OnyxEntry } from 'react-native-onyx'; +import CONST from '@src/CONST'; +import { ReportListItemType, TransactionListItemType } from '@components/SelectionList/types'; + +/** + * Hook used to get the Search results' new transaction key for animation purposes. + */ +function useNewSearchResultKey(searchResults: OnyxEntry) { + // Get the previous search results + const previousSearchResults = usePrevious(searchResults?.data); + + // State to store the new search result key + const [newSearchResultKey, setNewSearchResultKey] = useState(null); + + // Ref to keep track of transaction IDs that have been highlighted + const highlightedTransactionIDs = useRef>(new Set()); + + // Ref to ensure initialization happens only once + const initializedRef = useRef(false); + + // Initialize the set with existing transaction IDs only once + useEffect(() => { + if (!initializedRef.current && searchResults?.data) { + const existingTransactionIDs = extractTransactionIDsFromSearchResults(searchResults.data); + highlightedTransactionIDs.current = new Set(existingTransactionIDs); + initializedRef.current = true; + } + }, [searchResults?.data]); + + useEffect(() => { + if (!previousSearchResults || !searchResults?.data) { + return; + } + + const previousTransactionIDs = extractTransactionIDsFromSearchResults(previousSearchResults); + const currentTransactionIDs = extractTransactionIDsFromSearchResults(searchResults.data); + + // Find new transaction IDs that are not in the previousTransactionIDs and not already highlighted + const newTransactionIDs = currentTransactionIDs.filter( + (id) => + !previousTransactionIDs.includes(id) && !highlightedTransactionIDs.current.has(id) + ); + + if (newTransactionIDs.length > 0) { + const newTransactionID = newTransactionIDs[0]; + const newTransactionKey = `${ONYXKEYS.COLLECTION.TRANSACTION}${newTransactionID}`; + + setNewSearchResultKey(newTransactionKey); + highlightedTransactionIDs.current.add(newTransactionID); + } + }, [searchResults, previousSearchResults]); + + // Reset newSearchResultKey after it's been used + useEffect(() => { + if (newSearchResultKey !== null) { + // Reset after a delay if needed (e.g. match animation highlight duration) + const timer = setTimeout(() => { + setNewSearchResultKey(null); + }, CONST.ANIMATED_HIGHLIGHT_START_DURATION); + + return () => clearTimeout(timer); + } + }, [newSearchResultKey]); + + return newSearchResultKey; +} + +function extractTransactionIDsFromSearchResults(searchResultsData: SearchResults['data']): string[] { + const transactionIDs: string[] = []; + + Object.values(searchResultsData).forEach((item) => { + // Check for transactionID directly on the item (TransactionListItemType) + if ((item as TransactionListItemType)?.transactionID) { + transactionIDs.push((item as TransactionListItemType)?.transactionID); + } + + // Check for transactions array within the item (ReportListItemType) + if (Array.isArray((item as ReportListItemType)?.transactions)) { + (item as ReportListItemType)?.transactions?.forEach((transaction) => { + if (transaction?.transactionID) { + transactionIDs.push(transaction?.transactionID); + } + }); + } + }); + + return transactionIDs; +} + +export default useNewSearchResultKey; diff --git a/src/hooks/useTriggerSearchOnTransactionIncrease.ts b/src/hooks/useTriggerSearchOnTransactionIncrease.ts new file mode 100644 index 000000000000..c582f70567d6 --- /dev/null +++ b/src/hooks/useTriggerSearchOnTransactionIncrease.ts @@ -0,0 +1,51 @@ +import { useEffect, useRef } from 'react'; +import * as SearchActions from '@libs/actions/Search'; +import { OnyxCollection } from 'react-native-onyx'; +import { Transaction } from '@src/types/onyx'; +import { SearchQueryJSON } from '@components/Search/types'; + +type UseTriggerSearchOnNewTransaction = { + transactions: OnyxCollection; + previousTransactions: OnyxCollection; + queryJSON: SearchQueryJSON; + offset: number; +} + +/** + * Hook used to trigger a new Search API call when a new transaction is created. + */ +function useTriggerSearchOnNewTransaction({ + transactions, + previousTransactions, + queryJSON, + offset, +}: UseTriggerSearchOnNewTransaction) { + const searchTriggeredRef = useRef(false); + + useEffect(() => { + const previousTransactionsLength = previousTransactions && Object.keys(previousTransactions).length; + const transactionsLength = transactions && Object.keys(transactions).length; + + // Return early if search was already triggered or there's no change in transactions length + if (searchTriggeredRef.current || previousTransactionsLength === transactionsLength) { + return; + } + + // Checking if length exists (we check if previousTransactionsLength is number because + // if we check for existance and initially it's 0 then the check will fail) + if (transactionsLength && typeof previousTransactionsLength === 'number' && transactionsLength > previousTransactionsLength) { + // A new transaction was added, trigger the action + console.log('queryJSON, offset', queryJSON, offset); + SearchActions.search({ queryJSON, offset }); + // Set the flag to true to prevent further triggers + searchTriggeredRef.current = true; + } + + // Reset the flag when transactions are updated + return () => { + searchTriggeredRef.current = false; + }; + }, [transactions, previousTransactions, queryJSON, offset]); +} + +export default useTriggerSearchOnNewTransaction; From cc892d835edf53c14571ad3a88358ffd1c1245cc Mon Sep 17 00:00:00 2001 From: Kevin Brian Bader Date: Wed, 25 Sep 2024 18:22:27 -0700 Subject: [PATCH 11/13] fixed lint and prettier --- src/components/Search/index.tsx | 8 ++-- src/hooks/useNewSearchResultKey.ts | 46 +++++++++---------- .../useTriggerSearchOnTransactionIncrease.ts | 20 +++----- 3 files changed, 34 insertions(+), 40 deletions(-) diff --git a/src/components/Search/index.tsx b/src/components/Search/index.tsx index eb284ddcb09f..9264ac2c7d2a 100644 --- a/src/components/Search/index.tsx +++ b/src/components/Search/index.tsx @@ -1,6 +1,6 @@ import {useNavigation} from '@react-navigation/native'; import type {StackNavigationProp} from '@react-navigation/stack'; -import React, {useCallback, useEffect, useMemo, useRef, useState} from 'react'; +import React, {useCallback, useEffect, useRef, useState} from 'react'; import type {OnyxEntry} from 'react-native-onyx'; import {useOnyx} from 'react-native-onyx'; import FullPageOfflineBlockingView from '@components/BlockingViews/FullPageOfflineBlockingView'; @@ -14,9 +14,11 @@ import SearchStatusSkeleton from '@components/Skeletons/SearchStatusSkeleton'; import useLocalize from '@hooks/useLocalize'; import useMobileSelectionMode from '@hooks/useMobileSelectionMode'; import useNetwork from '@hooks/useNetwork'; +import useNewSearchResultKey from '@hooks/useNewSearchResultKey'; import usePrevious from '@hooks/usePrevious'; import useResponsiveLayout from '@hooks/useResponsiveLayout'; import useThemeStyles from '@hooks/useThemeStyles'; +import useTriggerSearchOnNewTransaction from '@hooks/useTriggerSearchOnTransactionIncrease'; import {turnOffMobileSelectionMode, turnOnMobileSelectionMode} from '@libs/actions/MobileSelectionMode'; import * as SearchActions from '@libs/actions/Search'; import * as DeviceCapabilities from '@libs/DeviceCapabilities'; @@ -36,8 +38,6 @@ import {useSearchContext} from './SearchContext'; import SearchPageHeader from './SearchPageHeader'; import SearchStatusBar from './SearchStatusBar'; import type {SearchColumnType, SearchQueryJSON, SearchStatus, SelectedTransactionInfo, SelectedTransactions, SortOrder} from './types'; -import useTriggerSearchOnNewTransaction from '@hooks/useTriggerSearchOnTransactionIncrease'; -import useNewSearchResultKey from '@hooks/useNewSearchResultKey'; type SearchProps = { queryJSON: SearchQueryJSON; @@ -268,7 +268,7 @@ function Search({queryJSON}: SearchProps) { }); // Determine if either the base key or any transaction key matches const shouldAnimateInHighlight = isBaseKeyMatch || isAnyTransactionMatch; - + return mapToItemWithSelectionInfo(item, selectedTransactions, canSelectMultiple, shouldAnimateInHighlight); }); diff --git a/src/hooks/useNewSearchResultKey.ts b/src/hooks/useNewSearchResultKey.ts index fc442744557c..f65f41654bda 100644 --- a/src/hooks/useNewSearchResultKey.ts +++ b/src/hooks/useNewSearchResultKey.ts @@ -1,10 +1,10 @@ -import { useEffect, useMemo, useRef, useState } from 'react'; -import usePrevious from './usePrevious'; // Assuming you have this hook -import ONYXKEYS from '@src/ONYXKEYS'; -import { SearchResults } from '@src/types/onyx'; -import { OnyxEntry } from 'react-native-onyx'; +import {useEffect, useRef, useState} from 'react'; +import type {OnyxEntry} from 'react-native-onyx'; +import type {ReportListItemType, TransactionListItemType} from '@components/SelectionList/types'; import CONST from '@src/CONST'; -import { ReportListItemType, TransactionListItemType } from '@components/SelectionList/types'; +import ONYXKEYS from '@src/ONYXKEYS'; +import type {SearchResults} from '@src/types/onyx'; +import usePrevious from './usePrevious'; /** * Hook used to get the Search results' new transaction key for animation purposes. @@ -24,11 +24,12 @@ function useNewSearchResultKey(searchResults: OnyxEntry) { // Initialize the set with existing transaction IDs only once useEffect(() => { - if (!initializedRef.current && searchResults?.data) { - const existingTransactionIDs = extractTransactionIDsFromSearchResults(searchResults.data); - highlightedTransactionIDs.current = new Set(existingTransactionIDs); - initializedRef.current = true; + if (initializedRef.current || !searchResults?.data) { + return; } + const existingTransactionIDs = extractTransactionIDsFromSearchResults(searchResults.data); + highlightedTransactionIDs.current = new Set(existingTransactionIDs); + initializedRef.current = true; }, [searchResults?.data]); useEffect(() => { @@ -40,10 +41,7 @@ function useNewSearchResultKey(searchResults: OnyxEntry) { const currentTransactionIDs = extractTransactionIDsFromSearchResults(searchResults.data); // Find new transaction IDs that are not in the previousTransactionIDs and not already highlighted - const newTransactionIDs = currentTransactionIDs.filter( - (id) => - !previousTransactionIDs.includes(id) && !highlightedTransactionIDs.current.has(id) - ); + const newTransactionIDs = currentTransactionIDs.filter((id) => !previousTransactionIDs.includes(id) && !highlightedTransactionIDs.current.has(id)); if (newTransactionIDs.length > 0) { const newTransactionID = newTransactionIDs[0]; @@ -56,14 +54,15 @@ function useNewSearchResultKey(searchResults: OnyxEntry) { // Reset newSearchResultKey after it's been used useEffect(() => { - if (newSearchResultKey !== null) { - // Reset after a delay if needed (e.g. match animation highlight duration) - const timer = setTimeout(() => { - setNewSearchResultKey(null); - }, CONST.ANIMATED_HIGHLIGHT_START_DURATION); - - return () => clearTimeout(timer); + if (newSearchResultKey === null) { + return; } + // Reset after a delay if needed (e.g. match animation highlight duration) + const timer = setTimeout(() => { + setNewSearchResultKey(null); + }, CONST.ANIMATED_HIGHLIGHT_START_DURATION); + + return () => clearTimeout(timer); }, [newSearchResultKey]); return newSearchResultKey; @@ -81,9 +80,10 @@ function extractTransactionIDsFromSearchResults(searchResultsData: SearchResults // Check for transactions array within the item (ReportListItemType) if (Array.isArray((item as ReportListItemType)?.transactions)) { (item as ReportListItemType)?.transactions?.forEach((transaction) => { - if (transaction?.transactionID) { - transactionIDs.push(transaction?.transactionID); + if (!transaction?.transactionID) { + return; } + transactionIDs.push(transaction?.transactionID); }); } }); diff --git a/src/hooks/useTriggerSearchOnTransactionIncrease.ts b/src/hooks/useTriggerSearchOnTransactionIncrease.ts index c582f70567d6..0e5b114bb6ca 100644 --- a/src/hooks/useTriggerSearchOnTransactionIncrease.ts +++ b/src/hooks/useTriggerSearchOnTransactionIncrease.ts @@ -1,25 +1,20 @@ -import { useEffect, useRef } from 'react'; +import {useEffect, useRef} from 'react'; +import type {OnyxCollection} from 'react-native-onyx'; +import type {SearchQueryJSON} from '@components/Search/types'; import * as SearchActions from '@libs/actions/Search'; -import { OnyxCollection } from 'react-native-onyx'; -import { Transaction } from '@src/types/onyx'; -import { SearchQueryJSON } from '@components/Search/types'; +import type {Transaction} from '@src/types/onyx'; type UseTriggerSearchOnNewTransaction = { transactions: OnyxCollection; previousTransactions: OnyxCollection; queryJSON: SearchQueryJSON; offset: number; -} +}; /** * Hook used to trigger a new Search API call when a new transaction is created. */ -function useTriggerSearchOnNewTransaction({ - transactions, - previousTransactions, - queryJSON, - offset, -}: UseTriggerSearchOnNewTransaction) { +function useTriggerSearchOnNewTransaction({transactions, previousTransactions, queryJSON, offset}: UseTriggerSearchOnNewTransaction) { const searchTriggeredRef = useRef(false); useEffect(() => { @@ -35,8 +30,7 @@ function useTriggerSearchOnNewTransaction({ // if we check for existance and initially it's 0 then the check will fail) if (transactionsLength && typeof previousTransactionsLength === 'number' && transactionsLength > previousTransactionsLength) { // A new transaction was added, trigger the action - console.log('queryJSON, offset', queryJSON, offset); - SearchActions.search({ queryJSON, offset }); + SearchActions.search({queryJSON, offset}); // Set the flag to true to prevent further triggers searchTriggeredRef.current = true; } From 90ddc4d89b3e3d691a7b6e112518b0feed74a060 Mon Sep 17 00:00:00 2001 From: Kevin Brian Bader Date: Fri, 27 Sep 2024 12:59:42 -0700 Subject: [PATCH 12/13] refactoring and bugfixing --- src/components/Search/index.tsx | 29 +-- src/hooks/useNewSearchResultKey.ts | 94 ---------- src/hooks/useSearchHighlightAndScroll.ts | 177 ++++++++++++++++++ .../useTriggerSearchOnTransactionIncrease.ts | 45 ----- 4 files changed, 186 insertions(+), 159 deletions(-) delete mode 100644 src/hooks/useNewSearchResultKey.ts create mode 100644 src/hooks/useSearchHighlightAndScroll.ts delete mode 100644 src/hooks/useTriggerSearchOnTransactionIncrease.ts diff --git a/src/components/Search/index.tsx b/src/components/Search/index.tsx index 9264ac2c7d2a..49d4c8be27c1 100644 --- a/src/components/Search/index.tsx +++ b/src/components/Search/index.tsx @@ -7,18 +7,17 @@ import FullPageOfflineBlockingView from '@components/BlockingViews/FullPageOffli import ConfirmModal from '@components/ConfirmModal'; import DecisionModal from '@components/DecisionModal'; import SearchTableHeader from '@components/SelectionList/SearchTableHeader'; -import type {ReportActionListItemType, ReportListItemType, SelectionListHandle, TransactionListItemType} from '@components/SelectionList/types'; +import type {ReportActionListItemType, ReportListItemType, TransactionListItemType} from '@components/SelectionList/types'; import SelectionListWithModal from '@components/SelectionListWithModal'; import SearchRowSkeleton from '@components/Skeletons/SearchRowSkeleton'; import SearchStatusSkeleton from '@components/Skeletons/SearchStatusSkeleton'; import useLocalize from '@hooks/useLocalize'; import useMobileSelectionMode from '@hooks/useMobileSelectionMode'; import useNetwork from '@hooks/useNetwork'; -import useNewSearchResultKey from '@hooks/useNewSearchResultKey'; import usePrevious from '@hooks/usePrevious'; import useResponsiveLayout from '@hooks/useResponsiveLayout'; +import useSearchHighlightAndScroll from '@hooks/useSearchHighlightAndScroll'; import useThemeStyles from '@hooks/useThemeStyles'; -import useTriggerSearchOnNewTransaction from '@hooks/useTriggerSearchOnTransactionIncrease'; import {turnOffMobileSelectionMode, turnOnMobileSelectionMode} from '@libs/actions/MobileSelectionMode'; import * as SearchActions from '@libs/actions/Search'; import * as DeviceCapabilities from '@libs/DeviceCapabilities'; @@ -136,8 +135,6 @@ function Search({queryJSON}: SearchProps) { SearchActions.search({queryJSON, offset}); }, [isOffline, offset, queryJSON]); - useTriggerSearchOnNewTransaction({transactions, previousTransactions, queryJSON, offset}); - const handleOnCancelConfirmModal = () => { setDeleteExpensesConfirmModalVisible(false); }; @@ -194,22 +191,14 @@ function Search({queryJSON}: SearchProps) { } const searchResults = currentSearchResults?.data ? currentSearchResults : lastSearchResultsRef.current; - const newSearchResultKey = useNewSearchResultKey(searchResults); - - const handleSelectionListScroll = useCallback( - (data: Array) => (ref: SelectionListHandle | null) => { - const indexOfNewTransaction = data?.findIndex( - (transaction) => `${ONYXKEYS.COLLECTION.TRANSACTION}${(transaction as TransactionListItemType)?.transactionID}` === newSearchResultKey, - ); - - if (!ref || indexOfNewTransaction < 0) { - return; - } - ref?.scrollToIndex(indexOfNewTransaction); - }, - [newSearchResultKey], - ); + const {newSearchResultKey, handleSelectionListScroll} = useSearchHighlightAndScroll({ + searchResults, + transactions, + previousTransactions, + queryJSON, + offset, + }); // There's a race condition in Onyx which makes it return data from the previous Search, so in addition to checking that the data is loaded // we also need to check that the searchResults matches the type and status of the current search diff --git a/src/hooks/useNewSearchResultKey.ts b/src/hooks/useNewSearchResultKey.ts deleted file mode 100644 index f65f41654bda..000000000000 --- a/src/hooks/useNewSearchResultKey.ts +++ /dev/null @@ -1,94 +0,0 @@ -import {useEffect, useRef, useState} from 'react'; -import type {OnyxEntry} from 'react-native-onyx'; -import type {ReportListItemType, TransactionListItemType} from '@components/SelectionList/types'; -import CONST from '@src/CONST'; -import ONYXKEYS from '@src/ONYXKEYS'; -import type {SearchResults} from '@src/types/onyx'; -import usePrevious from './usePrevious'; - -/** - * Hook used to get the Search results' new transaction key for animation purposes. - */ -function useNewSearchResultKey(searchResults: OnyxEntry) { - // Get the previous search results - const previousSearchResults = usePrevious(searchResults?.data); - - // State to store the new search result key - const [newSearchResultKey, setNewSearchResultKey] = useState(null); - - // Ref to keep track of transaction IDs that have been highlighted - const highlightedTransactionIDs = useRef>(new Set()); - - // Ref to ensure initialization happens only once - const initializedRef = useRef(false); - - // Initialize the set with existing transaction IDs only once - useEffect(() => { - if (initializedRef.current || !searchResults?.data) { - return; - } - const existingTransactionIDs = extractTransactionIDsFromSearchResults(searchResults.data); - highlightedTransactionIDs.current = new Set(existingTransactionIDs); - initializedRef.current = true; - }, [searchResults?.data]); - - useEffect(() => { - if (!previousSearchResults || !searchResults?.data) { - return; - } - - const previousTransactionIDs = extractTransactionIDsFromSearchResults(previousSearchResults); - const currentTransactionIDs = extractTransactionIDsFromSearchResults(searchResults.data); - - // Find new transaction IDs that are not in the previousTransactionIDs and not already highlighted - const newTransactionIDs = currentTransactionIDs.filter((id) => !previousTransactionIDs.includes(id) && !highlightedTransactionIDs.current.has(id)); - - if (newTransactionIDs.length > 0) { - const newTransactionID = newTransactionIDs[0]; - const newTransactionKey = `${ONYXKEYS.COLLECTION.TRANSACTION}${newTransactionID}`; - - setNewSearchResultKey(newTransactionKey); - highlightedTransactionIDs.current.add(newTransactionID); - } - }, [searchResults, previousSearchResults]); - - // Reset newSearchResultKey after it's been used - useEffect(() => { - if (newSearchResultKey === null) { - return; - } - // Reset after a delay if needed (e.g. match animation highlight duration) - const timer = setTimeout(() => { - setNewSearchResultKey(null); - }, CONST.ANIMATED_HIGHLIGHT_START_DURATION); - - return () => clearTimeout(timer); - }, [newSearchResultKey]); - - return newSearchResultKey; -} - -function extractTransactionIDsFromSearchResults(searchResultsData: SearchResults['data']): string[] { - const transactionIDs: string[] = []; - - Object.values(searchResultsData).forEach((item) => { - // Check for transactionID directly on the item (TransactionListItemType) - if ((item as TransactionListItemType)?.transactionID) { - transactionIDs.push((item as TransactionListItemType)?.transactionID); - } - - // Check for transactions array within the item (ReportListItemType) - if (Array.isArray((item as ReportListItemType)?.transactions)) { - (item as ReportListItemType)?.transactions?.forEach((transaction) => { - if (!transaction?.transactionID) { - return; - } - transactionIDs.push(transaction?.transactionID); - }); - } - }); - - return transactionIDs; -} - -export default useNewSearchResultKey; diff --git a/src/hooks/useSearchHighlightAndScroll.ts b/src/hooks/useSearchHighlightAndScroll.ts new file mode 100644 index 000000000000..12228bbe7b86 --- /dev/null +++ b/src/hooks/useSearchHighlightAndScroll.ts @@ -0,0 +1,177 @@ +import {useCallback, useEffect, useRef, useState} from 'react'; +import type {OnyxCollection, OnyxEntry} from 'react-native-onyx'; +import type {SearchQueryJSON} from '@components/Search/types'; +import type {ReportActionListItemType, ReportListItemType, SelectionListHandle, TransactionListItemType} from '@components/SelectionList/types'; +import * as SearchActions from '@libs/actions/Search'; +import CONST from '@src/CONST'; +import ONYXKEYS from '@src/ONYXKEYS'; +import type {SearchResults, Transaction} from '@src/types/onyx'; +import usePrevious from './usePrevious'; + +type UseSearchHighlightAndScroll = { + searchResults: OnyxEntry; + transactions: OnyxCollection; + previousTransactions: OnyxCollection; + queryJSON: SearchQueryJSON; + offset: number; +}; + +/** + * Hook used to trigger a search when a new transaction is added and handle highlighting and scrolling. + */ +function useSearchHighlightAndScroll({searchResults, transactions, previousTransactions, queryJSON, offset}: UseSearchHighlightAndScroll) { + // Ref to track if the search was triggered by this hook + const triggeredByHookRef = useRef(false); + const searchTriggeredRef = useRef(false); + const previousSearchResults = usePrevious(searchResults?.data); + const [newSearchResultKey, setNewSearchResultKey] = useState(null); + const highlightedTransactionIDs = useRef>(new Set()); + const initializedRef = useRef(false); + + // Trigger search when a new transaction is added + useEffect(() => { + const previousTransactionsLength = previousTransactions && Object.keys(previousTransactions).length; + const transactionsLength = transactions && Object.keys(transactions).length; + + // Return early if search was already triggered or there's no change in transactions length + if (searchTriggeredRef.current || previousTransactionsLength === transactionsLength) { + return; + } + + // Check if a new transaction was added + if (transactionsLength && typeof previousTransactionsLength === 'number' && transactionsLength > previousTransactionsLength) { + // Set the flag indicating the search is triggered by the hook + triggeredByHookRef.current = true; + + // Trigger the search + SearchActions.search({queryJSON, offset}); + + // Set the ref to prevent further triggers until reset + searchTriggeredRef.current = true; + } + + // Reset the ref when transactions are updated + return () => { + searchTriggeredRef.current = false; + }; + }, [transactions, previousTransactions, queryJSON, offset]); + + // Initialize the set with existing transaction IDs only once + useEffect(() => { + if (initializedRef.current || !searchResults?.data) { + return; + } + + const existingTransactionIDs = extractTransactionIDsFromSearchResults(searchResults.data); + highlightedTransactionIDs.current = new Set(existingTransactionIDs); + initializedRef.current = true; + }, [searchResults?.data]); + + // Detect new transactions + useEffect(() => { + if (!previousSearchResults || !searchResults?.data) { + return; + } + + const previousTransactionIDs = extractTransactionIDsFromSearchResults(previousSearchResults); + const currentTransactionIDs = extractTransactionIDsFromSearchResults(searchResults.data); + + // Find new transaction IDs that are not in the previousTransactionIDs and not already highlighted + const newTransactionIDs = currentTransactionIDs.filter((id) => !previousTransactionIDs.includes(id) && !highlightedTransactionIDs.current.has(id)); + + if (!triggeredByHookRef.current || newTransactionIDs.length === 0) { + return; + } + + const newTransactionID = newTransactionIDs[0]; + const newTransactionKey = `${ONYXKEYS.COLLECTION.TRANSACTION}${newTransactionID}`; + + setNewSearchResultKey(newTransactionKey); + highlightedTransactionIDs.current.add(newTransactionID); + }, [searchResults, previousSearchResults]); + + // Reset newSearchResultKey after it's been used + useEffect(() => { + if (newSearchResultKey === null) { + return; + } + + const timer = setTimeout(() => { + setNewSearchResultKey(null); + }, CONST.ANIMATED_HIGHLIGHT_START_DURATION); + + return () => clearTimeout(timer); + }, [newSearchResultKey]); + + /** + * Callback to handle scrolling to the new search result. + */ + const handleSelectionListScroll = useCallback( + (data: Array) => (ref: SelectionListHandle | null) => { + // Early return if there's no ref, new transaction wasn't brought in by this hook + // or there's no new search result key + if (!ref || !triggeredByHookRef.current || newSearchResultKey === null) { + return; + } + + // Extract the transaction ID from the newSearchResultKey + const newTransactionID = newSearchResultKey.replace(ONYXKEYS.COLLECTION.TRANSACTION, ''); + + // Find the index of the new transaction in the data array + const indexOfNewTransaction = data.findIndex((item) => { + // Handle TransactionListItemType + if ('transactionID' in item && item.transactionID === newTransactionID) { + return true; + } + + // Handle ReportListItemType with transactions array + if ('transactions' in item && Array.isArray(item.transactions)) { + return item.transactions.some((transaction) => transaction?.transactionID === newTransactionID); + } + + return false; + }); + + // Early return if the transaction is not found in the data array + if (indexOfNewTransaction <= 0) { + return; + } + + // Perform the scrolling action + ref.scrollToIndex(indexOfNewTransaction); + // Reset the trigger flag to prevent unintended future scrolls and highlights + triggeredByHookRef.current = false; + }, + [newSearchResultKey], + ); + + return {newSearchResultKey, handleSelectionListScroll}; +} + +/** + * Helper function to extract transaction IDs from search results data. + */ +function extractTransactionIDsFromSearchResults(searchResultsData: Partial): string[] { + const transactionIDs: string[] = []; + + Object.values(searchResultsData).forEach((item) => { + // Check for transactionID directly on the item (TransactionListItemType) + if ((item as TransactionListItemType)?.transactionID) { + transactionIDs.push((item as TransactionListItemType).transactionID); + } + + // Check for transactions array within the item (ReportListItemType) + if (Array.isArray((item as ReportListItemType)?.transactions)) { + (item as ReportListItemType).transactions.forEach((transaction) => { + if (!transaction?.transactionID) { + return; + } + transactionIDs.push(transaction.transactionID); + }); + } + }); + + return transactionIDs; +} + +export default useSearchHighlightAndScroll; diff --git a/src/hooks/useTriggerSearchOnTransactionIncrease.ts b/src/hooks/useTriggerSearchOnTransactionIncrease.ts deleted file mode 100644 index 0e5b114bb6ca..000000000000 --- a/src/hooks/useTriggerSearchOnTransactionIncrease.ts +++ /dev/null @@ -1,45 +0,0 @@ -import {useEffect, useRef} from 'react'; -import type {OnyxCollection} from 'react-native-onyx'; -import type {SearchQueryJSON} from '@components/Search/types'; -import * as SearchActions from '@libs/actions/Search'; -import type {Transaction} from '@src/types/onyx'; - -type UseTriggerSearchOnNewTransaction = { - transactions: OnyxCollection; - previousTransactions: OnyxCollection; - queryJSON: SearchQueryJSON; - offset: number; -}; - -/** - * Hook used to trigger a new Search API call when a new transaction is created. - */ -function useTriggerSearchOnNewTransaction({transactions, previousTransactions, queryJSON, offset}: UseTriggerSearchOnNewTransaction) { - const searchTriggeredRef = useRef(false); - - useEffect(() => { - const previousTransactionsLength = previousTransactions && Object.keys(previousTransactions).length; - const transactionsLength = transactions && Object.keys(transactions).length; - - // Return early if search was already triggered or there's no change in transactions length - if (searchTriggeredRef.current || previousTransactionsLength === transactionsLength) { - return; - } - - // Checking if length exists (we check if previousTransactionsLength is number because - // if we check for existance and initially it's 0 then the check will fail) - if (transactionsLength && typeof previousTransactionsLength === 'number' && transactionsLength > previousTransactionsLength) { - // A new transaction was added, trigger the action - SearchActions.search({queryJSON, offset}); - // Set the flag to true to prevent further triggers - searchTriggeredRef.current = true; - } - - // Reset the flag when transactions are updated - return () => { - searchTriggeredRef.current = false; - }; - }, [transactions, previousTransactions, queryJSON, offset]); -} - -export default useTriggerSearchOnNewTransaction; From c66f1483942e951ff57398a4cbfb20f35251806a Mon Sep 17 00:00:00 2001 From: Kevin Brian Bader Date: Fri, 27 Sep 2024 13:08:21 -0700 Subject: [PATCH 13/13] prettier --- help/index.markdown | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/help/index.markdown b/help/index.markdown index e5d075402ecb..b198c5e20781 100644 --- a/help/index.markdown +++ b/help/index.markdown @@ -1,5 +1,7 @@ --- title: New Expensify Help --- + Pages: -* [Expensify Superapp](/superapp.html) + +- [Expensify Superapp](/superapp.html)