Skip to content

fix: Fix missing bar chart bar when there is only one bar#2300

Open
pulpdrew wants to merge 2 commits into
mainfrom
drew/fix-bar-chart-one-bar
Open

fix: Fix missing bar chart bar when there is only one bar#2300
pulpdrew wants to merge 2 commits into
mainfrom
drew/fix-bar-chart-one-bar

Conversation

@pulpdrew
Copy link
Copy Markdown
Contributor

Summary

This PR fixes a bug which caused the bar chart to show no bars when there should be exactly 1 bar.

Screenshots or video

Screenshot 2026-05-18 at 3 33 51 PM Screenshot 2026-05-18 at 3 33 43 PM

Closes HDX-4230

@vercel
Copy link
Copy Markdown

vercel Bot commented May 18, 2026

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

Project Deployment Actions Updated (UTC)
hyperdx-oss Ready Ready Preview, Comment May 18, 2026 8:34pm

Request Review

@changeset-bot
Copy link
Copy Markdown

changeset-bot Bot commented May 18, 2026

🦋 Changeset detected

Latest commit: 3e8797c

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

@github-actions github-actions Bot added the review/tier-2 Low risk — AI review + quick human skim label May 18, 2026
@github-actions
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: 1
  • Production lines changed: 22
  • Branch: drew/fix-bar-chart-one-bar
  • Author: pulpdrew

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

@github-actions
Copy link
Copy Markdown
Contributor

PR Review

  • ⚠️ setContainerWidth(w) inside onResize triggers a re-render on every resize event (the original kept size in a ref to avoid this) → Only call setContainerWidth when the value actually changes, or gate it on displayType === DisplayType.StackedBar && graphResults.length === 1 so the common path stays ref-only.
  • ⚠️ Magic number 50 in containerWidth - 50 is decoupled from the actual YAxis width={40} declared at line 701 → Reference a shared constant (e.g., Y_AXIS_WIDTH = 40) used by both sites so the bar sizing stays correct if the axis width is ever tuned.

Minor: the {...(singlePointBarSize !== undefined ? { barSize: singlePointBarSize } : {})} spread can simply be barSize={singlePointBarSize} — Recharts already treats undefined as "unset" and auto-calculates.

@github-actions
Copy link
Copy Markdown
Contributor

Deep Review

🟡 P2 -- recommended

  • packages/app/src/HDXMultiSeriesTimeChart.tsx:476-490 -- containerWidth is initialized to 0, so on first render of a single-point StackedBar chart drawableWidth = max(0, 0 - 50) = 0, singlePointBarSize returns undefined, and Recharts paints the same zero-width bar this PR is meant to fix until ResponsiveContainer.onResize fires and triggers a second render.
    • Fix: Seed containerWidth synchronously via a ref + useLayoutEffect measurement of the wrapping element so the first paint already has a usable width.
    • correctness, julik-frontend-races
  • packages/app/src/HDXMultiSeriesTimeChart.tsx -- No test covers the regression: MemoChart is exported but there is no HDXMultiSeriesTimeChart.test.tsx, and sibling charts under packages/app/src/components/__tests__/ (e.g. DBTimeChart.test.tsx) demonstrate the prop-assertion pattern via a mocked Recharts component.
    • Fix: Add a test that mocks BarChart, renders MemoChart with displayType=StackedBar and graphResults.length===1, drives ResponsiveContainer.onResize with a known width, and asserts barSize is a positive number; include negative cases for length===2 and displayType!==StackedBar.
    • testing
🔵 P3 nitpicks (4)
  • packages/app/src/HDXMultiSeriesTimeChart.tsx:487,489 -- The literals 50 (YAxis + padding) and 0.8 (bar-to-drawable ratio) are unlabeled; the inline comment says YAxis reserves 40px while the code subtracts 50, leaving the implicit 10px padding unstated.
    • Fix: Extract named constants like Y_AXIS_RESERVE_PX = 40, RIGHT_PADDING_PX = 10, and SINGLE_BAR_WIDTH_RATIO = 0.8 so the comment and code stay aligned.
    • correctness, maintainability
  • packages/app/src/HDXMultiSeriesTimeChart.tsx:571-573 -- The conditional spread {...(singlePointBarSize !== undefined ? { barSize: singlePointBarSize } : {})} is functionally equivalent to passing barSize={singlePointBarSize} directly because Recharts treats an explicit undefined the same as an omitted prop.
    • Fix: Replace the spread with barSize={singlePointBarSize}.
    • correctness, maintainability
  • packages/app/src/HDXMultiSeriesTimeChart.tsx:558-562 -- setContainerWidth(w) now runs on every ResponsiveContainer.onResize tick, triggering a MemoChart re-render during parent resizes even for the non-StackedBar or multi-point cases that do not consume containerWidth.
    • Fix: Gate the state update with if (displayType === DisplayType.StackedBar && graphResults.length === 1) so other code paths retain the previous ref-only behavior.
    • correctness, julik-frontend-races
  • packages/app/src/HDXMultiSeriesTimeChart.tsx:483-490 -- The containerWidth - 50 calculation implicitly relies on the chart being rendered without an explicit margin prop, so any future margin={...} change would silently shift the bar out of the plot area.
    • Fix: Reference the chart's actual margin (or extract it to a shared constant used by both the <ChartComponent> and this calculation) so they cannot drift independently.
    • correctness

Reviewers (4): correctness, testing, maintainability, julik-frontend-races.

Testing gaps:

  • No test asserts barSize is forwarded when displayType=StackedBar and graphResults.length===1.
  • No test pins the negative cases (length===2 or displayType=Line) so the gating logic in singlePointBarSize can regress silently.
  • No test exercises the boundary where containerWidth <= 50 and the memo returns undefined.

@github-actions
Copy link
Copy Markdown
Contributor

E2E Test Results

All tests passed • 177 passed • 3 skipped • 1237s

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

Tests ran across 4 shards in parallel.

View full report →

@pulpdrew
Copy link
Copy Markdown
Contributor Author

@claude fix these:

⚠️ setContainerWidth(w) inside onResize triggers a re-render on every resize event (the original kept size in a ref to avoid this) → Only call setContainerWidth when the value actually changes, or gate it on displayType === DisplayType.StackedBar && graphResults.length === 1 so the common path stays ref-only.
⚠️ Magic number 50 in containerWidth - 50 is decoupled from the actual YAxis width={40} declared at line 701 → Reference a shared constant (e.g., Y_AXIS_WIDTH = 40) used by both sites so the bar sizing stays correct if the axis width is ever tuned.
Minor: the {...(singlePointBarSize !== undefined ? { barSize: singlePointBarSize } : {})} spread can simply be barSize={singlePointBarSize} — Recharts already treats undefined as "unset" and auto-calculates.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 18, 2026

Claude finished @pulpdrew's task in 5m 22s —— View job


Applied PR feedback

  • Read packages/app/src/HDXMultiSeriesTimeChart.tsx
  • Gate setContainerWidth so it only fires for single-point StackedBar (common path stays ref-only). Also using functional setState to bail out when width is unchanged.
  • Extract Y_AXIS_WIDTH = 40 constant; the <YAxis width={...}> prop and the singlePointBarSize calc now both reference it. Also extracted SINGLE_POINT_BAR_RIGHT_PADDING = 10 and SINGLE_POINT_BAR_WIDTH_RATIO = 0.8 so the comment no longer drifts from the code.
  • Replace the conditional {...({ barSize })} spread with barSize={singlePointBarSize} directly.
  • Commit and push (3e8797c9)

Notes

  • I did not run yarn lint:fix / make ci-lint locally — those yarn/npx commands are not on the allowlist in this environment so they required approval. The pre-commit hook (lint-staged) ran on commit. Existing CI should catch anything missed.
  • I left the P2 first-paint concern from the deep review (initial containerWidth=0 → one frame of blank chart before resize fires) and the testing gap unaddressed since they were outside the scope of this comment. Happy to tackle either in a follow-up if you want.
    • Branch: drew/fix-bar-chart-one-bar

- Extract Y_AXIS_WIDTH constant and use it in both YAxis width prop
  and the single-point bar size calculation so they stay in sync
- Also extract right-padding and width-ratio constants for clarity
- Gate setContainerWidth on the single-point StackedBar case so other
  display types stay ref-only on resize and avoid unnecessary re-renders
- Use functional setState to bail out when width is unchanged
- Replace conditional spread with direct barSize prop (Recharts treats
  undefined as unset)

Co-authored-by: Drew Davis <pulpdrew@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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