diff --git a/web/src/lib/collections/createDefaultCallers.test.ts b/web/src/lib/collections/createDefaultCallers.test.ts new file mode 100644 index 000000000..e1da987a3 --- /dev/null +++ b/web/src/lib/collections/createDefaultCallers.test.ts @@ -0,0 +1,86 @@ +// Node-project test (no DOM): SOURCE-level guard that every create path takes +// its status default from `createDefaults` rather than reading `options[0]` +// itself (BUG-3078). +// +// This is the defect's own shape, so it is the thing worth pinning. The bug was +// not a wrong line — it was SIX copies of one question ("what status should a +// new item start with?") in six files, none of which asked whether the field +// could hold the answer. A behaviour test at one site says nothing about the +// other five, and a mutant that re-inlines the read at a site with no behaviour +// test of its own is invisible to every assertion in the suite. The claim "there +// is one copy" is a fact about the SOURCE, so it is asserted where it lives — +// the same shape as `laneKeyCallers.test.ts`, which exists for the sibling +// question about a lane KEY. +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'); + +/** Every surface that creates an item with a default status. */ +const CALLERS: { name: string; rel: string; expectedCalls: number }[] = [ + { name: 'QuickCaptureSheet', rel: '../components/layout/QuickCaptureSheet.svelte', expectedCalls: 1 }, + { name: 'Sidebar quick-add', rel: '../components/layout/Sidebar.svelte', expectedCalls: 1 }, + { name: 'EditorBubbleMenu', rel: '../components/editor/EditorBubbleMenu.svelte', expectedCalls: 1 }, + // Three separate create paths in one file: "Untitled", create-in-lane, and + // quick-create. Counted rather than merely detected, so removing two of the + // three still fails. + { name: 'collection page', rel: '../../routes/[username]/[workspace]/[collection]/+page.svelte', expectedCalls: 3 }, +]; + +/** + * The shape the fix removed, written to match the VARIANTS rather than the one + * spelling that happened to be there. + * + * Three of the six wrote `f => f.key` and three wrote `(f) => f.key`; two used a + * local named `defaultFields` and one used `fields`. A guard keyed on one + * spelling would have passed on the unfixed tree for half the sites, which is + * the same mistake the code made. This keys on the ASSIGNMENT — anything + * reading an `options[0]` off a status-ish field def into a result object. + */ +const INLINE_READ = /\.options\s*(\?\.)?\[\s*0\s*\]/; + +describe('every create path takes its status default from createDefaults', () => { + for (const caller of CALLERS) { + it(`${caller.name} calls createDefaultFields ${caller.expectedCalls}x`, () => { + const src = read(caller.rel); + expect(src, `${caller.name} should import createDefaultFields`).toContain( + "from '$lib/collections/createDefaults'", + ); + const calls = src.match(/createDefaultFields\s*\(/g) ?? []; + expect(calls.length, `${caller.name} call count`).toBe(caller.expectedCalls); + }); + + it(`${caller.name} reads no options[0] of its own`, () => { + expect(INLINE_READ.test(read(caller.rel)), `${caller.name} has an inline options[0]`).toBe( + false, + ); + }); + } + + it('the inline-read pattern matches every spelling the six sites used', () => { + // Positive control. Without this the guard above could be a regex that + // matches nothing, which passes on any tree and proves nothing — the + // failure mode that makes an absence assertion vacuous. + const spellings = [ + "defaultFields.status = statusField.options[0];", + "fields.status = field.options[0];", + "defaultFields.status = statusField.options?.[0];", + "defaultFields.status = statusField.options[ 0 ];", + ]; + for (const s of spellings) { + expect(INLINE_READ.test(s), s).toBe(true); + } + }); + + it('the inline-read pattern does not match the module that legitimately reads options', () => { + // Negative control, and a real one: `createDefaults.ts` and + // `laneWriteValue.ts` both index `options`, and a guard that flagged them + // would have to be weakened until it flagged nothing. + expect(INLINE_READ.test('const first = statusField?.options?.[0];')).toBe(true); + expect(read('./createDefaults.ts')).toContain('options?.[0]'); + // ...which is exactly why the guard is scoped to the CALLERS list above + // and never swept across the tree. + expect(CALLERS.some((c) => c.rel.includes('createDefaults'))).toBe(false); + }); +}); diff --git a/web/src/lib/collections/createDefaults.test.ts b/web/src/lib/collections/createDefaults.test.ts new file mode 100644 index 000000000..a1ec56f40 --- /dev/null +++ b/web/src/lib/collections/createDefaults.test.ts @@ -0,0 +1,121 @@ +// Node-project test: the status default a create starts with (BUG-3078). +// +// The four arms below are the four outcomes MEASURED against the real create +// handler before this module was written, not four cases imagined from the +// types. On a collection whose `status` is `multi_select` with a stale scalar +// default that survived the retype: +// +// supplied `{"status":"open"}` -> 400, "must be an array of strings" +// omitted `{}` -> 201, stored {"status":"open"} (unchecked) +// converted `{"status":["open"]}` -> 201, stored {"status":["open"]} +// omitted, no stale default -> 201, stored {} +// +// That is why this converts where it can and omits only where it cannot: the +// converted write is the one arm that both lands AND stores a correctly-typed +// value. The probe and its numbers are on the BUG-3078 trail. +import { describe, it, expect } from 'vitest'; +import { createDefaultFields } from './createDefaults'; +import { laneWriteValue } from './laneWriteValue'; +import type { Collection } from '$lib/types'; + +const coll = (statusField: Record | null): Collection => + ({ + slug: 'tasks', + name: 'Tasks', + schema: JSON.stringify({ fields: statusField ? [statusField] : [] }), + settings: '{}', + }) as unknown as Collection; + +describe('createDefaultFields', () => { + it('uses the first option for a select — the case that was never broken', () => { + expect(createDefaultFields(coll({ key: 'status', type: 'select', options: ['open', 'done'] }))) + .toEqual({ status: 'open' }); + }); + + it('CONVERTS for a multi_select instead of omitting', () => { + // The arm the server accepts and stores correctly typed. Omitting here + // would fall through to the schema default, which is assigned WITHOUT a + // type check — so omitting would be strictly worse when that default is + // a stale scalar. + expect( + createDefaultFields(coll({ key: 'status', type: 'multi_select', options: ['open', 'done'] })), + ).toEqual({ status: ['open'] }); + }); + + it('OMITS for number and checkbox, where no conversion exists', () => { + // Reachable only because `options` survives a retype; a field that was + // always a number has none and never reached the old code either. + // + // KEY ABSENCE, not `toEqual({})`. A mutant that drops the `ok` check + // assigns `undefined` — `laneWriteValue`'s refusal carries no `value` — + // and `toEqual` treats an undefined-valued key as absent, so it SURVIVED + // the first version of this test. `Object.keys` is what tells the two + // apart. It matters beyond the mutant only for a future caller that + // spreads or enumerates the result: today's callers `JSON.stringify` it, + // which drops an undefined value, so the wire payload is identical and + // the distinction is the module's contract rather than an observed bug. + for (const type of ['number', 'checkbox']) { + const got = createDefaultFields(coll({ key: 'status', type, options: ['open', 'done'] })); + expect(Object.keys(got), type).toEqual([]); + expect('status' in got, type).toBe(false); + } + }); + + it('OMITS for relation and multi_relation, whose values are item ids', () => { + // Codex round 1 found `multi_relation`; `relation` is the same class and + // came from asking what else reaches that branch. Both refusals were + // measured against the create handler, not reasoned about: + // relation -> 400 "open" does not name an item in collection "tasks" + // multi_relation -> 400 must be an array of strings (item IDs, refs, …) + // There is no honest conversion for either: wrapping as ['open'] clears + // the shape check and is then refused by referent validation, because a + // leftover select option names no item. + for (const type of ['relation', 'multi_relation']) { + const got = createDefaultFields( + coll({ key: 'status', type, collection: 'tasks', options: ['open', 'done'] }), + ); + expect(Object.keys(got), type).toEqual([]); + } + }); + + it('does NOT let laneWriteValue decide the reference case, because it cannot', () => { + // Pins the asymmetry rather than the outcome, so a later "simplification" + // that deletes the REFERENCE_TYPES guard and leans on laneWriteValue + // fails here with the reason attached. `laneWriteValue` passes a relation + // key straight through BY DESIGN — a lane key for a relation is already + // the target's item id. `options[0]` is not a lane key and never an id. + const relField = { key: 'status', type: 'relation', collection: 'tasks', options: ['open'] }; + expect(laneWriteValue(relField as never, 'open')).toEqual({ ok: true, value: 'open' }); + expect(Object.keys(createDefaultFields(coll(relField)))).toEqual([]); + }); + + it('omits when the field declares no options at all', () => { + expect(Object.keys(createDefaultFields(coll({ key: 'status', type: 'select' })))).toEqual([]); + expect( + Object.keys(createDefaultFields(coll({ key: 'status', type: 'select', options: [] }))), + ).toEqual([]); + }); + + it('omits when there is no status field, and when there is no collection', () => { + expect(createDefaultFields(coll(null))).toEqual({}); + expect(createDefaultFields(coll({ key: 'priority', type: 'select', options: ['high'] }))).toEqual({}); + expect(createDefaultFields(null)).toEqual({}); + expect(createDefaultFields(undefined)).toEqual({}); + }); + + it('survives a malformed schema rather than throwing', () => { + // `parseSchema` swallows the parse error and returns defaults; a create + // path that threw here would be a worse failure than the one being fixed. + expect(createDefaultFields({ schema: 'not json', settings: '{}' } as unknown as Collection)) + .toEqual({}); + }); + + it('returns a FRESH object each call', () => { + // Callers mutate the result (they add lane values to it), so a shared + // object would leak one create's lane value into the next create. + const c = coll({ key: 'status', type: 'select', options: ['open'] }); + const a = createDefaultFields(c); + (a as Record).leaked = true; + expect(createDefaultFields(c)).toEqual({ status: 'open' }); + }); +}); diff --git a/web/src/lib/collections/createDefaults.ts b/web/src/lib/collections/createDefaults.ts new file mode 100644 index 000000000..fecf13fb4 --- /dev/null +++ b/web/src/lib/collections/createDefaults.ts @@ -0,0 +1,90 @@ +// The `status` a create should start with (BUG-3078). +// +// Six create paths independently did this: +// +// const statusField = schema.fields.find((f) => f.key === 'status'); +// if (statusField?.options?.length) { +// defaultFields.status = statusField.options[0]; +// } +// +// Nothing asked whether `status` could still HOLD a string. `options` survives a +// retype — the premise BUG-3057, BUG-3067, BUG-3068 and BUG-3074 all turn on — +// so on a collection whose `status` was retyped to `multi_select`, `number` or +// `checkbox` while keeping its options, every one of those sites sent a scalar +// into a field that cannot hold it and the server REFUSED the create outright. +// Not a degraded create: no item, an error toast, and no route to a good +// outcome. Quick capture simply stopped working on that collection. +// +// ONE MODULE for the same reason `laneKeyCallers.test.ts` exists: six sites +// asking one question in six places is how the question comes to have six +// answers. `createDefaultCallers.test.ts` pins that they all call this. +// +// WHAT EACH ARM DOES, and why it is not uniform: +// +// * The conversion is `laneWriteValue`'s, not a second copy of it. For a +// `multi_select` it yields `['open']` rather than `'open'`, which the server +// ACCEPTS and stores correctly typed (measured against the create handler, +// not assumed — see the BUG-3078 trail for the four-arm probe). +// * Where `laneWriteValue` REFUSES — `number`, `checkbox` — the key is OMITTED +// and the server applies the collection's own schema default. Omitting is +// the only remaining move: a create has no lane to refuse into, and failing +// the create is the behaviour being fixed. +// * REFERENCE-SHAPED types are omitted HERE rather than by `laneWriteValue`, +// and that asymmetry is the one thing in this module that is not obvious. +// `laneWriteValue` answers "can this LANE KEY become a value", and a lane +// key for a relation is ALREADY the target's item id — its own comment says +// so — which is why it passes `relation` straight through. This caller does +// not hold a lane key. It holds `options[0]`, a leftover `select` option, +// which is never an item id. So the same string is a good value there and a +// bad one here, and changing `laneWriteValue` to refuse it would break the +// lane path it was written for. +// +// Both arms measured against the create handler rather than reasoned about: +// `relation` answers 400 `"open" does not name an item in collection +// "tasks"`, and `multi_relation` answers 400 `must be an array of strings +// (item IDs, refs, or exact titles)`. There is no honest conversion for +// either — wrapping it as `['open']` passes the shape check and is then +// refused by referent validation, because the option names no item — so the +// key is omitted and the collection's own default applies. +// +// Codex found `multi_relation`; `relation` is the same class and was found +// by asking what else reaches that branch (CONVE-18). +// +// A CAVEAT THE OMIT ARM INHERITS, recorded because it is invisible from here. +// `ValidateFieldsDetailed` assigns a schema default and skips its own type +// check, so a collection whose stale SCALAR default also survived the retype +// stores that scalar unvalidated when the key is omitted. That is a server-side +// hole with its own item, reachable today by any caller that creates without a +// status (CLI, MCP, plain API) — this module routes onto it rather than opening +// it. It is still strictly better than the refusal it replaces, because the +// alternative is no item at all. +import type { Collection } from '$lib/types'; +import { parseSchema } from '$lib/types'; +import { laneWriteValue } from './laneWriteValue'; + +/** Field types whose values are item references, never a declared option. */ +const REFERENCE_TYPES = new Set(['relation', 'multi_relation']); + +/** + * The `fields` object a new item should be created with. + * + * Returns an EMPTY object rather than omitting `status` selectively, so callers + * keep their existing `JSON.stringify(defaultFields)` shape and no caller has to + * know which arm it landed on. + */ +export function createDefaultFields(collection: Collection | null | undefined): Record { + const defaults: Record = {}; + if (!collection) return defaults; + + const statusField = parseSchema(collection).fields.find((f) => f.key === 'status'); + const first = statusField?.options?.[0]; + if (first === undefined) return defaults; + + // See the header: a stale `select` option is never an item id, so no + // reference-shaped field can be defaulted from one. + if (REFERENCE_TYPES.has(statusField?.type ?? '')) return defaults; + + const write = laneWriteValue(statusField, first); + if (write.ok) defaults.status = write.value; + return defaults; +} diff --git a/web/src/lib/components/editor/EditorBubbleMenu.svelte b/web/src/lib/components/editor/EditorBubbleMenu.svelte index b529abb39..0127a06a0 100644 --- a/web/src/lib/components/editor/EditorBubbleMenu.svelte +++ b/web/src/lib/components/editor/EditorBubbleMenu.svelte @@ -1,7 +1,8 @@