fix(fields): FieldEditWidget forwards the DOM pass-through block it declares - #7009
Merged
Merged
Conversation
…eclares Its props are `FieldWidgetComponentProps` — the controlled-input keys intersected with `FieldWidgetDomProps`, `AriaAttributes` and the open `data-` family — so a host could always pass `id`, `name`, `autoFocus`, `tabIndex`, `onBlur`, `onFocus`, `onClick`, any `aria-*` and any `data-*` with no type error. The body destructured five keys and rendered the widget with those, so `autoFocus` was the only survivor of the whole block. The factory now hands the widget `toDomProps(props)` — this package's own executor of that declaration — rather than a second key list. The reuse is the guard: `toDomProps.ts`'s direction-2 assertion makes `keyof FieldWidgetDomProps extends DomPassThroughKey` a compile error to violate, so a key added to the declared block reaches the widget through this factory automatically. Semantic props stay explicit and are applied after the spread. Not a widening: the keys were already declared, and every widget still re-filters through its own `toDomProps` before anything reaches the DOM. Also corrects the `data-table.tsx` comment this falsifies — the injected-editor listener's other justification (the `renderCellEditor` context has nowhere to put an `onBlur`) is untouched and still load-bearing. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013hfmP9hoMd3dJwTh85J4yB
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-sam
marked this pull request as ready for review
August 31, 2026 09:26
This was referenced Aug 31, 2026
This was referenced Sep 1, 2026
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 #6909
FieldEditWidgetdeclared the whole DOM pass-through block and delivered one key of it. It now delivers the block.Direction 1 from the card, and only direction 1. Direction 2 (narrowing this component's published prop type to the five keys it honoured) is deliberately not taken here: it would break any caller passing those keys today, and it is a contract decision for the maintainer.
The defect
The component's props are
FieldWidgetComponentProps— the controlled-input keys intersected withFieldWidgetDomProps,AriaAttributesand the opendata-family. A host could always passid,name,autoFocus,tabIndex,onBlur,onFocus,onClick, anyaria-and anydata-attribute with no type error. The body then destructuredfield, value, onChange, readonly, autoFocusand rendered the widget with those five pluscompact.autoFocuswas the only survivor of the whole block.That is this package's own first-class defect class, named in
packages/fields/src/widgets/toDomProps.ts: a key that type-checks, reads as supported, and silently never reaches the element (#3290'saria-required, #3222's validation slot).toDomPropsbinds the WIDGET contract to its whitelist with compile-time assertions in both directions. Nothing bound this factory to either, so the factory was the one unbound link in the chain.Not a widening, and not a contract change
The keys were already declared, and every widget still re-filters through its own
toDomPropsbefore anything reaches a DOM element. What any widget accepts or rejects is unchanged — this only makes the implementation deliver what the declaration already promised.The fix, and why it is the reuse rather than a new list
The factory hands the widget
toDomProps(props)— this package's own executor of the declaration — instead of a second key list written out in the factory. That reuse is the structural guard:toDomProps.ts's direction-2 assertion already makeskeyof FieldWidgetDomProps extends DomPassThroughKeya compile error to violate, so a key added to the declared DOM block now reaches the widget through this factory automatically. One mechanism, one judge. A private list here would have been free to drift, which is exactly how the factory came to deliver one key out of seven.The forwarded set is a deliberate superset of
FieldWidgetDomProps: it also carriesclassNameanddisabled, declared on the controlled-input block and forwarded by the same executor for the reason stated there (withholding them makes it a silent styling- and interactivity-dropper). Semantic props —field,value,onChange,readonly, andcompactfor the relational pickers — stay explicit and are applied after the spread, so a host cannot displace them.The triage measurement the card asked for
Per host, read at the call site on this branch's base:
FieldEditWidgetObjectGrid.renderCellEditorpackages/plugin-grid/src/ObjectGrid.tsx:3690field,value,onChangeInlineFieldInputpackages/plugin-detail/src/InlineFieldInput.tsx:434field,value,onChange,autoFocusautoFocusis the one key that already survivedRequiredFieldsDialogpackages/plugin-kanban/src/RequiredFieldsDialog.tsx:125field,value,onChange,readonlyThose are the only three JSX call sites in the repo outside this package's own tests (
grep -rn "FieldEditWidget" --include=*.tsx --include=*.tsoverpackagesandapps).So: plain repair, not a live regression — no host is silently losing a key today. But one host had already WORKED AROUND the drop.
RequiredFieldsDialogwraps every control in alabeland says why: "FieldEditWidgetrenders the widget itself and takes noidto associate with, and widening its contract belongs to@object-ui/fields, not to a caller." The premise was false at the type level all along —idis declared — and true only at the implementation level, which is this defect. It takes anidnow.Card measurement re-derived, not inherited:
EDIT_WIDGETShas 31 keys resolving to 27 distinct components; 26 calltoDomPropsthemselves andUserFielddelegates its whole props object toLookupFieldvia{...(props as any)}; none stripsonBlur— the five that own a blur handler compose the host's (#6780, #6802). That matches the card's 27 / 26 / 1 exactly.Evidence the fix is load-bearing
Pin:
packages/fields/src/__tests__/FieldEditWidget.domPassThrough-6909.test.tsx, written before the fix.RED first (pin only, before touching the factory) —
Test Files 1 failed (1) · Tests 3 failed (3). The boundary assertion printed the defect directly:GREEN after — same command,
Test Files 1 passed (1) · Tests 3 passed (3).Ablation — revert only the forwarding (
toDomProps(props)spread replaced byautoFocus={props.autoFocus}, i.e. the exact pre-fix shape) and the pin goes red again with the identical 5-key set. No build ordistleg is involved and that is measured, not assumed:vitest.config.mtsaliases@object-ui/fieldstopackages/fields/src, and the pin imports../FieldEditWidgetrelatively, so the test executes the source file the script mutates (both asserted in the script's preflight).Mutation proved on disk by anchored counts in BOTH directions plus hashes, never by an exit code:
The restore is
git checkout HEAD -- <absolute path>(never bare — a bare restore reads the index, which a mutation can pollute), undertrap ... EXIT INT TERM, with the repo root taken fromgit rev-parse --show-toplevel.Control — an undeclared authored key (
zzcanary) must still be dropped, or "everything forwards now" would be indistinguishable from having reopened the bare spreadtoDomPropsexists to prevent. It is asserted at two points, because the DOM alone cannot see a reopened spread (each widget re-filters and would quietly rescue the mistake):zzcanaryabsent, and no other extra key;data-probe present in the same render so the absence is a measurement and not an empty tree.Verification
Union run after the final commit, at
0071a0825(git status --porcelainempty):pnpm exec vitest run packages/fields/—Test Files 126 passed (126) · Tests 2105 passed (2105)pnpm exec vitest run packages/plugin-grid/ packages/plugin-detail/ packages/plugin-kanban/(the three hosts) —Test Files 235 passed (235) · Tests 2105 passed (2105)pnpm --filter @object-ui/fields --filter @object-ui/components type-check— bothDone, exit 0. The new test file is a real program input, not merely present: before a typing fix, this same command failed withsrc/__tests__/FieldEditWidget.domPassThrough-6909.test.tsx(168,24): error TS2769, which is what provestsconfig.test.jsoncompiles it.pnpm exec eslint . --format json(plain form) — 4035 files, 0 errors, exit 0. Repo-wide, so no narrowing to justify.check-changeset-presence"3 source file(s) of 2 released package(s) changed, and this change declares 1 changeset(s)";check-changeset-no-major,check-changeset-fixed,check-changeset-overwrite,check-control-bytes(5815 files),check-side-effects-array,check-entry-guard,check-type-check-coverage(45/46),check-lint-coverage(46/46),check-package-self-import,check-node-esm-load --specifiers-only,check-vi-mock-specifiers,check-vi-mock-inherit— all exit 0.check-eager-closure-budgetis NOT MEASURED locally, not green and not red: it needsapps/console/dist/eager-closure.jsonfrom a console build this branch never ran, and it says so itself ("This is a broken gauge, not a passing budget"). Nothing in this diff touchesapps/consoleor the eager-closure surface. CI builds the console first.Dependency closure built before any type-check (
pnpm --filter '@object-ui/fields^...' build), so no unbuilt-tree false red.One bounded in-place correction, declared
packages/components/src/renderers/complex/data-table.tsx— comment only, no code. Its injected-editor comment justified the document-levelpointerdownlistener partly with "FieldEditWidgetforwardsautoFocusand nothing else out of the DOM block, so a host handler could not reach the control through it even if one were passed." This PR makes that sentence false, so it is corrected in the same change rather than left to rot — that comment was itself landed to replace an earlier stale one (#6912, for #6859), and this repo treats a confidently wrong comment as a first-class defect.The listener stays load-bearing, on the untouched half of the same reason: the
renderCellEditorcontext object still has nowhere to put anonBlur. Widening it is aDataTableSchemacontract change and is not proposed here. No test pinned the corrected sentence (grepped), andpackages/componentstype-check and the repo-wide eslint are green above.Not in this PR
FieldEditWidgetalso does not forward the DECLARED non-DOM host-plumbing keys —error,dataSource,dependentValues,dependsOn,dependsOnLabels,emptyHint,onUploadingChange,onSelectRecord,onCreateNew. Same declared-but-not-delivered shape, but each carries real design content (errordrivesaria-invalid; who producesdependsOnLabelsfor an inline host is a question, not a mechanical answer), so it is not a mechanical extension of this repair. That half is filed as #7008 and remains open — out of scope here.Generated by Claude Code
Generated by Claude Code