feat(categories): rename + delete a category (bookshelf-0v44o.2) #1091
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "bd-bookshelf-0v44o.2"
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
Test plan
go build ./internal/categories/...— clean compilego test ./internal/categories/...— all unit tests passnpm test— all 3766 JS tests pass (including 31 category_manage tests)go build -tags e2e ./e2e/...— browser e2e compilesOrderedcontainer, BeforeAll seeds DB, clicks kebab, asserts rename+delete modals open and mutations reload pageCloses bead bookshelf-0v44o.2 on merge.
Ordered journey covering the full kebab→modal→submit flow for both the rename (PATCH /categories/{id}) and delete (DELETE /categories/{id}) operations. Uploads three screenshots to PR (rename modal, rename submitted, delete modal). Justified as browser e2e (not Vitest/jsdom or API e2e): - Requires real Chromium: Stimulus controller builds DOM dynamically; fetch calls must reach the live HTTP server. - Multi-step user journey: kebab click → menu → modal → submit → reload. - DOM assertion after controller-driven page reload is not testable in Vitest (no real HTTP) or e2e/api/ (no DOM interaction). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>Category management screenshot (rename-modal)
Category rename/delete — kebab menu and modal on /categories page
Category management screenshot (rename-submitted)
Category rename/delete — kebab menu and modal on /categories page
Category management screenshot (delete-modal)
Category rename/delete — kebab menu and modal on /categories page
Security Review — PR #1091 (
feat(categories): rename + delete)Authorization
PATCH /categories/{id} and DELETE /categories/{id} are both wrapped by
manageRequired(eh.Wrap(...))inroutes.go(lines 19–20 of that file). Inwire.go,manageRequiredresolves tod.LibraryManageMetadataConfigRequired, which is wired inapp.goasusers.PermissionRequired(check: row.PermissionManageMetadataConfig, logger). The gate:denyRequest).forbidRequest.forbidRequest.Result: fails closed. No authorization gap.
SQL injection
RenameCategoryusesUPDATE category SET name = ? WHERE id = ?with bound parameters.DeleteCategoryusesDELETE FROM category WHERE id = ?.categoryExistsusesSELECT 1 FROM category WHERE id = ? LIMIT 1. No string concatenation in SQL. Clean.XSS
Server side: both
data-category-name="{{.Name}}"andaria-label="Actions for {{.Name}}"incategories_index.htmlare in double-quoted attribute context —html/templateescapes",<,>,&automatically. Clean.Client side (
category_manage_controller.js):input.value = this._activeName— sets the.valueproperty, notinnerHTML. Clean._el("p", null, "Delete " + name + "?…")—_elsetsel.textContent, notinnerHTML. Even a name containing<script>would be inert. Clean._showError(el, msg)setsel.textContent. Clean.The
_activeIDvalue flows from the server-rendereddata-category-id="{{.ID}}"(a Goint64→ decimal literal) into the fetch URL.strconv.ParseInton the server rejects anything non-numeric with a 400. Clean.CSRF
Both
_patchRenameand_deleteCategorysend"X-CSRF-Token": this._csrfToken(). The_csrfToken()implementation readsmeta[name="csrf-token"], which is the same pattern used in the siblingauthor_manage_controller.js(lines 519–521 of that file). The global CSRF middleware enforces the header on unsafe methods server-side. Clean.Architecture boundary
internal/categoriesimports: stdlib,github.com/go-sql-driver/mysql, andinternal/middleware. No workflow engine import. Clean.Secrets / PII
No tokens, keys, or PII appear in log messages or error strings. Category names in error wraps (
"rename category %d: %w") are not sensitive. Clean.Findings
[MINOR] internal/categories/manage_handler.go:409 — no maximum length cap on the rename name field
req.Nameis validated for non-empty but not for maximum length. The DB column isvarchar(255), so a name longer than 255 bytes reaches MySQL and returns a "Data too long for column" error that surfaces as an opaque 500 rather than a 400. No security impact, but the error signal is poor. Fix: addif len(req.Name) > 255 { return fmt.Errorf("name too long: %w", middleware.ErrValidation) }after the empty check.REVIEW VERDICT: 0 blocker, 0 major, 1 minor
Code Review: bookshelf-0v44o.2
Phase 0: DEMO Verification
CI is green (confirmed via commit status API). PR is mergeable. No DEMO block is present — this is a UI/mutation feature reviewed via diff + CI as the behavioral source of truth per project convention.
Phase 1: Spec Compliance
All stated requirements are present:
PATCH /categories/{id}rename handler — returns 409 on name conflict, 404 not-found, 204 successDELETE /categories/{id}delete handler — cascades via FK, 404 not-found, 204 successd.LibraryManageMetadataConfigRequiredon both routes (internal/categories/routes.go:19-20)./internal/categories/...added toUNIT_PKGSincheck-coverage.sh(inclusion, not exclusion — correct)Phase 2: Code Quality
Authz — Both mutation routes are correctly gated:
mux.Handle("PATCH /categories/{id}", manageRequired(eh.Wrap(rename)))andmux.Handle("DELETE /categories/{id}", manageRequired(eh.Wrap(delete)))(internal/categories/routes.go:19-20). Wire passesd.LibraryManageMetadataConfigRequired— the same field used by the author/series pattern.Multi-user — Category browse remains library-scoped (via
getUserLibraryIDsinListHandler). Mutations are admin-gated; categories are global metadata, so admin-only global mutation is the correct and intended model, matching authors/series.SQL injection — All queries use parameterized placeholders. No string concatenation in SQL paths.
Duplicate key detection —
isDuplicateCategoryKeyuseserrors.As(err, &mysqlErr) && mysqlErr.Number == 1062— typed error check, not string matching (internal/categories/manage_service.go:784-786). Correct.Rename semantics — Rename to current name:
RowsAffected == 0→ existence check → category exists → returns nil (no-op success). Tested inmanage_service_test.go"Category exists (same name, no-op)" context. Rename nonexistent:RowsAffected == 0→ existence check → empty rows →ErrNotFound→ 404. Correct.DELETE cascade — Relies on MySQL FK
ON DELETE CASCADEonbook_metadata_category_mapping. The singleDELETE FROM category WHERE id = ?statement is atomic; cascade is automatic. No orphaned rows. 404 on missing viaRowsAffected == 0.CSRF — Both
_patchRenameand_deleteCategorysendX-CSRF-Tokenread from<meta name="csrf-token">. This matches the established project pattern (base.html line 6 renders the meta tag;author_manage_controller.jsreads it identically).XSS — Category names flow through: server →
{{.Name}}(html/template-escaped) →data-category-nameattribute →btn.dataset.categoryName→el.textContent. All safe. Delete modal body usesel.textContent = textvia_el(), neverinnerHTML.CSP — No inline
style=attributes in the template. The JS controller usesmenu.style.setProperty("--menu-top", ...)— CSS custom property via JS, which is the project-approved alternative per the no-inline-style rule.Tests — All test files declare
package categories_test(black-box). Curried-DI pattern throughout. Handler tests fold nil-error into value assertion (Expect(resp.StatusCode, reqErr).To(...)). Service tests cover all branches: success, duplicate key, generic error, zero-rows-affected (both exists and not-exists sub-cases), RowsAffected error, and existence-check query failure. Vitest covers the full controller lifecycle — connect, disconnect, open menu, rename modal (all paths), delete modal (all paths), CSRF header, keyboard/backdrop dismissal.Browser e2e — Ordered journey,
BeforeAllwith single shared browser/server,BeforeEachresets page timeout (go-rod gotcha respected). Justification comment explains why real Chromium is required.[MINOR]
static/js/test/category_manage_controller.test.js:1484-1490— The DISCONNECT test setsctrl._activeID,ctrl._activeName, and callsctrl._openRenameModal()directly (white-box JS). JavaScript has no language-level module privacy, so this is workable and CI passes, but it couples the test to implementation naming. Matches the likely author controller test pattern. Does not block.REVIEW VERDICT: 0 blocker, 0 major, 1 minor
UI Review — PR #1091 (bookshelf-0v44o.2) — category rename/delete modals + kebab
Screenshots reviewed: rename-modal.png, delete-modal.png, rename-submitted.png.
What I see in the rendered screenshots
Rename modal — canonical chrome present: header ("Rename Category") with X close button top-right, padded body with "New name" label + pre-filled text input, right-aligned footer with Cancel (ghost) + Save (accent blue). Width is properly constrained (~480px). Input has focus ring. Clean, on-brand.
Delete modal — canonical chrome: header ("Delete Category") with X close button, body with clear confirmation text naming the category, right-aligned footer with Cancel (ghost) + Delete (red outlined,
.btn-dangeraffordance). Correct danger styling. Clean.Post-rename page — modal closes cleanly, card updates to new name, no artifacts.
Canonical-component reuse
The modals correctly reuse:
.modal-overlay+.modal-dialog+.modal-dialog--author-manage(canonical modal shell + sizing variant).modal-header/.modal-title/.modal-close-btn/.modal-body/.modal-footer(canonical sections).btn,.btn-ghost,.btn-danger(canonical buttons).library-kebab-menu__item,.library-kebab-menu__item--danger(canonical kebab item buttons).author-manage-menu(canonical shared popup menu — correctly shared, not re-invented).metadata-field-label,.metadata-field-input(canonical form field classes).am-error-msg(shared error message style from the author-manage section)No bespoke parallel class system was invented. No inline
style=attributes (JS usesstyle.setProperty()for CSS custom properties, which is CSP-safe).Findings
[MINOR] static/css/main.css (missing rule) / templates/pages/categories_index.html:19 —
category-card-kebabclass applied in template has no CSS definitionThe kebab button has
class="btn btn-icon category-card-kebab"but.category-card-kebabhas zero CSS rules (grep confirms). The button renders correctly only because.btn+.btn-iconsupply all visual styling. The undefined class is dead weight — either add minimal positioning/sizing rules if future CSS needs a hook, or remove it and rely on[data-action]for JS targeting. No visual defect visible; purely a convention issue.[MINOR] static/js/controllers/category_manage_controller.js:69,78,138 —
author-manage-*/am-*namespace reused for category UI without aliasingThe controller builds menu
<ul>with classauthor-manage-menu,<li>elements withauthor-manage-menu-item, and error paragraphs witham-error-msg. These are defined in the author-manage CSS section and the<li>classauthor-manage-menu-itemhas no CSS definition at all (same gap exists inauthor_manage_controller.js— this is a shared latent nit). Functionally harmless but makes the category controller appear to be "borrowing" author-manage identity. Ideal fix is to either rename these to domain-neutral names (manage-menu,manage-menu-item,manage-error-msg) or document them as shared in main.css.REVIEW VERDICT: 0 blocker, 0 major, 2 minor
643f4c3dec8cd3eb5d2e