fix(vim): keep the j/k cursor clear of the HUD bands when scrolling

packages/ui/components/Viewer.tsx

@@ -248,6 +248,9 @@ export const Viewer = forwardRef<ViewerHandle, ViewerProps>(({
     }
   };
   const containerRef = useRef<HTMLDivElement>(null);
+  // The element that actually scrolls; shared by the Vim scroll math, the
+  // sticky-header observer, and the reticle geometry.
+  const scrollViewport = useScrollViewport();
   // The badge cluster (repo chips / diff badge) is absolutely positioned in the
   // card's top padding. One row fits; a second row (diff badge) or mobile
   // wrapping outgrows the padding and lands on the document's first heading.
@@ -461,6 +464,7 @@ export const Viewer = forwardRef<ViewerHandle, ViewerProps>(({
   }, []);
   const vim = useVimSelection({
     containerRef,
+    scrollViewport,
     enabled: vimModeActive,
     hudEnabled: vimHudEnabled,
     blocked: vimBlocked,
@@ -535,16 +539,15 @@ export const Viewer = forwardRef<ViewerHandle, ViewerProps>(({
   // Detect when sticky action bar is "stuck" to show card background.
   // The IntersectionObserver root must be the actual scroll element — the
   // OverlayScrollArea viewport — not the <main> host, which doesn't scroll.
-  const stickyScrollViewport = useScrollViewport();
   useEffect(() => {
-    if (!stickyActions || !stickySentinelRef.current || !stickyScrollViewport) return;
+    if (!stickyActions || !stickySentinelRef.current || !scrollViewport) return;
     const observer = new IntersectionObserver(
       ([entry]) => setIsStuck(!entry.isIntersecting),
-      { root: stickyScrollViewport, threshold: 0 }
+      { root: scrollViewport, threshold: 0 }
     );
     observer.observe(stickySentinelRef.current);
     return () => observer.disconnect();
-  }, [stickyActions, stickyScrollViewport]);
+  }, [stickyActions, scrollViewport]);
 
   useEffect(() => {
     const handleHashChange = () => {
@@ -561,7 +564,7 @@ export const Viewer = forwardRef<ViewerHandle, ViewerProps>(({
     if (!anchor) return false;
 
     const container = containerRef.current;
-    if (!container || !stickyScrollViewport) return false;
+    if (!container || !scrollViewport) return false;
 
     const target = document.getElementById(anchor);
     if (!target || !container.contains(target)) return false;
@@ -573,27 +576,27 @@ export const Viewer = forwardRef<ViewerHandle, ViewerProps>(({
     const headerOffset = stickyActionsEl
       ? stickyActionsEl.getBoundingClientRect().height + stickyTop
       : 0;
-    const containerRect = stickyScrollViewport.getBoundingClientRect();
+    const containerRect = scrollViewport.getBoundingClientRect();
     const targetRect = target.getBoundingClientRect();
     const relativeTop = targetRect.top - containerRect.top;
-    const offsetPosition = stickyScrollViewport.scrollTop + relativeTop - headerOffset;
+    const offsetPosition = scrollViewport.scrollTop + relativeTop - headerOffset;
 
-    stickyScrollViewport.scrollTo({
+    scrollViewport.scrollTo({
       top: Math.max(0, offsetPosition),
       behavior: 'smooth',
     });
     return true;
-  }, [stickyScrollViewport]);
+  }, [scrollViewport]);
 
   useEffect(() => {
-    if (!stickyScrollViewport || !locationHash || lastAutoScrolledHashRef.current === locationHash) return;
+    if (!scrollViewport || !locationHash || lastAutoScrolledHashRef.current === locationHash) return;
     const timer = window.setTimeout(() => {
       if (scrollToAnchor(locationHash)) {
         lastAutoScrolledHashRef.current = locationHash;
       }
     }, 0);
     return () => window.clearTimeout(timer);
-  }, [blocks, locationHash, scrollToAnchor, stickyScrollViewport]);
+  }, [blocks, locationHash, scrollToAnchor, scrollViewport]);
 
   // Use the native copy event so clipboard writes are synchronous (Safari
   // rejects the async navigator.clipboard API outside the user-gesture window).

packages/ui/hooks/useVimSelection.ts

@@ -47,11 +47,14 @@ import {
   type VimVisualBlockState,
   type VimVisualState,
 } from '../utils/vimNavigation';
+import { scrollVimTargetIntoView } from '../utils/vimScroll';
 import { useVimDocumentFocus } from './useVimDocumentFocus';
 
 /** Inputs required by the Markdown semantic Vim controller. */
 export interface UseVimSelectionOptions {
   readonly containerRef: RefObject<HTMLElement | null>;
+  /** The element that actually scrolls (ScrollViewportContext value). */
+  readonly scrollViewport?: HTMLElement | null;
   readonly enabled: boolean;
   readonly hudEnabled: boolean;
   readonly blocked: boolean;
@@ -234,6 +237,7 @@ function applyVisualBlockSelection(
  */
 export function useVimSelection({
   containerRef,
+  scrollViewport,
   enabled,
   hudEnabled,
   blocked,
@@ -257,6 +261,11 @@ export function useVimSelection({
   const pointerFocusRef = useRef(false);
   const restoringFocusRef = useRef(false);
 
+  // Read the live scroll viewport without adding a dependency to every
+  // navigation callback below.
+  const scrollViewportRef = useRef(scrollViewport);
+  scrollViewportRef.current = scrollViewport;
+
   const setState = useCallback((next: VimSelectionState) => {
     stateRef.current = next;
     setStateValue(next);
@@ -283,7 +292,7 @@ export function useVimSelection({
     const next: VimBlockState = { phase: 'block', targetKey: initial.key };
     setState(next);
     window.getSelection()?.removeAllRanges();
-    initial.element.scrollIntoView({ block: 'nearest' });
+    scrollVimTargetIntoView(initial.element, scrollViewportRef.current);
     return next;
   }, [containerRef, setState]);
 
@@ -358,7 +367,7 @@ export function useVimSelection({
   const setSemanticTarget = useCallback((target: SemanticTarget) => {
     setState(semanticStateForTarget(target));
     window.getSelection()?.removeAllRanges();
-    target.element.scrollIntoView({ block: 'nearest' });
+    scrollVimTargetIntoView(target.element, scrollViewportRef.current);
   }, [setState]);
 
   const updateTextState = useCallback((
@@ -373,9 +382,9 @@ export function useVimSelection({
       normalized.cursor,
       normalized.phase === 'visual' ? normalized.anchor : null,
     );
-    resolveTextPosition(graph.container, normalized.cursor)
-      ?.node.parentElement
-      ?.scrollIntoView({ block: 'nearest' });
+    const cursorParent = resolveTextPosition(graph.container, normalized.cursor)
+      ?.node.parentElement;
+    if (cursorParent) scrollVimTargetIntoView(cursorParent, scrollViewportRef.current);
   }, [setState]);
 
   const enterTextAtTarget = useCallback((
@@ -420,7 +429,7 @@ export function useVimSelection({
     if (!getTextElementBounds(graph.container, block.element)) return false;
     setState(next);
     applyVisualBlockSelection(graph, next);
-    block.element.scrollIntoView({ block: 'nearest' });
+    scrollVimTargetIntoView(block.element, scrollViewportRef.current);
     return true;
   }, [setState]);
 
@@ -730,7 +739,7 @@ export function useVimSelection({
       };
       setState(nextState);
       applyVisualBlockSelection(graph, nextState);
-      next.element.scrollIntoView({ block: 'nearest' });
+      scrollVimTargetIntoView(next.element, scrollViewportRef.current);
       return true;
     }
     if (key === 'o') {

packages/ui/utils/vimScroll.test.ts

@@ -0,0 +1,195 @@
+import { describe, expect, test } from 'bun:test';
+import {
+  VIM_SCROLL_MARGIN_MAX,
+  VIM_SCROLL_MARGIN_MIN,
+  computeVimScrollDelta,
+  resolveVimScrollMargin,
+  scrollVimTargetIntoView,
+} from './vimScroll';
+
+const hasDom = typeof document !== 'undefined';
+
+// A 1000px-tall viewport whose top edge sits at page y=0, with a 200px HUD band
+// reserved at each edge — the safe band is [200, 800].
+const viewport = { top: 0, height: 1000 };
+const band = { topMargin: 200, bottomMargin: 200 };
+
+describe('computeVimScrollDelta', () => {
+  test('leaves a target already inside the safe band untouched', () => {
+    expect(computeVimScrollDelta(viewport, { top: 400, bottom: 460 }, band)).toBe(0);
+    // Flush against the inner edges of the band still counts as safe.
+    expect(computeVimScrollDelta(viewport, { top: 200, bottom: 800 }, band)).toBe(0);
+  });
+
+  test('reveals a target parked behind the bottom HUD (the j-to-bottom bug)', () => {
+    // scrollIntoView({ block: 'nearest' }) would pin this line at y≈980, behind
+    // the bottom HUD. We instead scroll it down to the bottom margin at y=800.
+    const delta = computeVimScrollDelta(viewport, { top: 960, bottom: 980 }, band);
+    expect(delta).toBe(180); // 980 - (1000 - 200)
+  });
+
+  test('reveals a target parked behind the top HUD (the k-to-top bug)', () => {
+    // A line at y≈20 sits under the sticky action bar; scroll up to the margin.
+    const delta = computeVimScrollDelta(viewport, { top: 20, bottom: 40 }, band);
+    expect(delta).toBe(-180); // 20 - 200
+  });
+
+  test('aligns a target taller than the band to its top edge, not its bottom', () => {
+    // A 700px block (taller than the 600px safe band) sitting low: honouring the
+    // bottom margin alone would push its top above the top margin and hide the
+    // start of the block. Reading order wins — clamp to the top margin instead.
+    const bottomOnly = 940 - 800; // 140 if we only chased the bottom
+    const topRoom = 240 - 200; //   40 before the top slips under the margin
+    const delta = computeVimScrollDelta(viewport, { top: 240, bottom: 940 }, band);
+    expect(delta).toBe(Math.min(bottomOnly, topRoom));
+    expect(delta).toBe(40);
+  });
+
+  test('clears the top HUD for a too-tall target whose top is occluded', () => {
+    // A block spanning 100..900 is taller than the band and straddles both
+    // edges. Its top sits behind the top HUD (100 < 200), so reveal the start
+    // of the block by scrolling up to the top margin — reading order wins.
+    expect(computeVimScrollDelta(viewport, { top: 100, bottom: 900 }, band)).toBe(-100);
+  });
+
+  test('leaves a too-tall target straddling the band untouched once its top is clear', () => {
+    // Top already at the margin, bottom past it: moving either way would hide an
+    // edge, so hold position.
+    expect(computeVimScrollDelta(viewport, { top: 200, bottom: 900 }, band)).toBe(0);
+  });
+
+  test('accounts for a viewport offset from the page top', () => {
+    const offset = { top: 300, height: 400 }; // safe band = page [400, 500]
+    const smallBand = { topMargin: 100, bottomMargin: 200 };
+    // Target at page y=650..680 is below the safe bottom (500) → scroll down 180.
+    expect(computeVimScrollDelta(offset, { top: 650, bottom: 680 }, smallBand)).toBe(180);
+  });
+});
+
+describe('resolveVimScrollMargin', () => {
+  test('uses the 20% ratio in the ordinary range', () => {
+    expect(resolveVimScrollMargin(600)).toBe(120);
+  });
+
+  test('clamps short viewports up to the minimum', () => {
+    expect(resolveVimScrollMargin(50)).toBe(VIM_SCROLL_MARGIN_MIN);
+  });
+
+  test('clamps tall viewports down to the maximum', () => {
+    expect(resolveVimScrollMargin(4000)).toBe(VIM_SCROLL_MARGIN_MAX);
+  });
+});
+
+describe.if(hasDom)('scrollVimTargetIntoView', () => {
+  function stubRect(element: HTMLElement, rect: Partial<DOMRect>): void {
+    const full = { top: 0, bottom: 0, left: 0, right: 0, width: 0, height: 0, x: 0, y: 0 };
+    const merged = { ...full, ...rect } as DOMRect;
+    element.getBoundingClientRect = () => merged;
+  }
+
+  function buildViewport(
+    clientHeight: number,
+    opts: { withAttribute?: boolean } = {},
+  ): {
+    viewport: HTMLElement;
+    target: HTMLElement;
+  } {
+    const viewportEl = document.createElement('div');
+    // The real app scrolls a native <div> with no OverlayScrollbars attribute —
+    // the caller passes the viewport explicitly. Only opt into the attribute
+    // when a test is exercising the closest() fallback.
+    if (opts.withAttribute) {
+      viewportEl.setAttribute('data-overlayscrollbars-viewport', '');
+    }
+    Object.defineProperty(viewportEl, 'clientHeight', {
+      configurable: true,
+      value: clientHeight,
+    });
+    viewportEl.scrollTop = 0;
+    const target = document.createElement('p');
+    viewportEl.appendChild(target);
+    document.body.appendChild(viewportEl);
+    return { viewport: viewportEl, target };
+  }
+
+  test('scrolls a target out of the bottom HUD band via the explicit viewport', () => {
+    const { viewport: viewportEl, target } = buildViewport(1000);
+    stubRect(viewportEl, { top: 0, height: 1000 });
+    // Margin = clamp(1000 * 0.2) = 160. Target at 950..970 is behind it.
+    stubRect(target, { top: 950, bottom: 970, height: 20, width: 100 });
+
+    // The native-scroll host carries no attribute; the caller passes it in.
+    scrollVimTargetIntoView(target, viewportEl);
+
+    // 970 - (1000 - 160) = 130.
+    expect(viewportEl.scrollTop).toBe(130);
+    document.body.replaceChildren();
+  });
+
+  test('falls back to the OverlayScrollbars attribute when no viewport is passed', () => {
+    const { viewport: viewportEl, target } = buildViewport(1000, {
+      withAttribute: true,
+    });
+    stubRect(viewportEl, { top: 0, height: 1000 });
+    stubRect(target, { top: 950, bottom: 970, height: 20, width: 100 });
+
+    scrollVimTargetIntoView(target);
+
+    expect(viewportEl.scrollTop).toBe(130);
+    document.body.replaceChildren();
+  });
+
+  test('leaves scrollTop untouched when the target is already safe', () => {
+    const { viewport: viewportEl, target } = buildViewport(1000);
+    stubRect(viewportEl, { top: 0, height: 1000 });
+    stubRect(target, { top: 480, bottom: 500, height: 20, width: 100 });
+
+    scrollVimTargetIntoView(target, viewportEl);
+
+    expect(viewportEl.scrollTop).toBe(0);
+    document.body.replaceChildren();
+  });
+
+  test('widens the top band to clear a sticky action bar', () => {
+    const { viewport: viewportEl, target } = buildViewport(1000);
+    stubRect(viewportEl, { top: 0, height: 1000 });
+    const sticky = document.createElement('div');
+    sticky.setAttribute('data-sticky-actions', '');
+    viewportEl.appendChild(sticky);
+    stubRect(sticky, { top: 0, bottom: 300, height: 300 }); // taller than the 160 ratio band
+    // Target at 250..270 sits under the sticky bar (bottom 300 → topMargin 308).
+    stubRect(target, { top: 250, bottom: 270, height: 20, width: 100 });
+
+    scrollVimTargetIntoView(target, viewportEl);
+
+    // 250 - 308 = -58, scrolled up.
+    expect(viewportEl.scrollTop).toBe(-58);
+    document.body.replaceChildren();
+  });
+
+  test('falls back to scrollIntoView when there is no scroll viewport', () => {
+    const orphan = document.createElement('p');
+    document.body.appendChild(orphan);
+    let called = false;
+    orphan.scrollIntoView = () => {
+      called = true;
+    };
+
+    scrollVimTargetIntoView(orphan);
+
+    expect(called).toBe(true);
+    document.body.replaceChildren();
+  });
+
+  test('ignores a zero-size target (unrendered / collapsed)', () => {
+    const { viewport: viewportEl, target } = buildViewport(1000);
+    stubRect(viewportEl, { top: 0, height: 1000 });
+    stubRect(target, { top: 0, bottom: 0, height: 0, width: 0 });
+    viewportEl.scrollTop = 42;
+
+    scrollVimTargetIntoView(target, viewportEl);
+
+    expect(viewportEl.scrollTop).toBe(42);
+    document.body.replaceChildren();
+  });
+});