Isolate Label positioning logic to new file#6829
Conversation
WalkthroughAdds a new cartesian positioning utility (getCartesianPosition), refactors Label to use it, introduces type overloads for cartesianViewBoxToTrapezoid, updates snapshot manifests, removes legacy tests for getAttrsOfCartesianLabel, and adds a new comprehensive test suite for getCartesianPosition. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
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: 2
🧹 Nitpick comments (4)
test/cartesian/useCartesianPosition.spec.ts (2)
554-575: Remove leftover debugging comments.These comments appear to be developer notes from implementation/debugging and should be cleaned up before merging.
🔎 Suggested fix
it('should return correct attributes for position "insideLeft"', () => { const options: GetCartesianPositionOptions = { position: 'insideLeft', offset, viewBox, parentViewBox, clamp: true, }; const actual = getCartesianPosition(options); - // Wait, insideLeft logic clamps width? - // In original: - /* - if (position === 'insideLeft') { - return { - ... - ...sizeAttrs, - }; - } - */ - // sizeAttrs = parentViewBox ? { width: midHeightWidth, height } : {}; - const expected: CartesianPosition = { x: 105, y: 100, horizontalAnchor: 'start', verticalAnchor: 'middle', - width: 200, // midHeightWidth -> (200+200)/2 = 200 + width: 200, height: 100, }; expect(actual).toEqual(expected); });
590-611: Remove leftover debugging comments.Similar to above, these comments should be cleaned up.
🔎 Suggested fix
it('should NOT clamp attributes for position "top"', () => { const options: GetCartesianPositionOptions = { position: 'top', offset, viewBox, parentViewBox, clamp: false, }; const actual = getCartesianPosition(options); - // Height and width should NOT be present if not clamped, ideally, or just standard. - // My implementation returns width/height only if clamped? - // "const sizeAttrs = clamp && parentViewBox ? { width: midHeightWidth, height } : {};" - // For top/bottom/left/right, they return size if clamped inside their blocks. - const expected: CartesianPosition = { x: 200, y: 45, horizontalAnchor: 'middle', verticalAnchor: 'end', }; expect(actual).toEqual(expected); });src/cartesian/CartesianPosition.tsx (2)
71-76: Consider removing implementation notes.These comments appear to be development notes explaining assumptions. If they're meant to stay, they could be condensed.
🔎 Suggested simplification
- // We assume parentViewBox is generic if provided. - // If parentViewBox is Polar, we might ignore it or handle it differently in the future. - // For now, if we are here we assume we are not doing polar positioning logic *inside* this hook, - // but if parentViewBox is passed for clamping, we check if it is generic enough. - // The original code did: `if (parentViewBoxFromProps != null && !isPolar(parentViewBoxFromProps))` - // We will assume `parentViewBox` passed here is relevant for Cartesian. + // parentViewBox is assumed to be Cartesian if provided; polar viewBoxes should be filtered by callers.
264-270: Add a comment documenting the fallthrough positions.Several positions defined in
CartesianLabelPositionfall through to this default case:'inside','outside','insideStart','insideEnd','end','center','centerTop','centerBottom', and'middle'. All resolve to center positioning. Consider adding a comment to document this behavior.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
scripts/snapshots/es6Files.txtscripts/snapshots/libFiles.txtscripts/snapshots/typesFiles.txtsrc/cartesian/CartesianPosition.tsxsrc/component/Label.tsxsrc/context/chartLayoutContext.tsxtest/cartesian/useCartesianPosition.spec.tstest/component/Label.getAttrsOfCartesianLabel.spec.ts
💤 Files with no reviewable changes (1)
- test/component/Label.getAttrsOfCartesianLabel.spec.ts
🧰 Additional context used
📓 Path-based instructions (6)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
**/*.{ts,tsx}: Never useanytype (implicit or explicit) in TypeScript code
Preferunknownoveranyand refine the type in TypeScript
Type function parameters and return values explicitly in TypeScript, do not rely on implicit any or inference; exceptions are React components and trivial functions
Do not useastype assertions in TypeScript; the only exception isas constAll imports from
rechartsmust use the public API entry point (e.g.,import { TooltipIndex } from 'recharts'). Imports from internal paths likerecharts/types/*orrecharts/src/*are not allowed and will fail the linter.
Files:
src/context/chartLayoutContext.tsxtest/cartesian/useCartesianPosition.spec.tssrc/component/Label.tsxsrc/cartesian/CartesianPosition.tsx
**/*.{js,ts,tsx}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Ensure code lints by running
npm run lintand follows Airbnb's Style Guide
Files:
src/context/chartLayoutContext.tsxtest/cartesian/useCartesianPosition.spec.tssrc/component/Label.tsxsrc/cartesian/CartesianPosition.tsx
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/context/chartLayoutContext.tsxsrc/component/Label.tsxsrc/cartesian/CartesianPosition.tsx
test/**/*.spec.{ts,tsx}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Aim for 100% unit test code coverage when writing new code
Files:
test/cartesian/useCartesianPosition.spec.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/cartesian/useCartesianPosition.spec.ts
**/*.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 testUnit tests should be placed in the
testdirectory, with some tests also allowed inwww/test. Test files follow the naming convention*.spec.tsx.
Files:
test/cartesian/useCartesianPosition.spec.ts
🧠 Learnings (6)
📚 Learning: 2025-12-16T08:12:13.355Z
Learnt from: PavelVanecek
Repo: recharts/recharts PR: 6783
File: test/util/ChartUtils.spec.tsx:15-16
Timestamp: 2025-12-16T08:12:13.355Z
Learning: In the recharts codebase, files in the `test` folder are allowed to import from internal paths (e.g., `../../src/state/cartesianAxisSlice`) and do not need to use the public API entry point (`src/index.ts`). The public API import restriction applies only to non-test code.
Applied to files:
scripts/snapshots/es6Files.txtscripts/snapshots/libFiles.txttest/cartesian/useCartesianPosition.spec.tsscripts/snapshots/typesFiles.txtsrc/component/Label.tsx
📚 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/context/chartLayoutContext.tsxsrc/component/Label.tsxsrc/cartesian/CartesianPosition.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/cartesian/useCartesianPosition.spec.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: Add tests when adding new functionality or changing existing functionality; prefer unit tests for helper functions or RTL tests for rendering
Applied to files:
test/cartesian/useCartesianPosition.spec.ts
📚 Learning: 2025-12-26T15:59:11.238Z
Learnt from: CR
Repo: recharts/recharts PR: 0
File: DEVELOPING.md:0-0
Timestamp: 2025-12-26T15:59:11.238Z
Learning: Applies to **/*.{ts,tsx} : All imports from `recharts` must use the public API entry point (e.g., `import { TooltipIndex } from 'recharts'`). Imports from internal paths like `recharts/types/*` or `recharts/src/*` are not allowed and will fail the linter.
Applied to files:
scripts/snapshots/typesFiles.txtsrc/component/Label.tsx
📚 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.txt
🧬 Code graph analysis (2)
src/context/chartLayoutContext.tsx (1)
src/util/types.ts (2)
CartesianViewBoxRequired(1010-1010)TrapezoidViewBox(1024-1052)
src/cartesian/CartesianPosition.tsx (4)
src/util/types.ts (2)
TrapezoidViewBox(1024-1052)CartesianViewBoxRequired(1010-1010)src/component/Text.tsx (2)
TextAnchor(61-61)TextVerticalAnchor(70-70)src/context/chartLayoutContext.tsx (1)
cartesianViewBoxToTrapezoid(24-38)src/util/DataUtils.ts (3)
isNumber(24-25)isPercent(21-22)getPercentValue(51-82)
⏰ 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 (11)
scripts/snapshots/libFiles.txt (1)
19-19: Snapshot entry correctly added.The new
lib/cartesian/CartesianPosition.jsentry is properly placed in alphabetical order within the cartesian section, consistent with the new module being introduced.scripts/snapshots/typesFiles.txt (1)
19-19: Snapshot entry correctly added.The new
types/cartesian/CartesianPosition.d.tsentry is properly placed in alphabetical order, matching the new public type declarations.scripts/snapshots/es6Files.txt (1)
19-19: Snapshot entry correctly added.The new
es6/cartesian/CartesianPosition.jsentry is properly placed in alphabetical order, consistent with the lib and types snapshot updates.src/context/chartLayoutContext.tsx (1)
22-38: Type overloads correctly improve type safety.The function overloads provide precise type narrowing:
- Passing
undefinedreturnsundefined- Passing a valid box returns
TrapezoidViewBoxThis eliminates the need for callers to handle
undefinedwhen they know they're passing a defined value, while preserving safety when the input might be undefined. Implementation correctly handles both cases.src/cartesian/CartesianPosition.tsx (2)
66-69: Clean implementation with proper type safety.The function signature is well-typed with explicit parameter and return types. The call to
cartesianViewBoxToTrapezoid(viewBox)is type-safe because the overload guarantees aTrapezoidViewBoxreturn when the input is defined (which it always is here sinceviewBoxis required).
246-262: Good acknowledgment of known limitation.The TODO comment appropriately documents a known approximation when using percentage-based coordinates on trapezoid shapes. This is transparent about the current behavior.
src/component/Label.tsx (5)
163-167: Good defensive handling of undefined context.The updated logic correctly handles the case when
chartContextis undefined (outside Provider context) by returningundefinedinstead of attempting to convert. This aligns with the codebase pattern where hooks return undefined outside Redux Provider context.
333-335: Type guard correctly updated for undefined handling.The
isPolartype guard now properly handles the case whereviewBoxmight be undefined, returningfalsesafely via theviewBox != nullcheck.
354-370: New helper converts polar viewBox to trapezoid representation.The
polarViewBoxToTrapezoidfunction correctly converts a polar viewBox (circular) to a trapezoid by using the bounding square of the outer circle. The diameter-based dimensions create a square trapezoid representation suitable for Cartesian positioning.
451-471: Refactored Cartesian label positioning using new utility.The integration with
getCartesianPositionis well-implemented:
- Null guard for
cartesianBoxprevents runtime errorsparentViewBoxis filtered throughisPolarto passundefinedfor polar casesclamp: trueenables width/height constraintsThe conditional spread pattern on lines 468-469 works but could be slightly cleaner.
🔎 Optional simplification
positionAttrs = { x: cartesianResult.x, y: cartesianResult.y, textAnchor: cartesianResult.horizontalAnchor, verticalAnchor: cartesianResult.verticalAnchor, - ...(cartesianResult.width !== undefined ? { width: cartesianResult.width } : {}), - ...(cartesianResult.height !== undefined ? { height: cartesianResult.height } : {}), + ...(cartesianResult.width != null && { width: cartesianResult.width }), + ...(cartesianResult.height != null && { height: cartesianResult.height }), };Or keep the explicit pattern for clarity—both are valid.
445-450: Comment indicates future refactoring intent.The TODO comment about "Generic Polar Hook" suggests this area is flagged for future work, which aligns with the PR description mentioning potential reuse for Legend and Tooltip positioning.
| }); | ||
| }); | ||
|
|
||
| describe('in regular Funnel (wide on top, narrow on bottom', () => { |
There was a problem hiding this comment.
Minor typo: missing closing parenthesis in describe block name.
The describe block name is missing a closing parenthesis.
🔎 Suggested fix
- describe('in regular Funnel (wide on top, narrow on bottom', () => {
+ describe('in regular Funnel (wide on top, narrow on bottom)', () => {📝 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.
| describe('in regular Funnel (wide on top, narrow on bottom', () => { | |
| describe('in regular Funnel (wide on top, narrow on bottom)', () => { |
🤖 Prompt for AI Agents
In test/cartesian/useCartesianPosition.spec.ts around line 237, the describe
block title string "in regular Funnel (wide on top, narrow on bottom" is missing
a closing parenthesis; edit that line to add the missing ')' before the closing
quote so the title reads "in regular Funnel (wide on top, narrow on bottom)"
ensuring the string and parentheses are balanced.
| describe('in reversed Funnel (narrow on top, wide on bottom', () => { | ||
| const viewBox: TrapezoidViewBox = { x: 100, y: 50, upperWidth: 150, lowerWidth: 200, width: 175, height: 100 }; | ||
|
|
||
| // ... (rest of tests would be similar, checking a few key ones to be sure) | ||
|
|
||
| it('should return correct attributes for position "insideTop"', () => { | ||
| const options: GetCartesianPositionOptions = { | ||
| position: 'insideTop', | ||
| offset: 0, | ||
| viewBox, | ||
| }; | ||
| const actual = getCartesianPosition(options); | ||
| const expected: CartesianPosition = { | ||
| x: 175, | ||
| y: 50, | ||
| horizontalAnchor: 'middle', | ||
| verticalAnchor: 'start', | ||
| }; | ||
| expect(actual).toEqual(expected); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find CartesianLabelPosition type definition
echo "=== CartesianLabelPosition definition ==="
rg "type CartesianLabelPosition|interface CartesianLabelPosition" src/ -A 10
echo ""
echo "=== Checking test file for position tests ==="
rg "position: '[^']+'" test/cartesian/useCartesianPosition.spec.ts | sort -u | head -30
echo ""
echo "=== Checking line 617 area in test file ==="
sed -n '610,625p' test/cartesian/useCartesianPosition.spec.tsRepository: recharts/recharts
Length of output: 1764
🏁 Script executed:
#!/bin/bash
# Get the full CartesianLabelPosition definition
echo "=== Full CartesianLabelPosition definition ==="
rg "export type CartesianLabelPosition" src/cartesian/CartesianPosition.tsx -A 15
echo ""
echo "=== Check if any of the claimed missing positions appear anywhere in the codebase ==="
for pos in 'insideStart' 'insideEnd' 'end' 'centerTop' 'centerBottom' 'middle'; do
count=$(rg "position.*['\"]$pos['\"]" src/ test/ | wc -l)
echo "$pos: found $count times"
done
echo ""
echo "=== Check what positions are actually tested ==="
rg "position: '[^']+'" test/cartesian/useCartesianPosition.spec.ts | sed "s/.*position: '//" | sed "s/',.*//" | sort -uRepository: recharts/recharts
Length of output: 816
Incomplete test coverage for reversed Funnel — update missing positions list.
The test suite is incomplete (confirmed by the placeholder comment), but the list of missing positions contains inaccuracies. CartesianLabelPosition only defines these positions that are tested:
'top' | 'left' | 'right' | 'bottom' | 'insideLeft' | 'insideRight' | 'insideTop' | 'insideBottom' | 'insideTopLeft' | 'insideBottomLeft' | 'insideTopRight' | 'insideBottomRight'
And these that are actually not tested:
'inside''outside''insideStart'
The positions 'insideEnd', 'end', 'centerTop', 'centerBottom', and 'middle' do not exist in CartesianLabelPosition and should not be listed as missing. Consider completing test coverage for the three actual missing positions per the 100% coverage guideline.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6829 +/- ##
==========================================
- Coverage 93.85% 93.85% -0.01%
==========================================
Files 531 532 +1
Lines 49391 49408 +17
Branches 5163 5166 +3
==========================================
+ Hits 46355 46370 +15
- Misses 3029 3031 +2
Partials 7 7 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Staging Deployment Details
These deployments will remain available for 30 days. To update snapshots: Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
test/cartesian/getCartesianPosition.spec.ts (1)
599-817: Incomplete test coverage for severalCartesianLabelPositionvalues.Per the past review, the test suite doesn't cover these position values defined in
CartesianLabelPosition:'inside','outside', and'insideStart'. Additionally, based on the type definition in the source file, the following positions are also untested:'insideEnd','end','centerTop','centerBottom', and'middle'.Per coding guidelines, aim for 100% unit test code coverage when writing new code.
🧹 Nitpick comments (1)
test/cartesian/getCartesianPosition.spec.ts (1)
9-9: Describe block name doesn't match the function under test.The describe block is named
'useCartesianPosition'but the function being tested isgetCartesianPosition. This naming mismatch could cause confusion.Suggested fix
-describe('useCartesianPosition', () => { +describe('getCartesianPosition', () => {
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
scripts/snapshots/es6Files.txtscripts/snapshots/libFiles.txtscripts/snapshots/typesFiles.txtsrc/cartesian/getCartesianPosition.tsxsrc/component/Label.tsxtest/cartesian/getCartesianPosition.spec.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- scripts/snapshots/es6Files.txt
- scripts/snapshots/typesFiles.txt
🧰 Additional context used
📓 Path-based instructions (6)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
**/*.{ts,tsx}: Never useanytype (implicit or explicit) in TypeScript code
Preferunknownoveranyand refine the type in TypeScript
Type function parameters and return values explicitly in TypeScript, do not rely on implicit any or inference; exceptions are React components and trivial functions
Do not useastype assertions in TypeScript; the only exception isas constAll imports from
rechartsmust use the public API entry point (e.g.,import { TooltipIndex } from 'recharts'). Imports from internal paths likerecharts/types/*orrecharts/src/*are not allowed and will fail the linter.
Files:
src/cartesian/getCartesianPosition.tsxtest/cartesian/getCartesianPosition.spec.tssrc/component/Label.tsx
**/*.{js,ts,tsx}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Ensure code lints by running
npm run lintand follows Airbnb's Style Guide
Files:
src/cartesian/getCartesianPosition.tsxtest/cartesian/getCartesianPosition.spec.tssrc/component/Label.tsx
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/cartesian/getCartesianPosition.tsxsrc/component/Label.tsx
test/**/*.spec.{ts,tsx}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Aim for 100% unit test code coverage when writing new code
Files:
test/cartesian/getCartesianPosition.spec.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/cartesian/getCartesianPosition.spec.ts
**/*.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 testUnit tests should be placed in the
testdirectory, with some tests also allowed inwww/test. Test files follow the naming convention*.spec.tsx.
Files:
test/cartesian/getCartesianPosition.spec.ts
🧠 Learnings (7)
📚 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/cartesian/getCartesianPosition.tsxsrc/component/Label.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/cartesian/getCartesianPosition.spec.ts
📚 Learning: 2025-12-16T08:12:13.355Z
Learnt from: PavelVanecek
Repo: recharts/recharts PR: 6783
File: test/util/ChartUtils.spec.tsx:15-16
Timestamp: 2025-12-16T08:12:13.355Z
Learning: In the recharts codebase, files in the `test` folder are allowed to import from internal paths (e.g., `../../src/state/cartesianAxisSlice`) and do not need to use the public API entry point (`src/index.ts`). The public API import restriction applies only to non-test code.
Applied to files:
test/cartesian/getCartesianPosition.spec.tsscripts/snapshots/libFiles.txtsrc/component/Label.tsx
📚 Learning: 2025-12-26T15:59:11.238Z
Learnt from: CR
Repo: recharts/recharts PR: 0
File: DEVELOPING.md:0-0
Timestamp: 2025-12-26T15:59:11.238Z
Learning: Applies to test-vr/**/*.spec.ts : Visual regression tests should follow the naming convention `*.spec.ts` and be placed in the `test-vr` directory. When updating snapshots, new files created in `test-vr/__snapshots__` should be committed to the repository.
Applied to files:
test/cartesian/getCartesianPosition.spec.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/cartesian/getCartesianPosition.spec.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:
test/cartesian/getCartesianPosition.spec.ts
📚 Learning: 2025-12-26T15:59:11.238Z
Learnt from: CR
Repo: recharts/recharts PR: 0
File: DEVELOPING.md:0-0
Timestamp: 2025-12-26T15:59:11.238Z
Learning: Applies to **/*.{ts,tsx} : All imports from `recharts` must use the public API entry point (e.g., `import { TooltipIndex } from 'recharts'`). Imports from internal paths like `recharts/types/*` or `recharts/src/*` are not allowed and will fail the linter.
Applied to files:
src/component/Label.tsx
🧬 Code graph analysis (3)
src/cartesian/getCartesianPosition.tsx (4)
src/util/types.ts (2)
TrapezoidViewBox(1024-1052)CartesianViewBoxRequired(1010-1010)src/component/Text.tsx (2)
TextAnchor(61-61)TextVerticalAnchor(70-70)src/context/chartLayoutContext.tsx (1)
cartesianViewBoxToTrapezoid(24-38)src/util/DataUtils.ts (3)
isNumber(24-25)isPercent(21-22)getPercentValue(51-82)
test/cartesian/getCartesianPosition.spec.ts (2)
src/util/types.ts (2)
TrapezoidViewBox(1024-1052)CartesianViewBoxRequired(1010-1010)src/cartesian/getCartesianPosition.tsx (3)
GetCartesianPositionOptions(33-49)getCartesianPosition(66-264)CartesianPosition(51-58)
src/component/Label.tsx (4)
src/context/chartLayoutContext.tsx (1)
cartesianViewBoxToTrapezoid(24-38)src/util/types.ts (3)
CartesianViewBoxRequired(1010-1010)TrapezoidViewBox(1024-1052)PolarViewBoxRequired(1022-1022)src/util/DataUtils.ts (1)
isNumber(24-25)src/cartesian/getCartesianPosition.tsx (1)
getCartesianPosition(66-264)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build, Test, Pack
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (7)
scripts/snapshots/libFiles.txt (1)
30-30: LGTM!The new
lib/cartesian/getCartesianPosition.jsentry is correctly placed in alphabetical order within the snapshot manifest.test/cartesian/getCartesianPosition.spec.ts (1)
1-8: LGTM!Test file structure is well-organized with clear test cases across multiple scenarios (rectangle, regular funnel, reversed funnel, negative dimensions, clamping). Imports from internal paths are correctly used per project conventions for test files.
src/cartesian/getCartesianPosition.tsx (2)
60-65: LGTM!Well-documented function with clear JSDoc. The implementation correctly handles:
- Trapezoid geometry calculations for funnel charts
- Sign-based logic for negative dimensions
- Clamping against parent view box
- Multiple position variants with consistent anchor calculations
69-69: No issue here. The functioncartesianViewBoxToTrapezoidhas overloaded signatures: it returnsundefinedonly when passedundefined, and returnsTrapezoidViewBoxwhen passed a required, non-undefined value. SinceviewBoxinGetCartesianPositionOptionsis required (non-optional), TypeScript correctly resolves the return type toTrapezoidViewBox, making the destructuring on line 69 type-safe.src/component/Label.tsx (3)
354-370: LGTM!The
polarViewBoxToTrapezoidhelper cleanly converts polar viewBox to trapezoid format for unified positioning. The diameter-based bounding box calculation is correct for circular polar charts.
333-335: LGTM!The
isPolartype guard is correctly updated to handleundefinedviewBox, improving type safety when determining which positioning logic to use.
463-470: LGTM!Clean mapping from
CartesianPositiontoLabelPositionAttributeswith proper handling of optionalwidthandheightproperties.
| const cartesianResult = getCartesianPosition({ | ||
| viewBox: cartesianBox, | ||
| position, | ||
| offset: props.offset, | ||
| parentViewBox: isPolar(parentViewBox) ? undefined : parentViewBox, | ||
| clamp: true, | ||
| }); |
There was a problem hiding this comment.
Type mismatch: parentViewBox may not satisfy CartesianViewBoxRequired.
getCartesianPosition expects parentViewBox?: CartesianViewBoxRequired (with all properties required), but parentViewBox from props has type ViewBox which includes CartesianViewBox (with optional properties). Passing a CartesianViewBox with undefined x, y, width, or height could cause runtime errors in the clamping logic.
Consider adding validation or type narrowing:
Suggested fix
+const isCartesianViewBoxRequired = (
+ box: ViewBox | undefined,
+): box is CartesianViewBoxRequired =>
+ box != null &&
+ !isPolar(box) &&
+ isNumber(box.x) &&
+ isNumber(box.y) &&
+ isNumber(box.width) &&
+ isNumber(box.height);
const cartesianResult = getCartesianPosition({
viewBox: cartesianBox,
position,
offset: props.offset,
- parentViewBox: isPolar(parentViewBox) ? undefined : parentViewBox,
+ parentViewBox: isCartesianViewBoxRequired(parentViewBox) ? parentViewBox : undefined,
clamp: true,
});🤖 Prompt for AI Agents
In src/component/Label.tsx around lines 455-461, the value passed as
parentViewBox may be a CartesianViewBox with optional x,y,width,height but
getCartesianPosition requires CartesianViewBoxRequired; validate/narrow the prop
before calling: if parentViewBox is polar keep undefined, otherwise check that
x, y, width and height are defined (or coerce defaults) and only pass a
fully-populated CartesianViewBoxRequired; if any required field is missing
either supply sensible defaults or bail out/skip clamping so you never pass an
object with undefined required fields to getCartesianPosition.
Bundle ReportChanges will increase total bundle size by 7.55kB (0.27%) ⬆️. 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:
|
|
Staging Deployment Details
These deployments will remain available for 30 days. To update snapshots: Comment |
|
ah RIP, conflicts :( |
d3814d2 to
6d49545
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/component/Label.tsx (1)
460-466: Type safety issue: parentViewBox may not satisfy CartesianViewBoxRequired.The type mismatch flagged in the previous review remains unresolved.
getCartesianPositionexpectsparentViewBox?: CartesianViewBoxRequired(all properties required), butparentViewBoxfrom props has typeViewBoxwhich includesCartesianViewBoxwith optionalx,y,width, andheight. The current code only filters out polar viewBoxes but doesn't validate that all required Cartesian properties are defined.If a
CartesianViewBoxwith undefined properties is passed, this could cause runtimeNaNerrors in the clamping logic (e.g.,Math.max(y - parentViewBox.y, 0)whenparentViewBox.yis undefined).🔎 Proposed fix
+const isCartesianViewBoxRequired = ( + box: ViewBox | undefined, +): box is CartesianViewBoxRequired => + box != null && + !isPolar(box) && + isNumber(box.x) && + isNumber(box.y) && + isNumber(box.width) && + isNumber(box.height); + const cartesianResult = getCartesianPosition({ viewBox: cartesianBox, position, offset: props.offset, - parentViewBox: isPolar(parentViewBox) ? undefined : parentViewBox, + parentViewBox: isCartesianViewBoxRequired(parentViewBox) ? parentViewBox : undefined, clamp: true, });
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
scripts/snapshots/es6Files.txtscripts/snapshots/libFiles.txtscripts/snapshots/typesFiles.txtsrc/cartesian/getCartesianPosition.tsxsrc/component/Label.tsxsrc/context/chartLayoutContext.tsxtest/cartesian/getCartesianPosition.spec.tstest/component/Label.getAttrsOfCartesianLabel.spec.ts
💤 Files with no reviewable changes (1)
- test/component/Label.getAttrsOfCartesianLabel.spec.ts
✅ Files skipped from review due to trivial changes (1)
- scripts/snapshots/es6Files.txt
🚧 Files skipped from review as they are similar to previous changes (3)
- src/cartesian/getCartesianPosition.tsx
- scripts/snapshots/libFiles.txt
- src/context/chartLayoutContext.tsx
🧰 Additional context used
📓 Path-based instructions (6)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
**/*.{ts,tsx}: Never useanytype (implicit or explicit) in TypeScript code
Preferunknownoveranyand refine the type in TypeScript
Type function parameters and return values explicitly in TypeScript, do not rely on implicit any or inference; exceptions are React components and trivial functions
Do not useastype assertions in TypeScript; the only exception isas constAll imports from
rechartsmust use the public API entry point (e.g.,import { TooltipIndex } from 'recharts'). Imports from internal paths likerecharts/types/*orrecharts/src/*are not allowed and will fail the linter.
Files:
test/cartesian/getCartesianPosition.spec.tssrc/component/Label.tsx
test/**/*.spec.{ts,tsx}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Aim for 100% unit test code coverage when writing new code
Files:
test/cartesian/getCartesianPosition.spec.ts
**/*.{js,ts,tsx}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Ensure code lints by running
npm run lintand follows Airbnb's Style Guide
Files:
test/cartesian/getCartesianPosition.spec.tssrc/component/Label.tsx
test/**/*.{test,spec}.{ts,tsx}
📄 CodeRabbit inference engine (test/README.md)
test/**/*.{test,spec}.{ts,tsx}: Aim for 100% unit test code coverage when writing new code
Prefer to use thecreateSelectorTestCasehelper function when writing or modifying tests
Use theexpectLastCalledWithhelper function instead ofexpect(spy).toHaveBeenLastCalledWith(...)for better typing and autocompletion
Verify the number of selector calls using the spy object fromcreateSelectorTestCaseto spot unnecessary re-renders and improve performance
MockgetBoundingClientRectin tests using the helper function provided intest/helper/MockGetBoundingClientRect.ts
Usevi.useFakeTimers()in all tests due to Redux autoBatchEnhancer dependency on timers andrequestAnimationFrame
Callvi.runOnlyPendingTimers()to advance timers after renders when not usingcreateSelectorTestCasehelper, and avoidvi.runAllTimers()to prevent infinite loops
UseuserEvent.setup({ advanceTimers: vi.runOnlyPendingTimers })or theuserEventSetuphelper function fromtest/helper/userEventSetup.tswhen creating userEvent instances
When testing tooltips on hover, usevi.runOnlyPendingTimers()after eachuserEvent.hover()call or use theshowTooltiphelper function fromtooltipTestHelpers.tsto account for requestAnimationFrame delays
Files:
test/cartesian/getCartesianPosition.spec.ts
**/*.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 testUnit tests should be placed in the
testdirectory, with some tests also allowed inwww/test. Test files follow the naming convention*.spec.tsx.
Files:
test/cartesian/getCartesianPosition.spec.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/component/Label.tsx
🧠 Learnings (7)
📚 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/cartesian/getCartesianPosition.spec.ts
📚 Learning: 2025-12-16T08:12:13.355Z
Learnt from: PavelVanecek
Repo: recharts/recharts PR: 6783
File: test/util/ChartUtils.spec.tsx:15-16
Timestamp: 2025-12-16T08:12:13.355Z
Learning: In the recharts codebase, files in the `test` folder are allowed to import from internal paths (e.g., `../../src/state/cartesianAxisSlice`) and do not need to use the public API entry point (`src/index.ts`). The public API import restriction applies only to non-test code.
Applied to files:
test/cartesian/getCartesianPosition.spec.tsscripts/snapshots/typesFiles.txtsrc/component/Label.tsx
📚 Learning: 2025-12-26T15:59:11.254Z
Learnt from: CR
Repo: recharts/recharts PR: 0
File: DEVELOPING.md:0-0
Timestamp: 2025-12-26T15:59:11.254Z
Learning: Applies to test-vr/**/*.spec.ts : Visual regression tests should follow the naming convention `*.spec.ts` and be placed in the `test-vr` directory. When updating snapshots, new files created in `test-vr/__snapshots__` should be committed to the repository.
Applied to files:
test/cartesian/getCartesianPosition.spec.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/cartesian/getCartesianPosition.spec.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:
test/cartesian/getCartesianPosition.spec.ts
📚 Learning: 2025-12-26T15:59:11.254Z
Learnt from: CR
Repo: recharts/recharts PR: 0
File: DEVELOPING.md:0-0
Timestamp: 2025-12-26T15:59:11.254Z
Learning: Applies to **/*.{ts,tsx} : All imports from `recharts` must use the public API entry point (e.g., `import { TooltipIndex } from 'recharts'`). Imports from internal paths like `recharts/types/*` or `recharts/src/*` are not allowed and will fail the linter.
Applied to files:
scripts/snapshots/typesFiles.txtsrc/component/Label.tsx
📚 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/component/Label.tsx
🧬 Code graph analysis (2)
test/cartesian/getCartesianPosition.spec.ts (2)
src/util/types.ts (2)
TrapezoidViewBox(1028-1056)CartesianViewBoxRequired(1014-1014)src/cartesian/getCartesianPosition.tsx (3)
GetCartesianPositionOptions(33-49)getCartesianPosition(66-264)CartesianPosition(51-58)
src/component/Label.tsx (4)
src/context/chartLayoutContext.tsx (1)
cartesianViewBoxToTrapezoid(24-38)src/util/types.ts (3)
CartesianViewBoxRequired(1014-1014)TrapezoidViewBox(1028-1056)PolarViewBoxRequired(1026-1026)src/util/DataUtils.ts (1)
isNumber(24-25)src/cartesian/getCartesianPosition.tsx (1)
getCartesianPosition(66-264)
⏰ 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 (5)
scripts/snapshots/typesFiles.txt (1)
30-30: LGTM!The new type declaration file is correctly added to the public types snapshot and properly sorted alphabetically within the cartesian section.
src/component/Label.tsx (3)
15-26: LGTM!The import changes correctly reflect the refactoring:
getPercentValuehas been removed since it's now used internally bygetCartesianPosition, and the new imports forCartesianLabelPositionandgetCartesianPositionhave been properly added.
359-375: LGTM!The new
polarViewBoxToTrapezoidhelper correctly converts polar viewBoxes to trapezoid format by calculating the bounding rectangle. The logic properly handles the conversion:x = cx - outerRadius,y = cy - outerRadius, and sets all dimensions todiameter = outerRadius * 2.
468-475: LGTM!The mapping from
getCartesianPositionresult topositionAttrsis well-structured. The conditional spreading of optionalwidthandheightproperties correctly handles cases where clamping is enabled versus disabled.test/cartesian/getCartesianPosition.spec.ts (1)
1-818: Add tests for allCartesianLabelPositionvalues to achieve 100% coverage.The test suite covers 13 explicit position branches and object positions, but the
CartesianLabelPositiontype defines 22 possible values. The following 8 positions are missing test coverage:'inside','outside','insideStart','insideEnd','end','centerTop','centerBottom', and'middle'. These positions are not explicitly handled in the implementation and currently fall through to the default case (which returns center coordinates). Add test cases for each to document this behavior explicitly, or if these positions are not intended to be supported, remove them from theCartesianLabelPositiontype definition.⛔ Skipped due to learnings
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} : Aim for 100% unit test code coverage when writing new codeLearnt from: CR Repo: recharts/recharts PR: 0 File: CONTRIBUTING.md:0-0 Timestamp: 2025-11-25T01:22:59.729Z Learning: Applies to test/**/*.spec.{ts,tsx} : Aim for 100% unit test code coverage when writing new code
| } from '../../src/cartesian/getCartesianPosition'; | ||
| import { CartesianViewBoxRequired, TrapezoidViewBox } from '../../src/util/types'; | ||
|
|
||
| describe('useCartesianPosition', () => { |
There was a problem hiding this comment.
Fix describe block name to match function under test.
The describe block is named 'useCartesianPosition' but the function being tested is getCartesianPosition (not a hook). The name should match the function to avoid confusion.
🔎 Proposed fix
-describe('useCartesianPosition', () => {
+describe('getCartesianPosition', () => {📝 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.
| describe('useCartesianPosition', () => { | |
| describe('getCartesianPosition', () => { |
🤖 Prompt for AI Agents
In test/cartesian/getCartesianPosition.spec.ts around line 9, the describe block
is incorrectly named 'useCartesianPosition' while the test targets the
getCartesianPosition function; rename the describe string to
'getCartesianPosition' (or another exact name matching the exported function
under test) so the test suite accurately reflects the unit under test and avoid
implying it's a hook.
|
Staging Deployment Details
These deployments will remain available for 30 days. To update snapshots: Comment |
2 participants
Description
Non-functional refactoring. Move function from one file to another.
Related Issue
#5055
#2541
#488
#6643
#807
#5115
Motivation and Context
I like the API for Label positioning and I plan to reuse it for Legend and Tooltip positions in the future. For now only refactor though, no new features.
Types of changes
Checklist:
Summary by CodeRabbit
New Features
Refactor
Tests
✏️ Tip: You can customize this high-level summary in your review settings.