feat(bookdrop): replace Import Metadata modal with inline expandable row [shot:import-metadata] (bookshelf-qgd90.2) #1230
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "bd-bookshelf-qgd90.2"
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
#fetchdirectlysetFetchedData(candidate)onbookdrop-file-editorbfe-panel--no-fetchedhides right column + arrows when no fetched data (no DOM toggle, JS removes the class on first fetch)fetchedCoverImg/fetchedCoverPlaceholdertargetscomicDetailsSection) when candidate carriescomicdataimg.src = urlnot innerHTML; CSS classes not inlinestyle=Test plan
make test)make build,go build -tags e2e ./e2e/...,go build -tags integration ./internal/...)Closes bead bookshelf-qgd90.2 on merge.
Import Metadata inline screenshot (bookdrop-import-metadata-inline)
Security Review — PR #1230 (bookshelf-qgd90.2)
Bookdrop fetched-metadata moved from modal to inline expandable row.
Findings: none.
Verification:
img.src = urlDOM property(
_populateFetchedCover, controller L274), neverinnerHTMLconcat. Identicalto the pre-move modal behavior (old
bookdrop_import_metadata_controller.jsalso did
img.src = c.comic.cover_url). No SafeURL regression: there was noJS-side SafeURL to lose (no
SafeURL/urlutilhelper exists understatic/js/),and
img.srcdoes not executejavascript:/data:scheme URIs. No XSS sink.rendered via
textContent:_addComicFieldusesdt.textContent/dd.textContent(L385-388);
_populateFetchedFieldsusesspan.textContent/authSpan.textContent(L318/L323). DOM built with
document.createElement+replaceChildren. NoinnerHTMLstring-building reintroduced by the move tosetFetchedData.handler/route change. Bookdrop's
BookdropRequiredgate is unchanged.is a browser image load of a provider-returned URL (same as before).
REVIEW VERDICT: 0 blocker, 0 major, 0 minor
Code Review: #1230 — BookDrop Import Metadata Modal → Inline
Phase 1: Spec Compliance
Modal Removal: Complete ✓
<div class="modal-overlay" id="import-metadata-modal">) fully removed from templateopen(),close(),onOverlayClick()removed from bookdrop-import-metadata controllerselectedCount,fetchBtn,applyBtn,resultsArea,overlay#open→#fetch, ID:#import-metadata-open-btn→#import-metadata-fetch-btnapplyUrlValuedata attribute removed; apply endpoint no longer referencedInline Expand/Compare Routing: Correct ✓
bfe-panel--no-fetchedsetFetchedData(candidate)public method (bookdrop-file-editor) called by import-metadata on fetch result_onFetchDone(): querySelector +getControllerForElementAndIdentifier()finds row by proposal_id, callssetFetchedData(c)if (c.error) return;CSS + Styling: CSP-Compliant ✓
style=attributes.bfe-panel--no-fetchedmodifier hides right column + arrows + copy-bar; grid collapses to 1 column<dl>grid layout with proper gap/column rules.btn,.form-control,.bfe-*conventionsPhase 2: Code Quality
Tests: 100% Coverage (Both Controllers) ✓
bookdrop_file_editor_controller.test.js: 60+ specs
setFetchedData()removesbfe-panel--no-fetched, opens panel, shows badge/notice<dl>when candidate.comic exists, hidden for ebooksfetchedCoverImg,fetchedCoverPlaceholder,comicDetailsSpacer,comicDetailsArrow,comicDetailsSectionbookdrop_import_metadata_controller.test.js: 40+ specs
setFetchedData()on matching file-editor controller (via spied getControllerForElementAndIdentifier)package *_test) ✓E2E Browser Test: Cross-Controller Wiring ✓
Error Paths: Comprehensive ✓
Logic: No Race Conditions ✓
No Security Issues ✓
Conventions Followed ✓
REVIEW VERDICT: 0 blocker, 0 major, 0 minor
This is a well-executed UX refactor: modal → inline expandable rows. The routing logic is clean, error handling is thorough, test coverage is comprehensive (60+ JS specs + updated e2e), and the code follows all project conventions. No correctness issues found.
Inline expanded row — comic metadata + side-by-side covers (bookshelf-qgd90.2)
Shows: two-column compare layout, original cover placeholder (left) + fetched comic data (right), comic details section with Issue #1, Volume, Cover Date, Characters, Creators.
Inline expanded row — comic metadata + side-by-side covers (bookshelf-qgd90.2)
Shows: two-column compare layout, original cover placeholder (left) + fetched comic data (right), comic details section with Issue #1, Volume, Cover Date, Characters, Creators.
UI Review — bookshelf-qgd90.2 (PR #1230)
Screenshot verdict
Read the PNG directly. The rendered inline expand panel is clean and readable:
← Copy allbar sits cleanly above the columns. The⚠unreviewed notice banner is visible and prominent.Canonical-component reuse
btn btn-small btn-ghost/btn btn-small btn-primarythroughout — canonical.form-control bfe-input— uses canonicalform-control;bfe-inputis a modifier, not a replacement.var(--fg-muted),var(--fg),var(--border),var(--space-2)in the new CSS additions — token-based.bfe-panel--no-fetched,bfe-fetched-cover-img,bfe-comic-details) are genuinely new layout needs with no canonical equivalent — a CSS state modifier for the single/two-column switch, an inline cover image style, and a comic-specific dt/dd grid. None duplicates an existing canonical class..modal-overlay/.modal-dialog/.modal-header/.modal-footercanonical modal structure is removed in this diff, not bypassed. Correct.No inline style=
Confirmed — zero
style=attributes in the changed template. Visibility toggling useshiddenattribute and CSS classes only. CSP-safe.Findings
REVIEW VERDICT: 0 blocker, 0 major, 2 minor
Import Metadata inline screenshot (bookdrop-import-metadata-inline)
bd5f7073aeca19c6eb18Import Metadata inline screenshot (bookdrop-import-metadata-inline)