fix(components): route ui:sidebar-trigger's spread through the form-control DOM declaration - #8660
Merged
Merged
Conversation
…ontrol DOM declaration `ui:sidebar-trigger` forwarded its whole prop bag to `SidebarTrigger`, which spreads its own rest onto the `<button>` it renders — so every authored SDUI key on the node became an attribute. Fourteen of them, one more than the shape this target derives from, because this registration also never destructured `schema`: every other registration in `renderers/navigation/sidebar.tsx` names it (they need `schema.body`), while this one renders no children and named only `className`, so the node `SchemaRenderer` injects on every render rode the same spread and landed as `schema="[object Object]"`. Two mechanisms, one filter — a whitelist does not have to enumerate what it drops. The declaration is the form-control one, not the bare `toDomProps`. The host is a `<button>`, where HTML defines `name`, and the ledger row is the evidence: it recorded thirteen attributes plus `schema` and never `name`, because the shared judge counts an authored `name` as legitimate here. A bare `toDomProps` would have un-named this control without moving one number in the gate that grades it. `style` is forwarded BY NAME (objectui#4435), as every converged sibling in this package does — it reaches the DOM today and the whitelist does not carry it. Measured through the real `SchemaRenderer` path, before and after: the leak set went 14 -> 0 underneath an UNCHANGED 8-attribute legitimate set (`aria-label`, `aria-describedby`, `class`, `data-obj-id`, `data-obj-type`, `data-sidebar`, `id`, `name`), the authored `className` still merges into the primitive's computed `h-7 w-7` rather than replacing it, and the trigger still toggles the sidebar. The ledger row is DELETED, as the two-way exact-set assertion requires, and the sweep gate gains the inverted case that refuses to let it back in. That takes the `packages/components` reading to 89 rows in FOUR shapes. The hand-kept counts in that docblock had drifted (they read 95 of 158 while the tree held 90 rows of 159 targets); they are re-derived here. Part of #5632 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YBWFb5YgMU5dw8p2VKj16S
…alect census's read
`scripts/__tests__/body-dialect-census.test.ts` slices this file from
`register('sidebar-trigger'` to the end and asserts the slice contains neither
`schema.body` nor `schema.children` — it is how the census pins that this one
`sidebar-*` registration reads no child list. The new comment quoted the first
of those literals while explaining why the OTHER registrations destructure
`schema`, which reddened the pin on a prose match.
Reworded to say the same thing without the literal, and the constraint itself is
now written down beside the code so the next editor does not rediscover it.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01YBWFb5YgMU5dw8p2VKj16S
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
|
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.
Part of #5632 — one slice, the
ui:sidebar-triggergroup. The parent burn-down stays OPEN; #5632 is not addressed in full here and remains open, exactly as PR #7564 left it.The defect, and the second one hiding inside it
ComponentRegistry.register('sidebar-trigger', …)destructuredclassNameand spread the rest ontoSidebarTrigger, which spreads its own rest onto theButtonit renders. So every canary family became an attribute on a real button element. Fourteen of them — one more than the shape this target derives from — and the extra one is the interesting half:schemawas never taken off the bag. Every other registration inrenderers/navigation/sidebar.tsxnames it (({ schema, ...props })) because it renders a child list. This one renders none and named onlyclassName, so the nodeSchemaRendererinjects on every render rode the same spread and landed asschema="[object Object]".That is a second mechanism, not a variant of the group's — which is why this target was ledgered on its own. One filter closes both, because a whitelist never has to enumerate what it drops; re-ledgering
schemawould have been the wrong repair, and the sweep gate now carries an inverted case that refuses it.Why the FORM-CONTROL declaration and not the bare
toDomPropsThe host is a button element, where HTML defines
nameanddisabled. The ledger row is the evidence: it recorded thirteen attributes plusschemaand nevername, because the shared judge counts an authorednameas legitimate on this element. A baretoDomPropswould therefore have un-named this control without moving a single number in the gate that grades the change — the exact failurepackages/components/src/lib/form-control-dom-props.tsexists to prevent, one host over.styleis forwarded BY NAME (#4435), as every converged sibling in this package does: it reaches the DOM today and the whitelist does not carry it. Dropping it is the plausible wrong fix, and it is pinned (leg E below).Measured, through the real SchemaRenderer path
Renders-real-markup established before any reading counted: the trigger renders a button element carrying the primitive's own
data-sidebar="trigger"hook, a lucidepanel-leftglyph and the sr-only "Toggle Sidebar" label — asserted as markup, so an error boundary or an early bail cannot satisfy it. Dump written to a file, with a lit control asserted first (a deliberately leaky div must report leaks), because vitest swallows console output from a passing test.class… h-7 w-7 os-sweep-readyThe 14:
ariadescribedby,arialabel,bind,colorvariant,datasource,events,props,reference_to,schema,zzcanary,zzcanarycamel,zzcanarynum,zzcanaryobj,zzcanaryprop— re-derived fromLEAK_LEDGER([...BARE_SPREAD_MINUS_NAME, 'schema'].sort()), not from the card's summary table, and confirmed by measurement.The 8 legitimate, unchanged in both runs:
aria-describedby,aria-label,class,data-obj-id,data-obj-type,data-sidebar,id,name. This is the half a leak gate reports in neither direction — PR #7564's lesson, asked again on this host rather than inherited from the SVG one. On a button element the answer isname, notstroke/fill.Lesson 1 checked too: this host does not have
ui:spinner's clobber shape. The renderer computes no class of its own;SidebarTriggermerges viacn("h-7 w-7", className), andclassNameis destructured so the filtered bag can never become a second writer for it. Both halves are asserted.Ledger
The row is DELETED, as the two-way exact-set assertion requires.
ui:gridstays out.BARE_SPREAD_MINUS_NAMEsurvives as the base for the two groups that still derive from it (action:menu,ui:form). The three meta-cases still hold, and the sweep gains a fourth inverted case (ui:sidebar-triggeris CLEAN — theschemarow may not be re-absorbed).95 of 158/97 of 181/61 cleanwhile the file's own arrays held 90 rows of 159 targets and 69 clean. Re-derived here by parsing those arrays; the class is filed as #8659 and is not addressed in this PR.Pins added —
examples/schema-catalog/test/sidebar-trigger-dom-leak-5632.test.tsxFive cases, every one observed RED on purpose:
name(the same bag reported differently on a button and on a div)data-statethe sidebar itself carriesstyleincludedEvery case asserts real markup first, and
document.cookieis cleared per case becauseSidebarProviderpersistssidebar_stateand reads it back on mount (#4234).Red legs — mutate, prove on disk, run, restore, prove restored
Each leg re-run against the final commit. Restore is
git checkout HEAD -- pathunder an EXIT/INT/TERM trap, proven by an emptygit diff HEAD; every leg's mutated blob hash differed from the HEAD blob, so no leg is void. Classified from vitest's JSON reporter.leaked 14 non-DOM)aria-*/data-*families droppeddisabled)styleno longer forwarded by namestylecase)Both caricature directions go red on the pin, which is the point: B, C, D and E are invisible to the sweep gate. A filter that strips every attribute, one that strips the ARIA and the designer
data-*, one that leaves an inert button, and one that swallowsstyleall read as a perfect zero there. That is the same blind spot #7564 documented on the SVG host, in this host's vocabulary.Verification
pnpm exec vitest run packages/components/ packages/app-shell/ examples/schema-catalog/ scripts/__tests__/body-dialect-census.test.ts— Test Files 934 passed (934), Tests 10761 passed | 1 skipped (10762).Test Files 3 passed (3), Tests 222 passed (222).pnpm --filter @object-ui/components --filter @object-ui/app-shell --filter @object-ui/example-schema-catalog run type-check— all threeDone, on a tree whose dependency closure was built first (an unbuilt tree would have been a precondition, not a result).pnpm exec eslinton the three changed source files — 0 errors, 11 pre-existingno-explicit-anywarnings on the index signatures already in the file.node scripts/check-changeset-presence.mjs—2 source file(s) of 2 released package(s) changed, and this change declares 1 changeset(s): .changeset/5632-sidebar-trigger-dom-passthrough.md.node scripts/check-changeset-no-major.mjs—No changeset declares a major bump.check:control-bytes,check:unreferenced-sources,check:phantom-deps,type-check:coverage,lint:coverage— all exit 0.check:sdui-registration-pinsexits 2 with a stated PRECONDITION ("No console build to weigh"), which is NOT MEASURED locally rather than a failure — it needsapps/consolebuilt, and this diff adds, removes and renames no registration (11sidebar-*registrations before and after). Declared to CI.One repo pin this nearly broke, recorded because it is not obvious
scripts/__tests__/body-dialect-census.test.tsslices this file fromregister('sidebar-trigger'to EOF and asserts the slice contains neitherschema.bodynorschema.children— that is how it pins this as the onesidebar-*registration reading no child list. The first draft of the new comment quotedschema.bodywhile explaining why the other registrations destructureschema, and reddened the pin on a prose match. Reworded, and the constraint is now written beside the code. It is also why this fix does not add aschemaparameter: the filter drops the key without one.🤖 Generated with Claude Code
https://claude.ai/code/session_01YBWFb5YgMU5dw8p2VKj16S
Generated by Claude Code