Branch batch scroll (#12680)

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<typeof import('ink')>();
@@ -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('<Scrollable />', () => {
+  beforeEach(() => {
+    vi.restoreAllMocks();
+  });
+
   it('renders children', () => {
     const { lastFrame } = renderWithProviders(
       <Scrollable hasFocus={false} height={5}>
@@ -52,4 +69,52 @@ describe('<Scrollable />', () => {
     );
     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(
+      <Scrollable hasFocus={true} height={5}>
+        <Text>Line 1</Text>
+        <Text>Line 2</Text>
+        <Text>Line 3</Text>
+        <Text>Line 4</Text>
+        <Text>Line 5</Text>
+        <Text>Line 6</Text>
+        <Text>Line 7</Text>
+        <Text>Line 8</Text>
+        <Text>Line 9</Text>
+        <Text>Line 10</Text>
+      </Scrollable>,
+    );
+
+    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);
+  });
 });

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<ScrollableProps> = ({
     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<ScrollableProps> = ({
 
   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]);

packages/cli/src/ui/components/shared/VirtualizedList.test.tsx

@@ -280,4 +280,48 @@ describe('<VirtualizedList />', () => {
     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<VirtualizedListRef<string>>();
+    const longData = Array.from({ length: 100 }, (_, i) => `Item ${i}`);
+    const itemHeight = 1;
+    const renderItem1px = ({ item }: { item: string }) => (
+      <Box height={itemHeight}>
+        <Text>{item}</Text>
+      </Box>
+    );
+    const keyExtractor = (item: string) => item;
+
+    render(
+      <Box height={10} width={100} borderStyle="round">
+        <VirtualizedList
+          ref={ref}
+          data={longData}
+          renderItem={renderItem1px}
+          keyExtractor={keyExtractor}
+          estimatedItemHeight={() => itemHeight}
+        />
+      </Box>,
+    );
+    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);
+  });
 });

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<T>(
     }
   }
 
+  const { getScrollTop, setPendingScrollTop } = useBatchedScroll(scrollTop);
+
   useImperativeHandle(
     ref,
     () => ({
@@ -370,7 +373,7 @@ function VirtualizedList<T>(
         if (delta < 0) {
           setIsStickingToBottom(false);
         }
-        const currentScrollTop = scrollTop;
+        const currentScrollTop = getScrollTop();
         const newScrollTop = Math.max(
           0,
           Math.min(
@@ -378,6 +381,7 @@ function VirtualizedList<T>(
             currentScrollTop + delta,
           ),
         );
+        setPendingScrollTop(newScrollTop);
         setScrollAnchor(getAnchorForScrollTop(newScrollTop, offsets));
       },
       scrollTo: (offset: number) => {
@@ -386,6 +390,7 @@ function VirtualizedList<T>(
           0,
           Math.min(totalHeight - scrollableContainerHeight, offset),
         );
+        setPendingScrollTop(newScrollTop);
         setScrollAnchor(getAnchorForScrollTop(newScrollTop, offsets));
       },
       scrollToEnd: () => {
@@ -416,6 +421,7 @@ function VirtualizedList<T>(
               offset - viewPosition * scrollableContainerHeight + viewOffset,
             ),
           );
+          setPendingScrollTop(newScrollTop);
           setScrollAnchor(getAnchorForScrollTop(newScrollTop, offsets));
         }
       },
@@ -440,13 +446,14 @@ function VirtualizedList<T>(
                 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<T>(
       getAnchorForScrollTop,
       data,
       scrollableContainerHeight,
-      scrollTop,
+      getScrollTop,
+      setPendingScrollTop,
       containerHeight,
     ],
   );

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<typeof import('ink')>();
+  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<DOMElement>(null);
+    useImperativeHandle(ref, () => elementRef.current);
+
+    useScrollable(
+      {
+        ref: elementRef as RefObject<DOMElement>,
+        getScrollState: props.getScrollState,
+        scrollBy: props.scrollBy,
+        hasFocus: () => true,
+        flashScrollbar: () => {},
+      },
+      true,
+    );
+
+    return <Box ref={elementRef} />;
+  },
+);
+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(
+      <ScrollProvider>
+        <TestScrollable
+          id="test-scrollable"
+          scrollBy={scrollBy}
+          getScrollState={getScrollState}
+        />
+      </ScrollProvider>,
+    );
+
+    // 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(
+      <ScrollProvider>
+        <TestScrollable
+          id="test-scrollable"
+          scrollBy={scrollBy}
+          getScrollState={getScrollState}
+        />
+      </ScrollProvider>,
+    );
+
+    // 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(
+      <ScrollProvider>
+        <TestScrollable
+          id="test-scrollable"
+          scrollBy={scrollBy}
+          getScrollState={getScrollState}
+        />
+      </ScrollProvider>,
+    );
+
+    // 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);
+  });
+});

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<string, number>());
+  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;
       }
     }

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);
+  });
+});

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<number | null>(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 };
+}