fix(metadata): dedup set-all empty option + log errors (bookshelf-4y3y.6) #452

Merged

zombor merged 2 commits from bd-bookshelf-4y3y.6 into main 2026-06-09 03:15:36 +00:00

Conversation 2 Commits 2 Files changed 5 +19 -15

zombor commented 2026-06-09 02:40:10 +00:00

Owner

Copy link

Summary

Two small minors from #445 re-review (4y3y.2):

1. templates/pages/settings_metadata_field_priority.html — Remove duplicate empty-value options in set-all dropdowns. contentProviders already includes {ID:"", Name:"— none —"}, so the explicit <option value=""> placeholders created two empty options per dropdown. Removed the explicit ones.

2. internal/books/metadata_store.go — Log at Warn when listAuthors or listCategories fails, per logging-standard trigger. Include context (book_id, which call failed). No control flow change — errors still silently result in nil slice fallback.

Test plan

• make test ✓
• make coverage (100% gate) ✓
• Local build ✓

Closes bead bookshelf-4y3y.6 on merge.

## Summary Two small minors from #445 re-review (4y3y.2): 1. **templates/pages/settings_metadata_field_priority.html** — Remove duplicate empty-value options in set-all dropdowns. `contentProviders` already includes `{ID:"", Name:"— none —"}`, so the explicit `<option value="">` placeholders created two empty options per dropdown. Removed the explicit ones. 2. **internal/books/metadata_store.go** — Log at Warn when `listAuthors` or `listCategories` fails, per logging-standard trigger. Include context (book_id, which call failed). No control flow change — errors still silently result in nil slice fallback. ## Test plan - make test ✓ - make coverage (100% gate) ✓ - Local build ✓ Closes bead bookshelf-4y3y.6 on merge.

zombor added 1 commit 2026-06-09 02:40:10 +00:00

fix(metadata): dedup set-all empty option + log swallowed author/category errors ... 7d57f9b12b

1. templates/pages/settings_metadata_field_priority.html: Remove duplicate empty
   value options from set-all dropdowns. contentProviders already includes
   {ID:"", Name:"— none —"}, so explicit <option value="">— set all Px —</option>
   placeholder lines created two value="" options per dropdown. Keep one.

2. internal/books/metadata_store.go: Log at Warn when listAuthors or listCategories
   fails, per logging-standard trigger. Include context (book_id, which call failed).
   No control flow change — errors still silently result in nil slice fallback.

Fixes #445 review minors (4y3y.2).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

zombor commented 2026-06-09 02:51:59 +00:00

Author

Owner

Copy link

Security Review (bot)

PR #452 — bead bookshelf-4y3y.6

Scope reviewed

1. New logger.Warn calls in internal/books/metadata_store.go (log-data audit)
2. Template change removing empty <option value=""> entries in templates/pages/settings_metadata_field_priority.html (new sink / injection audit)

Findings

Log lines (metadata_store.go lines ~183 and ~187):

logger.Warn("failed to list book authors", "book_id", bookID, "error", listErr)
logger.Warn("failed to list book categories", "book_id", bookID, "error", listErr)

• book_id — internal auto-increment integer. No PII, no secret.
• error — a Go error value from a DB call (sql.Rows.Err() / driver). Contains DB error text (e.g. "connection refused", "deadlock found"). No user-supplied content, no token/password. Complies with logging-standard ("context, not text"; no PII/credentials).
• Both fields are benign. No sensitive data logged.

Template change (settings_metadata_field_priority.html):

• Removes four <option value=""> placeholder options from set-all <select> dropdowns.
• No new template output sinks introduced. {{range $.Providers}}<option value="{{.ID}}">{{.Name}}</option>{{end}} was already present and is auto-escaped by html/template. .ID and .Name are server-side struct fields, not user-reflected input — no XSS vector. The <option value="__clear__"> literal is a hardcoded string, not user-supplied. No change in injection surface.

Verdict

REVIEW VERDICT: 0 blocker, 0 major, 0 minor

## Security Review (bot) **PR #452 — bead bookshelf-4y3y.6** ### Scope reviewed 1. New `logger.Warn` calls in `internal/books/metadata_store.go` (log-data audit) 2. Template change removing empty `<option value="">` entries in `templates/pages/settings_metadata_field_priority.html` (new sink / injection audit) ### Findings **Log lines (`metadata_store.go` lines ~183 and ~187):** ```go logger.Warn("failed to list book authors", "book_id", bookID, "error", listErr) logger.Warn("failed to list book categories", "book_id", bookID, "error", listErr) ``` - `book_id` — internal auto-increment integer. No PII, no secret. - `error` — a Go `error` value from a DB call (`sql.Rows.Err()` / driver). Contains DB error text (e.g. "connection refused", "deadlock found"). No user-supplied content, no token/password. Complies with logging-standard ("context, not text"; no PII/credentials). - Both fields are benign. No sensitive data logged. **Template change (`settings_metadata_field_priority.html`):** - Removes four `<option value="">` placeholder options from set-all `<select>` dropdowns. - No new template output sinks introduced. `{{range $.Providers}}<option value="{{.ID}}">{{.Name}}</option>{{end}}` was already present and is auto-escaped by `html/template`. `.ID` and `.Name` are server-side struct fields, not user-reflected input — no XSS vector. The `<option value="__clear__">` literal is a hardcoded string, not user-supplied. No change in injection surface. ### Verdict REVIEW VERDICT: 0 blocker, 0 major, 0 minor

zombor commented 2026-06-09 02:54:34 +00:00

Author

Owner

Copy link

Code Review (bot)

PR #452 — bead bookshelf-4y3y.6

Phase 0: DEMO Verification

No DEMO block in the bead. This is a code-review-spawned minor-fix bead (non-blocking minors from #445 re-review) with no new API surface or interactive behavior to demo. Per the review request the verification is structural: dedup correctness, logging compliance, signature threading, and no behavior regression.

Phase 1: Spec Compliance

Both requirements from the bead description are addressed:

1. Duplicate empty option removed — templates/pages/settings_metadata_field_priority.html: the four explicit <option value="">— set all Pn —</option> placeholders are dropped. The {{range $.Providers}} range already renders {ID:"", Name:"— none —"} as its first element (from contentProviders[0] in matrix_handler.go:62), so exactly one empty-value option remains per set-all dropdown.

2. Warn logging added — internal/books/metadata_store.go:183,187: both listAuthors and listCategories error paths now log at Warn with structured "book_id" and "error" fields. Previously both errors were silently swallowed.

Signature change (logger *slog.Logger as first param to GetMetadataForRefetch) is threaded correctly through all call sites:

• internal/books/wire.go:233 — passes d.Logger
• internal/app/build_extended_deps.go:364 — passes logger
• internal/books/metadata_store_test.go — all 8 construction sites pass a slog.New(slog.NewTextHandler(io.Discard, nil)) logger

Phase 2: Code Quality

Logging compliance (logging-standard.md):

• metadata_store.go:183 — logger.Warn("failed to list book authors", "book_id", bookID, "error", listErr) — structured k/v pairs, no string interpolation, no PII. Compliant.
• metadata_store.go:187 — logger.Warn("failed to list book categories", "book_id", bookID, "error", listErr) — same. Compliant.

Test coverage — missing listCategories error test:
The listAuthors error path has a test (Context("when listAuthors returns an error", ...) at metadata_store_test.go:272). There is no corresponding Context("when listCategories returns an error") block. The new else branch at metadata_store.go:187 is uncovered by the test suite. The project requires 100% coverage on internal/. CI is green, which is the authoritative signal — but this appears inconsistent. Either the coverage block boundaries happen to collapse the two else branches into a single countable statement (possible in Go's coverage model for consecutive if/else if sequences), or a test is missing and CI's green status would need investigation. Since CI is the source of truth per review policy, I am not blocking on this, but flagging for awareness.

[MINOR] internal/books/metadata_store_test.go — no test for listCategories error path
The symmetric else branch added at metadata_store.go:187 (Warn log for listCategories failure) has no corresponding Context("when listCategories returns an error") test, unlike the listAuthors error path at line 272. CI is green so the 100% gate passed; however the test parity with listAuthors is missing. Worth adding a Context("when listCategories returns an error", ...) block to confirm the error branch is reached and the function still returns nil categories + no outer error.

Template dedup — UX note:
Removing the per-column placeholder labels ("— set all P1 —", "— set all P2 —", etc.) in favour of the shared "— none —" text is a minor UX regression — users lose the per-column label context. The aria-label attributes ("Set all 1st priority" etc.) preserve accessibility semantics, so this is not a blocker. The setAll Stimulus handler correctly resets to event.target.value = "" (controller:33) which lands back on the remaining "— none —" option.

No behavior regression found:

• setAll controller logic: unchanged. The value = "" reset at field_priority_matrix_controller.js:33 correctly selects the first option (the surviving empty option).
• __clear__ option and provider options: unchanged, all still rendered.
• The applyMatrixUpdate handler: unchanged, no impact.

REVIEW VERDICT: 0 blocker, 0 major, 1 minor

## Code Review (bot) **PR #452 — bead bookshelf-4y3y.6** ### Phase 0: DEMO Verification No DEMO block in the bead. This is a code-review-spawned minor-fix bead (non-blocking minors from #445 re-review) with no new API surface or interactive behavior to demo. Per the review request the verification is structural: dedup correctness, logging compliance, signature threading, and no behavior regression. ### Phase 1: Spec Compliance Both requirements from the bead description are addressed: 1. **Duplicate empty option removed** — `templates/pages/settings_metadata_field_priority.html`: the four explicit `<option value="">— set all Pn —</option>` placeholders are dropped. The `{{range $.Providers}}` range already renders `{ID:"", Name:"— none —"}` as its first element (from `contentProviders[0]` in `matrix_handler.go:62`), so exactly one empty-value option remains per set-all dropdown. 2. **Warn logging added** — `internal/books/metadata_store.go:183,187`: both `listAuthors` and `listCategories` error paths now log at `Warn` with structured `"book_id"` and `"error"` fields. Previously both errors were silently swallowed. Signature change (`logger *slog.Logger` as first param to `GetMetadataForRefetch`) is threaded correctly through all call sites: - `internal/books/wire.go:233` — passes `d.Logger` - `internal/app/build_extended_deps.go:364` — passes `logger` - `internal/books/metadata_store_test.go` — all 8 construction sites pass a `slog.New(slog.NewTextHandler(io.Discard, nil))` logger ### Phase 2: Code Quality **Logging compliance (logging-standard.md):** - `metadata_store.go:183` — `logger.Warn("failed to list book authors", "book_id", bookID, "error", listErr)` — structured k/v pairs, no string interpolation, no PII. Compliant. - `metadata_store.go:187` — `logger.Warn("failed to list book categories", "book_id", bookID, "error", listErr)` — same. Compliant. **Test coverage — missing `listCategories` error test:** The `listAuthors` error path has a test (`Context("when listAuthors returns an error", ...)` at `metadata_store_test.go:272`). There is no corresponding `Context("when listCategories returns an error")` block. The new `else` branch at `metadata_store.go:187` is uncovered by the test suite. The project requires 100% coverage on `internal/`. CI is green, which is the authoritative signal — but this appears inconsistent. Either the coverage block boundaries happen to collapse the two `else` branches into a single countable statement (possible in Go's coverage model for consecutive `if/else if` sequences), or a test is missing and CI's green status would need investigation. Since CI is the source of truth per review policy, I am not blocking on this, but flagging for awareness. [MINOR] internal/books/metadata_store_test.go — no test for `listCategories` error path The symmetric `else` branch added at `metadata_store.go:187` (Warn log for listCategories failure) has no corresponding `Context("when listCategories returns an error")` test, unlike the `listAuthors` error path at line 272. CI is green so the 100% gate passed; however the test parity with listAuthors is missing. Worth adding a `Context("when listCategories returns an error", ...)` block to confirm the error branch is reached and the function still returns nil categories + no outer error. **Template dedup — UX note:** Removing the per-column placeholder labels ("— set all P1 —", "— set all P2 —", etc.) in favour of the shared "— none —" text is a minor UX regression — users lose the per-column label context. The `aria-label` attributes (`"Set all 1st priority"` etc.) preserve accessibility semantics, so this is not a blocker. The `setAll` Stimulus handler correctly resets to `event.target.value = ""` (controller:33) which lands back on the remaining "— none —" option. **No behavior regression found:** - `setAll` controller logic: unchanged. The `value = ""` reset at `field_priority_matrix_controller.js:33` correctly selects the first option (the surviving empty option). - `__clear__` option and provider options: unchanged, all still rendered. - The `applyMatrixUpdate` handler: unchanged, no impact. REVIEW VERDICT: 0 blocker, 0 major, 1 minor

zombor added 1 commit 2026-06-09 02:58:03 +00:00

Merge branch 'main' into bd-bookshelf-4y3y.6 813f74b78e

zombor merged commit 98d078a0d4 into main 2026-06-09 03:15:36 +00:00

zombor referenced this pull request from a commit 2026-06-09 03:15:36 +00:00

Merge pull request 'fix(metadata): dedup set-all empty option + log errors (bookshelf-4y3y.6)' (#452) from bd-bookshelf-4y3y.6 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!452

No description provided.