fix: reading sessions silently never sync for books linked outside the plugin - #201
fix: reading sessions silently never sync for books linked outside the plugin#201luizcorreia wants to merge 6 commits into
Conversation
GrimmoryAPI:recordSession hardcoded bookType to "EPUB" for every reading session pushed to Grimmory, regardless of the book's actual format (PDF, CBZ, MOBI, ...). Sessions for non-EPUB books were being recorded on the server with the wrong format. Derive it instead from the book's file extension (falling back to the previous "EPUB" default when it can't be determined), and thread it through recordSession as an explicit parameter instead of a constant.
…immory pushBookSessions would break out of the whole loop the first time it hit a session whose book had no grimmory_id, logging it as a "session-error". Since grimmory_id comes from the book itself, it's the same for every pending session of that book - so this wasn't a transient error worth retrying, it just silently stalled session sync for that book on every future run (as long as it kept having reading activity to push), with no path to recovery. Check once up front instead: if the book isn't linked yet, report a single "session-unlinked" event and return, rather than looping through every pending session only to fail (and log) each one individually. Real recordSession failures still break the loop so a run can retry.
Books only ever got a grimmory_id in two situations: being downloaded
by this plugin, or the user manually running "Sync with Grimmory" on
that specific file from the File Manager. A book already on disk from
somewhere else (an OPDS download, a manual copy, a previous KOReader
setup, ...) never got linked automatically, which meant a full/periodic
sync silently never pushed its reading sessions or progress at all:
getBooksPendingSync requires grimmory_id IS NOT NULL, and nothing in
the full-sync path ever tried to establish that link in the first
place.
Add GrimmoryLocalRepository:getUnlinkedBooksWithEvents() to find local
books that have reading activity but no Grimmory link, and
GrimmorySynchronize:associateUnlinkedBooks() to try to match them
against the remote catalog (reusing the existing ISBN/ASIN-based
associateBook matching) before a full sync decides what's pending.
associateBook now accepts an optional pre-fetched catalog so the
remote book list is only fetched once per sync, no matter how many
local books need matching, instead of once per book.
Surface both new outcomes ("book-linked", "session-unlinked") in the
sync summary toast so it's no longer silent when a book can't have its
reading activity synced.
…her name Shelf sync only recognized a book as "already downloaded" by checking a previously-tracked grimmory_id, or by guessing a filename inside the configured download directory from the catalog's primaryFile name. A book already present on disk from anywhere else - OPDS, a manual copy, a prior KOReader setup - was invisible to that check, so shelf sync would download a second copy under its own generated filename. The original file (often the one with all the actual reading history) was left as an orphan with no grimmory_id, and the fresh duplicate had none of that history. Add GrimmoryLocalRepository:getUnlinkedBooks() and GrimmorySynchronize:findLocalBookMatch(), which checks already-known local files with no grimmory_id against the remote catalog entry via the same ISBN/ASIN-based doc_metadata:isBook() matching used elsewhere, before falling back to constructing a fresh download path. When a match is found, getBookDownloadPath returns the existing file instead, and pullBook's existing upsertBook call links it in place - no download, no duplicate.
📝 WalkthroughWalkthroughThe synchronization flow derives session book types, identifies unlinked activity, associates local books with remote catalog entries, reuses matching local files, and reports linking outcomes. Repository queries and synchronization specifications cover these behaviors. ChangesUnlinked synchronization
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Sync
participant Repository
participant API
participant Metadata
Sync->>Repository: Fetch unlinked books with events
Sync->>API: Fetch remote catalog once
Sync->>Metadata: Match local documents
Sync->>Repository: Associate matching books
Sync-->>Sync: Report book-linked
Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
spec/grimmory/synchronize_spec.lua (1)
194-312: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGood coverage for the new matching/reuse logic, but no test for
pushAllPendingBookMetadata's integration withassociateUnlinkedBooks.Consider adding a test asserting
associateUnlinkedBooksis invoked (and that a failure inside it doesn't preventbook_idsprocessing), which would also guard against the missing-pcallconcern raised ingrimmory.koplugin/grimmory/synchronize.lua(Lines 258-264).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@spec/grimmory/synchronize_spec.lua` around lines 194 - 312, The existing specs cover associateUnlinkedBooks directly but not its integration with pushAllPendingBookMetadata. Add a test around pushAllPendingBookMetadata that spies on associateUnlinkedBooks, verifies it is invoked, and simulates its failure to confirm processing of book_ids still occurs without propagating the error. Use the existing callback and repository/API spies to assert the remaining metadata flow continues.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@grimmory.koplugin/grimmory/synchronize.lua`:
- Around line 511-521: The reused local file returned by findLocalBookMatch must
be linked to book.id even when downloadBook is skipped because the file already
exists. Update the existing-book branch in getBookDownloadPath and its caller so
the reused path performs upsertBook with the resolved download path and book.id,
while preserving the current download-success handling without duplicating
links.
- Around line 258-264: Wrap the associateUnlinkedBooks call in
GrimmorySynchronize:pushAllPendingBookMetadata with pcall, matching the
error-isolation pattern used by pushBookMetadata. Ensure any exception from
associateUnlinkedBooks is handled without aborting the subsequent book_ids
computation and metadata/session synchronization for already-linked books.
---
Nitpick comments:
In `@spec/grimmory/synchronize_spec.lua`:
- Around line 194-312: The existing specs cover associateUnlinkedBooks directly
but not its integration with pushAllPendingBookMetadata. Add a test around
pushAllPendingBookMetadata that spies on associateUnlinkedBooks, verifies it is
invoked, and simulates its failure to confirm processing of book_ids still
occurs without propagating the error. Use the existing callback and
repository/API spies to assert the remaining metadata flow continues.
🪄 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 Plus
Run ID: e44f37de-0e4d-4715-805f-7b9219ce1268
📒 Files selected for processing (5)
grimmory.koplugin/grimmory/grimmory_api.luagrimmory.koplugin/grimmory/repository.luagrimmory.koplugin/grimmory/synchronize.luagrimmory.koplugin/main.luaspec/grimmory/synchronize_spec.lua
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
grimmory-tools/grimmory-docs(manual)
📜 Review details
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-06-26T20:12:43.855Z
Learnt from: imnotjames
Repo: grimmory-tools/grimmory.koplugin PR: 161
File: grimmory.koplugin/grimmory/doc_metadata.lua:0-0
Timestamp: 2026-06-26T20:12:43.855Z
Learning: In the `grimmory.koplugin` sync architecture, the book-specific sync flow runs on a different thread from the UI code. When reviewing “duplicate UI reload” or “duplicate refresh” concerns, don’t assume calls in `grimmory.koplugin/main.lua` (e.g., `refreshUI()`) share the same execution path as progress updates like `DocMetadata:setProgress()` in `grimmory.koplugin/grimmory/doc_metadata.lua`. Confirm whether the code paths can run concurrently/independently before flagging them as duplicates.
Applied to files:
grimmory.koplugin/grimmory/repository.luagrimmory.koplugin/main.luagrimmory.koplugin/grimmory/grimmory_api.luagrimmory.koplugin/grimmory/synchronize.lua
🔇 Additional comments (7)
grimmory.koplugin/grimmory/repository.lua (1)
813-858: LGTM!Also applies to: 861-900
grimmory.koplugin/main.lua (1)
479-480: LGTM!Also applies to: 553-556, 642-652
grimmory.koplugin/grimmory/grimmory_api.lua (1)
549-593: LGTM!grimmory.koplugin/grimmory/synchronize.lua (3)
9-21: LGTM!
102-115: LGTM!Also applies to: 136-151
640-667: 🚀 Performance & ScalabilityNo change needed.
isBookcaches document props by path, so repeated comparisons for the same local file do not re-parse the file on every remote book.spec/grimmory/synchronize_spec.lua (1)
108-192: LGTM!
| function GrimmorySynchronize:associateBook(book_path) | ||
| for book in self.api:getBooks() do | ||
| function GrimmorySynchronize:associateBook(book_path, remote_books) | ||
| local books = remote_books |
There was a problem hiding this comment.
This will run out of memory on some devices.
|
This looks AI generated. Is it? |
pushAllPendingBookMetadata called associateUnlinkedBooks directly, unlike the per-book pushBookMetadata call a few lines below which is wrapped in pcall. If associateUnlinkedBooks raised - e.g. GrimmoryAPI's getBooks() iterator calls error() on a mid-pagination network failure - the exception would abort the whole function before book_ids was even computed, silently skipping progress/session sync for every other already-linked book in that run. That's exactly the kind of one-book failure taking down unrelated books that this branch is otherwise trying to fix. (Flagged by CodeRabbit on PR grimmory-tools#201.)
…f loading it all into memory associateUnlinkedBooks fetched the entire remote catalog into a Lua table up front and reused it across every locally unlinked book, to avoid re-fetching per book. On a large library this means holding every Book (with its nested metadata/shelves tables) in memory at once, which is a real concern on the memory-constrained e-readers this plugin runs on. Invert the loop instead: walk the catalog one page at a time via the existing getBooks() iterator, and check each page against whichever local books are still unmatched. Matched books are removed from the remaining set as they're linked, so memory use stays bounded by however many local books are still unlinked (typically small) rather than the size of the whole remote catalog, and the scan can stop as soon as everything's matched instead of reading the rest of the catalog. associateBook (used separately by the single-file "Sync with Grimmory" action) no longer needs a pre-fetched catalog parameter now that associateUnlinkedBooks does its own streaming match, so it reverts to its original single-book form. (Flagged by a maintainer on PR grimmory-tools#201.)
Yeah, AI (Claude Code) helped write this, but it didn't just generate a diff from a description - it was directed through the actual investigation. The bug was diagnosed against a real self-hosted Grimmory instance and my own KOReader So: AI-assisted, not AI-generated. Happy to walk through any part of it in more detail if that helps the review. |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
grimmory.koplugin/grimmory/synchronize.lua (1)
517-550: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winReused local-match still isn't linked to
book.id— same gap flagged in the previous review.
findLocalBookMatchreturns an existing local file path but never persists the association;getBookDownloadPaththen returns that path immediately (line 546-549) without callingupsertBook. SinceassociateUnlinkedBooksonly links unlinked books that already have reading-session events, a book matched here but not yet read stays permanently unlinked in the repository even though the correct file was identified — defeating the "detect existing local books... to avoid duplicate downloads" goal for any book association bookkeeping that depends on the link (progress/session sync for that file going forward). The prior review already surfaced this exact issue with verified evidence; the code is unchanged in that respect.
grimmory.koplugin/grimmory/synchronize.lua#L517-L550: callself.repository:upsertBook(local_match, book.id)(logging on failure, mirroring the pattern used elsewhere) before returninglocal_matchfromgetBookDownloadPath.spec/grimmory/synchronize_spec.lua#L317-L333: add an assertion thatfake_repository.upsertBookwas called with the matched path andremote_book.id, so this regression is caught going forward.🐛 Proposed fix
local local_match = self:findLocalBookMatch(book) if local_match ~= nil then + local link_ok = self.repository:upsertBook(local_match, book.id) + if not link_ok then + logger:err("Failed to link matched local book:", local_match, "-", book.id) + end return local_match endWorth confirming the caller a few lines below (
downloadBook, not shown in this diff) doesn't already handle this separately:#!/bin/bash rg -n -B5 -A40 'getBookDownloadPath\s*\(' grimmory.koplugin/grimmory/synchronize.lua🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@grimmory.koplugin/grimmory/synchronize.lua` around lines 517 - 550, Update getBookDownloadPath to call self.repository:upsertBook(local_match, book.id) before returning a reused local match, logging failure consistently with the existing upsertBook handling. In spec/grimmory/synchronize_spec.lua lines 317-333, assert fake_repository.upsertBook receives the matched path and remote_book.id.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@grimmory.koplugin/grimmory/synchronize.lua`:
- Around line 517-550: Update getBookDownloadPath to call
self.repository:upsertBook(local_match, book.id) before returning a reused local
match, logging failure consistently with the existing upsertBook handling. In
spec/grimmory/synchronize_spec.lua lines 317-333, assert
fake_repository.upsertBook receives the matched path and remote_book.id.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 036ff076-3437-4e65-970b-87bd71674580
📒 Files selected for processing (2)
grimmory.koplugin/grimmory/synchronize.luaspec/grimmory/synchronize_spec.lua
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
grimmory-tools/grimmory-docs(manual)
📜 Review details
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-06-26T20:12:43.855Z
Learnt from: imnotjames
Repo: grimmory-tools/grimmory.koplugin PR: 161
File: grimmory.koplugin/grimmory/doc_metadata.lua:0-0
Timestamp: 2026-06-26T20:12:43.855Z
Learning: In the `grimmory.koplugin` sync architecture, the book-specific sync flow runs on a different thread from the UI code. When reviewing “duplicate UI reload” or “duplicate refresh” concerns, don’t assume calls in `grimmory.koplugin/main.lua` (e.g., `refreshUI()`) share the same execution path as progress updates like `DocMetadata:setProgress()` in `grimmory.koplugin/grimmory/doc_metadata.lua`. Confirm whether the code paths can run concurrently/independently before flagging them as duplicates.
Applied to files:
grimmory.koplugin/grimmory/synchronize.lua
🔇 Additional comments (2)
grimmory.koplugin/grimmory/synchronize.lua (1)
11-21: LGTM!Also applies to: 88-183, 258-289, 645-660, 663-712
spec/grimmory/synchronize_spec.lua (1)
109-293: LGTM!Also applies to: 295-316, 335-341
|
Any feedback on this PR? |
2 participants
Summary
Reading progress (page position) syncs fine, but reading session
stats (time spent, pages read per session) can silently never reach
Grimmory for books that weren't downloaded through this plugin - e.g.
books added via OPDS, copied in manually, or carried over from a
previous KOReader setup.
Root cause, in order:
grimmory_idby being downloaded throughthis plugin, or by the user manually running "Sync with Grimmory"
on that specific file. Nothing in the full/periodic sync path ever
tries to establish that link for books that already exist locally.
getBooksPendingSyncrequiresgrimmory_id IS NOT NULL, so anunlinked book's sessions are never even attempted during a full
sync - not an error, just silently skipped, forever.
manual per-file sync),
pushBookSessionshit asession.grimmory_id == nilbranch that logged an error andbreak-ed out of the loopentirely, discarding every other pending session for that book in
the same run.
by an exact expected filename inside the configured download
directory. A book present locally under any other name was
invisible to that check, so shelf sync would download a duplicate
copy - leaving the original (with all the real reading history)
orphaned with no
grimmory_id, and the fresh duplicate with none ofthat history.
GrimmoryAPI:recordSessionalso hardcodedbookType = "EPUB"forevery session regardless of the book's actual format.
Changes (4 commits)
fix: derive reading-session book type from the file extensionbookTypefrom the file extension instead of hardcoding"EPUB", falling back to the old default when it can't bedetermined.
fix: don't abort every pending session when a book isn't linked to Grimmorygrimmory_idis constant per book for a givenpushBookSessionscall, so a missing link is checked once up front and reported as a
single
session-unlinkedevent instead of failing (and breakingout on) every pending session individually. Real API failures
still break the loop so a run can retry.
feat: automatically link books with reading activity during full syncgetUnlinkedBooksWithEvents()+associateUnlinkedBooks(),which tries to match locally-known books with reading activity but
no
grimmory_idagainst the remote catalog (reusing the existingISBN/ASIN
doc_metadata:isBookmatching) before a full syncdecides what's pending. The remote catalog is fetched once per
sync and reused across every unlinked book, not once per book.
Surfaces
book-linked/session-unlinkedin the sync summarytoast so this is no longer silent.
fix: avoid re-downloading books that already exist locally under another namegetUnlinkedBooks()+findLocalBookMatch(): beforedownloading, checks already-known local files with no
grimmory_idagainst the remote catalog entry via the sameISBN/ASIN matching, and reuses the existing file instead of
downloading a duplicate when one matches.
This was found and reproduced against a real self-hosted Grimmory
instance + KOReader device: a book read entirely outside the plugin's
own download flow had 0 rows ever recorded via
book_sync_status/reading-sessions, and a shelf sync produced anexact duplicate of a book already present locally under a different
filename.
Test plan
busted spec/- 48/48 passing (39 pre-existing + 9 new, plusextended coverage on the 3 pre-existing behaviors touched)
luacheck . --no-self- 0 warnings across all filesstep, not just at the tip)
coverage so far -
synchronize.luahad no existing spec file,this PR adds one)
Summary by CodeRabbit