From a03c6d60bd248be18b4e863e085e693b9caaa8b4 Mon Sep 17 00:00:00 2001 From: xarmian Date: Mon, 14 Sep 2026 03:15:47 -0400 Subject: [PATCH] fix(web): ten surfaces ask a field type before printing its value (BUG-3067) (#1358) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(web): four schema-less surfaces ask the field's type before printing it (BUG-3067) 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; these are the ones that read by name with no schema. THE POPULATION WAS WIDENED THREE TIMES, twice by review and once here, so this commit runs the instrument the item's own trail prescribes before touching anything: grep the two KEY NAMES across `web/src`, rather than grepping for field-rendering shapes. That found two sites nobody had listed (`QuickActionsMenu` and `quick-action-preview`, see below) and confirmed the four fixed here. The body's premise that these need PLUMBING turns out to be too pessimistic, and that is worth recording because it is what made the unit cheap: `collectionStore` is a singleton any component may import, and every one of these rows already carries its own `collection_slug`. `ChildItems` was even importing `collectionStore` and `parseSchema` already — for the collection PICKER, never for the row it was drawing. No prop threading was required anywhere. One shared helper (`$lib/collections/categoricalFieldValue`) rather than four guards, for the reason `laneKeyCallers.test.ts` exists: four call sites asking one question in four spellings is how the question comes to have four answers, which is this class's documented history. Its core is pure — it takes the collections rather than reading the store — so it tests without a store mock. An unloaded store FAILS CLOSED: a row renders without its chip for a moment rather than printing a value nothing has vouched for. Withholding a chip is recoverable and printing an id is the bug. Asserted, and documented as the one empty answer that is not an error. `graph/DetailCard` gets both of its reads: `priority` from the item's fields and `status` from the graph NODE, which is a server projection carrying no schema — but it carries the collection slug, which is all the question needs, and it renders before the item fetch lands. Tests. The pure helper has its own; a source-level guard pins the CLASS (every surface routes through the shared question, none answers it privately), since a mutant that re-inlines a correct private copy is invisible to every behavioural assertion; and `DetailCard` is driven end to end because it reads both keys from two different places. Counterfactuals measured, not assumed: reverting `NestedChildren` fails two legs of the source guard, reverting `CommandPalette` fails one (its import leg — it renders `{meta.status}`, the guarded function's OUTPUT, so the mustache leg cannot cover it, and the test says so), and reverting `DetailCard` prints the raw uuid TWICE, which is the defect reproduced directly. NOT in this commit: `QuickActionsMenu.svelte` and `quick-action-preview.ts` hold the same pair of schema-less by-name reads, but they substitute into a PROMPT rather than into a chip, so "withhold it" may be the wrong remedy — and the two are one site with two implementations that must agree, since the preview exists to show what copying produces. Recorded on the trail for a decision rather than folded in silently. BUG-3067 Claude-Session: https://claude.ai/code/session_019EmDd3Wy8b3cPG1TZBSzBu * fix(web): a fifth surface, a workspace-scoping hole, and one helper instead of two (BUG-3067, round 1) Three findings, all from the adversarial round, all real. 1. A MEMBER I DISMISSED WITHOUT CHECKING. `playbooks/+page.svelte` reads `fields.status` by name, prints it through `statusLabel`, and keys its card styling off it. I had put it in the non-member bucket with the reason "system collection, a retype is not reachable the same way" — and wrote, in the same comment, that I would check rather than assume. I did not. `handleUpdateCollection` has NO `is_system` gate on schema edits, so the playbooks `status` can be retyped exactly like any user collection's. The surface list in the source guard is data now, so a missed member is a missing ROW someone can see rather than a silent absence. (`conventions/+page.svelte` really is a non-member, re-checked this round: it compares and WRITES `fields.status`, never renders it as a chip.) 2. A SLUG IS ONLY UNIQUE WITHIN A WORKSPACE. `collectionStore.collections` is one global array that DELIBERATELY retains the previous workspace's collections while the next load is in flight — the store documents this itself, from BUG-1461. Two workspaces can each 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. `fieldDefFor` and both value forms now take the store's own `collectionsAreFreshFor(wsSlug)` answer and fail closed when it is false — the same direction as an unloaded store. It DEFAULTS to fresh, pinned by its own leg: a surface with no workspace slug in hand must not be silently blanked, and the stale window is the caller's to close where it can. 3. "ONE SHARED HELPER" WAS LITERALLY FALSE. `graph/DetailCard` called `categoricalChipValue` — the lower-level dependency — directly for its `status` read, because that read starts from a graph NODE rather than an Item. Added `categoricalValueForSlug` so the projection case is served by the same module, and the source guard now REFUSES an import of the lower-level helper instead of accepting it as an alternative. The guard previously treated the bypass as a valid spelling, which is how it passed a file that was performing the bypass. BUG-3067 Claude-Session: https://claude.ai/code/session_019EmDd3Wy8b3cPG1TZBSzBu * fix(web): three more surfaces, and the freshness guard is now actually used (BUG-3067, round 2) Round 2 found that my enumeration was wrong a SECOND time, and that the previous commit described a guard nobody called. 1. "BOTH VALUE FORMS NOW TAKE THE STORE'S OWN collectionsAreFreshFor(wsSlug)" WAS FALSE IN PRODUCTION. The parameter existed, defaulted to true, and no call site passed it — so the cross-workspace hole was documented and tested and not actually closed anywhere. A guard nobody uses reads, in a commit message, exactly like a guard that works. It is passed now at every surface that can reach a workspace slug: `ChildItems` and `NestedChildren` from their `wsSlug` prop, `playbooks` and the dashboard from `page.params.workspace`. The two graph surfaces have no workspace slug in hand and are named below rather than quietly left. 2. THREE MORE MEMBERS, two fixed here. The dashboard's active AND starred cards (one block, twice on the page) and `graph/ItemGraph`'s selection panel all render SERVER PROJECTIONS — values the server itself obtained by name through `extractFieldValue` — so a sweep for client-side `parseFields` reads could not see them. That is the lesson rather than the fix: the class is "a value obtained BY NAME", and where the name was resolved does not matter. `OpenChildrenDialog` is the third and is NOT fixed: its DTO carries `ref`, `title` and `status` and no collection identifier at all, so the declared type is unanswerable client-side. It needs a server change, which is a different unit. 3. The `playbooks` lookup hardcodes the slug `'playbooks'` and therefore does not survive a collection rename. It inherits that from the page, which already loads its items and its collection by the same literal — so the page is already broken after a rename and this adds no new assumption. Said out loud rather than left for the next reader to discover. The source guard's surface list grew by the two fixed surfaces. It remains an import-and-mustache check: it cannot prove the imported helper is CALLED, only that the raw read is not rendered and the lower-level helper is not reached past. Stated here because the previous commit's phrasing implied more. BUG-3067 Claude-Session: https://claude.ai/code/session_019EmDd3Wy8b3cPG1TZBSzBu * fix(web): narrow the staleness question, and close three rows the table exposed (BUG-3067, round 3) Round 3 produced the POPULATION TABLE instead of another search, which is what CONVE-35 asks for once successive rounds keep returning new members of one class — at that point the rounds are measuring the enumeration, not the code. The table found a regression I shipped last round and three rows I had called covered or uncoverable. 1. MY ROUND-2 FRESHNESS GUARD BLANKED VALID CHIPS. It asked the store's `collectionsAreFreshFor(wsSlug)`, which is false in TWO situations: the loaded collections belong to another workspace, and no load has been stamped at all. Only the first is a reason to withhold. The second is routine rather than exceptional: `playbooks` and the dashboard both 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 that load failed while their own fetch succeeded. Withholding a value for a load nobody is waiting on is not fail-closed, it is just wrong. `collectionsNotStaleFor` asks only "does the stamp name a DIFFERENT workspace", with one spelling shared by all six call sites. The unloaded case was always covered by the data — an empty array has no field to find — which is the argument for narrowing the flag rather than widening it. 2. A FILE IS NOT A RENDERER. `ChildItems` draws twice: interactive rows (`priority`) and a print list (`status`). Two passes fixed the first and reported the file done, because both were looking for the first hit per file. The guard now carries a second-renderer row that the file-level leg cannot satisfy. 3. `OpenChildrenDialog` IS fixable client-side. I filed it as needing a server change, claiming its DTO carried no collection identifier. It carries `collection_slug` — the link rendered two lines from the status is built from it. A claim about a shape I had not opened, in an item whose entire history is enumeration claims that were not checked. 4. `ItemGraph` has a `workspace` prop, so my "the two graph surfaces have no workspace slug in hand" was false for one of them. It passes staleness now. `graph/DetailCard` genuinely has none and is named rather than left silent. Also corrected: the stale "five surfaces" phrasing, and the source guard's overstated reach — it proves the files and keys in its own list, and does not sweep server projections, print markup, share routes or prompt substitution. It says so now. Still uncovered, deliberately and by name: the individual public-share route (IDEA-3066, entangled with per-target redaction) and the `QuickActionsMenu` / `quick-action-preview` pair, which substitutes into a PROMPT rather than a chip — an empty substitution and a resolved title are both defensible and it is one site with two implementations that must agree. BUG-3067 Claude-Session: https://claude.ai/code/session_019EmDd3Wy8b3cPG1TZBSzBu * fix(web): prompt variables resolve to a title, and two pages use the schema they fetched (BUG-3067, round 4) The lead's ruling on the quick-action row, plus round 4's finding that my own staleness plumbing was asking about the wrong array. 1. THE PROMPT SUBSTITUTION, per the ruling on the trail. A `status` or `priority` retyped to a relation stores an item id, and both `QuickActionsMenu` and `quick-action-preview` pasted that id straight into a prompt the user hands to an agent. Chips answer this by WITHHOLDING; a prompt variable cannot, because `{status}` has to become something and a blank is a lie of a different kind. So, matching #1352: resolve to the target's TITLE, fall back to its REF when the row will not resolve, never emit the raw id, join a list with ', '. An unresolvable id yields empty only because nothing true is left to say, not by choice. The structural half matters as much: the two files held byte-identical copies, and the preview module's own doc says it MIRRORS the menu so the preview shows what copying produces — which is precisely what makes a drift between them invisible, since the artifact that would reveal it is the one built to match. One function now, in the preview module, imported by the menu, with a guard asserting neither file carries the old spelling. 2. TWO PAGES NOW USE THE SCHEMA THEY ALREADY FETCHED. Round 3 narrowed the staleness question; round 4 pointed out I was asking it about the wrong array. The dashboard fetches collections alongside its dashboard under a sequence guard, and `playbooks` fetches its collection under a documented stale-response guard — and both then resolved the field type from the SHARED store, whose stamp is set by the workspace layout and not by them. So a page whose own fetch had succeeded could still render blank because a different global had not loaded. Both resolve from their own state now. The staleness question does not arise there: the fetch is already workspace-guarded. The playbooks read also uses that object's OWN slug, so it survives a rename of the playbooks collection where the hardcoded literal would not — the rest of the page still addresses it by literal, so the page as a whole does not, and the comment says so rather than implying the rename is handled. Still open from round 4, named rather than silently left: `graph/DetailCard` and `OpenChildrenDialog` pass no staleness argument and have no workspace slug in hand, so they default to permissive; and nine of the ten guarded renderers have no behavioural coverage, only the source guard plus `DetailCard`'s. Corrected: "six call sites" was wrong — there are seven guarded invocations and two unguarded — and the source guard's comment claimed per-surface render tests prove two surfaces call the helper, when only `DetailCard` has such coverage. BUG-3067 Claude-Session: https://claude.ai/code/session_019EmDd3Wy8b3cPG1TZBSzBu * fix(web): three defects round 5 found, plus both of the lead's ruled rows (BUG-3067, round 5) 1. I IMPLEMENTED THE RULING'S FALLBACK AND THEN DESCRIBED IT WRONGLY. My comment and commit said the substitution falls back to the REF "when the index cannot resolve the row". It does not, and it cannot: a ref is built FROM the row (its collection prefix and item number), so an id that resolves to nothing yields no row and therefore no ref. The fallback is for a row that resolves but has NO TITLE, which is what the code does and what the test pins. An unresolvable id yields empty because nothing true is left to say — the one case the ruling's "never empty by choice" does not reach. Prose corrected rather than code changed; the code was right. 2. `EditCollectionModal` called `contextFromItem` WITHOUT a resolver, which I had defaulted to one that resolves nothing — so every relation-valued `{status}`/`{priority}` in that preview rendered empty, quietly disagreeing with the menu the preview exists to mirror. A default that silently produces the wrong answer for an existing caller is worse than a required argument; the caller passes the local-index resolver now. 3. "NO SCHEMA YET" IS NOT "DRAFT". The playbooks page's two loaders are independent — `loadPlaybooks` clears `loading` on its own — so cards render while `playbooksCollection` is still null. The helper answers '' for that, which fell through to the `'draft'` default and reported every card as Draft: a MISREPORT, not a missing chip, and permanent if the schema fetch fails. The raw stored value stands in while the schema is unknown, and the declared answer takes over the moment it lands. THE LEAD'S TWO RULED ROWS, both closed: - The permissive staleness default was allowed to stand only on a grep showing every mount site sits under the `[workspace]` layout. `DetailCard` has ONE mount site and it qualifies — and the slug is threaded anyway, because a prop is checkable where an argument about mount sites goes stale the first time someone mounts it elsewhere. `OpenChildrenDialog` FAILS that test: it is mounted twice in the ROOT layout, so it can be on screen while the store holds another workspace entirely. It takes the workspace from the route. - Behavioural coverage: a parametrised leg over the shared helper across all ten renderer shapes, plus the control that would catch a helper returning '' for everything. A `NestedChildren` mount leg was written and REMOVED rather than shipped — it asserted only the id's absence from a self-fetching component with no precondition that any row rendered, so it passed on an empty container. Making it real means mocking that component's loading, at which point it measures the mock. The file says so instead of carrying the green. BUG-3067 Claude-Session: https://claude.ai/code/session_019EmDd3Wy8b3cPG1TZBSzBu * fix(web): my own fallback printed the id it was written to hide (BUG-3067, round 6) 1. THE REGRESSION I PREDICTED AND SHIPPED ANYWAY. Round 5's playbooks fix stood the RAW stored value in while the schema was still null, reasoning that it is what the page showed before this unit. For a retyped status the raw value IS the id — so the fallback printed exactly what the unit exists to prevent, for the duration of the window it was meant to fix, and worse than the "Draft" misreport it replaced. I named this as the finding I most expected from the round, in the round's own prompt, and it was there. The honest answer for an unknown schema is to claim NOTHING: no status chip and no draft/deprecated styling until the schema lands. The card still renders with its title, trigger and steps. 2. A CONTROL LEG FOUND A REAL HOLE the moment the DetailCard test started passing its now-required `wsSlug`: `collectionsNotStaleFor` compared the stamp against `null` alone, so an `undefined` fell through to the equality and answered STALE for every workspace. The store's field is typed `string | null`, which is why it read as safe — but a guard whose answer depends on WHICH empty it is handed is not a guard. Both spellings now mean "nothing stamped"; the mutant reverting it dies. The same test was passing no `wsSlug` at all and hiding it behind `as never`, so it had been exercising a branch the component no longer takes. Fixed, and its store mock now supplies the stamp so the legs run production's path. 3. `OpenChildrenDialog`'s staleness check is INERT on routes with no workspace param — the console and the root, where this root-layout dialog can also appear. That follows from treating an absent slug as permissive, which is deliberate (blanking a value for a caller that cannot ask the question is the failure that already cost this unit two pages of chips). Documented in the component rather than closed: closing it needs the dialog to learn which workspace the ITEM belongs to, which its DTO does not carry. Also removed the stale "five surfaces" / "six call sites" counts from comments that had outlived them — numbers in prose go stale silently, and this branch has now changed that count four times. Still open and named, not silently dropped: `playbooks/[slug]/+page.svelte` assigns `fields.status` into a form control and writes it back. That is an EDITOR rather than a renderer — a different sub-class, since the failure is a relation id round-tripping through a select rather than being displayed — and it wants its own unit. BUG-3067 Claude-Session: https://claude.ai/code/session_019EmDd3Wy8b3cPG1TZBSzBu * fix(web): a label that contradicted its own action, and a guard fed the wrong input (BUG-3067, round 7) 1. WITHHOLDING THE CHIP BROKE A BUTTON. Round 6 made the playbooks list claim nothing while its schema is unknown, which was right for the chip — and the toggle's label is computed from that same `status`, so every card read "Mark as Draft" regardless of what it held, while the handler read the real stored value and did something else. A label contradicting its own action is worse than the wrong chip round 6 was fixing. The button waits for the schema instead of guessing. That is this unit's shape one more time: a value withheld at one site is a value some other site was reading. 2. `OpenChildrenDialog` WAS COMPARING AGAINST A SLUG THAT MAY BE WRONG. Round 5 gave it the route's workspace param; round 7 pointed out the dialog is root-mounted and its request can outlive a navigation, so that param can name a workspace the children do not belong to. A guard fed the wrong input does not fail safe — it answers "stale" for a store that is correct and "fresh" for one that is not, confidently either way. Worse than no comparison. It makes none now and says so. The real fix is for the children DTO to carry the item's workspace, which it does not. The residual risk is rated and stated: the value is still type-checked against whatever schema is loaded, so the failure mode is a withheld or shown chip under a same-slug collision between two workspaces — not a printed id, which is the thing this unit exists to prevent. 3. The REF-fallback misstatement was still sitting in the ruling's own test file, one commit after I corrected it in the implementation. Same wrong sentence, second artifact — which is the pre-push habit this seat already owes and did not run: a claim corrected in the source is not corrected in the file that pins it. BUG-3067 Claude-Session: https://claude.ai/code/session_019EmDd3Wy8b3cPG1TZBSzBu * fix(web): sort on the declared value too, and stop pretending to compare (BUG-3067, round 8) Round 8 returned the whole withheld-value table I asked for instead of one more instance, which is what finally made this converge. 1. THE SORT WAS STILL READING THE RAW VALUE. Withholding the chip fixed what a playbook card SHOWED and left what it was ordered BY — the same value wearing a different hat. 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. The sort asks the same question the cards ask now. This is the third instance of one shape in this branch, and the reason to state it as a shape: a value withheld at one site is a value some other site was reading. Round 7 found it in a button LABEL, round 8 in a SORT KEY. The table is on the trail so the next person starts from it. 2. NOTHING PRETENDS TO COMPARE ANY MORE. `OpenChildrenDialog` was left holding `notStale = $derived(true)` and an unused import — a comparison that reads like a guard and is a constant. Both are gone: the call simply does not pass the argument, and the comment says why no comparison is possible here rather than leaving a variable to imply one. Two dead imports on the dashboard page went with them. 3. The dialog's prose still carried the round-5 paragraph calling the route workspace the "honest source", directly above the round-7 paragraph explaining that it is not. Corrected: one account, not two contradicting ones, with the residual risk rated in it. Left alone deliberately, from the same table: `isDone` / `doneCount` in `ChildItems` and `NestedChildren` still test the raw status against the terminal list. A relation id matches nothing there, so such a child reads as NOT DONE — which is the honest answer for a status nobody can interpret, and changing it would make a checkbox depend on a schema fetch. BUG-3067 Claude-Session: https://claude.ai/code/session_019EmDd3Wy8b3cPG1TZBSzBu * perf(web): resolve the sort's field once, through a door the module was missing (BUG-3067, round 9) Round 9 found no functional defect. Three things, all small, and one of them is the source guard earning its keep. 1. The playbooks sort called the helper INSIDE the comparator, so it reparsed the collection's schema JSON on every comparison — O(n log n) parses of one string to answer a question that is per-COLLECTION, not per-row. Resolved once in a `$derived` and reused. 2. MY OWN GUARD CAUGHT THE FIX. Hoisting meant holding a `FieldDef` and needing a way to answer with it, and the shortest path was to import `categoricalChipValue` — the lower-level dependency the guard forbids. The test failed the moment I ran it, which is the whole point: a bypass is how a shared question acquires a second answer, and round 1 of this branch found `DetailCard` doing exactly that. The guard was right and the module was missing a door; `categoricalValueForField` is that door. 3. Two comments outlived their facts. `categoricalFieldValue.ts` still counted the call sites, a number this branch has changed five times — it names them without counting now. And `schemalessReaders.test.ts` claimed "per-surface render tests prove two of the surfaces call it", which was never true after the render test for the second surface was deleted as vacuous; it now states exactly what it proves (no surface IN ITS OWN LIST answers privately) and what it cannot (it does not sweep server projections, print markup, share routes or prompt substitution, and cannot tell an imported helper that is called from one that is ignored). BUG-3067 Claude-Session: https://claude.ai/code/session_019EmDd3Wy8b3cPG1TZBSzBu --- .../collections/categoricalFieldValue.test.ts | 205 ++++++++++++++++++ .../lib/collections/categoricalFieldValue.ts | 153 +++++++++++++ .../categoricalRenderers.svelte.test.ts | 121 +++++++++++ .../lib/collections/schemalessReaders.test.ts | 170 +++++++++++++++ web/src/lib/components/ChildItems.svelte | 29 ++- web/src/lib/components/NestedChildren.svelte | 15 +- .../lib/components/OpenChildrenDialog.svelte | 31 ++- .../collections/EditCollectionModal.svelte | 9 +- .../detailCardRetypedField.svelte.test.ts | 109 ++++++++++ .../components/common/QuickActionsMenu.svelte | 19 +- web/src/lib/components/graph/ItemGraph.svelte | 8 +- .../components/search/CommandPalette.svelte | 17 +- web/src/lib/utils/quick-action-preview.ts | 60 ++++- .../lib/utils/quickActionCategorical.test.ts | 87 ++++++++ .../[username]/[workspace]/+page.svelte | 41 +++- .../[username]/[workspace]/graph/+page.svelte | 1 + .../[workspace]/graph/DetailCard.svelte | 38 +++- .../[workspace]/playbooks/+page.svelte | 92 +++++++- 18 files changed, 1165 insertions(+), 40 deletions(-) create mode 100644 web/src/lib/collections/categoricalFieldValue.test.ts create mode 100644 web/src/lib/collections/categoricalFieldValue.ts create mode 100644 web/src/lib/collections/categoricalRenderers.svelte.test.ts create mode 100644 web/src/lib/collections/schemalessReaders.test.ts create mode 100644 web/src/lib/components/collections/detailCardRetypedField.svelte.test.ts create mode 100644 web/src/lib/utils/quickActionCategorical.test.ts 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}