fix(layout): avoid binary search for masonry visible item detection#2207
fix(layout): avoid binary search for masonry visible item detection#2207shopify-github-actions-access[bot] wants to merge 3 commits intomainfrom
Conversation
The base class getVisibleLayouts uses binary search which assumes items are sorted by y-position. In masonry layout, items are placed in columns so y-positions are NOT monotonically sorted by index. This caused items to disappear during scrolling, especially with optimizeItemArrangement disabled where column height divergence is more pronounced. Override getVisibleLayouts in MasonryLayoutManager to scan all items and correctly identify which ones overlap with the viewport, regardless of their index ordering. Fixes #2103
|
@agent The linear scan might be too slow. Can we not degrade performance? FIx this and also test and fix lint. |
|
Claude finished @naqvitalha's task in 4m 24s —— View job Optimized masonry visible item detection
What changed: Replaced the O(n) linear scan in
Performance: O(numColumns × log(n/numColumns)) per scroll event instead of O(n). For a typical 2-column masonry with 10k items, this is ~28 comparisons vs 10,000. Why it works: Within each masonry column, y-positions are monotonically non-decreasing (column heights only increase during placement), so binary search is valid per-column even though the global index order is unsorted. All 185 tests pass, type-check and lint clean. |
…ction Replace O(n) linear scan with per-column binary search for O(numColumns * log(n/numColumns)) performance. Within each masonry column, y-positions are monotonically sorted, so binary search is valid. Column-to-item mappings are built during recomputeLayouts (which already iterates all items) so the visibility hot path stays fast during scrolling. Co-authored-by: Talha Naqvi <naqvitalha@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR fixes Masonry layout visibility detection where the base RVLayoutManager.getVisibleLayouts uses a binary search that assumes layouts are ordered by y-position, which is not true for masonry columns—causing items to disappear while scrolling (Issue #2103).
Changes:
- Adds a
RVMasonryLayoutManagerImpl.getVisibleLayoutsoverride that finds visible items via per-column visibility search (instead of a single global binary search). - Introduces
columnItemsand rebuilds per-column item mappings after layout recomputation. - Adds unit tests covering the unsorted-y-by-index masonry visibility scenario.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
src/recyclerview/layout-managers/MasonryLayoutManager.ts |
Adds per-column visibility detection and column-to-item index mappings. |
src/__tests__/MasonryLayoutManager.test.ts |
Adds tests ensuring visible items aren’t missed when masonry y-positions are unsorted by index. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const mid = (lo + hi) >>> 1; | ||
| const layout = this.layouts[items[mid]]; | ||
| if (layout.y < unboundDimensionEnd) { | ||
| colLast = mid; | ||
| lo = mid + 1; | ||
| } else { | ||
| hi = mid - 1; |
There was a problem hiding this comment.
Visibility boundary semantics here differ from the base RVLayoutManager implementation (findLastVisibleIndex treats position <= unboundDimensionEnd as visible). Using layout.y < unboundDimensionEnd may exclude items that start exactly at unboundDimensionEnd, potentially changing engaged/visible index behavior compared to other layout managers. Align this comparison with the base implementation to keep consistent edge handling across layout managers.
| /** | ||
| * Override getVisibleLayouts to use per-column binary search instead of | ||
| * a single binary search on the full layouts array. In masonry layout, | ||
| * y-positions are NOT sorted by index (items are placed in columns), but | ||
| * they ARE sorted within each column. Binary searching each column gives | ||
| * O(numColumns * log(n/numColumns)) performance. | ||
| */ |
There was a problem hiding this comment.
The PR title/description says masonry visibility detection is fixed by “avoiding binary search” / doing a “linear scan”, but this implementation still uses binary search (per-column). Either update the PR description/title to match what’s implemented, or adjust the implementation/inline comment so they’re consistent.
| getVisibleLayouts( | ||
| unboundDimensionStart: number, | ||
| unboundDimensionEnd: number | ||
| ): ConsecutiveNumbers { | ||
| if (this.layouts.length === 0) { | ||
| return ConsecutiveNumbers.EMPTY; | ||
| } | ||
|
|
||
| let firstVisibleIndex = -1; | ||
| let lastVisibleIndex = -1; | ||
|
|
||
| for (const items of this.columnItems) { | ||
| if (items.length === 0) continue; |
There was a problem hiding this comment.
getVisibleLayouts relies on this.columnItems, but columnItems is only rebuilt in recomputeLayouts(). When a RVMasonryLayoutManagerImpl is constructed with a previousLayoutManager (see RecyclerViewManager.updateLayoutParams passing the old manager into the new one), this.layouts can be non-empty while columnItems is still [], causing this method to incorrectly return EMPTY and potentially render nothing. Consider lazily calling rebuildColumnItems() when columnItems.length !== this.maxColumns (or when the total count across columns doesn’t match this.layouts.length), and/or rebuilding once in the constructor when this.layouts.length > 0.
Description
In masonry layout, the base class
getVisibleLayoutsuses binary search to find visible items, which assumes y-positions are sorted by index. However, masonry places items in columns, so y-positions are NOT monotonically sorted by index — items in a shorter column can have much lower y-positions than earlier items in a taller column. This causes the binary search to miss visible items, making them disappear during scrolling.The fix overrides
getVisibleLayoutsinMasonryLayoutManagerto scan all items and correctly detect viewport overlap, regardless of index ordering. This is especially noticeable withoptimizeItemArrangement={false}where column height divergence is more pronounced.Fixes #2103
Reviewers' hat-rack 🎩
MasonryLayoutManager.getVisibleLayoutsuses a linear scan instead of binary search. This is correct because masonry y-positions are unsorted by index. For typical masonry lists (hundreds to low thousands of items), this has negligible performance impact.Test plan
yarn test)yarn type-check)