Requesting one format doesn't monitor the sibling, even with format sync on - #178
Open
JordanFromIT wants to merge 1 commit into
Open
JordanFromIT wants to merge 1 commit into
JordanFromIT wants to merge 1 commit into
Conversation
…ync on "Sync Monitored Across Formats" only reconciles when an EXISTING book's monitored flag changes, through one of four methods: SetBookMonitored, SetMonitored, UpdateBook, UpdateMany. A book that arrives already monitored never goes through any of them, because its Monitored flag is set once, at insert time, inside the same call that creates the row - and every book request does exactly that (POST /api/v1/book with monitored: true). So a request for the audiobook never monitors the ebook sibling, or vice versa, even on an author with sync fully turned on. Confirmed on a real library: Carl's Doomsday Scenario had its audiobook requested and owned, an unmonitored ebook sibling sitting on the same Goodreads work id, sync configured correctly on the author - and the ebook was never touched. `BulkSyncFormatMonitoringCommand` exists to reconcile this kind of gap after the fact, but nothing calls it automatically; nothing calls it on add either. Run the insert through the same reconciliation the four existing mutation paths already share: diff the new book's monitored state against an unmonitored baseline, so the sync logic sees exactly what it would see if the book had just been toggled on. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016egDLGPNYH9NDewUXf1Gkq
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
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
"Sync Monitored Across Formats" is supposed to mean: monitor the ebook, and the audiobook gets monitored too (or vice versa). It only half works. Toggling an existing book's monitored flag reconciles the sibling correctly — that path has been solid since it shipped. But a book that arrives already monitored, which is exactly what every book request does (
POST /api/v1/bookwithmonitored: true), never goes through any of the four methods the reconciliation is wired into (SetBookMonitored,SetMonitored,UpdateBook,UpdateMany). Its monitored flag is set once, inline, at insert time — so the sibling is left behind, silently, even on an author with sync fully configured.Technical detail
BookService.AddBookinserts the row, setsnewBook.Monitoredfrom the caller, publishesBookAddedEvent, and returns. None of that touchesGetSyncUpdatesForMutations, the private helper the four existing mutation methods all call. Confirmed on a real library: an audiobook was requested and owned; its ebook sibling sat on the same Goodreads work id, unmonitored, with the author'sSyncMonitoredAcrossFormatscorrectlytrue— and nothing ever reconciled it, because nothing ever called one of the four methods for that row after its creation.BulkSyncFormatMonitoringCommandexists specifically to reconcile this kind of drift after the fact, but nothing in the app invokes it automatically — not on add, not on refresh, not on a schedule. It's dead code unless someone calls it manually viaPOST /api/v1/command.What this does. Runs the insert through the exact same reconciliation the four existing methods already share: build a clone of the new book with both format-monitored flags forced to
falseas the "prior" state, and hand(newBook, priorState)toGetSyncUpdatesForMutations. From the reconciliation's point of view this looks identical to "a book just became monitored" — which is the true story — so it produces the same sibling update it always would have.Deliberate choices worth flagging for review
InsertManywas never one of the four sync-triggering methods, and this PR doesn't add it there. I chose the narrower fix because it's the path every book request goes through (AddBookService.AddBook→BookService.AddBook), so it's the one that matters for a site that lets people request books; the bulk-import path only matters in the rarer sequence where a refresh discovers a new sibling for a work whose other format is already monitored outside of a request. That's the case I found the original bug in, historically — I fixed the data for it by hand — but I don't have evidence it recurs going forward the same way, since a freshly-requested author's other books are typically all-unmonitored on both sides when refresh later fills them in (nothing to sync against yet). Happy to extend this intoInsertManytoo if you'd rather close the whole surface in one PR.BookService.cs, plus tests. Nothing else changes.Blast radius.
AddBookis the single insertion point for both the manual "add a book" UI flow and every programmatic book request. The new call is a no-op whenever the book isn't monitored, or the author has sync off, or there's no sibling row to reconcile — which is true for the overwhelming majority of inserts (most new books are catalog-only, unmonitored, or the sibling doesn't exist yet).Database Migration
NO.
How was this tested?
Four tests, written first and watched failing for the right reason:
(All four failed initially — the other three assert
AddBookshould not touch the sibling under specific conditions — sync off, the new book itself unmonitored, no sibling at all — and those only started passing once the surrounding infrastructure in the test double was wired up correctly; once fixed, all three passed unmodified before the actual code change, confirming they're real regression guards and not accidentally-passing assertions.)package.jsonparsesversion_guard.pysync / monotonic / commit-hygienedotnet build src/Chaptarr.NoTests.sln -c Releasedotnet testNo frontend files are touched, so the yarn steps aren't applicable.
On a real instance: deployed to my own instance, cherry-picked on top of several other local branches (see that branch's own commit history — one of them adds
Author.AudiobookMonitorExisting/EbookMonitorExistingand an extra> 0gate on top of upstream's simpler sync-enable rule; the cherry-picked copy of this PR's test carries one additional commit setting those fields so the test proves the fix against what that build actually gates on, without changing the PR itself). The specific historical gap (an owned, monitored audiobook with an unmonitored ebook sibling on the same work id, sync on) was corrected by hand via the existingPUT /book/monitorendpoint — proving that endpoint's reconciliation already worked correctly; this PR's job is only to make a fresh insert behave the same way going forward.What I did not test. I did not add coverage for the bulk-import (
InsertMany) path — see the deliberate-choices note above. I also did not exercise this through the full HTTPPOST /api/v1/bookrequest path in a test (only throughBookService.AddBookdirectly); the controller layer between them does no monitoring-relevant work, so I don't expect a difference, but I haven't proven it in a test.Screenshots (UI changes only)
None. No UI changes.
🤖 Generated with Claude Code
https://claude.ai/code/session_016egDLGPNYH9NDewUXf1Gkq