diff --git a/.changeset/olive-donkeys-repeat.md b/.changeset/olive-donkeys-repeat.md new file mode 100644 index 0000000000..034859b3bc --- /dev/null +++ b/.changeset/olive-donkeys-repeat.md @@ -0,0 +1,27 @@ +--- +"@object-ui/app-shell": patch +--- + +metadata-admin: name the offending column when `config.columns` is rejected + +`config.columns` is `string[] | ColumnDef[]` — a union with no discriminant — so +Zod reported every rejection as one collapsed issue on the field itself: +`config.columns` / `Invalid input`, on the create gate and the edit gate alike. +The field was reachable, but nothing said which column was wrong, which key, or +what was expected. + +The union member is now chosen by the value's own first element — a list of +field names or a list of column objects — and that member's real diagnostics are +reported at their draft-absolute path. A mis-typed key reports +`config.columns.0.field` with `expected string, received number`; a stray number +in a list of field names reports the element that broke it rather than every +element of the shape the author never chose. The aggregated container reaches +the same union as `list.columns.…`, and both gates now report identically. + +Only unions that really are "an array of A or an array of B" are read this way, +so neighbours such as `config.sort` (`string | ColumnSort[]`) are untouched. +Where the content elects nothing — a first element that is neither a string nor +an object — the previous message is kept rather than guessing. + +Validation verdicts are unchanged: the accept/reject decision is still made by +the one gate, and this only changes how an already-failed draft is presented. diff --git a/packages/app-shell/src/views/metadata-admin/clientValidation.ts b/packages/app-shell/src/views/metadata-admin/clientValidation.ts index c7af252776..1d7c121166 100644 --- a/packages/app-shell/src/views/metadata-admin/clientValidation.ts +++ b/packages/app-shell/src/views/metadata-admin/clientValidation.ts @@ -117,12 +117,17 @@ function viewSchemaForDraft(item: ZodLikeSchema, container: ZodLikeSchema): ZodL } /** - * ── Union-failure diagnostics for the `view` EDIT gate (objectui#3606) ── + * ── Union-failure diagnostics for the `view` gates (objectui#3606, #3626) ── * * PRESENTATION ONLY. Nothing below can change a verdict: it runs strictly * inside the final issue→`SchemaFormIssue` mapping, after `ok` has already been - * decided by the one gate (`ViewMetadataSchema`) and after every issue filter - * has run. Same input set, same `ok`; only the rendered `path`/`message` move. + * decided by the one gate (`ViewMetadataSchema` on edit, the authoring schemas + * on create) and after every issue filter has run. Same input set, same `ok`; + * only the rendered `path`/`message` move. + * + * Two rules live here, one per union depth. This first block is the ROOT rule + * (#3606, edit gate only — neither authoring schema has a union at its root); + * the NESTED rule for `config.columns` follows below (#3626, both gates). * * The edit gate is `z.preprocess(stripViewConsoleDecorations, z.union([…]))`. * Zod reports a union failure as a SINGLE root issue — `code: 'invalid_union'`, @@ -173,26 +178,160 @@ function viewUnionMemberIndex(draft: unknown): number { } /** - * Expand a ROOT union failure into the selected member's own issues, or return - * `null` to say "nothing better to show" — in which case the caller renders the - * root issue unchanged, exactly as before. It can never return an empty list, - * so a rejected draft always renders at least one issue. + * ── NESTED unions: the array-variant rule (objectui#3626) ── + * + * #3606 expanded the union at the ROOT only. `config.columns` is a union too — + * `string[] | ColumnDef[]`, no discriminant — and it collapsed the same way, on + * BOTH gates: create reported `config.columns` / `Invalid input` as a top-level + * issue, edit reported it one level down inside the selected root member. The + * user was taken to the right field and told nothing about it: not which + * column, not which key, not what was expected. + * + * The discriminant here is the value's OWN CONTENT, which is the same class of + * rule as the root's `viewKind` — a fact about what the author wrote, not a + * comparison across error groups. A `columns` array is a list of field NAMES or + * a list of column OBJECTS, and its first element says which. ("Fewest issues / + * deepest path" remains banned: it ranks the groups against each other, and + * #3606 measured it picking wrong.) + * + * Applying that naively to every nested union would mis-select, so the rule is + * narrowed to unions that really are "an array of A or an array of B". Measured + * over the whole `view` family (@objectstack/spec 17.0.0-rc.5), 16 nested + * discriminant-less unions exist and `columns` is the only one whose members + * are BOTH arrays. The neighbour that would break a naive rule is `config.sort` + * (`string | ColumnSort[]`): for `sort: ['name']` the first element is a string, + * so first-element-typeof alone would select the plain-`string` member and + * report "expected string, received array" — technically true, and the wrong + * thing to say to someone who correctly wrote an array. So a member that + * rejected the value's TYPE at the union node itself (a bare `invalid_type` at + * its own relative root) is not a candidate: it never looked at the contents, + * so contents cannot be evidence for it. That single categorical test — asked + * of each group on its own, never group-vs-group — is what keeps `sort`, + * `filter[].value`, `gantt.tooltipFields[]` and the rest untouched. * - * Root-only by design: a member issue's `path` is relative to the union node, - * and only at the root is that the same as the draft-absolute path `SchemaForm` - * needs. A union nested deeper (e.g. `config.columns`) keeps Zod's own message; - * it still carries a real path, so it is addressable — unlike the root case. + * Boundaries, all measured rather than assumed: + * + * - `columns: []` is VALID under both members, so the empty array never + * reaches this code — the union succeeds and there is no issue to expand. + * The "what do we do with an empty array" question is structurally moot. + * - `columns: [42]` / `[null]` — the first element names neither variant, so + * nothing is selected and the node keeps Zod's own message. Both members + * reject it identically anyway; picking one would be inventing a preference. + * - `columns: 'nope'` — not an array, nothing to discriminate on. (Both + * members do agree here, but "all members said the same thing so promote it" + * is a different mechanism — issue #3626's direction 1 — not this one.) + * - Mixed arrays are the interesting case and they come out right: for + * `['name', {field: 1}]` the first element elects `string[]`, which reports + * `config.columns.1` — the element that actually broke the list the author + * was writing — instead of the object member's two rejections of the shape + * they never chose. + * + * Like the root rule this indexes members POSITIONALLY and is pinned the same + * way: the CANARY tests assert the exact `path` + `message` of the selected + * member, so a spec-side reorder of the `columns` union's two members goes red + * instead of silently reporting the other variant's complaint. + */ +const ARRAY_VARIANT_MEMBERS = { ofStrings: 0, ofObjects: 1 } as const; + +/** + * Did this member reject the value's TYPE at the union node itself, without + * ever looking at its contents? Asked of one group in isolation — it filters + * which unions the content rule may speak about, it does not pick a winner. */ -function expandViewUnionIssue(issue: ZodLikeIssue, draft: unknown): ZodLikeIssue[] | null { +function memberRejectedNodeType(group: ZodLikeIssue[]): boolean { + return group.some((i) => i.code === 'invalid_type' && (i.path ?? []).length === 0); +} + +/** Read the draft value a union node's absolute path addresses, or `undefined`. */ +function valueAtPath(draft: unknown, path: Array): unknown { + let cursor: unknown = draft; + for (const seg of path) { + if (cursor === null || typeof cursor !== 'object') return undefined; + cursor = (cursor as Record)[seg]; + } + return cursor; +} + +/** + * The `array | array` rule described above. Returns the member index the + * value's first element elects, or `null` when this union is not of that shape + * or the content elects nothing. + * + * The value is read from the ORIGINAL draft while the paths come from a parse + * of the PREPROCESSED one. That is exact for this purpose, measured rather than + * assumed: `stripViewConsoleDecorations` only deletes `id` keys from `filter` / + * `sort` rows — it maps arrays element-wise and never reorders or drops one, so + * no index a path carries can shift. + */ +function arrayVariantMemberIndex(groups: ZodLikeIssue[][], value: unknown): number | null { + if (groups.length !== 2) return null; + if (!Array.isArray(value) || value.length === 0) return null; + if (groups.some(memberRejectedNodeType)) return null; + const first = value[0]; + if (typeof first === 'string') return ARRAY_VARIANT_MEMBERS.ofStrings; + if (first !== null && typeof first === 'object' && !Array.isArray(first)) + return ARRAY_VARIANT_MEMBERS.ofObjects; + return null; +} + +/** + * Pick the union member whose issues should be shown for one `invalid_union`, + * or `null` for "nothing better than the union node's own message". + * + * `absPath` is the union node's DRAFT-ABSOLUTE path — the root union's is `[]`, + * which is what selects between the two rules. A member issue's own `path` is + * relative to its union node, which is why the caller composes prefixes on the + * way down instead of trusting `issue.path` to be absolute below the root. + */ +function selectViewUnionGroup( + issue: ZodLikeIssue, + absPath: Array, + draft: unknown, +): ZodLikeIssue[] | null { if (issue.code !== 'invalid_union') return null; - if ((issue.path ?? []).length !== 0) return null; const groups = issue.errors; if (!Array.isArray(groups)) return null; - const group = groups[viewUnionMemberIndex(draft)]; + const index = + absPath.length === 0 + ? viewUnionMemberIndex(draft) + : arrayVariantMemberIndex(groups, valueAtPath(draft, absPath)); + if (index === null) return null; + const group = groups[index]; if (!Array.isArray(group) || group.length === 0) return null; return group; } +/** + * Rewrite a `view` gate's issues into draft-absolute, field-addressed ones. + * + * Every issue is emitted with `prefix ++ issue.path`; a union whose member the + * rules above can name is replaced by that member's issues, recursively, with + * the union node's own path becoming their prefix. A union no rule can speak + * for is emitted unchanged — which is the pre-#3606 behaviour, so nothing can + * be lost. Each expansion descends into strictly-contained sub-issues of a + * finite tree and only ever yields a non-empty group, so this terminates and a + * rejected draft always renders at least one issue. + * + * There is no depth counter: the array-variant rule declines every union whose + * node value is not an array, which is what actually bounds the descent. In the + * measured schema that is exactly one nested level — e.g. a bad + * `columns[0].summary` (`enum | {type, field}`, an object value) stops there and + * keeps its own message, now correctly addressed to `config.columns.0.summary` + * rather than to `config.columns`. + */ +function expandViewIssues( + issues: ZodLikeIssue[], + prefix: Array, + draft: unknown, +): ZodLikeIssue[] { + return issues.flatMap((issue) => { + const absPath = [...prefix, ...(issue.path ?? [])]; + const group = selectViewUnionGroup(issue, absPath, draft); + if (!group) return [{ ...issue, path: absPath }]; + return expandViewIssues(group, absPath, draft); + }); +} + // Map metadata-type name → loader for that type's root Zod schema. // Each loader pulls only one spec subpath so we don't drag the whole // 2MB schema bundle into the studio bundle. @@ -517,18 +656,16 @@ export async function validateMetadataDraft( } if (rawIssues.length === 0 && celIssues.length === 0) return { ok: true, issues: [] }; - // Presentation only — see `expandViewUnionIssue`. `ok` is already `false` - // here and `rawIssues` is already final; this only decides what gets RENDERED - // for each of them, so the verdict cannot move (pinned by the parity test in + // Presentation only — see `expandViewIssues`. `ok` is already `false` here + // and `rawIssues` is already final; this only decides what gets RENDERED for + // each of them, so the verdict cannot move (pinned by the parity test in // `clientValidation.viewDiagnostics.test.ts`). + const renderable = type === 'view' ? expandViewIssues(rawIssues, [], draft) : rawIssues; const issues: SchemaFormIssue[] = [ - ...rawIssues.flatMap((i) => { - const expanded = type === 'view' ? expandViewUnionIssue(i, draft) : null; - return (expanded ?? [i]).map((issue) => ({ - path: (issue.path ?? []).map((seg) => String(seg)).join('.'), - message: issue.message, - })); - }), + ...renderable.map((issue) => ({ + path: (issue.path ?? []).map((seg) => String(seg)).join('.'), + message: issue.message, + })), ...celIssues, ]; return { ok: false, issues }; diff --git a/packages/app-shell/src/views/metadata-admin/clientValidation.viewDiagnostics.test.ts b/packages/app-shell/src/views/metadata-admin/clientValidation.viewDiagnostics.test.ts index 0465db7442..e414e2d288 100644 --- a/packages/app-shell/src/views/metadata-admin/clientValidation.viewDiagnostics.test.ts +++ b/packages/app-shell/src/views/metadata-admin/clientValidation.viewDiagnostics.test.ts @@ -287,3 +287,224 @@ describe('view verdict parity — presentation moved, judgement did not (objectu }); } }); + +/** + * ── NESTED union diagnostics: `config.columns` (objectui#3626) ── + * + * #3606 expanded the ROOT union only, and said so: `config.columns` is + * `string[] | ColumnDef[]` with no discriminant, and it stayed collapsed. It + * collapsed on BOTH gates, and had done on the create gate since long before + * #3606 — measured on @objectstack/spec 17.0.0-rc.5, before this change: + * + * create (`ViewItemSchema`) → path `config.columns` msg `Invalid input` + * edit (`ViewMetadataSchema`) → path `config.columns` msg `Invalid input` + * + * The path was real, so the field was reachable; the message said nothing about + * WHICH column or WHAT was expected. The rule that fixes it selects the union + * member the value's own first element elects — a fact about what the author + * wrote, the same class of rule as the root's `viewKind`, and not a ranking of + * the error groups against each other. + * + * Three kinds of claim are pinned below, and they fail for different reasons: + * + * - CANARY — the elected member's exact `path` + `message`. Members are + * indexed positionally, so a spec-side reorder of the `columns` union turns + * these red instead of quietly reporting the other variant's complaint. + * - NARROWING — the unions this rule must NOT speak for. `config.sort` + * (`string | ColumnSort[]`) is the live counter-example: drop the guard that + * ignores members which rejected the node's type outright, and a bare + * first-element test starts answering for it. These pin today's untouched + * output, so that regression is loud. + * - CONVERGENCE — create and edit report the SAME thing. #3626 was filed on + * the observation that PR #3624 made the two gates agree on a bad message; + * they have to keep agreeing on the good one. + */ +describe('view nested union — `config.columns` diagnostics (objectui#3626)', () => { + // `STORED_ITEM` carries `isPinned`, which the AUTHORING gate rejects on + // purpose (it is Studio state the console writes, pinned by "still rejects + // platform-written keys on the authoring surface" above). Comparing the two + // gates issue-for-issue needs a body whose only defect is the one under test, + // so these cases author it themselves — same record, minus the console's pin. + const AUTHORABLE_ITEM: Record = { ...STORED_ITEM }; + delete AUTHORABLE_ITEM.isPinned; + + const withColumns = (columns: unknown) => ({ + ...AUTHORABLE_ITEM, + config: { ...(AUTHORABLE_ITEM.config as Record), columns }, + }); + + /** Every case here must read identically through both gates. */ + const bothGates = async (body: unknown) => { + const created = await validateMetadataDraft('view', body, undefined, { mode: 'create' }); + const edited = await validateMetadataDraft('view', body, undefined, EDIT); + expect(created.ok).toBe(false); + expect(edited.ok).toBe(false); + // CONVERGENCE — the create gate reaches this union as a top-level issue, + // the edit gate reaches it one level down inside the root member it + // selected. Both compose to the same draft-absolute path. + expect(edited.issues).toEqual(created.issues); + return created.issues; + }; + + // ── CANARY ─────────────────────────────────────────────────────────────── + + it('CANARY: a mis-typed key on a column object is addressed to that key', async () => { + // Before: `config.columns` / `Invalid input` (both gates). + const issues = await bothGates(withColumns([{ field: 123 }])); + expect(issues).toHaveLength(1); + expect(issues[0].path).toBe('config.columns.0.field'); + expect(issues[0].message).toContain('expected string, received number'); + }); + + it('CANARY: a column object missing its required `field` names the missing key', async () => { + const issues = await bothGates(withColumns([{}])); + expect(issues).toHaveLength(1); + expect(issues[0].path).toBe('config.columns.0.field'); + expect(issues[0].message).toContain('expected string, received undefined'); + }); + + it('CANARY: a stray non-string in a field-NAME list is addressed to that element', async () => { + // First element is a string, so the author is writing `string[]`; the + // element that broke it is index 2. The object member's THREE rejections of + // a shape they never chose are not shown — that is the noise control. + const issues = await bothGates(withColumns(['a', 'b', 42])); + expect(issues).toHaveLength(1); + expect(issues[0].path).toBe('config.columns.2'); + expect(issues[0].message).toContain('expected string, received number'); + }); + + it('CANARY: a mixed list is judged by the variant its FIRST element elects', async () => { + // Object first → this is a `ColumnDef[]`, and element 1 is the odd one out. + const issues = await bothGates(withColumns([{ field: 'a' }, 'b'])); + expect(issues).toHaveLength(1); + expect(issues[0].path).toBe('config.columns.1'); + expect(issues[0].message).toContain('expected object, received string'); + }); + + it('CANARY: the same union under the aggregated container reports `list.columns.…`', async () => { + // The container reaches the identical union by a different route: top-level + // on create, under root member [1] on edit. Prefix composition is the thing + // under test — a member issue's path is relative to its own union node. + const issues = await bothGates({ + ...CONTAINER, + list: { type: 'grid', columns: [{ field: 123 }] }, + }); + expect(issues).toHaveLength(1); + expect(issues[0].path).toBe('list.columns.0.field'); + expect(issues[0].message).toContain('expected string, received number'); + }); + + it('shows ONLY the elected variant — never both members’ complaints', async () => { + const issues = await bothGates(withColumns(['a', 'b', 42])); + const messages = issues.map((i) => i.message).join('\n'); + // Positive anchor FIRST — without it the negative below passes vacuously + // against a collapsed `Invalid input`, which contains no forbidden + // substring either (the #3606 lesson, same trap). + expect(messages).toContain('expected string, received number'); + expect(messages).not.toContain('expected object, received string'); + }); + + // ── NARROWING: unions this rule must stay out of ───────────────────────── + + it('does NOT answer for `config.sort` — one member never accepted the array', async () => { + // `sort` is `string | ColumnSort[]`. For `['name']` a bare first-element + // test would elect the plain-`string` member and report "expected string, + // received array" — true, and the wrong thing to tell someone who correctly + // wrote an array. A member that rejected the node's TYPE outright never + // looked at the contents, so the contents are not evidence for it, and the + // union is left alone. Remove that guard and this goes red. + const issues = await bothGates({ + ...AUTHORABLE_ITEM, + config: { ...(AUTHORABLE_ITEM.config as Record), sort: ['name'] }, + }); + expect(issues).toEqual([{ path: 'config.sort', message: 'Invalid input' }]); + }); + + it('does NOT answer for a filter value union (five members, not two)', async () => { + const issues = await bothGates({ + ...AUTHORABLE_ITEM, + config: { + ...(AUTHORABLE_ITEM.config as Record), + filter: [{ field: 'f', operator: 'in', value: [{}] }], + }, + }); + expect(issues).toEqual([{ path: 'config.filter.0.value', message: 'Invalid input' }]); + }); + + // ── Boundaries: what the content cannot elect ──────────────────────────── + + it('an EMPTY `columns` is VALID — the undiscriminable case never arises', async () => { + // Measured, not assumed: `[]` satisfies BOTH members, so the union succeeds + // and there is no failure to expand. "What do we show for an empty array" + // has no answer because it has no question. + const created = await validateMetadataDraft('view', withColumns([]), undefined, { + mode: 'create', + }); + const edited = await validateMetadataDraft('view', withColumns([]), undefined, EDIT); + expect(created.ok).toBe(true); + expect(edited.ok).toBe(true); + }); + + it('a first element that elects NEITHER variant keeps the union’s own message', async () => { + // `42` is neither a field name nor a column object; both members reject it + // identically. Electing one would be inventing a preference the author + // never expressed, so this stays exactly as it was — still addressable at + // `config.columns`, which is what made #3626 milder than #3606. + expect(await bothGates(withColumns([42]))).toEqual([ + { path: 'config.columns', message: 'Invalid input' }, + ]); + expect(await bothGates(withColumns([null]))).toEqual([ + { path: 'config.columns', message: 'Invalid input' }, + ]); + }); + + it('a `columns` that is not an array at all keeps the union’s own message', async () => { + // Both members do agree here ("expected array, received string"), but + // promoting a message because all members happen to share it is a DIFFERENT + // mechanism (#3626's direction 1), deliberately not built here. + expect(await bothGates(withColumns('nope'))).toEqual([ + { path: 'config.columns', message: 'Invalid input' }, + ]); + }); + + it('stops at the next union down, but addressed to it rather than to `columns`', async () => { + // `columns[0].summary` is `enum | {type, field}` — an OBJECT value, so the + // array rule declines it and it keeps Zod's message. That is the descent + // bound: not a depth counter, but a rule that has nothing to say here. + // Before this change the whole thing collapsed to `config.columns`. + const issues = await bothGates(withColumns([{ field: 'a', summary: { type: 'bogus' } }])); + expect(issues).toEqual([{ path: 'config.columns.0.summary', message: 'Invalid input' }]); + }); +}); + +/** + * PARITY, nested layer — same claim as the block above, same reason it is green + * in both directions: the expansion runs inside the issue→form-issue mapping, + * downstream of `ok`. Listed separately because these bodies are the ones + * #3626 moves, so they are the ones worth re-pinning. + */ +describe('view verdict parity — `columns` bodies (objectui#3626)', () => { + const withColumns = (columns: unknown) => ({ + ...STORED_ITEM, + config: { ...(STORED_ITEM.config as Record), columns }, + }); + + const CASES: Array<{ label: string; body: unknown; ok: boolean }> = [ + { label: 'empty columns', body: withColumns([]), ok: true }, + { label: 'field-name list', body: withColumns(['name', 'amount']), ok: true }, + { label: 'column-def list', body: withColumns([{ field: 'name' }]), ok: true }, + { label: 'column def with a bad key type', body: withColumns([{ field: 123 }]), ok: false }, + { label: 'field-name list with a stray number', body: withColumns(['a', 42]), ok: false }, + { label: 'mixed list', body: withColumns([{ field: 'a' }, 'b']), ok: false }, + { label: 'columns elected by nothing', body: withColumns([42]), ok: false }, + { label: 'columns not an array', body: withColumns('nope'), ok: false }, + ]; + + for (const c of CASES) { + it(`${c.label}: edit=${c.ok ? 'ok' : 'not ok'}`, async () => { + const edited = await validateMetadataDraft('view', c.body, undefined, EDIT); + expect(edited.ok, `edit: ${JSON.stringify(edited.issues)}`).toBe(c.ok); + expect(edited.issues.length === 0).toBe(c.ok); + }); + } +});