diff --git a/.changeset/9302-filter-builder-valueless-canonical-fold.md b/.changeset/9302-filter-builder-valueless-canonical-fold.md new file mode 100644 index 0000000000..235fb83ea1 --- /dev/null +++ b/.changeset/9302-filter-builder-valueless-canonical-fold.md @@ -0,0 +1,51 @@ +--- +'@object-ui/components': patch +--- + +fix(components): `FilterBuilder`'s value-input gate folds the operator, so one operator draws one row whichever spelling it arrives in (objectui#9302) + +`needsValueInput` did a raw `has()` against `VALUELESS_FILTER_BUILDER_OPERATORS`, +whose members are this builder's six camelCase dropdown ids. Every OTHER +spelling the spec publishes for those same operators missed the set and was +treated as value-taking — the canonical members of `VIEW_FILTER_OPERATORS` that +those ids fold onto, which is what `foldFilterGroupToSpecRules` persists and what +any spec-side producer emits, and every row of `VIEW_FILTER_OPERATOR_ALIASES` +pointing at one of them. The row drew a box to type a value into, directly beside +a trigger reading `Is null`. + +Which spellings those are is deliberately not listed here. The pin walks those +two published tables and names every row it measured, so the population is +re-derived on each run rather than restated in prose that cannot move with it — +an earlier draft of this changeset hand-listed that population and undercounted +it, which is why it is named by instrument here (objectui#9302). Measured +through the real builder on one `text` column, before the repair: every +spelling the pin's own `firing` filter yields — the enumerated rows that are +not themselves members of the exported set, so each has a DIFFERENT spelling as +its twin — drew **1** value input where that camelCase dropdown twin drew **0**, +with `equals` drawing 1 in both arms (it really does take a value) — the firing +control that makes a uniform zero a reading rather than a dead harness. + +Both sides of the lookup now fold through the spec's `normalizeFilterOperator`. +That is the same fold `filterValueArity` and `reconcileOperatorForField` in this +component already perform, so this is one more site joining a fold the file does +rather than a new dialect, and the canonical set is DERIVED from the exported one +rather than restated beside it. + +Pre-existing, and objectui#7561 did not cause it: before that repair the same +stray input was drawn under a **blank** trigger. What changed is that the +contradiction became legible. + +**The exported set is unchanged, deliberately.** Its stated job is *which rows +this builder leaves value-less* — a fact about the dropdown's own ids — and two +other layers read it: `plugin-list`'s `convertFilterGroupToAST` (what the live +grid queries) and `app-shell`'s `foldFilterGroupToSpecRules` (what a saved view +persists, already documented as this set plus the canonical spellings only that +layer sees). Widening it here would have made that layer's deliberate +compensation redundant by side effect, in a file nobody is editing. The defect +was never a set missing members; it was a reader that forgot to normalize its +input, so the reader is what was repaired. No published type or export changes, +and no row's stored `operator` is rewritten — rendering is not an edit. + +Which operator vocabulary should WIN is a separate, still-open question and is +not decided here. The `contains` / `icontains` boundary is untouched and pinned: +the fold this gate routes through maps neither onto the other. diff --git a/packages/components/src/__tests__/filter-builder-valueless-canonical-spelling-9302.test.tsx b/packages/components/src/__tests__/filter-builder-valueless-canonical-spelling-9302.test.tsx new file mode 100644 index 0000000000..4db003dc75 --- /dev/null +++ b/packages/components/src/__tests__/filter-builder-valueless-canonical-spelling-9302.test.tsx @@ -0,0 +1,411 @@ +/** + * 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 value-input gate reads the operator through the same fold the rest of + * this component already uses, so BOTH spellings of one value-less operator + * draw the SAME row (objectui#9302). + * + * ## The one site + * + * `needsValueInput` did a raw `has()` against + * `VALUELESS_FILTER_BUILDER_OPERATORS`, whose members are this builder's six + * camelCase dropdown ids. A row carrying the spec's canonical spelling + * (`is_null`) missed the set and was treated as value-taking, so it drew a + * value box next to a trigger that reads `Is null` — two spellings of ONE + * operator drawing two different rows. + * + * This is the same CLASS as objectui#7561 and not the same site: that card + * repaired the operator trigger's identity comparison, this is the value-input + * gate. The component already imports `normalizeFilterOperator` and already + * folds through it in `filterValueArity` and `reconcileOperatorForField`, so + * the repair is one more site joining a fold this file already performs. + * + * ## What the repair deliberately does NOT do (the PM ruling on this card) + * + * - ⛔ the EXPORTED set is not widened. Its stated job is *which rows this + * builder leaves value-less* — a fact about the dropdown's own six ids — + * and two other layers read it: `plugin-list`'s `convertFilterGroupToAST` + * (what the live grid QUERIES) and `app-shell`'s + * `foldFilterGroupToSpecRules` (what a saved view PERSISTS, documented as + * this set PLUS the canonical spellings only that layer sees). Widening + * here would make another layer's deliberate compensation redundant by + * side effect, in a file nobody is editing. + * - ⛔ no row's stored `operator` is rewritten; rendering is not an edit. + * - ⛔ no second spelling joins the dropdown; the emitted vocabulary is + * unchanged. + * + * ⛔ Out of scope, and untouched here: WHICH operator vocabulary wins + * (objectui#7561's open question, filed as decision card objectui#9306). It + * carries a hard constraint recorded on objectui#7379 — `AST_OPERATOR_MAP` + * holds that `contains` / `icontains` "must never be folded onto" each other, + * "That is a semantic boundary, not two spellings of one thing." The boundary + * guard below pins that the fold this repair routes through does not cross it. + * + * ## DIRECTION, predicted before running + * + * On the unmodified tree every spelling of a value-less operator that is NOT + * literally a member of the exported set is RED — each draws 1 value input + * where 0 is required. ⛔ That population is not written down here: the derived + * table below walks `VIEW_FILTER_OPERATORS` and `VIEW_FILTER_OPERATOR_ALIASES` + * and `--reporter=verbose` names each row it measured. Writing the count down + * is what went wrong the first time — the literal table named the canonical + * spellings and missed the all-lowercase alias rows, which move exactly the + * same way and had nothing pinning them — the first pass of this pin, on the + * branch that adds it, made exactly that mistake. + * + * Everything else is GREEN in both directions and is carried for a named + * reason, not for coverage: + * + * - the six DROPDOWN ids draw 0 today and must keep drawing 0 — the + * over-reach guard. A repair that folded only one direction, or that + * replaced the set's members with canonical spellings, moves these; + * - `equals` is the FIRING CONTROL. It really does take a value and must + * draw exactly 1 both before and after. Without it "0 inputs everywhere" + * and a broken renderer are the same reading; + * - `exists` / `notExists` are builder-only ids with no canonical twin in + * the spec's vocabulary, so the fold returns them verbatim. They pin that + * routing the lookup through the fold did not drop the two members that + * have nothing to fold to. + */ +import { describe, it, expect, vi } from 'vitest'; +import React from 'react'; +import { render, screen, cleanup } from '@testing-library/react'; +import '@testing-library/jest-dom'; +import { + normalizeFilterOperator, + VIEW_FILTER_OPERATORS, + VIEW_FILTER_OPERATOR_ALIASES, +} from '@objectstack/spec/ui'; +import { + FilterBuilder, + VALUELESS_FILTER_BUILDER_OPERATORS, +} from '../custom/filter-builder'; + +/** One `text` column — the card's reproduction shape. */ +const FIELDS = [{ value: 'title', label: 'Title', type: 'text' }]; + +/** + * One condition row. `value: ''` is the seed `addCondition` writes and what a + * row still carries after the operator dropdown is changed, i.e. the exact + * state the panel holds while the user edits. + * + * The `value` prop is hoisted out of the JSX because the builder re-seeds its + * internal state when that prop's IDENTITY changes. + */ +function renderRow(operator: string, extraOperators: readonly string[] = []) { + const onChange = vi.fn(); + const value = { + id: 'root', + logic: 'and', + conditions: [{ id: 'c1', field: 'title', operator, value: '' }], + }; + const utils = render( + , + ); + return { ...utils, onChange }; +} + +/** + * The value input is the only `` the row draws — the field and operator + * pickers are Radix selects, i.e. buttons. Counting elements rather than + * matching a placeholder keeps this independent of the active locale. + */ +const valueInputCount = (container: HTMLElement) => container.querySelectorAll('input').length; + +/** What the operator trigger DISPLAYS — the label the stray box contradicts. */ +const operatorTriggerText = () => screen.getAllByRole('combobox')[1].textContent; + +/** + * The card's acceptance table, measured through the real `FilterBuilder`. + * + * `dialect` is load-bearing for reading a failure, not decoration: + * - `dropdown` — one of this builder's own camelCase ids, i.e. a member of + * the exported set. 0 inputs today and after: the over-reach guard; + * - `canonical` — `@objectstack/spec`'s snake_case spelling of the SAME + * operator, which a stored view carries. 1 input today (the defect), 0 + * after: the firing cases; + * - `control` — really does take a value. 1 input in both directions. + */ +const ROWS: ReadonlyArray<{ + operator: string; + label: string; + inputs: number; + dialect: 'dropdown' | 'canonical' | 'control'; + /** `OPT_IN_OPERATORS` ids the consumer must grant before they are mounted. */ + extraOperators?: readonly string[]; +}> = [ + { operator: 'isNull', label: 'Is null', inputs: 0, dialect: 'dropdown' }, + { operator: 'is_null', label: 'Is null', inputs: 0, dialect: 'canonical' }, + { operator: 'isNotNull', label: 'Is not null', inputs: 0, dialect: 'dropdown' }, + { operator: 'is_not_null', label: 'Is not null', inputs: 0, dialect: 'canonical' }, + { operator: 'isEmpty', label: 'Is empty', inputs: 0, dialect: 'dropdown' }, + { operator: 'is_empty', label: 'Is empty', inputs: 0, dialect: 'canonical' }, + { operator: 'isNotEmpty', label: 'Is not empty', inputs: 0, dialect: 'dropdown' }, + { operator: 'is_not_empty', label: 'Is not empty', inputs: 0, dialect: 'canonical' }, + // No canonical twin exists for these two — the spec's vocabulary has no + // `exists` member and its alias table deliberately has no row for one. + // + // They are `OPT_IN_OPERATORS` (objectui#4736), so a consumer that does not + // grant them never mounts their `SelectItem` and the trigger reads blank. + // Granting them here is what makes this a reading about the value gate + // rather than about the opt-in gate — measured: without the grant the + // trigger is `''`, which is the documented opt-in behaviour and not a defect. + { + operator: 'exists', + label: 'Is set', + inputs: 0, + dialect: 'dropdown', + extraOperators: ['exists', 'notExists'], + }, + { + operator: 'notExists', + label: 'Is not set', + inputs: 0, + dialect: 'dropdown', + extraOperators: ['exists', 'notExists'], + }, + // THE FIRING CONTROL, in the same render as the rest. + { operator: 'equals', label: 'Equals', inputs: 1, dialect: 'control' }, +]; + +/** + * ## The DERIVED table — the instrument, not a second literal list + * + * `ROWS` above is a hand-written table, and a hand-written table is exactly how + * a spelling goes unmeasured: the first pass of this pin, on the branch that + * adds it, named four canonical rows and stopped there, while the repair moves + * every spelling the spec publishes for those operators. So the row set below + * is not listed — it is re-derived on every run from the two published tables, + * and whatever they hold on the day of the run is what gets measured. + * + * - `VIEW_FILTER_OPERATORS` — the spec's canonical vocabulary. Intersected + * with the exported set folded onto it, this yields the canonical spellings + * THIS builder leaves value-less. Nothing here decides which those are; the + * builder's own export does, and the fold carries it across. + * - `VIEW_FILTER_OPERATOR_ALIASES` — every other spelling the spec accepts + * for one of those canonical members. The camelCase rows of this table are + * the builder's own dropdown ids; the rest (today: the all-lowercase rows) + * are spellings no literal table here ever named. + * + * ⛔ Do not replace this with the list it currently produces. The count is not + * written down anywhere in this file on purpose — `--reporter=verbose` names + * each case it ran, and the guards below fail rather than run zero cases. + */ +const foldedExportedSet: ReadonlySet = new Set( + [...VALUELESS_FILTER_BUILDER_OPERATORS].map(normalizeFilterOperator), +); + +/** The canonical spellings this builder leaves value-less — derived, not listed. */ +const CANONICAL_VALUELESS: readonly string[] = ( + VIEW_FILTER_OPERATORS as readonly string[] +).filter((op) => foldedExportedSet.has(op)); + +/** + * The dropdown id that folds onto a canonical spelling — that row's TWIN, and + * the anchor each derived case is read against. `exists` / `notExists` have no + * canonical member to fold to, so they never appear here; they stay in `ROWS`. + */ +const dropdownTwin = (canonical: string): string | undefined => + [...VALUELESS_FILTER_BUILDER_OPERATORS].find( + (id) => normalizeFilterOperator(id) === canonical, + ); + +/** Every spelling the spec publishes for one of those operators. */ +const SPEC_SPELLINGS: ReadonlyArray<{ + spelling: string; + canonical: string; + dialect: 'canonical' | 'alias(dropdown id)' | 'alias'; +}> = [ + ...CANONICAL_VALUELESS.map( + (canonical) => ({ spelling: canonical, canonical, dialect: 'canonical' }) as const, + ), + ...Object.entries(VIEW_FILTER_OPERATOR_ALIASES) + .filter(([, canonical]) => CANONICAL_VALUELESS.includes(canonical)) + .map(([spelling, canonical]) => ({ + spelling, + canonical: canonical as string, + dialect: VALUELESS_FILTER_BUILDER_OPERATORS.has(spelling) + ? ('alias(dropdown id)' as const) + : ('alias' as const), + })), +]; + +describe('objectui#9302 — every spelling the spec publishes for these operators, derived', () => { + it.each(SPEC_SPELLINGS)( + '$dialect `$spelling` folds to `$canonical` and draws its twin row with 0 value inputs', + ({ spelling, canonical }) => { + const twin = dropdownTwin(canonical); + expect(twin, `no dropdown id folds onto \`${canonical}\``).toBeDefined(); + + // The twin is this reading's anchor, so read it first and refuse a + // degenerate one: a BLANK trigger, or one echoing the raw id, would make + // "draws the same row" vacuously true for every spelling at once. + const twinRender = renderRow(twin as string); + const twinTrigger = operatorTriggerText(); + expect(twinTrigger, `the twin \`${twin}\` drew a blank trigger`).toMatch(/\S/); + expect(twinTrigger, `the twin \`${twin}\` echoed its raw id`).not.toBe(twin); + expect(valueInputCount(twinRender.container)).toBe(0); + cleanup(); + + const { container } = renderRow(spelling); + expect(operatorTriggerText()).toBe(twinTrigger); + expect( + valueInputCount(container), + `the builder drew ${valueInputCount(container)} value input(s) for "${spelling}" ` + + `but the row reads "${twinTrigger}". Two spellings of one operator must draw one row`, + ).toBe(0); + }, + ); + + it('⛔ the derived table is not vacuous, and it reaches past the literal one', () => { + // An `it.each` over an empty or mis-derived array runs ZERO cases and is + // green, so the derivation is guarded here rather than trusted. These are + // FLOORS, deliberately not a census — they fail when the derivation breaks + // and stay quiet when the spec grows a spelling (which is the point). + expect(CANONICAL_VALUELESS.length).toBeGreaterThanOrEqual(4); + expect(SPEC_SPELLINGS.length).toBeGreaterThan(CANONICAL_VALUELESS.length); + + // Every canonical row has a twin to be read against and at least one alias, + // so no canonical is measured alone and no case silently skips its anchor. + for (const canonical of CANONICAL_VALUELESS) { + expect(dropdownTwin(canonical), canonical).toBeDefined(); + expect( + SPEC_SPELLINGS.filter((r) => r.dialect !== 'canonical' && r.canonical === canonical).length, + canonical, + ).toBeGreaterThanOrEqual(1); + } + + // The instrument reaches spellings `ROWS` never named — the gap this pin's + // own first pass left. Without this the derived table could quietly shrink + // to the literal one and still be green. + const literal = new Set(ROWS.map((r) => r.operator)); + const beyond = SPEC_SPELLINGS.filter((r) => !literal.has(r.spelling)); + expect(beyond.length, 'the derived table adds nothing over the literal one').toBeGreaterThanOrEqual(4); + + // …and it never drifts BEHIND it: every literal row the spec knows is + // derived too, so the two tables cannot disagree about a shared spelling. + const derived = new Set(SPEC_SPELLINGS.map((r) => r.spelling)); + for (const op of literal) { + const specKnows = + (VIEW_FILTER_OPERATORS as readonly string[]).includes(op) + || Object.prototype.hasOwnProperty.call(VIEW_FILTER_OPERATOR_ALIASES, op); + if (specKnows && foldedExportedSet.has(normalizeFilterOperator(op))) { + expect(derived.has(op), op).toBe(true); + } + } + }); +}); + +describe('objectui#9302 — one operator, one row, whichever spelling it arrives in', () => { + it.each(ROWS)( + '$dialect `$operator` draws "$label" and $inputs value input(s)', + ({ operator, label, inputs, extraOperators }) => { + const { container } = renderRow(operator, extraOperators); + // Read the trigger in the same render: a row that draws no value input + // under a BLANK trigger is a different (and worse) outcome than the one + // this card asks for, and only reading both can tell them apart. + expect(operatorTriggerText()).toBe(label); + expect( + valueInputCount(container), + `the builder drew ${valueInputCount(container)} value input(s) for "${operator}" ` + + `but the row reads "${label}". Two spellings of one operator must draw one row`, + ).toBe(inputs); + }, + ); + + it('the firing cases really can fire — they are outside the exported set', () => { + // The instrument standard: a table built only on spellings the raw `has()` + // already matched would be green before ANY fix and would measure nothing. + // + // Read off the DERIVED table, not a literal one, so the answer to "which + // rows actually fire" comes from the published tables too. The firing rows + // are exactly those the raw `has()` could not match literally — today that + // is every canonical member plus every alias that is not itself a dropdown + // id (the first pass of this pin saw only half of them). + const firing = SPEC_SPELLINGS.filter( + (r) => !VALUELESS_FILTER_BUILDER_OPERATORS.has(r.spelling), + ); + expect(firing.length).toBeGreaterThan(CANONICAL_VALUELESS.length); + for (const row of firing) { + // Not a member — so the raw lookup could not have matched it literally… + expect(VALUELESS_FILTER_BUILDER_OPERATORS.has(row.spelling), row.spelling).toBe(false); + // …and it folds ONTO a member, which is what makes the repair reach it. + expect(foldedExportedSet.has(normalizeFilterOperator(row.spelling)), row.spelling).toBe(true); + } + // …and the control is in neither, which is why it keeps its input. + expect(VALUELESS_FILTER_BUILDER_OPERATORS.has('equals')).toBe(false); + expect(normalizeFilterOperator('equals')).toBe('equals'); + expect(foldedExportedSet.has(normalizeFilterOperator('equals'))).toBe(false); + }); +}); + +describe('objectui#9302 — ⛔ the repair moves nothing but the gate', () => { + it('the EXPORTED set keeps its dropdown-only membership', () => { + // Acceptance criterion 3, and the ruling's whole point: two other layers + // read this set, and one of them already compensates for the canonical + // spellings. Widening it would make that layer's deliberate half redundant + // by side effect. Green in both directions by construction — it fails only + // for a repair that widened the export instead of folding at the gate. + expect([...VALUELESS_FILTER_BUILDER_OPERATORS].sort()).toEqual([ + 'exists', + 'isEmpty', + 'isNotEmpty', + 'isNotNull', + 'isNull', + 'notExists', + ]); + }); + + it.each(['is_null', 'isNull'])('a row spelled `%s` is not migrated on render', (operator) => { + // Rendering is not an edit. A "repair" that rewrote the stored id to the + // dropdown's spelling would also make the table above green, and would be + // the wrong fix — it changes what a saved view carries. + const { onChange } = renderRow(operator); + expect(onChange).not.toHaveBeenCalled(); + }); + + it('an unknown operator still draws a value input', () => { + // The gate's default must stay "takes a value". A fold that swallowed + // unknown spellings into the value-less set would hide the input on a row + // nobody can then fill in — the objectui#4744 failure, inverted. + expect(normalizeFilterOperator('totally_unknown')).toBe('totally_unknown'); + const { container } = renderRow('totally_unknown'); + expect(valueInputCount(container)).toBe(1); + }); +}); + +describe('objectui#9302 — ⛔ the fold does not cross the `contains` boundary', () => { + it('`contains` and `icontains` are not folded onto each other', () => { + // objectui#7379 / objectstack#7536: `AST_OPERATOR_MAP` holds that these + // "must never be folded onto" each other — "That is a semantic boundary, + // not two spellings of one thing." The gate now routes through + // `normalizeFilterOperator`, so pin that this fold leaves the boundary + // standing. A seat that folds those two has broken a ruling, not fixed a + // bug. Green in both directions by construction — a boundary guard, not a + // control. + expect(normalizeFilterOperator('contains')).toBe('contains'); + expect(normalizeFilterOperator('icontains')).toBe('icontains'); + expect(normalizeFilterOperator('contains')).not.toBe(normalizeFilterOperator('icontains')); + // …and neither is value-less under either spelling, so the gate this card + // repairs never had an opinion about them. + for (const op of ['contains', 'icontains', 'containsCaseInsensitive']) { + expect(VALUELESS_FILTER_BUILDER_OPERATORS.has(op)).toBe(false); + expect( + new Set([...VALUELESS_FILTER_BUILDER_OPERATORS].map(normalizeFilterOperator)).has( + normalizeFilterOperator(op), + ), + ).toBe(false); + } + }); +}); diff --git a/packages/components/src/custom/filter-builder.tsx b/packages/components/src/custom/filter-builder.tsx index 6ec603fca6..9b92ee4f9d 100644 --- a/packages/components/src/custom/filter-builder.tsx +++ b/packages/components/src/custom/filter-builder.tsx @@ -225,14 +225,28 @@ export const FILTER_BUILDER_OPERATORS = defaultOperators.map(o => o.value) export type FilterBuilderOperator = (typeof defaultOperators)[number]['value'] /** - * Operator ids for which this builder renders NO value input — so "no value" - * is the row's FINISHED state, not an unfinished one (objectui#4744). + * Operator ids — this dropdown's OWN camelCase ids — for which this builder + * renders no value input, so "no value" is the row's FINISHED state, not an + * unfinished one (objectui#4744). * * This is the source of truth for that distinction, and it lives here because - * this component is the thing that decides it: `needsValueInput` below is - * defined as the complement of this set, so the two cannot say different - * things. Every consumer that has to tell a complete value-less row from a - * half-filled one reads it FROM here rather than restating it: + * this component is the thing that decides it. But membership here is not the + * whole question: `needsValueInput` below is the complement of this set's + * FOLD-CLOSURE under the spec's `normalizeFilterOperator`, not of this set + * itself (objectui#9302), so the gate answers "no value" for spellings that + * are NOT members — the canonical form `foldFilterGroupToSpecRules` persists, + * and the alias rows the spec publishes for those same operators. A consumer + * holding a spelling that did not come from this dropdown must therefore ask + * that FOLD-CLOSURE, not this set: fold this set's own members through + * `normalizeFilterOperator` as well as the spelling, and ask the result — + * the same fold the gate applies to BOTH sides. How much wider the gate's + * preimage is is NOT restated here: the pin + * `filter-builder-valueless-canonical-spelling-9302.test.tsx` walks the + * spec's two published tables and names every row it measures, which is the + * only form of that answer that moves when those tables do (AGENTS.md #9). + * + * Every consumer that has to tell a complete value-less row from a half-filled + * one reads the membership FROM here rather than restating it: * * - `plugin-list`'s `convertFilterGroupToAST` — what the live grid QUERIES; * - `app-shell`'s `foldFilterGroupToSpecRules` — what a saved view PERSISTS @@ -266,6 +280,34 @@ export const VALUELESS_FILTER_BUILDER_OPERATORS: ReadonlySet = new Set([ "notExists", ]) +/** + * The same six operators, keyed by the spelling `normalizeFilterOperator` + * folds each of them to — the lookup table the value-input gate below reads + * (objectui#9302). + * + * DERIVED, never a second literal: a hand-kept canonical copy beside the + * exported set is exactly how the two could come to disagree, and the + * disagreement would be invisible — a row that draws an input the label says + * it does not take. + * + * Why it exists separately instead of widening the export: the exported set + * states a fact about what THIS DROPDOWN draws, and its members are this + * builder's own camelCase ids. Two other layers read it — `plugin-list`'s + * `convertFilterGroupToAST` and `app-shell`'s `foldFilterGroupToSpecRules`, + * the latter already documented as this set PLUS the canonical spellings only + * that layer sees. Folding the canonical spellings INTO the export would make + * that layer's deliberate compensation redundant by side effect, in a file + * nobody is editing. The defect was never a set missing members; it was a + * reader that forgot to normalize its input. + * + * `exists` / `notExists` fold to themselves — the spec's vocabulary has no + * member for either and its alias table deliberately has no row for them — so + * this set is the same size as the one it derives from. + */ +const VALUELESS_FILTER_BUILDER_OPERATORS_CANONICAL: ReadonlySet = new Set( + [...VALUELESS_FILTER_BUILDER_OPERATORS].map(normalizeFilterOperator), +) + /** * The SHAPE an operator's `value` must have — the question * `ViewFilterRuleSchema` asks, asked here in the one place that decides what @@ -1276,11 +1318,23 @@ function FilterBuilder({ }) } - // The complement of the exported set, never a second literal beside it: + // The complement of the exported set's FOLD-CLOSURE, 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. + // + // BOTH sides are folded through the spec's `normalizeFilterOperator` — the + // same fold `filterValueArity` and `reconcileOperatorForField` already + // perform, so this is one more site joining a fold this file does rather + // than a new dialect. The gate used to do a raw `has()` on whatever spelling + // the row carried, and the set's members are the dropdown's camelCase ids: + // a stored rule spelled `is_null` — the spec's CANONICAL form, which is what + // `foldFilterGroupToSpecRules` persists and what any spec-side producer + // emits — missed the set and was treated as value-taking. The row then drew + // a box to type a value into, directly beside a trigger reading `Is null` + // (objectui#9302). One operator, two spellings, two different rows. const needsValueInput = (operator: string) => { - return !VALUELESS_FILTER_BUILDER_OPERATORS.has(operator) + return !VALUELESS_FILTER_BUILDER_OPERATORS_CANONICAL.has(normalizeFilterOperator(operator)) } // Derived from the value FAMILY rather than from a second branch ladder over