Repository navigation
fix(react,components): make toRenderableSchema behaviour-preserving for the falsy primitives (objectui#8908) - #9029
Conversation
…or the falsy primitives (objectui#8908)
`toRenderableSchema`'s docblock promised "the value a caller forwards renders
identically whether or not it passes through here". That was false for exactly
two of the six `SchemaNode` members: the bridge mapped every `number` /
`boolean` onto `String(node)`, and `String(0)` / `String(false)` are the
NON-EMPTY strings '0' and 'false', which are truthy — so they sailed past
`SchemaRenderer`'s own `!evaluatedSchema` leg and rendered as their own text,
while the raw values render nothing (pinned since objectui#4548).
The shipped, reachable instance: `empty`'s `action` slot gates on nullish
(objectui#7105, so a bare string node renders), so
`{ type: 'empty', title: 'T', action: 0 }` painted a stray "T0".
The mapping now has two legs and mirrors the renderer's own narrowing order: a
truthy primitive becomes its text form, a falsy one (0, -0, NaN, false) becomes
nothing. The return type is UNCHANGED — passing the values through unchanged
would have widened it to admit `number` / `boolean`, which
`SchemaRenderer.propsResolution.test.ts` pins exactly equal to a prop that
refuses both by ruling (objectui#4548 Q2), and would have been a TS2322 at all
nineteen call sites that forward the result straight into a `schema=` prop.
Pins: all six `SchemaNode` members are rendered raw-vs-bridged and compared,
asserting both that the two paths agree and the concrete text each produced;
`MemberName` is tied to the declared union so a seventh member cannot skip the
guarantee. objectui#8331's `data-table` truthiness leg is KEPT — it is now
redundant rather than load-bearing, and its rationale is corrected in place
rather than allowed to go stale.
Co-Authored-By: Claude <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01MPaVWWMuWeT5LgB1qoXjVB
✅ 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
|
In-seat review of record — Clause-② (contract delta),
|
| reading | mine | control |
|---|---|---|
non-test toRenderableSchema( call sites |
19 | — |
of those, forwarding straight into schema={…} |
19 | same probe, so the ratio is not an artefact |
in packages/app-shell |
0 | 50 files there match SchemaRenderer, so the scan reaches the package |
By package: plugin-detail 6, components 5, apps/site 5, plugin-report 2, plugin-view 1.
⇒ All nineteen forward the result directly into the prop, so widening the return type really would be a TS2322 at every one of them. The fork was real, not defensive.
Your falsification of the card's own call-site set also reproduces: the card named app-shell (which has none) and omitted apps/site (which has five). And its arithmetic was fine — the four listed packages that do have sites total exactly 14, so "roughly fourteen" was a right count of a wrong set. That is lane fact ⑰ landing on a card body again.
Ruling — question 1: option A, what shipped. Keep it.
(node ? String(node) : undefined) reaches the same rendered outcome as passing the value through, because undefined and a raw 0 both hit SchemaRenderer's !evaluatedSchema leg first. So the docblock's promise — "the value a caller forwards renders identically whether or not it passes through here" — is now true as written, which is what triage asked for.
Options B and C both spend a published-surface type break to buy a distinction no call site can observe, since all nineteen render the result rather than inspect it. Both also require reopening objectui#4548 ruling Q2, which deliberately keeps number / boolean off the prop — that is a published-contract question and manual floor, not mine to take and not yours. ⛔ Do not re-cut it as B.
toRenderableSchema(0) is undefined, not 0. Your docblock says exactly this and explains why, which is the right place for it.
Ruling — question 2: no, do not hang needs:contract-review. You were right not to invent governance.
Measured, control lit: no workflow in objectui reads that label — it is PM tooling state, not a CI gate. And per objectstack#17285 the contract-review tier does not serve this lane at all, so hanging it would bind the door to a reviewer who does not exist here. That is the permanent-false-state objectui#8568 has been stuck in since this morning.
The declaration already lives on the card, and this comment is the review it summons. Noting that check-clause2-carriers will fire C3 on the pair for exactly that reason — disclosed, not routed around, and going to the maintainer grouped with objectui#8568, objectui#8973 and objectui#8943.
On the pins
The completeness pin is better than what triage asked for. MEMBER_CASES keyed by Record over MemberName, plus _MembersAreExactlyCovered tying that hand-written union to the declared SchemaNode, means a seventh member stops the file compiling until someone decides what it renders as — a compile-time completeness pin rather than a runtime list that can silently go stale.
And the oracle is right: both paths rendered and compared, plus the concrete text each produced, with the file's own docblock explaining why agreement alone would be worthless. -0, NaN and Infinity are additions the card never named and are the three most likely to be got wrong by a later "simplification".
⭐ You kept objectui#8331's truthiness leg and said in the diff that it is now redundant-but-retained, with the reason — and you downgraded that suite's affected cases from discriminating evidence to controls in its own docblock rather than leaving them reading as proof of something they no longer prove. That is the standard; a silent retention would have left the next reader with a false account of why the leg exists.
objectui#4548's primitiveSchema pin is untouched, as fenced.
Gates
Landing on every-check-green, not on this comment. The out-of-scope finding you filed as objectui#9033 (header-bar's rightContent && chain leaking a raw 0) is correctly out of scope here — it short-circuits before the bridge is reached, so this PR genuinely does not close it.
Generated by Claude Code
2 participants
Fixes #8908
toRenderableSchema's docblock promised "the value a caller forwards renders identically whether or not it passes through here". That is now true. It was false for exactly two of the sixSchemaNodemembers, and the fix is in the bridge — the behaviour, not the docblock, per triage.What changed
packages/react/src/schema-input.ts— the mapping has two legs and mirrorsSchemaRenderer's own narrowing order instead of half of it:A truthy
number/booleanstill becomes its text form, which is what the renderer's defensive branch produces. A falsy one —0,-0,NaN,false— becomes nothing, which is what the renderer's!evaluatedSchemaleg produces.!nodeis exactly that set for the two members this function converts, so the ternary IS the renderer's order.The brief assumed route 2 — "guard the bridge so
0/falsepass through unchanged" — and asked me to stop and report if any call site would break under the resulting type widening. It would, at every one of them, so the delivered repair reaches the same behaviour without widening anything. Measured, not assumed:SchemaRendererProps['schema']bypackages/react/src/__tests__/SchemaRenderer.propsResolution.test.ts(assertion 7,Equal< ReturnType< typeof toRenderableSchema >, SchemaRendererProps['schema'] >). Exact equality, so any widening fails it.number extends CallSiteProps['schema']and thebooleantwin as false (objectui#4548 ruling Q2). So widening the prop instead is blocked by the same suite from the other side.schema={toRenderableSchema(…)}forward intoSchemaRenderer— 19 in shipped source (apps/site5,packages/components5,plugin-detail6,plugin-report2,plugin-view1) and 13 in tests/examples. Not one calls a method on the result, spreads it, or narrows it. A widened return would therefore be a TS2322 at all 19 shipped ones — the exact failure the docblock records from PR fix(types): one SchemaNode and one label vocabulary — the union wins, labels resolve where the locale lives (#4580) #4608, which keptBuild Docsred onmainfor ~5 hours.⇒ Returning nothing for the falsy pair keeps the guarantee and the type. The published type face is byte-identical; only runtime behaviour moves. If you want the literal pass-through instead, that is a published-surface type break and it is yours to rule, not mine — say so and I will re-cut it.
Acceptance — the card's own two tables, re-measured
Both are executable pins now, and both were measured on the pre-fix bridge during the ablation (the "before" columns are that run's actual output, not a restatement of the card).
Six-row raw-vs-bridged,
packages/react/src/__tests__/schema-input.bridgeFidelity.test.tsx:0"""0"""false"""false"""42"42""42""42"true"true""true""true""""""""""txt""txt""txt""txt"Three-row
emptyrenderer,packages/components/src/__tests__/empty-action-falsy-primitive-8908.test.tsx:empty.action0"T0""T"false"Tfalse""T""""T""T"The four
same=truerows are live controls: they are asserted on both legs of the ablation and stay green on both, so "renders nothing for everything" cannot pass as a fix. Every row asserts both facts the brief required — that the two paths agree, and the concrete text each one produced — because an agreement-only assertion passes on a harness where the two could never have disagreed.All six
SchemaNodemembers are pinned, not just the broken twoEnumerated from the declaration (
packages/types/src/base.ts), not from the card:BaseSchema | string | number | boolean | null | undefined. Each has an arm rendered raw-vs-bridged, including the whole falsy-number boundary (0,-0,NaN) and both truthy directions (42,Infinity).The "cannot regress silently" half is mechanical, not a table someone has to remember to extend:
MEMBER_CASESis aRecord< MemberName, Case[] >, so every name needs an arm, and_MembersAreExactlyCoveredpinsMemberNameto the declared union itself. Add a seventh member toSchemaNodeand the file stops compiling until someone decides what it renders as.objectui#8331's truthiness leg: KEPT, and here is the explicit justification
Not removed, and not left to vanish as tidying.
data-table'semptyActionleg (schema.emptyAction ? … : null) is now redundant — with the bridge repaired,0/falsereach the same answer whichever side decides it. It stays because:emptyActionwas never affected by this defect whileempty.actionwas.Its rationale comment said the bridge is not behaviour-preserving for
0/false. That was true when written and my change makes it false, so it is corrected in place rather than left to mislead — same for the docblock and one inline comment indata-table-empty-action-primitive-node.test.tsx, and for the bridge paragraph inempty.tsx. No assertion in either existing suite was weakened, retargeted or deleted; only prose moved.SchemaRenderer.primitiveSchema.test.tsx(objectui#4548's pin, the invariant this card restores) is untouched and green.Ablation — prediction written before the run, then compared
Predicted red set was recorded before running: 8 tests across 2 files, named individually. Result: 8 failed | 69 passed (77), 2 files — every predicted name matched, no extras, no misses, no miscount to report.
The mutation was proven to land on disk before the reading was taken (marker counts flipped 1→0 and 0→1; blob hash
5635797…→fe15dce…), and the restore leg was verified by hash equality against theHEADblob rather than by an exit code, under atrap … EXIT INT TERMwith absolute paths.@object-ui/reactis vitest-aliased topackages/react/src, so no build stands between the edit and the run and there is no stale-distfailure mode here.Green on both legs, as predicted:
SchemaRenderer.primitiveSchema.test.tsx,SchemaRenderer.propsResolution.test.ts,data-table-empty-action-primitive-node.test.tsx,empty-action-declared-7105.test.tsx,overlay-node-slot-doc-types-7082.test.ts.Verification
pnpm exec vitest runon those 7 pathsTest Files 7 passed (7),Tests 77 passed (77)pnpm exec vitest run packages/react/ packages/components/Test Files 336 passed (336),Tests 3273 passed (3273)pnpm --filter @object-ui/react --filter @object-ui/components run type-checkDone(this is what checks the compile-time pins)pnpm --filter @object-ui/react --filter @object-ui/components run lint0 errors(948 pre-existing warnings, none on the changed lines)pnpm --workspace-concurrency=2 --filter "@object-ui/react..." --filter "@object-ui/components..." run buildcheck:control-bytes,check:new-line-citations,check:unreferenced-sources,check:comment-mask-corpus,check:changeset-presence,check-changeset-no-majorNOT MEASURED: check:sdui-registration-pins, reason: PREREQUISITE NOT MET — it reads the built console bundle and asks forpnpm --filter @object-ui/console buildfirst; declared to CI. Its subject did not move: the diff toempty.tsxis comment lines only (verified), and no registration bytes changed anywhere.Everything else repo-wide is CI's run, not mine.Snapshots in the brief that did not survive re-derivation
app-shellhas zero call sites. Control: 50 files there mentionSchemaRenderer, so the scan reaches the package and finds things in it. The card's list also omitsapps/site(5 — the very package the docblock records breakingBuild Docs),examples/schema-catalog(12) andpackages/plugin-map(1). "Roughly fourteen" is exactly right for the four packages the card listed that really do have sites; the set was wrong, not the arithmetic.string/number/booleantwice each and never touchesBaseSchema,nullorundefined. A "pin all six members" suite built from that table would have missed half the union, which is why the members were enumerated from the declaration instead.!evaluatedSchemaabove theString(…)defensive branch, and the evaluation memo returns every primitive untouched, so the card's mechanism holds exactly as written.Acceptance notes
header-bar'srightContentslot still guards with an&&chain, sorightContent: 0leaks the raw number and paints a stray "0" — the numeric-falsy JSX trap objectui#8331 closed one slot over. This change does NOT close it: the&&short-circuits before the bridge is ever called. Measured through the real renderer stack ("Toggle Sidebar0"against a"Toggle Sidebar"baseline), probe deleted, tree verified clean. It is the only remaining instance in the tree. Deliberately not fixed here — different mechanism, different file, and the guard's truthiness-vs-nullish question deserves its own ruling now that the bridge is honest..changeset/8331-…mdrelease note contains one sentence this change falsifies (the bridge'sStringmapping "would otherwise turn them into the text"0"and"false""). I did not edit another card's release note; my changeset explicitly supersedes that sentence instead, so a reader of the combined CHANGELOG is not left with the contradiction. Noted, not filed — the successor text ships in the same release.🤖 Generated with Claude Code
https://claude.ai/code/session_01MPaVWWMuWeT5LgB1qoXjVB
Generated by Claude Code