Skip to content

fix preserve color rules for SQL and PromQL charts - #2940

Open
espenloov wants to merge 3 commits into
hyperdxio:mainfrom
espenloov:bug/fix-color-rules-sql-promql
Open

fix preserve color rules for SQL and PromQL charts#2940
espenloov wants to merge 3 commits into
hyperdxio:mainfrom
espenloov:bug/fix-color-rules-sql-promql

Conversation

@espenloov

Copy link
Copy Markdown

Summary

Conditional color rules configured for Number tiles were silently dropped when the chart used raw SQL or PromQL. The settings drawer accepted the rules and saving succeeded, but the rules disappeared after reloading the dashboard.

convertFormStateToSavedChartConfig and convertFormStateToChartConfig use explicit pick() allowlists for SQL and PromQL configs. When colorRules was added to SharedChartSettingsSchema, it was not added to these allowlists.

This change:

  • Adds colorRules to the SQL and PromQL branches of both conversion functions.
  • Adds regression tests covering persisted and rendered configs for both SQL and PromQL Number tiles.

Investigation

The issue was reproduced on a self-hosted ClickStack deployment using HyperDX 2.35.0:

  1. Created a Number tile in SQL mode.
  2. Added conditional color rules through Display Settings.
  3. Saved and reloaded the dashboard.
  4. Confirmed that the rules disappeared without any client or server error.

(promql later diagnosed after looking at the code)

The failure was then isolated as follows:

  • Inspected the dashboard documents in MongoDB and found that the static color was stored while colorRules was absent.
  • Confirmed that RawSqlBaseChartConfigSchema and the other chart schemas inherit colorRules from SharedChartSettingsSchema.
  • Wrote colorRules directly into MongoDB and confirmed that the existing renderer applied them correctly.
  • Created an equivalent Builder tile and confirmed that its rules persisted normally.
  • Compared ChartEditor/utils.ts on main with the released application and confirmed that both contained the same missing allowlist entries.

Validation

Test Suites: 1 passed
Tests: 90 passed

The new tests verify that colorRules survive:

  • SQL saved-config conversion
  • SQL render-config conversion
  • PromQL saved-config conversion
  • PromQL render-config conversion

How to test on Vercel preview

Preview routes: /dashboards

Steps:

  1. Open Dashboards and create a dashboard.
  2. Add a tile, set its display type to Number, switch to SQL mode, enter SELECT 1, and run the query.
  3. Open Display Settings, select Add rule under Conditional colors, configure a rule matching the returned value, and select a different color.
  4. Apply the display settings and save the tile.
  5. Reload the dashboard, reopen the tile editor, and open Display Settings.
  6. Verify that the conditional color rule is still present and that the Number tile uses its configured conditional color.

References

PR #2386#2386

@changeset-bot

changeset-bot Bot commented Aug 18, 2026

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 2df59d1

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 3 packages
Name Type
@hyperdx/app Patch
@hyperdx/api Patch
@hyperdx/otel-collector Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@vercel

vercel Bot commented Aug 18, 2026

Copy link
Copy Markdown

@espenloov is attempting to deploy a commit to the HyperDX Team on Vercel.

A member of the Team first needs to authorize it.

@greptile-apps

greptile-apps Bot commented Aug 18, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

The PR now preserves conditional color rules for SQL and PromQL Number tiles across editor conversion and external dashboard API round trips.

  • Adds colorRules to SQL and PromQL chart-config allowlists.
  • Extends raw-SQL external API conversion, validation, and OpenAPI documentation.
  • Adds regression coverage for editor persistence/rendering and external create/read/update behavior.

Confidence Score: 5/5

The PR appears safe to merge.

No blocking failure remains; the previously reported external-representation loss is addressed on both external conversion legs and covered through create, read, and update.

Important Files Changed

Filename Overview
packages/app/src/components/ChartEditor/utils.ts Adds colorRules to both saved and rendered SQL/PromQL config allowlists, addressing the original persistence loss.
packages/api/src/routers/external-api/v2/utils/dashboards.ts Preserves and normalizes raw-SQL Number color rules in both directions of external dashboard conversion.
packages/api/src/utils/zod.ts Extends the raw-SQL Number external schema with the bounded Number-tile color-rule contract.
packages/api/src/routers/external-api/tests/dashboards.int.test.ts Covers create, read, update, normalization, and validation of raw-SQL Number color rules.
packages/app/src/components/ChartEditor/tests/utils.test.ts Verifies SQL and PromQL color rules survive saved-config and render-config conversion.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart LR
  Editor[Chart editor form] --> Conversion[SQL / PromQL config conversion]
  Conversion --> Stored[(Saved dashboard)]
  Stored --> ExternalGET[External API GET]
  ExternalGET --> ExternalPUT[External API PUT]
  ExternalPUT --> Stored
  Stored --> Renderer[Number tile renderer]
Loading

Reviews (3): Last reviewed commit: "test raw SQL color rules" | Re-trigger Greptile

Comment thread packages/app/src/components/ChartEditor/utils.ts
@espenloov

Copy link
Copy Markdown
Author

I overlooked the external dashboard conversion path and had only tested the editor round trip. I’ve now updated the external schema and both conversion directions.

@github-actions

github-actions Bot commented Aug 18, 2026

Copy link
Copy Markdown
Contributor

Deep Review

Conditional color rules (colorRules) are now preserved for SQL and PromQL Number tiles. The fix is a clean, symmetric mirror of the existing static color handling across four touch points — the app-side pick() allowlists, the external zod schema, the external↔internal conversions, and the OpenAPI docs. Correctness, adversarial, and TypeScript reviewers independently traced the POST→GET→PUT round-trip and confirmed it is faithful, type-safe, and backward-compatible (previously-ignored input is now honored; no request shape regresses).

✅ No critical issues found.

🟡 P2 -- recommended

  • .changeset/large-suits-smoke.md:1 -- the changeset lists only @hyperdx/app, but this PR makes a behavior-changing edit to the External API v2 contract in packages/api (zod.ts, utils/dashboards.ts, openapi.json), so packages/api/CHANGELOG.md will not record the contract change.
    • Fix: Add @hyperdx/api to the changeset frontmatter and describe the raw SQL number tile colorRules contract change in the body.
    • project-standards, api-contract
  • packages/api/src/mcp/tools/dashboards/schemas.ts:98 -- the MCP dashboard tool schema and prompt text still assert raw SQL number tiles do not support colorRules, so the MCP surface silently strips a field the REST API v2 now accepts and round-trips.
    • Fix: Extend colorRules support to the MCP raw SQL number tile schema, or track the REST/MCP parity gap as an explicit follow-up.
🔵 P3 nitpicks (3)
  • packages/api/src/routers/external-api/v2/utils/dashboards.ts:184 -- toExternalColorRules emits the full resolved array uncapped, so a tile stored with >10 rules (reachable only via direct DB write) returns a payload the max(10) input schema rejects on a GET→PUT round-trip.
    • Fix: Cap the output at 10 rules so the emitted array cannot exceed the input schema's max(10).
  • packages/api/src/utils/zod.ts:325 -- the z.array(NumberTileColorConditionSchema).max(10).optional() fragment is duplicated verbatim in the builder and raw-SQL number schemas with no shared constant, so a future cap or condition change risks being applied to only one copy.
    • Fix: Extract a shared NumberTileColorRulesSchema constant and reuse it at both call sites.
  • .changeset/large-suits-smoke.md:5 -- the description misspells "PromQL" as "PromQl".
    • Fix: Correct "PromQl" to "PromQL".

Reviewers (7): correctness, adversarial, api-contract, kieran-typescript, testing, maintainability, project-standards.

Testing gaps:

  • No raw-SQL test for the "all stored colorRules colors unresolvable → field omitted" case; only the mixed valid/invalid case is exercised for raw SQL (the builder path covers full omission).
  • No test asserts colorRules is undefined when the form field is unset in the new app-side omits color test, nor an end-to-end empty-array (colorRules: []) round-trip.

@espenloov

Copy link
Copy Markdown
Author

Added the suggested raw SQL validation and normalization tests, and updated the stale comments. #2940 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant