Skip to content

Commit 998484c

Browse files
mountinymountiny
authored
Merge pull request #91958 from callstack-internal/perf/hoist-transaction-violations
perf: hoist transaction violations for MoneyRequestReportTransactionList items
2 parents 00a1025 + 5870f7c commit 998484c

5 files changed

Lines changed: 125 additions & 37 deletions

File tree

‎src/components/MoneyRequestReportView/MoneyRequestReportTransactionItem.tsx‎

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -15,20 +15,22 @@ import useStyleUtils from '@hooks/useStyleUtils';
1515
import useTheme from '@hooks/useTheme';
1616
import useThemeStyles from '@hooks/useThemeStyles';
1717
import useTransactionInlineEdit from '@hooks/useTransactionInlineEdit';
18-
import useTransactionViolations from '@hooks/useTransactionViolations';
1918
import ControlSelection from '@libs/ControlSelection';
2019
import canUseTouchScreen from '@libs/DeviceCapabilities/canUseTouchScreen';
2120
import {hasFlexColumn} from '@libs/SearchUIUtils';
2221
import {getTransactionPendingAction, isTransactionPendingDelete} from '@libs/TransactionUtils';
2322
import variables from '@styles/variables';
2423
import CONST from '@src/CONST';
25-
import type {CardList, Policy, Report} from '@src/types/onyx';
24+
import type {CardList, Policy, Report, TransactionViolations} from '@src/types/onyx';
2625
import type {TransactionWithOptionalHighlight} from './MoneyRequestReportTransactionList';
2726

2827
type MoneyRequestReportTransactionItemProps = {
2928
/** The transaction that is being displayed */
3029
transaction: TransactionWithOptionalHighlight;
3130

31+
/** Pre-filtered violations for this transaction. Computed once at the parent so each row doesn't subscribe to Onyx individually. */
32+
violations: TransactionViolations;
33+
3234
/** Report to which the transaction belongs */
3335
report: Report;
3436

@@ -91,6 +93,7 @@ type MoneyRequestReportTransactionItemBodyProps = MoneyRequestReportTransactionI
9193

9294
function MoneyRequestReportTransactionItemBody({
9395
transaction,
96+
violations,
9497
report,
9598
policy,
9699
isSelectionModeEnabled,
@@ -121,8 +124,6 @@ function MoneyRequestReportTransactionItemBody({
121124
const {shouldUseNarrowLayout} = useResponsiveLayoutOnWideRHP();
122125
const isPendingDelete = isTransactionPendingDelete(transaction);
123126
const pendingAction = getTransactionPendingAction(transaction);
124-
// Filter violations based on user visibility and dismissal state at the row level.
125-
const filteredViolations = useTransactionViolations(transaction.transactionID);
126127

127128
// On narrow layouts `inlineEdit` is undefined (the parent skips the hook). The fallback ref
128129
// keeps the press handler shape identical without ever being mutated on narrow.
@@ -187,7 +188,7 @@ function MoneyRequestReportTransactionItemBody({
187188
{({hovered}) => (
188189
<TransactionItemRow
189190
transactionItem={transaction}
190-
violations={filteredViolations}
191+
violations={violations}
191192
report={report}
192193
policy={policy}
193194
isSelected={isSelected}

‎src/components/MoneyRequestReportView/MoneyRequestReportTransactionList.tsx‎

Lines changed: 39 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -63,7 +63,7 @@ import type {SortableColumnName} from '@libs/ReportUtils';
6363
import {compareValues, getColumnsToShow, getTableMinWidth, hasFlexColumn, isTransactionAmountTooLong, isTransactionTaxAmountTooLong} from '@libs/SearchUIUtils';
6464
import {getPendingSubmitFollowUpAction} from '@libs/telemetry/submitFollowUpAction';
6565
import {transactionHasRBR} from '@libs/TransactionPreviewUtils';
66-
import {getTransactionPendingAction, isTransactionPendingDelete, shouldShowExpenseBreakdown} from '@libs/TransactionUtils';
66+
import {getTransactionPendingAction, getVisibleTransactionViolations, isTransactionPendingDelete, shouldShowExpenseBreakdown} from '@libs/TransactionUtils';
6767
import shouldShowTransactionYear from '@libs/TransactionUtils/shouldShowTransactionYear';
6868
import isReportOpenInSuperWideRHP from '@navigation/helpers/isReportOpenInSuperWideRHP';
6969
import Navigation from '@navigation/Navigation';
@@ -86,6 +86,32 @@ import SearchMoneyRequestReportEmptyState from './SearchMoneyRequestReportEmptyS
8686

8787
const PENDING_EXPENSE_REASON_ATTRIBUTES = {context: 'MoneyRequestReportTransactionList.PendingExpensePlaceholder'} as const;
8888

89+
const EMPTY_VIOLATIONS: OnyxTypes.TransactionViolations = [];
90+
91+
/**
92+
* Looks up violations from the bulk collection and filters them via `getVisibleTransactionViolations`.
93+
* Returns the stable EMPTY_VIOLATIONS reference for the common no-violations case so the row's prop
94+
* identity stays stable across FlashList recycles.
95+
*/
96+
function filterTransactionViolations(
97+
transaction: TransactionWithOptionalHighlight,
98+
allViolations: Record<string, OnyxTypes.TransactionViolations | undefined> | undefined,
99+
email: string,
100+
accountID: number,
101+
report: OnyxTypes.Report,
102+
policy: OnyxTypes.Policy | undefined,
103+
): OnyxTypes.TransactionViolations {
104+
if (!allViolations) {
105+
return EMPTY_VIOLATIONS;
106+
}
107+
const raw = allViolations[`${ONYXKEYS.COLLECTION.TRANSACTION_VIOLATIONS}${transaction.transactionID}`];
108+
if (!raw?.length) {
109+
return EMPTY_VIOLATIONS;
110+
}
111+
const filtered = getVisibleTransactionViolations(transaction, raw, email, accountID, report, policy);
112+
return filtered.length === 0 ? EMPTY_VIOLATIONS : filtered;
113+
}
114+
89115
type MoneyRequestReportTransactionListProps = {
90116
/** The money request report containing the transactions */
91117
report: OnyxTypes.Report;
@@ -614,10 +640,22 @@ function MoneyRequestReportTransactionList({
614640
return visibleTransactions.at(-1)?.transactionID;
615641
}, [shouldShowGroupedTransactions, groupedTransactions, resolvedTransactions, isOffline]);
616642

643+
const violationsByTransactionID = useMemo(() => {
644+
const map = new Map<string, OnyxTypes.TransactionViolations>();
645+
const email = currentUserDetails.email ?? '';
646+
const accountID = currentUserDetails.accountID ?? CONST.DEFAULT_NUMBER_ID;
647+
648+
for (const transaction of resolvedTransactions) {
649+
map.set(transaction.transactionID, filterTransactionViolations(transaction, allTransactionViolations, email, accountID, report, policy ?? undefined));
650+
}
651+
return map;
652+
}, [resolvedTransactions, allTransactionViolations, currentUserDetails.email, currentUserDetails.accountID, report, policy]);
653+
617654
const renderTransactionItem = (transaction: TransactionWithOptionalHighlight) => (
618655
<MoneyRequestReportTransactionItem
619656
key={transaction.transactionID}
620657
transaction={transaction}
658+
violations={violationsByTransactionID.get(transaction.transactionID) ?? EMPTY_VIOLATIONS}
621659
shouldBeHighlighted={highlightedTransactionIDs.has(transaction.transactionID)}
622660
columns={columnsToShow}
623661
report={report}

‎src/hooks/useTransactionViolations.ts‎

Lines changed: 9 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
import {useMemo} from 'react';
22
import getNonEmptyStringOnyxID from '@libs/getNonEmptyStringOnyxID';
3-
import {isViolationDismissed, mergeProhibitedViolations, shouldShowViolation} from '@libs/TransactionUtils';
3+
import {getVisibleTransactionViolations} from '@libs/TransactionUtils';
44
import ONYXKEYS from '@src/ONYXKEYS';
55
import type {TransactionViolation, TransactionViolations} from '@src/types/onyx';
66
import getEmptyArray from '@src/types/utils/getEmptyArray';
@@ -16,12 +16,14 @@ function useTransactionViolations(transactionID?: string, shouldShowRterForSettl
1616

1717
return useMemo(
1818
() =>
19-
mergeProhibitedViolations(
20-
transactionViolations.filter(
21-
(violation: TransactionViolation) =>
22-
!isViolationDismissed(transaction, violation, currentUserDetails.email ?? '', currentUserDetails.accountID, iouReport, policy) &&
23-
shouldShowViolation(iouReport, policy, violation.name, currentUserDetails.email ?? '', shouldShowRterForSettledReport, transaction),
24-
),
19+
getVisibleTransactionViolations(
20+
transaction,
21+
transactionViolations,
22+
currentUserDetails.email ?? '',
23+
currentUserDetails.accountID,
24+
iouReport,
25+
policy,
26+
shouldShowRterForSettledReport,
2527
),
2628
[transaction, transactionViolations, iouReport, policy, shouldShowRterForSettledReport, currentUserDetails.email, currentUserDetails.accountID],
2729
);

‎src/libs/TransactionUtils/index.ts‎

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1616,6 +1616,28 @@ function mergeProhibitedViolations(transactionViolations: TransactionViolations)
16161616
return [...transactionViolations.filter((violation: TransactionViolation) => violation.name !== CONST.VIOLATIONS.PROHIBITED_EXPENSE), mergedProhibitedViolations];
16171617
}
16181618

1619+
/**
1620+
* Returns transaction violations visible to the current user after applying dismiss/show filters
1621+
* and merging prohibited-expense violations.
1622+
*/
1623+
function getVisibleTransactionViolations(
1624+
transaction: OnyxEntry<Transaction>,
1625+
transactionViolations: TransactionViolations,
1626+
currentUserEmail: string,
1627+
currentUserAccountID: number,
1628+
iouReport: OnyxEntry<Report>,
1629+
policy: OnyxEntry<Policy>,
1630+
shouldShowRterForSettledReport = true,
1631+
): TransactionViolations {
1632+
return mergeProhibitedViolations(
1633+
transactionViolations.filter(
1634+
(violation) =>
1635+
!isViolationDismissed(transaction, violation, currentUserEmail, currentUserAccountID, iouReport, policy) &&
1636+
shouldShowViolation(iouReport, policy, violation.name, currentUserEmail, shouldShowRterForSettledReport, transaction),
1637+
),
1638+
);
1639+
}
1640+
16191641
/**
16201642
* Check if the user should see the violation
16211643
*/
@@ -2957,6 +2979,7 @@ export {
29572979
isCorporateCardTransaction,
29582980
isExpenseUnreported,
29592981
mergeProhibitedViolations,
2982+
getVisibleTransactionViolations,
29602983
getOriginalAttendees,
29612984
getReportOwnerAsAttendee,
29622985
isFromCreditCardImport,

‎tests/unit/useTransactionViolationsTest.ts‎

Lines changed: 48 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -11,35 +11,59 @@ import waitForBatchedUpdates from '../utils/waitForBatchedUpdates';
1111
jest.mock('@libs/TransactionUtils', () => {
1212
// eslint-disable-next-line @typescript-eslint/no-unsafe-member-access
1313
const CONST_MOCK = jest.requireActual('@src/CONST').default as typeof CONST_TYPE;
14-
return {
15-
isViolationDismissed: jest.fn(),
16-
shouldShowViolation: jest.fn(),
17-
mergeProhibitedViolations: (transactionViolations: Array<{name: string; type: string; showInReview?: boolean; data?: {prohibitedExpenseRule?: string | string[]}}>) => {
18-
const prohibitedViolations = transactionViolations.filter((violation) => violation.name === CONST_MOCK.VIOLATIONS.PROHIBITED_EXPENSE);
1914

20-
if (prohibitedViolations.length === 0) {
21-
return transactionViolations;
15+
const mergeProhibitedViolations = (transactionViolations: Array<{name: string; type: string; showInReview?: boolean; data?: {prohibitedExpenseRule?: string | string[]}}>) => {
16+
const prohibitedViolations = transactionViolations.filter((violation) => violation.name === CONST_MOCK.VIOLATIONS.PROHIBITED_EXPENSE);
17+
18+
if (prohibitedViolations.length === 0) {
19+
return transactionViolations;
20+
}
21+
22+
const prohibitedExpenses = prohibitedViolations.flatMap((violation) => {
23+
const rule = violation.data?.prohibitedExpenseRule;
24+
if (!rule) {
25+
return [];
2226
}
27+
return Array.isArray(rule) ? rule : [rule];
28+
});
2329

24-
const prohibitedExpenses = prohibitedViolations.flatMap((violation) => {
25-
const rule = violation.data?.prohibitedExpenseRule;
26-
if (!rule) {
27-
return [];
28-
}
29-
return Array.isArray(rule) ? rule : [rule];
30-
});
30+
const mergedProhibitedViolations = {
31+
name: CONST_MOCK.VIOLATIONS.PROHIBITED_EXPENSE,
32+
data: {
33+
prohibitedExpenseRule: prohibitedExpenses,
34+
},
35+
type: CONST_MOCK.VIOLATION_TYPES.VIOLATION,
36+
showInReview: prohibitedViolations.some((v) => v.showInReview),
37+
};
3138

32-
const mergedProhibitedViolations = {
33-
name: CONST_MOCK.VIOLATIONS.PROHIBITED_EXPENSE,
34-
data: {
35-
prohibitedExpenseRule: prohibitedExpenses,
36-
},
37-
type: CONST_MOCK.VIOLATION_TYPES.VIOLATION,
38-
showInReview: prohibitedViolations.some((v) => v.showInReview),
39-
};
39+
return [...transactionViolations.filter((violation) => violation.name !== CONST_MOCK.VIOLATIONS.PROHIBITED_EXPENSE), mergedProhibitedViolations];
40+
};
4041

41-
return [...transactionViolations.filter((violation) => violation.name !== CONST_MOCK.VIOLATIONS.PROHIBITED_EXPENSE), mergedProhibitedViolations];
42-
},
42+
const mockIsViolationDismissed = jest.fn();
43+
const mockShouldShowViolation = jest.fn();
44+
45+
const getVisibleTransactionViolations = (
46+
transaction: Transaction | undefined,
47+
transactionViolations: TransactionViolations,
48+
currentUserEmail: string,
49+
currentUserAccountID: number,
50+
iouReport: Report | undefined,
51+
policy: Policy | undefined,
52+
shouldShowRterForSettledReport = true,
53+
) =>
54+
mergeProhibitedViolations(
55+
transactionViolations.filter(
56+
(violation) =>
57+
!mockIsViolationDismissed(transaction, violation, currentUserEmail, currentUserAccountID, iouReport, policy) &&
58+
mockShouldShowViolation(iouReport, policy, violation.name, currentUserEmail, shouldShowRterForSettledReport, transaction),
59+
),
60+
);
61+
62+
return {
63+
isViolationDismissed: mockIsViolationDismissed,
64+
shouldShowViolation: mockShouldShowViolation,
65+
mergeProhibitedViolations,
66+
getVisibleTransactionViolations,
4367
};
4468
});
4569

0 commit comments

Comments
 (0)