From 27c395ea0d427dc1f89c9957598c1b10c3ebaa5d Mon Sep 17 00:00:00 2001 From: Vito Sansevero Date: Fri, 30 Oct 2026 11:12:32 -0700 Subject: [PATCH] fix(review): keep manual links out of the foreign repair and show dialog states The repair tool could relink a manually linked row and drop its pair link. It now skips those rows and refuses an apply when one became manual after the plan. The picker's disabled action and chosen prompt are now visibly distinct. --- tests/browser/workspace-review.spec.mjs | 2 + tests/test_repair_foreign_links.py | 45 ++++++++++++++++ tools/repair_foreign_links.py | 10 ++++ tools/vendor/frontend-coverage-baseline.json | 56 ++++++++++---------- web/js/workspace/review-link.mjs | 2 +- web/js/workspace/workspace.css | 3 +- 6 files changed, 88 insertions(+), 30 deletions(-) diff --git a/tests/browser/workspace-review.spec.mjs b/tests/browser/workspace-review.spec.mjs index d509f57e..6f552b50 100644 --- a/tests/browser/workspace-review.spec.mjs +++ b/tests/browser/workspace-review.spec.mjs @@ -642,6 +642,8 @@ test('#536: nothing selectable says why and links through the picker, then the n const negative = dialog.getByRole('combobox', { name: 'Negative prompt', exact: true }); await expect(negative.locator('option')).toHaveText(['Choose a negative…', 'washed out', 'grainy', 'No negative']); await expect(submit).toHaveAttribute('aria-disabled', 'true'); + await expect(submit).toHaveCSS('opacity', '0.55'); + await expect(harbor).toHaveAttribute('aria-pressed', 'true'); await negative.selectOption({ label: 'grainy' }); const linked = page.waitForResponse(item => /\/link$/.test(item.url())); await submit.click(); diff --git a/tests/test_repair_foreign_links.py b/tests/test_repair_foreign_links.py index 61a81910..bf1692f1 100644 --- a/tests/test_repair_foreign_links.py +++ b/tests/test_repair_foreign_links.py @@ -285,6 +285,51 @@ class DryRunTests(RepairFixture): self.assertIn("would lose every link", markdown) +class ManualLinkTests(RepairFixture): + """A prompt the reviewer named in Needs review (#536) is not ours to undo.""" + + def mark_manual(self, key): + row = self.rows[key] + ((prompt,),) = self.query( + "SELECT prompt_id FROM generated_images WHERE id=?", (row,) + ) + self.query_write( + "INSERT OR IGNORE INTO prompt_pairs(positive_prompt_id,created_at," + "updated_at) VALUES(?,'2026-10-30T00:00:00Z','2026-10-30T00:00:00Z')", + (prompt,), + ) + ((pair,),) = self.query( + "SELECT id FROM prompt_pairs WHERE positive_prompt_id=? " + "AND negative_prompt_id IS NULL", + (prompt,), + ) + self.query_write( + "INSERT INTO image_prompt_pairs(pair_id,image_id,provenance,created_at) " + "VALUES(?,?,?,'2026-10-30T00:00:00Z')", + (pair, row, '{"kind":"manual-link"}'), + ) + + def test_a_manual_link_is_skipped_in_the_plan(self): + self.mark_manual("A") + code, out = self.dry_run() + self.assertEqual(code, 0, out) + items = {item["row_id"]: item for item in self.plan()["rows"]} + item = items[self.rows["A"]] + self.assertEqual(item["action"], "skip") + self.assertEqual(item["reason"], "linked by hand in Needs review") + self.assertEqual(items[self.rows["B"]]["action"], "relink") + + def test_a_manual_link_made_after_the_plan_refuses_the_apply(self): + code, out = self.dry_run() + self.assertEqual(code, 0, out) + self.mark_manual("A") + before = self.links() + code, out = self.apply() + self.assertNotEqual(code, 0, out) + self.assertIn("action changed", out) + self.assertEqual(self.links(), before) + + class DryRunReviewTests(RepairFixture): def test_plan_records_its_library_and_fingerprint(self): self.dry_run() diff --git a/tools/repair_foreign_links.py b/tools/repair_foreign_links.py index 699faa96..fe75611d 100644 --- a/tools/repair_foreign_links.py +++ b/tools/repair_foreign_links.py @@ -112,6 +112,7 @@ COUNTED = ( # The ladder steps whose answer is certain enough to keep a link. KEEP_SOURCES = ("recorded-run", "inspector") # What must be unchanged since the dry run for a row to be written. +MANUAL_REASON = "linked by hand in Needs review" DRIFT_KEYS = ( "action", "file", @@ -181,6 +182,13 @@ class Judge: ) return self.decisions[path] + def _manually_linked(self, image_id): + """True when the reviewer named this row's prompt in Needs review (#536).""" + return any( + link["provenance"].get("kind") == "manual-link" + for link in self.db.get_image_prompt_pairs(image_id) + ) + def row(self, row): """The plan entry of one ``generated_images`` row.""" conn = self.db.model.get_connection() @@ -200,6 +208,8 @@ class Judge: except OSError: return {**item, "action": "skip", "reason": "file gone"} item.update(file_size=stat.st_size, file_mtime_ns=stat.st_mtime_ns) + if self._manually_linked(row["id"]): + return {**item, "action": "skip", "reason": MANUAL_REASON} decision = self.decide(path, stat.st_size) action = action_of(decision, prompt_hash) if action == "skip": diff --git a/tools/vendor/frontend-coverage-baseline.json b/tools/vendor/frontend-coverage-baseline.json index 583e3160..6d15c317 100644 --- a/tools/vendor/frontend-coverage-baseline.json +++ b/tools/vendor/frontend-coverage-baseline.json @@ -43,14 +43,14 @@ }, "gallery-settings.js": { "lines": 98, - "branches": 94, + "branches": 95, "functions": 98, "statements": 98 }, "gallery.js": { "lines": 54, "branches": 72, - "functions": 57, + "functions": 58, "statements": 54 }, "html-escape.js": { @@ -210,10 +210,10 @@ "statements": 98 }, "workspace/file-inspector.mjs": { - "lines": 96, + "lines": 97, "branches": 93, "functions": 93, - "statements": 96 + "statements": 97 }, "workspace/heading.mjs": { "lines": 98, @@ -282,16 +282,10 @@ "statements": 98 }, "workspace/images-delete.mjs": { - "lines": 80, - "branches": 70, - "functions": 80, - "statements": 80 - }, - "workspace/images-return.mjs": { - "lines": 80, - "branches": 70, - "functions": 80, - "statements": 80 + "lines": 95, + "branches": 81, + "functions": 92, + "statements": 95 }, "workspace/images-export.mjs": { "lines": 98, @@ -311,6 +305,12 @@ "functions": 98, "statements": 98 }, + "workspace/images-return.mjs": { + "lines": 93, + "branches": 86, + "functions": 98, + "statements": 93 + }, "workspace/images-selection.mjs": { "lines": 98, "branches": 93, @@ -349,7 +349,7 @@ }, "workspace/images.mjs": { "lines": 97, - "branches": 89, + "branches": 90, "functions": 98, "statements": 97 }, @@ -362,7 +362,7 @@ "workspace/inspect.mjs": { "lines": 98, "branches": 92, - "functions": 93, + "functions": 94, "statements": 98 }, "workspace/inspection-state.mjs": { @@ -373,7 +373,7 @@ }, "workspace/inspector-collections.mjs": { "lines": 98, - "branches": 92, + "branches": 93, "functions": 98, "statements": 98 }, @@ -391,7 +391,7 @@ }, "workspace/library-editor.mjs": { "lines": 98, - "branches": 93, + "branches": 94, "functions": 92, "statements": 98 }, @@ -492,10 +492,10 @@ "statements": 98 }, "workspace/more-menu.mjs": { - "lines": 80, - "branches": 70, - "functions": 80, - "statements": 80 + "lines": 98, + "branches": 86, + "functions": 98, + "statements": 98 }, "workspace/plural.mjs": { "lines": 98, @@ -540,14 +540,14 @@ "statements": 95 }, "workspace/review-link.mjs": { - "lines": 80, - "branches": 50, - "functions": 70, - "statements": 80 + "lines": 95, + "branches": 70, + "functions": 91, + "statements": 95 }, "workspace/review-state.mjs": { "lines": 96, - "branches": 84, + "branches": 85, "functions": 98, "statements": 96 }, @@ -613,7 +613,7 @@ }, "workspace/system-sources.mjs": { "lines": 98, - "branches": 88, + "branches": 89, "functions": 95, "statements": 98 }, diff --git a/web/js/workspace/review-link.mjs b/web/js/workspace/review-link.mjs index a5f76946..c0eff877 100644 --- a/web/js/workspace/review-link.mjs +++ b/web/js/workspace/review-link.mjs @@ -71,7 +71,7 @@ export function openPromptLink({ api, scope, controller, filename, trigger }) { const warning = el('p', '', 'warning-note'); warning.setAttribute('role', 'status'); warning.hidden = true; - const actions = el('div', undefined, 'dialog-actions'); + const actions = el('div', undefined, 'dialog-actions review-link-actions'); const cancel = createButton(LINK_COPY.cancel); const submit = createButton(LINK_COPY.action, 'ca-primary'); actions.append(cancel, submit); diff --git a/web/js/workspace/workspace.css b/web/js/workspace/workspace.css index fbc3908e..022e6fde 100644 --- a/web/js/workspace/workspace.css +++ b/web/js/workspace/workspace.css @@ -438,7 +438,8 @@ pre.prompt-text { font: inherit; font-size: 12px; line-height: 1.75; margin-bloc .review-unreadable { display: grid; gap: 6px; margin-block: 12px; overflow-wrap: anywhere; } .review-link-results { display: grid; gap: 6px; max-block-size: 40vh; overflow-y: auto; } .review-link-results > button { justify-content: flex-start; inline-size: 100%; overflow: hidden; text-overflow: ellipsis; white-space: nowrap; } -.review-link-results > button[aria-pressed="true"] { border-color: var(--ca-accent); } +.review-link-results > button[aria-pressed="true"] { border-color: var(--ca-accent); background: var(--soft-blue); } +.review-panel button[aria-disabled="true"], .review-link-actions button[aria-disabled="true"] { opacity: .55; cursor: not-allowed; } /* Auto-tag selected images (#249 PR D, D85/D90): the Tag image dialog's model panel, then How to apply, a radio group in the concept's `.field` row whose radios keep their own size (the concept's `.field input` is a full-width text field) beside their label, as the concept's own radio rows (s1160 `.compare-item label`). */