Add verticalAlign prop for Sankey by dbnl-kat · Pull Request #6576 · recharts/recharts

dbnl-kat · 2025-11-06T19:37:21Z

Description

Adding support for top vertical alignment of nodes in Sankey

Related Issue

Motivation and Context

It's helpful in some use cases like in representing sequential processes like user journeys to be able to have nodes "stick" to the top so that the vertical position follows the natural reading order.

How Has This Been Tested?

Storybook

Screenshots (if appropriate):

verticalAlign justify

verticalAlign top

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

New Features
- Sankey charts now support vertical alignment control. Choose 'justify' (default) for even spacing or 'top' to stack nodes from the top edge.
Documentation
- API docs updated to include the optional verticalAlign prop and its behavior.
Tests
- Added visual tests validating Sankey rendering for both 'justify' and 'top' alignments.

coderabbitai · 2025-11-06T19:37:34Z

Walkthrough

Adds a verticalAlign prop (type SankeyVerticalAlign = 'justify' | 'top') to the Sankey chart, threads it through SankeyImpl, computeData, and updateYOfTree, conditions relaxation passes on the prop, updates defaults and memo/dependency wiring, adds docs and two visual-regression tests.

Changes

Cohort / File(s)	Summary
Core Sankey implementation `src/chart/Sankey.tsx`	Add `SankeyVerticalAlign` type and optional `verticalAlign` prop on `SankeyProps`; include `verticalAlign: 'justify'` in `sankeyDefaultProps`; thread `verticalAlign` through `SankeyImpl`, `computeData` calls, and memo/effect dependency arrays.
Layout logic (same file) `src/chart/Sankey.tsx`	Extend `updateYOfTree(..., verticalAlign)` to compute node `y`/`dy` differently for `'top'` (stack from top using `currentY`) vs `'justify'` (even distribution); update `computeData(..., verticalAlign)` to pass the prop and only run relaxation loops when `verticalAlign === 'justify'`.
Visual regression tests `test-vr/tests/SankeyChart.spec-vr.tsx`	Add two tests asserting Sankey renderings for `verticalAlign: 'justify'` and `verticalAlign: 'top'`.
Docs / API `www/src/docs/api/Sankey.ts`	Add `verticalAlign` API entry (type `"'justify'

Sequence Diagram(s)

sequenceDiagram
    participant Sankey as Sankey Component
    participant computeData
    participant updateYOfTree
    participant Relax as Relaxation Loop

    Sankey->>computeData: computeData(data, ..., verticalAlign)
    computeData->>updateYOfTree: updateYOfTree(depthTree, ..., verticalAlign)

    alt verticalAlign == "justify"
        updateYOfTree->>updateYOfTree: distribute nodes evenly within depth
        computeData->>Relax: perform iterative relaxation passes (R->L, L->R)
        Relax->>Relax: adjust node y positions and link offsets
    else verticalAlign == "top"
        updateYOfTree->>updateYOfTree: stack nodes from top using currentY progression
        Note right of Relax: relaxation loop skipped
    end

    computeData-->>Sankey: return positioned nodes and links

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Check propagation of verticalAlign through SankeyImpl → computeData → updateYOfTree.
Validate top-aligned stacking math (running currentY, dy/y assignments) and that skipping relaxation is safe.
Confirm memo/dependency updates include verticalAlign to trigger re-layout.

Possibly related issues

Add ability to vertically top align nodes in Sankey #6575 — Implements Sankey vertical alignment prop to support top-aligned nodes; this PR implements the same feature and likely addresses the issue's objective.

Possibly related PRs

Add support for left-aligned Sankey nodes #6568 — Threads an alignment prop through the same Sankey layout pipeline; closely related at code-level (similar prop semantics).
Rename SankeyChart doc to Sankey #6560 — Touches www/src/docs/api/Sankey.ts; may overlap with the documented API addition.

Suggested reviewers

PavelVanecek
ckifer

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
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 (2 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title 'Add verticalAlign prop for Sankey' directly and clearly describes the main change: introducing a new verticalAlign prop to the Sankey component.
Description check	✅ Passed	The description covers all major template sections including description, related issue, motivation, testing approach, screenshots, types of changes, and completed checklist items.

✨ Finishing touches

📝 Generate docstrings

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment

📜 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 4c83929 and 2994094.

📒 Files selected for processing (1)

src/chart/Sankey.tsx (9 hunks)

🧰 Additional context used

📓 Path-based instructions (1)

src/**/*.{js,jsx,ts,tsx}

📄 CodeRabbit inference engine (AGENTS.md)

Do not hardcode any user-facing strings or formatting choices in the library code; leave internationalization to library users

Files:

src/chart/Sankey.tsx

🧬 Code graph analysis (1)

src/chart/Sankey.tsx (1)

src/util/types.ts (1)

SankeyNode (915-929)

🔇 Additional comments (6)

src/chart/Sankey.tsx (6)

169-169: LGTM! Clean type definition.

The SankeyVerticalAlign union type is clear and follows TypeScript best practices.

171-201: LGTM! Conditional alignment logic is well-implemented.

The function correctly handles both alignment modes:

'top': Explicitly positions nodes sequentially from the top with proper spacing

'justify': Uses temporary y-values that are refined by subsequent collision resolution and relaxation passes

The logic is sound and follows the documented behavior.

324-373: LGTM! Correct conditional execution of relaxation passes.

The implementation correctly skips the iterative relaxation passes when verticalAlign is 'top', preserving the sequential top-down positioning. The resolveCollisions call at line 353 appropriately remains unconditional, ensuring overlap prevention for both alignment modes.

528-528: LGTM! Interface updated correctly.

The verticalAlign prop is properly typed and marked as optional, consistent with the rest of the interface.

845-845: LGTM! Backward-compatible default value.

Setting verticalAlign: 'justify' as the default preserves the original behavior, ensuring backward compatibility with existing code.

851-930: LGTM! Complete prop threading and dependency tracking.

The verticalAlign prop is correctly:

Destructured from props (line 867)

Passed to computeData (line 890)

Included in the useMemo dependency array (line 929)

This ensures the layout recomputes when verticalAlign changes.

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

coderabbitai

Actionable comments posted: 0

🧹 Nitpick comments (1)

test-vr/tests/SankeyChart.spec-vr.tsx (1)

39-55: LGTM! Good test coverage for the new prop.

The visual regression tests follow the existing pattern and cover both values ('justify' and 'top') of the new verticalAlign prop. The tests will effectively catch any visual regressions in the layout behavior.

Optional enhancement: Consider adding combination tests that exercise both align and verticalAlign together (e.g., align="left" verticalAlign="top") to ensure the props interact correctly. This would provide more comprehensive coverage but can be deferred if needed.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 5dc2e03 and 4c83929.

⛔ Files ignored due to path filters (6)

test-vr/__snapshots__/tests/SankeyChart.spec-vr.tsx-snapshots/Sankey-verticalAlign-justify-1-chromium-linux.png is excluded by !**/*.png
test-vr/__snapshots__/tests/SankeyChart.spec-vr.tsx-snapshots/Sankey-verticalAlign-justify-1-firefox-linux.png is excluded by !**/*.png
test-vr/__snapshots__/tests/SankeyChart.spec-vr.tsx-snapshots/Sankey-verticalAlign-justify-1-webkit-linux.png is excluded by !**/*.png
test-vr/__snapshots__/tests/SankeyChart.spec-vr.tsx-snapshots/Sankey-verticalAlign-top-1-chromium-linux.png is excluded by !**/*.png
test-vr/__snapshots__/tests/SankeyChart.spec-vr.tsx-snapshots/Sankey-verticalAlign-top-1-firefox-linux.png is excluded by !**/*.png
test-vr/__snapshots__/tests/SankeyChart.spec-vr.tsx-snapshots/Sankey-verticalAlign-top-1-webkit-linux.png is excluded by !**/*.png

📒 Files selected for processing (2)

test-vr/tests/SankeyChart.spec-vr.tsx (1 hunks)
www/src/docs/api/Sankey.ts (1 hunks)

🧰 Additional context used

📓 Path-based instructions (1)

www/**

📄 CodeRabbit inference engine (DEVELOPING.md)

Use the www directory to add and commit examples for the documentation website (recharts.github.io)

Files:

www/src/docs/api/Sankey.ts

🧠 Learnings (4)

📚 Learning: 2025-10-25T07:35:46.188Z

Learnt from: CR
Repo: recharts/recharts PR: 0
File: test/README.md:0-0
Timestamp: 2025-10-25T07:35:46.188Z
Learning: Applies to test/**/*.test.{ts,tsx} : Mock getBoundingClientRect using test/helper/MockGetBoundingClientRect.ts when rendering Recharts components (e.g., Tooltip, Legend, charts)

Applied to files:

test-vr/tests/SankeyChart.spec-vr.tsx

📚 Learning: 2025-10-25T07:35:46.188Z

Learnt from: CR
Repo: recharts/recharts PR: 0
File: test/README.md:0-0
Timestamp: 2025-10-25T07:35:46.188Z
Learning: Applies to test/**/*.test.{ts,tsx} : When testing selectors, verify render counts using the spy and rerenderSameComponent from createSelectorTestCase

Applied to files:

test-vr/tests/SankeyChart.spec-vr.tsx

📚 Learning: 2025-10-25T07:35:46.188Z

Learnt from: CR
Repo: recharts/recharts PR: 0
File: test/README.md:0-0
Timestamp: 2025-10-25T07:35:46.188Z
Learning: Applies to test/**/*.test.{ts,tsx} : In tests not using createSelectorTestCase, advance timers after renders with vi.runOnlyPendingTimers()

Applied to files:

test-vr/tests/SankeyChart.spec-vr.tsx

📚 Learning: 2025-10-25T07:35:46.188Z

Learnt from: CR
Repo: recharts/recharts PR: 0
File: test/README.md:0-0
Timestamp: 2025-10-25T07:35:46.188Z
Learning: Write unit tests using Vitest and React Testing Library

Applied to files:

test-vr/tests/SankeyChart.spec-vr.tsx

🧬 Code graph analysis (1)

test-vr/tests/SankeyChart.spec-vr.tsx (1)

src/chart/Sankey.tsx (1)

Sankey (976-1019)

🔇 Additional comments (1)

www/src/docs/api/Sankey.ts (1)

234-243: LGTM! Clear and comprehensive documentation.

The documentation for the new verticalAlign prop is well-written, follows the existing pattern, and clearly explains the behavior of both 'justify' and 'top' options.

ckifer · 2025-11-07T06:51:07Z

src/chart/Sankey.tsx

      modifiedNodes: newModifiedNodes,
    };
-  }, [data, width, height, margin, iterations, nodeWidth, nodePadding, sort, link, node, linkCurvature, align]);
+  }, [data, width, height, margin, iterations, nodeWidth, nodePadding, sort, link, node, linkCurvature, align, verticalAlign]);


looks like prettier needs to format this but otherwise LGTM. Thank you

dbnl-kat · 2025-11-07T15:27:50Z

@PavelVanecek @ckifer Just fixed the prettier issue, thanks

codecov · 2025-11-07T16:04:16Z

Codecov Report

❌ Patch coverage is 87.03704% with 7 lines in your changes missing coverage. Please review.
✅ Project coverage is 91.17%. Comparing base (9dd5431) to head (2994094).
⚠️ Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
src/chart/Sankey.tsx	84.09%	7 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6576      +/-   ##
==========================================
- Coverage   91.18%   91.17%   -0.01%     
==========================================
  Files         492      492              
  Lines       40999    41041      +42     
  Branches     4584     4585       +1     
==========================================
+ Hits        37384    37419      +35     
- Misses       3598     3605       +7     
  Partials       17       17

☔ 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 · 2025-11-07T16:04:19Z

Bundle Report

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

Detailed changes

Bundle name	Size	Change
recharts/bundle-cjs	1.12MB	517 bytes (0.05%) ⬆️

Affected Assets, Files, and Routes:

view changes for bundle: recharts/bundle-cjs

Assets Changed:

Asset Name	Size Change	Total Size	Change (%)
`chart/Sankey.js`	517 bytes	27.18kB	1.94%

dbnl-kat added 3 commits November 6, 2025 12:28

add verticalAlign prop for Sankey

5dc2e03

tests

d0b6817

docs

4c83929

dbnl-kat changed the title ~~add verticalAlign prop for Sankey~~ Add verticalAlign prop for Sankey Nov 6, 2025

coderabbitai bot reviewed Nov 6, 2025

View reviewed changes

PavelVanecek approved these changes Nov 7, 2025

View reviewed changes

ckifer approved these changes Nov 7, 2025

View reviewed changes

prettier

2994094

ckifer merged commit b94365a into recharts:main Nov 7, 2025
21 of 23 checks passed

coderabbitai bot mentioned this pull request Nov 24, 2025

Omnidoc: fix default values for polar charts, and Sankey #6682

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Add verticalAlign prop for Sankey#6576

Add verticalAlign prop for Sankey#6576
ckifer merged 4 commits intorecharts:mainfrom
dbnlAI:sankey-node-vertical-alignment

dbnl-kat commented Nov 6, 2025 •

edited by coderabbitai bot

Loading

Uh oh!

coderabbitai bot commented Nov 6, 2025 •

edited

Loading

Uh oh!

coderabbitai bot left a comment

Uh oh!

ckifer Nov 7, 2025 •

edited

Loading

Uh oh!

dbnl-kat commented Nov 7, 2025

Uh oh!

codecov bot commented Nov 7, 2025 •

edited

Loading

Uh oh!

codecov bot commented Nov 7, 2025

Assets Changed:

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

dbnl-kat commented Nov 6, 2025 • edited by coderabbitai bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

Checklist:

Summary by CodeRabbit

Uh oh!

coderabbitai bot commented Nov 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Sequence Diagram(s)

Estimated code review effort

Possibly related issues

Possibly related PRs

Suggested reviewers

Pre-merge checks and finishing touches

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

ckifer Nov 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dbnl-kat commented Nov 7, 2025

Uh oh!

codecov bot commented Nov 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

codecov bot commented Nov 7, 2025

Bundle Report

Affected Assets, Files, and Routes:

Assets Changed:

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

dbnl-kat commented Nov 6, 2025 •

edited by coderabbitai bot

Loading

coderabbitai bot commented Nov 6, 2025 •

edited

Loading

ckifer Nov 7, 2025 •

edited

Loading

codecov bot commented Nov 7, 2025 •

edited

Loading