From 29db94479bc6133a94d135f5e4021f6d66ad2bf5 Mon Sep 17 00:00:00 2001
From: JSONbored <49853598+JSONbored@users.noreply.github.com>
Date: Wed, 15 Jul 2026 19:48:45 -0700
Subject: [PATCH] feat(review): add a visible caption under each Visual/Scroll
 preview thumbnail

Advances #6324

The screenshot-table contract this bot enforces on contributors (see e.g.
.claude/skills/metagraphed/SKILL.md's Phase B2 in JSONbored/metagraphed)
requires "each image is a clickable thumbnail... with a one-line caption
underneath". The bot's own Visual preview / Scroll preview tables never
rendered one -- the route/viewport/theme label existed only as an
invisible alt attribute.

Adds a <br><sub>label</sub> line (the same label already used for alt)
after each thumbnail, inside the same table cell (a literal newline would
break the GFM table row). Purely additive to both buildBeforeAfterCollapsible
and buildScrollPreviewCollapsible's cell() builders -- every existing
assertion about the <a>/<img> markup still matches unchanged.

This is one of two fixes for #6324; the image-downscaling half (the
"before images are massive" file-size complaint) is a separate, larger
change and will land in its own follow-up PR.
---
 src/review/unified-comment-bridge.ts | 10 ++++++++--
 test/unit/visual-collapsible.test.ts | 16 ++++++++++++++++
 2 files changed, 24 insertions(+), 2 deletions(-)

diff --git a/src/review/unified-comment-bridge.ts b/src/review/unified-comment-bridge.ts
index 426da542c6..e3ffcc9222 100644
--- a/src/review/unified-comment-bridge.ts
+++ b/src/review/unified-comment-bridge.ts
@@ -458,8 +458,13 @@ export function buildBeforeAfterCollapsible(routes: CaptureRoute[]): UnifiedColl
       .replace(/`/g, "\\`")
       .replace(/\|/g, "\\|")
       .replace(/[<>]/g, (char) => (char === "<" ? "&lt;" : "&gt;"))}\``;
+  // #6324: the same one-line caption UNDER the thumbnail that the screenshot-table contract itself requires
+  // of contributors (see e.g. .claude/skills/metagraphed/SKILL.md's Phase B2 in JSONbored/metagraphed) --
+  // previously only present as the invisible `alt` attribute, never rendered as visible text. <br> (not a
+  // literal newline, which would break the GFM table row) keeps the caption inside the same cell; <sub> is
+  // the same de-emphasized styling this table already uses for its own footer legend line below.
   const cell = (url: string | undefined, label: string): string =>
-    url ? `<a href="${attr(url)}" target="_blank" rel="noopener"><img width="360" alt="${attr(label)}" src="${attr(url)}"></a>` : "—";
+    url ? `<a href="${attr(url)}" target="_blank" rel="noopener"><img width="360" alt="${attr(label)}" src="${attr(url)}"></a><br><sub>${attr(label)}</sub>` : "—";
   const rows: string[] = [];
   let hasAnyDiff = false;
   for (const route of routes) {
@@ -506,8 +511,9 @@ export function buildScrollPreviewCollapsible(routes: CaptureRoute[]): UnifiedCo
       .replace(/`/g, "\\`")
       .replace(/\|/g, "\\|")
       .replace(/[<>]/g, (char) => (char === "<" ? "&lt;" : "&gt;"))}\``;
+  // #6324: same visible one-line caption as buildBeforeAfterCollapsible's own cell() -- see its doc comment.
   const cell = (url: string | undefined, label: string): string =>
-    url ? `<a href="${attr(url)}" target="_blank" rel="noopener"><img width="360" alt="${attr(label)}" src="${attr(url)}"></a>` : "—";
+    url ? `<a href="${attr(url)}" target="_blank" rel="noopener"><img width="360" alt="${attr(label)}" src="${attr(url)}"></a><br><sub>${attr(label)}</sub>` : "—";
   const rows: string[] = [];
   for (const route of routes) {
     if (!route.beforeGifUrl && !route.afterGifUrl) continue;
diff --git a/test/unit/visual-collapsible.test.ts b/test/unit/visual-collapsible.test.ts
index 2effea8005..5ad3955434 100644
--- a/test/unit/visual-collapsible.test.ts
+++ b/test/unit/visual-collapsible.test.ts
@@ -50,6 +50,19 @@ describe("buildBeforeAfterCollapsible", () => {
     expect(c?.body).toContain("| `/` | desktop | — | <a href=");
   });
 
+  it("#6324: renders a VISIBLE one-line caption under each thumbnail, matching the contributor screenshot contract's own shape", () => {
+    const c = buildBeforeAfterCollapsible(routes);
+    // The caption text is the SAME string already used as the (invisible) alt attribute -- now also visible.
+    expect(c?.body).toContain('<img width="360" alt="before /app/analytics" src="https://api.example.dev/gittensory/shot?key=gittensory/shots/abc.png"></a><br><sub>before /app/analytics</sub>');
+    expect(c?.body).toContain('<img width="360" alt="after /app/analytics" src="https://api.example.dev/gittensory/shot?key=gittensory/shots/def.png"></a><br><sub>after /app/analytics</sub>');
+  });
+
+  it("#6324: a dash cell has no caption to escape (no <br><sub> emitted for a missing slot)", () => {
+    const c = buildBeforeAfterCollapsible([{ path: "/", afterUrl: "https://api.example.dev/gittensory/shot?key=gittensory/shots/x.png" }]);
+    expect(c?.body).toContain("| `/` | desktop | — | <a href=");
+    expect(c?.body).not.toContain("—<br>");
+  });
+
   it("returns null when no route has any shot URL (no empty table)", () => {
     expect(buildBeforeAfterCollapsible([])).toBeNull();
     expect(buildBeforeAfterCollapsible([{ path: "/" }])).toBeNull();
@@ -169,6 +182,9 @@ describe("buildScrollPreviewCollapsible (#3612)", () => {
     expect(c?.body).toContain('<a href="https://api.example.dev/gittensory/shot?key=gittensory/shots/before.gif"');
     expect(c?.body).toContain('alt="before /app/analytics (scroll)"');
     expect(c?.body).toContain('alt="after /app/analytics (scroll)"');
+    // #6324: same visible caption as buildBeforeAfterCollapsible's own cell().
+    expect(c?.body).toContain("<br><sub>before /app/analytics (scroll)</sub>");
+    expect(c?.body).toContain("<br><sub>after /app/analytics (scroll)</sub>");
   });
 
   it("returns null when no route has a scroll GIF — byte-identical to pre-#3612 for every non-opted-in repo", () => {