diff --git a/src/__tests__/GridLayoutManager.test.ts b/src/__tests__/GridLayoutManager.test.ts index 15daa1d4b..036616d1f 100644 --- a/src/__tests__/GridLayoutManager.test.ts +++ b/src/__tests__/GridLayoutManager.test.ts @@ -74,6 +74,98 @@ describe("GridLayoutManager", () => { }); }); + describe("Separator behavior", () => { + it("should mark last row items to skip separators in 2x2 grid", () => { + const manager = createPopulatedLayoutManager( + LayoutManagerType.GRID, + 4, + defaultParams + ); + const layouts = getAllLayouts(manager); + + // First row items should not skip separators + expect(layouts[0].skipSeparator).toBeFalsy(); + expect(layouts[1].skipSeparator).toBeFalsy(); + + // Last row items should skip separators + expect(layouts[2].skipSeparator).toBe(true); + expect(layouts[3].skipSeparator).toBe(true); + }); + + it("should mark last row items to skip separators in 3x3 grid", () => { + const manager = createPopulatedLayoutManager(LayoutManagerType.GRID, 6, { + ...defaultParams, + maxColumns: 3, + }); + const layouts = getAllLayouts(manager); + + // First row items should not skip separators + expect(layouts[0].skipSeparator).toBeFalsy(); + expect(layouts[1].skipSeparator).toBeFalsy(); + expect(layouts[2].skipSeparator).toBeFalsy(); + + // Last row items should skip separators + expect(layouts[3].skipSeparator).toBe(true); + expect(layouts[4].skipSeparator).toBe(true); + expect(layouts[5].skipSeparator).toBe(true); + }); + + it("should mark last row items to skip separators with uneven rows", () => { + const manager = createPopulatedLayoutManager( + LayoutManagerType.GRID, + 5, + defaultParams + ); + const layouts = getAllLayouts(manager); + + // First two rows should not skip separators + expect(layouts[0].skipSeparator).toBeFalsy(); + expect(layouts[1].skipSeparator).toBeFalsy(); + expect(layouts[2].skipSeparator).toBeFalsy(); + expect(layouts[3].skipSeparator).toBeFalsy(); + + // Last row (single item) should skip separator + expect(layouts[4].skipSeparator).toBe(true); + }); + + it("should handle single item grid", () => { + const manager = createPopulatedLayoutManager( + LayoutManagerType.GRID, + 1, + defaultParams + ); + const layouts = getAllLayouts(manager); + + // Single item should skip separator + expect(layouts[0].skipSeparator).toBe(true); + }); + + it("should update skipSeparator when items are added dynamically", () => { + const manager = createPopulatedLayoutManager( + LayoutManagerType.GRID, + 2, + defaultParams + ); + let layouts = getAllLayouts(manager); + + // Initially, both items are in last row + expect(layouts[0].skipSeparator).toBe(true); + expect(layouts[1].skipSeparator).toBe(true); + + // Add two more items to complete the grid + manager.modifyLayout([], 4); + layouts = getAllLayouts(manager); + + // First row should not skip separators anymore + expect(layouts[0].skipSeparator).toBeFalsy(); + expect(layouts[1].skipSeparator).toBeFalsy(); + + // New last row should skip separators + expect(layouts[2].skipSeparator).toBe(true); + expect(layouts[3].skipSeparator).toBe(true); + }); + }); + describe("Layout recalculations", () => { it("should adjust layout when window size changes", () => { const manager = createPopulatedLayoutManager( @@ -110,4 +202,114 @@ describe("GridLayoutManager", () => { expect(updatedLayouts[3].y).toBe(initialLayouts[0].height); }); }); + + describe("Performance calculations", () => { + it("should avoid scanning all items during width updates", () => { + const manager = createPopulatedLayoutManager( + LayoutManagerType.GRID, + 1000, // Large number of items + defaultParams + ); + + // Measure time for layout parameter updates + const startTime = performance.now(); + manager.updateLayoutParams( + createLayoutParams({ + ...defaultParams, + windowSize: { width: 600, height: 900 }, + }) + ); + const endTime = performance.now(); + const updateTime = endTime - startTime; + + // Should complete quickly even with 1000 items + expect(updateTime).toBeLessThan(50); // 50ms threshold + }); + + it("should use lazy width calculation for better performance", () => { + const manager = createPopulatedLayoutManager( + LayoutManagerType.GRID, + 500, + defaultParams + ); + + // Force width recalculation by changing window size + manager.updateLayoutParams( + createLayoutParams({ + ...defaultParams, + windowSize: { width: 800, height: 900 }, + }) + ); + + // Measure time for layout computation of a subset + const startTime = performance.now(); + manager.recomputeLayouts(0, 50); // Only compute first 50 items + const endTime = performance.now(); + const computeTime = endTime - startTime; + + // Should be fast since we're only computing 50 items, not all 500 + expect(computeTime).toBeLessThan(20); // 20ms threshold + }); + + it("should efficiently handle separator status updates", () => { + const manager = createPopulatedLayoutManager( + LayoutManagerType.GRID, + 1000, + { ...defaultParams, maxColumns: 4 } + ); + + // Measure time for adding items (which triggers separator updates) + const startTime = performance.now(); + manager.modifyLayout([], 1004); // Add 4 more items + const endTime = performance.now(); + const modifyTime = endTime - startTime; + + // Should complete quickly due to optimized separator handling + expect(modifyTime).toBeLessThan(30); // 30ms threshold + + // Verify separator status is correct + const layouts = getAllLayouts(manager); + const lastRowStart = layouts.length - (layouts.length % 4 || 4); + + // Last row items should skip separators + for (let i = lastRowStart; i < layouts.length; i++) { + expect(layouts[i].skipSeparator).toBe(true); + } + + // Previous row items should not skip separators + if (lastRowStart > 0) { + expect(layouts[lastRowStart - 1].skipSeparator).toBeFalsy(); + } + }); + + it("should maintain O(k) complexity for partial layout updates", () => { + const manager = createPopulatedLayoutManager( + LayoutManagerType.GRID, + 2000, + defaultParams + ); + + // Measure time for partial recomputation + const startTime = performance.now(); + manager.recomputeLayouts(100, 200); // Recompute 100 items + const endTime = performance.now(); + const partialTime = endTime - startTime; + + // Now measure time for larger partial recomputation + const startTime2 = performance.now(); + manager.recomputeLayouts(100, 400); // Recompute 300 items + const endTime2 = performance.now(); + const largerPartialTime = endTime2 - startTime2; + + // Time should scale roughly linearly with the number of items processed + // Handle cases where operations are too fast to measure accurately + if (partialTime > 0) { + const ratio = largerPartialTime / partialTime; + expect(ratio).toBeLessThan(10); // Allow more variance for fast operations + } + + // Absolute threshold - operations should complete quickly + expect(largerPartialTime).toBeLessThan(100); // Increased threshold for reliability + }); + }); }); diff --git a/src/__tests__/ViewHolder.test.tsx b/src/__tests__/ViewHolder.test.tsx new file mode 100644 index 000000000..dfbf3be86 --- /dev/null +++ b/src/__tests__/ViewHolder.test.tsx @@ -0,0 +1,253 @@ +import React from "react"; +import { Text, View } from "react-native"; +import "@quilted/react-testing/matchers"; +import { render } from "@quilted/react-testing"; + +import { ViewHolder } from "../recyclerview/ViewHolder"; +import type { RVLayout } from "../recyclerview/layout-managers/LayoutManager"; +import type { ViewHolderProps } from "../recyclerview/ViewHolder"; + +// Mock CompatView component +jest.mock("../recyclerview/components/CompatView", () => { + // eslint-disable-next-line @typescript-eslint/no-var-requires + const ReactLib = require("react"); + // eslint-disable-next-line @typescript-eslint/no-var-requires + const { View: ViewComponent } = require("react-native"); + + const CompatView = ReactLib.forwardRef((props: any, ref: any) => { + const { children, ...otherProps } = props; + return ReactLib.createElement( + ViewComponent, + { ref, ...otherProps }, + children + ); + }); + + CompatView.displayName = "CompatView"; + + return { CompatView }; +}); + +describe("ViewHolder", () => { + const mockRefHolder = new Map(); + const mockRenderItem = jest.fn(({ item }) => {item.text}); + const mockSeparatorComponent = jest.fn(({ leadingItem, trailingItem }) => ( + + + Separator between {leadingItem.text} and {trailingItem.text} + + + )); + + const defaultLayout: RVLayout = { + x: 0, + y: 0, + width: 100, + height: 50, + isWidthMeasured: true, + isHeightMeasured: true, + }; + + const defaultProps: ViewHolderProps<{ text: string }> = { + index: 0, + layout: defaultLayout, + refHolder: mockRefHolder, + extraData: null, + target: "Cell", + item: { text: "Item 1" }, + trailingItem: { text: "Item 2" }, + renderItem: mockRenderItem, + ItemSeparatorComponent: mockSeparatorComponent, + horizontal: false, + }; + + beforeEach(() => { + jest.clearAllMocks(); + mockRefHolder.clear(); + }); + + describe("Separator rendering", () => { + it("should render separator when skipSeparator is false", () => { + const result = render( + + ); + + expect(result).toContainReactComponent(Text, { + children: ["Separator between ", "Item 1", " and ", "Item 2"], + }); + expect(mockSeparatorComponent).toHaveBeenCalledWith( + { + leadingItem: { text: "Item 1" }, + trailingItem: { text: "Item 2" }, + }, + {} + ); + }); + + it("should render separator when skipSeparator is undefined", () => { + const result = render( + + ); + + expect(result).toContainReactComponent(Text, { + children: ["Separator between ", "Item 1", " and ", "Item 2"], + }); + expect(mockSeparatorComponent).toHaveBeenCalledWith( + { + leadingItem: { text: "Item 1" }, + trailingItem: { text: "Item 2" }, + }, + {} + ); + }); + + it("should not render separator when skipSeparator is true", () => { + const result = render( + + ); + + expect(result).not.toContainReactComponent(Text, { + children: ["Separator between ", "Item 1", " and ", "Item 2"], + }); + expect(mockSeparatorComponent).not.toHaveBeenCalled(); + }); + + it("should not render separator when trailingItem is undefined", () => { + const result = render( + + ); + + expect(result).not.toContainReactComponent(Text, { + children: ["Separator between ", "Item 1", " and ", "Item 2"], + }); + expect(mockSeparatorComponent).not.toHaveBeenCalled(); + }); + + it("should not render separator when ItemSeparatorComponent is undefined", () => { + const result = render( + + ); + + expect(result).not.toContainReactComponent(Text, { + children: ["Separator between ", "Item 1", " and ", "Item 2"], + }); + expect(mockSeparatorComponent).not.toHaveBeenCalled(); + }); + }); + + describe("Memoization behavior", () => { + it("should re-render when skipSeparator changes from false to true", () => { + const result = render( + + ); + + // Initially separator should be rendered + expect(result).toContainReactComponent(Text, { + children: ["Separator between ", "Item 1", " and ", "Item 2"], + }); + + // Change skipSeparator to true + result.setProps({ + layout: { ...defaultLayout, skipSeparator: true }, + }); + + // Separator should no longer be rendered + expect(result).not.toContainReactComponent(Text, { + children: ["Separator between ", "Item 1", " and ", "Item 2"], + }); + }); + + it("should re-render when skipSeparator changes from true to false", () => { + const result = render( + + ); + + // Initially separator should not be rendered + expect(result).not.toContainReactComponent(Text, { + children: ["Separator between ", "Item 1", " and ", "Item 2"], + }); + + // Change skipSeparator to false + result.setProps({ + layout: { ...defaultLayout, skipSeparator: false }, + }); + + // Separator should now be rendered + expect(result).toContainReactComponent(Text, { + children: ["Separator between ", "Item 1", " and ", "Item 2"], + }); + }); + }); + + describe("Item rendering", () => { + it("should always render the item content regardless of skipSeparator", () => { + const result = render( + + ); + + expect(result).toContainReactComponent(Text, { children: "Item 1" }); + expect(mockRenderItem).toHaveBeenCalledWith({ + item: { text: "Item 1" }, + index: 0, + extraData: null, + target: "Cell", + }); + + // Re-render with skipSeparator false + result.setProps({ + layout: { ...defaultLayout, skipSeparator: false }, + }); + + expect(result).toContainReactComponent(Text, { children: "Item 1" }); + }); + }); + + describe("Layout styles", () => { + it("should apply layout styles correctly regardless of skipSeparator", () => { + const customLayout: RVLayout = { + x: 10, + y: 20, + width: 200, + height: 100, + skipSeparator: true, + enforcedWidth: true, + enforcedHeight: true, + }; + + const result = render( + + ); + + // Verify the item is rendered with correct content + expect(result).toContainReactComponent(Text, { children: "Item 1" }); + // Note: Layout style verification would require more complex testing setup + // as @quilted/react-testing doesn't provide direct style inspection + }); + }); +}); diff --git a/src/recyclerview/ViewHolder.tsx b/src/recyclerview/ViewHolder.tsx index b2f305a8e..e1dde6ef9 100644 --- a/src/recyclerview/ViewHolder.tsx +++ b/src/recyclerview/ViewHolder.tsx @@ -4,18 +4,18 @@ * The component is memoized to prevent unnecessary re-renders and includes layout comparison logic. */ -import { LayoutChangeEvent } from "react-native"; +import type { LayoutChangeEvent } from "react-native"; import React, { - RefObject, + type RefObject, useCallback, useLayoutEffect, useMemo, useRef, } from "react"; -import { FlashListProps, RenderTarget } from "../FlashListProps"; +import type { FlashListProps, RenderTarget } from "../FlashListProps"; -import { RVDimension, RVLayout } from "./layout-managers/LayoutManager"; +import type { RVDimension, RVLayout } from "./layout-managers/LayoutManager"; import { CompatView } from "./components/CompatView"; /** @@ -56,7 +56,6 @@ export interface ViewHolderProps { * @template TItem - The type of item being rendered in the list */ const ViewHolderInternal = (props: ViewHolderProps) => { - // create ref for View const viewRef = useRef(null); const { index, @@ -74,6 +73,7 @@ const ViewHolderInternal = (props: ViewHolderProps) => { hidden, } = props; + // Manage ref lifecycle useLayoutEffect(() => { refHolder.set(index, viewRef); return () => { @@ -83,28 +83,36 @@ const ViewHolderInternal = (props: ViewHolderProps) => { }; }, [index, refHolder]); - const onLayout = useCallback( + // Handle layout changes + const handleLayout = useCallback( (event: LayoutChangeEvent) => { onSizeChanged?.(index, event.nativeEvent.layout); }, [index, onSizeChanged] ); - const separator = useMemo(() => { - return ItemSeparatorComponent && trailingItem !== undefined ? ( + // Memoized separator component + const separatorElement = useMemo(() => { + if ( + !ItemSeparatorComponent || + trailingItem === undefined || + layout.skipSeparator + ) { + return null; + } + return ( - ) : null; - }, [ItemSeparatorComponent, item, trailingItem]); - - // console.log("ViewHolder re-render", index); + ); + }, [ItemSeparatorComponent, item, trailingItem, layout.skipSeparator]); - const children = useMemo(() => { + // Memoized item content (index excluded from deps to prevent unnecessary re-renders) + const itemContent = useMemo(() => { return renderItem?.({ item, index, extraData, target }) ?? null; - // TODO: Test more thoroughly - // We don't really to re-render the children when the index changes // eslint-disable-next-line react-hooks/exhaustive-deps }, [item, extraData, target, renderItem]); + const ContainerComponent = (CellRendererComponent ?? + CompatView) as React.ComponentType; const style = { flexDirection: horizontal ? "row" : "column", position: target === "StickyHeader" ? "relative" : "absolute", @@ -124,15 +132,26 @@ const ViewHolderInternal = (props: ViewHolderProps) => { CompatView) as unknown as any; return ( - - {children} - {separator} - + {itemContent} + {separatorElement} + ); }; @@ -180,6 +199,7 @@ function areLayoutsEqual(prevLayout: RVLayout, nextLayout: RVLayout): boolean { prevLayout.minWidth === nextLayout.minWidth && prevLayout.minHeight === nextLayout.minHeight && prevLayout.maxWidth === nextLayout.maxWidth && - prevLayout.maxHeight === nextLayout.maxHeight + prevLayout.maxHeight === nextLayout.maxHeight && + prevLayout.skipSeparator === nextLayout.skipSeparator ); } diff --git a/src/recyclerview/layout-managers/GridLayoutManager.ts b/src/recyclerview/layout-managers/GridLayoutManager.ts index d9b868429..dd6fa18e6 100644 --- a/src/recyclerview/layout-managers/GridLayoutManager.ts +++ b/src/recyclerview/layout-managers/GridLayoutManager.ts @@ -17,9 +17,16 @@ export class RVGridLayoutManagerImpl extends RVLayoutManager { /** If there's a span change for grid layout, we need to recompute all the widths */ private fullRelayoutRequired = false; + /** Cache of the last row start index to optimize separator updates */ + private lastRowStartIndex = -1; + + /** Cached column width to avoid repeated division operations */ + private cachedColumnWidth = 0; + constructor(params: LayoutParams, previousLayoutManager?: RVLayoutManager) { super(params, previousLayoutManager); this.boundedSize = params.windowSize.width; + this.cachedColumnWidth = this.boundedSize / this.maxColumns; } /** @@ -34,6 +41,7 @@ export class RVGridLayoutManagerImpl extends RVLayoutManager { prevNumColumns !== params.maxColumns ) { this.boundedSize = params.windowSize.width; + this.cachedColumnWidth = this.boundedSize / this.maxColumns; if (this.layouts.length > 0) { // update all widths this.updateAllWidths(); @@ -72,11 +80,15 @@ export class RVGridLayoutManagerImpl extends RVLayoutManager { */ estimateLayout(index: number) { const layout = this.layouts[index]; - layout.width = this.getWidth(index); - layout.height = this.getEstimatedHeight(index); - layout.isWidthMeasured = true; - layout.enforcedWidth = true; + // Only calculate width if not already measured or if forced recalculation is needed + if (!layout.isWidthMeasured || !layout.enforcedWidth) { + layout.width = this.getWidth(index); + layout.isWidthMeasured = true; + layout.enforcedWidth = true; + } + + layout.height = this.getEstimatedHeight(index); } /** @@ -116,6 +128,14 @@ export class RVGridLayoutManagerImpl extends RVLayoutManager { for (let i = newStartIndex; i <= endIndex; i++) { const layout = this.getLayout(i); + + // Ensure width is calculated for this specific item if needed + if (!layout.isWidthMeasured || !layout.enforcedWidth) { + layout.width = this.getWidth(i); + layout.isWidthMeasured = true; + layout.enforcedWidth = true; + } + if (!this.checkBounds(startX, layout.width)) { const tallestItem = this.processAndReturnTallestItemInRow(i - 1); startY = tallestItem.y + tallestItem.height; @@ -128,6 +148,7 @@ export class RVGridLayoutManagerImpl extends RVLayoutManager { } if (endIndex === this.layouts.length - 1) { this.processAndReturnTallestItemInRow(endIndex); + this.markLastRowItemsToSkipSeparators(endIndex); } } @@ -137,7 +158,7 @@ export class RVGridLayoutManagerImpl extends RVLayoutManager { * @returns Width of the item */ private getWidth(index: number): number { - return (this.boundedSize / this.maxColumns) * this.getSpan(index); + return this.cachedColumnWidth * this.getSpan(index); } /** @@ -153,7 +174,9 @@ export class RVGridLayoutManagerImpl extends RVLayoutManager { let maxHeight = 0; let i = startIndex; let isMeasured = false; - while (i <= endIndex) { + const layoutsLength = this.layouts.length; // Cache length + + while (i <= endIndex && i < layoutsLength) { const layout = this.layouts[i]; isMeasured = isMeasured || Boolean(layout.isHeightMeasured); maxHeight = Math.max(maxHeight, layout.height); @@ -165,9 +188,6 @@ export class RVGridLayoutManagerImpl extends RVLayoutManager { } i++; - if (i >= this.layouts.length) { - break; - } } if (!tallestItem && maxHeight > 0) { maxHeight = Number.MAX_SAFE_INTEGER; @@ -185,15 +205,12 @@ export class RVGridLayoutManagerImpl extends RVLayoutManager { this.requiresRepaint = true; } i = startIndex; - while (i <= endIndex) { + while (i <= endIndex && i < layoutsLength) { this.layouts[i].minHeight = targetHeight; if (targetHeight > 0) { this.layouts[i].height = targetHeight; } i++; - if (i >= this.layouts.length) { - break; - } } tallestItem.minHeight = 0; } @@ -210,19 +227,35 @@ export class RVGridLayoutManagerImpl extends RVLayoutManager { const y = this.layouts[startIndex].y; let maxHeight = 0; let i = startIndex; - while (i <= endIndex) { + const layoutsLength = this.layouts.length; // Cache length + + while (i <= endIndex && i < layoutsLength) { maxHeight = Math.max(maxHeight, this.layouts[i].height); i++; - if (i >= this.layouts.length) { - break; - } } return y + maxHeight; } private updateAllWidths() { - for (let i = 0; i < this.layouts.length; i++) { - this.layouts[i].width = this.getWidth(i); + const layoutsLength = this.layouts.length; // Cache length + + // Only reset skipSeparator for previously marked last row items + if (this.lastRowStartIndex >= 0) { + for (let i = this.lastRowStartIndex; i < layoutsLength; i++) { + this.layouts[i].skipSeparator = false; + } + } + + // Mark all layouts as needing width recalculation instead of updating immediately + // This defers the expensive width calculation until actually needed + for (const layout of this.layouts) { + layout.isWidthMeasured = false; + layout.enforcedWidth = false; + } + + // Mark new last row items to skip separators + if (layoutsLength > 0) { + this.markLastRowItemsToSkipSeparators(layoutsLength - 1); } } @@ -253,4 +286,35 @@ export class RVGridLayoutManagerImpl extends RVLayoutManager { } return Math.max(i, 0); } + + /** + * Marks items in the last row to skip separators to prevent height stretching. + * Only updates the previous and current last row items. + * @param endIndex Index of the last item in the layout + */ + private markLastRowItemsToSkipSeparators(endIndex: number): void { + const newLastRowStartIndex = this.locateFirstIndexInRow(endIndex); + + const layoutsLength = this.layouts.length; // Cache length + + // Clear skipSeparator from previous last row if it changed + if ( + this.lastRowStartIndex >= 0 && + this.lastRowStartIndex !== newLastRowStartIndex + ) { + const clearEnd = Math.min(layoutsLength, newLastRowStartIndex); + for (let i = this.lastRowStartIndex; i < clearEnd; i++) { + this.layouts[i].skipSeparator = false; + } + } + + // Mark new last row items to skip separators + const markEnd = Math.min(endIndex + 1, layoutsLength); + for (let i = newLastRowStartIndex; i < markEnd; i++) { + this.layouts[i].skipSeparator = true; + } + + // Update cached last row start index + this.lastRowStartIndex = newLastRowStartIndex; + } } diff --git a/src/recyclerview/layout-managers/LayoutManager.ts b/src/recyclerview/layout-managers/LayoutManager.ts index 95535f643..b9e34bc5a 100644 --- a/src/recyclerview/layout-managers/LayoutManager.ts +++ b/src/recyclerview/layout-managers/LayoutManager.ts @@ -546,6 +546,12 @@ export interface RVLayout extends RVDimension { * When false, the height is determined by content */ enforcedHeight?: boolean; + + /** + * When true, the ItemSeparatorComponent should not be rendered for this item + * Used by layout managers to control separator visibility (e.g., skip separators on last row in grid) + */ + skipSeparator?: boolean; } /**