feat(books): fold More-in-Series + Files into Details bottom tabs, polish series scroller (bookshelf-7ahb0) #1088
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "bd-bookshelf-7ahb0"
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?
What changed
Restructures the book detail page to match Grimmory: the standalone More in Series and Files top-level tabs are folded into the Details page bottom sub-tab bar.
Top-level tab bar (before → after)
Details sub-tab bar (before → after)
Polish
.cover-card-series-numspan inside the.cover-card-imagediv (no inline style — CSP safe)home-book-stripwithscrollbar-width: thin— identical to the Similar Books scroller.series-panel__series-linkCSS (accent colour, hover underline) instead of a bare unstyled link; no inline style?tab=filesand?tab=seriesfall through to?tab=details(removed fromvalidBookTab) — no 404Files changed
internal/books/handler.go—validBookTabtrimmed,buildShowSubTabshelper added,SubTabswiredinternal/books/handler_test.go— updated tab-fallback tests + new SubTabs coveragetemplates/pages/books_show.html— top-level nav trimmed, two new sub-tab panels added, old panels removedstatic/css/main.css—.cover-card-series-numcorner badge +.series-panel__*stylese2e/browser/journey_book_detail_subtabs_test.go— new Ordered browser journeyTest plan
make test— all unit tests pass including new SubTabs handler testsmake coverage— 100% coverage gate passesmake e2e-policy-check— new browser test is an Ordered journey containermake test-policy-check— no white-box testsgo build -tags e2e ./...— e2e browser tests compileCloses bead bookshelf-7ahb0 on merge.
Security Review — PR #1088 (bookshelf-7ahb0)
Scope: Book detail page restructure — More in Series + Files moved from top-level tabs into the Details sub-tab bar; series-number badge added; series link styled.
Multi-user scoping
No scoping violations introduced. The
SeriesBooksandBook.Filesslices are populated by the existingShowhandler before rendering; the PR does not touch the data-fetch paths.listSeriesBooksis called withshowUserLibraryIDs(derived from the authenticated session's library memberships, fail-closed to[]int64{}), which is unchanged. The file download links remain at/books/{{$.Book.ID}}/file/{{.ID}}— the existing authz-guarded route — in both the new sub-tab location and the removed top-level panel. No cross-user leak, no ownership-check removal.XSS / template escaping
All new template output goes through
html/templateauto-escaping:{{.Title}}inalt=anddata-cover-img-title-value=— attribute context, auto-escaped.{{printf "%.4g" .SeriesNumber}}—printfreturns a plainstring; in HTML text node contexthtml/templateHTML-escapes it. Numeric float values cannot contain HTML special characters regardless.{{.FileName}},{{.BookType}},{{.ArchiveType}}— auto-escaped.{{.SeriesLink}}inhref=—buildSeriesLinkalways constructs a root-relative URL (/series/<url.PathEscape(name)>);html/templateallows/-prefixed relative paths and additionally attribute-escapes the value. Nojavascript:injection risk.{{$.Book.ID}}and{{.ID}}in URL paths — int64 values; numeric, no escaping concern.Inline
style=/ CSPNo
style=attributes added. The series-number badge (.cover-card-series-num) and series panel header (.series-panel__header,.series-panel__series-link) are all CSS class–based instatic/css/main.css. CSPstyle-src 'self'unaffected.No new unauthenticated surface
validBookTabnow excludes"files"and"series"(both fall through to"details"), which is correct — no authz gap. No new route or handler added.Findings
[MINOR] internal/books/handler_test.go:336 — HTTP call placed in
BeforeEachinstead ofJustBeforeEachThe new
Describe("Show handler SubTabs", ...)suite performs the actual handler invocation (HTTP GET +io.ReadAll) insideBeforeEach, violating the project convention thatBeforeEachis pure setup andJustBeforeEachis the single point of invocation. This makes it impossible to override setup in nestedContextblocks without re-running the call, and breaks the standard Ginkgo expectation-ordering guarantee. The fix is to move themakeServer+ HTTP call into aJustBeforeEach, reading into closure-scopedresp/body/respErrvars thatItblocks then assert on.REVIEW VERDICT: 0 blocker, 0 major, 1 minor
CODE REVIEW: APPROVED
Phase 0: DEMO
No DEMO block in bead comments — the bead is a spec-and-work list, not a runnable command. CI is green per requester context. Proceeding on green CI.
Phase 1: Spec Compliance
All six required work items are present: More in Series prepended as first sub-tab, Files folded into sub-tab bar, two top-level tabs removed from nav and from
validBookTab, "View full series page" link styled,.home-book-stripreused for the series scroller, and series number badge added in cover-corner position. All panels remain server-rendered in the single page response.Phase 2: Code Quality
[MINOR]
templates/pages/books_show.html:494— deadlinkCSS class in series link markup<a class="series-panel__series-link link">references a.linkutility class that does not exist anywhere inmain.css. All styling comes fromseries-panel__series-link. The dead class is harmless but misleading — removelinkfrom the class list or define the utility.[MINOR]
internal/books/handler.go:1147vstemplates/pages/books_show.html:491— sub-tab guard condition mismatchbuildShowSubTabsadds the "series" nav button only whenbook.Series != nil && book.Series.Name != "". The template panel uses{{if .Book.Series}}(nil check only). If aBook.Seriespointer is non-nil butNameis empty, the panel renders but its nav button is absent — an orphaned, inaccessible panel. Unlikely in production but logically inconsistent. Fix:{{if and .Book.Series .Book.Series.Name}}.[MINOR]
internal/books/handler_test.go:6217+— new SubTabs tests violate BeforeEach/JustBeforeEach conventionThe two
Contextblocks underDescribe("Show handler SubTabs")both make the HTTP call to the SUT insideBeforeEach(notJustBeforeEach). Perproject-conventions.md:BeforeEachis pure setup;JustBeforeEachcalls the system under test. Additionally,io.ReadAllerrors are silently discarded with_. Tests pass (CI green) but do not follow the canonical pattern — movemakeServer(getBook)and the HTTP GET into aJustBeforeEach, and handle theio.ReadAllerror.[MINOR]
internal/books/delete_file_handler.go:86,replace_content_handler.go:113,upload_handler.go:121,static/js/controllers/book_file_delete_controller.js:172,book_file_detach_controller.js:184,188— post-file-operation deep-links are now deadThese redirect/navigate to
?tab=files, which silently falls through to thedetailstab (first sub-tab active, not Files). After deleting, replacing, uploading, or detaching a file, users land on the Details panel rather than the Files sub-tab. The bead spec explicitly permitted "at minimum default to details without 404", so this is within spec — but these are known dead deep-links worth a follow-up bead.No issues found on:
style=attribute in template changes; badge uses.cover-card-series-numCSS classcover-card-imagealready hasposition: relative; overflow: hidden— badge absolute positioning works.home-book-strip,.status-badge,.btn,.table,--space-*tokens all reused; no bespoke parallel class systemrole="tab"on the sub-tab nav button was correctly removed from the old templateOrdered,BeforeAllseeds DB once,BeforeEachcallsrefreshPageTimeout(go-rod deadline reset), clicks sub-tabs, asserts panel visibility withEventually— all per specpackage books_test(black-box)?tab=seriesand?tab=fileshandler tests correctly updated to assert fall-through todetailsREVIEW VERDICT: 0 blocker, 0 major, 4 minor
Book detail: More in Series sub-tab active
Book detail: Files sub-tab active
UI Review — PR #1088 (bookshelf-7ahb0) · Details page sub-tab restructure
Screenshots read and reviewed (More in Series panel + Files panel). Source cross-checked against
static/css/main.cssandtemplates/pages/books_show.htmlonorigin/bd-bookshelf-7ahb0.What looks correct
.subtab-nav/.subtab-btn/.subtab-btn--activepattern with the accent-color underline indicator. Spacing and alignment match the existing Similar Books / Notes / Reading History tabs that were already on the page..home-book-stripclass — not a bespoke parallel scroller. Card sizing and spacing are visually consistent with the dashboard scrollers..cover-card/.cover-card-image/.cover-card-meta/.cover-card-titlethroughout. No bespoke card class.#2): positioned bottom-right of the cover image, readable, not overlapping the title. Sits insidecover-card-imagewhich isposition:relative— layout is sound..table,.btn,.btn-sm,.btn--secondary,.btn-danger-ghostthroughout. Delete button rendered in danger styling as expected. No bespoke button classes.style=attributes anywhere in the changed files.Findings
[MINOR]
static/css/main.css:1302—.cover-card-series-numuses hardcodedcolor: #fffandbackground: rgba(0, 0, 0, 0.65)instead of CSS variable tokenscolor: #fffshould becolor: var(--bg)(or a new--overlay-texttoken), andrgba(0,0,0,0.65)should follow the project token convention. The codebase has many existingrgba()literals for overlays, so this is consistent with the current codebase norm, but the#fffhard-code is a clear token miss. Fix:color: var(--bg);and optionally extract--overlay-bg: rgba(0,0,0,0.65)at:rootto match the one existing--danger-alphapattern.[MINOR]
static/css/main.css:1316—.series-panel__series-linkis partially redundant alongside the basea { color: var(--accent) }ruleThe bespoke class adds only
font-size: 0.875remandtext-decoration: nonebeyond what the baseaselector already provides. This is a lightweight touch (not a parallel class system), but it introduces a named class for two properties that could be handled by a shared utility (e.g.,font-size: 0.875remvia a.text-smutil) or dropped entirely sinceaalready suppresses underline by default in this stylesheet. Low-friction to simplify on a follow-up.REVIEW VERDICT: 0 blocker, 0 major, 2 minor
CODE REVIEW (re-review of fix commits): APPROVED
Focused re-review of the three fix commits (
73e62fb5,efbc0135,5db9e718) on top of the previously approved base.What was checked
73e62fb5fix(ui) — template + CSS fixes.linkclass removed from<a class="series-panel__series-link">as intended.{{if and .Book.Series .Book.Series.Name}}— matches the server-sidebuildShowSubTabscondition (series tab only appended whenbook.Series != nil && book.Series.Name != ""). Consistent.color: var(--fg)resolves to#e2e4ef(off-white) onbackground: rgba(0,0,0,0.65). Light on dark — correct. The bug wasvar(--bg)(#0f1117, near-black on near-black), which would have been invisible.efbc0135fix(test) — handler_test.go SubTabs correctionsBeforeEachtoJustBeforeEachin all threeContextblocks. ✓io.ReadAllerror captured intorespErr;elsebranch assigns theGeterror whenresp == nil. Correct perhttp.Clientcontract. ✓Contextadded;Expect(body, respErr).NotTo(...)folds the nil-error check. ✓books_test— black-box. ✓5db9e718fix(books): restore files sub-tab deep-linkSafety of
?subtab=param:sub_tab_controller.js:33–36—params.get("subtab")result is matched viael.dataset.panelId === requested(string equality against actual DOM panel elements). If no panel matches, falls back topanelTargets[0].dataset.panelId. The URL value is never used as a CSS selector or passed to innerHTML. No injection surface. ✓Interaction with outer
book_tabs_controller:book_tabs_controller._activateusesurl.searchParams.set("tab", panelId)— preserves?subtab=in the URL unchanged.sub_tab_controller.connect()runs after (inner controller, DOM order), reads?subtab=filescorrectly. No race or overwrite. ✓Default preserved: when no
?subtab=is present,panelTargets[0]is used. Covered by Vitest "shows the first panel on connect". ✓Nonexistent param:
find()returns undefined → falsy → fallback to first panel. Covered by Vitest "keeps the first panel active when ?subtab= param does not match any panel". ✓CSP-safe: no
style=attribute written, noeval.el.hiddenandclassList.toggleonly. ✓Redirect consistency: all five callsites updated to
?tab=details&subtab=files:delete_file_handler.go:86,replace_content_handler.go:113,upload_handler.go:120,book_file_delete_controller.js:172,book_file_detach_controller.js:184+188. ✓Server-side
?tab=detailsrenders details panel withouthidden(validBookTab["details"] == true, template:{{if ne .ActiveTab "details"}} hidden{{end}}), sosub_tab_controlleris connected immediately, not inside a hidden panel. ✓Browser e2e:
Orderedjourney,page = refreshPageTimeout(page)per-It timeout reset,Eventually(5s/100ms)polling#subtab-panel-files.hiddenproperty. Correct go-rod pattern. ✓[MINOR]
internal/books/upload_handler_test.go:232,internal/books/replace_content_handler_test.go:221— These two tests assertStatusSeeOtherbut not theLocationheader after the redirect URL changed to?tab=details&subtab=files.delete_file_handler_test.gowas updated to assert the new Location. The gap is pre-existing (these tests never checked Location on main either), but the fix commit left the test coverage inconsistent across the three handlers. Not a blocker — the browser e2e covers the full round-trip — but worth closing for symmetry.REVIEW VERDICT: 0 blocker, 0 major, 1 minor
5db9e718f68c6015263c