Fix bulk expense edit performance hang on large reports #90709
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,4 @@ | ||
| import React, {useEffect, useState} from 'react'; | ||
| import {View} from 'react-native'; | ||
| import type {ValueOf} from 'type-fest'; | ||
| import Button from '@components/Button'; | ||
|
|
@@ -44,7 +44,7 @@ function SearchEditMultiplePage() { | |
| const {currentSearchHash} = useSearchQueryContext(); | ||
| const {currentSearchResults} = useSearchResultsContext(); | ||
| const {clearSelectedTransactions} = useSearchSelectionActions(); | ||
| const {accountID: currentUserAccountID} = useCurrentUserPersonalDetails(); | ||
| const delegateAccountID = useDelegateAccountID(); | ||
| const [policies] = useOnyx(ONYXKEYS.COLLECTION.POLICY); | ||
| const [activePolicyID] = useOnyx(ONYXKEYS.NVP_ACTIVE_POLICY_ID); | ||
|
|
@@ -54,8 +54,6 @@ function SearchEditMultiplePage() { | |
| const [allReportActions] = useOnyx(ONYXKEYS.COLLECTION.REPORT_ACTIONS); | ||
| const [allPolicyTags] = useOnyx(ONYXKEYS.COLLECTION.POLICY_TAGS); | ||
| const [allPolicyCategories] = useOnyx(ONYXKEYS.COLLECTION.POLICY_CATEGORIES); | ||
|
|
||
| const snapshotData = currentSearchResults?.data; | ||
| const mergedTransactions = withSnapshotTransactions(allTransactions, snapshotData); | ||
|
|
@@ -127,8 +125,10 @@ function SearchEditMultiplePage() { | |
| }; | ||
| }, []); | ||
|
|
||
| const [isSaving, setIsSaving] = useState(false); | ||
|
|
||
| const save = () => { | ||
| if (!draftTransaction || isSaving) { | ||
| return; | ||
| } | ||
|
|
||
|
|
@@ -166,29 +166,32 @@ function SearchEditMultiplePage() { | |
| return; | ||
| } | ||
|
|
||
| setIsSaving(true); | ||
|
|
||
| // Defer the bulk edit loop so the loading spinner has a chance to paint | ||
| // before the synchronous Onyx writes block the JS thread. | ||
| requestAnimationFrame(() => { | ||
|
Contributor
There was a problem hiding this comment. ❌ PERF-12 (docs)The Store the rAF ID and cancel it on unmount, or use a ref to track whether the component is still mounted: const rafIdRef = useRef<number | null>(null);
useEffect(() => {
return () => {
if (rafIdRef.current !== null) {
cancelAnimationFrame(rafIdRef.current);
}
};
}, []);
// In save():
rafIdRef.current = requestAnimationFrame(() => {
// ... existing logic
rafIdRef.current = null;
});Reviewed at: 613f9f5 | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
Contributor
There was a problem hiding this comment. @MelvinBot do you think this comment has merit?
Contributor
Author
There was a problem hiding this comment. The concern is theoretically valid but practically a no-op here. The rAF callback only calls stateless action functions ( That said, it's a very cheap fix (3 lines) and makes the intent clearer. I'd call it nice-to-have rather than must-fix — your call on whether it's worth an extra commit.
Contributor
There was a problem hiding this comment. ❌ PERF-12 (docs)The Store the rAF ID and cancel it on unmount, or cancel it in the existing cleanup effect: const rafIdRef = useRef<number>();
useEffect(() => {
return () => {
if (rafIdRef.current) {
cancelAnimationFrame(rafIdRef.current);
}
clearBulkEditDraftTransaction();
};
}, []);
// In save():
rafIdRef.current = requestAnimationFrame(() => {
// ... existing callback
});Reviewed at: efc4f02 | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency. |
||
| updateMultipleMoneyRequests({ | ||
| transactionIDs: selectedTransactionIDs, | ||
| changes, | ||
| policy, | ||
| reports: mergedReports, | ||
| transactions: mergedTransactions, | ||
| reportActions: mergedReportActions, | ||
| policyCategories: allPolicyCategories, | ||
| policyTags: allPolicyTags, | ||
| hash: currentSearchHash, | ||
| allPolicies: policies, | ||
| currentUserAccountID, | ||
| delegateAccountID, | ||
| }); | ||
| // Bulk edit can start from report (ID-based selection) or search (map-based selection), | ||
| // so clear both stores to keep deselection behavior consistent. | ||
| clearSelectedTransactions(true); | ||
| clearSelectedTransactions(); | ||
|
|
||
| Navigation.dismissToPreviousRHP(); | ||
| }); | ||
| }; | ||
|
|
||
| const currency = policy?.outputCurrency ?? CONST.CURRENCY.USD; | ||
|
|
@@ -324,6 +327,8 @@ function SearchEditMultiplePage() { | |
| large | ||
| text={translate('common.save')} | ||
| onPress={save} | ||
| isLoading={isSaving} | ||
| isDisabled={isSaving} | ||
| style={[styles.m5]} | ||
| /> | ||
| </View> | ||
|
|
||
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.
When a transaction has no existing thread, this code only seeds
REPORT_<threadID>but never updates the parent IOU report action withchildReportID. In this flow, thread discovery still depends ontransaction.transactionThreadReportID ?? reportAction.childReportID(see earlier in the same function), so subsequent offline edits of the same expense will generate a new random thread each time and previously addedMODIFIED_EXPENSEactions become unreachable from the parent expense report until a server round-trip repairs links. The previouscreateTransactionThreadReport()path handled this linkage viaopenReportoptimistic data.Useful? React with 👍 / 👎.
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.
@MelvinBot Yeah... Good point. Please link newly created thread back to parent IOU action as shared in review comments.
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.
Good catch — the review comment is correct. When
createTransactionThreadReportwas replaced with inline optimistic thread creation, thechildReportIDlinkage on the parent IOU action was lost.What's missing: After creating the optimistic thread at BulkEdit.ts:139-147, the optimistic data block at BulkEdit.ts:289-303 only seeds the thread report into Onyx but never merges
childReportIDonto the parent report action. The reference pattern at Transaction.ts:1356-1379 shows the correct approach.Fix needed inside the
if (didCreateThreadInThisIteration ...)block:childReportID: transactionThreadReportIDonto the parent IOU report action inREPORT_ACTIONSchildReportIDtonullmarcaaron— should I implement this fix? (I can't push changes atrojiphil's request since they aren't the author, assignee, or co-author on this PR.)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.
This is a valid catch. The optimistic thread creation here seeds
REPORT_<threadID>but doesn't writechildReportIDback to the parent report action ortransactionThreadReportIDon the transaction. The oldcreateTransactionThreadReportpath handled this viaopenReportoptimistic data.Within a single bulk edit invocation this isn't an issue (each transaction is processed once), but if the user triggers two separate bulk edits while offline on threadless expenses, the lookup at line 128 would again find both fields undefined and generate a duplicate thread.
The fix should add two more optimistic updates when
didCreateThreadInThisIterationis true:childReportIDonto the parent report actiontransactionThreadReportIDonto the transactionmarcaaron— this is worth addressing before merge. I can implement the fix if you'd like (add me as assignee and reply).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.
@MelvinBot proceed with the fix.
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.
Done — pushed 248ea3d. When
didCreateThreadInThisIterationis true, the optimistic data now:childReportIDonto the parent IOU report actiontransactionThreadReportIDin the transaction optimistic updateBoth have failure rollbacks. All 25 BulkEdit tests pass.