Split scale definition into two parts#6783
Conversation
WalkthroughThis PR refactors Recharts' scale abstraction system by introducing new Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Areas requiring extra attention:
Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
@ckifer this is an important one to review - the external interface will be locked for major versions
There was a problem hiding this comment.
does this break if someone uses a method from a d3 scale prior to passing it in and then pass it in?
It shouldn't
Something that I've done in the past is used https://d3js.org/d3-scale/linear#linear_invert
In this case I would define my scale with a given domain/range and then because recharts is using that same scale use it to convert X and Y values to my scale. This should still work, cool
There was a problem hiding this comment.
and its always easier to add more later
There was a problem hiding this comment.
what happens if D3 breaks us here?
There was a problem hiding this comment.
does this break if someone uses a method from a d3 scale prior to passing it in and then pass it in?
They can do whatever with the scale, this interface is the list of methods we require inside Recharts. It doesn't even need to be imported from d3, as long as it implements this interface.
and its always easier to add more later
That's the thing, adding more methods here will be a breaking change. If someone does have a custom implementation (so, skips d3) then having a new required method will break their code.
This is in contrast to the internal interface where we can add new stuff anytime. We can return it from a hook and make it external that way - and because there it's in a consumer position, adding new methods in that context will not break anyone.
what happens if D3 breaks us here?
Then that triggers a major version for Recharts ¯\_(ツ)_/¯
There was a problem hiding this comment.
And yes the invert method will be one of the things we export from a hook. I would prefer to keep it simple however, so perhaps we will export only a single method (invert) instead of the whole scale.
There was a problem hiding this comment.
@ckifer this will be internal only, and we will be free to change it. First step will be to add the apply method from ScaleHelper
|
|
||
| describe('getTicksForAxis', () => { | ||
| const Y_AXIS_EXAMPLE: AxisPropsNeededForTicksGenerator = { | ||
| // @ts-expect-error we need to wrap the d3 scales in unified interface |
There was a problem hiding this comment.
We did wrap d3 scale in unified interface now!
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/polar/RadialBar.tsx (1)
640-654: Potential issue:endAnglemay be undefined at return.The
@ts-expect-errorcomment indicates thatendAnglemight not be assigned in all code paths. Specifically, if both the radial and non-radial layout guards fail (e.g.,innerRadiusis null),endAngleremains unassigned.Consider providing a fallback value for
endAnglein the return object:startAngle, - // @ts-expect-error endAngle is used before assigned (?) - endAngle, + endAngle: endAngle ?? rootEndAngle, ...(cells && cells[index] && cells[index].props),Alternatively, initialize
endAnglewith a default value at declaration (line 574):- endAngle: number, + endAngle: number = rootEndAngle,
🧹 Nitpick comments (4)
src/state/selectors/tooltipSelectors.ts (1)
390-397: Helpful documentation noting related implementations.This comment documenting the four tick generation implementations is valuable for maintainability. Consider opening an issue to consolidate these if they share significant logic.
src/cartesian/Area.tsx (1)
81-84: Avoid usinganytype for payload.The
payloadproperty usesany, which violates the coding guidelines. Consider using a generic type parameter orunknownwith proper type guards.-type BaseValueCoordinate = NullableCoordinate & { payload: any }; +type BaseValueCoordinate = NullableCoordinate & { payload: unknown };As per coding guidelines,
anyshould be avoided in TypeScript code.src/util/scale/RechartsScale.ts (1)
54-67: Type verification could be improved ingetD3ScaleFromType.The two
@ts-expect-errorcomments indicate weak type verification. Consider adding runtime validation or a type map for better safety.A type-safe approach could use a mapping object:
const scaleFactories: Partial<Record<D3ScaleType, () => CustomScaleDefinition>> = { linear: d3Scales.scaleLinear, band: d3Scales.scaleBand, point: d3Scales.scalePoint, // ... etc };This would eliminate the
@ts-expect-errorcomments and provide compile-time safety.src/state/selectors/axisSelectors.ts (1)
1089-1091: Consider using a more robust scale name validation.The current check
name in d3Scaleswill returntruefor any property on thed3Scalesobject, including inherited properties. While this works for the expected d3 scale names likescaleLinear, it could potentially match unintended properties.Consider using
Object.hasOwnfor stricter property checking:function isSupportedScaleName(name: string): name is ScaleType { - return name in d3Scales; + return Object.hasOwn(d3Scales, name); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (13)
package-lock.jsonis excluded by!**/package-lock.jsontest-vr/__snapshots__/tests/ErrorBar.Scatter.spec-vr.tsx-snapshots/ComposedChart-vertical-both-directions-item-data-1-chromium-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/ErrorBar.Scatter.spec-vr.tsx-snapshots/ComposedChart-vertical-both-directions-item-data-1-firefox-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/ErrorBar.Scatter.spec-vr.tsx-snapshots/ComposedChart-vertical-both-directions-item-data-1-webkit-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/ErrorBar.Scatter.spec-vr.tsx-snapshots/ComposedChart-vertical-both-directions-root-data-1-chromium-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/ErrorBar.Scatter.spec-vr.tsx-snapshots/ComposedChart-vertical-both-directions-root-data-1-firefox-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/ErrorBar.Scatter.spec-vr.tsx-snapshots/ComposedChart-vertical-both-directions-root-data-1-webkit-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/ErrorBar.Scatter.spec-vr.tsx-snapshots/ScatterChart-vertical-both-directions-item-data-1-chromium-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/ErrorBar.Scatter.spec-vr.tsx-snapshots/ScatterChart-vertical-both-directions-item-data-1-firefox-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/ErrorBar.Scatter.spec-vr.tsx-snapshots/ScatterChart-vertical-both-directions-item-data-1-webkit-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/ErrorBar.Scatter.spec-vr.tsx-snapshots/ScatterChart-vertical-both-directions-root-data-1-chromium-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/ErrorBar.Scatter.spec-vr.tsx-snapshots/ScatterChart-vertical-both-directions-root-data-1-firefox-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/ErrorBar.Scatter.spec-vr.tsx-snapshots/ScatterChart-vertical-both-directions-root-data-1-webkit-linux.pngis excluded by!**/*.png
📒 Files selected for processing (37)
scripts/snapshots/es6Files.txt(1 hunks)scripts/snapshots/libFiles.txt(1 hunks)scripts/snapshots/typesFiles.txt(1 hunks)src/cartesian/Area.tsx(3 hunks)src/cartesian/Bar.tsx(3 hunks)src/cartesian/ErrorBar.tsx(2 hunks)src/cartesian/Line.tsx(1 hunks)src/cartesian/ReferenceArea.tsx(1 hunks)src/cartesian/Scatter.tsx(1 hunks)src/cartesian/ZAxis.tsx(2 hunks)src/index.ts(1 hunks)src/polar/PolarAngleAxis.tsx(2 hunks)src/polar/PolarRadiusAxis.tsx(1 hunks)src/polar/Radar.tsx(4 hunks)src/polar/RadialBar.tsx(5 hunks)src/state/cartesianAxisSlice.ts(2 hunks)src/state/selectors/axisSelectors.ts(13 hunks)src/state/selectors/polarScaleSelectors.ts(1 hunks)src/state/selectors/radarSelectors.ts(1 hunks)src/state/selectors/radialBarSelectors.ts(2 hunks)src/state/selectors/tooltipSelectors.ts(4 hunks)src/util/ChartUtils.ts(6 hunks)src/util/scale/CustomScaleDefinition.ts(1 hunks)src/util/scale/RechartsScale.ts(1 hunks)src/util/types.ts(5 hunks)storybook/storybook-addon-recharts/inspectors/generic/ScaleInspector.tsx(1 hunks)test/cartesian/Area.spec.tsx(7 hunks)test/cartesian/CartesianAxis.spec.tsx(2 hunks)test/component/Tooltip/itemSorter.spec.tsx(12 hunks)test/helper/expectScale.ts(1 hunks)test/helper/mockAxes.ts(1 hunks)test/helper/toBeRechartsScale.ts(1 hunks)test/state/selectors/axisSelectors.spec.tsx(1 hunks)test/state/selectors/selectAxisScale.spec.tsx(1 hunks)test/util/ChartUtils.spec.tsx(3 hunks)test/util/ChartUtils/checkDomainOfScale.spec.ts(1 hunks)test/util/ChartUtils/getCateCoordinateOfLine.spec.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (8)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
**/*.{ts,tsx}: Never useanytype (implicit or explicit) in TypeScript code
Preferunknownoveranyand refine the type in TypeScript
Type function parameters and return values explicitly in TypeScript, do not rely on implicit any or inference; exceptions are React components and trivial functions
Do not useastype assertions in TypeScript; the only exception isas const
Files:
test/util/ChartUtils/checkDomainOfScale.spec.tssrc/state/cartesianAxisSlice.tstest/state/selectors/axisSelectors.spec.tsxsrc/state/selectors/radialBarSelectors.tssrc/util/scale/CustomScaleDefinition.tstest/helper/mockAxes.tstest/util/ChartUtils/getCateCoordinateOfLine.spec.tsstorybook/storybook-addon-recharts/inspectors/generic/ScaleInspector.tsxtest/cartesian/CartesianAxis.spec.tsxsrc/util/scale/RechartsScale.tssrc/polar/PolarRadiusAxis.tsxsrc/polar/PolarAngleAxis.tsxtest/state/selectors/selectAxisScale.spec.tsxsrc/cartesian/Bar.tsxtest/cartesian/Area.spec.tsxtest/helper/toBeRechartsScale.tssrc/cartesian/ReferenceArea.tsxsrc/util/ChartUtils.tssrc/index.tssrc/cartesian/Scatter.tsxsrc/polar/RadialBar.tsxsrc/state/selectors/radarSelectors.tstest/helper/expectScale.tstest/component/Tooltip/itemSorter.spec.tsxsrc/cartesian/ZAxis.tsxsrc/cartesian/Line.tsxsrc/state/selectors/polarScaleSelectors.tssrc/cartesian/Area.tsxsrc/cartesian/ErrorBar.tsxsrc/polar/Radar.tsxsrc/util/types.tssrc/state/selectors/tooltipSelectors.tstest/util/ChartUtils.spec.tsxsrc/state/selectors/axisSelectors.ts
test/**/*.spec.{ts,tsx}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Aim for 100% unit test code coverage when writing new code
Files:
test/util/ChartUtils/checkDomainOfScale.spec.tstest/state/selectors/axisSelectors.spec.tsxtest/util/ChartUtils/getCateCoordinateOfLine.spec.tstest/cartesian/CartesianAxis.spec.tsxtest/state/selectors/selectAxisScale.spec.tsxtest/cartesian/Area.spec.tsxtest/component/Tooltip/itemSorter.spec.tsxtest/util/ChartUtils.spec.tsx
**/*.{js,ts,tsx}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Ensure code lints by running
npm run lintand follows Airbnb's Style Guide
Files:
test/util/ChartUtils/checkDomainOfScale.spec.tssrc/state/cartesianAxisSlice.tstest/state/selectors/axisSelectors.spec.tsxsrc/state/selectors/radialBarSelectors.tssrc/util/scale/CustomScaleDefinition.tstest/helper/mockAxes.tstest/util/ChartUtils/getCateCoordinateOfLine.spec.tsstorybook/storybook-addon-recharts/inspectors/generic/ScaleInspector.tsxtest/cartesian/CartesianAxis.spec.tsxsrc/util/scale/RechartsScale.tssrc/polar/PolarRadiusAxis.tsxsrc/polar/PolarAngleAxis.tsxtest/state/selectors/selectAxisScale.spec.tsxsrc/cartesian/Bar.tsxtest/cartesian/Area.spec.tsxtest/helper/toBeRechartsScale.tssrc/cartesian/ReferenceArea.tsxsrc/util/ChartUtils.tssrc/index.tssrc/cartesian/Scatter.tsxsrc/polar/RadialBar.tsxsrc/state/selectors/radarSelectors.tstest/helper/expectScale.tstest/component/Tooltip/itemSorter.spec.tsxsrc/cartesian/ZAxis.tsxsrc/cartesian/Line.tsxsrc/state/selectors/polarScaleSelectors.tssrc/cartesian/Area.tsxsrc/cartesian/ErrorBar.tsxsrc/polar/Radar.tsxsrc/util/types.tssrc/state/selectors/tooltipSelectors.tstest/util/ChartUtils.spec.tsxsrc/state/selectors/axisSelectors.ts
test/**/*.{test,spec}.{ts,tsx}
📄 CodeRabbit inference engine (test/README.md)
test/**/*.{test,spec}.{ts,tsx}: Aim for 100% unit test code coverage when writing new code
Prefer to use thecreateSelectorTestCasehelper function when writing or modifying tests
Use theexpectLastCalledWithhelper function instead ofexpect(spy).toHaveBeenLastCalledWith(...)for better typing and autocompletion
Verify the number of selector calls using the spy object fromcreateSelectorTestCaseto spot unnecessary re-renders and improve performance
MockgetBoundingClientRectin tests using the helper function provided intest/helper/MockGetBoundingClientRect.ts
Usevi.useFakeTimers()in all tests due to Redux autoBatchEnhancer dependency on timers andrequestAnimationFrame
Callvi.runOnlyPendingTimers()to advance timers after renders when not usingcreateSelectorTestCasehelper, and avoidvi.runAllTimers()to prevent infinite loops
UseuserEvent.setup({ advanceTimers: vi.runOnlyPendingTimers })or theuserEventSetuphelper function fromtest/helper/userEventSetup.tswhen creating userEvent instances
When testing tooltips on hover, usevi.runOnlyPendingTimers()after eachuserEvent.hover()call or use theshowTooltiphelper function fromtooltipTestHelpers.tsto account for requestAnimationFrame delays
Files:
test/util/ChartUtils/checkDomainOfScale.spec.tstest/state/selectors/axisSelectors.spec.tsxtest/util/ChartUtils/getCateCoordinateOfLine.spec.tstest/cartesian/CartesianAxis.spec.tsxtest/state/selectors/selectAxisScale.spec.tsxtest/cartesian/Area.spec.tsxtest/component/Tooltip/itemSorter.spec.tsxtest/util/ChartUtils.spec.tsx
**/*.spec.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
When running unit tests, prefer to run a single test file using
npm run test -- path/to/TestFile.spec.tsxrather than running all tests withnpm test
Files:
test/util/ChartUtils/checkDomainOfScale.spec.tstest/state/selectors/axisSelectors.spec.tsxtest/util/ChartUtils/getCateCoordinateOfLine.spec.tstest/cartesian/CartesianAxis.spec.tsxtest/state/selectors/selectAxisScale.spec.tsxtest/cartesian/Area.spec.tsxtest/component/Tooltip/itemSorter.spec.tsxtest/util/ChartUtils.spec.tsx
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (DEVELOPING.md)
All imports from
rechartsmust use the public API entry point; imports from internal paths likerecharts/types/*orrecharts/src/*are not allowed
Files:
test/util/ChartUtils/checkDomainOfScale.spec.tssrc/state/cartesianAxisSlice.tstest/state/selectors/axisSelectors.spec.tsxsrc/state/selectors/radialBarSelectors.tssrc/util/scale/CustomScaleDefinition.tstest/helper/mockAxes.tstest/util/ChartUtils/getCateCoordinateOfLine.spec.tsstorybook/storybook-addon-recharts/inspectors/generic/ScaleInspector.tsxtest/cartesian/CartesianAxis.spec.tsxsrc/util/scale/RechartsScale.tssrc/polar/PolarRadiusAxis.tsxsrc/polar/PolarAngleAxis.tsxtest/state/selectors/selectAxisScale.spec.tsxsrc/cartesian/Bar.tsxtest/cartesian/Area.spec.tsxtest/helper/toBeRechartsScale.tssrc/cartesian/ReferenceArea.tsxsrc/util/ChartUtils.tssrc/index.tssrc/cartesian/Scatter.tsxsrc/polar/RadialBar.tsxsrc/state/selectors/radarSelectors.tstest/helper/expectScale.tstest/component/Tooltip/itemSorter.spec.tsxsrc/cartesian/ZAxis.tsxsrc/cartesian/Line.tsxsrc/state/selectors/polarScaleSelectors.tssrc/cartesian/Area.tsxsrc/cartesian/ErrorBar.tsxsrc/polar/Radar.tsxsrc/util/types.tssrc/state/selectors/tooltipSelectors.tstest/util/ChartUtils.spec.tsxsrc/state/selectors/axisSelectors.ts
src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Do not hardcode any strings or formatting choices in library code; users should provide localized strings as desired
Files:
src/state/cartesianAxisSlice.tssrc/state/selectors/radialBarSelectors.tssrc/util/scale/CustomScaleDefinition.tssrc/util/scale/RechartsScale.tssrc/polar/PolarRadiusAxis.tsxsrc/polar/PolarAngleAxis.tsxsrc/cartesian/Bar.tsxsrc/cartesian/ReferenceArea.tsxsrc/util/ChartUtils.tssrc/index.tssrc/cartesian/Scatter.tsxsrc/polar/RadialBar.tsxsrc/state/selectors/radarSelectors.tssrc/cartesian/ZAxis.tsxsrc/cartesian/Line.tsxsrc/state/selectors/polarScaleSelectors.tssrc/cartesian/Area.tsxsrc/cartesian/ErrorBar.tsxsrc/polar/Radar.tsxsrc/util/types.tssrc/state/selectors/tooltipSelectors.tssrc/state/selectors/axisSelectors.ts
test/component/**/*.spec.tsx
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Use React Testing Library for testing component interactions and behavior upon rendering
Files:
test/component/Tooltip/itemSorter.spec.tsx
🧠 Learnings (16)
📓 Common learnings
Learnt from: CR
Repo: recharts/recharts PR: 0
File: DEVELOPING.md:0-0
Timestamp: 2025-12-06T03:36:59.377Z
Learning: Applies to **/*.{ts,tsx,js,jsx} : All imports from `recharts` must use the public API entry point; imports from internal paths like `recharts/types/*` or `recharts/src/*` are not allowed
Learnt from: PavelVanecek
Repo: recharts/recharts PR: 6771
File: src/shape/Curve.tsx:39-40
Timestamp: 2025-12-14T13:58:49.197Z
Learning: In the recharts codebase, `useAppSelector` and hooks like `useChartLayout()` are designed to return `undefined` when used outside Redux Provider context, rather than throwing errors. This allows components like `Curve` that call these hooks to work standalone by falling back to prop values.
📚 Learning: 2025-12-06T03:36:59.377Z
Learnt from: CR
Repo: recharts/recharts PR: 0
File: DEVELOPING.md:0-0
Timestamp: 2025-12-06T03:36:59.377Z
Learning: Applies to **/*.{ts,tsx,js,jsx} : All imports from `recharts` must use the public API entry point; imports from internal paths like `recharts/types/*` or `recharts/src/*` are not allowed
Applied to files:
scripts/snapshots/typesFiles.txtscripts/snapshots/libFiles.txtsrc/state/cartesianAxisSlice.tssrc/state/selectors/radialBarSelectors.tstest/helper/mockAxes.tstest/util/ChartUtils/getCateCoordinateOfLine.spec.tsstorybook/storybook-addon-recharts/inspectors/generic/ScaleInspector.tsxtest/cartesian/CartesianAxis.spec.tsxsrc/util/scale/RechartsScale.tssrc/polar/PolarRadiusAxis.tsxsrc/polar/PolarAngleAxis.tsxtest/helper/toBeRechartsScale.tssrc/cartesian/ReferenceArea.tsxsrc/util/ChartUtils.tssrc/index.tssrc/state/selectors/radarSelectors.tstest/helper/expectScale.tsscripts/snapshots/es6Files.txtsrc/state/selectors/polarScaleSelectors.tssrc/polar/Radar.tsxsrc/util/types.tssrc/state/selectors/tooltipSelectors.tstest/util/ChartUtils.spec.tsxsrc/state/selectors/axisSelectors.ts
📚 Learning: 2025-11-23T13:30:10.395Z
Learnt from: PavelVanecek
Repo: recharts/recharts PR: 6669
File: www/src/docs/exampleComponents/ScatterChart/ScatterChartWithLabels.tsx:2-2
Timestamp: 2025-11-23T13:30:10.395Z
Learning: The `TooltipIndex` type from recharts is defined in `src/state/tooltipSlice.ts` but is not currently exported from the public API entry points. It should not be imported from `recharts/types/state/tooltipSlice` as this is an internal implementation path. An ESLint rule is needed to prevent regressions.
Applied to files:
scripts/snapshots/typesFiles.txtsrc/state/cartesianAxisSlice.tssrc/state/selectors/radialBarSelectors.tstest/helper/mockAxes.tstest/util/ChartUtils/getCateCoordinateOfLine.spec.tsstorybook/storybook-addon-recharts/inspectors/generic/ScaleInspector.tsxtest/cartesian/CartesianAxis.spec.tsxsrc/util/scale/RechartsScale.tssrc/polar/PolarRadiusAxis.tsxsrc/polar/PolarAngleAxis.tsxtest/helper/toBeRechartsScale.tssrc/cartesian/ReferenceArea.tsxsrc/util/ChartUtils.tssrc/index.tssrc/state/selectors/radarSelectors.tstest/helper/expectScale.tstest/component/Tooltip/itemSorter.spec.tsxsrc/cartesian/ZAxis.tsxsrc/state/selectors/polarScaleSelectors.tssrc/polar/Radar.tsxsrc/util/types.tssrc/state/selectors/tooltipSelectors.tstest/util/ChartUtils.spec.tsxsrc/state/selectors/axisSelectors.ts
📚 Learning: 2025-12-14T13:58:49.197Z
Learnt from: PavelVanecek
Repo: recharts/recharts PR: 6771
File: src/shape/Curve.tsx:39-40
Timestamp: 2025-12-14T13:58:49.197Z
Learning: In the recharts codebase, `useAppSelector` and hooks like `useChartLayout()` are designed to return `undefined` when used outside Redux Provider context, rather than throwing errors. This allows components like `Curve` that call these hooks to work standalone by falling back to prop values.
Applied to files:
scripts/snapshots/libFiles.txtsrc/state/cartesianAxisSlice.tssrc/state/selectors/radialBarSelectors.tstest/helper/mockAxes.tsstorybook/storybook-addon-recharts/inspectors/generic/ScaleInspector.tsxsrc/util/ChartUtils.tssrc/state/selectors/radarSelectors.tssrc/state/selectors/polarScaleSelectors.tssrc/util/types.tssrc/state/selectors/tooltipSelectors.tssrc/state/selectors/axisSelectors.ts
📚 Learning: 2025-11-25T01:23:08.250Z
Learnt from: CR
Repo: recharts/recharts PR: 0
File: test/README.md:0-0
Timestamp: 2025-11-25T01:23:08.250Z
Learning: Applies to test/**/*.{test,spec}.{ts,tsx} : Verify the number of selector calls using the spy object from `createSelectorTestCase` to spot unnecessary re-renders and improve performance
Applied to files:
test/state/selectors/axisSelectors.spec.tsxtest/helper/mockAxes.tstest/util/ChartUtils/getCateCoordinateOfLine.spec.tstest/cartesian/CartesianAxis.spec.tsxtest/state/selectors/selectAxisScale.spec.tsxtest/cartesian/Area.spec.tsxtest/helper/toBeRechartsScale.tssrc/state/selectors/radarSelectors.tstest/helper/expectScale.tstest/component/Tooltip/itemSorter.spec.tsxsrc/state/selectors/tooltipSelectors.ts
📚 Learning: 2025-11-25T01:23:08.250Z
Learnt from: CR
Repo: recharts/recharts PR: 0
File: test/README.md:0-0
Timestamp: 2025-11-25T01:23:08.250Z
Learning: Applies to test/**/*.{test,spec}.{ts,tsx} : Prefer to use the `createSelectorTestCase` helper function when writing or modifying tests
Applied to files:
test/state/selectors/axisSelectors.spec.tsxtest/util/ChartUtils/getCateCoordinateOfLine.spec.tstest/cartesian/CartesianAxis.spec.tsxtest/state/selectors/selectAxisScale.spec.tsxtest/cartesian/Area.spec.tsxtest/helper/toBeRechartsScale.tstest/helper/expectScale.ts
📚 Learning: 2025-11-25T01:23:08.250Z
Learnt from: CR
Repo: recharts/recharts PR: 0
File: test/README.md:0-0
Timestamp: 2025-11-25T01:23:08.250Z
Learning: Applies to test/**/*.{test,spec}.{ts,tsx} : Use the `expectLastCalledWith` helper function instead of `expect(spy).toHaveBeenLastCalledWith(...)` for better typing and autocompletion
Applied to files:
test/state/selectors/axisSelectors.spec.tsxtest/helper/mockAxes.tstest/util/ChartUtils/getCateCoordinateOfLine.spec.tstest/state/selectors/selectAxisScale.spec.tsxtest/cartesian/Area.spec.tsxtest/helper/toBeRechartsScale.tstest/helper/expectScale.ts
📚 Learning: 2025-11-25T01:23:08.250Z
Learnt from: CR
Repo: recharts/recharts PR: 0
File: test/README.md:0-0
Timestamp: 2025-11-25T01:23:08.250Z
Learning: Applies to test/**/*.{test,spec}.{ts,tsx} : Call `vi.runOnlyPendingTimers()` to advance timers after renders when not using `createSelectorTestCase` helper, and avoid `vi.runAllTimers()` to prevent infinite loops
Applied to files:
test/state/selectors/axisSelectors.spec.tsxtest/cartesian/CartesianAxis.spec.tsxtest/state/selectors/selectAxisScale.spec.tsx
📚 Learning: 2025-11-25T01:23:08.250Z
Learnt from: CR
Repo: recharts/recharts PR: 0
File: test/README.md:0-0
Timestamp: 2025-11-25T01:23:08.250Z
Learning: Applies to test/**/*.{test,spec}.{ts,tsx} : Mock `getBoundingClientRect` in tests using the helper function provided in `test/helper/MockGetBoundingClientRect.ts`
Applied to files:
test/helper/mockAxes.tstest/util/ChartUtils/getCateCoordinateOfLine.spec.tstest/cartesian/Area.spec.tsxtest/helper/toBeRechartsScale.tstest/helper/expectScale.ts
📚 Learning: 2025-11-25T01:23:08.250Z
Learnt from: CR
Repo: recharts/recharts PR: 0
File: test/README.md:0-0
Timestamp: 2025-11-25T01:23:08.250Z
Learning: Applies to test/**/*.{test,spec}.{ts,tsx} : Use `vi.useFakeTimers()` in all tests due to Redux autoBatchEnhancer dependency on timers and `requestAnimationFrame`
Applied to files:
test/helper/mockAxes.tstest/state/selectors/selectAxisScale.spec.tsxtest/cartesian/Area.spec.tsxtest/helper/toBeRechartsScale.ts
📚 Learning: 2025-11-25T01:22:59.729Z
Learnt from: CR
Repo: recharts/recharts PR: 0
File: CONTRIBUTING.md:0-0
Timestamp: 2025-11-25T01:22:59.729Z
Learning: Applies to test/component/**/*.spec.tsx : Use React Testing Library for testing component interactions and behavior upon rendering
Applied to files:
test/cartesian/CartesianAxis.spec.tsxtest/helper/toBeRechartsScale.tstest/helper/expectScale.ts
📚 Learning: 2025-12-14T13:58:49.197Z
Learnt from: PavelVanecek
Repo: recharts/recharts PR: 6771
File: src/shape/Curve.tsx:39-40
Timestamp: 2025-12-14T13:58:49.197Z
Learning: In the recharts codebase, hooks like useAppSelector and other hooks (e.g., useChartLayout()) return undefined when used outside Redux Provider context, instead of throwing. This enables components that call these hooks, such as Curve, to operate in standalone mode by falling back to prop values. During code reviews, ensure TSX files gracefully handle undefined results from hooks and implement fallbacks (e.g., via default props or explicit prop-based values) rather than assuming the hook is always within Provider.
Applied to files:
src/polar/PolarRadiusAxis.tsxsrc/polar/PolarAngleAxis.tsxsrc/cartesian/Bar.tsxsrc/cartesian/ReferenceArea.tsxsrc/cartesian/Scatter.tsxsrc/polar/RadialBar.tsxsrc/cartesian/ZAxis.tsxsrc/cartesian/Line.tsxsrc/cartesian/Area.tsxsrc/cartesian/ErrorBar.tsxsrc/polar/Radar.tsx
📚 Learning: 2025-11-19T14:08:01.728Z
Learnt from: PavelVanecek
Repo: recharts/recharts PR: 6659
File: www/src/components/GuideView/Performance/index.tsx:232-250
Timestamp: 2025-11-19T14:08:01.728Z
Learning: In Recharts version 3.4.2, object-as-prop optimizations were introduced to reduce unnecessary re-renders when new object references are passed as props. This affects the recommendation for the `react-perf/jsx-no-new-object-as-prop` ESLint rule.
Applied to files:
src/polar/PolarAngleAxis.tsxsrc/cartesian/ReferenceArea.tsxsrc/state/selectors/tooltipSelectors.ts
📚 Learning: 2025-11-25T01:23:08.250Z
Learnt from: CR
Repo: recharts/recharts PR: 0
File: test/README.md:0-0
Timestamp: 2025-11-25T01:23:08.250Z
Learning: Applies to test/**/*.{test,spec}.{ts,tsx} : When testing tooltips on hover, use `vi.runOnlyPendingTimers()` after each `userEvent.hover()` call or use the `showTooltip` helper function from `tooltipTestHelpers.ts` to account for requestAnimationFrame delays
Applied to files:
test/state/selectors/selectAxisScale.spec.tsxtest/component/Tooltip/itemSorter.spec.tsxsrc/state/selectors/tooltipSelectors.ts
📚 Learning: 2025-11-25T01:22:59.729Z
Learnt from: CR
Repo: recharts/recharts PR: 0
File: CONTRIBUTING.md:0-0
Timestamp: 2025-11-25T01:22:59.729Z
Learning: Applies to storybook/stories/**/*.stories.tsx : Update Storybook stories when APIs have been changed to ensure they work as expected
Applied to files:
test/cartesian/Area.spec.tsxtest/helper/toBeRechartsScale.tstest/helper/expectScale.ts
📚 Learning: 2025-12-08T08:23:26.261Z
Learnt from: PavelVanecek
Repo: recharts/recharts PR: 6750
File: src/state/selectors/axisSelectors.ts:593-602
Timestamp: 2025-12-08T08:23:26.261Z
Learning: In the recharts codebase, `DataKey<any>` is an intentional exception to the "no any" rule while proper typing is being developed. This should not be flagged in reviews.
Applied to files:
src/util/ChartUtils.ts
🧬 Code graph analysis (16)
test/util/ChartUtils/checkDomainOfScale.spec.ts (1)
src/util/ChartUtils.ts (1)
checkDomainOfScale(283-300)
src/state/cartesianAxisSlice.ts (3)
src/util/types.ts (1)
ScaleType(182-182)src/index.ts (1)
CustomScaleDefinition(97-97)src/util/scale/CustomScaleDefinition.ts (1)
CustomScaleDefinition(13-63)
test/state/selectors/axisSelectors.spec.tsx (2)
src/state/cartesianAxisSlice.ts (1)
XAxisSettings(78-82)src/state/selectors/axisSelectors.ts (1)
BaseAxisWithScale(2051-2051)
src/util/scale/CustomScaleDefinition.ts (2)
src/index.ts (1)
CustomScaleDefinition(97-97)src/util/types.ts (1)
CategoricalDomainItem(732-732)
test/util/ChartUtils/getCateCoordinateOfLine.spec.ts (2)
test/helper/mockAxes.ts (1)
mockScale(5-5)src/util/scale/RechartsScale.ts (1)
RechartsScale(15-52)
test/cartesian/CartesianAxis.spec.tsx (2)
src/index.ts (1)
CustomScaleDefinition(97-97)src/util/scale/CustomScaleDefinition.ts (1)
CustomScaleDefinition(13-63)
src/util/scale/RechartsScale.ts (6)
src/util/types.ts (2)
CategoricalDomainItem(732-732)D3ScaleType(160-175)src/index.ts (1)
CustomScaleDefinition(97-97)src/util/scale/CustomScaleDefinition.ts (1)
CustomScaleDefinition(13-63)src/util/DataUtils.ts (1)
upperFirst(189-195)src/state/selectors/axisSelectors.ts (1)
AxisRange(1415-1415)src/util/ChartUtils.ts (1)
checkDomainOfScale(283-300)
src/polar/PolarAngleAxis.tsx (3)
src/util/types.ts (1)
ScaleType(182-182)src/index.ts (1)
CustomScaleDefinition(97-97)src/util/scale/CustomScaleDefinition.ts (1)
CustomScaleDefinition(13-63)
src/util/ChartUtils.ts (4)
src/util/types.ts (2)
TickItem(827-832)CategoricalDomainItem(732-732)src/util/isWellBehavedNumber.ts (1)
isWellBehavedNumber(1-3)src/util/DataUtils.ts (3)
isNotNil(202-204)isNullish(180-182)isNumber(24-25)src/util/scale/RechartsScale.ts (1)
RechartsScale(15-52)
src/polar/RadialBar.tsx (1)
src/util/DataUtils.ts (1)
mathSign(5-14)
src/cartesian/ZAxis.tsx (3)
src/util/types.ts (1)
ScaleType(182-182)src/index.ts (1)
CustomScaleDefinition(97-97)src/util/scale/CustomScaleDefinition.ts (1)
CustomScaleDefinition(13-63)
src/cartesian/Area.tsx (3)
src/util/types.ts (1)
NullableCoordinate(105-108)src/util/ChartUtils.ts (1)
getCateCoordinateOfLine(503-542)src/state/selectors/areaSelectors.ts (1)
AreaPointItem(24-29)
src/polar/Radar.tsx (1)
src/util/DataUtils.ts (1)
isNullish(180-182)
src/util/types.ts (2)
src/index.ts (1)
CustomScaleDefinition(97-97)src/util/scale/CustomScaleDefinition.ts (1)
CustomScaleDefinition(13-63)
src/state/selectors/tooltipSelectors.ts (3)
src/util/types.ts (2)
TickItem(827-832)CategoricalDomainItem(732-732)src/util/isWellBehavedNumber.ts (1)
isWellBehavedNumber(1-3)src/util/DataUtils.ts (1)
isNotNil(202-204)
test/util/ChartUtils.spec.tsx (5)
src/index.ts (1)
CustomScaleDefinition(97-97)src/util/scale/CustomScaleDefinition.ts (1)
CustomScaleDefinition(13-63)src/util/ChartUtils.ts (2)
AxisPropsNeededForTicksGenerator(152-167)getBandSizeOfAxis(650-678)src/state/cartesianAxisSlice.ts (1)
defaultAxisId(12-12)src/state/selectors/axisSelectors.ts (1)
BaseAxisWithScale(2051-2051)
⏰ 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 (69)
test/util/ChartUtils/checkDomainOfScale.spec.ts (1)
168-178: LGTM!The test correctly verifies that
checkDomainOfScalenormalizes domains with more than 2 elements while preserving domains with 2 or fewer elements. The before/after comparison logic is sound.test/util/ChartUtils/getCateCoordinateOfLine.spec.ts (1)
3-6: LGTM!The import path update aligns with the scale definition refactor. The typed mock with
@ts-expect-erroris appropriate for test purposes, as the mock implements only the call signature without the fullRechartsScaleinterface (which requiresdomain(),range(), etc.).src/state/selectors/polarScaleSelectors.ts (1)
27-27: LGTM!Import path update aligns with the scale definition refactor, moving
RechartsScaleto its dedicated module.test/helper/toBeRechartsScale.ts (1)
3-4: LGTM!Import path update aligns with the scale definition refactor.
src/polar/PolarRadiusAxis.tsx (1)
27-27: LGTM!Import path update aligns with the scale definition refactor.
test/state/selectors/selectAxisScale.spec.tsx (1)
40-40: Clarify why the call count assertion was removed.The commented-out call count assertion removes an important performance check. Per coding guidelines, verifying selector call counts helps spot unnecessary re-renders.
If the call count intentionally changed due to the scale refactor, please:
- Document why the call count changed
- Update the assertion with the new expected count instead of removing it
- Consider whether the change indicates a performance regression
If this assertion is no longer valid, please remove the commented line entirely rather than leaving it in the code.
src/cartesian/ReferenceArea.tsx (1)
22-22: LGTM!Import path update aligns with the scale definition refactor.
storybook/storybook-addon-recharts/inspectors/generic/ScaleInspector.tsx (1)
3-3: LGTM: Import path updated to new scale module.The import path has been correctly updated to reference the new dedicated RechartsScale module, aligning with the PR's refactoring of scale definitions.
scripts/snapshots/es6Files.txt (1)
240-241: LGTM: Snapshot correctly tracks new scale modules.The snapshot file properly includes the new CustomScaleDefinition and RechartsScale modules under the es6/util/scale/ directory, ensuring build consistency.
src/index.ts (1)
97-97: LGTM: CustomScaleDefinition added to public API.The new CustomScaleDefinition type is correctly exported as part of the public API, enabling users to provide custom scale implementations while maintaining a stable external interface.
src/polar/PolarAngleAxis.tsx (2)
26-27: LGTM: Imports updated for scale type split.The addition of both RechartsScale and CustomScaleDefinition imports supports the external/internal scale type separation pattern.
66-66: LGTM: Public prop correctly uses CustomScaleDefinition.The scale prop type has been appropriately changed to use CustomScaleDefinition for the public API. Meanwhile, InsideProps at line 107 correctly retains RechartsScale for internal use. This demonstrates the intended pattern: stable external types for users, flexible internal types for implementation.
test/helper/expectScale.ts (1)
4-4: LGTM: Test helper import updated to new scale module.The import path has been correctly updated to reference the dedicated RechartsScale module. Test helpers appropriately use the internal RechartsScale type for validation.
src/state/selectors/radarSelectors.ts (1)
12-12: LGTM: Imports reorganized for scale module separation.The imports have been appropriately split: utility functions remain imported from ChartUtils while RechartsScale is imported from its dedicated module. Selector logic remains unchanged.
Also applies to: 16-16
scripts/snapshots/libFiles.txt (1)
240-241: LGTM: Snapshot correctly tracks new CommonJS scale modules.The snapshot file properly includes the new CustomScaleDefinition and RechartsScale modules in the lib (CommonJS) build output, ensuring build consistency across module formats.
scripts/snapshots/typesFiles.txt (1)
240-241: LGTM: Snapshot correctly tracks new type declarations.The snapshot file properly includes the type declaration files for CustomScaleDefinition and RechartsScale, ensuring TypeScript consumers have access to proper type definitions for the new scale modules.
test/state/selectors/axisSelectors.spec.tsx (1)
2453-2484: expectedXAxis typing matches new BaseAxisWithScale contractUsing
Omit<XAxisSettings, 'scale'> & BaseAxisWithScalecorrectly avoids a duplicatescaleproperty while asserting the richer internal axis+scale shape the selector returns. The structure matches the asserted object below, so this refinement looks good.src/cartesian/Line.tsx (1)
923-936: Coercing undefined scale outputs tonullis correctChanging the horizontal-branch point construction to
y: y ?? nullensuresLinePointItem.yis alwaysnumber | nulleven when the scale returnsundefined, aligning with the type and gap-handling semantics without affecting valid numeric cases.test/cartesian/Area.spec.tsx (1)
973-980: OK to suppress type errors for intentionally incomplete axis mocksThe added
// @ts-expect-error incomplete mockannotations aroundscale(andtype) onBaseAxisWithScaletest doubles are appropriate here: these tests exercisegetBaseValue’s runtime behavior with minimal axis mocks and don’t warrant fully populating the axis shape after the RechartsScale typing changes.Also applies to: 1029-1035, 1047-1053, 1104-1110, 1121-1127, 1181-1187, 1198-1204
test/component/Tooltip/itemSorter.spec.tsx (1)
1311-1434: Aligning radial tooltip payload expectations with explicitstartAngle: 0Updating the expected tooltip payloads to include
startAngle: 0for RadialBar-derived entries matches the new sector metadata shape while keeping the rest of the assertions intact. This keeps the tests faithful to the runtime objects without changing itemSorter behavior.Also applies to: 1475-1597, 1628-1760, 1789-1917
src/state/selectors/radialBarSelectors.ts (1)
14-15: RechartsScale import refactor preserves selector behaviorSwitching
RechartsScaleto the dedicatedutil/scale/RechartsScalemodule and narrowing the ChartUtils import list keeps the scale typing consistent with the new abstraction without altering howselectRadiusAxisWithScale/selectAngleAxisWithScalebuildBaseAxisWithScale. No further changes needed.Also applies to: 37-51, 73-90
src/cartesian/ZAxis.tsx (1)
3-9: ZAxisscaletyping correctly migrated toCustomScaleDefinitionAllowing
scale?: ScaleType | CustomScaleDefinition | undefinedand importingCustomScaleDefinitionfrom the shared scale module aligns ZAxis with the new external scale contract, while remaining backward compatible with string scale types and implicit defaults.Also applies to: 61-71
src/cartesian/ErrorBar.tsx (1)
169-188: Guarding against null scale outputs in error bar segmentsThe added
xMin/xMaxandyMin/yMaxnull checks ensure error bar segments are only rendered when the scale returns valid numeric coordinates, avoiding lines with undefined attributes while preserving existing behavior for well-formed data.Also applies to: 189-207
test/cartesian/CartesianAxis.spec.tsx (1)
5-6: Tests now validateCartesianAxisagainstCustomScaleDefinitionTyping
exampleScaleasCustomScaleDefinition<number>and importing it from the public entry point updates the tests to reflect the new external scale contract. UsingscaleLinear()as the concrete implementation remains appropriate and structurally compatible.Also applies to: 20-20
test/helper/mockAxes.ts (1)
3-5: LGTM!The import correctly uses the public API entry point (
'../../src'resolves tosrc/index.ts), andCustomScaleDefinition<number>is the appropriate type for user-provided scales. The d3scaleLinear()function returns a scale compatible with this interface.src/polar/Radar.tsx (2)
239-241: LGTM!The nullish coalescing operators (
?? 0) correctly handle the case wherescale()returnsundefined(as per theCustomScaleDefinitioninterface). TheisNullish(pointValue)guard provides an early fallback to0for null/undefined values before invoking the scale, which is appropriate defensive coding.
264-276: LGTM!The null-safety pattern for
baseValueradius calculation is consistent with the earlierpointValuehandling at line 241. This ensures robust behavior when scale returnsundefined.src/state/cartesianAxisSlice.ts (1)
20-25: LGTM!The type change from
RechartsScaletoCustomScaleDefinitionforBaseCartesianAxis.scalecorrectly reflects the PR's objective:CustomScaleDefinitionis the external/public type for user-provided props, whileRechartsScaleis the internal type. The updated documentation clarifies this is user-defined axis settings.src/cartesian/Bar.tsx (1)
976-1006: LGTM!The null checks for
baseValueScaleandcurrentValueScalein both horizontal (lines 976-980) and vertical (lines 1002-1006) code paths correctly handle the case wherescale()returnsundefined. The early return ofnullensures bars with uncomputable coordinates are filtered out via.filter(Boolean)at line 1047.src/state/selectors/tooltipSelectors.ts (2)
355-368: LGTM!The tick generation now correctly guards against invalid scale results using
isWellBehavedNumber(which checksNumber.isFinite). This filters outundefined,NaN, andInfinityvalues that could result from the scale function, ensuring only valid numeric coordinates are used for ticks.
372-388: LGTM!The domain-based tick generation mirrors the categorical domain pattern with the same
isWellBehavedNumberguard. TheCategoricalDomainItemtype annotation provides proper typing for the domain entries.src/cartesian/Area.tsx (3)
1030-1030: Good null-safety improvement for scale results.The
?? nullfallback correctly handles the case whereyAxis.scale(value1)returnsundefined, ensuring consistentnullvalues for missing coordinates.
1037-1037: Consistent null-safety for vertical layout.Same defensive pattern applied correctly for the x-coordinate in vertical layout.
1044-1067: Proper handling of baseLine with null-safety and fallback.The expanded
baseLinetype now correctly accounts forundefined, and the mapping logic properly applies?? nullfor scale results. The finalbaseLine ?? 0ensures a valid fallback is always returned.src/util/scale/CustomScaleDefinition.ts (1)
1-63: Well-designed public scale interface.The
CustomScaleDefinitioninterface is cleanly designed with:
- Clear separation of getter/setter overloads for
domainandrange- Proper documentation about immutability concerns
- Optional methods for scale-specific features (
bandwidth,ticks)- Correct return type
number | undefinedfor the callable signaturesrc/polar/RadialBar.tsx (4)
570-575: Good explicit type annotations for local variables.The explicit types for
value,innerRadius,outerRadius,startAngle,endAngle, andbackgroundSectorimprove readability and maintainability.
588-589: Good fallback handling for scale results.Using
?? rootStartAngleand?? rootEndAngleas fallbacks ensures the angles have sensible defaults when the scale returnsundefined.
598-617: Properly guarded computation block.The null checks for
innerRadius,endAngle, andstartAnglebefore computing derived values prevent arithmetic on null/undefined values.
629-637: Consistent null-safety for non-radial layout.Same defensive pattern applied for the arc layout path.
test/util/ChartUtils.spec.tsx (2)
35-39: Good explicit typing for scale in tests.Using
CustomScaleDefinition<string>provides clear type annotation for the string-domain scale, improving test clarity and type safety.
114-126: Expanded axis mock objects improve type coverage.Adding all required properties for
BaseAxisWithScaleensures the test objects properly match the expected interface shape.src/util/scale/RechartsScale.ts (3)
8-52: Well-documented internal scale interface.The
RechartsScaleinterface is properly designed as an immutable internal representation:
- Clear documentation about immutability constraints
- No setter methods exposed
- Forward-looking comment about better typing in 4.0
75-99: Good encapsulation of scale creation logic.The
makeRechartsScalefunction properly:
- Uses
copy()to avoid mutating user-provided scales (line 81)- Returns
undefinedfor invalid inputs- Encapsulates the mutation from
checkDomainOfScale(acknowledged in comment)The mutation on line 90 is intentional and well-documented. The internal mutation is acceptable since
getD3ScaleFromTypecreates a fresh scale instance.
80-81: The chained method return type is compatible withRechartsScale<Domain>. D3 scales follow the method chaining pattern where setter methods return the scale itself, and the returned d3-scale is structurally compatible with theRechartsScale<Domain>interface, which requiresdomain(),range(), and a callable function signature—all of which d3-scale instances provide.src/util/types.ts (4)
35-35: LGTM!The import of
CustomScaleDefinitionfrom the new scale module is correctly structured and aligns with the PR's goal of separating external/public scale types from internal ones.
160-182: Well-structured type definitions for scale types.The separation of
D3ScaleType(listing d3-scale string literals) andScaleType(adding 'auto') provides a clean public API. The JSDoc comment appropriately referencesCustomScaleDefinitionfor users who need custom scales.
732-734: Good addition of categorical domain types.
CategoricalDomainItemcorrectly captures the possible domain value types (number, string, Date), andCategoricalDomainas aReadonlyArrayenforces immutability appropriately.
755-755: LGTM!The updated
scaleprop type correctly accepts either a string-basedScaleTypeor a user-providedCustomScaleDefinitionfunction, which is the core goal of this PR.src/state/selectors/axisSelectors.ts (11)
18-25: LGTM!The new type imports (
CategoricalDomainItem,D3ScaleType,ScaleType) are correctly sourced from../../util/typesand align with the updated type definitions.
89-89: LGTM!The import of
makeRechartsScaleandRechartsScalefrom the new dedicated scale module follows the PR's refactoring pattern of centralizing scale utilities.
1093-1133: LGTM!The updated
combineRealScaleTypefunction correctly returnsScaleType | undefinedand properly usesisSupportedScaleNameto validate the generated scale name before returning it.
1144-1158: Good centralization of scale construction.The updated
combineScaleFunctionproperly delegates scale construction tomakeRechartsScale, which handles both user-provided custom scales and string-based scale types. This aligns well with the PR's goal of separating scale definition concerns.
1829-1836: Helpful documentation for related implementations.The JSDoc comments documenting the "four horsemen of tick generation" provide useful navigation for developers working with tick generation logic across the codebase.
Also applies to: 1953-1960
1870-1885: Good defensive null-safety for tick coordinate calculation.The added
isWellBehavedNumbercheck properly handles cases where the scale returnsundefined(as now specified by theRechartsScaleinterface), preventingNaNcoordinates from propagating into tick items.
1890-1904: Consistent null-safety pattern applied.The categorical domain tick mapping correctly applies the same
isWellBehavedNumbervalidation andisNotNilfiltering pattern.
1916-1932: Consistent null-safety in domain-based tick generation.The fallback path for generating ticks from
scale.domain()properly validates scaled values and filters out invalid entries.
1984-1997: Parallel null-safety implementation incombineGraphicalItemTicks.The same defensive patterns are correctly applied in
combineGraphicalItemTicks, maintaining consistency withcombineAxisTicks.Also applies to: 2010-2026
2046-2051: Clear type definition for internal axis representation.The
BaseAxisWithScaletype correctly documents that it represents an axis with an already-computed scale function, replacing the union type with justRechartsScale.
2082-2082: Consistent type pattern for ZAxis.
ZAxisWithScalefollows the same pattern asBaseAxisWithScale, maintaining type consistency across axis types.src/util/ChartUtils.ts (10)
15-15: LGTM!The updated import correctly includes
isNotNilwhich is used for filtering null values from tick arrays.
23-23: LGTM!The
CategoricalDomainItemimport aligns with the new type definitions and is used appropriately in the tick generation functions.
42-42: LGTM!The import of
RechartsScalefrom the new dedicated module follows the PR's refactoring pattern.
170-176: Helpful cross-reference documentation.The JSDoc listing the four related tick generation implementations aids maintainability.
210-230: Good null-safety for grid tick generation.The validation using
isWellBehavedNumberproperly handles cases where the scale returnsundefined, preventing invalid coordinates from being included in the result.
234-248: Consistent null-safety in categorical domain handling.The same defensive pattern is correctly applied to the categorical domain branch.
250-261: LGTM!The numeric ticks branch correctly validates scale results before constructing tick items.
264-280: Consistent handling for domain-based ticks.The fallback path using
scale.domain()properly validates and filters invalid entries, completing the null-safety implementation across all tick generation paths in this function.
515-515: LGTM!The axis scale property is correctly typed as
RechartsScale, aligning with the internal type representation.
564-571: Good defensive handling for bar coordinate calculation.The added null checks for both the value and scaled result properly handle edge cases where the scale returns
undefined. The use ofisNumberis appropriate here to validate the scaled output before performing arithmetic operations.
| const stackedDomain: ReadonlyArray<number> = stackedData ? numericAxis.scale.domain() : null; | ||
| const baseValue = getBaseValueOfBar({ numericAxis }); | ||
| const stackedBarStart: number = numericAxis.scale(baseValue); | ||
| const stackedBarStart: number | undefined = numericAxis.scale(baseValue); |
There was a problem hiding this comment.
Type mismatch: stackedBarStart may be undefined but BarRectangleItem requires number.
After this change, numericAxis.scale(baseValue) can return undefined, making stackedBarStart: number | undefined. However, the BarRectangleItem interface at line 111 declares stackedBarStart: number (non-optional). This creates a type mismatch when constructing the barRectangleItem at line 1032.
Consider either:
- Updating
BarRectangleItem.stackedBarStartto benumber | undefined, or - Adding a fallback:
const stackedBarStart: number = numericAxis.scale(baseValue) ?? 0;
- const stackedBarStart: number | undefined = numericAxis.scale(baseValue);
+ const stackedBarStart: number = numericAxis.scale(baseValue) ?? 0;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const stackedBarStart: number | undefined = numericAxis.scale(baseValue); | |
| const stackedBarStart: number = numericAxis.scale(baseValue) ?? 0; |
🤖 Prompt for AI Agents
In src/cartesian/Bar.tsx around line 952, numericAxis.scale(baseValue) can
return undefined so stackedBarStart is number | undefined which conflicts with
BarRectangleItem.stackedBarStart (declared as number); either (A) update the
BarRectangleItem type (where it's declared) to stackedBarStart: number |
undefined, or (B) keep the type and coerce the value at line 952 to a number by
providing a fallback (e.g., const stackedBarStart: number =
numericAxis.scale(baseValue) ?? 0) and ensure any downstream logic handles the
fallback appropriately.
| }); | ||
| const size = z !== '-' ? zAxis.scale(z) : defaultZ; | ||
| const radius = Math.sqrt(Math.max(size, 0) / Math.PI); | ||
| const radius = size == null ? 0 : Math.sqrt(Math.max(size, 0) / Math.PI); |
There was a problem hiding this comment.
Type inconsistency between runtime guard and interface definition.
The null check size == null ? 0 guards against nullish values, but ScatterPointItem.size is typed as number (Line 113), not nullable. If size can be null/undefined at runtime (e.g., when zAxis.scale(z) returns undefined), update the interface to reflect this.
Apply this diff to align the type with runtime behavior:
/**
* ScatterPointItem size is an abstract number that is used to calculate the radius of the point.
* It's not the radius itself, but rather a value that is used to calculate the radius.
* Interacts with the zAxis range.
*/
- size: number;
+ size: number | undefined;Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/cartesian/Scatter.tsx around line 741 (and update the ScatterPointItem
definition at line 113), the runtime uses a nullish guard for size but the
interface types size as a non-nullable number; change the ScatterPointItem.size
type to allow null/undefined (e.g., size?: number | null or size: number | null)
so it matches runtime behavior, update any related usages/props/types to handle
the optional value accordingly, and run type checks to fix any resulting
callsites that assume size is always a number.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6783 +/- ##
==========================================
- Coverage 93.81% 93.75% -0.06%
==========================================
Files 516 518 +2
Lines 43979 44094 +115
Branches 5005 5035 +30
==========================================
+ Hits 41257 41340 +83
- Misses 2716 2747 +31
- Partials 6 7 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Bundle ReportChanges will increase total bundle size by 12.26kB (0.45%) ⬆️. This is within the configured threshold ✅ Detailed changes
Affected Assets, Files, and Routes:view changes for bundle: recharts/bundle-umdAssets Changed:
view changes for bundle: recharts/bundle-es6Assets Changed:
view changes for bundle: recharts/bundle-cjsAssets Changed:
|
|
Staging Deployment Details
These deployments will remain available for 30 days. To update snapshots: Comment |
| ); | ||
|
|
||
| /** | ||
| * Of on four almost identical implementations of tick generation. |
|
|
||
| /** | ||
| * Of on four almost identical implementations of tick generation. | ||
| * The four horsemen of tick generation are: |
There was a problem hiding this comment.
does this break if someone uses a method from a d3 scale prior to passing it in and then pass it in?
It shouldn't
Something that I've done in the past is used https://d3js.org/d3-scale/linear#linear_invert
In this case I would define my scale with a given domain/range and then because recharts is using that same scale use it to convert X and Y values to my scale. This should still work, cool
There was a problem hiding this comment.
and its always easier to add more later
There was a problem hiding this comment.
what happens if D3 breaks us here?
Description
I like how ScaleHelper (
recharts/src/util/CartesianUtils.ts
Line 18 in b9835fe
Because we accept the function from the outside as props, we can't just add new methods and new arguments to it. So step one is to split the scale function type into two: the internal one, and external one. External one stays stable, internal one will have new methods.
So I did that and fixed all violations - there were again quite a few
anyhiding undefineds. You can see on the screenshot that we got rid of mysteriousy: 0error bars too.Related Issue
#6645
Having new methods on the internal scale will also allow fix for #5262
Motivation and Context
No
anySummary by CodeRabbit
Release Notes
New Features
Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.