Skip to content

Fix/sankey unique key error#6352

Merged
ckifer merged 2 commits intorecharts:mainfrom
daiboom:fix/sankey-unique-key-error
Sep 20, 2025
Merged

Fix/sankey unique key error#6352
ckifer merged 2 commits intorecharts:mainfrom
daiboom:fix/sankey-unique-key-error

Conversation

@daiboom
Copy link
Contributor

@daiboom daiboom commented Sep 20, 2025

Description

Standardized Sankey node key generation to follow the same pattern as link keys for consistency and improved code maintainability. Updated node key to use data-attribute-based pattern node-${index}-${x}-${y} format, aligning with the existing link key pattern link-${source}-${target}-${value}.

Related Issue

#6322

Motivation and Context

The Sankey component had inconsistent key generation patterns between nodes and links. Links were using meaningful data attributes (source, target, value) while nodes were using basic array indices. This inconsistency made the codebase harder to maintain and could potentially cause React rendering issues. By aligning both components to use similar data-driven key generation approaches, we improve code consistency, maintainability, and ensure stable component identification across re-renders.

How Has This Been Tested?

  • Ran npm run test - all existing tests pass ✅
  • Ran npm run lint - no linting errors ✅
  • Verified React console warnings about unique keys are resolved
  • Tested with various Sankey chart configurations to ensure key uniqueness
  • Confirmed chart functionality remains unchanged (no visual or behavioral regressions)
  • Tested with different node counts and data structures to ensure key stability
  • Verified proper rendering during data updates and re-renders

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

@codecov
Copy link

codecov bot commented Sep 20, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 96.65%. Comparing base (e08883a) to head (07dec09).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #6352   +/-   ##
=======================================
  Coverage   96.65%   96.65%           
=======================================
  Files         221      221           
  Lines       20345    20346    +1     
  Branches     4171     4171           
=======================================
+ Hits        19665    19666    +1     
  Misses        673      673           
  Partials        7        7           

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

Copy link
Member

@ckifer ckifer left a comment

Choose a reason for hiding this comment

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

LGTM, thanks.

Still not used to AI writing PR descriptions. It's almost too much to read for a change so small 😅

@ckifer
Copy link
Member

ckifer commented Sep 20, 2025

Thanks!

@ckifer ckifer merged commit 640fe51 into recharts:main Sep 20, 2025
20 checks passed
@PavelVanecek
Copy link
Collaborator

I wonder how difficult is it to write a unit test for these warnings? Or how else to turn the warnings into errors?

@ckifer
Copy link
Member

ckifer commented Sep 20, 2025

I've tried in the past without success but not sure if that's changed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants