mirror of
https://github.com/PerpetualSoftware/pad.git
synced 2026-10-25 22:10:25 +00:00
fix(web): ten surfaces ask a field type before printing its value (BUG-3067) (#1358)
* 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
This commit is contained in:
@@ -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);
|
||||
});
|
||||
});
|
||||
@@ -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<Item, 'collection_slug'> & { 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;
|
||||
}
|
||||
@@ -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<string, ItemIndexRow> = {
|
||||
[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');
|
||||
});
|
||||
});
|
||||
});
|
||||
@@ -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(/<!--[\s\S]*?-->/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.<key>}` 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']");
|
||||
});
|
||||
});
|
||||
@@ -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)}
|
||||
<!-- ASKED OF THE CHILD'S OWN COLLECTION, not of this item's
|
||||
(BUG-3067). A child row may live in ANY collection, so the
|
||||
declared type of `priority` is a per-child question — and
|
||||
until now nothing asked it at all, so a `priority` retyped
|
||||
to a relation printed its stored item id. This component
|
||||
already had `collectionStore` and `parseSchema` for the
|
||||
collection PICKER; what it lacked was asking them about the
|
||||
row it was drawing. -->
|
||||
{@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 @@
|
||||
<a href="/{username}/{wsSlug}/{child.collection_slug}/{child.slug}" class="child-row" class:has-toggle={canExpand} onclick={(e) => handleChildClick(e, child)}>
|
||||
<span class="child-ref">{formatItemRef(child) ?? ''}</span>
|
||||
<span class="child-title" class:done={isDone}>{child.title}</span>
|
||||
{#if fields.priority}
|
||||
{#if priority}
|
||||
<span
|
||||
class="child-priority"
|
||||
class:high={fields.priority === 'high'}
|
||||
class:critical={fields.priority === 'critical'}
|
||||
class:high={priority === 'high'}
|
||||
class:critical={priority === 'critical'}
|
||||
>
|
||||
{fields.priority}
|
||||
{priority}
|
||||
</span>
|
||||
{/if}
|
||||
</a>
|
||||
@@ -917,6 +927,13 @@
|
||||
<ul class="print-child-list">
|
||||
{#each children as child (child.id)}
|
||||
{@const childFields = parseFields(child)}
|
||||
<!-- THE SECOND RENDERER IN THIS FILE, and the reason the review round
|
||||
found it rather than I did: fixing "ChildItems" is not fixing a
|
||||
FILE, it is fixing each place the file draws a value. The print
|
||||
view draws `status` where the interactive rows draw `priority`,
|
||||
so a sweep that stopped at the first hit in each file missed it
|
||||
(BUG-3067 round 3). -->
|
||||
{@const printStatus = categoricalValueFor(collectionStore.collections, child, 'status', childFields.status, collectionsNotStaleFor(collectionStore.collectionsWorkspace, wsSlug))}
|
||||
{@const isDone = terminal.includes(childFields.status)}
|
||||
<li class="print-child-row" class:done={isDone}>
|
||||
<span class="print-check">{isDone ? '[x]' : '[ ]'}</span>
|
||||
@@ -924,8 +941,8 @@
|
||||
<span class="print-child-ref">{formatItemRef(child)}</span>
|
||||
{/if}
|
||||
<span class="print-child-title">{child.title}</span>
|
||||
{#if childFields.status}
|
||||
<span class="print-child-status">({formatLabel(childFields.status)})</span>
|
||||
{#if printStatus}
|
||||
<span class="print-child-status">({formatLabel(printStatus)})</span>
|
||||
{/if}
|
||||
</li>
|
||||
{/each}
|
||||
|
||||
@@ -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 @@
|
||||
</div>
|
||||
{#each children as child (child.id)}
|
||||
{@const fields = parseFields(child)}
|
||||
<!-- Per CHILD, from the child's own collection (BUG-3067) — see the
|
||||
note in ChildItems. No prop threading was needed: `collectionStore`
|
||||
is a singleton and the row already carries its `collection_slug`,
|
||||
which is why this site turned out to be the cheap half after all. -->
|
||||
{@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 @@
|
||||
<span class="nested-ref">{formatItemRef(child) ?? ''}</span>
|
||||
<span class="nested-title" class:done={isDone}>{child.title}</span>
|
||||
</a>
|
||||
{#if fields.priority}
|
||||
{#if priority}
|
||||
<span
|
||||
class="nested-priority"
|
||||
class:high={fields.priority === 'high'}
|
||||
class:critical={fields.priority === 'critical'}
|
||||
class:high={priority === 'high'}
|
||||
class:critical={priority === 'critical'}
|
||||
>
|
||||
{fields.priority}
|
||||
{priority}
|
||||
</span>
|
||||
{/if}
|
||||
</div>
|
||||
|
||||
@@ -1,4 +1,6 @@
|
||||
<script lang="ts">
|
||||
import { collectionStore } from '$lib/stores/collections.svelte';
|
||||
import { categoricalValueForSlug } from '$lib/collections/categoricalFieldValue';
|
||||
// BUG-1538 / TASK-1539 — Confirm dialog that surfaces the server's
|
||||
// `open_children` 409 guard (IDEA-1494) in the web UI. Mounted once
|
||||
// from +layout.svelte; driven by the openChildrenDialog singleton
|
||||
@@ -17,6 +19,26 @@
|
||||
import Modal from '$lib/components/common/Modal.svelte';
|
||||
import Button from '$lib/components/common/Button.svelte';
|
||||
|
||||
// NO STALENESS COMPARISON IS MADE HERE, and that is a decision rather than an
|
||||
// omission (BUG-3067, rounds 5-8).
|
||||
//
|
||||
// The permissive default was allowed to stand only where a grep shows every
|
||||
// mount site sits under the `[workspace]` layout that stamps the collection
|
||||
// store. This dialog FAILS that test — it is mounted twice in the ROOT layout.
|
||||
// So round 5 gave it the route's workspace param, and round 7 established that
|
||||
// the param can be WRONG: the dialog's request can outlive a navigation, so the
|
||||
// route may name a workspace its children do not belong to. A guard fed the
|
||||
// wrong input does not fail safe; it answers "stale" for a correct store and
|
||||
// "fresh" for a wrong one, confidently either way.
|
||||
//
|
||||
// The children DTO carries each child's `collection_slug` — the link beside the
|
||||
// status is built from it — but NOT the workspace the item lives in, which is
|
||||
// the one thing this comparison would need. So the comparison is not made, and
|
||||
// the residual risk is stated rather than hidden: the value is still
|
||||
// type-checked against whatever schema is loaded, so the failure mode under a
|
||||
// same-slug collision between two workspaces is a withheld or shown chip, not
|
||||
// a printed id — which is the thing this unit exists to prevent.
|
||||
|
||||
let active = $derived(openChildrenDialog.active);
|
||||
|
||||
function onCancel() {
|
||||
@@ -83,6 +105,13 @@
|
||||
</div>
|
||||
<ul class="child-list">
|
||||
{#each active.details.open_children as child (child.ref)}
|
||||
<!-- I FILED THIS AS UNFIXABLE CLIENT-SIDE, claiming the DTO
|
||||
carried no collection identifier. It carries
|
||||
`collection_slug` — the link two lines below is built from
|
||||
it. The claim was about a shape I had not opened, in an
|
||||
item whose whole history is enumeration claims that were
|
||||
not checked (BUG-3067 round 3). -->
|
||||
{@const childStatus = categoricalValueForSlug(collectionStore.collections, child.collection_slug, 'status', child.status)}
|
||||
<li class="child-row">
|
||||
{#if canLink}
|
||||
<a
|
||||
@@ -97,7 +126,7 @@
|
||||
<span class="child-ref">{child.ref}</span>
|
||||
{/if}
|
||||
<span class="child-title">{child.title}</span>
|
||||
<span class="child-status">{child.status}</span>
|
||||
{#if childStatus}<span class="child-status">{childStatus}</span>{/if}
|
||||
</li>
|
||||
{/each}
|
||||
</ul>
|
||||
|
||||
@@ -1,4 +1,5 @@
|
||||
<script lang="ts">
|
||||
import { localIndex } from '$lib/stores/localIndex.svelte';
|
||||
import { isRelationType } from '$lib/items/relationFieldTypes';
|
||||
import { api, isConflictOrNotFound } from '$lib/api/client';
|
||||
import type { Collection, CollectionUpdate, CollectionSettings, FieldDef, FieldMigration, QuickAction } from '$lib/types';
|
||||
@@ -231,7 +232,13 @@
|
||||
limit: 1
|
||||
});
|
||||
if (items && items.length > 0) {
|
||||
previewContext = contextFromItem(items[0], collection);
|
||||
// WITH A RESOLVER (BUG-3067 round 5). `contextFromItem` defaults it
|
||||
// to one that resolves nothing, so this preview rendered every
|
||||
// relation-valued `{status}`/`{priority}` as empty — quietly
|
||||
// disagreeing with the menu it exists to mirror.
|
||||
previewContext = contextFromItem(items[0], collection, (id) =>
|
||||
localIndex.findByIdOrSlug(wsSlug, id),
|
||||
);
|
||||
return;
|
||||
}
|
||||
} catch {
|
||||
|
||||
@@ -0,0 +1,109 @@
|
||||
// BUG-3067 — the behavioural half for the graph detail card.
|
||||
//
|
||||
// The source guard in `schemalessReaders.test.ts` proves every surface in the
|
||||
// class routes through the shared question; it cannot prove a surface renders
|
||||
// the ANSWER correctly. This drives one surface end to end, chosen because it
|
||||
// reads BOTH keys and reads them from two different places — `priority` from the
|
||||
// fetched item's fields, `status` from the graph NODE, which is a server
|
||||
// projection that arrives before the item does.
|
||||
import { describe, it, expect, vi, afterEach } from 'vitest';
|
||||
import { render, cleanup } from '@testing-library/svelte';
|
||||
import type { Collection, Item } from '$lib/types';
|
||||
|
||||
const ID = 'f47ac10b-58cc-4372-a567-0e02b2c3d479';
|
||||
|
||||
let collections: Collection[] = [];
|
||||
let stampedWorkspace: string | null = 'ws';
|
||||
vi.mock('$lib/stores/collections.svelte', () => ({
|
||||
collectionStore: {
|
||||
get collections() {
|
||||
return collections;
|
||||
},
|
||||
// The stamp the component compares its `wsSlug` against. Supplied so these
|
||||
// legs exercise the SAME staleness path production takes; without it the
|
||||
// mock answered undefined and every leg ran the permissive branch.
|
||||
get collectionsWorkspace() {
|
||||
return stampedWorkspace;
|
||||
},
|
||||
},
|
||||
}));
|
||||
|
||||
import DetailCard from '../../../routes/[username]/[workspace]/graph/DetailCard.svelte';
|
||||
|
||||
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: 'X',
|
||||
} as unknown as Collection;
|
||||
}
|
||||
|
||||
const ORDINARY = collection('tasks', [
|
||||
{ key: 'status', label: 'Status', type: 'select', options: ['open', 'done'] },
|
||||
{ key: 'priority', label: 'Priority', type: 'select', options: ['low', 'high'] },
|
||||
]);
|
||||
const RETYPED = collection('cars', [
|
||||
{ key: 'status', label: 'Status', type: 'relation', collection: 'colors' },
|
||||
{ key: 'priority', label: 'Priority', type: 'relation', collection: 'colors' },
|
||||
]);
|
||||
|
||||
function renderCard(collSlug: string, storedStatus: string, storedPriority: string) {
|
||||
const item = {
|
||||
id: 'i1', workspace_id: 'ws1', collection_id: 'c1', slug: 'i1', title: 'An item',
|
||||
content: '', fields: JSON.stringify({ status: storedStatus, priority: storedPriority }),
|
||||
tags: '[]', collection_slug: collSlug, created_at: 'x', updated_at: 'x',
|
||||
} as unknown as Item;
|
||||
return render(DetailCard, {
|
||||
props: {
|
||||
node: {
|
||||
ref: 'CAR-1', title: 'An item', collection: collSlug, status: storedStatus,
|
||||
is_terminal: false, child_count: 0, updated_at: '2026-01-01T00:00:00Z',
|
||||
},
|
||||
// THE REQUIRED PROP, passed rather than left to `as never` (round 6).
|
||||
// Omitting it type-checked because of the cast and silently exercised
|
||||
// the permissive staleness path — a test of a branch the component
|
||||
// no longer takes in production.
|
||||
wsSlug: 'ws',
|
||||
color: '#888', item, itemLoading: false,
|
||||
blockedBy: [], blocksCount: 0, chainDepth: 0,
|
||||
onjump: vi.fn(), onopen: vi.fn(), onclose: vi.fn(),
|
||||
} as never,
|
||||
});
|
||||
}
|
||||
|
||||
afterEach(() => {
|
||||
cleanup();
|
||||
collections = [];
|
||||
stampedWorkspace = 'ws';
|
||||
});
|
||||
|
||||
describe('the graph detail card and a retyped field', () => {
|
||||
it('prints neither id when `status` and `priority` are relations', () => {
|
||||
collections = [ORDINARY, RETYPED];
|
||||
const screen = renderCard('cars', ID, ID);
|
||||
// PRECONDITION: the card rendered, so "no id" is not "no card".
|
||||
expect(screen.container.querySelector('.detail-card')).not.toBeNull();
|
||||
expect(screen.container.textContent).toContain('An item');
|
||||
expect(screen.container.textContent).not.toContain(ID);
|
||||
});
|
||||
|
||||
it('CONTROL: an ordinary collection still shows both', () => {
|
||||
// Without this, withholding every value would satisfy the leg above while
|
||||
// blanking the card for every workspace that never retyped anything.
|
||||
collections = [ORDINARY, RETYPED];
|
||||
const screen = renderCard('tasks', 'open', 'high');
|
||||
expect(screen.container.textContent).toContain('open');
|
||||
expect(screen.container.textContent).toContain('high');
|
||||
});
|
||||
|
||||
it('withholds both while the collections are still loading', () => {
|
||||
// The fail-closed case the helper documents: an unloaded store cannot
|
||||
// vouch for a value, and a momentarily missing chip is recoverable where a
|
||||
// printed id is the bug.
|
||||
collections = [];
|
||||
const screen = renderCard('tasks', 'open', 'high');
|
||||
expect(screen.container.querySelector('.detail-card')).not.toBeNull();
|
||||
expect(screen.container.textContent).not.toContain('open');
|
||||
expect(screen.container.textContent).not.toContain('high');
|
||||
});
|
||||
});
|
||||
@@ -1,4 +1,6 @@
|
||||
<script lang="ts">
|
||||
import { localIndex } from '$lib/stores/localIndex.svelte';
|
||||
import { categoricalTemplateValue } from '$lib/utils/quick-action-preview';
|
||||
import { tick, untrack } from 'svelte';
|
||||
import type { QuickAction, Item, Collection } from '$lib/types';
|
||||
import { parseFields, formatItemRef, parseSettings } from '$lib/types';
|
||||
@@ -71,6 +73,8 @@
|
||||
|
||||
let filtered = $derived(actions.filter((a) => a.scope === scope));
|
||||
|
||||
const resolveRow = (id: string) => localIndex.findByIdOrSlug(wsSlug, id);
|
||||
|
||||
function resolvePrompt(action: QuickAction): string {
|
||||
let prompt = action.prompt;
|
||||
const fields = item ? parseFields(item) : {};
|
||||
@@ -78,8 +82,19 @@
|
||||
const vars: Record<string, string> = {
|
||||
ref: item ? formatItemRef(item) ?? '' : '',
|
||||
title: item?.title ?? '',
|
||||
status: item ? String(fields['status'] ?? '') : '',
|
||||
priority: item ? String(fields['priority'] ?? '') : '',
|
||||
// ONE IMPLEMENTATION, imported rather than mirrored (BUG-3067, lead
|
||||
// ruling). A `status`/`priority` retyped to a relation stores an item
|
||||
// id, and this pasted it straight into a prompt handed to an agent.
|
||||
// The chips answer by withholding; a prompt variable cannot, so it
|
||||
// resolves to the target's TITLE with its REF as the fallback.
|
||||
//
|
||||
// This file and `quick-action-preview.ts` held byte-identical copies of
|
||||
// the substitution, and that module's own doc says it MIRRORS this one
|
||||
// so the preview shows what copying produces. Two mirrors drift; the
|
||||
// preview existing to match makes the drift invisible until someone
|
||||
// compares them.
|
||||
status: item ? categoricalTemplateValue(collection, 'status', fields['status'], resolveRow) : '',
|
||||
priority: item ? categoricalTemplateValue(collection, 'priority', fields['priority'], resolveRow) : '',
|
||||
collection: collection.name,
|
||||
content: item?.content ? item.content.slice(0, 200) : '',
|
||||
fields: Object.entries(fields)
|
||||
|
||||
@@ -14,6 +14,8 @@
|
||||
|
||||
import { onMount, untrack } from 'svelte';
|
||||
import { api } from '$lib/api/client';
|
||||
import { collectionStore } from '$lib/stores/collections.svelte';
|
||||
import { categoricalValueForSlug, collectionsNotStaleFor } from '$lib/collections/categoricalFieldValue';
|
||||
import type { GraphEdge, GraphNode, GraphResponse, PaneTarget } from '$lib/types';
|
||||
import { createCollectionColorMap } from '$lib/graph/palette';
|
||||
import { sseService, type ItemEvent } from '$lib/services/sse.svelte';
|
||||
@@ -899,6 +901,10 @@
|
||||
<!-- Node detail panel (single-click selection). -->
|
||||
{#if selectedNode}
|
||||
{@const sel = selectedNode}
|
||||
<!-- The node is a server projection carrying no schema, but it carries
|
||||
its collection slug, which is all the question needs (BUG-3067): a
|
||||
`status` retyped to a relation printed its stored id in this panel. -->
|
||||
{@const selStatus = categoricalValueForSlug(collectionStore.collections, sel.collection, 'status', sel.status, collectionsNotStaleFor(collectionStore.collectionsWorkspace, workspace))}
|
||||
<div class="detail-card" role="group" aria-label="Item details">
|
||||
<div class="detail-head">
|
||||
<span class="detail-ref">{sel.ref}</span>
|
||||
@@ -912,7 +918,7 @@
|
||||
<p class="detail-title">{sel.title}</p>
|
||||
<div class="detail-meta">
|
||||
<span class="detail-chip" style:background-color="color-mix(in srgb, {sel.color} 22%, transparent)" style:border-color={sel.color}>{sel.collection}</span>
|
||||
{#if sel.status}<span class="detail-stat">{sel.status}</span>{/if}
|
||||
{#if selStatus}<span class="detail-stat">{selStatus}</span>{/if}
|
||||
{#if sel.isTerminal}<span class="detail-stat done">✓ done</span>{/if}
|
||||
{#if sel.childCount > 0}
|
||||
<span class="detail-stat">{sel.childCount} child{sel.childCount === 1 ? '' : 'ren'}</span>
|
||||
|
||||
@@ -5,6 +5,7 @@
|
||||
import { statusColor } from '$lib/utils/fieldColors';
|
||||
import { workspaceStore } from '$lib/stores/workspace.svelte';
|
||||
import { collectionStore } from '$lib/stores/collections.svelte';
|
||||
import { categoricalValueFor } from '$lib/collections/categoricalFieldValue';
|
||||
import { localIndex } from '$lib/stores/localIndex.svelte';
|
||||
import { localSearch, parseSearchQuery, parseGoToTarget } from '$lib/stores/localSearch.svelte';
|
||||
import { uiStore } from '$lib/stores/ui.svelte';
|
||||
@@ -791,10 +792,22 @@
|
||||
|
||||
function renderResultCard(r: AugmentedSearchResult, i: number): { ref: string | null; status: string | undefined; priority: string | undefined } {
|
||||
void i;
|
||||
// ASKED OF EACH RESULT'S OWN COLLECTION (BUG-3067). The palette searches
|
||||
// across every collection, so the declared type of `status`/`priority` is
|
||||
// a per-RESULT question — and reading them by name with no schema printed
|
||||
// a stored item id as a title-cased pill whenever either field had been
|
||||
// retyped to a relation, exactly as the cards did before BUG-3016.
|
||||
//
|
||||
// This was the cheap half of the class all along: the file already looks a
|
||||
// result's collection up by slug in two other places.
|
||||
return {
|
||||
ref: formatItemRef(r.item),
|
||||
status: getFieldValue(r.item, 'status'),
|
||||
priority: getFieldValue(r.item, 'priority')
|
||||
status:
|
||||
categoricalValueFor(collectionStore.collections, r.item, 'status', getFieldValue(r.item, 'status')) ||
|
||||
undefined,
|
||||
priority:
|
||||
categoricalValueFor(collectionStore.collections, r.item, 'priority', getFieldValue(r.item, 'priority')) ||
|
||||
undefined
|
||||
};
|
||||
}
|
||||
|
||||
|
||||
@@ -1,5 +1,7 @@
|
||||
import type { Collection, Item } from '$lib/types';
|
||||
import type { Collection, Item, ItemIndexRow } from '$lib/types';
|
||||
import { formatItemRef, parseFields } from '$lib/types';
|
||||
import { fieldDefFor } from '$lib/collections/categoricalFieldValue';
|
||||
import { isRelationType, relationValuesOf } from '$lib/items/relationFieldTypes';
|
||||
|
||||
/**
|
||||
* The set of template variables the QuickActionsMenu substitutes at
|
||||
@@ -25,6 +27,52 @@ const TEMPLATE_VARIABLE_SET: ReadonlySet<string> = new Set(TEMPLATE_VARIABLES);
|
||||
|
||||
export type PreviewContext = Record<TemplateVariable, string>;
|
||||
|
||||
/**
|
||||
* THE ONE SUBSTITUTION for a categorical template variable (BUG-3067, lead
|
||||
* ruling on the trail).
|
||||
*
|
||||
* A `status` or `priority` retyped to a relation stores an item id, and both
|
||||
* this module and `QuickActionsMenu` pasted that id straight into a prompt the
|
||||
* user then hands to an agent. The chips answer this by WITHHOLDING; a prompt
|
||||
* variable cannot — `{status}` has to become something, and a blank is a lie of
|
||||
* a different kind.
|
||||
*
|
||||
* So the ruling, matching what #1352 did for chips: resolve to the target's
|
||||
* TITLE, fall back to its REF, and never emit the raw id. A list joins with ', '.
|
||||
*
|
||||
* THE FALLBACK IS FOR A ROW WITH NO TITLE, not for a row that does not resolve,
|
||||
* and an earlier version of this comment said the latter — which is not a thing
|
||||
* this function can do. A ref is built FROM the row (its collection prefix and
|
||||
* item number); an id the index cannot resolve yields no row and therefore no
|
||||
* ref, so there is nothing to fall back to. Such a value yields the empty string
|
||||
* because nothing true is left to say about it, which is the one case the
|
||||
* ruling's "never empty by choice" does not reach.
|
||||
*
|
||||
* It lives HERE, and `QuickActionsMenu` imports it, because the two were one
|
||||
* site with two implementations: this module's own doc says it MIRRORS the
|
||||
* menu so the preview shows what copying produces, and two mirrors drift.
|
||||
*/
|
||||
export function categoricalTemplateValue(
|
||||
collection: Collection | undefined,
|
||||
key: string,
|
||||
raw: unknown,
|
||||
resolve: (id: string) => ItemIndexRow | null | undefined,
|
||||
): string {
|
||||
const field = collection ? fieldDefFor([collection], collection.slug, key) : undefined;
|
||||
if (!field || !isRelationType(field.type)) {
|
||||
return raw === null || raw === undefined ? '' : String(raw);
|
||||
}
|
||||
const ids = relationValuesOf(field.type, raw);
|
||||
const labels: string[] = [];
|
||||
for (const id of ids) {
|
||||
const row = resolve(id);
|
||||
if (!row) continue;
|
||||
labels.push(row.title || formatItemRef(row) || '');
|
||||
}
|
||||
return labels.filter(Boolean).join(', ');
|
||||
}
|
||||
|
||||
|
||||
/**
|
||||
* Reshape an item-scope preview context into a collection-scope one by
|
||||
* clearing all item-only variables. Mirrors the runtime in
|
||||
@@ -74,13 +122,17 @@ export function placeholderContext(collectionName: string): PreviewContext {
|
||||
* substitution logic in QuickActionsMenu.svelte so the preview is a true
|
||||
* representation of what copying the prompt would produce.
|
||||
*/
|
||||
export function contextFromItem(item: Item, collection: Collection): PreviewContext {
|
||||
export function contextFromItem(
|
||||
item: Item,
|
||||
collection: Collection,
|
||||
resolve: (id: string) => ItemIndexRow | null | undefined = () => null,
|
||||
): PreviewContext {
|
||||
const fields = parseFields(item);
|
||||
return {
|
||||
ref: formatItemRef(item) ?? '',
|
||||
title: item.title ?? '',
|
||||
status: String(fields['status'] ?? ''),
|
||||
priority: String(fields['priority'] ?? ''),
|
||||
status: categoricalTemplateValue(collection, 'status', fields['status'], resolve),
|
||||
priority: categoricalTemplateValue(collection, 'priority', fields['priority'], resolve),
|
||||
collection: collection.name,
|
||||
content: item.content ? item.content.slice(0, 200) : '',
|
||||
fields: Object.entries(fields)
|
||||
|
||||
@@ -0,0 +1,87 @@
|
||||
// BUG-3067, lead ruling on the trail — the prompt-substitution half of the class.
|
||||
//
|
||||
// The chips answer a retyped `status`/`priority` by WITHHOLDING the value. A
|
||||
// prompt variable cannot: `{status}` has to become something, and a blank is a
|
||||
// lie of a different kind. The ruling matches what #1352 did for chips —
|
||||
// resolve to the target's TITLE, fall back to its REF, never emit the raw id.
|
||||
// The FALLBACK IS FOR A RESOLVED ROW WITH NO TITLE — an earlier version of this
|
||||
// comment said "when the row cannot be resolved", which the code cannot do and
|
||||
// the legs below show it does not: a ref is built from the row, so an id that
|
||||
// resolves to nothing has no ref to fall back to and yields empty.
|
||||
//
|
||||
// The second half of the ruling is structural: `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. Two mirrors drift, and a preview whose purpose is to
|
||||
// match makes the drift invisible. They are one function now, imported.
|
||||
import { describe, it, expect } from 'vitest';
|
||||
import type { Collection, ItemIndexRow } from '$lib/types';
|
||||
import { categoricalTemplateValue } from './quick-action-preview';
|
||||
|
||||
function collection(fields: unknown[]): Collection {
|
||||
return {
|
||||
id: 'c1', workspace_id: 'ws1', name: 'Cars', slug: 'cars', 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 ORDINARY = collection([{ key: 'status', label: 'Status', type: 'select', options: ['open'] }]);
|
||||
const RETYPED = collection([{ key: 'status', label: 'Status', type: 'relation', collection: 'colors' }]);
|
||||
const LIST = collection([{ key: 'status', label: 'Status', type: 'multi_relation', collection: 'colors' }]);
|
||||
|
||||
const RED = 'id-red';
|
||||
const BLUE = 'id-blue';
|
||||
const NAMELESS = 'id-nameless';
|
||||
const GONE = 'id-gone';
|
||||
|
||||
const ROWS: Record<string, ItemIndexRow> = {
|
||||
[RED]: { id: RED, title: 'Red', item_number: 1, collection_prefix: 'COLOR', collection_slug: 'colors', deleted_at: null } as unknown as ItemIndexRow,
|
||||
[BLUE]: { id: BLUE, title: 'Blue', item_number: 2, collection_prefix: 'COLOR', collection_slug: 'colors', deleted_at: null } as unknown as ItemIndexRow,
|
||||
// Title-less on purpose: this is what makes the REF fallback reachable.
|
||||
[NAMELESS]: { id: NAMELESS, title: '', item_number: 7, collection_prefix: 'COLOR', collection_slug: 'colors', deleted_at: null } as unknown as ItemIndexRow,
|
||||
};
|
||||
const resolve = (id: string) => ROWS[id] ?? null;
|
||||
|
||||
describe('a categorical template variable', () => {
|
||||
it('passes an ordinary value straight through', () => {
|
||||
expect(categoricalTemplateValue(ORDINARY, 'status', 'open', resolve)).toBe('open');
|
||||
});
|
||||
|
||||
it('resolves a relation to the TITLE, never the id', () => {
|
||||
const out = categoricalTemplateValue(RETYPED, 'status', RED, resolve);
|
||||
expect(out).toBe('Red');
|
||||
expect(out).not.toContain(RED);
|
||||
});
|
||||
|
||||
it('falls back to the REF when the row has no title', () => {
|
||||
// The fallback the ruling names, and it needs a title-less row to be
|
||||
// reachable at all — a fixture that always has a title would make this
|
||||
// leg pass without ever exercising the branch.
|
||||
expect(categoricalTemplateValue(RETYPED, 'status', NAMELESS, resolve)).toBe('COLOR-7');
|
||||
});
|
||||
|
||||
it('joins a list with a comma and a space', () => {
|
||||
expect(categoricalTemplateValue(LIST, 'status', [RED, BLUE], resolve)).toBe('Red, Blue');
|
||||
});
|
||||
|
||||
it('emits nothing for an id that resolves to nothing — and never the id itself', () => {
|
||||
// Empty here is not a choice, it is what is left when there is nothing
|
||||
// true to say. The assertion that matters is the second one.
|
||||
const out = categoricalTemplateValue(RETYPED, 'status', GONE, resolve);
|
||||
expect(out).toBe('');
|
||||
expect(out).not.toContain(GONE);
|
||||
});
|
||||
|
||||
it('drops only the unresolvable element of a list', () => {
|
||||
expect(categoricalTemplateValue(LIST, 'status', [RED, GONE, BLUE], resolve)).toBe('Red, Blue');
|
||||
});
|
||||
|
||||
it('CONTROL: with no collection in hand it still does not invent a lookup', () => {
|
||||
// A caller that cannot supply the schema gets the old stringify. That is
|
||||
// the honest degradation: this helper cannot tell a relation from a
|
||||
// select without the declared type, and guessing from the value's shape
|
||||
// is the mistake the whole class is made of.
|
||||
expect(categoricalTemplateValue(undefined, 'status', 'open', resolve)).toBe('open');
|
||||
});
|
||||
});
|
||||
@@ -1,4 +1,5 @@
|
||||
<script lang="ts">
|
||||
import { categoricalValueFor } from '$lib/collections/categoricalFieldValue';
|
||||
import { page } from '$app/state';
|
||||
import { onMount, onDestroy, untrack } from 'svelte';
|
||||
import { browser } from '$app/environment';
|
||||
@@ -515,6 +516,14 @@
|
||||
</div>
|
||||
<div class="active-grid">
|
||||
{#each dashboard.active_items as item (item.slug)}
|
||||
<!-- TWO RENDERERS, one defect. Both values are server projections
|
||||
read BY NAME (`extractFieldValue`), so a retyped field arrives as
|
||||
a stored item id (BUG-3067). The row carries its collection slug,
|
||||
so the declared type is answerable here. This pair appears twice
|
||||
on the page — active cards and starred cards — which is why the
|
||||
enumeration is a grep and not a reading. -->
|
||||
{@const cardStatus = categoricalValueFor(collections, item, 'status', item.status)}
|
||||
{@const cardPriority = categoricalValueFor(collections, item, 'priority', item.priority)}
|
||||
<a href="/{username}/{wsSlug}/{item.collection_slug}/{item.slug}" class="active-card" class:just-created={justCreatedSlugs.has(item.slug)}>
|
||||
{#if justCreatedSlugs.has(item.slug)}
|
||||
<span class="just-created-badge">✨ your agent just created this</span>
|
||||
@@ -527,11 +536,13 @@
|
||||
</div>
|
||||
<div class="active-title">{item.title}</div>
|
||||
<div class="active-card-bottom">
|
||||
<span class="status-pill" style="background: color-mix(in srgb, {statusColor(item.status)} 15%, transparent); color: {statusColor(item.status)};">
|
||||
{item.status.replace(/-/g, ' ')}
|
||||
</span>
|
||||
{#if item.priority}
|
||||
<span class="active-priority" style="--chip-c: {priorityColor(item.priority)};">{item.priority}</span>
|
||||
{#if cardStatus}
|
||||
<span class="status-pill" style="background: color-mix(in srgb, {statusColor(cardStatus)} 15%, transparent); color: {statusColor(cardStatus)};">
|
||||
{cardStatus.replace(/-/g, ' ')}
|
||||
</span>
|
||||
{/if}
|
||||
{#if cardPriority}
|
||||
<span class="active-priority" style="--chip-c: {priorityColor(cardPriority)};">{cardPriority}</span>
|
||||
{/if}
|
||||
<span class="active-time" title={new Date(item.updated_at).toLocaleString()}>{relativeTime(item.updated_at)}</span>
|
||||
</div>
|
||||
@@ -551,6 +562,14 @@
|
||||
</div>
|
||||
<div class="active-grid">
|
||||
{#each dashboard.starred_items as item (item.slug)}
|
||||
<!-- TWO RENDERERS, one defect. Both values are server projections
|
||||
read BY NAME (`extractFieldValue`), so a retyped field arrives as
|
||||
a stored item id (BUG-3067). The row carries its collection slug,
|
||||
so the declared type is answerable here. This pair appears twice
|
||||
on the page — active cards and starred cards — which is why the
|
||||
enumeration is a grep and not a reading. -->
|
||||
{@const cardStatus = categoricalValueFor(collections, item, 'status', item.status)}
|
||||
{@const cardPriority = categoricalValueFor(collections, item, 'priority', item.priority)}
|
||||
<a href="/{username}/{wsSlug}/{item.collection_slug}/{item.slug}" class="active-card">
|
||||
<div class="active-card-top">
|
||||
{#if item.item_ref}
|
||||
@@ -560,11 +579,13 @@
|
||||
</div>
|
||||
<div class="active-title">{item.title}</div>
|
||||
<div class="active-card-bottom">
|
||||
<span class="status-pill" style="background: color-mix(in srgb, {statusColor(item.status)} 15%, transparent); color: {statusColor(item.status)};">
|
||||
{item.status.replace(/-/g, ' ')}
|
||||
</span>
|
||||
{#if item.priority}
|
||||
<span class="active-priority" style="--chip-c: {priorityColor(item.priority)};">{item.priority}</span>
|
||||
{#if cardStatus}
|
||||
<span class="status-pill" style="background: color-mix(in srgb, {statusColor(cardStatus)} 15%, transparent); color: {statusColor(cardStatus)};">
|
||||
{cardStatus.replace(/-/g, ' ')}
|
||||
</span>
|
||||
{/if}
|
||||
{#if cardPriority}
|
||||
<span class="active-priority" style="--chip-c: {priorityColor(cardPriority)};">{cardPriority}</span>
|
||||
{/if}
|
||||
</div>
|
||||
</a>
|
||||
|
||||
@@ -1326,6 +1326,7 @@
|
||||
<!-- Focus-mode detail card (slides in from the right when a node is selected). -->
|
||||
{#if selectedNode}
|
||||
<DetailCard
|
||||
{wsSlug}
|
||||
node={selectedNode}
|
||||
color={colorForCollection(selectedNode.collection)}
|
||||
item={selectedItem}
|
||||
|
||||
@@ -8,6 +8,12 @@
|
||||
// (backdrop-blur, color-mix surfaces) so the focus layer reads as the same UI.
|
||||
import { relativeTime } from '$lib/utils/markdown';
|
||||
import type { Item } from '$lib/types';
|
||||
import { collectionStore } from '$lib/stores/collections.svelte';
|
||||
import {
|
||||
categoricalValueFor,
|
||||
categoricalValueForSlug,
|
||||
collectionsNotStaleFor,
|
||||
} from '$lib/collections/categoricalFieldValue';
|
||||
|
||||
// The selected node's renderer-facing shape (a subset of the page's GraphNode3D).
|
||||
// Kept structural so the page can pass its mapped node straight through.
|
||||
@@ -23,6 +29,7 @@
|
||||
|
||||
let {
|
||||
node,
|
||||
wsSlug,
|
||||
color,
|
||||
item,
|
||||
itemLoading,
|
||||
@@ -34,6 +41,16 @@
|
||||
onclose
|
||||
}: {
|
||||
node: SelectedNode;
|
||||
/**
|
||||
* THREADED RATHER THAN ARGUED (BUG-3067 round 5, lead ruling). The
|
||||
* permissive staleness default was allowed to stand only on a grep showing
|
||||
* every mount site sits under the `[workspace]` layout that stamps the
|
||||
* collection store. This component has exactly one mount site — the graph
|
||||
* page, which does — so the argument held; the slug is threaded anyway
|
||||
* because a prop is checkable and an argument about mount sites goes stale
|
||||
* the first time someone mounts it somewhere else.
|
||||
*/
|
||||
wsSlug: string;
|
||||
/** collection color (hex) — shared with the node's renderer color. */
|
||||
color: string;
|
||||
/** full item, fetched lazily by the page; null until it arrives. */
|
||||
@@ -75,8 +92,23 @@
|
||||
}
|
||||
});
|
||||
|
||||
// ASKED OF THE ITEM'S OWN COLLECTION (BUG-3067). `typeof … === 'string'` is
|
||||
// the right question for a LIST-typed field and the wrong one for a scalar
|
||||
// `relation`, whose value IS a string — so a retyped `priority` printed its
|
||||
// stored item id here, exactly as it did on the cards before BUG-3016.
|
||||
const notStale = $derived(collectionsNotStaleFor(collectionStore.collectionsWorkspace, wsSlug));
|
||||
|
||||
const priority = $derived(
|
||||
typeof fields.priority === 'string' && fields.priority ? fields.priority : null
|
||||
item ? categoricalValueFor(collectionStore.collections, item, 'priority', fields.priority, notStale) || null : null
|
||||
);
|
||||
|
||||
// The STATUS pill comes from the graph NODE rather than from the fetched
|
||||
// item, and the node is a server projection that carries no schema — but it
|
||||
// does carry the item's collection slug, which is all the question needs. It
|
||||
// renders before `item` lands, so it is resolved from the node rather than
|
||||
// waiting for the fetch.
|
||||
const nodeStatus = $derived(
|
||||
categoricalValueForSlug(collectionStore.collections, node.collection, 'status', node.status, notStale),
|
||||
);
|
||||
const assignee = $derived(item?.assigned_user_name ?? null);
|
||||
</script>
|
||||
@@ -92,8 +124,8 @@
|
||||
<h2 class="title">{node.title}</h2>
|
||||
|
||||
<div class="pills">
|
||||
{#if node.status}
|
||||
<span class="pill" class:terminal={node.is_terminal}>{node.status}</span>
|
||||
{#if nodeStatus}
|
||||
<span class="pill" class:terminal={node.is_terminal}>{nodeStatus}</span>
|
||||
{/if}
|
||||
{#if node.child_count > 0}
|
||||
<span class="meta">{node.child_count} {node.child_count === 1 ? 'child' : 'children'}</span>
|
||||
|
||||
@@ -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, unknown>): 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 @@
|
||||
<div class="cards">
|
||||
{#each sorted as item (item.id)}
|
||||
{@const fields = parseFields(item)}
|
||||
{@const status = fields.status ?? 'draft'}
|
||||
<!-- ASKED OF THE PLAYBOOKS COLLECTION'S OWN SCHEMA (BUG-3067). A
|
||||
SYSTEM collection is not an exempt one: nothing in
|
||||
`handleUpdateCollection` refuses a schema edit for
|
||||
`is_system`, so `status` here can be retyped to a relation
|
||||
exactly as on any user collection — and this page printed the
|
||||
stored value through `statusLabel`, and keyed its card
|
||||
styling off it. I had classified this site as a non-member on
|
||||
the assumption that a system schema was immutable, and filed
|
||||
that assumption without checking it; a review round found it.
|
||||
|
||||
RESOLVED FROM `playbooksCollection`, the object this page
|
||||
already fetched under its own stale-response guard, rather
|
||||
than from the shared store. Two things fall out for free: the
|
||||
staleness question does not arise, because the fetch is
|
||||
already workspace-guarded; and the lookup uses that object's
|
||||
OWN slug, so it survives a rename of the playbooks collection
|
||||
where a hardcoded `'playbooks'` literal would not. The rest
|
||||
of this page still addresses the collection by literal, so
|
||||
the page as a whole does not yet survive that rename — this
|
||||
read simply stops adding to the problem. -->
|
||||
{@const declaredStatus = categoricalValueFor(playbooksCollection ? [playbooksCollection] : [], { collection_slug: playbooksCollection?.slug }, 'status', fields.status)}
|
||||
<!-- "NO SCHEMA YET" IS NOT "DRAFT" (BUG-3067 round 5). The two
|
||||
loaders are independent — `loadPlaybooks` clears `loading` on
|
||||
its own — so cards render while `playbooksCollection` is still
|
||||
null, which the helper answers with ''. Falling through to the
|
||||
`?? 'draft'` default then reported every card as Draft, which
|
||||
is a MISREPORT rather than a missing chip: it names a status
|
||||
the item may not have. Reachable on first load, on a slow
|
||||
schema fetch, and permanently if that fetch fails.
|
||||
MY FIRST ANSWER WAS TO STAND THE RAW VALUE IN while the schema
|
||||
was unknown, on the reasoning that it is what the page showed
|
||||
before this unit. That reintroduced the defect for the
|
||||
duration of the window it was meant to fix: a retyped status
|
||||
IS an id, so the raw value is the id, printed. Worse than the
|
||||
Draft misreport it replaced, and the review round said so.
|
||||
|
||||
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 — the affordance is withheld, not the row. -->
|
||||
{@const status = playbooksCollection ? declaredStatus : ''}
|
||||
{@const trigger = fields.trigger ?? 'manual'}
|
||||
{@const scope = fields.scope ?? 'all'}
|
||||
{@const steps = countSteps(item.content)}
|
||||
@@ -514,7 +584,7 @@
|
||||
<button class="card-header" onclick={() => toggleExpand(item.id)} aria-expanded={isExpanded}>
|
||||
<div class="card-title-row">
|
||||
<span class="card-title" class:deprecated-title={status === 'deprecated'}>{item.title}</span>
|
||||
<Chip size="sm" color={statusColor(status)}>{statusLabel(status)}</Chip>
|
||||
{#if status}<Chip size="sm" color={statusColor(status)}>{statusLabel(status)}</Chip>{/if}
|
||||
</div>
|
||||
<div class="card-meta">
|
||||
<Chip size="sm" color="var(--status-blue)">{trigger}</Chip>
|
||||
@@ -548,9 +618,19 @@
|
||||
<div class="card-divider"></div>
|
||||
<div class="card-actions">
|
||||
<Button variant="secondary" size="sm" onclick={() => goto(`/${username}/${wsSlug}/playbooks/${itemUrlId(item)}`)}>Edit</Button>
|
||||
<Button variant="secondary" size="sm" disabled={togglingStatus === item.slug} onclick={() => toggleStatus(item)}>
|
||||
{togglingStatus === item.slug ? '...' : nextStatusLabel(status)}
|
||||
</Button>
|
||||
<!-- DISABLED WHILE THE SCHEMA IS UNKNOWN (BUG-3067 round 7).
|
||||
Withholding the chip left `status` empty, and this label is
|
||||
computed FROM it — so every card read "Mark as Draft"
|
||||
regardless of what it actually held, while the handler read
|
||||
the real stored value and did something else. A label that
|
||||
contradicts its own action is worse than the wrong chip
|
||||
this round was fixing; the button waits for the schema
|
||||
rather than guessing. -->
|
||||
{#if status}
|
||||
<Button variant="secondary" size="sm" disabled={togglingStatus === item.slug} onclick={() => toggleStatus(item)}>
|
||||
{togglingStatus === item.slug ? '...' : nextStatusLabel(status)}
|
||||
</Button>
|
||||
{/if}
|
||||
<Button variant="secondary" size="sm" disabled={duplicating === item.slug} onclick={() => duplicatePlaybook(item)}>
|
||||
{duplicating === item.slug ? '...' : 'Duplicate'}
|
||||
</Button>
|
||||
|
||||
Reference in New Issue
Block a user