Uh oh!
There was an error while loading. Please reload this page.
fix(core): rate-limit ExpressionEvaluator's fault warnings to one per authored source - #6492
Merged
Merged
Conversation
… source `evaluate()`'s two built-in fault paths logged on EVERY evaluation, and it is the hottest of the three predicate paths in this area — `SchemaRenderer` calls it for every `properties.*` value, every `props.*` value and `content`, for every node, on every render. Measured on the built evaluator at 830ed58: three identical faulting `evaluateCondition` calls produced 3 console lines where the `{ dialect: 'cel' }` envelope produced 1, and one broken `${…}` prop across a 200-row list produced 200 lines per render. Both sibling reporters already carry a one-per-source rate limit (`warnPredicateFailure` in `fieldRules.ts`, `visibilityDiagnostic.ts` in `@object-ui/react`); this reuses that shape rather than adding a third. The key is the authoring identity — `[site, source]`, never the scope — which is both the siblings' precedent and the defect itself: the 200-row flood is ONE authored source against 200 distinct scopes, so a scope-sensitive key emits all 200 lines again. The rate limit governs the built-in line only: `onFault` still fires on every fault (#6038's passback contract) and `throwOnError` still throws every time. No symbol is added to the published surface — the whole `packages/core` `.d.ts` tree is byte-identical. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011SfZeFWrhGLHmfq61xbz4q
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011SfZeFWrhGLHmfq61xbz4q
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 26, 2026 06:04
Uh oh!
There was an error while loading. Please reload this page.
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 freeto 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#6444
ExpressionEvaluator.evaluate()'s two built-in fault paths logged on every evaluation, and it is the hottest of the three predicate paths in this area —SchemaRenderercalls it for everyproperties.*value, everyprops.*value andcontent, for every node, on every render. This gives them the same one-per-source rate limit both sibling reporters already carry. No third mechanism is introduced, and no symbol is added to the published surface.The flood, reproduced before the fix
Measured on the built evaluator (
packages/core/dist), same script both sides, base830ed5803:evaluateCondition('${nosuchroot.x > 1}')x3 (the card's headline){ dialect: 'cel' }envelope x3 — the control the card compares against${…}prop rendered across a 200-row listThe two message texts are byte-identical before and after (
Expression evaluation failed for: …/Failed to evaluate expression: …). The four cases run in one process against four distinct sources and produce four lines in total after the fix — i.e. the same run also shows the "different sources still log" direction.⛔ Not fixed by deleting the warn: a distinct broken source still gets its own loud line; only the repeats are dropped.
The card's open point: keyed on source text alone, and why
Chosen: the predicate's authoring identity —
[site, source]— never the scope it ran against.Read from the sibling reporters first, as triage directed. Both key on authoring identity and neither keys on the data:
fieldRules.tswarnPredicateFailure—JSON.stringify([expr.dialect, expr.source]), because "a broken predicate is re-evaluated on every render/keystroke, and the point is one loud line, not a scrolling wall".packages/react/src/utils/visibilityDiagnostic.ts—JSON.stringify([type, key, predicateSourceText(raw)]), wheretype/keyare the authoring location (node type, prop key), not the row: "the same broken predicate authored once and rendered over many rows is ONE authoring bug, and an object key would report it once per row".Here the precedent is also the defect. The 200-row flood is one authored source evaluated against 200 different scopes, so a scope-sensitive key is not a weaker fix — it is no fix. Replayed over the same real fault stream:
The
sitehalf of the key ('template-part'vs'whole-expression') is not a scope discriminator — it is the same defensive taggingvisibilityDiagnosticgives its two legs so two different faults cannot silence each other. The card's counter-consideration (the same text genuinely broken in one scope and fine in another) is real but is the caller's to report:onFaultis deliberately left outside the rate limit, so a caller doing per-node reporting keeps every fault.What deliberately does not move
onFaultstill fires on every fault — [Decision] What diagnostic budget should production carry for a faultingvisibleWhen? Today a node-gate fault is entirely silent in a production bundle #6038's passback contract, matching whatfieldRules.tsalready documents ("independent of … the one-time-warning dedupe … so a caller doing its own warn-once bookkeeping keeps control of it"). The rate limit governs the built-in line only.throwOnErrorstill throws on every evaluation — the fail-closed signal is not rate limited.visibleWhen? Today a node-gate fault is entirely silent in a production bundle #6038) silence are untouched.Clause ②: the published surface is unchanged — chain followed, not grepped
packages/coreis published and the chain is a two-hop wildcard, so a name grep on the entry would prove nothing:Anything exported from the source file would land on the published surface. Nothing is:
EvaluationFaultSite,warnedEvaluationFaultsandreportEvaluationFaultare all module-local. Measured rather than asserted, before/after a rebuild:packages/core/dist/**/*.d.ts— 93 files, all 93 SHA-256 hashes identical (diffof the hash lists is empty).Object.keys(await import('@object-ui/core'))is 283 names before, 283 after, diff empty.No test-only reset export was added either. This follows the in-package precedent (
fieldRules.ts's dedupe exports no reset);@object-ui/react's__resetVisibilityPredicateWarningswas not copied because no consumer needs it — the tests here use the sanctionedvi.resetModules()path instead.Tests — both directions, plus the cell that discriminates the granularity
New:
packages/core/src/evaluator/__tests__/ExpressionEvaluator.faultWarnDedupe.test.ts(8 cells).Either direction alone is vacuous — "same source logs once" passes if everything was silenced, "different sources log twice" passes if nothing was deduped — so both are pinned, together with the cell that separates the two candidate keyings (one source, 200 different scopes, one line: 1 under the shipped keying, 200 under source+scope).
Anti-ghost-assertion: every dedupe cell also asserts the fault really happened (the documented fail-soft value came back) and that a healthy expression in the same run still evaluates to its real value — so "one warn" cannot be an evaluator that stopped being called. The 200-row cell asserts all 200 renders produced 200 distinct outputs interpolating each row's own id.
Cross-test leakage was the live hazard here (a module-level
Setoutlives a test case).beforeEachdoesvi.resetModules()+ a fresh dynamic import — the exemptionobject-ui/no-dynamic-import-in-test-hookdocuments for exactly this — cells use distinct source texts anyway, and one cell proves the reset works rather than assuming it: same source deduped within one module instance, thenvi.resetModules(), then the same source warns again.Updated:
ExpressionEvaluator.onFault.test.ts. Its last cell asserted "nothing about the existing console output moves" and reusedFAULT_TEMPLATE, which an earlier cell in the same file already faults on — after this change it would have read that cell's dedupe entry, seen silence, and passed having measured nothing. It now faults on a source unique to the file, and its title says what it actually pins.Ablation — the right cells red, the controls green
Rate limit removed (message text left untouched), mutation confirmed on disk in both directions (injected marker 1, removed guard 0; blob
678d9b85->78fb1620), then restored with a path-scopedgit checkout HEADagainst the file's absolute path under atrap … EXIT INT TERM, and verified byte-identical (git hash-objectback to678d9b85,git diff HEADempty):The ablated numbers reproduce the card's measurements exactly (3 and 200). The mutation was made to source and the tests import
../ExpressionEvaluator.jsrelatively, so this run is also the proof that these cells read source rather than a staledist/.Gates run locally — union re-run on the final commit
4b1196142vitest run packages/core/Test Files 102 passed (102)/Tests 2039 passed (2039)vitest run packages/react/ packages/components/ packages/plugin-detail/Test Files 360 passed (360)/Tests 3620 passed (3620)turbo run type-check --filter=@object-ui/coreTasks: 3 successful, 3 totalturbo run build --filter=@object-ui/coreTasks: 2 successful, 2 totaleslint .inpackages/core✖ 515 problems (0 errors, 515 warnings), exit 0check-control-bytes✅ check-control-bytes: OK (scanned 5369 tracked text file(s); skipped 85 binary)check-changeset-presence✅ 3 source file(s) of 1 released package(s) changed, and this change declares 1 changeset(s)check-changeset-no-major✅ No changeset declares a 'major' bump.check-vi-mock-specifiers✅ check-vi-mock-specifiers: OKcheck-package-self-import✅ No package names itself inside its own src/.Exit codes were captured before any pipe (redirect to a file, then read
$?), never from atail.packages/core'stype-checkistsc --noEmit && tsc -p tsconfig.test.json, and--listFilesconfirms both edited test files are in the checked set — so "typecheck green" is a real statement about the new tests, not a silently-excluded one.Lint narrowing, measured.
pnpm lintisturbo run lint(per-packageeslint .); onlypackages/corewas run. (1) The population comes from eslint's own config, not my guess: (2)--format jsonreports 197 files selected underpackages/core. (3)eslint.config.jssets noproject/projectService, i.e. type-aware linting is not enabled, so a file's verdict depends only on its own text plus config — a diff confined topackages/core/src/evaluator/cannot move the verdict on any untouched file in any other package, and.changeset/*.mdis outside the config's**/*.{ts,tsx}selector entirely.Not measured, reported as such:
check-readme-exportsexits 1 in this worktree with356 self-import(s) could not be judged … type entry ./dist/index.d.ts is not on disk — run 'pnpm build' first. That is an unbuilt-worktree prerequisite, not a finding: zero of its 356 items concernpackages/coreor any file in this diff, and this PR touches no README and adds no export. CI builds first and runs it properly..changeset/6444-evaluator-fault-warn-dedupe.md—patchon@object-ui/core(nevermajor; fixed group).Draft on purpose, no auto-merge — the PM lands it.
Generated by Claude Code