fix(hardcover): use hardcover edition based on ISBN for syncing progress#173
Conversation
📝 WalkthroughWalkthroughThis refactoring consolidates four separate Hardcover lookup methods into a single unified Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 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)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 291-294: The code currently falls back to the book-level page
count by assigning info.pages = extractInteger(book.get("pages")) when an
edition has no pages; remove these fallback assignments so that info.pages
remains null when the chosen edition lacks a page count (this affects the block
using info.pages and the similar fallback at the second occurrence around the
382-385 region). Locate the assignment to info.pages (and any analogous
assignment) in HardcoverSyncService and delete or disable the fallback to
extractInteger(book.get("pages")), leaving info.pages null so
syncProgressToHardcover() can correctly abort on a null page count.
- Around line 220-225: In HardcoverSyncService, stop binding empty strings into
the GraphQL `_or` edition filters (the block that builds `editions(where: {_or:
[...]})`); instead construct the `_or` array only with present identifiers
(include `{isbn_13: {_eq: ...}}` only when isbn13 is non-empty and `{isbn_10:
{_eq: ...}}` only when isbn10 is non-empty) and if neither ISBN is present skip
adding the entire `editions` edition-match filter. Apply the same change to the
other query constructions referenced (the similar blocks around the lines called
out) so that edition-match filters are only included when at least one real ISBN
value exists.
- Around line 188-199: The current catch for NumberFormatException in
HardcoverSyncService stops processing and returns null, preventing fallback ISBN
resolution; instead, when Integer.parseInt(hardcoverBookId) fails, call
resolveByIsbn(isbn13, isbn10) (same args used below) so malformed/stale
hardcoverBookId falls through to ISBN-based lookup; update the catch block to
log the warning (log.warn("Invalid Hardcover book ID format: {}",
hardcoverBookId)) and then return resolveByIsbn(isbn13, isbn10) rather than
returning null, leaving resolveByBookId and resolveByIsbn usage unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1ab84877-5456-43c6-939d-9be8402692bb
📒 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
| // We have a specific bookId, try to resolve using it (with optional ISBN for edition matching) | ||
| if (hardcoverBookId != null && !hardcoverBookId.isBlank()) { | ||
| try { | ||
| return resolveByBookId(Integer.parseInt(hardcoverBookId), isbn13, isbn10); | ||
| } catch (NumberFormatException e) { | ||
| log.warn("Invalid Hardcover book ID format: {}", hardcoverBookId); | ||
| return null; | ||
| } | ||
|
|
||
| // Navigate the response to get book info | ||
| Map<String, Object> data = (Map<String, Object>) response.get("data"); | ||
| if (data == null) return null; | ||
|
|
||
| Map<String, Object> search = (Map<String, Object>) data.get("search"); | ||
| if (search == null) return null; | ||
|
|
||
| Map<String, Object> results = (Map<String, Object>) search.get("results"); | ||
| if (results == null) return null; | ||
|
|
||
| List<Map<String, Object>> hits = (List<Map<String, Object>>) results.get("hits"); | ||
| if (hits == null || hits.isEmpty()) return null; | ||
|
|
||
| Map<String, Object> document = (Map<String, Object>) hits.getFirst().get("document"); | ||
| if (document == null) return null; | ||
|
|
||
| // Extract book info | ||
| HardcoverBookInfo info = new HardcoverBookInfo(); | ||
|
|
||
| // The 'id' field contains the book ID | ||
| Object idObj = document.get("id"); | ||
| if (idObj instanceof String) { | ||
| info.bookId = (String) idObj; | ||
| } else if (idObj instanceof Number) { | ||
| info.bookId = String.valueOf(((Number) idObj).intValue()); | ||
| } | ||
|
|
||
| // Get page count | ||
| Object pagesObj = document.get("pages"); | ||
| if (pagesObj instanceof Number) { | ||
| info.pages = ((Number) pagesObj).intValue(); | ||
| } | ||
|
|
||
| // Try to get default_physical_edition_id from the search results | ||
| Object defaultPhysicalEditionObj = document.get("default_physical_edition_id"); | ||
| if (defaultPhysicalEditionObj instanceof Number) { | ||
| info.editionId = ((Number) defaultPhysicalEditionObj).intValue(); | ||
| } else if (defaultPhysicalEditionObj instanceof String) { | ||
| try { | ||
| info.editionId = Integer.parseInt((String) defaultPhysicalEditionObj); | ||
| } catch (NumberFormatException e) { | ||
| // Ignore | ||
| } | ||
| } | ||
|
|
||
| // If no default physical edition found, try to look up edition by ISBN as fallback | ||
| if (info.bookId != null && info.editionId == null) { | ||
| EditionInfo edition = findEditionByIsbn(info.bookId, isbn); | ||
| if (edition != null) { | ||
| info.editionId = edition.id; | ||
| } | ||
| } | ||
|
|
||
| // Fetch page count from the edition (prioritizing edition page count over book-level page count) | ||
| if (info.editionId != null) { | ||
| EditionInfo edition = findEditionById(info.editionId); | ||
| if (edition != null && edition.pages != null && edition.pages > 0) { | ||
| info.pages = edition.pages; | ||
| log.debug("Using page count from edition {}: {} pages", info.editionId, info.pages); | ||
| } | ||
| } | ||
|
|
||
| log.info("Found Hardcover book: bookId={}, editionId={}, pages={}", | ||
| info.bookId, info.editionId, info.pages); | ||
|
|
||
| return info.bookId != null ? info : null; | ||
|
|
||
| } catch (Exception e) { | ||
| log.warn("Failed to search Hardcover by ISBN {}: {}", isbn, e.getMessage()); | ||
| return null; | ||
| } | ||
|
|
||
| // No bookId but we have ISBN, try to resolve book by ISBN | ||
| return resolveByIsbn(isbn13, isbn10); |
There was a problem hiding this comment.
Fall back to ISBN resolution when hardcoverBookId is malformed.
A stale/non-numeric stored ID currently aborts the whole lookup even when isbn13/isbn10 is present. That turns a recoverable metadata issue into a skipped sync.
Suggested fix
if (hardcoverBookId != null && !hardcoverBookId.isBlank()) {
try {
return resolveByBookId(Integer.parseInt(hardcoverBookId), isbn13, isbn10);
} catch (NumberFormatException e) {
log.warn("Invalid Hardcover book ID format: {}", hardcoverBookId);
- return null;
+ return resolveByIsbn(isbn13, isbn10);
}
}🤖 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 188 - 199, The current catch for NumberFormatException in
HardcoverSyncService stops processing and returns null, preventing fallback ISBN
resolution; instead, when Integer.parseInt(hardcoverBookId) fails, call
resolveByIsbn(isbn13, isbn10) (same args used below) so malformed/stale
hardcoverBookId falls through to ISBN-based lookup; update the catch block to
log the warning (log.warn("Invalid Hardcover book ID format: {}",
hardcoverBookId)) and then return resolveByIsbn(isbn13, isbn10) rather than
returning null, leaving resolveByBookId and resolveByIsbn usage unchanged.
There was a problem hiding this comment.
I think we should prefer to respect the user metadata instead of overriding it, but am open to suggestions
There was a problem hiding this comment.
@pedronave, that's a fair point — respecting explicit user intent is the safer choice here. If a user has deliberately set a hardcoverBookId, silently falling back to ISBN could sync progress to a completely different book than intended, which is arguably worse than skipping the sync altogether. The current behavior surfaces the data problem (malformed ID) rather than masking it, and the warning log at least gives visibility into the issue.
The only scenario where the fallback would help is a transient/accidental corruption of the stored ID, but the risk of a wrong-book sync outweighs that benefit. I'm fine with the existing approach — no change needed here.
✏️ Learnings added
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:44.627Z
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.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
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:45:53.922Z
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.
| editions(where: { | ||
| _or: [ | ||
| {isbn_13: {_eq: $isbn13}}, | ||
| {isbn_10: {_eq: $isbn10}} | ||
| ] | ||
| }, order_by: {users_count: desc}, limit: 1) { |
There was a problem hiding this comment.
Don't bind missing ISBNs to "" in these _or filters.
On the bookId-only path, and on single-ISBN lookups, the missing side of the predicate becomes _eq: "". That makes blank-ISBN editions/books eligible for selection, so a more popular unrelated record can win over the real ISBN match or the intended default-edition fallback. Build the _or from populated identifiers only, and skip the edition-match filter entirely when no ISBN is present.
Also applies to: 237-238, 318-333, 344-345
🤖 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 220 - 225, In HardcoverSyncService, stop binding empty strings into
the GraphQL `_or` edition filters (the block that builds `editions(where: {_or:
[...]})`); instead construct the `_or` array only with present identifiers
(include `{isbn_13: {_eq: ...}}` only when isbn13 is non-empty and `{isbn_10:
{_eq: ...}}` only when isbn10 is non-empty) and if neither ISBN is present skip
adding the entire `editions` edition-match filter. Apply the same change to the
other query constructions referenced (the similar blocks around the lines called
out) so that edition-match filters are only included when at least one real ISBN
value exists.
There was a problem hiding this comment.
This is not an issue as Hardcover does not allow empty strings for ISBNs.
In fact, running this query with both ISBNs empty returns no results across all books
There was a problem hiding this comment.
@pedronave, thanks for clarifying! Good to know that Hardcover's API rejects empty strings for ISBN fields, so _eq: "" will match nothing — the concern doesn't apply here. I'll note this for future reviews.
✏️ Learnings added
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:45:53.922Z
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.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| // Fallback to book-level pages if edition has no pages | ||
| if (info.pages == null) { | ||
| info.pages = extractInteger(book.get("pages")); | ||
| } |
There was a problem hiding this comment.
Keep pages null when the chosen edition has no page count.
Falling back to book.pages here bypasses the new guard in syncProgressToHardcover(): an ISBN-matched edition with pages = null will still sync using a book-level count that may belong to a different edition. Let the caller see pages == null and abort the sync instead.
Also applies to: 382-385
🤖 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 291 - 294, The code currently falls back to the book-level page
count by assigning info.pages = extractInteger(book.get("pages")) when an
edition has no pages; remove these fallback assignments so that info.pages
remains null when the chosen edition lacks a page count (this affects the block
using info.pages and the similar fallback at the second occurrence around the
382-385 region). Locate the assignment to info.pages (and any analogous
assignment) in HardcoverSyncService and delete or disable the fallback to
extractInteger(book.get("pages")), leaving info.pages null so
syncProgressToHardcover() can correctly abort on a null page count.
There was a problem hiding this comment.
Hardcover does not allow setting progress of the edition if it doesn't have a pages count (1), but it is possible to do so over API. It'll display in some places (2) and not in others (3).
Since the pages count is just a proxy for the progress %, I'd prefer to have it synced anyway, but am open to other suggestions
(1)
(2)
(3)
There was a problem hiding this comment.
@pedronave, thank you for the detailed screenshots — that makes the behavior very clear:
- Image 1: The Hardcover UI blocks progress entry when an edition has no page count.
- Image 2: The API bypass works — 41% progress is visible in the book detail dropdown.
- Image 3: The dashboard "Currently Reading" section shows "Add page count" rather than the synced percentage, so there is a partial display gap.
Your reasoning is sound: the page count is only a denominator to convert a percentage into page numbers for the API call, and when the matched edition has no pages, a book-level fallback still produces an accurate progress percentage (just anchored to a slightly different total). Skipping the sync entirely in that case would be strictly worse than the small approximation risk.
I'm happy to withdraw this suggestion. The current fallback to book.pages is a reasonable choice for this use case, and the behavior is consistent with what users would expect (sync continues rather than silently fails).
✏️ Learnings added
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:05.492Z
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).
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
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:44.627Z
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: 173
File: booklore-api/src/main/java/org/booklore/service/hardcover/HardcoverSyncService.java:220-225
Timestamp: 2026-03-24T18:45:53.922Z
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.
WTF did we all decide to read this series? I too am getting progress issues. Specifically, when Grimmory updates HC, it seems to remove my reading start date and it changes to physical copy of the book. |
balazs-szucs
left a comment
There was a problem hiding this comment.
Thank you Pedro!
Hope to see more Hardcover PRs from, this is great :)
…ess (grimmory-tools#173) * 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
…ess (grimmory-tools#173) * 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
…ess (#173) * 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
…ess (#173) * 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
Description
Updates the Hardcover progress sync to try to get the appropriate edition using ISBN, instead of always defaulting to the physical edition.
If the ISBN edition is not found, falls back to the default_ebook_edition; if that is not found, falls back to the default_physical_edition
This also optimizes the requests for the correct edition, having only to perform 1 API request to fetch the edition id and page count.
I'm not too happy with the code or the tests, but I don't want to be changing too much before we've stabilized for v3.
Linked Issue: Fixes #161
Changes
Summary by CodeRabbit
Refactor
Bug Fixes