From f6499487132f82cc6b498d9e51c939d28ccc2c70 Mon Sep 17 00:00:00 2001 From: Jacob Richman Date: Sat, 8 Nov 2025 16:09:22 -0800 Subject: [PATCH] Branch batch scroll (#12680) --- .../ui/components/shared/Scrollable.test.tsx | 67 ++++- .../src/ui/components/shared/Scrollable.tsx | 20 +- .../shared/VirtualizedList.test.tsx | 44 ++++ .../ui/components/shared/VirtualizedList.tsx | 14 +- .../src/ui/contexts/ScrollProvider.test.tsx | 237 ++++++++++++++++++ .../cli/src/ui/contexts/ScrollProvider.tsx | 32 ++- .../cli/src/ui/hooks/useBatchedScroll.test.ts | 86 +++++++ packages/cli/src/ui/hooks/useBatchedScroll.ts | 35 +++ 8 files changed, 519 insertions(+), 16 deletions(-) create mode 100644 packages/cli/src/ui/contexts/ScrollProvider.test.tsx create mode 100644 packages/cli/src/ui/hooks/useBatchedScroll.test.ts create mode 100644 packages/cli/src/ui/hooks/useBatchedScroll.ts diff --git a/packages/cli/src/ui/components/shared/Scrollable.test.tsx b/packages/cli/src/ui/components/shared/Scrollable.test.tsx index 09e27f6af9..22c2055f49 100644 --- a/packages/cli/src/ui/components/shared/Scrollable.test.tsx +++ b/packages/cli/src/ui/components/shared/Scrollable.test.tsx @@ -7,7 +7,9 @@ import { renderWithProviders } from '../../../test-utils/render.js'; import { Scrollable } from './Scrollable.js'; import { Text } from 'ink'; -import { describe, it, expect, vi } from 'vitest'; +import { describe, it, expect, vi, beforeEach } from 'vitest'; +import * as ScrollProviderModule from '../../contexts/ScrollProvider.js'; +import { act } from 'react'; vi.mock('ink', async (importOriginal) => { const actual = await importOriginal(); @@ -19,7 +21,22 @@ vi.mock('ink', async (importOriginal) => { }; }); +vi.mock('../../hooks/useAnimatedScrollbar.js', () => ({ + useAnimatedScrollbar: ( + hasFocus: boolean, + scrollBy: (delta: number) => void, + ) => ({ + scrollbarColor: 'white', + flashScrollbar: vi.fn(), + scrollByWithAnimation: scrollBy, + }), +})); + describe('', () => { + beforeEach(() => { + vi.restoreAllMocks(); + }); + it('renders children', () => { const { lastFrame } = renderWithProviders( @@ -52,4 +69,52 @@ describe('', () => { ); expect(lastFrame()).toMatchSnapshot(); }); + + it('updates scroll position correctly when scrollBy is called multiple times in the same tick', () => { + let capturedEntry: ScrollProviderModule.ScrollableEntry | undefined; + vi.spyOn(ScrollProviderModule, 'useScrollable').mockImplementation( + (entry, isActive) => { + if (isActive) { + capturedEntry = entry as ScrollProviderModule.ScrollableEntry; + } + }, + ); + + renderWithProviders( + + Line 1 + Line 2 + Line 3 + Line 4 + Line 5 + Line 6 + Line 7 + Line 8 + Line 9 + Line 10 + , + ); + + expect(capturedEntry).toBeDefined(); + + if (!capturedEntry) { + throw new Error('capturedEntry is undefined'); + } + + // Initial state (starts at bottom because of auto-scroll logic) + expect(capturedEntry.getScrollState().scrollTop).toBe(5); + + // Call scrollBy multiple times (upwards) in the same tick + act(() => { + capturedEntry!.scrollBy(-1); + capturedEntry!.scrollBy(-1); + }); + // Should have moved up by 2 + expect(capturedEntry.getScrollState().scrollTop).toBe(3); + + act(() => { + capturedEntry!.scrollBy(-2); + }); + expect(capturedEntry.getScrollState().scrollTop).toBe(1); + }); }); diff --git a/packages/cli/src/ui/components/shared/Scrollable.tsx b/packages/cli/src/ui/components/shared/Scrollable.tsx index 57bfd32a65..16436be7c6 100644 --- a/packages/cli/src/ui/components/shared/Scrollable.tsx +++ b/packages/cli/src/ui/components/shared/Scrollable.tsx @@ -16,6 +16,7 @@ import { Box, getInnerHeight, getScrollHeight, type DOMElement } from 'ink'; import { useKeypress, type Key } from '../../hooks/useKeypress.js'; import { useScrollable } from '../../contexts/ScrollProvider.js'; import { useAnimatedScrollbar } from '../../hooks/useAnimatedScrollbar.js'; +import { useBatchedScroll } from '../../hooks/useBatchedScroll.js'; interface ScrollableProps { children?: React.ReactNode; @@ -81,17 +82,20 @@ export const Scrollable: React.FC = ({ childrenCountRef.current = childCountCurrent; }); + const { getScrollTop, setPendingScrollTop } = useBatchedScroll(scrollTop); + const scrollBy = useCallback( (delta: number) => { const { scrollHeight, innerHeight } = sizeRef.current; - setScrollTop((prev: number) => - Math.min( - Math.max(0, prev + delta), - Math.max(0, scrollHeight - innerHeight), - ), + const current = getScrollTop(); + const next = Math.min( + Math.max(0, current + delta), + Math.max(0, scrollHeight - innerHeight), ); + setPendingScrollTop(next); + setScrollTop(next); }, - [sizeRef], + [sizeRef, getScrollTop, setPendingScrollTop], ); const { scrollbarColor, flashScrollbar, scrollByWithAnimation } = @@ -113,11 +117,11 @@ export const Scrollable: React.FC = ({ const getScrollState = useCallback( () => ({ - scrollTop, + scrollTop: getScrollTop(), scrollHeight: size.scrollHeight, innerHeight: size.innerHeight, }), - [scrollTop, size.scrollHeight, size.innerHeight], + [getScrollTop, size.scrollHeight, size.innerHeight], ); const hasFocusCallback = useCallback(() => hasFocus, [hasFocus]); diff --git a/packages/cli/src/ui/components/shared/VirtualizedList.test.tsx b/packages/cli/src/ui/components/shared/VirtualizedList.test.tsx index 241a02df62..88fba88bfb 100644 --- a/packages/cli/src/ui/components/shared/VirtualizedList.test.tsx +++ b/packages/cli/src/ui/components/shared/VirtualizedList.test.tsx @@ -280,4 +280,48 @@ describe('', () => { expect(lastFrame()).toContain('Item 1'); expect(lastFrame()).toContain('Item 9'); }); + + it('updates scroll position correctly when scrollBy is called multiple times in the same tick', async () => { + const ref = createRef>(); + const longData = Array.from({ length: 100 }, (_, i) => `Item ${i}`); + const itemHeight = 1; + const renderItem1px = ({ item }: { item: string }) => ( + + {item} + + ); + const keyExtractor = (item: string) => item; + + render( + + itemHeight} + /> + , + ); + await act(async () => { + await delay(0); + }); + + expect(ref.current?.getScrollState().scrollTop).toBe(0); + + await act(async () => { + ref.current?.scrollBy(1); + ref.current?.scrollBy(1); + await delay(0); + }); + + expect(ref.current?.getScrollState().scrollTop).toBe(2); + + await act(async () => { + ref.current?.scrollBy(2); + await delay(0); + }); + + expect(ref.current?.getScrollState().scrollTop).toBe(4); + }); }); diff --git a/packages/cli/src/ui/components/shared/VirtualizedList.tsx b/packages/cli/src/ui/components/shared/VirtualizedList.tsx index 598234a308..02adae3e5d 100644 --- a/packages/cli/src/ui/components/shared/VirtualizedList.tsx +++ b/packages/cli/src/ui/components/shared/VirtualizedList.tsx @@ -16,6 +16,7 @@ import { } from 'react'; import type React from 'react'; import { theme } from '../../semantic-colors.js'; +import { useBatchedScroll } from '../../hooks/useBatchedScroll.js'; import { type DOMElement, measureElement, Box } from 'ink'; @@ -363,6 +364,8 @@ function VirtualizedList( } } + const { getScrollTop, setPendingScrollTop } = useBatchedScroll(scrollTop); + useImperativeHandle( ref, () => ({ @@ -370,7 +373,7 @@ function VirtualizedList( if (delta < 0) { setIsStickingToBottom(false); } - const currentScrollTop = scrollTop; + const currentScrollTop = getScrollTop(); const newScrollTop = Math.max( 0, Math.min( @@ -378,6 +381,7 @@ function VirtualizedList( currentScrollTop + delta, ), ); + setPendingScrollTop(newScrollTop); setScrollAnchor(getAnchorForScrollTop(newScrollTop, offsets)); }, scrollTo: (offset: number) => { @@ -386,6 +390,7 @@ function VirtualizedList( 0, Math.min(totalHeight - scrollableContainerHeight, offset), ); + setPendingScrollTop(newScrollTop); setScrollAnchor(getAnchorForScrollTop(newScrollTop, offsets)); }, scrollToEnd: () => { @@ -416,6 +421,7 @@ function VirtualizedList( offset - viewPosition * scrollableContainerHeight + viewOffset, ), ); + setPendingScrollTop(newScrollTop); setScrollAnchor(getAnchorForScrollTop(newScrollTop, offsets)); } }, @@ -440,13 +446,14 @@ function VirtualizedList( offset - viewPosition * scrollableContainerHeight + viewOffset, ), ); + setPendingScrollTop(newScrollTop); setScrollAnchor(getAnchorForScrollTop(newScrollTop, offsets)); } } }, getScrollIndex: () => scrollAnchor.index, getScrollState: () => ({ - scrollTop, + scrollTop: getScrollTop(), scrollHeight: totalHeight, innerHeight: containerHeight, }), @@ -458,7 +465,8 @@ function VirtualizedList( getAnchorForScrollTop, data, scrollableContainerHeight, - scrollTop, + getScrollTop, + setPendingScrollTop, containerHeight, ], ); diff --git a/packages/cli/src/ui/contexts/ScrollProvider.test.tsx b/packages/cli/src/ui/contexts/ScrollProvider.test.tsx new file mode 100644 index 0000000000..6ad43742c3 --- /dev/null +++ b/packages/cli/src/ui/contexts/ScrollProvider.test.tsx @@ -0,0 +1,237 @@ +/** + * @license + * Copyright 2025 Google LLC + * SPDX-License-Identifier: Apache-2.0 + */ + +import { render } from '../../test-utils/render.js'; +import { + ScrollProvider, + useScrollable, + type ScrollState, +} from './ScrollProvider.js'; +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import { useRef, useImperativeHandle, forwardRef, type RefObject } from 'react'; +import { Box, type DOMElement } from 'ink'; +import type { MouseEvent } from '../hooks/useMouse.js'; + +// Mock useMouse hook +const mockUseMouseCallbacks = new Set<(event: MouseEvent) => void>(); +vi.mock('../hooks/useMouse.js', async () => { + // We need to import React dynamically because this factory runs before top-level imports + const React = await import('react'); + return { + useMouse: (callback: (event: MouseEvent) => void) => { + React.useEffect(() => { + mockUseMouseCallbacks.add(callback); + return () => { + mockUseMouseCallbacks.delete(callback); + }; + }, [callback]); + }, + }; +}); + +// Mock ink's getBoundingBox +vi.mock('ink', async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + getBoundingBox: vi.fn(() => ({ x: 0, y: 0, width: 10, height: 10 })), + }; +}); + +const TestScrollable = forwardRef( + ( + props: { + id: string; + scrollBy: (delta: number) => void; + getScrollState: () => ScrollState; + }, + ref, + ) => { + const elementRef = useRef(null); + useImperativeHandle(ref, () => elementRef.current); + + useScrollable( + { + ref: elementRef as RefObject, + getScrollState: props.getScrollState, + scrollBy: props.scrollBy, + hasFocus: () => true, + flashScrollbar: () => {}, + }, + true, + ); + + return ; + }, +); +TestScrollable.displayName = 'TestScrollable'; + +describe('ScrollProvider', () => { + beforeEach(() => { + vi.useFakeTimers(); + mockUseMouseCallbacks.clear(); + }); + + afterEach(() => { + vi.useRealTimers(); + }); + + it('batches multiple scroll events into a single update', async () => { + const scrollBy = vi.fn(); + const getScrollState = vi.fn(() => ({ + scrollTop: 0, + scrollHeight: 100, + innerHeight: 10, + })); + + render( + + + , + ); + + // Simulate multiple scroll events + const mouseEvent: MouseEvent = { + name: 'scroll-down', + col: 5, + row: 5, + shift: false, + ctrl: false, + meta: false, + }; + for (const callback of mockUseMouseCallbacks) { + callback(mouseEvent); + callback(mouseEvent); + callback(mouseEvent); + } + + // Should not have called scrollBy yet + expect(scrollBy).not.toHaveBeenCalled(); + + // Advance timers to trigger the batched update + await vi.runAllTimersAsync(); + + // Should have called scrollBy once with accumulated delta (3) + expect(scrollBy).toHaveBeenCalledTimes(1); + expect(scrollBy).toHaveBeenCalledWith(3); + }); + + it('handles mixed direction scroll events in batch', async () => { + const scrollBy = vi.fn(); + const getScrollState = vi.fn(() => ({ + scrollTop: 10, + scrollHeight: 100, + innerHeight: 10, + })); + + render( + + + , + ); + + // Simulate mixed scroll events: down (1), down (1), up (-1) + for (const callback of mockUseMouseCallbacks) { + callback({ + name: 'scroll-down', + col: 5, + row: 5, + shift: false, + ctrl: false, + meta: false, + }); + callback({ + name: 'scroll-down', + col: 5, + row: 5, + shift: false, + ctrl: false, + meta: false, + }); + callback({ + name: 'scroll-up', + col: 5, + row: 5, + shift: false, + ctrl: false, + meta: false, + }); + } + + expect(scrollBy).not.toHaveBeenCalled(); + + await vi.runAllTimersAsync(); + + expect(scrollBy).toHaveBeenCalledTimes(1); + expect(scrollBy).toHaveBeenCalledWith(1); // 1 + 1 - 1 = 1 + }); + + it('respects scroll limits during batching', async () => { + const scrollBy = vi.fn(); + // Start near bottom + const getScrollState = vi.fn(() => ({ + scrollTop: 89, + scrollHeight: 100, + innerHeight: 10, + })); + + render( + + + , + ); + + // Try to scroll down 3 times, but only 1 is allowed before hitting bottom + for (const callback of mockUseMouseCallbacks) { + callback({ + name: 'scroll-down', + col: 5, + row: 5, + shift: false, + ctrl: false, + meta: false, + }); + callback({ + name: 'scroll-down', + col: 5, + row: 5, + shift: false, + ctrl: false, + meta: false, + }); + callback({ + name: 'scroll-down', + col: 5, + row: 5, + shift: false, + ctrl: false, + meta: false, + }); + } + + await vi.runAllTimersAsync(); + + // Should have accumulated only 1, because subsequent scrolls would be blocked + // Actually, the logic in ScrollProvider uses effectiveScrollTop to check bounds. + // scrollTop=89, max=90. + // 1st scroll: pending=1, effective=90. Allowed. + // 2nd scroll: pending=1, effective=90. canScrollDown checks effective < 90. 90 < 90 is false. Blocked. + expect(scrollBy).toHaveBeenCalledTimes(1); + expect(scrollBy).toHaveBeenCalledWith(1); + }); +}); diff --git a/packages/cli/src/ui/contexts/ScrollProvider.tsx b/packages/cli/src/ui/contexts/ScrollProvider.tsx index b11d25431b..0018a03027 100644 --- a/packages/cli/src/ui/contexts/ScrollProvider.tsx +++ b/packages/cli/src/ui/contexts/ScrollProvider.tsx @@ -95,6 +95,25 @@ export const ScrollProvider: React.FC<{ children: React.ReactNode }> = ({ scrollablesRef.current = scrollables; }, [scrollables]); + const pendingScrollsRef = useRef(new Map()); + const flushScheduledRef = useRef(false); + + const scheduleFlush = useCallback(() => { + if (!flushScheduledRef.current) { + flushScheduledRef.current = true; + setTimeout(() => { + flushScheduledRef.current = false; + for (const [id, delta] of pendingScrollsRef.current.entries()) { + const entry = scrollablesRef.current.get(id); + if (entry) { + entry.scrollBy(delta); + } + } + pendingScrollsRef.current.clear(); + }, 0); + } + }, []); + const handleScroll = (direction: 'up' | 'down', mouseEvent: MouseEvent) => { const delta = direction === 'up' ? -1 : 1; const candidates = findScrollableCandidates( @@ -105,18 +124,23 @@ export const ScrollProvider: React.FC<{ children: React.ReactNode }> = ({ for (const candidate of candidates) { const { scrollTop, scrollHeight, innerHeight } = candidate.getScrollState(); + const pendingDelta = pendingScrollsRef.current.get(candidate.id) || 0; + const effectiveScrollTop = scrollTop + pendingDelta; // Epsilon to handle floating point inaccuracies. - const canScrollUp = scrollTop > 0.001; - const canScrollDown = scrollTop < scrollHeight - innerHeight - 0.001; + const canScrollUp = effectiveScrollTop > 0.001; + const canScrollDown = + effectiveScrollTop < scrollHeight - innerHeight - 0.001; if (direction === 'up' && canScrollUp) { - candidate.scrollBy(delta); + pendingScrollsRef.current.set(candidate.id, pendingDelta + delta); + scheduleFlush(); return; } if (direction === 'down' && canScrollDown) { - candidate.scrollBy(delta); + pendingScrollsRef.current.set(candidate.id, pendingDelta + delta); + scheduleFlush(); return; } } diff --git a/packages/cli/src/ui/hooks/useBatchedScroll.test.ts b/packages/cli/src/ui/hooks/useBatchedScroll.test.ts new file mode 100644 index 0000000000..268c5b6bfa --- /dev/null +++ b/packages/cli/src/ui/hooks/useBatchedScroll.test.ts @@ -0,0 +1,86 @@ +/** + * @license + * Copyright 2025 Google LLC + * SPDX-License-Identifier: Apache-2.0 + */ + +import { describe, it, expect } from 'vitest'; +import { renderHook } from '../../test-utils/render.js'; +import { useBatchedScroll } from './useBatchedScroll.js'; + +describe('useBatchedScroll', () => { + it('returns initial scrollTop', () => { + const { result } = renderHook(() => useBatchedScroll(10)); + expect(result.current.getScrollTop()).toBe(10); + }); + + it('returns updated scrollTop from props', () => { + let currentScrollTop = 10; + const { result, rerender } = renderHook(() => + useBatchedScroll(currentScrollTop), + ); + + expect(result.current.getScrollTop()).toBe(10); + + currentScrollTop = 100; + rerender(); + + expect(result.current.getScrollTop()).toBe(100); + }); + + it('returns pending scrollTop when set', () => { + const { result } = renderHook(() => useBatchedScroll(10)); + + result.current.setPendingScrollTop(50); + expect(result.current.getScrollTop()).toBe(50); + }); + + it('overwrites pending scrollTop with subsequent sets before render', () => { + const { result } = renderHook(() => useBatchedScroll(10)); + + result.current.setPendingScrollTop(50); + result.current.setPendingScrollTop(75); + expect(result.current.getScrollTop()).toBe(75); + }); + + it('resets pending scrollTop after rerender', () => { + let currentScrollTop = 10; + const { result, rerender } = renderHook(() => + useBatchedScroll(currentScrollTop), + ); + + result.current.setPendingScrollTop(50); + expect(result.current.getScrollTop()).toBe(50); + + // Rerender with new prop + currentScrollTop = 100; + rerender(); + + // Should now be the new prop value, pending should be cleared + expect(result.current.getScrollTop()).toBe(100); + }); + + it('resets pending scrollTop after rerender even if prop is same', () => { + const { result, rerender } = renderHook(() => useBatchedScroll(10)); + + result.current.setPendingScrollTop(50); + expect(result.current.getScrollTop()).toBe(50); + + // Rerender with same prop + rerender(); + + // Pending should still be cleared because useEffect runs after every render + expect(result.current.getScrollTop()).toBe(10); + }); + + it('maintains stable function references', () => { + const { result, rerender } = renderHook(() => useBatchedScroll(10)); + const initialGetScrollTop = result.current.getScrollTop; + const initialSetPendingScrollTop = result.current.setPendingScrollTop; + + rerender(); + + expect(result.current.getScrollTop).toBe(initialGetScrollTop); + expect(result.current.setPendingScrollTop).toBe(initialSetPendingScrollTop); + }); +}); diff --git a/packages/cli/src/ui/hooks/useBatchedScroll.ts b/packages/cli/src/ui/hooks/useBatchedScroll.ts new file mode 100644 index 0000000000..05b73a9068 --- /dev/null +++ b/packages/cli/src/ui/hooks/useBatchedScroll.ts @@ -0,0 +1,35 @@ +/** + * @license + * Copyright 2025 Google LLC + * SPDX-License-Identifier: Apache-2.0 + */ + +import { useRef, useEffect, useCallback } from 'react'; + +/** + * A hook to manage batched scroll state updates. + * It allows multiple scroll operations within the same tick to accumulate + * by keeping track of a 'pending' state that resets after render. + */ +export function useBatchedScroll(currentScrollTop: number) { + const pendingScrollTopRef = useRef(null); + // We use a ref for currentScrollTop to allow getScrollTop to be stable + // and not depend on the currentScrollTop value directly in its dependency array. + const currentScrollTopRef = useRef(currentScrollTop); + + useEffect(() => { + currentScrollTopRef.current = currentScrollTop; + pendingScrollTopRef.current = null; + }); + + const getScrollTop = useCallback( + () => pendingScrollTopRef.current ?? currentScrollTopRef.current, + [], + ); + + const setPendingScrollTop = useCallback((newScrollTop: number) => { + pendingScrollTopRef.current = newScrollTop; + }, []); + + return { getScrollTop, setPendingScrollTop }; +}