Skip to content

Commit 57ab06b

Browse files
fix(components)!: the filter builder's list value holds the spec list's entries, so a boolean switched to In is a rule the spec accepts (#26380)
<!-- claude-projects-attribution: {"github_login":"os-steve"} --> _Requested by **Steve** · [project thread](https://claude.ai/code/project/chan_01FdL1XeUyvVNxLreh3F4Qi9?thread=cmsg_01FdL1XeUyvVNxLreh3F4Qi9Bvvb9XG5ZxhuycX3NS7AUQ)_ Fixes #26348 Clause-②: yes (narrowing) Metadata-protocol: none ## 维护者速读 - 改了什么:筛选构建器里「等于 true」的行切到「属于」时,不再产出 spec 拒收的 `[true]`,而是 `["true"]`;构建器行值的列表成员收窄为 spec 的字符串/数字列表。 - 为什么改:构建器自己产出的筛选规则被 `ViewFilterRuleSchema` 拒收,保存视图时才报错;这是 #26306 留下的半截。 - 风险与回滚:仅编译期收窄,运行时只改布尔值进列表时的拼写;回滚即 revert 本 PR。 - 先看:`filter-builder.tsx` 的 `toFilterListEntry` / `normalizeToArray`;`UserFilters.tsx` 的 `lookupPickerValue`;changeset。 - 两分钟验证:`pnpm --dir ui exec vitest run packages/components/src/__tests__/filter-builder-boolean-list-26348.test.tsx`,应 4 条全绿。 - 需要你做的:无,席位落地。 ## What changes - **`FilterBuilderRow.value`** (`ui/packages/components/src/custom/filter-builder.tsx`) is now exactly `ViewFilterRule['value']`. The builder's own wider list member `(string | number | boolean)[]` is gone. A module-private `FilterListEntry` reads the list element from the spec's value type instead of restating it. - **The source writer.** Every list or range the builder builds from a scalar goes through one conversion, `toFilterListEntry`: a boolean becomes its string spelling (`true` becomes `"true"`). It is called by `normalizeToArray` (so `reshapeFilterValue`'s list and pair arms), by `retypeFilterValue`'s list and pair arms, by `MultiValueInput`'s commit and by the range input's `setBound`. That is the conversion the builder already made for a boolean that reaches a text column, and every bucket offering a list operator is a text-family column (`select` / `lookup`). The list controls compare entries with `String()`, so the option whose value is `"true"` stays ticked. - **`UserFilters`** (`ui/packages/plugin-list/src/UserFilters.tsx`) hands a lookup chip's selection to `LookupValuePicker` through `lookupPickerValue`, the same conversion. The selection map stays typed for every control kind, because a boolean chip's selection really is `[true]`. The two casts in the picker's `onChange` are gone, since the narrowed value no longer needs them. - `docs/FilterBuilder.md` shows the new member. No sdui record. The per-package records were retired by #26367 while this PR was open. The main merge took the modify/delete with `git rm`, and CI's Build & E2E UI regenerates the manifest. Changeset: `.changeset/26348-filter-builder-boolean-list-entry.md` (`patch` for both packages, BREAKING banner, prose migration for writers). ### Writers measured (the card's step 1) | writer | produced a boolean list entry? | now | |---|---|---| | operator switch, `changeOperator` → `reshapeFilterValue` → `normalizeToArray` | yes: `equals true` → `in` gave `[true]`; → `between` gave `[true, ""]` | `["true"]`, `["true", ""]` | | field switch, `retypeFilterValue` list / pair arms | type-level only: a text target already stringified, the boolean family has no list operator | typed through `toFilterListEntry` | | `MultiValueInput` commit, range `setBound` | type-level only: their inputs hand back strings and numbers | typed through `toFilterListEntry` | | checkbox list, `LookupValuePicker.toggleId` | no: option values and ids are strings and numbers | unchanged | | `UserFilters` lookup chip → `LookupValuePicker` | yes: an authored `defaultValues: [true]` on a lookup filter field reached the query as `[true, id]` | `["true", id]` | The compiler confirmed the list: narrowing the member first left exactly one more site red (the range writer, TS2322), fixed here. ### ADR-0087 stop-check (the card's step 3) Run before the narrowing edit, against a throwaway probe commit (the narrowing line plus a probe changeset with a prose migration), with `node scripts/check-adr-0087-registration.mjs --base origin/main --head PROBE_SHA`: exit 0, `not-required (runtime-interface-only) -- verified: ui/packages/components/src/custom/filter-builder.tsx#FilterBuilderRow (interface)`. No literal FROM → TO is needed for writers, so no ADR wording and no decision were needed. The real changeset carries the same disposition and passes the gate on this head. ## Tests - New `ui/packages/components/src/__tests__/filter-builder-boolean-list-26348.test.tsx`: type pins (`FilterBuilderRow['value']` equals the spec value; a boolean list is a `@ts-expect-error`; a boolean scalar stays admitted), the schema refusing `[true]` and accepting `["true"]` (non-vacuity), the real operator dropdown driving `equals true` → In and `equals false` → Not in with the emitted rule parsed by `ViewFilterRuleSchema`, and the helpers. - New `ui/packages/plugin-list/src/__tests__/UserFilters.lookupBooleanEntry-26348.test.tsx`: a lookup chip with an authored `true` default; a picked id joins it as `["true", "acc_1"]`; string and number ids pass through untouched. - **Forced test edits**, both type pins that stated the old width on purpose so a narrowing would have to change them: `filter-builder-spec-row-26306.test.tsx` (the `Equal` pin and its docblock row) and `filter-builder-name-authority-9306.test.tsx` (the `Equal` pin and its comment). ## Acceptance notes - A boolean on a date column switched to Between now carries `["true", ""]`: a spec-valid shape, but not a date the range input can show. Reaching it needs a host to hand a date column a boolean in the first place, and a field switch to a date column already clears it. Noted, not filed (no defect class: the builder never authors that row). - `FilterBuilderSchema.onChange` signature mismatch (out of scope by the card). Measured on this head: `FilterBuilderSchema` in `ui/packages/types/src/complex.ts` declares `onChange?: (filter: FilterGroup) => void`, while the `filter-builder` renderer calls `onChange` with a `{ target: { name, value } }` event shape. Reported to the seat in the report's `out_of_scope_findings`; not touched here. ## Verification All readings on head `80dc2dca` (after merging `origin/main` at `6390fe52`), under `scripts/pm/os-verify-lock.sh`. <details><summary>Type-check and tests</summary> - `pnpm --filter @object-ui/components type-check` and `pnpm --filter @object-ui/plugin-list type-check` (both `tsc --noEmit && tsc -p tsconfig.test.json`): green before and after the merge. The new `@ts-expect-error` pin compiles only because the boolean list is refused. - Before the merge, both whole packages: `pnpm --dir ui exec vitest run --maxWorkers=2 packages/components/ packages/plugin-list/`: Test Files 600 passed, 1 skipped; Tests 5913 passed, 24 skipped; VERDICT command-exit 0. - After the merge, scoped (main moved `components` renderers): `packages/components/src/__tests__/` plus `packages/plugin-list/src/__tests__/UserFilters*`: Test Files 259 passed, 1 skipped; Tests 2528 passed; VERDICT command-exit 0. - Console: `pnpm exec turbo run build --filter=@object-ui/console --concurrency=2`: 71 of 71 tasks, VERDICT command-exit 0. </details> <details><summary>Ablation (one-shot, not kept)</summary> Through `node scripts/ablation-replace.mjs`, each restored to the HEAD blob with `git diff HEAD` empty: - `normalizeToArray` back to keeping the boolean: `filter-builder-boolean-list-26348.test.tsx` goes 3 failed, 1 passed (the schema non-vacuity case stays green). - `lookupPickerValue` back to the identity: `UserFilters.lookupBooleanEntry-26348.test.tsx` goes 1 failed, 1 passed (the pass-through case stays green). </details> <details><summary>Gates</summary> - `node scripts/pm/dispatch-gates.mjs --commands --repo objectstack-ai/objectstack` derived 45 families on this head; all 45 ran with exit 0, and `--ran` reconciles 45 run, 0 NOT-MEASURED, 0 UNRUN. Among them `node scripts/check-adr-0087-registration.mjs --base origin/main` verifies the `runtime-interface-only` disposition, and `node scripts/check-sdui-manifest.mjs` is green on the re-recorded sources. - Every family CI runs is read from this PR's check-runs, not re-run here. </details> <details><summary>Dogfood battery (owed by the narrowing arm)</summary> `pnpm --filter @objectstack/dogfood test` after building its closure: Test Files 2 failed, 297 passed, 3 skipped; Tests 10 failed, 2521 passed, 18 skipped. Every failure is in `test/authz-conformance.test.ts` (the #17111 ledger-figure pins: "packages/rest/src/rest-route-ledger.ts: docblock says 80 rows, the table holds 81") and `test/authz-probe-blind-spot.test.ts` (the #13260 census over `rest-route-ledger.ts` and `rest-server.ts`). This diff touches no file under `packages/rest`; both files last changed on `main` in #26361 (the record-anchored approval lock read), so this is a main-side red, reported to the seat, not fixed here. </details> --- _Generated by [Claude Code](https://claude.ai/code/session_01ADHkmPyFNmACDisa4w6cr1)_ 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01ADHkmPyFNmACDisa4w6cr1 --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent e677770 commit 57ab06b

8 files changed

Lines changed: 319 additions & 37 deletions
Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,19 @@
1+
---
2+
'@object-ui/components': patch
3+
'@object-ui/plugin-list': patch
4+
---
5+
6+
fix(components)!: the filter builder's list value holds the spec list's entries, so an `equals true` row switched to In is a rule the spec accepts
7+
8+
Clause-②: yes (narrowing)
9+
Metadata-protocol: none
10+
11+
<!-- adr-0087: not-required (runtime-interface-only ui/packages/components/src/custom/filter-builder.tsx#FilterBuilderRow) A published runtime TypeScript interface of a React component with no metadata surface: no Zod schema, no spec declaration and no stored representation, so `objectstack migrate meta` has nothing to rewrite; the affected party is a TypeScript consumer, reached by its own compiler. -->
12+
13+
**BREAKING** type-surface change, compile-time only. `FilterBuilderRow` is exported from `@object-ui/components`. Its `value` is now exactly `ViewFilterRule['value']` from `@objectstack/spec/ui`: the builder's own wider list member, which also admitted boolean entries, is gone. A writer that put a boolean into a list value no longer compiles.
14+
15+
The defect it fixes: in the filter builder, a row `equals true` on a select column switched to In emitted `value: [true]`, and `ViewFilterRuleSchema` refuses a boolean list entry, so the builder produced a rule the spec rejects. A boolean carried into a list or a range is now written as its string spelling, so the same switch emits `["true"]`. That is the conversion the builder already made for a boolean that reaches a text column, and every column that offers a list operator is one. The option whose value is `"true"` stays ticked, because the list controls compare entries as strings.
16+
17+
`UserFilters` (`@object-ui/plugin-list`) hands a lookup chip's selection to `LookupValuePicker` the same way. An authored `defaultValues` entry `true` on a lookup filter field used to reach the query beside a picked id as a boolean; it now arrives as `"true"`, still drawn as a chip the user can remove.
18+
19+
**Migration.** Code that writes a list into a `FilterBuilderRow` value writes strings and numbers only. Write a boolean entry as its string spelling, the way the builder does, or keep the boolean as a scalar under `equals`. Code that reads `row.value` needs no change: a list it reads already holds only strings and numbers.

‎ui/packages/components/docs/FilterBuilder.md‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -136,9 +136,10 @@ interface FilterBuilderRow {
136136
id: string; // Row identifier (required)
137137
field: string; // Field value
138138
operator: FilterBuilderOperator; // Operator id (see below)
139-
value?: ViewFilterRule['value'] | (string | number | boolean)[];
139+
value?: ViewFilterRule['value'];
140140
// `ViewFilterRule` from `@objectstack/spec/ui`: optional (value-less operators
141-
// carry none) and may be null; the builder's own list may also hold booleans
141+
// carry none) and may be null; a list holds strings and numbers, so a boolean
142+
// carried into `in` / `not_in` / `between` is written as `"true"` / `"false"`
142143
}
143144
```
144145

Lines changed: 143 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,143 @@
1+
/**
2+
* ObjectUI
3+
* Copyright (c) 2024-present ObjectStack Inc.
4+
*
5+
* This source code is licensed under the MIT license found in the
6+
* LICENSE file in the root directory of this source tree.
7+
*/
8+
9+
/**
10+
* The builder's list values hold what the spec rule's list holds (#26348).
11+
*
12+
* `ViewFilterRuleSchema` (`@objectstack/spec/ui`) takes `in` / `not_in` /
13+
* `between` values as arrays of strings and numbers. The builder's row used to
14+
* declare a wider list member of its own, `(string | number | boolean)[]`, and
15+
* its one list writer for a scalar, `normalizeToArray`, kept a boolean a
16+
* boolean. So an `equals true` row (a boolean scalar, which the spec admits) on
17+
* a `select` column, switched to `In`, emitted `[true]`, and the schema refused
18+
* the rule the builder had just produced.
19+
*
20+
* The emitted list now matches the spec's list element at the source: a boolean
21+
* carried into a list or a range is written as its string spelling, the
22+
* conversion the builder already makes for a boolean that reaches a text
23+
* column. The row's value is the spec rule's value and nothing wider.
24+
*
25+
* Type-level pins are judged by `tsc -p tsconfig.test.json`; a green vitest run
26+
* says nothing about them.
27+
*/
28+
import { describe, it, expect, vi } from 'vitest';
29+
import React from 'react';
30+
import { render, screen, fireEvent, waitFor } from '@testing-library/react';
31+
import '@testing-library/jest-dom';
32+
import { ViewFilterRuleSchema, type ViewFilterRule } from '@objectstack/spec/ui';
33+
import type { FilterGroup } from '@object-ui/types';
34+
import {
35+
FilterBuilder,
36+
reshapeFilterValue,
37+
retypeFilterValue,
38+
type FilterBuilderProps,
39+
type FilterBuilderRow,
40+
} from '../custom/filter-builder';
41+
42+
type Equal<A, B> =
43+
(<T>() => T extends A ? 1 : 2) extends (<T>() => T extends B ? 1 : 2) ? true : false;
44+
function expectType<T extends true>(_: T = true as T): void { /* compile-time only */ }
45+
46+
/* ── The shape ────────────────────────────────────────────────────────────── */
47+
48+
// The row's value IS the spec rule's value, with no wider list member beside it.
49+
expectType<Equal<FilterBuilderRow['value'], ViewFilterRule['value']>>();
50+
// So a boolean list is refused where it is written, as the schema refuses it.
51+
const booleanList: FilterBuilderRow = {
52+
id: 'c1',
53+
field: 'stage',
54+
operator: 'in',
55+
// @ts-expect-error a list entry is a string or a number, as the spec's is
56+
value: [true],
57+
};
58+
// A boolean SCALAR stays admitted: the spec admits it, and `equals true` is a row.
59+
const booleanScalar: FilterBuilderRow = { id: 'c1', field: 'active', operator: 'equals', value: true };
60+
void booleanList;
61+
void booleanScalar;
62+
63+
const FIELDS: NonNullable<FilterBuilderProps['fields']> = [
64+
{
65+
value: 'stage',
66+
label: 'Stage',
67+
type: 'select',
68+
options: [
69+
{ value: 'true', label: 'True' },
70+
{ value: 'won', label: 'Won' },
71+
],
72+
},
73+
{ value: 'closed_at', label: 'Closed at', type: 'date' },
74+
];
75+
76+
function renderAuthored(row: FilterGroup['conditions'][number]) {
77+
const onChange = vi.fn();
78+
// Hoisted out of the JSX: the sync effect re-seeds state when the `value`
79+
// prop's identity changes, which would undo the interaction under test.
80+
const value: FilterGroup = { logic: 'and', conditions: [row] };
81+
render(<FilterBuilder fields={FIELDS} value={value} onChange={onChange} />);
82+
return onChange;
83+
}
84+
85+
function lastRow(onChange: ReturnType<typeof vi.fn>): FilterBuilderRow {
86+
expect(onChange.mock.calls.length, 'the builder never called onChange').toBeGreaterThan(0);
87+
return onChange.mock.calls.at(-1)?.[0].conditions[0];
88+
}
89+
90+
/** Drive the row's REAL operator dropdown (index 1: field, operator, value). */
91+
async function pickOperator(label: string) {
92+
fireEvent.keyDown(screen.getAllByRole('combobox')[1], { key: 'ArrowDown' });
93+
const option = await waitFor(() => {
94+
const found = screen.getAllByRole('option').find((o) => o.textContent === label);
95+
expect(found, `no "${label}" option in the operator dropdown`).toBeTruthy();
96+
return found!;
97+
});
98+
fireEvent.click(option);
99+
}
100+
101+
function specAccepts(row: FilterBuilderRow): boolean {
102+
return ViewFilterRuleSchema.safeParse({ field: row.field, operator: row.operator, value: row.value })
103+
.success;
104+
}
105+
106+
describe('#26348 — a boolean carried into a list is a list the spec accepts', () => {
107+
it('the schema refuses the list the builder used to emit (non-vacuity)', () => {
108+
expect(ViewFilterRuleSchema.safeParse({ field: 'stage', operator: 'in', value: [true] }).success).toBe(false);
109+
expect(ViewFilterRuleSchema.safeParse({ field: 'stage', operator: 'in', value: ['true'] }).success).toBe(true);
110+
});
111+
112+
it('an `equals true` row on a select column, switched to In, emits ["true"]', async () => {
113+
const onChange = renderAuthored({ id: 'c1', field: 'stage', operator: 'equals', value: true });
114+
await pickOperator('In');
115+
const row = lastRow(onChange);
116+
expect(row.operator).toBe('in');
117+
expect(row.value).toEqual(['true']);
118+
expect(specAccepts(row)).toBe(true);
119+
// Still drawn: the list control compares entries with `String()`, so the
120+
// option whose value is "true" is the one ticked.
121+
const ticked = screen.getAllByRole('checkbox').filter((c) => c.getAttribute('data-state') === 'checked');
122+
expect(ticked).toHaveLength(1);
123+
expect(ticked[0].closest('label')).toHaveTextContent('True');
124+
});
125+
126+
it('`equals false` switched to Not in emits ["false"]', async () => {
127+
const onChange = renderAuthored({ id: 'c1', field: 'stage', operator: 'equals', value: false });
128+
await pickOperator('Not in');
129+
const row = lastRow(onChange);
130+
expect(row.value).toEqual(['false']);
131+
expect(specAccepts(row)).toBe(true);
132+
});
133+
134+
it('the helpers write a boolean list or range entry as its string spelling', () => {
135+
expect(reshapeFilterValue(true, 'in')).toEqual(['true']);
136+
expect(reshapeFilterValue(false, 'not_in')).toEqual(['false']);
137+
expect(reshapeFilterValue(true, 'between')).toEqual(['true', '']);
138+
// A scalar target is untouched: `equals true` keeps its boolean.
139+
expect(reshapeFilterValue(true, 'equals')).toBe(true);
140+
// A field switch into a list column converts the same way.
141+
expect(retypeFilterValue(true, 'select', 'in')).toEqual(['true']);
142+
});
143+
});

‎ui/packages/components/src/__tests__/filter-builder-name-authority-9306.test.tsx‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -97,14 +97,14 @@ const offVocabulary: FilterBuilderRow = {
9797
value: 1,
9898
};
9999

100-
// The value extension: the SPEC rule's value, by reference (#26306), plus the
101-
// builder's own boolean-bearing list — never the
100+
// The value extension: the SPEC rule's value, by reference (#26306), and
101+
// nothing wider since #26348 — never the
102102
// authority's `value?: any`, which would switch the typing of every exported
103103
// value helper off in silence. Optional, as the spec's is: a value-less
104104
// operator is authored with no value, and a required member refused the
105105
// authored group the renderer hands over. The per-case pins live in
106106
// `filter-builder-spec-row-26306.test.tsx`.
107-
expectType<Equal<FilterBuilderRow['value'], ViewFilterRule['value'] | (string | number | boolean)[]>>();
107+
expectType<Equal<FilterBuilderRow['value'], ViewFilterRule['value']>>();
108108
const rowWithoutValue: FilterBuilderRow = { id: 'c1', field: 'a', operator: 'is_null' };
109109
// …and still not `any`: a value the spec refuses is refused here.
110110
const objectValue: FilterBuilderRow = {

‎ui/packages/components/src/__tests__/filter-builder-spec-row-26306.test.tsx‎

Lines changed: 8 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,7 @@
2222
* |--------------------------------|------------------------|---------------------------------|
2323
* | value omitted (unary operator) | `value?` | `value` required |
2424
* | value `null` | admitted | refused |
25-
* | boolean entry in a list / pair | refused | admitted (kept, see below) |
25+
* | boolean entry in a list / pair | refused | admitted (narrowed by #26348) |
2626
* | operator | `VIEW_FILTER_OPERATORS` | those + opt-in `exists` / `notExists` |
2727
*
2828
* The operator row is not a divergence on the way IN: every spec id is a
@@ -36,12 +36,11 @@
3636
* threw on `.trim()` and on a text column came back as the string
3737
* `"undefined"`.
3838
*
39-
* The third case runs the other way — the builder is WIDER than the spec, so it
40-
* does not keep an authored group out — and is left as it is here: the row
41-
* keeps its boolean list member beside the spec's value, and narrowing it is a
42-
* breaking change to a published type with a consumer in `@object-ui/plugin-list`
43-
* (`UserFilters` hands `LookupValuePicker` a boolean-bearing list). The pin
44-
* below states the width as it is, so the narrowing has to change it.
39+
* The third case runs the other way — the builder was WIDER than the spec, so it
40+
* did not keep an authored group out — and was left as it was here. #26348
41+
* narrowed it: the row's value is now the spec's value and nothing wider, and
42+
* the writers that produced a boolean list entry write its string spelling.
43+
* Its pins live in `filter-builder-boolean-list-26348.test.tsx`.
4544
*
4645
* Type-level pins are judged by `tsc -p tsconfig.test.json`; a green vitest run
4746
* says nothing about them.
@@ -66,9 +65,8 @@ type Assignable<From, To> = [From] extends [To] ? true : false;
6665

6766
/* ── The shapes ───────────────────────────────────────────────────────────── */
6867

69-
// The row's value IS the spec rule's value — by reference, not a copy of it —
70-
// plus the builder's own wider list member.
71-
expectType<Equal<FilterBuilderRow['value'], ViewFilterRule['value'] | (string | number | boolean)[]>>();
68+
// The row's value IS the spec rule's value — by reference, not a copy of it.
69+
expectType<Equal<FilterBuilderRow['value'], ViewFilterRule['value']>>();
7270
expectType<Assignable<ViewFilterRule['value'], FilterBuilderRow['value']>>();
7371
// The renderer's two reads cross with no cast: the authored group and the
7472
// authored field list are what the builder's props take.

0 commit comments

Comments
 (0)