feat(spec,lint): pin <ObjectChart> aggregate result-column naming and validate its axes (#3701) - #3725
Merged
Conversation
… validate its axes (#3701) #3684 extended ADR-0021 axis checking to report charts, list-view charts and dataset-bound page chart components, but had to leave the react <ObjectChart> block out: it is OBJECT-bound (objectName + an inline aggregate), `aggregate` existed in the contract only as the description string '{ field, function, groupBy }', and nothing in the repo said what the aggregated result columns were called. With no columns to resolve against, its axis bindings could not be validated, and inventing a convention would have manufactured false positives (ADR-0072 D1). Record the convention rather than invent one. Every path that can serve an object-bound chart already agreed: the engine's structured-groupBy aggregate (whose alias objectui sets to `field || function`), the legacy analytics query (which remaps its measure key back to `field`), the client-side fallback, and the console's own chart-view wiring (`xAxisKey: groupBy`, `series[].dataKey: field`). An object-bound aggregate returns rows keyed by the RAW FIELD NAMES — `groupBy` for the category column, `field` for the value column, the literal `count` for a fieldless count, plus `<field>__comparison` under a comparison overlay. That is the deliberate opposite of the dataset path, whose rows are keyed by the declared measure `name`; only the dataset path has an author-chosen name to key by. spec: - ChartAggregateSchema / ChartGroupBySchema / ChartAggregateFunctionSchema in chart.zod.ts replace the description string, and reject a non-count function with no field (which reached the renderer as sum(undefined) and drew a blank chart). - chart-aggregate.ts records the convention and exports the derivations — chartAggregateCategoryKey / chartAggregateValueKey / chartAggregateResultKeys — so producers and checkers cannot drift apart by re-deriving the rule separately. - <ObjectChart>'s contract now names the props the block actually reads. ChartConfig's xAxis/yAxis/series shapes reached it and were silently dropped, which ADR-0078 forbids; they leave dataProps, and chartType, xAxisKey and series join the React overlay where the other bindings live. lint: validate-react-page-props now reads attribute VALUES, not just names, for <ObjectChart> — react-chart-field-unknown (aggregate.field/groupBy off the object), react-chart-aggregate-invalid (unimplemented function, or a non-count function with nothing to aggregate), react-chart-axis-unknown (an axis naming a column the aggregate never returns, incl. a dataset-style sum_total, or a category axis bound to the value column), react-chart-axis-inert (the axis shapes this block never reads). Value reading evaluates only static literals: a prop driven by React state, a usage carrying a spread, a chart given inline data, and objects another package defines are skipped — an unresolvable binding is not a wrong one. The golden page binds its axes explicitly so the chain is proven end to end: swapping `total` for `sum_total` there fails `os validate` with the new rule. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UMHZBHTjH4rw8xmDYirFC7
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
github-actions
Bot
added
documentation
Contributor
📓 Docs Drift CheckThis PR changes 2 package(s): 104 hand-written doc(s) reference the affected code and may need an implementation-accuracy re-verification:
|
6 tasks
os-zhuang
marked this pull request as ready for review
July 28, 2026 00:43
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.
Closes #3701.
#3684 extended ADR-0021 axis checking to report charts, list-view charts and dataset-bound page chart components, but had to leave the react
<ObjectChart>block out: it is OBJECT-bound (objectName+ an inlineaggregate),aggregateexisted in the contract only as the description string'{ field, function, groupBy }', and nothing in the repo said what the aggregated result columns were called. With no columns to resolve against, its axis bindings could not be validated — and inventing a convention would have manufactured false positives (ADR-0072 D1).1. The convention, recorded rather than invented
Every path that can serve an object-bound chart already agreed; it was simply never written down:
aggregate()w/ structuredgroupBy(objectuiObjectChart)groupBy.alias ?? groupBy.fieldalias, set tofield || functiongroupByfieldaggregateRecords)groupByfieldObjectView)xAxisKey: groupByseries[].dataKey: fieldSo: an object-bound aggregate returns rows keyed by the RAW FIELD NAMES —
groupByfor the category column,fieldfor the value column, the literalcountfor a fieldless count, plus<field>__comparisonunder a comparison overlay. That is the deliberate opposite of the dataset path, whose rows are keyed by the declared measurename(sum_amount) — the trapchart-measure-unknowncatches. Only the dataset path has an author-chosen name to key by.chart.zod.ts—ChartAggregateSchema/ChartGroupBySchema/ChartAggregateFunctionSchemareplace the description string, and reject a non-countfunction with nofield(which reached the renderer assum(undefined)and drew a blank chart).chart-aggregate.ts— records the convention and exports the derivations (chartAggregateCategoryKey/chartAggregateValueKey/chartAggregateResultKeys) so producers and checkers cannot drift apart by re-deriving the rule separately.2.
<ObjectChart>'s contract now names the props the block actually readsChartConfig'sxAxis/yAxis/seriesshapes reached this block and were silently dropped — it consumesxAxisKeyandseries[].dataKey. Advertising them violated ADR-0078 (a prop the author writes is honored or rejected, never silently dropped), and validatingyAxis[].fieldwould have meant validating a prop that does nothing. They leavedataProps;chartType,xAxisKeyandseriesjoin the React overlay where the other bindings live, per ADR-0082 decision 3.(
chartTypewas not published at all before, so an author following the contract had no way to pick anything but the default bar chart.)3. The prop gate now reads attribute VALUES
validate-react-page-propspreviously only checked prop names. For<ObjectChart>it now evaluates static literal values:react-chart-field-unknownaggregate.field/aggregate.groupBynaming a field the bound object does not declarereact-chart-aggregate-invalidcountfunction with nothing to aggregatereact-chart-axis-unknownsum_total), or a category axis bound to the value columnreact-chart-axis-inertxAxis/yAxisshapes this block never readsValue reading is opt-in per block and evaluates only static literals. Skipped silently, to keep false positives at zero: a prop driven by React state or a variable, an object literal with a shorthand or spread property, a usage carrying
{...spread}, a chart given inlinedata(its columns are the author's own), and objects another package defines.Verification
@objectstack/spec6702 tests,@objectstack/lint467 tests — all pass.check:react-blocks,check:api-surface,check:skill-refs,check:skill-docs,check:docs,check:spec-changes,check:skill-examples.renewals-pipeline.page.ts) now binds its axes explicitly, so the ADR-0082 chain is proven end to end — swappingtotalforsum_totalthere failsos validate:os validateotherwise passes clean (remaining warnings are pre-existing and unrelated).Companion PR
objectstack-ai/objectui — the convention exposed a real bug: a fieldless
countkeyed its value columnundefinedin three of the four runtime paths, and the legacy analytics path additionally deleted thecountthe server had returned. Since this PR's lint and skill docs now recommend the fieldless form, that had to be fixed rather than advertised (Prime Directive #10).Out of scope — follow-ups worth filing
Found while auditing what this block actually honors; not fixed here to keep the change proportionate (Prime Directive #10 says file, don't silently expand):
<ObjectChart>'s remaining inert config props —showLegendis read by nothing, andtitleis only used for the drill-down drawer title, never rendered as a chart title.validate-chart-bindings§3 validatesproperties.yAxis[].fieldfor dataset-bound page chart components, but the same renderer ignoresyAxisthere too — so that rule checks a no-op prop.🤖 Generated with Claude Code
https://claude.ai/code/session_01UMHZBHTjH4rw8xmDYirFC7
Generated by Claude Code