refactor(kobo): implement Grimmory kepub transformation - #2002
Draft
imnotjames wants to merge 22 commits into
Draft
refactor(kobo): implement Grimmory kepub transformation#2002imnotjames wants to merge 22 commits into
imnotjames wants to merge 22 commits into
Conversation
Contributor
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: trueWalkthroughThe PR replaces external ChangesIn-process kepub conversion
Runtime media tooling
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested labels: Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant EPUBInput
participant EpubReader
participant KepubConversionService
participant KepubHtmlConversionService
participant EpubWriter
EPUBInput->>EpubReader: read EPUB stream
EpubReader->>KepubConversionService: return Book
KepubConversionService->>KepubHtmlConversionService: transform HTML/XHTML resources
KepubHtmlConversionService-->>KepubConversionService: return Kobo XHTML
KepubConversionService->>EpubWriter: write converted Book
EpubWriter-->>EPUBInput: produce kepub EPUB output
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
imnotjames
force-pushed
the
refactor/-/kepubify
branch
from
July 14, 2026 19:48
a732ab5 to
45eb987
Compare
![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/kobo/KepubConversionService.java`:
- Around line 328-350: Update the XHTML conversion flow around the document
created in KepubConversionService so the serialized root html element always
declares xmlns="http://www.w3.org/1999/xhtml", and normalize embedded SVG
namespace declarations to their official namespace before creating the Resource.
Preserve the existing XML syntax, UTF-8 encoding, and application/xhtml+xml
output.
- Around line 160-175: Update the Integrator in KepubConversionService so it
evaluates whether the current element ends the sentence before
state.append(element). After punctuation, flush the existing state when the next
element is a non-whitespace, non-extra character, then begin the new sentence
with that element; preserve existing handling for whitespace and
SENTENCE_EXTRA_CHARS.
- Around line 282-291: Update transformContentAddWrappers to move every node
from document.body(), including text, comments, and other non-element nodes,
into `#book-inner` by using the body’s childNodes rather than only its element
children. Preserve the existing Kobo wrapper hierarchy and append the resulting
`#book-columns` to the body.
- Around line 513-531: The overloaded convertEpubToKepub(File epubFile, File
tempDir, boolean forceEnableHyphenation) must create its output file within
tempDir instead of delegating to the system-temp overload. Refactor the shared
conversion flow so the supplied tempDir is passed to
Files.createTempFile(tempDir.toPath(), ...) while preserving validation,
conversion, logging, and return behavior.
- Around line 113-120: Update KepubConversionService so EpubWriter is not stored
and reused as a singleton field. Create a fresh EpubWriter for each conversion,
or inject a factory that produces one per conversion, while keeping the existing
EpubReader handling unchanged.
- Around line 495-500: Update the conversion flow around transformOPF and
EpubWriter.write to handle a null original.getCoverImage() without dereferencing
it, and remove the unnecessary kepub.setOpfResource call since EpubWriter.write
regenerates content.opf from the Book state.
🪄 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: 1b5b3fc4-b477-4906-a7df-95445a1743c7
📒 Files selected for processing (2)
Dockerfilebackend/src/main/java/org/booklore/service/kobo/KepubConversionService.java
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
grimmory-tools/grimmory-docs(manual)
💤 Files with no reviewable changes (1)
- Dockerfile
📜 Review details
🧰 Additional context used
📓 Path-based instructions (3)
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/kobo/KepubConversionService.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/kobo/KepubConversionService.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/kobo/KepubConversionService.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/kobo/KepubConversionService.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/kobo/KepubConversionService.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/kobo/KepubConversionService.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/kobo/KepubConversionService.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/kobo/KepubConversionService.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/kobo/KepubConversionService.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/kobo/KepubConversionService.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/kobo/KepubConversionService.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/kobo/KepubConversionService.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/kobo/KepubConversionService.java
🪛 ast-grep (0.44.1)
backend/src/main/java/org/booklore/service/kobo/KepubConversionService.java
[warning] 515-515: Temporary file not deleted
Context: Files.createTempFile("grimmory-kepub-", ".kepub.epub")
Note: [CWE-377] Insecure Temporary File. Security best practice.
(tempfile-delete)
[warning] 416-416: TransformerFactory used without secure processing is vulnerable to XXE
Context: TransformerFactory.newInstance().newTransformer()
Note: [CWE-611] Improper Restriction of XML External Entity Reference.
(xml-parsing-xxe-transformer)
imnotjames
changed the title
imnotjames
changed the title
imnotjames
force-pushed
the
refactor/-/kepubify
branch
2 times, most recently
from
July 26, 2026 23:26
b786629 to
d82f18e
Compare
imnotjames
marked this pull request as ready for review
July 26, 2026 23:39
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Dockerfile (1)
50-52: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winPin the Dockerfile ffprobe image immutably.
mwader/static-ffmpeg:8.1is a moving image tag. Use a specific patch release plus its confirmed multi-architecture manifest digest, e.g.mwader/static-ffmpeg:8.1.2@sha256:<manifest-digest>, so rebuilds cannot switch ffprobe binaries silently.🤖 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 `@Dockerfile` around lines 50 - 52, Update the ffprobe-layer base image reference from the moving mwader/static-ffmpeg:8.1 tag to the confirmed immutable patch-version tag and multi-architecture manifest digest, using the required image@sha256 format while leaving the eclipse-temurin stage unchanged.Source: Path instructions
🤖 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/kobo/KepubConversionService.java`:
- Around line 275-279: Update KepubConversionService.validateInputs to throw the
project-standard exception created through ApiError.createException(...) instead
of IllegalArgumentException, while preserving the existing invalid-input
condition and path value in the error context.
- Around line 275-279: Update validateInputs to validate the filename’s suffix
rather than calling Path.endsWith(".epub"). Preserve the existing null and
regular-file checks, and accept valid filenames whose final name ends with the
EPUB extension.
- Around line 201-228: Update convertBookToKepub so navResource and ncxResource
pass through getTransformedContentResource with forceEnableHyphenation before
being assigned. Add each transformed resource to kepub explicitly by its
href/id, avoiding duplicate entries while preserving the existing navigation and
NCX associations.
In
`@backend/src/main/java/org/booklore/service/kobo/KepubHtmlConversionService.java`:
- Around line 264-266: Track the outstanding cleanup in
KepubHtmlConversionService by creating an issue or task for removing invalid
UTF-8 replacement characters (�) and empty MSWord o:p/st1:* tags; do not leave
this as an untracked TODO.
- Around line 177-196: Update the image/SVG span creation branch in the
node-processing logic to generate its ID with ID_FORMAT_KOBO_SPAN via the same
String.format pattern used for sentence spans, replacing the hardcoded "kobo."
concatenation while preserving the existing counter.
- Around line 61-74: Update SENTENCE_PUNCTUATION and SENTENCE_EXTRA_CHARS in
KepubHtmlConversionService to be static final fields, keeping their immutable
Set.of contents unchanged.
- Around line 192-201: Add test coverage for the conversion logic around the
img/svg handling in KepubHtmlConversionService: verify body-level img and svg
elements are wrapped in generated koboSpan elements, and repeat conversion to
confirm the Kobo span mapping remains correct and stable.
In
`@backend/src/test/java/org/booklore/service/kobo/KepubConversionServiceTest.java`:
- Around line 1-133: Add tests covering the file-based convertEpubToKepub(File,
File, boolean) entrypoint, including validateInputs, using real temporary
input/output paths with an input filename ending in .epub and a real temporary
directory. Verify valid files reach conversion successfully and add assertions
for invalid input/output paths so the filename-extension and temporary-directory
validation behavior is exercised.
- Around line 38-50: Update convertEpubToKepub_WithValidEpub_ShouldConvert to
assert the expected successful conversion behavior, using Mockito verification
of epubReader.readEpub and the relevant epubWriter interaction. Keep the test
focused on the valid EPUB path and ensure it fails if conversion no longer
invokes the expected collaborators.
---
Outside diff comments:
In `@Dockerfile`:
- Around line 50-52: Update the ffprobe-layer base image reference from the
moving mwader/static-ffmpeg:8.1 tag to the confirmed immutable patch-version tag
and multi-architecture manifest digest, using the required image@sha256 format
while leaving the eclipse-temurin stage unchanged.
🪄 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 Plus
Run ID: ff0f7355-99e8-44cc-b72a-fb7317bbc890
📒 Files selected for processing (5)
Dockerfilebackend/src/main/java/org/booklore/service/kobo/KepubConversionService.javabackend/src/main/java/org/booklore/service/kobo/KepubHtmlConversionService.javabackend/src/test/java/org/booklore/service/kobo/KepubConversionServiceTest.javabackend/src/test/java/org/booklore/service/kobo/KepubHtmlConversionServiceTest.java
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
grimmory-tools/grimmory-docs(manual)
📜 Review details
🧰 Additional context used
📓 Path-based instructions (4)
**/*
⚙️ 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/kobo/KepubConversionServiceTest.javabackend/src/test/java/org/booklore/service/kobo/KepubHtmlConversionServiceTest.javaDockerfilebackend/src/main/java/org/booklore/service/kobo/KepubHtmlConversionService.javabackend/src/main/java/org/booklore/service/kobo/KepubConversionService.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/kobo/KepubConversionServiceTest.javabackend/src/test/java/org/booklore/service/kobo/KepubHtmlConversionServiceTest.javabackend/src/main/java/org/booklore/service/kobo/KepubHtmlConversionService.javabackend/src/main/java/org/booklore/service/kobo/KepubConversionService.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/kobo/KepubConversionServiceTest.javabackend/src/test/java/org/booklore/service/kobo/KepubHtmlConversionServiceTest.java
**/Dockerfile*
⚙️ CodeRabbit configuration file
**/Dockerfile*: Dockerfile review:
- Prefer multi-stage builds (builder + runtime).
- Use non-root USER in the final stage.
- Pin base image versions (e.g., eclipse-temurin:25-jre-noble).
- Flag secrets or credentials embedded in the image.
- Ensure --enable-preview JVM flag is carried into ENTRYPOINT/CMD if required at runtime.
Files:
Dockerfile
🧠 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/kobo/KepubConversionServiceTest.javabackend/src/test/java/org/booklore/service/kobo/KepubHtmlConversionServiceTest.javabackend/src/main/java/org/booklore/service/kobo/KepubHtmlConversionService.javabackend/src/main/java/org/booklore/service/kobo/KepubConversionService.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/kobo/KepubConversionServiceTest.javabackend/src/test/java/org/booklore/service/kobo/KepubHtmlConversionServiceTest.javabackend/src/main/java/org/booklore/service/kobo/KepubHtmlConversionService.javabackend/src/main/java/org/booklore/service/kobo/KepubConversionService.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/kobo/KepubConversionServiceTest.javabackend/src/test/java/org/booklore/service/kobo/KepubHtmlConversionServiceTest.javabackend/src/main/java/org/booklore/service/kobo/KepubHtmlConversionService.javabackend/src/main/java/org/booklore/service/kobo/KepubConversionService.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/kobo/KepubConversionServiceTest.javabackend/src/test/java/org/booklore/service/kobo/KepubHtmlConversionServiceTest.javabackend/src/main/java/org/booklore/service/kobo/KepubHtmlConversionService.javabackend/src/main/java/org/booklore/service/kobo/KepubConversionService.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/kobo/KepubConversionServiceTest.javabackend/src/test/java/org/booklore/service/kobo/KepubHtmlConversionServiceTest.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/kobo/KepubConversionServiceTest.javabackend/src/test/java/org/booklore/service/kobo/KepubHtmlConversionServiceTest.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/kobo/KepubConversionServiceTest.javabackend/src/test/java/org/booklore/service/kobo/KepubHtmlConversionServiceTest.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/kobo/KepubHtmlConversionService.javabackend/src/main/java/org/booklore/service/kobo/KepubConversionService.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/kobo/KepubHtmlConversionService.javabackend/src/main/java/org/booklore/service/kobo/KepubConversionService.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/kobo/KepubHtmlConversionService.javabackend/src/main/java/org/booklore/service/kobo/KepubConversionService.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/kobo/KepubHtmlConversionService.javabackend/src/main/java/org/booklore/service/kobo/KepubConversionService.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/kobo/KepubHtmlConversionService.javabackend/src/main/java/org/booklore/service/kobo/KepubConversionService.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/kobo/KepubHtmlConversionService.javabackend/src/main/java/org/booklore/service/kobo/KepubConversionService.java
🪛 ast-grep (0.44.1)
backend/src/test/java/org/booklore/service/kobo/KepubHtmlConversionServiceTest.java
[warning] 59-64: Regular expression is compiled from a non-literal, possibly user-controlled value. A crafted regex (or input matched against one) can trigger catastrophic backtracking and hang the thread (ReDoS). Use a hardcoded literal pattern, wrap untrusted text with Pattern.quote(...), or validate/length-limit the input and enforce a matching timeout before passing it to Pattern.compile / String.matches / String.replaceAll / String.replaceFirst.
Context: assertThat(actual).matches(
Pattern.compile(
".\s<div id="book-columns">\s*<div id="book-inner">.*",
Pattern.DOTALL | Pattern.MULTILINE
)
)
Note: [CWE-1333] Inefficient Regular Expression Complexity.
(redos-non-literal-regex-java)
backend/src/main/java/org/booklore/service/kobo/KepubConversionService.java
[warning] 269-269: Temporary file not deleted
Context: Files.createTempFile(tempDir.getPath(), ".kepub.epub")
Note: [CWE-377] Insecure Temporary File. Security best practice.
(tempfile-delete)
[warning] 155-155: TransformerFactory used without secure processing is vulnerable to XXE
Context: TransformerFactory.newInstance().newTransformer()
Note: [CWE-611] Improper Restriction of XML External Entity Reference.
(xml-parsing-xxe-transformer)
🔇 Additional comments (10)
Dockerfile (4)
8-8: LGTM!
19-19: LGTM!
81-81: LGTM!
101-104: LGTM!backend/src/main/java/org/booklore/service/kobo/KepubConversionService.java (3)
269-273: 🗄️ Data Integrity & IntegrationTemp output file still isn't created inside
tempDir— wrongcreateTempFileoverload used.
tempDir.getPath()returns aString, so this call resolves toFiles.createTempFile(String prefix, String suffix, FileAttribute<?>... attrs)— the system default temp dir overload — notcreateTempFile(Path dir, String prefix, String suffix, ...). The caller-suppliedtempDiris not honored (its full path string is instead used as an oddly-formed prefix), so this still doesn't fix the previously-flagged issue of writing outside the caller-managed temp directory.🐛 Proposed fix
public File convertEpubToKepub(File epubFile, File tempDir, boolean forceEnableHyphenation) throws IOException { - var outputPath = Files.createTempFile(tempDir.getPath(), ".kepub.epub"); + var outputPath = Files.createTempFile(tempDir.toPath(), "grimmory-kepub-", ".kepub.epub"); convertEpubToKepub(epubFile.toPath(), outputPath, forceEnableHyphenation); return outputPath.toFile(); }
1-268: LGTM!
82-92: 🗄️ Data Integrity & IntegrationNo change needed.
The 5-arg
Resource(String id, byte[] data, String href, MediaType mediaType, String inputEncoding)constructor exists, and the 4-arg variant delegates into it.backend/src/main/java/org/booklore/service/kobo/KepubHtmlConversionService.java (2)
107-138: LGTM!
1-60: LGTM!Also applies to: 140-260, 269-308
backend/src/test/java/org/booklore/service/kobo/KepubHtmlConversionServiceTest.java (1)
1-78: LGTM!
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 (3)
backend/src/test/java/org/booklore/service/kobo/KepubHtmlConversionServiceTest.java (1)
18-87: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd null, empty, stream, and boundary-input tests.
The suite currently covers only non-empty string happy paths. Add an explicit null-input contract, empty HTML, the
InputStreamoverload with a non-default encoding, and punctuation followed by closing quotes or Unicode code points.As per path instructions, tests must cover null, empty, and boundary values.
🤖 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/kobo/KepubHtmlConversionServiceTest.java` around lines 18 - 87, Extend KepubHtmlConversionServiceTest around transform_ShouldSplitSentences with tests for null input, empty HTML, punctuation followed by closing quotes, and Unicode code points; also exercise the InputStream overload using a non-default encoding and verify the decoded content. Cover the expected boundary behavior explicitly while preserving the existing happy-path assertions.Source: Path instructions
backend/src/main/java/org/booklore/service/kobo/KepubHtmlConversionService.java (1)
192-200: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winCollect the wrapping targets before mutating SVG descendants.
After wrapping an
svg, Jsoup’sNodeIteratorcontinues into its descendants; an<svg><text>child will receive the sentence-wrappingspanbranch and insert XHTML into the SVG markup. Collect the directimg/svgtargets first (or exclude descendants of wrapped nodes), and add a nested-SVG/regression test.🤖 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/kobo/KepubHtmlConversionService.java` around lines 192 - 200, Update the traversal around the img/svg wrapping logic in KepubHtmlConversionService so direct wrapping targets are collected before any DOM mutation, preventing NodeIterator from processing descendants of wrapped SVG elements. Ensure descendants such as svg text nodes are not passed through the sentence-wrapping span branch, and add a regression test covering nested SVG content.backend/src/main/java/org/booklore/service/kobo/KepubConversionService.java (1)
201-218: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winPrune spine references to filtered resources.
isIncludedResource(...)can skip a resource from the manifest, butoriginal.getSpine()is copied intokepubas-is. Any spine entry whose resource is filtered becomes a danglingitemrefin the written OPF because EpubWriter serializes spine references by resource ID without validating manifest membership. Rebuild the spine from kept manifest resources, or remove non-manifest references before copying it.🤖 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/kobo/KepubConversionService.java` around lines 201 - 218, The convertBookToKepub method must prune spine entries that reference resources excluded by isIncludedResource. Rebuild or filter original.getSpine() using the resources added to kepub, then set the resulting spine so no dangling itemref references remain.
🤖 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/kobo/KepubConversionService.java`:
- Around line 201-218: The convertBookToKepub method must prune spine entries
that reference resources excluded by isIncludedResource. Rebuild or filter
original.getSpine() using the resources added to kepub, then set the resulting
spine so no dangling itemref references remain.
In
`@backend/src/main/java/org/booklore/service/kobo/KepubHtmlConversionService.java`:
- Around line 192-200: Update the traversal around the img/svg wrapping logic in
KepubHtmlConversionService so direct wrapping targets are collected before any
DOM mutation, preventing NodeIterator from processing descendants of wrapped SVG
elements. Ensure descendants such as svg text nodes are not passed through the
sentence-wrapping span branch, and add a regression test covering nested SVG
content.
In
`@backend/src/test/java/org/booklore/service/kobo/KepubHtmlConversionServiceTest.java`:
- Around line 18-87: Extend KepubHtmlConversionServiceTest around
transform_ShouldSplitSentences with tests for null input, empty HTML,
punctuation followed by closing quotes, and Unicode code points; also exercise
the InputStream overload using a non-default encoding and verify the decoded
content. Cover the expected boundary behavior explicitly while preserving the
existing happy-path assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 52800fee-40d8-437f-80d4-93e61832515f
📒 Files selected for processing (4)
backend/src/main/java/org/booklore/service/kobo/KepubConversionService.javabackend/src/main/java/org/booklore/service/kobo/KepubHtmlConversionService.javabackend/src/test/java/org/booklore/service/kobo/KepubConversionServiceTest.javabackend/src/test/java/org/booklore/service/kobo/KepubHtmlConversionServiceTest.java
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
grimmory-tools/grimmory-docs(manual)
📜 Review details
⏰ Context from checks skipped due to timeout. (5)
- GitHub Check: Test Suite / Backend Tests
- GitHub Check: Test Suite / Frontend Tests
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (java-kotlin)
- GitHub Check: Frontend Lint Threshold Check
🧰 Additional context used
📓 Path-based instructions (3)
**/*
⚙️ 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/kobo/KepubConversionServiceTest.javabackend/src/test/java/org/booklore/service/kobo/KepubHtmlConversionServiceTest.javabackend/src/main/java/org/booklore/service/kobo/KepubHtmlConversionService.javabackend/src/main/java/org/booklore/service/kobo/KepubConversionService.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/kobo/KepubConversionServiceTest.javabackend/src/test/java/org/booklore/service/kobo/KepubHtmlConversionServiceTest.javabackend/src/main/java/org/booklore/service/kobo/KepubHtmlConversionService.javabackend/src/main/java/org/booklore/service/kobo/KepubConversionService.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/kobo/KepubConversionServiceTest.javabackend/src/test/java/org/booklore/service/kobo/KepubHtmlConversionServiceTest.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/kobo/KepubConversionServiceTest.javabackend/src/test/java/org/booklore/service/kobo/KepubHtmlConversionServiceTest.javabackend/src/main/java/org/booklore/service/kobo/KepubHtmlConversionService.javabackend/src/main/java/org/booklore/service/kobo/KepubConversionService.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/kobo/KepubConversionServiceTest.javabackend/src/test/java/org/booklore/service/kobo/KepubHtmlConversionServiceTest.javabackend/src/main/java/org/booklore/service/kobo/KepubHtmlConversionService.javabackend/src/main/java/org/booklore/service/kobo/KepubConversionService.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/kobo/KepubConversionServiceTest.javabackend/src/test/java/org/booklore/service/kobo/KepubHtmlConversionServiceTest.javabackend/src/main/java/org/booklore/service/kobo/KepubHtmlConversionService.javabackend/src/main/java/org/booklore/service/kobo/KepubConversionService.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/kobo/KepubConversionServiceTest.javabackend/src/test/java/org/booklore/service/kobo/KepubHtmlConversionServiceTest.javabackend/src/main/java/org/booklore/service/kobo/KepubHtmlConversionService.javabackend/src/main/java/org/booklore/service/kobo/KepubConversionService.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/kobo/KepubConversionServiceTest.javabackend/src/test/java/org/booklore/service/kobo/KepubHtmlConversionServiceTest.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/kobo/KepubConversionServiceTest.javabackend/src/test/java/org/booklore/service/kobo/KepubHtmlConversionServiceTest.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/kobo/KepubConversionServiceTest.javabackend/src/test/java/org/booklore/service/kobo/KepubHtmlConversionServiceTest.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/kobo/KepubHtmlConversionService.javabackend/src/main/java/org/booklore/service/kobo/KepubConversionService.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/kobo/KepubHtmlConversionService.javabackend/src/main/java/org/booklore/service/kobo/KepubConversionService.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/kobo/KepubHtmlConversionService.javabackend/src/main/java/org/booklore/service/kobo/KepubConversionService.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/kobo/KepubHtmlConversionService.javabackend/src/main/java/org/booklore/service/kobo/KepubConversionService.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/kobo/KepubHtmlConversionService.javabackend/src/main/java/org/booklore/service/kobo/KepubConversionService.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/kobo/KepubHtmlConversionService.javabackend/src/main/java/org/booklore/service/kobo/KepubConversionService.java
🔇 Additional comments (8)
backend/src/main/java/org/booklore/service/kobo/KepubConversionService.java (4)
220-225: Remove or actually consume the transformed OPF.
EpubWriter.write(...)regeneratesOEBPS/content.opf; it does not readBook.getOpfResource(). Therefore this transformed resource never affects the generated EPUB and only adds parsing work and another failure point. Remove this assignment/helper or change the writer contract. (raw.githubusercontent.com)
265-268: Use thePathoverload fortempDir.
tempDir.getPath()is aString, so this resolves toFiles.createTempFile(String prefix, String suffix)and uses the default temporary directory instead oftempDir. Use:- var outputPath = Files.createTempFile(tempDir.getPath(), ".kepub.epub"); + var outputPath = Files.createTempFile( + tempDir.toPath(), "grimmory-kepub-", ".kepub.epub");The Java 25 API distinguishes the default-directory
Stringoverload from the explicitPath-directory overload. (docs.oracle.com)
271-274: UseApiErrorfor validation failures.This service still throws a raw
IllegalArgumentException, bypassing the project’s standard error mapping. Use the appropriateApiError.createException(...)path.As per path instructions, service-layer exceptions must use the
ApiErrorenum.Source: Path instructions
66-108: LGTM!Also applies to: 178-199, 230-263
backend/src/test/java/org/booklore/service/kobo/KepubConversionServiceTest.java (2)
38-52: Add coverage for the file-based conversion API.These tests only exercise the stream overload, so they cannot catch regressions in
convertEpubToKepub(File, File, boolean), including the temporary-directory and invalid-input behavior. Add a real temporary.epubinput test plus invalid path/directory cases.
54-134: LGTM!backend/src/main/java/org/booklore/service/kobo/KepubHtmlConversionService.java (1)
61-68: LGTM!Also applies to: 107-138, 145-191, 205-302
backend/src/test/java/org/booklore/service/kobo/KepubHtmlConversionServiceTest.java (1)
32-39: LGTM!
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/kobo/KepubHtmlConversionService.java`:
- Around line 150-203: Update the document.body().filter traversal to avoid
calling textNode.before, element.before, or koboSpan.appendChild within the
callback. Collect the required sentence wrappers and image/SVG wrapper
operations in a list during traversal, then apply all before() insertions and
reparenting after filtering completes while preserving the existing skip/remove
behavior.
🪄 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 Plus
Run ID: fb857976-63ec-42ce-b9d0-b5d93e1884be
📒 Files selected for processing (2)
backend/src/main/java/org/booklore/service/kobo/KepubHtmlConversionService.javabackend/src/test/java/org/booklore/service/kobo/KepubHtmlConversionServiceTest.java
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
grimmory-tools/grimmory-docs(manual)
📜 Review details
⏰ Context from checks skipped due to timeout. (3)
- GitHub Check: Test Suite / Backend Tests
- GitHub Check: Test Suite / Frontend Tests
- GitHub Check: Analyze (java-kotlin)
🧰 Additional context used
📓 Path-based instructions (3)
**/*
⚙️ 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/kobo/KepubHtmlConversionService.javabackend/src/test/java/org/booklore/service/kobo/KepubHtmlConversionServiceTest.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/kobo/KepubHtmlConversionService.javabackend/src/test/java/org/booklore/service/kobo/KepubHtmlConversionServiceTest.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/kobo/KepubHtmlConversionServiceTest.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/kobo/KepubHtmlConversionService.javabackend/src/test/java/org/booklore/service/kobo/KepubHtmlConversionServiceTest.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/kobo/KepubHtmlConversionService.javabackend/src/test/java/org/booklore/service/kobo/KepubHtmlConversionServiceTest.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/kobo/KepubHtmlConversionService.javabackend/src/test/java/org/booklore/service/kobo/KepubHtmlConversionServiceTest.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/kobo/KepubHtmlConversionService.javabackend/src/test/java/org/booklore/service/kobo/KepubHtmlConversionServiceTest.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/kobo/KepubHtmlConversionService.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/kobo/KepubHtmlConversionService.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/kobo/KepubHtmlConversionService.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/kobo/KepubHtmlConversionService.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/kobo/KepubHtmlConversionService.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/kobo/KepubHtmlConversionService.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/kobo/KepubHtmlConversionServiceTest.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/kobo/KepubHtmlConversionServiceTest.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/kobo/KepubHtmlConversionServiceTest.java
🪛 ast-grep (0.44.1)
backend/src/test/java/org/booklore/service/kobo/KepubHtmlConversionServiceTest.java
[warning] 65-70: Regular expression is compiled from a non-literal, possibly user-controlled value. A crafted regex (or input matched against one) can trigger catastrophic backtracking and hang the thread (ReDoS). Use a hardcoded literal pattern, wrap untrusted text with Pattern.quote(...), or validate/length-limit the input and enforce a matching timeout before passing it to Pattern.compile / String.matches / String.replaceAll / String.replaceFirst.
Context: assertThat(actual).matches(
Pattern.compile(
".<svg[^<>]+>\s\sExample\s.*",
Pattern.MULTILINE | Pattern.DOTALL
)
)
Note: [CWE-1333] Inefficient Regular Expression Complexity.
(redos-non-literal-regex-java)
🔀 Multi-repo context
Linked repositories findings
grimmory-tools/grimmory-docs
src/content/docs/integration/kobo.mdx:23-27documents automatic EPUB→KePub conversion as a supported setting and promises Kobo reading enhancements. The new in-process converter must preserve this user-facing behavior.src/content/docs/integration/kobo.mdx:164-169documents bidirectional Kobo reading-progress synchronization. Since the PR changes generatedkoboSpanmarkers, this is the relevant documented consumer of the transformed KePub output.
🔇 Additional comments (2)
backend/src/main/java/org/booklore/service/kobo/KepubHtmlConversionService.java (1)
105-149: LGTM!Also applies to: 206-304
backend/src/test/java/org/booklore/service/kobo/KepubHtmlConversionServiceTest.java (1)
62-72: LGTM! Good addition closing the previously-requested img/svg Kobo-span coverage gap.
imnotjames
changed the title
imnotjames
force-pushed
the
refactor/-/kepubify
branch
from
July 30, 2026 20:13
628fb6b to
dff76e4
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
backend/src/main/java/org/booklore/service/kobo/KepubConversionService.java (2)
271-274: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winUse
ApiErrorinstead of rawIllegalArgumentException.As per path instructions for
**/service/**/*.java, "Use ApiError enum for throwing exceptions."validateInputsstill throws a plainIllegalArgumentException— the.epub-extension check bug itself was fixed by removing that check, but the exception type was never migrated to the project convention.🤖 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/kobo/KepubConversionService.java` around lines 271 - 274, Update KepubConversionService.validateInputs to throw the project-standard ApiError enum instead of IllegalArgumentException when the input path is null or not a regular file, preserving the existing validation condition and invalid-path context.Source: Path instructions
220-225: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftVerify
setOpfResource(...)actually takes effect, given the coverless-EPUB NPE fix.The coverless-book NPE from the prior review is fixed (
transformOPF/transformOPFCoverImagenow null-checkcover). The other half of that same review comment — thatEpubWriter.write(...)regeneratesOEBPS/content.opffrom theBook's structured state and therefore ignores a manually-setopfResource— was never disputed and appears unaddressed. If accurate, the entire point oftransformOPFCoverImage(adding the EPUB3cover-imageproperty Kobo requires) may not reach the final output at all.🤖 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/kobo/KepubConversionService.java` around lines 220 - 225, Verify the final EPUB-writing path after the kepub.setOpfResource call and ensure the transformed OPF, including the EPUB3 cover-image property, is what EpubWriter.write emits rather than regenerated content from stale Book state. Update the conversion flow around KepubConversionService and the writer integration to synchronize or use the structured metadata required by the writer, while preserving null-cover handling for coverless books.
🤖 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/kobo/KepubConversionService.java`:
- Around line 265-269: Fix KepubConversionService.convertEpubToKepub(File, File,
boolean) to create the output with the Path-based Files.createTempFile overload
using the supplied tempDir and the “grimmory-kepub-” prefix and “.kepub.epub”
suffix. In KepubConversionServiceTest, add coverage using a real temporary
directory and real .epub input, asserting the output is created inside that
directory, and add negative-path coverage for validateInputs with null and
non-regular-file input. Affected sites:
backend/src/main/java/org/booklore/service/kobo/KepubConversionService.java
lines 265-269 requires the production fix;
backend/src/test/java/org/booklore/service/kobo/KepubConversionServiceTest.java
lines 1-135 requires the tests.
- Around line 56-64: Update the Lombok constructor annotation on
KepubConversionService so its generated required-args constructor is annotated
for Spring injection, ensuring the managed KepubHtmlConversionService bean is
selected over the explicit no-arg constructor. Keep the no-arg constructor’s
existing behavior unchanged.
In
`@backend/src/test/java/org/booklore/service/kobo/KepubHtmlConversionServiceTest.java`:
- Around line 17-98: Add edge-case tests around
KepubHtmlConversionServiceTest.transform: verify an empty body transforms
without errors, text lacking terminal punctuation is emitted rather than
dropped, and Unicode supplementary characters such as emoji are preserved
correctly when sentence processing uses code points. Include coverage for
multiple images in one input and assert each image is wrapped with the expected
Kobo span sequencing.
---
Duplicate comments:
In `@backend/src/main/java/org/booklore/service/kobo/KepubConversionService.java`:
- Around line 271-274: Update KepubConversionService.validateInputs to throw the
project-standard ApiError enum instead of IllegalArgumentException when the
input path is null or not a regular file, preserving the existing validation
condition and invalid-path context.
- Around line 220-225: Verify the final EPUB-writing path after the
kepub.setOpfResource call and ensure the transformed OPF, including the EPUB3
cover-image property, is what EpubWriter.write emits rather than regenerated
content from stale Book state. Update the conversion flow around
KepubConversionService and the writer integration to synchronize or use the
structured metadata required by the writer, while preserving null-cover handling
for coverless books.
🪄 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 Plus
Run ID: 0fd18e70-2418-4e42-a4a8-a3ebfd6bc4b2
📒 Files selected for processing (5)
Dockerfilebackend/src/main/java/org/booklore/service/kobo/KepubConversionService.javabackend/src/main/java/org/booklore/service/kobo/KepubHtmlConversionService.javabackend/src/test/java/org/booklore/service/kobo/KepubConversionServiceTest.javabackend/src/test/java/org/booklore/service/kobo/KepubHtmlConversionServiceTest.java
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
grimmory-tools/grimmory-docs(manual)
💤 Files with no reviewable changes (1)
- Dockerfile
📜 Review details
⏰ Context from checks skipped due to timeout. (4)
- GitHub Check: Test Suite / Frontend Tests
- GitHub Check: Test Suite / Backend Tests
- GitHub Check: Analyze (java-kotlin)
- GitHub Check: Frontend Lint Threshold Check
🧰 Additional context used
📓 Path-based instructions (3)
**/*
⚙️ 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/kobo/KepubHtmlConversionServiceTest.javabackend/src/test/java/org/booklore/service/kobo/KepubConversionServiceTest.javabackend/src/main/java/org/booklore/service/kobo/KepubConversionService.javabackend/src/main/java/org/booklore/service/kobo/KepubHtmlConversionService.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/kobo/KepubHtmlConversionServiceTest.javabackend/src/test/java/org/booklore/service/kobo/KepubConversionServiceTest.javabackend/src/main/java/org/booklore/service/kobo/KepubConversionService.javabackend/src/main/java/org/booklore/service/kobo/KepubHtmlConversionService.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/kobo/KepubHtmlConversionServiceTest.javabackend/src/test/java/org/booklore/service/kobo/KepubConversionServiceTest.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/kobo/KepubHtmlConversionServiceTest.javabackend/src/test/java/org/booklore/service/kobo/KepubConversionServiceTest.javabackend/src/main/java/org/booklore/service/kobo/KepubConversionService.javabackend/src/main/java/org/booklore/service/kobo/KepubHtmlConversionService.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/kobo/KepubHtmlConversionServiceTest.javabackend/src/test/java/org/booklore/service/kobo/KepubConversionServiceTest.javabackend/src/main/java/org/booklore/service/kobo/KepubConversionService.javabackend/src/main/java/org/booklore/service/kobo/KepubHtmlConversionService.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/kobo/KepubHtmlConversionServiceTest.javabackend/src/test/java/org/booklore/service/kobo/KepubConversionServiceTest.javabackend/src/main/java/org/booklore/service/kobo/KepubConversionService.javabackend/src/main/java/org/booklore/service/kobo/KepubHtmlConversionService.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/kobo/KepubHtmlConversionServiceTest.javabackend/src/test/java/org/booklore/service/kobo/KepubConversionServiceTest.javabackend/src/main/java/org/booklore/service/kobo/KepubConversionService.javabackend/src/main/java/org/booklore/service/kobo/KepubHtmlConversionService.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/kobo/KepubHtmlConversionServiceTest.javabackend/src/test/java/org/booklore/service/kobo/KepubConversionServiceTest.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/kobo/KepubHtmlConversionServiceTest.javabackend/src/test/java/org/booklore/service/kobo/KepubConversionServiceTest.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/kobo/KepubHtmlConversionServiceTest.javabackend/src/test/java/org/booklore/service/kobo/KepubConversionServiceTest.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/kobo/KepubConversionService.javabackend/src/main/java/org/booklore/service/kobo/KepubHtmlConversionService.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/kobo/KepubConversionService.javabackend/src/main/java/org/booklore/service/kobo/KepubHtmlConversionService.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/kobo/KepubConversionService.javabackend/src/main/java/org/booklore/service/kobo/KepubHtmlConversionService.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/kobo/KepubConversionService.javabackend/src/main/java/org/booklore/service/kobo/KepubHtmlConversionService.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/kobo/KepubConversionService.javabackend/src/main/java/org/booklore/service/kobo/KepubHtmlConversionService.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/kobo/KepubConversionService.javabackend/src/main/java/org/booklore/service/kobo/KepubHtmlConversionService.java
🪛 ast-grep (0.45.0)
backend/src/test/java/org/booklore/service/kobo/KepubHtmlConversionServiceTest.java
[warning] 65-70: Regular expression is compiled from a non-literal, possibly user-controlled value. A crafted regex (or input matched against one) can trigger catastrophic backtracking and hang the thread (ReDoS). Use a hardcoded literal pattern, wrap untrusted text with Pattern.quote(...), or validate/length-limit the input and enforce a matching timeout before passing it to Pattern.compile / String.matches / String.replaceAll / String.replaceFirst.
Context: assertThat(actual).matches(
Pattern.compile(
".<svg[^<>]+>\s\sExample\s.*",
Pattern.MULTILINE | Pattern.DOTALL
)
)
Note: [CWE-1333] Inefficient Regular Expression Complexity.
(redos-non-literal-regex-java)
[warning] 80-85: Regular expression is compiled from a non-literal, possibly user-controlled value. A crafted regex (or input matched against one) can trigger catastrophic backtracking and hang the thread (ReDoS). Use a hardcoded literal pattern, wrap untrusted text with Pattern.quote(...), or validate/length-limit the input and enforce a matching timeout before passing it to Pattern.compile / String.matches / String.replaceAll / String.replaceFirst.
Context: assertThat(actual).matches(
Pattern.compile(
".\s<div id="book-columns">\s*<div id="book-inner">.*",
Pattern.DOTALL | Pattern.MULTILINE
)
)
Note: [CWE-1333] Inefficient Regular Expression Complexity.
(redos-non-literal-regex-java)
backend/src/main/java/org/booklore/service/kobo/KepubConversionService.java
[warning] 155-155: TransformerFactory used without secure processing is vulnerable to XXE
Context: TransformerFactory.newInstance().newTransformer()
Note: [CWE-611] Improper Restriction of XML External Entity Reference.
(xml-parsing-xxe-transformer)
[warning] 265-265: Temporary file not deleted
Context: Files.createTempFile(tempDir.getPath(), ".kepub.epub")
Note: [CWE-377] Insecure Temporary File. Security best practice.
(tempfile-delete)
🔀 Multi-repo context grimmory-tools/grimmory-docs
Linked repositories findings
grimmory-tools/grimmory-docs
src/content/docs/integration/kobo.mdx:23-27documents automatic EPUB→KePub conversion as a supported setting; the replacement converter must preserve this behavior.[::grimmory-tools/grimmory-docs::]src/content/docs/integration/kobo.mdx:164-169documents bidirectional Kobo reading-progress synchronization, making generatedkoboSpanstructure compatibility-sensitive.[::grimmory-tools/grimmory-docs::]
🔇 Additional comments (3)
backend/src/main/java/org/booklore/service/kobo/KepubHtmlConversionService.java (2)
1-136: LGTM!
138-304: LGTM! Previously raised concerns for this file (static constants,ID_FORMAT_KOBO_SPANreuse, TODO tracking, body-wrapperchildNodes(), XHTML/SVG xmlns, and the jsoup in-callback reparenting pattern) are addressed or already acknowledged as intentional.backend/src/main/java/org/booklore/service/kobo/KepubConversionService.java (1)
66-219: LGTM! Prior singleton-EpubWritersharing concern is resolved (writer is now passed per-call/created fresh); resource filtering, media-type gating, and HTML transformation delegation look correct.
2 tasks
imnotjames
force-pushed
the
refactor/-/kepubify
branch
3 times, most recently
from
August 9, 2026 05:38
1cda60c to
01cfff2
Compare
2 tasks
imnotjames
force-pushed
the
refactor/-/kepubify
branch
from
September 11, 2026 16:44
01cfff2 to
cd06a7b
Compare
alexhb1
reviewed
Sep 12, 2026
alexhb1
left a comment
alexhb1
left a comment
Member
There was a problem hiding this comment.
I don't think we need to use EpubWriter at all here? It builds a new epub from scratch which kepubify doesn't do.
I'm going to bed now so not had chance to write up any specifics, but I've put up a test branch i've been using to mess around and verify some things with test epubs. c68bbdc
Firstly includes tests for each of the issues i was facing, verifies their errors on current 2002, plus tweaks to simplify the code that doesn't use EpubWriter and passes the tests. In all, this seems to work identically to kepubify.
Feel free to poke around if you get chance (:
Contributor
Author
The epubwriter is how we should be crafting all epubs, isn't it? Since that's how we craft them elsewhere within Grimmory. It looks like the concerns are:
The first two seem to be problems with the epub4j library not supporting these use cases. I don't know if the upstream does or not, but after some testing this is just every epub file. The last I'm not sure I understand why we need to retain the source filename when it's a temporary file that's meant to be read and then erased. I can make the change, I just don't really understand where the requirement comes from. I'm going to hold on that one as it requires extra sanitization.
I pulled the HTML tests and integrated them, along with fixing the HTML conversion to handle them. |
Member
|
I couldn't find anywhere that epubwriter is used in grimmory before this? This would be the first. And I'm sure that kepub creation doesn't need it at all. You need the chapter markers and kobo spans and everything that the HTML conversion service does, but not generate an entirely new file at the end. All those test scenarios came from running the various EPUB spec test files via both kepubify and the new transformer and seeing what regressions existed. If you avoid creating a brand new epub unnecessarily, most of those issues go away. There are some other kepub-specific edge cases (which you've fixed), so i think the other issues I saw from kepubify (Encryption) would be solved by removing the epub writer step. The file name isn't temp I don't think? I guess you could argue consistency with downloading the file yourself or converting comic -> EPUB, which both preserve a file name. Either way, it came from a regression vs the old kepubify version, gone from proper filename -> random filename. |
Contributor
Author
We are still generating a new file at the end, just in a different way.
I'll switch it to use the archive service then instead of zip file.
As far as I can tell we create the file with kepubify, stream the file contents (not the file itself), and then immediately delete it. The kobo device does not seem to see the file name in my testing. |
imnotjames
marked this pull request as draft
September 14, 2026 01:10
imnotjames
force-pushed
the
refactor/-/kepubify
branch
from
September 14, 2026 04:53
662498b to
7dd68bd
Compare
imnotjames
force-pushed
the
refactor/-/kepubify
branch
from
September 14, 2026 04:54
7dd68bd to
47973bf
Compare
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
because kepubify is fairly unmaintained, this PR implements logic similar to kepubify directly in Grimmory
Linked Issue
fixes #2001
Changes
Manual Testing Steps
downloaded book and read in kepub
checked that progress pushed back makes sense
did an HTML diff to check for differences in how kobo spans are generated
Screenshots (Optional)
Additional Context (Optional)
the exact kobo IDs are different because this implementation uses a single integer for indexing
AI Disclosure
None.
Checklist
just ui checkandjust api check.Summary by CodeRabbit
ffprobeinstead ofkepubify.