diff --git a/web/src/components/PieceList.tsx b/web/src/components/PieceList.tsx index f9f55167..b7f0ce4c 100644 --- a/web/src/components/PieceList.tsx +++ b/web/src/components/PieceList.tsx @@ -24,8 +24,8 @@ import type { PieceSortOrder } from "../util/api"; import { DEFAULT_PIECE_SORT, PIECE_SORT_OPTIONS } from "../util/api"; import { MasonryScroller, - createPositioner, useContainerPosition, + usePositioner, useResizeObserver, } from "masonic"; import { @@ -47,31 +47,6 @@ const MASONRY_GUTTER = 8; const MASONRY_MAX_COLUMNS_MOBILE = 2; const MASONRY_MAX_COLUMNS_DESKTOP = 4; -function getMasonryColumns( - width = 0, - minimumWidth = 0, - gutter = 8, - columnCount?: number, - maxColumnCount?: number, - maxColumnWidth?: number, -): [number, number] { - columnCount = - columnCount || - Math.min( - Math.floor((width + gutter) / (minimumWidth + gutter)), - maxColumnCount || Infinity, - ) || - 1; - let computedColumnWidth = Math.floor( - (width - gutter * (columnCount - 1)) / columnCount, - ); - - if (maxColumnWidth !== undefined && computedColumnWidth > maxColumnWidth) { - computedColumnWidth = maxColumnWidth; - } - - return [computedColumnWidth, columnCount]; -} function useWindowHeight(): number { const [height, setHeight] = useState(() => @@ -408,6 +383,12 @@ type PieceListProps = { hasMore?: boolean; loading?: boolean; loadingMore?: boolean; + /** + * Increment this whenever the parent mutates pieces in a non-append way + * (e.g. prepending a newly created piece). The positioner resets so masonic + * re-lays out all items with correct index→height mappings. + */ + positionerResetKey?: number; }; const PieceList = (props: PieceListProps) => { @@ -420,6 +401,7 @@ const PieceList = (props: PieceListProps) => { hasMore = false, loading = false, loadingMore = false, + positionerResetKey = 0, } = props; const theme = useTheme(); const isMobile = useMediaQuery(theme.breakpoints.down("sm")); @@ -442,8 +424,22 @@ const PieceList = (props: PieceListProps) => { }, [onLoadMore]); const sentinelRef = useRef(null); + const windowHeight = useWindowHeight(); + const masonryRef = useRef(null); + const columnWidth = isMobile + ? MASONRY_COLUMN_WIDTH_MOBILE + : MASONRY_COLUMN_WIDTH_DESKTOP; + const { width: masonryWidth, offset: masonryOffset } = useContainerPosition( + masonryRef, + [isMobile], + ); + useEffect(() => { - if (!hasMore) return; + // Guard on masonryWidth: when width=0 the masonry grid hasn't rendered yet, + // so the sentinel sits at top≈0 and check() would fire onLoadMore prematurely + // before any cards are visible. Deferring until width>0 ensures the sentinel + // is at its true position in the document. + if (!hasMore || masonryWidth === 0) return; function check() { const sentinel = sentinelRef.current; if (!sentinel) return; @@ -453,7 +449,7 @@ const PieceList = (props: PieceListProps) => { window.addEventListener("scroll", check, { passive: true }); check(); return () => window.removeEventListener("scroll", check); - }, [hasMore]); + }, [hasMore, masonryWidth]); const availableTags = useMemo(() => { const deduped = new Map(); @@ -505,41 +501,36 @@ const PieceList = (props: PieceListProps) => { }, [activeFilters, activeTagIds, activeTags]); const hasActiveFilters = activeFilters.length > 0 || activeTagIds.length > 0; - const windowHeight = useWindowHeight(); - const masonryRef = useRef(null); - const columnWidth = isMobile - ? MASONRY_COLUMN_WIDTH_MOBILE - : MASONRY_COLUMN_WIDTH_DESKTOP; - const { width: masonryWidth, offset: masonryOffset } = useContainerPosition( - masonryRef, - [isMobile], - ); - const positioner = useMemo(() => { - const [computedColumnWidth, computedColumnCount] = getMasonryColumns( - masonryWidth, - columnWidth, - MASONRY_GUTTER, - undefined, - isMobile ? MASONRY_MAX_COLUMNS_MOBILE : MASONRY_MAX_COLUMNS_DESKTOP, - ); - const nextPositioner = createPositioner( - computedColumnCount, - computedColumnWidth, - MASONRY_GUTTER, - MASONRY_GUTTER, - ); - filteredPieces.forEach((piece, index) => { - if (piece.thumbnail?.crop) { - nextPositioner.set( - index, - getPieceCardLayout(piece, nextPositioner.columnWidth).estimatedHeight, - ); - } - }); + // usePositioner resets when sort, filters, or positionerResetKey change. + // Pagination appends change none of these, so the positioner is reused and + // existing item positions survive — eliminating the re-layout flash. + // positionerResetKey is incremented by the parent for non-append mutations + // (e.g. handleCreated prepend) that would otherwise leave stale positions. + const positioner = usePositioner( + { + width: masonryWidth, + columnWidth, + columnGutter: MASONRY_GUTTER, + rowGutter: MASONRY_GUTTER, + maxColumnCount: isMobile + ? MASONRY_MAX_COLUMNS_MOBILE + : MASONRY_MAX_COLUMNS_DESKTOP, + }, + [positionerResetKey, sortOrder ?? "", activeFilters.join(","), activeTagIds.join(",")], + ); - return nextPositioner; - }, [filteredPieces, masonryWidth, columnWidth, isMobile]); + // Seed crop heights for unpositioned items only. On a pure append the existing + // items already have positions (get returns non-undefined) so they are skipped. + // After a sort/filter reset all items are undefined and get fully reseeded. + filteredPieces.forEach((piece, index) => { + if (piece.thumbnail?.crop && positioner.get(index) === undefined) { + positioner.set( + index, + getPieceCardLayout(piece, positioner.columnWidth).estimatedHeight, + ); + } + }); const resizeObserver = useResizeObserver(positioner); const toggleFilter = useCallback( diff --git a/web/src/components/__tests__/PieceList.test.tsx b/web/src/components/__tests__/PieceList.test.tsx index eb85ffa3..2b80df18 100644 --- a/web/src/components/__tests__/PieceList.test.tsx +++ b/web/src/components/__tests__/PieceList.test.tsx @@ -1,5 +1,6 @@ import type React from "react"; import { useState } from "react"; +import { fireEvent } from "@testing-library/react"; import { describe, it, expect, vi } from "vitest"; import { render, screen, waitFor, within } from "@testing-library/react"; import userEvent from "@testing-library/user-event"; @@ -42,8 +43,14 @@ vi.mock("../CloudinaryImage", () => ({ ), })); -const { mockPositioner, mockContainerPosition } = vi.hoisted(() => { +const { mockPositioner, mockContainerPosition, mockState } = vi.hoisted(() => { const mockContainerPosition = { width: 440, offset: 0 }; + // mockState is shared between vi.mock (hoisted) and beforeEach so that the + // usePositioner mock can simulate positioner resets by clearing seededMap. + const mockState = { + seededMap: new Map(), + positionerDeps: [] as unknown[], + }; return { mockPositioner: { get: vi.fn().mockReturnValue(undefined), @@ -58,6 +65,7 @@ const { mockPositioner, mockContainerPosition } = vi.hoisted(() => { all: vi.fn().mockReturnValue([]), }, mockContainerPosition, + mockState, }; }); @@ -138,7 +146,19 @@ vi.mock("masonic", () => ({ width: mockContainerPosition.width, offset: mockContainerPosition.offset, }), - usePositioner: () => mockPositioner, + usePositioner: (_opts: unknown, deps: unknown[] = []) => { + // Simulate usePositioner's reset behaviour: when deps change, clear the + // seeded-heights map so get() returns undefined for all items, exactly as + // the real hook creates a fresh positioner and discards cached positions. + const depsChanged = + deps.length !== mockState.positionerDeps.length || + !deps.every((d, i) => Object.is(d, mockState.positionerDeps[i])); + if (depsChanged) { + mockState.positionerDeps = [...deps]; + mockState.seededMap.clear(); + } + return mockPositioner; + }, createPositioner: ( columnCount: number, columnWidth: number, @@ -205,14 +225,23 @@ function RerenderHarness({ pieces }: { pieces: PieceSummary[] }) { describe("PieceList", () => { beforeEach(() => { + // Reset mockState so each test starts with an empty positioner. + mockState.seededMap.clear(); + mockState.positionerDeps = []; mockPositioner.set.mockReset(); mockPositioner.get.mockReset(); mockPositioner.update.mockReset(); - mockPositioner.set.mockImplementation(() => { + // set() writes to mockState.seededMap so get() can return the seeded value, + // matching real positioner behaviour (once placed, get() returns non-undefined). + mockPositioner.set.mockImplementation((index: number, height: number) => { + mockState.seededMap.set(index, height); rerenderMasonryScroller?.(); - return undefined; }); - mockPositioner.get.mockImplementation(() => undefined); + mockPositioner.get.mockImplementation((index: number) => + mockState.seededMap.has(index) + ? { height: mockState.seededMap.get(index)!, top: 0, left: 0, column: 0 } + : undefined, + ); mockPositioner.update.mockImplementation(() => undefined); rerenderMasonryScroller = undefined; mockContainerPosition.width = 440; @@ -339,6 +368,132 @@ describe("PieceList", () => { expect(mockPositioner.set).toHaveBeenCalledTimes(1); }); + it("does not re-seed existing crop heights when pieces are appended via pagination", () => { + // Regression for #734: each filteredPieces change previously called createPositioner + // from scratch, discarding all cached positions and re-seeding every item. + // The fix switches to usePositioner so the positioner is reused on append, + // and guards the seeding loop with positioner.get(index) === undefined so + // already-positioned items are never re-seeded. + // beforeEach makes get() stateful (returns seeded value after set()), + // matching real positioner behaviour. + const pieceWithCrop = makePiece({ + id: "p-crop", + thumbnail: { + url: "https://example.com/img.jpg", + cloudinary_public_id: "id", + cloud_name: "demo", + crop: { x: 0, y: 0, width: 200, height: 400 }, + }, + }); + + function AppendHarness() { + const [pieces, setPieces] = useState([pieceWithCrop]); + return ( + <> + + + + ); + } + + const router = createMemoryRouter( + [{ path: "/", element: }], + { initialEntries: ["/"] }, + ); + render(); + + // Initial render: crop piece at index 0 is seeded once + expect(mockPositioner.set).toHaveBeenCalledTimes(1); + expect(mockPositioner.set).toHaveBeenCalledWith(0, expect.any(Number)); + + // Simulate pagination: append a plain piece + fireEvent.click(screen.getByRole("button", { name: /append/i })); + + // Index 0 is already positioned — must not be re-seeded + expect(mockPositioner.set).toHaveBeenCalledTimes(1); + }); + + it("resets the positioner when positionerResetKey changes (e.g. piece prepended)", () => { + // Regression for the handleCreated prepend edge case: prepending a new piece + // shifts all existing item indices. Without a reset, the positioner still has + // heights cached at the old indices, so masonic uses the wrong height for the + // prepended slot. PieceListPage increments positionerResetKey on create, + // which changes usePositioner deps → fresh positioner → all items re-seeded. + const pieceA = makePiece({ + id: "p-a", + thumbnail: { + url: "https://example.com/a.jpg", + cloudinary_public_id: "a", + cloud_name: "demo", + crop: { x: 0, y: 0, width: 200, height: 400 }, + }, + }); + const pieceB = makePiece({ + id: "p-b", + thumbnail: { + url: "https://example.com/b.jpg", + cloudinary_public_id: "b", + cloud_name: "demo", + crop: { x: 0, y: 0, width: 100, height: 150 }, + }, + }); + + function PrependHarness() { + const [pieces, setPieces] = useState([pieceA]); + const [resetKey, setResetKey] = useState(0); + return ( + <> + + + + ); + } + + const router = createMemoryRouter( + [{ path: "/", element: }], + { initialEntries: ["/"] }, + ); + render(); + + // Initial render: pieceA at index 0 seeded + expect(mockPositioner.set).toHaveBeenCalledTimes(1); + expect(mockPositioner.set).toHaveBeenCalledWith( + 0, + estimateCardHeight(pieceA, mockPositioner.columnWidth), + ); + + // Prepend pieceB with explicit reset signal: order is now [pieceB(0), pieceA(1)] + fireEvent.click(screen.getByRole("button", { name: /prepend/i })); + + // usePositioner deps changed → seededMap cleared → both items re-seeded. + // Without the reset: seededMap still has {0: h_A}, get(0)!==undefined, + // pieceB never seeded, masonic uses pieceA's height for the wrong slot. + expect(mockPositioner.set).toHaveBeenCalledTimes(3); + expect(mockPositioner.set).toHaveBeenCalledWith( + 0, + estimateCardHeight(pieceB, mockPositioner.columnWidth), + ); + expect(mockPositioner.set).toHaveBeenCalledWith( + 1, + estimateCardHeight(pieceA, mockPositioner.columnWidth), + ); + }); + it("reserves the thumbnail crop ratio in the card shell", () => { renderPieceList([ makePiece({ @@ -1166,6 +1321,34 @@ describe("PieceList", () => { }); }); + describe("scroll sentinel", () => { + it("does not call onLoadMore before the masonry container has a measured width", () => { + // Regression for #734: when hasMore=true, check() fires immediately on mount. + // At that moment masonryWidth=0 (ResizeObserver not yet fired), so the sentinel + // sits at top≈0 and onLoadMore would fire before any cards are visible. + // The fix adds masonryWidth to the effect deps and returns early when it is 0. + mockContainerPosition.width = 0; + const onLoadMore = vi.fn(); + const router = createMemoryRouter( + [ + { + path: "/", + element: ( + + ), + }, + ], + { initialEntries: ["/"] }, + ); + render(); + expect(onLoadMore).not.toHaveBeenCalled(); + }); + }); + describe("loading states", () => { it("dims the existing list during replace-style refreshes", () => { const router = createMemoryRouter( diff --git a/web/src/pages/PieceListPage.tsx b/web/src/pages/PieceListPage.tsx index 22cf694e..73d9ba03 100644 --- a/web/src/pages/PieceListPage.tsx +++ b/web/src/pages/PieceListPage.tsx @@ -39,6 +39,7 @@ export default function PieceListPage() { ? sortFromUrl : DEFAULT_PIECE_SORT; const [dialogOpen, setDialogOpen] = useState(false); + const [positionerResetKey, setPositionerResetKey] = useState(0); const theme = useTheme(); const isMobile = useMediaQuery(theme.breakpoints.down("sm")); @@ -137,6 +138,9 @@ export default function PieceListPage() { function handleCreated(piece: PieceDetail) { setPieces((prev) => [piece, ...prev]); setCount((c) => c + 1); + // Prepending shifts all existing item indices, invalidating cached positions. + // Signal PieceList to reset the positioner so masonic re-lays out correctly. + setPositionerResetKey((k) => k + 1); } const hasMore = pieces.length < count; @@ -182,6 +186,7 @@ export default function PieceListPage() { hasMore={hasMore} loading={refreshing} loadingMore={loadingMore} + positionerResetKey={positionerResetKey} /> )}