-
-
Notifications
You must be signed in to change notification settings - Fork 3.1k
feat: youtube embeds #4841
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
base: main
Are you sure you want to change the base?
feat: youtube embeds #4841
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -0,0 +1,40 @@ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
import { memo } from "react"; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
import { cn } from "@/lib/utils"; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
interface Props { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
videoIds: string[]; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
const MemoYoutubeEmbedListView: React.FC<Props> = ({ videoIds }: Props) => { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
if (!videoIds || videoIds.length === 0) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
return null; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
const EmbedCard = ({ videoId, className }: { videoId: string; className?: string }) => { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
return ( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
<div className={cn("relative w-full", className)}> | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
<div className="relative w-full pt-[56.25%] rounded-lg overflow-hidden border border-border/60 bg-popover"> | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
<iframe | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
className="absolute top-0 left-0 w-full h-full" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
src={`https://www.youtube.com/embed/${videoId}`} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The videoId is directly interpolated into the iframe src without validation or sanitization. Although the video IDs are extracted using regex patterns, consider adding additional validation to ensure the videoId contains only expected characters (alphanumeric, hyphens, underscores) to prevent potential XSS attacks. Copilot uses AI. Check for mistakes. Positive FeedbackNegative Feedback |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
title="YouTube video player" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
allow="accelerometer; clipboard-write; encrypted-media; gyroscope; picture-in-picture; web-share" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [nitpick] The iframe permissions are quite permissive, including clipboard-write and accelerometer access. Consider restricting permissions to only what's necessary for video playback (e.g., encrypted-media, picture-in-picture) to follow the principle of least privilege.
Suggested change
Copilot uses AI. Check for mistakes. Positive FeedbackNegative Feedback |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
allowFullScreen | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
/> | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
</div> | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
</div> | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
}; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
return ( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
<div className="w-full flex flex-row justify-start overflow-auto gap-2"> | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
{videoIds.map((id) => ( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
<div key={id} className="w-80 flex flex-col justify-start items-start shrink-0"> | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
<EmbedCard videoId={id} className="max-h-64 grow" /> | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
</div> | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
))} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
</div> | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
}; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Comment on lines
+13
to
+38
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The EmbedCard component is defined inside the render function, which means it will be recreated on every render. Consider moving this component outside of the main component or using useCallback to optimize performance.
Suggested change
Copilot uses AI. Check for mistakes. Positive FeedbackNegative Feedback There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I already commented on Copilots "suggestions" once. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
export default memo(MemoYoutubeEmbedListView); |
Original file line number | Diff line number | Diff line change | ||||||
---|---|---|---|---|---|---|---|---|
@@ -0,0 +1,85 @@ | ||||||||
import { Node, NodeType } from "@/types/proto/api/v1/markdown_service"; | ||||||||
|
||||||||
// Regular expressions to match various YouTube URL formats. | ||||||||
const YOUTUBE_REGEXPS: RegExp[] = [ | ||||||||
/https?:\/\/(?:www\.)?youtube\.com\/watch\?v=([^&\s]+)/i, | ||||||||
/https?:\/\/(?:www\.)?youtu\.be\/([^?\s]+)/i, | ||||||||
/https?:\/\/(?:www\.)?youtube\.com\/shorts\/([^?\s]+)/i, | ||||||||
/https?:\/\/(?:www\.)?youtube\.com\/embed\/([^?\s]+)/i, | ||||||||
]; | ||||||||
|
||||||||
/** | ||||||||
* Extract the YouTube video ID from a given URL, if any. | ||||||||
* @param url The URL string to parse. | ||||||||
* @returns The video ID, or undefined if the URL is not a YouTube link. | ||||||||
*/ | ||||||||
export const extractYoutubeIdFromUrl = (url: string): string | undefined => { | ||||||||
for (const regexp of YOUTUBE_REGEXPS) { | ||||||||
const match = url.match(regexp); | ||||||||
if (match?.[1]) { | ||||||||
return match[1]; | ||||||||
} | ||||||||
} | ||||||||
return undefined; | ||||||||
}; | ||||||||
|
||||||||
/** | ||||||||
* Extract YouTube video IDs from markdown nodes. | ||||||||
* @param nodes The array of markdown nodes to extract YouTube video IDs from. | ||||||||
* @returns A deduplicated array of YouTube video IDs. | ||||||||
*/ | ||||||||
export const extractYoutubeVideoIdsFromNodes = (nodes: Node[]): string[] => { | ||||||||
const ids = new Set<string>(); | ||||||||
|
||||||||
const isNodeArray = (value: unknown): value is Node[] => | ||||||||
Array.isArray(value) && | ||||||||
value.length > 0 && | ||||||||
typeof value[0] === "object" && | ||||||||
value[0] !== null && | ||||||||
"type" in (value[0] as Record<string, unknown>); | ||||||||
|
||||||||
// Collect all child Node instances nested anywhere inside the given node | ||||||||
const collectChildren = (node: Node): Node[] => { | ||||||||
const collected: Node[] = []; | ||||||||
const queue: unknown[] = Object.values(node); | ||||||||
|
||||||||
while (queue.length) { | ||||||||
const item = queue.shift(); | ||||||||
if (!item) continue; | ||||||||
|
||||||||
if (isNodeArray(item)) { | ||||||||
collected.push(...item); | ||||||||
continue; | ||||||||
} | ||||||||
|
||||||||
if (Array.isArray(item)) { | ||||||||
queue.push(...item); | ||||||||
} else if (typeof item === "object") { | ||||||||
queue.push(...Object.values(item as Record<string, unknown>)); | ||||||||
} | ||||||||
} | ||||||||
|
||||||||
return collected; | ||||||||
}; | ||||||||
|
||||||||
const stack: Node[] = [...nodes]; | ||||||||
|
||||||||
while (stack.length) { | ||||||||
const node = stack.pop()!; | ||||||||
renja-g marked this conversation as resolved.
Show resolved
Hide resolved
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Using the non-null assertion operator (!) on stack.pop() assumes the stack is never empty, but this could lead to runtime errors if the stack becomes empty unexpectedly. Consider using a safer approach like checking if the popped value exists before proceeding.
Suggested change
Copilot uses AI. Check for mistakes. Positive FeedbackNegative Feedback |
||||||||
|
||||||||
if (node.type === NodeType.LINK && node.linkNode) { | ||||||||
const id = extractYoutubeIdFromUrl(node.linkNode.url); | ||||||||
if (id) ids.add(id); | ||||||||
} else if (node.type === NodeType.AUTO_LINK && node.autoLinkNode) { | ||||||||
const id = extractYoutubeIdFromUrl(node.autoLinkNode.url); | ||||||||
if (id) ids.add(id); | ||||||||
} | ||||||||
|
||||||||
const children = collectChildren(node); | ||||||||
if (children.length) { | ||||||||
stack.push(...children); | ||||||||
} | ||||||||
} | ||||||||
|
||||||||
return [...ids]; | ||||||||
}; |
Uh oh!
There was an error while loading. Please reload this page.