fix(hardcover): hardcover sync duplicates reads and doesn't update progress#247
Merged
balazs-szucs merged 12 commits intoApr 7, 2026
Conversation
Contributor
📝 WalkthroughWalkthroughRefactors HardcoverSyncService to compute finished from progress (>=99%), fetch Changes
Sequence Diagram(s)sequenceDiagram
participant Service as HardcoverSyncService
participant GraphQL as GraphQL API
participant DB as DB/Backend
Service->>GraphQL: resolve hardcoverBook.bookId -> getUserBookAndReads(hardcoverId)
GraphQL-->>Service: UserBookWithReads / null
alt user book missing
Service->>GraphQL: insertUserBook(...)
GraphQL-->>Service: insertedUserBook or null
else user book exists
Service->>Service: compute isFinished from progress
alt userBook.status != CURRENTLY_READING or edition mismatch
Service->>GraphQL: updateUserBook(...)
GraphQL-->>Service: updated UserBookWithReads
end
end
alt try update compatible read
Service->>GraphQL: updateUserBookRead(userBookId, readInfo)
GraphQL-->>Service: success/failure
else insert read
Service->>GraphQL: insertUserBookRead(...)
GraphQL-->>Service: success/failure
end
Service->>DB: log result / update local state
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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
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 |
Contributor
|
So far, everything looks good on my end. I don't see any obvious functional issues. You clearly have deeper expertise here than I do, so if you're comfortable with this code, I am too. As mentioned, nothing stood out during local testing either Thanks for the PR! |
Contributor
Author
|
Hey @balazs-szucs I added some tests. I really don't like the way they're being done, but what I'd like to improve in them would require a few more changes, and I'm not sure it's worth it to hold this up for that. If it's okay, I propose we merge this PR, and then I could work on improving this to use a proper Hardcover service that we can mock to make the code easier to test and more reliable. What do you think? |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/hardcover/HardcoverSyncService.java`:
- Around line 167-187: The success log is emitted unconditionally because
requiresNewReadEntry is cleared even when updateUserBookRead(...) or
insertUserBookRead(...) fail; change both helper methods to return a boolean/or
result object indicating success, then only set requiresNewReadEntry = false
when the call returns success, and only emit the "Synced progress to Hardcover"
log after a successful return; apply the same pattern to the other Hardcover
sync blocks that call updateUserBookRead/insertUserBookRead (the repeated sync
sections referenced in the comment).
- Around line 156-159: The code assumes userBook.reads.getLast() is the most
recent read but the reads are fetched without an explicit order; update the
query that populates userBook.reads (the code that builds the user_book_reads
list used by UserBookReadInfo) to include an explicit ORDER BY (e.g., order by
read_timestamp or created_at DESC) and then pick the appropriate element
consistently (if ordering DESC use getFirst(), or if ordering ASC use getLast())
so userBook.reads.getLast()/UserBookReadInfo resolves to the actual latest read;
apply the same ordering change to the other fetch path that populates reads (the
block around the other user_book_reads fetch) to keep behavior consistent.
- Around line 160-171: HardcoverSyncService currently treats a missing
readInfo.editionId as a match but never backfills it, so later writes leave
user_book/user_book_read detached from the resolved edition; before calling
updateUserBookRead (and the corresponding user book update in the other branch
where editionId is null), set readInfo.editionId = hardcoverBook.editionId (and
also update the userBook's editionId field that will be persisted) so the
resolved hardcoverBook.editionId is written into both user_book_read and
user_book when you call updateUserBookRead(...) and the user-book update method.
- Around line 594-623: The updateUserBook method and the insert new read flow
build a bookInput map that sets the user_book's edition_id and status, but the
mutation variables always set status_id to STATUS_CURRENTLY_READING and ignore
isFinished. To fix, modify the mutation variable construction to use the correct
status_id based on isFinished (e.g., finished or currently reading) and include
the bookInput in the mutation variables so the parent user_book updates
alongside any new user_book_read inserts, ensuring the parent stays in sync with
the read rows.
🪄 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: de48dc9d-227c-4421-8e06-e0ea5f75b719
📒 Files selected for processing (2)
booklore-api/src/main/java/org/booklore/service/hardcover/HardcoverSyncService.javabooklore-api/src/test/java/org/booklore/service/hardcover/HardcoverSyncServiceTest.java
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Packaging Smoke Test
🧰 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/service/hardcover/HardcoverSyncServiceTest.javabooklore-api/src/main/java/org/booklore/service/hardcover/HardcoverSyncService.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/hardcover/HardcoverSyncServiceTest.java
🧠 Learnings (5)
📓 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/test/java/org/booklore/service/hardcover/HardcoverSyncServiceTest.javabooklore-api/src/main/java/org/booklore/service/hardcover/HardcoverSyncService.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/service/hardcover/HardcoverSyncServiceTest.javabooklore-api/src/main/java/org/booklore/service/hardcover/HardcoverSyncService.java
📚 Learning: 2026-03-24T18:46:10.869Z
Learnt from: pedronave
Repo: grimmory-tools/grimmory PR: 173
File: booklore-api/src/main/java/org/booklore/service/hardcover/HardcoverSyncService.java:220-225
Timestamp: 2026-03-24T18:46:10.869Z
Learning: In the Hardcover GraphQL API, empty strings (`""`) are not valid ISBN values and querying with `_eq: ""` returns no results. Therefore, binding a missing ISBN to `""` in `_or` edition filters in HardcoverSyncService.java is safe and will not accidentally match unrelated editions or books.
Applied to files:
booklore-api/src/test/java/org/booklore/service/hardcover/HardcoverSyncServiceTest.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/service/hardcover/HardcoverSyncService.java
🔇 Additional comments (1)
booklore-api/src/main/java/org/booklore/service/hardcover/HardcoverSyncService.java (1)
156-159: No action needed —List.getLast()is fully supported.The project targets Java 25 (configured in
booklore-api/build.gradle.kts), which includes full support forList.getLast()(available since Java 21). The code will compile and run correctly as written.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
booklore-api/src/main/java/org/booklore/service/hardcover/HardcoverSyncService.java (3)
624-626:⚠️ Potential issue | 🟠 MajorKeep
user_bookin sync when inserting a new read.
updateUserBook(...)hardcodesSTATUS_CURRENTLY_READING, andinsertUserBookRead(...)computesbookInputbut never sends it. If this flow inserts a new read for a finished sync or an edition switch, the childuser_book_readchanges but the parentuser_bookcan keep the oldstatus_id/edition_id, which leaves Hardcover’s library state inconsistent with the row you just inserted.🩹 Possible shape
- bookInput.put("status_id", STATUS_CURRENTLY_READING); + bookInput.put("status_id", isFinished ? STATUS_READ : STATUS_CURRENTLY_READING);- String mutation = """ - mutation InsertUserBookRead($userBookId: Int!, $userBookReadObject: DatesReadInput!) { - insert_user_book_read(user_book_id: $userBookId, user_book_read: $userBookReadObject) { + String mutation = """ + mutation InsertUserBookRead($userBookId: Int!, $userBookObject: UserBookUpdateInput!, $userBookReadObject: DatesReadInput!) { + update_user_book(id: $userBookId, object: $userBookObject) { + id + error + } + insert_user_book_read(user_book_id: $userBookId, user_book_read: $userBookReadObject) { user_book_read { id } @@ request.setVariables(Map.of( "userBookId", userBookId, + "userBookObject", bookInput, "userBookReadObject", readInput ));Also applies to: 678-709
🤖 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/hardcover/HardcoverSyncService.java` around lines 624 - 626, The parent user_book row isn't being updated when a new user_book_read is inserted because insertUserBookRead builds bookInput (edition_id/status_id) but never forwards it, and updateUserBook currently hardcodes STATUS_CURRENTLY_READING; modify insertUserBookRead to pass the computed bookInput into the call that updates/creates the parent (call/update to updateUserBook or the create flow) and change updateUserBook to accept and apply the incoming status_id and edition_id instead of always setting STATUS_CURRENTLY_READING; apply the same fix in the other similar block referenced (the block covering the 678-709 flow) so parent user_book edition_id/status_id are kept in sync with the newly inserted user_book_read.
159-170:⚠️ Potential issue | 🟠 MajorPopulate missing fields before reusing the blank read row.
When the latest read has
editionId == null, this path still sendsedition_id: nullback throughupdateUserBookRead(...), so the reused row stays detached from the resolved edition. The same problem exists forstartedAt: if Hardcover created a blank row, a finished sync still sendsstarted_at: null, which can recreate the “finish date only” entry from issue#143.🩹 Possible fix
if (readInfo.editionId == null || (readInfo.editionId != null && readInfo.editionId.equals(hardcoverBook.editionId))) { + if (readInfo.editionId == null) { + readInfo.editionId = hardcoverBook.editionId; + } + if (readInfo.startedAt == null && (progressPages > 0 || isFinished)) { + readInfo.startedAt = LocalDate.now().format(DateTimeFormatter.ISO_LOCAL_DATE); + } readInfo.progressPages = progressPages; if (isFinished) { readInfo.finishedAt = LocalDate.now().format(DateTimeFormatter.ISO_LOCAL_DATE); }Also applies to: 760-769
791-804:⚠️ Potential issue | 🟠 MajorReturn failure when either nested mutation reports an error.
This helper only treats top-level GraphQL
errorsas fatal. Ifupdate_user_book.errororupdate_user_book_read.erroris populated, it logs a warning and still returnstrue, so the caller can log a successful sync even though Hardcover rejected part of the update.🤖 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/hardcover/HardcoverSyncService.java` around lines 791 - 804, The current logic in HardcoverSyncService inspects nested GraphQL results (update_user_book and update_user_book_read) but only logs warnings and still returns true; change it so that if either updateUserBookResult.get("error") or updateUserBookReadResult.get("error") is non-null the method logs the error (or warn) and returns false instead of proceeding to the final "return true". Locate the block that reads response -> data -> update_user_book / update_user_book_read and add a boolean failure flag (or early return) that flips to false when either nested "error" is present, ensuring the method returns false to signal the overall sync failure.
🤖 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/hardcover/HardcoverSyncService.java`:
- Around line 124-185: Race condition: concurrent calls to
syncProgressToHardcover can read/modify/insert overlapping reads and create
duplicate user_book_read entries. Fix by serializing per user+book inside
syncProgressToHardcover: acquire a short-lived mutex keyed by userId+bookId
before calling getUserBookAndReads and hold it until after
insertUserBookRead/updateUserBookRead completes (and release it after), so only
one thread can perform the read-modify-write; implement the mutex using an
in-memory ConcurrentHashMap<String, Lock>/ReentrantLock or a dedicated
single-flight mechanism and ensure locks are always released (try/finally).
Apply this around the code paths that call getUserBookAndReads, insertUserBook,
updateUserBook, updateUserBookRead, and insertUserBookRead.
---
Duplicate comments:
In
`@booklore-api/src/main/java/org/booklore/service/hardcover/HardcoverSyncService.java`:
- Around line 624-626: The parent user_book row isn't being updated when a new
user_book_read is inserted because insertUserBookRead builds bookInput
(edition_id/status_id) but never forwards it, and updateUserBook currently
hardcodes STATUS_CURRENTLY_READING; modify insertUserBookRead to pass the
computed bookInput into the call that updates/creates the parent (call/update to
updateUserBook or the create flow) and change updateUserBook to accept and apply
the incoming status_id and edition_id instead of always setting
STATUS_CURRENTLY_READING; apply the same fix in the other similar block
referenced (the block covering the 678-709 flow) so parent user_book
edition_id/status_id are kept in sync with the newly inserted user_book_read.
- Around line 791-804: The current logic in HardcoverSyncService inspects nested
GraphQL results (update_user_book and update_user_book_read) but only logs
warnings and still returns true; change it so that if either
updateUserBookResult.get("error") or updateUserBookReadResult.get("error") is
non-null the method logs the error (or warn) and returns false instead of
proceeding to the final "return true". Locate the block that reads response ->
data -> update_user_book / update_user_book_read and add a boolean failure flag
(or early return) that flips to false when either nested "error" is present,
ensuring the method returns false to signal the overall sync failure.
🪄 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: fc8408b6-4549-417a-a733-0edfe8c2e8b0
📒 Files selected for processing (1)
booklore-api/src/main/java/org/booklore/service/hardcover/HardcoverSyncService.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/hardcover/HardcoverSyncService.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.
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:03.758Z
Learning: In grimmory-tools/grimmory (HardcoverSyncService.java), the Hardcover API already returns `user_book_reads` in a consistent, ordered fashion (ascending by recency), so `getLast()` reliably retrieves the most recent read entry. Adding an explicit `order_by` to the `user_book_reads` query is unnecessary.
📚 Learning: 2026-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/hardcover/HardcoverSyncService.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/hardcover/HardcoverSyncService.java
📚 Learning: 2026-04-01T17:50:03.758Z
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:03.758Z
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/hardcover/HardcoverSyncService.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/hardcover/HardcoverSyncService.java
📚 Learning: 2026-03-24T18:46:10.869Z
Learnt from: pedronave
Repo: grimmory-tools/grimmory PR: 173
File: booklore-api/src/main/java/org/booklore/service/hardcover/HardcoverSyncService.java:220-225
Timestamp: 2026-03-24T18:46:10.869Z
Learning: In the Hardcover GraphQL API, empty strings (`""`) are not valid ISBN values and querying with `_eq: ""` returns no results. Therefore, binding a missing ISBN to `""` in `_or` edition filters in HardcoverSyncService.java is safe and will not accidentally match unrelated editions or books.
Applied to files:
booklore-api/src/main/java/org/booklore/service/hardcover/HardcoverSyncService.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/service/hardcover/HardcoverSyncService.java
Contributor
Author
|
@balazs-szucs I found another quirk in this logic, need a bit longer to fix it. Hope to get it done today |
Contributor
Sounds good, no pressure :) |
Member
Can you resolve the open convos w/ a quick note for posterity why or why not they don't need to be addressed? Let me know if this is ready for review again and we can work to get this in ASAP :) |
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/hardcover/HardcoverSyncService.java (1)
690-703:⚠️ Potential issue | 🟡 MinorDead code:
bookInputis constructed but never used.Lines 690-692 build a
bookInputmap withedition_idandstatus_id, but this map is never included in the mutation variables (line 706-709). The mutation only sendsuserBookIdandreadInput.If the intent was to also update the parent
user_bookstatus/edition when inserting a new read, this is currently not happening. Otherwise, remove the dead code.🩹 Proposed fix (if dead code)
- Map<String, Object> bookInput = new java.util.HashMap<>(); - bookInput.put("edition_id", editionId); - bookInput.put("status_id", isFinished ? STATUS_READ : STATUS_CURRENTLY_READING); - Map<String, Object> readInput = new java.util.HashMap<>();🤖 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/hardcover/HardcoverSyncService.java` around lines 690 - 703, The Map bookInput is built but never used; either remove its construction or wire it into the GraphQL mutation variables so the parent user_book is updated. Locate the code that prepares the mutation variables around where readInput and userBookId are sent (see symbols bookInput, readInput, STATUS_READ, STATUS_CURRENTLY_READING and the enclosing class HardcoverSyncService) and either (A) delete the bookInput creation block if you do not intend to update the user_book, or (B) add bookInput to the mutation payload/variables alongside readInput and userBookId so edition_id and status_id are actually sent to the server. Ensure tests/usage paths that expect the parent status/edition behave accordingly after the change.
🧹 Nitpick comments (2)
booklore-api/src/main/java/org/booklore/service/hardcover/HardcoverSyncService.java (2)
43-50: Consider using@RequiredArgsConstructorfor constructor injection.The fields are already
final, so this class could use Lombok's@RequiredArgsConstructorinstead of explicit@Autowiredconstructor to align with Lombok patterns used elsewhere in the codebase.♻️ Suggested refactor
+import lombok.RequiredArgsConstructor; + `@Slf4j` `@Service` +@RequiredArgsConstructor public class HardcoverSyncService { private static final String HARDCOVER_API_URL = "https://api.hardcover.app/v1/graphql"; private static final int STATUS_CURRENTLY_READING = 2; private static final int STATUS_READ = 3; - private final RestClient restClient; + private final RestClient restClient = RestClient.builder() + .baseUrl(HARDCOVER_API_URL) + .build(); private final HardcoverSyncSettingsService hardcoverSyncSettingsService; private final BookRepository bookRepository; // Thread-local to hold the current API token for GraphQL requests private final ThreadLocal<String> currentApiToken = new ThreadLocal<>(); - - `@Autowired` - public HardcoverSyncService(HardcoverSyncSettingsService hardcoverSyncSettingsService, BookRepository bookRepository) { - this.hardcoverSyncSettingsService = hardcoverSyncSettingsService; - this.bookRepository = bookRepository; - this.restClient = RestClient.builder() - .baseUrl(HARDCOVER_API_URL) - .build(); - }As per coding guidelines: "Prefer constructor injection via Lombok patterns already used in the codebase."
🤖 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/hardcover/HardcoverSyncService.java` around lines 43 - 50, Replace the explicit `@Autowired` constructor in HardcoverSyncService with Lombok's `@RequiredArgsConstructor`: make sure the fields hardcoverSyncSettingsService, bookRepository and restClient are declared final (initialize restClient lazily or via a `@PostConstruct` if it requires HARDCOVER_API_URL), remove the manual constructor, and annotate the class with `@RequiredArgsConstructor` so Spring will use the generated constructor for HardcoverSyncSettingsService and BookRepository injection while handling restClient initialization appropriately (e.g., move RestClient.builder() into a field initializer or `@PostConstruct` method).
486-490: Consider using a more specific exception type.Throwing generic
Exceptionmakes it harder for callers to distinguish between error types. A custom exception (e.g.,HardcoverApiException) or a standard type likeIOExceptionwould be more descriptive.🤖 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/hardcover/HardcoverSyncService.java` around lines 486 - 490, Replace the two generic "throw new Exception(...)" usages in HardcoverSyncService (the method that processes the Hardcover response) with a more specific exception type: either create and throw a custom HardcoverApiException (e.g., extends IOException or RuntimeException) or throw an existing specific type such as IOException; update the method signature or callers if necessary to declare/handle the new exception, and ensure the new exception includes the same descriptive message (e.g., "No response from Hardcover for book ID ..." and "No data returned from Hardcover for book ID ...") so callers can distinguish Hardcover API errors from other failures.
🤖 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/hardcover/HardcoverSyncService.java`:
- Around line 780-789: The code in HardcoverSyncService currently logs a warning
when updateUserBookReadResult.get("error") is non-null but still returns true;
change the logic in the block that inspects response -> data ->
update_user_book_read so that if updateUserBookReadResult != null and
updateUserBookReadResult.get("error") != null the method returns false (instead
of falling through to return true), preserving the existing log.warn; use the
existing local variable updateUserBookReadResult to implement the early return
and ensure all null-safety checks remain in place.
---
Outside diff comments:
In
`@booklore-api/src/main/java/org/booklore/service/hardcover/HardcoverSyncService.java`:
- Around line 690-703: The Map bookInput is built but never used; either remove
its construction or wire it into the GraphQL mutation variables so the parent
user_book is updated. Locate the code that prepares the mutation variables
around where readInput and userBookId are sent (see symbols bookInput,
readInput, STATUS_READ, STATUS_CURRENTLY_READING and the enclosing class
HardcoverSyncService) and either (A) delete the bookInput creation block if you
do not intend to update the user_book, or (B) add bookInput to the mutation
payload/variables alongside readInput and userBookId so edition_id and status_id
are actually sent to the server. Ensure tests/usage paths that expect the parent
status/edition behave accordingly after the change.
---
Nitpick comments:
In
`@booklore-api/src/main/java/org/booklore/service/hardcover/HardcoverSyncService.java`:
- Around line 43-50: Replace the explicit `@Autowired` constructor in
HardcoverSyncService with Lombok's `@RequiredArgsConstructor`: make sure the
fields hardcoverSyncSettingsService, bookRepository and restClient are declared
final (initialize restClient lazily or via a `@PostConstruct` if it requires
HARDCOVER_API_URL), remove the manual constructor, and annotate the class with
`@RequiredArgsConstructor` so Spring will use the generated constructor for
HardcoverSyncSettingsService and BookRepository injection while handling
restClient initialization appropriately (e.g., move RestClient.builder() into a
field initializer or `@PostConstruct` method).
- Around line 486-490: Replace the two generic "throw new Exception(...)" usages
in HardcoverSyncService (the method that processes the Hardcover response) with
a more specific exception type: either create and throw a custom
HardcoverApiException (e.g., extends IOException or RuntimeException) or throw
an existing specific type such as IOException; update the method signature or
callers if necessary to declare/handle the new exception, and ensure the new
exception includes the same descriptive message (e.g., "No response from
Hardcover for book ID ..." and "No data returned from Hardcover for book ID
...") so callers can distinguish Hardcover API errors from other failures.
🪄 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: 7efcae4f-8ad5-4711-bee3-7ea1d521d1d4
📒 Files selected for processing (1)
booklore-api/src/main/java/org/booklore/service/hardcover/HardcoverSyncService.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/hardcover/HardcoverSyncService.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).
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.
Learnt from: pedronave
Repo: grimmory-tools/grimmory PR: 247
File: booklore-api/src/main/java/org/booklore/service/hardcover/HardcoverSyncService.java:156-159
Timestamp: 2026-04-01T17:50:06.817Z
Learning: In grimmory-tools/grimmory (HardcoverSyncService.java), the Hardcover API already returns `user_book_reads` in a consistent, ordered fashion (ascending by recency), so `getLast()` reliably retrieves the most recent read entry. Adding an explicit `order_by` to the `user_book_reads` query is unnecessary.
📚 Learning: 2026-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/hardcover/HardcoverSyncService.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/hardcover/HardcoverSyncService.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/hardcover/HardcoverSyncService.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/hardcover/HardcoverSyncService.java
📚 Learning: 2026-03-24T18:46:10.869Z
Learnt from: pedronave
Repo: grimmory-tools/grimmory PR: 173
File: booklore-api/src/main/java/org/booklore/service/hardcover/HardcoverSyncService.java:220-225
Timestamp: 2026-03-24T18:46:10.869Z
Learning: In the Hardcover GraphQL API, empty strings (`""`) are not valid ISBN values and querying with `_eq: ""` returns no results. Therefore, binding a missing ISBN to `""` in `_or` edition filters in HardcoverSyncService.java is safe and will not accidentally match unrelated editions or books.
Applied to files:
booklore-api/src/main/java/org/booklore/service/hardcover/HardcoverSyncService.java
📚 Learning: 2026-04-04T15:36:55.394Z
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:55.394Z
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/hardcover/HardcoverSyncService.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/service/hardcover/HardcoverSyncService.java
🔇 Additional comments (6)
booklore-api/src/main/java/org/booklore/service/hardcover/HardcoverSyncService.java (6)
159-177: BackfillreadInfo.editionIdwhen null to ensure the read is associated with the resolved edition.When
readInfo.editionId == null, the code proceeds to update but never assignshardcoverBook.editionIdtoreadInfo.editionId. Subsequently, line 756 writes the still-nulleditionIdto the API, leaving the read detached from the resolved edition.Also, the condition on line 160 has a redundant null check:
(readInfo.editionId != null && ...)is always true at that point due to short-circuit evaluation.🩹 Proposed fix
// Only update if the edition matches (to avoid updating progress on a different edition if the user restarted the book with a different edition). If edition is missing, we assume it's the same edition and update it. - if (readInfo.editionId == null || (readInfo.editionId != null && readInfo.editionId.equals(hardcoverBook.editionId))) { + if (readInfo.editionId == null || readInfo.editionId.equals(hardcoverBook.editionId)) { + // Backfill edition_id if missing so the read is properly associated + if (readInfo.editionId == null) { + readInfo.editionId = hardcoverBook.editionId; + } readInfo.progressPages = progressPages;
124-149: Serialization of sync per user/book remains unaddressed.This multi-step read/modify/write flow is still vulnerable to race conditions when concurrent async calls for the same user+book overlap. Both could observe the same remote state and insert duplicate reads, which is the exact issue this PR aims to fix.
524-588: LGTM!The
insertUserBookmethod has proper error handling for both GraphQL-level errors and response-level errors, and correctly returns the createdUserBookWithReadsor null on failure.
597-666: LGTM!The
updateUserBookmethod appropriately handles the Hardcover API quirks as documented in the comments, and consistently returns null on any error condition.
857-895: LGTM!The helper classes
UserBookWithReadsandUserBookReadInfoare appropriate internal DTOs for mapping GraphQL responses. ThefromMapfactory method cleanly handles the conversion.
825-838: LGTM!The
extractFloathelper follows the same pattern asextractIntegerand properly handles the various types that can come from GraphQL responses.
Contributor
Author
|
Sorry, was away for the extended easter weekend. So far, the only other quirk I can find is when the user_book is not marked as read, and we insert a finished user_book_read (this happens if the first progress sync is for 100% progress), the book status stays Currently Reading instead of Read. |
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
booklore-api/src/main/java/org/booklore/service/hardcover/HardcoverSyncService.java (2)
160-177:⚠️ Potential issue | 🟠 MajorBackfill missing read
edition_idbeforeupdate_user_book_read.The null-edition match branch updates progress but keeps
readInfo.editionIdnull, and then Line 756 sends that null back in mutation payload. This leaves the read detached from the resolved edition.🩹 Suggested fix
- if (readInfo.editionId == null || (readInfo.editionId != null && readInfo.editionId.equals(hardcoverBook.editionId))) { + if (readInfo.editionId == null || readInfo.editionId.equals(hardcoverBook.editionId)) { + if (readInfo.editionId == null) { + readInfo.editionId = hardcoverBook.editionId; + } readInfo.progressPages = progressPages;Also applies to: 756-757
🤖 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/hardcover/HardcoverSyncService.java` around lines 160 - 177, The branch in HardcoverSyncService where readInfo.editionId is null but matches the hardcoverBook should set readInfo.editionId = hardcoverBook.editionId before calling updateUserBookRead so the mutation payload contains the resolved edition; specifically, in the block that logs "Updating existing reading progress..." and calls updateUserBookRead(userBook.id, readInfo), assign readInfo.editionId = hardcoverBook.editionId (and then proceed to set progressPages/finishedAt and call updateUserBookRead) to avoid persisting a null edition_id.
61-192:⚠️ Potential issue | 🟠 MajorSerialize sync per
userId+bookIdto prevent duplicateuser_book_readinserts.
syncProgressToHardcover(...)is async and invoked from multiple services; concurrent calls for the same user/book can still run the same read-modify-write sequence and both choose the “insert new read” path.🛠️ Suggested fix
+import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.locks.ReentrantLock; ... public class HardcoverSyncService { + private final ConcurrentHashMap<String, ReentrantLock> syncLocks = new ConcurrentHashMap<>(); ... public void syncProgressToHardcover(Long bookId, Float progressPercent, Long userId) { try { ... - Integer hardcoverBookIdInt = extractInteger(hardcoverBook.bookId); + Integer hardcoverBookIdInt = extractInteger(hardcoverBook.bookId); + String lockKey = userId + ":" + hardcoverBookIdInt; + ReentrantLock lock = syncLocks.computeIfAbsent(lockKey, k -> new ReentrantLock()); + lock.lock(); + try { // existing getUserBookAndReads / update / insert flow + } finally { + lock.unlock(); + if (!lock.hasQueuedThreads()) { + syncLocks.remove(lockKey, lock); + } + }🤖 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/hardcover/HardcoverSyncService.java` around lines 61 - 192, Concurrent executions of syncProgressToHardcover(userId, bookId, ...) can race and create duplicate user_book_read entries; serialize operations per userId+bookId. Fix by adding a per-key lock (e.g., a ConcurrentHashMap<String, ReentrantLock> or Guava Striped lock) in HardcoverSyncService and acquire the lock at the start of syncProgressToHardcover using a key like userId + ":" + bookId, then hold the lock around the read-modify-write sequence that calls getUserBookAndReads, insertUserBook, updateUserBook, insertUserBookRead and updateUserBookRead, and finally release the lock in a try/finally (cleaning up the lock map entry when no longer needed); alternatively, implement the serialization using a DB-level pessimistic lock when loading the user_book row in getUserBookAndReads to ensure only one updater proceeds.
🧹 Nitpick comments (1)
booklore-api/src/main/java/org/booklore/service/hardcover/HardcoverSyncService.java (1)
516-523: Align Javadocs with actual method signatures and behavior.
insertUserBook(...)Javadoc documentsprogressPagesandisFinishedparams that are not in the signature, andinsertUserBookRead(...)Javadoc says it updatesuser_bookstate even though this method only sendsuserBookReadObject.Also applies to: 668-677
🤖 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/hardcover/HardcoverSyncService.java` around lines 516 - 523, The Javadocs are out of sync: update the JavaDoc for insertUserBook(...) to only document the actual method parameters and behavior (remove references to progressPages and isFinished if they are not in the signature, and describe that it inserts a user_book and creates a user_book_read with the editionId provided), and update the JavaDoc for insertUserBookRead(...) to reflect that it only sends/persists the provided userBookReadObject (do not claim it updates user_book state if it does not). Make the descriptions and `@param/`@return tags match the actual method signatures and returned types (e.g., UserBookWithReads, userBookReadObject, editionId) and ensure consistency with the related JavaDoc block at the other location (lines ~668-677).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In
`@booklore-api/src/main/java/org/booklore/service/hardcover/HardcoverSyncService.java`:
- Around line 160-177: The branch in HardcoverSyncService where
readInfo.editionId is null but matches the hardcoverBook should set
readInfo.editionId = hardcoverBook.editionId before calling updateUserBookRead
so the mutation payload contains the resolved edition; specifically, in the
block that logs "Updating existing reading progress..." and calls
updateUserBookRead(userBook.id, readInfo), assign readInfo.editionId =
hardcoverBook.editionId (and then proceed to set progressPages/finishedAt and
call updateUserBookRead) to avoid persisting a null edition_id.
- Around line 61-192: Concurrent executions of syncProgressToHardcover(userId,
bookId, ...) can race and create duplicate user_book_read entries; serialize
operations per userId+bookId. Fix by adding a per-key lock (e.g., a
ConcurrentHashMap<String, ReentrantLock> or Guava Striped lock) in
HardcoverSyncService and acquire the lock at the start of
syncProgressToHardcover using a key like userId + ":" + bookId, then hold the
lock around the read-modify-write sequence that calls getUserBookAndReads,
insertUserBook, updateUserBook, insertUserBookRead and updateUserBookRead, and
finally release the lock in a try/finally (cleaning up the lock map entry when
no longer needed); alternatively, implement the serialization using a DB-level
pessimistic lock when loading the user_book row in getUserBookAndReads to ensure
only one updater proceeds.
---
Nitpick comments:
In
`@booklore-api/src/main/java/org/booklore/service/hardcover/HardcoverSyncService.java`:
- Around line 516-523: The Javadocs are out of sync: update the JavaDoc for
insertUserBook(...) to only document the actual method parameters and behavior
(remove references to progressPages and isFinished if they are not in the
signature, and describe that it inserts a user_book and creates a user_book_read
with the editionId provided), and update the JavaDoc for insertUserBookRead(...)
to reflect that it only sends/persists the provided userBookReadObject (do not
claim it updates user_book state if it does not). Make the descriptions and
`@param/`@return tags match the actual method signatures and returned types (e.g.,
UserBookWithReads, userBookReadObject, editionId) and ensure consistency with
the related JavaDoc block at the other location (lines ~668-677).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6bbb588c-e3d3-48e7-9b8a-13cd0b4b8716
📒 Files selected for processing (1)
booklore-api/src/main/java/org/booklore/service/hardcover/HardcoverSyncService.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/hardcover/HardcoverSyncService.java
🧠 Learnings (9)
📓 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.
Learnt from: pedronave
Repo: grimmory-tools/grimmory PR: 247
File: booklore-api/src/main/java/org/booklore/service/hardcover/HardcoverSyncService.java:156-159
Timestamp: 2026-04-01T17:50:06.817Z
Learning: In grimmory-tools/grimmory (HardcoverSyncService.java), the Hardcover API already returns `user_book_reads` in a consistent, ordered fashion (ascending by recency), so `getLast()` reliably retrieves the most recent read entry. Adding an explicit `order_by` to the `user_book_reads` query is unnecessary.
📚 Learning: 2026-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/hardcover/HardcoverSyncService.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/hardcover/HardcoverSyncService.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/hardcover/HardcoverSyncService.java
📚 Learning: 2026-03-24T18:46:10.869Z
Learnt from: pedronave
Repo: grimmory-tools/grimmory PR: 173
File: booklore-api/src/main/java/org/booklore/service/hardcover/HardcoverSyncService.java:220-225
Timestamp: 2026-03-24T18:46:10.869Z
Learning: In the Hardcover GraphQL API, empty strings (`""`) are not valid ISBN values and querying with `_eq: ""` returns no results. Therefore, binding a missing ISBN to `""` in `_or` edition filters in HardcoverSyncService.java is safe and will not accidentally match unrelated editions or books.
Applied to files:
booklore-api/src/main/java/org/booklore/service/hardcover/HardcoverSyncService.java
📚 Learning: 2026-04-04T15:36:55.394Z
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:55.394Z
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/hardcover/HardcoverSyncService.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/hardcover/HardcoverSyncService.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/hardcover/HardcoverSyncService.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/service/hardcover/HardcoverSyncService.java
🔇 Additional comments (1)
booklore-api/src/main/java/org/booklore/service/hardcover/HardcoverSyncService.java (1)
780-790: Good fix on mutation error handling.Returning
falsewhenupdate_user_book_read.erroris present correctly prevents false success logging in the caller path.
Member
@pedronave Given the improvements in behaviour within this PR and the fact your mentioned issue is not a regression from current behaviour, I wouldn't necessarily consider that issue a blocker for this first approach to fixing Hardcover. Would you like to take a stab at the Currently Reading issue in this PR, or should we make a follow up issue for it? |
Contributor
Author
I'd say we can go ahead with this PR and I'll try to get to it later on. Maybe the hardcover will make some improvements in the meantime :D |
balazs-szucs
approved these changes
Apr 7, 2026
balazs-szucs
left a comment
Contributor
There was a problem hiding this comment.
Thank you! Sounds good to me.
zachyale
pushed a commit
to zachyale/grimmory
that referenced
this pull request
Apr 17, 2026
…ogress (grimmory-tools#247) * fix: use ISBN to match the edition to sync * fix: optimize fallback to default editions when ISBN is not present * chore: added tests for hardcover sync * fix: handle cases where hardcover does not have page count * fix: initial fix to hardcover sync * added some additional logic to cover the hardcover API * feat: added tests for hardcover progress sync * chore: code cleanup * fix: check if the progress insert and update are actually successful before logging * fix: ensure book edition id is being updated * fix: updateUserBookRead returned true even if there was an error
zachyale
pushed a commit
to zachyale/grimmory
that referenced
this pull request
Apr 17, 2026
…ogress (grimmory-tools#247) * fix: use ISBN to match the edition to sync * fix: optimize fallback to default editions when ISBN is not present * chore: added tests for hardcover sync * fix: handle cases where hardcover does not have page count * fix: initial fix to hardcover sync * added some additional logic to cover the hardcover API * feat: added tests for hardcover progress sync * chore: code cleanup * fix: check if the progress insert and update are actually successful before logging * fix: ensure book edition id is being updated * fix: updateUserBookRead returned true even if there was an error
zachyale
pushed a commit
that referenced
this pull request
Apr 17, 2026
…ogress (#247) * fix: use ISBN to match the edition to sync * fix: optimize fallback to default editions when ISBN is not present * chore: added tests for hardcover sync * fix: handle cases where hardcover does not have page count * fix: initial fix to hardcover sync * added some additional logic to cover the hardcover API * feat: added tests for hardcover progress sync * chore: code cleanup * fix: check if the progress insert and update are actually successful before logging * fix: ensure book edition id is being updated * fix: updateUserBookRead returned true even if there was an error
zachyale
pushed a commit
that referenced
this pull request
Apr 22, 2026
…ogress (#247) * fix: use ISBN to match the edition to sync * fix: optimize fallback to default editions when ISBN is not present * chore: added tests for hardcover sync * fix: handle cases where hardcover does not have page count * fix: initial fix to hardcover sync * added some additional logic to cover the hardcover API * feat: added tests for hardcover progress sync * chore: code cleanup * fix: check if the progress insert and update are actually successful before logging * fix: ensure book edition id is being updated * fix: updateUserBookRead returned true even if there was an error
dsmouse
pushed a commit
to dsmouse/grimmory
that referenced
this pull request
May 11, 2026
…ogress (grimmory-tools#247) * fix: use ISBN to match the edition to sync * fix: optimize fallback to default editions when ISBN is not present * chore: added tests for hardcover sync * fix: handle cases where hardcover does not have page count * fix: initial fix to hardcover sync * added some additional logic to cover the hardcover API * feat: added tests for hardcover progress sync * chore: code cleanup * fix: check if the progress insert and update are actually successful before logging * fix: ensure book edition id is being updated * fix: updateUserBookRead returned true even if there was an error
This was referenced May 25, 2026
2 tasks
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
3 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
This PR fixes the issue where hardcover progress sync creates two dates read, and the progress doesn't update.
There are some quirks with the Hardcover API that make this update flow a bit harder, and may change other read dates that are not finished yet, but overall should work well for simpler cases, and should ensure that the progress update is always visible.
Linked Issue: Fixes #143
Changes
The sync process now considers the existing read dates and current status of the book.
We update the existing user_book (book in the user's library) to match the reading status and edition.
Then we update the latest user_book_read that has the edition ID we're trying to sync. If it doesn't exist, we insert a new one. We need to update the last one because this is what Hardcover uses to display the progress in the main page.
Summary by CodeRabbit
Bug Fixes
Tests