Conversation
WalkthroughConsolidates tooltip axis selectors from a dedicated module into Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6581 +/- ##
==========================================
- Coverage 91.18% 91.17% -0.01%
==========================================
Files 492 491 -1
Lines 40999 41036 +37
Branches 4584 4585 +1
==========================================
+ Hits 37384 37414 +30
- Misses 3598 3605 +7
Partials 17 17 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
scripts/snapshots/es6Files.txt(1 hunks)scripts/snapshots/libFiles.txt(1 hunks)scripts/snapshots/typesFiles.txt(1 hunks)src/context/useTooltipAxis.ts(1 hunks)src/state/selectors/axisSelectors.ts(1 hunks)src/state/selectors/polarScaleSelectors.ts(1 hunks)src/state/selectors/selectAxisSettings.ts(1 hunks)src/state/selectors/selectTooltipAxis.ts(1 hunks)src/state/selectors/tooltipSelectors.ts(1 hunks)test/cartesian/XAxis.spec.tsx(1 hunks)test/cartesian/YAxis/YAxis.spec.tsx(2 hunks)test/cartesian/YAxis/YAxis.ticks.spec.tsx(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
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/selectors/polarScaleSelectors.tssrc/state/selectors/tooltipSelectors.tssrc/state/selectors/selectAxisSettings.tssrc/state/selectors/selectTooltipAxis.tssrc/state/selectors/axisSelectors.tssrc/context/useTooltipAxis.ts
{test,www/test}/**
📄 CodeRabbit inference engine (DEVELOPING.md)
Place unit tests in the test directory; some tests may also live in www/test
Files:
test/cartesian/YAxis/YAxis.spec.tsxtest/cartesian/XAxis.spec.tsxtest/cartesian/YAxis/YAxis.ticks.spec.tsx
🧠 Learnings (5)
📓 Common learnings
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)
📚 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:
scripts/snapshots/es6Files.txttest/cartesian/YAxis/YAxis.spec.tsxtest/cartesian/XAxis.spec.tsxtest/cartesian/YAxis/YAxis.ticks.spec.tsx
📚 Learning: 2025-10-25T07:36:02.229Z
Learnt from: CR
Repo: recharts/recharts PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-10-25T07:36:02.229Z
Learning: Recharts aims for simple, declarative, and composable charts; prioritize consistency, usability, performance, and accessibility
Applied to files:
src/state/selectors/tooltipSelectors.ts
📚 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:
src/state/selectors/tooltipSelectors.tstest/cartesian/XAxis.spec.tsxtest/cartesian/YAxis/YAxis.ticks.spec.tsxsrc/context/useTooltipAxis.ts
📚 Learning: 2025-10-25T07:35:46.188Z
Learnt from: CR
Repo: recharts/recharts PR: 0
File: test/README.md:0-0
Timestamp: 2025-10-25T07:35:46.188Z
Learning: Applies to test/**/*.test.{ts,tsx} : Prefer using the createSelectorTestCase helper when writing or modifying tests
Applied to files:
test/cartesian/YAxis/YAxis.spec.tsxtest/cartesian/XAxis.spec.tsxtest/cartesian/YAxis/YAxis.ticks.spec.tsx
🧬 Code graph analysis (1)
src/state/selectors/selectAxisSettings.ts (5)
src/state/cartesianAxisSlice.ts (3)
XAxisSettings(74-78)YAxisSettings(82-87)AxisId(8-8)src/state/polarAxisSlice.ts (2)
AngleAxisSettings(7-7)RadiusAxisSettings(5-5)src/state/store.ts (1)
RechartsRootState(94-94)src/state/selectors/axisSelectors.ts (3)
XorYType(101-101)selectXAxisSettings(139-145)selectYAxisSettings(183-189)src/state/selectors/polarAxisSelectors.ts (2)
selectAngleAxis(89-97)selectRadiusAxis(99-107)
⏰ 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 (8)
scripts/snapshots/libFiles.txt (1)
163-163: LGTM!The new module path is correctly added to the build artifacts snapshot in alphabetical order.
scripts/snapshots/typesFiles.txt (1)
163-163: LGTM!The type declaration path is correctly added to the TypeScript artifacts snapshot.
scripts/snapshots/es6Files.txt (1)
163-163: LGTM!The ES6 module path is correctly added, completing the consistent snapshot updates across all three build artifact types (lib, types, es6).
src/state/selectors/polarScaleSelectors.ts (1)
27-27: LGTM!The import path for
selectAxisSettingsis correctly updated to reference the new dedicated module, which helps break the circular dependency. The remaining imports fromaxisSelectorsare appropriately left unchanged.src/state/selectors/selectTooltipAxis.ts (1)
6-6: LGTM!Both
AxisWithTicksSettingstype andselectAxisSettingsfunction are correctly imported from the new centralized module. The refactoring maintains type safety while resolving circular dependencies.test/cartesian/YAxis/YAxis.ticks.spec.tsx (1)
7-9: LGTM!The imports are correctly split between the two modules: axis-specific selectors remain in
axisSelectors, while the centralizedselectAxisSettingsis imported from the new dedicated module. Test logic remains unchanged.src/state/selectors/tooltipSelectors.ts (1)
86-86: LGTM!The
AxisWithTicksSettingstype is correctly imported from the new module. The refactoring appropriately imports only the type needed in this file, while other axis selectors remain imported fromaxisSelectors.test/cartesian/XAxis.spec.tsx (1)
52-52: LGTM!The import for
selectAxisSettingsis correctly updated to reference the new dedicated module. The refactoring maintains test functionality while helping to resolve circular import issues.
| export const selectAxisSettings = ( | ||
| state: RechartsRootState, | ||
| axisType: XorYType, | ||
| axisId: AxisId, | ||
| ): AxisWithTicksSettings => { | ||
| switch (axisType) { | ||
| case 'xAxis': { | ||
| return selectXAxisSettings(state, axisId); | ||
| } | ||
| case 'yAxis': { | ||
| return selectYAxisSettings(state, axisId); | ||
| } | ||
| case 'angleAxis': { | ||
| return selectAngleAxis(state, axisId); | ||
| } | ||
| case 'radiusAxis': { | ||
| return selectRadiusAxis(state, axisId); | ||
| } | ||
| default: | ||
| throw new Error(`Unexpected axis type: ${axisType}`); | ||
| } |
There was a problem hiding this comment.
Circular dependency still present
Relocating selectAxisSettings here doesn’t actually break the problematic cycle: this file pulls selectXAxisSettings/selectYAxisSettings from axisSelectors.ts, while axisSelectors.ts still imports selectTooltipAxis, which now imports this module. The graph is still axisSelectors → selectTooltipAxis → selectAxisSettings → axisSelectors, so Next.js will keep flagging the circular import and the SSR crash remains. Please restructure so the cartesian/polar selector helpers live in a module that has no dependency back into axisSelectors (or keep the implementation inside axisSelectors and avoid re-importing it), eliminating the runtime cycle.
🤖 Prompt for AI Agents
In src/state/selectors/selectAxisSettings.ts around lines 16 to 36, the current
refactor still creates a runtime circular import because this module imports
selectXAxisSettings/selectYAxisSettings from axisSelectors while axisSelectors
imports selectTooltipAxis which imports this module; break the cycle by moving
the cartesian/polar helper selector implementations (selectXAxisSettings,
selectYAxisSettings, selectAngleAxis, selectRadiusAxis) into a new independent
module (e.g., axisHelpers.ts) that does not import axisSelectors, or
alternatively inline the selectAxisSettings logic back inside axisSelectors so
no re-import occurs; update imports so axisSelectors and selectTooltipAxis
import only from the new independent helpers (or use the inlined version) to
ensure there is no import path that returns to axisSelectors.
There was a problem hiding this comment.
@ckifer No worries! Circular dependencies can be sneaky, especially when they span multiple files. 🐰
If you'd like, I can help map out the full dependency chain or verify the fix once you refactor. The two main approaches I suggested should work:
- Extract helpers to an independent module (e.g.,
axisSettingsHelpers.ts) that containsselectXAxisSettings,selectYAxisSettings,selectAngleAxis, andselectRadiusAxiswithout importing fromaxisSelectors - Inline the logic back into
axisSelectors.tsto avoid the re-import
Either way should break the cycle cleanly. Let me know if you need any help working through the refactor!
/)_/)
(='.'=) Hang in there!
(")_(")
Bundle ReportChanges will decrease total bundle size by 717 bytes (-0.03%) ⬇️. This is within the configured threshold ✅ Detailed changes
Affected Assets, Files, and Routes:view changes for bundle: recharts/bundle-es6Assets Changed:
view changes for bundle: recharts/bundle-cjsAssets Changed:
|
cc77b81 to
46548df
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
src/state/selectors/selectors.ts (1)
29-29: Use type-only import for AxisRange to avoid runtime couplingAxisRange is a type; importing it as a value can create unnecessary runtime edges. Split into a type-only import.
-import { AxisRange, selectTooltipAxisDataKey } from './axisSelectors'; +import type { AxisRange } from './axisSelectors'; +import { selectTooltipAxisDataKey } from './axisSelectors';src/context/useTooltipAxis.ts (2)
3-3: Mark AxisWithTicksSettings as type-onlyPrevents unnecessary value import and tight coupling.
-import { AxisWithTicksSettings, selectTooltipAxis } from '../state/selectors/axisSelectors'; +import type { AxisWithTicksSettings } from '../state/selectors/axisSelectors'; +import { selectTooltipAxis } from '../state/selectors/axisSelectors';
8-16: New hook logic is soundGracefully falls back when axis/scale are missing; uses existing helpers. Consider adding a brief JSDoc for public API clarity later.
src/state/selectors/axisSelectors.ts (1)
89-91: Prefer type-only import for unions to minimize runtime edgesThese are types; import them with type-only to avoid accidental value imports.
-import { selectTooltipAxisType, XorYorZType, XorYType } from './selectTooltipAxisType'; +import { selectTooltipAxisType } from './selectTooltipAxisType'; +import type { XorYorZType, XorYType } from './selectTooltipAxisType';
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
scripts/snapshots/es6Files.txt(0 hunks)scripts/snapshots/libFiles.txt(0 hunks)scripts/snapshots/typesFiles.txt(0 hunks)src/context/useTooltipAxis.ts(1 hunks)src/state/selectors/axisSelectors.ts(3 hunks)src/state/selectors/radialBarSelectors.ts(1 hunks)src/state/selectors/selectTooltipAxis.ts(0 hunks)src/state/selectors/selectTooltipAxisType.ts(1 hunks)src/state/selectors/selectors.ts(1 hunks)src/state/selectors/tooltipSelectors.ts(2 hunks)test/component/Tooltip/Tooltip.payload.spec.tsx(1 hunks)test/component/Tooltip/Tooltip.visibility.spec.tsx(1 hunks)test/polar/Pie.spec.tsx(1 hunks)
💤 Files with no reviewable changes (4)
- scripts/snapshots/es6Files.txt
- scripts/snapshots/typesFiles.txt
- scripts/snapshots/libFiles.txt
- src/state/selectors/selectTooltipAxis.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/state/selectors/tooltipSelectors.ts
🧰 Additional context used
📓 Path-based instructions (2)
{test,www/test}/**
📄 CodeRabbit inference engine (DEVELOPING.md)
Place unit tests in the test directory; some tests may also live in www/test
Files:
test/polar/Pie.spec.tsxtest/component/Tooltip/Tooltip.visibility.spec.tsxtest/component/Tooltip/Tooltip.payload.spec.tsx
src/**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Do not hardcode any user-facing strings or formatting choices in the library code; leave internationalization to library users
Files:
src/state/selectors/selectors.tssrc/state/selectors/radialBarSelectors.tssrc/state/selectors/selectTooltipAxisType.tssrc/state/selectors/axisSelectors.tssrc/context/useTooltipAxis.ts
🧠 Learnings (5)
📓 Common learnings
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)
📚 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/polar/Pie.spec.tsxsrc/state/selectors/selectors.tstest/component/Tooltip/Tooltip.visibility.spec.tsxsrc/state/selectors/selectTooltipAxisType.tstest/component/Tooltip/Tooltip.payload.spec.tsxsrc/context/useTooltipAxis.ts
📚 Learning: 2025-10-25T07:35:46.188Z
Learnt from: CR
Repo: recharts/recharts PR: 0
File: test/README.md:0-0
Timestamp: 2025-10-25T07:35:46.188Z
Learning: Applies to test/**/*.test.{ts,tsx} : Prefer using the createSelectorTestCase helper when writing or modifying tests
Applied to files:
test/polar/Pie.spec.tsxtest/component/Tooltip/Tooltip.visibility.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/polar/Pie.spec.tsxtest/component/Tooltip/Tooltip.visibility.spec.tsxtest/component/Tooltip/Tooltip.payload.spec.tsx
📚 Learning: 2025-10-25T07:36:02.229Z
Learnt from: CR
Repo: recharts/recharts PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-10-25T07:36:02.229Z
Learning: Recharts aims for simple, declarative, and composable charts; prioritize consistency, usability, performance, and accessibility
Applied to files:
src/state/selectors/selectTooltipAxisType.ts
🧬 Code graph analysis (1)
src/state/selectors/axisSelectors.ts (4)
src/state/store.ts (1)
RechartsRootState(94-94)src/state/selectors/selectTooltipAxisType.ts (1)
selectTooltipAxisType(15-31)src/state/selectors/selectTooltipAxisId.ts (1)
selectTooltipAxisId(4-4)src/util/types.ts (1)
DataKey(58-58)
⏰ 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 (6)
src/state/selectors/radialBarSelectors.ts (1)
11-11: Consolidated axis imports LGTMPulling BaseAxisWithScale/combineStackGroups/selectTooltipAxis from axisSelectors keeps axis logic centralized and reduces cycles.
test/component/Tooltip/Tooltip.visibility.spec.tsx (1)
91-91: Import consolidation LGTMMatches the new public surface; no further changes needed here.
src/state/selectors/selectTooltipAxisType.ts (1)
4-14: Localizing axis type unions LGTMDefining XorYorZType/XorYType here breaks cycles and keeps axisSelectors lean.
test/component/Tooltip/Tooltip.payload.spec.tsx (1)
77-77: Import refactor verified—no lingering references to removed modulesThe script confirmed that all test files have been updated consistently. Test imports now reference individual selector modules (
selectTooltipAxisId,selectTooltipAxisType) rather than any removed/old module, and the line 77 import toaxisSelectorsaligns with the broader refactor pattern across the codebase.test/polar/Pie.spec.tsx (1)
57-57: Import consolidation verified and correctThe selectTooltipAxis.ts file does not exist, and all references to selectTooltipAxis across the codebase already import from axisSelectors. The change in line 57 is consistent with the rest of the codebase (radialBarSelectors.ts, useTooltipAxis.ts, and all test files). No other files require updates.
src/state/selectors/axisSelectors.ts (1)
586-596: Code changes verified and working correctlyThe new selectors are properly implemented:
selectTooltipAxiscorrectly composesselectTooltipAxisTypeandselectTooltipAxisIdwithout creating circular dependenciesselectTooltipAxisDataKeyis correctly memoized withcreateSelector- Integration confirmed: tooltipSelectors imports and uses
selectTooltipAxisextensively while individual modules remain acyclic- No circular import chains exist in the current structure
|
Looks like our builds got hit by rollup/rollup#6168 |
|
Can we have a new recharts-integ scenario to reproduce this problem and prevent regression? |
|
lol! I thought my PR was bad |
I'm not that sure that 1. this solves the problem (though it is a problem) and that 2. I can reproduce it This is "solving" it blindly. I thought that the no-cycles eslint plugin was supposed to solve this but apparently it doesn't |
|
what this comment is saying could be true but I can't confirm because I can't reproduce either way. If this fixes it then great, but it might not and no clue how to actually repro |
Description
Related Issue
#6578
Motivation and Context
There are circular imports in our axis selectors causing issues for next js users. Unconfirmed if this fixes the issue but its worth doing anyways
How Has This Been Tested?
N/A, green tests, etc.
Not worth testing if it fixes the issue
Types of changes
Checklist:
Summary by CodeRabbit
New Features
useTooltipAxisBandSize()hook to compute the tooltip axis band size based on current axis configuration, ticks, and scale settings.Chores