From 361aa398626e2c86f0edd2677047d7a445b5089a Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 10 Aug 2026 10:44:29 +0000 Subject: [PATCH] fix(components): publish `element:record_picker.filter`, the A-class key #3808's triage dropped (#3830) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `filter` appears in objectui#3808'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 gap of exactly the same shape as the four that PR #3841 fixed: `@objectstack/spec` declares `ElementRecordPickerProps.filter`, the renderer has read it all along (`composed?.filter ?? props.filter` -> `query.$filter`), and the registry `inputs` never mentioned it. `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`. Verified end to end rather than argued: with the declaration reverted, `compile()` over the live registration returns ` has no prop "filter"` on a key the renderer then filters the entire candidate set by. The description is derived from what the renderer DOES, 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. `type: 'object'`, from the spec's actual shape on the resolved pin and not the `'array'` the issue's landing sketch guessed. `filter` is `FilterConditionSchema`, i.e. `z.record(z.string(), z.unknown())` intersected with the `$and`/`$or`/`$not` group, so a rule array is rejected — measured with `safeParse`, and `sdui-parser`'s `checkType` object arm draws exactly the same partition. This is the one key in the family where `ComponentInput`'s coarse typing costs nothing, so unlike `element:text_input.defaultValue` there is no narrowing to disclose in the description (data point for objectui#3832, not a widening of this card). The gate's explicit exemption for this key is deleted in the same change, as its own `carries no stale unpublished-key exemption` assertion demands, and the key joins #3808's four in the by-name "declared, not merely not-failing" pin — now five. Reverse verification (declaration reverted, exemption removal kept): element:record_picker publishes every top-level key its spec props schema declares -> AssertionError: expected [ 'filter' ] to deeply equal [] the five A-class keys objectui#3808 / #3830 declared -> does not publish filter a JSX page writing `filter` -> `has no prop "filter"` (control: `searchFields` still reported, so the probe is not silently vacuous) 7 failed | 50 passed, restored to 57 passed. Verified: vitest packages/components/ + 4 console gates -> 116 files, 1111 tests passed vitest packages/sdui-parser/ + RefComponentWidget -> 5 files, 24 passed type-check @object-ui/components + @object-ui/console -> Done (after building the console dependency closure; the first run's TS2882s were stale artifacts) eslint on the three touched files -> 0 errors (12 pre-existing warnings in the untouched effect body) check-control-bytes / check-changeset-presence / -no-major / -fixed -> OK Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_017Qqyix2QcnpUC9XeYVDzx3 --- .changeset/record-picker-filter-input-3830.md | 41 ++++ .../registry-inputs-spec-parity.test.ts | 36 ++-- .../record-picker-inputs-spec-parity.test.ts | 202 ++++++++++++++++++ .../src/renderers/basic/record-picker.tsx | 38 ++++ 4 files changed, 302 insertions(+), 15 deletions(-) create mode 100644 .changeset/record-picker-filter-input-3830.md create mode 100644 packages/components/src/__tests__/record-picker-inputs-spec-parity.test.ts 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' },