fix(reader): restore annotation list auto-scroll to the nearest item (#4428)

Virtualizing BooknoteView (#4352) dropped the auto-scroll that centers the note nearest the current reading position, leaving the list stranded at the top. The single scrollToIndex that replaced the per-item useScrollToItem was missing the machinery TOCView already uses for virtualized auto-scroll: - Re-apply the scroll inside the OverlayScrollbars `initialized` callback (read via a ref): its deferred init resets the viewport scrollTop to 0, and the lastScrolledCfiRef guard otherwise blocked any retry (reload case). - Mount Virtuoso natively centered via initialTopMostItemIndex with a skip-gate, so opening the panel while reading doesn't fire a scrollToIndex that races and wedges the freshly mounted, unmeasured list (tab-switch case). - Jump instantly (behavior 'auto') for far moves and on eink, animating 'smooth' only for short in-session updates — mirroring TOCView. Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
2026-06-03 00:39:45 +08:00
parent 9d8062ae27
commit 578b7ba14f
2 changed files with 304 additions and 10 deletions
@@ -0,0 +1,223 @@
+import { render, act, cleanup } from '@testing-library/react';
+import { afterEach, beforeEach, describe, expect, it, vi, type Mock } from 'vitest';
+
+import type { BookNote } from '@/types/book';
+
+// ---------- Shared mutable test state (captured by the mock factories) ----------
+let scrollToIndexSpy: Mock<(arg: unknown) => void>;
+// Only the FIRST (mount-time) `initialized` callback is captured, mirroring how
+// OverlayScrollbars binds its event handlers when it initializes the viewport.
+// A fix that relied on a fresher render closure would pass against the latest
+// callback but still break in the real app — so the test forces a ref-based fix.
+let capturedInitialized:
+  | ((instance: { elements: () => { viewport: HTMLElement } }) => void)
+  | undefined;
+// Latest props Virtuoso was rendered with, so tests can assert the mount-time
+// position (initialTopMostItemIndex) the panel hands it.
+let capturedVirtuosoProps: Record<string, unknown> | undefined;
+let mockProgress: { location: string } | null;
+let mockBooknotes: BookNote[];
+
+// ---------- Mocks ----------
+vi.mock('@/store/bookDataStore', () => ({
+  useBookDataStore: () => ({ getConfig: () => ({ booknotes: mockBooknotes }) }),
+}));
+
+vi.mock('@/store/readerStore', () => ({
+  useReaderStore: () => ({ getProgress: () => mockProgress }),
+}));
+
+vi.mock('@/store/sidebarStore', () => ({
+  useSidebarStore: () => ({
+    setActiveBooknoteType: vi.fn(),
+    setBooknoteResults: vi.fn(),
+  }),
+}));
+
+// Derive a per-chapter TOC group from the spine step of each note's CFI so the
+// flattened list is [header, note, header, note, ...] sorted by chapter.
+vi.mock('@/services/nav', () => ({
+  findTocItemBS: (_toc: unknown, cfi: string) => {
+    const match = cfi.match(/\/6\/(\d+)!/);
+    const n = match ? Number(match[1]) : 0;
+    return { id: n, href: `ch${n}.html`, label: `Chapter ${n}`, index: n };
+  },
+}));
+
+vi.mock('@/hooks/useTranslation', () => ({
+  useTranslation: () => (s: string) => s,
+}));
+
+vi.mock('@/utils/event', () => ({
+  eventDispatcher: { dispatch: vi.fn(), on: vi.fn(), off: vi.fn() },
+}));
+
+vi.mock('@/app/reader/components/sidebar/BooknoteItem', () => ({
+  default: () => null,
+}));
+
+vi.mock('@/app/reader/components/EmptyState', () => ({
+  default: () => null,
+}));
+
+// Virtuoso is replaced with a stub that exposes a spy-able `scrollToIndex`
+// through the imperative handle and hands BooknoteView a scroller element.
+vi.mock('react-virtuoso', async () => {
+  const ReactMod = await import('react');
+  return {
+    Virtuoso: ReactMod.forwardRef(
+      (
+        props: { scrollerRef?: (el: HTMLElement | Window | null) => void },
+        ref: React.Ref<unknown>,
+      ) => {
+        capturedVirtuosoProps = props as Record<string, unknown>;
+        ReactMod.useImperativeHandle(ref, () => ({
+          scrollToIndex: (arg: unknown) => scrollToIndexSpy(arg),
+        }));
+        ReactMod.useEffect(() => {
+          props.scrollerRef?.(document.createElement('div'));
+        }, []);
+        return null;
+      },
+    ),
+  };
+});
+
+// Capture the OverlayScrollbars `initialized` callback so the test can fire it
+// on demand (the real hook fires it after a deferred, timing-dependent init).
+vi.mock('overlayscrollbars-react', () => ({
+  useOverlayScrollbars: (opts: {
+    events?: { initialized?: (i: { elements: () => { viewport: HTMLElement } }) => void };
+  }) => {
+    if (!capturedInitialized) capturedInitialized = opts.events?.initialized;
+    return [vi.fn(), () => undefined];
+  },
+}));
+
+// eslint-disable-next-line import/first
+import BooknoteView from '@/app/reader/components/sidebar/BooknoteView';
+
+const makeNote = (cfi: string): BookNote =>
+  ({
+    id: cfi,
+    type: 'annotation',
+    cfi,
+    text: cfi,
+    note: '',
+    createdAt: 0,
+    updatedAt: 0,
+  }) as BookNote;
+
+const fireOverlayScrollbarsInitialized = () => {
+  act(() => {
+    capturedInitialized?.({ elements: () => ({ viewport: document.createElement('div') }) });
+  });
+};
+
+beforeEach(() => {
+  scrollToIndexSpy = vi.fn<(arg: unknown) => void>();
+  capturedInitialized = undefined;
+  capturedVirtuosoProps = undefined;
+  mockProgress = null;
+  mockBooknotes = [
+    makeNote('epubcfi(/6/4!/4/2:0)'),
+    makeNote('epubcfi(/6/6!/4/4:0)'),
+    makeNote('epubcfi(/6/8!/4/2:0)'),
+    makeNote('epubcfi(/6/10!/4/6:0)'),
+    makeNote('epubcfi(/6/26!/4/2:0)'),
+  ];
+  // Run rAF synchronously so the callback's scroll happens inside act().
+  vi.stubGlobal('requestAnimationFrame', (cb: FrameRequestCallback) => {
+    cb(0);
+    return 0;
+  });
+});
+
+afterEach(() => {
+  cleanup();
+  vi.unstubAllGlobals();
+});
+
+describe('BooknoteView — OverlayScrollbars init does not rewind the list to the top', () => {
+  it('re-applies the auto-scroll to the nearest note when OverlayScrollbars initializes after the reading position arrives', () => {
+    // Fresh open: BooknoteView mounts before the first relocate, so `progress`
+    // (and thus the nearest cfi) has no reading position yet.
+    mockProgress = null;
+    const { rerender } = render(<BooknoteView type='annotation' bookKey='book1' toc={[]} />);
+
+    // The relocate arrives → the normal auto-scroll effect centers the nearest
+    // note. The list is [h4, n4, h6, n6, h8, n8, h10, n10, h26, n26]; the
+    // reading position is in the last chapter, so the nearest note is index 9.
+    mockProgress = { location: 'epubcfi(/6/26!/4/10:0)' };
+    act(() => {
+      rerender(<BooknoteView type='annotation' bookKey='book1' toc={[]} />);
+    });
+
+    // Ignore that first scroll; we only care whether the OverlayScrollbars init
+    // (which clobbers scrollTop) re-applies it instead of stranding the top.
+    scrollToIndexSpy.mockClear();
+
+    fireOverlayScrollbarsInitialized();
+
+    expect(scrollToIndexSpy).toHaveBeenCalledWith(expect.objectContaining({ index: 9 }));
+  });
+
+  it('does not force a scroll on OverlayScrollbars init when there is no reading position', () => {
+    mockProgress = null;
+    render(<BooknoteView type='annotation' bookKey='book1' toc={[]} />);
+
+    scrollToIndexSpy.mockClear();
+    fireOverlayScrollbarsInitialized();
+
+    expect(scrollToIndexSpy).not.toHaveBeenCalled();
+  });
+
+  it('positions Virtuoso natively on mount (without a racing scrollToIndex) when the reading position is already known', () => {
+    // Switching to the panel while reading: the reading position is available at
+    // mount. A scrollToIndex against the freshly mounted, unmeasured list no-ops
+    // or wedges it into rendering nothing — so the panel must mount Virtuoso
+    // already centered via initialTopMostItemIndex and the scroll effect must
+    // skip its first jump.
+    mockProgress = { location: 'epubcfi(/6/26!/4/10:0)' };
+    act(() => {
+      render(<BooknoteView type='annotation' bookKey='book1' toc={[]} />);
+    });
+
+    expect(capturedVirtuosoProps?.['initialTopMostItemIndex']).toEqual({
+      index: 9,
+      align: 'center',
+    });
+    expect(scrollToIndexSpy).not.toHaveBeenCalled();
+
+    // The deferred OverlayScrollbars init resets scrollTop; the re-apply then
+    // restores the centered position (now that the rows have been measured).
+    fireOverlayScrollbarsInitialized();
+    expect(scrollToIndexSpy).toHaveBeenCalledWith(expect.objectContaining({ index: 9 }));
+  });
+
+  it('jumps instantly (behavior auto) for a far scroll instead of animating it, like TOCView', () => {
+    // 12 notes in distinct chapters → flat list [h, n, h, n, ...] of 24 rows;
+    // the nearest note (last chapter) sits at index 23, far from the top.
+    mockBooknotes = Array.from({ length: 12 }, (_, i) =>
+      makeNote(`epubcfi(/6/${4 + i * 2}!/4/2:0)`),
+    );
+
+    // Reload-style: no reading position at mount, so initialTopMostItemIndex
+    // does not handle it and the scroll effect performs the jump.
+    mockProgress = null;
+    const { rerender } = render(<BooknoteView type='annotation' bookKey='book1' toc={[]} />);
+
+    mockProgress = { location: 'epubcfi(/6/26!/4/10:0)' };
+    act(() => {
+      rerender(<BooknoteView type='annotation' bookKey='book1' toc={[]} />);
+    });
+
+    // distance (23 - 0) > 16 → instant jump, not a smooth animation.
+    expect(scrollToIndexSpy).toHaveBeenCalledWith(
+      expect.objectContaining({ index: 23, behavior: 'auto' }),
+    );
+    expect(scrollToIndexSpy).not.toHaveBeenCalledWith(
+      expect.objectContaining({ behavior: 'smooth' }),
+    );
+  });
+});
@@ -96,6 +96,17 @@ const BooknoteView: React.FC<{
    return findNearestCfi(allSorted, progress?.location);
  }, [progress?.location, sortedGroups]);

+  // Index of the nearest note in the flattened list (-1 when none). Memoized so
+  // the scroll effect and the OverlayScrollbars `initialized` callback share a
+  // single source of truth.
+  const nearestIndex = useMemo(
+    () =>
+      nearestCfi
+        ? flatItems.findIndex((row) => row.kind === 'note' && row.item.cfi === nearestCfi)
+        : -1,
+    [nearestCfi, flatItems],
+  );
+
  const handleBrowseBookNotes = useCallback(() => {
    if (filteredNotes.length === 0) return;
    const sorted = [...filteredNotes].sort((a, b) => CFI.compare(a.cfi, b.cfi));
@@ -111,6 +122,27 @@ const BooknoteView: React.FC<{
  const [containerHeight, setContainerHeight] = useState(400);
  const lastScrolledCfiRef = useRef<string | null>(null);

+  // Mirror the nearest index so the OverlayScrollbars `initialized` callback —
+  // created at mount but fired after a deferred, timing-dependent init — reads
+  // the current target instead of its stale mount-time closure.
+  const nearestIndexRef = useRef(nearestIndex);
+  nearestIndexRef.current = nearestIndex;
+
+  // Center index of the currently visible window, kept fresh by Virtuoso's
+  // rangeChanged. Lets the scroll effect jump instantly for far moves and
+  // animate only short ones (mirrors TOCView).
+  const visibleCenterRef = useRef(0);
+
+  // When the reading position is already known at open time (the common case of
+  // switching to the panel while reading), mount Virtuoso *natively* centered on
+  // the nearest note via initialTopMostItemIndex. A scrollToIndex against a
+  // freshly mounted, unmeasured list no-ops (smooth) or wedges it, so the scroll
+  // effect skips that first jump and lets initialTopMostItemIndex handle it; the
+  // OverlayScrollbars `initialized` re-apply restores it after the deferred init
+  // resets scrollTop (mirrors TOCView).
+  const [initialTopIndex] = useState(() => nearestIndex);
+  const initialScrollHandledRef = useRef(initialTopIndex > 0);
+
  const [initialize, osInstance] = useOverlayScrollbars({
    defer: true,
    options: { scrollbars: { autoHide: 'scroll' } },
@@ -119,6 +151,22 @@ const BooknoteView: React.FC<{
        const { viewport } = instance.elements();
        viewport.style.overflowX = 'var(--os-viewport-overflow-x)';
        viewport.style.overflowY = 'var(--os-viewport-overflow-y)';
+        // OverlayScrollbars resets the wrapped viewport's scrollTop to 0 as it
+        // initializes (deferred), clobbering the mount-time auto-scroll and
+        // stranding the list at the top. Re-apply it to the *current* nearest
+        // note — read via ref since this is the mount-time closure. The first
+        // rAF lets the reset settle; the second re-asserts once the freshly
+        // mounted rows are measured (a lone scrollToIndex to a far, unmeasured
+        // row otherwise lands short).
+        const reapply = () => {
+          const index = nearestIndexRef.current;
+          if (index < 0) return;
+          virtuosoRef.current?.scrollToIndex({ index, align: 'center', behavior: 'auto' });
+        };
+        requestAnimationFrame(() => {
+          reapply();
+          requestAnimationFrame(reapply);
+        });
      },
    },
  });
@@ -169,18 +217,35 @@ const BooknoteView: React.FC<{
  // per-item useScrollToItem (which forced 1000 layout reads) with a single
  // virtuosoRef.scrollToIndex call.
  useEffect(() => {
-    if (!nearestCfi || !flatItems.length) return;
+    if (nearestIndex < 0) return;
    if (nearestCfi === lastScrolledCfiRef.current) return;
-    const idx = flatItems.findIndex((row) => row.kind === 'note' && row.item.cfi === nearestCfi);
-    if (idx < 0) return;
-    const isEink = document.documentElement.getAttribute('data-eink') === 'true';
-    virtuosoRef.current?.scrollToIndex({
-      index: idx,
-      align: 'center',
-      behavior: isEink ? 'auto' : 'smooth',
-    });
    lastScrolledCfiRef.current = nearestCfi;
-  }, [nearestCfi, flatItems]);
+    // initialTopMostItemIndex already centered the mount position; a
+    // scrollToIndex that races Virtuoso's first render no-ops or wedges it, so
+    // skip this one and let the `initialized` re-apply restore it if needed.
+    if (initialScrollHandledRef.current) {
+      initialScrollHandledRef.current = false;
+      return;
+    }
+    const isEink = document.documentElement.getAttribute('data-eink') === 'true';
+    // Jump instantly for far moves (and on eink, which ghosts during a smooth
+    // animation) to avoid blanking the virtualized list mid-animation; keep
+    // smooth only for short, in-session progress updates (mirrors TOCView). A
+    // far instant jump can land short until the target rows are measured, so
+    // re-assert once on the next frame.
+    const distance = Math.abs(nearestIndex - visibleCenterRef.current);
+    const behavior = isEink || distance > 16 ? 'auto' : 'smooth';
+    virtuosoRef.current?.scrollToIndex({ index: nearestIndex, align: 'center', behavior });
+    if (behavior === 'auto') {
+      requestAnimationFrame(() => {
+        virtuosoRef.current?.scrollToIndex({
+          index: nearestIndex,
+          align: 'center',
+          behavior: 'auto',
+        });
+      });
+    }
+  }, [nearestCfi, nearestIndex]);

  const renderItem = useCallback(
    (index: number) => {
@@ -250,6 +315,12 @@ const BooknoteView: React.FC<{
          <Virtuoso
            ref={virtuosoRef}
            scrollerRef={handleScrollerRef}
+            initialTopMostItemIndex={
+              initialTopIndex > 0 ? { index: initialTopIndex, align: 'center' } : 0
+            }
+            rangeChanged={({ startIndex, endIndex }) => {
+              visibleCenterRef.current = Math.floor((startIndex + endIndex) / 2);
+            }}
            style={{ height: containerHeight }}
            totalCount={flatItems.length}
            computeItemKey={(index) => flatItems[index]?.key ?? index}