perf(Search): row-tap cascade kill via row-level live recompute + compiler-stable refs#91438
Conversation
…l action recompute
… lift bankAccountList to SearchContext
8c4aca1 to
0c40c99
Compare
…ite + selection dep from renderItem Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
… dead applySelectionToItem Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
# Conflicts: # src/components/Search/SearchSelectionProvider.tsx
|
|
Codecov Report❌ Looks like you've decreased code coverage for some files. Please write tests to increase, or at least maintain, the existing level of code coverage. See our documentation here for how to interpret this table.
|
# Conflicts: # src/components/Search/SearchList/ListItem/ChatListItem.tsx
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8d7b8334fd
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
- Derive expense-report row selection from child transactions, not the report key, so non-empty reports reflect selection (was always false). - Apply the same primaryActionExclusions as getSections in the live recompute via new getPrimaryAction, so the live Action button matches the snapshot (SUBMIT hidden for non-owners; PAY hidden as primary for non-reimbursable-only reports). - Read refreshed holdItem.canPay instead of stale snapshot allActions in the hold-menu nonHeldAmount math. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
| const liveActionsArray = liveReportActions ? (Object.values(liveReportActions) as ReportAction[]) : snapshotActions; | ||
| const liveAllActions = getActions( | ||
| snapshotData, | ||
| snapshotData as unknown as OnyxCollection<TransactionViolation[]>, |
There was a problem hiding this comment.
useLiveRowCapabilities.ts
Why it matters: snapshotData is SearchResults['data'] | undefined. Casting it through unknown to OnyxCollection<TransactionViolation[]> bypasses the type system entirely. If getActions ever changes its parameter shape, this will fail silently at runtime.
Suggested fix: Use a proper type guard or typed selector.
There was a problem hiding this comment.
Removed the cast entirely. Added a typed getViolationsFromSearchData helper in SearchUIUtils that narrows data to violation keys via the existing isViolationEntry guard — getActions now receives a properly-typed OnyxCollection<TransactionViolation[]>, no unknown hop.
| if (selectedTransactions[key]?.isSelected) { | ||
| selected += 1; | ||
| } | ||
| } |
There was a problem hiding this comment.
useSelectionCounts uses for...in without hasOwnProperty guard
Why it matters: for...in iterates over inherited enumerable properties. While selectedTransactions is likely a plain object today, any prototype pollution or future polyfill could leak into this count and show incorrect selection counts in the bulk actions bar.
Suggested fix:
for (const key in selectedTransactions) {
if (Object.prototype.hasOwnProperty.call(selectedTransactions, key) && selectedTransactions[key]?.isSelected) {
selected += 1;
}
}Or simpler, use Object.entries(selectedTransactions) which is safe and more idiomatic:
const selected = Object.entries(selectedTransactions).filter(([, value]) => value?.isSelected).length;There was a problem hiding this comment.
Done — switched to Object.values(selectedTransactions).filter((value) => value?.isSelected).length.
| * Submit/ChangeApprover. The four booleans stay as primitives at the equality | ||
| * guard so React Compiler keeps downstream consumers memoized. | ||
| */ | ||
| function useLiveRowCapabilities<T extends LiveRowItem>(params: UseLiveRowCapabilitiesParams<T>): T { |
There was a problem hiding this comment.
useLiveRowCapabilities creates N individual Onyx subscriptions per row
Each instance of useLiveRowCapabilities subscribes to:
${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${reportID}${ONYXKEYS.COLLECTION.REPORT_METADATA}${reportID}
For 100 visible rows, that's 200 additional Onyx subscriptions (vs. the previous single collection-level subscription at the screen level).
Why it matters: While the measurements look good for the tested scenario, react-native-onyx's per-key subscription overhead scales differently than a single collection subscription. On lower-end devices with large lists, this could cause memory pressure and jank when scrolling fast (JIT churn from subscribe/unsubscribe).
Recommended fix / mitigation: This is an architectural trade-off, but consider adding a JSDoc warning about the subscription count:
/**
* ⚠️ Creates 2 Onyx subscriptions per row. For large lists, consider
* capping the number of active subscriptions (e.g., via IntersectionObserver
* or windowed subscription).
*/There was a problem hiding this comment.
This is intentional — it replaces the screen-level collection merge that re-fired every visible row on any row tap (the regression this PR fixes). The list is virtualized, so subscriptions track on-screen rows, not the full result set; net result is the measured -20.5% commit duration. Added a JSDoc note capturing the trade-off and a windowing escape hatch if a future very large list shows churn.
📊 TestingThe PR adds:
Removed: 242-line Why it matters: There's no test verifying that selecting all transactions in a report marks the report row as selected, or that the reference-equality optimization in Suggested tests to add: // tests/components/Search/useLiveRowCapabilitiesTest.ts
describe('useLiveRowCapabilities', () => {
it('returns same reference when capabilities have not changed', () => {
// verify equality guard in useLiveRowCapabilities
});
it('updates action when reportActions Onyx key changes', () => {
// simulate Pusher update
});
});
// tests/components/Search/useRowSelectionTest.ts
describe('useRowSelection', () => {
it('marks row selected when areAllMatchingItemsSelected is true', () => {
// even if key is not in selectedTransactions
});
it('reads per-key state when areAllMatchingItemsSelected is false', () => {
// normal single-key selection
});
}); |
ikevin127
left a comment
There was a problem hiding this comment.
🟢 LGTM
Merge readiness: Conditional -> needs the type-safety cast fixed, for...in loop hardened, and test coverage for the new hooks before I'd feel confident shipping this. See comments above for details 🙌
|
No new product considerations - removing my assignment and unsubscribing. |
…matic count, hook tests - Replace unsafe `as unknown as OnyxCollection<TransactionViolation[]>` cast in useLiveRowCapabilities with a typed getViolationsFromSearchData helper that narrows snapshot data to violation keys via the existing isViolationEntry guard. - useSelectionCounts: swap for...in for Object.values(...).filter(...).length. - Document the intentional per-row Onyx subscription trade-off in the hook JSDoc. - Add tests for useRowSelection, useSelectionCounts, useLiveRowCapabilities, and getViolationsFromSearchData. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
@ikevin127 re: testing — added tests for all three new hooks plus the new helper:
|
Drop the now-unused exports flagged by knip: getAction and getRowCapabilities stay module-private in SearchUIUtils (internal call sites unchanged), and the orphaned selectFilteredReportActions helper in ReportUtils is deleted along with its export (its only consumer was the screen-level reportActions subscription this PR removed). Rewrite useRowSelectionTest harnesses with renderHook so they no longer mutate a captured outer-scope object inside a render component, which tripped the React Compiler "value cannot be modified" bailout on the new test file. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
ESLint no-unused-vars failed on CI for isApprovedAction, isDynamicExternalWorkflowApproveFailedAction, and isDynamicExternalWorkflowSubmitFailedAction in ReportUtils.ts. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
@ikevin127 can you recheck now please? |
|
@mountiny Yep, all comments were addressed and changes look good for merge 👍 |
mountiny
left a comment
There was a problem hiding this comment.
only nabs, can you please add those in follow ups?
| type LiveRowItem = { | ||
| action?: SearchTransactionAction; | ||
| canPay?: boolean; | ||
| canApprove?: boolean; | ||
| canSubmit?: boolean; | ||
| canChangeApprover?: boolean; | ||
| allActions?: SearchTransactionAction[]; | ||
| }; | ||
|
|
||
| type UseLiveRowCapabilitiesParams<T> = { | ||
| item: T; | ||
| reportID: string | undefined; | ||
| itemKey: string; | ||
| snapshotData: SearchResults['data'] | undefined; | ||
| snapshotActions: ReportAction[]; | ||
| enabled: boolean; | ||
| }; |
There was a problem hiding this comment.
docs, should we update the ai skill to make sure types always have docs?
| * (SUBMIT hidden for non-owners; PAY hidden as the primary action for non-reimbursable-only reports). | ||
| * Used by the row-level live recompute so the live Action button matches the snapshot. | ||
| */ | ||
| function getPrimaryAction(allActions: SearchTransactionAction[], data: OnyxTypes.SearchResults['data'], key: string, currentAccountID: number): SearchTransactionAction { |
There was a problem hiding this comment.
I think this is not unit tested
|
🚧 @mountiny has triggered a test Expensify/App build. You can view the workflow run here. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
🚀 Deployed to staging by https://github.com/mountiny in version: 9.4.0-0 🚀
Bundle Size Analysis (Sentry): |
|
🤖 No help site ( I reviewed the full diff against the help-site authoring guidelines, naming conventions, and template. This is a pure performance refactor of the Search screen's internal rendering and data-flow — it changes how the screen computes and renders, not what the user sees or does. Why no docs update is neededThe help articles document user-facing behavior (features, settings, tabs, buttons, workflows). This PR contains none of those:
Since user-facing behavior is unchanged, the existing help content remains accurate. If you believe a specific article should still be updated, reply with @adhorodyski, no linked help site PR was created because no documentation changes are required. If you disagree, let me know which behavior changed and I'll draft the update. |
|
🚀 Deployed to production by https://github.com/Julesssss in version: 9.4.0-7 🚀
|
Explanation of Change
The Search screen ran a parallel double-merge of live Onyx data:
getSectionspulled the snapshot plus ~18 live inputs (policies,cardFeeds,allTransactionViolations,allReportMetadata,reportActions,reportAttributesDerivedValue, …) while each row was already self-hydrating the same data via its own subscriptions. Row taps wrote optimistic primers that fed back into the screen's live deps, invalidating the 28-depuseMemo, re-creating the 24-deprenderItem, and reconciling every visible row in the tap frame — visible jank inSPAN_OPEN_REPORT(16–45ms).This PR removes the parallel merge and lets each row own its live computation:
reportActions,exportReportActions,allReportMetadata,reportAttributesDerivedValuefrom<Search>andgetSections.getSectionsbecomes snapshot-only for these inputs.allActionsfield on row items with four primitive booleans (canPay/canApprove/canSubmit/canChangeApprover) flowing through selection state and bulk-action consumers. Inlined as primitives at row level so the React Compiler tracks each as a value-dep.useLiveRowCapabilities— Row-level live recompute lives in a single hook consumed byExpenseReportListItem,TransactionGroupListItem,TransactionListItem. Wave C PRs edit the hook, not the three call sites.useRowSelection— Per-row selection read replacesapplySelectionToItem. The old screen-level selection dep onrenderItemis gone;applySelectionToItemis deleted.Measurements (React DevTools Profiler, 5 trials of warm row-tap, averaged):
PRD:
docs/superpowers/plans/2026-05-20-search-decomposition-prd.mdFixed Issues
$ #92080
PROPOSAL:
Tests
SPAN_OPEN_REPORTP90 below baselinenonHeldAmountmath reflects PAY availabilityOffline tests
QA Steps
Same as tests
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand 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