Conversation
WalkthroughThis PR expands the public TypeScript API surface of Recharts by exporting previously internal types across multiple modules (Sankey, Pie, Treemap, SunburstChart, axis utilities, etc.). It introduces a new test in Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ 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 |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
scripts/verify-exports.test.ts (2)
186-199: Potential undefined access when processing member declarations.At line 189,
member.getDeclarations()[0]could beundefinedif the member has no declarations. WhilegetTypeAtLocationmay handle this gracefully with a fallback tosrcIndex, this could produce unexpected results silently.🔎 Proposed defensive check
const members = symbol.getMembers(); for (const member of members) { - // Only public members const decl = member.getDeclarations()[0]; + if (!decl) { + continue; // Skip members without declarations + } - const memberType = member.getTypeAtLocation(decl || srcIndex); + const memberType = member.getTypeAtLocation(decl); const memberName = member.getName();
40-46: Symbol ID collision risk for declaration-less symbols.When
symbol.getDeclarations()[0]is undefined,getSymbolIdfalls back to just the symbol name. This could cause collisions for different symbols with the same name from different modules, potentially leading to incorrect visitation tracking.🔎 Proposed improvement
function getSymbolId(symbol: Symbol): string { // Name + declaration location is usually unique enough for this purpose const decl = symbol.getDeclarations()[0]; - if (!decl) return symbol.getName(); + if (!decl) { + // Include fully qualified name if available to reduce collision risk + return symbol.getFullyQualifiedName?.() ?? symbol.getName(); + } return `${decl.getSourceFile().getFilePath()}:${decl.getStart()}`; }src/index.ts (1)
110-118: Consider consolidating Sankey exports.The exports are correct, but
SankeyNodeOptionson line 118 could be combined with the other Sankey types on lines 112-117 for consistency.🔎 Proposed consolidation
export type { Props as SankeyProps, NodeProps as SankeyNodeProps, LinkProps as SankeyLinkProps, SankeyData, + SankeyNodeOptions, } from './chart/Sankey'; -export type { SankeyNodeOptions } from './chart/Sankey';
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
.github/workflows/ci.ymlpackage.jsonscripts/verify-exports.test.tssrc/chart/Sankey.tsxsrc/chart/SunburstChart.tsxsrc/chart/Treemap.tsxsrc/index.tssrc/polar/Pie.tsxwww/src/docs/apiExamples/SankeyChart/SankeyCustomNodeExample.tsxwww/src/docs/exampleComponents/LineChart/CompareTwoLines.tsx
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{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/polar/Pie.tsxwww/src/docs/exampleComponents/LineChart/CompareTwoLines.tsxsrc/chart/Treemap.tsxscripts/verify-exports.test.tswww/src/docs/apiExamples/SankeyChart/SankeyCustomNodeExample.tsxsrc/chart/Sankey.tsxsrc/chart/SunburstChart.tsxsrc/index.ts
**/*.{js,ts,tsx}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Ensure code lints by running
npm run lintand follows Airbnb's Style Guide
Files:
src/polar/Pie.tsxwww/src/docs/exampleComponents/LineChart/CompareTwoLines.tsxsrc/chart/Treemap.tsxscripts/verify-exports.test.tswww/src/docs/apiExamples/SankeyChart/SankeyCustomNodeExample.tsxsrc/chart/Sankey.tsxsrc/chart/SunburstChart.tsxsrc/index.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/polar/Pie.tsxsrc/chart/Treemap.tsxsrc/chart/Sankey.tsxsrc/chart/SunburstChart.tsxsrc/index.ts
🧠 Learnings (21)
📓 Common learnings
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.
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.
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.
Learnt from: CR
Repo: recharts/recharts PR: 0
File: DEVELOPING.md:0-0
Timestamp: 2025-12-26T15:59:11.254Z
Learning: For release testing, use `yalc` to test Recharts changes in a test package as close as possible to the results of `npm publish` to ensure no unintended breaking changes are published.
📚 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:
src/polar/Pie.tsxwww/src/docs/exampleComponents/LineChart/CompareTwoLines.tsxsrc/chart/Treemap.tsxscripts/verify-exports.test.tswww/src/docs/apiExamples/SankeyChart/SankeyCustomNodeExample.tsxsrc/chart/SunburstChart.tsxsrc/index.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:
src/polar/Pie.tsxwww/src/docs/exampleComponents/LineChart/CompareTwoLines.tsxsrc/chart/Treemap.tsxscripts/verify-exports.test.tswww/src/docs/apiExamples/SankeyChart/SankeyCustomNodeExample.tsxsrc/chart/Sankey.tsxsrc/chart/SunburstChart.tsxsrc/index.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/Pie.tsxsrc/chart/Treemap.tsxsrc/chart/Sankey.tsxsrc/chart/SunburstChart.tsx
📚 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:
www/src/docs/exampleComponents/LineChart/CompareTwoLines.tsxsrc/chart/Treemap.tsxscripts/verify-exports.test.tswww/src/docs/apiExamples/SankeyChart/SankeyCustomNodeExample.tsxsrc/chart/Sankey.tsxsrc/chart/SunburstChart.tsxsrc/index.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:
www/src/docs/exampleComponents/LineChart/CompareTwoLines.tsxwww/src/docs/apiExamples/SankeyChart/SankeyCustomNodeExample.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:
www/src/docs/exampleComponents/LineChart/CompareTwoLines.tsxwww/src/docs/apiExamples/SankeyChart/SankeyCustomNodeExample.tsxsrc/chart/SunburstChart.tsx
📚 Learning: 2025-12-14T13:58:58.361Z
Learnt from: PavelVanecek
Repo: recharts/recharts PR: 6771
File: src/shape/Curve.tsx:39-40
Timestamp: 2025-12-14T13:58:58.361Z
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:
www/src/docs/exampleComponents/LineChart/CompareTwoLines.tsxwww/src/docs/apiExamples/SankeyChart/SankeyCustomNodeExample.tsx
📚 Learning: 2025-11-16T09:14:24.918Z
Learnt from: PavelVanecek
Repo: recharts/recharts PR: 6640
File: src/cartesian/Bar.tsx:156-159
Timestamp: 2025-11-16T09:14:24.918Z
Learning: In recharts, SSR (Server-Side Rendering) is not yet supported—charts don't render at all in SSR environments. The `isAnimationActive: 'auto'` mode is preparatory work for future SSR support, so testing of this mode should be deferred until SSR support is actually implemented.
Applied to files:
www/src/docs/exampleComponents/LineChart/CompareTwoLines.tsxwww/src/docs/apiExamples/SankeyChart/SankeyCustomNodeExample.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:
package.jsonscripts/verify-exports.test.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: Run `npm run lint` and `npm run check-types` to verify linting and type correctness before submitting changes.
Applied to files:
package.json.github/workflows/ci.yml
📚 Learning: 2025-11-25T01:23:14.977Z
Learnt from: CR
Repo: recharts/recharts PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T01:23:14.977Z
Learning: Applies to **/*.spec.{ts,tsx} : When running unit tests, prefer to run a single test file using `npm run test -- path/to/TestFile.spec.tsx` rather than running all tests with `npm test`
Applied to files:
package.json
📚 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 **/*.spec.{ts,tsx} : Unit tests should be placed in the `test` directory, with some tests also allowed in `www/test`. Test files follow the naming convention `*.spec.tsx`.
Applied to files:
package.json
📚 Learning: 2025-12-25T23:55:31.361Z
Learnt from: PavelVanecek
Repo: recharts/recharts PR: 6824
File: package.json:0-0
Timestamp: 2025-12-25T23:55:31.361Z
Learning: In Recharts, the engines field in package.json should only be changed as part of major version releases to avoid breaking consumers. For development environment Node version requirements, prefer .nvmrc at the repository root to standardize the development environment without affecting published library constraints.
Applied to files:
package.json
📚 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:
scripts/verify-exports.test.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} : Aim for 100% unit test code coverage when writing new code
Applied to files:
scripts/verify-exports.test.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/**/*.spec.{ts,tsx} : Aim for 100% unit test code coverage when writing new code
Applied to files:
scripts/verify-exports.test.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 **/*.{ts,tsx} : Type function parameters and return values explicitly in TypeScript, do not rely on implicit any or inference; exceptions are React components and trivial functions
Applied to files:
www/src/docs/apiExamples/SankeyChart/SankeyCustomNodeExample.tsx
📚 Learning: 2025-12-25T23:55:37.579Z
Learnt from: PavelVanecek
Repo: recharts/recharts PR: 6824
File: package.json:0-0
Timestamp: 2025-12-25T23:55:37.579Z
Learning: In the recharts repository, the `engines` field in package.json specifies requirements for library consumers and should only be changed with major version releases to avoid breaking changes. For development environment Node version requirements, use the `.nvmrc` file instead, which standardizes the development environment without impacting library consumers.
Applied to files:
www/src/docs/apiExamples/SankeyChart/SankeyCustomNodeExample.tsx
📚 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 **/*.{js,ts,tsx} : Ensure code lints by running `npm run lint` and follows Airbnb's Style Guide
Applied to files:
.github/workflows/ci.yml
📚 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: Ensure the test suite passes by running `npm run test` before submitting a pull request
Applied to files:
.github/workflows/ci.yml
🧬 Code graph analysis (4)
www/src/docs/exampleComponents/LineChart/CompareTwoLines.tsx (1)
src/index.ts (1)
MouseHandlerDataParam(168-168)
src/chart/Treemap.tsx (1)
src/index.ts (2)
TreemapContentType(110-110)TreemapNode(110-110)
scripts/verify-exports.test.ts (1)
test/helper/assertNotNull.ts (1)
assertNotNull(1-5)
src/chart/SunburstChart.tsx (1)
storybook/stories/API/props/TextProps.ts (1)
TextProps(3-61)
🪛 Biome (2.1.2)
scripts/verify-exports.test.ts
[error] 1-1: Do not shadow the global "Symbol" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
⏰ 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 (18)
www/src/docs/exampleComponents/LineChart/CompareTwoLines.tsx (1)
2-11: LGTM! Type safety improvements align with public API expansion.The additions properly type the component using the newly exported
MouseHandlerDataParamfrom the public API, along with a well-defined localDataPointinterface. The typing is consistent throughout and improves type safety for the example.Also applies to: 25-35, 45-45, 790-790
src/polar/Pie.tsx (1)
122-122: LGTM! Type exports enhance the public API surface.Exporting
LabelListPropsWithPositionandPieShapeallows consumers to properly type their code when working with Pie component props. These types are part of the component's public interface and should be exported.Also applies to: 185-185
src/chart/Sankey.tsx (2)
606-609: Verify the broadened return type for custom node functions.The function variant of
SankeyNodeOptionsnow returnsReactNodeinstead ofReactElement<SVGProps<SVGRectElement>>. This is more flexible and aligns with standard React patterns, but it's a breaking change for TypeScript consumers who may have relied on the more specific type.While this change is more permissive at runtime (accepts strings, numbers, fragments, etc.), please confirm:
- This broadening is intentional and aligns with actual usage patterns
- Any necessary migration notes are documented for consumers using the function variant
742-742: LGTM! Props type export completes the public API.Exporting the
Propstype allows consumers to properly type Sankey component references and props, aligning with the PR's goal to expose all reachable public types.src/chart/Treemap.tsx (1)
356-356: LGTM! TreemapContentType export completes the public API.Exporting
TreemapContentTypeallows consumers to properly type custom content renderers when working with the Treemap component. This aligns with the PR's goal to expose all reachable public types.src/chart/SunburstChart.tsx (1)
9-9: LGTM! Replacing local type with Text component's public type.Replacing the local
TextOptionsinterface withTextPropsfrom the Text component eliminates duplicate type definitions and establishes a single source of truth. This improves maintainability and ensures consistency with the Text component's API.Also applies to: 84-84
package.json (1)
44-44: LGTM!The new
test-exportsscript follows the existing pattern of running vitest directly on a specific test file, consistent with similar scripts liketest-omnidoc..github/workflows/ci.yml (1)
113-115: LGTM!The new "Verify Public Exports" step is well-positioned after linting and before documentation tests, ensuring the public API is validated early in the pipeline.
www/src/docs/apiExamples/SankeyChart/SankeyCustomNodeExample.tsx (2)
1-1: LGTM!The import correctly uses the public API entry point (
recharts) as required by coding guidelines. This demonstrates the value of the newly exportedSankeyNodePropstype. Based on learnings, this follows the pattern for fixing similar issues like theTooltipIndextype.
22-22: LGTM!Replacing the implicit
anywith the properSankeyNodePropstype improves type safety and aligns with the coding guidelines that prohibit the use ofany. This removes the need for the previousts-expect-errorworkaround.scripts/verify-exports.test.ts (3)
1-1: Static analysis:Symbolshadowing is a false positive.The Biome warning about shadowing the global
Symbolis a false positive in this context. TheSymbolimported fromts-morphis a TypeScript AST node type required for type analysis, not related to the JavaScriptSymbolprimitive. This import is necessary and correct for the ts-morph API.
305-314: BFS iteration with safety cap is well-designed.The
MAX_ITERATIONS = 20000cap prevents infinite loops while the BFS traversal covers type dependencies. The queue-based approach with shift/push correctly implements BFS semantics.
316-322: Test assertions are clear and effective.The two test cases clearly separate concerns: forbidden types (security/encapsulation) vs. missing exports (API completeness). Using
toEqual([])provides clear error messages when violations occur.src/index.ts (5)
19-22: LGTM!Adding
TooltipPayloadEntryalias forPayloadtype makes this commonly-needed type available through the public API, following the established naming convention.
60-68: LGTM!Exporting
LabelListPropsWithPositionandPieShapeenables proper typing of custom Pie chart components through the public API.
125-125: LGTM!Exporting
SunburstDataandSunburstChartPropsenables proper typing for SunburstChart consumers.
134-146: LGTM!The expanded global type exports provide essential utility types (
Coordinate,Margin, etc.) that users need for typed chart customizations.
168-171: LGTM!Exporting
MouseHandlerDataParam,ActiveLabel,AxisId, andAxisRangecompletes the public API surface for chart interaction and axis handling. Based on learnings, this follows the pattern established whenTooltipIndexwas identified as a type that should be exported from the public API.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6852 +/- ##
=======================================
Coverage 93.90% 93.90%
=======================================
Files 531 531
Lines 49429 49429
Branches 5169 5169
=======================================
Hits 46415 46415
Misses 3007 3007
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 |
Bundle ReportChanges will increase total bundle size by 124 bytes (0.0%) ⬆️. This is within the configured threshold ✅ Detailed changes
Affected Assets, Files, and Routes:view changes for bundle: recharts/bundle-umdAssets Changed:
|
Description
Every type that is reachable via the public API should be considered public and exported. Things like:
On top of that, we also never want to export the root state type because that would lock us from ever changing it.
So I got an AI to write a test for it which found a bunch of missing types so I exported them all.
Related Issue
#6291
#3619
#2899
Motivation and Context
What's in
index.tsis public and what is elsewhere is internal.Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.