diff --git a/.changeset/18791-row-color-vocabulary-honesty.md b/.changeset/18791-row-color-vocabulary-honesty.md new file mode 100644 index 00000000000..93ddca86d92 --- /dev/null +++ b/.changeset/18791-row-color-vocabulary-honesty.md @@ -0,0 +1,73 @@ +--- +'@objectstack/spec': minor +--- + +fix(spec): `rowColor`'s own prescription stops handing authors the one spelling the renderer drops (#18791) + +Clause-②: yes + +`RowColorConfigSchema.colors` advertised `Map of field value to color (hex/token)`. +The only renderer — objectui `plugin-grid`'s `useRowColor` — hands a `bg-`-prefixed +literal through untouched, otherwise lower-cases and trims the value and resolves it +through its own closed vocabulary of colour NAMES, and returns `undefined` for +everything else. A hex is not a key, and Tailwind v4 has no runtime, so no class can +be fabricated from one. + +The `view/row-color-without-colors` diagnostic checks PRESENCE only, so every link in +the chain was shipping code except the author's step: the gate fires, **the gate +itself hands the author a hex**, the hex parses, publishes, turns the gate green, and +colours nothing. A control whose own prescription switches it off. Measured, not +argued: #18787's reverse-verification leg B swapped four colour names for the four +hexes the `priority` field already declares — the app-local resolvability arm went red +naming all four while the presence arm stayed green. + +Three things change, none of which moves an accept set: + +- **The describe** now names the two spellings that actually reach a class, and names + a hex only as the thing that does not. An author who comes to ask "can I paste the + option colours in?" now finds the answer instead of an invitation. +- **The `fix` string** the presence diagnostic emits prescribes a resolvable colour + name. `token` went with the hex: read as the renderer's colour names it was still + standing beside hex as an equal alternative, and putting a bad option first is as + harmful as offering only the bad option. The string is pinned by feeding the value + it suggests back through `checkViewCompleteness`, so the prescription can only ever + name something the new rule below accepts. +- **A new author-time warning, `view/row-color-unresolvable-value`**, reports values + the resolver drops. This is the half presence-only structurally cannot see: a hex + map CLEARS the `!config.colors` guard, which is exactly what silences the older + rule. + +The new rule judges the SHAPE a value has, and deliberately does not transcribe +objectui's 23-entry map. Two structural facts about the resolver are enough and +neither depends on what the map contains: the `bg-` branch tests the raw value, and +every key is a bare lower-case word matched after `toLowerCase()` and `trim()`. So a +value that is neither `bg-`-prefixed nor a bare alphabetic word once normalised cannot +be a key, whatever the map holds. That makes the rule **sound** — it never accuses a +value the renderer would have resolved, including `'RED'` and `' red '` — and +deliberately **incomplete**: an unknown colour name such as `chartreuse` is shaped +like a key and is passed, pinned as a NON-rule. A hand-copy of another repo's +vocabulary is a second opinion that drifts silently in both directions, and where the +vocabulary should be declared so the two sides cannot drift is a cross-repo question +this change deliberately does not answer. + +Not breaking, and measured rather than assumed: the finding is `warning` severity, +like its sibling. `@objectstack/lint`'s `splitBySeverity` sorts everything that is not +`error` into advisories, so `os build` / `os validate` / `os lint` still exit 0 on their +DEFAULT paths, and the registration-time twin in `@objectstack/objectql` is field-only — +it calls `checkFieldCompleteness` and never the view predicate — and warns without ever +throwing. Nothing that builds today on a default run starts failing, and nothing authored +today is refused. Under `os lint --strict` / `os validate --strict` a warning IS a +failure — that is what the flag is for — so a stack carrying an unresolvable +`rowColor.colors` value, typically a hex, starts failing those strict runs on upgrade; +the fix is the one the finding prescribes: a resolvable colour name (`red`) or a complete +Tailwind background class (`bg-red-200`). + +Blast radius measured over this repo, the five example apps and objectui at the pinned +`.objectui-sha` `53ded82bf7a494f54e344e19099dbf00854b8694`: **zero** authored `colors` +maps reach this rule carrying an unresolvable value — the one shipped map, +`examples/app-showcase`'s task grid, spells all four values as colour names and resolves +clean. The pinned sibling does hold three hex `colors` literals, and they are named here +so the zero is checkable rather than asserted: all three are objectui's OWN React test +fixtures (`ObjectView.rowColorRelay-7218.test.tsx`, in `app-shell` and in `plugin-view`), +they assert a relay by `toEqual`, and they never traverse `checkViewCompleteness` — so +this rule does not judge them and does not change their verdict. diff --git a/content/docs/references/ui/view.mdx b/content/docs/references/ui/view.mdx index 256d05a9359..869a9b386ea 100644 --- a/content/docs/references/ui/view.mdx +++ b/content/docs/references/ui/view.mdx @@ -1051,7 +1051,7 @@ View filter rule | Property | Type | Required | Description | | :--- | :--- | :--- | :--- | | **field** | `string` | ✅ | Field whose value is looked up in the `colors` map below to pick a row colour (typically a select/status field). The map is what does the colouring — with no `colors`, no row is ever coloured, whatever this field holds. Author-time diagnostic `view/row-color-without-colors` reports that combination. | -| **colors** | `Record` | optional | Map of field value to color (hex/token) | +| **colors** | `Record` | optional | Map of field value to row colour. The spellings that actually paint a row are not free-form: objectui `plugin-grid`'s `useRowColor` hands a value already written as a complete Tailwind background class (`bg-red-200`) straight through, otherwise lower-cases and trims it and resolves it through its own closed vocabulary of colour NAMES (`red`, `blue`, `slate`, … each mapping to `bg-NAME-100`), and returns undefined for anything else. A hex, an `rgb()` or a CSS variable parses here, publishes, and colours no row — Tailwind v4 has no runtime, so no class can be fabricated from one. Author-time diagnostic `view/row-color-unresolvable-value` reports a value that cannot resolve. | ### Nested Shape: `ListView.bulkActionDefs[number]` @@ -1449,7 +1449,7 @@ View filter rule | Property | Type | Required | Description | | :--- | :--- | :--- | :--- | | **field** | `string` | ✅ | Field whose value is looked up in the `colors` map below to pick a row colour (typically a select/status field). The map is what does the colouring — with no `colors`, no row is ever coloured, whatever this field holds. Author-time diagnostic `view/row-color-without-colors` reports that combination. | -| **colors** | `Record` | optional | Map of field value to color (hex/token) | +| **colors** | `Record` | optional | Map of field value to row colour. The spellings that actually paint a row are not free-form: objectui `plugin-grid`'s `useRowColor` hands a value already written as a complete Tailwind background class (`bg-red-200`) straight through, otherwise lower-cases and trims it and resolves it through its own closed vocabulary of colour NAMES (`red`, `blue`, `slate`, … each mapping to `bg-NAME-100`), and returns undefined for anything else. A hex, an `rgb()` or a CSS variable parses here, publishes, and colours no row — Tailwind v4 has no runtime, so no class can be fabricated from one. Author-time diagnostic `view/row-color-unresolvable-value` reports a value that cannot resolve. | ### Nested Shape: `ObjectListView.bulkActionDefs[number]` @@ -1607,7 +1607,7 @@ Row color configuration based on field values | Property | Type | Required | Description | | :--- | :--- | :--- | :--- | | **field** | `string` | ✅ | Field whose value is looked up in the `colors` map below to pick a row colour (typically a select/status field). The map is what does the colouring — with no `colors`, no row is ever coloured, whatever this field holds. Author-time diagnostic `view/row-color-without-colors` reports that combination. | -| **colors** | `Record` | optional | Map of field value to color (hex/token) | +| **colors** | `Record` | optional | Map of field value to row colour. The spellings that actually paint a row are not free-form: objectui `plugin-grid`'s `useRowColor` hands a value already written as a complete Tailwind background class (`bg-red-200`) straight through, otherwise lower-cases and trims it and resolves it through its own closed vocabulary of colour NAMES (`red`, `blue`, `slate`, … each mapping to `bg-NAME-100`), and returns undefined for anything else. A hex, an `rgb()` or a CSS variable parses here, publishes, and colours no row — Tailwind v4 has no runtime, so no class can be fabricated from one. Author-time diagnostic `view/row-color-unresolvable-value` reports a value that cannot resolve. | --- diff --git a/packages/spec/api-surface/kernel.json b/packages/spec/api-surface/kernel.json index 28c96e28e22..e7dd1ab8e3a 100644 --- a/packages/spec/api-surface/kernel.json +++ b/packages/spec/api-surface/kernel.json @@ -436,6 +436,7 @@ "UpgradeSnapshotParsed (type)", "UpgradeSnapshotSchema (const)", "VIEW_LAYOUT_WITHOUT_BINDING (const)", + "VIEW_ROW_COLOR_UNRESOLVABLE_VALUE (const)", "VIEW_ROW_COLOR_WITHOUT_COLORS (const)", "VIEW_TREE_WITHOUT_PARENT_FIELD (const)", "ValidationError (type)", diff --git a/packages/spec/export-origins/kernel.json b/packages/spec/export-origins/kernel.json index 5d943613131..295a526bc42 100644 --- a/packages/spec/export-origins/kernel.json +++ b/packages/spec/export-origins/kernel.json @@ -433,6 +433,7 @@ "UpgradeSnapshotParsed": "src/kernel/package-upgrade.zod.ts#UpgradeSnapshotParsed (type)", "UpgradeSnapshotSchema": "src/kernel/package-upgrade.zod.ts#UpgradeSnapshotSchema (const)", "VIEW_LAYOUT_WITHOUT_BINDING": "src/kernel/functional-completeness.ts#VIEW_LAYOUT_WITHOUT_BINDING (const)", + "VIEW_ROW_COLOR_UNRESOLVABLE_VALUE": "src/kernel/functional-completeness.ts#VIEW_ROW_COLOR_UNRESOLVABLE_VALUE (const)", "VIEW_ROW_COLOR_WITHOUT_COLORS": "src/kernel/functional-completeness.ts#VIEW_ROW_COLOR_WITHOUT_COLORS (const)", "VIEW_TREE_WITHOUT_PARENT_FIELD": "src/kernel/functional-completeness.ts#VIEW_TREE_WITHOUT_PARENT_FIELD (const)", "ValidationError": "src/kernel/plugin-validator.zod.ts#ValidationError (type)", diff --git a/packages/spec/src/kernel/functional-completeness.test.ts b/packages/spec/src/kernel/functional-completeness.test.ts index 75f9687797e..125e926c577 100644 --- a/packages/spec/src/kernel/functional-completeness.test.ts +++ b/packages/spec/src/kernel/functional-completeness.test.ts @@ -29,6 +29,7 @@ import { VIEW_LAYOUT_WITHOUT_BINDING, VIEW_TREE_WITHOUT_PARENT_FIELD, VIEW_ROW_COLOR_WITHOUT_COLORS, + VIEW_ROW_COLOR_UNRESOLVABLE_VALUE, WEBHOOK_WITHOUT_TRIGGERS, } from './functional-completeness'; @@ -331,10 +332,46 @@ describe('checkViewCompleteness — rowColor without a colour map (the parse-cle expect(f.fix).toContain('colors'); }); + // #18791 — the sharpest half of this card. The `fix` string this rule hands + // the author read `colors: { '': '' }`, and a hex + // is the ONE spelling `colorToClass` cannot resolve. So the chain ran: the + // gate fires, the gate itself hands the author a hex, the hex parses, + // publishes, turns this rule GREEN, and colours nothing. A control whose own + // prescription switches it off. + // + // Pinning the literal string would rot. What is pinned instead is the + // PROPERTY that made it wrong: the value the prescription suggests is fed + // back through this module, and must survive it. + it('hands the author a prescription this module itself accepts (#18791)', () => { + const f = only(checkViewCompleteness({ type: 'grid', rowColor: { field: 'status' } }) as never); + const suggested = /'':\s*'([^']+)'/.exec(f.fix)?.[1]; + expect(suggested, `no suggested colour value in the prescription: ${f.fix}`).toBeDefined(); + expect( + checkViewCompleteness({ type: 'grid', rowColor: { field: 'status', colors: { open: suggested! } } }), + `the prescription suggests \`${suggested}\`, which this module's own resolvability rule rejects — ` + + 'the gate would be handing the author the defect it just reported', + ).toEqual([]); + }); + + it('⛔ names no hex placeholder anywhere in the prescription (#18791)', () => { + // The direct, dumb half of the pin above: whatever the wording becomes, it + // must not put a hex back in front of an author. `token` is refused for the + // same reason — it named nothing an author could look up, and stood beside + // hex as an equal alternative. + const f = only(checkViewCompleteness({ type: 'grid', rowColor: { field: 'status' } }) as never); + expect(f.fix).not.toMatch(/hex|#[0-9a-f]{3}|token/i); + }); + it('is silent once a `colors` map is declared — the negative fixture', () => { + // ⚠️ This fixture used to spell the colour `'#0f0'`. That hex is exactly + // the shape the sibling rule below exists to catch, so the negative + // fixture for THIS rule was modelling the trap: it asserted "presence is + // enough" over a map that colours nothing. The value is now a resolvable + // colour name, which is what makes this a clean negative for one rule + // instead of a silent positive for the other. expect(checkViewCompleteness({ type: 'grid', - rowColor: { field: 'status', colors: { open: '#0f0' } }, + rowColor: { field: 'status', colors: { open: 'green' } }, })).toEqual([]); }); @@ -379,6 +416,109 @@ describe('checkViewCompleteness — rowColor without a colour map (the parse-cle }); }); +/** + * #18791 — the half `view/row-color-without-colors` structurally cannot see. + * + * `RowColorConfigSchema.colors` is `z.record(z.string(), z.string())`, so every + * string parses. `useRowColor.ts`'s `colorToClass` resolves far less: a + * `bg-`-prefixed literal passes through, the lower-cased and trimmed value is + * looked up in a closed vocabulary of colour NAMES, and everything else returns + * `undefined`. A hex map therefore CLEARS the `!config.colors` guard — which is + * to say it turns the presence rule GREEN — and colours nothing, which is why + * presence-only can never be the detector for it. + * + * Measured, not argued: PR #18787's reverse-verification leg B swapped four + * colour names for the four hexes the `priority` field already declares; the + * app-local resolvability arm went red naming all four, and the presence arm + * stayed green. + */ +describe('checkViewCompleteness — rowColor values the renderer resolves to nothing (#18791)', () => { + const grid = (colors: Record) => + checkViewCompleteness({ type: 'grid', rowColor: { field: 'priority', colors } }); + + it('flags a hex map as a WARNING, naming every dead value', () => { + const f = only(grid({ low: '#94A3B8', high: '#EF4444' }) as never); + expect(f.rule).toBe(VIEW_ROW_COLOR_UNRESOLVABLE_VALUE); + expect(f.severity).toBe('warning'); + expect(f.path).toBe('rowColor.colors'); + // The author has to be able to find them, so each offending entry is named + // with the value it holds — a count alone sends them re-reading the map. + expect(f.message).toContain('`low` = "#94A3B8"'); + expect(f.message).toContain('`high` = "#EF4444"'); + // …and the runtime symbol that makes it true, per this module's discipline. + expect(f.message).toContain('colorToClass'); + expect(f.fix).toContain("field: 'priority'"); + }); + + it('⭐ says out loud that this shape SILENCES the presence rule', () => { + // The whole reason the card is p1: the obvious "fix" for + // `view/row-color-without-colors` is to paste the field's own option + // colours in, which are hexes — strictly worse than the original defect, + // because it removes the one signal that was working. A finding that does + // not say so invites exactly that move again. + const f = only(grid({ low: '#94A3B8' }) as never); + expect(f.message).toContain(VIEW_ROW_COLOR_WITHOUT_COLORS); + expect(grid({ low: '#94A3B8' }).map((x) => x.rule)).not.toContain(VIEW_ROW_COLOR_WITHOUT_COLORS); + }); + + it('accepts what the renderer accepts — colour names and `bg-` classes', () => { + // The showcase's shipped map, verbatim. + expect(grid({ low: 'slate', medium: 'blue', high: 'amber', urgent: 'red' })).toEqual([]); + // A complete Tailwind class is handed through untouched by `colorToClass`. + expect(grid({ open: 'bg-red-200', shut: 'bg-emerald-50/50' })).toEqual([]); + // The lookup lower-cases and trims, so these resolve too. A rule that + // tested the raw value would report both — a false prescription. + expect(grid({ open: 'RED', shut: ' green ' })).toEqual([]); + }); + + it('flags the other unresolvable spellings, not just hex', () => { + for (const dead of ['rgb(255,0,0)', 'var(--danger)', '#f00', 'hsl(0 100% 50%)', 'red-500', '']) { + const findings = grid({ open: dead }); + expect(findings.map((x) => x.rule), `\`${dead}\` should be reported`) + .toContain(VIEW_ROW_COLOR_UNRESOLVABLE_VALUE); + } + // ⚠️ ` bg-red-100` with a leading space is NOT resolvable: `startsWith` + // tests the RAW value and sees the space, and the lower-cased form is not a + // bare word either. Pinned because it is the one place where "looks like a + // Tailwind class" and "resolves" come apart. + expect(grid({ open: ' bg-red-100' }).map((x) => x.rule)).toContain(VIEW_ROW_COLOR_UNRESOLVABLE_VALUE); + }); + + it('⛔ PINNED NON-RULE: an unknown colour NAME is passed, deliberately', () => { + // `chartreuse` is shaped like a key and is almost certainly not one, so + // this rule lets it through. That is the price of refusing to transcribe + // another repo's 23-entry map: a copy drifts silently in both directions, + // and a rule that accuses a value the renderer WOULD have resolved is the + // false prescription this module's discipline forbids. Sound, not complete + // — if someone "completes" it by pasting the vocabulary in, this is where + // the trade-off they are reversing is written down. + expect(grid({ open: 'chartreuse' })).toEqual([]); + }); + + it('⛔ does not double-report the shapes the presence rule owns', () => { + // `{}` is the presence rule's second spelling; it must not also arrive here + // as "zero resolvable values", which would report one defect twice in two + // vocabularies — the thing the sibling block's own tests refuse. + const empty = checkViewCompleteness({ type: 'grid', rowColor: { field: 'priority', colors: {} } }); + expect(empty.map((f) => f.rule)).toEqual([VIEW_ROW_COLOR_WITHOUT_COLORS]); + }); + + it('is silent on the view types whose renderer never reads `rowColor`', () => { + for (const type of ['kanban', 'gallery', 'chart', 'timeline']) { + const findings = checkViewCompleteness({ type, rowColor: { field: 'priority', colors: { a: '#fff' } } }); + expect(findings.map((f) => f.rule)).not.toContain(VIEW_ROW_COLOR_UNRESOLVABLE_VALUE); + } + }); + + it('leaves what the schema refuses to the schema, and never throws', () => { + // Non-string values and non-record maps are parse errors, not completeness + // findings — this module is not a second parser. + expect(grid({ open: 42, shut: null })).toEqual([]); + expect(checkViewCompleteness({ type: 'grid', rowColor: { field: 'priority', colors: 'red' } })).toEqual([]); + expect(() => grid({ open: { nested: true } })).not.toThrow(); + }); +}); + describe('checkWebhookCompleteness — the rule the runtime comment argued against', () => { it('flags a webhook with no `triggers` as an ERROR', () => { const f = only(checkWebhookCompleteness({ name: 'notify_slack', url: 'https://x' }) as never); @@ -431,6 +571,7 @@ describe('registry hygiene', () => { 'field/relationship-without-reference', 'field/summary-without-operations', 'view/layout-without-binding', + 'view/row-color-unresolvable-value', 'view/row-color-without-colors', 'view/tree-without-parent-field', 'webhook/without-triggers', @@ -447,9 +588,10 @@ describe('registry hygiene', () => { ...checkViewCompleteness({ type: 'kanban' }), ...checkViewCompleteness({ type: 'tree', tree: {} }, { name: 'unit', fields: {} }), ...checkViewCompleteness({ type: 'grid', rowColor: { field: 'status' } }), + ...checkViewCompleteness({ type: 'grid', rowColor: { field: 'status', colors: { open: '#0f0' } } }), ...checkWebhookCompleteness({ url: 'https://x' }), ]; - expect(all).toHaveLength(9); + expect(all).toHaveLength(10); for (const f of all) { expect(f.fix.length).toBeGreaterThan(8); expect(f.message.length).toBeGreaterThan(60); diff --git a/packages/spec/src/kernel/functional-completeness.ts b/packages/spec/src/kernel/functional-completeness.ts index 7011cf6afda..5b89b1e1bd6 100644 --- a/packages/spec/src/kernel/functional-completeness.ts +++ b/packages/spec/src/kernel/functional-completeness.ts @@ -55,6 +55,14 @@ * `useRowColor.ts` `if (!config?.field || !config.colors) return undefined;` * — the row-className resolver bails before it reads a single row, so every * row keeps the default colour. See {@link VIEW_ROW_COLOR_WITHOUT_COLORS}. + * - a grid view's `rowColor.colors` VALUE the resolver cannot resolve → the + * same file, eleven lines further down: `colorToClass` returns `undefined` + * for anything that is neither a `bg-`-prefixed literal nor a member of its + * own closed vocabulary of colour names. The block clears the guard above, + * silences that rule, and still colours nothing. See + * {@link VIEW_ROW_COLOR_UNRESOLVABLE_VALUE} — and read the note on + * {@link isUnresolvableRowColor} for why this rule judges the SHAPE a value + * has rather than transcribing the other repo's 23-entry map. * - `webhook` w/o `triggers` → `auto-enqueuer.ts` `if (triggers.size === 0) … * return null`. Note this one needed a SECOND source: that skip site's own * comment blesses the empty case as "a manual-only webhook", which reads @@ -88,6 +96,7 @@ export const FIELD_CHOICE_WITHOUT_OPTIONS = 'field/choice-without-options'; export const VIEW_LAYOUT_WITHOUT_BINDING = 'view/layout-without-binding'; export const VIEW_TREE_WITHOUT_PARENT_FIELD = 'view/tree-without-parent-field'; export const VIEW_ROW_COLOR_WITHOUT_COLORS = 'view/row-color-without-colors'; +export const VIEW_ROW_COLOR_UNRESOLVABLE_VALUE = 'view/row-color-unresolvable-value'; export const WEBHOOK_WITHOUT_TRIGGERS = 'webhook/without-triggers'; /** Every rule id this module can emit — pinned by tests so ids cannot drift. */ @@ -99,6 +108,7 @@ export const FUNCTIONAL_COMPLETENESS_RULES = [ VIEW_LAYOUT_WITHOUT_BINDING, VIEW_TREE_WITHOUT_PARENT_FIELD, VIEW_ROW_COLOR_WITHOUT_COLORS, + VIEW_ROW_COLOR_UNRESOLVABLE_VALUE, WEBHOOK_WITHOUT_TRIGGERS, ] as const; @@ -390,6 +400,68 @@ const ROW_COLOR_VIEW_TYPES: ReadonlySet = new Set(['grid']); const hasNoColorMap = (colors: unknown): boolean => colors === undefined || colors === null || (isRec(colors) && Object.keys(colors).length === 0); +/** + * Whether an authored `rowColor.colors` VALUE can never reach a class name. + * + * ## The trap this exists for + * + * `RowColorConfigSchema.colors` is `z.record(z.string(), z.string())`, so every + * string parses. The only renderer resolves far less than that — objectui + * `plugin-grid`'s `useRowColor.ts`, verbatim: + * + * ``` + * if (color.startsWith('bg-')) return color; + * const lower = color.toLowerCase().trim(); + * return Object.prototype.hasOwnProperty.call(COLOR_TO_CLASS, lower) + * ? COLOR_TO_CLASS[lower] + * : undefined; + * ``` + * + * So a hex — the spelling a select field already uses for its own option + * colours, and therefore the obvious thing to copy — clears the + * `!config.colors` guard {@link VIEW_ROW_COLOR_WITHOUT_COLORS} watches, + * SILENCES that rule, and still colours no row. Presence-only cannot see it; + * this is the check that can. + * + * ## Why it judges SHAPE and not membership — and why that is sound + * + * The obvious implementation transcribes `COLOR_TO_CLASS` here. ⛔ It is not + * done, for the reason `examples/app-showcase`'s own arm already records: a + * hand-copy of another repo's map is a second opinion that drifts, silently, in + * BOTH directions — a name objectui adds becomes a false positive here, a name + * it drops becomes a false negative. The vocabulary lives in the renderer; only + * its SHAPE is a fact this side can hold without owning it. + * + * Two structural facts about that snippet are enough, and neither depends on + * what the map contains: the `bg-` branch tests the RAW value, and every key of + * `COLOR_TO_CLASS` is a bare lower-case word, matched after `toLowerCase()` and + * `trim()`. Therefore a value that is neither `bg-`-prefixed nor a bare + * alphabetic word once lower-cased and trimmed CANNOT be a key, whatever the + * map holds. That makes this predicate **sound** (it never accuses a value the + * renderer would have resolved) and deliberately **incomplete** (a misspelled + * or simply absent colour name — `chartreuse` — is shaped like a key and is + * passed). Soundness is the half a gate must have; an over-eager rule here + * would be the false prescription this module's discipline forbids. + * + * Three consequences worth stating, all measured against the snippet above: + * + * - `'RED'` and `' red '` ARE resolvable — the lookup lower-cases and trims — + * so this predicate applies both before judging, rather than testing the raw + * value the way an app-local pin can afford to. + * - `' bg-red-100'` is NOT resolvable: `startsWith` sees the leading space, and + * the lower-cased form is not a bare word. Flagged, correctly. + * - `''` never colours (`if (!color) return undefined` one frame out) and is + * not a bare word either. Flagged, correctly. + * + * ⛔ **Recorded NON-rule:** whether a `bg-…` literal names a class Tailwind + * actually compiled is NOT judged here. `colorToClass` returns it untouched, so + * the resolver resolved it; whether the compiled stylesheet carries a rule for + * it is a fact about another repo's build, and asserting it from here would be + * asserting what was not verified. + */ +const isUnresolvableRowColor = (value: string): boolean => + !value.startsWith('bg-') && !/^[a-z]+$/.test(value.toLowerCase().trim()); + /** * Completeness of a single list-view definition (a container's `list` / * `listViews.*` entry). @@ -496,10 +568,44 @@ export function checkViewCompleteness(view: unknown, boundObject?: unknown): Com + '`useRowColor.ts` — `if (!config?.field || !config.colors) return undefined`), so every row keeps ' + 'the default background while parsing and publishing report success. An empty `colors: {}` is the ' + 'same dead shape spelled out — it passes that guard and then matches no value. The map is what ' - + 'does the colouring; the field only says which value to look up.', - fix: `rowColor: { field: '${field}', colors: { '': '' } }`, + + 'does the colouring; the field only says which value to look up. Each value is a colour NAME from ' + + 'the resolver\'s own vocabulary (`red`, `blue`, `slate`, …) or a complete Tailwind background class ' + + '(`bg-red-200`) — a hex parses, publishes and silences this very rule while still colouring ' + + `nothing (\`${VIEW_ROW_COLOR_UNRESOLVABLE_VALUE}\`).`, + // ⛔ The prescription must name a spelling that RESOLVES. It used to + // read `''`, which put the one spelling the renderer + // cannot resolve in first position: the gate fired, handed the author a + // hex, the hex parsed and published, and this rule went green over a + // grid that coloured nothing — a control whose own prescription + // switched it off. `functional-completeness.test.ts` pins this string + // by feeding the value back through `checkViewCompleteness`, so it can + // only ever suggest something the sibling rule below accepts. + fix: `rowColor: { field: '${field}', colors: { '': 'red' } }`, }); } + if (field !== undefined && isRec(view.rowColor.colors)) { + const dead = Object.entries(view.rowColor.colors) + .filter(([, colour]) => typeof colour === 'string' && isUnresolvableRowColor(colour)) + .map(([key, colour]) => `\`${key}\` = ${JSON.stringify(colour)}`); + if (dead.length > 0) { + out.push({ + rule: VIEW_ROW_COLOR_UNRESOLVABLE_VALUE, + severity: 'warning', + path: 'rowColor.colors', + message: + `A \`${type}\` view whose \`rowColor\` binds \`${field}\` declares ${dead.length} colour ` + + `value${dead.length === 1 ? '' : 's'} the renderer resolves to nothing: ${dead.join(', ')}. ` + + 'objectui `useRowColor.ts` — `colorToClass` — hands a `bg-`-prefixed literal through untouched ' + + 'and otherwise looks the lower-cased, trimmed value up in its own closed vocabulary of colour ' + + 'NAMES, returning `undefined` for everything else; Tailwind v4 has no runtime, so no class can ' + + 'be fabricated from a hex. A map like this CLEARS the `!config.colors` guard, so ' + + `\`${VIEW_ROW_COLOR_WITHOUT_COLORS}\` goes quiet, and every row still keeps its default ` + + 'background while parsing and publishing report success. Write a colour name (`red`, `blue`, ' + + '`slate`, …) or a complete Tailwind background class (`bg-red-200`).', + fix: `rowColor: { field: '${field}', colors: { '': 'red' } }`, + }); + } + } } return out; diff --git a/packages/spec/src/ui/view.test.ts b/packages/spec/src/ui/view.test.ts index 162772c6506..8d76005ba66 100644 --- a/packages/spec/src/ui/view.test.ts +++ b/packages/spec/src/ui/view.test.ts @@ -2613,12 +2613,56 @@ describe('RowColorConfigSchema', () => { }, }; + // ⚠️ #18791 — this ASSERTION is correct and is deliberately left alone: the + // accept set really is `z.record(z.string(), z.string())`, and hexes really + // do parse. What is wrong is believing a parse means a colour. None of + // these three values resolves — objectui `useRowColor.ts`'s `colorToClass` + // returns `undefined` for every one of them — so this map parses, + // publishes, and paints nothing. The schema is not the enforcement point + // for that; the author-time diagnostic + // `view/row-color-unresolvable-value` (`kernel/functional-completeness.ts`) + // is, and it reports exactly this fixture. expect(() => RowColorConfigSchema.parse(rowColor)).not.toThrow(); }); it('should require field', () => { expect(() => RowColorConfigSchema.parse({})).toThrow(); }); + + // #18791 — the describe read `Map of field value to color (hex/token)`, and a + // hex is the one spelling the only renderer cannot resolve. The schema told + // an author to write the value that silently does nothing; the diagnostic + // that would have caught it checks presence only, so the hex map turned it + // GREEN. This pins the two properties that made the old sentence a trap, + // rather than the wording that replaced it. + it('⛔ the `colors` describe never offers a hex — it names what actually resolves (#18791)', () => { + const description = (RowColorConfigSchema as unknown as { + shape: { colors: { description?: string } }; + }).shape.colors.description ?? ''; + + // ⛔ The trap literal, verbatim from the sentence this replaced. + expect(description).not.toContain('hex/token'); + // ⚠️ `token` went with it. Read as the renderer's colour NAMES it was still + // standing beside hex as an equal alternative, and putting a bad option + // first is as harmful as offering only the bad option. + expect(description).not.toMatch(/\btokens?\b/i); + + // ⭐ The property, not the wording: hex must still be NAMED — an author who + // comes here asking "can I paste the option colours in?" has to find the + // answer — but only ever in the same sentence as the consequence. Deleting + // the word would pass a bare `not.toMatch(/hex/)` and leave that reader + // with nothing, which is how the old sentence got written in the first + // place. + expect(description, 'the describe answers the hex question nowhere').toMatch(/hex/i); + for (const sentence of description.split('. ')) { + if (/hex/i.test(sentence)) expect(sentence).toContain('colours no row'); + } + + // And it names a spelling that DOES reach a class, plus the rule that + // reports the ones that do not. + expect(description).toContain('bg-red-200'); + expect(description).toContain('view/row-color-unresolvable-value'); + }); }); describe('ListColumnSchema pinned and summary', () => { @@ -2747,13 +2791,18 @@ describe('Airtable-style ListView enhancements', () => { it('should accept list view with row color', () => { const listView: ListView = { columns: ['name', 'priority'], + // #18791 — colour NAMES, not hexes. The assertion below only says the + // shape parses, and a hex parses just as well; what changed is that a + // fixture is read as an example, and this corpus was demonstrating the + // one spelling `colorToClass` resolves to `undefined`. The deliberate + // "a hex does parse" pin is kept, once, in `RowColorConfigSchema` above. rowColor: { field: 'priority', colors: { - critical: '#ff0000', - high: '#ff8800', - medium: '#ffcc00', - low: '#00cc00', + critical: 'red', + high: 'orange', + medium: 'amber', + low: 'green', }, }, }; @@ -2839,12 +2888,14 @@ describe('Airtable-style ListView enhancements', () => { ], }, rowHeight: 'medium', + // #18791 — colour NAMES: this is the "realistic, fully-loaded view" + // fixture, so it is the one most likely to be copied as a template. rowColor: { field: 'status', colors: { - on_track: '#22c55e', - at_risk: '#f59e0b', - blocked: '#ef4444', + on_track: 'emerald', + at_risk: 'amber', + blocked: 'red', }, }, hiddenFields: ['internal_id', 'sys_updated_at'], diff --git a/packages/spec/src/ui/view.zod.ts b/packages/spec/src/ui/view.zod.ts index 6ff7ba320a8..0592d26d7fb 100644 --- a/packages/spec/src/ui/view.zod.ts +++ b/packages/spec/src/ui/view.zod.ts @@ -1186,7 +1186,7 @@ export const RowColorConfigSchema = lazySchema(() => strictObject({ history: VIEW_HISTORY, }, { field: z.string().describe('Field whose value is looked up in the `colors` map below to pick a row colour (typically a select/status field). The map is what does the colouring — with no `colors`, no row is ever coloured, whatever this field holds. Author-time diagnostic `view/row-color-without-colors` reports that combination.'), - colors: z.record(z.string(), z.string()).optional().describe('Map of field value to color (hex/token)'), + colors: z.record(z.string(), z.string()).optional().describe('Map of field value to row colour. The spellings that actually paint a row are not free-form: objectui `plugin-grid`\'s `useRowColor` hands a value already written as a complete Tailwind background class (`bg-red-200`) straight through, otherwise lower-cases and trims it and resolves it through its own closed vocabulary of colour NAMES (`red`, `blue`, `slate`, … each mapping to `bg-NAME-100`), and returns undefined for anything else. A hex, an `rgb()` or a CSS variable parses here, publishes, and colours no row — Tailwind v4 has no runtime, so no class can be fabricated from one. Author-time diagnostic `view/row-color-unresolvable-value` reports a value that cannot resolve.'), }).describe('Row color configuration based on field values')); /**