Skip to content

Commit 57326a9

Browse files
committed
fix(dashboard): auto-default --limit for grouped widgets (CLI-WW)
The Sentry API rejects grouped widgets without a limit. Previously the CLI threw `ValidationError` when users passed `--group-by` without `--limit`, forcing them to read the error and retry. The Sentry UI defaults to 5 in this case, so the CLI now applies the same default transparently and emits an `[info]` nudge so users understand what happened. Both `dashboard widget add` and `dashboard widget edit` share the same `applyGroupLimitAutoDefault()` helper and `DEFAULT_GROUP_BY_LIMIT` constant (`src/commands/dashboard/resolve.ts`). The predicate only fires when the user *explicitly* passes `--group-by` — auto-defaulted columns like `["issue"]` on `dataset=issue display=table` are left alone, since those widgets legitimately work without a limit. Removed the now-dead `validateGroupByRequiresLimit` helper and its imports. Added unit tests for both helpers plus integration tests in `add.test.ts` and `edit.test.ts` covering: auto-default fires; explicit `--limit` wins; ungrouped widgets don't get a limit; existing widget limit is preserved when adding `--group-by`. Based on work in #784 by @cursor; split out per review.
1 parent cde2c0e commit 57326a9

5 files changed

Lines changed: 308 additions & 46 deletions

File tree

src/commands/dashboard/resolve.ts

Lines changed: 63 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -339,27 +339,74 @@ export function validateSortReferencesAggregate(
339339
}
340340

341341
/**
342-
* Validate that grouped widgets (those with columns/group-by) include a limit.
343-
* The Sentry API rejects grouped widgets without a limit.
342+
* Default limit for grouped widgets.
344343
*
345-
* @param columns - Group-by columns
346-
* @param limit - Widget limit (undefined if not set)
344+
* Matches the Sentry UI default for grouped widgets. The Sentry API rejects
345+
* grouped widgets without a limit, so the CLI transparently applies this
346+
* value when the user passes --group-by without --limit instead of erroring.
347347
*/
348-
export function validateGroupByRequiresLimit(
348+
export const DEFAULT_GROUP_BY_LIMIT = 5;
349+
350+
/**
351+
* Compute the limit to use for a grouped widget.
352+
*
353+
* Returns the provided limit when set. When the widget has group-by columns
354+
* and no limit, returns `DEFAULT_GROUP_BY_LIMIT` so the API accepts the
355+
* widget. Otherwise returns `undefined` (ungrouped widgets don't need a limit).
356+
*
357+
* Callers should `log.info` when the auto-default is applied so users
358+
* understand why their request accepted without an explicit limit.
359+
*
360+
* @param columns - Group-by columns (empty for ungrouped widgets)
361+
* @param limit - User-provided or existing limit (undefined/null if unset)
362+
* @returns Effective limit, or `undefined` when no limit is needed
363+
*/
364+
export function autoDefaultGroupLimit(
349365
columns: string[],
350-
limit: number | undefined
351-
): void {
352-
if (columns.length > 0 && limit === undefined) {
353-
throw new ValidationError(
354-
"Widgets with --group-by require --limit. " +
355-
"Add --limit <n> to specify the maximum number of groups to display.",
356-
"limit"
357-
);
366+
limit: number | null | undefined
367+
): number | undefined {
368+
if (limit !== undefined && limit !== null) {
369+
return limit;
358370
}
371+
if (columns.length > 0) {
372+
return DEFAULT_GROUP_BY_LIMIT;
373+
}
374+
return;
359375
}
360376

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

379+
/**
380+
* Apply the group-by limit auto-default for a user-initiated --group-by.
381+
*
382+
* Only fires when the user explicitly passed --group-by (not for auto-defaulted
383+
* columns like the `["issue"]` default for issue-dataset tables — those widgets
384+
* work without a limit). Emits `log.info` when the auto-default takes effect so
385+
* users see why their command accepted without an explicit --limit.
386+
*
387+
* @param userGroupBy - The raw --group-by flag (undefined when not passed)
388+
* @param columns - Effective columns after any defaulting
389+
* @param limit - User-provided or existing limit (undefined/null if unset)
390+
* @returns Effective limit to use in the widget payload
391+
*/
392+
export function applyGroupLimitAutoDefault(
393+
userGroupBy: string[] | undefined,
394+
columns: string[],
395+
limit: number | null | undefined
396+
): number | null | undefined {
397+
if (!userGroupBy || userGroupBy.length === 0) {
398+
return limit;
399+
}
400+
const effective = autoDefaultGroupLimit(columns, limit);
401+
if (effective !== limit && effective !== undefined) {
402+
log.info(
403+
`Auto-defaulting --limit to ${DEFAULT_GROUP_BY_LIMIT} for grouped widget. Pass --limit <n> to override.`
404+
);
405+
return effective;
406+
}
407+
return limit;
408+
}
409+
363410
/**
364411
* Known aggregatable fields for the spans dataset.
365412
*
@@ -473,11 +520,8 @@ export function buildWidgetFromFlags(opts: {
473520
orderby = `-${aggregates[0]}`;
474521
}
475522

476-
// Only validate when user explicitly passes --group-by, not for auto-defaulted columns
477-
// (e.g., issue dataset auto-defaults columns to ["issue"] for table display)
478-
if (opts.groupBy) {
479-
validateGroupByRequiresLimit(columns, opts.limit);
480-
}
523+
const limit = applyGroupLimitAutoDefault(opts.groupBy, columns, opts.limit);
524+
481525
if (orderby) {
482526
validateSortReferencesAggregate(orderby, aggregates);
483527
}
@@ -495,7 +539,7 @@ export function buildWidgetFromFlags(opts: {
495539
name: "",
496540
},
497541
],
498-
...(opts.limit !== undefined && { limit: opts.limit }),
542+
...(limit !== undefined && { limit }),
499543
};
500544
return prepareWidgetQueries(parseWidgetInput(raw));
501545
}

src/commands/dashboard/widget/edit.ts

Lines changed: 24 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -27,13 +27,13 @@ import {
2727
type WidgetLayoutFlags,
2828
} from "../../../types/dashboard.js";
2929
import {
30+
applyGroupLimitAutoDefault,
3031
enrichDashboardError,
3132
normalizeDataset,
3233
parseDashboardPositionalArgs,
3334
resolveDashboardId,
3435
resolveOrgFromTarget,
3536
resolveWidgetIndex,
36-
validateGroupByRequiresLimit,
3737
validateSortReferencesAggregate,
3838
validateWidgetEnums,
3939
type WidgetQueryFlags,
@@ -128,35 +128,25 @@ function validateEnumsAndAggregates(
128128
}
129129

130130
/**
131-
* Validate group-by+limit and sort constraints on the effective (merged) widget state.
132-
* Only runs when the user changes query, group-by, or sort — not when preserving
133-
* existing widget state which may predate these validations.
131+
* Validate sort constraints on the effective (merged) widget state.
132+
*
133+
* Only runs when the user explicitly passes --sort — not when merely changing
134+
* --query (which may leave the existing auto-defaulted sort stale), and not
135+
* when preserving existing widget state which may predate these validations.
134136
*/
135137
function validateQueryConstraints(
136138
flags: EditFlags,
137139
existing: DashboardWidget,
138-
mergedQueries: DashboardWidgetQuery[] | undefined,
139-
limit: number | null | undefined
140+
mergedQueries: DashboardWidgetQuery[] | undefined
140141
): void {
141-
// Only validate when user explicitly passes --group-by, not when merely
142-
// changing --query on an existing grouped widget (which may have auto-defaulted
143-
// columns like ["issue"] with no limit)
144-
if (flags["group-by"]) {
145-
const columns =
146-
mergedQueries?.[0]?.columns ?? existing.queries?.[0]?.columns ?? [];
147-
validateGroupByRequiresLimit(columns, limit ?? undefined);
142+
if (!flags.sort) {
143+
return;
148144
}
149-
150-
// Only validate sort when user explicitly passes --sort, not when merely
151-
// changing --query (which may leave the existing auto-defaulted sort stale)
152-
if (flags.sort) {
153-
const orderby =
154-
mergedQueries?.[0]?.orderby ?? existing.queries?.[0]?.orderby;
155-
const aggregates =
156-
mergedQueries?.[0]?.aggregates ?? existing.queries?.[0]?.aggregates ?? [];
157-
if (orderby && aggregates.length > 0) {
158-
validateSortReferencesAggregate(orderby, aggregates);
159-
}
145+
const orderby = mergedQueries?.[0]?.orderby ?? existing.queries?.[0]?.orderby;
146+
const aggregates =
147+
mergedQueries?.[0]?.aggregates ?? existing.queries?.[0]?.aggregates ?? [];
148+
if (orderby && aggregates.length > 0) {
149+
validateSortReferencesAggregate(orderby, aggregates);
160150
}
161151
}
162152

@@ -166,10 +156,17 @@ function buildReplacement(
166156
existing: DashboardWidget
167157
): DashboardWidget {
168158
const mergedQueries = mergeQueries(flags, existing.queries?.[0]);
169-
const limit = flags.limit !== undefined ? flags.limit : existing.limit;
159+
const baseLimit = flags.limit !== undefined ? flags.limit : existing.limit;
160+
const columns =
161+
mergedQueries?.[0]?.columns ?? existing.queries?.[0]?.columns ?? [];
162+
const limit = applyGroupLimitAutoDefault(
163+
flags["group-by"],
164+
columns,
165+
baseLimit
166+
);
170167

171168
validateEnumsAndAggregates(flags, existing, mergedQueries);
172-
validateQueryConstraints(flags, existing, mergedQueries, limit);
169+
validateQueryConstraints(flags, existing, mergedQueries);
173170

174171
const raw: Record<string, unknown> = {
175172
title: flags["new-title"] ?? existing.title,
@@ -184,7 +181,7 @@ function buildReplacement(
184181
} else if (existing.widgetType) {
185182
raw.widgetType = existing.widgetType;
186183
}
187-
if (limit !== undefined) {
184+
if (limit !== undefined && limit !== null) {
188185
raw.limit = limit;
189186
}
190187

test/commands/dashboard/resolve.test.ts

Lines changed: 57 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,9 @@
77

88
import { afterEach, beforeEach, describe, expect, spyOn, test } from "bun:test";
99
import {
10+
applyGroupLimitAutoDefault,
11+
autoDefaultGroupLimit,
12+
DEFAULT_GROUP_BY_LIMIT,
1013
enrichDashboardError,
1114
normalizeDataset,
1215
parseDashboardListArgs,
@@ -623,3 +626,57 @@ describe("validateWidgetEnums (with normalizeDataset)", () => {
623626
);
624627
});
625628
});
629+
630+
// ---------------------------------------------------------------------------
631+
// autoDefaultGroupLimit / applyGroupLimitAutoDefault
632+
// ---------------------------------------------------------------------------
633+
634+
describe("autoDefaultGroupLimit", () => {
635+
test("returns the provided limit when set", () => {
636+
expect(autoDefaultGroupLimit(["browser.name"], 10)).toBe(10);
637+
});
638+
639+
test("returns DEFAULT_GROUP_BY_LIMIT for grouped widgets with no limit", () => {
640+
expect(autoDefaultGroupLimit(["browser.name"], undefined)).toBe(
641+
DEFAULT_GROUP_BY_LIMIT
642+
);
643+
expect(autoDefaultGroupLimit(["browser.name"], null)).toBe(
644+
DEFAULT_GROUP_BY_LIMIT
645+
);
646+
});
647+
648+
test("returns undefined for ungrouped widgets with no limit", () => {
649+
expect(autoDefaultGroupLimit([], undefined)).toBeUndefined();
650+
expect(autoDefaultGroupLimit([], null)).toBeUndefined();
651+
});
652+
653+
test("preserves explicit limit even for ungrouped widgets", () => {
654+
expect(autoDefaultGroupLimit([], 3)).toBe(3);
655+
});
656+
});
657+
658+
describe("applyGroupLimitAutoDefault", () => {
659+
test("skips auto-default when --group-by not passed", () => {
660+
// Auto-defaulted columns (e.g., ["issue"] for issue/table) should NOT
661+
// trigger the auto-default limit — caller signals intent via userGroupBy.
662+
expect(
663+
applyGroupLimitAutoDefault(undefined, ["issue"], undefined)
664+
).toBeUndefined();
665+
});
666+
667+
test("applies default when --group-by passed without limit", () => {
668+
expect(
669+
applyGroupLimitAutoDefault(["browser.name"], ["browser.name"], undefined)
670+
).toBe(DEFAULT_GROUP_BY_LIMIT);
671+
});
672+
673+
test("preserves explicit limit", () => {
674+
expect(
675+
applyGroupLimitAutoDefault(["browser.name"], ["browser.name"], 25)
676+
).toBe(25);
677+
});
678+
679+
test("empty --group-by is treated as not passed", () => {
680+
expect(applyGroupLimitAutoDefault([], [], undefined)).toBeUndefined();
681+
});
682+
});

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

Lines changed: 76 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -513,6 +513,82 @@ describe("dashboard widget add", () => {
513513
expect(addedWidget.queries[0].orderby).toBe("-count()");
514514
});
515515

516+
test("auto-defaults --limit to 5 when --group-by is used without --limit", async () => {
517+
const { context } = createMockContext();
518+
const func = await addCommand.loader();
519+
await func.call(
520+
context,
521+
{
522+
json: false,
523+
display: "line",
524+
query: ["count"],
525+
"group-by": ["browser.name"],
526+
},
527+
"123",
528+
"Top Browsers"
529+
);
530+
531+
const body = updateDashboardSpy.mock.calls[0]?.[2];
532+
const addedWidget = body.widgets.at(-1);
533+
expect(addedWidget.limit).toBe(5);
534+
expect(addedWidget.queries[0].columns).toEqual(["browser.name"]);
535+
expect(addedWidget.queries[0].orderby).toBe("-count()");
536+
});
537+
538+
test("explicit --limit wins over auto-default for grouped widgets", async () => {
539+
const { context } = createMockContext();
540+
const func = await addCommand.loader();
541+
await func.call(
542+
context,
543+
{
544+
json: false,
545+
display: "bar",
546+
query: ["count"],
547+
"group-by": ["browser.name"],
548+
limit: 10,
549+
},
550+
"123",
551+
"Top Browsers"
552+
);
553+
554+
const body = updateDashboardSpy.mock.calls[0]?.[2];
555+
const addedWidget = body.widgets.at(-1);
556+
expect(addedWidget.limit).toBe(10);
557+
});
558+
559+
test("ungrouped widget does not get an auto-default limit", async () => {
560+
const { context } = createMockContext();
561+
const func = await addCommand.loader();
562+
await func.call(
563+
context,
564+
{ json: false, display: "big_number", query: ["count"] },
565+
"123",
566+
"Total Count"
567+
);
568+
569+
const body = updateDashboardSpy.mock.calls[0]?.[2];
570+
const addedWidget = body.widgets.at(-1);
571+
expect(addedWidget.limit).toBeUndefined();
572+
});
573+
574+
test("issue-dataset table default columns do NOT trigger auto-default limit", async () => {
575+
// Regression guard: the issue/table combo auto-defaults columns to
576+
// ["issue"] but should not auto-default a limit — existing widgets of
577+
// this shape may legitimately have no limit.
578+
const { context } = createMockContext();
579+
const func = await addCommand.loader();
580+
await func.call(
581+
context,
582+
{ json: false, display: "table", dataset: "issue" },
583+
"123",
584+
"Top Issues"
585+
);
586+
587+
const body = updateDashboardSpy.mock.calls[0]?.[2];
588+
const addedWidget = body.widgets.at(-1);
589+
expect(addedWidget.limit).toBeUndefined();
590+
});
591+
516592
// -- Layout mode flag tests -----------------------------------------------
517593

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

0 commit comments

Comments
 (0)