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 341c19fba7..2507f93e5a 100644 --- a/packages/api/openapi.json +++ b/packages/api/openapi.json @@ -2302,7 +2302,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/routers/external-api/__tests__/dashboards.int.test.ts b/packages/api/src/routers/external-api/__tests__/dashboards.int.test.ts index 3b97264360..67d93b3357 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 @@ -3073,7 +3073,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', }, }; @@ -4783,7 +4783,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', }, }; @@ -5360,6 +5360,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 () => { @@ -5455,21 +5462,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 ───────────────────────── @@ -5499,6 +5537,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' }], @@ -5520,6 +5568,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' }], @@ -5531,6 +5588,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: [ @@ -5629,13 +5697,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( @@ -5649,6 +5711,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 5e868f85ca..fb175693df 100644 --- a/packages/api/src/routers/external-api/v2/dashboards.ts +++ b/packages/api/src/routers/external-api/v2/dashboards.ts @@ -1174,9 +1174,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 6700532138..0421e58aeb 100644 --- a/packages/api/src/routers/external-api/v2/utils/dashboards.ts +++ b/packages/api/src/routers/external-api/v2/utils/dashboards.ts @@ -237,10 +237,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 { @@ -685,12 +683,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 4dd85762c9..c8c011200c 100644 --- a/packages/api/src/utils/zod.ts +++ b/packages/api/src/utils/zod.ts @@ -321,14 +321,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 = @@ -368,9 +362,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 aed30696f9..2d17a383a3 100644 --- a/packages/app/src/components/ChartEditor/__tests__/utils.test.ts +++ b/packages/app/src/components/ChartEditor/__tests__/utils.test.ts @@ -1575,6 +1575,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 830f6c1865..bcf9449a7d 100644 --- a/packages/app/src/components/ChartEditor/utils.ts +++ b/packages/app/src/components/ChartEditor/utils.ts @@ -157,6 +157,7 @@ export function convertFormStateToSavedChartConfig( 'displayType', 'numberFormat', 'color', + 'colorRules', 'granularity', 'compareToPreviousPeriod', 'fillNulls', @@ -180,6 +181,7 @@ export function convertFormStateToSavedChartConfig( 'displayType', 'numberFormat', 'color', + 'colorRules', 'granularity', 'compareToPreviousPeriod', 'fillNulls', @@ -238,6 +240,7 @@ export function convertFormStateToChartConfig( 'displayType', 'numberFormat', 'color', + 'colorRules', 'granularity', 'compareToPreviousPeriod', 'fillNulls', @@ -261,6 +264,7 @@ export function convertFormStateToChartConfig( 'displayType', 'numberFormat', 'color', + 'colorRules', 'granularity', 'compareToPreviousPeriod', 'fillNulls',