diff --git a/web/src/lib/components/collections/BoardView.relationGroup.svelte.test.ts b/web/src/lib/components/collections/BoardView.relationGroup.svelte.test.ts index d3eb881da..4d97c264d 100644 --- a/web/src/lib/components/collections/BoardView.relationGroup.svelte.test.ts +++ b/web/src/lib/components/collections/BoardView.relationGroup.svelte.test.ts @@ -775,6 +775,94 @@ describe('a REFUSED grouping must not write a group value (U4, codex round 1 P5) expect(plus).toHaveLength(0); }); + it('RENDERS a card whose status is a list, instead of throwing (BUG-3041)', () => { + // The crash this fixture used to be written AROUND. The leg below still + // says "the item's status is ABSENT on purpose: a card carrying an ARRAY + // there trips scalar status formatting and the board throws before + // rendering" — that was true when it was written and is what BUG-3041 + // filed. This leg is the same schema WITH the array present. + // + // `status` and `priority` are conventional key names, not reserved ones, + // so the schema editor will type them however it likes and nothing + // rewrites the values already stored. The board correctly refuses to + // GROUP by such a field; it then threw while drawing the card's chip, + // which is a rendering contract and not a grouping one. + const coll = collection(); + coll.schema = JSON.stringify({ + fields: [ + { + key: 'status', + label: 'Status', + type: 'multi_relation', + collection: 'colors', + options: ['old-a', 'old-b'], + }, + { key: 'priority', label: 'Priority', type: 'multi_relation', collection: 'colors' }, + ], + }); + const listy = (id: string) => + ({ + ...item(id), + fields: JSON.stringify({ status: ['id-red', 'id-blue'], priority: ['id-green'] }), + }) as Item; + + const screen = render(BoardView, { + props: { + items: [listy('car-1')], + collection: coll, + wsSlug: 'ws', + groupField: 'status', + onStatusChange: vi.fn(), + } as never, + }); + + // The card is THERE — the point is that the board draws rather than dies. + expect([...screen.container.querySelectorAll('.card-title')].map((e) => e.textContent?.trim())) + .toEqual(['car-1']); + expect(screen.container.textContent).toContain('more than one group'); + + // And no chip, rather than an empty one: a chip that formats nothing + // describes nothing. Both keys are covered, since `priority` reaches the + // same helpers by a different branch. + expect(screen.container.querySelectorAll('.item-card .chip')).toHaveLength(0); + // The raw ids must not leak out through a stringified array either. + expect(screen.container.textContent).not.toContain('id-red'); + }); + + it('CONTROL: ordinary string status AND priority still render their chips', () => { + // Without this, withholding the chip for EVERY value would satisfy the + // leg above, and the boards that work today would quietly lose their + // chips. PRIORITY is here because the first version of this control + // covered status alone — and the enumeration round showed that deleting + // every ItemCard priority chip left the whole suite green (d4). Each + // gate needs a leg that fails when it is removed. + const coll = collection(); + coll.schema = JSON.stringify({ + fields: [ + { key: 'status', label: 'Status', type: 'select', options: ['open', 'done'] }, + { key: 'priority', label: 'Priority', type: 'select', options: ['high', 'low'] }, + ], + }); + const withBoth = (id: string) => + ({ ...item(id), fields: JSON.stringify({ status: 'in_progress', priority: 'high' }) }) as Item; + + const screen = render(BoardView, { + props: { + items: [withBoth('car-1')], + collection: coll, + wsSlug: 'ws', + groupField: 'status', + onStatusChange: vi.fn(), + } as never, + }); + + const chips = [...screen.container.querySelectorAll('.item-card .chip')].map((c) => + (c.textContent ?? '').trim(), + ); + expect(chips).toContain('In Progress'); + expect(chips).toContain('High'); + }); + it('offers no bulk "Move all to" either — the FIFTH group-writing affordance', async () => { // Round 8 enumeration. Round 7 enumerated four affordances that write the // group value and this was not among them: the lane menu reads diff --git a/web/src/lib/components/collections/ItemCard.svelte b/web/src/lib/components/collections/ItemCard.svelte index 3e7a17b5a..6ba71ee7f 100644 --- a/web/src/lib/components/collections/ItemCard.svelte +++ b/web/src/lib/components/collections/ItemCard.svelte @@ -226,7 +226,18 @@
- {#if statusField && fields.status} + + {#if statusField && typeof fields.status === 'string' && fields.status} {#if statusCyclable} {/if} {/if} - {#if priorityField && fields.priority} + {#if priorityField && typeof fields.priority === 'string' && fields.priority} {formatLabel(fields.priority)} diff --git a/web/src/lib/components/collections/TableView.listValue.svelte.test.ts b/web/src/lib/components/collections/TableView.listValue.svelte.test.ts new file mode 100644 index 000000000..6d9db9021 --- /dev/null +++ b/web/src/lib/components/collections/TableView.listValue.svelte.test.ts @@ -0,0 +1,224 @@ +// The table's status chip asks the VALUE's shape, not the field's options +// (BUG-3041). +// +// The chip arm was gated on `field.options && onStatusChange` — and `options` +// SURVIVES a retype in the schema editor, so a `status` that is now a +// `multi_relation` still took it, with an array as its value. Two things +// followed: the scalar formatting threw (`value?.toLowerCase is not a +// function`, killing the whole table), and the chip it was drawing is CLICKABLE +// — cycling it writes a scalar into a list field, which the server refuses. +// +// So this is not only a rendering repair. The plain-text arm shows the value +// and offers no write, which is the honest state for something the chip cannot +// describe. +import { describe, it, expect, vi, afterEach } from 'vitest'; +import { render, cleanup } from '@testing-library/svelte'; +import type { Collection, Item } from '$lib/types'; + +import TableView from './TableView.svelte'; + +afterEach(() => { + cleanup(); +}); + +function collection(fields: unknown[]): Collection { + return { + id: 'c1', + workspace_id: 'ws1', + name: 'Cars', + slug: 'cars', + icon: '', + description: '', + schema: JSON.stringify({ fields }), + settings: JSON.stringify({}), + sort_order: 0, + is_default: true, + is_system: false, + created_at: '2026-01-01T00:00:00Z', + updated_at: '2026-01-01T00:00:00Z', + prefix: 'CAR', + } as unknown as Collection; +} + +function item(id: string, fields: Record): Item { + return { + id, + workspace_id: 'ws1', + collection_id: 'c1', + slug: id, + title: id, + content: '', + fields: JSON.stringify(fields), + tags: '[]', + status: 'open', + created_at: '2026-01-01T00:00:00Z', + updated_at: '2026-01-01T00:00:00Z', + } as unknown as Item; +} + +const chips = (c: HTMLElement) => [...c.querySelectorAll('.chip')].map((e) => (e.textContent ?? '').trim()); + +describe('a status field holding a LIST', () => { + // Retained `options` are what make this reachable: the schema editor does not + // strip them when a field's type changes, and nothing rewrites the values + // already stored. + const retyped = collection([ + { key: 'status', label: 'Status', type: 'multi_relation', collection: 'colors', options: ['old-a', 'old-b'] }, + ]); + + it('renders the row instead of throwing, and offers no status chip', () => { + const onStatusChange = vi.fn(); + const screen = render(TableView, { + props: { + items: [item('car-1', { status: ['id-red', 'id-blue'] })], + collection: retyped, + onStatusChange, + } as never, + }); + + // PRECONDITION: the row is there. "No chip" must not be "no table". + expect(screen.container.textContent).toContain('car-1'); + expect(chips(screen.container)).toHaveLength(0); + // The cell still SHOWS something — a fix that deleted the offending value, + // or every non-string cell, would satisfy "no chip" while losing the data + // (enumeration round, d2). + expect(screen.container.querySelector('.cell-value')?.textContent).toContain('id-red'); + }); + + it('and offers no WRITE either — the withheld chip is the point, not its styling', () => { + // The chip this arm draws is CLICKABLE, and cycling it sends a SCALAR into + // a list field, which the server refuses. Asserting the absence of a + // `.chip` class checks decoration; this clicks every control in the row + // and asserts nothing reached the handler (enumeration round, d3). + const onStatusChange = vi.fn(); + const screen = render(TableView, { + props: { + items: [item('car-1', { status: ['id-red', 'id-blue'] })], + collection: retyped, + onStatusChange, + } as never, + }); + + const clickables = [...screen.container.querySelectorAll('.table-cell [role="button"], .table-cell button, .table-cell .chip')]; + for (const el of clickables) (el as HTMLElement).click(); + expect(onStatusChange).not.toHaveBeenCalled(); + }); + + it('CONTROL: the same click DOES cycle an ordinary string status', () => { + // The counterfactual for the leg above — without it, a row that rendered + // no controls at all would pass, and so would a chip made inert. + const ordinary = collection([ + { key: 'status', label: 'Status', type: 'select', options: ['open', 'in_progress'] }, + ]); + const onStatusChange = vi.fn(); + const screen = render(TableView, { + props: { + items: [item('car-1', { status: 'open' })], + collection: ordinary, + onStatusChange, + } as never, + }); + + const chip = screen.container.querySelector('.chip') as HTMLElement | null; + expect(chip, 'no chip to click').not.toBeNull(); + chip!.click(); + expect(onStatusChange).toHaveBeenCalledTimes(1); + expect(onStatusChange.mock.calls[0][1]).toBe('in_progress'); + }); + + it('keeps the setter chip for a row with NO status stored', () => { + // The affordance the first version of the gate removed: with nothing + // stored, the chip renders empty and clicking it sets the first option, + // which is how a row gets its first status. An absent value and an empty + // string must not diverge here (enumeration round, c2). + const ordinary = collection([ + { key: 'status', label: 'Status', type: 'select', options: ['open', 'in_progress'] }, + ]); + const onStatusChange = vi.fn(); + const screen = render(TableView, { + props: { items: [item('car-1', {})], collection: ordinary, onStatusChange } as never, + }); + + const chip = screen.container.querySelector('.chip') as HTMLElement | null; + expect(chip, 'the missing-status setter chip is gone').not.toBeNull(); + chip!.click(); + expect(onStatusChange).toHaveBeenCalledTimes(1); + expect(onStatusChange.mock.calls[0][1]).toBe('open'); + }); + + it('CONTROL: a string status in the same column still gets its clickable chip', () => { + // Without this, withholding the chip for every value would satisfy the leg + // above while removing a working affordance from every table. + const ordinary = collection([ + { key: 'status', label: 'Status', type: 'select', options: ['open', 'in_progress'] }, + ]); + const screen = render(TableView, { + props: { + items: [item('car-1', { status: 'in_progress' })], + collection: ordinary, + onStatusChange: vi.fn(), + } as never, + }); + + expect(chips(screen.container)).toContain('In Progress'); + const clickable = [...screen.container.querySelectorAll('.chip')].filter( + (c) => c.getAttribute('title') === 'Click to cycle status', + ); + expect(clickable).toHaveLength(1); + }); + + it('a STRING value in a non-status column still reaches the shared resolver', () => { + // REPLACES a leg that supplied an array here and claimed to exercise the + // resolver. It did not: that branch has ALWAYS required a string, so the + // leg measured the fallback text and passed against the original + // implementation too — verified by the enumeration round (d1), which + // restored the old TableView and watched it stay green. + // + // A string is what reaches the resolver, so that is what this supplies, + // and the assertion is the resolver's ANSWER: a canonical status word in + // a non-status column gets the status palette, not plain text. + const other = collection([ + { key: 'flavour', label: 'Flavour', type: 'select', options: ['blocked', 'plain'] }, + ]); + const screen = render(TableView, { + props: { + items: [item('car-1', { flavour: 'blocked' })], + collection: other, + onStatusChange: vi.fn(), + } as never, + }); + expect(chips(screen.container)).toContain('Blocked'); + + cleanup(); + + // CONTROL: a word in neither vocabulary renders as text, no chip — which + // is what makes the assertion above about the resolver rather than about + // "this column draws chips". + const screen2 = render(TableView, { + props: { + items: [item('car-2', { flavour: 'plain' })], + collection: other, + onStatusChange: vi.fn(), + } as never, + }); + expect(chips(screen2.container)).toHaveLength(0); + expect(screen2.container.querySelector('.cell-value')?.textContent).toContain('plain'); + }); + + it('a list value in a NON-status column renders as text and does not throw', () => { + const other = collection([ + { key: 'flavour', label: 'Flavour', type: 'multi_relation', collection: 'colors', options: ['a'] }, + ]); + const screen = render(TableView, { + props: { + items: [item('car-1', { flavour: ['id-red'] })], + collection: other, + onStatusChange: vi.fn(), + } as never, + }); + + expect(screen.container.textContent).toContain('car-1'); + expect(chips(screen.container)).toHaveLength(0); + expect(screen.container.querySelector('.cell-value')?.textContent).toContain('id-red'); + }); +}); diff --git a/web/src/lib/components/collections/TableView.svelte b/web/src/lib/components/collections/TableView.svelte index 3ad4b3cc6..0ee508e10 100644 --- a/web/src/lib/components/collections/TableView.svelte +++ b/web/src/lib/components/collections/TableView.svelte @@ -4,7 +4,7 @@ import { itemComparator, type SortMode } from '$lib/collections/itemSort'; import { reorderGroup, disabledDirections, type ReorderDirection } from '$lib/collections/reorder'; import { page } from '$app/state'; - import { statusColor, priorityColor, hasCanonicalStatus, formatFieldLabel as formatLabel } from '$lib/utils/fieldColors'; + import { statusColor, canonicalValueColor, formatFieldLabel as formatLabel } from '$lib/utils/fieldColors'; import Chip from '$lib/components/common/Chip.svelte'; import EmptyState from '../common/EmptyState.svelte'; import ItemActionsMenu from './ItemActionsMenu.svelte'; @@ -162,17 +162,6 @@ onStatusChange(item, next); } - /** FieldEditor's canonical-value resolution: canonical statuses + the four - * priorities get the fieldColors palette; unknown values return null so - * the cell renders as plain text instead of a chip. */ - function selectValueColor(val: string): string | null { - if (hasCanonicalStatus(val)) return statusColor(val); - const p = val?.toLowerCase(); - if (p === 'critical' || p === 'high' || p === 'medium' || p === 'low') { - return priorityColor(val); - } - return null; - } function relativeTime(dateStr: string): string { const now = Date.now(); @@ -270,7 +259,24 @@
{#each visibleFields as field (field.key)}
- {#if field.key === 'status' && field.options && onStatusChange} + + {#if field.key === 'status' && field.options && onStatusChange && (fields[field.key] == null || typeof fields[field.key] === 'string')} {:else if field.options && typeof fields[field.key] === 'string' && fields[field.key]} - {@const chipColor = selectValueColor(fields[field.key])} + {@const chipColor = canonicalValueColor(fields[field.key])} {#if chipColor} {formatLabel(fields[field.key])} {:else} diff --git a/web/src/lib/components/fields/FieldEditor.svelte b/web/src/lib/components/fields/FieldEditor.svelte index 27d5022d0..ae6b571a4 100644 --- a/web/src/lib/components/fields/FieldEditor.svelte +++ b/web/src/lib/components/fields/FieldEditor.svelte @@ -29,7 +29,7 @@ handlers — onchange is never called. import { shouldOpenInPane } from '$lib/components/collections/itemCardClick'; import BottomSheet from '$lib/components/common/BottomSheet.svelte'; import { viewport } from '$lib/stores/breakpoint.svelte'; - import { statusColor, priorityColor, hasCanonicalStatus, formatFieldLabel as formatLabel } from '$lib/utils/fieldColors'; + import { canonicalValueColor, formatFieldLabel as formatLabel } from '$lib/utils/fieldColors'; interface Props { field: FieldDef; @@ -649,16 +649,9 @@ handlers — onchange is never called. let triggerEl: HTMLButtonElement | undefined = $state(undefined); let dropdownEl: HTMLDivElement | undefined = $state(undefined); - /** Canonical palette from $lib/utils/fieldColors; priorities colored too. - * Returns null for unrecognized values so the color dot is suppressed. */ - function getStatusColor(val: string): string | null { - if (hasCanonicalStatus(val)) return statusColor(val); - const p = val?.toLowerCase(); - if (p === 'critical' || p === 'high' || p === 'medium' || p === 'low') { - return priorityColor(val); - } - return null; - } + /** The canonical palette lives in `$lib/utils/fieldColors`; this used to be + * a third copy of `canonicalValueColor` and is now an alias (BUG-3041). */ + const getStatusColor = canonicalValueColor; function toggleDropdown() { dropdownOpen = !dropdownOpen; diff --git a/web/src/lib/utils/fieldColors.test.ts b/web/src/lib/utils/fieldColors.test.ts new file mode 100644 index 000000000..c7d608d07 --- /dev/null +++ b/web/src/lib/utils/fieldColors.test.ts @@ -0,0 +1,185 @@ +// The canonical-palette helpers are TOTAL over what a fields blob can hold +// (BUG-3041). +// +// These five functions are handed values straight out of an item's `fields` +// JSON. Every one of them was typed `string` and none of them checked, so an +// array reached `.toLowerCase()` / `.replace()` and RENDERING THREW — +// `value?.toLowerCase is not a function` — taking down the whole board or table +// rather than one chip. A declared field type is not a promise about the stored +// value: nothing rewrites existing data when a field is retyped in the schema +// editor, so a `status` that was a `multi_select` can hold an array while its +// schema says `multi_relation`. +// +// THE INPUT DOMAIN IS JSON, not "string or nothing", so the legs below are +// drawn from what JSON can actually put in that blob — array, number, boolean, +// object, null, undefined — rather than from the one non-string value the +// original report happened to name. +import { describe, it, expect } from 'vitest'; +import { + statusColor, + priorityColor, + hasCanonicalStatus, + formatFieldLabel, + columnAccentClassFor, + canonicalValueColor, +} from './fieldColors'; + +/** Everything a `fields` blob can hold that is not a string. */ +const NON_STRINGS: [string, unknown][] = [ + ['an array of references', ['uuid-a', 'uuid-b']], + ['an empty array', []], + ['a number', 42], + ['a boolean', true], + ['an object', { a: 1 }], + ['null', null], + ['undefined', undefined], +]; + +describe('the palette helpers do not throw on a non-string value', () => { + for (const [label, value] of NON_STRINGS) { + it(`answers a USABLE colour for ${label}`, () => { + // Not-throwing was the whole assertion here at first, and the + // enumeration round pointed out that returning `undefined`, an object, + // or a wrong colour would all have passed. The colour helpers always + // return a colour, so the honest one for an unreadable value is muted. + expect(statusColor(value)).toBe(MUTED); + expect(priorityColor(value)).toBe(MUTED); + }); + + it(`survives ${label}`, () => { + // Each assertion is the CALL not throwing plus a usable answer: a + // helper that threw would fail before any expectation ran, which is + // the failure mode this file exists for. + expect(() => statusColor(value)).not.toThrow(); + expect(() => priorityColor(value)).not.toThrow(); + expect(() => hasCanonicalStatus(value)).not.toThrow(); + expect(() => formatFieldLabel(value)).not.toThrow(); + expect(() => columnAccentClassFor(undefined, value)).not.toThrow(); + expect(() => canonicalValueColor(value)).not.toThrow(); + }); + + it(`reads ${label} as "no canonical value", not as some value`, () => { + // Not-throwing is not enough: answering as though the value WERE a + // known status would colour a chip by accident. The honest answer for + // something this palette cannot read is the unknown one. + expect(hasCanonicalStatus(value)).toBe(false); + expect(canonicalValueColor(value)).toBeNull(); + expect(formatFieldLabel(value)).toBe(''); + expect(columnAccentClassFor({ terminal_options: ['done'] }, value)).toBe(''); + }); + } +}); + +/** + * The palette's actual values, written out. + * + * The first version of this file asserted RELATIONSHIPS — that two canonical + * statuses agree, that a priority equals what the shared resolver returns — + * and the enumeration round showed why that is not enough: forcing every + * priority colour to muted left every leg green, because the assertions + * compared the implementation against itself. A palette test has to name the + * colours (harness note `end-state-assertions-need-a-counterfactual`). + */ +const GREEN = 'var(--accent-green)'; +const AMBER = 'var(--accent-amber)'; +const BLUE = 'var(--status-blue)'; +const ORANGE = 'var(--accent-orange)'; +const GRAY = 'var(--accent-gray)'; +const MUTED = 'var(--text-muted)'; +const SECONDARY = 'var(--text-secondary)'; + +describe('CONTROL: strings still resolve exactly as before', () => { + // Without these, making every helper answer '' for everything would satisfy + // the file above completely. + it('canonical statuses keep their exact colours', () => { + expect(hasCanonicalStatus('in_progress')).toBe(true); + expect(hasCanonicalStatus('in-progress')).toBe(true); + expect(statusColor('done')).toBe(GREEN); + expect(statusColor('completed')).toBe(GREEN); + expect(statusColor('in_progress')).toBe(AMBER); + expect(statusColor('open')).toBe(BLUE); + expect(statusColor('blocked')).toBe(ORANGE); + expect(statusColor('cancelled')).toBe(GRAY); + }); + + it('an UNKNOWN string reads muted rather than borrowing a palette entry', () => { + expect(statusColor('shipped')).toBe(MUTED); + }); + + it('an unknown string is unknown, which is not the same as unreadable', () => { + // The distinction the non-string legs turn on: 'shipped' is a value the + // palette read and did not recognise; an array is one it cannot read. + // Both end at "no canonical colour", and only one of them used to crash. + expect(hasCanonicalStatus('shipped')).toBe(false); + expect(canonicalValueColor('shipped')).toBeNull(); + expect(columnAccentClassFor({ terminal_options: ['shipped'] }, 'shipped')).toBe('col-done'); + }); + + it('priorities keep their exact colours, not merely the helper\'s own answer', () => { + // Named values, for the reason above: comparing `canonicalValueColor` to + // `priorityColor` passes however wrong both of them are together. + expect(priorityColor('critical')).toBe(ORANGE); + expect(priorityColor('high')).toBe(AMBER); + expect(priorityColor('medium')).toBe(SECONDARY); + expect(priorityColor('low')).toBe(MUTED); + expect(canonicalValueColor('Medium')).toBe(SECONDARY); + expect(canonicalValueColor('critical')).toBe(ORANGE); + }); + + it('critical is ORANGE and not red — red is reserved for destructive actions', () => { + // A long-standing app convention that a "make it louder" edit would break + // without any other leg noticing. + expect(priorityColor('critical')).not.toContain('danger'); + expect(priorityColor('critical')).not.toContain('error'); + }); + + it('formats underscores and casing the way the chips expect', () => { + expect(formatFieldLabel('in_progress')).toBe('In Progress'); + expect(formatFieldLabel('done')).toBe('Done'); + }); + + it('an EMPTY string stays empty rather than becoming a label', () => { + expect(formatFieldLabel('')).toBe(''); + expect(canonicalValueColor('')).toBeNull(); + }); +}); + +describe('canonicalValueColor is the one copy of a question that had two', () => { + it('answers the status palette for a word in BOTH vocabularies', () => { + // The precedence claim, stated honestly. The two vocabularies are + // currently DISJOINT — no canonical status is also a priority word — so + // no fixture built from them can distinguish "statuses first" from + // "priorities first", and the leg that used 'blocked' to prove ordering + // proved nothing (enumeration round, d6). + // + // What CAN be pinned is the observable consequence: 'blocked' is a status + // and takes the status colour, 'high' is a priority and takes the + // priority one. If the vocabularies ever overlap, whoever adds the + // overlapping word owns the ordering question, and this comment is where + // they will find it. + expect(canonicalValueColor('blocked')).toBe(ORANGE); + expect(canonicalValueColor('high')).toBe(AMBER); + expect(statusColor('high')).toBe(MUTED); + }); + + it('leaves a value in neither vocabulary to plain text', () => { + expect(canonicalValueColor('needs-triage')).toBeNull(); + }); +}); + +describe('a value that happens to name an OBJECT PROPERTY is not a status', () => { + // `STATUS_COLORS` is an object literal, so `'__proto__' in STATUS_COLORS` is + // true and `STATUS_COLORS['constructor']` is a FUNCTION. Both are reachable + // from a plain string a user can type into a text field, and neither is a + // CSS value — so the helpers answered yes to a status they have never heard + // of and handed their callers something unusable (enumeration round, a16). + for (const key of ['__proto__', 'constructor', 'toString', 'hasOwnProperty', 'valueOf']) { + it(`treats ${key} as an ordinary unknown value`, () => { + expect(hasCanonicalStatus(key)).toBe(false); + expect(statusColor(key)).toBe(MUTED); + expect(typeof statusColor(key)).toBe('string'); + expect(canonicalValueColor(key)).toBeNull(); + expect(columnAccentClassFor({ terminal_options: [] }, key)).toBe(''); + }); + } +}); diff --git a/web/src/lib/utils/fieldColors.ts b/web/src/lib/utils/fieldColors.ts index 10e76cc83..37c24ce39 100644 --- a/web/src/lib/utils/fieldColors.ts +++ b/web/src/lib/utils/fieldColors.ts @@ -65,19 +65,48 @@ const STATUS_COLORS: Record = { deprecated: MUTED, }; -/** Normalize a select value for lookup: lowercase, hyphens → underscores. */ -function norm(value: string): string { - return value?.toLowerCase().replace(/-/g, '_') ?? ''; +/** + * Normalize a select value for lookup: lowercase, hyphens → underscores. + * + * Takes `unknown`, and answers '' for anything that is not a string (BUG-3041). + * These helpers are handed values straight out of an item's `fields` blob, and + * a field's DECLARED type is not a guarantee about what is STORED there: + * nothing rewrites existing values when a field is retyped in the schema + * editor, so a `status` that used to be a `multi_select` can hold an array + * while its schema says `multi_relation` — or `number`, or `json`. The old + * signature said `string` and the old body's `?.` guarded only null and + * undefined, so an array reached `.toLowerCase()` and rendering THREW: + * `value?.toLowerCase is not a function`, with the whole board or table going + * down rather than one chip. + * + * Typed `unknown` rather than `string` on purpose. A `string` parameter that + * the compiler cannot enforce at these call sites is a claim, not a check — + * every caller reads from a `Record` — and it was the claim that + * made the crash invisible until it happened. + */ +function norm(value: unknown): string { + return typeof value === 'string' ? value.toLowerCase().replace(/-/g, '_') : ''; } -/** Canonical status → color (CSS var reference). Unknown values read muted. */ -export function statusColor(status: string): string { - return STATUS_COLORS[norm(status)] ?? MUTED; +/** + * Canonical status → color (CSS var reference). Unknown values read muted. + * + * `Object.hasOwn` rather than a bare index (BUG-3041, enumeration round). The + * map is an ordinary object literal, so `STATUS_COLORS['__proto__']` answers + * `Object.prototype` and `['constructor']` answers a FUNCTION — neither is a + * CSS value, and both are reachable from a plain string a user can type into a + * text field. `?? MUTED` does not catch them, because both are truthy. The + * signature promises a colour string; without this it returned whatever the + * prototype chain had. + */ +export function statusColor(status: unknown): string { + const key = norm(status); + return Object.hasOwn(STATUS_COLORS, key) ? STATUS_COLORS[key] : MUTED; } /** Canonical priority → color. Critical is orange by long-standing app * convention (red stays reserved for destructive actions). */ -export function priorityColor(priority: string): string { +export function priorityColor(priority: unknown): string { switch (norm(priority)) { case 'critical': return ORANGE; @@ -95,12 +124,23 @@ export function priorityColor(priority: string): string { /** True when the canonical status palette has an exact entry for the value — * lets schema-aware callers (shareView.fieldValueColor) fall back to * terminal_options semantics for custom vocabularies. */ -export function hasCanonicalStatus(value: string): boolean { - return norm(value) in STATUS_COLORS; +export function hasCanonicalStatus(value: unknown): boolean { + // OWN keys only — `'__proto__' in STATUS_COLORS` is true for every object + // literal, so `in` answered yes for a value the palette has never heard of + // and every caller then took the canonical branch. See `statusColor`. + return Object.hasOwn(STATUS_COLORS, norm(value)); } -/** "in_progress" → "In Progress". Shared by FieldEditor/ItemCard/chips. */ -export function formatFieldLabel(value: string): string { +/** + * "in_progress" → "In Progress". Shared by FieldEditor/ItemCard/chips. + * + * '' for a non-string, for `norm`'s reason above — and note this one had NO + * guard at all, so it threw on `null` and `undefined` as readily as on an + * array. Callers that pass `fields[key] ?? ''` were compensating for that by + * hand, one call site at a time. + */ +export function formatFieldLabel(value: unknown): string { + if (typeof value !== 'string') return ''; return value.replace(/_/g, ' ').replace(/\b\w/g, (c) => c.toUpperCase()); } @@ -122,11 +162,36 @@ const COLOR_TO_COLUMN_CLASS: Record = { export function columnAccentClassFor( field: { terminal_options?: string[] } | undefined, - value: string + value: unknown ): string { if (hasCanonicalStatus(value)) { return COLOR_TO_COLUMN_CLASS[statusColor(value)] ?? ''; } - if (value && field?.terminal_options?.includes(value)) return 'col-done'; + if (typeof value === 'string' && value && field?.terminal_options?.includes(value)) return 'col-done'; return ''; } + +/** + * The canonical-palette colour for a raw field value, or null when the value + * is not one this palette knows — in which case the caller renders it as plain + * text rather than as a chip. + * + * ONE copy of a question that had TWO (BUG-3041, CONVE-35): `TableView`'s + * `selectValueColor` and `FieldEditor`'s `getStatusColor` were + * character-for-character the same function, each carrying its own unguarded + * `val?.toLowerCase()`. `norm` below is a third unguarded lowercase but NOT a + * third copy of this composite — a distinction the first write-up of this + * change got wrong, and the enumeration round corrected. + */ +export function canonicalValueColor(value: unknown): string | null { + if (hasCanonicalStatus(value)) return statusColor(value); + switch (norm(value)) { + case 'critical': + case 'high': + case 'medium': + case 'low': + return priorityColor(value); + default: + return null; + } +}