refactor(metadata): improve hardcover edition selection#1800
Open
sabrina553 wants to merge 89 commits into
Open
refactor(metadata): improve hardcover edition selection#1800sabrina553 wants to merge 89 commits into
sabrina553 wants to merge 89 commits into
Conversation
Introduces `HardcoverWorkResponse` and `HardcoverWorkDetails` classes to handle metadata parsing for hardcover editions.
Implements logic to retrieve ISBN-10 and ISBN-13 using `HardcoverWorkDetails` and update results dynamically during metadata parsing.
…itions by title and author Implements GraphQL query to retrieve ISBN-10 and ISBN-13 for hardcover editions and returns the top result based on score.
…n specific edition
…for ebook or audiobook metadata.
Determine if result is the same as the search term, or if it only contains it, and if so does the author match then this is the Highest quality result, and we can skip further parsing. Once best Result is select, determine the language to filter
…First (main) Author from that result, and the language, and reading format as filter. If returns a valid result update the result object with the correct isbn details. Otherwise return oldResults (no processing applied as a fallback)
…r, language, and reading format. returns isbn
…th, 4 - ebook) in the GraphQLResponse
…th, 4 - ebook) in the GraphQLResponse
…g and field customization**
…rove query parameters**
Contributor
|
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:
Walkthrough
ChangesHardcover Metadata Search and Mapping Refactor
Sequence Diagram(s)sequenceDiagram
participant Caller
participant HardcoverParser
participant HardcoverBookSearchService
participant GraphQLAPI
Caller->>HardcoverParser: fetchMetadata(request)
alt ISBN present
HardcoverParser->>HardcoverBookSearchService: searchBookByIsbn(List~String~)
HardcoverBookSearchService->>GraphQLAPI: GraphQL query (_in isbn/hcid, score desc)
GraphQLAPI-->>HardcoverBookSearchService: List~BookWithEditions~
HardcoverBookSearchService-->>HardcoverParser: List~BookWithEditions~
else No ISBN or empty result
HardcoverParser->>HardcoverBookSearchService: searchBooks(title, author)
HardcoverBookSearchService->>GraphQLAPI: parameterized searchBooks query
GraphQLAPI-->>HardcoverBookSearchService: List~Hit~
HardcoverBookSearchService-->>HardcoverParser: List~Hit~
HardcoverParser->>HardcoverParser: filterSearch (Levenshtein title+author scoring)
HardcoverParser->>HardcoverBookSearchService: searchBookByHcid(List~Integer~)
HardcoverBookSearchService->>GraphQLAPI: GraphQL query (_in hcid)
GraphQLAPI-->>HardcoverBookSearchService: List~BookWithEditions~
HardcoverBookSearchService-->>HardcoverParser: List~BookWithEditions~
end
HardcoverParser->>HardcoverParser: filterEditionsByFormat (audiobook vs other)
HardcoverParser->>HardcoverParser: filterEditionsByLanguage (locale match)
HardcoverParser->>HardcoverParser: mapBookToMetadata (series, rating, ISBN, contributors, moods)
HardcoverParser-->>Caller: List~BookMetadata~
Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 minutes Possibly related PRs
Suggested reviewers
🚥 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
Warning Review ran into problems🔥 ProblemsLinked repositories: Your configuration references 1 linked repositories, but your current plan allows 0. Analyzed ``, skipped Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@backend/src/main/java/org/booklore/service/metadata/parser/HardcoverParser.java`:
- Around line 167-168: The methods getAuthorNames() on line 167, getTitle() on
line 206, and getUsersCount() on line 223 are being called without null checks
on their parent objects, which can cause NullPointerException at runtime when
upstream fields are null. Add null checks before each of these method calls to
ensure the parent object (doc) and any intermediate objects in the call chain
are not null before dereferencing them.
- Around line 421-424: The code in the series mapping section dereferences
getSeries() without null-checking its return value, which can cause a
NullPointerException when a partial payload lacks series metadata. Add an
additional null check after calling getSeries() on the result of
getFeaturedBookSeries() to ensure that getSeries() does not return null before
invoking getName() and getPrimaryBooksCount() on it. This will guard against
null series objects in the metadata payload.
- Around line 191-198: The conditional logic in the ternary operator that checks
whether best is less than threshold is inverted, causing low-quality author
matches to be included when a good match exists. Swap the two branches of the
ternary operator so that when best is less than threshold (indicating a good
match exists), the method returns the filtered docs stream using the Levenshtein
distance filter, and when best is greater than or equal to threshold (no good
match), it returns all docs unfiltered. This ensures poor author matches are
filtered out when better matches are available.
- Around line 302-313: The language filtering logic in the stream operation
incorrectly retrieves editions from the first book in results using
results.getFirst().getEditions() instead of from the current book being
processed. Replace results.getFirst().getEditions() with result.getEditions() in
the filter operation to ensure each book's editions are filtered individually
rather than all books being filtered against the first book's editions.
- Around line 52-57: In the searchByIsbn method, the null check on line 56 is
checking if the isbnCleaned list is null, but the list itself is always non-null
since it's created on line 53. The actual issue is that ParserUtils.cleanIsbn
might return null, which gets added to the list and triggers unnecessary remote
queries. Instead, check if the cleaned ISBN returned from ParserUtils.cleanIsbn
is null before adding it to the list, and if it is null, return
Collections.emptyList() immediately to prevent the avoidable failure path.
In
`@backend/src/test/java/org/booklore/service/metadata/parser/HardcoverParserTest.java`:
- Around line 56-67: The setUp method mutates global state by setting the
default locale to ENGLISH at line 61, but the tearDown method does not restore
the original locale, which can cause test suite flakiness. Store the original
default locale before calling Locale.setDefault(Locale.ENGLISH) in the setUp
method as a class field, then restore it in the tearDown method after closing
the mockJsoup static mock. This ensures that any locale changes made during test
execution are properly cleaned up and do not affect other tests.
🪄 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: 78b8fffc-158c-40ca-acc9-5c64d3aa8186
📒 Files selected for processing (11)
backend/src/main/java/org/booklore/model/enums/BookCategory.javabackend/src/main/java/org/booklore/service/metadata/MetadataRefreshService.javabackend/src/main/java/org/booklore/service/metadata/parser/HardcoverParser.javabackend/src/main/java/org/booklore/service/metadata/parser/hardcover/GraphQLResponse.javabackend/src/main/java/org/booklore/service/metadata/parser/hardcover/HardcoverBookSearchService.javabackend/src/test/java/org/booklore/service/metadata/parser/HardcoverParserTest.javabackend/src/test/java/org/booklore/service/metadata/parser/hardcover/HardcoverBookSearchServiceTest.javabackend/src/test/resources/hardcover/example-results-author.json.fixturebackend/src/test/resources/hardcover/example-results-lamb.json.fixturebackend/src/test/resources/hardcover/example-results-purged.json.fixturebackend/src/test/resources/hardcover/example-results.json.fixture
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Test Suite / Backend Tests
- GitHub Check: Test Suite / Frontend Tests
- GitHub Check: Analyze (java-kotlin)
- GitHub Check: Analyze (javascript-typescript)
🧰 Additional context used
📓 Path-based instructions (6)
**/*
⚙️ 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/test/resources/hardcover/example-results-author.json.fixturebackend/src/test/java/org/booklore/service/metadata/parser/hardcover/HardcoverBookSearchServiceTest.javabackend/src/main/java/org/booklore/service/metadata/MetadataRefreshService.javabackend/src/main/java/org/booklore/service/metadata/parser/hardcover/GraphQLResponse.javabackend/src/main/java/org/booklore/service/metadata/parser/HardcoverParser.javabackend/src/main/java/org/booklore/service/metadata/parser/hardcover/HardcoverBookSearchService.javabackend/src/test/java/org/booklore/service/metadata/parser/HardcoverParserTest.java
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/test/java/org/booklore/service/metadata/parser/hardcover/HardcoverBookSearchServiceTest.javabackend/src/main/java/org/booklore/service/metadata/MetadataRefreshService.javabackend/src/main/java/org/booklore/service/metadata/parser/hardcover/GraphQLResponse.javabackend/src/main/java/org/booklore/service/metadata/parser/HardcoverParser.javabackend/src/main/java/org/booklore/service/metadata/parser/hardcover/HardcoverBookSearchService.javabackend/src/test/java/org/booklore/service/metadata/parser/HardcoverParserTest.java
backend/src/test/**/*.java
📄 CodeRabbit inference engine (AGENTS.md)
Prefer focused unit tests; use
@SpringBootTestonly when the Spring context is required in backend code
Files:
backend/src/test/java/org/booklore/service/metadata/parser/hardcover/HardcoverBookSearchServiceTest.javabackend/src/test/java/org/booklore/service/metadata/parser/HardcoverParserTest.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/test/java/org/booklore/service/metadata/parser/hardcover/HardcoverBookSearchServiceTest.javabackend/src/main/java/org/booklore/service/metadata/MetadataRefreshService.javabackend/src/main/java/org/booklore/service/metadata/parser/hardcover/GraphQLResponse.javabackend/src/main/java/org/booklore/service/metadata/parser/HardcoverParser.javabackend/src/main/java/org/booklore/service/metadata/parser/hardcover/HardcoverBookSearchService.javabackend/src/test/java/org/booklore/service/metadata/parser/HardcoverParserTest.java
**/metadata/**/*.java
⚙️ CodeRabbit configuration file
**/metadata/**/*.java: Metadata and Sidecar review:
- Ensure perfect official compliance with Dublin Core and ANSI standards.
- Strictly follow official XML-compliant methods for EPUB2, EPUB3, CBX, and PDF formats.
- Sidecar files must be written in a way that is compatible with widespread e-reader standards.
- Avoid proprietary or non-standard XML tags unless absolutely necessary for internal tracking.
Files:
backend/src/test/java/org/booklore/service/metadata/parser/hardcover/HardcoverBookSearchServiceTest.javabackend/src/main/java/org/booklore/service/metadata/MetadataRefreshService.javabackend/src/main/java/org/booklore/service/metadata/parser/hardcover/GraphQLResponse.javabackend/src/main/java/org/booklore/service/metadata/parser/HardcoverParser.javabackend/src/main/java/org/booklore/service/metadata/parser/hardcover/HardcoverBookSearchService.javabackend/src/test/java/org/booklore/service/metadata/parser/HardcoverParserTest.java
**/*Test.java
⚙️ CodeRabbit configuration file
**/*Test.java: Java test review:
- Prefer
@ExtendWith(SpringExtension.class) or@SpringBootTestfor integration tests.- Flag tests with no assertions.
- Flag Thread.sleep() in tests; use Awaitility or virtual-thread-friendly alternatives.
- Flag hardcoded ports or file paths.
- Flag missing edge case coverage: null, empty, boundary values.
- Prefer AssertJ over JUnit's built-in assertions for readability.
- Prefer
@Sqlor Testcontainers for database state; not hand-rolled setup/teardown.
Files:
backend/src/test/java/org/booklore/service/metadata/parser/hardcover/HardcoverBookSearchServiceTest.javabackend/src/test/java/org/booklore/service/metadata/parser/HardcoverParserTest.java
🧠 Learnings (13)
📚 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/test/java/org/booklore/service/metadata/parser/hardcover/HardcoverBookSearchServiceTest.javabackend/src/main/java/org/booklore/service/metadata/MetadataRefreshService.javabackend/src/main/java/org/booklore/service/metadata/parser/hardcover/GraphQLResponse.javabackend/src/main/java/org/booklore/service/metadata/parser/HardcoverParser.javabackend/src/main/java/org/booklore/service/metadata/parser/hardcover/HardcoverBookSearchService.javabackend/src/test/java/org/booklore/service/metadata/parser/HardcoverParserTest.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/test/java/org/booklore/service/metadata/parser/hardcover/HardcoverBookSearchServiceTest.javabackend/src/main/java/org/booklore/service/metadata/MetadataRefreshService.javabackend/src/main/java/org/booklore/service/metadata/parser/hardcover/GraphQLResponse.javabackend/src/main/java/org/booklore/service/metadata/parser/HardcoverParser.javabackend/src/main/java/org/booklore/service/metadata/parser/hardcover/HardcoverBookSearchService.javabackend/src/test/java/org/booklore/service/metadata/parser/HardcoverParserTest.java
📚 Learning: 2026-05-07T21:21:55.233Z
Learnt from: imnotjames
Repo: grimmory-tools/grimmory PR: 1194
File: backend/src/main/java/org/booklore/service/ReadingSessionService.java:0-0
Timestamp: 2026-05-07T21:21:55.233Z
Learning: When reviewing Java 23+ code, treat `java.time.Instant#until(Instant endExclusive)` as a valid API/method call (it returns a `Duration`, equivalent to `Duration.between(this, endExclusive)`). Do not flag `instant.until(otherInstant)` as a compile error or API misuse when the project targets Java 25+ (as in grimmory-tools/grimmory); the call should be considered correct and returns a `Duration`.
Applied to files:
backend/src/test/java/org/booklore/service/metadata/parser/hardcover/HardcoverBookSearchServiceTest.javabackend/src/main/java/org/booklore/service/metadata/MetadataRefreshService.javabackend/src/main/java/org/booklore/service/metadata/parser/hardcover/GraphQLResponse.javabackend/src/main/java/org/booklore/service/metadata/parser/HardcoverParser.javabackend/src/main/java/org/booklore/service/metadata/parser/hardcover/HardcoverBookSearchService.javabackend/src/test/java/org/booklore/service/metadata/parser/HardcoverParserTest.java
📚 Learning: 2026-05-08T06:19:20.621Z
Learnt from: imnotjames
Repo: grimmory-tools/grimmory PR: 1201
File: backend/src/main/java/org/booklore/model/dto/AccessTokenDto.java:3-10
Timestamp: 2026-05-08T06:19:20.621Z
Learning: For Jackson 3 codebases, do not treat imports from `com.fasterxml.jackson.annotation.*` (e.g., `JsonInclude`, `JsonProperty`, `JsonView`) as incorrect. In Jackson 3, `jackson-annotations` intentionally remains under `com.fasterxml.jackson.annotation.*` for backward compatibility, while only the core processing packages (e.g., `jackson-core`, `jackson-databind`) move to the `tools.jackson.*` namespace.
Applied to files:
backend/src/test/java/org/booklore/service/metadata/parser/hardcover/HardcoverBookSearchServiceTest.javabackend/src/main/java/org/booklore/service/metadata/MetadataRefreshService.javabackend/src/main/java/org/booklore/service/metadata/parser/hardcover/GraphQLResponse.javabackend/src/main/java/org/booklore/service/metadata/parser/HardcoverParser.javabackend/src/main/java/org/booklore/service/metadata/parser/hardcover/HardcoverBookSearchService.javabackend/src/test/java/org/booklore/service/metadata/parser/HardcoverParserTest.java
📚 Learning: 2026-04-27T15:25:55.042Z
Learnt from: imnotjames
Repo: grimmory-tools/grimmory PR: 930
File: backend/src/test/java/org/booklore/service/metadata/parser/ComicvineBookParserTest.java:68-75
Timestamp: 2026-04-27T15:25:55.042Z
Learning: In this repository’s JUnit 5 test sources (e.g., under backend/src/test/java/), do not flag “bare” `assert` statements as a bug. The project test runner is configured to always execute tests with assertions enabled (e.g., `-ea`), so `assert` behavior is consistent. Continue to review for correctness, but don’t treat unguarded `assert` usage in test classes as a static-analysis issue.
Applied to files:
backend/src/test/java/org/booklore/service/metadata/parser/hardcover/HardcoverBookSearchServiceTest.javabackend/src/test/java/org/booklore/service/metadata/parser/HardcoverParserTest.java
📚 Learning: 2026-05-22T03:20:45.559Z
Learnt from: imnotjames
Repo: grimmory-tools/grimmory PR: 1446
File: backend/src/test/java/org/booklore/service/metadata/MetadataManagementServiceTest.java:465-470
Timestamp: 2026-05-22T03:20:45.559Z
Learning: In backend unit tests (Java files under backend/src/test/java), do not flag hardcoded placeholder filesystem paths (e.g., setting LibraryPathEntity.path = "/example") as violations when the value is intentionally unused for filesystem access. Only suppress the "no hardcoded paths" concern if the test does not perform any filesystem/network IO using that path (no reads/writes/Files.* calls or code paths that access the filesystem with that value); if the placeholder is actually used to touch the filesystem, flag it.
Applied to files:
backend/src/test/java/org/booklore/service/metadata/parser/hardcover/HardcoverBookSearchServiceTest.javabackend/src/test/java/org/booklore/service/metadata/parser/HardcoverParserTest.java
📚 Learning: 2026-05-04T05:01:33.919Z
Learnt from: imnotjames
Repo: grimmory-tools/grimmory PR: 1081
File: backend/src/test/java/org/booklore/service/metadata/parser/AudibleParserTest.java:132-135
Timestamp: 2026-05-04T05:01:33.919Z
Learning: In grimmory-tools/grimmory unit/integration test classes (e.g., files matching **/*Test.java under backend/src/test/java/**), it is acceptable to directly instantiate Jackson mappers (e.g., `new ObjectMapper()` / `new JsonMapper(...)`) and this should NOT be flagged. The Jackson guidance to use Spring bean injection or `JsonMapper.shared()` applies only to production code; tests may construct dependencies directly as standard practice. Production-code mapper instantiation rules should still be enforced outside test sources.
Applied to files:
backend/src/test/java/org/booklore/service/metadata/parser/hardcover/HardcoverBookSearchServiceTest.javabackend/src/test/java/org/booklore/service/metadata/parser/HardcoverParserTest.java
📚 Learning: 2026-05-04T20:31:11.075Z
Learnt from: imnotjames
Repo: grimmory-tools/grimmory PR: 1086
File: backend/src/main/java/org/booklore/service/metadata/BookReviewUpdateService.java:63-66
Timestamp: 2026-05-04T20:31:11.075Z
Learning: For this repository, reviewers should treat string truncation done via `String.length()` and `String.substring(0, maxLength)` (UTF-16 code units) as an accepted, consistent convention. Do not flag individual occurrences of this pattern as bugs, even though it is not code-point-aware for surrogate pairs. A separate global effort is already tracked to move toward code-point-aware truncation, so per-site fixes should be avoided during code review.
Applied to files:
backend/src/main/java/org/booklore/service/metadata/MetadataRefreshService.javabackend/src/main/java/org/booklore/service/metadata/parser/hardcover/GraphQLResponse.javabackend/src/main/java/org/booklore/service/metadata/parser/HardcoverParser.javabackend/src/main/java/org/booklore/service/metadata/parser/hardcover/HardcoverBookSearchService.java
📚 Learning: 2026-05-13T12:34:49.607Z
Learnt from: balazs-szucs
Repo: grimmory-tools/grimmory PR: 1293
File: backend/src/main/java/org/booklore/service/metadata/DuckDuckGoCoverService.java:246-252
Timestamp: 2026-05-13T12:34:49.607Z
Learning: In this repo’s Java code, when catching Jsoup `org.jsoup.HttpStatusException` (and similar exceptions originating from external libraries) and wrapping/rethrowing them, do not require preserving the original exception stack trace (e.g., as flagged by PMD `PreserveStackTrace`) as long as the application already captures the actionable diagnostics in logs or the thrown exception message (such as HTTP status code and the requested URL). Reviewers should still ensure the log/message contains those details; the intent is to avoid noisy stack traces that only reflect external-library internals rather than application code.
Applied to files:
backend/src/main/java/org/booklore/service/metadata/MetadataRefreshService.javabackend/src/main/java/org/booklore/service/metadata/parser/hardcover/GraphQLResponse.javabackend/src/main/java/org/booklore/service/metadata/parser/HardcoverParser.javabackend/src/main/java/org/booklore/service/metadata/parser/hardcover/HardcoverBookSearchService.java
📚 Learning: 2026-05-17T13:38:16.462Z
Learnt from: balazs-szucs
Repo: grimmory-tools/grimmory PR: 1366
File: backend/src/main/java/org/booklore/service/FileStreamingService.java:65-66
Timestamp: 2026-05-17T13:38:16.462Z
Learning: In the grimmory-tools/grimmory repo, it’s an accepted pattern to pass the raw AccessDeniedException.getMessage() (even if it may include filesystem path details) into ApiError.PERMISSION_DENIED.createException(...). During code review, do not raise a security/information-disclosure issue solely based on that exception message being propagated to the API when using ApiError.PERMISSION_DENIED.createException with the AccessDeniedException message.
Applied to files:
backend/src/main/java/org/booklore/service/metadata/MetadataRefreshService.javabackend/src/main/java/org/booklore/service/metadata/parser/hardcover/GraphQLResponse.javabackend/src/main/java/org/booklore/service/metadata/parser/HardcoverParser.javabackend/src/main/java/org/booklore/service/metadata/parser/hardcover/HardcoverBookSearchService.java
📚 Learning: 2026-05-23T23:01:25.769Z
Learnt from: imnotjames
Repo: grimmory-tools/grimmory PR: 1456
File: backend/src/main/java/org/booklore/service/metadata/parser/GoodReadsParser.java:618-630
Timestamp: 2026-05-23T23:01:25.769Z
Learning: In this codebase (grimmory-tools/grimmory), it’s intentional to omit per-request timeouts on individual Java HttpRequest.Builder instances (e.g., GoodReadsParser.fetchJson). During reviews, do not flag missing builder-level timeouts as a best-practice violation; rely on framework-level and/or HttpClient-level timeouts configured elsewhere for consistent behavior. Only raise an issue if you can verify that no effective timeout is configured at the HttpClient/framework level.
Applied to files:
backend/src/main/java/org/booklore/service/metadata/MetadataRefreshService.javabackend/src/main/java/org/booklore/service/metadata/parser/hardcover/GraphQLResponse.javabackend/src/main/java/org/booklore/service/metadata/parser/HardcoverParser.javabackend/src/main/java/org/booklore/service/metadata/parser/hardcover/HardcoverBookSearchService.java
📚 Learning: 2026-06-12T01:10:31.416Z
Learnt from: imnotjames
Repo: grimmory-tools/grimmory PR: 1724
File: backend/src/main/java/org/booklore/repository/BookRepository.java:58-59
Timestamp: 2026-06-12T01:10:31.416Z
Learning: In this codebase (grimmory-tools/grimmory), reviews should not treat inline `LIMIT`/`OFFSET` clauses inside `Query` JPQL/HQL strings as a JPA compliance risk. This is intentional: `hibernate.jpa.compliance.query=true` is intentionally not set, and Hibernate 7.3+ supports `LIMIT`/`OFFSET` as valid HQL extensions. Therefore, do not flag or require changes to `Query` annotations solely due to `LIMIT`/`OFFSET` usage.
Applied to files:
backend/src/main/java/org/booklore/service/metadata/MetadataRefreshService.javabackend/src/main/java/org/booklore/service/metadata/parser/hardcover/GraphQLResponse.javabackend/src/main/java/org/booklore/service/metadata/parser/HardcoverParser.javabackend/src/main/java/org/booklore/service/metadata/parser/hardcover/HardcoverBookSearchService.java
📚 Learning: 2026-05-07T21:37:46.988Z
Learnt from: imnotjames
Repo: grimmory-tools/grimmory PR: 1194
File: backend/src/main/java/org/booklore/service/ReadingSessionService.java:121-123
Timestamp: 2026-05-07T21:37:46.988Z
Learning: In grimmory-tools/grimmory service-layer code, if arithmetic overflow occurs inside inference/business-logic (e.g., when deriving inferred fields like durationSeconds from start/end timestamps), treat it as a server-side anomaly. Prefer letting the global exception handler translate it into a generic 5xx response rather than throwing an explicit ApiError 4xx (e.g., do not convert overflow into a client error).
Applied to files:
backend/src/main/java/org/booklore/service/metadata/MetadataRefreshService.javabackend/src/main/java/org/booklore/service/metadata/parser/hardcover/GraphQLResponse.javabackend/src/main/java/org/booklore/service/metadata/parser/HardcoverParser.javabackend/src/main/java/org/booklore/service/metadata/parser/hardcover/HardcoverBookSearchService.java
🪛 PMD (7.25.0)
backend/src/test/java/org/booklore/service/metadata/parser/HardcoverParserTest.java
[Medium] 48-48: UnusedPrivateField (Best Practices): Avoid unused private fields such as 'log'.
(UnusedPrivateField (Best Practices))
🔇 Additional comments (5)
backend/src/main/java/org/booklore/service/metadata/MetadataRefreshService.java (1)
31-31: LGTM!backend/src/test/resources/hardcover/example-results-author.json.fixture (1)
1-62: LGTM!backend/src/test/java/org/booklore/service/metadata/parser/hardcover/HardcoverBookSearchServiceTest.java (1)
31-40: LGTM!backend/src/main/java/org/booklore/service/metadata/parser/hardcover/GraphQLResponse.java (1)
5-8: LGTM!Also applies to: 166-167, 245-249
backend/src/main/java/org/booklore/service/metadata/parser/hardcover/HardcoverBookSearchService.java (1)
38-50: LGTM!Also applies to: 122-140, 149-163, 219-219
…o author match (then retry title only)
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
backend/src/main/java/org/booklore/service/metadata/parser/HardcoverParser.java (3)
280-280:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPotential NPE if
getBookType()returns null.Line 276 guards against null
primaryFile, but ifgetBookType()returns null, calling.equals()on it will throw NPE.- boolean isAudiobook = book.getPrimaryFile().getBookType().equals(BookFileType.AUDIOBOOK); + boolean isAudiobook = BookFileType.AUDIOBOOK.equals(book.getPrimaryFile().getBookType());🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/src/main/java/org/booklore/service/metadata/parser/HardcoverParser.java` at line 280, The code at line 280 calls `.equals()` on the result of `getBookType()` without checking if it returns null, which will cause a null pointer exception. Add a null check for the `getBookType()` result before calling `.equals()` on it in the isAudiobook assignment. Consider using a null-safe approach such as checking if getBookType() is not null before comparing, or using Optional if available in your codebase.
341-341:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDuplicate series mapping—
mapSeiesDataandmapSeriesInfoboth called.Both methods set
seriesName,seriesTotal, andseriesNumberfrom the samebook.getFeaturedBookSeries()source. This redundantly overwrites the same fields twice. Additionally,mapSeiesDatahas a typo in its name.Remove one of these methods and its invocation.
Suggested fix
Keep only
mapSeriesInfo(which has the null-safe guards) and remove the duplicate:- mapSeiesData(metadata, book); mapRating(metadata, book);Then delete the entire
mapSeiesDatamethod (lines 389-403).Also applies to: 351-351
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/src/main/java/org/booklore/service/metadata/parser/HardcoverParser.java` at line 341, Remove the duplicate series mapping by deleting all invocations of the misnamed method mapSeiesData (which appear at lines 341 and 351 in the HardcoverParser class) and then delete the entire mapSeiesData method definition (lines 389-403). Keep only the mapSeriesInfo method invocation and its implementation, as it contains proper null-safe guards and achieves the same result without redundancy.
331-333:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSubtitle removal uses inconsistent sources.
The condition checks
edition.getSubtitle(), but the replacement removesbook.getSubtitle(). If these differ, the replacement will fail to match or remove the wrong text.if (edition.getSubtitle() != null && book.getTitle().contains(edition.getSubtitle())) { - book.setTitle(book.getTitle().replace(": " + book.getSubtitle(), "")); + book.setTitle(book.getTitle().replace(": " + edition.getSubtitle(), "")); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/src/main/java/org/booklore/service/metadata/parser/HardcoverParser.java` around lines 331 - 333, The subtitle removal logic in the HardcoverParser class contains an inconsistency where the condition checks edition.getSubtitle() but the replacement uses book.getSubtitle(). If these two values differ, the replacement will fail to match the expected text. Ensure consistency by using the same subtitle source in both the condition and the replace operation. Determine whether to use edition.getSubtitle() or book.getSubtitle() throughout based on the intended logic, and update both the condition check and the replacement call to use the same source.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@backend/src/test/java/org/booklore/service/metadata/parser/HardcoverParserTest.java`:
- Around line 965-968: The Edition Filtering Tests in HardcoverParserTest are
providing an author parameter via .author("Ernest Cline") in the request but
only stubbing the single-argument hardcoverBookSearchService.searchBooks(String
title) method. When an author is present, the parser invokes the two-argument
searchBooks(title, author) method first, which is not stubbed and returns null,
causing the test to fall back to the single-argument method. To fix this and
make the tests directly exercise the intended code path, add a stub for the
two-argument method by calling
when(hardcoverBookSearchService.searchBooks("Ready Player One", "Ernest
Cline")).thenReturn(hit) before the existing single-argument stub. Apply this
change to all affected test methods at the specified line ranges: 965-968,
1000-1003, 1034-1037, and 1069-1072.
---
Outside diff comments:
In
`@backend/src/main/java/org/booklore/service/metadata/parser/HardcoverParser.java`:
- Line 280: The code at line 280 calls `.equals()` on the result of
`getBookType()` without checking if it returns null, which will cause a null
pointer exception. Add a null check for the `getBookType()` result before
calling `.equals()` on it in the isAudiobook assignment. Consider using a
null-safe approach such as checking if getBookType() is not null before
comparing, or using Optional if available in your codebase.
- Line 341: Remove the duplicate series mapping by deleting all invocations of
the misnamed method mapSeiesData (which appear at lines 341 and 351 in the
HardcoverParser class) and then delete the entire mapSeiesData method definition
(lines 389-403). Keep only the mapSeriesInfo method invocation and its
implementation, as it contains proper null-safe guards and achieves the same
result without redundancy.
- Around line 331-333: The subtitle removal logic in the HardcoverParser class
contains an inconsistency where the condition checks edition.getSubtitle() but
the replacement uses book.getSubtitle(). If these two values differ, the
replacement will fail to match the expected text. Ensure consistency by using
the same subtitle source in both the condition and the replace operation.
Determine whether to use edition.getSubtitle() or book.getSubtitle() throughout
based on the intended logic, and update both the condition check and the
replacement call to use the same source.
🪄 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: 2fd3e14a-781d-47c9-8a1c-762420b49d4c
📒 Files selected for processing (2)
backend/src/main/java/org/booklore/service/metadata/parser/HardcoverParser.javabackend/src/test/java/org/booklore/service/metadata/parser/HardcoverParserTest.java
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Test Suite / Backend Tests
- GitHub Check: Test Suite / Frontend Tests
- GitHub Check: Analyze (java-kotlin)
🧰 Additional context used
📓 Path-based instructions (6)
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/metadata/parser/HardcoverParser.javabackend/src/test/java/org/booklore/service/metadata/parser/HardcoverParserTest.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/metadata/parser/HardcoverParser.javabackend/src/test/java/org/booklore/service/metadata/parser/HardcoverParserTest.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/metadata/parser/HardcoverParser.javabackend/src/test/java/org/booklore/service/metadata/parser/HardcoverParserTest.java
**/metadata/**/*.java
⚙️ CodeRabbit configuration file
**/metadata/**/*.java: Metadata and Sidecar review:
- Ensure perfect official compliance with Dublin Core and ANSI standards.
- Strictly follow official XML-compliant methods for EPUB2, EPUB3, CBX, and PDF formats.
- Sidecar files must be written in a way that is compatible with widespread e-reader standards.
- Avoid proprietary or non-standard XML tags unless absolutely necessary for internal tracking.
Files:
backend/src/main/java/org/booklore/service/metadata/parser/HardcoverParser.javabackend/src/test/java/org/booklore/service/metadata/parser/HardcoverParserTest.java
backend/src/test/**/*.java
📄 CodeRabbit inference engine (AGENTS.md)
Prefer focused unit tests; use
@SpringBootTestonly when the Spring context is required in backend code
Files:
backend/src/test/java/org/booklore/service/metadata/parser/HardcoverParserTest.java
**/*Test.java
⚙️ CodeRabbit configuration file
**/*Test.java: Java test review:
- Prefer
@ExtendWith(SpringExtension.class) or@SpringBootTestfor integration tests.- Flag tests with no assertions.
- Flag Thread.sleep() in tests; use Awaitility or virtual-thread-friendly alternatives.
- Flag hardcoded ports or file paths.
- Flag missing edge case coverage: null, empty, boundary values.
- Prefer AssertJ over JUnit's built-in assertions for readability.
- Prefer
@Sqlor Testcontainers for database state; not hand-rolled setup/teardown.
Files:
backend/src/test/java/org/booklore/service/metadata/parser/HardcoverParserTest.java
🧠 Learnings (13)
📚 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/metadata/parser/HardcoverParser.javabackend/src/test/java/org/booklore/service/metadata/parser/HardcoverParserTest.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/metadata/parser/HardcoverParser.javabackend/src/test/java/org/booklore/service/metadata/parser/HardcoverParserTest.java
📚 Learning: 2026-05-07T21:21:55.233Z
Learnt from: imnotjames
Repo: grimmory-tools/grimmory PR: 1194
File: backend/src/main/java/org/booklore/service/ReadingSessionService.java:0-0
Timestamp: 2026-05-07T21:21:55.233Z
Learning: When reviewing Java 23+ code, treat `java.time.Instant#until(Instant endExclusive)` as a valid API/method call (it returns a `Duration`, equivalent to `Duration.between(this, endExclusive)`). Do not flag `instant.until(otherInstant)` as a compile error or API misuse when the project targets Java 25+ (as in grimmory-tools/grimmory); the call should be considered correct and returns a `Duration`.
Applied to files:
backend/src/main/java/org/booklore/service/metadata/parser/HardcoverParser.javabackend/src/test/java/org/booklore/service/metadata/parser/HardcoverParserTest.java
📚 Learning: 2026-05-08T06:19:20.621Z
Learnt from: imnotjames
Repo: grimmory-tools/grimmory PR: 1201
File: backend/src/main/java/org/booklore/model/dto/AccessTokenDto.java:3-10
Timestamp: 2026-05-08T06:19:20.621Z
Learning: For Jackson 3 codebases, do not treat imports from `com.fasterxml.jackson.annotation.*` (e.g., `JsonInclude`, `JsonProperty`, `JsonView`) as incorrect. In Jackson 3, `jackson-annotations` intentionally remains under `com.fasterxml.jackson.annotation.*` for backward compatibility, while only the core processing packages (e.g., `jackson-core`, `jackson-databind`) move to the `tools.jackson.*` namespace.
Applied to files:
backend/src/main/java/org/booklore/service/metadata/parser/HardcoverParser.javabackend/src/test/java/org/booklore/service/metadata/parser/HardcoverParserTest.java
📚 Learning: 2026-05-04T20:31:11.075Z
Learnt from: imnotjames
Repo: grimmory-tools/grimmory PR: 1086
File: backend/src/main/java/org/booklore/service/metadata/BookReviewUpdateService.java:63-66
Timestamp: 2026-05-04T20:31:11.075Z
Learning: For this repository, reviewers should treat string truncation done via `String.length()` and `String.substring(0, maxLength)` (UTF-16 code units) as an accepted, consistent convention. Do not flag individual occurrences of this pattern as bugs, even though it is not code-point-aware for surrogate pairs. A separate global effort is already tracked to move toward code-point-aware truncation, so per-site fixes should be avoided during code review.
Applied to files:
backend/src/main/java/org/booklore/service/metadata/parser/HardcoverParser.java
📚 Learning: 2026-05-13T12:34:49.607Z
Learnt from: balazs-szucs
Repo: grimmory-tools/grimmory PR: 1293
File: backend/src/main/java/org/booklore/service/metadata/DuckDuckGoCoverService.java:246-252
Timestamp: 2026-05-13T12:34:49.607Z
Learning: In this repo’s Java code, when catching Jsoup `org.jsoup.HttpStatusException` (and similar exceptions originating from external libraries) and wrapping/rethrowing them, do not require preserving the original exception stack trace (e.g., as flagged by PMD `PreserveStackTrace`) as long as the application already captures the actionable diagnostics in logs or the thrown exception message (such as HTTP status code and the requested URL). Reviewers should still ensure the log/message contains those details; the intent is to avoid noisy stack traces that only reflect external-library internals rather than application code.
Applied to files:
backend/src/main/java/org/booklore/service/metadata/parser/HardcoverParser.java
📚 Learning: 2026-05-17T13:38:16.462Z
Learnt from: balazs-szucs
Repo: grimmory-tools/grimmory PR: 1366
File: backend/src/main/java/org/booklore/service/FileStreamingService.java:65-66
Timestamp: 2026-05-17T13:38:16.462Z
Learning: In the grimmory-tools/grimmory repo, it’s an accepted pattern to pass the raw AccessDeniedException.getMessage() (even if it may include filesystem path details) into ApiError.PERMISSION_DENIED.createException(...). During code review, do not raise a security/information-disclosure issue solely based on that exception message being propagated to the API when using ApiError.PERMISSION_DENIED.createException with the AccessDeniedException message.
Applied to files:
backend/src/main/java/org/booklore/service/metadata/parser/HardcoverParser.java
📚 Learning: 2026-05-23T23:01:25.769Z
Learnt from: imnotjames
Repo: grimmory-tools/grimmory PR: 1456
File: backend/src/main/java/org/booklore/service/metadata/parser/GoodReadsParser.java:618-630
Timestamp: 2026-05-23T23:01:25.769Z
Learning: In this codebase (grimmory-tools/grimmory), it’s intentional to omit per-request timeouts on individual Java HttpRequest.Builder instances (e.g., GoodReadsParser.fetchJson). During reviews, do not flag missing builder-level timeouts as a best-practice violation; rely on framework-level and/or HttpClient-level timeouts configured elsewhere for consistent behavior. Only raise an issue if you can verify that no effective timeout is configured at the HttpClient/framework level.
Applied to files:
backend/src/main/java/org/booklore/service/metadata/parser/HardcoverParser.java
📚 Learning: 2026-06-12T01:10:31.416Z
Learnt from: imnotjames
Repo: grimmory-tools/grimmory PR: 1724
File: backend/src/main/java/org/booklore/repository/BookRepository.java:58-59
Timestamp: 2026-06-12T01:10:31.416Z
Learning: In this codebase (grimmory-tools/grimmory), reviews should not treat inline `LIMIT`/`OFFSET` clauses inside `Query` JPQL/HQL strings as a JPA compliance risk. This is intentional: `hibernate.jpa.compliance.query=true` is intentionally not set, and Hibernate 7.3+ supports `LIMIT`/`OFFSET` as valid HQL extensions. Therefore, do not flag or require changes to `Query` annotations solely due to `LIMIT`/`OFFSET` usage.
Applied to files:
backend/src/main/java/org/booklore/service/metadata/parser/HardcoverParser.java
📚 Learning: 2026-05-07T21:37:46.988Z
Learnt from: imnotjames
Repo: grimmory-tools/grimmory PR: 1194
File: backend/src/main/java/org/booklore/service/ReadingSessionService.java:121-123
Timestamp: 2026-05-07T21:37:46.988Z
Learning: In grimmory-tools/grimmory service-layer code, if arithmetic overflow occurs inside inference/business-logic (e.g., when deriving inferred fields like durationSeconds from start/end timestamps), treat it as a server-side anomaly. Prefer letting the global exception handler translate it into a generic 5xx response rather than throwing an explicit ApiError 4xx (e.g., do not convert overflow into a client error).
Applied to files:
backend/src/main/java/org/booklore/service/metadata/parser/HardcoverParser.java
📚 Learning: 2026-04-27T15:25:55.042Z
Learnt from: imnotjames
Repo: grimmory-tools/grimmory PR: 930
File: backend/src/test/java/org/booklore/service/metadata/parser/ComicvineBookParserTest.java:68-75
Timestamp: 2026-04-27T15:25:55.042Z
Learning: In this repository’s JUnit 5 test sources (e.g., under backend/src/test/java/), do not flag “bare” `assert` statements as a bug. The project test runner is configured to always execute tests with assertions enabled (e.g., `-ea`), so `assert` behavior is consistent. Continue to review for correctness, but don’t treat unguarded `assert` usage in test classes as a static-analysis issue.
Applied to files:
backend/src/test/java/org/booklore/service/metadata/parser/HardcoverParserTest.java
📚 Learning: 2026-05-22T03:20:45.559Z
Learnt from: imnotjames
Repo: grimmory-tools/grimmory PR: 1446
File: backend/src/test/java/org/booklore/service/metadata/MetadataManagementServiceTest.java:465-470
Timestamp: 2026-05-22T03:20:45.559Z
Learning: In backend unit tests (Java files under backend/src/test/java), do not flag hardcoded placeholder filesystem paths (e.g., setting LibraryPathEntity.path = "/example") as violations when the value is intentionally unused for filesystem access. Only suppress the "no hardcoded paths" concern if the test does not perform any filesystem/network IO using that path (no reads/writes/Files.* calls or code paths that access the filesystem with that value); if the placeholder is actually used to touch the filesystem, flag it.
Applied to files:
backend/src/test/java/org/booklore/service/metadata/parser/HardcoverParserTest.java
📚 Learning: 2026-05-04T05:01:33.919Z
Learnt from: imnotjames
Repo: grimmory-tools/grimmory PR: 1081
File: backend/src/test/java/org/booklore/service/metadata/parser/AudibleParserTest.java:132-135
Timestamp: 2026-05-04T05:01:33.919Z
Learning: In grimmory-tools/grimmory unit/integration test classes (e.g., files matching **/*Test.java under backend/src/test/java/**), it is acceptable to directly instantiate Jackson mappers (e.g., `new ObjectMapper()` / `new JsonMapper(...)`) and this should NOT be flagged. The Jackson guidance to use Spring bean injection or `JsonMapper.shared()` applies only to production code; tests may construct dependencies directly as standard practice. Production-code mapper instantiation rules should still be enforced outside test sources.
Applied to files:
backend/src/test/java/org/booklore/service/metadata/parser/HardcoverParserTest.java
🔇 Additional comments (8)
backend/src/main/java/org/booklore/service/metadata/parser/HardcoverParser.java (7)
54-58: LGTM!
167-169: LGTM!
194-198: LGTM!
206-208: LGTM!Also applies to: 223-226
305-309: LGTM!
425-428: LGTM!
65-87: LGTM!Also applies to: 89-120, 122-129, 131-161, 231-264, 266-272, 361-380, 382-387, 405-412, 414-422, 441-448, 450-457, 459-466, 468-488, 490-498, 500-509, 511-515, 517-521, 523-527
backend/src/test/java/org/booklore/service/metadata/parser/HardcoverParserTest.java (1)
49-50: LGTM!Also applies to: 63-70
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
backend/src/test/java/org/booklore/service/metadata/parser/HardcoverParserTest.java (2)
728-728: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueSimplify by removing unnecessary stream conversion.
booksis already aList, so.stream().toList()creates an unnecessary intermediate copy before callingforEach. You can callforEachdirectly on the list.Simplified version
- books.stream().toList().forEach(n -> n.setImage(null)); + books.forEach(n -> n.setImage(null));🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/src/test/java/org/booklore/service/metadata/parser/HardcoverParserTest.java` at line 728, The line in HardcoverParserTest where books.stream().toList().forEach is called creates an unnecessary intermediate stream and list conversion. Since books is already a List, remove the unnecessary stream() and toList() calls and invoke forEach directly on the books list object. This simplifies the code by eliminating redundant conversions that don't add any value.
949-985: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winComplete the stubbing fix: add two-argument searchBooks stub.
This test provides an author (line 962) but only stubs the single-argument
searchBooks("Ready Player One")method (line 967). The sibling tests in this nested class at lines 1000-1020, 1034-1055, and 1069-1089 were updated to stub the two-argument method when author is provided, but this test was missed.Without the two-argument stub, the parser calls
searchBooks(title, author)→ returns null → falls back tosearchBooks(title). The test passes but unintentionally exercises the search-fallback path rather than directly testing edition filtering.Proposed fix matching sibling tests
List<GraphQLResponse.Hit> hit = new ArrayList<>(); hit.add(createHitWithAuthor("Ready Player One", "Ernest Cline", "26363")); - when(hardcoverBookSearchService.searchBooks("Ready Player One")) + when(hardcoverBookSearchService.searchBooks("Ready Player One", "Ernest Cline")) .thenReturn(hit); String booksFixture = readFixture("example-results.json"); List<GraphQLResponse.BookWithEditions> books = parseBooks(booksFixture); when(hardcoverBookSearchService.searchBookByHcid((List<Integer>) any())) .thenReturn(books); // Act List<BookMetadata> results = parser.fetchMetadata(book, request); // Assert - verify(hardcoverBookSearchService).searchBooks("Ready Player One"); + verify(hardcoverBookSearchService).searchBooks("Ready Player One", "Ernest Cline");🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/src/test/java/org/booklore/service/metadata/parser/HardcoverParserTest.java` around lines 949 - 985, The test fetchAudiobookEditions_whenFormat_returnsList provides an author in the FetchMetadataRequest but only stubs the single-argument searchBooks method. Add a stub for the two-argument searchBooks method that accepts both title and author parameters (following the pattern of the sibling test cases) right after the existing single-argument stub to ensure the parser tests the intended code path with the author parameter instead of falling back to the title-only search.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In
`@backend/src/test/java/org/booklore/service/metadata/parser/HardcoverParserTest.java`:
- Line 728: The line in HardcoverParserTest where
books.stream().toList().forEach is called creates an unnecessary intermediate
stream and list conversion. Since books is already a List, remove the
unnecessary stream() and toList() calls and invoke forEach directly on the books
list object. This simplifies the code by eliminating redundant conversions that
don't add any value.
- Around line 949-985: The test fetchAudiobookEditions_whenFormat_returnsList
provides an author in the FetchMetadataRequest but only stubs the
single-argument searchBooks method. Add a stub for the two-argument searchBooks
method that accepts both title and author parameters (following the pattern of
the sibling test cases) right after the existing single-argument stub to ensure
the parser tests the intended code path with the author parameter instead of
falling back to the title-only search.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: 83193628-fe6d-4875-b6a6-e42d5ec9401a
📒 Files selected for processing (1)
backend/src/test/java/org/booklore/service/metadata/parser/HardcoverParserTest.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 (6)
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/test/java/org/booklore/service/metadata/parser/HardcoverParserTest.java
backend/src/test/**/*.java
📄 CodeRabbit inference engine (AGENTS.md)
Prefer focused unit tests; use
@SpringBootTestonly when the Spring context is required in backend code
Files:
backend/src/test/java/org/booklore/service/metadata/parser/HardcoverParserTest.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/test/java/org/booklore/service/metadata/parser/HardcoverParserTest.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/test/java/org/booklore/service/metadata/parser/HardcoverParserTest.java
**/metadata/**/*.java
⚙️ CodeRabbit configuration file
**/metadata/**/*.java: Metadata and Sidecar review:
- Ensure perfect official compliance with Dublin Core and ANSI standards.
- Strictly follow official XML-compliant methods for EPUB2, EPUB3, CBX, and PDF formats.
- Sidecar files must be written in a way that is compatible with widespread e-reader standards.
- Avoid proprietary or non-standard XML tags unless absolutely necessary for internal tracking.
Files:
backend/src/test/java/org/booklore/service/metadata/parser/HardcoverParserTest.java
**/*Test.java
⚙️ CodeRabbit configuration file
**/*Test.java: Java test review:
- Prefer
@ExtendWith(SpringExtension.class) or@SpringBootTestfor integration tests.- Flag tests with no assertions.
- Flag Thread.sleep() in tests; use Awaitility or virtual-thread-friendly alternatives.
- Flag hardcoded ports or file paths.
- Flag missing edge case coverage: null, empty, boundary values.
- Prefer AssertJ over JUnit's built-in assertions for readability.
- Prefer
@Sqlor Testcontainers for database state; not hand-rolled setup/teardown.
Files:
backend/src/test/java/org/booklore/service/metadata/parser/HardcoverParserTest.java
🧠 Learnings (7)
📚 Learning: 2026-04-10T08:15:37.436Z
Learnt from: imnotjames
Repo: grimmory-tools/grimmory PR: 449
File: booklore-api/src/main/java/org/booklore/service/book/BookDownloadService.java:139-145
Timestamp: 2026-04-10T08:15:37.436Z
Learning: When using Spring `ContentDisposition.builder(...).filename(name, StandardCharsets.UTF_8).build()` (i.e., explicitly providing UTF-8), the resulting header value should include both the quoted `filename="=?UTF-8?..."` and the RFC 5987 `filename*=` parameters. In this case, any extra ASCII fallback computation (e.g., deriving an ASCII `fallbackFilename` via `NON_ASCII_PATTERN` and calling `.filename(fallbackFilename)`) is likely redundant—prefer calling only `.filename(fallbackName?, StandardCharsets.UTF_8)` as appropriate and let Spring handle the UTF-8 header parameters. Verify by comparing the emitted header for `filename` and `filename*` before deciding to keep an ASCII fallback.
Applied to files:
backend/src/test/java/org/booklore/service/metadata/parser/HardcoverParserTest.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/test/java/org/booklore/service/metadata/parser/HardcoverParserTest.java
📚 Learning: 2026-05-07T21:21:55.233Z
Learnt from: imnotjames
Repo: grimmory-tools/grimmory PR: 1194
File: backend/src/main/java/org/booklore/service/ReadingSessionService.java:0-0
Timestamp: 2026-05-07T21:21:55.233Z
Learning: When reviewing Java 23+ code, treat `java.time.Instant#until(Instant endExclusive)` as a valid API/method call (it returns a `Duration`, equivalent to `Duration.between(this, endExclusive)`). Do not flag `instant.until(otherInstant)` as a compile error or API misuse when the project targets Java 25+ (as in grimmory-tools/grimmory); the call should be considered correct and returns a `Duration`.
Applied to files:
backend/src/test/java/org/booklore/service/metadata/parser/HardcoverParserTest.java
📚 Learning: 2026-05-08T06:19:20.621Z
Learnt from: imnotjames
Repo: grimmory-tools/grimmory PR: 1201
File: backend/src/main/java/org/booklore/model/dto/AccessTokenDto.java:3-10
Timestamp: 2026-05-08T06:19:20.621Z
Learning: For Jackson 3 codebases, do not treat imports from `com.fasterxml.jackson.annotation.*` (e.g., `JsonInclude`, `JsonProperty`, `JsonView`) as incorrect. In Jackson 3, `jackson-annotations` intentionally remains under `com.fasterxml.jackson.annotation.*` for backward compatibility, while only the core processing packages (e.g., `jackson-core`, `jackson-databind`) move to the `tools.jackson.*` namespace.
Applied to files:
backend/src/test/java/org/booklore/service/metadata/parser/HardcoverParserTest.java
📚 Learning: 2026-04-27T15:25:55.042Z
Learnt from: imnotjames
Repo: grimmory-tools/grimmory PR: 930
File: backend/src/test/java/org/booklore/service/metadata/parser/ComicvineBookParserTest.java:68-75
Timestamp: 2026-04-27T15:25:55.042Z
Learning: In this repository’s JUnit 5 test sources (e.g., under backend/src/test/java/), do not flag “bare” `assert` statements as a bug. The project test runner is configured to always execute tests with assertions enabled (e.g., `-ea`), so `assert` behavior is consistent. Continue to review for correctness, but don’t treat unguarded `assert` usage in test classes as a static-analysis issue.
Applied to files:
backend/src/test/java/org/booklore/service/metadata/parser/HardcoverParserTest.java
📚 Learning: 2026-05-22T03:20:45.559Z
Learnt from: imnotjames
Repo: grimmory-tools/grimmory PR: 1446
File: backend/src/test/java/org/booklore/service/metadata/MetadataManagementServiceTest.java:465-470
Timestamp: 2026-05-22T03:20:45.559Z
Learning: In backend unit tests (Java files under backend/src/test/java), do not flag hardcoded placeholder filesystem paths (e.g., setting LibraryPathEntity.path = "/example") as violations when the value is intentionally unused for filesystem access. Only suppress the "no hardcoded paths" concern if the test does not perform any filesystem/network IO using that path (no reads/writes/Files.* calls or code paths that access the filesystem with that value); if the placeholder is actually used to touch the filesystem, flag it.
Applied to files:
backend/src/test/java/org/booklore/service/metadata/parser/HardcoverParserTest.java
📚 Learning: 2026-05-04T05:01:33.919Z
Learnt from: imnotjames
Repo: grimmory-tools/grimmory PR: 1081
File: backend/src/test/java/org/booklore/service/metadata/parser/AudibleParserTest.java:132-135
Timestamp: 2026-05-04T05:01:33.919Z
Learning: In grimmory-tools/grimmory unit/integration test classes (e.g., files matching **/*Test.java under backend/src/test/java/**), it is acceptable to directly instantiate Jackson mappers (e.g., `new ObjectMapper()` / `new JsonMapper(...)`) and this should NOT be flagged. The Jackson guidance to use Spring bean injection or `JsonMapper.shared()` applies only to production code; tests may construct dependencies directly as standard practice. Production-code mapper instantiation rules should still be enforced outside test sources.
Applied to files:
backend/src/test/java/org/booklore/service/metadata/parser/HardcoverParserTest.java
imnotjames
changed the title
Contributor
Author
|
Applied and tested rabbits fixes, mostly null handling, biggest error with filterAuthor function, fixed this by flipping the "<" to ">" rather than it's own suggestion which seemed pointlessly long-winded, had to adjust some tests to account for this change and hence returned an empty list rather than previous behaviour which was to return the initially received "hits" . Which should lead to: try title+author -> no author matches -> try title only. where as prior it was just continuing with the same list of hits, rather than trying again, with a title only search ++ Fixed the nitpicks in the tests. ++ Ran one more test with the above changes. where Only searching by title, then following up with searching by, title+author for same 200 library sample.. Key takeaways: total time - 556189ms (2.82s/book), with the initial PR I was getting around 489000ms (2.48s/book), Though I believe that might be accounted for with grabbing covers aswell, None of the recent commits really should have such a significant affect on performance. Which is on-par with the code as I inherited it, if you don't account for the second time one would fetch, where it would grab all new information, totalling about (4s/book), which makes sense, most of the time is api (1 call per second), and both methods required about 2 calls. Now the Isbn search should only require one call, and the title/author still takes 2 calls, though these calls are now more refined, as is the logic within grimmory's code. Looking at about a 15-41% speed increase. meanwhile accuracy is vastly improved where initally i was finding myself with 13 languages and 4 variations of english for my sample, it is now all "en" as expected.. Moreover, 173(/197) of these books sit within 70-79% (good) metadata score, from title search alone
20 of them sit in 50-69% (fair), these fall under a few categorises
Finally there are 4 books in the weak (30-49%)
++ On covers, I didn't want to check, all of them. as this is both tedious and not something that can really be helped, genuinely i think the best fix would just to let some other provider take the wheel for covers. But of 100: 1 was "ugly", 12 were of poor quality, 2 were photographs, and 3 had none. I was also being very generous in saying so. ++ Further notes: somethings i forgot to mention, but there definitely could also be some improvements with differentiating titles and subtitles. I did provide a little bit of logic in this regard, and i think if the "alternative" titles, multiple languages comes into full fruition it would be accounted for here. But it's very much a bit of a rabbit-hole and it's best imo to just get the barebones product released before being too nitpicky. |
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/src/test/java/org/booklore/service/metadata/BookMetadataServiceTest.java (1)
141-149:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate stale ISBN lookup tests to configure
fieldOptions.isbn13, nottitle.Several
LookupByIsbntests still seed provider order viafieldOptions.title(...), but the service now derives the chain fromfieldOptions.isbn13. These tests can pass via fallback and no longer validate intended provider ordering/dedup behavior. Please switch these setups toisbn13(...)(except explicit fallback tests), and keep one dedicated null-edge test forisbn13 == null.As per coding guidelines: "Flag missing edge case coverage: null, empty, boundary values."
Also applies to: 196-205, 219-227, 275-285, 302-312
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/src/test/java/org/booklore/service/metadata/BookMetadataServiceTest.java` around lines 141 - 149, The LookupByIsbn test cases are configuring the provider chain via fieldOptions.title() but the service now derives the provider order from fieldOptions.isbn13(). Update the builder chain in the test setup to call isbn13() instead of title() when configuring the FieldOptions.FieldProvider within the MetadataRefreshOptions configuration. Apply this change to the test methods mentioned (including the diff shown and those at lines 196-205, 219-227, 275-285, 302-312), and add one dedicated test case where isbn13 is left null to ensure edge case coverage for the null scenario.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In
`@backend/src/test/java/org/booklore/service/metadata/BookMetadataServiceTest.java`:
- Around line 141-149: The LookupByIsbn test cases are configuring the provider
chain via fieldOptions.title() but the service now derives the provider order
from fieldOptions.isbn13(). Update the builder chain in the test setup to call
isbn13() instead of title() when configuring the FieldOptions.FieldProvider
within the MetadataRefreshOptions configuration. Apply this change to the test
methods mentioned (including the diff shown and those at lines 196-205, 219-227,
275-285, 302-312), and add one dedicated test case where isbn13 is left null to
ensure edge case coverage for the null scenario.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6a4204f6-61b3-4fd3-9fab-5cc79725e51c
📒 Files selected for processing (2)
backend/src/main/java/org/booklore/service/metadata/BookMetadataService.javabackend/src/test/java/org/booklore/service/metadata/BookMetadataServiceTest.java
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: Test Suite / Backend Tests
🧰 Additional context used
📓 Path-based instructions (6)
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/metadata/BookMetadataService.javabackend/src/test/java/org/booklore/service/metadata/BookMetadataServiceTest.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/metadata/BookMetadataService.javabackend/src/test/java/org/booklore/service/metadata/BookMetadataServiceTest.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/metadata/BookMetadataService.javabackend/src/test/java/org/booklore/service/metadata/BookMetadataServiceTest.java
**/metadata/**/*.java
⚙️ CodeRabbit configuration file
**/metadata/**/*.java: Metadata and Sidecar review:
- Ensure perfect official compliance with Dublin Core and ANSI standards.
- Strictly follow official XML-compliant methods for EPUB2, EPUB3, CBX, and PDF formats.
- Sidecar files must be written in a way that is compatible with widespread e-reader standards.
- Avoid proprietary or non-standard XML tags unless absolutely necessary for internal tracking.
Files:
backend/src/main/java/org/booklore/service/metadata/BookMetadataService.javabackend/src/test/java/org/booklore/service/metadata/BookMetadataServiceTest.java
backend/src/test/**/*.java
📄 CodeRabbit inference engine (AGENTS.md)
Prefer focused unit tests; use
@SpringBootTestonly when the Spring context is required in backend code
Files:
backend/src/test/java/org/booklore/service/metadata/BookMetadataServiceTest.java
**/*Test.java
⚙️ CodeRabbit configuration file
**/*Test.java: Java test review:
- Prefer
@ExtendWith(SpringExtension.class) or@SpringBootTestfor integration tests.- Flag tests with no assertions.
- Flag Thread.sleep() in tests; use Awaitility or virtual-thread-friendly alternatives.
- Flag hardcoded ports or file paths.
- Flag missing edge case coverage: null, empty, boundary values.
- Prefer AssertJ over JUnit's built-in assertions for readability.
- Prefer
@Sqlor Testcontainers for database state; not hand-rolled setup/teardown.
Files:
backend/src/test/java/org/booklore/service/metadata/BookMetadataServiceTest.java
🧠 Learnings (13)
📚 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/metadata/BookMetadataService.javabackend/src/test/java/org/booklore/service/metadata/BookMetadataServiceTest.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/metadata/BookMetadataService.javabackend/src/test/java/org/booklore/service/metadata/BookMetadataServiceTest.java
📚 Learning: 2026-05-07T21:21:55.233Z
Learnt from: imnotjames
Repo: grimmory-tools/grimmory PR: 1194
File: backend/src/main/java/org/booklore/service/ReadingSessionService.java:0-0
Timestamp: 2026-05-07T21:21:55.233Z
Learning: When reviewing Java 23+ code, treat `java.time.Instant#until(Instant endExclusive)` as a valid API/method call (it returns a `Duration`, equivalent to `Duration.between(this, endExclusive)`). Do not flag `instant.until(otherInstant)` as a compile error or API misuse when the project targets Java 25+ (as in grimmory-tools/grimmory); the call should be considered correct and returns a `Duration`.
Applied to files:
backend/src/main/java/org/booklore/service/metadata/BookMetadataService.javabackend/src/test/java/org/booklore/service/metadata/BookMetadataServiceTest.java
📚 Learning: 2026-05-08T06:19:20.621Z
Learnt from: imnotjames
Repo: grimmory-tools/grimmory PR: 1201
File: backend/src/main/java/org/booklore/model/dto/AccessTokenDto.java:3-10
Timestamp: 2026-05-08T06:19:20.621Z
Learning: For Jackson 3 codebases, do not treat imports from `com.fasterxml.jackson.annotation.*` (e.g., `JsonInclude`, `JsonProperty`, `JsonView`) as incorrect. In Jackson 3, `jackson-annotations` intentionally remains under `com.fasterxml.jackson.annotation.*` for backward compatibility, while only the core processing packages (e.g., `jackson-core`, `jackson-databind`) move to the `tools.jackson.*` namespace.
Applied to files:
backend/src/main/java/org/booklore/service/metadata/BookMetadataService.javabackend/src/test/java/org/booklore/service/metadata/BookMetadataServiceTest.java
📚 Learning: 2026-05-04T20:31:11.075Z
Learnt from: imnotjames
Repo: grimmory-tools/grimmory PR: 1086
File: backend/src/main/java/org/booklore/service/metadata/BookReviewUpdateService.java:63-66
Timestamp: 2026-05-04T20:31:11.075Z
Learning: For this repository, reviewers should treat string truncation done via `String.length()` and `String.substring(0, maxLength)` (UTF-16 code units) as an accepted, consistent convention. Do not flag individual occurrences of this pattern as bugs, even though it is not code-point-aware for surrogate pairs. A separate global effort is already tracked to move toward code-point-aware truncation, so per-site fixes should be avoided during code review.
Applied to files:
backend/src/main/java/org/booklore/service/metadata/BookMetadataService.java
📚 Learning: 2026-05-13T12:34:49.607Z
Learnt from: balazs-szucs
Repo: grimmory-tools/grimmory PR: 1293
File: backend/src/main/java/org/booklore/service/metadata/DuckDuckGoCoverService.java:246-252
Timestamp: 2026-05-13T12:34:49.607Z
Learning: In this repo’s Java code, when catching Jsoup `org.jsoup.HttpStatusException` (and similar exceptions originating from external libraries) and wrapping/rethrowing them, do not require preserving the original exception stack trace (e.g., as flagged by PMD `PreserveStackTrace`) as long as the application already captures the actionable diagnostics in logs or the thrown exception message (such as HTTP status code and the requested URL). Reviewers should still ensure the log/message contains those details; the intent is to avoid noisy stack traces that only reflect external-library internals rather than application code.
Applied to files:
backend/src/main/java/org/booklore/service/metadata/BookMetadataService.java
📚 Learning: 2026-05-17T13:38:16.462Z
Learnt from: balazs-szucs
Repo: grimmory-tools/grimmory PR: 1366
File: backend/src/main/java/org/booklore/service/FileStreamingService.java:65-66
Timestamp: 2026-05-17T13:38:16.462Z
Learning: In the grimmory-tools/grimmory repo, it’s an accepted pattern to pass the raw AccessDeniedException.getMessage() (even if it may include filesystem path details) into ApiError.PERMISSION_DENIED.createException(...). During code review, do not raise a security/information-disclosure issue solely based on that exception message being propagated to the API when using ApiError.PERMISSION_DENIED.createException with the AccessDeniedException message.
Applied to files:
backend/src/main/java/org/booklore/service/metadata/BookMetadataService.java
📚 Learning: 2026-05-23T23:01:25.769Z
Learnt from: imnotjames
Repo: grimmory-tools/grimmory PR: 1456
File: backend/src/main/java/org/booklore/service/metadata/parser/GoodReadsParser.java:618-630
Timestamp: 2026-05-23T23:01:25.769Z
Learning: In this codebase (grimmory-tools/grimmory), it’s intentional to omit per-request timeouts on individual Java HttpRequest.Builder instances (e.g., GoodReadsParser.fetchJson). During reviews, do not flag missing builder-level timeouts as a best-practice violation; rely on framework-level and/or HttpClient-level timeouts configured elsewhere for consistent behavior. Only raise an issue if you can verify that no effective timeout is configured at the HttpClient/framework level.
Applied to files:
backend/src/main/java/org/booklore/service/metadata/BookMetadataService.java
📚 Learning: 2026-06-12T01:10:31.416Z
Learnt from: imnotjames
Repo: grimmory-tools/grimmory PR: 1724
File: backend/src/main/java/org/booklore/repository/BookRepository.java:58-59
Timestamp: 2026-06-12T01:10:31.416Z
Learning: In this codebase (grimmory-tools/grimmory), reviews should not treat inline `LIMIT`/`OFFSET` clauses inside `Query` JPQL/HQL strings as a JPA compliance risk. This is intentional: `hibernate.jpa.compliance.query=true` is intentionally not set, and Hibernate 7.3+ supports `LIMIT`/`OFFSET` as valid HQL extensions. Therefore, do not flag or require changes to `Query` annotations solely due to `LIMIT`/`OFFSET` usage.
Applied to files:
backend/src/main/java/org/booklore/service/metadata/BookMetadataService.java
📚 Learning: 2026-05-07T21:37:46.988Z
Learnt from: imnotjames
Repo: grimmory-tools/grimmory PR: 1194
File: backend/src/main/java/org/booklore/service/ReadingSessionService.java:121-123
Timestamp: 2026-05-07T21:37:46.988Z
Learning: In grimmory-tools/grimmory service-layer code, if arithmetic overflow occurs inside inference/business-logic (e.g., when deriving inferred fields like durationSeconds from start/end timestamps), treat it as a server-side anomaly. Prefer letting the global exception handler translate it into a generic 5xx response rather than throwing an explicit ApiError 4xx (e.g., do not convert overflow into a client error).
Applied to files:
backend/src/main/java/org/booklore/service/metadata/BookMetadataService.java
📚 Learning: 2026-04-27T15:25:55.042Z
Learnt from: imnotjames
Repo: grimmory-tools/grimmory PR: 930
File: backend/src/test/java/org/booklore/service/metadata/parser/ComicvineBookParserTest.java:68-75
Timestamp: 2026-04-27T15:25:55.042Z
Learning: In this repository’s JUnit 5 test sources (e.g., under backend/src/test/java/), do not flag “bare” `assert` statements as a bug. The project test runner is configured to always execute tests with assertions enabled (e.g., `-ea`), so `assert` behavior is consistent. Continue to review for correctness, but don’t treat unguarded `assert` usage in test classes as a static-analysis issue.
Applied to files:
backend/src/test/java/org/booklore/service/metadata/BookMetadataServiceTest.java
📚 Learning: 2026-05-22T03:20:45.559Z
Learnt from: imnotjames
Repo: grimmory-tools/grimmory PR: 1446
File: backend/src/test/java/org/booklore/service/metadata/MetadataManagementServiceTest.java:465-470
Timestamp: 2026-05-22T03:20:45.559Z
Learning: In backend unit tests (Java files under backend/src/test/java), do not flag hardcoded placeholder filesystem paths (e.g., setting LibraryPathEntity.path = "/example") as violations when the value is intentionally unused for filesystem access. Only suppress the "no hardcoded paths" concern if the test does not perform any filesystem/network IO using that path (no reads/writes/Files.* calls or code paths that access the filesystem with that value); if the placeholder is actually used to touch the filesystem, flag it.
Applied to files:
backend/src/test/java/org/booklore/service/metadata/BookMetadataServiceTest.java
📚 Learning: 2026-05-04T05:01:33.919Z
Learnt from: imnotjames
Repo: grimmory-tools/grimmory PR: 1081
File: backend/src/test/java/org/booklore/service/metadata/parser/AudibleParserTest.java:132-135
Timestamp: 2026-05-04T05:01:33.919Z
Learning: In grimmory-tools/grimmory unit/integration test classes (e.g., files matching **/*Test.java under backend/src/test/java/**), it is acceptable to directly instantiate Jackson mappers (e.g., `new ObjectMapper()` / `new JsonMapper(...)`) and this should NOT be flagged. The Jackson guidance to use Spring bean injection or `JsonMapper.shared()` applies only to production code; tests may construct dependencies directly as standard practice. Production-code mapper instantiation rules should still be enforced outside test sources.
Applied to files:
backend/src/test/java/org/booklore/service/metadata/BookMetadataServiceTest.java
🔇 Additional comments (1)
backend/src/main/java/org/booklore/service/metadata/BookMetadataService.java (1)
14-22: LGTM!Also applies to: 39-42, 137-141
imnotjames
reviewed
Jun 19, 2026
Contributor
There was a problem hiding this comment.
Is it possible to remove the whitespace changes from BookCategory?
Contributor
Author
There was a problem hiding this comment.
File should just be deleted, no? was a remanent of something that didn't pass a preliminary check
Comment on lines
-21
to
+22
| private static final long INITIAL_DELAY_MS = 1200; | ||
| private static final long INITIAL_DELAY_MS = 1000; |
Contributor
Author
There was a problem hiding this comment.
It was my understanding this related to the rate-limiting.
"API calls are rate-limited to 60 requests per minute."
https://docs.hardcover.app/api/getting-started/
| titleProvider.getP1(), titleProvider.getP2(), | ||
| titleProvider.getP3(), titleProvider.getP4()) | ||
| isbnProvider.getP1(), isbnProvider.getP2(), | ||
| isbnProvider.getP3(), isbnProvider.getP4()) |
Contributor
There was a problem hiding this comment.
Is this change required for the hardcover side of things? Could it be brought in separately?
Contributor
Author
There was a problem hiding this comment.
Required is a stretch, but it definitely is needed to get the best out of Hardcover as is, and makes sense in the grand scheme of things. happy to apply it elsewhere but is required ASAP in my opinon
Contributor
There was a problem hiding this comment.
Doesn't this affect other providers rather than just hardcover, though? Will it negatively affect providers that don't pull ISBN?
I suggest carving it out into its own PR.
Co-authored-by: James Ward <james@notjam.es>
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/src/main/java/org/booklore/service/metadata/parser/hardcover/HardcoverBookSearchService.java (1)
215-218:⚠️ Potential issue | 🟡 MinorUse
Duration-based sleep in the service rate-limiter.
Thread.sleep(long)violates the service-layer guideline. Refactor to use theDurationoverload and add the necessary import.Proposed fix
+import java.time.Duration; ... - Thread.sleep(delay - elapsed); + Thread.sleep(Duration.ofMillis(delay - elapsed));🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/src/main/java/org/booklore/service/metadata/parser/hardcover/HardcoverBookSearchService.java` around lines 215 - 218, In the HardcoverBookSearchService class, the rate-limiter logic is using the long-based Thread.sleep() overload which violates service-layer guidelines. Replace the Thread.sleep(delay - elapsed) call with Thread.sleep(Duration.ofMillis(delay - elapsed)) to use the Duration-based overload instead. Add the necessary import statement for java.time.Duration at the top of the file.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In
`@backend/src/main/java/org/booklore/service/metadata/parser/hardcover/HardcoverBookSearchService.java`:
- Around line 215-218: In the HardcoverBookSearchService class, the rate-limiter
logic is using the long-based Thread.sleep() overload which violates
service-layer guidelines. Replace the Thread.sleep(delay - elapsed) call with
Thread.sleep(Duration.ofMillis(delay - elapsed)) to use the Duration-based
overload instead. Add the necessary import statement for java.time.Duration at
the top of the file.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: 049f749b-ce19-4863-a390-897a53456e1c
📒 Files selected for processing (1)
backend/src/main/java/org/booklore/service/metadata/parser/hardcover/HardcoverBookSearchService.java
📜 Review details
⏰ Context from checks skipped due to timeout. (5)
- GitHub Check: Test Suite / Frontend Tests
- GitHub Check: Test Suite / Backend Tests
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (java-kotlin)
- GitHub Check: Frontend Lint Threshold Check
🧰 Additional context used
📓 Path-based instructions (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/metadata/parser/hardcover/HardcoverBookSearchService.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/metadata/parser/hardcover/HardcoverBookSearchService.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/metadata/parser/hardcover/HardcoverBookSearchService.java
**/metadata/**/*.java
⚙️ CodeRabbit configuration file
**/metadata/**/*.java: Metadata and Sidecar review:
- Ensure perfect official compliance with Dublin Core and ANSI standards.
- Strictly follow official XML-compliant methods for EPUB2, EPUB3, CBX, and PDF formats.
- Sidecar files must be written in a way that is compatible with widespread e-reader standards.
- Avoid proprietary or non-standard XML tags unless absolutely necessary for internal tracking.
Files:
backend/src/main/java/org/booklore/service/metadata/parser/hardcover/HardcoverBookSearchService.java
🧠 Learnings (10)
📚 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/metadata/parser/hardcover/HardcoverBookSearchService.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/metadata/parser/hardcover/HardcoverBookSearchService.java
📚 Learning: 2026-05-07T21:21:55.233Z
Learnt from: imnotjames
Repo: grimmory-tools/grimmory PR: 1194
File: backend/src/main/java/org/booklore/service/ReadingSessionService.java:0-0
Timestamp: 2026-05-07T21:21:55.233Z
Learning: When reviewing Java 23+ code, treat `java.time.Instant#until(Instant endExclusive)` as a valid API/method call (it returns a `Duration`, equivalent to `Duration.between(this, endExclusive)`). Do not flag `instant.until(otherInstant)` as a compile error or API misuse when the project targets Java 25+ (as in grimmory-tools/grimmory); the call should be considered correct and returns a `Duration`.
Applied to files:
backend/src/main/java/org/booklore/service/metadata/parser/hardcover/HardcoverBookSearchService.java
📚 Learning: 2026-05-08T06:19:20.621Z
Learnt from: imnotjames
Repo: grimmory-tools/grimmory PR: 1201
File: backend/src/main/java/org/booklore/model/dto/AccessTokenDto.java:3-10
Timestamp: 2026-05-08T06:19:20.621Z
Learning: For Jackson 3 codebases, do not treat imports from `com.fasterxml.jackson.annotation.*` (e.g., `JsonInclude`, `JsonProperty`, `JsonView`) as incorrect. In Jackson 3, `jackson-annotations` intentionally remains under `com.fasterxml.jackson.annotation.*` for backward compatibility, while only the core processing packages (e.g., `jackson-core`, `jackson-databind`) move to the `tools.jackson.*` namespace.
Applied to files:
backend/src/main/java/org/booklore/service/metadata/parser/hardcover/HardcoverBookSearchService.java
📚 Learning: 2026-05-04T20:31:11.075Z
Learnt from: imnotjames
Repo: grimmory-tools/grimmory PR: 1086
File: backend/src/main/java/org/booklore/service/metadata/BookReviewUpdateService.java:63-66
Timestamp: 2026-05-04T20:31:11.075Z
Learning: For this repository, reviewers should treat string truncation done via `String.length()` and `String.substring(0, maxLength)` (UTF-16 code units) as an accepted, consistent convention. Do not flag individual occurrences of this pattern as bugs, even though it is not code-point-aware for surrogate pairs. A separate global effort is already tracked to move toward code-point-aware truncation, so per-site fixes should be avoided during code review.
Applied to files:
backend/src/main/java/org/booklore/service/metadata/parser/hardcover/HardcoverBookSearchService.java
📚 Learning: 2026-05-13T12:34:49.607Z
Learnt from: balazs-szucs
Repo: grimmory-tools/grimmory PR: 1293
File: backend/src/main/java/org/booklore/service/metadata/DuckDuckGoCoverService.java:246-252
Timestamp: 2026-05-13T12:34:49.607Z
Learning: In this repo’s Java code, when catching Jsoup `org.jsoup.HttpStatusException` (and similar exceptions originating from external libraries) and wrapping/rethrowing them, do not require preserving the original exception stack trace (e.g., as flagged by PMD `PreserveStackTrace`) as long as the application already captures the actionable diagnostics in logs or the thrown exception message (such as HTTP status code and the requested URL). Reviewers should still ensure the log/message contains those details; the intent is to avoid noisy stack traces that only reflect external-library internals rather than application code.
Applied to files:
backend/src/main/java/org/booklore/service/metadata/parser/hardcover/HardcoverBookSearchService.java
📚 Learning: 2026-05-17T13:38:16.462Z
Learnt from: balazs-szucs
Repo: grimmory-tools/grimmory PR: 1366
File: backend/src/main/java/org/booklore/service/FileStreamingService.java:65-66
Timestamp: 2026-05-17T13:38:16.462Z
Learning: In the grimmory-tools/grimmory repo, it’s an accepted pattern to pass the raw AccessDeniedException.getMessage() (even if it may include filesystem path details) into ApiError.PERMISSION_DENIED.createException(...). During code review, do not raise a security/information-disclosure issue solely based on that exception message being propagated to the API when using ApiError.PERMISSION_DENIED.createException with the AccessDeniedException message.
Applied to files:
backend/src/main/java/org/booklore/service/metadata/parser/hardcover/HardcoverBookSearchService.java
📚 Learning: 2026-05-23T23:01:25.769Z
Learnt from: imnotjames
Repo: grimmory-tools/grimmory PR: 1456
File: backend/src/main/java/org/booklore/service/metadata/parser/GoodReadsParser.java:618-630
Timestamp: 2026-05-23T23:01:25.769Z
Learning: In this codebase (grimmory-tools/grimmory), it’s intentional to omit per-request timeouts on individual Java HttpRequest.Builder instances (e.g., GoodReadsParser.fetchJson). During reviews, do not flag missing builder-level timeouts as a best-practice violation; rely on framework-level and/or HttpClient-level timeouts configured elsewhere for consistent behavior. Only raise an issue if you can verify that no effective timeout is configured at the HttpClient/framework level.
Applied to files:
backend/src/main/java/org/booklore/service/metadata/parser/hardcover/HardcoverBookSearchService.java
📚 Learning: 2026-06-12T01:10:31.416Z
Learnt from: imnotjames
Repo: grimmory-tools/grimmory PR: 1724
File: backend/src/main/java/org/booklore/repository/BookRepository.java:58-59
Timestamp: 2026-06-12T01:10:31.416Z
Learning: In this codebase (grimmory-tools/grimmory), reviews should not treat inline `LIMIT`/`OFFSET` clauses inside `Query` JPQL/HQL strings as a JPA compliance risk. This is intentional: `hibernate.jpa.compliance.query=true` is intentionally not set, and Hibernate 7.3+ supports `LIMIT`/`OFFSET` as valid HQL extensions. Therefore, do not flag or require changes to `Query` annotations solely due to `LIMIT`/`OFFSET` usage.
Applied to files:
backend/src/main/java/org/booklore/service/metadata/parser/hardcover/HardcoverBookSearchService.java
📚 Learning: 2026-05-07T21:37:46.988Z
Learnt from: imnotjames
Repo: grimmory-tools/grimmory PR: 1194
File: backend/src/main/java/org/booklore/service/ReadingSessionService.java:121-123
Timestamp: 2026-05-07T21:37:46.988Z
Learning: In grimmory-tools/grimmory service-layer code, if arithmetic overflow occurs inside inference/business-logic (e.g., when deriving inferred fields like durationSeconds from start/end timestamps), treat it as a server-side anomaly. Prefer letting the global exception handler translate it into a generic 5xx response rather than throwing an explicit ApiError 4xx (e.g., do not convert overflow into a client error).
Applied to files:
backend/src/main/java/org/booklore/service/metadata/parser/hardcover/HardcoverBookSearchService.java
🔇 Additional comments (1)
backend/src/main/java/org/booklore/service/metadata/parser/hardcover/HardcoverBookSearchService.java (1)
22-172: LGTM!Also applies to: 224-239
imnotjames
reviewed
Jun 23, 2026
imnotjames
left a comment
imnotjames
left a comment
Contributor
There was a problem hiding this comment.
A big part of my comments have to do with reworking the code to avoid creating methods that cause side effects by updating objects in place and instead try to use the various stream APIs.
Comment on lines
+82
to
+84
| if (metadata != null) { | ||
| results.add(metadata); | ||
| } |
Contributor
There was a problem hiding this comment.
This can never be null.
Comment on lines
+76
to
+81
| BookMetadata metadata = new BookMetadata(); | ||
| log.debug("Processing edition '{}' with id '{}' of book '{}'", book.getEditions().getFirst().getTitle(), | ||
| book.getEditions().getFirst().getId(), | ||
| book.getTitle()); | ||
| mapBookToMetadata(book, book.getEditions().getFirst(), metadata); | ||
|
|
Contributor
There was a problem hiding this comment.
You probably can move the book metadata creation into the mapBookToMetadata helper.
private BookMetadata toMetadata(GraphQLResponse.BookWithEditions book, GraphQLResponse.Edition edition) {
var builder = BookMetadata.builder()
.hardcoverId(book.getSlug());
// etc
return builder.build();
}
Then the mapping helpers just need to accept the builder class instead if you still want them all split up like that, and they can have void returns.
| return docs.stream() | ||
| .sorted(Comparator.comparingDouble(GraphQLResponse.Document::getLevenshteinDistanceTitle)) | ||
| .toList(); | ||
| } |
Contributor
There was a problem hiding this comment.
Is it possible to do this by using predicates/functions and a single stream instead of multiple array lists? We could also avoid using the GraphQLResponse document holding state by creating a helper record in this class.
eg, a private record in the HardcoverParser
record ScoredDocument (
GraphQLResponse.Document document,
double score
) {}We'd update the filter to provide a predicate:
private Function<GraphQLResponse.Document, ScoredDocument> mapTitleLevenshtein(String searchTitle) {
LevenshteinDistance levenshtein = LevenshteinDistance.getDefaultInstance();
String searchTitleLower = searchTitle.toLowerCase();
return (GraphQLResponse.Document doc) -> {
if (doc.getTitle() == null || doc.getTitle().isBlank()) {
return new ScoredDocument(doc, Double.MAX_VALUE);
}
// calculate the lev.dist for the Work-level title and the search provided term
double totalScore = levenshtein.apply(searchTitleLower, doc.getTitle().toLowerCase());
if (doc.getAlternativeTitles() != null) {
//repeat the lev.dist calc for each of the alternative titles
totalScore += doc.getAlternativeTitles()
.stream()
.map(String::toLowerCase)
.map(title -> levenshtein.apply(searchTitleLower, title))
.min(Double::compare)
.orElse(Integer.MAX_VALUE);
}
// (minTitleDist+minAltTitleDist)/1+userCount. best scores should approach 0
int usersCount = doc.getUsersCount() == null ? 0 : doc.getUsersCount();
return new ScoredDocument(doc, (totalScore + 1)/(usersCount + 1));
};
}
and then finally..
private List<GraphQLResponse.Document> filterSearch(List<GraphQLResponse.Hit> hits, FetchMetadataRequest request) {
if (hits == null || hits.isEmpty()) {
log.info("Hardcover: No results found for title '{}'", request.getTitle());
return Collections.emptyList();
}
String searchTitle = request.getTitle() != null ? request.getTitle() : "";
String searchAuthor = request.getAuthor() != null ? formatRequestAuthor(request.getAuthor()) : "";
//convert hits into document format.
Stream<GraphQLResponse.Document> docs = hits.stream()
.map(GraphQLResponse.Hit::getDocument)
.filter(Objects::nonNull);
log.debug("Filtering by title: '{}', author: '{}'", searchTitle, searchAuthor);
if (!searchTitle.isBlank()) {
docs = docs
.map(mapTitleLevenshtein(searchTitle))
.sorted(Comparator.comparingDouble(ScoredDocument::score))
.map(ScoredDocument::document);
log.debug("Filtered by title: {}", searchTitle);
}
// Similar with the author - different because it uses "max" distance as part of a filter?
return docs.toList();
}
Happy to chat through this more, but I would suggest similar throughout this change set to make for an easier to follow code path.
| filterEditionsByLanguage(results, result); | ||
| } | ||
| return results; | ||
| } |
Contributor
There was a problem hiding this comment.
This could be more cleanly defined as a map over a stream rather than relying on mutations of objects we're passing into the functions, right?
| titleProvider.getP1(), titleProvider.getP2(), | ||
| titleProvider.getP3(), titleProvider.getP4()) | ||
| isbnProvider.getP1(), isbnProvider.getP2(), | ||
| isbnProvider.getP3(), isbnProvider.getP4()) |
Contributor
There was a problem hiding this comment.
Doesn't this affect other providers rather than just hardcover, though? Will it negatively affect providers that don't pull ISBN?
I suggest carving it out into its own PR.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Improve overall functionality of the hardcover parser. For faster and more consistent, reliable results, and fixes issue with
hardcover reading tracker
Linked Issue
#1284
Changes
Manual Testing Steps
Additional Context (Optional)
This is a good start in regards to the general improvements here and the feasibility of using hardcover as p1 provider, and also introducing "guardrails." However there are still inadequacies mostly on the end of hc's data. But Specifically regarding these changes. I think extra thought should go toward the filtering editions section...
Using the user's locale is not the be all and end all to searching languages, and their will be plenty of edge cases. Looking at manga. Where this method isn't sufficient.
Furthermore, The use of alternative titles, and matching therein, does work wholly. It isn't so straightforward after this point, to return the given title and potential foreign-language/edition specific results, like the description. There is some work to weight this up between hc md quality, and
With the audiobook working via the primary file only it doesn't register when the audiobook is there, but not a primary file. this isn't a massive to fix, but again requires some further consideration with implementation
AI Disclosure
Qwen-coder: local model for checking over my code.
Checklist
just ui checkandjust api check.Summary by CodeRabbit