fix(Pie): skip minAngle redistribution when no segment needs it#7097
Conversation
When all segments already have angles above the minAngle threshold, the redistribution logic was still kicking in and shifting every segment's angle unnecessarily. Moved sum calculation before realTotalAngle so we can check each segment's natural angle first. If none fall below minAngle, we set effectiveMinAngle to 0 and skip the whole redistribution. Fixes recharts#6814
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughComputePieSectors now only redistributes the Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
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🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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
🧹 Nitpick comments (1)
src/polar/Pie.tsx (1)
755-770: Please add a regression test forpaddingAngle+ conditionalminAnglebehavior.This change is important and subtle; add a focused case where
percent * absDeltaAngle >= minAnglebutpercent * (absDeltaAngle - totalPaddingAngle) < minAngleto protect against future regressions.As per coding guidelines "
src/**/*.{ts,tsx}: Aim for 100% unit test code coverage when writing new code".Also applies to: 795-795
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/polar/Pie.tsx` around lines 755 - 770, Add a regression unit test that verifies paddingAngle + the conditional minAngle logic in Pie.tsx: create displayedData with an entry whose percent satisfies percent * absDeltaAngle >= minAngle but percent * (absDeltaAngle - totalPaddingAngle) < minAngle, then assert that needsMinAngleAdjustment (the minAngle decision using displayedData, getValueByDataKey, dataKey, absDeltaAngle, totalPaddingAngle) is true and that effectiveMinAngle is applied only when appropriate so sectors layout uses the adjusted realTotalAngle; place the test alongside other Pie tests to ensure this edge case (paddingAngle reducing available angle below minAngle threshold) cannot regress.
🤖 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/polar/Pie.tsx`:
- Around line 759-766: The min-angle trigger uses percent * absDeltaAngle but
must use the angle budget after padding; inside the needsMinAngleAdjustment
predicate (the displayedData.some callback that calls getValueByDataKey with
dataKey), compute an effectiveAngle = Math.max(0, absDeltaAngle - (paddingAngle
|| 0) * displayedData.length) (or subtract per-sector padding if a different
convention is used) and replace percent * absDeltaAngle with percent *
effectiveAngle so the check accounts for paddingAngle when deciding
redistribution.
---
Nitpick comments:
In `@src/polar/Pie.tsx`:
- Around line 755-770: Add a regression unit test that verifies paddingAngle +
the conditional minAngle logic in Pie.tsx: create displayedData with an entry
whose percent satisfies percent * absDeltaAngle >= minAngle but percent *
(absDeltaAngle - totalPaddingAngle) < minAngle, then assert that
needsMinAngleAdjustment (the minAngle decision using displayedData,
getValueByDataKey, dataKey, absDeltaAngle, totalPaddingAngle) is true and that
effectiveMinAngle is applied only when appropriate so sectors layout uses the
adjusted realTotalAngle; place the test alongside other Pie tests to ensure this
edge case (paddingAngle reducing available angle below minAngle threshold)
cannot regress.
| const needsMinAngleAdjustment = | ||
| minAngle > 0 && | ||
| sum > 0 && | ||
| displayedData.some(entry => { | ||
| const val = getValueByDataKey(entry, dataKey, 0); | ||
| const percent = (isNumber(val) ? val : 0) / sum; | ||
| return val !== 0 && percent * absDeltaAngle < minAngle; | ||
| }); |
There was a problem hiding this comment.
Use post-padding natural angle in the minAngle trigger check.
On Line 764-Line 765, the check uses percent * absDeltaAngle, but actual sector allocation is based on angle budget after padding. With non-zero paddingAngle, this can incorrectly skip redistribution even when a sector ends up below minAngle.
Suggested fix
const needsMinAngleAdjustment =
minAngle > 0 &&
sum > 0 &&
displayedData.some(entry => {
const val = getValueByDataKey(entry, dataKey, 0);
- const percent = (isNumber(val) ? val : 0) / sum;
- return val !== 0 && percent * absDeltaAngle < minAngle;
+ const numericVal = isNumber(val) ? val : 0;
+ const percent = numericVal / sum;
+ const naturalAngle = percent * (absDeltaAngle - totalPaddingAngle);
+ return numericVal !== 0 && naturalAngle < minAngle;
});📝 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.
| const needsMinAngleAdjustment = | |
| minAngle > 0 && | |
| sum > 0 && | |
| displayedData.some(entry => { | |
| const val = getValueByDataKey(entry, dataKey, 0); | |
| const percent = (isNumber(val) ? val : 0) / sum; | |
| return val !== 0 && percent * absDeltaAngle < minAngle; | |
| }); | |
| const needsMinAngleAdjustment = | |
| minAngle > 0 && | |
| sum > 0 && | |
| displayedData.some(entry => { | |
| const val = getValueByDataKey(entry, dataKey, 0); | |
| const numericVal = isNumber(val) ? val : 0; | |
| const percent = numericVal / sum; | |
| const naturalAngle = percent * (absDeltaAngle - totalPaddingAngle); | |
| return numericVal !== 0 && naturalAngle < minAngle; | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/polar/Pie.tsx` around lines 759 - 766, The min-angle trigger uses percent
* absDeltaAngle but must use the angle budget after padding; inside the
needsMinAngleAdjustment predicate (the displayedData.some callback that calls
getValueByDataKey with dataKey), compute an effectiveAngle = Math.max(0,
absDeltaAngle - (paddingAngle || 0) * displayedData.length) (or subtract
per-sector padding if a different convention is used) and replace percent *
absDeltaAngle with percent * effectiveAngle so the check accounts for
paddingAngle when deciding redistribution.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7097 +/- ##
==========================================
+ Coverage 89.58% 89.61% +0.03%
==========================================
Files 535 534 -1
Lines 40188 40294 +106
Branches 5464 5490 +26
==========================================
+ Hits 36001 36109 +108
+ Misses 4178 4177 -1
+ Partials 9 8 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
We're going to need a test for this |
…ve threshold Covers the case from recharts#6814 where all segments naturally have angles above minAngle, so redistribution should not happen.
|
Done @PavelVanecek , I added a regression test in the latest commit. |
Fixes #6814
What's the problem?
Right now
minAnglealways reserves angle space for every non-zero segment and redistributes proportionally — even when all segments are already above the threshold. So with data like[300, 30, 20, 10]andminAngle={9}, you get[279°, 36°, 27°, 18°]instead of the natural[300°, 30°, 20°, 10°].As @gregalexsmith showed in the issue, the angles shift for no reason.
What does this fix do?
Before doing the redistribution, I check whether any non-zero segment's natural angle actually falls below
minAngle. If none do, we skip the redistribution entirely (seteffectiveMinAngle = 0).Had to move the
sumcalculation up a few lines so it's available before we computerealTotalAngle— we needsumto figure out each segment's natural angle.How it works
When
minAngleIS needed (some segment is too small), behavior is exactly the same as before.Testing
Types of changes
Summary by CodeRabbit
Bug Fixes
Tests