Skip to content

fix: Validate column name for metrics when creating charts#1597

Merged
kodiakhq[bot] merged 1 commit intomainfrom
fleon/hdx-1792
Jan 15, 2026
Merged

fix: Validate column name for metrics when creating charts#1597
kodiakhq[bot] merged 1 commit intomainfrom
fleon/hdx-1792

Conversation

@fleon
Copy link
Copy Markdown
Contributor

@fleon fleon commented Jan 13, 2026

When creating a chart, validate input before executing the query, so that we show a validation error instead of a query execution error if the column name is empty. The error state on a particular input is cleared once the input is populated.

Fixes HDX-1792.

Screenshots

Before:

image

After:

Error state:

CleanShot 2026-01-13 at 19 57 17@2x

Video:

CleanShot.2026-01-13.at.19.58.01.mp4

@vercel
Copy link
Copy Markdown

vercel Bot commented Jan 13, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Review Updated (UTC)
hyperdx-v2-oss-app Ready Ready Preview, Comment Jan 15, 2026 3:13pm

@changeset-bot
Copy link
Copy Markdown

changeset-bot Bot commented Jan 13, 2026

🦋 Changeset detected

Latest commit: 9f97813

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

This PR includes changesets to release 2 packages
Name Type
@hyperdx/app Patch
@hyperdx/api 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

@claude
Copy link
Copy Markdown

claude Bot commented Jan 13, 2026

PR Review: Metric Name Validation

Good changes:

  • Validation prevents query execution errors
  • Tests are comprehensive and cover edge cases
  • Error clearing on focus improves UX

⚠️ Issues to address:

  • Missing import in test file → Test uses renderWithMantine (line 166) but doesn't import it. Based on setupTests.tsx, this is a global function but tests should explicitly import it if not using a setup file that auto-imports it.

  • Type safety concern in validation → In DBEditTimeChartForm.tsx:603, the condition series.metricType && !series.metricName assumes metricType presence means validation is needed. However, if metricType can be falsy but still valid (e.g., empty string vs undefined), this could miss validation cases. Consider explicit check: series.metricType != null && !series.metricName.

  • Error propagation inconsistency → The validation returns early (line 612) without calling onChartConfigChanged or other callbacks that might be expected. Verify this doesn't break any parent component expectations.

  • Test mock accuracy → Test mock for MetricNameSelect (lines 66-86) simplifies the component but doesn't test the actual MetricNameSelect component behavior. Consider if integration tests should test against the real component.

Recommendation: Fix the missing import issue before merge. Other items are low-risk but worth considering.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Jan 13, 2026

E2E Test Results

All tests passed • 60 passed • 4 skipped • 753s

Status Count
✅ Passed 60
❌ Failed 0
⚠️ Flaky 0
⏭️ Skipped 4

Tests ran across 4 shards in parallel.

View full report →

@teeohhem
Copy link
Copy Markdown
Contributor

@fleon looks like a small linting/ts error is present in this PR.

Comment thread .changeset/tasty-wombats-flash.md Outdated
@fleon
Copy link
Copy Markdown
Contributor Author

fleon commented Jan 13, 2026

@teeohhem Ready to take another look. I also cleaned up the test file to reduce the number of mocks (only mocking what is necessary to test this particular bug fix).

@pulpdrew
Copy link
Copy Markdown
Contributor

LGTM on the Chart Explorer page. One additional related thing that could be good is to do the same validation when saving dashboard tiles:

Screen.Recording.2026-01-15.at.9.33.50.AM.mov

@teeohhem
Copy link
Copy Markdown
Contributor

Good callout @pulpdrew . Up to you @fleon whether you want to take that suggestion as a part of this ticket or create a follow up ticket and handle it there. Thanks!

When creating a chart, validate input before executing the query, so that
we show a validation error instead of a query execution error if the
column name is empty.

Fixes HDX-1792.
@fleon
Copy link
Copy Markdown
Contributor Author

fleon commented Jan 15, 2026

Thanks for the feedback @pulpdrew. I'll create a new ticket to update the dashboard page as well. I'll also think about if we can abstract the logic into a separate file to share between the two components.

Edit: Tracking in HDX-3222

Copy link
Copy Markdown
Contributor

@teeohhem teeohhem left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good stuff. TY!

@kodiakhq kodiakhq Bot merged commit ac3082a into main Jan 15, 2026
14 checks passed
@kodiakhq kodiakhq Bot deleted the fleon/hdx-1792 branch January 15, 2026 15:23
kodiakhq Bot pushed a commit that referenced this pull request Jan 16, 2026
knudtty pushed a commit that referenced this pull request Apr 16, 2026
When creating a chart, validate input before executing the query, so that we show a validation error instead of a query execution error if the column name is empty. The error state on a particular input is cleared once the input is populated.

Fixes HDX-1792.

Screenshots

## Before:

<img width="2599" height="791" alt="image" src="https://github.com/user-attachments/assets/3965d6d1-4e2f-444d-80ac-70ba4f148afa" />

## After:

### Error state:

<img width="2184" height="1438" alt="CleanShot 2026-01-13 at 19 57 17@2x" src="https://github.com/user-attachments/assets/86bb5e60-7d60-4f38-a30c-170524dd32d4" />

### Video:


https://github.com/user-attachments/assets/27382379-cb10-43cb-acd4-8f97cc257511
knudtty pushed a commit that referenced this pull request Apr 16, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants