diff --git a/src/commands/dashboard/resolve.ts b/src/commands/dashboard/resolve.ts index 6887c0d7b..6d6693382 100644 --- a/src/commands/dashboard/resolve.ts +++ b/src/commands/dashboard/resolve.ts @@ -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 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 to override.` + ); + return effective; + } + return limit; +} + /** * Known aggregatable fields for the spans dataset. * @@ -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); } @@ -495,7 +539,7 @@ export function buildWidgetFromFlags(opts: { name: "", }, ], - ...(opts.limit !== undefined && { limit: opts.limit }), + ...(limit !== undefined && { limit }), }; return prepareWidgetQueries(parseWidgetInput(raw)); } diff --git a/src/commands/dashboard/widget/edit.ts b/src/commands/dashboard/widget/edit.ts index 9022dd866..01a4b14e0 100644 --- a/src/commands/dashboard/widget/edit.ts +++ b/src/commands/dashboard/widget/edit.ts @@ -27,13 +27,13 @@ import { type WidgetLayoutFlags, } from "../../../types/dashboard.js"; import { + applyGroupLimitAutoDefault, enrichDashboardError, normalizeDataset, parseDashboardPositionalArgs, resolveDashboardId, resolveOrgFromTarget, resolveWidgetIndex, - validateGroupByRequiresLimit, validateSortReferencesAggregate, validateWidgetEnums, type WidgetQueryFlags, @@ -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); } } @@ -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 = { title: flags["new-title"] ?? existing.title, @@ -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; } diff --git a/test/commands/dashboard/resolve.test.ts b/test/commands/dashboard/resolve.test.ts index bb85a3cee..8ca1cf869 100644 --- a/test/commands/dashboard/resolve.test.ts +++ b/test/commands/dashboard/resolve.test.ts @@ -7,6 +7,9 @@ import { afterEach, beforeEach, describe, expect, spyOn, test } from "bun:test"; import { + applyGroupLimitAutoDefault, + autoDefaultGroupLimit, + DEFAULT_GROUP_BY_LIMIT, enrichDashboardError, normalizeDataset, parseDashboardListArgs, @@ -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(); + }); +}); diff --git a/test/commands/dashboard/widget/add.test.ts b/test/commands/dashboard/widget/add.test.ts index 23ad433a1..978887946 100644 --- a/test/commands/dashboard/widget/add.test.ts +++ b/test/commands/dashboard/widget/add.test.ts @@ -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 () => { diff --git a/test/commands/dashboard/widget/edit.test.ts b/test/commands/dashboard/widget/edit.test.ts index 89f4e3b0f..d3d9c46de 100644 --- a/test/commands/dashboard/widget/edit.test.ts +++ b/test/commands/dashboard/widget/edit.test.ts @@ -465,4 +465,92 @@ describe("dashboard widget edit", () => { const body = updateDashboardSpy.mock.calls[0]?.[2]; expect(body.widgets[0].widgetType).toBe("transaction-like"); }); + + test("auto-defaults --limit to 5 when adding --group-by without --limit", async () => { + const { context } = createMockContext(); + const func = await editCommand.loader(); + + // Widget 0 (Error Count) has no existing limit or group-by. + await func.call( + context, + { + json: false, + index: 0, + display: "line", + "group-by": ["browser.name"], + }, + "123" + ); + + const body = updateDashboardSpy.mock.calls[0]?.[2]; + expect(body.widgets[0].limit).toBe(5); + expect(body.widgets[0].queries[0].columns).toEqual(["browser.name"]); + }); + + test("explicit --limit wins over auto-default when adding --group-by", async () => { + const { context } = createMockContext(); + const func = await editCommand.loader(); + + // line widgets have no row cap; bar/table cap at 10, which would mask the + // explicit-limit signal we want to assert. + await func.call( + context, + { + json: false, + index: 0, + display: "line", + "group-by": ["browser.name"], + limit: 25, + }, + "123" + ); + + const body = updateDashboardSpy.mock.calls[0]?.[2]; + expect(body.widgets[0].limit).toBe(25); + }); + + test("preserves existing limit when adding --group-by to a widget that has one", async () => { + // Seed an existing widget that already has a limit below any display + // cap so clamping doesn't hide the preservation signal. + getDashboardSpy.mockResolvedValueOnce({ + ...sampleDashboard, + widgets: [ + { + ...sampleDashboard.widgets[0], + limit: 8, + }, + ...sampleDashboard.widgets.slice(1), + ], + }); + + const { context } = createMockContext(); + const func = await editCommand.loader(); + await func.call( + context, + { + json: false, + index: 0, + display: "line", + "group-by": ["browser.name"], + }, + "123" + ); + + const body = updateDashboardSpy.mock.calls[0]?.[2]; + expect(body.widgets[0].limit).toBe(8); + }); + + test("does not auto-default limit when only changing --query on an ungrouped widget", async () => { + const { context } = createMockContext(); + const func = await editCommand.loader(); + + await func.call( + context, + { json: false, index: 0, query: ["p95:span.duration"] }, + "123" + ); + + const body = updateDashboardSpy.mock.calls[0]?.[2]; + expect(body.widgets[0].limit).toBeUndefined(); + }); });