From 1828fafb25d2d86b3bdb5d0d38dcc19ba1af12b5 Mon Sep 17 00:00:00 2001 From: Jacob Richman Date: Thu, 14 May 2026 16:00:26 -0700 Subject: [PATCH] Bug fixes. --- .../shared/VirtualizedList.test.tsx | 118 +++++++++++++++++- .../ui/components/shared/VirtualizedList.tsx | 72 +++++++---- 2 files changed, 167 insertions(+), 23 deletions(-) diff --git a/packages/cli/src/ui/components/shared/VirtualizedList.test.tsx b/packages/cli/src/ui/components/shared/VirtualizedList.test.tsx index 594b56eeb5..eb7a77848a 100644 --- a/packages/cli/src/ui/components/shared/VirtualizedList.test.tsx +++ b/packages/cli/src/ui/components/shared/VirtualizedList.test.tsx @@ -6,7 +6,11 @@ import { renderWithProviders as render } from '../../../test-utils/render.js'; import { waitFor } from '../../../test-utils/async.js'; -import { VirtualizedList, type VirtualizedListRef } from './VirtualizedList.js'; +import { + SCROLL_TO_ITEM_END, + VirtualizedList, + type VirtualizedListRef, +} from './VirtualizedList.js'; import { Text, Box } from 'ink'; import { createRef, @@ -417,6 +421,118 @@ describe('', () => { unmount(); }); + it('culls the backbuffer by measured row height instead of item count', async () => { + const longData = Array.from({ length: 100 }, (_, i) => `Item ${i}`); + const renderedIndices = new Set(); + const ref = createRef>(); + const { unmount, waitUntilReady } = await render( + + { + renderedIndices.add(index); + return ( + + {item} + + ); + }} + keyExtractor={(item) => item} + estimatedItemHeight={() => 2} + initialScrollIndex={99} + overflowToBackbuffer={true} + maxScrollbackLength={10} + /> + , + ); + + await waitUntilReady(); + + const state = ref.current?.getScrollState(); + expect(state?.scrollHeight).toBe(20); + expect(state?.innerHeight).toBe(10); + expect(renderedIndices.has(90)).toBe(true); + expect(renderedIndices.has(85)).toBe(false); + + unmount(); + }); + + it('keeps keyboard scrolling in logical history coordinates after culling', async () => { + const longData = Array.from({ length: 100 }, (_, i) => `Item ${i}`); + const ref = createRef>(); + const { lastFrame, unmount, waitUntilReady } = await render( + + ( + + {item} + + )} + keyExtractor={(item) => item} + estimatedItemHeight={() => 1} + initialScrollIndex={99} + overflowToBackbuffer={true} + maxScrollbackLength={10} + /> + , + ); + + await waitUntilReady(); + + expect(ref.current?.getScrollState().scrollTop).toBe(10); + + await act(async () => { + ref.current?.scrollBy(-1); + }); + await waitUntilReady(); + + const state = ref.current?.getScrollState(); + expect(state?.scrollTop).toBeGreaterThan(0); + expect(lastFrame()).not.toContain('Item 79'); + expect(lastFrame()).not.toContain('Item 80'); + + unmount(); + }); + + it('measures mounted zero-height items instead of keeping their estimate', async () => { + const ref = createRef>(); + const data = ['Item 0', 'Item 1', 'pending']; + const { unmount, waitUntilReady } = await render( + + + item === 'pending' ? ( + + ) : ( + + {item} + + ) + } + keyExtractor={(item) => item} + estimatedItemHeight={() => 10} + initialScrollIndex={2} + initialScrollOffsetInIndex={SCROLL_TO_ITEM_END} + /> + , + ); + + await waitUntilReady(); + + expect(ref.current?.getScrollState()).toEqual({ + scrollTop: 0, + scrollHeight: 2, + innerHeight: 50, + }); + + unmount(); + }); + it('does not forget item heights when items are prepended', async () => { const ref = createRef>(); const data = ['Item 1', 'Item 2']; diff --git a/packages/cli/src/ui/components/shared/VirtualizedList.tsx b/packages/cli/src/ui/components/shared/VirtualizedList.tsx index 1611a970b0..0d9e2ecdce 100644 --- a/packages/cli/src/ui/components/shared/VirtualizedList.tsx +++ b/packages/cli/src/ui/components/shared/VirtualizedList.tsx @@ -120,6 +120,25 @@ function findLastIndex( return -1; } +function findOffsetIndexAtOrBefore(offsets: number[], target: number): number { + let low = 0; + let high = offsets.length - 1; + let result = 0; + + while (low <= high) { + const mid = Math.floor((low + high) / 2); + const offset = offsets[mid] ?? 0; + if (offset <= target) { + result = mid; + low = mid + 1; + } else { + high = mid - 1; + } + } + + return Math.min(result, Math.max(0, offsets.length - 2)); +} + const extractClickableAreas = (rootNode: DOMElement): ClickableArea[] => { const rootBox = getBoundingBox(rootNode); const results: ClickableArea[] = []; @@ -394,7 +413,7 @@ function VirtualizedList( }; const height = Math.round(currentHack.yogaNode?.getComputedHeight() ?? 0); if ( - height > 0 && + height >= 0 && (state.current.measuredHeights[index] !== height || state.current.measuredKeys[index] !== key) ) { @@ -440,9 +459,9 @@ function VirtualizedList( const key = target._virtualKey; if (typeof index === 'number' && key !== undefined) { const height = Math.round(entry.contentRect.height); - // Ignore 0 height measurements which can happen when an element is unmounting if ( - height > 0 && + height >= 0 && + state.current.itemRefs[index] === entry.target && (state.current.measuredHeights[index] !== height || state.current.measuredKeys[index] !== key) ) { @@ -551,13 +570,14 @@ function VirtualizedList( if (isNearBottom) { const scrollBottom = scrollTop + scrollableContainerHeight; - const index = findLastIndex( + const rawIndex = findLastIndex( offsets, (offset) => offset <= scrollBottom, ); - if (index === -1) { + if (rawIndex === -1) { return { index: 0, offset: 0, isBottom: true }; } + const index = Math.min(rawIndex, Math.max(0, offsets.length - 2)); return { index, offset: scrollBottom - offsets[index], @@ -565,10 +585,11 @@ function VirtualizedList( }; } - const index = findLastIndex(offsets, (offset) => offset <= scrollTop); - if (index === -1) { + const rawIndex = findLastIndex(offsets, (offset) => offset <= scrollTop); + if (rawIndex === -1) { return { index: 0, offset: 0 }; } + const index = Math.min(rawIndex, Math.max(0, offsets.length - 2)); return { index, offset: scrollTop - offsets[index] }; }, @@ -646,24 +667,28 @@ function VirtualizedList( ? data.length - 1 : Math.min(data.length - 1, endIndexOffset); - const culledHeight = useMemo(() => { + const backbufferStartIndex = useMemo(() => { if ( overflowToBackbuffer && typeof maxScrollbackLength === 'number' && maxScrollbackLength > 0 ) { - // Keep maxScrollbackLength items before the viewport to satisfy the backbuffer budget. - // We use items as a proxy for lines to be robust against estimation errors. - // We add 1 to startIndex to account for the 1-item overscan it includes. - const targetIndex = Math.max(0, startIndex + 1 - maxScrollbackLength); - return offsets[targetIndex] ?? 0; + // Cull at measured item boundaries. If the target line falls inside a + // tall item, keep that whole item so the backbuffer has no blank gap. + const targetOffset = Math.max(0, actualScrollTop - maxScrollbackLength); + return findOffsetIndexAtOrBefore(offsets, targetOffset); } return 0; - }, [overflowToBackbuffer, maxScrollbackLength, startIndex, offsets]); + }, [overflowToBackbuffer, maxScrollbackLength, actualScrollTop, offsets]); - const scrollTop = isStickingToBottom + const culledHeight = + overflowToBackbuffer && maxScrollbackLength > 0 + ? (offsets[backbufferStartIndex] ?? 0) + : 0; + + const logicalScrollTop = isStickingToBottom ? Number.MAX_SAFE_INTEGER - : actualScrollTop - culledHeight; + : actualScrollTop; useLayoutEffect(() => { if (state.current.prevDataLength === -1) { @@ -842,15 +867,17 @@ function VirtualizedList( const renderRangeStart = useMemo(() => { if (overflowToBackbuffer) { if (typeof maxScrollbackLength === 'number' && maxScrollbackLength > 0) { - // We render everything from the culledHeight boundary to ensure the - // backbuffer is fully populated. - const targetIndex = Math.max(0, startIndex + 1 - maxScrollbackLength); - return targetIndex; + return backbufferStartIndex; } return 0; } return startIndex; - }, [overflowToBackbuffer, maxScrollbackLength, startIndex]); + }, [ + overflowToBackbuffer, + maxScrollbackLength, + backbufferStartIndex, + startIndex, + ]); const topSpacerHeight = Math.max(0, offsets[renderRangeStart] - culledHeight); @@ -1039,7 +1066,8 @@ function VirtualizedList( toggledKeys, ]); - const { getScrollTop, setPendingScrollTop } = useBatchedScroll(scrollTop); + const { getScrollTop, setPendingScrollTop } = + useBatchedScroll(logicalScrollTop); const { broadcast } = useMouseContext();