Skip to content

refactor(dashboards): extract shared Sparkline primitive from number tile#2520

Open
alex-fedotyev wants to merge 2 commits into
mainfrom
alex/HDX-1360-shared-sparkline
Open

refactor(dashboards): extract shared Sparkline primitive from number tile#2520
alex-fedotyev wants to merge 2 commits into
mainfrom
alex/HDX-1360-shared-sparkline

Conversation

@alex-fedotyev

Copy link
Copy Markdown
Contributor

Builds on the number-tile background sparkline (#2489, #2501) and sets up the table-cell trend follow-up tracked in HDX-4604.

Summary

Extracts the chrome-less recharts core out of NumberTileBackgroundChart into a reusable <Sparkline> component (line / area / bar) and renders the number tile's background trend through it. This is a pure consolidation: the number tile issues the same query and draws the same faint line / area behind the value, so there is no user-visible change.

The table-cell trend feature (HDX-4604) needs the identical render shell in every numeric cell. A shared primitive is the alternative to standing up a second chart component next to this one, which is also the reuse question Mike raised on #2489 ("any reason we couldn't reuse/extend the usual time chart instead?").

What

  • New packages/app/src/components/Sparkline.tsx: <Sparkline points type={'line' | 'area' | 'bar'} color height />. Chrome-less (no axes, grid, legend, or tooltip; dots and animation off), sized to fill its parent via ResponsiveContainer (height defaults to 100%). Renders nothing for fewer than two points. The line and area branches are lifted from NumberTileBackgroundChart unchanged; the bar branch follows the existing PatternTrendChart in DBRowTable and is here for the table-cell consumer that lands next.
  • NumberTileBackgroundChart.tsx now renders <Sparkline points={points} type={backgroundChart.type} color={color} /> inside its unchanged aria-hidden wrapper. Its data path (buildSparklineTimeConfig, convertToTimeChartConfig, useQueriedChartConfig, formatResponseForTimeChart) and the single-series sparklinePointsFromGraphResults helper are unchanged.

Why the helper stays put

sparklinePointsFromGraphResults is single-series and specific to the number tile's data path. The table render reconstructs per-group trends differently, so the helper would not be shared; it stays with the number tile. Only the genuinely shared pieces (the component and the SparklinePoint type) live in Sparkline.tsx. This keeps the existing NumberTileBackgroundChart tests in place and avoids a dead re-export.

No behavior change

The render is a verbatim lift, so the recharts output is identical. The diff on NumberTileBackgroundChart.tsx shows the same <Area> / <Line> props (stroke, strokeOpacity 0.5, strokeWidth 2, fillOpacity 0.15, dataKey, margin) moving into <Sparkline>, wrapped in the same ResponsiveContainer inside the same wrapper. The number tile passes the same resolved color and type it computed before, and keeps its own "fewer than two points" guard so the background layer is still absent when there is no trend.

Test plan

  • nx run @hyperdx/app:ci:lint (eslint + tsc) clean
  • nx run @hyperdx/app:ci:unit: 2091 pass, 0 fail. Includes the existing number-tile render-wiring test and the DBNumberChart consumer test, both unchanged and green.
  • New Sparkline.test.tsx: mounts the component and asserts the line / area / bar recharts layers render in the given color, and that it renders nothing below two points.
  • Dev stack builds and serves the branch (app returns 200), confirming the refactor compiles and runs in the Next.js build, not only under jest.

Visual note: this is a verbatim render extraction with no visual delta, so the proof of "renders identically" is the render-block diff plus the unchanged consumer test rather than a before/after screenshot. [ui-check: allow]

What's not in this PR (follow-up, HDX-4604)

  • The table-tile "show trend sparklines" display toggle and its schema field.
  • The per-cell scoped query and grouped cell render in DBTableChart, which consumes <Sparkline> (lands after feat(dashboards): per-column color on table tiles #2517 so the per-column color drives the sparkline color).
  • External API v2 / MCP parity and customer docs for the toggle.

No changeset: internal refactor with no user-facing behavior change. [no-changeset: allow]

…tile

Lift the chrome-less recharts core out of NumberTileBackgroundChart into
a reusable <Sparkline> (line / area / bar) and render the number tile's
background trend through it. No user-visible change: the number tile
issues the same query and renders the same faint line / area behind the
value.

The upcoming table-cell trend feature needs the identical render shell
per cell, so a shared primitive is the alternative to a second parallel
chart component. It also answers the reuse question raised on #2489.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@changeset-bot

changeset-bot Bot commented Jun 25, 2026

Copy link
Copy Markdown

⚠️ No Changeset found

Latest commit: f6449bd

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

vercel Bot commented Jun 25, 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, Comment Jun 25, 2026 2:19am
hyperdx-storybook Ready Ready Preview, Comment Jun 25, 2026 2:19am

Request Review

@github-actions github-actions Bot added the review/tier-2 Low risk — AI review + quick human skim label Jun 25, 2026
@github-actions

github-actions Bot commented Jun 25, 2026

Copy link
Copy Markdown
Contributor

🔵 Tier 2 — Low Risk

Small, isolated change with no API route or data model modifications.

Why this tier:

  • Standard feature/fix — introduces new logic or modifies core functionality

Review process: AI review + quick human skim (target: 5–15 min). Reviewer validates AI assessment and checks for domain-specific concerns.
SLA: Resolve within 4 business hours.

Stats
  • Production files changed: 2
  • Production lines changed: 142 (+ 88 in test files, excluded from tier calculation)
  • Branch: alex/HDX-1360-shared-sparkline
  • Author: alex-fedotyev

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

@greptile-apps

greptile-apps Bot commented Jun 25, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR extracts the recharts rendering core from NumberTileBackgroundChart into a standalone <Sparkline> component (line | area | bar) and threads the number tile's background chart through it, with no change to query logic, props, or visual output.

  • Sparkline.tsx: New chrome-less component; accepts typed points, type, color, and optional height; renders null for fewer than two points; wraps whichever recharts chart in a ResponsiveContainer sized to fill the parent.
  • NumberTileBackgroundChart.tsx: Inline recharts JSX (52 lines) replaced by a single <Sparkline> call; all data-path helpers, the two-point guard at line 140, and the aria-hidden wrapper are untouched.
  • Sparkline.test.tsx: Exercises all three variants and the null-render guard, with a mocked ResponsiveContainer that records the requested height so forwarding is directly assertable.

Confidence Score: 5/5

Safe to merge — this is a pure render extraction with no changes to data fetching, query logic, or existing guards.

The diff moves recharts JSX verbatim into a new component and replaces the original call site with a single delegating element. The two-point guard in NumberTileBackgroundChartInner and the aria-hidden wrapper are both untouched. The new Sparkline has its own matching guard as a safety net for standalone use. Tests cover all three chart variants and the null-render path, and the height-forwarding assertion uses the recorded mock array rather than an indirect DOM check.

No files require special attention.

Important Files Changed

Filename Overview
packages/app/src/components/Sparkline.tsx New chrome-less recharts sparkline primitive supporting line, area, and bar variants; correctly guards against fewer than two points and fills parent via ResponsiveContainer
packages/app/src/components/NumberTileBackgroundChart.tsx Rendering logic replaced by ; data path, existing two-point guard at line 140, and aria-hidden wrapper are all unchanged
packages/app/src/components/tests/Sparkline.test.tsx Covers all three chart variants, the fewer-than-two-points null path, default height, and explicit numeric height forwarding via the recorded mock array

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    NTBCOuter["NumberTileBackgroundChart\n(public, ErrorBoundary wrapper)"]
    NTBCInner["NumberTileBackgroundChartInner"]
    DataPath["Data path\nbuildSparklineTimeConfig → convertToTimeChartConfig\n→ useQueriedChartConfig → formatResponseForTimeChart\n→ sparklinePointsFromGraphResults"]
    Guard["points.length < 2?\n→ return null"]
    Sparkline["Sparkline\ntype: line | area | bar\ncolor, height"]
    RC["ResponsiveContainer\nwidth=100% height=height"]
    LineChart["LineChart"]
    AreaChart["AreaChart"]
    BarChart["BarChart"]

    NTBCOuter --> NTBCInner
    NTBCInner --> DataPath
    DataPath --> Guard
    Guard -->|"≥ 2 points"| Sparkline
    Sparkline --> RC
    RC -->|"type=line"| LineChart
    RC -->|"type=area"| AreaChart
    RC -->|"type=bar"| BarChart
Loading
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
    NTBCOuter["NumberTileBackgroundChart\n(public, ErrorBoundary wrapper)"]
    NTBCInner["NumberTileBackgroundChartInner"]
    DataPath["Data path\nbuildSparklineTimeConfig → convertToTimeChartConfig\n→ useQueriedChartConfig → formatResponseForTimeChart\n→ sparklinePointsFromGraphResults"]
    Guard["points.length < 2?\n→ return null"]
    Sparkline["Sparkline\ntype: line | area | bar\ncolor, height"]
    RC["ResponsiveContainer\nwidth=100% height=height"]
    LineChart["LineChart"]
    AreaChart["AreaChart"]
    BarChart["BarChart"]

    NTBCOuter --> NTBCInner
    NTBCInner --> DataPath
    DataPath --> Guard
    Guard -->|"≥ 2 points"| Sparkline
    Sparkline --> RC
    RC -->|"type=line"| LineChart
    RC -->|"type=area"| AreaChart
    RC -->|"type=bar"| BarChart
Loading

Reviews (2): Last reviewed commit: "test(dashboards): assert Sparkline forwa..." | Re-trigger Greptile

ResponsiveContainer,
} from 'recharts';

type SparklineType = 'line' | 'area' | 'bar';

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 SparklineType is not exported. Consumers who need to track or type the type prop in their own state will have to re-declare 'line' | 'area' | 'bar' themselves or use React.ComponentProps<typeof Sparkline>['type']. Given SparklinePoint is already exported and the follow-up DBTableChart consumer is the intended caller, exporting SparklineType alongside it would keep the public surface consistent.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Kept local on purpose for now. This package trims unused exports (knip reports unused exported types), and the only current consumer passes a value that is already typed by its own schema, so exporting it now would be a dead export. The table-cell consumer in the follow-up imports it, and the export lands with that change so it is never unused.

Comment thread packages/app/src/components/__tests__/Sparkline.test.tsx Outdated
@github-actions

github-actions Bot commented Jun 25, 2026

Copy link
Copy Markdown
Contributor

E2E Test Results

All tests passed • 217 passed • 3 skipped • 1494s

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

Tests ran across 4 shards in parallel.

View full report →

The height test only checked that the chart still rendered; capture the
height passed to ResponsiveContainer and assert it (the 100% default and
an explicit numeric height), so the prop's contract is actually covered.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

automerge review/tier-2 Low risk — AI review + quick human skim

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant