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

Fix - Search - Expense from the list does not disappear after deletion #54407

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
35 changes: 23 additions & 12 deletions src/hooks/useSearchHighlightAndScroll.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import isEqual from 'lodash/isEqual';
import {useCallback, useEffect, useRef, useState} from 'react';
import type {OnyxCollection, OnyxEntry} from 'react-native-onyx';
import type {SearchQueryJSON} from '@components/Search/types';
Expand Down Expand Up @@ -26,6 +27,7 @@ function useSearchHighlightAndScroll({searchResults, transactions, previousTrans
// Ref to track if the search was triggered by this hook
const triggeredByHookRef = useRef(false);
const searchTriggeredRef = useRef(false);
const hasItemBeenAddedRef = useRef(false);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const hasItemBeenAddedRef = useRef(false);
const hasNewItemsRef = useRef(false);

const previousSearchResults = usePrevious(searchResults?.data);
const [newSearchResultKey, setNewSearchResultKey] = useState<string | null>(null);
const highlightedIDs = useRef<Set<string>>(new Set());
Expand All @@ -34,20 +36,29 @@ function useSearchHighlightAndScroll({searchResults, transactions, previousTrans

// Trigger search when a new report action is added while on chat or when a new transaction is added for the other search types.
useEffect(() => {
const previousTransactionsLength = previousTransactions && Object.keys(previousTransactions).length;
const transactionsLength = transactions && Object.keys(transactions).length;
const previousTransactionIDList = Object.keys(previousTransactions ?? {});
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const previousTransactionIDList = Object.keys(previousTransactions ?? {});
const previousTransactionsIDs = Object.keys(previousTransactions ?? {});

const transactionIDList = Object.keys(transactions ?? {});
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const transactionIDList = Object.keys(transactions ?? {});
const transactionsIDs = Object.keys(transactions ?? {});


const reportActionsLength = reportActions && Object.values(reportActions).reduce((sum, curr) => sum + Object.keys(curr ?? {}).length, 0);
const prevReportActionsLength = previousReportActions && Object.values(previousReportActions).reduce((sum, curr) => sum + Object.keys(curr ?? {}).length, 0);
// Return early if search was already triggered or there's no change in current and previous data length
if (searchTriggeredRef.current || (!isChat && previousTransactionsLength === transactionsLength) || (isChat && reportActionsLength === prevReportActionsLength)) {
const reportActionIDList = Object.values(reportActions ?? {})
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const reportActionIDList = Object.values(reportActions ?? {})
const reportActionsIDs = Object.values(reportActions ?? {})

.map((actions) => Object.keys(actions ?? {}))
.flat();
const previousReportActionIDList = Object.values(previousReportActions ?? {})
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const previousReportActionIDList = Object.values(previousReportActions ?? {})
const previousReportActionsIDs = Object.values(previousReportActions ?? {})

.map((actions) => Object.keys(actions ?? {}))
.flat();

if (searchTriggeredRef.current) {
return;
}
const newTransactionAdded = transactionsLength && typeof previousTransactionsLength === 'number' && transactionsLength > previousTransactionsLength;
const newReportActionAdded = reportActionsLength && typeof prevReportActionsLength === 'number' && reportActionsLength > prevReportActionsLength;
const hasTransactionChange = !isEqual(transactionIDList, previousTransactionIDList);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const hasTransactionChange = !isEqual(transactionIDList, previousTransactionIDList);
const hasTransactionsIDsChange = !isEqual(transactionIDList, previousTransactionIDList);

const hasReportActionChange = !isEqual(reportActionIDList, previousReportActionIDList);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const hasReportActionChange = !isEqual(reportActionIDList, previousReportActionIDList);
const hasReportActionsIDsChange = !isEqual(reportActionIDList, previousReportActionIDList);


// Check if there is a change in transaction or report action list
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// Check if there is a change in transaction or report action list
// Check if there is a change in the transactions list or the report actions list

if ((!isChat && hasTransactionChange) || (isChat && hasReportActionChange)) {
// We only want to highlight new items only if addition of transactions or report actions triggered the search.
// This is because on deletion of items sometimes the BE returns old items in place of the deleted ones
// but we don't want to highlight these old items although they are new to the current search result.
Comment on lines +57 to +59
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// We only want to highlight new items only if addition of transactions or report actions triggered the search.
// This is because on deletion of items sometimes the BE returns old items in place of the deleted ones
// but we don't want to highlight these old items although they are new to the current search result.
// We only want to highlight new items if the addition of transactions or report actions triggered the search.
// This is because, on deletion of items, the backend sometimes returns old items in place of the deleted ones.
// We don't want to highlight these old items, even if they appear new in the current search results.

hasItemBeenAddedRef.current = isChat ? reportActionIDList.length > previousReportActionIDList.length : transactionIDList.length > previousTransactionIDList.length;

// Check if a new transaction or report action was added
if ((!isChat && !!newTransactionAdded) || (isChat && !!newReportActionAdded)) {
// Set the flag indicating the search is triggered by the hook
triggeredByHookRef.current = true;

Expand Down Expand Up @@ -87,7 +98,7 @@ function useSearchHighlightAndScroll({searchResults, transactions, previousTrans
// Find new report action IDs that are not in the previousReportActionIDs and not already highlighted
const newReportActionIDs = currentReportActionIDs.filter((id) => !previousReportActionIDs.includes(id) && !highlightedIDs.current.has(id));

if (!triggeredByHookRef.current || newReportActionIDs.length === 0) {
if (!triggeredByHookRef.current || newReportActionIDs.length === 0 || !hasItemBeenAddedRef.current) {
return;
}

Expand All @@ -103,7 +114,7 @@ function useSearchHighlightAndScroll({searchResults, transactions, previousTrans
// Find new transaction IDs that are not in the previousTransactionIDs and not already highlighted
const newTransactionIDs = currentTransactionIDs.filter((id) => !previousTransactionIDs.includes(id) && !highlightedIDs.current.has(id));

if (!triggeredByHookRef.current || newTransactionIDs.length === 0) {
if (!triggeredByHookRef.current || newTransactionIDs.length === 0 || !hasItemBeenAddedRef.current) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice catch!

return;
}

Expand Down
Loading