From e9fa47fabf5f29da69af251eba55b97913561ae0 Mon Sep 17 00:00:00 2001 From: Jean-Philippe Sirois Date: Fri, 31 Jul 2026 20:27:20 -0300 Subject: [PATCH] ui(github): render schema drift as captions, not a list Every other expansion states its detail in captions tight under its gate row. Schema drift alone rendered a bold group heading and a bulleted list at body size, so its entries read as loud as the gate rows they sat beneath, and the roster's leading was computed against them: 27.1px entering the block and 14.9px leaving it, against 21px everywhere else. Each line now carries its own kind, which is what retires the heading. That in turn retires schemaChangeHeading, and the (removed) placeholder it was standing in for. Co-Authored-By: Claude --- src/reporters/github/github.test.ts | 32 +++++++++++++--------- src/reporters/github/github.ts | 3 -- src/reporters/github/schema-change.test.ts | 6 ++-- src/reporters/github/schema-change.ts | 21 ++++++++------ src/reporters/github/success.md.j2 | 7 ++--- 5 files changed, 37 insertions(+), 32 deletions(-) diff --git a/src/reporters/github/github.test.ts b/src/reporters/github/github.test.ts index e983008..8fca354 100644 --- a/src/reporters/github/github.test.ts +++ b/src/reporters/github/github.test.ts @@ -781,11 +781,11 @@ describe("schema change section", () => { }); const output = renderTemplate(ctx); - expect(output).toContain("Added"); - expect(output).toContain("**Added**"); - expect(output).toContain("table public.orders"); - expect(output).toContain("**Removed**"); - expect(output).toContain("index (removed)"); + // Each line states its own kind, so the block needs no heading above it. + expect(output).toContain("Added table public.orders"); + expect(output).toContain("Removed index"); + expect(output).not.toContain("**Added**"); + expect(output).not.toContain("**Removed**"); }); test("closes the schema list so the gates after it are not nested inside it", () => { @@ -800,15 +800,22 @@ describe("schema change section", () => { }), }); const lines = renderTemplate(ctx).split("\n"); + const gateRow = lines.findIndex((l) => l.includes("**Schema drift**")); + const entries = lines.filter((l) => l.startsWith("")); - // A line straight after a list item is lazy continuation: markdown folds it - // into that `
  • `, so every gate row below renders inside the list. - const lastEntry = lines.findLastIndex((l) => l.startsWith("- ")); + // Every expansion sits tight under its gate row, the way the regression and + // recommendation blocks do. A blank line here would drop the block further + // than any other caption in the comment. + expect(lines[gateRow + 1]).toBe(entries[0]); + + // But the block must close, or the roster's leading is computed against a + // 12px line box and the next gate row is pulled up tighter than the rest. + const lastEntry = lines.lastIndexOf(entries.at(-1)!); expect(lines[lastEntry + 1]).toBe(""); - // And a line straight after the gate row is a soft break inside the gate's - // own paragraph, so the group heading loses the space above it. - expect(lines[lines.indexOf("**Added**") - 1]).toBe(""); + // Caption size, not body size — otherwise the detail reads as loud as the + // gate rows it sits beneath. + expect(entries.every((l) => l.endsWith(""))).toBe(true); }); test("template renders no schema section when unchanged", () => { @@ -830,8 +837,7 @@ describe("schema change section", () => { }), }); const output = renderTemplate(ctx); - expect(output).toContain("**Added**"); - expect(output).toContain("table public.orders"); + expect(output).toContain("Added table public.orders"); }); }); diff --git a/src/reporters/github/github.ts b/src/reporters/github/github.ts index a0e300b..6badf4e 100644 --- a/src/reporters/github/github.ts +++ b/src/reporters/github/github.ts @@ -21,7 +21,6 @@ import { import type { CiQueryPayload, ImprovedQuery, RegressedQuery } from "../site-api.ts"; import { buildSchemaChangeView, - schemaChangeHeading, schemaChangeLabel, type SchemaChangeView, } from "./schema-change.ts"; @@ -242,7 +241,6 @@ export function buildViewModel(ctx: ReportContext) { hasComparison: false, queryLinks, schemaChange, - schemaChangeHeading, schemaChangeLabel, modeledTablesNotice, gateSummary, @@ -310,7 +308,6 @@ export function buildViewModel(ctx: ReportContext) { hasComparison: true, queryLinks, schemaChange, - schemaChangeHeading, schemaChangeLabel, modeledTablesNotice, gateSummary, diff --git a/src/reporters/github/schema-change.test.ts b/src/reporters/github/schema-change.test.ts index 2d516a6..6b1bf33 100644 --- a/src/reporters/github/schema-change.test.ts +++ b/src/reporters/github/schema-change.test.ts @@ -54,7 +54,7 @@ describe("buildSchemaChangeView", () => { const ops: Op[] = [{ op: "remove", path: "/constraints/2" }]; const view = buildSchemaChangeView(ops); const removed = entriesFor(view, "removed"); - expect(removed).toEqual([{ kind: "removed", object: "constraint", name: "(removed)" }]); + expect(removed).toEqual([{ kind: "removed", object: "constraint", name: "" }]); }); test("property-level replace is a 'changed' entry carrying the sub-path", () => { @@ -106,12 +106,12 @@ describe("schemaChangeLabel", () => { test("named entry", () => { expect( schemaChangeLabel({ kind: "added", object: "table", name: "public.users" }), - ).toBe("table public.users"); + ).toBe("Added table public.users"); }); test("changed entry with detail and no name", () => { expect( schemaChangeLabel({ kind: "changed", object: "index", name: "", detail: "isUnique" }), - ).toBe("index · isUnique"); + ).toBe("Changed index · isUnique"); }); }); diff --git a/src/reporters/github/schema-change.ts b/src/reporters/github/schema-change.ts index f5dd2e9..79ddbf2 100644 --- a/src/reporters/github/schema-change.ts +++ b/src/reporters/github/schema-change.ts @@ -119,7 +119,10 @@ function entryFromOp(op: Op): SchemaChangeEntry | null { return { kind: "added", object, name: name ?? "(unknown)" }; } if (op.op === "remove" && isElementRoot) { - return { kind: "removed", object, name: "(removed)" }; + // A `remove` op carries no value, so there is no name to print. The line + // reads "removed index" — the kind prefix already says what happened, which + // the old "(removed)" placeholder was standing in for. + return { kind: "removed", object, name: "" }; } // Property-level add/replace, or a nested remove — all "changed" on the object. const detail = parsed.rest.length > 0 ? parsed.rest.join(".") : undefined; @@ -152,18 +155,20 @@ export function buildSchemaChangeView(operations: Op[]): SchemaChangeView { }; } -const KIND_HEADINGS: Record = { +const KIND_LABELS: Record = { added: "Added", removed: "Removed", changed: "Changed", }; -export function schemaChangeHeading(kind: SchemaChangeKind): string { - return KIND_HEADINGS[kind]; -} - -/** One-line label for an entry, e.g. "table public.users" or "index users.idx · isUnique". */ +/** + * One-line label for an entry, e.g. "Added table public.users" or + * "Changed index users.idx · isUnique". Each line states its own kind, so the + * entries need no group heading above them — which is what lets the block sit + * tight under its gate row the way every other expansion does. + */ export function schemaChangeLabel(entry: SchemaChangeEntry): string { - const base = entry.name ? `${entry.object} ${entry.name}` : entry.object; + const object = entry.name ? `${entry.object} ${entry.name}` : entry.object; + const base = `${KIND_LABELS[entry.kind]} ${object}`; return entry.detail ? `${base} · ${entry.detail}` : base; } diff --git a/src/reporters/github/success.md.j2 b/src/reporters/github/success.md.j2 index 49b0fec..16177f3 100644 --- a/src/reporters/github/success.md.j2 +++ b/src/reporters/github/success.md.j2 @@ -29,15 +29,12 @@ {% endfor %} {% endif %} {% if g.condition == "schema-drift" and g.fired and schemaChange.hasChanges %} - {% for group in schemaChange.groups %} -**{{ schemaChangeHeading(group.kind) }}** - {% for entry in group.entries %} -- {{ schemaChangeLabel(entry) }} +{{ schemaChangeLabel(entry) }} {% endfor %} - {% endfor %} + {% endif %} {% endfor %}