chore: add pre-commit configuration for code quality checks#104
Closed
Muneerali199 wants to merge 3 commits intoAOSSIE-Org:mainfrom
Closed
chore: add pre-commit configuration for code quality checks#104Muneerali199 wants to merge 3 commits intoAOSSIE-Org:mainfrom
Muneerali199 wants to merge 3 commits intoAOSSIE-Org:mainfrom
Conversation
- Added PULL_REQUEST_TEMPLATE.md with AOSSIE-Org standard format - Added ISSUE_TEMPLATE/ folder with bug_report.yml, feature_request.yml, good_first_issue.yml, and config.yml - All templates include Discord link and CodeRabbit AI notice sections Fixes AOSSIE-Org#18
- Added .pre-commit-config.yaml with hooks for: - Trailing whitespace removal - End-of-file fixing - Merge conflict detection - Large file prevention - Mixed line ending normalization - YAML, JSON, TOML validation - Secret detection (detect-secrets) This implements one of the open-source best practices from issue AOSSIE-Org#18. Fixes AOSSIE-Org#18
Contributor
WalkthroughThis PR introduces GitHub issue and PR templates for structured contributions, adds pre-commit hooks for code quality checks, and significantly enhances the SocialShareButton library with runtime configurability features and comprehensive React wrapper lifecycle management. Changes
Sequence DiagramssequenceDiagram
participant User
participant ReactComponent as React Component
participant Browser as Browser/DOM
participant CoreLibrary as Core Library
User->>ReactComponent: Mount component with props
ReactComponent->>Browser: Create container div
Note over ReactComponent: Mount effect triggered
ReactComponent->>CoreLibrary: Check if window.SocialShareButton exists
ReactComponent->>CoreLibrary: Instantiate new SocialShareButton()
CoreLibrary->>Browser: Create button & modal in container
ReactComponent->>ReactComponent: Store instance in ref
Note over ReactComponent: Props change detected
ReactComponent->>ReactComponent: Derive currentUrl/currentTitle
ReactComponent->>CoreLibrary: Call updateOptions(newOptions)
CoreLibrary->>Browser: Update modal URL input
CoreLibrary->>CoreLibrary: Re-apply color customizations
User->>ReactComponent: Unmount component
Note over ReactComponent: Cleanup effect triggered
ReactComponent->>CoreLibrary: Call destroy()
CoreLibrary->>Browser: Remove button & modal from DOM
CoreLibrary->>CoreLibrary: Clear event listeners
sequenceDiagram
participant User as User (clicks button)
participant CoreLibrary as Core Library
participant Modal as Modal UI
participant Browser as Browser
User->>CoreLibrary: Click share button
CoreLibrary->>CoreLibrary: Increment openModalCount
CoreLibrary->>Browser: Get current body.overflow
CoreLibrary->>Browser: Store in originalBodyOverflow
CoreLibrary->>Modal: Show modal (disable scroll)
User->>Modal: Select platform & share
CoreLibrary->>Browser: Close modal
CoreLibrary->>CoreLibrary: Decrement openModalCount
alt openModalCount === 0
CoreLibrary->>Browser: Restore originalBodyOverflow
else openModalCount > 0
CoreLibrary->>Browser: Keep scroll disabled
end
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly Related PRs
Suggested Labels
Suggested Reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Important Merge conflicts detected
✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 Tip You can generate walkthrough in a markdown collapsible section to save space.Enable the |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/social-share-button.js (1)
129-134:⚠️ Potential issue | 🔴 CriticalPrevent XSS by removing raw option interpolation in
innerHTML.Line 133 (
buttonText) and Line 159 (modalPosition) interpolate option values into HTML directly. If those values are attacker-controlled, this can become DOM XSS.🛡️ Proposed fix
createButton() { const button = document.createElement('button'); button.className = `social-share-btn ${this.options.buttonStyle} ${this.options.customClass}`; button.setAttribute('aria-label', 'Share'); button.innerHTML = ` <svg width="20" height="20" viewBox="0 0 24 24" fill="none" xmlns="http://www.w3.org/2000/svg" class="share-icon"> <path d="M18 16.08C17.24 16.08 16.56 16.38 16.04 16.85L8.91 12.7C8.96 12.47 9 12.24 9 12C9 11.76 8.96 11.53 8.91 11.3L15.96 7.19C16.5 7.69 17.21 8 18 8C19.66 8 21 6.66 21 5C21 3.34 19.66 2 18 2C16.34 2 15 3.34 15 5C15 5.24 15.04 5.47 15.09 5.7L8.04 9.81C7.5 9.31 6.79 9 6 9C4.34 9 3 10.34 3 12C3 13.66 4.34 15 6 15C6.79 15 7.5 14.69 8.04 14.19L15.16 18.35C15.11 18.56 15.08 18.78 15.08 19C15.08 20.61 16.39 21.92 18 21.92C19.61 21.92 20.92 20.61 20.92 19C20.92 17.39 19.61 16.08 18 16.08Z" fill="currentColor"/> </svg> - <span>${this.options.buttonText}</span> + <span class="social-share-btn-label"></span> `; + const label = button.querySelector('.social-share-btn-label'); + if (label) label.textContent = String(this.options.buttonText ?? '');createModal() { const modal = document.createElement('div'); modal.className = `social-share-modal-overlay ${this.options.theme}`; modal.style.display = 'none'; + const safeModalPosition = String(this.options.modalPosition || 'center').replace(/[^\w-]/g, ''); modal.innerHTML = ` - <div class="social-share-modal-content ${this.options.modalPosition}"> + <div class="social-share-modal-content ${safeModalPosition}">Also applies to: 159-159
🤖 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 129 - 134, The code assigns unescaped user-controlled values directly into button.innerHTML (see button.innerHTML and this.options.buttonText) and into the modal markup (modalPosition), creating a DOM XSS risk; fix by stopping raw HTML interpolation — build DOM nodes programmatically or set text via element.textContent for button label (create a span and assign span.textContent = this.options.buttonText) and use safe attribute setters or validated/sanitized values for modalPosition instead of injecting into innerHTML, ensuring any dynamic strings are escaped or validated before insertion; update the code paths that create the icon/label and the modal placement logic to use createElement/appendChild and textContent/setAttribute rather than template innerHTML.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/PULL_REQUEST_TEMPLATE.md:
- Line 1: Add a single top-level H1 markdown heading at the very top of the PR
template to satisfy markdownlint MD041 (e.g., insert "# Pull Request" or "#
Project Name" as the first line) and leave the existing "### Addressed Issues:"
section below it so the file begins with a lone leading "#" heading.
In @.pre-commit-config.yaml:
- Around line 18-37: The pre-commit config contains duplicated repo entries for
"https://github.com/pre-commit/pre-commit-hooks" at the same rev v4.5.0;
consolidate them into a single repo block and merge the hook lists
(trailing-whitespace, end-of-file-fixer, check-merge-conflict,
check-added-large-files, mixed-line-ending, check-yaml, check-json, check-toml)
under one repo entry so there is only one reference to that repo/rev in the
file.
In `@src/social-share-button-react.jsx`:
- Around line 3-26: Remove the method-level JSDoc block and replace it with a
short, concise inline comment above the SocialShareButton component (or at the
top of the file) summarizing purpose and any non-obvious prop behavior; keep
detailed API docs out of src/, condense descriptions for props like url, title,
hashtags, platforms, theme, buttonText, onShare/onCopy into one-line notes only
where necessary, and leave the rest to prop names and default values in the
component itself.
In `@src/social-share-button.js`:
- Around line 100-103: Guard the auto-initialisation so it only runs in
DOM-capable environments: before calling this.init() when this.options.container
is present (and in the similar block around the other init path), check for
browser globals (e.g. typeof window !== 'undefined' && typeof document !==
'undefined') or a small helper like isDOMAvailable(); only call the instance
method init() when that check passes to avoid SSR/non-DOM errors in
src/social-share-button.js (referencing this.options.container and init()).
- Around line 8-30: The file contains a long JSDoc block describing the
SocialShareButton constructor options; per repo guidelines replace this JSDoc
with short inline comments inside the implementation (e.g., near the
SocialShareButton constructor function or class constructor, and where options
are merged in mergeOptions/init methods). Remove the multi-line JSDoc block and
instead add 1–2 brief inline comments that call out only non-obvious behavior or
edge cases (defaults merging, auto-init when a container is supplied, and
callbacks onShare/onCopy) next to the relevant symbols (SocialShareButton,
mergeOptions, init) so the code follows the src/* comment style.
---
Outside diff comments:
In `@src/social-share-button.js`:
- Around line 129-134: The code assigns unescaped user-controlled values
directly into button.innerHTML (see button.innerHTML and
this.options.buttonText) and into the modal markup (modalPosition), creating a
DOM XSS risk; fix by stopping raw HTML interpolation — build DOM nodes
programmatically or set text via element.textContent for button label (create a
span and assign span.textContent = this.options.buttonText) and use safe
attribute setters or validated/sanitized values for modalPosition instead of
injecting into innerHTML, ensuring any dynamic strings are escaped or validated
before insertion; update the code paths that create the icon/label and the modal
placement logic to use createElement/appendChild and textContent/setAttribute
rather than template innerHTML.
🪄 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
Run ID: 2e5b052b-ef1e-46e5-9c5f-480a5214b4d1
📒 Files selected for processing (8)
.github/ISSUE_TEMPLATE/bug_report.yml.github/ISSUE_TEMPLATE/config.yml.github/ISSUE_TEMPLATE/feature_request.yml.github/ISSUE_TEMPLATE/good_first_issue.yml.github/PULL_REQUEST_TEMPLATE.md.pre-commit-config.yamlsrc/social-share-button-react.jsxsrc/social-share-button.js
| @@ -0,0 +1,45 @@ | |||
| ### Addressed Issues: | |||
Contributor
There was a problem hiding this comment.
Add a top-level H1 heading to satisfy markdownlint.
Line 1 starts with ###, which triggers MD041. Add a single # heading at the top.
🔧 Proposed fix
+# Pull Request
+
### Addressed Issues:📝 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.
Suggested change
| ### Addressed Issues: | |
| # Pull Request | |
| ### Addressed Issues: |
🧰 Tools
🪛 markdownlint-cli2 (0.21.0)
[warning] 1-1: First line in a file should be a top-level heading
(MD041, first-line-heading, first-line-h1)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/PULL_REQUEST_TEMPLATE.md at line 1, Add a single top-level H1
markdown heading at the very top of the PR template to satisfy markdownlint
MD041 (e.g., insert "# Pull Request" or "# Project Name" as the first line) and
leave the existing "### Addressed Issues:" section below it so the file begins
with a lone leading "#" heading.
Comment on lines
+18
to
+37
| - repo: https://github.com/pre-commit/pre-commit-hooks | ||
| rev: v4.5.0 | ||
| hooks: | ||
| - id: trailing-whitespace | ||
| - id: end-of-file-fixer | ||
| - id: check-merge-conflict | ||
| - id: check-added-large-files | ||
| - id: mixed-line-ending | ||
| args: ['--fix=lf'] | ||
|
|
||
| # ---------------------------------- | ||
| # 2. Config & data files validation | ||
| # ---------------------------------- | ||
| - repo: https://github.com/pre-commit/pre-commit-hooks | ||
| rev: v4.5.0 | ||
| hooks: | ||
| - id: check-yaml | ||
| - id: check-json | ||
| - id: check-toml | ||
|
|
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consolidate duplicated pre-commit-hooks repo blocks.
Line 18 and Line 31 repeat the same repo/rev. Merge into one entry to reduce maintenance drift risk.
♻️ Proposed refactor
- repo: https://github.com/pre-commit/pre-commit-hooks
rev: v4.5.0
hooks:
- id: trailing-whitespace
- id: end-of-file-fixer
- id: check-merge-conflict
- id: check-added-large-files
- id: mixed-line-ending
args: ['--fix=lf']
-
- # ----------------------------------
- # 2. Config & data files validation
- # ----------------------------------
- - repo: https://github.com/pre-commit/pre-commit-hooks
- rev: v4.5.0
- hooks:
- id: check-yaml
- id: check-json
- id: check-toml📝 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.
Suggested change
| - repo: https://github.com/pre-commit/pre-commit-hooks | |
| rev: v4.5.0 | |
| hooks: | |
| - id: trailing-whitespace | |
| - id: end-of-file-fixer | |
| - id: check-merge-conflict | |
| - id: check-added-large-files | |
| - id: mixed-line-ending | |
| args: ['--fix=lf'] | |
| # ---------------------------------- | |
| # 2. Config & data files validation | |
| # ---------------------------------- | |
| - repo: https://github.com/pre-commit/pre-commit-hooks | |
| rev: v4.5.0 | |
| hooks: | |
| - id: check-yaml | |
| - id: check-json | |
| - id: check-toml | |
| - repo: https://github.com/pre-commit/pre-commit-hooks | |
| rev: v4.5.0 | |
| hooks: | |
| - id: trailing-whitespace | |
| - id: end-of-file-fixer | |
| - id: check-merge-conflict | |
| - id: check-added-large-files | |
| - id: mixed-line-ending | |
| args: ['--fix=lf'] | |
| - id: check-yaml | |
| - id: check-json | |
| - id: check-toml |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.pre-commit-config.yaml around lines 18 - 37, The pre-commit config contains
duplicated repo entries for "https://github.com/pre-commit/pre-commit-hooks" at
the same rev v4.5.0; consolidate them into a single repo block and merge the
hook lists (trailing-whitespace, end-of-file-fixer, check-merge-conflict,
check-added-large-files, mixed-line-ending, check-yaml, check-json, check-toml)
under one repo entry so there is only one reference to that repo/rev in the
file.
Comment on lines
+3
to
+26
| /** | ||
| * SocialShareButton - React wrapper for the SocialShareButton vanilla-JS library. | ||
| * | ||
| * Mounts the imperative {@link window.SocialShareButton} instance into a `<div>` | ||
| * ref on first render, syncs all prop changes to the underlying instance via | ||
| * `updateOptions()`, and calls `destroy()` on unmount to clean up listeners and | ||
| * DOM nodes. | ||
| * | ||
| * @param {Object} props - Component props. | ||
| * @param {string} [props.url] - URL to share; defaults to `window.location.href`. | ||
| * @param {string} [props.title] - Share title; defaults to `document.title`. | ||
| * @param {string} [props.description] - Optional description appended to share text. | ||
| * @param {string[]} [props.hashtags=[]] - Hashtags (without '#') for supported platforms. | ||
| * @param {string} [props.via=''] - Twitter @username to attribute the tweet to. | ||
| * @param {string[]} [props.platforms] - Platforms to display; defaults to all six built-ins. | ||
| * @param {string} [props.theme='dark'] - Modal colour theme: `'dark'` or `'light'`. | ||
| * @param {string} [props.buttonText='Share'] - Label rendered inside the share button. | ||
| * @param {string} [props.customClass=''] - Extra CSS class(es) added to the share button. | ||
| * @param {Function} [props.onShare=null] - Callback fired after a platform link is opened. | ||
| * @param {Function} [props.onCopy=null] - Callback fired after the URL is copied to clipboard. | ||
| * @param {string} [props.buttonStyle='default'] - Visual variant of the trigger button (e.g. `'pill'`). | ||
| * @param {string} [props.modalPosition='center'] - Where the share modal appears: `'center'`, `'bottom'`, etc. | ||
| * @returns {JSX.Element} A single `<div>` container element into which the share button is mounted. | ||
| */ |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Prefer brief inline comments over method-level JSDoc in src/ files.
For this repository, comments in src should stay minimal and focused on non-obvious logic blocks rather than full JSDoc-style API docs.
As per coding guidelines "src/**/*.{js,jsx}: Use minimal, concise inline comments (not JSDoc style)..."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/social-share-button-react.jsx` around lines 3 - 26, Remove the
method-level JSDoc block and replace it with a short, concise inline comment
above the SocialShareButton component (or at the top of the file) summarizing
purpose and any non-obvious prop behavior; keep detailed API docs out of src/,
condense descriptions for props like url, title, hashtags, platforms, theme,
buttonText, onShare/onCopy into one-line notes only where necessary, and leave
the rest to prop names and default values in the component itself.
Comment on lines
+8
to
+30
| /** | ||
| * Creates a new SocialShareButton instance. | ||
| * Merges user-provided options with defaults, initialises instance state, | ||
| * and auto-calls init() when a container element is supplied. | ||
| * @param {Object} options - Configuration options for the share button. | ||
| * @param {string} [options.url] - URL to share; defaults to current page URL. | ||
| * @param {string} [options.title] - Title text; defaults to document.title. | ||
| * @param {string} [options.description] - Optional description appended to share text. | ||
| * @param {string[]} [options.hashtags] - Hashtag list (without '#') for supported platforms. | ||
| * @param {string} [options.via] - Twitter @username to attribute the tweet to. | ||
| * @param {string[]} [options.platforms] - Platforms to show; defaults to all six built-ins. | ||
| * @param {string} [options.theme] - Modal colour theme: 'dark' or 'light'. | ||
| * @param {string} [options.buttonText] - Label rendered inside the share button. | ||
| * @param {string} [options.customClass] - Extra CSS class(es) added to the share button. | ||
| * @param {string} [options.buttonColor] - Custom background colour for the share button. | ||
| * @param {string} [options.buttonHoverColor] - Background colour on hover (falls back to buttonColor). | ||
| * @param {Function} [options.onShare] - Callback fired after a platform link is opened. | ||
| * @param {Function} [options.onCopy] - Callback fired after the URL is copied. | ||
| * @param {string|Element} [options.container] - DOM element or CSS selector to mount the button into. | ||
| * @param {boolean} [options.showButton] - Whether to render the trigger button; defaults to true. | ||
| * @param {string} [options.buttonStyle] - Visual variant for the button (e.g. 'default', 'pill'). | ||
| * @param {string} [options.modalPosition] - Where the modal appears: 'center', 'bottom', etc. | ||
| */ |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Use concise inline comments instead of JSDoc blocks in src/ implementation methods.
The updated section is heavily JSDoc-based; this repo’s src guideline asks for brief inline comments focused on non-obvious logic/edge cases.
As per coding guidelines "src/**/*.{js,jsx}: Use minimal, concise inline comments (not JSDoc style)..."
🤖 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 8 - 30, The file contains a long
JSDoc block describing the SocialShareButton constructor options; per repo
guidelines replace this JSDoc with short inline comments inside the
implementation (e.g., near the SocialShareButton constructor function or class
constructor, and where options are merged in mergeOptions/init methods). Remove
the multi-line JSDoc block and instead add 1–2 brief inline comments that call
out only non-obvious behavior or edge cases (defaults merging, auto-init when a
container is supplied, and callbacks onShare/onCopy) next to the relevant
symbols (SocialShareButton, mergeOptions, init) so the code follows the src/*
comment style.
Comment on lines
+100
to
103
| // Auto-initialise when a container is provided at construction time. | ||
| if (this.options.container) { | ||
| this.init(); | ||
| } |
Contributor
There was a problem hiding this comment.
Guard auto-init for SSR/non-DOM environments.
Line 101 can call init() during construction if container is provided, but init() performs DOM operations. In SSR runtimes this can throw.
🧱 Proposed fix
- if (this.options.container) {
+ if (this.options.container && typeof document !== 'undefined') {
this.init();
}
}
init() {
+ if (typeof document === 'undefined') return;
if (this.options.showButton) {
this.createButton();Also applies to: 111-118
🤖 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 100 - 103, Guard the
auto-initialisation so it only runs in DOM-capable environments: before calling
this.init() when this.options.container is present (and in the similar block
around the other init path), check for browser globals (e.g. typeof window !==
'undefined' && typeof document !== 'undefined') or a small helper like
isDOMAvailable(); only call the instance method init() when that check passes to
avoid SSR/non-DOM errors in src/social-share-button.js (referencing
this.options.container and init()).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Addressed Issues:
Fixes #18
Screenshots/Recordings:
Not applicable (configuration-only changes)
Additional Notes:
This PR adds a .pre-commit-config.yaml file to enforce code quality checks before commits, following the open-source best practices outlined in issue #18.
The configuration includes:
Universal Git hygiene hooks:
Config & data validation:
Security:
This is one of the open-source best practices from the AOSSIE-Org Template-Repo that was missing from SocialShareButton.
Checklist
Summary by CodeRabbit
New Features
Documentation
Chores