Conversation
WalkthroughAdded Changes
Sequence Diagram(s)sequenceDiagram
participant Consumer as Custom Tooltip Consumer
participant Tooltip as Tooltip Component
participant DefaultContent as DefaultTooltipContent
participant Sorter as Item Sorter
participant Formatter as Formatter Function
Consumer->>Tooltip: Provide payload data and itemSorter
Tooltip->>DefaultContent: Pass TooltipContentProps with payload & itemSorter
DefaultContent->>Sorter: Call itemSorter on each payload item
Sorter-->>DefaultContent: Return sort key (dataKey | value | name | number | string)
DefaultContent->>DefaultContent: Sort payload using lodashLikeSortBy
loop For each sorted payload entry
DefaultContent->>Formatter: Resolve entry.formatter or prop formatter
Formatter-->>DefaultContent: Return formatted [value, name] or single value
DefaultContent->>DefaultContent: Render tooltip item with finalValue/finalName
end
DefaultContent-->>Consumer: Render sorted and formatted tooltip items
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes The changes introduce new types and logic patterns ( Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/component/Tooltip.tsx (1)
55-67: Use@ts-expect-errorcomment instead ofas anycast for React.createElement typing limitation.This TypeScript limitation occurs when narrowing generic function types for
React.createElement. The codebase establishes a pattern for this insrc/component/Customized.tsxusing@ts-expect-errorcomment, which is more explicit than anas anycast:} else if (typeof component === 'function') { // `@ts-expect-error` TS cannot verify that C is FunctionComponent<P> here child = createElement(component, props as any);Apply the same approach to
renderContentfor consistency.
🧹 Nitpick comments (2)
www/src/docs/exampleComponents/AreaChart/PercentAreaChart.tsx (1)
52-68: Improve type safety intoNumberhelper.The
vvariable is declared without a type or initial value, causing potential type ambiguity when passed toparseFloat(). Consider initializing it explicitly and usingelse iffor clearer control flow.♻️ Suggested improvement
const toNumber = (value: TooltipValueType | undefined): number => { if (typeof value === 'number') { return value; } - let v; - if (typeof value === 'string') { + let v: string | undefined; + if (value == null) { + return 0; + } else if (typeof value === 'string') { v = value; - } - if (Array.isArray(value)) { + } else if (Array.isArray(value)) { [, v] = value; } - const parsed = parseFloat(v); + const parsed = parseFloat(v ?? ''); if (!Number.isNaN(parsed)) { return parsed; } return 0; };src/component/DefaultTooltipContent.tsx (1)
97-103: Verify the type assertion suppression is necessary.The
@ts-expect-errorcomment suppresses a type mismatch fromsortBy. Consider whether the types could be improved to avoid suppression, or add a more descriptive comment explaining the specific type issue.💡 Suggested improvement
function lodashLikeSortBy<T>(array: ReadonlyArray<T>, itemSorter: TooltipItemSorter | undefined): ReadonlyArray<T> { if (itemSorter == null) { return array; } - // `@ts-expect-error` sortBy types somehow are returning a number type. + // `@ts-expect-error` es-toolkit/compat sortBy return type is incorrectly narrowed to number[] + // when the iteratee returns number | string | undefined. The actual runtime behavior is correct. return sortBy(array, itemSorter); }
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #6925 +/- ##
==========================================
- Coverage 94.29% 94.26% -0.03%
==========================================
Files 571 571
Lines 56002 56023 +21
Branches 5222 5223 +1
==========================================
+ Hits 52807 52810 +3
- Misses 3186 3204 +18
Partials 9 9 ☔ 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 649 bytes (0.02%) ⬆️. This is within the configured threshold ✅ Detailed changes
Affected Assets, Files, and Routes:view changes for bundle: recharts/bundle-umdAssets Changed:
view changes for bundle: recharts/bundle-cjsAssets Changed:
view changes for bundle: recharts/bundle-es6Assets Changed:
|
ckifer
left a comment
There was a problem hiding this comment.
Hard to tell if this is breaking or not. I suppose if it is then it's a correction
|
conflicts |
a2a018d to
aa5cdb9
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
www/src/docs/exampleComponents/AreaChart/PercentAreaChart.tsx (1)
1-71: Avoid implicitanyintoNumber.
let v;becomesanyunder TS and violates the no-anyrule. Please type it explicitly. As per coding guidelines, avoid implicitany.🐛 Proposed fix
- let v; + let v: number | string | undefined; if (typeof value === 'string') { v = value; } if (Array.isArray(value)) { [, v] = value; } - const parsed = parseFloat(v); + const parsed = typeof v === 'number' ? v : parseFloat(v ?? '');
🤖 Fix all issues with AI agents
In `@src/component/Tooltip.tsx`:
- Line 177: Change the labelFormatter prop signature in Tooltip.tsx from (label:
any, payload: TooltipPayload) => ReactNode to (label: ReactNode, payload:
TooltipPayload) => ReactNode so it matches DefaultTooltipContent.tsx and the
actual prop type; update the import/usage to reference ReactNode (import from
'react' or use React.ReactNode) and adjust any internal calls or tests that pass
a typed label to the labelFormatter to satisfy the new ReactNode parameter type
(identify the prop name labelFormatter and the Tooltip component type definition
to make the change).
In `@test/component/Tooltip/itemSorter.spec.tsx`:
- Line 1: The test file is missing an explicit import of beforeEach from vitest;
update the top import statement (the line that currently imports describe,
expect, it, vi) to also import beforeEach so all usages of beforeEach (e.g., the
beforeEach blocks in this file) are explicitly imported from vitest and avoid
relying on globals.
In `@www/src/docs/exampleComponents/BarChart/PopulationPyramidExample.tsx`:
- Around line 59-61: formatPercent currently calls Number(val) and can produce
NaN when val is an array (TooltipValueType may be ReadonlyArray<number|string>);
update formatPercent to guard for arrays by checking Array.isArray(val) and
extracting a single numeric candidate (e.g., first element) before conversion,
then coerce that candidate to Number (handling string numbers) and fall back to
0 if conversion fails, finally returning `${Math.abs(numberValue).toFixed(1)}%`;
reference the formatPercent function and TooltipValueType/RenderableText when
making the change.
🧹 Nitpick comments (2)
www/src/docs/exampleComponents/ScatterChart/BubbleChart.tsx (1)
79-103: Add an explicit return type forrenderTooltip.This helper isn’t a React component; make the return type explicit to satisfy the TS typing rule. As per coding guidelines, please avoid inferred return types here.
♻️ Proposed change
-const renderTooltip = (props: TooltipContentProps) => { +const renderTooltip = (props: TooltipContentProps): ReactNode => {www/src/docs/exampleComponents/AreaChart/PercentAreaChart.tsx (1)
77-80: Add an explicit return type forrenderTooltipContent.This helper is not a React component; give it an explicit return type. As per coding guidelines, avoid inferred return types for non-component functions.
♻️ Proposed change
-const renderTooltipContent = (o: TooltipContentProps) => { +const renderTooltipContent = (o: TooltipContentProps): JSX.Element => {
| * The formatter function of label in tooltip. | ||
| */ | ||
| labelFormatter?: (label: any, payload: Payload<TValue, TName>[]) => ReactNode; | ||
| labelFormatter?: (label: any, payload: TooltipPayload) => ReactNode; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's find and examine the Tooltip.tsx file
fd -t f "Tooltip.tsx" src/
# Then read the relevant section
cat -n src/component/Tooltip.tsx | sed -n '170,185p'Repository: recharts/recharts
Length of output: 754
🏁 Script executed:
# Search for how labelFormatter is actually used in the codebase
rg "labelFormatter" -A 3 -B 3 src/Repository: recharts/recharts
Length of output: 2158
🏁 Script executed:
# Check what types are typically passed to labelFormatter
rg "labelFormatter\s*=" -A 5 src/ | head -50Repository: recharts/recharts
Length of output: 43
🏁 Script executed:
# Search for labelFormatter usage in examples and tests
rg "labelFormatter\s*:\s*\(" -B 2 -A 5 src/ | head -80Repository: recharts/recharts
Length of output: 43
🏁 Script executed:
# Check if there are any test files showing labelFormatter usage
fd -t f -e "test.ts" -e "spec.ts" -e "test.tsx" src/ | xargs rg "labelFormatter" -A 3 | head -60Repository: recharts/recharts
Length of output: 4213
🏁 Script executed:
# Look at the TooltipPayload type to understand context
rg "type TooltipPayload" -A 10 src/Repository: recharts/recharts
Length of output: 3351
🏁 Script executed:
# Check what type of label is passed to labelFormatter in DefaultTooltipContent
cat -n src/component/DefaultTooltipContent.tsx | sed -n '1,100p' | grep -A 20 "labelFormatter"Repository: recharts/recharts
Length of output: 771
🏁 Script executed:
# Check the actual implementation where labelFormatter is called
cat -n src/component/DefaultTooltipContent.tsx | sed -n '80,120p'Repository: recharts/recharts
Length of output: 1507
🏁 Script executed:
# Look for how label is defined/passed in DefaultTooltipContent
rg "label\s*[:=]" src/component/DefaultTooltipContent.tsx | head -20Repository: recharts/recharts
Length of output: 213
🏁 Script executed:
# Check how Tooltip passes labelFormatter to DefaultTooltipContent
rg "DefaultTooltipContent" src/component/Tooltip.tsx -B 5 -A 5Repository: recharts/recharts
Length of output: 1791
🏁 Script executed:
# Check if there's a reason for the type difference between the two files
git log -p --follow -S "labelFormatter" -- src/component/Tooltip.tsx src/component/DefaultTooltipContent.tsx 2>/dev/null | head -100Repository: recharts/recharts
Length of output: 43
Update labelFormatter parameter type from any to ReactNode for consistency.
The label parameter currently uses any, which violates the TypeScript guidelines. Additionally, DefaultTooltipContent.tsx already types this as ReactNode, and the label value passed to labelFormatter comes directly from the component's label: ReactNode prop. Align Tooltip.tsx with the implementation.
Proposed fix
- labelFormatter?: (label: any, payload: TooltipPayload) => ReactNode;
+ labelFormatter?: (label: ReactNode, payload: TooltipPayload) => ReactNode;📝 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.
| labelFormatter?: (label: any, payload: TooltipPayload) => ReactNode; | |
| labelFormatter?: (label: ReactNode, payload: TooltipPayload) => ReactNode; |
🤖 Prompt for AI Agents
In `@src/component/Tooltip.tsx` at line 177, Change the labelFormatter prop
signature in Tooltip.tsx from (label: any, payload: TooltipPayload) => ReactNode
to (label: ReactNode, payload: TooltipPayload) => ReactNode so it matches
DefaultTooltipContent.tsx and the actual prop type; update the import/usage to
reference ReactNode (import from 'react' or use React.ReactNode) and adjust any
internal calls or tests that pass a typed label to the labelFormatter to satisfy
the new ReactNode parameter type (identify the prop name labelFormatter and the
Tooltip component type definition to make the change).
| @@ -1,4 +1,4 @@ | |||
| import { describe, it, expect, vi } from 'vitest'; | |||
| import { describe, expect, it, vi } from 'vitest'; | |||
There was a problem hiding this comment.
Missing beforeEach import.
The beforeEach function is used throughout this file (lines 42, 911, 964, 1280, 1946, 2337) but is not included in the vitest imports. While vitest may provide it globally, explicit imports are preferred for clarity and to avoid potential issues.
🔧 Suggested fix
-import { describe, expect, it, vi } from 'vitest';
+import { beforeEach, describe, expect, it, vi } from 'vitest';📝 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.
| import { describe, expect, it, vi } from 'vitest'; | |
| import { beforeEach, describe, expect, it, vi } from 'vitest'; |
🤖 Prompt for AI Agents
In `@test/component/Tooltip/itemSorter.spec.tsx` at line 1, The test file is
missing an explicit import of beforeEach from vitest; update the top import
statement (the line that currently imports describe, expect, it, vi) to also
import beforeEach so all usages of beforeEach (e.g., the beforeEach blocks in
this file) are explicitly imported from vitest and avoid relying on globals.
| function formatPercent(val: RenderableText | TooltipValueType): string { | ||
| return `${Math.abs(Number(val)).toFixed(1)}%`; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's look at the file and the formatPercent function
cat -n www/src/docs/exampleComponents/BarChart/PopulationPyramidExample.tsx | head -80 | tail -30Repository: recharts/recharts
Length of output: 1133
🏁 Script executed:
# Search for TooltipValueType definition
rg "type TooltipValueType|interface TooltipValueType" -A 5Repository: recharts/recharts
Length of output: 43
🏁 Script executed:
# Search for RenderableText definition
rg "type RenderableText|interface RenderableText" -A 5Repository: recharts/recharts
Length of output: 446
🏁 Script executed:
# Check imports in the file to see where these types come from
head -30 www/src/docs/exampleComponents/BarChart/PopulationPyramidExample.tsxRepository: recharts/recharts
Length of output: 672
🏁 Script executed:
# Look for all usages of formatPercent in the file
rg "formatPercent" www/src/docs/exampleComponents/BarChart/PopulationPyramidExample.tsx -B 2 -A 2Repository: recharts/recharts
Length of output: 759
🏁 Script executed:
# Search for TooltipValueType definition in recharts source
rg "type TooltipValueType|export.*TooltipValueType" -A 5Repository: recharts/recharts
Length of output: 43
🏁 Script executed:
# Also check the types directory
fd "TooltipValueType|tooltip" -t f src/Repository: recharts/recharts
Length of output: 194
🏁 Script executed:
# Look for tooltip-related type definitions
rg "TooltipValueType" src/ -A 3 | head -50Repository: recharts/recharts
Length of output: 350
🏁 Script executed:
# Find the ValueType definition in DefaultTooltipContent
rg "type ValueType|export.*ValueType" src/component/DefaultTooltipContent.tsx -A 5Repository: recharts/recharts
Length of output: 1083
🏁 Script executed:
# Also check what's exported from that file
cat src/component/DefaultTooltipContent.tsx | head -100Repository: recharts/recharts
Length of output: 3105
🏁 Script executed:
# Let's verify what Number([1,2]) does
python3 << 'EOF'
# JavaScript behavior
results = []
results.append(f"Number([1,2]) behavior:")
# In JavaScript: Number([1,2]) = NaN (single element arrays work, multiple don't)
# Number([1]) = 1
# Number([]) = 0
# Number(['1']) = 1
# Number(['1', '2']) = NaN
# Let's verify this with Node if available
import subprocess
try:
result = subprocess.run(['node', '-e', 'console.log(Number([1,2]), Number([1]), Number([]), Number("1,2"))'],
capture_output=True, text=True, timeout=5)
results.append(f"Node.js output: {result.stdout.strip()}")
except Exception as e:
results.append(f"Could not test: {e}")
for r in results:
print(r)
EOFRepository: recharts/recharts
Length of output: 147
🏁 Script executed:
# Check how the Tooltip actually passes values to the formatter
rg "formatter" src/component/Tooltip.tsx -B 2 -A 2 | head -50Repository: recharts/recharts
Length of output: 408
🏁 Script executed:
# Look for context on how formatters receive their values
rg "formatter.*value" src/component/DefaultTooltipContent.tsx -B 2 -A 2 | head -60Repository: recharts/recharts
Length of output: 43
🏁 Script executed:
# Check if there are other examples in the codebase handling array values in formatters
rg "Array.isArray.*val|is.*Array.*formatter" src/ --type tsx --type ts -B 2 -A 2 | head -40Repository: recharts/recharts
Length of output: 88
🏁 Script executed:
# Look for tooltip examples that might show array handling
fd ".*ooltip.*example" www/src/docs -iRepository: recharts/recharts
Length of output: 43
🏁 Script executed:
# Check what real data gets passed to formatPercent by looking at tooltip usage context
rg "Tooltip" www/src/docs/exampleComponents/BarChart/PopulationPyramidExample.tsx -B 5 -A 5Repository: recharts/recharts
Length of output: 1012
Handle array-valued TooltipValueType to prevent NaN in formatting.
TooltipValueType includes ReadonlyArray<number | string>, but the function doesn't guard against arrays. When Number() is called on multi-element arrays like [1,2], it returns NaN. Add a guard to extract a numeric value from array inputs.
♻️ Proposed change
function formatPercent(val: RenderableText | TooltipValueType): string {
- return `${Math.abs(Number(val)).toFixed(1)}%`;
+ const value = Array.isArray(val) ? val[1] ?? val[0] : val;
+ return `${Math.abs(Number(value)).toFixed(1)}%`;
}🤖 Prompt for AI Agents
In `@www/src/docs/exampleComponents/BarChart/PopulationPyramidExample.tsx` around
lines 59 - 61, formatPercent currently calls Number(val) and can produce NaN
when val is an array (TooltipValueType may be ReadonlyArray<number|string>);
update formatPercent to guard for arrays by checking Array.isArray(val) and
extracting a single numeric candidate (e.g., first element) before conversion,
then coerce that candidate to Number (handling string numbers) and fall back to
0 if conversion fails, finally returning `${Math.abs(numberValue).toFixed(1)}%`;
reference the formatPercent function and TooltipValueType/RenderableText when
making the change.
|
Staging Deployment Details
These deployments will remain available for 30 days. To update snapshots: Comment |
Description
Changed Tooltip.TooltipContentProps.payload from
anytoTooltipPayloadand it cascaded from there.Related Issue
Fixes #6564
Motivation and Context
I tried to keep the strict Tooltip generics but they are mostly a lie - there is zero guarantee that the other graphical elements are actually going to provide those types so it was a false sense of security. So I mostly reverted to the basic
ValueTypeandNameTypewhich are unions of the allowed values.Types of changes
Checklist:
Summary by CodeRabbit
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.