diff --git a/.changeset/warn-hardcoded-surface-colors.md b/.changeset/warn-hardcoded-surface-colors.md new file mode 100644 index 0000000..449927a --- /dev/null +++ b/.changeset/warn-hardcoded-surface-colors.md @@ -0,0 +1,10 @@ +--- +"sideshow": patch +--- + +Warn at publish time when an html surface hardcodes scheme colors. A +`background`/`color` set to a literal (hex/rgb/hsl/white/black) instead of a +`--color-*` theme token renders washed-out on a board in the opposite scheme; +the publish/revise API now returns a non-blocking `warnings` array pointing the +agent at the theme tokens. Token-driven surfaces (and other surface kinds) are +unaffected. diff --git a/server/app.ts b/server/app.ts index f208132..4966551 100644 --- a/server/app.ts +++ b/server/app.ts @@ -33,6 +33,7 @@ import { type TraceStep, } from "./types.ts"; import { validateSurfaces } from "./postSurfaces.ts"; +import { lintSurfaces } from "./surfaceLint.ts"; export type { FeedEvent } from "./events.ts"; @@ -581,7 +582,8 @@ export function createApp({ agent?: string; cwd?: string; }): Promise< - { surface: Post; userFeedback?: Feedback[] } | { error: string; status: 400 | 404 | 413 } + | { surface: Post; userFeedback?: Feedback[]; warnings?: string[] } + | { error: string; status: 400 | 404 | 413 } > { if (input.parts.length === 0) { return { error: "a post needs at least one surface", status: 400 }; @@ -611,7 +613,12 @@ export function createApp({ }); if (!surface) return { error: "session not found", status: 404 }; bus.broadcast({ type: "post-created", id: surface.id, sessionId, version: 1 }); - return { surface, userFeedback: await collectFeedback(sessionId) }; + const warnings = lintSurfaces(input.parts); + return { + surface, + userFeedback: await collectFeedback(sessionId), + ...(warnings.length && { warnings }), + }; } // Store an uploaded blob. Like publishSurface, an explicit session is @@ -654,7 +661,8 @@ export function createApp({ id: string, patch: { parts?: Surface[]; title?: string }, ): Promise< - { surface: Post; userFeedback?: Feedback[] } | { error: string; status: 400 | 404 | 413 } + | { surface: Post; userFeedback?: Feedback[]; warnings?: string[] } + | { error: string; status: 400 | 404 | 413 } > { if (patch.parts) { if (patch.parts.length === 0) { @@ -673,7 +681,12 @@ export function createApp({ sessionId: surface.sessionId, version: surface.version, }); - return { surface, userFeedback: await collectFeedback(surface.sessionId) }; + const warnings = patch.parts ? lintSurfaces(patch.parts) : []; + return { + surface, + userFeedback: await collectFeedback(surface.sessionId), + ...(warnings.length && { warnings }), + }; } // --- per-surface flow functions (append / replace / remove / reorder) --- @@ -686,7 +699,8 @@ export function createApp({ surface: Surface, pos?: { before?: string; after?: string }, ): Promise< - { surface: Post; userFeedback?: Feedback[] } | { error: string; status: 400 | 404 | 413 } + | { surface: Post; userFeedback?: Feedback[]; warnings?: string[] } + | { error: string; status: 400 | 404 | 413 } > { const existing = await store.getPost(id); if (!existing) return { error: "post not found", status: 404 }; @@ -710,7 +724,8 @@ export function createApp({ target: string, replacement: { surface?: Surface; content?: string; kits?: unknown }, ): Promise< - { surface: Post; userFeedback?: Feedback[] } | { error: string; status: 400 | 404 | 413 } + | { surface: Post; userFeedback?: Feedback[]; warnings?: string[] } + | { error: string; status: 400 | 404 | 413 } > { const existing = await store.getPost(id); if (!existing) return { error: "post not found", status: 404 }; @@ -754,7 +769,8 @@ export function createApp({ id: string, target: string, ): Promise< - { surface: Post; userFeedback?: Feedback[] } | { error: string; status: 400 | 404 | 413 } + | { surface: Post; userFeedback?: Feedback[]; warnings?: string[] } + | { error: string; status: 400 | 404 | 413 } > { const existing = await store.getPost(id); if (!existing) return { error: "post not found", status: 404 }; @@ -771,7 +787,8 @@ export function createApp({ id: string, order: (string | number)[], ): Promise< - { surface: Post; userFeedback?: Feedback[] } | { error: string; status: 400 | 404 | 413 } + | { surface: Post; userFeedback?: Feedback[]; warnings?: string[] } + | { error: string; status: 400 | 404 | 413 } > { const existing = await store.getPost(id); if (!existing) return { error: "post not found", status: 404 }; @@ -1313,6 +1330,7 @@ export function createApp({ { ...writeResult(result.surface), ...(result.userFeedback && { userFeedback: result.userFeedback }), + ...(result.warnings && { warnings: result.warnings }), }, 201, ); @@ -1347,6 +1365,7 @@ export function createApp({ return c.json({ ...writeResult(result.surface), ...(result.userFeedback && { userFeedback: result.userFeedback }), + ...(result.warnings && { warnings: result.warnings }), }); }; app.put("/api/surfaces/:id", revise); @@ -1412,6 +1431,7 @@ export function createApp({ return c.json({ ...writeResult(result.surface), ...(result.userFeedback && { userFeedback: result.userFeedback }), + ...(result.warnings && { warnings: result.warnings }), }); }); @@ -1434,6 +1454,7 @@ export function createApp({ return c.json({ ...writeResult(result.surface), ...(result.userFeedback && { userFeedback: result.userFeedback }), + ...(result.warnings && { warnings: result.warnings }), }); }); @@ -1460,6 +1481,7 @@ export function createApp({ return c.json({ ...writeResult(result.surface), ...(result.userFeedback && { userFeedback: result.userFeedback }), + ...(result.warnings && { warnings: result.warnings }), }); }); @@ -1471,6 +1493,7 @@ export function createApp({ return c.json({ ...writeResult(result.surface), ...(result.userFeedback && { userFeedback: result.userFeedback }), + ...(result.warnings && { warnings: result.warnings }), }); }); @@ -1485,6 +1508,7 @@ export function createApp({ return c.json({ ...writeResult(result.surface), ...(result.userFeedback && { userFeedback: result.userFeedback }), + ...(result.warnings && { warnings: result.warnings }), }); }); diff --git a/server/surfaceLint.ts b/server/surfaceLint.ts new file mode 100644 index 0000000..d563ae1 --- /dev/null +++ b/server/surfaceLint.ts @@ -0,0 +1,63 @@ +// Advisory publish-time lint for surfaces. The one firm design rule is "dark +// mode is mandatory" (guide/DESIGN_GUIDE.md): an html surface must drive every +// color from the --color-* theme tokens so it adapts to the board's light/dark +// scheme. An agent that hardcodes a hex/rgb color instead renders washed-out on +// a board in the opposite scheme (e.g. a `background:#fff` note on a dark board). +// The renderer can't fix this — it can't tell an accidental light card from a +// deliberate one — so the publish path flags it back to the author as a +// non-blocking warning. It never rejects: an intentional light/dark design is +// flagged too, and the agent is free to ignore it. +import type { Surface } from "./types.ts"; + +// Adaptiveness-critical CSS properties. A hardcoded value on one of these is +// what makes a surface ignore the board scheme. Deliberately limited to +// background + text: SVG fill/stroke and decorative borders carry literal colors +// far more often without breaking adaptiveness, so including them would mostly +// add false positives. +const SCHEME_PROPS = ["background", "background-color", "color"]; + +// A literal color value: hex, rgb()/rgba(), hsl()/hsla(), or the two named +// colors that actually break a scheme (white/black). A value that reads a theme +// token (var(--…)) is adaptive by construction and never flagged — including +// `var(--x, #fff)`, where the hardcoded hex is only a last-resort fallback. +const LITERAL_COLOR = /#[0-9a-f]{3,8}\b|\brgba?\(|\bhsla?\(|\b(?:white|black)\b/i; + +// Pull hardcoded scheme colors out of an html surface's markup. Scans both +// inline `style="…"` attributes and `` rules for a +// background/color declaration whose value is a literal (and not a var()). +// Returns the offending declarations, deduped and capped, for the warning text. +export function findHardcodedColors(html: string): string[] { + // The leading lookbehind keeps `border-color` / `caret-color` from matching as + // `color` (and `-background` from matching `background`). + const decl = new RegExp(`(?= 4) break; + } + return hits; +} + +// One advisory warning per html surface that hardcodes scheme colors. Other +// kinds (markdown/code/diff/mermaid/terminal) are themed by the trusted renderer +// — markdown even escapes raw HTML — so they can't carry an applied hardcoded +// color; they're skipped. +export function lintSurfaces(parts: Surface[]): string[] { + const warnings: string[] = []; + parts.forEach((part, i) => { + if (part.kind !== "html") return; + const hits = findHardcodedColors(part.html); + if (hits.length === 0) return; + const where = parts.length > 1 ? `surface ${i + 1}` : "surface"; + warnings.push( + `${where} hardcodes colors (${hits.join(", ")}) — drive color from the ` + + `--color-* theme tokens so it adapts to light/dark; hardcoded colors render ` + + `washed-out on a board in the opposite scheme.`, + ); + }); + return warnings; +} diff --git a/test/api.test.ts b/test/api.test.ts index b1ac26d..bb8ee68 100644 --- a/test/api.test.ts +++ b/test/api.test.ts @@ -61,6 +61,45 @@ test("publish without session auto-creates one", async () => { assert.equal(sessions[0].surfaceCount, 1); }); +test("publishing an html surface that hardcodes colors returns a warning", async () => { + const app = makeApp(); + const res = await app.request( + "/api/snippets", + json({ html: '
note
' }), + ); + assert.equal(res.status, 201); + const out = (await res.json()) as any; + assert.ok(Array.isArray(out.warnings), "response should carry a warnings array"); + assert.match(out.warnings[0], /hardcodes colors/); + assert.match(out.warnings[0], /--color-\* theme tokens/); +}); + +test("a token-driven html surface publishes with no warnings", async () => { + const app = makeApp(); + const res = await app.request( + "/api/snippets", + json({ html: '
ok
' }), + ); + assert.equal(res.status, 201); + const out = (await res.json()) as any; + assert.equal(out.warnings, undefined); +}); + +test("revising into hardcoded colors warns too", async () => { + const app = makeApp(); + const first = (await ( + await app.request("/api/snippets", json({ html: "

clean

" })) + ).json()) as any; + assert.equal(first.warnings, undefined); + const res = await app.request(`/api/posts/${first.id}`, { + ...json({ surfaces: [{ kind: "html", html: '
x
' }] }), + method: "PUT", + }); + assert.equal(res.status, 200); + const out = (await res.json()) as any; + assert.match(out.warnings?.[0] ?? "", /hardcodes colors/); +}); + test("onEvent receives published feed events", async () => { const events: unknown[] = []; const app = makeApp(undefined, { onEvent: (event) => events.push(event) }); diff --git a/test/surfaceLint.test.ts b/test/surfaceLint.test.ts new file mode 100644 index 0000000..0ab9385 --- /dev/null +++ b/test/surfaceLint.test.ts @@ -0,0 +1,74 @@ +import assert from "node:assert/strict"; +import { test } from "node:test"; +import { findHardcodedColors, lintSurfaces } from "../server/surfaceLint.ts"; +import type { Surface } from "../server/types.ts"; + +const html = (s: string): Surface => ({ kind: "html", html: s }); + +test("flags a hardcoded background + text color in an inline style", () => { + const hits = findHardcodedColors('
x
'); + assert.deepEqual(hits, ["background: #ffffff", "color: #57606a"]); +}); + +test("flags rgb()/rgba()/hsl() and the named scheme-breakers", () => { + assert.ok(findHardcodedColors('

x

').length); + assert.ok(findHardcodedColors('

x

').length); + assert.ok(findHardcodedColors('

x

').length); + assert.ok(findHardcodedColors('

x

').length); + assert.ok(findHardcodedColors('

x

').length); +}); + +test("flags colors in a
"); + assert.deepEqual(hits, ["background: #0d1117"]); +}); + +test("does NOT flag theme-token values (var(--color-*))", () => { + assert.deepEqual( + findHardcodedColors( + '
x
', + ), + [], + ); +}); + +test("does NOT flag a var() with a hardcoded fallback (the literal is last-resort)", () => { + assert.deepEqual( + findHardcodedColors('
x
'), + [], + ); +}); + +test("ignores non-scheme properties (SVG fill/stroke, borders)", () => { + // fill/stroke carry literal colors constantly without breaking adaptiveness. + assert.deepEqual(findHardcodedColors(''), []); + assert.deepEqual(findHardcodedColors('
x
'), []); +}); + +test("caps the reported declarations at 4", () => { + const many = Array.from({ length: 10 }, (_, i) => `color:#${i}${i}${i}`).join(";"); + assert.equal(findHardcodedColors(`
x
`).length, 4); +}); + +test("lintSurfaces only warns on html surfaces, with a 1-based surface label", () => { + const parts: Surface[] = [ + { kind: "markdown", markdown: "color:#fff is just text here" }, + html('
x
'), + ]; + const warnings = lintSurfaces(parts); + assert.equal(warnings.length, 1); + assert.match(warnings[0], /^surface 2 hardcodes colors \(background: #fff\)/); + assert.match(warnings[0], /--color-\* theme tokens/); +}); + +test("lintSurfaces says just 'surface' when there is only one", () => { + const warnings = lintSurfaces([html('
x
')]); + assert.match(warnings[0], /^surface hardcodes colors/); +}); + +test("lintSurfaces is silent for a fully token-driven surface", () => { + assert.deepEqual( + lintSurfaces([html('
ok
')]), + [], + ); +});