From a60f1104a0cb89e87aac49bcebf1875b240dd985 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 31 Jul 2026 07:41:33 +0000 Subject: [PATCH 1/2] feat(lint): every field-bearing prop on a React page block resolves against the object it names (#4340) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #4329 closed ONE of them — — by running the metadata rule's core from the gate that owns React block props. That prop was an instance, not the class: every other prop a kind:'react' page binds BY FIELD NAME shipped exactly as typed, the same silent drift page-field-unknown already closes for the page-component `properties` bag one surface over. validate-react-page-props now resolves all of them: ListView fields/columns/sort/grouping/userFilters/hiddenFields/fieldOrder/filterableFields, ObjectForm fields + initialValues KEYS + sections[].fields[] + subforms (each entry against its own childObject, totalField against the form's), and the record:* blocks via the SAME COMPONENT_FIELD_SPECS table the metadata surface uses, keyed by the block's schemaType — so the two surfaces agree by construction rather than by two lists that happen to match. reaches that table by the type the author writes, so the escape hatch is checked instead of being a hole. Findings carry page-field-unknown at its advisory severity, because the consumer behaves the same way. A FILTER position gates instead. / name fields in a QUERY: an unknown column there is not a skipped column, the predicate can never match, SqlDriver swallows "no such column" and returns [], and the surface renders an empty list indistinguishable from "there is no data" — the silent zero filter-token-unknown and validate-flow-template-paths' own filter-position call both gate on. Filter positions are also resolved INDEPENDENTLY of one another, unlike every other value this gate reads: filters={['status','=',stage]} is the shape a react page actually writes, and the all-or-nothing static reader skipped the whole array including the one position that was knowable. is settled as the RELATED (child) object, which is what the spec schema always said, what record:related_list means on every metadata surface, and what validate-page-field-bindings already resolves its columns against. The React overlay had declared objectName a second time and glossed it "The parent object"; the generated contract publishes the overlay's description in place of the schema's, so the react surface both contradicted the spec and lost any way to name the object it renders — and the one live page authored against the gloss listed the wrong object. The class is closed too: REACT_OVERLAY_SHADOWS ledgers every overlay prop that restates a spec-schema prop, and a test asserts the ledger equals the real collision set. Also: the shared sort reader judges the LEGACY bare-string form ("amount desc") by its head instead of reporting the whole string as an unknown field — a finding no author could act on, removed from both surfaces at once. Verified against the live corpus: `os validate` is clean on app-showcase and app-crm, and re-introducing the old objectName spelling makes the new rule report both stale positions. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_012rK2McPogrTJBKdUsThrGs --- .changeset/react-block-field-props-lint.md | 78 ++++ .../src/ui/pages/renewals-pipeline.page.ts | 11 +- .../src/validate-page-field-bindings.test.ts | 27 ++ .../lint/src/validate-page-field-bindings.ts | 295 ++++++++---- .../src/validate-react-page-props.test.ts | 418 ++++++++++++++++++ .../lint/src/validate-react-page-props.ts | 393 ++++++++++++++-- packages/spec/api-surface.json | 1 + packages/spec/src/ui/react-blocks.test.ts | 89 ++++ packages/spec/src/ui/react-blocks.ts | 57 ++- .../contracts/react-blocks.contract.json | 8 +- .../objectstack-ui/references/react-blocks.md | 6 +- 11 files changed, 1249 insertions(+), 134 deletions(-) create mode 100644 .changeset/react-block-field-props-lint.md create mode 100644 packages/spec/src/ui/react-blocks.test.ts diff --git a/.changeset/react-block-field-props-lint.md b/.changeset/react-block-field-props-lint.md new file mode 100644 index 0000000000..36d9d86758 --- /dev/null +++ b/.changeset/react-block-field-props-lint.md @@ -0,0 +1,78 @@ +--- +"@objectstack/lint": minor +"@objectstack/spec": minor +--- + +feat(lint): every field-bearing prop on a React page block resolves against the +object it names + +#4329 closed ONE of them — `` — by running the +metadata rule's core from the gate that owns React block props. That prop was an +instance, not the class: every other prop a `kind:'react'` page binds BY FIELD +NAME shipped exactly as typed, the same silent drift `page-field-unknown` +already closes for the page-component `properties` bag one surface over. + +`validate-react-page-props` now resolves all of them: + +- `` `fields` / `columns` / `sort` / `grouping` / `userFilters` / + `hiddenFields` / `fieldOrder` / `filterableFields` +- `` `fields`, `initialValues` KEYS, `sections[].fields[]` +- `` / `` / `` / + `` — via the SAME `COMPONENT_FIELD_SPECS` table the + metadata surface uses, keyed by the block's `schemaType`, so the two surfaces + agree by construction rather than by two lists that happen to match +- `` — the escape hatch reaches the same table by the type the + author writes, so it is checked instead of being a hole + +Findings carry the metadata rule's id (`page-field-unknown`) at its advisory +severity, because the consumer behaves the same way: an unknown name is skipped +and the rest renders. + +**A FILTER position gates instead.** `` / `` name fields in a QUERY, and an unknown column there is not a skipped +column: the predicate can never match, `SqlDriver` swallows the driver's +"no such column" and returns `[]`, and the surface renders an empty list that +looks exactly like "there is no data" — the silent zero `filter-token-unknown` +and `validate-flow-template-paths`' filter-position call both gate on. Those +are reported as `error`. + +Filter positions are also resolved INDEPENDENTLY of each other, unlike every +other value this gate reads. `filters={['status', '=', stage]}` — a static field +beside a React-state value — is the shape a react page actually writes, and the +all-or-nothing static reader skipped the whole array, including the one position +that was knowable. + +Everything else is unchanged: a value from a variable, a call, or behind a +spread is unresolvable rather than wrong and is skipped silently (ADR-0072 D1), +as are cross-package objects, objects with no authored field map, dotted +relationship paths, and registry-injected system columns. + +### Breaking: `` is the RELATED object, as the spec always said + +`RecordRelatedListProps.objectName` is the related (child) object — that is what +`record:related_list` means on every metadata surface, what +`validate-page-field-bindings` resolves its `columns` against, and what the one +registry component behind both surfaces consumes. The React overlay declared +`objectName` a SECOND time and glossed it "The parent object", and the generated +contract publishes the overlay's description in place of the schema's — so the +react surface both contradicted the spec and lost any way to name the object it +renders. + +FROM → TO for a page authored against the old gloss: + +```diff +- ++ +``` + +`objectName` names the CHILD object being listed; the parent record stays bound +by `recordId`, and `relationshipField` is the child's field pointing back at it. +The lint above reports the old spelling (the child's columns and its FK do not +resolve against the parent). `objectName` is now also published as required, as +the schema declares it. + +The class is closed as well as the instance: `REACT_OVERLAY_SHADOWS` in +`@objectstack/spec/ui` ledgers every overlay prop that restates a spec-schema +prop, and a test asserts the ledger equals the real collision set — so the next +overlay entry that silently redefines a schema prop fails a test instead of +shipping a second dialect. diff --git a/examples/app-showcase/src/ui/pages/renewals-pipeline.page.ts b/examples/app-showcase/src/ui/pages/renewals-pipeline.page.ts index 2335986aee..aec955a207 100644 --- a/examples/app-showcase/src/ui/pages/renewals-pipeline.page.ts +++ b/examples/app-showcase/src/ui/pages/renewals-pipeline.page.ts @@ -24,6 +24,15 @@ import { definePage } from '@objectstack/spec/ui'; * `groupBy`) and `total` (its `field`) — not by a dataset-style measure name. * `os validate` checks both halves. * + * `` binds the CHILD object it lists (`showcase_invoice`), + * not the parent — the parent is `recordId`, and `relationshipField="account"` + * is the invoice's lookup back to it. This page used to pass the parent, which + * is what #4340 found: the react contract had glossed `objectName` as "the + * parent object" while the schema (and the renderer behind both surfaces) read + * it as the related one, so the list resolved `total` against an account and + * came back empty. Every field-bearing prop on the page is now checked against + * the object it actually names. + * * Styling (ADR-0065): no Tailwind — inline `style={{}}` with `hsl(var(--token))`; * data blocks and the drawer bring their own compiled styling. The drawer sets * NO pixel width: per #2578 pixel widths are deprecated (the author can't know @@ -122,7 +131,7 @@ function Page() { - + {editing ? ( { expect(findings).toEqual([]); }); }); + +describe('validatePageFieldBindings — legacy bare-string sort (#4340)', () => { + /** + * `ListViewSchema.sort` still accepts `"created_at desc"`. Reading the whole + * string as a field name reported `"amount desc"` as unknown — a finding + * whose "field" the author never wrote, and one no fix could satisfy. The + * shared `sortFieldRefs` reads its head instead, the way the renderer does. + */ + it('judges the head segment, not the whole "field desc" string', () => { + const listPage = (sort: unknown) => ({ + ...baseStack(), + pages: [ + { + name: 'list_page', + type: 'list', + object: 'crm_lead', + interfaceConfig: { source: 'crm_lead', columns: ['name'], sort }, + }, + ], + }); + expect(validatePageFieldBindings(listPage('amount desc'))).toEqual([]); + const bad = validatePageFieldBindings(listPage('ghost_col desc')); + expect(bad).toHaveLength(1); + expect(bad[0].message).toContain('"ghost_col"'); + expect(bad[0].message).not.toContain('ghost_col desc'); + }); +}); diff --git a/packages/lint/src/validate-page-field-bindings.ts b/packages/lint/src/validate-page-field-bindings.ts index 4bc236fbfe..d485073dd2 100644 --- a/packages/lint/src/validate-page-field-bindings.ts +++ b/packages/lint/src/validate-page-field-bindings.ts @@ -37,6 +37,18 @@ * `record:details` `sections[].fields[]` and `hideFields[]`, and the record * picker's `labelField`. Linting the schema's shape alone would find nothing on * the actual corpus. + * + * ── Shared with the react page surface ────────────────────────────────── + * + * A `kind:'react'` page authors the SAME components, one surface over, as JSX + * props instead of a `properties` bag (``). The + * extraction and the check are therefore exported — `COMPONENT_FIELD_SPECS`, + * {@link componentFieldRefs}, {@link relatedListFieldRefs}, + * {@link indexObjectFields}, {@link checkFieldRefs} — and + * `validate-react-page-props` runs them on the parsed JSX (#4340). Same table, + * same skips, same rule id: the two surfaces agree on what counts as a field by + * construction rather than by two lists that happen to match, which is the + * drift #4330 had just finished removing from the system-field lists. */ export const PAGE_FIELD_UNKNOWN = 'page-field-unknown'; @@ -44,7 +56,12 @@ export const PAGE_FIELD_UNKNOWN = 'page-field-unknown'; export type PageFieldSeverity = 'error' | 'warning'; export interface PageFieldFinding { - /** Always `warning` — page renderers skip an unknown field rather than fail. */ + /** + * `warning` on every surface this rule itself walks — page renderers skip an + * unknown field rather than fail. The shared {@link checkFieldRefs} core also + * serves the react page surface, where one batch of refs reaches a QUERY + * rather than a renderer and gates instead; see {@link FieldRefConsequence}. + */ severity: PageFieldSeverity; /** Diagnostic rule id. */ rule: string; @@ -81,7 +98,7 @@ function isRec(v: unknown): v is AnyRec { } /** A field reference found in a props bag, with the path that located it. */ -interface FieldRef { +export interface FieldRef { name: string; path: string; } @@ -92,7 +109,7 @@ interface FieldRef { * use interchangeably (`record:highlights` keys its object form `name`, while * columns/sort/filter key theirs `field`). */ -function fieldRefsFrom(value: unknown, basePath: string): FieldRef[] { +export function fieldRefsFrom(value: unknown, basePath: string): FieldRef[] { const out: FieldRef[] = []; const one = (v: unknown, path: string) => { const bare = strName(v); @@ -112,6 +129,24 @@ function fieldRefsFrom(value: unknown, basePath: string): FieldRef[] { return out; } +/** + * Field references in a `sort` value. + * + * The structured form (`[{ field, order }]`) is ordinary {@link fieldRefsFrom} + * work. The LEGACY bare-string form is not: `ListViewSchema.sort` still accepts + * `"created_at desc"`, where the string names ONE field followed by a direction + * word — so reading the whole string as a field name reports `"created_at desc"` + * as unknown, a finding whose "field" the author never wrote. Read its head + * instead, which is exactly what the renderer does with it. + */ +export function sortFieldRefs(value: unknown, basePath: string): FieldRef[] { + if (typeof value === 'string') { + const head = value.trim().split(/\s+/)[0]; + return head ? [{ name: head, path: basePath }] : []; + } + return fieldRefsFrom(value, basePath); +} + /** * Per-component-type descriptor: which `properties` paths hold field names, and * whether they resolve against this component's own object or another one. @@ -120,14 +155,14 @@ function fieldRefsFrom(value: unknown, basePath: string): FieldRef[] { * `nestedSections` walk `properties.[].fields[]` — the section shape real * pages author for `record:details`. */ -interface ComponentFieldSpec { +export interface ComponentFieldSpec { /** Props holding field names bound to the component's resolved object. */ props?: readonly string[]; /** Props holding `{...}[]` section objects whose `fields[]` are field names. */ nestedSections?: readonly string[]; } -const COMPONENT_FIELD_SPECS: Readonly> = { +export const COMPONENT_FIELD_SPECS: Readonly> = { 'record:highlights': { props: ['fields'] }, // `sections`/`hideFields` are not in RecordDetailsProps, but every real page // authors them (they survive because `properties` is unvalidated). @@ -145,15 +180,95 @@ const COMPONENT_FIELD_SPECS: Readonly> = { * against the RELATED object (`properties.objectName`), not the page's object, * so it cannot ride the generic table. */ -const RELATED_LIST_TYPE = 'record:related_list'; +export const RELATED_LIST_TYPE = 'record:related_list'; -export function validatePageFieldBindings(stack: AnyRec): PageFieldFinding[] { - const findings: PageFieldFinding[] = []; - if (!stack || typeof stack !== 'object') return findings; +/** + * The field references a component's props bag holds, per + * {@link COMPONENT_FIELD_SPECS}. `null` for a type with no descriptor — + * unregistered / non-field components are skipped silently, never flagged. + * + * `basePath` is the path of the props bag itself, and `sep` joins a prop name + * onto it: `.` for a metadata page (`…components[0].properties.fields[2]`), + * ` › ` for react source, whose props live inside one opaque `source` string + * and so are addressed the way #4329 established (`pages[0].source › fields[2]`). + */ +export function componentFieldRefs( + type: string, + props: AnyRec, + basePath: string, + sep = '.', +): FieldRef[] | null { + const spec = COMPONENT_FIELD_SPECS[type]; + if (!spec) return null; + const refs: FieldRef[] = []; + for (const key of spec.props ?? []) { + refs.push(...fieldRefsFrom(props[key], `${basePath}${sep}${key}`)); + } + for (const key of spec.nestedSections ?? []) { + const sections = Array.isArray(props[key]) ? (props[key] as unknown[]) : []; + for (let si = 0; si < sections.length; si++) { + const section = sections[si]; + // A `sections` that is a plain `string[]` (the shape `RecordDetailsProps` + // actually declares — section IDs) yields nothing here, which is right: + // those are not field names. + if (!isRec(section)) continue; + refs.push(...fieldRefsFrom(section.fields, `${basePath}${sep}${key}[${si}].fields`)); + } + } + return refs; +} - // object name → its declared field names. Built with `asArray` so BOTH - // `fields` shapes (array of `{name}` and name-keyed map) resolve. +/** A `record:related_list` props bag, split by which object each batch resolves against. */ +export interface RelatedListFieldRefs { + /** The related (child) object this list renders — `properties.objectName`. */ + relatedObject: string | undefined; + /** Refs resolved against {@link relatedObject}. */ + related: FieldRef[]; + /** Refs resolved against the PARENT object (the record the list hangs off). */ + parent: FieldRef[]; + /** The Add picker's own object, and the refs resolved against it. */ + pickerObject: string | undefined; + picker: FieldRef[]; +} + +/** + * Split a `record:related_list` props bag into its three object scopes. Shared + * so the react `` block and the metadata component cannot + * disagree about which object each prop addresses — the exact confusion #4340 + * found published under one prop name. + */ +export function relatedListFieldRefs( + props: AnyRec, + basePath: string, + sep = '.', +): RelatedListFieldRefs { + const add = isRec(props.add) ? props.add : undefined; + const picker = add && isRec(add.picker) ? add.picker : undefined; + const at = (key: string) => `${basePath}${sep}${key}`; + return { + relatedObject: strName(props.objectName), + related: [ + ...fieldRefsFrom(props.columns, at('columns')), + ...sortFieldRefs(props.sort, at('sort')), + ...fieldRefsFrom(props.filter, at('filter')), + ...fieldRefsFrom(props.relationshipField, at('relationshipField')), + ...(add ? fieldRefsFrom(add.linkField, at('add.linkField')) : []), + ], + parent: fieldRefsFrom(props.relationshipValueField, at('relationshipValueField')), + pickerObject: picker ? strName(picker.object) : undefined, + picker: picker + ? [ + ...fieldRefsFrom(picker.valueField, at('add.picker.valueField')), + ...fieldRefsFrom(picker.labelField, at('add.picker.labelField')), + ] + : [], + }; +} + +/** object name → its declared field names. Both `fields` shapes resolve. */ +export function indexObjectFields(stack: AnyRec): Map> { const objectFields = new Map>(); + if (!isRec(stack)) return objectFields; for (const obj of asArray(stack.objects)) { const name = strName(obj.name); if (!name) continue; @@ -164,6 +279,74 @@ export function validatePageFieldBindings(stack: AnyRec): PageFieldFinding[] { } objectFields.set(name, names); } + return objectFields; +} + +/** + * How a miss on this batch of refs fails at runtime — the two calls this + * package makes, named so the message and the severity cannot drift apart. + * + * - `skipped` (default): the consumer drops the unknown name and renders the + * rest. Advisory, like every other field-existence rule. + * - `queried`: the name reached a QUERY. An unknown column in a predicate + * matches no row (`SqlDriver` swallows the driver's "no such column" and + * returns `[]`), so the surface renders an empty list that is + * indistinguishable from "there is no data" — the silent-zero failure + * `filter-token-unknown` and `validate-flow-template-paths`' filter-position + * call both gate on. Gating. + */ +export type FieldRefConsequence = 'skipped' | 'queried'; + +/** + * Check one batch of refs against `objectName`'s declared fields. + * + * Bails out entirely when the object is not defined in this stack — it may come + * from another installed package, and we cannot judge fields on a schema we + * cannot see (the same skip the flow/widget rules use). + */ +export function checkFieldRefs( + refs: readonly FieldRef[], + objectName: string | undefined, + objectFields: ReadonlyMap>, + where: string, + consequence: FieldRefConsequence = 'skipped', +): PageFieldFinding[] { + const findings: PageFieldFinding[] = []; + if (!objectName) return findings; // nothing to resolve against + const known = objectFields.get(objectName); + if (!known) return findings; // cross-package object — unknowable here + for (const ref of refs) { + // A relationship path (`account.name`) is resolved by the query engine, + // not a base column, so it cannot be judged here. + if (ref.name.includes('.')) continue; + if (known.has(ref.name) || SYSTEM_FIELDS.has(ref.name)) continue; + findings.push({ + severity: consequence === 'queried' ? 'error' : 'warning', + rule: PAGE_FIELD_UNKNOWN, + where, + path: ref.path, + message: + `field "${ref.name}" is not a field on object "${objectName}" — ` + + (consequence === 'queried' + ? 'it is used in a QUERY, so the predicate can never match: the surface ' + + 'renders an empty result that looks exactly like "there is no data".' + : 'the component silently skips it, so it never renders.'), + hint: + `Fix the field name, or add "${ref.name}" to ${objectName}. ` + + `References must match the object's field names exactly.` + + (known.size > 0 ? ` Object fields: ${[...known].sort().join(', ')}.` : ''), + }); + } + return findings; +} + +export function validatePageFieldBindings(stack: AnyRec): PageFieldFinding[] { + const findings: PageFieldFinding[] = []; + if (!stack || typeof stack !== 'object') return findings; + + // object name → its declared field names. Built with `asArray` so BOTH + // `fields` shapes (array of `{name}` and name-keyed map) resolve. + const objectFields = indexObjectFields(stack); const pages = asArray(stack.pages); for (let pi = 0; pi < pages.length; pi++) { @@ -171,35 +354,8 @@ export function validatePageFieldBindings(stack: AnyRec): PageFieldFinding[] { if (!page || typeof page !== 'object') continue; const pageName = strName(page.name) ?? `#${pi}`; - /** - * Check one batch of refs against `objectName`. Bails out entirely when the - * object is not defined in this stack — it may come from another installed - * package, and we cannot judge fields on a schema we cannot see (the same - * skip the flow/widget rules use). - */ - const checkRefs = (refs: FieldRef[], objectName: string | undefined, where: string) => { - if (!objectName) return; // nothing to resolve against - const known = objectFields.get(objectName); - if (!known) return; // cross-package object — unknowable here - for (const ref of refs) { - // A relationship path (`account.name`) is resolved by the query engine, - // not a base column, so it cannot be judged here. - if (ref.name.includes('.')) continue; - if (known.has(ref.name) || SYSTEM_FIELDS.has(ref.name)) continue; - findings.push({ - severity: 'warning', - rule: PAGE_FIELD_UNKNOWN, - where, - path: ref.path, - message: - `field "${ref.name}" is not a field on object "${objectName}" — ` + - `the component silently skips it, so it never renders.`, - hint: - `Fix the field name, or add "${ref.name}" to ${objectName}. ` + - `References must match the object's field names exactly.` + - (known.size > 0 ? ` Object fields: ${[...known].sort().join(', ')}.` : ''), - }); - } + const checkRefs = (refs: readonly FieldRef[], objectName: string | undefined, where: string) => { + findings.push(...checkFieldRefs(refs, objectName, objectFields, where)); }; for (const { component, path, objectName } of walkPageComponents(page, `pages[${pi}]`)) { @@ -207,63 +363,20 @@ export function validatePageFieldBindings(stack: AnyRec): PageFieldFinding[] { const props = isRec(component.properties) ? component.properties : undefined; if (!type || !props) continue; const where = `page "${pageName}" · ${type}`; + const base = `${path}.properties`; if (type === RELATED_LIST_TYPE) { - // Columns / sort / filter address the RELATED object. - const relatedObject = strName(props.objectName); - const relatedRefs: FieldRef[] = [ - ...fieldRefsFrom(props.columns, `${path}.properties.columns`), - ...fieldRefsFrom(props.sort, `${path}.properties.sort`), - ...fieldRefsFrom(props.filter, `${path}.properties.filter`), - ...fieldRefsFrom(props.relationshipField, `${path}.properties.relationshipField`), - ]; - checkRefs(relatedRefs, relatedObject, where); + const split = relatedListFieldRefs(props, base); + checkRefs(split.related, split.relatedObject, where); // `relationshipValueField` names a field on the PARENT (page) object. - checkRefs( - fieldRefsFrom(props.relationshipValueField, `${path}.properties.relationshipValueField`), - objectName, - where, - ); + checkRefs(split.parent, objectName, where); // The add-picker resolves against its own object. - const add = isRec(props.add) ? props.add : undefined; - const picker = add && isRec(add.picker) ? add.picker : undefined; - if (picker) { - checkRefs( - [ - ...fieldRefsFrom(picker.valueField, `${path}.properties.add.picker.valueField`), - ...fieldRefsFrom(picker.labelField, `${path}.properties.add.picker.labelField`), - ], - strName(picker.object), - where, - ); - } - if (add) { - checkRefs( - fieldRefsFrom(add.linkField, `${path}.properties.add.linkField`), - relatedObject, - where, - ); - } + checkRefs(split.picker, split.pickerObject, where); continue; } - const spec = COMPONENT_FIELD_SPECS[type]; - if (!spec) continue; // unregistered / non-field component — skip silently - - const refs: FieldRef[] = []; - for (const key of spec.props ?? []) { - refs.push(...fieldRefsFrom(props[key], `${path}.properties.${key}`)); - } - for (const key of spec.nestedSections ?? []) { - const sections = Array.isArray(props[key]) ? (props[key] as unknown[]) : []; - for (let si = 0; si < sections.length; si++) { - const section = sections[si]; - if (!isRec(section)) continue; - refs.push( - ...fieldRefsFrom(section.fields, `${path}.properties.${key}[${si}].fields`), - ); - } - } + const refs = componentFieldRefs(type, props, base); + if (!refs) continue; // unregistered / non-field component — skip silently checkRefs(refs, objectName, where); } @@ -275,7 +388,7 @@ export function validatePageFieldBindings(stack: AnyRec): PageFieldFinding[] { const base = `pages[${pi}].interfaceConfig`; const refs: FieldRef[] = [ ...fieldRefsFrom(cfg.columns, `${base}.columns`), - ...fieldRefsFrom(cfg.sort, `${base}.sort`), + ...sortFieldRefs(cfg.sort, `${base}.sort`), ...fieldRefsFrom(cfg.filterBy, `${base}.filterBy`), ]; const userFilters = isRec(cfg.userFilters) ? cfg.userFilters : undefined; diff --git a/packages/lint/src/validate-react-page-props.test.ts b/packages/lint/src/validate-react-page-props.test.ts index 9e36f73c6b..55d00e9683 100644 --- a/packages/lint/src/validate-react-page-props.test.ts +++ b/packages/lint/src/validate-react-page-props.test.ts @@ -5,8 +5,10 @@ import { REACT_CHART_FIELD_UNKNOWN, REACT_CHART_AGGREGATE_INVALID, REACT_CHART_AXIS_UNKNOWN, + type ReactPropFinding as PropFinding, } from './validate-react-page-props.js'; import { SEARCHABLE_FIELD_UNKNOWN } from './validate-searchable-fields.js'; +import { PAGE_FIELD_UNKNOWN } from './validate-page-field-bindings.js'; const page = (source: string) => ({ pages: [{ name: 'p', kind: 'react', source }] }); @@ -347,3 +349,419 @@ describe('validateReactPageProps — searchableFields (#4329)', () => expect(f).toEqual([]); }); }); + +// ───────────────────────────────────────────────────────────────────────── +// Every OTHER field-bearing block prop (#4340) — the class #4329 opened one +// instance of. Resolved under the metadata rule's id (`page-field-unknown`), +// against the block's own bound object, at the metadata rule's severity — +// except a FILTER position, which reaches a query rather than a renderer. +// ───────────────────────────────────────────────────────────────────────── + +const invoiceObj = { + name: 'crm_invoice', + fields: { + name: { type: 'text' }, + status: { type: 'select' }, + total: { type: 'currency' }, + account_id: { type: 'lookup', reference: 'crm_account' }, + }, +}; + +const propsPage = (source: string, objects: unknown[] = [account, invoiceObj]) => ({ + objects, + pages: [{ name: 'p', kind: 'react', source }], +}); +const jsx = (tag: string, attrs: string) => `function Page(){ return <${tag} ${attrs} />; }`; +const unknownFields = (f: PropFinding[]) => f.filter((x) => x.rule === PAGE_FIELD_UNKNOWN); + +describe('validateReactPageProps — field props (#4340)', () => { + it('passes columns/fields/sort/grouping/userFilters that all resolve', () => { + const f = validateReactPageProps( + propsPage( + jsx( + 'ListView', + `objectName="crm_account" columns={['name']} fields={['billing_email']} ` + + `sort={[{ field: 'name', order: 'asc' }]} grouping={{ fields: [{ field: 'name' }] }} ` + + `userFilters={{ element: 'dropdown', fields: [{ field: 'billing_email' }] }}`, + ), + ), + ); + expect(f).toEqual([]); + }); + + it('flags a stale `columns` entry, advisory, under the metadata rule id', () => { + const f = unknownFields( + validateReactPageProps(propsPage(jsx('ListView', `objectName="crm_account" columns={['name', 'revenue']}`))), + ); + expect(f).toHaveLength(1); + expect(f[0].severity).toBe('warning'); + expect(f[0].where).toBe('page "p" › '); + expect(f[0].path).toBe('pages[0].source › columns[1]'); + expect(f[0].message).toContain('"revenue"'); + expect(f[0].message).toContain('crm_account'); + }); + + it('flags the React overlay `fields` prop too — either spelling names columns', () => { + const f = unknownFields( + validateReactPageProps(propsPage(jsx('ListView', `objectName="crm_account" fields={['nope']}`))), + ); + expect(f).toHaveLength(1); + expect(f[0].path).toBe('pages[0].source › fields[0]'); + }); + + it('reads a `{field}` column record, not just a bare name', () => { + const f = unknownFields( + validateReactPageProps( + propsPage(jsx('ListView', `objectName="crm_account" columns={[{ field: 'nope', width: 120 }]}`)), + ), + ); + expect(f).toHaveLength(1); + expect(f[0].message).toContain('"nope"'); + }); + + it('reads the LEGACY bare-string sort by its head, not the whole string', () => { + const ok = unknownFields( + validateReactPageProps(propsPage(jsx('ListView', `objectName="crm_account" sort="name desc"`))), + ); + expect(ok).toEqual([]); + const bad = unknownFields( + validateReactPageProps(propsPage(jsx('ListView', `objectName="crm_account" sort="revenue desc"`))), + ); + expect(bad).toHaveLength(1); + expect(bad[0].message).toContain('"revenue"'); + expect(bad[0].message).not.toContain('revenue desc'); + }); + + it('flags a stale nested userFilters/grouping field', () => { + const f = unknownFields( + validateReactPageProps( + propsPage( + jsx( + 'ListView', + `objectName="crm_account" userFilters={{ fields: [{ field: 'nope' }] }} grouping={{ fields: [{ field: 'also_nope' }] }}`, + ), + ), + ), + ); + expect(f.map((x) => x.path)).toEqual([ + 'pages[0].source › userFilters.fields[0].field', + 'pages[0].source › grouping.fields[0].field', + ]); + }); + + it('covers the legacy `filterableFields` shorthand', () => { + const f = unknownFields( + validateReactPageProps(propsPage(jsx('ListView', `objectName="crm_account" filterableFields={['nope']}`))), + ); + expect(f).toHaveLength(1); + }); +}); + +describe('validateReactPageProps — filter POSITIONS gate (#4340)', () => { + it('resolves the field position beside a NON-static value — the common react shape', () => { + // `staticValue` bails on the whole array here; the filter reader keeps the + // positions independent, so the knowable one is still checked. + const f = unknownFields( + validateReactPageProps( + propsPage( + 'function Page(){ const stage = "x"; return { + const f = validateReactPageProps( + propsPage(jsx('ListView', `objectName="crm_account" filters={['billing_email', '=', 'a@b.c']}`)), + ); + expect(f).toEqual([]); + }); + + it('descends compound and legacy-flat filter nodes', () => { + const f = unknownFields( + validateReactPageProps( + propsPage( + jsx('ListView', `objectName="crm_account" filters={['and', ['name', '=', 'x'], ['nope', '>', 1]]}`), + ), + ), + ); + expect(f).toHaveLength(1); + expect(f[0].path).toBe('pages[0].source › filters[2][0]'); + + const flat = unknownFields( + validateReactPageProps( + propsPage(jsx('ListView', `objectName="crm_account" filters={[['nope', '=', 1]]}`)), + ), + ); + expect(flat).toHaveLength(1); + }); + + it('says nothing when the field POSITION itself is not static', () => { + const f = validateReactPageProps( + propsPage('function Page(){ const c = "x"; return on the same terms', () => { + const f = unknownFields( + validateReactPageProps(propsPage(jsx('ObjectChart', `objectName="crm_invoice" filter={['nope', '=', 'x']}`))), + ); + expect(f).toHaveLength(1); + expect(f[0].severity).toBe('error'); + }); +}); + +describe('validateReactPageProps — field props (#4340)', () => { + it('flags a stale `fields` entry and a stale `initialValues` KEY', () => { + const f = unknownFields( + validateReactPageProps( + propsPage( + jsx('ObjectForm', `objectName="crm_account" fields={['name', 'nope']} initialValues={{ name: 'x', bogus: 1 }}`), + ), + ), + ); + expect(f.map((x) => x.path)).toEqual([ + 'pages[0].source › fields[1]', + 'pages[0].source › initialValues.bogus', + ]); + expect(f.every((x) => x.severity === 'warning')).toBe(true); + }); + + it('walks sections[].fields[] (both the bare and the {name} entry shapes)', () => { + const f = unknownFields( + validateReactPageProps( + propsPage( + jsx('ObjectForm', `objectName="crm_account" sections={[{ label: 'Basics', fields: ['name', 'nope', { name: 'also_nope' }] }]}`), + ), + ), + ); + expect(f.map((x) => x.path)).toEqual([ + 'pages[0].source › sections[0].fields[1]', + 'pages[0].source › sections[0].fields[2].name', + ]); + }); +}); + +describe('validateReactPageProps — record:* blocks share the metadata table (#4340)', () => { + it('flags against its object', () => { + const f = unknownFields( + validateReactPageProps( + propsPage(jsx('RecordHighlights', `objectName="crm_account" recordId={1} fields={['name', 'nope']}`)), + ), + ); + expect(f).toHaveLength(1); + expect(f[0].path).toBe('pages[0].source › fields[1]'); + }); + + it('flags and its authored sections', () => { + const f = unknownFields( + validateReactPageProps( + propsPage( + jsx('RecordDetails', `objectName="crm_account" fields={['nope']} hideFields={['gone']}`), + ), + ), + ); + expect(f.map((x) => x.path)).toEqual([ + 'pages[0].source › fields[0]', + 'pages[0].source › hideFields[0]', + ]); + }); + + it('leaves alone when it is the declared string[] of section IDs', () => { + const f = validateReactPageProps( + propsPage(jsx('RecordDetails', `objectName="crm_account" layout="custom" sections={['overview', 'billing']}`)), + ); + expect(f).toEqual([]); + }); + + it('flags ', () => { + const f = unknownFields( + validateReactPageProps(propsPage(jsx('RecordPath', `objectName="crm_account" statusField="nope"`))), + ); + expect(f).toHaveLength(1); + expect(f[0].path).toBe('pages[0].source › statusField'); + }); +}); + +describe('validateReactPageProps — binds the CHILD object (#4340)', () => { + const related = (attrs: string) => jsx('RecordRelatedList', attrs); + + it('resolves columns/sort/relationshipField against the RELATED object', () => { + const f = validateReactPageProps( + propsPage( + related( + `objectName="crm_invoice" recordId={1} relationshipField="account_id" columns={['name', 'total']}`, + ), + ), + ); + expect(f).toEqual([]); + }); + + it('flags the parent-object mix-up the old contract gloss invited', () => { + // The exact shape #4340 found live: the author passed the PARENT object and + // named the child's columns + the child's own FK. Neither `total` nor + // `account_id` is on the account, so both positions report. + const f = unknownFields( + validateReactPageProps( + propsPage( + related(`objectName="crm_account" recordId={1} relationshipField="account_id" columns={['name', 'total']}`), + ), + ), + ); + expect(f.map((x) => x.path)).toEqual([ + 'pages[0].source › columns[1]', + 'pages[0].source › relationshipField', + ]); + expect(f[0].message).toContain('"total"'); + expect(f[0].message).toContain('crm_account'); + }); + + it('says nothing about relationshipValueField — the parent object is unbound here', () => { + const f = validateReactPageProps( + propsPage( + related(`objectName="crm_invoice" recordId={1} relationshipField="account_id" relationshipValueField="nope"`), + ), + ); + expect(f).toEqual([]); + }); + + it('resolves the Add picker against its OWN object', () => { + const f = unknownFields( + validateReactPageProps( + propsPage( + related( + `objectName="crm_invoice" recordId={1} relationshipField="account_id" ` + + `add={{ picker: { object: 'crm_account', labelField: 'nope' }, linkField: 'total' }}`, + ), + ), + ), + ); + expect(f).toHaveLength(1); + expect(f[0].path).toBe('pages[0].source › add.picker.labelField'); + }); +}); + +describe('validateReactPageProps — escape hatch (#4340)', () => { + it('checks the props bag by the registered type the author names', () => { + const f = unknownFields( + validateReactPageProps( + propsPage(jsx('Block', `type="record:highlights" objectName="crm_account" fields={['nope']}`)), + ), + ); + expect(f).toHaveLength(1); + expect(f[0].where).toBe('page "p" › '); + expect(f[0].path).toBe('pages[0].source › fields[0]'); + }); + + it('reaches the related-list branch through too', () => { + const f = unknownFields( + validateReactPageProps( + propsPage(jsx('Block', `type="record:related_list" objectName="crm_account" columns={['total']}`)), + ), + ); + expect(f).toHaveLength(1); + }); + + it('skips a type with no descriptor, and a non-static type', () => { + const noSpec = validateReactPageProps( + propsPage(jsx('Block', `type="object-kanban" objectName="crm_account" fields={['nope']}`)), + ); + expect(noSpec).toEqual([]); + const dynamic = validateReactPageProps( + propsPage('function Page(){ const t = "record:highlights"; return ; }'), + ); + expect(dynamic).toEqual([]); + }); +}); + +describe('validateReactPageProps — field-prop false-positive guards (#4340)', () => { + it('skips an object this stack does not define', () => { + const f = validateReactPageProps( + propsPage(jsx('ListView', `objectName="pkg_thing" columns={['nope']} filters={['nope', '=', 1]}`)), + ); + expect(f).toEqual([]); + }); + + it('skips a block with no static objectName', () => { + const f = validateReactPageProps( + propsPage('function Page(){ const o = "crm_account"; return ; }'), + ); + expect(f).toEqual([]); + }); + + it('skips a non-static value', () => { + const f = validateReactPageProps( + propsPage('function Page(){ const c = ["nope"]; return ; }'), + ); + expect(f).toEqual([]); + }); + + it('skips everything behind a spread', () => { + const f = validateReactPageProps( + propsPage( + 'function Page(){ const p = {}; return ; }', + ), + ); + expect(f).toEqual([]); + }); + + it('accepts registry-injected system columns and relationship paths', () => { + const f = validateReactPageProps( + propsPage( + jsx('ListView', `objectName="crm_account" columns={['created_at', 'owner_id', 'owner_id.name']} filters={['created_at', '>', 1]}`), + ), + ); + expect(f).toEqual([]); + }); +}); + +describe('validateReactPageProps — resolve per child object (#4340)', () => { + it('checks each entry against its own childObject, and totalField against the form object', () => { + const f = unknownFields( + validateReactPageProps( + propsPage( + jsx( + 'ObjectForm', + `objectName="crm_account" subforms={[{ childObject: 'crm_invoice', relationshipField: 'account_id', ` + + `columns: ['total', 'nope'], amountField: 'total', totalField: 'not_on_account' }]}`, + ), + ), + ), + ); + expect(f.map((x) => x.path)).toEqual([ + 'pages[0].source › subforms[0].columns[1]', + 'pages[0].source › subforms[0].totalField', + ]); + // The child-scoped miss names the CHILD object, not the form's. + expect(f[0].message).toContain('crm_invoice'); + expect(f[1].message).toContain('crm_account'); + }); + + it('skips an entry whose childObject is absent or from another package', () => { + const f = validateReactPageProps( + propsPage( + jsx('ObjectForm', `objectName="crm_account" subforms={[{ columns: ['nope'] }, { childObject: 'pkg_line', columns: ['nope'] }]}`), + ), + ); + expect(f).toEqual([]); + }); +}); diff --git a/packages/lint/src/validate-react-page-props.ts b/packages/lint/src/validate-react-page-props.ts index bdfb7f387c..1d30a1d48c 100644 --- a/packages/lint/src/validate-react-page-props.ts +++ b/packages/lint/src/validate-react-page-props.ts @@ -20,6 +20,8 @@ // - 's `searchableFields` entries, resolved against the bound // object's declared fields (#4329) — the react-surface twin of the // metadata rule `searchable-field-unknown`, sharing its core. +// - EVERY OTHER field-bearing prop a react block can author (#4340) — see +// `REACT_FIELD_SPECS` below and the ledger beside it. // // Reading values is opt-in per block and per prop: everything below evaluates // only STATIC literals (`objectName="invoice"`, an `aggregate={{…}}` object @@ -30,10 +32,23 @@ import { createRequire } from 'node:module'; import type ts from 'typescript'; import { REACT_BLOCKS, chartAggregateResultKeys } from '@objectstack/spec/ui'; +import { VALID_AST_OPERATORS } from '@objectstack/spec/data'; import { checkSearchableFieldList, indexObjectSearchTargets, } from './validate-searchable-fields.js'; +import { + COMPONENT_FIELD_SPECS, + RELATED_LIST_TYPE, + checkFieldRefs, + componentFieldRefs, + fieldRefsFrom, + indexObjectFields, + relatedListFieldRefs, + sortFieldRefs, + type FieldRef, + type PageFieldFinding, +} from './validate-page-field-bindings.js'; import { SYSTEM_FIELDS } from './system-fields.js'; @@ -177,6 +192,29 @@ function attrValue(tsc: typeof ts, sf: ts.SourceFile, attr: ts.JsxAttribute): un return NOT_STATIC; } +/** + * The value of a FILTER attribute, resolved position by position: an array + * literal survives even when some of its elements do not, each unknowable + * element left as `NOT_STATIC` in place. + * + * `staticValue` collapses such an array to `NOT_STATIC` whole, which is correct + * where the value only means something entire (an `aggregate={{…}}`) and wrong + * for a filter, whose positions are independent: `['status', '=', stage]` has a + * knowable FIELD beside an unknowable VALUE, and that is the shape a react page + * writes when it drives a list from React state. See `filterFieldRefs`. + */ +function filterAttrValue(tsc: typeof ts, sf: ts.SourceFile, attr: ts.JsxAttribute): unknown { + const init = attr.initializer; + if (!init || !tsc.isJsxExpression(init)) return NOT_STATIC; + const perPosition = (node: ts.Node | undefined): unknown => { + if (!node) return NOT_STATIC; + if (tsc.isParenthesizedExpression(node)) return perPosition(node.expression); + if (tsc.isArrayLiteralExpression(node)) return node.elements.map((el) => perPosition(el)); + return staticValue(tsc, sf, node); + }; + return perPosition(init.expression); +} + // ─── binding integrity (#3701) ────────────────────────────── // // `validate-chart-bindings` covers every DATASET-bound chart surface: a @@ -211,37 +249,6 @@ export const REACT_CHART_AXIS_UNKNOWN = 'react-chart-axis-unknown'; const CHART_FUNCTIONS = ['count', 'sum', 'avg', 'min', 'max'] as const; -/** - * Both `objects` and an object's `fields` are authored either as an array of - * `{ name }` records or as a name-keyed map — normalize to the array form, the - * same way the other reference-integrity rules do. - */ -function namedArray(v: unknown): AnyRec[] { - if (Array.isArray(v)) return v.filter((x): x is AnyRec => !!x && typeof x === 'object'); - if (v && typeof v === 'object') { - return Object.entries(v as AnyRec).map(([name, def]) => ({ - name, - ...(def && typeof def === 'object' ? (def as AnyRec) : {}), - })); - } - return []; -} - -/** object name → its declared field names. */ -function indexObjectFields(stack: AnyRec): Map> { - const out = new Map>(); - for (const obj of namedArray(stack.objects)) { - const name = typeof obj.name === 'string' ? obj.name : undefined; - if (!name) continue; - const names = new Set(); - for (const f of namedArray(obj.fields)) { - if (typeof f.name === 'string' && f.name) names.add(f.name); - } - out.set(name, names); - } - return out; -} - const isRec = (v: unknown): v is Record => !!v && typeof v === 'object' && !Array.isArray(v); @@ -380,6 +387,315 @@ function checkObjectChart( } } +// ─── Field-bearing block props (#4340) ──────────────────────────────────── +// +// #4329 closed ONE prop — `` — by running the +// metadata rule's core from here. `searchableFields` was an instance, not the +// class: every other prop a react block binds BY FIELD NAME shipped exactly as +// typed, the same silent drift `validate-page-field-bindings` closes for the +// page-component `properties` bag one surface over. This section closes the +// class. +// +// ## Where the answers come from +// +// The `record:*` blocks ARE the components that rule already walks — one +// registry component, two authoring surfaces — so they are not re-described +// here at all: `componentFieldRefs` / `relatedListFieldRefs` read the SAME +// `COMPONENT_FIELD_SPECS` table, keyed by the block's own `schemaType`. A prop +// added there is checked on both surfaces at once, which is the point. +// +// `REACT_FIELD_SPECS` below covers only what the shared table cannot: the two +// blocks whose metadata twin lives under different prop names — +// `` (twin: a list page's `interfaceConfig`) and `` +// (twin: `element:form` + the form-layout rule). +// +// ## What is deliberately NOT checked, and why +// +// - Anything non-static (a variable, a call, a value behind a spread) — +// ADR-0072 D1: unresolvable is not wrong. `filters` is the one place this +// is resolved PER POSITION rather than all-or-nothing; see below. +// - ``: it names a field on the +// PARENT object, and the react surface binds the parent by `recordId` +// only — there is no parent OBJECT to resolve against. The metadata twin +// has the page's object and checks it there. This is the ONE field-bearing +// prop in the index that stays unresolved, and the reason is a missing +// binding rather than a missing rule. +// - ``'s axes: they name the aggregate's RESULT COLUMNS, not +// fields, and `checkObjectChart` above already owns them. +// +// `` does not ride the table either, but IS checked: +// each entry names its own `childObject`, so its refs are split per entry +// rather than pooled against the block's object (`subformFieldRefs`). + +/** + * Which props of a block carry field names, and in what shape. Each bucket is + * a different SHAPE, not a different meaning — every entry resolves against the + * block's own `objectName`. + */ +interface ReactFieldSpec { + /** Bare names, `{field}`/`{name}` records, or arrays of either. */ + fields?: readonly string[]; + /** A `sort`: structured `{field,order}[]` or the legacy `"field desc"` string. */ + sorts?: readonly string[]; + /** A `{ fields: […] }` wrapper — one level of nesting. */ + nestedFields?: readonly string[]; + /** `{…}[]` sections whose `fields[]` name fields. */ + sections?: readonly string[]; + /** An object literal whose KEYS name fields. */ + keyedByField?: readonly string[]; + /** An ObjectQL FilterArray — its field POSITIONS gate (see `filterFieldRefs`). */ + filterArrays?: readonly string[]; +} + +const REACT_FIELD_SPECS: Readonly> = { + ListView: { + // `fields` is the React overlay's "limit/order the columns"; `columns` the + // spec ListView prop. Both name columns on the bound object, and a page may + // write either. `hiddenFields`/`fieldOrder`/`filterableFields` are schema + // props outside the curated contract — unadvertised but honored by the + // renderer, so a stale name there is drift just the same. + fields: ['fields', 'columns', 'hiddenFields', 'fieldOrder', 'filterableFields'], + sorts: ['sort'], + nestedFields: ['userFilters', 'grouping'], + filterArrays: ['filters'], + }, + ObjectForm: { + fields: ['fields'], + keyedByField: ['initialValues'], + // `groups` is FormViewSchema's legacy alias for `sections`. + sections: ['sections', 'groups'], + }, + ObjectChart: { + // The axes are result columns (checkObjectChart owns them); `filter` is an + // ordinary ObjectQL predicate over the bound object, like ListView's. + filterArrays: ['filter'], + }, +}; + +/** + * How a prop name joins onto a react page's `path`. The props live inside one + * opaque `source` string, so there is no config path to extend — #4329 + * established `pages[0].source › searchableFields[1]` and every prop below + * follows it. + */ +const PATH_SEP = ' › '; + +/** tag → `schemaType`, read from the contract rather than restated. */ +const SCHEMA_TYPE_BY_TAG: ReadonlyMap = new Map( + (REACT_BLOCKS as Array<{ tag: string; schemaType: string }>).map((b) => [b.tag, b.schemaType]), +); + +/** + * Attribute names read POSITION BY POSITION rather than all-or-nothing (see + * `filterAttrValue`). Derived from the specs so a new `filterArrays` entry + * cannot forget to opt in — the failure mode would be silent, since the + * all-or-nothing reader simply finds nothing. + * + * `` rides along under the same name while holding a + * different shape (`{field, operator, value}[]`, not a FilterArray). That is + * harmless: a fully-static array reads identically either way, and the only + * difference — surviving with `NOT_STATIC` holes — is dropped by + * `fieldRefsFrom`, which ignores a non-string, non-record entry. + */ +const FILTER_PROPS: ReadonlySet = new Set( + Object.values(REACT_FIELD_SPECS).flatMap((s) => s.filterArrays ?? []), +); + +/** Strip the `NOT_STATIC` sentinel so a shared extractor sees plain data. */ +function readableProps(values: ReadonlyMap): AnyRec { + const out: AnyRec = {}; + for (const [k, v] of values) if (v !== NOT_STATIC) out[k] = v; + return out; +} + +/** + * `` — inline master-detail child collections. Each entry + * names the object its own refs resolve against (`childObject`), so unlike + * every bucket in {@link ReactFieldSpec} these cannot be pooled into one batch + * checked against the block's `objectName`. `totalField` is the exception + * inside the exception: it names the PARENT field the child sum rolls up into. + */ +function subformFieldRefs( + value: unknown, + basePath: string, +): { child: Array<{ objectName: string | undefined; refs: FieldRef[] }>; parent: FieldRef[] } { + const child: Array<{ objectName: string | undefined; refs: FieldRef[] }> = []; + const parent: FieldRef[] = []; + if (!Array.isArray(value)) return { child, parent }; + for (let i = 0; i < value.length; i++) { + const sub = value[i]; + if (!isRec(sub)) continue; + const at = (key: string) => `${basePath}[${i}].${key}`; + child.push({ + objectName: strOf(sub.childObject), + refs: [ + ...fieldRefsFrom(sub.columns, at('columns')), + ...fieldRefsFrom(sub.relationshipField, at('relationshipField')), + ...fieldRefsFrom(sub.amountField, at('amountField')), + ], + }); + parent.push(...fieldRefsFrom(sub.totalField, at('totalField'))); + } + return { child, parent }; +} + +/** + * Field references in an ObjectQL FilterArray, resolved PER POSITION. + * + * `staticValue` is all-or-nothing by design: an array containing one unknowable + * element is not a knowable array. That is right for an `aggregate={{…}}`, and + * wrong here, because the common react filter is exactly the mixed case — + * `filters={['status', '=', stage]}` pairs a STATIC field position with a + * React-state value. Bailing on the whole array would skip the only position + * this rule can judge, on the shape authors actually write. + * + * So the reader keeps each position separate (`filterAttrValue`) and this walk + * only ever reads position 0, and only when position 1 is a recognised operator + * — the same test `isFilterAST` makes, using the spec's own operator vocabulary + * so the two cannot drift. A non-static field position, a non-static operator, + * or a shape that is not a filter node yields nothing. + */ +function filterFieldRefs(node: unknown, basePath: string, out: FieldRef[]): void { + if (!Array.isArray(node) || node.length === 0) return; + const head = node[0]; + if (typeof head === 'string' && (head.toLowerCase() === 'and' || head.toLowerCase() === 'or')) { + for (let i = 1; i < node.length; i++) filterFieldRefs(node[i], `${basePath}[${i}]`, out); + return; + } + // Legacy flat array of conditions: `[[a,'=',1], [b,'>',2]]`. + if (Array.isArray(head)) { + for (let i = 0; i < node.length; i++) filterFieldRefs(node[i], `${basePath}[${i}]`, out); + return; + } + if ( + typeof head === 'string' && head.length > 0 && + node.length >= 2 && typeof node[1] === 'string' && + VALID_AST_OPERATORS.has(node[1].toLowerCase()) + ) { + out.push({ name: head, path: `${basePath}[0]` }); + } +} + +/** The field refs one block's statically-read attributes hold, by bucket. */ +function reactFieldRefs( + spec: ReactFieldSpec, + values: ReadonlyMap, + basePath: string, +): { own: FieldRef[]; queried: FieldRef[] } { + const own: FieldRef[] = []; + const queried: FieldRef[] = []; + const readable = (key: string): unknown => { + const v = values.get(key); + return v === NOT_STATIC ? undefined : v; + }; + const at = (key: string) => `${basePath}${PATH_SEP}${key}`; + + for (const key of spec.fields ?? []) { + own.push(...fieldRefsFrom(readable(key), at(key))); + } + for (const key of spec.sorts ?? []) { + own.push(...sortFieldRefs(readable(key), at(key))); + } + for (const key of spec.nestedFields ?? []) { + const v = readable(key); + if (isRec(v)) own.push(...fieldRefsFrom(v.fields, at(`${key}.fields`))); + } + for (const key of spec.sections ?? []) { + const v = readable(key); + if (!Array.isArray(v)) continue; + for (let i = 0; i < v.length; i++) { + const section = v[i]; + if (!isRec(section)) continue; + own.push(...fieldRefsFrom(section.fields, at(`${key}[${i}].fields`))); + } + } + for (const key of spec.keyedByField ?? []) { + const v = readable(key); + if (!isRec(v)) continue; + for (const k of Object.keys(v)) own.push({ name: k, path: at(`${key}.${k}`) }); + } + for (const key of spec.filterArrays ?? []) { + // Read the raw attribute here, NOT `readable`: a filter whose VALUE is + // non-static still has a knowable field position, and `filterAttrValue` + // preserved exactly that. + filterFieldRefs(values.get(key), at(key), queried); + } + return { own, queried }; +} + +/** + * Resolve every field-bearing prop of one block usage against its bound object. + * + * Three sources feed it, in the order a block can claim them: + * + * 1. `REACT_FIELD_SPECS` — the react-only descriptors (`ListView`, + * `ObjectForm`, `ObjectChart`'s `filter`). + * 2. the `record:related_list` split, when the block IS that component. + * 3. `COMPONENT_FIELD_SPECS`, keyed by the block's `schemaType` — the shared + * table the metadata surface already uses. `` reaches it + * by the type the author wrote, which is what makes the escape hatch + * checked rather than a hole. + * + * Findings come back in this rule's own shape but under the metadata rule's id + * (`page-field-unknown`): the same question, asked of the same component, with + * the same fix. + */ +function checkBlockFieldProps( + tag: string, + values: ReadonlyMap, + objectFields: ReadonlyMap>, + where: string, + path: string, +): ReactPropFinding[] { + const objectName = strOf(values.get('objectName')); + const out: PageFieldFinding[] = []; + + const spec = REACT_FIELD_SPECS[tag]; + if (spec) { + const { own, queried } = reactFieldRefs(spec, values, path); + out.push(...checkFieldRefs(own, objectName, objectFields, where)); + out.push(...checkFieldRefs(queried, objectName, objectFields, where, 'queried')); + } + + if (tag === 'ObjectForm') { + const raw = values.get('subforms'); + const subs = subformFieldRefs(raw === NOT_STATIC ? undefined : raw, `${path}${PATH_SEP}subforms`); + for (const sub of subs.child) { + out.push(...checkFieldRefs(sub.refs, sub.objectName, objectFields, where)); + } + // `totalField` names the FORM object's field the child sum rolls up into. + out.push(...checkFieldRefs(subs.parent, objectName, objectFields, where)); + } + + // `` renders the registered component the + // author names; every other block's type is fixed by its tag. + const schemaType = tag === 'Block' ? strOf(values.get('type')) : SCHEMA_TYPE_BY_TAG.get(tag); + if (schemaType) { + const props = readableProps(values); + if (schemaType === RELATED_LIST_TYPE) { + const split = relatedListFieldRefs(props, path, PATH_SEP); + out.push(...checkFieldRefs(split.related, split.relatedObject, objectFields, where)); + out.push(...checkFieldRefs(split.picker, split.pickerObject, objectFields, where)); + // `split.parent` (`relationshipValueField`) is deliberately dropped: the + // react surface binds the parent RECORD (`recordId`) but never its + // object, so there is nothing to resolve it against. See the section note. + } else if (COMPONENT_FIELD_SPECS[schemaType]) { + out.push( + ...checkFieldRefs( + componentFieldRefs(schemaType, props, path, PATH_SEP) ?? [], + objectName, + objectFields, + where, + ), + ); + } + } + + // The two finding shapes are structurally identical; `where`/`path` are + // already this surface's, so only the declared type differs. + return out as ReactPropFinding[]; +} + export function validateReactPageProps(stack: AnyRec): ReactPropFinding[] { const findings: ReactPropFinding[] = []; const objectFields = indexObjectFields(stack); @@ -420,7 +736,12 @@ export function validateReactPageProps(stack: AnyRec): ReactPropFinding[] { if (tsc.isJsxAttribute(a)) { const propName = a.name.getText(sf); used.add(propName); - values.set(propName, attrValue(tsc, sf, a)); + values.set( + propName, + FILTER_PROPS.has(propName) + ? filterAttrValue(tsc, sf, a) + : attrValue(tsc, sf, a), + ); } } const where = `page "${name}" › <${tag}>`; @@ -473,6 +794,14 @@ export function validateReactPageProps(stack: AnyRec): ReactPropFinding[] { ), ); } + // Every other field-bearing prop (#4340). Same skips as the chart + // and searchableFields checks: a spread hides the picture, and a + // non-static value is unresolvable rather than wrong. + if (!hasSpread) { + findings.push( + ...checkBlockFieldProps(tag, values, objectFields, where, path), + ); + } } } tsc.forEachChild(node, visit); diff --git a/packages/spec/api-surface.json b/packages/spec/api-surface.json index 345c324a51..da54103314 100644 --- a/packages/spec/api-surface.json +++ b/packages/spec/api-surface.json @@ -3400,6 +3400,7 @@ "PluralRule (type)", "PluralRuleSchema (const)", "REACT_BLOCKS (const)", + "REACT_OVERLAY_SHADOWS (const)", "ReactBlockDef (interface)", "ReactInteractionProp (interface)", "ReactPropKind (type)", diff --git a/packages/spec/src/ui/react-blocks.test.ts b/packages/spec/src/ui/react-blocks.test.ts new file mode 100644 index 0000000000..12255f11a1 --- /dev/null +++ b/packages/spec/src/ui/react-blocks.test.ts @@ -0,0 +1,89 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. +// +// The react-tier block index is half generated (`data` props read from each +// block's spec zod schema) and half hand-authored (the React interaction +// overlay). These tests guard the seam between the two halves — the place a +// hand edit can silently contradict the protocol. + +import { describe, it, expect } from 'vitest'; +import { z } from 'zod'; +import { REACT_BLOCKS, REACT_OVERLAY_SHADOWS } from './react-blocks'; + +/** The prop names a block's spec schema declares, as the contract generator reads them. */ +function schemaPropNames(schema: unknown): string[] { + let js: any; + try { + js = z.toJSONSchema(schema as any, { unrepresentable: 'any' } as any); + } catch { + return []; + } + if (js?.$ref && js?.$defs) js = js.$defs[String(js.$ref).split('/').pop()!] ?? js; + return Object.keys(js?.properties ?? {}); +} + +/** tag → overlay props that also exist on the block's spec schema. */ +function actualShadows(): Record { + const out: Record = {}; + for (const b of REACT_BLOCKS) { + if (!b.schema) continue; + const schemaProps = new Set(schemaPropNames(b.schema)); + const collisions = b.interactions.map((i) => i.name).filter((n) => schemaProps.has(n)); + if (collisions.length > 0) out[b.tag] = collisions.sort(); + } + return out; +} + +describe('REACT_BLOCKS — overlay/schema seam', () => { + /** + * The bug class #4340 opened on, closed structurally. + * + * `build-react-blocks-contract`'s `mergeProps` lets the overlay win on a name + * collision: the schema's type and description are DROPPED. So an overlay + * entry that duplicates a schema prop can publish a different meaning for it + * and nothing reports the divergence — which is exactly what happened to + * `` (schema: the related object; overlay: "the + * parent object"). Every live page authored against the published gloss then + * named the wrong object, and the lint that should have caught it had no way + * to know which reading to check against. + * + * A collision is therefore allowed only when it is deliberate and + * meaning-preserving, and must be ledgered with its reason in + * `REACT_OVERLAY_SHADOWS`. This asserts the ledger IS the collision set, in + * both directions: a new accidental shadow fails here, and a ledger entry + * that stops being a real collision fails here too rather than rotting. + */ + it('every overlay prop shadowing a spec-schema prop is ledgered', () => { + const ledger = Object.fromEntries( + Object.entries(REACT_OVERLAY_SHADOWS).map(([tag, props]) => [tag, [...props].sort()]), + ); + expect(actualShadows()).toEqual(ledger); + }); + + it('ledgers only tags that exist in the index', () => { + const tags = new Set(REACT_BLOCKS.map((b) => b.tag)); + for (const tag of Object.keys(REACT_OVERLAY_SHADOWS)) expect(tags.has(tag)).toBe(true); + }); + + /** + * The `record:related_list` reading, pinned where an author and the linter + * both read it. `validate-page-field-bindings` resolves this component's + * `columns`/`sort`/`filter` against `properties.objectName` as the RELATED + * object on the metadata surface, and `validate-react-page-props` now does + * the same on the react surface — both are wrong the moment this description + * says "parent" again. + */ + it(' is published as the related (child) object', () => { + const block = REACT_BLOCKS.find((b) => b.tag === 'RecordRelatedList'); + const objectName = block?.interactions.find((i) => i.name === 'objectName'); + expect(objectName).toBeDefined(); + expect(objectName!.description).toMatch(/RELATED \(child\) object/); + expect(objectName!.description).toMatch(/NOT the parent/); + // The spec schema is the authority it must agree with. + expect(schemaPropNames(block!.schema)).toContain('objectName'); + }); + + it('every block tag is unique (the contract is keyed by it)', () => { + const tags = REACT_BLOCKS.map((b) => b.tag); + expect(new Set(tags).size).toBe(tags.length); + }); +}); diff --git a/packages/spec/src/ui/react-blocks.ts b/packages/spec/src/ui/react-blocks.ts index 9ea86fe97d..e889cda3d7 100644 --- a/packages/spec/src/ui/react-blocks.ts +++ b/packages/spec/src/ui/react-blocks.ts @@ -24,6 +24,16 @@ import { ChartConfigSchema } from './chart.zod'; export type ReactPropKind = 'data' | 'binding' | 'controlled' | 'callback'; +/** + * One hand-authored overlay prop. + * + * ⚠️ An overlay entry that shares a NAME with a prop of the block's spec schema + * SHADOWS it: `build-react-blocks-contract` publishes the overlay's type and + * description and drops the schema's, so the two can mean different things and + * nothing says so. That is not hypothetical — see {@link REACT_OVERLAY_SHADOWS} + * for the one that shipped. Add an overlay prop only when the schema does not + * declare it; when a restatement really is wanted, ledger it there. + */ export interface ReactInteractionProp { name: string; type: string; @@ -32,6 +42,40 @@ export interface ReactInteractionProp { description: string; } +/** + * Overlay props that deliberately RESTATE a prop the block's spec schema also + * declares, per block tag. Anything not listed here must be absent from the + * schema — `react-blocks.test.ts` asserts the ledger equals the real collision + * set, so the next accidental shadow fails a test instead of shipping. + * + * ## Why the ledger exists (#4340) + * + * `RecordRelatedListProps.objectName` is the RELATED (child) object — that is + * what `record:related_list` means on every metadata surface, what + * `validate-page-field-bindings` resolves its `columns` against, and what the + * one registry component behind both surfaces consumes. The React overlay + * nonetheless declared `objectName` a second time, glossed "The parent object" + * by analogy with the blocks whose schema genuinely has no `objectName`. The + * generated contract published only that gloss, so the react surface both + * contradicted the spec AND lost any way to name the object it renders — and + * the one live page authored against the gloss listed the wrong object. + * + * A shadow is therefore allowed only when it is deliberate and + * MEANING-PRESERVING, and the reason has to be written down next to it. + */ +export const REACT_OVERLAY_SHADOWS: Readonly> = { + // Same prop, same meaning. The overlay restates it to publish the JSX + // shorthand an author writes (`navigation={{ mode: 'none' }}`) together with + // the React-side rule that pairs it with `onRowClick` — neither of which the + // declarative schema has anywhere to say. + ListView: ['navigation'], + // Same prop, same meaning as the schema's "Related object name". Kept in the + // overlay because it IS this block's data binding (`kind: 'binding'`, and + // required, as the schema declares it) and because the trap #4340 found is + // worth spelling out where an author reads it. + RecordRelatedList: ['objectName'], +}; + export interface ReactBlockDef { /** PascalCase name the author writes in JSX, e.g. ``. */ tag: string; @@ -147,11 +191,18 @@ export const REACT_BLOCKS: ReactBlockDef[] = [ { tag: 'RecordRelatedList', schemaType: 'record:related_list', - summary: 'Related child records via a lookup. Config props from the spec RecordRelatedList schema.', + summary: + 'Related child records via a lookup. `objectName` is the RELATED (child) object whose records are listed — NOT the parent; the parent record is bound by `recordId`, and `relationshipField` is the child field pointing back at it. Config props from the spec RecordRelatedList schema.', schema: RecordRelatedListProps, interactions: [ - { name: 'recordId', type: 'string | number', kind: 'controlled', description: 'The parent record.' }, - { name: 'objectName', type: 'string', kind: 'binding', description: 'The parent object.' }, + { name: 'recordId', type: 'string | number', kind: 'controlled', description: 'The parent record whose children are listed.' }, + // Restates RecordRelatedListProps.objectName — ledgered in + // REACT_OVERLAY_SHADOWS. This block is the ONE whose spec schema already + // declares `objectName`, and it means the CHILD object; the overlay used + // to gloss it as the parent, which is the contract conflict #4340 opened + // on. Same meaning as the schema now, said in the words that keep an + // author from writing the parent here. + { name: 'objectName', type: 'string', kind: 'binding', required: true, description: 'The RELATED (child) object whose records this list renders — e.g. objectName="invoice" on an account page. NOT the parent object: the parent record is bound by recordId.' }, ], }, { diff --git a/skills/objectstack-ui/contracts/react-blocks.contract.json b/skills/objectstack-ui/contracts/react-blocks.contract.json index f796ee2961..5b4ff2568d 100644 --- a/skills/objectstack-ui/contracts/react-blocks.contract.json +++ b/skills/objectstack-ui/contracts/react-blocks.contract.json @@ -502,22 +502,22 @@ { "tag": "RecordRelatedList", "schemaType": "record:related_list", - "summary": "Related child records via a lookup. Config props from the spec RecordRelatedList schema.", + "summary": "Related child records via a lookup. `objectName` is the RELATED (child) object whose records are listed — NOT the parent; the parent record is bound by `recordId`, and `relationshipField` is the child field pointing back at it. Config props from the spec RecordRelatedList schema.", "specSchema": true, "props": [ { "name": "objectName", "type": "string", "kind": "binding", - "required": false, - "description": "The parent object." + "required": true, + "description": "The RELATED (child) object whose records this list renders — e.g. objectName=\"invoice\" on an account page. NOT the parent object: the parent record is bound by recordId." }, { "name": "recordId", "type": "string | number", "kind": "controlled", "required": false, - "description": "The parent record." + "description": "The parent record whose children are listed." }, { "name": "relationshipField", diff --git a/skills/objectstack-ui/references/react-blocks.md b/skills/objectstack-ui/references/react-blocks.md index 8237d595a8..bbc168de26 100644 --- a/skills/objectstack-ui/references/react-blocks.md +++ b/skills/objectstack-ui/references/react-blocks.md @@ -113,12 +113,12 @@ Highlights panel — a strip of key fields. Config props from the spec RecordHig ## `` — `record:related_list` -Related child records via a lookup. Config props from the spec RecordRelatedList schema. +Related child records via a lookup. `objectName` is the RELATED (child) object whose records are listed — NOT the parent; the parent record is bound by `recordId`, and `relationshipField` is the child field pointing back at it. Config props from the spec RecordRelatedList schema. | prop | type | kind | required | description | |------|------|------|:--------:|-------------| -| `objectName` | `string` | binding | | The parent object. | -| `recordId` | `string \| number` | controlled | | The parent record. | +| `objectName` | `string` | binding | ✓ | The RELATED (child) object whose records this list renders — e.g. objectName="invoice" on an account page. NOT the parent object: the parent record is bound by recordId. | +| `recordId` | `string \| number` | controlled | | The parent record whose children are listed. | | `relationshipField` | `string` | data | ✓ | Field on related object that points to this record (e.g., "account_id") | | `relationshipValueField` | `string` | data | | Parent-record field whose value relationshipField stores (default 'id'; e.g. 'name' for name-keyed junctions). | | `columns` | `string[]` | data | | Fields to display in the related list. Optional: when omitted, columns derive from the related object's highlightFields / default list columns (a related list … | From 9249c3726179dc0a71a567c4822984f58be9206b Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 31 Jul 2026 07:54:05 +0000 Subject: [PATCH 2/2] docs: validating-metadata documents the react block field-prop class; fix a related-list example that named keys nothing reads MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two hand-written docs the docs-drift check flagged, both about the surface this PR changes. `deployment/validating-metadata.mdx` enumerates what `os validate` catches, and after this PR it missed the largest new class. §10 covers it: which block props resolve, that the record:* blocks ride the SAME descriptor table as §5 (so a component is described once and checked wherever it is authored), that is the related child object, and why a filter position gates when everything else advises. §5 gains one line pointing at it. `ui/pages.mdx` authored its record:related_list example as `properties: { object, relationship }`. Neither key exists on RecordRelatedListProps and no conversion-layer alias maps them, so `properties` being unvalidated is the only reason the example parses — an author copying it gets a related list bound to nothing. Corrected to `objectName` / `relationshipField`, with a comment naming which object each one is: the same confusion this PR settles in the contract, one doc over. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_012rK2McPogrTJBKdUsThrGs --- .../docs/deployment/validating-metadata.mdx | 46 ++++++++++++++++++- content/docs/ui/pages.mdx | 6 ++- 2 files changed, 49 insertions(+), 3 deletions(-) diff --git a/content/docs/deployment/validating-metadata.mdx b/content/docs/deployment/validating-metadata.mdx index 932de3dbcd..9acce34f0f 100644 --- a/content/docs/deployment/validating-metadata.mdx +++ b/content/docs/deployment/validating-metadata.mdx @@ -133,7 +133,8 @@ Which object a component binds follows `dataSource.object` → `properties.objec against one page-wide guess. A `record:related_list`'s `columns`/`sort`/`filter` resolve against its **related** object (`objectName`), and its add-picker against its own. Advisory, like form-layout field references — every consumer degrades -rather than failing. +rather than failing. The same descriptor table drives the react page surface +(§10), so a component is described once and checked wherever it is authored. Skipped, to keep false positives at zero: relationship paths (`account.name`, resolved by the query engine), registry-injected system fields (`created_at`, @@ -274,6 +275,49 @@ most-authored surfaces, so the tightening is scheduled on what this check finds rather than assumed. `defineStack` reports the same findings at config-load time, so an author sees them without running the CLI at all. +### 10. React block props naming fields the object doesn't have + +§5 is about a metadata page's untyped `properties` bag. A `kind:'react'` page +authors the **same** components as JSX props, and every prop that binds by field +name has the same failure: the block skips the name and renders one column, one +filter chip, or one form field short. + +```jsx + +// ↑ not a crm_account field → warning +``` + +Checked on every injected block: ``'s +`fields`/`columns`/`sort`/`grouping`/`userFilters`, ``'s `fields`, +`initialValues` keys, `sections[].fields[]` and `subforms` (each against its own +`childObject`), and `` / `` / `` / +`` — those last four through the **same** descriptor table +§5 uses, so a component's field-bearing props are described once and checked on +both surfaces. `` reaches that table by the type the author +writes, so the escape hatch is covered rather than left as a hole. + +`` is the **related (child)** object whose records +are listed — the parent record is bound by `recordId`, and `relationshipField` +is the child's field pointing back at it. Passing the parent there is the +mistake this check was extended to catch. + +A **filter position** is the exception that gates: + +```jsx +