Skip to content

feat(app): add shared ChartCard component and migrate ChartBox usages - #2829

Open
elizabetdev wants to merge 5 commits into
mainfrom
elizabet/chart-card-migration
Open

feat(app): add shared ChartCard component and migrate ChartBox usages#2829
elizabetdev wants to merge 5 commits into
mainfrom
elizabet/chart-card-migration

Conversation

@elizabetdev

Copy link
Copy Markdown
Contributor

Summary

  • Introduce ChartCard (packages/app/src/components/charts/ChartCard.tsx), a reusable card wrapper that gives standalone charts the same treatment as a custom dashboard tile — a bordered surface with a full-bleed header divider under the title. It's built on the exact primitives dashboard tiles already use (ChartContainerCardHeaderProvider + DASHBOARD_TILE_PADDING_INLINE), so the look stays in sync automatically.
  • Migrate the Service Dashboards (HTTP, Database, Errors, endpoint + DB-query side panels) and the ClickHouse page from the old plain ChartBox wrapper to ChartCard, and delete ChartBox.
  • Add a Storybook story (Charts/ChartCard) with Default, WithToolbar, RedRow, and NoHeader variants (dependency-free fake chart, no ClickHouse needed).
  • Document usage in agent_docs/code_style.md (when to use it, props, ✅/❌ examples, the height/flex requirement).

Why

ChartBox was a minimal bordered box with no header treatment, so charts on the Service Dashboards / ClickHouse page looked noticeably different from custom-dashboard tiles. ChartCard unifies these surfaces on one shared component with the preferred dashboard-tile look. It intentionally ships the visual chrome only — the tile-level controls (fullscreen, display switcher, kebab) stay on the dashboard Tile, which remains the grid-cell superset and is deliberately left untouched.

Test plan

  • Storybook: Charts/ChartCard renders all four variants correctly (divider, toolbar action, 3-up RED row, no-header degrade).
  • Service Dashboard (HTTP / Database / Errors tabs + endpoint & DB-query side panels): charts render as bordered cards with header dividers; tables that scrolled still scroll.
  • ClickHouse page: all ~12 chart cards render at their existing heights.
  • Visual parity check against a custom dashboard tile header.

Notes: tsc --noEmit and ESLint pass clean. Changeset included (@hyperdx/app patch). TraceRedMetricsChart (search-page RED charts) is intentionally excluded — it lives on the unmerged alex/red-metrics-trace-search branch and will adopt ChartCard there.

Made with Cursor

Introduce ChartCard (components/charts/), which gives standalone charts the
same card treatment as custom dashboard tiles (bordered surface + full-bleed
header divider) by reusing ChartContainerCardHeaderProvider and
DASHBOARD_TILE_PADDING_INLINE. Migrate the Service Dashboards (HTTP, Database,
Errors, endpoint and DB-query side panels) and the ClickHouse page off the old
ChartBox wrapper so chart surfaces look consistent across the app. Add a
Storybook story and document usage in agent_docs/code_style.md.

Co-authored-by: Cursor <cursoragent@cursor.com>
@changeset-bot

changeset-bot Bot commented Aug 6, 2026

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 1c25b9b

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

vercel Bot commented Aug 6, 2026

Copy link
Copy Markdown

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

Project Deployment Actions Updated (UTC)
hyperdx-oss Ready Ready Preview Aug 11, 2026 1:38pm
hyperdx-storybook Ready Ready Preview Aug 11, 2026 1:38pm

Request Review

@greptile-apps

greptile-apps Bot commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

Introduces a shared ChartCard surface and migrates standalone Service Dashboard and ClickHouse charts from ChartBox.

  • Uses dashboard-style headers and full-bleed dividers for standalone chart cards.
  • Routes bespoke table headings through ChartContainer.
  • Adds component tests, Storybook examples, usage documentation, and a patch changeset.

Confidence Score: 5/5

The PR appears safe to merge.

No blocking failure remains.

Important Files Changed

Filename Overview
packages/app/src/components/charts/ChartCard.tsx Defines the shared card chrome and card-header context; the previously reported headerless-spacing path is no longer used by current production callers.
packages/app/src/components/ServiceDashboardSlowestEventsTile.tsx Replaces the custom bare heading with a titled ChartContainer so the card receives header spacing and a divider.
packages/app/src/ClickhousePage.tsx Migrates ClickHouse charts to ChartCard and gives the Slowest Queries table a standard ChartContainer header.
packages/app/src/components/charts/ChartCard.stories.tsx Demonstrates titled chart, toolbar, multi-card, and table-card layouts without retaining the problematic headerless example.
packages/app/src/components/charts/tests/ChartCard.test.tsx Covers card-mode header activation, standalone behavior, style merging, and pinned divider padding.

Reviews (5): Last reviewed commit: "Merge branch 'main' into elizabet/chart-..." | Re-trigger Greptile

// Match the dashboard tile: no top padding (the card header supplies its
// own), slim bottom padding, and horizontal padding pinned to the tile
// inset so the header divider bleeds to the card edges.
paddingTop: 0,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P2 Headerless cards lose top spacing

paddingTop: 0 assumes a ChartContainer header will supply the top inset, but SlowestEventsTile renders a custom header and the documented NoHeader variant renders no header. Their content therefore sits flush against the top border instead of retaining the previous vertical spacing, making these cards inconsistent with titled chart cards.

Knowledge Base Used: App Components and Charts

Fix in Claude Code Fix in Conductor Fix in Cursor Fix in Codex

@github-actions

github-actions Bot commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

<!-- deep-review -->

Deep Review

✅ No critical issues found. This is a presentational extraction with no data, query, or auth surface touched — the migrated call sites pass their config objects through unchanged.

🟡 P2 — recommended

  • packages/app/src/ClickhousePage.tsx:799 — The "Slowest Queries" card passes a bare <Text> plus DBSqlRowTable as children instead of a ChartContainer with a title, so no card header renders and the label sits flush against the card's top border.
    • Fix: Pass the label through a ChartContainer title="Slowest Queries" so the header row and divider render, or make ChartCard restore top padding when no header is present.
  • packages/app/src/components/charts/ChartCard.tsx:31style is declared as BoxComponentProps['style'] (Mantine v9 MantineStyleProp, which also admits a resolver function and an array), but the implementation only object-spreads it, so a function-valued style is silently dropped and an array becomes numeric garbage keys.
    • Fix: Narrow the prop to React.CSSProperties, or resolve MantineStyleProp before merging instead of spreading it directly.
  • packages/app/src/components/charts/ChartCard.tsx:25 — The component's whole purpose — providing the context that makes a nested ChartContainer draw its header divider, plus the caller style override path — has no test asserting it, and the four Storybook stories are bare render functions with no play block or assertions.
    • Fix: Add a Jest/Testing Library test asserting that a ChartContainer with a title nested under ChartCard renders the divider header and that a caller style value wins over the defaults.
🔵 P3 nitpicks (4)
  • packages/app/src/components/charts/ChartCard.tsx:51 — The docstring states the horizontal padding must equal DASHBOARD_TILE_PADDING_INLINE or the full-bleed divider misaligns, yet ...style is spread last so any caller can override paddingInline and break that invariant.
    • Fix: Re-apply paddingInline: DASHBOARD_TILE_PADDING_INLINE after the spread so the invariant is enforced rather than only documented.
  • agent_docs/code_style.md:178 — The new prop table documents style as CSSProperties while the code declares BoxComponentProps['style'], and line 169 claims the component "draws the header divider" when the divider only appears if the nested ChartContainer receives a title or toolbarItems.
    • Fix: Correct the documented prop type and state that the divider requires a titled or toolbar-bearing ChartContainer child.
  • packages/app/src/components/charts/ChartCard.tsx:29 — Props are declared as an inline anonymous object type, while the repo's own guide in this same PR says to define TypeScript interfaces for props.
    • Fix: Extract and export a named ChartCardProps interface.
  • packages/app/src/ClickhousePage.tsx:802 — This call site passes overflow: 'hidden', which ChartCard already sets as its default.
    • Fix: Drop the redundant overflow override.

Reviewers (0 of 6 returned): correctness, adversarial, kieran-typescript, testing, maintainability, project-standards were dispatched but had not returned results at report time — every finding above was verified directly by the orchestrator against the file contents, not merged from reviewer output.

Environment caveat: Bash, Grep, and Glob were all non-functional in this checkout (bwrap: Can't create file at /home/.mcp.json), and node_modules is not installed. No git diff was obtainable; scope was reconstructed by reading files at known paths.

Testing gaps:

  • Exhaustive ChartBox reference removal is unverified — with no grep, a lingering import or a test asserting on markup the old wrapper produced could not be ruled out.
  • Overflow parity on the two cards that override to overflow: 'auto' (packages/app/src/ServicesDashboardPage/HttpTab.tsx:462, packages/app/src/ServicesDashboardPage/DatabaseTab.tsx:334) is unverified, since the deleted ChartBox source was not reachable for comparison.
  • packages/app/src/components/ServiceDashboardEndpointSidePanel.tsx and packages/app/src/components/ServiceDashboardDbQuerySidePanel.tsx were not read directly and are uncovered by this pass.
  • No test consumes the data-testid values added at HttpTab.tsx:401, :431, and :572, so the new test hook is currently unexercised.

@github-actions github-actions Bot added the review/tier-3 Standard — full human review required label Aug 7, 2026
@github-actions

github-actions Bot commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

🟡 Tier 3 — Standard

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

Why this tier:

  • Diff size: 502 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: 11
  • Production lines changed: 502 (+ 58 in test files, excluded from tier calculation)
  • Branch: elizabet/chart-card-migration
  • Author: elizabetdev

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

github-actions Bot commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

E2E Test Results

All tests passed • 277 passed • 1 skipped • 973s

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

Tests ran across 4 shards in parallel.

View full report →

@github-actions

github-actions Bot commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

Deep Review

No critical issues found. This is a clean, scope-contained visual refactor: a shared ChartCard wrapper replaces ChartBox across the Service Dashboards and ClickHouse page, ChartBox is fully removed (no dangling references), and the changeset/docs/story follow existing conventions. The change-sensitive mechanics were each traced and verified safe — the caller overflow: 'auto' override survives the base overflow: 'hidden' because ...style spreads after it, the full-bleed header's negative marginInline exactly cancels the pinned paddingInline so it cannot overflow horizontally, and recharts/Mantine overlays escape the new overflow: 'hidden' default via portals (the same pattern dashboard tiles already ship). The residual items below are all about test coverage for the behavior this PR changed.

🟡 P2 -- recommended

  • packages/app/src/components/charts/ChartCard.tsx:57 -- The migration introduces overflow: 'hidden' as a new default and relies on callers' style={{ overflow: 'auto' }} surviving the spread to keep the query/endpoint list tables scrollable, but no test locks this precedence.
    • Fix: Add a test rendering <ChartCard style={{ overflow: 'auto' }}> and assert the resolved style.overflow is 'auto'.
    • testing, correctness, adversarial
  • packages/app/src/components/ServiceDashboardSlowestEventsTile.tsx:70 -- The loading/error/empty/loaded render tree was moved from direct ChartBox children under a manual Group header into a titled ChartContainer (now inside its absolute inset:0 wrapper), a real structural change that ships with no regression test.
    • Fix: Add render tests covering the loading, error, empty, and loaded states at the default height so the restructure is protected.
    • testing, adversarial, correctness
🔵 P3 nitpicks (4)
  • packages/app/src/components/charts/__tests__/ChartCard.test.tsx:62 -- The padding-invariant test asserts paddingInline .not.toBe('999px'), which also passes if the value resolved to empty/undefined or any wrong constant, so it does not actually lock the documented invariant.
    • Fix: Import DASHBOARD_TILE_PADDING_INLINE and assert paddingInline equals it.
  • packages/app/src/ClickhousePage.tsx:498 -- The virtualized DBSqlRowTable with renderRowDetails (CodeMirror) now renders inside ChartContainer's absolute inset:0 wrapper for the first time; tracing shows the wrapper supplies a definite height, but the row-detail scroll/clip behavior at height 400 is untested.
    • Fix: Manually verify the table scroll body and expanded CodeMirror scroll internally rather than being clipped, and add a rendering test for this combination.
  • packages/app/src/components/charts/__tests__/ChartCard.test.tsx:28 -- The divider tests pin the exact literal inline-style string 1px solid var(--color-border), duplicated between test and source, making them brittle to a border-value refactor.
    • Fix: Export the divider border value as a named constant from ChartContainer.tsx and reference it in the test.
  • packages/app/src/components/charts/ChartCard.tsx:68 -- ChartCard is documented as not dashboard-specific yet hard-codes the imported constant DASHBOARD_TILE_PADDING_INLINE, whose name signals a dashboard-tile-only scope even though it is now shared card chrome.
    • Fix: Rename to a scope-neutral name (e.g. CARD_PADDING_INLINE) or re-export it under a neutral alias when next touched.

Reviewers (8): correctness, testing, maintainability, project-standards, kieran-typescript, adversarial, agent-native, learnings-researcher.

Testing gaps:

  • No test asserts the caller overflow override survives the ChartCard base overflow: 'hidden' — the exact style-merge behavior the migration changed.
  • SlowestEventsTile and the ClickHouse "Slowest Queries" card place a scrolling DBSqlRow* table inside ChartContainer's absolute wrapper with no rendering test; stories use a dependency-free FakeChart only, so real table scroll/clip at fixed heights is unverified.
  • The overflow: 'auto' list cards (Database top-queries, HTTP endpoints, endpoint-performance) have no test exercising a >20-item DBListBarChart actually scrolling inside ChartCard.

Route the "Slowest Queries" (ClickHouse page) and Slowest Events tile
titles through a titled ChartContainer instead of a bare Text, so they
get the same full-bleed card header divider as sibling chart cards and
their tables are properly bounded by the reactive container.

Remove the headerless ChartCard prop (padding-only patch) in favor of
this approach, keep the ChartCardProps interface, narrowed style type,
and enforced paddingInline divider invariant, and add ChartCard tests.

Co-authored-by: Cursor <cursoragent@cursor.com>
@elizabetdev

Copy link
Copy Markdown
Contributor Author

@copilot resolve the merge conflicts in this pull request

…yle.md conflict)

Co-authored-by: elizabetdev <2750668+elizabetdev@users.noreply.github.com>

Copilot AI commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

@copilot resolve the merge conflicts in this pull request

Merged main into this branch and resolved the conflict in agent_docs/code_style.md — both the ChartCard section (from this PR) and the new "UI text: use sentence case" section (from main) are now preserved side by side.

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

Labels

review/tier-3 Standard — full human review required

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants