From 6d5e59c79a00bfc70130b451a1494c8eafb7c79e Mon Sep 17 00:00:00 2001 From: Lex Moulton Date: Mon, 27 Apr 2026 21:02:40 -0400 Subject: [PATCH] fix(rsvp): resume at stop word, prevent section replay, restore full context (#3960) --- .../components/rsvp-section-advance.test.tsx | 203 ++++++++++++++++++ .../services/rsvp-persistence.test.ts | 25 +++ .../reader/components/rsvp/RSVPControl.tsx | 69 ++++-- .../reader/components/rsvp/RSVPOverlay.tsx | 68 +----- apps/readest-app/src/services/rsvp/index.ts | 1 + .../src/services/rsvp/persistence.ts | 12 ++ apps/readest-app/src/types/view.ts | 2 +- 7 files changed, 302 insertions(+), 78 deletions(-) create mode 100644 apps/readest-app/src/__tests__/components/rsvp-section-advance.test.tsx create mode 100644 apps/readest-app/src/__tests__/services/rsvp-persistence.test.ts create mode 100644 apps/readest-app/src/services/rsvp/persistence.ts diff --git a/apps/readest-app/src/__tests__/components/rsvp-section-advance.test.tsx b/apps/readest-app/src/__tests__/components/rsvp-section-advance.test.tsx new file mode 100644 index 00000000..67203144 --- /dev/null +++ b/apps/readest-app/src/__tests__/components/rsvp-section-advance.test.tsx @@ -0,0 +1,203 @@ +'use client'; + +import { render, act, cleanup } from '@testing-library/react'; +import { afterEach, beforeEach, describe, expect, test, vi } from 'vitest'; + +import RSVPControl from '@/app/reader/components/rsvp/RSVPControl'; +import { eventDispatcher } from '@/utils/event'; + +// ---------- Shared mutable test state ---------- +// These are captured by closure in the mock factories below. +// Each beforeEach resets them. + +let primaryIndex = 4; +const viewRelocateListeners: EventListener[] = []; +const loadedSections: number[] = []; +const controllerEventListeners = new Map(); +let capturedOnRequestNextPage: (() => Promise) | null = null; + +// ---------- Mocks ---------- + +vi.mock('@/app/reader/components/rsvp/RSVPOverlay', () => ({ + default: vi.fn(({ onRequestNextPage }: { onRequestNextPage: () => Promise }) => { + capturedOnRequestNextPage = onRequestNextPage; + return null; + }), +})); + +vi.mock('@/app/reader/components/rsvp/RSVPStartDialog', () => ({ + default: () => null, +})); + +vi.mock('@/hooks/useTranslation', () => ({ + useTranslation: () => (s: string) => s, +})); + +vi.mock('@/context/EnvContext', () => ({ + useEnv: () => ({ envConfig: {} }), +})); + +vi.mock('@/store/readerStore', () => ({ + useReaderStore: () => ({ + getView: () => mockView, + getProgress: () => ({ + location: 'epubcfi(/6/8!/4/1:0)', + sectionHref: `ch${primaryIndex}.xhtml`, + }), + }), +})); + +vi.mock('@/store/bookDataStore', () => ({ + useBookDataStore: () => ({ + getBookData: () => ({ book: { format: 'EPUB' }, bookDoc: { toc: [] } }), + getConfig: () => null, + setConfig: vi.fn(), + saveConfig: vi.fn(), + }), +})); + +vi.mock('@/store/settingsStore', () => ({ + useSettingsStore: () => ({ settings: {} }), +})); + +vi.mock('@/store/themeStore', () => ({ + useThemeStore: () => ({ themeCode: { primary: '#000' } }), +})); + +// RSVPController mock: fires rsvp-start-choice immediately from requestStart(), +// and tracks loadNextPageContent calls via the shared loadedSections array. +// Must use a regular function (not arrow function) so it can be called with `new`. +vi.mock('@/services/rsvp', () => ({ + // eslint-disable-next-line prefer-arrow-callback + RSVPController: vi.fn(function RSVPControllerMock() { + return { + seedPosition: vi.fn(), + setCurrentCfi: vi.fn(), + requestStart: vi.fn(() => { + const event = new CustomEvent('rsvp-start-choice', { + detail: { hasSavedPosition: false, hasSelection: false }, + }); + (controllerEventListeners.get('rsvp-start-choice') ?? []).forEach((h) => h(event)); + }), + startFromCurrentPosition: vi.fn(), + stop: vi.fn(), + loadNextPageContent: vi.fn(() => { + loadedSections.push(primaryIndex); + }), + getStoredPosition: vi.fn(() => null), + get currentState() { + return { active: true }; + }, + addEventListener: vi.fn((type: string, listener: EventListener) => { + if (!controllerEventListeners.has(type)) controllerEventListeners.set(type, []); + controllerEventListeners.get(type)!.push(listener); + }), + removeEventListener: vi.fn((type: string, listener: EventListener) => { + const arr = controllerEventListeners.get(type) ?? []; + controllerEventListeners.set( + type, + arr.filter((l) => l !== listener), + ); + }), + }; + }), + buildRsvpExitConfigUpdate: vi.fn(() => ({})), +})); + +// ---------- Mock FoliateView ---------- +const mockView = { + renderer: { + get primaryIndex() { + return primaryIndex; + }, + get atEnd() { + return false; + }, + // renderer.goTo is called directly with the exact target section index, + // bypassing the reverted primaryIndex that nextSection() would use. + goTo: vi.fn(async ({ index }: { index: number }) => { + const prevIdx = primaryIndex; + + // #goTo sets #primaryIndex = index before the navigation relocate fires + primaryIndex = index; + + const navEvent = new CustomEvent('relocate', { + detail: { + section: { current: index }, + tocItem: { href: `ch${index}.xhtml` }, + }, + }); + [...viewRelocateListeners].forEach((l) => l(navEvent)); + + // After navigation, a scroll event fires and #detectPrimaryView() + // reverts #primaryIndex to the old section (same revert behaviour + // as before, but now we navigated to the correct section in the first place) + primaryIndex = prevIdx; + }), + getContents: vi.fn(() => []), + }, + addEventListener: vi.fn((type: string, listener: EventListener) => { + if (type === 'relocate') viewRelocateListeners.push(listener); + }), + removeEventListener: vi.fn((type: string, listener: EventListener) => { + if (type === 'relocate') { + const idx = viewRelocateListeners.indexOf(listener); + if (idx >= 0) viewRelocateListeners.splice(idx, 1); + } + }), + book: { format: 'EPUB' }, + getCFI: vi.fn(() => null), + addAnnotation: vi.fn(), + resolveCFI: vi.fn(), +}; + +// ---------- Tests ---------- + +describe('RSVPControl — section advance tracking', () => { + beforeEach(() => { + primaryIndex = 4; + loadedSections.length = 0; + viewRelocateListeners.length = 0; + controllerEventListeners.clear(); + capturedOnRequestNextPage = null; + }); + + afterEach(() => { + cleanup(); + }); + + test('advances directly to rsvpSectionRef+1, loading section 6 on second advance', async () => { + render( + , + ); + + // Start RSVP — handleStart auto-starts without showing a dialog because + // the mock controller fires rsvp-start-choice with no saved position + await act(async () => { + eventDispatcher.dispatch('rsvp-start', { bookKey: 'test-book' }); + await new Promise((r) => setTimeout(r, 20)); + }); + + // RSVPOverlay should now be mounted and onRequestNextPage captured + expect(capturedOnRequestNextPage).not.toBeNull(); + + // First section advance: rsvpSectionRef starts at 4 → goTo(5) + await act(async () => { + await capturedOnRequestNextPage!(); + }); + + expect(loadedSections[0]).toBe(5); // section 5 correctly loaded on first advance + expect(primaryIndex).toBe(4); // confirms primaryIndex has reverted after navigation + expect(mockView.renderer.goTo).toHaveBeenNthCalledWith(1, { index: 5 }); + + // Second advance: rsvpSectionRef is now 5 → goTo(6). + // Without the fix, nextSection() used the reverted primaryIndex=4 and + // navigated to section 5 again, causing a stale-section freeze. + await act(async () => { + await capturedOnRequestNextPage!(); + }); + + expect(loadedSections).toEqual([5, 6]); + expect(mockView.renderer.goTo).toHaveBeenNthCalledWith(2, { index: 6 }); + }); +}); diff --git a/apps/readest-app/src/__tests__/services/rsvp-persistence.test.ts b/apps/readest-app/src/__tests__/services/rsvp-persistence.test.ts new file mode 100644 index 00000000..7691c0ea --- /dev/null +++ b/apps/readest-app/src/__tests__/services/rsvp-persistence.test.ts @@ -0,0 +1,25 @@ +import { describe, expect, test } from 'vitest'; +import { buildRsvpExitConfigUpdate } from '@/services/rsvp/persistence'; + +describe('buildRsvpExitConfigUpdate', () => { + test('pins location to the RSVP word CFI when present', () => { + const rsvpPosition = { + cfi: 'epubcfi(/6/8!/4/2/1:42)', + wordText: 'somewhere', + }; + + const update = buildRsvpExitConfigUpdate(rsvpPosition); + + expect(update.rsvpPosition).toEqual(rsvpPosition); + expect(update.location).toBe('epubcfi(/6/8!/4/2/1:42)'); + }); + + test('omits location when the RSVP word has no CFI', () => { + const rsvpPosition = { cfi: '', wordText: 'no-cfi' }; + + const update = buildRsvpExitConfigUpdate(rsvpPosition); + + expect(update.rsvpPosition).toEqual(rsvpPosition); + expect('location' in update).toBe(false); + }); +}); diff --git a/apps/readest-app/src/app/reader/components/rsvp/RSVPControl.tsx b/apps/readest-app/src/app/reader/components/rsvp/RSVPControl.tsx index 964a4b06..ac79836b 100644 --- a/apps/readest-app/src/app/reader/components/rsvp/RSVPControl.tsx +++ b/apps/readest-app/src/app/reader/components/rsvp/RSVPControl.tsx @@ -6,11 +6,17 @@ import { useReaderStore } from '@/store/readerStore'; import { useBookDataStore } from '@/store/bookDataStore'; import { useSettingsStore } from '@/store/settingsStore'; import { useThemeStore } from '@/store/themeStore'; -import { RSVPController, RsvpStartChoice, RsvpStopPosition } from '@/services/rsvp'; +import { + RSVPController, + RsvpStartChoice, + RsvpStopPosition, + buildRsvpExitConfigUpdate, +} from '@/services/rsvp'; import { eventDispatcher } from '@/utils/event'; import { useEnv } from '@/context/EnvContext'; import { useTranslation } from '@/hooks/useTranslation'; -import { BookNote } from '@/types/book'; +import { BookNote, PageInfo } from '@/types/book'; +import { TOCItem } from '@/libs/document'; import { Insets } from '@/types/misc'; import RSVPOverlay from './RSVPOverlay'; import RSVPStartDialog from './RSVPStartDialog'; @@ -116,6 +122,10 @@ const RSVPControl: React.FC = ({ bookKey, gridInsets }) => { const [startChoice, setStartChoice] = useState(null); const controllerRef = useRef(null); const tempHighlightRef = useRef(null); + // renderer.primaryIndex reverts after navigation (paginator #detectPrimaryView), + // so track RSVP's actual section and chapter href in stable refs instead. + const rsvpSectionRef = useRef(-1); + const rsvpChapterHrefRef = useRef(null); // Helper to remove any existing RSVP highlight const removeRsvpHighlight = useCallback(() => { @@ -141,6 +151,8 @@ const RSVPControl: React.FC = ({ bookKey, gridInsets }) => { } // Remove any existing RSVP highlight when component unmounts removeRsvpHighlight(); + rsvpSectionRef.current = -1; + rsvpChapterHrefRef.current = null; }; // eslint-disable-next-line react-hooks/exhaustive-deps }, []); @@ -198,6 +210,8 @@ const RSVPControl: React.FC = ({ bookKey, gridInsets }) => { // Create controller if not exists if (!controllerRef.current) { controllerRef.current = new RSVPController(view, bookKey); + rsvpSectionRef.current = view.renderer.primaryIndex; + rsvpChapterHrefRef.current = progress?.sectionHref ?? null; } const controller = controllerRef.current; @@ -385,13 +399,17 @@ const RSVPControl: React.FC = ({ bookKey, gridInsets }) => { controller.stop(); } - // Persist RSVP position to BookConfig so it syncs to the cloud + // Persist RSVP position to BookConfig so it syncs to the cloud. Pin + // `location` to the RSVP word's CFI so the next normal-mode load resumes + // here instead of at a section boundary that a mid-RSVP relocate left + // behind in the auto-saved config. const rsvpPosition = controller?.getStoredPosition(); if (rsvpPosition) { const config = getConfig(bookKey); if (config) { - setConfig(bookKey, { rsvpPosition }); - saveConfig(envConfig, bookKey, { ...config, rsvpPosition }, settings); + const update = buildRsvpExitConfigUpdate(rsvpPosition); + setConfig(bookKey, update); + saveConfig(envConfig, bookKey, { ...config, ...update }, settings); } } @@ -414,8 +432,11 @@ const RSVPControl: React.FC = ({ bookKey, gridInsets }) => { const view = getView(bookKey); if (!view) return; - const onRelocate = () => { + const onRelocate = (e: Event) => { view.removeEventListener('relocate', onRelocate); + const detail = (e as CustomEvent).detail as { section?: PageInfo; tocItem?: TOCItem }; + rsvpSectionRef.current = detail.section?.current ?? view.renderer.primaryIndex; + rsvpChapterHrefRef.current = detail.tocItem?.href ?? null; const controller = controllerRef.current; if (controller) { const progress = getProgress(bookKey); @@ -437,18 +458,30 @@ const RSVPControl: React.FC = ({ bookKey, gridInsets }) => { removeRsvpHighlight(); - const indexBefore = view.renderer.primaryIndex; + if (view.renderer.atEnd) { + controllerRef.current?.pause(); + return; + } + + const indexBefore = + rsvpSectionRef.current >= 0 ? rsvpSectionRef.current : view.renderer.primaryIndex; + + let cleanup: ReturnType | null = null; + + const onRelocate = (e: Event) => { + const detail = (e as CustomEvent).detail as { section?: PageInfo; tocItem?: TOCItem }; + const newIndex = detail.section?.current ?? view.renderer.primaryIndex; + + if (newIndex === indexBefore) return; // revert relocate — keep waiting - const onRelocate = () => { view.removeEventListener('relocate', onRelocate); + if (cleanup) clearTimeout(cleanup); + const controller = controllerRef.current; if (!controller) return; - // Pause at the end of the book instead of advancing - if (view.renderer.primaryIndex === indexBefore) { - controller.pause(); - return; - } + rsvpSectionRef.current = newIndex; + rsvpChapterHrefRef.current = detail.tocItem?.href ?? null; const progress = getProgress(bookKey); if (progress?.location) { @@ -456,15 +489,21 @@ const RSVPControl: React.FC = ({ bookKey, gridInsets }) => { } controller.loadNextPageContent(); }; + view.addEventListener('relocate', onRelocate); - await view.renderer.nextSection?.(); + cleanup = setTimeout(() => view.removeEventListener('relocate', onRelocate), 5000); + // Navigate directly to rsvpSectionRef.current + 1 rather than calling nextSection(), + // which uses renderer.primaryIndex internally. primaryIndex reverts to the previous + // section after navigation (#detectPrimaryView), so nextSection() would re-navigate + // to the already-current section and the onRelocate filter would discard the event. + await view.renderer.goTo({ index: rsvpSectionRef.current + 1 }); }, [bookKey, getProgress, getView, removeRsvpHighlight]); // Get current chapter info const progress = getProgress(bookKey); const bookData = getBookData(bookKey); const chapters = bookData?.bookDoc?.toc || []; - const currentChapterHref = progress?.sectionHref || null; + const currentChapterHref = rsvpChapterHrefRef.current ?? progress?.sectionHref ?? null; // Use portal to render overlay at body level to avoid stacking context issues const portalContainer = typeof document !== 'undefined' ? document.body : null; diff --git a/apps/readest-app/src/app/reader/components/rsvp/RSVPOverlay.tsx b/apps/readest-app/src/app/reader/components/rsvp/RSVPOverlay.tsx index 65306567..d6327a10 100644 --- a/apps/readest-app/src/app/reader/components/rsvp/RSVPOverlay.tsx +++ b/apps/readest-app/src/app/reader/components/rsvp/RSVPOverlay.tsx @@ -34,20 +34,6 @@ const STORAGE_KEY_FONT_SIZE = 'readest_rsvp_fontsize'; const STORAGE_KEY_ORP_COLOR = 'readest_rsvp_orp_color'; const STORAGE_KEY_CONTEXT = 'readest_rsvp_context'; -// Context window: render only a sliding window of words around the current index. -// Why: full-chapter rendering can be thousands of spans, and iOS WebKit's layout cost -// for getBoundingClientRect (used by auto-scroll) scales with DOM size, which throttles -// the word-advance interval well below the configured WPM. -const CONTEXT_WINDOW_SIZE = 500; -const CONTEXT_WINDOW_SLIDE_THRESHOLD = 100; - -const computeContextWindow = (total: number, currentIndex: number) => { - if (total <= CONTEXT_WINDOW_SIZE) return { start: 0, end: total }; - const half = Math.floor(CONTEXT_WINDOW_SIZE / 2); - const start = Math.max(0, Math.min(total - CONTEXT_WINDOW_SIZE, currentIndex - half)); - return { start, end: start + CONTEXT_WINDOW_SIZE }; -}; - interface RSVPOverlayProps { gridInsets: Insets; controller: RSVPController; @@ -107,11 +93,7 @@ const RSVPOverlay: React.FC = ({ } return 0; }); - const [contextWindow, setContextWindow] = useState(() => - computeContextWindow(state.words.length, state.currentIndex), - ); const contextWordRef = useRef(null); - const contextPanelRef = useRef(null); const touchStartX = useRef(0); const touchStartY = useRef(0); const touchStartTime = useRef(0); @@ -241,48 +223,11 @@ const RSVPOverlay: React.FC = ({ } }, [state]); - // Stable word list that only changes when contextWindow changes - const contextWords = useMemo( - () => state.words.slice(contextWindow.start, contextWindow.end), - [state.words, contextWindow], - ); - - // Slide/reset the context window as currentIndex moves or the chapter changes. + // Auto-scroll: keep highlighted word in view useEffect(() => { - const total = state.words.length; - const cur = state.currentIndex; - setContextWindow((prev) => { - if (total <= CONTEXT_WINDOW_SIZE) { - if (prev.start === 0 && prev.end === total) return prev; - return { start: 0, end: total }; - } - const currentSize = prev.end - prev.start; - const outOfBounds = cur < prev.start || cur >= prev.end; - const nearStartEdge = prev.start > 0 && cur < prev.start + CONTEXT_WINDOW_SLIDE_THRESHOLD; - const nearEndEdge = prev.end < total && cur >= prev.end - CONTEXT_WINDOW_SLIDE_THRESHOLD; - const sizeMismatch = currentSize !== CONTEXT_WINDOW_SIZE || prev.end > total; - if (!outOfBounds && !nearStartEdge && !nearEndEdge && !sizeMismatch) return prev; - return computeContextWindow(total, cur); - }); - }, [state.currentIndex, state.words]); - - // Auto-scroll: keep highlighted word away from top/bottom edges - useEffect(() => { - const panel = contextPanelRef.current; - const word = contextWordRef.current; - if (contextCollapsed || !panel || !word) return; - - const panelRect = panel.getBoundingClientRect(); - const wordRect = word.getBoundingClientRect(); - const margin = panelRect.height * 0.15; - const topLine = panelRect.top + margin; - - if (wordRect.top < topLine) { - panel.scrollTop -= topLine - wordRect.top; - } else if (wordRect.bottom > panelRect.bottom - margin) { - panel.scrollTop += wordRect.top - topLine; - } - }, [state.currentIndex, contextCollapsed, contextWindow]); + if (contextCollapsed) return; + contextWordRef.current?.scrollIntoView({ block: 'nearest', behavior: 'instant' }); + }, [state.currentIndex, contextCollapsed]); useEffect(() => { if (!showChapterDropdown) return; @@ -604,14 +549,13 @@ const RSVPOverlay: React.FC = ({ {!contextCollapsed && (
e.stopPropagation()} onTouchEnd={(e) => e.stopPropagation()} >
- {contextWords.map((w, i) => { - const wordIndex = contextWindow.start + i; + {state.words.map((w, i) => { + const wordIndex = i; const isCurrent = wordIndex === state.currentIndex; return ( => { + if (rsvpPosition.cfi) { + return { rsvpPosition, location: rsvpPosition.cfi }; + } + return { rsvpPosition }; +}; diff --git a/apps/readest-app/src/types/view.ts b/apps/readest-app/src/types/view.ts index 91a3c5c5..e3f44c5d 100644 --- a/apps/readest-app/src/types/view.ts +++ b/apps/readest-app/src/types/view.ts @@ -34,7 +34,7 @@ export interface Renderer extends HTMLElement { nextSection?: () => Promise; prevSection?: () => Promise; render?: () => Promise; - goTo: (params: { index: number; anchor?: number | RangeAnchor }) => void; + goTo: (params: { index: number; anchor?: number | RangeAnchor }) => Promise; setStyles?: (css: string) => void; primaryIndex: number; getContents: () => { doc: Document; index?: number; overlayer?: unknown }[];