From 64f46ffdce9e54da47d557b38a2678461353ca34 Mon Sep 17 00:00:00 2001 From: Matt Bertrand Date: Mon, 29 Jun 2026 16:38:32 -0400 Subject: [PATCH 01/10] Fix flaky test (#3536) --- .../LearningResourceDrawer/LearningResourceDrawer.test.tsx | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/frontends/main/src/page-components/LearningResourceDrawer/LearningResourceDrawer.test.tsx b/frontends/main/src/page-components/LearningResourceDrawer/LearningResourceDrawer.test.tsx index 8f32ac54d1..b178bbea49 100644 --- a/frontends/main/src/page-components/LearningResourceDrawer/LearningResourceDrawer.test.tsx +++ b/frontends/main/src/page-components/LearningResourceDrawer/LearningResourceDrawer.test.tsx @@ -98,7 +98,9 @@ describe("LearningResourceDrawer", () => { ])( "Renders drawer content when resource=id is in the URL and captures the view if PostHog $descriptor", async ({ enablePostHog }) => { - const { resource } = setupApis() + const { resource } = setupApis({ + resource: { resource_type: ResourceTypeEnum.Course }, + }) process.env.NEXT_PUBLIC_POSTHOG_API_KEY = enablePostHog ? "12345abcdef" // pragma: allowlist secret : "" From 967157ad9761a2a57008a448693e080f78946a39 Mon Sep 17 00:00:00 2001 From: Ahtesham Quraish Date: Tue, 30 Jun 2026 14:06:06 +0500 Subject: [PATCH 02/10] feat: Implement timestamp-aware share/embed links for video player (#3513) * feat: Implement timestamp-aware share/embed links for video player --------- Co-authored-by: Ahtesham Quraish --- .../VideoEmbedPage/VideoEmbedPage.tsx | 7 +- .../ShareDialog.test.tsx | 141 +++++++++++ .../ShareDialog.tsx | 98 +++++++- .../UpNextSection.tsx | 4 + .../VideoDetailPage.test.tsx | 40 +++ .../VideoDetailPage.tsx | 7 + .../VideoDetailPageRouter.tsx | 4 + .../VideoJsPlayer.tsx | 12 + .../VideoResourcePlayer.tsx | 232 +++++++++++------- .../VideoSeriesDetailPage.test.tsx | 86 ++++++- .../VideoSeriesDetailPage.tsx | 4 + .../VideoShareButton.test.tsx | 112 +++++++++ .../VideoShareButton.tsx | 4 + .../YouTubeIframePlayer.tsx | 149 +++++++++-- .../src/app/(embed)/video/embed/[id]/page.tsx | 15 +- .../src/app/(site)/video/[id]/[slug]/page.tsx | 10 +- 16 files changed, 799 insertions(+), 126 deletions(-) create mode 100644 frontends/main/src/app-pages/VideoPlaylistCollectionPage/VideoShareButton.test.tsx diff --git a/frontends/main/src/app-pages/VideoEmbedPage/VideoEmbedPage.tsx b/frontends/main/src/app-pages/VideoEmbedPage/VideoEmbedPage.tsx index 94ed9009a2..ac3b8640fc 100644 --- a/frontends/main/src/app-pages/VideoEmbedPage/VideoEmbedPage.tsx +++ b/frontends/main/src/app-pages/VideoEmbedPage/VideoEmbedPage.tsx @@ -13,9 +13,13 @@ const EmbedPlayer = styled(VideoResourcePlayer)({ type VideoEmbedPageProps = { videoResource: VideoResource + startTime?: number } -const VideoEmbedPage: React.FC = ({ videoResource }) => { +const VideoEmbedPage: React.FC = ({ + videoResource, + startTime, +}) => { const videoTitleLabel = videoResource.title.trim() return ( @@ -25,6 +29,7 @@ const VideoEmbedPage: React.FC = ({ videoResource }) => { isLoading={false} videoTitleLabel={videoTitleLabel} videoThumbnailAlt={`Video thumbnail for ${videoTitleLabel}`} + startTime={startTime} /> ) } diff --git a/frontends/main/src/app-pages/VideoPlaylistCollectionPage/ShareDialog.test.tsx b/frontends/main/src/app-pages/VideoPlaylistCollectionPage/ShareDialog.test.tsx index 4d8640e241..8a97999752 100644 --- a/frontends/main/src/app-pages/VideoPlaylistCollectionPage/ShareDialog.test.tsx +++ b/frontends/main/src/app-pages/VideoPlaylistCollectionPage/ShareDialog.test.tsx @@ -13,6 +13,31 @@ const makeVideo = (overrides: Partial = {}): VideoResource => ...overrides, }) as VideoResource +const makeYouTubeVideo = ( + overrides: Partial = {}, +): VideoResource => + makeVideo({ + url: "https://www.youtube.com/watch?v=abc123XYZ", + platform: { code: "youtube", name: "YouTube" } as VideoResource["platform"], + ...overrides, + }) + +const makeOvsVideo = (overrides: Partial = {}): VideoResource => + makeVideo({ + platform: { + code: "ovs", + name: "ODL Video Service", + } as VideoResource["platform"], + video: { + id: 1, + caption_urls: [], + streaming_url: "https://cdn.odl.mit.edu/video/stream.m3u8", + duration: "300", + cover_image_url: null, + }, + ...overrides, + }) + const renderDialog = (video: VideoResource = makeVideo()) => renderWithProviders( />, ) +const renderDialogWithTime = ( + getCurrentTime: () => number, + video: VideoResource = makeYouTubeVideo(), +) => + renderWithProviders( + , + ) + describe("VideoShareDialog", () => { describe("tab switching", () => { test("defaults to the Share tab", async () => { @@ -143,4 +183,105 @@ describe("VideoShareDialog", () => { expect(textarea.value).not.toContain("youtube.com") }) }) + + describe("timestamp-aware share links", () => { + describe("Share tab", () => { + test("does not append ?t= when video is at position 0", async () => { + renderDialogWithTime(() => 0) + expect(await screen.findByDisplayValue(PAGE_URL)).toBeInTheDocument() + }) + }) + + describe("Embed tab — timestamp checkbox", () => { + test("shows 'Start at' checkbox when video is mid-play", async () => { + renderDialogWithTime(() => 90) + await user.click(await screen.findByRole("tab", { name: /embed/i })) + expect( + screen.getByRole("checkbox", { name: /start at/i }), + ).toBeInTheDocument() + }) + + test("does not show 'Start at' checkbox when video is at position 0", async () => { + renderDialogWithTime(() => 0) + await user.click(await screen.findByRole("tab", { name: /embed/i })) + expect( + screen.queryByRole("checkbox", { name: /start at/i }), + ).not.toBeInTheDocument() + }) + + test("formats sub-hour timestamp as m:ss", async () => { + renderDialogWithTime(() => 90) // 1 min 30 sec + await user.click(await screen.findByRole("tab", { name: /embed/i })) + expect(screen.getByText("Start at 1:30")).toBeInTheDocument() + }) + + test("formats over-hour timestamp as h:mm:ss", async () => { + renderDialogWithTime(() => 3661) // 1 h 1 min 1 sec + await user.click(await screen.findByRole("tab", { name: /embed/i })) + expect(screen.getByText("Start at 1:01:01")).toBeInTheDocument() + }) + + test("unchecking 'Start at' removes timestamp from YouTube embed HTML", async () => { + renderDialogWithTime(() => 90) + await user.click(await screen.findByRole("tab", { name: /embed/i })) + await user.click(screen.getByRole("checkbox", { name: /start at/i })) + const textarea = screen.getByRole("textbox", { + name: "Embed HTML", + }) as HTMLTextAreaElement + expect(textarea.value).not.toContain("start=") + }) + + test("re-checking 'Start at' restores timestamp in YouTube embed HTML", async () => { + renderDialogWithTime(() => 90) + await user.click(await screen.findByRole("tab", { name: /embed/i })) + const checkbox = screen.getByRole("checkbox", { name: /start at/i }) + await user.click(checkbox) // uncheck + await user.click(checkbox) // re-check + const textarea = screen.getByRole("textbox", { + name: "Embed HTML", + }) as HTMLTextAreaElement + expect(textarea.value).toContain("start=90") + }) + }) + + describe("Embed tab — YouTube timestamp", () => { + test("YouTube embed HTML includes start= param", async () => { + renderDialogWithTime(() => 90) + await user.click(await screen.findByRole("tab", { name: /embed/i })) + const textarea = screen.getByRole("textbox", { + name: "Embed HTML", + }) as HTMLTextAreaElement + expect(textarea.value).toContain("start=90") + }) + + test("YouTube embed HTML omits start= when at position 0", async () => { + renderDialogWithTime(() => 0) + await user.click(await screen.findByRole("tab", { name: /embed/i })) + const textarea = screen.getByRole("textbox", { + name: "Embed HTML", + }) as HTMLTextAreaElement + expect(textarea.value).not.toContain("start=") + }) + }) + + describe("Embed tab — OVS timestamp", () => { + test("OVS embed HTML includes t= param", async () => { + renderDialogWithTime(() => 90, makeOvsVideo()) + await user.click(await screen.findByRole("tab", { name: /embed/i })) + const textarea = screen.getByRole("textbox", { + name: "Embed HTML", + }) as HTMLTextAreaElement + expect(textarea.value).toContain("t=90") + }) + + test("OVS embed HTML omits t= when at position 0", async () => { + renderDialogWithTime(() => 0, makeOvsVideo()) + await user.click(await screen.findByRole("tab", { name: /embed/i })) + const textarea = screen.getByRole("textbox", { + name: "Embed HTML", + }) as HTMLTextAreaElement + expect(textarea.value).not.toMatch(/[?&]t=/) + }) + }) + }) }) diff --git a/frontends/main/src/app-pages/VideoPlaylistCollectionPage/ShareDialog.tsx b/frontends/main/src/app-pages/VideoPlaylistCollectionPage/ShareDialog.tsx index df1f64048c..60587e75d8 100644 --- a/frontends/main/src/app-pages/VideoPlaylistCollectionPage/ShareDialog.tsx +++ b/frontends/main/src/app-pages/VideoPlaylistCollectionPage/ShareDialog.tsx @@ -152,6 +152,24 @@ const EmbedFooter = styled.div({ alignItems: "center", }) +const StartAtRow = styled.div(({ theme }) => ({ + display: "flex", + alignItems: "center", + gap: "8px", + ...theme.typography.body2, + color: theme.custom.colors.darkGray2, + "input[type='checkbox']": { + width: "16px", + height: "16px", + cursor: "pointer", + accentColor: theme.custom.colors.red, + }, + label: { + cursor: "pointer", + userSelect: "none", + }, +})) + // ─── Helpers ─────────────────────────────────────────────────────────────── async function copyToClipboard(text: string): Promise { @@ -180,21 +198,46 @@ function escapeHtmlAttr(value: string): string { .replace(/ 0 ? `${h}:${mm}:${ss}` : `${m}:${ss}` +} + +function withTimeParam(url: string, seconds: number, param = "t"): string { + try { + const u = new URL(url) + u.searchParams.set(param, String(Math.floor(seconds))) + return u.toString() + } catch { + return url + } +} + function buildVideoEmbedHtml( video: VideoResource, title: string, embedPageUrl: string, + startTime = 0, ): string { const isOvs = video.platform?.code === "ovs" || video.platform?.code === "ocw" const escapedTitle = escapeHtmlAttr(title) + const t = Math.floor(startTime) if (isOvs) { if (!embedPageUrl) return "" - return `` + const src = t > 0 ? withTimeParam(embedPageUrl, t) : embedPageUrl + return `` } const youtubeId = getYouTubeVideoId(video.url) if (!youtubeId) return "" - return `` + const ytBase = `https://www.youtube.com/embed/${youtubeId}` + const src = t > 0 ? withTimeParam(ytBase, t, "start") : ytBase + return `` } function buildPodcastEmbedHtml( @@ -218,6 +261,7 @@ type ShareDialogProps = { resource?: PodcastEpisodeResource pageUrl: string title: string + getCurrentTime?: () => number } const ShareDialog = ({ @@ -227,10 +271,24 @@ const ShareDialog = ({ resource, pageUrl, title, + + getCurrentTime, }: ShareDialogProps) => { const [activeTab, setActiveTab] = useState("share") const [copyLinkText, setCopyLinkText] = useState("Copy Link") const [copyEmbedText, setCopyEmbedText] = useState("Copy") + const [startTime, setStartTime] = useState(0) + const [includeTime, setIncludeTime] = useState(false) + + const wasOpenRef = useRef(false) + useEffect(() => { + if (open && !wasOpenRef.current) { + const t = getCurrentTime?.() ?? 0 + setStartTime(t) + setIncludeTime(t > 0) + } + wasOpenRef.current = open + }, [open, getCurrentTime]) const copyLinkTimerRef = useRef | null>(null) const copyEmbedTimerRef = useRef | null>(null) @@ -276,14 +334,23 @@ const ShareDialog = ({ ? `${NEXT_PUBLIC_ORIGIN}/podcast/embed/${resource.id}` : null + const t = includeTime && startTime > 0 ? Math.floor(startTime) : 0 + + const activeShareUrl = pageUrl + const activeEmbedUrl = + embedUrl && t > 0 ? withTimeParam(embedUrl, t) : embedUrl + const embedHtml = video - ? buildVideoEmbedHtml(video, title, videoEmbedPageUrl) + ? buildVideoEmbedHtml(video, title, videoEmbedPageUrl, t) : resource ? buildPodcastEmbedHtml(resource, title, embedUrl || "") : "" const embedUrlLabel = video ? "Video URL" : "Audio URL" + const startAtLabel = + video && startTime > 0 ? `Start at ${formatTimestamp(startTime)}` : null + return ( Share on Social { const input = event.currentTarget.querySelector("input") @@ -383,9 +450,9 @@ const ShareDialog = ({ variant="bordered" startIcon={} onClick={async () => { - if (!pageUrl) return + if (!activeShareUrl) return try { - await copyToClipboard(pageUrl) + await copyToClipboard(activeShareUrl) setCopyLinkText("Copied!") } catch { setCopyLinkText("Failed to copy") @@ -409,7 +476,7 @@ const ShareDialog = ({ {embedUrlLabel} { @@ -429,6 +496,17 @@ const ShareDialog = ({ /> + {startAtLabel && ( + + setIncludeTime(e.target.checked)} + /> + + + )} string currentVideo: VideoResource shareUrl: string + playerRef?: React.RefObject } const UpNextSection: React.FC = ({ @@ -22,6 +24,7 @@ const UpNextSection: React.FC = ({ getVideoHref, currentVideo, shareUrl, + playerRef, }) => { return ( @@ -34,6 +37,7 @@ const UpNextSection: React.FC = ({ video={currentVideo} title={currentVideo.title ?? ""} pageUrl={shareUrl} + playerRef={playerRef} /> { }, ) + describe("share button", () => { + test("is not shown while data is still loading", async () => { + const video = makeVideo({ title: "Loading Test" }) + renderPage({ video }) + // Synchronously after render the API hasn't resolved yet + expect( + screen.queryByRole("button", { name: /share/i }), + ).not.toBeInTheDocument() + // Confirm it does appear once loaded (proving the above was the loading state) + await screen.findByRole("button", { name: /share loading test/i }) + }) + + test("aria-label includes the video title", async () => { + const video = makeVideo({ title: "Quantum Computing" }) + renderPage({ video }) + expect( + await screen.findByRole("button", { name: /share quantum computing/i }), + ).toBeInTheDocument() + }) + + test("clicking the button opens the share dialog", async () => { + const video = makeVideo({ title: "Open Dialog Test" }) + renderPage({ video }) + await user.click( + await screen.findByRole("button", { name: /share open dialog test/i }), + ) + expect(screen.getByRole("dialog")).toBeInTheDocument() + }) + + test("closing the dialog removes it from the page", async () => { + const video = makeVideo({ title: "Close Dialog Test" }) + renderPage({ video }) + await user.click( + await screen.findByRole("button", { name: /share close dialog test/i }), + ) + await user.click(screen.getByRole("button", { name: /^close$/i })) + expect(screen.queryByRole("dialog")).not.toBeInTheDocument() + }) + }) + test("renders VideoResourcePlayer for the video", async () => { const video = makeVideo({ video: { diff --git a/frontends/main/src/app-pages/VideoPlaylistCollectionPage/VideoDetailPage.tsx b/frontends/main/src/app-pages/VideoPlaylistCollectionPage/VideoDetailPage.tsx index 262b5eb7dd..a24e7ccd3a 100644 --- a/frontends/main/src/app-pages/VideoPlaylistCollectionPage/VideoDetailPage.tsx +++ b/frontends/main/src/app-pages/VideoPlaylistCollectionPage/VideoDetailPage.tsx @@ -21,6 +21,7 @@ import { formatDurationClockTime } from "ol-utilities" import { videoDetailPageView, videoPlaylistPageView } from "@/common/urls" import { buildVideoStructuredData } from "./videoStructuredData" import VideoResourcePlayer from "./VideoResourcePlayer" +import type { VideoPlayerHandle } from "./VideoResourcePlayer" const NEXT_PUBLIC_ORIGIN = env("NEXT_PUBLIC_ORIGIN") @@ -291,6 +292,7 @@ type VideoDetailPageProps = { playlistId: number | null playlistData?: VideoPlaylistResource playlistLoading: boolean + startTime?: number } const VideoDetailPage: React.FC = ({ @@ -298,8 +300,10 @@ const VideoDetailPage: React.FC = ({ playlistId, playlistData, playlistLoading, + startTime, }) => { const titleRef = useRef(null) + const playerRef = useRef(null) const { data: resource, isLoading: videoLoading } = useLearningResourcesDetail(videoId) @@ -440,11 +444,13 @@ const VideoDetailPage: React.FC = ({ )} @@ -468,6 +474,7 @@ const VideoDetailPage: React.FC = ({ video={video} title={video.title ?? "video"} pageUrl={`${NEXT_PUBLIC_ORIGIN}${videoDetailPageView(video.id, playlistId ?? undefined, video.title)}`} + playerRef={playerRef} /> )} diff --git a/frontends/main/src/app-pages/VideoPlaylistCollectionPage/VideoDetailPageRouter.tsx b/frontends/main/src/app-pages/VideoPlaylistCollectionPage/VideoDetailPageRouter.tsx index fcb13f0f78..f9ab0a1fee 100644 --- a/frontends/main/src/app-pages/VideoPlaylistCollectionPage/VideoDetailPageRouter.tsx +++ b/frontends/main/src/app-pages/VideoPlaylistCollectionPage/VideoDetailPageRouter.tsx @@ -11,11 +11,13 @@ import { LoadingSpinner } from "ol-components" type VideoDetailPageRouterProps = { videoId: number playlistId: number | null + startTime?: number } const VideoDetailPageRouter: React.FC = ({ videoId, playlistId, + startTime, }) => { const { data: playlist, isLoading: playlistLoading } = useQuery({ ...videoPlaylistQueries.detail(playlistId ?? 0), @@ -31,6 +33,7 @@ const VideoDetailPageRouter: React.FC = ({ if (isOcw) { return ( = ({ playlistId={playlistId} playlistData={playlist} playlistLoading={playlistLoading} + startTime={startTime} /> ) } diff --git a/frontends/main/src/app-pages/VideoPlaylistCollectionPage/VideoJsPlayer.tsx b/frontends/main/src/app-pages/VideoPlaylistCollectionPage/VideoJsPlayer.tsx index 6b4867c687..a472b92d29 100644 --- a/frontends/main/src/app-pages/VideoPlaylistCollectionPage/VideoJsPlayer.tsx +++ b/frontends/main/src/app-pages/VideoPlaylistCollectionPage/VideoJsPlayer.tsx @@ -25,6 +25,7 @@ export type VideoJsPlayerProps = { playsinline?: boolean ariaLabel?: string ariaDescribedBy?: string + startTime?: number onReady?: (player: Player) => void } @@ -44,6 +45,7 @@ const VideoJsPlayer: React.FC = ({ playsinline = false, ariaLabel, ariaDescribedBy, + startTime, onReady, }) => { const videoRef = useRef(null) @@ -116,6 +118,15 @@ const VideoJsPlayer: React.FC = ({ // Add tracks inside the ready callback — this is the earliest safe // point; adding them before ready can silently fail on some browsers. addTracks(this, tracks) + if (startTime && startTime > 0) { + // Seek only after metadata (duration/seekable range) is available. + // Calling currentTime() in ready() runs before metadata loads, so the + // seek is silently ignored for non-YouTube (HTML5) sources. `one` + // fires a single time, so it seeks on initial load only. + this.one("loadedmetadata", () => { + this.currentTime(startTime) + }) + } onReady?.(this) // Set the flag here so the update effect only runs after the player // is truly ready and the initial setup is complete. @@ -136,6 +147,7 @@ const VideoJsPlayer: React.FC = ({ playsinline, poster, sources, + startTime, tracks, ]) diff --git a/frontends/main/src/app-pages/VideoPlaylistCollectionPage/VideoResourcePlayer.tsx b/frontends/main/src/app-pages/VideoPlaylistCollectionPage/VideoResourcePlayer.tsx index beb5835688..37dfa0d54d 100644 --- a/frontends/main/src/app-pages/VideoPlaylistCollectionPage/VideoResourcePlayer.tsx +++ b/frontends/main/src/app-pages/VideoPlaylistCollectionPage/VideoResourcePlayer.tsx @@ -1,15 +1,18 @@ "use client" -import React, { useMemo } from "react" +import React, { useImperativeHandle, useMemo, useRef } from "react" import dynamic from "next/dynamic" import Image from "next/image" import { Skeleton, styled } from "ol-components" import { NoVideoMessage } from "./shared.styled" import { resolveVideoSources } from "./videoSources" import { convertToEmbedUrl } from "@/common/utils" -import YouTubeIframePlayer from "./YouTubeIframePlayer" +import YouTubeIframePlayer, { + type YouTubePlayerHandle, +} from "./YouTubeIframePlayer" import type { VideoResource } from "api/v1" import type { VideoJsPlayerProps } from "./VideoJsPlayer" +import type Player from "video.js/dist/types/player" const VideoJsPlayer = dynamic( () => import("./VideoJsPlayer"), @@ -79,6 +82,10 @@ const ScreenReaderOnly = styled.span({ border: 0, }) +export type VideoPlayerHandle = { + getCurrentTime: () => number +} + export type VideoResourcePlayerProps = { video: VideoResource | undefined videoId: number @@ -86,6 +93,7 @@ export type VideoResourcePlayerProps = { videoTitleLabel: string videoThumbnailAlt: string ariaDescribedBy?: string + startTime?: number className?: string } @@ -95,98 +103,136 @@ export type VideoResourcePlayerProps = { * * Accepts `className` so it can be extended with `styled(VideoResourcePlayer)` * for per-page layout overrides (e.g. margin, border). + * + * Exposes a `VideoPlayerHandle` ref so callers can read `getCurrentTime()`. + * OVS (VideoJS) returns the actual playback position; YouTube returns 0 + * until the YouTube IFrame API is wired up. */ -const VideoResourcePlayer: React.FC = ({ - video, - videoId, - isLoading, - videoTitleLabel, - videoThumbnailAlt, - ariaDescribedBy = "video-description", - className, -}) => { - const sources = useMemo( - () => - video - ? resolveVideoSources( - video.video?.streaming_url, - video.url, - video.content_files?.[0]?.youtube_id, - ) - : [], - [video], - ) - - const captionUrls = video?.video?.caption_urls ?? [] - - const embedUrl = - sources[0]?.type === "video/youtube" - ? convertToEmbedUrl(sources[0].src) - : null - - const thumbnailUrl = - video?.image?.url ?? video?.content_files?.[0]?.image_src ?? null - - const posterUrl = - video?.video?.cover_image_url ?? - video?.content_files?.[0]?.image_src ?? - video?.image?.url ?? - undefined - - return ( - - {isLoading ? ( -
- -
- ) : embedUrl ? ( - - ) : sources.length > 0 ? ( - - ) : thumbnailUrl ? ( - - {videoThumbnailAlt}( + ( + { + video, + videoId, + isLoading, + videoTitleLabel, + videoThumbnailAlt, + ariaDescribedBy = "video-description", + startTime, + className, + }, + ref, + ) => { + const vjsPlayerRef = useRef(null) + const ytPlayerRef = useRef(null) + + const sources = useMemo( + () => + video + ? resolveVideoSources( + video.video?.streaming_url, + video.url, + video.content_files?.[0]?.youtube_id, + ) + : [], + [video], + ) + + const captionUrls = video?.video?.caption_urls ?? [] + + const embedUrl = + sources[0]?.type === "video/youtube" + ? convertToEmbedUrl(sources[0].src) + : null + + useImperativeHandle( + ref, + () => ({ + getCurrentTime: () => + embedUrl + ? (ytPlayerRef.current?.getCurrentTime() ?? 0) + : (vjsPlayerRef.current?.currentTime() ?? 0), + }), + [embedUrl], + ) + + const thumbnailUrl = + video?.image?.url ?? video?.content_files?.[0]?.image_src ?? null + + const posterUrl = + video?.video?.cover_image_url ?? + video?.content_files?.[0]?.image_src ?? + video?.image?.url ?? + undefined + + return ( + + {isLoading ? ( +
+ +
+ ) : embedUrl ? ( + - - ) : ( - <> - - No playable source available for this video. - - - No playable source available for this video. - - - )} - - ) -} + ) : sources.length > 0 ? ( + { + vjsPlayerRef.current = player + }} + /> + ) : thumbnailUrl ? ( + + {videoThumbnailAlt} + + ) : ( + <> + + No playable source available for this video. + + + No playable source available for this video. + + + )} + + ) + }, +) +VideoResourcePlayer.displayName = "VideoResourcePlayer" export default VideoResourcePlayer diff --git a/frontends/main/src/app-pages/VideoPlaylistCollectionPage/VideoSeriesDetailPage.test.tsx b/frontends/main/src/app-pages/VideoPlaylistCollectionPage/VideoSeriesDetailPage.test.tsx index e97f6cf20d..9f466eac50 100644 --- a/frontends/main/src/app-pages/VideoPlaylistCollectionPage/VideoSeriesDetailPage.test.tsx +++ b/frontends/main/src/app-pages/VideoPlaylistCollectionPage/VideoSeriesDetailPage.test.tsx @@ -1,6 +1,6 @@ import React from "react" import { setMockResponse, urls, factories } from "api/test-utils" -import { renderWithProviders, screen } from "@/test-utils" +import { renderWithProviders, screen, user } from "@/test-utils" import VideoSeriesDetailPage from "./VideoSeriesDetailPage" import { ResourceTypeEnum } from "api/v1" import type { VideoResource, VideoPlaylistResource } from "api/v1" @@ -265,6 +265,90 @@ describe("VideoSeriesDetailPage", () => { }) }) + describe("share button in Up Next section", () => { + test("is not shown while playlist items are still loading", async () => { + const playlist = makePlaylist() + const video = makeVideo({ title: "Loading Items Test" }) + + setupVideoApi(video) + setMockResponse.get( + urls.learningResources.items({ id: playlist.id }), + new Promise(() => {}), // never resolves → items stay loading + ) + + renderWithProviders( + , + ) + + await screen.findByRole("heading", { name: video.title }) + expect( + screen.queryByRole("button", { name: /share/i }), + ).not.toBeInTheDocument() + }) + + test("share URL uses the slugged canonical form with playlist param", async () => { + const playlist = makePlaylist({ id: 99 }) + const current = makeVideo({ id: 720, title: "Intro to Machine Learning" }) + const next = makeVideo({ title: "Next Lecture" }) + renderPage({ + video: current, + playlistId: playlist.id, + playlistData: playlist, + playlistItems: [current, next], + }) + + await screen.findByText("Up Next") + await user.click( + screen.getByRole("button", { + name: /share intro to machine learning/i, + }), + ) + expect(screen.getByRole("textbox")).toHaveValue( + "http://test.learn.odl.local:8062/video/720/intro-to-machine-learning?playlist=99", + ) + }) + + test("clicking the share button opens the dialog", async () => { + const playlist = makePlaylist() + const current = makeVideo({ title: "Current Video" }) + const next = makeVideo({ title: "Next Video" }) + renderPage({ + video: current, + playlistId: playlist.id, + playlistData: playlist, + playlistItems: [current, next], + }) + + await user.click( + await screen.findByRole("button", { name: /share current video/i }), + ) + expect(screen.getByRole("dialog")).toBeInTheDocument() + }) + + test("closing the share dialog removes it from the page", async () => { + const playlist = makePlaylist() + const current = makeVideo({ title: "Close Dialog Test" }) + const next = makeVideo({ title: "Next Video" }) + renderPage({ + video: current, + playlistId: playlist.id, + playlistData: playlist, + playlistItems: [current, next], + }) + + await user.click( + await screen.findByRole("button", { name: /share close dialog test/i }), + ) + await user.click(screen.getByRole("button", { name: /^close$/i })) + expect(screen.queryByRole("dialog")).not.toBeInTheDocument() + }) + }) + describe("Up Next section", () => { test("renders Up Next with a Continue button when there is a next video", async () => { const playlist = makePlaylist() diff --git a/frontends/main/src/app-pages/VideoPlaylistCollectionPage/VideoSeriesDetailPage.tsx b/frontends/main/src/app-pages/VideoPlaylistCollectionPage/VideoSeriesDetailPage.tsx index b748acb9df..5b642bb29c 100644 --- a/frontends/main/src/app-pages/VideoPlaylistCollectionPage/VideoSeriesDetailPage.tsx +++ b/frontends/main/src/app-pages/VideoPlaylistCollectionPage/VideoSeriesDetailPage.tsx @@ -14,6 +14,7 @@ import * as Styled from "./VideoSeriesDetailPage.styled" import { env } from "@/env" import { buildVideoStructuredData } from "./videoStructuredData" import VideoResourcePlayer from "./VideoResourcePlayer" +import type { VideoPlayerHandle } from "./VideoResourcePlayer" const NEXT_PUBLIC_ORIGIN = env("NEXT_PUBLIC_ORIGIN") @@ -35,6 +36,7 @@ const VideoSeriesDetailPage: React.FC = ({ playlistLoading, }) => { const titleRef = useRef(null) + const playerRef = useRef(null) const { data: resource, isLoading: videoLoading } = useLearningResourcesDetail(videoId) @@ -171,6 +173,7 @@ const VideoSeriesDetailPage: React.FC = ({ )} {/* Video player */} = ({ nextVideo={nextVideo} getVideoHref={getVideoHref} currentVideo={video} + playerRef={playerRef} shareUrl={`${NEXT_PUBLIC_ORIGIN}${videoDetailPageView(video.id, playlistId ?? undefined, video.title)}`} /> )} diff --git a/frontends/main/src/app-pages/VideoPlaylistCollectionPage/VideoShareButton.test.tsx b/frontends/main/src/app-pages/VideoPlaylistCollectionPage/VideoShareButton.test.tsx new file mode 100644 index 0000000000..04b28d0de5 --- /dev/null +++ b/frontends/main/src/app-pages/VideoPlaylistCollectionPage/VideoShareButton.test.tsx @@ -0,0 +1,112 @@ +import React from "react" +import { renderWithProviders, screen, user } from "@/test-utils" +import { factories } from "api/test-utils" +import { ResourceTypeEnum } from "api/v1" +import type { VideoResource } from "api/v1" +import VideoShareButton from "./VideoShareButton" +import type { VideoPlayerHandle } from "./VideoResourcePlayer" + +const makeVideo = (overrides: Partial = {}): VideoResource => + factories.learningResources.video({ + resource_type: ResourceTypeEnum.Video, + ...overrides, + }) as VideoResource + +const PAGE_URL = "https://learn.mit.edu/video/42?playlist=1" + +type RenderOptions = { + video?: VideoResource + title?: string + pageUrl?: string + playerRef?: React.RefObject +} + +const renderButton = ({ + video = makeVideo(), + title = video.title ?? "Test video", + pageUrl = PAGE_URL, + playerRef, +}: RenderOptions = {}) => + renderWithProviders( + , + ) + +describe("VideoShareButton", () => { + describe("share button", () => { + test("renders with an aria-label containing the video title", async () => { + renderButton({ title: "My Cool Video" }) + expect( + await screen.findByRole("button", { name: /share my cool video/i }), + ).toBeInTheDocument() + }) + + test("dialog is not visible before the button is clicked", async () => { + renderButton() + await screen.findByRole("button", { name: /share/i }) + expect(screen.queryByRole("dialog")).not.toBeInTheDocument() + }) + + test("clicking the button opens the share dialog", async () => { + renderButton({ title: "Opening Test" }) + await user.click( + await screen.findByRole("button", { name: /share opening test/i }), + ) + expect(screen.getByRole("dialog")).toBeInTheDocument() + }) + + test("closing the dialog hides it", async () => { + renderButton({ title: "Close Test" }) + await user.click( + await screen.findByRole("button", { name: /share close test/i }), + ) + await user.click(screen.getByRole("button", { name: /^close$/i })) + expect(screen.queryByRole("dialog")).not.toBeInTheDocument() + }) + }) + + describe("getCurrentTime integration", () => { + test("passes 0 to ShareDialog when no playerRef is provided", async () => { + renderButton({ title: "No Ref" }) + await user.click( + await screen.findByRole("button", { name: /share no ref/i }), + ) + // With no getCurrentTime value > 0 the share URL is the bare page URL + expect(screen.getByDisplayValue(PAGE_URL)).toBeInTheDocument() + }) + + test("passes 0 to ShareDialog when playerRef.current is null", async () => { + const playerRef = React.createRef() + renderButton({ title: "Null Ref", playerRef }) + await user.click( + await screen.findByRole("button", { name: /share null ref/i }), + ) + expect(screen.getByDisplayValue(PAGE_URL)).toBeInTheDocument() + }) + + test("reads current time from playerRef and passes it to ShareDialog", async () => { + const handle: VideoPlayerHandle = { getCurrentTime: () => 90 } + const playerRef = { + current: handle, + } as React.RefObject + + renderButton({ title: "With Time", playerRef }) + await user.click( + await screen.findByRole("button", { name: /share with time/i }), + ) + // The share-tab link is always the bare page URL; the timestamp read + // from playerRef is surfaced in the Embed tab. + await user.click(await screen.findByRole("tab", { name: /embed/i })) + // 90s read from playerRef.getCurrentTime() → "Start at 1:30" + ?t=90 + expect(screen.getByText("Start at 1:30")).toBeInTheDocument() + const videoUrlInput = screen.getByRole("textbox", { + name: "Video URL", + }) as HTMLInputElement + expect(videoUrlInput.value).toContain("t=90") + }) + }) +}) diff --git a/frontends/main/src/app-pages/VideoPlaylistCollectionPage/VideoShareButton.tsx b/frontends/main/src/app-pages/VideoPlaylistCollectionPage/VideoShareButton.tsx index 3dcfab283c..f70dad6447 100644 --- a/frontends/main/src/app-pages/VideoPlaylistCollectionPage/VideoShareButton.tsx +++ b/frontends/main/src/app-pages/VideoPlaylistCollectionPage/VideoShareButton.tsx @@ -4,12 +4,14 @@ import React, { useState } from "react" import { RiShareForwardFill } from "@remixicon/react" import type { VideoResource } from "api/v1" import ShareDialog from "./ShareDialog" +import type { VideoPlayerHandle } from "./VideoResourcePlayer" import * as Styled from "./VideoSeriesDetailPage.styled" type VideoShareButtonProps = { video: VideoResource title: string pageUrl: string + playerRef?: React.RefObject className?: string } @@ -17,6 +19,7 @@ const VideoShareButton: React.FC = ({ video, title, pageUrl, + playerRef, className, }) => { const [shareOpen, setShareOpen] = useState(false) @@ -37,6 +40,7 @@ const VideoShareButton: React.FC = ({ title={title} onClose={() => setShareOpen(false)} pageUrl={pageUrl} + getCurrentTime={() => playerRef?.current?.getCurrentTime() ?? 0} /> ) diff --git a/frontends/main/src/app-pages/VideoPlaylistCollectionPage/YouTubeIframePlayer.tsx b/frontends/main/src/app-pages/VideoPlaylistCollectionPage/YouTubeIframePlayer.tsx index f8e77ea146..2ab88b36f6 100644 --- a/frontends/main/src/app-pages/VideoPlaylistCollectionPage/YouTubeIframePlayer.tsx +++ b/frontends/main/src/app-pages/VideoPlaylistCollectionPage/YouTubeIframePlayer.tsx @@ -1,39 +1,151 @@ "use client" import { requiredEnv } from "@/env" -import React from "react" +import React, { useEffect, useImperativeHandle, useRef } from "react" + +export type YouTubePlayerHandle = { + getCurrentTime: () => number +} export type YouTubeIframePlayerProps = { embedUrl: string title?: string ariaLabel?: string ariaDescribedBy?: string + startTime?: number +} + +interface YTPlayerInstance { + getCurrentTime(): number + destroy(): void +} + +declare global { + interface Window { + YT?: { + Player: new ( + elementOrId: HTMLElement | string, + options?: { + events?: { + onReady?: () => void + } + }, + ) => YTPlayerInstance + } + onYouTubeIframeAPIReady?: () => void + _ytApiCallbacks?: Array<() => void> + } } /** - * Renders a plain YouTube embed iframe. - * - * Used instead of the VideoJS wrapper for YouTube videos so that: - * - YouTube's native keyboard/accessibility controls work without conflict - * - Thumbnail scrubbing and other native YouTube features are preserved - * - * Accepts a base `embedUrl` (e.g. from `convertToEmbedUrl`) and appends - * the required `rel=0` and `origin` query params automatically. + * Loads the YouTube IFrame API script once and calls `onReady` when it is + * initialised (or immediately if the API is already available). */ -const YouTubeIframePlayer: React.FC = ({ - embedUrl, - title, - ariaLabel, - ariaDescribedBy, -}) => { +function ensureYTApi(onReady: () => void): void { + if (window.YT?.Player) { + onReady() + return + } + + // Queue the callback regardless of whether the script is already loading. + if (!window._ytApiCallbacks) window._ytApiCallbacks = [] + window._ytApiCallbacks.push(onReady) + + // Chain into any previously registered onYouTubeIframeAPIReady callback so + // other components' players still initialise correctly. + if (!document.querySelector('script[src*="youtube.com/iframe_api"]')) { + const prev = window.onYouTubeIframeAPIReady + window.onYouTubeIframeAPIReady = () => { + prev?.() + window._ytApiCallbacks?.forEach((fn) => fn()) + window._ytApiCallbacks = [] + } + const s = document.createElement("script") + s.src = "https://www.youtube.com/iframe_api" + document.head.appendChild(s) + } +} + +const YouTubeIframePlayer = React.forwardRef< + YouTubePlayerHandle, + YouTubeIframePlayerProps +>(({ embedUrl, title, ariaLabel, ariaDescribedBy, startTime }, ref) => { + const iframeRef = useRef(null) + const playerRef = useRef(null) + + useImperativeHandle(ref, () => ({ + // Before onReady the postMessage bridge isn't established so + // getCurrentTime() can return undefined instead of a number. + // Guard with a typeof+isFinite check so we always return a valid number. + getCurrentTime: () => { + if (typeof playerRef.current?.getCurrentTime !== "function") return 0 + try { + const t = playerRef.current.getCurrentTime() + return typeof t === "number" && isFinite(t) && t >= 0 ? t : 0 + } catch { + return 0 + } + }, + })) + + useEffect(() => { + let destroyed = false + const iframe = iframeRef.current + if (!iframe) return + + const createPlayer = () => { + // Guard against double-creation (load event + fallback timer both firing). + if (destroyed || playerRef.current) return + ensureYTApi(() => { + if (destroyed || playerRef.current || !iframeRef.current) return + if (!window.YT?.Player) return + // Pass the element reference directly rather than an id string so + // YT.Player tracks this specific element; an id string would cause + // document.getElementById lookup at cleanup time and could match a + // newly-mounted iframe sharing the same position-based id. + playerRef.current = new window.YT.Player(iframeRef.current, {}) + }) + } + + // Wrap the iframe with YT.Player only AFTER its embedded player has loaded. + // Wrapping a not-yet-loaded iframe leaves the postMessage bridge — and so + // getCurrentTime() — permanently disconnected. On the first page load the + // YT API script fetch naturally delays us past the iframe load, but on + // client-side navigation window.YT already exists, so without this guard we + // would wrap the brand-new iframe synchronously, before its content loads. + iframe.addEventListener("load", createPlayer) + // Fallback for an iframe that already finished loading before this effect + // attached the listener (e.g. a server-rendered iframe after hydration), + // where the load event will not fire again. No-op once createPlayer's guard + // wins via the load event on the navigation path. + const fallbackTimer = setTimeout(createPlayer, 1500) + + return () => { + destroyed = true + clearTimeout(fallbackTimer) + iframe.removeEventListener("load", createPlayer) + // Do NOT call playerRef.current?.destroy() here. + // YT.Player.destroy() removes the iframe element from the DOM. In React 18 + // Strict Mode (and during key-based remounts), this fires before the next + // effect runs, leaving iframeRef.current pointing to a detached element so + // the new YT.Player call silently fails. React owns the iframe's DOM + // lifecycle — when it removes the element, the browser terminates the + // embedded player automatically. + playerRef.current = null + } + }, []) + const url = new URL(embedUrl) url.searchParams.set("rel", "0") url.searchParams.set("origin", requiredEnv("NEXT_PUBLIC_ORIGIN")) - + url.searchParams.set("enablejsapi", "1") + if (startTime && startTime > 0) { + url.searchParams.set("start", String(Math.floor(startTime))) + } const src = url.toString() - return (