Fix BarStack clipPath in charts with stackOffset=sign#6806
Fix BarStack clipPath in charts with stackOffset=sign#6806PavelVanecek merged 3 commits intomainfrom
Conversation
WalkthroughUpdated rectangle expansion logic in bar stack selectors to compute min/max from all rectangle corners (handles negative dimensions). Added unit and visual-regression tests exercising BarChart/BarStack with multiple stackOffset modes including "sign" and mixed negative/positive data. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6806 +/- ##
=======================================
Coverage 93.78% 93.78%
=======================================
Files 518 518
Lines 45395 45395
Branches 5045 5045
=======================================
Hits 42575 42575
Misses 2813 2813
Partials 7 7 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Staging Deployment Details
These deployments will remain available for 30 days. To update snapshots: Comment |
Bundle ReportChanges will increase total bundle size by 326 bytes (0.01%) ⬆️. This is within the configured threshold ✅ Detailed changes
Affected Assets, Files, and Routes:view changes for bundle: recharts/bundle-umdAssets Changed:
view changes for bundle: recharts/bundle-cjsAssets Changed:
view changes for bundle: recharts/bundle-es6Assets Changed:
|
|
ooops, gave you a conflict |
28b46cd to
6261b57
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
test-vr/tests/BarChart/BarChart.stackOffset.spec-vr.tsx (1)
12-101: Consider using parameterized tests to reduce duplication.The 14 tests follow an identical pattern, differing only in
stackOffsetvalue and whether usingBarStack. This could be condensed using Playwright's parameterized test patterns.🔎 Possible refactor using parameterized tests
const stackOffsets = ['none', 'expand', 'sign', 'wiggle', 'silhouette', 'positive'] as const; for (const stackOffset of stackOffsets) { test(`BarChart with stackOffset=${stackOffset}`, async ({ mount }) => { const component = await mount( <BarChart data={rangedStackedBarData} width={200} height={200} stackOffset={stackOffset}> <YAxis /> <XAxis dataKey="name" /> <Bar dataKey="value1" stackId="a" isAnimationActive={false} fill="red" /> <Bar dataKey="value2" stackId="a" isAnimationActive={false} fill="green" /> <Bar dataKey="value3" stackId="a" isAnimationActive={false} fill="blue" /> </BarChart>, ); await expect(component).toHaveScreenshot(); }); }The same pattern can be applied for BarStack tests.
test/state/selectors/barStackSelectors.spec.tsx (1)
145-194: Consider verifying selector call counts for performance validation.Per coding guidelines, tests using
createSelectorTestCaseshould verify the number of selector calls to spot unnecessary re-renders. This applies to the new test cases as well.🔎 Example of verifying call count
test('selectStackRects should select edges of all rectangles in this stack', () => { const { spy } = renderTestCase(state => selectStackRects(state, 'mystackid', false)); expectLastCalledWith(spy, [{ height: 75, width: 38, x: 10, y: 25 }]); + expect(spy).toHaveBeenCalledTimes(1); });Based on coding guidelines: "Verify the number of selector calls using the spy object from createSelectorTestCase to spot unnecessary re-renders and improve performance."
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (42)
test-vr/__snapshots__/tests/BarChart/BarChart.stackOffset.spec-vr.tsx-snapshots/BarChart-with-default-stackOffset-1-chromium-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/BarChart/BarChart.stackOffset.spec-vr.tsx-snapshots/BarChart-with-default-stackOffset-1-firefox-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/BarChart/BarChart.stackOffset.spec-vr.tsx-snapshots/BarChart-with-default-stackOffset-1-webkit-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/BarChart/BarChart.stackOffset.spec-vr.tsx-snapshots/BarChart-with-stackOffset-expand-1-chromium-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/BarChart/BarChart.stackOffset.spec-vr.tsx-snapshots/BarChart-with-stackOffset-expand-1-firefox-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/BarChart/BarChart.stackOffset.spec-vr.tsx-snapshots/BarChart-with-stackOffset-expand-1-webkit-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/BarChart/BarChart.stackOffset.spec-vr.tsx-snapshots/BarChart-with-stackOffset-none-1-chromium-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/BarChart/BarChart.stackOffset.spec-vr.tsx-snapshots/BarChart-with-stackOffset-none-1-firefox-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/BarChart/BarChart.stackOffset.spec-vr.tsx-snapshots/BarChart-with-stackOffset-none-1-webkit-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/BarChart/BarChart.stackOffset.spec-vr.tsx-snapshots/BarChart-with-stackOffset-positive-1-chromium-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/BarChart/BarChart.stackOffset.spec-vr.tsx-snapshots/BarChart-with-stackOffset-positive-1-firefox-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/BarChart/BarChart.stackOffset.spec-vr.tsx-snapshots/BarChart-with-stackOffset-positive-1-webkit-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/BarChart/BarChart.stackOffset.spec-vr.tsx-snapshots/BarChart-with-stackOffset-sign-1-chromium-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/BarChart/BarChart.stackOffset.spec-vr.tsx-snapshots/BarChart-with-stackOffset-sign-1-firefox-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/BarChart/BarChart.stackOffset.spec-vr.tsx-snapshots/BarChart-with-stackOffset-sign-1-webkit-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/BarChart/BarChart.stackOffset.spec-vr.tsx-snapshots/BarChart-with-stackOffset-silhouette-1-chromium-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/BarChart/BarChart.stackOffset.spec-vr.tsx-snapshots/BarChart-with-stackOffset-silhouette-1-firefox-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/BarChart/BarChart.stackOffset.spec-vr.tsx-snapshots/BarChart-with-stackOffset-silhouette-1-webkit-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/BarChart/BarChart.stackOffset.spec-vr.tsx-snapshots/BarChart-with-stackOffset-wiggle-1-chromium-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/BarChart/BarChart.stackOffset.spec-vr.tsx-snapshots/BarChart-with-stackOffset-wiggle-1-firefox-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/BarChart/BarChart.stackOffset.spec-vr.tsx-snapshots/BarChart-with-stackOffset-wiggle-1-webkit-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/BarChart/BarChart.stackOffset.spec-vr.tsx-snapshots/BarStack-with-default-stackOffset-1-chromium-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/BarChart/BarChart.stackOffset.spec-vr.tsx-snapshots/BarStack-with-default-stackOffset-1-firefox-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/BarChart/BarChart.stackOffset.spec-vr.tsx-snapshots/BarStack-with-default-stackOffset-1-webkit-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/BarChart/BarChart.stackOffset.spec-vr.tsx-snapshots/BarStack-with-stackOffset-expand-1-chromium-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/BarChart/BarChart.stackOffset.spec-vr.tsx-snapshots/BarStack-with-stackOffset-expand-1-firefox-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/BarChart/BarChart.stackOffset.spec-vr.tsx-snapshots/BarStack-with-stackOffset-expand-1-webkit-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/BarChart/BarChart.stackOffset.spec-vr.tsx-snapshots/BarStack-with-stackOffset-none-1-chromium-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/BarChart/BarChart.stackOffset.spec-vr.tsx-snapshots/BarStack-with-stackOffset-none-1-firefox-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/BarChart/BarChart.stackOffset.spec-vr.tsx-snapshots/BarStack-with-stackOffset-none-1-webkit-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/BarChart/BarChart.stackOffset.spec-vr.tsx-snapshots/BarStack-with-stackOffset-positive-1-chromium-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/BarChart/BarChart.stackOffset.spec-vr.tsx-snapshots/BarStack-with-stackOffset-positive-1-firefox-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/BarChart/BarChart.stackOffset.spec-vr.tsx-snapshots/BarStack-with-stackOffset-positive-1-webkit-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/BarChart/BarChart.stackOffset.spec-vr.tsx-snapshots/BarStack-with-stackOffset-sign-1-chromium-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/BarChart/BarChart.stackOffset.spec-vr.tsx-snapshots/BarStack-with-stackOffset-sign-1-firefox-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/BarChart/BarChart.stackOffset.spec-vr.tsx-snapshots/BarStack-with-stackOffset-sign-1-webkit-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/BarChart/BarChart.stackOffset.spec-vr.tsx-snapshots/BarStack-with-stackOffset-silhouette-1-chromium-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/BarChart/BarChart.stackOffset.spec-vr.tsx-snapshots/BarStack-with-stackOffset-silhouette-1-firefox-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/BarChart/BarChart.stackOffset.spec-vr.tsx-snapshots/BarStack-with-stackOffset-silhouette-1-webkit-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/BarChart/BarChart.stackOffset.spec-vr.tsx-snapshots/BarStack-with-stackOffset-wiggle-1-chromium-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/BarChart/BarChart.stackOffset.spec-vr.tsx-snapshots/BarStack-with-stackOffset-wiggle-1-firefox-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/BarChart/BarChart.stackOffset.spec-vr.tsx-snapshots/BarStack-with-stackOffset-wiggle-1-webkit-linux.pngis excluded by!**/*.png
📒 Files selected for processing (3)
src/state/selectors/barStackSelectors.ts(1 hunks)test-vr/tests/BarChart/BarChart.stackOffset.spec-vr.tsx(1 hunks)test/state/selectors/barStackSelectors.spec.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/state/selectors/barStackSelectors.ts
🧰 Additional context used
📓 Path-based instructions (6)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
**/*.{ts,tsx}: Never useanytype (implicit or explicit) in TypeScript code
Preferunknownoveranyand refine the type in TypeScript
Type function parameters and return values explicitly in TypeScript, do not rely on implicit any or inference; exceptions are React components and trivial functions
Do not useastype assertions in TypeScript; the only exception isas const
Files:
test/state/selectors/barStackSelectors.spec.tsxtest-vr/tests/BarChart/BarChart.stackOffset.spec-vr.tsx
test/**/*.spec.{ts,tsx}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Aim for 100% unit test code coverage when writing new code
Files:
test/state/selectors/barStackSelectors.spec.tsx
**/*.{js,ts,tsx}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Ensure code lints by running
npm run lintand follows Airbnb's Style Guide
Files:
test/state/selectors/barStackSelectors.spec.tsxtest-vr/tests/BarChart/BarChart.stackOffset.spec-vr.tsx
test/**/*.{test,spec}.{ts,tsx}
📄 CodeRabbit inference engine (test/README.md)
test/**/*.{test,spec}.{ts,tsx}: Aim for 100% unit test code coverage when writing new code
Prefer to use thecreateSelectorTestCasehelper function when writing or modifying tests
Use theexpectLastCalledWithhelper function instead ofexpect(spy).toHaveBeenLastCalledWith(...)for better typing and autocompletion
Verify the number of selector calls using the spy object fromcreateSelectorTestCaseto spot unnecessary re-renders and improve performance
MockgetBoundingClientRectin tests using the helper function provided intest/helper/MockGetBoundingClientRect.ts
Usevi.useFakeTimers()in all tests due to Redux autoBatchEnhancer dependency on timers andrequestAnimationFrame
Callvi.runOnlyPendingTimers()to advance timers after renders when not usingcreateSelectorTestCasehelper, and avoidvi.runAllTimers()to prevent infinite loops
UseuserEvent.setup({ advanceTimers: vi.runOnlyPendingTimers })or theuserEventSetuphelper function fromtest/helper/userEventSetup.tswhen creating userEvent instances
When testing tooltips on hover, usevi.runOnlyPendingTimers()after eachuserEvent.hover()call or use theshowTooltiphelper function fromtooltipTestHelpers.tsto account for requestAnimationFrame delays
Files:
test/state/selectors/barStackSelectors.spec.tsx
**/*.spec.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
When running unit tests, prefer to run a single test file using
npm run test -- path/to/TestFile.spec.tsxrather than running all tests withnpm test
Files:
test/state/selectors/barStackSelectors.spec.tsx
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (DEVELOPING.md)
All imports from
rechartsmust use the public API entry point; imports from internal paths likerecharts/types/*orrecharts/src/*are not allowed
Files:
test/state/selectors/barStackSelectors.spec.tsxtest-vr/tests/BarChart/BarChart.stackOffset.spec-vr.tsx
🧠 Learnings (9)
📚 Learning: 2025-11-25T01:23:08.250Z
Learnt from: CR
Repo: recharts/recharts PR: 0
File: test/README.md:0-0
Timestamp: 2025-11-25T01:23:08.250Z
Learning: Applies to test/**/*.{test,spec}.{ts,tsx} : Verify the number of selector calls using the spy object from `createSelectorTestCase` to spot unnecessary re-renders and improve performance
Applied to files:
test/state/selectors/barStackSelectors.spec.tsxtest-vr/tests/BarChart/BarChart.stackOffset.spec-vr.tsx
📚 Learning: 2025-11-25T01:23:08.250Z
Learnt from: CR
Repo: recharts/recharts PR: 0
File: test/README.md:0-0
Timestamp: 2025-11-25T01:23:08.250Z
Learning: Applies to test/**/*.{test,spec}.{ts,tsx} : Prefer to use the `createSelectorTestCase` helper function when writing or modifying tests
Applied to files:
test/state/selectors/barStackSelectors.spec.tsx
📚 Learning: 2025-12-16T08:12:06.809Z
Learnt from: PavelVanecek
Repo: recharts/recharts PR: 6783
File: test/util/ChartUtils.spec.tsx:15-16
Timestamp: 2025-12-16T08:12:06.809Z
Learning: In tests (files under any test directory, e.g., test/, __tests__/, etc.), allow importing internal module paths (e.g., ../../src/...) and do not require imports to use the public API entry point (src/index.ts). The public API import restriction should apply only to non-test code. During reviews, accept internal imports in test files and enforce the public API pattern only for non-test code.
Applied to files:
test/state/selectors/barStackSelectors.spec.tsx
📚 Learning: 2025-11-25T01:23:08.250Z
Learnt from: CR
Repo: recharts/recharts PR: 0
File: test/README.md:0-0
Timestamp: 2025-11-25T01:23:08.250Z
Learning: Applies to test/**/*.{test,spec}.{ts,tsx} : Mock `getBoundingClientRect` in tests using the helper function provided in `test/helper/MockGetBoundingClientRect.ts`
Applied to files:
test/state/selectors/barStackSelectors.spec.tsx
📚 Learning: 2025-11-25T01:23:08.250Z
Learnt from: CR
Repo: recharts/recharts PR: 0
File: test/README.md:0-0
Timestamp: 2025-11-25T01:23:08.250Z
Learning: Applies to test/**/*.{test,spec}.{ts,tsx} : Call `vi.runOnlyPendingTimers()` to advance timers after renders when not using `createSelectorTestCase` helper, and avoid `vi.runAllTimers()` to prevent infinite loops
Applied to files:
test/state/selectors/barStackSelectors.spec.tsx
📚 Learning: 2025-12-14T13:58:58.361Z
Learnt from: PavelVanecek
Repo: recharts/recharts PR: 6771
File: src/shape/Curve.tsx:39-40
Timestamp: 2025-12-14T13:58:58.361Z
Learning: In the recharts codebase, `useAppSelector` and hooks like `useChartLayout()` are designed to return `undefined` when used outside Redux Provider context, rather than throwing errors. This allows components like `Curve` that call these hooks to work standalone by falling back to prop values.
Applied to files:
test/state/selectors/barStackSelectors.spec.tsx
📚 Learning: 2025-12-16T08:12:13.355Z
Learnt from: PavelVanecek
Repo: recharts/recharts PR: 6783
File: test/util/ChartUtils.spec.tsx:15-16
Timestamp: 2025-12-16T08:12:13.355Z
Learning: In the recharts codebase, files in the `test` folder are allowed to import from internal paths (e.g., `../../src/state/cartesianAxisSlice`) and do not need to use the public API entry point (`src/index.ts`). The public API import restriction applies only to non-test code.
Applied to files:
test-vr/tests/BarChart/BarChart.stackOffset.spec-vr.tsx
📚 Learning: 2025-11-25T01:22:59.729Z
Learnt from: CR
Repo: recharts/recharts PR: 0
File: CONTRIBUTING.md:0-0
Timestamp: 2025-11-25T01:22:59.729Z
Learning: Applies to test/component/**/*.spec.tsx : Use React Testing Library for testing component interactions and behavior upon rendering
Applied to files:
test-vr/tests/BarChart/BarChart.stackOffset.spec-vr.tsx
📚 Learning: 2025-11-25T01:22:59.729Z
Learnt from: CR
Repo: recharts/recharts PR: 0
File: CONTRIBUTING.md:0-0
Timestamp: 2025-11-25T01:22:59.729Z
Learning: Applies to storybook/stories/**/*.stories.tsx : Use Storybook for smoke tests and add play functions with assertions for actual tests
Applied to files:
test-vr/tests/BarChart/BarChart.stackOffset.spec-vr.tsx
🧬 Code graph analysis (1)
test/state/selectors/barStackSelectors.spec.tsx (3)
test/helper/createSelectorTestCase.tsx (1)
createSelectorTestCase(78-139)src/state/selectors/barStackSelectors.ts (2)
selectStackRects(86-93)expandRectangle(51-68)test/helper/expectLastCalledWith.ts (1)
expectLastCalledWith(14-34)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build, Test, Pack
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (7)
test-vr/tests/BarChart/BarChart.stackOffset.spec-vr.tsx (2)
1-10: LGTM! Good test setup with appropriate data.The test data includes a mix of negative and positive values across all entries, which is essential for validating the
stackOffset=signfix. Disabling animations withisAnimationActive={false}is the correct approach for deterministic VR tests.
103-206: Good coverage of BarStack with radius prop.Including
radius={[10, 20, 30, 40]}in the BarStack tests validates the interaction between stackOffset modes and rounded corners, which is particularly important since the PR fixes clipPath behavior. This ensures the fix doesn't regress corner rendering.test/state/selectors/barStackSelectors.spec.tsx (5)
145-162: Test setup looks good, but verify thedataKey="amt"reference.The test setup correctly exercises
stackOffset="sign"with negative values (issue #6802). However, line 160 includes aBarwithdataKey="amt"but the test data at line 151 only containsvalue1andvalue2. This bar will have no data to render.If this is intentional to match the existing test pattern and verify that orphan bars don't break the selectors, consider adding a brief comment explaining this. Otherwise, either remove the bar or add
amtto the test data.
164-167: LGTM! Correctly validates the bounding box for mixed-sign stacks.The expected result
{ height: 75, width: 38, x: 10, y: 25 }correctly encompasses both the negative-height bar (extending from y=75 to y=100) and the positive-height bar (extending from y=25 to y=75), validating the fix toexpandRectangle.
169-180: Good test for negative height rectangles.This directly tests the core behavior: bars with negative values should have negative heights when using
stackOffset="sign". Thevalue: [0, -100]assertion confirms the correct stack offset calculation.
182-193: LGTM! Completes the sign-offset coverage.Together with the negative height test, this validates that
stackOffset="sign"correctly handles both positive and negative bars in the same stack.
284-314: Excellent test for the core fix with thorough documentation.This test validates the updated
expandRectanglelogic that computes min/max from all four corners of each rectangle. Testing with both negative height and negative width ensures the fix is robust against any sign combinations. The detailed comments explaining the expected calculation make the test self-documenting and easier to maintain.
|
Staging Deployment Details
These deployments will remain available for 30 days. To update snapshots: Comment |
Description
The new selectors did not handle negative-height bars (ikr) properly so here is a fix.
Related Issue
Fixes #6802
Screenshots (if appropriate):
Before/after
Types of changes
Checklist:
Summary by CodeRabbit
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.