Clicking "Annotate" on a selection eagerly creates a highlight (with an empty note) as the anchor for the note being typed, so the selection stays visible while the NoteEditor is open. Cancelling the note instead of saving left that empty highlight behind: it leaked into the config DB, showed as a stale card in the Booknotes list, and left a phantom yellow highlight. handleHighlight now returns the created BookNote only when it pushes a new record (null when it restyles an existing highlight, which predates the flow and must survive a cancel). handleAnnotate tracks that id via the new notebookNewHighlightId store field; cleanup is keyed on the id, not the cfi, so a fresh selection that collides with an existing highlight's cfi can't wrongly delete it. removeEmptyAnnotationPlaceholder tombstones the tracked placeholder only when it still has no note text, and the Notebook tears its overlay down. Cleanup is presentation-driven: an effect removes the placeholder whenever the creation editor stops being shown (Cancel, Escape, overlay, close, swipe, navigate), plus a second effect for book-switch and reader-close. Save survives the guard and clears the tracked id. Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -10,6 +10,7 @@ beforeEach(() => {
|
||||
isNotebookPinned: false,
|
||||
notebookActiveTab: 'notes',
|
||||
notebookNewAnnotation: null,
|
||||
notebookNewHighlightId: null,
|
||||
notebookEditAnnotation: null,
|
||||
notebookAnnotationDrafts: {},
|
||||
});
|
||||
@@ -137,6 +138,20 @@ describe('notebookStore', () => {
|
||||
});
|
||||
});
|
||||
|
||||
// ── New highlight placeholder id ───────────────────────────────
|
||||
describe('setNotebookNewHighlightId', () => {
|
||||
test('tracks the placeholder highlight id', () => {
|
||||
useNotebookStore.getState().setNotebookNewHighlightId('hl-1');
|
||||
expect(useNotebookStore.getState().notebookNewHighlightId).toBe('hl-1');
|
||||
});
|
||||
|
||||
test('clears the placeholder highlight id when set to null', () => {
|
||||
useNotebookStore.getState().setNotebookNewHighlightId('hl-1');
|
||||
useNotebookStore.getState().setNotebookNewHighlightId(null);
|
||||
expect(useNotebookStore.getState().notebookNewHighlightId).toBeNull();
|
||||
});
|
||||
});
|
||||
|
||||
// ── Edit annotation ────────────────────────────────────────────
|
||||
describe('setNotebookEditAnnotation', () => {
|
||||
test('sets a note for editing', () => {
|
||||
@@ -216,6 +231,7 @@ describe('notebookStore', () => {
|
||||
expect(state.isNotebookPinned).toBe(false);
|
||||
expect(state.notebookActiveTab).toBe('notes');
|
||||
expect(state.notebookNewAnnotation).toBeNull();
|
||||
expect(state.notebookNewHighlightId).toBeNull();
|
||||
expect(state.notebookEditAnnotation).toBeNull();
|
||||
expect(state.notebookAnnotationDrafts).toEqual({});
|
||||
});
|
||||
|
||||
@@ -4,6 +4,7 @@ import {
|
||||
getExternalDragHandle,
|
||||
getHighlightColorLabel,
|
||||
removeBookNoteOverlays,
|
||||
removeEmptyAnnotationPlaceholder,
|
||||
toParentViewportPoint,
|
||||
} from '@/app/reader/utils/annotatorUtil';
|
||||
import { Point } from '@/utils/sel';
|
||||
@@ -227,6 +228,78 @@ describe('removeBookNoteOverlays', () => {
|
||||
});
|
||||
});
|
||||
|
||||
describe('removeEmptyAnnotationPlaceholder', () => {
|
||||
const baseNote = (overrides: Partial<BookNote> = {}): BookNote => ({
|
||||
id: 'ph-1',
|
||||
type: 'annotation',
|
||||
cfi: 'epubcfi(/6/4!/4/2)',
|
||||
style: 'highlight',
|
||||
color: 'yellow',
|
||||
text: 'selected text',
|
||||
note: '',
|
||||
createdAt: 1,
|
||||
updatedAt: 1,
|
||||
...overrides,
|
||||
});
|
||||
|
||||
it('tombstones the empty placeholder by id and returns it', () => {
|
||||
const placeholder = baseNote();
|
||||
const booknotes = [placeholder];
|
||||
|
||||
const removed = removeEmptyAnnotationPlaceholder(booknotes, 'ph-1', 1234);
|
||||
|
||||
expect(removed).toBe(placeholder);
|
||||
expect(booknotes[0]!.deletedAt).toBe(1234);
|
||||
});
|
||||
|
||||
it('returns null and leaves booknotes untouched when the record carries note text', () => {
|
||||
const saved = baseNote({ note: 'a real note' });
|
||||
const booknotes = [saved];
|
||||
|
||||
const removed = removeEmptyAnnotationPlaceholder(booknotes, 'ph-1', 1234);
|
||||
|
||||
expect(removed).toBeNull();
|
||||
expect(booknotes[0]!.deletedAt).toBeUndefined();
|
||||
});
|
||||
|
||||
it('treats whitespace-only note text as empty and tombstones it', () => {
|
||||
const placeholder = baseNote({ note: ' \n ' });
|
||||
const booknotes = [placeholder];
|
||||
|
||||
const removed = removeEmptyAnnotationPlaceholder(booknotes, 'ph-1', 1234);
|
||||
|
||||
expect(removed).toBe(placeholder);
|
||||
expect(booknotes[0]!.deletedAt).toBe(1234);
|
||||
});
|
||||
|
||||
it('returns null when no record matches the id', () => {
|
||||
const booknotes = [baseNote({ id: 'other' })];
|
||||
|
||||
const removed = removeEmptyAnnotationPlaceholder(booknotes, 'ph-1', 1234);
|
||||
|
||||
expect(removed).toBeNull();
|
||||
expect(booknotes[0]!.deletedAt).toBeUndefined();
|
||||
});
|
||||
|
||||
it('returns null when the matching record is already soft-deleted', () => {
|
||||
const booknotes = [baseNote({ deletedAt: 5 })];
|
||||
|
||||
const removed = removeEmptyAnnotationPlaceholder(booknotes, 'ph-1', 1234);
|
||||
|
||||
expect(removed).toBeNull();
|
||||
});
|
||||
|
||||
it('ignores a non-annotation record with the same id', () => {
|
||||
const bookmark = baseNote({ type: 'bookmark', style: undefined });
|
||||
const booknotes = [bookmark];
|
||||
|
||||
const removed = removeEmptyAnnotationPlaceholder(booknotes, 'ph-1', 1234);
|
||||
|
||||
expect(removed).toBeNull();
|
||||
expect(booknotes[0]!.deletedAt).toBeUndefined();
|
||||
});
|
||||
});
|
||||
|
||||
describe('buildTTSSentenceHighlight', () => {
|
||||
const params = {
|
||||
cfi: 'epubcfi(/6/4!/4/10,/1:0,/1:42)',
|
||||
|
||||
@@ -109,7 +109,8 @@ const Annotator: React.FC<{ bookKey: string; contentInsets: Insets }> = ({
|
||||
const getView = useReaderStore((s) => s.getView);
|
||||
const getViewsById = useReaderStore((s) => s.getViewsById);
|
||||
const getViewSettings = useReaderStore((s) => s.getViewSettings);
|
||||
const { setNotebookVisible, setNotebookNewAnnotation } = useNotebookStore();
|
||||
const { setNotebookVisible, setNotebookNewAnnotation, setNotebookNewHighlightId } =
|
||||
useNotebookStore();
|
||||
const { clearBooknotesNav } = useSidebarStore();
|
||||
const { listenToNativeTouchEvents } = useDeviceControlStore();
|
||||
const { loadCustomDictionaries } = useCustomDictionaryStore();
|
||||
@@ -1081,12 +1082,12 @@ const Annotator: React.FC<{ bookKey: string; contentInsets: Insets }> = ({
|
||||
handleDismissPopupAndSelection();
|
||||
};
|
||||
|
||||
const handleHighlight = (update = false, highlightStyle?: HighlightStyle) => {
|
||||
if (!selection || !selection.text) return;
|
||||
const handleHighlight = (update = false, highlightStyle?: HighlightStyle): BookNote | null => {
|
||||
if (!selection || !selection.text) return null;
|
||||
setHighlightOptionsVisible(true);
|
||||
const { booknotes: annotations = [] } = config;
|
||||
const cfi = view?.getCFI(selection.index, selection.range);
|
||||
if (!cfi) return;
|
||||
if (!cfi) return null;
|
||||
const style = highlightStyle || settings.globalReadSettings.highlightStyle;
|
||||
const color = settings.globalReadSettings.highlightStyles[style];
|
||||
setSelectedStyle(style);
|
||||
@@ -1111,6 +1112,9 @@ const Annotator: React.FC<{ bookKey: string; contentInsets: Insets }> = ({
|
||||
!annotation.deletedAt,
|
||||
);
|
||||
const views = getViewsById(bookKey.split('-')[0]!);
|
||||
// Only a brand-new highlight is a placeholder the cancel flow may remove;
|
||||
// restyling/toggling an existing one must never tear down the user's record.
|
||||
let created: BookNote | null = null;
|
||||
if (existingIndex !== -1) {
|
||||
const existing = annotations[existingIndex]!;
|
||||
// Tear down both the original anchor and any global fan-outs that
|
||||
@@ -1142,12 +1146,14 @@ const Annotator: React.FC<{ bookKey: string; contentInsets: Insets }> = ({
|
||||
annotations.push(annotation);
|
||||
views.forEach((view) => view?.addAnnotation(annotation));
|
||||
setSelection({ ...selection, cfi, annotated: true });
|
||||
created = annotation;
|
||||
}
|
||||
|
||||
const updatedConfig = updateBooknotes(bookKey, annotations);
|
||||
if (updatedConfig) {
|
||||
saveConfig(envConfig, bookKey, updatedConfig, settings);
|
||||
}
|
||||
return created;
|
||||
};
|
||||
|
||||
const handleCreateTTSHighlight = (event: CustomEvent) => {
|
||||
@@ -1214,9 +1220,13 @@ const Annotator: React.FC<{ bookKey: string; contentInsets: Insets }> = ({
|
||||
if (!selection || !selection.text) return;
|
||||
const { sectionHref: href } = progress;
|
||||
selection.href = href;
|
||||
handleHighlight(true);
|
||||
const created = handleHighlight(true);
|
||||
setNotebookVisible(true);
|
||||
setNotebookNewAnnotation(selection);
|
||||
// Remember the eagerly-created highlight so the notebook can remove it if the
|
||||
// note is never saved. A restyle of an existing highlight returns null — that
|
||||
// record predates this flow and must survive a cancel (#4791).
|
||||
setNotebookNewHighlightId(created?.id ?? null);
|
||||
handleDismissPopup();
|
||||
};
|
||||
|
||||
|
||||
@@ -79,6 +79,8 @@ const NoteEditor: React.FC<NoteEditorProps> = ({ onSave, onEdit }) => {
|
||||
|
||||
const handleEscape = () => {
|
||||
if (notebookNewAnnotation) {
|
||||
// Clearing the selection ends the creation flow; Notebook reacts to that
|
||||
// and tears down the empty placeholder highlight it created (#4791).
|
||||
setNotebookNewAnnotation(null);
|
||||
}
|
||||
if (notebookEditAnnotation) {
|
||||
|
||||
@@ -23,7 +23,11 @@ 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 {
|
||||
findAnnotationAtCfi,
|
||||
removeBookNoteOverlays,
|
||||
removeEmptyAnnotationPlaceholder,
|
||||
} from '../../utils/annotatorUtil';
|
||||
import BooknoteItem from '../sidebar/BooknoteItem';
|
||||
import AIAssistant from './AIAssistant';
|
||||
import NotebookHeader from './Header';
|
||||
@@ -45,11 +49,11 @@ const Notebook: React.FC = ({}) => {
|
||||
useNotebookStore();
|
||||
const { notebookNewAnnotation, notebookEditAnnotation, setNotebookPin } = useNotebookStore();
|
||||
const { getBookData, getConfig, saveConfig, updateBooknotes } = useBookDataStore();
|
||||
const { getView, getProgress, getViewSettings } = useReaderStore();
|
||||
const { getView, getViewsById, getProgress, getViewSettings } = useReaderStore();
|
||||
const { getNotebookWidth, setNotebookWidth, setNotebookVisible, toggleNotebookPin } =
|
||||
useNotebookStore();
|
||||
const { setNotebookNewAnnotation, setNotebookEditAnnotation, setNotebookActiveTab } =
|
||||
useNotebookStore();
|
||||
const { setNotebookNewAnnotation, setNotebookNewHighlightId } = useNotebookStore();
|
||||
const { setNotebookEditAnnotation, setNotebookActiveTab } = useNotebookStore();
|
||||
const { activeConversationId } = useAIChatStore();
|
||||
|
||||
const [isSearchBarVisible, setIsSearchBarVisible] = useState(false);
|
||||
@@ -140,6 +144,59 @@ const Notebook: React.FC = ({}) => {
|
||||
saveSysSettings(envConfig, 'globalReadSettings', newGlobalReadSettings);
|
||||
};
|
||||
|
||||
// Abandon a note-creation flow: tear down the empty highlight the "Annotate"
|
||||
// action eagerly created as the note anchor so it doesn't leak into the
|
||||
// booknotes list (#4791). A saved note carries text, so it survives the guard
|
||||
// in removeEmptyAnnotationPlaceholder; a restyled pre-existing highlight has no
|
||||
// tracked id and is left alone. `bookKey` is passed explicitly so the unmount/
|
||||
// book-switch cleanup targets the book the placeholder belongs to.
|
||||
const handleCancelNewAnnotation = useCallback(
|
||||
(bookKey: string | null) => {
|
||||
const { notebookNewHighlightId } = useNotebookStore.getState();
|
||||
if (bookKey && notebookNewHighlightId) {
|
||||
const config = getConfig(bookKey);
|
||||
const { booknotes: annotations = [] } = config || {};
|
||||
const placeholder = removeEmptyAnnotationPlaceholder(
|
||||
annotations,
|
||||
notebookNewHighlightId,
|
||||
Date.now(),
|
||||
);
|
||||
if (placeholder) {
|
||||
const views = getViewsById(bookKey.split('-')[0]!);
|
||||
views.forEach((view) => removeBookNoteOverlays(view, placeholder));
|
||||
const updatedConfig = updateBooknotes(bookKey, annotations);
|
||||
if (updatedConfig) {
|
||||
// Read settings fresh: this callback has stable identity (empty deps)
|
||||
// so a captured `settings` would go stale across saves.
|
||||
saveConfig(envConfig, bookKey, updatedConfig, useSettingsStore.getState().settings);
|
||||
}
|
||||
}
|
||||
}
|
||||
setNotebookNewHighlightId(null);
|
||||
setNotebookNewAnnotation(null);
|
||||
},
|
||||
// eslint-disable-next-line react-hooks/exhaustive-deps
|
||||
[],
|
||||
);
|
||||
|
||||
// The "Annotate" action keeps a placeholder highlight alive only while its
|
||||
// editor is on screen. The moment that creation flow stops being presented —
|
||||
// Cancel/Escape (selection cleared), the notebook closing, swipe-dismiss, or a
|
||||
// navigate — clean the placeholder up (#4791). Save clears the tracked id (and
|
||||
// the placeholder gains note text), so this no-ops for saved annotations.
|
||||
useEffect(() => {
|
||||
if (!(isNotebookVisible && notebookNewAnnotation)) {
|
||||
handleCancelNewAnnotation(sideBarBookKey);
|
||||
}
|
||||
}, [isNotebookVisible, notebookNewAnnotation, sideBarBookKey, handleCancelNewAnnotation]);
|
||||
|
||||
// Switching books (notebook pinned, so it stays presented) or closing the
|
||||
// reader leaves the placeholder behind; clean it up against the book we are
|
||||
// leaving on the way out (#4791).
|
||||
useEffect(() => {
|
||||
return () => handleCancelNewAnnotation(sideBarBookKey);
|
||||
}, [sideBarBookKey, handleCancelNewAnnotation]);
|
||||
|
||||
const handleClickOverlay = () => {
|
||||
setNotebookVisible(false);
|
||||
setNotebookNewAnnotation(null);
|
||||
@@ -196,6 +253,9 @@ const Notebook: React.FC = ({}) => {
|
||||
saveConfig(envConfig, sideBarBookKey, updatedConfig, settings);
|
||||
}
|
||||
setNotebookNewAnnotation(null);
|
||||
// The placeholder now carries a note (or a fresh unified record was created),
|
||||
// so it's a real annotation — drop the cancel-cleanup handle (#4791).
|
||||
setNotebookNewHighlightId(null);
|
||||
};
|
||||
|
||||
const handleEditNote = (note: BookNote, isDelete: boolean) => {
|
||||
|
||||
@@ -195,6 +195,36 @@ export function removeBookNoteOverlays(view: FoliateView | null, note: BookNote)
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* The "Annotate" action eagerly creates an empty highlight as the anchor for the
|
||||
* note the user is about to type, so the selection stays visible while the editor
|
||||
* is open. If the user cancels without saving, that placeholder must be torn down
|
||||
* so it doesn't leak into the booknotes list (#4791).
|
||||
*
|
||||
* Tombstones the live annotation identified by `placeholderId` in `booknotes`
|
||||
* (mutating in place, matching the surrounding highlight handlers) and returns it
|
||||
* so the caller can remove its overlay. Returns null — leaving `booknotes`
|
||||
* untouched — when there's nothing to clean up: no live annotation with that id,
|
||||
* or the record already carries note text (the user saved, so it's real now).
|
||||
*/
|
||||
export function removeEmptyAnnotationPlaceholder(
|
||||
booknotes: BookNote[],
|
||||
placeholderId: string,
|
||||
now: number,
|
||||
): BookNote | null {
|
||||
const index = booknotes.findIndex(
|
||||
(note) =>
|
||||
note.id === placeholderId &&
|
||||
note.type === 'annotation' &&
|
||||
!note.deletedAt &&
|
||||
!note.note?.trim(),
|
||||
);
|
||||
if (index === -1) return null;
|
||||
const placeholder = booknotes[index]!;
|
||||
booknotes[index] = { ...placeholder, deletedAt: now };
|
||||
return placeholder;
|
||||
}
|
||||
|
||||
/**
|
||||
* Build a persistent highlight BookNote for a TTS-spoken sentence, or return
|
||||
* `null` when one already exists at the same CFI (idempotent — pressing the
|
||||
|
||||
@@ -10,6 +10,10 @@ interface NotebookState {
|
||||
isNotebookPinned: boolean;
|
||||
notebookActiveTab: NotebookTab;
|
||||
notebookNewAnnotation: TextSelection | null;
|
||||
// Id of the highlight eagerly created by the "Annotate" action as the anchor
|
||||
// for a note in progress. Tracked so a cancelled creation flow can tear that
|
||||
// empty placeholder back down instead of leaking it (#4791).
|
||||
notebookNewHighlightId: string | null;
|
||||
notebookEditAnnotation: BookNote | null;
|
||||
notebookAnnotationDrafts: { [key: string]: string };
|
||||
getIsNotebookVisible: () => boolean;
|
||||
@@ -21,6 +25,7 @@ interface NotebookState {
|
||||
setNotebookPin: (pinned: boolean) => void;
|
||||
setNotebookActiveTab: (tab: NotebookTab) => void;
|
||||
setNotebookNewAnnotation: (selection: TextSelection | null) => void;
|
||||
setNotebookNewHighlightId: (id: string | null) => void;
|
||||
setNotebookEditAnnotation: (note: BookNote | null) => void;
|
||||
saveNotebookAnnotationDraft: (key: string, note: string) => void;
|
||||
getNotebookAnnotationDraft: (key: string) => string | undefined;
|
||||
@@ -32,6 +37,7 @@ export const useNotebookStore = create<NotebookState>((set, get) => ({
|
||||
isNotebookPinned: false,
|
||||
notebookActiveTab: 'notes',
|
||||
notebookNewAnnotation: null,
|
||||
notebookNewHighlightId: null,
|
||||
notebookEditAnnotation: null,
|
||||
notebookAnnotationDrafts: {},
|
||||
getIsNotebookVisible: () => get().isNotebookVisible,
|
||||
@@ -44,6 +50,7 @@ export const useNotebookStore = create<NotebookState>((set, get) => ({
|
||||
setNotebookActiveTab: (tab: NotebookTab) => set({ notebookActiveTab: tab }),
|
||||
setNotebookNewAnnotation: (selection: TextSelection | null) =>
|
||||
set({ notebookNewAnnotation: selection }),
|
||||
setNotebookNewHighlightId: (id: string | null) => set({ notebookNewHighlightId: id }),
|
||||
setNotebookEditAnnotation: (note: BookNote | null) => set({ notebookEditAnnotation: note }),
|
||||
saveNotebookAnnotationDraft: (key: string, note: string) =>
|
||||
set((state) => ({
|
||||
|
||||
Reference in New Issue
Block a user