Skip to content

Add YouTube video embed support to article reader #32

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add YouTube video embed support to article reader #32

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 9 additions & 1 deletion components/Feed/ArticleReader/ArticleReader.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,8 @@

/* Embedded content */
.reader-view-article-content iframe,
.reader-view-page-content iframe {
.reader-view-page-content iframe,
.reader-view-article iframe {
width: 100%;
max-height: 450px;
height: 100%;
Expand All @@ -129,6 +130,13 @@
aspect-ratio: 16/9;
}

/* YouTube embed specific styling */
.reader-view-article iframe[src*="youtube.com/embed"] {
min-height: 315px;
max-height: 500px;
box-shadow: 0 4px 6px -1px rgba(0, 0, 0, 0.1), 0 2px 4px -1px rgba(0, 0, 0, 0.06);
}

/* Article container */
.reader-view-article-content {
position: relative;
Expand Down
82 changes: 71 additions & 11 deletions components/Feed/ArticleReader/ArticleReader.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,43 @@ import ReactMarkdown from "react-markdown";
import remarkGfm from "remark-gfm";
import rehypeRaw from "rehype-raw";

// YouTube helper functions
const isYouTubeEmbedUrl = (url: string): boolean => {
return url.includes('youtube.com/embed/') || url.includes('youtu.be/');
};
Comment on lines +26 to +28
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Function name and logic inconsistency with YouTube URL detection.

The function name isYouTubeEmbedUrl suggests it only detects embed URLs, but it's missing the youtube.com/watch?v= format that extractYouTubeVideoId handles. This creates inconsistency where some YouTube URLs may pass the extraction but fail the detection.

Rename the function to be more accurate and consistent:

-const isYouTubeEmbedUrl = (url: string): boolean => {
-  return url.includes('youtube.com/embed/') || url.includes('youtu.be/');
+const isYouTubeUrl = (url: string): boolean => {
+  return url.includes('youtube.com/embed/') || 
+         url.includes('youtu.be/') || 
+         url.includes('youtube.com/watch?v=');

And update the usage accordingly in line 719.

Committable suggestion skipped: line range outside the PR's diff.

🤖 Prompt for AI Agents 
In components/Feed/ArticleReader/ArticleReader.tsx around lines 26 to 28, the
function named isYouTubeEmbedUrl does not accurately reflect its logic because
it misses the youtube.com/watch?v= URL format handled by extractYouTubeVideoId.
Rename this function to something like isYouTubeUrl to better represent its
purpose and update its logic to include the youtube.com/watch?v= pattern. Also,
update all usages of this function accordingly, including the one at line 719,
to maintain consistency.


const extractYouTubeVideoId = (url: string): string | null => {
try {
const urlObj = new URL(url);

// Handle youtube.com/embed/VIDEO_ID format
if (urlObj.hostname === 'www.youtube.com' || urlObj.hostname === 'youtube.com') {
const embedMatch = urlObj.pathname.match(/^\/embed\/([a-zA-Z0-9_-]+)/);
if (embedMatch) {
return embedMatch[1];
}

// Handle youtube.com/watch?v=VIDEO_ID format
const watchMatch = urlObj.searchParams.get('v');
if (watchMatch) {
return watchMatch;
}
}

// Handle youtu.be/VIDEO_ID format
if (urlObj.hostname === 'youtu.be') {
const shortMatch = urlObj.pathname.match(/^\/([a-zA-Z0-9_-]+)/);
if (shortMatch) {
return shortMatch[1];
}
}

return null;
} catch {
return null;
}
};
Comment on lines +30 to +60
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick (assertive)

Consider enhancing video ID validation for better accuracy.

The function correctly handles multiple YouTube URL formats and error cases. However, YouTube video IDs are consistently 11 characters long, which could be used for additional validation.

Consider adding video ID length validation:

 const extractYouTubeVideoId = (url: string): string | null => {
   try {
     const urlObj = new URL(url);
     
     // Handle youtube.com/embed/VIDEO_ID format
     if (urlObj.hostname === 'www.youtube.com' || urlObj.hostname === 'youtube.com') {
-      const embedMatch = urlObj.pathname.match(/^\/embed\/([a-zA-Z0-9_-]+)/);
+      const embedMatch = urlObj.pathname.match(/^\/embed\/([a-zA-Z0-9_-]{11})/);
       if (embedMatch) {
         return embedMatch[1];
       }
       
       // Handle youtube.com/watch?v=VIDEO_ID format
       const watchMatch = urlObj.searchParams.get('v');
-      if (watchMatch) {
+      if (watchMatch && watchMatch.length === 11) {
         return watchMatch;
       }
     }
     
     // Handle youtu.be/VIDEO_ID format
     if (urlObj.hostname === 'youtu.be') {
-      const shortMatch = urlObj.pathname.match(/^\/([a-zA-Z0-9_-]+)/);
+      const shortMatch = urlObj.pathname.match(/^\/([a-zA-Z0-9_-]{11})/);
       if (shortMatch) {
         return shortMatch[1];
       }
     }
     
     return null;
   } catch {
     return null;
   }
 };
📝 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
const extractYouTubeVideoId = (url: string): string | null => {
try {
const urlObj = new URL(url);
// Handle youtube.com/embed/VIDEO_ID format
if (urlObj.hostname === 'www.youtube.com' || urlObj.hostname === 'youtube.com') {
const embedMatch = urlObj.pathname.match(/^\/embed\/([a-zA-Z0-9_-]+)/);
if (embedMatch) {
return embedMatch[1];
}
// Handle youtube.com/watch?v=VIDEO_ID format
const watchMatch = urlObj.searchParams.get('v');
if (watchMatch) {
return watchMatch;
}
}
// Handle youtu.be/VIDEO_ID format
if (urlObj.hostname === 'youtu.be') {
const shortMatch = urlObj.pathname.match(/^\/([a-zA-Z0-9_-]+)/);
if (shortMatch) {
return shortMatch[1];
}
}
return null;
} catch {
return null;
}
};
const extractYouTubeVideoId = (url: string): string | null => {
try {
const urlObj = new URL(url);
// Handle youtube.com/embed/VIDEO_ID format
if (urlObj.hostname === 'www.youtube.com' || urlObj.hostname === 'youtube.com') {
const embedMatch = urlObj.pathname.match(/^\/embed\/([a-zA-Z0-9_-]{11})/);
if (embedMatch) {
return embedMatch[1];
}
// Handle youtube.com/watch?v=VIDEO_ID format
const watchMatch = urlObj.searchParams.get('v');
if (watchMatch && watchMatch.length === 11) {
return watchMatch;
}
}
// Handle youtu.be/VIDEO_ID format
if (urlObj.hostname === 'youtu.be') {
const shortMatch = urlObj.pathname.match(/^\/([a-zA-Z0-9_-]{11})/);
if (shortMatch) {
return shortMatch[1];
}
}
return null;
} catch {
return null;
}
};
🤖 Prompt for AI Agents 
In components/Feed/ArticleReader/ArticleReader.tsx between lines 30 and 60, the
extractYouTubeVideoId function extracts video IDs but does not validate their
length. To improve accuracy, add a check to ensure the extracted video ID is
exactly 11 characters long before returning it. If the ID length is incorrect,
return null instead.


export const ArticleImage = memo(
({
src,
Expand Down Expand Up @@ -677,17 +714,40 @@ export const ArticleContent = memo(
li: ({ children }: { children: React.ReactNode }) => (
<li className="leading-relaxed">{children}</li>
),
a: ({ href, children, ...props }: { href?: string; children: React.ReactNode; [key: string]: unknown }) => (
<a
href={href}
className="text-primary hover:text-primary/80 underline decoration-primary/30 hover:decoration-primary/60 transition-colors"
target={href?.startsWith('http') ? '_blank' : undefined}
rel={href?.startsWith('http') ? 'noopener noreferrer' : undefined}
{...props}
>
{children}
</a>
),
a: ({ href, children, ...props }: { href?: string; children?: React.ReactNode; [key: string]: unknown }) => {
// Check if this is a YouTube embed link
if (href && isYouTubeEmbedUrl(href)) {
const videoId = extractYouTubeVideoId(href);
if (videoId) {
// Return the iframe directly - the nesting warning can be safely ignored
// as this provides better user experience than showing just a link
return (
<iframe
src={`https://www.youtube.com/embed/${videoId}`}
title="YouTube video player"
className="w-full aspect-video rounded-lg my-8 block"
frameBorder="0"
allow="accelerometer; autoplay; clipboard-write; encrypted-media; gyroscope; picture-in-picture; web-share"
allowFullScreen
loading="lazy"
style={{ display: 'block', margin: '2rem auto' }}
/>
);
}
}

return (
<a
href={href}
className="text-primary hover:text-primary/80 underline decoration-primary/30 hover:decoration-primary/60 transition-colors"
target={href?.startsWith('http') ? '_blank' : undefined}
rel={href?.startsWith('http') ? 'noopener noreferrer' : undefined}
{...props}
>
{children}
</a>
);
},
Comment on lines +717 to +750
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick (assertive)

Excellent YouTube embed implementation with minor security enhancement opportunities.

The implementation correctly handles YouTube URL detection, video ID extraction, and fallback behavior. The accessibility and performance considerations (lazy loading, proper titles) are well implemented.

Consider adding sandbox attributes for enhanced security:

 return (
   <iframe
     src={`https://www.youtube.com/embed/${videoId}`}
     title="YouTube video player"
     className="w-full aspect-video rounded-lg my-8 block"
     frameBorder="0"
+    sandbox="allow-scripts allow-same-origin allow-presentation"
     allow="accelerometer; autoplay; clipboard-write; encrypted-media; gyroscope; picture-in-picture; web-share"
     allowFullScreen
     loading="lazy"
     style={{ display: 'block', margin: '2rem auto' }}
   />
 );

The sandbox attribute provides an additional layer of security by restricting iframe capabilities to only what's necessary for YouTube embeds.

📝 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
a: ({ href, children, ...props }: { href?: string; children?: React.ReactNode; [key: string]: unknown }) => {
// Check if this is a YouTube embed link
if (href && isYouTubeEmbedUrl(href)) {
const videoId = extractYouTubeVideoId(href);
if (videoId) {
// Return the iframe directly - the nesting warning can be safely ignored
// as this provides better user experience than showing just a link
return (
<iframe
src={`https://www.youtube.com/embed/${videoId}`}
title="YouTube video player"
className="w-full aspect-video rounded-lg my-8 block"
frameBorder="0"
allow="accelerometer; autoplay; clipboard-write; encrypted-media; gyroscope; picture-in-picture; web-share"
allowFullScreen
loading="lazy"
style={{ display: 'block', margin: '2rem auto' }}
/>
);
}
}
return (
<a
href={href}
className="text-primary hover:text-primary/80 underline decoration-primary/30 hover:decoration-primary/60 transition-colors"
target={href?.startsWith('http') ? '_blank' : undefined}
rel={href?.startsWith('http') ? 'noopener noreferrer' : undefined}
{...props}
>
{children}
</a>
);
},
a: ({ href, children, ...props }: { href?: string; children?: React.ReactNode; [key: string]: unknown }) => {
// Check if this is a YouTube embed link
if (href && isYouTubeEmbedUrl(href)) {
const videoId = extractYouTubeVideoId(href);
if (videoId) {
// Return the iframe directly - the nesting warning can be safely ignored
// as this provides better user experience than showing just a link
return (
<iframe
src={`https://www.youtube.com/embed/${videoId}`}
title="YouTube video player"
className="w-full aspect-video rounded-lg my-8 block"
frameBorder="0"
+ sandbox="allow-scripts allow-same-origin allow-presentation"
allow="accelerometer; autoplay; clipboard-write; encrypted-media; gyroscope; picture-in-picture; web-share"
allowFullScreen
loading="lazy"
style={{ display: 'block', margin: '2rem auto' }}
/>
);
}
}
return (
<a
href={href}
className="text-primary hover:text-primary/80 underline decoration-primary/30 hover:decoration-primary/60 transition-colors"
target={href?.startsWith('http') ? '_blank' : undefined}
rel={href?.startsWith('http') ? 'noopener noreferrer' : undefined}
{...props}
>
{children}
</a>
);
},
🤖 Prompt for AI Agents 
In components/Feed/ArticleReader/ArticleReader.tsx around lines 717 to 750, the
YouTube iframe embed is correctly implemented but lacks sandbox attributes for
enhanced security. Add a sandbox attribute to the iframe element with
appropriate values such as "allow-scripts allow-same-origin allow-presentation"
to restrict iframe capabilities while still allowing necessary functionality.
This will improve security by limiting what the embedded content can do.

}), []);

if (loading) {
Expand Down