From e8deccb3042efb1de3835897a937c9446bfad1eb Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 6 Aug 2026 09:59:54 +0000 Subject: [PATCH 1/2] fix(plugin-list): drop UserFilters' private operator table, lower tab presets through the spec (#3470) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `UserFilters.specOperatorToAst` was the second hand-kept operator map in this package, and it had drifted: it lowered `not_in` — the spec's OWN canonical spelling — and the legacy `nin` to the spaced `'not in'`, which appears in no spec vocabulary. `isFilterAST` refuses it, so a `ViewTab.filter` preset written the canonical way produced an empty list plus 400 the moment its tab was clicked. Measured against a real backend (published @objectstack/*@17.0.0-rc.2 + app-showcase, showcase_task): $filter=[["status","not in",["done"]]] -> 400 {"code":"INVALID_FILTER"} $filter=[["status","not_in",["done"]]] -> 200, 8 rows $filter=[["status","!=","done"]] -> 200, the same 8 rows The table is deleted rather than repaired: all 19 VIEW_FILTER_OPERATORS are already members of VALID_AST_OPERATORS, so the rule -> AST lowering is purely structural and needs no translation. Only the legacy spellings stored metadata still carries need folding, and the spec's own `normalizeFilterOperator` does that — the same single exit the write side (viewFilterFold) and core's saved-view fold (#3431) use, so the directions cannot drift into two dialects. An unknown spelling is passed through verbatim so the server still refuses it loudly. before/after are now passed through instead of being rewritten to `<`/`>`. That was the one judgement call and it was settled by measurement, not assumption: on the same live backend the word and the symbol return identical status and identical record ids, on a `date` field and a `datetime` field, both directions. The other 18 operators were measured the same way and are unchanged in what the server answers; not_in is the only one whose answer changes, 400 -> the rows. Reverse-verified: with the table restored, 41 of the 63 new assertions go red, including the headline `isFilterAST` one. The 22 that stay green are the ones that should — the spec-vocabulary reads, the legacy triplet passthrough, and the unknown-spelling contract pin, whose behaviour the old table's `default:` branch already had. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01GTRjn8xBqp75dk7kFupVRt --- ...ilters-tab-preset-operators-single-exit.md | 9 + packages/plugin-list/src/UserFilters.tsx | 66 ++++--- .../UserFilters.tabPresetOperators.test.tsx | 176 ++++++++++++++++++ .../src/__tests__/UserFilters.test.tsx | 8 +- 4 files changed, 235 insertions(+), 24 deletions(-) create mode 100644 .changeset/user-filters-tab-preset-operators-single-exit.md create mode 100644 packages/plugin-list/src/__tests__/UserFilters.tabPresetOperators.test.tsx diff --git a/.changeset/user-filters-tab-preset-operators-single-exit.md b/.changeset/user-filters-tab-preset-operators-single-exit.md new file mode 100644 index 0000000000..748e426b95 --- /dev/null +++ b/.changeset/user-filters-tab-preset-operators-single-exit.md @@ -0,0 +1,9 @@ +--- +'@object-ui/plugin-list': patch +--- + +`UserFilters` no longer carries its own operator table when it lowers a `ViewTab.filter` preset into an ObjectQL AST node. The private `specOperatorToAst` was the second hand-kept operator map in this package and it had drifted: it lowered `not_in` — the spec's OWN canonical spelling — and the legacy `nin` to the spaced `'not in'`, a spelling that appears in no spec vocabulary. `isFilterAST` refuses it, so clicking such a tab produced an empty list plus `400 INVALID_FILTER`. Measured against a real backend (published `@objectstack/*@17.0.0-rc.2` + app-showcase, on `showcase_task`): `$filter=[["status","not in",["done"]]]` returned `400 INVALID_FILTER`, while `[["status","not_in",["done"]]]` returned `200` with the same 8 records as the `["status","!=","done"]` baseline. + +Lowering is now purely structural — all 19 `VIEW_FILTER_OPERATORS` are already members of the wire's `VALID_AST_OPERATORS`, so nothing needs translating — with the spec's own `normalizeFilterOperator` as the single exit for the legacy spellings stored metadata still carries (`gt`, `eq`, `nin`, `notEquals`, …). That is the same exit the write side (`viewFilterFold`) and the saved-view fold in `@object-ui/core` already use, so the directions cannot drift into two dialects. An operator the spec does not know is passed through verbatim, so a misspelling still earns a loud `400` rather than being coerced into a valid filter. + +`before` and `after` are now passed through rather than rewritten to `<` and `>`. That was the one judgement call, and it was settled by measurement rather than assumption: on the same live backend the word and the symbol return identical status and identical record ids, on a `date` field and on a `datetime` field, in both directions — so the rewrite was a no-op and dropping it is a pure fix. The remaining 18 canonical operators were measured the same way and are likewise unchanged in what the server answers; `not_in` is the only one whose answer changes, from `400` to the correct rows. Tab presets given in the legacy already-lowered `filters: triplet[]` shape are untouched, as before. diff --git a/packages/plugin-list/src/UserFilters.tsx b/packages/plugin-list/src/UserFilters.tsx index 8094926806..4117b4a40c 100644 --- a/packages/plugin-list/src/UserFilters.tsx +++ b/packages/plugin-list/src/UserFilters.tsx @@ -10,6 +10,7 @@ import * as React from 'react'; import { cn, Button, Popover, PopoverContent, PopoverTrigger, LookupValuePicker } from '@object-ui/components'; import { ChevronDown, X, Plus } from 'lucide-react'; import type { ListViewSchema } from '@object-ui/types'; +import { normalizeFilterOperator } from '@objectstack/spec/ui'; import { useSafeFieldLabel, useObjectTranslation } from '@object-ui/i18n'; function useMoreLabel(): string { @@ -96,31 +97,52 @@ export interface UserFiltersProps { onSelectionsChange?: (selections: Record>) => void; } -/** - * Map @objectstack/spec ViewFilterRule operators to ObjectQL AST operators. - * Accepts the canonical vocabulary (`greater_than`, `not_equals`, `before`, …) - * that the Studio filter builder now writes, plus legacy shorthand spellings - * (`gt`, `eq`, `nin`) still present in already-stored view metadata. - */ -function specOperatorToAst(op: string | undefined): string { - switch (op) { - case undefined: case 'equals': case 'eq': return '='; - case 'not_equals': case 'ne': case 'neq': return '!='; - case 'greater_than_or_equal': case 'gte': return '>='; - case 'less_than_or_equal': case 'lte': return '<='; - case 'greater_than': case 'gt': case 'after': return '>'; - case 'less_than': case 'lt': case 'before': return '<'; - case 'not_contains': return 'notcontains'; - case 'starts_with': return 'startswith'; - case 'not_in': case 'nin': return 'not in'; - default: return op; - } -} - /** * Normalize tab presets to the client shape. Accepts both: * - @objectstack/spec ViewTab: `{ name, label, filter: ViewFilterRule[], isDefault }` * - legacy client shape: `{ id, label, filters: triplet[], default }` + * + * **The spec rule → AST lowering is purely structural**, and deliberately owns + * NO operator table of its own (#3470). All 19 `VIEW_FILTER_OPERATORS` are + * already members of the wire's `VALID_AST_OPERATORS`, so nothing needs + * translating; only the legacy spellings stored view metadata still carries + * (`gt`, `eq`, `nin`, `notEquals`, …) need folding onto the canonical word, and + * that is exactly what the spec's OWN {@link normalizeFilterOperator} does — + * the same single exit the WRITE side uses (`app-shell/views/viewFilterFold.ts`) + * and the same one `@object-ui/core`'s `viewFilterRuleToNode` uses for a saved + * view's `filter` (#3431). One exit, so the directions cannot drift apart. + * + * The private table this replaced was the second hand-kept operator map in this + * package, and it had drifted: it lowered `not_in`/`nin` to the SPACED + * `'not in'`, which is in no spec vocabulary — `isFilterAST()` refuses it and + * the wire answers `400 INVALID_FILTER`. Measured against a real backend + * (published `@objectstack/*@17.0.0-rc.2` + app-showcase, `showcase_task`): + * `[["status","not in",["done"]]]` → **400 INVALID_FILTER**; + * `[["status","not_in",["done"]]]` → **200, 8 rows** (same rows as the + * `["status","!=","done"]` baseline). Every other operator the old table + * rewrote was measured to be a no-op on the answer — see below. + * + * **`before`/`after` are passed through, not mapped** (the one judgement call + * this change had to make: the old table rewrote them to `<`/`>`, and both + * words are themselves `VALID_AST_OPERATORS` members). Measured on the same + * backend, on a `date` field and a `datetime` field, both directions — + * identical status AND identical record ids: + * + * `["due_date","before","2026-08-01"]` → 200, 2 rows + * `["due_date","<","2026-08-01"]` → 200, the SAME 2 rows + * `["due_date","after","2026-08-01"]` → 200, 8 rows + * `["due_date",">","2026-08-01"]` → 200, the SAME 8 rows + * `["created_at","before","2026-08-01T00:00:00.000Z"]` → 200, 6 rows (`<` idem) + * `["created_at","after","2026-08-01T00:00:00.000Z"]` → 200, 3 rows (`>` idem) + * + * So dropping that rewrite is a pure fix, not a behaviour change. (The spec + * agrees independently: `canonicalAstOperator('before')` is `'<'`.) + * + * An operator the spec does not know is passed through **verbatim**, so + * `isFilterAST()` still refuses it and the server still answers a loud `400`. + * A misspelling must never be coerced into a valid operator (AGENTS.md #0.1): + * silently reading `bfore` as `before` would return a plausible-looking wrong + * record set instead of an error the author can see. */ function normalizeTabPresets(tabs: any[]): Array<{ id: string; label: string; filters: any[]; default?: boolean }> { return (tabs || []) @@ -133,7 +155,7 @@ function normalizeTabPresets(tabs: any[]): Array<{ id: string; label: string; fi : (Array.isArray(t.filter) ? t.filter .filter((r: any) => r && typeof r.field === 'string') - .map((r: any) => [r.field, specOperatorToAst(r.operator), r.value]) + .map((r: any) => [r.field, normalizeFilterOperator(r.operator), r.value]) : []), default: t.default ?? t.isDefault, })); diff --git a/packages/plugin-list/src/__tests__/UserFilters.tabPresetOperators.test.tsx b/packages/plugin-list/src/__tests__/UserFilters.tabPresetOperators.test.tsx new file mode 100644 index 0000000000..f704e7334c --- /dev/null +++ b/packages/plugin-list/src/__tests__/UserFilters.tabPresetOperators.test.tsx @@ -0,0 +1,176 @@ +/** + * 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. + */ + +/** + * Tab-preset operator lowering: `ViewTab.filter` → ObjectQL AST (#3470). + * + * `UserFilters` used to carry its OWN operator table (`specOperatorToAst`) — + * the second hand-kept operator map in this package — and it had drifted: it + * lowered `not_in`/`nin` to the SPACED `'not in'`, a spelling that appears in + * no spec vocabulary. `isFilterAST()` refuses it and the wire answers `400 + * INVALID_FILTER`, so a tab preset written with the spec's own canonical + * spelling produced an empty list plus an error the moment it was clicked. + * + * The table is gone. Lowering is now purely structural — all 19 + * `VIEW_FILTER_OPERATORS` are already `VALID_AST_OPERATORS` members — with the + * spec's OWN `normalizeFilterOperator` as the single exit, exactly as the write + * side (`app-shell/views/viewFilterFold.ts`) and `@object-ui/core`'s saved-view + * fold (#3431) do. + * + * These assertions pin the acceptance fact OFFLINE, using the spec's own + * `isFilterAST` — the very predicate that gates the filter server-side — so the + * fix cannot regress without a live backend in the loop. The corresponding LIVE + * measurement (published `@objectstack/*@17.0.0-rc.2` + app-showcase) is + * recorded on `normalizeTabPresets` and in the PR: `'not in'` → 400, + * `not_in` → 200 with the 8 rows the `!=` baseline returns, and `before`/`after` + * → byte-identical answers to `<`/`>` on both a `date` and a `datetime` field. + */ + +import * as React from 'react'; +import { describe, it, expect, vi } from 'vitest'; +import { render } from '@testing-library/react'; +import { isFilterAST, VALID_AST_OPERATORS } from '@objectstack/spec/data'; +import { + VIEW_FILTER_OPERATORS, + VIEW_FILTER_OPERATOR_ALIASES, + normalizeFilterOperator, +} from '@objectstack/spec/ui'; +import { UserFilters } from '../UserFilters'; + +/** + * Render a tabs-mode `UserFilters` whose single preset is the given spec + * `ViewFilterRule`, and return the AST node it emits. + * + * The preset is the DEFAULT tab, so `TabFilters`' mount effect emits it without + * a click — the same path a user takes when a view opens on its default tab, + * and the path that shipped the 400. + */ +function emitFor(rule: Record): unknown[] { + const onFilterChange = vi.fn(); + render( + , + ); + expect(onFilterChange).toHaveBeenCalledTimes(1); + return onFilterChange.mock.calls[0][0]; +} + +describe('UserFilters tab presets — the reported defect (#3470)', () => { + it('lowers a canonical `not_in` preset to a node isFilterAST ACCEPTS', () => { + // BEFORE this fix: emitted `['status', 'not in', ['done']]` — isFilterAST + // false, 400 INVALID_FILTER measured against a real backend. This is the + // assertion that flips red→green. + const node = emitFor({ field: 'status', operator: 'not_in', value: ['done'] }); + expect(node).toEqual([['status', 'not_in', ['done']]]); + expect( + isFilterAST(node), + `a not_in tab preset produced ${JSON.stringify(node)}, which isFilterAST() rejects — ` + + 'the server answers 400 INVALID_FILTER and the tab shows an empty list', + ).toBe(true); + }); + + it('folds the legacy `nin` spelling onto the canonical `not_in`', () => { + // Stored view metadata carries the shorthand: saveMeta persists the authored + // body verbatim, so the spec's own z.preprocess never reaches the row. + const node = emitFor({ field: 'status', operator: 'nin', value: ['done'] }); + expect(node).toEqual([['status', 'not_in', ['done']]]); + expect(isFilterAST(node)).toBe(true); + }); + + it('never emits the spaced `not in`, which no spec vocabulary defines', () => { + for (const spelling of ['not_in', 'nin', 'notin', 'notIn']) { + const [[, operator]] = emitFor({ field: 'status', operator: spelling, value: ['done'] }) as [ + [string, string, unknown], + ]; + expect(operator, `authored '${spelling}' lowered to '${operator}'`).not.toBe('not in'); + expect(operator).toBe('not_in'); + } + }); +}); + +describe('UserFilters tab presets — lowering is structural, not translated', () => { + it('reads both vocabularies from the spec', () => { + // Guards every it.each below against silently passing on an empty list. + expect(VIEW_FILTER_OPERATORS.length).toBe(19); + expect(VALID_AST_OPERATORS.size).toBeGreaterThan(0); + }); + + it('needs no operator table at all: every view operator is already AST-valid', () => { + // This is WHY the private table could be deleted rather than repaired. + expect(VIEW_FILTER_OPERATORS.filter((op) => !VALID_AST_OPERATORS.has(op))).toEqual([]); + }); + + it.each(VIEW_FILTER_OPERATORS)('a `%s` preset survives the isFilterAST gate', (op) => { + const value = op === 'in' || op === 'not_in' ? ['a', 'b'] : op === 'between' ? [1, 2] : 'x'; + const node = emitFor({ field: 'some_field', operator: op, value }); + expect( + isFilterAST(node), + `a '${op}' tab preset produced ${JSON.stringify(node)}, which isFilterAST() rejects`, + ).toBe(true); + }); + + it.each(Object.keys(VIEW_FILTER_OPERATOR_ALIASES))( + 'the legacy spelling `%s` lowers through the spec, not a local table', + (alias) => { + const [[, operator]] = emitFor({ field: 'f', operator: alias, value: ['a', 'b'] }) as [ + [string, string, unknown], + ]; + // The single-exit pin: whatever the spec says, verbatim. A second table + // reintroduced here fails this the moment it disagrees by one spelling. + expect(operator).toBe(normalizeFilterOperator(alias)); + expect(VALID_AST_OPERATORS.has(operator)).toBe(true); + }, + ); + + it('passes `before`/`after` through instead of rewriting them to `<`/`>`', () => { + // The one judgement call in #3470, settled by MEASUREMENT rather than + // assumption: on the live rc.2 backend the word and the symbol return + // identical status and identical record ids, on a `date` field and on a + // `datetime` field, in both directions. So the rewrite was a no-op and + // dropping it is a pure fix. (`canonicalAstOperator('before') === '<'` + // in the spec says the same thing independently.) + expect(emitFor({ field: 'due_date', operator: 'before', value: '2026-08-01' })) + .toEqual([['due_date', 'before', '2026-08-01']]); + expect(emitFor({ field: 'due_date', operator: 'after', value: '2026-08-01' })) + .toEqual([['due_date', 'after', '2026-08-01']]); + }); + + it('passes an unknown spelling through VERBATIM so the server rejects it loudly', () => { + // Direction note (honest reverse-verification): this one was GREEN BEFORE + // the change too — the deleted table's `default:` branch also passed + // unknowns through. It is here to pin the CONTRACT, not to flip: coercing + // `bfore` to `before` would answer a plausible-looking wrong record set + // instead of the 400 that tells the author their metadata is misspelled. + const node = emitFor({ field: 'due_date', operator: 'bfore', value: '2026-08-01' }); + expect(node).toEqual([['due_date', 'bfore', '2026-08-01']]); + expect(isFilterAST(node)).toBe(false); + }); + + it('leaves an already-lowered legacy `filters` triplet list untouched', () => { + // The other accepted tab shape (`{ id, label, filters: triplet[] }`) never + // went through the operator table and must not start now. + const onFilterChange = vi.fn(); + render( + , + ); + expect(onFilterChange).toHaveBeenCalledWith([['status', '=', 'active']]); + }); +}); diff --git a/packages/plugin-list/src/__tests__/UserFilters.test.tsx b/packages/plugin-list/src/__tests__/UserFilters.test.tsx index 6da68f60d5..c1d8a70bc0 100644 --- a/packages/plugin-list/src/__tests__/UserFilters.test.tsx +++ b/packages/plugin-list/src/__tests__/UserFilters.test.tsx @@ -114,8 +114,12 @@ describe('UserFilters — selection persistence (ADR-0047)', () => { />, ); - // Restored tab wins over the isDefault tab and emits its preset filter - expect(onFilterChange).toHaveBeenCalledWith([['priority', '=', 'urgent']]); + // Restored tab wins over the isDefault tab and emits its preset filter. + // The operator stays the spec's canonical `equals` — lowering a rule to an + // AST node is structural and translates nothing (#3470); `equals` is itself + // a `VALID_AST_OPERATORS` member, measured 200 on a live backend. Operator + // coverage proper lives in `UserFilters.tabPresetOperators.test.tsx`. + expect(onFilterChange).toHaveBeenCalledWith([['priority', 'equals', 'urgent']]); }); it('reports tab switches through onSelectionsChange', () => { From 1e99da4e05328c97fed18bc1ccd1cdbbd4684c86 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 6 Aug 2026 10:04:42 +0000 Subject: [PATCH 2/2] =?UTF-8?q?test(plugin-list):=20pin=20the=20one=20deli?= =?UTF-8?q?berate=20behaviour=20change=20=E2=80=94=20a=20rule=20with=20no?= =?UTF-8?q?=20operator=20(#3470)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The deleted table opened with `case undefined: … return '='`, so a tab-preset rule carrying `{field, value}` and NO operator lowered to `['status','=','x']`: accepted by isFilterAST and answered by the server as a real equality predicate. Through the spec's `normalizeFilterOperator` (which returns non-strings untouched) it now lowers to `[field, undefined, value]`, which isFilterAST refuses — the same loud 400 every other off-spec spelling gets. That is the ONLY sub-case in this change whose "before" was a passing 200 rather than a 400, so it is called out rather than folded into the pure-fix claim. It is nonetheless the correct direction: `ViewFilterRuleSchema.operator` is a bare `z.enum` with no default, so an operator-less rule fails `safeParse` with `invalid_value` and cannot be published — inventing `=` for it was a lenient consumer standing in for the contract (AGENTS.md #0.1). Both facts (the spec's refusal and the emitted node's) are asserted, so the claim cannot rot. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01GTRjn8xBqp75dk7kFupVRt --- packages/plugin-list/src/UserFilters.tsx | 11 ++++++++++ .../UserFilters.tabPresetOperators.test.tsx | 22 +++++++++++++++++++ 2 files changed, 33 insertions(+) diff --git a/packages/plugin-list/src/UserFilters.tsx b/packages/plugin-list/src/UserFilters.tsx index 4117b4a40c..19e220c043 100644 --- a/packages/plugin-list/src/UserFilters.tsx +++ b/packages/plugin-list/src/UserFilters.tsx @@ -143,6 +143,17 @@ export interface UserFiltersProps { * A misspelling must never be coerced into a valid operator (AGENTS.md #0.1): * silently reading `bfore` as `before` would return a plausible-looking wrong * record set instead of an error the author can see. + * + * That applies to a rule which OMITS the operator too, and it is the one place + * this is a deliberate behaviour change rather than a pure fix. The deleted + * table opened with `case undefined: … return '='`, inventing an equality + * predicate for a rule that has no operator at all. `ViewFilterRuleSchema` + * REQUIRES `operator` (it is a bare `z.enum`, no default — an operator-less rule + * fails `safeParse` with `invalid_value`), so such a rule is off-spec metadata + * that publish validation refuses; silently answering it with `field = value` + * was a lenient consumer standing in for the contract. It now lowers to + * `[field, undefined, value]`, which `isFilterAST()` refuses — the same loud + * `400` every other off-spec spelling gets. */ function normalizeTabPresets(tabs: any[]): Array<{ id: string; label: string; filters: any[]; default?: boolean }> { return (tabs || []) diff --git a/packages/plugin-list/src/__tests__/UserFilters.tabPresetOperators.test.tsx b/packages/plugin-list/src/__tests__/UserFilters.tabPresetOperators.test.tsx index f704e7334c..ad9bd1ac29 100644 --- a/packages/plugin-list/src/__tests__/UserFilters.tabPresetOperators.test.tsx +++ b/packages/plugin-list/src/__tests__/UserFilters.tabPresetOperators.test.tsx @@ -38,6 +38,7 @@ import { isFilterAST, VALID_AST_OPERATORS } from '@objectstack/spec/data'; import { VIEW_FILTER_OPERATORS, VIEW_FILTER_OPERATOR_ALIASES, + ViewFilterRuleSchema, normalizeFilterOperator, } from '@objectstack/spec/ui'; import { UserFilters } from '../UserFilters'; @@ -157,6 +158,27 @@ describe('UserFilters tab presets — lowering is structural, not translated', ( expect(isFilterAST(node)).toBe(false); }); + it('refuses to invent `=` for a rule that omits the operator', () => { + // The ONE deliberate behaviour change in #3470, and the only assertion here + // whose "before" was a passing 200 rather than a 400. The deleted table + // opened with `case undefined: … return '='`, so `{field, value}` with no + // operator lowered to `['status','=','x']` — accepted by isFilterAST, and + // answered by the server as a real equality predicate. + // + // `ViewFilterRuleSchema` REQUIRES `operator` (bare z.enum, no default), so + // that rule is off-spec metadata publish validation refuses. Inventing an + // answer for it was a lenient consumer standing in for the contract + // (AGENTS.md #0.1); it now earns the same loud 400 as any other off-spec + // spelling. + expect(ViewFilterRuleSchema.safeParse({ field: 'status', value: 'x' }).success).toBe(false); + + const node = emitFor({ field: 'status', value: 'x' }); + expect(isFilterAST(node)).toBe(false); + // What the old table would have produced, for contrast — accepted, and so + // silently answered as `status = 'x'`. + expect(isFilterAST([['status', '=', 'x']])).toBe(true); + }); + it('leaves an already-lowered legacy `filters` triplet list untouched', () => { // The other accepted tab shape (`{ id, label, filters: triplet[] }`) never // went through the operator table and must not start now.