diff --git a/.changeset/record-picker-filter-input-3830.md b/.changeset/record-picker-filter-input-3830.md new file mode 100644 index 0000000000..fc2bce209f --- /dev/null +++ b/.changeset/record-picker-filter-input-3830.md @@ -0,0 +1,41 @@ +--- +"@object-ui/components": patch +--- + +`element:record_picker.filter` is now discoverable from the published `inputs` + +The fourth A-class gap of objectui#3808's own list, and the one its three-way +triage dropped: `filter` appears in that issue's raw key dump for this block and +then in none of its A / B / C lists, so the change that added the repo-wide +parity gate exempted it by name instead of declaring it. It is the same shape as +the four #3808 fixed — `@objectstack/spec` declares +`ElementRecordPickerProps.filter`, the renderer has read it all along +(`composed?.filter ?? props.filter`, straight into the picker query's `$filter`), +and the registry `inputs` never mentioned it. + +`element:record_picker` is not in the public tier ("record picking is a field +widget, not a page block"), so the gap was not in `sdui.manifest.json` — it was +in the JSX-page compiler's prop whitelist, which `renderers/layout/page.tsx` +builds from `getKnownTypes()` plus these same `inputs`. A JSX page writing +`filter` therefore got an `unknown-prop` warning from `sdui-parser`'s prop walk +on the very key that decided which records the picker offered, and the designer +panel gave an author no way to discover the key existed at all. + +The description is derived from what the renderer does, not from restating the +spec's one-liner, because the one thing an author cannot read off the spec is +which of the two places they may write a filter wins: a node-level `dataSource` +filter (itself AND-combined with any saved `view` it names) is taken and this +top-level `filter` is DROPPED, not merged — so this key applies only when the +node carries no `dataSource` filter. + +`type` is `'object'`, taken from the spec's actual shape on the resolved pin +rather than the `'array'` the issue's landing sketch guessed: +`FilterConditionSchema` is `z.record(z.string(), z.unknown())` intersected with +the `$and` / `$or` / `$not` group, so a rule array is rejected. This is the one +key in the family where `ComponentInput`'s coarse typing costs nothing — +`sdui-parser`'s `checkType` accepts exactly the values the spec accepts here, so +unlike `element:text_input.defaultValue` there is no narrowing to disclose. + +The parity gate's explicit exemption for this key is deleted in the same change +(its own `carries no stale unpublished-key exemption` assertion demands it), and +the key joins #3808's four in the by-name "declared, not merely not-failing" pin. diff --git a/apps/console/src/__tests__/registry-inputs-spec-parity.test.ts b/apps/console/src/__tests__/registry-inputs-spec-parity.test.ts index 0a9d69daa1..57b5870914 100644 --- a/apps/console/src/__tests__/registry-inputs-spec-parity.test.ts +++ b/apps/console/src/__tests__/registry-inputs-spec-parity.test.ts @@ -511,17 +511,12 @@ const UNPUBLISHED_EXEMPTIONS: Record = { 'page:card.body': 'Retired upstream by objectstack#5775 / PR objectstack#6281 (ADR-0087 D2 tombstone, converging on the `children` this renderer reads and now publishes); declaring it would publish a key the spec rejects by name — objectui#4027. Listed here only because the pinned @objectstack/spec@17.0.0-rc.5 predates the retirement. Resolves via objectui#3809, not via the pin bump.', - // ── element:record_picker.filter — a real A-class gap, out of scope here ─── - // The renderer DOES read it (`record-picker.tsx:78`, `ds.filter ?? props.filter`, - // into `query.$filter` at :103) and the spec DOES declare it, so by the bar - // above this key should be declared, not exempted. It is exempted because - // objectui#3808's own three-class triage never sorted it into A, B or C — it - // appears in that issue's raw key dump and then in none of the three lists — - // so it fell outside the dispatched scope of the change that added this gate. - // Filed as objectui#3830 with the same evidence, rather than widened into a - // PR nobody reviewed for it. - 'element:record_picker.filter': - 'A genuine A-class gap (renderer reads it at record-picker.tsx:78 → query.$filter at :103), not a deliberate omission — it fell out of objectui#3808\'s three-class triage and so out of that PR\'s scope. Owned by objectui#3830; delete this entry when it declares the input.', + // `element:record_picker.filter` was the ninth entry here — a real A-class gap + // that fell out of objectui#3808's three-class triage, exempted only because it + // was outside that PR's dispatched scope. objectui#3830 declared the input, so + // the entry stopped describing anything and `carries no stale unpublished-key + // exemption` demanded its deletion. It is now pinned as DECLARED, by name, + // alongside #3808's four at the bottom of this file. // ── targetVariable — the spec's own "declarative hint" (2 keys) ──────────── // Zero read points repo-wide (`grep -rn targetVariable packages/ apps/` is @@ -676,8 +671,10 @@ describe('registry `inputs` vs `@objectstack/spec` ComponentPropsMap (repo-wide) it('every unpublished-key exemption states a reason and references a tracking issue', () => { // The discipline that separates "deliberately not published, and here is who - // owns the decision" from "we forgot". Four of the nine entries below exist - // only because objectui#3829 / #3830 / #3834 were opened to own them. + // owns the decision" from "we forgot". Four of the nine entries once here + // existed only because objectui#3829 / #3830 / #3834 were opened to own + // them, and #3830's is already gone — declaring the input is what retires an + // entry, which is the point of the stale check below. const unjustified = Object.entries(UNPUBLISHED_EXEMPTIONS) .filter(([, reason]) => !/#\d+/.test(reason)) .map(([key]) => key); @@ -699,17 +696,26 @@ describe('registry `inputs` vs `@objectstack/spec` ComponentPropsMap (repo-wide) expect(stale).toEqual([]); }); - it('the four keys objectui#3808 declared are discoverable, block by block', () => { + it('the five A-class keys objectui#3808 / #3830 declared are discoverable, block by block', () => { // Named, not just covered by the derived loop above. The derived assertion - // would also pass if these four were added to `UNPUBLISHED_EXEMPTIONS` + // would also pass if these five were added to `UNPUBLISHED_EXEMPTIONS` // instead of declared — which is precisely the move #3808 exists to rule // out — so the keys it fixed are pinned by name, and pinned as DECLARED // rather than merely "not failing". + // + // The fifth is objectui#3830's `element:record_picker.filter`, the A-class + // key #3808's own triage dropped between its raw key dump and its three + // lists. It is listed HERE, in the same place as the other four, because it + // is the same fact about the same gate: the entry that used to exempt it + // (deleted above) is not evidence of anything once the input exists, and a + // future change that dropped the declaration and re-added the exemption + // would restore the gap while leaving every derived assertion green. const fixed: Array<[string, string]> = [ ['record:details', 'hideFields'], ['record:related_list', 'relationshipValueField'], ['record:related_list', 'add'], ['element:text_input', 'defaultValue'], + ['element:record_picker', 'filter'], ]; for (const [type, key] of fixed) { expect(specTopLevelKeys(type), `${type} spec no longer declares ${key}`).toContain(key); diff --git a/packages/components/src/__tests__/record-picker-inputs-spec-parity.test.ts b/packages/components/src/__tests__/record-picker-inputs-spec-parity.test.ts new file mode 100644 index 0000000000..decbc08d2e --- /dev/null +++ b/packages/components/src/__tests__/record-picker-inputs-spec-parity.test.ts @@ -0,0 +1,202 @@ +/** + * ObjectUI + * Copyright (c) 2024-present ObjectStack Inc. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + * + * `element:record_picker` — the published authoring surface stays in parity with + * `@objectstack/spec` `ElementRecordPickerProps` for `filter` (objectui#3830). + * + * The sibling of `text-input-inputs-spec-parity.test.ts`, for the key that fell + * out of objectui#3808's own three-class triage: `filter` appears in that + * issue's raw key dump for this block and then in none of its A / B / C lists, + * so the change that added the repo-wide parity gate exempted it by name instead + * of declaring it. It is the fourth A-class gap of exactly the same shape — + * renderer reads it, spec declares it, `inputs` omitted it. + * + * WHY THIS BLOCK NEEDED IT. `element:record_picker` is deliberately NOT in + * `PUBLIC_BLOCKS` ("record picking is a field widget, not a page block", + * `packages/core/src/registry/public-blocks.ts`), so it never reaches + * `sdui.manifest.json` and the usual argument — "the manifest advertises it" — + * does not apply. Its `inputs` are a live prop whitelist anyway: + * `renderers/layout/page.tsx` builds the JSX-page compiler's whitelist from + * `getKnownTypes()` plus these same `inputs`, so while `filter` was undeclared, + * `sdui-parser/src/validate.ts` reported `unknown-prop` for it on every JSX + * page — a warning against a key the renderer then filtered the picker's whole + * candidate set by. The last test in this file is that path, end to end. + * + * Expectations are derived from the spec at runtime, not restated. + */ + +import { describe, it, expect } from 'vitest'; +import { ComponentRegistry } from '@object-ui/core'; +import { compile, manifestFromConfigs } from '@object-ui/sdui-parser'; +import { ElementRecordPickerPropsSchema } from '@objectstack/spec/ui'; +// Module scope, not a hook: the cold transform is billed to the import phase, +// which has no test/hook timeout (AGENTS.md §测试纪律, objectui#3010). +import '../renderers'; + +type ShapeCarrier = { shape?: unknown; _def?: { shape?: unknown } }; + +/** Resolve the props object's `.shape` through both spellings, lazy or plain. */ +function specTopLevelKeys(): string[] { + const carrier = ElementRecordPickerPropsSchema as unknown as ShapeCarrier; + const shape = carrier.shape ?? carrier._def?.shape; + const resolved = typeof shape === 'function' ? (shape as () => object)() : shape; + return resolved && typeof resolved === 'object' ? Object.keys(resolved) : []; +} + +const TYPE = 'element:record_picker'; +const config = () => ComponentRegistry.getConfig(TYPE); +const inputs = () => config()?.inputs ?? []; +const inputNames = () => inputs().map((i) => i.name); +const input = (name: string) => inputs().find((i) => i.name === name); +const filterDescription = () => input('filter')?.description ?? ''; + +/** A minimal spec-valid props object, so a `filter` probe fails only on `filter`. */ +const withFilter = (filter: unknown) => ({ object: 'account', displayField: 'name', filter }); + +describe('element:record_picker — registry inputs vs @objectstack/spec', () => { + it('is registered with a non-empty `inputs` surface', () => { + expect(config()).toBeDefined(); + expect(inputNames().length).toBeGreaterThan(0); + }); + + it('resolves a non-empty spec key set', () => { + // Guards the probe, not the subject: a Zod internals change would return `[]` + // here and make every assertion below vacuously agreeable. + expect(specTopLevelKeys().length).toBeGreaterThan(0); + }); + + it('publishes `filter`, which the renderer has read all along', () => { + // A KEY-reachability claim, so the criterion is that the key SURVIVES the + // parse — not that the parse succeeds. This props schema is a strip-mode + // `z.object`, so an UNDECLARED key parses green too and is simply absent + // from `data` afterwards; asserting `success` alone would prove nothing. + expect(specTopLevelKeys()).toContain('filter'); + const parsed = ElementRecordPickerPropsSchema.safeParse(withFilter({ status: 'open' })); + expect(parsed.success).toBe(true); + expect(parsed.data?.filter).toEqual({ status: 'open' }); + + // The contrast that makes the criterion meaningful: same green parse, key + // gone, no diagnostic. That is what `filter` looked like to every manifest + // consumer before it was declared here. + const undeclared = ElementRecordPickerPropsSchema.safeParse({ + object: 'account', + displayField: 'name', + notASpecKey: 1, + } as never); + expect(undeclared.success).toBe(true); + expect(Object.keys(undeclared.data ?? {})).not.toContain('notASpecKey'); + + expect(inputNames()).toContain('filter'); + expect(filterDescription()).not.toBe(''); + }); + + it('declares `object` as the type the spec actually accepts, not `array`', () => { + // objectui#3830's landing sketch guessed `'array'` and flagged the guess as + // needing checking against the resolved pin. It is wrong, and this is why: + // `ElementRecordPickerProps.filter` is `FilterConditionSchema`, which is + // `z.record(z.string(), z.unknown()).and(z.object({ $and, $or, $not }))` — + // an OBJECT. A rule array (an ObjectQL AST, a view's `ViewFilterRule[]`) is + // rejected outright. + expect(ElementRecordPickerPropsSchema.safeParse(withFilter({ status: 'open' })).success).toBe(true); + expect(ElementRecordPickerPropsSchema.safeParse(withFilter({ $and: [{ a: 1 }] })).success).toBe(true); + expect(ElementRecordPickerPropsSchema.safeParse(withFilter([['a', '=', 1]])).success).toBe(false); + expect(ElementRecordPickerPropsSchema.safeParse(withFilter('a = 1')).success).toBe(false); + expect(ElementRecordPickerPropsSchema.safeParse(withFilter(42)).success).toBe(false); + + expect(input('filter')?.type).toBe('object'); + }); + + it('the coarse `object` type costs nothing here — it accepts exactly what the spec accepts', () => { + // The `element:text_input.defaultValue` sibling had to name a narrowing in + // prose, because `ComponentInput.type` is one coarse control kind and the + // spec's type there is the union `string | number` (objectui#3832). This key + // is the case where the two agree exactly: `checkType`'s `'object'` arm in + // `sdui-parser/src/validate.ts` passes a non-null non-array object and warns + // `type-mismatch` on everything else — the same partition `safeParse` draws + // above. Asserted through the real validator, not by reading its source, so + // a future widening of either side shows up here as a disagreement. + const manifest = manifestFromConfigs([ + { type: TYPE, namespace: 'element', inputs: [{ name: 'filter', type: 'object' }] }, + ]); + const codesFor = (literal: string) => + compile(`<${TYPE} filter={${literal}} />`, manifest).diagnostics.map((d) => d.code); + + expect(codesFor('{"status":"open"}')).toEqual([]); + expect(codesFor('{"$and":[{"a":1}]}')).toEqual([]); + expect(codesFor('[["a","=",1]]')).toContain('type-mismatch'); + expect(codesFor('42')).toContain('type-mismatch'); + expect(codesFor('null')).toContain('type-mismatch'); + }); + + it('the `filter` description says which of the two filters an author writes wins', () => { + // The renderer reads `composed?.filter ?? props.filter`, so a node that + // carries a `dataSource` filter DROPS this key rather than combining with + // it — while the binding's own filter AND-combines with the saved view it + // names. A description saying only "filter criteria" would be true and + // useless: an author writing both would have no way to know which one + // decides the candidate set, which is the one thing objectui#3830 insists + // this entry has to state. + const description = filterDescription(); + expect(description).toMatch(/dataSource/); + expect(description).toMatch(/precedence/i); + expect(description).toMatch(/\$filter/); + }); + + it('carries no `defaultValue` on the filter entry', () => { + // A default here would pre-fill every picker in the designer with a filter + // the renderer has no opinion about — and a filter's default is not "empty + // object", it is "no filter at all", which `undefined` already is. The spec + // declares no default either. + // + // Existence asserted first: `input('filter')?.defaultValue` is also + // `undefined` when the input is GONE, so without this line the check would + // pass most loudly in the one case it is supposed to notice. + expect(input('filter')).toBeDefined(); + expect(input('filter')?.defaultValue).toBeUndefined(); + expect( + ElementRecordPickerPropsSchema.safeParse({ object: 'account', displayField: 'name' }).data, + ).not.toHaveProperty('filter'); + }); + + it('a JSX page writing `filter` no longer gets `unknown-prop` from the compiler', () => { + // The harm objectui#3830 describes, end to end. The manifest is assembled + // the way `renderers/layout/page.tsx` assembles the JSX-page compiler's + // whitelist — `getKnownTypes()` mapped through each type's registered meta — + // so this runs against the LIVE registration, not a hand-written fixture + // that could agree with itself. + const manifest = manifestFromConfigs( + ComponentRegistry.getKnownTypes().map((t) => { + const meta = ComponentRegistry.getMeta(t); + return { type: t, namespace: meta?.namespace, isContainer: meta?.isContainer, inputs: meta?.inputs }; + }) as unknown as Parameters[0], + ); + + // Non-vacuity, and the reason this test can fail for the right reason: the + // SAME compile call carries `searchFields`, a spec key this block + // deliberately does not publish (an ADR-0087 tombstone upstream — see the + // exemptions in `apps/console/src/__tests__/registry-inputs-spec-parity.test.ts`). + // It must still come back as `unknown-prop`. Without this control, "no + // unknown-prop for filter" would also be what a broken manifest, an + // unregistered tag or a silent parse failure looks like. + const r = compile( + `<${TYPE} object="account" filter={{"status":"open"}} searchFields={["name"]} />`, + manifest, + ); + + const unknownProps = r.diagnostics + .filter((d) => d.code === 'unknown-prop') + .map((d) => d.message); + expect(unknownProps.join(' | ')).toMatch(/searchFields/); + expect(unknownProps.join(' | ')).not.toMatch(/"filter"/); + + // And the key survives into the compiled tree as itself — the whole point of + // publishing it is that the author's `filter` reaches the renderer, which + // turns it into the picker query's `$filter`. + expect(r.tree).toMatchObject({ type: TYPE, filter: { status: 'open' } }); + expect(r.diagnostics.some((d) => d.severity === 'error')).toBe(false); + }); +}); diff --git a/packages/components/src/renderers/basic/record-picker.tsx b/packages/components/src/renderers/basic/record-picker.tsx index bb82d4eacf..90f66a01bb 100644 --- a/packages/components/src/renderers/basic/record-picker.tsx +++ b/packages/components/src/renderers/basic/record-picker.tsx @@ -221,8 +221,46 @@ ComponentRegistry.register('record_picker', ElementRecordPickerRenderer, { skipFallback: true, label: 'Record Picker', category: 'input', + // `filter` is DECLARED, not merely honoured (objectui#3830) — the fourth key + // of objectui#3808's A class, which that issue's own three-way triage dropped + // between the raw key dump and the lists. The renderer has read it all along + // (`composed?.filter ?? props.filter` above, into `query.$filter`), and the + // spec declares it (`ElementRecordPickerProps.filter`), but while it was + // missing from this list every layer that reads a manifest said the opposite: + // `element:record_picker` is not in `PUBLIC_BLOCKS` ("record picking is a + // field widget, not a page block"), so the gap was not in `sdui.manifest.json` + // — it was in the JSX-page compiler's prop whitelist, which + // `renderers/layout/page.tsx` builds from `getKnownTypes()` plus these same + // `inputs`. A JSX page writing `filter` got an `unknown-prop` warning from + // `sdui-parser/src/validate.ts` on a key the renderer then went on to filter + // by. That is objectui#3407 in the same shape as `readonly` — honoured, + // undiscoverable — and the reverse half of the parity gate in + // `apps/console/src/__tests__/registry-inputs-spec-parity.test.ts`, whose + // explicit exemption for this key is deleted by the same change. inputs: [ { name: 'object', type: 'string', label: 'Object' }, + { + name: 'filter', + // `'object'` is the spec's shape, not a chosen arm. `filter` is + // `FilterConditionSchema.optional()`, and that schema is + // `z.record(z.string(), z.unknown()).and(z.object({ $and, $or, $not }))` + // — a plain object. `checkType`'s `'object'` case in + // `sdui-parser/src/validate.ts` accepts exactly what the spec accepts + // here (a non-null non-array object) and rejects exactly what it rejects + // (arrays, strings, numbers, booleans — all verified against + // `ElementRecordPickerPropsSchema.safeParse` in the parity test next to + // this file). So this is the one case in the family where + // `ComponentInput`'s coarse typing costs nothing: no narrowing to name in + // the description, unlike `element:text_input.defaultValue`'s + // `string | number` (objectui#3832). + type: 'object', + label: 'Filter', + // Taken from what the renderer DOES with the key, because the one thing + // an author cannot read off the spec is which of the two places they may + // write a filter actually wins. + description: + 'Filter criteria narrowing which records the picker offers, as a spec FilterCondition object — `{ status: "open" }`, or `{ $and: [ … ] }` for a group. It becomes the `$filter` of the picker\'s own query, so it decides which records exist for the user, not merely how they are shown. PRECEDENCE: a node-level `dataSource` binding wins outright. The renderer reads `dataSource.filter ?? filter`, so when the binding — or the saved view its `view` names, which AND-combine with each other because the spec calls the binding\'s filter *additional* — supplies a filter, THIS key is dropped entirely rather than merged into it; it applies only when the node carries no `dataSource`, or that `dataSource` and its view both leave `filter` unset. A rule ARRAY (an ObjectQL AST, or a view\'s rule list) is not a FilterCondition and the spec rejects it here.', + }, { name: 'labelField', type: 'string', label: 'Label Field' }, { name: 'valueField', type: 'string', label: 'Value Field' }, { name: 'placeholder', type: 'string', label: 'Placeholder' },