fix(progress-sync): web reader to koreader progress sync#1064
fix(progress-sync): web reader to koreader progress sync#1064wesamjabali wants to merge 5 commits intogrimmory-tools:developfrom
Conversation
WalkthroughReadingProgressService now calls ChangesKOReader Progress Synchronization
Sequence DiagramsequenceDiagram
participant Client
participant RPS as ReadingProgressService
participant KRS as KoreaderService
participant DB as Database
Client->>RPS: updateReadProgress(request)
activate RPS
Note over RPS: compute progress & percentage
alt percentage != null
RPS->>KRS: syncProgressToKoreader(bookId, percentage, userId)
activate KRS
KRS->>DB: resolve Koreader user & book & progress record
DB-->>KRS: user/book/progress
alt KOReader sync enabled for user/book
KRS->>DB: upsert progress (koreaderProgressPercent, koreaderLastSyncTime)
alt primary file is EPUB and epubProgress exists
KRS->>KRS: convert EPUB CFI → KOReader XPointer
KRS->>DB: save koreaderProgress (xpointer) on success
end
end
DB-->>KRS: saved
deactivate KRS
end
RPS->>RPS: continue other syncs (hardcover...)
deactivate RPS
RPS-->>Client: response
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.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: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@backend/src/main/java/org/booklore/service/progress/ReadingProgressService.java`:
- Around line 514-516: ReadingProgressService currently only checks
KoreaderUserEntity::isSyncWithBookloreReader when deciding to sync, so it still
performs syncs even if the global KOReader sync toggle is disabled; update the
logic around koreaderUserRepository.findByBookLoreUserId(user.getId()) to also
honor the global toggle by either adding an additional filter that checks the
global flag (e.g., settings.isKoreaderSyncEnabled() or
syncConfig.isSyncEnabled()) before calling ifPresent, or by wrapping the
ifPresent body with an early return when the global toggle is false; reference
the existing koreaderUserRepository.findByBookLoreUserId(...) call and the
KoreaderUserEntity::isSyncWithBookloreReader predicate when making the change.
- Around line 528-530: The catch block inside ReadingProgressService that
currently does log.warn("Failed to convert CFI to XPointer for KOReader sync:
{}", e.getMessage()) should log the exception object so the stack trace and full
context are preserved; update the catch to call the logger with the exception
(e.g., pass e as the throwable parameter) or include both message and throwable
so the log shows the error text plus stack trace for better diagnostics.
🪄 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: Repository YAML (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3120b86d-4028-4d35-a718-4c533c9c0646
📒 Files selected for processing (2)
backend/src/main/java/org/booklore/service/progress/ReadingProgressService.javabackend/src/test/java/org/booklore/service/progress/ReadingProgressServiceTest.java
📜 Review details
🧰 Additional context used
📓 Path-based instructions (5)
backend/src/**/*.java
📄 CodeRabbit inference engine (AGENTS.md)
backend/src/**/*.java: Use 4-space indentation and match surrounding Java style in backend code
Prefer constructor injection via Lombok patterns already used in the codebase. Do not introduce@Autowiredfield injection in backend code
Use MapStruct for entity/DTO mapping in backend code
Keep JPA entities on the *Entity suffix in backend code
Files:
backend/src/test/java/org/booklore/service/progress/ReadingProgressServiceTest.javabackend/src/main/java/org/booklore/service/progress/ReadingProgressService.java
backend/src/test/**/*.java
📄 CodeRabbit inference engine (AGENTS.md)
Prefer focused unit tests; use
@SpringBootTestonly when the Spring context is required in backend code
Files:
backend/src/test/java/org/booklore/service/progress/ReadingProgressServiceTest.java
**/*
⚙️ CodeRabbit configuration file
**/*: This project is being developed using current and future-facing technologies:
- Java 25 with --enable-preview (preview features are INTENTIONAL and encouraged)
- Spring Boot 4 (latest major version, check APIs accordingly)
- Jackson 3 (new package: tools.jackson.* instead of com.fasterxml.jackson.*)
- Hibernate 7.3.x (Jakarta Persistence 3.2, new APIs; avoid deprecated Hibernate 5/6 patterns)
- Angular 21 (signals-based reactivity, no NgModules unless legacy)
Grimmory Internal Tools
- epub4j and pdfium4j are our own internal tools developed by the Grimmory team.
- Always verify behavior and API changes against the upstream repositories:
- If you encounter issues with these libraries, check if a fix exists in the upstream grimmory-tools organization.
Metadata Standards and Compliance
- For all metadata writing and parsing logic, double-check against Dublin Core and ANSI standards to ensure perfect official compliance.
- We strictly follow the widespread and official XML-compliant methods for EPUB2, EPUB3, CBX, and PDF formats.
General Java and Spring rules
- ALWAYS prefer modern, idiomatic Java 25 constructs over legacy patterns.
- Preview features (--enable-preview) are enabled and intentional; do NOT flag them as risky unless there is a concrete runtime issue.
- Prefer: records, sealed classes/interfaces, pattern matching (switch expressions, instanceof), structured concurrency (StructuredTaskScope), scoped values, string templates, unnamed patterns/variables.
- Prefer virtual threads (Thread.ofVirtual(), Executors.newVirtualThreadPerTaskExecutor()) over platform threads for I/O-bound work.
- Prefer the new Sequenced Collections API (SequencedCollection, SequencedMap) where applicable.
- Prefer
varfor local variables when the type is obvious from context.- Use stream().toList() instead of stream().collect(Collectors.toList()) for imm...
Files:
backend/src/test/java/org/booklore/service/progress/ReadingProgressServiceTest.javabackend/src/main/java/org/booklore/service/progress/ReadingProgressService.java
**/service/**/*.java
⚙️ CodeRabbit configuration file
**/service/**/*.java: Spring Framework 7 service layer review:
- Flag missing
@Transactionalon methods that perform multiple writes.- Prefer constructor injection over
@Autowiredfield injection. Use@AllArgsConstructor.- Use ApiError enum for throwing exceptions.
- Flag checked exceptions swallowed silently; must log or rethrow.
- Flag Thread.sleep(); prefer Duration-based overloads or ScheduledExecutorService.
- Prefer virtual threads (Thread.ofVirtual()) for I/O-bound operations.
- Flag mutable shared state in singleton beans.
Files:
backend/src/test/java/org/booklore/service/progress/ReadingProgressServiceTest.javabackend/src/main/java/org/booklore/service/progress/ReadingProgressService.java
**/*Test.java
⚙️ CodeRabbit configuration file
**/*Test.java: Java test review:
- Prefer
@ExtendWith(SpringExtension.class) or@SpringBootTestfor integration tests.- Flag tests with no assertions.
- Flag Thread.sleep() in tests; use Awaitility or virtual-thread-friendly alternatives.
- Flag hardcoded ports or file paths.
- Flag missing edge case coverage: null, empty, boundary values.
- Prefer AssertJ over JUnit's built-in assertions for readability.
- Prefer
@Sqlor Testcontainers for database state; not hand-rolled setup/teardown.
Files:
backend/src/test/java/org/booklore/service/progress/ReadingProgressServiceTest.java
🧠 Learnings (3)
📚 Learning: 2026-04-10T08:15:37.436Z
Learnt from: imnotjames
Repo: grimmory-tools/grimmory PR: 449
File: booklore-api/src/main/java/org/booklore/service/book/BookDownloadService.java:139-145
Timestamp: 2026-04-10T08:15:37.436Z
Learning: When using Spring `ContentDisposition.builder(...).filename(name, StandardCharsets.UTF_8).build()` (i.e., explicitly providing UTF-8), the resulting header value should include both the quoted `filename="=?UTF-8?..."` and the RFC 5987 `filename*=` parameters. In this case, any extra ASCII fallback computation (e.g., deriving an ASCII `fallbackFilename` via `NON_ASCII_PATTERN` and calling `.filename(fallbackFilename)`) is likely redundant—prefer calling only `.filename(fallbackName?, StandardCharsets.UTF_8)` as appropriate and let Spring handle the UTF-8 header parameters. Verify by comparing the emitted header for `filename` and `filename*` before deciding to keep an ASCII fallback.
Applied to files:
backend/src/test/java/org/booklore/service/progress/ReadingProgressServiceTest.javabackend/src/main/java/org/booklore/service/progress/ReadingProgressService.java
📚 Learning: 2026-04-14T12:43:08.698Z
Learnt from: balazs-szucs
Repo: grimmory-tools/grimmory PR: 502
File: booklore-api/src/main/java/org/booklore/service/reader/ChapterCacheService.java:0-0
Timestamp: 2026-04-14T12:43:08.698Z
Learning: For this codebase (booklore-api), target Java 25 with `--enable-preview`, so `_` is intentionally used as an unnamed/ignored variable (e.g., lambda parameter or pattern variable) per Java’s preview feature JEP 456. Do not flag `_` in those contexts as an invalid/reserved identifier; only flag it if it’s used in a non-supported position (e.g., where an unnamed variable is not applicable for the Java preview rules).
Applied to files:
backend/src/test/java/org/booklore/service/progress/ReadingProgressServiceTest.javabackend/src/main/java/org/booklore/service/progress/ReadingProgressService.java
📚 Learning: 2026-04-27T15:25:55.042Z
Learnt from: imnotjames
Repo: grimmory-tools/grimmory PR: 930
File: backend/src/test/java/org/booklore/service/metadata/parser/ComicvineBookParserTest.java:68-75
Timestamp: 2026-04-27T15:25:55.042Z
Learning: In this repository’s JUnit 5 test sources (e.g., under backend/src/test/java/), do not flag “bare” `assert` statements as a bug. The project test runner is configured to always execute tests with assertions enabled (e.g., `-ea`), so `assert` behavior is consistent. Continue to review for correctness, but don’t treat unguarded `assert` usage in test classes as a static-analysis issue.
Applied to files:
backend/src/test/java/org/booklore/service/progress/ReadingProgressServiceTest.java
🔀 Multi-repo context grimmory-tools/grimmory-docs
Findings for grimmory-docs
-
KOReader integration docs describe bidirectional sync, and explicitly mention converting between KOReader XPointer and EPUB CFI when "Sync reading progress with Grimmory EBook Reader" is enabled: docs/integration/koreader.md (intro & steps, e.g. lines explaining conversion and caveats). [::grimmory-tools/grimmory-docs::]
-
Devices and Users docs reference UI/settings that control KOReader sync per-user and per-device (enable toggle, API path, username/password, and "KOReader Sync" user permission): docs/tools/devices.md, docs/tools/user-management.md. These confirm the code change should respect per-user sync settings. [::grimmory-tools/grimmory-docs::]
-
Docs show UX expectations for KOReader progress display (KOReader Progress on book detail page) and reset behavior, implying new koreaderProgress/koreaderLastSyncTime fields are surfaced in the UI: docs/integration/koreader.md and referenced images. [::grimmory-tools/grimmory-docs::]
-
Search across this repo found no code references to EpubCfiService, KoreaderUserRepository, convertCfiToProgressXPointer, or koreaderProgress identifiers — only documentation and images. This suggests the new Java service/classes are internal/backend changes; ensure API/DTOs and database fields used by front-end and any other services (e.g., the AppBookService mentioned in related PRs) remain compatible. [::grimmory-tools/grimmory-docs::]
Conclusion: Documentation aligns with the PR intent (syncing KOReader and converting CFI↔XPointer). No other repositories contained code consumers in this search. Recommend verifying backend-to-frontend DTOs / database schema for koreader progress fields and that any other backend modules (e.g., AppBookService in related PRs) consume the updated progress fields consistently.
🔇 Additional comments (1)
backend/src/test/java/org/booklore/service/progress/ReadingProgressServiceTest.java (1)
296-390: Nice coverage for KOReader sync on/off behavior.These tests validate both state mutation and conversion call/no-call behavior, which gives good regression protection for the new sync helper.
|
Please include:
|
|
Hey there, thanks for responding to this and the other PR so quickly. CodeRabbit actually made a couple good action items I'm working out now.
Kosync is an existing feature that promises 2 way sync, but it currently only syncs from the koreader app to grimmory and not the other way around. There have been multiple people complaining of this, myself one of them, and this PR fixes that issue.
Created a new functoin called
|
…pass through. add tests.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@backend/src/test/java/org/booklore/service/progress/ReadingProgressServiceTest.java`:
- Around line 296-348: Add a new unit test (e.g.,
updateReadProgress_shouldSyncPercentButNotXPointerWhenCfiConversionFails) that
reuses the same Book/BookFile/BookLoreUser/KoreaderUser setup from
updateReadProgress_shouldSyncToKoreaderWhenEnabled, but mock
epubCfiService.convertCfiToProgressXPointer(any(Path.class), eq("cfi")) to throw
a RuntimeException; call readingProgressService.updateReadProgress(req) and
assert that progress.getKoreaderProgressPercent() equals 1.0f,
progress.getKoreaderProgress() is null, and progress.getKoreaderLastSyncTime()
is not null, and verify convertCfiToProgressXPointer was invoked.
- Around line 370-373: The test shouldNotSyncToKoreaderWhenDisabled currently
leaves KoreaderUserEntity.syncEnabled at its default false so the global filter
in syncKoreaderFromBookloreProgress (isSyncEnabled) short-circuits before the
per-user check; update the KoreaderUserEntity.builder() invocation in that test
to explicitly set syncEnabled(true) while keeping syncWithBookloreReader(false)
so the test isolates the per-user toggle (refer to KoreaderUserEntity.builder(),
syncWithBookloreReader, syncEnabled, syncKoreaderFromBookloreProgress and the
koreaderUserRepository.findByBookLoreUserId stub).
🪄 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: Repository YAML (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: b8e7c5ee-5054-43bf-9d36-f2599e77e64a
📒 Files selected for processing (2)
backend/src/main/java/org/booklore/service/progress/ReadingProgressService.javabackend/src/test/java/org/booklore/service/progress/ReadingProgressServiceTest.java
📜 Review details
🧰 Additional context used
📓 Path-based instructions (5)
backend/src/**/*.java
📄 CodeRabbit inference engine (AGENTS.md)
backend/src/**/*.java: Use 4-space indentation and match surrounding Java style in backend code
Prefer constructor injection via Lombok patterns already used in the codebase. Do not introduce@Autowiredfield injection in backend code
Use MapStruct for entity/DTO mapping in backend code
Keep JPA entities on the *Entity suffix in backend code
Files:
backend/src/main/java/org/booklore/service/progress/ReadingProgressService.javabackend/src/test/java/org/booklore/service/progress/ReadingProgressServiceTest.java
**/*
⚙️ CodeRabbit configuration file
**/*: This project is being developed using current and future-facing technologies:
- Java 25 with --enable-preview (preview features are INTENTIONAL and encouraged)
- Spring Boot 4 (latest major version, check APIs accordingly)
- Jackson 3 (new package: tools.jackson.* instead of com.fasterxml.jackson.*)
- Hibernate 7.3.x (Jakarta Persistence 3.2, new APIs; avoid deprecated Hibernate 5/6 patterns)
- Angular 21 (signals-based reactivity, no NgModules unless legacy)
Grimmory Internal Tools
- epub4j and pdfium4j are our own internal tools developed by the Grimmory team.
- Always verify behavior and API changes against the upstream repositories:
- If you encounter issues with these libraries, check if a fix exists in the upstream grimmory-tools organization.
Metadata Standards and Compliance
- For all metadata writing and parsing logic, double-check against Dublin Core and ANSI standards to ensure perfect official compliance.
- We strictly follow the widespread and official XML-compliant methods for EPUB2, EPUB3, CBX, and PDF formats.
General Java and Spring rules
- ALWAYS prefer modern, idiomatic Java 25 constructs over legacy patterns.
- Preview features (--enable-preview) are enabled and intentional; do NOT flag them as risky unless there is a concrete runtime issue.
- Prefer: records, sealed classes/interfaces, pattern matching (switch expressions, instanceof), structured concurrency (StructuredTaskScope), scoped values, string templates, unnamed patterns/variables.
- Prefer virtual threads (Thread.ofVirtual(), Executors.newVirtualThreadPerTaskExecutor()) over platform threads for I/O-bound work.
- Prefer the new Sequenced Collections API (SequencedCollection, SequencedMap) where applicable.
- Prefer
varfor local variables when the type is obvious from context.- Use stream().toList() instead of stream().collect(Collectors.toList()) for imm...
Files:
backend/src/main/java/org/booklore/service/progress/ReadingProgressService.javabackend/src/test/java/org/booklore/service/progress/ReadingProgressServiceTest.java
**/service/**/*.java
⚙️ CodeRabbit configuration file
**/service/**/*.java: Spring Framework 7 service layer review:
- Flag missing
@Transactionalon methods that perform multiple writes.- Prefer constructor injection over
@Autowiredfield injection. Use@AllArgsConstructor.- Use ApiError enum for throwing exceptions.
- Flag checked exceptions swallowed silently; must log or rethrow.
- Flag Thread.sleep(); prefer Duration-based overloads or ScheduledExecutorService.
- Prefer virtual threads (Thread.ofVirtual()) for I/O-bound operations.
- Flag mutable shared state in singleton beans.
Files:
backend/src/main/java/org/booklore/service/progress/ReadingProgressService.javabackend/src/test/java/org/booklore/service/progress/ReadingProgressServiceTest.java
backend/src/test/**/*.java
📄 CodeRabbit inference engine (AGENTS.md)
Prefer focused unit tests; use
@SpringBootTestonly when the Spring context is required in backend code
Files:
backend/src/test/java/org/booklore/service/progress/ReadingProgressServiceTest.java
**/*Test.java
⚙️ CodeRabbit configuration file
**/*Test.java: Java test review:
- Prefer
@ExtendWith(SpringExtension.class) or@SpringBootTestfor integration tests.- Flag tests with no assertions.
- Flag Thread.sleep() in tests; use Awaitility or virtual-thread-friendly alternatives.
- Flag hardcoded ports or file paths.
- Flag missing edge case coverage: null, empty, boundary values.
- Prefer AssertJ over JUnit's built-in assertions for readability.
- Prefer
@Sqlor Testcontainers for database state; not hand-rolled setup/teardown.
Files:
backend/src/test/java/org/booklore/service/progress/ReadingProgressServiceTest.java
🧠 Learnings (3)
📚 Learning: 2026-04-10T08:15:37.436Z
Learnt from: imnotjames
Repo: grimmory-tools/grimmory PR: 449
File: booklore-api/src/main/java/org/booklore/service/book/BookDownloadService.java:139-145
Timestamp: 2026-04-10T08:15:37.436Z
Learning: When using Spring `ContentDisposition.builder(...).filename(name, StandardCharsets.UTF_8).build()` (i.e., explicitly providing UTF-8), the resulting header value should include both the quoted `filename="=?UTF-8?..."` and the RFC 5987 `filename*=` parameters. In this case, any extra ASCII fallback computation (e.g., deriving an ASCII `fallbackFilename` via `NON_ASCII_PATTERN` and calling `.filename(fallbackFilename)`) is likely redundant—prefer calling only `.filename(fallbackName?, StandardCharsets.UTF_8)` as appropriate and let Spring handle the UTF-8 header parameters. Verify by comparing the emitted header for `filename` and `filename*` before deciding to keep an ASCII fallback.
Applied to files:
backend/src/main/java/org/booklore/service/progress/ReadingProgressService.javabackend/src/test/java/org/booklore/service/progress/ReadingProgressServiceTest.java
📚 Learning: 2026-04-14T12:43:08.698Z
Learnt from: balazs-szucs
Repo: grimmory-tools/grimmory PR: 502
File: booklore-api/src/main/java/org/booklore/service/reader/ChapterCacheService.java:0-0
Timestamp: 2026-04-14T12:43:08.698Z
Learning: For this codebase (booklore-api), target Java 25 with `--enable-preview`, so `_` is intentionally used as an unnamed/ignored variable (e.g., lambda parameter or pattern variable) per Java’s preview feature JEP 456. Do not flag `_` in those contexts as an invalid/reserved identifier; only flag it if it’s used in a non-supported position (e.g., where an unnamed variable is not applicable for the Java preview rules).
Applied to files:
backend/src/main/java/org/booklore/service/progress/ReadingProgressService.javabackend/src/test/java/org/booklore/service/progress/ReadingProgressServiceTest.java
📚 Learning: 2026-04-27T15:25:55.042Z
Learnt from: imnotjames
Repo: grimmory-tools/grimmory PR: 930
File: backend/src/test/java/org/booklore/service/metadata/parser/ComicvineBookParserTest.java:68-75
Timestamp: 2026-04-27T15:25:55.042Z
Learning: In this repository’s JUnit 5 test sources (e.g., under backend/src/test/java/), do not flag “bare” `assert` statements as a bug. The project test runner is configured to always execute tests with assertions enabled (e.g., `-ea`), so `assert` behavior is consistent. Continue to review for correctness, but don’t treat unguarded `assert` usage in test classes as a static-analysis issue.
Applied to files:
backend/src/test/java/org/booklore/service/progress/ReadingProgressServiceTest.java
🔀 Multi-repo context grimmory-tools/grimmory-docs
Findings for grimmory-tools/grimmory-docs
-
KOReader integration docs confirm bidirectional sync and describe converting KOReader XPointer <-> EPUB CFI when "Sync reading progress with Grimmory eBook Reader" is enabled; docs explicitly mention approximate/chapter-level conversion and that KOReader progress is shown as "KOReader Progress" in the UI. [::grimmory-tools/grimmory-docs::docs/integration/koreader.md:1-8, 69-73]
-
Devices and User Management docs describe per-user KOReader sync toggles, credentials, and permissions (KOReader Sync), matching the PR's behavior of respecting per-user and global sync flags. [::grimmory-tools/grimmory-docs::docs/tools/devices.md:9-34, docs/tools/user-management.md:107-116]
-
Images and UI text in docs indicate frontend surfaces for KOReader progress (percentage, reset icon, last-sync), implying backend adds (koreaderProgress, koreaderLastSyncTime, etc.) should be exposed to the UI/DTOs. [::grimmory-tools/grimmory-docs::docs/integration/koreader.md:69-73]
-
Search across this repo found no code references to EpubCfiService, KoreaderUserRepository, convertCfiToProgressXPointer, or the specific backend identifiers introduced in the PR — only documentation and images. This suggests the changes are backend-only and the docs are consistent with the PR intent, but reviewers should confirm backend DTOs/database schema are compatible with the frontend docs and any UI consumers. [::grimmory-tools/grimmory-docs::]
Conclusion: Documentation aligns with the PR's intent (two-way KOReader sync and CFI/XPointer conversion). No code consumers in this docs repo directly reference the new backend classes/methods, but ensure the new fields are surfaced to any frontend components or APIs that render KOReader progress.
🔇 Additional comments (3)
backend/src/main/java/org/booklore/service/progress/ReadingProgressService.java (3)
510-516: Both sync-flag guards are correctly in place.
isSyncEnabledandisSyncWithBookloreReaderare both chained as filters beforeifPresent, fully resolving the previously-raised concern.
529-531: Exception context is now correctly logged.The throwable
eis passed as the second argument tolog.warn, preserving the full stack trace in production logs — resolving the previously-raised concern.
526-527: No issue found.BookEntity.getFullFilePath()returnsjava.nio.file.Path, which correctly matches thePathoverload ofEpubCfiService.convertCfiToProgressXPointer. The test stub at line 329 (any(Path.class)) matches the actual argument type, andkoreaderProgresswill be properly set.
| @Test | ||
| void updateReadProgress_shouldSyncToKoreaderWhenEnabled() { | ||
| long bookId = 1L; | ||
| BookEntity book = new BookEntity(); | ||
| book.setId(bookId); | ||
|
|
||
| LibraryPathEntity libraryPath = new LibraryPathEntity(); | ||
| libraryPath.setPath("/library"); | ||
| book.setLibraryPath(libraryPath); | ||
|
|
||
| BookFileEntity primaryFile = new BookFileEntity(); | ||
| primaryFile.setId(1L); | ||
| primaryFile.setBook(book); | ||
| primaryFile.setBookType(BookFileType.EPUB); | ||
| primaryFile.setFileName("book.epub"); | ||
| primaryFile.setFileSubPath("subdir"); | ||
| book.setBookFiles(List.of(primaryFile)); | ||
|
|
||
| BookLoreUser user = mock(BookLoreUser.class); | ||
| when(user.getId()).thenReturn(2L); | ||
| when(authenticationService.getAuthenticatedUser()).thenReturn(user); | ||
| when(bookRepository.findByIdWithBookFiles(bookId)).thenReturn(Optional.of(book)); | ||
|
|
||
| BookLoreUserEntity userEntity = new BookLoreUserEntity(); | ||
| userEntity.setId(2L); | ||
| when(userRepository.findById(2L)).thenReturn(Optional.of(userEntity)); | ||
|
|
||
| KoreaderUserEntity koreaderUser = KoreaderUserEntity.builder() | ||
| .syncEnabled(true) | ||
| .syncWithBookloreReader(true) | ||
| .build(); | ||
| when(koreaderUserRepository.findByBookLoreUserId(2L)).thenReturn(Optional.of(koreaderUser)); | ||
|
|
||
| when(epubCfiService.convertCfiToProgressXPointer(any(Path.class), eq("cfi"))) | ||
| .thenReturn("/6/8!/4/2/6/1:15"); | ||
|
|
||
| UserBookProgressEntity progress = new UserBookProgressEntity(); | ||
| when(userBookProgressRepository.findByUserIdAndBookId(2L, bookId)).thenReturn(Optional.of(progress)); | ||
|
|
||
| when(userBookFileProgressRepository.findByUserIdAndBookFileId(2L, 1L)).thenReturn(Optional.empty()); | ||
|
|
||
| ReadProgressRequest req = new ReadProgressRequest(); | ||
| req.setBookId(bookId); | ||
| EpubProgress epubProgress = EpubProgress.builder().cfi("cfi").percentage(100f).build(); | ||
| req.setEpubProgress(epubProgress); | ||
|
|
||
| readingProgressService.updateReadProgress(req); | ||
|
|
||
| assertEquals(1.0f, progress.getKoreaderProgressPercent()); | ||
| assertEquals("/6/8!/4/2/6/1:15", progress.getKoreaderProgress()); | ||
| assertNotNull(progress.getKoreaderLastSyncTime()); | ||
| verify(epubCfiService).convertCfiToProgressXPointer(any(Path.class), eq("cfi")); | ||
| } |
There was a problem hiding this comment.
Missing test for the CFI-conversion exception path.
When epubCfiService.convertCfiToProgressXPointer throws, the service logs a warning and continues — so koreaderProgressPercent and koreaderLastSyncTime should still be written, but koreaderProgress should remain null. This partial-sync behaviour isn't covered by any of the three new tests.
✅ Suggested test skeleton
`@Test`
void updateReadProgress_shouldSyncPercentButNotXPointerWhenCfiConversionFails() {
// same book/user/koreaderUser setup as shouldSyncToKoreaderWhenEnabled...
when(epubCfiService.convertCfiToProgressXPointer(any(Path.class), eq("cfi")))
.thenThrow(new RuntimeException("conversion failed"));
// ... fire updateReadProgress ...
assertEquals(1.0f, progress.getKoreaderProgressPercent());
assertNull(progress.getKoreaderProgress()); // XPointer not set on failure
assertNotNull(progress.getKoreaderLastSyncTime()); // sync time still recorded
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@backend/src/test/java/org/booklore/service/progress/ReadingProgressServiceTest.java`
around lines 296 - 348, Add a new unit test (e.g.,
updateReadProgress_shouldSyncPercentButNotXPointerWhenCfiConversionFails) that
reuses the same Book/BookFile/BookLoreUser/KoreaderUser setup from
updateReadProgress_shouldSyncToKoreaderWhenEnabled, but mock
epubCfiService.convertCfiToProgressXPointer(any(Path.class), eq("cfi")) to throw
a RuntimeException; call readingProgressService.updateReadProgress(req) and
assert that progress.getKoreaderProgressPercent() equals 1.0f,
progress.getKoreaderProgress() is null, and progress.getKoreaderLastSyncTime()
is not null, and verify convertCfiToProgressXPointer was invoked.
| KoreaderUserEntity koreaderUser = KoreaderUserEntity.builder() | ||
| .syncWithBookloreReader(false) | ||
| .build(); | ||
| when(koreaderUserRepository.findByBookLoreUserId(2L)).thenReturn(Optional.of(koreaderUser)); |
There was a problem hiding this comment.
shouldNotSyncToKoreaderWhenDisabled doesn't isolate syncWithBookloreReader=false.
syncEnabled is not explicitly set in the builder, so it defaults to false via @Builder.Default. The filter chain in syncKoreaderFromBookloreProgress evaluates isSyncEnabled first — the entity is filtered out before isSyncWithBookloreReader is ever checked. The test therefore only covers the syncEnabled=false scenario (the same thing shouldNotSyncToKoreaderWhenGlobalSyncDisabled is intended to cover), not the per-user toggle in isolation.
🔧 Fix: explicitly enable global sync to isolate the per-user toggle
KoreaderUserEntity koreaderUser = KoreaderUserEntity.builder()
+ .syncEnabled(true)
.syncWithBookloreReader(false)
.build();📝 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.
| KoreaderUserEntity koreaderUser = KoreaderUserEntity.builder() | |
| .syncWithBookloreReader(false) | |
| .build(); | |
| when(koreaderUserRepository.findByBookLoreUserId(2L)).thenReturn(Optional.of(koreaderUser)); | |
| KoreaderUserEntity koreaderUser = KoreaderUserEntity.builder() | |
| .syncEnabled(true) | |
| .syncWithBookloreReader(false) | |
| .build(); | |
| when(koreaderUserRepository.findByBookLoreUserId(2L)).thenReturn(Optional.of(koreaderUser)); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@backend/src/test/java/org/booklore/service/progress/ReadingProgressServiceTest.java`
around lines 370 - 373, The test shouldNotSyncToKoreaderWhenDisabled currently
leaves KoreaderUserEntity.syncEnabled at its default false so the global filter
in syncKoreaderFromBookloreProgress (isSyncEnabled) short-circuits before the
per-user check; update the KoreaderUserEntity.builder() invocation in that test
to explicitly set syncEnabled(true) while keeping syncWithBookloreReader(false)
so the test isolates the per-user toggle (refer to KoreaderUserEntity.builder(),
syncWithBookloreReader, syncEnabled, syncKoreaderFromBookloreProgress and the
koreaderUserRepository.findByBookLoreUserId stub).
imnotjames
changed the title
imnotjames
changed the title
| progress.setDateFinished(request.getDateFinished()); | ||
| } | ||
|
|
||
| syncKoreaderFromBookloreProgress(userEntity, book, progress, percentage); |
There was a problem hiding this comment.
Looks like if you moved this down to line 278 you could avoid using Float and could have java auto-unbox the Float to float.
This would remove an extra if branch in your syncKoreaderFromBookloreProgress helper
|
|
||
| private void syncKoreaderFromBookloreProgress(BookLoreUserEntity user, BookEntity book, | ||
| UserBookProgressEntity progress, Float percentage) { | ||
| if (percentage == null) return; |
There was a problem hiding this comment.
Similar to the hardcover service, perhaps much of this logic should live over in the KoreaderService?
Something like..
void syncProgressToKoreader(long bookId, float percentage, long userId);
Then it's also a little easier to test directly, as it'd make sense as public over on the KoreaderService.
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
`@backend/src/main/java/org/booklore/service/progress/ReadingProgressService.java`:
- Around line 516-529: When convertCfiToProgressXPointer(...) throws in
ReadingProgressService, the old KOReader sync point is left in progress; update
the catch block to clear the KOReader sync state by calling
progress.setKoreaderProgress(null) (and optionally
progress.setKoreaderProgressPercent(0f) if you want to reset percent) so stale
values are not used on the next sync; ensure this change is applied in the catch
surrounding the call to epubCfiService.convertCfiToProgressXPointer(...) where
progress.setKoreaderProgress(xpointer) is currently set.
🪄 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: Repository YAML (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: eac87f54-8ae3-4c5c-a9aa-3b6f34611b61
📒 Files selected for processing (1)
backend/src/main/java/org/booklore/service/progress/ReadingProgressService.java
📜 Review details
🧰 Additional context used
📓 Path-based instructions (3)
backend/src/**/*.java
📄 CodeRabbit inference engine (AGENTS.md)
backend/src/**/*.java: Use 4-space indentation and match surrounding Java style in backend code
Prefer constructor injection via Lombok patterns already used in the codebase. Do not introduce@Autowiredfield injection in backend code
Use MapStruct for entity/DTO mapping in backend code
Keep JPA entities on the *Entity suffix in backend code
Files:
backend/src/main/java/org/booklore/service/progress/ReadingProgressService.java
**/*
⚙️ CodeRabbit configuration file
**/*: This project is being developed using current and future-facing technologies:
- Java 25 with --enable-preview (preview features are INTENTIONAL and encouraged)
- Spring Boot 4 (latest major version, check APIs accordingly)
- Jackson 3 (new package: tools.jackson.* instead of com.fasterxml.jackson.*)
- Hibernate 7.3.x (Jakarta Persistence 3.2, new APIs; avoid deprecated Hibernate 5/6 patterns)
- Angular 21 (signals-based reactivity, no NgModules unless legacy)
Grimmory Internal Tools
- epub4j and pdfium4j are our own internal tools developed by the Grimmory team.
- Always verify behavior and API changes against the upstream repositories:
- If you encounter issues with these libraries, check if a fix exists in the upstream grimmory-tools organization.
Metadata Standards and Compliance
- For all metadata writing and parsing logic, double-check against Dublin Core and ANSI standards to ensure perfect official compliance.
- We strictly follow the widespread and official XML-compliant methods for EPUB2, EPUB3, CBX, and PDF formats.
General Java and Spring rules
- ALWAYS prefer modern, idiomatic Java 25 constructs over legacy patterns.
- Preview features (--enable-preview) are enabled and intentional; do NOT flag them as risky unless there is a concrete runtime issue.
- Prefer: records, sealed classes/interfaces, pattern matching (switch expressions, instanceof), structured concurrency (StructuredTaskScope), scoped values, string templates, unnamed patterns/variables.
- Prefer virtual threads (Thread.ofVirtual(), Executors.newVirtualThreadPerTaskExecutor()) over platform threads for I/O-bound work.
- Prefer the new Sequenced Collections API (SequencedCollection, SequencedMap) where applicable.
- Prefer
varfor local variables when the type is obvious from context.- Use stream().toList() instead of stream().collect(Collectors.toList()) for imm...
Files:
backend/src/main/java/org/booklore/service/progress/ReadingProgressService.java
**/service/**/*.java
⚙️ CodeRabbit configuration file
**/service/**/*.java: Spring Framework 7 service layer review:
- Flag missing
@Transactionalon methods that perform multiple writes.- Prefer constructor injection over
@Autowiredfield injection. Use@AllArgsConstructor.- Use ApiError enum for throwing exceptions.
- Flag checked exceptions swallowed silently; must log or rethrow.
- Flag Thread.sleep(); prefer Duration-based overloads or ScheduledExecutorService.
- Prefer virtual threads (Thread.ofVirtual()) for I/O-bound operations.
- Flag mutable shared state in singleton beans.
Files:
backend/src/main/java/org/booklore/service/progress/ReadingProgressService.java
🧠 Learnings (2)
📚 Learning: 2026-04-10T08:15:37.436Z
Learnt from: imnotjames
Repo: grimmory-tools/grimmory PR: 449
File: booklore-api/src/main/java/org/booklore/service/book/BookDownloadService.java:139-145
Timestamp: 2026-04-10T08:15:37.436Z
Learning: When using Spring `ContentDisposition.builder(...).filename(name, StandardCharsets.UTF_8).build()` (i.e., explicitly providing UTF-8), the resulting header value should include both the quoted `filename="=?UTF-8?..."` and the RFC 5987 `filename*=` parameters. In this case, any extra ASCII fallback computation (e.g., deriving an ASCII `fallbackFilename` via `NON_ASCII_PATTERN` and calling `.filename(fallbackFilename)`) is likely redundant—prefer calling only `.filename(fallbackName?, StandardCharsets.UTF_8)` as appropriate and let Spring handle the UTF-8 header parameters. Verify by comparing the emitted header for `filename` and `filename*` before deciding to keep an ASCII fallback.
Applied to files:
backend/src/main/java/org/booklore/service/progress/ReadingProgressService.java
📚 Learning: 2026-04-14T12:43:08.698Z
Learnt from: balazs-szucs
Repo: grimmory-tools/grimmory PR: 502
File: booklore-api/src/main/java/org/booklore/service/reader/ChapterCacheService.java:0-0
Timestamp: 2026-04-14T12:43:08.698Z
Learning: For this codebase (booklore-api), target Java 25 with `--enable-preview`, so `_` is intentionally used as an unnamed/ignored variable (e.g., lambda parameter or pattern variable) per Java’s preview feature JEP 456. Do not flag `_` in those contexts as an invalid/reserved identifier; only flag it if it’s used in a non-supported position (e.g., where an unnamed variable is not applicable for the Java preview rules).
Applied to files:
backend/src/main/java/org/booklore/service/progress/ReadingProgressService.java
🔀 Multi-repo context grimmory-tools/grimmory-docs
Findings
[::grimmory-tools/grimmory-docs::docs/integration/koreader.md:1-8, 12-13, 69-73]
- Docs describe KOReader bidirectional sync and explicitly mention converting KOReader XPointer ↔ EPUB CFI when "Sync reading progress with Grimmory eBook Reader" is enabled. They also note conversion is approximate (chapter-level) and that KOReader progress appears in the UI as "KOReader Progress" with percentage and reset action.
[::grimmory-tools/grimmory-docs::docs/tools/devices.md:3-22, 28-34, 52-56, 81-85, 123-126]
- Devices docs show per-user KOReader sync credentials and toggles:
- "Enable KOReader Sync" and "Sync reading progress with Grimmory eBook Reader" (bidirectional sync) settings.
- KOReader API path, username/password fields are exposed per user.
- Notes that KOReader credentials are stored per user and that KOReader progress is forwarded to Hardcover when enabled.
- States KOReader and Grimmory reader use separate position formats unless the CFI/XPointer sync option is turned on.
[::grimmory-tools/grimmory-docs::docs/tools/user-management.md:107-116]
- User Management docs list a "KOReader Sync" permission and bulk-reset entries (Bulk Reset KOReader Read Progress), confirming per-user permission controls exist in the product documentation.
[::grimmory-tools/grimmory-docs::src/pages/HomepageFeatures.tsx:43-44]
- UI copy references "Kobo & KOReader Sync" highlighting sync as a product feature.
Absence of backend-symbol matches in this repo
- The repository search for "EpubCfiService", "KoreaderUserRepository", "convertCfiToProgressXPointer", "koreaderProgress", and "koreaderLastSyncTime" returned only documentation/strings related to KOReader; no implementation or code references to those backend classes/methods were found in this docs repo.
Conclusion
- Documentation aligns with the PR behavior (two-way KOReader sync, CFI/XPointer conversions, per-user/global toggles, UI exposure of KOReader progress). Reviewers should ensure the new backend fields and conversion behavior introduced in the PR are exposed to the frontend/API consumers consistent with these docs and that per-user/global toggle logic matches documented permission/settings behavior.
🔇 Additional comments (1)
backend/src/main/java/org/booklore/service/progress/ReadingProgressService.java (1)
275-278: Looks good: KOReader sync is only triggered for real progress updates.Gating the new helper behind
percentage != nullkeeps this path from firing on metadata-only requests, and it doesn’t interfere with the existing Hardcover sync flow.
| float koreaderPercent = percentage / 100.0f; | ||
| progress.setKoreaderProgressPercent(koreaderPercent); | ||
| progress.setKoreaderLastSyncTime(Instant.now()); | ||
|
|
||
| BookFileEntity primaryFile = book.getPrimaryBookFile(); | ||
| if (primaryFile != null && primaryFile.getBookType() == BookFileType.EPUB | ||
| && progress.getEpubProgress() != null) { | ||
| try { | ||
| String xpointer = epubCfiService.convertCfiToProgressXPointer( | ||
| book.getFullFilePath(), progress.getEpubProgress()); | ||
| progress.setKoreaderProgress(xpointer); | ||
| } catch (Exception e) { | ||
| log.warn("Failed to convert CFI to XPointer for KOReader sync", e); | ||
| } |
There was a problem hiding this comment.
Clear KOReader sync state when CFI conversion fails.
If convertCfiToProgressXPointer(...) throws, the code leaves the previous koreaderProgress value in place. That can make the next KOReader sync point at stale content instead of failing closed.
Suggested fix
try {
String xpointer = epubCfiService.convertCfiToProgressXPointer(
book.getFullFilePath(), progress.getEpubProgress());
progress.setKoreaderProgress(xpointer);
} catch (Exception e) {
+ progress.setKoreaderProgress(null);
log.warn("Failed to convert CFI to XPointer for KOReader sync", e);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@backend/src/main/java/org/booklore/service/progress/ReadingProgressService.java`
around lines 516 - 529, When convertCfiToProgressXPointer(...) throws in
ReadingProgressService, the old KOReader sync point is left in progress; update
the catch block to clear the KOReader sync state by calling
progress.setKoreaderProgress(null) (and optionally
progress.setKoreaderProgressPercent(0f) if you want to reset percent) so stale
values are not used on the next sync; ensure this change is applied in the catch
surrounding the call to epubCfiService.convertCfiToProgressXPointer(...) where
progress.setKoreaderProgress(xpointer) is currently set.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/src/main/java/org/booklore/service/koreader/KoreaderService.java`:
- Around line 117-124: The catch block in KoreaderService around
epubCfiService.convertCfiToProgressXPointer currently only logs the exception,
leaving any previous koreaderProgress intact; modify the catch to clear or reset
the progress.koreaderProgress (call progress.setKoreaderProgress(null) or
equivalent) so stale KOReader state is not preserved when conversion fails,
ensuring the next KOReader pull uses the updated percent; keep the log.warn call
but add the reset logic referencing convertCfiToProgressXPointer and
progress.setKoreaderProgress in the same catch.
- Around line 96-128: The syncProgressToKoreader method is doing blocking I/O on
the request thread and runs in the caller's transaction; change its execution to
be isolated and asynchronous by annotating syncProgressToKoreader with `@Async`
and `@Transactional`(propagation = Propagation.REQUIRES_NEW) (import Propagation),
so its work (including koreaderUserRepository.findByBookLoreUserId,
epubCfiService.convertCfiToProgressXPointer and progressRepository.save) runs in
a new transaction off the request thread; also ensure any unchecked exceptions
inside the method are caught and logged (so failures do not bubble up and mark
the caller transaction for rollback).
In
`@backend/src/test/java/org/booklore/service/progress/ReadingProgressServiceTest.java`:
- Around line 292-390: Add unit tests for KoreaderService.syncProgressToKoreader
to cover the missing logic: write tests that toggle
isSyncEnabled()/isSyncWithBookloreReader() to assert the progress repositories
are never written when disabled, test the EPUB path where
convertCfiToProgressXPointer(...) returns an xpointer so the XPointer is saved
via getOrCreateUserProgress(...) and persisted, add a test where
convertCfiToProgressXPointer(...) throws to assert koreaderProgress is cleared
(null) but percentage and timestamp still get written, and add no-op tests when
book or user lookups fail; target the KoreaderService class and its methods
syncProgressToKoreader, convertCfiToProgressXPointer, getOrCreateUserProgress
and verify interactions with the koreaderProgress repository and user progress
persistence accordingly.
🪄 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: Repository YAML (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4666f4fd-7337-4f2e-abb5-3e881704bd5b
📒 Files selected for processing (3)
backend/src/main/java/org/booklore/service/koreader/KoreaderService.javabackend/src/main/java/org/booklore/service/progress/ReadingProgressService.javabackend/src/test/java/org/booklore/service/progress/ReadingProgressServiceTest.java
📜 Review details
🧰 Additional context used
📓 Path-based instructions (5)
backend/src/**/*.java
📄 CodeRabbit inference engine (AGENTS.md)
backend/src/**/*.java: Use 4-space indentation and match surrounding Java style in backend code
Prefer constructor injection via Lombok patterns already used in the codebase. Do not introduce@Autowiredfield injection in backend code
Use MapStruct for entity/DTO mapping in backend code
Keep JPA entities on the *Entity suffix in backend code
Files:
backend/src/main/java/org/booklore/service/progress/ReadingProgressService.javabackend/src/test/java/org/booklore/service/progress/ReadingProgressServiceTest.javabackend/src/main/java/org/booklore/service/koreader/KoreaderService.java
**/*
⚙️ CodeRabbit configuration file
**/*: This project is being developed using current and future-facing technologies:
- Java 25 with --enable-preview (preview features are INTENTIONAL and encouraged)
- Spring Boot 4 (latest major version, check APIs accordingly)
- Jackson 3 (new package: tools.jackson.* instead of com.fasterxml.jackson.*)
- Hibernate 7.3.x (Jakarta Persistence 3.2, new APIs; avoid deprecated Hibernate 5/6 patterns)
- Angular 21 (signals-based reactivity, no NgModules unless legacy)
Grimmory Internal Tools
- epub4j and pdfium4j are our own internal tools developed by the Grimmory team.
- Always verify behavior and API changes against the upstream repositories:
- If you encounter issues with these libraries, check if a fix exists in the upstream grimmory-tools organization.
Metadata Standards and Compliance
- For all metadata writing and parsing logic, double-check against Dublin Core and ANSI standards to ensure perfect official compliance.
- We strictly follow the widespread and official XML-compliant methods for EPUB2, EPUB3, CBX, and PDF formats.
General Java and Spring rules
- ALWAYS prefer modern, idiomatic Java 25 constructs over legacy patterns.
- Preview features (--enable-preview) are enabled and intentional; do NOT flag them as risky unless there is a concrete runtime issue.
- Prefer: records, sealed classes/interfaces, pattern matching (switch expressions, instanceof), structured concurrency (StructuredTaskScope), scoped values, string templates, unnamed patterns/variables.
- Prefer virtual threads (Thread.ofVirtual(), Executors.newVirtualThreadPerTaskExecutor()) over platform threads for I/O-bound work.
- Prefer the new Sequenced Collections API (SequencedCollection, SequencedMap) where applicable.
- Prefer
varfor local variables when the type is obvious from context.- Use stream().toList() instead of stream().collect(Collectors.toList()) for imm...
Files:
backend/src/main/java/org/booklore/service/progress/ReadingProgressService.javabackend/src/test/java/org/booklore/service/progress/ReadingProgressServiceTest.javabackend/src/main/java/org/booklore/service/koreader/KoreaderService.java
**/service/**/*.java
⚙️ CodeRabbit configuration file
**/service/**/*.java: Spring Framework 7 service layer review:
- Flag missing
@Transactionalon methods that perform multiple writes.- Prefer constructor injection over
@Autowiredfield injection. Use@AllArgsConstructor.- Use ApiError enum for throwing exceptions.
- Flag checked exceptions swallowed silently; must log or rethrow.
- Flag Thread.sleep(); prefer Duration-based overloads or ScheduledExecutorService.
- Prefer virtual threads (Thread.ofVirtual()) for I/O-bound operations.
- Flag mutable shared state in singleton beans.
Files:
backend/src/main/java/org/booklore/service/progress/ReadingProgressService.javabackend/src/test/java/org/booklore/service/progress/ReadingProgressServiceTest.javabackend/src/main/java/org/booklore/service/koreader/KoreaderService.java
backend/src/test/**/*.java
📄 CodeRabbit inference engine (AGENTS.md)
Prefer focused unit tests; use
@SpringBootTestonly when the Spring context is required in backend code
Files:
backend/src/test/java/org/booklore/service/progress/ReadingProgressServiceTest.java
**/*Test.java
⚙️ CodeRabbit configuration file
**/*Test.java: Java test review:
- Prefer
@ExtendWith(SpringExtension.class) or@SpringBootTestfor integration tests.- Flag tests with no assertions.
- Flag Thread.sleep() in tests; use Awaitility or virtual-thread-friendly alternatives.
- Flag hardcoded ports or file paths.
- Flag missing edge case coverage: null, empty, boundary values.
- Prefer AssertJ over JUnit's built-in assertions for readability.
- Prefer
@Sqlor Testcontainers for database state; not hand-rolled setup/teardown.
Files:
backend/src/test/java/org/booklore/service/progress/ReadingProgressServiceTest.java
🧠 Learnings (3)
📚 Learning: 2026-04-10T08:15:37.436Z
Learnt from: imnotjames
Repo: grimmory-tools/grimmory PR: 449
File: booklore-api/src/main/java/org/booklore/service/book/BookDownloadService.java:139-145
Timestamp: 2026-04-10T08:15:37.436Z
Learning: When using Spring `ContentDisposition.builder(...).filename(name, StandardCharsets.UTF_8).build()` (i.e., explicitly providing UTF-8), the resulting header value should include both the quoted `filename="=?UTF-8?..."` and the RFC 5987 `filename*=` parameters. In this case, any extra ASCII fallback computation (e.g., deriving an ASCII `fallbackFilename` via `NON_ASCII_PATTERN` and calling `.filename(fallbackFilename)`) is likely redundant—prefer calling only `.filename(fallbackName?, StandardCharsets.UTF_8)` as appropriate and let Spring handle the UTF-8 header parameters. Verify by comparing the emitted header for `filename` and `filename*` before deciding to keep an ASCII fallback.
Applied to files:
backend/src/main/java/org/booklore/service/progress/ReadingProgressService.javabackend/src/test/java/org/booklore/service/progress/ReadingProgressServiceTest.javabackend/src/main/java/org/booklore/service/koreader/KoreaderService.java
📚 Learning: 2026-04-14T12:43:08.698Z
Learnt from: balazs-szucs
Repo: grimmory-tools/grimmory PR: 502
File: booklore-api/src/main/java/org/booklore/service/reader/ChapterCacheService.java:0-0
Timestamp: 2026-04-14T12:43:08.698Z
Learning: For this codebase (booklore-api), target Java 25 with `--enable-preview`, so `_` is intentionally used as an unnamed/ignored variable (e.g., lambda parameter or pattern variable) per Java’s preview feature JEP 456. Do not flag `_` in those contexts as an invalid/reserved identifier; only flag it if it’s used in a non-supported position (e.g., where an unnamed variable is not applicable for the Java preview rules).
Applied to files:
backend/src/main/java/org/booklore/service/progress/ReadingProgressService.javabackend/src/test/java/org/booklore/service/progress/ReadingProgressServiceTest.javabackend/src/main/java/org/booklore/service/koreader/KoreaderService.java
📚 Learning: 2026-04-27T15:25:55.042Z
Learnt from: imnotjames
Repo: grimmory-tools/grimmory PR: 930
File: backend/src/test/java/org/booklore/service/metadata/parser/ComicvineBookParserTest.java:68-75
Timestamp: 2026-04-27T15:25:55.042Z
Learning: In this repository’s JUnit 5 test sources (e.g., under backend/src/test/java/), do not flag “bare” `assert` statements as a bug. The project test runner is configured to always execute tests with assertions enabled (e.g., `-ea`), so `assert` behavior is consistent. Continue to review for correctness, but don’t treat unguarded `assert` usage in test classes as a static-analysis issue.
Applied to files:
backend/src/test/java/org/booklore/service/progress/ReadingProgressServiceTest.java
🔀 Multi-repo context grimmory-tools/grimmory-docs
Findings for grimmory-tools/grimmory-docs
-
Docs confirm bidirectional KOReader sync, per-user toggles/permissions, and CFI ↔ KOReader XPointer conversion behavior:
- docs/integration/koreader.md:1-85 — explains KOReader sync workflow, binary file matching, "Sync reading progress with Grimmory eBook Reader" option (CFI ↔ XPointer conversion, approximate/chapter-level), UI display ("KOReader Progress"), troubleshooting. [::grimmory-tools/grimmory-docs::docs/integration/koreader.md:1-85]
- docs/tools/devices.md:1-240 — lists device settings: "Enable KOReader Sync", "Sync reading progress with Grimmory eBook Reader", KOReader API path/credentials, and note that KOReader progress is forwarded to Hardcover when enabled. [::grimmory-tools/grimmory-docs::docs/tools/devices.md:1-240]
- docs/tools/user-management.md:1-240 — documents the "KOReader Sync" permission and bulk-reset KOReader progress. [::grimmory-tools/grimmory-docs::docs/tools/user-management.md:1-240]
-
No backend/source-code consumers in this repo — only user-facing documentation. No references to backend classes/fields (e.g., KoreaderService, EpubCfiService, koreaderProgress, koreaderLastSyncTime) beyond described UX/feature expectations.
Implication for PR review:
- Documentation aligns with the PR's behavior (two-way sync and CFI↔XPointer conversion). Reviewers should verify the new backend logic respects the documented toggles/permissions and that any new fields persisted (koreaderProgress, koreaderProgressPercent, koreaderLastSyncTime) are surfaced to the UI/API as expected.
🔇 Additional comments (2)
backend/src/main/java/org/booklore/service/progress/ReadingProgressService.java (1)
274-277: LGTM — delegation is correct; see the note onKoreaderService.syncProgressToKoreader.The
nullguard at line 274 correctly mirrors the one insidesyncProgressToKoreader, sopercentagecan never benullat the call site. The ordering (KOReader before Hardcover) is sensible.The transaction coupling and synchronous I/O concerns are rooted in
KoreaderService.syncProgressToKoreader— addressed in the comment there.backend/src/test/java/org/booklore/service/progress/ReadingProgressServiceTest.java (1)
50-51: Mock registration forKoreaderServiceis correct.
@Mock KoreaderService koreaderServicecombined with@InjectMocks ReadingProgressServiceis sufficient — Mockito constructsReadingProgressServicevia@RequiredArgsConstructor-generated constructor injection with all mocks resolved.
| @Transactional | ||
| public void syncProgressToKoreader(Long bookId, Float percentage, Long userId) { | ||
| if (percentage == null) return; | ||
|
|
||
| koreaderUserRepository.findByBookLoreUserId(userId) | ||
| .filter(KoreaderUserEntity::isSyncEnabled) | ||
| .filter(KoreaderUserEntity::isSyncWithBookloreReader) | ||
| .ifPresent(koreaderUser -> { | ||
| BookEntity book = bookRepository.findById(bookId).orElse(null); | ||
| BookLoreUserEntity user = userRepository.findById(userId).orElse(null); | ||
| if (book == null || user == null) return; | ||
|
|
||
| UserBookProgressEntity progress = getOrCreateUserProgress(user, book); | ||
|
|
||
| float koreaderPercent = percentage / 100.0f; | ||
| progress.setKoreaderProgressPercent(koreaderPercent); | ||
| progress.setKoreaderLastSyncTime(Instant.now()); | ||
|
|
||
| BookFileEntity primaryFile = book.getPrimaryBookFile(); | ||
| if (primaryFile != null && primaryFile.getBookType() == BookFileType.EPUB | ||
| && progress.getEpubProgress() != null) { | ||
| try { | ||
| String xpointer = epubCfiService.convertCfiToProgressXPointer( | ||
| book.getFullFilePath(), progress.getEpubProgress()); | ||
| progress.setKoreaderProgress(xpointer); | ||
| } catch (Exception e) { | ||
| log.warn("Failed to convert CFI to XPointer for KOReader sync", e); | ||
| } | ||
| } | ||
|
|
||
| progressRepository.save(progress); | ||
| }); | ||
| } |
There was a problem hiding this comment.
Synchronous I/O on the request thread and unconstrained transaction coupling.
syncProgressToKoreader runs entirely on the HTTP request thread, including the epubCfiService.convertCfiToProgressXPointer call which reads the EPUB file from disk. Because the method uses the default REQUIRED propagation, it joins updateReadProgress's transaction — any uncaught exception here marks the caller's transaction for rollback, which would silently undo the user's reading-progress write at line 272 in ReadingProgressService.
HardcoverSyncService.syncProgressToHardcover (called immediately after) is @Async @transactional(readOnly = true), which decouples fault isolation and keeps the request thread clean. The KOReader sync should follow the same pattern.
As per coding guidelines, virtual threads / async dispatch should be preferred for I/O-bound work.
♻️ Proposed alignment with HardcoverSyncService pattern
+import org.springframework.scheduling.annotation.Async;
- `@Transactional`
+ `@Async`
+ `@Transactional`(propagation = Propagation.REQUIRES_NEW)
public void syncProgressToKoreader(Long bookId, Float percentage, Long userId) {Using REQUIRES_NEW ensures the KOReader sync has its own transaction that can be rolled back without affecting the caller. @Async moves the I/O work off the request thread and keeps updateReadProgress's latency unaffected.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/src/main/java/org/booklore/service/koreader/KoreaderService.java`
around lines 96 - 128, The syncProgressToKoreader method is doing blocking I/O
on the request thread and runs in the caller's transaction; change its execution
to be isolated and asynchronous by annotating syncProgressToKoreader with `@Async`
and `@Transactional`(propagation = Propagation.REQUIRES_NEW) (import Propagation),
so its work (including koreaderUserRepository.findByBookLoreUserId,
epubCfiService.convertCfiToProgressXPointer and progressRepository.save) runs in
a new transaction off the request thread; also ensure any unchecked exceptions
inside the method are caught and logged (so failures do not bubble up and mark
the caller transaction for rollback).
| try { | ||
| String xpointer = epubCfiService.convertCfiToProgressXPointer( | ||
| book.getFullFilePath(), progress.getEpubProgress()); | ||
| progress.setKoreaderProgress(xpointer); | ||
| } catch (Exception e) { | ||
| log.warn("Failed to convert CFI to XPointer for KOReader sync", e); | ||
| } | ||
| } |
There was a problem hiding this comment.
Stale koreaderProgress value survives CFI conversion failure.
When convertCfiToProgressXPointer throws, the catch block only logs — the existing koreaderProgress (set by a previous Koreader→Booklore sync) is left intact. On the next KOReader pull, it will resume at the old position rather than the new one derived from the updated percent.
This was flagged in the previous review ("Clear KOReader sync state when CFI conversion fails") and was not addressed when the logic was moved here.
🐛 Proposed fix
} catch (Exception e) {
+ progress.setKoreaderProgress(null);
log.warn("Failed to convert CFI to XPointer for KOReader sync", e);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/src/main/java/org/booklore/service/koreader/KoreaderService.java`
around lines 117 - 124, The catch block in KoreaderService around
epubCfiService.convertCfiToProgressXPointer currently only logs the exception,
leaving any previous koreaderProgress intact; modify the catch to clear or reset
the progress.koreaderProgress (call progress.setKoreaderProgress(null) or
equivalent) so stale KOReader state is not preserved when conversion fails,
ensuring the next KOReader pull uses the updated percent; keep the log.warn call
but add the reset logic referencing convertCfiToProgressXPointer and
progress.setKoreaderProgress in the same catch.
| @Test | ||
| void updateReadProgress_shouldDelegateKoreaderSync() { | ||
| long bookId = 1L; | ||
| BookEntity book = new BookEntity(); | ||
| book.setId(bookId); | ||
| BookFileEntity primaryFile = new BookFileEntity(); | ||
| primaryFile.setId(1L); | ||
| primaryFile.setBook(book); | ||
| primaryFile.setBookType(BookFileType.EPUB); | ||
| book.setBookFiles(List.of(primaryFile)); | ||
|
|
||
| BookLoreUser user = mock(BookLoreUser.class); | ||
| when(user.getId()).thenReturn(2L); | ||
| when(authenticationService.getAuthenticatedUser()).thenReturn(user); | ||
| when(bookRepository.findByIdWithBookFiles(bookId)).thenReturn(Optional.of(book)); | ||
|
|
||
| BookLoreUserEntity userEntity = new BookLoreUserEntity(); | ||
| userEntity.setId(2L); | ||
| when(userRepository.findById(2L)).thenReturn(Optional.of(userEntity)); | ||
|
|
||
| UserBookProgressEntity progress = new UserBookProgressEntity(); | ||
| when(userBookProgressRepository.findByUserIdAndBookId(2L, bookId)).thenReturn(Optional.of(progress)); | ||
|
|
||
| when(userBookFileProgressRepository.findByUserIdAndBookFileId(2L, 1L)).thenReturn(Optional.empty()); | ||
|
|
||
| ReadProgressRequest req = new ReadProgressRequest(); | ||
| req.setBookId(bookId); | ||
| EpubProgress epubProgress = EpubProgress.builder().cfi("cfi").percentage(100f).build(); | ||
| req.setEpubProgress(epubProgress); | ||
|
|
||
| readingProgressService.updateReadProgress(req); | ||
|
|
||
| verify(koreaderService).syncProgressToKoreader(bookId, 100f, 2L); | ||
| } | ||
|
|
||
| @Test | ||
| void updateReadProgress_shouldDelegateKoreaderSyncWithCorrectPercentage() { | ||
| long bookId = 1L; | ||
| BookEntity book = new BookEntity(); | ||
| book.setId(bookId); | ||
| BookFileEntity primaryFile = new BookFileEntity(); | ||
| primaryFile.setId(1L); | ||
| primaryFile.setBook(book); | ||
| primaryFile.setBookType(BookFileType.PDF); | ||
| book.setBookFiles(List.of(primaryFile)); | ||
|
|
||
| BookLoreUser user = mock(BookLoreUser.class); | ||
| when(user.getId()).thenReturn(2L); | ||
| when(authenticationService.getAuthenticatedUser()).thenReturn(user); | ||
| when(bookRepository.findByIdWithBookFiles(bookId)).thenReturn(Optional.of(book)); | ||
|
|
||
| BookLoreUserEntity userEntity = new BookLoreUserEntity(); | ||
| userEntity.setId(2L); | ||
| when(userRepository.findById(2L)).thenReturn(Optional.of(userEntity)); | ||
|
|
||
| UserBookProgressEntity progress = new UserBookProgressEntity(); | ||
| when(userBookProgressRepository.findByUserIdAndBookId(2L, bookId)).thenReturn(Optional.of(progress)); | ||
|
|
||
| when(userBookFileProgressRepository.findByUserIdAndBookFileId(2L, 1L)).thenReturn(Optional.empty()); | ||
|
|
||
| ReadProgressRequest req = new ReadProgressRequest(); | ||
| req.setBookId(bookId); | ||
| PdfProgress pdfProgress = PdfProgress.builder().page(10).percentage(50f).build(); | ||
| req.setPdfProgress(pdfProgress); | ||
|
|
||
| readingProgressService.updateReadProgress(req); | ||
|
|
||
| verify(koreaderService).syncProgressToKoreader(bookId, 50f, 2L); | ||
| } | ||
|
|
||
| @Test | ||
| void updateReadProgress_shouldNotDelegateKoreaderSyncWhenPercentageNull() { | ||
| long bookId = 1L; | ||
| BookEntity book = new BookEntity(); | ||
| book.setId(bookId); | ||
| BookFileEntity primaryFile = new BookFileEntity(); | ||
| primaryFile.setId(1L); | ||
| primaryFile.setBook(book); | ||
| primaryFile.setBookType(BookFileType.EPUB); | ||
| book.setBookFiles(List.of(primaryFile)); | ||
|
|
||
| BookLoreUser user = mock(BookLoreUser.class); | ||
| when(user.getId()).thenReturn(2L); | ||
| when(authenticationService.getAuthenticatedUser()).thenReturn(user); | ||
| when(bookRepository.findByIdWithBookFiles(bookId)).thenReturn(Optional.of(book)); | ||
|
|
||
| BookLoreUserEntity userEntity = new BookLoreUserEntity(); | ||
| userEntity.setId(2L); | ||
| when(userRepository.findById(2L)).thenReturn(Optional.of(userEntity)); | ||
|
|
||
| when(userBookProgressRepository.findByUserIdAndBookId(2L, bookId)).thenReturn(Optional.empty()); | ||
|
|
||
| ReadProgressRequest req = new ReadProgressRequest(); | ||
| req.setBookId(bookId); | ||
|
|
||
| readingProgressService.updateReadProgress(req); | ||
|
|
||
| verify(koreaderService, never()).syncProgressToKoreader(anyLong(), anyFloat(), anyLong()); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Three new delegation tests cover the happy path and the no-op path, but KoreaderService.syncProgressToKoreader logic is entirely untested.
The new tests correctly verify that ReadingProgressService delegates to KoreaderService for both EPUB/PDF and skips when percentage is null. However, the logic now inside KoreaderService.syncProgressToKoreader — sync toggle checks, getOrCreateUserProgress, CFI→XPointer conversion, and the exception path (stale koreaderProgress) — has no unit tests at all. A previous review comment flagged the CFI-exception path as missing test coverage; that gap still exists, now against KoreaderService directly.
Key scenarios that should be covered in a KoreaderServiceTest:
isSyncEnabled = false→ progress repository never writtenisSyncEnabled = true,isSyncWithBookloreReader = false→ repository never writtenisSyncEnabled = true,isSyncWithBookloreReader = true, book is EPUB with CFI → XPointer writtenconvertCfiToProgressXPointerthrows →koreaderProgressisnull, percent and timestamp still written (once the stale-clear fix is applied)- Book or user not found → no-op
Would you like me to draft a KoreaderServiceTest covering these scenarios?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@backend/src/test/java/org/booklore/service/progress/ReadingProgressServiceTest.java`
around lines 292 - 390, Add unit tests for
KoreaderService.syncProgressToKoreader to cover the missing logic: write tests
that toggle isSyncEnabled()/isSyncWithBookloreReader() to assert the progress
repositories are never written when disabled, test the EPUB path where
convertCfiToProgressXPointer(...) returns an xpointer so the XPointer is saved
via getOrCreateUserProgress(...) and persisted, add a test where
convertCfiToProgressXPointer(...) throws to assert koreaderProgress is cleared
(null) but percentage and timestamp still get written, and add no-op tests when
book or user lookups fail; target the KoreaderService class and its methods
syncProgressToKoreader, convertCfiToProgressXPointer, getOrCreateUserProgress
and verify interactions with the koreaderProgress repository and user progress
persistence accordingly.
| } | ||
|
|
||
| @Transactional | ||
| public void syncProgressToKoreader(Long bookId, Float percentage, Long userId) { |
There was a problem hiding this comment.
I think we can unbox these types.
| public void syncProgressToKoreader(Long bookId, Float percentage, Long userId) { | |
| public void syncProgressToKoreader(long bookId, float percentage, long userId) { |
| try { | ||
| String xpointer = epubCfiService.convertCfiToProgressXPointer( | ||
| book.getFullFilePath(), progress.getEpubProgress()); | ||
| progress.setKoreaderProgress(xpointer); |
There was a problem hiding this comment.
What happens if this fails to convert and the koreader progress progress is mismatched with koreader progress percent?
If that's a mismatch causes failure elsewhere, should we try to do the conversion first and then bail if it doesn't work as expected?
| && progress.getEpubProgress() != null) { | ||
| try { | ||
| String xpointer = epubCfiService.convertCfiToProgressXPointer( | ||
| book.getFullFilePath(), progress.getEpubProgress()); |
There was a problem hiding this comment.
Instead of using the book full file path we should be explicit and use the primaryFile full file path, right? That way if we ever have to change the book file to be something else (eg, we know a different file is being used by koreader) it's less churn to update it.
| .ifPresent(koreaderUser -> { | ||
| BookEntity book = bookRepository.findById(bookId).orElse(null); | ||
| BookLoreUserEntity user = userRepository.findById(userId).orElse(null); | ||
| if (book == null || user == null) return; |
There was a problem hiding this comment.
We should probably log here - maybe a warn? This probably shouldn't happen.
|
I've tested this with koreader and it seems to work pretty well. It's not perfect in some cases but it's close enough. |
I’ve spent a lot of time trying to make it more accurate and I simply can’t get it much better with the current framework. It might take some deeper digging to get it 100%, but this is an improvement. I see the comments you made and will get to them when I have the opportunity. |
2 participants
Fixes #414
a description of why you made this change
Kosync is an existing feature that promises 2 way sync, but it currently only syncs from the koreader app to grimmory and not the other way around. There have been multiple people complaining of this, myself one of them, and this PR fixes that issue.
how it fixes the issue you'd linked
#414
Created a new functoin called
syncKoreaderFromBookloreProgresswhich is called when new progress is made, and the correct settings are enabled. It uses all existing functionality to achieve getting the right location and setting the sync.how you tested this
Summary by CodeRabbit
New Features
Tests