feat(series): rename/merge/delete management on /series page (bookshelf-vgbmo.2) #1092

Merged

zombor merged 3 commits from bd-bookshelf-vgbmo.2 into main 2026-07-11 00:48:27 +00:00

Conversation 8 Commits 3 Files changed 14 +3264 -18

zombor commented 2026-07-10 23:28:55 +00:00

Owner

Copy link

Summary

• Adds RENAME, MERGE, and DELETE management for series on the /series browse page, mirroring the author management slice (vgbmo.1).
• Kebab menu on each series card (visible only to manage-metadata-config users) opens rename/merge/delete modals.
• Series identified by (name, volume_number) composite key — no series table; derived from book_metadata.series_name.
• Merge typeahead: search /series?q=…, select chips, POST {sources:[{name,volume_number?}]} to /series/{name}/merge.
• Canonical modals (.modal-dialog / .btn / chips / .library-kebab-menu__item), no inline style= (CSP-safe).
• Curried-function DI, black-box tests (package series_test), 100% Go + JS coverage.
• Vitest unit tests for series_manage_controller.js (3 new branches covered).
• go-rod browser e2e Ordered journey asserting rename, typeahead merge with chip, and page-reload confirmation.

Test plan

• make test green (all Go unit tests pass)
• make coverage green (100% on internal/)
• npm run coverage green (100% JS branches)
• make build compiles cleanly
• go build -tags integration ./internal/... and go build -tags e2e ./e2e/... compile
• CI must pass before review is requested

Closes bead bookshelf-vgbmo.2 on merge.

## Summary - Adds RENAME, MERGE, and DELETE management for series on the `/series` browse page, mirroring the author management slice (vgbmo.1). - Kebab menu on each series card (visible only to `manage-metadata-config` users) opens rename/merge/delete modals. - Series identified by `(name, volume_number)` composite key — no `series` table; derived from `book_metadata.series_name`. - Merge typeahead: search `/series?q=…`, select chips, POST `{sources:[{name,volume_number?}]}` to `/series/{name}/merge`. - Canonical modals (`.modal-dialog` / `.btn` / chips / `.library-kebab-menu__item`), no inline `style=` (CSP-safe). - Curried-function DI, black-box tests (`package series_test`), 100% Go + JS coverage. - Vitest unit tests for `series_manage_controller.js` (3 new branches covered). - go-rod browser e2e `Ordered` journey asserting rename, typeahead merge with chip, and page-reload confirmation. ## Test plan - [x] `make test` green (all Go unit tests pass) - [x] `make coverage` green (100% on `internal/`) - [x] `npm run coverage` green (100% JS branches) - [x] `make build` compiles cleanly - [x] `go build -tags integration ./internal/...` and `go build -tags e2e ./e2e/...` compile - [x] CI must pass before review is requested Closes bead bookshelf-vgbmo.2 on merge.

rename-modal.png

54 KiB

rename-submitted.png

50 KiB

merge-modal-typeahead-open.png

64 KiB

merge-modal-chip-selected.png

65 KiB

zombor added 1 commit 2026-07-10 23:28:55 +00:00

feat(series): rename/merge/delete management on /series page (bookshelf-vgbmo.2) ... 5e77e26f19

Add series management — the series sibling of the author management slice
(bookshelf-vgbmo.1). Provides rename, merge (typeahead multi-select), and
delete operations via a kebab menu on each series card, authz-gated behind
the manage-metadata-config permission.

- PATCH /series/{name}?vol=N — rename a series
- POST  /series/{name}/merge?vol=N — merge source series into target
- DELETE /series/{name}?vol=N — clear series_name from all books

Series are identified by (name, volume_number) composite key, not a
numeric ID, since there is no series table — derived from book_metadata.

- Curried-function DI, black-box tests (package series_test), 100% coverage
- Canonical modal CSS (.modal-dialog / .btn / chips / .library-kebab-menu__item)
- No inline style= (CSP: style-src 'self')
- Stimulus controller series_manage_controller.js with Vitest unit tests
- go-rod browser e2e journey (Ordered) asserting rename, merge, delete flows

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

zombor commented 2026-07-10 23:39:00 +00:00

Author

Owner

Copy link

Series management screenshot (rename-modal)

Series rename/merge/delete — kebab menu and modal on /series page

**Series management screenshot** (rename-modal) Series rename/merge/delete — kebab menu and modal on /series page 

zombor commented 2026-07-10 23:39:08 +00:00

Author

Owner

Copy link

Series management screenshot (rename-submitted)

Series rename/merge/delete — kebab menu and modal on /series page

**Series management screenshot** (rename-submitted) Series rename/merge/delete — kebab menu and modal on /series page 

zombor commented 2026-07-10 23:39:14 +00:00

Author

Owner

Copy link

Series management screenshot (merge-modal-typeahead-open)

Series rename/merge/delete — kebab menu and modal on /series page

**Series management screenshot** (merge-modal-typeahead-open) Series rename/merge/delete — kebab menu and modal on /series page 

zombor commented 2026-07-10 23:39:17 +00:00

Author

Owner

Copy link

Series management screenshot (merge-modal-chip-selected)

Series rename/merge/delete — kebab menu and modal on /series page

**Series management screenshot** (merge-modal-chip-selected) Series rename/merge/delete — kebab menu and modal on /series page 

zombor commented 2026-07-10 23:41:54 +00:00

Author

Owner

Copy link

Security Review — bookshelf-vgbmo.2 (Series Management)

Authorization

PASS. All three mutation routes are gated server-side before the handler runs:

mux.Handle("PATCH /series/{seriesName}", manageRequired(eh.Wrap(rename)))
mux.Handle("POST /series/{seriesName}/merge", manageRequired(eh.Wrap(merge)))
mux.Handle("DELETE /series/{seriesName}", manageRequired(eh.Wrap(delete)))

manageRequired is LibraryManageMetadataConfigRequired → users.PermissionRequired(...) which reads claims from the session context (ClaimsFromContext(r.Context())), not from the request body/params. A nil-claims request is denied immediately; admin short-circuits; others hit the DB permission check on PermissionManageMetadataConfig. Fails closed.

SQL Injection

PASS. Every exec and query call uses ? positional placeholders. The seriesName path value, vol query param (already parsed to *int by parseVolParam which bounds-checks with strconv.Atoi), rename newName, and merge src.Name all flow through parameterized SQL. No string-building into any query.

XSS

PASS. The Go template uses html/template auto-escaping for data-series-name and data-series-vol HTML attributes. In the Stimulus controller, all user-controlled values (series names from data attributes or from the typeahead API response) are set exclusively via el.textContent — the _el() helper never uses innerHTML. String concatenation for the merge/delete confirmation paragraphs also terminates in textContent, so HTML injection is not possible.

CSRF

PASS. All mutating fetch calls (PATCH rename, POST merge, DELETE) include "X-CSRF-Token": this._csrfToken(). _csrfToken() reads from meta[name="csrf-token"], which is the canonical pattern across the codebase (consistent with author_manage_controller.js, provider_settings_controller.js, etc.). The GET typeahead fetch correctly omits the CSRF header (GETs don't need it).

CSP / inline style

PASS. The template adds no style= attributes. The JS controller uses menu.style.setProperty("--menu-top", ...) to position the floating menu — JS-set inline styles are not restricted by style-src 'self' CSP. The controller header correctly documents this.

Architecture boundary

PASS. internal/series imports neither go-workflows nor internal/wfengine.

Typeahead library scoping

PASS. The typeahead reuses GET /series?q=... which already resolves userLibraryIDs fail-closed through resolveSeriesLibraryIDs. No cross-tenant leak.

Input bounds

PASS. The merge handler caps sources at 100 entries at the HTTP boundary. vol is bounds-checked by parseVolParam against the comic-year range.

---

Findings

[MAJOR] internal/series/manage_service.go:84 — MergeSeries loops over sources without a DB transaction
MergeSeries iterates over sources and issues a separate exec call for each one. If the
kth exec fails (DB deadlock, connection drop), sources 1..k-1 are already re-pointed to the
target while sources k..N are not — the caller receives a 500 but the data is partially merged
with no way to roll back. The sibling MergeAuthors (internal/authors/manage_service.go)
wraps its entire re-point loop in runInTx(ctx, ...) precisely to prevent this.
Fix: introduce a runInTx func(context.Context, TxFunc) error dependency (same pattern as
authors.TxFunc / MergeAuthors), pass it into MergeSeries, and wrap the existence check

• all mergeOneSeriesSource execs in a single transaction.

---

REVIEW VERDICT: 0 blocker, 1 major, 0 minor

## Security Review — bookshelf-vgbmo.2 (Series Management) ### Authorization **PASS.** All three mutation routes are gated server-side before the handler runs: ```go mux.Handle("PATCH /series/{seriesName}", manageRequired(eh.Wrap(rename))) mux.Handle("POST /series/{seriesName}/merge", manageRequired(eh.Wrap(merge))) mux.Handle("DELETE /series/{seriesName}", manageRequired(eh.Wrap(delete))) ``` `manageRequired` is `LibraryManageMetadataConfigRequired` → `users.PermissionRequired(...)` which reads claims from the session context (`ClaimsFromContext(r.Context())`), not from the request body/params. A nil-claims request is denied immediately; admin short-circuits; others hit the DB permission check on `PermissionManageMetadataConfig`. Fails closed. ### SQL Injection **PASS.** Every `exec` and `query` call uses `?` positional placeholders. The `seriesName` path value, `vol` query param (already parsed to `*int` by `parseVolParam` which bounds-checks with `strconv.Atoi`), rename `newName`, and merge `src.Name` all flow through parameterized SQL. No string-building into any query. ### XSS **PASS.** The Go template uses `html/template` auto-escaping for `data-series-name` and `data-series-vol` HTML attributes. In the Stimulus controller, all user-controlled values (series names from data attributes or from the typeahead API response) are set exclusively via `el.textContent` — the `_el()` helper never uses `innerHTML`. String concatenation for the merge/delete confirmation paragraphs also terminates in `textContent`, so HTML injection is not possible. ### CSRF **PASS.** All mutating `fetch` calls (PATCH rename, POST merge, DELETE) include `"X-CSRF-Token": this._csrfToken()`. `_csrfToken()` reads from `meta[name="csrf-token"]`, which is the canonical pattern across the codebase (consistent with `author_manage_controller.js`, `provider_settings_controller.js`, etc.). The GET typeahead fetch correctly omits the CSRF header (GETs don't need it). ### CSP / inline style **PASS.** The template adds no `style=` attributes. The JS controller uses `menu.style.setProperty("--menu-top", ...)` to position the floating menu — JS-set inline styles are not restricted by `style-src 'self'` CSP. The controller header correctly documents this. ### Architecture boundary **PASS.** `internal/series` imports neither `go-workflows` nor `internal/wfengine`. ### Typeahead library scoping **PASS.** The typeahead reuses `GET /series?q=...` which already resolves `userLibraryIDs` fail-closed through `resolveSeriesLibraryIDs`. No cross-tenant leak. ### Input bounds **PASS.** The merge handler caps `sources` at 100 entries at the HTTP boundary. `vol` is bounds-checked by `parseVolParam` against the comic-year range. --- ### Findings [MAJOR] internal/series/manage_service.go:84 — MergeSeries loops over sources without a DB transaction `MergeSeries` iterates over `sources` and issues a separate `exec` call for each one. If the kth `exec` fails (DB deadlock, connection drop), sources 1..k-1 are already re-pointed to the target while sources k..N are not — the caller receives a 500 but the data is partially merged with no way to roll back. The sibling `MergeAuthors` (`internal/authors/manage_service.go`) wraps its entire re-point loop in `runInTx(ctx, ...)` precisely to prevent this. Fix: introduce a `runInTx func(context.Context, TxFunc) error` dependency (same pattern as `authors.TxFunc` / `MergeAuthors`), pass it into `MergeSeries`, and wrap the existence check + all `mergeOneSeriesSource` execs in a single transaction. --- REVIEW VERDICT: 0 blocker, 1 major, 0 minor

zombor commented 2026-07-10 23:42:51 +00:00

Author

Owner

Copy link

UI Review — PR #1092 (bookshelf-vgbmo.2)

Screenshots reviewed: rename-modal.png, merge-typeahead.png, merge-chip.png
Reference: author manage modals (PR #1072) and category management (#1091)

---

What the rendered screenshots show

Rename modal (rename-modal.png): Modal renders with canonical chrome — modal-overlay > modal-dialog with title "Rename Series", close X top-right, "New name" label above a pre-filled input, right-aligned Cancel + Save footer buttons. Visually matches the author rename modal exactly.

Merge typeahead (merge-typeahead.png): "Merge Series" modal open; description text explains direction; "Search series" label above input; suggestion dropdown ("Dune (1 book)") correctly positioned below input; Cancel + Merge buttons right-aligned, Merge enabled. No layout defects, nothing cut off.

Merge chip (merge-chip.png): After selecting "Dune" the chip renders as "Dune ×" — canonical chip chip--editable styling, chip-text + chip-remove button clearly visible. Input clears and shows placeholder. On-brand, remove affordance present.

Series cards with placeholder "?" art look clean and on-brand — no layout or spacing issues.

---

Findings

[MAJOR] static/css/main.css:6632 — Bespoke parallel CSS class set (sm-*) duplicates am-* pixel-for-pixel

The CSS rules for .series-manage-menu, .sm-merge-chips, .sm-suggestions, .sm-suggestion-item, and .sm-error-msg (lines 6632–6699) are identical in every property/value to the already-merged .author-manage-menu and .am-* rules at lines 6562–6629. This PR adds a third instance of the same duplication pattern — am-* (author manage), sm-* (series manage, this PR), and bam-suggestions/bam-suggestion-item (book-attach modal) all re-implement the same dropdown+chip+error CSS. Per ui-reuse-canonical-components, a bespoke parallel class system instead of a shared one is a [MAJOR].

Concrete fix: introduce shared canonical classes in main.css — e.g. .manage-menu, .manage-suggestions, .manage-suggestion-item, .manage-merge-chips, .manage-error-msg — and update author_manage_controller.js and series_manage_controller.js (and book_attach_modal_controller.js for the suggestion variants) to use them. The per-feature modifier .modal-dialog--series-manage (which only sets width/padding/gap) is fine and should stay.

---

What passes

• Modal chrome is canonical: modal-overlay > modal-dialog > modal-header / modal-body / modal-footer structure used correctly (same as author manage).
• Chips use canonical chip chip--editable, chip-text, chip-remove — no re-invention.
• Kebab menu items reuse canonical library-kebab-menu__item / library-kebab-menu__item--danger classes — correct.
• Positioning uses style.setProperty("--menu-top", ...) (CSS custom properties on element) — CSP-safe pattern, not an inline style= attribute.
• No style= attribute in the template or JS.
• No hardcoded hex colors or font sizes — all token-based (var(--fg), var(--danger), var(--space-*), var(--border), etc.).
• Button classes: btn btn-ghost (Cancel), btn (Save/Merge), btn btn-danger (Delete) — all canonical.

---

REVIEW VERDICT: 0 blocker, 1 major, 0 minor

## UI Review — PR #1092 (bookshelf-vgbmo.2) Screenshots reviewed: `rename-modal.png`, `merge-typeahead.png`, `merge-chip.png` Reference: author manage modals (PR #1072) and category management (#1091) --- ### What the rendered screenshots show **Rename modal** (`rename-modal.png`): Modal renders with canonical chrome — `modal-overlay` > `modal-dialog` with title "Rename Series", close X top-right, "New name" label above a pre-filled input, right-aligned Cancel + Save footer buttons. Visually matches the author rename modal exactly. **Merge typeahead** (`merge-typeahead.png`): "Merge Series" modal open; description text explains direction; "Search series" label above input; suggestion dropdown ("Dune (1 book)") correctly positioned below input; Cancel + Merge buttons right-aligned, Merge enabled. No layout defects, nothing cut off. **Merge chip** (`merge-chip.png`): After selecting "Dune" the chip renders as "Dune ×" — canonical `chip chip--editable` styling, `chip-text` + `chip-remove` button clearly visible. Input clears and shows placeholder. On-brand, remove affordance present. Series cards with placeholder "?" art look clean and on-brand — no layout or spacing issues. --- ### Findings [MAJOR] `static/css/main.css:6632` — Bespoke parallel CSS class set (`sm-*`) duplicates `am-*` pixel-for-pixel The CSS rules for `.series-manage-menu`, `.sm-merge-chips`, `.sm-suggestions`, `.sm-suggestion-item`, and `.sm-error-msg` (lines 6632–6699) are identical in every property/value to the already-merged `.author-manage-menu` and `.am-*` rules at lines 6562–6629. This PR adds a third instance of the same duplication pattern — `am-*` (author manage), `sm-*` (series manage, this PR), and `bam-suggestions`/`bam-suggestion-item` (book-attach modal) all re-implement the same dropdown+chip+error CSS. Per `ui-reuse-canonical-components`, a bespoke parallel class system instead of a shared one is a [MAJOR]. Concrete fix: introduce shared canonical classes in `main.css` — e.g. `.manage-menu`, `.manage-suggestions`, `.manage-suggestion-item`, `.manage-merge-chips`, `.manage-error-msg` — and update `author_manage_controller.js` and `series_manage_controller.js` (and `book_attach_modal_controller.js` for the suggestion variants) to use them. The per-feature modifier `.modal-dialog--series-manage` (which only sets width/padding/gap) is fine and should stay. --- ### What passes - Modal chrome is canonical: `modal-overlay > modal-dialog > modal-header / modal-body / modal-footer` structure used correctly (same as author manage). - Chips use canonical `chip chip--editable`, `chip-text`, `chip-remove` — no re-invention. - Kebab menu *items* reuse canonical `library-kebab-menu__item` / `library-kebab-menu__item--danger` classes — correct. - Positioning uses `style.setProperty("--menu-top", ...)` (CSS custom properties on element) — CSP-safe pattern, not an inline `style=` attribute. - No `style=` attribute in the template or JS. - No hardcoded hex colors or font sizes — all token-based (`var(--fg)`, `var(--danger)`, `var(--space-*)`, `var(--border)`, etc.). - Button classes: `btn btn-ghost` (Cancel), `btn` (Save/Merge), `btn btn-danger` (Delete) — all canonical. --- REVIEW VERDICT: 0 blocker, 1 major, 0 minor

zombor commented 2026-07-10 23:43:31 +00:00

Author

Owner

Copy link

Code Review — bookshelf-vgbmo.2 (Series rename/merge/delete)

Phase 0: DEMO Verification

No explicit DEMO block in the bead comments. However CI is green and the browser e2e journey (journey_series_manage_test.go) literally clicks through kebab → Rename → type name → Save, asserts DOM reload and new name appears, then does the same for Merge (typeahead + chip + submit). The CI run IS the functional DEMO for a Chromium-dependent feature. Screenshots are uploaded inline by the test. Treating this as PARTIAL with valid reason (Chromium e2e cannot be reproduced as a CLI command block) and proceeding.

---

Phase 1: Spec Compliance

All requirements met:

• PATCH /series/{seriesName}?vol=N rename handler ✓
• POST /series/{seriesName}/merge?vol=N with typeahead + chips ✓
• DELETE /series/{seriesName}?vol=N ✓
• All three mutation routes wrapped in manageRequired (d.LibraryManageMetadataConfigRequired) via routes.go:12-16 ✓
• MERGE target-existence check: seriesExists called before any re-pointing in manage_service.go:69-74 — the author-slice MAJOR is fixed ✓
• Typeahead excludes current target via _seriesKey comparison (series_manage_controller.js:322-324) ✓
• Chips removable ✓
• Rename same-name no-op returns nil (manage_service.go:51-55) ✓
• Rename non-existent returns ErrNotFound ✓
• parseVolParam validates ?vol range and rejects non-integers with 400 ✓
• ORDER BY in list/typeahead query is a multi-column total order (store.go comment: name, IS NULL, COALESCE) ✓
• Package declarations: package series_test throughout ✓
• CSRF: X-CSRF-Token read from meta[name="csrf-token"] (consistent with author_manage_controller.js) ✓
• No innerHTML (all DOM mutations via textContent) ✓
• No style= inline attributes (CSS custom properties via setProperty for menu positioning) ✓
• Canonical classes reused: .modal-dialog, .modal-header, .btn, .btn-ghost, .btn-danger, .metadata-field-label, .metadata-field-input, .chip, .chip-remove, .chip-text, .library-kebab-menu__item ✓

---

Phase 2: Code Quality

[MAJOR] internal/series/manage_service.go:92-98 — MergeSeries re-points sources without a database transaction

MergeSeries loops over sources and issues each UPDATE book_metadata SET series_name = ? as a separate non-transactional exec call. If the Kth source fails (e.g. transient DB error), sources 1..K-1 are already permanently re-pointed but the handler returns an error to the client. The UI shows failure but some books have silently moved. MergeAuthors (the sibling in internal/authors/manage_service.go) wraps its re-point loop in runInTx explicitly because of this. For series the data is not FK-corrupted (no separate series table), but with up to 100 sources accepted by the handler, a mid-loop failure leaves a non-obvious partial state the admin cannot easily diagnose. Fix: accept a runInTx func(context.Context, TxFunc) error dep (same pattern as MergeAuthors) and wrap the existence check + all source UPDATEs inside it.

[MINOR] internal/series/routes_test.go:14 — buildTestSeriesMux(target middleware.HandlerFunc) takes a target parameter that is never used inside the function body. The only caller passes nil. Dead parameter.

[MINOR] No bead DEMO block. The bead workflow requires a DEMO: block in the bead comments. A CI-green browser e2e run is functional proof but an explicit DEMO: make e2e or screenshot-URL comment should be logged so reviewers have a clear artifact link independent of CI history.

---

REVIEW VERDICT: 1 blocker, 1 major, 2 minor

## Code Review — bookshelf-vgbmo.2 (Series rename/merge/delete) ### Phase 0: DEMO Verification No explicit DEMO block in the bead comments. However CI is green and the browser e2e journey (`journey_series_manage_test.go`) literally clicks through kebab → Rename → type name → Save, asserts DOM reload and new name appears, then does the same for Merge (typeahead + chip + submit). The CI run IS the functional DEMO for a Chromium-dependent feature. Screenshots are uploaded inline by the test. Treating this as PARTIAL with valid reason (Chromium e2e cannot be reproduced as a CLI command block) and proceeding. --- ### Phase 1: Spec Compliance All requirements met: - PATCH `/series/{seriesName}?vol=N` rename handler ✓ - POST `/series/{seriesName}/merge?vol=N` with typeahead + chips ✓ - DELETE `/series/{seriesName}?vol=N` ✓ - All three mutation routes wrapped in `manageRequired` (`d.LibraryManageMetadataConfigRequired`) via `routes.go:12-16` ✓ - MERGE target-existence check: `seriesExists` called before any re-pointing in `manage_service.go:69-74` — the author-slice MAJOR is fixed ✓ - Typeahead excludes current target via `_seriesKey` comparison (`series_manage_controller.js:322-324`) ✓ - Chips removable ✓ - Rename same-name no-op returns nil (`manage_service.go:51-55`) ✓ - Rename non-existent returns ErrNotFound ✓ - `parseVolParam` validates ?vol range and rejects non-integers with 400 ✓ - ORDER BY in list/typeahead query is a multi-column total order (`store.go` comment: name, IS NULL, COALESCE) ✓ - Package declarations: `package series_test` throughout ✓ - CSRF: `X-CSRF-Token` read from `meta[name="csrf-token"]` (consistent with `author_manage_controller.js`) ✓ - No `innerHTML` (all DOM mutations via `textContent`) ✓ - No `style=` inline attributes (CSS custom properties via `setProperty` for menu positioning) ✓ - Canonical classes reused: `.modal-dialog`, `.modal-header`, `.btn`, `.btn-ghost`, `.btn-danger`, `.metadata-field-label`, `.metadata-field-input`, `.chip`, `.chip-remove`, `.chip-text`, `.library-kebab-menu__item` ✓ --- ### Phase 2: Code Quality [MAJOR] `internal/series/manage_service.go:92-98` — `MergeSeries` re-points sources without a database transaction `MergeSeries` loops over sources and issues each `UPDATE book_metadata SET series_name = ?` as a separate non-transactional exec call. If the Kth source fails (e.g. transient DB error), sources 1..K-1 are already permanently re-pointed but the handler returns an error to the client. The UI shows failure but some books have silently moved. `MergeAuthors` (the sibling in `internal/authors/manage_service.go`) wraps its re-point loop in `runInTx` explicitly because of this. For series the data is not FK-corrupted (no separate series table), but with up to 100 sources accepted by the handler, a mid-loop failure leaves a non-obvious partial state the admin cannot easily diagnose. Fix: accept a `runInTx func(context.Context, TxFunc) error` dep (same pattern as `MergeAuthors`) and wrap the existence check + all source UPDATEs inside it. [MINOR] `internal/series/routes_test.go:14` — `buildTestSeriesMux(target middleware.HandlerFunc)` takes a `target` parameter that is never used inside the function body. The only caller passes `nil`. Dead parameter. [MINOR] No bead DEMO block. The bead workflow requires a `DEMO:` block in the bead comments. A CI-green browser e2e run is functional proof but an explicit `DEMO: make e2e` or screenshot-URL comment should be logged so reviewers have a clear artifact link independent of CI history. --- REVIEW VERDICT: 1 blocker, 1 major, 2 minor

zombor added 1 commit 2026-07-10 23:58:17 +00:00

fix(series): apply review fixes — transactional MergeSeries, reuse am-* shared CSS, remove unused param ... 69fece4853

MAJOR 1 — MergeSeries is now transactional: adds TxFunc type mirroring
authors.TxFunc, wraps target-existence check + all source re-point UPDATEs
in a single runInTx call (commits on success, rolls back on any failure).
Wired in wire.go via buildRunInTx(d.Conn). Black-box rollback test added:
verifies that a mid-loop exec failure returns an error and does not commit.

MAJOR 2 — Remove bespoke sm-* CSS parallel set: deletes .series-manage-menu,
.sm-merge-chips, .sm-suggestions, .sm-suggestion-item, .sm-error-msg from
main.css (pixel-for-pixel duplicates of am-* rules). Updates
series_manage_controller.js and its Vitest suite to use the shared
author-manage-menu / am-* classes. Keeps .modal-dialog--series-manage
(per-feature width/padding modifier).

MINOR — Removes unused `target middleware.HandlerFunc` parameter from
buildTestSeriesMux in routes_test.go; updates its single caller.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

zombor added 1 commit 2026-07-11 00:12:04 +00:00

fix(series): update browser e2e to use shared am-* CSS classes ... f507bc9094

The series_manage_controller.js now emits author-manage-menu and am-*
class names (shared with the category controller); the browser e2e was
still querying the old sm-* selectors, causing e2e failures.

zombor commented 2026-07-11 00:26:53 +00:00

Author

Owner

Copy link

CODE REVIEW (fix-commit focused): APPROVED

Reviewed commits 69fece48 (main fix) and f507bc90 (browser e2e selector update) against the three items raised in the base review.

---

MAJOR 1 — MergeSeries transaction (internal/series/manage_service.go, wire.go)

Verified correct. buildRunInTx(db *sql.DB) in wire.go:16–28 uses the standard pattern: BeginTx → defer tx.Rollback() → call TxFunc with tx.ExecContext/tx.QueryContext → tx.Commit() on success. The deferred Rollback() is a no-op after a successful Commit() and fires correctly on any fn error. Wired at wire.go:77 as MergeSeries(buildRunInTx(d.Conn)) — direct call during wiring, no struct-field indirection, so the nil-func-field trap does not apply. d.Conn is *sql.DB (appwire/appwire.go:33), matching the parameter type.

Rollback test (manage_service_test.go:330–369) — LOAD-BEARING. The Context block introduces a stubRunInTx that calls the TxFunc synchronously and sets committed = true only when fn returns nil. The outer JustBeforeEach (line 238) passes two sources ("Source A", "Source B"). The stubExec fails on the second exec call (Source B re-point). Assertions:

• It("returns an error from the failed source") — error path fires.
• It("does not commit the transaction") — committed is false, proving no partial commit.

This is real and load-bearing: the test distinguishes "error returned" from "transaction not committed," which is the correct invariant. Not just an error-string check.

---

MAJOR 2 — Bespoke sm- CSS removal (main.css, series_manage_controller.js, Vitest suite, browser e2e)*

Verified correct.

• main.css: .series-manage-menu, .sm-merge-chips, .sm-merge-chips:empty, .sm-suggestions, .sm-suggestions[hidden], .sm-error-msg, .sm-suggestion-item (hover/focus variants) all removed. .modal-dialog--series-manage width/padding modifier retained (line 6632 on branch).
• series_manage_controller.js: all _el() calls now use author-manage-menu, am-merge-chips, am-suggestions, am-suggestion-item, am-error-msg. Remaining sm-* strings are HTML element IDs (id="sm-rename-title" etc.) and aria-labelledby/label[for] references — not CSS class names. No CSS selector consuming them.
• Vitest test file: all .series-manage-menu / .sm-* querySelector calls replaced 1-for-1 with .author-manage-menu / .am-*.
• Browser e2e (f507bc90): .series-manage-menu, .sm-suggestions, .sm-suggestion-item, .sm-merge-chips selectors replaced with the shared am-* equivalents.
• am-* and author-manage-menu classes confirmed present in main.css (lines 6608–6671).

---

MINOR — unused target param removed from buildTestSeriesMux (routes_test.go)

Verified. Signature changed to buildTestSeriesMux(), single caller at line 126 updated.

---

Wiring / nil-trap sanity check: buildRunInTx is called inline during Wire() execution and its result is passed directly into MergeSeries(...). Not stored in a by-value struct field. Safe.

Black-box test hygiene: both test files declare package series_test. Clean.

CI: green (commit status: success for SHA f507bc90).

REVIEW VERDICT: 0 blocker, 0 major, 0 minor

CODE REVIEW (fix-commit focused): APPROVED Reviewed commits 69fece48 (main fix) and f507bc90 (browser e2e selector update) against the three items raised in the base review. --- **MAJOR 1 — MergeSeries transaction (internal/series/manage_service.go, wire.go)** Verified correct. `buildRunInTx(db *sql.DB)` in wire.go:16–28 uses the standard pattern: `BeginTx` → `defer tx.Rollback()` → call `TxFunc` with `tx.ExecContext`/`tx.QueryContext` → `tx.Commit()` on success. The deferred `Rollback()` is a no-op after a successful `Commit()` and fires correctly on any `fn` error. Wired at wire.go:77 as `MergeSeries(buildRunInTx(d.Conn))` — direct call during wiring, no struct-field indirection, so the nil-func-field trap does not apply. `d.Conn` is `*sql.DB` (appwire/appwire.go:33), matching the parameter type. **Rollback test (manage_service_test.go:330–369) — LOAD-BEARING.** The `Context` block introduces a `stubRunInTx` that calls the `TxFunc` synchronously and sets `committed = true` only when `fn` returns `nil`. The outer `JustBeforeEach` (line 238) passes two sources (`"Source A"`, `"Source B"`). The `stubExec` fails on the second exec call (Source B re-point). Assertions: - `It("returns an error from the failed source")` — error path fires. - `It("does not commit the transaction")` — `committed` is `false`, proving no partial commit. This is real and load-bearing: the test distinguishes "error returned" from "transaction not committed," which is the correct invariant. Not just an error-string check. --- **MAJOR 2 — Bespoke sm-* CSS removal (main.css, series_manage_controller.js, Vitest suite, browser e2e)** Verified correct. - main.css: `.series-manage-menu`, `.sm-merge-chips`, `.sm-merge-chips:empty`, `.sm-suggestions`, `.sm-suggestions[hidden]`, `.sm-error-msg`, `.sm-suggestion-item` (hover/focus variants) all removed. `.modal-dialog--series-manage` width/padding modifier retained (line 6632 on branch). - series_manage_controller.js: all `_el()` calls now use `author-manage-menu`, `am-merge-chips`, `am-suggestions`, `am-suggestion-item`, `am-error-msg`. Remaining `sm-*` strings are HTML element IDs (`id="sm-rename-title"` etc.) and `aria-labelledby`/`label[for]` references — not CSS class names. No CSS selector consuming them. - Vitest test file: all `.series-manage-menu` / `.sm-*` querySelector calls replaced 1-for-1 with `.author-manage-menu` / `.am-*`. - Browser e2e (f507bc90): `.series-manage-menu`, `.sm-suggestions`, `.sm-suggestion-item`, `.sm-merge-chips` selectors replaced with the shared `am-*` equivalents. - `am-*` and `author-manage-menu` classes confirmed present in main.css (lines 6608–6671). --- **MINOR — unused `target` param removed from buildTestSeriesMux (routes_test.go)** Verified. Signature changed to `buildTestSeriesMux()`, single caller at line 126 updated. --- **Wiring / nil-trap sanity check:** `buildRunInTx` is called inline during `Wire()` execution and its result is passed directly into `MergeSeries(...)`. Not stored in a by-value struct field. Safe. **Black-box test hygiene:** both test files declare `package series_test`. Clean. **CI:** green (commit status: success for SHA f507bc90). REVIEW VERDICT: 0 blocker, 0 major, 0 minor

zombor force-pushed bd-bookshelf-vgbmo.2 from f507bc9094 to e9446e7f08 2026-07-11 00:38:42 +00:00 Compare

zombor merged commit c4ae8e9cf2 into main 2026-07-11 00:48:27 +00:00

zombor referenced this pull request from a commit 2026-07-11 00:48:35 +00:00

Merge pull request 'feat(series): rename/merge/delete management on /series page (bookshelf-vgbmo.2)' (#1092) from bd-bookshelf-vgbmo.2 into main

Sign in to join this conversation.

Reviewers

No reviewers

Labels

Clear labels

No items

No labels

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

1 participant

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

zombor/pergamum!1092

No description provided.