feat: add reading progress endpoints to mobile api#377
Merged
zachyale merged 5 commits intoApr 14, 2026
Conversation
Contributor
📝 WalkthroughWalkthroughAdded two Mobile App API endpoints for per-book reading progress: Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Controller as Controller\n(AppBookController)
participant Service as Service\n(AppBookService)
participant Repo as Repository\n(BookRepository / ProgressRepo)
rect rgba(200,220,255,0.5)
Client->>Controller: GET /api/v1/app/books/{bookId}/progress
Controller->>Service: getBookProgress(bookId)
Service->>Repo: loadBookWithFiles(bookId)
Service->>Repo: fetchUserProgress(bookId, userId)
Service->>Repo: fetchLatestFileProgress(bookId, userId)
Service->>Service: mobileBookMapper.toProgressResponse(...)
Service->>Controller: AppBookProgressResponse
Controller->>Client: 200 OK (progress)
end
sequenceDiagram
participant Client as Client
participant Controller as Controller\n(AppBookController)
participant Service as Service\n(AppBookService)
participant BookSvc as BookService
participant Repo as Repository\n(BookRepository)
rect rgba(200,255,200,0.5)
Client->>Controller: PUT /api/v1/app/books/{bookId}/progress (UpdateProgressRequest)
Controller->>Service: updateBookProgress(bookId, request)
Service->>Repo: loadBookWithFiles(bookId)
Service->>BookSvc: bookService.updateReadProgress(ReadProgressRequest)
BookSvc-->>Service: OK
Service->>Controller: 200 OK
Controller->>Client: 200 OK
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 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. Comment |
Contributor
|
Looks good, is this for 3rd party clients, or what's the context here? |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
booklore-api/src/main/java/org/booklore/app/service/AppBookService.java (2)
154-154: Redundant@Transactional(readOnly = true)annotation.Same as above — this is already the default from the class-level annotation.
🧹 Remove redundant annotation
- `@Transactional`(readOnly = true) public AppPageResponse<AppBookSummary> searchBooks(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@booklore-api/src/main/java/org/booklore/app/service/AppBookService.java` at line 154, Remove the redundant method-level `@Transactional`(readOnly = true) annotation in AppBookService: the class already declares the same transactional default, so delete the method-level `@Transactional`(readOnly = true) annotation (the specific annotation instance shown) to avoid duplication and rely on the class-level transactional behavior.
116-116: Redundant@Transactional(readOnly = true)annotation.The class is already annotated with
@Transactional(readOnly = true)at the class level (line 44), so this method-level annotation is unnecessary.🧹 Remove redundant annotation
- `@Transactional`(readOnly = true) public AppBookProgressResponse getBookProgress(Long bookId) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@booklore-api/src/main/java/org/booklore/app/service/AppBookService.java` at line 116, Remove the redundant method-level `@Transactional`(readOnly = true) annotation from the method in class AppBookService; the class is already annotated with `@Transactional`(readOnly = true) so simply delete the method-level annotation (leave the method body and signature unchanged) to avoid duplication.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@booklore-api/src/main/java/org/booklore/app/controller/AppBookController.java`:
- Around line 57-64: The controller currently ignores ReadProgressRequest.bookId
(clients must send it due to `@NotNull`) because AppBookService.updateBookProgress
overwrites it; fix by either (A) explicitly set the path variable into the
request before calling the service (in AppBookController.updateBookProgress call
request.setBookId(bookId) so mobileBookService.updateBookProgress receives the
canonical id), or (B) enforce consistency by validating that request.getBookId()
== bookId and return 400 if they differ, and additionally change the DTO
annotation on ReadProgressRequest.bookId from `@NotNull` to `@Null` (or remove the
constraint) if you choose option A; also add API docs/comments to explain that
the path id is authoritative.
---
Nitpick comments:
In `@booklore-api/src/main/java/org/booklore/app/service/AppBookService.java`:
- Line 154: Remove the redundant method-level `@Transactional`(readOnly = true)
annotation in AppBookService: the class already declares the same transactional
default, so delete the method-level `@Transactional`(readOnly = true) annotation
(the specific annotation instance shown) to avoid duplication and rely on the
class-level transactional behavior.
- Line 116: Remove the redundant method-level `@Transactional`(readOnly = true)
annotation from the method in class AppBookService; the class is already
annotated with `@Transactional`(readOnly = true) so simply delete the method-level
annotation (leave the method body and signature unchanged) to avoid duplication.
🪄 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: 91d9b825-5e46-4457-918e-144d9e3d5495
📒 Files selected for processing (5)
booklore-api/src/main/java/org/booklore/app/controller/AppBookController.javabooklore-api/src/main/java/org/booklore/app/dto/AppBookProgressResponse.javabooklore-api/src/main/java/org/booklore/app/mapper/AppBookMapper.javabooklore-api/src/main/java/org/booklore/app/service/AppBookService.javabooklore-api/src/test/java/org/booklore/app/service/AppBookServiceFilterOptionsTest.java
📜 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). (3)
- GitHub Check: Test Suite / Frontend Tests
- GitHub Check: Test Suite / Backend Tests
- GitHub Check: Analyze (java-kotlin)
🧰 Additional context used
📓 Path-based instructions (2)
booklore-api/src/**/*.java
📄 CodeRabbit inference engine (AGENTS.md)
booklore-api/src/**/*.java: Use 4-space indentation in Java files and match surrounding Java style
Prefer constructor injection via Lombok patterns already used in the codebase. Do not introduce@Autowiredfield injection
Use MapStruct for entity/DTO mapping
Files:
booklore-api/src/test/java/org/booklore/app/service/AppBookServiceFilterOptionsTest.javabooklore-api/src/main/java/org/booklore/app/mapper/AppBookMapper.javabooklore-api/src/main/java/org/booklore/app/dto/AppBookProgressResponse.javabooklore-api/src/main/java/org/booklore/app/service/AppBookService.javabooklore-api/src/main/java/org/booklore/app/controller/AppBookController.java
booklore-api/src/test/**/*.java
📄 CodeRabbit inference engine (AGENTS.md)
Prefer focused unit tests; use
@SpringBootTestonly when the Spring context is required
Files:
booklore-api/src/test/java/org/booklore/app/service/AppBookServiceFilterOptionsTest.java
🧠 Learnings (6)
📚 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 booklore-api/src/**/*.java : Prefer constructor injection via Lombok patterns already used in the codebase. Do not introduce `Autowired` field injection
Applied to files:
booklore-api/src/test/java/org/booklore/app/service/AppBookServiceFilterOptionsTest.java
📚 Learning: 2026-04-04T15:36:55.394Z
Learnt from: balazs-szucs
Repo: grimmory-tools/grimmory PR: 372
File: booklore-api/src/main/java/org/booklore/app/service/AppBookService.java:357-359
Timestamp: 2026-04-04T15:36:55.394Z
Learning: In `booklore-api/src/main/java/org/booklore/app/service/AppBookService.java`, `shelfId` and `magicShelfId` are mutually exclusive navigation contexts in `getFilterOptions`. A request will never supply both at the same time, so the `else if (shelfId != null)` branch after the `magicBookIds != null` check is intentional and correct — there is no missing shelf-∩-magicShelf intersection case. Do not flag this pattern as a bug.
Applied to files:
booklore-api/src/test/java/org/booklore/app/service/AppBookServiceFilterOptionsTest.java
📚 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 booklore-api/src/test/**/*.java : Prefer focused unit tests; use `SpringBootTest` only when the Spring context is required
Applied to files:
booklore-api/src/test/java/org/booklore/app/service/AppBookServiceFilterOptionsTest.java
📚 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 booklore-api/src/**/*.java : Use MapStruct for entity/DTO mapping
Applied to files:
booklore-api/src/main/java/org/booklore/app/mapper/AppBookMapper.javabooklore-api/src/main/java/org/booklore/app/dto/AppBookProgressResponse.javabooklore-api/src/main/java/org/booklore/app/service/AppBookService.javabooklore-api/src/main/java/org/booklore/app/controller/AppBookController.java
📚 Learning: 2026-04-01T17:50:06.817Z
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.
Applied to files:
booklore-api/src/main/java/org/booklore/app/service/AppBookService.java
📚 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:
booklore-api/src/main/java/org/booklore/app/service/AppBookService.java
🔇 Additional comments (5)
booklore-api/src/main/java/org/booklore/app/dto/AppBookProgressResponse.java (1)
1-25: LGTM!Clean DTO design with appropriate Lombok annotations and
@JsonInclude(NON_NULL)for cleaner API responses. Reusing the nested progress types fromAppBookDetailis a good choice to maintain consistency.booklore-api/src/main/java/org/booklore/app/mapper/AppBookMapper.java (1)
77-89: LGTM!The
defaultmethod pattern is consistent with other mapping methods in this interface (e.g.,toShelfSummary,toLibrarySummary), and it properly reuses the existing named mapping methods for individual progress types.booklore-api/src/main/java/org/booklore/app/service/AppBookService.java (1)
140-152: LGTM!The
@Transactionalannotation correctly overrides the class-levelreadOnly = trueto allow writes. The access validation pattern is consistent with other methods, and delegation toBookServicemaintains single responsibility.booklore-api/src/main/java/org/booklore/app/controller/AppBookController.java (1)
50-55: LGTM!Clean endpoint that delegates to the service layer with proper path variable binding.
booklore-api/src/test/java/org/booklore/app/service/AppBookServiceFilterOptionsTest.java (1)
51-51: LGTM!Test correctly updated to include the new
BookServicemock dependency with proper constructor argument ordering.Also applies to: 61-65
Contributor
|
Ah, just read the issue, my apologies. Yeah good. I'll pass the review onto Zach, though. |
zachyale
requested changes
Apr 9, 2026
Member
There was a problem hiding this comment.
Sorry for the delay in a review here- we've been working around the clock to get some major refactors in- luckily no conflicts on this area 😄
Some minor comments w/ some suggested changes- open to your input here, but at minimum if we're going to introduce this support we should ensure we're using one source of truth for bookId
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
booklore-api/src/main/java/org/booklore/app/service/AppBookService.java (1)
71-87:⚠️ Potential issue | 🔴 CriticalConstructor is missing the
BookServicedependency.The field
bookServiceis declared at line 62 but is never initialized in the constructor. This will cause either a compilation error (final field not initialized) or aNullPointerExceptionat line 195 whenupdateBookProgresscallsbookService.updateReadProgress(...).🐛 Proposed fix to add the missing constructor parameter
public AppBookService(BookRepository bookRepository, UserBookProgressRepository userBookProgressRepository, UserBookFileProgressRepository userBookFileProgressRepository, ShelfRepository shelfRepository, AuthenticationService authenticationService, AppBookMapper mobileBookMapper, + BookService bookService, MagicShelfBookService magicShelfBookService, EntityManager entityManager) { this.bookRepository = bookRepository; this.userBookProgressRepository = userBookProgressRepository; this.userBookFileProgressRepository = userBookFileProgressRepository; this.shelfRepository = shelfRepository; this.authenticationService = authenticationService; this.mobileBookMapper = mobileBookMapper; + this.bookService = bookService; this.magicShelfBookService = magicShelfBookService; this.entityManager = entityManager; }As per coding guidelines: "Prefer constructor injection via Lombok patterns already used in the codebase."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@booklore-api/src/main/java/org/booklore/app/service/AppBookService.java` around lines 71 - 87, The AppBookService constructor is missing injection of the BookService dependency which leaves the field bookService uninitialized; update the constructor signature to accept a BookService parameter and assign it to this.bookService (i.e., add BookService bookService to the AppBookService(...) parameters and set this.bookService = bookService) so calls like updateBookProgress -> bookService.updateReadProgress(...) use a proper injected instance.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@booklore-api/src/main/java/org/booklore/app/service/AppBookService.java`:
- Around line 71-87: The AppBookService constructor is missing injection of the
BookService dependency which leaves the field bookService uninitialized; update
the constructor signature to accept a BookService parameter and assign it to
this.bookService (i.e., add BookService bookService to the AppBookService(...)
parameters and set this.bookService = bookService) so calls like
updateBookProgress -> bookService.updateReadProgress(...) use a proper injected
instance.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 62f9f400-b6f5-4f83-8d06-d675d7061651
📒 Files selected for processing (5)
booklore-api/src/main/java/org/booklore/app/controller/AppBookController.javabooklore-api/src/main/java/org/booklore/app/dto/UpdateProgressRequest.javabooklore-api/src/main/java/org/booklore/app/service/AppBookService.javabooklore-api/src/test/java/org/booklore/app/service/AppBookServiceFilterOptionsTest.javabooklore-api/src/test/java/org/booklore/app/service/AppBookServiceProgressTest.java
✅ Files skipped from review due to trivial changes (1)
- booklore-api/src/test/java/org/booklore/app/service/AppBookServiceFilterOptionsTest.java
🚧 Files skipped from review as they are similar to previous changes (1)
- booklore-api/src/main/java/org/booklore/app/controller/AppBookController.java
📜 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 (2)
booklore-api/src/**/*.java
📄 CodeRabbit inference engine (AGENTS.md)
booklore-api/src/**/*.java: Use 4-space indentation in Java files and match surrounding Java style
Prefer constructor injection via Lombok patterns already used in the codebase. Do not introduce@Autowiredfield injection
Use MapStruct for entity/DTO mapping
Files:
booklore-api/src/main/java/org/booklore/app/dto/UpdateProgressRequest.javabooklore-api/src/main/java/org/booklore/app/service/AppBookService.javabooklore-api/src/test/java/org/booklore/app/service/AppBookServiceProgressTest.java
booklore-api/src/test/**/*.java
📄 CodeRabbit inference engine (AGENTS.md)
Prefer focused unit tests; use
@SpringBootTestonly when the Spring context is required
Files:
booklore-api/src/test/java/org/booklore/app/service/AppBookServiceProgressTest.java
🧠 Learnings (7)
📚 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:
booklore-api/src/main/java/org/booklore/app/dto/UpdateProgressRequest.javabooklore-api/src/main/java/org/booklore/app/service/AppBookService.javabooklore-api/src/test/java/org/booklore/app/service/AppBookServiceProgressTest.java
📚 Learning: 2026-04-01T17:50:06.817Z
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.
Applied to files:
booklore-api/src/main/java/org/booklore/app/service/AppBookService.java
📚 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 booklore-api/src/**/*.java : Use MapStruct for entity/DTO mapping
Applied to files:
booklore-api/src/main/java/org/booklore/app/service/AppBookService.javabooklore-api/src/test/java/org/booklore/app/service/AppBookServiceProgressTest.java
📚 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:
booklore-api/src/main/java/org/booklore/app/service/AppBookService.java
📚 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:
booklore-api/src/main/java/org/booklore/app/service/AppBookService.java
📚 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 booklore-api/src/test/**/*.java : Prefer focused unit tests; use `SpringBootTest` only when the Spring context is required
Applied to files:
booklore-api/src/test/java/org/booklore/app/service/AppBookServiceProgressTest.java
📚 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 booklore-api/src/**/*.java : Prefer constructor injection via Lombok patterns already used in the codebase. Do not introduce `Autowired` field injection
Applied to files:
booklore-api/src/test/java/org/booklore/app/service/AppBookServiceProgressTest.java
🔇 Additional comments (6)
booklore-api/src/main/java/org/booklore/app/dto/UpdateProgressRequest.java (1)
1-33: LGTM!The DTO is well-structured with appropriate validation. The
@AssertTruevalidation ensures at least one progress field is provided, mirroring the validation pattern inReadProgressRequest. The deprecated annotations on legacy format-specific fields clearly signal the migration path tofileProgress.booklore-api/src/main/java/org/booklore/app/service/AppBookService.java (2)
152-174: LGTM!The
getBookProgressmethod follows the established pattern fromgetBookDetail, with proper authentication, library access validation, and null-safe handling of optional progress entities.
176-196: LGTM!The
updateBookProgressmethod correctly uses the pathbookIdparameter (line 187) as the authoritative source rather than accepting it in the request body, which addresses the prior review feedback. TheUpdateProgressRequestDTO appropriately omitsbookIdentirely, eliminating any ambiguity in the API contract.booklore-api/src/test/java/org/booklore/app/service/AppBookServiceProgressTest.java (3)
57-64: Test assumes the corrected constructor signature.The test correctly constructs
AppBookServicewithbookServiceas the 7th parameter. This confirms the intended fix for the missing constructor parameter in the service class.
70-154: Good test coverage for the new progress update flow.The tests appropriately cover:
- Path
bookIdis authoritative (verifying the fix for the prior review concern)- Field mapping from
UpdateProgressRequesttoReadProgressRequest- Access control for non-admin users (both forbidden and permitted cases)
- Book-not-found error handling
This addresses the prior review request for "focused test coverage for this new flow."
160-188: LGTM!Helper methods are well-factored and correctly model the authentication and authorization scenarios needed for the tests.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
booklore-api/src/main/java/org/booklore/app/service/AppBookService.java (2)
154-176: Consider extracting shared book-access logic to reduce duplication.This method shares most of its logic with
getBookDetail(lines 131-152): authentication, library access check, and progress fetching. Additionally, the inline access check here differs fromupdateBookProgresswhich usesvalidateLibraryAccess.A small helper could consolidate this:
♻️ Suggested helper extraction
private BookEntity fetchBookWithAccessCheck(Long bookId) { BookLoreUser user = authenticationService.getAuthenticatedUser(); Set<Long> accessibleLibraryIds = getAccessibleLibraryIds(user); BookEntity book = bookRepository.findByIdWithBookFiles(bookId) .orElseThrow(() -> ApiError.BOOK_NOT_FOUND.createException(bookId)); validateLibraryAccess(accessibleLibraryIds, book.getLibrary().getId()); return book; }Then both
getBookProgressandgetBookDetailcan call this helper, reducing duplication and ensuring consistent access validation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@booklore-api/src/main/java/org/booklore/app/service/AppBookService.java` around lines 154 - 176, Extract the repeated authentication, book loading and access check into a helper (e.g., fetchBookWithAccessCheck) and replace the duplicated logic in getBookProgress and getBookDetail with calls to that helper; the helper should use authenticationService to get the user, call getAccessibleLibraryIds(user), load the BookEntity via bookRepository.findByIdWithBookFiles(bookId) (throw ApiError.BOOK_NOT_FOUND if missing) and then call the existing validateLibraryAccess(accessibleLibraryIds, book.getLibrary().getId()) to enforce consistent access validation before returning the BookEntity.
200-200: Redundant@Transactionalannotation.The class-level
@Transactional(readOnly = true)at line 50 already applies to this method. This method-level annotation can be removed.🧹 Proposed removal
- `@Transactional`(readOnly = true) public AppPageResponse<AppBookSummary> searchBooks(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@booklore-api/src/main/java/org/booklore/app/service/AppBookService.java` at line 200, Remove the redundant method-level `@Transactional`(readOnly = true) annotation in the AppBookService class: the class already carries `@Transactional`(readOnly = true) so delete the duplicate annotation applied to the method (the method-level `@Transactional` at the shown location) and leave the method signature intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@booklore-api/src/main/java/org/booklore/app/service/AppBookService.java`:
- Around line 154-176: Extract the repeated authentication, book loading and
access check into a helper (e.g., fetchBookWithAccessCheck) and replace the
duplicated logic in getBookProgress and getBookDetail with calls to that helper;
the helper should use authenticationService to get the user, call
getAccessibleLibraryIds(user), load the BookEntity via
bookRepository.findByIdWithBookFiles(bookId) (throw ApiError.BOOK_NOT_FOUND if
missing) and then call the existing validateLibraryAccess(accessibleLibraryIds,
book.getLibrary().getId()) to enforce consistent access validation before
returning the BookEntity.
- Line 200: Remove the redundant method-level `@Transactional`(readOnly = true)
annotation in the AppBookService class: the class already carries
`@Transactional`(readOnly = true) so delete the duplicate annotation applied to
the method (the method-level `@Transactional` at the shown location) and leave the
method signature intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 816ab2b0-d03f-4481-bfba-4740e060738d
📒 Files selected for processing (1)
booklore-api/src/main/java/org/booklore/app/service/AppBookService.java
📜 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). (3)
- GitHub Check: Test Suite / Backend Tests
- GitHub Check: Test Suite / Frontend Tests
- GitHub Check: Analyze (java-kotlin)
🧰 Additional context used
📓 Path-based instructions (1)
booklore-api/src/**/*.java
📄 CodeRabbit inference engine (AGENTS.md)
booklore-api/src/**/*.java: Use 4-space indentation in Java files and match surrounding Java style
Prefer constructor injection via Lombok patterns already used in the codebase. Do not introduce@Autowiredfield injection
Use MapStruct for entity/DTO mapping
Files:
booklore-api/src/main/java/org/booklore/app/service/AppBookService.java
🧠 Learnings (5)
📚 Learning: 2026-04-01T17:50:06.817Z
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.
Applied to files:
booklore-api/src/main/java/org/booklore/app/service/AppBookService.java
📚 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 booklore-api/src/**/*.java : Use MapStruct for entity/DTO mapping
Applied to files:
booklore-api/src/main/java/org/booklore/app/service/AppBookService.java
📚 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:
booklore-api/src/main/java/org/booklore/app/service/AppBookService.java
📚 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:
booklore-api/src/main/java/org/booklore/app/service/AppBookService.java
📚 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:
booklore-api/src/main/java/org/booklore/app/service/AppBookService.java
🔇 Additional comments (2)
booklore-api/src/main/java/org/booklore/app/service/AppBookService.java (2)
62-62: LGTM!Clean constructor injection for
BookServicefollowing the existing pattern in this service.Also applies to: 77-77, 86-86
188-197: The code correctly copies all available fields fromUpdateProgressRequesttoReadProgressRequest. Neither class includes akoreaderProgressfield—KoReader progress is handled separately via the dedicatedKoreaderControllerendpoint. No data is being lost on update.
zachyale
approved these changes
Apr 14, 2026
Member
|
@afairgiant Thanks for the contribution- sorry for the delay on re-review 🚀 |
zachyale
pushed a commit
to zachyale/grimmory
that referenced
this pull request
Apr 17, 2026
* feat: add reading progress endpoints to mobile api * fix(app-api): replace ReadProgressRequest with app-specific DTO for progress endpoint * fix(app-api): missing BookService dependecy for constructor
zachyale
pushed a commit
to zachyale/grimmory
that referenced
this pull request
Apr 17, 2026
* feat: add reading progress endpoints to mobile api * fix(app-api): replace ReadProgressRequest with app-specific DTO for progress endpoint * fix(app-api): missing BookService dependecy for constructor
zachyale
pushed a commit
that referenced
this pull request
Apr 17, 2026
* feat: add reading progress endpoints to mobile api * fix(app-api): replace ReadProgressRequest with app-specific DTO for progress endpoint * fix(app-api): missing BookService dependecy for constructor
zachyale
pushed a commit
that referenced
this pull request
Apr 22, 2026
* feat: add reading progress endpoints to mobile api * fix(app-api): replace ReadProgressRequest with app-specific DTO for progress endpoint * fix(app-api): missing BookService dependecy for constructor
dsmouse
pushed a commit
to dsmouse/grimmory
that referenced
this pull request
May 11, 2026
* feat: add reading progress endpoints to mobile api * fix(app-api): replace ReadProgressRequest with app-specific DTO for progress endpoint * fix(app-api): missing BookService dependecy for constructor
This was referenced May 26, 2026
2 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This pull request adds app API support for retrieving and updating a user's reading progress for a specific book in Grimmory. It introduces new endpoints, a response DTO, and supporting service and mapping logic to handle book progress data in a structured way.
Linked Issue: Fixes #364
Changes
API Enhancements:
AppBookController: one for fetching (GET /{bookId}/progress) and one for updating (PUT /{bookId}/progress) the reading progress of a book, using the newAppBookProgressResponseDTO andReadProgressRequestrespectively.Data Transfer Object (DTO) Additions:
AppBookProgressResponseto encapsulate reading progress details, including progress metrics for various book formats and last read time.Service Layer Updates:
getBookProgressandupdateBookProgressmethods toAppBookServiceto handle fetching and updating user reading progress, with appropriate access validation and delegation toBookServicefor updates.Mapping Improvements:
toProgressResponsemethod inAppBookMapperto map progress entities to the new response DTO.Dependency Injection and Testing:
BookServiceas a dependency inAppBookServiceand updated related tests to mock and inject this dependency. [1] [2] [3]Summary by CodeRabbit
New Features
Tests