Skip to content

fix(app): pulse the list-bar and histogram charts while they refresh - #3229

Merged
kodiakhq[bot] merged 6 commits into
ClickHouse:mainfrom
Harshul1484:fix/services-tiles-refresh-pulse
Oct 5, 2026
Merged

kodiakhq[bot] merged 6 commits into
ClickHouse:mainfrom
Harshul1484:fix/services-tiles-refresh-pulse

Conversation

@Harshul1484

@Harshul1484 Harshul1484 commented Sep 25, 2026 •

Copy link
Copy Markdown
Contributor

Summary

A follow-up to #3205, for the Services dashboard. Two chart types there already keep their previous data during a refresh (placeholderData: prev => prev) but give no sign that new data is loading: the "Top 20 Most Time Consuming Endpoints/Queries" list-bar charts and the "Request Latency" histogram. They now pulse while the refetch runs, like the categorical charts fixed in #3211.

  • DBListBarChart pulses the bar list, and its empty state, while isPlaceholderData is true.
  • DBHistogramChart does the same through a new className prop on HistogramChart, which is applied to its ResponsiveContainer.
  • DBHistogramChart's loading state now shows only before the first data arrives (isLoading && !data), as DBListBarChart and DBBarChart already do. useQueriedChartConfig also reports isLoading while the materialized-view check for the new range runs, which briefly replaced the kept histogram with "Loading Chart Data...".
  • DBListBarChart is also used by the LLM token-cost charts and the endpoint side panel's performance chart, which now pulse the same way.

Tests

  • DBListBarChart.test.tsx:
    • the bars pulse during a refresh and stop once fresh data loads;
    • the empty state pulses during a refresh, but not for a fresh empty result.
  • DBHistogramChart.test.tsx:
    • the histogram stays on screen and pulses while a refresh loads, and stops once fresh data loads;
    • the empty state pulses during a refresh, but not for a fresh empty result;
    • the loading state still shows before the first data.
  • Without the source changes, 4 of the 9 new tests fail. The ones that pass either way are the guards: no pulse once fresh, and the loading state before the first data.

How to test on Vercel preview

Preview routes: /services

Steps:

  1. Open the Services dashboard on the demo trace source. On the "HTTP Service" tab, switch "Request Latency" to "Display as Histogram".
  2. Click "Refresh dashboard".
  3. Verify the "Request Latency" histogram and the "Top 20 Most Time Consuming Endpoints" list keep their data on screen and pulse while they reload, and stop pulsing once the new data appears.

References

Checks run locally:

  • jest --findRelatedTests for both charts: 33 passed across 4 suites.
  • eslint on the changed files: 0 errors, no new warnings.
  • prettier --check.
  • tsc --noEmit for packages/app.

@changeset-bot

changeset-bot Bot commented Sep 25, 2026 •

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 7cef5c2

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 Sep 25, 2026

Copy link
Copy Markdown

@Harshul1484 is attempting to deploy a commit to the HyperDX Team on Vercel.

A member of the Team first needs to authorize it.

@github-actions

github-actions Bot commented Sep 25, 2026 •

Copy link
Copy Markdown
Contributor

PR Review

✅ No issues found.

@greptile-apps

greptile-apps Bot commented Sep 25, 2026 •

Copy link
Copy Markdown
Contributor

RetriggerConfidence Score: 5/5

[Low risk] Adds visual feedback while charts refresh.

The PR appears safe to merge; no new actionable issue or outstanding previous finding remains.

Summary

The PR keeps previous list-bar and histogram data visible during refresh and pulses the displayed chart or empty state until fresh results arrive. It also adds focused refresh-state tests.

Reviews (6) · Last reviewed commit: "Merge branch 'main' into fix/services-ti..."

@github-actions

github-actions Bot commented Sep 25, 2026 •

Copy link
Copy Markdown
Contributor

Deep Review

✅ No critical issues found.

This is a small, well-scoped follow-up that adds effect-pulse refresh feedback to DBListBarChart and DBHistogramChart, keyed off react-query's isPlaceholderData, with matching tests. No correctness, security, data, or contract regressions were identified in the diff. The state transitions are coherent: when isLoading is true with placeholder data present, react-query reports isPlaceholderData true, so the kept chart both renders (via the new isLoading && !data gate) and pulses.

🔵 P3 nitpicks (3)
  • packages/app/src/components/DBHistogramChart.tsx:1 — the file is now 308 lines and DBListBarChart.tsx is 309, both over the AGENTS.md "keep component files under 300 lines" guideline; this repeats a prior review comment on the PR that remains unaddressed.
    • Fix: extract the inner HistogramChart/ListBar presentational components (or the tooltip) into sibling files to bring both back under the guideline, or confirm the guideline is advisory for a marginal overage.
  • packages/app/src/components/DBHistogramChart.tsx:303 — the chart pulse is keyed on isPlaceholderData alone, whereas sibling charts (DBBarChart, DBPieChart, DBNumberChart) key it on isLoading || isPlaceholderData; the behavior is equivalent in practice here but the divergence is a consistency snag for future readers.
    • Fix: align on isLoading || isPlaceholderData for the chart pulse to match the established pattern, or leave a short comment explaining why isPlaceholderData alone is sufficient given the new isLoading && !data gate.
  • packages/app/src/components/__tests__/DBHistogramChart.test.tsx:161 — the refresh tests assert against document.querySelector('.recharts-responsive-container'), coupling the test to a Recharts-internal DOM class that could change on a library upgrade.
    • Fix: accept the coupling as low-risk, or target a stable app-owned wrapper/data-testid instead of the library class.

Reviewers (7): correctness, testing, maintainability, project-standards, kieran-typescript, julik-frontend-races, previous-comments.

Testing gaps: No test covers the list-bar chart pulsing while isLoading is true with kept data (only the isPlaceholderData-true path is asserted); the histogram covers this case but the list-bar does not.

No finding blocks merge automatically. A maintainer will expect P0/P1 findings in code this PR changes to be fixed; P2/P3 are your call -- fix or reply. Never fix findings about surrounding code here; reply instead. Do not widen the PR. How to respond

@vercel

vercel Bot commented Oct 5, 2026 •

Copy link
Copy Markdown

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

Project Deployment Actions Updated
hyperdx-oss Ready Ready Preview Oct 5, 2026 1:44pm UTC
hyperdx-storybook Ready Ready Preview Oct 5, 2026 1:44pm UTC

Request Review

@teeohhem

teeohhem commented Oct 5, 2026

Copy link
Copy Markdown
Contributor

Thanks for the PR! This looks great.

@kodiakhq
kodiakhq Bot merged commit c372a71 into ClickHouse:main Oct 5, 2026
26 of 28 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants