[HOLD][No QA] Make bulk-actions hooks React Compiler compliant and perf-clean them#88189
Draft
roryabraham wants to merge 20 commits intomainfrom
Draft
[HOLD][No QA] Make bulk-actions hooks React Compiler compliant and perf-clean them#88189roryabraham wants to merge 20 commits intomainfrom
roryabraham wants to merge 20 commits intomainfrom
Conversation
Replace the duplicateHandlerRef pattern (which reads a ref during render and bails React Compiler) with a useState slot, strip manual useMemo/ useCallback wrappers now that the compiler can own memoization, and regroup hooks, state, and handlers for readability. Made-with: Cursor
Replace the duplicateHandlerRef and onBulkPaySelectedRef patterns (render-time ref reads that bail React Compiler) with a useState slot and direct callback return, swap the lastNonEmptySearchResultsRef fallback for usePreviousDefined, strip manual useMemo/useCallback wrappers now that the compiler can own memoization, inline the headerButtonsOptions IIFE into an if/else chain, and regroup hooks, state, and handlers for readability. Made-with: Cursor
isDeleteModalVisible, showDeleteModal, and hideDeleteModal were exported but not consumed: SelectionToolbar passes its own onDeleteSelected callback and MoneyReportHeader overrides the Delete option's onSelected with a local showConfirmModal flow. Drop the useState, both helpers, and the unreachable else branch; collapse the onSelected to a single optional-chained call. Remove the stale assertions and the "show and hide delete modal" test case from the unit test. Made-with: Cursor
useSearchBulkActions: - Drop unused ShouldShowBulkDuplicateParams type export - Drop unreachable selectedTransactionReportIDs ?? selectedReportIDs fallback (selectedTransactionReportIDs is always a defined array) - Drop redundant status == null re-check in handleBasicExport (already handled by the earlier early return) - Drop the standalone accountID destructuring and consistently use currentUserPersonalDetails?.accountID Stop re-exporting raw Onyx collections from both hooks. Consumers (MoneyReportHeader, SelectionToolbar, SearchBulkActionsButton) now read COLLECTION.TRANSACTION / COLLECTION.REPORT directly and pull searchData from useSearchStateContext, matching how they already subscribe to related keys. Updated the duplicate-action unit test helper to do the same. Made-with: Cursor
The hooks already subscribe to COLLECTION.TRANSACTION / COLLECTION.REPORT internally (needed for shouldShowBulkDuplicateOption and other derived values). Having consumers also subscribe directly creates a duplicate subscription to the same key on the same fiber without eliminating the hook-internal one. Go back to returning allTransactions, allReports, and searchData from the hooks so BulkDuplicateHandler can receive them as props from a single subscription. Reverts only the Onyx-reexport portion of 99585ef; the dead-code cleanups (ShouldShowBulkDuplicateParams, selectedTransactionReportIDs fallback, status re-check, accountID destructuring) stay. Made-with: Cursor
The list was built on every render by iterating every workspace policy, just to answer one O(1) question inside onBulkPaySelected: does this specific policy have a VBBA? Replace the list + .includes() check with a direct policy lookup at click time. Made-with: Cursor
The submitterReports scan walked every key in currentSearchResults.data on every render just to answer a question that is only asked inside the hold option's onSelected. Wrap the computation in a thunk that the hold handler invokes on click, so search pages with thousands of results stop paying this per render. Made-with: Cursor
…Actions The hook previously iterated selectedTransactionIDs three times at the top of render (build selectedTransactionsList, owner analysis, build selectedTransactionsForDuplicate) with two redundant allTransactions lookups per ID. Fold them into a single pass. Made-with: Cursor
The hook previously iterated Object.values(selectedTransactions) seven times per render to derive selectedTransactionReportIDs, selectedPolicyIDs, the selectedBulkCurrency fallback, expenseCount/uniqueReportCount, isAnyTransactionOnHold, isAllOneTransactionReport, and the selectedTransactionsList inside the non-all-matching branch. Fold them into a single Object.entries pass at the top of the hook so each selected transaction is visited once per render. Event-handler-local traversals (handleApproveWithDEWCheck, onBulkPaySelected, edit-multiple onSelected) are unchanged - they only run on click. Made-with: Cursor
Drop the useMemo wrapper - React Compiler already memoizes this hook. Replace the Object.keys + filter + reduce + double-spread chain with a single shallow copy of the Onyx collection followed by one flat loop that folds in search-only transactions. Behavior is unchanged: Onyx values still take precedence over search snapshots. Made-with: Cursor
When the current search result set does not surface any transactions that aren't already in the Onyx collection, return the Onyx reference directly instead of eagerly cloning it. The clone still happens when there is genuinely something to merge, preserving the "Onyx takes precedence" semantics. Made-with: Cursor
The bulk-actions hooks cleanup dropped five warnings (via the ref-forwarding rewrite and the useMemo/useCallback stripping). Sync the cap with the new count so it catches any regression. Made-with: Cursor
The previous filter+reduce+double-spread chain always returned a defined
object because of the trailing spread. The defer-copy rewrite lost that
guarantee: when search data is present but contains no transaction
entries and the Onyx collection is also empty, the hook returned
undefined. That broke "useAllTransactions > should handle empty
collection and empty search results". Restore the always-defined return
with a trailing ?? {} in the search-data branch.
Made-with: Cursor
…SelectedTransactionsActions Made-with: Cursor # Conflicts: # android/app/build.gradle # ios/NewExpensify/Info.plist # ios/NotificationServiceExtension/Info.plist # ios/ShareViewController/Info.plist # package-lock.json # package.json # src/hooks/useSearchBulkActions.ts # src/hooks/useSelectedTransactionsActions.ts # src/libs/actions/Link.ts # src/pages/iou/request/step/IOURequestStepConfirmation.tsx # src/pages/iou/request/step/IOURequestStepScan/components/ReceiptPreviews/index.tsx
The merge resolution left a stray blank line that the CI Prettier check flagged. Apply `npm run prettier` to satisfy the formatter. Made-with: Cursor
Both came in via a merge commit on this branch and aren't part of this PR's intent. getAdaptedStateFromPath's Plaid OAuth iOS redirect is already handled in main by a different code path, and the Mobile-Expensify submodule hash should track main. Restore both to origin/main. Made-with: Cursor
The test helper was passing Object.keys(mockSelectedTransactions) inline
as selectedTransactionsKeys on every render. Because our state-based
duplicate-handler slot updates state (rather than a ref) whenever
setDuplicateHandler is called, an unstable handleDuplicate identity
caused BulkDuplicateHandler's useEffect to re-fire after every render,
in turn triggering another state update and another render - an infinite
loop that CI reports as a Jest worker SIGTERM.
Memoize the keys with an empty-deps useMemo in the test helper; renderHook
spins up a fresh hook instance per test, so there's no cross-test
staleness.
Also decouple the duplicate option's visibility from whether the handler
has been registered yet: keep the option in the dropdown as soon as
isDuplicateOptionVisible is true, falling back to a no-op onSelected
when duplicateHandler is still null. This matches the pre-refactor
behavior where the ref defaulted to () => {} so the option was visible
before BulkDuplicateHandler's useEffect registered the real handler.
Made-with: Cursor
The empty-deps useMemo with a `// eslint-disable-next-line
react-hooks/exhaustive-deps` suppression triggers a React Compiler bail
("skipped optimizing this component because one or more React ESLint
rules were disabled"), which the compliance check flags as a regression
against main.
List mockSelectedTransactions explicitly in the deps array instead.
beforeEach reassigns the module-level `let`, which lets the useMemo
re-memoize across test boundaries; within a single render cycle it stays
stable, so handleDuplicate downstream is still stable. No lint
suppression, no compiler bail.
I confirmed locally that dropping the useMemo entirely and relying on
the React Compiler to auto-memoize doesn't work: the compiler can't
prove the module-level let is stable, so Object.keys(...) is treated as
a new expression each render and we get the same infinite render loop.
Made-with: Cursor
The dep was only there to satisfy the exhaustive-deps lint rule without a suppression (an empty deps array previously required one, which in turn tripped React Compiler's bail). The lint rule actually accepts an empty deps array here since the hook returns a stable value, and an empty deps array is semantically correct: renderHook tears down the hook between tests, so we never need the useMemo to recompute within a single instance. Made-with: Cursor
|
npm has a |
roryabraham
changed the title
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
…loop
On the money-request report header path, opening selection mode and
selecting multiple transactions reliably reproduced "Maximum update
depth exceeded". MoneyReportHeader carries a
react-hooks/exhaustive-deps suppression (around clearSelectedTransactions,
unrelated to this PR) that causes React Compiler to skip the whole
component. Without compiler memoization, the inline
`onAfterDuplicate={() => clearSelectedTransactions(true)}` prop to
BulkDuplicateHandler churns each render, which in turn churns
handleDuplicate inside useBulkDuplicateAction, which refires
BulkDuplicateHandler's onHandlerReady effect, which drives
useSelectedTransactionsActions' setDuplicateHandler state into a loop.
Stop handing the parent the raw handleDuplicate. Keep the latest handler
in a ref (updated in useInsertionEffect so callers always see the latest
at call time) and expose a useCallback-stable invoker to the parent via
onHandlerReady. The effect now depends only on that stable invoker and
runs once per mount, regardless of whether props from upstream
components are memoized or not.
Made-with: Cursor
Contributor
Author
|
Ugh, I kind of hit an impasse with this because fundamentally the data flow that was causing the Rule of React violations was wrong:
So here are my thoughts:
So what options do we have to address this? I see a few options, but of all of the options available, this seems the best/most correct... Move the consumer down, rather than the data up. Currently our mental model is something like: If Then But that's a big, big refactor that needs to start at the lowest level and work its way up. Fortunately, a decomposition of |
roryabraham
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
HOLD #79387 (comment)
Explanation of Change
Make
useSelectedTransactionsActionsanduseSearchBulkActionsReact Compiler-compliant, remove manual memoization, plus targeted perf and dead-code cleanup in the same areas.Fixed Issues
related to #68765
potentially related to #77171 (comment)
Tests
policyIDsWithVBBApath).areAllTransactionsFromSubmitterpath).useAllTransactionssearch-merge path).Offline tests
QA Steps
Existing BrowserStack regression cases (App project
PR-51):TC-3758Add Expenses to reportTC-3759Batch moving Expenses to another reportTC-3760Remove Expense from ReportTC-3819Splitting an expenseTC-3820Post-split actions - Revert SplitTC-3821Post-split actionsTC-3740Manual Submission Reports - Open state and Duplicate optionTC-5053Export multiple reports - Custom user CSV and Workspace CSV - Reports tabTC-5054Export multiple expenses - Custom user CSV and Workspace CSV - Reports tabTC-5057Export multiple expenses - Custom user CSV and Workspace CSV - Inbox tabTC-177533Export Report - Bulk Export to QBO and Manual Export StatesTC-135501Expense Merge - Cross Report Merging and Duplicate PreventionVerify that no errors appear in the JS console
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectioncanBeMissingparam foruseOnyxtoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari