fix(reader): add fetchFreshBookDetail method to ensure up-to-date reading position across devices#358
Conversation
…ding position across devices
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughComponents now request fresh book details instead of cached ones and several progress-save endpoints apply cache-patching side effects that update specific book-progress fields in the local QueryClient after successful HTTP posts. Tests were extended to assert these cache interactions. Changes
Sequence Diagram(s)sequenceDiagram
participant Component as Reader Component
participant BookSvc as BookService
participant Server as API Server
participant QueryClient as QueryClient (cache)
Component->>BookSvc: fetchFreshBookDetail(bookId)
BookSvc->>Server: GET /books/:id (staleTime:0)
Server-->>BookSvc: Book detail
BookSvc-->>Component: Book detail
Component->>Server: POST /books/:id/progress { progress }
Server-->>Component: 200 OK
Component->>QueryClient: patchBookFieldsInCache(bookId, progress fields)
QueryClient-->>Component: cache updated
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/src/app/features/readers/audiobook-player/audiobook.service.ts`:
- Around line 51-69: The optimistic cache patch in the AudiobookService
currently writes book-level audiobookProgress even when the request sends only
fileProgress; update the logic in the block that creates request$ (and the
subsequent tap that calls patchBookFieldsInCache) so that when bookFileId is
present you do not patch audiobookProgress at the book level (or alternatively
include audiobookProgress in the POST payload for that branch). In practice,
modify the file-progress branch around request$ and the tap: either add
audiobookProgress to the POST body when bookFileId is set, or conditionally call
patchBookFieldsInCache only for the branch that sent audiobookProgress, leaving
the file-progress branch to avoid writing audiobookProgress into the cache.
Ensure you change the code paths referenced (request$ creation and the tap that
calls patchBookFieldsInCache) to keep frontend cache consistent with
ReadingProgressService file-level storage.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8461f2ba-77c2-4393-9171-625e86e4d80d
📒 Files selected for processing (8)
frontend/src/app/features/book/service/book-patch.service.tsfrontend/src/app/features/book/service/book.service.tsfrontend/src/app/features/readers/audiobook-player/audiobook-player.component.tsfrontend/src/app/features/readers/audiobook-player/audiobook.service.tsfrontend/src/app/features/readers/cbx-reader/cbx-reader.component.scssfrontend/src/app/features/readers/cbx-reader/cbx-reader.component.tsfrontend/src/app/features/readers/ebook-reader/ebook-reader.component.tsfrontend/src/app/features/readers/pdf-reader/pdf-reader.component.ts
💤 Files with no reviewable changes (1)
- frontend/src/app/features/readers/cbx-reader/cbx-reader.component.scss
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Test Suite / Frontend Tests
- GitHub Check: Analyze (java-kotlin)
🧰 Additional context used
📓 Path-based instructions (3)
frontend/src/**/*.{ts,tsx,html,scss}
📄 CodeRabbit inference engine (AGENTS.md)
Use 2-space indentation in TypeScript, HTML, and SCSS files
Files:
frontend/src/app/features/readers/audiobook-player/audiobook-player.component.tsfrontend/src/app/features/readers/pdf-reader/pdf-reader.component.tsfrontend/src/app/features/readers/ebook-reader/ebook-reader.component.tsfrontend/src/app/features/readers/cbx-reader/cbx-reader.component.tsfrontend/src/app/features/book/service/book.service.tsfrontend/src/app/features/book/service/book-patch.service.tsfrontend/src/app/features/readers/audiobook-player/audiobook.service.ts
frontend/src/app/**/*.component.ts
📄 CodeRabbit inference engine (AGENTS.md)
Keep Angular code on standalone components. Do not add NgModules
Files:
frontend/src/app/features/readers/audiobook-player/audiobook-player.component.tsfrontend/src/app/features/readers/pdf-reader/pdf-reader.component.tsfrontend/src/app/features/readers/ebook-reader/ebook-reader.component.tsfrontend/src/app/features/readers/cbx-reader/cbx-reader.component.ts
frontend/src/app/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
frontend/src/app/**/*.{ts,tsx}: Preferinject()over constructor injection
Followfrontend/eslint.config.js: component selectors useapp-*, directive selectors useapp*, andanyis disallowed
Files:
frontend/src/app/features/readers/audiobook-player/audiobook-player.component.tsfrontend/src/app/features/readers/pdf-reader/pdf-reader.component.tsfrontend/src/app/features/readers/ebook-reader/ebook-reader.component.tsfrontend/src/app/features/readers/cbx-reader/cbx-reader.component.tsfrontend/src/app/features/book/service/book.service.tsfrontend/src/app/features/book/service/book-patch.service.tsfrontend/src/app/features/readers/audiobook-player/audiobook.service.ts
🧠 Learnings (1)
📓 Common learnings
Learnt from: pedronave
Repo: grimmory-tools/grimmory PR: 247
File: booklore-api/src/main/java/org/booklore/service/hardcover/HardcoverSyncService.java:156-159
Timestamp: 2026-04-01T17:50:06.817Z
Learning: In grimmory-tools/grimmory (HardcoverSyncService.java), the Hardcover API already returns `user_book_reads` in a consistent, ordered fashion (ascending by recency), so `getLast()` reliably retrieves the most recent read entry. Adding an explicit `order_by` to the `user_book_reads` query is unnecessary.
🔇 Additional comments (6)
frontend/src/app/features/book/service/book.service.ts (1)
123-135: Well-documented and correctly implemented fresh-fetch method.The implementation correctly uses
fetchQuerywithstaleTime: 0to ensure the query always hits the network. The documentation clearly explains the rationale—handling cross-device progress synchronization—which is helpful for future maintainers.frontend/src/app/features/readers/cbx-reader/cbx-reader.component.ts (1)
246-246: Change correctly ensures fresh book details are fetched on reader initialization.The switch from
ensureBookDetailtofetchFreshBookDetailaligns with the PR objective of ensuring up-to-date reading positions across devices.frontend/src/app/features/readers/audiobook-player/audiobook-player.component.ts (1)
181-186: Change correctly ensures fresh book details for audiobook progress restoration.The switch to
fetchFreshBookDetailensures the audiobook player receives the latestaudiobookProgressfrom the server, enabling proper cross-device position synchronization as handled inapplyBookDetails.frontend/src/app/features/readers/ebook-reader/ebook-reader.component.ts (1)
233-233: Change correctly ensures fresh book details for ebook progress restoration.The switch to
fetchFreshBookDetailensures the ebook reader receives the latestepubProgress(CFI and percentage) from the server for accurate cross-device position synchronization.frontend/src/app/features/readers/pdf-reader/pdf-reader.component.ts (1)
119-119: Change correctly ensures fresh book details for PDF progress restoration.The switch to
fetchFreshBookDetailensures the PDF reader receives the latestpdfProgress(page number) from the server for accurate cross-device position synchronization.frontend/src/app/features/book/service/book-patch.service.ts (1)
82-89: This invalidation has no effect because readers don't subscribe to the detail query during reading.
patchBookFieldsInCache()does callinvalidateBookDetailQueries(), but readers fetch the book detail once on initialization viafetchFreshBookDetail()(a one-time Promise call), then work with locally stored data. There are no active observers of the detail query, so invalidating it triggers no background refetch. The invalidation calls are harmless but unnecessary.Also applies to: 137-144, 177-184
> Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
frontend/src/app/features/readers/audiobook-player/audiobook.service.ts (1)
50-67:⚠️ Potential issue | 🔴 CriticalBackend does not persist
audiobookProgressat the book level whenfileProgressis provided.The audiobook progress is stored only in
UserBookFileProgressEntity(file-level). The legacy book-level entity has no audiobook-specific columns. This means the frontend'spatchBookFieldsInCachecall withaudiobookProgresswill cache data that the backend does not persist, causing drift after refresh or across devices.Either:
- Remove the book-level
audiobookProgressfrom the cache patch and rely on file-level progress, or- Update the backend to persist
audiobookProgressat the book level alongside file-level storage.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/app/features/readers/audiobook-player/audiobook.service.ts` around lines 50 - 67, The frontend is caching a book-level audiobookProgress that the backend does not persist when fileProgress is provided; in saveProgress remove the patchBookFieldsInCache call that writes audiobookProgress at the book level and instead update or invalidate the file-level cache entry that represents UserBookFileProgressEntity (use the fileProgress data you send in body.fileProgress or trigger a refetch for the book's progress). Update references in saveProgress (and any callers) to stop writing book-level audiobookProgress and ensure the cache is updated only at the file-level (or refreshed) to prevent drift across devices.
🧹 Nitpick comments (1)
frontend/src/app/features/book/service/book-patch.service.spec.ts (1)
61-61: Use query-key helpers instead of hard-coded detail keys in expectations.Prefer
bookDetailQueryPrefix(bookId)in tests to keep expectations aligned with the canonical key builder and avoid future key-shape drift.♻️ Suggested refactor
import {ReadStatus} from '../model/book.model'; -import {BOOKS_QUERY_KEY} from './book-query-keys'; +import {BOOKS_QUERY_KEY, bookDetailQueryPrefix} from './book-query-keys'; import {BookPatchService} from './book-patch.service'; @@ - expect(queryClient.invalidateQueries).toHaveBeenCalledWith({queryKey: ['books', 'detail', 11]}); + expect(queryClient.invalidateQueries).toHaveBeenCalledWith({queryKey: bookDetailQueryPrefix(11)}); @@ - expect(queryClient.invalidateQueries).toHaveBeenCalledWith({queryKey: ['books', 'detail', 7]}); + expect(queryClient.invalidateQueries).toHaveBeenCalledWith({queryKey: bookDetailQueryPrefix(7)});Also applies to: 87-87
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/app/features/book/service/book-patch.service.spec.ts` at line 61, The test uses a hard-coded query key ['books','detail',11] in the expectation for queryClient.invalidateQueries; replace that literal with the canonical helper bookDetailQueryPrefix(bookId) so the test uses the same key builder as production code (update the expect at the assertion(s) referencing queryClient.invalidateQueries — e.g., the assertion around queryClient.invalidateQueries in book-patch.service.spec.ts and the similar assertion at line ~87 — to call bookDetailQueryPrefix(11) instead of the hard-coded array).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@frontend/src/app/features/readers/audiobook-player/audiobook.service.ts`:
- Around line 50-67: The frontend is caching a book-level audiobookProgress that
the backend does not persist when fileProgress is provided; in saveProgress
remove the patchBookFieldsInCache call that writes audiobookProgress at the book
level and instead update or invalidate the file-level cache entry that
represents UserBookFileProgressEntity (use the fileProgress data you send in
body.fileProgress or trigger a refetch for the book's progress). Update
references in saveProgress (and any callers) to stop writing book-level
audiobookProgress and ensure the cache is updated only at the file-level (or
refreshed) to prevent drift across devices.
---
Nitpick comments:
In `@frontend/src/app/features/book/service/book-patch.service.spec.ts`:
- Line 61: The test uses a hard-coded query key ['books','detail',11] in the
expectation for queryClient.invalidateQueries; replace that literal with the
canonical helper bookDetailQueryPrefix(bookId) so the test uses the same key
builder as production code (update the expect at the assertion(s) referencing
queryClient.invalidateQueries — e.g., the assertion around
queryClient.invalidateQueries in book-patch.service.spec.ts and the similar
assertion at line ~87 — to call bookDetailQueryPrefix(11) instead of the
hard-coded array).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d357699b-b121-4e11-896a-a793f3e2c3d7
📒 Files selected for processing (3)
frontend/src/app/features/book/service/book-patch.service.spec.tsfrontend/src/app/features/readers/audiobook-player/audiobook.service.spec.tsfrontend/src/app/features/readers/audiobook-player/audiobook.service.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (3)
frontend/src/**/*.{ts,tsx,html,scss}
📄 CodeRabbit inference engine (AGENTS.md)
Use 2-space indentation in TypeScript, HTML, and SCSS files
Files:
frontend/src/app/features/book/service/book-patch.service.spec.tsfrontend/src/app/features/readers/audiobook-player/audiobook.service.spec.tsfrontend/src/app/features/readers/audiobook-player/audiobook.service.ts
frontend/src/app/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
frontend/src/app/**/*.{ts,tsx}: Preferinject()over constructor injection
Followfrontend/eslint.config.js: component selectors useapp-*, directive selectors useapp*, andanyis disallowed
Files:
frontend/src/app/features/book/service/book-patch.service.spec.tsfrontend/src/app/features/readers/audiobook-player/audiobook.service.spec.tsfrontend/src/app/features/readers/audiobook-player/audiobook.service.ts
frontend/src/**/*.{test,spec}.ts
📄 CodeRabbit inference engine (AGENTS.md)
Use Vitest for tests in the frontend
Files:
frontend/src/app/features/book/service/book-patch.service.spec.tsfrontend/src/app/features/readers/audiobook-player/audiobook.service.spec.ts
🧠 Learnings (5)
📓 Common learnings
Learnt from: pedronave
Repo: grimmory-tools/grimmory PR: 247
File: booklore-api/src/main/java/org/booklore/service/hardcover/HardcoverSyncService.java:156-159
Timestamp: 2026-04-01T17:50:06.817Z
Learning: In grimmory-tools/grimmory (HardcoverSyncService.java), the Hardcover API already returns `user_book_reads` in a consistent, ordered fashion (ascending by recency), so `getLast()` reliably retrieves the most recent read entry. Adding an explicit `order_by` to the `user_book_reads` query is unnecessary.
📚 Learning: 2026-03-26T01:46:48.863Z
Learnt from: CR
Repo: grimmory-tools/grimmory PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-26T01:46:48.863Z
Learning: Applies to frontend/src/**/*.{test,spec}.ts : Use Vitest for tests in the frontend
Applied to files:
frontend/src/app/features/readers/audiobook-player/audiobook.service.spec.ts
📚 Learning: 2026-04-02T09:25:37.417Z
Learnt from: balazs-szucs
Repo: grimmory-tools/grimmory PR: 334
File: booklore-api/src/main/java/org/booklore/service/reader/EpubReaderService.java:402-407
Timestamp: 2026-04-02T09:25:37.417Z
Learning: In grimmory-tools/grimmory, before commenting on any file processing code (epub or pdf), always verify the current state of the relevant upstream grimmory libraries: `grimmory-tools/epub4j` (for epub) and `grimmory-tools/PDFium4j` (for pdf). These custom libraries may have different APIs, capabilities, and limitations compared to the third-party libraries they replace. Issues about streaming, buffering, or API surface may need to be filed/addressed in those upstream repos rather than in grimmory itself.
Applied to files:
frontend/src/app/features/readers/audiobook-player/audiobook.service.ts
📚 Learning: 2026-03-24T18:46:47.249Z
Learnt from: pedronave
Repo: grimmory-tools/grimmory PR: 173
File: booklore-api/src/main/java/org/booklore/service/hardcover/HardcoverSyncService.java:188-199
Timestamp: 2026-03-24T18:46:47.249Z
Learning: In grimmory-tools/grimmory (HardcoverSyncService.java), when a `hardcoverBookId` is stored in book metadata but is malformed (non-numeric), the preferred behavior is to return null and skip the sync rather than falling back to ISBN-based lookup. The reasoning is that an explicitly set bookId represents deliberate user intent, and silently resolving via ISBN could sync progress to a different book than intended.
Applied to files:
frontend/src/app/features/readers/audiobook-player/audiobook.service.ts
📚 Learning: 2026-03-24T18:58:08.199Z
Learnt from: pedronave
Repo: grimmory-tools/grimmory PR: 173
File: booklore-api/src/main/java/org/booklore/service/hardcover/HardcoverSyncService.java:291-294
Timestamp: 2026-03-24T18:58:08.199Z
Learning: In grimmory-tools/grimmory (HardcoverSyncService.java), when a Hardcover edition has no page count, falling back to the book-level page count (book.pages) is the preferred behavior. Skipping the sync entirely in this case is considered worse than using a slightly approximate total, because the page count is only a proxy for converting progress % to page numbers for the API. The Hardcover API allows setting page-based progress even when the edition lacks a page count, and progress displays in at least some UI views (e.g., the book detail dropdown).
Applied to files:
frontend/src/app/features/readers/audiobook-player/audiobook.service.ts
🔇 Additional comments (8)
frontend/src/app/features/book/service/book-patch.service.spec.ts (1)
60-60: Good coverage added for optimistic cache updates.These assertions are valuable—they verify cache patching behavior directly after progress handling, which tightens regression protection.
Also applies to: 86-86
frontend/src/app/features/readers/audiobook-player/audiobook.service.ts (2)
1-10: LGTM!The new imports are correctly structured:
tapfromrxjs/operatorsfor the side-effect operator,QueryClientfrom@tanstack/angular-query-experimentalfor cache access, andpatchBookFieldsInCachefrom the local book-query-cache module.
21-21: LGTM!
QueryClientis correctly injected usinginject(), following the project's coding guidelines.frontend/src/app/features/readers/audiobook-player/audiobook.service.spec.ts (5)
14-21: LGTM!Good test setup: creating a fresh
QueryClientper test, spying onsetQueryDataandinvalidateQueriesto verify cache interactions. This allows the tests to confirm that the service correctly patches the cache after saving progress.
28-29: LGTM!Correctly providing the
QueryClientinstance to the TestBed so the service can inject it.
37-42: LGTM!Proper cleanup:
queryClient.clear()ensures no cache state leaks between tests, andvi.restoreAllMocks()resets all spies.
71-93: LGTM!The test correctly verifies that:
- When no
bookFileIdis provided, onlyaudiobookProgressis included in the payload- After the request completes, the cache is patched via
setQueryDatawith the['books']query keyThe assertion on line 92 confirms the cache-patching side effect works as expected.
95-119: LGTM!The test correctly verifies that when
bookFileIdis present:
- Both
audiobookProgressandfileProgressare included in the POST payload- The
fileProgressobject contains the expected transformed values (positionDataas string,positionHrefas string,progressPercent)- The cache is patched after the request completes
This aligns with the updated implementation in
audiobook.service.ts.
…ding position across devices (grimmory-tools#358) * fix(reader): add fetchFreshBookDetail method to ensure up-to-date reading position across devices * test(audiobook): enhance tests to verify queryClient interactions for progress updates * fix(audiobook): include audiobookProgress and fileProgress in progress payload
…ding position across devices (grimmory-tools#358) * fix(reader): add fetchFreshBookDetail method to ensure up-to-date reading position across devices * test(audiobook): enhance tests to verify queryClient interactions for progress updates * fix(audiobook): include audiobookProgress and fileProgress in progress payload
…ding position across devices (#358) * fix(reader): add fetchFreshBookDetail method to ensure up-to-date reading position across devices * test(audiobook): enhance tests to verify queryClient interactions for progress updates * fix(audiobook): include audiobookProgress and fileProgress in progress payload
…ding position across devices (#358) * fix(reader): add fetchFreshBookDetail method to ensure up-to-date reading position across devices * test(audiobook): enhance tests to verify queryClient interactions for progress updates * fix(audiobook): include audiobookProgress and fileProgress in progress payload
…ding position across devices (grimmory-tools#358) * fix(reader): add fetchFreshBookDetail method to ensure up-to-date reading position across devices * test(audiobook): enhance tests to verify queryClient interactions for progress updates * fix(audiobook): include audiobookProgress and fileProgress in progress payload
Description
Linked Issue: Fixes #
Related to: #162
Changes
This pull request improves how book progress updates are handled and ensures that reader components always display the latest reading position, even if it was updated on another device (or another client). The main changes include updating the cache optimistically after saving progress and adding a method to force-fetch book details from the server, which is now used by all reader components. Additionally, some minor cleanups were made in SCSS files. (linter was complaining... 🤷♂️ )
Summary by CodeRabbit
Release Notes
Bug Fixes
Improvements
Tests
Style