Prevent LHN crash on undefined item and out-of-range web scroll index#91488
Prevent LHN crash on undefined item and out-of-range web scroll index#91488wildan-m wants to merge 4 commits into
Conversation
…index Guard renderItem against a transient undefined item instead of reading item.reportID directly, and only restore the saved web scroll index when it is still within the current data range so FlashList is never asked to render a non-existent row.
…tions-list-undefined-item
Move the in-range scroll-index validation into a small pure helper so it can be unit-tested directly, and add tests covering the index validation (in-range, out-of-range, negative, empty list, no saved index) and the renderItem guard returning null for an undefined item.
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
@gijoe0295 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] |
There was a problem hiding this comment.
Remove this test. This is niche
| // FlashList can momentarily hand us an undefined item while the list shrinks before the | ||
| // recycler rebuilds. Bail out for that transient slot instead of reading item.reportID and | ||
| // crashing. The rest of this callback already guards item with optional chaining. |
There was a problem hiding this comment.
| // FlashList can momentarily hand us an undefined item while the list shrinks before the | |
| // recycler rebuilds. Bail out for that transient slot instead of reading item.reportID and | |
| // crashing. The rest of this callback already guards item with optional chaining. |
| const itemOneTransactionThreadReport = reports?.[`${ONYXKEYS.COLLECTION.REPORT}${itemReportAttributes?.oneTransactionThreadReportID}`]; | ||
|
|
||
| let invoiceReceiverPolicyID = '-1'; | ||
| if (item?.invoiceReceiver && 'policyID' in item.invoiceReceiver) { |
There was a problem hiding this comment.
| if (item.invoiceReceiver && 'policyID' in item.invoiceReceiver) { |
| invoiceReceiverPolicyID = itemParentReport.invoiceReceiver.policyID; | ||
| } | ||
| const itemInvoiceReceiverPolicy = policy?.[`${ONYXKEYS.COLLECTION.POLICY}${invoiceReceiverPolicyID}`]; | ||
| const itemPolicy = policy?.[`${ONYXKEYS.COLLECTION.POLICY}${item?.policyID}`]; |
There was a problem hiding this comment.
| const itemPolicy = policy?.[`${ONYXKEYS.COLLECTION.POLICY}${item.policyID}`]; |
| }); | ||
| }, [getScrollOffset, route]); | ||
|
|
||
| const initialScrollIndex = getInitialScrollIndex(isWeb ? getScrollIndex(route) : undefined, data.length); |
There was a problem hiding this comment.
| const initialScrollIndex = getInitialScrollIndex(isWeb ? getScrollIndex(route) : undefined, data.length); | |
| const savedScrollIndex = getScrollIndex(route); | |
| const initialScrollIndex = isWeb && savedScrollIndex && savedScrollIndex >= 0 && savedScrollIndex < data.length ? savedScrollIndex : undefined; |
| /** | ||
| * Restores the saved web scroll index only when it is still in range. The list length changes | ||
| * constantly (archived/deleted reports, filters, priority mode, account switches), so a saved index | ||
| * can point past the end. Returning undefined makes FlashList start at the top instead of looking up | ||
| * data[index] === undefined and handing renderItem a missing item. | ||
| */ | ||
| function getInitialScrollIndex(savedScrollIndex: number | undefined, dataLength: number): number | undefined { | ||
| if (savedScrollIndex === undefined || savedScrollIndex < 0 || savedScrollIndex >= dataLength) { | ||
| return undefined; | ||
| } | ||
| return savedScrollIndex; | ||
| } | ||
|
|
There was a problem hiding this comment.
| /** | |
| * Restores the saved web scroll index only when it is still in range. The list length changes | |
| * constantly (archived/deleted reports, filters, priority mode, account switches), so a saved index | |
| * can point past the end. Returning undefined makes FlashList start at the top instead of looking up | |
| * data[index] === undefined and handing renderItem a missing item. | |
| */ | |
| function getInitialScrollIndex(savedScrollIndex: number | undefined, dataLength: number): number | undefined { | |
| if (savedScrollIndex === undefined || savedScrollIndex < 0 || savedScrollIndex >= dataLength) { | |
| return undefined; | |
| } | |
| return savedScrollIndex; | |
| } |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppScreen.Recording.2026-05-26.at.01.07.18.movAndroid: mWeb ChromeScreen.Recording.2026-05-26.at.01.11.47.moviOS: HybridAppScreen.Recording.2026-05-26.at.01.14.16.moviOS: mWeb SafariMacOS: Chrome / SafariScreen.Recording.2026-05-26.at.01.02.06.mov |
|
Let's remove the redundant wordy explanation from PR body. The |
Explanation of Change
On web, the app could crash with a
TypeErrorwhile rendering the left-hand navigation list. The list's row renderer read a report's ID as its very first step, without checking that the row actually existed.This was reachable because, on web only, the list restores a previously saved scroll position when it mounts. That position is never re-validated against the current list, which grows and shrinks constantly (reports archived or deleted, filters applied, priority mode toggled, accounts switched). Once the list had shrunk below the saved position, the list component looked up a row that no longer existed and passed an empty row to the renderer, which crashed reading the report's ID. On native the saved position is not used, which is why the crash was web-only.
This fixes both layers. The row renderer now returns an empty row when it has no data, instead of reading the report's ID — consistent with the rest of that callback, which already treats the row as possibly missing. And the saved web scroll position is now used only when it still falls within the current list; otherwise the list starts at the top. The second change removes the actual trigger, and the first guarantees the crash cannot happen regardless of how an empty row arrives.
The only behavioral change is in the out-of-range case: when the saved web scroll position no longer fits the list, the navigation starts at the top instead of crashing. When the saved position is still valid, behavior is unchanged.
Fixed Issues
$ #91226
PROPOSAL: #91226 (comment)
Tests
This is a non-deterministic crash reported through Sentry — the issue lists reproduction as "Unknown" (it happens during a concurrent-rendering recovery race when the left-hand navigation (LHN) list shrinks below a previously saved web scroll index, so it can't be triggered by normal use). The patch below forces the underlying out-of-range-index condition deterministically, on any account — no need for many chats.
Deterministic reproduction (any account size)
The crash needs the saved web scroll index to point past the end of the current list. You can't naturally save a high index on a short list, so temporarily force
getScrollIndexto return an out-of-range value. This file is not modified by this PR, so the exact same patch applies to bothmainand this branch.In
src/components/ScrollOffsetContextProvider.tsx, replace the body ofgetScrollIndex:mainwith the patch applied: open the app on web and sign in. The LHN crashes into the "Uh-oh, something went wrong!" error boundary on load. The JS console showscrash caught by error boundary - index out of bounds, not enough layoutswith a component stack throughLHNOptionsList. (This is the deterministic, mount-time sibling of the productiont.reportIDrace — same root cause: an out-of-range index reaching FlashList.)Regression checks (real fix in place, no patch)
npx jest tests/ui/components/LHNOptionsListTest.tsx tests/ui/components/LHNOptionsListRenderItemTest.tsx. They cover the scroll-index validation (in range, past the end, negative, empty list, no saved index) and the guard that returns an empty row for a missing item.Offline tests
No offline-specific behavior changes. The LHN renders the same offline; the guards only change what happens when the list shrinks (which can also occur offline during an optimistic delete) — preventing the crash and starting at the top when the saved scroll position is out of range.
QA Steps
This crash can't be reproduced on demand — it's a non-deterministic race, and the deterministic repro in the Tests section needs a local code patch that doesn't apply on staging. QA can confirm there's no regression in the left-hand navigation (LHN). This works on any account and does not depend on how many chats it has:
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
Kapture.2026-05-23.at.13.25.23.mp4
Android: mWeb Chrome
Kapture.2026-05-23.at.13.29.33.mp4
iOS: Native
Kapture.2026-05-23.at.13.17.23.mp4
iOS: mWeb Safari
Kapture.2026-05-23.at.13.20.53.mp4
MacOS: Chrome / Safari
Kapture.2026-05-23.at.11.47.49_compressed.mp4