Bookdrop: auto-check row when fetched metadata is copied (bookshelf-j8pvj) #1424

Merged

zombor merged 1 commit from bd-bookshelf-j8pvj into main 2026-08-11 14:40:15 +00:00

Conversation 2 Commits 1 Files changed 6 +191 -1

zombor commented 2026-08-11 00:31:14 +00:00

Owner

Copy link

Summary

• Applying fetched metadata via a per-field copy-arrow (copyField), Copy all (copyAll), or Copy missing (copyMissing) in bookdrop_file_editor_controller.js now auto-checks the proposal row's checkbox — mirroring the existing auto-select-on-library-assign behavior from bookshelf-fvug6.
• Wired via a bubbling bookdrop:metadatacopied CustomEvent (detail: {proposalId}) dispatched from _autoSave() (the shared tail of copyField/copyAll/copyMissing), listened for by bookdrop_review_controller.js which maps proposalId to the matching row via data-proposal-id, checks its checkbox, and calls _updateUI().
• Extended the existing "clicking the title copy arrow auto-saves" browser e2e journey step to also assert the row checkbox auto-checks.
• Docs: updated docs/content/docs/importing/bookdrop.md to describe the new auto-check-on-copy behavior.

Test plan

• npm run coverage — 100% JS coverage, all 4740 tests pass (new unit tests in both controller test files cover the dispatch + listener + edge cases: unknown proposalId, missing detail, already-checked row, disconnect teardown)
• go build -tags e2e ./e2e/... compiles
• go vet ./..., make e2e-policy-check, make test-policy-check, bash scripts/check-css-vars.sh all pass
• CI green

Closes bead bookshelf-j8pvj on merge.

## Summary - Applying fetched metadata via a per-field copy-arrow (copyField), Copy all (copyAll), or Copy missing (copyMissing) in `bookdrop_file_editor_controller.js` now auto-checks the proposal row's checkbox — mirroring the existing auto-select-on-library-assign behavior from bookshelf-fvug6. - Wired via a bubbling `bookdrop:metadatacopied` CustomEvent (detail: `{proposalId}`) dispatched from `_autoSave()` (the shared tail of copyField/copyAll/copyMissing), listened for by `bookdrop_review_controller.js` which maps `proposalId` to the matching row via `data-proposal-id`, checks its checkbox, and calls `_updateUI()`. - Extended the existing "clicking the title copy arrow auto-saves" browser e2e journey step to also assert the row checkbox auto-checks. - Docs: updated `docs/content/docs/importing/bookdrop.md` to describe the new auto-check-on-copy behavior. ## Test plan - [x] `npm run coverage` — 100% JS coverage, all 4740 tests pass (new unit tests in both controller test files cover the dispatch + listener + edge cases: unknown proposalId, missing detail, already-checked row, disconnect teardown) - [x] `go build -tags e2e ./e2e/...` compiles - [x] `go vet ./...`, `make e2e-policy-check`, `make test-policy-check`, `bash scripts/check-css-vars.sh` all pass - [x] CI green Closes bead bookshelf-j8pvj on merge.

zombor added 1 commit 2026-08-11 00:31:15 +00:00

feat(bookdrop): auto-check row when fetched metadata is copied ... acdcbd1567

Applying fetched metadata via a per-field copy-arrow, Copy all, or Copy
missing now auto-checks the proposal row's checkbox, mirroring the
existing auto-select-on-library-assign behavior (bookshelf-fvug6).

bookdrop_file_editor_controller's copyField/copyAll/copyMissing all
funnel through _autoSave(), which now also dispatches a bubbling
"bookdrop:metadatacopied" CustomEvent carrying the proposalId.
bookdrop_review_controller listens for it and checks the matching row
(via data-proposal-id) + calls _updateUI().

Closes bead bookshelf-j8pvj.

zombor force-pushed bd-bookshelf-j8pvj from acdcbd1567 to c1c20f2ada 2026-08-11 02:35:33 +00:00 Compare

zombor commented 2026-08-11 12:27:41 +00:00

Author

Owner

Copy link

Security Review — PR #1424 (bookshelf-j8pvj)

Reviewed the diff: static/js/controllers/bookdrop_file_editor_controller.js,
static/js/controllers/bookdrop_review_controller.js, their Vitest specs, one
e2e journey addition, and a docs tweak. No server-side code, no new endpoint,
no new authz surface.

Checked:

• The bookdrop:metadatacopied CustomEvent carries only detail: { proposalId }
  — a single ID, no PII/tokens/sensitive payload. It's dispatched with
  bubbles: true on this.element and listened for via
  document.addEventListener — same-document only, no postMessage/
  BroadcastChannel/cross-origin exposure.
• proposalId is Stimulus's proposalIdValue, sourced from a server-rendered
  data-* attribute on the row — the same trust boundary as every other
  Stimulus value binding in this codebase, not new attacker-controlled input.
• Consumption in _onMetadataCopied (bookdrop_review_controller.js) uses
  parseInt(row.getAttribute("data-proposal-id"), 10) === proposalId inside
  Array.prototype.findIndex — a numeric equality comparison, never
  interpolated into a querySelector string or any DOM sink (innerHTML,
  insertAdjacentHTML, etc.). No selector-injection or XSS vector.
• disconnect() is a net-new method (verified against pre-PR file) that just
  removes the listener added in connect() — no double-registration/leak, no
  overwritten existing lifecycle hook.
• No new fetch/XHR, no new route, no change to accept/reject flows' server
  calls — purely a client-side UX convenience (auto-check a checkbox).

No BLOCKER/MAJOR/MINOR findings.

REVIEW VERDICT: 0 blocker, 0 major, 0 minor

## Security Review — PR #1424 (bookshelf-j8pvj) Reviewed the diff: `static/js/controllers/bookdrop_file_editor_controller.js`, `static/js/controllers/bookdrop_review_controller.js`, their Vitest specs, one e2e journey addition, and a docs tweak. No server-side code, no new endpoint, no new authz surface. **Checked:** - The `bookdrop:metadatacopied` `CustomEvent` carries only `detail: { proposalId }` — a single ID, no PII/tokens/sensitive payload. It's dispatched with `bubbles: true` on `this.element` and listened for via `document.addEventListener` — same-document only, no `postMessage`/ `BroadcastChannel`/cross-origin exposure. - `proposalId` is Stimulus's `proposalIdValue`, sourced from a server-rendered `data-*` attribute on the row — the same trust boundary as every other Stimulus value binding in this codebase, not new attacker-controlled input. - Consumption in `_onMetadataCopied` (`bookdrop_review_controller.js`) uses `parseInt(row.getAttribute("data-proposal-id"), 10) === proposalId` inside `Array.prototype.findIndex` — a numeric equality comparison, never interpolated into a `querySelector` string or any DOM sink (`innerHTML`, `insertAdjacentHTML`, etc.). No selector-injection or XSS vector. - `disconnect()` is a net-new method (verified against pre-PR file) that just removes the listener added in `connect()` — no double-registration/leak, no overwritten existing lifecycle hook. - No new fetch/XHR, no new route, no change to accept/reject flows' server calls — purely a client-side UX convenience (auto-check a checkbox). No BLOCKER/MAJOR/MINOR findings. REVIEW VERDICT: 0 blocker, 0 major, 0 minor

zombor commented 2026-08-11 12:29:11 +00:00

Author

Owner

Copy link

Code Review — PR #1424 (bookshelf-j8pvj)

Reviewed the diff (bookdrop_file_editor_controller.js, bookdrop_review_controller.js, Vitest specs, e2e journey, docs). No blockers or majors found.

[MINOR] static/js/controllers/bookdrop_file_editor_controller.js:390-397 — _autoSave dispatches bookdrop:metadatacopied unconditionally, even if the underlying _doSave() request later fails. The row gets auto-checked even when the copy wasn't actually persisted server-side. Low impact (save failure surfaces via the status indicator, and Accept flushes/gates on save success before submitting), but worth a comment noting this is deliberate optimistic-UI behavior, or moving the dispatch to the save-success path if strictness is wanted.

Verified as sound:

• Single dispatch point: all 7 _autoSave() call sites are exactly the 5 branches of copyField + copyAll + copyMissing (grep-verified) — no triple-dispatch risk.
• Listener lifecycle: connect() stores this._boundMetadataCopied = this._onMetadataCopied.bind(this) and adds it; disconnect() removes the same bound reference — no leak. Vitest's MutationObserver-driven disconnect test covers this.
• proposalId plumbing: editor's proposalId: Number Stimulus value vs review's parseInt(row.getAttribute('data-proposal-id'), 10) === proposalId — types match.
• Idempotency: if (!rowCheckbox || rowCheckbox.checked) return; guards re-running _updateUI() on an already-checked row — covered by a dedicated Vitest spec asserting _updateUI is not called again.
• Unknown proposalId and missing detail.proposalId both handled gracefully (no throw, no-op) — covered by tests.
• Vitest coverage is thorough across dispatch-on-click (per-field/copyAll/copyMissing), bubbling, and review-controller check/no-check/unknown-id/missing-detail/idempotent/disconnect branches.
• e2e: the new assertion is folded into the existing Describe("Auto-save on copy (bookshelf-qgd90.12)", Ordered, ...) journey's existing It step (no new top-level Describe), and that It already calls page = refreshPageTimeout(page) at its start — satisfying the per-It page.Timeout reset convention.
• Docs: docs/content/docs/importing/bookdrop.md is end-user framed (no source/route/internal references), and the "← Copy Missing" label matches the template exactly.

REVIEW VERDICT: 0 blocker, 0 major, 1 minor

## Code Review — PR #1424 (bookshelf-j8pvj) Reviewed the diff (bookdrop_file_editor_controller.js, bookdrop_review_controller.js, Vitest specs, e2e journey, docs). No blockers or majors found. [MINOR] static/js/controllers/bookdrop_file_editor_controller.js:390-397 — `_autoSave` dispatches `bookdrop:metadatacopied` unconditionally, even if the underlying `_doSave()` request later fails. The row gets auto-checked even when the copy wasn't actually persisted server-side. Low impact (save failure surfaces via the status indicator, and Accept flushes/gates on save success before submitting), but worth a comment noting this is deliberate optimistic-UI behavior, or moving the dispatch to the save-success path if strictness is wanted. Verified as sound: - Single dispatch point: all 7 `_autoSave()` call sites are exactly the 5 branches of `copyField` + `copyAll` + `copyMissing` (grep-verified) — no triple-dispatch risk. - Listener lifecycle: `connect()` stores `this._boundMetadataCopied = this._onMetadataCopied.bind(this)` and adds it; `disconnect()` removes the same bound reference — no leak. Vitest's MutationObserver-driven disconnect test covers this. - proposalId plumbing: editor's `proposalId: Number` Stimulus value vs review's `parseInt(row.getAttribute('data-proposal-id'), 10) === proposalId` — types match. - Idempotency: `if (!rowCheckbox || rowCheckbox.checked) return;` guards re-running `_updateUI()` on an already-checked row — covered by a dedicated Vitest spec asserting `_updateUI` is not called again. - Unknown proposalId and missing `detail.proposalId` both handled gracefully (no throw, no-op) — covered by tests. - Vitest coverage is thorough across dispatch-on-click (per-field/copyAll/copyMissing), bubbling, and review-controller check/no-check/unknown-id/missing-detail/idempotent/disconnect branches. - e2e: the new assertion is folded into the existing `Describe("Auto-save on copy (bookshelf-qgd90.12)", Ordered, ...)` journey's existing `It` step (no new top-level Describe), and that `It` already calls `page = refreshPageTimeout(page)` at its start — satisfying the per-It page.Timeout reset convention. - Docs: `docs/content/docs/importing/bookdrop.md` is end-user framed (no source/route/internal references), and the "← Copy Missing" label matches the template exactly. REVIEW VERDICT: 0 blocker, 0 major, 1 minor

zombor force-pushed bd-bookshelf-j8pvj from c1c20f2ada to 370a2d7e50 2026-08-11 14:32:16 +00:00 Compare

zombor merged commit e97814ff87 into main 2026-08-11 14:40:15 +00:00

zombor referenced this pull request from a commit 2026-08-11 14:40:19 +00:00

Merge pull request 'Bookdrop: auto-check row when fetched metadata is copied (bookshelf-j8pvj)' (#1424) from bd-bookshelf-j8pvj into main

zombor referenced this pull request 2026-08-12 19:56:22 +00:00

Bookdrop: dispatch metadatacopied on save-success, not optimistically (bookshelf-v3bi0) #1435

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

No description provided.