diff --git a/apps/readest-app/src/__tests__/utils/global-annotations.test.ts b/apps/readest-app/src/__tests__/utils/global-annotations.test.ts new file mode 100644 index 00000000..81ce49bd --- /dev/null +++ b/apps/readest-app/src/__tests__/utils/global-annotations.test.ts @@ -0,0 +1,126 @@ +import { describe, it, expect, vi } from 'vitest'; +import { + expandGlobalAnnotation, + removeGlobalAnnotationOverlays, +} from '@/app/reader/utils/globalAnnotations'; +import { BookNote } from '@/types/book'; +import { FoliateView } from '@/types/view'; + +/** + * Regression coverage for issue #4575: highlighting recurring character names + * (global annotations) made page turning very laggy. The `progress` effect + * re-fans-out every global annotation across every rendered section on EVERY + * page turn. Because the overlays are already drawn, re-walking the DOM and + * re-computing `view.getCFI()` for every occurrence is pure wasted work — the + * dominant per-page-turn cost on books with frequent highlighted words. + * + * `expandGlobalAnnotation` must therefore be idempotent: expanding the same + * note into the same (already-expanded) section a second time must do no work. + */ + +const note = (overrides: Partial): BookNote => ({ + id: 'note-1', + type: 'annotation', + cfi: 'home-cfi', + note: '', + text: 'foo', + style: 'highlight', + color: 'yellow', + global: true, + createdAt: 1, + updatedAt: 1, + ...overrides, +}); + +/** A jsdom section document where `text` recurs `count` times. */ +const makeDoc = (needle: string, count: number): Document => { + const doc = document.implementation.createHTMLDocument('section'); + const body = Array.from({ length: count }, () => `${needle} `).join('gap '); + doc.body.innerHTML = `

${body}

`; + return doc; +}; + +type FakeView = FoliateView & { getCfiCalls: number }; + +/** + * Minimal stand-in for the foliate view. `getCFI` returns a unique value per + * call (so it never collides with the note's home anchor) and counts how many + * times it ran — our proxy for the expensive per-occurrence work a page turn + * would repeat. + */ +const makeView = (docs: Document[]): FakeView => { + const overlayer = { add: vi.fn(), remove: vi.fn() }; + const view = { + getCfiCalls: 0, + renderer: { + getContents: () => docs.map((doc, index) => ({ index, doc, overlayer })), + }, + getCFI(_index: number, _range: Range) { + this.getCfiCalls += 1; + return `cfi-${this.getCfiCalls}`; + }, + dispatchEvent: () => true, + }; + return view as unknown as FakeView; +}; + +describe('expandGlobalAnnotation idempotency (issue #4575)', () => { + it('expands every occurrence on first call for a section', () => { + const doc = makeDoc('foo', 3); + const view = makeView([doc]); + const added = expandGlobalAnnotation(view, note({}), doc, 0); + expect(added).toHaveLength(3); + expect(view.getCfiCalls).toBe(3); + }); + + it('does NO work when re-expanding the same note into the same section', () => { + const doc = makeDoc('foo', 3); + const view = makeView([doc]); + const n = note({ id: 'same-note' }); + expandGlobalAnnotation(view, n, doc, 0); + expect(view.getCfiCalls).toBe(3); + + // Second pass simulates the next page turn — overlays already exist. + const added2 = expandGlobalAnnotation(view, n, doc, 0); + expect(added2).toEqual([]); + expect(view.getCfiCalls).toBe(3); // unchanged: no re-walk, no re-getCFI + }); + + it('re-expands when the note content changes (edit/recolor bumps updatedAt)', () => { + const doc = makeDoc('foo', 2); + const view = makeView([doc]); + const n = note({ id: 'edited-note', updatedAt: 10 }); + expandGlobalAnnotation(view, n, doc, 0); + expect(view.getCfiCalls).toBe(2); + const added = expandGlobalAnnotation(view, { ...n, updatedAt: 11 }, doc, 0); + expect(added).toHaveLength(2); + expect(view.getCfiCalls).toBe(4); + }); + + it('re-expands after overlays are removed (toggle global off then on)', () => { + const doc = makeDoc('foo', 2); + const view = makeView([doc]); + const n = note({ id: 'retoggle-note' }); + expandGlobalAnnotation(view, n, doc, 0); + expect(view.getCfiCalls).toBe(2); + + // Toggle off: overlays torn down and the memo cleared. + removeGlobalAnnotationOverlays(view, n); + + // Toggle back on with identical content — must re-fan-out, not short-circuit. + const added = expandGlobalAnnotation(view, n, doc, 0); + expect(added).toHaveLength(2); + expect(view.getCfiCalls).toBe(4); + }); + + it('re-expands into a freshly rendered section (different doc)', () => { + const docA = makeDoc('foo', 2); + const docB = makeDoc('foo', 2); + const view = makeView([docA, docB]); + const n = note({ id: 'multi-section' }); + expandGlobalAnnotation(view, n, docA, 0); + const addedB = expandGlobalAnnotation(view, n, docB, 1); + expect(addedB).toHaveLength(2); + expect(view.getCfiCalls).toBe(4); // 2 per distinct section + }); +}); diff --git a/apps/readest-app/src/app/reader/utils/globalAnnotations.ts b/apps/readest-app/src/app/reader/utils/globalAnnotations.ts index 7efd801e..3ecb655b 100644 --- a/apps/readest-app/src/app/reader/utils/globalAnnotations.ts +++ b/apps/readest-app/src/app/reader/utils/globalAnnotations.ts @@ -174,6 +174,28 @@ interface OverlayerLike { remove: (value: string) => void; } +/** + * Per-section memo of which global notes have already been fanned out. + * + * Issue #4575: the `progress` effect re-expands every global annotation across + * every rendered section on EVERY page turn. The overlays already exist after + * the first pass, so re-walking the DOM and re-computing `view.getCFI()` for + * each occurrence is pure wasted work — and on books where highlighted words + * (e.g. recurring character names) appear hundreds of times per chapter it was + * the dominant per-page-turn cost (tens of ms of synchronous main-thread work + * each turn, multiplied on slower mobile hardware). + * + * Keying on the live section `Document` makes invalidation automatic: a section + * that is unloaded and later re-rendered gets a fresh `Document`, so its + * overlays are rebuilt; the stale entry is collected with the old document. The + * signature embeds `updatedAt` (bumped on every edit/recolor/global toggle) so + * an edited note re-expands once the caller has torn down its old overlays. + */ +const expandedByDoc = new WeakMap>(); + +const annotationSignature = (note: BookNote): string => + `${note.updatedAt ?? 0}:${note.style ?? ''}:${note.color ?? ''}:${note.text ?? ''}`; + /** * For a single `note` with `global=true`, fan out one overlay per * occurrence of `note.text` in the given section. @@ -212,6 +234,12 @@ export function expandGlobalAnnotation( if (!live) return []; const { overlayer } = live; + // Skip the (expensive) re-fan-out when this exact note has already been + // expanded into this section's current document — see `expandedByDoc`. + const signature = annotationSignature(note); + let docMemo = expandedByDoc.get(doc); + if (docMemo?.get(note.id) === signature) return []; + const added: string[] = []; const seen = new Set(); let occ = 0; @@ -253,6 +281,17 @@ export function expandGlobalAnnotation( console.warn('Failed to add global annotation overlay', { note: note.id, err }); } } + + // Record that this note (at its current content signature) is now expanded + // into this section's document, so later page turns short-circuit above. + // Recorded even when there were zero matches — a note that doesn't appear in + // this section must not be re-walked every turn either. + if (!docMemo) { + docMemo = new Map(); + expandedByDoc.set(doc, docMemo); + } + docMemo.set(note.id, signature); + return added; } @@ -309,11 +348,15 @@ export function removeGlobalAnnotationOverlays( if (!view) return; const sections = (view.renderer?.getContents?.() ?? []) as Array<{ index?: number; + doc?: Document; overlayer?: OverlayerLike; }>; for (const section of sections) { - const { index, overlayer } = section; + const { index, doc, overlayer } = section; if (typeof index !== 'number' || !overlayer) continue; + // Drop the expansion memo so a later re-add re-fans-out even if the note's + // content signature is unchanged (e.g. toggle global off then on again). + if (doc) expandedByDoc.get(doc)?.delete(note.id); for (let i = 0; i < maxOccurrencesPerSection; i += 1) { const value = makeSyntheticValue(note, index, i); try {