-
Notifications
You must be signed in to change notification settings - Fork 14
feat: added link obfuscation prop #435
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: added link obfuscation prop #435
Conversation
This adds a new prop to obfuscate links in the SocialShare component
Summary of ChangesHello @JoPrignol, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
Welcome, and thank you for your first contribution! This is a great feature addition for improving SEO. The implementation is well on its way. I've left a couple of suggestions to enhance security and robustness, particularly around opening links in new tabs. Great job on identifying the need to add border: none for the button version to maintain visual consistency. Looking forward to your next contributions!
| <component | ||
| :is="isObfuscated ? 'button' : 'a'" | ||
| v-if="selectedNetwork" | ||
| :href="!isObfuscated ? selectedNetwork.shareUrl : null" | ||
| :aria-label="`Share with ${selectedNetwork.name}`" | ||
| target="_blank" | ||
| class="social-share-button" | ||
| :class="[`social-share-button--${network}`, { 'social-share-button--styled': isStyled }]" | ||
| :href="selectedNetwork.shareUrl" | ||
| :style="`--color-brand:${selectedNetwork.color}`" | ||
| :aria-label="`Share with ${selectedNetwork.name}`" | ||
| target="_blank" | ||
| @click="isObfuscated ? openLink(selectedNetwork.shareUrl) : null" | ||
| > |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To improve security and HTML validity, I suggest a few changes:
- Security: When using
target="_blank", it's a best practice to also addrel="noopener noreferrer"to prevent potential security vulnerabilities (tabnabbing). - HTML Validity: The
targetattribute is not valid on a<button>element. It's better to apply it conditionally, only when rendering an<a>tag. The same applies to therelattribute.
The suggestion below applies these changes.
<component
:is="isObfuscated ? 'button' : 'a'"
v-if="selectedNetwork"
:href="!isObfuscated ? selectedNetwork.shareUrl : null"
:aria-label="`Share with ${selectedNetwork.name}`"
:target="!isObfuscated ? '_blank' : null"
:rel="!isObfuscated ? 'noopener noreferrer' : null"
class="social-share-button"
:class="[`social-share-button--${network}`, { 'social-share-button--styled': isStyled }]"
:style="`--color-brand:${selectedNetwork.color}`"
@click="isObfuscated ? openLink(selectedNetwork.shareUrl) : null"
>
| function openLink(url) { | ||
| const newWindow = window.open(url, '_blank') | ||
| newWindow.focus() | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The openLink function can be made more robust and secure:
- If a user has a popup blocker,
window.open()might returnnull. Calling.focus()onnullwill cause a runtime error. It's good practice to check if the window was successfully opened before trying to focus it. - For security, it's best to use the
noopener,noreferreroptions withwindow.open(), similar to therelattribute on an<a>tag.
function openLink(url) {
const newWindow = window.open(url, '_blank', 'noopener,noreferrer')
if (newWindow) newWindow.focus()
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR adds a link obfuscation feature to the SocialShare component through a new isObfuscated prop. When enabled, social share links are rendered as buttons that programmatically open URLs instead of standard anchor tags, which can help with SEO and internal linking optimization.
Key Changes:
- Added
isObfuscatedboolean prop to toggle between button and anchor tag rendering - Implemented
openLinkfunction to handle programmatic link opening - Updated styles by removing
@layer componentswrapper and adding button reset styles
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| function openLink(url) { | ||
| const newWindow = window.open(url, '_blank') | ||
| newWindow.focus() |
Copilot
AI
Nov 25, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The openLink function doesn't handle the case where window.open() returns null (when popup is blocked). Calling .focus() on a null reference will throw a runtime error. Add a null check before calling focus:
function openLink(url) {
const newWindow = window.open(url, '_blank')
if (newWindow) {
newWindow.focus()
}
}| newWindow.focus() | |
| if (newWindow) { | |
| newWindow.focus() | |
| } |
| :style="`--color-brand:${selectedNetwork.color}`" | ||
| :aria-label="`Share with ${selectedNetwork.name}`" | ||
| target="_blank" | ||
| @click="isObfuscated ? openLink(selectedNetwork.shareUrl) : null" |
Copilot
AI
Nov 25, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The @click handler should prevent the default action for the button element and should use a proper function reference. The current implementation @click="isObfuscated ? openLink(selectedNetwork.shareUrl) : null" will execute on every click even when isObfuscated is false. Consider:
@click="isObfuscated && openLink(selectedNetwork.shareUrl)"Or better yet, use a method:
@click="handleClick"And in the script:
function handleClick(event) {
if (isObfuscated) {
event.preventDefault()
openLink(selectedNetwork.shareUrl)
}
}| v-if="selectedNetwork" | ||
| :href="!isObfuscated ? selectedNetwork.shareUrl : null" | ||
| :aria-label="`Share with ${selectedNetwork.name}`" | ||
| target="_blank" |
Copilot
AI
Nov 25, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When rendered as a <button>, the target="_blank" attribute is invalid and has no effect on buttons. This attribute should be conditionally applied only when the component is rendered as an <a> tag:
:target="!isObfuscated ? '_blank' : null"Or use v-bind conditionally to avoid adding invalid attributes to buttons.
| target="_blank" | |
| :target="!isObfuscated ? '_blank' : null" |
| gap: 0.5em; | ||
| align-items: center; | ||
| text-decoration: none; | ||
| width: min-content; |
Copilot
AI
Nov 25, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The border: none; style was added to .social-share-button--styled, but buttons rendered without this class will still have default browser button styles (borders, background, etc.) when isObfuscated is true. Consider adding button reset styles to .social-share-button to ensure consistent appearance:
.social-share-button {
display: flex;
gap: 0.5em;
align-items: center;
text-decoration: none;
width: min-content;
border: none;
background: none;
padding: 0;
cursor: pointer;
font: inherit;
}| width: min-content; | |
| width: min-content; | |
| border: none; | |
| background: none; | |
| padding: 0; | |
| cursor: pointer; | |
| font: inherit; |
| function openLink(url) { | ||
| const newWindow = window.open(url, '_blank') | ||
| newWindow.focus() | ||
| } |
Copilot
AI
Nov 25, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Opening a link with window.open() and target="_blank" without specifying rel="noopener noreferrer" attributes (for anchor tags) or equivalent security measures poses a security risk. The opened page can access the window.opener property and potentially redirect the parent page.
For the anchor tag case, add:
:rel="!isObfuscated ? 'noopener noreferrer' : null"For the button case with window.open(), while the security risk is lower in modern browsers, it's still good practice to set opener to null or use the third parameter:
function openLink(url) {
const newWindow = window.open(url, '_blank', 'noopener,noreferrer')
if (newWindow) {
newWindow.focus()
}
}|
@JoPrignol Thanks for your contribution. For a general consideration of the proposed solution, I have already answered you on the linked issue #434 Since I guess that opening this PR was also for educational purposes on your side, I added some extra considerations about this PR quality:
Before continuing to work on this PR (that, for reasons explained on the #434 issue, I'm not inclined to merge), let's continue the discussion on the issue. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.
|
closed in favor of #436 |
Added a link obfuscation prop.
This adds a new optional 'isObfuscated' prop to obfuscate links in the SocialShare component. This props takes a boolean.
When a link is obfuscated, it is rendered with a tag that triggers the 'openLink' function that opens the link in a new tab. When it's not obfuscated, it is rendered with a tag, as usual.
This is useful for SEO purposes in order to optimize the site's internal linking.
Resolves: #434 --> [Feature]: Add a props for link obfuscation
This is my first GitHub contribution, so please don't hesitate to give me feedback :)