fixes an issue where XAxis and YAxis padding were ignored by the clipping mask when allowDataOverflow={true} was used alongside a restricted domain#7232
Conversation
WalkthroughUse Redux axis-range selectors in Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
|
@ckifer can you please review? |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/cartesian/GraphicalItemClipPath.tsx (1)
35-51: Please add/keep a regression test that assertsclipPathrect geometry from axis ranges.This path is the core fix, so a focused assertion on
<rect x/y/width/height>with restricted domain +allowDataOverflow={true}+ axis padding will protect against regressions.As per coding guidelines, "
src/**/*.{ts,tsx}: Aim for 100% unit test code coverage when writing new code".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cartesian/GraphicalItemClipPath.tsx` around lines 35 - 51, Add or retain a regression unit test that mounts GraphicalItemClipPath (or the parent that renders it) with a restricted domain, allowDataOverflow={true}, and axis padding enabled, then assert the rendered <rect> has x/y/width/height matching calculations derived from selectXAxisRange/selectYAxisRange (i.e., Math.min range start/end for x/y and Math.abs difference for width/height) for the clipPath with id `clipPath-${clipPathId}`; target the props and internal flags needClipX/needClipY and plotArea values used by GraphicalItemClipPath so the test fails if the clip geometry deviates.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/cartesian/GraphicalItemClipPath.tsx`:
- Around line 35-51: Add or retain a regression unit test that mounts
GraphicalItemClipPath (or the parent that renders it) with a restricted domain,
allowDataOverflow={true}, and axis padding enabled, then assert the rendered
<rect> has x/y/width/height matching calculations derived from
selectXAxisRange/selectYAxisRange (i.e., Math.min range start/end for x/y and
Math.abs difference for width/height) for the clipPath with id
`clipPath-${clipPathId}`; target the props and internal flags
needClipX/needClipY and plotArea values used by GraphicalItemClipPath so the
test fails if the clip geometry deviates.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a2dafde8-0d96-473f-8348-2da4549612e6
📒 Files selected for processing (1)
src/cartesian/GraphicalItemClipPath.tsx
|
Commit the reproduction too please, we will need a test. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7232 +/- ##
=======================================
Coverage 89.07% 89.07%
=======================================
Files 539 539
Lines 41014 41015 +1
Branches 5554 5556 +2
=======================================
+ Hits 36532 36533 +1
Misses 4474 4474
Partials 8 8 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Bundle ReportChanges will increase total bundle size by 4.84kB (0.09%) ⬆️. This is within the configured threshold ✅ Detailed changes
Affected Assets, Files, and Routes:view changes for bundle: recharts/bundle-treeshaking-cartesianAssets Changed:
view changes for bundle: recharts/bundle-es6Assets Changed:
view changes for bundle: recharts/bundle-umdAssets Changed:
view changes for bundle: recharts/bundle-cjsAssets Changed:
|
… the clipping rectangle matches the padded range when overflow is true and default clipping behavior remains unchanged when overflow is false.
|
@PavelVanecek I have created a test file. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/cartesian/XAxis/XAxis.padding_clip.spec.tsx (1)
6-58: PrefercreateSelectorTestCasehere to track selector-call churn.This spec is a good candidate for
createSelectorTestCaseso you can assert both behavior and selector call counts, which helps prevent rerender regressions in cartesian selectors.As per coding guidelines, "Prefer to use the
createSelectorTestCasehelper function when writing or modifying tests" and "Verify the number of selector calls using the spy object fromcreateSelectorTestCaseto spot unnecessary re-renders and improve performance."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/cartesian/XAxis/XAxis.padding_clip.spec.tsx` around lines 6 - 58, The test for XAxis padding should use the createSelectorTestCase helper to assert both behavior and selector call counts: replace the two standalone it() cases with a createSelectorTestCase that renders the same BarChart + XAxis scenarios (allowDataOverflow true/false), import and use createSelectorTestCase and its selectorSpy to verify the expected DOM clipPath results while also asserting the relevant cartesian selectors were called the expected number of times; locate the test around the current XAxis padding specs and update the render logic for BarChart/XAxis and assertions to use the helper and selectorSpy instead of plain render + DOM-only expectations.
🤖 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/cartesian/XAxis/XAxis.padding_clip.spec.tsx`:
- Around line 8-21: After each render call in XAxis.padding_clip.spec.tsx (the
render invoking <BarChart ...> and any subsequent renders in the same test),
call vi.runOnlyPendingTimers() to flush pending timers and RAF callbacks before
performing DOM assertions; update the test so that immediately after the
render(...) that mounts the BarChart (the one that includes <XAxis dataKey="x"
... padding={{ left: 10, right: 50 }} /> and <Bar dataKey="y" ... />) you invoke
vi.runOnlyPendingTimers() to ensure clipPath rect attributes are stable before
querying them.
---
Nitpick comments:
In `@test/cartesian/XAxis/XAxis.padding_clip.spec.tsx`:
- Around line 6-58: The test for XAxis padding should use the
createSelectorTestCase helper to assert both behavior and selector call counts:
replace the two standalone it() cases with a createSelectorTestCase that renders
the same BarChart + XAxis scenarios (allowDataOverflow true/false), import and
use createSelectorTestCase and its selectorSpy to verify the expected DOM
clipPath results while also asserting the relevant cartesian selectors were
called the expected number of times; locate the test around the current XAxis
padding specs and update the render logic for BarChart/XAxis and assertions to
use the helper and selectorSpy instead of plain render + DOM-only expectations.
🪄 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: 5ac7c75d-2bf9-4987-a091-88aa9d0add12
📒 Files selected for processing (1)
test/cartesian/XAxis/XAxis.padding_clip.spec.tsx
| const { container } = render( | ||
| <BarChart | ||
| width={400} | ||
| height={400} | ||
| data={[ | ||
| { x: 0, y: 10 }, | ||
| { x: 100, y: 30 }, | ||
| ]} | ||
| > | ||
| <XAxis dataKey="x" type="number" domain={[0, 100]} allowDataOverflow padding={{ left: 10, right: 50 }} /> | ||
| <YAxis /> | ||
| <Bar dataKey="y" isAnimationActive={false} /> | ||
| </BarChart>, | ||
| ); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n test/cartesian/XAxis/XAxis.padding_clip.spec.tsxRepository: recharts/recharts
Length of output: 2322
🏁 Script executed:
rg -A 10 "BarChart" test/ -t tsx -t ts | grep -A 10 "render(" | head -50Repository: recharts/recharts
Length of output: 88
🏁 Script executed:
rg -A 10 "BarChart" test/cartesian/ | grep -B 5 -A 10 "vi.runOnlyPendingTimers\|render(" | head -80Repository: recharts/recharts
Length of output: 5526
🏁 Script executed:
rg "render\(" test/cartesian/ -A 3 | grep -B 3 "vi.runOnlyPendingTimers" | head -60Repository: recharts/recharts
Length of output: 43
🏁 Script executed:
rg "vi.runOnlyPendingTimers" test/ -B 5 | head -100Repository: recharts/recharts
Length of output: 6393
🏁 Script executed:
rg "isAnimationActive={false}" test/cartesian/ -A 10 | grep -B 5 -A 5 "render\(" | head -80Repository: recharts/recharts
Length of output: 211
🏁 Script executed:
rg "isAnimationActive=\{false\}" test/cartesian/ -A 10 -B 3 | head -100Repository: recharts/recharts
Length of output: 7746
🏁 Script executed:
cat test/cartesian/XAxis/XAxis.tick.spec.tsx | head -100Repository: recharts/recharts
Length of output: 3153
🏁 Script executed:
rg "render\(" test/cartesian/XAxis/ -A 20 | rg -c "vi.runOnlyPendingTimers"Repository: recharts/recharts
Length of output: 43
🏁 Script executed:
cat test/cartesian/XAxis/XAxis.padding.spec.tsx | head -80Repository: recharts/recharts
Length of output: 2851
🏁 Script executed:
find test/cartesian -name "*.spec.tsx" -exec grep -l "render" {} \; | while read file; do echo "=== $file ==="; grep -A 1 "const { container } = render" "$file" | head -4; done | head -100Repository: recharts/recharts
Length of output: 4378
🏁 Script executed:
rg "render\(" test/cartesian/ -A 5 | rg -B 5 "querySelectorAll\|querySelector" | head -100Repository: recharts/recharts
Length of output: 43
🏁 Script executed:
grep -n "querySelectorAll\|querySelector" test/cartesian/XAxis/XAxis.padding_clip.spec.tsxRepository: recharts/recharts
Length of output: 219
🏁 Script executed:
cat test/cartesian/XAxis/XAxis.padding_clip.spec.tsxRepository: recharts/recharts
Length of output: 1916
Add vi.runOnlyPendingTimers() after each render to ensure timers and RAF callbacks are flushed before DOM assertions.
The test renders BarChart without calling vi.runOnlyPendingTimers() after render on lines 8 and 32. Per coding guidelines, tests must advance pending timers after renders when not using createSelectorTestCase. Even with isAnimationActive={false}, Redux batching and requestAnimationFrame callbacks may cause DOM state changes that need to be flushed before querying clipPath rect attributes.
Suggested patch
-import { describe, expect, it } from 'vitest';
+import { describe, expect, it, vi } from 'vitest';
@@
);
+ vi.runOnlyPendingTimers();
@@
);
+ vi.runOnlyPendingTimers();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/cartesian/XAxis/XAxis.padding_clip.spec.tsx` around lines 8 - 21, After
each render call in XAxis.padding_clip.spec.tsx (the render invoking <BarChart
...> and any subsequent renders in the same test), call
vi.runOnlyPendingTimers() to flush pending timers and RAF callbacks before
performing DOM assertions; update the test so that immediately after the
render(...) that mounts the BarChart (the one that includes <XAxis dataKey="x"
... padding={{ left: 10, right: 50 }} /> and <Bar dataKey="y" ... />) you invoke
vi.runOnlyPendingTimers() to ensure clipPath rect attributes are stable before
querying them.
|
Hi @PavelVanecek 👋 |
|
Before 
After 
|
PavelVanecek
changed the title
2 participants
Description
This PR fixes an issue where XAxis and YAxis padding were ignored by the clipping mask when
allowDataOverflow={true}was used alongside a restricted domain.Previously, the
GraphicalItemClipPathcomponent calculated its clipping rectangle using the broadplotAreadimensions. This caused data to "bleed" into the padded areas because the mask wasn't aware of the specific padding applied to the axes.I updated
GraphicalItemClipPathto use the actual computed ranges for the axes (selectXAxisRangeandselectYAxisRange). This ensures the clipping rectangle strictly matches the padded boundaries, cutting off overflowing data precisely where the axis ends.Related Issue
Fixes #6947
How Has This Been Tested?
I tested this locally using Vitest.
allowDataOverflow={true}, andpadding={{ right: 50 }}. I verified that the rendered<clipPath>now correctly reduces its width/height to respect the padding (e.g., width goes from 330px to 280px).vitest run test/cartesian/XAxis/XAxis.padding.spec.tsxandtest/cartesian/Bar.truncateByDomain.spec.tsxto verify that existing truncation and padding behaviors are unaffected and no regressions were introduced.Screenshots (if appropriate):
(Note: You can drag and drop a before/after screenshot of the chart here if you'd like to show the visual difference)
Types of changes
Checklist:
Summary by CodeRabbit