From ff96c6d3f771cb3b20770b1cabe8635bb2284a2f Mon Sep 17 00:00:00 2001 From: Huang Xin Date: Thu, 18 Jun 2026 21:37:46 +0800 Subject: [PATCH] feat(annotations): unify highlights and annotations into one record (#3870, #4511) (#4647) A highlight and its note are now a single BookNote. Adding a note attaches it to the highlight at that CFI (or creates one with the current global style) instead of creating a second record, and a unified record renders as both a highlight overlay and a note bubble. - onDrawAnnotation chooses the draw kind from the overlay value prefix (cfi -> highlight, NOTE_PREFIX -> bubble) instead of annotation.note, so a record with both a style and a note draws both. Fixes notes synced from KOReader losing their highlight (#4511). - handleSaveNote updates the existing annotation at the CFI rather than pushing a new record (#3870); re-styling preserves the note. - unifyAnnotations migration (book config schema v1 -> v2, run in deserializeConfig) collapses existing split highlight+note records into one survivor and tombstones the redundant record (deletedAt) so the merge syncs to the cloud and KOReader. - Sidebar: a note's quoted highlight text uses the theme foreground so it stays legible on the highlight background. Co-authored-by: Claude Opus 4.8 (1M context) --- .../app/reader/utils/annotatorUtil.test.ts | 82 ++++++++++++++++ .../__tests__/utils/booknoteMigration.test.ts | 98 +++++++++++++++++++ .../src/__tests__/utils/serializer.test.ts | 67 +++++++++++++ .../reader/components/annotator/Annotator.tsx | 34 ++++--- .../reader/components/notebook/Notebook.tsx | 49 +++++++--- .../components/sidebar/BooknoteItem.tsx | 2 +- .../src/app/reader/utils/annotatorUtil.ts | 47 +++++++++ apps/readest-app/src/types/book.ts | 2 +- .../src/utils/booknoteMigration.ts | 75 ++++++++++++++ apps/readest-app/src/utils/serializer.ts | 6 ++ 10 files changed, 436 insertions(+), 26 deletions(-) create mode 100644 apps/readest-app/src/__tests__/app/reader/utils/annotatorUtil.test.ts create mode 100644 apps/readest-app/src/__tests__/utils/booknoteMigration.test.ts create mode 100644 apps/readest-app/src/utils/booknoteMigration.ts diff --git a/apps/readest-app/src/__tests__/app/reader/utils/annotatorUtil.test.ts b/apps/readest-app/src/__tests__/app/reader/utils/annotatorUtil.test.ts new file mode 100644 index 00000000..16ea1197 --- /dev/null +++ b/apps/readest-app/src/__tests__/app/reader/utils/annotatorUtil.test.ts @@ -0,0 +1,82 @@ +import { describe, expect, it } from 'vitest'; +import { + decideAnnotationDraw, + findAnnotationAtCfi, + mergeRestyledAnnotation, +} from '@/app/reader/utils/annotatorUtil'; +import { BookNote } from '@/types/book'; +import { NOTE_PREFIX } from '@/types/view'; + +const makeNote = (over: Partial): BookNote => ({ + id: 'id', + type: 'annotation', + cfi: 'epubcfi(/6/4!/4)', + note: '', + createdAt: 1, + updatedAt: 1, + ...over, +}); + +describe('decideAnnotationDraw', () => { + it('returns bubble for a note-prefixed overlay value regardless of style', () => { + expect(decideAnnotationDraw(`${NOTE_PREFIX}epubcfi(/6/4!/4)`, 'highlight')).toBe('bubble'); + expect(decideAnnotationDraw(`${NOTE_PREFIX}epubcfi(/6/4!/4)`, undefined)).toBe('bubble'); + }); + + it('returns the style kind for a plain cfi overlay value', () => { + expect(decideAnnotationDraw('epubcfi(/6/4!/4)', 'highlight')).toBe('highlight'); + expect(decideAnnotationDraw('epubcfi(/6/4!/4)', 'underline')).toBe('underline'); + expect(decideAnnotationDraw('epubcfi(/6/4!/4)', 'squiggly')).toBe('squiggly'); + }); + + it('returns none when there is no style and it is not a note overlay', () => { + expect(decideAnnotationDraw('epubcfi(/6/4!/4)', undefined)).toBe('none'); + expect(decideAnnotationDraw(undefined, undefined)).toBe('none'); + }); +}); + +describe('findAnnotationAtCfi', () => { + it('finds the live annotation at the cfi', () => { + const notes = [makeNote({ id: 'a', cfi: 'X' }), makeNote({ id: 'b', cfi: 'Y' })]; + expect(findAnnotationAtCfi(notes, 'Y')).toBe(1); + }); + + it('ignores deleted annotations and non-annotation types', () => { + const notes = [ + makeNote({ id: 'a', cfi: 'X', deletedAt: 5 }), + makeNote({ id: 'b', cfi: 'X', type: 'bookmark' }), + ]; + expect(findAnnotationAtCfi(notes, 'X')).toBe(-1); + }); +}); + +describe('mergeRestyledAnnotation', () => { + it('keeps the existing id, note, text, createdAt, and global while taking the new style/color', () => { + const existing = makeNote({ + id: 'a', + style: 'highlight', + color: 'yellow', + note: 'hi', + text: 'word', + global: true, + createdAt: 100, + }); + const restyled = makeNote({ + id: 'tmp', + style: 'underline', + color: 'red', + note: '', + text: 'word', + createdAt: 200, + updatedAt: 200, + }); + const merged = mergeRestyledAnnotation(existing, restyled); + expect(merged.id).toBe('a'); + expect(merged.style).toBe('underline'); + expect(merged.color).toBe('red'); + expect(merged.note).toBe('hi'); + expect(merged.global).toBe(true); + expect(merged.createdAt).toBe(100); + expect(merged.updatedAt).toBe(200); + }); +}); diff --git a/apps/readest-app/src/__tests__/utils/booknoteMigration.test.ts b/apps/readest-app/src/__tests__/utils/booknoteMigration.test.ts new file mode 100644 index 00000000..2df0e3f7 --- /dev/null +++ b/apps/readest-app/src/__tests__/utils/booknoteMigration.test.ts @@ -0,0 +1,98 @@ +import { describe, expect, it } from 'vitest'; +import { unifyAnnotations } from '@/utils/booknoteMigration'; +import { BookNote } from '@/types/book'; + +const note = (over: Partial): BookNote => ({ + id: 'id', + type: 'annotation', + cfi: 'C', + note: '', + createdAt: 1, + updatedAt: 1, + ...over, +}); + +describe('unifyAnnotations', () => { + it('merges a separate highlight and note at the same cfi into a survivor + a tombstone', () => { + const input = [ + note({ + id: 'h', + cfi: 'C1', + style: 'highlight', + color: 'yellow', + text: 'hello', + note: '', + createdAt: 10, + }), + note({ id: 'n', cfi: 'C1', text: 'hello', note: 'my note', createdAt: 20 }), + ]; + const out = unifyAnnotations(input); + const survivor = out.find((n) => n.id === 'h')!; + const tombstone = out.find((n) => n.id === 'n')!; + expect(survivor.style).toBe('highlight'); + expect(survivor.note).toBe('my note'); + expect(survivor.deletedAt).toBeFalsy(); + expect(tombstone.deletedAt).toBeTruthy(); + }); + + it('deterministically prefers the styled record as survivor', () => { + const input = [ + note({ id: 'n', cfi: 'C', note: 'note', createdAt: 5 }), + note({ id: 'h', cfi: 'C', style: 'highlight', note: '', createdAt: 50 }), + ]; + const out = unifyAnnotations(input); + expect(out.find((n) => n.id === 'h')!.deletedAt).toBeFalsy(); + expect(out.find((n) => n.id === 'n')!.deletedAt).toBeTruthy(); + }); + + it('keeps the latest-updated non-empty note', () => { + const input = [ + note({ id: 'h', cfi: 'C', style: 'highlight', note: 'old', updatedAt: 10, createdAt: 1 }), + note({ id: 'n', cfi: 'C', note: 'new', updatedAt: 99, createdAt: 2 }), + ]; + const out = unifyAnnotations(input); + expect(out.find((n) => n.id === 'h')!.note).toBe('new'); + }); + + it('leaves a single annotation per cfi untouched (same reference)', () => { + const input = [note({ id: 'a', cfi: 'C', style: 'highlight', note: 'x' })]; + expect(unifyAnnotations(input)).toBe(input); + }); + + it('is idempotent', () => { + const input = [ + note({ id: 'h', cfi: 'C', style: 'highlight', note: '', createdAt: 10 }), + note({ id: 'n', cfi: 'C', note: 'note', createdAt: 20 }), + ]; + const once = unifyAnnotations(input); + const twice = unifyAnnotations(once); + expect(twice.filter((n) => !n.deletedAt).length).toBe(once.filter((n) => !n.deletedAt).length); + expect(twice.find((n) => n.id === 'h')!.note).toBe('note'); + }); + + it('does not merge bookmarks, excerpts, or global highlights', () => { + const input = [ + note({ id: 'b1', cfi: 'C', type: 'bookmark' }), + note({ id: 'b2', cfi: 'C', type: 'bookmark' }), + note({ id: 'e1', cfi: 'C', type: 'excerpt' }), + note({ id: 'e2', cfi: 'C', type: 'excerpt' }), + note({ id: 'g1', cfi: 'C2', style: 'highlight', global: true, note: '' }), + note({ id: 'g2', cfi: 'C2', style: 'highlight', global: true, note: '' }), + ]; + const out = unifyAnnotations(input); + expect(out.filter((n) => n.deletedAt).length).toBe(0); + }); + + it('collapses three records at one cfi to a single survivor + two tombstones', () => { + const input = [ + note({ id: 'h', cfi: 'C', style: 'highlight', note: '', createdAt: 10 }), + note({ id: 'n1', cfi: 'C', note: 'first', updatedAt: 20, createdAt: 11 }), + note({ id: 'n2', cfi: 'C', note: 'second', updatedAt: 30, createdAt: 12 }), + ]; + const out = unifyAnnotations(input); + expect(out.filter((n) => !n.deletedAt).length).toBe(1); + const survivor = out.find((n) => !n.deletedAt)!; + expect(survivor.id).toBe('h'); + expect(survivor.note).toBe('second'); + }); +}); diff --git a/apps/readest-app/src/__tests__/utils/serializer.test.ts b/apps/readest-app/src/__tests__/utils/serializer.test.ts index 7c53ca30..ca958664 100644 --- a/apps/readest-app/src/__tests__/utils/serializer.test.ts +++ b/apps/readest-app/src/__tests__/utils/serializer.test.ts @@ -70,4 +70,71 @@ describe('BookConfig serialization', () => { expect(config.viewSettings?.zoomLevel).toBe(90); expect(config.searchConfig?.query).toBe('rabbit'); }); + + it('migrates legacy split annotations on load when schemaVersion < 2', () => { + const config = deserializeConfig( + JSON.stringify({ + updatedAt: 1, + booknotes: [ + { + id: 'h', + type: 'annotation', + cfi: 'C1', + style: 'highlight', + color: 'yellow', + text: 't', + note: '', + createdAt: 10, + updatedAt: 10, + }, + { + id: 'n', + type: 'annotation', + cfi: 'C1', + text: 't', + note: 'hello', + createdAt: 20, + updatedAt: 20, + }, + ], + }), + globalViewSettings, + defaultSearchConfig, + ); + const survivor = config.booknotes!.find((n) => n.id === 'h')!; + const tombstone = config.booknotes!.find((n) => n.id === 'n')!; + expect(survivor.note).toBe('hello'); + expect(survivor.deletedAt).toBeFalsy(); + expect(tombstone.deletedAt).toBeTruthy(); + }); + + it('does not migrate annotations when schemaVersion is already 2', () => { + const config = deserializeConfig( + JSON.stringify({ + schemaVersion: 2, + booknotes: [ + { + id: 'h', + type: 'annotation', + cfi: 'C1', + style: 'highlight', + note: '', + createdAt: 10, + updatedAt: 10, + }, + { + id: 'n', + type: 'annotation', + cfi: 'C1', + note: 'hello', + createdAt: 20, + updatedAt: 20, + }, + ], + }), + globalViewSettings, + defaultSearchConfig, + ); + expect(config.booknotes!.every((n) => !n.deletedAt)).toBe(true); + }); }); 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 5ab8cd99..28824c05 100644 --- a/apps/readest-app/src/app/reader/components/annotator/Annotator.tsx +++ b/apps/readest-app/src/app/reader/components/annotator/Annotator.tsx @@ -56,7 +56,9 @@ import { TransformContext } from '@/services/transformers/types'; import { transformContent } from '@/services/transformService'; import { buildTTSSentenceHighlight, + decideAnnotationDraw, getHighlightColorHex, + mergeRestyledAnnotation, removeBookNoteOverlays, } from '../../utils/annotatorUtil'; import { buildAnnotationIndex, selectLocationAnnotations } from '../../utils/annotationIndex'; @@ -495,21 +497,27 @@ const Annotator: React.FC<{ bookKey: string; contentInsets: Insets }> = ({ const detail = (event as CustomEvent).detail; const { draw, annotation, doc, range } = detail; const { style, color } = annotation as BookNote; + const value = (annotation as BookNote & { value?: string }).value; const hexColor = getHighlightColorHex(settings, color); const einkBgColor = isDarkMode ? '#000000' : '#ffffff'; const einkFgColor = isDarkMode ? '#ffffff' : '#000000'; - if (annotation.note) { + // Choose what to draw from the overlay's `value` (cfi vs NOTE_PREFIX+cfi), + // not from `annotation.note`: a unified record (style + note) is added as + // two overlays and must draw a highlight for the cfi overlay AND a bubble + // for the note overlay. Keying off `note` drew only the bubble (#4511). + const kind = decideAnnotationDraw(value, style); + if (kind === 'bubble') { const { defaultView } = doc; const node = range.startContainer; const el = node.nodeType === 1 ? node : node.parentElement; const { writingMode } = defaultView.getComputedStyle(el); draw(Overlayer.bubble, { writingMode }); - } else if (style === 'highlight') { + } else if (kind === 'highlight') { draw(Overlayer.highlight, { color: isBwEink ? einkBgColor : hexColor, vertical: viewSettings.vertical, }); - } else if (['underline', 'squiggly'].includes(style as string)) { + } else if (kind === 'underline' || kind === 'squiggly') { const { defaultView } = doc; const node = range.startContainer; const el = node.nodeType === 1 ? node : node.parentElement; @@ -522,7 +530,7 @@ const Annotator: React.FC<{ bookKey: string; contentInsets: Insets }> = ({ const padding = viewSettings.vertical ? (lineHeightValue - fontSizeValue) / 2 - strokeWidth + verticalCompensation : (lineHeightValue - fontSizeValue) / 2 - strokeWidth + horizontalCompensation; - draw(Overlayer[style as keyof typeof Overlayer], { + draw(Overlayer[kind], { writingMode, color: isBwEink ? einkFgColor : hexColor, padding, @@ -1089,15 +1097,17 @@ const Annotator: React.FC<{ bookKey: string; contentInsets: Insets }> = ({ views.forEach((view) => removeGlobalAnnotationOverlays(view, existing)); } if (update) { - annotation.id = existing.id; - // Carry the existing `global` flag forward — toggling color/style - // shouldn't silently demote a global highlight back to single-range. - if (existing.global) annotation.global = true; - annotations[existingIndex] = annotation; - views.forEach((view) => view?.addAnnotation(annotation)); - if (annotation.global) { + // Preserve the note/text/createdAt and the `global` flag of the existing + // record so a restyle (color/style change) of a unified annotation + // doesn't wipe its note or silently demote a global highlight. The note + // bubble overlay (NOTE_PREFIX) isn't torn down above, so it persists; we + // only redraw the highlight overlay (value = cfi). + const merged = mergeRestyledAnnotation(existing, annotation); + annotations[existingIndex] = merged; + views.forEach((view) => view?.addAnnotation(merged)); + if (merged.global) { views.forEach((view) => { - if (view) expandAllRenderedSections(view, annotation); + if (view) expandAllRenderedSections(view, merged); }); } } else { 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 460ab4bd..3e643234 100644 --- a/apps/readest-app/src/app/reader/components/notebook/Notebook.tsx +++ b/apps/readest-app/src/app/reader/components/notebook/Notebook.tsx @@ -23,6 +23,7 @@ import { Overlay } from '@/components/Overlay'; import { saveSysSettings } from '@/helpers/settings'; import { NOTE_PREFIX } from '@/types/view'; import useShortcuts from '@/hooks/useShortcuts'; +import { findAnnotationAtCfi } from '../../utils/annotatorUtil'; import BooknoteItem from '../sidebar/BooknoteItem'; import AIAssistant from './AIAssistant'; import NotebookHeader from './Header'; @@ -154,18 +155,42 @@ const Notebook: React.FC = ({}) => { if (!cfi) return; const { booknotes: annotations = [] } = config; - const annotation: BookNote = { - id: uniqueId(), - type: 'annotation', - cfi, - note, - page: selection.page, - text: selection.text, - createdAt: Date.now(), - updatedAt: Date.now(), - }; - view?.addAnnotation({ ...annotation, value: `${NOTE_PREFIX}${annotation.cfi}` }); - annotations.push(annotation); + const existingIndex = findAnnotationAtCfi(annotations, cfi); + if (existingIndex !== -1) { + // Attach the note to the existing highlight at this CFI instead of + // creating a second record. The highlight overlay (value = cfi) already + // exists; add the note bubble overlay (value = NOTE_PREFIX+cfi). + const existing = annotations[existingIndex]!; + const updated: BookNote = { + ...existing, + note, + text: selection.text || existing.text, + updatedAt: Date.now(), + }; + annotations[existingIndex] = updated; + view?.addAnnotation({ ...updated, value: `${NOTE_PREFIX}${updated.cfi}` }); + } else { + // No highlight at this CFI yet (e.g. a note added without first + // highlighting): create one unified record with the current global style + // so the note still shows an underlying highlight, and draw both overlays. + const style = settings.globalReadSettings.highlightStyle; + const color = settings.globalReadSettings.highlightStyles[style]; + const annotation: BookNote = { + id: uniqueId(), + type: 'annotation', + cfi, + style, + color, + note, + page: selection.page, + text: selection.text, + createdAt: Date.now(), + updatedAt: Date.now(), + }; + view?.addAnnotation(annotation); + view?.addAnnotation({ ...annotation, value: `${NOTE_PREFIX}${annotation.cfi}` }); + annotations.push(annotation); + } const updatedConfig = updateBooknotes(sideBarBookKey, annotations); if (updatedConfig) { saveConfig(envConfig, sideBarBookKey, updatedConfig, settings); diff --git a/apps/readest-app/src/app/reader/components/sidebar/BooknoteItem.tsx b/apps/readest-app/src/app/reader/components/sidebar/BooknoteItem.tsx index 1f5c4ce9..ca5c2c3c 100644 --- a/apps/readest-app/src/app/reader/components/sidebar/BooknoteItem.tsx +++ b/apps/readest-app/src/app/reader/components/sidebar/BooknoteItem.tsx @@ -199,7 +199,7 @@ const BooknoteItem: React.FC = ({ bookKey, item, isNearest, o note.type === 'annotation' && note.cfi === cfi && !note.deletedAt, + ); +} + +/** + * Merge a freshly-built restyle (`restyled`, carrying the new style/color) onto + * an `existing` annotation, preserving the parts a restyle must not lose: the + * record id, its note text, the selected text, the original creation time, and + * the `global` flag. Without preserving `note`, recoloring a unified annotation + * would wipe the note. + */ +export function mergeRestyledAnnotation(existing: BookNote, restyled: BookNote): BookNote { + return { + ...restyled, + id: existing.id, + createdAt: existing.createdAt, + note: existing.note, + text: existing.text ?? restyled.text, + global: existing.global || restyled.global, + }; +} diff --git a/apps/readest-app/src/types/book.ts b/apps/readest-app/src/types/book.ts index fdb9e2d0..1d18f201 100644 --- a/apps/readest-app/src/types/book.ts +++ b/apps/readest-app/src/types/book.ts @@ -434,7 +434,7 @@ export interface BookSearchResult { progress?: number; } -export const BOOK_CONFIG_SCHEMA_VERSION = 1; +export const BOOK_CONFIG_SCHEMA_VERSION = 2; export interface BookConfig { schemaVersion?: number; diff --git a/apps/readest-app/src/utils/booknoteMigration.ts b/apps/readest-app/src/utils/booknoteMigration.ts new file mode 100644 index 00000000..043e2604 --- /dev/null +++ b/apps/readest-app/src/utils/booknoteMigration.ts @@ -0,0 +1,75 @@ +import { BookNote } from '@/types/book'; + +/** + * Schema v1 → v2 migration: collapse a highlight and its note that were stored + * as two separate `type:'annotation'` records at the same CFI into one unified + * record, tombstoning the redundant record(s) so the deletion propagates to the + * cloud and KOReader. + * + * The survivor is chosen deterministically (prefer a record with a `style`, then + * earliest `createdAt`, then smallest `id`) so independent devices converge under + * the server's last-writer-wins merge. The latest-updated non-empty note wins. + * + * Idempotent: a CFI that already has a single live record is left untouched, and + * a re-run after migration finds nothing to merge. Bookmarks, excerpts, and + * `global` highlights are never touched. Returns the same array reference when + * nothing changed. + */ +export function unifyAnnotations(booknotes: BookNote[]): BookNote[] { + const groups = new Map(); + for (const note of booknotes) { + if (note.type !== 'annotation') continue; + if (note.deletedAt) continue; + if (note.global) continue; + if (!note.cfi) continue; + const bucket = groups.get(note.cfi); + if (bucket) bucket.push(note); + else groups.set(note.cfi, [note]); + } + + const survivorById = new Map(); + const tombstonedIds = new Set(); + let changed = false; + const now = Date.now(); + + for (const group of groups.values()) { + if (group.length < 2) continue; + changed = true; + + const sorted = [...group].sort((a, b) => { + const aStyled = a.style ? 0 : 1; + const bStyled = b.style ? 0 : 1; + if (aStyled !== bStyled) return aStyled - bStyled; + const aCreated = a.createdAt ?? 0; + const bCreated = b.createdAt ?? 0; + if (aCreated !== bCreated) return aCreated - bCreated; + return a.id < b.id ? -1 : a.id > b.id ? 1 : 0; + }); + const survivor = sorted[0]!; + + let note = survivor.note ?? ''; + let noteUpdatedAt = note.trim().length > 0 ? (survivor.updatedAt ?? 0) : -1; + for (const member of group) { + const memberNote = member.note ?? ''; + if (memberNote.trim().length === 0) continue; + const memberUpdatedAt = member.updatedAt ?? 0; + if (memberUpdatedAt > noteUpdatedAt) { + note = memberNote; + noteUpdatedAt = memberUpdatedAt; + } + } + + survivorById.set(survivor.id, { ...survivor, note, updatedAt: now }); + for (const member of group) { + if (member.id !== survivor.id) tombstonedIds.add(member.id); + } + } + + if (!changed) return booknotes; + + return booknotes.map((note) => { + if (survivorById.has(note.id)) return survivorById.get(note.id)!; + if (tombstonedIds.has(note.id)) return { ...note, deletedAt: now, updatedAt: now }; + return note; + }); +} diff --git a/apps/readest-app/src/utils/serializer.ts b/apps/readest-app/src/utils/serializer.ts index 49586a0e..615f0a24 100644 --- a/apps/readest-app/src/utils/serializer.ts +++ b/apps/readest-app/src/utils/serializer.ts @@ -4,6 +4,7 @@ import { BookSearchConfig, ViewSettings, } from '@/types/book'; +import { unifyAnnotations } from '@/utils/booknoteMigration'; export const stampBookConfigSchema = >(config: T): T => { return { ...config, schemaVersion: BOOK_CONFIG_SCHEMA_VERSION }; @@ -64,6 +65,11 @@ export const deserializeConfig = ( const { viewSettings, searchConfig } = config; config.viewSettings = { ...globalViewSettings, ...viewSettings }; config.searchConfig = { ...defaultSearchConfig, ...searchConfig }; + // v1 -> v2: collapse split highlight+note records into one unified record so a + // note renders with its highlight and round-trips cleanly to KOReader. + if ((config.schemaVersion ?? 0) < 2 && config.booknotes?.length) { + config.booknotes = unifyAnnotations(config.booknotes); + } config.schemaVersion ??= BOOK_CONFIG_SCHEMA_VERSION; config.updatedAt ??= Date.now(); return config;