fix(books): layered fallback for comic Fetch Metadata SERIES/issue prefill (bookshelf-qnmg) #847

Merged

zombor merged 2 commits from bd-bookshelf-qnmg into main 2026-06-29 17:02:35 +00:00

Conversation 7 Commits 2 Files changed 4 +350 -7

zombor commented 2026-06-29 16:18:41 +00:00

Owner

Copy link

Summary

Fixes the comic Fetch Metadata form's SERIES/Issue fields being blank when comic_metadata.volume_name/issue_number are empty — even when the book had a Series association (e.g. "Doc Savage #2") or a parseable filename like "15761 Doc Savage 002".

Root cause: handler.go computed the prefill ONLY from cd.VolumeName/cd.IssueNumber, with no fallback when those fields were empty.

Fix: Layered fallback for isComic books:

• SERIES: cd.VolumeName → book.Series.Name → parsed from primary CBX filename (strips leading numeric scan-id + trailing zero-padded issue number)
• ISSUE: cd.IssueNumber → book.Series.Number → parsed from filename
• YEAR: cd.CoverDate[:4] only; left blank when absent

New helpers (all 100% covered, black-box tests):

• comicSearchPrefill — orchestrates the layered fallback
• parseComicFilenameParts — strips leading scan-id prefix + trailing zero-padded issue (e.g. "15761 Doc Savage 002" → series="Doc Savage", issue="2")
• primaryComicFileStem — extension-stripped name of first CBX file in book.Files
• formatSeriesNumber — formats float64 as integer or minimal decimal string

Test plan

• 4 Ginkgo test cases in comic_search_prefill_test.go: cd with volume_name set; cd empty + book.Series set; cd nil + book.Series set; cd nil + no Series + filename-only
• Edge cases: all-zero issue number; fractional series number; no CBX files; empty stem
• make coverage passes (zero uncovered blocks)
• go vet + golangci-lint run ./internal/books/... clean

Closes bead bookshelf-qnmg on merge.

## Summary Fixes the comic Fetch Metadata form's SERIES/Issue fields being blank when `comic_metadata.volume_name`/`issue_number` are empty — even when the book had a Series association (e.g. "Doc Savage #2") or a parseable filename like "15761 Doc Savage 002". **Root cause:** `handler.go` computed the prefill ONLY from `cd.VolumeName`/`cd.IssueNumber`, with no fallback when those fields were empty. **Fix:** Layered fallback for `isComic` books: - SERIES: `cd.VolumeName` → `book.Series.Name` → parsed from primary CBX filename (strips leading numeric scan-id + trailing zero-padded issue number) - ISSUE: `cd.IssueNumber` → `book.Series.Number` → parsed from filename - YEAR: `cd.CoverDate[:4]` only; left blank when absent **New helpers (all 100% covered, black-box tests):** - `comicSearchPrefill` — orchestrates the layered fallback - `parseComicFilenameParts` — strips leading scan-id prefix + trailing zero-padded issue (e.g. "15761 Doc Savage 002" → series="Doc Savage", issue="2") - `primaryComicFileStem` — extension-stripped name of first CBX file in book.Files - `formatSeriesNumber` — formats float64 as integer or minimal decimal string ## Test plan - [x] 4 Ginkgo test cases in `comic_search_prefill_test.go`: cd with volume_name set; cd empty + book.Series set; cd nil + book.Series set; cd nil + no Series + filename-only - [x] Edge cases: all-zero issue number; fractional series number; no CBX files; empty stem - [x] `make coverage` passes (zero uncovered blocks) - [x] `go vet` + `golangci-lint run ./internal/books/...` clean Closes bead bookshelf-qnmg on merge.

recompute-match-score-kebab-open.png

109 KiB

wf-detail-older-execution.png

55 KiB

wf-detail-older-execution.png

55 KiB

recompute-match-score-kebab-open.png

109 KiB

wf-detail-older-execution.png

55 KiB

recompute-match-score-kebab-open.png

109 KiB

zombor added 1 commit 2026-06-29 16:18:41 +00:00

fix(books): layered fallback for comic Fetch Metadata SERIES/issue prefill (bookshelf-qnmg) ... 87c73a0b9c

Previously the Fetch Metadata form's SERIES/Issue fields were blank whenever
comic_metadata.volume_name/issue_number were empty — even when the book had a
Series association or a parseable filename like "15761 Doc Savage 002".

Now applies a layered fallback for isComic books:
- SERIES:  cd.VolumeName → book.Series.Name → parsed from primary CBX filename
- ISSUE:   cd.IssueNumber → book.Series.Number → parsed from filename
- YEAR:    cd.CoverDate[:4] only; left blank when absent

New helpers (100% covered, black-box):
- comicSearchPrefill: orchestrates the layered fallback
- parseComicFilenameParts: strips leading scan-id prefix + trailing zero-padded issue
- primaryComicFileStem: returns extension-stripped name of first CBX file
- formatSeriesNumber: formats float64 as integer string (or decimal when fractional)

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

forgejo-actions commented 2026-06-29 16:20:28 +00:00

Copy link

Recompute Match Score — kebab open screenshot (recompute-match-score-kebab-open)

**Recompute Match Score — kebab open screenshot** (recompute-match-score-kebab-open) 

forgejo-actions commented 2026-06-29 16:20:45 +00:00

Copy link

Workflow Detail page screenshot (wf-detail-older-execution)

Older completed ContinueAsNew epoch detail — execution ID and state visible, Cancel absent.

**Workflow Detail page screenshot** (wf-detail-older-execution) Older completed ContinueAsNew epoch detail — execution ID and state visible, Cancel absent. 

zombor commented 2026-06-29 16:31:17 +00:00

Author

Owner

Copy link

CODE REVIEW — bookshelf-qnmg

Phase 0: DEMO Verification

No explicit DEMO block exists in the PR body or bead comments. This is a form-prefill UI fix whose observable behavior requires a live browser session, not a CLI-runnable command. CI E2E Browser tests are green (87c73a0b — all 6 check contexts: success). Treating as PARTIAL with valid reason; proceeding.

---

Phase 1: Spec Compliance

All four scenarios from the bead description are covered: (a) cd with volume_name; (b) cd present, volume_name empty, book.Series set; (c) cd == nil, book.Series set; (d) cd == nil, no Series, filename-only. Edge cases (empty stem, all-zero issue, fractional series number, no CBX files) are present. Handler change is localized to the Show-handler prefill block — List handler and save path are untouched. ✓

---

Phase 2: Code Quality

---

[MAJOR] internal/books/export_test.go:101-107 — three new var-exports widen the white-box shim when the public wrapper already exists

The file adds:

var ParseComicFilenameParts = parseComicFilenameParts
var PrimaryComicFileStem   = primaryComicFileStem
var FormatSeriesNumber     = formatSeriesNumber

All three private functions are reachable through the ComicSearchPrefill wrapper that is also added to this file. Every test case in the parseComicFilenameParts and primaryComicFileStem describes can be driven via ComicSearchPrefill(nil, books.Book{Files: []books.BookFile{{BookType: "cbx", FileName: "<stem>.cbz"}}}). formatSeriesNumber(2.1) maps to ComicSearchPrefill(nil, books.Book{Series: &books.Series{Number: 2.1}}). Per project-conventions.md: "Cover an unexported helper by driving the public caller that reaches it." The ComicSearchPrefill wrapper is exactly that public caller — adding three additional raw-var exports alongside it contradicts the rule the wrapper was added to satisfy. Fix: remove the three var-exports and consolidate their edge-case test contexts into the existing comicSearchPrefill — layered fallback Describe block, driven through ComicSearchPrefill.

---

[MINOR] internal/books/comic_search_prefill_test.go:43 — context description contradicts the fixture

The context is titled "with a long title and no leading scan-id" but the fixture is "15759 Tales of the Legion of Super-Heroes 354", which does have a leading scan-id (15759). The regex correctly strips it and the assertions are right; only the description is wrong. Rename to "with a scan-id prefix and a long multi-word title".

---

[MINOR] internal/books/comic_search_prefill.go:30-34 — the cd == nil guard is dead code in the production call path

handler.go:1244-1246 guarantees cd is always non-nil for isComic books:

if cd == nil {
    cd = &comicDisplay{}
}

This runs before comicSearchPrefill is ever called, so comicSearchPrefill will never receive a nil cd from handler.go. The fix is still correct (the empty &comicDisplay{} has empty VolumeName/IssueNumber/CoverDate which triggers the fallback chain), but test scenarios (c) and (d) exercise a production-unreachable code path. Harmless, but worth noting so the next reader doesn't add a second nil-guard thinking it is needed.

---

[MINOR] internal/books/comic_search_prefill.go:43-46 — Series.Number != 0 silently skips issue #0

if issue == "" && book.Series != nil && book.Series.Number != 0 {

Issue #0 exists in comics (preview/zero issues). Because Series.Number is float64, zero is the Go zero-value with no "not set" distinction. A book at issue #0 falls through to filename parsing instead. This is a pre-existing schema limitation (Grimmory float64 encoding), not introduced here, so it is not a blocker — but a comment explaining the zero-value skip would prevent future confusion.

---

REVIEW VERDICT: 0 blocker, 1 major, 3 minor

## CODE REVIEW — bookshelf-qnmg ### Phase 0: DEMO Verification No explicit `DEMO` block exists in the PR body or bead comments. This is a form-prefill UI fix whose observable behavior requires a live browser session, not a CLI-runnable command. CI E2E Browser tests are **green** (`87c73a0b` — all 6 check contexts: success). Treating as PARTIAL with valid reason; proceeding. --- ### Phase 1: Spec Compliance All four scenarios from the bead description are covered: (a) `cd` with `volume_name`; (b) `cd` present, `volume_name` empty, `book.Series` set; (c) `cd == nil`, `book.Series` set; (d) `cd == nil`, no Series, filename-only. Edge cases (empty stem, all-zero issue, fractional series number, no CBX files) are present. Handler change is localized to the Show-handler prefill block — List handler and save path are untouched. ✓ --- ### Phase 2: Code Quality --- [MAJOR] `internal/books/export_test.go:101-107` — three new var-exports widen the white-box shim when the public wrapper already exists The file adds: ```go var ParseComicFilenameParts = parseComicFilenameParts var PrimaryComicFileStem = primaryComicFileStem var FormatSeriesNumber = formatSeriesNumber ``` All three private functions are reachable through the `ComicSearchPrefill` wrapper that is *also* added to this file. Every test case in the `parseComicFilenameParts` and `primaryComicFileStem` describes can be driven via `ComicSearchPrefill(nil, books.Book{Files: []books.BookFile{{BookType: "cbx", FileName: "<stem>.cbz"}}})`. `formatSeriesNumber(2.1)` maps to `ComicSearchPrefill(nil, books.Book{Series: &books.Series{Number: 2.1}})`. Per `project-conventions.md`: "Cover an unexported helper by driving the **public caller** that reaches it." The `ComicSearchPrefill` wrapper is exactly that public caller — adding three additional raw-var exports alongside it contradicts the rule the wrapper was added to satisfy. Fix: remove the three var-exports and consolidate their edge-case test contexts into the existing `comicSearchPrefill — layered fallback` Describe block, driven through `ComicSearchPrefill`. --- [MINOR] `internal/books/comic_search_prefill_test.go:43` — context description contradicts the fixture The context is titled `"with a long title and no leading scan-id"` but the fixture is `"15759 Tales of the Legion of Super-Heroes 354"`, which **does** have a leading scan-id (`15759`). The regex correctly strips it and the assertions are right; only the description is wrong. Rename to `"with a scan-id prefix and a long multi-word title"`. --- [MINOR] `internal/books/comic_search_prefill.go:30-34` — the `cd == nil` guard is dead code in the production call path `handler.go:1244-1246` guarantees `cd` is always non-nil for `isComic` books: ```go if cd == nil { cd = &comicDisplay{} } ``` This runs before `comicSearchPrefill` is ever called, so `comicSearchPrefill` will never receive a nil `cd` from `handler.go`. The fix is still correct (the empty `&comicDisplay{}` has empty `VolumeName`/`IssueNumber`/`CoverDate` which triggers the fallback chain), but test scenarios (c) and (d) exercise a production-unreachable code path. Harmless, but worth noting so the next reader doesn't add a second nil-guard thinking it is needed. --- [MINOR] `internal/books/comic_search_prefill.go:43-46` — `Series.Number != 0` silently skips issue #0 ```go if issue == "" && book.Series != nil && book.Series.Number != 0 { ``` Issue #0 exists in comics (preview/zero issues). Because `Series.Number` is `float64`, zero is the Go zero-value with no "not set" distinction. A book at issue #0 falls through to filename parsing instead. This is a pre-existing schema limitation (Grimmory float64 encoding), not introduced here, so it is not a blocker — but a comment explaining the zero-value skip would prevent future confusion. --- REVIEW VERDICT: 0 blocker, 1 major, 3 minor

zombor added 1 commit 2026-06-29 16:39:25 +00:00

fix(books): address review — cover helpers via public ComicSearchPrefill (bookshelf-qnmg) ... 7e36e36b79

Remove three direct var-exports (ParseComicFilenameParts, PrimaryComicFileStem,
FormatSeriesNumber) that were testing unexported helpers directly. Consolidate
all edge-case coverage (zero-padded issue, all-zero "000"→"0", no trailing issue,
fractional Series.Number) into the existing comicSearchPrefill layered-fallback
Describe block using the public ComicSearchPrefill wrapper.

Add defensive-guard and Grimmory float64 zero-as-unset comments to production
code per review minors.

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

forgejo-actions commented 2026-06-29 16:42:09 +00:00

Copy link

Workflow Detail page screenshot (wf-detail-older-execution)

Older completed ContinueAsNew epoch detail — execution ID and state visible, Cancel absent.

**Workflow Detail page screenshot** (wf-detail-older-execution) Older completed ContinueAsNew epoch detail — execution ID and state visible, Cancel absent. 

forgejo-actions commented 2026-06-29 16:42:36 +00:00

Copy link

Recompute Match Score — kebab open screenshot (recompute-match-score-kebab-open)

**Recompute Match Score — kebab open screenshot** (recompute-match-score-kebab-open) 

zombor force-pushed bd-bookshelf-qnmg from 7e36e36b79 to 0a8020c414 2026-06-29 16:58:29 +00:00 Compare

forgejo-actions commented 2026-06-29 17:00:45 +00:00

Copy link

Workflow Detail page screenshot (wf-detail-older-execution)

Older completed ContinueAsNew epoch detail — execution ID and state visible, Cancel absent.

**Workflow Detail page screenshot** (wf-detail-older-execution) Older completed ContinueAsNew epoch detail — execution ID and state visible, Cancel absent. 

forgejo-actions commented 2026-06-29 17:00:49 +00:00

Copy link

Recompute Match Score — kebab open screenshot (recompute-match-score-kebab-open)

**Recompute Match Score — kebab open screenshot** (recompute-match-score-kebab-open) 

zombor merged commit d9c6e12822 into main 2026-06-29 17:02:35 +00:00

zombor referenced this pull request from a commit 2026-06-29 17:02:39 +00:00

Merge pull request 'fix(books): layered fallback for comic Fetch Metadata SERIES/issue prefill (bookshelf-qnmg)' (#847) from bd-bookshelf-qnmg 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

2 participants

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!847

No description provided.