Skip to content

fix: eliminate PieceList flash on pagination#735

Merged
shaoster merged 3 commits into
mainfrom
issue/734-piecelist-pagination-flash
May 28, 2026
Merged

fix: eliminate PieceList flash on pagination#735
shaoster merged 3 commits into
mainfrom
issue/734-piecelist-pagination-flash

Conversation

@shaoster
Copy link
Copy Markdown
Owner

Problem

Fixes #734. On the PieceList page with 17+ pieces, a dark flash appears immediately after the first 16 cards render. Two bugs compound to cause it.

Fix

Bug 1 — Sentinel fires before masonry renders (PieceList.tsx):

The scroll sentinel useEffect called check() immediately when hasMore became true. At that moment masonryWidth=0 (the ResizeObserver hasn't fired yet), so the sentinel sits at top≈0 and onLoadMore fires before any cards are visible — triggering the second page fetch while the masonry is still empty.

Fix: add masonryWidth to the effect's dependency list and return early when masonryWidth === 0. The sentinel check is deferred until the masonry has rendered its cards and the sentinel is at its true document position.

Bug 2 — Full positioner reset on every append (PieceList.tsx):

The positioner was computed via useMemo with filteredPieces as a dependency. Every time the second page arrived and filteredPieces grew 16→32, createPositioner was called from scratch — discarding all cached positions — and MasonryScroller re-laid out all items from zero. That re-layout is the flash.

Fix: replace createPositioner + useMemo with masonic's usePositioner hook. usePositioner only resets when its deps array (sortOrder, active filters, active tags) or layout options (masonryWidth, columnWidth, isMobile) change — not when items are appended. Crop heights are seeded inline with a positioner.get(index) === undefined guard so only new unpositioned items are seeded; existing positions survive pagination.

Also removes the now-unused getMasonryColumns helper (its logic is handled internally by usePositioner).

Regression Tests

Two tests added to web/src/components/__tests__/PieceList.test.tsx:

  • scroll sentinel > does not call onLoadMore before the masonry container has a measured width — renders with masonryWidth=0 and hasMore=true; asserts onLoadMore is never called. Failed before the fix (Bug 1).

  • masonry height pre-seeding > does not re-seed existing crop heights when pieces are appended via pagination — renders with a cropped piece, appends a second piece, asserts positioner.set is still called exactly once (index 0 not re-seeded). Failed before the fix (Bug 2).

The beforeEach mock for mockPositioner.get was updated to be stateful (returns the value written by set), matching the real positioner's behaviour and enabling the "does not reseed on unrelated rerender" test to remain valid.

Verification

rtk bazel test //web:web_piece_list_test --test_output=all  # 53 tests pass
rtk bazel test //web:web_test --test_output=errors          # 37 test targets pass

Closes #734

shaoster and others added 3 commits May 28, 2026 17:26
Bug 1: the scroll sentinel effect called check() immediately when hasMore
became true, but masonryWidth=0 at that point so the sentinel was at top≈0
and onLoadMore fired before any cards were visible. Fix: add masonryWidth to
the effect deps and return early when masonryWidth === 0.

Bug 2: the positioner useMemo depended on filteredPieces, so every pagination
append called createPositioner from scratch and MasonryScroller re-laid out
all items from zero — the flash. Fix: switch to usePositioner with sort/filter
keys as deps so the positioner is reused on append and only reset on sort or
filter changes. Crop heights are seeded inline with a get(index) === undefined
guard so existing positions survive.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
A new piece prepended via handleCreated shifts all existing item indices.
Without a reset, the positioner reuses stale index→height mappings, causing
masonic to size the prepended item incorrectly.

Fix: track whether filteredPieces is a pure suffix-append of the previous
render. Any non-append mutation (prepend, sort, filter) increments
positionerResetCounterRef, which is the sole dep passed to usePositioner.
A changed dep clears the seeded-heights cache and creates a fresh positioner.
Pagination appends leave the counter unchanged so their positions survive.

Updates the usePositioner test mock to simulate dep-based resets by clearing
the shared seededMap when deps change, enabling the new regression test to
distinguish "reset happened" from "reuse happened".

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The previous approach read and wrote useRef.current during render to detect
non-pure-appends, which violates the react-hooks/refs lint rule (refs must
only be accessed outside render).

Rework: PieceListPage, which already knows when handleCreated performs a
prepend, increments a positionerResetKey state counter and passes it to
PieceList as a prop. PieceList includes it alongside sort/filter keys in
the usePositioner deps array. No ref reads during render, and the parent
explicitly signals the reset rather than inferring it from item order.

Update the prepend regression test to mirror this contract: the harness
increments resetKey alongside the prepend, exactly as the real parent does.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@shaoster shaoster merged commit c7f70d8 into main May 28, 2026
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix: PieceList flashes on pagination as positioner is recreated from scratch

1 participant