Allow extending domain if ReferenceLine is defined by segment#6592
Conversation
WalkthroughThe PR introduces a public Changes
Sequence Diagram(s)sequenceDiagram
participant Chart as Chart Component
participant RL as ReferenceLine
participant Validate as isWellBehavedNumber
participant State as Redux State
participant Selector as Domain Selector
participant Axis as YAxis
Chart->>RL: render ReferenceLine(segment, ifOverflow)
RL->>Validate: validate x1,y1,x2,y2
alt all valid
RL->>State: register/update line with `segment`
State->>Selector: selectReferenceLinesDomain()
Selector->>Selector: extractXCoordinates / extractYCoordinates
Selector->>Axis: request domain including segment endpoints
Axis->>Chart: re-render with extended domain (ifOverflow="extendDomain")
else invalid coord
RL-->>Chart: return null (do not render reference line)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
🧰 Additional context used🧠 Learnings (1)📚 Learning: 2025-10-25T07:35:46.188ZApplied to files:
⏰ 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)
🔇 Additional comments (3)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
test/cartesian/YAxis/YAxis.spec.tsx (1)
2042-2042: Consider reordering assertion for consistency.The domain assertion appears before tick assertions, while similar tests place it after (e.g., line 2036). Reordering would improve consistency.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (15)
test-vr/__snapshots__/tests/ReferenceLine/ifOverflow.spec-vr.tsx-snapshots/ReferenceLine-ifOverflow-with-segment-ifOverfl-38027-xtends-the-domain-to-include-overflowed-parts-1-chromium-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/ReferenceLine/ifOverflow.spec-vr.tsx-snapshots/ReferenceLine-ifOverflow-with-segment-ifOverfl-38027-xtends-the-domain-to-include-overflowed-parts-1-firefox-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/ReferenceLine/ifOverflow.spec-vr.tsx-snapshots/ReferenceLine-ifOverflow-with-segment-ifOverfl-38027-xtends-the-domain-to-include-overflowed-parts-1-webkit-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/ReferenceLine/ifOverflow.spec-vr.tsx-snapshots/ReferenceLine-ifOverflow-with-segment-ifOverflow-default-behavior-is-discard-1-chromium-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/ReferenceLine/ifOverflow.spec-vr.tsx-snapshots/ReferenceLine-ifOverflow-with-segment-ifOverflow-default-behavior-is-discard-1-firefox-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/ReferenceLine/ifOverflow.spec-vr.tsx-snapshots/ReferenceLine-ifOverflow-with-segment-ifOverflow-default-behavior-is-discard-1-webkit-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/ReferenceLine/ifOverflow.spec-vr.tsx-snapshots/ReferenceLine-ifOverflow-with-segment-ifOverflow-discard-does-not-render-overflowed-parts-1-chromium-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/ReferenceLine/ifOverflow.spec-vr.tsx-snapshots/ReferenceLine-ifOverflow-with-segment-ifOverflow-discard-does-not-render-overflowed-parts-1-firefox-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/ReferenceLine/ifOverflow.spec-vr.tsx-snapshots/ReferenceLine-ifOverflow-with-segment-ifOverflow-discard-does-not-render-overflowed-parts-1-webkit-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/ReferenceLine/ifOverflow.spec-vr.tsx-snapshots/ReferenceLine-ifOverflow-with-segment-ifOverflow-hidden-does-not-render-overflowed-parts-1-chromium-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/ReferenceLine/ifOverflow.spec-vr.tsx-snapshots/ReferenceLine-ifOverflow-with-segment-ifOverflow-hidden-does-not-render-overflowed-parts-1-firefox-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/ReferenceLine/ifOverflow.spec-vr.tsx-snapshots/ReferenceLine-ifOverflow-with-segment-ifOverflow-hidden-does-not-render-overflowed-parts-1-webkit-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/ReferenceLine/ifOverflow.spec-vr.tsx-snapshots/ReferenceLine-ifOverflow-with-segment-ifOverflow-visible-renders-overflowed-parts-1-chromium-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/ReferenceLine/ifOverflow.spec-vr.tsx-snapshots/ReferenceLine-ifOverflow-with-segment-ifOverflow-visible-renders-overflowed-parts-1-firefox-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/ReferenceLine/ifOverflow.spec-vr.tsx-snapshots/ReferenceLine-ifOverflow-with-segment-ifOverflow-visible-renders-overflowed-parts-1-webkit-linux.pngis excluded by!**/*.png
📒 Files selected for processing (10)
src/cartesian/ReferenceLine.tsx(6 hunks)src/index.ts(2 hunks)src/state/referenceElementsSlice.ts(2 hunks)src/state/selectors/axisSelectors.ts(1 hunks)test-vr/tests/ReferenceLine/ReferenceLineComponents.tsx(1 hunks)test-vr/tests/ReferenceLine/ifOverflow.spec-vr.tsx(1 hunks)test/cartesian/ReferenceLine/ReferenceLine.spec.tsx(2 hunks)test/cartesian/ReferenceLine/getEndPoints.spec.tsx(5 hunks)test/cartesian/YAxis/YAxis.spec.tsx(3 hunks)test/state/referenceElementsSlice.spec.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
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/state/referenceElementsSlice.tssrc/cartesian/ReferenceLine.tsxsrc/index.tssrc/state/selectors/axisSelectors.ts
🧠 Learnings (6)
📚 Learning: 2025-10-25T07:35:46.188Z
Learnt from: CR
Repo: recharts/recharts PR: 0
File: test/README.md:0-0
Timestamp: 2025-10-25T07:35:46.188Z
Learning: Applies to test/**/*.test.{ts,tsx} : Mock getBoundingClientRect using test/helper/MockGetBoundingClientRect.ts when rendering Recharts components (e.g., Tooltip, Legend, charts)
Applied to files:
test-vr/tests/ReferenceLine/ifOverflow.spec-vr.tsxtest/cartesian/ReferenceLine/ReferenceLine.spec.tsxtest/cartesian/ReferenceLine/getEndPoints.spec.tsxsrc/index.tstest-vr/tests/ReferenceLine/ReferenceLineComponents.tsxtest/cartesian/YAxis/YAxis.spec.tsx
📚 Learning: 2025-10-25T07:35:46.188Z
Learnt from: CR
Repo: recharts/recharts PR: 0
File: test/README.md:0-0
Timestamp: 2025-10-25T07:35:46.188Z
Learning: Applies to test/**/*.test.{ts,tsx} : When testing selectors, verify render counts using the spy and rerenderSameComponent from createSelectorTestCase
Applied to files:
test-vr/tests/ReferenceLine/ifOverflow.spec-vr.tsxtest/cartesian/ReferenceLine/ReferenceLine.spec.tsxtest/cartesian/YAxis/YAxis.spec.tsx
📚 Learning: 2025-10-25T07:35:46.188Z
Learnt from: CR
Repo: recharts/recharts PR: 0
File: test/README.md:0-0
Timestamp: 2025-10-25T07:35:46.188Z
Learning: Applies to test/**/*.test.{ts,tsx} : In tests not using createSelectorTestCase, advance timers after renders with vi.runOnlyPendingTimers()
Applied to files:
test-vr/tests/ReferenceLine/ifOverflow.spec-vr.tsx
📚 Learning: 2025-10-25T07:35:46.188Z
Learnt from: CR
Repo: recharts/recharts PR: 0
File: test/README.md:0-0
Timestamp: 2025-10-25T07:35:46.188Z
Learning: Write unit tests using Vitest and React Testing Library
Applied to files:
test-vr/tests/ReferenceLine/ifOverflow.spec-vr.tsxtest/cartesian/ReferenceLine/getEndPoints.spec.tsx
📚 Learning: 2025-10-25T07:35:46.188Z
Learnt from: CR
Repo: recharts/recharts PR: 0
File: test/README.md:0-0
Timestamp: 2025-10-25T07:35:46.188Z
Learning: Applies to test/**/*.test.{ts,tsx} : Use expectLastCalledWith instead of toHaveBeenLastCalledWith for typed last-call assertions
Applied to files:
test/cartesian/ReferenceLine/getEndPoints.spec.tsxtest/cartesian/YAxis/YAxis.spec.tsx
📚 Learning: 2025-10-25T07:35:46.188Z
Learnt from: CR
Repo: recharts/recharts PR: 0
File: test/README.md:0-0
Timestamp: 2025-10-25T07:35:46.188Z
Learning: Applies to test/**/*.test.{ts,tsx} : Avoid vi.runAllTimers() in tests to prevent infinite loops caused by chained scheduled timers
Applied to files:
test/cartesian/ReferenceLine/getEndPoints.spec.tsx
🧬 Code graph analysis (7)
test-vr/tests/ReferenceLine/ifOverflow.spec-vr.tsx (1)
test-vr/tests/ReferenceLine/ReferenceLineComponents.tsx (1)
IfOverflowTestWithSegment(5-21)
src/state/referenceElementsSlice.ts (2)
src/cartesian/ReferenceLine.tsx (1)
ReferenceLineSegment(36-45)src/index.ts (1)
ReferenceLineSegment(64-64)
test/cartesian/ReferenceLine/ReferenceLine.spec.tsx (4)
src/state/hooks.ts (1)
useAppSelector(40-50)src/state/selectors/axisSelectors.ts (1)
selectReferenceLinesByAxis(908-915)test/_data.ts (1)
PageData(4-11)src/cartesian/ReferenceLine.tsx (1)
ReferenceLine(305-320)
test/cartesian/ReferenceLine/getEndPoints.spec.tsx (2)
src/cartesian/ReferenceLine.tsx (1)
ReferenceLineSegment(36-45)src/index.ts (1)
ReferenceLineSegment(64-64)
src/cartesian/ReferenceLine.tsx (2)
src/index.ts (1)
ReferenceLineSegment(64-64)src/util/isWellBehavedNumber.ts (1)
isWellBehavedNumber(1-3)
test-vr/tests/ReferenceLine/ReferenceLineComponents.tsx (2)
storybook/stories/data/Page.ts (1)
pageData(259-259)src/cartesian/ReferenceLine.tsx (1)
ReferenceLine(305-320)
src/state/selectors/axisSelectors.ts (4)
src/state/referenceElementsSlice.ts (1)
ReferenceLineSettings(26-30)src/state/selectors/selectTooltipAxisType.ts (1)
XorYType(13-13)src/util/types.ts (1)
NumberDomain(686-686)src/state/selectors/pickAxisType.ts (1)
pickAxisType(3-3)
⏰ 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 (24)
test/state/referenceElementsSlice.spec.ts (1)
67-67: LGTM! Test data aligns with updated type.The explicit
segment: undefinedcorrectly reflects the new optional property inReferenceLineSettings.src/state/referenceElementsSlice.ts (2)
5-5: LGTM! Correct import for the new segment type.
29-29: LGTM! Type extension properly adds segment support.The optional
segmentproperty allows ReferenceLine to work with both traditional coordinate-based definitions and segment-based definitions.test-vr/tests/ReferenceLine/ifOverflow.spec-vr.tsx (1)
1-30: LGTM! Comprehensive VR test coverage for segment-based ReferenceLine.The test suite covers all
ifOverflowvalues including the criticalextendDomainbehavior that fixes the reported issue. Screenshot-based assertions are appropriate for visual regression testing.test/cartesian/ReferenceLine/getEndPoints.spec.tsx (6)
2-2: LGTM! Correct import for explicit type annotations.
359-360: LGTM! Proper documentation of intentional type violation.The
ts-expect-errorcomment correctly indicates that the test is verifying error handling for invalid input (empty segment array).
367-380: LGTM! Explicit type annotation improves test clarity.
384-395: LGTM! Type annotation ensures test data matches API.
398-408: LGTM! Explicit typing for discard overflow test.
411-421: LGTM! Type safety in segment coordinate test.test/cartesian/ReferenceLine/ReferenceLine.spec.tsx (2)
21-21: LGTM! Test data import for segment testing.
521-555: LGTM! Comprehensive state integration test for segment prop.The test verifies that ReferenceLine with a segment is correctly reported to Redux state, including all associated properties. This validates the state layer changes introduced by the PR.
test-vr/tests/ReferenceLine/ReferenceLineComponents.tsx (1)
1-21: LGTM! Well-designed test component for overflow scenarios.The component correctly sets up a ReferenceLine with a segment where the second point (
y: 2000) intentionally falls outside the chart domain, making it ideal for testingifOverflow="extendDomain"behavior. The inline comment clearly documents this design choice.src/index.ts (2)
64-64: LGTM! Public API export for ReferenceLineSegment type.The export enables consumers to use the new segment type when defining ReferenceLine components.
111-111: LGTM! Public API export for IfOverflow type.This export provides type information for the
ifOverflowprop, improving type safety for library consumers.src/state/selectors/axisSelectors.ts (3)
945-965: LGTM! Helper functions correctly extract coordinates from segments.The
extractXCoordinatesandextractYCoordinateshelpers properly handle both traditional coordinate-based (line.x,line.y) and segment-based ReferenceLine definitions. The fallback to segment coordinates enables domain extension whenifOverflow="extendDomain"is used with segments, fixing the reported issue.
967-978: LGTM! Domain calculation now accounts for segment coordinates.The updated
combineLinesDomaincorrectly uses the extraction helpers to compute domains from both traditional and segment-based coordinates.
980-980: LGTM! Selector signature updated to use array input.The array input syntax
[selectReferenceLinesByAxis, pickAxisType]is the correct pattern for createSelector with multiple input selectors.test/cartesian/YAxis/YAxis.spec.tsx (1)
2135-2169: LGTM! Test correctly validates domain extension for segment-based ReferenceLine.The test properly validates that when a ReferenceLine with a segment is used with
ifOverflow="extendDomain", the domain extends to include both segment points. The expectations correctly reflect the extended domain [0, 2000] and corresponding ticks.src/cartesian/ReferenceLine.tsx (5)
24-24: LGTM! Appropriate import for coordinate validation.
36-45: LGTM! Type definition is clear and well-structured.The tuple type correctly represents a segment as exactly two points with optional x/y coordinates in data space. The type aligns with the JSDoc above and supports both numeric and categorical axes.
92-92: LGTM! Property type is consistent with the new ReferenceLineSegment definition.
190-190: LGTM! Changes correctly wire segment data through the component.The function signature update and prop passing ensure segment data flows properly for domain calculation.
Also applies to: 315-315
125-132: Verify whether coordinate validation should apply to custom shapes.The validation guard at lines 125-132 correctly prevents NaN from reaching the default
<line>element. However, this validation pattern is inconsistent with the stated PR goal. When custom shapes (element or function) are provided, validation is bypassed—allowing NaN coordinates to reach custom rendering logic. This same bypass pattern exists across multiple reference components (ReferenceArea, ReferenceDot, Scatter, CartesianGrid), suggesting it's intentional design.This raises a clarification question: Is the bypass intentional (custom shapes own responsibility for validation), or is the validation added in this PR incomplete? If the goal is to prevent NaN from reaching the DOM, coordinate validation should apply to all rendering paths, not just the default shape.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6592 +/- ##
==========================================
+ Coverage 94.53% 94.59% +0.06%
==========================================
Files 491 491
Lines 40761 40793 +32
Branches 4752 4769 +17
==========================================
+ Hits 38532 38587 +55
+ Misses 2224 2201 -23
Partials 5 5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Bundle ReportChanges will increase total bundle size by 2.7kB (0.1%) ⬆️. 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-umdAssets Changed:
view changes for bundle: recharts/bundle-es6Assets Changed:
|
2 participants
Description
I was going through the warnings where ReferenceLine is sometimes passing NaN to DOM and discovered #3379 so I fixed it.
Related Issue
Fixes #3379
How Has This Been Tested?
VR tests
Screenshots (if appropriate):
Types of changes
Checklist:
Summary by CodeRabbit
New Features
Tests