Skip to content

Comments

feat: Improve pie chart implementation#1773

Open
pulpdrew wants to merge 1 commit intomainfrom
drew/pie-chart-fixes
Open

feat: Improve pie chart implementation#1773
pulpdrew wants to merge 1 commit intomainfrom
drew/pie-chart-fixes

Conversation

@pulpdrew
Copy link
Contributor

@pulpdrew pulpdrew commented Feb 20, 2026

Closes HDX-3479

Summary

This PR makes a number of improvements to new pie chart implementation (#1704)

  1. Pie charts are now limited to 1 series. Previously, the pie chart summed the values of each series by group, and used the sum as the slice value. This is non-obvious and probably not what users expect. With a one-series limit, this problem is eliminated. Further, the logic for formatting the pie chart data from the clickhouse response is dramatically simpler.
  2. Slices are now ordered by value decreasing, to avoid randomly changing slice order on refresh
  3. Instead of being randomly generated, slice colors are now consistent with the theme colors and auto-detect log and trace severity levels, matching line/bar chart behavior
  4. The external dashboards API now supports reading and writing pie charts. The transformation code has been updated so that there will be a type error if any new chart types are added in the future without updating the external API code.
  5. The pie chart's tooltip now matches the style of the line chart tooltip, and is updated appropriately based on the app theme and light/dark mode.
  6. The chart's number format is now applied to values in the pie chart tooltip
  7. Slice labels are now correctly populated when a map is accessed in the Group By (eg. when grouping by ResourceAttributes['app'], the slice labels include the app value instead of being empty).
  8. Also, added some unit tests for the pie chart data transformation, and moved it to ChartUtils with the other similar chart data transformation code.

@changeset-bot
Copy link

changeset-bot bot commented Feb 20, 2026

🦋 Changeset detected

Latest commit: 2dd0fea

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

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

vercel bot commented Feb 20, 2026

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

Project Deployment Actions Updated (UTC)
hyperdx-v2-oss-app Ready Ready Preview, Comment Feb 20, 2026 9:30pm

Request Review

@github-actions
Copy link
Contributor

PR Review: feat: Improve pie chart implementation

Overall this is a clean refactor with good test coverage. A few items to address:

  • ⚠️ Dead code: aliasMap is still computed in DBPieChart.tsx:85-98 but is no longer used after the pieChartData refactor → Remove the aliasMap useMemo block

  • ⚠️ Silent NaN in pie data: Number.parseFloat(rawValue) when rawValue is null/non-numeric produces NaN, which is passed as a pie slice value with no guard → Add .filter(entry => isFinite(entry.value)) before the sort

  • ⚠️ Unsafe select[0] access in packages/api/src/routers/external-api/v2/utils/dashboards.ts pie case: convertToExternalSelectItem(config.select[0]) is called without checking if the array is empty (e.g. if internal state is somehow corrupted) → Add a fallback: config.select.length ? convertToExternalSelectItem(config.select[0]) : DEFAULT_SELECT_ITEM

✅ The type-exhaustive approach for chart type conversions (ensuring a TS error when new types are added) is a solid pattern. Tests cover the round-trip well.

@github-actions
Copy link
Contributor

github-actions bot commented Feb 20, 2026

PR Review

Note: This PR is a draft.

Overall the implementation is clean and well-structured. A few items to address:

  • ⚠️ convertToPieChartConfig doesn't trim to 1 series → The PR states pie charts are limited to 1 series, but ChartUtils.tsx:1117 only strips granularity. If a user switches from a multi-series chart to pie, extra series persist in the saved config (only the first is rendered). Either trim select to 1 item in convertToPieChartConfig, or remove extra series in DBEditTimeChartForm when switching to pie tab.

  • ⚠️ ChartTooltip.tsx indicator='none' still renders an empty <div>ChartTooltipItem at line 64 always wraps the indicator in a <div>, even when indicator === 'none'. The empty div adds unwanted gap/padding in the pie chart tooltip. Move the <div> wrapper inside the conditional.

  • ℹ️ convertToInternalTileConfig default fallback → The internalConfig = {} as SavedChartConfig at line 308 is a type assertion hack for a branch that should never execute. Acceptable given the comment and satisfies never guard, but worth noting it would silently save a broken tile if somehow triggered.

  • CopyableLegendItem removal is clean (confirmed not referenced anywhere)

  • ✅ Exhaustiveness check via satisfies never pattern is a solid improvement for future chart type safety

  • ✅ Test coverage for external API round-trip looks thorough

@pulpdrew pulpdrew force-pushed the drew/pie-chart-fixes branch from a384913 to e851b3f Compare February 20, 2026 21:06
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moving this from HDXMultiSeriesTimeChart to a shared location for re-use in the pie chart.

@github-actions
Copy link
Contributor

github-actions bot commented Feb 20, 2026

PR Review: feat: Improve pie chart implementation

  • ⚠️ Missing unit tests (PR TODO acknowledged) → The PR description notes missing unit tests for pie chart data transformation; this is a DRAFT, but tests should be added before merge.

  • ⚠️ Order-dependent test assertion in dashboards.test.ts:190tiles[5] relies on tile insertion order being preserved through the round-trip; consider using .find(t => t.config.displayType === 'pie') for resilience.

  • ⚠️ Runtime escape hatch in convertToInternalTileConfig (utils/dashboards.ts) → The default branch assigns {} as SavedChartConfig as a fallback after the satisfies never exhaustiveness check. The cast is safe at compile time, but if schema mismatches occur at runtime (e.g. data from an older DB record), this silently returns a broken config. Consider throwing/logging instead.

  • ✅ The satisfies never exhaustiveness pattern in convertToExternalTileChartConfig is a nice improvement — will catch future display types at compile time.

  • ✅ No security issues found. Color handling, tooltip, and external API changes all look correct.

This is a DRAFT PR — no critical blockers beyond the missing tests.

@pulpdrew pulpdrew force-pushed the drew/pie-chart-fixes branch from e851b3f to e413120 Compare February 20, 2026 21:18
@pulpdrew pulpdrew marked this pull request as ready for review February 20, 2026 21:21
@github-actions
Copy link
Contributor

github-actions bot commented Feb 20, 2026

PR Review

✅ No critical issues found.

A few minor observations:

  • ⚠️ In convertToExternalTileChartConfig (utils/dashboards.ts): if config.select is an array but empty (e.g. due to a malformed internal record), config.select[0] will be undefined and passed to convertToExternalSelectItem. The Zod schema enforces length(1) on inbound API requests, but internally-saved documents have no such guarantee. Worth adding a guard or fallback to DEFAULT_SELECT_ITEM to match the pattern used in other chart types.

  • The default: config.displayType satisfies never exhaustiveness check in convertToExternalTileChartConfig is a good addition — will catch missing cases at compile time when new DisplayType values are added.

  • The CopyableLegendItem removal from HDXMultiSeriesTimeChart.tsx is clean — confirmed it has no other usages.

  • getColorProps signature (index, level) => string correctly matches the formatResponseForPieChart callback contract.

Overall this is a clean, well-tested improvement. The tooltip extraction to ChartTooltip.tsx, sorting by value, and consistent color assignment are all solid additions.

🤖 Generated with Claude Code

@github-actions
Copy link
Contributor

E2E Test Results

All tests passed • 70 passed • 4 skipped • 842s

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

Tests ran across 4 shards in parallel.

View full report →

@pulpdrew pulpdrew requested a review from knudtty February 20, 2026 22:39
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.

1 participant