feat(books): select-all-visible + deselect-all controls in action bar (bookshelf-pts) #205

Merged

zombor merged 1 commit from bd-bookshelf-pts into main 2026-05-30 23:55:52 +00:00

Conversation 2 Commits 1 Files changed 3 +132

zombor commented 2026-05-30 04:14:17 +00:00

Owner

Copy link

Summary

• Adds a Select all visible (✓) button to the bulk-action bar that selects every book on the current page without clearing cross-page selections
• Adds a selectAllVisible() action to books_select_controller.js that iterates rowCheckboxTargets, adds each ID+title to the set, persists to sessionStorage, and re-renders
• Confirms the existing Clear selection (✕) button already satisfies the "Deselect all" requirement — verified it clears the full cross-page selection set
• Adds e2e browser tests covering table view, grid view, cross-page accumulation, and full select→deselect round-trip

Test plan

• Table view: seed 3 books, select 1, click ✓ → count shows 3
• Grid view: seed 3 books, select 1, click ✓ → count shows 3
• Clear: after select-all, click ✕ → bar hides
• Cross-page: select 1 on page 1, navigate to page 2, click ✓ → count shows 3 (1 + 2 on page 2)
• e2e/browser/books_select_all_test.go covers all the above

Closes bead bookshelf-pts on merge.

## Summary - Adds a **Select all visible** (✓) button to the bulk-action bar that selects every book on the current page without clearing cross-page selections - Adds a `selectAllVisible()` action to `books_select_controller.js` that iterates `rowCheckboxTargets`, adds each ID+title to the set, persists to sessionStorage, and re-renders - Confirms the existing **Clear selection** (✕) button already satisfies the "Deselect all" requirement — verified it clears the full cross-page selection set - Adds e2e browser tests covering table view, grid view, cross-page accumulation, and full select→deselect round-trip ## Test plan - [ ] Table view: seed 3 books, select 1, click ✓ → count shows 3 - [ ] Grid view: seed 3 books, select 1, click ✓ → count shows 3 - [ ] Clear: after select-all, click ✕ → bar hides - [ ] Cross-page: select 1 on page 1, navigate to page 2, click ✓ → count shows 3 (1 + 2 on page 2) - [ ] `e2e/browser/books_select_all_test.go` covers all the above Closes bead bookshelf-pts on merge.

zombor added 1 commit 2026-05-30 04:14:17 +00:00

feat(books): select-all-visible + deselect-all controls in action bar (bookshelf-pts) ... dc4220e42b

Add a "Select all visible" (✓) button to the bulk-action bar that selects
every book on the current page while preserving cross-page selections.
The existing "Clear selection" (✕) button already satisfies "Deselect all"
requirements; aria-label and title tooltip confirm its purpose.

Add e2e browser tests covering both table and grid views, the cross-page
accumulation behaviour, and the full select→deselect round-trip.

Closes bead bookshelf-pts on merge.

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

zombor commented 2026-05-30 23:40:00 +00:00

Author

Owner

Copy link

Security Review — bookshelf-pts (select-all-visible)

Scope: template + JS only (3 files, no new endpoints). Confirmed.

---

XSS — book title flow

Server → template: {{.Title}} in data-book-title="{{.Title}}" is Go's html/template, which HTML-attribute-escapes the value at render time. Safe.

data attribute → JS: cb.dataset.bookTitle reads the (already-escaped) string. It is written to this._titles[id] (a plain JS object) and serialised into sessionStorage via JSON.stringify. Never passed to innerHTML, insertAdjacentHTML, or a template literal that builds markup.

sessionStorage → DOM: The only write-back to the DOM is countLabelTarget.textContent = selected + " selected" — a numeric count, not the title. Titles stored in sessionStorage are not rendered back to HTML anywhere in this diff.

No XSS path introduced.

---

CSP

New template lines (books_index.html): a single <button> with data-action="click->books-select#selectAllVisible". No inline <script>, no onXxx attributes, no javascript: URIs. Clean.

---

sessionStorage DoS

Titles are appended individually during selectAllVisible (and the existing toggle/restore paths). All items on the current visible page are added in one call. The existing _saveToStorage already catches QuotaExceededError and degrades gracefully. The new method calls _saveToStorage() the same way — no new unbounded write path. No change to risk posture.

---

localStorage

No localStorage usage introduced. Only sessionStorage (pre-existing pattern).

---

Template change

The added <button> emits a ✓ literal character (U+2713). This is a text node, not markup, so no escaping concern. The aria-label and title attributes are static strings.

---

REVIEW VERDICT: 0 blocker, 0 major, 0 minor

## Security Review — bookshelf-pts (select-all-visible) **Scope:** template + JS only (3 files, no new endpoints). Confirmed. --- ### XSS — book title flow **Server → template:** `{{.Title}}` in `data-book-title="{{.Title}}"` is Go's `html/template`, which HTML-attribute-escapes the value at render time. Safe. **data attribute → JS:** `cb.dataset.bookTitle` reads the (already-escaped) string. It is written to `this._titles[id]` (a plain JS object) and serialised into sessionStorage via `JSON.stringify`. Never passed to `innerHTML`, `insertAdjacentHTML`, or a template literal that builds markup. **sessionStorage → DOM:** The only write-back to the DOM is `countLabelTarget.textContent = selected + " selected"` — a numeric count, not the title. Titles stored in sessionStorage are not rendered back to HTML anywhere in this diff. No XSS path introduced. --- ### CSP New template lines (`books_index.html`): a single `<button>` with `data-action="click->books-select#selectAllVisible"`. No inline `<script>`, no `onXxx` attributes, no `javascript:` URIs. Clean. --- ### sessionStorage DoS Titles are appended individually during `selectAllVisible` (and the existing toggle/restore paths). All items on the current visible page are added in one call. The existing `_saveToStorage` already catches `QuotaExceededError` and degrades gracefully. The new method calls `_saveToStorage()` the same way — no new unbounded write path. No change to risk posture. --- ### localStorage No `localStorage` usage introduced. Only `sessionStorage` (pre-existing pattern). --- ### Template change The added `<button>` emits a ✓ literal character (U+2713). This is a text node, not markup, so no escaping concern. The `aria-label` and `title` attributes are static strings. --- REVIEW VERDICT: 0 blocker, 0 major, 0 minor

zombor commented 2026-05-30 23:40:24 +00:00

Author

Owner

Copy link

CODE REVIEW: APPROVED

Phase 0: DEMO Verification

No explicit DEMO block in the bead. The bead's deliverable #4 is 'e2e browser tests confirming selection state across page navigation' — CI is green at SHA dc4220e42bd61834ccc6e94532fdc1cd88265001, meaning the 5 e2e specs in e2e/browser/books_select_all_test.go ran and passed in CI (ubuntu-latest, MySQL service container). This is accepted as equivalent verification.

Phase 1: Spec Compliance

Bead deliverables checked against diff:

1. Two new buttons — 'Select all visible' button added. The 'deselect-all/clear' button was already present on main before this PR (books-select-bar-clear at templates/pages/books_index.html:305 on main). Net: one new button. Acceptable — the deselect-all capability was already delivered.
2. Select-all scope: current page — implemented via selectAllVisible() iterating rowCheckboxTargets. Correct scope.
3. 'Select all N matching' secondary affordance — not implemented. The bead description scopes this as optional ('If user wants…, add a secondary affordance'). Not a blocker.
4. Cross-page accumulation — selectAllVisible() calls this._ids.add(id) (set union, not replace) and the e2e test 'select-all-visible adds visible items without clearing cross-page selections' verifies this. Correct.
5. CSP-safe — button uses data-action attribute wiring only, no inline JS. Consistent with strict script-src 'self' policy in internal/middleware/security_headers.go:23.
6. Tests — 5 e2e browser specs, CI green.

Phase 2: Code Quality

No BLOCKER or MAJOR findings.

[MINOR] static/js/controllers/books_select_controller.js — selectAllVisible() uses function(cb) {...}, this callback style while the rest of the controller uses arrow functions. Works correctly (forEach thisArg binding is valid), but inconsistent with the surrounding style.

[MINOR] templates/pages/books_index.html — the new button uses a unicode checkmark (✓) as its visible label. The pre-existing clear button also uses unicode (✕). These are visually consistent with each other; minor nit only.

XSS check: data-book-title={{.Title}} is rendered by html/template which HTML-attribute-escapes the value. The title is read back via cb.dataset.bookTitle into this._titles (a plain JS object stored as JSON in sessionStorage) and only surfaced via countLabelTarget.textContent (line 299). No innerHTML anywhere in the controller. No XSS vector.

Aria: aria-label='Select all visible books' present on the new button. Good.

Test structure: Each It block has exactly one Expect. Consistent with project conventions.

REVIEW VERDICT: 0 blocker, 0 major, 2 minor

CODE REVIEW: APPROVED ## Phase 0: DEMO Verification No explicit DEMO block in the bead. The bead's deliverable #4 is 'e2e browser tests confirming selection state across page navigation' — CI is green at SHA `dc4220e42bd61834ccc6e94532fdc1cd88265001`, meaning the 5 e2e specs in `e2e/browser/books_select_all_test.go` ran and passed in CI (ubuntu-latest, MySQL service container). This is accepted as equivalent verification. ## Phase 1: Spec Compliance Bead deliverables checked against diff: 1. **Two new buttons** — 'Select all visible' button added. The 'deselect-all/clear' button was already present on main before this PR (books-select-bar-clear at templates/pages/books_index.html:305 on main). Net: one new button. Acceptable — the deselect-all capability was already delivered. 2. **Select-all scope: current page** — implemented via selectAllVisible() iterating rowCheckboxTargets. Correct scope. 3. **'Select all N matching' secondary affordance** — not implemented. The bead description scopes this as optional ('If user wants…, add a secondary affordance'). Not a blocker. 4. **Cross-page accumulation** — selectAllVisible() calls this._ids.add(id) (set union, not replace) and the e2e test 'select-all-visible adds visible items without clearing cross-page selections' verifies this. Correct. 5. **CSP-safe** — button uses data-action attribute wiring only, no inline JS. Consistent with strict script-src 'self' policy in internal/middleware/security_headers.go:23. 6. **Tests** — 5 e2e browser specs, CI green. ## Phase 2: Code Quality No BLOCKER or MAJOR findings. [MINOR] static/js/controllers/books_select_controller.js — selectAllVisible() uses function(cb) {...}, this callback style while the rest of the controller uses arrow functions. Works correctly (forEach thisArg binding is valid), but inconsistent with the surrounding style. [MINOR] templates/pages/books_index.html — the new button uses a unicode checkmark (✓) as its visible label. The pre-existing clear button also uses unicode (✕). These are visually consistent with each other; minor nit only. **XSS check:** data-book-title={{.Title}} is rendered by html/template which HTML-attribute-escapes the value. The title is read back via cb.dataset.bookTitle into this._titles (a plain JS object stored as JSON in sessionStorage) and only surfaced via countLabelTarget.textContent (line 299). No innerHTML anywhere in the controller. No XSS vector. **Aria:** aria-label='Select all visible books' present on the new button. Good. **Test structure:** Each It block has exactly one Expect. Consistent with project conventions. REVIEW VERDICT: 0 blocker, 0 major, 2 minor

zombor force-pushed bd-bookshelf-pts from dc4220e42b to 685dfc0a21 2026-05-30 23:44:59 +00:00 Compare

zombor merged commit e1fdf17e11 into main 2026-05-30 23:55:52 +00:00

zombor referenced this pull request from a commit 2026-05-30 23:55:53 +00:00

Merge pull request 'feat(books): select-all-visible + deselect-all controls in action bar (bookshelf-pts)' (#205) from bd-bookshelf-pts 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!205

No description provided.