fix(layout): invalidate cached row heights on in-place data swap#2287
Open
pranko17 wants to merge 1 commit into
Open
fix(layout): invalidate cached row heights on in-place data swap#2287pranko17 wants to merge 1 commit into
pranko17 wants to merge 1 commit into
Conversation
When the `data` prop is replaced with a different array of items (e.g. filter / sort / category change) without remounting the list, indices that were already measured retain their `isHeightMeasured`, `height` and `minHeight` from the previous data. The ViewHolder for the new item at that position then renders against the stale `minHeight`, and the LayoutManager places the next row at a Y derived from the stale `height`. In multi-column grids this surfaces as a taller new card overflowing into the row below. Track per-index `keyExtractor` results across renders. In `processDataUpdate`, clear `isHeightMeasured` and `minHeight` for any index whose key changed, so the next `modifyLayout` pass recomputes the row against the new content. No-op when `keyExtractor` is not provided. Tests cover: per-index identity change, full array replacement, re-emit of identical keys, append (pagination), and absence of `keyExtractor`.
2548d0f to
406e17e
Compare
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
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.
Description
When the
dataprop of aFlashListis replaced in place — that is, the list itself is not unmounted but the underlying array is swapped for a different one (e.g. on category / filter / sort change) — items in a multi-column grid can overlap with the row below them.The screenshot below is a real reproduction in a production app on
@shopify/flash-list@2.3.1withnumColumns={2}. After switching the category, the new product cards in row N are taller than the cards that were previously at those positions, and they overlap into row N+1:Root cause
RecyclerViewManager.processDataUpdateis called fromuseRecyclerViewManager'suseMemo([data]). Its current implementation forwards an emptylayoutInfoarray tomodifyChildrenLayout, so on an in-place data swap theLayoutManageris told that the data length changed but nothing else:The per-index entries in
LayoutManager.layoutstherefore keep their previous values:isHeightMeasured: true(set after the previous render'sonLayout)height(the previously measured pixel height)minHeight(the row's normalized height set byprocessAndReturnTallestItemInRow)On the next render the new item at that index is mounted by the
ViewHolderwith the staleminHeightapplied as a style. Two things go wrong:ViewHolderdoes not enforceheightfor non-grid items, so the new content stretches the cell past its cachedheight. The row above looks taller than the layout says it is.recomputeLayoutsplaces the next row'syfrom the cachedheightof the current row before any fresh measurement arrives. The next row therefore starts too high, and the overflowing cell of the previous row visually overlaps it.There is also a second-order effect inside
GridLayoutManager.processAndReturnTallestItemInRow: a row's "tallest item" is only nominated iflayout.height > layout.minHeight. After the previous render's normalization, the previously tallest item hasminHeight = 0while its neighbours haveminHeight === height. When the new data is taller at a previously short position, the comparisonheight > minHeightfails for the previously normalized neighbour, so it cannot be picked as the tallest. The row's normalized height ends up driven by the wrong item, and one further frame is required before the layout converges — sometimes only after the user scrolls the affected rows back into view.The library already has
clearLayoutCacheOnUpdate()for the heavy case (e.g. orientation change), but that wipes the entire cache and the docs say it should be called manually before render, which doesn't fit in-place data updates.The fix
Track each render's
keyExtractorresults per index. WhenprocessDataUpdateruns and the layout manager already exists, diff the new keys against the previously recorded ones; for any index whose key changed, clearisHeightMeasuredandminHeighton its layout. The next timemodifyLayoutreceives real measurements from the freshly renderedViewHolders,computeEstimatesAndMinMaxChangedLayoutsees!isHeightMeasuredand triggers a fresh recompute of those rows. No row is positioned against a stale height for the new content.The patch is entirely additive on a private code path and is a no-op when
keyExtractoris not provided, so it cannot regress existing callers.What's covered
keyExtractorprovidedminHeightReviewers' hat-rack 🎩
RecyclerViewManager.invalidateChangedLayoutsonly mutates layouts whose index is in[0, min(layoutCount, newLength)). Indices that exist only in the previous data (i.e., the array shrunk) are trimmed insidemodifyLayoutas before. Indices that exist only in the new data are created bymodifyLayoutafter this call and are not touched.recordCurrentDataKeysis called from both branches ofprocessDataUpdateand from the early-return insideinvalidateChangedLayouts, soprevDataKeysstays in sync with the most recently rendered data even when the layout manager isn't ready yet (first paint).keyExtractoris undefined,prevDataKeysis explicitly truncated to zero length so a later flip to providing akeyExtractordoesn't reuse stale keys from an earlier session.Tests
src/__tests__/RecyclerViewManager.test.tsadds five new cases underprocessDataUpdate — layout invalidation on in-place data swap. Full suite (yarn jest) — 188/188 passing locally.yarn lintandyarn type-checkclean.Screenshots or videos
Repro is a 2-column product list with cards of varying height. Switching categories swaps the underlying
datain place. Before the fix, the first frame after the swap shows cards from the new category overlapping the row below; after the fix, the next row is positioned against the fresh measurement and there is no overlap.