fix(kobo-sync): use KoboSpan for progress sync#253
fix(kobo-sync): use KoboSpan for progress sync#253alexhb1 wants to merge 26 commits intogrimmory-tools:developfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds Kobo span‑map extraction, storage, and resolution; threads per‑file content progress into reading‑state and two‑way sync flows; updates DTOs/entities, DB migrations, services, repos, frontend restore logic, i18n, and many tests. Changes
Sequence DiagramsequenceDiagram
participant Frontend as Frontend App
participant API as Backend API
participant DB as Database
participant Extract as KoboSpanMapExtractionService
participant Kobo as Kobo Device
Frontend->>API: UpdateReadProgress (includes contentSourceProgressPercent)
API->>DB: save UserBookFileProgress
API->>DB: find synced EPUB file progress
API->>DB: fetch preloaded KoboSpanMaps for file IDs
API->>API: KoboBookmarkLocationResolver.resolve(progress, fileProgress, preloadedMaps)
API->>API: EpubCfiService / CfiConvertor -> resolve href + sourceOffset(percent)
API->>API: select chapter, compute chapterProgress, pick nearest span
API->>DB: persist updated reading state / current bookmark
API->>Extract: computeAndStoreIfNeeded(bookFile, kepubFile)
Extract->>DB: save KoboSpanMapEntity (span_map_json, file_hash)
API->>Kobo: push reading state (bookmark with kobo.<spanId> location)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
booklore-api/src/main/java/org/booklore/model/entity/KoboSpanMapEntity.java
Outdated
Show resolved
Hide resolved
booklore-api/src/main/java/org/booklore/model/dto/settings/KoboSettings.java
Show resolved
Hide resolved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
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/appsettings/SettingPersistenceHelper.java (1)
299-307:⚠️ Potential issue | 🟠 MajorThis default change won't reach upgraded installs.
getJsonSettingInternal()only writes defaults when the setting row is missing. Any deployment that already persisted the old default (convertToKepub:false) will keep that legacy value after upgrade, so the KoboSpan fix stays off until an admin manually changes settings. Please add a one-time backfill/upgrade path for untouched legacy rows instead of relying on the new default alone.🤖 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/appsettings/SettingPersistenceHelper.java` around lines 299 - 307, Add a one-time backfill/migration that updates existing persisted KoboSettings rows that were never modified (i.e., still match the old default snapshot) so they receive the new convertToKepub=true default; implement this in SettingPersistenceHelper as a method like backfillLegacyKoboSettings() that runs at startup/migration, query the settings store for the KoboSettings key, detect rows whose JSON equals the old-default shape (convertToKepub:false and other old-default values) and update them to the new default produced by getDefaultKoboSettings(), leaving any rows that have been changed by admins untouched; call this backfill from the application startup path so upgraded installs get the fix.
🤖 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/kobo/KoboBookmarkLocationResolver.java`:
- Around line 181-190: The comparator currently uses min(...) with scores where
exact matches score 0 and suffix matches score negative length, causing suffix
matches to win; in KoboBookmarkLocationResolver change the stream reduction to
select the chapter with the highest score (e.g. use max(...) instead of
min(...)) and invert the scoring so exact matches receive the highest score and
suffix matches receive lower scores (reference the chapters().stream() filter,
the normalizedHref variable, and the Comparator.comparingInt(...) lambda) so an
exact normalizedHref match is chosen over a suffix match.
In
`@booklore-api/src/main/java/org/booklore/service/kobo/KoboEntitlementService.java`:
- Around line 380-385: The branch currently sets reflowable EPUBs to EPUB3 when
koboSettings.isConvertToKepub() is false/null, which causes reflowable books to
bypass the new KEPUB-based KoboSpan positioning; update the conditional in
KoboEntitlementService so that any reflowable EPUB (isEpubFile &&
!primaryFile.isFixedLayout()) is assigned KoboBookFormat.KEPUB (i.e., move the
KEPUB assignment to the reflowable EPUB case), leaving fixed-layout handling
(primaryFile.isFixedLayout()) and CBX conversion (isCbxFile &&
koboSettings.isConvertCbxToEpub()) intact so that positioning uses the KEPUB
path for reflowable EPUBs regardless of convertToKepub flag.
In `@booklore-api/src/main/java/org/booklore/service/kobo/KoboEpubUtils.java`:
- Around line 23-27: The normalizeHref method currently percent-decodes the href
before stripping the fragment, which lets URLDecoder convert + to space and
corrupt EPUB hrefs; update normalizeHref to remove the fragment first (strip
"#...") and only then perform any percent-decoding if you must, then normalize
backslashes to slashes and trim leading slashes; consider removing
URLDecoder.decode entirely or replacing it with a proper path decoder if encoded
reserved characters (like '+') must be preserved—changes should be made inside
the normalizeHref function.
- Around line 34-35: clampPercent currently accepts a nullable Float but
immediately autounboxes which throws NPE for null; add an explicit null guard at
the top of the method (e.g., if (value == null) return null;) then perform the
clamping using the existing logic (Math.max(0f, Math.min(value, 100f))) so the
signature stays Float and null inputs are preserved; update only the body of
clampPercent to include this check.
In
`@booklore-api/src/main/java/org/booklore/service/kobo/KoboReadingStateBuilder.java`:
- Around line 47-52: The early return in buildBookmarkFromProgress currently
gates the web-reader path on isWebReaderNewer(progress) which itself requires
UserBookProgressEntity.getEpubProgress() to be non-null, preventing newer
web-reader-derived positions from being considered; change the condition so
buildBookmarkFromWebReaderProgress(...) is invoked whenever web-reader-derived
inputs are available (e.g., file-level position data in
UserBookFileProgressEntity, an href or global percent in UserBookProgressEntity)
rather than only when epubProgress is non-null. Update the branching in
buildBookmarkFromProgress (and the similar block around the 57-71 range) to
check for presence of web-reader indicators (fileProgress position, href,
percent) or isWebReaderNewer(progress) and call
buildBookmarkFromWebReaderProgress when any of those are true so the resolver
can use file-level, href, or percent fallbacks instead of short-circuiting to
Kobo data.
In
`@booklore-api/src/main/java/org/booklore/service/kobo/KoboReadingStateService.java`:
- Around line 297-301: The code in KoboReadingStateService currently preserves a
stale chapter-relative value by only updating
fileProgress.setContentSourceProgressPercent(...) when
bookmark.getContentSourceProgressPercent() is non-null; change this so that when
bookmark.getContentSourceProgressPercent() is null you explicitly clear the
stored value (e.g., call fileProgress.setContentSourceProgressPercent(null)) so
the stored contentSourceProgressPercent is not reused by
KoboBookmarkLocationResolver; update the block that handles fileProgress and
bookmark to always set or clear the contentSourceProgressPercent based on the
bookmark value.
In
`@booklore-api/src/main/java/org/booklore/service/kobo/KoboSpanMapExtractionService.java`:
- Around line 190-198: The current code in KoboSpanMapExtractionService returns
null when spans.isEmpty(), which drops span-less chapters and skews progress
calculations; instead remove the null return and always create and return an
ExtractedChapter (using fileHeader.getFileName(), spineIndex,
Math.max(html.length(), 1), and the spans collection even if empty) so span-less
spine items remain in extractedChapters and preserve correct
globalStartProgress/globalEndProgress normalization.
- Around line 217-228: The function resolveHref currently uses URLDecoder.decode
which treats '+' as space; change it to an RFC3986 percent-decoding approach:
replace the URLDecoder.decode call in resolveHref with a call that obtains the
decoded path via java.net.URI (e.g. use URI.create(href).getPath() or new
URI(href).getPath()), catching URISyntaxException/IllegalArgumentException and
falling back to the original href if parsing fails; keep the rest of resolveHref
(leading '/' strip, rootPath resolve, normalize and replace backslashes)
unchanged and update variable decodedHref accordingly.
In
`@booklore-api/src/main/java/org/booklore/service/kobo/KoboSpanMapService.java`:
- Around line 68-74: getValidMaps() currently adds entries for rows whose file
hash matches even when entity.getSpanMap() is null, causing fileId->null results
unlike computeAndStoreIfNeeded(); update the bulk loop that iterates
koboSpanMapRepository.findByBookFileIdIn(...) in
KoboSpanMapService.getValidMaps() to skip entities with a null span map
(KoboSpanMapEntity.getSpanMap()) before putting into the result map so only
non-null span maps are published.
In `@frontend/src/app/features/readers/ebook-reader/ebook-reader.component.ts`:
- Around line 414-430: Do not clear pendingInitialChapterRestore before you know
a fraction was resolved: call resolveChapterFraction(...) and if it returns null
return false without mutating this.pendingInitialChapterRestore; only set
this.pendingInitialChapterRestore = null after targetFraction !== null (i.e.,
after the null check) and before you either compare fractions or call
this.viewManager.goToFraction(targetFraction). This keeps the pending restore
around across relocate races until a valid in-chapter fraction is available.
In
`@frontend/src/app/features/settings/device-settings/component/kobo-sync-settings/kobo-sync-settings-component.html`:
- Around line 66-73: Wrap or update the warning paragraph (the element with
class "setting-warning" that renders the icon <i class="pi
pi-exclamation-triangle"></i> and the localized text via
t('kobo.twoWaySyncKepubWarningAdmin') / t('kobo.twoWaySyncKepubWarning')) to be
announced to screen readers when it appears: add an appropriate live region
attribute (e.g., role="status" or aria-live="polite" and aria-atomic="true") to
the paragraph so the newly rendered text is announced, and mark the decorative
icon element as aria-hidden="true" so it is ignored by assistive tech.
---
Outside diff comments:
In
`@booklore-api/src/main/java/org/booklore/service/appsettings/SettingPersistenceHelper.java`:
- Around line 299-307: Add a one-time backfill/migration that updates existing
persisted KoboSettings rows that were never modified (i.e., still match the old
default snapshot) so they receive the new convertToKepub=true default; implement
this in SettingPersistenceHelper as a method like backfillLegacyKoboSettings()
that runs at startup/migration, query the settings store for the KoboSettings
key, detect rows whose JSON equals the old-default shape (convertToKepub:false
and other old-default values) and update them to the new default produced by
getDefaultKoboSettings(), leaving any rows that have been changed by admins
untouched; call this backfill from the application startup path so upgraded
installs get the fix.
🪄 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: 0a8260c9-5d71-45da-8545-8be4a3dd2769
📒 Files selected for processing (39)
booklore-api/src/main/java/org/booklore/convertor/KoboSpanMapJsonConverter.javabooklore-api/src/main/java/org/booklore/model/dto/kobo/KoboSpanPositionMap.javabooklore-api/src/main/java/org/booklore/model/dto/progress/EpubProgress.javabooklore-api/src/main/java/org/booklore/model/dto/request/BookFileProgress.javabooklore-api/src/main/java/org/booklore/model/dto/settings/KoboSettings.javabooklore-api/src/main/java/org/booklore/model/entity/KoboSpanMapEntity.javabooklore-api/src/main/java/org/booklore/model/entity/UserBookFileProgressEntity.javabooklore-api/src/main/java/org/booklore/repository/KoboSpanMapRepository.javabooklore-api/src/main/java/org/booklore/repository/UserBookFileProgressRepository.javabooklore-api/src/main/java/org/booklore/service/appsettings/SettingPersistenceHelper.javabooklore-api/src/main/java/org/booklore/service/book/BookDownloadService.javabooklore-api/src/main/java/org/booklore/service/kobo/KoboBookmarkLocationResolver.javabooklore-api/src/main/java/org/booklore/service/kobo/KoboEntitlementService.javabooklore-api/src/main/java/org/booklore/service/kobo/KoboEpubUtils.javabooklore-api/src/main/java/org/booklore/service/kobo/KoboLibrarySyncService.javabooklore-api/src/main/java/org/booklore/service/kobo/KoboReadingStateBuilder.javabooklore-api/src/main/java/org/booklore/service/kobo/KoboReadingStateService.javabooklore-api/src/main/java/org/booklore/service/kobo/KoboSpanMapExtractionService.javabooklore-api/src/main/java/org/booklore/service/kobo/KoboSpanMapService.javabooklore-api/src/main/java/org/booklore/service/progress/ReadingProgressService.javabooklore-api/src/main/java/org/booklore/util/koreader/CfiConvertor.javabooklore-api/src/main/java/org/booklore/util/koreader/EpubCfiService.javabooklore-api/src/main/resources/db/migration/V134__Add_content_source_progress_percent_to_user_book_file_progress.sqlbooklore-api/src/main/resources/db/migration/V135__Create_kobo_span_map_table.sqlbooklore-api/src/test/java/org/booklore/service/KoboEntitlementServiceTest.javabooklore-api/src/test/java/org/booklore/service/KoboReadingStateBuilderTest.javabooklore-api/src/test/java/org/booklore/service/KoboReadingStateServiceTest.javabooklore-api/src/test/java/org/booklore/service/kobo/KoboBookmarkLocationResolverTest.javabooklore-api/src/test/java/org/booklore/service/kobo/KoboCompatibilityServiceTest.javabooklore-api/src/test/java/org/booklore/service/kobo/KoboSpanMapExtractionServiceTest.javabooklore-api/src/test/java/org/booklore/service/kobo/KoboSpanMapServiceTest.javabooklore-api/src/test/java/org/booklore/service/progress/ReadingProgressServiceTest.javabooklore-api/src/test/java/org/booklore/util/koreader/EpubCfiServiceTest.javafrontend/src/app/features/book/model/book.model.tsfrontend/src/app/features/readers/ebook-reader/ebook-reader.component.tsfrontend/src/app/features/settings/device-settings/component/kobo-sync-settings/kobo-sync-settings-component.htmlfrontend/src/app/features/settings/device-settings/component/kobo-sync-settings/kobo-sync-settings-component.scssfrontend/src/app/features/settings/device-settings/component/kobo-sync-settings/kobo-sync-settings-component.tsfrontend/src/i18n/en/settings-device.json
💤 Files with no reviewable changes (1)
- booklore-api/src/test/java/org/booklore/service/kobo/KoboCompatibilityServiceTest.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). (2)
- GitHub Check: Test Suite / Backend Tests
- GitHub Check: Analyze (java-kotlin)
🧰 Additional context used
📓 Path-based instructions (8)
booklore-api/src/main/resources/db/migration/V*.sql
📄 CodeRabbit inference engine (AGENTS.md)
Add Flyway migrations as new files named
V<number>__<Description>.sql
Files:
booklore-api/src/main/resources/db/migration/V134__Add_content_source_progress_percent_to_user_book_file_progress.sqlbooklore-api/src/main/resources/db/migration/V135__Create_kobo_span_map_table.sql
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/model/dto/settings/KoboSettings.javabooklore-api/src/main/java/org/booklore/model/entity/UserBookFileProgressEntity.javabooklore-api/src/main/java/org/booklore/service/appsettings/SettingPersistenceHelper.javabooklore-api/src/main/java/org/booklore/service/kobo/KoboLibrarySyncService.javabooklore-api/src/main/java/org/booklore/service/book/BookDownloadService.javabooklore-api/src/main/java/org/booklore/model/dto/progress/EpubProgress.javabooklore-api/src/main/java/org/booklore/service/progress/ReadingProgressService.javabooklore-api/src/main/java/org/booklore/model/entity/KoboSpanMapEntity.javabooklore-api/src/main/java/org/booklore/repository/UserBookFileProgressRepository.javabooklore-api/src/main/java/org/booklore/repository/KoboSpanMapRepository.javabooklore-api/src/test/java/org/booklore/service/progress/ReadingProgressServiceTest.javabooklore-api/src/main/java/org/booklore/convertor/KoboSpanMapJsonConverter.javabooklore-api/src/test/java/org/booklore/util/koreader/EpubCfiServiceTest.javabooklore-api/src/main/java/org/booklore/model/dto/request/BookFileProgress.javabooklore-api/src/main/java/org/booklore/service/kobo/KoboEpubUtils.javabooklore-api/src/main/java/org/booklore/util/koreader/CfiConvertor.javabooklore-api/src/test/java/org/booklore/service/kobo/KoboSpanMapServiceTest.javabooklore-api/src/test/java/org/booklore/service/KoboEntitlementServiceTest.javabooklore-api/src/main/java/org/booklore/util/koreader/EpubCfiService.javabooklore-api/src/main/java/org/booklore/service/kobo/KoboSpanMapExtractionService.javabooklore-api/src/main/java/org/booklore/model/dto/kobo/KoboSpanPositionMap.javabooklore-api/src/test/java/org/booklore/service/kobo/KoboSpanMapExtractionServiceTest.javabooklore-api/src/test/java/org/booklore/service/kobo/KoboBookmarkLocationResolverTest.javabooklore-api/src/main/java/org/booklore/service/kobo/KoboSpanMapService.javabooklore-api/src/main/java/org/booklore/service/kobo/KoboEntitlementService.javabooklore-api/src/main/java/org/booklore/service/kobo/KoboReadingStateBuilder.javabooklore-api/src/main/java/org/booklore/service/kobo/KoboReadingStateService.javabooklore-api/src/main/java/org/booklore/service/kobo/KoboBookmarkLocationResolver.javabooklore-api/src/test/java/org/booklore/service/KoboReadingStateServiceTest.javabooklore-api/src/test/java/org/booklore/service/KoboReadingStateBuilderTest.java
booklore-api/src/**/*Entity.java
📄 CodeRabbit inference engine (AGENTS.md)
Keep JPA entities on the
*Entitysuffix
Files:
booklore-api/src/main/java/org/booklore/model/entity/UserBookFileProgressEntity.javabooklore-api/src/main/java/org/booklore/model/entity/KoboSpanMapEntity.java
frontend/src/**/*.{ts,tsx,html,scss}
📄 CodeRabbit inference engine (AGENTS.md)
Use 2-space indentation in TypeScript, HTML, and SCSS files
Files:
frontend/src/app/features/settings/device-settings/component/kobo-sync-settings/kobo-sync-settings-component.htmlfrontend/src/app/features/settings/device-settings/component/kobo-sync-settings/kobo-sync-settings-component.scssfrontend/src/app/features/settings/device-settings/component/kobo-sync-settings/kobo-sync-settings-component.tsfrontend/src/app/features/book/model/book.model.tsfrontend/src/app/features/readers/ebook-reader/ebook-reader.component.ts
frontend/src/app/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
frontend/src/app/**/*.{ts,tsx}: Preferinject()over constructor injection
Followfrontend/eslint.config.js: component selectors useapp-*, directive selectors useapp*, andanyis disallowed
Files:
frontend/src/app/features/settings/device-settings/component/kobo-sync-settings/kobo-sync-settings-component.tsfrontend/src/app/features/book/model/book.model.tsfrontend/src/app/features/readers/ebook-reader/ebook-reader.component.ts
booklore-api/src/test/**/*.java
📄 CodeRabbit inference engine (AGENTS.md)
Prefer focused unit tests; use
@SpringBootTestonly when the Spring context is required
Files:
booklore-api/src/test/java/org/booklore/service/progress/ReadingProgressServiceTest.javabooklore-api/src/test/java/org/booklore/util/koreader/EpubCfiServiceTest.javabooklore-api/src/test/java/org/booklore/service/kobo/KoboSpanMapServiceTest.javabooklore-api/src/test/java/org/booklore/service/KoboEntitlementServiceTest.javabooklore-api/src/test/java/org/booklore/service/kobo/KoboSpanMapExtractionServiceTest.javabooklore-api/src/test/java/org/booklore/service/kobo/KoboBookmarkLocationResolverTest.javabooklore-api/src/test/java/org/booklore/service/KoboReadingStateServiceTest.javabooklore-api/src/test/java/org/booklore/service/KoboReadingStateBuilderTest.java
frontend/src/i18n/**
📄 CodeRabbit inference engine (AGENTS.md)
Put user-facing strings in Transloco files under
frontend/src/i18n/
Files:
frontend/src/i18n/en/settings-device.json
frontend/src/app/**/*.component.ts
📄 CodeRabbit inference engine (AGENTS.md)
Keep Angular code on standalone components. Do not add NgModules
Files:
frontend/src/app/features/readers/ebook-reader/ebook-reader.component.ts
🧠 Learnings (9)
📚 Learning: 2026-03-26T01:46:48.863Z
Learnt from: CR
Repo: grimmory-tools/grimmory PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-26T01:46:48.863Z
Learning: Applies to booklore-api/src/main/resources/db/migration/V*.sql : Add Flyway migrations as new files named `V<number>__<Description>.sql`
Applied to files:
booklore-api/src/main/resources/db/migration/V134__Add_content_source_progress_percent_to_user_book_file_progress.sqlbooklore-api/src/main/resources/db/migration/V135__Create_kobo_span_map_table.sql
📚 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/kobo/KoboLibrarySyncService.javabooklore-api/src/main/java/org/booklore/service/progress/ReadingProgressService.javabooklore-api/src/test/java/org/booklore/service/KoboEntitlementServiceTest.javabooklore-api/src/main/java/org/booklore/service/kobo/KoboEntitlementService.javabooklore-api/src/main/java/org/booklore/service/kobo/KoboReadingStateService.javabooklore-api/src/test/java/org/booklore/service/KoboReadingStateServiceTest.java
📚 Learning: 2026-03-27T15:06:25.913Z
Learnt from: imnotjames
Repo: grimmory-tools/grimmory PR: 217
File: booklore-api/src/main/java/org/booklore/service/kobo/KoboLibrarySyncService.java:151-153
Timestamp: 2026-03-27T15:06:25.913Z
Learning: In `booklore-api/src/main/java/org/booklore/service/kobo/KoboLibrarySyncService.java`, the try/catch around `koboServerProxy.proxyCurrentRequest()` in `syncLibrary()` is intentionally scoped only to handle a missing/failed proxy response (null). Once a response is successfully obtained, its body and headers (including the upstream sync token decoded via `tokenGenerator.fromBase64()`) are processed without additional defensive wrapping. This is a deliberate design decision: other parts of the response are equally unguarded, and the goal is to handle proxy unavailability gracefully — not to catch every possible downstream processing error.
Applied to files:
booklore-api/src/main/java/org/booklore/service/kobo/KoboLibrarySyncService.javabooklore-api/src/test/java/org/booklore/service/KoboEntitlementServiceTest.javabooklore-api/src/main/java/org/booklore/service/kobo/KoboEntitlementService.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/kobo/KoboLibrarySyncService.javabooklore-api/src/main/java/org/booklore/service/kobo/KoboEpubUtils.javabooklore-api/src/test/java/org/booklore/service/KoboEntitlementServiceTest.javabooklore-api/src/main/java/org/booklore/service/kobo/KoboEntitlementService.javabooklore-api/src/main/java/org/booklore/service/kobo/KoboReadingStateBuilder.javabooklore-api/src/main/java/org/booklore/service/kobo/KoboReadingStateService.javabooklore-api/src/main/java/org/booklore/service/kobo/KoboBookmarkLocationResolver.javabooklore-api/src/test/java/org/booklore/service/KoboReadingStateServiceTest.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/book/BookDownloadService.java
📚 Learning: 2026-03-26T01:46:48.863Z
Learnt from: CR
Repo: grimmory-tools/grimmory PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-26T01:46:48.863Z
Learning: Applies to booklore-api/src/**/*Entity.java : Keep JPA entities on the `*Entity` suffix
Applied to files:
booklore-api/src/main/java/org/booklore/model/entity/KoboSpanMapEntity.javabooklore-api/src/test/java/org/booklore/service/KoboReadingStateServiceTest.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/main/java/org/booklore/model/entity/KoboSpanMapEntity.javabooklore-api/src/test/java/org/booklore/service/progress/ReadingProgressServiceTest.javabooklore-api/src/test/java/org/booklore/service/kobo/KoboSpanMapServiceTest.javabooklore-api/src/main/java/org/booklore/model/dto/kobo/KoboSpanPositionMap.javabooklore-api/src/main/java/org/booklore/service/kobo/KoboReadingStateService.javabooklore-api/src/test/java/org/booklore/service/KoboReadingStateServiceTest.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/test/**/*.java : Prefer focused unit tests; use `SpringBootTest` only when the Spring context is required
Applied to files:
booklore-api/src/test/java/org/booklore/service/kobo/KoboSpanMapServiceTest.javabooklore-api/src/test/java/org/booklore/service/KoboEntitlementServiceTest.javabooklore-api/src/test/java/org/booklore/service/kobo/KoboBookmarkLocationResolverTest.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/kobo/KoboEntitlementService.java
🔇 Additional comments (21)
booklore-api/src/main/java/org/booklore/service/kobo/KoboLibrarySyncService.java (1)
79-79: LGTM - batch size increase is reasonable.The increase from 5 to 100 reduces sync round-trips and has been verified with real hardware per the PR description.
One optional consideration: extracting
100to a named constant (e.g.,MAX_SYNC_BATCH_SIZE) would clarify intent and ensure both paths stay in sync, but this is a minor point given the straightforward nature of the change.Also applies to: 119-119
booklore-api/src/main/java/org/booklore/repository/UserBookFileProgressRepository.java (1)
66-75: LGTM!The new repository method follows the existing patterns in this file. The
@EntityGraphannotation appropriately eager-loadsbookFileandbookFile.bookto avoid N+1 queries when bulk-fetching progress entities.booklore-api/src/main/java/org/booklore/service/progress/ReadingProgressService.java (3)
152-158: LGTM!The
contentSourceProgressPercentis correctly rounded using the existingroundToOneDecimalutility and integrated into theEpubProgressbuilder alongside other EPUB-type progress fields.
306-306: LGTM!The new field is correctly persisted from the incoming
BookFileProgressrecord to the entity.
321-321: LGTM!Explicitly setting
contentSourceProgressPercenttonullin the legacy path is appropriate since this field is not applicable to legacy progress updates.frontend/src/app/features/settings/device-settings/component/kobo-sync-settings/kobo-sync-settings-component.ts (2)
49-56: LGTM!The default
convertToKepub: truealigns with the PR's intent that two-way sync requires KEPUB conversion. The default change is consistent with the backend DTO defaults.
97-100: LGTM!The new
loadKepubSetting()method appropriately loads the KEPUB conversion setting for non-admin users when they gain Kobo token permission. This ensures non-admin users can view the setting state without loading admin-only settings.Also applies to: 133-138
booklore-api/src/main/java/org/booklore/model/dto/progress/EpubProgress.java (1)
13-19: LGTM!Making
cfioptional (no@NotNull) is appropriate since Kobo sync provides span-based positions rather than CFI. The newcontentSourceProgressPercentfield is correctly nullable since it's only populated during content source sync. Thepercentagefield appropriately retains its@NotNullconstraint.frontend/src/app/features/book/model/book.model.ts (1)
63-68: LGTM!The
EpubProgressinterface changes align with the backend DTO modifications. The optionalcfi, newhreffor chapter-based position restoration, andcontentSourceProgressPercentfor content-source progress tracking are all appropriately typed as optional.booklore-api/src/main/java/org/booklore/repository/KoboSpanMapRepository.java (1)
14-21: LGTM!The repository provides appropriate query methods for single and batch retrieval of span maps by book file ID. The explicit
@Queryannotations are clear, though Spring Data JPA could derive these from method names.booklore-api/src/main/resources/db/migration/V135__Create_kobo_span_map_table.sql (1)
1-14: LGTM!The migration is well-structured:
- Unique constraint on
book_file_idenforces one span map per fileON DELETE CASCADEappropriately removes span maps when the associated book file is deletedfile_hashenables cache invalidation when the file content changesLONGTEXTis suitable for potentially large JSON span mapsThe migration follows the Flyway naming convention
V<number>__<Description>.sql.booklore-api/src/main/java/org/booklore/model/dto/request/BookFileProgress.java (1)
5-12: LGTM!The new
contentSourceProgressPercentcomponent is appropriately nullable (no@NotNull) and placed at the end of the record to minimize impact on any code using positional construction.booklore-api/src/main/java/org/booklore/model/entity/KoboSpanMapEntity.java (1)
23-50: Entity mapping looks consistent and production-ready.The table constraint, one-to-one relation, JSON conversion, and required timestamps all align with the span-map caching model.
booklore-api/src/test/java/org/booklore/util/koreader/EpubCfiServiceTest.java (1)
183-193: Good coverage for CFI round-trip and location-resolution behavior.These cases meaningfully exercise the new CFI-to-location flow, including monotonic progress assertions and invalid-input handling.
Also applies to: 244-267
booklore-api/src/test/java/org/booklore/service/progress/ReadingProgressServiceTest.java (1)
157-167: Nice test expansion for file-level progress overlay and persistence.The new assertions close the gap around
positionHrefandcontentSourceProgressPercentpropagation.Also applies to: 208-249
booklore-api/src/test/java/org/booklore/service/kobo/KoboSpanMapExtractionServiceTest.java (1)
25-81: Strong targeted tests for extraction edge cases.The encoded-manifest-href and progress-boundary assertions are valuable for preventing sync-position regressions.
booklore-api/src/main/java/org/booklore/util/koreader/CfiConvertor.java (2)
145-180:resolveSourceOffsetfallback chain is well implemented.This addition cleanly handles text-offset resolution first and gracefully falls back to element-level offsets.
Also applies to: 519-547
370-371: Java 25 is configured;List.getFirst()is fully supported and appropriate.The project targets Java 25 (configured in
build.gradle.kts), which exceeds the Java 21 minimum requirement forList.getFirst(). This method is the preferred modern approach for accessing the first element and poses no compatibility risk.> Likely an incorrect or invalid review comment.frontend/src/i18n/en/settings-device.json (1)
65-101: Kobo sync messaging updates are clear and aligned with behavior.The new warnings and two-way sync descriptions set correct expectations around KEPUB prerequisites and mapping accuracy.
booklore-api/src/test/java/org/booklore/service/kobo/KoboSpanMapServiceTest.java (1)
44-109: Good unit coverage for cache validity and update semantics.The tests effectively guard hash-based skip/update behavior and stale-map rejection.
booklore-api/src/main/java/org/booklore/convertor/KoboSpanMapJsonConverter.java (1)
6-7: Resolved: imports are consistent with codebase standards.The
tools.jackson.*namespace is used throughout the codebase for all serialization/deserialization logic (converters, services, mappers, tests). These imports in KoboSpanMapJsonConverter match the established pattern and are compatible with the project's Jackson 3 setup.
booklore-api/src/main/java/org/booklore/service/kobo/KoboBookmarkLocationResolver.java
Outdated
Show resolved
Hide resolved
booklore-api/src/main/java/org/booklore/service/kobo/KoboEntitlementService.java
Show resolved
Hide resolved
booklore-api/src/main/java/org/booklore/service/kobo/KoboEpubUtils.java
Outdated
Show resolved
Hide resolved
booklore-api/src/main/java/org/booklore/service/kobo/KoboReadingStateBuilder.java
Show resolved
Hide resolved
booklore-api/src/main/java/org/booklore/service/kobo/KoboSpanMapExtractionService.java
Outdated
Show resolved
Hide resolved
booklore-api/src/main/java/org/booklore/service/kobo/KoboSpanMapExtractionService.java
Show resolved
Hide resolved
booklore-api/src/main/java/org/booklore/service/kobo/KoboSpanMapService.java
Show resolved
Hide resolved
frontend/src/app/features/readers/ebook-reader/ebook-reader.component.ts
Show resolved
Hide resolved
...ures/settings/device-settings/component/kobo-sync-settings/kobo-sync-settings-component.html
Outdated
Show resolved
Hide resolved
booklore-api/src/main/java/org/booklore/service/kobo/KoboEpubUtils.java
Outdated
Show resolved
Hide resolved
booklore-api/src/main/java/org/booklore/convertor/KoboSpanMapJsonConverter.java
Show resolved
Hide resolved
Language fixes + helper cleanup + coderabbit suggestions
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
booklore-api/src/main/java/org/booklore/service/kobo/KoboSpanMapExtractionService.java (1)
185-187: Consider using method reference for null filter.Minor style improvement for consistency.
♻️ Suggested change
- .filter(span -> span != null) + .filter(java.util.Objects::nonNull)🤖 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/kobo/KoboSpanMapExtractionService.java` around lines 185 - 187, Replace the inline lambda null-check in KoboSpanMapExtractionService (the stream ending with .filter(span -> span != null).toList()) with a method reference for consistency and readability by using Objects::nonNull; ensure java.util.Objects is imported if not already present.
🤖 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/kobo/KoboEpubUtils.java`:
- Around line 36-39: The normalizeHref method can throw an NPE because
decodeHrefPath(href) may return null; update normalizeHref to guard against null
by handling decodeHrefPath's null return (e.g., treat as empty string) before
calling replaceFirst. Locate normalizeHref and decodeHrefPath in KoboEpubUtils
and ensure normalizeHref uses a null-safe value (or a conditional) so
.replaceFirst is never invoked on null; return the normalized (possibly empty)
string.
In `@frontend/src/i18n/hr/settings-device.json`:
- Around line 100-101: The Croatian locale file still contains English for the
two new keys; update the hr/settings-device.json entries
"twoWaySyncKepubWarning" and "twoWaySyncKepubWarningAdmin" with proper Croatian
translations so Croatian users don’t see mixed-language UI; locate those keys in
the file and replace the English values with Croatian equivalents that convey
the same meaning (administrative enablement and requirement for KEPUB conversion
for two-way progress sync).
In `@frontend/src/i18n/ja/settings-device.json`:
- Around line 65-66: The Japanese locale file contains English text for the keys
convertKepubPros and convertKepubCons (and the similar strings at lines
100-101); replace those English strings with proper Japanese translations so the
ja settings are fully localized—update the values for convertKepubPros and
convertKepubCons (and the matching keys at lines 100-101) to their correct
Japanese translations while keeping the JSON keys unchanged.
---
Nitpick comments:
In
`@booklore-api/src/main/java/org/booklore/service/kobo/KoboSpanMapExtractionService.java`:
- Around line 185-187: Replace the inline lambda null-check in
KoboSpanMapExtractionService (the stream ending with .filter(span -> span !=
null).toList()) with a method reference for consistency and readability by using
Objects::nonNull; ensure java.util.Objects is imported if not already present.
🪄 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: f3b33d12-494a-4c46-a3db-591b126c396a
📒 Files selected for processing (33)
booklore-api/src/main/java/org/booklore/model/entity/KoboSpanMapEntity.javabooklore-api/src/main/java/org/booklore/service/kobo/KoboBookmarkLocationResolver.javabooklore-api/src/main/java/org/booklore/service/kobo/KoboEpubUtils.javabooklore-api/src/main/java/org/booklore/service/kobo/KoboReadingStateBuilder.javabooklore-api/src/main/java/org/booklore/service/kobo/KoboReadingStateService.javabooklore-api/src/main/java/org/booklore/service/kobo/KoboSpanMapExtractionService.javabooklore-api/src/main/java/org/booklore/service/kobo/KoboSpanMapService.javabooklore-api/src/test/java/org/booklore/service/KoboReadingStateBuilderTest.javabooklore-api/src/test/java/org/booklore/service/KoboReadingStateServiceTest.javabooklore-api/src/test/java/org/booklore/service/kobo/KoboBookmarkLocationResolverTest.javabooklore-api/src/test/java/org/booklore/service/kobo/KoboSpanMapExtractionServiceTest.javabooklore-api/src/test/java/org/booklore/service/kobo/KoboSpanMapServiceTest.javafrontend/src/app/features/readers/ebook-reader/ebook-reader.component.tsfrontend/src/app/features/settings/device-settings/component/kobo-sync-settings/kobo-sync-settings-component.htmlfrontend/src/i18n/da/settings-device.jsonfrontend/src/i18n/de/settings-device.jsonfrontend/src/i18n/en/settings-device.jsonfrontend/src/i18n/es/settings-device.jsonfrontend/src/i18n/fr/settings-device.jsonfrontend/src/i18n/hr/settings-device.jsonfrontend/src/i18n/hu/settings-device.jsonfrontend/src/i18n/id/settings-device.jsonfrontend/src/i18n/it/settings-device.jsonfrontend/src/i18n/ja/settings-device.jsonfrontend/src/i18n/nl/settings-device.jsonfrontend/src/i18n/pl/settings-device.jsonfrontend/src/i18n/pt/settings-device.jsonfrontend/src/i18n/ru/settings-device.jsonfrontend/src/i18n/sk/settings-device.jsonfrontend/src/i18n/sl/settings-device.jsonfrontend/src/i18n/sv/settings-device.jsonfrontend/src/i18n/uk/settings-device.jsonfrontend/src/i18n/zh/settings-device.json
✅ Files skipped from review due to trivial changes (19)
- frontend/src/i18n/sk/settings-device.json
- frontend/src/i18n/es/settings-device.json
- frontend/src/i18n/da/settings-device.json
- frontend/src/i18n/sl/settings-device.json
- frontend/src/i18n/uk/settings-device.json
- frontend/src/i18n/nl/settings-device.json
- frontend/src/i18n/pt/settings-device.json
- frontend/src/i18n/sv/settings-device.json
- frontend/src/i18n/hu/settings-device.json
- frontend/src/i18n/de/settings-device.json
- frontend/src/i18n/it/settings-device.json
- frontend/src/i18n/id/settings-device.json
- frontend/src/i18n/zh/settings-device.json
- booklore-api/src/main/java/org/booklore/model/entity/KoboSpanMapEntity.java
- frontend/src/i18n/pl/settings-device.json
- booklore-api/src/test/java/org/booklore/service/kobo/KoboBookmarkLocationResolverTest.java
- frontend/src/i18n/fr/settings-device.json
- frontend/src/i18n/ru/settings-device.json
- booklore-api/src/test/java/org/booklore/service/kobo/KoboSpanMapServiceTest.java
🚧 Files skipped from review as they are similar to previous changes (3)
- frontend/src/i18n/en/settings-device.json
- frontend/src/app/features/readers/ebook-reader/ebook-reader.component.ts
- booklore-api/src/test/java/org/booklore/service/KoboReadingStateBuilderTest.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). (2)
- GitHub Check: Test Suite / Backend Tests
- GitHub Check: Analyze (java-kotlin)
🧰 Additional context used
📓 Path-based instructions (4)
frontend/src/i18n/**
📄 CodeRabbit inference engine (AGENTS.md)
Put user-facing strings in Transloco files under
frontend/src/i18n/
Files:
frontend/src/i18n/hr/settings-device.jsonfrontend/src/i18n/ja/settings-device.json
frontend/src/**/*.{ts,tsx,html,scss}
📄 CodeRabbit inference engine (AGENTS.md)
Use 2-space indentation in TypeScript, HTML, and SCSS files
Files:
frontend/src/app/features/settings/device-settings/component/kobo-sync-settings/kobo-sync-settings-component.html
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/kobo/KoboEpubUtils.javabooklore-api/src/main/java/org/booklore/service/kobo/KoboSpanMapExtractionService.javabooklore-api/src/main/java/org/booklore/service/kobo/KoboSpanMapService.javabooklore-api/src/main/java/org/booklore/service/kobo/KoboReadingStateBuilder.javabooklore-api/src/test/java/org/booklore/service/KoboReadingStateServiceTest.javabooklore-api/src/main/java/org/booklore/service/kobo/KoboReadingStateService.javabooklore-api/src/test/java/org/booklore/service/kobo/KoboSpanMapExtractionServiceTest.javabooklore-api/src/main/java/org/booklore/service/kobo/KoboBookmarkLocationResolver.java
booklore-api/src/test/**/*.java
📄 CodeRabbit inference engine (AGENTS.md)
Prefer focused unit tests; use
@SpringBootTestonly when the Spring context is required
Files:
booklore-api/src/test/java/org/booklore/service/KoboReadingStateServiceTest.javabooklore-api/src/test/java/org/booklore/service/kobo/KoboSpanMapExtractionServiceTest.java
🧠 Learnings (8)
📓 Common learnings
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).
📚 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/kobo/KoboEpubUtils.javabooklore-api/src/main/java/org/booklore/service/kobo/KoboSpanMapExtractionService.javabooklore-api/src/main/java/org/booklore/service/kobo/KoboSpanMapService.javabooklore-api/src/main/java/org/booklore/service/kobo/KoboReadingStateBuilder.javabooklore-api/src/test/java/org/booklore/service/KoboReadingStateServiceTest.javabooklore-api/src/main/java/org/booklore/service/kobo/KoboReadingStateService.javabooklore-api/src/main/java/org/booklore/service/kobo/KoboBookmarkLocationResolver.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/kobo/KoboSpanMapExtractionService.javabooklore-api/src/main/java/org/booklore/service/kobo/KoboReadingStateBuilder.javabooklore-api/src/test/java/org/booklore/service/KoboReadingStateServiceTest.javabooklore-api/src/main/java/org/booklore/service/kobo/KoboReadingStateService.java
📚 Learning: 2026-03-26T03:22:24.500Z
Learnt from: imnotjames
Repo: grimmory-tools/grimmory PR: 209
File: booklore-api/src/main/java/org/booklore/util/FileUtils.java:32-38
Timestamp: 2026-03-26T03:22:24.500Z
Learning: In `grimmory-tools/grimmory`, `BookFileEntity.fileSubPath` (in `booklore-api/src/main/java/org/booklore/model/entity/BookFileEntity.java`) is declared with `Column(name = "file_sub_path", length = 512, nullable = false)`, meaning it is never null. Do not flag null-guard concerns for this field.
Applied to files:
booklore-api/src/main/java/org/booklore/service/kobo/KoboSpanMapService.java
📚 Learning: 2026-03-26T02:20:27.862Z
Learnt from: imnotjames
Repo: grimmory-tools/grimmory PR: 198
File: booklore-api/src/main/java/org/booklore/controller/KoboController.java:100-103
Timestamp: 2026-03-26T02:20:27.862Z
Learning: In `booklore-api/src/main/java/org/booklore/controller/KoboController.java`, the Kobo reader device already makes direct outbound connections to Kobo's CDN/servers for other requests (confirmed by imnotjames via testing). Therefore, returning a `307 TEMPORARY_REDIRECT` to `cdn.kobo.com` for non-`BL-` book cover thumbnails (instead of server-side proxying) does NOT introduce a new privacy or behavioral regression — the Kobo device's IP and User-Agent are already exposed to Kobo through other direct requests.
Applied to files:
booklore-api/src/main/java/org/booklore/service/kobo/KoboReadingStateBuilder.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/KoboReadingStateServiceTest.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/KoboReadingStateServiceTest.javabooklore-api/src/main/java/org/booklore/service/kobo/KoboReadingStateService.java
📚 Learning: 2026-03-27T15:06:25.913Z
Learnt from: imnotjames
Repo: grimmory-tools/grimmory PR: 217
File: booklore-api/src/main/java/org/booklore/service/kobo/KoboLibrarySyncService.java:151-153
Timestamp: 2026-03-27T15:06:25.913Z
Learning: In `booklore-api/src/main/java/org/booklore/service/kobo/KoboLibrarySyncService.java`, the try/catch around `koboServerProxy.proxyCurrentRequest()` in `syncLibrary()` is intentionally scoped only to handle a missing/failed proxy response (null). Once a response is successfully obtained, its body and headers (including the upstream sync token decoded via `tokenGenerator.fromBase64()`) are processed without additional defensive wrapping. This is a deliberate design decision: other parts of the response are equally unguarded, and the goal is to handle proxy unavailability gracefully — not to catch every possible downstream processing error.
Applied to files:
booklore-api/src/main/java/org/booklore/service/kobo/KoboReadingStateService.java
🔇 Additional comments (14)
frontend/src/app/features/settings/device-settings/component/kobo-sync-settings/kobo-sync-settings-component.html (1)
65-74: A11y handling for the dynamic warning is solid.Line 66 correctly makes the newly rendered warning announceable, and Line 67 properly marks the icon as decorative.
booklore-api/src/main/java/org/booklore/service/kobo/KoboEpubUtils.java (1)
10-43: LGTM - well-structured utility class.Good use of
@UtilityClass, proper null handling indecodeHrefPath, and the switch fromURLDecodertoURI.create().getPath()correctly preserves+characters in EPUB paths. TheclampUnitmethod using primitivefloatavoids the NPE concern from the previous review.booklore-api/src/test/java/org/booklore/service/kobo/KoboSpanMapExtractionServiceTest.java (1)
18-193: LGTM - comprehensive test coverage.Good focused unit tests covering spine extraction, URL-encoded paths (
%20→ space), plus-sign preservation (%2B→+), and span-less chapters. The use of@TempDirand programmatic KEPUB fixture creation keeps tests self-contained. Float assertions appropriately use tolerance values.booklore-api/src/main/java/org/booklore/service/kobo/KoboSpanMapExtractionService.java (1)
36-88: LGTM - solid extraction logic.The extraction correctly handles OPF parsing, spine ordering, and chapter progress calculation. Good that span-less chapters are now preserved in the progress model (addressing the previous review concern). The use of
SecureXmlUtilsfor XML parsing is appropriate for security.booklore-api/src/main/java/org/booklore/service/kobo/KoboSpanMapService.java (1)
20-94: LGTM - well-implemented caching service.Good use of
@RequiredArgsConstructorfor constructor injection. ThegetValidMapsmethod now correctly filters out null span maps (addressing the previous review). TheDataIntegrityViolationExceptionhandling for concurrent inserts is appropriate. Transactional boundaries are correctly applied.booklore-api/src/main/java/org/booklore/service/kobo/KoboReadingStateService.java (2)
260-309: LGTM - robust two-way sync logic.The
crossPopulateEpubFieldsFromKobomethod now properly clears stalecontentSourceProgressPercentwhen the bookmark omits it (addressing the previous review concern). The timestamp comparison logic correctly gates updates based on freshness, and the file progress entity is created on-demand when needed.
311-331: LGTM - clean helper methods.The
findSyncedEpubFileProgressoverloads andisWebReaderNewerThanBookmarkhelper provide clear abstractions for the sync logic.booklore-api/src/main/java/org/booklore/service/kobo/KoboReadingStateBuilder.java (2)
57-71: LGTM - improved freshness check.The
isWebReaderNewermethod now correctly requires bothepubProgressPercentandlastReadTimeto be present before comparing timestamps, addressing the previous concern about blocking the web-reader path ongetEpubProgress()alone.
24-26: Timestamp format matches service layer.Good that
KOBO_TIMESTAMP_FORMATuses the same pattern (yyyy-MM-dd'T'HH:mm:ss.SSSSSSS'Z') asKoboReadingStateService, ensuring consistency when timestamps are compared or re-parsed.booklore-api/src/test/java/org/booklore/service/KoboReadingStateServiceTest.java (2)
116-123: LGTM - good test helper.The
setPrimaryEpubhelper method cleanly encapsulates the common setup pattern for tests requiring an EPUB primary file, reducing duplication across test cases.
798-918: LGTM - thorough two-way sync test coverage.Good addition of tests for:
- Clearing stale EPUB CFI while preserving chapter source when Kobo sends KoboSpan
- Clearing stale
contentSourceProgressPercentwhen bookmark omits itThese tests properly validate the stale-data clearing behavior addressed in production code.
booklore-api/src/main/java/org/booklore/service/kobo/KoboBookmarkLocationResolver.java (3)
175-190: LGTM - correct chapter matching priority.The
findChapterByHrefmethod now correctly usesmax()with a comparator that prioritizes exact matches (score 1) over suffix matches (score 0), then by length. This addresses the previous concern where exact matches could lose to suffix matches.
199-216: LGTM - proper unit/percent conversion.The
resolveChapterProgressPercentmethod correctly handles the conversion between chapter progress ranges[0,1]and percent values[0,100], usingclampUnit()and multiplication appropriately.
29-75: LGTM - well-structured resolution flow.The main
resolvemethod has a clear fallback chain: CFI → href → global progress. Good separation of concerns with dedicated methods for each resolution step.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/kobo/KoboSpanMapExtractionService.java`:
- Around line 173-175: The method in KoboSpanMapExtractionService currently
returns null when html.isBlank(), which removes entries from extractedChapters
and shifts global progress; instead, return a preserved placeholder so the spine
entry remains (e.g., create and return an empty/blank Chapter/SpanMap result or
an object with html set to empty string and no spans). Update the branch that
checks html.isBlank() to construct and return a non-null placeholder result
(referencing the method that produces chapter/span results and the
extractedChapters collection) so blank XHTML spine items are kept rather than
dropped.
🪄 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: 6d2e3873-a5cc-446f-a5dd-194302fd6818
📒 Files selected for processing (1)
booklore-api/src/main/java/org/booklore/service/kobo/KoboSpanMapExtractionService.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). (2)
- 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/kobo/KoboSpanMapExtractionService.java
🧠 Learnings (3)
📓 Common learnings
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).
📚 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/kobo/KoboSpanMapExtractionService.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/kobo/KoboSpanMapExtractionService.java
🔇 Additional comments (1)
booklore-api/src/main/java/org/booklore/service/kobo/KoboSpanMapExtractionService.java (1)
217-224: Href decoding/resolution update looks good.Using
KoboEpubUtils.decodeHrefPathbefore root-path resolution in this block is the right direction and avoids prior path-decoding mismatches.
booklore-api/src/main/java/org/booklore/service/kobo/KoboSpanMapExtractionService.java
Show resolved
Hide resolved
|
Regarding the translation the problem wasn't that you haven't added the new ones to all .json files, rather that you modified existing translations. I think I may have not clear enough, the main problem with that is that: while English says Y the rest say X because you updated the English string to Y BUT the translation are based on the old X English string therefore they are out of sync. That's problematic, the fact that new variables weren't added to the json files are not, because transloco I believe can pick that up, in those cases automatically uses the english one as a fallback 😅 Sorry! |
|
@balazs-szucs No worries, latest commit I've removed the edits anyway, made sense to just add the new lines only (with english placeholders for the other languages). Never worked with transloco before, is that the correct way? |
Ah if you removed the changes to the originals, yes it 100% fine. Thank you! |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@booklore-api/src/main/java/org/booklore/service/kobo/KoboBookmarkLocationResolver.java`:
- Around line 57-64: When href-based chapter resolution fails (in resolveChapter
usage around the first block and similarly in the 197-211 block), do not reuse
chapterProgressPercent from the originally attempted chapter; instead call
resolveChapterProgressPercent with null for the chapter-local percent when you
fall back to the chapter chosen by globalProgressPercent, then pass the
resulting clamped value (ensure it's constrained to 0..100 inside
resolveChapterProgressPercent or immediately after) into resolveSpanMarker;
update the fallback path so chapterProgressPercent is null and the returned
percent is clamped before resolving the span.
In
`@booklore-api/src/main/java/org/booklore/service/kobo/KoboEntitlementService.java`:
- Around line 141-148: The stream mapping can NPE when progress.getBook() is
null; before calling buildChangedReadingState use a filter to exclude entries
with null books (i.e., apply progress -> progress.getBook() != null) so the
subsequent use of progress.getBook().getId() when looking up
syncedEpubFileIdsByBookId and fileProgressByFileId is safe; update the chain
that processes progressEntries (the call site using buildChangedReadingState,
fileProgressByFileId and syncedEpubFileIdsByBookId) to perform this null check
before the map.
🪄 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: 0f2da032-43a5-462a-842b-c8b006897169
📒 Files selected for processing (4)
booklore-api/src/main/java/org/booklore/service/kobo/KoboBookmarkLocationResolver.javabooklore-api/src/main/java/org/booklore/service/kobo/KoboEntitlementService.javabooklore-api/src/main/java/org/booklore/service/kobo/KoboEpubUtils.javabooklore-api/src/main/java/org/booklore/service/kobo/KoboSpanMapExtractionService.java
🚧 Files skipped from review as they are similar to previous changes (2)
- booklore-api/src/main/java/org/booklore/service/kobo/KoboEpubUtils.java
- booklore-api/src/main/java/org/booklore/service/kobo/KoboSpanMapExtractionService.java
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Test Suite / Backend Tests
- GitHub Check: Test Suite / Frontend Tests
- GitHub Check: Analyze (java-kotlin)
🧰 Additional context used
📓 Path-based instructions (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/kobo/KoboEntitlementService.javabooklore-api/src/main/java/org/booklore/service/kobo/KoboBookmarkLocationResolver.java
🧠 Learnings (5)
📚 Learning: 2026-03-27T15:06:25.913Z
Learnt from: imnotjames
Repo: grimmory-tools/grimmory PR: 217
File: booklore-api/src/main/java/org/booklore/service/kobo/KoboLibrarySyncService.java:151-153
Timestamp: 2026-03-27T15:06:25.913Z
Learning: In `booklore-api/src/main/java/org/booklore/service/kobo/KoboLibrarySyncService.java`, the try/catch around `koboServerProxy.proxyCurrentRequest()` in `syncLibrary()` is intentionally scoped only to handle a missing/failed proxy response (null). Once a response is successfully obtained, its body and headers (including the upstream sync token decoded via `tokenGenerator.fromBase64()`) are processed without additional defensive wrapping. This is a deliberate design decision: other parts of the response are equally unguarded, and the goal is to handle proxy unavailability gracefully — not to catch every possible downstream processing error.
Applied to files:
booklore-api/src/main/java/org/booklore/service/kobo/KoboEntitlementService.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/kobo/KoboEntitlementService.javabooklore-api/src/main/java/org/booklore/service/kobo/KoboBookmarkLocationResolver.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/kobo/KoboEntitlementService.java
📚 Learning: 2026-03-26T02:20:27.862Z
Learnt from: imnotjames
Repo: grimmory-tools/grimmory PR: 198
File: booklore-api/src/main/java/org/booklore/controller/KoboController.java:100-103
Timestamp: 2026-03-26T02:20:27.862Z
Learning: In `booklore-api/src/main/java/org/booklore/controller/KoboController.java`, the Kobo reader device already makes direct outbound connections to Kobo's CDN/servers for other requests (confirmed by imnotjames via testing). Therefore, returning a `307 TEMPORARY_REDIRECT` to `cdn.kobo.com` for non-`BL-` book cover thumbnails (instead of server-side proxying) does NOT introduce a new privacy or behavioral regression — the Kobo device's IP and User-Agent are already exposed to Kobo through other direct requests.
Applied to files:
booklore-api/src/main/java/org/booklore/service/kobo/KoboEntitlementService.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/kobo/KoboEntitlementService.java
🔇 Additional comments (7)
booklore-api/src/main/java/org/booklore/service/kobo/KoboEntitlementService.java (5)
380-386: Format selection logic acknowledged.This segment was previously discussed and confirmed correct by the author—
KoboBookFormatmust match what the device receives on download, so reportingKEPUBwhenconvertToKepubis disabled would create a mismatch.
46-56: LGTM!New dependencies properly added using the existing Lombok constructor injection pattern via
@AllArgsConstructor.
217-227: LGTM!Method signature cleanly extended to support the new file-level progress and span map data, passing through to the builder.
258-278: LGTM!The method correctly retrieves the synced EPUB file progress and threads it through to the bookmark builder, with proper null handling via
orElse(null).
299-305: LGTM!Clean helper method with proper null handling and appropriate use of
Optional.booklore-api/src/main/java/org/booklore/service/kobo/KoboBookmarkLocationResolver.java (2)
173-187: Href match ranking looks correct.Exact normalized hrefs now win over suffix matches, while still preferring the most specific suffix when there isn't an exact hit.
220-221: The Java version is configured to Java 25 (languageVersion = JavaLanguageVersion.of(25)inbooklore-api/build.gradle.kts), which is well above the Java 21 requirement forList#getFirst(). The code is compatible and requires no changes.> Likely an incorrect or invalid review comment.
booklore-api/src/main/java/org/booklore/service/kobo/KoboBookmarkLocationResolver.java
Show resolved
Hide resolved
booklore-api/src/main/java/org/booklore/service/kobo/KoboEntitlementService.java
Show resolved
Hide resolved
There was a problem hiding this comment.
🧹 Nitpick comments (2)
booklore-api/src/test/java/org/booklore/service/KoboReadingStateServiceTest.java (1)
926-930: Use a fixed instant to avoid time-dependent test behavior.
Instant.now().plusSeconds(3600)is unnecessary here and makes intent less explicit. Prefer a fixed timestamp newer than the bookmark timestamp.♻️ Suggested deterministic change
- fileProgress.setLastReadTime(Instant.now().plusSeconds(3600)); + fileProgress.setLastReadTime(Instant.parse("2025-01-01T01:00:00Z"));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@booklore-api/src/test/java/org/booklore/service/KoboReadingStateServiceTest.java` around lines 926 - 930, In KoboReadingStateServiceTest replace the time-dependent Instant usage by setting fileProgress.setLastReadTime to a fixed, deterministic Instant that is after the bookmark timestamp (e.g., Instant.parse("2020-01-01T01:00:00Z")), so update the setup around primaryFile (setPrimaryEpub) and fileProgress to use that fixed Instant instead of Instant.now().plusSeconds(3600) to make the test deterministic.booklore-api/src/main/java/org/booklore/service/kobo/KoboLibrarySyncService.java (1)
79-79: Extract duplicated sync batch size into a named constant.Line 79 and Line 119 duplicate
100; centralizing it will prevent branch drift when tuning pagination later.Proposed refactor
public class KoboLibrarySyncService { + private static final int SYNC_BATCH_LIMIT = 100; @@ - int maxRemaining = 100; + int maxRemaining = SYNC_BATCH_LIMIT; @@ - int maxRemaining = 100; + int maxRemaining = SYNC_BATCH_LIMIT;Also applies to: 119-119
🤖 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/kobo/KoboLibrarySyncService.java` at line 79, The literal 100 is duplicated in KoboLibrarySyncService (used for pagination/batch sizes at the places that set maxRemaining); extract it into a single static final constant (e.g., private static final int SYNC_BATCH_SIZE = 100) at the class level and replace both uses (the local maxRemaining initializations/usages around where 100 appears) with that constant so future tuning changes are made in one place and maintain readability.
🤖 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/service/kobo/KoboLibrarySyncService.java`:
- Line 79: The literal 100 is duplicated in KoboLibrarySyncService (used for
pagination/batch sizes at the places that set maxRemaining); extract it into a
single static final constant (e.g., private static final int SYNC_BATCH_SIZE =
100) at the class level and replace both uses (the local maxRemaining
initializations/usages around where 100 appears) with that constant so future
tuning changes are made in one place and maintain readability.
In
`@booklore-api/src/test/java/org/booklore/service/KoboReadingStateServiceTest.java`:
- Around line 926-930: In KoboReadingStateServiceTest replace the time-dependent
Instant usage by setting fileProgress.setLastReadTime to a fixed, deterministic
Instant that is after the bookmark timestamp (e.g.,
Instant.parse("2020-01-01T01:00:00Z")), so update the setup around primaryFile
(setPrimaryEpub) and fileProgress to use that fixed Instant instead of
Instant.now().plusSeconds(3600) to make the test deterministic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 61671260-7b62-44d3-828a-4e70d573ecbd
📒 Files selected for processing (5)
booklore-api/src/main/java/org/booklore/service/kobo/KoboLibrarySyncService.javabooklore-api/src/main/java/org/booklore/service/kobo/KoboReadingStateService.javabooklore-api/src/test/java/org/booklore/service/KoboEntitlementServiceTest.javabooklore-api/src/test/java/org/booklore/service/KoboReadingStateServiceTest.javabooklore-api/src/test/java/org/booklore/service/kobo/KoboLibrarySyncServiceTest.java
🚧 Files skipped from review as they are similar to previous changes (2)
- booklore-api/src/test/java/org/booklore/service/KoboEntitlementServiceTest.java
- booklore-api/src/main/java/org/booklore/service/kobo/KoboReadingStateService.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). (2)
- GitHub Check: Test Suite / Backend Tests
- GitHub Check: Analyze (java-kotlin)
🧰 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/main/java/org/booklore/service/kobo/KoboLibrarySyncService.javabooklore-api/src/test/java/org/booklore/service/kobo/KoboLibrarySyncServiceTest.javabooklore-api/src/test/java/org/booklore/service/KoboReadingStateServiceTest.java
booklore-api/src/test/**/*.java
📄 CodeRabbit inference engine (AGENTS.md)
Prefer focused unit tests; use
@SpringBootTestonly when the Spring context is required
Files:
booklore-api/src/test/java/org/booklore/service/kobo/KoboLibrarySyncServiceTest.javabooklore-api/src/test/java/org/booklore/service/KoboReadingStateServiceTest.java
🧠 Learnings (7)
📓 Common learnings
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).
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.
📚 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/kobo/KoboLibrarySyncService.javabooklore-api/src/test/java/org/booklore/service/KoboReadingStateServiceTest.java
📚 Learning: 2026-03-27T15:06:25.913Z
Learnt from: imnotjames
Repo: grimmory-tools/grimmory PR: 217
File: booklore-api/src/main/java/org/booklore/service/kobo/KoboLibrarySyncService.java:151-153
Timestamp: 2026-03-27T15:06:25.913Z
Learning: In `booklore-api/src/main/java/org/booklore/service/kobo/KoboLibrarySyncService.java`, the try/catch around `koboServerProxy.proxyCurrentRequest()` in `syncLibrary()` is intentionally scoped only to handle a missing/failed proxy response (null). Once a response is successfully obtained, its body and headers (including the upstream sync token decoded via `tokenGenerator.fromBase64()`) are processed without additional defensive wrapping. This is a deliberate design decision: other parts of the response are equally unguarded, and the goal is to handle proxy unavailability gracefully — not to catch every possible downstream processing error.
Applied to files:
booklore-api/src/main/java/org/booklore/service/kobo/KoboLibrarySyncService.javabooklore-api/src/test/java/org/booklore/service/kobo/KoboLibrarySyncServiceTest.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/kobo/KoboLibrarySyncService.javabooklore-api/src/test/java/org/booklore/service/KoboReadingStateServiceTest.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/test/java/org/booklore/service/kobo/KoboLibrarySyncServiceTest.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/KoboReadingStateServiceTest.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/KoboReadingStateServiceTest.java
🔇 Additional comments (6)
booklore-api/src/test/java/org/booklore/service/KoboReadingStateServiceTest.java (3)
116-123: Nice helper extraction for primary EPUB setup.
setPrimaryEpub(...)reduces repeated fixture setup and keeps new tests focused on behavior instead of boilerplate.As per coding guidelines
booklore-api/src/test/**/*.java: "Prefer focused unit tests; use@SpringBootTestonly when the Spring context is required."
638-694: Good coverage for overlaying location from web-reader/file-progress context.This test clearly verifies replacement of stale bookmark location with resolved KoboSpan location/content-source data.
798-918: Strong stale-data clearing coverage for two-way sync edge cases.The new cases for clearing stale EPUB CFI and nulling stale
contentSourceProgressPercentare valuable and directly protect regression-prone paths.booklore-api/src/test/java/org/booklore/service/kobo/KoboLibrarySyncServiceTest.java (2)
233-246: Good regression coverage for href-only web-reader sync.Line 233 adds the right focused case for
epubProgressHref+ percent, which protects the two-way sync behavior introduced in this PR.
304-304: Web-reader sync guard is correctly aligned with percent-based logic.Line 304 now keys off
epubProgressPercent, matching the new sync criteria while still preventing null-percent sync attempts.booklore-api/src/main/java/org/booklore/service/kobo/KoboLibrarySyncService.java (1)
225-226: Two-way progress predicate update looks correct.Line 226 correctly allows sync based on
epubProgressPercentwithout requiring CFI, which is needed for href-only/proxy progress flows.
2 participants
Description
This PR migrates to the use of KoboSpan data for progress tracking between Grimmory and Kobo. The existing implementation didn't work for two-way progression sync and failed to do anything other than show a dummy percentage figure in the Nickel menu. This new approach maps existing EPUB percentage + CFI data to a KoboSpan position within the sent KEPUB file, as well as the reverse to update Grimmory with accurate positioning.
Approx +1195 lines is production code (Most of this is heavily inspired by Komga for a tried and tested approach), another +1035 lines of added tests.
Linked Issue: Fixes #36
Changes
KoboSpanMapExtractionService,KoboSpanMapService,KoboBookmarkLocationResolver,KoboReadingStateBuilder,KoboReadingStateService,CfiConvertor. Exact behavior is largely similar to other projects which implement robust Kobo Sync support.kobo_span_maptable that caches the extracted span map per book file, invalidated automatically via file hash when a book is re-importeduser_book_file_progressto store progress from external sourcesKoboEntitlementServicepre loads the span mapsI'm reasonably happy with the actual guts of the code, nothing here is reinventing what's already done elsewhere, but @balazs-szucs would appreciate a look at the database/migration pieces and whether that's up to scratch. It works fine but not sure if there's a better way to do this in-line with what you're working on.
Verification
Tested live with a Kobo Libra Colour to confirm full two-way position syncing is working, freshest data is used, and to the best of our ability the exact book position is translated between percentage+CFI and KoboSpan forms. Updates in both directions are also instant.
Full backend + frontend tests ran and passed, including new tests.
Summary by CodeRabbit
New Features
Improvements
Configuration