From bc9b8b23e6e88585303e209a1ef3588f52394589 Mon Sep 17 00:00:00 2001 From: Huang Xin Date: Tue, 23 Jun 2026 00:12:39 +0800 Subject: [PATCH] fix(reader): stop per-chapter listener leak that degrades paragraph mode (#4735) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The annotator's foliate `load` handler (onLoad) attached a renderer `scroll` listener and, on Android, a global `native-touch` dispatcher listener on every section load. Both the renderer and the eventDispatcher outlive individual sections — and foliate fires `load` for preloaded neighbour sections too — so these listeners accumulated without bound, one set per chapter. Each renderer `scroll` (fired on every paragraph-mode `goTo`) then ran all of them, and on Android the scroll/native-touch handlers do real work. Reading a long book (e.g. a 3000-chapter web novel) in paragraph mode slowed down steadily after a few chapters and only an app restart cleared it. Register these listeners once per view via a new `useRendererInputListeners` hook with cleanup, instead of once per section load. The native-touch handler now resolves the CURRENT primary section's doc/index at fire time rather than capturing a (possibly off-screen, preloaded) section's. The redundant `scroll` → `repositionPopups` listener is dropped — a dedicated effect already repositions popups on scroll. Doc-scoped listeners stay in onLoad, since they die with the section's iframe. Add useRendererInputListeners unit tests covering register-once-per-view, no-accumulation-across-re-renders, latest-handler routing, Android gating, and unmount cleanup. Co-authored-by: Claude Opus 4.8 (1M context) --- .../hooks/useRendererInputListeners.test.ts | 173 ++++++++++++++++++ .../reader/components/annotator/Annotator.tsx | 71 ++++--- .../reader/hooks/useRendererInputListeners.ts | 72 ++++++++ 3 files changed, 289 insertions(+), 27 deletions(-) create mode 100644 apps/readest-app/src/__tests__/app/reader/hooks/useRendererInputListeners.test.ts create mode 100644 apps/readest-app/src/app/reader/hooks/useRendererInputListeners.ts diff --git a/apps/readest-app/src/__tests__/app/reader/hooks/useRendererInputListeners.test.ts b/apps/readest-app/src/__tests__/app/reader/hooks/useRendererInputListeners.test.ts new file mode 100644 index 00000000..16c01d60 --- /dev/null +++ b/apps/readest-app/src/__tests__/app/reader/hooks/useRendererInputListeners.test.ts @@ -0,0 +1,173 @@ +import { afterEach, describe, expect, it, vi } from 'vitest'; +import { cleanup, renderHook } from '@testing-library/react'; + +// A controllable stand-in for the global eventDispatcher: it tracks the +// `native-touch` listeners so a test can assert they never accumulate (the bug +// was that each section load added one and none were ever removed). +const nativeTouchListeners = new Set<(e: CustomEvent) => void>(); +vi.mock('@/utils/event', () => ({ + eventDispatcher: { + on: vi.fn((event: string, cb: (e: CustomEvent) => void) => { + if (event === 'native-touch') nativeTouchListeners.add(cb); + }), + off: vi.fn((event: string, cb: (e: CustomEvent) => void) => { + if (event === 'native-touch') nativeTouchListeners.delete(cb); + }), + }, +})); + +import { useRendererInputListeners } from '@/app/reader/hooks/useRendererInputListeners'; +import type { FoliateView } from '@/types/view'; + +// Stand-in for the long-lived foliate paginator (an EventTarget). It records its +// `scroll` listeners so a test can prove they stay at exactly one per view. +class MockRenderer extends EventTarget { + scrollListeners = new Set(); + override addEventListener(type: string, cb: EventListenerOrEventListenerObject, opts?: unknown) { + if (type === 'scroll') this.scrollListeners.add(cb); + super.addEventListener(type, cb, opts as AddEventListenerOptions); + } + override removeEventListener( + type: string, + cb: EventListenerOrEventListenerObject, + opts?: unknown, + ) { + if (type === 'scroll') this.scrollListeners.delete(cb); + super.removeEventListener(type, cb, opts as EventListenerOptions); + } +} + +const makeView = (renderer: MockRenderer) => ({ renderer }) as unknown as FoliateView; + +const noopOpts = { + onRendererScroll: () => {}, + enableNativeTouch: false, + listenToNativeTouchEvents: () => {}, +}; + +afterEach(() => { + cleanup(); + nativeTouchListeners.clear(); + vi.clearAllMocks(); +}); + +describe('useRendererInputListeners (paragraph-mode scroll/touch leak)', () => { + it('registers exactly one renderer scroll listener and never accumulates across re-renders', () => { + const renderer = new MockRenderer(); + const view = makeView(renderer); + const { rerender } = renderHook( + ({ s }) => useRendererInputListeners(view, { ...noopOpts, onRendererScroll: s }), + { initialProps: { s: vi.fn() } }, + ); + + expect(renderer.scrollListeners.size).toBe(1); + + // A long reading session re-renders the annotator many times (every section + // load, progress update, popup toggle…). None of those may add a listener. + for (let i = 0; i < 20; i++) rerender({ s: vi.fn() }); + expect(renderer.scrollListeners.size).toBe(1); + }); + + it('routes scroll events to the latest handler without re-subscribing', () => { + const renderer = new MockRenderer(); + const view = makeView(renderer); + const first = vi.fn(); + const { rerender } = renderHook( + ({ s }) => useRendererInputListeners(view, { ...noopOpts, onRendererScroll: s }), + { initialProps: { s: first } }, + ); + + const second = vi.fn(); + rerender({ s: second }); + renderer.dispatchEvent(new Event('scroll')); + + expect(first).not.toHaveBeenCalled(); + expect(second).toHaveBeenCalledTimes(1); + expect(renderer.scrollListeners.size).toBe(1); + }); + + it('removes the renderer scroll listener on unmount', () => { + const renderer = new MockRenderer(); + const { unmount } = renderHook(() => useRendererInputListeners(makeView(renderer), noopOpts)); + + expect(renderer.scrollListeners.size).toBe(1); + unmount(); + expect(renderer.scrollListeners.size).toBe(0); + }); + + it('wires the Android native-touch bridge exactly once and tears it down on unmount', () => { + const renderer = new MockRenderer(); + const view = makeView(renderer); + const listenToNativeTouchEvents = vi.fn(); + const { rerender, unmount } = renderHook( + ({ t }) => + useRendererInputListeners(view, { + onRendererScroll: () => {}, + onNativeTouch: t, + enableNativeTouch: true, + listenToNativeTouchEvents, + }), + { initialProps: { t: vi.fn() } }, + ); + + expect(nativeTouchListeners.size).toBe(1); + expect(listenToNativeTouchEvents).toHaveBeenCalledTimes(1); + + for (let i = 0; i < 20; i++) rerender({ t: vi.fn() }); + expect(nativeTouchListeners.size).toBe(1); + expect(listenToNativeTouchEvents).toHaveBeenCalledTimes(1); + + unmount(); + expect(nativeTouchListeners.size).toBe(0); + }); + + it('does not wire native-touch when not on Android', () => { + const renderer = new MockRenderer(); + const listenToNativeTouchEvents = vi.fn(); + renderHook(() => + useRendererInputListeners(makeView(renderer), { + onRendererScroll: () => {}, + onNativeTouch: vi.fn(), + enableNativeTouch: false, + listenToNativeTouchEvents, + }), + ); + + expect(nativeTouchListeners.size).toBe(0); + expect(listenToNativeTouchEvents).not.toHaveBeenCalled(); + }); + + it('delivers native-touch events to the latest handler with the live event detail', () => { + const renderer = new MockRenderer(); + const view = makeView(renderer); + const first = vi.fn(); + const { rerender } = renderHook( + ({ t }) => + useRendererInputListeners(view, { + onRendererScroll: () => {}, + onNativeTouch: t, + enableNativeTouch: true, + listenToNativeTouchEvents: () => {}, + }), + { initialProps: { t: first } }, + ); + + const second = vi.fn(); + rerender({ t: second }); + const detail = { type: 'touchmove', x: 1, y: 2 }; + nativeTouchListeners.forEach((cb) => cb(new CustomEvent('native-touch', { detail }))); + + expect(first).not.toHaveBeenCalled(); + expect(second).toHaveBeenCalledWith(detail); + }); + + it('does not register anything until a view exists', () => { + const { rerender } = renderHook(({ v }) => useRendererInputListeners(v, noopOpts), { + initialProps: { v: null as FoliateView | null }, + }); + // No view yet → no throw, nothing wired. + const renderer = new MockRenderer(); + rerender({ v: makeView(renderer) }); + expect(renderer.scrollListeners.size).toBe(1); + }); +}); diff --git a/apps/readest-app/src/app/reader/components/annotator/Annotator.tsx b/apps/readest-app/src/app/reader/components/annotator/Annotator.tsx index 28e63fb6..33cb8b88 100644 --- a/apps/readest-app/src/app/reader/components/annotator/Annotator.tsx +++ b/apps/readest-app/src/app/reader/components/annotator/Annotator.tsx @@ -22,6 +22,7 @@ import { useTranslation } from '@/hooks/useTranslation'; import { useResponsiveSize } from '@/hooks/useResponsiveSize'; import { useDeviceControlStore } from '@/store/deviceStore'; import { useFoliateEvents } from '../../hooks/useFoliateEvents'; +import { useRendererInputListeners } from '../../hooks/useRendererInputListeners'; import { useNotesSync } from '../../hooks/useNotesSync'; import { useReadwiseSync } from '../../hooks/useReadwiseSync'; import { useHardcoverSync } from '../../hooks/useHardcoverSync'; @@ -356,35 +357,16 @@ const Annotator: React.FC<{ bookKey: string; contentInsets: Insets }> = ({ handleTouchMove(ev); }; - const handleNativeTouch = (event: CustomEvent) => { - const ev = event.detail as NativeTouchEventType; - if (ev.type === 'touchstart') { - androidTouchEndRef.current = false; - beginGesture(deferredQuickActionRef.current); - handleTouchStart(); - } else if (ev.type === 'touchmove') { - // The Android pointer engagement signal (throttled in MainActivity.kt). - handleNativeTouchMove(ev.x, ev.y, doc); - } else if (ev.type === 'touchend') { - androidTouchEndRef.current = true; - handleTouchEnd(); - handlePointerUp(doc, index); - flushDeferredAction(deferredQuickActionRef.current); - } - }; - - if (appService?.isAndroidApp) { - listenToNativeTouchEvents(); - eventDispatcher.on('native-touch', handleNativeTouch); - } - // Attach generic selection listeners for all formats, including PDF. // For PDF we only guarantee Copy & Translate; highlight/annotate may be limited by CFI support. - view?.renderer?.addEventListener('scroll', handleScroll); - // Reposition popups on scroll to keep them in view - view?.renderer?.addEventListener('scroll', () => { - repositionPopups(); - }); + // + // The renderer `scroll` listener and the Android `native-touch` bridge are + // NOT attached here: onLoad fires for every (pre)loaded section, but those + // listeners live on the renderer / global dispatcher, which outlive sections. + // Attaching them per load leaked one set per chapter and degraded paragraph + // mode over a long session. They are registered once per view via + // useRendererInputListeners below. Popup repositioning on scroll is already + // handled by the dedicated effect further down. const opts = { passive: false }; detail.doc?.addEventListener('touchstart', handleTouchStart, opts); detail.doc?.addEventListener('touchmove', handleTouchmove, opts); @@ -592,6 +574,41 @@ const Annotator: React.FC<{ bookKey: string; contentInsets: Insets }> = ({ useFoliateEvents(view, { onLoad, onCreateOverlay, onDrawAnnotation, onShowAnnotation }); + // Android native-touch handler (the per-gesture engagement signal bridged from + // MainActivity.kt). Registered once per view by useRendererInputListeners; it + // resolves the CURRENT primary section's doc/index at fire time rather than + // capturing them at load time, because foliate also fires `load` for preloaded + // neighbour sections, whose doc/index would be off-screen. + const handleNativeTouch = (ev: NativeTouchEventType) => { + const contents = view?.renderer?.getContents?.() ?? []; + const content = contents.find((c) => c.index === view?.renderer?.primaryIndex) ?? contents[0]; + const doc = content?.doc; + const index = content?.index; + if (!doc || index === undefined) return; + if (ev.type === 'touchstart') { + androidTouchEndRef.current = false; + beginGesture(deferredQuickActionRef.current); + handleTouchStart(); + } else if (ev.type === 'touchmove') { + handleNativeTouchMove(ev.x, ev.y, doc); + } else if (ev.type === 'touchend') { + androidTouchEndRef.current = true; + handleTouchEnd(); + handlePointerUp(doc, index); + flushDeferredAction(deferredQuickActionRef.current); + } + }; + + // Register the renderer `scroll` listener and (on Android) the `native-touch` + // bridge once per view, with cleanup — see the hook for why attaching these in + // onLoad leaked listeners and degraded paragraph mode over a long session. + useRendererInputListeners(view, { + onRendererScroll: handleScroll, + onNativeTouch: handleNativeTouch, + enableNativeTouch: !!appService?.isAndroidApp, + listenToNativeTouchEvents, + }); + // Word Lens: open the dictionary popup for a tapped glossed word. The tap is // detected in the iframe click handler (iframeEventHandlers.ts), which sends // the gloss element here. We synthesize a selection over the base word diff --git a/apps/readest-app/src/app/reader/hooks/useRendererInputListeners.ts b/apps/readest-app/src/app/reader/hooks/useRendererInputListeners.ts new file mode 100644 index 00000000..f47d84a4 --- /dev/null +++ b/apps/readest-app/src/app/reader/hooks/useRendererInputListeners.ts @@ -0,0 +1,72 @@ +import { useEffect, useRef } from 'react'; +import { FoliateView } from '@/types/view'; +import { NativeTouchEventType } from '@/types/system'; +import { eventDispatcher } from '@/utils/event'; + +interface RendererInputListenersOptions { + /** Fired on every renderer `scroll` event (the long-lived paginator). */ + onRendererScroll: () => void; + /** Fired on each Android `native-touch` event; ignored when not on Android. */ + onNativeTouch?: (ev: NativeTouchEventType) => void; + /** Whether to wire the Android native-touch bridge for this view. */ + enableNativeTouch: boolean; + /** Starts the native touch-event bridge (safe to call once per view). */ + listenToNativeTouchEvents: () => void; +} + +/** + * Registers the input listeners that must live as long as the book view does — + * the renderer `scroll` listener and (on Android) the global `native-touch` + * dispatcher listener — exactly ONCE per view, with cleanup. + * + * These used to be attached inside the foliate `load` handler (the annotator's + * `onLoad`), which runs on every section load — including foliate's *preloaded* + * neighbour sections. Both the renderer and the global eventDispatcher outlive + * individual sections, so those listeners accumulated without bound: every + * chapter transition added more, and each renderer `scroll` (fired on every + * paragraph-mode `goTo`) then ran all of them. Reading a long book — especially + * in paragraph mode on Android, where the scroll/native-touch handlers do real + * work — degraded steadily until the app was restarted. + * + * The handlers are read through refs, so a re-render never re-subscribes; the + * effect re-runs only when the view (or the Android gate) changes. + */ +export const useRendererInputListeners = ( + view: FoliateView | null, + { + onRendererScroll, + onNativeTouch, + enableNativeTouch, + listenToNativeTouchEvents, + }: RendererInputListenersOptions, +) => { + const onRendererScrollRef = useRef(onRendererScroll); + onRendererScrollRef.current = onRendererScroll; + const onNativeTouchRef = useRef(onNativeTouch); + onNativeTouchRef.current = onNativeTouch; + + useEffect(() => { + const renderer = view?.renderer; + if (!renderer) return; + + const handleScroll = () => onRendererScrollRef.current(); + renderer.addEventListener('scroll', handleScroll); + + let offNativeTouch: (() => void) | undefined; + if (enableNativeTouch) { + listenToNativeTouchEvents(); + const handleNativeTouch = (event: CustomEvent) => + onNativeTouchRef.current?.(event.detail as NativeTouchEventType); + eventDispatcher.on('native-touch', handleNativeTouch); + offNativeTouch = () => eventDispatcher.off('native-touch', handleNativeTouch); + } + + return () => { + renderer.removeEventListener('scroll', handleScroll); + offNativeTouch?.(); + }; + // `listenToNativeTouchEvents` is a stable store action and the handlers are + // routed through refs, so the effect only depends on the view + the gate. + // eslint-disable-next-line react-hooks/exhaustive-deps + }, [view, enableNativeTouch]); +};