fix(caching): add Cache-Control headers to responses for improved caching behavior#802
Closed
balazs-szucs wants to merge 4 commits intogrimmory-tools:developfrom
Closed
Conversation
Contributor
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAdds app-facing endpoints (bootstrap, dashboard, book context), new DTOs and services for bootstrap, dashboard scrollers, book context and menu counts, applies Cache-Control headers, refactors AppBookService query/scoping/specifications, updates security to allow unauthenticated bootstrap, and adds tests for menu counts. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Controller as AppBookContextController
participant Auth as AuthenticationService
participant AppBookSvc as AppBookService
participant ContextSvc as AppBookContextService
participant DB as Repositories/DB
Client->>Controller: GET /api/v1/app/books/{bookId}/context
Controller->>Auth: getAuthenticatedUser()
Auth-->>Controller: BookLoreUser (or null)
alt user present
Controller->>AppBookSvc: getBookDetail(bookId, userId)
AppBookSvc->>DB: query book detail (repositories)
DB-->>AppBookSvc: Book detail
AppBookSvc-->>Controller: AppBookDetail
Controller->>ContextSvc: getBookContext(bookId, userId)
ContextSvc->>DB: fetch viewer preference entities
DB-->>ContextSvc: preference entities
ContextSvc-->>Controller: AppBookContextResponse (preferences)
else anonymous
Controller->>AppBookSvc: getBookDetail(bookId, null)
AppBookSvc->>DB: query public book detail
DB-->>AppBookSvc: Book detail
AppBookSvc-->>Controller: AppBookDetail
Controller->>ContextSvc: getBookContext(bookId, null)
ContextSvc-->>Controller: AppBookContextResponse (null prefs)
end
Controller->>Client: 200 OK + Cache-Control: private, max-age=60 + body
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
backend/src/main/java/org/booklore/service/ShelfService.java (1)
113-116:⚠️ Potential issue | 🟠 MajorNPE risk:
getAuthenticatedUserId()no longer null-safe.Since
authenticationService.getAuthenticatedUser()can now returnnull(seeAuthenticationService.getAuthenticatedUserin this PR),user.getId()here will throwNullPointerExceptionfor unauthenticated callers. This propagates intocreateShelf(line 44),getUserKoboShelf(line 101), and any other caller of this helper, turning an expected auth failure into a 500.🛡️ Suggested fix
private Long getAuthenticatedUserId() { BookLoreUser user = authenticationService.getAuthenticatedUser(); - return user.getId(); + if (user == null || user.getId() == null) { + throw new AccessDeniedException("Authentication required"); + } + return user.getId(); }🤖 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/ShelfService.java` around lines 113 - 116, getAuthenticatedUserId() is not null-safe now that authenticationService.getAuthenticatedUser() can return null; change it to check for null and surface a proper authentication failure instead of letting user.getId() throw an NPE. In the getAuthenticatedUserId method (and callers like createShelf and getUserKoboShelf), detect when authenticationService.getAuthenticatedUser() returns null and throw the appropriate auth/unauthorized exception used in the project (or return an Optional/explicit error) so unauthenticated callers receive a 401/403 rather than a 500; update method behavior and callers accordingly to handle the explicit auth failure.backend/src/main/java/org/booklore/app/controller/AppBookController.java (1)
31-48:⚠️ Potential issue | 🟡 MinorReview cache TTL for personalized book lists that mutate immediately.
GET /api/v1/app/booksand/books/idsreturn user-filtered book lists and applyCache-Control: private, max-age=60. Both endpoints retrieve data based on the authenticated user's ID viaauthenticationService.getAuthenticatedUser()and include user-specific filters and progress data.However, mutations via
PUT /{bookId}/progress,PUT /{bookId}/status, andPUT /{bookId}/ratingdirectly persist changes toUserBookProgressRepositorysynchronously. SincegetBooks()queries the current user's progress, these updates immediately affect what the list endpoints would return. With the 60-second TTL and no cache invalidation mechanism, clients may serve stale results for up to a minute after updates (e.g., just-rated books not appearing in filters, progress not reflected in lists).Additionally, similar user-scoped endpoints like
/continue-reading,/continue-listening,/recently-added,/recently-scanned, and/random— which all callauthenticationService.getAuthenticatedUser()and filter by user ID — have no cache headers at all, creating an inconsistent caching strategy.Consider:
- Reducing
max-ageto 0–15 seconds or usingno-cachewith ETag/Last-Modified for client revalidation- Adding
must-revalidatewith ETag to ensure mutations invalidate stale caches- Standardizing cache headers across all user-scoped endpoints
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/main/java/org/booklore/app/controller/AppBookController.java` around lines 31 - 48, The current cache headers on user-scoped list endpoints (getBooks and getAllBookIds) allow up to 60s of private caching while those endpoints (mobileBookService.getBooks, mobileBookService.getAllBookIds) read user-specific data via authenticationService.getAuthenticatedUser(), causing stale results after synchronous mutations (PUT /{bookId}/progress, /status, /rating); update the controller methods that return user-scoped lists (getBooks, getAllBookIds and other user-scoped endpoints like /continue-reading, /continue-listening, /recently-added, /recently-scanned, /random) to use a consistent conservative caching policy—e.g., set Cache-Control to no-cache or private, max-age=5 (or 0–15s) with must-revalidate and add ETag/Last-Modified support so clients revalidate after mutations (and ensure any existing cache headers are standardized across these methods).backend/src/main/java/org/booklore/config/security/service/AuthenticationService.java (1)
82-96:⚠️ Potential issue | 🔴 CriticalUnprotected endpoints will NullPointerException on unauthenticated access due to null-unsafe dereference chain.
The return-type contract of
getAuthenticatedUser()now permits null (returns null on lines 85, 95), but multiple unprotected endpoints dereference the result without guards:
AppShelfController.getShelves()andgetMagicShelves()(lines 46-47, 65-66): no@PreAuthorize, directuser.getId()call → will NPE for unauthenticated requests.ShelfService.getAuthenticatedUserId()(line 115): private method chainsuser.getId()without null check; used bycreateShelf()andgetUserKoboShelf().KoreaderUserService.upsertUser()(lines 31, 52): chained dereference without guard.BookNoteService(lines 36, 49): same issue.- Similar patterns across
NotebookService,CustomFontController,TaskService, etc.Endpoints like the "app" shelves endpoints appear intentionally unprotected for bootstrap/anonymous access, making this a critical gap. Recommended fix:
- Preferred: Retain the prior exception-throwing behavior for authenticated-only paths; add a separate
Optional<BookLoreUser> tryGetAuthenticatedUser()for endpoints that tolerate null.- Alternative: Change return type to
Optional<BookLoreUser>and audit all 80+ call sites to add proper null handling.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/main/java/org/booklore/config/security/service/AuthenticationService.java` around lines 82 - 96, The current getAuthenticatedUser() allows null and causes NPEs in unprotected endpoints; restore the original throwing behavior for auth-required paths and add a new null-tolerant accessor. Change getAuthenticatedUser() (AuthenticationService.getAuthenticatedUser) to throw an AuthenticationException or IllegalStateException when authentication is missing/unauthenticated (as before), and implement a new tryGetAuthenticatedUser() or Optional<BookLoreUser> tryGetAuthenticatedUser() that returns Optional.empty() when unauthenticated (and performs defaultSettingInitializer.ensureDefaultSettings(user) only when a user is present). Then update call sites that must tolerate anonymous access (e.g., AppShelfController.getShelves/getMagicShelves, ShelfService.getAuthenticatedUserId, KoreaderUserService.upsertUser, BookNoteService methods, NotebookService, CustomFontController, TaskService, etc.) to use tryGetAuthenticatedUser() and handle Optional.empty() instead of dereferencing a possibly-null user.backend/src/main/java/org/booklore/app/service/AppBookService.java (1)
917-922:⚠️ Potential issue | 🟡 Minor
matchScorebypassescleanValues()sanitization — likely unintentional.Every other multi-value filter in
buildSpecificationcallsBookListRequest.cleanValues(req.xxx())to trim/strip empty entries before checkingisEmpty()and passing to the spec. The new branch formatchScoreuses the raw list:List<String> cleaned = req.matchScore(); // no cleanValues() if (!cleaned.isEmpty()) { ... }This leaves the variable misnamed and lets whitespace/empty strings through to
withMatchScores, potentially producing empty-bucket matches or breaking theCASEbucket lookup. Please align with the sibling filters unless there is a specific reason to skip cleaning here.🛠️ Proposed fix
- if (req.matchScore() != null && !req.matchScore().isEmpty()) { - List<String> cleaned = req.matchScore(); - if (!cleaned.isEmpty()) { - specs.add(AppBookSpecification.withMatchScores(cleaned, req.effectiveFilterMode())); - } - } + if (req.matchScore() != null && !req.matchScore().isEmpty()) { + List<String> cleaned = BookListRequest.cleanValues(req.matchScore()); + if (!cleaned.isEmpty()) { + specs.add(AppBookSpecification.withMatchScores(cleaned, req.effectiveFilterMode())); + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/main/java/org/booklore/app/service/AppBookService.java` around lines 917 - 922, The matchScore branch in AppBookService (buildSpecification) bypasses sanitization—use BookListRequest.cleanValues(req.matchScore()) instead of taking req.matchScore() raw; assign the cleaned list to the variable (e.g., cleaned), check isEmpty() on that cleaned list, and pass the cleaned list into AppBookSpecification.withMatchScores(cleaned, req.effectiveFilterMode()) so whitespace/empty entries are removed just like the other multi-value filters.
🧹 Nitpick comments (11)
backend/src/main/java/org/booklore/controller/AuthenticationController.java (1)
64-77: Redundant try/catch; removing@Validdrops bean-validation.Two small concerns:
- The try/catch only logs
e.getMessage()then rethrows — it adds no behavior and risks log noise (the global exception handler will log/translate theAPIExceptionanyway). Either drop it, or actually translate the exception into a response here.- Removing
@ValidfromRefreshTokenRequestdisables any bean-validation annotations on that DTO (e.g.@NotBlank). The manual null/blank check only coversrefreshToken; other fields that might be added later won't be validated. Consider keeping@Validalongside the explicit guard.♻️ Proposed simplification
- public ResponseEntity<?> refreshToken(`@RequestBody` RefreshTokenRequest request) { - log.debug("[Auth] Refresh request received. Request object exists: {}", request != null); + public ResponseEntity<?> refreshToken(`@Valid` `@RequestBody` RefreshTokenRequest request) { if (request == null || request.getRefreshToken() == null || request.getRefreshToken().isBlank()) { log.warn("[Auth] Refresh request failed: missing or empty refresh token"); return ResponseEntity.badRequest().body(Map.of("message", "Refresh token is missing")); } - - try { - return authenticationService.refreshToken(request.getRefreshToken()); - } catch (Exception e) { - log.error("[Auth] Token refresh failed: {}", e.getMessage()); - throw e; - } + return authenticationService.refreshToken(request.getRefreshToken()); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/main/java/org/booklore/controller/AuthenticationController.java` around lines 64 - 77, Remove the redundant try/catch from the refreshToken method and re-enable bean validation on the request: keep the `@Valid` annotation on the RefreshTokenRequest parameter so DTO-level constraints (e.g., `@NotBlank`) are enforced, and delete the try/catch that only logs e.getMessage() then rethrows (or alternatively, replace it with logic that maps specific exceptions from authenticationService.refreshToken(...) to appropriate ResponseEntity responses if you want local translation); ensure the method signature is public ResponseEntity<?> refreshToken(`@Valid` `@RequestBody` RefreshTokenRequest request) and rely on the global exception handler for other exceptions.backend/src/main/java/org/booklore/service/library/LibraryService.java (1)
234-250: LGTM — null-safe user scoping.The overload cleanly separates the authenticated-lookup from the permission-based scoping and returns an empty list for unauthenticated callers, which is consistent with the new
AppBootstrapServiceflow.Minor nit (optional): admin branch performs an unnecessary
findByIdWithLibrariesprior tofindAll(). Could short-circuit by checking admin via the already-loadeduser.getPermissions()before hitting the repo.🤖 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/library/LibraryService.java` around lines 234 - 250, The method getLibraries(BookLoreUser user) currently always fetches BookLoreUserEntity via userRepository.findByIdWithLibraries(user.getId()) even for admins; change the logic to check admin status on the incoming BookLoreUser (user.getPermissions().isPermissionAdmin()) first and short-circuit to libraryRepository.findAll() for admins without calling findByIdWithLibraries; only call userRepository.findByIdWithLibraries(...) and map userEntity.getLibraries() when the caller is not an admin, then map entities via libraryMapper::toLibrary as before.backend/src/main/java/org/booklore/app/controller/AppBootstrapController.java (1)
14-14: Nit:@Slf4jis unused in this controller.No logging is performed in
AppBootstrapController, so the@Slf4jannotation can be removed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/main/java/org/booklore/app/controller/AppBootstrapController.java` at line 14, Remove the unused Lombok logging annotation from the controller: delete the `@Slf4j` annotation (and the corresponding import lombok.extern.slf4j.Slf4j if present) from AppBootstrapController since no logging calls use the generated "log" field; then run a build to confirm there are no remaining references to "log" in that class.backend/src/test/java/org/booklore/app/service/AppBookServiceFilterOptionsTest.java (1)
222-225: Nit: importLibraryEntityinstead of using FQN.
LibraryEntityis used only in this helper and was added via fully-qualified name. Adding animport org.booklore.model.entity.LibraryEntity;next to the existing entity imports would be more consistent with the rest of the file.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/test/java/org/booklore/app/service/AppBookServiceFilterOptionsTest.java` around lines 222 - 225, The test uses the fully-qualified class name org.booklore.model.entity.LibraryEntity in the when(...) stub; replace that FQN with a simple import for LibraryEntity and update the three occurrences in the helper so the lines read LibraryEntity.builder()..., keeping the same builder calls and the when(libraryRepository.findAll()) stub unchanged; add import org.booklore.model.entity.LibraryEntity; near the other entity imports to match file style.backend/src/main/java/org/booklore/app/service/AppBootstrapService.java (1)
43-43: Consider avoiding the username in error log for PII hygiene.
user.getUsername()is user-identifying PII and ends up in error logs for any transient failure in downstream services. If these logs ship to aggregators, that's a low-level privacy exposure. Consider logginguser.getId()instead (or alongside) to enable debugging without persisting the username.- log.error("[Bootstrap] Failed to fetch complete bootstrap data for user {}: {}", user.getUsername(), e.getMessage(), e); + log.error("[Bootstrap] Failed to fetch complete bootstrap data for user id {}: {}", user.getId(), e.getMessage(), 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/app/service/AppBootstrapService.java` at line 43, The error log in AppBootstrapService currently logs user.getUsername(), which exposes PII; update the log to avoid the username—use user.getId() instead (or include both id and username but redact/omit the username) in the log call where log.error("[Bootstrap] Failed to fetch complete bootstrap data for user {}: {}", ... ) is invoked so debugging retains an identifier without persisting the username.backend/src/main/java/org/booklore/service/MenuCountsService.java (2)
21-32: Redundant wildcard import.
import java.util.*;on line 21 makes the specific imports on lines 28–32 (Collections,LinkedHashMap,Map,Set) redundant. Keep only the specific ones (matches the surrounding code style in this repo).🤖 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/MenuCountsService.java` around lines 21 - 32, Remove the redundant wildcard import "import java.util.*;" from MenuCountsService.java and keep the explicit java.util imports already listed (e.g., Collections, LinkedHashMap, Map, Set, and stream.Collectors); ensure there are no other missing java.util types used in methods such as any in MenuCountsService so compilation still passes and import ordering/style matches the repo.
81-103: Consider using a Specification/Criteria count for library counts for consistency.
computeShelfCountsand the totals already use thevisibleBooksSpecSpecification, butcomputeLibraryCountsre-implements the "visible books" predicate as JPQL string concatenation with(b.deleted IS NULL OR b.deleted = false). IfAppBookSpecification.notDeleted()ever changes (e.g., adds anarchivedcheck), the sidebar will silently drift. Consider a Criteria-based grouped count that usesvisibleBooksSpec.toPredicate(...)likecomputeShelfCountsdoes.🤖 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/MenuCountsService.java` around lines 81 - 103, computeLibraryCounts currently builds a JPQL string with an ad-hoc deleted check instead of reusing the existing Specification logic; replace the JPQL approach with a Criteria/Specification-based grouped count similar to computeShelfCounts by building a CriteriaQuery<Tuple> (or CriteriaQuery<Object[]>) that applies visibleBooksSpec.toPredicate(root, query, cb) (or uses AppBookSpecification.notDeleted() via the same mechanism) and then groups by root.get("library").get("id") to produce counts, set the accessibleLibraryIds predicate the same way computeShelfCounts does, execute the criteria query via entityManager and populate the counts Map; ensure you reference computeLibraryCounts, computeShelfCounts, visibleBooksSpec (or AppBookSpecification.notDeleted()), and entityManager when locating code to change.backend/src/main/java/org/booklore/app/controller/AppBookContextController.java (1)
51-58: Nit: Use method references.♻️ Proposed refactor
- .pdfSettings(pdfPrefsRepo.findByBookIdAndUserId(bookId, userId) - .map(e -> bookMapper.toPdfViewerPreferences(e)).orElse(null)) - .newPdfSettings(newPdfPrefsRepo.findByBookIdAndUserId(bookId, userId) - .map(e -> bookMapper.toNewPdfViewerPreferences(e)).orElse(null)) - .ebookSettings(ebookPrefsRepo.findByBookIdAndUserId(bookId, userId) - .map(e -> bookMapper.toEbookViewerPreferences(e)).orElse(null)) - .cbxSettings(cbxPrefsRepo.findByBookIdAndUserId(bookId, userId) - .map(e -> bookMapper.toCbxViewerPreferences(e)).orElse(null)) + .pdfSettings(pdfPrefsRepo.findByBookIdAndUserId(bookId, userId) + .map(bookMapper::toPdfViewerPreferences).orElse(null)) + .newPdfSettings(newPdfPrefsRepo.findByBookIdAndUserId(bookId, userId) + .map(bookMapper::toNewPdfViewerPreferences).orElse(null)) + .ebookSettings(ebookPrefsRepo.findByBookIdAndUserId(bookId, userId) + .map(bookMapper::toEbookViewerPreferences).orElse(null)) + .cbxSettings(cbxPrefsRepo.findByBookIdAndUserId(bookId, userId) + .map(bookMapper::toCbxViewerPreferences).orElse(null))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/main/java/org/booklore/app/controller/AppBookContextController.java` around lines 51 - 58, Replace the lambda maps with method references to simplify the mapping calls: change .map(e -> bookMapper.toPdfViewerPreferences(e)) to .map(bookMapper::toPdfViewerPreferences) (and similarly use bookMapper::toNewPdfViewerPreferences, bookMapper::toEbookViewerPreferences, bookMapper::toCbxViewerPreferences) on the results of pdfPrefsRepo.findByBookIdAndUserId, newPdfPrefsRepo.findByBookIdAndUserId, ebookPrefsRepo.findByBookIdAndUserId, and cbxPrefsRepo.findByBookIdAndUserId respectively.backend/src/main/java/org/booklore/app/controller/AppDashboardController.java (1)
38-84: LGTM with a minor note on scroller type casing.Supporting both
camelCaseandUPPER_CASEin the switch is a nice back-compat hedge, and the unknown-type/nullfallbacks log and return empty lists rather than failing the whole dashboard — good resilience. The per-scroller error isolation could be tightened further by wrapping eachswitchbranch in try/catch similar toMenuCountsService.computeMagicShelfCounts, so one failing scroller doesn't fail the whole dashboard response; optional.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/main/java/org/booklore/app/controller/AppDashboardController.java` around lines 38 - 84, The per-scroller fetch in AppDashboardController.getDashboard currently runs all scrollers inside a single loop without isolating exceptions, so an exception in mobileBookService (or any branch) can abort the entire dashboard; update the loop that iterates config.getScrollers() to wrap the switch/fetch logic for each scroller in a try/catch (catch Exception) and on error log with scroller id/type and continue, then put an empty list into scrollerData for that scroller (matching existing fallback behavior); use the same error-isolation pattern as MenuCountsService.computeMagicShelfCounts and reference getDashboard, resolveDashboardConfig, mobileBookService and scroller.getId()/getType()/getMaxItems()/getMagicShelfId() when implementing.backend/src/test/java/org/booklore/service/MenuCountsServiceTest.java (1)
90-149: Mock setup complexity signals missing integration coverage.The ~50 lines of
CriteriaBuilder/CriteriaQuery/Root/Join/Pathmocking duplicated across three tests (lines 101–130, 167–189, 217–241) is brittle — any change tocomputeShelfCounts's criteria wiring forces simultaneous updates in every test. Consider complementing these with a@DataJpaTest-backed test that exercises the real JPQL/Criteria paths against an in-memory DB; the mock-based unit tests then only need to verify the service's orchestration logic, not Criteria builder call sequences.🤖 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/MenuCountsServiceTest.java` around lines 90 - 149, The test over-mocks JPA Criteria plumbing (CriteriaBuilder/CriteriaQuery/Root/Join/Path) for MenuCountsServiceTest which is brittle; instead add a complementary integration test using a `@DataJpaTest` (or `@SpringBootTest` with an in-memory DB) that persists sample BookEntity/Library/Shelf rows and calls MenuCountsService.getMenuCounts (and its helper computeShelfCounts) to exercise the real JPQL/Criteria paths, keeping the existing unit tests but simplifying them to only assert orchestration (mocking bookRepository.count and magicShelfBookService.toSpecification); this requires creating fixtures for libraries/shelves/books, wiring the real EntityManager/BookRepository, and asserting libraryCounts, shelfCounts, magicShelfCounts, totalBookCount and unshelvedBookCount against persisted data.backend/src/main/java/org/booklore/app/service/AppBookService.java (1)
767-779: Admin scoping now issuesfindAll()on every request — project to IDs and consider caching.
getAccessibleLibraryIdsis invoked from virtually every public endpoint in this service (getBooks,getAllBookIds,getBookDetail,getBookProgress,updateBookProgress,searchBooks,getContinueReading,getContinueListening,getRecentlyAdded,getRecentlyScanned,getRandomBooks,getFilterOptions,validateAccessAndGetProgress). For admins this now fetches and hydrates everyLibraryEntity(plus any eagerly fetched associations) on each call just to extract IDs.Two recommendations:
- Use a projection query (e.g.,
libraryRepository.findAllIds()returningList<Long>) to avoid hydrating entities.- Consider a short-lived cache (similar to
filterOptionsCache) or reuse theaccessibleLibraryIdscomputed once per request to avoid redundant DB hits withingetFilterOptions→ downstream helpers.♻️ Sketch: projection + Caffeine cache
// LibraryRepository `@Query`("SELECT l.id FROM LibraryEntity l") List<Long> findAllIds();private Set<Long> getAccessibleLibraryIds(BookLoreUser user) { if (user.getPermissions().isAdmin()) { - return libraryRepository.findAll().stream() - .map(LibraryEntity::getId) - .collect(Collectors.toSet()); + return new HashSet<>(libraryRepository.findAllIds()); } ... }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/main/java/org/booklore/app/service/AppBookService.java` around lines 767 - 779, getAccessibleLibraryIds currently calls libraryRepository.findAll() for admins and hydrates full LibraryEntity objects on every request; change it to use a projection query (e.g., add libraryRepository.findAllIds() that returns List<Long> or Set<Long> and map those to the returned Set) to avoid entity hydration, and add a short-lived cache or reuse a per-request computed set (similar to filterOptionsCache) so repeated calls within the same request (e.g., getFilterOptions → downstream helpers) don’t hit the DB repeatedly; update references in getAccessibleLibraryIds to use the new repository method and consider storing the result in a transient accessibleLibraryIds variable for the request lifecycle.
🤖 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/app/controller/AppBookContextController.java`:
- Around line 29-35: AppBookContextController currently injects
PdfViewerPreferencesRepository, NewPdfViewerPreferencesRepository,
EbookViewerPreferenceRepository, and CbxViewerPreferencesRepository directly;
extract those repository reads into a new service (e.g. AppBookContextService)
with a method getBookContext(bookId, userId) that performs all four preference
lookups inside a single `@Transactional`(readOnly = true) method and returns a
composed context (use BookMapper if needed). Then remove the four repository
fields from AppBookContextController and delegate calls to
AppBookContextService.getBookContext(...) from the controller (keeping
mobileBookService, authenticationService, and bookMapper usage as appropriate).
Ensure AppBookContextService is injected into the controller and that
transactionality and layering are preserved.
In `@backend/src/main/java/org/booklore/app/service/AppBookService.java`:
- Line 53: Remove the unused UserRepository dependency from AppBookService:
delete the private field userRepository, remove the corresponding UserRepository
parameter from the AppBookService constructor, and remove the assignment that
sets this.userRepository = userRepository; then update any tests or wiring that
pass a UserRepository into AppBookService (e.g., test fixtures, mocks, or
configuration) so they no longer supply this argument. Ensure constructor
signature and imports are cleaned up accordingly.
- Line 1060: The current sort mapping case "lastreadtime" -> "lastReadTime" is
invalid because BookEntity has no lastReadTime field (the timestamp lives on
UserBookProgressEntity), so either remove the "lastreadtime" sort option from
the switch in AppBookService (delete the case mapping for "lastreadtime"), or
implement one of the supported fixes: add a transient/derived lastReadTime on
BookEntity (e.g., via `@Formula` or a getter that derives the value from
UserBookProgressEntity) or replace the generic sort handling with a custom
repository `@Query` that joins BookEntity to UserBookProgressEntity and sorts by
the appropriate aggregated progress timestamp; update any consumers/tests
accordingly.
In
`@backend/src/main/java/org/booklore/config/security/filter/AuthenticationCheckFilter.java`:
- Around line 21-33: AuthenticationCheckFilter uses brittle string checks
(path.contains and endsWith) to skip auth for the bootstrap endpoint; replace
this with the same path-matching mechanism used by SecurityConfig (e.g., inject
or construct a RequestMatcher/AntPathRequestMatcher or AntPathMatcher that
matches "/api/v1/app/bootstrap" exactly and respects the servlet context) and
use that matcher in AuthenticationCheckFilter instead of path.contains/endsWith
so the check aligns with SecurityConfig and only skips auth for the exact
AppBootstrapController route; keep the existing chain.doFilter(request,
response) behavior when the matcher matches.
In `@backend/src/main/java/org/booklore/config/security/SecurityConfig.java`:
- Line 286: The SecurityConfig currently uses
.requestMatchers("/api/v1/app/bootstrap") which only allows the exact path while
AuthenticationCheckFilter bypass logic includes subpaths; make them consistent
by either changing .requestMatchers(...) to
.requestMatchers("/api/v1/app/bootstrap/**") to permit the bootstrap endpoint
and any subpaths, or adjust the AuthenticationCheckFilter bypass to only match
the exact "/api/v1/app/bootstrap" path; update the code paths where
requestMatchers("/api/v1/app/bootstrap") and the AuthenticationCheckFilter
bypass are defined so both use the same scope.
- Around line 277-283: SecurityConfig currently disables Spring Security's
default cache-control headers for the entire /api/** chain (and similarly for
the WS chain), which can expose authenticated responses to caching; instead,
restore the default cache-control behavior for the general API/WS HttpSecurity
configuration and only disable or alter cache-control for the specific mobile
endpoints that must be client-cacheable (e.g., create a separate HttpSecurity
matcher for the mobile path such as the /api/v1/app/** request matcher or the
specific filter chain that handles mobile requests and apply
cacheControl(...disable) only there), or simply remove the global
.cacheControl(...disable) call in the API/WS chains and rely on the few
endpoints that already set ResponseEntity.ok().header("Cache-Control", ...) to
override the default headers; update SecurityConfig's header configuration
accordingly and mirror the change for the WS chain referenced in the review.
In `@backend/src/main/java/org/booklore/mapper/BookMapper.java`:
- Around line 41-44: The three mapper methods PdfViewerPreferences
toPdfViewerPreferences(PdfViewerPreferencesEntity), NewPdfViewerPreferences
toNewPdfViewerPreferences(NewPdfViewerPreferencesEntity), and
CbxViewerPreferences toCbxViewerPreferences(CbxViewerPreferencesEntity) are
dropping the entity.userId (causing silent data loss under
unmappedTargetPolicy=IGNORE); fix by either adding a userId property to the
corresponding DTOs (PdfViewerPreferences, NewPdfViewerPreferences,
CbxViewerPreferences) so MapStruct can map it automatically, or explicitly
annotate each mapper method with `@Mapping`(target = "userId", ignore = true) to
document intentional exclusion—update whichever of the two approaches you choose
consistently across those three mapper methods/entities.
In `@backend/src/test/java/org/booklore/service/MenuCountsServiceTest.java`:
- Around line 108-120: The test contains duplicate Mockito stubbings that
overwrite each other—remove the repeated stubs and keep a single consistent stub
for each interaction: replace the two cb.count(any()) lines with one that
returns a single shared Expression mock, and remove the duplicated
when(cq.multiselect(...)), when(cq.where(...)) and when(cq.groupBy(...))
duplicates so each of cq.multiselect, cq.where and cq.groupBy is stubbed only
once; ensure the remaining stubs still include
when(entityManager.getCriteriaBuilder()), when(cb.createTupleQuery()),
when(cq.from(BookEntity.class)), when(root.join("shelves")),
when(shelfJoin.get("id")) and the single multiselect/where/groupBy stubs so
behavior is unchanged but not overwritten.
---
Outside diff comments:
In `@backend/src/main/java/org/booklore/app/controller/AppBookController.java`:
- Around line 31-48: The current cache headers on user-scoped list endpoints
(getBooks and getAllBookIds) allow up to 60s of private caching while those
endpoints (mobileBookService.getBooks, mobileBookService.getAllBookIds) read
user-specific data via authenticationService.getAuthenticatedUser(), causing
stale results after synchronous mutations (PUT /{bookId}/progress, /status,
/rating); update the controller methods that return user-scoped lists (getBooks,
getAllBookIds and other user-scoped endpoints like /continue-reading,
/continue-listening, /recently-added, /recently-scanned, /random) to use a
consistent conservative caching policy—e.g., set Cache-Control to no-cache or
private, max-age=5 (or 0–15s) with must-revalidate and add ETag/Last-Modified
support so clients revalidate after mutations (and ensure any existing cache
headers are standardized across these methods).
In `@backend/src/main/java/org/booklore/app/service/AppBookService.java`:
- Around line 917-922: The matchScore branch in AppBookService
(buildSpecification) bypasses sanitization—use
BookListRequest.cleanValues(req.matchScore()) instead of taking req.matchScore()
raw; assign the cleaned list to the variable (e.g., cleaned), check isEmpty() on
that cleaned list, and pass the cleaned list into
AppBookSpecification.withMatchScores(cleaned, req.effectiveFilterMode()) so
whitespace/empty entries are removed just like the other multi-value filters.
In
`@backend/src/main/java/org/booklore/config/security/service/AuthenticationService.java`:
- Around line 82-96: The current getAuthenticatedUser() allows null and causes
NPEs in unprotected endpoints; restore the original throwing behavior for
auth-required paths and add a new null-tolerant accessor. Change
getAuthenticatedUser() (AuthenticationService.getAuthenticatedUser) to throw an
AuthenticationException or IllegalStateException when authentication is
missing/unauthenticated (as before), and implement a new
tryGetAuthenticatedUser() or Optional<BookLoreUser> tryGetAuthenticatedUser()
that returns Optional.empty() when unauthenticated (and performs
defaultSettingInitializer.ensureDefaultSettings(user) only when a user is
present). Then update call sites that must tolerate anonymous access (e.g.,
AppShelfController.getShelves/getMagicShelves,
ShelfService.getAuthenticatedUserId, KoreaderUserService.upsertUser,
BookNoteService methods, NotebookService, CustomFontController, TaskService,
etc.) to use tryGetAuthenticatedUser() and handle Optional.empty() instead of
dereferencing a possibly-null user.
In `@backend/src/main/java/org/booklore/service/ShelfService.java`:
- Around line 113-116: getAuthenticatedUserId() is not null-safe now that
authenticationService.getAuthenticatedUser() can return null; change it to check
for null and surface a proper authentication failure instead of letting
user.getId() throw an NPE. In the getAuthenticatedUserId method (and callers
like createShelf and getUserKoboShelf), detect when
authenticationService.getAuthenticatedUser() returns null and throw the
appropriate auth/unauthorized exception used in the project (or return an
Optional/explicit error) so unauthenticated callers receive a 401/403 rather
than a 500; update method behavior and callers accordingly to handle the
explicit auth failure.
---
Nitpick comments:
In
`@backend/src/main/java/org/booklore/app/controller/AppBookContextController.java`:
- Around line 51-58: Replace the lambda maps with method references to simplify
the mapping calls: change .map(e -> bookMapper.toPdfViewerPreferences(e)) to
.map(bookMapper::toPdfViewerPreferences) (and similarly use
bookMapper::toNewPdfViewerPreferences, bookMapper::toEbookViewerPreferences,
bookMapper::toCbxViewerPreferences) on the results of
pdfPrefsRepo.findByBookIdAndUserId, newPdfPrefsRepo.findByBookIdAndUserId,
ebookPrefsRepo.findByBookIdAndUserId, and cbxPrefsRepo.findByBookIdAndUserId
respectively.
In
`@backend/src/main/java/org/booklore/app/controller/AppBootstrapController.java`:
- Line 14: Remove the unused Lombok logging annotation from the controller:
delete the `@Slf4j` annotation (and the corresponding import
lombok.extern.slf4j.Slf4j if present) from AppBootstrapController since no
logging calls use the generated "log" field; then run a build to confirm there
are no remaining references to "log" in that class.
In
`@backend/src/main/java/org/booklore/app/controller/AppDashboardController.java`:
- Around line 38-84: The per-scroller fetch in
AppDashboardController.getDashboard currently runs all scrollers inside a single
loop without isolating exceptions, so an exception in mobileBookService (or any
branch) can abort the entire dashboard; update the loop that iterates
config.getScrollers() to wrap the switch/fetch logic for each scroller in a
try/catch (catch Exception) and on error log with scroller id/type and continue,
then put an empty list into scrollerData for that scroller (matching existing
fallback behavior); use the same error-isolation pattern as
MenuCountsService.computeMagicShelfCounts and reference getDashboard,
resolveDashboardConfig, mobileBookService and
scroller.getId()/getType()/getMaxItems()/getMagicShelfId() when implementing.
In `@backend/src/main/java/org/booklore/app/service/AppBookService.java`:
- Around line 767-779: getAccessibleLibraryIds currently calls
libraryRepository.findAll() for admins and hydrates full LibraryEntity objects
on every request; change it to use a projection query (e.g., add
libraryRepository.findAllIds() that returns List<Long> or Set<Long> and map
those to the returned Set) to avoid entity hydration, and add a short-lived
cache or reuse a per-request computed set (similar to filterOptionsCache) so
repeated calls within the same request (e.g., getFilterOptions → downstream
helpers) don’t hit the DB repeatedly; update references in
getAccessibleLibraryIds to use the new repository method and consider storing
the result in a transient accessibleLibraryIds variable for the request
lifecycle.
In `@backend/src/main/java/org/booklore/app/service/AppBootstrapService.java`:
- Line 43: The error log in AppBootstrapService currently logs
user.getUsername(), which exposes PII; update the log to avoid the username—use
user.getId() instead (or include both id and username but redact/omit the
username) in the log call where log.error("[Bootstrap] Failed to fetch complete
bootstrap data for user {}: {}", ... ) is invoked so debugging retains an
identifier without persisting the username.
In `@backend/src/main/java/org/booklore/controller/AuthenticationController.java`:
- Around line 64-77: Remove the redundant try/catch from the refreshToken method
and re-enable bean validation on the request: keep the `@Valid` annotation on the
RefreshTokenRequest parameter so DTO-level constraints (e.g., `@NotBlank`) are
enforced, and delete the try/catch that only logs e.getMessage() then rethrows
(or alternatively, replace it with logic that maps specific exceptions from
authenticationService.refreshToken(...) to appropriate ResponseEntity responses
if you want local translation); ensure the method signature is public
ResponseEntity<?> refreshToken(`@Valid` `@RequestBody` RefreshTokenRequest request)
and rely on the global exception handler for other exceptions.
In `@backend/src/main/java/org/booklore/service/library/LibraryService.java`:
- Around line 234-250: The method getLibraries(BookLoreUser user) currently
always fetches BookLoreUserEntity via
userRepository.findByIdWithLibraries(user.getId()) even for admins; change the
logic to check admin status on the incoming BookLoreUser
(user.getPermissions().isPermissionAdmin()) first and short-circuit to
libraryRepository.findAll() for admins without calling findByIdWithLibraries;
only call userRepository.findByIdWithLibraries(...) and map
userEntity.getLibraries() when the caller is not an admin, then map entities via
libraryMapper::toLibrary as before.
In `@backend/src/main/java/org/booklore/service/MenuCountsService.java`:
- Around line 21-32: Remove the redundant wildcard import "import java.util.*;"
from MenuCountsService.java and keep the explicit java.util imports already
listed (e.g., Collections, LinkedHashMap, Map, Set, and stream.Collectors);
ensure there are no other missing java.util types used in methods such as any in
MenuCountsService so compilation still passes and import ordering/style matches
the repo.
- Around line 81-103: computeLibraryCounts currently builds a JPQL string with
an ad-hoc deleted check instead of reusing the existing Specification logic;
replace the JPQL approach with a Criteria/Specification-based grouped count
similar to computeShelfCounts by building a CriteriaQuery<Tuple> (or
CriteriaQuery<Object[]>) that applies visibleBooksSpec.toPredicate(root, query,
cb) (or uses AppBookSpecification.notDeleted() via the same mechanism) and then
groups by root.get("library").get("id") to produce counts, set the
accessibleLibraryIds predicate the same way computeShelfCounts does, execute the
criteria query via entityManager and populate the counts Map; ensure you
reference computeLibraryCounts, computeShelfCounts, visibleBooksSpec (or
AppBookSpecification.notDeleted()), and entityManager when locating code to
change.
In
`@backend/src/test/java/org/booklore/app/service/AppBookServiceFilterOptionsTest.java`:
- Around line 222-225: The test uses the fully-qualified class name
org.booklore.model.entity.LibraryEntity in the when(...) stub; replace that FQN
with a simple import for LibraryEntity and update the three occurrences in the
helper so the lines read LibraryEntity.builder()..., keeping the same builder
calls and the when(libraryRepository.findAll()) stub unchanged; add import
org.booklore.model.entity.LibraryEntity; near the other entity imports to match
file style.
In `@backend/src/test/java/org/booklore/service/MenuCountsServiceTest.java`:
- Around line 90-149: The test over-mocks JPA Criteria plumbing
(CriteriaBuilder/CriteriaQuery/Root/Join/Path) for MenuCountsServiceTest which
is brittle; instead add a complementary integration test using a `@DataJpaTest`
(or `@SpringBootTest` with an in-memory DB) that persists sample
BookEntity/Library/Shelf rows and calls MenuCountsService.getMenuCounts (and its
helper computeShelfCounts) to exercise the real JPQL/Criteria paths, keeping the
existing unit tests but simplifying them to only assert orchestration (mocking
bookRepository.count and magicShelfBookService.toSpecification); this requires
creating fixtures for libraries/shelves/books, wiring the real
EntityManager/BookRepository, and asserting libraryCounts, shelfCounts,
magicShelfCounts, totalBookCount and unshelvedBookCount against persisted data.
🪄 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: 5908efbe-777a-40c3-be6e-56871d7edc93
📒 Files selected for processing (26)
backend/src/main/java/org/booklore/app/controller/AppBookContextController.javabackend/src/main/java/org/booklore/app/controller/AppBookController.javabackend/src/main/java/org/booklore/app/controller/AppBootstrapController.javabackend/src/main/java/org/booklore/app/controller/AppDashboardController.javabackend/src/main/java/org/booklore/app/controller/AppFilterController.javabackend/src/main/java/org/booklore/app/dto/AppBookContextResponse.javabackend/src/main/java/org/booklore/app/dto/AppBootstrapResponse.javabackend/src/main/java/org/booklore/app/dto/AppDashboardResponse.javabackend/src/main/java/org/booklore/app/service/AppBookService.javabackend/src/main/java/org/booklore/app/service/AppBootstrapService.javabackend/src/main/java/org/booklore/app/specification/AppBookSpecification.javabackend/src/main/java/org/booklore/config/WebMvcConfig.javabackend/src/main/java/org/booklore/config/security/SecurityConfig.javabackend/src/main/java/org/booklore/config/security/filter/AuthenticationCheckFilter.javabackend/src/main/java/org/booklore/config/security/service/AuthenticationService.javabackend/src/main/java/org/booklore/controller/AuthenticationController.javabackend/src/main/java/org/booklore/controller/BookController.javabackend/src/main/java/org/booklore/controller/UserController.javabackend/src/main/java/org/booklore/mapper/BookMapper.javabackend/src/main/java/org/booklore/model/dto/response/MenuCountsResponse.javabackend/src/main/java/org/booklore/service/MenuCountsService.javabackend/src/main/java/org/booklore/service/ShelfService.javabackend/src/main/java/org/booklore/service/library/LibraryService.javabackend/src/test/java/org/booklore/app/service/AppBookServiceFilterOptionsTest.javabackend/src/test/java/org/booklore/app/service/AppBookServiceProgressTest.javabackend/src/test/java/org/booklore/service/MenuCountsServiceTest.java
💤 Files with no reviewable changes (1)
- backend/src/main/java/org/booklore/app/specification/AppBookSpecification.java
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
backend/src/main/java/org/booklore/app/service/AppBookService.java (2)
764-776:⚠️ Potential issue | 🟠 MajorAdmin library scoping change: performance + semantic shift — consider optimizing.
Two concerns with switching admin access from
nullto a materializedSet<Long>:
Performance:
libraryRepository.findAll()is invoked on virtually every read path (getBooks,getAllBookIds,getBookDetail,getBookProgress,searchBooks,getContinueReading,getContinueListening,getRecentlyAdded,getRecentlyScanned,getRandomBooks,getFilterOptions,validateAccessAndGetProgress, …) for admin users. Each call hydrates fullLibraryEntityrows (with all mapped columns) just to extract theid. For admins on a hot path this is a noticeable regression vs. the previousnullshort-circuit. Consider either a short-lived cache, a projection query returning only IDs (e.g.@Query("SELECT l.id FROM LibraryEntity l")), or keepingnullas the "unrestricted" sentinel for admins.Semantic shift: Previously
nullmeant "no library filter"; now admins get an explicitIN (:libraryIds)clause viaAppBookSpecification.inLibraries(...)and the JPQLAND b.library.id IN :libraryIdsingetFilterOptions. If an admin somehow has zero libraries, the behavior flips from "see all books" to "see nothing". Worth confirming that's the intended contract.♻️ Suggested projection
// LibraryRepository `@Query`("SELECT l.id FROM LibraryEntity l") List<Long> findAllIds();- return libraryRepository.findAll().stream() - .map(LibraryEntity::getId) - .collect(Collectors.toSet()); + return new HashSet<>(libraryRepository.findAllIds());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/main/java/org/booklore/app/service/AppBookService.java` around lines 764 - 776, The getAccessibleLibraryIds(BookLoreUser) change materializes all LibraryEntity rows for admins via libraryRepository.findAll() which hurts performance and changes semantics (admins with zero libraries now see nothing); revert to using a sentinel meaning "no filter" (return null for admins) or replace the findAll() call with a lightweight ID-only query (e.g., add a repository method like findAllIds()) and use that instead; ensure callers that use AppBookSpecification.inLibraries(...) and getFilterOptions continue to treat null as "unrestricted" or adapt them to handle an empty collection intentionally so behavior for admins remains correct.
914-919:⚠️ Potential issue | 🟡 Minor
matchScoreno longer sanitized — inconsistent with sibling filters and silently drops invalid input.Every other list-based filter in
buildSpecificationruns values throughBookListRequest.cleanValues(...); onlymatchScorenow bypasses it. PerAppBookSpecification.withMatchScores(seebackend/src/main/java/org/booklore/app/specification/AppBookSpecification.java:575-578), invalid/non-numeric strings causeparseIntListto return empty and the spec degrades tocb.conjunction()— i.e. the filter is silently disabled instead of applying the valid subset. Either restorecleanValueshere, or leave a comment justifying the deliberate divergence.🛠️ Proposed fix
- if (req.matchScore() != null && !req.matchScore().isEmpty()) { - List<String> cleaned = req.matchScore(); - if (!cleaned.isEmpty()) { - specs.add(AppBookSpecification.withMatchScores(cleaned, req.effectiveFilterMode())); - } - } + if (req.matchScore() != null && !req.matchScore().isEmpty()) { + List<String> cleaned = BookListRequest.cleanValues(req.matchScore()); + if (!cleaned.isEmpty()) { + specs.add(AppBookSpecification.withMatchScores(cleaned, req.effectiveFilterMode())); + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/main/java/org/booklore/app/service/AppBookService.java` around lines 914 - 919, The matchScore list is not being sanitized before building the spec, unlike other list-based filters, which causes AppBookSpecification.withMatchScores (and its parseIntList logic) to silently drop the entire filter when non-numeric values exist; run the incoming matchScore values through BookListRequest.cleanValues(...) (same sanitization used by sibling filters) and only call AppBookSpecification.withMatchScores(cleaned, req.effectiveFilterMode()) when the cleaned list is non-empty, or alternatively add a comment in buildSpecification explaining why this field intentionally bypasses cleanValues if that was deliberate.
🧹 Nitpick comments (3)
backend/src/main/java/org/booklore/model/dto/CbxViewerPreferences.java (1)
15-15: Consider whetheruserIdbelongs in the response DTO.Since
getBookContextis always scoped to the calling user, echoinguserIdback in the per-viewer preference DTOs is redundant and adds a small information-exposure surface (the field will serialize into every/app/books/{bookId}/contextresponse as well as anywhere else this DTO is returned). If it’s only needed for mapping/internal use, consider marking it@JsonIgnoreor keeping it off the outbound DTO. Same note applies to the parallel additions inPdfViewerPreferencesandNewPdfViewerPreferences.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/main/java/org/booklore/model/dto/CbxViewerPreferences.java` at line 15, CbxViewerPreferences currently exposes a public userId field which is redundant for the per-caller scoped getBookContext response and increases surface area; remove userId from the outbound DTOs (CbxViewerPreferences, PdfViewerPreferences, NewPdfViewerPreferences) or mark the field with `@JsonIgnore` and keep any mapping-only userId on an internal/domain object or mapper helper so it is not serialized in responses.backend/src/test/java/org/booklore/service/MenuCountsServiceTest.java (2)
101-185: Heavy Criteria API mocking is fragile — consider a lightweight integration test.Both
countsAllLibrariesShelvesAndMagicShelvesForAdminandmagicShelfCountFallsBackToZeroOnEvaluatorFailurecouple tightly to the exact sequence ofCriteriaBuilder/CriteriaQuery/Root/Join/Pathcalls insidecomputeShelfCounts. Any refactor to that private helper (e.g., switching from criteria to JPQL, or adjusting the join path) will force these mocks to be rewritten even though the externally observable behavior is unchanged.If feasible, consider complementing this with a slice test using
@DataJpaTest(or a minimal@SpringBootTest) backed by an in-memory DB so the shelf-count path is exercised end-to-end and the unit test can drop the Criteria plumbing stubs. Not blocking — the current tests do verify the contract.🤖 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/MenuCountsServiceTest.java` around lines 101 - 185, The tests countsAllLibrariesShelvesAndMagicShelvesForAdmin and magicShelfCountFallsBackToZeroOnEvaluatorFailure are brittle due to heavy Criteria API mocking inside computeShelfCounts; replace or supplement them with a lightweight slice/integration test (e.g., `@DataJpaTest` or minimal `@SpringBootTest`) that uses an in-memory DB to exercise service.getMenuCounts end-to-end, or alternatively refactor to avoid stubbing CriteriaBuilder by moving the query logic into a repository method you can mock (e.g., extract computeShelfCounts into a repository method or DAO and mock that) so unit tests only assert behavior of getMenuCounts and magicShelfBookService.toSpecification interactions without stubbing CriteriaBuilder/CriteriaQuery/Root/Join/Path.
128-136: Document the ordering ofbookRepository.count(...)return values.The inline comment on line 129 (
magic counts (2) + total + unshelved) helpfully captures the call order, but it's worth noting this test will silently break ifgetMenuCountsis ever refactored to reorder thebookRepository.countinvocations (e.g., computing total before magic shelves). The consecutive-return stubbing (thenReturn(9L, 2L, 150L, 40L)) is positional and offers no safety net. Consider using distinctwhen(...).thenReturn(...)stubs keyed on the actualSpecificationarg (e.g., viamagicSpec1/magicSpec2/visibleBooksSpecmatchers) so the assertions remain meaningful if call order changes. Optional.🤖 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/MenuCountsServiceTest.java` around lines 128 - 136, The consecutive thenReturn on bookRepository.count(…) is order-dependent and fragile for getMenuCounts; instead stub counts by matching the actual Specification arguments so each call returns the intended value regardless of call order: replace the single when(bookRepository.count(any(Specification.class))).thenReturn(9L, 2L, 150L, 40L) with separate stubs that target the specific specs produced by magicShelfBookService.toSpecification (magicSpec1, magicSpec2) and the visible/total specs used in getMenuCounts (e.g., visibleBooksSpec/totalBooksSpec) by using argument matchers (eq or Mockito.argThat) to return 9L for magicSpec1, 2L for magicSpec2 and the appropriate totals for the other specs so assertions remain correct if call order changes.
🤖 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/MenuCountsServiceTest.java`:
- Around line 201-248: The never() verification on
magicShelfBookService.toSpecification(...) is meaningless because
magicShelfService.getUserShelves() was stubbed to return an empty list; update
the test so it actually asserts the service behavior: change the stub of
magicShelfService.getUserShelves() to return a non-empty list (e.g., a
MagicShelf DTO or id list containing 5L) so computeMagicShelfCounts runs, then
replace the never() verify with
verify(magicShelfBookService).toSpecification(eq(5L), any()) (or
verify(...).times(1)) to confirm toSpecification is invoked for that shelf,
referencing magicShelfService.getUserShelves() and
magicShelfBookService.toSpecification(...) in your changes; alternatively, if
you prefer not to assert magic-shelf behavior here, drop the verify entirely.
---
Outside diff comments:
In `@backend/src/main/java/org/booklore/app/service/AppBookService.java`:
- Around line 764-776: The getAccessibleLibraryIds(BookLoreUser) change
materializes all LibraryEntity rows for admins via libraryRepository.findAll()
which hurts performance and changes semantics (admins with zero libraries now
see nothing); revert to using a sentinel meaning "no filter" (return null for
admins) or replace the findAll() call with a lightweight ID-only query (e.g.,
add a repository method like findAllIds()) and use that instead; ensure callers
that use AppBookSpecification.inLibraries(...) and getFilterOptions continue to
treat null as "unrestricted" or adapt them to handle an empty collection
intentionally so behavior for admins remains correct.
- Around line 914-919: The matchScore list is not being sanitized before
building the spec, unlike other list-based filters, which causes
AppBookSpecification.withMatchScores (and its parseIntList logic) to silently
drop the entire filter when non-numeric values exist; run the incoming
matchScore values through BookListRequest.cleanValues(...) (same sanitization
used by sibling filters) and only call
AppBookSpecification.withMatchScores(cleaned, req.effectiveFilterMode()) when
the cleaned list is non-empty, or alternatively add a comment in
buildSpecification explaining why this field intentionally bypasses cleanValues
if that was deliberate.
---
Nitpick comments:
In `@backend/src/main/java/org/booklore/model/dto/CbxViewerPreferences.java`:
- Line 15: CbxViewerPreferences currently exposes a public userId field which is
redundant for the per-caller scoped getBookContext response and increases
surface area; remove userId from the outbound DTOs (CbxViewerPreferences,
PdfViewerPreferences, NewPdfViewerPreferences) or mark the field with
`@JsonIgnore` and keep any mapping-only userId on an internal/domain object or
mapper helper so it is not serialized in responses.
In `@backend/src/test/java/org/booklore/service/MenuCountsServiceTest.java`:
- Around line 101-185: The tests
countsAllLibrariesShelvesAndMagicShelvesForAdmin and
magicShelfCountFallsBackToZeroOnEvaluatorFailure are brittle due to heavy
Criteria API mocking inside computeShelfCounts; replace or supplement them with
a lightweight slice/integration test (e.g., `@DataJpaTest` or minimal
`@SpringBootTest`) that uses an in-memory DB to exercise service.getMenuCounts
end-to-end, or alternatively refactor to avoid stubbing CriteriaBuilder by
moving the query logic into a repository method you can mock (e.g., extract
computeShelfCounts into a repository method or DAO and mock that) so unit tests
only assert behavior of getMenuCounts and magicShelfBookService.toSpecification
interactions without stubbing CriteriaBuilder/CriteriaQuery/Root/Join/Path.
- Around line 128-136: The consecutive thenReturn on bookRepository.count(…) is
order-dependent and fragile for getMenuCounts; instead stub counts by matching
the actual Specification arguments so each call returns the intended value
regardless of call order: replace the single
when(bookRepository.count(any(Specification.class))).thenReturn(9L, 2L, 150L,
40L) with separate stubs that target the specific specs produced by
magicShelfBookService.toSpecification (magicSpec1, magicSpec2) and the
visible/total specs used in getMenuCounts (e.g.,
visibleBooksSpec/totalBooksSpec) by using argument matchers (eq or
Mockito.argThat) to return 9L for magicSpec1, 2L for magicSpec2 and the
appropriate totals for the other specs so assertions remain correct if call
order changes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f81e47d8-75b0-4cbc-b858-8e05ac677193
📒 Files selected for processing (11)
backend/src/main/java/org/booklore/app/controller/AppBookContextController.javabackend/src/main/java/org/booklore/app/service/AppBookContextService.javabackend/src/main/java/org/booklore/app/service/AppBookService.javabackend/src/main/java/org/booklore/config/security/SecurityConfig.javabackend/src/main/java/org/booklore/config/security/filter/AuthenticationCheckFilter.javabackend/src/main/java/org/booklore/model/dto/CbxViewerPreferences.javabackend/src/main/java/org/booklore/model/dto/NewPdfViewerPreferences.javabackend/src/main/java/org/booklore/model/dto/PdfViewerPreferences.javabackend/src/test/java/org/booklore/app/service/AppBookServiceFilterOptionsTest.javabackend/src/test/java/org/booklore/app/service/AppBookServiceProgressTest.javabackend/src/test/java/org/booklore/service/MenuCountsServiceTest.java
✅ Files skipped from review due to trivial changes (3)
- backend/src/main/java/org/booklore/config/security/filter/AuthenticationCheckFilter.java
- backend/src/main/java/org/booklore/model/dto/NewPdfViewerPreferences.java
- backend/src/main/java/org/booklore/model/dto/PdfViewerPreferences.java
🚧 Files skipped from review as they are similar to previous changes (2)
- backend/src/main/java/org/booklore/config/security/SecurityConfig.java
- backend/src/main/java/org/booklore/app/controller/AppBookContextController.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). (4)
- GitHub Check: Test Suite / Frontend Tests
- GitHub Check: Test Suite / Backend Tests
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (java-kotlin)
🧰 Additional context used
📓 Path-based instructions (2)
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/model/dto/CbxViewerPreferences.javabackend/src/test/java/org/booklore/app/service/AppBookServiceFilterOptionsTest.javabackend/src/test/java/org/booklore/app/service/AppBookServiceProgressTest.javabackend/src/main/java/org/booklore/app/service/AppBookContextService.javabackend/src/test/java/org/booklore/service/MenuCountsServiceTest.javabackend/src/main/java/org/booklore/app/service/AppBookService.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/app/service/AppBookServiceFilterOptionsTest.javabackend/src/test/java/org/booklore/app/service/AppBookServiceProgressTest.javabackend/src/test/java/org/booklore/service/MenuCountsServiceTest.java
🧠 Learnings (9)
📚 Learning: 2026-03-31T19:52:51.386Z
Learnt from: balazs-szucs
Repo: grimmory-tools/grimmory PR: 306
File: frontend/src/app/features/settings/user-management/user.service.ts:64-75
Timestamp: 2026-03-31T19:52:51.386Z
Learning: In `grimmory-tools/grimmory`, CBX reader settings (including `CbxPageViewMode`, `CbxPageSplitOption`, `CbxScrollMode`, etc.) are stored as a raw JSON string blob in the `user_settings` table under a setting key (e.g., `CBX_READER_SETTING`). The backend does not deserialize these values into typed Java enum instances, so adding new frontend enum values (e.g., `TWO_PAGE_REVERSED`, `CbxPageSplitOption` members) does NOT cause Jackson `InvalidFormatException` errors. Do not flag frontend-only enum additions as backend serialization issues for these settings.
Applied to files:
backend/src/main/java/org/booklore/model/dto/CbxViewerPreferences.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:
backend/src/main/java/org/booklore/model/dto/CbxViewerPreferences.javabackend/src/test/java/org/booklore/app/service/AppBookServiceFilterOptionsTest.javabackend/src/test/java/org/booklore/app/service/AppBookServiceProgressTest.javabackend/src/main/java/org/booklore/app/service/AppBookContextService.javabackend/src/test/java/org/booklore/service/MenuCountsServiceTest.javabackend/src/main/java/org/booklore/app/service/AppBookService.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/model/dto/CbxViewerPreferences.javabackend/src/test/java/org/booklore/app/service/AppBookServiceFilterOptionsTest.javabackend/src/test/java/org/booklore/app/service/AppBookServiceProgressTest.javabackend/src/main/java/org/booklore/app/service/AppBookContextService.javabackend/src/test/java/org/booklore/service/MenuCountsServiceTest.javabackend/src/main/java/org/booklore/app/service/AppBookService.java
📚 Learning: 2026-04-04T15:36:56.558Z
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:56.558Z
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:
backend/src/test/java/org/booklore/app/service/AppBookServiceFilterOptionsTest.javabackend/src/test/java/org/booklore/service/MenuCountsServiceTest.javabackend/src/main/java/org/booklore/app/service/AppBookService.java
📚 Learning: 2026-04-14T22:11:02.436Z
Learnt from: balazs-szucs
Repo: grimmory-tools/grimmory PR: 513
File: booklore-api/src/main/java/org/booklore/service/reader/AudiobookReaderService.java:38-39
Timestamp: 2026-04-14T22:11:02.436Z
Learning: In `grimmory-tools/grimmory`, for the `AudiobookReaderService` pattern of wrapping a DB lookup + file I/O under a single `Transactional` boundary, the preferred refactoring approach is **Option B**: extract the `BookFileEntity` lookup into a dedicated `Service` bean (e.g., `AudiobookEntityLoader`) annotated with `Transactional(readOnly = true)`, so that the proxy always narrows the transaction to the DB access only and all filesystem I/O runs outside the persistence context. Do NOT use `TransactionTemplate` (Option A) for this pattern in this codebase.
Applied to files:
backend/src/main/java/org/booklore/app/service/AppBookContextService.javabackend/src/main/java/org/booklore/app/service/AppBookService.java
📚 Learning: 2026-04-04T14:03:28.295Z
Learnt from: balazs-szucs
Repo: grimmory-tools/grimmory PR: 372
File: frontend/src/app/features/book/service/app-books-api.service.ts:150-178
Timestamp: 2026-04-04T14:03:28.295Z
Learning: In `frontend/src/app/features/book/service/app-books-api.service.ts`, the `summaryToBook` function intentionally uses `as unknown as Book` to cast a partial object to the `Book` type. The returned object omits some required `Book` fields (e.g. `metadata.bookId`, `pdfProgress.page`) and includes an extra `bookFiles: []` property not in the `Book` interface. This is by design — consumers of `AppBooksApiService.books()` (e.g. `BookCardComponent`) only access the subset of properties that are populated from `AppBookSummary`, so no runtime issues occur. Do not flag this as a type safety error.
Applied to files:
backend/src/main/java/org/booklore/app/service/AppBookService.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:
backend/src/main/java/org/booklore/app/service/AppBookService.java
📚 Learning: 2026-04-22T01:56:39.495Z
Learnt from: CR
Repo: grimmory-tools/grimmory PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-22T01:56:39.495Z
Learning: Applies to backend/src/**/*.java : Prefer constructor injection via Lombok patterns already used in the codebase. Do not introduce Autowired field injection in backend code
Applied to files:
backend/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:
backend/src/main/java/org/booklore/app/service/AppBookService.java
🔇 Additional comments (5)
backend/src/main/java/org/booklore/app/service/AppBookContextService.java (1)
23-39: LGTM!Clean aggregator with read-only transaction, constructor injection via Lombok, and MapStruct-based DTO mapping — consistent with the codebase conventions. Returning
nullfor missing viewer preferences is an explicit design choice that matches the DTO builder semantics.backend/src/test/java/org/booklore/app/service/AppBookServiceProgressTest.java (2)
43-66: LGTM — test wiring matches the updated constructor.
LibraryRepositorymock is correctly added and passed in the same position as the production constructor.
162-171: Stub is sufficient for current tests; note potential brittleness.Stubbing
findAll()to return a single library withlibraryId=5Laligns admin access with the book fixture used in these tests. If future tests exercise books in libraries not in this stubbed set, admin flows will unexpectedly 403. Fine as-is given@MockitoSettings(LENIENT), just something to be aware of.backend/src/test/java/org/booklore/app/service/AppBookServiceFilterOptionsTest.java (1)
49-70: LGTM — mock wiring and admin stub cover both library IDs used in assertions.Returning libraries
5Land10Lmatches thegetFilterOptions_withLibraryId_admin_succeedsfixture. Consider importingLibraryEntityat the top rather than using the fully qualified name inline for consistency with the rest of the file, but non-blocking.Also applies to: 220-223
backend/src/test/java/org/booklore/service/MenuCountsServiceTest.java (1)
108-126: Past duplicate-stub concern resolved.The previously flagged duplicate stubs on
cb.count(any()),cq.multiselect,cq.where, andcq.groupByhave been consolidated into single stubs. Looks good.
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
1 participant
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
Linked Issue: Fixes #
Changes
NOTE: I am well aware of /menu-counts or /bootstrap not being used, i already another branch for the FE changes for it. But wanted to split this up, so that it won't get overwhelming with the reviewing. It is not a mistake. It is known. Thanks.
This pull request introduces several new API endpoints for the mobile app and makes improvements to existing ones, focusing on providing consolidated data responses and improved caching. It adds new controllers and DTOs for bootstrap, dashboard, and book context data, and standardizes cache headers across endpoints. Additionally, it refines the service layer for better handling of user permissions and book queries.
New consolidated endpoints:
AppBootstrapControllerwith/api/v1/app/bootstrapendpoint to deliver all necessary bootstrap data (user, settings, version, counts, libraries, shelves) in a single request, along with the supportingAppBootstrapResponseDTO.AppDashboardControllerwith/api/v1/app/dashboardendpoint to provide dashboard scroller data in one call, using user settings to determine which scrollers to return, with the supportingAppDashboardResponseDTO.AppBookContextControllerwith/api/v1/app/books/{bookId}/contextendpoint to return consolidated book details and all reader preferences for a book, with the supportingAppBookContextResponseDTO.Caching improvements:
Standardized cache headers (
Cache-Control) for relevant endpoints inAppBookControllerandAppFilterControllerto improve client-side caching and performance.Service layer and permissions:
Improved admin library access: admins now receive all library IDs, not just
null, enabling more consistent permission checks.Removed unnecessary
hasDigitalFile()specification from book queries to broaden search and recently added results, allowing books without digital files to appear if appropriate.Refactored
AppBookServiceto inject and useLibraryRepositoryandUserRepositorywhere needed, improving dependency management.Summary by CodeRabbit
New Features
Performance