fix: filter textBreakAll from props passed to custom Label content element by supermonkeyguys · Pull Request #7208 · recharts/recharts

supermonkeyguys · 2026-04-03T10:46:27Z

Problem

When Label's content prop is a React element or function, textBreakAll (a recharts-specific prop) was being forwarded via cloneElement / createElement along with other props. If the custom content renders a DOM element (e.g. div), React emits a console warning:

Warning: React does not recognize the `textBreakAll` prop on a DOM element.
If you intentionally want it to appear in the DOM as a custom attribute, spell it as lowercase `textbreakall` instead.

Fixes #3177

Root Cause

In Label.tsx, when building propsWithViewBox, textBreakAll was included. When content is a React element, the code does:

const { labelRef: _, ...propsWithoutLabelRef } = propsWithViewBox;
return cloneElement(content, propsWithoutLabelRef);

Only labelRef was excluded — textBreakAll slipped through and got passed to whatever DOM element the custom content renders.

Fix

Destructure textBreakAll out in both the isValidElement(content) and typeof content === 'function' branches, so it never reaches DOM elements:

// before
const { labelRef: _, ...propsWithoutLabelRef } = propsWithViewBox;

// after
const { labelRef: _, textBreakAll: __, ...propsWithoutLabelRef } = propsWithViewBox;

textBreakAll is already correctly consumed earlier in the component and passed as breakAll to the <Text> component, so this change does not affect any functionality.

Summary by CodeRabbit

Refactor
- Label component prop forwarding clarified so style/formatting props are handled correctly depending on the provided content, avoiding unexpected behavior.
Tests
- New tests added to cover prop forwarding and prevent regressions related to content passed as elements or functions.

…ement When Label's content prop is a React element, textBreakAll (a recharts-specific prop) was being forwarded via cloneElement along with other props. If the custom content renders a DOM element (e.g. div), React warns: Warning: React does not recognize the `textBreakAll` prop on a DOM element. Fix: destructure textBreakAll out before spreading propsWithoutLabelRef and propsForContent, so it never reaches DOM elements. Fixes recharts#3177

coderabbitai · 2026-04-03T10:46:44Z

Walkthrough

When Label renders content as a React element, the cloned props now always omit the internal labelRef. The textBreakAll prop is omitted from cloned props only if the content element’s type is a native DOM/SVG element (typeof content.type === 'string'); for custom components, textBreakAll is preserved. Tests added to verify DOM vs custom-component forwarding and function-content behavior.

Changes

Cohort / File(s)	Summary
Label component `src/component/Label.tsx`	Always omit internal `labelRef` when cloning a `content` React element. Omit `textBreakAll` from cloned props only for native DOM/SVG element types (`typeof content.type === 'string'`); preserve `textBreakAll` for custom components.
Tests for Label `test/component/Label.spec.tsx`	Added tests for regression `#3177`: verify `textBreakAll` is not forwarded to native DOM `content` (avoids React unknown-prop warning), and that it is forwarded to custom component `content` and to function `content` via the first argument. Also added imports for `FC`, `beforeEach`, `afterEach` and console.error spying.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

Fix crash when Label is passed as content to LabelList #6732: Adjusts props forwarded to Label’s content rendering in src/component/Label.tsx, related to controlling which props are propagated to DOM or custom components.

Suggested reviewers

PavelVanecek
ckifer

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and concisely describes the main fix: filtering textBreakAll from props passed to custom Label content elements, which is the primary change in this PR.
Description check	✅ Passed	The description includes Problem, Root Cause, and Fix sections explaining the issue, its cause, and the solution. It provides sufficient context and links to the related issue `#3177`.
Linked Issues check	✅ Passed	The PR successfully addresses the objective from issue `#3177` by preventing textBreakAll from being passed to DOM elements rendered by custom Label content, eliminating the React console warning.
Out of Scope Changes check	✅ Passed	All changes are directly related to fixing issue `#3177`. Code modifications in Label.tsx filter textBreakAll from custom content props, and new tests verify this behavior with no extraneous changes.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests

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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

coderabbitai

Actionable comments posted: 1

🧹 Nitpick comments (1)

src/component/Label.tsx (1)
431-439: Please add regression tests for both updated custom-content branches.

This change touches both cloneElement and createElement paths, so we should lock behavior with tests:

no React unknown-prop warning for DOM-rendering custom content, and

expected behavior for custom component content that needs label wrapping semantics.

As per coding guidelines, "src/**/*.{ts,tsx}: Aim for 100% unit test code coverage when writing new code".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/component/Label.tsx` around lines 431 - 439, Add unit tests for the Label
component to cover both custom-content branches touched: (1) when content is a
React element (cloneElement path) ensure no unknown DOM prop warnings are
emitted for DOM-rendering custom content and that propsWithViewBox filters out
labelRef and textBreakAll before cloning; (2) when content is a
function/component (createElement path) verify the returned element receives
propsForContent and that label wrapping semantics (e.g., positioning,
className/text wrapping) behave as expected. Locate tests to target Label (use
the content, propsWithViewBox, labelRef, textBreakAll symbols) and assert no
React unknown-prop warnings and correct rendered structure/props for both
cloneElement and createElement cases.

🤖 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/Label.tsx`:
- Around line 432-437: The current code unconditionally strips textBreakAll from
propsWithViewBox before passing to cloneElement or invoking content, which
prevents custom renderers from receiving it; update the logic in the Label
component around propsWithViewBox/content handling so textBreakAll is only
removed for intrinsic DOM elements (i.e., when content is a string or a React
element whose type is a string) but is preserved when content is a function or a
custom component, ensuring functions passed to content and custom components
still receive textBreakAll (refer to symbols: propsWithViewBox, content,
cloneElement).

---

Nitpick comments:
In `@src/component/Label.tsx`:
- Around line 431-439: Add unit tests for the Label component to cover both
custom-content branches touched: (1) when content is a React element
(cloneElement path) ensure no unknown DOM prop warnings are emitted for
DOM-rendering custom content and that propsWithViewBox filters out labelRef and
textBreakAll before cloning; (2) when content is a function/component
(createElement path) verify the returned element receives propsForContent and
that label wrapping semantics (e.g., positioning, className/text wrapping)
behave as expected. Locate tests to target Label (use the content,
propsWithViewBox, labelRef, textBreakAll symbols) and assert no React
unknown-prop warnings and correct rendered structure/props for both cloneElement
and createElement cases.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

Push a commit to this branch (recommended)
Create a new PR with the fixes

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 8623489e-15dd-46bc-8b77-8e435d9924f6

📥 Commits

Reviewing files that changed from the base of the PR and between 93ea794 and ee027f9.

📒 Files selected for processing (1)

src/component/Label.tsx

coderabbitai · 2026-04-03T10:51:03Z

+    const { labelRef: _, textBreakAll: __, ...propsWithoutLabelRef } = propsWithViewBox;
    return cloneElement(content, propsWithoutLabelRef);
  }

  if (typeof content === 'function') {
-    const { content: _, ...propsForContent } = propsWithViewBox;
+    const { content: _, textBreakAll: __, ...propsForContent } = propsWithViewBox;


⚠️ Potential issue | 🟠 Major

Potential regression: textBreakAll is now impossible for custom content to consume.

At Line 432 and Line 437, textBreakAll is stripped unconditionally. That fixes DOM warnings, but it also removes this value from all custom renderers (including components that intentionally pass it to nested Label/Text). Given LabelList still provides textBreakAll to Label, this breaks that propagation path for custom-content compositions.

Consider filtering it only for intrinsic DOM targets, or exposing a stable custom-content prop contract that preserves this behavior without leaking unknown props to DOM nodes.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed. In `@src/component/Label.tsx` around lines 432 - 437, The current code unconditionally strips textBreakAll from propsWithViewBox before passing to cloneElement or invoking content, which prevents custom renderers from receiving it; update the logic in the Label component around propsWithViewBox/content handling so textBreakAll is only removed for intrinsic DOM elements (i.e., when content is a string or a React element whose type is a string) but is preserved when content is a function or a custom component, ensuring functions passed to content and custom components still receive textBreakAll (refer to symbols: propsWithViewBox, content, cloneElement).

Per code review feedback: - textBreakAll should only be stripped when content is a React element whose type is a native DOM string (e.g. 'div', 'span'). Custom components should still receive the prop so they can handle it themselves. - content function path: restore textBreakAll forwarding (was incorrectly stripped in the previous commit) - Add regression tests covering: 1. No React unknown-prop warning when content is a native DOM element 2. textBreakAll is preserved when content is a custom React component 3. textBreakAll is forwarded to content functions

supermonkeyguys · 2026-04-03T11:31:06Z

Thanks for the thorough review @coderabbitai!

Addressed both points in the latest commit:

Selective filtering: textBreakAll is now only stripped when content is a React element with a native DOM string type (e.g. 'div', 'span'). Custom components and content functions still receive the prop so they can handle it themselves.
Regression tests: Added three tests covering:
- No React unknown-prop warning when content is a native DOM element
- textBreakAll is preserved and forwarded to custom React component content
- textBreakAll is forwarded to content functions

coderabbitai · 2026-04-03T11:32:13Z

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Check the current Label.tsx implementation
fd -e tsx -e ts "Label" --full-path | grep -v node_modules | grep -v ".d.ts" | head -20

Length of output: 877

🏁 Script executed:

#!/bin/bash
# Check the relevant section of Label.tsx
cat src/component/Label.tsx

Length of output: 16189

🏁 Script executed:

#!/bin/bash
# Check the new/updated test file for the textBreakAll tests
grep -n "textBreakAll\|DOM\|unknown.*prop\|native\|custom.*component\|content.*function" test/component/Label.spec.tsx | head -60

Length of output: 2085

🏁 Script executed:

#!/bin/bash
# Get the full content of the new regression test block
sed -n '685,745p' test/component/Label.spec.tsx

Length of output: 2091

@supermonkeyguys: The updated implementation looks excellent! Here's a quick summary of what I verified:

Label.tsx — selective filtering:
The typeof content.type === 'string' guard is the idiomatic React pattern for distinguishing intrinsic DOM elements from custom components, and the inline comment clearly captures the intent. The content-function branch correctly leaves textBreakAll untouched since only content itself is stripped there.

Regression tests:
All three cases are well-targeted and correct:

Test	What it checks
Native DOM content	`console.error` is not called with `'textBreakAll'`
Custom component content	`textBreakAll: true` is received by the component
Content function	`textBreakAll: true` is included in the call args

The beforeEach/afterEach spy lifecycle is handled properly, and expect.objectContaining in the third test is the right tool for a partial props assertion. This is a clean, minimal fix — great work! 🎉

🧠 Learnings used

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.

If you found this review helpful, would you consider giving us a shout-out on X?

Thank you for using CodeRabbit!

coderabbitai

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@test/component/Label.spec.tsx`:
- Around line 737-740: The current assertion using
expect(contentFn).toHaveBeenCalledWith(...) can give false positives; change the
test to assert the function was called exactly once and that its last call
matches the exact arguments. Replace the call with
expect(contentFn).toHaveBeenCalledTimes(1) and
expect(contentFn).toHaveBeenLastCalledWith({ textBreakAll: true }, {}),
targeting the contentFn invocation and removing the loose
expect.objectContaining matcher so the assertion is strict.
- Around line 706-708: The test's warning detection only checks the first
console.error argument via consoleErrorSpy.mock.calls.find(args =>
String(args[0]).includes('textBreakAll')), which misses format-placeholder
warnings; update the detection (referencing the unknownPropWarning lookup) to
inspect all arguments for the string (e.g., use args.some(a =>
String(a).includes('textBreakAll')) or join all args into one string) so
warnings like console.error('Unknown prop %s', 'textBreakAll') are correctly
matched.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

Push a commit to this branch (recommended)
Create a new PR with the fixes

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: a474f891-95db-4fb4-8b90-8d8889c4f3ec

📥 Commits

Reviewing files that changed from the base of the PR and between ee027f9 and 2d122b6.

📒 Files selected for processing (2)

src/component/Label.tsx
test/component/Label.spec.tsx

🚧 Files skipped from review as they are similar to previous changes (1)

src/component/Label.tsx

coderabbitai · 2026-04-03T11:36:03Z

+      const unknownPropWarning = consoleErrorSpy.mock.calls.find(args =>
+        String(args[0]).includes('textBreakAll'),
+      );


⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🌐 Web query:

In React 18.3.1 development warnings, can console.error messages use format placeholders with substituted values passed in later arguments (not only args[0])?

💡 Result:

Yes, in React 18.3.1 development warnings, console.error messages support format placeholders (%s, %d, etc.) with substituted values passed in later arguments, just like native console.error. React's development build transforms these calls to append component stacks while preserving native formatting behavior, as confirmed by source code analysis and recent changes removing custom stringification that could interfere (PR #29839). Examples from React source like console.error('Invalid attribute ref supplied to React.Fragment.') show simple strings, but the mechanism handles multiple args with placeholders natively.

Citations:

1: Remove Warning: prefix and toString on console Arguments react/react#29839

2: chore[react-devtools]: improve console arguments formatting before passing it to original console react/react#29873

3: Relying on console.log native string substitution interferes with typical patched console logging react/react#16637

4: Bug: Error message prints "Warning: ..." instead of "Error: ..." react/react#20981

5: RFC: Warning System Revamp react/react#16753

6: Remove Component Stack from React Logged Warnings and Error Reporting react/react#30308

7: Use console directly instead of warning() modules react/react#17599

8: React.warn() and React.error() react/react#15170

9: https://developer.mozilla.org/en-US/docs/Web/API/console.warn

🏁 Script executed:

# Inspect the actual code at lines 706-708 and surrounding context sed -n '700,745p' test/component/Label.spec.tsx

Repository: recharts/recharts

Length of output: 1525

🏁 Script executed:

# Search for similar console.error spy patterns in the test file to see if this is an isolated issue rg 'consoleErrorSpy|console\.error' test/component/Label.spec.tsx -n -B 2 -A 2

Repository: recharts/recharts

Length of output: 664

Warning detection check only inspects first argument and can miss React format-placeholder warnings

React 18.3.1 development warnings support format placeholders with substituted values passed in later arguments (e.g., console.error('Unknown prop %s', 'textBreakAll')). This test only checks args[0] and will false-pass if the actual warning contains the value in a later argument.

Proposed fix

- const unknownPropWarning = consoleErrorSpy.mock.calls.find(args => - String(args[0]).includes('textBreakAll'), - ); + const unknownPropWarning = consoleErrorSpy.mock.calls.find(args => + args.some(arg => String(arg).includes('textBreakAll')), + ); expect(unknownPropWarning).toBeUndefined();

📝 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.

Suggested change

const unknownPropWarning = consoleErrorSpy.mock.calls.find(args =>

String(args[0]).includes('textBreakAll'),

);

const unknownPropWarning = consoleErrorSpy.mock.calls.find(args =>

args.some(arg => String(arg).includes('textBreakAll')),

);

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed. In `@test/component/Label.spec.tsx` around lines 706 - 708, The test's warning detection only checks the first console.error argument via consoleErrorSpy.mock.calls.find(args => String(args[0]).includes('textBreakAll')), which misses format-placeholder warnings; update the detection (referencing the unknownPropWarning lookup) to inspect all arguments for the string (e.g., use args.some(a => String(a).includes('textBreakAll')) or join all args into one string) so warnings like console.error('Unknown prop %s', 'textBreakAll') are correctly matched.

coderabbitai · 2026-04-03T11:36:03Z

+      expect(contentFn).toHaveBeenCalledWith(
+        expect.objectContaining({ textBreakAll: true }),
+        {},
+      );


⚠️ Potential issue | 🟡 Minor

Make the content-function assertion strict to avoid false positives

toHaveBeenCalledWith passes if any call matches. For a regression test, assert single invocation and exact last call.

Proposed fix

- expect(contentFn).toHaveBeenCalledWith( + expect(contentFn).toHaveBeenCalledTimes(1); + expect(contentFn).toHaveBeenLastCalledWith( expect.objectContaining({ textBreakAll: true }), {}, );

📝 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.

Suggested change

expect(contentFn).toHaveBeenCalledWith(

expect.objectContaining({ textBreakAll: true }),

{},

);

expect(contentFn).toHaveBeenCalledTimes(1);

expectLastCalledWith(

contentFn,

expect.objectContaining({ textBreakAll: true }),

{},

);

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed. In `@test/component/Label.spec.tsx` around lines 737 - 740, The current assertion using expect(contentFn).toHaveBeenCalledWith(...) can give false positives; change the test to assert the function was called exactly once and that its last call matches the exact arguments. Replace the call with expect(contentFn).toHaveBeenCalledTimes(1) and expect(contentFn).toHaveBeenLastCalledWith({ textBreakAll: true }, {}), targeting the contentFn invocation and removing the loose expect.objectContaining matcher so the assertion is strict.

PavelVanecek · 2026-04-04T03:58:54Z

Same question as https://github.com/recharts/recharts/pull/7069/changes#r2899496695: people who rely on this prop being passed are going to see broken charts?

coderabbitai Bot reviewed Apr 3, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

fix: filter textBreakAll from props passed to custom Label content element#7208

fix: filter textBreakAll from props passed to custom Label content element#7208
supermonkeyguys wants to merge 2 commits into
recharts:mainfrom
supermonkeyguys:fix/label-textbreakall-dom-warning

supermonkeyguys commented Apr 3, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

coderabbitai Bot commented Apr 3, 2026 •

edited

Loading

Uh oh!

coderabbitai Bot left a comment

Uh oh!

coderabbitai Bot Apr 3, 2026

Uh oh!

supermonkeyguys commented Apr 3, 2026

Uh oh!

coderabbitai Bot commented Apr 3, 2026

Uh oh!

coderabbitai Bot left a comment

Uh oh!

coderabbitai Bot Apr 3, 2026

Uh oh!

coderabbitai Bot Apr 3, 2026

Uh oh!

PavelVanecek commented Apr 4, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Uh oh!

Conversation

supermonkeyguys commented Apr 3, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem

Root Cause

Fix

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented Apr 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Estimated code review effort

Possibly related PRs

Suggested reviewers

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

coderabbitai Bot Apr 3, 2026

Choose a reason for hiding this comment

Uh oh!

supermonkeyguys commented Apr 3, 2026

Uh oh!

coderabbitai Bot commented Apr 3, 2026

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

coderabbitai Bot Apr 3, 2026

Choose a reason for hiding this comment

Uh oh!

coderabbitai Bot Apr 3, 2026

Choose a reason for hiding this comment

Uh oh!

PavelVanecek commented Apr 4, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

supermonkeyguys commented Apr 3, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Apr 3, 2026 •

edited

Loading