fix(types): declare id on the filter-builder condition so it stops being stripped - #8432
Conversation
…being stripped
`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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01QtGhnU3WnnWyiWeYQhw2aX
✅ Console Performance Budget
The eager closure is every chunk the entry reaches through static imports — what the browser fetches and parses before the app renders. The entry chunk on its own is a small fraction of it. 📦 Bundle Size Report
Size Limits
|
契约复核裁决 — FAIL,逐字采纳
降档保险丝已过 —— 档位是读数,不是自述。 复核子代理在裁决块里自称 ⛔
PM 侧的处置裁决判 FAIL 的是已发布文本,不是契约本身 —— 复核明确写了「The contract increment itself passes every item below and does not need to be reopened」。所以补丁轮是措辞轮,⛔ 不重开验收面。 这一条值得单独点名,因为它推翻的是我自己在 ACCEPT 评论里赞同过的话:三处已发布文本把机制说反了。 补丁轮已派发。⛔ Generated by Claude Code |
… surfaces
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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01QtGhnU3WnnWyiWeYQhw2aX
✅ Console Performance Budget
The eager closure is every chunk the entry reaches through static imports — what the browser fetches and parses before the app renders. The entry chunk on its own is a small fraction of it. 📦 Bundle Size Report
Size Limits
|
Refs #8415
Clause-②: yes— this narrows a published accept set.needs:contract-reviewis hung on this PR and on the card. ⛔ Draft, unqueued, no auto-merge, no self-approval: parking green is the sanctioned resting state.The defect
FilterBuilderConditionSchemadeclared a condition as{ field, operator, value? }and its body is a plainz.object, which strips undeclared keys. So an author who correctly wroteidhad it discarded in silence — the document validated, the row rendered, and from that point the row had no individual identity — every affordance on it is handedundefined, andc.id === conditionIdis true for every id-less row, so each affordance acts on all of them at once. Measured under Patch round below.Measured on this branch's base
0203a29e, throughFilterBuilderConditionSchema.safeParse:successat base{ id: 'c1', field: 'a', operator: 'equals', value: 'x' }truefield,operator,value— noidThat is accepted-and-discarded, not refused — the class #6150 closed for
tree-view.title.Patch round — the mechanism statement was INVERTED, and it shipped
A
CONTRACT_REVIEW_TIERreviewer returned FAIL on this PR's own explanation of what breaks whenidis stripped. The earlier wording said the row "could never be edited or removed", thatremoveCondition"deletes every OTHER row" and thatupdateCondition/changeField"match none". That is the opposite of what the code does, and two of the four surfaces carrying it are published text: the changeset reachesCHANGELOG.md, which is in the package'sfiles[]; the zod comment survives intodist/zod/complex.zod.js; the TSDoc intodist/complex.d.ts.Re-read independently from
packages/components/src/custom/filter-builder.tsx— symbolsremoveCondition,updateCondition,changeOperator,changeField, and the rowkey— then simulated on the four helper bodies transcribed verbatim, over three id-less rows plus onecrypto.randomUUID()row. That mix is the realistic one: the mirror strips every authored row'sid, while rows the user adds in-session are born with one.Both sides of every comparison are
undefined, andundefined === undefinedis true, so each helper matches every id-less row rather than none:removeCondition(undefined)conditions.filter((c) => c.id !== conditionId)updateCondition(undefined, …)c.id === conditionId ? { ...c, ...updates } : cchangeOperator(undefined, …)c.id === conditionId ? { ...c, operator, value } : cchangeField(undefined, …)if (c.id !== conditionId) return c⇒ 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 more severe than the old text claimed, not less.
One correction to the reviewer's dictated wording, because taking it verbatim would have shipped a second wrong mechanism. The instruction said React "keys them all
undefined" / that the stripped rows "collide on a duplicateundefinedReact key". They do not collide:key={condition.id}on an id-less row iskey={undefined}, which React reads as no key at all. Measured on React 19.2.8 throughreact/jsx-runtime,element.keyisnullfor every such row, the list falls back to index reconciliation, and React logsEach child in a list should have a unique "key" prop.The published text now says that, not the duplicate-key version. Everything else in the reviewer's instruction reproduced exactly.The two additions the reviewer asked for, both measured
The
invalid_unionpath precision.value.conditions.0.idis the logical location — the concatenation of the paths down the arm tree. The issuesafeValidateSchemaactually reports is a single rootinvalid_unionatpath: []across 13 arms, with theidleaf three nested unions further down, inside arm 8:Parsed against
FilterBuilderConditionSchemadirectly, the same refusal is reported flat atpath: ["id"]. Both are now stated rather than one deleted, because a consumer that readsissue.pathoff the document-level result will not findidthere.The compile-time consequence, stated explicitly — it was only implicit before, and #7774 stated its compile-time face (
groupField?: never, "refused at compile time"). Measured withtsc --strictagainst the builtdist/complex.d.ts: four positive spellings and two negative controls, exit 0 with every@ts-expect-errorconsumed.FilterBuilderConditionwithoutidFilterGroup['conditions'][number]withoutidFilterGroupliteralFilterBuilderSchema['value']literal{ logic: 'and', conditions: [] }, the groupidstill optionalThe message is
Property 'id' is missing in type '{ field: string; operator: "equals"; value: string; }' but required in type 'FilterBuilderCondition'. Falsifiability checked: deleting one@ts-expect-errorturns the run red (exit 2) with exactly that message.Comment-only — proved, not asserted
No executable text moved. Method: each
.tsfile is lexed with the TypeScript parser (ts.createSourceFile), and two independent readings are taken.node.getText()starts atgetStart(), i.e. after leading trivia, so comments and whitespace never enter it. Joined and sha256'd. JSDoc subtrees are excluded:getChildren()folds a/** … */block into the tree as real nodes, so without that exclusion a JSDoc-only edit moves the hash while the token count holds. Measured — and it is why the first version of this prover was wrong.getLeadingCommentRanges/getTrailingCommentRanges), then lines still holding non-whitespace counted and sha256'd.Not a regex, and not the raw
ts.createScanner: the scanner is context-free and cannot tell a backtick inside a single-quoted string from a template start. Measured oncomplex.zod.ts, whosedescribe(...)strings carry backticks, it desynced after 3 comment tokens and emitted a single 2396-character "template" token.Readings across this round's own commit,
44b668f3(before) toc7150ca7(after):packages/types/src/complex.ts3aba1367…unchanged35b464c7…unchangedpackages/types/src/zod/complex.zod.ts033ec6d4…unchanged9c24c106…unchangedpackages/types/src/__tests__/filter-builder-condition-id-8415.test.tsf0787a35…unchangedf4adf622…unchangedThe only reading that moved is
complex.zod.ts's comment-range count, 89 → 100 — one long//block split into a bulleted list, which is exactly what a comment-only edit is allowed to move.Prover falsifiability, four scratch mutations of these same files: a JSDoc-text-only edit and a line-comment-only edit each leave both hashes at the post-edit values, while renaming one identifier (
field→fieldX) and changing one string literal ('Field name'→'Field NAME') each move both hashes.And the comments really are published — checked in the built
dist/at this head:dist/zod/complex.zod.jscarries// helper matches EVERY id-less row rather than none:, anddist/complex.d.tscarriesen bloc.) Declaring it is what makes ....44b668f3still carries the old wording. Amending a pushed commit is forbidden here, and this repo squashes on merge using the PR title and body — so the text that lands onmainis this body, not that message.Gates for this round — head
997280e4Every exit code captured before any pipe (redirect to a log, capture
$?, then read the log).origin/mainhad moved (ca394272→786bc91e), so it was merged in — never rebased, nothing force-pushed or amended. The merge left all four surfaces byte-identical (git diff c7150ca7 HEADover them is empty), and main had not touched them.pnpm --filter @object-ui/types type-checktsc -p tsconfig.test.jsontsc -p tsconfig.test.json --listFiles@ts-expect-errorpins are really compiledpnpm --filter @object-ui/types builddist completeness: 1 package(s) complete (124 emitted files verified)node scripts/check-changeset-presence.mjsnode scripts/check-changeset-no-major.mjsnode scripts/check-changeset-fixed.mjsnode scripts/check-changeset-overwrite.mjspnpm check:control-bytespnpm check:comment-mask-corpuspnpm exec vitest run packages/types/src/__tests__/filter-builder-condition-id-8415.test.tspnpm exec vitest run packages/types/Test Files 143 passed (143)·Tests 2724 passed (2724)⛔ No contract change in this round. The
idmember's shape on both faces, the union, the accept set, the barrel, and every test assertion and fixture are byte-identical — which is precisely what the comment-only table proves. This round is wording only.Symbols, re-derived on
origin/main(no line number copied from #8415 or #7562)packages/types/src/zod/complex.zod.ts—FilterBuilderConditionObject, the object the exportedz.lazyreturns. That is the edit target, not thez.lazywrapper.packages/types/src/complex.ts—FilterBuilderCondition, the TypeScript twin. It omittedidtoo and moved with the mirror.⛔ The
z.lazyshape is untouched. The docblock directly above the edit point records that this one lazy can be memoised because its body is not recursive, and that seven otherz.lazyexports on the same face cannot take that shape. Nothing here reshapes any lazy;zod-lazy-getter-identity-7918.test.tsstays green.The condition
idvs the groupid— the trap this card was split out to avoid.idinpackages/components/src/custom/filter-builder.tsxhits 17 (control: a bogus key hits 0). Disambiguated site by site rather than reported as a total:id— MATCH (decides which row a mutation lands on)removeConditionc.id !== conditionId;updateConditionc.id === conditionId;changeOperatorc.id === conditionId;changeFieldif (c.id !== conditionId) return cid— Reactkeyon the rowkey={condition.id}id— call sites feeding one of the fourupdateCondition(condition.id, …)/changeField/changeOperatorcalls, plusremoveCondition(condition.id)idr?.[idField] ?? r?.id ?? r?._idin the lookup option loader (a fetched record)⇒ 16 condition-side sites, 0 group-side. The card's "four plus the React key" is right about the match sites and the key; the other eleven are call sites feeding them. The two
ids take opposite answers and are not unified here:FilterGroupSchema.idstays optional, and a new pin holds them apart.Three other faces already declared it required — the mirror was the only one that did not:
FilterBuilderConditiondeclaresid: string;addConditionemitsid: crypto.randomUUID();content/docs/components/complex/filter-builder.mdxpublishesid: string; // Condition identifier, with no?.Accept-set table, from source, both entry paths
FilterBuilderSchema.valueand.defaultValueare each a union of condition and group, so a condition reaches the mirror two ways; conditions also nest inside a group'sconditions.ididid: 42(wrong type)group[cond without id]subgroup[cond without id]value = group[cond without id]value = bare cond without iddefaultValue = group[cond without id]idsurvives the parse outputNegative controls — refused before, still refused, so the narrowing did not swallow them: condition with no
field; bad operator (with and withoutid); group spelledoperator; groupid: 42.Positive controls — accepted before, still accepted: condition with
id; empty group; group without its ownid; nested subgroup withid; a document with novalueat all; catalogempty-filter-builderanduser-filters.Unchanged and not this PR's business: catalog
product-searchandwith-conditionsrefuse at base and still refuse, on the operator aliaseq/gt/lt— that is #7561, deliberately not folded in. #7562's items 1–3 are untouched:FilterOperatorSchema,FilterFieldSchemaandFilterBuilderSchema's own keys are byte-identical.Corpus census, with a firing control
Instrument: a structural walker over
apps/**,examples/**,content/**,packages/**(5027 files) —JSON.parsefor JSON, fenced JSON in Markdown/MDX, and the TypeScript parser for object literals in TS/TSX. It reports every object carryingfieldandoperatorinside aconditionsarray.Authored metadata — the population the re-grade trigger asks about: 7 of 7 conditions carry
id, 0 do not. All inexamples/schema-catalog/src/schemas/components-complex-filter-builder/—product-search3/3,search-interface2/2,with-conditions2/2. (empty-filter-builderanduser-filtersauthor zero rows.) No document incontent/**orapps/**authors a condition; the published doc renders the catalog entry rather than inlining one.Firing control — a known-positive planted in both directions and then removed: totals moved
59 | 39 with / 20 without→61 | 40 / 21, and the walker named the planted file and both of its rows (control_positivewith anid,control_negativeabsent). Removal restored the exact baseline and leftgit statusclean. So the zero above is a reading, not a blind walk.Radius, stated rather than implied: this repository's working tree only. Authorship outside it — downstream applications built on
@object-ui/types— is not measurable from here and is not covered by that zero.Parity: measured, not asserted
The instrument is⚠️
pnpm --filter @object-ui/types type-check, whose third legtsc -p tsconfig.test.jsoncompiles the tests the package tsconfig excludes.vitest run zod-mirror-parity.test.tsis a false green here — the ledger is a type map and vitest does not typecheck.Proved live first, before measuring anything: mirroring
ObjectViewSchema.listViews(the key the ledger pins as unmirrored) reddenedtscwithwhile the same mutated tree ran
vitest31/31 green — the false green reproduced. Restore proven by blob hash back to HEAD.Result on this change: no ledger row and no header figure moves.
type-checkis green. Mechanistically consistent with the ledger's own entry:complex.zod.ts#FilterBuilderConditionSchemasits in the not-compared half ("declaredz.ZodTypeoverany, which exposes no.shapeto read"), andFilterBuilderConditionis not one of its paired TypeScript imports. ⇒zod-mirror-parity.test.ts, held by parked PR #8354, is not touched.Fixture triage
zod-lazy-getter-identity-7918.test.tshad two condition fixtures withoutid. Both now carry one, and the negative fixture carries one for a reason that is not cosmetic: without it the row would be refused for the missing key, and an assertion whose subject isFilterOperatorSchemawould have stayed green with the operator vocabulary deleted outright. Carryingidisolates the operator.No other fixture in the mirror's consumer radius needed repair — verified by running the full suites of every package that validates documents through
safeValidateSchema.Reverse verification
Leg 1 — ablate the runtime declaration. Blob
0e69e4d7→9a6473bbon disk; anchored counts1 → 0for the conditionidline and1 → 1for the group's optional one, so exactly the member this card added was removed. New pin: 4 failed / 8 passed — the four failures are precisely the narrowing assertions, and the eight that stay green are the negative controls and the source-derived read-site probes, which must not move. Whole types suite under ablation: exactly one file fails, mine (1 failed / 141 passed). Restored: blob equal to HEAD, count back to 1,git diff HEADempty.Leg 2 — ablate the TypeScript declaration (
id: stringtoid?: string). Anchored counts6 → 5required and3 → 4optional.tsc -p tsconfig.test.jsonexit 1 with both compile-time pins firing:Restored: blob equal to HEAD, count back to 6,
git diff HEADempty. Both ablation scripts carry anEXIT INT TERMtrap restoring fromHEADby absolute path.Green control re-run after restore: 142 files / 2707 tests pass,
type-checkgreen,buildgreen.Gates — first round, head
44b668f3Every exit code captured before any pipe. These are the first round's readings, taken before
origin/mainwas merged in; this round's gates, at head997280e4, are in Patch round above.pnpm exec vitest run --maxWorkers=2 packages/types/Test Files 142 passed (142)·Tests 2707 passed (2707)pnpm --filter @object-ui/types type-checkpnpm --filter @object-ui/types builddist completeness: 1 package(s) complete (124 emitted files verified)pnpm --filter @object-ui/types lintnode scripts/check-changeset-presence.mjsnode scripts/check-changeset-no-major.mjsNo changeset declares a major bumpfilter-builder-condition-id-8415.test.ts(new)zod-lazy-getter-identity-7918.test.tsexamples/schema-catalog,packages/cli,packages/coreTest Files 167 passed·Tests 4910 passednode scripts/check-governed-queue-guard.mjs --teston all 5 pathsNOT GOVERNEDRepo-wide
eslint .is CI's run, not this PR's. One correction worth recording: an earlier free-handeslint . --no-inline-configreported 5 errors inpackages/types; all five were in files this PR does not touch and were an artifact of--no-inline-configstripping theeslint-disabledirectives those rule-demonstration pins rely on. objectui's own script iseslint .without that flag, and it exits 0.Grading input for the PM
The card is
priority:p3with a written-down re-grade trigger: p2 if authored conditions commonly carryid, so the strip is hitting real documents rather than a theoretical author. Measured: 7 of 7 — 100% of authored conditions in this repository carryid, and every one of them was having it discarded. My judgement is that the trigger fires and this is p2. The number is above; the call is the PM's.Verification notes
content/docs/releases/, and no branch was force-pushed, rebased or amended after publication.FilterGroupSchemaand its optionalidare byte-identical to base.🤖 Generated with Claude Code
Generated by Claude Code
Generated by Claude Code