fix(toc): fix auto scroll on book open with pinned sidebar, closes #3945 (#3975)

apps/readest-app/src/__tests__/app/reader/toc-view-init.test.ts

@@ -1,4 +1,4 @@
-import { describe, it, expect } from 'vitest';
+import { describe, it, expect, vi } from 'vitest';
 import type { TOCItem } from '@/libs/document';
 
 /**
@@ -204,3 +204,293 @@ describe('TOC initial scroll target', () => {
     expect(index).toBe(0);
   });
 });
+
+/**
+ * Regression test for desktop pinned-sidebar auto-scroll failure.
+ *
+ * Setup: with the sidebar pinned, TOCView mounts as soon as bookData is
+ * ready — BEFORE FoliateViewer emits its first relocate event. So at mount
+ * time progress is null and `initialScrollTarget.index` is 0, which means
+ * Virtuoso's `initialTopMostItemIndex` doesn't perform the initial scroll.
+ *
+ * Root cause (confirmed via on-device tracing): when progress finally
+ * arrives, the post-mount progress effect (a) queues `setExpandedItems` to
+ * expand the active section's parents and (b) sets `pendingScrollRef.current
+ * = true`. In the SAME commit the second effect runs against the stale
+ * pre-update `flatItems` (the active section's parent isn't expanded yet),
+ * `findIndex` returns -1, and the old code unconditionally cleared
+ * `pendingScrollRef`. By the time the next render arrives with `flatItems`
+ * containing the active section, the pending flag is gone and the second
+ * effect bails. Toggling the sidebar off-then-on worked around it by
+ * remounting TOCView with progress already set, so Virtuoso's
+ * `initialTopMostItemIndex` handled the first scroll on its own.
+ *
+ * Fix: leave `pendingScrollRef.current = true` when `idx === -1` so the
+ * second effect retries on the next render once `flatItems` reflects the
+ * expanded parents. Defensively also gate the userScrolled bail in the
+ * progress effect on a `initialAutoScrollProcessedRef`, and clear pending
+ * on a real user-driven scroll so a stale pending can't yank the user
+ * later.
+ */
+describe('TOC pinned-sidebar initial auto-scroll', () => {
+  type Refs = {
+    userScrolled: boolean;
+    pendingScroll: boolean;
+    initialScrollHandled: boolean;
+    initialAutoScrollProcessed: boolean;
+  };
+
+  type EffectInput = {
+    isSideBarVisible: boolean;
+    sideBarBookKey: string | null;
+    bookKey: string;
+    sectionHref: string | undefined;
+  };
+
+  // Mirrors the post-mount effect in TOCView.tsx BEFORE the fix.
+  // userScrolled alone gates the early return.
+  const runEffectOld = (refs: Refs, input: EffectInput): void => {
+    if (!input.isSideBarVisible || input.sideBarBookKey !== input.bookKey) {
+      refs.userScrolled = false;
+      refs.pendingScroll = false;
+      return;
+    }
+    if (refs.userScrolled) return;
+    if (input.sectionHref) {
+      if (refs.initialScrollHandled) {
+        refs.initialScrollHandled = false;
+      } else {
+        refs.pendingScroll = true;
+      }
+    }
+  };
+
+  // Mirrors the post-mount effect in TOCView.tsx AFTER the fix.
+  // The userScrolled gate is qualified by initialAutoScrollProcessed
+  // so spurious OS-init scrolls before the first progress arrives can't
+  // suppress the initial auto-scroll.
+  const runEffectNew = (refs: Refs, input: EffectInput): void => {
+    if (!input.isSideBarVisible || input.sideBarBookKey !== input.bookKey) {
+      refs.userScrolled = false;
+      refs.pendingScroll = false;
+      refs.initialAutoScrollProcessed = false;
+      return;
+    }
+    if (refs.userScrolled && refs.initialAutoScrollProcessed) return;
+    if (input.sectionHref) {
+      if (refs.initialScrollHandled) {
+        refs.initialScrollHandled = false;
+      } else {
+        refs.pendingScroll = true;
+      }
+      refs.initialAutoScrollProcessed = true;
+    }
+  };
+
+  describe('before fix (demonstrates the bug)', () => {
+    it('pinned-sidebar mount + spurious OS-init scroll event suppresses the initial auto-scroll', () => {
+      // Sidebar pinned: mounts before relocate. Progress is null at mount.
+      const refs: Refs = {
+        userScrolled: false,
+        pendingScroll: false,
+        initialScrollHandled: false, // index === 0 at mount, no Virtuoso initial scroll
+        initialAutoScrollProcessed: false,
+      };
+
+      // First effect fire: no progress yet.
+      runEffectOld(refs, {
+        isSideBarVisible: true,
+        sideBarBookKey: 'book1',
+        bookKey: 'book1',
+        sectionHref: undefined,
+      });
+
+      // OverlayScrollbars wraps the viewport, scrollTop resets to 0, and
+      // Virtuoso's onScroll handler flips the ref.
+      refs.userScrolled = true;
+
+      // FoliateViewer's first relocate finally fires.
+      runEffectOld(refs, {
+        isSideBarVisible: true,
+        sideBarBookKey: 'book1',
+        bookKey: 'book1',
+        sectionHref: 'ch3.html',
+      });
+
+      // BUG: pending scroll is never set, so the TOC stays at the top.
+      expect(refs.pendingScroll).toBe(false);
+    });
+  });
+
+  describe('after fix', () => {
+    it('schedules pending scroll when progress arrives after mount even if a spurious scroll event was logged', () => {
+      const refs: Refs = {
+        userScrolled: false,
+        pendingScroll: false,
+        initialScrollHandled: false,
+        initialAutoScrollProcessed: false,
+      };
+
+      runEffectNew(refs, {
+        isSideBarVisible: true,
+        sideBarBookKey: 'book1',
+        bookKey: 'book1',
+        sectionHref: undefined,
+      });
+      expect(refs.pendingScroll).toBe(false);
+      expect(refs.initialAutoScrollProcessed).toBe(false);
+
+      refs.userScrolled = true;
+
+      runEffectNew(refs, {
+        isSideBarVisible: true,
+        sideBarBookKey: 'book1',
+        bookKey: 'book1',
+        sectionHref: 'ch3.html',
+      });
+
+      expect(refs.pendingScroll).toBe(true);
+      expect(refs.initialAutoScrollProcessed).toBe(true);
+    });
+
+    it('still suppresses auto-scroll once the initial progress has been processed and the user scrolled', () => {
+      const refs: Refs = {
+        userScrolled: false,
+        pendingScroll: false,
+        initialScrollHandled: true, // mobile case: mounted with valid progress
+        initialAutoScrollProcessed: false,
+      };
+
+      runEffectNew(refs, {
+        isSideBarVisible: true,
+        sideBarBookKey: 'book1',
+        bookKey: 'book1',
+        sectionHref: 'ch3.html',
+      });
+      expect(refs.initialScrollHandled).toBe(false);
+      expect(refs.pendingScroll).toBe(false);
+      expect(refs.initialAutoScrollProcessed).toBe(true);
+
+      refs.userScrolled = true;
+
+      runEffectNew(refs, {
+        isSideBarVisible: true,
+        sideBarBookKey: 'book1',
+        bookKey: 'book1',
+        sectionHref: 'ch5.html',
+      });
+      expect(refs.pendingScroll).toBe(false);
+    });
+
+    it('hide-then-show resets the processed flag so re-showing re-runs auto-scroll', () => {
+      const refs: Refs = {
+        userScrolled: true,
+        pendingScroll: false,
+        initialScrollHandled: false,
+        initialAutoScrollProcessed: true,
+      };
+
+      runEffectNew(refs, {
+        isSideBarVisible: false,
+        sideBarBookKey: 'book1',
+        bookKey: 'book1',
+        sectionHref: 'ch3.html',
+      });
+      expect(refs.userScrolled).toBe(false);
+      expect(refs.initialAutoScrollProcessed).toBe(false);
+
+      runEffectNew(refs, {
+        isSideBarVisible: true,
+        sideBarBookKey: 'book1',
+        bookKey: 'book1',
+        sectionHref: 'ch3.html',
+      });
+      expect(refs.pendingScroll).toBe(true);
+    });
+  });
+});
+
+describe('TOC scroll effect retries when flatItems is stale', () => {
+  type Refs = { pendingScroll: boolean };
+
+  // Mirrors the scroll effect in TOCView.tsx BEFORE the fix: clears
+  // pendingScrollRef unconditionally even when the active section is not
+  // yet in flatItems.
+  const runScrollOld = (
+    refs: Refs,
+    activeHref: string | null,
+    flatItems: { href: string }[],
+    onScroll: (idx: number) => void,
+  ): void => {
+    if (!refs.pendingScroll || !activeHref) return;
+    const idx = flatItems.findIndex((f) => f.href === activeHref);
+    if (idx !== -1) onScroll(idx);
+    refs.pendingScroll = false; // bug: cleared even on idx === -1
+  };
+
+  // Mirrors the scroll effect AFTER the fix: leaves pendingScrollRef set
+  // when idx === -1 so the next render with refreshed flatItems retries.
+  const runScrollNew = (
+    refs: Refs,
+    activeHref: string | null,
+    flatItems: { href: string }[],
+    onScroll: (idx: number) => void,
+  ): void => {
+    if (!refs.pendingScroll || !activeHref) return;
+    const idx = flatItems.findIndex((f) => f.href === activeHref);
+    if (idx === -1) return; // wait for flatItems to include the section
+    onScroll(idx);
+    refs.pendingScroll = false;
+  };
+
+  describe('before fix (demonstrates the bug)', () => {
+    it('clears pendingScroll on the stale flatItems pass and never recovers', () => {
+      const refs: Refs = { pendingScroll: true };
+      const scroll = vi.fn();
+
+      // Render N: setExpandedItems was just queued by the progress
+      // effect — flatItems still reflects the pre-update state and does
+      // not include the deeply-nested active section.
+      const staleFlat = [{ href: 'parent.html' }];
+      runScrollOld(refs, 'child.html', staleFlat, scroll);
+      expect(scroll).not.toHaveBeenCalled();
+      expect(refs.pendingScroll).toBe(false); // BUG: cleared
+
+      // Render N+1: flatItems now includes the active section, but
+      // pendingScroll has already been cleared.
+      const freshFlat = [{ href: 'parent.html' }, { href: 'child.html' }];
+      runScrollNew(refs, 'child.html', freshFlat, scroll);
+      expect(scroll).not.toHaveBeenCalled();
+    });
+  });
+
+  describe('after fix', () => {
+    it('preserves pendingScroll on stale flatItems and scrolls on the next render', () => {
+      const refs: Refs = { pendingScroll: true };
+      const scroll = vi.fn();
+
+      const staleFlat = [{ href: 'parent.html' }];
+      runScrollNew(refs, 'child.html', staleFlat, scroll);
+      expect(scroll).not.toHaveBeenCalled();
+      expect(refs.pendingScroll).toBe(true); // preserved
+
+      const freshFlat = [{ href: 'parent.html' }, { href: 'child.html' }];
+      runScrollNew(refs, 'child.html', freshFlat, scroll);
+      expect(scroll).toHaveBeenCalledWith(1);
+      expect(refs.pendingScroll).toBe(false);
+    });
+
+    it('clears pendingScroll once the scroll fires so it does not re-trigger', () => {
+      const refs: Refs = { pendingScroll: true };
+      const scroll = vi.fn();
+      const flat = [{ href: 'a.html' }, { href: 'b.html' }];
+
+      runScrollNew(refs, 'b.html', flat, scroll);
+      expect(scroll).toHaveBeenCalledTimes(1);
+
+      // A subsequent flatItems change without a new pending must not scroll again.
+      runScrollNew(refs, 'b.html', flat, scroll);
+      expect(scroll).toHaveBeenCalledTimes(1);
+    });
+  });
+});

apps/readest-app/src/app/reader/components/sidebar/TOCView.tsx

@@ -71,6 +71,12 @@ const TOCView: React.FC<{
   const pendingScrollRef = useRef(false);
   const visibleCenterRef = useRef(0);
   const initialScrollHandledRef = useRef(initialScrollTarget.index > 0);
+  // Don't honor userScrolledRef before the first post-mount progress arrives.
+  // With a pinned sidebar, TOCView mounts before FoliateViewer emits its
+  // first relocate; if any programmatic scroll (e.g. OverlayScrollbars'
+  // viewport-wrap scrollTop reset) flips userScrolledRef in that window it
+  // would otherwise suppress the auto-scroll once progress finally arrives.
+  const initialAutoScrollProcessedRef = useRef(false);
 
   // OverlayScrollbars + Virtuoso integration (same pattern as Bookshelf)
   const osRootRef = useRef<HTMLDivElement>(null);
@@ -169,9 +175,10 @@ const TOCView: React.FC<{
     if (!isSideBarVisible || sideBarBookKey !== bookKey) {
       userScrolledRef.current = false;
       pendingScrollRef.current = false;
+      initialAutoScrollProcessedRef.current = false;
       return;
     }
-    if (userScrolledRef.current) return;
+    if (userScrolledRef.current && initialAutoScrollProcessedRef.current) return;
     setExpandedItems((prev) => {
       const next = computeExpandedSet(toc, progress?.sectionHref);
       return setsHaveSameContents(prev, next) ? prev : next;
@@ -182,21 +189,28 @@ const TOCView: React.FC<{
       } else {
         pendingScrollRef.current = true;
       }
+      initialAutoScrollProcessedRef.current = true;
     }
   }, [isSideBarVisible, sideBarBookKey, bookKey, toc, progress]);
 
   useEffect(() => {
     if (!pendingScrollRef.current || !activeHref || !isSideBarVisible) return;
     const idx = flatItems.findIndex((f) => f.item.href === activeHref);
-    if (idx !== -1) {
-      // Eink displays ghost previous frames during smooth JS scroll
-      // animations; force an instant jump to avoid the artifact. A CSS-only
-      // fix is impossible because scrollTo({ behavior: 'smooth' }) overrides
-      // CSS scroll-behavior and is not a CSS transition.
-      const distance = Math.abs(idx - visibleCenterRef.current);
-      const behavior = isEink || distance > 16 ? 'auto' : 'smooth';
-      virtuosoRef.current?.scrollToIndex({ index: idx, align: 'center', behavior });
+    if (idx === -1) {
+      // The active section's parents were just queued to expand by the
+      // post-mount progress effect above — flatItems still reflects the
+      // pre-update expandedItems. Leave pendingScrollRef set so this
+      // effect retries on the next render once flatItems contains the
+      // active section. Clearing it here would strand the scroll.
+      return;
     }
+    // Eink displays ghost previous frames during smooth JS scroll
+    // animations; force an instant jump to avoid the artifact. A CSS-only
+    // fix is impossible because scrollTo({ behavior: 'smooth' }) overrides
+    // CSS scroll-behavior and is not a CSS transition.
+    const distance = Math.abs(idx - visibleCenterRef.current);
+    const behavior = isEink || distance > 16 ? 'auto' : 'smooth';
+    virtuosoRef.current?.scrollToIndex({ index: idx, align: 'center', behavior });
     pendingScrollRef.current = false;
   }, [flatItems, activeHref, isSideBarVisible, isEink]);
 
@@ -215,6 +229,11 @@ const TOCView: React.FC<{
             visibleCenterRef.current = Math.floor((startIndex + endIndex) / 2);
           }}
           onScroll={() => {
+            // A scroll arriving while a pending auto-scroll is still
+            // queued (idx === -1, waiting on flatItems to expand) means
+            // the user is now driving — drop the queued auto-scroll so
+            // the next render doesn't yank them away.
+            pendingScrollRef.current = false;
             userScrolledRef.current = true;
             if (scrollCooldownRef.current) clearTimeout(scrollCooldownRef.current);
             scrollCooldownRef.current = setTimeout(() => {