feat: video playlist page design implementation by ahtesham-quraish · Pull Request #3135 · mitodl/mit-learn

ahtesham-quraish · 2026-03-31T11:11:21Z

What are the relevant tickets?

https://github.com/mitodl/hq/issues/10734

Description (What does it do?)

MIT Learn already has a video playlist object, but it does not exist as a meaningful user-facing thing.

Right now playlists live as backend metadata. Users mostly land on individual videos. There is no real page for the playlist itself, and no clear way to see or navigate the grouping. Even when the content is clearly meant to belong together, it shows up as isolated videos.

Screenshots (if appropriate):

Desktop:

Mobile:

How can this be tested?

if you dont have playlist data locally then in frontend env file you should add the following variable NEXT_PUBLIC_MITOL_API_BASE_URL="https://api.rc.learn.mit.edu" it will connect with RC learn backend
then we need to enable the flag which is video-playlist-page and then visit the following url http://open.odl.local:8062/playlist/6831

Additional Context

github-actions · 2026-03-31T11:11:40Z

OpenAPI Changes

27 changes: 0 error, 22 warning, 5 info

View full changelog

Unexpected changes? Ensure your branch is up-to-date with main (consider rebasing).

Copilot

Pull request overview

This PR introduces an initial user-facing “video playlist” page in the Next.js App Router, backed by new API client/query support for video playlists and gated behind a PostHog feature flag.

Changes:

Added a new /playlist/[id] route that server-prefetches playlist + item data and hydrates a new client-side playlist page.
Implemented new playlist UI components (header, featured video, collection list, modal player) including a video.js-based player with YouTube support.
Extended the API client + react-query hooks to retrieve video playlist details, and added the new video-playlist-page feature flag.

Reviewed changes

Copilot reviewed 13 out of 14 changed files in this pull request and generated 14 comments.

Show a summary per file

File	Description
yarn.lock	Locks new dependencies for video.js / YouTube tech and related transitive packages.
frontends/main/package.json	Adds `video.js` and `videojs-youtube` dependencies for the new player.
frontends/main/src/common/feature_flags.ts	Adds `VideoPlaylistPage` feature flag key.
frontends/main/src/app/playlist/[id]/page.tsx	New App Router route for playlist detail with React Query SSR hydration.
frontends/main/src/app-pages/VideoPage/VideoPage.tsx	Client playlist page: feature flag gating, queries, featured video selection, modal state.
frontends/main/src/app-pages/VideoPage/VideoPageHeader.tsx	Playlist header UI (title/description).
frontends/main/src/app-pages/VideoPage/FeaturedVideo.tsx	Featured video hero section with play interaction.
frontends/main/src/app-pages/VideoPage/VideoCollection.tsx	Renders the remaining playlist videos as a list of cards.
frontends/main/src/app-pages/VideoPage/VideoCard.tsx	Video list item card with thumbnail, duration, and play affordance.
frontends/main/src/app-pages/VideoPage/VideoPlayerModal.tsx	Modal wrapper for playback and “no playable source” fallback.
frontends/main/src/app-pages/VideoPage/VideoJsPlayer.tsx	Video.js wrapper + source resolution for HLS/DASH/MP4/YouTube.
frontends/api/src/clients.ts	Adds and exports `videoPlaylistsApi` client.
frontends/api/src/hooks/learningResources/queries.ts	Adds `videoPlaylistQueries.detail` and query keys.
frontends/api/src/hooks/learningResources/index.ts	Re-exports `videoPlaylistQueries` for consumers.

Copilot · 2026-03-31T11:54:29Z

+  await Promise.all([
+    queryClient.prefetchQuery(videoPlaylistQueries.detail(playlistId)),
+    queryClient.prefetchQuery(
+      learningResourceQueries.items(playlistId, {
+        learning_resource_id: playlistId,
+      }),
+    ),
+  ])
+


This page prefetches playlist data with prefetchQuery, but doesn't ensure a missing playlist returns a real 404 response. Consider using queryClient.fetchQueryOr404(videoPlaylistQueries.detail(playlistId)) for the playlist detail (similar to src/app/news/[slugOrId]/page.tsx) so a 404 from the API becomes a Next.js 404 page on initial load.

Suggested change

await Promise.all([

queryClient.prefetchQuery(videoPlaylistQueries.detail(playlistId)),

queryClient.prefetchQuery(

learningResourceQueries.items(playlistId, {

learning_resource_id: playlistId,

}),

),

])

// Ensure a missing playlist results in a real Next.js 404 on initial load

await queryClient.fetchQueryOr404(videoPlaylistQueries.detail(playlistId))

await queryClient.prefetchQuery(

learningResourceQueries.items(playlistId, {

learning_resource_id: playlistId,

}),

)

Not sure about it need to think more on it

this is a good idea

Copilot · 2026-03-31T11:54:29Z

+    notFound()
+  }
+  const isLoading = playlistLoading || itemsLoading
+  const videos = (items ?? []) as VideoResource[]


items is cast to VideoResource[] without checking the discriminator (e.g. resource_type). Since learningResourcesItemsList returns a generic learning resource union, this can break at runtime if a playlist contains a non-video item (or the API evolves). Prefer narrowing/filtering by resource_type === "video" (or using a video-playlist-specific endpoint/type) instead of a blanket cast.

Suggested change

const videos = (items ?? []) as VideoResource[]

const videos = (items ?? []).filter(

(item: any): item is VideoResource => item?.resource_type === "video",

)

Copilot · 2026-03-31T11:54:31Z

+  return (
+    <ModalBackdrop
+      onClick={(e) => {
+        if (e.target === e.currentTarget) onClose()
+      }}
+    >
+      <ModalContent>
+        <ModalHeader>
+          <ModalTitle>{video.title}</ModalTitle>
+          <ModalCloseButton
+            type="button"
+            aria-label="Close video"
+            onClick={onClose}
+          >
+            <RiCloseLine />
+          </ModalCloseButton>
+        </ModalHeader>
+        <PlayerWrapper>


The modal markup is missing common dialog accessibility attributes/behavior (e.g., role="dialog", aria-modal="true", and an aria-labelledby pointing at the title). Also consider handling Escape to close and trapping/restoring focus while the modal is open to keep keyboard navigation usable.

Copilot · 2026-03-31T11:54:31Z

    "sharp": "0.34.4",
    "slick-carousel": "^1.8.1",
    "tiny-invariant": "^1.3.3",
+    "video.js": "8.23.7",


Pinning video.js to 8.23.7 causes two versions to be installed (8.23.7 and 8.23.8 via videojs-youtube), increasing bundle size. Consider aligning on a single version (e.g., bump to 8.23.8 or use a compatible range) and/or adding a Yarn resolutions entry to dedupe.

Suggested change

"video.js": "8.23.7",

"video.js": "^8.23.8",

Copilot · 2026-03-31T11:54:32Z

+const VideoPage: React.FC<VideoPageProps> = ({ playlistId }) => {
+  const [activeVideo, setActiveVideo] = useState<VideoResource | null>(null)
+
+  const openVideo = useCallback((resource: VideoResource) => {
+    setActiveVideo(resource)
+  }, [])
+
+  const closeVideo = useCallback(() => {
+    setActiveVideo(null)
+  }, [])


This new page introduces several behaviors that would benefit from automated tests (feature-flag gating without 404 flicker, rendering playlist header/content based on fetched data, and opening/closing the player modal). There are existing page/component tests under src/app-pages/ProductPages/*.test.tsx; adding similar tests for VideoPage would help prevent regressions.

Copilot · 2026-03-31T11:54:32Z

+import VideoJsPlayer, { resolveVideoSources } from "./VideoJsPlayer"
+


video.js is a fairly large dependency; importing VideoJsPlayer eagerly means the playlist page bundle will include video.js even if the user never opens the modal. Consider lazy-loading the player with next/dynamic (or React.lazy) when the modal opens to keep initial JS/CSS payload smaller (similar to dynamic imports in app-pages/ChannelPage/ChannelPage.tsx).

Suggested change

import VideoJsPlayer, { resolveVideoSources } from "./VideoJsPlayer"

import dynamic from "next/dynamic"

import { resolveVideoSources } from "./VideoJsPlayer"

const VideoJsPlayer = dynamic(() => import("./VideoJsPlayer"), {

ssr: false,

loading: () => null,

})

zamanafzal

I've left a few comments and the UI doesn't look good on two screens.

Here are the screenshots of the screens.

zamanafzal · 2026-04-08T11:24:41Z

+            variant="light"
+            ancestors={[
+              { href: "/", label: "Home" },
+              { href: "/videos", label: "Videos" },


This breadcrumb points to /videos, but there is no matching route in the app router. Clicking "Videos" leads to a dead page. Can we update this to a valid destination (or remove it) to avoid broken navigation?

@zamanafzal Ferdi has provided me the design in which video is part of breadcrumb. I already asked him about it but he did not reply me let me ask him again where it should point to.

zamanafzal · 2026-04-08T11:57:48Z

+        <Container>
+          <StyledBreadcrumbs
+            variant="light"
+            ancestors={[{ href: `/playlist/${playlistId}`, label: "Playlist" }]}


If playlistId is missing from search params, this renders /playlist/null in the breadcrumb link. Repro: open /playlist/detail/ without ?playlist=. Suggest guarding this link when playlistId is null or falling back to a safe route.

Currently I have removed the detail page from this PR and moved it here #3173 so when we click on the video then for now we are showing 404 page which obviously will available in new PR

zamanafzal · 2026-04-08T12:11:34Z

+            <VideoTitle>{video?.title}</VideoTitle>
+          )}
+
+          {/* Description */}


nit: I think we should remove these comments.

I have removed the videoDetailPage which will be available in new PR #3173

zamanafzal · 2026-04-08T12:13:23Z

+  },
+  [theme.breakpoints.down("sm")]: {
+    padding: "16px 0 0",
+    letteSpacing: "inherit",


typo: letteSpacing should be letterSpacing.

zamanafzal · 2026-04-08T12:15:11Z

+    <Section>
+      <VideoContainer>
+        <FeaturedGrid>
+          <ImageWrapper onClick={() => onPlay(video)}>


Could we switch to semantic button/link behaviour (or add role/tabIndex + Enter/Space handlers) for accessibility parity?

Semantic elements would be great!

zamanafzal · 2026-04-08T12:15:24Z

+          </ImageWrapper>
+
+          <TextSide>
+            <FeaturedTitle onClick={() => onPlay(video)}>


Same as above.

zamanafzal · 2026-04-08T12:46:42Z

+import videojs from "video.js"
+import Player from "video.js/dist/types/player"
+import "video.js/dist/video-js.css"
+// Register YouTube tech so video.js can play youtube:// sources


I think this comment is a bit misleading. Code passes normal YouTube URLs (e.g. https://youtube.com/...), not necessarily youtube://

I have removed this comment

zamanafzal · 2026-04-08T12:47:59Z

+// Helper: extract instructor info
+// ---------------------------------------------------------------------------
+
+function getInstructorInfo(video: VideoResource): {


How about renaming it to getPrimaryInstructorInfo for better clarity?

I have removed the videoDetailPage from this PR

zamanafzal · 2026-04-08T12:51:02Z

+}
+
+const FeaturedVideo: React.FC<FeaturedVideoProps> = ({ videos, onPlay }) => {
+  const [currentIndex] = useState(0)


nit: currentIndex is always 0, so useState here adds cognitive overhead. Consider replacing with const video = videos[0] for clearer intent.

Request: similar to Zaman's comment, change the videos prop to video. We are only using one. No need to pass the whole array. Confusing for "FeaturedVideo" to take an array, IMO.

ChristopherChudzicki

This will be a great feature. I left some comments on naming, existing utility funcs that might help, queries, and questions about files that seem to be unused.

ChristopherChudzicki · 2026-04-08T20:01:00Z

+  const { data: playlistData, isLoading: playlistLoading } = useQuery(
+    videoPlaylistQueries.detail(playlistId),
+  )
+  const playlist = playlistData as VideoPlaylistResource | undefined


Request: Let's remove this. The type is already VideoPlaylistResource | undefined, the cast does nothing.

ChristopherChudzicki · 2026-04-08T20:02:58Z

+  const { data: similarData, isLoading: similarLoading } = useQuery(
+    learningResourceQueries.vectorSimilar(playlistId),
+  )


I noticed that:

below you did otherCollections.slice(0,6)

and there is filtering on the results of vectorSimilar

This has the downside that you're getting an unpredictable number of results. (E.g., if vectorSimilar returns mostly courses or podcasts or whatever, they are filtered out by your filter)

It would be better to let the APIs handle the trimming and filtering. You can do that by changing the vectorSimilar query...for some reason right now it only supports a single id param, but the API supports much more.

Suggest changing to:

--- a/frontends/api/src/hooks/learningResources/queries.ts +++ b/frontends/api/src/hooks/learningResources/queries.ts @@ -20,6 +20,7 @@ import type { LearningResourcesApiLearningResourcesItemsListRequest as ItemsListRequest, LearningResourcesApiLearningResourcesSummaryListRequest as LearningResourcesSummaryListRequest, VideoPlaylistResource, + LearningResourcesApiLearningResourcesVectorSimilarListRequest, } from "../../generated/v1" import type { VectorLearningResourcesSearchApiVectorLearningResourcesSearchRetrieveRequest as VectorLearningResourcesSearchRetrieveRequest } from "../../generated/v0" import { queryOptions } from "@tanstack/react-query" @@ -42,10 +43,9 @@ const learningResourceKeys = { detailsRoot: () => [...learningResourceKeys.root, "detail"], detail: (id: number) => [...learningResourceKeys.detailsRoot(), id], similar: (id: number) => [...learningResourceKeys.detail(id), "similar"], - vectorSimilar: (id: number) => [ - ...learningResourceKeys.detail(id), - "vector_similar", - ], + vectorSimilar: ( + params: LearningResourcesApiLearningResourcesVectorSimilarListRequest, + ) => [...learningResourceKeys.detail(params.id), "vector_similar", params], itemsRoot: (id: number) => [...learningResourceKeys.detail(id), "items"], items: (id: number, params: ItemsListRequest) => [ ...learningResourceKeys.itemsRoot(id), @@ -128,12 +128,14 @@ const learningResourceQueries = { .learningResourcesSimilarList({ id }) .then((res) => res.data), }), - vectorSimilar: (id: number) => + vectorSimilar: ( + params: LearningResourcesApiLearningResourcesVectorSimilarListRequest, + ) => queryOptions({ - queryKey: learningResourceKeys.vectorSimilar(id), + queryKey: learningResourceKeys.vectorSimilar(params), queryFn: () => learningResourcesApi - .learningResourcesVectorSimilarList({ id }) + .learningResourcesVectorSimilarList(params) .then((res) => res.data), }), list: (params: LearningResourcesListRequest) =>

then using

const { data: similarData, isLoading: similarLoading } = useQuery( learningResourceQueries.vectorSimilar({ id: playlistId, resource_type: [ResourceTypeEnum.VideoPlaylist], limit: 6, }), )

to get exactly the data you want.

Can you please check I have changed the implementation and let me know in case i have done wrong

Good: Well, these changes look great. Exactly what I was hoping for.

Disappointing: Thank you for making this change. Unfortunately, it seems that the OpenAPI spec is wrong, and the resource_type filter doesn't actually work. I've opened https://github.com/mitodl/hq/issues/10896 about this.

My recommendation for now: would be to leave the change as-is for the moment BUT restore a resource_type="video_playlist" filter to prevent crashing. (⚠️ ⚠️ it's currently crashing for some playlists for me, like playlist 6382 from prod):

const { data: similarData, isLoading: similarLoading } = useQuery({ ...learningResourceQueries.vectorSimilar({ id: playlistId, resource_type: [ResourceTypeEnum.VideoPlaylist], limit: 6, }), select: (resources) => resources.filter( (r) => r.resource_type === ResourceTypeEnum.VideoPlaylist, ), })

A selector like this wouldn't be bad to leave in even once the backend filtering is in place. It should be a no-op, but would also give you the correct TS type without typecasting.

Then we can coordinate on Monday about supporting resource_type filters on the similarity endpoint.

ChristopherChudzicki · 2026-04-08T20:23:05Z

+}
+
+const FeaturedVideo: React.FC<FeaturedVideoProps> = ({ videos, onPlay }) => {
+  const [currentIndex] = useState(0)


Request: similar to Zaman's comment, change the videos prop to video. We are only using one. No need to pass the whole array. Confusing for "FeaturedVideo" to take an array, IMO.

ChristopherChudzicki · 2026-04-08T20:26:28Z

+            <PageDescription>
+              {playlist?.description ??
+                "Conversations with MIT faculty on the future of science, technology, and society."}
+            </PageDescription>


is this resolved? I still see the hard-coded description. (Maybe forgot a push?)

ChristopherChudzicki · 2026-04-08T20:35:21Z

+
+  const otherCollections = getResults(similarData).filter(
+    (resource): resource is VideoPlaylistResource =>
+      resource.resource_type === ResourceTypeEnum.VideoPlaylist &&


See https://github.com/mitodl/mit-learn/pull/3135/changes#r3053866744 ... would be better to filter at API level.

ChristopherChudzicki · 2026-04-08T21:40:38Z

+  minHeight: "100vh",
+}))
+
+const getResults = (


The only current usage of getResults is getResults(similarData), and similarData is already a plain array, LearningResource[]. I'd suggest we remove this for now.

I have updated the code can you please check

ChristopherChudzicki · 2026-04-08T22:10:53Z

+  const truncatedTitle =
+    video.title.length > FEATURED_TITLE_MAX_CHARS
+      ? `${video.title.slice(0, FEATURED_TITLE_MAX_CHARS).trimEnd()}...`
+      : video.title


It would be better to use CSS line clamping (you do that elsewhere in this PR). We actually have a helper defined for this,

mit-learn/frontends/ol-components/src/components/TruncateText/TruncateText.tsx

Line 11 in a4e5633

const truncateText = (lines?: number | "none") =>

Ferdi wants to have this title truncate on the basis of characters, the approach you are talking is based on lines that is why I have created this function to show only 30 characters.

See

OK. We really shouldn't do it this way:

CSS is more robust visually, see screenshot below ... awkward partially filled line.

True truncation (not CSS based) has accessibility implications, e.g., screenreaders now get incomplete headings.

I posted something in the slack thread about it.

Note
It also seems to me that slack thread is mostly about the description not the title?

ChristopherChudzicki · 2026-04-08T22:11:29Z

+    <Section>
+      <VideoContainer>
+        <FeaturedGrid>
+          <ImageWrapper onClick={() => onPlay(video)}>


Semantic elements would be great!

ChristopherChudzicki · 2026-04-08T22:37:14Z

+  lineHeight: "16px" /* 133.333% */,
+})
+
+const parseDurationToHoursAndMinutes = (duration?: string): string | null => {


We have a utility function formatDurationClockTime that is used to do this elsewhere.

Could we use that instead?

I have created new util which will display the date time in human form like in the design

ChristopherChudzicki · 2026-04-08T22:41:46Z

+const OtherCollections: React.FC<OtherCollectionsProps> = ({
+  collections,
+  isLoading,
+}) => {


Looking at the Figma, I'm guessing the component naming was chosen to match Figma. In theory I think that is a good idea, but it kind of just moves the burden of picking good names from us to Bilal. And naming is hard.

Looking at the two playlist pages (Series and Collection) I see they both use this component, and this component displays both types of playlists:

IMO, "RelatedPlaylist" would be a better name for this component. cc @mbilalmughal in case you want to update yours. (Doesn't need to be that name in particular, but it seems this isn't particular to collections).

zamanafzal

I've left some minor suggestions.

zamanafzal · 2026-04-10T11:05:24Z

+  const videoRef = useRef<HTMLDivElement>(null)
+  const playerRef = useRef<Player | null>(null)
+
+  useEffect(() => {


The useEffect has unstable dependencies that could cause the video player to be recreated unnecessarily, leading to memory leaks and performance issues.

Suggestion:

useEffect(() => { // Only initialise once if (playerRef.current) return const videoEl = document.createElement("video-js") videoEl.classList.add("vjs-big-play-centered") videoEl.style.width = "100%" videoEl.style.height = "100%" videoRef.current!.appendChild(videoEl) const player = videojs( videoEl, { autoplay, controls, fluid, fill: !fluid, responsive: true, poster: poster ?? undefined, sources, techOrder: ["youtube", "html5"], }, function (this: Player) { onReady?.(this) }, ) playerRef.current = player }, []) // Remove all dependencies to prevent re-initialization // Handle updates in separate effects useEffect(() => { const player = playerRef.current if (!player) return player.src(sources) }, [sources]) useEffect(() => { const player = playerRef.current if (!player || !poster) return player.poster(poster) }, [poster])

zamanafzal · 2026-04-10T11:08:02Z

+    : null
+  const description = video.description ?? ""
+  const FEATURED_TITLE_MAX_CHARS = 30
+  const truncatedTitle =


The title truncation is calculated on every render. How about memoized this for better performance?

const FEATURED_TITLE_MAX_CHARS = 30 const truncatedTitle = useMemo(() => video.title.length > FEATURED_TITLE_MAX_CHARS ? `${video.title.slice(0, FEATURED_TITLE_MAX_CHARS).trimEnd()}...` : video.title , [video.title])

zamanafzal · 2026-04-10T11:21:57Z

+  )
+  const flagsLoaded = useFeatureFlagsLoaded()
+
+  const { data: playlist, isLoading: playlistLoading } = useQuery(


I think we need to add error handling for API failures.

Suggestion:

const { data: playlist, isLoading: playlistLoading, error: playlistError } = useQuery( videoPlaylistQueries.detail(playlistId), ) const { data: items, isLoading: itemsLoading, error: itemsError } = useQuery( learningResourceQueries.items(playlistId, { learning_resource_id: playlistId, }), ) if (playlistError || itemsError) { return ( <Page> <VideoContainer> <Typography variant="h4">Failed to load playlist</Typography> <Typography>Please try refreshing the page.</Typography> </VideoContainer> </Page> ) }

@ChristopherChudzicki what do you think about it?

Probably best to handle this on the nextjs server for proper 404s, etc.

https://github.com/mitodl/mit-learn/pull/3135/changes#r3015405136

zamanafzal · 2026-04-10T12:39:39Z

@ahtesham-quraish On a 1024 screen size, the content(Title, BreadCrumbs) is not properly responsive.

ChristopherChudzicki

Changes look good.. Added a few comemnts. Two things I'd highlight especially:

Some files seem unused... VideoPlayerModal, videoSources, VideoJsPlayer. Let's remove these until they are actually used. (You could keep them on a different branch if you want to save them.)
See https://github.com/mitodl/mit-learn/pull/3135/changes#r3066507911 ... important comment there, unfortunately. Page is crashing right now sometimes 😦

ChristopherChudzicki · 2026-04-08T23:00:09Z

@ahtesham-quraish This is a video playlist collection page, right? What do you think about a url like /video-collections/:id ? Ideally we'd use a slug, but I don't think we have the data in place for that.

@ChristopherChudzicki i dont mind to change it but i think it should be more general because we will have video series as well

@ahtesham-quraish But those will be a different URL, right?

From the issue:

This distinction carries through to the video pages.

On a series video page... On a collection video page...

...Do not collapse this into one template just because it is easier to implement. Components can be shared. Behavior should not.

ChristopherChudzicki · 2026-04-10T20:05:32Z

+  const { data: similarData, isLoading: similarLoading } = useQuery(
+    learningResourceQueries.vectorSimilar(playlistId),
+  )


Good: Well, these changes look great. Exactly what I was hoping for.

Disappointing: Thank you for making this change. Unfortunately, it seems that the OpenAPI spec is wrong, and the resource_type filter doesn't actually work. I've opened https://github.com/mitodl/hq/issues/10896 about this.

My recommendation for now: would be to leave the change as-is for the moment BUT restore a resource_type="video_playlist" filter to prevent crashing. (⚠️ ⚠️ it's currently crashing for some playlists for me, like playlist 6382 from prod):

const { data: similarData, isLoading: similarLoading } = useQuery({ ...learningResourceQueries.vectorSimilar({ id: playlistId, resource_type: [ResourceTypeEnum.VideoPlaylist], limit: 6, }), select: (resources) => resources.filter( (r) => r.resource_type === ResourceTypeEnum.VideoPlaylist, ), })

A selector like this wouldn't be bad to leave in even once the backend filtering is in place. It should be a no-op, but would also give you the correct TS type without typecasting.

Then we can coordinate on Monday about supporting resource_type filters on the similarity endpoint.

ChristopherChudzicki · 2026-04-10T21:23:43Z

+        : video?.title,
+    [video?.title],
+  )
+  if (!video) return null


This prop is required, so no need for this early exit.

ChristopherChudzicki · 2026-04-10T21:38:13Z

+  const truncatedTitle =
+    video.title.length > FEATURED_TITLE_MAX_CHARS
+      ? `${video.title.slice(0, FEATURED_TITLE_MAX_CHARS).trimEnd()}...`
+      : video.title


See

OK. We really shouldn't do it this way:

CSS is more robust visually, see screenshot below ... awkward partially filled line.

True truncation (not CSS based) has accessibility implications, e.g., screenreaders now get incomplete headings.

I posted something in the slack thread about it.

Note
It also seems to me that slack thread is mostly about the description not the title?

ChristopherChudzicki

Changes generally look good but I'm still seeing it crash. See https://github.com/mitodl/mit-learn/pull/3135/changes#r3073950679

In addition to that, I think a collection-specific URL would be good, as would the 404 handling.

ChristopherChudzicki · 2026-04-13T14:58:08Z

@ahtesham-quraish But those will be a different URL, right?

From the issue:

This distinction carries through to the video pages.

On a series video page... On a collection video page...

...Do not collapse this into one template just because it is easier to implement. Components can be shared. Behavior should not.

ChristopherChudzicki · 2026-04-13T15:00:00Z

+  )
+  const flagsLoaded = useFeatureFlagsLoaded()
+
+  const { data: playlist, isLoading: playlistLoading } = useQuery(


Probably best to handle this on the nextjs server for proper 404s, etc.

https://github.com/mitodl/mit-learn/pull/3135/changes#r3015405136

ChristopherChudzicki · 2026-04-13T15:09:35Z

+  isLoading: boolean
+}
+
+const RelatedPlaylist: React.FC<OtherCollectionsProps> = ({


Should make OtherCollectionsProps match the component name.

ChristopherChudzicki · 2026-04-13T15:11:57Z

+  const otherCollections = ((similarData ?? []) as VideoPlaylistResource[])
+    .filter((resource) => resource.id !== playlistId)
+    .slice(0, 6)


Currently this list can include things other than video playlists, and that causes the page to crash.

For example, using production base URL, it crashes for pretty consistently for http://open.odl.local:8062/playlist/6382

I'd suggest doing the resource_type filtering at the query call, as mentioned in https://github.com/mitodl/mit-learn/pull/3135/changes#r3066507911

That will make sure everywhere is getting playlists only.

Suggest doing the filtering when you make the query:

const { data: similarData, isLoading: similarLoading } = useQuery({ ...learningResourceQueries.vectorSimilar(playlistId), select: (data) => data.filter( (resource) => resource.resource_type === ResourceTypeEnum.VideoPlaylist, ), })

This:

Filters on the frontend—we should filter on backend once supported

Refines the type.

(1) will be irrelevant once we improve the backend, (2) is still useful, though

ChristopherChudzicki · 2026-04-13T16:28:45Z

+  if (isError) {
+    return notFound()
+  }


This can be addressed in a followup PR, but this is best to do on the server side (See copilot's suggestion, #3135 (comment)).

ChristopherChudzicki

👍 one suggestion for followup PR 62d2fdf#r3074389926

zamanafzal

LGTM

sentry · 2026-04-13T16:59:49Z

+  const getVideoHref = (resource: VideoResource) =>
+    `/playlist/detail/${resource.id}?playlist=${playlistId}`


Bug: Links to video detail pages point to a non-existent /playlist/detail/[id] route, which will cause a 404 error when users click them.
_{Severity: HIGH}

Suggested Fix

Update the link generation logic to use the correct URL pattern for video detail pages. The investigation did not identify the correct route, but it should be changed from /playlist/detail/[id] to whichever route is configured to display individual video details.

Prompt for AI Agent

Review the code at the location below. A potential bug has been identified by an AI agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not valid. Location: frontends/main/src/app-pages/VideoPlaylistCollectionPage/VideoPlaylistCollectionPage.tsx#L33-L34 Potential issue: The code in `VideoPlaylistCollectionPage.tsx` generates links for featured and collection videos that point to the URL pattern `/playlist/detail/[id]`. However, the application does not have a corresponding route handler for this path. There is no `page.tsx` file under `app/playlist/detail/[id]` and no rewrites in `next.config.js` to handle this URL. As a result, when a user clicks on a video from the new video playlist page, they will be directed to a non-existent page, resulting in a 404 error.

ahtesham-quraish force-pushed the ahtesham/video-page-design branch from 3bdf681 to 1905541 Compare March 31, 2026 11:15

feat: playlist page video design implementation

87b7578

ahtesham-quraish force-pushed the ahtesham/video-page-design branch 2 times, most recently from 8649d77 to 39e00af Compare March 31, 2026 11:43

ahtesham-quraish marked this pull request as ready for review March 31, 2026 11:44

Copilot AI review requested due to automatic review settings March 31, 2026 11:44

ahtesham-quraish marked this pull request as draft March 31, 2026 11:44

Copilot started reviewing on behalf of ahtesham-quraish March 31, 2026 11:45 View session

sentry bot reviewed Mar 31, 2026

View reviewed changes

Comment thread frontends/main/src/app-pages/VideoPage/VideoPage.tsx

Copilot AI reviewed Mar 31, 2026

View reviewed changes

ahtesham-quraish marked this pull request as ready for review March 31, 2026 12:40

ahtesham-quraish added the Needs Review An open Pull Request that is ready for review label Mar 31, 2026

ahtesham-quraish changed the title ~~WIP~~ feat: video playlist page design implementation Mar 31, 2026

ahtesham-quraish force-pushed the ahtesham/video-page-design branch from 39e00af to e2114b3 Compare April 1, 2026 08:32

sentry bot reviewed Apr 1, 2026

View reviewed changes

Comment thread frontends/main/src/app-pages/VideoPage/FeaturedVideo.tsx Outdated

ahtesham-quraish force-pushed the ahtesham/video-page-design branch from e2114b3 to f2d37ca Compare April 1, 2026 11:09

sentry bot reviewed Apr 1, 2026

View reviewed changes

Comment thread frontends/main/src/app-pages/VideoPage/VideoDetailPage.tsx Outdated

ahtesham-quraish force-pushed the ahtesham/video-page-design branch from f2d37ca to d25941c Compare April 1, 2026 11:13

sentry bot reviewed Apr 1, 2026

View reviewed changes

Comment thread frontends/main/src/app-pages/VideoPage/VideoDetailPage.tsx Outdated

Merge branch 'main' into ahtesham/video-page-design

a6282ce

ahtesham-quraish force-pushed the ahtesham/video-page-design branch from d25941c to a6282ce Compare April 1, 2026 11:27

address the final design change

bc652c1

sentry bot reviewed Apr 6, 2026

View reviewed changes

Comment thread frontends/main/src/app-pages/VideoPage/VideoPage.tsx Outdated

rebase with main

5194a58

ahtesham-quraish force-pushed the ahtesham/video-page-design branch from 2ff7f4c to 5194a58 Compare April 6, 2026 12:47

sentry bot reviewed Apr 6, 2026

View reviewed changes

Comment thread frontends/main/src/app-pages/VideoPage/VideoDetailPage.tsx Outdated

final desktop design

3308227

ahtesham-quraish removed the Needs Review An open Pull Request that is ready for review label Apr 7, 2026

implement the design for mobile

86f834f

ahtesham-quraish force-pushed the ahtesham/video-page-design branch from 4cc560c to f4d366b Compare April 8, 2026 11:51

zamanafzal requested changes Apr 8, 2026

View reviewed changes

ChristopherChudzicki requested changes Apr 8, 2026

View reviewed changes

arslanashraf7 assigned ahtesham-quraish Apr 9, 2026

address the review

f851da2

ahtesham-quraish force-pushed the ahtesham/video-page-design branch from cb62412 to f851da2 Compare April 9, 2026 09:57

ahtesham-quraish requested review from ChristopherChudzicki and zamanafzal April 9, 2026 10:10

ahtesham-quraish added Needs Review An open Pull Request that is ready for review labels Apr 9, 2026

zamanafzal requested changes Apr 10, 2026

View reviewed changes

address feedback

ad210f9

ahtesham-quraish force-pushed the ahtesham/video-page-design branch from e122c02 to ad210f9 Compare April 10, 2026 12:34

sentry bot reviewed Apr 10, 2026

View reviewed changes

Comment thread frontends/main/src/app-pages/VideoPlaylistCollectionPage/VideoPlaylistCollectionPage.tsx Outdated

ChristopherChudzicki requested changes Apr 10, 2026

View reviewed changes

address another review

3e6c20b

ChristopherChudzicki requested changes Apr 13, 2026

View reviewed changes

ChristopherChudzicki reviewed Apr 13, 2026

View reviewed changes

ChristopherChudzicki approved these changes Apr 13, 2026

View reviewed changes

ahtesham-quraish force-pushed the ahtesham/video-page-design branch from 62d2fdf to 7f2c916 Compare April 13, 2026 16:32

ahtesham-quraish requested a review from zamanafzal April 13, 2026 16:47

zamanafzal approved these changes Apr 13, 2026

View reviewed changes

address review

8e0dcf3

ahtesham-quraish force-pushed the ahtesham/video-page-design branch from 7f2c916 to 8e0dcf3 Compare April 13, 2026 16:56

sentry bot reviewed Apr 13, 2026

View reviewed changes

ahtesham-quraish merged commit e9cd8b5 into main Apr 13, 2026
14 checks passed

ahtesham-quraish deleted the ahtesham/video-page-design branch April 13, 2026 17:05

This was referenced Apr 13, 2026

Release 0.63.0 #3191

Closed

Release 0.63.1 #3196

Merged

-  const videos = (items ?? []) as VideoResource[]
+  const videos = (items ?? []).filter(
+    (item: any): item is VideoResource => item?.resource_type === "video",
+  )

		import VideoJsPlayer, { resolveVideoSources } from "./VideoJsPlayer"

-import VideoJsPlayer, { resolveVideoSources } from "./VideoJsPlayer"
+import dynamic from "next/dynamic"
+import { resolveVideoSources } from "./VideoJsPlayer"
+const VideoJsPlayer = dynamic(() => import("./VideoJsPlayer"), {
+  ssr: false,
+  loading: () => null,
+})

		const getVideoHref = (resource: VideoResource) =>
		`/playlist/detail/${resource.id}?playlist=${playlistId}`

Conversation

ahtesham-quraish commented Mar 31, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What are the relevant tickets?

Description (What does it do?)

Screenshots (if appropriate):

How can this be tested?

Additional Context

Uh oh!

github-actions bot commented Mar 31, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

OpenAPI Changes

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Copilot AI Mar 31, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Copilot AI Mar 31, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI Mar 31, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 31, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 31, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 31, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

zamanafzal left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ahtesham-quraish Apr 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

ahtesham-quraish commented Mar 31, 2026 •

edited

Loading

github-actions bot commented Mar 31, 2026 •

edited

Loading

ahtesham-quraish Apr 9, 2026 •

edited

Loading