Skip to content

Commit a51eb86

Browse files
claude[bot]claude
andauthored
fix(lint): chart series[].name advises instead of blocking — presentation, not a query binding (#15741)
* wip: per-position tier + consequence for chart-measure-unknown * test(lint): pin the per-position tiers; changeset * refactor(lint): re-point validate-chart-bindings onto recordsOf (#15636 seam) --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent ce244dd commit a51eb86

3 files changed

Lines changed: 526 additions & 39 deletions

File tree

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,20 @@
1+
---
2+
"@objectstack/lint": patch
3+
---
4+
5+
`chart-measure-unknown` no longer blocks a build over a chart `series[].name` (or a page chart's `yAxis[].field`) that names nothing — those positions are presentation, and the message now says so.
6+
7+
The rule fired at `error` on every measure position of the three chart surfaces it covers, with one consequence sentence: *"result rows are keyed by MEASURE NAME … so this series comes back empty"*. Read at the `@object-ui` revision this repo pins (`.objectui-sha`), that is true only where the position feeds the dataset query, and the three surfaces do not agree:
8+
9+
- **Report charts** run the chart's own query out of the two axis strings (`useDatasetRows(dataset, [xAxis], [yAxis], …)`*"the embedded chart queries only `chart.xAxis` × `chart.yAxis`"*), so `chart.xAxis`/`chart.yAxis` are the binding. `chart.series[]` is *"the author's per-chart override for ONE measure's display name"*, lowered through `mergeAuthoredSeries`, where *"an authored entry naming a measure that is NOT in the dataset selection is **ignored** — membership belongs to the dataset"*.
10+
- **List-view charts** have no presentation position at all: `ListChartConfigSchema` is a strict object of `chartType`/`dataset`/`dimensions`/`values`, and `values[]` is handed to the chart as the dataset measures.
11+
- **Dataset-bound page chart components** query `{ dimensions, measures: values }` and then replace the authored series wholesale with one derived entry per selected measure, so `properties.series[].name` reaches the renderer not at all and `properties.yAxis[].field` re-points nothing.
12+
13+
**Behaviour change users see:** the three presentation positions — report `chart.series[].name`, page-component `properties.series[].name` and `properties.yAxis[].field` — drop from `error` to `warning`. A build or a metadata publish that used to be refused because of one of them now succeeds, with the finding on the advisory channel. The finding is KEPT, not deleted: the metadata really is wrong — the author wrote a key and believes it is in force. Every query position (report `chart.yAxis`, and `values[]` on all three surfaces) keeps `error` and its existing message verbatim.
14+
15+
Two smaller corrections ride along, both from the same read:
16+
17+
- The page surface's `yAxis[].field` refs are no longer concatenated into the `series[]` limb before the measure walk, so an axis position no longer takes the series message. Reading both shapes on that surface stays deliberate; giving them one sentence was not.
18+
- `chart-axis-not-selected` (a declared measure outside the selection) took the same one-size consequence, *"the query does not return it, so the series plots nothing"*. It keeps that wording at a query position and states the real one at a presentation position, where no series is derived for the name in the first place.
19+
20+
Note that none of these three surfaces declares `suppressWarnings` — it is a dashboard-widget key — so the new advisories cannot be individually silenced; the hint says so instead of pointing at a key that does not exist.

packages/lint/src/validate-chart-bindings.test.ts

Lines changed: 299 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -175,6 +175,260 @@ describe('validateChartBindings — report charts', () => {
175175
});
176176
expect(findings).toHaveLength(1);
177177
expect(findings[0].path).toBe('reports[0].chart.series[0].name');
178+
// #15575 — still reported, one tier down: see the block below for why.
179+
expect(findings[0].severity).toBe('warning');
180+
});
181+
});
182+
183+
/**
184+
* #15575 — the per-position tier and consequence, pinned per surface against
185+
* the `@object-ui` revision `.objectui-sha` names. Each title names the
186+
* renderer line that decides it, as the `chart-field-unknown` tests do: the
187+
* tier follows that measurement, and a test that does not name it cannot be
188+
* re-checked when the pin moves.
189+
*/
190+
describe('validateChartBindings — binding vs presentation positions (#15575)', () => {
191+
const reportWith = (chart: Record<string, unknown>) => ({
192+
...baseStack(),
193+
reports: [
194+
{ name: 'r', dataset: 'task_metrics', values: ['task_count'], chart },
195+
],
196+
});
197+
198+
const pageWith = (properties: Record<string, unknown>) => ({
199+
...baseStack(),
200+
pages: [
201+
{
202+
name: 'p',
203+
regions: [
204+
{ name: 'main', components: [{ type: 'object-chart', properties }] },
205+
],
206+
},
207+
],
208+
});
209+
210+
// ── report charts ──────────────────────────────────────────────────────
211+
it('report chart.yAxis stays ERROR — DatasetReportRenderer runs `useDatasetRows(dataset, [xAxis], [yAxis], …)`, i.e. "the embedded chart queries only `chart.xAxis` × `chart.yAxis`"', () => {
212+
const findings = validateChartBindings(
213+
reportWith({ type: 'bar', xAxis: 'status', yAxis: 'estimate_hours' }),
214+
);
215+
expect(findings).toHaveLength(1);
216+
expect(findings[0].rule).toBe(CHART_MEASURE_UNKNOWN);
217+
expect(findings[0].severity).toBe('error');
218+
expect(findings[0].path).toBe('reports[0].chart.yAxis');
219+
expect(findings[0].message).toContain('this series comes back empty');
220+
});
221+
222+
it('report chart.series[].name is WARNING — `mergeAuthoredSeries` pairs an authored entry with the derived series whose key it EQUALS, so "an authored entry naming a measure that is NOT in the dataset selection is ignored"', () => {
223+
const findings = validateChartBindings(
224+
reportWith({
225+
type: 'bar',
226+
xAxis: 'status',
227+
yAxis: 'task_count',
228+
series: [{ name: 'estimate_hours', color: '#f00' }],
229+
}),
230+
);
231+
expect(findings).toHaveLength(1);
232+
expect(findings[0].rule).toBe(CHART_MEASURE_UNKNOWN);
233+
expect(findings[0].severity).toBe('warning');
234+
expect(findings[0].path).toBe('reports[0].chart.series[0].name');
235+
// The consequence the pin actually has — and NOT the one it refutes.
236+
expect(findings[0].message).toContain('DISPLAY-NAME override');
237+
expect(findings[0].message).toContain('lands on nothing');
238+
expect(findings[0].message).not.toContain('comes back empty');
239+
// No surface here carries `suppressWarnings` (dashboard widgets only), and
240+
// the hint says so rather than advertising a key that does not exist.
241+
expect(findings[0].hint).toContain('no `suppressWarnings` key');
242+
});
243+
244+
it('one report chart reports BOTH tiers — the query position gates, the presentation position advises', () => {
245+
const findings = validateChartBindings(
246+
reportWith({
247+
type: 'bar',
248+
xAxis: 'status',
249+
yAxis: 'estimate_hours',
250+
series: [{ name: 'ghost' }],
251+
}),
252+
);
253+
expect(findings.map((f) => [f.path, f.severity])).toEqual([
254+
['reports[0].chart.yAxis', 'error'],
255+
['reports[0].chart.series[0].name', 'warning'],
256+
]);
257+
});
258+
259+
it('chart-axis-not-selected at a report series position drops the query sentence — the chart derives ONE series, from its own `chart.yAxis`', () => {
260+
const findings = validateChartBindings(
261+
reportWith({
262+
type: 'bar',
263+
xAxis: 'status',
264+
yAxis: 'task_count',
265+
series: [{ name: 'est_hours' }],
266+
}),
267+
);
268+
expect(findings).toHaveLength(1);
269+
expect(findings[0].rule).toBe(CHART_AXIS_NOT_SELECTED);
270+
expect(findings[0].severity).toBe('warning');
271+
expect(findings[0].message).toContain('display-name override');
272+
expect(findings[0].message).not.toContain('the query does not return it');
273+
expect(findings[0].hint).toContain('chart.yAxis');
274+
});
275+
276+
it('chart-axis-not-selected at the report yAxis position keeps its wording — that position IS the query', () => {
277+
const findings = validateChartBindings(
278+
reportWith({ type: 'bar', xAxis: 'status', yAxis: 'est_hours' }),
279+
);
280+
expect(findings).toHaveLength(1);
281+
expect(findings[0].rule).toBe(CHART_AXIS_NOT_SELECTED);
282+
expect(findings[0].message).toContain('the query does not return it');
283+
});
284+
285+
// ── list-view charts ───────────────────────────────────────────────────
286+
it('list chart values[] stays ERROR — ObjectView hands `values` to the chart as the dataset measures (`values: vals`), and the series list is synthesised FROM it', () => {
287+
const findings = validateChartBindings({
288+
...baseStack(),
289+
views: [
290+
{
291+
name: 'v',
292+
list: {
293+
chart: {
294+
chartType: 'bar',
295+
dataset: 'task_metrics',
296+
dimensions: ['status'],
297+
values: ['estimate_hours'],
298+
},
299+
},
300+
},
301+
],
302+
});
303+
expect(findings).toHaveLength(1);
304+
expect(findings[0].rule).toBe(CHART_MEASURE_UNKNOWN);
305+
expect(findings[0].severity).toBe('error');
306+
expect(findings[0].path).toBe('views[0].list.chart.values[0]');
307+
});
308+
309+
it('a list chart has NO presentation position at all — `ListChartConfigSchema` is a strict object of chartType/dataset/dimensions/values, so a stray `series` key is not this rule to report', () => {
310+
const findings = validateChartBindings({
311+
...baseStack(),
312+
views: [
313+
{
314+
name: 'v',
315+
list: {
316+
chart: {
317+
chartType: 'bar',
318+
dataset: 'task_metrics',
319+
dimensions: ['status'],
320+
values: ['task_count'],
321+
// Not declared by the schema — the strict parse refuses it, and
322+
// this rule stays silent rather than inventing a second verdict.
323+
series: [{ name: 'ghost_measure' }],
324+
},
325+
},
326+
},
327+
],
328+
});
329+
expect(findings).toEqual([]);
330+
});
331+
332+
// ── dataset-bound page chart components ────────────────────────────────
333+
it('page component series[].name is WARNING — ObjectChart REPLACES the authored array (`series: datasetChart.series`, one entry per selected measure)', () => {
334+
const findings = validateChartBindings(
335+
pageWith({
336+
dataset: 'task_metrics',
337+
dimensions: ['status'],
338+
values: ['task_count'],
339+
series: [{ name: 'ghost_measure', stack: 'a' }],
340+
}),
341+
);
342+
expect(findings).toHaveLength(1);
343+
expect(findings[0].rule).toBe(CHART_MEASURE_UNKNOWN);
344+
expect(findings[0].severity).toBe('warning');
345+
expect(findings[0].path).toBe('pages[0].regions[0].components[0].properties.series[0].name');
346+
expect(findings[0].message).toContain('REPLACES the authored array');
347+
expect(findings[0].message).not.toContain('comes back empty');
348+
});
349+
350+
it('page component yAxis[].field takes the AXIS sentence, not the series one — the two limbs no longer share a message', () => {
351+
const findings = validateChartBindings(
352+
pageWith({
353+
dataset: 'task_metrics',
354+
dimensions: ['status'],
355+
values: ['task_count'],
356+
yAxis: [{ field: 'ghost_measure', stepSize: 1 }],
357+
}),
358+
);
359+
expect(findings).toHaveLength(1);
360+
expect(findings[0].rule).toBe(CHART_MEASURE_UNKNOWN);
361+
expect(findings[0].severity).toBe('warning');
362+
expect(findings[0].path).toBe('pages[0].regions[0].components[0].properties.yAxis[0].field');
363+
expect(findings[0].message).toContain('axis PRESENTATION');
364+
expect(findings[0].message).toContain('the plotted columns come from `values`');
365+
expect(findings[0].message).not.toContain('REPLACES the authored array');
366+
});
367+
368+
it('page component values[] stays ERROR — ObjectChart queries `{ dimensions: schema.dimensions, measures: schema.values }`', () => {
369+
const findings = validateChartBindings(
370+
pageWith({
371+
dataset: 'task_metrics',
372+
dimensions: ['status'],
373+
values: ['estimate_hours'],
374+
}),
375+
);
376+
expect(findings).toHaveLength(1);
377+
expect(findings[0].rule).toBe(CHART_MEASURE_UNKNOWN);
378+
expect(findings[0].severity).toBe('error');
379+
expect(findings[0].path).toBe('pages[0].regions[0].components[0].properties.values[0]');
380+
expect(findings[0].message).toContain('this series comes back empty');
381+
});
382+
383+
it('chart-axis-not-selected on the page surface names the derivation, per limb', () => {
384+
const findings = validateChartBindings(
385+
pageWith({
386+
dataset: 'task_metrics',
387+
dimensions: ['status'],
388+
values: ['task_count'],
389+
yAxis: [{ field: 'est_hours' }],
390+
series: [{ name: 'est_hours' }],
391+
}),
392+
);
393+
expect(findings.map((f) => [f.path, f.rule, f.severity])).toEqual([
394+
[
395+
'pages[0].regions[0].components[0].properties.yAxis[0].field',
396+
CHART_AXIS_NOT_SELECTED,
397+
'warning',
398+
],
399+
[
400+
'pages[0].regions[0].components[0].properties.series[0].name',
401+
CHART_AXIS_NOT_SELECTED,
402+
'warning',
403+
],
404+
]);
405+
expect(findings[0].message).toContain('the axis entry re-points nothing');
406+
expect(findings[1].message).toContain('the series are derived from `values`');
407+
expect(findings[0].message).not.toContain('the query does not return it');
408+
expect(findings[1].message).not.toContain('the query does not return it');
409+
});
410+
411+
it('the advisory positions never gate — every presentation finding this rule can raise is below `error`', () => {
412+
const findings = validateChartBindings(
413+
pageWith({
414+
dataset: 'task_metrics',
415+
dimensions: ['status'],
416+
values: ['task_count'],
417+
yAxis: [{ field: 'ghost_a' }],
418+
series: [{ name: 'ghost_b' }],
419+
}),
420+
).concat(
421+
validateChartBindings(
422+
reportWith({
423+
type: 'bar',
424+
xAxis: 'status',
425+
yAxis: 'task_count',
426+
series: [{ name: 'ghost_c' }],
427+
}),
428+
),
429+
);
430+
expect(findings).toHaveLength(3);
431+
expect(findings.every((f) => f.severity === 'warning')).toBe(true);
178432
});
179433
});
180434

@@ -281,6 +535,8 @@ describe('validateChartBindings — dataset-bound page chart components', () =>
281535
expect(findings[0].path).toBe(
282536
'pages[0].regions[0].components[0].properties.yAxis[0].field',
283537
);
538+
// #15575 — presentation on this surface, so advisory rather than gating.
539+
expect(findings[0].severity).toBe('warning');
284540
});
285541

286542
it('accepts a resolved page chart', () => {
@@ -353,6 +609,49 @@ describe('validateChartBindings — floor', () => {
353609
expect(findings).toEqual([]);
354610
});
355611

612+
// The #15636 seam this file carries: its collection reader was a hand-copied
613+
// `asArray` whose array branch was an unchecked cast, so a junk member was
614+
// DEREFERENCED (`strName(entry.name)` on `null`) rather than skipped and the
615+
// rule threw where it should have reported. Now `recordsOf`, which filters.
616+
it('survives a null member in every collection it reads — skipped, not dereferenced', () => {
617+
const stack = {
618+
datasets: [
619+
null,
620+
{
621+
name: 'task_metrics',
622+
object: 'showcase_task',
623+
dimensions: [null, { name: 'status', field: 'status' }],
624+
measures: [null, { name: 'task_count', aggregate: 'count' }],
625+
},
626+
],
627+
reports: [
628+
null,
629+
{
630+
name: 'r',
631+
dataset: 'task_metrics',
632+
values: ['task_count'],
633+
chart: {
634+
type: 'bar',
635+
xAxis: 'status',
636+
yAxis: 'task_count',
637+
series: [null, { name: 'ghost_measure' }],
638+
},
639+
},
640+
],
641+
} as unknown as Record<string, unknown>;
642+
643+
const findings = validateChartBindings(stack);
644+
// The junk members are gone rather than fatal, and the real declarations
645+
// around them still resolve: `status`/`task_count` are found (no unknown-
646+
// ref finding for either), and the one genuine defect is still reported.
647+
expect(findings).toHaveLength(1);
648+
expect(findings[0].rule).toBe(CHART_MEASURE_UNKNOWN);
649+
expect(findings[0].severity).toBe('warning');
650+
// Positions are indexes into the FILTERED collection — a dropped member
651+
// shifts them, which is the honest price of not crashing on junk.
652+
expect(findings[0].path).toBe('reports[0].chart.series[0].name');
653+
});
654+
356655
it('does not mistake a tree view named "org_chart" for a chart', () => {
357656
const findings = validateChartBindings({
358657
...baseStack(),

0 commit comments

Comments
 (0)