refactor(cache): improve resource caching strategies and add new endpoints for menu counts and user preferences#806
Open
balazs-szucs wants to merge 3 commits intogrimmory-tools:developfrom
Open
Conversation
…oints for menu counts and user preferences
Contributor
📝 WalkthroughWalkthroughAdds HTTP caching policies for static assets and API responses; exposes a bootstrap endpoint that bypasses JWT auth; extends viewer-preference DTOs with Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Controller as BookController
participant Service as MenuCountsService
participant RepoB as BookRepository
participant RepoS as ShelfRepository
participant MSvc as MagicShelfBookService
Client->>Controller: GET /api/v1/books/menu-counts
Controller->>Service: getMenuCounts()
Service->>RepoB: query grouped book counts by library (visibility spec)
Service->>RepoS: query grouped book counts by shelf (criteria tuple)
Service->>MSvc: compute/count books per magic-shelf (per-shelf spec)
RepoB-->>Service: library counts map
RepoS-->>Service: shelf counts map
MSvc-->>Service: magic-shelf counts (or 0 on error)
Service-->>Controller: MenuCountsResponse
Controller-->>Client: 200 OK (JSON)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 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: 5
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/service/ShelfService.java (1)
113-116:⚠️ Potential issue | 🟡 Minor
getAuthenticatedUserId()still dereferences a potentially null user.Now that
authenticationService.getAuthenticatedUser()can returnnull,user.getId()on line 115 will NPE instead of throwing the previous explicitIllegalStateException. In practicecreateShelf/getUserKoboShelfare behind.authenticated()so this is mostly defensive, but a clearer error keeps the contract explicit.Proposed fix
private Long getAuthenticatedUserId() { BookLoreUser user = authenticationService.getAuthenticatedUser(); + if (user == null || user.getId() == null) { + throw new AccessDeniedException("No authenticated user"); + } 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, The helper getAuthenticatedUserId currently dereferences a possibly null result from authenticationService.getAuthenticatedUser(); change it to null-check the returned BookLoreUser and throw a clear IllegalStateException (or similar runtime exception) with a descriptive message if it's null so callers like createShelf and getUserKoboShelf keep the explicit contract; update getAuthenticatedUserId to call authenticationService.getAuthenticatedUser(), verify non-null, then return user.getId() otherwise throw.backend/src/main/java/org/booklore/config/security/service/AuthenticationService.java (1)
82-96:⚠️ Potential issue | 🔴 CriticalBreaking change: nullable return mixes inconsistent null-handling across callsites.
getAuthenticatedUser()now returns null for unauthenticated/non-BookLoreUser principals, but this contract change was only partially addressed at call sites:Updated callsites (proper null checks added):
MenuCountsService.getMenuCounts()— guards withif (user == null || user.getId() == null)ShelfService.getShelves()— guards withif (user == null || user.getId() == null)Unhandled callsites (no null checks, dereference immediately):
ReadingSessionService.recordSession()line 95 —authenticatedUser.getId()ReadingSessionService.getSessionHeatmapForYear()line 122 —getAuthenticatedUser().getId()LibraryService.createLibrary()line 155 —bookLoreUser.getId()AuthorMetadataService.getAllAuthors()line 58 —user.getPermissions().isAdmin()ShelfService.getAuthenticatedUserId()—user.getId()(helper used bygetUserKoboShelf,createShelf)While these endpoints are currently protected by
.authenticated()in SecurityConfig, the missing@Nullableannotation means the type system provides no protection against future misuse. Any new bootstrap-style permit-all endpoint calling these services will silently NPE.Recommended fix: Add
@Nullableto the method signature and audit the unhandled callsites, or revert to a throwing contract with a separategetAuthenticatedUserOrNull()for the null-aware callers.🤖 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, getAuthenticatedUser() changed to return null for unauthenticated principals but lacks `@Nullable` and several callers (ReadingSessionService.recordSession, ReadingSessionService.getSessionHeatmapForYear, LibraryService.createLibrary, AuthorMetadataService.getAllAuthors, ShelfService.getAuthenticatedUserId) still dereference the result causing potential NPEs; either annotate AuthenticationService.getAuthenticatedUser() with `@Nullable` and update the listed callsites to null-check before calling getId()/getPermissions(), or revert to a non-null contract by restoring a throwing variant (e.g., getAuthenticatedUserOrThrow()) and update null-aware callers to call the new nullable helper (or vice versa) so the contract is explicit and all usages are audited and fixed.
🧹 Nitpick comments (7)
backend/src/main/java/org/booklore/mapper/BookMapper.java (1)
41-45: Migrate callers to the new MapStruct methods.The new mapper methods are the right direction, but
BookServicestill hand-buildsPdfViewerPreferences,NewPdfViewerPreferences,EbookViewerPreferences, andCbxViewerPreferencesvia.builder()(lines 211, 229, 236, and 246). The new mapper methods remain unused, which means:
- Any future entity field addition still has to be mirrored manually in these builders.
- Manual builders are error-prone and don't benefit from MapStruct's automatic mapping.
Replace the manual builders with the mapper methods:
bookMapper.toPdfViewerPreferences(pdfPref),bookMapper.toNewPdfViewerPreferences(pdfPref),bookMapper.toEbookViewerPreferences(epubPref), andbookMapper.toCbxViewerPreferences(cbxPref).This aligns with the guideline: "Use MapStruct for entity/DTO mapping in backend code".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/main/java/org/booklore/mapper/BookMapper.java` around lines 41 - 45, BookService is still constructing PdfViewerPreferences, NewPdfViewerPreferences, EbookViewerPreferences, and CbxViewerPreferences via manual builders; replace those manual constructions with the MapStruct mapper methods to avoid duplicated mapping logic. Concretely, in the places currently calling the builders (the Pdf/NewPdf/Ebook/Cbx preference creation in BookService) swap the builder code for calls to bookMapper.toPdfViewerPreferences(pdfPref), bookMapper.toNewPdfViewerPreferences(pdfPref), bookMapper.toEbookViewerPreferences(epubPref), and bookMapper.toCbxViewerPreferences(cbxPref) respectively, ensuring you pass the same source entity variables (pdfPref/epubPref/cbxPref) and remove the now-redundant manual field assignments. Ensure imports and the BookMapper field usage remain correct and run tests to verify behavior.backend/src/main/java/org/booklore/config/WebMvcConfig.java (2)
68-97: Consider movingindex.htmloff the SPA fallback handler.Currently
index.htmlis only served via the/**fallback (line 101-116) which setsNO_CACHE_REVALIDATE. That's correct, but it couples the revalidate behavior forindex.htmlto whatever else lands on the fallback. An explicit handler (e.g. registering"/", "/index.html"before the/**fallback and pointing atclasspath:/static/) would make the policy for the entry document explicit and match the intent described in the comment on lines 99-100. Optional — current behavior is correct.🤖 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/WebMvcConfig.java` around lines 68 - 97, Add an explicit resource handler for the SPA entry document in WebMvcConfig.addResourceHandlers: register "/" and "/index.html" (or equivalent root path) before the existing /** fallback handler and point it to "classpath:/static/" with NO_CACHE_REVALIDATE so the cache policy for index.html is explicit and not coupled to the generic /** fallback handler; update the handler ordering in addResourceHandlers so the new handler runs before the existing /** fallback registration.
92-97: Consider optimizing resource handler location order for root files.The handler registers
/manifest.webmanifestand/favicon.*but checksclasspath:/static/assets/andclasspath:/static/icons/beforeclasspath:/static/. Since these files reside only in the root static directory, Spring performs unnecessary existence checks for the first two locations on each request. Either reorder locations to putclasspath:/static/first, or register root-level files in a dedicated handler with only the root location.🤖 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/WebMvcConfig.java` around lines 92 - 97, The resource handler registration in registry.addResourceHandler currently lists "classpath:/static/assets/" and "classpath:/static/icons/" before "classpath:/static/", causing extra file-existence checks for root files; fix by either reordering addResourceLocations to put "classpath:/static/" first or (preferred) register a separate handler for the root files (e.g., registry.addResourceHandler("/manifest.webmanifest", "/favicon.*") with addResourceLocations("classpath:/static/") and setCacheControl(ONE_DAY_PUBLIC)), leaving the existing "/assets/**" and "/icons/**" handler pointing to their specific locations.backend/src/main/java/org/booklore/controller/UserController.java (1)
33-37: Consider using the typedCacheControlAPI.Minor: Spring offers
.cacheControl(CacheControl.noStore())onResponseEntitybuilder, which avoids the raw header string and keeps consistency withWebMvcConfig's use ofCacheControl. Either works.Proposed refactor
- return ResponseEntity.ok() - .header("Cache-Control", "no-cache, no-store, must-revalidate") - .body(userService.getMyself()); + return ResponseEntity.ok() + .cacheControl(CacheControl.noStore().mustRevalidate()) + .body(userService.getMyself());🤖 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/UserController.java` around lines 33 - 37, Replace the raw Cache-Control header in the UserController.getMyself() response with Spring's typed CacheControl API: call ResponseEntity.ok().cacheControl(CacheControl.noStore().mustRevalidate()) (or CacheControl.noStore() to match project conventions) and remove the manual .header(...) usage; ensure the CacheControl class (org.springframework.http.CacheControl) is imported so getMyself() returns the same semantic cache policy via the typed API.backend/src/main/java/org/booklore/controller/AuthenticationController.java (1)
73-76: Consider lowering the failure log level and narrowing the catch.
log.errorfor every refresh failure will be noisy since expired/invalid/revoked refresh tokens are expected user-triggered outcomes (seeAuthenticationService.refreshTokenwhich throwsAPIExceptionviaApiError.INVALID_CREDENTIALSfor not-found/expired/revoked). Alog.warnis more appropriate here, and catchingExceptionbroadly then rethrowing adds little beyond what a dedicated exception handler or the service-level logging could do. If the intent is purely observability, consider letting the global exception handler log it, or at minimum use WARN.Suggested change
- try { - return authenticationService.refreshToken(request.getRefreshToken()); - } catch (Exception e) { - log.error("[Auth] Token refresh failed: {}", e.getMessage()); - throw e; - } + try { + return authenticationService.refreshToken(request.getRefreshToken()); + } catch (RuntimeException e) { + log.warn("[Auth] Token refresh failed: {}", e.getMessage()); + throw 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/controller/AuthenticationController.java` around lines 73 - 76, The catch in AuthenticationController (around the token refresh handling) is too broad and logs expected user-driven failures at ERROR; change the catch to only handle the specific service exception (e.g., catch APIException or the specific exception thrown by AuthenticationService.refreshToken/ApiError.INVALID_CREDENTIALS) and lower the log level to WARN (log.warn(...)) or remove logging so the global exception handler can record it; ensure other unexpected exceptions still propagate (do not swallow) so higher-level handlers can deal with them.backend/src/main/java/org/booklore/service/MenuCountsService.java (2)
136-156: Per-shelf count query may become a hotspot for users with many magic shelves.Each magic shelf triggers a separate
count(Specification)SQL query, and each Specification is built by parsing the shelf's JSON rules (MagicShelfBookService.toSpecificationdeserializesfilterJsonon every call). For a user with N magic shelves this is N round-trips plus N JSON parses on every sidebar refresh, and the response is cached for only 60 seconds. Worth benchmarking with a realistic shelf count; if it shows up, consider caching the parsed specs or computing counts in a single aggregate query.🤖 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 136 - 156, fetchMagicShelfCounts performs one bookRepository.count(spec) per shelf and calls MagicShelfBookService.toSpecification for each, causing N SQL round-trips and N JSON deserializations; fix by avoiding per-shelf round-trips: either (1) cache parsed Specifications/parsed filterJson keyed by shelf id (e.g., use a short-lived in-memory cache or a method-local map to call toSpecification once per shelf and reuse on repeated refreshes) and reuse them when building counts, or (2) compute counts in a single aggregate query by translating each shelf spec into a predicate and combining them into one query that returns counts grouped by shelf id (so replace repeated bookRepository.count calls with one batched query). Update fetchMagicShelfCounts to use the chosen approach and adjust error handling around MagicShelfBookService.toSpecification and bookRepository so parsing failures for one shelf don’t trigger repeated parsing or queries for others.
50-54: Consider cache-loading the user's libraries/shelves via a single call path.
getMenuCountstriggersauthenticationService.getAuthenticatedUser()once here, butlibraryService.getLibraries(user)andshelfService.getShelves(user)each do their ownuserRepository.findByIdWithLibraries(...)/ shelf lookups. For a single-page menu endpoint this is fine, but if this is hit on every navigation consider either caching per-request or having callers pass a resolvedBookLoreUserEntityto avoid the repeated DB lookups.🤖 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 50 - 54, getMenuCounts calls authenticationService.getAuthenticatedUser(), then libraryService.getLibraries(user) and shelfService.getShelves(user), which each re-query the user's libraries/shelves; fix by loading the user with libraries/shelves once and reusing it: retrieve a fully-initialized BookLoreUserEntity (e.g. via userRepository.findByIdWithLibraries(user.getId()) or an auth method that returns the entity with associations) and pass that single resolved entity into libraryService and shelfService (or add overloads like getLibrariesForEntity/getShelvesForEntity) so you avoid duplicate DB lookups and/or implement a per-request cache for the resolved user entity used by getMenuCounts, authenticationService.getAuthenticatedUser(), libraryService.getLibraries(), shelfService.getShelves(), and userRepository.findByIdWithLibraries.
🤖 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/controller/AuthenticationController.java`:
- Line 64: Add the javax.validation.Valid annotation to the RefreshTokenRequest
parameter in the refreshToken method (i.e., change the signature to use `@Valid`
`@RequestBody` RefreshTokenRequest request), ensure the import for
javax.validation.Valid is present, and remove the redundant manual null/blank
checks in refreshToken so the framework enforces the `@NotBlank` constraint
declared on RefreshTokenRequest (consistent with registerUser and loginUser).
In `@backend/src/main/java/org/booklore/model/dto/CbxViewerPreferences.java`:
- Line 15: The new field userId in CbxViewerPreferences is left null because
CbxViewerPreferences.builder() call sites (notably in BookService within the
method that builds viewer prefs) do not set .userId(...); update those builders
to pass the current user's id (e.g., user.getId()) when constructing
CbxViewerPreferences, mirroring how EbookViewerPreferences.builder() populates
userId, and audit other CbxViewerPreferences.builder() usages to ensure
.userId(...) is always set.
In `@backend/src/main/java/org/booklore/model/dto/NewPdfViewerPreferences.java`:
- Line 15: The NewPdfViewerPreferences builder call inside BookService (the
lambda passed to
newPdfViewerPreferencesRepository.findByBookIdAndUserId(...).ifPresent(...)) is
missing the userId field; update the builder used in the
settingsBuilder.newPdfSettings(...) invocation to include .userId(user.getId())
so the DTO mirrors EbookViewerPreferences.builder() where userId is set,
ensuring the resulting NewPdfViewerPreferences has the correct userId populated.
In `@backend/src/main/java/org/booklore/model/dto/PdfViewerPreferences.java`:
- Line 14: The PdfViewerPreferences instances are missing userId because
PdfViewerPreferences.builder() call sites (notably in BookService where the PDF
response is constructed) do not set it; update the builder invocation to call
.userId(user.getId()) when creating the PdfViewerPreferences, or replace that
builder usage with the new BookMapper.toPdfViewerPreferences(...) which
correctly populates userId; locate the construction in BookService (around the
PDF response creation) and add .userId(user.getId()) or swap to
BookMapper.toPdfViewerPreferences(...) to ensure userId is serialized.
In `@backend/src/main/java/org/booklore/service/MenuCountsService.java`:
- Around line 105-134: fetchShelfCounts currently groups by shelves.id but only
restricts books by visibleLibraryIds/isAdmin, allowing shelf IDs belonging to
other users to appear; restrict the shelf side by intersecting the joined shelf
IDs with the set of shelves the caller can see (the IDs produced by
shelfService.getShelves(user) that is computed earlier) — i.e., obtain the
allowedShelfIds and add a predicate like shelfIdPath.in(allowedShelfIds) (or
apply the same filter when joining) to the CriteriaQuery so only user's/public
shelves are counted; update fetchShelfCounts to accept or compute
allowedShelfIds when calling shelfService.getShelves and include that predicate
alongside visiblePredicate and notDeletedPredicate.
---
Outside diff comments:
In
`@backend/src/main/java/org/booklore/config/security/service/AuthenticationService.java`:
- Around line 82-96: getAuthenticatedUser() changed to return null for
unauthenticated principals but lacks `@Nullable` and several callers
(ReadingSessionService.recordSession,
ReadingSessionService.getSessionHeatmapForYear, LibraryService.createLibrary,
AuthorMetadataService.getAllAuthors, ShelfService.getAuthenticatedUserId) still
dereference the result causing potential NPEs; either annotate
AuthenticationService.getAuthenticatedUser() with `@Nullable` and update the
listed callsites to null-check before calling getId()/getPermissions(), or
revert to a non-null contract by restoring a throwing variant (e.g.,
getAuthenticatedUserOrThrow()) and update null-aware callers to call the new
nullable helper (or vice versa) so the contract is explicit and all usages are
audited and fixed.
In `@backend/src/main/java/org/booklore/service/ShelfService.java`:
- Around line 113-116: The helper getAuthenticatedUserId currently dereferences
a possibly null result from authenticationService.getAuthenticatedUser(); change
it to null-check the returned BookLoreUser and throw a clear
IllegalStateException (or similar runtime exception) with a descriptive message
if it's null so callers like createShelf and getUserKoboShelf keep the explicit
contract; update getAuthenticatedUserId to call
authenticationService.getAuthenticatedUser(), verify non-null, then return
user.getId() otherwise throw.
---
Nitpick comments:
In `@backend/src/main/java/org/booklore/config/WebMvcConfig.java`:
- Around line 68-97: Add an explicit resource handler for the SPA entry document
in WebMvcConfig.addResourceHandlers: register "/" and "/index.html" (or
equivalent root path) before the existing /** fallback handler and point it to
"classpath:/static/" with NO_CACHE_REVALIDATE so the cache policy for index.html
is explicit and not coupled to the generic /** fallback handler; update the
handler ordering in addResourceHandlers so the new handler runs before the
existing /** fallback registration.
- Around line 92-97: The resource handler registration in
registry.addResourceHandler currently lists "classpath:/static/assets/" and
"classpath:/static/icons/" before "classpath:/static/", causing extra
file-existence checks for root files; fix by either reordering
addResourceLocations to put "classpath:/static/" first or (preferred) register a
separate handler for the root files (e.g.,
registry.addResourceHandler("/manifest.webmanifest", "/favicon.*") with
addResourceLocations("classpath:/static/") and setCacheControl(ONE_DAY_PUBLIC)),
leaving the existing "/assets/**" and "/icons/**" handler pointing to their
specific locations.
In `@backend/src/main/java/org/booklore/controller/AuthenticationController.java`:
- Around line 73-76: The catch in AuthenticationController (around the token
refresh handling) is too broad and logs expected user-driven failures at ERROR;
change the catch to only handle the specific service exception (e.g., catch
APIException or the specific exception thrown by
AuthenticationService.refreshToken/ApiError.INVALID_CREDENTIALS) and lower the
log level to WARN (log.warn(...)) or remove logging so the global exception
handler can record it; ensure other unexpected exceptions still propagate (do
not swallow) so higher-level handlers can deal with them.
In `@backend/src/main/java/org/booklore/controller/UserController.java`:
- Around line 33-37: Replace the raw Cache-Control header in the
UserController.getMyself() response with Spring's typed CacheControl API: call
ResponseEntity.ok().cacheControl(CacheControl.noStore().mustRevalidate()) (or
CacheControl.noStore() to match project conventions) and remove the manual
.header(...) usage; ensure the CacheControl class
(org.springframework.http.CacheControl) is imported so getMyself() returns the
same semantic cache policy via the typed API.
In `@backend/src/main/java/org/booklore/mapper/BookMapper.java`:
- Around line 41-45: BookService is still constructing PdfViewerPreferences,
NewPdfViewerPreferences, EbookViewerPreferences, and CbxViewerPreferences via
manual builders; replace those manual constructions with the MapStruct mapper
methods to avoid duplicated mapping logic. Concretely, in the places currently
calling the builders (the Pdf/NewPdf/Ebook/Cbx preference creation in
BookService) swap the builder code for calls to
bookMapper.toPdfViewerPreferences(pdfPref),
bookMapper.toNewPdfViewerPreferences(pdfPref),
bookMapper.toEbookViewerPreferences(epubPref), and
bookMapper.toCbxViewerPreferences(cbxPref) respectively, ensuring you pass the
same source entity variables (pdfPref/epubPref/cbxPref) and remove the
now-redundant manual field assignments. Ensure imports and the BookMapper field
usage remain correct and run tests to verify behavior.
In `@backend/src/main/java/org/booklore/service/MenuCountsService.java`:
- Around line 136-156: fetchMagicShelfCounts performs one
bookRepository.count(spec) per shelf and calls
MagicShelfBookService.toSpecification for each, causing N SQL round-trips and N
JSON deserializations; fix by avoiding per-shelf round-trips: either (1) cache
parsed Specifications/parsed filterJson keyed by shelf id (e.g., use a
short-lived in-memory cache or a method-local map to call toSpecification once
per shelf and reuse on repeated refreshes) and reuse them when building counts,
or (2) compute counts in a single aggregate query by translating each shelf spec
into a predicate and combining them into one query that returns counts grouped
by shelf id (so replace repeated bookRepository.count calls with one batched
query). Update fetchMagicShelfCounts to use the chosen approach and adjust error
handling around MagicShelfBookService.toSpecification and bookRepository so
parsing failures for one shelf don’t trigger repeated parsing or queries for
others.
- Around line 50-54: getMenuCounts calls
authenticationService.getAuthenticatedUser(), then
libraryService.getLibraries(user) and shelfService.getShelves(user), which each
re-query the user's libraries/shelves; fix by loading the user with
libraries/shelves once and reusing it: retrieve a fully-initialized
BookLoreUserEntity (e.g. via userRepository.findByIdWithLibraries(user.getId())
or an auth method that returns the entity with associations) and pass that
single resolved entity into libraryService and shelfService (or add overloads
like getLibrariesForEntity/getShelvesForEntity) so you avoid duplicate DB
lookups and/or implement a per-request cache for the resolved user entity used
by getMenuCounts, authenticationService.getAuthenticatedUser(),
libraryService.getLibraries(), shelfService.getShelves(), and
userRepository.findByIdWithLibraries.
🪄 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: df542915-67d7-44cb-b679-919165141d8e
📒 Files selected for processing (16)
backend/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/CbxViewerPreferences.javabackend/src/main/java/org/booklore/model/dto/NewPdfViewerPreferences.javabackend/src/main/java/org/booklore/model/dto/PdfViewerPreferences.javabackend/src/main/java/org/booklore/model/dto/response/MenuCountsResponse.javabackend/src/main/java/org/booklore/repository/LibraryRepository.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.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). (5)
- GitHub Check: Test Suite / Frontend Tests
- GitHub Check: Test Suite / Backend Tests
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (java-kotlin)
- GitHub Check: Frontend Lint Threshold Check
🧰 Additional context used
📓 Path-based instructions (1)
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/config/security/SecurityConfig.javabackend/src/main/java/org/booklore/model/dto/CbxViewerPreferences.javabackend/src/main/java/org/booklore/config/security/filter/AuthenticationCheckFilter.javabackend/src/main/java/org/booklore/model/dto/NewPdfViewerPreferences.javabackend/src/main/java/org/booklore/repository/LibraryRepository.javabackend/src/main/java/org/booklore/controller/UserController.javabackend/src/main/java/org/booklore/config/security/service/AuthenticationService.javabackend/src/main/java/org/booklore/model/dto/response/MenuCountsResponse.javabackend/src/main/java/org/booklore/mapper/BookMapper.javabackend/src/main/java/org/booklore/controller/AuthenticationController.javabackend/src/main/java/org/booklore/service/ShelfService.javabackend/src/main/java/org/booklore/service/library/LibraryService.javabackend/src/main/java/org/booklore/service/MenuCountsService.javabackend/src/main/java/org/booklore/controller/BookController.javabackend/src/main/java/org/booklore/model/dto/PdfViewerPreferences.javabackend/src/main/java/org/booklore/config/WebMvcConfig.java
🧠 Learnings (6)
📚 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/config/security/SecurityConfig.javabackend/src/main/java/org/booklore/model/dto/CbxViewerPreferences.javabackend/src/main/java/org/booklore/config/security/filter/AuthenticationCheckFilter.javabackend/src/main/java/org/booklore/model/dto/NewPdfViewerPreferences.javabackend/src/main/java/org/booklore/repository/LibraryRepository.javabackend/src/main/java/org/booklore/controller/UserController.javabackend/src/main/java/org/booklore/config/security/service/AuthenticationService.javabackend/src/main/java/org/booklore/model/dto/response/MenuCountsResponse.javabackend/src/main/java/org/booklore/mapper/BookMapper.javabackend/src/main/java/org/booklore/controller/AuthenticationController.javabackend/src/main/java/org/booklore/service/ShelfService.javabackend/src/main/java/org/booklore/service/library/LibraryService.javabackend/src/main/java/org/booklore/service/MenuCountsService.javabackend/src/main/java/org/booklore/controller/BookController.javabackend/src/main/java/org/booklore/model/dto/PdfViewerPreferences.javabackend/src/main/java/org/booklore/config/WebMvcConfig.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/config/security/SecurityConfig.javabackend/src/main/java/org/booklore/model/dto/CbxViewerPreferences.javabackend/src/main/java/org/booklore/config/security/filter/AuthenticationCheckFilter.javabackend/src/main/java/org/booklore/model/dto/NewPdfViewerPreferences.javabackend/src/main/java/org/booklore/repository/LibraryRepository.javabackend/src/main/java/org/booklore/controller/UserController.javabackend/src/main/java/org/booklore/config/security/service/AuthenticationService.javabackend/src/main/java/org/booklore/model/dto/response/MenuCountsResponse.javabackend/src/main/java/org/booklore/mapper/BookMapper.javabackend/src/main/java/org/booklore/controller/AuthenticationController.javabackend/src/main/java/org/booklore/service/ShelfService.javabackend/src/main/java/org/booklore/service/library/LibraryService.javabackend/src/main/java/org/booklore/service/MenuCountsService.javabackend/src/main/java/org/booklore/controller/BookController.javabackend/src/main/java/org/booklore/model/dto/PdfViewerPreferences.javabackend/src/main/java/org/booklore/config/WebMvcConfig.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 : Use MapStruct for entity/DTO mapping in backend code
Applied to files:
backend/src/main/java/org/booklore/mapper/BookMapper.java
📚 Learning: 2026-03-25T19:09:09.638Z
Learnt from: imnotjames
Repo: grimmory-tools/grimmory PR: 189
File: booklore-api/src/main/java/org/booklore/service/kobo/KoboLibrarySyncService.java:113-116
Timestamp: 2026-03-25T19:09:09.638Z
Learning: In `booklore-api/src/main/java/org/booklore/service/kobo/KoboLibrarySyncService.java`, when deciding whether to forward the `Authorization` header to the upstream Kobo server proxy on the `/v1/library/sync` endpoint, a simple null check (`request.getHeader(HttpHeaders.AUTHORIZATION) != null`) is the correct guard. The header is either absent or present — if present it may or may not be a valid token, but validity is determined by the upstream Kobo server, not by Grimmory. Blank/whitespace intermediate states do not occur in practice, and pre-validating the token value before forwarding is not the responsibility of this service.
Applied to files:
backend/src/main/java/org/booklore/controller/AuthenticationController.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/main/java/org/booklore/service/ShelfService.javabackend/src/main/java/org/booklore/service/MenuCountsService.java
📚 Learning: 2026-04-16T19:53:00.375Z
Learnt from: imnotjames
Repo: grimmory-tools/grimmory PR: 507
File: booklore-api/src/main/java/org/booklore/controller/KoboController.java:219-220
Timestamp: 2026-04-16T19:53:00.375Z
Learning: In `booklore-api/src/main/java/org/booklore/controller/KoboController.java`, the `/v1/products/{bookId}/nextread` Kobo endpoint should be registered as GET only (`GetMapping`), not POST. Although calibre-web's kobo.py registers this route with both GET and POST methods as a proxy implementation choice, the Kobo device itself only sends GET requests to this endpoint per imnotjames (PR `#507`).
Applied to files:
backend/src/main/java/org/booklore/controller/BookController.java
🔇 Additional comments (9)
backend/src/main/java/org/booklore/mapper/BookMapper.java (1)
41-44: Field names align correctly — no mapping issues detected.Verification confirms that all entity field names match their corresponding DTO field names exactly. MapStruct's implicit name-based mapping will work correctly for these methods:
- PdfViewerPreferencesEntity → fields:
userId,bookId,zoom,spread,isDarkThemeall match DTO- NewPdfViewerPreferencesEntity → fields:
userId,bookId,pageSpread,pageViewMode,fitMode,scrollMode,backgroundColorall match DTO- EbookViewerPreferenceEntity → fields:
userId,bookId,fontFamily,fontSize,gap,hyphenate,isDark,justify,lineHeight,maxBlockSize,maxColumnCount,maxInlineSize,theme,flowall match DTO- CbxViewerPreferencesEntity → fields:
userId,bookId,pageSpread,pageViewMode,fitMode,scrollMode,backgroundColorall match DTONo runtime null values will occur from field name mismatches.
backend/src/main/java/org/booklore/repository/LibraryRepository.java (1)
19-20: LGTM — consistent with existing pattern.Matches the
findAllIds()pattern already established inBookdropFileRepositoryfor optimized ID-only retrieval.backend/src/main/java/org/booklore/model/dto/response/MenuCountsResponse.java (1)
5-11: LGTM.Clean record DTO for the menu counts aggregate.
backend/src/main/java/org/booklore/config/security/SecurityConfig.java (1)
284-284: LGTM.Whitelisting
/api/v1/app/bootstrapviapermitAll()is correct here since this chain'ssecurityMatcherdoesn't exclude the path (it's intentionally not added towhitelistedPatterns), so the JWT filter still populates the security context when a token is present, but unauthenticated access is allowed through.backend/src/main/java/org/booklore/service/ShelfService.java (1)
78-88: LGTM — null-safe overload is a good pattern.The
getShelves(BookLoreUser)overload correctly short-circuits toList.of()when the user or its id is missing, matching the new nullable contract ofAuthenticationService.getAuthenticatedUser().backend/src/main/java/org/booklore/config/security/filter/AuthenticationCheckFilter.java (1)
21-25: This filter is disabled and has no functional impact — consider removing the disabled code.The
@FilterRegistration(enabled = false)annotation means this filter never executes, making the path-matching logic irrelevant. The actual authorization for/api/v1/app/bootstrapis handled bySecurityConfig.requestMatchers()on line 284, which is the only active source of truth. Since this disabled filter has no effect on request handling, either re-enable it with proper integration or remove it to reduce code clutter.> Likely an incorrect or invalid review comment.backend/src/main/java/org/booklore/service/library/LibraryService.java (1)
234-250: LGTM.The new overload integrates cleanly with the
AuthenticationService.getAuthenticatedUser()null-return behavior, and the existing permission-based flow is preserved. Exposing auser-parameter overload also lets callers likeMenuCountsServiceshare an already-resolved principal without a second lookup.backend/src/main/java/org/booklore/controller/BookController.java (1)
84-92: LGTM on the new menu-counts endpoint.Delegation to
MenuCountsServiceplus the short private cache header is consistent with the other list endpoint change. The service handles unauthenticated users by returning empty/zero aggregates, matching the behavior pattern used byLibraryController.getLibraries().backend/src/main/java/org/booklore/service/MenuCountsService.java (1)
59-59: No action needed — accessor is correct.The
BookLoreUser.UserPermissionsDTO exposesisAdmin()(generated by Lombok from theboolean isAdminfield), and this method is used consistently across the codebase, including in MenuCountsService line 59. The method carries the expected semantics for checking admin privilege. There is no inconsistency between the DTO accessor and other parts of the code.
| public ResponseEntity<Map<String, String>> refreshToken( | ||
| @Parameter(description = "Refresh token request") @Valid @RequestBody RefreshTokenRequest request) { | ||
| return authenticationService.refreshToken(request.getRefreshToken()); | ||
| public ResponseEntity<?> refreshToken(@RequestBody RefreshTokenRequest request) { |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
fd -e java RefreshTokenRequest.java --exec cat {}Repository: grimmory-tools/grimmory
Length of output: 398
🏁 Script executed:
#!/bin/bash
# Find and examine the AuthenticationController.java file
fd -e java AuthenticationController.java --exec cat -n {}Repository: grimmory-tools/grimmory
Length of output: 6241
Restore @Valid annotation for consistency with other endpoints in this controller.
The @Valid annotation is missing from the RefreshTokenRequest parameter. While a manual null/blank check is present (line 66), RefreshTokenRequest declares a @NotBlank constraint that isn't being enforced by the validation framework. Other endpoints in this controller—registerUser (line 48) and loginUser (line 57)—both use @Valid on their request DTOs. Restoring @Valid here maintains consistency and allows the framework to handle validation rather than relying on manual checks.
🤖 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`
at line 64, Add the javax.validation.Valid annotation to the RefreshTokenRequest
parameter in the refreshToken method (i.e., change the signature to use `@Valid`
`@RequestBody` RefreshTokenRequest request), ensure the import for
javax.validation.Valid is present, and remove the redundant manual null/blank
checks in refreshToken so the framework enforces the `@NotBlank` constraint
declared on RefreshTokenRequest (consistent with registerUser and loginUser).
…er ID handling in preferences
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
backend/src/main/java/org/booklore/controller/AuthenticationController.java (2)
64-69: Redundant manual validation now that@Validis restored.With
@Validon the parameter and@NotBlankdeclared onRefreshTokenRequest.refreshToken, Spring will reject missing/blank tokens withMethodArgumentNotValidExceptionbefore this method body executes. Therequest == nullbranch is also effectively unreachable for@RequestBody(a missing body raisesHttpMessageNotReadableException). Keeping the manual check produces a response shape ({"message": "..."}) that differs from your global validation error handler, yielding inconsistent 400 payloads for the same class of error.Consider removing the manual guard and letting the framework handle it:
♻️ Proposed simplification
- public ResponseEntity<?> refreshToken(`@RequestBody` `@Valid` RefreshTokenRequest request) { - log.debug("[Auth] Refresh request received. Request object exists: {}", request != null); - 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 { + public ResponseEntity<Map<String, String>> refreshToken(`@RequestBody` `@Valid` RefreshTokenRequest request) { + try { return authenticationService.refreshToken(request.getRefreshToken()); } catch (Exception e) { - log.error("[Auth] Token refresh failed: {}", e.getMessage()); + log.error("[Auth] Token refresh failed", e); throw 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/controller/AuthenticationController.java` around lines 64 - 69, The manual null/blank guard inside AuthenticationController.refreshToken (the check on request == null || request.getRefreshToken() == null || isBlank() and the ResponseEntity.badRequest(Map.of("message", ...))) should be removed because `@RequestBody` `@Valid` plus `@NotBlank` on RefreshTokenRequest.refreshToken will let Spring's validation and your global exception handlers produce a consistent 400 payload; delete the conditional and its custom log/response, keep the initial debug log if desired, and rely on your existing MethodArgumentNotValidException/HttpMessageNotReadableException handlers to handle missing/blank body cases.
64-64: Return type widened fromResponseEntity<Map<String, String>>toResponseEntity<?>.
AuthenticationService.refreshToken(...)still returnsResponseEntity<Map<String, String>>. Widening to?here loses the response contract in the signature and weakens generated OpenAPI/type info for clients. Once the manual error branch is removed (see adjacent comment), the specific generic can be restored.🤖 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` at line 64, The controller method AuthenticationController.refreshToken currently widens its return type to ResponseEntity<?>; change its signature back to ResponseEntity<Map<String, String>> to match the service method AuthenticationService.refreshToken(...) so the response contract and OpenAPI/type information are preserved, and ensure any manual error-branch handling is reconciled with the specific generic return type (and update imports if necessary).
🤖 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/controller/AuthenticationController.java`:
- Around line 73-75: In AuthenticationController's token refresh catch block,
change the logging to pass the Throwable itself instead of only e.getMessage()
so SLF4J will include the stacktrace (i.e., call log.error with the exception as
the last argument for the catch block that currently logs "[Auth] Token refresh
failed: {}"), and consider narrowing the caught exception type or removing the
local catch in favor of a global `@ControllerAdvice` handler if the block only
exists for diagnostics.
In `@backend/src/main/java/org/booklore/service/MenuCountsService.java`:
- Around line 90-94: Replace the duplicated inline "not deleted" Criteria
predicate in fetchShelfCounts with the existing notDeleted() Specification
helper by calling notDeleted().toPredicate(root, query, cb) and using that
Predicate instead of recreating the IS NULL OR = false logic; leave
fetchLibraryCounts as-is (it would need a larger refactor to switch from JPQL to
Criteria) and ensure BookEntity.deleted nullable semantics are preserved by
using the helper's NULL-or-false logic.
- Around line 181-183: Change unshelved() to accept the caller's visibleShelfIds
and build a predicate that excludes books that are on any of those shelves:
update the method signature to unshelved(Set<Long> visibleShelfIds) and replace
the cb.isEmpty(root.get("shelves")) check with a left-join/exists-style
predicate that ensures no joined ShelfEntity id is in visibleShelfIds (i.e. the
book has zero shelves among the user's visibleShelfIds). Then update the
unshelvedBookCount query call to pass visibleShelfIds. Reference symbols:
unshelved(), unshelvedBookCount, visibleShelfIds, BookEntity.shelves.
---
Nitpick comments:
In `@backend/src/main/java/org/booklore/controller/AuthenticationController.java`:
- Around line 64-69: The manual null/blank guard inside
AuthenticationController.refreshToken (the check on request == null ||
request.getRefreshToken() == null || isBlank() and the
ResponseEntity.badRequest(Map.of("message", ...))) should be removed because
`@RequestBody` `@Valid` plus `@NotBlank` on RefreshTokenRequest.refreshToken will let
Spring's validation and your global exception handlers produce a consistent 400
payload; delete the conditional and its custom log/response, keep the initial
debug log if desired, and rely on your existing
MethodArgumentNotValidException/HttpMessageNotReadableException handlers to
handle missing/blank body cases.
- Line 64: The controller method AuthenticationController.refreshToken currently
widens its return type to ResponseEntity<?>; change its signature back to
ResponseEntity<Map<String, String>> to match the service method
AuthenticationService.refreshToken(...) so the response contract and
OpenAPI/type information are preserved, and ensure any manual error-branch
handling is reconciled with the specific generic return type (and update imports
if necessary).
🪄 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: e1fde587-71da-4820-b09b-4721238b6230
📒 Files selected for processing (3)
backend/src/main/java/org/booklore/controller/AuthenticationController.javabackend/src/main/java/org/booklore/service/MenuCountsService.javabackend/src/main/java/org/booklore/service/book/BookService.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 (1)
backend/src/**/*.java
📄 CodeRabbit inference engine (AGENTS.md)
backend/src/**/*.java: Use 4-space indentation and match surrounding Java style in backend code
Prefer constructor injection via Lombok patterns already used in the codebase. Do not introduce@Autowiredfield injection in backend code
Use MapStruct for entity/DTO mapping in backend code
Keep JPA entities on the *Entity suffix in backend code
Files:
backend/src/main/java/org/booklore/service/book/BookService.javabackend/src/main/java/org/booklore/controller/AuthenticationController.javabackend/src/main/java/org/booklore/service/MenuCountsService.java
🧠 Learnings (5)
📚 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/service/book/BookService.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/service/book/BookService.javabackend/src/main/java/org/booklore/controller/AuthenticationController.javabackend/src/main/java/org/booklore/service/MenuCountsService.java
📚 Learning: 2026-04-14T12:43:08.698Z
Learnt from: balazs-szucs
Repo: grimmory-tools/grimmory PR: 502
File: booklore-api/src/main/java/org/booklore/service/reader/ChapterCacheService.java:0-0
Timestamp: 2026-04-14T12:43:08.698Z
Learning: For this codebase (booklore-api), target Java 25 with `--enable-preview`, so `_` is intentionally used as an unnamed/ignored variable (e.g., lambda parameter or pattern variable) per Java’s preview feature JEP 456. Do not flag `_` in those contexts as an invalid/reserved identifier; only flag it if it’s used in a non-supported position (e.g., where an unnamed variable is not applicable for the Java preview rules).
Applied to files:
backend/src/main/java/org/booklore/service/book/BookService.javabackend/src/main/java/org/booklore/controller/AuthenticationController.javabackend/src/main/java/org/booklore/service/MenuCountsService.java
📚 Learning: 2026-03-25T19:09:09.638Z
Learnt from: imnotjames
Repo: grimmory-tools/grimmory PR: 189
File: booklore-api/src/main/java/org/booklore/service/kobo/KoboLibrarySyncService.java:113-116
Timestamp: 2026-03-25T19:09:09.638Z
Learning: In `booklore-api/src/main/java/org/booklore/service/kobo/KoboLibrarySyncService.java`, when deciding whether to forward the `Authorization` header to the upstream Kobo server proxy on the `/v1/library/sync` endpoint, a simple null check (`request.getHeader(HttpHeaders.AUTHORIZATION) != null`) is the correct guard. The header is either absent or present — if present it may or may not be a valid token, but validity is determined by the upstream Kobo server, not by Grimmory. Blank/whitespace intermediate states do not occur in practice, and pre-validating the token value before forwarding is not the responsibility of this service.
Applied to files:
backend/src/main/java/org/booklore/controller/AuthenticationController.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/main/java/org/booklore/service/MenuCountsService.java
🔇 Additional comments (3)
backend/src/main/java/org/booklore/service/book/BookService.java (1)
231-250: LGTM!The
userIdis correctly sourced from the authenticated user and populated consistently across PDF, new PDF, and CBX preference builders, matching the pattern already used for the EPUB branch above.backend/src/main/java/org/booklore/service/MenuCountsService.java (2)
108-140: Shelf visibility filter is now correctly applied.The prior-review concern about shelf counts leaking other users' shelf IDs is addressed:
visibleShelfIdsis threaded through and used in theshelfVisibilityPredicate, and the early-return also bails out when the non-admin user has no visible shelves. Looks good.
142-162:⚠️ Potential issue | 🟠 MajorPer-magic-shelf
count()loop can become an N+1 hot spot on a sidebar endpoint.For each magic shelf, this loop:
- Calls
magicShelfBookService.toSpecification(userId, shelfId), which internally invokesvalidateMagicShelfAccess→magicShelfRepository.findById(...)anduserRepository.findByIdWithDetails(userId)(per context snippet fromMagicShelfBookService).- Issues a separate
bookRepository.count(spec.and(notDeleted()))query.So a user with N magic shelves triggers roughly
3NDB round-trips (user fetch + shelf fetch + count), every time the sidebar is refreshed. Given the PR description markets this endpoint as "lightweight counts for libraries, shelves, and magic-shelves", this is likely to dominate its cost.Suggestions (any one is a big win):
- Resolve the user once and pass an overload that skips the per-call
userRepository.findByIdWithDetailsinvalidateMagicShelfAccess.- Batch all magic shelves in a single query where possible, or at minimum cache
access.user()/GroupRuleparsing across the loop.- Consider short-TTL caching of the menu-counts response itself (it's already backed by the new
private, max-age=60policy on the book endpoints per the PR summary — make sure this endpoint has a similar cache-control header).⛔ Skipped due to learnings
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.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.
…visible shelf IDs and streamline not deleted predicate
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
backend/src/main/java/org/booklore/service/MenuCountsService.java (1)
141-161: Magic-shelf counts are N+1 — one count query (plus an access/parse round-trip) per shelf.Each iteration calls
magicShelfBookService.toSpecification(userId, shelfId)(which doesvalidateMagicShelfAccess+ JSON parse on the filter spec) and thenbookRepository.count(spec.and(notDeleted())). For users (especially admins) with many magic shelves this fans out into a lot of sequential DB round-trips on what is intended to be a cheap "menu counts" endpoint.Given magic shelf rules are arbitrary specifications, collapsing to a single query is not trivial. Two pragmatic options to keep this endpoint snappy:
- Cache
MenuCountsResponseper user with a short TTL (seconds), since this endpoint is now behind theprivate, max-age=60cache header introduced elsewhere in this PR.- If the count for a magic shelf isn't strictly needed on first render, consider lazily fetching per-shelf counts from the frontend on demand.
Not blocking, but worth a follow-up if magic-shelf counts become a hot path.
🤖 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 141 - 161, fetchMagicShelfCounts currently performs N+1 work by calling magicShelfBookService.toSpecification(userId, shelf.getId()) and bookRepository.count(...) for each MagicShelf, causing many DB/parse round-trips; fix by introducing a short-lived per-user cache for the whole MenuCountsResponse (or at least the magic-shelf counts) so fetchMagicShelfCounts can return cached Map<Long,Long> for the user when present, falling back to the current computation only on cache miss and populating the cache with a TTL (e.g., seconds); reference fetchMagicShelfCounts, magicShelfBookService.toSpecification, bookRepository.count and the MenuCountsResponse payload when adding the cache key and TTL logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/src/main/java/org/booklore/service/MenuCountsService.java`:
- Around line 115-139: fetchShelfCounts currently uses root.join("shelves") (an
INNER JOIN) so shelves with zero books are omitted; to ensure every visible
shelf is present, pre-seed the counts Map<Long,Long> with visibleShelfIds mapped
to 0L (using the visibleShelfIds collection) before iterating query results and
overlaying actual counts, or alternatively change the join to a LEFT JOIN on
"shelves" and adjust predicates to preserve empty shelves; update the logic
around join("shelves"), visibleShelfIds and counts to guarantee keys for all
visible shelves.
---
Nitpick comments:
In `@backend/src/main/java/org/booklore/service/MenuCountsService.java`:
- Around line 141-161: fetchMagicShelfCounts currently performs N+1 work by
calling magicShelfBookService.toSpecification(userId, shelf.getId()) and
bookRepository.count(...) for each MagicShelf, causing many DB/parse
round-trips; fix by introducing a short-lived per-user cache for the whole
MenuCountsResponse (or at least the magic-shelf counts) so fetchMagicShelfCounts
can return cached Map<Long,Long> for the user when present, falling back to the
current computation only on cache miss and populating the cache with a TTL
(e.g., seconds); reference fetchMagicShelfCounts,
magicShelfBookService.toSpecification, bookRepository.count and the
MenuCountsResponse payload when adding the cache key and TTL logic.
🪄 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: 133fe2c7-f84c-42b9-bf13-15fdc403043d
📒 Files selected for processing (1)
backend/src/main/java/org/booklore/service/MenuCountsService.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). (5)
- GitHub Check: Test Suite / Frontend Tests
- GitHub Check: Test Suite / Backend Tests
- GitHub Check: Analyze (java-kotlin)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Frontend Lint Threshold Check
🧰 Additional context used
📓 Path-based instructions (1)
backend/src/**/*.java
📄 CodeRabbit inference engine (AGENTS.md)
backend/src/**/*.java: Use 4-space indentation and match surrounding Java style in backend code
Prefer constructor injection via Lombok patterns already used in the codebase. Do not introduce@Autowiredfield injection in backend code
Use MapStruct for entity/DTO mapping in backend code
Keep JPA entities on the *Entity suffix in backend code
Files:
backend/src/main/java/org/booklore/service/MenuCountsService.java
🧠 Learnings (6)
📚 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/main/java/org/booklore/service/MenuCountsService.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/service/MenuCountsService.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/service/MenuCountsService.java
📚 Learning: 2026-03-26T03:22:24.500Z
Learnt from: imnotjames
Repo: grimmory-tools/grimmory PR: 209
File: booklore-api/src/main/java/org/booklore/util/FileUtils.java:32-38
Timestamp: 2026-03-26T03:22:24.500Z
Learning: In `grimmory-tools/grimmory`, `BookFileEntity.fileSubPath` (in `booklore-api/src/main/java/org/booklore/model/entity/BookFileEntity.java`) is declared with `Column(name = "file_sub_path", length = 512, nullable = false)`, meaning it is never null. Do not flag null-guard concerns for this field.
Applied to files:
backend/src/main/java/org/booklore/service/MenuCountsService.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/service/MenuCountsService.java
📚 Learning: 2026-04-14T12:43:08.698Z
Learnt from: balazs-szucs
Repo: grimmory-tools/grimmory PR: 502
File: booklore-api/src/main/java/org/booklore/service/reader/ChapterCacheService.java:0-0
Timestamp: 2026-04-14T12:43:08.698Z
Learning: For this codebase (booklore-api), target Java 25 with `--enable-preview`, so `_` is intentionally used as an unnamed/ignored variable (e.g., lambda parameter or pattern variable) per Java’s preview feature JEP 456. Do not flag `_` in those contexts as an invalid/reserved identifier; only flag it if it’s used in a non-supported position (e.g., where an unnamed variable is not applicable for the Java preview rules).
Applied to files:
backend/src/main/java/org/booklore/service/MenuCountsService.java
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
The new endpoints will be used. Right now they aren't. I know it.
This pull request introduces several improvements across caching strategy, authentication handling, API endpoints, and model mapping in the backend. The most significant changes are the addition of fine-grained cache-control headers for static resources and API responses, improved handling of authentication edge cases, and the introduction of a new lightweight endpoint for retrieving menu counts. Several DTOs and mappers are also updated to support new features.
Caching and Static Resource Handling
WebMvcConfig, ensuring fingerprinted assets are cached for one year, service worker files andindex.htmlare always revalidated, and third-party assets are cached for one day. This improves frontend performance and update reliability.private, max-age=60) and user profile (no-cache, no-store, must-revalidate) to optimize client-side caching and data freshness.Authentication and Security Improvements
/api/v1/app/bootstrapendpoint by updating both the security config and authentication filter, ensuring smoother client bootstrapping.AuthenticationService: now safely returnsnullfor unauthenticated or anonymous users instead of throwing exceptions.AuthenticationController, providing clearer error messages for invalid requests.API and Model Enhancements
/books/menu-countsendpoint that returns lightweight counts for libraries, shelves, and magic-shelves, reducing load on the client when rendering sidebars. Introduced theMenuCountsResponseDTO and supporting service.BookMapperinterface and related DTOs (PdfViewerPreferences,NewPdfViewerPreferences,CbxViewerPreferences) to includeuserIdfields, supporting user-specific viewer preferences.Repository Improvements
findAllIds()toLibraryRepositoryto efficiently fetch all library IDs, supporting the new menu counts feature.Summary by CodeRabbit
New Features
Bug Fixes
Performance