Repository navigation
Guard search snapshot data reads that crash while a search is loading - #103937
Conversation
A new search's snapshot has only `search` until its results arrive, so `currentSearchResults?.data[...]` throws. Guard the remaining unguarded reads and cover the bulk actions case with a test. Co-authored-by: Kevin Brian Bader <[email protected]>
melvin-bot
Bot
added
Melvin-Test-Android
Melvin-Test-Web
|
I created this PR from an upstream branch because I couldn't find a fork of App under your account to push to. To take ownership of this branch and push updates, fork the repo and run: Then you can close this PR and open a new one from your fork. |
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
This comment has been minimized.
This comment has been minimized.
|
|
This comment has been minimized.
This comment has been minimized.
|
@ikevin127 Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
|
I updated the PR body and marked the PR ready for review.
|
Reviewer Checklist
Screenshots/VideosScreen.Recording.2026-10-13.at.21.07.30.mov |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f10366a8f5
ℹ️ 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".
| mockCurrentSearchResults = {search: {type: CONST.SEARCH.DATA_TYPES.EXPENSE_REPORT}}; | ||
|
|
||
| // When the bulk actions are created for an expense-report search | ||
| const {result} = renderHook(() => useSearchBulkActions({queryJSON: expenseReportQueryJSON}), {wrapper: OnyxListItemProvider}); |
There was a problem hiding this comment.
This renders only useSearchBulkActions, whose selectedMergeReports access already used currentSearchResults?.data?.[...] in the parent commit; it never executes MoneyRequestView, useMergeTransactions, or SearchMergeReports, which are the three production readers changed here. Consequently, this test passes with all three fixes reverted and cannot prevent this crash from regressing; add coverage that supplies a data-less snapshot to the changed call paths.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
@MelvinBot 🟡 Agree with Codex here. This test protects the selectedMergeReports line that #100674 already fixed, and that's still useful since #100674 shipped without a test.
But none of the 3 lines this PR changes run here, so reverting the fixes in MoneyRequestView, useMergeTransactions and SearchReportsMergeReports still passes. Can we add one case that hits a changed line? useMergeTransactions is the cheapest one: renderHook it with useSearchResultsContext mocked to return a snapshot that has only search, plus a mergeTransaction whose target ID isn't in the merge draft, so getTransaction reaches the snapshot read:
// Given a merge started from search while the new search's snapshot only has `search` (optimistic loading state)
mockCurrentSearchResults = {search: {type: CONST.SEARCH.DATA_TYPES.EXPENSE}};
// When the hook resolves the target transaction
const {result} = renderHook(() => useMergeTransactions({mergeTransaction: {targetTransactionID: '1', sourceTransactionID: '2'}}));
// Then it falls back to the Onyx transaction instead of throwing
expect(result.current.targetTransaction).toEqual(onyxTransaction1);There was a problem hiding this comment.
I added the case in c30a062, as a new file: tests/unit/hooks/useMergeTransactionsTest.ts.
It renders useMergeTransactions with a snapshot that has only search. The merge draft has no eligibleTransactions, so getTransaction reaches the snapshot read changed at useMergeTransactions.ts:59. It asserts that both target and source fall back to the Onyx transactions. If you revert the data?. fix, the read throws during render and the test fails.
It passed in CI (test (job 7)), along with typecheck, ESLint and Oxfmt.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
This request was already done in my earlier comment, and I made no new changes. I checked the PR body again just now:
|
Co-authored-by: Kevin Brian Bader <[email protected]>
ikevin127
left a comment
ikevin127
left a comment
There was a problem hiding this comment.
🟢 LGTM - Tests well, crash fixed (for now, this PR covers all unguarded snapshot reads, see below for details)
@mountiny All unguarded snapshot reads are fixed in this PR. The bigger problem is that SearchResults.data is typed as required, while the optimistic snapshot from a new search has no data until the response arrives.
That's why TypeScript never flagged any of these crashes, and a new currentSearchResults?.data[...] would typecheck and crash the same way.
I tried making data optional locally to see how big that change would be. It adds around 95 type errors in 8 files, not counting tests. Most of them are in SearchUIUtils, which passes data into many helpers as if it's always there.
Errors per file
| File | Errors |
|---|---|
src/libs/SearchUIUtils.ts |
81 |
src/libs/MergeTransactionUtils.ts |
8 |
src/hooks/useSearchAutoRefetch.ts |
2 |
src/components/Search/SearchResultsProvider.tsx |
1 |
src/components/Search/SearchList/ListItem/TransactionGroupListItem.tsx |
1 |
src/components/Search/hooks/useOptimisticSearchTracking.ts |
1 |
src/libs/actions/IOU/SplitTransactionUpdate.ts |
1 |
That's too much for this PR, so I'd keep it separate. Do you think it's worth opening a follow-up issue for the type change, or should we leave it as is ?
mountiny
left a comment
mountiny
left a comment
There was a problem hiding this comment.
Thanks! @ikevin127 what if we make it optional only in SearchResultsProvider currentSearchResults.data?
That is where these crashes came from right?
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚧 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! 🧪🧪
|
@mountiny Yes, all of them came through I tried it on this PR's head by typing They're all spots that pass the whole context object into something typed as
🟢 It also catches the crash pattern we care about: putting back the old One caveat: this only protects context consumers. Direct It's small enough for its own PR - I think it's worth a follow-up issue. |
|
🚀 Deployed to staging by https://github.com/mountiny in version: 9.5.13-0 🚀
|
|
No help site update is needed. This PR is a crash fix: it adds optional chaining to three search snapshot reads so the app doesn't crash while a search is loading, and it doesn't change any user-facing feature, label, or workflow. |
|
@ikevin127 @flaviadefaria For Android - Test2, when ticking one or more reports so the bulk actions button appears, we cannot proceed to run a search without clearing the selection. Could you please check if we missed anything? document_5084946931470305735.mp4 |
|
@MelvinBot how to complete steps from Test 2 on Android and iOS when ticking one or more reports so the bulk actions button appears, we cannot proceed to run a search without clearing the selection? |
|
You can't run Test 2 on Android or iOS phones. You didn't miss a step. Please mark Test 2 N/A on native phones and run it only on a wide layout: desktop web (Chrome or Safari), the desktop app, or a tablet in landscape. On a phone, ticking reports turns on selection mode. Selection mode replaces the search bar, type tabs, sort, and Filters with a "Select multiple" header and the bulk actions button. Every way out of selection mode (header back, Android back, switching tabs) clears the selection. So you can't reach Filters while reports stay selected. On wide layouts, use Filters (a new filter or date range) for step 3, not sort. Changing the sort clears the selection. Test 1 (the Onyx console step) is the reliable repro for this fix. Code references
|
|
🚀 Deployed to production by https://github.com/grgia in version: 9.5.13-6 🚀
Bundle Size Analysis (Sentry): |
5 participants
Explanation of Change
When a search starts for a new hash, its optimistic update creates the snapshot with only
searchanderrors.datastays missing until the response arrives, and stays missing if the request fails. AnycurrentSearchResults?.data[...]read during that window throwsTypeError: Cannot read properties of undefined, because?.only guardscurrentSearchResults. TypeScript doesn't flag it becauseSearchResults.datais typed as required.The main crash (Sentry APP-KT0,
selectedMergeReportsinuseSearchBulkActions) was already guarded onmainby #100674. As the C+ review asked, this PR fixes the remaining sibling reads with the same cause:MoneyRequestView.tsx: the parent report fallback (Sentry APP-C89).useMergeTransactions.ts:getTransaction.SearchReportsMergeReports.tsx: theisLoadingcheck.Each read now uses
currentSearchResults?.data?.[...], which matches every other snapshot read in these files.useMergeTransactions.tslines 85-88 were left unchanged. They already sit inside anif (currentSearchHash && currentSearchResults?.data)block, so they can't throw.This PR also adds a regression test to
tests/unit/hooks/useSearchBulkActionsTest.ts. It renders the hook with a selected report and a snapshot that hassearchbut nodata. Then it asserts the hook renders without throwing and doesn't offer Merge.Making
dataoptional on theSearchResultstype is the long-term guard, and is left for a separate cleanup.Fixed Issues
$ #103775
PROPOSAL: #103775 (comment)
Tests
Web (dev), where
window.Onyxis available in the console:type:expense-report) and tick one or more reports so the bulk actions button appears.datafrom the current snapshot and keep onlysearch:Offline tests
N/A
QA Steps
Test 1: Snapshot without
data(web, usingOnyxin the console)type:expense-report) and tick one or more reports so the bulk actions button appears.datafrom the current snapshot and keep onlysearch:Test 2: New search while reports are selected (all platforms, no console)
type:expense-report).PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectionAvatar, I verified the components usingAvatarare working as expected)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.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.AI Tests
npm run typecheck: passed fortsconfig.app.json,tsconfig.bun.json,tsconfig.node.jsonandserver/victory-chart-renderer. It failed on one error intests/actions/PolicyTest.ts(782,17)(TS1117, duplicate property), a file this PR doesn't touch. None of the changed files has a type error.tests/unit/hooks/useSearchBulkActionsTest.ts),oxfmtformatting, ESLint (out of memory with the default heap), and the React Compiler check (GITHUB_BASE_REFwas empty in this environment). CI covers these.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari