fix: add try-catch for querySelector and log fallbackCopy errors (#132) - #199
fix: add try-catch for querySelector and log fallbackCopy errors (#132)#199afzalansari12 wants to merge 2 commits into
Conversation
github-actions
Bot
added
javascript
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
Walkthrough
ChangesSocialShareButton error handling
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/social-share-button.js`:
- Around line 536-537: Update the catch block in the clipboard fallback flow to
use a domain-neutral debug warning instead of the analytics-specific _debugWarn
labeling, and add a concise inline comment explaining this debug-only failure
path. Generalize _debugWarn or introduce a suitable neutral helper while
preserving the existing error details.
- Around line 736-748: Update static _resolveContainer to route invalid-selector
diagnostics through the shared _debugWarn helper instead of calling console.warn
directly. Make the helper usable from this pre-instance path, passing the
relevant debug setting such as options.debug or using a module-level helper,
while keeping the no-console suppression centralized in that helper.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 4bfe145e-42c6-48ba-8a2d-f759661c510c
📒 Files selected for processing (1)
src/social-share-button.js
|
|
||
| static _resolveContainer(raw) { | ||
| if (!raw) return null; | ||
| if (typeof document === "undefined") return null; | ||
| return typeof raw === "string" ? document.querySelector(raw) : raw; | ||
| if (typeof raw !== "string") return raw; | ||
|
|
||
| try { | ||
| return document.querySelector(raw); | ||
| } catch (error) { | ||
| // eslint-disable-next-line no-console | ||
| console.warn("[SocialShareButton] Invalid container selector:", raw, error); | ||
| return null; | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Route selector diagnostics through the shared warning helper.
Lines 745-746 introduce another inline console.warn and bypass _debugWarn(), causing invalid selectors to log regardless of the library’s debug setting. Make the helper callable from this static, pre-instance path—such as by passing options.debug into the resolver or using a module-level helper—and keep the no-console suppression in one place.
Based on learnings, debugging logs in src/social-share-button.js should be consolidated through a private _debugWarn helper instead of adding per-line inline console.* calls.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/social-share-button.js` around lines 736 - 748, Update static
_resolveContainer to route invalid-selector diagnostics through the shared
_debugWarn helper instead of calling console.warn directly. Make the helper
usable from this pre-instance path, passing the relevant debug setting such as
options.debug or using a module-level helper, while keeping the no-console
suppression centralized in that helper.
Source: Learnings
|
Thanks for the suggestion! Since _resolveContainer() is a static method and _debugWarn() is instance-based, would you prefer converting _debugWarn() into a shared/static helper, or is guarding console.warn() behind the debug option acceptable here |
1 participant
Addressed Issues:
Fixes #132
Screenshots/Recordings:
Not applicable — this is an internal error-handling improvement with no visual/UI change.
Additional Notes:
document.querySelector(raw)in_resolveContainer()with a try-catch, sincequerySelectorthrows aSyntaxErroron an invalid CSS selector, and the selector is user-supplied viaoptions.container.fallbackCopy()catch block to bind the error variable (_err→error) and log it viathis._debugWarn()instead of silently swallowing it.npm run lintpasses with no errors and the script still parses correctly (node --check).###invalid[[[) now logs a warning via console instead of throwing an uncaughtSyntaxError.document.execCommandto throw insidefallbackCopy()now logs[SocialShareButton Analytics] fallbackCopy failed <error>via_debugWarn(withdebug: true), and the copy button correctly updates to "Failed".Checklist
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. AI slop is strongly discouraged and may lead to banning and blocking. Do not spam our repos with AI slop.
Summary by CodeRabbit