From 6de887ecc48e0b021fbe9e5791ecbdf287209fed Mon Sep 17 00:00:00 2001 From: Ruud Date: Thu, 15 Oct 2026 17:15:27 +0200 Subject: [PATCH] fix(virtual-list): keep the ref handle stable across scroll renders --- .changeset/virtuallist-stable-ref.md | 5 + .../VirtualList/VirtualList.spec.tsx | 73 +++++++++++ .../components/VirtualList/VirtualList.tsx | 77 +++++++++--- .../VirtualList/createTestRenderer.ts | 116 ++++++++++++++++++ .../VirtualList/useContentCross.spec.tsx | 113 +---------------- 5 files changed, 254 insertions(+), 130 deletions(-) create mode 100644 .changeset/virtuallist-stable-ref.md create mode 100644 packages/react-lightning-components/src/components/VirtualList/VirtualList.spec.tsx create mode 100644 packages/react-lightning-components/src/components/VirtualList/createTestRenderer.ts diff --git a/.changeset/virtuallist-stable-ref.md b/.changeset/virtuallist-stable-ref.md new file mode 100644 index 00000000..faada133 --- /dev/null +++ b/.changeset/virtuallist-stable-ref.md @@ -0,0 +1,5 @@ +--- +'@plextv/react-lightning-components': patch +--- + +virtual list keeps one ref handle for its lifetime, so cells reading it from a layout effect during a scroll no longer get null. diff --git a/packages/react-lightning-components/src/components/VirtualList/VirtualList.spec.tsx b/packages/react-lightning-components/src/components/VirtualList/VirtualList.spec.tsx new file mode 100644 index 00000000..c15cfd87 --- /dev/null +++ b/packages/react-lightning-components/src/components/VirtualList/VirtualList.spec.tsx @@ -0,0 +1,73 @@ +import { type ReactNode, useLayoutEffect } from 'react'; +import { describe, expect, it, vi } from 'vitest'; + +vi.mock('@plextv/react-lightning', () => ({ + FocusGroup: ({ children }: { children?: ReactNode }) => children, + useFocusManager: () => ({ setFocusedChild: () => {} }), +})); + +vi.mock('@plextv/react-lightning-plugin-flexbox', () => ({ + FlexBoundary: ({ children }: { children?: ReactNode }) => children, + FlexRoot: ({ children }: { children?: ReactNode }) => children, + useIsInFlex: () => false, + onFlexLayoutSettled: () => () => {}, +})); + +import { createTestRenderer } from './createTestRenderer'; +import { VirtualList } from './VirtualList'; +import type { ItemLayout, VirtualListRef } from './VirtualListTypes'; + +type Read = { + index: number; + hasHandle: boolean; + range?: { startIndex: number; endIndex: number }; + layout?: ItemLayout; +}; + +describe('VirtualList', () => { + it('keeps the ref attached while cells run layout effects after a scroll', () => { + const { render, flush } = createTestRenderer(); + const listRef: { current: VirtualListRef | null } = { current: null }; + const reads: Read[] = []; + + function Cell({ index }: { index: number }) { + useLayoutEffect(() => { + const handle = listRef.current; + + reads.push({ + index, + hasHandle: handle != null, + range: handle?.getVisibleRange(), + layout: handle?.getLayout(index), + }); + }, [index]); + + return null; + } + + render( + i)} + drawDistance={0} + overrideItemLayout={(layout) => { + layout.size = 100; + }} + renderItem={({ index }) => } + style={{ w: 500, h: 300 }} + />, + ); + + reads.length = 0; + listRef.current?.scrollToOffset({ offset: 2000, animated: false }); + flush(); + + expect(reads.length).toBeGreaterThan(0); + + for (const read of reads) { + expect(read.hasHandle).toBe(true); + expect(read.range?.startIndex).toBe(20); + expect(read.layout?.y).toBe(read.index * 100); + } + }); +}); diff --git a/packages/react-lightning-components/src/components/VirtualList/VirtualList.tsx b/packages/react-lightning-components/src/components/VirtualList/VirtualList.tsx index a8d69409..11bad7ca 100644 --- a/packages/react-lightning-components/src/components/VirtualList/VirtualList.tsx +++ b/packages/react-lightning-components/src/components/VirtualList/VirtualList.tsx @@ -7,6 +7,7 @@ import { useContext, useEffect, useImperativeHandle, + useInsertionEffect, useLayoutEffect, useRef, useState, @@ -705,26 +706,64 @@ function VirtualListInner( } }; - useImperativeHandle(ref, () => ({ - scrollToIndex: (params) => - scrollToIndex( - params.index, - params.animated, - params.viewPosition, - params.viewOffset, - ), - scrollToOffset: (params) => scrollToOffset(params.offset, params.animated), - scrollToEnd: (params) => scrollToEnd(params?.animated), - getScrollOffset: () => scrollOffsetRef.current, - getVisibleRange: () => visibleRange, - getLayout: (index) => { - const layout = layoutManager.getLayout(index); + const handleStateRef = useRef({ + scrollToIndex, + scrollToOffset, + scrollToEnd, + visibleRange, + itemAreaOffset, + paddingCross, + horizontal, + }); - return layout - ? computeItemRect(layout, itemAreaOffset, paddingCross, horizontal) - : undefined; - }, - })); + // Insertion effects run before any layout effect in the commit, so cells + // calling the handle from their own layout effects already see this render. + useInsertionEffect(() => { + handleStateRef.current = { + scrollToIndex, + scrollToOffset, + scrollToEnd, + visibleRange, + itemAreaOffset, + paddingCross, + horizontal, + }; + }); + + // One handle for the list's lifetime: a rebuilt handle detaches the ref + // for the whole commit, so cells' layout effects would read null. + useImperativeHandle( + ref, + () => ({ + scrollToIndex: (params) => + handleStateRef.current.scrollToIndex( + params.index, + params.animated, + params.viewPosition, + params.viewOffset, + ), + scrollToOffset: (params) => + handleStateRef.current.scrollToOffset(params.offset, params.animated), + scrollToEnd: (params) => + handleStateRef.current.scrollToEnd(params?.animated), + getScrollOffset: () => scrollOffsetRef.current, + getVisibleRange: () => handleStateRef.current.visibleRange, + getLayout: (index) => { + const layout = layoutManager.getLayout(index); + const state = handleStateRef.current; + + return layout + ? computeItemRect( + layout, + state.itemAreaOffset, + state.paddingCross, + state.horizontal, + ) + : undefined; + }, + }), + [], + ); const loadTimeRef = useRef(Date.now()); const hasLoadedRef = useRef(false); diff --git a/packages/react-lightning-components/src/components/VirtualList/createTestRenderer.ts b/packages/react-lightning-components/src/components/VirtualList/createTestRenderer.ts new file mode 100644 index 00000000..9364cba3 --- /dev/null +++ b/packages/react-lightning-components/src/components/VirtualList/createTestRenderer.ts @@ -0,0 +1,116 @@ +import { type ReactNode, createContext } from 'react'; +import createReconciler, { type ReactContext } from 'react-reconciler'; +import { DiscreteEventPriority, NoEventPriority } from 'react-reconciler/constants'; + +const noop = () => {}; + +// Bare host config: nothing paints, only scheduling and effects matter. Every +// update is sync so tests can flush in place. +export type TestRenderer = { + render: (element: ReactNode) => void; + flush: () => void; +}; + +export function createTestRenderer(): TestRenderer { + const transitionContext = createContext(null); + let priority: number = NoEventPriority; + + // oxlint-disable-next-line typescript/no-explicit-any -- test-only host config, nothing is a real instance + const reconciler = createReconciler< + any, + any, + any, + any, + any, + any, + any, + any, + any, + any, + any, + any, + any, + any + >({ + supportsMutation: true, + supportsPersistence: false, + supportsHydration: false, + isPrimaryRenderer: false, + warnsIfNotActing: false, + supportsMicrotasks: true, + scheduleMicrotask: queueMicrotask, + scheduleTimeout: setTimeout, + cancelTimeout: clearTimeout, + noTimeout: -1, + getRootHostContext: () => ({}), + getChildHostContext: (ctx: unknown) => ctx, + getPublicInstance: (instance: unknown) => instance, + createInstance: () => ({ node: { x: 0, y: 0, w: 0, h: 0 } }), + createTextInstance: () => ({}), + appendInitialChild: noop, + appendChild: noop, + appendChildToContainer: noop, + insertBefore: noop, + insertInContainerBefore: noop, + removeChild: noop, + removeChildFromContainer: noop, + clearContainer: noop, + commitUpdate: noop, + commitTextUpdate: noop, + hideInstance: noop, + unhideInstance: noop, + hideTextInstance: noop, + unhideTextInstance: noop, + detachDeletedInstance: noop, + finalizeInitialChildren: () => false, + prepareForCommit: () => null, + resetAfterCommit: noop, + shouldSetTextContent: () => false, + preparePortalMount: noop, + getInstanceFromNode: () => null, + beforeActiveInstanceBlur: noop, + afterActiveInstanceBlur: noop, + prepareScopeUpdate: noop, + getInstanceFromScope: () => null, + resetFormInstance: noop, + requestPostPaintCallback: noop, + shouldAttemptEagerTransition: () => false, + trackSchedulerEvent: noop, + resolveEventType: () => null, + resolveEventTimeStamp: () => -1.1, + maySuspendCommit: () => false, + startSuspendingCommit: noop, + suspendInstance: noop, + preloadInstance: () => true, + waitForCommitToBeReady: () => null, + setCurrentUpdatePriority: (next: number) => { + priority = next; + }, + getCurrentUpdatePriority: () => priority, + resolveUpdatePriority: () => (priority !== NoEventPriority ? priority : DiscreteEventPriority), + NotPendingTransition: null, + // React's own context object carries the internal fields the typings ask for. + HostTransitionContext: transitionContext as unknown as ReactContext, + }); + + const container = reconciler.createContainer( + {}, + 0, + null, + false, + null, + '', + noop, + noop, + noop, + noop, + ); + + return { + render: (element) => { + reconciler.updateContainerSync(element, container, null, null); + reconciler.flushSyncWork(); + }, + flush: () => reconciler.flushSyncWork(), + }; +} diff --git a/packages/react-lightning-components/src/components/VirtualList/useContentCross.spec.tsx b/packages/react-lightning-components/src/components/VirtualList/useContentCross.spec.tsx index 151b966e..0eefc13b 100644 --- a/packages/react-lightning-components/src/components/VirtualList/useContentCross.spec.tsx +++ b/packages/react-lightning-components/src/components/VirtualList/useContentCross.spec.tsx @@ -1,118 +1,9 @@ -import { Activity, createContext } from 'react'; -import createReconciler, { type ReactContext } from 'react-reconciler'; -import { DiscreteEventPriority, NoEventPriority } from 'react-reconciler/constants'; +import { Activity } from 'react'; import { describe, expect, it } from 'vitest'; +import { createTestRenderer } from './createTestRenderer'; import { type ContentCross, useContentCross } from './useContentCross'; -const noop = () => {}; - -// Bare host config: the probe renders nothing, so only the scheduling and -// effect machinery matters. Every update is sync so tests can flush in place. -function createTestRenderer() { - const transitionContext = createContext(null); - let priority: number = NoEventPriority; - - // oxlint-disable-next-line typescript/no-explicit-any -- test-only host config, nothing is a real instance - const reconciler = createReconciler< - any, - any, - any, - any, - any, - any, - any, - any, - any, - any, - any, - any, - any, - any - >({ - supportsMutation: true, - supportsPersistence: false, - supportsHydration: false, - isPrimaryRenderer: false, - warnsIfNotActing: false, - supportsMicrotasks: true, - scheduleMicrotask: queueMicrotask, - scheduleTimeout: setTimeout, - cancelTimeout: clearTimeout, - noTimeout: -1, - getRootHostContext: () => null, - getChildHostContext: (ctx: unknown) => ctx, - getPublicInstance: (instance: unknown) => instance, - createInstance: () => ({}), - createTextInstance: () => ({}), - appendInitialChild: noop, - appendChild: noop, - appendChildToContainer: noop, - insertBefore: noop, - insertInContainerBefore: noop, - removeChild: noop, - removeChildFromContainer: noop, - clearContainer: noop, - commitUpdate: noop, - commitTextUpdate: noop, - hideInstance: noop, - unhideInstance: noop, - hideTextInstance: noop, - unhideTextInstance: noop, - detachDeletedInstance: noop, - finalizeInitialChildren: () => false, - prepareForCommit: () => null, - resetAfterCommit: noop, - shouldSetTextContent: () => false, - preparePortalMount: noop, - getInstanceFromNode: () => null, - beforeActiveInstanceBlur: noop, - afterActiveInstanceBlur: noop, - prepareScopeUpdate: noop, - getInstanceFromScope: () => null, - resetFormInstance: noop, - requestPostPaintCallback: noop, - shouldAttemptEagerTransition: () => false, - trackSchedulerEvent: noop, - resolveEventType: () => null, - resolveEventTimeStamp: () => -1.1, - maySuspendCommit: () => false, - startSuspendingCommit: noop, - suspendInstance: noop, - preloadInstance: () => true, - waitForCommitToBeReady: () => null, - setCurrentUpdatePriority: (next: number) => { - priority = next; - }, - getCurrentUpdatePriority: () => priority, - resolveUpdatePriority: () => (priority !== NoEventPriority ? priority : DiscreteEventPriority), - NotPendingTransition: null, - // React's own context object carries the internal fields the typings ask for. - HostTransitionContext: transitionContext as unknown as ReactContext, - }); - - const container = reconciler.createContainer( - {}, - 0, - null, - false, - null, - '', - noop, - noop, - noop, - noop, - ); - - return { - render: (element: Parameters[0]) => { - reconciler.updateContainerSync(element, container, null, null); - reconciler.flushSyncWork(); - }, - flush: () => reconciler.flushSyncWork(), - }; -} - function setup() { const { render, flush } = createTestRenderer(); let current: ContentCross | undefined;