fix(plugin-charts): pie / donut / radar / scatter render as the family they name - #8034
Merged
Merged
Conversation
`pie-chart`, `donut-chart`, `radar-chart` and `scatter-chart` all rendered
as BAR charts. Each registration declared its family as
`defaultProps: { chartType: … }`, and nothing on the SDUI path has ever read
a registration's `defaultProps` — `SchemaRenderer` does not, and the tree's
one consumer (`WidgetRegistry`) writes manifest defaults INTO the registry
rather than reading these back. So `ChartRenderer` resolved no family and
`AdvancedChartImpl` fell to its `'bar'` default: valid data, a confidently
wrong picture, no refusal that could fire.
`normalizeChartSchema` — the package's single translation point, and the
producer of the `spec.chartType` that `ChartRenderer` already resolves
through — now maps the registered chart-type keywords to their families,
both the bare and the namespaced spelling. An explicit `chartType` keeps
precedence. The five inert `defaultProps: { chartType: … }` are removed
with it.
The app-shell DOM-leak sweep swept those four renderers for the first time
as a result; its scatter target now carries a schema scatter can draw.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01QnpvbdoRisQdRAczkLwnf5
`SchemaRendererProvider` requires one; every schema in the pin carries its rows inline, so the stub answers with nothing — a chart that started querying instead of plotting its own `data` shows up as an empty plot, not a pass. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QnpvbdoRisQdRAczkLwnf5
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 was referenced Sep 6, 2026
os-steve
marked this pull request as ready for review
September 6, 2026 12:21
This was referenced Sep 6, 2026
This was referenced Sep 6, 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
1 participant
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 #7401
Implements the ruling recorded on the card (5557016463, director seat, on maintainer authorisation): route C —
ChartRendererderives the chart family from the schema's owntype, and the inertdefaultPropsgo with it. ⛔ A/B are not re-opened and the A radius is not re-measured.What was wrong
pie-chart,donut-chart,radar-chartandscatter-charteach declared their family asdefaultProps: { chartType: … }on the registration, and nothing on the SDUI path has ever read a registration'sdefaultProps. SoChartRendererresolved no family andAdvancedChartImplfell to its'bar'default: four documented component types were one family, on valid data, with nodata-chart-errorthat could fire.The five ruled items
1. The family is derived from the schema's own
type. ✅ At the resolution point the ruling names —ChartRenderer.tsx:163,chartType: schema.chartType ?? spec.chartType— which is unchanged. The derivation lands in thespec.chartTypehalf, insidenormalizeChartSchema, which already carries atypelimb (chartType→specType→ a bare spec family name) and is documented as the package's ONE translation point. Two reasons it belongs there rather than inline inChartRenderer:normalizeChartSchemais exported from the package entry precisely so a consumer can ask whatAdvancedChartImplis actually handed. A derivation that lived only inChartRendererwould make that exported function answerundefinedfor a schema the runtime draws as a pie — a published function lying about the runtime.schema.chartTypestill wins at both layers, soplugin-charts:chartwithchartType: 'scatter'(the card's control row) is byte-unchanged, andtype: 'pie-chart'+chartType: 'line'draws a line.Both spellings resolve — bare (
pie-chart) and namespaced (plugin-charts:pie-chart); the card measured both drawing a bar.-chartsuffix rule. A suffix rule accepts the whole cross product ofRENDERABLEand would answerfunnelforfunnel-chart, a keyword nothing registers — an unreachable answer today that becomes load-bearing the day someone registers it meaning something else.CHART_TYPE_KEYWORD_FAMILIESnames exactly the keywords that exist and is pinned against the live registry in both directions, so adding a registration without a family entry (or an entry without a registration) is what goes red.2. The five inert
defaultProps: { chartType: … }are removed. ✅index.tsxlines 144 / 155 / 166 / 177 / 188 (chart:bar,pie-chart,donut-chart,radar-chart,scatter-chart), re-measured onf5c8b8eand unchanged from the dispatch's reading.One mechanism, stated:
bar-chartkeeps its wrapper. It is registered toChartBarRenderer, a different component that renders throughChartImpland never callsnormalizeChartSchemaat all — so a family entry for it would be inert in exactly the way this card removed. It is deliberately absent from the table, and the docblock at both ends says so.chart:baris the one that moved onto the derivation: it renders throughChartRenderer, so it is a table entry and now declaresbarrather than inheriting it from a default.The two surviving
defaultPropsobjects in this file (bar-chart,plugin-charts:chart) are sample-data seeds, not family declarations, and are not among the ruled five. They remain unread on the SDUI path — part of ruling item 5's 32-site census, not this card.3.
examples/chart-examples.tsdraws a pie. ✅ Confirmed by rendering the example object itself, not by argument —pieChartExample,donutChartExampleandradarChartExampleare imported into the new pin and rendered throughSchemaRenderer. No edit to the examples file was needed; it was correct all along and the renderer was not.4. The
app-shellDOM-leak sweep went red, and the red was correct. ✅ See its own section below.5. Not A. ✅ No change to how the SDUI path treats registration
defaultPropsgenerally. That mechanism is still unread repo-wide.Ruling item 4 — the expected red, and what it actually was
packages/app-shell/src/__tests__/widget-dom-leak-sweep.test.tsxwent red on exactly one target, first run after the fix:That is not a DOM leak. It is PR #7400's refusal firing for the first time on a target that had been passing as a bar chart — the very evidence the card was opened on ("PR #7400's refusal should have reddened this two-series entry and did not"). The sweep's four chart targets were never sweeping pie / donut / radar / scatter; they were sweeping one bar chart four times.
Leaks exposed: zero. With the four renderers rendering as themselves for the first time,
pie-chart,donut-chartandradar-chartpass the attribute scan unchanged, andscatter-chartpasses once it is handed a schema scatter can draw.plugin-chartsstays at 9 targets / 0 leaking / 0 attributes — the reading in that file's table is still true, and is now true about four more renderers than it was. No leak cards to file, and none folded in.⛔ Nothing was reverted, skipped, quarantined or
.skipped. What changed is the scatter target's fixture, on two counts, and both of them add coverage rather than remove it:series[0].dataKeyis theYAxiskey) and refuses a second by name. Two series here would have swept the refusal's markup, leaving the scatter renderer exactly as unswept as it has been.name: 'Jan'has no position and every row is unplaceable (no-plottable-points) — the other refusal, and the same blindness one step over.Both refusals render attribute-clean markup, which is precisely the phantom-clean pass that file's own objectui#5630 section warns about. The comment on
SCATTER_DATArecords all of this so the next editor does not "simplify" it back onto the shared constants.The #7396 / #7400 interaction — confirmed, not folded in
scatter-chartnow genuinely reaches the scatter arm, so it meets both siblings. ⛔ Neither is folded in.scatter-multi-series). Reachable through this type for the first time, and now pinned as such: a two-seriesscatter-chartrendersdata-chart-error="scatter-multi-series"where it previously drew two silent bars. This is the ruled behaviour (bug(plugin-charts): a multi-series scatter draws every series at the FIRST series' y values — two measures, one cloud painted twice #7194 ruling B: refuse loudly, do not project), not a regression.Mechanism assumptions the dispatch asked to be measured
ChartRenderer.tsx:163still the right resolution point? Yes, andschema.typeis in scope there. The derivation is nonetheless placed one layer down for the two reasons in item 1 above; line 163 is untouched and still the point where the family is resolved.defaultPropsbreak a reader? No, andWidgetRegistry.ts:189specifically does not reach them. Traced: it is a write, not a read — it copies a widget manifest'sdefaultPropsINTOComponentRegistry.register(...). Nothing flows the other way. The tree's only otherdefaultPropsreader isPageDesigner.tsx:173,paletteItem?.defaultProps, which readsDEFAULT_PALETTE— a designer-local literal in the same file that has nodefaultPropson any entry and no chart-type entries at all. No gate script or doc reads them either.Tests
Ablation (reverse verification), run from the committed implementation:
?? familyFromComponentType(rawType)limb. Proven on disk before running: deleted-text count 1 → 0, injected-text count 1, andgit hash-objectdiffering from the HEAD blob. The mutation reaches the code under test through vitest's source alias (vitest.config.mts:421maps@object-ui/plugin-chartstosrc), so there is nodiststaleness leg.git checkout HEAD -- ..., proven by an emptygit diff HEADand a blob hash byte-identical to HEAD, not by an exit code.packages/plugin-charts/src/__tests__/chart-family-from-type-7401.test.tsxpackages/plugin-charts/(the whole package)packages/app-shell/+packages/plugin-report/+packages/plugin-dashboard/+examples/schema-catalog/pnpm --filter @object-ui/plugin-charts type-check/@object-ui/app-shell type-checkpnpm --filter @object-ui/plugin-charts lint/@object-ui/app-shell lintplugin-reportis in that list becauseReportRenderer.test.tsx:44authorschart: { type: 'pie-chart' }— the other in-repo consumer of these keywords besides the examples file.examples/schema-catalogcarries noFAMILY-chartfixture (checked); its two chart fixtures aretype: "chart"with an explicitchartType, which the derivation cannot reach.Gates, derived by hand from the changed paths —⚠️
scripts/pm/dispatch-gates.mjsdoes not exist in this repo, it is objectstack-only. Exit codes read before any pipe:check:sdui-registration-pinsis left to CI: it weighsapps/console/dist/assetsand needs a console build. It reads the registration KEY set, which this PR does not move — all fiveComponentRegistry.registercalls keep their literal keys and only theirdefaultPropsmember is removed.node scripts/check-governed-queue-guard.mjs --teston the five changed paths: NOT GOVERNED. This PR nonetheless stays a draft — it is dispatched work and the maintainer merges it.One observation, reported rather than swallowed
On the first full
packages/plugin-charts/run,ChartRenderer.catalogRender-6939.test.tsx > advanced-line-chart renders identically to BASEfailed withnothing drew— stuck on the Suspense skeleton at the default 1swaitFor, in a run whose own summary reports 279s of import time across 47 files. The same file passes 5/5 in isolation, the whole package re-ran 47/47 files, 433/433 tests green, and both of that file's fixtures aretype: "chart"with an explicitchartType, which the derivation provably cannot reach. Recorded as a load-induced timeout in that pin, not as a result of this change; ⛔ not fixed here and not folded in.Scope
Clause-②: no— nopackages/typesor@objectstack/specfile is touched; this is registration mechanics insideplugin-charts. Siblings #7681 #7682 #7690 #7698 #7396 are not folded in. Changeset:@object-ui/plugin-chartspatch, naming the four types.skip-changesetlabel is inert in this repo and is not applied.Generated by Claude Code