-
Notifications
You must be signed in to change notification settings - Fork 4
[TAS-5149] ✨ Implement annotation feature for EPUB reader #643
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Implements an annotation (highlights + notes) feature for the EPUB reader, including persistence via Firestore-backed APIs and new UI components for creating/editing/navigating annotations.
Changes:
- Added shared/server annotation types and Firestore utility functions for CRUD operations.
- Introduced
/api/books/:nftClassId/annotationsendpoints (list/create/update/delete). - Updated the EPUB reader UI to render highlights, show a selection menu + modal editor, and display an annotations list; added i18n strings and color constants.
Reviewed changes
Copilot reviewed 13 out of 15 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| shared/types/annotation.d.ts | Adds shared annotation type definitions (color, base fields, create/update shapes). |
| server/types/annotation.d.ts | Defines Firestore data shape for annotations. |
| server/utils/annotations.ts | Firestore CRUD utilities for annotations under user/book documents. |
| server/api/books/[nftClassId]/annotations/index.get.ts | Lists annotations for a user/book. |
| server/api/books/[nftClassId]/annotations/index.post.ts | Creates an annotation with basic validation. |
| server/api/books/[nftClassId]/annotations/[annotationId].post.ts | Updates annotation color/note. |
| server/api/books/[nftClassId]/annotations/[annotationId].delete.ts | Deletes an annotation. |
| constants/annotations.ts | Centralizes annotation color options and rgba mappings. |
| composables/use-annotations.ts | Client composable for fetching and mutating annotations via the new APIs. |
| components/AnnotationsList.vue | UI list for existing highlights/notes and navigation to a highlight. |
| components/AnnotationMenu.vue | Context menu for selection-based highlight color + “note” action. |
| components/AnnotationModal.vue | Modal UI for editing color and note, deleting highlights. |
| pages/reader/epub.vue | Integrates selection handling, highlight rendering, and annotation UI into the EPUB reader. |
| i18n/locales/en.json | Adds English strings for annotations UI. |
| i18n/locales/zh-Hant.json | Adds Traditional Chinese strings for annotations UI. |
| try { | ||
| const annotation = await createAnnotation(walletAddress, nftClassId, body.id, { | ||
| cfi: body.cfi, | ||
| text: body.text, | ||
| color: body.color, | ||
| note: body.note, | ||
| chapterTitle: body.chapterTitle, | ||
| }) |
Copilot
AI
Jan 27, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note and chapterTitle are accepted from the request body without type validation. A client could send a non-string value that then gets persisted (or causes downstream UI/type issues). Validate these optional fields when present (string or undefined) before calling createAnnotation.
| }>() | ||
|
|
||
| const emit = defineEmits<{ | ||
| (e: 'navigate' | 'edit', annotation: Annotation): void |
Copilot
AI
Jan 27, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
defineEmits declares an 'edit' event but the component never emits it. Removing unused emits helps keep the component API accurate and avoids confusion for consumers.
| (e: 'navigate' | 'edit', annotation: Annotation): void | |
| (e: 'navigate', annotation: Annotation): void |
| await updateAnnotation(editingAnnotation.value.id, { | ||
| color: data.color, | ||
| note: data.note, | ||
| }) |
Copilot
AI
Jan 27, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
handleAnnotationModalSave closes the modal and re-renders the highlight even if updateAnnotation(...) fails (the composable returns null on failure). This can desync UI state from the server. Check the return value and only update the highlight/close the modal on success; otherwise keep the modal open and surface an error/toast.
| await updateAnnotation(editingAnnotation.value.id, { | |
| color: data.color, | |
| note: data.note, | |
| }) | |
| const updatedAnnotation = await updateAnnotation(editingAnnotation.value.id, { | |
| color: data.color, | |
| note: data.note, | |
| }) | |
| if (!updatedAnnotation) { | |
| // Keep modal open on failure and surface an error | |
| try { | |
| const toast = useToast?.() | |
| toast?.add?.({ | |
| title: 'Failed to save annotation', | |
| description: 'Please check your connection and try again.', | |
| color: 'red', | |
| }) | |
| } | |
| catch { | |
| // Ignore toast errors | |
| } | |
| return | |
| } |
| if (editingAnnotation.value) { | ||
| const cfi = editingAnnotation.value.cfi | ||
| await deleteAnnotation(editingAnnotation.value.id) | ||
| try { | ||
| rendition.value?.annotations.remove(cfi, 'highlight') | ||
| } | ||
| catch { | ||
| // Ignore error if highlight doesn't exist | ||
| } | ||
| } |
Copilot
AI
Jan 27, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
handleAnnotationModalDelete removes the highlight and closes the modal regardless of whether deleteAnnotation(...) succeeds (the composable returns false on failure). This can leave the annotation existing on the server but missing in the UI. Use the boolean result to decide whether to remove the highlight/close, and show an error if deletion fails.
| if (editingAnnotation.value) { | |
| const cfi = editingAnnotation.value.cfi | |
| await deleteAnnotation(editingAnnotation.value.id) | |
| try { | |
| rendition.value?.annotations.remove(cfi, 'highlight') | |
| } | |
| catch { | |
| // Ignore error if highlight doesn't exist | |
| } | |
| } | |
| if (!editingAnnotation.value) { | |
| return | |
| } | |
| const cfi = editingAnnotation.value.cfi | |
| let isDeleted = false | |
| try { | |
| isDeleted = await deleteAnnotation(editingAnnotation.value.id) | |
| } | |
| catch { | |
| isDeleted = false | |
| } | |
| if (!isDeleted) { | |
| // Show an error and keep the modal open if deletion fails | |
| window.alert('Failed to delete annotation. Please try again.') | |
| return | |
| } | |
| try { | |
| rendition.value?.annotations.remove(cfi, 'highlight') | |
| } | |
| catch { | |
| // Ignore error if highlight doesn't exist | |
| } |
| await getAnnotationsCollection(userWallet, nftClassId) | ||
| .doc(annotationId) | ||
| .set(annotationData) | ||
|
|
||
| const createdDoc = await getAnnotationsCollection(userWallet, nftClassId) | ||
| .doc(annotationId) | ||
| .get() |
Copilot
AI
Jan 27, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
createAnnotation uses .set(annotationData) which will overwrite an existing annotation if the client reuses an annotationId. If overwriting isn’t intended, use .create(...) (and map Firestore already-exists to 409) or explicitly check doc.exists and reject duplicates.
| await getAnnotationsCollection(userWallet, nftClassId) | |
| .doc(annotationId) | |
| .set(annotationData) | |
| const createdDoc = await getAnnotationsCollection(userWallet, nftClassId) | |
| .doc(annotationId) | |
| .get() | |
| const docRef = getAnnotationsCollection(userWallet, nftClassId).doc(annotationId) | |
| await docRef.create(annotationData) | |
| const createdDoc = await docRef.get() |
| let body: AnnotationUpdateData | ||
| try { | ||
| body = await readBody(event) | ||
| } | ||
| catch (error) { | ||
| console.error(error) | ||
| throw createError({ | ||
| statusCode: 400, | ||
| message: 'INVALID_BODY', | ||
| }) | ||
| } | ||
|
|
||
| if (!body) { | ||
| throw createError({ | ||
| statusCode: 400, | ||
| message: 'MISSING_BODY', | ||
| }) | ||
| } | ||
|
|
||
| if (body.color !== undefined && !ANNOTATION_COLORS.includes(body.color)) { | ||
| throw createError({ | ||
| statusCode: 400, | ||
| message: 'INVALID_COLOR', | ||
| }) | ||
| } |
Copilot
AI
Jan 27, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
readBody can return non-object values (string/array). With the current checks, a non-object body can slip through (e.g. a string), resulting in an update that only bumps updatedAt. Add a guard that the body is a plain object (and optionally require at least one of color/note to be present) before calling updateAnnotation.
components/AnnotationModal.vue
Outdated
| isSaving.value = true | ||
| try { | ||
| emit('save', { | ||
| color: selectedColor.value, | ||
| note: note.value, | ||
| }) | ||
| } | ||
| finally { | ||
| isSaving.value = false | ||
| } |
Copilot
AI
Jan 27, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isSaving is set back to false immediately after emitting save, but the parent’s save handler performs async work. This makes the loading state inaccurate and allows repeated clicks while the request is still in flight. Keep isSaving true until the modal closes (similar to isDeleting) or change the event contract so the modal can await completion.
| isSaving.value = true | |
| try { | |
| emit('save', { | |
| color: selectedColor.value, | |
| note: note.value, | |
| }) | |
| } | |
| finally { | |
| isSaving.value = false | |
| } | |
| // Prevent multiple simultaneous save actions | |
| if (isSaving.value) { | |
| return | |
| } | |
| isSaving.value = true | |
| emit('save', { | |
| color: selectedColor.value, | |
| note: note.value, | |
| }) | |
| // Keep loading state until modal is closed (see watcher on `open`) |
| useEventListener(view.window, 'touchend', (event: TouchEvent) => { | ||
| setTimeout(() => { | ||
| const touch = event.changedTouches[0] | ||
| if (touch) { | ||
| const mouseEvent = { clientX: touch.clientX, clientY: touch.clientY } as MouseEvent | ||
| handleTextSelection(mouseEvent, view.window) | ||
| } | ||
| }, 300) | ||
| }) |
Copilot
AI
Jan 27, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The touchend listener is registered inside the rendition.on('rendered') callback but its cleanup function isn’t stored/removed. Since rendered fires repeatedly, this will accumulate listeners and can cause multiple handleTextSelection executions per gesture. Track the stop function (like removeMouseUpListener) and dispose it before adding a new listener (or register it once outside the rendered handler).
2755eb5 to
1baa810
Compare
Uh oh!
There was an error while loading. Please reload this page.