fix(dto): add DailyListeningDto and update session-related DTOs to use LocalDate#403
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughController metadata update now delegates to a new transactional service method; analytics and session queries moved from native/timezone SQL to JPQL with Instant-based period bounds; notebook entries consolidated into a read-only Subselect entity; repository methods adopted keyset/Pageable patterns and library paths are eagerly loaded at call sites. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant MetadataController as "MetadataController"
participant BookMetadataService as "BookMetadataService"
participant BookRepository as "BookRepository"
participant BookMetadataUpdater as "BookMetadataUpdater"
participant AuditService as "AuditService"
participant BookMetadataMapper as "BookMetadataMapper"
Client->>MetadataController: PUT /books/{id}/metadata (wrapper, merge, replaceMode)
MetadataController->>BookMetadataService: updateBookMetadata(bookId, wrapper, merge, replaceMode)
BookMetadataService->>BookRepository: findAllWithMetadataByIds([bookId])
BookRepository-->>BookMetadataService: BookEntity
BookMetadataService->>BookMetadataUpdater: setBookMetadata(context)
BookMetadataUpdater-->>BookMetadataService: applied
BookMetadataService->>AuditService: audit(METADATA_UPDATED, details)
BookMetadataService->>BookMetadataMapper: toBookMetadata(metadata, true)
BookMetadataMapper-->>BookMetadataService: BookMetadata DTO
BookMetadataService-->>MetadataController: BookMetadata
MetadataController-->>Client: 200 OK (BookMetadata)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (2 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 |
…ository method calls
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
booklore-api/src/main/java/org/booklore/service/opds/OpdsBookService.java (1)
163-170:⚠️ Potential issue | 🟡 MinorHandle non-positive
countbefore building the page request.
PageRequest.of(0, count)throws forcount < 1, so this path now fails with anIllegalArgumentExceptioninstead of handling the edge case explicitly.Suggested change
public List<Book> getRandomBooks(Long userId, int count) { + if (count < 1) { + return List.of(); + } + List<Library> accessibleLibraries = getAccessibleLibraries(userId); if (accessibleLibraries == null || accessibleLibraries.isEmpty()) { return List.of(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@booklore-api/src/main/java/org/booklore/service/opds/OpdsBookService.java` around lines 163 - 170, In getRandomBooks(Long userId, int count) validate the count parameter before creating the PageRequest: if count < 1, return an empty list (List.of()) to avoid PageRequest.of(0, count) throwing IllegalArgumentException; keep the rest of the flow (getAccessibleLibraries, libraryIds, and bookOpdsRepository.findRandomBookIdsByLibraryIds(..., PageRequest.of(0, count))) unchanged for valid counts.
🧹 Nitpick comments (4)
booklore-api/src/main/java/org/booklore/service/ReadingSessionService.java (1)
681-691: Weekly trend results may not be chronologically ordered across year boundaries.The
LinkedHashMappreserves insertion order based on the repository'sORDER BY sessionDateclause. However, if sessions span a year boundary (e.g., week 52 of 2025 and week 1 of 2026), the aggregated results will be in date order rather than strict year-week order. Consider adding a sort:♻️ Optional: Sort by year then week
return weeklyMap.entrySet().stream() .map(entry -> { String[] parts = entry.getKey().split("-"); return WeeklyListeningTrendResponse.builder() .year(Integer.parseInt(parts[0])) .week(Integer.parseInt(parts[1])) .totalDurationSeconds(entry.getValue()[0]) .sessions(entry.getValue()[1]) .build(); }) + .sorted(Comparator.comparingInt(WeeklyListeningTrendResponse::getYear) + .thenComparingInt(WeeklyListeningTrendResponse::getWeek)) .collect(Collectors.toList());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@booklore-api/src/main/java/org/booklore/service/ReadingSessionService.java` around lines 681 - 691, The weeklyMap stream maps entries with keys like "YYYY-W" into WeeklyListeningTrendResponse but doesn't guarantee chronological ordering across year boundaries; modify the pipeline to sort the entries before mapping by parsing the entry.getKey into year and week and applying a comparator that compares year then week (e.g., sort by Integer.parseInt(parts[0]) then Integer.parseInt(parts[1])) so the resulting List<WeeklyListeningTrendResponse> is ordered by year then week chronologically.booklore-api/src/main/java/org/booklore/model/entity/NotebookEntryView.java (1)
4-5: Drop the setters on this read-only entity.
@Immutablemeans Hibernate will never flush changes back, so exposing mutators here just makes it easier to create in-memory state that can never be persisted.Suggested change
import jakarta.persistence.*; import lombok.Getter; -import lombok.Setter; import org.hibernate.annotations.Immutable; import org.hibernate.annotations.Subselect; import org.hibernate.annotations.Synchronize; @@ `@Getter` -@Setter `@Entity` `@Immutable`Also applies to: 13-16
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@booklore-api/src/main/java/org/booklore/model/entity/NotebookEntryView.java` around lines 4 - 5, The class NotebookEntryView is a read-only/@Immutable entity but still exposes mutators; remove all setter generation by deleting the lombok.Setter import and removing any `@Setter` annotations on the NotebookEntryView class or its fields so only getters are generated (keep lombok.Getter and the `@Immutable` annotation intact); this ensures no setter methods are present for the NotebookEntryView entity.booklore-api/src/main/java/org/booklore/service/metadata/BookMetadataService.java (1)
70-91: Unify this withupdateMetadata(...)below.This new entry point duplicates the older update path almost line-for-line, and the two implementations have already drifted on how the book is loaded. Extracting a shared helper would keep both call paths from diverging further.
Suggested change
`@Transactional` public BookMetadata updateBookMetadata(long bookId, MetadataUpdateWrapper metadataUpdateWrapper, boolean mergeCategories, MetadataReplaceMode replaceMode) { - BookEntity bookEntity = bookRepository.findAllWithMetadataByIds(Collections.singleton(bookId)).stream() - .findFirst() - .orElseThrow(() -> ApiError.BOOK_NOT_FOUND.createException(bookId)); - - MetadataUpdateContext context = MetadataUpdateContext.builder() - .bookEntity(bookEntity) - .metadataUpdateWrapper(metadataUpdateWrapper) - .updateThumbnail(true) - .mergeCategories(mergeCategories) - .replaceMode(replaceMode) - .mergeMoods(false) - .mergeTags(false) - .build(); - - bookMetadataUpdater.setBookMetadata(context); - auditService.log(AuditAction.METADATA_UPDATED, "Book", bookId, - "Updated metadata for book: " + bookEntity.getMetadata().getTitle()); - return bookMetadataMapper.toBookMetadata(bookEntity.getMetadata(), true); + return doUpdateMetadata(bookId, metadataUpdateWrapper, mergeCategories, replaceMode); }private BookMetadata doUpdateMetadata(long bookId, MetadataUpdateWrapper wrapper, boolean mergeCategories, MetadataReplaceMode replaceMode) { BookEntity bookEntity = bookRepository.findByIdFull(bookId) .orElseThrow(() -> ApiError.BOOK_NOT_FOUND.createException(bookId)); MetadataUpdateContext context = MetadataUpdateContext.builder() .bookEntity(bookEntity) .metadataUpdateWrapper(wrapper) .updateThumbnail(true) .mergeCategories(mergeCategories) .replaceMode(replaceMode) .mergeMoods(false) .mergeTags(false) .build(); bookMetadataUpdater.setBookMetadata(context); auditService.log(AuditAction.METADATA_UPDATED, "Book", bookId, "Updated metadata for book: " + bookEntity.getMetadata().getTitle()); return bookMetadataMapper.toBookMetadata(bookEntity.getMetadata(), true); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@booklore-api/src/main/java/org/booklore/service/metadata/BookMetadataService.java` around lines 70 - 91, The new updateBookMetadata duplicates logic from updateMetadata and they diverge in how the book is loaded; extract a single private helper (e.g., doUpdateMetadata) used by both updateBookMetadata and updateMetadata that: loads the BookEntity via bookRepository.findByIdFull(bookId) (or unify to the preferred loader), builds MetadataUpdateContext (updateThumbnail true, mergeMoods false, mergeTags false), calls bookMetadataUpdater.setBookMetadata(context), logs via auditService.log(AuditAction.METADATA_UPDATED, "Book", bookId, "Updated metadata for book: " + bookEntity.getMetadata().getTitle()), and returns bookMetadataMapper.toBookMetadata(..., true); then update both public methods to delegate to this helper to avoid duplication and drift.booklore-api/src/test/java/org/booklore/service/OpdsBookServiceTest.java (1)
314-315: Assert the requested page size explicitly.Both stubs accept any
Pageable, so these tests still pass ifgetRandomBooks()stops usingPageRequest.of(0, count)or asks for too many IDs. Verifying the pageable argument would lock in the behavior this refactor is meant to provide.Suggested change
- when(bookOpdsRepository.findRandomBookIdsByLibraryIds(anyList(), any())).thenReturn(List.of(1L, 2L)); + when(bookOpdsRepository.findRandomBookIdsByLibraryIds( + eq(List.of(1L)), + argThat(pageable -> pageable.getPageNumber() == 0 && pageable.getPageSize() == 1))) + .thenReturn(List.of(1L)); @@ + verify(bookOpdsRepository).findRandomBookIdsByLibraryIds( + eq(List.of(1L)), + argThat(pageable -> pageable.getPageNumber() == 0 && pageable.getPageSize() == 1));Apply the same pattern to the
count = 5case below.Also applies to: 753-754
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@booklore-api/src/test/java/org/booklore/service/OpdsBookServiceTest.java` around lines 314 - 315, The test currently stubs findRandomBookIdsByLibraryIds with any Pageable so it won't fail if getRandomBooks() changes page size; update the stubbing/assertion in OpdsBookServiceTest to verify the pageable requested (use an argument matcher or capture the Pageable) and assert it equals PageRequest.of(0, count) for the count=2 case shown (targeting the repository method findRandomBookIdsByLibraryIds and the test exercising getRandomBooks()), and apply the same explicit pageable check for the count=5 case later (lines referenced in the review).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@booklore-api/src/main/java/org/booklore/repository/BookRepository.java`:
- Around line 174-195: The repository method
findFirstByLibraryIdAndLibraryPathIdAndFileSubPathAndFileName relies on an
unordered JPQL query (the `@Query` on
findByLibraryIdAndLibraryPathIdAndFileSubPathAndFileName), which can return an
arbitrary row when multiple matches exist; make the result deterministic by
adding an explicit ORDER BY to that JPQL (for example order by b.deleted asc,
b.updatedAt desc, b.id asc to prefer non-deleted and most-recent records) or
another ordering appropriate for your domain, so the PageRequest.of(0,1) in
findFirstByLibraryIdAndLibraryPathIdAndFileSubPathAndFileName consistently
returns the intended record. Ensure the ORDER BY is added inside the `@Query`
string for the method findByLibraryIdAndLibraryPathIdAndFileSubPathAndFileName.
In
`@booklore-api/src/main/java/org/booklore/service/book/BookDownloadService.java`:
- Around line 44-45: The class-level `@Transactional`(readOnly = true) on
BookDownloadService keeps a DB transaction open for long-running operations
(e.g., downloadAllBookFiles and downloadKoboBook); remove the class-level
annotation and instead annotate only the short-lived lookup methods (or helper
methods that perform entity queries) with `@Transactional`(readOnly = true) so DB
lookups run inside a scoped transaction/persistence context while ZIP creation,
conversion and streaming in downloadAllBookFiles and downloadKoboBook execute
outside any transaction/connection.
---
Outside diff comments:
In `@booklore-api/src/main/java/org/booklore/service/opds/OpdsBookService.java`:
- Around line 163-170: In getRandomBooks(Long userId, int count) validate the
count parameter before creating the PageRequest: if count < 1, return an empty
list (List.of()) to avoid PageRequest.of(0, count) throwing
IllegalArgumentException; keep the rest of the flow (getAccessibleLibraries,
libraryIds, and bookOpdsRepository.findRandomBookIdsByLibraryIds(...,
PageRequest.of(0, count))) unchanged for valid counts.
---
Nitpick comments:
In `@booklore-api/src/main/java/org/booklore/model/entity/NotebookEntryView.java`:
- Around line 4-5: The class NotebookEntryView is a read-only/@Immutable entity
but still exposes mutators; remove all setter generation by deleting the
lombok.Setter import and removing any `@Setter` annotations on the
NotebookEntryView class or its fields so only getters are generated (keep
lombok.Getter and the `@Immutable` annotation intact); this ensures no setter
methods are present for the NotebookEntryView entity.
In
`@booklore-api/src/main/java/org/booklore/service/metadata/BookMetadataService.java`:
- Around line 70-91: The new updateBookMetadata duplicates logic from
updateMetadata and they diverge in how the book is loaded; extract a single
private helper (e.g., doUpdateMetadata) used by both updateBookMetadata and
updateMetadata that: loads the BookEntity via
bookRepository.findByIdFull(bookId) (or unify to the preferred loader), builds
MetadataUpdateContext (updateThumbnail true, mergeMoods false, mergeTags false),
calls bookMetadataUpdater.setBookMetadata(context), logs via
auditService.log(AuditAction.METADATA_UPDATED, "Book", bookId, "Updated metadata
for book: " + bookEntity.getMetadata().getTitle()), and returns
bookMetadataMapper.toBookMetadata(..., true); then update both public methods to
delegate to this helper to avoid duplication and drift.
In `@booklore-api/src/main/java/org/booklore/service/ReadingSessionService.java`:
- Around line 681-691: The weeklyMap stream maps entries with keys like "YYYY-W"
into WeeklyListeningTrendResponse but doesn't guarantee chronological ordering
across year boundaries; modify the pipeline to sort the entries before mapping
by parsing the entry.getKey into year and week and applying a comparator that
compares year then week (e.g., sort by Integer.parseInt(parts[0]) then
Integer.parseInt(parts[1])) so the resulting List<WeeklyListeningTrendResponse>
is ordered by year then week chronologically.
In `@booklore-api/src/test/java/org/booklore/service/OpdsBookServiceTest.java`:
- Around line 314-315: The test currently stubs findRandomBookIdsByLibraryIds
with any Pageable so it won't fail if getRandomBooks() changes page size; update
the stubbing/assertion in OpdsBookServiceTest to verify the pageable requested
(use an argument matcher or capture the Pageable) and assert it equals
PageRequest.of(0, count) for the count=2 case shown (targeting the repository
method findRandomBookIdsByLibraryIds and the test exercising getRandomBooks()),
and apply the same explicit pageable check for the count=5 case later (lines
referenced in the review).
🪄 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: 0affebbf-9e3b-42fc-b6d2-a879112a5d27
📒 Files selected for processing (26)
booklore-api/src/main/java/org/booklore/controller/MetadataController.javabooklore-api/src/main/java/org/booklore/model/dto/DailyListeningDto.javabooklore-api/src/main/java/org/booklore/model/dto/FavoriteReadingDayDto.javabooklore-api/src/main/java/org/booklore/model/dto/SessionScatterDto.javabooklore-api/src/main/java/org/booklore/model/entity/LibraryEntity.javabooklore-api/src/main/java/org/booklore/model/entity/NotebookEntryView.javabooklore-api/src/main/java/org/booklore/repository/BookOpdsRepository.javabooklore-api/src/main/java/org/booklore/repository/BookRepository.javabooklore-api/src/main/java/org/booklore/repository/NotebookEntryRepository.javabooklore-api/src/main/java/org/booklore/repository/ReadingSessionRepository.javabooklore-api/src/main/java/org/booklore/service/ReadingSessionService.javabooklore-api/src/main/java/org/booklore/service/book/BookCreatorService.javabooklore-api/src/main/java/org/booklore/service/book/BookDownloadService.javabooklore-api/src/main/java/org/booklore/service/file/FileMoveService.javabooklore-api/src/main/java/org/booklore/service/library/LibraryProcessingService.javabooklore-api/src/main/java/org/booklore/service/metadata/BookMetadataService.javabooklore-api/src/main/java/org/booklore/service/migration/migrations/GenerateCoverHashMigration.javabooklore-api/src/main/java/org/booklore/service/migration/migrations/PopulateSearchTextMigration.javabooklore-api/src/main/java/org/booklore/service/opds/OpdsBookService.javabooklore-api/src/main/resources/application.yamlbooklore-api/src/main/resources/db/migration/V136__Add_reading_session_booktype_index.sqlbooklore-api/src/test/java/org/booklore/controller/MetadataControllerTest.javabooklore-api/src/test/java/org/booklore/service/OpdsBookServiceTest.javabooklore-api/src/test/java/org/booklore/service/file/FileMoveServiceOrderingTest.javabooklore-api/src/test/java/org/booklore/service/file/FileMoveServiceTest.javabooklore-api/src/test/java/org/booklore/service/library/LibraryProcessingServiceTest.java
💤 Files with no reviewable changes (1)
- booklore-api/src/main/resources/application.yaml
📜 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). (1)
- GitHub Check: Packaging Smoke Test
🧰 Additional context used
📓 Path-based instructions (4)
booklore-api/src/**/*.java
📄 CodeRabbit inference engine (AGENTS.md)
booklore-api/src/**/*.java: Use 4-space indentation in Java files and match surrounding Java style
Prefer constructor injection via Lombok patterns already used in the codebase. Do not introduce@Autowiredfield injection
Use MapStruct for entity/DTO mapping
Files:
booklore-api/src/main/java/org/booklore/controller/MetadataController.javabooklore-api/src/main/java/org/booklore/model/dto/DailyListeningDto.javabooklore-api/src/main/java/org/booklore/model/entity/LibraryEntity.javabooklore-api/src/main/java/org/booklore/service/migration/migrations/GenerateCoverHashMigration.javabooklore-api/src/main/java/org/booklore/model/dto/SessionScatterDto.javabooklore-api/src/test/java/org/booklore/service/library/LibraryProcessingServiceTest.javabooklore-api/src/main/java/org/booklore/model/dto/FavoriteReadingDayDto.javabooklore-api/src/main/java/org/booklore/service/migration/migrations/PopulateSearchTextMigration.javabooklore-api/src/main/java/org/booklore/service/library/LibraryProcessingService.javabooklore-api/src/test/java/org/booklore/service/file/FileMoveServiceOrderingTest.javabooklore-api/src/test/java/org/booklore/service/file/FileMoveServiceTest.javabooklore-api/src/test/java/org/booklore/service/OpdsBookServiceTest.javabooklore-api/src/main/java/org/booklore/service/file/FileMoveService.javabooklore-api/src/main/java/org/booklore/service/metadata/BookMetadataService.javabooklore-api/src/main/java/org/booklore/service/book/BookDownloadService.javabooklore-api/src/main/java/org/booklore/service/book/BookCreatorService.javabooklore-api/src/test/java/org/booklore/controller/MetadataControllerTest.javabooklore-api/src/main/java/org/booklore/service/opds/OpdsBookService.javabooklore-api/src/main/java/org/booklore/service/ReadingSessionService.javabooklore-api/src/main/java/org/booklore/repository/BookOpdsRepository.javabooklore-api/src/main/java/org/booklore/repository/NotebookEntryRepository.javabooklore-api/src/main/java/org/booklore/model/entity/NotebookEntryView.javabooklore-api/src/main/java/org/booklore/repository/BookRepository.javabooklore-api/src/main/java/org/booklore/repository/ReadingSessionRepository.java
booklore-api/src/**/*Entity.java
📄 CodeRabbit inference engine (AGENTS.md)
Keep JPA entities on the
*Entitysuffix
Files:
booklore-api/src/main/java/org/booklore/model/entity/LibraryEntity.java
booklore-api/src/test/**/*.java
📄 CodeRabbit inference engine (AGENTS.md)
Prefer focused unit tests; use
@SpringBootTestonly when the Spring context is required
Files:
booklore-api/src/test/java/org/booklore/service/library/LibraryProcessingServiceTest.javabooklore-api/src/test/java/org/booklore/service/file/FileMoveServiceOrderingTest.javabooklore-api/src/test/java/org/booklore/service/file/FileMoveServiceTest.javabooklore-api/src/test/java/org/booklore/service/OpdsBookServiceTest.javabooklore-api/src/test/java/org/booklore/controller/MetadataControllerTest.java
booklore-api/src/main/resources/db/migration/V*.sql
📄 CodeRabbit inference engine (AGENTS.md)
Add Flyway migrations as new files named
V<number>__<Description>.sql
Files:
booklore-api/src/main/resources/db/migration/V136__Add_reading_session_booktype_index.sql
🧠 Learnings (10)
📚 Learning: 2026-03-26T01:46:48.863Z
Learnt from: CR
Repo: grimmory-tools/grimmory PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-26T01:46:48.863Z
Learning: Applies to booklore-api/src/**/*.java : Use MapStruct for entity/DTO mapping
Applied to files:
booklore-api/src/main/java/org/booklore/controller/MetadataController.java
📚 Learning: 2026-03-26T01:46:48.863Z
Learnt from: CR
Repo: grimmory-tools/grimmory PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-26T01:46:48.863Z
Learning: Applies to booklore-api/src/**/*.java : Prefer constructor injection via Lombok patterns already used in the codebase. Do not introduce `Autowired` field injection
Applied to files:
booklore-api/src/main/java/org/booklore/controller/MetadataController.javabooklore-api/src/main/java/org/booklore/service/migration/migrations/GenerateCoverHashMigration.javabooklore-api/src/main/java/org/booklore/service/migration/migrations/PopulateSearchTextMigration.javabooklore-api/src/test/java/org/booklore/service/OpdsBookServiceTest.javabooklore-api/src/main/java/org/booklore/service/book/BookDownloadService.java
📚 Learning: 2026-03-26T01:46:48.863Z
Learnt from: CR
Repo: grimmory-tools/grimmory PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-26T01:46:48.863Z
Learning: Applies to booklore-api/src/**/*Entity.java : Keep JPA entities on the `*Entity` suffix
Applied to files:
booklore-api/src/main/java/org/booklore/model/entity/LibraryEntity.javabooklore-api/src/main/java/org/booklore/model/entity/NotebookEntryView.javabooklore-api/src/main/java/org/booklore/repository/ReadingSessionRepository.java
📚 Learning: 2026-04-01T17:50:06.817Z
Learnt from: pedronave
Repo: grimmory-tools/grimmory PR: 247
File: booklore-api/src/main/java/org/booklore/service/hardcover/HardcoverSyncService.java:156-159
Timestamp: 2026-04-01T17:50:06.817Z
Learning: In grimmory-tools/grimmory (HardcoverSyncService.java), the Hardcover API already returns `user_book_reads` in a consistent, ordered fashion (ascending by recency), so `getLast()` reliably retrieves the most recent read entry. Adding an explicit `order_by` to the `user_book_reads` query is unnecessary.
Applied to files:
booklore-api/src/main/java/org/booklore/service/migration/migrations/GenerateCoverHashMigration.javabooklore-api/src/main/java/org/booklore/service/opds/OpdsBookService.javabooklore-api/src/main/java/org/booklore/repository/ReadingSessionRepository.java
📚 Learning: 2026-03-26T01:46:48.863Z
Learnt from: CR
Repo: grimmory-tools/grimmory PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-26T01:46:48.863Z
Learning: Applies to booklore-api/src/main/resources/db/migration/V*.sql : Add Flyway migrations as new files named `V<number>__<Description>.sql`
Applied to files:
booklore-api/src/main/resources/db/migration/V136__Add_reading_session_booktype_index.sql
📚 Learning: 2026-03-26T01:46:48.863Z
Learnt from: CR
Repo: grimmory-tools/grimmory PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-26T01:46:48.863Z
Learning: Applies to booklore-api/src/test/**/*.java : Prefer focused unit tests; use `SpringBootTest` only when the Spring context is required
Applied to files:
booklore-api/src/test/java/org/booklore/service/OpdsBookServiceTest.javabooklore-api/src/test/java/org/booklore/controller/MetadataControllerTest.java
📚 Learning: 2026-03-23T18:36:05.843Z
Learnt from: imnotjames
Repo: grimmory-tools/grimmory PR: 146
File: booklore-api/src/main/java/org/booklore/util/FileService.java:151-168
Timestamp: 2026-03-23T18:36:05.843Z
Learning: In `booklore-api/src/main/java/org/booklore/util/FileService.java`, the `"bin"` search path in `getSystemSearchPath()` is intentionally left as a relative path (not derived from `appProperties.getPathConfig()`), so binary lookup resolves relative to the process working directory. Do not flag this as an issue.
Applied to files:
booklore-api/src/main/java/org/booklore/service/file/FileMoveService.javabooklore-api/src/main/java/org/booklore/service/book/BookCreatorService.java
📚 Learning: 2026-03-26T02:20:27.862Z
Learnt from: imnotjames
Repo: grimmory-tools/grimmory PR: 198
File: booklore-api/src/main/java/org/booklore/controller/KoboController.java:100-103
Timestamp: 2026-03-26T02:20:27.862Z
Learning: In `booklore-api/src/main/java/org/booklore/controller/KoboController.java`, the Kobo reader device already makes direct outbound connections to Kobo's CDN/servers for other requests (confirmed by imnotjames via testing). Therefore, returning a `307 TEMPORARY_REDIRECT` to `cdn.kobo.com` for non-`BL-` book cover thumbnails (instead of server-side proxying) does NOT introduce a new privacy or behavioral regression — the Kobo device's IP and User-Agent are already exposed to Kobo through other direct requests.
Applied to files:
booklore-api/src/main/java/org/booklore/service/book/BookDownloadService.java
📚 Learning: 2026-04-04T15:36:55.394Z
Learnt from: balazs-szucs
Repo: grimmory-tools/grimmory PR: 372
File: booklore-api/src/main/java/org/booklore/app/service/AppBookService.java:357-359
Timestamp: 2026-04-04T15:36:55.394Z
Learning: In `booklore-api/src/main/java/org/booklore/app/service/AppBookService.java`, `shelfId` and `magicShelfId` are mutually exclusive navigation contexts in `getFilterOptions`. A request will never supply both at the same time, so the `else if (shelfId != null)` branch after the `magicBookIds != null` check is intentional and correct — there is no missing shelf-∩-magicShelf intersection case. Do not flag this pattern as a bug.
Applied to files:
booklore-api/src/main/java/org/booklore/service/book/BookCreatorService.java
📚 Learning: 2026-03-24T18:46:47.249Z
Learnt from: pedronave
Repo: grimmory-tools/grimmory PR: 173
File: booklore-api/src/main/java/org/booklore/service/hardcover/HardcoverSyncService.java:188-199
Timestamp: 2026-03-24T18:46:47.249Z
Learning: In grimmory-tools/grimmory (HardcoverSyncService.java), when a `hardcoverBookId` is stored in book metadata but is malformed (non-numeric), the preferred behavior is to return null and skip the sync rather than falling back to ISBN-based lookup. The reasoning is that an explicitly set bookId represents deliberate user intent, and silently resolving via ISBN could sync progress to a different book than intended.
Applied to files:
booklore-api/src/main/java/org/booklore/service/book/BookCreatorService.javabooklore-api/src/main/java/org/booklore/service/opds/OpdsBookService.java
🔇 Additional comments (24)
booklore-api/src/main/java/org/booklore/model/entity/LibraryEntity.java (1)
38-41: LGTM! Good optimization for batched lazy-loading.Adding
@BatchSize(size = 20)to thelibraryPathscollection helps mitigate N+1 query problems when multiple libraries have their paths accessed. This aligns well with the PR's shift toward usingfindByIdWithPaths(...)for explicit path loading where needed, while still optimizing the lazy-load fallback case.booklore-api/src/test/java/org/booklore/service/file/FileMoveServiceOrderingTest.java (1)
183-183: LGTM! Test stubs correctly updated to match the new repository method.The stubs for
libraryRepository.findByIdWithPaths(1L)returningOptional.of(library)align with the production code changes inFileMoveService. The test fixture already properly initializeslibrary.setLibraryPaths(List.of(libraryPath))at line 104, ensuring thegetLibraryPaths().stream()...code path inFileMoveServiceis exercised correctly.Also applies to: 320-320
booklore-api/src/test/java/org/booklore/service/library/LibraryProcessingServiceTest.java (1)
95-95: LGTM! Test stubs updated consistently across allprocessLibrary_*test cases.The stubbing changes from
findByIdtofindByIdWithPathscorrectly align with the production code changes inLibraryProcessingService. The tests properly verify the service behavior with the new repository method signature.Also applies to: 157-157, 191-191, 239-239, 284-284
booklore-api/src/main/java/org/booklore/service/library/LibraryProcessingService.java (1)
53-54: LGTM! Consistent use offindByIdWithPathsfor path-dependent operations.Switching to
findByIdWithPaths(libraryId)ensures thatlibraryPathsare eagerly loaded beforelibraryFileHelper.getLibraryFiles(libraryEntity)is invoked, which likely iterates over the library's paths. This aligns with the PR's broader goal of ensuring path associations are available during processing flows.booklore-api/src/test/java/org/booklore/service/file/FileMoveServiceTest.java (2)
147-152: LGTM! Test helper method updated consistently.The
mockMonitoredLibrary()helper properly stubsfindByIdWithPaths(1L)to return the library fixture, ensuring the monitored-library code path inFileMoveServiceis correctly exercised. This reduces duplication across multiple test cases.
787-787: LGTM! Bulk move test stubs correctly updated for new repository method.All bulk move scenarios properly stub
findByIdWithPaths(...)for both source and target libraries, including edge cases where the library is not found (Optional.empty()). The tests comprehensively cover the updated production code paths.Also applies to: 953-953, 984-984, 1014-1014, 1050-1051, 1091-1092
booklore-api/src/main/java/org/booklore/service/file/FileMoveService.java (3)
74-76: LGTM! Consistent library re-registration after bulk move.Using
findByIdWithPaths(libraryId)ensures the library entity has its paths loaded before mapping to DTO and re-registering with the monitoring service. This prevents potential lazy-loading issues.
395-407: Excellent documentation explaining the query separation rationale.The comment clearly explains why
libraryPathsmust be loaded via a separate query rather than through thebookWithFilesentity graph—specifically to avoidLazyInitializationException(OSIV disabled) andMultipleBagFetchException(cannot eagerly fetch bothbookFilesandlibraryPaths). This is valuable context for future maintainers.
441-445: LGTM! Clean re-registration pattern usingifPresent.The refactored code properly wraps the mapping/watch/registration logic inside
findByIdWithPaths(...).ifPresent(...), which elegantly handles the case where the library might not exist (though unlikely in practice). Settingwatch=truebefore registration ensures the library remains monitored after the move operation.booklore-api/src/main/java/org/booklore/model/dto/SessionScatterDto.java (1)
3-8: LGTM!The interface correctly exposes
LocalDate getSessionDate()to align with the JPQLcast(... as LocalDate)projection. The service layer appropriately derives day-of-week viadto.getSessionDate().getDayOfWeek().getValue().booklore-api/src/main/java/org/booklore/model/dto/FavoriteReadingDayDto.java (1)
3-9: LGTM!The interface correctly defines
LocalDate getSessionDate()matching the repository query'ssessionDatealias. The service layer properly aggregates results by ISO day-of-week for improved dialect portability.booklore-api/src/main/java/org/booklore/model/dto/DailyListeningDto.java (1)
1-9: LGTM!The new projection interface is consistent with the DTO contract pattern used throughout this PR. The accessor names align with the repository query aliases (
sessionDate,totalDurationSeconds,sessions).booklore-api/src/main/java/org/booklore/repository/ReadingSessionRepository.java (3)
56-72: Inconsistent temporal type:LocalDateTimevsInstant.This query uses
LocalDateTimeforperiodStart/periodEndparameters while all other period-based queries useInstant. This may cause unexpected behavior ifrs.createdAtis mapped as anInstantin the entity.Consider aligning with the other queries by using
Instantparameters and adjusting the service method accordingly, or verify that the entity'screatedAtfield is typed asLocalDateTime.[raise_minor_issue, request_verification]
#!/bin/bash # Description: Check the type of createdAt field in ReadingSessionEntity ast-grep --pattern $'class ReadingSessionEntity { $$$ createdAt $$$ }' # Also search for the field declaration directly rg -n 'createdAt' --type=java -C3 | rg -i 'ReadingSessionEntity|private.*createdAt'
19-32: LGTM on the period-based query pattern.The migration to JPQL with
timestampadd(second, :tzOffsetSeconds, rs.startTime)for timezone adjustment andInstant-based period bounds provides a clean, consistent pattern across the repository.
207-212: Pagination withListreturn type and projection interfaces works correctly.The method properly applies pagination—Spring Data JPA applies
LIMITto@Querymethods whenPageableis used, even withListreturn types and projection interfaces. The service correctly passesPageRequest.of(0, 500)(lines 534-535 inReadingSessionService), confirming the 500-row limit is applied as intended.> Likely an incorrect or invalid review comment.booklore-api/src/main/resources/db/migration/V136__Add_reading_session_booktype_index.sql (1)
1-3: LGTM!The covering index
(user_id, book_type, start_time DESC)directly supports the audiobook-filtered queries introduced in the repository refactoring. UsingIF NOT EXISTSensures idempotency. As per coding guidelines, the migration follows theV<number>__<Description>.sqlnaming convention.booklore-api/src/main/java/org/booklore/service/ReadingSessionService.java (2)
55-79: Clean timezone and period bounds abstraction.The
PeriodBoundsrecord and helper methods (computeYearBounds,computeMonthBounds,computeOptionalBounds) provide a clean, consistent pattern for period-based queries. UsingZoneId.systemDefault()for timezone calculations is appropriate for server-side analytics.
615-620: No issues found.BookTimelineDto.getFirstSessionDate()andgetLastSessionDate()correctly returnLocalDateTime, and calling.toLocalDate()on them is the proper way to convert toLocalDate.booklore-api/src/main/java/org/booklore/service/migration/migrations/PopulateSearchTextMigration.java (1)
60-60: No changes needed. The project is configured to use Java 25, which fully supportsList.getLast()(available since Java 21).booklore-api/src/main/java/org/booklore/service/migration/migrations/GenerateCoverHashMigration.java (1)
51-51: The concern raised in this review comment is invalid. Thebooklore-apimodule is configured to use Java 25 (as specified inbuild.gradle.ktsline 25:languageVersion = JavaLanguageVersion.of(25)), which fully supportsList.getLast()— available since Java 21. No fallback or code change is required.> Likely an incorrect or invalid review comment.booklore-api/src/main/java/org/booklore/repository/BookOpdsRepository.java (1)
145-149: Good push-down of the random-book limit.Letting the repository page the random-ID query removes the extra truncation step from the service and keeps the limiting where it belongs.
booklore-api/src/main/java/org/booklore/repository/NotebookEntryRepository.java (1)
3-15: Nice simplification around the unified notebook view.Querying
NotebookEntryViewdirectly keeps these methods projection-based and removes a lot of duplicated union/filter SQL from the repository.Also applies to: 44-115
booklore-api/src/main/java/org/booklore/controller/MetadataController.java (1)
53-59: Nice controller thinning.This keeps the endpoint focused on HTTP concerns and moves the metadata-update workflow into a reusable service boundary.
booklore-api/src/test/java/org/booklore/controller/MetadataControllerTest.java (1)
33-47: These tests are now at the right level.After moving the update flow into
BookMetadataService, asserting delegation and parameter forwarding here is exactly the right granularity.Also applies to: 50-61
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
booklore-api/src/main/java/org/booklore/service/opds/OpdsBookService.java (1)
19-23:⚠️ Potential issue | 🟡 MinorDuplicate import:
BookUtilsis imported twice.Lines 19 and 23 both import
org.booklore.util.BookUtils. Remove one of them.🧹 Proposed fix
import org.booklore.util.BookUtils; import org.booklore.repository.BookRepository; import org.booklore.service.library.LibraryService; import org.booklore.service.restriction.ContentRestrictionService; -import org.booklore.util.BookUtils;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@booklore-api/src/main/java/org/booklore/service/opds/OpdsBookService.java` around lines 19 - 23, The file imports org.booklore.util.BookUtils twice; remove the duplicate import so there's only a single import of BookUtils in the OpdsBookService class (look for the import statements near the top of the OpdsBookService file and delete the redundant org.booklore.util.BookUtils line).
♻️ Duplicate comments (1)
booklore-api/src/main/java/org/booklore/repository/BookRepository.java (1)
174-196:⚠️ Potential issue | 🟠 Major
findFirst...()can still return a soft-deleted match.This query includes deleted rows, and the helper immediately takes the first result. With
ORDER BY b.id ASC, an older soft-deleted duplicate can still win over the live book.Suggested ordering change
- ORDER BY b.id ASC + ORDER BY + CASE WHEN b.deleted = true THEN 1 ELSE 0 END, + b.id ASC🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@booklore-api/src/main/java/org/booklore/repository/BookRepository.java` around lines 174 - 196, The query used by findByLibraryIdAndLibraryPathIdAndFileSubPathAndFileName (and therefore the helper findFirstByLibraryIdAndLibraryPathIdAndFileSubPathAndFileName) can return soft-deleted rows; update the JPQL in the `@Query` for findByLibraryIdAndLibraryPathIdAndFileSubPathAndFileName to explicitly exclude soft-deleted entities (e.g. add "AND b.deleted = false" or "AND b.deletedAt IS NULL" depending on your entity field) and adjust the ORDER BY to prefer non-deleted rows first (for example "ORDER BY b.deleted ASC, b.id ASC" or equivalent) so the default method's PageRequest.of(0,1) returns a live book.
🧹 Nitpick comments (1)
booklore-api/src/main/java/org/booklore/model/entity/NotebookEntryView.java (1)
13-15: Rename this JPA entity to follow the*Entitysuffix convention.
NotebookEntryViewis annotated with@Entity, but the class/file naming does not follow the repository’s entity suffix convention. Consider renaming to something likeNotebookEntryViewEntity(and updating repository references) for consistency.Based on learnings: Keep JPA entities on the
*Entitysuffix.Also applies to: 71-71
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@booklore-api/src/main/java/org/booklore/model/entity/NotebookEntryView.java` around lines 13 - 15, Rename the JPA entity class NotebookEntryView to NotebookEntryViewEntity and update the filename accordingly; change the class declaration annotated with `@Entity/`@Immutable/@Subselect from "NotebookEntryView" to "NotebookEntryViewEntity", then update all usages and imports (including any repository interfaces or services that reference NotebookEntryView) to the new class name so compilation and Spring Data JPA wiring remain correct; ensure repository names or types that followed the old name are renamed or updated to reference NotebookEntryViewEntity to keep naming consistent across the codebase.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@booklore-api/src/main/java/org/booklore/service/opds/OpdsBookService.java`:
- Around line 19-23: The file imports org.booklore.util.BookUtils twice; remove
the duplicate import so there's only a single import of BookUtils in the
OpdsBookService class (look for the import statements near the top of the
OpdsBookService file and delete the redundant org.booklore.util.BookUtils line).
---
Duplicate comments:
In `@booklore-api/src/main/java/org/booklore/repository/BookRepository.java`:
- Around line 174-196: The query used by
findByLibraryIdAndLibraryPathIdAndFileSubPathAndFileName (and therefore the
helper findFirstByLibraryIdAndLibraryPathIdAndFileSubPathAndFileName) can return
soft-deleted rows; update the JPQL in the `@Query` for
findByLibraryIdAndLibraryPathIdAndFileSubPathAndFileName to explicitly exclude
soft-deleted entities (e.g. add "AND b.deleted = false" or "AND b.deletedAt IS
NULL" depending on your entity field) and adjust the ORDER BY to prefer
non-deleted rows first (for example "ORDER BY b.deleted ASC, b.id ASC" or
equivalent) so the default method's PageRequest.of(0,1) returns a live book.
---
Nitpick comments:
In `@booklore-api/src/main/java/org/booklore/model/entity/NotebookEntryView.java`:
- Around line 13-15: Rename the JPA entity class NotebookEntryView to
NotebookEntryViewEntity and update the filename accordingly; change the class
declaration annotated with `@Entity/`@Immutable/@Subselect from
"NotebookEntryView" to "NotebookEntryViewEntity", then update all usages and
imports (including any repository interfaces or services that reference
NotebookEntryView) to the new class name so compilation and Spring Data JPA
wiring remain correct; ensure repository names or types that followed the old
name are renamed or updated to reference NotebookEntryViewEntity to keep naming
consistent across the codebase.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f7118090-1d08-4655-b920-3b2e1f351147
📒 Files selected for processing (5)
booklore-api/src/main/java/org/booklore/model/entity/NotebookEntryView.javabooklore-api/src/main/java/org/booklore/repository/BookRepository.javabooklore-api/src/main/java/org/booklore/service/book/BookDownloadService.javabooklore-api/src/main/java/org/booklore/service/metadata/BookMetadataService.javabooklore-api/src/main/java/org/booklore/service/opds/OpdsBookService.java
🚧 Files skipped from review as they are similar to previous changes (2)
- booklore-api/src/main/java/org/booklore/service/book/BookDownloadService.java
- booklore-api/src/main/java/org/booklore/service/metadata/BookMetadataService.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 / Backend Tests
- GitHub Check: Test Suite / Frontend Tests
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (java-kotlin)
- GitHub Check: Frontend Lint Threshold Check
🧰 Additional context used
📓 Path-based instructions (1)
booklore-api/src/**/*.java
📄 CodeRabbit inference engine (AGENTS.md)
booklore-api/src/**/*.java: Use 4-space indentation in Java files and match surrounding Java style
Prefer constructor injection via Lombok patterns already used in the codebase. Do not introduce@Autowiredfield injection
Use MapStruct for entity/DTO mapping
Files:
booklore-api/src/main/java/org/booklore/service/opds/OpdsBookService.javabooklore-api/src/main/java/org/booklore/repository/BookRepository.javabooklore-api/src/main/java/org/booklore/model/entity/NotebookEntryView.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:
booklore-api/src/main/java/org/booklore/service/opds/OpdsBookService.javabooklore-api/src/main/java/org/booklore/repository/BookRepository.java
📚 Learning: 2026-04-01T17:50:06.817Z
Learnt from: pedronave
Repo: grimmory-tools/grimmory PR: 247
File: booklore-api/src/main/java/org/booklore/service/hardcover/HardcoverSyncService.java:156-159
Timestamp: 2026-04-01T17:50:06.817Z
Learning: In grimmory-tools/grimmory (HardcoverSyncService.java), the Hardcover API already returns `user_book_reads` in a consistent, ordered fashion (ascending by recency), so `getLast()` reliably retrieves the most recent read entry. Adding an explicit `order_by` to the `user_book_reads` query is unnecessary.
Applied to files:
booklore-api/src/main/java/org/booklore/service/opds/OpdsBookService.javabooklore-api/src/main/java/org/booklore/repository/BookRepository.java
📚 Learning: 2026-03-24T18:58:08.199Z
Learnt from: pedronave
Repo: grimmory-tools/grimmory PR: 173
File: booklore-api/src/main/java/org/booklore/service/hardcover/HardcoverSyncService.java:291-294
Timestamp: 2026-03-24T18:58:08.199Z
Learning: In grimmory-tools/grimmory (HardcoverSyncService.java), when a Hardcover edition has no page count, falling back to the book-level page count (book.pages) is the preferred behavior. Skipping the sync entirely in this case is considered worse than using a slightly approximate total, because the page count is only a proxy for converting progress % to page numbers for the API. The Hardcover API allows setting page-based progress even when the edition lacks a page count, and progress displays in at least some UI views (e.g., the book detail dropdown).
Applied to files:
booklore-api/src/main/java/org/booklore/service/opds/OpdsBookService.java
📚 Learning: 2026-04-04T15:36:55.394Z
Learnt from: balazs-szucs
Repo: grimmory-tools/grimmory PR: 372
File: booklore-api/src/main/java/org/booklore/app/service/AppBookService.java:357-359
Timestamp: 2026-04-04T15:36:55.394Z
Learning: In `booklore-api/src/main/java/org/booklore/app/service/AppBookService.java`, `shelfId` and `magicShelfId` are mutually exclusive navigation contexts in `getFilterOptions`. A request will never supply both at the same time, so the `else if (shelfId != null)` branch after the `magicBookIds != null` check is intentional and correct — there is no missing shelf-∩-magicShelf intersection case. Do not flag this pattern as a bug.
Applied to files:
booklore-api/src/main/java/org/booklore/repository/BookRepository.java
📚 Learning: 2026-03-26T01:46:48.863Z
Learnt from: CR
Repo: grimmory-tools/grimmory PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-26T01:46:48.863Z
Learning: Applies to booklore-api/src/**/*Entity.java : Keep JPA entities on the `*Entity` suffix
Applied to files:
booklore-api/src/main/java/org/booklore/model/entity/NotebookEntryView.java
🔇 Additional comments (5)
booklore-api/src/main/java/org/booklore/service/opds/OpdsBookService.java (2)
164-166: LGTM!Good defensive guard clause that prevents unnecessary database calls for invalid count values.
174-184: LGTM!Clean refactoring that moves the limiting logic to the database layer via
Pageable. This is more efficient than fetching all random IDs and truncating in Java, as it reduces both database transfer and memory overhead.booklore-api/src/main/java/org/booklore/model/entity/NotebookEntryView.java (1)
15-70: Solid read-only view modeling for repository projections.Using
@Immutable+@Subselect+@Synchronizehere is a good fit for consolidating notebook entry sources into one queryable model, and it aligns cleanly with the JPQL property usage inNotebookEntryRepository.booklore-api/src/main/java/org/booklore/repository/BookRepository.java (2)
37-39: Targeted fetch plan for the Kobo download path.This loads the associations
BookDownloadService.downloadKoboBook(...)actually touches, so it should close the lazy-load gap from issue#401without widening a heavier fetch graph for unrelated callers.
137-138: Cursor-based batching fits the migration loop well.
b.id > :afterId ORDER BY b.idlines up cleanly with thelastIdpagination pattern in both migration classes and avoids offset-based rescans.
…opService and related tests
# Conflicts: # booklore-api/src/main/java/org/booklore/service/bookdrop/BookDropService.java # booklore-api/src/test/java/org/booklore/service/bookdrop/BookDropServiceTest.java
# Conflicts: # booklore-api/src/main/java/org/booklore/model/entity/LibraryEntity.java # booklore-api/src/main/java/org/booklore/repository/BookRepository.java
zachyale
left a comment
zachyale
left a comment
There was a problem hiding this comment.
Some notes re: issues on Sunday/Monday indexing, and an existing (but still wrong) implementation of timezone awareness that is DST agnostic.
Depending on how you proceed you may need to address the favourite days chart/session archetype charts w/ these indexing related changes.
Thanks for the review. This stuff was interesting, bit painful, but i think i got it: For context: 670644a
the obvious trade-off/ still problematic parts are the following:
i am hoping these 2 are not that bad. By "not that bad", that this is still self-hosted app, not some big corpo site, so probably users do not have that "many" readingSession to cause actual problems, well at least i hope so. But this has already gotten too big. My apologies about that. If you have questions still i am open. |
2 participants
Description
Linked Issue: Fixes #401
Changes
This pull request introduces several improvements and refactorings to the backend data access and service layers, focusing on notebook entry management, reading session analytics, and repository query optimizations. The changes include the addition of a new unified JPA view for notebook entries, migration of several complex native queries to JPQL using this view, and enhancements to repository methods for better paging and maintainability. Additionally, the reading session service logic is refactored for improved time zone handling and period calculations.
Notebook Entries Refactor and Repository Improvements:
NotebookEntryViewthat unifies highlights, notes, and bookmarks into a single immutable view, simplifying queries and data access for notebook entries.NotebookEntryRepositoryto useNotebookEntryViewand migrated all major native SQL queries to JPQL, improving maintainability and leveraging JPA features. This includes changes to the main entry, book list, and book-with-count queries.Repository Query and Paging Enhancements:
Pageablefor paging instead of manualLIMIT/OFFSETin native queries, and refactored methods to use JPQL instead of native SQL. This affects bothBookRepositoryandBookOpdsRepository, improving consistency and Spring Data compatibility.Reading Session Analytics Refactor:
ReadingSessionServiceto improve time zone handling (now using offset in seconds) and introduced helper methods for calculating period bounds (year/month) for analytics queries. This makes the code more robust and easier to extendDTO and Projection Updates:
LocalDateinstead of day-of-week integers, providing more precise and flexible date handling.Summary by CodeRabbit
Performance Improvements
New Features
Backend Improvements
Tests