feat(categories): /categories browse page with counts + nav item (bookshelf-0v44o.1) #1074
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "bd-bookshelf-0v44o.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
internal/categoriespackage: cursor-paginatedGET /categoriesendpoint (HTML + JSON, content-negotiated) with?q=search,?sort=name|count,?dir=asc|desc; book counts via a single GROUP BY query — no N+1userLibraryIDsJOIN mirrors theauthorspattern (empty IDs → empty result)categories_index.html+categories_index_controls.htmlusing canonical CSS classes andsort_dropdown_controllerwire.goexcluded per gate conventionTest plan
make testgreen (all 45 categories specs + middleware nav tests pass)make coveragegreen — zero uncovered statement blocksmake buildgreen/categoriespage posted belowCloses bead bookshelf-0v44o.1 on merge.
UI Screenshot —
/categoriesbrowse pagePopulated with 8 categories (seeded data), sidebar showing category count badge:
Security Review — PR #1072 (bookshelf-vgbmo.1): admin author rename/merge/delete
Independent adversarial security review of the diff (
origin/main...origin/bd-bookshelf-vgbmo.1). Read-only; did not run tests.Authorization (HARD RULE) — PASS. All three destructive routes are gated by the admin permission in
internal/authors/routes.go:26-28:PATCH /authors/{id},POST /authors/{id}/merge, andDELETE /authors/{id}are each wrapped withmanageRequired(...), bound tod.LibraryManageMetadataConfigRequiredinwire.go:71-73, which isusers.PermissionRequired(... PermissionManageMetadataConfig)(app.go:356). That middleware fails closed (403 on no-claims / no-perms-row / check-false) and admin-short-circuits. Defense in depth is correct: the template gates the kebab + controller behind{{if .CurrentUser.CanManageMetadata}}(authors_index.html) AND the server enforces independently. No route left ungated. SQL is fully parameterized; author IDs areParseInt-validated; theauthor_idFK (fk_book_metadata_author_mapping_author ... ON DELETE CASCADE, migration 0001:286) means a merge remap to a nonexistent target rolls back rather than orphaning rows. XSS is clean — the JS controller builds all modal content viatextContent/_el(...), neverinnerHTML, and the template auto-escapes.Name.Two findings below.
[BLOCKER] static/js/controllers/author_manage_controller.js:172,266,341 — mutation fetches omit the X-CSRF-Token header; every rename/merge/delete returns 403
All three state-changing fetches (
_patchRenameL172,_postMergeL266,_deleteAuthorL341) send onlyContent-Type: application/json(or no headers for DELETE) and NOX-CSRF-Tokenheader. The global CSRF middleware (internal/middleware/csrf.go:91-99) validates every unsafe method (POST/PATCH/DELETE) against the double-submit token;/authors/*is not in the exempt list. With no header and a JSON (not form) body,csrfTokenFromRequestreturns "" ->tokenEqualfails ->403 "CSRF token mismatch". Result: the feature is 100% non-functional in production — no author can be renamed, merged, or deleted. This escapes CI because the browser e2e (journey_author_manage_test.go) only opens the rename modal and asserts the input is pre-filled; it never submits a mutation. Fix: read thebookshelf_csrfcookie and add"X-CSRF-Token": <token>to all three fetches — mirror the existing_csrfToken()helper used by every other mutation controller (e.g.column_picker_controller.js:48,bookdrop_bulk_edit_controller.js:132). Add an e2e/Vitest assertion that a mutation actually returns 204 (not just that the modal opens) so this cannot regress.[MAJOR] internal/authors/manage_service.go:216 (primaryBookIDs) + RefreshSortAuthorNameBatch — unbounded query + single-statement IN() can exceed MySQL's placeholder limit on a large author
primaryBookIDsrunsSELECT book_id ... WHERE author_id = ? AND sort_order = 0with no LIMIT, then the collected slice is passed straight toRefreshSortAuthorNameBatch, which expands to one?placeholder per book in a singleUPDATE ... WHERE bm.book_id IN (?,?,...)statement (internal/db/sqlc/metadata.sql.go:437-449). At the project's stated target scale (hundreds of thousands of books/library), a catch-all author like "Unknown"/"Various" can map to >65,535 books; the batch UPDATE then fails with "Prepared statement contains too many placeholders", and merge/delete/rename of that author breaks. It also runs synchronously inline in the request handler (conventions push expensive per-book fan-out to background/batched work). AdditionallyMergeHandlercapssource_idsat "not empty" but has no upper bound (onlyMaxByteslimits it indirectly). Fix: chunk the book-ID set into fixed-size batches for the sort refresh, or cap/paginateprimaryBookIDs; and add an explicit upper bound onsource_idslength in the handler.REVIEW VERDICT: 1 blocker, 1 major, 0 minor
UI Review — /categories browse page (bookshelf-0v44o.1)
Screenshot reviewed:
/categoriespage — header + Search+Sort toolbar, vertical list of 5 categories each with an inline book count, "Showing 5" footer, Categories nav item active with badge "8" between Authors and Notebook.[MINOR] templates/pages/categories_index.html:7-16 — bespoke layout class names with no CSS backing
Five new CSS class names are introduced (
.category-grid,.category-card,.category-card-link,.category-card-name,.category-card-count) butstatic/css/main.csshas zero CSS rules for any of them (confirmed via diff — no CSS changes in this PR). The page renders entirely via browser defaults: block<article>elements stacked vertically, inline spans flowing together. The class names declare intent for a grid/card layout (mirroring the.author-grid/.author-cardnaming convention, which DO have CSS rules in main.css at lines 6457–6526) but deliver none. Fix: either add CSS rules for the category list layout, or rename to reflect the actual plain-list structure (e.g..category-list/.category-item) so intent matches reality.[MINOR] templates/pages/categories_index.html:12 — book count has no visual treatment vs design system badges
<span class="category-card-count">{{.BookCount}}</span>renders inline right after the category name span with no CSS, producing "Adventure 2", "Classic 2" etc. — count glued to name with only whitespace separation. Every other count treatment in the design system uses a badge:.nav-countin the sidebar,.author-card-count-badgeon the author photo overlay,.series-card-count-badgeon series cover overlays. The count is functional and correct, but inconsistent with the visual language. Fix: add a.category-card-countCSS rule that visually distinguishes the count (e.g.color: var(--fg-muted); font-size: 0.75rem;), or separate name and count with a·delimiter, matching the scale used for similar text-list items elsewhere.Passing checks:
style=attributes — CSP clean..btn.btn-smfor Go/Clear buttons and mirrorsauthors_index_controls.htmlstructure exactly..pagination,.pagination-showing,.btn.btn-load-more,.empty-state— all canonical..nav-countbadge showing "8" — consistent with all other nav items.sort-dropdownStimulus controller reused, not re-invented.REVIEW VERDICT: 0 blocker, 0 major, 2 minor
Security Review — PR #1074 (bookshelf-0v44o.1)
/categoriesbrowse pageRead-only adversarial review of
git diff origin/main...origin/bd-bookshelf-0v44o.1, per.claude/rules/review-standard.md.[MAJOR] internal/db/queries/nav_counts.sql:24 (+ internal/db/sqlc/nav_counts.sql.go CountCategories, internal/middleware/nav.go:325-337, internal/app/app.go:719) — nav Categories badge count is NOT library-scoped
CountCategoriesisSELECT COUNT(*) FROM categorywith no user/library filter, wired viamiddleware.WrapInt64(q.CountCategories, …)(the unscoped wrapper — same as CountAuthors) and called ascounts.CountCategories(ctx)with nouserLibraryIDs. The list page IS scoped (buildCategoryLibraryJoin joins through book +WHERE b.library_id IN (…)), so the badge counts categories across ALL libraries while the page only shows those reachable through the user's accessible libraries. This is exactly the observed screenshot discrepancy (nav badge 8 vs page 5): the badge leaks the existence/number of categories in libraries the user cannot access — a per-user-scoping HARD RULE violation (cross-library data leak). Fix: mirrorseries.CountDistinctSeries— giveCountCategoriesa[]int64userLibraryIDs param that scopes viaJOIN book_metadata_category_mapping m → JOIN book b WHERE b.library_id IN (…)(fail-closed on non-nil empty), wire it withmiddleware.WrapInt64WithLibraries+ resolveGetUserLibraryIDsin the nav goroutine (as the series count already does at nav.go:288-300).[MAJOR] internal/categories/service.go (buildCategoryLibraryJoin) — list AND count query omit
b.deleted = 0buildCategoryLibraryJoinemitsJOIN book b ON b.id = m.book_id WHERE b.library_id IN (…)with noAND b.deleted = 0. Every other book-joined query filters soft-deleted rows (internal/books/store.go:606WHERE b.deleted = 0; series list/count likewise). As written, a category reachable only through soft-deleted books is still listed, and its per-categoryCOUNT(m.book_id)includes deleted books — so the counts shown to the user are wrong and categories that should be gone still appear. Fix: addAND b.deleted = 0to the book join in both the list query and the (to-be-scoped) count query.[MINOR] internal/categories/service.go (buildListCategoriesQuery,
qbranch) — LIKE metacharacters in?q=not escapedSearch builds
c.name LIKE ?with argp.Query+"%". Parameterization prevents SQL injection (good), but%and_in the term act as LIKE wildcards, soq=%matches everything andq=a_cmatchesabc. The books search escapes this (LIKE CONCAT(?, '%') ESCAPE '\\'+escapeLikeMetachars). Not a security hole (LIMIT-bounded, suffix-anchored so no leading-wildcard table scan), but inconsistent/surprising search semantics. Fix: escape% _ \in the term with anESCAPEclause as books does.[MINOR] internal/categories/service.go (buildCategoryCursorPredicate, name-DESC branch) — cursor id tiebreaker direction mismatch under name DESC
For
sort=name&dir=descthe ORDER BY isc.name DESC, c.id DESC, but the cursor predicate usesc.name < ? OR (c.name = ? AND c.id > ?)— the id tiebreaker walks ascending while the rows are ordered id-descending. This only misbehaves when two categories share a name (category names are effectively unique today, so low impact) but is a latent skip/repeat at the tie boundary. Fix: make the id comparator track the sort direction (c.id < ?for DESC), matching the ORDER BY.Verified clean:
?sort=/?dir=(?order=) validated against closed allowlists (validSortKeys/validSortDirs) before reaching the interpolated ORDER BY; onlyASC/DESCand enum columns can appear — no ORDER BY injection.?q=and cursor fields are bound as?params.?limit=is Atoi + clampLimit.c.idtiebreaker.{{.Name}},{{.SearchQuery}}render in html/template HTML/attribute contexts (auto-escaped);/books?category={{urlquery .Name}}uses urlquery. No inlinestyle=(CSP OK).LIMIT ?, clamped to [1,200]; single GROUP BY (no N+1).GetUserLibraryIDsnormalizes nil→[]int64{}, andListCategoriesreturns an empty page for non-nil-empty userLibraryIDs, so an authenticated user with no library access sees nothing.REVIEW VERDICT: 0 blocker, 2 major, 2 minor
UI Screenshot —
/categoriesbrowse pagePopulated with 8 categories (seeded data), sidebar showing category count badge:
UI Review — /categories card grid (bookshelf-0v44o.1)
Screenshot reviewed:
cat_grid.png— 5 category cards (Adventure/Classic/Cyberpunk/Fantasy/Science Fiction), nav badge = 5.What I see in the rendered screenshot
Five equal-height cards in a responsive grid. Each card: centered category name, count badge (dark pill with white number) below the name. Cards have border, rounded corners, and sit on the dark
var(--bg-card)surface. Clean, not sparse — the centered text + badge fills the card well at itsmin-height: 90px. Nav count badge reads 5, matching the grid. Overall a solid text-only card treatment.[MINOR]
static/css/main.css:6573,6575— hardcodedrgba(0, 0, 0, 0.55)/#fffin.category-card-countThe badge background and text are hardcoded instead of CSS-variable tokens. The existing
.author-card-count-badgedoes the same (rgba(0, 0, 0, 0.65)/#fff), so this is consistent with the established pattern rather than a new sin — but it will look wrong on any future light theme since a near-black overlay over a light card is invisible. Worth extracting to something likevar(--badge-bg)/var(--badge-fg)in a follow-up sweep that also fixes the author badge. Not blocking here since the pattern predates this PR.[MINOR]
static/css/main.css:6556—min-height: 90pxis a bare pixel value, not a space tokenAll other sizing in this PR uses
var(--space-*).min-height: 90pxis an outlier.var(--space-24)(if it exists at 96px) or the closest space token would be cleaner. Minor; does not affect visual correctness at this viewport.Checklist
.category-grid/.category-cardare purpose-built new classes for this text-only card type, not re-inventing.modal-dialog,.metadata-field, or any canonical shared component. Correct approach.var(--space-*)/var(--bg-card)/var(--border)/var(--accent)/var(--radius-sm)/var(--fg)correctly.transform: translateY(-2px)(lift) +border-color: var(--accent)matches the required lift-and-accent-border pattern.repeat(auto-fill, minmax(140px, 1fr))desktop,minmax(100px, 1fr))mobile — wraps sensibly at small widths.style=— PASS. No CSP violation.REVIEW VERDICT: 0 blocker, 0 major, 2 minor
CODE REVIEW: RE-VERIFICATION OF FIX COMMITS
Commit:
ef9bf1da— fix(categories): scope nav count, fix cursor DESC, deleted filter, LIKE escape, card grid CSSVerified all 5 fix items against the prior review findings:
1. Nav count scoping (internal/middleware/nav.go + internal/categories/service.go)
CountCategoriesin service.go correctly handles library scoping:AND b.library_id IN (...)resolveNavLibraryIDsin nav.go properly handles errors (suppresses badge, returns false)launchNavMetaCountsmirrors the series pattern exactly2. Cursor DESC tiebreaker (internal/categories/service.go:buildCategoryCursorPredicate)
For SortName:
(c.name > ? OR (c.name = ? AND c.id > ?))— correct, after cursor(c.name < ? OR (c.name = ? AND c.id < ?))— correct, before cursorFor SortCount:
(book_count > ? OR (book_count = ? AND c.id > ?))— correct, greater(book_count < ? OR (book_count = ? AND c.id < ?))— correct, lessTiebreaker
c.idcomparison direction matches sort direction in all cases. service_test.go covers name/count sorting in both ASC and DESC directions.3. b.deleted = 0 filter (internal/categories/service.go)
buildCountCategoriesQuery:JOIN book b ON b.id = m.book_id AND b.deleted = 0✓buildCategoryLibraryJoin:JOIN book b ON b.id = m.book_id AND b.deleted = 0✓4. LIKE escaping (internal/categories/service.go:buildListCategoriesQuery)
strings.NewReplacerto escape\,%,_ESCAPE '\\'clauseescaped+"%"5. Card grid CSS (static/css/main.css + templates/pages/categories_index.html)
CSS classes reuse canonical design tokens:
.category-grid:gap: var(--space-6),grid-template-columns: repeat(auto-fill, minmax(140px, 1fr)).category-card:var(--bg-card),var(--border),var(--accent),var(--radius-sm).category-card-count: Dark badge withborder-radius: var(--radius-sm)6. Test integrity
package categories_test(black-box, no white-box)7. Configuration
.golangci.yml✓REVIEW VERDICT: 0 blockers, 0 majors, 0 minors
All fixes verified as correct and complete. Ready for merge.