fix(components): stop previousValues leaking onto the <form> DOM node - #6413
Merged
os-support-ai merged 1 commit intoAug 25, 2026
Merged
Conversation
`FormSchema.previousValues` is a declared schema key with a real consumer —
the form renderer's `previousRecord` memo, which binds `previous` for
field-rule CEL predicates and is the INSERT/UPDATE signal the read-only
submit strip gates on (objectui#3484). That consumer reads it off `schema`
and is untouched here.
The leak was on the other channel. `SchemaRenderer` spreads every
non-metadata top-level schema key as a React prop *in addition* to handing
the node over as `schema`, so an edit-mode host (`ObjectForm`, which is what
the `object-master-detail-form` header composes) delivered a second,
top-level copy in `...props`. The renderer already consume-and-drops that
whole family before its DOM spread — `objectName`, `onDirtyChange`,
`defaultValues`, `fields`, `layout`, … — and `previousValues` was the one
member missing from the list, so it rode `...formProps` onto `<form>`.
Two things followed on every edit-mode header render: React declined the
prop ("does not recognize the `previousValues` prop on a DOM element"), and
— measured on React 19, not recorded on the card — the persisted record was
still stamped onto the element as `previousvalues="[object Object]"`.
The new test renders through `SchemaRenderer` (the host path) rather than
off the registry directly, which is precisely why every existing
`previousValues` test missed this: rendering `<Form schema={…} />` never
populates `...props`. It asserts on captured `console.error` output and
carries two controls — a positive control proving the capture still catches
a real unknown-prop warning, and a degenerate control proving the pin
matches this one named prop rather than "the render printed nothing".
Scope is the runtime leak only. The declared key is unchanged.
Fixes #6396
Contributor
✅ 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
|
os-support-ai
marked this pull request as ready for review
August 25, 2026 22:15
os-support-ai
deleted the
claude/issue-6396-previousvalues-spread-hygiene
branch
August 25, 2026 22:27
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #6396
Verified tree:
643094dee— every reading below was taken on that exact tree (working tree clean at push time).Step 1 — the fork: does
previousValueshave a real consumer on the master-detail header path?Yes. This is step 2 (spread hygiene), not step 3 (enforce-or-remove). The measurement:
packages/components/src/renderers/form/form.tsx:979destructurespreviousValuesoffschema, and:1140-1147buildspreviousRecordfrom it. That memo is load-bearing twice: it bindspreviousfor field-rule CEL predicates, andisCreateForm = previousRecord === undefinedis the INSERT/UPDATE signal the read-only submit strip gates on (objectui#3484).ObjectForm.tsx:1057computespreviousValuesformode === 'edit' && recordId, and puts it on the form node at both:1270and:1423.MasterDetailForm.tsx:976renders thatObjectFormas its header.form-readonly-when-previous,form-readonly-submit-strip,form-required-when-server-owned).So the declared key is not dead, nothing here goes to the enforce-or-remove channel, and the authorable surface is untouched:
packages/types/src/form.tsandpackages/types/src/zod/form.zod.tsare not in this diff. Clause-② no.The defect
The consumer above reads
schema.previousValues. The leak is on the other channel.SchemaRendererspreads every non-metadata top-level schema key as a React prop in addition to handing the node over asschema(packages/react/src/SchemaRenderer.tsx:1035/:1105). So an edit-mode host delivers a second, top-level copy ofpreviousValuesin the renderer's...props.The renderer already consume-and-drops that entire family before its DOM spread —
objectName,onDirtyChange,defaultValues,fields,layout,columns, … atform.tsx:2531-2582, with a comment naming exactly this mechanism ("Some callers / the SDUI dispatch spread the whole form node at the top level … so these arrive inpropsand would leak onto the DOM").previousValueswas the one member of the family missing from that list, so it rode...formPropsonto<form>.The one-line fix adds it to the existing list. The
schemaconsumer never readsprops, so it cannot be affected.Premise re-derived on the merged tree
The card measured this on
origin/main @ 631d81dbf;mainhas moved many times since. Re-derived on062943f86(the merge-base this branch was cut from) — still present, and worse than the card recorded. React 19 does not merely decline the prop, it also stamps it on the element:So the persisted record was reaching the DOM as
previousvalues="[object Object]"on every edit-mode header render. (The attribute stringifies to[object Object], so no field values were exposed — but the attribute should not be there at all.)Ghost-assertion guard — both readings
The instrument asserts on captured
console.error, because the symptom is a React console warning, not a thrown error or a DOM difference. Each call's arguments are joined before matching, since React passes the prop name as a separate%sargument rather than inside the format string.Against unmodified
origin/main(source untouched, test file only) — FAILING:With the fix — PASSING:
The two controls that keep the pin honest
The 2 tests that passed in both runs are deliberate, and they are the reason a green pin means something:
POSITIVE CONTROL: the capture catches a real unknown-prop warningrenders a bare<div madeUpProp={…}>and asserts the capture does return React's notice. Without it, the pin would keep passing if the capture broke or React stopped warning — a green vacuous assertion, which is the exact failure class this card is about.DEGENERATE CONTROL: tolerates unrelated console output on a clean renderrenders the same node withoutpreviousValues, emits an unrelatedconsole.error, and asserts the pin still passes. The matcher requires/does not recognize/and the prop name, so it pins one named prop rather than "the render printed nothing"; the next unrelated warning cannot break it.The new test renders through
SchemaRendererrather than pulling the component off the registry directly. That is the whole reason a declared key could leak to the DOM on every edit-mode header render without a single existing test noticing:<Form schema={…} />never populates...props.Verification
All heavy runs serialized through the container's shared verify lock; exit codes captured before any pipe, and each verdict quoted from the tool's own output.
mainTests 2 failed | 2 passed (4)— ghost-assertion guardTests 4 passed (4)vitest run packages/components/src/renderers/form/Test Files 55 passed (55)/Tests 366 passed (366)vitest run packages/plugin-form/(master-detail host path)Test Files 71 passed (71)/Tests 726 passed (726)pnpm --filter @object-ui/components run type-checkTYPECHECK_EXIT=0eslint .— whole repo, one passfiles linted: 3788,errorCount total: 0check:control-bytesOK (scanned 5302 tracked text file(s))check:phantom-deps/check:self-import/check:esm-specifiers/check:vi-mock-specifiersEXIT=0check-changeset-presence1 source file(s) of 1 released package(s) changed … declares 1 changeset(s)check-changeset-no-majorNo changeset declares a major bumpThe lint reading is a full repo-wide run, not a narrowed one — no scope-reduction argument is being relied on.
Type-check coverage was measured rather than assumed:
tsc -p packages/components/tsconfig.test.json --listFilesreports the new test file andrenderers/form/form.tsxeach present in the program (1 hit each), so "type-check clean" actually covers this diff.Dependency closure (
pnpm --filter '@object-ui/components^...' build) was built before type-checking, so the.d.tsfiles read are current rather than stale.Scope
One line of production change plus its test and changeset (
@object-ui/components,patch).Deliberately not touched:
packages/types/src/form.ts,packages/types/src/zod/form.zod.ts) — the key has a live consumer, so there is nothing to retire.previousValuesonly as a row in the zod-mirror drift census. This PR is the runtime DOM leak, a separate concern.Generated by Claude Code