Skip to content

Conversation

@karl-power
Copy link
Contributor

Closes HDX-3096

This PR adds the ability to validate an entered highlighted attribute SQL expression and alias in the sources form.

Once an expression (and an optional alias) has been entered in the source form (for a trace or log source), the "Validate expression" button can be clicked to check that the expression is valid. If the expression is invalid, the error will be displayed under the button. Validation results are cached by react-query via the useExplainQuery hook.

Untitled.mov

@changeset-bot
Copy link

changeset-bot bot commented Jan 8, 2026

⚠️ No Changeset found

Latest commit: fe285e5

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

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

@vercel
Copy link

vercel bot commented Jan 8, 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 8, 2026 5:45pm

luceneExpression: '',
alias: '',
},
{ shouldFocus: false },
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I noticed that the Lucene expression input was being focused for the newly appended field which was a bit confusing as that field is optional. This change stops any field being autofocused in the new row.

@claude
Copy link

claude bot commented Jan 8, 2026

Code Review

✅ Overall looks good! Clean implementation with proper error handling.

Minor Issues:

  • ⚠️ Error message styling inconsistency (SourceForm.tsx:291) → Error message text should have c="red" for consistency with validation message above (line 285) and other error displays in codebase

Good patterns followed:

  • ✓ Proper use of ChartConfigWithOptDateRange type
  • ✓ Correct import of ClickHouseQueryError
  • ✓ Query caching via react-query with enabled: false
  • instanceof check for error type
  • ✓ Component extraction to reduce file size

Optional improvement: Consider adding color to error message at line 291 for better UX consistency.

@github-actions
Copy link
Contributor

github-actions bot commented Jan 8, 2026

E2E Test Results

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

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

Tests ran across 4 shards in parallel.

View full report →

Copy link
Contributor

@pulpdrew pulpdrew left a comment

Choose a reason for hiding this comment

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

Functionally, this looks great!

IMO, the button and error messages being in the middle of an attribute row makes it harder for me to determine where one attribute section begins and another ends.

Screenshot 2026-01-08 at 2 19 30 PM

I wonder if @elizabetdev has any thoughts on the placement of this button and error message?

Some ideas I have:

  1. Move the button to the right of the lucene expression, or make it an icon button (checkmark or X depending on validation result?) next to the trash can icon.
  2. Use a toast/notification to indicate error or success instead of a message for displaying the error
  3. Add some sort of divider between rows
Screen.Recording.2026-01-08.at.3.11.15.PM.mov

@karl-power
Copy link
Contributor Author

karl-power commented Jan 9, 2026

Functionally, this looks great!

IMO, the button and error messages being in the middle of an attribute row makes it harder for me to determine where one attribute section begins and another ends.

I wonder if @elizabetdev has any thoughts on the placement of this button and error message?

Some ideas I have:

  1. Move the button to the right of the lucene expression, or make it an icon button (checkmark or X depending on validation result?) next to the trash can icon.
  2. Use a toast/notification to indicate error or success instead of a message for displaying the error
  3. Add some sort of divider between rows

Screen.Recording.2026-01-08.at.3.11.15.PM.mov

Thanks for the review! I agree that the layout feels a bit cluttered now, especially when there is an error to display. I tried a few different placements for the button and message and nothing felt 100% right. I like your 3 ideas, but will wait for Elizabet to chime in as well :)

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants