refactor(epub): migrate from documentnode to grimmory epub4j library and update related code#334
Merged
zachyale merged 17 commits intogrimmory-tools:developfrom Apr 10, 2026
Merged
Conversation
…and update related code
# Conflicts: # booklore-api/build.gradle.kts
Contributor
📝 WalkthroughWalkthroughReplaces io.documentnode:epub4j + zip4j EPUB handling with org.grimmory:epub4j, migrating parsing, cover detection, CFI conversion, container streaming, and EPUB repackaging; adds Jsoup-based DocumentNavigator and enables Java preview flags in the Gradle build. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant EpubMetadataExtractor
participant EpubReader as "EpubReader (epub4j)"
participant CoverDetector
participant EpubContainer
participant Filesystem
Client->>EpubMetadataExtractor: request cover bytes
EpubMetadataExtractor->>EpubReader: readEpubLazy(path, "UTF-8")
EpubReader->>CoverDetector: detectCoverImageWithMethod(book)
alt cover found
CoverDetector-->>EpubMetadataExtractor: resource bytes
else fallback
EpubMetadataExtractor->>EpubContainer: open(path)
EpubContainer->>EpubMetadataExtractor: parseOpf()
EpubMetadataExtractor->>EpubContainer: readBytes(resolvedHref)
EpubContainer-->>EpubMetadataExtractor: bytes (if exists)
end
EpubMetadataExtractor->>Client: return cover bytes or empty
sequenceDiagram
participant WriterAPI
participant EpubContainer
participant TempDir
participant ZipOut as "ZipOutputStream (JDK)"
participant Filesystem
WriterAPI->>EpubContainer: open(sourceEpub)
EpubContainer->>TempDir: listAllFiles() -> stream bytes per entry
TempDir-->>WriterAPI: files written to temp dir
WriterAPI->>ZipOut: create target zip
ZipOut->>Filesystem: write mimetype (STORED entry with CRC/size)
ZipOut->>TempDir: walk files -> add entries (exclude mimetype), write directories
ZipOut->>Filesystem: finalize EPUB zip
WriterAPI->>Filesystem: return new epub path
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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: 2
🧹 Nitpick comments (3)
booklore-api/src/main/java/org/booklore/service/reader/EpubReaderService.java (1)
247-254: Redundant condition in navHref detection.Line 250 checks
resource.getMediaType() != MediaTypes.NCX, but this is always true whenresource.getMediaType() == MediaTypes.XHTML(line 248) since they are mutually exclusive media types.for (Resource resource : book.getResources().getAll()) { if (resource.getMediaType() == MediaTypes.XHTML - && !spineHrefs.contains(resource.getHref()) - && resource.getMediaType() != MediaTypes.NCX) { + && !spineHrefs.contains(resource.getHref())) { navHref = rootPath + resource.getHref(); break; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@booklore-api/src/main/java/org/booklore/service/reader/EpubReaderService.java` around lines 247 - 254, The loop in EpubReaderService that sets navHref contains a redundant check: when iterating resources from book.getResources().getAll() you already test resource.getMediaType() == MediaTypes.XHTML, so the additional resource.getMediaType() != MediaTypes.NCX is unnecessary; remove the redundant NCX comparison and simplify the if to only check MediaTypes.XHTML and !spineHrefs.contains(resource.getHref()), keeping assignment to navHref and the break as-is. Ensure you update the conditional in the method where navHref is computed (reference symbols: navHref, book.getResources().getAll(), Resource, MediaTypes.XHTML, MediaTypes.NCX, spineHrefs) and run tests to confirm behavior unchanged.booklore-api/src/main/java/org/booklore/util/epub/EpubContentReader.java (1)
22-26: Verify the null entry in MEDIA_TYPES is intentional.Adding
nullto theMEDIA_TYPESlist is unusual. If this is required by the epub4j library to handle resources with unknown/null media types, consider adding a brief comment explaining the purpose.private static final List<MediaType> MEDIA_TYPES = new ArrayList<>(); static { MEDIA_TYPES.addAll(Arrays.asList(MediaTypes.mediaTypes)); - MEDIA_TYPES.add(null); + MEDIA_TYPES.add(null); // Include null to handle resources with unspecified media types }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@booklore-api/src/main/java/org/booklore/util/epub/EpubContentReader.java` around lines 22 - 26, The MEDIA_TYPES list in EpubContentReader currently appends a null entry (MEDIA_TYPES and MediaTypes.mediaTypes); verify whether epub4j requires a null media type sentinel — if it is required keep the null but add a clear inline comment explaining why (e.g., "null used as sentinel for unknown/unspecified media types required by epub4j resource lookup") and ensure any code that iterates MEDIA_TYPES handles null safely; if it is not required, remove the MEDIA_TYPES.add(null) line and update any callers/tests that relied on the null to use explicit handling for unknown media types instead.Dockerfile (1)
64-71: Consider removing redundant JVM flag from CMD.The
--enable-native-access=ALL-UNNAMEDflag in CMD (line 107) is now redundant since it's already set inJAVA_TOOL_OPTIONS(line 70). The--enable-previewflag is also missing from CMD but is covered byJAVA_TOOL_OPTIONS.You could simplify the CMD to:
CMD ["java", "-jar", "/app/app.jar"]Or, for consistency/explicitness, keep CMD with both flags. The current configuration works but has asymmetric redundancy.
Also applies to: 107-107
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Dockerfile` around lines 64 - 71, The Dockerfile sets JVM flags in the ENV variable JAVA_TOOL_OPTIONS (including --enable-native-access=ALL-UNNAMED and --enable-preview) but the CMD also redundantly repeats --enable-native-access and omits --enable-preview; update the CMD invocation (the java -jar startup command) to either remove redundant flags and simply use CMD ["java","-jar","/app/app.jar"] or make it explicit and consistent by including both --enable-native-access=ALL-UNNAMED and --enable-preview in CMD to match ENV, ensuring no asymmetric duplication between ENV JAVA_TOOL_OPTIONS and the CMD startup command.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@booklore-api/build.gradle.kts`:
- Around line 94-95: The build fails because ArchiveService.java still imports
and uses nightcompress APIs (imports com.github.gotson.nightcompress.Archive,
ArchiveEntry, LibArchiveException and calls Archive.isAvailable(),
Archive.getEntries(), Archive.getInputStream()) while the dependency was
removed; fix by either restoring the nightcompress dependency in
build.gradle.kts or migrating ArchiveService.java to junrar: replace
nightcompress imports/usages with equivalent junrar classes and APIs (e.g., open
archive streams, iterate entries, extract InputStreams and handle exceptions)
and update all call sites in ArchiveService.java (the imports at the top and
calls around lines where Archive.isAvailable(), Archive.getEntries(),
Archive.getInputStream() are used) to the junrar API ensuring exception handling
and resource closing match junrar patterns.
In
`@booklore-api/src/main/java/org/booklore/service/reader/EpubReaderService.java`:
- Around line 402-407: streamEntryFromZip currently calls NativeArchive.open and
archive.readEntry which buffers the entire entry into a byte[]; change it to
stream the entry directly to the provided OutputStream (similar to
ArchiveService.transferEntryTo) by opening an InputStream for the entry from
NativeArchive (e.g., archive.openEntryStream / getInputStream for entryName) and
calling inputStream.transferTo(outputStream) or using a buffered copy loop,
ensuring the entry InputStream is closed in a try-with-resources and preserving
the existing exception signature of streamEntryFromZip.
---
Nitpick comments:
In
`@booklore-api/src/main/java/org/booklore/service/reader/EpubReaderService.java`:
- Around line 247-254: The loop in EpubReaderService that sets navHref contains
a redundant check: when iterating resources from book.getResources().getAll()
you already test resource.getMediaType() == MediaTypes.XHTML, so the additional
resource.getMediaType() != MediaTypes.NCX is unnecessary; remove the redundant
NCX comparison and simplify the if to only check MediaTypes.XHTML and
!spineHrefs.contains(resource.getHref()), keeping assignment to navHref and the
break as-is. Ensure you update the conditional in the method where navHref is
computed (reference symbols: navHref, book.getResources().getAll(), Resource,
MediaTypes.XHTML, MediaTypes.NCX, spineHrefs) and run tests to confirm behavior
unchanged.
In `@booklore-api/src/main/java/org/booklore/util/epub/EpubContentReader.java`:
- Around line 22-26: The MEDIA_TYPES list in EpubContentReader currently appends
a null entry (MEDIA_TYPES and MediaTypes.mediaTypes); verify whether epub4j
requires a null media type sentinel — if it is required keep the null but add a
clear inline comment explaining why (e.g., "null used as sentinel for
unknown/unspecified media types required by epub4j resource lookup") and ensure
any code that iterates MEDIA_TYPES handles null safely; if it is not required,
remove the MEDIA_TYPES.add(null) line and update any callers/tests that relied
on the null to use explicit handling for unknown media types instead.
In `@Dockerfile`:
- Around line 64-71: The Dockerfile sets JVM flags in the ENV variable
JAVA_TOOL_OPTIONS (including --enable-native-access=ALL-UNNAMED and
--enable-preview) but the CMD also redundantly repeats --enable-native-access
and omits --enable-preview; update the CMD invocation (the java -jar startup
command) to either remove redundant flags and simply use CMD
["java","-jar","/app/app.jar"] or make it explicit and consistent by including
both --enable-native-access=ALL-UNNAMED and --enable-preview in CMD to match
ENV, ensuring no asymmetric duplication between ENV JAVA_TOOL_OPTIONS and the
CMD startup command.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5c79d929-e00e-4e1d-b1d3-179188e15325
📒 Files selected for processing (9)
Dockerfilebooklore-api/build.gradle.ktsbooklore-api/src/main/java/org/booklore/service/metadata/extractor/EpubMetadataExtractor.javabooklore-api/src/main/java/org/booklore/service/metadata/writer/EpubMetadataWriter.javabooklore-api/src/main/java/org/booklore/service/reader/EpubReaderService.javabooklore-api/src/main/java/org/booklore/util/epub/EpubContentReader.javabooklore-api/src/test/java/org/booklore/service/reader/EpubReaderServiceTest.javabooklore-api/src/test/java/org/booklore/util/epub/EpubContentReaderTest.javabooklore-api/src/test/java/org/booklore/util/koreader/EpubCfiServiceTest.java
📜 Review details
🧰 Additional context used
📓 Path-based instructions (2)
booklore-api/src/**/*.java
📄 CodeRabbit inference engine (AGENTS.md)
booklore-api/src/**/*.java: Use 4-space indentation in Java files and match surrounding Java style
Prefer constructor injection via Lombok patterns already used in the codebase. Do not introduce@Autowiredfield injection
Use MapStruct for entity/DTO mapping
Files:
booklore-api/src/test/java/org/booklore/util/koreader/EpubCfiServiceTest.javabooklore-api/src/test/java/org/booklore/util/epub/EpubContentReaderTest.javabooklore-api/src/main/java/org/booklore/util/epub/EpubContentReader.javabooklore-api/src/main/java/org/booklore/service/metadata/writer/EpubMetadataWriter.javabooklore-api/src/test/java/org/booklore/service/reader/EpubReaderServiceTest.javabooklore-api/src/main/java/org/booklore/service/metadata/extractor/EpubMetadataExtractor.javabooklore-api/src/main/java/org/booklore/service/reader/EpubReaderService.java
booklore-api/src/test/**/*.java
📄 CodeRabbit inference engine (AGENTS.md)
Prefer focused unit tests; use
@SpringBootTestonly when the Spring context is required
Files:
booklore-api/src/test/java/org/booklore/util/koreader/EpubCfiServiceTest.javabooklore-api/src/test/java/org/booklore/util/epub/EpubContentReaderTest.javabooklore-api/src/test/java/org/booklore/service/reader/EpubReaderServiceTest.java
🧠 Learnings (6)
📚 Learning: 2026-03-26T01:46:48.863Z
Learnt from: CR
Repo: grimmory-tools/grimmory PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-26T01:46:48.863Z
Learning: Applies to booklore-api/src/test/**/*.java : Prefer focused unit tests; use `SpringBootTest` only when the Spring context is required
Applied to files:
booklore-api/src/test/java/org/booklore/util/koreader/EpubCfiServiceTest.javabooklore-api/src/test/java/org/booklore/util/epub/EpubContentReaderTest.javabooklore-api/src/test/java/org/booklore/service/reader/EpubReaderServiceTest.java
📚 Learning: 2026-03-24T18:46:47.249Z
Learnt from: pedronave
Repo: grimmory-tools/grimmory PR: 173
File: booklore-api/src/main/java/org/booklore/service/hardcover/HardcoverSyncService.java:188-199
Timestamp: 2026-03-24T18:46:47.249Z
Learning: In grimmory-tools/grimmory (HardcoverSyncService.java), when a `hardcoverBookId` is stored in book metadata but is malformed (non-numeric), the preferred behavior is to return null and skip the sync rather than falling back to ISBN-based lookup. The reasoning is that an explicitly set bookId represents deliberate user intent, and silently resolving via ISBN could sync progress to a different book than intended.
Applied to files:
booklore-api/src/test/java/org/booklore/util/epub/EpubContentReaderTest.javabooklore-api/src/main/java/org/booklore/service/metadata/extractor/EpubMetadataExtractor.javabooklore-api/src/main/java/org/booklore/service/reader/EpubReaderService.java
📚 Learning: 2026-03-26T01:46:48.863Z
Learnt from: CR
Repo: grimmory-tools/grimmory PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-26T01:46:48.863Z
Learning: Applies to booklore-api/src/**/*Entity.java : Keep JPA entities on the `*Entity` suffix
Applied to files:
booklore-api/src/test/java/org/booklore/service/reader/EpubReaderServiceTest.java
📚 Learning: 2026-03-26T01:46:48.863Z
Learnt from: CR
Repo: grimmory-tools/grimmory PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-26T01:46:48.863Z
Learning: Applies to booklore-api/src/**/*.java : Use MapStruct for entity/DTO mapping
Applied to files:
booklore-api/src/test/java/org/booklore/service/reader/EpubReaderServiceTest.javabooklore-api/build.gradle.kts
📚 Learning: 2026-03-31T06:22:25.311Z
Learnt from: imnotjames
Repo: grimmory-tools/grimmory PR: 113
File: booklore-api/build.gradle.kts:89-90
Timestamp: 2026-03-31T06:22:25.311Z
Learning: When reviewing build logic or CI checks that call the JitPack build status API, do not treat a response like {"status":"none"} as evidence that a dependency is unavailable or that a build has failed. JitPack typically builds packages on-demand the first time they’re requested (e.g., via Gradle); "none" means the package isn’t pre-built/cached yet. Only raise a build failure concern when the status explicitly indicates an error/failure (e.g., failed/cancelled), or when dependent resolution actually fails.
Applied to files:
booklore-api/build.gradle.kts
📚 Learning: 2026-03-26T01:46:48.863Z
Learnt from: CR
Repo: grimmory-tools/grimmory PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-26T01:46:48.863Z
Learning: Applies to booklore-api/src/**/*.java : Prefer constructor injection via Lombok patterns already used in the codebase. Do not introduce `Autowired` field injection
Applied to files:
booklore-api/src/main/java/org/booklore/service/reader/EpubReaderService.java
🔇 Additional comments (13)
booklore-api/src/main/java/org/booklore/util/epub/EpubContentReader.java (1)
31-56: LGTM!The migration to
org.grimmory.epub4jis clean. The method correctly validates spine bounds and resource existence before accessing data.booklore-api/src/test/java/org/booklore/util/epub/EpubContentReaderTest.java (1)
1-332: LGTM!The test migration is well done:
- Uses real EPUB generation via
EpubWriterinstead of mocks- Properly tests the behavior change where
getSpineSizenow throwsEpubReadExceptionfor non-existent files- Follows the coding guideline of using focused unit tests without
@SpringBootTestBased on learnings: "Prefer focused unit tests; use
SpringBootTestonly when the Spring context is required"booklore-api/src/test/java/org/booklore/util/koreader/EpubCfiServiceTest.java (1)
3-6: LGTM!Import-only change to align with the epub4j library migration. No test logic was modified.
booklore-api/build.gradle.kts (2)
29-31: LGTM on preview feature configuration.The
--enable-previewflag is consistently applied to:
- All
JavaCompiletasks (line 30)- Test execution (line 147)
bootRun(line 175)This aligns with the Dockerfile changes and supports Java preview features like guarded pattern matching (as shown in
AppSettingService.java).Also applies to: 147-147, 175-175
88-89: The code already uses org.grimmory.epub4j extensively; verify the build succeeds to confirm dependency availability.The codebase imports from
org.grimmory.epub4jpackages across multiple files (EpubContentReader, EpubReaderService, EpubMetadataWriter, EpubMetadataExtractor, and test files). Since the project uses these classes, the dependenciesorg.grimmory:epub4j-core:0.4.0andorg.grimmory:epub4j-native:0.4.0must be resolvable for the build to succeed. Theorg.grimmorygroup format (rather than the typicalcom.github.*format for JitPack) suggests a non-standard repository setup. Ensure the build succeeds to confirm these versions are available.booklore-api/src/main/java/org/booklore/service/metadata/writer/EpubMetadataWriter.java (2)
524-535: LGTM! Good security implementation for ZIP extraction.The path traversal check at lines 528-530 correctly prevents zip-slip attacks by validating that extracted paths stay within the target directory using
normalize()andstartsWith().
537-579: LGTM! EPUB ZIP creation follows spec requirements.The implementation correctly:
- Places
mimetypeas the first uncompressed (STORED) entry per EPUB specification- Computes and sets CRC32, size, and compressedSize for the STORED entry
- Handles directory entries with trailing slashes
- Converts path separators to forward slashes for cross-platform ZIP compatibility
booklore-api/src/main/java/org/booklore/service/reader/EpubReaderService.java (1)
223-231: LGTM on the core parsing migration.The migration to
EpubReader.readEpubLazy()and the new mapping methods (mapBookToInfo,mapManifest,mapSpine,mapMetadata,mapToc) are well-structured and correctly extract EPUB metadata using the new library.Also applies to: 233-271
booklore-api/src/test/java/org/booklore/service/reader/EpubReaderServiceTest.java (2)
60-90: LGTM! Excellent test fixture generation.The
writeTestEpub()method creates a realistic EPUB with:
- Proper metadata (title, author, language)
- Multiple chapters in the spine
- A stylesheet resource
- A cover image
This approach is superior to mocking ZIP structures as it validates the actual integration with the epub4j library.
Based on learnings: "Prefer focused unit tests; use
SpringBootTestonly when the Spring context is required" - this test correctly uses Mockito without requiring Spring context.
92-341: LGTM!The test suite comprehensively covers:
- Book info retrieval and caching
- File streaming with path validation
- Content type detection
- File size retrieval
- Spine ordering
- TOC parsing
- Manifest properties including cover detection
- Security (path traversal blocking)
booklore-api/src/main/java/org/booklore/service/metadata/extractor/EpubMetadataExtractor.java (3)
119-120: Unable to verify review comment — persistent infrastructure errors prevent access to the repository. Additionally, the file path (booklore-api/src/...) does not match the stated repository under review (grimmory-tools/grimmory). Manual verification is required to confirm:
- The actual code at lines 119-120 and 172-173
- Whether
EpubContainers.open()truly bypassesEpubReader/ResourcesLoaderprotections- The specific behavior of
ZipEpubContainerand its eager-loading implications- Whether the major issue classification is justified
129-130: ReplaceURLDecoderwithUriUtils.decode()for proper URI component decoding.
URLDecoderis designed for HTML form decoding (application/x-www-form-urlencoded) and converts+to space. This breaks archive entry lookups for valid assets likeImages/front+01.jpg, which would be resolved asImages/front 01.jpg. UseUriUtils.decode()instead to properly decode URI components per RFC 3986 without the+conversion.🩹 Suggested fix
- String href = URLDecoder.decode(item.getAttribute("href"), StandardCharsets.UTF_8); + String href = UriUtils.decode(item.getAttribute("href"), StandardCharsets.UTF_8.name());- String decodedHref = URLDecoder.decode(href, StandardCharsets.UTF_8); + String decodedHref = UriUtils.decode(href, StandardCharsets.UTF_8.name());import org.springframework.web.util.UriUtils;Also applies to lines 146-147.
123-125: Use a namespace-aware manifest lookup here.
parseOpf()is backed by a namespace-aware DOM builder, and later in this class you already querymetadatawithgetElementsByTagNameNS("*", "metadata"). UsinggetElementsByTagName("item")here makes the cover fallback depend on the OPF manifest using an unprefixed tag name;getElementsByTagNameNS("*", "item")is the safer match. This is an inference from the DOM contract plus epub4j's parser configuration.🩹 Suggested change
- NodeList items = opf.getElementsByTagName("item"); + NodeList items = opf.getElementsByTagNameNS("*", "item");
booklore-api/src/main/java/org/booklore/service/reader/EpubReaderService.java
Show resolved
Hide resolved
1 task
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
booklore-api/src/test/java/org/booklore/util/koreader/CfiConverterTest.java (1)
136-470: Optional: extract converter factory helper to reduce repeated setup.
new CfiConverter(new JsoupDocumentNavigator(...), spineIndex)is repeated many times; a tiny helper would improve readability and reduce churn if constructor wiring changes again.♻️ Proposed test refactor
class CfiConverterTest { private Document simpleDocument; private Document complexDocument; + + private CfiConverter converter(Document doc, int spineIndex) { + return new CfiConverter(new JsoupDocumentNavigator(doc), spineIndex); + } @@ - CfiConverter converter = new CfiConverter(new JsoupDocumentNavigator(simpleDocument), 0); + CfiConverter converter = converter(simpleDocument, 0);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@booklore-api/src/test/java/org/booklore/util/koreader/CfiConverterTest.java` around lines 136 - 470, Tests repeat constructing converters via new CfiConverter(new JsoupDocumentNavigator(...), spineIndex); extract a small private factory helper in the test class (e.g., a method named createConverter(Document doc, int spineIndex) or createConverter(int spineIndex) that returns CfiConverter) and update all tests to call that helper; reference CfiConverter and JsoupDocumentNavigator when locating the repeated constructor calls and ensure the helper centralizes creation to simplify future changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@booklore-api/src/main/java/org/booklore/util/koreader/EpubCfiService.java`:
- Around line 52-55: convertXPointerRangeToCfi currently only extracts the spine
index from startXPointer (via CfiConverter.extractSpineIndex) and uses that to
build a converter, which allows silently creating an invalid range if
endXPointer references a different spine; update convertXPointerRangeToCfi to
also extract the spine index from endXPointer, compare the two indices, and if
they differ throw an appropriate runtime exception (e.g.,
IllegalArgumentException) with a clear message; only proceed to call
createConverter(epubFile, spineIndex) and converter.xPointerToCfi(...) when the
two extracted spine indices match.
---
Nitpick comments:
In `@booklore-api/src/test/java/org/booklore/util/koreader/CfiConverterTest.java`:
- Around line 136-470: Tests repeat constructing converters via new
CfiConverter(new JsoupDocumentNavigator(...), spineIndex); extract a small
private factory helper in the test class (e.g., a method named
createConverter(Document doc, int spineIndex) or createConverter(int spineIndex)
that returns CfiConverter) and update all tests to call that helper; reference
CfiConverter and JsoupDocumentNavigator when locating the repeated constructor
calls and ensure the helper centralizes creation to simplify future changes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5f5446f7-9ee1-493f-8f60-4dc86ff9a787
📒 Files selected for processing (7)
booklore-api/build.gradle.ktsbooklore-api/src/main/java/org/booklore/service/metadata/extractor/EpubMetadataExtractor.javabooklore-api/src/main/java/org/booklore/util/koreader/CfiConvertor.javabooklore-api/src/main/java/org/booklore/util/koreader/EpubCfiService.javabooklore-api/src/main/java/org/booklore/util/koreader/JsoupDocumentNavigator.javabooklore-api/src/test/java/org/booklore/util/koreader/CfiConverterTest.javabooklore-api/src/test/java/org/booklore/util/koreader/EpubCfiServiceTest.java
💤 Files with no reviewable changes (1)
- booklore-api/src/main/java/org/booklore/util/koreader/CfiConvertor.java
🚧 Files skipped from review as they are similar to previous changes (2)
- booklore-api/src/test/java/org/booklore/util/koreader/EpubCfiServiceTest.java
- booklore-api/build.gradle.kts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Test Suite / Backend Tests
- GitHub Check: Test Suite / Frontend Tests
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (java-kotlin)
- GitHub Check: Analyze (actions)
🧰 Additional context used
📓 Path-based instructions (2)
booklore-api/src/**/*.java
📄 CodeRabbit inference engine (AGENTS.md)
booklore-api/src/**/*.java: Use 4-space indentation in Java files and match surrounding Java style
Prefer constructor injection via Lombok patterns already used in the codebase. Do not introduce@Autowiredfield injection
Use MapStruct for entity/DTO mapping
Files:
booklore-api/src/test/java/org/booklore/util/koreader/CfiConverterTest.javabooklore-api/src/main/java/org/booklore/util/koreader/JsoupDocumentNavigator.javabooklore-api/src/main/java/org/booklore/util/koreader/EpubCfiService.javabooklore-api/src/main/java/org/booklore/service/metadata/extractor/EpubMetadataExtractor.java
booklore-api/src/test/**/*.java
📄 CodeRabbit inference engine (AGENTS.md)
Prefer focused unit tests; use
@SpringBootTestonly when the Spring context is required
Files:
booklore-api/src/test/java/org/booklore/util/koreader/CfiConverterTest.java
🧠 Learnings (5)
📓 Common learnings
Learnt from: balazs-szucs
Repo: grimmory-tools/grimmory PR: 334
File: booklore-api/src/main/java/org/booklore/service/reader/EpubReaderService.java:402-407
Timestamp: 2026-04-02T09:25:37.417Z
Learning: In grimmory-tools/grimmory, epub processing uses `org.grimmory.epub4j` from https://github.com/grimmory-tools/epub4j. This is a grimmory-maintained fork and is fundamentally different from `io.documentnode.epub4j`. Do NOT generalize API knowledge, behavior, or limitations of `io.documentnode.epub4j` to the grimmory fork. Before raising review comments on epub-related file processing code, verify the current API and capabilities in the grimmory-tools/epub4j repository.
Learnt from: balazs-szucs
Repo: grimmory-tools/grimmory PR: 334
File: booklore-api/src/main/java/org/booklore/service/reader/EpubReaderService.java:402-407
Timestamp: 2026-04-02T09:25:37.417Z
Learning: In grimmory-tools/grimmory, before commenting on any file processing code (epub or pdf), always verify the current state of the relevant upstream grimmory libraries: `grimmory-tools/epub4j` (for epub) and `grimmory-tools/PDFium4j` (for pdf). These custom libraries may have different APIs, capabilities, and limitations compared to the third-party libraries they replace. Issues about streaming, buffering, or API surface may need to be filed/addressed in those upstream repos rather than in grimmory itself.
Learnt from: balazs-szucs
Repo: grimmory-tools/grimmory PR: 334
File: booklore-api/src/main/java/org/booklore/service/reader/EpubReaderService.java:402-407
Timestamp: 2026-04-02T09:25:37.417Z
Learning: In grimmory-tools/grimmory, PDF processing uses `grimmory PDFium4j` from https://github.com/grimmory-tools/PDFium4j. This is a grimmory-maintained library and is fundamentally different from Apache PDFBox. Do NOT generalize API knowledge, behavior, or limitations of PDFBox to grimmory PDFium4j. Before raising review comments on PDF-related file processing code, verify the current API and capabilities in the grimmory-tools/PDFium4j repository.
Learnt from: balazs-szucs
Repo: grimmory-tools/grimmory PR: 334
File: booklore-api/build.gradle.kts:0-0
Timestamp: 2026-04-02T09:12:48.158Z
Learning: In `grimmory-tools/grimmory`, nightcompress (`com.github.gotson.nightcompress:nightcompress`) is the preferred library for archive/unarchiving operations (including RAR and other formats). Flag any PR changes that remove or replace nightcompress with an alternative (e.g., junrar, commons-compress, JNI-based libs, or standard Java ZipInputStream/ZipFile) for unarchiving purposes. Nightcompress is considered better than JNI Java alternatives and generally better than standard Java unzip. Exceptions are acceptable when nightcompress cannot handle a specific format, but such trade-offs should be made explicit and confirmed by the maintainer.
📚 Learning: 2026-04-02T09:25:37.417Z
Learnt from: balazs-szucs
Repo: grimmory-tools/grimmory PR: 334
File: booklore-api/src/main/java/org/booklore/service/reader/EpubReaderService.java:402-407
Timestamp: 2026-04-02T09:25:37.417Z
Learning: In grimmory-tools/grimmory, epub processing uses `org.grimmory.epub4j` from https://github.com/grimmory-tools/epub4j. This is a grimmory-maintained fork and is fundamentally different from `io.documentnode.epub4j`. Do NOT generalize API knowledge, behavior, or limitations of `io.documentnode.epub4j` to the grimmory fork. Before raising review comments on epub-related file processing code, verify the current API and capabilities in the grimmory-tools/epub4j repository.
Applied to files:
booklore-api/src/test/java/org/booklore/util/koreader/CfiConverterTest.javabooklore-api/src/main/java/org/booklore/util/koreader/JsoupDocumentNavigator.java
📚 Learning: 2026-04-02T09:25:37.417Z
Learnt from: balazs-szucs
Repo: grimmory-tools/grimmory PR: 334
File: booklore-api/src/main/java/org/booklore/service/reader/EpubReaderService.java:402-407
Timestamp: 2026-04-02T09:25:37.417Z
Learning: In grimmory-tools/grimmory, before commenting on any file processing code (epub or pdf), always verify the current state of the relevant upstream grimmory libraries: `grimmory-tools/epub4j` (for epub) and `grimmory-tools/PDFium4j` (for pdf). These custom libraries may have different APIs, capabilities, and limitations compared to the third-party libraries they replace. Issues about streaming, buffering, or API surface may need to be filed/addressed in those upstream repos rather than in grimmory itself.
Applied to files:
booklore-api/src/test/java/org/booklore/util/koreader/CfiConverterTest.javabooklore-api/src/main/java/org/booklore/service/metadata/extractor/EpubMetadataExtractor.java
📚 Learning: 2026-04-02T09:25:37.417Z
Learnt from: balazs-szucs
Repo: grimmory-tools/grimmory PR: 334
File: booklore-api/src/main/java/org/booklore/service/reader/EpubReaderService.java:402-407
Timestamp: 2026-04-02T09:25:37.417Z
Learning: When reviewing epub-related file processing code in this project, remember that grimmory-tools/grimmory uses the `org.grimmory.epub4j` library (from grimmory-tools/epub4j) and that it is a grimmory-maintained fork fundamentally different from `io.documentnode.epub4j`. Do not assume the same API behavior, limitations, or semantics as `io.documentnode.epub4j`. Before flagging issues or proposing changes, verify the current API/capabilities in the grimmory-tools/epub4j repository.
Applied to files:
booklore-api/src/main/java/org/booklore/util/koreader/EpubCfiService.javabooklore-api/src/main/java/org/booklore/service/metadata/extractor/EpubMetadataExtractor.java
📚 Learning: 2026-03-24T18:46:47.249Z
Learnt from: pedronave
Repo: grimmory-tools/grimmory PR: 173
File: booklore-api/src/main/java/org/booklore/service/hardcover/HardcoverSyncService.java:188-199
Timestamp: 2026-03-24T18:46:47.249Z
Learning: In grimmory-tools/grimmory (HardcoverSyncService.java), when a `hardcoverBookId` is stored in book metadata but is malformed (non-numeric), the preferred behavior is to return null and skip the sync rather than falling back to ISBN-based lookup. The reasoning is that an explicitly set bookId represents deliberate user intent, and silently resolving via ISBN could sync progress to a different book than intended.
Applied to files:
booklore-api/src/main/java/org/booklore/service/metadata/extractor/EpubMetadataExtractor.java
🔇 Additional comments (7)
booklore-api/src/main/java/org/booklore/service/metadata/extractor/EpubMetadataExtractor.java (5)
3-11: LGTM!Clean import migration to the grimmory epub4j library with appropriate types for the new cover detection and container APIs.
107-154: LGTM!Well-structured fallback logic with proper resource management via try-with-resources. Good defensive programming with
container.exists()checks before reading, and comprehensive heuristics (OPF properties → id/href matching → filename scanning).
161-162: LGTM!Proper use of try-with-resources ensures the
EpubContaineris closed after metadata extraction.
561-583: LGTM!Correct path resolution logic that properly handles absolute paths, normalizes
..and.components, and avoids filesystem dependencies by operating purely on strings. The use ofLinkedListis appropriate for theremoveLast()operation.
549-549: No action needed. TheMediaType.name()method is the correct API for the grimmory epub4j fork (it's a Java record accessor) and correctly returns a MIME type string starting with "image" for image resources.booklore-api/src/main/java/org/booklore/util/koreader/JsoupDocumentNavigator.java (1)
25-72: Good adapter implementation for epub4j CFI navigation.
DocumentNavigatormethods consistently return Jsoup-backed nodes/elements and keep traversal behavior coherent for CFI conversion paths.booklore-api/src/main/java/org/booklore/util/koreader/EpubCfiService.java (1)
32-108: Migration to grimmory CFI types is clean and consistent.The service-level API/type migration to
CfiConverter/XPointerResultis consistently applied across conversion and validation methods.
booklore-api/src/main/java/org/booklore/util/koreader/EpubCfiService.java
Show resolved
Hide resolved
# Conflicts: # Dockerfile # booklore-api/build.gradle.kts
# Conflicts: # booklore-api/build.gradle.kts # booklore-api/src/main/java/org/booklore/service/reader/EpubReaderService.java
…n build configuration
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
booklore-api/src/main/java/org/booklore/util/koreader/EpubCfiService.java (1)
33-35: Route all spine-index parsing through theextractSpineIndex(...)wrapper method.Lines 34, 44, 80, and 91 directly call
CfiConverter.extractSpineIndex(...), while the local wrapper method at line 100 already exists. Using the wrapper everywhere would centralize this dependency and make it easy to add consistent error handling across all entry points, aligning with the project's preference forApiError-based exceptions.Suggested refactor
public XPointerResult convertCfiToXPointer(File epubFile, String cfi) { - int spineIndex = CfiConverter.extractSpineIndex(cfi); + int spineIndex = extractSpineIndex(cfi); CfiConverter converter = createConverter(epubFile, spineIndex); public String convertXPointerToCfi(File epubFile, String xpointer) { - int spineIndex = CfiConverter.extractSpineIndex(xpointer); + int spineIndex = extractSpineIndex(xpointer); CfiConverter converter = createConverter(epubFile, spineIndex); public boolean validateCfi(File epubFile, String cfi) { try { - int spineIndex = CfiConverter.extractSpineIndex(cfi); + int spineIndex = extractSpineIndex(cfi); CfiConverter converter = createConverter(epubFile, spineIndex); public boolean validateXPointer(File epubFile, String xpointer) { try { - int spineIndex = CfiConverter.extractSpineIndex(xpointer); + int spineIndex = extractSpineIndex(xpointer); CfiConverter converter = createConverter(epubFile, spineIndex);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@booklore-api/src/main/java/org/booklore/util/koreader/EpubCfiService.java` around lines 33 - 35, Replace every direct call to CfiConverter.extractSpineIndex(...) in this class with the class's existing local wrapper method (the extractSpineIndex wrapper defined near line 100) so that convertCfiToXPointer and the other methods currently calling CfiConverter.extractSpineIndex (the calls at lines you flagged) route through that wrapper; update createConverter and any other places using the static call to use the wrapper and ensure the wrapper throws/returns ApiError-style exceptions consistently for invalid input.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@booklore-api/src/main/java/org/booklore/util/koreader/EpubCfiService.java`:
- Around line 33-35: Replace every direct call to
CfiConverter.extractSpineIndex(...) in this class with the class's existing
local wrapper method (the extractSpineIndex wrapper defined near line 100) so
that convertCfiToXPointer and the other methods currently calling
CfiConverter.extractSpineIndex (the calls at lines you flagged) route through
that wrapper; update createConverter and any other places using the static call
to use the wrapper and ensure the wrapper throws/returns ApiError-style
exceptions consistently for invalid input.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a9440ab7-7c27-4f61-b8ea-6f49e6aeb659
📒 Files selected for processing (2)
booklore-api/build.gradle.ktsbooklore-api/src/main/java/org/booklore/util/koreader/EpubCfiService.java
🚧 Files skipped from review as they are similar to previous changes (1)
- booklore-api/build.gradle.kts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (1)
booklore-api/src/**/*.java
📄 CodeRabbit inference engine (AGENTS.md)
booklore-api/src/**/*.java: Use 4-space indentation in Java files and match surrounding Java style
Prefer constructor injection via Lombok patterns already used in the codebase. Do not introduce@Autowiredfield injection
Use MapStruct for entity/DTO mapping
Files:
booklore-api/src/main/java/org/booklore/util/koreader/EpubCfiService.java
🧠 Learnings (7)
📓 Common learnings
Learnt from: balazs-szucs
Repo: grimmory-tools/grimmory PR: 334
File: booklore-api/src/main/java/org/booklore/service/reader/EpubReaderService.java:402-407
Timestamp: 2026-04-02T09:25:37.417Z
Learning: In grimmory-tools/grimmory, epub processing uses `org.grimmory.epub4j` from https://github.com/grimmory-tools/epub4j. This is a grimmory-maintained fork and is fundamentally different from `io.documentnode.epub4j`. Do NOT generalize API knowledge, behavior, or limitations of `io.documentnode.epub4j` to the grimmory fork. Before raising review comments on epub-related file processing code, verify the current API and capabilities in the grimmory-tools/epub4j repository.
Learnt from: balazs-szucs
Repo: grimmory-tools/grimmory PR: 334
File: booklore-api/src/main/java/org/booklore/service/reader/EpubReaderService.java:402-407
Timestamp: 2026-04-02T09:25:37.417Z
Learning: In grimmory-tools/grimmory, before commenting on any file processing code (epub or pdf), always verify the current state of the relevant upstream grimmory libraries: `grimmory-tools/epub4j` (for epub) and `grimmory-tools/PDFium4j` (for pdf). These custom libraries may have different APIs, capabilities, and limitations compared to the third-party libraries they replace. Issues about streaming, buffering, or API surface may need to be filed/addressed in those upstream repos rather than in grimmory itself.
Learnt from: balazs-szucs
Repo: grimmory-tools/grimmory PR: 334
File: booklore-api/src/main/java/org/booklore/service/reader/EpubReaderService.java:402-407
Timestamp: 2026-04-02T09:25:37.417Z
Learning: In grimmory-tools/grimmory, PDF processing uses `grimmory PDFium4j` from https://github.com/grimmory-tools/PDFium4j. This is a grimmory-maintained library and is fundamentally different from Apache PDFBox. Do NOT generalize API knowledge, behavior, or limitations of PDFBox to grimmory PDFium4j. Before raising review comments on PDF-related file processing code, verify the current API and capabilities in the grimmory-tools/PDFium4j repository.
Learnt from: balazs-szucs
Repo: grimmory-tools/grimmory PR: 334
File: booklore-api/build.gradle.kts:0-0
Timestamp: 2026-04-02T09:12:48.158Z
Learning: In `grimmory-tools/grimmory`, nightcompress (`com.github.gotson.nightcompress:nightcompress`) is the preferred library for archive/unarchiving operations (including RAR and other formats). Flag any PR changes that remove or replace nightcompress with an alternative (e.g., junrar, commons-compress, JNI-based libs, or standard Java ZipInputStream/ZipFile) for unarchiving purposes. Nightcompress is considered better than JNI Java alternatives and generally better than standard Java unzip. Exceptions are acceptable when nightcompress cannot handle a specific format, but such trade-offs should be made explicit and confirmed by the maintainer.
📚 Learning: 2026-04-02T09:25:37.417Z
Learnt from: balazs-szucs
Repo: grimmory-tools/grimmory PR: 334
File: booklore-api/src/main/java/org/booklore/service/reader/EpubReaderService.java:402-407
Timestamp: 2026-04-02T09:25:37.417Z
Learning: When reviewing epub-related file processing code in this project, remember that grimmory-tools/grimmory uses the `org.grimmory.epub4j` library (from grimmory-tools/epub4j) and that it is a grimmory-maintained fork fundamentally different from `io.documentnode.epub4j`. Do not assume the same API behavior, limitations, or semantics as `io.documentnode.epub4j`. Before flagging issues or proposing changes, verify the current API/capabilities in the grimmory-tools/epub4j repository.
Applied to files:
booklore-api/src/main/java/org/booklore/util/koreader/EpubCfiService.java
📚 Learning: 2026-04-02T09:25:37.417Z
Learnt from: balazs-szucs
Repo: grimmory-tools/grimmory PR: 334
File: booklore-api/src/main/java/org/booklore/service/reader/EpubReaderService.java:402-407
Timestamp: 2026-04-02T09:25:37.417Z
Learning: In grimmory-tools/grimmory, before commenting on any file processing code (epub or pdf), always verify the current state of the relevant upstream grimmory libraries: `grimmory-tools/epub4j` (for epub) and `grimmory-tools/PDFium4j` (for pdf). These custom libraries may have different APIs, capabilities, and limitations compared to the third-party libraries they replace. Issues about streaming, buffering, or API surface may need to be filed/addressed in those upstream repos rather than in grimmory itself.
Applied to files:
booklore-api/src/main/java/org/booklore/util/koreader/EpubCfiService.java
📚 Learning: 2026-04-03T09:45:33.392Z
Learnt from: balazs-szucs
Repo: grimmory-tools/grimmory PR: 334
File: booklore-api/src/main/java/org/booklore/util/koreader/EpubCfiService.java:53-62
Timestamp: 2026-04-03T09:45:33.392Z
Learning: In grimmory-tools/grimmory, always prefer throwing errors using `ApiError` (e.g., `ApiError.INVALID_INPUT.createException("...")`) instead of generic Java exceptions like `IllegalArgumentException` or `RuntimeException`. When suggesting error handling in code reviews for this project, always recommend `ApiError`-based exceptions over raw Java exceptions.
Applied to files:
booklore-api/src/main/java/org/booklore/util/koreader/EpubCfiService.java
📚 Learning: 2026-03-24T18:46:47.249Z
Learnt from: pedronave
Repo: grimmory-tools/grimmory PR: 173
File: booklore-api/src/main/java/org/booklore/service/hardcover/HardcoverSyncService.java:188-199
Timestamp: 2026-03-24T18:46:47.249Z
Learning: In grimmory-tools/grimmory (HardcoverSyncService.java), when a `hardcoverBookId` is stored in book metadata but is malformed (non-numeric), the preferred behavior is to return null and skip the sync rather than falling back to ISBN-based lookup. The reasoning is that an explicitly set bookId represents deliberate user intent, and silently resolving via ISBN could sync progress to a different book than intended.
Applied to files:
booklore-api/src/main/java/org/booklore/util/koreader/EpubCfiService.java
📚 Learning: 2026-04-02T09:25:37.417Z
Learnt from: balazs-szucs
Repo: grimmory-tools/grimmory PR: 334
File: booklore-api/src/main/java/org/booklore/service/reader/EpubReaderService.java:402-407
Timestamp: 2026-04-02T09:25:37.417Z
Learning: In grimmory-tools/grimmory, PDF processing uses `grimmory PDFium4j` from https://github.com/grimmory-tools/PDFium4j. This is a grimmory-maintained library and is fundamentally different from Apache PDFBox. Do NOT generalize API knowledge, behavior, or limitations of PDFBox to grimmory PDFium4j. Before raising review comments on PDF-related file processing code, verify the current API and capabilities in the grimmory-tools/PDFium4j repository.
Applied to files:
booklore-api/src/main/java/org/booklore/util/koreader/EpubCfiService.java
📚 Learning: 2026-03-24T18:58:08.199Z
Learnt from: pedronave
Repo: grimmory-tools/grimmory PR: 173
File: booklore-api/src/main/java/org/booklore/service/hardcover/HardcoverSyncService.java:291-294
Timestamp: 2026-03-24T18:58:08.199Z
Learning: In grimmory-tools/grimmory (HardcoverSyncService.java), when a Hardcover edition has no page count, falling back to the book-level page count (book.pages) is the preferred behavior. Skipping the sync entirely in this case is considered worse than using a slightly approximate total, because the page count is only a proxy for converting progress % to page numbers for the API. The Hardcover API allows setting page-based progress even when the edition lacks a page count, and progress displays in at least some UI views (e.g., the book detail dropdown).
Applied to files:
booklore-api/src/main/java/org/booklore/util/koreader/EpubCfiService.java
🔇 Additional comments (1)
booklore-api/src/main/java/org/booklore/util/koreader/EpubCfiService.java (1)
53-62: Nice early rejection of mixed-spine ranges.Failing fast before
xPointerToCfi(...)sees a cross-spine range is the right safeguard here, and theApiError.INVALID_INPUTpath matches the project's preferred input-error handling.Based on learnings, "In grimmory-tools/grimmory, always prefer throwing errors using
ApiError(e.g.,ApiError.INVALID_INPUT.createException("...")) instead of generic Java exceptions likeIllegalArgumentExceptionorRuntimeException."
zachyale
reviewed
Apr 8, 2026
booklore-api/src/main/java/org/booklore/service/reader/EpubReaderService.java
Outdated
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
booklore-api/src/main/java/org/booklore/service/metadata/writer/EpubMetadataWriter.java (1)
75-80:⚠️ Potential issue | 🟠 MajorUse the container-reported OPF path instead of scanning for the first
.opf.
findOpfFile()selects the first.opffile encountered during directory traversal rather than using the canonical package path fromcontainer.xml. In archives with multiple.opffiles, this can cause metadata edits to be applied to the wrong package whileapplyCoverImageToEpub()later resolves cover paths relative to the true rootfile, creating internal EPUB inconsistencies.EpubContainerexposes the authoritative OPF path viagetOpfName(), which is already used elsewhere in the codebase.Refactor
extractZipToDirectory()to return the OPF path by callingcontainer.getOpfName()before the container closes, eliminating the need forfindOpfFile()at both call sites (lines 75–80 and 364–369).Suggested implementation
- private void extractZipToDirectory(File zipSource, Path targetDir) throws IOException { + private Path extractZipToDirectory(File zipSource, Path targetDir) throws IOException { try (EpubContainer container = EpubContainers.open(zipSource.toPath())) { for (String name : container.listAllFiles()) { Path entryPath = targetDir.resolve(name).normalize(); if (!entryPath.startsWith(targetDir)) { throw new IOException("ZIP entry outside target directory: " + name); } Files.createDirectories(entryPath.getParent()); try (OutputStream out = Files.newOutputStream(entryPath)) { container.streamTo(name, out); } } + + Path opfPath = targetDir.resolve(container.getOpfName()).normalize(); + if (!opfPath.startsWith(targetDir) || !Files.isRegularFile(opfPath)) { + throw new IOException("Invalid OPF path: " + container.getOpfName()); + } + return opfPath; } }Update call sites:
- extractZipToDirectory(epubFile, tempDir); - File opfFile = findOpfFile(tempDir.toFile()); - if (opfFile == null) { - log.warn("Could not locate OPF file in EPUB"); - return; - } + File opfFile = extractZipToDirectory(epubFile, tempDir).toFile();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@booklore-api/src/main/java/org/booklore/service/metadata/writer/EpubMetadataWriter.java` around lines 75 - 80, Refactor extractZipToDirectory to return the authoritative OPF path (String) by opening the EPUB container inside extractZipToDirectory, calling container.getOpfName() before closing the container, and returning that path along with extracting files; then update callers (the two sites that currently call extractZipToDirectory and then findOpfFile) to accept the returned opfPath and use it instead of calling findOpfFile(), and remove the now-unnecessary findOpfFile usage; ensure applyCoverImageToEpub and any metadata-writing logic resolve paths relative to the returned opfPath/rootfile.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@booklore-api/src/main/java/org/booklore/service/metadata/writer/EpubMetadataWriter.java`:
- Around line 543-558: The code currently logs a warning when the EPUB
`mimetype` file is missing; instead, in EpubMetadataWriter (the method that
opens the ZipOutputStream `zos` and reads `Path mimetypeFile`) make this a hard
failure by throwing an IOException with a clear message if
Files.exists(mimetypeFile) is false, and perform that existence check before
writing any entries so the archive is not created; preserve the existing logic
that writes the mimetype as an uncompressed STORED ZipEntry with correct size,
crc and name "mimetype" when present, but abort by throwing the IOException (no
warning-only) if the file is absent.
---
Outside diff comments:
In
`@booklore-api/src/main/java/org/booklore/service/metadata/writer/EpubMetadataWriter.java`:
- Around line 75-80: Refactor extractZipToDirectory to return the authoritative
OPF path (String) by opening the EPUB container inside extractZipToDirectory,
calling container.getOpfName() before closing the container, and returning that
path along with extracting files; then update callers (the two sites that
currently call extractZipToDirectory and then findOpfFile) to accept the
returned opfPath and use it instead of calling findOpfFile(), and remove the
now-unnecessary findOpfFile usage; ensure applyCoverImageToEpub and any
metadata-writing logic resolve paths relative to the returned opfPath/rootfile.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e9beb927-c4f1-49ce-ac8a-77fbfde64107
📒 Files selected for processing (5)
booklore-api/build.gradle.ktsbooklore-api/src/main/java/org/booklore/service/metadata/extractor/EpubMetadataExtractor.javabooklore-api/src/main/java/org/booklore/service/metadata/writer/EpubMetadataWriter.javabooklore-api/src/main/java/org/booklore/service/reader/EpubReaderService.javabooklore-api/src/main/java/org/booklore/util/epub/EpubContentReader.java
🚧 Files skipped from review as they are similar to previous changes (3)
- booklore-api/src/main/java/org/booklore/util/epub/EpubContentReader.java
- booklore-api/build.gradle.kts
- booklore-api/src/main/java/org/booklore/service/metadata/extractor/EpubMetadataExtractor.java
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Test Suite / Frontend Tests
- GitHub Check: Test Suite / Backend Tests
- GitHub Check: Analyze (java-kotlin)
🧰 Additional context used
📓 Path-based instructions (1)
booklore-api/src/**/*.java
📄 CodeRabbit inference engine (AGENTS.md)
booklore-api/src/**/*.java: Use 4-space indentation in Java files and match surrounding Java style
Prefer constructor injection via Lombok patterns already used in the codebase. Do not introduce@Autowiredfield injection
Use MapStruct for entity/DTO mapping
Files:
booklore-api/src/main/java/org/booklore/service/metadata/writer/EpubMetadataWriter.javabooklore-api/src/main/java/org/booklore/service/reader/EpubReaderService.java
🧠 Learnings (13)
📓 Common learnings
Learnt from: balazs-szucs
Repo: grimmory-tools/grimmory PR: 334
File: booklore-api/src/main/java/org/booklore/service/reader/EpubReaderService.java:402-407
Timestamp: 2026-04-02T09:25:48.330Z
Learning: In grimmory-tools/grimmory, epub processing uses `org.grimmory.epub4j` from https://github.com/grimmory-tools/epub4j. This is a grimmory-maintained fork and is fundamentally different from `io.documentnode.epub4j`. Do NOT generalize API knowledge, behavior, or limitations of `io.documentnode.epub4j` to the grimmory fork. Before raising review comments on epub-related file processing code, verify the current API and capabilities in the grimmory-tools/epub4j repository.
Learnt from: balazs-szucs
Repo: grimmory-tools/grimmory PR: 334
File: booklore-api/src/main/java/org/booklore/service/reader/EpubReaderService.java:402-407
Timestamp: 2026-04-02T09:25:48.330Z
Learning: In grimmory-tools/grimmory, before commenting on any file processing code (epub or pdf), always verify the current state of the relevant upstream grimmory libraries: `grimmory-tools/epub4j` (for epub) and `grimmory-tools/PDFium4j` (for pdf). These custom libraries may have different APIs, capabilities, and limitations compared to the third-party libraries they replace. Issues about streaming, buffering, or API surface may need to be filed/addressed in those upstream repos rather than in grimmory itself.
Learnt from: balazs-szucs
Repo: grimmory-tools/grimmory PR: 334
File: booklore-api/src/main/java/org/booklore/service/reader/EpubReaderService.java:402-407
Timestamp: 2026-04-02T09:25:48.330Z
Learning: In grimmory-tools/grimmory, PDF processing uses `grimmory PDFium4j` from https://github.com/grimmory-tools/PDFium4j. This is a grimmory-maintained library and is fundamentally different from Apache PDFBox. Do NOT generalize API knowledge, behavior, or limitations of PDFBox to grimmory PDFium4j. Before raising review comments on PDF-related file processing code, verify the current API and capabilities in the grimmory-tools/PDFium4j repository.
Learnt from: balazs-szucs
Repo: grimmory-tools/grimmory PR: 334
File: booklore-api/build.gradle.kts:0-0
Timestamp: 2026-04-02T09:12:50.790Z
Learning: In `grimmory-tools/grimmory`, nightcompress (`com.github.gotson.nightcompress:nightcompress`) is the preferred library for archive/unarchiving operations (including RAR and other formats). Flag any PR changes that remove or replace nightcompress with an alternative (e.g., junrar, commons-compress, JNI-based libs, or standard Java ZipInputStream/ZipFile) for unarchiving purposes. Nightcompress is considered better than JNI Java alternatives and generally better than standard Java unzip. Exceptions are acceptable when nightcompress cannot handle a specific format, but such trade-offs should be made explicit and confirmed by the maintainer.
📚 Learning: 2026-04-02T09:25:37.417Z
Learnt from: balazs-szucs
Repo: grimmory-tools/grimmory PR: 334
File: booklore-api/src/main/java/org/booklore/service/reader/EpubReaderService.java:402-407
Timestamp: 2026-04-02T09:25:37.417Z
Learning: When reviewing epub-related file processing code in this project, remember that grimmory-tools/grimmory uses the `org.grimmory.epub4j` library (from grimmory-tools/epub4j) and that it is a grimmory-maintained fork fundamentally different from `io.documentnode.epub4j`. Do not assume the same API behavior, limitations, or semantics as `io.documentnode.epub4j`. Before flagging issues or proposing changes, verify the current API/capabilities in the grimmory-tools/epub4j repository.
Applied to files:
booklore-api/src/main/java/org/booklore/service/metadata/writer/EpubMetadataWriter.javabooklore-api/src/main/java/org/booklore/service/reader/EpubReaderService.java
📚 Learning: 2026-04-02T09:25:48.330Z
Learnt from: balazs-szucs
Repo: grimmory-tools/grimmory PR: 334
File: booklore-api/src/main/java/org/booklore/service/reader/EpubReaderService.java:402-407
Timestamp: 2026-04-02T09:25:48.330Z
Learning: In grimmory-tools/grimmory, before commenting on any file processing code (epub or pdf), always verify the current state of the relevant upstream grimmory libraries: `grimmory-tools/epub4j` (for epub) and `grimmory-tools/PDFium4j` (for pdf). These custom libraries may have different APIs, capabilities, and limitations compared to the third-party libraries they replace. Issues about streaming, buffering, or API surface may need to be filed/addressed in those upstream repos rather than in grimmory itself.
Applied to files:
booklore-api/src/main/java/org/booklore/service/metadata/writer/EpubMetadataWriter.java
📚 Learning: 2026-04-02T09:06:44.150Z
Learnt from: balazs-szucs
Repo: grimmory-tools/grimmory PR: 334
File: booklore-api/build.gradle.kts:94-95
Timestamp: 2026-04-02T09:06:44.150Z
Learning: In `grimmory-tools/grimmory`, the project is committed to `com.github.gotson.nightcompress:nightcompress` for archive/RAR support. Do not suggest replacing it with junrar or any other archive library. `ArchiveService.java` uses nightcompress APIs (`Archive`, `ArchiveEntry`, `LibArchiveException`).
Applied to files:
booklore-api/src/main/java/org/booklore/service/metadata/writer/EpubMetadataWriter.javabooklore-api/src/main/java/org/booklore/service/reader/EpubReaderService.java
📚 Learning: 2026-04-02T09:12:50.790Z
Learnt from: balazs-szucs
Repo: grimmory-tools/grimmory PR: 334
File: booklore-api/build.gradle.kts:0-0
Timestamp: 2026-04-02T09:12:50.790Z
Learning: In `grimmory-tools/grimmory`, nightcompress (`com.github.gotson.nightcompress:nightcompress`) is the preferred library for archive/unarchiving operations (including RAR and other formats). Flag any PR changes that remove or replace nightcompress with an alternative (e.g., junrar, commons-compress, JNI-based libs, or standard Java ZipInputStream/ZipFile) for unarchiving purposes. Nightcompress is considered better than JNI Java alternatives and generally better than standard Java unzip. Exceptions are acceptable when nightcompress cannot handle a specific format, but such trade-offs should be made explicit and confirmed by the maintainer.
Applied to files:
booklore-api/src/main/java/org/booklore/service/metadata/writer/EpubMetadataWriter.javabooklore-api/src/main/java/org/booklore/service/reader/EpubReaderService.java
📚 Learning: 2026-04-02T09:25:37.417Z
Learnt from: balazs-szucs
Repo: grimmory-tools/grimmory PR: 334
File: booklore-api/src/main/java/org/booklore/service/reader/EpubReaderService.java:402-407
Timestamp: 2026-04-02T09:25:37.417Z
Learning: When reviewing code in grimmory-tools/grimmory that processes EPUB or PDF files (e.g., readers/services), first verify the current status and APIs of the corresponding upstream grimmory replacement libraries: `grimmory-tools/epub4j` for EPUB and `grimmory-tools/PDFium4j` for PDF. If an issue relates to streaming/buffering behavior or mismatched API capabilities/surface, prefer filing or fixing it in the relevant upstream repo (epub4j/PDFium4j) rather than changing grimmory itself, unless the upstream integration is confirmed to be correct and the defect is in grimmory.
Applied to files:
booklore-api/src/main/java/org/booklore/service/reader/EpubReaderService.java
📚 Learning: 2026-04-02T09:25:48.330Z
Learnt from: balazs-szucs
Repo: grimmory-tools/grimmory PR: 334
File: booklore-api/src/main/java/org/booklore/service/reader/EpubReaderService.java:402-407
Timestamp: 2026-04-02T09:25:48.330Z
Learning: In grimmory-tools/grimmory, PDF processing uses `grimmory PDFium4j` from https://github.com/grimmory-tools/PDFium4j. This is a grimmory-maintained library and is fundamentally different from Apache PDFBox. Do NOT generalize API knowledge, behavior, or limitations of PDFBox to grimmory PDFium4j. Before raising review comments on PDF-related file processing code, verify the current API and capabilities in the grimmory-tools/PDFium4j repository.
Applied to files:
booklore-api/src/main/java/org/booklore/service/reader/EpubReaderService.java
📚 Learning: 2026-03-24T18:46:47.249Z
Learnt from: pedronave
Repo: grimmory-tools/grimmory PR: 173
File: booklore-api/src/main/java/org/booklore/service/hardcover/HardcoverSyncService.java:188-199
Timestamp: 2026-03-24T18:46:47.249Z
Learning: In grimmory-tools/grimmory (HardcoverSyncService.java), when a `hardcoverBookId` is stored in book metadata but is malformed (non-numeric), the preferred behavior is to return null and skip the sync rather than falling back to ISBN-based lookup. The reasoning is that an explicitly set bookId represents deliberate user intent, and silently resolving via ISBN could sync progress to a different book than intended.
Applied to files:
booklore-api/src/main/java/org/booklore/service/reader/EpubReaderService.java
📚 Learning: 2026-03-24T18:58:08.199Z
Learnt from: pedronave
Repo: grimmory-tools/grimmory PR: 173
File: booklore-api/src/main/java/org/booklore/service/hardcover/HardcoverSyncService.java:291-294
Timestamp: 2026-03-24T18:58:08.199Z
Learning: In grimmory-tools/grimmory (HardcoverSyncService.java), when a Hardcover edition has no page count, falling back to the book-level page count (book.pages) is the preferred behavior. Skipping the sync entirely in this case is considered worse than using a slightly approximate total, because the page count is only a proxy for converting progress % to page numbers for the API. The Hardcover API allows setting page-based progress even when the edition lacks a page count, and progress displays in at least some UI views (e.g., the book detail dropdown).
Applied to files:
booklore-api/src/main/java/org/booklore/service/reader/EpubReaderService.java
📚 Learning: 2026-04-01T17:50:06.817Z
Learnt from: pedronave
Repo: grimmory-tools/grimmory PR: 247
File: booklore-api/src/main/java/org/booklore/service/hardcover/HardcoverSyncService.java:156-159
Timestamp: 2026-04-01T17:50:06.817Z
Learning: In grimmory-tools/grimmory (HardcoverSyncService.java), the Hardcover API already returns `user_book_reads` in a consistent, ordered fashion (ascending by recency), so `getLast()` reliably retrieves the most recent read entry. Adding an explicit `order_by` to the `user_book_reads` query is unnecessary.
Applied to files:
booklore-api/src/main/java/org/booklore/service/reader/EpubReaderService.java
📚 Learning: 2026-04-04T15:36:56.558Z
Learnt from: balazs-szucs
Repo: grimmory-tools/grimmory PR: 372
File: booklore-api/src/main/java/org/booklore/app/service/AppBookService.java:357-359
Timestamp: 2026-04-04T15:36:56.558Z
Learning: In `booklore-api/src/main/java/org/booklore/app/service/AppBookService.java`, `shelfId` and `magicShelfId` are mutually exclusive navigation contexts in `getFilterOptions`. A request will never supply both at the same time, so the `else if (shelfId != null)` branch after the `magicBookIds != null` check is intentional and correct — there is no missing shelf-∩-magicShelf intersection case. Do not flag this pattern as a bug.
Applied to files:
booklore-api/src/main/java/org/booklore/service/reader/EpubReaderService.java
📚 Learning: 2026-03-25T19:09:09.638Z
Learnt from: imnotjames
Repo: grimmory-tools/grimmory PR: 189
File: booklore-api/src/main/java/org/booklore/service/kobo/KoboLibrarySyncService.java:113-116
Timestamp: 2026-03-25T19:09:09.638Z
Learning: In `booklore-api/src/main/java/org/booklore/service/kobo/KoboLibrarySyncService.java`, when deciding whether to forward the `Authorization` header to the upstream Kobo server proxy on the `/v1/library/sync` endpoint, a simple null check (`request.getHeader(HttpHeaders.AUTHORIZATION) != null`) is the correct guard. The header is either absent or present — if present it may or may not be a valid token, but validity is determined by the upstream Kobo server, not by Grimmory. Blank/whitespace intermediate states do not occur in practice, and pre-validating the token value before forwarding is not the responsibility of this service.
Applied to files:
booklore-api/src/main/java/org/booklore/service/reader/EpubReaderService.java
📚 Learning: 2026-03-26T01:46:48.863Z
Learnt from: CR
Repo: grimmory-tools/grimmory PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-26T01:46:48.863Z
Learning: Applies to booklore-api/src/**/*.java : Prefer constructor injection via Lombok patterns already used in the codebase. Do not introduce `Autowired` field injection
Applied to files:
booklore-api/src/main/java/org/booklore/service/reader/EpubReaderService.java
booklore-api/src/main/java/org/booklore/service/metadata/writer/EpubMetadataWriter.java
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
booklore-api/src/main/java/org/booklore/util/koreader/EpubCfiService.java (1)
52-62: Consider using consistent API call style throughout the class.Lines 53-54 call the instance wrapper
extractSpineIndex(), while lines 33, 43, 79, and 90 callCfiConverter.extractSpineIndex()directly. Both work identically, but for consistency within the class, consider using one style throughout.♻️ Suggested change for consistency
public String convertXPointerRangeToCfi(File epubFile, String startXPointer, String endXPointer) { - int startSpineIndex = extractSpineIndex(startXPointer); - int endSpineIndex = extractSpineIndex(endXPointer); + int startSpineIndex = CfiConverter.extractSpineIndex(startXPointer); + int endSpineIndex = CfiConverter.extractSpineIndex(endXPointer); if (startSpineIndex != endSpineIndex) { throw ApiError.INVALID_INPUT.createException("Start and end XPointers must reference the same spine index"); } CfiConverter converter = createConverter(epubFile, startSpineIndex); return converter.xPointerToCfi(startXPointer, endXPointer); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@booklore-api/src/main/java/org/booklore/util/koreader/EpubCfiService.java` around lines 52 - 62, convertXPointerRangeToCfi uses the instance method extractSpineIndex() while other places call the static/class method CfiConverter.extractSpineIndex(); change the two calls in convertXPointerRangeToCfi to use CfiConverter.extractSpineIndex(startXPointer) and CfiConverter.extractSpineIndex(endXPointer) so the class consistently uses the static/class-style call alongside other usages (leave createConverter(...) and converter.xPointerToCfi(...) unchanged).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@booklore-api/src/main/java/org/booklore/util/koreader/EpubCfiService.java`:
- Around line 52-62: convertXPointerRangeToCfi uses the instance method
extractSpineIndex() while other places call the static/class method
CfiConverter.extractSpineIndex(); change the two calls in
convertXPointerRangeToCfi to use CfiConverter.extractSpineIndex(startXPointer)
and CfiConverter.extractSpineIndex(endXPointer) so the class consistently uses
the static/class-style call alongside other usages (leave createConverter(...)
and converter.xPointerToCfi(...) unchanged).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b3193ef1-b929-4bff-92ec-c674e7b628b9
📒 Files selected for processing (1)
booklore-api/src/main/java/org/booklore/util/koreader/EpubCfiService.java
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Test Suite / Frontend Tests
- GitHub Check: Test Suite / Backend Tests
- GitHub Check: Analyze (java-kotlin)
🧰 Additional context used
📓 Path-based instructions (1)
booklore-api/src/**/*.java
📄 CodeRabbit inference engine (AGENTS.md)
booklore-api/src/**/*.java: Use 4-space indentation in Java files and match surrounding Java style
Prefer constructor injection via Lombok patterns already used in the codebase. Do not introduce@Autowiredfield injection
Use MapStruct for entity/DTO mapping
Files:
booklore-api/src/main/java/org/booklore/util/koreader/EpubCfiService.java
🧠 Learnings (7)
📓 Common learnings
Learnt from: balazs-szucs
Repo: grimmory-tools/grimmory PR: 334
File: booklore-api/src/main/java/org/booklore/service/reader/EpubReaderService.java:402-407
Timestamp: 2026-04-02T09:25:48.330Z
Learning: In grimmory-tools/grimmory, epub processing uses `org.grimmory.epub4j` from https://github.com/grimmory-tools/epub4j. This is a grimmory-maintained fork and is fundamentally different from `io.documentnode.epub4j`. Do NOT generalize API knowledge, behavior, or limitations of `io.documentnode.epub4j` to the grimmory fork. Before raising review comments on epub-related file processing code, verify the current API and capabilities in the grimmory-tools/epub4j repository.
Learnt from: balazs-szucs
Repo: grimmory-tools/grimmory PR: 334
File: booklore-api/src/main/java/org/booklore/service/reader/EpubReaderService.java:402-407
Timestamp: 2026-04-02T09:25:48.330Z
Learning: In grimmory-tools/grimmory, before commenting on any file processing code (epub or pdf), always verify the current state of the relevant upstream grimmory libraries: `grimmory-tools/epub4j` (for epub) and `grimmory-tools/PDFium4j` (for pdf). These custom libraries may have different APIs, capabilities, and limitations compared to the third-party libraries they replace. Issues about streaming, buffering, or API surface may need to be filed/addressed in those upstream repos rather than in grimmory itself.
Learnt from: balazs-szucs
Repo: grimmory-tools/grimmory PR: 334
File: booklore-api/src/main/java/org/booklore/service/reader/EpubReaderService.java:402-407
Timestamp: 2026-04-02T09:25:48.330Z
Learning: In grimmory-tools/grimmory, PDF processing uses `grimmory PDFium4j` from https://github.com/grimmory-tools/PDFium4j. This is a grimmory-maintained library and is fundamentally different from Apache PDFBox. Do NOT generalize API knowledge, behavior, or limitations of PDFBox to grimmory PDFium4j. Before raising review comments on PDF-related file processing code, verify the current API and capabilities in the grimmory-tools/PDFium4j repository.
📚 Learning: 2026-04-02T09:25:37.417Z
Learnt from: balazs-szucs
Repo: grimmory-tools/grimmory PR: 334
File: booklore-api/src/main/java/org/booklore/service/reader/EpubReaderService.java:402-407
Timestamp: 2026-04-02T09:25:37.417Z
Learning: When reviewing epub-related file processing code in this project, remember that grimmory-tools/grimmory uses the `org.grimmory.epub4j` library (from grimmory-tools/epub4j) and that it is a grimmory-maintained fork fundamentally different from `io.documentnode.epub4j`. Do not assume the same API behavior, limitations, or semantics as `io.documentnode.epub4j`. Before flagging issues or proposing changes, verify the current API/capabilities in the grimmory-tools/epub4j repository.
Applied to files:
booklore-api/src/main/java/org/booklore/util/koreader/EpubCfiService.java
📚 Learning: 2026-04-02T09:25:48.330Z
Learnt from: balazs-szucs
Repo: grimmory-tools/grimmory PR: 334
File: booklore-api/src/main/java/org/booklore/service/reader/EpubReaderService.java:402-407
Timestamp: 2026-04-02T09:25:48.330Z
Learning: In grimmory-tools/grimmory, before commenting on any file processing code (epub or pdf), always verify the current state of the relevant upstream grimmory libraries: `grimmory-tools/epub4j` (for epub) and `grimmory-tools/PDFium4j` (for pdf). These custom libraries may have different APIs, capabilities, and limitations compared to the third-party libraries they replace. Issues about streaming, buffering, or API surface may need to be filed/addressed in those upstream repos rather than in grimmory itself.
Applied to files:
booklore-api/src/main/java/org/booklore/util/koreader/EpubCfiService.java
📚 Learning: 2026-04-03T09:45:37.512Z
Learnt from: balazs-szucs
Repo: grimmory-tools/grimmory PR: 334
File: booklore-api/src/main/java/org/booklore/util/koreader/EpubCfiService.java:53-62
Timestamp: 2026-04-03T09:45:37.512Z
Learning: In grimmory-tools/grimmory, always prefer throwing errors using `ApiError` (e.g., `ApiError.INVALID_INPUT.createException("...")`) instead of generic Java exceptions like `IllegalArgumentException` or `RuntimeException`. When suggesting error handling in code reviews for this project, always recommend `ApiError`-based exceptions over raw Java exceptions.
Applied to files:
booklore-api/src/main/java/org/booklore/util/koreader/EpubCfiService.java
📚 Learning: 2026-03-24T18:46:47.249Z
Learnt from: pedronave
Repo: grimmory-tools/grimmory PR: 173
File: booklore-api/src/main/java/org/booklore/service/hardcover/HardcoverSyncService.java:188-199
Timestamp: 2026-03-24T18:46:47.249Z
Learning: In grimmory-tools/grimmory (HardcoverSyncService.java), when a `hardcoverBookId` is stored in book metadata but is malformed (non-numeric), the preferred behavior is to return null and skip the sync rather than falling back to ISBN-based lookup. The reasoning is that an explicitly set bookId represents deliberate user intent, and silently resolving via ISBN could sync progress to a different book than intended.
Applied to files:
booklore-api/src/main/java/org/booklore/util/koreader/EpubCfiService.java
📚 Learning: 2026-04-02T09:25:48.330Z
Learnt from: balazs-szucs
Repo: grimmory-tools/grimmory PR: 334
File: booklore-api/src/main/java/org/booklore/service/reader/EpubReaderService.java:402-407
Timestamp: 2026-04-02T09:25:48.330Z
Learning: In grimmory-tools/grimmory, PDF processing uses `grimmory PDFium4j` from https://github.com/grimmory-tools/PDFium4j. This is a grimmory-maintained library and is fundamentally different from Apache PDFBox. Do NOT generalize API knowledge, behavior, or limitations of PDFBox to grimmory PDFium4j. Before raising review comments on PDF-related file processing code, verify the current API and capabilities in the grimmory-tools/PDFium4j repository.
Applied to files:
booklore-api/src/main/java/org/booklore/util/koreader/EpubCfiService.java
📚 Learning: 2026-03-24T18:58:08.199Z
Learnt from: pedronave
Repo: grimmory-tools/grimmory PR: 173
File: booklore-api/src/main/java/org/booklore/service/hardcover/HardcoverSyncService.java:291-294
Timestamp: 2026-03-24T18:58:08.199Z
Learning: In grimmory-tools/grimmory (HardcoverSyncService.java), when a Hardcover edition has no page count, falling back to the book-level page count (book.pages) is the preferred behavior. Skipping the sync entirely in this case is considered worse than using a slightly approximate total, because the page count is only a proxy for converting progress % to page numbers for the API. The Hardcover API allows setting page-based progress even when the edition lacks a page count, and progress displays in at least some UI views (e.g., the book detail dropdown).
Applied to files:
booklore-api/src/main/java/org/booklore/util/koreader/EpubCfiService.java
🔇 Additional comments (7)
booklore-api/src/main/java/org/booklore/util/koreader/EpubCfiService.java (7)
3-12: LGTM!The imports are correctly updated to use the new
org.grimmory.epub4j.cfipackage forCfiConverterandXPointerResult, and theApiErrorimport supports the improved error handling.
32-40: LGTM!The migration to the new
XPointerResultreturn type and usage ofCfiConverter.extractSpineIndex()is clean. ThePathoverload correctly delegates to theFileversion.
42-50: LGTM!Method correctly uses the new library's static
CfiConverter.extractSpineIndex()and instancexPointerToCfi()methods.
68-75: LGTM!The method correctly chains the new library's
XPointerResult.getXpointer()withCfiConverter.normalizeProgressXPointer()for progress normalization.
77-97: LGTM!Both validation methods follow a clean pattern: extract spine index, create converter, validate, and gracefully return
falseon any exception. The DEBUG-level logging for validation failures is appropriate.
99-101: LGTM!The wrapper provides a clean public API for external callers to extract spine indices without coupling to
CfiConverterdirectly.
107-114: LGTM!The converter creation correctly uses the new library's constructor with
JsoupDocumentNavigator, bridging Jsoup document parsing with epub4j's CFI navigation capabilities.
zachyale
approved these changes
Apr 9, 2026
Member
zachyale
left a comment
zachyale
left a comment
There was a problem hiding this comment.
Thank you for engaging in such a constructive debate about this- looks good to me!
😄 🚢 🚀
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
Linked Issue: Fixes #
Changes
This pull request modernizes EPUB handling in the codebase by switching from the legacy
io.documentnode.epub4jlibrary to the neworg.grimmory.epub4jfork, and by removing the dependency onzip4jin favor of standard Java ZIP utilities. It also updates JVM and build configurations to enable Java preview features and native access, and improves cover extraction logic for EPUB files.EPUB Library Migration and Refactoring:
io.documentnode.epub4jwithorg.grimmory.epub4jthroughout the codebase, including imports and EPUB processing logic inEpubMetadataExtractor.javaandEpubMetadataWriter.java.EpubContainerandCoverDetectorfrom the new library, improving robustness and removing custom XML/ZIP heuristics.zip4jfor ZIP file handling, replacing it with standard Java ZIP utilities and new helper methods for extracting and creating EPUB archives.Dependency and Build Configuration Updates:
io.documentnode:epub4j-corewithorg.grimmory:epub4j-coreand addedorg.grimmory:epub4j-native; switched OpenAPI dependency tospringdoc-openapi-starter-webmvc-ui.Summary by CodeRabbit
Dependencies
Refactor
Chores
Tests