From 17f2a17adcdbe7d764ce556fdad8156aaa1a45f0 Mon Sep 17 00:00:00 2001 From: Huang Xin Date: Mon, 27 Apr 2026 21:35:19 +0800 Subject: [PATCH] fix(toc): fix auto scroll on book open with pinned sidebar, closes #3945 (#3975) --- .../app/reader/toc-view-init.test.ts | 292 +++++++++++++++++- .../app/reader/components/sidebar/TOCView.tsx | 37 ++- 2 files changed, 319 insertions(+), 10 deletions(-) diff --git a/apps/readest-app/src/__tests__/app/reader/toc-view-init.test.ts b/apps/readest-app/src/__tests__/app/reader/toc-view-init.test.ts index cc15ea02..a1696206 100644 --- a/apps/readest-app/src/__tests__/app/reader/toc-view-init.test.ts +++ b/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); + }); + }); +}); diff --git a/apps/readest-app/src/app/reader/components/sidebar/TOCView.tsx b/apps/readest-app/src/app/reader/components/sidebar/TOCView.tsx index 713a4482..aa0294bf 100644 --- a/apps/readest-app/src/app/reader/components/sidebar/TOCView.tsx +++ b/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(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(() => {