diff --git a/.changeset/26348-filter-builder-boolean-list-entry.md b/.changeset/26348-filter-builder-boolean-list-entry.md new file mode 100644 index 00000000000..a31c61a9ab4 --- /dev/null +++ b/.changeset/26348-filter-builder-boolean-list-entry.md @@ -0,0 +1,19 @@ +--- +'@object-ui/components': patch +'@object-ui/plugin-list': patch +--- + +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 + +Clause-②: yes (narrowing) +Metadata-protocol: none + + + +**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. + +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. + +`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. + +**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. diff --git a/ui/packages/components/docs/FilterBuilder.md b/ui/packages/components/docs/FilterBuilder.md index 0e2824d9990..4f923970761 100644 --- a/ui/packages/components/docs/FilterBuilder.md +++ b/ui/packages/components/docs/FilterBuilder.md @@ -136,9 +136,10 @@ interface FilterBuilderRow { id: string; // Row identifier (required) field: string; // Field value operator: FilterBuilderOperator; // Operator id (see below) - value?: ViewFilterRule['value'] | (string | number | boolean)[]; + value?: ViewFilterRule['value']; // `ViewFilterRule` from `@objectstack/spec/ui`: optional (value-less operators - // carry none) and may be null; the builder's own list may also hold booleans + // carry none) and may be null; a list holds strings and numbers, so a boolean + // carried into `in` / `not_in` / `between` is written as `"true"` / `"false"` } ``` diff --git a/ui/packages/components/src/__tests__/filter-builder-boolean-list-26348.test.tsx b/ui/packages/components/src/__tests__/filter-builder-boolean-list-26348.test.tsx new file mode 100644 index 00000000000..a5b47287622 --- /dev/null +++ b/ui/packages/components/src/__tests__/filter-builder-boolean-list-26348.test.tsx @@ -0,0 +1,143 @@ +/** + * 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. + */ + +/** + * The builder's list values hold what the spec rule's list holds (#26348). + * + * `ViewFilterRuleSchema` (`@objectstack/spec/ui`) takes `in` / `not_in` / + * `between` values as arrays of strings and numbers. The builder's row used to + * declare a wider list member of its own, `(string | number | boolean)[]`, and + * its one list writer for a scalar, `normalizeToArray`, kept a boolean a + * boolean. So an `equals true` row (a boolean scalar, which the spec admits) on + * a `select` column, switched to `In`, emitted `[true]`, and the schema refused + * the rule the builder had just produced. + * + * The emitted list now matches the spec's list element at the source: a boolean + * carried into a list or a range is written as its string spelling, the + * conversion the builder already makes for a boolean that reaches a text + * column. The row's value is the spec rule's value and nothing wider. + * + * Type-level pins are judged by `tsc -p tsconfig.test.json`; a green vitest run + * says nothing about them. + */ +import { describe, it, expect, vi } from 'vitest'; +import React from 'react'; +import { render, screen, fireEvent, waitFor } from '@testing-library/react'; +import '@testing-library/jest-dom'; +import { ViewFilterRuleSchema, type ViewFilterRule } from '@objectstack/spec/ui'; +import type { FilterGroup } from '@object-ui/types'; +import { + FilterBuilder, + reshapeFilterValue, + retypeFilterValue, + type FilterBuilderProps, + type FilterBuilderRow, +} from '../custom/filter-builder'; + +type Equal = + (() => T extends A ? 1 : 2) extends (() => T extends B ? 1 : 2) ? true : false; +function expectType(_: T = true as T): void { /* compile-time only */ } + +/* ── The shape ────────────────────────────────────────────────────────────── */ + +// The row's value IS the spec rule's value, with no wider list member beside it. +expectType>(); +// So a boolean list is refused where it is written, as the schema refuses it. +const booleanList: FilterBuilderRow = { + id: 'c1', + field: 'stage', + operator: 'in', + // @ts-expect-error a list entry is a string or a number, as the spec's is + value: [true], +}; +// A boolean SCALAR stays admitted: the spec admits it, and `equals true` is a row. +const booleanScalar: FilterBuilderRow = { id: 'c1', field: 'active', operator: 'equals', value: true }; +void booleanList; +void booleanScalar; + +const FIELDS: NonNullable = [ + { + value: 'stage', + label: 'Stage', + type: 'select', + options: [ + { value: 'true', label: 'True' }, + { value: 'won', label: 'Won' }, + ], + }, + { value: 'closed_at', label: 'Closed at', type: 'date' }, +]; + +function renderAuthored(row: FilterGroup['conditions'][number]) { + const onChange = vi.fn(); + // Hoisted out of the JSX: the sync effect re-seeds state when the `value` + // prop's identity changes, which would undo the interaction under test. + const value: FilterGroup = { logic: 'and', conditions: [row] }; + render(); + return onChange; +} + +function lastRow(onChange: ReturnType): FilterBuilderRow { + expect(onChange.mock.calls.length, 'the builder never called onChange').toBeGreaterThan(0); + return onChange.mock.calls.at(-1)?.[0].conditions[0]; +} + +/** Drive the row's REAL operator dropdown (index 1: field, operator, value). */ +async function pickOperator(label: string) { + fireEvent.keyDown(screen.getAllByRole('combobox')[1], { key: 'ArrowDown' }); + const option = await waitFor(() => { + const found = screen.getAllByRole('option').find((o) => o.textContent === label); + expect(found, `no "${label}" option in the operator dropdown`).toBeTruthy(); + return found!; + }); + fireEvent.click(option); +} + +function specAccepts(row: FilterBuilderRow): boolean { + return ViewFilterRuleSchema.safeParse({ field: row.field, operator: row.operator, value: row.value }) + .success; +} + +describe('#26348 — a boolean carried into a list is a list the spec accepts', () => { + it('the schema refuses the list the builder used to emit (non-vacuity)', () => { + expect(ViewFilterRuleSchema.safeParse({ field: 'stage', operator: 'in', value: [true] }).success).toBe(false); + expect(ViewFilterRuleSchema.safeParse({ field: 'stage', operator: 'in', value: ['true'] }).success).toBe(true); + }); + + it('an `equals true` row on a select column, switched to In, emits ["true"]', async () => { + const onChange = renderAuthored({ id: 'c1', field: 'stage', operator: 'equals', value: true }); + await pickOperator('In'); + const row = lastRow(onChange); + expect(row.operator).toBe('in'); + expect(row.value).toEqual(['true']); + expect(specAccepts(row)).toBe(true); + // Still drawn: the list control compares entries with `String()`, so the + // option whose value is "true" is the one ticked. + const ticked = screen.getAllByRole('checkbox').filter((c) => c.getAttribute('data-state') === 'checked'); + expect(ticked).toHaveLength(1); + expect(ticked[0].closest('label')).toHaveTextContent('True'); + }); + + it('`equals false` switched to Not in emits ["false"]', async () => { + const onChange = renderAuthored({ id: 'c1', field: 'stage', operator: 'equals', value: false }); + await pickOperator('Not in'); + const row = lastRow(onChange); + expect(row.value).toEqual(['false']); + expect(specAccepts(row)).toBe(true); + }); + + it('the helpers write a boolean list or range entry as its string spelling', () => { + expect(reshapeFilterValue(true, 'in')).toEqual(['true']); + expect(reshapeFilterValue(false, 'not_in')).toEqual(['false']); + expect(reshapeFilterValue(true, 'between')).toEqual(['true', '']); + // A scalar target is untouched: `equals true` keeps its boolean. + expect(reshapeFilterValue(true, 'equals')).toBe(true); + // A field switch into a list column converts the same way. + expect(retypeFilterValue(true, 'select', 'in')).toEqual(['true']); + }); +}); diff --git a/ui/packages/components/src/__tests__/filter-builder-name-authority-9306.test.tsx b/ui/packages/components/src/__tests__/filter-builder-name-authority-9306.test.tsx index 35eb5e0df7b..76659cc2515 100644 --- a/ui/packages/components/src/__tests__/filter-builder-name-authority-9306.test.tsx +++ b/ui/packages/components/src/__tests__/filter-builder-name-authority-9306.test.tsx @@ -97,14 +97,14 @@ const offVocabulary: FilterBuilderRow = { value: 1, }; -// The value extension: the SPEC rule's value, by reference (#26306), plus the -// builder's own boolean-bearing list — never the +// The value extension: the SPEC rule's value, by reference (#26306), and +// nothing wider since #26348 — never the // authority's `value?: any`, which would switch the typing of every exported // value helper off in silence. Optional, as the spec's is: a value-less // operator is authored with no value, and a required member refused the // authored group the renderer hands over. The per-case pins live in // `filter-builder-spec-row-26306.test.tsx`. -expectType>(); +expectType>(); const rowWithoutValue: FilterBuilderRow = { id: 'c1', field: 'a', operator: 'is_null' }; // …and still not `any`: a value the spec refuses is refused here. const objectValue: FilterBuilderRow = { diff --git a/ui/packages/components/src/__tests__/filter-builder-spec-row-26306.test.tsx b/ui/packages/components/src/__tests__/filter-builder-spec-row-26306.test.tsx index e6267077c4c..b71dcd5d72e 100644 --- a/ui/packages/components/src/__tests__/filter-builder-spec-row-26306.test.tsx +++ b/ui/packages/components/src/__tests__/filter-builder-spec-row-26306.test.tsx @@ -22,7 +22,7 @@ * |--------------------------------|------------------------|---------------------------------| * | value omitted (unary operator) | `value?` | `value` required | * | value `null` | admitted | refused | - * | boolean entry in a list / pair | refused | admitted (kept, see below) | + * | boolean entry in a list / pair | refused | admitted (narrowed by #26348) | * | operator | `VIEW_FILTER_OPERATORS` | those + opt-in `exists` / `notExists` | * * The operator row is not a divergence on the way IN: every spec id is a @@ -36,12 +36,11 @@ * threw on `.trim()` and on a text column came back as the string * `"undefined"`. * - * The third case runs the other way — the builder is WIDER than the spec, so it - * does not keep an authored group out — and is left as it is here: the row - * keeps its boolean list member beside the spec's value, and narrowing it is a - * breaking change to a published type with a consumer in `@object-ui/plugin-list` - * (`UserFilters` hands `LookupValuePicker` a boolean-bearing list). The pin - * below states the width as it is, so the narrowing has to change it. + * The third case runs the other way — the builder was WIDER than the spec, so it + * did not keep an authored group out — and was left as it was here. #26348 + * narrowed it: the row's value is now the spec's value and nothing wider, and + * the writers that produced a boolean list entry write its string spelling. + * Its pins live in `filter-builder-boolean-list-26348.test.tsx`. * * Type-level pins are judged by `tsc -p tsconfig.test.json`; a green vitest run * says nothing about them. @@ -66,9 +65,8 @@ type Assignable = [From] extends [To] ? true : false; /* ── The shapes ───────────────────────────────────────────────────────────── */ -// The row's value IS the spec rule's value — by reference, not a copy of it — -// plus the builder's own wider list member. -expectType>(); +// The row's value IS the spec rule's value — by reference, not a copy of it. +expectType>(); expectType>(); // The renderer's two reads cross with no cast: the authored group and the // authored field list are what the builder's props take. diff --git a/ui/packages/components/src/custom/filter-builder.tsx b/ui/packages/components/src/custom/filter-builder.tsx index 208b54d36dd..7be93bedf86 100644 --- a/ui/packages/components/src/custom/filter-builder.tsx +++ b/ui/packages/components/src/custom/filter-builder.tsx @@ -73,11 +73,13 @@ import { Popover, PopoverContent, PopoverTrigger } from "../ui/popover" * would switch their checking off in silence. OPTIONAL and admitting * `null`, as the spec's is: a value-less operator (`is_null`, `is_empty`, * …) is authored with no `value`, and a required member refused the - * authored `FilterGroup` the `filter-builder` renderer hands over. One - * member is the builder's own and WIDER than the spec's: a list entry may - * be a boolean, because a boolean scalar carried into a list operator - * (`reshapeFilterValue`, `normalizeToArray`) stays a boolean. The spec's - * list holds strings and numbers only. + * authored `FilterGroup` the `filter-builder` renderer hands over. Its + * list holds what the spec's list holds, strings and numbers + * ({@link FilterListEntry}), and nothing wider (#26348): a boolean scalar + * carried into a list or a range is written as its string spelling by + * `normalizeToArray`, so an `equals true` row switched to `in` emits + * `["true"]`, a list `ViewFilterRuleSchema` accepts, where it used to emit + * `[true]`, which the schema refuses. * * Its own NAME, not the authority's (objectui#6349, batch 12). Under the * authority's name this was a second declaration of `FilterBuilderCondition` @@ -90,9 +92,16 @@ import { Popover, PopoverContent, PopoverTrigger } from "../ui/popover" export interface FilterBuilderRow extends Omit { operator: FilterBuilderOperator - value?: ViewFilterRule["value"] | (string | number | boolean)[] + value?: ViewFilterRule["value"] } +/** + * One entry of a list or range value: the element of the spec rule's own list + * member, read from `ViewFilterRule["value"]` rather than restated, so the + * builder's lists cannot hold a type the spec's list refuses (#26348). + */ +type FilterListEntry = Extract, readonly unknown[]>[number] + /** * The group the FilterBuilder edits: one `logic` over FLAT rows. * @@ -868,17 +877,17 @@ export function retypeFilterValue( switch (filterValueArity(operator)) { case "list": { - const carried: (string | number | boolean)[] = [] + const carried: FilterListEntry[] = [] for (const entry of normalizeToArray(value)) { const converted = convertScalarToFamily(entry, family) - if (converted !== undefined && converted !== "") carried.push(converted) + if (converted !== undefined && converted !== "") carried.push(toFilterListEntry(converted)) } return carried } case "pair": { const [min = "", max = ""] = normalizeToArray(value) - const lower = convertScalarToFamily(min, family) ?? "" - const upper = convertScalarToFamily(max, family) ?? "" + const lower = toFilterListEntry(convertScalarToFamily(min, family) ?? "") + const upper = toFilterListEntry(convertScalarToFamily(max, family) ?? "") return lower === "" && upper === "" ? [] : [lower, upper] } default: { @@ -1114,7 +1123,7 @@ export function isFilterValueComplete( /** The two bounds a `pair` row edits, with the gaps filled in for rendering. */ function toPairBounds( value: FilterBuilderRow["value"], -): [string | number | boolean, string | number | boolean] { +): [FilterListEntry, FilterListEntry] { const [min = "", max = ""] = normalizeToArray(value) return [min, max] } @@ -1372,10 +1381,30 @@ function displayScalarValue(value: FilterBuilderRow["value"]): string { return isValueUnset(value) ? "" : String(value) } -/** Normalize a filter value into an array for multi-select scenarios */ -function normalizeToArray(value: FilterBuilderRow["value"]): (string | number | boolean)[] { +/** + * A scalar as a list or range ENTRY: the spec's list holds strings and numbers + * only, so a boolean is written as its string spelling (`true` → `"true"`) + * (#26348). + * + * The string spelling, not a dropped entry, because it is the conversion this + * builder already makes for a boolean that reaches a text column + * (`convertScalarToFamily`'s `text` arm), and every bucket that offers a list + * operator is a text-family column (`select` / `lookup`). It also keeps the + * entry visible: every list control compares entries with `String()`, so a + * `"true"` entry is drawn exactly where a `true` one was. + */ +function toFilterListEntry(value: string | number | boolean): FilterListEntry { + return typeof value === "boolean" ? String(value) : value +} + +/** + * Normalize a filter value into an array for multi-select scenarios. A scalar + * becomes a one-entry list through {@link toFilterListEntry}, so what this + * returns is always a list the spec's rule accepts. + */ +function normalizeToArray(value: FilterBuilderRow["value"]): FilterListEntry[] { if (Array.isArray(value)) return value - if (!isValueUnset(value)) return [value] + if (!isValueUnset(value)) return [toFilterListEntry(value)] return [] } @@ -1825,7 +1854,7 @@ function FilterBuilder({ // refusal. `|| undefined` so a complete pair carries NO attribute at all. const boundInvalid = (index: 0 | 1) => (missingBound === index ? true : undefined) const setBound = (index: 0 | 1, raw: string) => { - const next: [string | number | boolean, string | number | boolean] = [bounds[0], bounds[1]] + const next: [FilterListEntry, FilterListEntry] = [bounds[0], bounds[1]] // The same one reading as the field-switch path (objectui#4875), in // place of `parseFloat(raw) || 0`: a bound is a clean number or it is // not filled in (`""`). A half-read bound (`"42abc"` → 42) or an @@ -1833,7 +1862,7 @@ function FilterBuilder({ // silently narrows a RANGE, and `0` is a plausible-looking bound. // `convertScalarToFamily("", "number")` is `""`, so the old // `raw !== ""` guard is subsumed rather than dropped. - next[index] = numeric ? (convertScalarToFamily(raw, "number") ?? "") : raw + next[index] = numeric ? toFilterListEntry(convertScalarToFamily(raw, "number") ?? "") : raw // Both bounds cleared → back to the "nothing filled in yet" shape the // write path drops, rather than persisting an empty range. updateCondition(condition.id, { @@ -2295,13 +2324,13 @@ function TemporalValueEditor({ // ============================================================================ interface MultiValueInputProps { - values: (string | number | boolean)[] + values: FilterListEntry[] /** The `` the field's own type asks for (`text` / `number` / …). */ inputType: string /** Numeric field → committed tokens are numbers, so the wire carries numbers. */ numeric: boolean testId: string - onChange: (next: (string | number | boolean)[]) => void + onChange: (next: FilterListEntry[]) => void } /** @@ -2336,7 +2365,7 @@ function MultiValueInput({ values, inputType, numeric, testId, onChange }: Multi if (next === undefined || next === "") return setDraft("") if (values.some((v) => String(v) === String(next))) return - onChange([...values, next]) + onChange([...values, toFilterListEntry(next)]) } const removeAt = (index: number) => { diff --git a/ui/packages/plugin-list/src/UserFilters.tsx b/ui/packages/plugin-list/src/UserFilters.tsx index 0616d3864dc..e8a896622a1 100644 --- a/ui/packages/plugin-list/src/UserFilters.tsx +++ b/ui/packages/plugin-list/src/UserFilters.tsx @@ -512,6 +512,22 @@ interface DropdownFiltersProps { /** Selected values per field name, the dropdown bar's whole filter state. */ type DropdownSelection = Record; +/** + * A lookup-like chip's selection as the list `LookupValuePicker` edits: the + * filter builder's list value, whose entries are the spec `ViewFilterRule` + * list's, strings and numbers (#26348). + * + * The selection map is typed for every control kind at once, because a + * boolean chip's selection really is `[true]`; a lookup id never is a boolean. + * The one door a boolean has into a lookup chip is an authored `defaultValues` + * entry, whose spec type is shared by every control kind. It is handed over as + * its string spelling, the same entry the builder writes for a boolean carried + * into a list, so the chip still draws it and the user can remove it. + */ +function lookupPickerValue(selected: (string | number | boolean)[]): (string | number)[] { + return selected.map(v => (typeof v === 'boolean' ? String(v) : v)); +} + /** * The starting selection for `fields`: the author's `defaultValues`, * overridden by a restored selection (`initialSelections`, e.g. the host's @@ -830,14 +846,14 @@ function DropdownFilters({ fields, objectDef, objectDefPending, data, onFilterCh referenceTo: f.referenceTo, displayField: f.displayField, }} - value={selected} + value={lookupPickerValue(selected)} multiple={true} onChange={(value) => { const arr = Array.isArray(value) - ? (value as (string | number | boolean)[]) + ? value : (value === undefined || value === null || value === '') ? [] - : [value as string | number | boolean]; + : [value]; handleChange(f.field, arr); }} /> diff --git a/ui/packages/plugin-list/src/__tests__/UserFilters.lookupBooleanEntry-26348.test.tsx b/ui/packages/plugin-list/src/__tests__/UserFilters.lookupBooleanEntry-26348.test.tsx new file mode 100644 index 00000000000..3b4a1c74fc0 --- /dev/null +++ b/ui/packages/plugin-list/src/__tests__/UserFilters.lookupBooleanEntry-26348.test.tsx @@ -0,0 +1,76 @@ +/** + * 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. + */ + +/** + * #26348 — a lookup chip hands `LookupValuePicker` the list the filter builder + * edits: strings and numbers, the spec `ViewFilterRule` list's own entries. + * + * The bar's selection map is typed for every control kind at once, so its + * entries may be booleans (a boolean chip's selection is `[true]`). The one door + * a boolean has into a LOOKUP chip's selection is an authored `defaultValues` + * entry, whose spec type is shared by every control kind. The chip used to hand + * that list to the picker as it stood, so a picked id was appended beside a + * boolean and the query carried `[true, id]`. It is now handed over in its + * string spelling, the entry the builder writes for a boolean carried into a + * list, so the chip still draws it and the user can remove it. + * + * No `SchemaRendererProvider` here on purpose: without a DataSource the picker + * draws its by-hand id input, which is enough to read what it was handed and + * to drive what it hands back. + */ +import * as React from 'react'; +import { describe, it, expect, vi, afterEach } from 'vitest'; +import { cleanup, render, screen, fireEvent, within } from '@testing-library/react'; +import { UserFilters } from '../UserFilters'; + +afterEach(cleanup); + +function renderLookupChip(defaultValues: (string | number | boolean)[]) { + const onFilterChange = vi.fn(); + render( + , + ); + fireEvent.click(screen.getByTestId('filter-badge-account_id')); + // Non-vacuity: the lookup picker's by-hand id list is what rendered. + expect(screen.getByTestId('filter-lookup-account_id')).toBeTruthy(); + return { onFilterChange, ids: screen.getByTestId('lookup-ids-account_id') }; +} + +/** The `in` values the most recent `onFilterChange` carried for the chip. */ +function lastValues(onFilterChange: ReturnType): unknown[] { + const conditions = onFilterChange.mock.calls.at(-1)?.[0] as Array<[string, string, unknown[]]>; + const condition = conditions.find(([field]) => field === 'account_id'); + expect(condition, 'no condition for the lookup chip').toBeTruthy(); + expect(condition![1]).toBe('in'); + return condition![2]; +} + +describe('#26348 — the lookup chip hands the picker spec list entries', () => { + it('a picked id joins an authored boolean default in its string spelling', () => { + const { onFilterChange, ids } = renderLookupChip([true]); + // Drawn, so the user can see and remove it. + expect(within(ids).getByText('true')).toBeTruthy(); + const input = within(ids).getByRole('textbox'); + fireEvent.change(input, { target: { value: 'acc_1' } }); + fireEvent.keyDown(input, { key: 'Enter' }); + expect(lastValues(onFilterChange)).toEqual(['true', 'acc_1']); + }); + + it('ids that are already strings and numbers pass through untouched', () => { + const { onFilterChange, ids } = renderLookupChip(['acc_1', 7]); + const input = within(ids).getByRole('textbox'); + fireEvent.change(input, { target: { value: 'acc_2' } }); + fireEvent.keyDown(input, { key: 'Enter' }); + expect(lastValues(onFilterChange)).toEqual(['acc_1', 7, 'acc_2']); + }); +});