fix(types): BaseSchema.visible accepts the predicate string the renderer evaluates (#4581) - #4593
Merged
Merged
Conversation
…rer evaluates (#4581) `visible` was declared `boolean`, but the renderer never read it as one — it evaluates the key. `SchemaRenderer.tsx:382` calls `evaluator.evaluateCondition(schema.visible)`, and `evaluateCondition` is declared `(condition: string | boolean | undefined, context?) => boolean`. The sibling keys `visibleWhen` and the deprecated `visibleOn` are `string` for that same reason; `visible` under-reported a capability it already had, and fixtures exercising it had to cast past the declaration. Widened to `boolean | string` — exactly what the evaluator accepts, no wider — and the two `as unknown as BaseSchema` casts in the expressions suite that existed only for this gap are dropped. Type-only: all 54 emitted `.js` files in @object-ui/types are byte-identical to origin/main. Part of #4581. #4580 and the ariaLabel half are escalated rather than implemented — see the PR body. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017Qqyix2QcnpUC9XeYVDzx3
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
This was referenced Aug 13, 2026
Contributor
✅ Console Performance Budget
📦 Bundle Size Report
Size Limits
|
yinlianghui
marked this pull request as ready for review
August 13, 2026 17:14
This was referenced Aug 13, 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.
Part of #4581.
SchemaNode(#4580) and to ride #4581 along with it. Measurement blocked both of those on one unruled contract question, so this PR ships only the half that measured clean, and the rest is escalated rather than guessed. #4580 is not touched here and stays open; #4581 is half done, hencePart of, notFixes.What landed
BaseSchema.visibleis declaredboolean | string.It was
boolean, but the renderer never read it as a boolean — it evaluates the key. Both call sites the ruling asked me to verify, cited:visible—packages/react/src/SchemaRenderer.tsx:382:and
evaluateConditionis declared(condition: string | boolean | undefined, context?) => boolean(packages/core/dist/evaluator/ExpressionEvaluator.d.ts:143). The sibling keysvisibleWhenand the deprecatedvisibleOnarestringfor exactly this reason. The widening isboolean | string— what the evaluator accepts, no wider.ariaLabel—packages/react/src/SchemaRenderer.tsx:111callsresolveKeyedI18nLabel(schema.ariaLabel). This call site is what disproved the ruling's spelling for the ariaLabel half. See "Why ariaLabel did not land" below.Two
as unknown as BaseSchemacasts inSchemaRenderer.expressions.test.tsxexisted only for this gap and are dropped.The census — the recorded "five" is not what is on disk
The ruling said five casts, "each carrying a comment naming this gap". Grepping
as BaseSchema/as unknown as BaseSchemaacrosspackagesandappsfinds six, in three different classes, and only one carries such a comment:react/__tests__/SchemaRenderer.expressions.test.tsx:52visible: '${data.role === "admin"}'react/__tests__/SchemaRenderer.expressions.test.tsx:61visible: '${data.role === "admin"}'react/__tests__/SchemaRenderer.expressions.test.tsx:132disabled: '${data.status === "locked"}'disabledgap — not ruled, leftreact/__tests__/SchemaRenderer.expressions.test.tsx:141disabled: '${data.status === "locked"}'disabledgap — not ruled, leftreact/__tests__/SchemaRenderer.aria.test.tsx:59ariaLabel: { key: …, defaultValue: … }components/__tests__/html-anchor-links.test.tsx:33{ type: 'a', ...schema }Record< string, unknown >spread, no commentSo of the six, this card's ruled widenings close two. Three further notes:
disabledis the same defect asvisible, with the same evidence —SchemaRenderer.tsx:466evaluates it through the sameevaluateCondition, and adisabledOn?: stringsibling exists for the same reason. finding(types): BaseSchema under-declares visible (predicate string) and ariaLabel (I18nLabel) — the renderer supports both, five test fixtures now cast past the gap #4581 named onlyvisibleandariaLabel, so I did not widen it unruled. Two casts and one one-line widening are waiting on a word.html-anchor-links.test.tsxis the file finding(types): BaseSchema under-declares visible (predicate string) and ariaLabel (I18nLabel) — the renderer supports both, five test fixtures now cast past the gap #4581 named, but the cast in it is not this gap — it is aRecord< string, unknown >spread being cast toBaseSchema.plugin-dashboard's twoas BaseSchemaare finding(react): SchemaRenderer carries the #4422 prop erasure in a spelling every sweep and both guards are blind to —Record< string, any >instead of[key: string]: any#4548's deliberate narrowing casts, untouched.Red-first — predictions written into the test header before the run
packages/types/src/__tests__/base-schema-visible-predicate.test.tspins the widening. Againstorigin/main(92250d648),tsc -p packages/types/tsconfig.test.jsonreported, verbatim:Post-fix: clean, exit 0.
The equality assertion is invariant (
Equal, notextends) on purpose, and the header says why: asatisfies-style or one-wayextendscheck would be vacuous in both directions here — the narrowbooleanis assignable to the wideboolean | string, so a widening that never happened and a widening that overshot toanywould both stay green.BaseSchema's[key: string]: anyindex signature makes the overshoot a live risk, not a hypothetical: deleting the declared property altogether leavesvisibletypedanyand every fixture still compiling. Pinning the exact union is the only assertion that can go red for the right reason.must-not-change
.jsfiles in@object-ui/types'disthave equal sha256 against anorigin/maincompare worktree, built the same way. This is the finding(plugin-dashboard, plugin-list): the two packages #4422 left unswept still erase every declared prop — and the #4438 guard cannot see them #4528 bundle-sha256 bar, and a type-only card meets it exactly — ruling 4's "zero runtime behavior change" is satisfied at the strongest available standard rather than argued.packages/types+packages/coresuites: 119 files, 2200 tests, all passing.--filter='...@object-ui/types'= the downstream consumers, 27 packages): zero new errors attributable to this widening. Measured in isolation, with theSchemaNodechange reverted, precisely so the two changes could not launder each other.origin/maincompare worktree: 21 warnings / 0 errors both sides — net zero; the new test file contributes 0.Published
.d.tsdiff, and the grading analysisBuilt both ways with
dist/and*.tsbuildinfocleared between builds. The entire diff:/** * Controls whether the component is visible. * When false, component is not rendered (display: none). + * … (evidence comment) * @default true */ - visible?: boolean; + visible?: boolean | string;Graded
minorby position analysis, not by assumption: the diff adds a member to a union on an authored-input-dominant property, removes nothing, and touches no other declaration — the #4586/#4591 shape. Authors gain a spelling; nothing that type-checked before stops doing so. Readers ofschema.visiblewere already coping withanythrough the index signature. Never major (major tracks@objectstack).@object-ui/coregets no changeset entry: its declaration is untouched in this PR, so it has no source or declaration diff.toRenderableSchemainpackages/reactSTAYSStated explicitly so no future card "cleans it up":
SchemaRenderer's component-level union deliberately excludesnumber/boolean(#4548 ruling, Q2), so the bridge still normalizes those onto their text form. It is a total function, not a cast. Nothing in this PR makes it an identity function, and nothing here is a reason to remove it.Why
SchemaNode(#4580) did not land — the measured blockerThe reconciliation itself works. Implemented as ruled (core's
interface SchemaNodebecomesexport type { SchemaNode } from '@object-ui/types';), built clean, and the red-first collision pin went red pre-fix exactly as predicted and clean post-fix — the #4548 error class, verbatim, naming bothdistidentities:core/dist/index.d.tswas unchanged by it — the entry surface is preserved, as the ruling required.What stopped it is the repo-wide canary. The reconciliation surfaces 274 errors in
@object-ui/react:spec-bridgesuites) —TS18049×134 andTS2339×131, all of the formProperty 'sections' does not exist on type 'string | number | boolean | BaseSchema'. Mechanical narrowing, in scope, mine — but 272 of them is a cost worth a decision on its own, and it points at the bridges' declared return type as the real lever rather than 272 local narrowings.packages/react/src/spec-bridge/bridges/list-view.ts:180and:224:from
if (spec.label) node.label = spec.label;and thedescriptiontwin.spec.labelis the spec'sI18nLabel;BaseSchema.labelisstring. Core's[key: string]: anyhad been absorbing that mismatch — remove the duplicate declaration and a real latent defect surfaces. Fixing it needs a cast (the lenient-consumer fallback the contract-first rule forbids) or a producer-side widening that nobody has ruled. Per ruling 4, runtime-source reshaping is a STOP, so I stopped.Why
ariaLabeldid not land — the ruling's spelling is the wrong vocabularyThe ruling says widen
ariaLabeltostring | I18nLabel. Measured, that spelling declares a shape the renderer cannot resolve, and still rejects the shape it can.@object-ui/typesre-exportsI18nLabelfrom@objectstack/spec/ui, where it is the inline locale mapstring | Record< string, string >. ButSchemaRenderer.tsx:111resolvesariaLabelwithresolveKeyedI18nLabel, whose declared input is the keyed formstring | { key: string; defaultValue?: string; params?: Record< string, any > }.packages/react/src/utils/i18n.tsdocuments these as two vocabularies that "answer wrongly for the other's input, silently" (objectui#4167, PR #4169).A throwaway probe measured all four consequences, and all four predictions held:
I18nLabelisstring | Record< string, string >{ key, defaultValue }understring | I18nLabelkeyanddefaultValueparams: { name: 'Ada' }Type '{ name: string; }' is not assignable to type 'string'{ en: 'Owner' }resolveKeyedI18nLabelreturnsundefinedfor it at runtime, rendering an emptyaria-labelSo the ruled widening would have turned the aria test green vacuously, invited a shape that renders empty, and still rejected keyed labels with
params. That is the opposite of the ruling's own stated intent ("matching what the renderer actually supports"), so I did not write it.The two escalations are one question
BaseSchemadeclares three sibling label slots —label?: string(:56),description?: string(:62),ariaLabel?: string(:173) — and all three under-declare, in two different i18n vocabularies:label/descriptionreceive the spec's inlineI18nLabelfrom the bridges, whileariaLabelis resolved with the keyed resolver. Core's index signature was hiding all of it.That is why #4580 is blocked on #4581 rather than the other way round: the ruling treated the
ariaLabelwidening as a rider on the reconciliation, but the reconciliation is what forces the label-vocabulary decision. One ruling on "which vocabulary does eachBaseSchemalabel slot declare" unblocks the reconciliation, theariaLabelwidening, and the twolist-view.tsdefects together. Options and a recommendation are in the seat's report.Generated by Claude Code