Skip to content

Conversation

brandon-pereira
Copy link
Member

Introduces the ability to edit the y, count, and group by values on the search event deltas heatmap allowing for deeper analysis

heatmap-demo.mov

Fixes HDX-2422

Copy link

changeset-bot bot commented Oct 7, 2025

🦋 Changeset detected

Latest commit: a40c0fc

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

Copy link

vercel bot commented Oct 7, 2025

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

Project Deployment Preview Comments Updated (UTC)
hyperdx-v2-oss-app Ready Ready Preview Comment Oct 10, 2025 9:45pm

Copy link
Contributor

github-actions bot commented Oct 7, 2025

E2E Test Results

All tests passed • 25 passed • 3 skipped • 227s

Status Count
✅ Passed 25
❌ Failed 0
⚠️ Flaky 0
⏭️ Skipped 3

View full report →

@brandon-pereira
Copy link
Member Author

@claude

Copy link

claude bot commented Oct 10, 2025

Claude finished @brandon-pereira's task —— View job


Clarification Needed

Hi @brandon-pereira! I see you mentioned me, but I'm not sure what you'd like me to help with. Here are some options:

  1. Review the PR code - I can provide a code review of the heatmap customization changes
  2. Address review comments - I can implement the suggestions from @teeohhem's review:
    • Move aggregation config to a shared location
    • Add info tooltips for the y-axis, count, and group by fields
    • Adjust the play button sizing
  3. Something else - Let me know what you need!

Could you clarify what you'd like me to do?


Job Run • Branch: brandon/heatmap-customization

@brandon-pereira
Copy link
Member Author

@claude review the pr

Copy link

claude bot commented Oct 10, 2025

PR Review: Event Deltas Heatmap Customization

Thank you for this feature! The ability to customize the heatmap visualization adds valuable flexibility for deeper analysis.

Strengths

  1. Good architecture: Extracting logic into a dedicated DBSearchHeatmapChart component improves maintainability
  2. User-friendly: The form-based customization with tooltips provides clear guidance
  3. URL state management: Using nuqs for query parameters preserves user selections
  4. Better error handling: Enhanced error display with modal details
  5. Bug fix in DBDeltaChart: Merging unique keys from both outlier and inlier maps prevents missing properties

Critical Issue: SQL Injection Risk

User input for value and count fields is passed directly to SQL queries without validation. These arbitrary SQL expressions could allow malicious users to extract sensitive data, modify/delete data, or bypass access controls.

Recommendation: Add SQL expression validation, whitelist allowed functions/columns, and ensure backend properly sanitizes expressions.

Other Issues

  1. isAggregateFunction logic has false positives (mycount matches count) and misses uppercase (COUNT)
  2. Performance: connection object not memoized causing unnecessary re-renders
  3. Inconsistent with clause handling between heatmap and delta configs
  4. Missing test coverage for new functionality
  5. Unused Input import in DBSearchPage.tsx

Recommendations

  1. CRITICAL: Address SQL injection before merging
  2. Add comprehensive test coverage
  3. Fix aggregate function detection with word boundaries and case-insensitive matching
  4. Add input validation hints in UI
  5. Consider query timeout/resource limits for expensive custom queries

Overall: Great feature with solid architecture, but the SQL injection vulnerability is a blocker that must be addressed before merging.

@brandon-pereira
Copy link
Member Author

brandon-pereira commented Oct 10, 2025

Feedback from @MikeShi42 (documenting for viz):

  • Remove Group By for now, we can re-add it later if we see a need
  • Make form it always visible to the user
  • Fix bug with Outlier logic
  • Add Docs to ClickHouse docs repo (open PR, we will link out to this page in another PR later)

@teeohhem marking this as ready for re-review as all feedback has been addressed!

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.

2 participants