Library content kind: decouple comic detection from CBX format [shot:content-kind-screenshot] (bookshelf-qi21k.1) #1421
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "bd-bookshelf-qi21k.1"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Summary
Adds a per-library Content Kind (Auto/Comics/Magazines/Ebooks/Audiobooks)
so a magazine stored as
.cbrstops being treated as a comic, withouttouching the reader (Option A, confirmed 2026-08-09 on the parent epic
bookshelf-qi21k).
app_settingskeyed by library ID (content_kind.library.<id>)— the Grimmory
librarytable gets no new column. Absent key = Auto =today's extension-based behavior, zero regression.
IsComic = hasCBXFile AND kind ∈ {Comics, Auto}.internal/books.Showgets an optional variadic content-kind resolverparam so the ~30 existing
Show(...)test call sites keep compilingunchanged; wired for real in
books/wire.go.internal/books.WithLibraryContentKindwraps the single-book metadatagetter used by refetch / interactive candidate search / the step-through
editor, so a Magazines/Ebooks/Audiobooks library's CBX files skip comic
provider routing and comic-issue query building.
.cbrstill opens in the CBX page readerregardless of kind — nothing in the reader path was touched.
<select>on the library create/edit modal(
internal/library), admin-gated per existing library management(
ManipulateLibrary).0053sets the user's existing "Magazines"library to
kind=magazines(data-only, no schema change); every otherlibrary defaults to Auto.
docs/content/docs/your-library/libraries.mdgets aContent Kind section.
(
internal/app/build_extended_deps.go,build_llm_deps.go,build_enrich_deps.go) constructs its own separateGetMetadataForRefetchand is intentionally untouched here.Test plan
go test ./internal/settings/...— newlibrary_content_kind.gologic, 100% coverage
go test ./internal/books/...—WithLibraryContentKind+resolveShowIsComic(Show's HTML and JSON branches), 100% coverageon new code, all ~5000 pre-existing specs unaffected
go test ./internal/library/...— content-kind select rendering(real template), save-on-create/update, nil-safe fallback for tests
that don't wire the new dep, 100% coverage on new code
make coverage— full repo, zero uncovered statement blocksmake integration— migration0053applies cleanlymake lint(scoped) — 0 issuesmake e2e-policy-check— OKincluding the new steps (seed CBX book → Comic Info panel visible
under Auto → set library to Magazines via the modal → Comic Info
panel hidden, screenshot posted when running in CI)
Closes bead bookshelf-qi21k.1 on merge.
🤖 Generated with Claude Code
https://claude.ai/code/session_016tRKybTpfjQ4SxmNdVFLHi
Add a per-library Content Kind (Auto/Comics/Magazines/Ebooks/Audiobooks), stored in app_settings (no new column on the Grimmory `library` table). IsComic now resolves as hasCBXFile AND kind in {Auto, Comics}: a magazine stored as .cbr in a Magazines-kind library stops being routed to comic metadata providers and stops showing the Comic Info panel. The reader is unchanged — a .cbr still opens in the CBX page reader regardless of kind. - internal/settings/library_content_kind.go: kind storage + ResolveIsComic - internal/books: WithLibraryContentKind wraps the single-book metadata getter (refetch/candidates/step-edit); Show gets an optional variadic content-kind resolver (keeps all ~30 existing test call sites compiling) - internal/library: Content Kind <select> on the create/edit library modal, saved best-effort alongside the existing name/mode/formats fields - internal/db/migrations/0053: data-only backfill of the user's existing "Magazines" library to kind=magazines; every other library defaults to Auto (absent key), zero regression - e2e/browser: Journey 3 gains steps seeding a CBX book, confirming the Comic Info panel is visible under Auto and hidden after switching the library to Magazines, with a screenshot - docs/content/docs/your-library/libraries.md: end-user Content Kind section Bulk/LLM sweep wiring is intentionally untouched (separate GetMetadataForRefetch construction in internal/app/build_*.go) — tracked as a v2 follow-up per the qi21k epic. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016tRKybTpfjQ4SxmNdVFLHi[MAJOR] PR #1421 — no rendered screenshot to review
The PR description claims "Focused e2e run of Journey 3 (Manage Library) locally: 12/12 passed, ... screenshot posted when running in CI" — but this is explicitly conditional ("when running in CI"). Checking the Forgejo API:
issues/1421/commentsreturns[](0 comments) andissues/1421/assetsreturns[](0 attachments). No screenshot has actually been posted to this PR by the go-rod harness. Per.claude/rules/review-standard.mdUI review gate andpost-ui-screenshots-in-pr, a templates/CSS-touching PR with no rendered screenshot attached is not review-ready — I cannot judge the rendered Content Kind select (label/select styling, spacing vs canonical .modal-dialog/.form-control, grouping vs --space-* tokens, accessibility) without seeing the actual pixels.Fix: run the Journey 3 e2e in CI (or manually capture) and post the actual screenshot attachment(s) to this PR via the go-rod harness / Forgejo comment-attachment API, then re-request UI review. Do not rely on "will post when running in CI" as a substitute for a screenshot actually being present on the PR at review time.
REVIEW VERDICT: 0 blocker, 1 major, 0 minor
@ -0,0 +1,13 @@-- Backfill the content-kind override (bookshelf-qi21k) for the user's existingWe don't need this backfill migration.
@ -43,0 +50,4 @@{{end}}</select><p class="form-hint">Auto treats any comic-archive (.cbz/.cbr/.cb7/.cbt) file as a comic — today's default.Let's reword this to be more factual. What does "today's default" even mean?
[MAJOR] internal/library/handler.go:344 (
saveContentKindBestEffort) — invalidcontent_kindis silently swallowed instead of surfaced as 422, unlike the identicalErrUnknownProviderpattern it claims to mirrorupdateHandler/createHandlercallsaveContentKindBestEffort, which on any error (includingsettings.ErrUnknownContentKindfrom an out-of-range value) only logs aWarnand returns 204/303 as if the save succeeded (seeinternal/library/handler.go:735-757). The doc comment claims this "mirrors" the metadata-provider save, but the actual provider-save code path (internal/library/handler.go:310-330, pre-existing) does the opposite for its analogous sentinel: it special-caseserrors.Is(saveErr, settings.ErrUnknownProvider)and returns422 Unprocessable Entitywith the error re-rendered in the modal — it does NOT swallow it.content_kind_handler_test.go's only "save fails" spec asserts a generic error is swallowed (204) and never exercisesErrUnknownContentKindspecifically to confirm intended behavior.Impact: a JSON API client that submits an unrecognized
content_kind(typo, stale client, future kind not yet supported by an older server) gets a200/204 successwhile the field is silently NOT saved — masking a client bug and violating "validate at the boundary" (resilience-standard) and the same-file precedent it explicitly claims to follow.Fix: in
saveContentKindBestEffort(or its caller), special-caseerrors.Is(err, settings.ErrUnknownContentKind)the same wayErrUnknownProvideris handled — surface a 422 (JSON: return the error; HTML: re-render the modal with the error) — and keep only genuine infra failures (DB error) as best-effort/logged.REVIEW VERDICT: 0 blocker, 1 major, 0 minor
Library content kind: decouple comic detection from CBX format (bookshelf-qi21k.1)to Library content kind: decouple comic detection from CBX format [shot:content-kind-screenshot] (bookshelf-qi21k.1)108d2973e2f4e275341eSecurity review of PR #1421 (bd-bookshelf-qi21k.1) — per-library Content Kind.
Scope reviewed: internal/settings/library_content_kind.go, internal/library/handler.go + wire.go + routes.go, internal/books/library_content_kind.go + handler.go (Show), templates/pages/library_modal.html, migration 0053.
Findings:
No BLOCKER or MAJOR findings.
[MINOR] internal/library/handler.go:735 (saveContentKindBestEffort) — malformed content_kind from a JSON client is swallowed
A JSON caller (Content-Type: application/json) can submit an arbitrary
content_kindstring. SaveLibraryContentKind correctly rejects it via ErrUnknownContentKind, but saveContentKindBestEffort only logs a Warn and returns 204/201 as if the update succeeded — the caller has no way to know their content_kind was silently dropped. Not a security hole (the enum check in internal/settings/library_content_kind.go:isKnownContentKind still fully prevents an invalid value from ever being persisted or read back — no injection, no arbitrary state), but it's a confusing API contract. Consider surfacing a 422/validation error for JSON clients specifically (the HTML path can stay best-effort since it can only submit a valid value). [MINOR] internal/db/migrations/0053_backfill_magazines_library_content_kind.up.sql / .down.sql — name-based backfill match The backfill matches WHERE name = 'Magazines' (and the down migration matches the same). If an admin later creates or renames an unrelated library to "Magazines", a future re-run/rollback context could not accidentally affect it further (migrations run once), so this is very low risk — noted only for completeness, no action required. Positive notes (verified, not findings): Authz: PUT/PATCH /libraries/{id} (the only route that persists content_kind) is gated by g.ManipulateLibrary (permission_manipulate_library / admin) in internal/library/routes.go, in addition to the per-request checkLibraryOwnership scoping in updateHandler. A non-admin/non-authorized user cannot reach the write path. Enum validation: settings.SaveLibraryContentKind / isKnownContentKind reject any non-canonical value before persistence (internal/settings/library_content_kind.go); LoadLibraryContentKind also defaults any unrecognized stored value back to KindAuto defensively. Injection: the app_settings key is built with fmt.Sprintf("%s%d", ...) from an int64 libraryID (ContentKindKey) — no string concatenation of user input into the key, and all app_settings access goes through parameterized sqlc queries (UpsertAppSettingParams). Multi-user scoping: reads (loadContentKindForModal, GET edit-modal) and writes both go through checkLibraryOwnership against d.GetUserLibraryIDs before touching library {id}; a library outside the requester's access returns the standard not-found (no existence leak). books.WithLibraryContentKind / resolveShowIsComic resolve kind via the already-ownership-scoped book/library lookup — no direct pass-through of a request-supplied library id. Compat: migration 0053 adds no column to the Grimmory library table (data-only INSERT/DELETE against the existing app_settings key/value table), consistent with the project's schema-compat rule. CSP: the new Content Kind in templates/pages/library_modal.html uses only html/template auto-escaping, no inline style= attributes.cf2da51c2) that the branch (cut before #1411/bookshelf-h4oh2.2 merged) merges cleanly and does NOT revert the unrelated bookdrop auto-fetch cover+title confidence gate — the two-way diff vs main looked like a large unrelated deletion but the real merge preserves both features correctly.REVIEW VERDICT: 0 blocker, 0 major, 2 minor
Security review of PR #1421 (bd-bookshelf-qi21k.1) — per-library Content Kind.
Scope reviewed: internal/settings/library_content_kind.go, internal/library/handler.go + wire.go + routes.go, internal/books/library_content_kind.go + handler.go (Show), templates/pages/library_modal.html, migration 0053.
Findings:
No BLOCKER or MAJOR findings.
[MINOR] internal/library/handler.go:735 (saveContentKindBestEffort) — malformed content_kind from a JSON client is swallowed
A JSON caller (Content-Type: application/json) can submit an arbitrary content_kind string. SaveLibraryContentKind correctly rejects it via ErrUnknownContentKind, but saveContentKindBestEffort only logs a Warn and returns 204/201 as if the update succeeded — the caller has no way to know their content_kind was silently dropped. Not a security hole (the enum check in internal/settings/library_content_kind.go isKnownContentKind still fully prevents an invalid value from ever being persisted or read back — no injection, no arbitrary state), but it is a confusing API contract. Consider surfacing a 422/validation error for JSON clients specifically (the HTML select path can stay best-effort since it can only submit a valid value).
[MINOR] internal/db/migrations/0053_backfill_magazines_library_content_kind.up.sql / .down.sql — name-based backfill match
The backfill matches WHERE name = 'Magazines' (and the down migration matches the same). If an admin later creates or renames an unrelated library to Magazines, this is very low risk since migrations run once; noted only for completeness, no action required.
Positive notes (verified, not findings):
cf2da51c2) that the branch (cut before #1411/bookshelf-h4oh2.2 merged) merges cleanly and does NOT revert the unrelated bookdrop auto-fetch cover+title confidence gate — the two-way diff vs main looked like a large unrelated deletion but the real merge preserves both features correctly.REVIEW VERDICT: 0 blocker, 0 major, 2 minor
Library content kind — book Show page screenshot (magazines-kind-hides-comic-info-panel)
Library content kind — book Show page screenshot (magazines-kind-hides-comic-info-panel)
UI re-review of PR #1421 (branch bd-bookshelf-qi21k.1, head
51d96619c).Screenshot gate: RESOLVED. Both attachments (comments 17426, 17438) are valid 1280x800 PNGs of the book Show page for a magazines-kind library. Actually Read the pixels (not just the diff).
RENDERED — Comic Info panel hidden cleanly for magazines-kind library:
The Details tab renders Library/Read Status, File Path, action buttons (Read/Download/Fetch Metadata), then Similar Books/Files/Notes/Reading History sub-tabs with "No similar books found." No Comic Info heading, no empty
.comic-metadatacontainer, no orphaned label, no dead spacing gap. Source confirms why: templates/pages/books_show.html:802 gates the ENTIRE panel with{{if .ComicMeta}}, so when ComicMeta is nil the whole block (including the<h2>Comic Info</h2>heading) doesn't render at all — structurally absent, not just visually hidden. Clean.NEW CONTROL — Content Kind select in templates/pages/library_modal.html:43-59:
Neither screenshot shows the modal itself (both are the book Show page), so this was assessed via template source. The new field:
is byte-for-byte the same canonical pattern as the sibling "Organization Mode" select a few lines above it (
.metadata-field>.metadata-field-label+.metadata-field-inputselect), and the.form-hintparagraph matches the existing Allowed Formats/File Naming hint pattern elsewhere in the same file. No bespoke parallel class system.grep -n 'style="' templates/pages/library_modal.htmlreturns zero matches — no inline style, no CSP concern.No findings.
REVIEW VERDICT: 0 blocker, 0 major, 0 minor
51d96619cb1e16c066d0Library content kind — book Show page screenshot (magazines-kind-hides-comic-info-panel)