Skip to content

feat(review): add a visible caption under each Visual/Scroll preview thumbnail #6341

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
loopover-orb merged 1 commit into from
Jul 16, 2026
Merged

feat(review): add a visible caption under each Visual/Scroll preview thumbnail #6341

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 8 additions & 2 deletions src/review/unified-comment-bridge.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -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;
Expand Down
16 changes: 16 additions & 0 deletions test/unit/visual-collapsible.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down Expand Up @@ -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", () => {
Expand Down