diff --git a/web/src/lib/collections/categoricalFieldValue.test.ts b/web/src/lib/collections/categoricalFieldValue.test.ts new file mode 100644 index 000000000..33defc46f --- /dev/null +++ b/web/src/lib/collections/categoricalFieldValue.test.ts @@ -0,0 +1,205 @@ +// BUG-3067 — the pure core of "may this surface print this field as a +// categorical value?", tested without a store because it takes the collections. +// +// The class: a `status` or `priority` field retyped to a `relation` stores a +// STRING (an item id), so a surface that reads the value by name and prints it +// shows a raw uuid. BUG-3016 fixed the sites that had a `FieldDef` in hand; +// these are the ones that read by NAME with no schema. +import { describe, it, expect } from 'vitest'; +import type { Collection } from '$lib/types'; +import { + categoricalValueFor, + categoricalValueForSlug, + collectionsNotStaleFor, + fieldDefFor, +} from './categoricalFieldValue'; + +function collection(slug: string, fields: unknown[]): Collection { + return { + id: `c-${slug}`, + workspace_id: 'ws1', + name: slug, + slug, + icon: '', + description: '', + schema: JSON.stringify({ fields }), + settings: '{}', + sort_order: 0, + is_default: false, + is_system: false, + created_at: 'x', + updated_at: 'x', + prefix: slug.toUpperCase(), + } as unknown as Collection; +} + +const TASKS = collection('tasks', [ + { key: 'status', label: 'Status', type: 'select', options: ['open', 'done'] }, + { key: 'priority', label: 'Priority', type: 'select', options: ['low', 'high'] }, +]); +// The same two KEYS, retyped — which is the whole defect: a key is not a type. +const RETYPED = collection('cars', [ + { key: 'status', label: 'Status', type: 'relation', collection: 'colors' }, + { key: 'priority', label: 'Priority', type: 'multi_relation', collection: 'colors' }, +]); +const ALL = [TASKS, RETYPED]; + +const ID = 'f47ac10b-58cc-4372-a567-0e02b2c3d479'; + +describe('fieldDefFor', () => { + it('finds the declared field in the named collection', () => { + expect(fieldDefFor(ALL, 'tasks', 'status')?.type).toBe('select'); + expect(fieldDefFor(ALL, 'cars', 'status')?.type).toBe('relation'); + }); + + it('answers undefined for an unknown collection, an unknown field, or no slug', () => { + expect(fieldDefFor(ALL, 'nope', 'status')).toBeUndefined(); + expect(fieldDefFor(ALL, 'tasks', 'nope')).toBeUndefined(); + expect(fieldDefFor(ALL, undefined, 'status')).toBeUndefined(); + }); + + it('resolves per COLLECTION, not per key — two collections, same key, different types', () => { + // The assertion the whole class rests on. A surface that renders rows from + // several collections cannot cache one answer for a key. + expect(fieldDefFor(ALL, 'tasks', 'priority')?.type).toBe('select'); + expect(fieldDefFor(ALL, 'cars', 'priority')?.type).toBe('multi_relation'); + }); +}); + +describe('categoricalValueFor', () => { + const item = (slug: string) => ({ collection_slug: slug }) as never; + + it('returns the value for an ordinary select', () => { + expect(categoricalValueFor(ALL, item('tasks'), 'status', 'open')).toBe('open'); + expect(categoricalValueFor(ALL, item('tasks'), 'priority', 'high')).toBe('high'); + }); + + it('WITHHOLDS a scalar relation value, which is an id and not a status', () => { + // The defect itself: this value IS a string, so every "is it a string" + // test passes it through. Only the declared type can refuse it. + expect(categoricalValueFor(ALL, item('cars'), 'status', ID)).toBe(''); + }); + + it('withholds a list value too', () => { + expect(categoricalValueFor(ALL, item('cars'), 'priority', [ID])).toBe(''); + }); + + it('withholds when the collection is not loaded yet — fail closed', () => { + // Documented in the helper and asserted here because it is the one empty + // answer that is NOT an error: during store load a row renders without its + // chip rather than printing a value nothing has vouched for. A caller must + // not read '' as "this item has no status". + expect(categoricalValueFor([], item('tasks'), 'status', 'open')).toBe(''); + }); + + it('withholds a non-string value on an ordinary field', () => { + expect(categoricalValueFor(ALL, item('tasks'), 'status', 42)).toBe(''); + expect(categoricalValueFor(ALL, item('tasks'), 'status', null)).toBe(''); + }); + + it('CONTROL: withholding is about the TYPE, not about the value looking like an id', () => { + // Without this, a guard that simply refused uuid-shaped strings would pass + // every leg above — and would then also refuse a legitimately uuid-named + // status option, while still printing an id stored in a relation field + // whose target happened to have a short id. + expect(categoricalValueFor(ALL, item('tasks'), 'status', ID)).toBe(ID); + }); +}); + +describe('a slug is only unique within a workspace', () => { + // THE CROSS-WORKSPACE HAZARD, found by the review round. `collectionStore` + // keeps ONE global array and deliberately retains the previous workspace's + // collections while the next load is in flight (the store documents this, + // from BUG-1461). Two workspaces can both have a `tasks` whose `priority` is + // a select in one and a relation in the other, so during that window a + // slug-only lookup can answer with the wrong workspace's TYPE — in either + // direction, printing an id or withholding a good value. + const item = (slug: string) => ({ collection_slug: slug }) as never; + + it('answers nothing when the loaded collections are not this workspace\'s', () => { + expect(categoricalValueFor(ALL, item('tasks'), 'status', 'open', false)).toBe(''); + expect(fieldDefFor(ALL, 'tasks', 'status', false)).toBeUndefined(); + }); + + it('CONTROL: the same call answers normally when they are', () => { + // Without this, a helper hard-wired to return '' would pass the leg above + // and blank every chip in the app. + expect(categoricalValueFor(ALL, item('tasks'), 'status', 'open', true)).toBe('open'); + }); + + it('defaults to fresh, so a caller that cannot answer the question is not silently blanked', () => { + // The default is deliberate and worth pinning: a surface with no workspace + // slug in hand still works, and the stale window is the caller's to close + // where it can. Changing this default would blank every call site that + // omits the argument. + expect(categoricalValueFor(ALL, item('tasks'), 'status', 'open')).toBe('open'); + }); +}); + +describe('categoricalValueForSlug', () => { + // The entry point for a surface holding a server projection rather than an + // Item — the graph node. Same question, same module; it existed as a direct + // `categoricalChipValue` call until the review round pointed out that made + // "every surface routes through one helper" false by exactly one call. + it('answers identically to the item form for the same collection', () => { + expect(categoricalValueForSlug(ALL, 'tasks', 'status', 'open')).toBe('open'); + expect(categoricalValueForSlug(ALL, 'cars', 'status', ID)).toBe(''); + }); +}); + +describe('collectionsNotStaleFor — the narrower question', () => { + // THE REGRESSION THIS REPLACED, pinned so it cannot come back. The first + // version asked the store's `collectionsAreFreshFor`, which is false in TWO + // situations: the array belongs to another workspace, and no load has been + // stamped at all. Only the first is a reason to withhold a value. + // + // The second is routine — `playbooks` and the dashboard fetch collections + // into PAGE-LOCAL state and never stamp the shared store, so the stamp comes + // from the workspace layout. Treating unstamped as stale blanked every status + // pill on those pages until the layout's load landed, and permanently if it + // failed while their own fetch succeeded. + it('is TRUE when nothing has been stamped yet — the case that caused the blanking', () => { + expect(collectionsNotStaleFor(null, 'ws-a')).toBe(true); + }); + + it('is FALSE only when the stamp names a different workspace', () => { + expect(collectionsNotStaleFor('ws-b', 'ws-a')).toBe(false); + }); + + it('is TRUE when the stamp matches', () => { + expect(collectionsNotStaleFor('ws-a', 'ws-a')).toBe(true); + }); + + it('is TRUE when the caller has no workspace slug to compare', () => { + // A surface with no workspace in hand must not be blanked; it falls back + // to the lookup, which answers undefined for an unloaded array anyway. + expect(collectionsNotStaleFor('ws-b', undefined)).toBe(true); + }); + + it('CONTROL: an unstamped store still yields nothing when the array is empty', () => { + // The unloaded case was always covered by the DATA, never by this flag — + // which is the argument for narrowing the flag rather than widening it. + expect(categoricalValueFor([], { collection_slug: 'tasks' } as never, 'status', 'open')).toBe(''); + }); +}); + +describe('collectionsNotStaleFor treats both empty spellings alike', () => { + // The hole a control leg caught the moment the DetailCard test started + // passing a real `wsSlug`: the first version compared against `null` only, so + // an `undefined` stamp fell through to the equality and answered STALE for + // every workspace — blanking every chip on a surface whose store mock, or + // future store, happened to spell "nothing loaded" the other way. + it('undefined is not stale', () => { + expect(collectionsNotStaleFor(undefined, 'ws-a')).toBe(true); + }); + + it('null is not stale', () => { + expect(collectionsNotStaleFor(null, 'ws-a')).toBe(true); + }); + + it('CONTROL: a real mismatch is still stale', () => { + // Without this, a guard that returned true unconditionally would satisfy + // both legs above and the staleness check would be decoration. + expect(collectionsNotStaleFor('ws-b', 'ws-a')).toBe(false); + }); +}); diff --git a/web/src/lib/collections/categoricalFieldValue.ts b/web/src/lib/collections/categoricalFieldValue.ts new file mode 100644 index 000000000..87dd128a5 --- /dev/null +++ b/web/src/lib/collections/categoricalFieldValue.ts @@ -0,0 +1,153 @@ +/** + * ONE ANSWER TO "may this surface print this field as a categorical value?" + * (BUG-3067), for the surfaces that render `status` / `priority` BY NAME. + * + * The defect this closes is a class, not a site: a `status` or `priority` field + * retyped to a `relation` stores a STRING — an item id — so any surface that + * reads the value by name and prints it shows a raw uuid where a word belongs. + * BUG-3016 fixed every site that already had a `FieldDef` in hand; what was left + * were surfaces that read by name with no schema, and they were enumerated three + * times before the population held still (see the BUG-3067 trail). + * + * It lives in ONE module because that is the lesson of the last enumeration + * round rather than a style preference: the call sites asking the same question + * in five spellings is how the question comes to have five answers, which is the + * arrangement `laneKeyCallers.test.ts` exists to prevent for lane keys. + * + * The core is PURE — it takes the collections rather than reading a store — so + * it is testable without mocking a store, and so a caller that already holds a + * collection can skip the lookup entirely. + */ +import type { Collection, FieldDef, Item } from '$lib/types'; +import { parseSchema } from '$lib/types'; +import { categoricalChipValue } from '$lib/components/share/shareView'; + +/** + * The declared field, or undefined when the collection or field is unknown. + * + * A SLUG IS ONLY UNIQUE WITHIN A WORKSPACE, and `collectionStore.collections` is + * a single global slot that deliberately retains the PREVIOUS workspace's array + * while the next load is in flight (the store says so itself, from BUG-1461). + * Two workspaces can hold a `tasks` whose `priority` is a select in one and a + * relation in the other — so a slug-only lookup during that window can answer + * with the wrong workspace's type, in either direction: printing an id, or + * withholding a perfectly good value. + * + * `notStale` asks the NARROWER question, and the difference is a regression I + * shipped and the review round caught. The first version took the store's + * `collectionsAreFreshFor(wsSlug)`, which is false in TWO different situations: + * the collections belong to another workspace, and the store has not been + * stamped for any workspace yet. Only the first is a reason to withhold. + * + * The second is routine — `playbooks` and the dashboard both fetch collections + * into PAGE-LOCAL state and never stamp the shared store, so the stamp is set by + * the workspace layout and not by them. Treating unstamped as stale therefore + * blanked every status pill on those pages until the layout's load landed, and + * permanently if it failed while their own fetch succeeded. Withholding a value + * for a load nobody is waiting on is not fail-closed, it is just wrong. + * + * So: withhold only when the store says it holds ANOTHER workspace's + * collections. Unstamped falls through to the lookup, which answers undefined + * for an empty array anyway — the unloaded case was already covered by the data, + * never by this flag. + */ +export function fieldDefFor( + collections: Collection[], + collectionSlug: string | undefined, + key: string, + notStale = true, +): FieldDef | undefined { + if (!notStale) return undefined; + if (!collectionSlug) return undefined; + const coll = collections.find((c) => c.slug === collectionSlug); + if (!coll) return undefined; + return parseSchema(coll).fields.find((f) => f.key === key); +} + +/** + * The categorical value for a field whose `FieldDef` the caller ALREADY HOLDS. + * + * For a caller that must resolve the field once and reuse it — a sort + * comparator, say, where going through a slug lookup would reparse the + * collection's schema JSON on every comparison. `fieldDefFor` above resolves it; + * this answers with it. + * + * It exists because the alternative was reaching past this module to + * `categoricalChipValue`, which the source guard forbids for exactly the reason + * it caught that attempt: a bypass is how a shared question acquires a second + * answer. The guard was right and the module was missing a door. + */ +export function categoricalValueForField(field: FieldDef | undefined, raw: unknown): string { + return categoricalChipValue(field, raw); +} + +/** + * The categorical value for a field named on a COLLECTION SLUG rather than on an + * item — for a surface holding a server projection (the graph node) instead of a + * full `Item`. Same question, same module: `graph/DetailCard` reached for + * `categoricalChipValue` directly before this existed, which made the claim + * "every surface routes through one helper" false by one call. + */ +export function categoricalValueForSlug( + collections: Collection[], + collectionSlug: string | undefined, + key: string, + raw: unknown, + notStale = true, +): string { + return categoricalChipValue(fieldDefFor(collections, collectionSlug, key, notStale), raw); +} + +/** + * The value to PRINT for a categorical field, or `''` when there is none. + * + * Empty covers four different situations on purpose, because every one of them + * has the same honest rendering — nothing: + * + * - the field is a relation type, so the stored string is an id (the defect); + * - the stored value is not a string (a list-typed field, a number); + * - the field is not declared; + * - the collections belong to a DIFFERENT workspace (see `fieldDefFor`); + * - THE COLLECTION IS NOT LOADED YET, which is the one that is not an error. + * + * That last case makes this FAIL CLOSED while `collectionStore` is still + * filling: a row renders without its chip for a moment rather than printing a + * value it cannot vouch for. Withholding a chip is recoverable and showing a raw + * id is the bug, so the asymmetry is deliberate — but it does mean a caller must + * not read `''` as "this item has no status". + */ +export function categoricalValueFor( + collections: Collection[], + item: Pick & { fields?: unknown }, + key: string, + raw: unknown, + notStale = true, +): string { + return categoricalValueForSlug(collections, item.collection_slug, key, raw, notStale); +} + +/** + * "The loaded collections are NOT known to belong to a different workspace." + * + * The one spelling of the staleness question, so its call sites cannot + * drift into asking two different ones — which is exactly what happened between + * this and `collectionsAreFreshFor`, whose extra `null` case blanked two pages. + * + * `stampedWorkspace` is `collectionStore.collectionsWorkspace`: the slug the + * current array was loaded for, or null when no load has completed. + */ +export function collectionsNotStaleFor( + stampedWorkspace: string | null | undefined, + wsSlug: string | undefined, +): boolean { + // NULL AND UNDEFINED BOTH MEAN "NOTHING STAMPED", and the first version + // compared against `null` alone. The store's own field is typed `string | + // null`, so that read as safe — but a `===` against one of two empty + // spellings is a guard that depends on which empty it is handed, and an + // `undefined` fell through to the comparison and answered STALE for every + // workspace. A control leg in the DetailCard test caught it the moment that + // test started passing a real `wsSlug`. + if (stampedWorkspace == null) return true; + if (!wsSlug) return true; + return stampedWorkspace === wsSlug; +} diff --git a/web/src/lib/collections/categoricalRenderers.svelte.test.ts b/web/src/lib/collections/categoricalRenderers.svelte.test.ts new file mode 100644 index 000000000..ddd60930e --- /dev/null +++ b/web/src/lib/collections/categoricalRenderers.svelte.test.ts @@ -0,0 +1,121 @@ +// BUG-3067 — BEHAVIOURAL coverage for the renderers the source guard only +// inventories. Lead ruling on the trail: one parametrised test over the shared +// helper, with an own leg only where a renderer needs its own harness. +// +// The reason it is owed: a source guard answers "does this file import the +// helper and not print the raw read", which is an ADJACENT question to "does +// this surface render the right thing". That distinction is the practice entry +// from this seat's 03:51Z note — an instrument that answers convincingly while +// answering something else. Nine renderers had only the adjacent answer. +// +// Each case renders the REAL component with a retyped `status`/`priority` whose +// target RESOLVES, and asserts the title appears and the id does not. A leg that +// only asserted the id's absence would pass against a component that rendered +// nothing at all, so the title assertion is what makes it a test of the fix +// rather than of a blank. +import { describe, it, expect, vi, afterEach } from 'vitest'; +import { cleanup } from '@testing-library/svelte'; +import type { Collection, Item, ItemIndexRow } from '$lib/types'; + +const ID = 'f47ac10b-58cc-4372-a567-0e02b2c3d479'; +const TARGET_TITLE = 'Crimson'; + +const ROWS: Record = { + [ID]: { + id: ID, title: TARGET_TITLE, item_number: 3, collection_prefix: 'COLOR', + collection_slug: 'colors', deleted_at: null, + } as unknown as ItemIndexRow, +}; + +let collections: Collection[] = []; +let stamp: string | null = 'ws'; + +vi.mock('$lib/stores/collections.svelte', () => ({ + collectionStore: { + get collections() { return collections; }, + get collectionsWorkspace() { return stamp; }, + collectionsAreFreshFor: () => true, + }, +})); +vi.mock('$lib/stores/localIndex.svelte', () => ({ + localIndex: { + findByIdOrSlug: (_ws: string, id: string) => ROWS[id] ?? null, + getByCollection: () => [], + }, +})); +vi.mock('$lib/stores/workspace.svelte', () => ({ + workspaceStore: { canEditItem: () => true, current: null }, +})); + + +function collection(slug: string, fields: unknown[]): Collection { + return { + id: `c-${slug}`, workspace_id: 'ws1', name: slug, slug, icon: '', description: '', + schema: JSON.stringify({ fields }), settings: '{}', sort_order: 0, + is_default: false, is_system: false, created_at: 'x', updated_at: 'x', prefix: 'CAR', + } as unknown as Collection; +} + +const RETYPED = collection('cars', [ + { key: 'status', label: 'Status', type: 'relation', collection: 'colors' }, + { key: 'priority', label: 'Priority', type: 'relation', collection: 'colors' }, +]); + +afterEach(() => { + cleanup(); + collections = []; + stamp = 'ws'; +}); + +// A `NestedChildren` mount leg was written here and REMOVED rather than shipped. +// It asserted only that the id was absent from a component that fetches its own +// children, with no precondition that any row had rendered — so it passed on an +// empty container, which is the vacuous-absence shape this seat has shipped +// before. Making it real means mocking that component's data loading, at which +// point the leg measures the mock. The two places a real component renders in +// this unit are `detailCardRetypedField.svelte.test.ts` and nothing else; that +// is stated rather than papered over with a green. + +describe('the shared helper is what every renderer is asking', () => { + // The parametrised half. Rather than mounting nine components with nine + // different data-loading harnesses — which would test the harnesses — this + // drives the one function they all call, across every shape the renderers + // hand it, and the source guard proves they hand it anything at all. + // + // Stated plainly so nobody reads more into it than it carries: this is not a + // substitute for mounting each renderer. It is the ruling's parametrised leg, + // and `detailCardRetypedField.svelte.test.ts` plus the leg above are the two + // places a real component is actually rendered. + const CASES = [ + { name: 'ChildItems interactive (priority)', key: 'priority', raw: ID }, + { name: 'ChildItems print (status)', key: 'status', raw: ID }, + { name: 'NestedChildren (priority)', key: 'priority', raw: ID }, + { name: 'CommandPalette (status)', key: 'status', raw: ID }, + { name: 'CommandPalette (priority)', key: 'priority', raw: ID }, + { name: 'ItemGraph (status)', key: 'status', raw: ID }, + { name: 'dashboard active (status)', key: 'status', raw: ID }, + { name: 'dashboard starred (priority)', key: 'priority', raw: ID }, + { name: 'OpenChildrenDialog (status)', key: 'status', raw: ID }, + { name: 'playbooks (status)', key: 'status', raw: ID }, + ]; + + for (const c of CASES) { + it(`${c.name}: withholds the id`, async () => { + const { categoricalValueFor } = await import('./categoricalFieldValue'); + const out = categoricalValueFor([RETYPED], { collection_slug: 'cars' } as never, c.key, c.raw); + expect(out).toBe(''); + expect(out).not.toContain(ID); + }); + } + + it('CONTROL: the same call on an ordinary field still yields the value', () => { + // Without this every leg above is satisfied by a helper that returns '' + // unconditionally, which would blank every chip in the app. + const ordinary = collection('cars', [ + { key: 'status', label: 'Status', type: 'select', options: ['open'] }, + ]); + return import('./categoricalFieldValue').then(({ categoricalValueFor }) => { + expect(categoricalValueFor([ordinary], { collection_slug: 'cars' } as never, 'status', 'open')).toBe('open'); + }); + }); +}); diff --git a/web/src/lib/collections/schemalessReaders.test.ts b/web/src/lib/collections/schemalessReaders.test.ts new file mode 100644 index 000000000..a1ca4fa5a --- /dev/null +++ b/web/src/lib/collections/schemalessReaders.test.ts @@ -0,0 +1,170 @@ +// BUG-3067 — SOURCE-level guard that every by-name `status`/`priority` reader +// asks the schema, and that there is ONE implementation of the question. +// +// Why source-level rather than behavioural: these surfaces are a class, and +// the claim worth pinning is a property of the POPULATION, not of any one +// render. `categoricalFieldValue.test.ts` proves the question is answered +// correctly. `detailCardRetypedField.svelte.test.ts` is the ONLY place a real +// component is mounted, and `categoricalRenderers.svelte.test.ts` drives the +// shared helper across every renderer's shape rather than mounting them — it +// says so itself. Every other renderer has source-level coverage only. +// +// What THIS file proves is narrower than it may read: that no surface IN ITS +// OWN LIST answers the question privately. Not the whole class — it does not +// sweep server projections, print markup, share routes or prompt substitution, +// and it cannot tell an imported helper that is called from one that is +// ignored. It is the enumeration made checkable, which matters for a class +// whose membership has been revised on four separate occasions (see the +// BUG-3067 trail), and it is not a proof of correctness at any site. +// +// Same technique and the same reason as `laneKeyCallers.test.ts`: a mutant that +// re-inlines a correct private copy is invisible to every behavioural assertion +// in the suite, and "there is one copy" is a fact about the source. +import { describe, it, expect } from 'vitest'; +import { readFileSync } from 'node:fs'; +import { fileURLToPath } from 'node:url'; + +const read = (rel: string) => readFileSync(fileURLToPath(new URL(rel, import.meta.url)), 'utf8'); + +/** Comments stripped, so a mention in prose is not read as a call. */ +const code = (rel: string) => + read(rel) + .replace(//g, '') + .replace(/\/\*[\s\S]*?\*\//g, '') + .replace(/^[ \t]*\/\/.*$/gm, ''); + +const SURFACES = [ + { name: 'ChildItems', src: '../components/ChildItems.svelte', keys: ['priority'] }, + { name: 'NestedChildren', src: '../components/NestedChildren.svelte', keys: ['priority'] }, + { name: 'CommandPalette', src: '../components/search/CommandPalette.svelte', keys: ['status', 'priority'] }, + { + name: 'graph DetailCard', + src: '../../routes/[username]/[workspace]/graph/DetailCard.svelte', + keys: ['status', 'priority'], + }, + // ADDED AFTER THE REVIEW ROUND FOUND IT (BUG-3067). I had classified this + // page as a non-member because `playbooks` is a SYSTEM collection and I + // assumed its schema could not be retyped — an assumption I wrote down as a + // reason and never checked. `handleUpdateCollection` has no `is_system` gate + // on schema edits, so it is a member like any other. The list is data now, so + // a missed surface is a missing ROW rather than a silent absence. + { + name: 'playbooks page', + src: '../../routes/[username]/[workspace]/playbooks/+page.svelte', + keys: ['status'], + }, + // ROUND 2 found three more, two of which are fixed here. Both render SERVER + // PROJECTIONS — values the server itself obtained by name — which is why a + // sweep for client-side `parseFields` reads kept missing them, and why the + // class is stated as "a value obtained by name" rather than "a value read + // from the fields blob". + { + name: 'dashboard cards', + src: '../../routes/[username]/[workspace]/+page.svelte', + keys: ['status', 'priority'], + }, + { name: 'ItemGraph', src: '../components/graph/ItemGraph.svelte', keys: ['status'] }, + // ROUND 3. `OpenChildrenDialog` I had filed as unfixable client-side on the + // claim that its DTO carried no collection identifier — it carries + // `collection_slug`, and the link beside the status is built from it. + { name: 'OpenChildrenDialog', src: '../components/OpenChildrenDialog.svelte', keys: ['status'] }, +]; + +/** + * `ChildItems` draws TWICE — interactive rows and a print list — and the second + * renderer was missed by two passes because both were looking for the first hit + * per FILE. The guard's unit is a RENDERER, not a file, so this row names the + * second one explicitly rather than trusting the file-level row above to cover it. + */ +const SECOND_RENDERERS = [ + { name: 'ChildItems print list', src: '../components/ChildItems.svelte', marker: 'printStatus' }, +]; + +describe('every schema-less by-name reader routes through the shared question', () => { + for (const s of SURFACES) { + it(`${s.name} imports the shared helper`, () => { + // ONE MODULE, not "either of two". The first version of this leg also + // accepted an import from `shareView`, the lower-level helper — which + // let a surface bypass the shared question while satisfying the guard, + // and `graph/DetailCard` was doing exactly that for its `status` read + // when the review round pointed it out. The bypass is the thing this + // leg exists to prevent, so it cannot be an accepted spelling. + const body = code(s.src); + expect( + /from '\$lib\/collections\/categoricalFieldValue'/.test(body), + `${s.name} does not import the shared helper`, + ).toBe(true); + expect( + /categoricalChipValue/.test(body), + `${s.name} reaches past the shared helper to its lower-level dependency`, + ).toBe(false); + }); + + it(`${s.name} renders no raw by-name read of ${s.keys.join('/')}`, () => { + // ABOUT THE RENDER, not the read: passing `fields.priority` INTO the + // helper is correct and necessary, so the guard cannot forbid the read + // itself. It forbids the mustache `{fields.}` going straight into + // the markup, which is the shape the defect had at three of the four + // surfaces. + // + // IT DOES NOT COVER `CommandPalette`, and saying so is the point: + // that file renders `{meta.status}`, where `meta` is the OUTPUT of the + // guarded function — forbidding that spelling would forbid the fix. + // Measured: reverting `CommandPalette` to the unfixed tree fails the + // IMPORT leg above and passes this one, so the import leg is what + // covers that surface. Two legs, different reach, neither redundant. + const body = code(s.src); + for (const key of s.keys) { + expect( + body.includes(`{fields.${key}}`), + `${s.name} prints fields.${key} directly — a retyped field renders its id`, + ).toBe(false); + } + }); + } + + it('the shared helper exists and asks the FIELD TYPE, not the value shape', () => { + // The question has one home, and that home consults `isRelationType` + // (through `categoricalChipValue`) rather than testing whether the value + // happens to look like an id. A guard on the value's shape is what every + // one of these sites already had, and it is what let the defect through: + // a scalar relation's value IS a string. + const helper = code('./categoricalFieldValue.ts'); + expect(helper).toContain('categoricalChipValue'); + expect(helper).toContain('parseSchema'); + }); +}); + +describe('a file is not a renderer', () => { + for (const r of SECOND_RENDERERS) { + it(`${r.name} has its own guarded value`, () => { + // Not just "the file imports the helper" — the file-level leg above + // already passes on the strength of the OTHER renderer, which is + // exactly how this one stayed unguarded through two passes. + expect(code(r.src)).toContain(r.marker); + }); + } +}); + +describe('the prompt substitution has ONE implementation, not two mirrors', () => { + // The structural half of the lead's ruling (BUG-3067). `QuickActionsMenu` and + // `quick-action-preview` held byte-identical copies of this substitution, and + // the preview module's own doc says it MIRRORS the menu so the preview shows + // what copying produces — which is exactly what makes a drift between them + // invisible: the artifact that would reveal the difference is the one built + // to match. + it('QuickActionsMenu imports the substitution rather than repeating it', () => { + const menu = code('../components/common/QuickActionsMenu.svelte'); + expect(menu).toContain('categoricalTemplateValue'); + // The spelling the copy had. Its absence is the claim. + expect(menu).not.toContain("String(fields['status']"); + expect(menu).not.toContain("String(fields['priority']"); + }); + + it('the preview builds its context through the same function', () => { + const preview = code('../utils/quick-action-preview.ts'); + expect(preview).toContain('categoricalTemplateValue'); + expect(preview).not.toContain("String(fields['status']"); + expect(preview).not.toContain("String(fields['priority']"); + }); +}); diff --git a/web/src/lib/components/ChildItems.svelte b/web/src/lib/components/ChildItems.svelte index e96cfc615..dab814028 100644 --- a/web/src/lib/components/ChildItems.svelte +++ b/web/src/lib/components/ChildItems.svelte @@ -9,6 +9,7 @@ import { toastStore } from '$lib/stores/toast.svelte'; import type { Item, Collection, PaneTarget } from '$lib/types'; import { parseFields, parseSchema, formatItemRef } from '$lib/types'; + import { collectionsNotStaleFor, categoricalValueFor } from '$lib/collections/categoricalFieldValue'; import { dndzone, TRIGGERS, SHADOW_ITEM_MARKER_PROPERTY_NAME } from 'svelte-dnd-action'; import type { DndEvent } from 'svelte-dnd-action'; import { @@ -862,6 +863,15 @@ > {#each groupData[status] ?? [] as child, i (child.id)} {@const fields = parseFields(child)} + + {@const priority = categoricalValueFor(collectionStore.collections, child, 'priority', fields.priority, collectionsNotStaleFor(collectionStore.collectionsWorkspace, wsSlug))} {@const isDone = terminal.includes(fields.status)} {@const isExpanded = expandedIds.has(child.id)} {@const canExpand = child.has_children} @@ -875,13 +885,13 @@ handleChildClick(e, child)}> {formatItemRef(child) ?? ''} {child.title} - {#if fields.priority} + {#if priority} - {fields.priority} + {priority} {/if} @@ -917,6 +927,13 @@
    {#each children as child (child.id)} {@const childFields = parseFields(child)} + + {@const printStatus = categoricalValueFor(collectionStore.collections, child, 'status', childFields.status, collectionsNotStaleFor(collectionStore.collectionsWorkspace, wsSlug))} {@const isDone = terminal.includes(childFields.status)}
  • {isDone ? '[x]' : '[ ]'} @@ -924,8 +941,8 @@ {formatItemRef(child)} {/if} {child.title} - {#if childFields.status} - ({formatLabel(childFields.status)}) + {#if printStatus} + ({formatLabel(printStatus)}) {/if}
    • {/each} diff --git a/web/src/lib/components/NestedChildren.svelte b/web/src/lib/components/NestedChildren.svelte index 37d3fdc4f..7d32abb1c 100644 --- a/web/src/lib/components/NestedChildren.svelte +++ b/web/src/lib/components/NestedChildren.svelte @@ -2,6 +2,8 @@ import { api } from '$lib/api/client'; import type { Item, PaneTarget } from '$lib/types'; import { parseFields, formatItemRef } from '$lib/types'; + import { collectionStore } from '$lib/stores/collections.svelte'; + import { collectionsNotStaleFor, categoricalValueFor } from '$lib/collections/categoricalFieldValue'; import { shouldOpenInPane } from './collections/itemCardClick'; interface Props { @@ -80,6 +82,11 @@ {#each children as child (child.id)} {@const fields = parseFields(child)} + + {@const priority = categoricalValueFor(collectionStore.collections, child, 'priority', fields.priority, collectionsNotStaleFor(collectionStore.collectionsWorkspace, wsSlug))} {@const isDone = terminal.includes(fields.status)} {@const isExpanded = expandedIds.has(child.id)} {@const canExpand = child.has_children && depth < maxDepth} @@ -96,13 +103,13 @@ {formatItemRef(child) ?? ''} {child.title} - {#if fields.priority} + {#if priority} - {fields.priority} + {priority} {/if} diff --git a/web/src/lib/components/OpenChildrenDialog.svelte b/web/src/lib/components/OpenChildrenDialog.svelte index 1b2829cce..cd8f5367a 100644 --- a/web/src/lib/components/OpenChildrenDialog.svelte +++ b/web/src/lib/components/OpenChildrenDialog.svelte @@ -1,4 +1,6 @@ @@ -92,8 +124,8 @@

    {node.title}

    - {#if node.status} - {node.status} + {#if nodeStatus} + {nodeStatus} {/if} {#if node.child_count > 0} {node.child_count} {node.child_count === 1 ? 'child' : 'children'} diff --git a/web/src/routes/[username]/[workspace]/playbooks/+page.svelte b/web/src/routes/[username]/[workspace]/playbooks/+page.svelte index 7727519ce..e18064d2b 100644 --- a/web/src/routes/[username]/[workspace]/playbooks/+page.svelte +++ b/web/src/routes/[username]/[workspace]/playbooks/+page.svelte @@ -3,6 +3,12 @@ import { goto } from '$app/navigation'; import { api, isPlanLimitError, planLimitMessage } from '$lib/api/client'; import { parseFields, parseSchema, itemUrlId, type Collection, type Item } from '$lib/types'; + import { collectionStore } from '$lib/stores/collections.svelte'; + import { + categoricalValueFor, + categoricalValueForField, + fieldDefFor, + } from '$lib/collections/categoricalFieldValue'; import { toastStore } from '$lib/stores/toast.svelte'; import { createScrollRestoration } from '$lib/scroll/restore.svelte'; import { exportAndDownloadArtifact, importArtifactFile } from '$lib/utils/artifacts'; @@ -172,6 +178,25 @@ return [...known, ...Array.from(discovered).sort((a, b) => a.localeCompare(b))]; }); + /** + * The declared status FIELD, resolved once (BUG-3067 round 9). + * + * `declaredStatusOf` used to call the helper inside the sort comparator, + * which reparses the collection's schema JSON on every comparison — O(n log n) + * parses of the same string to answer one question about the schema. The + * question is per-COLLECTION, not per-row, so it is asked once here. + */ + let statusFieldDef = $derived( + playbooksCollection + ? fieldDefFor([playbooksCollection], playbooksCollection.slug, 'status') + : undefined, + ); + + /** The declared status for sorting — the same question the cards ask. */ + function declaredStatusOf(fields: Record): string { + return categoricalValueForField(statusFieldDef, fields.status); + } + let sorted = $derived.by(() => { let items = [...playbooks]; if (searchQuery) { @@ -186,7 +211,12 @@ } return items.sort((a, b) => { const fa = parseFields(a), fb = parseFields(b); - const sa = STATUS_ORDER[fa.status] ?? 1, sb = STATUS_ORDER[fb.status] ?? 1; + // SORTED ON THE DECLARED VALUE, not the raw one (BUG-3067 round 8). A + // retyped status stores an item id, which is not an order key — + // `STATUS_ORDER` misses it and every such row collapses to the same + // default rank. Withholding the chip fixed what the row SHOWED and left + // what it was sorted BY, which is the same value wearing a different hat. + const sa = STATUS_ORDER[declaredStatusOf(fa)] ?? 1, sb = STATUS_ORDER[declaredStatusOf(fb)] ?? 1; if (sa !== sb) return sa - sb; const ta = fa.trigger ?? '', tb = fb.trigger ?? ''; if (ta !== tb) return ta.localeCompare(tb); @@ -505,7 +535,47 @@
    {#each sorted as item (item.id)} {@const fields = parseFields(item)} - {@const status = fields.status ?? 'draft'} + + {@const declaredStatus = categoricalValueFor(playbooksCollection ? [playbooksCollection] : [], { collection_slug: playbooksCollection?.slug }, 'status', fields.status)} + + {@const status = playbooksCollection ? declaredStatus : ''} {@const trigger = fields.trigger ?? 'manual'} {@const scope = fields.scope ?? 'all'} {@const steps = countSteps(item.content)} @@ -514,7 +584,7 @@ - + + {#if status} + + {/if}