Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
82 changes: 63 additions & 19 deletions src/commands/dashboard/resolve.ts
Original file line number Diff line number Diff line change
Expand Up @@ -339,27 +339,74 @@ export function validateSortReferencesAggregate(
}

/**
* Validate that grouped widgets (those with columns/group-by) include a limit.
* The Sentry API rejects grouped widgets without a limit.
* Default limit for grouped widgets.
*
* @param columns - Group-by columns
* @param limit - Widget limit (undefined if not set)
* Matches the Sentry UI default for grouped widgets. The Sentry API rejects
* grouped widgets without a limit, so the CLI transparently applies this
* value when the user passes --group-by without --limit instead of erroring.
*/
export function validateGroupByRequiresLimit(
export const DEFAULT_GROUP_BY_LIMIT = 5;

/**
* Compute the limit to use for a grouped widget.
*
* Returns the provided limit when set. When the widget has group-by columns
* and no limit, returns `DEFAULT_GROUP_BY_LIMIT` so the API accepts the
* widget. Otherwise returns `undefined` (ungrouped widgets don't need a limit).
*
* Callers should `log.info` when the auto-default is applied so users
* understand why their request accepted without an explicit limit.
*
* @param columns - Group-by columns (empty for ungrouped widgets)
* @param limit - User-provided or existing limit (undefined/null if unset)
* @returns Effective limit, or `undefined` when no limit is needed
*/
export function autoDefaultGroupLimit(
columns: string[],
limit: number | undefined
): void {
if (columns.length > 0 && limit === undefined) {
throw new ValidationError(
"Widgets with --group-by require --limit. " +
"Add --limit <n> to specify the maximum number of groups to display.",
"limit"
);
limit: number | null | undefined
): number | undefined {
if (limit !== undefined && limit !== null) {
return limit;
}
if (columns.length > 0) {
return DEFAULT_GROUP_BY_LIMIT;
}
return;
}

const log = logger.withTag("dashboard");

/**
* Apply the group-by limit auto-default for a user-initiated --group-by.
*
* Only fires when the user explicitly passed --group-by (not for auto-defaulted
* columns like the `["issue"]` default for issue-dataset tables — those widgets
* work without a limit). Emits `log.info` when the auto-default takes effect so
* users see why their command accepted without an explicit --limit.
*
* @param userGroupBy - The raw --group-by flag (undefined when not passed)
* @param columns - Effective columns after any defaulting
* @param limit - User-provided or existing limit (undefined/null if unset)
* @returns Effective limit to use in the widget payload
*/
export function applyGroupLimitAutoDefault(
userGroupBy: string[] | undefined,
columns: string[],
limit: number | null | undefined
): number | null | undefined {
if (!userGroupBy || userGroupBy.length === 0) {
return limit;
}
const effective = autoDefaultGroupLimit(columns, limit);
if (effective !== limit && effective !== undefined) {
log.info(
`Auto-defaulting --limit to ${DEFAULT_GROUP_BY_LIMIT} for grouped widget. Pass --limit <n> to override.`
);
return effective;
}
return limit;
}

/**
* Known aggregatable fields for the spans dataset.
*
Expand Down Expand Up @@ -473,11 +520,8 @@ export function buildWidgetFromFlags(opts: {
orderby = `-${aggregates[0]}`;
}

// Only validate when user explicitly passes --group-by, not for auto-defaulted columns
// (e.g., issue dataset auto-defaults columns to ["issue"] for table display)
if (opts.groupBy) {
validateGroupByRequiresLimit(columns, opts.limit);
}
const limit = applyGroupLimitAutoDefault(opts.groupBy, columns, opts.limit);

if (orderby) {
validateSortReferencesAggregate(orderby, aggregates);
}
Expand All @@ -495,7 +539,7 @@ export function buildWidgetFromFlags(opts: {
name: "",
},
],
...(opts.limit !== undefined && { limit: opts.limit }),
...(limit !== undefined && { limit }),
};
return prepareWidgetQueries(parseWidgetInput(raw));
}
Expand Down
51 changes: 24 additions & 27 deletions src/commands/dashboard/widget/edit.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,13 +27,13 @@ import {
type WidgetLayoutFlags,
} from "../../../types/dashboard.js";
import {
applyGroupLimitAutoDefault,
enrichDashboardError,
normalizeDataset,
parseDashboardPositionalArgs,
resolveDashboardId,
resolveOrgFromTarget,
resolveWidgetIndex,
validateGroupByRequiresLimit,
validateSortReferencesAggregate,
validateWidgetEnums,
type WidgetQueryFlags,
Expand Down Expand Up @@ -128,35 +128,25 @@ function validateEnumsAndAggregates(
}

/**
* Validate group-by+limit and sort constraints on the effective (merged) widget state.
* Only runs when the user changes query, group-by, or sort — not when preserving
* existing widget state which may predate these validations.
* Validate sort constraints on the effective (merged) widget state.
*
* Only runs when the user explicitly passes --sort — not when merely changing
* --query (which may leave the existing auto-defaulted sort stale), and not
* when preserving existing widget state which may predate these validations.
*/
function validateQueryConstraints(
flags: EditFlags,
existing: DashboardWidget,
mergedQueries: DashboardWidgetQuery[] | undefined,
limit: number | null | undefined
mergedQueries: DashboardWidgetQuery[] | undefined
): void {
// Only validate when user explicitly passes --group-by, not when merely
// changing --query on an existing grouped widget (which may have auto-defaulted
// columns like ["issue"] with no limit)
if (flags["group-by"]) {
const columns =
mergedQueries?.[0]?.columns ?? existing.queries?.[0]?.columns ?? [];
validateGroupByRequiresLimit(columns, limit ?? undefined);
if (!flags.sort) {
return;
}

// Only validate sort when user explicitly passes --sort, not when merely
// changing --query (which may leave the existing auto-defaulted sort stale)
if (flags.sort) {
const orderby =
mergedQueries?.[0]?.orderby ?? existing.queries?.[0]?.orderby;
const aggregates =
mergedQueries?.[0]?.aggregates ?? existing.queries?.[0]?.aggregates ?? [];
if (orderby && aggregates.length > 0) {
validateSortReferencesAggregate(orderby, aggregates);
}
const orderby = mergedQueries?.[0]?.orderby ?? existing.queries?.[0]?.orderby;
const aggregates =
mergedQueries?.[0]?.aggregates ?? existing.queries?.[0]?.aggregates ?? [];
if (orderby && aggregates.length > 0) {
validateSortReferencesAggregate(orderby, aggregates);
}
}

Expand All @@ -166,10 +156,17 @@ function buildReplacement(
existing: DashboardWidget
): DashboardWidget {
const mergedQueries = mergeQueries(flags, existing.queries?.[0]);
const limit = flags.limit !== undefined ? flags.limit : existing.limit;
const baseLimit = flags.limit !== undefined ? flags.limit : existing.limit;
const columns =
mergedQueries?.[0]?.columns ?? existing.queries?.[0]?.columns ?? [];
const limit = applyGroupLimitAutoDefault(
flags["group-by"],
columns,
baseLimit
);

validateEnumsAndAggregates(flags, existing, mergedQueries);
validateQueryConstraints(flags, existing, mergedQueries, limit);
validateQueryConstraints(flags, existing, mergedQueries);

const raw: Record<string, unknown> = {
title: flags["new-title"] ?? existing.title,
Expand All @@ -184,7 +181,7 @@ function buildReplacement(
} else if (existing.widgetType) {
raw.widgetType = existing.widgetType;
}
if (limit !== undefined) {
if (limit !== undefined && limit !== null) {
raw.limit = limit;
}

Expand Down
57 changes: 57 additions & 0 deletions test/commands/dashboard/resolve.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,9 @@

import { afterEach, beforeEach, describe, expect, spyOn, test } from "bun:test";
import {
applyGroupLimitAutoDefault,
autoDefaultGroupLimit,
DEFAULT_GROUP_BY_LIMIT,
enrichDashboardError,
normalizeDataset,
parseDashboardListArgs,
Expand Down Expand Up @@ -623,3 +626,57 @@ describe("validateWidgetEnums (with normalizeDataset)", () => {
);
});
});

// ---------------------------------------------------------------------------
// autoDefaultGroupLimit / applyGroupLimitAutoDefault
// ---------------------------------------------------------------------------

describe("autoDefaultGroupLimit", () => {
test("returns the provided limit when set", () => {
expect(autoDefaultGroupLimit(["browser.name"], 10)).toBe(10);
});

test("returns DEFAULT_GROUP_BY_LIMIT for grouped widgets with no limit", () => {
expect(autoDefaultGroupLimit(["browser.name"], undefined)).toBe(
DEFAULT_GROUP_BY_LIMIT
);
expect(autoDefaultGroupLimit(["browser.name"], null)).toBe(
DEFAULT_GROUP_BY_LIMIT
);
});

test("returns undefined for ungrouped widgets with no limit", () => {
expect(autoDefaultGroupLimit([], undefined)).toBeUndefined();
expect(autoDefaultGroupLimit([], null)).toBeUndefined();
});

test("preserves explicit limit even for ungrouped widgets", () => {
expect(autoDefaultGroupLimit([], 3)).toBe(3);
});
});

describe("applyGroupLimitAutoDefault", () => {
test("skips auto-default when --group-by not passed", () => {
// Auto-defaulted columns (e.g., ["issue"] for issue/table) should NOT
// trigger the auto-default limit — caller signals intent via userGroupBy.
expect(
applyGroupLimitAutoDefault(undefined, ["issue"], undefined)
).toBeUndefined();
});

test("applies default when --group-by passed without limit", () => {
expect(
applyGroupLimitAutoDefault(["browser.name"], ["browser.name"], undefined)
).toBe(DEFAULT_GROUP_BY_LIMIT);
});

test("preserves explicit limit", () => {
expect(
applyGroupLimitAutoDefault(["browser.name"], ["browser.name"], 25)
).toBe(25);
});

test("empty --group-by is treated as not passed", () => {
expect(applyGroupLimitAutoDefault([], [], undefined)).toBeUndefined();
});
});
76 changes: 76 additions & 0 deletions test/commands/dashboard/widget/add.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -513,6 +513,82 @@ describe("dashboard widget add", () => {
expect(addedWidget.queries[0].orderby).toBe("-count()");
});

test("auto-defaults --limit to 5 when --group-by is used without --limit", async () => {
const { context } = createMockContext();
const func = await addCommand.loader();
await func.call(
context,
{
json: false,
display: "line",
query: ["count"],
"group-by": ["browser.name"],
},
"123",
"Top Browsers"
);

const body = updateDashboardSpy.mock.calls[0]?.[2];
const addedWidget = body.widgets.at(-1);
expect(addedWidget.limit).toBe(5);
expect(addedWidget.queries[0].columns).toEqual(["browser.name"]);
expect(addedWidget.queries[0].orderby).toBe("-count()");
});

test("explicit --limit wins over auto-default for grouped widgets", async () => {
const { context } = createMockContext();
const func = await addCommand.loader();
await func.call(
context,
{
json: false,
display: "bar",
query: ["count"],
"group-by": ["browser.name"],
limit: 10,
},
"123",
"Top Browsers"
);

const body = updateDashboardSpy.mock.calls[0]?.[2];
const addedWidget = body.widgets.at(-1);
expect(addedWidget.limit).toBe(10);
});

test("ungrouped widget does not get an auto-default limit", async () => {
const { context } = createMockContext();
const func = await addCommand.loader();
await func.call(
context,
{ json: false, display: "big_number", query: ["count"] },
"123",
"Total Count"
);

const body = updateDashboardSpy.mock.calls[0]?.[2];
const addedWidget = body.widgets.at(-1);
expect(addedWidget.limit).toBeUndefined();
});

test("issue-dataset table default columns do NOT trigger auto-default limit", async () => {
// Regression guard: the issue/table combo auto-defaults columns to
// ["issue"] but should not auto-default a limit — existing widgets of
// this shape may legitimately have no limit.
const { context } = createMockContext();
const func = await addCommand.loader();
await func.call(
context,
{ json: false, display: "table", dataset: "issue" },
"123",
"Top Issues"
);

const body = updateDashboardSpy.mock.calls[0]?.[2];
const addedWidget = body.widgets.at(-1);
expect(addedWidget.limit).toBeUndefined();
});

// -- Layout mode flag tests -----------------------------------------------

test("--layout dense passes dense mode to auto-placer", async () => {
Expand Down
Loading
Loading