Enhance modal copy functionality with improved event cleanup and lifecycle management#41
Enhance modal copy functionality with improved event cleanup and lifecycle management#41Aarya-Chaudhari wants to merge 14 commits intoAOSSIE-Org:mainfrom
Conversation
📝 WalkthroughWalkthroughAdded copy-to-clipboard UI and behavior for code blocks in Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant Btn as Share Button
participant Modal as Share Modal
participant Input as URL Input
participant Clipboard as Clipboard API
rect rgba(200,200,255,0.5)
User->>Btn: click
Btn->>Modal: open modal (attach handlers)
end
rect rgba(200,255,200,0.5)
User->>Modal: click copy
Modal->>Input: read URL
Modal->>Clipboard: navigator.clipboard.writeText(url)
Clipboard-->>Modal: success / failure
Modal->>Modal: update UI ("Copied!" state), set timeout to reset
end
rect rgba(255,200,200,0.5)
User->>Modal: close / ESC
Modal->>Btn: remove bindings (destroy) if owned
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/social-share-button.js (1)
397-400: Release retained references indestroy()after teardown.After removing listeners and nodes, handler and element references are still kept on the instance. Clearing them helps GC and avoids accidental reuse after destroy.
Suggested refactor
document.body.style.overflow = ''; this.eventsAttached = false; this.isCopying = false; + this.platformBtnHandlers = []; + this.handleButtonClick = null; + this.handleOverlayClick = null; + this.handleCloseClick = null; + this.handleCopyClick = null; + this.handleInputClick = null; + this.handleKeydown = null; + this.customColorMouseEnterHandler = null; + this.customColorMouseLeaveHandler = null; + this.modal = null; + this.button = null; + this.ownsButton = false;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/social-share-button.js` around lines 397 - 400, The destroy() method currently resets document.body.style.overflow and toggles this.eventsAttached and this.isCopying but retains DOM and handler references; update destroy() to null out all instance-held DOM elements and event handler references (for example this.shareButton, this.shareMenu, this.tooltip, this.handleClick, this.handleOutsideClick or any bound handlers you created) and reset state flags (this.eventsAttached = false, this.isCopying = false) so the instance no longer holds references that prevent GC or allow accidental reuse after teardown.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@index.html`:
- Around line 392-393: The markup nests a second <code> tag around the import
statement, creating invalid HTML; fix by removing the inner/outer duplication so
the import line "import SocialShareButton from 'social-share-button';" is
enclosed by a single <code> element (or close the outer <code> before the
import), ensuring only one <code> wraps the import and no nested <code> elements
remain.
- Around line 576-604: The copy button handler (copyButtons.forEach ->
button.addEventListener) must guard against missing/unsupported Clipboard API
and handle writeText failures: check for navigator.clipboard and typeof
navigator.clipboard.writeText === 'function' before calling it, and add a .catch
handler to set the copy-status aria-live text to an error message and restore
button text; if unavailable, fall back to a safe legacy copy method (create a
hidden textarea, select and document.execCommand('copy'), then remove it) and
similarly update statusSpan and button text on success or failure. Ensure all
branches restore the original button.textContent and clear/update statusSpan
(class "copy-status") appropriately.
In `@src/social-share-button.js`:
- Around line 326-347: The copy flow currently accesses navigator, window, and
document directly causing SSR failures; wrap those checks with typeof guards
before using them: in the block that calls navigator.clipboard and
window.isSecureContext, first ensure typeof navigator !== 'undefined' && typeof
window !== 'undefined' and only then call
navigator.clipboard.writeText(this.options.url), and in fallbackCopy(input)
ensure typeof document !== 'undefined' before calling document.execCommand,
returning false if document is unavailable; keep behavior of reset(...) and
fallbackCopy(...) unchanged otherwise.
---
Nitpick comments:
In `@src/social-share-button.js`:
- Around line 397-400: The destroy() method currently resets
document.body.style.overflow and toggles this.eventsAttached and this.isCopying
but retains DOM and handler references; update destroy() to null out all
instance-held DOM elements and event handler references (for example
this.shareButton, this.shareMenu, this.tooltip, this.handleClick,
this.handleOutsideClick or any bound handlers you created) and reset state flags
(this.eventsAttached = false, this.isCopying = false) so the instance no longer
holds references that prevent GC or allow accidental reuse after teardown.
| const copyButtons = document.querySelectorAll('.copy-btn'); | ||
|
|
||
| copyButtons.forEach((button) => { | ||
| button.addEventListener('click', () => { | ||
| const codeBlock = button.closest('.code-block')?.querySelector('code'); | ||
| const text = codeBlock ? codeBlock.innerText : ''; | ||
|
|
||
| navigator.clipboard.writeText(text).then(() => { | ||
| const originalText = button.textContent; | ||
|
|
||
| // Find aria-live span | ||
| const statusSpan = button.nextElementSibling; | ||
|
|
||
| if (statusSpan && statusSpan.classList.contains("copy-status")) { | ||
| statusSpan.textContent = "Code copied to clipboard"; | ||
| } | ||
|
|
||
| button.textContent = "Copied!"; | ||
|
|
||
| setTimeout(() => { | ||
| button.textContent = originalText; | ||
|
|
||
| if (statusSpan && statusSpan.classList.contains("copy-status")) { | ||
| statusSpan.textContent = ""; | ||
| } | ||
| }, 2000); | ||
| }); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
navigator.clipboard.writeText secure context requirements fallback
💡 Result:
navigator.clipboard.writeText() — secure-context requirements
1) Must be a secure context
- The Async Clipboard API (including
navigator.clipboard.writeText) is only available in secure contexts (typically HTTPS). (developer.mozilla.org) - Browsers also treat some local/dev origins as “potentially trustworthy” (commonly
http://localhost,http://127.0.0.1, and oftenfile://). (developer.mozilla.org) - Practical symptom: on non-secure pages,
navigator.clipboardmay be missing/undefined, or calls reject. (webkit.org)
2) User-gesture / transient activation is often required
- Many browsers require the call to happen in a user gesture (e.g., inside a click handler) or it will be rejected. (webkit.org)
3) Permissions differ by browser
- Spec-wise, writing expects Permissions API
clipboard-write, but real behavior varies:- Chromium: write requires either
clipboard-writepermission or transient activation; if permission is granted it can persist. (developer.mozilla.org) - Firefox/Safari: generally require transient activation;
clipboard-writepermission via Permissions API isn’t supported the same way. (developer.mozilla.org)
- Chromium: write requires either
4) If inside an <iframe>
- Clipboard access from iframes may require the appropriate Permissions-Policy allowances (notably in Chromium). (developer.mozilla.org)
Fallback when writeText() isn’t available (or fails)
Use the legacy (deprecated) document.execCommand('copy') approach:
async function copyText(text) {
// Prefer async clipboard when available
if (window.isSecureContext && navigator.clipboard?.writeText) {
return navigator.clipboard.writeText(text);
}
// Fallback: execCommand('copy')
const ta = document.createElement('textarea');
ta.value = text;
ta.setAttribute('readonly', '');
ta.style.position = 'absolute';
ta.style.left = '-9999px';
document.body.appendChild(ta);
ta.select();
const ok = document.execCommand('copy');
document.body.removeChild(ta);
if (!ok) throw new Error('Copy failed');
}Notes:
writeText()is secure-context-only. (developer.mozilla.org)- The
execCommand('copy')pattern is commonly used as a compatibility fallback (despite being deprecated). (copypasterun.com)
Citations:
- 1: https://developer.mozilla.org/en-US/docs/Web/API/Clipboard/writeText?utm_source=openai
- 2: https://developer.mozilla.org/en-US/docs/Web/Security/Secure_Contexts?utm_source=openai
- 3: https://webkit.org/blog/10855/async-clipboard-api/?utm_source=openai
- 4: https://webkit.org/blog/10855/async-clipboard-api/?utm_source=openai
- 5: https://developer.mozilla.org/en-US/docs/Web/API/Clipboard_API?utm_source=openai
- 6: https://developer.mozilla.org/en-US/docs/Web/API/Clipboard_API?utm_source=openai
- 7: https://developer.mozilla.org/en-US/docs/Web/API/Clipboard_API?utm_source=openai
- 8: https://developer.mozilla.org/en-US/docs/Web/API/Clipboard/writeText?utm_source=openai
- 9: https://www.copypasterun.com/javascript/javascript-and-the-clipboard-api-copy-and-paste/?utm_source=openai
🌐 Web query:
document.execCommand copy fallback clipboard compatibility
💡 Result:
Use the Async Clipboard API first (navigator.clipboard.writeText()), and fall back to document.execCommand('copy') only when needed (it’s deprecated and may stop working in the future). [1][2]
export async function copyToClipboard(text) {
// Preferred: Async Clipboard API (secure context + usually a user gesture)
if (navigator.clipboard?.writeText) {
try {
await navigator.clipboard.writeText(text);
return true;
} catch (_) {
// fall through to execCommand fallback
}
}
// Fallback: execCommand('copy') using a temporary textarea + selection
const ta = document.createElement("textarea");
ta.value = text;
// keep it off-screen but selectable
ta.style.position = "fixed";
ta.style.top = "-9999px";
ta.style.left = "-9999px";
document.body.appendChild(ta);
ta.focus();
ta.select();
let ok = false;
try {
ok = document.execCommand("copy");
} catch (_) {
ok = false;
} finally {
document.body.removeChild(ta);
}
return ok;
}Compatibility notes (high-signal):
navigator.clipboardis the recommended API, but writing commonly requires “transient user activation” (a user gesture) and a secure context (HTTPS). [2]document.execCommand('copy')typically only works when called from a short-lived user gesture handler (e.g., a click). [3]- Don’t rely on
document.queryCommandSupported()for detection; it’s also deprecated. [1]
Sources: MDN on document.execCommand() deprecation and clipboard commands [1]; MDN Clipboard API security/activation requirements and browser behavior notes [2]; MDN (extensions) note about user-gesture requirement for execCommand('copy') [3].
🌐 Web query:
Clipboard API vs document.execCommand copy browser support 2025
💡 Result:
TL;DR (2025)
- Use the Async Clipboard API (
navigator.clipboard.writeText()/readText()) when possible: it’s the modern, standards-based approach, but requires HTTPS (“secure context”) and typically a user gesture / transient activation, with permission-model differences by browser. [1][2] document.execCommand('copy')is deprecated and “may cease to work at any time,” but it remained very widely supported across browsers in 2025 largely for legacy compatibility. [3][4]
Browser support reality (as of 2025)
Async Clipboard API (recommended)
- Supported in modern browsers (roughly): Chrome 76+ / Edge 79+ / Safari 13.1+ / Firefox 127+. [2]
- Constraints/behavior differences:
- Secure context required (HTTPS). [1][2]
- Chrome/Chromium may use Permissions API (
clipboard-read/clipboard-write) and has iframe policy controls. [2] - Firefox & Safari don’t support those Permissions API clipboard permissions (per MDN) and generally rely on transient activation (and for reads, a user-facing paste prompt flow). [2]
document.execCommand('copy') (legacy fallback)
- Marked deprecated and not recommended for new work. [3]
- Still showed broad support across major browsers (Chrome/Edge/Firefox/Safari) on compatibility tables, but behavior is inconsistent and can be restricted to user gestures/selections. [3][4]
Practical recommendation (2025)
- Implement Clipboard API first, and keep
execCommand('copy')only as a fallback for older/incompatible environments—especially if you must support older Firefox versions (pre-127) or unusual embedded browsers. [2][3]
Sources: MDN Clipboard API & security/permission notes [2], MDN Clipboard write secure-context note [1], MDN execCommand deprecation warning [3], Can I use execCommand support table [4].
Guard clipboard usage and add a failure path for unsupported/insecure contexts.
navigator.clipboard.writeText() requires a secure context (HTTPS); on non-secure contexts (http://, file://, or restricted browsers), it becomes unavailable or fails. The current code has no fallback and no error handling, causing silent failures.
Suggested fix
+const copyText = (text) => {
+ if (window.isSecureContext && navigator.clipboard?.writeText) {
+ return navigator.clipboard.writeText(text).then(() => true);
+ }
+
+ const textarea = document.createElement('textarea');
+ textarea.value = text;
+ textarea.setAttribute('readonly', '');
+ textarea.style.position = 'absolute';
+ textarea.style.left = '-9999px';
+ document.body.appendChild(textarea);
+ textarea.select();
+ const ok = document.execCommand('copy');
+ document.body.removeChild(textarea);
+ return Promise.resolve(ok);
+};
+
copyButtons.forEach((button) => {
button.addEventListener('click', () => {
const codeBlock = button.closest('.code-block')?.querySelector('code');
-const text = codeBlock ? codeBlock.innerText : '';
+ const text = codeBlock ? codeBlock.innerText : '';
- navigator.clipboard.writeText(text).then(() => {
+ copyText(text).then((ok) => {
+ if (!ok) return;
const originalText = button.textContent;
// Find aria-live span
const statusSpan = button.nextElementSibling;
@@
-});
+ }).catch(() => {});
});
});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@index.html` around lines 576 - 604, The copy button handler
(copyButtons.forEach -> button.addEventListener) must guard against
missing/unsupported Clipboard API and handle writeText failures: check for
navigator.clipboard and typeof navigator.clipboard.writeText === 'function'
before calling it, and add a .catch handler to set the copy-status aria-live
text to an error message and restore button text; if unavailable, fall back to a
safe legacy copy method (create a hidden textarea, select and
document.execCommand('copy'), then remove it) and similarly update statusSpan
and button text on success or failure. Ensure all branches restore the original
button.textContent and clear/update statusSpan (class "copy-status")
appropriately.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/social-share-button.js`:
- Around line 371-408: The destroy() method currently removes event listeners
and DOM nodes but retains references (platformBtnHandlers array, this.modal,
this.button, and handler properties), risking detached-node retention; after
removing listeners and removing DOM nodes in destroy(), clear and null out
retained references: empty platformBtnHandlers (e.g., platformBtnHandlers.length
= 0), set this.modal = null, this.button = null, this.handleCopyClick = null,
this.handleInputClick = null, this.handleKeydown = null,
this.customColorMouseEnterHandler = null, this.customColorMouseLeaveHandler =
null, and also reset flags like this.ownsButton = false (if used),
this.eventsAttached = false, and this.isCopying = false so the instance no
longer holds DOM or handler references.
- Around line 327-338: The guard currently checks for navigator.clipboard but
not that writeText exists/callable, so calling
navigator.clipboard.writeText(this.options.url) can throw synchronously; modify
the block around navigator.clipboard.writeText to first verify typeof
navigator.clipboard.writeText === 'function' and then call it inside a
try/catch; on success call reset(true) in the try's resolved path, and in the
catch fall back to const success = this.fallbackCopy(input); reset(success) to
ensure isCopying is always cleared; keep references to
navigator.clipboard.writeText, this.options.url, this.fallbackCopy(input), and
reset(...) when implementing the change.
- Around line 357-387: The destroy() method currently touches DOM globals and
will throw in SSR; add a guard at the top of destroy() so it returns early when
not running in a browser (use this.isBrowser), and ensure any direct uses of
document or DOM queries (document.removeEventListener,
this.modal?.querySelector, etc.) are only executed when this.isBrowser is true;
update the destroy() function to check this.isBrowser before performing event
removal and querySelector operations to avoid ReferenceError during SSR.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
index.htmlsrc/social-share-button.js
🚧 Files skipped from review as they are similar to previous changes (1)
- index.html
| if ( | ||
| typeof window !== 'undefined' && | ||
| typeof navigator !== 'undefined' && | ||
| navigator.clipboard && | ||
| window.isSecureContext | ||
| ) { | ||
| navigator.clipboard.writeText(this.options.url) | ||
| .then(() => reset(true)) | ||
| .catch(() => { | ||
| const success = this.fallbackCopy(input); | ||
| reset(success); | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
ls -la src/social-share-button.js && wc -l src/social-share-button.jsRepository: AOSSIE-Org/SocialShareButton
Length of output: 179
🏁 Script executed:
cat -n src/social-share-button.js | sed -n '320,350p'Repository: AOSSIE-Org/SocialShareButton
Length of output: 970
🏁 Script executed:
cat -n src/social-share-button.js | sed -n '310,345p'Repository: AOSSIE-Org/SocialShareButton
Length of output: 1189
🏁 Script executed:
rg -B 5 "navigator.clipboard" src/social-share-button.jsRepository: AOSSIE-Org/SocialShareButton
Length of output: 261
🏁 Script executed:
cat -n src/social-share-button.js | sed -n '300,350p'Repository: AOSSIE-Org/SocialShareButton
Length of output: 1639
🏁 Script executed:
rg "isCopying" src/social-share-button.jsRepository: AOSSIE-Org/SocialShareButton
Length of output: 206
🌐 Web query:
navigator.clipboard.writeText synchronous throw error MDN Clipboard API
💡 Result:
navigator.clipboard.writeText() is asynchronous: it returns a Promise, so failures usually show up as a Promise rejection, not as an immediate (“synchronous”) throw. MDN documents that writeText() returns a Promise and that failure can be a NotAllowedError (a DOMException) when writing isn’t allowed. (developer.mozilla.org)
Why you might think it “throws synchronously”:
-
You didn’t
await(or attach.catch), so the error appears later as an unhandled promise rejection, and a surrounding non-awaittry/catchwon’t catch it:try { navigator.clipboard.writeText("x"); } catch (e) {} // won't catch async rejection
Correct:
try { await navigator.clipboard.writeText("x"); } catch (e) {} // or: navigator.clipboard.writeText("x").catch(console.error);
-
navigator.clipboardis missing/undefined, commonly because the API is only available in secure contexts (HTTPS;localhostis treated as secure for dev). In that case, you’ll get an immediateTypeErrorlike “Cannot read properties of undefined (reading 'writeText')”. (developer.mozilla.org)
Common reasons the Promise is rejected (often as NotAllowedError):
- Not a secure context (HTTPS requirement). (developer.mozilla.org)
- Missing transient user activation (must be triggered by a user gesture like a click; this is explicitly required for writing in Firefox/Safari per MDN). (developer.mozilla.org)
- In iframes, blocked by Permissions-Policy (
clipboard-write). (developer.mozilla.org)
Citations:
- 1: https://developer.mozilla.org/en-US/docs/Web/API/Clipboard/writeText?utm_source=openai
- 2: https://developer.mozilla.org/en-US/docs/Web/API/Clipboard/writeText?utm_source=openai
- 3: https://developer.mozilla.org/en-US/docs/Web/API/Clipboard/writeText?utm_source=openai
- 4: https://developer.mozilla.org/en-US/docs/Web/API/Clipboard/writeText?utm_source=openai
- 5: https://developer.mozilla.org/docs/Web/API/Clipboard_API?utm_source=openai
- 6: https://developer.mozilla.org/docs/Web/API/Clipboard_API?utm_source=openai
Guard navigator.clipboard.writeText against partial implementations.
While the code checks for navigator.clipboard, it doesn't verify that writeText is callable. In browsers with partial Clipboard API implementations where navigator.clipboard exists but lacks the writeText method, calling it throws a synchronous TypeError before a Promise is created. This bypasses the .catch() handler and leaves isCopying stuck true.
Add a type check for navigator.clipboard.writeText and wrap the call in try-catch:
Proposed fix
+ const canUseClipboardApi =
+ typeof window !== 'undefined' &&
+ typeof navigator !== 'undefined' &&
+ !!navigator.clipboard &&
+ typeof navigator.clipboard.writeText === 'function' &&
+ window.isSecureContext;
+
+ if (canUseClipboardApi) {
+ try {
+ navigator.clipboard.writeText(this.options.url)
+ .then(() => reset(true))
+ .catch(() => {
+ const success = this.fallbackCopy(input);
+ reset(success);
+ });
+ } catch {
+ const success = this.fallbackCopy(input);
+ reset(success);
+ }
+ } else {
const success = this.fallbackCopy(input);
reset(success);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if ( | |
| typeof window !== 'undefined' && | |
| typeof navigator !== 'undefined' && | |
| navigator.clipboard && | |
| window.isSecureContext | |
| ) { | |
| navigator.clipboard.writeText(this.options.url) | |
| .then(() => reset(true)) | |
| .catch(() => { | |
| const success = this.fallbackCopy(input); | |
| reset(success); | |
| }); | |
| const canUseClipboardApi = | |
| typeof window !== 'undefined' && | |
| typeof navigator !== 'undefined' && | |
| !!navigator.clipboard && | |
| typeof navigator.clipboard.writeText === 'function' && | |
| window.isSecureContext; | |
| if (canUseClipboardApi) { | |
| try { | |
| navigator.clipboard.writeText(this.options.url) | |
| .then(() => reset(true)) | |
| .catch(() => { | |
| const success = this.fallbackCopy(input); | |
| reset(success); | |
| }); | |
| } catch { | |
| const success = this.fallbackCopy(input); | |
| reset(success); | |
| } | |
| } else { | |
| const success = this.fallbackCopy(input); | |
| reset(success); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/social-share-button.js` around lines 327 - 338, The guard currently
checks for navigator.clipboard but not that writeText exists/callable, so
calling navigator.clipboard.writeText(this.options.url) can throw synchronously;
modify the block around navigator.clipboard.writeText to first verify typeof
navigator.clipboard.writeText === 'function' and then call it inside a
try/catch; on success call reset(true) in the try's resolved path, and in the
catch fall back to const success = this.fallbackCopy(input); reset(success) to
ensure isCopying is always cleared; keep references to
navigator.clipboard.writeText, this.options.url, this.fallbackCopy(input), and
reset(...) when implementing the change.
| destroy() { | ||
| if (this.button && this.handleButtonClick) { | ||
| this.button.removeEventListener('click', this.handleButtonClick); | ||
| } | ||
|
|
||
| destroy() { | ||
| if (this.button && this.customColorMouseEnterHandler) { | ||
| this.button.removeEventListener('mouseenter', this.customColorMouseEnterHandler); | ||
| this.customColorMouseEnterHandler = null; | ||
| } | ||
| if (this.button && this.customColorMouseLeaveHandler) { | ||
| this.button.removeEventListener('mouseleave', this.customColorMouseLeaveHandler); | ||
| this.customColorMouseLeaveHandler = null; | ||
| } | ||
| if (this.modal && this.handleOverlayClick) { | ||
| this.modal.removeEventListener('click', this.handleOverlayClick); | ||
| } | ||
|
|
||
| if (this.button && this.button.parentNode) { | ||
| this.button.parentNode.removeChild(this.button); | ||
| } | ||
| if (this.modal && this.modal.parentNode) { | ||
| this.modal.parentNode.removeChild(this.modal); | ||
| } | ||
| document.body.style.overflow = ''; | ||
| const closeBtn = this.modal?.querySelector('.social-share-modal-close'); | ||
| if (closeBtn && this.handleCloseClick) { | ||
| closeBtn.removeEventListener('click', this.handleCloseClick); | ||
| } | ||
|
|
||
| this.platformBtnHandlers.forEach(({ btn, handler }) => { | ||
| btn.removeEventListener('click', handler); | ||
| }); | ||
|
|
||
| const copyBtn = this.modal?.querySelector('.social-share-copy-btn'); | ||
| if (copyBtn && this.handleCopyClick) { | ||
| copyBtn.removeEventListener('click', this.handleCopyClick); | ||
| } | ||
|
|
||
| const input = this.modal?.querySelector('.social-share-link-input input'); | ||
| if (input && this.handleInputClick) { | ||
| input.removeEventListener('click', this.handleInputClick); | ||
| } | ||
|
|
||
| if (this.handleKeydown) { | ||
| document.removeEventListener('keydown', this.handleKeydown); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd /dev/null && pwdRepository: AOSSIE-Org/SocialShareButton
Length of output: 121
🏁 Script executed:
git ls-files | grep -E "social-share-button\.js|\.js$" | head -20Repository: AOSSIE-Org/SocialShareButton
Length of output: 98
🏁 Script executed:
cat -n src/social-share-button.js | sed -n '350,420p'Repository: AOSSIE-Org/SocialShareButton
Length of output: 2600
🏁 Script executed:
rg "isBrowser" src/social-share-button.js -B 2 -A 2Repository: AOSSIE-Org/SocialShareButton
Length of output: 277
🏁 Script executed:
rg "destroy\(\)" src/social-share-button.js -B 5 -A 50 | head -100Repository: AOSSIE-Org/SocialShareButton
Length of output: 1720
🏁 Script executed:
rg "destroy" src/social-share-button.js -B 3 -A 3Repository: AOSSIE-Org/SocialShareButton
Length of output: 211
🏁 Script executed:
rg "destroy\(" --type js | grep -v node_modules | head -20Repository: AOSSIE-Org/SocialShareButton
Length of output: 186
🏁 Script executed:
cat -n src/social-share-button.js | head -100Repository: AOSSIE-Org/SocialShareButton
Length of output: 4598
Guard destroy() against SSR execution contexts.
The method directly accesses document at lines 386 and 405 without checking the browser environment. When called during SSR (e.g., from React cleanup), this will throw a ReferenceError. Use the existing this.isBrowser guard consistently:
Proposed fix
destroy() {
+ if (!this.isBrowser) {
+ this.eventsAttached = false;
+ this.isCopying = false;
+ return;
+ }
+
if (this.button && this.handleButtonClick) {
this.button.removeEventListener('click', this.handleButtonClick);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| destroy() { | |
| if (this.button && this.handleButtonClick) { | |
| this.button.removeEventListener('click', this.handleButtonClick); | |
| } | |
| destroy() { | |
| if (this.button && this.customColorMouseEnterHandler) { | |
| this.button.removeEventListener('mouseenter', this.customColorMouseEnterHandler); | |
| this.customColorMouseEnterHandler = null; | |
| } | |
| if (this.button && this.customColorMouseLeaveHandler) { | |
| this.button.removeEventListener('mouseleave', this.customColorMouseLeaveHandler); | |
| this.customColorMouseLeaveHandler = null; | |
| } | |
| if (this.modal && this.handleOverlayClick) { | |
| this.modal.removeEventListener('click', this.handleOverlayClick); | |
| } | |
| if (this.button && this.button.parentNode) { | |
| this.button.parentNode.removeChild(this.button); | |
| } | |
| if (this.modal && this.modal.parentNode) { | |
| this.modal.parentNode.removeChild(this.modal); | |
| } | |
| document.body.style.overflow = ''; | |
| const closeBtn = this.modal?.querySelector('.social-share-modal-close'); | |
| if (closeBtn && this.handleCloseClick) { | |
| closeBtn.removeEventListener('click', this.handleCloseClick); | |
| } | |
| this.platformBtnHandlers.forEach(({ btn, handler }) => { | |
| btn.removeEventListener('click', handler); | |
| }); | |
| const copyBtn = this.modal?.querySelector('.social-share-copy-btn'); | |
| if (copyBtn && this.handleCopyClick) { | |
| copyBtn.removeEventListener('click', this.handleCopyClick); | |
| } | |
| const input = this.modal?.querySelector('.social-share-link-input input'); | |
| if (input && this.handleInputClick) { | |
| input.removeEventListener('click', this.handleInputClick); | |
| } | |
| if (this.handleKeydown) { | |
| document.removeEventListener('keydown', this.handleKeydown); | |
| } | |
| destroy() { | |
| if (!this.isBrowser) { | |
| this.eventsAttached = false; | |
| this.isCopying = false; | |
| return; | |
| } | |
| if (this.button && this.handleButtonClick) { | |
| this.button.removeEventListener('click', this.handleButtonClick); | |
| } | |
| if (this.modal && this.handleOverlayClick) { | |
| this.modal.removeEventListener('click', this.handleOverlayClick); | |
| } | |
| const closeBtn = this.modal?.querySelector('.social-share-modal-close'); | |
| if (closeBtn && this.handleCloseClick) { | |
| closeBtn.removeEventListener('click', this.handleCloseClick); | |
| } | |
| this.platformBtnHandlers.forEach(({ btn, handler }) => { | |
| btn.removeEventListener('click', handler); | |
| }); | |
| const copyBtn = this.modal?.querySelector('.social-share-copy-btn'); | |
| if (copyBtn && this.handleCopyClick) { | |
| copyBtn.removeEventListener('click', this.handleCopyClick); | |
| } | |
| const input = this.modal?.querySelector('.social-share-link-input input'); | |
| if (input && this.handleInputClick) { | |
| input.removeEventListener('click', this.handleInputClick); | |
| } | |
| if (this.handleKeydown) { | |
| document.removeEventListener('keydown', this.handleKeydown); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/social-share-button.js` around lines 357 - 387, The destroy() method
currently touches DOM globals and will throw in SSR; add a guard at the top of
destroy() so it returns early when not running in a browser (use
this.isBrowser), and ensure any direct uses of document or DOM queries
(document.removeEventListener, this.modal?.querySelector, etc.) are only
executed when this.isBrowser is true; update the destroy() function to check
this.isBrowser before performing event removal and querySelector operations to
avoid ReferenceError during SSR.
| this.platformBtnHandlers.forEach(({ btn, handler }) => { | ||
| btn.removeEventListener('click', handler); | ||
| }); | ||
|
|
||
| const copyBtn = this.modal?.querySelector('.social-share-copy-btn'); | ||
| if (copyBtn && this.handleCopyClick) { | ||
| copyBtn.removeEventListener('click', this.handleCopyClick); | ||
| } | ||
|
|
||
| const input = this.modal?.querySelector('.social-share-link-input input'); | ||
| if (input && this.handleInputClick) { | ||
| input.removeEventListener('click', this.handleInputClick); | ||
| } | ||
|
|
||
| if (this.handleKeydown) { | ||
| document.removeEventListener('keydown', this.handleKeydown); | ||
| } | ||
|
|
||
| if (this.button && this.customColorMouseEnterHandler) { | ||
| this.button.removeEventListener('mouseenter', this.customColorMouseEnterHandler); | ||
| } | ||
|
|
||
| if (this.button && this.customColorMouseLeaveHandler) { | ||
| this.button.removeEventListener('mouseleave', this.customColorMouseLeaveHandler); | ||
| } | ||
|
|
||
| if (this.ownsButton && this.button?.parentNode) { | ||
| this.button.parentNode.removeChild(this.button); | ||
| } | ||
|
|
||
| if (this.modal?.parentNode) { | ||
| this.modal.parentNode.removeChild(this.modal); | ||
| } | ||
|
|
||
| document.body.style.overflow = ''; | ||
| this.eventsAttached = false; | ||
| this.isCopying = false; | ||
| } |
There was a problem hiding this comment.
Release retained instance references during teardown to avoid detached-node retention.
destroy() removes listeners but keeps platformBtnHandlers, element refs, and handler refs. If the instance remains referenced, detached DOM can stay retained.
Proposed fix
if (this.modal?.parentNode) {
this.modal.parentNode.removeChild(this.modal);
}
document.body.style.overflow = '';
+ this.platformBtnHandlers = [];
+ this.handleButtonClick = null;
+ this.handleOverlayClick = null;
+ this.handleCloseClick = null;
+ this.handleCopyClick = null;
+ this.handleInputClick = null;
+ this.handleKeydown = null;
+ this.customColorMouseEnterHandler = null;
+ this.customColorMouseLeaveHandler = null;
+ this.button = null;
+ this.modal = null;
+ this.ownsButton = false;
this.eventsAttached = false;
this.isCopying = false;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/social-share-button.js` around lines 371 - 408, The destroy() method
currently removes event listeners and DOM nodes but retains references
(platformBtnHandlers array, this.modal, this.button, and handler properties),
risking detached-node retention; after removing listeners and removing DOM nodes
in destroy(), clear and null out retained references: empty platformBtnHandlers
(e.g., platformBtnHandlers.length = 0), set this.modal = null, this.button =
null, this.handleCopyClick = null, this.handleInputClick = null,
this.handleKeydown = null, this.customColorMouseEnterHandler = null,
this.customColorMouseLeaveHandler = null, and also reset flags like
this.ownsButton = false (if used), this.eventsAttached = false, and
this.isCopying = false so the instance no longer holds DOM or handler
references.
|
Hello @kpj2006 This includes the modal copy enhancements and improved event lifecycle cleanup within src/social-share-button.js. The previous PR now remains strictly scoped to the demo page changes, as requested. Please review. |
|
@Aarya-Chaudhari pls consider some points:
|
|
@CodeRabbit according to last commit 4d7a14e review the changes in this pr. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 20225
Review of
|
| Severity | Issue |
|---|---|
| 🔴 Critical | Hardcoded promotional text in getShareURL() for WhatsApp, Telegram, Email |
| 🟠 Medium | destroy() doesn't null handler/DOM references after cleanup |
| 🟠 Medium | In-flight copy setTimeout fires on detached DOM node after destroy() |
| 🟡 Minor | Inconsistent indentation in constructor |
| 🟡 Minor | Redundant typeof window/navigator checks in copyLink() |
| 🟡 Minor | eventsAttached = true set before listeners are registered |
2 participants
Additional Notes:
This PR enhances the modal copy functionality in
src/social-share-button.jswith improved reliability and better lifecycle management.Changes Made
isCopyingguard to prevent duplicate copy actionsconsole.errorfrom production codedestroy()ownsButtonflag to ensure safe DOM cleanupWhy This Change?
Previously:
Now:
Checklist
AI Usage Disclosure
Check one of the checkboxes below:
I have used the following AI models and tools: TODO
We encourage contributors to use AI tools responsibly when creating Pull Requests. While AI can be a valuable aid, it is essential to ensure that your contributions meet the task requirements, build successfully, include relevant tests, and pass all linters. Submissions that do not meet these standards may be closed without warning to maintain the quality and integrity of the project. Please take the time to understand the changes you are proposing and their impact.
Summary by CodeRabbit
New Features
Bug Fixes
Improvements