diff --git a/.changeset/large-suits-smoke.md b/.changeset/large-suits-smoke.md new file mode 100644 index 0000000000..182fde5e03 --- /dev/null +++ b/.changeset/large-suits-smoke.md @@ -0,0 +1,5 @@ +--- +"@hyperdx/app": patch +--- + +Preserve conditional color rules for SQL and PromQl chart configs diff --git a/packages/api/openapi.json b/packages/api/openapi.json index 841e3d9757..6ceb379443 100644 --- a/packages/api/openapi.json +++ b/packages/api/openapi.json @@ -2435,7 +2435,15 @@ }, "color": { "$ref": "#/components/schemas/ChartPaletteToken", - "description": "Optional static color applied to the displayed number. Raw SQL number tiles do not support conditional colorRules.\n" + "description": "Optional static color applied to the displayed number." + }, + "colorRules": { + "type": "array", + "maxItems": 10, + "description": "Ordered conditional color rules evaluated against the displayed value (last match wins). Falls back to color, then the default text color when no rule matches.\n", + "items": { + "$ref": "#/components/schemas/NumberTileColorCondition" + } } } } diff --git a/packages/api/src/mcp/__tests__/dashboards/patchDashboard.int.test.ts b/packages/api/src/mcp/__tests__/dashboards/patchDashboard.int.test.ts index 4b498562ef..ed4f69b7a0 100644 --- a/packages/api/src/mcp/__tests__/dashboards/patchDashboard.int.test.ts +++ b/packages/api/src/mcp/__tests__/dashboards/patchDashboard.int.test.ts @@ -655,6 +655,65 @@ describe('MCP Dashboard Tools - clickstack_patch_dashboard', () => { ]); }); + it('should preserve raw SQL number tile colorRules across a get/patch round trip', async () => { + const connectionId = ctx.connection._id.toString(); + const colorRules = [ + { operator: 'lt', value: 0.95, color: 'chart-error', label: 'Down' }, + { operator: 'between', value: [0.95, 0.99], color: 'chart-warning' }, + ]; + const createResult = await callTool( + ctx.client!, + 'clickstack_save_dashboard', + { + name: 'SQL ColorRules Patch Test', + tiles: [ + { + name: 'SLO', + config: { + configType: 'sql', + displayType: 'number', + connectionId, + sqlTemplate: 'SELECT 0.99 AS value LIMIT 1', + color: 'chart-success', + colorRules, + }, + }, + ], + }, + ); + const created = JSON.parse(getFirstText(createResult)); + const tileId = created.tiles[0].id; + + const getDashboardResult = await callTool( + ctx.client!, + 'clickstack_get_dashboard', + { id: created.id }, + ); + const fetched = JSON.parse(getFirstText(getDashboardResult)); + + // Feed the fetched tile straight back in, the way an agent editing an + // existing tile would; the config must survive the round trip intact. + const patchResult = await callTool( + ctx.client!, + 'clickstack_patch_dashboard', + { + dashboardId: created.id, + tileId, + tile: { name: 'SLO (patched)', config: fetched.tiles[0].config }, + }, + ); + expect(patchResult.isError).toBeFalsy(); + + const getResult = await callTool( + ctx.client!, + 'clickstack_get_dashboard_tile', + { dashboardId: created.id, tileId }, + ); + const tile = JSON.parse(getFirstText(getResult)); + expect(tile.config.color).toBe('chart-success'); + expect(tile.config.colorRules).toEqual(colorRules); + }); + it('should round-trip backgroundChart: patch then get_dashboard_tile', async () => { const sourceId = ctx.traceSource._id.toString(); const createResult = await callTool( diff --git a/packages/api/src/mcp/__tests__/dashboards/saveDashboard.int.test.ts b/packages/api/src/mcp/__tests__/dashboards/saveDashboard.int.test.ts index 760a6b8556..ebc9ceb9fb 100644 --- a/packages/api/src/mcp/__tests__/dashboards/saveDashboard.int.test.ts +++ b/packages/api/src/mcp/__tests__/dashboards/saveDashboard.int.test.ts @@ -816,34 +816,42 @@ describe('MCP Dashboard Tools - clickstack_save_dashboard', () => { expect(result.isError).toBe(true); }); - it('should keep color but drop colorRules on a raw SQL number tile', async () => { + it('should round-trip color and colorRules on a raw SQL number tile', async () => { const connectionId = ctx.connection._id.toString(); + const sqlConfig = { + configType: 'sql', + displayType: 'number', + connectionId, + sqlTemplate: 'SELECT 0.99 AS value LIMIT 1', + color: 'chart-success', + colorRules: [ + { operator: 'lt', value: 0.95, color: 'chart-error', label: 'Down' }, + { operator: 'between', value: [0.95, 0.99], color: 'chart-warning' }, + ], + }; + const saveResult = await callTool( ctx.client!, 'clickstack_save_dashboard', { name: 'SQL Number Color', - tiles: [ - { - name: 'SLO', - config: { - configType: 'sql', - displayType: 'number', - connectionId, - sqlTemplate: 'SELECT 0.99 AS value LIMIT 1', - color: 'chart-success', - colorRules: [ - { operator: 'gte', value: 1, color: 'chart-error' }, - ], - }, - }, - ], + tiles: [{ name: 'SLO', config: sqlConfig }], }, ); expect(saveResult.isError).toBeFalsy(); const saved = JSON.parse(getFirstText(saveResult)); - expect(saved.tiles[0].config.color).toBe('chart-success'); - expect(saved.tiles[0].config.colorRules).toBeUndefined(); + expect(saved.tiles[0].config).toMatchObject(sqlConfig); + + const getResult = await callTool( + ctx.client!, + 'clickstack_get_dashboard', + { + id: saved.id, + }, + ); + expect(getResult.isError).toBeFalsy(); + const fetched = JSON.parse(getFirstText(getResult)); + expect(fetched.tiles[0].config).toMatchObject(sqlConfig); }); it('should round-trip number-tile backgroundChart through save and get', async () => { diff --git a/packages/api/src/mcp/prompts/dashboards/content.ts b/packages/api/src/mcp/prompts/dashboards/content.ts index 036ad2ee9e..d94c7f94de 100644 --- a/packages/api/src/mcp/prompts/dashboards/content.ts +++ b/packages/api/src/mcp/prompts/dashboards/content.ts @@ -1060,7 +1060,7 @@ Colors are palette tokens, not hex. Order rules from least to most severe so the ] } -colorRules is for number builder tiles only. Raw SQL number tiles (configType: "sql") support color but not colorRules. +Both color and colorRules work on builder number tiles and on raw SQL number tiles (configType: "sql"). == NUMBER TILE BACKGROUND CHART == diff --git a/packages/api/src/mcp/tools/dashboards/schemas.ts b/packages/api/src/mcp/tools/dashboards/schemas.ts index d1ea8c4a12..1a1740635b 100644 --- a/packages/api/src/mcp/tools/dashboards/schemas.ts +++ b/packages/api/src/mcp/tools/dashboards/schemas.ts @@ -127,8 +127,12 @@ const numberTileColorRulesDescription = const rawSqlNumberTileColorDescription = 'Static color for the displayed number, as a palette token such as ' + '"chart-blue" or "chart-success". Valid only when displayType is ' + - '"number", ignored otherwise. Raw SQL number tiles do not support ' + - 'conditional colorRules.'; + '"number", ignored otherwise. Applied unless a colorRules entry matches ' + + 'the value.'; + +const rawSqlNumberTileColorRulesDescription = + `${numberTileColorRulesDescription} Valid only when displayType is ` + + '"number", ignored otherwise.'; const numberTileBackgroundChartDescription = 'Optional background trend sparkline drawn behind the number, derived ' + @@ -1052,6 +1056,11 @@ GROUP BY ServiceName, ts color: ChartPaletteTokenSchema.optional().describe( rawSqlNumberTileColorDescription, ), + colorRules: z + .array(NumberTileColorConditionSchema) + .max(10) + .optional() + .describe(rawSqlNumberTileColorRulesDescription), onClick: mcpOnClickSchema.optional(), }), }); diff --git a/packages/api/src/routers/external-api/__tests__/dashboards.int.test.ts b/packages/api/src/routers/external-api/__tests__/dashboards.int.test.ts index 76ae1f76c5..2558cf609d 100644 --- a/packages/api/src/routers/external-api/__tests__/dashboards.int.test.ts +++ b/packages/api/src/routers/external-api/__tests__/dashboards.int.test.ts @@ -3086,7 +3086,7 @@ describe('External API v2 Dashboards - new format', () => { sqlTemplate, sourceId, numberFormat: { output: 'currency', currencySymbol: '$' }, - // Raw SQL number tiles carry the static tile color (no colorRules). + // This fixture exercises the static color; raw SQL number tiles also support colorRules. color: 'chart-purple', }, }; @@ -4780,7 +4780,7 @@ describe('External API v2 Dashboards - new format', () => { sqlTemplate, sourceId, numberFormat: { output: 'currency', currencySymbol: '$' }, - // Raw SQL number tiles carry the static tile color (no colorRules). + // This fixture exercises the static color; raw SQL number tiles also support colorRules. color: 'chart-purple', }, }; @@ -5879,6 +5879,13 @@ describe('External API v2 Dashboards - new format', () => { }, }); + const postRawSqlTile = (config: Record) => + authRequest('post', BASE_URL).send({ + name: 'Raw SQL number color dashboard', + tiles: [rawSqlNumberTile(config)], + tags: [], + }); + // ── Positive: one per UI input ────────────────────────────────────── it('round-trips a builder number tile with a static color', async () => { @@ -5974,21 +5981,52 @@ describe('External API v2 Dashboards - new format', () => { }); }); - it('strips colorRules from a raw SQL number tile, keeping color', async () => { + it('round-trips colorRules for a raw SQL number tile', async () => { + const colorRules = [ + { operator: 'gt', value: 1, color: 'chart-red' }, + { operator: 'lte', value: 1, color: 'chart-green' }, + ]; + const create = await authRequest('post', BASE_URL) .send({ name: 'Raw SQL colorRules', tiles: [ rawSqlNumberTile({ color: 'chart-blue', - colorRules: [{ operator: 'gt', value: 1, color: 'chart-red' }], + colorRules, }), ], tags: [], }) .expect(200); - expect(create.body.data.tiles[0].config.color).toBe('chart-blue'); - expect(create.body.data.tiles[0].config.colorRules).toBeUndefined(); + + expect(create.body.data.tiles[0].config).toMatchObject({ + color: 'chart-blue', + colorRules, + }); + + const dashboardId = create.body.data.id; + const get = await authRequest('get', `${BASE_URL}/${dashboardId}`).expect( + 200, + ); + + expect(get.body.data.tiles[0].config).toMatchObject({ + color: 'chart-blue', + colorRules, + }); + + const update = await authRequest('put', `${BASE_URL}/${dashboardId}`) + .send({ + name: get.body.data.name, + tiles: get.body.data.tiles, + tags: get.body.data.tags, + }) + .expect(200); + + expect(update.body.data.tiles[0].config).toMatchObject({ + color: 'chart-blue', + colorRules, + }); }); // ── Negative: one per schema rejection rule ───────────────────────── @@ -6018,6 +6056,16 @@ describe('External API v2 Dashboards - new format', () => { expect(res.body.message).toContain('tiles.0.config.colorRules'); }); + it('rejects more than 10 colorRules for a raw SQL number tile', async () => { + const colorRules = Array.from({ length: 11 }, (_, i) => ({ + operator: 'gt', + value: i, + color: 'chart-blue', + })); + const res = await postRawSqlTile({ colorRules }).expect(400); + expect(res.body.message).toContain('tiles.0.config.colorRules'); + }); + it('rejects a between rule whose value is not a two-number tuple', async () => { await postTile({ colorRules: [{ operator: 'between', value: 100, color: 'chart-blue' }], @@ -6039,6 +6087,15 @@ describe('External API v2 Dashboards - new format', () => { } }); + it('rejects unsupported colorRule operators for a raw SQL number tile', async () => { + for (const operator of ['contains', 'startsWith', 'endsWith', 'regex']) { + const res = await postRawSqlTile({ + colorRules: [{ operator, value: 'error', color: 'chart-blue' }], + }).expect(400); + expect(res.body.message).toContain('tiles.0.config.colorRules'); + } + }); + it('rejects a per-rule color that is not a palette token', async () => { const res = await postTile({ colorRules: [{ operator: 'gt', value: 1, color: 'red' }], @@ -6050,6 +6107,17 @@ describe('External API v2 Dashboards - new format', () => { }).expect(400); }); + it('rejects a per-rule color that is not a palette token for a raw SQL number tile', async () => { + const res = await postRawSqlTile({ + colorRules: [{ operator: 'gt', value: 1, color: 'red' }], + }).expect(400); + expect(res.body.message).toContain('tiles.0.config.colorRules'); + + await postRawSqlTile({ + colorRules: [{ operator: 'gt', value: 1, color: 'chart-1' }], + }).expect(400); + }); + it('rejects a rule label longer than 40 characters', async () => { await postTile({ colorRules: [ @@ -6148,13 +6216,7 @@ describe('External API v2 Dashboards - new format', () => { }); it('normalizes a legacy numeric token on a raw SQL number tile to its hue name on read', async () => { - const create = await authRequest('post', BASE_URL) - .send({ - name: 'Raw SQL legacy color', - tiles: [rawSqlNumberTile({ color: 'chart-blue' })], - tags: [], - }) - .expect(200); + const create = await postRawSqlTile({ color: 'chart-blue' }).expect(200); const dashboardId = create.body.data.id; await Dashboard.updateOne( @@ -6168,6 +6230,32 @@ describe('External API v2 Dashboards - new format', () => { // chart-4 maps to chart-red. expect(get.body.data.tiles[0].config.color).toBe('chart-red'); }); + + it('normalizes legacy raw SQL colorRule colors and drops unresolvable ones on read', async () => { + const create = await postRawSqlTile({ + colorRules: [{ operator: 'gt', value: 1, color: 'chart-green' }], + }).expect(200); + const dashboardId = create.body.data.id; + + await Dashboard.updateOne( + { _id: dashboardId }, + { + $set: { + 'tiles.0.config.colorRules': [ + { operator: 'gt', value: 1, color: 'chart-1' }, + { operator: 'gt', value: 2, color: 'not-a-token' }, + ], + }, + }, + ); + + const get = await authRequest('get', `${BASE_URL}/${dashboardId}`).expect( + 200, + ); + expect(get.body.data.tiles[0].config.colorRules).toEqual([ + { operator: 'gt', value: 1, color: 'chart-green' }, + ]); + }); }); describe('Number tile background chart (HDX-1360)', () => { diff --git a/packages/api/src/routers/external-api/v2/dashboards.ts b/packages/api/src/routers/external-api/v2/dashboards.ts index 4df66f5275..e62ea8ade5 100644 --- a/packages/api/src/routers/external-api/v2/dashboards.ts +++ b/packages/api/src/routers/external-api/v2/dashboards.ts @@ -1299,9 +1299,16 @@ const EXTERNAL_DASHBOARD_PROJECTION = { * example: "number" * color: * $ref: '#/components/schemas/ChartPaletteToken' + * description: Optional static color applied to the displayed number. + * colorRules: + * type: array + * maxItems: 10 * description: > - * Optional static color applied to the displayed number. Raw - * SQL number tiles do not support conditional colorRules. + * Ordered conditional color rules evaluated against the displayed + * value (last match wins). Falls back to color, then the default + * text color when no rule matches. + * items: + * $ref: '#/components/schemas/NumberTileColorCondition' * * PieRawSqlChartConfig: * description: Raw SQL configuration for a pie chart. diff --git a/packages/api/src/routers/external-api/v2/utils/dashboards.ts b/packages/api/src/routers/external-api/v2/utils/dashboards.ts index 46c4a85071..85615a1a33 100644 --- a/packages/api/src/routers/external-api/v2/utils/dashboards.ts +++ b/packages/api/src/routers/external-api/v2/utils/dashboards.ts @@ -254,10 +254,8 @@ const convertToExternalTileChartConfig = ( sqlTemplate: config.sqlTemplate, sourceId: config.source, numberFormat: config.numberFormat, - // Raw SQL number tiles carry the static tile color too (no - // colorRules; see the schema). Normalize a legacy token saved - // before the hue rename to its hue name on output. color: resolveChartPaletteToken(config.color), + colorRules: toExternalColorRules(config.colorRules), }; case DisplayType.Pie: return { @@ -726,12 +724,14 @@ export function convertToInternalTileConfig( externalConfig.displayType === 'table' ? externalConfig.onClick : undefined, - // Only the raw SQL number variant carries `color`; table and pie - // do not expose it. `_.omitBy(_.isNil)` below drops it when absent. color: externalConfig.displayType === 'number' ? externalConfig.color : undefined, + colorRules: + externalConfig.displayType === 'number' + ? externalConfig.colorRules + : undefined, } satisfies RawSqlSavedChartConfig; break; default: diff --git a/packages/api/src/utils/zod.ts b/packages/api/src/utils/zod.ts index 727aad14d7..6cd71bae75 100644 --- a/packages/api/src/utils/zod.ts +++ b/packages/api/src/utils/zod.ts @@ -390,14 +390,8 @@ const externalDashboardTableRawSqlChartConfigSchema = const externalDashboardNumberRawSqlChartConfigSchema = externalDashboardRawSqlChartConfigBaseSchema.extend({ displayType: z.literal('number'), - // Raw SQL number tiles expose the same static tile color as builder - // number tiles: the editor gates the picker on displayType, not - // configType (`ChartDisplaySettingsDrawer`). `colorRules` is - // intentionally omitted here because the editor's save path - // (`convertFormStateToSavedChartConfig`) picks `color` but not - // `colorRules` for raw SQL configs, so persisted raw SQL number tiles - // never carry rules. color: ChartPaletteTokenSchema.optional(), + colorRules: z.array(NumberTileColorConditionSchema).max(10).optional(), }); const externalDashboardPieRawSqlChartConfigSchema = @@ -446,9 +440,9 @@ const externalDashboardNumberChartConfigSchema = z.object({ // `configType === 'sql'`). The save path // (`convertFormStateToSavedChartConfig`) persists `backgroundChart` only on // the builder branch (the raw SQL / promql picks omit it), so it lives on - // the builder number schema only, like `colorRules`. `BackgroundChartSchema` - // is imported from common-utils so the external surface cannot drift from - // what the UI persists. + // the builder number schema only. `BackgroundChartSchema` is imported from + // common-utils so the external surface cannot drift from what the UI + // persists. backgroundChart: BackgroundChartSchema.optional(), }); diff --git a/packages/app/src/components/ChartEditor/__tests__/utils.test.ts b/packages/app/src/components/ChartEditor/__tests__/utils.test.ts index aaed7ff08f..54728a4ffa 100644 --- a/packages/app/src/components/ChartEditor/__tests__/utils.test.ts +++ b/packages/app/src/components/ChartEditor/__tests__/utils.test.ts @@ -1627,6 +1627,52 @@ describe('color round-trip (sql/promql Number tile)', () => { expect((result as any).color).toBe('chart-error'); }); + it('preserves colorRules through both conversions for sql Number tile', () => { + const form: ChartEditorFormState = { + configType: 'sql', + displayType: DisplayType.Number, + sqlTemplate: 'SELECT count() FROM logs', + connection: 'conn-1', + colorRules: [ + { operator: 'lt', value: 4, color: 'chart-error' }, + { operator: 'gte', value: 4, color: 'chart-success' }, + ], + series: [], + }; + + expect(convertFormStateToSavedChartConfig(form, undefined)).toMatchObject({ + colorRules: form.colorRules, + }); + expect( + convertFormStateToChartConfig(form, dateRange, undefined), + ).toMatchObject({ + colorRules: form.colorRules, + }); + }); + + it('preserves colorRules through both conversions for promql Number tile', () => { + const form: ChartEditorFormState = { + configType: 'promql', + displayType: DisplayType.Number, + promqlExpression: 'up', + connection: 'conn-1', + colorRules: [ + { operator: 'lt', value: 1, color: 'chart-error' }, + { operator: 'gte', value: 1, color: 'chart-success' }, + ], + series: [], + }; + + expect(convertFormStateToSavedChartConfig(form, undefined)).toMatchObject({ + colorRules: form.colorRules, + }); + expect( + convertFormStateToChartConfig(form, dateRange, undefined), + ).toMatchObject({ + colorRules: form.colorRules, + }); + }); + it('omits color when not set on sql Number tile', () => { const form: ChartEditorFormState = { configType: 'sql', diff --git a/packages/app/src/components/ChartEditor/utils.ts b/packages/app/src/components/ChartEditor/utils.ts index 97fe7f3828..ebe5def777 100644 --- a/packages/app/src/components/ChartEditor/utils.ts +++ b/packages/app/src/components/ChartEditor/utils.ts @@ -194,6 +194,7 @@ export function convertFormStateToSavedChartConfig( 'displayType', 'numberFormat', 'color', + 'colorRules', 'granularity', 'compareToPreviousPeriod', 'fillNulls', @@ -218,6 +219,7 @@ export function convertFormStateToSavedChartConfig( 'displayType', 'numberFormat', 'color', + 'colorRules', 'granularity', 'compareToPreviousPeriod', 'fillNulls', @@ -276,6 +278,7 @@ export function convertFormStateToChartConfig( 'displayType', 'numberFormat', 'color', + 'colorRules', 'granularity', 'compareToPreviousPeriod', 'fillNulls', @@ -300,6 +303,7 @@ export function convertFormStateToChartConfig( 'displayType', 'numberFormat', 'color', + 'colorRules', 'granularity', 'compareToPreviousPeriod', 'fillNulls',