From 8431fec088e7f159b9264606bd454112a8a4a3d4 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 16 Aug 2026 05:52:18 +0000 Subject: [PATCH] =?UTF-8?q?fix(components):=20FilterBuilder=20=E5=88=87?= =?UTF-8?q?=E6=8D=A2=20field=20=E6=97=B6=E6=8A=8A=E7=AE=97=E5=AD=90?= =?UTF-8?q?=E5=BD=92=E5=88=B0=E6=96=B0=E6=A1=B6=E5=86=85=20(#4768)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 算子桶按字段类型划分且互不包含（select 有 in/notIn，text 没有；between 只有日期桶有）。切换一行的 field 只写 { field }，算子就留在了新桶没有的 位置——Radix 的 SelectValue 拿实际挂载的 SelectItem 匹配，于是 trigger 渲染成空白，用户看不见也点不到自己正在用的算子。 现在把「改字段」和「定算子、定 value 形状」合成一次编辑，与 #3958 / PR #4762 在算子侧的机制同形：新桶仍提供的算子予以保留（value 一并不动）， 新桶不提供的换成该桶首项并按新家族重整 value。归属判断走 spec 自己的 normalizeFilterOperator，所以存量视图读回的 not_in 会被认成下拉里列的 notIn，不会被无谓重置；该折叠在本组件的 22 个算子 id 上是单射的，测试 把这条性质一并钉住。 Co-authored-by: Claude --- ...er-builder-field-switch-resets-operator.md | 33 ++ ...ter-builder-field-switch-operator.test.tsx | 323 ++++++++++++++++++ .../components/src/custom/filter-builder.tsx | 83 ++++- 3 files changed, 436 insertions(+), 3 deletions(-) create mode 100644 .changeset/filter-builder-field-switch-resets-operator.md create mode 100644 packages/components/src/__tests__/filter-builder-field-switch-operator.test.tsx diff --git a/.changeset/filter-builder-field-switch-resets-operator.md b/.changeset/filter-builder-field-switch-resets-operator.md new file mode 100644 index 000000000..b2dbd1e0b --- /dev/null +++ b/.changeset/filter-builder-field-switch-resets-operator.md @@ -0,0 +1,33 @@ +--- +'@object-ui/components': patch +--- + +`FilterBuilder` settles a row's operator when its **field** changes, instead of leaving an operator the new field's dropdown does not list. + +The operator buckets are per field type and they do not nest: a `select` column +offers `in` / `notIn`, a `text` column offers none of them, and only a date +column offers `between`. Changing a row's field wrote `{ field }` alone, so the +operator survived into a bucket that no longer contained it. Radix's +`SelectValue` matches against the `SelectItem`s actually mounted, so the +operator trigger rendered **blank** — while the row went on filtering by an +operator the user could neither see nor reach, and could only clear by deleting +the row. + +Changing the field is now one edit with the operator and the value's shape, the +same way objectui#3958 / PR #4762 made changing the operator one edit with the +value's shape: + +- an operator the new field's bucket still offers is **kept** — switching + `contains` from one text column to another must not silently become `equals`, + and the value it carries is left alone; +- one the new bucket cannot offer is replaced by that bucket's **first** entry, + and the row's `value` is then re-shaped for the family it lands in — a list + under `in` collapses to its first entry under `equals`, a `between` range + keeps its lower bound, an untouched `[]` becomes `''`. + +Membership is decided through the spec's own `normalizeFilterOperator`, the fold +`filterValueArity` already uses, so a stored rule that reaches the builder +spelled `not_in` is recognised as the operator the dropdown lists as `notIn` and +is not reset out from under the author. The fold is injective over this +builder's whole operator vocabulary, which is what makes comparing through it +safe; a test pins that, and fails the day an added operator would break it. diff --git a/packages/components/src/__tests__/filter-builder-field-switch-operator.test.tsx b/packages/components/src/__tests__/filter-builder-field-switch-operator.test.tsx new file mode 100644 index 000000000..7c8c50381 --- /dev/null +++ b/packages/components/src/__tests__/filter-builder-field-switch-operator.test.tsx @@ -0,0 +1,323 @@ +/** + * 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. + */ + +/** + * Changing a row's FIELD must leave the row's operator inside the new field's + * bucket (objectui#4768). + * + * The buckets are per field type and they do not nest: `select` offers + * `in` / `notIn`, `text` does not, `date` offers `between` and nobody else + * does. Changing the field used to write `{ field }` alone, so a row built on a + * `select` column with `in` kept `in` after being pointed at a `text` column — + * an operator that column's dropdown does not list. Radix's `SelectValue` + * matches against the `SelectItem`s actually mounted, so the trigger rendered + * BLANK while the row went on filtering by an operator the user could neither + * see nor reach. + * + * This is the field-side half of the family objectui#3958 / PR #4762 opened on + * the operator side: there, changing the operator had to re-shape the value; + * here, changing the field has to settle the operator AND then the value, for + * the same reason — a row's field, operator and value shape are one edit. + * + * DIRECTION, predicted before running: every pin in the first three `describe` + * blocks is RED on `origin/main` (the field switch keeps the stale operator + * there) and green after. The two exceptions are stated where they sit: + * "leaves an operator the new bucket still offers alone" is green in BOTH + * directions — it is the guard that stops the fix from over-reaching into + * discarding valid user choices, and a fix that reset unconditionally would + * turn it red. The `describe` on the canonical fold is likewise green in both + * directions on `main`: `reconcileOperatorForField` does not exist there, so + * those cases do not compile rather than fail — they are pins on the NEW + * helper's contract, not on the old behaviour. + */ +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 { normalizeFilterOperator } from '@objectstack/spec/ui'; +import { + FilterBuilder, + FILTER_BUILDER_OPERATORS, + operatorsForFieldType, + reconcileOperatorForField, +} from '../custom/filter-builder'; + +/** + * One column per bucket this builder draws, plus a SECOND text column: the + * within-one-bucket switch is the case that must NOT reset, and it needs two + * fields of the same type to be expressible at all. + */ +const FIELDS = [ + { value: 'stage', label: 'Stage', type: 'select' }, + { value: 'title', label: 'Title', type: 'text' }, + { value: 'owner_note', label: 'Owner note', type: 'text' }, + { value: 'amount', label: 'Amount', type: 'number' }, + { value: 'closed_at', label: 'Closed at', type: 'date' }, + { value: 'is_won', label: 'Won', type: 'boolean' }, +]; + +/** The operator ids the dropdown offers for a field of `type`, ungranted. */ +function offeredFor(type: string): string[] { + return operatorsForFieldType(type).map((op) => op.value); +} + +function renderRow(condition: Record) { + const onChange = vi.fn(); + // Hoisted OUT of the JSX for the same reason PR #4762's suite hoists it: + // `FilterBuilder`'s sync effect re-seeds its internal state whenever the + // `value` PROP's identity changes, so an inline literal would undo the + // interaction under test on the next render. + const value = { id: 'root', logic: 'and', conditions: [{ id: 'c1', ...condition }] }; + const utils = render( + , + ); + return { ...utils, onChange }; +} + +/** The row the component handed back on its most recent `onChange`. */ +function lastRow(onChange: ReturnType) { + const calls = onChange.mock.calls; + expect(calls.length, 'the builder never called onChange').toBeGreaterThan(0); + return calls[calls.length - 1][0].conditions[0]; +} + +/** + * Drive a REAL dropdown. The row's controls are Radix comboboxes in + * field / operator / value order, so index 0 is the field and 1 the operator. + */ +async function pick(triggerIndex: 0 | 1, label: string) { + const triggers = screen.getAllByRole('combobox'); + fireEvent.keyDown(triggers[triggerIndex], { key: 'ArrowDown' }); + const option = await waitFor(() => { + const found = screen.getAllByRole('option').find((o) => o.textContent === label); + expect(found, `no "${label}" option in the dropdown`).toBeTruthy(); + return found!; + }); + fireEvent.click(option); +} + +/** What the operator trigger DISPLAYS — blank is the symptom this card is about. */ +function operatorTriggerText() { + return screen.getAllByRole('combobox')[1].textContent; +} + +describe('switching the field settles the operator into the new bucket', () => { + it('resets `in` when the new column has no set operator, and shows a label again', async () => { + // The exact row from the report: a `select` column filtered with `in`, + // pointed at a `text` column whose bucket has no `in` at all. + const { onChange } = renderRow({ field: 'stage', operator: 'in', value: ['won', 'lost'] }); + await pick(0, 'Title'); + + const row = lastRow(onChange); + expect(row.field).toBe('title'); + expect(row.operator).toBe('equals'); + expect(offeredFor('text')).toContain(row.operator); + // The visible symptom, pinned directly: the trigger showed nothing at all + // because `in` was not among the items the text bucket mounts. + expect(operatorTriggerText()).toBe('Equals'); + }); + + it('resets `between` when the new column is not a date one', async () => { + const { onChange } = renderRow({ + field: 'closed_at', + operator: 'between', + value: ['2024-01-01', '2024-12-31'], + }); + await pick(0, 'Title'); + + const row = lastRow(onChange); + expect(row.operator).toBe('equals'); + expect(offeredFor('text')).toContain(row.operator); + }); + + it('resets a text-only operator when the new column is numeric', async () => { + // The reverse direction of the same fact: `contains` is a text operator and + // the number bucket does not carry it. + const { onChange } = renderRow({ field: 'title', operator: 'contains', value: 'ac' }); + await pick(0, 'Amount'); + + const row = lastRow(onChange); + expect(row.field).toBe('amount'); + expect(row.operator).toBe('equals'); + expect(offeredFor('number')).toContain(row.operator); + }); + + it('resets down to the two operators a boolean column offers', async () => { + // The narrowest bucket in the builder — `["equals", "notEquals"]` — so it + // is the one most switches land outside of. + const { onChange } = renderRow({ field: 'title', operator: 'startsWith', value: 'ac' }); + await pick(0, 'Won'); + + expect(lastRow(onChange).operator).toBe('equals'); + expect(offeredFor('boolean')).toContain(lastRow(onChange).operator); + }); + + it('the reset operator is the new bucket’s FIRST entry, not a hard-coded id', () => { + // `equals` is what every bucket happens to start with today. The rule is + // "the first offered entry", and it is asserted as such so that reordering + // a bucket moves the fallback with it instead of silently disagreeing. + for (const type of ['text', 'number', 'date', 'select', 'lookup', 'boolean']) { + const offered = operatorsForFieldType(type); + expect(reconcileOperatorForField('nonexistentOperator', offered)).toBe(offered[0].value); + } + }); +}); + +describe('the value is re-shaped for the family the operator lands in', () => { + it('a list value collapses to its first entry when the operator becomes scalar', async () => { + const { onChange } = renderRow({ field: 'stage', operator: 'in', value: ['won', 'lost'] }); + await pick(0, 'Title'); + + const row = lastRow(onChange); + // `equals` compares against ONE value; carrying the array through would + // leave the row in the very shape `ViewFilterRuleSchema` refuses, which is + // the defect PR #4762 closed on the operator side. + expect(row.value).toBe('won'); + }); + + it('an untouched list row becomes the empty scalar, not `[]`', async () => { + const { onChange } = renderRow({ field: 'stage', operator: 'in', value: [] }); + await pick(0, 'Title'); + + expect(lastRow(onChange)).toMatchObject({ operator: 'equals', value: '' }); + }); + + it('a range keeps its lower bound when the operator becomes scalar', async () => { + const { onChange } = renderRow({ + field: 'closed_at', + operator: 'between', + value: ['2024-01-01', '2024-12-31'], + }); + await pick(0, 'Amount'); + + expect(lastRow(onChange).value).toBe('2024-01-01'); + }); + + it('a scalar value is carried through untouched — only the SHAPE is settled', async () => { + // Deliberately not blanked: the reset answers the operator's family, and + // scalar-to-scalar has no shape question to answer. What the user typed is + // theirs; a field switch is not a licence to discard it. + const { onChange } = renderRow({ field: 'title', operator: 'contains', value: 'acme' }); + await pick(0, 'Amount'); + + expect(lastRow(onChange)).toMatchObject({ operator: 'equals', value: 'acme' }); + }); +}); + +describe('an operator the new bucket still offers is LEFT ALONE', () => { + // Green in both directions by design: `main` does not reset at all, so it + // passes there too. It is the guard on the fix's blast radius — a fix that + // reset the operator on every field change would turn all three red. + it('keeps `contains` across two text columns, value and all', async () => { + const { onChange } = renderRow({ field: 'title', operator: 'contains', value: 'acme' }); + await pick(0, 'Owner note'); + + expect(lastRow(onChange)).toMatchObject({ + field: 'owner_note', + operator: 'contains', + value: 'acme', + }); + expect(operatorTriggerText()).toBe('Contains'); + }); + + it('keeps `in` and its list across select → lookup-shaped buckets', () => { + // Driven through the pure helper rather than the dropdown: both buckets + // offer `in`, so the operator survives and the value must not be re-shaped. + expect(reconcileOperatorForField('in', operatorsForFieldType('lookup'))).toBe('in'); + expect(reconcileOperatorForField('in', operatorsForFieldType('select'))).toBe('in'); + }); + + it('keeps a value-less operator both buckets carry', async () => { + const { onChange } = renderRow({ field: 'title', operator: 'isNull', value: '' }); + await pick(0, 'Amount'); + + expect(lastRow(onChange)).toMatchObject({ field: 'amount', operator: 'isNull' }); + }); +}); + +describe('membership is decided through the spec’s canonical fold', () => { + it('recognises the canonical spelling of an operator the bucket lists as an alias', () => { + // A stored view read back without camelCasing carries `not_in`; the + // dropdown lists the alias `notIn`. They are ONE operator, so a + // select → lookup switch must not reset the row to `equals` merely + // because the two spellings differ — and the row keeps its own spelling, + // because a field switch is not a spelling migration. + expect(normalizeFilterOperator('notIn')).toBe('not_in'); + expect(offeredFor('lookup')).toContain('notIn'); + expect(offeredFor('lookup')).not.toContain('not_in'); + expect(reconcileOperatorForField('not_in', operatorsForFieldType('lookup'))).toBe('not_in'); + }); + + it('still resets a canonical spelling the new bucket cannot express', () => { + // The other half: `not_in` is canonically absent from the text bucket, so + // the fold must not turn "compare canonically" into "accept anything". + expect(reconcileOperatorForField('not_in', operatorsForFieldType('text'))).toBe('equals'); + }); + + it('the fold is INJECTIVE over this builder’s vocabulary', () => { + // What makes comparing through `normalizeFilterOperator` safe: if two + // OFFERED ids collapsed onto one canonical operator, the check could keep + // an operator the bucket does not actually list. They do not collapse, and + // this pin fails the day an added operator would make them. + const canonical = FILTER_BUILDER_OPERATORS.map((id) => normalizeFilterOperator(id)); + expect(new Set(canonical).size).toBe(FILTER_BUILDER_OPERATORS.length); + }); +}); + +describe('the invariant, over every field-type pair the builder can draw', () => { + const TYPES = ['text', 'number', 'currency', 'date', 'datetime', 'select', 'lookup', 'boolean']; + + it('after any switch, the operator is one the NEW bucket offers', () => { + for (const from of TYPES) { + for (const to of TYPES) { + const target = operatorsForFieldType(to); + for (const op of offeredFor(from)) { + const settled = reconcileOperatorForField(op, target); + expect( + offeredFor(to), + `${from} → ${to} left "${op}" as "${settled}"`, + ).toContain(settled); + } + } + } + }); + + it('and it holds for the canonical spellings a stored rule can carry', () => { + for (const to of TYPES) { + const target = operatorsForFieldType(to); + for (const op of FILTER_BUILDER_OPERATORS) { + const settled = reconcileOperatorForField(normalizeFilterOperator(op), target); + const offered = offeredFor(to); + const ok = + offered.includes(settled) || + offered.some((id) => normalizeFilterOperator(id) === normalizeFilterOperator(settled)); + expect(ok, `→ ${to} left "${op}" as "${settled}"`).toBe(true); + } + } + }); +}); + +describe('PR #4762’s operator-side behaviour is unchanged by this', () => { + it('switching the operator after a field switch still re-shapes the value', async () => { + const { onChange } = renderRow({ field: 'title', operator: 'equals', value: 'won' }); + // `equals` is offered by the select bucket too, so the field switch keeps + // it — and the value with it. + await pick(0, 'Stage'); + expect(lastRow(onChange)).toMatchObject({ field: 'stage', operator: 'equals', value: 'won' }); + + // Then the operator switch does what PR #4762 made it do. + await pick(1, 'In'); + expect(lastRow(onChange)).toMatchObject({ operator: 'in', value: ['won'] }); + }); + + it('a field switch that resets to `equals` leaves a row `in` can be re-entered from', async () => { + const { onChange } = renderRow({ field: 'stage', operator: 'in', value: ['won'] }); + await pick(0, 'Title'); + expect(lastRow(onChange)).toMatchObject({ field: 'title', operator: 'equals', value: 'won' }); + }); +}); diff --git a/packages/components/src/custom/filter-builder.tsx b/packages/components/src/custom/filter-builder.tsx index dd22db160..5b1325a62 100644 --- a/packages/components/src/custom/filter-builder.tsx +++ b/packages/components/src/custom/filter-builder.tsx @@ -341,6 +341,50 @@ export function reshapeFilterValue( } } +/** + * The operator a row keeps after its FIELD is changed, given the operators the + * NEW field offers (objectui#4768). + * + * Each field type has its own operator bucket, and the buckets are not nested: + * a `select` column offers `in` / `notIn`, a `text` column does not. Changing + * the field used to write `{ field }` alone, so the row's operator survived + * into a bucket that no longer contains it — `in` on a text column. The Radix + * trigger then had nothing to render (its `SelectValue` matches against the + * `SelectItem`s actually mounted) and went BLANK, while the row went on + * filtering by an operator the user could no longer see or reach. + * + * So the two edits are one edit, exactly as `changeOperator` made the operator + * and its value shape one edit: an operator the new bucket still offers is the + * user's choice and is KEPT; one it does not is replaced by the bucket's first + * entry (`equals` for every bucket this builder draws), and the caller then + * re-shapes the value for that operator's family. + * + * Membership is decided CANONICALLY, through the spec's own + * `normalizeFilterOperator` — the same fold `filterValueArity` uses. A stored + * rule can reach this builder spelled `not_in` while the dropdown lists the + * alias `notIn`; those are one operator, so a select→lookup switch must not + * silently rewrite the author's operator to `equals` just because the two + * spellings differ. The fold is safe to compare through because it is + * INJECTIVE over this builder's vocabulary — all 22 ids in `defaultOperators` + * normalize to 22 distinct canonical operators, pinned in + * `filter-builder-field-switch-operator.test.tsx` — so no two OFFERED + * operators can ever collapse onto one another. + * + * @internal exported for tests + */ +export function reconcileOperatorForField( + operator: string, + offeredOperators: ReadonlyArray<{ value: string }>, +): string { + const canonical = normalizeFilterOperator(operator) + const stillOffered = offeredOperators.some( + (op) => normalizeFilterOperator(op.value) === canonical, + ) + // Kept in the row's OWN spelling: a field switch is not a spelling migration. + if (stillOffered) return operator + return offeredOperators[0]?.value ?? operator +} + /** The two bounds a `pair` row edits, with the gaps filled in for rendering. */ function toPairBounds( value: FilterBuilderCondition["value"], @@ -566,6 +610,41 @@ function FilterBuilder({ return operatorsForFieldType(field?.type, extraOperators) } + /** + * Change a row's field AND, when the new field's bucket no longer offers the + * row's operator, reset that operator — re-shaping the value for the family + * it lands in (objectui#4768). + * + * Deliberately not `updateCondition(id, { field })`: the operator buckets are + * per field type and do not nest, so that update could leave `in` on a text + * column — an operator the dropdown no longer lists, which rendered as a + * BLANK trigger. The same shape as `changeOperator` above and for the same + * reason: a row's field, its operator and its value shape are not + * independently settable, so they are settled together here rather than left + * for each caller to remember. + * + * An operator the new bucket DOES offer is left alone — resetting it would + * throw away a choice that is still valid (switching `contains` from one text + * column to another must not silently become `equals`). + */ + const changeField = (conditionId: string, nextField: string) => { + const offered = getOperatorsForField(nextField) + handleChange({ + ...filterGroup, + conditions: filterGroup.conditions.map((c) => { + if (c.id !== conditionId) return c + const nextOperator = reconcileOperatorForField(c.operator, offered) + if (nextOperator === c.operator) return { ...c, field: nextField } + return { + ...c, + field: nextField, + operator: nextOperator, + value: reshapeFilterValue(c.value, nextOperator), + } + }), + }) + } + // The complement of the exported set, never a second literal beside it: // that set's whole job is to let other layers know which rows this builder // leaves value-less, and a hand-kept copy here is how they drifted apart. @@ -813,9 +892,7 @@ function FilterBuilder({