Animation tests#7255
Conversation
Document that useNeedsClip is purely driven by the allowDataOverflow prop,
not by axis type (category vs number) or domain configuration.
Key findings:
- Neither categorical nor numerical axes enable clipPath by default
- Only explicit allowDataOverflow={true} triggers GraphicalItemClipPath rendering
- There is always a global chart-level clipPath from ClipPathProvider
- The visual difference with type='number' is about pixel position computation
(scaleLinear keeps points in bounds), not about SVG clipping
Co-authored-by: Copilot <[email protected]>
…nd numerical axes
WalkthroughAdds two comprehensive test suites for Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
test/cartesian/GraphicalItemClipPath.spec.tsx (1)
65-69: Add call-count assertions for selector spies.These
createSelectorTestCaseusages inspect the selector output but do not verify how many times the selector ran. Addingexpect(spy).toHaveBeenCalledTimes(...)keeps these tests aligned with the render-performance guardrail.As per coding guidelines,
Verify the number of selector calls using the spy object from createSelectorTestCase to spot unnecessary re-renders and improve performance.Also applies to: 101-105, 134-138, 290-301, 304-315, 365-376
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/cartesian/GraphicalItemClipPath.spec.tsx` around lines 65 - 69, Add an assertion that verifies how many times the selector spy was invoked: after rendering the test case that uses renderTestCase(state => selectXAxisSettings(state, 0)) and before inspecting spy.mock.lastCall, add expect(spy).toHaveBeenCalledTimes(1) so the test ensures selectXAxisSettings was executed exactly once and catches unintended extra selector runs.test/cartesian/GraphicalItemClipPath.scale-behavior.spec.tsx (1)
284-285: Assert selector call counts for these selector test cases.These tests consume
createSelectorTestCasespies but only inspect the last result. Add explicittoHaveBeenCalledTimes(...)checks around the initial render and post-slide update to catch unexpected extra selector runs.As per coding guidelines,
Verify the number of selector calls using the spy object from createSelectorTestCase to spot unnecessary re-renders and improve performance.Also applies to: 372-373, 441-442, 492-515
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/cartesian/GraphicalItemClipPath.scale-behavior.spec.tsx` around lines 284 - 285, Add explicit call-count assertions for the selector spy returned by createSelectorTestCase: after the initial render of renderTestCase (using selectLinePoints) assert expect(spy).toHaveBeenCalledTimes(1), then after the slide/update action assert expect(spy).toHaveBeenCalledTimes(2) to ensure no extra selector runs; apply the same pattern to the other selector test cases that use createSelectorTestCase (the other renderTestCase/selector usages referenced in the review) so each test verifies initial and post-update call counts via the spy.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/cartesian/GraphicalItemClipPath.scale-behavior.spec.tsx`:
- Around line 475-530: The test renders static categorical (catChart) and
numerical (numChart) charts but never exercises the "slide" transition; update
the test to perform a Slide state transition for both charts via
createSelectorTestCase so you capture pre- and post-slide positions using
selectLinePoints (e.g., call the selector once before the slide and once after
applying the Slide transition/update), then compute spacing delta (catSpacing
and numSpacing) from the before→after positions and assert that the shifts
match; keep isAnimationActive={false} and reuse the existing id="line" selector
to locate points.
- Around line 294-300: Import vi from 'vitest' at the top of the test file and
ensure each simulated button click is followed by flushing pending timers so
selector updates scheduled via requestAnimationFrame are applied before reading
spy.mock.calls; specifically, around the act(() => { button.click(); }) blocks
(the ones using spy and reading spy.mock.calls[…]) call vi.runAllTimers() (or
vi.advanceTimersByTime(...)) inside act or immediately after to flush RAF
timers, and apply this pattern to the click blocks referenced (the blocks around
act(() => { button.click(); }) that are read into afterPoints and similar
variables), i.e., add the import and wrap/append vi.runAllTimers() for the click
code at the three locations mentioned.
---
Nitpick comments:
In `@test/cartesian/GraphicalItemClipPath.scale-behavior.spec.tsx`:
- Around line 284-285: Add explicit call-count assertions for the selector spy
returned by createSelectorTestCase: after the initial render of renderTestCase
(using selectLinePoints) assert expect(spy).toHaveBeenCalledTimes(1), then after
the slide/update action assert expect(spy).toHaveBeenCalledTimes(2) to ensure no
extra selector runs; apply the same pattern to the other selector test cases
that use createSelectorTestCase (the other renderTestCase/selector usages
referenced in the review) so each test verifies initial and post-update call
counts via the spy.
In `@test/cartesian/GraphicalItemClipPath.spec.tsx`:
- Around line 65-69: Add an assertion that verifies how many times the selector
spy was invoked: after rendering the test case that uses renderTestCase(state =>
selectXAxisSettings(state, 0)) and before inspecting spy.mock.lastCall, add
expect(spy).toHaveBeenCalledTimes(1) so the test ensures selectXAxisSettings was
executed exactly once and catches unintended extra selector runs.
🪄 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: a5bbaeb4-a9d4-456c-b1c3-85d9c3fe806e
📒 Files selected for processing (2)
test/cartesian/GraphicalItemClipPath.scale-behavior.spec.tsxtest/cartesian/GraphicalItemClipPath.spec.tsx
| // Slide the data | ||
| const button = container.querySelector('button')!; | ||
| act(() => { | ||
| button.click(); | ||
| }); | ||
|
|
||
| const afterPoints: ReadonlyArray<LinePointItem> | undefined = spy.mock.calls[spy.mock.calls.length - 1]?.[0]; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify click-driven selector assertions flush pending timers before reading the spy.
rg -n -C4 'button\.click\(\)|runOnlyPendingTimers|spy\.mock\.calls\[spy\.mock\.calls\.length - 1\]' test/cartesian/GraphicalItemClipPath.scale-behavior.spec.tsxRepository: recharts/recharts
Length of output: 3126
🏁 Script executed:
#!/bin/bash
# Check imports and timer handling in the test file
head -30 test/cartesian/GraphicalItemClipPath.scale-behavior.spec.tsxRepository: recharts/recharts
Length of output: 1649
🏁 Script executed:
#!/bin/bash
# Check if vi is imported and search for any timer-related calls
rg -n "import.*vi|vi\.useFakeTimers|vi\.runOnlyPendingTimers" test/cartesian/GraphicalItemClipPath.scale-behavior.spec.tsx | head -20Repository: recharts/recharts
Length of output: 190
🏁 Script executed:
#!/bin/bash
# Find all button click instances in the file to understand full scope
rg -n "button\.click\(\)" test/cartesian/GraphicalItemClipPath.scale-behavior.spec.tsxRepository: recharts/recharts
Length of output: 144
🏁 Script executed:
#!/bin/bash
# Check the createSelectorTestCase helper to see if it handles timers
head -50 test/helper/createSelectorTestCase.tsRepository: recharts/recharts
Length of output: 157
🏁 Script executed:
#!/bin/bash
# Search for similar patterns in other test files to see if they flush timers after clicks
rg -l "button\.click\(\)" test --type ts --type tsx | head -5Repository: recharts/recharts
Length of output: 88
🏁 Script executed:
#!/bin/bash
# Look for patterns where clicks are followed by timer flushes in other test files
rg -B2 -A2 "button\.click\(\).*\n.*vi\.runOnlyPendingTimers" test --type ts --type tsx | head -30Repository: recharts/recharts
Length of output: 252
🏁 Script executed:
#!/bin/bash
# Find the createSelectorTestCase helper file
find test -name "*createSelectorTestCase*" -type fRepository: recharts/recharts
Length of output: 99
🏁 Script executed:
#!/bin/bash
# Search for button clicks followed by timer flushes
rg -U "button\.click\(\).*\n.*runOnlyPendingTimers" testRepository: recharts/recharts
Length of output: 43
🏁 Script executed:
#!/bin/bash
# Look for tests that use runOnlyPendingTimers after click events
rg -B3 -A3 "runOnlyPendingTimers" test --type ts | head -50Repository: recharts/recharts
Length of output: 2999
🏁 Script executed:
#!/bin/bash
# Check the createSelectorTestCase helper
cat test/helper/createSelectorTestCase.tsxRepository: recharts/recharts
Length of output: 8431
🏁 Script executed:
#!/bin/bash
# Check the DynamicZIndex.spec.tsx file to see the full pattern
rg -B5 -A5 "button\.click" test/zIndex/DynamicZIndex.spec.tsx | head -40Repository: recharts/recharts
Length of output: 481
🏁 Script executed:
#!/bin/bash
# Check if vi is imported in DynamicZIndex.spec.tsx
head -10 test/zIndex/DynamicZIndex.spec.tsxRepository: recharts/recharts
Length of output: 446
Add vi import and flush pending timers after each button click.
These click-driven updates trigger Redux auto-batched state changes that are deferred to requestAnimationFrame. Reading spy.mock.calls[...] immediately after the click can capture stale selector output. The createSelectorTestCase helper already flushes timers during initial render, but additional timer flushes are required after user interactions.
Required changes
-import { describe, it, expect, beforeEach } from 'vitest';
+import { describe, it, expect, beforeEach, vi } from 'vitest';Then wrap each button click with timer flush:
- act(() => {
+ act(() => {
button.click();
+ vi.runOnlyPendingTimers();
});
const afterPoints: ReadonlyArray<LinePointItem> | undefined = spy.mock.calls[spy.mock.calls.length - 1]?.[0];Apply to lines 294–300, 381–387, and 451–456.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/cartesian/GraphicalItemClipPath.scale-behavior.spec.tsx` around lines
294 - 300, Import vi from 'vitest' at the top of the test file and ensure each
simulated button click is followed by flushing pending timers so selector
updates scheduled via requestAnimationFrame are applied before reading
spy.mock.calls; specifically, around the act(() => { button.click(); }) blocks
(the ones using spy and reading spy.mock.calls[…]) call vi.runAllTimers() (or
vi.advanceTimersByTime(...)) inside act or immediately after to flush RAF
timers, and apply this pattern to the click blocks referenced (the blocks around
act(() => { button.click(); }) that are read into afterPoints and similar
variables), i.e., add the import and wrap/append vi.runAllTimers() for the click
code at the three locations mentioned.
| it('both axis types produce the same shift magnitude when item count stays constant', () => { | ||
| // Categorical | ||
| const catChart = ({ children }: { children: ReactNode }) => ( | ||
| <LineChart | ||
| width={500} | ||
| height={300} | ||
| data={[ | ||
| { name: 'A', value: 10 }, | ||
| { name: 'B', value: 20 }, | ||
| { name: 'C', value: 30 }, | ||
| ]} | ||
| > | ||
| <XAxis dataKey="name" /> | ||
| <Line isAnimationActive={false} dataKey="value" id="line" /> | ||
| {children} | ||
| </LineChart> | ||
| ); | ||
| const catRender = createSelectorTestCase(catChart); | ||
| const { spy: catSpy } = catRender(state => selectLinePoints(state, 0, 0, false, 'line')); | ||
| const catPoints: ReadonlyArray<LinePointItem> | undefined = catSpy.mock.calls[catSpy.mock.calls.length - 1]?.[0]; | ||
| assertNotNull(catPoints); | ||
|
|
||
| // Numerical | ||
| const numChart = ({ children }: { children: ReactNode }) => ( | ||
| <LineChart | ||
| width={500} | ||
| height={300} | ||
| data={[ | ||
| { x: 1, value: 10 }, | ||
| { x: 2, value: 20 }, | ||
| { x: 3, value: 30 }, | ||
| ]} | ||
| > | ||
| <XAxis dataKey="x" type="number" domain={['dataMin', 'dataMax']} /> | ||
| <Line isAnimationActive={false} dataKey="value" id="line" /> | ||
| {children} | ||
| </LineChart> | ||
| ); | ||
| const numRender = createSelectorTestCase(numChart); | ||
| const { spy: numSpy } = numRender(state => selectLinePoints(state, 0, 0, false, 'line')); | ||
| const numPoints: ReadonlyArray<LinePointItem> | undefined = numSpy.mock.calls[numSpy.mock.calls.length - 1]?.[0]; | ||
| assertNotNull(numPoints); | ||
|
|
||
| // Both should have same number of points | ||
| expect(catPoints).toHaveLength(3); | ||
| expect(numPoints).toHaveLength(3); | ||
|
|
||
| // Both should use the same range, so the first and last point positions | ||
| // should be similar (they both map to the edges of the plot area) | ||
| // Note: exact values may differ due to YAxis presence and padding | ||
| const catSpacing = catPoints[1]!.x! - catPoints[0]!.x!; | ||
| const numSpacing = numPoints[1]!.x! - numPoints[0]!.x!; | ||
|
|
||
| // Both spacings should be equal because both have the same number of points | ||
| // across the same plot width | ||
| expect(catSpacing).toBeCloseTo(numSpacing, 0); |
There was a problem hiding this comment.
Make this test exercise the slide it describes.
The test title says “during identical data slide”, but it renders one static categorical chart and one static numerical chart. This can pass even if the slide/update behavior regresses; consider giving both charts a Slide state transition and comparing before/after shifts.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/cartesian/GraphicalItemClipPath.scale-behavior.spec.tsx` around lines
475 - 530, The test renders static categorical (catChart) and numerical
(numChart) charts but never exercises the "slide" transition; update the test to
perform a Slide state transition for both charts via createSelectorTestCase so
you capture pre- and post-slide positions using selectLinePoints (e.g., call the
selector once before the slide and once after applying the Slide
transition/update), then compute spacing delta (catSpacing and numSpacing) from
the before→after positions and assert that the shifts match; keep
isAnimationActive={false} and reuse the existing id="line" selector to locate
points.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7255 +/- ##
=======================================
Coverage 89.07% 89.07%
=======================================
Files 539 539
Lines 41010 41010
Branches 5557 5558 +1
=======================================
Hits 36528 36528
Misses 4474 4474
Partials 8 8 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Staging Deployment Details
These deployments will remain available for 30 days. To update snapshots: Comment |
Bundle ReportBundle size has no change ✅ |
1 participant
Tests only. I'm extracting changes from the huge animations branch that we can merge independently to make the huge branch a bit smaller.
Summary by CodeRabbit