Conversation
WalkthroughAdds blur handling so charts dispatch a Changes
Sequence DiagramsequenceDiagram
participant User
participant RechartsWrapper
participant Middleware as keyboardEventsMiddleware
participant State as AccessibilityState
User->>RechartsWrapper: focus moves away (blur)
RechartsWrapper->>Middleware: dispatch blurAction
Middleware->>Middleware: is accessibility layer active?
alt active
Middleware->>State: dispatch setKeyboardInteraction(active: false)
State->>State: clear activeIndex and activeCoordinate
end
Note over RechartsWrapper,State: Tooltip hides due to cleared state
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@test/chart/AccessibilityLayer.spec.tsx`:
- Around line 202-231: The new test "Tooltip closes when chart loses focus" is
missing fake timers and doesn't use the createSelectorTestCase helper; update
the spec to call vi.useFakeTimers()/vi.restoreAllMocks() in the describe/test
setup (or add beforeAll/afterAll) so timers and requestAnimationFrame are
controlled, and refactor the test to use createSelectorTestCase instead of raw
render (or wrap the render in the helper) so selector-call assertions are
supported—adjust usages of render, act, and arrowRight to the
createSelectorTestCase pattern and ensure timers are advanced where needed
(e.g., vi.advanceTimersByTime) to avoid flakiness.
🧹 Nitpick comments (1)
src/state/keyboardEventsMiddleware.ts (1)
160-178: Add explicit types for the blur listener parameters.The handler currently relies on inference; please make the parameter and return types explicit for consistency and clarity.
🛠️ Suggested change
- effect: (_action, listenerApi: ListenerEffectAPI<RechartsRootState, AppDispatch>) => { + effect: ( + _action: ReturnType<typeof blurAction>, + listenerApi: ListenerEffectAPI<RechartsRootState, AppDispatch>, + ): void => {As per coding guidelines, type function parameters and return values explicitly.
| test('Tooltip closes when chart loses focus', () => { | ||
| const { container } = render( | ||
| <AreaChart width={100} height={50} data={PageData} accessibilityLayer={accessibilityLayer}> | ||
| <Area type="monotone" dataKey="uv" stroke="#ff7300" fill="#ff7300" /> | ||
| <Tooltip /> | ||
| <Legend /> | ||
| <XAxis dataKey="name" /> | ||
| <YAxis /> | ||
| </AreaChart>, | ||
| ); | ||
|
|
||
| const svg = getMainSurface(container); | ||
| assertNotNull(svg); | ||
| const tooltip = getTooltip(container); | ||
|
|
||
| // Tooltip not visible initially | ||
| expect(tooltip.textContent).toBe(''); | ||
|
|
||
| // Focus the chart - tooltip should appear | ||
| act(() => svg.focus()); | ||
| expect(tooltip).toHaveTextContent('Page A'); | ||
|
|
||
| // Navigate to another data point | ||
| arrowRight(svg); | ||
| expect(tooltip).toHaveTextContent('Page B'); | ||
|
|
||
| // Blur the chart - tooltip should disappear | ||
| act(() => svg.blur()); | ||
| expect(tooltip.textContent).toBe(''); | ||
| }); |
There was a problem hiding this comment.
Use fake timers (and prefer createSelectorTestCase) in the new tests.
These new tests are added without vi.useFakeTimers(), and they use render directly instead of createSelectorTestCase, which can lead to timer-related flakiness and misses selector-call assertions. Consider adding fake timers for the describes that include these tests and, if feasible, refactor the new cases to createSelectorTestCase.
🧪 Suggested timer setup
describe.each([true, undefined])('AccessibilityLayer with accessibilityLayer=%s', accessibilityLayer => {
+ beforeEach(() => {
+ vi.useFakeTimers();
+ });
+
+ afterEach(() => {
+ vi.useRealTimers();
+ }); describe('Multiple charts navigation', () => {
+ beforeEach(() => {
+ vi.useFakeTimers();
+ });
+
+ afterEach(() => {
+ vi.useRealTimers();
+ });As per coding guidelines, use vi.useFakeTimers() in all tests due to Redux autoBatchEnhancer dependency on timers and requestAnimationFrame, and prefer the createSelectorTestCase helper when writing or modifying tests.
🤖 Prompt for AI Agents
In `@test/chart/AccessibilityLayer.spec.tsx` around lines 202 - 231, The new test
"Tooltip closes when chart loses focus" is missing fake timers and doesn't use
the createSelectorTestCase helper; update the spec to call
vi.useFakeTimers()/vi.restoreAllMocks() in the describe/test setup (or add
beforeAll/afterAll) so timers and requestAnimationFrame are controlled, and
refactor the test to use createSelectorTestCase instead of raw render (or wrap
the render in the helper) so selector-call assertions are supported—adjust
usages of render, act, and arrowRight to the createSelectorTestCase pattern and
ensure timers are advanced where needed (e.g., vi.advanceTimersByTime) to avoid
flakiness.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6958 +/- ##
==========================================
- Coverage 91.13% 90.38% -0.75%
==========================================
Files 509 515 +6
Lines 37967 38522 +555
Branches 5288 5342 +54
==========================================
+ Hits 34601 34818 +217
- Misses 3357 3695 +338
Partials 9 9 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Bundle ReportChanges will decrease total bundle size by 21.24kB (-0.74%) ⬇️. This is within the configured threshold ✅ Detailed changes
ℹ️ *Bundle size includes cached data from a previous commit Affected Assets, Files, and Routes:view changes for bundle: recharts/bundle-cjsAssets Changed:
view changes for bundle: recharts/bundle-umdAssets Changed:
view changes for bundle: recharts/bundle-es6Assets Changed:
|
There was a problem hiding this comment.
@gVguy can you undo these changes in the lockfile please? I think this is from running an older version of npm
Description
When focus leaves the chart (Tab to next element or blur event), the
blurActionis dispatched, which sets the keyboard interaction to inactive, causing the tooltip to disappear.Pressing Escape still closes tooltips as before.
Related Issue
Closes #6937
Motivation and Context
Solves the problem described in the related issue ^
How Has This Been Tested?
Screenshots (if appropriate):
Types of changes
Checklist:
Summary by CodeRabbit
New Features
Tests