feat(series): show missing entry gaps as ghost cards (bookshelf-4ztq.1) #694
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "bd-bookshelf-4ztq.1"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Summary
computeMissingEntries(pure function) that derives interior and trailing series gaps from ownedseries_numbervalues vsseries_total— interior-only when total is unknown (0), full range including tail whenseries_total > max ownedbuildSeriesEntriesto merge owned books + ghost placeholders into a single sorted[]SeriesEntryslice for the templateSeriesDetailgainsMissingEntries,TotalKnown, andEntriesfields; populated inbuildSeriesDetail— no schema changeseries_number; header shows "OWN N OF M" summarycover-card--ghostandbook-card-tile--ghostCSS (canonicalcover-cardclasses extended, no bespoke system)ownedNumbersscoped touserLibraryIDsfail-closed, same as existingbooksInSeriesqueryTest plan
make test— all 260 series tests passmake coverage— gate passes (100% on internal/, wire.go excluded)make build— clean compilegolangci-lint run ./internal/series/...— 0 issuesCloses bead bookshelf-4ztq.1 on merge.
UI Screenshots
Grid view — owned books + ghost cards interleaved, sorted by series number. "Own N of M" summary in heading.
Book List tab — ghost rows with MISSING badge interleaved with owned rows.
Seeded: Davina Ravine Psychic Crime Thriller Series (series_total=8), owned #4 and #6, showing gaps #5, #7, #8.
Security Review — PR #694 (bd-bookshelf-4ztq.1): Series detail ghost cards
Reviewed per
.claude/rules/review-standard.md. Diff-only — no tests re-run.[MAJOR] internal/series/store.go:354 — BooksInSeries missing fail-closed guard for empty
userLibraryIDsBooksInSeriesapplies the library filter only whenlen(userLibraryIDs) > 0. WhenGetUserLibraryIDsreturns a non-nil empty slice (the documented fail-closed signal for "authenticated user with no library access"), the library filter is silently omitted and all books in the series are returned unscoped — leaking the series contents to any authenticated user regardless of library access.The correct pattern (already used in three other functions in this file, e.g. lines 77–80 and 211) is:
Fix: add the
nil != nil && len == 0 → return nil, nilguard before the existinglen > 0branch, exactly asListSeriesCoversdoes at line 211.[MAJOR] internal/series/series.go:76–86 — Unbounded
seriesTotalloop; no cap on ghost-card countcomputeMissingEntriesloops fromrangeMintorangeMax = max(maxOwned, seriesTotal)with no upper bound.series_totalis a plainINTcolumn, user-writable viaPOST /books/{id}/metadata(series_totalform field —metadata_handler.go:289) with no validation. An authenticated user who edits any book toseries_total = 2,000,000,000and then views its series page will cause the server to allocate a 2-billion-element[]MissingEntryslice and render 2 billion ghost cards, consuming all available memory and crashing the process (DoS).Fix: cap the effective range before the loop, e.g.:
(Or clamp
seriesTotalto a reasonable ceiling, e.g. 1000, inbuildSeriesDetailbefore it is passed tocomputeMissingEntries.)[MINOR] templates/pages/series_show.html — Ghost card
aria-labelusesprintf "%.0f"onSeriesNumberSeriesNumberis afloat64rendered as an integer viaprintf "%.0f". Values that do not fit in the displayed range (or rounding artefacts for large floats) could produce misleading labels. This is cosmetic-only (no security impact) sincehtml/templateauto-escapes the output. No action required before merge, but worth noting.XSS — CLEAR: All user-controlled strings (
series name,book title,author name) render via{{.Title}}/{{$a.Name}}throughhtml/template's auto-escaping. Notemplate.HTMLcast orsafeHTMLcall found in the diff. Ghost-card series numbers are integers rendered viaprintf "%.0f"— not user-supplied strings. No XSS risk.SQL injection — CLEAR:
BooksInSeriesbuilds?placeholder strings programmatically and passes all values asargs— no raw string interpolation of user data.Series existence leak — CLEAR:
GetDetailreturnsmiddleware.ErrNotFoundwhenbooksInSeriesreturns zero rows (after the library filter is applied), so a user with no library access to a series cannot discover its existence via the detail page.REVIEW VERDICT: 0 blocker, 2 major, 1 minor
CODE REVIEW: NOT APPROVED
Phase 0: DEMO Verification
No runnable CLI DEMO block exists in the bead comments. The implementer posted two rendered screenshots (
series_gaps_grid.png,series_gaps_list.png) as the visual proof. Screenshots were downloaded and inspected. The screenshots actually expose the BLOCKER rather than verifying correctness: the seeded scenario (owned #4 and #6 in an 8-book series) shows ghost cards only for #5, #7, #8 — entries #1, #2, #3 are entirely absent from both the grid and the book-list tab. The heading shows "OWN 2 OF 8" confirmingseries_total=8is known, so gaps #1–#3 should be visible but are not.Phase 1 & 2: Findings
[BLOCKER] internal/series/series.go:134–138 —
computeMissingEntriesstarts atminOwned, not1, whenseriesTotal > maxOwnedThe docstring at line 40 explicitly states the
totalKnownstrategy is "gaps in 1..seriesTotal". The implementation setsrangeMin = minOwnedunconditionally (line 134) and only extendsrangeMaxtoseriesTotal(lines 136–138). WhenseriesTotal > maxOwned, the loop runsminOwned..seriesTotal— so any entry before the first owned book is silently dropped.Concrete example: series_total=8, owned={#4, #6}. Expected missing: #1,#2,#3,#5,#7,#8. Actual: #5,#7,#8. The screenshots confirm this exact bug — #1, #2, #3 are invisible. The test at line 340 (
"returns interior gaps and tail when seriesTotal > maxOwned") usesownedNumbers=[1,3]whereminOwned=1, so it cannot catch this.Fix: when
seriesTotal > maxOwned, setrangeMin = 1instead ofminOwned. Also add a test:ownedNumbers=[3,5], seriesTotal=7→ expected missing[1,2,4,6,7].[MAJOR] internal/series/service.go:243–250 —
buildSeriesEntriessort comparator violates strict-weak-ordering for two owned books at the sameseries_numberThe comparator at line 248:
return !entries[i].Missing. When bothiandjare owned books (Missing=false) at the sameseries_number,less(i,j) = trueANDless(j,i) = true— both claim to be less than each other. This violates antisymmetry. Go'ssort.SliceStablewon't panic but produces undefined ordering for that pair on every page render (non-deterministic display output for users with duplicate series numbers in their metadata).Fix: add a stable tiebreaker — when
ni == njand neither is missing, tiebreak on book ID:return entries[i].Book.ID < entries[j].Book.ID. Guard the nil-Book case (ghost entries haveBook=nil).[MINOR] internal/series/service.go:170 —
TotalKnownsettruewheneverseriesTotal > 0, even whenseriesTotal <= maxOwnedtotalKnown := seriesTotal > 0(line 170) fires regardless of whetherseriesTotalis actually used as the range ceiling (which only happens whenseriesTotal > maxOwned). Result: when a user owns books numbered pastseriesTotal(e.g., own #1 and #5, seriesTotal=3), the template renders "own 2 of 3" while the ghost cards show gaps through #5. The docstring (line 41) saystotalKnownmeans "seriesTotal > 0 and > max owned", but the code doesn't match. Low real-world impact (requires bad metadata), but the field name is misleading and the label can be wrong.Fix:
totalKnown := seriesTotal > 0 && seriesTotal > maxOwned(requiresmaxOwnedto be accessible at that point inbuildSeriesDetail— it already is, sincecomputeMissingEntriesreceives it).Phase 0 summary
The screenshot DEMO, while not a runnable command, functions as a visual verification. It reveals the BLOCKER directly: entries before the first owned book are not shown even when the total is known.
REVIEW VERDICT: 1 blocker, 1 major, 1 minor
Security Review — PR #694 (bd-bookshelf-4ztq.1): Series detail ghost cards
Reviewed per
.claude/rules/review-standard.md. Diff-only — no tests re-run.[MAJOR] internal/series/store.go:354 — BooksInSeries missing fail-closed guard for empty
userLibraryIDsBooksInSeriesapplies the library filter only whenlen(userLibraryIDs) > 0. WhenGetUserLibraryIDsreturns a non-nil empty slice (the documented fail-closed signal for “authenticated user with no library access”), the library filter is silently omitted and all books in the series are returned unscoped — leaking the series contents to any authenticated user regardless of library access.The correct pattern (already used in three other functions in this file, e.g. lines 77–80 and 211) is:
Fix: add the
nil != nil && len == 0 -> return nil, nilguard before the existinglen > 0branch, exactly asListSeriesCoversdoes at line 211.[MAJOR] internal/series/series.go:76-86 — Unbounded
seriesTotalloop; no cap on ghost-card countcomputeMissingEntriesloops fromrangeMintorangeMax = max(maxOwned, seriesTotal)with no upper bound.series_totalis a plain INT column, user-writable viaPOST /books/{id}/metadata(series_totalform field —metadata_handler.go:289) with no validation. An authenticated user who edits any book toseries_total = 2,000,000,000and then views its series page will cause the server to allocate a 2-billion-element[]MissingEntryslice and render 2 billion ghost cards, consuming all available memory and crashing the process (DoS).Fix: cap the effective range before the loop, e.g.:
Or clamp
seriesTotalto a reasonable ceiling (e.g. 1000) inbuildSeriesDetailbefore passing it tocomputeMissingEntries.[MINOR] templates/pages/series_show.html — Ghost card
aria-labelusesprintf %.0fonSeriesNumberSeriesNumberis afloat64rendered as an integer viaprintf "%.0f". Values that do not fit in the displayed range (or rounding artefacts for large floats) could produce misleading labels. This is cosmetic-only (no security impact) sincehtml/templateauto-escapes the output. No action required before merge, but worth noting.XSS — CLEAR: All user-controlled strings (
series name,book title,author name) render via{{.Title}}/{{$a.Name}}throughhtml/template's auto-escaping. Notemplate.HTMLcast orsafeHTMLcall found in the diff. Ghost-card series numbers are integers rendered viaprintf "%.0f"— not user-supplied strings. No XSS risk.SQL injection — CLEAR:
BooksInSeriesbuilds?placeholder strings programmatically and passes all values asargs— no raw string interpolation of user data.Series existence leak — CLEAR:
GetDetailreturnsmiddleware.ErrNotFoundwhenbooksInSeriesreturns zero rows (after the library filter is applied), so a user with no library access to a series cannot discover its existence via the detail page.REVIEW VERDICT: 0 blocker, 2 major, 1 minor
CODE REVIEW: NOT APPROVED
Phase 0: DEMO Verification
No runnable CLI DEMO block exists in the bead comments. The implementer posted two rendered screenshots as the visual proof. Screenshots were downloaded and inspected. The screenshots actually expose the BLOCKER rather than verifying correctness: the seeded scenario (owned #4 and #6 in an 8-book series) shows ghost cards only for #5, #7, #8 — entries #1, #2, #3 are entirely absent from both the grid and the book-list tab. The heading shows "OWN 2 OF 8" confirming series_total=8 is known, so gaps #1–#3 should be visible but are not.
Phase 1 & 2: Findings
[BLOCKER] internal/series/series.go:134-138 — computeMissingEntries starts at minOwned, not 1, when seriesTotal > maxOwned
The docstring at line 40 explicitly states the totalKnown strategy is "gaps in 1..seriesTotal". The implementation sets rangeMin = minOwned unconditionally (line 134) and only extends rangeMax to seriesTotal (lines 136–138). When seriesTotal > maxOwned, the loop runs minOwned..seriesTotal — so any entry before the first owned book is silently dropped.
Concrete example: series_total=8, owned={#4, #6}. Expected missing: #1,#2,#3,#5,#7,#8. Actual: #5,#7,#8. The screenshots confirm this exact bug — #1, #2, #3 are invisible. The test at line 340 ("returns interior gaps and tail when seriesTotal > maxOwned") uses ownedNumbers=[1,3] where minOwned=1, so it cannot catch this.
Fix: when seriesTotal > maxOwned, set rangeMin = 1 instead of minOwned. Also add a test: ownedNumbers=[3,5], seriesTotal=7 → expected missing [1,2,4,6,7].
[MAJOR] internal/series/service.go:243-250 — buildSeriesEntries sort comparator violates strict-weak-ordering for two owned books at the same series_number
The comparator at line 248:
return !entries[i].Missing. When both i and j are owned books (Missing=false) at the same series_number, less(i,j) = true AND less(j,i) = true — both claim to be less than each other. This violates antisymmetry. Go's sort.SliceStable won't panic but produces undefined ordering for that pair on every page render (non-deterministic display for users with duplicate series numbers in their metadata).Fix: add a stable tiebreaker — when ni == nj and neither is missing, tiebreak on book ID:
entries[i].Book.ID < entries[j].Book.ID. Guard the nil-Book case (ghost entries have Book=nil).[MINOR] internal/series/service.go:170 — TotalKnown set true whenever seriesTotal > 0, even when seriesTotal <= maxOwned
totalKnown := seriesTotal > 0fires regardless of whether seriesTotal is actually used as the range ceiling (which only happens when seriesTotal > maxOwned). When a user owns books numbered past seriesTotal (e.g., own #1 and #5, seriesTotal=3), the template renders "own 2 of 3" while ghost cards show gaps through #5. The docstring at line 41 says totalKnown means "seriesTotal > 0 AND > max owned", but the code doesn't match.Fix:
totalKnown := seriesTotal > 0 && seriesTotal > maxOwned(maxOwned is already available in buildSeriesDetail at that point).REVIEW VERDICT: 1 blocker, 1 major, 1 minor
Security Review — PR #694 (bd-bookshelf-4ztq.1): Series detail ghost cards
Reviewed per
.claude/rules/review-standard.md. Diff-only — no tests re-run.[MAJOR] internal/series/store.go:354 — BooksInSeries missing fail-closed guard for empty
userLibraryIDsBooksInSeriesapplies the library filter only whenlen(userLibraryIDs) > 0. WhenGetUserLibraryIDsreturns a non-nil empty slice (the documented fail-closed signal for 'authenticated user with no library access'), the library filter is silently omitted and all books in the series are returned unscoped — leaking the series contents to any authenticated user regardless of library access.The correct pattern (already used in three other functions in this file, e.g. lines 77-80 and 211) is:
Fix: add the
nil check && len == 0 -> return nil, nilguard before the existinglen > 0branch, exactly asListSeriesCoversdoes at line 211.[MAJOR] internal/series/series.go:76-86 — Unbounded
seriesTotalloop; no cap on ghost-card countcomputeMissingEntriesloops fromrangeMintorangeMax = max(maxOwned, seriesTotal)with no upper bound.series_totalis a plain INT column, user-writable viaPOST /books/{id}/metadata(series_totalform field —metadata_handler.go:289) with no validation. An authenticated user who edits any book toseries_total = 2,000,000,000and then views its series page will cause the server to allocate a 2-billion-element[]MissingEntryslice and render 2 billion ghost cards, consuming all available memory and crashing the process (DoS).Fix: cap the effective range before the loop, e.g.:
Or clamp
seriesTotalto a reasonable ceiling (e.g. 1000) inbuildSeriesDetailbefore passing it tocomputeMissingEntries.[MINOR] templates/pages/series_show.html — Ghost card
aria-labelusesprintf %.0fonSeriesNumberSeriesNumberis afloat64rendered as an integer viaprintf "%.0f". Values that do not fit in the displayed range (or rounding artefacts for large floats) could produce misleading labels. This is cosmetic-only (no security impact) sincehtml/templateauto-escapes the output. No action required before merge, but worth noting.XSS — CLEAR: All user-controlled strings render via
html/templateauto-escaping. Notemplate.HTMLcast orsafeHTMLcall found in the diff. No XSS risk.SQL injection — CLEAR:
BooksInSeriesbuilds?placeholder strings programmatically and passes all values asargs— no raw string interpolation of user data.Series existence leak — CLEAR:
GetDetailreturnsmiddleware.ErrNotFoundwhenbooksInSeriesreturns zero rows (after the library filter is applied), so a user with no library access to a series cannot discover its existence via the detail page.REVIEW VERDICT: 0 blocker, 2 major, 1 minor
CODE REVIEW: NOT APPROVED
Phase 0: DEMO Verification
No runnable CLI DEMO block exists in the bead comments. The implementer posted two rendered screenshots as the visual proof. Screenshots were downloaded and inspected. The screenshots actually expose the BLOCKER rather than verifying correctness: the seeded scenario (owned #4 and #6 in an 8-book series) shows ghost cards only for #5, #7, #8 — entries #1, #2, #3 are entirely absent from both the grid and the book-list tab. The heading shows "OWN 2 OF 8" confirming series_total=8 is known, so gaps #1–#3 should be visible but are not.
Phase 1 & 2: Findings
[BLOCKER] internal/series/series.go:134-138 — computeMissingEntries starts at minOwned, not 1, when seriesTotal > maxOwned
The docstring at line 40 explicitly states the totalKnown strategy is "gaps in 1..seriesTotal". The implementation sets rangeMin = minOwned unconditionally (line 134) and only extends rangeMax to seriesTotal (lines 136–138). When seriesTotal > maxOwned, the loop runs minOwned..seriesTotal — so any entry before the first owned book is silently dropped.
Concrete example: series_total=8, owned={#4, #6}. Expected missing: #1,#2,#3,#5,#7,#8. Actual: #5,#7,#8. The screenshots confirm this exact bug — #1, #2, #3 are invisible. The test at line 340 ("returns interior gaps and tail when seriesTotal > maxOwned") uses ownedNumbers=[1,3] where minOwned=1, so it cannot catch this.
Fix: when seriesTotal > maxOwned, set rangeMin = 1 instead of minOwned. Also add a test: ownedNumbers=[3,5], seriesTotal=7 → expected missing [1,2,4,6,7].
[MAJOR] internal/series/service.go:243-250 — buildSeriesEntries sort comparator violates strict-weak-ordering for two owned books at the same series_number
The comparator at line 248:
return !entries[i].Missing. When both i and j are owned books (Missing=false) at the same series_number, less(i,j) = true AND less(j,i) = true — both claim to be less than each other. This violates antisymmetry. Go's sort.SliceStable won't panic but produces undefined ordering for that pair on every page render (non-deterministic display for users with duplicate series numbers in their metadata).Fix: add a stable tiebreaker — when ni == nj and neither is missing, tiebreak on book ID:
entries[i].Book.ID < entries[j].Book.ID. Guard the nil-Book case (ghost entries have Book=nil).[MINOR] internal/series/service.go:170 — TotalKnown set true whenever seriesTotal > 0, even when seriesTotal <= maxOwned
totalKnown := seriesTotal > 0fires regardless of whether seriesTotal is actually used as the range ceiling (which only happens when seriesTotal > maxOwned). When a user owns books numbered past seriesTotal (e.g., own #1 and #5, seriesTotal=3), the template renders "own 2 of 3" while ghost cards show gaps through #5. The docstring at line 41 says totalKnown means "seriesTotal > 0 AND > max owned", but the code doesn't match.Fix:
totalKnown := seriesTotal > 0 && seriesTotal > maxOwned(maxOwned is already available in buildSeriesDetail at that point).REVIEW VERDICT: 1 blocker, 1 major, 1 minor
CODE REVIEW: NOT APPROVED
Phase 0: DEMO Verification
No runnable CLI DEMO block exists in the bead comments. The implementer posted two rendered screenshots as the visual proof. Screenshots were downloaded and inspected. The screenshots actually expose the BLOCKER rather than verifying correctness: the seeded scenario (owned #4 and #6 in an 8-book series) shows ghost cards only for #5, #7, #8 — entries #1, #2, #3 are entirely absent from both the grid and the book-list tab. The heading shows "OWN 2 OF 8" confirming series_total=8 is known, so gaps #1–#3 should be visible but are not.
Phase 1 & 2: Findings
[BLOCKER] internal/series/series.go:134-138 — computeMissingEntries starts at minOwned, not 1, when seriesTotal > maxOwned
The docstring at line 40 explicitly states the totalKnown strategy is "gaps in 1..seriesTotal". The implementation sets rangeMin = minOwned unconditionally (line 134) and only extends rangeMax to seriesTotal (lines 136–138). When seriesTotal > maxOwned, the loop runs minOwned..seriesTotal — so any entry before the first owned book is silently dropped.
Concrete example: series_total=8, owned={#4, #6}. Expected missing: #1,#2,#3,#5,#7,#8. Actual: #5,#7,#8. The screenshots confirm this exact bug — #1, #2, #3 are invisible. The test at line 340 ("returns interior gaps and tail when seriesTotal > maxOwned") uses ownedNumbers=[1,3] where minOwned=1, so it cannot catch this.
Fix: when seriesTotal > maxOwned, set rangeMin = 1 instead of minOwned. Also add a test: ownedNumbers=[3,5], seriesTotal=7 → expected missing [1,2,4,6,7].
[MAJOR] internal/series/service.go:243-250 — buildSeriesEntries sort comparator violates strict-weak-ordering for two owned books at the same series_number
The comparator at line 248:
return !entries[i].Missing. When both i and j are owned books (Missing=false) at the same series_number, less(i,j) = true AND less(j,i) = true — both claim to be less than each other. This violates antisymmetry. Go's sort.SliceStable won't panic but produces undefined ordering for that pair on every page render (non-deterministic display for users with duplicate series numbers in their metadata).Fix: add a stable tiebreaker — when ni == nj and neither is missing, tiebreak on book ID:
entries[i].Book.ID < entries[j].Book.ID. Guard the nil-Book case (ghost entries have Book=nil).[MINOR] internal/series/service.go:170 — TotalKnown set true whenever seriesTotal > 0, even when seriesTotal <= maxOwned
totalKnown := seriesTotal > 0fires regardless of whether seriesTotal is actually used as the range ceiling (which only happens when seriesTotal > maxOwned). When a user owns books numbered past seriesTotal (e.g., own #1 and #5, seriesTotal=3), the template renders "own 2 of 3" while ghost cards show gaps through #5. The docstring at line 41 says totalKnown means "seriesTotal > 0 AND > max owned", but the code doesn't match.Fix:
totalKnown := seriesTotal > 0 && seriesTotal > maxOwned(maxOwned is already available in buildSeriesDetail at that point).REVIEW VERDICT: 1 blocker, 1 major, 1 minor
Re-screenshot: leading gaps fix verified
Series detail for "The Wheel of Time" — owned books at positions #4 and #6 out of 8 total.
Before fix: ghost cards would only show #5, #7, #8 (interior + tail). Leading gaps #1, #2, #3 were silently dropped.
After fix: ghost cards correctly show #1, #2, #3 (leading) + #5 (interior) + #7, #8 (trailing) — OWN 2 OF 8.
7520518a8a30231f68f4