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/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 +` 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 … |