Skip to content

Commit d83d079

Browse files
baozhoutaoclaude
andauthored
fix(lint): validateChartBindings resolves a report's dataset and rows/columns whether or not it has a chart (#16397)
* wip(lint): lift report dataset resolution above the chart branch Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Vbw3RPgdtqesx4azk9SbW8 * fix(lint): validateChartBindings resolves a report's dataset and rows/columns whether or not it has a chart `validateChartBindings` reached a report through one closure whose first line was `if (!isRec(chart)) return`, and that closure was the only site that ever received `report.dataset`. A report authored without a chart was therefore not checked at all, and `rows` / `columns` were never resolved against the dataset on any report — on one and the same report object the measure selection was validated and the dimension selection beside it was not. Dataset resolution is lifted out of the chart closure into `resolveDataset`, called once per report and once per block before the chart question is asked. The resolved dataset is then fed to two groups of positions: the report's own selection (`rows` / `columns` -> `chart-dimension-unknown`, `values` -> `chart-measure-unknown`) and, when a chart is present, its axis refs exactly as before. One path entered unconditionally, not a second pass after the early return, so an unresolvable dataset is still exactly one finding. No new rule id and no severity moved. The report dataset finding now points at `reports[i].dataset` rather than the position `reports[i].chart.dataset`, which a report does not have, and its sentence no longer names a chart the report may not draw. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Vbw3RPgdtqesx4azk9SbW8 --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent 000fd05 commit d83d079

3 files changed

Lines changed: 358 additions & 56 deletions

File tree

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,19 @@
1+
---
2+
"@objectstack/lint": patch
3+
---
4+
5+
A report's `dataset`, `rows`, `columns` and `values` are checked whether or not the report draws a chart (#16105)
6+
7+
**Behaviour change — new findings on reports that used to publish clean.** `validateChartBindings` reached a report through one closure that opened `if (!isRec(chart)) return`, and that closure was the only place a report's `dataset` was ever passed to the resolver. Two gaps followed, and both are closed:
8+
9+
- **A report authored without a `chart` was not checked at all.** Bind it to a dataset that does not exist and `os lint` exited 0 and named nothing. It now reports `chart-dataset-unknown` at `error`, the same as a charted report always did.
10+
- **`rows` and `columns` were resolved on no report, charted or not.** On one and the same report object the measure selection (`values`) was resolved against the dataset and the dimension selection beside it was not. Both now report `chart-dimension-unknown` at `error` for a name the bound dataset does not declare as a dimension, at `reports[i].rows[j]` / `reports[i].columns[j]`. A chartless report's `values` is resolved for the first time too, under the existing `chart-measure-unknown`.
11+
12+
`ReportSchema` is what makes these bindings rather than free text: it requires `dataset` + `values` on every non-`joined` report, and declares `rows` (the down axis) and `columns` (the across axis a `matrix` pivots on, ADR-0021 D2) as dimension names taken from that dataset. The chart is optional decoration on top of a binding the report already has. So a report bound to a missing dataset, or grouping on a dimension its dataset does not declare, now fails authoring instead of rendering blank or mis-grouped in production.
13+
14+
No new rule id, no severity moved, and the charted path is unchanged — `chart-axis-not-selected` stays a `warning` and still resolves against the chart's own `chart.yAxis`. Two smaller corrections come with the restructure, both on messages an author reads:
15+
16+
- The dataset finding on a report now points at `reports[i].dataset`, the key the author wrote. It used to say `reports[i].chart.dataset`, a position a report does not have.
17+
- Its sentence ends "there is no data to render" rather than "the chart has no data to render", which is not true of a report that draws no chart.
18+
19+
Blocks of a `joined` report carry the same keys and take the same checks. An unresolvable dataset is still exactly one finding per report or block.

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

Lines changed: 194 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -236,6 +236,193 @@ describe('validateChartBindings — report charts', () => {
236236
});
237237
});
238238

239+
/**
240+
* #16105 — a report is dataset-bound whether or not it draws a chart.
241+
*
242+
* Four pins, mirroring the four injections the card measured on a real app,
243+
* and each reads the findings ARRAY rather than a pass/fail: the tier that
244+
* moved here is `error`, but `chart-axis-not-selected` rides along at
245+
* `warning` and changes no exit code, so a test that only asked "did it fail"
246+
* could not tell the two apart. Two of the four are NEGATIVE controls — the
247+
* behaviour that already worked before the walk was restructured — because the
248+
* failure mode this rule is now exposed to is a later refactor breaking the
249+
* charted path while the new chartless assertions stay green.
250+
*/
251+
describe('validateChartBindings — a report binds a dataset with or without a chart', () => {
252+
/** The card's `pipeline_coverage_by_quarter`: a matrix report, no chart. */
253+
const chartlessMatrix = (over: Record<string, unknown> = {}) => ({
254+
...baseStack(),
255+
reports: [
256+
{
257+
name: 'coverage_by_quarter',
258+
type: 'matrix',
259+
dataset: 'task_metrics',
260+
rows: ['status'],
261+
columns: ['priority'],
262+
values: ['task_count'],
263+
...over,
264+
},
265+
],
266+
});
267+
268+
/** The card's `opportunities_by_stage`: the same binding, plus a chart. */
269+
const chartedReport = (over: Record<string, unknown> = {}) => ({
270+
...baseStack(),
271+
reports: [
272+
{
273+
name: 'hours_by_status',
274+
type: 'summary',
275+
dataset: 'task_metrics',
276+
rows: ['status'],
277+
values: ['est_hours'],
278+
chart: { type: 'bar', xAxis: 'status', yAxis: 'est_hours' },
279+
...over,
280+
},
281+
],
282+
});
283+
284+
// P1 — the card's Fact 1. Before the lift the ONLY site that received
285+
// `report.dataset` was the chart closure, whose first line returned on a
286+
// report with no `chart` key.
287+
it('P1 · resolves a CHARTLESS report\'s dataset — the entrance the chart early-return used to own', () => {
288+
const findings = validateChartBindings(chartlessMatrix({ dataset: 'task_metrics_nope' }));
289+
expect(findings.map((f) => [f.rule, f.path, f.severity])).toEqual([
290+
[CHART_DATASET_UNKNOWN, 'reports[0].dataset', 'error'],
291+
]);
292+
// The path names the key the author wrote. It used to be spelled
293+
// `reports[0].chart.dataset` — a position a report does not have, and one
294+
// a chartless report cannot have at all.
295+
expect(findings[0].hint).toContain('Did you mean "task_metrics"?');
296+
// The dataset is unresolvable, so nothing downstream of it is resolved
297+
// against anything: no dimension or measure finding piles on the typo.
298+
expect(findings).toHaveLength(1);
299+
});
300+
301+
// P2 — the card's Fact 2, on the CHARTED report: on one and the same report
302+
// object the measure selection was resolved and the dimension selection
303+
// beside it was not.
304+
it('P2 · resolves `rows` on a charted report — the selection `values` was resolved without', () => {
305+
const findings = validateChartBindings(chartedReport({ rows: ['status_nope'] }));
306+
expect(findings.map((f) => [f.rule, f.path, f.severity])).toEqual([
307+
[CHART_DIMENSION_UNKNOWN, 'reports[0].rows[0]', 'error'],
308+
]);
309+
expect(findings[0].message).toContain('not a dimension declared by dataset "task_metrics"');
310+
expect(findings[0].hint).toContain('Did you mean "status"?');
311+
});
312+
313+
it('P2 · resolves `columns` — the across axis a `matrix` pivots on (ADR-0021 D2)', () => {
314+
const findings = validateChartBindings(chartlessMatrix({ columns: ['priority_nope'] }));
315+
expect(findings.map((f) => [f.rule, f.path, f.severity])).toEqual([
316+
[CHART_DIMENSION_UNKNOWN, 'reports[0].columns[0]', 'error'],
317+
]);
318+
});
319+
320+
it('P2 · resolves `rows` on a chartless report too — Fact 2 says every report', () => {
321+
const findings = validateChartBindings(chartlessMatrix({ rows: ['status_nope'] }));
322+
expect(findings.map((f) => [f.rule, f.path, f.severity])).toEqual([
323+
[CHART_DIMENSION_UNKNOWN, 'reports[0].rows[0]', 'error'],
324+
]);
325+
});
326+
327+
// The same entrance, one collection down: `values` was resolved only when a
328+
// chart existed, so a chartless report's measure selection was as invisible
329+
// as its dimensions.
330+
it('resolves a chartless report\'s `values` measures', () => {
331+
const findings = validateChartBindings(chartlessMatrix({ values: ['task_count_nope'] }));
332+
expect(findings.map((f) => [f.rule, f.path, f.severity])).toEqual([
333+
[CHART_MEASURE_UNKNOWN, 'reports[0].values[0]', 'error'],
334+
]);
335+
expect(findings[0].message).toContain('this series comes back empty');
336+
});
337+
338+
// Every position of a joined report's block, which declares the same keys.
339+
it('resolves a joined BLOCK\'s dataset and `rows` with no chart on the block', () => {
340+
const findings = validateChartBindings({
341+
...baseStack(),
342+
reports: [
343+
{
344+
name: 'overview',
345+
type: 'joined',
346+
blocks: [
347+
{ name: 'b1', dataset: 'task_metrics', rows: ['status_nope'], values: ['task_count'] },
348+
{ name: 'b2', dataset: 'task_metrics_nope', rows: ['status'], values: ['task_count'] },
349+
],
350+
},
351+
],
352+
});
353+
expect(findings.map((f) => [f.rule, f.path, f.where])).toEqual([
354+
[
355+
CHART_DIMENSION_UNKNOWN,
356+
'reports[0].blocks[0].rows[0]',
357+
'report "overview" · block "b1"',
358+
],
359+
[
360+
CHART_DATASET_UNKNOWN,
361+
'reports[0].blocks[1].dataset',
362+
'report "overview" · block "b2"',
363+
],
364+
]);
365+
});
366+
367+
// N1 — the working control the card ran beside P1: the SAME edit on a report
368+
// that does have a chart. It gated before the lift and must still gate, and
369+
// exactly once: the dataset is resolved per report surface, not per group of
370+
// positions, so restructuring must not double-report the one typo.
371+
it('N1 · a charted report\'s unresolvable dataset still gates — exactly ONE finding, not one per position group', () => {
372+
const findings = validateChartBindings(chartedReport({ dataset: 'task_metrics_nope' }));
373+
expect(findings).toHaveLength(1);
374+
expect(findings[0].rule).toBe(CHART_DATASET_UNKNOWN);
375+
expect(findings[0].severity).toBe('error');
376+
expect(findings[0].path).toBe('reports[0].dataset');
377+
});
378+
379+
// N2 — the card's other working control: the measure selection renamed on a
380+
// charted report. Both tiers, in order, and the tiers are the point: the
381+
// `error` is what an exit code sees, the `warning` rides along and does not
382+
// change it (the card's third implementer note).
383+
it('N2 · a charted report\'s renamed `values` still gates, with the advisory tier riding along unchanged', () => {
384+
const findings = validateChartBindings(
385+
chartedReport({
386+
values: ['est_hours_nope', 'est_hours'],
387+
chart: {
388+
type: 'bar',
389+
xAxis: 'status',
390+
yAxis: 'est_hours',
391+
series: [{ name: 'task_count' }],
392+
},
393+
}),
394+
);
395+
expect(findings.map((f) => [f.rule, f.path, f.severity])).toEqual([
396+
[CHART_MEASURE_UNKNOWN, 'reports[0].values[0]', 'error'],
397+
[CHART_AXIS_NOT_SELECTED, 'reports[0].chart.series[0].name', 'warning'],
398+
]);
399+
expect(findings.filter((f) => f.severity === 'error')).toHaveLength(1);
400+
});
401+
402+
// N3 — the floor on both shapes. A clean report of either kind says nothing,
403+
// which is what makes the four pins above readings rather than noise.
404+
it('N3 · a clean charted report and a clean chartless report both report nothing', () => {
405+
expect(validateChartBindings(chartedReport())).toEqual([]);
406+
expect(validateChartBindings(chartlessMatrix())).toEqual([]);
407+
});
408+
409+
// A `joined` container selects nothing itself (`ReportSchema` puts its data
410+
// on `blocks`), so it binds no dataset and has nothing to resolve.
411+
it('N3 · a joined container with no dataset of its own reports nothing for itself', () => {
412+
const findings = validateChartBindings({
413+
...baseStack(),
414+
reports: [
415+
{
416+
name: 'overview',
417+
type: 'joined',
418+
blocks: [{ name: 'b1', dataset: 'task_metrics', rows: ['status'], values: ['task_count'] }],
419+
},
420+
],
421+
});
422+
expect(findings).toEqual([]);
423+
});
424+
});
425+
239426
/**
240427
* #15575 — the per-position tier and consequence, pinned per surface against
241428
* the `@object-ui` revision `.objectui-sha` names. Each title names the
@@ -735,7 +922,13 @@ describe('validateChartBindings — floor', () => {
735922
expect(validateChartBindings(null as unknown as Record<string, unknown>)).toEqual([]);
736923
});
737924

738-
it('ignores a report with no chart', () => {
925+
// #16105 re-scoped this from "ignores a report with no chart" — a global
926+
// zero that held for the wrong reason. The report below is CLEAN: its
927+
// dataset resolves, `status` is a declared dimension and `task_count` a
928+
// declared measure. The zero is now a reading about this fixture rather than
929+
// about the walk skipping it; the readings about the walk are the pins in
930+
// the "with or without a chart" block above.
931+
it('says nothing about a chartless report whose every binding resolves', () => {
739932
const findings = validateChartBindings({
740933
...baseStack(),
741934
reports: [{ name: 'plain', dataset: 'task_metrics', rows: ['status'], values: ['task_count'] }],

0 commit comments

Comments
 (0)