Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a proof of concept for z-index functionality in Recharts using React portals and SVG <g> elements. The implementation allows components to specify a z-index prop to control their rendering order, similar to CSS z-index.
- Adds z-index support to Bar and LabelList components with portal-based rendering
- Implements Redux state management for tracking z-index registrations and portal IDs
- Creates a portal system that dynamically renders SVG groups for each requested z-index layer
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| storybook/stories/API/chart/BarChart.stories.tsx | Updates story to demonstrate z-index functionality with overlapping bars |
| src/zindex/zIndexSelectors.ts | Adds selector for querying z-index portal element IDs |
| src/zindex/ZIndexable.tsx | Core component that wraps children in portals based on z-index |
| src/zindex/ZIndexPortal.tsx | Renders SVG portal containers sorted by z-index |
| src/state/zIndexSlice.ts | Redux slice managing z-index registrations and portal tracking |
| src/state/store.ts | Integrates z-index reducer into the Redux store |
| src/container/RootSurface.tsx | Renders z-index portals in the main chart surface |
| src/component/LabelList.tsx | Adds z-index prop and wraps rendering in ZIndexable |
| src/cartesian/Bar.tsx | Adds z-index prop and wraps BarImpl in ZIndexable |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
ckifer
left a comment
There was a problem hiding this comment.
I like it. Maybe can test it out with another chart like scatter before continuing just to double check things but LGTM
|
I will add that next. Do you have some specific example in mind that is problematic? |
|
Eh not really. Everything is encapsulated in the Impl component so nothing really gets lost. Other charts should be the same |
|
That went smooth. I made the scatters larger so that it's easier to see 
|
PavelVanecek
left a comment
There was a problem hiding this comment.
The zIndex generates a lot of visual diff and lot of failed tests but so far it looks like an improvement.
There was a problem hiding this comment.
This got a lot better - the labels are now on top
There was a problem hiding this comment.
Labels on top, improvement
There was a problem hiding this comment.
Line is now on top of my custom drawings - this may or may not be desired. Do we need to export the ZIndexable and ZIndexLayer components so that people can position their custom things properly?
There was a problem hiding this comment.
Hm I think we don't worry about it for now. I thought these were labels but they're custom svg? Shouldn't those still render either before or after our stuff?
There was a problem hiding this comment.
Or no, the custom code is rendered within the chart wrapper still
There was a problem hiding this comment.
Scatter is gone, I need to add zIndex support to Scatter next
There was a problem hiding this comment.
Now grid is rendering on top of axis lines, needs fix
There was a problem hiding this comment.
Much improvement on Scatter labels
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6479 +/- ##
==========================================
+ Coverage 93.38% 93.43% +0.04%
==========================================
Files 431 436 +5
Lines 39178 39545 +367
Branches 4533 4599 +66
==========================================
+ Hits 36588 36950 +362
- Misses 2574 2579 +5
Partials 16 16 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Bundle ReportChanges will increase total bundle size by 47.66kB (1.88%) ⬆️. 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:
|
|
Visual Regression tests failed.
|
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds a z-index layering system: new DefaultZIndexes, ZIndexLayer/ZIndexPortal components, Redux zIndex slice and selectors, and extends many chart and UI components and tests to accept and use an optional Changes
Sequence Diagram(s)sequenceDiagram
participant Comp as Chart Component
participant ZLayer as ZIndexLayer
participant Store as Redux Store
participant PortalMgr as AllZIndexPortals
participant DOM as Portal DOM element
Comp->>ZLayer: render(children, zIndex)
ZLayer->>Store: registerZIndexPortal(zIndex) [useLayoutEffect]
Store-->>ZLayer: portal entry created/consumers updated
alt portal id available (selectZIndexPortalId -> id)
ZLayer->>DOM: createPortal(children → portalElement[id])
DOM-->>ZLayer: children rendered in portal
else portal missing or outside chart context
ZLayer-->>Comp: render(children inline)
end
Note over PortalMgr,Store: PortalMgr reads registered zIndexes and renders SVG containers (negatives before children, positives after)
Comp->>Store: unregisterZIndexPortal(zIndex) [cleanup]
Store-->>Store: decrement/remove non-default entries
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Areas needing extra attention:
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (10)
test/component/Legend.itemSorter.spec.tsx (1)
96-99: Make zIndex assertions less brittle with objectContaining.Asserting the entire payload object is fragile. Narrow to zIndex inside each entry’s payload.
Apply this pattern (example for the first case):
- payload: [ - { - color: '#3182bd', - dataKey: 'value', - inactive: false, - payload: { - ... - zIndex: DefaultZIndexes.line, - }, - type: 'line', - value: 'A', - }, - { - color: '#3182bd', - dataKey: 'percent', - inactive: false, - payload: { - ... - zIndex: DefaultZIndexes.line, - }, - type: 'line', - value: 'B', - }, - ], + payload: expect.arrayContaining([ + expect.objectContaining({ + dataKey: 'value', + payload: expect.objectContaining({ zIndex: DefaultZIndexes.line }), + }), + expect.objectContaining({ + dataKey: 'percent', + payload: expect.objectContaining({ zIndex: DefaultZIndexes.line }), + }), + ]),Repeat similarly for the second test block.
Also applies to: 124-127
test/component/Legend.spec.tsx (1)
471-479: Tighten assertions to only verify zIndex where needed.Use expect.objectContaining to assert zIndex without pinning all other fields, reducing churn when defaults evolve.
Example for one entry:
- { - color: '#8884d8', - dataKey: 'pv', - inactive: false, - payload: { - ... - zIndex: DefaultZIndexes.line, - }, - type: 'line', - value: 'pv', - }, + expect.objectContaining({ + dataKey: 'pv', + payload: expect.objectContaining({ zIndex: DefaultZIndexes.line }), + }),Apply the same shape to the other entries adding zIndex.
Also applies to: 499-507, 817-825, 845-853
test/cartesian/CartesianGrid.spec.tsx (1)
1329-1336: Optional: prefer objectContaining for expectedProps to reduce brittleness.Consider asserting only stable keys (including zIndex) via expect.objectContaining rather than full structural equality.
Also applies to: 1425-1432, 1518-1524, 1607-1612
test-vr/tests/zIndex.spec-vr.tsx (1)
6-31: Good baseline VR coverage.Consider naming and tightening the snapshot for stability:
- await expect(component).toHaveScreenshot(); + await expect(component).toHaveScreenshot('bar-line-no-zindex.png', { maxDiffPixelRatio: 0.003 });Optionally, add a global reduce‑motion style to further harden snapshots.
storybook/stories/API/chart/BarChart.stories.tsx (1)
44-59: Nice z-index POC.Optional enhancements:
- Expose zIndex and label.zIndex as Storybook controls to demo layering live.
- Add a variant with negative zIndex once supported to document behavior.
When negative zIndex lands, ping this story to add a case.
src/zindex/DefaultZIndexes.tsx (1)
4-26: Make defaults readonly to prevent mutation.Add a const assertion so keys/values are immutable at type level.
-export const DefaultZIndexes = { +export const DefaultZIndexes = { grid: -100, /* … */ tooltip: 10000, -}; +} as const; + +export type DefaultZIndexKey = keyof typeof DefaultZIndexes;test/zindex/zIndexSelectors.spec.ts (1)
1-85: Add a couple more selector edge cases.
- Duplicate registration of the same zIndex should not duplicate in the list; assert de-dup behavior.
- Register/unregister a portalId without unregistering zIndex; assert selectAllRegisteredZIndexes remains unchanged.
src/state/selectors/tooltipSelectors.ts (1)
86-86: Revisit memoization equality strategy.Switching to emptyArraysAreEqualCheck may increase recomputation when arrays are recreated with same contents. If perf regresses, consider:
- keeping content-equality for small arrays, or
- ensuring producers return stable references (avoid new arrays) so ref equality holds.
Also applies to: 109-110, 126-127
src/cartesian/Line.tsx (1)
642-648: Consider layering ActivePoints above the line via ZIndexLayer.If you expect hover/active points to sit above the line/labels consistently, wrap ActivePoints in its own ZIndexLayer with DefaultZIndexes.activeShape.
<ActivePoints - activeDot={this.props.activeDot} - points={points} - mainColor={this.props.stroke} - itemDataKey={this.props.dataKey} - /> + {/* keep behavior if you decide not to portal */} + /> + {/* Optional: ensure consistent layering across portalized layers */} + {false && ( + <ZIndexLayer zIndex={DefaultZIndexes.activeShape}> + <ActivePoints + activeDot={this.props.activeDot} + points={points} + mainColor={this.props.stroke} + itemDataKey={this.props.dataKey} + /> + </ZIndexLayer> + )}src/zindex/ZIndexLayer.tsx (1)
24-81: LGTM with optional optimization suggestion.The implementation correctly handles the portal rendering lifecycle with proper guards for missing portal IDs and elements. The past review concerns about undefined handling have been addressed.
Optional: You could avoid calling the selector when
shouldRenderInPortalis false for a minor performance improvement:}, [dispatch, zIndex, shouldRenderInPortal]); - const portalId: string | undefined = useAppSelector(state => selectZIndexPortalId(state, zIndex)); - if (!shouldRenderInPortal) { // If no zIndex is provided or zIndex is 0, render normally without portals return children; } + const portalId: string | undefined = useAppSelector(state => selectZIndexPortalId(state, zIndex)); + if (!portalId) {This moves the selector call after the early return, avoiding unnecessary selector evaluation when not rendering in a portal.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (66)
test-vr/__snapshots__/tests/Label.spec-vr.tsx-snapshots/ReferenceLine-Label-1-chromium-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/Label.spec-vr.tsx-snapshots/ReferenceLine-Label-1-firefox-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/Label.spec-vr.tsx-snapshots/ReferenceLine-Label-1-webkit-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/LabelList.spec-vr.tsx-snapshots/Bar-implicit-LabelList-1-chromium-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/LabelList.spec-vr.tsx-snapshots/Bar-implicit-LabelList-1-firefox-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/LabelList.spec-vr.tsx-snapshots/Bar-implicit-LabelList-1-webkit-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/LabelList.spec-vr.tsx-snapshots/Line-explicit-LabelList-1-chromium-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/LabelList.spec-vr.tsx-snapshots/Line-explicit-LabelList-1-firefox-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/LabelList.spec-vr.tsx-snapshots/Line-explicit-LabelList-1-webkit-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/LabelList.spec-vr.tsx-snapshots/Line-implicit-LabelList-1-chromium-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/LabelList.spec-vr.tsx-snapshots/Line-implicit-LabelList-1-firefox-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/LabelList.spec-vr.tsx-snapshots/Line-implicit-LabelList-1-webkit-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/LabelList.spec-vr.tsx-snapshots/Pie-implicit-LabelList-1-chromium-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/LabelList.spec-vr.tsx-snapshots/Pie-implicit-LabelList-1-firefox-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/LabelList.spec-vr.tsx-snapshots/Pie-implicit-LabelList-1-webkit-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/LabelList.spec-vr.tsx-snapshots/RadialBar-implicit-LabelList-1-chromium-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/LabelList.spec-vr.tsx-snapshots/RadialBar-implicit-LabelList-1-firefox-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/LabelList.spec-vr.tsx-snapshots/RadialBar-implicit-LabelList-1-webkit-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/ResponsiveContainer.spec-vr.tsx-snapshots/ResponsiveContainer-with-hardcoded-width-and-height-1-chromium-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/ResponsiveContainer.spec-vr.tsx-snapshots/ResponsiveContainer-with-hardcoded-width-and-height-1-firefox-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/ResponsiveContainer.spec-vr.tsx-snapshots/ResponsiveContainer-with-hardcoded-width-and-height-1-webkit-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/ResponsiveContainer.spec-vr.tsx-snapshots/aspect-ratio-should-be-overridden-by-maxHeight-1-chromium-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/ResponsiveContainer.spec-vr.tsx-snapshots/aspect-ratio-should-be-overridden-by-maxHeight-1-firefox-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/ResponsiveContainer.spec-vr.tsx-snapshots/aspect-ratio-should-be-overridden-by-maxHeight-1-webkit-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/ResponsiveContainer.spec-vr.tsx-snapshots/should-calculate-height-based-on-aspect-ratio-1-chromium-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/ResponsiveContainer.spec-vr.tsx-snapshots/should-calculate-height-based-on-aspect-ratio-1-firefox-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/ResponsiveContainer.spec-vr.tsx-snapshots/should-calculate-height-based-on-aspect-ratio-1-webkit-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/ResponsiveContainer.spec-vr.tsx-snapshots/should-create-a-square-chart-that-overflows-a-wide-screen-1-chromium-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/ResponsiveContainer.spec-vr.tsx-snapshots/should-create-a-square-chart-that-overflows-a-wide-screen-1-firefox-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/ResponsiveContainer.spec-vr.tsx-snapshots/should-create-a-square-chart-that-overflows-a-wide-screen-1-webkit-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/ResponsiveContainer.spec-vr.tsx-snapshots/should-create-a-square-chart-without-width-or-height-specified-1-chromium-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/ResponsiveContainer.spec-vr.tsx-snapshots/should-create-a-square-chart-without-width-or-height-specified-1-firefox-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/ResponsiveContainer.spec-vr.tsx-snapshots/should-create-a-square-chart-without-width-or-height-specified-1-webkit-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/ResponsiveContainer.spec-vr.tsx-snapshots/should-handle-mixed-fixed-and-percentage-sizing-1-chromium-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/ResponsiveContainer.spec-vr.tsx-snapshots/should-handle-mixed-fixed-and-percentage-sizing-1-firefox-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/ResponsiveContainer.spec-vr.tsx-snapshots/should-handle-mixed-fixed-and-percentage-sizing-1-webkit-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/ResponsiveContainer.spec-vr.tsx-snapshots/should-render-at-100-width-and-height-of-a-fixed-size-parent-1-chromium-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/ResponsiveContainer.spec-vr.tsx-snapshots/should-render-at-100-width-and-height-of-a-fixed-size-parent-1-firefox-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/ResponsiveContainer.spec-vr.tsx-snapshots/should-render-at-100-width-and-height-of-a-fixed-size-parent-1-webkit-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/ResponsiveContainer.spec-vr.tsx-snapshots/should-render-at-50-width-and-height-of-a-fixed-size-parent-1-chromium-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/ResponsiveContainer.spec-vr.tsx-snapshots/should-render-at-50-width-and-height-of-a-fixed-size-parent-1-firefox-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/ResponsiveContainer.spec-vr.tsx-snapshots/should-render-at-50-width-and-height-of-a-fixed-size-parent-1-webkit-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/ResponsiveContainer.spec-vr.tsx-snapshots/should-respect-dimensions-of-a-flexbox-parent-1-chromium-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/ResponsiveContainer.spec-vr.tsx-snapshots/should-respect-dimensions-of-a-flexbox-parent-1-firefox-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/ResponsiveContainer.spec-vr.tsx-snapshots/should-respect-dimensions-of-a-flexbox-parent-1-webkit-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/ResponsiveContainer.spec-vr.tsx-snapshots/should-respect-dimensions-of-a-grid-parent-1-chromium-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/ResponsiveContainer.spec-vr.tsx-snapshots/should-respect-dimensions-of-a-grid-parent-1-firefox-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/ResponsiveContainer.spec-vr.tsx-snapshots/should-respect-dimensions-of-a-grid-parent-1-webkit-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/ResponsiveContainer.spec-vr.tsx-snapshots/should-respect-minWidth-with-aspect-ratio-1-chromium-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/ResponsiveContainer.spec-vr.tsx-snapshots/should-respect-minWidth-with-aspect-ratio-1-firefox-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/ResponsiveContainer.spec-vr.tsx-snapshots/should-respect-minWidth-with-aspect-ratio-1-webkit-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/ResponsiveContainer.spec-vr.tsx-snapshots/should-respect-width-in-a-tall-container-when-aspect-is-set-1-chromium-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/ResponsiveContainer.spec-vr.tsx-snapshots/should-respect-width-in-a-tall-container-when-aspect-is-set-1-firefox-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/ResponsiveContainer.spec-vr.tsx-snapshots/should-respect-width-in-a-tall-container-when-aspect-is-set-1-webkit-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/www/ComposedChartApiExamples.spec-vr.tsx-snapshots/LineBarAreaComposedChart-1-chromium-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/www/ComposedChartApiExamples.spec-vr.tsx-snapshots/LineBarAreaComposedChart-1-firefox-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/www/ComposedChartApiExamples.spec-vr.tsx-snapshots/LineBarAreaComposedChart-1-webkit-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/www/LegendApiExamples.spec-vr.tsx-snapshots/LegendExample-1-chromium-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/www/LegendApiExamples.spec-vr.tsx-snapshots/LegendExample-1-firefox-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/www/LegendApiExamples.spec-vr.tsx-snapshots/LegendExample-1-webkit-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/www/ScatterChartApiExamples.spec-vr.tsx-snapshots/ScatterChartWithLabels-1-chromium-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/www/ScatterChartApiExamples.spec-vr.tsx-snapshots/ScatterChartWithLabels-1-firefox-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/www/ScatterChartApiExamples.spec-vr.tsx-snapshots/ScatterChartWithLabels-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 (29)
scripts/snapshots/es6Files.txt(2 hunks)scripts/snapshots/libFiles.txt(2 hunks)scripts/snapshots/typesFiles.txt(2 hunks)src/cartesian/Bar.tsx(5 hunks)src/cartesian/CartesianGrid.tsx(4 hunks)src/cartesian/Line.tsx(5 hunks)src/component/LabelList.tsx(3 hunks)src/container/RootSurface.tsx(2 hunks)src/context/chartLayoutContext.tsx(1 hunks)src/state/selectors/arrayEqualityCheck.ts(1 hunks)src/state/selectors/axisSelectors.ts(3 hunks)src/state/selectors/tooltipSelectors.ts(3 hunks)src/state/store.ts(2 hunks)src/state/zIndexSlice.ts(1 hunks)src/zindex/DefaultZIndexes.tsx(1 hunks)src/zindex/ZIndexLayer.tsx(1 hunks)src/zindex/ZIndexPortal.tsx(1 hunks)src/zindex/zIndexSelectors.ts(1 hunks)storybook/stories/API/chart/BarChart.stories.tsx(1 hunks)test-vr/tests/zIndex.spec-vr.tsx(1 hunks)test/cartesian/CartesianGrid.spec.tsx(5 hunks)test/component/Legend.itemSorter.spec.tsx(5 hunks)test/component/Legend.spec.tsx(5 hunks)test/component/Tooltip/Tooltip.visibility.spec.tsx(1 hunks)test/helper/logAllReduxActionsMiddleware.ts(1 hunks)test/state/selectors/selectCartesianItemsSettings.spec.tsx(1 hunks)test/zindex/AllZIndexPortals.spec.tsx(1 hunks)test/zindex/zIndexSelectors.spec.ts(1 hunks)www/src/docs/exampleComponents/ScatterChart/ScatterChartWithLabels.tsx(3 hunks)
🔇 Additional comments (32)
scripts/snapshots/es6Files.txt (1)
187-187: Snapshot updates are correctly ordered and follow naming conventions.The new entries for
zIndexSlice.jsand thezindex/directory are properly positioned in alphabetical order and follow existing naming conventions. The Redux slice naming (lowercase) is consistent with other slices liketooltipSlice.js, and the zindex module files maintain proper alphabetical ordering (DefaultZIndexes, ZIndexLayer, ZIndexPortal, zIndexSelectors).Also applies to: 250-254
test/state/selectors/selectCartesianItemsSettings.spec.tsx (1)
87-87: LGTM! Dynamic ID matching is appropriate.The change from a hardcoded ID to a regex pattern correctly accommodates the dynamic ID generation introduced by the z-index system.
src/context/chartLayoutContext.tsx (1)
136-154: LGTM! Well-designed utility hook.The implementation is clean and the documentation clearly explains the use case. Checking for layout presence is a reliable way to determine chart context.
test/helper/logAllReduxActionsMiddleware.ts (1)
1-6: LGTM! Useful debugging utility.This is a straightforward Redux middleware helper for test debugging. The
anytypes are acceptable for test utilities, and the console logging serves its intended purpose.src/state/selectors/arrayEqualityCheck.ts (2)
1-13: LGTM! Clean separation of equality checks.The split between reference/empty check and content check provides clearer semantics for different use cases. The implementation correctly handles empty arrays as a special case for memoization.
15-30: LGTM! Element-wise comparison is correct.The implementation properly checks length first before iterating, and uses strict equality for element comparison.
src/container/RootSurface.tsx (2)
10-10: LGTM! Z-index portal integration.The import integrates the new z-index portal system into the root surface rendering.
102-104: LGTM! Appropriate wrapping for z-index portals.Wrapping children with
AllZIndexPortalsonly in the main chart surface (not panorama) is the correct integration point for the z-index system.src/state/store.ts (2)
21-21: LGTM! Z-index slice import.Standard Redux Toolkit import following the existing pattern.
37-37: LGTM! Z-index reducer integration.The z-index slice is correctly added to the root reducer, extending the store state appropriately.
www/src/docs/exampleComponents/ScatterChart/ScatterChartWithLabels.tsx (4)
1-1: LGTM! Required import for ZAxis.
15-15: LGTM! Expanded width for better visualization.The increased width provides more space for the scatter plot with bubble sizing.
29-29: LGTM! Demonstrates the new zIndex feature.The
zIndex={3}prop demonstrates the new z-index functionality, andfill="black"improves label visibility.
31-31: LGTM! Adds bubble size dimension.The ZAxis with
range={[900, 4000]}provides bubble sizing based on thezdataKey.test/component/Tooltip/Tooltip.visibility.spec.tsx (1)
788-788: LGTM! Test expectation updated for z-index field.The test correctly expects the
zIndexproperty (likely fromDefaultZIndexes.line) in the legend payload, aligning with the new z-index system.test/component/Legend.itemSorter.spec.tsx (1)
10-11: LGTM on DefaultZIndexes import.Import path and intent are clear.
test/component/Legend.spec.tsx (1)
39-40: LGTM on DefaultZIndexes import.Consistent with the new z-index system.
test/cartesian/CartesianGrid.spec.tsx (1)
33-34: LGTM on DefaultZIndexes import and zIndex expectations.Validates zIndex passes through grid line renderers.
src/state/selectors/axisSelectors.ts (1)
80-81: Verify intent: confirm whether emptyArraysAreEqualCheck trade-off is intentional.The change has been applied to both selectors (lines 342–345 and 390–393), and tests validate that outputs are stable between renders when inputs don't change. However, this only validates that the same reference is returned when input selectors are unchanged—it does not validate the behavior when inputs produce new non-empty arrays with equal content.
With
emptyArraysAreEqualCheck, non-empty arrays fail equality unless they share the same reference, which could trigger unnecessary selector re-evaluations and downstream re-renders ifcombineGraphicalItemsSettingsorcombineGraphicalItemsDataconsistently produce new array instances despite equal content.The codebase already uses
arrayContentsAreEqualCheckelsewhere (e.g.,src/zindex/zIndexSelectors.ts:36), confirming both options are available. Clarify whether the trade-off favoring performance (faster shallow checks) over stability (deep equality) is intentional, and document the rationale if so.scripts/snapshots/libFiles.txt (1)
187-187: Snapshot additions look consistent; verify ES6/lib parity and public re-exports.Ensure es6 snapshots mirror these lib entries and that public entrypoints (lib/index.js, es6/index.js, types/index.d.ts) re-export zindex and zIndexSlice modules.
Also applies to: 250-254
scripts/snapshots/typesFiles.txt (1)
187-187: Types snapshot OK; confirm ZIndexable is exported and no circular deps.Make sure types/zindex/ZIndexLayer.d.ts exports ZIndexable, and that types/index.d.ts exposes these new declarations without cycles.
Also applies to: 250-254
src/cartesian/Bar.tsx (3)
83-85: Public zIndex prop added; defaults set.
- BarProps extending ZIndexable and default zIndex = DefaultZIndexes.bar looks good.
- Confirm docs/TS types mention negative zIndex support.
- Verify svgPropertiesNoEvents filters zIndex so it’s not passed to DOM.
Also applies to: 107-136, 647-648
860-863: Portal/clipPath interaction: please verify.ZIndexLayer wraps BarImpl (which defines /). Ensure the portal keeps rendering within the same so clipPath URLs resolve and pointer events layering matches expectations.
869-869: Type annotation on Bar export is optional.
ComponentType<Props>is safe but not required; no change needed unless tooling prefers it.src/cartesian/Line.tsx (1)
54-56: zIndex prop on Line: defaults and types look good.Default zIndex = DefaultZIndexes.line is reasonable. Please document the new public prop and negative values support.
Also applies to: 71-71, 111-111, 670-671
src/cartesian/CartesianGrid.tsx (1)
22-23: LGTM! Clean integration of z-index support.The z-index integration follows a consistent pattern: extending props with
ZIndexable, setting a sensible default fromDefaultZIndexes.grid, and wrapping the render output inZIndexLayer. The existing grid rendering logic is preserved, and the z-index layer is applied at the appropriate level.Also applies to: 64-64, 397-397, 496-527
src/zindex/ZIndexPortal.tsx (1)
21-42: LGTM! Correct portal rendering order.The portal rendering order (negative z-indexes → children → positive z-indexes) correctly implements layering behavior. The intentional exclusion of zero (the default layer) is well-documented and avoids unnecessary portal overhead.
src/zindex/zIndexSelectors.ts (1)
25-39: LGTM! Proper memoization and z-index aggregation.The selector correctly combines dynamic z-indexes from the map with default z-indexes, ensures uniqueness, and sorts the result. The use of
arrayContentsAreEqualCheckforresultEqualityCheckprevents unnecessary re-renders when the array contents haven't changed.src/component/LabelList.tsx (1)
12-13: LGTM! Clean z-index integration with appropriate fallback.The implementation correctly:
- Extends
LabelListPropswithZIndexablefor public API- Extracts
zIndexfrom props- Wraps rendering in
ZIndexLayerwith fallback toDefaultZIndexes.labelThe fallback pattern (
zIndex ?? DefaultZIndexes.label) ensures labels have a sensible default z-index when not explicitly specified.Also applies to: 52-52, 106-106, 115-146
test/zindex/AllZIndexPortals.spec.tsx (2)
38-41: Verify necessity ofvi.runOnlyPendingTimers().The use of
vi.runOnlyPendingTimers()after dispatching the Redux action seems unusual, as bothuseLayoutEffectand Redux dispatch are synchronous operations. This might be working around React's render batching, but it's worth verifying whether this timer advancement is actually necessary or if the test would pass without it.If timers are needed for this test, please add a comment explaining why. If they're not needed, consider removing the timer call for clarity:
act(() => { store.dispatch(registerZIndexPortal({ zIndex: newZIndexDefinitelyNotOneOfTheDefaults })); - vi.runOnlyPendingTimers(); });
14-71: Good test coverage of portal lifecycle.The tests effectively verify:
- Dynamic portal registration increases portal count
- Each portal has a unique element ID
- Portal IDs in the DOM match the store state
The test setup properly uses
RechartsReduxContextand creates an isolated store instance.src/state/zIndexSlice.ts (1)
1-110: LGTM! Well-structured Redux slice with proper lifecycle management.The implementation demonstrates good practices:
- Reference counting via
consumersto track active usage- Garbage collection of unused z-indexes while preserving defaults (Lines 68-76)
- Clear separation between portal registration and element ID registration
- Proper use of
prepareAutoBatchedfor all actions to optimize batching- Well-documented rationale for preserving default z-indexes
The dual-phase registration (portal → element ID) correctly handles the async nature of React rendering.
|
Visual Regression tests failed.
|
|
Visual Regression tests failed.
|
|
OK I think I'm done. It's a huge chunk, sorry @ckifer for the huge PR. I can break it down if you want. I'd like your opinion on the default ordering and visual diff please |
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/polar/PolarGrid.tsx (1)
178-187: Bug: default zIndex is overwritten by ‘...inputs’.Spreading inputs after setting zIndex can reset it to undefined. Move zIndex after the spread to preserve the default.
Apply:
const props: PropsWithDefaults = { cx: polarViewBox?.cx ?? cxFromOutside ?? 0, cy: polarViewBox?.cy ?? cyFromOutside ?? 0, innerRadius: polarViewBox?.innerRadius ?? innerRadiusFromOutside ?? 0, outerRadius: polarViewBox?.outerRadius ?? outerRadiusFromOutside ?? 0, - zIndex: inputs.zIndex ?? DefaultZIndexes.grid, polarAngles, polarRadius, - ...inputs, + ...inputs, + zIndex: inputs.zIndex ?? DefaultZIndexes.grid, };src/cartesian/ReferenceLine.tsx (1)
288-302: Missing ZIndexLayer wrapper.The
zIndexprop is declared, extended fromZIndexable(line 42), and defaulted toDefaultZIndexes.line(line 283), but it's never actually used in the rendering. Unlike other components (e.g., Bar, ActivePoints, LabelList),ReferenceLineImplis not wrapped withZIndexLayer.Apply this diff to wrap the rendering with ZIndexLayer:
+import { ZIndexLayer } from '../zindex/ZIndexLayer'; export function ReferenceLine(outsideProps: Props) { const props: PropsWithDefaults = resolveDefaultProps(outsideProps, referenceLineDefaultProps); return ( <> <ReportReferenceLine yAxisId={props.yAxisId} xAxisId={props.xAxisId} ifOverflow={props.ifOverflow} x={props.x} y={props.y} /> - <ReferenceLineImpl {...props} /> + <ZIndexLayer zIndex={props.zIndex}> + <ReferenceLineImpl {...props} /> + </ZIndexLayer> </> ); }
♻️ Duplicate comments (2)
src/zindex/ZIndexLayer.tsx (1)
84-91: Previous concern resolved: portalId is now guarded.The null-check prevents calling getElementById with undefined. No action needed.
src/cartesian/Line.tsx (1)
568-602: Line visuals are now properly portaled. LGTM.Wrapping the rendered Layer/ActivePoints with ZIndexLayer fixes the earlier “children wrapper” issue and makes zIndex effective for Line.
🧹 Nitpick comments (12)
test/state/selectors/selectCartesianItemsSettings.spec.tsx (1)
87-87: Anchor the dynamic id regex to avoid false positives.Match the whole id to keep the assertion strict.
Apply:
- id: expect.stringMatching(/recharts-line-.+/), + id: expect.stringMatching(/^recharts-line-[\w:-]+$/),src/zindex/DefaultZIndexes.tsx (2)
4-57: Make defaults immutable and strongly typed.Prevent accidental mutation and lock key/value shapes.
Apply:
-export const DefaultZIndexes = { +export type DefaultZIndexesType = Readonly<{ + grid: -100; + area: 100; + cursor: 200; + bar: 300; + line: 400; + axisLineAndTick: 500; + scatter: 600; + label: 600; + activeBar: 1000; + activeDot: 1100; +}>; + +export const DefaultZIndexes: DefaultZIndexesType = Object.freeze({ grid: -100, area: 100, cursor: 200, bar: 300, line: 400, axisLineAndTick: 500, scatter: 600, label: 600, activeBar: 1000, activeDot: 1100, -}; +} as const);Optional: rename file to
.tssince there’s no JSX.
42-48: Confirm tie-breaker semantics for equal zIndex (scatter vs label at 600).If labels must overlay dots, consider bumping
labelabovescatter, or document that equal values follow DOM order within the same portal.src/zindex/ZIndexPortal.tsx (1)
12-18: Use an isomorphic layout effect to avoid SSR warnings.Switch to an isomorphic layout effect so server rendering doesn’t emit “useLayoutEffect does nothing on the server” warnings.
Apply:
-import { useLayoutEffect } from 'react'; +import { useLayoutEffect, useEffect } from 'react'; +const useIsoLayoutEffect = typeof window !== 'undefined' ? useLayoutEffect : useEffect; @@ - useLayoutEffect(() => { + useIsoLayoutEffect(() => { dispatch(registerZIndexPortalId({ zIndex, elementId: portalId, isPanorama })); return () => { dispatch(unregisterZIndexPortalId({ zIndex, isPanorama })); }; }, [dispatch, zIndex, portalId, isPanorama]);src/zindex/ZIndexLayer.tsx (2)
93-97: SSR safety: guard direct DOM access.Accessing document during SSR can throw. Early-return if document is undefined.
- const zIndexPortal = document.getElementById(portalId); + if (typeof document === 'undefined') { + return null; + } + const zIndexPortal = document.getElementById(portalId);
20-31: Consider making zIndex optional in props for ergonomics.Changing to zIndex?: number matches the interface and avoids passing undefined explicitly.
-type ZIndexLayerProps = { +type ZIndexLayerProps = { - zIndex: number | undefined; + zIndex?: number; children?: React.ReactNode; };test/cartesian/Brush.spec.tsx (1)
57-58: Avoid unconditional debug noise in tests.Please remove or gate debug() behind a flag (e.g., process.env.DEBUG) to keep CI output clean.
- const { container, debug } = renderTestCase(); - debug(); + const { container /*, debug */ } = renderTestCase(); + // if (process.env.DEBUG) debug();test/cartesian/Bar.spec.tsx (1)
1029-1033: Confirm label zIndex default vs DefaultZIndexes.label.Test expects zIndex: 0 for label props, while DefaultZIndexes.label is 600. If intentional (labels render in base layer), add a note in docs/tests; otherwise align the default.
test/chart/BarChart.spec.tsx (1)
2346-2401: Harden Brush assertions against z-index/portal DOM reorderingPortals can change DOM order; asserting the full concatenated bar list at the root container is brittle. Prefer scoping to the Brush subtree (e.g., within(container.querySelector('.recharts-brush')), or a data-testid) or asserting set membership with order-agnostic checks for the Brush bars only. This will make the test resilient to z-index layering.
src/component/Cursor.tsx (1)
24-26: Cursor zIndex currently unused; confirm layering or wrap with ZIndexLayerYou pass a defaulted zIndex but CursorInternal doesn’t use it. If no higher layer handles portals for Cursor, wrap the rendered element here to ensure it sits below points but above grid. If Tooltip already controls it, ignore.
Optional change:
@@ -import { ZIndexable } from '../zindex/ZIndexLayer'; +import { ZIndexable, ZIndexLayer } from '../zindex/ZIndexLayer'; @@ export function CursorInternal(props: CursorConnectedProps) { - if (isValidElement(cursor)) { - // @ts-expect-error we don't know if cursorProps are correct for this element - return cloneElement(cursor, cursorProps); - } - return createElement(cursorComp, cursorProps); + if (isValidElement(cursor)) { + return ( + <ZIndexLayer zIndex={props.zIndex ?? DefaultZIndexes.cursor}> + {cloneElement(cursor, cursorProps)} + </ZIndexLayer> + ); + } + return ( + <ZIndexLayer zIndex={props.zIndex ?? DefaultZIndexes.cursor}> + {createElement(cursorComp, cursorProps)} + </ZIndexLayer> + ); }Also applies to: 34-40, 130-131
src/component/Dots.tsx (1)
88-99: Verify cross-portal clipPath behavior.Dots are portaled (zIndex 600 by default) while their clipPath defs can be defined in a different portal (e.g., Line at 400). This should work in modern browsers, but please double-check Safari/Firefox for any transient “no clipPath” frames during first render.
If issues arise, consider hoisting clipPath defs to a shared root defs container rendered unportaled by the Chart, or pre-registering portals earlier to avoid a one-tick null render.
Also applies to: 128-134
test/zindex/AllZIndexPortals.spec.tsx (1)
41-44: Stabilize timing in test to avoid flakiness.You call runOnlyPendingTimers without enabling fake timers. Use fake timers or switch to waitFor.
Suggested options:
- Use fake timers
beforeEach(() => { store = createRechartsStore(); + vi.useFakeTimers(); }); + afterEach(() => { + vi.useRealTimers(); + });
- Or, avoid timers and wait for DOM updates
- act(() => { - store.dispatch(registerZIndexPortal({ zIndex: newZIndexDefinitelyNotOneOfTheDefaults })); - vi.runOnlyPendingTimers(); - }); + store.dispatch(registerZIndexPortal({ zIndex: newZIndexDefinitelyNotOneOfTheDefaults })); + const zIndexPortals = await screen.findAllBySelector('[id^="recharts-zindex"]'); + expect(zIndexPortals).toHaveLength(allZIndexes.length + 1);(Add waitFor/screen as needed.)
Based on learnings
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (87)
test-vr/__snapshots__/tests/AreaChart.stacked.spec-vr.tsx-snapshots/multiple-data-arrays-exclusive-dataKey-1-chromium-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/AreaChart.stacked.spec-vr.tsx-snapshots/multiple-data-arrays-exclusive-dataKey-1-firefox-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/AreaChart.stacked.spec-vr.tsx-snapshots/multiple-data-arrays-exclusive-dataKey-1-webkit-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/AreaChart.stacked.spec-vr.tsx-snapshots/multiple-data-arrays-repeated-dataKey-1-chromium-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/AreaChart.stacked.spec-vr.tsx-snapshots/multiple-data-arrays-repeated-dataKey-1-firefox-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/AreaChart.stacked.spec-vr.tsx-snapshots/multiple-data-arrays-repeated-dataKey-1-webkit-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/AreaChart.stacked.spec-vr.tsx-snapshots/multiple-data-arrays-repeated-dataKey-without-XAxis-dataKey-1-chromium-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/AreaChart.stacked.spec-vr.tsx-snapshots/multiple-data-arrays-repeated-dataKey-without-XAxis-dataKey-1-firefox-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/AreaChart.stacked.spec-vr.tsx-snapshots/multiple-data-arrays-repeated-dataKey-without-XAxis-dataKey-1-webkit-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/AreaChart.stacked.spec-vr.tsx-snapshots/single-data-array-1-chromium-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/AreaChart.stacked.spec-vr.tsx-snapshots/single-data-array-1-firefox-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/AreaChart.stacked.spec-vr.tsx-snapshots/single-data-array-1-webkit-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/AreaChart.stacked.spec-vr.tsx-snapshots/single-data-array-without-XAxis-dataKey-1-chromium-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/AreaChart.stacked.spec-vr.tsx-snapshots/single-data-array-without-XAxis-dataKey-1-firefox-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/AreaChart.stacked.spec-vr.tsx-snapshots/single-data-array-without-XAxis-dataKey-1-webkit-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/Label.spec-vr.tsx-snapshots/ReferenceLine-Label-1-chromium-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/Label.spec-vr.tsx-snapshots/ReferenceLine-Label-1-firefox-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/Label.spec-vr.tsx-snapshots/ReferenceLine-Label-1-webkit-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/LabelList.spec-vr.tsx-snapshots/Bar-implicit-LabelList-1-chromium-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/LabelList.spec-vr.tsx-snapshots/Bar-implicit-LabelList-1-firefox-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/LabelList.spec-vr.tsx-snapshots/Bar-implicit-LabelList-1-webkit-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/LabelList.spec-vr.tsx-snapshots/Line-explicit-LabelList-1-chromium-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/LabelList.spec-vr.tsx-snapshots/Line-explicit-LabelList-1-firefox-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/LabelList.spec-vr.tsx-snapshots/Line-explicit-LabelList-1-webkit-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/LabelList.spec-vr.tsx-snapshots/Line-implicit-LabelList-1-chromium-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/LabelList.spec-vr.tsx-snapshots/Line-implicit-LabelList-1-firefox-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/LabelList.spec-vr.tsx-snapshots/Line-implicit-LabelList-1-webkit-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/LabelList.spec-vr.tsx-snapshots/Pie-implicit-LabelList-1-chromium-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/LabelList.spec-vr.tsx-snapshots/Pie-implicit-LabelList-1-firefox-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/LabelList.spec-vr.tsx-snapshots/Pie-implicit-LabelList-1-webkit-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/LabelList.spec-vr.tsx-snapshots/RadialBar-implicit-LabelList-1-chromium-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/LabelList.spec-vr.tsx-snapshots/RadialBar-implicit-LabelList-1-firefox-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/LabelList.spec-vr.tsx-snapshots/RadialBar-implicit-LabelList-1-webkit-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/ResponsiveContainer.spec-vr.tsx-snapshots/ResponsiveContainer-with-hardcoded-width-and-height-1-chromium-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/ResponsiveContainer.spec-vr.tsx-snapshots/ResponsiveContainer-with-hardcoded-width-and-height-1-firefox-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/ResponsiveContainer.spec-vr.tsx-snapshots/ResponsiveContainer-with-hardcoded-width-and-height-1-webkit-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/ResponsiveContainer.spec-vr.tsx-snapshots/aspect-ratio-should-be-overridden-by-maxHeight-1-chromium-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/ResponsiveContainer.spec-vr.tsx-snapshots/aspect-ratio-should-be-overridden-by-maxHeight-1-firefox-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/ResponsiveContainer.spec-vr.tsx-snapshots/aspect-ratio-should-be-overridden-by-maxHeight-1-webkit-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/ResponsiveContainer.spec-vr.tsx-snapshots/should-calculate-height-based-on-aspect-ratio-1-chromium-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/ResponsiveContainer.spec-vr.tsx-snapshots/should-calculate-height-based-on-aspect-ratio-1-firefox-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/ResponsiveContainer.spec-vr.tsx-snapshots/should-calculate-height-based-on-aspect-ratio-1-webkit-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/ResponsiveContainer.spec-vr.tsx-snapshots/should-create-a-square-chart-that-overflows-a-wide-screen-1-chromium-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/ResponsiveContainer.spec-vr.tsx-snapshots/should-create-a-square-chart-that-overflows-a-wide-screen-1-firefox-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/ResponsiveContainer.spec-vr.tsx-snapshots/should-create-a-square-chart-that-overflows-a-wide-screen-1-webkit-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/ResponsiveContainer.spec-vr.tsx-snapshots/should-create-a-square-chart-without-width-or-height-specified-1-chromium-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/ResponsiveContainer.spec-vr.tsx-snapshots/should-create-a-square-chart-without-width-or-height-specified-1-firefox-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/ResponsiveContainer.spec-vr.tsx-snapshots/should-create-a-square-chart-without-width-or-height-specified-1-webkit-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/ResponsiveContainer.spec-vr.tsx-snapshots/should-handle-mixed-fixed-and-percentage-sizing-1-chromium-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/ResponsiveContainer.spec-vr.tsx-snapshots/should-handle-mixed-fixed-and-percentage-sizing-1-firefox-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/ResponsiveContainer.spec-vr.tsx-snapshots/should-handle-mixed-fixed-and-percentage-sizing-1-webkit-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/ResponsiveContainer.spec-vr.tsx-snapshots/should-render-at-100-width-and-height-of-a-fixed-size-parent-1-chromium-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/ResponsiveContainer.spec-vr.tsx-snapshots/should-render-at-100-width-and-height-of-a-fixed-size-parent-1-firefox-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/ResponsiveContainer.spec-vr.tsx-snapshots/should-render-at-100-width-and-height-of-a-fixed-size-parent-1-webkit-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/ResponsiveContainer.spec-vr.tsx-snapshots/should-render-at-50-width-and-height-of-a-fixed-size-parent-1-chromium-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/ResponsiveContainer.spec-vr.tsx-snapshots/should-render-at-50-width-and-height-of-a-fixed-size-parent-1-firefox-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/ResponsiveContainer.spec-vr.tsx-snapshots/should-render-at-50-width-and-height-of-a-fixed-size-parent-1-webkit-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/ResponsiveContainer.spec-vr.tsx-snapshots/should-respect-dimensions-of-a-flexbox-parent-1-chromium-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/ResponsiveContainer.spec-vr.tsx-snapshots/should-respect-dimensions-of-a-flexbox-parent-1-firefox-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/ResponsiveContainer.spec-vr.tsx-snapshots/should-respect-dimensions-of-a-flexbox-parent-1-webkit-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/ResponsiveContainer.spec-vr.tsx-snapshots/should-respect-dimensions-of-a-grid-parent-1-chromium-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/ResponsiveContainer.spec-vr.tsx-snapshots/should-respect-dimensions-of-a-grid-parent-1-firefox-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/ResponsiveContainer.spec-vr.tsx-snapshots/should-respect-dimensions-of-a-grid-parent-1-webkit-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/ResponsiveContainer.spec-vr.tsx-snapshots/should-respect-minWidth-with-aspect-ratio-1-chromium-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/ResponsiveContainer.spec-vr.tsx-snapshots/should-respect-minWidth-with-aspect-ratio-1-firefox-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/ResponsiveContainer.spec-vr.tsx-snapshots/should-respect-minWidth-with-aspect-ratio-1-webkit-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/ResponsiveContainer.spec-vr.tsx-snapshots/should-respect-width-in-a-tall-container-when-aspect-is-set-1-chromium-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/ResponsiveContainer.spec-vr.tsx-snapshots/should-respect-width-in-a-tall-container-when-aspect-is-set-1-firefox-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/ResponsiveContainer.spec-vr.tsx-snapshots/should-respect-width-in-a-tall-container-when-aspect-is-set-1-webkit-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/www/LegendApiExamples.spec-vr.tsx-snapshots/LegendExample-1-chromium-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/www/LegendApiExamples.spec-vr.tsx-snapshots/LegendExample-1-firefox-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/www/LegendApiExamples.spec-vr.tsx-snapshots/LegendExample-1-webkit-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/www/LineChartApiExamples.spec-vr.tsx-snapshots/HighlightAndZoomLineChart-1-chromium-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/www/LineChartApiExamples.spec-vr.tsx-snapshots/HighlightAndZoomLineChart-1-firefox-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/www/LineChartApiExamples.spec-vr.tsx-snapshots/HighlightAndZoomLineChart-1-webkit-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/www/ReferenceLineApiExamples.spec-vr.tsx-snapshots/ReferenceLineExample-1-chromium-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/www/ReferenceLineApiExamples.spec-vr.tsx-snapshots/ReferenceLineExample-1-firefox-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/www/ReferenceLineApiExamples.spec-vr.tsx-snapshots/ReferenceLineExample-1-webkit-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/www/ReferenceLineApiExamples.spec-vr.tsx-snapshots/ReferenceLinePositionExample-1-chromium-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/www/ReferenceLineApiExamples.spec-vr.tsx-snapshots/ReferenceLinePositionExample-1-firefox-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/www/ReferenceLineApiExamples.spec-vr.tsx-snapshots/ReferenceLinePositionExample-1-webkit-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/www/ScatterChartApiExamples.spec-vr.tsx-snapshots/ScatterChartWithLabels-1-chromium-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/www/ScatterChartApiExamples.spec-vr.tsx-snapshots/ScatterChartWithLabels-1-firefox-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/www/ScatterChartApiExamples.spec-vr.tsx-snapshots/ScatterChartWithLabels-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 (48)
scripts/snapshots/es6Files.txt(2 hunks)scripts/snapshots/libFiles.txt(2 hunks)scripts/snapshots/typesFiles.txt(2 hunks)src/cartesian/Bar.tsx(5 hunks)src/cartesian/CartesianGrid.tsx(4 hunks)src/cartesian/Line.tsx(7 hunks)src/cartesian/ReferenceDot.tsx(2 hunks)src/cartesian/ReferenceLine.tsx(3 hunks)src/cartesian/Scatter.tsx(5 hunks)src/component/ActivePoints.tsx(6 hunks)src/component/Cursor.tsx(3 hunks)src/component/Dots.tsx(4 hunks)src/component/Label.tsx(3 hunks)src/component/LabelList.tsx(3 hunks)src/container/RootSurface.tsx(2 hunks)src/context/chartLayoutContext.tsx(1 hunks)src/polar/Pie.tsx(3 hunks)src/polar/PolarGrid.tsx(3 hunks)src/polar/Radar.tsx(3 hunks)src/polar/RadialBar.tsx(4 hunks)src/state/selectors/arrayEqualityCheck.ts(1 hunks)src/state/selectors/axisSelectors.ts(3 hunks)src/state/selectors/tooltipSelectors.ts(3 hunks)src/state/store.ts(2 hunks)src/state/zIndexSlice.ts(1 hunks)src/zindex/DefaultZIndexes.tsx(1 hunks)src/zindex/ZIndexLayer.tsx(1 hunks)src/zindex/ZIndexPortal.tsx(1 hunks)src/zindex/zIndexSelectors.ts(1 hunks)storybook/stories/API/chart/BarChart.stories.tsx(1 hunks)test-vr/tests/zIndex.spec-vr.tsx(1 hunks)test/cartesian/Bar.spec.tsx(1 hunks)test/cartesian/Brush.spec.tsx(1 hunks)test/cartesian/CartesianGrid.spec.tsx(5 hunks)test/cartesian/ReferenceLine/ReferenceLine.spec.tsx(0 hunks)test/chart/BarChart.spec.tsx(1 hunks)test/chart/ComposedChart.spec.tsx(1 hunks)test/chart/LineChart.spec.tsx(3 hunks)test/chart/ScatterChart.spec.tsx(1 hunks)test/component/Label.spec.tsx(8 hunks)test/component/Legend.itemSorter.spec.tsx(5 hunks)test/component/Legend.spec.tsx(5 hunks)test/component/Tooltip/Tooltip.visibility.spec.tsx(1 hunks)test/helper/logAllReduxActionsMiddleware.ts(1 hunks)test/polar/Radar.spec.tsx(3 hunks)test/state/selectors/selectCartesianItemsSettings.spec.tsx(1 hunks)test/zindex/AllZIndexPortals.spec.tsx(1 hunks)test/zindex/zIndexSelectors.spec.ts(1 hunks)
💤 Files with no reviewable changes (1)
- test/cartesian/ReferenceLine/ReferenceLine.spec.tsx
🚧 Files skipped from review as they are similar to previous changes (17)
- test/component/Legend.spec.tsx
- test/component/Tooltip/Tooltip.visibility.spec.tsx
- src/context/chartLayoutContext.tsx
- test/cartesian/CartesianGrid.spec.tsx
- src/state/selectors/arrayEqualityCheck.ts
- test/helper/logAllReduxActionsMiddleware.ts
- storybook/stories/API/chart/BarChart.stories.tsx
- src/state/store.ts
- test-vr/tests/zIndex.spec-vr.tsx
- src/state/selectors/tooltipSelectors.ts
- scripts/snapshots/es6Files.txt
- src/zindex/zIndexSelectors.ts
- scripts/snapshots/libFiles.txt
- src/state/selectors/axisSelectors.ts
- src/container/RootSurface.tsx
- test/zindex/zIndexSelectors.spec.ts
- scripts/snapshots/typesFiles.txt
🧰 Additional context used
📓 Path-based instructions (5)
{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/zindex/AllZIndexPortals.spec.tsxtest/component/Legend.itemSorter.spec.tsxtest/cartesian/Bar.spec.tsxtest/chart/ComposedChart.spec.tsxtest/polar/Radar.spec.tsxtest/cartesian/Brush.spec.tsxtest/state/selectors/selectCartesianItemsSettings.spec.tsxtest/component/Label.spec.tsxtest/chart/LineChart.spec.tsxtest/chart/ScatterChart.spec.tsxtest/chart/BarChart.spec.tsx
test/**/*.spec.{ts,tsx}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
test/**/*.spec.{ts,tsx}: When writing new code, aim for 100% unit test code coverage
When adding or changing functionality, add appropriate tests (prefer unit tests; use RTL if rendering is involved)
Files:
test/zindex/AllZIndexPortals.spec.tsxtest/component/Legend.itemSorter.spec.tsxtest/cartesian/Bar.spec.tsxtest/chart/ComposedChart.spec.tsxtest/polar/Radar.spec.tsxtest/cartesian/Brush.spec.tsxtest/state/selectors/selectCartesianItemsSettings.spec.tsxtest/component/Label.spec.tsxtest/chart/LineChart.spec.tsxtest/chart/ScatterChart.spec.tsxtest/chart/BarChart.spec.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
**/*.{ts,tsx}: Ensure code lints cleanly before submitting PRs (npm run lint)
Never use the TypeScript any type (implicit or explicit)
Prefer unknown over any and refine types appropriately
Explicitly type all function parameters and return values; do not rely on implicit any or inference
Do not use as type assertions; the only exception is as const
Files:
test/zindex/AllZIndexPortals.spec.tsxtest/component/Legend.itemSorter.spec.tsxsrc/cartesian/ReferenceDot.tsxtest/cartesian/Bar.spec.tsxsrc/polar/Pie.tsxsrc/zindex/ZIndexLayer.tsxsrc/component/Cursor.tsxsrc/polar/RadialBar.tsxsrc/component/Dots.tsxtest/chart/ComposedChart.spec.tsxsrc/component/Label.tsxtest/polar/Radar.spec.tsxtest/cartesian/Brush.spec.tsxsrc/cartesian/CartesianGrid.tsxsrc/polar/Radar.tsxsrc/polar/PolarGrid.tsxsrc/state/zIndexSlice.tssrc/zindex/ZIndexPortal.tsxtest/state/selectors/selectCartesianItemsSettings.spec.tsxsrc/component/LabelList.tsxsrc/cartesian/Line.tsxtest/component/Label.spec.tsxsrc/cartesian/Scatter.tsxtest/chart/LineChart.spec.tsxsrc/component/ActivePoints.tsxtest/chart/ScatterChart.spec.tsxsrc/cartesian/ReferenceLine.tsxsrc/zindex/DefaultZIndexes.tsxsrc/cartesian/Bar.tsxtest/chart/BarChart.spec.tsx
test/component/**/*.spec.tsx
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Test rendering behavior and component interactions with React Testing Library
Files:
test/component/Legend.itemSorter.spec.tsxtest/component/Label.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/ReferenceDot.tsxsrc/polar/Pie.tsxsrc/zindex/ZIndexLayer.tsxsrc/component/Cursor.tsxsrc/polar/RadialBar.tsxsrc/component/Dots.tsxsrc/component/Label.tsxsrc/cartesian/CartesianGrid.tsxsrc/polar/Radar.tsxsrc/polar/PolarGrid.tsxsrc/state/zIndexSlice.tssrc/zindex/ZIndexPortal.tsxsrc/component/LabelList.tsxsrc/cartesian/Line.tsxsrc/cartesian/Scatter.tsxsrc/component/ActivePoints.tsxsrc/cartesian/ReferenceLine.tsxsrc/zindex/DefaultZIndexes.tsxsrc/cartesian/Bar.tsx
🧠 Learnings (1)
📚 Learning: 2025-10-25T07:35:14.559Z
Learnt from: CR
PR: recharts/recharts#0
File: CONTRIBUTING.md:0-0
Timestamp: 2025-10-25T07:35:14.559Z
Learning: Applies to test/component/**/*.spec.tsx : Test rendering behavior and component interactions with React Testing Library
Applied to files:
test/zindex/AllZIndexPortals.spec.tsx
🧬 Code graph analysis (22)
test/zindex/AllZIndexPortals.spec.tsx (5)
src/state/RechartsReduxContext.tsx (2)
RechartsReduxContextValue(14-19)RechartsReduxContext(27-27)src/state/store.ts (2)
RechartsRootState(94-94)createRechartsStore(40-92)src/zindex/ZIndexPortal.tsx (1)
AllZIndexPortals(21-43)src/zindex/zIndexSelectors.ts (1)
selectAllRegisteredZIndexes(36-50)src/zindex/DefaultZIndexes.tsx (1)
DefaultZIndexes(4-57)
test/component/Legend.itemSorter.spec.tsx (1)
src/zindex/DefaultZIndexes.tsx (1)
DefaultZIndexes(4-57)
src/cartesian/ReferenceDot.tsx (2)
src/zindex/ZIndexLayer.tsx (1)
ZIndexable(11-18)src/zindex/DefaultZIndexes.tsx (1)
DefaultZIndexes(4-57)
src/polar/Pie.tsx (2)
src/zindex/ZIndexLayer.tsx (1)
ZIndexable(11-18)src/zindex/DefaultZIndexes.tsx (1)
DefaultZIndexes(4-57)
src/zindex/ZIndexLayer.tsx (4)
src/context/chartLayoutContext.tsx (1)
useIsInChartContext(147-154)src/context/PanoramaContext.tsx (1)
useIsPanorama(6-6)src/state/hooks.ts (2)
useAppDispatch(9-15)useAppSelector(40-50)src/zindex/zIndexSelectors.ts (1)
selectZIndexPortalId(13-34)
src/component/Cursor.tsx (2)
src/zindex/ZIndexLayer.tsx (1)
ZIndexable(11-18)src/zindex/DefaultZIndexes.tsx (1)
DefaultZIndexes(4-57)
src/polar/RadialBar.tsx (3)
src/zindex/ZIndexLayer.tsx (2)
ZIndexable(11-18)ZIndexLayer(44-103)src/container/Layer.tsx (1)
Layer(13-22)src/zindex/DefaultZIndexes.tsx (1)
DefaultZIndexes(4-57)
src/component/Dots.tsx (2)
src/zindex/ZIndexLayer.tsx (2)
ZIndexable(11-18)ZIndexLayer(44-103)src/zindex/DefaultZIndexes.tsx (1)
DefaultZIndexes(4-57)
src/component/Label.tsx (2)
src/zindex/ZIndexLayer.tsx (1)
ZIndexable(11-18)src/zindex/DefaultZIndexes.tsx (1)
DefaultZIndexes(4-57)
test/polar/Radar.spec.tsx (1)
src/zindex/DefaultZIndexes.tsx (1)
DefaultZIndexes(4-57)
src/cartesian/CartesianGrid.tsx (2)
src/zindex/ZIndexLayer.tsx (2)
ZIndexable(11-18)ZIndexLayer(44-103)src/zindex/DefaultZIndexes.tsx (1)
DefaultZIndexes(4-57)
src/polar/Radar.tsx (2)
src/zindex/ZIndexLayer.tsx (1)
ZIndexable(11-18)src/zindex/DefaultZIndexes.tsx (1)
DefaultZIndexes(4-57)
src/polar/PolarGrid.tsx (2)
src/zindex/ZIndexLayer.tsx (2)
ZIndexable(11-18)ZIndexLayer(44-103)src/zindex/DefaultZIndexes.tsx (1)
DefaultZIndexes(4-57)
src/state/zIndexSlice.ts (1)
src/zindex/DefaultZIndexes.tsx (1)
DefaultZIndexes(4-57)
src/zindex/ZIndexPortal.tsx (3)
src/util/useUniqueId.ts (1)
useUniqueId(13-27)src/state/hooks.ts (2)
useAppDispatch(9-15)useAppSelector(40-50)src/zindex/zIndexSelectors.ts (1)
selectAllRegisteredZIndexes(36-50)
src/component/LabelList.tsx (7)
src/zindex/ZIndexLayer.tsx (2)
ZIndexable(11-18)ZIndexLayer(44-103)src/zindex/DefaultZIndexes.tsx (1)
DefaultZIndexes(4-57)src/container/Layer.tsx (1)
Layer(13-22)src/util/DataUtils.ts (1)
isNullish(179-181)src/util/ChartUtils.ts (1)
getValueByDataKey(41-55)src/component/Label.tsx (1)
Label(542-625)src/util/svgPropertiesAndEvents.ts (1)
svgPropertiesAndEvents(19-24)
src/cartesian/Line.tsx (2)
src/zindex/ZIndexLayer.tsx (2)
ZIndexable(11-18)ZIndexLayer(44-103)src/zindex/DefaultZIndexes.tsx (1)
DefaultZIndexes(4-57)
test/component/Label.spec.tsx (1)
src/zindex/DefaultZIndexes.tsx (1)
DefaultZIndexes(4-57)
src/cartesian/Scatter.tsx (3)
src/zindex/ZIndexLayer.tsx (2)
ZIndexable(11-18)ZIndexLayer(44-103)src/container/Layer.tsx (1)
Layer(13-22)src/zindex/DefaultZIndexes.tsx (1)
DefaultZIndexes(4-57)
src/component/ActivePoints.tsx (3)
src/util/types.ts (1)
adaptEventHandlers(838-865)src/zindex/ZIndexLayer.tsx (2)
ZIndexable(11-18)ZIndexLayer(44-103)src/zindex/DefaultZIndexes.tsx (1)
DefaultZIndexes(4-57)
src/cartesian/ReferenceLine.tsx (2)
src/zindex/ZIndexLayer.tsx (1)
ZIndexable(11-18)src/zindex/DefaultZIndexes.tsx (1)
DefaultZIndexes(4-57)
src/cartesian/Bar.tsx (2)
src/zindex/ZIndexLayer.tsx (2)
ZIndexable(11-18)ZIndexLayer(44-103)src/zindex/DefaultZIndexes.tsx (1)
DefaultZIndexes(4-57)
⏰ 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). (1)
- GitHub Check: Build, Test, Pack
🔇 Additional comments (28)
test/component/Legend.itemSorter.spec.tsx (1)
10-10: LGTM: tests aligned with zIndex surface.Importing
DefaultZIndexesand assertingzIndexin payload keeps tests resilient to future default changes.Also applies to: 97-98, 125-126, 189-190, 216-217
src/zindex/ZIndexPortal.tsx (1)
32-41: Portal ordering looks correct (negatives below, positives above).Children render between negative and positive layers; zero excluded as the base layer. Solid.
src/polar/RadialBar.tsx (3)
266-297: Public props correctly extended with zIndex.Extending
InternalRadialBarPropswithZIndexablecleanly exposes the new prop.
374-382: Correct placement of ZIndexLayer wrapper.Wrapping the existing
<Layer>preserves structure while enabling portal-based ordering.If you notice an extra initial render while the portal id is registered, that’s expected; just confirming this doesn’t regress any RadialBar tests.
426-427: Sane default:DefaultZIndexes.bar.Defaulting RadialBar to the bar layer is consistent with the rest of the system.
test/polar/Radar.spec.tsx (2)
14-14: LGTM: asserting zIndex via DefaultZIndexes.Importing and using DefaultZIndexes in expectations aligns tests with the new layering system.
319-320: Verify zIndex consistency across polar components.These payload zIndex assertions look correct; please confirm other polar elements use the same defaults to avoid brittle tests.
Also applies to: 346-347
test/chart/ComposedChart.spec.tsx (1)
50-51: Selector update is correct.Switching to '.recharts-line-dot' is compatible with portal/wrapper changes.
test/chart/ScatterChart.spec.tsx (1)
672-677: Validate updated panorama coordinates.Many cx/cy and transforms changed. Please confirm these reflect intended scale/viewBox adjustments (e.g., Brush panorama sizing) rather than a regression.
Also applies to: 680-685, 688-693, 696-701, 704-709, 712-717, 720-725, 728-733, 736-741, 744-749, 752-757, 760-765
test/chart/LineChart.spec.tsx (3)
425-427: LGTM: flatter selectors for curve/dot queries.Matches DOM after layering/wrapper changes.
769-771: LGTM: using '.recharts-line-dots' directly.Keeps the test resilient to wrapper element changes.
793-796: LGTM: post-brush expectations updated.Updated dot container query is correct after brush filtering.
test/component/Label.spec.tsx (1)
8-9: LGTM: tests validate Label’s zIndex propagation to content propsImport and expectations look correct and improve coverage of the new default.
Also applies to: 306-307, 467-468, 501-502, 539-539, 610-611, 639-640, 671-672
src/component/Label.tsx (1)
30-32: LGTM: Label exposes and defaults zIndex; content receives itPublic surface and defaults align with tests; propagation to content props is correct.
If LabelList isn’t wrapped elsewhere, consider placing ZIndexLayer at the LabelList level (not here) to avoid duplicating portals. Please confirm it’s covered.Also applies to: 68-83, 537-541
src/cartesian/CartesianGrid.tsx (1)
22-24: LGTM: Grid properly participates in z-index via ZIndexLayerDefaulting to DefaultZIndexes.grid and wrapping the grid group is correct. This is the pattern to replicate across components.
Also applies to: 64-147, 397-398, 496-528
src/polar/PolarGrid.tsx (1)
196-214: ZIndexLayer integration for PolarGrid looks good.Negative default (-100) matches DefaultZIndexes.grid and the portal ordering logic. Approved.
src/cartesian/Scatter.tsx (1)
607-628: Scatter z-index layering integrated correctly.Wrapper and default (scatter: 600) align with the system; symbols and line render inside the portal. LGTM.
src/component/LabelList.tsx (2)
12-13: LGTM: Clean z-index integration.The imports and interface extension correctly integrate z-index support into LabelList's public API.
Also applies to: 52-52
106-106: LGTM: Correct z-index inheritance pattern.The implementation correctly:
- Defaults to
DefaultZIndexes.labelwhen zIndex is not provided- Wraps all labels in a single
ZIndexLayer- Sets
zIndex={0}on individualLabelcomponents to prevent separate portaling and allow inheritance from the parentLabelListThe comment at lines 143-146 clearly explains this design decision.
Also applies to: 115-152
src/component/ActivePoints.tsx (2)
21-73: LGTM: Clean refactor to component.The rename from
renderActivePointtoActivePointcomponent is clean, and moving event handlers to the end of the spread (line 58) ensures they correctly override base properties.
75-116: LGTM: Correct z-index integration.The z-index support is correctly integrated:
ActivePointsPropsextendsZIndexable(line 75)- Default parameter
zIndex = DefaultZIndexes.activeDot(line 92)- Rendering wrapped with
ZIndexLayer(lines 107-115)src/cartesian/Bar.tsx (3)
107-107: LGTM: Z-index support added to Bar props.The interface extension and default value are correctly implemented.
Also applies to: 647-647
860-862: LGTM: ZIndexLayer wrapper correctly applied.The
BarImplrendering is correctly wrapped withZIndexLayer, enabling z-index-based layering.
869-869: LGTM: Explicit type declaration for Bar.The explicit
ComponentType<Props>declaration is correct for the memoized component and improves type clarity.src/state/zIndexSlice.ts (4)
9-29: LGTM: Well-defined types with good documentation.The type definitions are clear and well-documented, with proper separation between regular and panorama element IDs.
54-87: LGTM: Correct registration/unregistration logic.The reducers correctly:
- Use defensive checks before operating on entries
- Implement proper reference counting with
consumers- Preserve default z-index entries during garbage collection (lines 74-80)
- Use
prepareAutoBatchedfor performance
88-119: LGTM: Correct panorama-aware ID management.The reducers correctly handle both regular and panorama element IDs:
- Use defensive checks before operating
- Properly branch on
isPanoramaflag- Create entries with appropriate initialization when missing
46-48: LGTM: Clean helper and exports.The
isDefaultZIndexhelper is correctly implemented, and all necessary actions and the reducer are properly exported.Also applies to: 123-126
| * @param zIndex numeric zIndex value, higher values are rendered on top of lower values | ||
| * @param children the content to render inside this zIndex layer | ||
| */ | ||
| export function ZIndexLayer({ zIndex, children }: ZIndexLayerProps) { |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add an explicit return type for the component.
TypeScript guideline: explicitly annotate return types. Suggest returning React.ReactNode (covers children | null).
-export function ZIndexLayer({ zIndex, children }: ZIndexLayerProps) {
+export function ZIndexLayer({ zIndex, children }: ZIndexLayerProps): React.ReactNode {📝 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.
| export function ZIndexLayer({ zIndex, children }: ZIndexLayerProps) { | |
| export function ZIndexLayer({ zIndex, children }: ZIndexLayerProps): React.ReactNode { |
🤖 Prompt for AI Agents
In src/zindex/ZIndexLayer.tsx around line 44, the component function lacks an
explicit return type; update the function signature to annotate its return type
as React.ReactNode (e.g. function ZIndexLayer(...): React.ReactNode) so
TypeScript explicitly knows the component may return children or null; ensure
React is imported/available for the type reference and keep the existing props
typing unchanged.
|
Hah did I forget to commit a bunch of changes maybe |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
src/cartesian/ErrorBar.tsx (1)
227-242: zIndex prop incorrectly forwarded to ErrorBarImpl.The
zIndexprop is spread intoErrorBarImpl(line 232), butErrorBarInternalProps(lines 67-80) does not includezIndex. This causes a type mismatch and may leakzIndexintosvgProps(line 93).Based on past review comments.
Apply this fix to omit
zIndexbefore spreading:return ( <> <ReportErrorBarSettings dataKey={props.dataKey} direction={realDirection} /> <ZIndexLayer zIndex={zIndex}> - <ErrorBarImpl - {...props} + {(() => { + const { zIndex: _omit, ...rest } = props; + return ( + <ErrorBarImpl + {...rest} + direction={realDirection} + width={width} + isAnimationActive={isAnimationActive} + animationBegin={animationBegin} + animationDuration={animationDuration} + animationEasing={animationEasing} + /> + ); + })()} - direction={realDirection} - width={width} - isAnimationActive={isAnimationActive} - animationBegin={animationBegin} - animationDuration={animationDuration} - animationEasing={animationEasing} - /> </ZIndexLayer> </> );src/component/ActivePoints.tsx (1)
54-59: Potential runtime crash: spreading null from adaptEventHandlers.
adaptEventHandlers(activeDot)can returnnullwhenactiveDotis a boolean, function, or has certain invalid shapes (seesrc/util/types.ts). Spreadingnullinto an object literal throws a runtime error.Apply this diff to guard against null:
const dotProps: ActiveDotProps = { ...dotPropsTyped, ...svgPropertiesNoEventsFromUnknown(activeDot), - ...adaptEventHandlers(activeDot), + ...(adaptEventHandlers(activeDot) ?? {}), };src/polar/RadialBar.tsx (1)
382-387: Fix incorrect root CSS class nameRoot layer uses 'recharts-area' but this is RadialBar. Use 'recharts-radial-bar' for consistent DOM/CSS/test selectors.
- const layerClass = clsx('recharts-area', className); + const layerClass = clsx('recharts-radial-bar', className);
🧹 Nitpick comments (7)
test/zindex/AllZIndexPortals.spec.tsx (1)
33-40: Assertion intent is good; minor hardening suggestion.If defaults ever include 0, portal count would differ (0 is excluded by design). Consider asserting against
selectAllRegisteredZIndexes(...).filter(z=>z!==0).lengthto make the test future‑proof.Also applies to: 46-49
src/cartesian/Line.tsx (1)
741-766: Optional: align wrapper placement with Bar for consistency.Bar portals at the call site; Line portals inside
LineWithState. Functionally fine—consider moving the wrapper to the call site in a follow‑up for consistency across components.storybook/stories/API/chart/BarChart.stories.tsx (1)
45-59: Prefer named defaults for clarity.Using
DefaultZIndexesimproves readability and keeps stories aligned with library defaults.-import { Bar, BarChart, CartesianGrid, Legend, ResponsiveContainer, Tooltip, XAxis, YAxis } from '../../../../src'; +import { Bar, BarChart, CartesianGrid, Legend, ResponsiveContainer, Tooltip, XAxis, YAxis } from '../../../../src'; +import { DefaultZIndexes } from '../../../../src/zindex/DefaultZIndexes'; @@ - <Bar zIndex={1} ... label={{ position: 'insideTop', zIndex: 3, fill: 'black' }} /> + <Bar zIndex={DefaultZIndexes.bar - 299} ... label={{ position: 'insideTop', zIndex: DefaultZIndexes.label - 697, fill: 'black' }} /> @@ - <Bar zIndex={2} ... label={{ position: 'insideTop', zIndex: 3, fill: 'black' }} /> + <Bar zIndex={DefaultZIndexes.bar - 298} ... label={{ position: 'insideTop', zIndex: DefaultZIndexes.label - 697, fill: 'black' }} />(Any small offsets are fine—intent is referencing the constants.)
src/polar/PolarGrid.tsx (1)
186-187: Prefer centralized defaults pattern.Consider mirroring CartesianGrid’s resolveDefaultProps approach for zIndex here to keep defaulting strategy uniform across components.
src/state/selectors/arrayEqualityCheck.ts (1)
7-13: Tighten equality helpers and add fast path.Remove redundant Array.isArray checks (types already guarantee arrays) and add a reference‑equality fast path for contents check.
Apply:
-export function emptyArraysAreEqualCheck<T>(a: ReadonlyArray<T>, b: ReadonlyArray<T>): boolean { - if (Array.isArray(a) && Array.isArray(b) && a.length === 0 && b.length === 0) { - // empty arrays are always equal, regardless of reference - return true; - } - return a === b; -} +export function emptyArraysAreEqualCheck<T>(a: ReadonlyArray<T>, b: ReadonlyArray<T>): boolean { + if (a.length === 0 && b.length === 0) return true; + return a === b; +} @@ -export function arrayContentsAreEqualCheck<T>(a: ReadonlyArray<T>, b: ReadonlyArray<T>): boolean { - if (a.length === b.length) { - for (let i = 0; i < a.length; i++) { - if (a[i] !== b[i]) { - return false; - } - } - return true; - } - return false; -} +export function arrayContentsAreEqualCheck<T>(a: ReadonlyArray<T>, b: ReadonlyArray<T>): boolean { + if (a === b) return true; + const len = a.length; + if (len !== b.length) return false; + for (let i = 0; i < len; i++) { + if (a[i] !== b[i]) return false; + } + return true; +}Also applies to: 20-30
src/zindex/zIndexSelectors.ts (1)
36-50: Stable, deduped ordering; minor micro‑tweak optional.Ascending sort is right for paint order. Optionally, pre-check a===b in arrayContentsAreEqualCheck (see suggested refactor) for faster memo hits.
src/cartesian/ReferenceLine.tsx (1)
42-99: Public zIndex + minor typing nitExtending with ZIndexable looks good. Also, className should be string, not number | string.
Apply:
- className?: number | string; + className?: string;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (127)
test-vr/__snapshots__/tests/AreaChart.stacked.spec-vr.tsx-snapshots/data-on-chart-root-1-webkit-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/AreaChart.stacked.spec-vr.tsx-snapshots/multiple-data-arrays-exclusive-dataKey-1-chromium-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/AreaChart.stacked.spec-vr.tsx-snapshots/multiple-data-arrays-exclusive-dataKey-1-firefox-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/AreaChart.stacked.spec-vr.tsx-snapshots/multiple-data-arrays-exclusive-dataKey-1-webkit-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/AreaChart.stacked.spec-vr.tsx-snapshots/multiple-data-arrays-repeated-dataKey-1-chromium-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/AreaChart.stacked.spec-vr.tsx-snapshots/multiple-data-arrays-repeated-dataKey-1-firefox-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/AreaChart.stacked.spec-vr.tsx-snapshots/multiple-data-arrays-repeated-dataKey-1-webkit-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/AreaChart.stacked.spec-vr.tsx-snapshots/multiple-data-arrays-repeated-dataKey-without-XAxis-dataKey-1-chromium-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/AreaChart.stacked.spec-vr.tsx-snapshots/multiple-data-arrays-repeated-dataKey-without-XAxis-dataKey-1-firefox-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/AreaChart.stacked.spec-vr.tsx-snapshots/multiple-data-arrays-repeated-dataKey-without-XAxis-dataKey-1-webkit-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/AreaChart.stacked.spec-vr.tsx-snapshots/single-data-array-1-chromium-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/AreaChart.stacked.spec-vr.tsx-snapshots/single-data-array-1-firefox-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/AreaChart.stacked.spec-vr.tsx-snapshots/single-data-array-1-webkit-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/AreaChart.stacked.spec-vr.tsx-snapshots/single-data-array-without-XAxis-dataKey-1-chromium-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/AreaChart.stacked.spec-vr.tsx-snapshots/single-data-array-without-XAxis-dataKey-1-firefox-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/AreaChart.stacked.spec-vr.tsx-snapshots/single-data-array-without-XAxis-dataKey-1-webkit-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/Label.spec-vr.tsx-snapshots/PieChart-Label-1-chromium-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/Label.spec-vr.tsx-snapshots/PieChart-Label-1-firefox-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/Label.spec-vr.tsx-snapshots/PieChart-Label-1-webkit-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/Label.spec-vr.tsx-snapshots/PolarRadiusAxis-Label-1-chromium-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/Label.spec-vr.tsx-snapshots/PolarRadiusAxis-Label-1-firefox-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/Label.spec-vr.tsx-snapshots/PolarRadiusAxis-Label-1-webkit-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/Label.spec-vr.tsx-snapshots/RadarChart-Label-1-chromium-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/Label.spec-vr.tsx-snapshots/RadarChart-Label-1-firefox-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/Label.spec-vr.tsx-snapshots/RadarChart-Label-1-webkit-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/Label.spec-vr.tsx-snapshots/ReferenceArea-Label-1-chromium-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/Label.spec-vr.tsx-snapshots/ReferenceArea-Label-1-firefox-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/Label.spec-vr.tsx-snapshots/ReferenceArea-Label-1-webkit-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/Label.spec-vr.tsx-snapshots/ReferenceLine-Label-1-chromium-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/Label.spec-vr.tsx-snapshots/ReferenceLine-Label-1-firefox-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/Label.spec-vr.tsx-snapshots/ReferenceLine-Label-1-webkit-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/LabelList.spec-vr.tsx-snapshots/Bar-implicit-LabelList-1-chromium-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/LabelList.spec-vr.tsx-snapshots/Bar-implicit-LabelList-1-firefox-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/LabelList.spec-vr.tsx-snapshots/Bar-implicit-LabelList-1-webkit-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/LabelList.spec-vr.tsx-snapshots/Line-explicit-LabelList-1-chromium-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/LabelList.spec-vr.tsx-snapshots/Line-explicit-LabelList-1-firefox-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/LabelList.spec-vr.tsx-snapshots/Line-explicit-LabelList-1-webkit-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/LabelList.spec-vr.tsx-snapshots/Line-implicit-LabelList-1-chromium-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/LabelList.spec-vr.tsx-snapshots/Line-implicit-LabelList-1-firefox-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/LabelList.spec-vr.tsx-snapshots/Line-implicit-LabelList-1-webkit-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/LabelList.spec-vr.tsx-snapshots/Pie-implicit-LabelList-1-chromium-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/LabelList.spec-vr.tsx-snapshots/Pie-implicit-LabelList-1-firefox-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/LabelList.spec-vr.tsx-snapshots/Pie-implicit-LabelList-1-webkit-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/LabelList.spec-vr.tsx-snapshots/RadialBar-implicit-LabelList-1-chromium-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/LabelList.spec-vr.tsx-snapshots/RadialBar-implicit-LabelList-1-firefox-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/LabelList.spec-vr.tsx-snapshots/RadialBar-implicit-LabelList-1-webkit-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/ResponsiveContainer.spec-vr.tsx-snapshots/ResponsiveContainer-with-hardcoded-width-and-height-1-chromium-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/ResponsiveContainer.spec-vr.tsx-snapshots/ResponsiveContainer-with-hardcoded-width-and-height-1-firefox-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/ResponsiveContainer.spec-vr.tsx-snapshots/ResponsiveContainer-with-hardcoded-width-and-height-1-webkit-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/ResponsiveContainer.spec-vr.tsx-snapshots/aspect-ratio-should-be-overridden-by-maxHeight-1-chromium-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/ResponsiveContainer.spec-vr.tsx-snapshots/aspect-ratio-should-be-overridden-by-maxHeight-1-firefox-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/ResponsiveContainer.spec-vr.tsx-snapshots/aspect-ratio-should-be-overridden-by-maxHeight-1-webkit-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/ResponsiveContainer.spec-vr.tsx-snapshots/should-calculate-height-based-on-aspect-ratio-1-chromium-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/ResponsiveContainer.spec-vr.tsx-snapshots/should-calculate-height-based-on-aspect-ratio-1-firefox-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/ResponsiveContainer.spec-vr.tsx-snapshots/should-calculate-height-based-on-aspect-ratio-1-webkit-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/ResponsiveContainer.spec-vr.tsx-snapshots/should-create-a-square-chart-that-overflows-a-wide-screen-1-chromium-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/ResponsiveContainer.spec-vr.tsx-snapshots/should-create-a-square-chart-that-overflows-a-wide-screen-1-firefox-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/ResponsiveContainer.spec-vr.tsx-snapshots/should-create-a-square-chart-that-overflows-a-wide-screen-1-webkit-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/ResponsiveContainer.spec-vr.tsx-snapshots/should-create-a-square-chart-without-width-or-height-specified-1-chromium-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/ResponsiveContainer.spec-vr.tsx-snapshots/should-create-a-square-chart-without-width-or-height-specified-1-firefox-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/ResponsiveContainer.spec-vr.tsx-snapshots/should-create-a-square-chart-without-width-or-height-specified-1-webkit-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/ResponsiveContainer.spec-vr.tsx-snapshots/should-handle-mixed-fixed-and-percentage-sizing-1-chromium-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/ResponsiveContainer.spec-vr.tsx-snapshots/should-handle-mixed-fixed-and-percentage-sizing-1-firefox-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/ResponsiveContainer.spec-vr.tsx-snapshots/should-handle-mixed-fixed-and-percentage-sizing-1-webkit-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/ResponsiveContainer.spec-vr.tsx-snapshots/should-render-at-100-width-and-height-of-a-fixed-size-parent-1-chromium-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/ResponsiveContainer.spec-vr.tsx-snapshots/should-render-at-100-width-and-height-of-a-fixed-size-parent-1-firefox-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/ResponsiveContainer.spec-vr.tsx-snapshots/should-render-at-100-width-and-height-of-a-fixed-size-parent-1-webkit-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/ResponsiveContainer.spec-vr.tsx-snapshots/should-render-at-50-width-and-height-of-a-fixed-size-parent-1-chromium-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/ResponsiveContainer.spec-vr.tsx-snapshots/should-render-at-50-width-and-height-of-a-fixed-size-parent-1-firefox-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/ResponsiveContainer.spec-vr.tsx-snapshots/should-render-at-50-width-and-height-of-a-fixed-size-parent-1-webkit-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/ResponsiveContainer.spec-vr.tsx-snapshots/should-respect-dimensions-of-a-flexbox-parent-1-chromium-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/ResponsiveContainer.spec-vr.tsx-snapshots/should-respect-dimensions-of-a-flexbox-parent-1-firefox-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/ResponsiveContainer.spec-vr.tsx-snapshots/should-respect-dimensions-of-a-flexbox-parent-1-webkit-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/ResponsiveContainer.spec-vr.tsx-snapshots/should-respect-dimensions-of-a-grid-parent-1-chromium-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/ResponsiveContainer.spec-vr.tsx-snapshots/should-respect-dimensions-of-a-grid-parent-1-firefox-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/ResponsiveContainer.spec-vr.tsx-snapshots/should-respect-dimensions-of-a-grid-parent-1-webkit-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/ResponsiveContainer.spec-vr.tsx-snapshots/should-respect-minWidth-with-aspect-ratio-1-chromium-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/ResponsiveContainer.spec-vr.tsx-snapshots/should-respect-minWidth-with-aspect-ratio-1-firefox-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/ResponsiveContainer.spec-vr.tsx-snapshots/should-respect-minWidth-with-aspect-ratio-1-webkit-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/ResponsiveContainer.spec-vr.tsx-snapshots/should-respect-width-in-a-tall-container-when-aspect-is-set-1-chromium-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/ResponsiveContainer.spec-vr.tsx-snapshots/should-respect-width-in-a-tall-container-when-aspect-is-set-1-firefox-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/ResponsiveContainer.spec-vr.tsx-snapshots/should-respect-width-in-a-tall-container-when-aspect-is-set-1-webkit-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/www/AreaChartApiExamples.spec-vr.tsx-snapshots/AreaChartFillByValue-1-chromium-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/www/AreaChartApiExamples.spec-vr.tsx-snapshots/AreaChartFillByValue-1-firefox-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/www/AreaChartApiExamples.spec-vr.tsx-snapshots/AreaChartFillByValue-1-webkit-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/www/BarChartApiExamples.spec-vr.tsx-snapshots/BarChartStackedBySign-1-chromium-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/www/BarChartApiExamples.spec-vr.tsx-snapshots/BarChartStackedBySign-1-firefox-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/www/BarChartApiExamples.spec-vr.tsx-snapshots/BarChartStackedBySign-1-webkit-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/www/BarChartApiExamples.spec-vr.tsx-snapshots/BrushBarChart-1-chromium-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/www/BarChartApiExamples.spec-vr.tsx-snapshots/BrushBarChart-1-firefox-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/www/BarChartApiExamples.spec-vr.tsx-snapshots/BrushBarChart-1-webkit-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/www/BarChartApiExamples.spec-vr.tsx-snapshots/CustomShapeBarChart-1-chromium-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/www/BarChartApiExamples.spec-vr.tsx-snapshots/CustomShapeBarChart-1-firefox-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/www/BarChartApiExamples.spec-vr.tsx-snapshots/CustomShapeBarChart-1-webkit-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/www/BarChartApiExamples.spec-vr.tsx-snapshots/MixBarChart-1-chromium-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/www/BarChartApiExamples.spec-vr.tsx-snapshots/MixBarChart-1-firefox-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/www/BarChartApiExamples.spec-vr.tsx-snapshots/MixBarChart-1-webkit-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/www/BarChartApiExamples.spec-vr.tsx-snapshots/PositiveAndNegativeBarChart-1-chromium-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/www/BarChartApiExamples.spec-vr.tsx-snapshots/PositiveAndNegativeBarChart-1-firefox-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/www/BarChartApiExamples.spec-vr.tsx-snapshots/PositiveAndNegativeBarChart-1-webkit-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/www/ComposedChartApiExamples.spec-vr.tsx-snapshots/ScatterAndLineOfBestFit-1-chromium-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/www/ComposedChartApiExamples.spec-vr.tsx-snapshots/ScatterAndLineOfBestFit-1-firefox-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/www/LegendApiExamples.spec-vr.tsx-snapshots/LegendExample-1-chromium-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/www/LegendApiExamples.spec-vr.tsx-snapshots/LegendExample-1-firefox-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/www/LegendApiExamples.spec-vr.tsx-snapshots/LegendExample-1-webkit-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/www/LineChartApiExamples.spec-vr.tsx-snapshots/HighlightAndZoomLineChart-1-chromium-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/www/LineChartApiExamples.spec-vr.tsx-snapshots/HighlightAndZoomLineChart-1-firefox-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/www/LineChartApiExamples.spec-vr.tsx-snapshots/HighlightAndZoomLineChart-1-webkit-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/www/LineChartApiExamples.spec-vr.tsx-snapshots/LineChartWithReferenceLines-1-webkit-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/www/PieChartApiExamples.spec-vr.tsx-snapshots/PieChartWithNeedle-1-chromium-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/www/PieChartApiExamples.spec-vr.tsx-snapshots/PieChartWithNeedle-1-firefox-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/www/PieChartApiExamples.spec-vr.tsx-snapshots/PieChartWithNeedle-1-webkit-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/www/ReferenceLineApiExamples.spec-vr.tsx-snapshots/ReferenceLineExample-1-chromium-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/www/ReferenceLineApiExamples.spec-vr.tsx-snapshots/ReferenceLineExample-1-firefox-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/www/ReferenceLineApiExamples.spec-vr.tsx-snapshots/ReferenceLineExample-1-webkit-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/www/ReferenceLineApiExamples.spec-vr.tsx-snapshots/ReferenceLinePositionExample-1-chromium-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/www/ReferenceLineApiExamples.spec-vr.tsx-snapshots/ReferenceLinePositionExample-1-firefox-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/www/ReferenceLineApiExamples.spec-vr.tsx-snapshots/ReferenceLinePositionExample-1-webkit-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/www/ResponsiveContainerApiExamples.spec-vr.tsx-snapshots/ResponsiveContainerExample-1-chromium-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/www/ResponsiveContainerApiExamples.spec-vr.tsx-snapshots/ResponsiveContainerExample-1-firefox-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/www/ResponsiveContainerApiExamples.spec-vr.tsx-snapshots/ResponsiveContainerExample-1-webkit-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/www/ScatterChartApiExamples.spec-vr.tsx-snapshots/ScatterChartWithLabels-1-chromium-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/www/ScatterChartApiExamples.spec-vr.tsx-snapshots/ScatterChartWithLabels-1-firefox-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/www/ScatterChartApiExamples.spec-vr.tsx-snapshots/ScatterChartWithLabels-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 (60)
scripts/snapshots/es6Files.txt(2 hunks)scripts/snapshots/libFiles.txt(2 hunks)scripts/snapshots/typesFiles.txt(2 hunks)src/cartesian/Area.tsx(6 hunks)src/cartesian/Bar.tsx(6 hunks)src/cartesian/CartesianAxis.tsx(4 hunks)src/cartesian/CartesianGrid.tsx(4 hunks)src/cartesian/ErrorBar.tsx(3 hunks)src/cartesian/Line.tsx(7 hunks)src/cartesian/ReferenceArea.tsx(3 hunks)src/cartesian/ReferenceDot.tsx(3 hunks)src/cartesian/ReferenceLine.tsx(4 hunks)src/cartesian/Scatter.tsx(5 hunks)src/component/ActivePoints.tsx(6 hunks)src/component/Cursor.tsx(3 hunks)src/component/Dots.tsx(4 hunks)src/component/Label.tsx(4 hunks)src/component/LabelList.tsx(3 hunks)src/container/RootSurface.tsx(2 hunks)src/context/chartLayoutContext.tsx(1 hunks)src/polar/Pie.tsx(6 hunks)src/polar/PolarAngleAxis.tsx(2 hunks)src/polar/PolarGrid.tsx(3 hunks)src/polar/PolarRadiusAxis.tsx(2 hunks)src/polar/Radar.tsx(5 hunks)src/polar/RadialBar.tsx(5 hunks)src/polar/defaultPolarAngleAxisProps.tsx(2 hunks)src/polar/defaultPolarRadiusAxisProps.tsx(2 hunks)src/state/selectors/arrayEqualityCheck.ts(1 hunks)src/state/selectors/axisSelectors.ts(3 hunks)src/state/selectors/tooltipSelectors.ts(3 hunks)src/state/store.ts(2 hunks)src/state/zIndexSlice.ts(1 hunks)src/zindex/DefaultZIndexes.tsx(1 hunks)src/zindex/ZIndexLayer.tsx(1 hunks)src/zindex/ZIndexPortal.tsx(1 hunks)src/zindex/zIndexSelectors.ts(1 hunks)storybook/stories/API/chart/BarChart.stories.tsx(1 hunks)test-vr/tests/zIndex.spec-vr.tsx(1 hunks)test/cartesian/Area.spec.tsx(3 hunks)test/cartesian/Bar.spec.tsx(6 hunks)test/cartesian/CartesianGrid.spec.tsx(5 hunks)test/cartesian/ReferenceLine/ReferenceLine.spec.tsx(1 hunks)test/cartesian/YAxis/YAxis.label.spec.tsx(1 hunks)test/chart/BarChart.spec.tsx(1 hunks)test/chart/ComposedChart.spec.tsx(1 hunks)test/chart/LineChart.spec.tsx(3 hunks)test/chart/ScatterChart.spec.tsx(1 hunks)test/component/Cursor.spec.tsx(5 hunks)test/component/Label.spec.tsx(8 hunks)test/component/Legend.itemSorter.spec.tsx(5 hunks)test/component/Legend.spec.tsx(6 hunks)test/component/Tooltip/Tooltip.visibility.spec.tsx(3 hunks)test/helper/expectBars.ts(2 hunks)test/helper/logAllReduxActionsMiddleware.ts(1 hunks)test/polar/Radar.spec.tsx(3 hunks)test/state/selectors/selectCartesianItemsSettings.spec.tsx(1 hunks)test/zindex/AllZIndexPortals.spec.tsx(1 hunks)test/zindex/zIndexSelectors.spec.ts(1 hunks)www/src/docs/exampleComponents/ScatterChart/ScatterChartWithLabels.tsx(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (30)
- test/polar/Radar.spec.tsx
- www/src/docs/exampleComponents/ScatterChart/ScatterChartWithLabels.tsx
- test/cartesian/Area.spec.tsx
- src/state/selectors/axisSelectors.ts
- test/helper/expectBars.ts
- src/context/chartLayoutContext.tsx
- test/component/Cursor.spec.tsx
- src/zindex/ZIndexPortal.tsx
- test/component/Legend.spec.tsx
- src/component/Label.tsx
- test/helper/logAllReduxActionsMiddleware.ts
- src/cartesian/ReferenceDot.tsx
- test/chart/LineChart.spec.tsx
- test/component/Tooltip/Tooltip.visibility.spec.tsx
- src/polar/PolarAngleAxis.tsx
- test-vr/tests/zIndex.spec-vr.tsx
- src/state/zIndexSlice.ts
- test/chart/ComposedChart.spec.tsx
- scripts/snapshots/es6Files.txt
- src/polar/defaultPolarAngleAxisProps.tsx
- src/polar/defaultPolarRadiusAxisProps.tsx
- test/component/Legend.itemSorter.spec.tsx
- test/cartesian/ReferenceLine/ReferenceLine.spec.tsx
- scripts/snapshots/typesFiles.txt
- test/chart/BarChart.spec.tsx
- src/zindex/DefaultZIndexes.tsx
- src/polar/Radar.tsx
- test/state/selectors/selectCartesianItemsSettings.spec.tsx
- test/zindex/zIndexSelectors.spec.ts
- scripts/snapshots/libFiles.txt
🧰 Additional context used
📓 Path-based instructions (7)
{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/chart/ScatterChart.spec.tsxtest/cartesian/YAxis/YAxis.label.spec.tsxtest/cartesian/CartesianGrid.spec.tsxtest/zindex/AllZIndexPortals.spec.tsxtest/cartesian/Bar.spec.tsxtest/component/Label.spec.tsx
test/**/*.spec.{ts,tsx}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
test/**/*.spec.{ts,tsx}: When writing new code, aim for 100% unit test code coverage
When adding or changing functionality, add appropriate tests (prefer unit tests; use RTL if rendering is involved)
Files:
test/chart/ScatterChart.spec.tsxtest/cartesian/YAxis/YAxis.label.spec.tsxtest/cartesian/CartesianGrid.spec.tsxtest/zindex/AllZIndexPortals.spec.tsxtest/cartesian/Bar.spec.tsxtest/component/Label.spec.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
**/*.{ts,tsx}: Ensure code lints cleanly before submitting PRs (npm run lint)
Never use the TypeScript any type (implicit or explicit)
Prefer unknown over any and refine types appropriately
Explicitly type all function parameters and return values; do not rely on implicit any or inference
Do not use as type assertions; the only exception is as const
Files:
test/chart/ScatterChart.spec.tsxsrc/cartesian/Scatter.tsxsrc/container/RootSurface.tsxsrc/cartesian/CartesianAxis.tsxsrc/cartesian/ErrorBar.tsxsrc/cartesian/Line.tsxsrc/state/store.tssrc/component/LabelList.tsxsrc/component/Cursor.tsxsrc/polar/PolarGrid.tsxsrc/state/selectors/tooltipSelectors.tssrc/component/ActivePoints.tsxsrc/cartesian/ReferenceArea.tsxtest/cartesian/YAxis/YAxis.label.spec.tsxtest/cartesian/CartesianGrid.spec.tsxsrc/cartesian/Area.tsxsrc/cartesian/ReferenceLine.tsxsrc/zindex/ZIndexLayer.tsxstorybook/stories/API/chart/BarChart.stories.tsxsrc/cartesian/CartesianGrid.tsxsrc/polar/PolarRadiusAxis.tsxtest/zindex/AllZIndexPortals.spec.tsxsrc/polar/Pie.tsxsrc/zindex/zIndexSelectors.tstest/cartesian/Bar.spec.tsxsrc/state/selectors/arrayEqualityCheck.tssrc/cartesian/Bar.tsxtest/component/Label.spec.tsxsrc/component/Dots.tsxsrc/polar/RadialBar.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/container/RootSurface.tsxsrc/cartesian/CartesianAxis.tsxsrc/cartesian/ErrorBar.tsxsrc/cartesian/Line.tsxsrc/state/store.tssrc/component/LabelList.tsxsrc/component/Cursor.tsxsrc/polar/PolarGrid.tsxsrc/state/selectors/tooltipSelectors.tssrc/component/ActivePoints.tsxsrc/cartesian/ReferenceArea.tsxsrc/cartesian/Area.tsxsrc/cartesian/ReferenceLine.tsxsrc/zindex/ZIndexLayer.tsxsrc/cartesian/CartesianGrid.tsxsrc/polar/PolarRadiusAxis.tsxsrc/polar/Pie.tsxsrc/zindex/zIndexSelectors.tssrc/state/selectors/arrayEqualityCheck.tssrc/cartesian/Bar.tsxsrc/component/Dots.tsxsrc/polar/RadialBar.tsx
storybook/**
📄 CodeRabbit inference engine (DEVELOPING.md)
Use Storybook for high-fidelity examples intended for publication; prefer unit or VR tests for low-fidelity cases due to Chromatic limits
Files:
storybook/stories/API/chart/BarChart.stories.tsx
storybook/stories/**/*.stories.tsx
📄 CodeRabbit inference engine (CONTRIBUTING.md)
storybook/stories/**/*.stories.tsx: Use Storybook stories as smoke tests and add play-function tests with assertions when appropriate
If APIs change, ensure all Storybook stories still work as expected
Files:
storybook/stories/API/chart/BarChart.stories.tsx
test/component/**/*.spec.tsx
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Test rendering behavior and component interactions with React Testing Library
Files:
test/component/Label.spec.tsx
🧠 Learnings (3)
📚 Learning: 2025-10-25T07:35:14.559Z
Learnt from: CR
PR: recharts/recharts#0
File: CONTRIBUTING.md:0-0
Timestamp: 2025-10-25T07:35:14.559Z
Learning: Applies to test/component/**/*.spec.tsx : Test rendering behavior and component interactions with React Testing Library
Applied to files:
test/zindex/AllZIndexPortals.spec.tsx
📚 Learning: 2025-10-25T07:35:46.180Z
Learnt from: CR
PR: recharts/recharts#0
File: test/README.md:0-0
Timestamp: 2025-10-25T07:35:46.180Z
Learning: Applies to test/**/*.test.{ts,tsx} : Prefer using the createSelectorTestCase helper when writing or modifying tests
Applied to files:
test/cartesian/Bar.spec.tsx
📚 Learning: 2025-10-25T07:35:46.180Z
Learnt from: CR
PR: recharts/recharts#0
File: test/README.md:0-0
Timestamp: 2025-10-25T07:35:46.180Z
Learning: Applies to test/**/*.test.{ts,tsx} : When testing selectors, verify render counts using the spy and rerenderSameComponent from createSelectorTestCase
Applied to files:
test/cartesian/Bar.spec.tsx
🧬 Code graph analysis (27)
src/cartesian/Scatter.tsx (2)
src/zindex/ZIndexLayer.tsx (2)
ZIndexable(11-18)ZIndexLayer(44-103)src/zindex/DefaultZIndexes.tsx (1)
DefaultZIndexes(4-60)
src/container/RootSurface.tsx (1)
src/zindex/ZIndexPortal.tsx (1)
AllZIndexPortals(21-43)
src/cartesian/CartesianAxis.tsx (3)
src/zindex/ZIndexLayer.tsx (2)
ZIndexable(11-18)ZIndexLayer(44-103)src/zindex/DefaultZIndexes.tsx (1)
DefaultZIndexes(4-60)src/container/Layer.tsx (1)
Layer(13-22)
src/cartesian/ErrorBar.tsx (3)
src/zindex/ZIndexLayer.tsx (2)
ZIndexable(11-18)ZIndexLayer(44-103)src/zindex/DefaultZIndexes.tsx (1)
DefaultZIndexes(4-60)src/util/resolveDefaultProps.tsx (1)
resolveDefaultProps(16-53)
src/cartesian/Line.tsx (2)
src/zindex/ZIndexLayer.tsx (2)
ZIndexable(11-18)ZIndexLayer(44-103)src/zindex/DefaultZIndexes.tsx (1)
DefaultZIndexes(4-60)
src/state/store.ts (1)
src/state/zIndexSlice.ts (1)
zIndexReducer(128-128)
src/component/LabelList.tsx (7)
src/zindex/ZIndexLayer.tsx (2)
ZIndexable(11-18)ZIndexLayer(44-103)src/zindex/DefaultZIndexes.tsx (1)
DefaultZIndexes(4-60)src/container/Layer.tsx (1)
Layer(13-22)src/util/DataUtils.ts (1)
isNullish(179-181)src/util/ChartUtils.ts (1)
getValueByDataKey(41-55)src/component/Label.tsx (1)
Label(542-627)src/util/svgPropertiesAndEvents.ts (1)
svgPropertiesAndEvents(19-24)
src/component/Cursor.tsx (2)
src/zindex/ZIndexLayer.tsx (2)
ZIndexable(11-18)ZIndexLayer(44-103)src/zindex/DefaultZIndexes.tsx (1)
DefaultZIndexes(4-60)
src/polar/PolarGrid.tsx (2)
src/zindex/ZIndexLayer.tsx (2)
ZIndexable(11-18)ZIndexLayer(44-103)src/zindex/DefaultZIndexes.tsx (1)
DefaultZIndexes(4-60)
src/state/selectors/tooltipSelectors.ts (1)
src/state/selectors/arrayEqualityCheck.ts (1)
emptyArraysAreEqualCheck(7-13)
src/component/ActivePoints.tsx (3)
src/util/types.ts (1)
adaptEventHandlers(838-865)src/zindex/ZIndexLayer.tsx (2)
ZIndexable(11-18)ZIndexLayer(44-103)src/zindex/DefaultZIndexes.tsx (1)
DefaultZIndexes(4-60)
src/cartesian/ReferenceArea.tsx (3)
src/zindex/ZIndexLayer.tsx (2)
ZIndexable(11-18)ZIndexLayer(44-103)src/container/Layer.tsx (1)
Layer(13-22)src/zindex/DefaultZIndexes.tsx (1)
DefaultZIndexes(4-60)
test/cartesian/CartesianGrid.spec.tsx (1)
src/zindex/DefaultZIndexes.tsx (1)
DefaultZIndexes(4-60)
src/cartesian/Area.tsx (2)
src/zindex/ZIndexLayer.tsx (2)
ZIndexable(11-18)ZIndexLayer(44-103)src/zindex/DefaultZIndexes.tsx (1)
DefaultZIndexes(4-60)
src/cartesian/ReferenceLine.tsx (4)
src/zindex/ZIndexLayer.tsx (2)
ZIndexable(11-18)ZIndexLayer(44-103)src/container/Layer.tsx (1)
Layer(13-22)src/component/Label.tsx (2)
CartesianLabelContextProvider(101-124)CartesianLabelFromLabelProp(669-679)src/zindex/DefaultZIndexes.tsx (1)
DefaultZIndexes(4-60)
src/zindex/ZIndexLayer.tsx (4)
src/context/chartLayoutContext.tsx (1)
useIsInChartContext(147-154)src/context/PanoramaContext.tsx (1)
useIsPanorama(6-6)src/state/hooks.ts (2)
useAppDispatch(9-15)useAppSelector(40-50)src/zindex/zIndexSelectors.ts (1)
selectZIndexPortalId(13-34)
storybook/stories/API/chart/BarChart.stories.tsx (1)
src/cartesian/Bar.tsx (1)
Bar(884-884)
src/cartesian/CartesianGrid.tsx (2)
src/zindex/ZIndexLayer.tsx (2)
ZIndexable(11-18)ZIndexLayer(44-103)src/zindex/DefaultZIndexes.tsx (1)
DefaultZIndexes(4-60)
src/polar/PolarRadiusAxis.tsx (2)
src/zindex/ZIndexLayer.tsx (2)
ZIndexable(11-18)ZIndexLayer(44-103)src/container/Layer.tsx (1)
Layer(13-22)
test/zindex/AllZIndexPortals.spec.tsx (5)
src/state/RechartsReduxContext.tsx (2)
RechartsReduxContextValue(14-19)RechartsReduxContext(27-27)src/state/store.ts (2)
RechartsRootState(94-94)createRechartsStore(40-92)src/zindex/ZIndexPortal.tsx (1)
AllZIndexPortals(21-43)src/zindex/zIndexSelectors.ts (1)
selectAllRegisteredZIndexes(36-50)src/zindex/DefaultZIndexes.tsx (1)
DefaultZIndexes(4-60)
src/polar/Pie.tsx (5)
src/zindex/ZIndexLayer.tsx (2)
ZIndexable(11-18)ZIndexLayer(44-103)src/zindex/DefaultZIndexes.tsx (1)
DefaultZIndexes(4-60)src/container/Layer.tsx (1)
Layer(13-22)src/util/ChartUtils.ts (1)
getValueByDataKey(41-55)src/state/SetTooltipEntrySettings.tsx (1)
SetTooltipEntrySettings(11-26)
src/zindex/zIndexSelectors.ts (3)
src/state/store.ts (1)
RechartsRootState(94-94)src/zindex/DefaultZIndexes.tsx (1)
DefaultZIndexes(4-60)src/state/selectors/arrayEqualityCheck.ts (1)
arrayContentsAreEqualCheck(20-30)
test/cartesian/Bar.spec.tsx (3)
test/helper/expectBars.ts (1)
expectActiveBars(39-51)test/component/Tooltip/tooltipTestHelpers.tsx (1)
showTooltipOnCoordinate(71-78)test/component/Tooltip/tooltipMouseHoverSelectors.ts (1)
barChartMouseHoverTooltipSelector(18-18)
src/cartesian/Bar.tsx (2)
src/zindex/ZIndexLayer.tsx (2)
ZIndexable(11-18)ZIndexLayer(44-103)src/zindex/DefaultZIndexes.tsx (1)
DefaultZIndexes(4-60)
test/component/Label.spec.tsx (1)
src/zindex/DefaultZIndexes.tsx (1)
DefaultZIndexes(4-60)
src/component/Dots.tsx (2)
src/zindex/ZIndexLayer.tsx (2)
ZIndexable(11-18)ZIndexLayer(44-103)src/zindex/DefaultZIndexes.tsx (1)
DefaultZIndexes(4-60)
src/polar/RadialBar.tsx (3)
src/zindex/ZIndexLayer.tsx (2)
ZIndexLayer(44-103)ZIndexable(11-18)src/zindex/DefaultZIndexes.tsx (1)
DefaultZIndexes(4-60)src/container/Layer.tsx (1)
Layer(13-22)
⏰ 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 (60)
test/cartesian/YAxis/YAxis.label.spec.tsx (1)
18-18: LGTM - selector updated for z-index portal system.The broader selector correctly accommodates labels that may be rendered in z-index portals outside the
.recharts-yAxiscontainer.src/state/store.ts (1)
21-21: LGTM - z-index slice integrated correctly.The zIndex reducer is added following the same pattern as other state slices, and the type system will automatically reflect this in RechartsRootState.
Also applies to: 37-37
src/container/RootSurface.tsx (1)
10-10: LGTM - z-index portal system integrated into rendering pipeline.Both panorama and non-panorama rendering paths correctly wrap children with AllZIndexPortals, ensuring consistent z-index behavior across all chart modes.
Also applies to: 99-103, 107-107
test/cartesian/CartesianGrid.spec.tsx (1)
33-34: LGTM - test expectations updated for z-index.Tests correctly verify that CartesianGrid passes
zIndex: DefaultZIndexes.gridto its grid line props, aligning with the new z-index system.Also applies to: 1333-1333, 1429-1429, 1521-1521, 1609-1609
test/chart/ScatterChart.spec.tsx (1)
672-717: LGTM - test coordinates updated for z-index rendering.Expected scatter point coordinates have been adjusted to reflect the rendering changes introduced by the z-index portal system. The pattern is consistent across all affected test cases.
src/state/selectors/tooltipSelectors.ts (1)
86-86: LGTM - updated to use refactored equality check.The switch from
arrayEqualityChecktoemptyArraysAreEqualCheckaligns with the refactored equality check API while preserving the existing empty-array equality semantics for memoization.Also applies to: 109-109, 126-126
src/polar/PolarRadiusAxis.tsx (1)
27-27: LGTM - z-index support added following established pattern.The PolarRadiusAxis correctly extends its props with
ZIndexableand wraps its output inZIndexLayer, following the same pattern used throughout the codebase for z-index support.Also applies to: 31-31, 206-215
src/cartesian/Line.tsx (2)
627-627: Sensible default.
zIndex: DefaultZIndexes.linealigns with the documented stacking order.
558-606: Verification confirmed: ActivePoints independently portals to DefaultZIndexes.activeDot.ActivePoints extends ZIndexable and internally uses ZIndexLayer with zIndex defaulting to DefaultZIndexes.activeDot, ensuring hover dots are not constrained to the line's zIndex layer. Code is correct as written.
src/cartesian/CartesianAxis.tsx (2)
474-522: Axis participates in layering correctly.Wrapping the entire axis tree at
DefaultZIndexes.axisis correct; tick measurement viagetComputedStylestill works post‑portal.If you’ve seen SSR usage, sanity‑check that portal code guards against
documentabsence.
111-112: Default axis z-index matches spec.
DefaultZIndexes.axis>lineensures ticks/labels render above lines.src/component/Cursor.tsx (1)
124-133: Cursor layering is correct with sensible default.Portal at
DefaultZIndexes.cursorkeeps the cursor beneath markers but above areas/lines per defaults.src/cartesian/CartesianGrid.tsx (2)
495-528: Z-index portal wrap looks good.The content is cleanly wrapped in ZIndexLayer; portal gating matches the intended behavior (0 → inline, others → portal).
397-398: Grid zIndex ordering is correct as implemented.Verification confirms:
DefaultZIndexes.grid=-100(src/zindex/DefaultZIndexes.tsx:8)selectAllRegisteredZIndexessorts ascending:uniqueNumbers.sort((a, b) => a - b)produces[-100, 100, 200, ...]AllZIndexPortalsrenders negative indexes first, then children (0 layer), then positive indexes- Result: grid (-100) portals behind main content (0) and above any more negative layers
- Behavior is consistent across both panorama and normal modes (isPanorama parameter handled properly)
- Test suite confirms expected ordering:
[-100, -3, 1, 2, 100, 200, ...]src/polar/PolarGrid.tsx (1)
195-214: Polar grid z-index layering is consistent.ZIndexLayer usage and defaulting align with CartesianGrid; good consistency.
src/zindex/zIndexSelectors.ts (1)
13-34: Selector signature and null-handling look solid.Accepts number | undefined and cleanly picks panorama vs. standard portal IDs.
src/component/Dots.tsx (1)
128-134: No issues found; URL(#id) references are correctly co-located with defs within portals.Verification confirms that all components using
Dots(Area, Line, Scatter) create their<defs>elements as siblings inside the sameZIndexLayerportal container whereclipPathattributes are referenced. The URL(#id) references will resolve correctly because the clip-path definitions and their references are colocated in the same DOM context, not split across portals. The code is safe for all target browsers.test/cartesian/Bar.spec.tsx (3)
18-18: LGTM!The import of
expectActiveBarsaligns with the test helper refactor to support the new z-index feature and provides a cleaner API for testing bar active states.
1032-1032: LGTM!The addition of
zIndex: 0to the label props reflects the new z-index feature integration. This value correctly indicates that individual Bar labels inherit zIndex from their parent layer rather than having their own z-index portal.
1453-1453: LGTM!The migration to
expectActiveBarsprovides a cleaner, more declarative testing API. The helper accepts an array of expected active bar payloads, making assertions more concise and maintainable compared to individual bar checks.Also applies to: 1483-1504, 1546-1546, 1730-1742
test/component/Label.spec.tsx (2)
8-8: LGTM!The
DefaultZIndexesimport is necessary for verifying the zIndex value passed to Label content functions in test assertions.
306-306: LGTM!The
zIndex: DefaultZIndexes.labeladditions correctly verify that Label content functions now receive zIndex as part of their props payload. This is consistent across all Label rendering contexts (cartesian and polar) and aligns with the z-index feature implementation.Also applies to: 467-467, 501-501, 538-538, 610-610, 639-639, 671-671
src/cartesian/ReferenceArea.tsx (3)
21-24: LGTM!The z-index infrastructure imports and the
ZIndexableextension toReferenceAreaPropscorrectly expose thezIndexprop as part of the public API. This follows the established pattern across other chart components.
130-140: LGTM!The
ZIndexLayerwrapper is correctly applied to enable z-index layering for the ReferenceArea component. The inner rendering structure (Layer, shape, labels) is preserved within the wrapper, maintaining backward compatibility while adding z-index capability.
153-153: LGTM!The default
zIndex: DefaultZIndexes.areais appropriate for ReferenceArea, which is a filled area element. This places it above the grid but below bars, lines, and interactive elements in the default stacking order.src/polar/Pie.tsx (4)
59-60: LGTM!The z-index infrastructure imports and the
ZIndexableextensions to bothInternalPiePropsandPiePropscorrectly expose thezIndexprop as part of the public API for the Pie component.Also applies to: 150-150, 181-181
403-412: LGTM!Wrapping each pie label in a
ZIndexLayerwithDefaultZIndexes.labelensures labels render above pie sectors (label zIndex 700 > area zIndex 100). This provides the correct visual stacking for pie chart labels.
740-740: LGTM!The default
zIndex: DefaultZIndexes.areais appropriate for Pie, which renders filled sectors. This places pie sectors in the same z-index layer as other filled area elements.
761-766: LGTM!The
ZIndexLayerwrapper around PieImpl correctly enables z-index layering for the Pie component. This addresses the previous review feedback and ensures Pie participates in the z-index system alongside other chart elements.src/cartesian/Scatter.tsx (3)
66-67: LGTM!The z-index infrastructure imports and the
ZIndexableextensions to bothScatterInternalPropsandScatterPropscorrectly expose thezIndexprop as part of the public API.Also applies to: 124-124, 159-159
608-627: LGTM!The
ZIndexLayerwrapper is correctly applied to the Scatter component's rendering, enabling z-index layering while preserving the existing structure (clipping, error bars, symbols) within the wrapper.
646-646: LGTM!The default
zIndex: DefaultZIndexes.scatter(600) is appropriate for Scatter points, placing them above bars, lines, and areas in the default stacking order. This ensures scatter points remain visible and interactive.src/component/LabelList.tsx (3)
12-13: LGTM!The z-index infrastructure imports and the
ZIndexableextension toLabelListPropscorrectly expose thezIndexprop as part of the public API for LabelList.Also applies to: 52-52
106-106: LGTM!The
ZIndexLayerwrapper with the fallbackzIndex ?? DefaultZIndexes.labelensures LabelList has a sensible default z-index (700) while allowing users to override it. This correctly implements the z-index feature for label lists.Also applies to: 115-152
147-147: LGTM!Setting
zIndex={0}on individual Label components is correct. This prevents each label from creating its own portal and instead allows all labels to inherit the z-index from their parent LabelList'sZIndexLayer. This design is more performant and maintains the intended layering behavior.src/zindex/ZIndexLayer.tsx (3)
11-18: LGTM!The
ZIndexableinterface provides a clean, well-documented public API for z-index support. The optional nature of the prop allows backward compatibility while enabling z-index layering when needed.
20-31: LGTM!The
ZIndexLayerPropstype is well-defined with clear documentation. Allowing undefined for bothzIndexandchildrenprovides flexibility for different rendering scenarios.
44-102: LGTM!The
ZIndexLayerimplementation correctly handles portal-based rendering with appropriate guards:
- Checks for chart context before attempting portal rendering
- Bypasses portals for zIndex 0 or undefined (default layer)
- Guards against missing portalId and portal elements, returning null to defer rendering
- Registers/unregisters zIndex with the Redux store for portal management
The logic is sound and addresses previous review concerns about null handling.
src/component/ActivePoints.tsx (3)
11-12: LGTM!The z-index infrastructure imports are necessary for implementing z-index support in the ActivePoints component.
75-85: LGTM!The
ZIndexableextension toActivePointsPropscorrectly exposes thezIndexprop as part of the public API for active points.
87-116: LGTM!The
ActivePointscomponent correctly implements z-index support withDefaultZIndexes.activeDot(1100), which is the highest z-index value. This ensures active dots render on top of all other chart elements during hover interactions, as expected.src/cartesian/Area.tsx (5)
59-61: Z-index plumbing imported correctly.Imports align with the new layering system.
67-67: Expose zIndex on internal propsExtending InternalAreaProps with ZIndexable is correct and keeps typing explicit.
114-114: Public API: zIndex on AreaPropsGood addition; matches the cross-component pattern.
678-679: Sane default layerDefault zIndex for Area set to DefaultZIndexes.area matches the spec.
611-612: ****ActivePoints already wraps its content with
<ZIndexLayer zIndex={zIndex}>and defaults zIndex toDefaultZIndexes.activeDot. The proposed wrapping in Area.tsx is redundant and unnecessary—the component handles z-index layering correctly without external ZIndexLayer wrappers.Likely an incorrect or invalid review comment.
src/cartesian/Bar.tsx (6)
83-85: Z-index utilities importedMatches the pattern used across components.
107-136: Public API: zIndex on BarPropsGood addition; preserves typing while extending behavior.
377-390: Active bar highlight correctly elevatedWrapping the active shape with ZIndexLayer at DefaultZIndexes.activeBar achieves the desired overlay behavior.
Please add/ensure a test that event handlers still fire when the active rectangle is portalized (React synthetic events should bubble across portals).
662-663: Default zIndex for BarAligned with DefaultZIndexes.bar.
875-878: Portalize Bar at component levelGood: the whole Bar subtree participates in the z-index system.
884-885: Export type is explicitExplicit ComponentType is fine and keeps the public type precise.
src/cartesian/ReferenceLine.tsx (3)
22-24: Z-index layer integration importedAll good.
264-273: ReferenceLine now respects zIndexPortal wrapper is correct; preserves clipPath behavior via id reference.
285-286: Default zIndex for lineMatches DefaultZIndexes.line and desired layering over areas/bars.
src/polar/RadialBar.tsx (5)
62-64: Z-index utilities importedConsistent with the system.
163-171: Active sector elevationGood: active sectors render above other layers using DefaultZIndexes.activeBar.
277-309: Expose zIndex on internal propsCorrectly extends InternalRadialBarProps with ZIndexable.
385-393: Portalize RadialBar at component levelWrapping with ZIndexLayer is correct and mirrors Bar/Area behavior.
437-438: Default zIndex for RadialBarAligned with DefaultZIndexes.bar.
| act(() => { | ||
| store.dispatch(registerZIndexPortal({ zIndex: newZIndexDefinitelyNotOneOfTheDefaults })); | ||
| vi.runOnlyPendingTimers(); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Fix timers: set up fake timers or remove the call.
vi.runOnlyPendingTimers() is used without vi.useFakeTimers(), which can fail and won’t flush RTK’s raf batching. Either remove the call, or enable/restore timers around the test.
Apply one of:
Option A — enable/restore timers:
-import { beforeEach, describe, expect, it } from 'vitest';
+import { beforeEach, afterEach, describe, expect, it, vi } from 'vitest';
@@
beforeEach(() => {
+ vi.useFakeTimers();
store = createRechartsStore();
});
+ afterEach(() => {
+ vi.useRealTimers();
+ });
@@
act(() => {
store.dispatch(registerZIndexPortal({ zIndex: newZIndexDefinitelyNotOneOfTheDefaults }));
vi.runOnlyPendingTimers();
});Option B — drop timers (registration is synchronous for this path):
- act(() => {
- store.dispatch(registerZIndexPortal({ zIndex: newZIndexDefinitelyNotOneOfTheDefaults }));
- vi.runOnlyPendingTimers();
- });
+ act(() => {
+ store.dispatch(registerZIndexPortal({ zIndex: newZIndexDefinitelyNotOneOfTheDefaults }));
+ });📝 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.
| act(() => { | |
| store.dispatch(registerZIndexPortal({ zIndex: newZIndexDefinitelyNotOneOfTheDefaults })); | |
| vi.runOnlyPendingTimers(); | |
| }); | |
| act(() => { | |
| store.dispatch(registerZIndexPortal({ zIndex: newZIndexDefinitelyNotOneOfTheDefaults })); | |
| }); |
🤖 Prompt for AI Agents
In test/zindex/AllZIndexPortals.spec.tsx around lines 41 to 44, the test calls
vi.runOnlyPendingTimers() without enabling fake timers which can fail and won’t
flush RTK’s raf batching; fix by either (A) wrapping the dispatch with fake
timers (call vi.useFakeTimers() before the dispatch and
vi.runOnlyPendingTimers() then restore timers with vi.useRealTimers() or
vi.restoreAllMocks()/vi.restoreAllTimers() after), or (B) remove the
vi.runOnlyPendingTimers() call entirely since this registration path is
synchronous and the dispatch inside act() is sufficient.
ckifer
left a comment
There was a problem hiding this comment.
Much improved in a lot of aspects I believe 🚀 nice work
| fillOpacity: 0.5, | ||
| stroke: 'none', | ||
| strokeWidth: 1, | ||
| zIndex: DefaultZIndexes.area, |
There was a problem hiding this comment.
should reference areas be over other areas by default? Or all reference elements at the same level?
I guess the user can control it so it doesn't really matter
There was a problem hiding this comment.
I can imagine some people will want different defaults so I decided to keep it simple.
| // @ts-expect-error svgPropertiesNoEventsFromUnknown(activeDot) is contributing unknown props | ||
| const dotProps: ActiveDotProps = { | ||
| ...dotPropsTyped, | ||
| ...svgPropertiesNoEventsFromUnknown(activeDot), | ||
| ...adaptEventHandlers(activeDot), | ||
| }; |
There was a problem hiding this comment.
This can return null so agreed, not a safe spread
There was a problem hiding this comment.
Hm I think we don't worry about it for now. I thought these were labels but they're custom svg? Shouldn't those still render either before or after our stuff?
There was a problem hiding this comment.
Or no, the custom code is rendered within the chart wrapper still
Implements z-index using React portals + elements, variant C of #6478
Fixes #6449
Fixes #4721
Helps with half of #6340
Fixes #1803
Fixes #3574
Fixes #5592
Works fine! The API mimics CSS z-index:
I am not planning on exposing any other elements as public API. The only interaction will be through thefrom the website examples I can see that we do need to expose the wrapper that allows rendering custom components in a given zIndex so I will export and document that.zIndexprop on various things.The Bar ZIndexable doesn't add any extra re-renders - the chart is already waiting for the
RegisterGraphicalItemIdregistration.The Label ZIndexable does add re-renders - the chart takes one more render cycle to settle. We should be able to optimize that with a bit of effort. Like registering the zindex ahead of time.I have added all default zindexes to the default state so now they render immediately. Custom zindexes will add one more render cycle.Next steps
Do we want to go ahead with this approach? Or anything else? #6478
What if left before merging this PR, if we choose to go ahead with this approach
Summary by CodeRabbit