Conversation
…ation/data - 抽出純計算函數：progressCalculations、tocLookup、ttsFollowCalculations、bookmarkUtils - 抽出 useBookmarks、useAnnotationPopups、useChapterPageScan 三個低耦合 hooks - book 生命週期與 TTS 跟讀邏輯因雙向耦合，合併為單一 useReaderEngine hook - Reader.tsx 從 2383 行降至 641 行，已通過完整功能實測 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: 30 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 (7)
📝 WalkthroughWalkthroughAdds documentation describing a refactor progress tracker and functional-programming guidance, extracts pure calculation/lookup utilities (bookmarks, progress calculations, TOC/spine lookup, TTS follow calculations) from Reader.tsx into new modules, introduces new hooks (useBookmarks, useAnnotationPopups, useChapterPageScan, useReaderEngine), and rewires Reader.tsx to compose these hooks. ChangesPWA Reader functional refactor
Estimated code review effort: 4 (Complex) | ~75 minutes Sequence Diagram(s)sequenceDiagram
participant ReaderPage
participant useReaderEngine
participant EpubRendition
participant TTSEngine
ReaderPage->>useReaderEngine: initialize with viewer/book refs
useReaderEngine->>EpubRendition: render book, restore CFI/annotations
ReaderPage->>useReaderEngine: nextPage / prevPage
useReaderEngine->>EpubRendition: display page
EpubRendition-->>useReaderEngine: relocated event
useReaderEngine->>TTSEngine: speakCurrentPage
TTSEngine-->>useReaderEngine: highlight offset updates
useReaderEngine->>EpubRendition: auto page turn (follow logic)
🚥 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: 5
🧹 Nitpick comments (7)
pwa/src/components/Reader/bookmarkUtils.ts (1)
9-14: 📐 Maintainability & Code Quality | 🔵 Trivial
toggleBookmarkisn't actually pure.It calls
crypto.randomUUID()andDate.now()internally, both of which the PR's ownfunctional-thinking.md.mdclassifies as "actions" (time/execution-dependent), not calculations. This contradicts the stated goal of extracting pure calculation utilities into this module. Behavior is unaffected today, but consider passingid/addedAtin as parameters (with the caller supplying them) to keep this function testable/pure and consistent with the ACD principle documented in this same PR.♻️ Optional: make toggleBookmark pure by injecting id/timestamp
-export const toggleBookmark = (bookmarks: Bookmark[], cfi: string, label: string): Bookmark[] => { +export const toggleBookmark = (bookmarks: Bookmark[], cfi: string, label: string, id: string, addedAt: number): Bookmark[] => { if (bookmarks.some((b) => b.cfi === cfi)) { return bookmarks.filter((b) => b.cfi !== cfi) } - return [...bookmarks, { id: crypto.randomUUID(), cfi, label, addedAt: Date.now() }] + return [...bookmarks, { id, cfi, label, addedAt }] }🤖 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 `@pwa/src/components/Reader/bookmarkUtils.ts` around lines 9 - 14, Make toggleBookmark pure by removing the internal crypto.randomUUID() and Date.now() calls. Update toggleBookmark in bookmarkUtils.ts to accept id and addedAt as parameters, and have the caller provide those values when creating the new Bookmark. Keep the existing cfi-based toggle behavior in toggleBookmark unchanged, and use the Bookmark shape to locate the append branch that currently constructs the new object.pwa/src/page/Reader.tsx (1)
51-51: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winWhole-store subscription re-renders the entire Reader on any annotation change.
useAnnotationStore()with no selector subscribes to the full store, so everyaddAnnotation/updateColor/removeAnnotation(frequent while highlighting) re-renders this large component even though only the two stable action functions are needed. Prefer selectors.♻️ Use selectors
- const { clearAll: clearAnnotations, loadForBook } = useAnnotationStore() + const clearAnnotations = useAnnotationStore((s) => s.clearAll) + const loadForBook = useAnnotationStore((s) => s.loadForBook)🤖 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 `@pwa/src/page/Reader.tsx` at line 51, The Reader component is subscribing to the entire annotation store instead of just the needed actions, which causes unnecessary re-renders on every annotation update. Update the useAnnotationStore usage in Reader to use selectors for clearAll and loadForBook so only those stable functions are subscribed, and keep the rest of the component from re-rendering on addAnnotation, updateColor, or removeAnnotation changes.pwa/src/hooks/reader/useReaderEngine.ts (1)
179-184: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueGate the ungated production logs.
TTS logs are correctly behind
DEBUG_TTS_FOLLOW, but the[Progress]/[Reader]console.logcalls fire on everyrelocated/save cycle regardless of environment, which spams the console on each page turn in production. Consider gating these behind a debug flag as well.Also applies to: 502-511
🤖 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 `@pwa/src/hooks/reader/useReaderEngine.ts` around lines 179 - 184, The ungated [Progress] and [Reader] console.log calls in useReaderEngine are still running during every relocated/save cycle in production, so gate them behind the same kind of debug flag used for the TTS logs. Update the logging inside useReaderEngine to only emit when a debug condition is enabled, and apply the same guard to the other console.log block referenced in the review so page turns no longer spam production logs.pwa/src/hooks/reader/useBookmarks.ts (1)
15-29: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winSide effect inside
setStateupdater breaks purity; can double-write in StrictMode dev.
saveBookmarksis called from inside thesetBookmarksupdater callback in bothtoggleandremove. React 18 intentionally double-invokes updater functions in development under StrictMode to surface non-idempotent code, sosaveBookmarkscan fire twice per toggle/remove in dev. Given the PR's own Grokking Simplicity goal (separating actions from calculations), the persistence side effect should live outside the pure state-update calculation.♻️ Proposed fix: compute next state, then persist as a separate step
const toggle = (cfi: string, label: string) => { - setBookmarks((prev) => { - const next = toggleBookmark(prev, cfi, label) - saveBookmarks(bookId, next) - return next - }) + setBookmarks((prev) => toggleBookmark(prev, cfi, label)) } const remove = (id: string) => { - setBookmarks((prev) => { - const next = removeBookmarkById(prev, id) - saveBookmarks(bookId, next) - return next - }) + setBookmarks((prev) => removeBookmarkById(prev, id)) } + + useEffect(() => { + saveBookmarks(bookId, bookmarks) + }, [bookId, bookmarks])Note: this alternative reintroduces persistence on every
bookmarkschange (including the initial load), so guard against redundant writes if that's undesirable.🤖 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 `@pwa/src/hooks/reader/useBookmarks.ts` around lines 15 - 29, The `toggle` and `remove` handlers in `useBookmarks` mix a side effect into the `setBookmarks` updater, which breaks updater purity and can double-call `saveBookmarks` in StrictMode. Refactor so the updater only computes the next value via `toggleBookmark` and `removeBookmarkById`, then persist with `saveBookmarks(bookId, next)` outside the `setBookmarks` callback; keep the fix localized to the `useBookmarks` hook and its `toggle`/`remove` functions.pwa/src/hooks/reader/useAnnotationPopups.ts (3)
29-29: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueLeftover debug logging.
console.log/console.errorwith Chinese debug tags ([Ann:add] ...) look like development artifacts left in the extracted hook.Also applies to: 62-62
🤖 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 `@pwa/src/hooks/reader/useAnnotationPopups.ts` at line 29, Remove the leftover debug logging from useAnnotationPopups, including the console.log calls with the [Ann:add] Chinese debug tags and the related console.error at the other referenced spot. Update the affected logic in the hook methods that handle annotation popup flow so it no longer emits development-only console output, while preserving the existing behavior of the annotation handling code.
102-108: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winFragile id retrieval after
addAnnotation.Grabbing the new annotation's id via
.at(-1)?.idafter callingaddAnnotation(ann)relies on the store'ssetbeing synchronous and appending strictly to the end of the array. This works today given Zustand's synchronous updates, but it's an implicit cross-module contract rather than an explicit one — any future change toaddAnnotation(e.g., sorting, async persistence, batching) silently breaks id correlation between the store record and the epub.js underline (ann-${id}), causing color-change/delete to target the wrong mark.Consider having
addAnnotationreturn the generated id directly (useAnnotationStore.ts) instead of re-readinggetState()after the fact.🤖 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 `@pwa/src/hooks/reader/useAnnotationPopups.ts` around lines 102 - 108, The annotation popup flow in useAnnotationPopups relies on reading the newly added record back from useAnnotationStore via .at(-1)?.id, which is brittle and can desync epub underline ids from the store record. Update addAnnotation in useAnnotationStore to generate and return the annotation id directly, then use that returned id in useAnnotationPopups when calling addEpubAnnotation so the store entry and ann-${id} stay explicitly linked.
20-20: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePossibly redundant
addEpubAnnotationRef.
addEpubAnnotationis already a stableuseCallback(its only dependency,lastIframeClickRef, is a ref and never changes identity), so mirroring it intoaddEpubAnnotationRefon every render and exposing both the ref and the function from the hook looks unnecessary. If this was a workaround for stale closures prior to the refactor, it may no longer be needed.Also applies to: 76-76, 170-171
🤖 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 `@pwa/src/hooks/reader/useAnnotationPopups.ts` at line 20, The `addEpubAnnotationRef` mirror in `useAnnotationPopups` appears redundant because `addEpubAnnotation` is already stable via `useCallback`; remove the extra ref wiring and use the callback directly where it’s currently being mirrored or exposed. Update the hook’s return value and any internal assignments around `addEpubAnnotationRef`, `addEpubAnnotation`, and the related popup setup so only one callable path remains and there’s no per-render ref syncing.
🤖 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 `@functional-thinking.md.md`:
- Line 17: The document has a few wording/grammar issues flagged by static
analysis, so do a quick polish pass on the affected guidance text. Review the
sentences around the referenced spots in the contributor instructions,
especially the phrase with 三種類別, the two instances using 地, and the likely typo
around 步驟, then rewrite them with natural, correct wording while preserving the
intended meaning and style.
In `@pwa/src/hooks/reader/useAnnotationPopups.ts`:
- Around line 137-153: handleChangeColor and handleDeleteMark in
useAnnotationPopups both call renditionRef.current?.annotations.remove without
protection, so mirror the existing defensive handling used elsewhere in this
hook by wrapping those removes in try/catch. Keep the flow in handleChangeColor
so addEpubAnnotation and updateColor still run even if remove fails, and in
handleDeleteMark so removeAnnotation and setEditPopup(null) always execute. Use
the existing annotation lookup via
useAnnotationStore.getState().annotations.find and the shared annotations.remove
call as the fix points.
In `@pwa/src/hooks/reader/useChapterPageScan.ts`:
- Around line 57-61: Restore the two-pass EPUB scan behavior in
useChapterPageScan: keep the initial hiddenRendition.renderTo pass without
allowScriptedContent, and only retry with allowScriptedContent: true if the
first scan returns no page counts. Update the scanBook/renderTo flow so the
fallback is conditional rather than always enabling scripts on every background
render.
- Around line 79-137: The final accurate-total update in useChapterPageScan is
currently guarded by scannedCount === spineTotal, so any skipped or unrenderable
chapter prevents the correction from ever applying. Move the
computeAccurateTotal and setPageInfo logic out of the per-item completion check
into a post-loop finalization path after the spineItems scan finishes, using
chapterPagesRef/current known entries plus the computed average to fill missing
chapters before updating the page totals.
In `@pwa/src/hooks/reader/useReaderEngine.ts`:
- Around line 263-269: The fetch flow in useReaderEngine currently assumes
fetch(bookPath) succeeded, but res.arrayBuffer() will still run on non-2xx
responses and defer the failure until ePub(buffer) much later. Update the
promise chain in useReaderEngine to check res.ok immediately after fetch
resolves and throw a clear error for bad responses so the existing catch path
can surface a proper failure instead of leaving the reader stuck on loading.
---
Nitpick comments:
In `@pwa/src/components/Reader/bookmarkUtils.ts`:
- Around line 9-14: Make toggleBookmark pure by removing the internal
crypto.randomUUID() and Date.now() calls. Update toggleBookmark in
bookmarkUtils.ts to accept id and addedAt as parameters, and have the caller
provide those values when creating the new Bookmark. Keep the existing cfi-based
toggle behavior in toggleBookmark unchanged, and use the Bookmark shape to
locate the append branch that currently constructs the new object.
In `@pwa/src/hooks/reader/useAnnotationPopups.ts`:
- Line 29: Remove the leftover debug logging from useAnnotationPopups, including
the console.log calls with the [Ann:add] Chinese debug tags and the related
console.error at the other referenced spot. Update the affected logic in the
hook methods that handle annotation popup flow so it no longer emits
development-only console output, while preserving the existing behavior of the
annotation handling code.
- Around line 102-108: The annotation popup flow in useAnnotationPopups relies
on reading the newly added record back from useAnnotationStore via .at(-1)?.id,
which is brittle and can desync epub underline ids from the store record. Update
addAnnotation in useAnnotationStore to generate and return the annotation id
directly, then use that returned id in useAnnotationPopups when calling
addEpubAnnotation so the store entry and ann-${id} stay explicitly linked.
- Line 20: The `addEpubAnnotationRef` mirror in `useAnnotationPopups` appears
redundant because `addEpubAnnotation` is already stable via `useCallback`;
remove the extra ref wiring and use the callback directly where it’s currently
being mirrored or exposed. Update the hook’s return value and any internal
assignments around `addEpubAnnotationRef`, `addEpubAnnotation`, and the related
popup setup so only one callable path remains and there’s no per-render ref
syncing.
In `@pwa/src/hooks/reader/useBookmarks.ts`:
- Around line 15-29: The `toggle` and `remove` handlers in `useBookmarks` mix a
side effect into the `setBookmarks` updater, which breaks updater purity and can
double-call `saveBookmarks` in StrictMode. Refactor so the updater only computes
the next value via `toggleBookmark` and `removeBookmarkById`, then persist with
`saveBookmarks(bookId, next)` outside the `setBookmarks` callback; keep the fix
localized to the `useBookmarks` hook and its `toggle`/`remove` functions.
In `@pwa/src/hooks/reader/useReaderEngine.ts`:
- Around line 179-184: The ungated [Progress] and [Reader] console.log calls in
useReaderEngine are still running during every relocated/save cycle in
production, so gate them behind the same kind of debug flag used for the TTS
logs. Update the logging inside useReaderEngine to only emit when a debug
condition is enabled, and apply the same guard to the other console.log block
referenced in the review so page turns no longer spam production logs.
In `@pwa/src/page/Reader.tsx`:
- Line 51: The Reader component is subscribing to the entire annotation store
instead of just the needed actions, which causes unnecessary re-renders on every
annotation update. Update the useAnnotationStore usage in Reader to use
selectors for clearAll and loadForBook so only those stable functions are
subscribed, and keep the rest of the component from re-rendering on
addAnnotation, updateColor, or removeAnnotation changes.
🪄 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: 905cdbba-d8a3-4f2e-8e29-523fb69acabd
📒 Files selected for processing (11)
FP_REFACTOR_PROGRESS.mdfunctional-thinking.md.mdpwa/src/components/Reader/bookmarkUtils.tspwa/src/components/Reader/progressCalculations.tspwa/src/components/Reader/tocLookup.tspwa/src/components/Reader/ttsFollowCalculations.tspwa/src/hooks/reader/useAnnotationPopups.tspwa/src/hooks/reader/useBookmarks.tspwa/src/hooks/reader/useChapterPageScan.tspwa/src/hooks/reader/useReaderEngine.tspwa/src/page/Reader.tsx
|
|
||
| ### **1\. 動作、計算與資料 (ACD) 分類準則** | ||
|
|
||
| 每次編寫程式碼前，你必須先識別這三種類別： |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Minor grammar issues flagged by static analysis.
LanguageTool flags a few wording issues: missing quantifier near line 17, adverbial "地" usage near lines 39/41, and a likely typo near line 67 ("步驟" vs "不驟" mis-render). Since this doc is used as an operating guideline for contributors/agents, worth a quick polish pass.
Also applies to: 39-39, 41-41, 67-67
🧰 Tools
🪛 LanguageTool
[uncategorized] ~17-~17: 数词与名词之间一般应存在量词，可能缺少量词。
Context: ... 動作、計算與資料 (ACD) 分類準則** 每次編寫程式碼前，你必須先識別這三種類別： - 動作 (Actions)：執行結果取決於「執行時機」或「執行...
(wa5)
🤖 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 `@functional-thinking.md.md` at line 17, The document has a few wording/grammar
issues flagged by static analysis, so do a quick polish pass on the affected
guidance text. Review the sentences around the referenced spots in the
contributor instructions, especially the phrase with 三種類別, the two instances
using 地, and the likely typo around 步驟, then rewrite them with natural, correct
wording while preserving the intended meaning and style.
Source: Linters/SAST tools
| const scanBook = ePub(buffer.slice(0)) | ||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| const hiddenRendition = (scanBook as any).renderTo(hiddenEl, { | ||
| width, height, spread: 'none', flow: 'paginated', allowScriptedContent: true, | ||
| }) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the target hook and the legacy reader scan logic.
git ls-files 'pwa/src/hooks/reader/useChapterPageScan.ts' 'mobile/reader-web/index.ts'
echo '--- useChapterPageScan.ts (relevant range) ---'
sed -n '1,220p' pwa/src/hooks/reader/useChapterPageScan.ts
echo '--- mobile/reader-web/index.ts (around legacy scan) ---'
sed -n '720,860p' mobile/reader-web/index.tsRepository: Retsomm/TravelInTime
Length of output: 13586
Restore the two-pass scan fallback
The hidden background rendition now enables allowScriptedContent: true for every EPUB scan. The older reader flow only retried with scripts enabled when a script-free pass returned no page counts, which avoids giving untrusted EPUBs extra script execution in a non-interactive background render. Keep the first pass with scripts off, and only retry with scripts on if the scan comes back empty.
🤖 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 `@pwa/src/hooks/reader/useChapterPageScan.ts` around lines 57 - 61, Restore the
two-pass EPUB scan behavior in useChapterPageScan: keep the initial
hiddenRendition.renderTo pass without allowScriptedContent, and only retry with
allowScriptedContent: true if the first scan returns no page counts. Update the
scanBook/renderTo flow so the fallback is conditional rather than always
enabling scripts on every background render.
- 修正章節掃描頁數校正被單一渲染失敗章節永久卡住的問題 - fetch 書本檔案時檢查 res.ok，避免非 2xx 回應被當成正常資料解析 - annotations.remove 呼叫加上防護，避免例外中斷後續狀態更新 - addAnnotation 回傳實際產生的 id，避免用 .at(-1) 猜測造成競態 - toggleBookmark 改為純函數，saveBookmarks 移出 setState updater - 移除多餘的 ref 鏡射與未加保護的 debug log，Reader 改用 store selector
1 participant
…ation/data
Summary by CodeRabbit
New Features
Bug Fixes