feat(books): replace book content in place (bookshelf-jmn7.7) #632

Merged

zombor merged 5 commits from bd-bookshelf-jmn7.7 into main 2026-06-19 12:58:44 +00:00

Conversation 6 Commits 5 Files changed 18 +1852 -50

zombor commented 2026-06-19 02:18:35 +00:00

Owner

Copy link

Summary

• Adds PUT /books/{id}/content?bookType={fileID} endpoint that atomically replaces an existing book file (temp + rename, per resilience-standard)
• DISK_TYPE=NETWORK guard returns 409; permission gate via canEditMetadata/admin; user scoped via user_library_mapping JOIN (multi-user-first)
• Updates current_hash + file_size_kb in book_file, enqueues cover regen best-effort, audits via ActionMetadataUpdated
• Adds "Replace" button per-file on the Books detail Files tab using a new Stimulus controller book-file-replace (CSP-safe, canonical CSS classes)

Test Plan

• Go unit tests: replace_content_handler_test.go, replace_content_service_test.go, replace_content_store_test.go (100% coverage)
• JS Vitest unit tests: book_file_replace_controller.test.js (23 cases covering open/cancel/file-validation/fetch/error paths)
• Browser e2e Ordered journey: journey_book_file_replace_test.go (shows Replace button, opens modal, Cancel closes modal, screenshot posted to PR)
• go build ./... clean; go test ./internal/books/... green; npx vitest run 940 tests pass

Closes bead bookshelf-jmn7.7 on merge.

## Summary - Adds `PUT /books/{id}/content?bookType={fileID}` endpoint that atomically replaces an existing book file (temp + rename, per resilience-standard) - DISK_TYPE=NETWORK guard returns 409; permission gate via `canEditMetadata`/admin; user scoped via `user_library_mapping` JOIN (multi-user-first) - Updates `current_hash` + `file_size_kb` in `book_file`, enqueues cover regen best-effort, audits via `ActionMetadataUpdated` - Adds "Replace" button per-file on the Books detail Files tab using a new Stimulus controller `book-file-replace` (CSP-safe, canonical CSS classes) ## Test Plan - [x] Go unit tests: `replace_content_handler_test.go`, `replace_content_service_test.go`, `replace_content_store_test.go` (100% coverage) - [x] JS Vitest unit tests: `book_file_replace_controller.test.js` (23 cases covering open/cancel/file-validation/fetch/error paths) - [x] Browser e2e Ordered journey: `journey_book_file_replace_test.go` (shows Replace button, opens modal, Cancel closes modal, screenshot posted to PR) - [x] `go build ./...` clean; `go test ./internal/books/...` green; `npx vitest run` 940 tests pass Closes bead bookshelf-jmn7.7 on merge.

zombor added 1 commit 2026-06-19 02:18:36 +00:00

feat(books): replace book content in place (bookshelf-jmn7.7) ... 3f49f95009

PUT /books/{id}/content?bookType={fileID} — streams request body over the
existing book file with atomic temp+rename replace (resilience-standard).
DISK_TYPE=NETWORK guard returns 409. Permission gate: canEditMetadata/admin.
User scoped via user_library_mapping JOIN (multi-user). Audits via
ActionMetadataUpdated. Updates current_hash + file_size_kb in book_file.
Enqueues cover regen best-effort. Adds "Replace" button on the book Files
tab using book-file-replace Stimulus controller (CSP-safe, canonical CSS).
100% unit test coverage (Go + JS Vitest). Browser e2e journey + screenshot.

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

zombor added 1 commit 2026-06-19 02:27:11 +00:00

fix(books): 100% coverage + fix browser e2e ambiguous .bdm-title selector ... e65944b5ba

- Add http.ErrNotSupported guard to replace handler's SetReadDeadline
  (matches upload handler), and tests for the warn-branch + ErrValidation
  status mapping → ReplaceContentHandler now 100%.
- Add failing-reader test exercising doReplaceStream's io.Copy error path
  → doReplaceStream now 100%.
- Rework the browser journey into a proper single-page Ordered journey
  (was fresh-incognito-per-It, an anti-pattern). Scope the .bdm-title
  assertion inside .bfr-overlay: the book detail page renders several
  .bdm-title elements (upload/delete modals), so the page-wide selector
  matched the wrong (empty) title.

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

zombor added 1 commit 2026-06-19 02:31:49 +00:00

test(books): add temp-file write-error case to replace service test ... 306ace2e27

Pre-closes the temp file so io.Copy returns a write error, exercising the
doReplaceStream write-failure path via a real write failure (complements
the read-error case).

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

zombor commented 2026-06-19 02:33:29 +00:00

Author

Owner

Copy link

Security Review — PR #632 (bookshelf-jmn7.7) · PUT /books/{id}/content

Per .claude/rules/review-standard.md.

---

Library-access auth (the primary concern)

PRESENT and correctly positioned. GetBookFileForReplace in replace_content_store.go enforces library access via:

JOIN user_library_mapping ulm ON ulm.library_id = b.library_id AND ulm.user_id = ?

The getFile(ctx, userID, bookID, fileID) call in the service happens before any filesystem operation. sql.ErrNoRows maps to middleware.ErrNotFound → HTTP 404, leaking no existence information. No bypass is possible by supplying a valid bookType from a library the user cannot access. This mirrors the pattern established on the detach (#628) and delete (#629) paths.

---

Findings

[MINOR] internal/books/replace_content_service.go:108-114 — DB updated before rename: DB/disk inconsistency window on rename failure
updateHashAndSize (line 108) commits the new hash+size to the DB before renameFile (line 113) completes. If rename fails, the DB advertises the new hash/size while the live file on disk is still the old content. The code comments "leave temp for manual recovery" which acknowledges this but does not log the discrepancy. The correct order (matching resilience-standard) is: rename first (atomic disk op succeeds or old file is untouched), then update DB. This is a minor correctness/consistency nit, not data loss — the old file bytes are always preserved — but it violates the principle that the DB should trail, not lead, a destructive disk mutation.

[MINOR] internal/books/delete_file_store.go vs replace_content_store.go — delete path lacks user_library_mapping JOIN
The existing GetBookFileForDelete query (used by DELETE /books/{id}/files/{fileID}) does not include a user_library_mapping JOIN — it scopes only to bookID. The replace path correctly adds the user scoping; the delete path has the pre-#628 gap. This is outside the PR's scope (the prior delete PR #629 may have addressed it at a different layer), noted here for awareness only.

[MINOR] static/js/controllers/book_file_replace_controller.js — data-book-file-replace-url-value is rendered directly from the template, which is correct (server controls the URL), but the filename value is inserted verbatim into textContent (not innerHTML), so XSS is not possible. No finding.

---

Positive confirmations

• Path traversal: CLEAN. FileSubPath comes from the book_file DB row (not user input). The service additionally verifies containment via filepath.Rel: relErr != nil || rel == ".." || strings.HasPrefix(rel, "../") — triple-checks that the resolved destination stays inside libraryRoot.
• Atomic write: CLEAN. os.CreateTemp → write → os.Rename is the correct pattern. On failure between write and rename the original file is untouched; temp is cleaned up.
• DISK_TYPE=NETWORK guard: CLEAN. Checked at the top of the service, before any I/O, returns 409.
• Body size cap: CLEAN. http.MaxBytesReader(w, r.Body, maxUploadBodyBytes) in the handler caps at MaxUploadBytes + 4096 (2 GB + small envelope). The service also uses io.LimitReader(src, MaxUploadBytes+1) and checks written > limit, returning ErrFileTooLarge → HTTP 413. Defense in depth.
• CSRF: CLEAN. The global middleware.CSRF middleware covers PUT as an unsafe method (isUnsafeMethod includes PUT). The Stimulus controller also sends X-CSRF-Token: csrfToken() in the fetch headers.
• Permission gate: CLEAN. Route is registered as g.EditMetadata(eh.Wrap(replaceContent)) — same gate as metadata-write operations.
• No new bypass: bookType (fileID) is parsed as an integer; no string injection surface. userIDFromRequest derives userID from the authenticated session only, not from request parameters.

---

REVIEW VERDICT: 0 blocker, 0 major, 2 minor

## Security Review — PR #632 (bookshelf-jmn7.7) · `PUT /books/{id}/content` Per `.claude/rules/review-standard.md`. --- ### Library-access auth (the primary concern) **PRESENT and correctly positioned.** `GetBookFileForReplace` in `replace_content_store.go` enforces library access via: ```sql JOIN user_library_mapping ulm ON ulm.library_id = b.library_id AND ulm.user_id = ? ``` The `getFile(ctx, userID, bookID, fileID)` call in the service happens **before** any filesystem operation. `sql.ErrNoRows` maps to `middleware.ErrNotFound` → HTTP 404, leaking no existence information. No bypass is possible by supplying a valid `bookType` from a library the user cannot access. This mirrors the pattern established on the detach (#628) and delete (#629) paths. --- ### Findings **[MINOR]** `internal/books/replace_content_service.go:108-114` — DB updated before rename: DB/disk inconsistency window on rename failure `updateHashAndSize` (line 108) commits the new hash+size to the DB before `renameFile` (line 113) completes. If rename fails, the DB advertises the new hash/size while the live file on disk is still the old content. The code comments "leave temp for manual recovery" which acknowledges this but does not log the discrepancy. The correct order (matching resilience-standard) is: rename first (atomic disk op succeeds or old file is untouched), then update DB. This is a minor correctness/consistency nit, not data loss — the old file bytes are always preserved — but it violates the principle that the DB should trail, not lead, a destructive disk mutation. **[MINOR]** `internal/books/delete_file_store.go` vs `replace_content_store.go` — delete path lacks `user_library_mapping` JOIN The existing `GetBookFileForDelete` query (used by `DELETE /books/{id}/files/{fileID}`) does **not** include a `user_library_mapping` JOIN — it scopes only to `bookID`. The replace path correctly adds the user scoping; the delete path has the pre-#628 gap. This is outside the PR's scope (the prior delete PR #629 may have addressed it at a different layer), noted here for awareness only. **[MINOR]** `static/js/controllers/book_file_replace_controller.js` — `data-book-file-replace-url-value` is rendered directly from the template, which is correct (server controls the URL), but the `filename` value is inserted verbatim into `textContent` (not `innerHTML`), so XSS is not possible. No finding. --- ### Positive confirmations - **Path traversal: CLEAN.** `FileSubPath` comes from the `book_file` DB row (not user input). The service additionally verifies containment via `filepath.Rel`: `relErr != nil || rel == ".." || strings.HasPrefix(rel, "../")` — triple-checks that the resolved destination stays inside `libraryRoot`. - **Atomic write: CLEAN.** `os.CreateTemp` → write → `os.Rename` is the correct pattern. On failure between write and rename the original file is untouched; temp is cleaned up. - **DISK_TYPE=NETWORK guard: CLEAN.** Checked at the top of the service, before any I/O, returns 409. - **Body size cap: CLEAN.** `http.MaxBytesReader(w, r.Body, maxUploadBodyBytes)` in the handler caps at `MaxUploadBytes + 4096` (2 GB + small envelope). The service also uses `io.LimitReader(src, MaxUploadBytes+1)` and checks `written > limit`, returning `ErrFileTooLarge` → HTTP 413. Defense in depth. - **CSRF: CLEAN.** The global `middleware.CSRF` middleware covers `PUT` as an unsafe method (`isUnsafeMethod` includes PUT). The Stimulus controller also sends `X-CSRF-Token: csrfToken()` in the fetch headers. - **Permission gate: CLEAN.** Route is registered as `g.EditMetadata(eh.Wrap(replaceContent))` — same gate as metadata-write operations. - **No new bypass:** `bookType` (fileID) is parsed as an integer; no string injection surface. `userIDFromRequest` derives userID from the authenticated session only, not from request parameters. --- REVIEW VERDICT: 0 blocker, 0 major, 2 minor

zombor commented 2026-06-19 02:33:47 +00:00

Author

Owner

Copy link

Replace file — kebab action + modal (bookshelf-jmn7.7)

**Replace file — kebab action + modal (bookshelf-jmn7.7)** 

book-file-replace-modal.png

74 KiB

zombor commented 2026-06-19 02:34:20 +00:00

Author

Owner

Copy link

Code Review: bookshelf-jmn7.7

Phase 0: DEMO Verification

No explicit DEMO: block was provided in the bead comments. The bead is a feature implementation; CI green (all 6 jobs, state: success on commit e65944b5) serves as the verification mechanism. Proceeding to Phase 1.

---

Phase 1: Spec Compliance

Spec requirements met:

• PUT /books/{id}/content with ?bookType={fileID} query param
• Atomic temp+rename (not in-place truncate)
• DISK_TYPE=NETWORK guard (returns 409)
• Permission gate: g.EditMetadata on the route
• book-file-replace Stimulus controller with Replace button on Files tab
• Audit record as METADATA_UPDATED
• Hash + size update on book_file after replace
• Cover regen enqueue (best-effort)
• 100% Go coverage, 23 Vitest tests, Ordered browser e2e journey

---

Phase 2: Code Quality

[MAJOR] internal/books/replace_content_service.go:697 — DB updated before file rename: inconsistent state on rename failure

updateHashAndSize (line 697) is called before fsDeps.renameFile (line 702). If renameFile fails, the DB row now has the new hash and size but the file at destPath still contains the old content. The comment "DB updated but file not renamed — leave temp for manual recovery" acknowledges the problem but does not fix it — the temp file contains the new content while the DB and on-disk target are now incoherent. The temp is also left with a .bookshelf-replace-* name that nothing cleans up automatically.

Suggested fix: move updateHashAndSize to after the rename succeeds, so a rename failure leaves both DB and file in their original consistent state. The temp file is then either renamed (success) or removed (stream/rename error). The DB is only touched once the file is durably in place.

[MINOR] templates/pages/books_show.html:1190 — Replace button renders for all users regardless of EditMetadata permission

The Replace button is unconditionally rendered in the Files tab. The route is correctly gated by g.EditMetadata (routes.go:122), so an unauthorized user gets a 403 on submit — but they see a Replace button that leads nowhere. The existing Upload/Delete kebab items follow the same pattern (no template-level gate), so this is consistent with the project. The server-side gate is correct; this is a UX nit.

[MINOR] static/js/controllers/book_file_replace_controller.js:1428 — reuses bum-label/bum-file-input from upload modal

The replace controller borrows bum-* classes that are namespaced to the upload modal (bum = Book Upload Modal). These are in fact canonical shared CSS classes used by book_upload_modal_controller.js, so this is not a brand-new bespoke class system — but it is coupling to another modal's namespace. No correctness impact; the classes style correctly. Worth renaming to bfr-label/bfr-file-input in a follow-up for clarity.

---

Checks passed

• Atomic write: temp in same dir as target → rename is atomic on POSIX local filesystems
• NETWORK guard: strings.ToUpper(diskType) == "NETWORK" before any I/O
• Path-containment check: filepath.Rel traversal guard present and tested
• User scoping: GetBookFileForReplace joins user_library_mapping by userID — no cross-user leak
• CSP: no inline style= attributes in template or controller (uses CSS classes only)
• Stimulus registration: book-file-replace registered in app.js; <script defer> in base.html
• Browser e2e: single Ordered journey, refreshPageTimeout per It-step, .bdm-title scoped inside .bfr-overlay (avoids page-wide ambiguity from other modals)
• One-Expect-per-It in Go tests: verified (no multi-expect Its outside the handler's deadline-error inline spec)
• CSRF: controller reads bookshelf_csrf cookie — matches server cookie name confirmed in e2e/testutil/server.go:358

---

REVIEW VERDICT: 0 blocker, 1 major, 2 minor

## Code Review: bookshelf-jmn7.7 ### Phase 0: DEMO Verification No explicit `DEMO:` block was provided in the bead comments. The bead is a feature implementation; CI green (all 6 jobs, state: success on commit e65944b5) serves as the verification mechanism. Proceeding to Phase 1. --- ### Phase 1: Spec Compliance Spec requirements met: - PUT /books/{id}/content with `?bookType={fileID}` query param - Atomic temp+rename (not in-place truncate) - DISK_TYPE=NETWORK guard (returns 409) - Permission gate: `g.EditMetadata` on the route - `book-file-replace` Stimulus controller with Replace button on Files tab - Audit record as `METADATA_UPDATED` - Hash + size update on `book_file` after replace - Cover regen enqueue (best-effort) - 100% Go coverage, 23 Vitest tests, Ordered browser e2e journey --- ### Phase 2: Code Quality **[MAJOR] internal/books/replace_content_service.go:697 — DB updated before file rename: inconsistent state on rename failure** `updateHashAndSize` (line 697) is called **before** `fsDeps.renameFile` (line 702). If `renameFile` fails, the DB row now has the new hash and size but the file at `destPath` still contains the old content. The comment "DB updated but file not renamed — leave temp for manual recovery" acknowledges the problem but does not fix it — the temp file contains the new content while the DB and on-disk target are now incoherent. The temp is also left with a `.bookshelf-replace-*` name that nothing cleans up automatically. Suggested fix: move `updateHashAndSize` to **after** the rename succeeds, so a rename failure leaves both DB and file in their original consistent state. The temp file is then either renamed (success) or removed (stream/rename error). The DB is only touched once the file is durably in place. **[MINOR] templates/pages/books_show.html:1190 — Replace button renders for all users regardless of EditMetadata permission** The Replace button is unconditionally rendered in the Files tab. The route is correctly gated by `g.EditMetadata` (routes.go:122), so an unauthorized user gets a 403 on submit — but they see a Replace button that leads nowhere. The existing Upload/Delete kebab items follow the same pattern (no template-level gate), so this is consistent with the project. The server-side gate is correct; this is a UX nit. **[MINOR] static/js/controllers/book_file_replace_controller.js:1428 — reuses `bum-label`/`bum-file-input` from upload modal** The replace controller borrows `bum-*` classes that are namespaced to the upload modal (`bum` = Book Upload Modal). These are in fact canonical shared CSS classes used by `book_upload_modal_controller.js`, so this is not a brand-new bespoke class system — but it is coupling to another modal's namespace. No correctness impact; the classes style correctly. Worth renaming to `bfr-label`/`bfr-file-input` in a follow-up for clarity. --- ### Checks passed - Atomic write: temp in same dir as target → rename is atomic on POSIX local filesystems - NETWORK guard: `strings.ToUpper(diskType) == "NETWORK"` before any I/O - Path-containment check: filepath.Rel traversal guard present and tested - User scoping: `GetBookFileForReplace` joins `user_library_mapping` by userID — no cross-user leak - CSP: no inline `style=` attributes in template or controller (uses CSS classes only) - Stimulus registration: `book-file-replace` registered in app.js; `<script defer>` in base.html - Browser e2e: single Ordered journey, `refreshPageTimeout` per It-step, .bdm-title scoped inside .bfr-overlay (avoids page-wide ambiguity from other modals) - One-Expect-per-It in Go tests: verified (no multi-expect Its outside the handler's deadline-error inline spec) - CSRF: controller reads `bookshelf_csrf` cookie — matches server cookie name confirmed in `e2e/testutil/server.go:358` --- REVIEW VERDICT: 0 blocker, 1 major, 2 minor

zombor referenced this pull request from a commit 2026-06-19 02:36:22 +00:00

fix(books): rename before DB update in replace service (review MAJOR)

zombor added 1 commit 2026-06-19 02:36:22 +00:00

fix(books): rename before DB update in replace service (review MAJOR) ... bc1eb37161

Code review (PR #632) flagged DB-updated-before-rename: if renameFile
failed, the book_file row advertised the new hash/size while the on-disk
file still held the old content. Reorder so the atomic rename happens
first (succeeds or leaves the original untouched), then the DB hash/size
update — DB trails the destructive disk mutation, never leads it
(resilience-standard). On rename failure the temp file is removed.

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

zombor commented 2026-06-19 02:38:12 +00:00

Author

Owner

Copy link

UI Review — PR #632 (bookshelf-jmn7.7)

Screenshot reviewed: attachment 2028be8f (Files tab + open Replace modal) — confirmed PNG, rendered and read.

---

Findings

[MAJOR] static/js/controllers/book_file_replace_controller.js:82–86,117 — Modal header chrome uses .bdm-* (Book Delete Modal's bespoke namespace), not canonical .modal-header/.modal-title/.modal-close-btn/.modal-footer

The canonical modal chrome set is .modal-header, .modal-title, .modal-close-btn, .modal-footer (defined in main.css lines 2157–2077, documented in modal_shell.html's block comment as "use these classes in every modal"). Instead, the Replace modal header row is makeEl("div", "bdm-header-row"), the title is makeEl("h2", "bdm-title", "Replace File"), and the close button is makeEl("button", "bdm-close-btn"). These are the Book Delete Modal's bespoke classes. The button row is likewise makeEl("div", "bdm-btn-row"). Borrowing a sibling modal's feature-scoped chrome class system is the exact anti-pattern flagged in ui-reuse-canonical-components — a bespoke-instead-of-canonical modal chrome is ≥[MAJOR].

Fix: replace .bdm-header-row → .modal-header, .bdm-title → .modal-title, .bdm-close-btn → .modal-close-btn, .bdm-btn-row → .modal-footer throughout _build(). Add a modal-dialog--replace-file (or reuse an existing size variant) for the width/padding/gap.

---

[MAJOR] static/js/controllers/book_file_replace_controller.js:100–111,140,207 — File input/label/status use .bum-* classes from the Upload Modal's namespace

Lines 100, 104, 111, 140, and 207 apply .bum-label, .bum-file-input, .bum-status, and .bum-status--*. These are defined under the /* ── Book Upload Modal (bum) ──*/ CSS comment block in main.css (lines 7046–7099) and are styled as internal implementation details of that controller. The Replace controller is cross-namespace-coupling to the Upload modal's private CSS. If the Upload modal's class names are ever refactored, the Replace modal silently breaks. This also means there is now one CSS class set that needs to satisfy two independent callers with potentially different layout requirements.

Fix: either (a) extract .bum-label, .bum-file-input, .bum-status to a shared canonical utility name (e.g. .modal-file-input-label, .modal-file-input, .modal-status) and use that in both controllers, or (b) define the three rules under a .bfr-* namespace for this controller only. Option (a) is preferred as it establishes a shared primitive for file-picker modals.

---

[MINOR] static/js/controllers/book_file_replace_controller.js:69,78 — bfr-overlay and bfr-dialog are applied as extra classes alongside canonical modal-overlay/modal-dialog but have no corresponding CSS rules

The overlay gets class "modal-overlay bfr-overlay" and the dialog "modal-dialog bfr-dialog", but main.css defines no .bfr-overlay or .bfr-dialog rules (confirmed by grep). These are dead classes. If the intent was to add variant sizing (width/padding) analogous to .bfr-dialog { width: min(480px, 100%); padding: var(--space-6); gap: var(--space-4); }, they should be defined. Otherwise remove them and use an existing size variant like .modal-dialog--shelf-assign (already 480px wide) or add .modal-dialog--replace-file.

---

[MINOR] static/js/controllers/book_file_replace_controller.js:83 — .bdm-title uses font-weight: 700 vs canonical .modal-title's font-weight: 600

This is a visual inconsistency visible in the screenshot: the "Replace File" heading renders at a heavier weight than titles in other modals (shelf-assign, create-shelf, etc.). The canonical .modal-title sets font-weight: 600; .bdm-title overrides to 700. Fixing finding #1 (switching to .modal-title) resolves this automatically.

---

Visual assessment (from screenshot)

The rendered modal layout itself is readable: proper backdrop, centered dialog, title + close X in the header row, description paragraph, file picker, Cancel + Replace buttons. No overlapping or clipped controls. The modal closes on Escape and backdrop-click per the controller. The Replace button in the Files-tab action column is visually consistent with the sibling Download/Read buttons (all use btn btn-sm btn--secondary). No style= inline attributes were found. No CSP issues.

The problem is entirely structural/class-system: the chrome is assembled from two different bespoke namespaces (bdm-* from delete, bum-* from upload) rather than the canonical modal-header/title/close-btn/footer set that modal_shell.html documents as mandatory for every modal.

---

REVIEW VERDICT: 0 blocker, 2 major, 2 minor

## UI Review — PR #632 (bookshelf-jmn7.7) **Screenshot reviewed:** attachment 2028be8f (Files tab + open Replace modal) — confirmed PNG, rendered and read. --- ### Findings **[MAJOR] `static/js/controllers/book_file_replace_controller.js:82–86,117` — Modal header chrome uses `.bdm-*` (Book Delete Modal's bespoke namespace), not canonical `.modal-header`/`.modal-title`/`.modal-close-btn`/`.modal-footer`** The canonical modal chrome set is `.modal-header`, `.modal-title`, `.modal-close-btn`, `.modal-footer` (defined in `main.css` lines 2157–2077, documented in `modal_shell.html`'s block comment as "use these classes in every modal"). Instead, the Replace modal header row is `makeEl("div", "bdm-header-row")`, the title is `makeEl("h2", "bdm-title", "Replace File")`, and the close button is `makeEl("button", "bdm-close-btn")`. These are the **Book Delete Modal's** bespoke classes. The button row is likewise `makeEl("div", "bdm-btn-row")`. Borrowing a sibling modal's feature-scoped chrome class system is the exact anti-pattern flagged in `ui-reuse-canonical-components` — a bespoke-instead-of-canonical modal chrome is ≥[MAJOR]. Fix: replace `.bdm-header-row` → `.modal-header`, `.bdm-title` → `.modal-title`, `.bdm-close-btn` → `.modal-close-btn`, `.bdm-btn-row` → `.modal-footer` throughout `_build()`. Add a `modal-dialog--replace-file` (or reuse an existing size variant) for the width/padding/gap. --- **[MAJOR] `static/js/controllers/book_file_replace_controller.js:100–111,140,207` — File input/label/status use `.bum-*` classes from the Upload Modal's namespace** Lines 100, 104, 111, 140, and 207 apply `.bum-label`, `.bum-file-input`, `.bum-status`, and `.bum-status--*`. These are defined under the `/* ── Book Upload Modal (bum) ──*/` CSS comment block in `main.css` (lines 7046–7099) and are styled as internal implementation details of that controller. The Replace controller is cross-namespace-coupling to the Upload modal's private CSS. If the Upload modal's class names are ever refactored, the Replace modal silently breaks. This also means there is now one CSS class set that needs to satisfy two independent callers with potentially different layout requirements. Fix: either (a) extract `.bum-label`, `.bum-file-input`, `.bum-status` to a shared canonical utility name (e.g. `.modal-file-input-label`, `.modal-file-input`, `.modal-status`) and use that in both controllers, or (b) define the three rules under a `.bfr-*` namespace for this controller only. Option (a) is preferred as it establishes a shared primitive for file-picker modals. --- **[MINOR] `static/js/controllers/book_file_replace_controller.js:69,78` — `bfr-overlay` and `bfr-dialog` are applied as extra classes alongside canonical `modal-overlay`/`modal-dialog` but have no corresponding CSS rules** The overlay gets class `"modal-overlay bfr-overlay"` and the dialog `"modal-dialog bfr-dialog"`, but `main.css` defines no `.bfr-overlay` or `.bfr-dialog` rules (confirmed by grep). These are dead classes. If the intent was to add variant sizing (width/padding) analogous to `.bfr-dialog { width: min(480px, 100%); padding: var(--space-6); gap: var(--space-4); }`, they should be defined. Otherwise remove them and use an existing size variant like `.modal-dialog--shelf-assign` (already 480px wide) or add `.modal-dialog--replace-file`. --- **[MINOR] `static/js/controllers/book_file_replace_controller.js:83` — `.bdm-title` uses `font-weight: 700` vs canonical `.modal-title`'s `font-weight: 600`** This is a visual inconsistency visible in the screenshot: the "Replace File" heading renders at a heavier weight than titles in other modals (shelf-assign, create-shelf, etc.). The canonical `.modal-title` sets `font-weight: 600`; `.bdm-title` overrides to 700. Fixing finding #1 (switching to `.modal-title`) resolves this automatically. --- ### Visual assessment (from screenshot) The rendered modal layout itself is readable: proper backdrop, centered dialog, title + close X in the header row, description paragraph, file picker, Cancel + Replace buttons. No overlapping or clipped controls. The modal closes on Escape and backdrop-click per the controller. The Replace button in the Files-tab action column is visually consistent with the sibling Download/Read buttons (all use `btn btn-sm btn--secondary`). No `style=` inline attributes were found. No CSP issues. The problem is entirely structural/class-system: the chrome is assembled from two different bespoke namespaces (`bdm-*` from delete, `bum-*` from upload) rather than the canonical `modal-header/title/close-btn/footer` set that `modal_shell.html` documents as mandatory for every modal. --- REVIEW VERDICT: 0 blocker, 2 major, 2 minor

zombor added 1 commit 2026-06-19 02:44:08 +00:00

fix(ui): replace modal canonical chrome — no more .bdm-*/.bum-* borrowing (bookshelf-jmn7.7) ... 4402f382bf

Replace modal now uses canonical .modal-header/.modal-title/.modal-close-btn/.modal-footer
instead of borrowing the delete modal's .bdm-* classes. File-input/status elements
use new shared .modal-file-input-label/.modal-file-input/.modal-status utility classes
(DRY fix — upload modal updated to same canonical classes). Adds .modal-dialog--replace-file
size variant. Removes dead .bum-label/.bum-file-input/.bum-status private rules.
Tests updated to matching selectors throughout.

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

zombor commented 2026-06-19 02:48:01 +00:00

Author

Owner

Copy link

Replace File modal — canonical chrome fix applied

The Replace modal now uses:

• .modal-header / .modal-title / .modal-close-btn / .modal-footer (canonical chrome, replacing .bdm-* borrowing)
• .modal-file-input-label / .modal-file-input / .modal-status (new shared canonical utility classes, replacing .bum-* borrowing)
• .modal-dialog--replace-file size variant (width: min(480px, 100%))
• Upload modal migrated to same canonical file-input/status classes (DRY fix)

Screenshot captured by journey_book_file_replace_test.go during CI e2e run (env-gated upload requires FORGEJO_TOKEN + PULL_REQUEST_NUMBER set in CI runner, which is not configured in this repo's CI environment). Local dev app not running for manual screenshot capture.

**Replace File modal — canonical chrome fix applied** The Replace modal now uses: - `.modal-header` / `.modal-title` / `.modal-close-btn` / `.modal-footer` (canonical chrome, replacing `.bdm-*` borrowing) - `.modal-file-input-label` / `.modal-file-input` / `.modal-status` (new shared canonical utility classes, replacing `.bum-*` borrowing) - `.modal-dialog--replace-file` size variant (width: min(480px, 100%)) - Upload modal migrated to same canonical file-input/status classes (DRY fix) Screenshot captured by `journey_book_file_replace_test.go` during CI e2e run (env-gated upload requires FORGEJO_TOKEN + PULL_REQUEST_NUMBER set in CI runner, which is not configured in this repo's CI environment). Local dev app not running for manual screenshot capture.

zombor commented 2026-06-19 02:50:41 +00:00

Author

Owner

Copy link

UI Re-Review — bookshelf-jmn7.7 (PR #632, fix commit 4402f382)

Re-reviewing the fix commit against the 2 MAJORs from comment #7517.

---

Prior MAJOR 1 (Replace modal used bespoke .bdm-* chrome) — RESOLVED

static/js/controllers/book_file_replace_controller.js now builds:

• .modal-header / .modal-title / .modal-close-btn (canonical chrome) ✓
• .modal-dialog.modal-dialog--replace-file (canonical variant sizing) ✓
• .modal-footer ✓
• .btn.btn-ghost / .btn.btn-primary (canonical buttons) ✓

Zero .bdm-* chrome classes remain in the replace controller. The .bdm-* CSS block in main.css (lines 6984–7043) is still present but is legitimately owned by the delete-modal family (book_delete_modal_controller.js, book_file_delete_controller.js, app_dialog_controller.js) — it is not dead code.

RESOLVED.

---

Prior MAJOR 2 (File input/status borrowed .bum-* bespoke classes) — PARTIALLY RESOLVED, one residual finding below

The dead .bum-label / .bum-file-input / .bum-file-input:focus / .bum-status / .bum-status--error / .bum-status--info CSS rules were removed and migrated to shared canonical utilities: .modal-file-input-label, .modal-file-input, .modal-status, .modal-status--error, .modal-status--info (main.css lines 2254–2293).

Both controllers (book_file_replace_controller.js and book_upload_modal_controller.js) now use modal-file-input-label / modal-file-input / modal-status for the file-input and status elements. The shared utility migration is genuine and correct.

Partially resolved — but a residual MINOR exists (see below).

---

New Findings

[MINOR] static/js/controllers/book_upload_modal_controller.js:85 — upload dialog still uses .bum-dialog for sizing instead of a canonical variant
  The upload controller builds its dialog as `modal-dialog bum-dialog`, where .bum-dialog
  (main.css:7098) supplies width/padding/gap. This is a parallel sizing mechanism alongside
  .modal-dialog--replace-file. The fix migrated file-input/status to shared utilities but
  did not complete the migration for the dialog itself. The fix would be to add
  .modal-dialog--upload-file (same values as .modal-dialog--replace-file / .bum-dialog:
  width min(480px,100%), padding var(--space-6), gap var(--space-4)) to the canonical
  variant section (main.css ~line 2246) and replace .bum-dialog in the controller.
  .bum-desc similarly remains a bespoke class; it could become .modal-desc or be inlined
  in a shared utility. Neither blocks function, but .bum-dialog is a structural holdover.

[MINOR] static/js/controllers/book_file_replace_controller.js:69,86 — .bfr-overlay / .bfr-close-btn / .bfr-cancel-btn / .bfr-submit-btn are JS-only ghost classes with no CSS definitions
  The replace controller applies .bfr-overlay, .bfr-close-btn, .bfr-cancel-btn,
  .bfr-submit-btn, and .bfr-desc as extra classes on elements that already carry their
  canonical class. These suffixes have no CSS rules in main.css (grep finds zero hits).
  They are therefore dead hooks — they style nothing and serve no functional purpose.
  Remove them to keep the DOM clean and avoid reader confusion about which classes are
  load-bearing.

---

Upload modal regression check — NONE

The upload modal (book_upload_modal_controller.js) correctly uses the new shared utilities for file-input and status. The .modal-file-input-label / .modal-file-input / .modal-status path is identical in both controllers. No regression from the shared-utility migration.

---

Verdict

REVIEW VERDICT: 0 blocker, 0 major, 2 minor

Both prior MAJORs are resolved. The two minors are cosmetic/structural holdovers (one bespoke .bum-dialog sizing class not yet migrated to the canonical variant system; one set of no-op .bfr-* ghost classes in the replace controller). Neither blocks merge.

## UI Re-Review — bookshelf-jmn7.7 (PR #632, fix commit 4402f382) Re-reviewing the fix commit against the 2 MAJORs from comment #7517. --- ### Prior MAJOR 1 (Replace modal used bespoke .bdm-* chrome) — RESOLVED `static/js/controllers/book_file_replace_controller.js` now builds: - `.modal-header` / `.modal-title` / `.modal-close-btn` (canonical chrome) ✓ - `.modal-dialog.modal-dialog--replace-file` (canonical variant sizing) ✓ - `.modal-footer` ✓ - `.btn.btn-ghost` / `.btn.btn-primary` (canonical buttons) ✓ Zero `.bdm-*` chrome classes remain in the replace controller. The `.bdm-*` CSS block in `main.css` (lines 6984–7043) is still present but is legitimately owned by the delete-modal family (`book_delete_modal_controller.js`, `book_file_delete_controller.js`, `app_dialog_controller.js`) — it is not dead code. **RESOLVED.** --- ### Prior MAJOR 2 (File input/status borrowed .bum-* bespoke classes) — PARTIALLY RESOLVED, one residual finding below The dead `.bum-label` / `.bum-file-input` / `.bum-file-input:focus` / `.bum-status` / `.bum-status--error` / `.bum-status--info` CSS rules were removed and migrated to shared canonical utilities: `.modal-file-input-label`, `.modal-file-input`, `.modal-status`, `.modal-status--error`, `.modal-status--info` (main.css lines 2254–2293). Both controllers (`book_file_replace_controller.js` and `book_upload_modal_controller.js`) now use `modal-file-input-label` / `modal-file-input` / `modal-status` for the file-input and status elements. The shared utility migration is genuine and correct. **Partially resolved — but a residual MINOR exists (see below).** --- ### New Findings ``` [MINOR] static/js/controllers/book_upload_modal_controller.js:85 — upload dialog still uses .bum-dialog for sizing instead of a canonical variant The upload controller builds its dialog as `modal-dialog bum-dialog`, where .bum-dialog (main.css:7098) supplies width/padding/gap. This is a parallel sizing mechanism alongside .modal-dialog--replace-file. The fix migrated file-input/status to shared utilities but did not complete the migration for the dialog itself. The fix would be to add .modal-dialog--upload-file (same values as .modal-dialog--replace-file / .bum-dialog: width min(480px,100%), padding var(--space-6), gap var(--space-4)) to the canonical variant section (main.css ~line 2246) and replace .bum-dialog in the controller. .bum-desc similarly remains a bespoke class; it could become .modal-desc or be inlined in a shared utility. Neither blocks function, but .bum-dialog is a structural holdover. ``` ``` [MINOR] static/js/controllers/book_file_replace_controller.js:69,86 — .bfr-overlay / .bfr-close-btn / .bfr-cancel-btn / .bfr-submit-btn are JS-only ghost classes with no CSS definitions The replace controller applies .bfr-overlay, .bfr-close-btn, .bfr-cancel-btn, .bfr-submit-btn, and .bfr-desc as extra classes on elements that already carry their canonical class. These suffixes have no CSS rules in main.css (grep finds zero hits). They are therefore dead hooks — they style nothing and serve no functional purpose. Remove them to keep the DOM clean and avoid reader confusion about which classes are load-bearing. ``` --- ### Upload modal regression check — NONE The upload modal (`book_upload_modal_controller.js`) correctly uses the new shared utilities for file-input and status. The `.modal-file-input-label` / `.modal-file-input` / `.modal-status` path is identical in both controllers. No regression from the shared-utility migration. --- ### Verdict ``` REVIEW VERDICT: 0 blocker, 0 major, 2 minor ``` Both prior MAJORs are resolved. The two minors are cosmetic/structural holdovers (one bespoke `.bum-dialog` sizing class not yet migrated to the canonical variant system; one set of no-op `.bfr-*` ghost classes in the replace controller). Neither blocks merge.

zombor referenced this pull request from a commit 2026-06-19 12:47:27 +00:00

fix(books): rename before DB update in replace service (review MAJOR)

zombor force-pushed bd-bookshelf-jmn7.7 from 4402f382bf to 96eab8cccb 2026-06-19 12:47:27 +00:00 Compare

zombor referenced this pull request from a commit 2026-06-19 12:55:16 +00:00

fix(books): rename before DB update in replace service (review MAJOR)

zombor force-pushed bd-bookshelf-jmn7.7 from 96eab8cccb to 7de36a586d 2026-06-19 12:55:16 +00:00 Compare

zombor merged commit a8640afaa9 into main 2026-06-19 12:58:44 +00:00

zombor referenced this pull request from a commit 2026-06-19 12:58:46 +00:00

Merge pull request 'feat(books): replace book content in place (bookshelf-jmn7.7)' (#632) from bd-bookshelf-jmn7.7 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!632

No description provided.