fix(review): publication tags removed in the re-review tab are actually removed (#635)

[berntpopp]

Fixes #635.

The bug

useReviewForm.submitForm() did not submit the curator's selection. It submitted the union of that selection and the set loaded from the server:

// BUG-05 fix: Merge original publications with current form data
const mergedPublications = [...new Set([...originalPublications.value, ...formData.publications])];

Removing a PMID cleared it from the reactive form — which is why the chip disappeared and the reporter saw the UI update — and the union restored it one line before serialisation. The PUT carried the pre-edit set, publication_replace_for_review() faithfully re-INSERTed the row, and the save reported success. GeneReviews had the identical defect.

The server was never complicit. publication_replace_for_review() is an honest DELETE-then-INSERT of exactly what it is sent, and the endpoint's empty-literature branch already means "remove them all". No API change was needed for the fix.

The union lived in exactly one composable. That is the report's other observation, explained:

Surface	Composable	Union?
Re-review Edit-Review modal	useReviewForm.ts	yes — the bug
Modify Entity	useEntityInfo.ts	no
Approve Review	useReviewApprovalActions.ts	no

The guard defended against a reactivity failure the current bindings cannot produce: ReviewFormFields.vue binds v-model straight onto the composable's own reactive object, loadReviewData() is atomic (a rejection propagates out of infoReview() before show()), and the modal never reads a draft back. A guard that cannot distinguish "the curator cleared the field" from "a hypothetical desync" is not a guard — it is this bug. It was also enshrined in a unit test ("Simulate form reactivity issue"), which is inverted here.

Two further defects, found by adversarially reviewing the fix

• A failed metadata fetch minted a duplicate review. submitReviewChange() derived create-vs-update from review_info.review_id, populated by useReviewData.loadReviewInfo() — which catches its errors, reports them, and returns, after which the modal opens anyway. On that failure isUpdate fell to false and the save POSTed /api/review/create, forking the curation record and INSERTing publications onto the new row. It now reads useReviewForm's own reviewId, whose load is atomic.
• The publication-decrease warning never fired on the path that can lose data. It sat inside publication_replace_for_review()'s is.null(conn) branch; the review write path always passes txn_conn. Moved into the shared path and read on the write connection, so it is transaction-local. Verified live: WARN Publication count decreasing for review 3155: 2 -> 1.

Verification

• Unit, red-green proven by reverting each fix in turn: useReviewForm.publications.spec.ts (9 tests — remove-one / remove-all / remove-genereview / add+remove / whitespace-dedupe), Review.submitMode.spec.ts (4), test-unit-review-literature-parsing.R (23 assertions across all four populated/empty literature shapes), test-unit-review-write-repositories.R (the count must be read on the caller's connection).
• E2E review.publication-removal.spec.ts drives the real modal and asserts through GET /api/review/<id>/publications — the check a UI-only assertion would have passed while the bug was live. Proven red against the pre-fix bundle: it fails on "the removed publication must be gone from the entity" only after the chip-removed and save-returned-200 steps pass, reproducing the report exactly.
• Manual monkey test on the dev stack as a curator: removed a PMID from a real re-review entity, captured the outgoing payload (additional_references: ["PMID:32705143"], method PUT), confirmed the row was gone from ndd_review_publication_join, confirmed exactly one review row for the entity, saw the new WARN line — then restored every mutated row.
• make ci-local ✅ · make code-quality-audit ✅ · 305 vitest files / 2436 tests ✅ · type-check + strict ✅ · lint 0 errors ✅ · build:bundle-budget ✅
• Playwright --workers=1: 165 passed, 3 env-gated skips, 1 pre-existing failure — see Known unrelated failure below. The fixture correction also re-activates four Variation ontology provenance: mark machine-derived annotations and stop silent promotion to curator-authored #608 provenance tests that were skipping behind the reviewSaveWorks probe.

Fixture correction

The baseline seeded publication_id = '12345678' / publication_type = 'PMID', which matches neither filter in loadReviewData() — so the row rendered as no chip at all, which is why this bug shipped without end-to-end coverage. Production holds only the two real shapes (join table 8148 additional_references / 1099 gene_review; publication 4304 / 386; zero of anything else — queried live), so both tables are corrected to match.

Folded-in Dependabot PRs

Closes #620 · Closes #628 · Closes #633 · Closes #634

#620 (markdown-it 14 → 15) was not a drop-in and is the reason it is worth calling out:

1. It bundles its own TypeScript declarations; @types/markdown-it is removed.
2. It moves to linkify-it v6, which disables fuzzy links by default — bare example.com would have silently stopped auto-linking in curator-authored About/CMS content. useMarkdownRenderer had no test at all, so nothing pinned this. It now has one; the fuzzy-link case was written against 14, went red on the bump, and is green again only because of an explicit md.linkify.set({ fuzzyLink: true }).

Because the three npm branches conflicted on the lockfile, it was regenerated. npm resolves each caret range to the newest in-range release, so five packages land slightly above their Dependabot pin, and three transitive, dev-only packages advance (svgo 4.1.0 and the css-select 6 / css-what 7 majors it requires). All are dev: true in the lockfile — build-time CSS/SVG minification that cannot enter the shipped bundle. Every intended bump is present at or above its pin. Detailed in the CHANGELOG rather than left silent.

Known unrelated failure

analyses.data-releases.spec.ts fails locally against the empty-DB Playwright fixture. /api/analysis/releases returns {"releases":[]} and the page correctly renders "No releases published yet", but the spec's guard uses non-waiting isVisible() before the async fetch resolves, so it never takes its own empty-state continue. No file in this diff is involved. Left for #573 — the real fix is deciding whether the fixture should seed a release, not silencing the guard.

Review

Two adversarial passes with Codex gpt-5.6-sol (high), archived under .planning/reviews/: the spec review pre-code (11 findings) and the diff review (4 findings). Each was triaged against the code rather than accepted — two spec-review findings were factually wrong and are rebutted with live query results. The diff review explicitly cleared the core fix, including that this.reviewForm.reviewId.value is correct because setup() ref-unwrapping is shallow.

One thing neither the spec nor either review caught, and only driving the real browser did: BFormTag renders title from its internal tagText, and ReviewFormFields.vue fills the slot with a <BLink>, so the attribute is the literal "[object Object]". The planned .b-form-tag[title="PMID:…"] selector matches nothing and would have made the E2E pass vacuously.

🤖 Generated with Claude Code

https://claude.ai/code/session_014EDxFc8ddBksJf33dibjN1