Skip to content

chore: Make dashboard tile error state consistent across chart types#2404

Merged
kodiakhq[bot] merged 4 commits into
mainfrom
cursor/consistent-chart-error-states-6404
Jun 3, 2026
Merged

chore: Make dashboard tile error state consistent across chart types#2404
kodiakhq[bot] merged 4 commits into
mainfrom
cursor/consistent-chart-error-states-6404

Conversation

@pulpdrew
Copy link
Copy Markdown
Contributor

@pulpdrew pulpdrew commented Jun 2, 2026

Summary

This PR updates each chart type to have a consistent error state, using the shared ChartErrorState component. Further, the variant of the component now matches ("collapsible" when the chart is a dashboard tile, "inline" when editing the chart or viewing on the search page).

Before:

Screenshot 2026-06-02 at 10 16 19 AM

After:

Screen.Recording.2026-06-03.at.8.16.17.AM.mov

How to test on Vercel preview

This can be tested in the preview environment - any invalid SQL WHERE filter should trigger a query failure and cause an error state, on dashboards or elsewhere.

References

  • Linear Issue: Closes HDX-4430

Replace custom error rendering in DBHeatmapChart, DBTableChart,
DBHistogramChart, DBListBarChart, and ServiceDashboardSlowestEventsTile
with the shared ChartErrorState component. Add errorVariant prop to each
chart component so callers can choose between collapsible (modal button,
default for dashboard tiles) and inline (expanded details, used in chart
explorer and tile editor).

Update ChartPreviewPanel to pass errorVariant='inline' to DBTableChart
and DBHeatmapChart, matching the existing pattern for DBTimeChart,
DBNumberChart, and DBPieChart.

Resolves HDX-4430

Co-authored-by: Drew Davis <pulpdrew@gmail.com>
@changeset-bot
Copy link
Copy Markdown

changeset-bot Bot commented Jun 2, 2026

🦋 Changeset detected

Latest commit: e3ab80b

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

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

vercel Bot commented Jun 2, 2026

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

Project Deployment Actions Updated (UTC)
hyperdx-oss Ready Ready Preview, Comment Jun 3, 2026 2:00pm
hyperdx-storybook Ready Ready Preview, Comment Jun 3, 2026 2:00pm

Request Review

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Jun 2, 2026

E2E Test Results

All tests passed • 196 passed • 3 skipped • 1304s

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

Tests ran across 4 shards in parallel.

View full report →

@pulpdrew pulpdrew changed the title Make dashboard tile error state consistent across chart types chore: Make dashboard tile error state consistent across chart types Jun 3, 2026
@pulpdrew pulpdrew marked this pull request as ready for review June 3, 2026 12:21
@github-actions github-actions Bot added the review/tier-3 Standard — full human review required label Jun 3, 2026
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Jun 3, 2026

🟡 Tier 3 — Standard

Introduces new logic, modifies core functionality, or touches areas with non-trivial risk.

Why this tier:

  • Diff size: 280 production lines changed (Tier 2 max: < 250)

Review process: Full human review — logic, architecture, edge cases.
SLA: First-pass feedback within 1 business day.

Stats
  • Production files changed: 9
  • Production lines changed: 280
  • Branch: cursor/consistent-chart-error-states-6404
  • Author: pulpdrew

To override this classification, remove the review/tier-3 label and apply a different review/tier-* label. Manual overrides are preserved on subsequent pushes.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Jun 3, 2026

Deep Review

✅ No critical issues found.

This is a focused, low-risk frontend refactor: each chart type's error branch is routed through the shared ChartErrorState component, and an errorVariant prop ("collapsible" for dashboard tiles, "inline" for edit/search) is threaded through the chart and table components. No P0/P1 issues. The most-cited concern — an alleged new TypeScript compile error from passing error={error} to a non-nullable prop without an && error guard — was verified against the base SHA and does not hold: the pre-PR code in the identical isError ? branch already dereferenced error.message non-optionally and shipped, the hook types error as ClickHouseQueryError | Error, and the null-deref path is not constructible under TanStack Query v5. It is a consistency nit, not a breakage.

🟡 P2 — recommended

  • packages/app/src/components/charts/ChartErrorState.tsx:14variant defaults to 'collapsible', so every chart call site that does not explicitly pass errorVariant (DBHistogramChart, DBListBarChart, DBTableChart, DBHeatmapChart) now renders the collapsed modal flow where the pre-refactor markup always showed error details inline, a real behavior change for untouched call sites.
    • Fix: Audit non-tile chart call sites and pass errorVariant="inline" where inline error detail is intended, confirming the collapsible default is correct everywhere else.
    • maintainability, correctness
🔵 P3 nitpicks (3)
  • packages/app/src/components/DBHistogramChart.tsx:277 — Error branch uses bare isError ? while DBTableChart and DBRowTable use isError && error ?; same pattern in DBListBarChart.tsx:279 and ServiceDashboardSlowestEventsTile.tsx:79. Behavior is equivalent here, so this is consistency only.

    • Fix: Standardize on isError && error ? across the chart components for a uniform guard.
  • packages/app/src/components/ServiceDashboardSlowestEventsTile.tsx:79 — Renders <ChartErrorState error={error} /> with no errorVariant, hardcoding the collapsible variant; unlike every other updated component it exposes no prop to override it.

    • Fix: Add an errorVariant prop to SlowestEventsTile and thread it through, matching the other chart components.
  • packages/app/src/components/charts/ChartErrorState.tsx:73 — The useDisclosure modal state lives inside ChartErrorState, which is mounted only while isError is true; if the query recovers while the modal is open the component unmounts and the modal disappears abruptly rather than closing through onClose.

    • Fix: Lift the open/close state to the parent that owns isError and pass it in, or accept the minor UX glitch as Mantine cleans up the portal on unmount.

Reviewers (9): correctness, kieran-typescript, maintainability, testing, adversarial, julik-frontend-races, project-standards, agent-native, learnings-researcher.

Testing gaps:

  • No unit test for ChartErrorState's collapsible vs inline branching or its ClickHouseQueryError "Sent Query" path.
  • No test covers the changed DBRowTable error branch (isError && error ?) or the per-call-site errorVariant selection across the updated chart components.

Coverage notes:

  • Pre-existing (not introduced here, excluded from verdict): useQueriedChartConfig returns { ...query, isLoading }, which collapses the TanStack Query discriminated union and weakens error narrowing for all consumers — the root cause of the guard-style discussion above.
  • docs/solutions/ does not exist; no prior learnings applied. No agent-native parity gap (pure presentation change). No project-standards violations found.

@pulpdrew pulpdrew requested review from a team and knudtty and removed request for a team June 3, 2026 12:33
Copy link
Copy Markdown
Contributor

@knudtty knudtty left a comment

Choose a reason for hiding this comment

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

LGTM. Great way to decrease debt

@kodiakhq kodiakhq Bot merged commit a6e7dcd into main Jun 3, 2026
19 checks passed
@kodiakhq kodiakhq Bot deleted the cursor/consistent-chart-error-states-6404 branch June 3, 2026 14:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

automerge review/tier-3 Standard — full human review required

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants