Fix memory leak by removing event listeners on destroy #7
Conversation
WalkthroughAdds per-instance listener and style management to the SocialShareButton component: introduces Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (5 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 |
![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 (1)
src/social-share-button.js (1)
203-241: Excellent listener tracking implementation!The
addListenerhelper effectively wraps all event registration and tracks listeners for cleanup. All listener types are properly covered: button, modal overlay, close button, platform buttons, copy button, input, and the document-level ESC key handler.💡 Optional: Consider making addListener a class method
The
addListenerhelper is currently scoped toattachEvents(). While this works for the current implementation, making it a class method would improve reusability if listeners need to be added dynamically elsewhere in the future:+ addListener(element, event, handler) { + element.addEventListener(event, handler); + this._listeners.push({ element, event, handler }); + } + attachEvents() { - // Helper to track listeners - const addListener = (element, event, handler) => { - element.addEventListener(event, handler); - this._listeners.push({ element, event, handler }); - }; - if (this.button) { - addListener(this.button, 'click', () => this.openModal()); + this.addListener(this.button, 'click', () => this.openModal()); } // ... apply to all other addListener calls
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/social-share-button.js
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-21T12:11:14.980Z
Learnt from: kpj2006
Repo: AOSSIE-Org/SocialShareButton PR: 1
File: src/social-share-button.js:10-11
Timestamp: 2025-12-21T12:11:14.980Z
Learning: SSR compatibility is important for public libraries. For files under src/, ensure the SocialShareButton (and related public APIs) render correctly in SSR environments across frameworks. Check for browser-only globals, conditional code paths, correct hydration behavior, and that the library exports behave consistently when used in SSR.
Applied to files:
src/social-share-button.js
🔇 Additional comments (3)
src/social-share-button.js (3)
33-35: LGTM! Clean listener tracking initialization.The
_listenersarray provides a centralized registry for all event listeners, enabling proper cleanup on destroy. The private naming convention is appropriate.
298-302: Good error handling improvement.The
.catch()handler properly falls back to manual text selection when the Clipboard API fails, improving robustness across different browsers and security contexts.
336-340: Excellent listener cleanup implementation!The listener removal logic correctly iterates through all tracked listeners and removes them, then clears the array. This effectively prevents memory leaks from event handlers.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/social-share-button.js (2)
344-373: Thorough cleanup implementation.The destroy method correctly handles all tracked resources in proper order. The listener cleanup loop and per-instance style tag removal address the core memory leak concerns.
Note: Minor untracked timeouts exist in
openModal()(10ms animation delay) andcopyLink()/fallbackCopy()(2s button text reset). These are harmless since they only modify element properties and won't throw errors on detached elements, but you could optionally track them for complete consistency.
402-404: Edge case:customClasswith spaces produces invalid CSS selector.If
customClasscontains multiple space-separated classes (e.g.,"btn primary"), the resulting selector.btn primary.social-share-btnis invalid and styles won't apply.🔧 Proposed fix: Handle multi-class customClass
- const buttonClass = this.options.customClass - ? `.${this.options.customClass}.social-share-btn` - : '.social-share-btn'; + const buttonClass = this.options.customClass + ? `.${this.options.customClass.split(/\s+/).join('.')}.social-share-btn` + : '.social-share-btn';
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/social-share-button.js
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-21T12:11:14.980Z
Learnt from: kpj2006
Repo: AOSSIE-Org/SocialShareButton PR: 1
File: src/social-share-button.js:10-11
Timestamp: 2025-12-21T12:11:14.980Z
Learning: SSR compatibility is important for public libraries. For files under src/, ensure the SocialShareButton (and related public APIs) render correctly in SSR environments across frameworks. Check for browser-only globals, conditional code paths, correct hydration behavior, and that the library exports behave consistently when used in SSR.
Applied to files:
src/social-share-button.js
🔇 Additional comments (5)
src/social-share-button.js (5)
33-36: LGTM! Clean initialization for cleanup tracking.The instance fields properly address both past review concerns:
_closeTimeoutfor the race condition fix and_styleTagfor per-instance style isolation.
204-243: Comprehensive listener tracking implementation.The
addListenerhelper correctly captures all event registrations including the document-level ESC key listener. This ensures all listeners can be properly cleaned up indestroy().
256-270: Race condition fix properly implemented.The timeout tracking with
_closeTimeoutand the safety check forthis.modalexistence directly address the previously identified race condition whendestroy()is called during modal animation.
392-420: Per-instance style tag correctly isolates custom colors.The switch from a shared ID-based style tag to an instance-owned
_styleTagproperty prevents one instance's destruction from affecting others' styling. This properly addresses the previously identified issue.
162-183: Formatting changes only, no functional impact.The message construction logic is unchanged; these are just whitespace/line-break adjustments.
pankajbagul01
left a comment
pankajbagul01
left a comment
There was a problem hiding this comment.
Thanks for the detailed review! 🙌
I’ve addressed both concerns:
- Fixed the potential race condition by tracking and clearing the modal close animation timeout during destroy()
- Updated custom color handling to use instance-specific style tags instead of a shared ID, preventing cross-instance side effects
Please let me know if anything else needs adjustment.
1 participant
This PR fixes a memory leak caused by event listeners not being removed when the SocialShareButton component is destroyed.
Problem
The component attaches multiple event listeners (button clicks, modal interactions, and a document-level
keydownlistener for ESC).Previously, these listeners were never removed when
destroy()was called, which could lead to memory leaks and duplicated handlers when the component is mounted/unmounted multiple times (e.g., in SPAs or dynamic rendering).Solution
_listenersarray.addEventListenercalls through a helper that stores{ element, event, handler }.destroy()removes all registered listeners, including:Result
Fixes #3
Summary by CodeRabbit
Bug Fixes
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.