Conversation
WalkthroughAdds an optional Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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: 1
🤖 Fix all issues with AI agents
In `@test/helper/expectLegendLabels.tsx`:
- Line 30: The test helper expectLegendLabels accesses
legend.querySelector('.recharts-legend-item-text').style.color directly which
can throw if querySelector returns null; update the logic in expectLegendLabels
to use optional chaining or a null check when reading .style.color (i.e., read
legend.querySelector('.recharts-legend-item-text')?.style.color or guard the
element before accessing .style.color) so the assertion produces a clear failure
instead of a TypeError.
🧹 Nitpick comments (2)
src/component/DefaultLegendContent.tsx (2)
113-117: JSDoc@defaultValue {}is misleading.The prop's actual default is
undefined(it's not listed indefaultLegendContentDefaultProps). The{}is only the internal fallback computed at line 228. Consider@defaultValue undefinedor removing the@defaultValueline.
228-229: Simplify the guard and considerlabelStyle.color === ''edge case.The
typeof labelStyle === 'object' && labelStyle !== nullcheck can be simplified to a truthiness check sincelabelStyleis typed asCSSProperties | undefined. Also, on line 229 the||operator means acolor: ''inlabelStylewould fall through toentry.color— this is likely fine, but worth being aware of.Suggested simplification
- const finalLabelStyle = typeof labelStyle === 'object' && labelStyle !== null ? { ...labelStyle } : {}; + const finalLabelStyle: CSSProperties = labelStyle ? { ...labelStyle } : {};
|
There's no way to change this with the current API? This seems fine to me but just double checking |
4c6f9ab to
e71c6ac
Compare
|
@devoldemar can you fix the failing unit tests? |
e71c6ac to
982bf8f
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/component/DefaultLegendContent.tsx (1)
123-130: Export name deviates from the establisheddefault[ComponentName]Propsconvention.The analogous export for
DefaultTooltipContent(added at line 38 ofcomponentsAndDefaultPropsMap.ts) is nameddefaultDefaultTooltipContentProps, following the patterndefault + ComponentName + Props. The new export isdefaultLegendContentDefaultProps, which inverts the word order. While not a functional issue, it is inconsistent with the nearest comparable export and will appear in the public API surface consumed byomnidoc.♻️ Proposed rename
-export const defaultLegendContentDefaultProps = { +export const defaultDefaultLegendContentProps = {Also update the import in
omnidoc/componentsAndDefaultPropsMap.ts:-import { defaultLegendContentDefaultProps } from '../src/component/DefaultLegendContent'; +import { defaultDefaultLegendContentProps } from '../src/component/DefaultLegendContent';And the entry:
- DefaultLegendContent: { defaultProps: defaultLegendContentDefaultProps }, + DefaultLegendContent: { defaultProps: defaultDefaultLegendContentProps },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/component/DefaultLegendContent.tsx` around lines 123 - 130, Rename the exported constant defaultLegendContentDefaultProps to follow the established pattern defaultDefaultLegendContentProps so the public API matches other exports (e.g., defaultDefaultTooltipContentProps); update the export name in src/component/DefaultLegendContent.tsx (the constant currently named defaultLegendContentDefaultProps) and update any imports/usages such as the entry in componentsAndDefaultPropsMap.ts to import the new name (and remove or update the old export to avoid a duplicate or broken import).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/component/DefaultLegendContent.tsx`:
- Around line 229-231: Remove the redundant runtime type guard around labelStyle
and instead spread labelStyle directly into finalLabelStyle typed as
CSSProperties (e.g., annotate finalLabelStyle as CSSProperties) to avoid the
union inference; then set finalLabelStyle.color = entry.inactive ? inactiveColor
: (finalLabelStyle.color ?? entry.color) using the nullish coalescing operator
so an explicit empty string in labelStyle.color is preserved; refer to
finalLabelStyle, labelStyle, inactiveColor, and entry.color when making the
change.
In `@test/helper/expectLegendLabels.tsx`:
- Line 30: The expression setting textColor uses
legend.querySelector('.recharts-legend-item-text') and then accesses .style
which TypeScript flags because querySelector returns Element | null; change the
logic in the textColor assignment in expectLegendLabels (use the local variable
legend and the same selector) to first capture the result into a variable, check
that the variable instanceof HTMLElement, and only then read .style.color;
otherwise return undefined—this narrows the type so .style is safe to access.
---
Nitpick comments:
In `@src/component/DefaultLegendContent.tsx`:
- Around line 123-130: Rename the exported constant
defaultLegendContentDefaultProps to follow the established pattern
defaultDefaultLegendContentProps so the public API matches other exports (e.g.,
defaultDefaultTooltipContentProps); update the export name in
src/component/DefaultLegendContent.tsx (the constant currently named
defaultLegendContentDefaultProps) and update any imports/usages such as the
entry in componentsAndDefaultPropsMap.ts to import the new name (and remove or
update the old export to avoid a duplicate or broken import).
test/helper/expectLegendLabels.tsx
Outdated
|
|
||
| const actualLabels = assertHasLegend(container).map(legend => ({ | ||
| textContent: legend.textContent, | ||
| textColor: expectsColor ? legend.querySelector('.recharts-legend-item-text')?.style.color : undefined, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, confirm the file exists and check line 30 context
echo "=== File content around line 30 ==="
sed -n '20,40p' test/helper/expectLegendLabels.tsx
echo -e "\n=== Search for querySelector with recharts-legend-item-text ==="
rg -n "querySelector.*recharts-legend-item-text" test/
echo -e "\n=== Check tsconfig files and their strictness ==="
fd "tsconfig" --type f | grep -E "\.json$" | head -5 | while read f; do
echo "File: $f"
grep -E '"strict"|"noImplicitAny"|"strictNullChecks"' "$f" || echo " (no strict flags found)"
doneRepository: recharts/recharts
Length of output: 1772
🏁 Script executed:
#!/bin/bash
# Find assertHasLegend definition to understand legend type
echo "=== Looking for assertHasLegend definition ==="
rg -n "function assertHasLegend|const assertHasLegend|export.*assertHasLegend" test/
echo -e "\n=== Full definition of assertHasLegend ==="
rg -A 10 "function assertHasLegend|const assertHasLegend" test/helper/expectLegendLabels.tsxRepository: recharts/recharts
Length of output: 713
Add instanceof HTMLElement guard before accessing .style property.
On line 30, legend.querySelector() returns Element | null, and Element does not have a .style property—that's only on HTMLElement. The optional chaining ?. prevents a null-deref crash but does not satisfy TypeScript's strict type checker. Add an instanceof guard to narrow the type:
- textColor: expectsColor ? legend.querySelector('.recharts-legend-item-text')?.style.color : undefined,
+ textColor: expectsColor
+ ? (() => {
+ const el = legend.querySelector('.recharts-legend-item-text');
+ return el instanceof HTMLElement ? el.style.color : undefined;
+ })()
+ : undefined,📝 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.
| textColor: expectsColor ? legend.querySelector('.recharts-legend-item-text')?.style.color : undefined, | |
| textColor: expectsColor | |
| ? (() => { | |
| const el = legend.querySelector('.recharts-legend-item-text'); | |
| return el instanceof HTMLElement ? el.style.color : undefined; | |
| })() | |
| : undefined, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/helper/expectLegendLabels.tsx` at line 30, The expression setting
textColor uses legend.querySelector('.recharts-legend-item-text') and then
accesses .style which TypeScript flags because querySelector returns Element |
null; change the logic in the textColor assignment in expectLegendLabels (use
the local variable legend and the same selector) to first capture the result
into a variable, check that the variable instanceof HTMLElement, and only then
read .style.color; otherwise return undefined—this narrows the type so .style is
safe to access.
982bf8f to
684dedb
Compare
There was a problem hiding this comment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/component/DefaultLegendContent.tsx`:
- Around line 229-230: The branch check on labelStyle is dead because
RequiresDefaultProps ensures labelStyle is always a CSSProperties object; remove
the unnecessary typeof labelStyle === 'object' guard and simply clone labelStyle
into finalLabelStyle, and fix the falsy-color override by using the nullish
coalescing operator (finalLabelStyle.color = entry.inactive ? inactiveColor :
finalLabelStyle.color ?? entry.color) so an explicit empty string or other falsy
but defined color value on labelStyle is not overwritten; update the code that
constructs finalLabelStyle (referencing finalLabelStyle, labelStyle,
entry.color, and inactiveColor) accordingly and remove the unreachable fallback
branch to restore branch coverage.
In `@test/helper/expectLegendLabels.tsx`:
- Line 30: The code accesses .style on
legend.querySelector('.recharts-legend-item-text') which is typed as Element |
null; add an instanceof HTMLElement type guard to narrow the result before
reading .style.color. Locate the expression using
legend.querySelector('.recharts-legend-item-text') in expectLegendLabels.tsx and
replace the direct property access with a guarded expression (e.g., store the
result in a local variable and use if (el instanceof HTMLElement) or a
conditional expression) so textColor returns the element's style.color only when
the query result is an HTMLElement, otherwise undefined.
Fixed. |
684dedb to
0cc7b82
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7012 +/- ##
========================================
Coverage 90.11% 90.12%
========================================
Files 524 526 +2
Lines 39071 39185 +114
Branches 5399 5424 +25
========================================
+ Hits 35209 35314 +105
- Misses 3853 3862 +9
Partials 9 9 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Bundle ReportChanges will decrease total bundle size by 1.88kB (-0.06%) ⬇️. This is within the configured threshold ✅ Detailed changes
ℹ️ *Bundle size includes cached data from a previous commit 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:
|
Description
Customize text style of each legend item like it's made in DefaultTooltipContent.
Motivation and Context
Use of single color for all text labels instead of item colors in Legend. Fit style accordingly to font size and line height in the document.
How Has This Been Tested?
Unit tests added, existing ones passed
Screenshots (if appropriate):
Types of changes
Checklist:
Summary by CodeRabbit
New Features
Documentation
Tests