Skip to content

Performance improvements#6634

Merged
ckifer merged 16 commits into
mainfrom
label-perf
Nov 16, 2025
Merged

Performance improvements#6634
ckifer merged 16 commits into
mainfrom
label-perf

Conversation

@PavelVanecek

@PavelVanecek PavelVanecek commented Nov 15, 2025 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Description

Mostly just focused on not doing stuff that is not necessary.

  • Tooltip setting optimization landed in Optimize SetTooltipEntrySettings to break infinite rendering loop #6616, that was important
  • I also added Legend and XAxis+YAxis from add+remove to add+replace+remove, this reduces selectors recalculation. This was probably the single largest difference.
  • I added requestAnimationFrame queue to mouse handlers. This was a lot of code but the difference in performance was small. Anyway. I left it in.
  • I changed (who am I kidding, AI changed) the SVG prop filtering function from Object.entries call into for ... in loop, turns out that improves the performance 10x, who knew.
  • SetCartesianGraphicalItem is memoized to prevent pushing state updates and reduce selector recalculation when not necessary
  • Area and Line have more specific dependencies for when a new animation should trigger. I think this should help with Updating a state breaks chart animation #6044 but I haven't tested it yet. (Rectangle has the same optimization already, since Optimize Bar component rendering when activeBar is falsy #6290)
  • And finally, some basic optimization of the example itself: make static objects static instead of re-creating on every render, add useCallback on functions, render null instead of zero-opacity text. I think these are things we can reasonably ask from people who aim to render thousands of SVG nodes at 60fps.
  • Once we have Missing cursor coordinate and active tooltip state - ideally from a hook #6299 then this will get even faster. But that will be in v3.5 so I will put that in another PR.
  • We could memoize JavascriptAnimate, Label, and CartesianGrid components but I did not find any performance impact whatsoever so I left those as-is.

Related Issue

I think this quite significantly helps with #6340. I dare to say it even fixes!
I am going to test if this has any impact on #6044 edit: it helps significantly but it's not a complete fix
I will also test impact on #6624 edit: not much changed, this case needs a different approach

Motivation and Context

60fps should be enough for everyone

How Has This Been Tested?

Devtools, I don't have automated tests for this unfortunately.

Screenshots (if appropriate):

Here is comparison to Recharts v2 on 6x slowdown. Notice that v2 comes with throttle built-in somehow, my branch was without throttle. I think this is about even BUT BONUS the latest variant can keep up with the label rendering too:
https://github.com/user-attachments/assets/bebe3e98-ee7a-443d-98cc-60ee08760233

Comparison to Recharts v3 on 6x slowdown. I think we can agree there is some improvement:
https://github.com/user-attachments/assets/39b591d2-ee81-44db-9072-38c8685b61cc

If you wish to replicate the v2 throttle you can do that too, here is a comparison on 20x slowdown. I think these are about even?
https://github.com/user-attachments/assets/8601c8f9-b658-48d4-97b4-626617d77c8b

Here is an example of what the throttle could look like:

function useThrottledState(defaultValue?: TooltipIndex) {
  const [state, setState] = React.useState<TooltipIndex | undefined>(defaultValue);
  const throttledSetState = useMemo(
    () =>
      throttle(
        props => {
          setState(Number.isNaN(props.activeTooltipIndex) ? undefined : Number(props.activeTooltipIndex));
        },
        100,
        { edges: ['trailing'] },
      ),
    [setState],
  );

  const clearState = useCallback(() => {
    setState(undefined);
  }, [setState]);
  return [state, throttledSetState, clearState] as const;
}

Here is the SVG calculation change, before (400+ms) and after (30+ms):
Screenshot 2025-11-15 at 22 08 43
Screenshot 2025-11-15 at 22 08 00

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

    • Added Target Price Chart example demonstrating active labels in composed charts.

  • Bug Fixes

    • Improved animation state consistency when data changes.
    • Enhanced mouse event timing to prevent tooltip flickering and improve responsiveness.
    • Refined axis configuration updates to properly handle ID changes.

  • Documentation

    • Added testing guidelines for tooltip behavior with timing considerations.

  • Tests

    • Expanded middleware test coverage for event handling.
    • Added test cases for dynamic axis configuration changes.
    • Updated test synchronization for asynchronous operations.

@coderabbitai

coderabbitai Bot commented Nov 15, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor

Walkthrough

This pull request changes animation input stability, refactors axis lifecycle to track previous settings and use replace semantics, adds replace reducers for axes and legend payloads, debounces external and mouse events using per-event RAF scheduling, adjusts SVG utilities, memoizes a graphical item export, updates many tests to use act()/RAF-aware helpers, and adds a new TargetPriceChart example and tests.

Changes

Cohort / File(s) Summary
Animation input memoization
src/cartesian/Area.tsx, src/cartesian/Line.tsx		 Use stable memoized objects (useMemo) for animation inputs and pass them to useAnimationId to change the identity basis for animation IDs.
Axis lifecycle management
src/cartesian/XAxis.tsx, src/cartesian/YAxis.tsx, src/cartesian/ZAxis.tsx		 Track previous axis settings via refs and switch from unconditional add/remove to add-on-first-mount, replace-on-change (dispatch replaceXAxis/replaceYAxis/replaceZAxis), and remove-previous-on-unmount semantics.
Redux axis reducers
src/state/cartesianAxisSlice.ts		 Add reducers replaceXAxis, replaceYAxis, replaceZAxis accepting { prev, next } to replace axis settings by id and export them.
Legend and graphical item state management
src/state/SetGraphicalItem.ts, src/state/SetLegendPayload.ts, src/state/legendSlice.ts		 Export SetCartesianGraphicalItem as a memoized const; track prev legend payloads via refs and replace-on-change/remove-on-unmount; add replaceLegendPayload reducer to legend slice.
Event middleware RAF handling
src/state/externalEventsMiddleware.ts, src/state/mouseEventsMiddleware.ts		 externalEventsMiddleware: per-event-type RAF map, persist event, capture latest state inside RAF, cancel per-event RAFs; mouseEventsMiddleware: debounce mousemove via RAF per chart, defer dispatchs to RAF.
Redux devtools and SVG utilities
src/state/reduxDevtoolsJsonStringifyReplacer.ts, src/util/svgPropertiesAndEvents.ts, src/util/svgPropertiesNoEvents.ts		 Renamed replacer param and mask children; replaced Object.fromEntries/filter with explicit for-in loops; introduced Set-based lookup for SVG prop keys and defensive handling for non-object inputs.
Memoized graphical item export
src/state/SetGraphicalItem.ts		 Changed exported SetCartesianGraphicalItem from function to const memo-wrapped component (memo(SetCartesianGraphicalItemImpl)).
Test infra & helpers
test/helper/createSelectorTestCase, test/component/Tooltip/tooltipTestHelpers.tsx, test/README.md		 Export rechartsTestRender; add showTooltip/hideTooltip helpers; wrap timer flushes in act(); document RAF timing for tooltip tests.
Test timing updates (many tests)
test/cartesian/*, test/chart/*.spec.tsx, test/component/Tooltip/*.spec.tsx, test/state/*.spec.ts, test-vr/tests/www/ComposedChartApiExamples.spec-vr.tsx		 Add/adjust act usage and wrap vi.runOnlyPendingTimers() inside act() after events; replace some mouseOver with mouseEnter; add new VR test and a TargetPriceChart test.
New middleware tests
test/state/externalEventsMiddleware.spec.ts, test/state/mouseEventsMiddleware.spec.ts		 Add unit tests covering RAF scheduling/cancellation, event persistence, per-event-type independent RAFs, and mousemove debouncing behavior.
New example: TargetPriceChart
www/src/docs/exampleComponents/ComposedChart/TargetPriceChart.tsx, www/src/docs/exampleComponents/ComposedChart/index.ts, www/src/navigation.data.ts		 Add TargetPriceChart component and data, register example and navigation entry, and add a visual regression test entry.
Misc tests & small docs
test/helper/selectorRecalculationDebugger.ts, test/cartesian/ZAxis.spec.tsx, test/cartesian/XAxis.spec.tsx, test/cartesian/YAxis/YAxis.spec.tsx		 Add/adjust tests to verify old-ID removal when ID changes; update test helper exports and usages.

Sequence Diagram(s)

sequenceDiagram
    participant Component
    participant Middleware
    participant BrowserRAF as Browser (rAF)
    participant Handler

    rect `#DDEBF7`
    note right of Component: Event dispatch (user/DOM)
    Component->>Middleware: dispatch externalEventAction(payload)
    Middleware->>Middleware: persist reactEvent
    Middleware->>BrowserRAF: requestAnimationFrame(callback)  -- store rafId by event type
    end

    rect `#F7F3DD`
    note right of BrowserRAF: On next frame
    BrowserRAF->>Middleware: invoke callback
    Middleware->>Middleware: read latest state (getState())
    Middleware->>Handler: call handler(latestState, reactEvent)
    Middleware->>Middleware: remove rafId entry for event type
    Handler->>Component: dispatches updates -> re-render
    end
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Focus review on:

  • externalEventsMiddleware.ts: per-event-type RAF lifecycle, event persistence and cancellation semantics.
  • mouseEventsMiddleware.ts: RAF debouncing and correctness of deferred axis/tooltip dispatches.
  • XAxis/YAxis/ZAxis refactor: prevSettingsRef capture/update/cleanup correctness and replace dispatch payload shapes.
  • cartesianAxisSlice.ts and legendSlice.ts: new replace reducers and prepare/payload shapes.
  • Test changes: ensure act-wrapped timer flushes are correctly placed and tests still validate intended behavior.

Possibly related PRs

  • zIndex #6479 — overlapping changes around z-index layering and cartesian components (shared files like Area.tsx, Line.tsx).
  • Fixing visual regressions #6553 — touches cartesian axis components and tick rendering; likely related to axis lifecycle and rendering changes.

Suggested labels

enhancement

Suggested reviewers

  • ckifer

Pre-merge checks and finishing touches

❌ Failed checks (1 warning, 1 inconclusive)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 16.67% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
Title check ❓ Inconclusive The title 'Performance improvements' is vague and does not convey specific details about which performance optimizations are included. Consider using a more specific title that highlights the main changes, such as 'Optimize animation dependencies and SVG prop filtering' or 'Reduce selector recalculations and improve rendering performance'.
✅ Passed checks (1 passed)
Check name Status Explanation
Description check ✅ Passed The PR description comprehensively covers all major sections of the template with detailed context on performance optimizations.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch label-perf

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.

❤️ Share

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

@codecov

codecov Bot commented Nov 15, 2025 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 96.92671% with 13 lines in your changes missing coverage. Please review.
✅ Project coverage is 94.15%. Comparing base (3c42b46) to head (9870814).
⚠️ Report is 5 commits behind head on main.

Files with missing lines Patch % Lines
...ampleComponents/ComposedChart/TargetPriceChart.tsx 93.61% 12 Missing ⚠️
src/state/reduxDevtoolsJsonStringifyReplacer.ts 75.00% 1 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #6634      +/-   ##
==========================================
- Coverage   94.15%   94.15%   -0.01%     
==========================================
  Files         493      494       +1     
  Lines       41104    41458     +354     
  Branches     4783     4826      +43     
==========================================
+ Hits        38700    39033     +333     
- Misses       2399     2420      +21     
  Partials        5        5

☔ 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.

coderabbitai[bot]
coderabbitai Bot reviewed Nov 15, 2025
View reviewed changes

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 7

🧹 Nitpick comments (5)
test/chart/BarChart.spec.tsx (1)

3-3: BarChart tests now correctly synchronize with debounced mouse/tooltip updates

  • Importing act and wrapping vi.runOnlyPendingTimers() in assertActiveBarInteractions and the chart synchronization tests aligns these specs with the new RAF-based mouse middleware and prevents React act warnings.
  • The initial expect in assertActiveBarInteractions (no active bar before hover) is a nice sanity check.

For full consistency, you might later also wrap the remaining vi.advanceTimersByTime(...) calls in this file with act(() => ...), since they now indirectly drive the same debounced mouse/tooltip work.

Also applies to: 64-79, 3533-3586, 3614-3630

src/state/externalEventsMiddleware.ts (1)

38-75: State capture inside RAF looks correct; consider defensive cleanup

Capturing state inside the requestAnimationFrame callback and building MouseHandlerDataParam there ensures handlers see the latest tooltip state, which is exactly what you want with queued mouse events. One small defensive tweak would be to ensure the rafIdMap entry is always cleared even if the handler throws:

-    const rafId = requestAnimationFrame(() => {
-      /*
-       * Here it is important that we get the latest state inside the animation frame callback,
-       * not from the outer scope, because there may have been other actions dispatched
-       * between the time the event was fired and the animation frame callback is executed.
-       * One of those actions is the one that actually sets the active tooltip state!
-       */
-      const state: RechartsRootState = listenerApi.getState();
-      const nextState: MouseHandlerDataParam = {
-        activeCoordinate: selectActiveTooltipCoordinate(state),
-        activeDataKey: selectActiveTooltipDataKey(state),
-        activeIndex: selectActiveTooltipIndex(state),
-        activeLabel: selectActiveLabel(state),
-        activeTooltipIndex: selectActiveTooltipIndex(state),
-        isTooltipActive: selectIsTooltipActive(state),
-      };
-
-      handler(nextState, reactEvent);
-
-      rafIdMap.delete(eventType);
-    });
+    const rafId = requestAnimationFrame(() => {
+      try {
+        const state: RechartsRootState = listenerApi.getState();
+        const nextState: MouseHandlerDataParam = {
+          activeCoordinate: selectActiveTooltipCoordinate(state),
+          activeDataKey: selectActiveTooltipDataKey(state),
+          activeIndex: selectActiveTooltipIndex(state),
+          activeLabel: selectActiveLabel(state),
+          activeTooltipIndex: selectActiveTooltipIndex(state),
+          isTooltipActive: selectIsTooltipActive(state),
+        };
+
+        handler(nextState, reactEvent);
+      } finally {
+        rafIdMap.delete(eventType);
+      }
+    });

Functionally you’re already fine, but this closes a small edge case.

src/state/SetLegendPayload.ts (1)

11-23: Legend payload lifecycle is well handled; note referential equality assumption

The prevPayloadRef + useLayoutEffect pattern cleanly switches legend updates from add+remove to add+replace+remove in both cartesian and polar cases, and the panorama/layout guards avoid unnecessary work.

One minor implication: the replaceLegendPayload branch is driven purely by referential equality (prevPayloadRef.current !== legendPayload). That’s fine given the ReadonlyArray<LegendPayload> type, but if callers ever mutate arrays in place instead of passing new references, changes won’t be detected. If that’s a concern, consider documenting the immutability expectation or doing a shallow equality check instead.

Also applies to: 25-32, 40-52, 54-61

www/src/docs/exampleComponents/ComposedChart/TargetPriceChart.tsx (2)

138-153: Handle null/undefined activeTooltipIndex instead of coercing to a number

useActiveIndexState currently coerces activeTooltipIndex with Number(...), which will turn undefined into NaN and null into 0. That can briefly highlight the wrong index or leave activeIndex in a NaN state.

You can keep the API the same but treat “no active tooltip” as undefined:

-  const setActiveIndex = useCallback(
-    (props: MouseHandlerDataParam) => {
-      setState(Number(props.activeTooltipIndex));
-    },
-    [setState],
-  );
+  const setActiveIndex = useCallback(
+    (props: MouseHandlerDataParam) => {
+      const index = props.activeTooltipIndex;
+      setState(index == null ? undefined : Number(index));
+    },
+    [setState],
+  );

This keeps activeIndex unset when there’s no meaningful tooltip index.

167-183: Optional: derive X-axis domain from data to avoid future drift

domain={[startingTimestamp, endingTimestamp]} is currently kept in sync with chartData via exported constants. If the sample data is edited later, it’s easy to forget updating these exports.

If you want to make the example more self-maintaining, you could instead derive the domain directly from chartData or use 'dataMin' | 'dataMax':

-        domain={[startingTimestamp, endingTimestamp]}
+        domain={['dataMin', 'dataMax']}

or

-        domain={[startingTimestamp, endingTimestamp]}
+        domain={[chartData[0]?.timestamp, chartData[chartData.length - 1]?.timestamp]}

Not required, but simplifies future tweaks to the dataset.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 3c42b46 and 31b75c6.

⛔ Files ignored due to path filters (4)
  • test-vr/__snapshots__/tests/www/ComposedChartApiExamples.spec-vr.tsx-snapshots/TargetPriceChart-1-chromium-linux.png is excluded by !**/*.png
  • test-vr/__snapshots__/tests/www/ComposedChartApiExamples.spec-vr.tsx-snapshots/TargetPriceChart-1-firefox-linux.png is excluded by !**/*.png
  • test-vr/__snapshots__/tests/www/ComposedChartApiExamples.spec-vr.tsx-snapshots/TargetPriceChart-1-webkit-linux.png is excluded by !**/*.png
  • www/test/__snapshots__/navigation.spec.ts.snap is excluded by !**/*.snap
📒 Files selected for processing (37)
  • src/cartesian/Area.tsx (2 hunks)
  • src/cartesian/Line.tsx (1 hunks)
  • src/cartesian/XAxis.tsx (2 hunks)
  • src/cartesian/YAxis.tsx (2 hunks)
  • src/cartesian/ZAxis.tsx (1 hunks)
  • src/state/SetGraphicalItem.ts (3 hunks)
  • src/state/SetLegendPayload.ts (1 hunks)
  • src/state/cartesianAxisSlice.ts (4 hunks)
  • src/state/externalEventsMiddleware.ts (1 hunks)
  • src/state/legendSlice.ts (2 hunks)
  • src/state/mouseEventsMiddleware.ts (1 hunks)
  • src/state/reduxDevtoolsJsonStringifyReplacer.ts (1 hunks)
  • src/util/svgPropertiesAndEvents.ts (1 hunks)
  • src/util/svgPropertiesNoEvents.ts (2 hunks)
  • test-vr/tests/www/ComposedChartApiExamples.spec-vr.tsx (1 hunks)
  • test/README.md (1 hunks)
  • test/cartesian/XAxis.spec.tsx (2 hunks)
  • test/cartesian/YAxis/YAxis.spec.tsx (2 hunks)
  • test/chart/AreaChart.spec.tsx (4 hunks)
  • test/chart/BarChart.spec.tsx (5 hunks)
  • test/chart/CategoricalChart.spec.tsx (4 hunks)
  • test/chart/ComposedChart.spec.tsx (2 hunks)
  • test/chart/FunnelChart.spec.tsx (2 hunks)
  • test/chart/LineChart.spec.tsx (4 hunks)
  • test/chart/PieChart.spec.tsx (4 hunks)
  • test/chart/RadialBarChart.spec.tsx (2 hunks)
  • test/chart/RechartsWrapper.spec.tsx (2 hunks)
  • test/chart/chartEvents.spec.tsx (8 hunks)
  • test/component/Tooltip/Tooltip.visibility.spec.tsx (4 hunks)
  • test/component/Tooltip/tooltipEventType.spec.tsx (3 hunks)
  • test/component/Tooltip/tooltipTestHelpers.tsx (2 hunks)
  • test/helper/selectorRecalculationDebugger.ts (1 hunks)
  • test/state/externalEventsMiddleware.spec.ts (1 hunks)
  • test/state/mouseEventsMiddleware.spec.ts (1 hunks)
  • www/src/docs/exampleComponents/ComposedChart/TargetPriceChart.tsx (1 hunks)
  • www/src/docs/exampleComponents/ComposedChart/index.ts (2 hunks)
  • www/src/navigation.data.ts (1 hunks)
🧰 Additional context used
🧬 Code graph analysis (23)
src/cartesian/Line.tsx (1)
src/util/useAnimationId.tsx (1)
  • useAnimationId (18-28)
src/state/mouseEventsMiddleware.ts (6)
src/util/types.ts (1)
  • MousePointer (1056-1060)
src/state/store.ts (2)
  • RechartsRootState (94-94)
  • AppDispatch (95-95)
src/util/getChartPointer.ts (1)
  • getChartPointer (16-34)
src/state/selectors/selectTooltipEventType.ts (1)
  • selectTooltipEventType (27-31)
src/state/selectors/selectActivePropsFromChartPointer.ts (1)
  • selectActivePropsFromChartPointer (14-29)
src/state/tooltipSlice.ts (2)
  • setMouseOverAxisIndex (328-335)
  • mouseLeaveChart (306-316)
src/cartesian/Area.tsx (1)
src/util/useAnimationId.tsx (1)
  • useAnimationId (18-28)
test/chart/PieChart.spec.tsx (1)
storybook/stories/API/props/EventHandlers.ts (2)
  • onMouseEnter (106-106)
  • onMouseLeave (107-107)
src/state/externalEventsMiddleware.ts (3)
src/state/store.ts (2)
  • RechartsRootState (94-94)
  • AppDispatch (95-95)
src/synchronisation/types.ts (1)
  • MouseHandlerDataParam (4-20)
src/state/selectors/tooltipSelectors.ts (5)
  • selectActiveTooltipCoordinate (435-444)
  • selectActiveTooltipDataKey (405-414)
  • selectActiveTooltipIndex (395-398)
  • selectActiveLabel (400-403)
  • selectIsTooltipActive (446-449)
src/state/legendSlice.ts (1)
src/component/DefaultLegendContent.tsx (1)
  • LegendPayload (26-42)
test/chart/BarChart.spec.tsx (1)
test/component/Tooltip/tooltipTestHelpers.tsx (1)
  • expectTooltipPayload (140-152)
src/state/SetLegendPayload.ts (4)
src/component/DefaultLegendContent.tsx (1)
  • LegendPayload (26-42)
src/state/hooks.ts (2)
  • useAppDispatch (9-15)
  • useAppSelector (40-50)
src/context/PanoramaContext.tsx (1)
  • useIsPanorama (6-6)
src/context/chartLayoutContext.tsx (1)
  • selectChartLayout (133-133)
www/src/docs/exampleComponents/ComposedChart/index.ts (1)
www/src/docs/exampleComponents/ComposedChart/TargetPriceChart.tsx (1)
  • TargetPriceChart (157-224)
test/chart/CategoricalChart.spec.tsx (1)
test/helper/createSelectorTestCase.tsx (1)
  • rechartsTestRender (172-184)
src/util/svgPropertiesAndEvents.ts (2)
src/util/svgPropertiesNoEvents.ts (2)
  • isSvgElementPropKey (318-323)
  • isDataAttribute (334-336)
src/util/excludeEventProps.ts (1)
  • isEventKey (167-173)
src/cartesian/XAxis.tsx (1)
src/state/cartesianAxisSlice.ts (1)
  • XAxisSettings (74-78)
test/component/Tooltip/Tooltip.visibility.spec.tsx (3)
src/component/TooltipBoundingBox.tsx (1)
  • render (78-145)
test/component/Tooltip/tooltipTestHelpers.tsx (1)
  • showTooltipOnCoordinate (78-85)
test/component/Tooltip/tooltipMouseHoverSelectors.ts (1)
  • lineChartMouseHoverTooltipSelector (26-26)
test/cartesian/XAxis.spec.tsx (1)
test/helper/createSelectorTestCase.tsx (1)
  • rechartsTestRender (172-184)
test/component/Tooltip/tooltipEventType.spec.tsx (2)
test/component/Tooltip/tooltipTestHelpers.tsx (2)
  • showTooltip (120-122)
  • hideTooltip (124-128)
test/component/Tooltip/tooltipMouseHoverSelectors.ts (1)
  • barChartMouseHoverTooltipSelector (18-18)
src/state/SetGraphicalItem.ts (1)
src/state/graphicalItemsSlice.ts (1)
  • CartesianGraphicalItemSettings (59-59)
test/cartesian/YAxis/YAxis.spec.tsx (1)
test/helper/createSelectorTestCase.tsx (1)
  • rechartsTestRender (172-184)
test-vr/tests/www/ComposedChartApiExamples.spec-vr.tsx (1)
www/src/docs/exampleComponents/ComposedChart/TargetPriceChart.tsx (1)
  • TargetPriceChart (157-224)
src/cartesian/ZAxis.tsx (2)
src/state/cartesianAxisSlice.ts (1)
  • ZAxisSettings (93-95)
src/state/hooks.ts (1)
  • useAppDispatch (9-15)
www/src/docs/exampleComponents/ComposedChart/TargetPriceChart.tsx (1)
storybook/stories/API/props/AnimationProps.ts (1)
  • isAnimationActive (29-36)
test/state/mouseEventsMiddleware.spec.ts (4)
src/util/types.ts (1)
  • MousePointer (1056-1060)
test/helper/mockGetBoundingClientRect.ts (1)
  • getMockDomRect (5-20)
src/state/store.ts (1)
  • createRechartsStore (40-92)
src/state/mouseEventsMiddleware.ts (2)
  • mouseClickAction (10-10)
  • mouseMoveAction (32-32)
src/cartesian/YAxis.tsx (1)
src/state/cartesianAxisSlice.ts (1)
  • YAxisSettings (82-87)
test/state/externalEventsMiddleware.spec.ts (2)
src/state/store.ts (2)
  • RechartsRootState (94-94)
  • createRechartsStore (40-92)
src/state/externalEventsMiddleware.ts (1)
  • externalEventAction (19-19)
🪛 LanguageTool
test/README.md

[style] ~99-~99: ‘take into account’ might be wordy. Consider a shorter alternative.
Context: ...t tooltips appear on hover, you need to take into account that the mouse move middleware is hidde...

(EN_WORDINESS_PREMIUM_TAKE_INTO_ACCOUNT)

⏰ 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 (33)
test/helper/selectorRecalculationDebugger.ts (1)

7-8: Documentation clarification improves API usability.

The example is now clearer, showing the simplified and correct usage pattern: pass only the selector name to create a debug function, then invoke that function with the dependency object inside your selector. This aligns with the actual API and makes the utility more approachable for developers.

src/state/reduxDevtoolsJsonStringifyReplacer.ts (2)

1-1: LGTM! Parameter rename is correct.

The rename from _key to key is appropriate since the parameter is now actively used in the children filtering logic below.

8-10: LGTM! Children filtering improves DevTools performance.

The addition correctly prevents serialization of complex React element trees when the key is 'children'. The null check is necessary since typeof null === 'object' in JavaScript, and the logic appropriately handles both object and array children types.

src/util/svgPropertiesNoEvents.ts (3)

316-317: LGTM: Excellent Set-based optimization.

Creating a Set from the property keys array enables O(1) lookup performance instead of O(n), a clear win for the repeated lookups in isSvgElementPropKey.

322-322: LGTM: Correct Set lookup implementation.

The Set.has() method provides the intended O(1) lookup performance.

344-356: LGTM: Clean for...in optimization with defensive guards.

The manual property iteration is significantly faster than Object.entries + filter + fromEntries. The guard correctly handles non-object inputs (including null, since typeof null === 'object'), and the hasOwnProperty check safely filters inherited properties.

src/util/svgPropertiesAndEvents.ts (1)

20-30: LGTM: Consistent for...in optimization.

This implementation mirrors the same performance optimization pattern used in svgPropertiesNoEvents, replacing the Object.entries-based approach with a direct for...in loop. The comment documenting the 10x performance improvement is helpful context.

src/cartesian/Area.tsx (1)

460-461: LGTM! Animation input correctly memoized.

The memoization prevents creating a new object reference on every render, ensuring useAnimationId only generates a new ID when points or baseLine actually change. This aligns well with the PR's goal of limiting unnecessary animation triggers.

src/cartesian/Line.tsx (1)

379-379: Selector is properly memoized—no changes needed.

The selectLinePoints selector uses createSelector from reselect, which provides built-in memoization. It returns the same array reference as long as its dependencies remain unchanged, satisfying the reference equality check required by useAnimationId. The animation ID dependency is stable and correct.

src/state/SetGraphicalItem.ts (2)

1-1: LGTM: Import addition supports memoization.

The addition of memo to the React imports is necessary and correct for the memoization applied on line 51.

13-49: LGTM: Internal implementation preserves original logic.

The refactoring of the original function to an internal implementation is clean and preserves all the original behavior, including the StrictMode-aware cleanup logic.

test/chart/RechartsWrapper.spec.tsx (1)

3-3: LGTM! Proper test synchronization with RAF-batched events.

The addition of act() blocks around timer flushing correctly synchronizes test assertions with the RAF-batched mouse event handlers introduced in this PR. This follows React Testing Library best practices.

Also applies to: 24-26, 33-35

test/chart/RadialBarChart.spec.tsx (1)

2-2: LGTM! Consistent test synchronization pattern.

The act() wrappers around timer flushing after mouseOver events ensure that active shape updates are processed before assertions, aligning with the RAF-batched event handling.

Also applies to: 19-29

test/chart/FunnelChart.spec.tsx (1)

2-2: LGTM! Event synchronization properly applied.

The act() wrapper ensures all event types (click, mouseEnter, mouseMove, mouseLeave) are processed through the RAF queue before assertions execute.

Also applies to: 100-102

test/chart/chartEvents.spec.tsx (1)

2-2: LGTM! Comprehensive event synchronization coverage.

Excellent test coverage with act() blocks applied consistently across all event types (mouse, touch, click). The addition of the pre-interaction assertion at line 240 is good practice, verifying the initial state before the event fires.

Also applies to: 70-73, 87-104, 120-141, 213-215, 240-270

test-vr/tests/www/ComposedChartApiExamples.spec-vr.tsx (1)

12-17: LGTM! VR test coverage for new example component.

The test properly disables animation for deterministic screenshot comparison and sets a specific defaultIndex to capture a meaningful state of the chart.

www/src/navigation.data.ts (1)

122-122: LGTM! New example registered in navigation.

The TargetPriceChart is properly added to the example components list, making it accessible in the documentation UI.

test/README.md (1)

97-102: LGTM! Clear documentation of tooltip testing pattern.

The new section clearly explains the RAF-based middleware behavior and provides practical solutions for testing tooltips. This will help developers write correct tests that synchronize with the new async event handling.

test/chart/ComposedChart.spec.tsx (1)

2-2: LGTM! Correct event type and proper synchronization.

The change from mouseOver to mouseEnter is semantically more appropriate for chart interaction testing, and the act() wrapper ensures proper synchronization with RAF-batched event handling.

Also applies to: 72-76

test/component/Tooltip/tooltipTestHelpers.tsx (1)

2-2: Wrapping timer flush in act looks correct

Using act(() => vi.runOnlyPendingTimers()) here is a good match for the new RAF-based tooltip behavior and avoids act warnings while keeping semantics identical.

If any tests call these helpers without vi.useFakeTimers() (or an equivalent global setup), vi.runOnlyPendingTimers() will throw—please confirm timers are always faked before using these helpers.

Also applies to: 54-60

test/chart/AreaChart.spec.tsx (1)

1-1: Timer flush wrapped in act is appropriate for RAF‑driven updates

The new act(() => vi.runOnlyPendingTimers()) calls after mouse interactions correctly synchronize tests with debounced tooltip/active-dot updates.

Please ensure this suite (or global test setup) enables fake timers before calling vi.runOnlyPendingTimers(), otherwise Vitest will error when these tests run.

Also applies to: 99-101, 125-127, 761-763

test/cartesian/XAxis.spec.tsx (1)

52-52: Using rechartsTestRender for the XAxis removal test is a good fit

Switching to rechartsTestRender keeps the same chart/store instance across rerenders, which matches the intent of verifying config removal from the store while simplifying the test API.

Also applies to: 2035-2035

test/chart/CategoricalChart.spec.tsx (1)

3-3: Reusing rechartsTestRender and syncing mouse events via act makes sense

Rendering through rechartsTestRender and then flushing pending timers inside act after the click/dblclick/contextmenu sequence matches the new debounced event handling and keeps the test harness consistent with other suites.

As with the other updated tests, please double‑check that fake timers are enabled before vi.runOnlyPendingTimers() is called in this file (either locally or via global setup).

Also applies to: 25-25, 189-189, 193-199, 207-209

src/state/mouseEventsMiddleware.ts (1)

36-45: RAF debouncing is good, but shared rafId may misbehave with multiple charts

The new debounced mouse-move handling is a solid perf win: you cancel the previous frame, compute chartPointer once, and only dispatch axis tooltip updates once per animation frame for the latest pointer position.

The risky bit is the module‑level rafId:

  • If a single mouseMoveMiddleware instance is ever attached to more than one Redux store (i.e., more than one chart sharing this middleware object), mouse moves in chart A can cancel the scheduled RAF for chart B, so only one chart’s axis state is updated per frame.
  • The comment claims “each chart has its own Redux store instance with its own middleware”; that’s only safe if each store gets its own listener middleware instance (and thus its own rafId) rather than all stores importing and reusing this singleton.

Please verify how mouseMoveMiddleware is wired in store.ts:

  • If the same exported mouseMoveMiddleware.middleware is reused across multiple chart stores, consider scoping rafId per store (e.g., by creating the listener middleware inside the store factory) or keeping a per‑store rafId map instead of a single shared variable.
  • If each chart truly gets its own middleware instance, it would be helpful to document that invariant here so future refactors don’t accidentally introduce cross‑chart interference.

Also applies to: 51-79

src/state/legendSlice.ts (1)

65-83: Legend payload swap strategy looks solid

Thanks for introducing the replace path that mirrors the axis slices—keeping the list stable while swapping the contents should cut down on needless add/remove churn, and the current lookup matches the existing removal pattern nicely.

src/cartesian/YAxis.tsx (1)

57-78: Lower churn on YAxis registrations

Caching the previous settings and leaning on replaceYAxis keeps the registration stable across renders, so we sidestep the old add/remove churn and the downstream selector invalidations. Nice clean improvement.

test/component/Tooltip/tooltipEventType.spec.tsx (1)

161-168: LGTM! Test helper usage improves consistency.

The refactoring to use showTooltip and hideTooltip helpers instead of direct DOM events improves test maintainability and ensures consistent RAF timing handling across the test suite.

test/component/Tooltip/Tooltip.visibility.spec.tsx (2)

432-434: LGTM! Proper act() usage for timer advancement.

Wrapping vi.runOnlyPendingTimers() in act() ensures React processes all state updates triggered by the RAF callback before assertions run. This is the correct pattern for testing RAF-based debouncing.

1583-1591: LGTM! Helper usage with debug support.

The refactoring to use showTooltipOnCoordinate with the optional debug parameter improves test maintainability and provides better debugging capability when tests fail.

test/state/mouseEventsMiddleware.spec.ts (1)

1-81: LGTM! Comprehensive middleware timing tests.

This test suite effectively validates the key performance optimization:

  • Click events process synchronously without RAF overhead
  • Mouse move events debounce via RAF, canceling previous pending frames when new moves arrive
  • Proper use of act() ensures timer execution is synchronized with React

The test expectations align with the PR's RAF-based debouncing strategy for mouse movements.

test/state/externalEventsMiddleware.spec.ts (1)

1-183: LGTM! Excellent test coverage of RAF scheduling semantics.

This comprehensive test suite validates:

  • Handler invocation timing and payload shape
  • React 16 compatibility via event.persist()
  • Per-event-type RAF scheduling (different event types have independent timers)
  • Proper cancellation when same event type is dispatched multiple times

The test expectations clearly document the middleware's behavior and provide confidence in the RAF-based batching implementation.

www/src/docs/exampleComponents/ComposedChart/index.ts (1)

15-16: LGTM! New example follows established pattern.

The TargetPriceChart example is correctly integrated into the examples registry, following the same structure as existing entries.

Also applies to: 51-55

src/state/externalEventsMiddleware.ts (1)

23-31: Per-event-type RAF map is a solid improvement

Using rafIdMap keyed by eventType prevents different DOM event types from cancelling each other’s frames while still allowing debouncing per type. This aligns well with the middleware’s multi-event responsibilities.

Comment thread src/cartesian/XAxis.tsx
Comment thread src/cartesian/ZAxis.tsx Outdated
Comment thread src/state/cartesianAxisSlice.ts
Comment thread src/state/cartesianAxisSlice.ts
Comment thread src/state/cartesianAxisSlice.ts
Comment thread src/state/SetGraphicalItem.ts
Comment thread www/src/docs/exampleComponents/ComposedChart/TargetPriceChart.tsx Outdated
@PavelVanecek PavelVanecek mentioned this pull request Nov 15, 2025
Closed
@codecov

codecov Bot commented Nov 15, 2025 •
edited
Loading

Copy link
Copy Markdown

Bundle Report

Changes will increase total bundle size by 15.77kB (0.61%) ⬆️. This is within the configured threshold ✅

Detailed changes
Bundle name Size Change
recharts/bundle-cjs 1.13MB 7.2kB (0.64%) ⬆️
recharts/bundle-es6 975.69kB 6.67kB (0.69%) ⬆️
recharts/bundle-umd 512.05kB 1.9kB (0.37%) ⬆️

Affected Assets, Files, and Routes:

view changes for bundle: recharts/bundle-cjs

Assets Changed:

Asset Name Size Change Total Size Change (%)
cartesian/Area.js 113 bytes 27.24kB 0.42%
cartesian/Line.js 1 bytes 24.46kB 0.0%
cartesian/YAxis.js 483 bytes 9.69kB 5.24% ⚠️
cartesian/XAxis.js 483 bytes 8.19kB 6.27% ⚠️
util/svgPropertiesNoEvents.js 168 bytes 7.11kB 2.42%
state/cartesianAxisSlice.js 1.43kB 6.26kB 29.71% ⚠️
state/mouseEventsMiddleware.js 791 bytes 3.58kB 28.34% ⚠️
cartesian/ZAxis.js 489 bytes 2.72kB 21.93% ⚠️
state/externalEventsMiddleware.js 1.45kB 2.7kB 115.05% ⚠️
state/legendSlice.js 499 bytes 2.68kB 22.91% ⚠️
state/SetLegendPayload.js 893 bytes 2.41kB 58.87% ⚠️
util/svgPropertiesAndEvents.js 181 bytes 2.41kB 8.12% ⚠️
state/SetGraphicalItem.js 115 bytes 2.24kB 5.41% ⚠️
state/reduxDevtoolsJsonStringifyReplacer.js 105 bytes 536 bytes 24.36% ⚠️
view changes for bundle: recharts/bundle-es6

Assets Changed:

Asset Name Size Change Total Size Change (%)
cartesian/Area.js 110 bytes 25.38kB 0.44%
cartesian/Line.js 1 bytes 22.95kB 0.0%
cartesian/YAxis.js 448 bytes 8.39kB 5.64% ⚠️
cartesian/XAxis.js 456 bytes 6.98kB 6.99% ⚠️
util/svgPropertiesNoEvents.js 168 bytes 6.83kB 2.52%
state/cartesianAxisSlice.js 1.18kB 5.33kB 28.3% ⚠️
state/mouseEventsMiddleware.js 791 bytes 3.11kB 34.12% ⚠️
state/externalEventsMiddleware.js 1.45kB 2.47kB 140.84% ⚠️
util/svgPropertiesAndEvents.js 181 bytes 2.13kB 9.28% ⚠️
state/legendSlice.js 363 bytes 2.09kB 21.07% ⚠️
state/SetLegendPayload.js 866 bytes 2.07kB 71.69% ⚠️
state/SetGraphicalItem.js 92 bytes 1.98kB 4.88%
cartesian/ZAxis.js 468 bytes 1.76kB 36.31% ⚠️
state/reduxDevtoolsJsonStringifyReplacer.js 105 bytes 382 bytes 37.91% ⚠️
view changes for bundle: recharts/bundle-umd

Assets Changed:

Asset Name Size Change Total Size Change (%)
Recharts.js 1.9kB 512.05kB 0.37%

@PavelVanecek
PavelVanecek requested a review from ckifer November 15, 2025 15:14
@github-actions

github-actions Bot commented Nov 15, 2025

Copy link
Copy Markdown
Contributor

Visual Regression tests failed.

To update snapshots: Comment /update-snapshots on this PR to automatically update the baseline screenshots.

ckifer
ckifer approved these changes Nov 16, 2025
View reviewed changes
Comment thread src/util/svgPropertiesAndEvents.ts
const result: Record<PropertyKey, any> = {};
// for ... in loop is 10x faster than Object.entries + filter + Object.fromEntries in Chrome
// eslint-disable-next-line no-restricted-syntax
for (const key in obj) {

@ckifer ckifer Nov 16, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whew

@ckifer
ckifer merged commit e95a710 into main Nov 16, 2025
28 of 29 checks passed
@ckifer
ckifer deleted the label-perf branch November 16, 2025 05:54
@coderabbitai coderabbitai Bot mentioned this pull request Nov 16, 2025
Merged
@PavelVanecek PavelVanecek mentioned this pull request Nov 16, 2025
Merged
7 tasks
ckifer pushed a commit that referenced this pull request Nov 18, 2025
@PavelVanecek
Fix Line animation when interrupted with non-data related changes (#6641
f34bda0 
)

## 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:


https://github.com/user-attachments/assets/d477a374-c216-4411-8b12-47d0b8ed95f7

This is what is currently in the main branch, notice how the length
suddenly jumps:


https://github.com/user-attachments/assets/f4adab16-a884-4c76-b896-b6ee0723dd53

After this PR:


https://github.com/user-attachments/assets/469c4fb5-22c2-461f-9613-589fcff5b0ce

## Types of changes

- [x] 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.
- [x] I have added tests to cover my changes.
- [x] I have added a storybook story or VR test, or extended an existing
story or VR test to show my changes


<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->

## Summary by CodeRabbit

* **Bug Fixes**
* Fixed line animations continuing from their previous position when
strokeWidth or other properties change during playback, eliminating
unwanted animation restarts.

* **Tests**
* Added test coverage for property changes occurring during line
animation.

<!-- end of auto-generated comment: release notes by coderabbit.ai -->
@coderabbitai coderabbitai Bot mentioned this pull request Nov 23, 2025
Merged
@PavelVanecek PavelVanecek mentioned this pull request Jan 17, 2026
Closed
This was referenced Jan 24, 2026
Merged
Merged
@coderabbitai coderabbitai Bot mentioned this pull request Feb 7, 2026
Closed
7 tasks
This was referenced Feb 15, 2026
Merged
Merged
@coderabbitai coderabbitai Bot mentioned this pull request Mar 24, 2026
Open
@coderabbitai coderabbitai Bot mentioned this pull request Jun 28, 2026
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@coderabbitai coderabbitai[bot] coderabbitai[bot] left review comments
@ckifer ckifer ckifer approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@PavelVanecek @ckifer