refactor(stats): push bulk queries to streams, deduplicate aggregation logic#937
refactor(stats): push bulk queries to streams, deduplicate aggregation logic#937balazs-szucs wants to merge 9 commits intogrimmory-tools:developfrom
Conversation
… deduplicate aggregation logic
|
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:
WalkthroughAdds a listening favorite-days analytics endpoint, converts multiple repository queries to streaming with fetch-size hints, adds session-timestamp projection queries, and refactors service logic to consume streams safely while centralizing peak-hour and favorite-day aggregations. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Controller as UserStatsController
participant Service as ReadingSessionService
participant Repo as ReadingSessionRepository
participant DB as Database
Client->>Controller: GET /api/v1/user-stats/listening/favorite-days?year=&month=
Controller->>Controller: authorize stats-access
Controller->>Service: getListeningFavoriteDays(year, month)
Service->>Repo: findListeningSessionTimestampsByUser(userId, periodStart?, periodEnd?)
Repo->>DB: execute query (stream, fetchSize=200)
DB-->>Repo: stream rows (SessionTimestampDto)
Repo-->>Service: Stream<SessionTimestampDto>
Service->>Service: computeFavoriteDays(stream) within try-with-resources
Service-->>Controller: List<FavoriteReadingDaysResponse>
Controller-->>Client: 200 OK with payload
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 6/8 reviews remaining, refill in 11 minutes and 39 seconds.Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/src/main/java/org/booklore/service/ReadingSessionService.java (1)
539-556:⚠️ Potential issue | 🟡 MinorReading scatter page size left at 500 — intentional?
The PR description states "Increase scatter plot page size from 500 to 1000," but only
getListeningSessionScatter(line 869) was bumped whilegetSessionScatterhere still usesPageRequest.of(0, 500). If the intent was to keep reading at 500, fine; otherwise update both for consistency.Proposed change if both should match
- return readingSessionRepository.findSessionTimestampsByUserInPeriod(userId, bounds.start(), bounds.end(), - PageRequest.of(0, 500)) + return readingSessionRepository.findSessionTimestampsByUserInPeriod(userId, bounds.start(), bounds.end(), + PageRequest.of(0, 1000))🤖 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/ReadingSessionService.java` around lines 539 - 556, The getSessionScatter method still uses PageRequest.of(0, 500) while getListeningSessionScatter was bumped to 1000; update getSessionScatter to match the intended page size by changing the PageRequest in ReadingSessionService#getSessionScatter (replace PageRequest.of(0, 500) with PageRequest.of(0, 1000)) so both scatter queries use the same limit, or explicitly document why reading remains 500 if that was intentional.
🧹 Nitpick comments (1)
backend/src/main/java/org/booklore/repository/ReadingSessionRepository.java (1)
28-31: Add fetch-size hints to Stream-returning query methods.Spring Data JPA
Stream<T>query methods only stream rows efficiently when the underlying JDBC driver is configured with appropriate fetch-size hints. MariaDB Connector/J (the configured driver) loads full result sets into memory by default, defeating the memory-efficiency goal of Stream queries. Add@QueryHintsto all Stream-returning methods:import org.springframework.data.jpa.repository.QueryHints; import jakarta.persistence.QueryHint; `@QueryHints`(`@QueryHint`(name = "org.hibernate.fetchSize", value = "200")) `@Query`("""...""") Stream<Instant> findSessionStartTimesByUserAndPeriod(...);Apply to:
findSessionStartTimesByUserAndPeriod(lines 28–31),findPageTurnerSessionsByUser(line 154),findCompletionRaceSessionsByUserAndYear(lines 171–173), andfindAllSessionStartTimesByUserStream(line 219).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/main/java/org/booklore/repository/ReadingSessionRepository.java` around lines 28 - 31, The Stream-returning repository methods (findSessionStartTimesByUserAndPeriod, findPageTurnerSessionsByUser, findCompletionRaceSessionsByUserAndYear, findAllSessionStartTimesByUserStream) lack JDBC fetch-size hints so MariaDB loads full result sets into memory; add `@QueryHints`(`@QueryHint`(name = "org.hibernate.fetchSize", value = "200")) above each of those method declarations (alongside their existing `@Query` if present) and add the imports for org.springframework.data.jpa.repository.QueryHints and jakarta.persistence.QueryHint to enable efficient streaming.
🤖 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/repository/ReadingSessionRepository.java`:
- Around line 175-181: Remove the unused method findDistinctReadingDatesByUser
from ReadingSessionRepository because it is not referenced and does DB-side
Instant→LocalDate conversion that ignores the user's zone; instead rely on
findAllSessionStartTimesByUserStream and convert each Instant with
atZone(zone).toLocalDate(), then deduplicate into a Set (as done in
getReadingStreak) when a caller needs distinct local dates. Ensure the method
signature and its `@Query` are deleted so no timezone-misleading DB conversion
remains.
---
Outside diff comments:
In `@backend/src/main/java/org/booklore/service/ReadingSessionService.java`:
- Around line 539-556: The getSessionScatter method still uses PageRequest.of(0,
500) while getListeningSessionScatter was bumped to 1000; update
getSessionScatter to match the intended page size by changing the PageRequest in
ReadingSessionService#getSessionScatter (replace PageRequest.of(0, 500) with
PageRequest.of(0, 1000)) so both scatter queries use the same limit, or
explicitly document why reading remains 500 if that was intentional.
---
Nitpick comments:
In `@backend/src/main/java/org/booklore/repository/ReadingSessionRepository.java`:
- Around line 28-31: The Stream-returning repository methods
(findSessionStartTimesByUserAndPeriod, findPageTurnerSessionsByUser,
findCompletionRaceSessionsByUserAndYear, findAllSessionStartTimesByUserStream)
lack JDBC fetch-size hints so MariaDB loads full result sets into memory; add
`@QueryHints`(`@QueryHint`(name = "org.hibernate.fetchSize", value = "200")) above
each of those method declarations (alongside their existing `@Query` if present)
and add the imports for org.springframework.data.jpa.repository.QueryHints and
jakarta.persistence.QueryHint to enable efficient streaming.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: de9f6046-bd40-47c9-864e-239630bfe274
📒 Files selected for processing (3)
backend/src/main/java/org/booklore/controller/UserStatsController.javabackend/src/main/java/org/booklore/repository/ReadingSessionRepository.javabackend/src/main/java/org/booklore/service/ReadingSessionService.java
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Test Suite / Frontend Tests
- GitHub Check: Test Suite / Backend Tests
- GitHub Check: Analyze (java-kotlin)
🧰 Additional context used
📓 Path-based instructions (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/controller/UserStatsController.javabackend/src/main/java/org/booklore/repository/ReadingSessionRepository.javabackend/src/main/java/org/booklore/service/ReadingSessionService.java
🧠 Learnings (3)
📚 Learning: 2026-04-10T08:15:37.436Z
Learnt from: imnotjames
Repo: grimmory-tools/grimmory PR: 449
File: booklore-api/src/main/java/org/booklore/service/book/BookDownloadService.java:139-145
Timestamp: 2026-04-10T08:15:37.436Z
Learning: When using Spring `ContentDisposition.builder(...).filename(name, StandardCharsets.UTF_8).build()` (i.e., explicitly providing UTF-8), the resulting header value should include both the quoted `filename="=?UTF-8?..."` and the RFC 5987 `filename*=` parameters. In this case, any extra ASCII fallback computation (e.g., deriving an ASCII `fallbackFilename` via `NON_ASCII_PATTERN` and calling `.filename(fallbackFilename)`) is likely redundant—prefer calling only `.filename(fallbackName?, StandardCharsets.UTF_8)` as appropriate and let Spring handle the UTF-8 header parameters. Verify by comparing the emitted header for `filename` and `filename*` before deciding to keep an ASCII fallback.
Applied to files:
backend/src/main/java/org/booklore/controller/UserStatsController.javabackend/src/main/java/org/booklore/repository/ReadingSessionRepository.javabackend/src/main/java/org/booklore/service/ReadingSessionService.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/controller/UserStatsController.javabackend/src/main/java/org/booklore/repository/ReadingSessionRepository.javabackend/src/main/java/org/booklore/service/ReadingSessionService.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/ReadingSessionService.java
🔇 Additional comments (4)
backend/src/main/java/org/booklore/controller/UserStatsController.java (1)
299-311: LGTM — new endpoint mirrors existing listening stats patterns.The mapping, security guard, OpenAPI metadata, and delegation to
readingSessionService.getListeningFavoriteDays(year, month)are consistent withgetListeningPeakHoursdirectly above.backend/src/main/java/org/booklore/service/ReadingSessionService.java (2)
90-122: Helper extraction + DB→Java timezone fix looks correct.Both helpers consistently use
s.getStartTime().atZone(zone)for hour/day-of-week extraction, which addresses the non-UTC user bug as described in the PR.EnumMap<DayOfWeek, ...>is appropriate, and the final sort bygetDayOfWeek()(Sunday-first int) yields a stable Sun→Sat order regardless ofEnumMap's natural Mon-first iteration.
343-347: try-with-resources around Stream-returning repo calls — good.All stream-returning calls (
findPageTurnerSessionsByUser,findCompletionRaceSessionsByUserAndYear,findAllSessionStartTimesByUserStream) are now properly closed, and terminal collection happens inside the try block before close. This correctly releases JDBC cursors/Hibernate ScrollableResults.Also applies to: 427-430, 461-463, 562-567
backend/src/main/java/org/booklore/repository/ReadingSessionRepository.java (1)
183-211: Filtered timestamp queries — LGTM.
findReadingSessionTimestampsByUser(non-audiobook) andfindListeningSessionTimestampsByUser(audiobook) cleanly replace the type-agnosticfindSessionTimestampsByUserfor stat aggregation, the optional:periodStart/:periodEndsemantics matchcomputeOptionalBoundsin the service, andcoalesce(rs.durationSeconds, 0)prevents NPEs when the service auto-unboxesInteger getDurationSeconds().
…ngSessionRepository
|
|
||
| @Query(""" | ||
| SELECT rs.startTime as startTime, | ||
| coalesce(rs.durationSeconds, 0) as durationSeconds |
There was a problem hiding this comment.
Isn't durationSeconds non-null? If so, is this still needed?
…ReadingSessionService
balazs-szucs
changed the title
# Conflicts: # backend/src/main/java/org/booklore/service/ReadingSessionService.java
…ven month and year
There was a problem hiding this comment.
♻️ Duplicate comments (1)
backend/src/main/java/org/booklore/service/ReadingSessionService.java (1)
866-866:⚠️ Potential issue | 🟡 MinorPlease avoid a one-off cap bump here.
This now returns up to 1000 listening points while reading scatter is still capped at 500, so the two endpoints diverge and large histories are still silently truncated either way. Either keep the cap consistent or move it behind a shared constant/config.
🤖 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/ReadingSessionService.java` at line 866, The code in ReadingSessionService calling readingSessionRepository.findListeningTimestampsByUserPaged(userId, PageRequest.of(0, 1000)) introduces a one-off cap (1000) that diverges from the 500 cap used elsewhere; replace the magic number with a shared constant or config value (e.g., LISTENING_POINTS_PAGE_SIZE) and use that in PageRequest.of(0, LISTENING_POINTS_PAGE_SIZE) so both listening and reading scatter endpoints use the same limit; if a shared constant already exists, reference that (or move the existing 500 into a common constant) and update this call to use it.
🧹 Nitpick comments (1)
backend/src/main/java/org/booklore/service/ReadingSessionService.java (1)
87-119: These helpers still force full materialization for unbounded stats calls.
getPeakReadingHours(...),getFavoriteReadingDays(...), and the listening equivalents now funnel through helpers that require aList<SessionTimestampDto>. Whenyear/monthare omitted, those endpoints still load the user's full history into memory before aggregating. If you want the streaming refactor end-to-end, make these helpers consume aStream<SessionTimestampDto>too, or push the hour/day aggregation into the repository.Also applies to: 218-219, 227-228, 814-815, 823-824
🤖 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/ReadingSessionService.java` around lines 87 - 119, computePeakHours and computeFavoriteDays currently accept List<SessionTimestampDto> which forces full materialization; change them to accept Stream<SessionTimestampDto> (or provide an overload) and perform aggregation using stream operations (e.g., collect with groupingBy/Collectors.toMap or a custom Collector) so you never materialize the entire list in memory; update callers getPeakReadingHours, getFavoriteReadingDays (and their listening variants) to pass the repository stream (Stream<SessionTimestampDto>) instead of a List, preserve existing keys/values (hour via s.getStartTime().atZone(zone).getHour(), DayOfWeek via .getDayOfWeek()) and keep the same response construction (PeakHoursResponse, FavoriteReadingDaysResponse) and sorting behavior (for days) so semantics remain unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@backend/src/main/java/org/booklore/service/ReadingSessionService.java`:
- Line 866: The code in ReadingSessionService calling
readingSessionRepository.findListeningTimestampsByUserPaged(userId,
PageRequest.of(0, 1000)) introduces a one-off cap (1000) that diverges from the
500 cap used elsewhere; replace the magic number with a shared constant or
config value (e.g., LISTENING_POINTS_PAGE_SIZE) and use that in
PageRequest.of(0, LISTENING_POINTS_PAGE_SIZE) so both listening and reading
scatter endpoints use the same limit; if a shared constant already exists,
reference that (or move the existing 500 into a common constant) and update this
call to use it.
---
Nitpick comments:
In `@backend/src/main/java/org/booklore/service/ReadingSessionService.java`:
- Around line 87-119: computePeakHours and computeFavoriteDays currently accept
List<SessionTimestampDto> which forces full materialization; change them to
accept Stream<SessionTimestampDto> (or provide an overload) and perform
aggregation using stream operations (e.g., collect with
groupingBy/Collectors.toMap or a custom Collector) so you never materialize the
entire list in memory; update callers getPeakReadingHours,
getFavoriteReadingDays (and their listening variants) to pass the repository
stream (Stream<SessionTimestampDto>) instead of a List, preserve existing
keys/values (hour via s.getStartTime().atZone(zone).getHour(), DayOfWeek via
.getDayOfWeek()) and keep the same response construction (PeakHoursResponse,
FavoriteReadingDaysResponse) and sorting behavior (for days) so semantics remain
unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: b04d2382-9947-4ff1-aeb1-9d3ef31ad2f6
📒 Files selected for processing (1)
backend/src/main/java/org/booklore/service/ReadingSessionService.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). (1)
- GitHub Check: Test Suite / Backend Tests
🧰 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/ReadingSessionService.java
🧠 Learnings (3)
📚 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/ReadingSessionService.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/ReadingSessionService.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/ReadingSessionService.java
🔇 Additional comments (1)
backend/src/main/java/org/booklore/service/ReadingSessionService.java (1)
158-160: Nice cleanup of stream lifecycle handling.Wrapping the repository streams in try-with-resources here is the right fix for deterministic cursor/connection cleanup.
Also applies to: 168-170, 340-344, 424-427, 458-460
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/src/main/java/org/booklore/repository/ReadingSessionRepository.java`:
- Around line 23-34: The query findSessionStartTimesByUserAndPeriod currently
returns all sessions including audiobooks; add a book-type filter and expose a
reading-only variant so reading analytics stay consistent: add a new repository
method like findSessionStartTimesByUserAndPeriodAndBookType (or
findReadingSessionStartTimesByUserAndPeriod) that accepts a BookFileType or a
boolean readingOnly and applies WHERE rs.book.fileType = :bookType (or WHERE
rs.book.fileType <> :excludedType), keeping the Stream<Instant> return and
`@QueryHints`; then update ReadingSessionService methods getSessionHeatmapForYear,
getSessionHeatmapForMonth, getReadingDates, and getReadingStreak to call the new
reading-only repository method (leave getPeakReadingHours and
getFavoriteReadingDays unchanged if they intentionally exclude audiobooks), and
replicate the same change for the other similar query block mentioned (lines
210-217) so both query variants are available and callers use the appropriate
filtered version.
In `@backend/src/main/java/org/booklore/service/ReadingSessionService.java`:
- Around line 340-344: The current use of Collectors.groupingBy and toList on
the stream from readingSessionRepository.findPageTurnerSessionsByUser(userId)
materializes all sessions into memory; instead, change getPageTurnerScores() to
consume the ordered stream incrementally by buffering sessions for the current
bookId and emitting/computing scores when bookId changes (process one book at a
time since query is ordered by bookId), and change getCompletionRace() to
maintain a bounded buffer keyed by bookId (capacity COMPLETION_RACE_BOOK_LIMIT)
or a deque of the last N book results while streaming so you never collect all
sessions; replace the groupingBy/toList and sessionStream.toList() usages
(including the similar code around getCompletionRace and lines 424-427) with
single-pass streaming logic over the stream returned by
findPageTurnerSessionsByUser to keep memory O(book) or
O(COMPLETION_RACE_BOOK_LIMIT) instead of O(total sessions).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: def7dcfa-4634-47c5-9f30-b7611db3c37a
📒 Files selected for processing (2)
backend/src/main/java/org/booklore/repository/ReadingSessionRepository.javabackend/src/main/java/org/booklore/service/ReadingSessionService.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: Analyze (java-kotlin)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Test Suite / Backend Tests
- GitHub Check: Test Suite / Frontend Tests
- GitHub Check: Frontend Lint Threshold Check
🧰 Additional context used
📓 Path-based instructions (4)
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/repository/ReadingSessionRepository.javabackend/src/main/java/org/booklore/service/ReadingSessionService.java
**/*
⚙️ CodeRabbit configuration file
**/*: This project is being developed using current and future-facing technologies:
- Java 25 with --enable-preview (preview features are INTENTIONAL and encouraged)
- Spring Boot 4 (latest major version, check APIs accordingly)
- Jackson 3 (new package: tools.jackson.* instead of com.fasterxml.jackson.*)
- Hibernate 7.3.x (Jakarta Persistence 3.2, new APIs; avoid deprecated Hibernate 5/6 patterns)
- Angular 21 (signals-based reactivity, no NgModules unless legacy)
Grimmory Internal Tools
- epub4j and pdfium4j are our own internal tools developed by the Grimmory team.
- Always verify behavior and API changes against the upstream repositories:
- If you encounter issues with these libraries, check if a fix exists in the upstream grimmory-tools organization.
Metadata Standards and Compliance
- For all metadata writing and parsing logic, double-check against Dublin Core and ANSI standards to ensure perfect official compliance.
- We strictly follow the widespread and official XML-compliant methods for EPUB2, EPUB3, CBX, and PDF formats.
General Java and Spring rules
- ALWAYS prefer modern, idiomatic Java 25 constructs over legacy patterns.
- Preview features (--enable-preview) are enabled and intentional; do NOT flag them as risky unless there is a concrete runtime issue.
- Prefer: records, sealed classes/interfaces, pattern matching (switch expressions, instanceof), structured concurrency (StructuredTaskScope), scoped values, string templates, unnamed patterns/variables.
- Prefer virtual threads (Thread.ofVirtual(), Executors.newVirtualThreadPerTaskExecutor()) over platform threads for I/O-bound work.
- Prefer the new Sequenced Collections API (SequencedCollection, SequencedMap) where applicable.
- Prefer
varfor local variables when the type is obvious from context.- Use stream().toList() instead of stream().collect(Collectors.toList()) for imm...
Files:
backend/src/main/java/org/booklore/repository/ReadingSessionRepository.javabackend/src/main/java/org/booklore/service/ReadingSessionService.java
**/repository/**/*.java
⚙️ CodeRabbit configuration file
**/repository/**/*.java: Spring Data / Hibernate repository review:
- Prefer Spring Data JPA repository interfaces with typed methods.
- Flag N+1 risks: use
@EntityGraphor JOIN FETCH in JPQL for collection associations.- Flag missing
@Transactional(readOnly = true) on query-only methods.- Flag any native query with string concatenation; use named parameters only.
- Prefer
@Querywith JPQL over native SQL unless performance-critical.
Files:
backend/src/main/java/org/booklore/repository/ReadingSessionRepository.java
**/service/**/*.java
⚙️ CodeRabbit configuration file
**/service/**/*.java: Spring Framework 7 service layer review:
- Flag missing
@Transactionalon methods that perform multiple writes.- Prefer constructor injection over
@Autowiredfield injection. Use@AllArgsConstructor.- Use ApiError enum for throwing exceptions.
- Flag checked exceptions swallowed silently; must log or rethrow.
- Flag Thread.sleep(); prefer Duration-based overloads or ScheduledExecutorService.
- Prefer virtual threads (Thread.ofVirtual()) for I/O-bound operations.
- Flag mutable shared state in singleton beans.
Files:
backend/src/main/java/org/booklore/service/ReadingSessionService.java
🧠 Learnings (4)
📓 Common learnings
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.
📚 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/repository/ReadingSessionRepository.javabackend/src/main/java/org/booklore/service/ReadingSessionService.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/repository/ReadingSessionRepository.javabackend/src/main/java/org/booklore/service/ReadingSessionService.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/repository/ReadingSessionRepository.javabackend/src/main/java/org/booklore/service/ReadingSessionService.java
…rner score calculation
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/src/main/java/org/booklore/repository/ReadingSessionRepository.java`:
- Line 16: Remove the unused import of java.time.LocalDate from the
ReadingSessionRepository file: since findDistinctReadingDatesByUser was removed,
delete the import line "import java.time.LocalDate;" from the top of the
ReadingSessionRepository class to eliminate the unused-import warning.
- Around line 210-223: There are two duplicate repository methods doing the same
JPQL work; remove the redundant method findListeningSessionTimestampsByUser from
ReadingSessionRepository and keep the concise findListeningTimestampsByUser
signature/Query; then update all call sites (the five service usages currently
calling findListeningSessionTimestampsByUser) to call
findListeningTimestampsByUser instead so everything compiles against the single
method (search for symbol findListeningSessionTimestampsByUser and replace
usages with findListeningTimestampsByUser).
In `@backend/src/main/java/org/booklore/service/ReadingSessionService.java`:
- Line 891: The call to
readingSessionRepository.findListeningTimestampsByUserPaged currently uses
PageRequest.of(0, 1000) which was accidental and inconsistent with the reading
scatter endpoint; change the PageRequest to use a page size of 500 (e.g.,
PageRequest.of(0, 500)) or otherwise make the page-size constant shared between
the reading scatter logic and the findListeningTimestampsByUserPaged call so
both use the same 500 value; update references to PageRequest usage around
readListeningTimestamps or reading scatter to ensure consistency.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: 65f655cc-7c33-4d18-8077-00a819164f10
📒 Files selected for processing (2)
backend/src/main/java/org/booklore/repository/ReadingSessionRepository.javabackend/src/main/java/org/booklore/service/ReadingSessionService.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 (4)
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/ReadingSessionService.javabackend/src/main/java/org/booklore/repository/ReadingSessionRepository.java
**/*
⚙️ CodeRabbit configuration file
**/*: This project is being developed using current and future-facing technologies:
- Java 25 with --enable-preview (preview features are INTENTIONAL and encouraged)
- Spring Boot 4 (latest major version, check APIs accordingly)
- Jackson 3 (new package: tools.jackson.* instead of com.fasterxml.jackson.*)
- Hibernate 7.3.x (Jakarta Persistence 3.2, new APIs; avoid deprecated Hibernate 5/6 patterns)
- Angular 21 (signals-based reactivity, no NgModules unless legacy)
Grimmory Internal Tools
- epub4j and pdfium4j are our own internal tools developed by the Grimmory team.
- Always verify behavior and API changes against the upstream repositories:
- If you encounter issues with these libraries, check if a fix exists in the upstream grimmory-tools organization.
Metadata Standards and Compliance
- For all metadata writing and parsing logic, double-check against Dublin Core and ANSI standards to ensure perfect official compliance.
- We strictly follow the widespread and official XML-compliant methods for EPUB2, EPUB3, CBX, and PDF formats.
General Java and Spring rules
- ALWAYS prefer modern, idiomatic Java 25 constructs over legacy patterns.
- Preview features (--enable-preview) are enabled and intentional; do NOT flag them as risky unless there is a concrete runtime issue.
- Prefer: records, sealed classes/interfaces, pattern matching (switch expressions, instanceof), structured concurrency (StructuredTaskScope), scoped values, string templates, unnamed patterns/variables.
- Prefer virtual threads (Thread.ofVirtual(), Executors.newVirtualThreadPerTaskExecutor()) over platform threads for I/O-bound work.
- Prefer the new Sequenced Collections API (SequencedCollection, SequencedMap) where applicable.
- Prefer
varfor local variables when the type is obvious from context.- Use stream().toList() instead of stream().collect(Collectors.toList()) for imm...
Files:
backend/src/main/java/org/booklore/service/ReadingSessionService.javabackend/src/main/java/org/booklore/repository/ReadingSessionRepository.java
**/service/**/*.java
⚙️ CodeRabbit configuration file
**/service/**/*.java: Spring Framework 7 service layer review:
- Flag missing
@Transactionalon methods that perform multiple writes.- Prefer constructor injection over
@Autowiredfield injection. Use@AllArgsConstructor.- Use ApiError enum for throwing exceptions.
- Flag checked exceptions swallowed silently; must log or rethrow.
- Flag Thread.sleep(); prefer Duration-based overloads or ScheduledExecutorService.
- Prefer virtual threads (Thread.ofVirtual()) for I/O-bound operations.
- Flag mutable shared state in singleton beans.
Files:
backend/src/main/java/org/booklore/service/ReadingSessionService.java
**/repository/**/*.java
⚙️ CodeRabbit configuration file
**/repository/**/*.java: Spring Data / Hibernate repository review:
- Prefer Spring Data JPA repository interfaces with typed methods.
- Flag N+1 risks: use
@EntityGraphor JOIN FETCH in JPQL for collection associations.- Flag missing
@Transactional(readOnly = true) on query-only methods.- Flag any native query with string concatenation; use named parameters only.
- Prefer
@Querywith JPQL over native SQL unless performance-critical.
Files:
backend/src/main/java/org/booklore/repository/ReadingSessionRepository.java
🧠 Learnings (5)
📓 Common learnings
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.
📚 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/ReadingSessionService.javabackend/src/main/java/org/booklore/repository/ReadingSessionRepository.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/ReadingSessionService.javabackend/src/main/java/org/booklore/repository/ReadingSessionRepository.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/ReadingSessionService.javabackend/src/main/java/org/booklore/repository/ReadingSessionRepository.java
📚 Learning: 2026-04-14T22:11:02.436Z
Learnt from: balazs-szucs
Repo: grimmory-tools/grimmory PR: 513
File: booklore-api/src/main/java/org/booklore/service/reader/AudiobookReaderService.java:38-39
Timestamp: 2026-04-14T22:11:02.436Z
Learning: In `grimmory-tools/grimmory`, for the `AudiobookReaderService` pattern of wrapping a DB lookup + file I/O under a single `Transactional` boundary, the preferred refactoring approach is **Option B**: extract the `BookFileEntity` lookup into a dedicated `Service` bean (e.g., `AudiobookEntityLoader`) annotated with `Transactional(readOnly = true)`, so that the proxy always narrows the transaction to the DB access only and all filesystem I/O runs outside the persistence context. Do NOT use `TransactionTemplate` (Option A) for this pattern in this codebase.
Applied to files:
backend/src/main/java/org/booklore/repository/ReadingSessionRepository.java
🔀 Multi-repo context grimmory-tools/grimmory-docs
[::grimmory-tools/grimmory-docs::]
- docs/reading-stats.md: Mentions "Favorite Reading Days: Distribution of reading sessions by day of week." — UX/docs reference for the new favorite-days stats endpoint.
- docs/readers/audiobook-player.md: Notes that "Listening time in reading sessions is adjusted to account for the playback rate." — indicates docs acknowledge listening sessions distinctly from reading, relevant to splitting reading vs listening queries.
No references found to specific backend API endpoints (e.g., /api/v1/user-stats/listening/favorite-days) or to repository/service method names changed in the PR.
🔇 Additional comments (14)
backend/src/main/java/org/booklore/service/ReadingSessionService.java (9)
74-85: LGTM!The change to accept
Stream<Instant>directly aligns with the streaming refactor. The method correctly processes the stream and produces sorted heatmap responses.
87-119: LGTM!The new helper methods
computePeakHoursandcomputeFavoriteDayscentralize aggregation logic and are correctly implemented. UsingTreeMapfor hourly stats andEnumMapfor daily stats is appropriate. The repository queries guaranteedurationSecondsis non-null viacoalesce(rs.durationSeconds, 0).
153-170: LGTM!Proper use of try-with-resources to ensure the database cursor is released after processing the stream.
213-229: LGTM!The reading peak hours and favorite days methods correctly delegate to the shared helper methods using the filtered repository queries.
340-360: LGTM - Incremental processing addresses prior review concern.The refactor to process sessions incrementally by book ID avoids materializing all sessions into memory. The try-with-resources ensures proper stream cleanup.
381-433: LGTM!The helper method correctly processes per-book sessions, calculates grip score metrics, and builds the response. Using
getFirst()from SequencedCollection API is appropriate for Java 25.
441-466: LGTM - Bounded buffer addresses prior memory concern.The bounded
Dequeapproach keeps only the lastCOMPLETION_RACE_BOOK_LIMITbooks in memory. Note: line 451 copies the list (new ArrayList<>(currentBookSessions)) before clearing, while line 461 adds the reference directly—this is correct sincecurrentBookSessionsisn't cleared after the final book.
479-486: LGTM!Both
getReadingDatesandgetReadingStreakcorrectly use try-with-resources to manage the stream lifecycle.Also applies to: 584-589
843-850: LGTM!The new
getListeningFavoriteDaysmethod mirrors the reading favorite-days behavior and correctly delegates to the sharedcomputeFavoriteDayshelper.backend/src/main/java/org/booklore/repository/ReadingSessionRepository.java (5)
23-49: LGTM!The start-time stream queries are now properly split by book type, addressing the prior review concern about reading analytics consistency. The
@QueryHintswithfetchSize=200is appropriate for streaming.
154-173: LGTM!Streaming with
@QueryHintsfor fetch size is appropriate for the page turner query which may return many sessions.
175-193: LGTM!Streaming with fetch size hint is appropriate for the completion race query.
195-223: LGTM - Book-type-filtered timestamp queries.The new
findReadingSessionTimestampsByUserandfindListeningSessionTimestampsByUsermethods correctly filter by book type and returnList<SessionTimestampDto>for the peak hours/favorite days aggregation use case.
225-243: LGTM!The start-time queries are properly split by book type with streaming return and fetch size hints.
2 participants
Description
Linked Issue: Fixes #
Changes
Refactors reading/listening session stat queries and their service-layer consumers to improve memory efficiency and correctness, and adds a missing
GET /listening/favorite-daysendpoint to bring audiobook stats to parity with reading stats.findPageTurnerSessionsByUser,findCompletionRaceSessionsByUserAndYear,findSessionStartTimesByUserAndPeriod, andfindAllSessionStartTimesByUserfromListtoStreamto avoid full result-set materializationfindDistinctReadingDatesByUserto offload date deduplication to the DBfindReadingSessionTimestampsByUserandfindListeningSessionTimestampsByUseras explicit, book-type-filtered replacements for the sharedfindSessionTimestampsByUserService
Stream-returning repository calls intry-with-resourcesto ensure cursor/connection releasegroupByLocalDateto acceptStream<Instant>directly, removing an unnecessary intermediatecollect(toList())getListeningFavoriteDaysservice methodSummary by CodeRabbit
New Features
Refactor