Conversation
WalkthroughThese changes refactor z-index keys and usage, introduce ZIndexLayer wrapping for cursor and several symbols, add a RenderCursor helper with context-aware preferred z-indexing, change Pie sector prop spread order so cell props can override computed values, and propagate axis type/ref changes through Cartesian axis/tick rendering with corresponding test updates. Changes
Sequence Diagram(s)sequenceDiagram
participant Chart
participant CursorInternal
participant RenderCursor
participant ZIndexLayer
participant DefaultZIndexes
Chart->>CursorInternal: render with props & context
CursorInternal->>DefaultZIndexes: determine preferredZIndex (chart type)
CursorInternal->>ZIndexLayer: wrap cursor (zIndex = props.zIndex ?? preferredZIndex)
ZIndexLayer->>RenderCursor: request cursor render
RenderCursor->>RenderCursor: clone element or instantiate component with props
RenderCursor-->>ZIndexLayer: rendered node
ZIndexLayer-->>Chart: layered cursor output
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40–60 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6553 +/- ##
=======================================
Coverage 91.27% 91.27%
=======================================
Files 489 489
Lines 40848 40877 +29
Branches 4570 4574 +4
=======================================
+ Hits 37282 37311 +29
Misses 3550 3550
Partials 16 16 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Bundle ReportChanges will increase total bundle size by 6.18kB (0.24%) ⬆️. 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-es6Assets Changed:
view changes for bundle: recharts/bundle-umdAssets Changed:
|
I think it's better this way. Labels are more important than dots, and people who want both to be visible have choices:
/update-snapshots |
|
Hm from the storybook diff:
|
|
Visual Regression tests failed.
To update snapshots: Comment |
|
✅ Visual regression snapshots have been updated and committed to this PR. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/cartesian/XAxis.spec.tsx (1)
143-144: Consider querying within the tick-labels container for consistency.While the current approach using
getElementsByClassNamewill work, it differs from the pattern used elsewhere in this file (lines 769, 1261, 3188, 3264, 4174) where queries are explicitly scoped to the.recharts-xAxis-tick-labelscontainer. For consistency and to make the test more robust:- const ticksGroup = container.getElementsByClassName('recharts-cartesian-axis-tick-label'); - expect(ticksGroup).toHaveLength(2); + const tickLabels = container.querySelectorAll('.recharts-xAxis-tick-labels .recharts-cartesian-axis-tick-label'); + expect(tickLabels).toHaveLength(2); - const firstTick = ticksGroup[0]; + const firstTick = tickLabels[0];
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (6)
test-vr/__snapshots__/tests/www/BarChartApiExamples.spec-vr.tsx-snapshots/BarChartWithMultiXAxis-1-chromium-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/www/BarChartApiExamples.spec-vr.tsx-snapshots/BarChartWithMultiXAxis-1-firefox-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/www/BarChartApiExamples.spec-vr.tsx-snapshots/BarChartWithMultiXAxis-1-webkit-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/zIndex.spec-vr.tsx-snapshots/bar-and-line-without-z-index-1-chromium-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/zIndex.spec-vr.tsx-snapshots/bar-and-line-without-z-index-1-firefox-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/zIndex.spec-vr.tsx-snapshots/bar-and-line-without-z-index-1-webkit-linux.pngis excluded by!**/*.png
📒 Files selected for processing (12)
src/cartesian/CartesianAxis.tsx(9 hunks)src/cartesian/Scatter.tsx(1 hunks)src/cartesian/XAxis.tsx(1 hunks)src/cartesian/YAxis.tsx(1 hunks)test/cartesian/ReferenceLine/ReferenceLine.spec.tsx(1 hunks)test/cartesian/XAxis.spec.tsx(6 hunks)test/cartesian/XAxis.tickFormatter.spec.tsx(0 hunks)test/cartesian/YAxis/YAxis.spec.tsx(3 hunks)test/cartesian/YAxis/YAxis.tickFormatter.spec.tsx(0 hunks)test/chart/ScatterChart.spec.tsx(1 hunks)test/helper/expectAxisTicks.ts(2 hunks)test/state/selectors/selectDisplayedData.spec.tsx(2 hunks)
💤 Files with no reviewable changes (2)
- test/cartesian/YAxis/YAxis.tickFormatter.spec.tsx
- test/cartesian/XAxis.tickFormatter.spec.tsx
🧰 Additional context used
📓 Path-based instructions (2)
{test,www/test}/**
📄 CodeRabbit inference engine (DEVELOPING.md)
Place unit tests in the test directory; some tests may also live in www/test
Files:
test/helper/expectAxisTicks.tstest/cartesian/ReferenceLine/ReferenceLine.spec.tsxtest/state/selectors/selectDisplayedData.spec.tsxtest/chart/ScatterChart.spec.tsxtest/cartesian/XAxis.spec.tsxtest/cartesian/YAxis/YAxis.spec.tsx
src/**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Do not hardcode any user-facing strings or formatting choices in the library code; leave internationalization to library users
Files:
src/cartesian/Scatter.tsxsrc/cartesian/XAxis.tsxsrc/cartesian/CartesianAxis.tsxsrc/cartesian/YAxis.tsx
🧠 Learnings (6)
📚 Learning: 2025-10-25T07:35:46.188Z
Learnt from: CR
Repo: recharts/recharts PR: 0
File: test/README.md:0-0
Timestamp: 2025-10-25T07:35:46.188Z
Learning: Applies to test/**/*.test.{ts,tsx} : Mock getBoundingClientRect using test/helper/MockGetBoundingClientRect.ts when rendering Recharts components (e.g., Tooltip, Legend, charts)
Applied to files:
test/helper/expectAxisTicks.tstest/cartesian/ReferenceLine/ReferenceLine.spec.tsxtest/state/selectors/selectDisplayedData.spec.tsxtest/chart/ScatterChart.spec.tsxtest/cartesian/XAxis.spec.tsxtest/cartesian/YAxis/YAxis.spec.tsx
📚 Learning: 2025-10-25T07:35:46.188Z
Learnt from: CR
Repo: recharts/recharts PR: 0
File: test/README.md:0-0
Timestamp: 2025-10-25T07:35:46.188Z
Learning: Applies to test/**/*.test.{ts,tsx} : When testing selectors, verify render counts using the spy and rerenderSameComponent from createSelectorTestCase
Applied to files:
test/helper/expectAxisTicks.tstest/cartesian/ReferenceLine/ReferenceLine.spec.tsxtest/state/selectors/selectDisplayedData.spec.tsxtest/chart/ScatterChart.spec.tsxtest/cartesian/XAxis.spec.tsxtest/cartesian/YAxis/YAxis.spec.tsx
📚 Learning: 2025-10-25T07:35:46.188Z
Learnt from: CR
Repo: recharts/recharts PR: 0
File: test/README.md:0-0
Timestamp: 2025-10-25T07:35:46.188Z
Learning: Applies to test/**/*.test.{ts,tsx} : Use expectLastCalledWith instead of toHaveBeenLastCalledWith for typed last-call assertions
Applied to files:
test/helper/expectAxisTicks.tstest/state/selectors/selectDisplayedData.spec.tsxtest/cartesian/XAxis.spec.tsxtest/cartesian/YAxis/YAxis.spec.tsx
📚 Learning: 2025-10-25T07:35:46.188Z
Learnt from: CR
Repo: recharts/recharts PR: 0
File: test/README.md:0-0
Timestamp: 2025-10-25T07:35:46.188Z
Learning: Applies to test/**/*.test.{ts,tsx} : Prefer using the createSelectorTestCase helper when writing or modifying tests
Applied to files:
test/helper/expectAxisTicks.tstest/state/selectors/selectDisplayedData.spec.tsxtest/chart/ScatterChart.spec.tsxtest/cartesian/XAxis.spec.tsxtest/cartesian/YAxis/YAxis.spec.tsx
📚 Learning: 2025-10-25T07:36:02.229Z
Learnt from: CR
Repo: recharts/recharts PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-10-25T07:36:02.229Z
Learning: Recharts aims for simple, declarative, and composable charts; prioritize consistency, usability, performance, and accessibility
Applied to files:
test/state/selectors/selectDisplayedData.spec.tsxtest/cartesian/YAxis/YAxis.spec.tsx
📚 Learning: 2025-10-25T07:35:46.188Z
Learnt from: CR
Repo: recharts/recharts PR: 0
File: test/README.md:0-0
Timestamp: 2025-10-25T07:35:46.188Z
Learning: Applies to test/**/*.test.{ts,tsx} : In tests not using createSelectorTestCase, advance timers after renders with vi.runOnlyPendingTimers()
Applied to files:
test/cartesian/XAxis.spec.tsx
🧬 Code graph analysis (4)
src/cartesian/Scatter.tsx (5)
src/zindex/ZIndexLayer.tsx (1)
ZIndexLayer(49-108)src/zindex/DefaultZIndexes.tsx (1)
DefaultZIndexes(4-81)src/container/Layer.tsx (1)
Layer(13-22)src/util/types.ts (1)
adaptEventsOfChild(875-897)src/util/ScatterUtils.tsx (1)
ScatterSymbol(7-20)
test/state/selectors/selectDisplayedData.spec.tsx (1)
test/helper/expectAxisTicks.ts (1)
expectXAxisTicks(12-21)
src/cartesian/CartesianAxis.tsx (4)
src/util/types.ts (1)
CartesianTickItem(768-772)src/container/Layer.tsx (1)
Layer(13-22)src/zindex/ZIndexLayer.tsx (1)
ZIndexLayer(49-108)src/zindex/DefaultZIndexes.tsx (1)
DefaultZIndexes(4-81)
test/cartesian/YAxis/YAxis.spec.tsx (1)
test/helper/expectAxisTicks.ts (1)
expectYAxisTicks(23-32)
⏰ 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 (6)
test/helper/expectAxisTicks.ts (1)
13-13: LGTM! Selector updates are consistent with the DOM restructuring.The updated selectors correctly target tick labels within the new axis-specific tick-label wrapper containers, maintaining consistency between XAxis and YAxis helpers.
Also applies to: 24-24
src/cartesian/Scatter.tsx (1)
352-374: LGTM! Z-index layering is correctly implemented.The ZIndexLayer wrapping with conditional zIndex based on active state ensures active scatter points render above inactive ones. Event handlers are properly preserved on the inner Layer, and the comment clearly explains the undefined vs. activeDot zIndex behavior.
test/chart/ScatterChart.spec.tsx (1)
409-412: LGTM! Test now correctly interacts with the actual active shape element.The change to capture and interact with the active shape element (instead of assuming sector2) makes the test more robust to DOM structural changes introduced by the ZIndexLayer portal rendering.
test/cartesian/YAxis/YAxis.spec.tsx (2)
589-589: LGTM! Selector updates align with the axis tick DOM structure changes.The updated selectors correctly target the new
.recharts-cartesian-axis-tick-labelwrapper elements, consistent with the helper changes intest/helper/expectAxisTicks.ts.Also applies to: 633-633
926-952: LGTM! Using the centralized helper improves test maintainability.Replacing manual DOM queries with the
expectYAxisTickshelper provides a clearer, more maintainable approach to validating axis tick rendering. If the DOM structure changes again, only the helper needs updating.test/cartesian/XAxis.spec.tsx (1)
769-770: LGTM! Test queries correctly updated for new tick-labels container structure.The changes consistently use the new
.recharts-xAxis-tick-labelscontainer class introduced to support z-index layering for axis ticks. The descendant selectors properly target tick value elements within the container, and the pattern aligns with the updatedexpectXAxisTickshelper.Also applies to: 1261-1265, 3188-3189, 3264-3265, 4174-4175
| const tickLines = tickLineCoords.map(({ entry, line: lineCoord }) => { | ||
| return ( | ||
| <Layer | ||
| className="recharts-cartesian-axis-tick" | ||
| key={`tick-${entry.value}-${entry.coordinate}-${entry.tickCoord}`} | ||
| > | ||
| {tickLine && ( | ||
| <line | ||
| {...tickLineProps} | ||
| {...lineCoord} | ||
| className={clsx('recharts-cartesian-axis-tick-line', get(tickLine, 'className'))} | ||
| /> | ||
| )} | ||
| </Layer> |
There was a problem hiding this comment.
Restore tick events on tick lines
Line 369 onward no longer spreads adaptEventsOfChild onto the tick-line Layer, so axis-level handlers (onClick, onMouseEnter, etc.) stop firing when users interact with the tick line itself. This regresses existing behavior where the combined tick container handled events for both the line and the label. Please reapply the adapted events to the tick-line layer.
- const tickLines = tickLineCoords.map(({ entry, line: lineCoord }) => {
+ const tickLines = tickLineCoords.map(({ entry, line: lineCoord }, index) => {
return (
<Layer
className="recharts-cartesian-axis-tick"
key={`tick-${entry.value}-${entry.coordinate}-${entry.tickCoord}`}
+ {...adaptEventsOfChild(events, entry, index)}
>
{tickLine && (📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const tickLines = tickLineCoords.map(({ entry, line: lineCoord }) => { | |
| return ( | |
| <Layer | |
| className="recharts-cartesian-axis-tick" | |
| key={`tick-${entry.value}-${entry.coordinate}-${entry.tickCoord}`} | |
| > | |
| {tickLine && ( | |
| <line | |
| {...tickLineProps} | |
| {...lineCoord} | |
| className={clsx('recharts-cartesian-axis-tick-line', get(tickLine, 'className'))} | |
| /> | |
| )} | |
| </Layer> | |
| const tickLines = tickLineCoords.map(({ entry, line: lineCoord }, index) => { | |
| return ( | |
| <Layer | |
| className="recharts-cartesian-axis-tick" | |
| key={`tick-${entry.value}-${entry.coordinate}-${entry.tickCoord}`} | |
| {...adaptEventsOfChild(events, entry, index)} | |
| > | |
| {tickLine && ( | |
| <line | |
| {...tickLineProps} | |
| {...lineCoord} | |
| className={clsx('recharts-cartesian-axis-tick-line', get(tickLine, 'className'))} | |
| /> | |
| )} | |
| </Layer> |
🤖 Prompt for AI Agents
In src/cartesian/CartesianAxis.tsx around lines 367 to 380, the tick-line Layer
no longer receives the adapted event handlers so axis-level handlers (onClick,
onMouseEnter, etc.) don't fire when interacting with the tick line; reapply the
adapted events by spreading adaptEventsOfChild(entry, props) (or the local
equivalent used elsewhere in this file) onto the Layer that wraps the tick line
(only when tickLine is rendered) so the tick-line Layer and its children receive
the same event adapters as the tick container.
There was a problem hiding this comment.
I put the events on labels only as it feels more relevant there. Also it's the larger target.
There was a problem hiding this comment.
@PavelVanecek That makes sense — labels are indeed the more intuitive and larger interaction target. The behavior change (events no longer firing when clicking directly on the thin tick lines) seems intentional and reasonable given the improved UX.
🧠 Learnings used
Learnt from: CR
Repo: recharts/recharts PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-10-25T07:36:02.229Z
Learning: Recharts aims for simple, declarative, and composable charts; prioritize consistency, usability, performance, and accessibility
There was a problem hiding this comment.
Still technically a breaking change
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
I'll risk this one.
| otherSvgProps={svgPropertiesNoEvents(props)} | ||
| /> | ||
| <Ticks | ||
| ref={layerRef} |
There was a problem hiding this comment.
I needed to ref one layer deeper so I added forwardRef.
Description
Fixing stories that I broke: https://www.chromatic.com/build?appId=63da8268a0da9970db6992aa&number=3070
Couple of things:
unknownobliterates any hope that we can have of strong typing, unfortunately couple of our stories rely on it - and I expect that our users do too. I will leave this change for 4.0.Motivation and Context
Unblocking 3.4 release
How Has This Been Tested?
This is fixing failed storybook diff.
Screenshots (if appropriate):
Types of changes
Checklist:
Summary by CodeRabbit
Improvements
Updates