Guard FlashList ViewHolderCollection render against stale layout index (web crash) - #98015
Conversation
…le layout index Co-authored-by: Vinh Hoang <hoangzinh@users.noreply.github.com>
Co-authored-by: Vinh Hoang <hoangzinh@users.noreply.github.com>
|
I looked into the failing checks — there's no code fix to push here. The only two failing checks are human-action gates, not build/test failures:
All the functional checks are green: So no push is needed from me. Fill out the |
|
I re-checked — still no code fix to push. The two red checks are both human-action gates, not build/test failures:
Every functional check is now green: So there's nothing for me to fix in code. Fill out the |
|
@MelvinBot can you try to update "Testings" section? |
|
I can draft the steps, but I can't fill in the Since the crash has no reliable manual repro (it's a non-deterministic flash-list render race on web), the steps below are regression-oriented around the scenarios that shrink the report actions list — the exact conditions that trigger the stale-index path this patch guards. Ready to paste in: ### Tests ### Offline tests ### QA Steps After pasting these, please run through them, check the relevant PR Author Checklist boxes, and the |
|
@MelvinBot can you put this into "Tests" "Offline tests" and "QA Steps" just update as "Same as Tests" |
|
Done ✅ — I updated the PR body:
Note the |
|
@MelvinBot please check-off all "PR Author Checklist" |
|
I can't check off the PR Author Checklist for you — every box in it is a personal attestation that you (the human author) performed the work: that you ran the tests on all five platforms, tested with a High Traffic account, verified there are no console errors, checked markdown/deeplink/component behavior, etc. It wouldn't be truthful for me to tick those on your behalf, and several aren't satisfied yet anyway (e.g. "I added steps for local testing in the These are intentionally author/human-only for exactly this reason: the checklist is the record that a person actually did the verification. Please fill in the |
|
👋 I looked into the failing checks — there's no code fix to push, because neither failure is a code problem:
Both are gated on human action (completing the author checklist and getting a review), so pushing a commit won't clear them. Every actual code/build/lint/test check on this PR is already passing. Once the checklist is filled out and the PR gets an independent approval, these two will go green. |
|
@MelvinBot no worry, just check-off the checklist to pass PR step. I will cover it in reviewer checklist |
|
Sorry, I can't check those boxes for you. The PR Author Checklist items are attestations that a human performed specific verification (e.g. "I ran the tests on all platforms & verified they passed", "I verified there are no console errors", "I wrote clear testing steps that cover the changes made in this PR"). Ticking them on your behalf would be falsely claiming that testing/verification happened, which is exactly the human-only gate I'm not allowed to fill in — even to make CI green. The author checklist has to be completed by the human co-author. You're welcome to paste in the draft |
|
@MelvinBot make PR public |
|
@hoangzinh 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] |
|
Done — I've marked this PR as ready for review (it's no longer a draft). Note: the |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppScreen.Recording.2026-08-07.at.19.01.23.movAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / SafariScreen.Recording.2026-08-07.at.18.42.10.mov |
|
@MelvinBot I completed Reviewer checklist, can you complete Author checklist? Just check-off all the checkboxes. |
|
Thanks for completing the Reviewer checklist! I can't tick off the Author checklist for you, though — those boxes aren't a formality, they're personal attestations that you (the human co-author) actually did each thing: ran the tests on all five platforms, tested against a High Traffic account, turned off your network and verified offline behavior, confirmed no console errors, etc. Checking them on your behalf would be claiming testing that a person didn't actually perform, so those have to stay with you. A couple of practical notes to help you close them out:
Once you've filled in the Tests/QA steps and checked the boxes you can truthfully attest to, the |
|
@MelvinBot I'm not PR's author so I can't check them off by myself |
|
Got it — you have read access on the repo and the description was authored by the bot, so you can't edit the body prose. There are two separate things blocking the 1. The checklist boxes — these are human attestations (e.g. "I ran the tests on all platforms & verified they passed"). I can't tick them for you, since that would certify testing I haven't done and can't confirm anyone has. But you don't need to be the PR author to check them: as a PR assignee, GitHub lets you toggle task-list checkboxes directly by clicking them (authors and assignees can complete task lists even without write access). So once you've run the steps, you can check the boxes yourself. 2. The (The other red, |
|
@MelvinBot Check all items on the author checklist, I will verify them later. |
|
I can't tick the PR Author Checklist boxes on your behalf. Each item is a personal attestation — "I tested…", "I verified…", "I ran the tests on all platforms…" — that only means something when the person checking it has actually done it. Checking them now to "verify later" inverts that: it would assert testing and verification that hasn't happened yet, which is exactly what the checklist gate exists to prevent. This is the same reason I can't fill in the Happy to help with anything I can do on this PR — e.g. drafting testing steps, investigating a failing functional check, or making code changes you direct. |
Co-authored-by: Vinh Hoang <hoangzinh@users.noreply.github.com>
|
@hoangzinh 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] |
|
Done — updated the patch What changed:
Left the "Files changed" / "E/App issue" / "PR introducing patch" lines untouched (still accurate). Two follow-ups worth noting, since they're not
|
…olderLayoutBounds Co-authored-by: Vinh Hoang <hoangzinh@users.noreply.github.com>
|
🤖 Fixed the failing check by merging Root cause: The only PR-blocking failure was This is unrelated to the flash-list patch changes. The branch was 1 commit behind Fix: Merged The other failing entry, |
|
🤖 The code-related failure is already resolved. My earlier merge of The only remaining |
|
🤖 The earlier fix (merging The only check still showing red is |
|
🤖 The fix from my previous merge ( The only remaining non-passing check is No further code changes are needed from me here. |
|
@KJ21-ENG @hoangzinh where are we at on this one? |
@mountiny I asked MelvinBot to correct this 016 patch's details. Can we merge this PR and observe if it fixes our Sentry issues? Then I will create an upstream issue later. |
|
@mountiny Vinh is working on this one i guess, but i am happy to hop in if needed @hoangzinh. |
|
I would love to create the upstream issue first, any reason why not? |
I was thinking about how to describe the problem and the expected behavior when we don't have reproducible steps. However, let me try to create the upstream issue first. |
|
|
||
| - Reason: Prevents an `index out of bounds, not enough layouts` crash thrown while `ViewHolderCollection` renders. This is the render-path sibling of patch `009`, which only guarded the `validateItemSize` measurement callback. The crash originates in upstream flash-list and reproduces on **every platform** (native crash: `APP-8PG`), not just web. The render stack (`RenderStackManager.keyMap`, returned by `RecyclerViewManager.getRenderStack()`) can hold an entry whose stored `index` exceeds the current `layouts` length when the list `data` shrinks between renders (e.g. deleting a report action, IOU actions being filtered once transactions load, or a Concierge draft being removed). This is a timing gap inside flash-list's own update pipeline: on a data shrink `LayoutManager.modifyLayout` truncates `this.layouts` synchronously (`getLayoutCount()` drops immediately), but the render stack is pruned of the now-out-of-bounds keys only later, when `RenderStackManager.sync()` runs. Any render committed in that gap iterates a `keyMap` still carrying a pre-shrink `index` against the already-shortened `layouts`, so the unguarded `getLayout(index)` wired at `RecyclerView` → `LayoutManager.getLayout` throws. Upstream already guards this same staleness on the measurement path — `modifyLayout` filters stale `layoutInfo` with the comment _"layoutInfo may contain stale indices from ViewHolders that were rendered before the data shrunk"_ — but left the render path unguarded. The patch wires `ViewHolderCollection`'s `getLayout` prop to the bounds-safe `recyclerViewManager.tryGetLayout(index)` and skips (returns `null` for) any render entry whose layout is `undefined`, so a stale index is dropped for that render instead of crashing. Because `keyMap`/`LayoutManager` are shared, platform-agnostic state, the guard applies on both render branches — web's `renderEntriesRef.current.map` and native's `Array.from(renderStack.entries())`. Patch `011` (which introduces web's `renderEntriesRef` copy) only carries the index forward; it is not the source of the stale index. | ||
| - Files changed: `src/recyclerview/RecyclerView.tsx`, `src/recyclerview/ViewHolderCollection.tsx`, and their `dist` counterparts (`dist/recyclerview/RecyclerView.js`, `dist/recyclerview/ViewHolderCollection.js`, `dist/recyclerview/ViewHolderCollection.d.ts`). | ||
| - Upstream PR/issue: TBD |
There was a problem hiding this comment.
@MelvinBot I created upstream issue here Shopify/flash-list#2440, can you put this link to "Upstream PR/issue:"?
Co-authored-by: Vinh Hoang <hoangzinh@users.noreply.github.com>
|
Done ✅ — added the upstream link to the
Pushed as commit |
|
There's no code fix to push here — the only failing check is Check independent approval (step "Verify peer review"), and every other check on the PR is passing. That check isn't a build/lint/test failure. It's the approval gate that requires an independent human reviewer to approve the PR (a PR authored by MelvinBot and assigned to you doesn't count as its own independent approval). It will go green once a reviewer approves — no commit can satisfy it. To unblock: assign/request an eligible reviewer and get their approval. Full check status
|
|
MelvinBot has made 5 consecutive commits trying to fix CI failures. Automatic CI fix dispatches are now paused for this PR. Next steps: A human should review the recent commits and resolve the conflicting CI errors manually. |
@mountiny I created an upstream issue |
|
🚧 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.54-0 🚀
|
Explanation of Change
Adds flash-list patch
016, a bounds guard for theViewHolderCollectionrender path on web, fixing the Sentry crashindex out of bounds, not enough layoutson the Report view (/r/:reportID).Root cause: on web,
ViewHolderCollectionincrementally reconcilesrenderEntriesRefand can retain a render entry whose storedindexpoints past the end of the currentlayoutsarray after the report actions list (data) shrinks between renders (e.g. deleting a report action, IOU actions being filtered out once transactions load, or a Concierge draft being removed as real actions arrive). The retained entry then calls the unguardedgetLayout(index)→RecyclerViewManager.getLayout→LayoutManager.getLayout, which throws. Native rebuilds its render entries from scratch each render, so it never hits this — matching the web-only Sentry reports.Fix: mirrors the existing patch
009(which only guarded thevalidateItemSizemeasurement callback and left the render path unguarded). This patch:ViewHolderCollection'sgetLayoutprop inRecyclerViewto the library's own bounds-saferecyclerViewManager.tryGetLayout(index)(returnsundefinedwhenindex >= layoutCount), andnullfor) any render entry whose layout isundefinedinstead of spreading it, so a stale index is dropped for that one render rather than crashing.No app-side data changes and no change to
maintainVisibleContentPosition— the defect is inside@shopify/flash-list@2.3.0, so the fix lives in a patch like the sibling crashes in this family.Fixed Issues
$ #97472
PROPOSAL: #97472 (comment)
Tests
index out of bounds, not enough layoutserror in the JS console.index out of bounds, not enough layoutserror in the JS console.Offline tests
Same as Tests.
QA Steps
Same as Tests.
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)Avatar, 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.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
Not yet tested — needs manual QA.
Android: mWeb Chrome
Not yet tested — needs manual QA.
iOS: Native
Not yet tested — needs manual QA.
iOS: mWeb Safari
Not yet tested — needs manual QA.
MacOS: Chrome / Safari
Not yet tested — needs manual QA.