From e1dad98e56059eb1f792c0572dd4de29308e249a Mon Sep 17 00:00:00 2001 From: Huang Xin Date: Mon, 20 Apr 2026 20:07:49 +0800 Subject: [PATCH] fix(toc): prevent auto-scroll snap-back on sidebar open (#3900) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The TOC occasionally flashed a scroll to the current item and then snapped back to the top, and on slow mobile first-opens sometimes stayed at the top entirely. Root cause: `useOverlayScrollbars({ defer: true })` schedules OS construction via `requestIdleCallback` with a ~2233 ms timeout. On a busy first open the timeout fires before the browser goes idle, so OS wraps the viewport late — and the wrap step resets the scroller's `scrollTop` synchronously, undoing Virtuoso's earlier scroll to the current item. Virtuoso's `rangeChanged` / `onScroll` don't propagate the reset for another frame, so any guard based on tracked scroll state reads stale. --- .../app/reader/toc-view-init.test.ts | 92 +++++++++++++++++++ .../reader/components/notebook/Notebook.tsx | 5 +- .../app/reader/components/sidebar/SideBar.tsx | 5 +- .../app/reader/components/sidebar/TOCView.tsx | 76 +++++++++++---- 4 files changed, 157 insertions(+), 21 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 8936aed3..cc15ea02 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 @@ -41,6 +41,39 @@ const flattenTOC = (items: TOCItem[], expandedItems: Set, depth = 0): TO return result; }; +// Helpers mirrored from TOCView.tsx for the initial-scroll-target logic. +// These drive the Virtuoso `initialTopMostItemIndex` prop, which avoids the +// race where a setTimeout-based scrollToIndex fires before Virtuoso has +// finished its first layout pass. +const findParentPath = (items: TOCItem[], href: string, path: TOCItem[] = []): TOCItem[] => { + for (const item of items) { + const newPath = [...path, item]; + if (item.href === href) return path; + if (item.subitems) { + const found = findParentPath(item.subitems, href, newPath); + if (found.length > 0) return found; + } + } + return []; +}; + +const computeExpandedSet = (toc: TOCItem[], href: string | undefined): Set => { + const topLevel = toc.filter((item) => item.subitems?.length).map(getItemIdentifier); + const parents = href ? findParentPath(toc, href).map(getItemIdentifier).filter(Boolean) : []; + return new Set([...topLevel, ...parents]); +}; + +const getInitialScrollTarget = ( + toc: TOCItem[], + href: string | undefined, +): { index: number; expanded: Set } => { + const expanded = computeExpandedSet(toc, href); + if (!href) return { index: 0, expanded }; + const flat = flattenTOC(toc, expanded); + const idx = flat.findIndex((item) => item.href === href); + return { index: idx > 0 ? idx : 0, expanded }; +}; + describe('TOC sidebar initialization', () => { const nestedTOC: TOCItem[] = [ { @@ -112,3 +145,62 @@ describe('TOC sidebar initialization', () => { }); }); }); + +/** + * Regression test for TOC auto-scroll race condition. + * + * When the TOC opens with an existing book progress, the view must scroll to + * the current item. The previous implementation used a 300 ms setTimeout to + * trigger `scrollToIndex` after mount. That races with Virtuoso's internal + * layout stabilization: under load the timer occasionally fires first, the + * TOC scrolls to the target, and then Virtuoso's late layout pass snaps the + * list back to the top. + * + * Fix: compute the initial scroll target synchronously during mount so it + * can be fed to Virtuoso's `initialTopMostItemIndex` prop, which Virtuoso + * uses to perform the first scroll itself — no setTimeout race. + */ +describe('TOC initial scroll target', () => { + const nestedTOC: TOCItem[] = [ + { + id: 0, + label: 'Book', + href: undefined, + subitems: [ + { id: 1, label: 'Chapter 1', href: 'ch1.html' }, + { id: 2, label: 'Chapter 2', href: 'ch2.html' }, + { id: 3, label: 'Chapter 3', href: 'ch3.html' }, + ], + } as unknown as TOCItem, + ]; + + const flatTOC: TOCItem[] = [ + { id: 1, label: 'Chapter 1', href: 'ch1.html' } as unknown as TOCItem, + { id: 2, label: 'Chapter 2', href: 'ch2.html' } as unknown as TOCItem, + { id: 3, label: 'Chapter 3', href: 'ch3.html' } as unknown as TOCItem, + ]; + + it('returns index 0 when no current href is provided', () => { + const { index, expanded } = getInitialScrollTarget(nestedTOC, undefined); + expect(index).toBe(0); + // Top-level container is still expanded so the list renders its chapters. + expect(expanded.size).toBe(1); + }); + + it('resolves the current chapter inside a nested TOC with parents expanded', () => { + const { index, expanded } = getInitialScrollTarget(nestedTOC, 'ch3.html'); + // flat order is Book, Ch1, Ch2, Ch3 → current chapter sits at index 3. + expect(index).toBe(3); + expect(expanded.has(getItemIdentifier(nestedTOC[0]!))).toBe(true); + }); + + it('resolves the current chapter inside a flat TOC', () => { + const { index } = getInitialScrollTarget(flatTOC, 'ch2.html'); + expect(index).toBe(1); + }); + + it('falls back to index 0 when the href cannot be found', () => { + const { index } = getInitialScrollTarget(nestedTOC, 'missing.html'); + expect(index).toBe(0); + }); +}); diff --git a/apps/readest-app/src/app/reader/components/notebook/Notebook.tsx b/apps/readest-app/src/app/reader/components/notebook/Notebook.tsx index 9168058f..8b583549 100644 --- a/apps/readest-app/src/app/reader/components/notebook/Notebook.tsx +++ b/apps/readest-app/src/app/reader/components/notebook/Notebook.tsx @@ -60,7 +60,10 @@ const Notebook: React.FC = ({}) => { panelHeight: notebookHeight, handleVerticalDragStart, } = useSwipeToDismiss( - () => setNotebookVisible(false), + () => { + setNotebookVisible(false); + setIsFullHeightInMobile(isMobile); + }, (data) => setIsFullHeightInMobile(data.clientY < 44), ); diff --git a/apps/readest-app/src/app/reader/components/sidebar/SideBar.tsx b/apps/readest-app/src/app/reader/components/sidebar/SideBar.tsx index 8b3e312f..041fc00a 100644 --- a/apps/readest-app/src/app/reader/components/sidebar/SideBar.tsx +++ b/apps/readest-app/src/app/reader/components/sidebar/SideBar.tsx @@ -76,7 +76,10 @@ const SideBar = ({}) => { panelHeight: sidebarHeight, handleVerticalDragStart, } = useSwipeToDismiss( - () => setSideBarVisible(false), + () => { + setSideBarVisible(false); + setIsFullHeightInMobile(isMobile); + }, (data) => setIsFullHeightInMobile(data.clientY < 44), ); 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 99f3833d..713a4482 100644 --- a/apps/readest-app/src/app/reader/components/sidebar/TOCView.tsx +++ b/apps/readest-app/src/app/reader/components/sidebar/TOCView.tsx @@ -34,6 +34,23 @@ const computeExpandedSet = (toc: TOCItem[], href: string | undefined): Set, b: Set): boolean => { + if (a.size !== b.size) return false; + for (const item of a) if (!b.has(item)) return false; + return true; +}; + +const getInitialScrollTarget = ( + toc: TOCItem[], + href: string | undefined, +): { index: number; expanded: Set } => { + const expanded = computeExpandedSet(toc, href); + if (!href) return { index: 0, expanded }; + const flat = flattenTOC(toc, expanded); + const idx = flat.findIndex((f) => f.item.href === href); + return { index: idx > 0 ? idx : 0, expanded }; +}; + const TOCView: React.FC<{ bookKey: string; toc: TOCItem[]; @@ -43,9 +60,8 @@ const TOCView: React.FC<{ const progress = getProgress(bookKey); const isEink = !!getViewSettings(bookKey)?.isEink; - const [expandedItems, setExpandedItems] = useState>(() => - computeExpandedSet(toc, progress?.sectionHref), - ); + const [initialScrollTarget] = useState(() => getInitialScrollTarget(toc, progress?.sectionHref)); + const [expandedItems, setExpandedItems] = useState>(initialScrollTarget.expanded); const [containerHeight, setContainerHeight] = useState(400); const containerRef = useRef(null); @@ -54,6 +70,7 @@ const TOCView: React.FC<{ const scrollCooldownRef = useRef | null>(null); const pendingScrollRef = useRef(false); const visibleCenterRef = useRef(0); + const initialScrollHandledRef = useRef(initialScrollTarget.index > 0); // OverlayScrollbars + Virtuoso integration (same pattern as Bookshelf) const osRootRef = useRef(null); @@ -66,6 +83,16 @@ const TOCView: React.FC<{ const { viewport } = instance.elements(); viewport.style.overflowX = 'var(--os-viewport-overflow-x)'; viewport.style.overflowY = 'var(--os-viewport-overflow-y)'; + const target = initialScrollTarget.index; + if (target > 0) { + requestAnimationFrame(() => { + virtuosoRef.current?.scrollToIndex({ + index: target, + align: 'center', + behavior: 'auto', + }); + }); + } }, }, }); @@ -145,26 +172,32 @@ const TOCView: React.FC<{ return; } if (userScrolledRef.current) return; - setExpandedItems(computeExpandedSet(toc, progress?.sectionHref)); - if (progress?.sectionHref) pendingScrollRef.current = true; + setExpandedItems((prev) => { + const next = computeExpandedSet(toc, progress?.sectionHref); + return setsHaveSameContents(prev, next) ? prev : next; + }); + if (progress?.sectionHref) { + if (initialScrollHandledRef.current) { + initialScrollHandledRef.current = false; + } else { + pendingScrollRef.current = true; + } + } }, [isSideBarVisible, sideBarBookKey, bookKey, toc, progress]); useEffect(() => { if (!pendingScrollRef.current || !activeHref || !isSideBarVisible) return; - const timer = setTimeout(() => { - 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 }); - } - pendingScrollRef.current = false; - }, 200); - return () => clearTimeout(timer); + 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 }); + } + pendingScrollRef.current = false; }, [flatItems, activeHref, isSideBarVisible, isEink]); return ( @@ -173,6 +206,11 @@ const TOCView: React.FC<{ 0 + ? { index: initialScrollTarget.index, align: 'center' } + : 0 + } rangeChanged={({ startIndex, endIndex }) => { visibleCenterRef.current = Math.floor((startIndex + endIndex) / 2); }}