fix(BarChart): render stacked bars when all values are 0 (#6235)#7199
Conversation
WalkthroughThis PR modifies domain calculation logic for stacked bar charts to properly handle cases where all numeric values are zero. It updates selector and utility functions to change how they treat empty and zero-only stack groups, and adds corresponding test coverage. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related issues
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/chart/BarChart.spec.tsx`:
- Around line 1383-1390: The tests render BarChart components directly and can
leave timer-driven updates pending; after each render call (e.g., the
render(...) invocation that renders <BarChart ...> in BarChart.spec.tsx and the
similar render blocks referenced around the other ranges), add a call to
vi.runOnlyPendingTimers() immediately after the render to advance pending timers
(do not use vi.runAllTimers()). Ensure this is applied to the render locations
corresponding to the first snippet and the other two test blocks mentioned so
assertions won't race with pending timer callbacks.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 60d8f360-4416-4395-bb2a-3acbf082a982
📒 Files selected for processing (4)
src/state/selectors/axisSelectors.tssrc/util/ChartUtils.tstest/chart/BarChart.spec.tsxtest/util/ChartUtils.spec.tsx
| const { container } = render( | ||
| <BarChart width={400} height={300} data={allZeroData}> | ||
| <YAxis /> | ||
| <Bar dataKey="uv" stackId="test" fill="#ff7300" isAnimationActive={false} /> | ||
| <Bar dataKey="pv" stackId="test" fill="#387908" isAnimationActive={false} /> | ||
| </BarChart>, | ||
| ); | ||
|
|
There was a problem hiding this comment.
Flush pending timers after render in these added tests.
These renders are outside createSelectorTestCase, so timer-driven updates can race assertions unless pending timers are advanced.
Proposed patch
@@
const { container } = render(
<BarChart width={400} height={300} data={allZeroData}>
<YAxis />
<Bar dataKey="uv" stackId="test" fill="#ff7300" isAnimationActive={false} />
<Bar dataKey="pv" stackId="test" fill="#387908" isAnimationActive={false} />
</BarChart>,
);
+ act(() => {
+ vi.runOnlyPendingTimers();
+ });
@@
const { container } = render(
<BarChart width={400} height={300} data={allZeroData}>
<YAxis />
<Bar dataKey="uv" stackId="test" shape={customShape} isAnimationActive={false} />
</BarChart>,
);
+ act(() => {
+ vi.runOnlyPendingTimers();
+ });
@@
const { container } = render(
<BarChart width={400} height={300} data={allZeroData} layout="vertical">
<XAxis type="number" />
<YAxis dataKey="name" type="category" />
<Bar dataKey="uv" stackId="test" shape={customShape} isAnimationActive={false} />
</BarChart>,
);
+ act(() => {
+ vi.runOnlyPendingTimers();
+ });As per coding guidelines "Call vi.runOnlyPendingTimers() to advance timers after renders when not using createSelectorTestCase helper, and avoid vi.runAllTimers() to prevent infinite loops".
Also applies to: 1406-1412, 1424-1431
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/chart/BarChart.spec.tsx` around lines 1383 - 1390, The tests render
BarChart components directly and can leave timer-driven updates pending; after
each render call (e.g., the render(...) invocation that renders <BarChart ...>
in BarChart.spec.tsx and the similar render blocks referenced around the other
ranges), add a call to vi.runOnlyPendingTimers() immediately after the render to
advance pending timers (do not use vi.runAllTimers()). Ensure this is applied to
the render locations corresponding to the first snippet and the other two test
blocks mentioned so assertions won't race with pending timer callbacks.
Bundle ReportChanges will decrease total bundle size by 554 bytes (-0.01%) ⬇️. This is within the configured threshold ✅ Detailed changes
Affected Assets, Files, and Routes:view changes for bundle: recharts/bundle-cjsAssets Changed:
view changes for bundle: recharts/bundle-treeshaking-cartesianAssets Changed:
view changes for bundle: recharts/bundle-treeshaking-polarAssets Changed:
view changes for bundle: recharts/bundle-es6Assets Changed:
view changes for bundle: recharts/bundle-umdAssets Changed:
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7199 +/- ##
=======================================
Coverage 89.06% 89.07%
=======================================
Files 539 539
Lines 41011 41007 -4
Branches 5553 5553
=======================================
- Hits 36527 36525 -2
+ Misses 4476 4474 -2
Partials 8 8 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Just recently we merged an optimization where we don't render Bars with zero height - isn't this a regression? |
|
Yeah here's the before/after of my fix: before: after: This fix technically does not regress #6800. That PR added the zero-dimension filter at computeBarRectangles line 1116-1124, which is still intact. This change is upstream it ensures the axis gets a domain so computeBarRectangles even runs. Zero-height bars without custom shapes are still filtered. The first test explicitly asserts .recharts-bar-rectangle count is 0. IMO the two cases don't overlap:
So no, our fix doesn't bring back the perf problem from #6800. Those sparse charts still have a non-zero domain and the zero-dimension filter still fires for their empty bars. @ckifer called it an "accidental breaking change" though in #6235, and it worked in v2, so I was assuming that was desired behavior so arguably this is fixing that regression But about whether the bar should show up or not, I think one person's feature is another person's bug lol. @PavelVanecek I'll defer to you one whether we should:
|
|
Yeah I think this is fine based on the above |
3 participants
Description
A [0, 0] domain was being discarded as "no data" but it meant two different things: no stacks at all, or stacks that are genuinely all zeros. We now detect the "no stacks" case earlier so all-zero stacked data isn't thrown away with it.
Related Issue
#6235
Motivation and Context
How Has This Been Tested?
92 ...getStoryArgsFromArgsTypesObject(BarChartArgs),
93 width: 500,
94 height: 300,
95 - data: pageData,
95 + data: pageData.map(d => ({ ...d, uv: 0, pv: 0 })),
96 margin: {
97 top: 20,
98 right: 30,
Screenshots (if appropriate):
Types of changes
Checklist:
Summary by CodeRabbit
Release Notes
Bug Fixes
Tests