mirror of
https://github.com/PerpetualSoftware/pad.git
synced 2026-10-25 22:10:25 +00:00
fix(web): a field value that is not a string no longer takes the view down (BUG-3041) (#1345)
* fix(web): a field value that is not a string no longer takes the view down
BUG-3041. `norm()` in `fieldColors.ts` did `value?.toLowerCase()`, and optional
chaining guards null and undefined — not a wrong TYPE. Hand it an array and
rendering threw `value?.toLowerCase is not a function`, taking the whole board
or table down rather than one chip.
It is reachable because a field's DECLARED type is not a promise about what is
STORED. `status` and `priority` are conventional key names, not reserved ones;
the schema editor will type any key however it likes; and nothing rewrites the
values already in the blob when a type changes. So a `status` that used to be a
`multi_select` can hold an array while its schema says `multi_relation` — the
same premise round 6 of TASK-2999 had to correct about `options`.
THE CLASS, grepped at the first instance rather than patched at the named one
(CONVE-18/CONVE-35). Three copies of ONE question: `fieldColors.norm`,
`TableView.selectValueColor` and `FieldEditor.getStatusColor` were the same
"canonical value to colour" function written out three times, each carrying its
own unguarded `.toLowerCase()`. Two of the three were reachable with a
non-string, and hardening them one at a time is how the third stays broken.
- The five exported helpers now take `unknown` and are total over it.
`formatFieldLabel` had NO guard at all, so it threw on null as readily as on
an array, and callers were compensating by hand with `?? ''` one site at a
time. Typed `unknown` rather than `string` deliberately: a `string` parameter
the compiler cannot enforce is a claim, not a check — every caller reads from
a `Record<string, any>` — and that claim is what hid this until it happened.
- `canonicalValueColor` is the single copy; TableView and FieldEditor call it.
THE CHIPS ASK THE VALUE'S SHAPE, NOT THE FIELD'S TYPE. `ItemCard` and
`TableView` gated their status/priority chips on truthiness, and a non-empty
array is truthy. They now require a string. Asking about the value rather than
the type is the point: a type allow-list goes stale the next time a type is
added, and this question does not. Nor is it cosmetic on the table — that chip
is CLICKABLE, and cycling it wrote a scalar into a list field, which the server
refuses.
Instruments, 26 tests across three files, every leg shown red against the
unfixed code:
- `fieldColors.test.ts` drives the whole JSON input domain — array, empty
array, number, boolean, object, null, undefined — rather than the one
non-string the report happened to name, and asserts each reads as "no
canonical value" rather than merely not throwing. Controls: strings resolve
exactly as before, and an unknown STRING ('shipped') stays distinguishable
from an unreadable value.
- The board leg renders the reported configuration and asserts the card is
drawn with no chip; its control keeps the ordinary string chip.
- `TableView.listValue.svelte.test.ts` is a new render harness for the table's
own copy of the defect, including the clickable-chip half.
Four mutants, each killed by its own leg: the unguarded `norm` (10 legs), each
chip gate reverted (one leg each), and both reverted together — the true
pre-fix tree, which reproduces the filed error text exactly.
Gates, each exit code read from a redirect rather than a piped summary line:
`svelte-check` 0 / 0 errors · `npm run test` 0, 2651 passed / 185 files ·
`npm run build` 0 · `check:tiptap-pins` 0 · `audit:ci` 0 · `make lint` 0 / 0
issues · `make test` 0, 0 FAIL / 30 ok · gofmt 0 files. `make test-pg` not owed
— no SQL changed.
BUG-3041
Claude-Session: https://claude.ai/code/session_019EmDd3Wy8b3cPG1TZBSzBu
* fix(web): the enumeration round's in-class findings, including one defect and one regression of mine
ONE enumeration-prompted codex round on the frozen tip. Category (b) — did the
consolidation change behaviour — came back CLEAN, with the comparison written
out. Categories (a) and (c) returned a large pre-existing class that is filed,
not fixed here. What lands here is the in-class remainder, and three of the
items are about this unit's own work.
A DEFECT IN THE CODE I JUST WROTE. `norm(value) in STATUS_COLORS` answers TRUE
for `__proto__`, and `STATUS_COLORS['constructor']` is a FUNCTION — so
`hasCanonicalStatus` said yes to a status the palette has never heard of, and
`statusColor` handed its callers an object or a function where its signature
promises a colour string. `?? MUTED` cannot catch either, because both are
truthy, and no non-JSON input is needed: these are plain strings a user can
type into a text field. Both lookups now use `Object.hasOwn`.
A REGRESSION I INTRODUCED. The first version of the table's gate required a
string outright, which silently removed the missing-status setter: with no
status stored, that chip renders empty and CLICKING IT sets the first option,
which is how a row gets its first status. `{}` and `{status:''}` began
behaving differently for no reason a user could see. Absent is admitted
alongside a string again — the non-string case, which is the actual defect,
stays gated.
A CLAIM I OVERSTATED. The previous commit says "three copies of ONE question".
There were TWO copies of the composite resolver (TableView, FieldEditor);
`norm` is a third unguarded lowercase but not a third copy of that function.
The comment in `fieldColors.ts` now says so, and says the first write-up got
it wrong.
FOUR OF MY OWN TEST LEGS PROVED LESS THAN THEY CLAIMED, which is the part
worth reading:
- A table leg supplied an array to a branch that has ALWAYS required a string,
so it measured the fallback text and PASSED AGAINST THE ORIGINAL
IMPLEMENTATION — codex restored the old TableView and watched it stay green.
Replaced with a string fixture that reaches the resolver, asserting the
resolver's answer, with a control for a word in neither vocabulary.
- The palette controls compared the implementation against itself
(`canonicalValueColor('critical') === priorityColor('critical')`), so muting
every priority colour left all 48 tests green. The colours are now written
out as literals.
- Deleting every ItemCard priority chip left the suite green: the control
covered status alone. It now covers both.
- "No chip" asserted the absence of a CSS class, which an inert span would
satisfy. The leg now clicks every control in the row and asserts no write
reached the handler, with a control proving the same click DOES cycle a
valid status.
One leg was also honest about what it cannot prove: the two vocabularies are
disjoint, so no fixture built from them distinguishes "statuses first" from
"priorities first". The leg pins the observable consequence and says where the
ordering question will land if they ever overlap.
Mutation matrix re-run: the four mutants codex reported as SURVIVING — muted
priorities, deleted priority chip, bare `in` lookup, string-only table gate —
now each die on their own leg. Nine mutants total, all killed.
Gates, each exit code read from a redirect rather than a piped summary line:
`svelte-check` 0 / 0 errors · `npm run test` 0, 2669 passed / 185 files ·
`npm run build` 0 · `check:tiptap-pins` 0 · `audit:ci` 0 · `make lint` 0 / 0
issues · `make test` 0, 0 FAIL / 30 ok · gofmt 0 files. `make test-pg` not owed
— no SQL changed.
BUG-3041
Claude-Session: https://claude.ai/code/session_019EmDd3Wy8b3cPG1TZBSzBu
This commit is contained in:
@@ -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
|
||||
|
||||
@@ -226,7 +226,18 @@
|
||||
</div>
|
||||
|
||||
<div class="card-meta">
|
||||
{#if statusField && fields.status}
|
||||
<!--
|
||||
STRING-SHAPED, not merely truthy (BUG-3041). These chips render a value
|
||||
straight out of the item's `fields` blob, and a field's declared type is
|
||||
not a promise about what is stored: retyping `status` to a list or
|
||||
number type in the schema editor leaves the old values in place, and a
|
||||
non-empty array is truthy. `statusColor`/`formatLabel` no longer throw on
|
||||
one, but an empty chip describes nothing — so the card shows no chip at
|
||||
all, exactly as it does for an absent value. Asking about the VALUE's
|
||||
shape rather than the FIELD's type is deliberate: a type allow-list goes
|
||||
stale the next time a type is added, and this question does not.
|
||||
-->
|
||||
{#if statusField && typeof fields.status === 'string' && fields.status}
|
||||
{#if statusCyclable}
|
||||
<Chip
|
||||
size="sm"
|
||||
@@ -243,7 +254,7 @@
|
||||
</Chip>
|
||||
{/if}
|
||||
{/if}
|
||||
{#if priorityField && fields.priority}
|
||||
{#if priorityField && typeof fields.priority === 'string' && fields.priority}
|
||||
<Chip size="sm" color={priorityColor(fields.priority)}>
|
||||
{formatLabel(fields.priority)}
|
||||
</Chip>
|
||||
|
||||
@@ -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<string, unknown>): 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');
|
||||
});
|
||||
});
|
||||
@@ -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 @@
|
||||
</div>
|
||||
{#each visibleFields as field (field.key)}
|
||||
<div class="table-cell" role="cell">
|
||||
{#if field.key === 'status' && field.options && onStatusChange}
|
||||
<!--
|
||||
SHAPE, not truthiness (BUG-3041): `field.options` survives a
|
||||
retype in the schema editor, so a `status` that is now a
|
||||
`multi_relation` still takes this arm, with an ARRAY as its
|
||||
value. The chip used to render it and the row threw. A
|
||||
non-string falls through to the plain-text arm, which is
|
||||
honest about holding something this chip cannot describe.
|
||||
|
||||
ABSENT is admitted alongside a string, and that is not
|
||||
sloppiness — it is the affordance this arm exists for. With
|
||||
no status stored, the chip renders empty and CLICKING IT sets
|
||||
the first option, which is how a row gets its first status.
|
||||
The first version of this gate required a string outright and
|
||||
silently removed that setter, making `{}` and `{status:''}`
|
||||
behave differently for no reason a user could see (found by
|
||||
the enumeration round, which reproduced the difference).
|
||||
-->
|
||||
{#if field.key === 'status' && field.options && onStatusChange && (fields[field.key] == null || typeof fields[field.key] === 'string')}
|
||||
<Chip
|
||||
size="sm"
|
||||
color={statusColor(fields[field.key] ?? '')}
|
||||
@@ -281,7 +287,7 @@
|
||||
{formatLabel(fields[field.key] ?? '')}
|
||||
</Chip>
|
||||
{: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}
|
||||
<Chip size="sm" color={chipColor}>{formatLabel(fields[field.key])}</Chip>
|
||||
{:else}
|
||||
|
||||
@@ -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;
|
||||
|
||||
@@ -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('');
|
||||
});
|
||||
}
|
||||
});
|
||||
@@ -65,19 +65,48 @@ const STATUS_COLORS: Record<string, string> = {
|
||||
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<string, any>` — 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<string, string> = {
|
||||
|
||||
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;
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user