Skip to content

Commit a00a739

Browse files
betegonclaudegithub-actions[bot]
authored
fix(dashboard): validate display types against all datasets (#577)
Fixes #535. Fixes #536. Supersedes the incorrect fixes in #570 (merged) and #571 (closed). ## What was wrong The CLI accepted any `--display` + `--dataset` combination and silently saved broken widgets. Three independent issues: 1. **Column default too broad** (#536, partially fixed in #570) — `columns: ["issue"]` was applied for all issue dataset display types, not just `table`. A `--display line --dataset issue` chart got wrong columns. 2. **Issue display type validation** (#535) — only `table/area/line/bar` are valid for the issue dataset but any display type was accepted. 3. **No validation for any other dataset** — `--display table --dataset tracemetrics` (not supported), `--display table --dataset preprod-app-size` (not supported), `--display details --dataset logs` (spans-only), all silently created broken widgets. ## Source of truth All constraints sourced from Sentry's frontend dataset configs at commit [`a42668e`](https://github.com/getsentry/sentry/blob/a42668e/static/app/views/dashboards/datasetConfig/): | Dataset | Supported display types | |---|---| | `issue` | `table`, `area`, `line`, `bar` | | `spans` | `area`, `bar`, `big_number`, `categorical_bar`, `line`, `stacked_area`, `table`, `top_n`, `details`, `server_tree` | | `error-events` / `transaction-like` / `metrics` / `logs` / `discover` | `area`, `bar`, `big_number`, `categorical_bar`, `line`, `stacked_area`, `table`, `top_n` | | `tracemetrics` | `area`, `bar`, `big_number`, `categorical_bar`, `line` | | `preprod-app-size` | `line` | ## What this does - Replaces `ISSUE_DATASET_DISPLAY_TYPES` with `DATASET_SUPPORTED_DISPLAY_TYPES` — a full map for all 9 widget types - `validateWidgetEnums` does one lookup into the map instead of a hardcoded issue check - Column default remains scoped to `issue + table` only ## Test plan All existing tests pass plus new coverage for: - `preprod-app-size + table` → error; `+ line` → ok - `tracemetrics + table` → error - `spans + details` → ok; `logs + details` → error --------- Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
1 parent 762369b commit a00a739

File tree

5 files changed

+351
-5
lines changed

5 files changed

+351
-5
lines changed

src/commands/dashboard/resolve.ts

Lines changed: 23 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@ import { resolveOrg } from "../../lib/resolve-target.js";
1717
import { setOrgProjectContext } from "../../lib/telemetry.js";
1818
import { isAllDigits } from "../../lib/utils.js";
1919
import {
20+
DATASET_SUPPORTED_DISPLAY_TYPES,
2021
type DashboardWidget,
2122
DISPLAY_TYPES,
2223
parseAggregate,
@@ -25,6 +26,7 @@ import {
2526
prepareWidgetQueries,
2627
validateAggregateNames,
2728
WIDGET_TYPES,
29+
type WidgetType,
2830
} from "../../types/dashboard.js";
2931

3032
/** Shared widget query flags used by `add` and `edit` commands */
@@ -325,10 +327,11 @@ export function buildWidgetFromFlags(opts: {
325327
const aggregates = (opts.query ?? ["count"]).map(parseAggregate);
326328
validateAggregateNames(aggregates, opts.dataset);
327329

328-
// The issue dataset requires at least one column (group-by) to produce a
329-
// visible table — without it, the Sentry UI shows "Columns: None".
330-
// Default to ["issue"] so a bare `--dataset issue --display table` just works.
331-
const columns = opts.groupBy ?? (opts.dataset === "issue" ? ["issue"] : []);
330+
// Issue table widgets need at least one column or the Sentry UI shows "Columns: None".
331+
// Default to ["issue"] for table display only — timeseries (line/area/bar) don't use columns.
332+
const columns =
333+
opts.groupBy ??
334+
(opts.dataset === "issue" && opts.display === "table" ? ["issue"] : []);
332335
// Auto-default orderby to first aggregate descending when group-by is used.
333336
// Without this, chart widgets (line/area/bar) with group-by + limit error
334337
// because the dashboard can't determine which top N groups to display.
@@ -380,4 +383,20 @@ export function validateWidgetEnums(display?: string, dataset?: string): void {
380383
"dataset"
381384
);
382385
}
386+
if (display && dataset) {
387+
// Untracked display types (text, wheel, rage_and_dead_clicks, agents_traces_table)
388+
// bypass Sentry's dataset query system entirely — no dataset constraint applies.
389+
const isTrackedDisplay = Object.values(
390+
DATASET_SUPPORTED_DISPLAY_TYPES
391+
).some((types) => (types as readonly string[]).includes(display));
392+
if (isTrackedDisplay) {
393+
const supported = DATASET_SUPPORTED_DISPLAY_TYPES[dataset as WidgetType];
394+
if (supported && !(supported as readonly string[]).includes(display)) {
395+
throw new ValidationError(
396+
`The "${dataset}" dataset supports: ${supported.join(", ")}. Got: "${display}".`,
397+
"display"
398+
);
399+
}
400+
}
401+
}
383402
}

src/commands/dashboard/widget/edit.ts

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -90,9 +90,20 @@ function buildReplacement(
9090

9191
const limit = flags.limit !== undefined ? flags.limit : existing.limit;
9292

93+
const effectiveDisplay = flags.display ?? existing.displayType;
94+
const effectiveDataset = flags.dataset ?? existing.widgetType;
95+
96+
// Re-validate after merging with existing values. validateWidgetEnums only
97+
// checks the cross-constraint when both args are provided, so it misses
98+
// e.g. `--dataset preprod-app-size` on a widget that's already `table`.
99+
// validateWidgetEnums itself skips untracked display types (text, wheel, etc.).
100+
if (flags.display || flags.dataset) {
101+
validateWidgetEnums(effectiveDisplay, effectiveDataset);
102+
}
103+
93104
const raw: Record<string, unknown> = {
94105
title: flags["new-title"] ?? existing.title,
95-
displayType: flags.display ?? existing.displayType,
106+
displayType: effectiveDisplay,
96107
queries: mergedQueries ?? existing.queries,
97108
layout: existing.layout,
98109
};

src/types/dashboard.ts

Lines changed: 95 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -299,6 +299,101 @@ export const DiscoverAggregateFunctionSchema = z.enum(
299299
DISCOVER_AGGREGATE_FUNCTIONS
300300
);
301301

302+
/**
303+
* Valid display types per widget dataset.
304+
*
305+
* Source: sentry/static/app/views/dashboards/datasetConfig/ @ a42668e87cc8a0b7410ac2acecee6074c52f376f
306+
* Each entry mirrors `supportedDisplayTypes` from the corresponding config:
307+
*
308+
* issues.tsx https://github.com/getsentry/sentry/blob/a42668e87cc8a0b7410ac2acecee6074c52f376f/static/app/views/dashboards/datasetConfig/issues.tsx#L90-L95
309+
* spans.tsx https://github.com/getsentry/sentry/blob/a42668e87cc8a0b7410ac2acecee6074c52f376f/static/app/views/dashboards/datasetConfig/spans.tsx#L287-L297
310+
* errors.tsx https://github.com/getsentry/sentry/blob/a42668e87cc8a0b7410ac2acecee6074c52f376f/static/app/views/dashboards/datasetConfig/errors.tsx#L115-L123
311+
* transactions.tsx https://github.com/getsentry/sentry/blob/a42668e87cc8a0b7410ac2acecee6074c52f376f/static/app/views/dashboards/datasetConfig/transactions.tsx#L76-L84
312+
* releases.tsx https://github.com/getsentry/sentry/blob/a42668e87cc8a0b7410ac2acecee6074c52f376f/static/app/views/dashboards/datasetConfig/releases.tsx#L90-L98
313+
* logs.tsx https://github.com/getsentry/sentry/blob/a42668e87cc8a0b7410ac2acecee6074c52f376f/static/app/views/dashboards/datasetConfig/logs.tsx#L201-L209
314+
* traceMetrics.tsx https://github.com/getsentry/sentry/blob/a42668e87cc8a0b7410ac2acecee6074c52f376f/static/app/views/dashboards/datasetConfig/traceMetrics.tsx#L285-L291
315+
* mobileAppSize.tsx https://github.com/getsentry/sentry/blob/a42668e87cc8a0b7410ac2acecee6074c52f376f/static/app/views/dashboards/datasetConfig/mobileAppSize.tsx#L255
316+
*
317+
* stacked_area is included for datasets that support timeseries (not in Sentry's UI picker
318+
* but accepted by the API and handled by the CLI's query engine).
319+
* details and server_tree are spans-only internal display types.
320+
*
321+
* Four display types are intentionally absent from this map — they bypass Sentry's standard
322+
* dataset query system and carry no dataset constraints:
323+
* text — renders markdown; no dataset or query involved; user-editable
324+
* wheel — Web Vitals ring chart; prebuilt dashboards only, not user-editable
325+
* rage_and_dead_clicks — rage/dead click viz; prebuilt only, fetches its own data
326+
* agents_traces_table — AI agent traces; prebuilt only, fetches its own data
327+
* Source: static/app/views/dashboards/utils.tsx (widgetFetchesOwnData / isWidgetEditable)
328+
*/
329+
export const DATASET_SUPPORTED_DISPLAY_TYPES = {
330+
issue: ["table", "area", "line", "bar"],
331+
spans: [
332+
"area",
333+
"bar",
334+
"big_number",
335+
"categorical_bar",
336+
"line",
337+
"stacked_area",
338+
"table",
339+
"top_n",
340+
"details",
341+
"server_tree",
342+
],
343+
"error-events": [
344+
"area",
345+
"bar",
346+
"big_number",
347+
"categorical_bar",
348+
"line",
349+
"stacked_area",
350+
"table",
351+
"top_n",
352+
],
353+
"transaction-like": [
354+
"area",
355+
"bar",
356+
"big_number",
357+
"categorical_bar",
358+
"line",
359+
"stacked_area",
360+
"table",
361+
"top_n",
362+
],
363+
metrics: [
364+
"area",
365+
"bar",
366+
"big_number",
367+
"categorical_bar",
368+
"line",
369+
"stacked_area",
370+
"table",
371+
"top_n",
372+
],
373+
logs: [
374+
"area",
375+
"bar",
376+
"big_number",
377+
"categorical_bar",
378+
"line",
379+
"stacked_area",
380+
"table",
381+
"top_n",
382+
],
383+
discover: [
384+
"area",
385+
"bar",
386+
"big_number",
387+
"categorical_bar",
388+
"line",
389+
"stacked_area",
390+
"table",
391+
"top_n",
392+
],
393+
tracemetrics: ["area", "bar", "big_number", "categorical_bar", "line"],
394+
"preprod-app-size": ["line"],
395+
} as const satisfies Record<WidgetType, readonly string[]>;
396+
302397
/**
303398
* Valid `is:` filter values for issue search conditions (--where flag).
304399
* Only valid when widgetType is "issue". Other datasets don't support `is:`.

test/commands/dashboard/widget/add.test.ts

Lines changed: 122 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -212,6 +212,53 @@ describe("dashboard widget add", () => {
212212
expect(err.message).toContain("Unknown aggregate function");
213213
});
214214

215+
test("throws ValidationError for big_number with issue dataset", async () => {
216+
const { context } = createMockContext();
217+
const func = await addCommand.loader();
218+
219+
const err = await func
220+
.call(
221+
context,
222+
{ json: false, display: "big_number", dataset: "issue" },
223+
"123",
224+
"Unresolved Count"
225+
)
226+
.catch((e: Error) => e);
227+
expect(err).toBeInstanceOf(ValidationError);
228+
expect(err.message).toContain('"issue" dataset supports');
229+
});
230+
231+
test("allows line/area/bar with issue dataset", async () => {
232+
const { context } = createMockContext();
233+
const func = await addCommand.loader();
234+
235+
for (const display of ["line", "area", "bar"]) {
236+
updateDashboardSpy.mockClear();
237+
await func.call(
238+
context,
239+
{ json: false, display, dataset: "issue" },
240+
"123",
241+
"Issues Over Time"
242+
);
243+
expect(updateDashboardSpy).toHaveBeenCalledTimes(1);
244+
}
245+
});
246+
247+
test("issue line dataset does not default columns", async () => {
248+
const { context } = createMockContext();
249+
const func = await addCommand.loader();
250+
await func.call(
251+
context,
252+
{ json: false, display: "line", dataset: "issue" },
253+
"123",
254+
"Issues Over Time"
255+
);
256+
257+
const body = updateDashboardSpy.mock.calls[0]?.[2];
258+
const addedWidget = body.widgets.at(-1);
259+
expect(addedWidget.queries[0].columns).toEqual([]);
260+
});
261+
215262
test("issue dataset defaults columns to ['issue'] and orderby to -count()", async () => {
216263
const { context } = createMockContext();
217264
const func = await addCommand.loader();
@@ -249,6 +296,81 @@ describe("dashboard widget add", () => {
249296
expect(addedWidget.queries[0].columns).toEqual(["project"]);
250297
});
251298

299+
// preprod-app-size: line only
300+
// https://github.com/getsentry/sentry/blob/a42668e/static/app/views/dashboards/datasetConfig/mobileAppSize.tsx#L255
301+
test("throws ValidationError for table with preprod-app-size dataset", async () => {
302+
const { context } = createMockContext();
303+
const func = await addCommand.loader();
304+
const err = await func
305+
.call(
306+
context,
307+
{ json: false, display: "table", dataset: "preprod-app-size" },
308+
"123",
309+
"App Size"
310+
)
311+
.catch((e: Error) => e);
312+
expect(err).toBeInstanceOf(ValidationError);
313+
expect(err.message).toContain('"preprod-app-size" dataset supports');
314+
});
315+
316+
test("allows line with preprod-app-size dataset", async () => {
317+
const { context } = createMockContext();
318+
const func = await addCommand.loader();
319+
await func.call(
320+
context,
321+
{ json: false, display: "line", dataset: "preprod-app-size" },
322+
"123",
323+
"App Size"
324+
);
325+
expect(updateDashboardSpy).toHaveBeenCalledTimes(1);
326+
});
327+
328+
// tracemetrics: no table or top_n
329+
// https://github.com/getsentry/sentry/blob/a42668e/static/app/views/dashboards/datasetConfig/traceMetrics.tsx#L285-L291
330+
test("throws ValidationError for table with tracemetrics dataset", async () => {
331+
const { context } = createMockContext();
332+
const func = await addCommand.loader();
333+
const err = await func
334+
.call(
335+
context,
336+
{ json: false, display: "table", dataset: "tracemetrics" },
337+
"123",
338+
"Trace Metrics"
339+
)
340+
.catch((e: Error) => e);
341+
expect(err).toBeInstanceOf(ValidationError);
342+
expect(err.message).toContain('"tracemetrics" dataset supports');
343+
});
344+
345+
// spans: only dataset supporting details and server_tree
346+
// https://github.com/getsentry/sentry/blob/a42668e/static/app/views/dashboards/datasetConfig/spans.tsx#L287-L297
347+
test("allows details display with spans dataset", async () => {
348+
const { context } = createMockContext();
349+
const func = await addCommand.loader();
350+
await func.call(
351+
context,
352+
{ json: false, display: "details", dataset: "spans" },
353+
"123",
354+
"Span Details"
355+
);
356+
expect(updateDashboardSpy).toHaveBeenCalledTimes(1);
357+
});
358+
359+
test("throws ValidationError for details display with non-spans dataset", async () => {
360+
const { context } = createMockContext();
361+
const func = await addCommand.loader();
362+
const err = await func
363+
.call(
364+
context,
365+
{ json: false, display: "details", dataset: "logs" },
366+
"123",
367+
"Details"
368+
)
369+
.catch((e: Error) => e);
370+
expect(err).toBeInstanceOf(ValidationError);
371+
expect(err.message).toContain('"logs" dataset supports');
372+
});
373+
252374
test("auto-defaults orderby when group-by + limit provided", async () => {
253375
const { context } = createMockContext();
254376
const func = await addCommand.loader();

0 commit comments

Comments
 (0)