feat(ui): collapsible sidebar section groups (bookshelf-7fc) #154

Merged

zombor merged 4 commits from bd-bookshelf-7fc into main 2026-05-28 18:21:14 +00:00

Conversation 2 Commits 4 Files changed 5 +357 -69

zombor commented 2026-05-28 16:33:51 +00:00

Owner

Copy link

Summary

• New Stimulus controller sidebar_section_controller.js — click label header collapses/expands the group; pressing Enter/Space also toggles (keyboard accessible)
• Collapsed state persisted per-user in localStorage key bookshelf:sidebar:collapsed (JSON array of group keys); no server round-trip; CSP-safe (no inline JS/styles)
• All five nav groups (HOME, LIBRARIES, SHELVES, INGEST, SYSTEM) wired with the controller, ARIA attributes (aria-expanded, aria-hidden, aria-controls), and tabindex=0 for focusability
• Clicking an inner <a> or <button> (e.g. Shelves +) does NOT toggle the section
• CSS: .sidebar-nav-group--collapsed hides the body; chevron rotates 90° when collapsed

Test plan

• make build passes
• make test passes (no Go changes affecting unit tests)
• E2E browser specs in e2e/browser/sidebar_section_test.go:
  • Click LIBRARIES header → body collapses
  • Reload page → collapsed state preserved from localStorage
  • Click Shelves + → section does NOT collapse

Closes bead bookshelf-7fc on merge.

## Summary - New Stimulus controller `sidebar_section_controller.js` — click label header collapses/expands the group; pressing Enter/Space also toggles (keyboard accessible) - Collapsed state persisted per-user in `localStorage` key `bookshelf:sidebar:collapsed` (JSON array of group keys); no server round-trip; CSP-safe (no inline JS/styles) - All five nav groups (HOME, LIBRARIES, SHELVES, INGEST, SYSTEM) wired with the controller, ARIA attributes (`aria-expanded`, `aria-hidden`, `aria-controls`), and `tabindex=0` for focusability - Clicking an inner `<a>` or `<button>` (e.g. Shelves `+`) does NOT toggle the section - CSS: `.sidebar-nav-group--collapsed` hides the body; chevron rotates 90° when collapsed ## Test plan - [ ] `make build` passes - [ ] `make test` passes (no Go changes affecting unit tests) - [ ] E2E browser specs in `e2e/browser/sidebar_section_test.go`: - Click LIBRARIES header → body collapses - Reload page → collapsed state preserved from localStorage - Click Shelves `+` → section does NOT collapse Closes bead bookshelf-7fc on merge.

zombor added 1 commit 2026-05-28 16:33:51 +00:00

feat(ui): collapsible sidebar section groups with localStorage persistence (bookshelf-7fc) ... 7ec3264c5a

- New Stimulus controller sidebar_section_controller.js with toggle, keyboard
  (Enter/Space), and aria-expanded/aria-hidden ARIA support
- localStorage key bookshelf:sidebar:collapsed persists collapsed groups per user
- All five nav groups (home/libraries/shelves/ingest/system) wired with the
  controller, body <div> wrappers, and matching ARIA IDs
- CSS: sidebar-nav-group--collapsed hides body; chevron rotates 90deg on collapse
- Clicking inner <a>/<button> (e.g. the Shelves "+") does NOT collapse the section
- E2E browser tests: collapse, reload-persist, and "+" non-collapse

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

zombor force-pushed bd-bookshelf-7fc from 7ec3264c5a to 40b4e2dfa9 2026-05-28 16:46:02 +00:00 Compare

zombor added 1 commit 2026-05-28 17:01:29 +00:00

fix(nav): drop /libraries header link so LIBRARIES section is collapsible (bookshelf-7fc) d4b177bfc6

zombor added 1 commit 2026-05-28 17:22:19 +00:00

chore(ci): re-trigger flaky 'Fetch source' step (bookshelf-7fc) 3de6bdbfca

zombor commented 2026-05-28 17:45:23 +00:00

Author

Owner

Copy link

Security Review — PR #154 (bookshelf-7fc): Collapsible Sidebar Stimulus Controller

Scope: localStorage defensive parsing, ARIA/DOM injection, CSP compliance, "+" link click handling, html/template XSS escaping on group keys.

---

[MAJOR] static/js/controllers/sidebar_section_controller.js:56 — onKeydown lacks the <a>/<button> guard present in toggle()

The toggle() method correctly bails out when event.target is an <a> or <button>. onKeydown() does not. Both the Libraries and Shelves header <p> elements contain a "+" <a> child. That <a> is reachable by keyboard (Tab), and keydown events on it bubble up to the <p>. When a keyboard user presses Enter or Space to activate the "+" link, onKeydown fires on the bubbled event, calls event.preventDefault() (suppressing the link navigation), and calls _toggle() (collapsing/expanding the section). Result: keyboard users cannot reach "New library" or "New shelf" via keyboard without inadvertently toggling the section; the Enter/Space activation of those links is silently eaten. Fix: mirror the toggle() guard at the top of onKeydown:

onKeydown(event) {
  var tag = event.target.tagName.toLowerCase();
  if (tag === "a" || tag === "button") return;   // ← add this guard
  if (event.key === "Enter" || event.key === " ") {
    event.preventDefault();
    this._toggle();
  }
}

---

[MINOR] templates/layouts/base.html:76,105,150,177,197 — <p> toggle elements lack role="button"

The header <p> elements have tabindex="0" and act as interactive controls, but carry no ARIA role. Screen readers will announce them as "paragraph" or "text", not as interactive buttons. Add role="button" to each toggle <p> so assistive technology exposes the correct widget role and announces aria-expanded state correctly.

---

Items confirmed clean:

• localStorage defensive parsing — readCollapsed() wraps JSON.parse in try/catch and validates Array.isArray(parsed), returning [] on any failure. Attacker-controlled JSON cannot crash the controller or execute code.
• No innerHTML/eval/Function — all DOM manipulation is via classList.toggle and setAttribute. No code injection vector.
• CSP compliance — no inline <script> or style= attributes introduced. The new <script src="...?v=..."> tag follows the existing pattern; script-src 'self' permits it. No 'unsafe-inline' required.
• "+" anchor click guard — toggle() checks event.target.tagName and returns early for <a> and <button>, so mouse clicks on "+" do not collapse the section. The e2e test sidebar_section_test.go covers this path.
• XSS via group keys — all data-sidebar-section-key-value attributes are hardcoded string literals in the template ("home", "libraries", "shelves", "ingest", "system"). No Go template variable is interpolated into these attributes.
• localStorage array poisoning — even if a third party wrote arbitrary strings into bookshelf:sidebar:collapsed, indexOf(this.keyValue) performs a string equality comparison against hardcoded keys; no code is executed on array contents.
• Storage quota exhaustion — writeCollapsed catches any setItem exception and silently ignores it; no crash path.

---

REVIEW VERDICT: 0 blocker, 1 major, 1 minor

## Security Review — PR #154 (bookshelf-7fc): Collapsible Sidebar Stimulus Controller **Scope:** localStorage defensive parsing, ARIA/DOM injection, CSP compliance, "+" link click handling, html/template XSS escaping on group keys. --- [MAJOR] static/js/controllers/sidebar_section_controller.js:56 — `onKeydown` lacks the `<a>`/`<button>` guard present in `toggle()` The `toggle()` method correctly bails out when `event.target` is an `<a>` or `<button>`. `onKeydown()` does not. Both the Libraries and Shelves header `<p>` elements contain a "+" `<a>` child. That `<a>` is reachable by keyboard (Tab), and keydown events on it bubble up to the `<p>`. When a keyboard user presses **Enter** or **Space** to activate the "+" link, `onKeydown` fires on the bubbled event, calls `event.preventDefault()` (suppressing the link navigation), and calls `_toggle()` (collapsing/expanding the section). Result: keyboard users cannot reach "New library" or "New shelf" via keyboard without inadvertently toggling the section; the Enter/Space activation of those links is silently eaten. Fix: mirror the `toggle()` guard at the top of `onKeydown`: ```js onKeydown(event) { var tag = event.target.tagName.toLowerCase(); if (tag === "a" || tag === "button") return; // ← add this guard if (event.key === "Enter" || event.key === " ") { event.preventDefault(); this._toggle(); } } ``` --- [MINOR] templates/layouts/base.html:76,105,150,177,197 — `<p>` toggle elements lack `role="button"` The header `<p>` elements have `tabindex="0"` and act as interactive controls, but carry no ARIA role. Screen readers will announce them as "paragraph" or "text", not as interactive buttons. Add `role="button"` to each toggle `<p>` so assistive technology exposes the correct widget role and announces `aria-expanded` state correctly. --- **Items confirmed clean:** - **localStorage defensive parsing** — `readCollapsed()` wraps `JSON.parse` in try/catch and validates `Array.isArray(parsed)`, returning `[]` on any failure. Attacker-controlled JSON cannot crash the controller or execute code. - **No `innerHTML`/`eval`/`Function`** — all DOM manipulation is via `classList.toggle` and `setAttribute`. No code injection vector. - **CSP compliance** — no inline `<script>` or `style=` attributes introduced. The new `<script src="...?v=...">` tag follows the existing pattern; `script-src 'self'` permits it. No `'unsafe-inline'` required. - **"+" anchor click guard** — `toggle()` checks `event.target.tagName` and returns early for `<a>` and `<button>`, so mouse clicks on "+" do not collapse the section. The e2e test `sidebar_section_test.go` covers this path. - **XSS via group keys** — all `data-sidebar-section-key-value` attributes are hardcoded string literals in the template ("home", "libraries", "shelves", "ingest", "system"). No Go template variable is interpolated into these attributes. - **localStorage array poisoning** — even if a third party wrote arbitrary strings into `bookshelf:sidebar:collapsed`, `indexOf(this.keyValue)` performs a string equality comparison against hardcoded keys; no code is executed on array contents. - **Storage quota exhaustion** — `writeCollapsed` catches any `setItem` exception and silently ignores it; no crash path. --- **REVIEW VERDICT: 0 blocker, 1 major, 1 minor**

zombor commented 2026-05-28 17:46:14 +00:00

Author

Owner

Copy link

CODE REVIEW: NOT APPROVED

Phase 0 — DEMO Verification: PARTIAL — the bead VERIFY section specifies rod e2e tests (click collapse, reload persist, "+" does not collapse). CI is green (state: success, SHA 3de6bdb), confirming all three behaviors pass under mouse clicks. The keyboard path for "+" is not covered by a test (see Major #2 below). Proceeding to Phase 1.

Phase 1 — Spec Compliance: All five requirements (toggle, localStorage, Stimulus controller, CSS, keyboard/ARIA) are addressed. The "+" affordance does NOT trigger collapse on mouse click (req 1). localStorage with invalid-JSON resilience is handled (readCollapsed try/catch, lines 16–24). Key bookshelf:sidebar:collapsed has a bookshelf: prefix; collision risk is low. CSP-safe: no inline JS, no inline styles, toggling is class-based. Stimulus naming is correct: sidebar_section_controller.js → data-controller="sidebar-section". Phase 1 passes.

Phase 2 — Code Quality:

---

[MAJOR] static/js/controllers/sidebar_section_controller.js:56 — onKeydown missing the <a>/<button> target guard that toggle() has
The toggle() method at line 46–53 correctly returns early if event.target is an <a> or <button>. The onKeydown() method at line 56–61 calls _toggle() unconditionally. The keydown action is bound to the <p> header element which receives bubbled keydown events from all its children. When a keyboard user tabs to the "+" link inside the LIBRARIES or SHELVES header and presses Enter, the browser activates the link (navigation) AND the bubbled keydown event triggers onKeydown() → _toggle() — flipping the localStorage state to collapsed. After the navigation returns to the page, the section appears collapsed even though the user was only activating the "+". Fix: add the same guard to onKeydown:

onKeydown(event) {
  var tag = event.target.tagName.toLowerCase();
  if (tag === "a" || tag === "button") return;
  if (event.key === "Enter" || event.key === " ") {
    event.preventDefault();
    this._toggle();
  }
}

[MAJOR] templates/layouts/base.html:73,102,147,174,194 — <p> interactive toggle elements missing role="button"
Each section header is a <p> element with tabindex="0", aria-expanded, aria-controls, and keyboard/click actions. The implicit ARIA role of <p> is paragraph. Screen readers (NVDA, VoiceOver, JAWS) will not announce these elements as interactive and may not surface the aria-expanded state, because aria-expanded is only meaningful on roles that support it (button, combobox, listbox, etc.). Per WCAG 4.1.2 (Name, Role, Value), interactive custom controls must have an appropriate role. Fix: add role="button" to each <p> that carries tabindex=0 + aria-expanded. Alternatively, consider replacing the <p> with an actual <button> element (which natively conveys interactivity, supports Enter/Space, and needs no tabindex).

[MINOR] e2e/browser/sidebar_section_test.go — no keyboard-activation test for the "+" link
The "+" non-collapse test at line 75–106 uses MustClick() (mouse event). The onKeydown bug (#1 above) is not exercised. A test that focuses the "+" link and dispatches new KeyboardEvent('keydown', {key:'Enter', bubbles:true}) on it would have caught this. Worth adding in the fix commit.

---

REVIEW VERDICT: 0 blocker, 2 major, 1 minor

## CODE REVIEW: NOT APPROVED **Phase 0 — DEMO Verification:** PARTIAL — the bead VERIFY section specifies rod e2e tests (click collapse, reload persist, "+" does not collapse). CI is green (state: success, SHA 3de6bdb), confirming all three behaviors pass under mouse clicks. The keyboard path for "+" is not covered by a test (see Major #2 below). Proceeding to Phase 1. **Phase 1 — Spec Compliance:** All five requirements (toggle, localStorage, Stimulus controller, CSS, keyboard/ARIA) are addressed. The "+" affordance does NOT trigger collapse on mouse click (req 1). localStorage with invalid-JSON resilience is handled (readCollapsed try/catch, lines 16–24). Key `bookshelf:sidebar:collapsed` has a `bookshelf:` prefix; collision risk is low. CSP-safe: no inline JS, no inline styles, toggling is class-based. Stimulus naming is correct: `sidebar_section_controller.js` → `data-controller="sidebar-section"`. Phase 1 passes. **Phase 2 — Code Quality:** --- [MAJOR] static/js/controllers/sidebar_section_controller.js:56 — `onKeydown` missing the `<a>`/`<button>` target guard that `toggle()` has The `toggle()` method at line 46–53 correctly returns early if `event.target` is an `<a>` or `<button>`. The `onKeydown()` method at line 56–61 calls `_toggle()` unconditionally. The `keydown` action is bound to the `<p>` header element which receives bubbled keydown events from all its children. When a keyboard user tabs to the "+" link inside the LIBRARIES or SHELVES header and presses Enter, the browser activates the link (navigation) AND the bubbled keydown event triggers `onKeydown()` → `_toggle()` — flipping the localStorage state to collapsed. After the navigation returns to the page, the section appears collapsed even though the user was only activating the "+". Fix: add the same guard to `onKeydown`: ```js onKeydown(event) { var tag = event.target.tagName.toLowerCase(); if (tag === "a" || tag === "button") return; if (event.key === "Enter" || event.key === " ") { event.preventDefault(); this._toggle(); } } ``` [MAJOR] templates/layouts/base.html:73,102,147,174,194 — `<p>` interactive toggle elements missing `role="button"` Each section header is a `<p>` element with `tabindex="0"`, `aria-expanded`, `aria-controls`, and keyboard/click actions. The implicit ARIA role of `<p>` is `paragraph`. Screen readers (NVDA, VoiceOver, JAWS) will not announce these elements as interactive and may not surface the `aria-expanded` state, because `aria-expanded` is only meaningful on roles that support it (button, combobox, listbox, etc.). Per WCAG 4.1.2 (Name, Role, Value), interactive custom controls must have an appropriate role. Fix: add `role="button"` to each `<p>` that carries `tabindex=0` + `aria-expanded`. Alternatively, consider replacing the `<p>` with an actual `<button>` element (which natively conveys interactivity, supports Enter/Space, and needs no `tabindex`). [MINOR] e2e/browser/sidebar_section_test.go — no keyboard-activation test for the "+" link The "+" non-collapse test at line 75–106 uses `MustClick()` (mouse event). The `onKeydown` bug (#1 above) is not exercised. A test that focuses the "+" link and dispatches `new KeyboardEvent('keydown', {key:'Enter', bubbles:true})` on it would have caught this. Worth adding in the fix commit. --- REVIEW VERDICT: 0 blocker, 2 major, 1 minor

zombor added 1 commit 2026-05-28 17:47:58 +00:00

fix(nav): a11y keydown guard + role=button on toggles per review feedback (bookshelf-7fc) caec83c4ad

zombor force-pushed bd-bookshelf-7fc from caec83c4ad to 5bd4d0b40f 2026-05-28 18:00:01 +00:00 Compare

zombor force-pushed bd-bookshelf-7fc from 5bd4d0b40f to 941aef475b 2026-05-28 18:16:08 +00:00 Compare

zombor merged commit ba4f5a0eb2 into main 2026-05-28 18:21:14 +00:00

zombor referenced this pull request from a commit 2026-05-28 18:21:17 +00:00

Merge pull request 'feat(ui): collapsible sidebar section groups (bookshelf-7fc)' (#154) from bd-bookshelf-7fc 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!154

No description provided.