forked from akai/readest
Highlighting recurring text (e.g. main-character names) as global annotations made page turning very laggy. The `progress` effect re-fans-out every global annotation across every rendered section on EVERY page turn, and each pass re-walks the section DOM, recomputes `view.getCFI()` for every occurrence, and tears down + recreates an SVG overlay per match. The overlays already exist after the first pass, so this is pure wasted work — profiled at ~25–45ms of synchronous main-thread time per page turn for 6 names / 226 occurrences across 2 rendered chapters, multiplied on slower mobile hardware. Memoize, per live section `Document`, which global notes have been expanded (signature embeds `updatedAt`/style/color/text). Subsequent page turns short-circuit to ~0ms. Keying on the `Document` makes invalidation automatic: a re-rendered section gets a fresh document (and fresh overlayer) so its overlays are rebuilt, while edits/recolors bump `updatedAt` and toggling global off clears the memo. Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -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>): 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 = `<p>${body}</p>`;
|
||||
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
|
||||
});
|
||||
});
|
||||
@@ -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<Document, Map<string, string>>();
|
||||
|
||||
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<string>();
|
||||
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<string, string>();
|
||||
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 {
|
||||
|
||||
Reference in New Issue
Block a user