Fix Line animation when interrupted with non-data related changes#6641
Fix Line animation when interrupted with non-data related changes#6641
Conversation
WalkthroughThis PR fixes an animation bug where updating React state during or after a chart animation would break the animation and restart it from the moment of state update. The fix introduces animation ID tracking and improves starting point calculation logic to be more robust to state changes and layout updates. Changes
Sequence DiagramsequenceDiagram
actor User
participant React as React<br/>(State Update)
participant Line as Line Component<br/>Animation Logic
participant Refs as Ref Tracking<br/>(animationIdRef,<br/>startingPointRef)
Note over User,Refs: Before Fix: State change causes animation restart
User->>React: Update state (e.g., hover)
React->>Line: Re-render with same animationId
Line->>Refs: startingPoint recalculated<br/>(lost progress)
Note over Refs: Animation restarts from 0
Note over User,Refs: After Fix: State change preserves animation progress
User->>React: Update state (e.g., hover)
React->>Line: Re-render with same animationId
Line->>Refs: animationIdRef unchanged<br/>startingPointRef persists
Note over Refs: Animation continues from current position
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
test/cartesian/Line.animation.spec.tsx (1)
1723-1810: Stroke-width interruption tests accurately capture issue #6044 behaviorThe new suite exercises exactly the problematic scenario (mid‑animation
strokeWidthchanges, including multiple toggles) and asserts dasharray continuity and final rendering, aligning well with the intended fix. As an optional enhancement, you could also assert thestroke-widthattribute after each click to prove the prop change took effect, but the current expectations are already sufficient to guard the regression.storybook/stories/API/chart/LineChart.stories.tsx (1)
1-1: Interactive hover story cleanly exercises strokeWidth changes during animationThe added
isHoveredstate plusonMouseEnter/onMouseLeavewired into<Line>give a clear, manual reproduction of mid‑animationstrokeWidthchanges and leverage a longanimationDurationto make the effect visible. The implementation is straightforward and idiomatic. If you want to trim boilerplate, the handlers could be simple inline functions withoutuseCallback, but that’s purely stylistic for a Storybook example.Also applies to: 15-24, 28-34
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/cartesian/Line.tsx(3 hunks)storybook/stories/API/chart/LineChart.stories.tsx(2 hunks)test/cartesian/Line.animation.spec.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
storybook/stories/API/chart/LineChart.stories.tsx (2)
storybook/stories/API/props/EventHandlers.ts (2)
onMouseEnter(106-106)onMouseLeave(107-107)src/cartesian/Line.tsx (1)
Line(822-822)
test/cartesian/Line.animation.spec.tsx (4)
test/helper/createSelectorTestCase.tsx (1)
createSelectorTestCase(78-139)test/_data.ts (1)
PageData(4-11)test/helper/assertNotNull.ts (1)
assertNotNull(1-5)test/helper/expectLine.ts (1)
expectLines(11-18)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build, Test, Pack
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (2)
src/cartesian/Line.tsx (2)
393-446: Animation ID & starting point tracking correctly separate new animations from cosmetic re-rendersUsing
animationIdReftogether withstartingPointRefto derivestartingPointonly whenanimationIdchanges is a solid way to:
- continue interrupted length animations from
longestAnimatedLengthRef.current, and- avoid restarting the length animation on non‑data re-renders (like
strokeWidth/color changes, as in issue #6044).The ref-based approach keeps this logic local to
CurveWithAnimationwithout extra renders, and the behavior aligns with the surrounding tests.
475-510: Deferring previous-points and longest-length updates avoids Legend/layout-induced glitchesThe new
if (t > 0 && totalLength > 0)block that updatespreviousPointsRef.currentandlongestAnimatedLengthRef.currentonly after the animation has a validtotalLengthneatly addresses the “path jumps after Legend/layout init” problem documented in the comments. It ensures:
- we don’t snapshot previous points before
pathRefand Legend height are settled, andlongestAnimatedLengthRefonly tracks real path lengths, so subsequent animations start from the true furthest length.The extra per-frame work is minimal and consistent with the existing animation loop.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6641 +/- ##
=======================================
Coverage 94.17% 94.17%
=======================================
Files 494 494
Lines 41666 41672 +6
Branches 4823 4822 -1
=======================================
+ Hits 39240 39246 +6
Misses 2421 2421
Partials 5 5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Bundle ReportChanges will increase total bundle size by 2.21kB (0.08%) ⬆️. This is within the configured threshold ✅ Detailed changes
Affected Assets, Files, and Routes:view changes for bundle: recharts/bundle-cjsAssets Changed:
view changes for bundle: recharts/bundle-es6Assets Changed:
view changes for bundle: recharts/bundle-umdAssets Changed:
|
|
looks way better noice |
Description
We already had a nice handing for when an animation is interrupted by a change in the data. We did not have anything for when an animation is interrupted by a render that does not change the data which is what #6044 is about.
Related Issue
Fixes #6044
Some improvements are already in main since #6634
Screenshots (if appropriate):
This is what it looked like before #6634, same as in the original report in #6044:
strokewidth-before.mov
This is what is currently in the main branch, notice how the length suddenly jumps:
strokewidth-after.mov
After this PR:
strokewidth-after-after.mov
Types of changes
Checklist:
Summary by CodeRabbit
Bug Fixes
Tests