Conversation
- 劃線/顏色/感想筆記/清單管理/匯出（Share.share）第一版實作 - 新增「劃線模式」切換，暫停翻頁點擊區以避免攔截長按選字手勢 - 補上網頁版已有的 annotations hooks.render 競態保險機制（verify+reinject） Co-Authored-By: Claude Sonnet 5 <[email protected]>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Review limit reached
Next review available in: 5 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThis PR adds a full annotation (highlight/notes) feature spanning data persistence (AsyncStorage-backed Annotation records), messaging protocol extensions between RN and the epub.js WebView, underline rendering with verification/reinjection logic, RN reader screen state/handlers, a new ListPanel notes tab with export, a new SelectionBar component, shared highlight colors, and setup-guide progress notes. ChangesAnnotation Feature
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant ReaderScreen
participant WebView
participant AsyncStorage
User->>WebView: select text in book
WebView->>ReaderScreen: textSelected(cfi, text)
ReaderScreen->>ReaderScreen: show SelectionBar
User->>ReaderScreen: choose highlight color
ReaderScreen->>WebView: setAnnotations(annotations)
WebView->>WebView: applyAnnotations / verifyAnnotationsRendered
ReaderScreen->>AsyncStorage: saveAnnotations(id, annotations)
ReaderScreen->>ReaderScreen: syncAnnotationsToWebView / update ListPanel
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
mobile/lib/library.ts (1)
140-153: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winAwait the storage cleanup, or handle its rejection.
updateMetaonly accepts a synchronous callback, soAsyncStorage.multiRemove(...)is fire-and-forget here. If it rejects, the rejection is unhandled andannotationsKey(id)can survive a successful delete. Move the cleanup outsideupdateMetaso it can be awaited, or attach a.catch()here.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@mobile/lib/library.ts` around lines 140 - 153, The storage cleanup in removeBook is currently fire-and-forget inside the updateMeta callback, so any AsyncStorage.multiRemove rejection is unhandled and some keys may remain. Move the multiRemove call out of updateMeta so removeBook can await it, or add explicit rejection handling with catch; keep the file/cover deletion and record removal logic in removeBook/updateMeta, and use the existing progressKey, settingsKey, bookmarksKey, and annotationsKey helpers to locate the cleanup call.
🧹 Nitpick comments (3)
mobile/components/SelectionBar.tsx (1)
31-42: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftMove new JSX styling out of inline
styleprops.This new component adds several inline style objects; please convert these to the project’s
classNamestyling approach. As per coding guidelines,**/*.tsx: React JSX 頁面/元件中應使用className寫法，並避免使用 inline style.Also applies to: 45-45, 49-49, 56-56, 66-68, 74-76, 84-86
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@mobile/components/SelectionBar.tsx` around lines 31 - 42, The new JSX in SelectionBar uses inline style objects in the component render, which should be converted to the project’s className-based styling. Update the relevant elements in SelectionBar.tsx to use className instead of style, and move the layout/spacing/positioning values currently applied inline into the existing styling system so the JSX stays consistent with the app’s tsx guidelines.Source: Coding guidelines
mobile/components/ListPanel.tsx (1)
262-283: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftMove the new notes UI styling out of inline
styleprops.The notes tab adds many inline style objects; please convert these new JSX nodes to the project’s
classNamestyling approach. As per coding guidelines,**/*.tsx: React JSX 頁面/元件中應使用className寫法，並避免使用 inline style.Also applies to: 285-421
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@mobile/components/ListPanel.tsx` around lines 262 - 283, The new notes UI in ListPanel should stop using inline style objects and follow the project’s className-based styling convention. Update the JSX around the annotations actions and related notes UI blocks in ListPanel to replace inline style props with className equivalents, keeping the same layout and visual behavior. Use the existing component structure in ListPanel and nearby notes-related nodes as the reference points while moving all new styling out of JSX inline objects.Source: Coding guidelines
mobile/app/reader/[id].tsx (1)
484-487: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftMove new JSX styling out of inline
styleprops.The newly added annotation toggle and banner use inline style objects; please convert these to the project’s
classNamestyling approach. As per coding guidelines,**/*.tsx: React JSX 頁面/元件中應使用className寫法，並避免使用 inline style.Also applies to: 535-540
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@mobile/app/reader/`[id].tsx around lines 484 - 487, The annotation toggle and banner in the reader screen are using inline JSX style objects instead of the project’s className approach. Update the relevant JSX in the reader component (including the annotation toggle and banner sections) to use className-based styling, and move any conditional styling currently handled in inline style props into reusable className logic or existing style utilities. Refer to the reader page component and the annotation-related JSX blocks to locate and replace these inline styles.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@mobile/app/reader/`[id].tsx:
- Around line 105-108: The reset path in the reader screen clears React Native
selection state but does not clear the WebView’s native selection, so keep both
in sync by routing these flows through a shared clear helper. Update the reader
screen logic around the selection reset handling in [id].tsx so the helper
clears local state and also invokes the WebView clearSelection action, and reuse
it from the reset/settings/list-panel paths that currently only call
setSelection(null) and related state setters.
- Around line 202-220: The reader event handlers are logging user-selected book
text outside development, which can expose private content. Update the logging
in the reader screen’s message handling path (the textSelected branch and the
other selected-text logging site in the same component) to either remove the
text snippet entirely or wrap it behind __DEV__ only. Keep non-sensitive event
logs like selectionCleared/annotationTapped if needed, but ensure no selected
content is emitted in production.
- Around line 376-403: The annotation update handlers in
handleChangeAnnotationColor, handleDeleteAnnotation, and
handleUpdateAnnotationNote call saveAnnotations without handling its async
failures, so the UI can appear saved even when persistence fails. Update these
handlers (and the related saveAnnotations(id, next) call in this block) to await
or catch the save result and handle rejection consistently, using the existing
annotation state/update flow in [id].tsx so failures do not get silently
ignored.
In `@mobile/reader-web/index.ts`:
- Around line 491-496: `verifyAnnotationsRendered` is checking the full
`renderedAnnotations` set against only the currently visible DOM, so annotations
from other pages/chapters are incorrectly reported missing and trigger
unnecessary `reinjectAllAnnotations` calls. Update the verification logic to
consider only annotations expected in the current rendered view (using the same
view/page/chapter context that drives rendering), and apply the same fix to the
duplicate logic around `renderAnnotations`/`verifyAnnotationsRendered` at the
other referenced block.
- Around line 498-528: Failures in addAnnotationMark are currently swallowed,
but applyAnnotations still records the annotation in renderedAnnotations, which
blocks future retries for marks that never rendered. Update the applyAnnotations
flow so only successfully added annotations are inserted into
renderedAnnotations, using addAnnotationMark’s success/failure outcome as the
gate. If underline registration throws or returns an invalid result, leave the
annotation out of the rendered set so later syncs can retry it. Apply the same
fix anywhere else in the same applyAnnotations block that marks annotations as
rendered after addAnnotationMark.
- Around line 294-299: The temporary debug bridge in debugLog is currently
always active and can leak touch telemetry and selected text/CFI data over the
RN bridge. Update debugLog in mobile/reader-web/index.ts to be gated behind an
explicit development-only flag or make it a no-op in production, and ensure the
later post({ type: 'debug', ... }) path is only reachable when that guard is
enabled. Keep the existing debugLog symbol so it is easy to locate and remove or
disable before release.
---
Outside diff comments:
In `@mobile/lib/library.ts`:
- Around line 140-153: The storage cleanup in removeBook is currently
fire-and-forget inside the updateMeta callback, so any AsyncStorage.multiRemove
rejection is unhandled and some keys may remain. Move the multiRemove call out
of updateMeta so removeBook can await it, or add explicit rejection handling
with catch; keep the file/cover deletion and record removal logic in
removeBook/updateMeta, and use the existing progressKey, settingsKey,
bookmarksKey, and annotationsKey helpers to locate the cleanup call.
---
Nitpick comments:
In `@mobile/app/reader/`[id].tsx:
- Around line 484-487: The annotation toggle and banner in the reader screen are
using inline JSX style objects instead of the project’s className approach.
Update the relevant JSX in the reader component (including the annotation toggle
and banner sections) to use className-based styling, and move any conditional
styling currently handled in inline style props into reusable className logic or
existing style utilities. Refer to the reader page component and the
annotation-related JSX blocks to locate and replace these inline styles.
In `@mobile/components/ListPanel.tsx`:
- Around line 262-283: The new notes UI in ListPanel should stop using inline
style objects and follow the project’s className-based styling convention.
Update the JSX around the annotations actions and related notes UI blocks in
ListPanel to replace inline style props with className equivalents, keeping the
same layout and visual behavior. Use the existing component structure in
ListPanel and nearby notes-related nodes as the reference points while moving
all new styling out of JSX inline objects.
In `@mobile/components/SelectionBar.tsx`:
- Around line 31-42: The new JSX in SelectionBar uses inline style objects in
the component render, which should be converted to the project’s className-based
styling. Update the relevant elements in SelectionBar.tsx to use className
instead of style, and move the layout/spacing/positioning values currently
applied inline into the existing styling system so the JSX stays consistent with
the app’s tsx guidelines.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: cc2bd079-b3af-4506-aba6-972afd366d29
📒 Files selected for processing (9)
RN_SETUP_GUIDE.mdmobile/app/reader/[id].tsxmobile/components/ListPanel.tsxmobile/components/SelectionBar.tsxmobile/lib/annotationColors.tsmobile/lib/library.tsmobile/lib/readerHtml.generated.tsmobile/lib/readerMessages.tsmobile/reader-web/index.ts
| saveAnnotations(id, next); | ||
| syncAnnotationsToWebView(next); | ||
| setSelection(null); | ||
| webviewRef.current?.postMessage(JSON.stringify({ type: 'clearSelection' })); | ||
| }; | ||
|
|
||
| const handleChangeAnnotationColor = (annotationId: string, color: string) => { | ||
| if (!id) return; | ||
| const next = annotations.map((a) => (a.id === annotationId ? { ...a, color } : a)); | ||
| setAnnotations(next); | ||
| saveAnnotations(id, next); | ||
| syncAnnotationsToWebView(next); | ||
| }; | ||
|
|
||
| const handleDeleteAnnotation = (annotationId: string) => { | ||
| if (!id) return; | ||
| const next = annotations.filter((a) => a.id !== annotationId); | ||
| setAnnotations(next); | ||
| saveAnnotations(id, next); | ||
| syncAnnotationsToWebView(next); | ||
| setEditingAnnotationId(null); | ||
| }; | ||
|
|
||
| const handleUpdateAnnotationNote = (annotationId: string, note: string) => { | ||
| if (!id) return; | ||
| const next = annotations.map((a) => (a.id === annotationId ? { ...a, note: note.trim() || undefined } : a)); | ||
| setAnnotations(next); | ||
| saveAnnotations(id, next); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Handle annotation persistence failures.
saveAnnotations is async, but create/change/delete/note updates ignore rejection. A failed AsyncStorage write leaves the UI showing saved annotations that will disappear after reload.
Proposed fix
- saveAnnotations(id, next);
+ void saveAnnotations(id, next).catch((err) => {
+ console.error('[reader] saveAnnotations failed', err);
+ setErrorMessage('註記儲存失敗，請稍後再試');
+ });Apply the same pattern to the other saveAnnotations(id, next) calls in this block.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@mobile/app/reader/`[id].tsx around lines 376 - 403, The annotation update
handlers in handleChangeAnnotationColor, handleDeleteAnnotation, and
handleUpdateAnnotationNote call saveAnnotations without handling its async
failures, so the UI can appear saved even when persistence fails. Update these
handlers (and the related saveAnnotations(id, next) call in this block) to await
or catch the save result and handle rejection consistently, using the existing
annotation state/update flow in [id].tsx so failures do not get silently
ignored.
Co-Authored-By: Claude Sonnet 5 <[email protected]>
1 participant
Summary by CodeRabbit
New Features
Bug Fixes