fix(RechartsWrapper): prevent ResizeObserver memory leak on ref update#7161
Conversation
WalkthroughThe Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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.
🧹 Nitpick comments (1)
test/chart/RechartsWrapper.spec.tsx (1)
42-82: Addvi.useFakeTimers()to comply with test guidelines.Per coding guidelines, all tests should use
vi.useFakeTimers()due to Redux autoBatchEnhancer dependency on timers andrequestAnimationFrame. The existing test in this file usesvi.runOnlyPendingTimers()after renders.♻️ Proposed fix
it('should disconnect the previous ResizeObserver before creating a new one when the DOM node changes', () => { + vi.useFakeTimers(); const observeSpy = vi.fn(); const disconnectSpy = vi.fn();And add timer handling after renders:
const { rerender } = render( <RechartsWrapper responsive width={100} height={100} ref={() => {}}> <div /> </RechartsWrapper>, ); + + act(() => { + vi.runOnlyPendingTimers(); + }); expect(ResizeObserverMock).toHaveBeenCalledTimes(1);rerender( <RechartsWrapper responsive width={100} height={100} ref={() => {}}> <div /> </RechartsWrapper>, ); + + act(() => { + vi.runOnlyPendingTimers(); + }); // The test asserts that the first observer was disconnected before the second one is createdAs per coding guidelines: "Use
vi.useFakeTimers()in all tests due to Redux autoBatchEnhancer dependency on timers andrequestAnimationFrame" and "Callvi.runOnlyPendingTimers()to advance timers after renders when not usingcreateSelectorTestCasehelper".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/chart/RechartsWrapper.spec.tsx` around lines 42 - 82, Add fake timers at the start of this test and run pending timers after renders: call vi.useFakeTimers() before creating the ResizeObserver mock (e.g. at start of the "should disconnect the previous ResizeObserver..." test) and call vi.runOnlyPendingTimers() immediately after render(...) and after rerender(...) to advance requestAnimationFrame timers; also ensure vi.useRealTimers() or vi.restoreAllMocks()/vi.unstubAllGlobals() cleanup as appropriate at the end of the test to avoid leaking fake timers.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@test/chart/RechartsWrapper.spec.tsx`:
- Around line 42-82: Add fake timers at the start of this test and run pending
timers after renders: call vi.useFakeTimers() before creating the ResizeObserver
mock (e.g. at start of the "should disconnect the previous ResizeObserver..."
test) and call vi.runOnlyPendingTimers() immediately after render(...) and after
rerender(...) to advance requestAnimationFrame timers; also ensure
vi.useRealTimers() or vi.restoreAllMocks()/vi.unstubAllGlobals() cleanup as
appropriate at the end of the test to avoid leaking fake timers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 40d80ca9-9bac-4a43-bd45-457fc3c91838
📒 Files selected for processing (2)
src/chart/RechartsWrapper.tsxtest/chart/RechartsWrapper.spec.tsx
Bundle ReportChanges will decrease total bundle size by 425 bytes (-0.01%) ⬇️. 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-treeshaking-sunburstAssets Changed:
view changes for bundle: recharts/bundle-treeshaking-treemapAssets Changed:
view changes for bundle: recharts/bundle-umdAssets Changed:
view changes for bundle: recharts/bundle-treeshaking-sankeyAssets Changed:
view changes for bundle: recharts/bundle-es6Assets Changed:
|
|
Did you prove that there were resize observers piling up? |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7161 +/- ##
==========================================
+ Coverage 89.61% 89.62% +0.01%
==========================================
Files 536 536
Lines 40479 40488 +9
Branches 5519 5523 +4
==========================================
+ Hits 36275 36288 +13
+ Misses 4196 4192 -4
Partials 8 8 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| return () => { | ||
| const observer = observerRef.current; | ||
| if (observer != null) { | ||
| observer.disconnect(); |
There was a problem hiding this comment.
The useEffect here cleans up the observers. I don't think there is a memory leak
|
Hi @ckifer! That is a great point—you are totally right that the useEffect correctly handles the cleanup when the component unmounts. However, the memory leak occurs if the innerRef callback gets triggered multiple times while the component is still mounted. The Mechanism of the Leak: Every time that callback runs, it instantiates a brand new ResizeObserver and overwrites observerRef.current with the new one. Because the useEffect cleanup only fires on unmount, it will only ever disconnect the very last observer created. All the previous observers get orphaned and continue running in the background. The Proof: Before this fix: The test output shows observe() being called 2 times, but disconnect() being called 0 times during the update. After this fix: The old observer is caught and disconnected before being overwritten. Adding the cleanup directly inside the callback ensures no observers are left behind mid-lifecycle. Let me know if you'd like me to adjust anything! |
|
Adding the extra clean up is fine, but want to clarify that this "leak" isn't causing any issues/actual excessive memory usage |
|
Totally agree! It is definitely more of a "micro-leak" / edge-case cleanup rather than a massive performance blocker in real-world usage. It just ensures things stay perfectly tidy under the hood if the ref happens to get spammed. Thanks so much for taking the time to review and discuss it! Let me know if there is anything else you need from me here. |
What is the purpose of this pull request?
This PR fixes a silent memory leak in the
RechartsWrapper(specifically withinResponsiveDiv).Previously, every time the
innerRefcallback was triggered with a valid DOM node, a newResizeObserverwas instantiated and attached to the node. However, the existing observer stored inobserverRef.currentwas never disconnected, leading to detached observers piling up in memory over the lifecycle of the application if the ref updated frequently.What does this PR do?
observerRef.current.disconnect()) inside theinnerRefcallback to destroy the previous observer before initiating a new one.RechartsWrapper.spec.tsxthat mocks the globalResizeObserverand explicitly asserts thatdisconnect()is called when the component is forced to re-render and update its ref.Checklist:
mainvia CI).Summary by CodeRabbit
Bug Fixes
Tests