Repository navigation
Commit c8ed4c9
fix(components): filter-builder renderer hands onChange the declared FilterGroup (#26590)
<!-- claude-projects-attribution: {"github_login":"os-steve"} -->
_Requested by **Steve** · [project
thread](https://claude.ai/code/project/chan_01FdL1XeUyvVNxLreh3F4Qi9?thread=cmsg_01FdL1XeUyvVNxLreh3F4Qi979uT1nAyKwVX3CFRQGb43w)_
Fixes #26382
Clause-②: no
Metadata-protocol: none
## 维护者速读
- **改了什么**:`filter-builder` 渲染器调用 `onChange` 时直接传 `FilterGroup`,不再传 `{
target: { name, value } }` 表单事件;渲染器自己的 `onChange` 类型改为
`FilterBuilderSchema['onChange']`,去掉了那个 `any`。
- **为什么改**:schema 声明 `onChange(filter:
FilterGroup)`,渲染器却传事件对象,按声明写的处理函数读 `.logic` / `.conditions` 只会拿到
`undefined`,而 `any` 让编译器看不见。仓内没有任何宿主按事件形状绑定这个节点,所以以声明为准。
- **风险与回滚**:低。仓外若有宿主读 `event.target.value`,要改成直接读参数(changeset
已写明)。宿主经节点转发的 `extraOperators` 不再传进构建器,所以本节点永远不提供 `exists` /
`notExists`;若存储的 value 绕过校验仍带这类运算符,渲染器不调用 `onChange`,并在 console
报出该运算符,不会抛错,也不会传出错的形状。回滚即 revert 本 PR。
-
**先看**:`ui/packages/components/src/renderers/complex/filter-builder.tsx`(`extraOperators`
的解构、`isAuthoredFilterGroup` 与
`handleChange`);`ui/packages/components/src/__tests__/filter-builder-renderer-onchange-26382.test.tsx`。
- **两分钟验证**:仓根执行 `pnpm --dir ui exec vitest run
packages/components/src/__tests__/filter-builder-renderer-onchange-26382.test.tsx`,预期
5 个测试全绿。
- **需要你做的**:无,席位落地。
## What changes
- `ui/packages/components/src/renderers/complex/filter-builder.tsx`: the
renderer's `onChange` prop is typed `FilterBuilderSchema['onChange']`
instead of `(event: any) => void`, and `handleChange` is typed on the
builder's `FilterBuilderRowGroup` instead of `any`. It calls
`onChange(group)` with the group itself.
- The builder's row type admits two opt-in operators (`exists` /
`notExists`) that `FilterGroup` cannot carry, so `FilterBuilderRowGroup`
is not assignable to `FilterGroup` (measured, below). The renderer
narrows through a type predicate, `isAuthoredFilterGroup`, that checks
every row's operator against the spec's `VIEW_FILTER_OPERATORS`.
- The renderer takes `extraOperators` out of the host spread and never
forwards it to `FilterBuilder`. It is the builder's only
operator-granting prop, `FilterBuilderSchema` declares no such key, and
what it grants (`exists` / `notExists`) is outside the declared
`FilterGroup`. So the node's dropdown offers only protocol operators,
and the off-contract branch cannot be reached from the node.
- The one way left into that branch is a stored `value` row that skipped
the `FilterGroup` zod mirror, which refuses such an operator. There the
renderer does not call `onChange` and logs a `console.error` naming the
operator. It does not hand on a group the declared type does not
describe, and it does not throw inside a click handler.
`isAuthoredFilterGroup` stays as the type guard. No `as any` or other
cast remains on the path.
- `FilterBuilderSchema` in `@object-ui/types` is unchanged: the
declaration is the contract.
- One root changeset,
`.changeset/26382-filter-builder-onchange-group.md`
(`@object-ui/components` patch).
### Step 1: which side is the contract (the triage's measurement)
Searched `ui/` and `packages/` on `origin/main` for every host that
could bind a `filter-builder` node's `onChange` through the event shape.
Nothing does, so the renderer adopts the declared `FilterGroup`
(`Clause-②: no`).
- **Form field hosts:** `renderFieldComponent` in
`ui/packages/components/src/renderers/form/form.tsx` resolves only
`field:`-namespaced widgets for a form field. The `filter-builder` node
is registered in the `ui` namespace, and no `field:filter-builder`
registration exists. A form field never reaches this renderer.
- **SchemaRenderer host props:** `SchemaRenderer` spreads the host's
`...props` last, so a host's `onChange` would reach the renderer. The
only in-repo hosts that pass `onChange` to `SchemaRenderer` are two
cases in `plugin-view`'s
`handlerEventNameThroughRenderer-10616.test.tsx`, and neither renders a
`filter-builder` node.
- **Metadata-admin `filter-builder` widget:** the spec's form rows
(`view.form.ts`, `page.form.ts`, `dataset.form.ts`) name `widget:
'filter-builder'`. That key resolves to `FilterBuilderWidget` in
app-shell's `metadata-admin/widgets.tsx`, which draws
`FilterBuilderField` over `FilterRuleLite[]`, and
`ObjectListViewInspector` reads the same `WIDGETS` entry. Neither
renders the `filter-builder` SDUI node, so neither reaches this
`onChange`.
- **Docs and examples:**
`ui/content/docs/components/complex/filter-builder.mdx` declares
`onChange?: (filter: FilterGroup) => void`, the same as the type. The
five `ui/examples/schema-catalog` JSON documents carry no function, and
their tests (`filter-builder-mirror-6939`,
`filter-builder-operator-alias-trigger-7561`,
`filter-builder-operator-vocabulary-6939`) read no `onChange`.
- **Other hits:** the `@object-ui/types` tests,
`inline-locale-label-read-sites.test.tsx`, `known-schema-types.ts`,
`widget-dom-leak-sweep.test.tsx` and `shadcn-components.json` name the
type but bind no `onChange` and read no `target`.
- Not searchable from here: the `objectstack-ai/cloud` and `hotcrm`
repositories.
## Tests
- New
`ui/packages/components/src/__tests__/filter-builder-renderer-onchange-26382.test.tsx`
invokes the registered renderer directly:
- a host `onChange` gets the group, with no `target`, the group's
`logic`, the existing row and the added row;
- a handler typed from `FilterBuilderSchema`'s own `onChange` (checked
with `satisfies`), the channel a node-authored `onChange` arrives on,
gets the same group;
- a host forwarding `extraOperators: ['exists', 'notExists']` through
the node gets an operator dropdown that offers neither `Is set` nor `Is
not set` (and does offer `Is empty`, so the dropdown really opened);
- a stored row holding `exists` (off contract) makes the renderer skip
`onChange` and log one `console.error` naming the operator;
- type level: `expectTypeOf` pins `FilterBuilderSchema['onChange']`'s
parameter EQUAL to `FilterGroup`, and a `@ts-expect-error` pins that a
handler taking the old `{ target: { name, value } }` event is not
assignable to it. The file is compiled by the components
`tsconfig.test.json` project, which `type-check` runs.
- No existing test needed an edit.
## Acceptance notes
- Seat ruling on the off-contract branch: close it at its source, so the
renderer stops forwarding `extraOperators`. On the remaining branch, the
choice was a throw kept as an invariant, or `console.error` and skip the
emit. This PR picks `console.error` and skip. The branch is not fully
unreachable (a stored `value` that skipped validation still reaches it),
so a throw there would still be a crash in the user's hands.
- The renderer still spreads the other undeclared host props into
`FilterBuilder`, as before. `extraOperators` is the only one that grants
operators, and it is now held back. No in-repo host forwarded it.
## Verification
<details><summary>components type-check (tsc --noEmit, then tsc -p
tsconfig.test.json)</summary>
`os-verify-lock: VERDICT command-exit 0`, with the new test file in the
test project.
</details>
<details><summary>vitest: new file plus
inline-locale-label-read-sites</summary>
`Test Files 2 passed (2)`, `Tests 10 passed (10)`; after the
extraOperators round: `Test Files 2 passed (2)`, `Tests 12 passed (12)`
(the new file: 5).
</details>
<details><summary>Ablation 1: call reverted to the event shape</summary>
`scripts/ablation-replace.mjs` replaced `onChange(group);` with the old
`{ target: { name, value } }` call (anchor 1 → 0, blob `96162878fc0f` →
`ece34562e098`). Two tests went red: `expected { target: { name:
'filters', …(1) } } to not have property "target"` and `Target cannot be
null or undefined.` (`Tests 2 failed | 2 passed`). Restored: blob equal
to HEAD, `git diff HEAD` empty.
</details>
<details><summary>Ablation 3: extraOperators forwarded again</summary>
`extraOperators: _refusedExtraOperators,` removed from the destructure,
so the spread forwards it again (blob `5645f25cbc23` → `57047d781a2f`):
`expected [ 'Equals', 'Does not equal', …(11) ] to not include 'Is
set'`, `Tests 1 failed | 4 passed (5)`. Restored: blob equal to HEAD,
`git diff HEAD` empty.
</details>
<details><summary>Ablation 4: skip-emit return removed</summary>
The `return;` after the `console.error` replaced with a no-op marker
(blob `5645f25cbc23` → `8a937573edee`): `expected "vi.fn()" to not be
called at all, but actually been called 1 times`, `Tests 1 failed | 4
passed (5)`. Restored: blob equal to HEAD. A first attempt was refused
by `ablation-replace` before any run, because its replacement text was a
substring of the anchor (the count did not rise), so it was a no-op and
not a reading.
</details>
<details><summary>Ablation 2: type guard removed (is it
load-bearing?)</summary>
With the predicate replaced by `false as boolean`, `tsc --noEmit`
reported `TS2345: Argument of type 'FilterBuilderRowGroup' is not
assignable to parameter of type 'FilterGroup'`, down to `Type '"exists"'
is not assignable to …`. Restored: blob equal to HEAD.
</details>
<details><summary>Full components suite</summary>
`pnpm --filter @object-ui/components test --maxWorkers=2` under the lock
at `070b433f`: `Test Files 438 passed | 1 skipped (439)`, `Tests 4273
passed | 24 skipped (4297)`, `VERDICT command-exit 0`.
After the extraOperators round, narrowed and declared as such at
`904e58cf`: every `filter-builder*` file in
`packages/components/src/__tests__/` (22) plus
`inline-locale-label-read-sites`: `Test Files 23 passed (23)`, `Tests
392 passed (392)`. components `type-check` at `904e58cf`: `VERDICT
command-exit 0`. The full suite is left to CI's Test UI.
</details>
<details><summary>Derived gates at 904e58c (dispatch-gates --commands,
then --ran)</summary>
Re-run in full at the final head, after merging `origin/main` (the
derivation flagged a stale tree; main touched none of this PR's files
and the derived list was the same 34). All 34 exit 0; `dispatch-gates
--ran`: `34 derived famil(ies) accounted for — 34 run, 0 NOT-MEASURED (a
DERIVED zero — all 34 recorded an exit code and none of them is 3)`.
`pnpm --dir ui exec vitest run scripts/`: `Test Files 178 passed | 2
skipped (180)`. `check-adr-0087-registration`: no declared-breaking
changeset.
```text
node scripts/check-adr-0087-registration.mjs --base origin/main :: exit 0
node scripts/check-adr-0087-registration.mjs --self-test :: exit 0
node scripts/check-changeset-no-major.mjs --base origin/main :: exit 0
node scripts/check-closing-keyword-parity.mjs :: exit 0
node scripts/check-closing-keyword-parity.mjs --self-test :: exit 0
node scripts/check-comment-mask-corpus.mjs :: exit 0
node scripts/check-empty-changeset.mjs --base origin/main :: exit 0
node scripts/check-empty-changeset.mjs --self-test :: exit 0
node scripts/check-sdui-manifest.mjs :: exit 0
node scripts/check-sdui-manifest.mjs --self-test :: exit 0
node scripts/pm/release-rehearsal-clone.mjs --self-test :: exit 0
node scripts/release-pending-publish.mjs --self-test :: exit 0
pnpm --dir ui exec vitest run scripts/ :: exit 0
pnpm --filter @objectstack/spec run check:duration-unit-keys :: exit 0
pnpm check:changeset-gate-self-tests :: exit 0
pnpm check:closed-ceremony-text :: exit 0
pnpm check:cross-package-test-inputs :: exit 0
pnpm check:dispatcher-error-vocabulary :: exit 0
pnpm check:driver-memory-census :: exit 0
pnpm check:engine-double-contract :: exit 0
pnpm check:gitlink-declared :: exit 0
pnpm check:nul-bytes :: exit 0
pnpm check:pm-changeset-deadline-census :: exit 0
pnpm check:published-files :: exit 0
pnpm check:query-options-erasure :: exit 0
pnpm check:refd-timer-probe :: exit 0
pnpm check:skill-identifier-liveness :: exit 0
pnpm check:test-source-alias :: exit 0
pnpm check:tier-file-adoption :: exit 0
pnpm check:type-check-coverage :: exit 0
pnpm check:type-check-debt :: exit 0
pnpm check:undeclared-dep-imports :: exit 0
pnpm check:watch-hint-literal :: exit 0
pnpm check:where-matcher :: exit 0
```
</details>
---
_Generated by [Claude
Code](https://claude.ai/code/session_01ADHkmNhEw4jVVQ4UTxrxMZ)_
---------
Co-authored-by: Claude <noreply@anthropic.com>1 parent c68869c commit c8ed4c9
3 files changed
Lines changed: 207 additions & 11 deletions
File tree
- .changeset
- ui/packages/components/src
- __tests__
- renderers/complex
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
| 1 | + | |
| 2 | + | |
| 3 | + | |
| 4 | + | |
| 5 | + | |
| 6 | + | |
| 7 | + | |
| 8 | + | |
| 9 | + | |
| 10 | + | |
| 11 | + | |
| 12 | + | |
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
| 1 | + | |
| 2 | + | |
| 3 | + | |
| 4 | + | |
| 5 | + | |
| 6 | + | |
| 7 | + | |
| 8 | + | |
| 9 | + | |
| 10 | + | |
| 11 | + | |
| 12 | + | |
| 13 | + | |
| 14 | + | |
| 15 | + | |
| 16 | + | |
| 17 | + | |
| 18 | + | |
| 19 | + | |
| 20 | + | |
| 21 | + | |
| 22 | + | |
| 23 | + | |
| 24 | + | |
| 25 | + | |
| 26 | + | |
| 27 | + | |
| 28 | + | |
| 29 | + | |
| 30 | + | |
| 31 | + | |
| 32 | + | |
| 33 | + | |
| 34 | + | |
| 35 | + | |
| 36 | + | |
| 37 | + | |
| 38 | + | |
| 39 | + | |
| 40 | + | |
| 41 | + | |
| 42 | + | |
| 43 | + | |
| 44 | + | |
| 45 | + | |
| 46 | + | |
| 47 | + | |
| 48 | + | |
| 49 | + | |
| 50 | + | |
| 51 | + | |
| 52 | + | |
| 53 | + | |
| 54 | + | |
| 55 | + | |
| 56 | + | |
| 57 | + | |
| 58 | + | |
| 59 | + | |
| 60 | + | |
| 61 | + | |
| 62 | + | |
| 63 | + | |
| 64 | + | |
| 65 | + | |
| 66 | + | |
| 67 | + | |
| 68 | + | |
| 69 | + | |
| 70 | + | |
| 71 | + | |
| 72 | + | |
| 73 | + | |
| 74 | + | |
| 75 | + | |
| 76 | + | |
| 77 | + | |
| 78 | + | |
| 79 | + | |
| 80 | + | |
| 81 | + | |
| 82 | + | |
| 83 | + | |
| 84 | + | |
| 85 | + | |
| 86 | + | |
| 87 | + | |
| 88 | + | |
| 89 | + | |
| 90 | + | |
| 91 | + | |
| 92 | + | |
| 93 | + | |
| 94 | + | |
| 95 | + | |
| 96 | + | |
| 97 | + | |
| 98 | + | |
| 99 | + | |
| 100 | + | |
| 101 | + | |
| 102 | + | |
| 103 | + | |
| 104 | + | |
| 105 | + | |
| 106 | + | |
| 107 | + | |
| 108 | + | |
| 109 | + | |
| 110 | + | |
| 111 | + | |
| 112 | + | |
| 113 | + | |
| 114 | + | |
| 115 | + | |
| 116 | + | |
| 117 | + | |
| 118 | + | |
| 119 | + | |
| 120 | + | |
| 121 | + | |
| 122 | + | |
| 123 | + | |
| 124 | + | |
| 125 | + | |
| 126 | + | |
| 127 | + | |
| 128 | + | |
| 129 | + | |
| 130 | + | |
| 131 | + | |
| 132 | + | |
| 133 | + | |
| 134 | + | |
| 135 | + | |
| 136 | + | |
| 137 | + | |
| 138 | + | |
| 139 | + | |
| 140 | + | |
| 141 | + | |
| 142 | + | |
| 143 | + | |
| 144 | + | |
| 145 | + | |
| 146 | + | |
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
13 | 13 | | |
14 | 14 | | |
15 | 15 | | |
16 | | - | |
17 | | - | |
| 16 | + | |
| 17 | + | |
18 | 18 | | |
19 | 19 | | |
| 20 | + | |
| 21 | + | |
| 22 | + | |
| 23 | + | |
| 24 | + | |
| 25 | + | |
| 26 | + | |
| 27 | + | |
| 28 | + | |
| 29 | + | |
| 30 | + | |
| 31 | + | |
| 32 | + | |
| 33 | + | |
| 34 | + | |
| 35 | + | |
| 36 | + | |
20 | 37 | | |
21 | 38 | | |
22 | 39 | | |
23 | 40 | | |
24 | | - | |
| 41 | + | |
| 42 | + | |
| 43 | + | |
| 44 | + | |
| 45 | + | |
| 46 | + | |
| 47 | + | |
| 48 | + | |
| 49 | + | |
| 50 | + | |
| 51 | + | |
| 52 | + | |
| 53 | + | |
| 54 | + | |
25 | 55 | | |
26 | 56 | | |
27 | 57 | | |
28 | 58 | | |
29 | | - | |
30 | | - | |
31 | | - | |
32 | | - | |
33 | | - | |
34 | | - | |
35 | | - | |
36 | | - | |
| 59 | + | |
| 60 | + | |
| 61 | + | |
| 62 | + | |
| 63 | + | |
| 64 | + | |
| 65 | + | |
| 66 | + | |
| 67 | + | |
| 68 | + | |
| 69 | + | |
| 70 | + | |
| 71 | + | |
| 72 | + | |
| 73 | + | |
37 | 74 | | |
| 75 | + | |
38 | 76 | | |
39 | 77 | | |
40 | 78 | | |
| |||
0 commit comments