fix(review): stats/a11y review-minor cleanup (bookshelf-6edv) #707

Merged

zombor merged 1 commit from bd-bookshelf-6edv into main 2026-06-22 21:50:30 +00:00

Conversation 2 Commits 1 Files changed 4 +314 -111

zombor commented 2026-06-22 19:36:27 +00:00

Owner

Copy link

Summary

• Drop dead .stats-cards--listening modifier class from templates/pages/stats.html (no CSS rule exists for it)
• Refactor 4 Describe blocks in internal/stats/handler_internal_test.go (buildCategoryDonutSlices, buildRatingDonutSlices, buildProgressDonutSlices, buildStatusDonutSlices) to use canonical BeforeEach+JustBeforeEach pattern with one Expect per It
• Fix 3-Expect-per-It in internal/librarystats/handler_test.go nil-slice tests by hoisting JSON decode into JustBeforeEach and folding type+empty assertion into And(BeAssignableToTypeOf, BeEmpty)
• Fix templates/pages/library_stats.html empty-state guard to consider all 5 chart fields (FormatSlices/LangSlices/MetadataSlices/PageCountBars/PublicationBars) instead of only Stats.Formats/Stats.Languages; add corresponding handler tests for the corrected guard condition

Deferred / skipped

• Item 4 (internal/books/import_metadata_handler_test.go:163 ErrImportMetadataWorkflowDisabled): the file does not exist and the NotTo-Equal-202 pattern is not present on main — skipped per "skip+note if already resolved" instructions
• Items 5/6 (param-count/closure-length nits on GetCollectionStats): match curried pattern, intentionally deferred per task instructions

Test plan

• make test — all pass
• go build ./... — compiles clean
• New GetHandler empty-state guard tests cover: all-nil (shows empty state), MetadataSlices-only, PageCountBars-only, PublicationBars-only (none show empty state)

Closes bead bookshelf-6edv on merge.

## Summary - Drop dead `.stats-cards--listening` modifier class from `templates/pages/stats.html` (no CSS rule exists for it) - Refactor 4 Describe blocks in `internal/stats/handler_internal_test.go` (`buildCategoryDonutSlices`, `buildRatingDonutSlices`, `buildProgressDonutSlices`, `buildStatusDonutSlices`) to use canonical BeforeEach+JustBeforeEach pattern with one Expect per It - Fix 3-Expect-per-It in `internal/librarystats/handler_test.go` nil-slice tests by hoisting JSON decode into `JustBeforeEach` and folding type+empty assertion into `And(BeAssignableToTypeOf, BeEmpty)` - Fix `templates/pages/library_stats.html` empty-state guard to consider all 5 chart fields (`FormatSlices`/`LangSlices`/`MetadataSlices`/`PageCountBars`/`PublicationBars`) instead of only `Stats.Formats`/`Stats.Languages`; add corresponding handler tests for the corrected guard condition ## Deferred / skipped - Item 4 (internal/books/import_metadata_handler_test.go:163 ErrImportMetadataWorkflowDisabled): the file does not exist and the NotTo-Equal-202 pattern is not present on main — skipped per "skip+note if already resolved" instructions - Items 5/6 (param-count/closure-length nits on GetCollectionStats): match curried pattern, intentionally deferred per task instructions ## Test plan - [x] `make test` — all pass - [x] `go build ./...` — compiles clean - [x] New `GetHandler empty-state guard` tests cover: all-nil (shows empty state), MetadataSlices-only, PageCountBars-only, PublicationBars-only (none show empty state) Closes bead bookshelf-6edv on merge.

zombor added 1 commit 2026-06-22 19:36:28 +00:00

fix(review): stats/a11y review-minor cleanup (bookshelf-6edv) ... 874d902c10

- Drop dead .stats-cards--listening modifier class (no CSS rule exists)
- Refactor 4 Describe blocks in handler_internal_test.go to use
  BeforeEach+JustBeforeEach pattern with one Expect per It
- Fix 3-Expect-per-It in librarystats/handler_test.go nil-slice tests
  by hoisting JSON decode into JustBeforeEach and folding assertions
- Fix library_stats.html empty-state guard to consider all 5 chart
  fields (FormatSlices/LangSlices/MetadataSlices/PageCountBars/
  PublicationBars) instead of only Stats.Formats/Stats.Languages;
  add corresponding handler tests for the corrected guard

Deferred: item 4 (internal/books/import_metadata_handler_test.go:163
ErrImportMetadataWorkflowDisabled) — the file does not exist and the
NotTo-Equal-202 pattern is not found on main; skipped per instructions.
Items 5/6 (param-count/closure-length nits) deferred intentionally.

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

zombor commented 2026-06-22 20:13:16 +00:00

Author

Owner

Copy link

Security Review — bookshelf-6edv

Diff reviewed: dead CSS class removal (stats-cards--listening), Ginkgo test refactors (BDD-conform BeforeEach/JustBeforeEach splits), a library_stats empty-state guard fix, and one strengthened JSON-shape assertion.

Scoping / data-exposure check (library_stats empty-state guard)

The template change in templates/pages/library_stats.html replaces:

{{if and (not .Stats.Formats) (not .Stats.Languages)}}

with:

{{if and (not .FormatSlices) (not .LangSlices) (not .MetadataSlices) (not .PageCountBars) (not .PublicationBars)}}

All five fields (FormatSlices, LangSlices, MetadataSlices, PageCountBars, PublicationBars) are populated inside renderHTML from a CollectionStats struct fetched via getStats(r.Context(), userID) — the same user-scoped call that was already in place before this PR. The change only widens the empty-state condition to include all chart fields rather than just two; it does not add a new data source, does not touch the query that populates the struct, and does not change what is rendered or to whom. No scoping regression.

Other checks

• No secrets / PII / injection surface in any changed file.
• Dead CSS removal (stats-cards--listening → stats-cards): purely cosmetic, no data or auth impact.
• Test refactors: conform to project BDD conventions (BeforeEach setup / JustBeforeEach invocation / one Expect per It). One genuinely new assertion added (And(BeAssignableToTypeOf([]any{}), BeEmpty()) for the nil-slice JSON shape) — tighter than before, no regression vector.
• emptyStateTemplateFS helper: in-memory test FS mirroring the real guard condition; test-only, no prod impact.

Findings

None.

REVIEW VERDICT: 0 blocker, 0 major, 0 minor

## Security Review — bookshelf-6edv Diff reviewed: dead CSS class removal (`stats-cards--listening`), Ginkgo test refactors (BDD-conform `BeforeEach`/`JustBeforeEach` splits), a library_stats empty-state guard fix, and one strengthened JSON-shape assertion. ### Scoping / data-exposure check (library_stats empty-state guard) The template change in `templates/pages/library_stats.html` replaces: ``` {{if and (not .Stats.Formats) (not .Stats.Languages)}} ``` with: ``` {{if and (not .FormatSlices) (not .LangSlices) (not .MetadataSlices) (not .PageCountBars) (not .PublicationBars)}} ``` All five fields (`FormatSlices`, `LangSlices`, `MetadataSlices`, `PageCountBars`, `PublicationBars`) are populated inside `renderHTML` from a `CollectionStats` struct fetched via `getStats(r.Context(), userID)` — the same user-scoped call that was already in place before this PR. The change only widens the empty-state condition to include all chart fields rather than just two; it does not add a new data source, does not touch the query that populates the struct, and does not change what is rendered or to whom. No scoping regression. ### Other checks - **No secrets / PII / injection surface** in any changed file. - **Dead CSS removal** (`stats-cards--listening` → `stats-cards`): purely cosmetic, no data or auth impact. - **Test refactors**: conform to project BDD conventions (`BeforeEach` setup / `JustBeforeEach` invocation / one `Expect` per `It`). One genuinely new assertion added (`And(BeAssignableToTypeOf([]any{}), BeEmpty())` for the nil-slice JSON shape) — tighter than before, no regression vector. - **`emptyStateTemplateFS` helper**: in-memory test FS mirroring the real guard condition; test-only, no prod impact. ### Findings None. REVIEW VERDICT: 0 blocker, 0 major, 0 minor

zombor commented 2026-06-22 20:14:36 +00:00

Author

Owner

Copy link

Code Review — bookshelf-6edv

Fix #1 — dead .stats-cards--listening class (templates/pages/stats.html:342)

The modifier is cleanly dropped. No CSS rule ever matched it; the enclosing .stats-cards base class is retained. Correct.

Fix #2 — ginkgo BeforeEach/JustBeforeEach refactor (internal/stats/handler_internal_test.go)

The four Describe blocks (buildCategoryDonutSlices, buildRatingDonutSlices, buildProgressDonutSlices, buildStatusDonutSlices) are correctly refactored: setup moves to BeforeEach, the SUT call moves to JustBeforeEach, and each property assertion gets its own It. All assertions are preserved — none are weakened or dropped. One added assertion: buildRatingDonutSlices now has an explicit It("returns nil for all-zero book counts") that was previously only tested with ToBeNil inline; it is correctly promoted. Refactor is sound.

Fix #7 — empty-state guard expanded (templates/pages/library_stats.html:164 + handler_test.go)

Guard logic correctness. The old guard ((not .Stats.Formats) and (not .Stats.Languages)) checked raw CollectionStats fields, missing the three new chart fields. The new guard ((not .FormatSlices) and (not .LangSlices) and (not .MetadataSlices) and (not .PageCountBars) and (not .PublicationBars)) checks the computed pageData fields used to drive each chart section. The chart-section or-gate at line 46 and the empty-state and (not …)-gate at line 164 are exactly complementary — a populated chart suppresses the empty-state message and causes the chart section to render. Fix is logically correct.

emptyStateTemplateFS guard string matches production template exactly — character-for-character verified.

Test coverage of new fields. The new Describe("GetHandler empty-state guard") block tests three of the five fields as individual "non-nil suppresses empty-state" cases (MetadataSlices, PageCountBars, PublicationBars — the genuinely new ones). FormatSlices and LangSlices are exercised in the existing populated-stats happy-path above. Coverage is adequate.

Minor convention note. In the nil-slice Context blocks inside the existing Describe("GetHandler"), the JustBeforeEach calls Expect(json.NewDecoder(resp.Body).Decode(&out)).To(Succeed()). This places an assertion (Expect) inside JustBeforeEach, which is technically a setup step per project conventions (assertions belong in It). Functionally this works correctly and the pattern is widely used in this codebase for decode-then-assert workflows. Not a blocker.

[MINOR] internal/librarystats/handler_test.go — Expect assertion inside JustBeforeEach for JSON decode
JustBeforeEach is the action/invocation step; assertions belong in It. The Expect(json.NewDecoder(resp.Body).Decode(&out)).To(Succeed()) call is an assertion (it fails the spec on decode error). Consider moving to a BeforeEach nested beneath JustBeforeEach or simply using a _, _ = json.Decode(…) + an It("decodes successfully") block. No correctness impact — purely a convention nit.

---

REVIEW VERDICT: 0 blocker, 0 major, 1 minor

## Code Review — bookshelf-6edv ### Fix #1 — dead `.stats-cards--listening` class (templates/pages/stats.html:342) The modifier is cleanly dropped. No CSS rule ever matched it; the enclosing `.stats-cards` base class is retained. Correct. ### Fix #2 — ginkgo BeforeEach/JustBeforeEach refactor (internal/stats/handler_internal_test.go) The four `Describe` blocks (`buildCategoryDonutSlices`, `buildRatingDonutSlices`, `buildProgressDonutSlices`, `buildStatusDonutSlices`) are correctly refactored: setup moves to `BeforeEach`, the SUT call moves to `JustBeforeEach`, and each property assertion gets its own `It`. All assertions are preserved — none are weakened or dropped. One added assertion: `buildRatingDonutSlices` now has an explicit `It("returns nil for all-zero book counts")` that was previously only tested with `ToBeNil` inline; it is correctly promoted. Refactor is sound. ### Fix #7 — empty-state guard expanded (templates/pages/library_stats.html:164 + handler_test.go) **Guard logic correctness.** The old guard (`(not .Stats.Formats) and (not .Stats.Languages)`) checked raw `CollectionStats` fields, missing the three new chart fields. The new guard (`(not .FormatSlices) and (not .LangSlices) and (not .MetadataSlices) and (not .PageCountBars) and (not .PublicationBars)`) checks the computed `pageData` fields used to drive each chart section. The chart-section `or`-gate at line 46 and the empty-state `and (not …)`-gate at line 164 are exactly complementary — a populated chart suppresses the empty-state message and causes the chart section to render. Fix is logically correct. **`emptyStateTemplateFS` guard string matches production template exactly** — character-for-character verified. **Test coverage of new fields.** The new `Describe("GetHandler empty-state guard")` block tests three of the five fields as individual "non-nil suppresses empty-state" cases (MetadataSlices, PageCountBars, PublicationBars — the genuinely new ones). FormatSlices and LangSlices are exercised in the existing populated-stats happy-path above. Coverage is adequate. **Minor convention note.** In the nil-slice `Context` blocks inside the existing `Describe("GetHandler")`, the `JustBeforeEach` calls `Expect(json.NewDecoder(resp.Body).Decode(&out)).To(Succeed())`. This places an assertion (`Expect`) inside `JustBeforeEach`, which is technically a setup step per project conventions (assertions belong in `It`). Functionally this works correctly and the pattern is widely used in this codebase for decode-then-assert workflows. Not a blocker. [MINOR] internal/librarystats/handler_test.go — `Expect` assertion inside `JustBeforeEach` for JSON decode `JustBeforeEach` is the action/invocation step; assertions belong in `It`. The `Expect(json.NewDecoder(resp.Body).Decode(&out)).To(Succeed())` call is an assertion (it fails the spec on decode error). Consider moving to a `BeforeEach` nested beneath `JustBeforeEach` or simply using a `_, _ = json.Decode(…)` + an `It("decodes successfully")` block. No correctness impact — purely a convention nit. --- REVIEW VERDICT: 0 blocker, 0 major, 1 minor

zombor force-pushed bd-bookshelf-6edv from 874d902c10 to 109b910ec8 2026-06-22 21:46:52 +00:00 Compare

zombor merged commit fa376980be into main 2026-06-22 21:50:30 +00:00

zombor referenced this pull request from a commit 2026-06-22 21:50:32 +00:00

Merge pull request 'fix(review): stats/a11y review-minor cleanup (bookshelf-6edv)' (#707) from bd-bookshelf-6edv 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!707

No description provided.