From 44b668f359b245d97800b703981ebd4360b6b885 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 7 Sep 2026 22:46:18 +0000 Subject: [PATCH 1/2] fix(types): declare `id` on the filter-builder condition so it stops being stripped MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `FilterBuilderConditionSchema` declared `{ field, operator, value? }` and is a plain `z.object`, which strips undeclared keys. An author who correctly wrote a condition `id` had it silently discarded: the document validated, the row rendered, and from then on it could never be edited or removed, because every affordance on the row matches on the identity that was no longer there. Measured on `0203a29e` through `safeParse`: a condition carrying `id` returned `success: true` with output keys `field` / `operator` / `value` — no `id`. That is accepted-and-discarded, the class objectui#6150 closed for `tree-view.title`. Re-derived from `packages/components/src/custom/filter-builder.tsx`: the condition `id` has sixteen read sites — the four MATCH sites (`removeCondition`, `updateCondition`, `changeOperator`, `changeField`), the React `key`, and eleven call sites feeding those four. The component's own `FilterBuilderCondition` declares `id: string`, `addCondition` emits `crypto.randomUUID()`, and the published doc declares it required too. The mirror was the only face omitting it. Declared REQUIRED on both published faces. The group's `id` is untouched and stays OPTIONAL (objectui#7560, zero read sites) — the two look alike and take opposite answers, and the new pin holds them apart. Refs #8415 Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01QtGhnU3WnnWyiWeYQhw2aX --- .../8415-filter-builder-condition-id.md | 70 +++++++ .../filter-builder-condition-id-8415.test.ts | 182 ++++++++++++++++++ .../zod-lazy-getter-identity-7918.test.ts | 16 +- packages/types/src/complex.ts | 16 ++ packages/types/src/zod/complex.zod.ts | 24 +++ 5 files changed, 304 insertions(+), 4 deletions(-) create mode 100644 .changeset/8415-filter-builder-condition-id.md create mode 100644 packages/types/src/__tests__/filter-builder-condition-id-8415.test.ts diff --git a/.changeset/8415-filter-builder-condition-id.md b/.changeset/8415-filter-builder-condition-id.md new file mode 100644 index 0000000000..f25eb047c8 --- /dev/null +++ b/.changeset/8415-filter-builder-condition-id.md @@ -0,0 +1,70 @@ +--- +'@object-ui/types': minor +--- + +**Breaking for authored metadata:** a `filter-builder` CONDITION must now declare +`id` (objectui#8415). It is declared on both published faces — the TypeScript +interface `FilterBuilderCondition` in `complex.ts` and the Zod mirror +`FilterBuilderConditionSchema` in `zod/complex.zod.ts` — so a key the renderer +has always required is finally validated instead of silently discarded. + +**What was measured, on this branch's base (`0203a29e`).** The mirror declared a +condition as `{ field, operator, value? }` and it is a plain `z.object`, which +STRIPS undeclared keys. So an author who correctly wrote `id` had it removed: +`FilterBuilderConditionSchema.safeParse({ id: 'c1', field, operator, value })` +returned `success: true` with an output whose keys were `field`, `operator`, +`value` — no `id`. The document then validated and the row rendered, and from +that point on it could never be edited or removed, because every affordance on +the row matches on the identity that was no longer there. + +Re-derived from `packages/components/src/custom/filter-builder.tsx` rather than +inherited: `id` has **sixteen** condition-side read sites — the four MATCH sites +that decide which row a mutation lands on (`removeCondition`'s +`c.id !== conditionId`, `updateCondition`'s and `changeOperator`'s +`c.id === conditionId`, `changeField`'s `c.id !== conditionId`), the React `key` +on the row, and eleven call sites that hand `condition.id` to one of those four. +Handed `undefined`, `removeCondition` deletes every OTHER row, `updateCondition` +and `changeField` match none, and React keys the row `undefined`. The +component's own exported `FilterBuilderCondition` has always declared +`id: string`, and `addCondition` emits `crypto.randomUUID()`. + +**Who is affected — a condition authored without `id`:** + +```json +{ "type": "filter-builder", "name": "f", + "fields": [{ "value": "a", "label": "A", "type": "text" }], + "value": { "id": "root", "logic": "and", + "conditions": [{ "field": "a", "operator": "equals", "value": "x" }] } } +``` + +now fails validation at `value.conditions.0.id`. **Migration:** give each +condition a stable unique string — any value the rest of the document does not +reuse; the component generates `crypto.randomUUID()` for rows the user adds. + +⭐ **The narrowing refuses only what was ALREADY broken.** A condition with no +`id` renders today but can never be edited or removed, so nothing that works +stops working; the state this refuses is *accepted-and-discarded*, the class +objectui#6150 closed for `tree-view.title`. Measured across `apps/**`, +`examples/**`, `content/**` and `packages/**`: **every** authored +`filter-builder` condition in this repository already carries `id` — 7 of 7, +across the three schema-catalog entries that author rows — so no shipped +document in this repo changes verdict on this key. + +**What else now refuses, and it is the second thing declaring a key buys:** +`id: 42`. An UNDECLARED key gets no type check at all, so a wrong-typed identity +parsed clean at base and was then dropped. It is now refused at its own path. + +**Who is NOT affected.** ⛔ The GROUP's `id` is untouched and stays OPTIONAL +(objectui#7560): `isValidGroup` never consults it, nothing reads +`filterGroup.id`, and deleting it from an authored group renders +byte-identically — the opposite reading, on a member that looks identical. +`{ logic: 'and', conditions: [] }` still validates. The renderer is unchanged; +`FilterOperatorSchema`, `FilterFieldSchema` and `FilterBuilderSchema`'s own keys +are byte-identical, so the three items parked on objectui#7562 and the operator +vocabulary of objectui#7561 are neither addressed nor moved here. + +Graded `minor`, not `patch`: this narrows the accepted input set, which is +breaking for any author who wrote a condition without an identity. It is not +`major` per this repo's fixed-group convention (objectui's own breaking changes +ship as `minor`; the group's major tracks `@objectstack` — AGENTS.md 版本号策略, +mechanically enforced by `scripts/check-changeset-no-major.mjs`). diff --git a/packages/types/src/__tests__/filter-builder-condition-id-8415.test.ts b/packages/types/src/__tests__/filter-builder-condition-id-8415.test.ts new file mode 100644 index 0000000000..d832a3c079 --- /dev/null +++ b/packages/types/src/__tests__/filter-builder-condition-id-8415.test.ts @@ -0,0 +1,182 @@ +/** + * 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. + * + * objectui#8415 — `FilterBuilderConditionSchema` declares `id`. + * + * ## What was wrong + * + * The mirror declared a condition as `{ field, operator, value? }`. The + * component's identity for a row is `id`, and because the mirror is a plain + * `z.object`, an author who wrote `id` correctly had it STRIPPED in silence. + * The document then validated and the row rendered — and no affordance could + * ever reach it again, because all four mutation helpers match on `c.id` and + * every one of them was handed `undefined`. + * + * Measured on the base commit, through `FilterBuilderConditionSchema.safeParse`: + * a condition carrying `id` parsed successfully and the parsed OUTPUT did not + * carry the key. That is the `accepted-and-discarded` class objectui#6150 + * closed for `tree-view.title`, not a refusal. + * + * ## Why REQUIRED, and why that is not the answer the GROUP got + * + * ⛔ The two `id`s look alike and take OPPOSITE answers — do not unify them. + * + * - `FilterGroupSchema.id` is declared OPTIONAL (objectui#7560): `isValidGroup` + * never consults it, nothing reads `filterGroup.id`, and deleting it from an + * authored group renders byte-identically. Requiring it would invent a + * refusal the renderer does not make. + * - A CONDITION's `id` is the opposite reading. `assertion the four match + * sites and the React key are live` below re-derives it from the component + * source rather than quoting a count. + * + * ⭐ The narrowing therefore refuses only what is ALREADY broken: a condition + * with no `id` renders today but can never be edited or removed. Nothing that + * works stops working. + */ +import { describe, it, expect } from 'vitest'; +import { readFileSync } from 'node:fs'; +import { fileURLToPath } from 'node:url'; +import { dirname, join } from 'node:path'; + +import { FilterBuilderConditionSchema, FilterGroupSchema } from '../zod/complex.zod'; +import { safeValidateSchema } from '../zod/index.zod'; +import type { FilterBuilderCondition as TsCondition, FilterGroup as TsGroup } from '../complex'; + +const HERE = dirname(fileURLToPath(import.meta.url)); +const REPO_ROOT = join(HERE, '..', '..', '..', '..'); +const READER = 'packages/components/src/custom/filter-builder.tsx'; +const readerSource = readFileSync(join(REPO_ROOT, READER), 'utf8'); + +const FIELDS = [{ value: 'a', label: 'A', type: 'text' }]; +const WELL_FORMED = { id: 'c1', field: 'a', operator: 'equals', value: 'x' }; +const NO_ID = { field: 'a', operator: 'equals', value: 'x' }; +const doc = (value: unknown) => ({ type: 'filter-builder', name: 'f', fields: FIELDS, value }); +const group = (conditions: unknown[]) => ({ id: 'root', logic: 'and', conditions }); + +/* ── Type-level pins: the TS twin moved WITH the mirror ───────────────────── */ + +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 */ } + +expectType>(); +// `id` is REQUIRED, not merely typed `string`: an object that OMITS the key must +// be rejected, and only this annotation proves it. The `@ts-expect-error` IS the +// assertion — delete the `id` declaration and this line stops erroring, which +// fails the compile. +// @ts-expect-error `FilterBuilderCondition.id` is required (objectui#8415) +const conditionWithoutId: TsCondition = { field: 'a', operator: 'equals' }; +// …and the GROUP's stayed optional, so the two faces cannot be "unified" by a +// future editor without one of these two lines going red. +const groupWithoutId: TsGroup = { logic: 'and', conditions: [] }; + +describe('objectui#8415 — the condition `id` is DECLARED, so it is no longer stripped', () => { + it('the repair itself: `id` survives the parse output', () => { + // The base measurement this moves away from: `success` was already `true`, + // and the key was GONE from `.data`. Asserting only `success` would have + // been green before and after. + const parsed = FilterBuilderConditionSchema.safeParse(WELL_FORMED); + expect(parsed.success).toBe(true); + expect(parsed.success && Object.keys(parsed.data as object).sort()) + .toEqual(['field', 'id', 'operator', 'value']); + expect(parsed.success && (parsed.data as { id?: unknown }).id).toBe('c1'); + }); + + it('REFUSES a condition with no `id`, on the direct arm and through a group', () => { + expect(FilterBuilderConditionSchema.safeParse(NO_ID).success).toBe(false); + expect(FilterGroupSchema.safeParse(group([NO_ID])).success).toBe(false); + expect(FilterGroupSchema.safeParse(group([{ id: 'g2', logic: 'or', conditions: [NO_ID] }])).success) + .toBe(false); + }); + + it('REFUSES it through the authored document too — both entry paths on `FilterBuilderSchema`', () => { + // `value` and `defaultValue` are each `union([condition, group])`, so a + // condition reaches the mirror two ways and the union must not launder it + // through the group arm. + expect(safeValidateSchema(doc(group([NO_ID]))).success).toBe(false); + expect(safeValidateSchema(doc(NO_ID)).success).toBe(false); + expect(safeValidateSchema({ type: 'filter-builder', name: 'f', fields: FIELDS, defaultValue: group([NO_ID]) }).success) + .toBe(false); + }); + + it('type-checks `id` now that it is declared — `id: 42` was ACCEPTED before', () => { + // The second thing declaring a key buys, and the one an author never sees: + // a `z.object` gives an UNDECLARED key no check at all, so `id: 42` parsed + // clean at base and was then dropped. + expect(FilterBuilderConditionSchema.safeParse({ ...WELL_FORMED, id: 42 }).success).toBe(false); + }); +}); + +describe('objectui#8415 — the negative controls that must NOT have moved', () => { + it('still ACCEPTS a well-formed condition (anti-vacuity)', () => { + expect(FilterBuilderConditionSchema.safeParse(WELL_FORMED).success).toBe(true); + expect(FilterGroupSchema.safeParse(group([WELL_FORMED])).success).toBe(true); + expect(safeValidateSchema(doc(group([WELL_FORMED]))).success).toBe(true); + expect(safeValidateSchema(doc(group([]))).success).toBe(true); + }); + + it('still REFUSES a bad operator — and the fixture carries `id` so THIS is what refuses it', () => { + // Without the `id`, this assertion would stay green with + // `FilterOperatorSchema` deleted outright: the row would be refused for the + // missing key instead. Carrying `id` isolates the operator. + expect(FilterBuilderConditionSchema.safeParse({ ...WELL_FORMED, operator: 'not_a_real_operator' }).success) + .toBe(false); + expect(FilterBuilderConditionSchema.safeParse({ id: 'c1', operator: 'equals', value: 'x' }).success) + .toBe(false); + }); + + it('the GROUP `id` is UNTOUCHED — still optional, still type-checked (objectui#7560)', () => { + // ⛔ The trap this card was split out to avoid. The group's `id` has zero + // read sites; requiring it would refuse a document that renders perfectly. + expect(FilterGroupSchema.safeParse({ logic: 'and', conditions: [WELL_FORMED] }).success).toBe(true); + expect(FilterGroupSchema.safeParse({ id: 42, logic: 'and', conditions: [] }).success).toBe(false); + expect(FilterGroupSchema.safeParse({ operator: 'and', conditions: [] }).success).toBe(false); + expect(groupWithoutId.conditions).toEqual([]); + expect(conditionWithoutId.field).toBe('a'); + }); +}); + +describe('objectui#8415 — the enforcement the declaration now matches, re-derived from the reader', () => { + it('the four MATCH sites are live in the component', () => { + // Re-derived from source rather than quoted as a count: if a refactor moves + // a row's identity off `id`, this reddens and the REQUIRED declaration has + // to be re-argued rather than silently outliving its reason. + const matchSites = [ + // removeCondition + 'conditions: filterGroup.conditions.filter((c) => c.id !== conditionId)', + // updateCondition + 'c.id === conditionId ? { ...c, ...updates } : c', + // changeOperator + 'c.id === conditionId', + // changeField + 'if (c.id !== conditionId) return c', + ]; + for (const site of matchSites) expect(readerSource).toContain(site); + }); + + it('the React `key` on the row is the condition `id`', () => { + expect(readerSource).toContain('key={condition.id}'); + }); + + it('a new row is BORN with an `id` — the producer agrees with the declaration', () => { + expect(readerSource).toContain('id: crypto.randomUUID(),'); + }); + + it('the component declares `id` non-optional on its own condition type', () => { + // The renderer half of `declared = enforced`. `id?: string` here would mean + // the component tolerates its absence, and the required mirror would be + // narrower than the thing it mirrors. + expect(readerSource).toContain('export interface FilterBuilderCondition {\n id: string\n'); + }); + + it('anti-vacuity for the source probes: a spelling that is NOT there reads false', () => { + // The instrument above is `String.prototype.includes`; a probe that matches + // nothing would make every assertion in this block unfalsifiable. + expect(readerSource).not.toContain('c.zzzNotAKey === conditionId'); + expect(readerSource.length).toBeGreaterThan(1000); + }); +}); diff --git a/packages/types/src/__tests__/zod-lazy-getter-identity-7918.test.ts b/packages/types/src/__tests__/zod-lazy-getter-identity-7918.test.ts index e2a9e3b396..ef24f1f66a 100644 --- a/packages/types/src/__tests__/zod-lazy-getter-identity-7918.test.ts +++ b/packages/types/src/__tests__/zod-lazy-getter-identity-7918.test.ts @@ -257,20 +257,28 @@ describe('objectui#7918 · z.lazy getter identity', () => { }); it('FilterBuilderConditionSchema still accepts a condition and refuses a bad operator', () => { + // ⚠️ Both fixtures carry `id` since objectui#8415 declared it REQUIRED on + // the condition. The NEGATIVE one carries it for a reason that is not + // cosmetic: without it the row would be refused for the MISSING KEY, and + // this assertion — whose subject is `FilterOperatorSchema` — would stay + // green with the operator vocabulary deleted outright. Carrying `id` + // isolates the operator as the only thing left to refuse it. expect(FilterBuilderConditionSchema.safeParse( - { field: 'amount', operator: 'greater_than', value: 100 }, + { id: 'c1', field: 'amount', operator: 'greater_than', value: 100 }, ).success).toBe(true); expect(FilterBuilderConditionSchema.safeParse( - { field: 'amount', operator: 'not_a_real_operator' }, + { id: 'c1', field: 'amount', operator: 'not_a_real_operator' }, ).success).toBe(false); }); it('FilterGroupSchema still nests conditions and sub-groups through the memoised arm', () => { + // The CONDITION rows carry `id` (required since objectui#8415); the + // sub-GROUP's `id` stays what it always was — declared but optional. const group = { id: 'g1', logic: 'and', conditions: [ - { field: 'amount', operator: 'greater_than', value: 100 }, - { id: 'g2', logic: 'or', conditions: [{ field: 'stage', operator: 'equals', value: 'won' }] }, + { id: 'c1', field: 'amount', operator: 'greater_than', value: 100 }, + { id: 'g2', logic: 'or', conditions: [{ id: 'c2', field: 'stage', operator: 'equals', value: 'won' }] }, ], }; expect(FilterGroupSchema.safeParse(group).success).toBe(true); diff --git a/packages/types/src/complex.ts b/packages/types/src/complex.ts index 60cb9fac9e..6dbabf2171 100644 --- a/packages/types/src/complex.ts +++ b/packages/types/src/complex.ts @@ -586,6 +586,22 @@ export type FilterBuilderOperator = * Filter condition */ export interface FilterBuilderCondition { + /** + * Row identity. + * + * REQUIRED, and deliberately asymmetric with `FilterGroup.id` below + * (objectui#8415). The group's `id` has zero read sites and is optional for + * that reason; a CONDITION's `id` is the identity every affordance on the row + * matches on — `removeCondition`, `updateCondition`, `changeOperator` and + * `changeField` in `packages/components/src/custom/filter-builder.tsx`, plus + * the React `key`. The component's own `FilterBuilderCondition` declares it + * `string`, and `addCondition` emits `crypto.randomUUID()`. + * + * Undeclared, it was STRIPPED by the `z.object` mirror in silence, so a + * correctly authored row validated and rendered and could then never be + * edited or removed. Declaring it is what makes `declared = enforced`. + */ + id: string; /** * Field to filter */ diff --git a/packages/types/src/zod/complex.zod.ts b/packages/types/src/zod/complex.zod.ts index ee9cf17f5d..0e69e4d70c 100644 --- a/packages/types/src/zod/complex.zod.ts +++ b/packages/types/src/zod/complex.zod.ts @@ -281,6 +281,30 @@ export const FilterOperatorSchema = z.enum([ * — read that before "fixing" any of them to match this one. */ const FilterBuilderConditionObject = z.object({ + // REQUIRED, and the asymmetry with `FilterGroupSchema.id` below is the whole + // point of declaring it here (objectui#8415). The group's `id` has ZERO read + // sites and is optional for that reason; a CONDITION's `id` is the identity + // every affordance on the row matches on, measured in + // `packages/components/src/custom/filter-builder.tsx`: + // + // - the four MATCH sites — `removeCondition` (`c.id !== conditionId`), + // `updateCondition` and `changeOperator` (`c.id === conditionId`) and + // `changeField` (`c.id !== conditionId`); + // - the React `key` on the row; + // - eleven call sites that hand `condition.id` to one of those four. + // + // The component's own exported `FilterBuilderCondition` declares it `string`, + // not `string | undefined`, and `addCondition` emits `crypto.randomUUID()`. + // + // ⭐ The narrowing refuses only what is ALREADY broken. Because a plain + // `z.object` STRIPS undeclared keys, an author who correctly wrote `id` had + // it discarded in silence: the document validated, the row rendered, and then + // no affordance could reach it — `removeCondition(undefined)` matches every + // OTHER row, `updateCondition(undefined)` matches none, and React keys the + // row `undefined`. Nothing that worked stops working; the state this refuses + // is accepted-and-discarded, the class objectui#6150 closed for + // `tree-view.title`. + id: z.string().describe('Row identity — matched by `removeCondition` / `updateCondition` / `changeOperator` / `changeField`, and the React key'), field: z.string().describe('Field name'), operator: FilterOperatorSchema.describe('Filter operator'), value: z.any().optional().describe('Filter value'), From c7150ca7c06d5bed11933460f94d129dd2cfff43 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 7 Sep 2026 23:29:35 +0000 Subject: [PATCH 2/2] docs(types): correct the inverted `id`-stripped mechanism on all four surfaces MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The changeset, the zod mirror comment, the TS interface TSDoc and the pin test header all said a stripped `id` makes a row unreachable — `removeCondition` "deletes every OTHER row", `updateCondition` and `changeField` "match none". That is inverted, and two of those surfaces ship (the changeset reaches CHANGELOG.md, which is in the package `files[]`; the zod comment survives into dist/zod/complex.zod.js; the TSDoc into dist/complex.d.ts). Measured on the four helper bodies in packages/components/src/custom/filter-builder.tsx, simulated verbatim over three id-less rows plus one crypto.randomUUID() row: both sides of every comparison are `undefined`, `undefined === undefined` is true, so each helper matches EVERY id-less row. removeCondition removes 3 of 4 (the clicked row included, the uuid row spared); updateCondition, changeOperator and changeField each move 3 of 4. The defect is loss of INDIVIDUAL identity — every affordance acts on all id-less rows en bloc — which is more severe than the text claimed, not less. Two additions folded in, both measured: - the compile-time face, stated explicitly: an object literal typed FilterBuilderCondition, FilterGroup['conditions'][number], or a condition inside a FilterGroup / FilterBuilderSchema['value'] / defaultValue literal now fails tsc with "Property 'id' is missing … but required in type 'FilterBuilderCondition'". Verified on all four spellings, with the group's own optional `id` as a negative control. - the invalid_union path precision: `value.conditions.0.id` is the LOGICAL location. safeValidateSchema reports one root invalid_union at path [] across 13 arms, with the id leaf three nested unions down inside arm 8. Parsed against FilterBuilderConditionSchema directly it is reported flat at ["id"]. One correction to the reviewer's dictated wording, because it would have shipped a second wrong mechanism: `key={condition.id}` on an id-less row is NOT a duplicate `undefined` key. React reads `key={undefined}` as no key at all — measured on React 19.2.8, element.key is null, the list reconciles by index and React logs the missing-key warning. Comment-only: no executable text moved. Proven per file by lexing with the TypeScript parser and hashing the leaf-token stream (JSDoc subtrees excluded) plus the comment-erased source lines; both hashes are byte-identical across the edit for all three .ts files. See the PR body for the readings. Refs #8415 Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01QtGhnU3WnnWyiWeYQhw2aX --- .../8415-filter-builder-condition-id.md | 78 +++++++++++++++---- .../filter-builder-condition-id-8415.test.ts | 17 +++- packages/types/src/complex.ts | 8 +- packages/types/src/zod/complex.zod.ts | 23 ++++-- 4 files changed, 99 insertions(+), 27 deletions(-) diff --git a/.changeset/8415-filter-builder-condition-id.md b/.changeset/8415-filter-builder-condition-id.md index f25eb047c8..be983b0ec9 100644 --- a/.changeset/8415-filter-builder-condition-id.md +++ b/.changeset/8415-filter-builder-condition-id.md @@ -14,8 +14,9 @@ STRIPS undeclared keys. So an author who correctly wrote `id` had it removed: `FilterBuilderConditionSchema.safeParse({ id: 'c1', field, operator, value })` returned `success: true` with an output whose keys were `field`, `operator`, `value` — no `id`. The document then validated and the row rendered, and from -that point on it could never be edited or removed, because every affordance on -the row matches on the identity that was no longer there. +that point on the row had no individual identity: every affordance on it is +handed `undefined`, so it acts on every OTHER id-less row along with it. The +measurement is below. Re-derived from `packages/components/src/custom/filter-builder.tsx` rather than inherited: `id` has **sixteen** condition-side read sites — the four MATCH sites @@ -23,8 +24,28 @@ that decide which row a mutation lands on (`removeCondition`'s `c.id !== conditionId`, `updateCondition`'s and `changeOperator`'s `c.id === conditionId`, `changeField`'s `c.id !== conditionId`), the React `key` on the row, and eleven call sites that hand `condition.id` to one of those four. -Handed `undefined`, `removeCondition` deletes every OTHER row, `updateCondition` -and `changeField` match none, and React keys the row `undefined`. The + +**What that does when `id` is stripped**, simulated on the four helper bodies +transcribed verbatim, over three id-less rows and one `crypto.randomUUID()` row +(the realistic mix: the mirror strips every AUTHORED row's `id`, while rows the +user adds in-session are born with one). Both sides of every comparison are +`undefined`, and `undefined === undefined` is TRUE, so each helper matches EVERY +id-less row rather than none: + +- `removeCondition(undefined)` keeps only rows where `c.id !== undefined`, so it + deletes all three id-less rows in a single click — the clicked one included — + and leaves the uuid row standing. Measured: 3 of 4 rows removed. +- `updateCondition(undefined, …)`, `changeOperator(undefined, …)` and + `changeField(undefined, …)` each apply the edit to all three at once. + Measured: 3 of 4 rows moved, each time. +- `key={condition.id}` becomes `key={undefined}`, which React reads as NO key at + all rather than as a duplicate one. Measured on React 19.2.8: `element.key` is + `null` for every such row, the list falls back to index reconciliation, and + React logs `Each child in a list should have a unique "key" prop.` + +So the defect is **loss of individual identity** — every affordance acts on all +the id-less rows en bloc, and a row cannot be edited or removed on its own. It +is not "matches none", and it is the more severe of the two readings. The component's own exported `FilterBuilderCondition` has always declared `id: string`, and `addCondition` emits `crypto.randomUUID()`. @@ -37,22 +58,49 @@ component's own exported `FilterBuilderCondition` has always declared "conditions": [{ "field": "a", "operator": "equals", "value": "x" }] } } ``` -now fails validation at `value.conditions.0.id`. **Migration:** give each -condition a stable unique string — any value the rest of the document does not -reuse; the component generates `crypto.randomUUID()` for rows the user adds. +now fails validation. **Where it is REPORTED is not where it logically is.** +`value.conditions.0.id` is the LOGICAL location — the concatenation of the paths +down the arm tree. The issue `safeValidateSchema` actually reports is a single +root `invalid_union` at `path: []`, across **13** arms; the `id` failure sits +three nested unions further down, inside arm 8: `invalid_union` at `["value"]` +→ arm 1 → `invalid_union` at `["conditions", 0]` → arm 0 → `invalid_type` at +`["id"]`, *"Invalid input: expected string, received undefined"*. Parsed against +`FilterBuilderConditionSchema` directly, the same refusal is reported flat, at +`path: ["id"]`. Both are stated because a consumer that reads `issue.path` off +the document-level result will not find `id` there. + +**The compile-time face, which lands before any document is parsed.** `id` is +declared on the TypeScript interface as well, so a TypeScript author is refused +by `tsc`, not only by the validator. An object literal typed +`FilterBuilderCondition`, or `FilterGroup['conditions'][number]`, or a condition +written inside a `FilterGroup` / `FilterBuilderSchema['value']` / `defaultValue` +literal, now fails type-check with *"Property 'id' is missing in type … but +required in type 'FilterBuilderCondition'"* — measured on all four spellings. +The GROUP's own `id` stays optional, so `{ logic: 'and', conditions: [] }` still +compiles. Stated explicitly for the reason objectui#7774 stated its compile-time +face (`groupField?: never`, "refused at compile time"): it is the half a +TypeScript author meets first, and a runtime-only description hides it. + +**Migration:** give each condition a stable unique string — any value the rest of +the document does not reuse; the component generates `crypto.randomUUID()` for +rows the user adds. ⭐ **The narrowing refuses only what was ALREADY broken.** A condition with no -`id` renders today but can never be edited or removed, so nothing that works -stops working; the state this refuses is *accepted-and-discarded*, the class -objectui#6150 closed for `tree-view.title`. Measured across `apps/**`, -`examples/**`, `content/**` and `packages/**`: **every** authored -`filter-builder` condition in this repository already carries `id` — 7 of 7, -across the three schema-catalog entries that author rows — so no shipped -document in this repo changes verdict on this key. +`id` renders today but cannot be edited or removed INDIVIDUALLY — as measured +above, one click removes every id-less row at once and one edit fans out across +all of them — so nothing that works stops working; the state this refuses is +*accepted-and-discarded*, the class objectui#6150 closed for `tree-view.title`. +Measured across `apps/**`, `examples/**`, `content/**` and `packages/**`: +**every** authored `filter-builder` condition in this repository already carries +`id` — 7 of 7, across the three schema-catalog entries that author rows — so no +shipped document in this repo changes verdict on this key. **What else now refuses, and it is the second thing declaring a key buys:** `id: 42`. An UNDECLARED key gets no type check at all, so a wrong-typed identity -parsed clean at base and was then dropped. It is now refused at its own path. +parsed clean at base and was then dropped. It is now refused — reported flat at +`path: ["id"]` on the condition schema (*"Invalid input: expected string, +received number"*), and through the document at the same place in the arm tree +as the missing key above. **Who is NOT affected.** ⛔ The GROUP's `id` is untouched and stays OPTIONAL (objectui#7560): `isValidGroup` never consults it, nothing reads diff --git a/packages/types/src/__tests__/filter-builder-condition-id-8415.test.ts b/packages/types/src/__tests__/filter-builder-condition-id-8415.test.ts index d832a3c079..3d1c1ffa59 100644 --- a/packages/types/src/__tests__/filter-builder-condition-id-8415.test.ts +++ b/packages/types/src/__tests__/filter-builder-condition-id-8415.test.ts @@ -12,9 +12,17 @@ * The mirror declared a condition as `{ field, operator, value? }`. The * component's identity for a row is `id`, and because the mirror is a plain * `z.object`, an author who wrote `id` correctly had it STRIPPED in silence. - * The document then validated and the row rendered — and no affordance could - * ever reach it again, because all four mutation helpers match on `c.id` and - * every one of them was handed `undefined`. + * The document then validated and the row rendered — and from then on the row + * had no individual identity. All four mutation helpers match on `c.id`, every + * one of them is handed `undefined`, and `undefined === undefined` is TRUE, so + * each matches EVERY id-less row: `removeCondition` deletes them all in one + * click (the clicked row included; only uuid-bearing rows survive), and + * `updateCondition` / `changeOperator` / `changeField` fan one edit out across + * all of them. `key={condition.id}` becomes `key={undefined}`, which React + * reads as no key at all rather than as a duplicate one. + * + * ⛔ Not "matches none" — the failure is EN BLOC, and it is the more severe of + * the two readings. * * Measured on the base commit, through `FilterBuilderConditionSchema.safeParse`: * a condition carrying `id` parsed successfully and the parsed OUTPUT did not @@ -34,7 +42,8 @@ * source rather than quoting a count. * * ⭐ The narrowing therefore refuses only what is ALREADY broken: a condition - * with no `id` renders today but can never be edited or removed. Nothing that + * with no `id` renders today but cannot be edited or removed INDIVIDUALLY — + * every affordance on it acts on all the id-less rows at once. Nothing that * works stops working. */ import { describe, it, expect } from 'vitest'; diff --git a/packages/types/src/complex.ts b/packages/types/src/complex.ts index 6dbabf2171..9b7cc8c591 100644 --- a/packages/types/src/complex.ts +++ b/packages/types/src/complex.ts @@ -598,8 +598,12 @@ export interface FilterBuilderCondition { * `string`, and `addCondition` emits `crypto.randomUUID()`. * * Undeclared, it was STRIPPED by the `z.object` mirror in silence, so a - * correctly authored row validated and rendered and could then never be - * edited or removed. Declaring it is what makes `declared = enforced`. + * correctly authored row validated and rendered with no individual identity: + * every affordance is handed `undefined`, `c.id === conditionId` is TRUE for + * every id-less row, and so one click removes all of them at once — the + * clicked row included — while one edit fans out across all of them. A row + * cannot be edited or removed on its own. (Not "matches none"; the failure is + * en bloc.) Declaring it is what makes `declared = enforced`. */ id: string; /** diff --git a/packages/types/src/zod/complex.zod.ts b/packages/types/src/zod/complex.zod.ts index 0e69e4d70c..aed0cb61dd 100644 --- a/packages/types/src/zod/complex.zod.ts +++ b/packages/types/src/zod/complex.zod.ts @@ -298,12 +298,23 @@ const FilterBuilderConditionObject = z.object({ // // ⭐ The narrowing refuses only what is ALREADY broken. Because a plain // `z.object` STRIPS undeclared keys, an author who correctly wrote `id` had - // it discarded in silence: the document validated, the row rendered, and then - // no affordance could reach it — `removeCondition(undefined)` matches every - // OTHER row, `updateCondition(undefined)` matches none, and React keys the - // row `undefined`. Nothing that worked stops working; the state this refuses - // is accepted-and-discarded, the class objectui#6150 closed for - // `tree-view.title`. + // it discarded in silence: the document validated, the row rendered, and the + // row then had no individual identity. Both sides of every comparison listed + // above are `undefined`, and `undefined === undefined` is TRUE, so each + // helper matches EVERY id-less row rather than none: + // + // - `removeCondition(undefined)` deletes them ALL in one click — the + // clicked row included; only rows carrying a real id survive it; + // - `updateCondition`, `changeOperator` and `changeField` fan a single + // edit out across all of them; + // - `key={condition.id}` becomes `key={undefined}`, which React reads as + // NO key at all rather than as a duplicate one, so the rows reconcile by + // index and React warns about the missing key. + // + // ⛔ Not "matches none": the failure is EN BLOC, and it is the more severe + // reading — a row cannot be edited or removed on its own. Nothing that worked + // stops working; the state this refuses is accepted-and-discarded, the class + // objectui#6150 closed for `tree-view.title`. id: z.string().describe('Row identity — matched by `removeCondition` / `updateCondition` / `changeOperator` / `changeField`, and the React key'), field: z.string().describe('Field name'), operator: FilterOperatorSchema.describe('Filter operator'),