-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
feat(web): play videos inline on bookmark cards #2895
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?
Changes from 1 commit
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 |
|---|---|---|
| @@ -1,8 +1,11 @@ | ||
| "use client"; | ||
|
|
||
| import { useState } from "react"; | ||
| import Image from "next/image"; | ||
| import Link from "next/link"; | ||
| import { useUserSettings } from "@/lib/userSettings"; | ||
| import { cn } from "@/lib/utils"; | ||
| import { Play } from "lucide-react"; | ||
|
|
||
| import type { ZBookmarkTypeLink } from "@karakeep/shared/types/bookmarks"; | ||
| import { | ||
|
|
@@ -39,6 +42,113 @@ function LinkTitle({ bookmark }: { bookmark: ZBookmarkTypeLink }) { | |
| ); | ||
| } | ||
|
|
||
| function getYouTubeId(url: string): string | null { | ||
| const m = url.match( | ||
| /(?:youtube\.com\/(?:watch\?(?:.*&)?v=|embed\/|shorts\/|v\/)|youtu\.be\/)([\w-]{11})/, | ||
| ); | ||
| return m ? m[1] : null; | ||
| } | ||
|
|
||
| function getVimeoId(url: string): string | null { | ||
| const m = url.match(/vimeo\.com\/(?:video\/)?(\d+)/); | ||
| return m ? m[1] : null; | ||
| } | ||
|
|
||
| function isVideoUrl(url: string): boolean { | ||
| return !!(getYouTubeId(url) || getVimeoId(url)); | ||
| } | ||
|
|
||
| /** | ||
| * Plays videos inline on the bookmark card: | ||
| * - self-hosted videos archived by yt-dlp (videoAssetId) via a native <video> | ||
| * player (the asset route supports range requests, so seeking works); | ||
| * - YouTube / Vimeo links via their privacy-friendly embeds, shown as a poster | ||
| * with a play button until the user clicks (so we don't mount an iframe per | ||
| * card up front). | ||
| * | ||
| * It fills whatever container the active layout provides (passed as className). | ||
| */ | ||
| function VideoEmbed({ | ||
| url, | ||
| title, | ||
| videoAssetId, | ||
| posterUrl, | ||
| className, | ||
| }: { | ||
| url: string; | ||
| title?: string | null; | ||
| videoAssetId?: string | null; | ||
| posterUrl?: string | null; | ||
| className?: string; | ||
| }) { | ||
| const [playing, setPlaying] = useState(false); | ||
| const youTubeId = videoAssetId ? null : getYouTubeId(url); | ||
| const vimeoId = videoAssetId || youTubeId ? null : getVimeoId(url); | ||
|
|
||
| let inner: React.ReactNode = null; | ||
| if (videoAssetId) { | ||
| inner = ( | ||
| <video | ||
| controls | ||
| preload="metadata" | ||
| playsInline | ||
| poster={posterUrl ?? undefined} | ||
| className="absolute inset-0 h-full w-full object-contain" | ||
| src={`/api/assets/${videoAssetId}`} | ||
| > | ||
| <track kind="captions" /> | ||
| </video> | ||
| ); | ||
| } else if (youTubeId || vimeoId) { | ||
| const embedSrc = youTubeId | ||
| ? `https://www.youtube-nocookie.com/embed/${youTubeId}?autoplay=1&rel=0` | ||
| : `https://player.vimeo.com/video/${vimeoId}?autoplay=1`; | ||
| inner = playing ? ( | ||
| <iframe | ||
| src={embedSrc} | ||
| title={title ?? "video"} | ||
| className="absolute inset-0 h-full w-full" | ||
| allow="autoplay; encrypted-media; picture-in-picture" | ||
| allowFullScreen | ||
| /> | ||
| ) : ( | ||
|
Contributor
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.
Without a Prompt To Fix With AIThis is a comment left during a code review.
Path: apps/web/components/dashboard/bookmarks/LinkCard.tsx
Line: 114
Comment:
**Missing `sandbox` attribute on embedded iframe**
Without a `sandbox` attribute the embedded YouTube/Vimeo iframe can navigate the top-level frame (`window.top.location`), open popups, and run scripts without restriction relative to its own origin. A permissive but still scoped sandbox — `"allow-scripts allow-same-origin allow-presentation allow-popups allow-popups-to-escape-sandbox"` — keeps autoplay and full-screen working while preventing top-navigation by the embed. The existing `allow` feature policy alone does not constrain these capabilities.
How can I resolve this? If you propose a fix, please make it concise. |
||
| <button | ||
| type="button" | ||
| onClick={(e) => { | ||
| e.preventDefault(); | ||
| e.stopPropagation(); | ||
| setPlaying(true); | ||
| }} | ||
| className="group/play absolute inset-0 h-full w-full" | ||
| aria-label="Play video" | ||
| > | ||
| {youTubeId && ( | ||
| <Image | ||
| src={`https://i.ytimg.com/vi/${youTubeId}/hqdefault.jpg`} | ||
| alt={title ?? "video thumbnail"} | ||
| fill | ||
| unoptimized | ||
| className="object-cover" | ||
| /> | ||
| )} | ||
| <span className="absolute inset-0 flex items-center justify-center bg-black/10 transition group-hover/play:bg-black/25"> | ||
| <span className="flex h-14 w-14 items-center justify-center rounded-full bg-black/55 backdrop-blur-sm transition group-hover/play:scale-110 group-hover/play:bg-black/75"> | ||
| <Play className="ml-0.5 h-6 w-6 fill-white text-white" /> | ||
| </span> | ||
| </span> | ||
| </button> | ||
|
Comment on lines
+121
to
+145
Contributor
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.
When Prompt To Fix With AIThis is a comment left during a code review.
Path: apps/web/components/dashboard/bookmarks/LinkCard.tsx
Line: 115-139
Comment:
**Vimeo preview shows no thumbnail before play**
When `vimeoId` is set and `playing` is `false`, the play button renders over a plain dark background — there is no poster image, unlike YouTube which fetches `i.ytimg.com/vi/${youTubeId}/hqdefault.jpg`. Vimeo thumbnails are available without authentication via their oEmbed endpoint (`https://vimeo.com/api/oembed.json?url=...`) or the static CDN pattern, so users see an uninformative black card until they click. The `{youTubeId && <Image … />}` guard on line 125 is the root of the discrepancy — there is no equivalent branch for Vimeo.
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time! |
||
| ); | ||
| } else { | ||
| return null; | ||
| } | ||
|
|
||
| return ( | ||
| <div className={cn("relative overflow-hidden bg-black", className)}> | ||
| {inner} | ||
| </div> | ||
| ); | ||
| } | ||
|
|
||
| function LinkImage({ | ||
| bookmark, | ||
| className, | ||
|
|
@@ -96,15 +206,28 @@ export default function LinkCard({ | |
| className?: string; | ||
| bookmarkIndex?: number; | ||
| }) { | ||
| const link = bookmarkLink.content; | ||
| const videoAssetId = link.videoAssetId; | ||
| const isVideo = !!videoAssetId || isVideoUrl(link.url); | ||
| return ( | ||
| <BookmarkLayoutAdaptingCard | ||
| title={<LinkTitle bookmark={bookmarkLink} />} | ||
| footer={<FooterLinkURL url={getSourceUrl(bookmarkLink)} />} | ||
| bookmark={bookmarkLink} | ||
| wrapTags={false} | ||
| image={(_layout, className) => ( | ||
| <LinkImage className={className} bookmark={bookmarkLink} /> | ||
| )} | ||
| image={(_layout, className) => | ||
| isVideo ? ( | ||
| <VideoEmbed | ||
| url={link.url} | ||
| title={getBookmarkTitle(bookmarkLink)} | ||
| videoAssetId={videoAssetId} | ||
| posterUrl={getBookmarkLinkImageUrl(link)?.url ?? null} | ||
| className={className} | ||
| /> | ||
| ) : ( | ||
| <LinkImage className={className} bookmark={bookmarkLink} /> | ||
| ) | ||
| } | ||
| className={className} | ||
| bookmarkIndex={bookmarkIndex} | ||
| /> | ||
|
|
||
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.
<track>element requires asrcattribute; omitting it produces a browser console warning and the track is effectively inert. The existing video inLinkContentSection.tsxuses aneslint-disablecomment to honestly skip the unfulfilled requirement. This<track>satisfies the static lint check but doesn't actually suppress the runtime warning.Prompt To Fix With AI
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!