diff --git a/plugins/sentry-cli/skills/sentry-cli/references/dashboard.md b/plugins/sentry-cli/skills/sentry-cli/references/dashboard.md index 0d2f32b94..2cfc11b27 100644 --- a/plugins/sentry-cli/skills/sentry-cli/references/dashboard.md +++ b/plugins/sentry-cli/skills/sentry-cli/references/dashboard.md @@ -86,6 +86,7 @@ Add a widget to a dashboard - `--y - Grid row position (0-based)` - `--width - Widget width in grid columns (1–6)` - `--height - Widget height in grid rows (min 1)` +- `-l, --layout - Layout mode: sequential (append in order) or dense (fill gaps) - (default: "sequential")` **Examples:** diff --git a/src/commands/dashboard/widget/add.ts b/src/commands/dashboard/widget/add.ts index b4163dd9f..c7c883d67 100644 --- a/src/commands/dashboard/widget/add.ts +++ b/src/commands/dashboard/widget/add.ts @@ -20,6 +20,7 @@ import { prepareDashboardForUpdate, validateWidgetLayout, type WidgetLayoutFlags, + type WidgetLayoutMode, } from "../../../types/dashboard.js"; import { buildWidgetFromFlags, @@ -34,6 +35,7 @@ import { type AddFlags = WidgetQueryFlags & WidgetLayoutFlags & { readonly display: string; + readonly layout: string; readonly json: boolean; readonly fields?: string[]; }; @@ -178,6 +180,12 @@ export const addCommand = buildCommand({ brief: "Widget height in grid rows (min 1)", optional: true, }, + layout: { + kind: "parsed", + parse: String, + brief: "Layout mode: sequential (append in order) or dense (fill gaps)", + default: "sequential", + }, }, aliases: { d: "display", @@ -186,6 +194,7 @@ export const addCommand = buildCommand({ g: "group-by", s: "sort", n: "limit", + l: "layout", }, }, async *func(this: SentryContext, flags: AddFlags, ...args: string[]) { @@ -217,6 +226,20 @@ export const addCommand = buildCommand({ limit: flags.limit, }); + // Validate layout mode before any network calls + if ( + flags.layout && + flags.layout !== "sequential" && + flags.layout !== "dense" + ) { + throw new ValidationError( + `Invalid --layout mode "${flags.layout}". Valid values: sequential, dense`, + "layout" + ); + } + const layoutMode: WidgetLayoutMode = + flags.layout === "dense" ? "dense" : "sequential"; + // Validate individual layout flag ranges before any network calls // (catches --x -1, --width 7, etc. early without needing the dashboard) validateWidgetLayout(flags); @@ -230,7 +253,7 @@ export const addCommand = buildCommand({ // Always run auto-layout first to get default position and dimensions, // then override with any explicit user flags. - newWidget = assignDefaultLayout(newWidget, updateBody.widgets); + newWidget = assignDefaultLayout(newWidget, updateBody.widgets, layoutMode); const hasExplicitLayout = flags.x !== undefined || diff --git a/src/types/dashboard.ts b/src/types/dashboard.ts index a32d35fac..243693a69 100644 --- a/src/types/dashboard.ts +++ b/src/types/dashboard.ts @@ -609,6 +609,16 @@ export function prepareWidgetQueries( /** Sentry dashboard grid column count */ export const GRID_COLUMNS = 6; +/** + * Controls how the auto-placer positions new widgets in the grid. + * + * - `sequential` — Cursor-based append: place after the last widget, + * wrap to a new row on overflow. Never backfills interior gaps. + * - `dense` — First-fit packing: scan top-to-bottom, left-to-right + * and place in the first available gap. Produces compact layouts. + */ +export type WidgetLayoutMode = "sequential" | "dense"; + /** Default widget dimensions by displayType */ const DEFAULT_WIDGET_SIZE: Partial< Record @@ -671,39 +681,112 @@ function regionFits( return true; } +/** Grid state computed from existing widget layouts */ +type OccupiedGrid = { occupied: Set; maxY: number }; + +/** Widget dimensions resolved from displayType */ +type WidgetSize = { w: number; h: number; minH: number }; + +/** + * Dense (first-fit) placement: scan the grid top-to-bottom, left-to-right + * and place the widget in the first gap where it fits. + */ +function assignLayoutDense( + widget: DashboardWidget, + size: WidgetSize, + grid: OccupiedGrid +): DashboardWidget { + const { w, h, minH } = size; + for (let y = 0; y <= grid.maxY; y++) { + for (let x = 0; x <= GRID_COLUMNS - w; x++) { + if (regionFits(grid.occupied, { px: x, py: y, w, h })) { + return { ...widget, layout: { x, y, w, h, minH } }; + } + } + } + return { ...widget, layout: { x: 0, y: grid.maxY, w, h, minH } }; +} + +/** + * Find the layout of the last widget in the array that has one. + * Reverse-scans because the API preserves insertion order. + */ +function findLastLayout( + widgets: DashboardWidget[] +): DashboardWidgetLayout | undefined { + for (let i = widgets.length - 1; i >= 0; i--) { + const layout = widgets[i]?.layout; + if (layout) { + return layout; + } + } +} + +/** + * Sequential (cursor-based) placement: place the widget immediately to the + * right of the last existing widget on the same row. When the row overflows + * or the position overlaps a manually-placed widget, wrap to a fresh row + * below all existing content. + */ +function assignLayoutSequential( + widget: DashboardWidget, + existingWidgets: DashboardWidget[], + size: WidgetSize, + grid: OccupiedGrid +): DashboardWidget { + const { w, h, minH } = size; + const lastLayout = findLastLayout(existingWidgets); + + if (lastLayout) { + const cursorX = lastLayout.x + lastLayout.w; + const cursorY = lastLayout.y; + + // Place at cursor if it fits within the grid and doesn't overlap + if ( + cursorX + w <= GRID_COLUMNS && + regionFits(grid.occupied, { px: cursorX, py: cursorY, w, h }) + ) { + return { ...widget, layout: { x: cursorX, y: cursorY, w, h, minH } }; + } + } + + // Wrap to a new row below all existing content + return { ...widget, layout: { x: 0, y: grid.maxY, w, h, minH } }; +} + /** * Assign a default layout to a widget if it doesn't already have one. - * Packs the widget into the first available space in a 6-column grid, - * scanning rows top-to-bottom and left-to-right. + * + * Two placement modes are available: + * - `"sequential"` (default) — Cursor-based append: the widget is placed + * immediately after the last existing widget, wrapping to a new row when + * the current row overflows. Interior gaps are never backfilled. + * - `"dense"` — First-fit packing: the widget is placed in the first + * available gap, scanning top-to-bottom and left-to-right. * * @param widget - Widget that may be missing a layout * @param existingWidgets - Widgets already in the dashboard (used to compute placement) + * @param mode - Layout strategy (`"sequential"` or `"dense"`) * @returns Widget with layout guaranteed */ export function assignDefaultLayout( widget: DashboardWidget, - existingWidgets: DashboardWidget[] + existingWidgets: DashboardWidget[], + mode: WidgetLayoutMode = "sequential" ): DashboardWidget { if (widget.layout) { return widget; } - const { w, h, minH } = + const size = DEFAULT_WIDGET_SIZE[widget.displayType as DisplayType] ?? FALLBACK_SIZE; + const grid = buildOccupiedGrid(existingWidgets); - const { occupied, maxY } = buildOccupiedGrid(existingWidgets); - - // Scan rows to find the first position where the widget fits - for (let y = 0; y <= maxY; y++) { - for (let x = 0; x <= GRID_COLUMNS - w; x++) { - if (regionFits(occupied, { px: x, py: y, w, h })) { - return { ...widget, layout: { x, y, w, h, minH } }; - } - } + if (mode === "dense") { + return assignLayoutDense(widget, size, grid); } - // No gap found — place below everything - return { ...widget, layout: { x: 0, y: maxY, w, h, minH } }; + return assignLayoutSequential(widget, existingWidgets, size, grid); } // --------------------------------------------------------------------------- diff --git a/test/commands/dashboard/widget/add.test.ts b/test/commands/dashboard/widget/add.test.ts index 9023835bf..0009456d4 100644 --- a/test/commands/dashboard/widget/add.test.ts +++ b/test/commands/dashboard/widget/add.test.ts @@ -491,4 +491,68 @@ describe("dashboard widget add", () => { const addedWidget = body.widgets.at(-1); expect(addedWidget.queries[0].orderby).toBe("-count()"); }); + + // -- Layout mode flag tests ----------------------------------------------- + + test("--layout dense passes dense mode to auto-placer", async () => { + const { context } = createMockContext(); + const func = await addCommand.loader(); + await func.call( + context, + { json: false, display: "big_number", query: ["count"], layout: "dense" }, + "123", + "Dense Widget" + ); + + const body = updateDashboardSpy.mock.calls[0]?.[2]; + const addedWidget = body.widgets.at(-1); + // Existing widgets: big_number at (0,0,2,1) and table at (2,0,4,2). + // Dense mode finds the first gap. The gap at (0,1) fits a 2x1 big_number. + expect(addedWidget.layout.x).toBe(0); + expect(addedWidget.layout.y).toBe(1); + }); + + test("--layout sequential (default) uses sequential placement", async () => { + const { context } = createMockContext(); + const func = await addCommand.loader(); + await func.call( + context, + { + json: false, + display: "big_number", + query: ["count"], + layout: "sequential", + }, + "123", + "Sequential Widget" + ); + + const body = updateDashboardSpy.mock.calls[0]?.[2]; + const addedWidget = body.widgets.at(-1); + // Existing widgets: big_number at (0,0,2,1) and table at (2,0,4,2). + // Last widget is table at x=2,w=4 → cursor=(6,0) → 6+2=8 > 6 → wrap to (0,2). + expect(addedWidget.layout.x).toBe(0); + expect(addedWidget.layout.y).toBe(2); + }); + + test("--layout invalid rejects with ValidationError", async () => { + const { context } = createMockContext(); + const func = await addCommand.loader(); + const err = await func + .call( + context, + { + json: false, + display: "big_number", + query: ["count"], + layout: "invalid", + }, + "123", + "Bad Layout" + ) + .catch((e: unknown) => e); + + expect(err).toBeInstanceOf(ValidationError); + expect((err as ValidationError).message).toContain("--layout"); + }); }); diff --git a/test/types/dashboard.property.test.ts b/test/types/dashboard.property.test.ts new file mode 100644 index 000000000..7f394dd7c --- /dev/null +++ b/test/types/dashboard.property.test.ts @@ -0,0 +1,129 @@ +/** + * Property-Based Tests for Dashboard Auto-Layout + * + * Uses fast-check to verify invariants that should always hold for + * assignDefaultLayout(), regardless of widget types, counts, or layout mode. + */ + +import { describe, expect, test } from "bun:test"; +import { array, constantFrom, assert as fcAssert, property } from "fast-check"; +import { + assignDefaultLayout, + type DashboardWidget, + DISPLAY_TYPES, + GRID_COLUMNS, + type WidgetLayoutMode, +} from "../../src/types/dashboard.js"; +import { DEFAULT_NUM_RUNS } from "../model-based/helpers.js"; + +// --------------------------------------------------------------------------- +// Arbitraries +// --------------------------------------------------------------------------- + +/** Arbitrary for a display type from the full set */ +const displayTypeArb = constantFrom(...DISPLAY_TYPES); + +/** Arbitrary for a widget without a layout */ +const widgetArb = displayTypeArb.map( + (dt): DashboardWidget => ({ + title: `Widget-${dt}`, + displayType: dt, + }) +); + +/** Arbitrary for layout mode */ +const modeArb = constantFrom("sequential", "dense"); + +/** + * Build a sequence of placed widgets by calling assignDefaultLayout + * repeatedly, simulating sequential `widget add` calls. + */ +function buildPlacedSequence( + widgets: DashboardWidget[], + mode: WidgetLayoutMode +): DashboardWidget[] { + const placed: DashboardWidget[] = []; + for (const w of widgets) { + placed.push(assignDefaultLayout(w, placed, mode)); + } + return placed; +} + +// --------------------------------------------------------------------------- +// Properties +// --------------------------------------------------------------------------- + +describe("property: assignDefaultLayout", () => { + test("always produces a layout within grid bounds (both modes)", () => { + fcAssert( + property( + array(widgetArb, { minLength: 0, maxLength: 20 }), + widgetArb, + modeArb, + (existing, newWidget, mode) => { + const placed = buildPlacedSequence(existing, mode); + const result = assignDefaultLayout(newWidget, placed, mode); + + expect(result.layout).toBeDefined(); + const l = result.layout!; + expect(l.x).toBeGreaterThanOrEqual(0); + expect(l.y).toBeGreaterThanOrEqual(0); + expect(l.x + l.w).toBeLessThanOrEqual(GRID_COLUMNS); + } + ), + { numRuns: DEFAULT_NUM_RUNS } + ); + }); + + test("never overlaps any existing widget (both modes)", () => { + fcAssert( + property( + array(widgetArb, { minLength: 1, maxLength: 15 }), + widgetArb, + modeArb, + (existing, newWidget, mode) => { + const placed = buildPlacedSequence(existing, mode); + const result = assignDefaultLayout(newWidget, placed, mode); + const n = result.layout!; + + for (const p of placed) { + if (!p.layout) { + continue; + } + const overlapX = + n.x < p.layout.x + p.layout.w && n.x + n.w > p.layout.x; + const overlapY = + n.y < p.layout.y + p.layout.h && n.y + n.h > p.layout.y; + expect(overlapX && overlapY).toBe(false); + } + } + ), + { numRuns: DEFAULT_NUM_RUNS } + ); + }); + + test("idempotent — widget with layout is returned unchanged (both modes)", () => { + fcAssert( + property(widgetArb, modeArb, (base, mode) => { + const placed = assignDefaultLayout(base, [], mode); + const again = assignDefaultLayout(placed, [], mode); + expect(again.layout).toEqual(placed.layout); + }), + { numRuns: DEFAULT_NUM_RUNS } + ); + }); + + test("sequential mode: placements have non-decreasing y-coordinate", () => { + fcAssert( + property(array(widgetArb, { minLength: 2, maxLength: 20 }), (widgets) => { + const placed = buildPlacedSequence(widgets, "sequential"); + for (let i = 1; i < placed.length; i++) { + expect(placed[i]!.layout!.y).toBeGreaterThanOrEqual( + placed[i - 1]!.layout!.y + ); + } + }), + { numRuns: DEFAULT_NUM_RUNS } + ); + }); +}); diff --git a/test/types/dashboard.test.ts b/test/types/dashboard.test.ts index 22b110cfb..6bac65048 100644 --- a/test/types/dashboard.test.ts +++ b/test/types/dashboard.test.ts @@ -594,7 +594,7 @@ describe("assignDefaultLayout", () => { expect(result.layout!.h).toBe(1); }); - test("widget in partially filled grid finds first gap", () => { + test("places after last widget on same row (default sequential mode)", () => { const existing: DashboardWidget[] = [ { title: "Existing", @@ -608,10 +608,195 @@ describe("assignDefaultLayout", () => { }; const result = assignDefaultLayout(widget, existing); expect(result.layout).toBeDefined(); - // Should be placed after the existing widget, not overlapping + // Cursor from last widget: (0+2, 0) = (2, 0) expect(result.layout!.x).toBe(2); expect(result.layout!.y).toBe(0); }); + + // -- Sequential mode tests ----------------------------------------------- + + test("sequential: three same-size widgets fill a row left-to-right", () => { + const existing: DashboardWidget[] = [ + { + title: "A", + displayType: "big_number", + layout: { x: 0, y: 0, w: 2, h: 1 }, + }, + { + title: "B", + displayType: "big_number", + layout: { x: 2, y: 0, w: 2, h: 1 }, + }, + ]; + const result = assignDefaultLayout( + { title: "C", displayType: "big_number" }, + existing + ); + expect(result.layout).toMatchObject({ x: 4, y: 0, w: 2, h: 1 }); + }); + + test("sequential: wraps to new row when cursor overflows grid width", () => { + const existing: DashboardWidget[] = [ + { + title: "A", + displayType: "line", + layout: { x: 0, y: 0, w: 3, h: 2 }, + }, + { + title: "B", + displayType: "line", + layout: { x: 3, y: 0, w: 3, h: 2 }, + }, + ]; + const result = assignDefaultLayout( + { title: "C", displayType: "line" }, + existing + ); + // cursor=(6,0) → 6+3=9 > 6, overflow → (0, 2) + expect(result.layout).toMatchObject({ x: 0, y: 2, w: 3, h: 2 }); + }); + + test("sequential: does NOT backfill gap beside taller widget", () => { + const existing: DashboardWidget[] = [ + { + title: "Chart", + displayType: "line", + layout: { x: 0, y: 0, w: 4, h: 2 }, + }, + { + title: "KPI-1", + displayType: "big_number", + layout: { x: 4, y: 0, w: 2, h: 1 }, + }, + ]; + const result = assignDefaultLayout( + { title: "KPI-2", displayType: "big_number" }, + existing + ); + // Old greedy algorithm would place at (4,1) — the gap beside the line chart. + // Sequential mode wraps to below everything instead. + expect(result.layout).toMatchObject({ x: 0, y: 2 }); + }); + + test("sequential: table (6x2) forces next widget to new row", () => { + const existing: DashboardWidget[] = [ + { + title: "Table", + displayType: "table", + layout: { x: 0, y: 0, w: 6, h: 2 }, + }, + ]; + const result = assignDefaultLayout( + { title: "Chart", displayType: "line" }, + existing + ); + // cursor=(6,0) → 6+3=9 > 6, overflow → (0, 2) + expect(result.layout).toMatchObject({ x: 0, y: 2, w: 3, h: 2 }); + }); + + test("sequential: all existing widgets lack layouts → place at (0, 0)", () => { + const existing: DashboardWidget[] = [ + { title: "No Layout 1", displayType: "line" }, + { title: "No Layout 2", displayType: "bar" }, + ]; + const result = assignDefaultLayout( + { title: "New", displayType: "big_number" }, + existing + ); + expect(result.layout).toMatchObject({ x: 0, y: 0, w: 2, h: 1 }); + }); + + test("sequential: uses earlier widget when last has no layout", () => { + const existing: DashboardWidget[] = [ + { + title: "Has Layout", + displayType: "big_number", + layout: { x: 0, y: 0, w: 2, h: 1 }, + }, + { title: "No Layout", displayType: "line" }, + ]; + const result = assignDefaultLayout( + { title: "New", displayType: "big_number" }, + existing + ); + // Last with layout is at (0,0) w=2, cursor=(2,0) + expect(result.layout).toMatchObject({ x: 2, y: 0, w: 2, h: 1 }); + }); + + test("sequential: cursor overlaps manually-placed widget → falls back to below", () => { + // B at (4,0) was placed before A in the array, simulating manual rearrangement. + // Last widget with layout = A at (0,0) w=3 → cursor = (3, 0). + // New big_number (2x1) at (3,0) would need (3,0) and (4,0). + // (4,0) is occupied by B → regionFits fails → fallback to (0, maxY=2). + const existing: DashboardWidget[] = [ + { + title: "B", + displayType: "big_number", + layout: { x: 4, y: 0, w: 2, h: 2 }, + }, + { + title: "A", + displayType: "line", + layout: { x: 0, y: 0, w: 3, h: 2 }, + }, + ]; + const result = assignDefaultLayout( + { title: "New", displayType: "big_number" }, + existing + ); + expect(result.layout).toMatchObject({ x: 0, y: 2 }); + }); + + // -- Dense mode tests ---------------------------------------------------- + + test("dense: fills gap beside taller widget", () => { + const existing: DashboardWidget[] = [ + { + title: "Chart", + displayType: "line", + layout: { x: 0, y: 0, w: 4, h: 2 }, + }, + { + title: "KPI-1", + displayType: "big_number", + layout: { x: 4, y: 0, w: 2, h: 1 }, + }, + ]; + const result = assignDefaultLayout( + { title: "KPI-2", displayType: "big_number" }, + existing, + "dense" + ); + // Dense mode finds the gap at (4,1) beside the line chart + expect(result.layout).toMatchObject({ x: 4, y: 1, w: 2, h: 1 }); + }); + + test("dense: finds first available gap top-to-bottom", () => { + const existing: DashboardWidget[] = [ + { + title: "A", + displayType: "big_number", + layout: { x: 0, y: 0, w: 2, h: 1 }, + }, + ]; + const result = assignDefaultLayout( + { title: "B", displayType: "big_number" }, + existing, + "dense" + ); + // First gap is at (2,0) + expect(result.layout).toMatchObject({ x: 2, y: 0 }); + }); + + // -- Shared behavior tests ----------------------------------------------- + + test("unknown displayType uses fallback size 3x2", () => { + const result = assignDefaultLayout( + { title: "Custom", displayType: "some_future_type" } as DashboardWidget, + [] + ); + expect(result.layout).toMatchObject({ x: 0, y: 0, w: 3, h: 2 }); + }); }); // ---------------------------------------------------------------------------