feat(Sankey): support accessibilityLayer, title and desc props#7546
Merged
PavelVanecek merged 1 commit intoJul 12, 2026
Merged
Conversation
Contributor
WalkthroughSankey now supports accessibility-layer configuration, SVG titles and descriptions, and default role/tabIndex behavior. Tests cover defaults, disabling the layer, explicit attribute overrides, and rendered descriptive elements. ChangesSankey accessibility
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/chart/Sankey.tsx (1)
860-861: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd JSDoc for
titleanddesc.
accessibilityLayerdocuments its default, buttitle/deschave no descriptions. Since API docs are auto-generated from these JSDoc comments, the props will render undocumented.📝 Suggested docs
- title?: string; - desc?: string; + /** + * Text rendered inside the SVG `<title>` element, announced by screen readers. + */ + title?: string; + /** + * Text rendered inside the SVG `<desc>` element, announced by screen readers. + */ + desc?: string;As per coding guidelines: "Update TypeScript interfaces and JSDoc comments in source code to automatically update API documentation".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/chart/Sankey.tsx` around lines 860 - 861, Add JSDoc comments describing the `title` and `desc` properties in the relevant TypeScript interface near `accessibilityLayer`, so generated API documentation includes both props. Keep the descriptions focused on their accessibility purpose and preserve the existing property types.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/chart/Sankey.spec.tsx`:
- Around line 75-114: Update each accessibility test in the Sankey describe
block to call act(() => vi.runOnlyPendingTimers()) immediately after its
render(...) call, ensuring the queued ReportChartSize update completes before
querying or asserting on the SVG and title/description elements.
---
Nitpick comments:
In `@src/chart/Sankey.tsx`:
- Around line 860-861: Add JSDoc comments describing the `title` and `desc`
properties in the relevant TypeScript interface near `accessibilityLayer`, so
generated API documentation includes both props. Keep the descriptions focused
on their accessibility purpose and preserve the existing property types.
🪄 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: 0d05ffde-5db6-4bfa-ae74-cebce4ef40ea
📒 Files selected for processing (2)
src/chart/Sankey.tsxtest/chart/Sankey.spec.tsx
Comment on lines
+75
to
+114
| describe('accessibility', () => { | ||
| it('should add tabindex and role to the svg element by default', () => { | ||
| const { container } = render(<Sankey width={1000} height={500} data={exampleSankeyData} />); | ||
|
|
||
| const svg = container.querySelector('svg'); | ||
| assertNotNull(svg); | ||
| expect(svg).toHaveAttribute('role', 'application'); | ||
| expect(svg).toHaveAttribute('tabindex', '0'); | ||
| }); | ||
|
|
||
| it('should not add tabindex and role to the svg element when accessibilityLayer=false', () => { | ||
| const { container } = render( | ||
| <Sankey width={1000} height={500} data={exampleSankeyData} accessibilityLayer={false} />, | ||
| ); | ||
|
|
||
| const svg = container.querySelector('svg'); | ||
| assertNotNull(svg); | ||
| expect(svg).not.toHaveAttribute('role'); | ||
| expect(svg).not.toHaveAttribute('tabindex'); | ||
| }); | ||
|
|
||
| it('should prefer explicit role and tabIndex over the accessibilityLayer defaults', () => { | ||
| const { container } = render( | ||
| <Sankey width={1000} height={500} data={exampleSankeyData} role="img" tabIndex={-1} />, | ||
| ); | ||
|
|
||
| const svg = container.querySelector('svg'); | ||
| assertNotNull(svg); | ||
| expect(svg).toHaveAttribute('role', 'img'); | ||
| expect(svg).toHaveAttribute('tabindex', '-1'); | ||
| }); | ||
|
|
||
| it('should set title and description correctly', () => { | ||
| const { container } = render( | ||
| <Sankey width={1000} height={500} data={exampleSankeyData} title="Sankey title" desc="Sankey description" />, | ||
| ); | ||
|
|
||
| expect(container.querySelector('title')).toHaveTextContent('Sankey title'); | ||
| expect(container.querySelector('desc')).toHaveTextContent('Sankey description'); | ||
| }); |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== test/chart/Sankey.spec.tsx ==\n'
wc -l test/chart/Sankey.spec.tsx
sed -n '1,220p' test/chart/Sankey.spec.tsx
printf '\n== Sankey implementation candidates ==\n'
git ls-files | rg '(^|/)(Sankey|ReportChartSize|useChartWidth|useChartHeight|ChartSize)' || true
printf '\n== vitest setup ==\n'
wc -l vitest.setup.ts
sed -n '1,240p' vitest.setup.tsRepository: recharts/recharts
Length of output: 11165
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== locate vitest setup ==\n'
git ls-files | rg 'vitest.*setup|setup.*vitest|test/setup|test/.*setup' || true
printf '\n== Sankey implementation outline ==\n'
ast-grep outline src/chart/Sankey.tsx --view expanded || true
printf '\n== chart layout context outline ==\n'
ast-grep outline src/context/chartLayoutContext.tsx --view expanded || true
printf '\n== search for timer advancement patterns in Sankey-related tests ==\n'
rg -n "runOnlyPendingTimers|useFakeTimers|createSelectorTestCase|render\\(<Sankey|render\\(" test/chart/Sankey.spec.tsx test test-vr -g '*Sankey*' -g '*.spec.tsx' -g '*.spec.ts' || trueRepository: recharts/recharts
Length of output: 50374
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== test/vitest.setup.ts ==\n'
sed -n '1,220p' test/vitest.setup.ts
printf '\n== Sankey implementation (relevant slices) ==\n'
sed -n '1220,1425p' src/chart/Sankey.tsx
printf '\n== chart layout context (ReportChartSize) ==\n'
sed -n '200,280p' src/context/chartLayoutContext.tsxRepository: recharts/recharts
Length of output: 10597
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== search for setChartSize and autobatch markers ==\n'
rg -n "setChartSize|prepareAutoBatched|SHOULD_AUTOBATCH|autoBatch|requestAnimationFrame" src test -g '*.ts' -g '*.tsx' | sed -n '1,220p'
printf '\n== locate chart size reducer/action file candidates ==\n'
git ls-files src/state src/context | rg 'chart.*(size|layout)|layout|store|slice' || trueRepository: recharts/recharts
Length of output: 7621
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== src/state/layoutSlice.ts ==\n'
sed -n '1,140p' src/state/layoutSlice.ts
printf '\n== src/state/store.ts ==\n'
sed -n '1,180p' src/state/store.ts
printf '\n== any auto-batched chart size tests? ==\n'
rg -n "ReportChartSize|setChartSize\\(|useChartWidth\\(|useChartHeight\\(" test src -g '*.ts' -g '*.tsx' | sed -n '1,220p'Repository: recharts/recharts
Length of output: 11259
Advance the pending timers after each render(...).
Sankey doesn’t populate useChartWidth/useChartHeight until ReportChartSize flushes through the rAF-batched store update, so these SVG assertions can race the queued size commit. Add act(() => vi.runOnlyPendingTimers()); after each render in this block.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/chart/Sankey.spec.tsx` around lines 75 - 114, Update each accessibility
test in the Sankey describe block to call act(() => vi.runOnlyPendingTimers())
immediately after its render(...) call, ensuring the queued ReportChartSize
update completes before querying or asserting on the SVG and title/description
elements.
Source: Path instructions
PavelVanecek
approved these changes
Jul 12, 2026
PavelVanecek
left a comment
PavelVanecek
left a comment
Collaborator
There was a problem hiding this comment.
Approve but I'm going to treat it as a new feature rather than a bugfix
PavelVanecek
changed the title
Bundle ReportChanges will increase total bundle size by 1.22kB (0.02%) ⬆️. This is within the configured threshold ✅ Detailed changes
Affected Assets, Files, and Routes:view changes for bundle: recharts/bundle-es6Assets Changed:
view changes for bundle: recharts/bundle-cjsAssets Changed:
view changes for bundle: recharts/bundle-umdAssets Changed:
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7546 +/- ##
=======================================
Coverage 88.17% 88.18%
=======================================
Files 613 613
Lines 14246 14254 +8
Branches 3579 3585 +6
=======================================
+ Hits 12562 12570 +8
Misses 1494 1494
Partials 190 190 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Sankey currently rejects the
accessibilityLayer,titleanddescprops at the type level, and even at runtimetitle/descget stripped bysvgPropertiesNoEventsbefore they reachSurface, so there's no way to give the chart an accessible name or description.This adds the three props to
SankeyPropsand handles them locally inSankeyImpl:title/descare passed explicitly to the existingSurface(which already knows how to render them), androle/tabIndexdefault toapplication/0whenaccessibilityLayeris on (the default, same as every other chart), while explicitrole/tabIndexprops still win. Same precedence logic asMainChartSurface.I deliberately did not move Sankey onto
RootSurface— that was the approach in #7153 and the concern there was that it drags in ZIndex portal support as a side effect. No changes toSurfaceeither. Keyboard navigation wiring for Sankey nodes/links is out of scope here; this covers the props and the SVG attributes.Related Issue
Fixes #7151
Motivation and Context
Users migrating from 2.x lost the ability to set
accessibilityLayer/title/descon Sankey (TS errors, and silently dropped at runtime). Every other chart supports them.How Has This Been Tested?
Added an
accessibilitydescribe block totest/chart/Sankey.spec.tsx: default role/tabindex,accessibilityLayer={false}omitting them, explicitrole/tabIndexoverriding the defaults, andtitle/descrendering. The default-attributes and title/desc tests fail on current main and pass with this change; the full Sankey spec (33 tests),AccessibilityLayer.spec.tsxandSankey.typed.spec.tsxall pass locally, plustscand eslint on the changed files.Screenshots (if appropriate):
n/a — attribute-only change, no visual difference.
Types of changes
Checklist:
Summary by CodeRabbit