Skip to content

fix: legend should not report its size when there is an external portal#6609

Merged
ckifer merged 1 commit intomainfrom
fix/legend-margin-external-portal
Nov 18, 2025
Merged

fix: legend should not report its size when there is an external portal#6609
ckifer merged 1 commit intomainfrom
fix/legend-margin-external-portal

Conversation

@ckifer
Copy link
Member

@ckifer ckifer commented Nov 12, 2025 •
edited by coderabbitai bot
Loading

Description

Related Issue

#6608

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • I have added a storybook story or VR test, or extended an existing story or VR test to show my changes

Summary by CodeRabbit

  • Bug Fixes
    • Fixed legend sizing behavior when using portal targets, preventing unnecessary sizing computations in portal scenarios while maintaining existing functionality for standard usage.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Nov 12, 2025 •
edited
Loading

Walkthrough

The Legend component is modified to conditionally render the LegendSizeDispatcher component only when no portal target is provided. Size dispatch computations are now bypassed for portal scenarios while maintaining existing behavior for non-portal usage.

Changes

Cohort / File(s) Summary
Conditional LegendSizeDispatcher rendering
src/component/Legend.tsx		 Modified to guard LegendSizeDispatcher rendering with a check for portalFromProps being falsy, skipping size dispatch when a portal target is configured

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

  • Straightforward conditional rendering optimization affecting a single file
  • Clear intent: avoid sizing computations in portal scenarios
  • Minimal scope and logic density

Pre-merge checks and finishing touches

❌ Failed checks (2 warnings)
Check name Status Explanation Resolution
Description check ⚠️ Warning The PR description is largely incomplete with only a related issue link provided; required sections like Motivation and Context, How Has This Been Tested, and all checklist items remain unfilled. Complete the description by filling in Motivation and Context explaining why the fix is needed, How Has This Been Tested with testing details, select appropriate Types of changes, and check relevant Checklist items.
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (1 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and specifically describes the main change: preventing the legend from reporting its size when an external portal is provided.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch fix/legend-margin-external-portal
📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between e41f3d6 and f1b240d.

📒 Files selected for processing (1)
  • src/component/Legend.tsx (1 hunks)
🔇 Additional comments (1)
src/component/Legend.tsx (1)

202-203: The fix is logically sound, but behavioral changes require test coverage before merging.

The conditional rendering of LegendSizeDispatcher is correct and aligns with the existing portal handling (line 178). When an external portal is provided, the legend renders outside the chart container and should not dispatch size updates to the Redux layout store.

However, verification found that:

  1. No tests exist for setLegendSize action: The legendSlice.spec.ts file only tests payload operations; there are no tests for the size dispatch behavior.

  2. Portal tests don't verify size dispatch: Existing portal tests in Legend.spec.tsx (lines 3015-3072) verify rendering location but don't validate that size dispatch is correctly skipped when a portal is provided.

  3. No mode-switching tests: There are no tests for dynamic portal prop changes (e.g., toggling between portal and non-portal rendering).

Required before merge:

  • Add tests verifying:

    • Size is dispatched to Redux store when no portal is provided
    • Size dispatch is skipped when portal prop is set
    • Cleanup behavior (size reset to {0, 0}) occurs on unmount
    • Mode switching (portal undefined ↔ HTMLElement) works correctly

  • Manually verify the change doesn't cause layout issues when switching between portal/non-portal modes at runtime

Tip

📝 Customizable high-level summaries are now available in beta!

You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.

  • Provide your own instructions using the high_level_summary_instructions setting.
  • Format the summary however you like (bullet lists, tables, multi-section layouts, contributor stats, etc.).
  • Use high_level_summary_in_walkthrough to move the summary from the description to the walkthrough section.

Example instruction:

"Divide the high-level summary into five sections:

  1. 📝 Description — Summarize the main change in 50–60 words, explaining what was done.
  2. 📓 References — List relevant issues, discussions, documentation, or related PRs.
  3. 📦 Dependencies & Requirements — Mention any new/updated dependencies, environment variable changes, or configuration updates.
  4. 📊 Contributor Summary — Include a Markdown table showing contributions:
    | Contributor | Lines Added | Lines Removed | Files Changed |
  5. ✔️ Additional Notes — Add any extra reviewer context.
    Keep each section concise (under 200 words) and use bullet or numbered lists for clarity."

Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later.

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.

@codecov
Copy link

codecov bot commented Nov 12, 2025 •
edited
Loading

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 94.16%. Comparing base (f0260cd) to head (f1b240d).
⚠️ Report is 32 commits behind head on main.

Additional details and impacted files 
@@           Coverage Diff           @@
##             main    #6609   +/-   ##
=======================================
  Coverage   94.16%   94.16%           
=======================================
  Files         493      493           
  Lines       41068    41068           
  Branches     4773     4774    +1     
=======================================
  Hits        38672    38672           
  Misses       2391     2391           
  Partials        5        5

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@codecov
Copy link

codecov bot commented Nov 12, 2025

Bundle Report

Changes will increase total bundle size by 747 bytes (0.03%) ⬆️. This is within the configured threshold ✅

Detailed changes
Bundle name Size Change
recharts/bundle-cjs 1.12MB 316 bytes (0.03%) ⬆️
recharts/bundle-es6 967.01kB 316 bytes (0.03%) ⬆️
recharts/bundle-umd 509.48kB 115 bytes (0.02%) ⬆️

Affected Assets, Files, and Routes:

view changes for bundle: recharts/bundle-es6

Assets Changed:

Asset Name Size Change Total Size Change (%)
component/Legend.js 316 bytes 8.38kB 3.92%
view changes for bundle: recharts/bundle-cjs

Assets Changed:

Asset Name Size Change Total Size Change (%)
component/Legend.js 316 bytes 9.43kB 3.47%
view changes for bundle: recharts/bundle-umd

Assets Changed:

Asset Name Size Change Total Size Change (%)
Recharts.js 115 bytes 509.48kB 0.02%

@ckifer ckifer force-pushed the fix/legend-margin-external-portal branch from 023aa80 to f1b240d Compare November 12, 2025 03:44
@ckifer ckifer marked this pull request as ready for review November 18, 2025 05:51
PavelVanecek
PavelVanecek approved these changes Nov 18, 2025
View reviewed changes
Copy link
Collaborator

@PavelVanecek PavelVanecek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would be nice to mention in the docs I think. Just in case someone decides to portal Legend somewhere inside the chart and then wonder why did the dimensions change.

@ckifer
Copy link
Member Author

ckifer commented Nov 18, 2025

Looks like we're missing portal entirely from the website docs 😓

@ckifer ckifer merged commit 3aad98c into main Nov 18, 2025
27 of 28 checks passed
@ckifer ckifer deleted the fix/legend-margin-external-portal branch November 18, 2025 16:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@PavelVanecek PavelVanecek PavelVanecek approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@ckifer @PavelVanecek