feat(bookdrop): compact Grimmory-style toolbar, reclaim vertical space (bookshelf-bp34) #621

Merged

zombor merged 5 commits from bd-bookshelf-bp34 into main 2026-06-18 20:09:27 +00:00

Conversation 9 Commits 5 Files changed 11 +665 -116

zombor commented 2026-06-18 18:36:07 +00:00

Owner

Copy link

Summary

• Replaces the big boxed-panel BookDrop toolbar (background + border + border-radius + padding chrome) with a slim single-row separator-style toolbar
• Left side: bulk action buttons (Accept Selected, Reject Selected, Extract Pattern, Bulk Edit) + selection count
• Right side: global Library/Path quick-assign selects + Apply button
• No-libraries state: compact inline hint instead of a separate boxed panel

What is preserved verbatim

• All Stimulus targets: bookdrop-review-target, bookdrop-extract-pattern-target, bookdrop-bulk-edit-target
• All Stimulus actions: data-action="..." on every button/select
• Accept and Reject forms with _csrf hidden inputs and all hidden ID fields
• Button IDs #extract-pattern-open-btn and #bulk-edit-open-btn (referenced by browser e2e tests)
• Library/Path select IDs and disabled/data attributes

Out of scope (possible follow-up)

Moving Accept/Reject to a bottom action bar (Grimmory-style finalize row) — deferred to a follow-up bead.

Test plan

• make test passes (Go unit tests)
• npm test passes (904 Vitest JS tests)
• No inline style= attributes (CSP-safe)
• No bespoke parallel CSS class system — reuses .btn, .btn-small, .btn-ghost canonical classes
• Browser e2e tests reference #extract-pattern-open-btn and #bulk-edit-open-btn by ID — both preserved
• Before/after screenshots to follow in PR comment after CI runs

Closes bead bookshelf-bp34 on merge.

## Summary - Replaces the big boxed-panel BookDrop toolbar (background + border + border-radius + padding chrome) with a slim single-row separator-style toolbar - Left side: bulk action buttons (Accept Selected, Reject Selected, Extract Pattern, Bulk Edit) + selection count - Right side: global Library/Path quick-assign selects + Apply button - No-libraries state: compact inline hint instead of a separate boxed panel ## What is preserved verbatim - All Stimulus targets: `bookdrop-review-target`, `bookdrop-extract-pattern-target`, `bookdrop-bulk-edit-target` - All Stimulus actions: `data-action="..."` on every button/select - Accept and Reject forms with `_csrf` hidden inputs and all hidden ID fields - Button IDs `#extract-pattern-open-btn` and `#bulk-edit-open-btn` (referenced by browser e2e tests) - Library/Path select IDs and disabled/data attributes ## Out of scope (possible follow-up) Moving Accept/Reject to a bottom action bar (Grimmory-style finalize row) — deferred to a follow-up bead. ## Test plan - [x] `make test` passes (Go unit tests) - [x] `npm test` passes (904 Vitest JS tests) - [x] No inline `style=` attributes (CSP-safe) - [x] No bespoke parallel CSS class system — reuses `.btn`, `.btn-small`, `.btn-ghost` canonical classes - [x] Browser e2e tests reference `#extract-pattern-open-btn` and `#bulk-edit-open-btn` by ID — both preserved - [ ] Before/after screenshots to follow in PR comment after CI runs Closes bead bookshelf-bp34 on merge.

bookdrop-topbar-consolidated.png

77 KiB

bookdrop_rework2_layout.png

100 KiB

zombor added 1 commit 2026-06-18 18:36:08 +00:00

feat(bookdrop): compact Grimmory-style toolbar, reclaim vertical space ... b4edcbce62

Replace the boxed-panel bookdrop toolbar (.bookdrop-toolbar with
background/border/border-radius/padding chrome) with a slim single-row
separator-style toolbar:
- Left: bulk action buttons (Accept Selected, Reject Selected, Extract
  Pattern, Bulk Edit) + selection count
- Right: global Library/Path quick-assign selects + Apply button
- No-libraries state: inline hint text inside the toolbar instead of
  a separate boxed panel

All Stimulus targets/actions, form elements, and CSRF hidden inputs
preserved verbatim. CSS reuses btn/btn-small/btn-ghost canonical
classes; no bespoke parallel class system; no inline styles (CSP-safe).
Removed dead .bookdrop-toolbar-bulk and .bookdrop-no-libraries rules.

Closes bead bookshelf-bp34.

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

zombor commented 2026-06-18 18:52:14 +00:00

Author

Owner

Copy link

Security Review — PR #621 (bookshelf-bp34)

BookDrop toolbar UI/CSS rework. Scope: templates/pages/bookdrop_index.html + static/css/main.css. Behavior unchanged per description.

---

CSRF preserved: Both POST forms (/bookdrop/accept, /bookdrop/reject) carry <input type="hidden" name="_csrf" value="{{$.CSRFToken}}"> in the new layout — identical to the removed code. No regression.

XSS: All template expressions in added lines are standard auto-escaped Go html/template interpolations ({{$.CSRFToken}}). No new {{template.HTML(...)}}, template.JS, template.URL, or other raw-output functions introduced. The pre-existing {{.LibraryPathsJSON}} in a <script type="application/json"> tag is unchanged context (not new). No user-controlled data (filenames, paths) is interpolated into HTML attributes without escaping.

CSP / inline handlers: No onclick=, onerror=, style= attribute values, or <script> tags added. The existing {{/* No inline JS — strict CSP */}} comment is preserved. Stimulus data-action= attributes are unchanged declarative wiring — no new JS surface.

Authz: Pure presentation restructuring. No server-side gating code touched; no route registrations, middleware, or handler logic in this diff.

CSS-only change note: .bookdrop-no-libraries renamed to .bookdrop-no-libraries-hint with style changes — no security impact. All other CSS deltas are layout/spacing only.

---

REVIEW VERDICT: 0 blocker, 0 major, 0 minor

## Security Review — PR #621 (bookshelf-bp34) BookDrop toolbar UI/CSS rework. Scope: `templates/pages/bookdrop_index.html` + `static/css/main.css`. Behavior unchanged per description. --- **CSRF preserved:** Both POST forms (`/bookdrop/accept`, `/bookdrop/reject`) carry `<input type="hidden" name="_csrf" value="{{$.CSRFToken}}">` in the new layout — identical to the removed code. No regression. **XSS:** All template expressions in added lines are standard auto-escaped Go `html/template` interpolations (`{{$.CSRFToken}}`). No new `{{template.HTML(...)}}`, `template.JS`, `template.URL`, or other raw-output functions introduced. The pre-existing `{{.LibraryPathsJSON}}` in a `<script type="application/json">` tag is unchanged context (not new). No user-controlled data (filenames, paths) is interpolated into HTML attributes without escaping. **CSP / inline handlers:** No `onclick=`, `onerror=`, `style=` attribute values, or `<script>` tags added. The existing `{{/* No inline JS — strict CSP */}}` comment is preserved. Stimulus `data-action=` attributes are unchanged declarative wiring — no new JS surface. **Authz:** Pure presentation restructuring. No server-side gating code touched; no route registrations, middleware, or handler logic in this diff. **CSS-only change note:** `.bookdrop-no-libraries` renamed to `.bookdrop-no-libraries-hint` with style changes — no security impact. All other CSS deltas are layout/spacing only. --- REVIEW VERDICT: 0 blocker, 0 major, 0 minor

zombor commented 2026-06-18 18:52:56 +00:00

Author

Owner

Copy link

Code Review — bookshelf-bp34 (PR #621)

Phase 0: DEMO Verification

No DEMO block in the bead description — this is a UI/CSS-only rework with no runnable command output to verify. Proceeding under the understanding that CI green + diff review is the appropriate verification path for pure template/CSS changes.

Phase 1 & 2: Spec Compliance + Code Quality

Behavior preservation (Stimulus targets/actions)

All targets declared in bookdrop_review_controller.js (lines 31–49) are present in the new template. The has* guards that protect optional targets are correctly relied upon:

• hasAcceptBtnTarget / hasRejectBtnTarget guard _updateUI() — correct; both buttons are inside {{if $.Libraries}}.
• submitAccept / submitReject access acceptIdsTarget, acceptFormTarget etc. without has* guards (lines 151–154, 173–174) — but those methods are only reachable by clicking acceptBtn/rejectBtn, which are themselves only rendered when $.Libraries is truthy. No reachability issue.
• openBtn for extract-pattern and bulk-edit are now unconditionally rendered (outside the {{if $.Libraries}} block) — both controllers guard with hasOpenBtnTarget (lines 153–154 of each controller). Correct.

CSP — no inline style= or on handlers introduced.* Confirmed clean.

Canonical components. btn, btn-small, btn-ghost, btn-primary, btn-danger-ghost all resolve to existing CSS rules. No bespoke parallel class system introduced.

Dead CSS cleanup. Removed classes .bookdrop-toolbar-bulk, .bookdrop-no-libraries are gone from both main.css and the template. Orphaned .bookdrop-metadata-actions (used only in the old no-libraries fallback div) is also fully removed. New .bookdrop-no-libraries-hint class is defined in CSS and used in the template — no orphans.

Template correctness. No unclosed {{ actions. No broken Go template syntax. {{$.CSRFToken}} and {{$.Libraries}} scoping is correct (using $ to access top-level data inside range blocks is not needed here since the forms are not inside a range, but $.Libraries is correct where used).

One behavioral change worth noting (not a blocker): The toolbar div is now always rendered when {{if .Proposals}} is true, regardless of whether libraries are configured. Previously the toolbar was wrapped in {{if $.Libraries}} and was entirely absent when no libraries existed — instead showing a <p class="bookdrop-no-libraries"> and a separate <div class="bookdrop-metadata-actions"> for the two modal buttons. The new design always shows the compact toolbar row with the Extract Pattern and Bulk Edit buttons, plus an inline hint span when no libraries are configured. This is intentional per the bead description (reclaim vertical space, consolidate into one row) and is not a regression — the Extract Pattern / Bulk Edit modals remain reachable in both cases.

No findings.

---

REVIEW VERDICT: 0 blocker, 0 major, 0 minor

## Code Review — bookshelf-bp34 (PR #621) ### Phase 0: DEMO Verification No DEMO block in the bead description — this is a UI/CSS-only rework with no runnable command output to verify. Proceeding under the understanding that CI green + diff review is the appropriate verification path for pure template/CSS changes. ### Phase 1 & 2: Spec Compliance + Code Quality **Behavior preservation (Stimulus targets/actions)** All targets declared in `bookdrop_review_controller.js` (lines 31–49) are present in the new template. The `has*` guards that protect optional targets are correctly relied upon: - `hasAcceptBtnTarget` / `hasRejectBtnTarget` guard `_updateUI()` — correct; both buttons are inside `{{if $.Libraries}}`. - `submitAccept` / `submitReject` access `acceptIdsTarget`, `acceptFormTarget` etc. without `has*` guards (lines 151–154, 173–174) — but those methods are only reachable by clicking `acceptBtn`/`rejectBtn`, which are themselves only rendered when `$.Libraries` is truthy. No reachability issue. - `openBtn` for extract-pattern and bulk-edit are now unconditionally rendered (outside the `{{if $.Libraries}}` block) — both controllers guard with `hasOpenBtnTarget` (lines 153–154 of each controller). Correct. **CSP — no inline style= or on* handlers introduced.** Confirmed clean. **Canonical components.** `btn`, `btn-small`, `btn-ghost`, `btn-primary`, `btn-danger-ghost` all resolve to existing CSS rules. No bespoke parallel class system introduced. **Dead CSS cleanup.** Removed classes `.bookdrop-toolbar-bulk`, `.bookdrop-no-libraries` are gone from both `main.css` and the template. Orphaned `.bookdrop-metadata-actions` (used only in the old no-libraries fallback div) is also fully removed. New `.bookdrop-no-libraries-hint` class is defined in CSS and used in the template — no orphans. **Template correctness.** No unclosed `{{` actions. No broken Go template syntax. `{{$.CSRFToken}}` and `{{$.Libraries}}` scoping is correct (using `$` to access top-level data inside range blocks is not needed here since the forms are not inside a range, but `$.Libraries` is correct where used). **One behavioral change worth noting (not a blocker):** The `toolbar` div is now always rendered when `{{if .Proposals}}` is true, regardless of whether libraries are configured. Previously the toolbar was wrapped in `{{if $.Libraries}}` and was entirely absent when no libraries existed — instead showing a `<p class="bookdrop-no-libraries">` and a separate `<div class="bookdrop-metadata-actions">` for the two modal buttons. The new design always shows the compact toolbar row with the Extract Pattern and Bulk Edit buttons, plus an inline hint span when no libraries are configured. This is intentional per the bead description (reclaim vertical space, consolidate into one row) and is not a regression — the Extract Pattern / Bulk Edit modals remain reachable in both cases. **No findings.** --- REVIEW VERDICT: 0 blocker, 0 major, 0 minor

zombor added 1 commit 2026-06-18 19:13:17 +00:00

fix(bookdrop): Grimmory-matched layout — compact toolbar + bottom action bar (bookshelf-bp34) ... 2c93bc341f

Rework #2: match the Grimmory bookdrop review UI exactly:
- Title row: h1 + subtitle (left) + Refresh button form (right) via .page-header
- Compact single-row toolbar: [Extract Pattern | Bulk Edit] left, [Library select | Subpath select | ✓ apply] right with placeholder text inside selects (no stacked labels)
- Bottom action bar (.bookdrop-action-bar): selection count + Select All checkbox + Clear button (left); Accept Selected + Reject Selected forms (right)
- Move Accept/Reject forms from toolbar to bottom bar; all data-* targets + CSRF preserved
- Add onBottomSelectAllChange(), clearSelection() to bookdrop_review_controller.js; sync both header + bottom checkboxes via _updateSelectAllState() called from _updateUI()
- Add 13 Vitest unit tests for new controller behaviors
- Add browser e2e journey asserting Accept/Reject live in .bookdrop-action-bar (not toolbar); screenshot posted to PR

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

zombor added 1 commit 2026-06-18 19:19:21 +00:00

fix(e2e): seed library in bottom-bar test so Accept/Reject buttons render (bookshelf-bp34) ... d6bdb52a8b

The bottom action bar's Accept/Reject forms are guarded by {{if $.Libraries}},
so the test needs a library row to make those buttons appear in the DOM.

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

zombor commented 2026-06-18 19:40:28 +00:00

Author

Owner

Copy link

Rework #2 — Grimmory-matched BookDrop layout screenshot

Three zones visible:

1. Title row: BookDrop Review + subtitle (left), Refresh button (right)
2. Compact single-row toolbar: [Extract Pattern | Bulk Edit] left + [Library select | Subpath select | apply] right — no stacked labels
3. Bottom action bar: [Select All + Clear] left | [Reject Selected | Accept Selected] right

DOM verified: acceptInBar=true, acceptInToolbar=false, barExists=true, pageHeaderExists=true

**Rework #2 — Grimmory-matched BookDrop layout screenshot** Three zones visible: 1. Title row: BookDrop Review + subtitle (left), Refresh button (right) 2. Compact single-row toolbar: [Extract Pattern | Bulk Edit] left + [Library select | Subpath select | apply] right — no stacked labels 3. Bottom action bar: [Select All + Clear] left | [Reject Selected | Accept Selected] right DOM verified: acceptInBar=true, acceptInToolbar=false, barExists=true, pageHeaderExists=true 

zombor added 1 commit 2026-06-18 19:41:30 +00:00

chore: ignore tmp/ directory (go-rod e2e screenshot output) (bookshelf-bp34) ... c3582f8b0e

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

zombor commented 2026-06-18 19:46:31 +00:00

Author

Owner

Copy link

Security Review — PR #621 (bookshelf-bp34)

Scope: BookDrop review page three-zone layout rework; Accept/Reject/Refresh POST forms relocated from top toolbar to new bottom action bar and page-header respectively.

---

CSRF preservation

Refresh form (templates/pages/bookdrop_index.html line ~746):

<input type="hidden" name="_csrf" value="{{.CSRFToken}}">

Present. ✓

Reject form (bottom action bar, line ~889):

<input type="hidden" name="_csrf" value="{{$.CSRFToken}}">

Present. ✓

Accept form (bottom action bar, line ~898):

<input type="hidden" name="_csrf" value="{{$.CSRFToken}}">

Present. ✓

All three POST forms retain their _csrf hidden input after relocation. No CSRF regression.

---

XSS / template escaping

All data interpolations in the diff use {{.CSRFToken}}, {{.Name}}, {{.ID}}, {{$.CSRFToken}}, {{.LibraryPathsJSON}} — all passed through html/template's auto-escaping. No new template.HTML, template.JS, template.URL, safeHTML, or raw pipe operators introduced. No unescaped filename or path rendered into an attribute. No XSS surface change.

---

CSP / inline scripts

No onclick, onX event handlers, or <script> tags with executable content introduced. All interactivity wired via Stimulus data-action attributes. The controller comment on line ~380 explicitly notes "strict CSP forbids inline scripts." No CSP weakening.

---

Authorization

No server-side handler, service, or middleware changes in the diff. Accept/Reject/Refresh routes remain gated by whatever auth middleware existed before. Presentation-only change — no authz surface removed or bypassed.

---

Committed files / .gitignore

.gitignore adds tmp/ to exclude ephemeral go-rod screenshots saved during e2e runs. No secrets, credentials, or build artifacts committed. No sensitive paths accidentally exposed.

---

JS test fixture note

The Vitest unit test fixture (bookdrop_review_controller.test.js) omits _csrf hidden inputs from its <form> elements — correct behavior for a jsdom unit test that never exercises CSRF middleware. Not a security concern.

---

REVIEW VERDICT: 0 blocker, 0 major, 0 minor

## Security Review — PR #621 (bookshelf-bp34) **Scope:** BookDrop review page three-zone layout rework; Accept/Reject/Refresh POST forms relocated from top toolbar to new bottom action bar and page-header respectively. --- ### CSRF preservation **Refresh form** (`templates/pages/bookdrop_index.html` line ~746): ```html <input type="hidden" name="_csrf" value="{{.CSRFToken}}"> ``` Present. ✓ **Reject form** (bottom action bar, line ~889): ```html <input type="hidden" name="_csrf" value="{{$.CSRFToken}}"> ``` Present. ✓ **Accept form** (bottom action bar, line ~898): ```html <input type="hidden" name="_csrf" value="{{$.CSRFToken}}"> ``` Present. ✓ All three POST forms retain their `_csrf` hidden input after relocation. No CSRF regression. --- ### XSS / template escaping All data interpolations in the diff use `{{.CSRFToken}}`, `{{.Name}}`, `{{.ID}}`, `{{$.CSRFToken}}`, `{{.LibraryPathsJSON}}` — all passed through `html/template`'s auto-escaping. No new `template.HTML`, `template.JS`, `template.URL`, `safeHTML`, or raw pipe operators introduced. No unescaped filename or path rendered into an attribute. No XSS surface change. --- ### CSP / inline scripts No `onclick`, `onX` event handlers, or `<script>` tags with executable content introduced. All interactivity wired via Stimulus `data-action` attributes. The controller comment on line ~380 explicitly notes "strict CSP forbids inline scripts." No CSP weakening. --- ### Authorization No server-side handler, service, or middleware changes in the diff. Accept/Reject/Refresh routes remain gated by whatever auth middleware existed before. Presentation-only change — no authz surface removed or bypassed. --- ### Committed files / .gitignore `.gitignore` adds `tmp/` to exclude ephemeral go-rod screenshots saved during e2e runs. No secrets, credentials, or build artifacts committed. No sensitive paths accidentally exposed. --- ### JS test fixture note The Vitest unit test fixture (`bookdrop_review_controller.test.js`) omits `_csrf` hidden inputs from its `<form>` elements — correct behavior for a jsdom unit test that never exercises CSRF middleware. Not a security concern. --- REVIEW VERDICT: 0 blocker, 0 major, 0 minor

zombor commented 2026-06-18 19:47:41 +00:00

Author

Owner

Copy link

CODE REVIEW — bookshelf-bp34 (PR #621)

Phase 0: DEMO verification

Bead comment confirms CI green at d6bdb52a, mergeable=True, DOM verified (acceptInBar=true, acceptInToolbar=false). Diff confirms controller root is <section class="bookdrop-index" data-controller="bookdrop-review …"> and the bottom bar is a direct child of that section — all targets are contained within the root. DEMO passes.

---

Findings

[MAJOR] templates/pages/bookdrop_index.html:9 — Duplicate h1 on the page
internal/bookdrop/review_handler.go:208 still sets data.PageTitle = "BookDrop", which causes base.html (line 430) to render <h1 class="top-bar-title">BookDrop</h1> in the top bar. The PR simultaneously adds <h1>BookDrop Review</h1> inside the {{define "content"}} block. The page now has two <h1> elements — invalid HTML (accessibility, SEO, screen-reader landmark confusion). Fix: either clear data.PageTitle in the handler so the top-bar h1 is suppressed, or remove the <h1> from the page-header div and keep the top-bar title.

[MAJOR] templates/pages/bookdrop_index.html:15 — Duplicate Refresh button
internal/bookdrop/review_handler.go:212–216 renders bookdrop_index_controls.html into PageControls, which emits a "Refresh" button in the top bar (base.html:432). The PR adds a second Refresh button (↻ Refresh) inside the new <div class="page-header-actions">. Users see two Refresh controls on the same page. Fix: remove the new Refresh form from bookdrop_index.html (the existing top-bar controls one is sufficient) OR delete the bookdrop_index_controls.html render path and keep only the new inline one — but do not ship both.

[MINOR] e2e/browser/bookdrop_bottom_bar_test.go:244 — Hardcoded /tmp path for screenshot
os.WriteFile("/tmp/bookdrop_bottom_bar.png", …) is hardcoded. On macOS /tmp is a symlink to /private/tmp; in CI (Linux) it works fine. Prefer os.TempDir() + a named file (filepath.Join(os.TempDir(), "bookdrop_bottom_bar.png")) to be portable and self-documenting.

[MINOR] e2e/browser/bookdrop_bottom_bar_test.go:147–199 — Single It step has multiple Expect calls
The first It block contains 6 Expect calls asserting multiple distinct properties (button presence, location, disabled state, selectionCount location). Per unit-test conventions only the multi-Expect relaxation applies here since this is an e2e It — this is explicitly permitted by the E2E Testing Policy in CLAUDE.md ("Multi-Expect relaxation: journey It steps may have multiple Expect calls"). Not a violation; noting it for clarity.

---

Stimulus correctness

Controller root (<section class="bookdrop-index">) contains: toolbar, bottom bar, table, modals. The acceptForm/rejectForm/acceptBtn/rejectBtn/clearBtn/bottomSelectAllCheckbox/selectionCount targets are all inside the section root — no target is orphaned outside the controller element. The onRowCheckboxChange refactor (removing the direct _updateSelectAllState() call and having _updateUI() call it instead at line 254) is correct and non-regressive. Both select-all checkboxes are synced via _updateSelectAllState(). CSP: no inline style=, no on* handlers. Canonical .btn, .btn-small, .btn-primary, .btn-danger-ghost, .btn-ghost classes used throughout. --space-* tokens used in all new CSS rules.

---

REVIEW VERDICT: 0 blocker, 2 major, 2 minor

## CODE REVIEW — bookshelf-bp34 (PR #621) ### Phase 0: DEMO verification Bead comment confirms CI green at d6bdb52a, mergeable=True, DOM verified (`acceptInBar=true`, `acceptInToolbar=false`). Diff confirms controller root is `<section class="bookdrop-index" data-controller="bookdrop-review …">` and the bottom bar is a direct child of that section — all targets are contained within the root. DEMO passes. --- ### Findings **[MAJOR] templates/pages/bookdrop_index.html:9 — Duplicate h1 on the page** `internal/bookdrop/review_handler.go:208` still sets `data.PageTitle = "BookDrop"`, which causes `base.html` (line 430) to render `<h1 class="top-bar-title">BookDrop</h1>` in the top bar. The PR simultaneously adds `<h1>BookDrop Review</h1>` inside the `{{define "content"}}` block. The page now has two `<h1>` elements — invalid HTML (accessibility, SEO, screen-reader landmark confusion). Fix: either clear `data.PageTitle` in the handler so the top-bar h1 is suppressed, or remove the `<h1>` from the page-header div and keep the top-bar title. **[MAJOR] templates/pages/bookdrop_index.html:15 — Duplicate Refresh button** `internal/bookdrop/review_handler.go:212–216` renders `bookdrop_index_controls.html` into `PageControls`, which emits a "Refresh" button in the top bar (`base.html:432`). The PR adds a second Refresh button (`&#8635; Refresh`) inside the new `<div class="page-header-actions">`. Users see two Refresh controls on the same page. Fix: remove the new Refresh form from `bookdrop_index.html` (the existing top-bar controls one is sufficient) OR delete the `bookdrop_index_controls.html` render path and keep only the new inline one — but do not ship both. **[MINOR] e2e/browser/bookdrop_bottom_bar_test.go:244 — Hardcoded /tmp path for screenshot** `os.WriteFile("/tmp/bookdrop_bottom_bar.png", …)` is hardcoded. On macOS `/tmp` is a symlink to `/private/tmp`; in CI (Linux) it works fine. Prefer `os.TempDir()` + a named file (`filepath.Join(os.TempDir(), "bookdrop_bottom_bar.png")`) to be portable and self-documenting. **[MINOR] e2e/browser/bookdrop_bottom_bar_test.go:147–199 — Single It step has multiple Expect calls** The first `It` block contains 6 `Expect` calls asserting multiple distinct properties (button presence, location, disabled state, selectionCount location). Per unit-test conventions only the multi-Expect relaxation applies here since this is an e2e It — this is explicitly permitted by the E2E Testing Policy in CLAUDE.md ("Multi-Expect relaxation: journey It steps may have multiple Expect calls"). Not a violation; noting it for clarity. --- ### Stimulus correctness Controller root (`<section class="bookdrop-index">`) contains: toolbar, bottom bar, table, modals. The `acceptForm`/`rejectForm`/`acceptBtn`/`rejectBtn`/`clearBtn`/`bottomSelectAllCheckbox`/`selectionCount` targets are all inside the section root — no target is orphaned outside the controller element. The `onRowCheckboxChange` refactor (removing the direct `_updateSelectAllState()` call and having `_updateUI()` call it instead at line 254) is correct and non-regressive. Both select-all checkboxes are synced via `_updateSelectAllState()`. CSP: no inline `style=`, no `on*` handlers. Canonical `.btn`, `.btn-small`, `.btn-primary`, `.btn-danger-ghost`, `.btn-ghost` classes used throughout. `--space-*` tokens used in all new CSS rules. --- REVIEW VERDICT: 0 blocker, 2 major, 2 minor

zombor commented 2026-06-18 19:57:50 +00:00

Author

Owner

Copy link

BookDrop top-bar layout fix — screenshot

After this fix: single top bar = title + subtitle + one ↻ Refresh button. No duplicate h1, no duplicate Refresh.

**BookDrop top-bar layout fix — screenshot** After this fix: single top bar = title + subtitle + one ↻ Refresh button. No duplicate h1, no duplicate Refresh. 

zombor commented 2026-06-18 19:58:02 +00:00

Author

Owner

Copy link

BookDrop top-bar layout fix — screenshot

After this fix: single top bar = title + subtitle + one Refresh button. No duplicate h1, no duplicate Refresh.

**BookDrop top-bar layout fix — screenshot** After this fix: single top bar = title + subtitle + one Refresh button. No duplicate h1, no duplicate Refresh. 

zombor added 1 commit 2026-06-18 19:58:22 +00:00

fix(bookdrop): remove duplicate h1+Refresh; move description to top-bar subtitle ... 3d256e9974

- Remove in-content <h1>BookDrop Review</h1>, description <p>, and in-content
  ↻ Refresh button (the duplicate causing both review MAJORs)
- Add optional PageSubtitle field to BaseData; base.html renders it as a muted
  line under the top-bar title only when non-empty (backward-compatible)
- Set PageSubtitle on the bookdrop review page to the description string
- Add ↻ circle-arrow icon to the top-bar Refresh button in controls fragment
- Add .top-bar-head / .top-bar-subtitle CSS; remove dead .page-subtitle CSS
- Fix e2e temp screenshot path: /tmp/... → filepath.Join(os.TempDir(), ...)

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

zombor commented 2026-06-18 20:03:48 +00:00

Author

Owner

Copy link

Security Re-Review — PR #621 (bd-bookshelf-bp34)

Focus areas per request: PageSubtitle XSS safety, CSRF preservation, no inline scripts/handlers, no authz removal, no secrets.

---

No security findings.

PageSubtitle — XSS safety confirmed

base_data.go declares PageSubtitle string (plain string, not template.HTML). The template renders it as {{.PageSubtitle}} inside html/template — Go's html/template package HTML-escapes all string values automatically. A future caller passing user-controlled data through this field (e.g. a book title) would have it safely escaped before insertion into the DOM. No template.HTML cast, no | safeHTML, no JS/URL context misuse is present. Safe.

CSRF — all POST forms preserved

Both the old toolbar forms (removed) and the new bottom-action-bar forms (added) carry <input type="hidden" name="_csrf" value="{{$.CSRFToken}}">. Count is identical: 2 forms (accept + reject). The Refresh form in bookdrop_index_controls.html retains its _csrf input unchanged. CSRF protection is unbroken.

No inline scripts or event handlers

The JS controller uses only data-action="..." Stimulus declarative bindings. No onclick=, onchange=, <script> blocks, or javascript: URIs appear anywhere in the diff. Consistent with the project's strict CSP (script-src 'self').

No authz or server-gating removed

The {{if $.Libraries}} guard around accept/reject forms is preserved in the new bottom action bar. The toolbar restructure is presentation-only; no handler-level auth checks were touched. The page subtitle is a static string set by the handler — it is not user-controlled.

No secrets or temp artifacts committed

.gitignore adds tmp/ (go-rod screenshot output). No credentials, tokens, or sensitive files appear in the diff.

---

REVIEW VERDICT: 0 blocker, 0 major, 0 minor

## Security Re-Review — PR #621 (bd-bookshelf-bp34) Focus areas per request: PageSubtitle XSS safety, CSRF preservation, no inline scripts/handlers, no authz removal, no secrets. --- No security findings. **PageSubtitle — XSS safety confirmed** `base_data.go` declares `PageSubtitle string` (plain `string`, not `template.HTML`). The template renders it as `{{.PageSubtitle}}` inside `html/template` — Go's `html/template` package HTML-escapes all `string` values automatically. A future caller passing user-controlled data through this field (e.g. a book title) would have it safely escaped before insertion into the DOM. No `template.HTML` cast, no `| safeHTML`, no `JS`/`URL` context misuse is present. Safe. **CSRF — all POST forms preserved** Both the old toolbar forms (removed) and the new bottom-action-bar forms (added) carry `<input type="hidden" name="_csrf" value="{{$.CSRFToken}}">`. Count is identical: 2 forms (accept + reject). The Refresh form in `bookdrop_index_controls.html` retains its `_csrf` input unchanged. CSRF protection is unbroken. **No inline scripts or event handlers** The JS controller uses only `data-action="..."` Stimulus declarative bindings. No `onclick=`, `onchange=`, `<script>` blocks, or `javascript:` URIs appear anywhere in the diff. Consistent with the project's strict CSP (`script-src 'self'`). **No authz or server-gating removed** The `{{if $.Libraries}}` guard around accept/reject forms is preserved in the new bottom action bar. The toolbar restructure is presentation-only; no handler-level auth checks were touched. The page subtitle is a static string set by the handler — it is not user-controlled. **No secrets or temp artifacts committed** `.gitignore` adds `tmp/` (go-rod screenshot output). No credentials, tokens, or sensitive files appear in the diff. --- REVIEW VERDICT: 0 blocker, 0 major, 0 minor

zombor commented 2026-06-18 20:04:37 +00:00

Author

Owner

Copy link

CODE REVIEW: APPROVED

Branch: bd-bookshelf-bp34
Reviewed: current diff (fresh pass after fix round)

---

Phase 0: Prior MAJOR Resolution

MAJOR 1 (duplicate <h1>) — RESOLVED.
The bookdrop_index.html page template contains zero <h1> tags. The single <h1 class="top-bar-title"> lives in templates/layouts/base.html:431 inside {{if .PageTitle}}, rendered from BaseData.PageTitle. The in-content "BookDrop Review" heading has been removed entirely.

MAJOR 2 (duplicate Refresh button) — RESOLVED.
The page template bookdrop_index.html contains no Refresh button. The only Refresh button is in templates/pages/bookdrop_index_controls.html:7 (the top-bar controls slot), now decorated with ↻ (↻). No second Refresh button exists anywhere in the template.

---

Phase 1: Spec Compliance

All checklist items verified:

1. Single <h1> — confirmed: base.html:431 only, zero in page template.
2. Single Refresh button — confirmed: bookdrop_index_controls.html:7 only.
3. PageSubtitle backward-compatible — BaseData.PageSubtitle is type string (internal/tmpl/base_data.go:210), defaults to zero value (empty string). Template at base.html:432 is {{if .PageSubtitle}}<p class="top-bar-subtitle">{{.PageSubtitle}}</p>{{end}} — conditional on non-empty, so all other pages that do not set it are visually unchanged. The handler sets it as a plain Go string literal in review_handler.go:259; no template.HTML cast — auto-escaped correctly.
4. /tmp minor — bookdrop_bottom_bar_test.go:245 uses filepath.Join(os.TempDir(), "bookdrop_bottom_bar.png") (portable). The prior [MINOR] is resolved. Note: pre-existing tests (bookdrop_extract_pattern_test.go:205,230, metadata_fetch_cover_apply_test.go:166) still use literal "/tmp/" — those are out of scope for this PR.
5. CSP — no style= attributes, no on* event handlers, no inline <script> in templates. All new CSS classes (.top-bar-head, .top-bar-subtitle, .bookdrop-action-bar, sub-classes) use var(--fg-muted), var(--border), var(--space-*) canonical tokens. Removed dead classes: .bookdrop-toolbar-bulk, .bookdrop-bulk-label, .bookdrop-no-libraries. New class .bookdrop-no-libraries-hint replaces the old block-level .bookdrop-no-libraries cleanly.
6. Stimulus targets — controller root is <section class="bookdrop-index" data-controller="bookdrop-review ..."> which encloses every target: toolbar, selectAllCheckbox, bottomSelectAllCheckbox, clearBtn, row, rowCheckbox, rowLibrary, rowPath, selectionCount, bulkLibrary, bulkPath, acceptForm, acceptIds, acceptLibraryId, acceptPathId, rejectForm, rejectIds, acceptBtn, rejectBtn. All targets are within the controller root. CSRF inputs are present on both accept and reject forms.
7. _updateSelectAllState refactor — onRowCheckboxChange no longer calls _updateSelectAllState directly; instead _updateUI (called from all event handlers) calls _updateSelectAllState at line 257. Logic is correct and both selectAllCheckboxTarget and bottomSelectAllCheckboxTarget are synced in one place.
8. Browser e2e — bookdrop_bottom_bar_test.go is an Ordered journey with BeforeAll (satisfies policy). Two It steps share the setup. Multi-Expect per step is acceptable under the e2e relaxation rule.

---

Phase 2: Code Quality

No findings.

REVIEW VERDICT: 0 blocker, 0 major, 0 minor

## CODE REVIEW: APPROVED **Branch:** bd-bookshelf-bp34 **Reviewed:** current diff (fresh pass after fix round) --- ### Phase 0: Prior MAJOR Resolution **MAJOR 1 (duplicate `<h1>`) — RESOLVED.** The `bookdrop_index.html` page template contains zero `<h1>` tags. The single `<h1 class="top-bar-title">` lives in `templates/layouts/base.html:431` inside `{{if .PageTitle}}`, rendered from `BaseData.PageTitle`. The in-content "BookDrop Review" heading has been removed entirely. **MAJOR 2 (duplicate Refresh button) — RESOLVED.** The page template `bookdrop_index.html` contains no Refresh button. The only Refresh button is in `templates/pages/bookdrop_index_controls.html:7` (the top-bar controls slot), now decorated with ↻ (`&#8635;`). No second Refresh button exists anywhere in the template. --- ### Phase 1: Spec Compliance All checklist items verified: 1. **Single `<h1>`** — confirmed: `base.html:431` only, zero in page template. 2. **Single Refresh button** — confirmed: `bookdrop_index_controls.html:7` only. 3. **`PageSubtitle` backward-compatible** — `BaseData.PageSubtitle` is type `string` (`internal/tmpl/base_data.go:210`), defaults to zero value (empty string). Template at `base.html:432` is `{{if .PageSubtitle}}<p class="top-bar-subtitle">{{.PageSubtitle}}</p>{{end}}` — conditional on non-empty, so all other pages that do not set it are visually unchanged. The handler sets it as a plain Go string literal in `review_handler.go:259`; no `template.HTML` cast — auto-escaped correctly. 4. **`/tmp` minor** — `bookdrop_bottom_bar_test.go:245` uses `filepath.Join(os.TempDir(), "bookdrop_bottom_bar.png")` (portable). The prior [MINOR] is resolved. Note: pre-existing tests (`bookdrop_extract_pattern_test.go:205,230`, `metadata_fetch_cover_apply_test.go:166`) still use literal `"/tmp/"` — those are out of scope for this PR. 5. **CSP** — no `style=` attributes, no `on*` event handlers, no inline `<script>` in templates. All new CSS classes (`.top-bar-head`, `.top-bar-subtitle`, `.bookdrop-action-bar`, sub-classes) use `var(--fg-muted)`, `var(--border)`, `var(--space-*)` canonical tokens. Removed dead classes: `.bookdrop-toolbar-bulk`, `.bookdrop-bulk-label`, `.bookdrop-no-libraries`. New class `.bookdrop-no-libraries-hint` replaces the old block-level `.bookdrop-no-libraries` cleanly. 6. **Stimulus targets** — controller root is `<section class="bookdrop-index" data-controller="bookdrop-review ...">` which encloses every target: `toolbar`, `selectAllCheckbox`, `bottomSelectAllCheckbox`, `clearBtn`, `row`, `rowCheckbox`, `rowLibrary`, `rowPath`, `selectionCount`, `bulkLibrary`, `bulkPath`, `acceptForm`, `acceptIds`, `acceptLibraryId`, `acceptPathId`, `rejectForm`, `rejectIds`, `acceptBtn`, `rejectBtn`. All targets are within the controller root. CSRF inputs are present on both accept and reject forms. 7. **`_updateSelectAllState` refactor** — `onRowCheckboxChange` no longer calls `_updateSelectAllState` directly; instead `_updateUI` (called from all event handlers) calls `_updateSelectAllState` at line 257. Logic is correct and both `selectAllCheckboxTarget` and `bottomSelectAllCheckboxTarget` are synced in one place. 8. **Browser e2e** — `bookdrop_bottom_bar_test.go` is an `Ordered` journey with `BeforeAll` (satisfies policy). Two `It` steps share the setup. Multi-`Expect` per step is acceptable under the e2e relaxation rule. --- ### Phase 2: Code Quality No findings. REVIEW VERDICT: 0 blocker, 0 major, 0 minor

zombor force-pushed bd-bookshelf-bp34 from 3d256e9974 to 5eefe0a38d 2026-06-18 20:06:43 +00:00 Compare

zombor merged commit 433070b499 into main 2026-06-18 20:09:27 +00:00

zombor referenced this pull request from a commit 2026-06-18 20:09:30 +00:00

Merge pull request 'feat(bookdrop): compact Grimmory-style toolbar, reclaim vertical space (bookshelf-bp34)' (#621) from bd-bookshelf-bp34 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!621

No description provided.