[HDX-4120] feat(api): support heatmap tiles in external dashboards API#2200
Open
alex-fedotyev wants to merge 11 commits intomainfrom
Open
[HDX-4120] feat(api): support heatmap tiles in external dashboards API#2200alex-fedotyev wants to merge 11 commits intomainfrom
alex-fedotyev wants to merge 11 commits intomainfrom
Conversation
🦋 Changeset detectedLatest commit: a217900 The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
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 |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
🔴 Tier 4 — CriticalTouches auth, data models, config, tasks, OTel pipeline, ClickHouse, or CI/CD. Why this tier:
Review process: Deep review from a domain expert. Synchronous walkthrough may be required. Stats
|
6 tasks
Contributor
E2E Test Results✅ All tests passed • 164 passed • 3 skipped • 1138s
Tests ran across 4 shards in parallel. |
Contributor
|
<!-- claude-code-review --> PR Review✅ No critical issues found. Implementation is clean and well-scoped:
Minor (non-blocking) observations:
|
Contributor
Author
|
Thanks for the review.
|
alex-fedotyev
added a commit
that referenced
this pull request
May 6, 2026
The external API surface in PR #2200 exposes where/whereLanguage at the chart-config level for heatmaps (UI HeatmapSeriesEditor doesn't render per-series filters; the persisted shape stores them once via SelectSQLStatementSchema). The MCP heatmap schema was dropping them, so once #2200 lands a save through hyperdx_save_dashboard would silently discard the filter on read-back. Add `where` and `whereLanguage` to mcpHeatmapTileSchema.config with the same defaults the search tile uses (where: '', whereLanguage: 'lucene'). Extend the round-trip test with a Lucene filter, add an explicit whereLanguage="sql" round-trip, and a negative test for an invalid whereLanguage. Update the example string and changeset to document the new fields.
6 tasks
Contributor
PR Review✅ No critical issues found. This PR cleanly fills the heatmap gap in the external dashboards API. End-to-end coverage (Zod, REST + MCP, OpenAPI, tests) is solid and the design choices are well-reasoned:
Minor observations (non-blocking):
|
Contributor
Compound Engineering Review✅ No critical issues found. Security audit returned no findings (tenant scoping, IDOR, info-leak, auth all sound). Type safety, test parity, and adherence to sibling patterns (Pie/Search/Markdown) are clean. P2 — Important
|
alex-fedotyev
pushed a commit
that referenced
this pull request
May 6, 2026
Compound-review feedback on #2200, all P2: - Drop redundant `whereLanguage.optional()`; the underlying `SearchConditionLanguageSchema` (alias `whereLanguageSchema`) is already `.optional()` and the sibling `externalDashboardSearchChartConfigSchema` writes it without the outer wrap. - Drop the `where: externalConfig.where ?? ''` redundancy in the deserializer; the Zod schema declares `.default('')` so the field is always a string post-parse. - Rename schema `HeatmapBuilderChartConfig` -> `HeatmapChartConfig` to match the sibling `SearchChartConfig` / `MarkdownChartConfig` naming. The `Builder` suffix is reserved for cases that disambiguate from a sibling `RawSqlChartConfig`; heatmap has no raw-SQL variant. - `convertToExternalHeatmapSelectItem` now takes a non-optional item. The case-arm caller checks for missing/string/empty-valueExpression items and falls through to `defaultTileConfig` with a logger.warn instead of silently emitting an out-of-contract payload that violates the external schema's `min(1)` rule. - Extract `HEATMAP_ALLOWED_SOURCE_KINDS` and `isHeatmapCompatibleSource` into common-utils. Both `ChartEditorControls.tsx` (UI) and the API's `getHeatmapTilesWithIncompatibleSources` (renamed from `...WithNonTraceSources`) reference the same set so UI and API gates move together. - Add a parity comment on the deserializer pointing to `applyHeatmapDefaults` so the `aggCondition`/`aggConditionLanguage` defaults stay coupled to the editor's behaviour. - Consolidate three pure-Zod schema-rejection tests (raw-SQL-heatmap absent, empty valueExpression, multiple select items) into a single parametrized `it.each` block; the runtime non-Trace-source rejection stays as its own test because it exercises the new `getHeatmapTilesWithIncompatibleSources` path.
Contributor
Author
|
P2 cleanups addressed in e89989d:
|
Contributor
Deep Review🔴 P0/P1 -- must fix
🟡 P2 -- recommended
🔵 P3 nitpicks (4)
Reviewers (10): correctness, testing, maintainability, project-standards, api-contract, security, performance, adversarial, kieran-typescript, reliability. Testing gaps:
|
alex-fedotyev
added a commit
that referenced
this pull request
May 7, 2026
Deep-review on #2200 flagged the previous comment as overstating parity. The editor's `applyHeatmapDefaults` writes `numberFormat: { output: 'duration', factor: 0.001 }`, `series.0.countExpression: 'count()'`, and `aggConditionLanguage: getStoredLanguage() ?? 'lucene'`, while this converter hardcodes `'lucene'` for the language and passes `numberFormat`/`countExpression` through verbatim from the external payload (or leaves them absent). Comment now enumerates the divergences and the rendering implications: the renderer does not read `aggConditionLanguage` for heatmap tiles (heatmap has no per-select where), and an API-built tile renders without duration formatting unless the caller specifies `numberFormat`. No behavior change.
alex-fedotyev
added a commit
that referenced
this pull request
May 7, 2026
Deep-review on #2200 flagged that buildCreateDashboardPrompt and buildQueryGuidePrompt enumerate every supported displayType but omit heatmap, hiding the new capability from agents reading the prompt. Adds heatmap to: - TILE TYPE GUIDE (buildCreateDashboardPrompt): one-line description plus the Trace-source-only and aggFn:"heatmap" + valueExpression requirements. - COMMON MISTAKES TO AVOID: a single bullet covering the source-kind and select-item rules. - PER-TILE TYPE CONSTRAINTS (buildQueryGuidePrompt): 1 select item, no groupBy, Trace sources only. Not in this commit: the MCP source-kind runtime gate (getHeatmapTilesWithIncompatibleSources) flagged as P0/P1 #2 in the same deep-review. The helper is added in #2200 and is not yet importable from this branch; the gate will land in the rebase commit after #2200 merges.
5 tasks
Heatmap is the only builder-mode display type that did not round-trip through the external dashboards API. The serializer dropped it into the "unsupported" fall-through, so creating, fetching, and updating heatmap tiles via /api/v2/dashboards lost the config. This adds: - A heatmap select-item schema in zod.ts that exposes the literal aggFn 'heatmap', valueExpression, optional countExpression, alias, and heatmapScaleType. Heatmap is included in the builder discriminated union only; it is intentionally not added to the raw SQL union since heatmap rendering requires isBuilderChartConfig. - A heatmap chart config schema with optional groupBy and numberFormat. - Builder serialization that reads the heatmap-specific fields off config.select[0] and emits aggFn 'heatmap' on the external surface, passing through groupBy and numberFormat. - Builder deserialization that maps the external aggFn 'heatmap' back to the internal aggFn 'count' the editor form persists, preserving countExpression, alias, and heatmapScaleType on the select item. - OpenAPI JSDoc with HeatmapSelectItem and HeatmapBuilderChartConfig components, and heatmap added to the TileConfig discriminator. - Integration tests: heatmap added to the existing "round-trip all supported chart types" tests for both POST and PUT, plus an explicit rejection test for raw SQL heatmap tiles. - Regenerated openapi.json. Follow-up to #2107 (review feedback from @pulpdrew).
knip flagged the exported type as unused. Extracted the inline heatmap select-item construction into a small convertToExternalHeatmapSelectItem helper that mirrors the existing convertToExternalSelectItem pattern and returns the typed shape, kills the knip warning, and removes the duplicated nullish-spread bookkeeping at the call site. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Reviewer flagged that convertToExternalHeatmapSelectItem used truthy checks (countExpression ? ...) while convertToInternalTileConfig used !== undefined. An empty-string countExpression/alias round-trip would silently drop the field. Use !== undefined consistently in both directions. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
The HeatmapSeriesEditor in the UI binds a single SearchWhereInput to the
top-level `where` / `whereLanguage` and does not render any groupBy
input. The previous schema had it backwards: it carried groupBy (not
exposed in the UI, never set by the form) and was missing
where / whereLanguage (the only filter heatmap actually uses).
- zod: drop groupBy, add where + whereLanguage to
externalDashboardHeatmapChartConfigSchema
- conversion utils: serializer emits chart-level where + whereLanguage;
deserializer maps them onto BuilderSavedChartConfig.where /
whereLanguage instead of stuffing them into select-item aggCondition
- OpenAPI: HeatmapBuilderChartConfig has where + whereLanguage instead
of groupBy; description notes the row-level filter is at chart level
- tests: POST round-trip exercises sql where ("ServiceName = 'api'")
with heatmapScaleType: 'log'; PUT round-trip exercises lucene where
("service:api") with heatmapScaleType: 'linear'. Both languages and
both scale types are now covered end-to-end.
- regenerated openapi.json
…ression
Two more gaps found while auditing the schema against the heatmap UI surface:
1. The heatmap UI's source picker is restricted to SourceKind.Trace
(ChartEditorControls.tsx:103-107: allowedSourceKinds={[SourceKind.Trace]}).
The external API previously accepted any source kind, so a metric or log
source would round-trip cleanly through /api/v2/dashboards and produce a
tile that does not render in the UI. Adds getHeatmapTilesWithNonTraceSources
and wires it into both POST and PUT alongside the existing source /
connection validators, returning 400 with a descriptive message.
2. validateChartForm in the editor rejects empty valueExpression on heatmap
("Value expression is required for heatmap charts"). The Zod schema
accepted empty string. Tightens to z.string().min(1).max(10000) and
updates the OpenAPI minLength to match.
Tests added: rejection on non-Trace source (with assertion on the error
message), rejection on empty valueExpression, rejection on multi-item
select array.
Regenerated openapi.json.
The existing realistic-payload round-trip exercises every optional field
populated. The minimal path (no countExpression / alias /
heatmapScaleType / where / whereLanguage / numberFormat) is what the
deserializer's !== undefined guards in v2/utils/dashboards.ts exist to
protect, and was uncovered until now.
The Zod schema applies where: z.string().optional().default(''), so the
round-trip surfaces an empty string for where while the other optional
fields stay undefined. The expected response keeps explicit on the
applied default rather than asserting strict equality with the request.
When the external API request omits whereLanguage on a heatmap tile, the deserializer at v2/utils/dashboards.ts:514 fills it with 'lucene' so the Mongo doc stays consistent across heatmap and non-heatmap chart types. The minimal-fields round-trip needs to expect whereLanguage on the response, not undefined. The schema-level constraint stays optional (matching the OpenAPI shape); the default lives at the persistence boundary, not the contract boundary.
Compound-review feedback on #2200, all P2: - Drop redundant `whereLanguage.optional()`; the underlying `SearchConditionLanguageSchema` (alias `whereLanguageSchema`) is already `.optional()` and the sibling `externalDashboardSearchChartConfigSchema` writes it without the outer wrap. - Drop the `where: externalConfig.where ?? ''` redundancy in the deserializer; the Zod schema declares `.default('')` so the field is always a string post-parse. - Rename schema `HeatmapBuilderChartConfig` -> `HeatmapChartConfig` to match the sibling `SearchChartConfig` / `MarkdownChartConfig` naming. The `Builder` suffix is reserved for cases that disambiguate from a sibling `RawSqlChartConfig`; heatmap has no raw-SQL variant. - `convertToExternalHeatmapSelectItem` now takes a non-optional item. The case-arm caller checks for missing/string/empty-valueExpression items and falls through to `defaultTileConfig` with a logger.warn instead of silently emitting an out-of-contract payload that violates the external schema's `min(1)` rule. - Extract `HEATMAP_ALLOWED_SOURCE_KINDS` and `isHeatmapCompatibleSource` into common-utils. Both `ChartEditorControls.tsx` (UI) and the API's `getHeatmapTilesWithIncompatibleSources` (renamed from `...WithNonTraceSources`) reference the same set so UI and API gates move together. - Add a parity comment on the deserializer pointing to `applyHeatmapDefaults` so the `aggCondition`/`aggConditionLanguage` defaults stay coupled to the editor's behaviour. - Consolidate three pure-Zod schema-rejection tests (raw-SQL-heatmap absent, empty valueExpression, multiple select items) into a single parametrized `it.each` block; the runtime non-Trace-source rejection stays as its own test because it exercises the new `getHeatmapTilesWithIncompatibleSources` path.
Deep-review on #2200 flagged the previous comment as overstating parity. The editor's `applyHeatmapDefaults` writes `numberFormat: { output: 'duration', factor: 0.001 }`, `series.0.countExpression: 'count()'`, and `aggConditionLanguage: getStoredLanguage() ?? 'lucene'`, while this converter hardcodes `'lucene'` for the language and passes `numberFormat`/`countExpression` through verbatim from the external payload (or leaves them absent). Comment now enumerates the divergences and the rendering implications: the renderer does not read `aggConditionLanguage` for heatmap tiles (heatmap has no per-select where), and an API-built tile renders without duration formatting unless the caller specifies `numberFormat`. No behavior change.
946412a to
d8f9af7
Compare
Six findings from the bot review on #2200: P0/P1 - MCP schema rejected heatmap tiles. Adds `mcpHeatmapTileSchema` (mirroring `externalDashboardHeatmapChartConfigSchema`) and includes it in the `mcpTilesParam` union so `hyperdx_save_dashboard` accepts the same heatmap shape the REST endpoint accepts. - MCP create/update did not run the heatmap source-kind gate. Both paths now call `getHeatmapTilesWithIncompatibleSources` after schema validation so an MCP-issued create cannot persist a heatmap that the REST PUT would reject with 400 on the next round-trip. The update path scopes the check to heatmap tiles whose `sourceId` or `displayType` changed in this request, matching the REST PUT. - `buildQueryGuidePrompt` did not document heatmap. Adds entries to TILE TYPE GUIDE, PER-TILE TYPE CONSTRAINTS, AGGREGATION FUNCTIONS, and COMMON MISTAKES so the LLM cannot skip the Trace-source and non-empty-`valueExpression` rules when generating tiles. P2 - `convertTileToExternalChart` returned `displayType: 'line'` via `defaultTileConfig` for any heatmap tile whose internal doc lacked a non-empty `valueExpression`. A GET / mutate / PUT round-trip would then silently overwrite the heatmap with a line chart in Mongo (data loss). The heatmap branch now emits a heatmap-shaped placeholder with empty `valueExpression`, so the response preserves `displayType` and a re-PUT surfaces the breakage as a clear schema error from the input schema's `min(1)` rule rather than dropping the heatmap. - The PUT-time `getHeatmapTilesWithIncompatibleSources` ran on every request regardless of whether heatmap tiles changed, so a source whose `kind` was changed to non-Trace after the dashboard was originally accepted wedged every PUT. The check now scopes to heatmap tiles whose `sourceId` or `displayType` changed compared to the existing dashboard via a new `filterChangedHeatmapTiles` helper. - POST and PUT each ran three separate `getSources(team)` queries (one per validation helper). The helpers now accept a pre-fetched `SourceForValidation[]`, hoisted into a single `fetchSourcesForValidation` call shared across `getMissingSources`, `getSourceConnectionMismatches`, and `getHeatmapTilesWithIncompatibleSources`. Same change applied to the MCP create/update paths. Drops two redundant DB round-trips per request. Tests cover the MCP schema accept/reject paths, the heatmap source-kind gate at the MCP layer, the GET placeholder behavior, the PUT scoping (no 400 on unrelated edits when the underlying source's `kind` was changed later), and that the prompt documents heatmap in all four sections. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Contributor
Author
|
Pushed a fix-pack addressing the deep-review findings in commit a217900. Mapping: P0/P1
P2
Tests cover the MCP schema accept/reject paths, the heatmap source-kind gate at the MCP layer, the GET placeholder behavior, the PUT scoping (no 400 on unrelated edits when the underlying source's Skipping the P2/P3 items the task spec marked as deferred (renames, cosmetic TS, |
pulpdrew
reviewed
May 9, 2026
Contributor
pulpdrew
left a comment
There was a problem hiding this comment.
Two small thoughts on the API spec, otherwise LGTM
| aggFn: z.literal('heatmap'), | ||
| valueExpression: z.string().min(1).max(10000), | ||
| countExpression: z.string().max(10000).optional(), | ||
| alias: z.string().max(10000).optional(), |
Contributor
There was a problem hiding this comment.
Alias isn't relevant to heatmaps, is it?
| // asks for it. | ||
| select: [ | ||
| { | ||
| aggFn: 'count', |
Contributor
There was a problem hiding this comment.
If we're just always saving count here, could we just remove the aggFn property from the OpenAPI spec / zod schema for the heatmap select item? Why make the user provide it if we ignore it?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Heatmap was the only builder-mode display type that did not round-trip through the external dashboards API. The serializer dropped it into the "unsupported" fall-through, so creating, fetching, and updating heatmap tiles via
/api/v2/dashboardslost the config.This wires up heatmap end-to-end on the external API: a dedicated select-item schema, an explicit case in both serialization directions, OpenAPI JSDoc, and tests.
Follow-up to #2107 (review feedback from @pulpdrew, who asked whether we had a follow-up ticket to update the external API for the new visualization type).
What's in the diff
packages/api/src/utils/zod.ts): a heatmap select-item schema that exposes the literalaggFn: "heatmap"plusvalueExpression, optionalcountExpression,alias,heatmapScaleType; and a heatmap chart-config schema with optionalgroupByandnumberFormat. Heatmap is added to the builder discriminated union only.packages/api/src/routers/external-api/v2/utils/dashboards.ts):case DisplayType.Heatmap:fall-through with an explicit case that reads heatmap-specific fields offconfig.select[0]and emitsaggFn: "heatmap"on the external surface.case 'heatmap':mirroring the Pie pattern; maps the externalaggFn: "heatmap"back to the internalaggFn: "count"that the editor form persists, while preservingcountExpression,alias, andheatmapScaleTypeon the select item.isBuilderChartConfig.packages/api/src/routers/external-api/v2/dashboards.ts):HeatmapSelectItemandHeatmapBuilderChartConfigcomponents, andheatmapadded to theTileConfigoneOfand discriminator mapping.packages/api/src/routers/external-api/__tests__/dashboards.test.ts): heatmap added to the existing "round-trip all supported chart types" tests for both POST and PUT, plus an explicit rejection test confirming raw-SQL heatmap tiles return 400.packages/api/openapi.json: regenerated.Notes for review
DBDashboardPage.tsxrequiresisBuilderChartConfigfor heatmap rendering, so the raw-SQL fall-through stays.heatmapScaleTypeandcountExpressionare persisted on the per-select-item level (viaDerivedColumnSchemainpackages/common-utils/src/types.ts), not on the chart config root. The form binds them asseries.0.heatmapScaleType/series.0.countExpression. The schema and conversion utilities follow that."heatmap"↔ internal"count") keeps the saved Mongo document identical to what the editor form produces, so heatmap tiles created via the API render the same way as ones created via the UI.Tier
Lands as
review/tier-4because anything underpackages/api/src/routers/external-api/is on the critical-path list. Diff is ~250 prod lines (most of it OpenAPI JSDoc and Zod boilerplate); no schema migrations or auth changes.Test plan
make ci-lint(yarn lint, tsc --noEmit, OpenAPI lint)make ci-unit(common-utils + app)make dev-intrequires Docker BuildKit which isn't available on this host.Deep-review carryover (2026-05-07)
convertToInternalTileConfigcomment vsapplyHeatmapDefaultsreality, see commit946412ad).mcpTilesParam) already shipped; P0/P1 feat: introduce usage-stats service + init changeset #3 (heatmap entry inbuildQueryGuidePrompt+buildCreateDashboardPrompt) added in commite583fc68.createDashboard/updateDashboardcallinggetHeatmapTilesWithIncompatibleSources), since the helper is added in this PR and not yet importable from feat(api/mcp): add heatmap tile schema for hyperdx_save_dashboard #2199's branch.