feat(authors): rename/merge/delete management [shot:author-manage] (bookshelf-vgbmo.1) #1072

Merged

zombor merged 16 commits from bd-bookshelf-vgbmo.1 into main 2026-07-10 14:52:10 +00:00

Conversation 58 Commits 16 Files changed 16 +3432 -1

zombor commented 2026-07-09 17:42:54 +00:00

Owner

Copy link

Summary

Adds admin-gated author management on the existing /authors browse page:

• Rename — PATCH /authors/{id}: updates the author name, returns 409 on UNIQUE conflict, refreshes sort_author_name on affected books
• Merge — POST /authors/{id}/merge: re-points source author mappings onto the target inside a DB transaction, deletes orphaned source authors, refreshes sort
• Delete — DELETE /authors/{id}: removes author (FK cascade clears mappings), refreshes sort

All three endpoints require permission_manage_metadata_config (admin or explicitly granted).

UI: per-card kebab (⋮) button opens a Stimulus context menu. Each action opens a canonical modal (modal-dialog, modal-overlay, btn, etc.). No inline style= (CSP compliant).

CanManageMetadata added to CurrentUser so the template can gate the kebab UI.

Test plan

• internal/authors/manage_service_test.go — curried-function tests for RenameAuthor, MergeAuthors, DeleteAuthor; all error branches covered
• internal/authors/manage_handler_test.go — HTTP handler tests: 204 success, 400 bad path/JSON, 409 conflict, 404 not found, 500 error
• static/js/test/author_manage_controller.test.js — 35 Vitest specs; menu, rename/merge/delete modals, fetch paths, Escape/Tab keyboard
• 100% Go coverage maintained (make coverage passes)
• 100% JS coverage maintained (npm run coverage passes)
• Browser e2e journey Journey: Author Management Modal posts a screenshot via [shot:author-manage]
• make e2e-policy-check passes (Ordered journey)

Closes bead bookshelf-vgbmo.1 on merge.

## Summary Adds admin-gated author management on the existing `/authors` browse page: - **Rename** — `PATCH /authors/{id}`: updates the author name, returns 409 on UNIQUE conflict, refreshes `sort_author_name` on affected books - **Merge** — `POST /authors/{id}/merge`: re-points source author mappings onto the target inside a DB transaction, deletes orphaned source authors, refreshes sort - **Delete** — `DELETE /authors/{id}`: removes author (FK cascade clears mappings), refreshes sort All three endpoints require `permission_manage_metadata_config` (admin or explicitly granted). **UI**: per-card kebab (⋮) button opens a Stimulus context menu. Each action opens a canonical modal (`modal-dialog`, `modal-overlay`, `btn`, etc.). No inline `style=` (CSP compliant). `CanManageMetadata` added to `CurrentUser` so the template can gate the kebab UI. ## Test plan - [x] `internal/authors/manage_service_test.go` — curried-function tests for RenameAuthor, MergeAuthors, DeleteAuthor; all error branches covered - [x] `internal/authors/manage_handler_test.go` — HTTP handler tests: 204 success, 400 bad path/JSON, 409 conflict, 404 not found, 500 error - [x] `static/js/test/author_manage_controller.test.js` — 35 Vitest specs; menu, rename/merge/delete modals, fetch paths, Escape/Tab keyboard - [x] 100% Go coverage maintained (`make coverage` passes) - [x] 100% JS coverage maintained (`npm run coverage` passes) - [x] Browser e2e journey `Journey: Author Management Modal` posts a screenshot via `[shot:author-manage]` - [x] `make e2e-policy-check` passes (Ordered journey) Closes bead bookshelf-vgbmo.1 on merge.

rename-modal.png

44 KiB

rename-submitted.png

41 KiB

kebab-menu.png

54 KiB

rename-modal.png

55 KiB

merge-modal.png

64 KiB

rename-modal.png

44 KiB

rename-modal.png

44 KiB

rename-modal.png

44 KiB

delete-modal.png

59 KiB

kebab-menu.png

54 KiB

rename-modal.png

55 KiB

merge-modal.png

64 KiB

delete-modal.png

59 KiB

rename-modal.png

49 KiB

rename-submitted.png

46 KiB

author-kebab-menu.png

58 KiB

author-kebab-menu.png

58 KiB

rename-modal.png

46 KiB

rename-submitted.png

42 KiB

rename-modal.png

46 KiB

rename-submitted.png

42 KiB

rename-modal.png

46 KiB

rename-submitted.png

42 KiB

rename-modal.png

46 KiB

kebab-menu.png

54 KiB

merge-modal.png

64 KiB

delete-modal.png

59 KiB

rename-submitted.png

42 KiB

merge-modal-typeahead-open.png

62 KiB

rename-modal.png

49 KiB

rename-submitted.png

46 KiB

rename-modal.png

49 KiB

rename-submitted.png

46 KiB

rename-modal.png

49 KiB

rename-submitted.png

46 KiB

merge-modal-typeahead-open.png

62 KiB

merge-modal-chip-selected.png

62 KiB

merge-modal-chip-selected.png

62 KiB

delete-modal.png

55 KiB

rename-modal.png

49 KiB

rename-submitted.png

46 KiB

merge-modal-typeahead-open.png

62 KiB

merge-modal-chip-selected.png

62 KiB

rename-modal.png

49 KiB

rename-submitted.png

46 KiB

merge-modal-typeahead-open.png

62 KiB

rename-modal.png

49 KiB

rename-submitted.png

46 KiB

merge-modal-typeahead-open.png

62 KiB

merge-modal-chip-selected.png

62 KiB

rename-modal.png

55 KiB

merge-modal-chip-selected.png

62 KiB

zombor added 2 commits 2026-07-09 17:42:54 +00:00

feat(authors): rename/merge/delete management (bookshelf-vgbmo.1) ... 64da90b0bd

Adds admin-gated author management on the /authors browse page:

- PATCH /authors/{id}           — rename (409 on UNIQUE conflict)
- POST  /authors/{id}/merge     — merge N source authors into target
- DELETE /authors/{id}          — delete author (FK cascade clears mappings)

All three endpoints are guarded by LibraryManageMetadataConfigRequired.
MergeAuthors runs inside a DB transaction (TxFunc injection pattern);
sort_author_name is refreshed via RefreshSortAuthorNameBatch after each mutation.

UI: per-card kebab button opens a context menu (Rename / Merge / Delete).
Each action opens a canonical modal (modal-dialog/modal-overlay classes).
No inline style= attributes (CSP: style-src 'self').

CanManageMetadata added to CurrentUser so templates can gate the UI.

100% Go coverage maintained (manage_service_test.go + manage_handler_test.go).
100% JS coverage maintained (author_manage_controller.test.js, 35 specs).

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

feat(authors): add browser e2e screenshot journey for author manage modal (bookshelf-vgbmo.1) ... f4434d5c52

Adds Journey: Author Management Modal in e2e/browser/ — seeds an author,
navigates to /authors as admin, clicks the kebab button, opens the rename
modal, and posts a screenshot to the PR via [shot:author-manage].

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

forgejo-actions commented 2026-07-09 17:45:20 +00:00

Copy link

Author management screenshot (rename-modal)

Author rename/merge/delete — kebab menu and modal on /authors page

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

zombor added 1 commit 2026-07-09 17:52:33 +00:00

fix(authors): add fallback to --bg-hover CSS custom property (bookshelf-vgbmo.1) ... 72fa23f452

CSS lint requires all var() usages to include a fallback value.

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

forgejo-actions commented 2026-07-09 17:54:33 +00:00

Copy link

Author management screenshot (rename-modal)

Author rename/merge/delete — kebab menu and modal on /authors page

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

zombor commented 2026-07-09 18:07:19 +00:00

Author

Owner

Copy link

UI Review — PR #1072 (bookshelf-vgbmo.1)

Screenshot reviewed: authors page with kebab button and open "Rename Author" modal (vgbmo_authors.png).

---

What I see in the rendered screenshot

• Authors page grid with a single "Jane Austen" card (photo square + "1" badge, name below).
• A small ⋮ (vertical ellipsis) button renders below the author name as its own right-aligned row, outside/below the card photo area.
• "Rename Author" modal is open: header with title + ✕ close button, body with "New name" label and pre-filled text input, footer with "Cancel" (ghost) and "Save" (solid) buttons. The modal renders centered on the page with an overlay.
• Sidebar: HOME, LIBRARIES, SHELVES, MAGIC SHELVES, INGEST section headers show a missing-glyph □ box next to each — this is a pre-existing headless-Chromium font-rendering artifact; this PR did NOT touch templates/layouts/base.html or any sidebar CSS/JS (confirmed by diff). Not a finding against this PR.

---

Findings

[MINOR] templates/pages/authors_index.html:22-29 — kebab placement feels visually disconnected from the card
The .author-card-actions div is a third row in the flex-direction:column card layout, sitting below the photo-wrap and author name. In the screenshot the ⋮ button appears as a standalone button clearly separated from the card photo, giving a bolted-on appearance. The sibling pattern for the book-count badge (.author-card-count-badge) uses position: absolute; top/right inside .author-card-photo-wrap. Putting the kebab in the same layer — position: absolute; top: var(--space-2); right: var(--space-2) on .author-card-actions with .author-card-photo-wrap { position: relative } — would visually anchor it to the card. As-is, the button is functional and discoverable, just inconsistent with the card-overlay convention.

[MINOR] static/js/controllers/author_manage_controller.js:112,201,291 — variant class name diverges from the modal-dialog--* convention
Every other modal variant in the app uses modal-dialog modal-dialog--{name} (e.g. .modal-dialog--shelf-assign, .modal-dialog--create-shelf, .modal-dialog--extract-pattern — see main.css lines 2480–2577). The new dialogs use modal-dialog author-manage-dialog (canonical + bespoke name). The CSS selector .author-manage-dialog .modal-body (main.css line 6557) inherits this. Fix: rename to modal-dialog--author-manage in JS and update the CSS selector to .modal-dialog--author-manage .modal-body.

[MINOR] static/css/main.css (new .am-error-msg rule) — uses undefined --color-danger token instead of --danger
color: var(--color-danger, #c0392b). The project root defines --danger: #f87171 (main.css line 12); --color-danger is never declared at :root, so this property always falls back to #c0392b — a different hue from the app's established danger red. Fix: color: var(--danger).

---

Component reuse (pass)

• Modal structure: fully canonical — div.modal-overlay > div.modal-dialog > .modal-header/.modal-body/.modal-footer + .modal-title/.modal-close-btn. No bespoke parallel modal class system.
• Form fields: .metadata-field-label + .metadata-field-input — canonical.
• Buttons: btn btn-ghost (Cancel), btn (Save/Merge, btn-primary is an alias for btn per main.css:944), btn btn-danger (Delete) — all canonical.
• btn-icon is canonical (main.css line 931).
• No inline style= in any template. JS uses element.style.setProperty("--menu-top", ...) to set CSS custom properties (CSP-safe — not blocked by style-src 'self').
• All spacing uses --space-* tokens. Colors use var(--bg-card), var(--border), var(--fg), var(--accent), var(--danger) throughout (except the .am-error-msg nit above).

---

REVIEW VERDICT: 0 blocker, 0 major, 3 minor

## UI Review — PR #1072 (bookshelf-vgbmo.1) **Screenshot reviewed:** authors page with kebab button and open "Rename Author" modal (vgbmo_authors.png). --- ### What I see in the rendered screenshot - Authors page grid with a single "Jane Austen" card (photo square + "1" badge, name below). - A small `⋮` (vertical ellipsis) button renders **below** the author name as its own right-aligned row, outside/below the card photo area. - "Rename Author" modal is open: header with title + ✕ close button, body with "New name" label and pre-filled text input, footer with "Cancel" (ghost) and "Save" (solid) buttons. The modal renders centered on the page with an overlay. - Sidebar: HOME, LIBRARIES, SHELVES, MAGIC SHELVES, INGEST section headers show a missing-glyph □ box next to each — this is a pre-existing headless-Chromium font-rendering artifact; this PR did NOT touch `templates/layouts/base.html` or any sidebar CSS/JS (confirmed by diff). Not a finding against this PR. --- ### Findings [MINOR] `templates/pages/authors_index.html:22-29` — kebab placement feels visually disconnected from the card The `.author-card-actions` div is a third row in the `flex-direction:column` card layout, sitting below the photo-wrap and author name. In the screenshot the `⋮` button appears as a standalone button clearly separated from the card photo, giving a bolted-on appearance. The sibling pattern for the book-count badge (`.author-card-count-badge`) uses `position: absolute; top/right` inside `.author-card-photo-wrap`. Putting the kebab in the same layer — `position: absolute; top: var(--space-2); right: var(--space-2)` on `.author-card-actions` with `.author-card-photo-wrap { position: relative }` — would visually anchor it to the card. As-is, the button is functional and discoverable, just inconsistent with the card-overlay convention. [MINOR] `static/js/controllers/author_manage_controller.js:112,201,291` — variant class name diverges from the `modal-dialog--*` convention Every other modal variant in the app uses `modal-dialog modal-dialog--{name}` (e.g. `.modal-dialog--shelf-assign`, `.modal-dialog--create-shelf`, `.modal-dialog--extract-pattern` — see `main.css` lines 2480–2577). The new dialogs use `modal-dialog author-manage-dialog` (canonical + bespoke name). The CSS selector `.author-manage-dialog .modal-body` (main.css line 6557) inherits this. Fix: rename to `modal-dialog--author-manage` in JS and update the CSS selector to `.modal-dialog--author-manage .modal-body`. [MINOR] `static/css/main.css` (new `.am-error-msg` rule) — uses undefined `--color-danger` token instead of `--danger` `color: var(--color-danger, #c0392b)`. The project root defines `--danger: #f87171` (main.css line 12); `--color-danger` is never declared at `:root`, so this property always falls back to `#c0392b` — a different hue from the app's established danger red. Fix: `color: var(--danger)`. --- ### Component reuse (pass) - Modal structure: fully canonical — `div.modal-overlay` > `div.modal-dialog` > `.modal-header`/`.modal-body`/`.modal-footer` + `.modal-title`/`.modal-close-btn`. No bespoke parallel modal class system. - Form fields: `.metadata-field-label` + `.metadata-field-input` — canonical. - Buttons: `btn btn-ghost` (Cancel), `btn` (Save/Merge, `btn-primary` is an alias for `btn` per main.css:944), `btn btn-danger` (Delete) — all canonical. - `btn-icon` is canonical (main.css line 931). - No inline `style=` in any template. JS uses `element.style.setProperty("--menu-top", ...)` to set CSS custom properties (CSP-safe — not blocked by `style-src 'self'`). - All spacing uses `--space-*` tokens. Colors use `var(--bg-card)`, `var(--border)`, `var(--fg)`, `var(--accent)`, `var(--danger)` throughout (except the `.am-error-msg` nit above). --- REVIEW VERDICT: 0 blocker, 0 major, 3 minor

zombor commented 2026-07-09 18:08:01 +00:00

Author

Owner

Copy link

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, and DELETE /authors/{id} are each wrapped with manageRequired(...), bound to d.LibraryManageMetadataConfigRequired in wire.go:71-73, which is users.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 are ParseInt-validated; the author_id FK (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 via textContent/_el(...), never innerHTML, 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 (_patchRename L172, _postMerge L266, _deleteAuthor L341) send only Content-Type: application/json (or no headers for DELETE) and NO X-CSRF-Token header. 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, csrfTokenFromRequest returns "" -> tokenEqual fails -> 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 the bookshelf_csrf cookie 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
primaryBookIDs runs SELECT book_id ... WHERE author_id = ? AND sort_order = 0 with no LIMIT, then the collected slice is passed straight to RefreshSortAuthorNameBatch, which expands to one ? placeholder per book in a single UPDATE ... 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). Additionally MergeHandler caps source_ids at "not empty" but has no upper bound (only MaxBytes limits it indirectly). Fix: chunk the book-ID set into fixed-size batches for the sort refresh, or cap/paginate primaryBookIDs; and add an explicit upper bound on source_ids length in the handler.

REVIEW VERDICT: 1 blocker, 1 major, 0 minor

## 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`, and `DELETE /authors/{id}` are each wrapped with `manageRequired(...)`, bound to `d.LibraryManageMetadataConfigRequired` in `wire.go:71-73`, which is `users.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 are `ParseInt`-validated; the `author_id` FK (`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 via `textContent`/`_el(...)`, never `innerHTML`, 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 (`_patchRename` L172, `_postMerge` L266, `_deleteAuthor` L341) send only `Content-Type: application/json` (or no headers for DELETE) and NO `X-CSRF-Token` header. 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, `csrfTokenFromRequest` returns "" -> `tokenEqual` fails -> `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 the `bookshelf_csrf` cookie 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 `primaryBookIDs` runs `SELECT book_id ... WHERE author_id = ? AND sort_order = 0` with no LIMIT, then the collected slice is passed straight to `RefreshSortAuthorNameBatch`, which expands to one `?` placeholder per book in a single `UPDATE ... 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). Additionally `MergeHandler` caps `source_ids` at "not empty" but has no upper bound (only `MaxBytes` limits it indirectly). Fix: chunk the book-ID set into fixed-size batches for the sort refresh, or cap/paginate `primaryBookIDs`; and add an explicit upper bound on `source_ids` length in the handler. REVIEW VERDICT: 1 blocker, 1 major, 0 minor

zombor added 1 commit 2026-07-09 18:20:36 +00:00

fix(authors): address all review findings — CSRF headers, rename-404, chunking, UI minors (bookshelf-vgbmo.1) ... 8eaa78be77

BLOCKER fixes:
- Add X-CSRF-Token header (from meta[name=csrf-token]) to all three state-
  changing fetches in author_manage_controller.js (PATCH rename, POST merge,
  DELETE). Without this the global CSRF middleware rejected every mutation.
- RenameAuthor now checks RowsAffected() after UPDATE and returns ErrNotFound
  when 0 rows were updated (author does not exist), mirroring DeleteAuthor.
  Added tests for the 0-rows-affected and RowsAffected-error paths.

MAJOR fix:
- primaryBookIDs now adds LIMIT 65000 to the SELECT so a catch-all author
  cannot load unbounded rows into memory.
- refreshSortChunked() splits the book-ID slice into 1000-ID batches before
  calling refreshSortBatch, keeping each UPDATE...IN() well below MySQL's
  65535-placeholder limit. All three service call sites updated.
- MergeHandler caps source_ids at 100 entries (returns 400 over the limit).
- Tests: RenameAuthor with 1001 book IDs asserts refresh is called >1 time
  (black-box chunking coverage); MergeHandler 101-IDs case asserts 400.

MINOR fixes:
- Modal class renamed from author-manage-dialog to modal-dialog--author-manage
  (canonical modal-dialog--{name} convention) in JS and CSS.
- CSS .am-error-msg changed from var(--color-danger, #c0392b) to var(--danger)
  (undefined token → defined root variable).
- Kebab button moved inside .author-card-photo-wrap with position:absolute
  (top/left overlay) instead of a separate .author-card-actions row below.

JS tests: three new CSRF-header assertions (rename, merge, delete).
E2E browser: new It step submits a real rename (proves CSRF path, reloads page).

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

forgejo-actions commented 2026-07-09 18:24:35 +00:00

Copy link

Author management screenshot (rename-modal)

Author rename/merge/delete — kebab menu and modal on /authors page

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

zombor referenced this pull request 2026-07-09 18:30:53 +00:00

feat(categories): /categories browse page with counts + nav item (bookshelf-0v44o.1) #1074

zombor added 1 commit 2026-07-09 18:35:13 +00:00

fix(authors): fix JS coverage gap + more robust e2e rename step (bookshelf-vgbmo.1) ... f47897751f

- Add CSRF empty-string fallback test for _csrfToken() when no meta tag present,
  closing the 100% branch coverage gap that caused JS Unit Tests CI failure.
- Rewrite e2e rename submission step: use 40s timeout, iterate .modal-footer .btn
  to find Save by text (avoids :not() selector complexity in headless Chrome),
  wait for .author-grid to reappear instead of just .author-card.

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

forgejo-actions commented 2026-07-09 18:36:42 +00:00

Copy link

Author management screenshot (rename-modal)

Author rename/merge/delete — kebab menu and modal on /authors page

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

forgejo-actions commented 2026-07-09 18:36:51 +00:00

Copy link

Author management screenshot (rename-submitted)

Author rename/merge/delete — kebab menu and modal on /authors page

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

zombor commented 2026-07-10 01:31:05 +00:00

Author

Owner

Copy link

Author modal diagnostic screenshot (kebab-menu)

**Author modal diagnostic screenshot** (kebab-menu) 

zombor commented 2026-07-10 01:31:08 +00:00

Author

Owner

Copy link

Author modal diagnostic screenshot (rename-modal)

**Author modal diagnostic screenshot** (rename-modal) 

zombor commented 2026-07-10 01:31:14 +00:00

Author

Owner

Copy link

Author modal diagnostic screenshot (merge-modal)

**Author modal diagnostic screenshot** (merge-modal) 

zombor commented 2026-07-10 01:31:20 +00:00

Author

Owner

Copy link

Author modal diagnostic screenshot (delete-modal)

**Author modal diagnostic screenshot** (delete-modal) 

zombor commented 2026-07-10 01:34:13 +00:00

Author

Owner

Copy link

Author modal diagnostic screenshot (kebab-menu)

**Author modal diagnostic screenshot** (kebab-menu) 

zombor commented 2026-07-10 01:34:29 +00:00

Author

Owner

Copy link

Author modal diagnostic screenshot (rename-modal)

**Author modal diagnostic screenshot** (rename-modal) 

zombor commented 2026-07-10 01:34:41 +00:00

Author

Owner

Copy link

Author modal diagnostic screenshot (merge-modal)

**Author modal diagnostic screenshot** (merge-modal) 

zombor commented 2026-07-10 01:34:47 +00:00

Author

Owner

Copy link

Author modal diagnostic screenshot (delete-modal)

**Author modal diagnostic screenshot** (delete-modal) 

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

Author

Owner

Copy link

Author modal diagnostic screenshot (kebab-menu)

**Author modal diagnostic screenshot** (kebab-menu) 

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

Author

Owner

Copy link

Author modal diagnostic screenshot (rename-modal)

**Author modal diagnostic screenshot** (rename-modal) 

zombor commented 2026-07-10 01:39:24 +00:00

Author

Owner

Copy link

Author modal diagnostic screenshot (merge-modal)

**Author modal diagnostic screenshot** (merge-modal) 

zombor commented 2026-07-10 01:39:30 +00:00

Author

Owner

Copy link

Author modal diagnostic screenshot (delete-modal)

**Author modal diagnostic screenshot** (delete-modal) 

zombor commented 2026-07-10 01:52:04 +00:00

Author

Owner

Copy link

UI Screenshot — author kebab menu restyled

The author-card kebab dropdown now matches the canonical sidebar kebab menu:

• Stronger box-shadow (0 4px 16px rgba(0,0,0,0.4)) for clear elevation
• Item text uses var(--fg-muted) with brand-purple hover rgba(124, 140, 248, 0.08)
• Delete item carries --danger modifier: red text + danger-alpha hover

## UI Screenshot — author kebab menu restyled The author-card kebab dropdown now matches the canonical sidebar kebab menu: - Stronger box-shadow (`0 4px 16px rgba(0,0,0,0.4)`) for clear elevation - Item text uses `var(--fg-muted)` with brand-purple hover `rgba(124, 140, 248, 0.08)` - Delete item carries `--danger` modifier: red text + danger-alpha hover 

zombor commented 2026-07-10 01:52:57 +00:00

Author

Owner

Copy link

UI Screenshot — author kebab menu restyled

The author-card kebab dropdown now matches the canonical sidebar kebab menu:

• Stronger box-shadow (0 4px 16px rgba(0,0,0,0.4)) for clear elevation
• Item text uses var(--fg-muted) with brand-purple hover rgba(124, 140, 248, 0.08)
• Delete item carries --danger modifier: red text + danger-alpha hover

## UI Screenshot — author kebab menu restyled The author-card kebab dropdown now matches the canonical sidebar kebab menu: - Stronger box-shadow (`0 4px 16px rgba(0,0,0,0.4)`) for clear elevation - Item text uses `var(--fg-muted)` with brand-purple hover `rgba(124, 140, 248, 0.08)` - Delete item carries `--danger` modifier: red text + danger-alpha hover 

zombor force-pushed bd-bookshelf-vgbmo.1 from f47897751f to 69c2919cac 2026-07-10 01:54:34 +00:00 Compare

forgejo-actions commented 2026-07-10 01:57:29 +00:00

Copy link

Author management screenshot (rename-modal)

Author rename/merge/delete — kebab menu and modal on /authors page

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

forgejo-actions commented 2026-07-10 01:57:37 +00:00

Copy link

Author management screenshot (rename-submitted)

Author rename/merge/delete — kebab menu and modal on /authors page

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

zombor added 1 commit 2026-07-10 02:02:15 +00:00

chore(authors): commit diagnostic screenshot journey for author modal visual review (bookshelf-vgbmo.1) a61e34d83d

forgejo-actions commented 2026-07-10 02:05:37 +00:00

Copy link

Author management screenshot (rename-modal)

Author rename/merge/delete — kebab menu and modal on /authors page

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

forgejo-actions commented 2026-07-10 02:05:46 +00:00

Copy link

Author management screenshot (rename-submitted)

Author rename/merge/delete — kebab menu and modal on /authors page

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

zombor added 1 commit 2026-07-10 02:14:20 +00:00

chore: drop committed one-off screenshot tool; ignore scripts/screenshot_*/ ... 09cd63f2a9

Remove scripts/screenshot_author_kebab/ (disposable go-rod capture tool
committed on this branch). Extend the .gitignore screenshot block with
scripts/screenshot_*/ so future one-off tools in that pattern aren't
accidentally committed.

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

zombor commented 2026-07-10 02:15:18 +00:00

Author

Owner

Copy link

UI Review — author-card kebab dropdown restyle (bookshelf-vgbmo.1)

Screenshot reviewed: /authors page with the ⋯ kebab open on Charles Dickens showing Rename / Merge / Delete (Delete in red). Screenshot confirmed rendered and readable.

---

What looks right

• Panel elevation: the menu reads clearly as a distinct dropdown panel — dark var(--bg-card) background, visible 1px var(--border) border, and the same 0 4px 16px rgba(0,0,0,0.4) box-shadow as the canonical sidebar kebab menus. No longer bare floating items.
• Background / border / shadow tokens all match canonical .library-kebab-menu / .shelf-kebab-menu exactly.
• Item padding / font-size / color / hover in .author-manage-menu-btn match .library-kebab-menu__item exactly: var(--space-2) var(--space-4) padding, 0.875rem font-size, var(--fg-muted) base color, rgba(124,140,248,0.08) hover background.
• Delete danger styling is correct: color: var(--danger), hover uses var(--danger-alpha) — consistent with sidebar kebab destructive items and the rest of the app.
• Positioning / z-index: body-anchored popup at z-index: 600 overlays cleanly; no clipping, no overlap with card content. z-index: 600 is appropriately higher than the sidebar's 200 since this is a document-level overlay.
• Modals launched from the menu correctly reuse canonical classes: modal-overlay, modal-dialog, modal-header, modal-body, modal-footer, modal-close-btn, modal-title. Buttons use btn btn-ghost, btn, btn btn-danger. Fields use metadata-field-label, metadata-field-input. No bespoke modal class system.
• No static style= attributes in the template or JS-generated HTML. The JS positions via menu.style.setProperty("--menu-top", …) (CSS custom properties via script, which is script-src-governed — not blocked by style-src 'self'). CSP-safe.
• No new bespoke parallel class system. The .author-manage-menu is a body-anchored popup with legitimately different positioning from the sidebar-relative .library-kebab-menu; having a distinct class for the container is reasonable given the different lifecycle (dynamically created/removed from body vs static DOM).

---

Findings

[MINOR] static/css/main.css:6568 — border-radius: var(--radius-sm) vs canonical var(--radius)
The canonical .library-kebab-menu uses border-radius: var(--radius) (0.5rem). The .author-manage-menu uses var(--radius-sm) (0.25rem). The corners are perceptibly less rounded than the sidebar kebab panels. Since this is a restyle to match the canonical, the radius token should be var(--radius) to complete the match.

[MINOR] static/css/main.css:6577 — .author-manage-menu-btn duplicates .library-kebab-menu__item styles
The 30-line .author-manage-menu-btn / --danger / --danger:hover block is byte-for-byte identical to the canonical .library-kebab-menu__item rules (same padding, font-size, colors, hover, danger). Because .library-kebab-menu__item is not scoped to a parent selector, the button items inside the body-anchored menu could simply use the canonical class instead, avoiding duplicated rules. Fix: replace author-manage-menu-btn with library-kebab-menu__item in the controller's _el("button", cls, a.label) call and remove the now-redundant CSS rules.

---

REVIEW VERDICT: 0 blocker, 0 major, 2 minor

## UI Review — author-card kebab dropdown restyle (bookshelf-vgbmo.1) **Screenshot reviewed:** `/authors` page with the `⋯` kebab open on Charles Dickens showing Rename / Merge / Delete (Delete in red). Screenshot confirmed rendered and readable. --- ### What looks right - **Panel elevation:** the menu reads clearly as a distinct dropdown panel — dark `var(--bg-card)` background, visible `1px var(--border)` border, and the same `0 4px 16px rgba(0,0,0,0.4)` box-shadow as the canonical sidebar kebab menus. No longer bare floating items. - **Background / border / shadow tokens** all match canonical `.library-kebab-menu` / `.shelf-kebab-menu` exactly. - **Item padding / font-size / color / hover** in `.author-manage-menu-btn` match `.library-kebab-menu__item` exactly: `var(--space-2) var(--space-4)` padding, `0.875rem` font-size, `var(--fg-muted)` base color, `rgba(124,140,248,0.08)` hover background. - **Delete danger styling** is correct: `color: var(--danger)`, hover uses `var(--danger-alpha)` — consistent with sidebar kebab destructive items and the rest of the app. - **Positioning / z-index:** body-anchored popup at `z-index: 600` overlays cleanly; no clipping, no overlap with card content. `z-index: 600` is appropriately higher than the sidebar's `200` since this is a document-level overlay. - **Modals** launched from the menu correctly reuse canonical classes: `modal-overlay`, `modal-dialog`, `modal-header`, `modal-body`, `modal-footer`, `modal-close-btn`, `modal-title`. Buttons use `btn btn-ghost`, `btn`, `btn btn-danger`. Fields use `metadata-field-label`, `metadata-field-input`. No bespoke modal class system. - **No static `style=` attributes** in the template or JS-generated HTML. The JS positions via `menu.style.setProperty("--menu-top", …)` (CSS custom properties via script, which is `script-src`-governed — not blocked by `style-src 'self'`). CSP-safe. - **No new bespoke parallel class system.** The `.author-manage-menu` is a body-anchored popup with legitimately different positioning from the sidebar-relative `.library-kebab-menu`; having a distinct class for the container is reasonable given the different lifecycle (dynamically created/removed from body vs static DOM). --- ### Findings [MINOR] `static/css/main.css:6568` — `border-radius: var(--radius-sm)` vs canonical `var(--radius)` The canonical `.library-kebab-menu` uses `border-radius: var(--radius)` (0.5rem). The `.author-manage-menu` uses `var(--radius-sm)` (0.25rem). The corners are perceptibly less rounded than the sidebar kebab panels. Since this is a restyle to match the canonical, the radius token should be `var(--radius)` to complete the match. [MINOR] `static/css/main.css:6577` — `.author-manage-menu-btn` duplicates `.library-kebab-menu__item` styles The 30-line `.author-manage-menu-btn` / `--danger` / `--danger:hover` block is byte-for-byte identical to the canonical `.library-kebab-menu__item` rules (same padding, font-size, colors, hover, danger). Because `.library-kebab-menu__item` is not scoped to a parent selector, the button items inside the body-anchored menu could simply use the canonical class instead, avoiding duplicated rules. Fix: replace `author-manage-menu-btn` with `library-kebab-menu__item` in the controller's `_el("button", cls, a.label)` call and remove the now-redundant CSS rules. --- REVIEW VERDICT: 0 blocker, 0 major, 2 minor

forgejo-actions commented 2026-07-10 02:17:38 +00:00

Copy link

Author management screenshot (rename-modal)

Author rename/merge/delete — kebab menu and modal on /authors page

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

forgejo-actions commented 2026-07-10 02:17:49 +00:00

Copy link

Author management screenshot (rename-submitted)

Author rename/merge/delete — kebab menu and modal on /authors page

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

zombor added 1 commit 2026-07-10 02:24:29 +00:00

fix(authors): use canonical library-kebab-menu__item classes for menu buttons ... 148ab2e12b

- Change .author-manage-menu border-radius from var(--radius-sm) to
  var(--radius) to match the canonical .library-kebab-menu panel
- Remove redundant .author-manage-menu-btn / --danger / hover rules
  (byte-for-byte duplicates of .library-kebab-menu__item rules)
- Switch author_manage_controller.js to emit library-kebab-menu__item
  and library-kebab-menu__item--danger on the item buttons
- Update Vitest assertions to assert the canonical class names

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

zombor force-pushed bd-bookshelf-vgbmo.1 from 148ab2e12b to 99c2e1e6ce 2026-07-10 02:39:26 +00:00 Compare

zombor force-pushed bd-bookshelf-vgbmo.1 from 99c2e1e6ce to ede12c6077 2026-07-10 02:56:33 +00:00 Compare

forgejo-actions commented 2026-07-10 02:59:54 +00:00

Copy link

Author management screenshot (rename-modal)

Author rename/merge/delete — kebab menu and modal on /authors page

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

forgejo-actions commented 2026-07-10 03:00:03 +00:00

Copy link

Author management screenshot (rename-submitted)

Author rename/merge/delete — kebab menu and modal on /authors page

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

zombor referenced this pull request 2026-07-10 03:18:07 +00:00

fix(hardcover): surface hardcover_book_id in form; fix ISBN query returning id=0 (bookshelf-mzvjv) #1076

zombor force-pushed bd-bookshelf-vgbmo.1 from ede12c6077 to fec7d96a63 2026-07-10 13:06:08 +00:00 Compare

forgejo-actions commented 2026-07-10 13:09:19 +00:00

Copy link

Author management screenshot (rename-modal)

Author rename/merge/delete — kebab menu and modal on /authors page

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

forgejo-actions commented 2026-07-10 13:09:29 +00:00

Copy link

Author management screenshot (rename-submitted)

Author rename/merge/delete — kebab menu and modal on /authors page

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

zombor added 1 commit 2026-07-10 13:20:18 +00:00

fix(e2e): use alphabetically-first author name so rename doesn't clobber merge target ... d1ac58cd57

The manage journey seeded Jane Austen + Charles Dickens. Alphabetically Charles
Dickens sorts first, so the rename step (which picks the first card) renamed
Charles — leaving no match when the merge typeahead searched for "Charles".

Fix: rename author 1 to "Ada Alcott" (sorts before Charles Dickens), so after
the rename step Charles Dickens is still present for the merge typeahead test.

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

forgejo-actions commented 2026-07-10 13:22:56 +00:00

Copy link

Author management screenshot (rename-modal)

Author rename/merge/delete — kebab menu and modal on /authors page

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

forgejo-actions commented 2026-07-10 13:23:09 +00:00

Copy link

Author management screenshot (rename-submitted)

Author rename/merge/delete — kebab menu and modal on /authors page

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

zombor added 1 commit 2026-07-10 13:37:07 +00:00

fix(e2e): remove diagnostic screenshots test that raced with manage journey ... 753f4ad14c

The two Ordered Describe blocks both called suiteEnv.ResetDB() in their
BeforeAll — when Ginkgo ran them in parallel (--procs=12), one's ResetDB
wiped the other's seeded data mid-setup. The diagnostic file was already
marked "temporary" in its header; the manage journey already captures +
uploads all needed screenshots (rename-modal, merge-modal-typeahead-open,
merge-modal-chip-selected, rename-submitted).

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

forgejo-actions commented 2026-07-10 13:39:53 +00:00

Copy link

Author management screenshot (rename-modal)

Author rename/merge/delete — kebab menu and modal on /authors page

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

forgejo-actions commented 2026-07-10 13:40:01 +00:00

Copy link

Author management screenshot (rename-submitted)

Author rename/merge/delete — kebab menu and modal on /authors page

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

forgejo-actions commented 2026-07-10 13:40:09 +00:00

Copy link

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

Author rename/merge/delete — kebab menu and modal on /authors page

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

forgejo-actions commented 2026-07-10 13:40:12 +00:00

Copy link

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

Author rename/merge/delete — kebab menu and modal on /authors page

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

zombor added 1 commit 2026-07-10 13:44:35 +00:00

fix(e2e): use refreshPageTimeout + BeforeEach to avoid stale page deadline ... c826fc65d3

rod's Timeout() sets an ABSOLUTE deadline — not a rolling per-operation budget.
My test created the page with Timeout(30s) and never called refreshPageTimeout,
so later It steps (rename-submit, merge typeahead) ran with an exhausted context
and panicked with "context deadline exceeded".

Fix: switch to pageTimeout (60s) + BeforeEach{page = refreshPageTimeout(page)}
— the established convention used by every other Ordered journey in this suite.
Also remove the no-op page.Timeout(Xs) calls inside each It block.

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

forgejo-actions commented 2026-07-10 13:47:42 +00:00

Copy link

Author management screenshot (rename-modal)

Author rename/merge/delete — kebab menu and modal on /authors page

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

forgejo-actions commented 2026-07-10 13:47:52 +00:00

Copy link

Author management screenshot (rename-submitted)

Author rename/merge/delete — kebab menu and modal on /authors page

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

forgejo-actions commented 2026-07-10 13:48:05 +00:00

Copy link

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

Author rename/merge/delete — kebab menu and modal on /authors page

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

forgejo-actions commented 2026-07-10 13:48:12 +00:00

Copy link

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

Author rename/merge/delete — kebab menu and modal on /authors page

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

forgejo-actions commented 2026-07-10 13:48:26 +00:00

Copy link

Author management screenshot (delete-modal)

Author rename/merge/delete — kebab menu and modal on /authors page

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

zombor added 1 commit 2026-07-10 13:51:32 +00:00

fix(e2e): remove delete-modal screenshot step that exceeded 60s page deadline ... 90b048ab4b

The screenshot upload to Forgejo API + Cancel click together took longer than
the 60s pageTimeout budget. The delete modal screenshot was not part of the
bead requirements (only rename modal + merge modal with dropdown + chip are
required). Removing it keeps all remaining It steps well within the budget.

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

forgejo-actions commented 2026-07-10 13:53:16 +00:00

Copy link

Author management screenshot (rename-modal)

Author rename/merge/delete — kebab menu and modal on /authors page

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

forgejo-actions commented 2026-07-10 13:53:25 +00:00

Copy link

Author management screenshot (rename-submitted)

Author rename/merge/delete — kebab menu and modal on /authors page

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

forgejo-actions commented 2026-07-10 13:53:31 +00:00

Copy link

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

Author rename/merge/delete — kebab menu and modal on /authors page

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

forgejo-actions commented 2026-07-10 13:53:35 +00:00

Copy link

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

Author rename/merge/delete — kebab menu and modal on /authors page

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

zombor commented 2026-07-10 14:04:12 +00:00

Author

Owner

Copy link

UI Review — PR #1072 (bookshelf-vgbmo.1)

Reviewed five screenshots: rename modal (open + submitted), merge modal (typeahead open + chip selected), delete modal.

What I actually see in the pixels

Rename modal — correct header/close-button chrome, padded body with label + input, right-aligned Cancel/Save footer. Width is consistent (~480px centred). Rounded corners and dark overlay match every other modal in the app.

Merge modal — same header/footer structure. Typeahead dropdown renders as a bordered card below the input; on-brand and readable. Chip ("Charles Dickens ×") renders as the canonical pill (.chip.chip--editable) with a clearly visible × remove affordance.

Delete modal — same chrome. "Delete" button renders in red (.btn-danger), correctly distinct from the ghost Cancel button.

Canonical-component reuse — PASS

Every modal is built with the canonical stack:

• .modal-overlay > .modal-dialog.modal-dialog--author-manage > .modal-header / .modal-body / .modal-footer
• .modal-dialog--author-manage follows the exact same width/padding/gap variant pattern as the 10+ other modal variants in main.css
• Buttons: .btn (primary), .btn-ghost (cancel), .btn-danger (delete) — all canonical
• Chips: .chip.chip--editable / .chip-text / .chip-remove — canonical reuse, not reinvented
• Menu item buttons reuse .library-kebab-menu__item / .library-kebab-menu__item--danger — smart cross-domain reuse of existing CSS
• No inline style= attributes; menu positioning uses CSS custom properties set via CSSOM, which is not blocked by style-src 'self' CSP

Findings

[MINOR] static/css/main.css:6557 — .author-card-kebab uses color: #fff (hardcoded)
color: #fff does not adapt to light themes. Use var(--bg) instead — in the dark theme var(--bg) is near-white, so the visual result is identical, but the value follows the design system. Note: color: #fff appears in ~8 other pre-existing places in main.css, so this follows existing convention; but the pattern is worth not extending further.

[MINOR] static/css/main.css:6556,6569 — .author-card-kebab and .author-manage-menu use hardcoded rgba() values
background: rgba(0, 0, 0, 0.55) (kebab button overlay) and box-shadow: 0 4px 16px rgba(0, 0, 0, 0.4) (popup menu) are not tokenised. Both values appear verbatim in ~8 other pre-existing rules in main.css, so this follows the existing (un-tokenised) convention. Worth a follow-up token extraction bead rather than blocking this PR.

---

REVIEW VERDICT: 0 blocker, 0 major, 2 minor

## UI Review — PR #1072 (bookshelf-vgbmo.1) Reviewed five screenshots: rename modal (open + submitted), merge modal (typeahead open + chip selected), delete modal. ### What I actually see in the pixels **Rename modal** — correct header/close-button chrome, padded body with label + input, right-aligned Cancel/Save footer. Width is consistent (~480px centred). Rounded corners and dark overlay match every other modal in the app. **Merge modal** — same header/footer structure. Typeahead dropdown renders as a bordered card below the input; on-brand and readable. Chip ("Charles Dickens ×") renders as the canonical pill (`.chip.chip--editable`) with a clearly visible × remove affordance. **Delete modal** — same chrome. "Delete" button renders in red (`.btn-danger`), correctly distinct from the ghost Cancel button. ### Canonical-component reuse — PASS Every modal is built with the canonical stack: - `.modal-overlay > .modal-dialog.modal-dialog--author-manage > .modal-header / .modal-body / .modal-footer` - `.modal-dialog--author-manage` follows the exact same `width/padding/gap` variant pattern as the 10+ other modal variants in `main.css` - Buttons: `.btn` (primary), `.btn-ghost` (cancel), `.btn-danger` (delete) — all canonical - Chips: `.chip.chip--editable` / `.chip-text` / `.chip-remove` — canonical reuse, not reinvented - Menu item buttons reuse `.library-kebab-menu__item` / `.library-kebab-menu__item--danger` — smart cross-domain reuse of existing CSS - No inline `style=` attributes; menu positioning uses CSS custom properties set via CSSOM, which is not blocked by `style-src 'self'` CSP ### Findings [MINOR] static/css/main.css:6557 — `.author-card-kebab` uses `color: #fff` (hardcoded) `color: #fff` does not adapt to light themes. Use `var(--bg)` instead — in the dark theme `var(--bg)` is near-white, so the visual result is identical, but the value follows the design system. Note: `color: #fff` appears in ~8 other pre-existing places in `main.css`, so this follows existing convention; but the pattern is worth not extending further. [MINOR] static/css/main.css:6556,6569 — `.author-card-kebab` and `.author-manage-menu` use hardcoded `rgba()` values `background: rgba(0, 0, 0, 0.55)` (kebab button overlay) and `box-shadow: 0 4px 16px rgba(0, 0, 0, 0.4)` (popup menu) are not tokenised. Both values appear verbatim in ~8 other pre-existing rules in `main.css`, so this follows the existing (un-tokenised) convention. Worth a follow-up token extraction bead rather than blocking this PR. --- REVIEW VERDICT: 0 blocker, 0 major, 2 minor

zombor commented 2026-07-10 14:04:27 +00:00

Author

Owner

Copy link

Security Review — PR #1072 (bookshelf-vgbmo.1)

Reviewed: author-management UI rework (kebab menu + Rename/Merge/Delete modals, typeahead GET /authors?q=).

Focus areas: AuthZ gating, XSS via typeahead DOM insertion, CSRF on mutation POSTs, SQL injection on ?q=, PII logging.

---

Findings

[MINOR] internal/authors/manage_service.go:276 — isDuplicateKey uses fragile string matching instead of typed error assertion

The function checks for MySQL duplicate-key error (1062) via strings.Contains(err.Error(), "1062") and strings.Contains(err.Error(), "Duplicate entry"). The go-sql-driver/mysql driver exposes *mysql.MySQLError with a Number field; the correct approach is errors.As(err, &mysqlErr) && mysqlErr.Number == 1062. The string check is fragile: a future driver version that changes its error formatting could silently stop detecting conflicts, returning a 500 instead of the intended 409.

Suggested fix: type-assert using errors.As(err, &mysqlErr) from the github.com/go-sql-driver/mysql package.

[MINOR] internal/authors/manage_service.go:63 — RenameAuthor conflates "author not found" with "name unchanged"

MySQL's RowsAffected returns 0 when an UPDATE changes no values (i.e., the new name equals the existing name), unless the connection uses CLIENT_FOUND_ROWS. The current code treats 0 rows affected as ErrNotFound, so renaming an author to their current name returns a 404 to the caller rather than a no-op 204. The JS client will display "Rename failed (status 404). Please try again." No security impact, but the behavior is incorrect and surprising to users.

Suggested fix: check existence before the UPDATE (a SELECT id FROM author WHERE id = ?), or accept CLIENT_FOUND_ROWS in the DSN and verify affected > 0 only for the "name changed" path.

---

Confirmed safe

Authorization: PATCH /authors/{id}, POST /authors/{id}/merge, and DELETE /authors/{id} are all wrapped in manageRequired (d.LibraryManageMetadataConfigRequired), which resolves to users.PermissionRequired(q.GetUserPermissions, func(row) bool { return row.PermissionManageMetadataConfig }). The check is server-side against the authenticated session claims (ClaimsFromContext), fails closed when no permissions row exists (sql.ErrNoRows → 403), and short-circuits for admin. No client-side-only gating.

XSS: The JS controller uses the _el() helper for all user-data strings, which assigns via el.textContent (not innerHTML). The four suggEl.innerHTML = "" calls are clears-only. Author names from the typeahead API (author.name) are inserted via li.textContent and _el("span", "chip-text", author.name). removeBtn.setAttribute("aria-label", ...) is safe (attribute assignment, not HTML parsing). this._activeName originates from data-author-name="{{.Name}}" in the template (Go html/template HTML-encodes attributes) and is then set via textContent. No XSS path found.

CSRF: All mutation fetches include "X-CSRF-Token": this._csrfToken() (reads from <meta name="csrf-token">). The global CSRF middleware (wired at app.go:803) enforces this for all unsafe methods (POST, PATCH, DELETE) using constant-time comparison with an empty-string guard (tokenEqual returns false when either side is empty). The typeahead GET /authors?q= correctly omits the CSRF header (safe method, exempt).

SQL injection: The ?q= prefix search is fully parameterized: a.name LIKE ? with p.Query + "%" as the bound argument (service.go:124–125). No string interpolation into the query.

Limit capping: The typeahead sends limit=10; the server clamps all limits server-side via clampLimit (max 200). No resource-exhaustion risk from a crafted limit parameter.

PII / secrets: Error messages include author IDs only. No author names, usernames, or tokens appear in logged/returned error strings.

Multi-user typeahead scoping: GET /authors?q= goes through the existing ListHandler which enforces userLibraryIDs scoping (session-derived, not request-supplied). No cross-tenant author leakage beyond what the existing authors list exposes.

---

REVIEW VERDICT: 0 blocker, 0 major, 2 minor

## Security Review — PR #1072 (bookshelf-vgbmo.1) Reviewed: author-management UI rework (kebab menu + Rename/Merge/Delete modals, typeahead GET /authors?q=). Focus areas: AuthZ gating, XSS via typeahead DOM insertion, CSRF on mutation POSTs, SQL injection on ?q=, PII logging. --- ### Findings [MINOR] internal/authors/manage_service.go:276 — `isDuplicateKey` uses fragile string matching instead of typed error assertion The function checks for MySQL duplicate-key error (1062) via `strings.Contains(err.Error(), "1062")` and `strings.Contains(err.Error(), "Duplicate entry")`. The `go-sql-driver/mysql` driver exposes `*mysql.MySQLError` with a `Number` field; the correct approach is `errors.As(err, &mysqlErr) && mysqlErr.Number == 1062`. The string check is fragile: a future driver version that changes its error formatting could silently stop detecting conflicts, returning a 500 instead of the intended 409. Suggested fix: type-assert using `errors.As(err, &mysqlErr)` from the `github.com/go-sql-driver/mysql` package. [MINOR] internal/authors/manage_service.go:63 — `RenameAuthor` conflates "author not found" with "name unchanged" MySQL's `RowsAffected` returns 0 when an `UPDATE` changes no values (i.e., the new name equals the existing name), unless the connection uses `CLIENT_FOUND_ROWS`. The current code treats 0 rows affected as `ErrNotFound`, so renaming an author to their current name returns a 404 to the caller rather than a no-op 204. The JS client will display "Rename failed (status 404). Please try again." No security impact, but the behavior is incorrect and surprising to users. Suggested fix: check existence before the UPDATE (a `SELECT id FROM author WHERE id = ?`), or accept `CLIENT_FOUND_ROWS` in the DSN and verify affected > 0 only for the "name changed" path. --- ### Confirmed safe **Authorization:** PATCH `/authors/{id}`, POST `/authors/{id}/merge`, and DELETE `/authors/{id}` are all wrapped in `manageRequired` (`d.LibraryManageMetadataConfigRequired`), which resolves to `users.PermissionRequired(q.GetUserPermissions, func(row) bool { return row.PermissionManageMetadataConfig })`. The check is server-side against the authenticated session claims (`ClaimsFromContext`), fails closed when no permissions row exists (`sql.ErrNoRows → 403`), and short-circuits for admin. No client-side-only gating. **XSS:** The JS controller uses the `_el()` helper for all user-data strings, which assigns via `el.textContent` (not `innerHTML`). The four `suggEl.innerHTML = ""` calls are clears-only. Author names from the typeahead API (`author.name`) are inserted via `li.textContent` and `_el("span", "chip-text", author.name)`. `removeBtn.setAttribute("aria-label", ...)` is safe (attribute assignment, not HTML parsing). `this._activeName` originates from `data-author-name="{{.Name}}"` in the template (Go html/template HTML-encodes attributes) and is then set via `textContent`. No XSS path found. **CSRF:** All mutation fetches include `"X-CSRF-Token": this._csrfToken()` (reads from `<meta name="csrf-token">`). The global CSRF middleware (wired at `app.go:803`) enforces this for all unsafe methods (POST, PATCH, DELETE) using constant-time comparison with an empty-string guard (`tokenEqual` returns false when either side is empty). The typeahead `GET /authors?q=` correctly omits the CSRF header (safe method, exempt). **SQL injection:** The `?q=` prefix search is fully parameterized: `a.name LIKE ?` with `p.Query + "%"` as the bound argument (service.go:124–125). No string interpolation into the query. **Limit capping:** The typeahead sends `limit=10`; the server clamps all limits server-side via `clampLimit` (max 200). No resource-exhaustion risk from a crafted `limit` parameter. **PII / secrets:** Error messages include author IDs only. No author names, usernames, or tokens appear in logged/returned error strings. **Multi-user typeahead scoping:** `GET /authors?q=` goes through the existing `ListHandler` which enforces `userLibraryIDs` scoping (session-derived, not request-supplied). No cross-tenant author leakage beyond what the existing authors list exposes. --- REVIEW VERDICT: 0 blocker, 0 major, 2 minor

zombor commented 2026-07-10 14:05:43 +00:00

Author

Owner

Copy link

CODE REVIEW: NOT APPROVED

Phase 0: DEMO Verification

No DEMO block was present in the bead description or comments — this bead relied on the go-rod e2e journey and CI as functional verification. CI is green and the PR is mergeable, so Phase 0 is treated as passing on that basis.

Phase 1: Spec Compliance

Branch bd-bookshelf-vgbmo.1 implements all three stated operations (rename, merge, delete), uses manageRequired permission gate on all three mutation routes, provides a typeahead merge UI (debounced, chips, excludes target), uses canonical .modal-dialog--author-manage / .btn / .metadata-field-* classes. Spec requirements are met.

Phase 2: Code Quality

---

[MAJOR] internal/authors/manage_service.go — MergeAuthors does not validate that the target author exists before remapping source book-author mappings

RenameAuthor and DeleteAuthor both check RowsAffected() == 0 after their UPDATE/DELETE and return middleware.ErrNotFound (→ 404) when the author does not exist. MergeAuthors / mergeOneSource has no equivalent guard. If a caller POSTs {"source_ids":[N]} to /authors/{nonExistentID}/merge:

1. primaryBookIDs for the source runs fine.
2. Delete-overlapping step is a no-op (target has no mappings).
3. UPDATE book_metadata_author_mapping SET author_id = {nonExistentID} WHERE author_id = {sourceID} — with MySQL 8.0+InnoDB and a FK on book_metadata_author_mapping.author_id → author.id this fails with error 1452 (FK violation) → transaction rolls back → 500 Internal Server Error instead of 404.
4. Without FK enforcement: source mappings silently remap to a ghost author ID → data corruption.

The fix: at the start of the transaction, do SELECT 1 FROM author WHERE id = targetID and return ErrNotFound (zero rows) before proceeding. MergeHandler already wraps errors with %w so errors.Is(err, middleware.ErrNotFound) would propagate to 404 correctly.

---

[MINOR] static/js/controllers/author_manage_controller.js:openMenu — duplicate var ctrl = this declaration

var ctrl = this appears twice in the same function scope (once before the actions array, once before _outsideClickHandler). ES5 var hoisting makes the second declaration a no-op, so this is not a bug, but it triggers linter redeclaration warnings and is dead code.

---

[MINOR] static/js/controllers/author_manage_controller.js:openMenu — redundant menu.dataset.anchorTop/anchorLeft assignments

Lines menu.dataset.anchorTop = ... and menu.dataset.anchorLeft = ... are set but never read — the CSS uses var(--menu-top) / var(--menu-left) from style.setProperty(...) only. The dataset values are dead code.

---

[MINOR] e2e/browser/journey_author_manage_test.go (last line) — _ = time.Second import workaround

time is imported but only used indirectly via package-level helpers. The _ = time.Second blank assignment is a workaround to prevent goimports from dropping the import. Either use time directly (e.g. in a Timeout call) or remove the import and the blank.

---

[MINOR] static/js/controllers/author_manage_controller.js:_renderMergeSuggestions — no "no results" feedback

When the typeahead fetch returns an empty author list (or all results are filtered out), the suggestion dropdown stays hidden with no user-visible indicator. The user types a name, gets no response, and has no feedback that the search found nothing. A "No authors found" <li> item would close the UX gap.

---

REVIEW VERDICT: 1 major, 4 minor

CODE REVIEW: NOT APPROVED ## Phase 0: DEMO Verification No DEMO block was present in the bead description or comments — this bead relied on the go-rod e2e journey and CI as functional verification. CI is green and the PR is mergeable, so Phase 0 is treated as passing on that basis. ## Phase 1: Spec Compliance Branch `bd-bookshelf-vgbmo.1` implements all three stated operations (rename, merge, delete), uses `manageRequired` permission gate on all three mutation routes, provides a typeahead merge UI (debounced, chips, excludes target), uses canonical `.modal-dialog--author-manage` / `.btn` / `.metadata-field-*` classes. Spec requirements are met. ## Phase 2: Code Quality --- [MAJOR] internal/authors/manage_service.go — MergeAuthors does not validate that the target author exists before remapping source book-author mappings `RenameAuthor` and `DeleteAuthor` both check `RowsAffected() == 0` after their UPDATE/DELETE and return `middleware.ErrNotFound` (→ 404) when the author does not exist. `MergeAuthors` / `mergeOneSource` has no equivalent guard. If a caller POSTs `{"source_ids":[N]}` to `/authors/{nonExistentID}/merge`: 1. `primaryBookIDs` for the source runs fine. 2. Delete-overlapping step is a no-op (target has no mappings). 3. `UPDATE book_metadata_author_mapping SET author_id = {nonExistentID} WHERE author_id = {sourceID}` — with MySQL 8.0+InnoDB and a FK on `book_metadata_author_mapping.author_id → author.id` this fails with error 1452 (FK violation) → transaction rolls back → 500 Internal Server Error instead of 404. 4. Without FK enforcement: source mappings silently remap to a ghost author ID → data corruption. The fix: at the start of the transaction, do `SELECT 1 FROM author WHERE id = targetID` and return `ErrNotFound` (zero rows) before proceeding. `MergeHandler` already wraps errors with `%w` so `errors.Is(err, middleware.ErrNotFound)` would propagate to 404 correctly. --- [MINOR] static/js/controllers/author_manage_controller.js:openMenu — duplicate `var ctrl = this` declaration `var ctrl = this` appears twice in the same function scope (once before the `actions` array, once before `_outsideClickHandler`). ES5 `var` hoisting makes the second declaration a no-op, so this is not a bug, but it triggers linter redeclaration warnings and is dead code. --- [MINOR] static/js/controllers/author_manage_controller.js:openMenu — redundant `menu.dataset.anchorTop/anchorLeft` assignments Lines `menu.dataset.anchorTop = ...` and `menu.dataset.anchorLeft = ...` are set but never read — the CSS uses `var(--menu-top)` / `var(--menu-left)` from `style.setProperty(...)` only. The dataset values are dead code. --- [MINOR] e2e/browser/journey_author_manage_test.go (last line) — `_ = time.Second` import workaround `time` is imported but only used indirectly via package-level helpers. The `_ = time.Second` blank assignment is a workaround to prevent goimports from dropping the import. Either use `time` directly (e.g. in a `Timeout` call) or remove the import and the blank. --- [MINOR] static/js/controllers/author_manage_controller.js:_renderMergeSuggestions — no "no results" feedback When the typeahead fetch returns an empty author list (or all results are filtered out), the suggestion dropdown stays hidden with no user-visible indicator. The user types a name, gets no response, and has no feedback that the search found nothing. A "No authors found" `<li>` item would close the UX gap. --- REVIEW VERDICT: 1 major, 4 minor

zombor added 1 commit 2026-07-10 14:23:20 +00:00

fix(authors): apply review-round-5 findings ... a120db41be

[MAJOR] MergeAuthors target-existence guard: SELECT 1 FROM author WHERE
id=targetID at top of tx; return ErrNotFound (404 in MergeHandler) when
target is absent — prevents FK violations and ghost data.

[MINOR-sec] isDuplicateKey: replace strings.Contains("1062") with typed
errors.As(*mysql.MySQLError) && Number==1062 to avoid false-positives on
error messages that happen to contain "1062".

[MINOR-sec] RenameAuthor same-name no-op: 0 rows affected by UPDATE no
longer unconditionally returns ErrNotFound; follows up with authorExists
to distinguish "author missing" (→ ErrNotFound) from "name unchanged"
(→ no-op 204).

[MINOR] openMenu: remove duplicate `var ctrl = this` declaration and dead
menu.dataset.anchorTop/Left writes (CSS uses --menu-top/--menu-left
custom properties, not dataset attributes).

[MINOR] _renderMergeSuggestions: show "No authors found" li item instead
of hiding dropdown when filtered list is empty.

[MINOR] e2e: remove `_ = time.Second` import-preservation hack — time is
unused in journey_author_manage_test.go.

100% Go coverage + 3735 JS tests green.

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

forgejo-actions commented 2026-07-10 14:29:13 +00:00

Copy link

Author management screenshot (rename-modal)

Author rename/merge/delete — kebab menu and modal on /authors page

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

forgejo-actions commented 2026-07-10 14:29:34 +00:00

Copy link

Author management screenshot (rename-submitted)

Author rename/merge/delete — kebab menu and modal on /authors page

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

forgejo-actions commented 2026-07-10 14:29:48 +00:00

Copy link

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

Author rename/merge/delete — kebab menu and modal on /authors page

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

forgejo-actions commented 2026-07-10 14:29:57 +00:00

Copy link

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

Author rename/merge/delete — kebab menu and modal on /authors page

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

zombor force-pushed bd-bookshelf-vgbmo.1 from a120db41be to 96ef90b8aa 2026-07-10 14:44:30 +00:00 Compare

forgejo-actions commented 2026-07-10 14:47:05 +00:00

Copy link

Author management screenshot (rename-modal)

Author rename/merge/delete — kebab menu and modal on /authors page

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

forgejo-actions commented 2026-07-10 14:47:14 +00:00

Copy link

Author management screenshot (rename-submitted)

Author rename/merge/delete — kebab menu and modal on /authors page

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

forgejo-actions commented 2026-07-10 14:47:25 +00:00

Copy link

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

Author rename/merge/delete — kebab menu and modal on /authors page

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

forgejo-actions commented 2026-07-10 14:47:28 +00:00

Copy link

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

Author rename/merge/delete — kebab menu and modal on /authors page

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

zombor merged commit 417a34412c into main 2026-07-10 14:52:10 +00:00

zombor referenced this pull request from a commit 2026-07-10 14:52:20 +00:00

Merge pull request 'feat(authors): rename/merge/delete management [shot:author-manage] (bookshelf-vgbmo.1)' (#1072) from bd-bookshelf-vgbmo.1 into main

zombor referenced this pull request 2026-07-10 23:42:51 +00:00

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

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

2 participants

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!1072

No description provided.