Conversation
- 新增 clients/apiClient.ts、services/{progressService,syncQueue,syncGate}.ts、
hooks/reader/useProgress.ts，翻頁存進度改成本機立即寫 + cache 立即同步 +
debounce 1400ms 才打網路，並在 pagehide/hook 卸載時 flush，避免退出時遺失
debounce 視窗內的最後幾次進度。
- cloudSync.ts 改用共用的 syncGate/syncQueue，避免跟新 Service 層各自維護一份
佇列導致「book 先於 progress 送達」的順序保證失效。
- 順手修正背景章節頁數掃描器的既有問題：新增 pageCountCache.ts 快取上次完整
掃描的每章頁數，開書時提前灌回，讓總頁數一開始就準；並修正掃描完成時可能被
過渡尺寸污染出錯誤偏低頁數的問題（比照翻頁邏輯加上不能變小的防呆，且不把
可疑結果寫入快取），快取加上版本號讓修正前寫入的舊快取失效。
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ice/Hook 分層 - bookmarks/annotations 後端 PUT 從整包覆蓋改成逐筆 upsert + 軟刪除（schema 新增 updatedAt/deletedAt），取代原本「只寫不讀」且無法正確處理跨裝置刪除的同步方式。 - 新增 bookService/bookmarkService/annotationService，取代 store/useAnnotationStore.ts （Zustand）與 utils/cloudSync.ts；useLibrary.ts/useBookmarks.ts 改為呼叫新 service。 - 新增 useCloudRestore.ts：登入時、以及每次開啟個別書本/進入「我的筆記」頁時， 背景跟雲端合併書籤/註記/書庫清單。 - 過程中修好的問題：useAnnotations.ts 回傳函式未用 useCallback 穩定參照導致的 「Maximum update depth exceeded」無限迴圈；還原流程漏了先補推 Book 列造成的 外鍵違反；extractMeta 用的獨立 epub.js book 實例沒套用既有的 destroy race 修補； 劃線色塊選單在螢幕邊緣被裁切；跳轉到既有註記後標記未重新渲染。 跨裝置同步目前仍有已知問題（使用者回報中），保留現狀待後續排查，不影響其餘功能。 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThe PWA adds service-based local and cloud synchronization with timestamped upserts, soft deletion, conflict merging, one-time cloud restoration, React Query integration, and reader persistence for annotations, bookmarks, progress, and page counts. ChangesOffline-first synchronization
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Reader
participant useCloudRestoreMutation
participant bookService
participant progressService
participant bookmarkService
participant annotationService
Reader->>useCloudRestoreMutation: trigger restore after sign-in
useCloudRestoreMutation->>bookService: merge and replace books
useCloudRestoreMutation->>progressService: restore merged progress
useCloudRestoreMutation->>bookmarkService: restore merged bookmarks
useCloudRestoreMutation->>annotationService: restore merged annotations
Reader->>progressService: save and flush progress
Reader->>bookmarkService: push bookmark changes
Reader->>annotationService: push annotation changes
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 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: 20
🧹 Nitpick comments (2)
pwa-next/src/services/annotationService.ts (2)
19-22: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMove
annotationsKeyinto@/constants/storageKeys.
bookmarkService.tsimportsbookmarksKeyfrom@/constants/storageKeysat Line 2. This file defines its own key builder inline instead. The two services now follow different conventions for the same concern. Any other module that needs the annotation key must re-derive thetit-annotations-${bookId}string, and the two definitions can drift.Export
annotationsKeyfrom@/constants/storageKeysand import it here.#!/bin/bash # Description: Find every place that builds the annotation storage key. rg -nP --type=ts -C2 'tit-annotations' pwa-next/src fd -t f 'storageKeys.ts' pwa-next/src --exec cat -n {}🤖 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-next/src/services/annotationService.ts` around lines 19 - 22, Move the annotationsKey builder from annotationService.ts into `@/constants/storageKeys`, export it there alongside bookmarksKey, and import it in annotationService.ts. Remove the local definition while preserving deletedKey’s use of annotationsKey and the existing `tit-annotations-${bookId}` format.
24-49: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoff
readJsonand the tombstone helpers duplicatebookmarkService.ts.Lines 24-31 and Lines 38-48 are identical to
pwa-next/src/services/bookmarkService.tsLines 21-28 and Lines 35-45, apart from the key builder and the item type. The tombstone format (Record<string, number>) and the:deletedkey suffix are also repeated. A future fix to the tombstone logic must be applied twice.Extract a shared generic helper, for example
createTombstoneStore(keyFor), and reuse it in both services.🤖 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-next/src/services/annotationService.ts` around lines 24 - 49, Extract the duplicated JSON persistence and tombstone behavior from the local service object and bookmarkService into a shared generic createTombstoneStore(keyFor) helper. Parameterize the key builder and item type while preserving the existing load, save, loadDeletedIds, recordDeleted, and clearDeletedIds behavior, including the :deleted key format, then have both services reuse the helper.
🤖 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 `@pwa-next/src/App.tsx`:
- Around line 44-50: The restore effect around useCloudRestoreMutation currently
marks RESTORE_ONCE_KEY before restoreMutation.mutate() completes. Return an
explicit success result from the mutation, set RESTORE_ONCE_KEY only after
successful completion, and add a separate in-flight guard to suppress duplicate
Strict Mode requests; clear that guard when the restore fails or returns no data
so later attempts can retry.
In `@pwa-next/src/app/api/books/`[bookId]/annotations/route.ts:
- Around line 52-53: Update the ownership checks in
pwa-next/src/app/api/books/[bookId]/annotations/route.ts:52-53 and
pwa-next/src/app/api/books/[bookId]/bookmarks/route.ts:46-47 to select bookId
alongside clerkUserId, and skip existing records unless both the authenticated
user and route bookId match; apply the guard before the upsert update path.
- Around line 48-85: In pwa-next/src/app/api/books/[bookId]/annotations/route.ts
lines 48-85, replace the per-record tx.annotation.findUnique ownership checks in
the upsert transaction with one findMany for all incoming IDs, build the
foreign-owner ID set in memory, skip those records, and pass an explicit
transaction timeout to prisma.$transaction; cap accepted upserts or batch them.
In pwa-next/src/app/api/books/[bookId]/bookmarks/route.ts lines 41-73, apply the
same batched tx.bookmark ownership lookup, explicit timeout, and
upsert-size/batching safeguard.
- Around line 42-46: Introduce one shared schema helper for validating PUT
payloads, then use it in both annotations and bookmarks routes. In
pwa-next/src/app/api/books/[bookId]/annotations/route.ts:42-46, validate each
upsert’s string id, cfi, and text, finite numeric createdAt and updatedAt, plus
string deletedIds; in
pwa-next/src/app/api/books/[bookId]/bookmarks/route.ts:35-39, validate id, cfi,
label, and finite numeric addedAt and updatedAt. Return 400 for any invalid
element before Prisma processing and remove reliance on unchecked casts.
- Around line 54-77: Update the annotation upsert ownership check around
tx.annotation.upsert to require both clerkUserId and bookId, ensuring an
existing annotation id from another book cannot enter the update branch. Keep
the existing update payload and create behavior unchanged.
In `@pwa-next/src/clients/apiClient.ts`:
- Line 14: Change the getAuthHeader return type from HeadersInit to
Record<string, string> so the object spread used when constructing request
headers remains safe. Keep the existing authentication-header merge behavior
unchanged.
In `@pwa-next/src/components/Reader/pageCountCache.ts`:
- Around line 27-29: Update the cache parsing flow around the parsed
PageCountCache result to validate required numeric settings and ensure
chapterPages exists with only numeric values before returning it as
PageCountCache. Return null for any invalid or incomplete entry, while
preserving the existing version check and valid-cache behavior.
- Around line 43-52: Update pwa-next/src/components/Reader/pageCountCache.ts
lines 43-52 and the PageCountCache definition to store reader width and height,
and require both dimensions plus all typography settings to match before
hydrating cached chapter counts. Update
pwa-next/src/hooks/reader/useChapterPageScan.ts lines 136-152 so that, after
layout stabilization, lower scanned totals are accepted and the completed scan
result replaces the stale cache.
In `@pwa-next/src/hooks/reader/useAnnotations.ts`:
- Around line 27-57: Prevent stale-snapshot overwrites in addAnnotation,
removeAnnotation, updateColor, and updateNote by maintaining the latest
annotation array in a ref or routing all four callbacks through a shared atomic
mutation helper. Ensure each mutation reads the current value, updates the ref
and React state, then passes that same merged array to
annotationService.local.save and pushNow, while preserving existing ID,
deletion, color, and note behavior.
In `@pwa-next/src/hooks/reader/useBookmarks.ts`:
- Around line 14-17: Update the useEffect keyed by bookId to track whether its
restoreForBook(bookId) request is still current, using a cleanup flag or request
identifier. Apply setBookmarks only when the response belongs to the active
effect and bookId, and invalidate the prior request during cleanup while
preserving the immediate local bookmark load.
In `@pwa-next/src/hooks/reader/useProgress.ts`:
- Around line 14-27: Update the useQuery configuration in useProgress to provide
local progress freshness via initialDataUpdatedAt, using the loaded progress
updatedAt value or 0 when absent. Preserve the existing staleTime and queryFn
behavior so stale initial data triggers the remote fetch.
In `@pwa-next/src/hooks/reader/useReaderEngine.ts`:
- Around line 805-809: Update the restoreForBook(bookId) callback to reconcile
existing rendered EPUB annotations against merged: remove overlays for local
annotations absent from merged, then refresh overlays for retained annotations
whose cloud color or CFI changed before adding the merged annotations. Preserve
the destroyed/renditionRef guard and keep the in-page annotation state
synchronized with the merged list.
In `@pwa-next/src/hooks/useLibrary.ts`:
- Around line 116-126: Update removeBook to delete all local progress and
bookmark records associated with id, and create the required deletion tombstones
before calling bookService.remote.syncRemoveBook(id). Preserve the existing
file, cover, settings, metadata, and record removal behavior.
- Around line 93-103: Track whether each import remains active across the
metadata extraction callback in the import flow, and invalidate that operation
in removeBook. Before writing the cover, updating records, saving metadata, or
calling syncBook, verify the import is still active; return immediately for
invalidated imports so a removal cannot be undone.
In `@pwa-next/src/services/bookmarkService.ts`:
- Around line 97-106: Update merge in
pwa-next/src/services/bookmarkService.ts#L97-L106 and the corresponding merge in
pwa-next/src/services/annotationService.ts#L103-L112 to accept remote tombstone
ids with deletedAt timestamps and discard local records when the remote deletion
is newer than local updatedAt. Update the GET handlers in
pwa-next/src/app/api/books/[bookId]/bookmarks/route.ts#L15-L15 and
pwa-next/src/app/api/books/[bookId]/annotations/route.ts#L15-L15 to return
recent soft-deleted ids and deletedAt values separately, retaining only
tombstones within a bounded retention window.
In `@pwa-next/src/services/bookService.ts`:
- Around line 23-29: Update listMeta to validate the parsed localStorage value
with Array.isArray before returning it; return the existing empty-array fallback
for valid non-array JSON as well as parse failures, while preserving valid
BookRecord[] results.
In `@pwa-next/src/services/progressService.ts`:
- Around line 69-89: Update pushDebounced and flush so the pending CFI is
retained until remote.pushNow confirms a successful write, restoring or
preserving it when the call fails instead of deleting it first. Ensure failed
writes remain retryable by flushAll through the existing online event or a
bounded retry schedule, while successful writes clear the dirty record.
- Around line 27-30: Update save, the progress PUT flow, and the merge
comparison around the affected progress-service symbols to use the
server-assigned revision or timestamp as the sole ordering value. After a
successful PUT, return and persist that server value instead of Date.now(),
ensure reads use the persisted server value, and compare local versus remote
progress only through this shared authoritative field.
In `@pwa-next/src/services/syncGate.ts`:
- Around line 4-10: Replace the boolean-only state in
setSyncEnabled/getSyncEnabled with a sync-session generation that advances
whenever the sync session changes. In progressService.remote.pushNow, capture
the current generation before enqueueing and have every queued task validate
that generation again immediately before calling apiClient; skip stale tasks so
work from a prior account cannot be sent.
In `@pwa-next/src/services/syncQueue.ts`:
- Around line 6-10: Update enqueue so each key’s queue continues from a settled
promise when a task rejects, rather than storing a rejected promise in queues.
Keep returning the current task’s Promise<boolean> so callers still receive its
success or failure, while attach rejection handling to the internal chain to
prevent unhandled rejections and allow later tasks to run.
---
Nitpick comments:
In `@pwa-next/src/services/annotationService.ts`:
- Around line 19-22: Move the annotationsKey builder from annotationService.ts
into `@/constants/storageKeys`, export it there alongside bookmarksKey, and import
it in annotationService.ts. Remove the local definition while preserving
deletedKey’s use of annotationsKey and the existing `tit-annotations-${bookId}`
format.
- Around line 24-49: Extract the duplicated JSON persistence and tombstone
behavior from the local service object and bookmarkService into a shared generic
createTombstoneStore(keyFor) helper. Parameterize the key builder and item type
while preserving the existing load, save, loadDeletedIds, recordDeleted, and
clearDeletedIds behavior, including the :deleted key format, then have both
services reuse the helper.
🪄 Autofix
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: 887e0dfb-dbb4-4a0a-8961-5186c15aa0e8
⛔ Files ignored due to path filters (1)
pwa-next/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (33)
pwa-next/package.jsonpwa-next/prisma/schema.prismapwa-next/src/App.tsxpwa-next/src/app/api/books/[bookId]/annotations/route.tspwa-next/src/app/api/books/[bookId]/bookmarks/route.tspwa-next/src/app/layout.tsxpwa-next/src/clients/apiClient.tspwa-next/src/components/NotePanel.tsxpwa-next/src/components/QueryProvider.tsxpwa-next/src/components/Reader/BookmarkList.tsxpwa-next/src/components/Reader/bookmarkUtils.tspwa-next/src/components/Reader/pageCountCache.tspwa-next/src/constants/storageKeys.tspwa-next/src/hooks/reader/useAnnotationPopups.tspwa-next/src/hooks/reader/useAnnotations.tspwa-next/src/hooks/reader/useBookmarks.tspwa-next/src/hooks/reader/useChapterPageScan.tspwa-next/src/hooks/reader/useProgress.tspwa-next/src/hooks/reader/useReaderEngine.tspwa-next/src/hooks/useCloudRestore.tspwa-next/src/hooks/useLibrary.tspwa-next/src/page/Notes.tsxpwa-next/src/page/Reader.tsxpwa-next/src/services/annotationService.tspwa-next/src/services/bookService.tspwa-next/src/services/bookmarkService.tspwa-next/src/services/progressService.tspwa-next/src/services/syncGate.tspwa-next/src/services/syncQueue.tspwa-next/src/store/useAnnotationStore.tspwa-next/src/utils/annotationExport.tspwa-next/src/utils/cloudSync.tspwa-next/src/utils/epubMetadata.ts
💤 Files with no reviewable changes (2)
- pwa-next/src/store/useAnnotationStore.ts
- pwa-next/src/utils/cloudSync.ts
| // 登入時觸發一次讀取／還原：跟雲端資料合併書庫清單/進度/書籤/註記，取代舊版 | ||
| // 只寫不讀的登入補推 effect（見 useCloudRestore.ts 的完整合併邏輯說明）。 | ||
| useEffect(() => { | ||
| if (!isSignedIn) return | ||
| Promise.all( | ||
| records.map(async (r) => { | ||
| const bookSynced = await syncBook(r.id, r.title, r.author, r.filename) | ||
| // Book 那筆列都沒寫成功，進度/書籤/註記會撞外鍵違反錯誤，不必送出 | ||
| if (!bookSynced) return | ||
| const cfi = loadProgress(r.id) | ||
| const bookmarks = loadBookmarks(r.id) | ||
| const annotations = loadAnnotationsForBook(r.id) | ||
| await Promise.all([ | ||
| cfi ? syncProgress(r.id, cfi) : undefined, | ||
| syncBookmarks(r.id, bookmarks), | ||
| syncAnnotations(r.id, annotations), | ||
| ]) | ||
| }), | ||
| ) | ||
| if (sessionStorage.getItem(RESTORE_ONCE_KEY) === '1') return | ||
| sessionStorage.setItem(RESTORE_ONCE_KEY, '1') | ||
| restoreMutation.mutate() |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Set the completion marker only after a successful restore.
Line 49 marks restoration as complete before any request finishes. useCloudRestoreMutation returns normally when fetchAll() returns null, and a later request can also fail. The current tab will not retry restoration after a transient offline failure.
Return an explicit completion result from the mutation. Set RESTORE_ONCE_KEY only when that result is successful. Keep a separate in-flight guard to prevent duplicate Strict Mode requests, and clear that guard after failure.
🤖 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-next/src/App.tsx` around lines 44 - 50, The restore effect around
useCloudRestoreMutation currently marks RESTORE_ONCE_KEY before
restoreMutation.mutate() completes. Return an explicit success result from the
mutation, set RESTORE_ONCE_KEY only after successful completion, and add a
separate in-flight guard to suppress duplicate Strict Mode requests; clear that
guard when the restore fails or returns no data so later attempts can retry.
| if (!Array.isArray(body.upserts) || !Array.isArray(body.deletedIds)) { | ||
| return NextResponse.json({ error: "upserts and deletedIds must be arrays" }, { status: 400 }); | ||
| } | ||
| const upserts = body.upserts; | ||
| const deletedIds = body.deletedIds; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Both PUT routes accept unvalidated record payloads. The body is cast with as and only the two top-level fields are checked with Array.isArray. No route validates the shape of each element, so a malformed createdAt, addedAt, updatedAt, or missing id reaches Prisma and produces an unhandled 500 instead of a 400.
pwa-next/src/app/api/books/[bookId]/annotations/route.ts#L42-L46: validate eachupsertselement for a stringid, stringcfi, stringtext, and finite numericcreatedAtandupdatedAt; validate that every entry ofdeletedIdsis a string.pwa-next/src/app/api/books/[bookId]/bookmarks/route.ts#L35-L39: apply the same element validation forid,cfi,label,addedAt, andupdatedAt.
Use one shared schema helper for both routes so the two contracts cannot drift.
📍 Affects 2 files
pwa-next/src/app/api/books/[bookId]/annotations/route.ts#L42-L46(this comment)pwa-next/src/app/api/books/[bookId]/bookmarks/route.ts#L35-L39
🤖 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-next/src/app/api/books/`[bookId]/annotations/route.ts around lines 42 -
46, Introduce one shared schema helper for validating PUT payloads, then use it
in both annotations and bookmarks routes. In
pwa-next/src/app/api/books/[bookId]/annotations/route.ts:42-46, validate each
upsert’s string id, cfi, and text, finite numeric createdAt and updatedAt, plus
string deletedIds; in
pwa-next/src/app/api/books/[bookId]/bookmarks/route.ts:35-39, validate id, cfi,
label, and finite numeric addedAt and updatedAt. Return 400 for any invalid
element before Prisma processing and remove reliance on unchecked casts.
| await prisma.$transaction(async (tx) => { | ||
| for (const a of upserts) { | ||
| // 同 bookmarks/route.ts：upsert 的 where 只能用唯一鍵（id），這裡先明確檢查歸屬， | ||
| // 避免惡意送出別人的註記 id 時被拿來竄改別人的資料（防禦性檢查，不影響正常流程）。 | ||
| const existing = await tx.annotation.findUnique({ where: { id: a.id }, select: { clerkUserId: true } }); | ||
| if (existing && existing.clerkUserId !== auth.userId) continue; | ||
| await tx.annotation.upsert({ | ||
| where: { id: a.id }, | ||
| create: { | ||
| id: a.id, | ||
| bookId, | ||
| clerkUserId: auth.userId, | ||
| cfi: a.cfi, | ||
| text: a.text, | ||
| note: a.note ?? "", | ||
| color: a.color ?? "", | ||
| chapter: a.chapter ?? "", | ||
| createdAt: new Date(a.createdAt), | ||
| updatedAt: new Date(a.updatedAt), | ||
| }, | ||
| update: { | ||
| cfi: a.cfi, | ||
| text: a.text, | ||
| note: a.note ?? "", | ||
| color: a.color ?? "", | ||
| chapter: a.chapter ?? "", | ||
| updatedAt: new Date(a.updatedAt), | ||
| deletedAt: null, // 重新新增一筆先前被軟刪除的同 id 註記時要復活 | ||
| }, | ||
| }); | ||
| } | ||
| if (deletedIds.length > 0) { | ||
| await tx.annotation.updateMany({ | ||
| where: { id: { in: deletedIds }, clerkUserId: auth.userId }, | ||
| data: { deletedAt: new Date() }, | ||
| }); | ||
| } | ||
| }); |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy lift
Both PUT routes run two sequential queries per record inside one interactive transaction. The shared root cause is the per-record findUnique ownership check in the upsert loop. The client services push the complete local list on every change, so the array grows without bound and the transaction can exceed the Prisma default timeout of 5000 ms. The client catches the failure and returns false, so synchronization stops without a user-visible signal.
pwa-next/src/app/api/books/[bookId]/annotations/route.ts#L48-L85: replace the per-recordtx.annotation.findUniquewith a singlefindManyover all incoming ids, build the foreign-owner set in memory, and pass an explicittimeouttoprisma.$transaction.pwa-next/src/app/api/books/[bookId]/bookmarks/route.ts#L41-L73: apply the same batchedfindManylookup fortx.bookmarkand the same explicittimeout.
Also cap the accepted upserts length, or have the client services push in bounded batches.
📍 Affects 2 files
pwa-next/src/app/api/books/[bookId]/annotations/route.ts#L48-L85(this comment)pwa-next/src/app/api/books/[bookId]/bookmarks/route.ts#L41-L73
🤖 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-next/src/app/api/books/`[bookId]/annotations/route.ts around lines 48 -
85, In pwa-next/src/app/api/books/[bookId]/annotations/route.ts lines 48-85,
replace the per-record tx.annotation.findUnique ownership checks in the upsert
transaction with one findMany for all incoming IDs, build the foreign-owner ID
set in memory, skip those records, and pass an explicit transaction timeout to
prisma.$transaction; cap accepted upserts or batch them. In
pwa-next/src/app/api/books/[bookId]/bookmarks/route.ts lines 41-73, apply the
same batched tx.bookmark ownership lookup, explicit timeout, and
upsert-size/batching safeguard.
| const existing = await tx.annotation.findUnique({ where: { id: a.id }, select: { clerkUserId: true } }); | ||
| if (existing && existing.clerkUserId !== auth.userId) continue; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
The ownership check in both routes omits bookId. Each route selects only clerkUserId and compares it to the authenticated user. The route path carries a bookId, but the check ignores it. If a client sends an id that belongs to the same user under a different book, the upsert takes the update branch and overwrites that record's content while it stays attached to the other book.
pwa-next/src/app/api/books/[bookId]/annotations/route.ts#L52-L53: addbookId: trueto theselectand skip the record whenexisting.bookId !== bookId.pwa-next/src/app/api/books/[bookId]/bookmarks/route.ts#L46-L47: apply the samebookIdselection and guard.
📍 Affects 2 files
pwa-next/src/app/api/books/[bookId]/annotations/route.ts#L52-L53(this comment)pwa-next/src/app/api/books/[bookId]/bookmarks/route.ts#L46-L47
🤖 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-next/src/app/api/books/`[bookId]/annotations/route.ts around lines 52 -
53, Update the ownership checks in
pwa-next/src/app/api/books/[bookId]/annotations/route.ts:52-53 and
pwa-next/src/app/api/books/[bookId]/bookmarks/route.ts:46-47 to select bookId
alongside clerkUserId, and skip existing records unless both the authenticated
user and route bookId match; apply the guard before the upsert update path.
| await tx.annotation.upsert({ | ||
| where: { id: a.id }, | ||
| create: { | ||
| id: a.id, | ||
| bookId, | ||
| clerkUserId: auth.userId, | ||
| cfi: a.cfi, | ||
| text: a.text, | ||
| note: a.note ?? "", | ||
| color: a.color ?? "", | ||
| chapter: a.chapter ?? "", | ||
| createdAt: new Date(a.createdAt), | ||
| updatedAt: new Date(a.updatedAt), | ||
| }, | ||
| update: { | ||
| cfi: a.cfi, | ||
| text: a.text, | ||
| note: a.note ?? "", | ||
| color: a.color ?? "", | ||
| chapter: a.chapter ?? "", | ||
| updatedAt: new Date(a.updatedAt), | ||
| deletedAt: null, // 重新新增一筆先前被軟刪除的同 id 註記時要復活 | ||
| }, | ||
| }); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
The update branch does not verify that the record belongs to bookId.
The ownership check at Line 53 compares only clerkUserId. If the same user sends an annotation id that exists under a different book, the upsert takes the update branch and overwrites cfi, text, and note on the record that belongs to the other book. The bookId column is not part of the update payload, so the record stays attached to the wrong book with the new content.
Include bookId in the ownership check.
🛡️ Proposed fix
- const existing = await tx.annotation.findUnique({ where: { id: a.id }, select: { clerkUserId: true } });
- if (existing && existing.clerkUserId !== auth.userId) continue;
+ const existing = await tx.annotation.findUnique({
+ where: { id: a.id },
+ select: { clerkUserId: true, bookId: true },
+ });
+ if (existing && (existing.clerkUserId !== auth.userId || existing.bookId !== bookId)) continue;🤖 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-next/src/app/api/books/`[bookId]/annotations/route.ts around lines 54 -
77, Update the annotation upsert ownership check around tx.annotation.upsert to
require both clerkUserId and bookId, ensuring an existing annotation id from
another book cannot enter the update branch. Keep the existing update payload
and create behavior unchanged.
| listMeta: (): BookRecord[] => { | ||
| try { | ||
| return JSON.parse(localStorage.getItem(META_KEY) ?? '[]') | ||
| } catch { | ||
| return [] | ||
| } | ||
| }, |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
listMeta can return a non-array value.
The try/catch handles only parse errors. If META_KEY holds valid JSON that is not an array (for example {} or "x"), JSON.parse succeeds and listMeta returns that value typed as BookRecord[]. merge at Line 80 then calls localRecords.map and throws a TypeError, which fails the whole useCloudRestoreMutation flow shown in pwa-next/src/hooks/useCloudRestore.ts Lines 29-31.
Add an Array.isArray guard.
🛡️ Proposed fix
listMeta: (): BookRecord[] => {
try {
- return JSON.parse(localStorage.getItem(META_KEY) ?? '[]')
+ const parsed = JSON.parse(localStorage.getItem(META_KEY) ?? '[]')
+ return Array.isArray(parsed) ? (parsed as BookRecord[]) : []
} catch {
return []
}
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| listMeta: (): BookRecord[] => { | |
| try { | |
| return JSON.parse(localStorage.getItem(META_KEY) ?? '[]') | |
| } catch { | |
| return [] | |
| } | |
| }, | |
| listMeta: (): BookRecord[] => { | |
| try { | |
| const parsed = JSON.parse(localStorage.getItem(META_KEY) ?? '[]') | |
| return Array.isArray(parsed) ? (parsed as BookRecord[]) : [] | |
| } catch { | |
| return [] | |
| } | |
| }, |
🤖 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-next/src/services/bookService.ts` around lines 23 - 29, Update listMeta
to validate the parsed localStorage value with Array.isArray before returning
it; return the existing empty-array fallback for valid non-array JSON as well as
parse failures, while preserving valid BookRecord[] results.
| save: (bookId: string, cfi: string, updatedAt: number = Date.now()): Progress => { | ||
| localStorage.setItem(progressKey(bookId), cfi) | ||
| localStorage.setItem(updatedAtKey(bookId), String(updatedAt)) | ||
| return { bookId, cfi, updatedAt } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Use one authoritative clock for merge decisions.
Line 27 writes a browser timestamp. Line 44 reads a server timestamp. Lines 95-99 compare both values directly. Device clock skew can make stale local progress win over newer remote progress.
Return the server-assigned revision or timestamp from the progress PUT endpoint. Persist that value locally after a successful write. Compare only values from that single authority.
Also applies to: 42-45, 95-99
🤖 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-next/src/services/progressService.ts` around lines 27 - 30, Update save,
the progress PUT flow, and the merge comparison around the affected
progress-service symbols to use the server-assigned revision or timestamp as the
sole ordering value. After a successful PUT, return and persist that server
value instead of Date.now(), ensure reads use the persisted server value, and
compare local versus remote progress only through this shared authoritative
field.
| const pushDebounced = (bookId: string, cfi: string) => { | ||
| pendingCfi.set(bookId, cfi) | ||
| const existing = debounceTimers.get(bookId) | ||
| if (existing) clearTimeout(existing) | ||
| const timer = setTimeout(() => { | ||
| debounceTimers.delete(bookId) | ||
| const latest = pendingCfi.get(bookId) | ||
| pendingCfi.delete(bookId) | ||
| if (latest) remote.pushNow(bookId, latest) | ||
| }, DEBOUNCE_MS) | ||
| debounceTimers.set(bookId, timer) | ||
| } | ||
|
|
||
| const flush = (bookId: string) => { | ||
| const existing = debounceTimers.get(bookId) | ||
| if (existing) clearTimeout(existing) | ||
| debounceTimers.delete(bookId) | ||
| const latest = pendingCfi.get(bookId) | ||
| pendingCfi.delete(bookId) | ||
| if (latest) remote.pushNow(bookId, latest) | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Keep failed writes pending for retry.
Lines 75-77 remove the pending CFI before remote.pushNow completes. Lines 49-56 convert every network failure into false without restoring the pending value. After a transient failure, flushAll cannot retry that update and cloud progress remains stale until another page change occurs.
Keep the dirty record until the API confirms the write. Retry it through an online event or a bounded retry schedule.
Also applies to: 49-56
🤖 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-next/src/services/progressService.ts` around lines 69 - 89, Update
pushDebounced and flush so the pending CFI is retained until remote.pushNow
confirms a successful write, restoring or preserving it when the call fails
instead of deleting it first. Ensure failed writes remain retryable by flushAll
through the existing online event or a bounded retry schedule, while successful
writes clear the dirty record.
| let syncEnabled = false | ||
|
|
||
| export const setSyncEnabled = (enabled: boolean) => { | ||
| syncEnabled = enabled | ||
| } | ||
|
|
||
| export const getSyncEnabled = () => syncEnabled |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Invalidate queued work from a prior session.
Line 6 stores only a Boolean. progressService.remote.pushNow checks it before enqueue, but syncQueue can run the task later. A task accepted for one account can run after the account changes and send the prior account’s reading data with a later session.
Track a sync-session generation. Capture it before enqueueing. Check it again inside every queued task before apiClient sends the request.
Proposed session-generation guard
let syncEnabled = false
+let syncGeneration = 0
export const setSyncEnabled = (enabled: boolean) => {
+ if (syncEnabled !== enabled) syncGeneration += 1
syncEnabled = enabled
}
export const getSyncEnabled = () => syncEnabled
+export const getSyncGeneration = () => syncGeneration- return enqueue(bookId, () =>
- apiClient.put(`/api/books/${bookId}/progress`, { cfi })
+ const generation = getSyncGeneration()
+ return enqueue(bookId, () => {
+ if (!getSyncEnabled() || getSyncGeneration() !== generation) return Promise.resolve(false)
+ return apiClient.put(`/api/books/${bookId}/progress`, { cfi })
.then(() => true)
.catch(() => false),
- )
+ })🤖 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-next/src/services/syncGate.ts` around lines 4 - 10, Replace the
boolean-only state in setSyncEnabled/getSyncEnabled with a sync-session
generation that advances whenever the sync session changes. In
progressService.remote.pushNow, capture the current generation before enqueueing
and have every queued task validate that generation again immediately before
calling apiClient; skip stale tasks so work from a prior account cannot be sent.
| export const enqueue = (key: string, task: () => Promise<boolean>): Promise<boolean> => { | ||
| const next = (queues.get(key) ?? Promise.resolve(true)).then(task) | ||
| queues.set(key, next) | ||
| return next | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
A rejected task permanently disables the queue for that key.
.then(task) attaches no rejection handler. If a task rejects, next is a rejected promise and it is stored in queues. Every later enqueue for the same key chains from that rejected promise, so task never runs and the rejection propagates again. Synchronization for that bookId then stops for the rest of the page session. The rejection is also unhandled, which triggers an unhandledrejection event.
Today every caller catches inside its own task, so the chain stays alive. The guarantee is fragile. One throw outside the caller's .catch (for example a JSON.stringify failure on the request body) is enough to break it.
Chain from a settled promise instead.
🛡️ Proposed fix
export const enqueue = (key: string, task: () => Promise<boolean>): Promise<boolean> => {
- const next = (queues.get(key) ?? Promise.resolve(true)).then(task)
+ const previous = queues.get(key) ?? Promise.resolve(true)
+ // 用 catch 收斂前一棒的失敗，避免整條 key 的佇列被一次 rejection 永久毒化。
+ const next = previous.catch(() => false).then(task)
queues.set(key, next)
return next
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export const enqueue = (key: string, task: () => Promise<boolean>): Promise<boolean> => { | |
| const next = (queues.get(key) ?? Promise.resolve(true)).then(task) | |
| queues.set(key, next) | |
| return next | |
| } | |
| export const enqueue = (key: string, task: () => Promise<boolean>): Promise<boolean> => { | |
| const previous = queues.get(key) ?? Promise.resolve(true) | |
| // 用 catch 收斂前一棒的失敗，避免整條 key 的佇列被一次 rejection 永久毒化。 | |
| const next = previous.catch(() => false).then(task) | |
| queues.set(key, next) | |
| return next | |
| } |
🤖 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-next/src/services/syncQueue.ts` around lines 6 - 10, Update enqueue so
each key’s queue continues from a settled promise when a task rejects, rather
than storing a rejected promise in queues. Keep returning the current task’s
Promise<boolean> so callers still receive its success or failure, while attach
rejection handling to the internal chain to prevent unhandled rejections and
allow later tasks to run.
CLOUD_SYNC_PROGRESS.md 記載的同步機制已被整個取代，加指標連到新文件； 新文件涵蓋整體目標範圍、三層架構摘要、Phase 進度、以及目前擱置中的跨裝置 同步已知問題，供之後任何一次對話接續。 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@CLIENT_SERVICE_HOOK_REFACTOR.md`:
- Around line 57-61: Remove the machine-local plan path from
CLIENT_SERVICE_HOOK_REFACTOR.md lines 57-61 and replace it with one relative
link to the tracked architecture summary, or remove the reference while
preserving the summary. Apply the same change to the duplicate reference at
CLIENT_SERVICE_HOOK_REFACTOR.md line 165; both sites must no longer point to the
local plan file.
- Line 67: Add the text language identifier to the fenced code block in
CLIENT_SERVICE_HOOK_REFACTOR.md by changing the opening fence to ```text, while
leaving the architecture outline content unchanged.
🪄 Autofix
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: f93e1684-2cb4-49ca-a7fb-958f0e52babb
📒 Files selected for processing (2)
CLIENT_SERVICE_HOOK_REFACTOR.mdCLOUD_SYNC_PROGRESS.md
| **完整的架構設計細節（Client/Service/Hook 三層的介面形狀、renderer 深連結認證流程的完整步驟、 | ||
| mobile Clerk Expo 整合方式、Bearer token 後端驗證路徑設計）記在 plan mode 產生的計畫書， | ||
| 路徑：`/Users/yutingchan/.claude/plans/fancy-frolicking-fairy.md`（這是 Claude Code 本機的 | ||
| plan 檔案，不在這個 git repo 裡，換一台機器或換人接手看不到——如果那個檔案未來不見了， | ||
| 下面「三層架構設計」小節有摘要版，足夠接續 renderer/mobile 階段）。** |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Remove the duplicated machine-local plan reference.
Both entries point to a file that other contributors and CI cannot access. Keep the design summary in tracked documentation and use one relative link, or remove both references.
CLIENT_SERVICE_HOOK_REFACTOR.md#L57-L61: replace the local plan path in the architecture summary.CLIENT_SERVICE_HOOK_REFACTOR.md#L165-L165: replace the duplicate local plan path in the related-documents section.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~57-~57: 您的意思是“"不"驟”？
Context: ...Service/Hook 三層的介面形狀、renderer 深連結認證流程的完整步驟、 mobile Clerk Expo 整合方式、Bearer token 後...
(BU)
📍 Affects 1 file
CLIENT_SERVICE_HOOK_REFACTOR.md#L57-L61(this comment)CLIENT_SERVICE_HOOK_REFACTOR.md#L165-L165
🤖 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 `@CLIENT_SERVICE_HOOK_REFACTOR.md` around lines 57 - 61, Remove the
machine-local plan path from CLIENT_SERVICE_HOOK_REFACTOR.md lines 57-61 and
replace it with one relative link to the tracked architecture summary, or remove
the reference while preserving the summary. Apply the same change to the
duplicate reference at CLIENT_SERVICE_HOOK_REFACTOR.md line 165; both sites must
no longer point to the local plan file.
|
|
||
| ## 三層架構設計摘要 | ||
|
|
||
| ``` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add a language identifier to the fenced code block.
markdownlint-cli2 reports MD040 for this fence. Use text because the block contains an architecture outline.
Proposed fix
-```
+```text📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ``` |
🧰 Tools
🪛 markdownlint-cli2 (0.23.2)
[warning] 67-67: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 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 `@CLIENT_SERVICE_HOOK_REFACTOR.md` at line 67, Add the text language identifier
to the fenced code block in CLIENT_SERVICE_HOOK_REFACTOR.md by changing the
opening fence to ```text, while leaving the architecture outline content
unchanged.
Source: Linters/SAST tools
1 participant
Summary by CodeRabbit