fix: add support for mp4 videos by ahtesham-quraish · Pull Request #2968 · mitodl/mit-learn

ahtesham-quraish · 2026-02-23T09:58:31Z

What are the relevant tickets?

https://mitodl.slack.com/archives/C0A16A1K1PE/p1771620781758439

Description (What does it do?)

We need to provide support for embedding the mp4 videos like we do for youtube and vimeo.

Screenshots (if appropriate):

How can this be tested?

For testing you need to paste the following url https://learn.mit.edu/media/shorts/1a8026f6e4cfefbf/1a8026f6e4cfefbf.mp4
It should embed the video tag.

Additional Context

ChristopherChudzicki

I could upload videos but noticed a few issues.

ChristopherChudzicki · 2026-02-23T17:02:04Z

frontends/main/src/page-components/TiptapEditor/extensions/node/MediaEmbed/lib.ts


+  // --- MIT LEARN MP4 VIDEOS ---
+  if (
+    hostname === "learn.mit.edu" &&


Suggestion: I think we should do VIDEO_HOST_WHITELIST since Ferdi/Matt indicated we need to support OVS videos (+their cloudflare URLs).

Request: Either switch to the whitelist (we'd need a new env var; They are added in ol-infrastructure. See, for example, https://github.com/mitodl/ol-infrastructure/pull/3977/changes) or at least use a check that isn't hard-coded, like new URL(process.env.NEXT_PUBLIC_ORIGIN).host so that this works with rc/local hosts.

Can you check now please?

ChristopherChudzicki · 2026-02-23T20:34:20Z

...ends/main/src/page-components/TiptapEditor/extensions/node/MediaEmbed/MediaEmbedNodeView.tsx

-        />
+        {isVideoUrl(src) ? (
+          <video src={src} controls title={caption}>
+            <track kind="captions" />


Here and in MediaEmbedViewer, a track without a src is useless. We should remove it.

Request: Could you also look into sharing the display between these two, unless there's a good reason not to? The display will get more complicated as we add tracks, etc

I have introduced env variable now

ChristopherChudzicki · 2026-02-24T14:22:51Z

frontends/main/src/page-components/TiptapEditor/extensions/node/MediaEmbed/MediaDisplay.tsx

+          {/* Empty track required by jsx-a11y/media-has-caption */}
+          <track kind="captions" />


Better to just suppress the linting error than having an HTML element with no function.

We will need to add proper captions eventually, though.

ChristopherChudzicki

Minor comments + we need to make a decision about https://github.com/mitodl/mit-learn/pull/2968/changes#r2848090299

ChristopherChudzicki · 2026-02-24T14:47:03Z

frontends/main/src/page-components/TiptapEditor/extensions/node/MediaEmbed/lib.ts

+          process.env.NEXT_PUBLIC_CLOUDFRONT_DOMAIN ||
+            "https://d3tsb3m56iwvoq.cloudfront.net",
+        ),
+      ).hostname.replace("www.", "") &&


A few questions:

Any reason for the www replacement? IMO it makes sense to just compare against the env var.

NEXT_PUBLIC_CLOUDFRONT_DOMAIN is a new env var, right? If adding a new env var anyway, I think it makes sense to just add the NEXT_PUBLIC_VIDEO_WHITELIST approach

ensureProtocol seems unnecessary...

NEXT_PUBLIC_ORIGIN is an origin, so it has a protocol

NEXT_PUBLIC_CLOUDFRONT_DOMAIN ... couldn't we use the origin? (That's what your fallback does) or preferable VIDEO_ORIGINS_WHITELIST

ChristopherChudzicki · 2026-02-24T16:04:21Z

frontends/main/src/page-components/TiptapEditor/extensions/node/MediaEmbed/lib.ts

+            "https://d3tsb3m56iwvoq.cloudfront.net",
+        ),
+      ).hostname.replace("www.", "") &&
+    parsed.pathname.toLowerCase().endsWith(".m3u8")


Native video element has pretty mixed support for .m3u8... This won't work on firefox as-is. To support m3u8, I believe we need something like VideoJS or hls.js, though I haven't researched it too much.

So we need to either:

Add real support for m3u8 here

or use OVS's embed endpoint with an iframe, at least for now,

Or only support mp4s coming from learn. This seems not particularly useful. Seems like main discussions now are around supporting OVS videos.

Somewhat separately: From the plugin's perspective, I don't think it makes sense for some domains to support one type of video (learn, mp4), and the other domains to support only another type (ovs cloudfront, m3u8).

Somewhat separately: From the plugin's perspective, I don't think it makes sense for some domains to support one type of video (learn, mp4), and the other domains to support only another type (ovs cloudfront, m3u8).

We need detail about what domain will support what videos or atleast we need to be sure about the total number of formats we need to support regardless of domains

We need detail about what domain will support what videos o

I don't think we need to know which domains support which video types. That's up to the domains.

We need to know which domains we trust

We need tow know which video types our tech supports

Those are separate issues, so it makes sense to separate the checks.

if (!we_trust_the_domain) {return or error} if (mp4 or m3u8) { our_custom_player} otherwise, iframe embedding

sentry · 2026-02-26T12:48:22Z

...ends/main/src/page-components/TiptapEditor/extensions/node/MediaEmbed/MediaEmbedNodeView.tsx

-          inert={editable}
-        />
-      </MediaContainer>
+      <MediaDisplay src={src} caption={caption} />


Bug: The refactored MediaDisplay component no longer applies the inert attribute, making embedded iframes interactive in editor mode, which was previously prevented.
_{Severity: MEDIUM}

Suggested Fix

Add an editable boolean prop to the MediaDisplay component. Pass the editable state from MediaEmbedNodeView.tsx to MediaDisplay. Inside MediaDisplay, conditionally apply the inert={editable} attribute to the iframe element to restore the intended non-interactive behavior during editing.

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/page-components/TiptapEditor/extensions/node/MediaEmbed/MediaEmbedNodeView.tsx#L191 Potential issue: The refactoring of media embed rendering into the `MediaDisplay` component resulted in the loss of the `inert={editable}` attribute on iframes. Previously, this attribute made embedded media non-interactive when the editor was in an editable state. Now, because the `MediaDisplay` component is not aware of the editor's `editable` status, embedded iframes (e.g., YouTube, Vimeo) are interactive during editing. This is a functional regression that allows unintended user interaction with embeds while editing content.

_{Did we get this right? 👍 / 👎 to inform future reviews.}

ChristopherChudzicki

@ahtesham-quraish Let's talk more in the channel about this. We really need transcripts and cover images for the urgent use-case, so we are looking at a different route.

ChristopherChudzicki · 2026-02-26T17:49:29Z

frontends/main/src/page-components/TiptapEditor/extensions/node/MediaEmbed/lib.ts

+            "https://d3tsb3m56iwvoq.cloudfront.net",
+        ),
+      ).hostname.replace("www.", "") &&
+    parsed.pathname.toLowerCase().endsWith(".m3u8")


We need detail about what domain will support what videos o

I don't think we need to know which domains support which video types. That's up to the domains.

We need to know which domains we trust

We need tow know which video types our tech supports

Those are separate issues, so it makes sense to separate the checks.

if (!we_trust_the_domain) {return or error} if (mp4 or m3u8) { our_custom_player} otherwise, iframe embedding

ChristopherChudzicki · 2026-02-27T01:26:31Z

frontends/main/src/page-components/TiptapEditor/extensions/node/MediaEmbed/MediaDisplay.tsx

+  ".video-js": {
+    width: "100%",
+    height: "100%",
+    borderRadius: "6px",


Pretty minor comment, but I'm not sure if any of this .video-js css is necessary/doing anything.

The height is being overridden to 0px, then videojs does some padding shenanigans to make it look good. And, perhaps related to that, the border radius isn't doing anything:

And videojs already sets width: 100%

I see the border radius being applied if you see and I am not following your point sir

ChristopherChudzicki · 2026-02-27T01:27:20Z

frontends/main/src/page-components/TiptapEditor/extensions/node/MediaEmbed/MediaDisplay.tsx

+          // eslint-disable-next-line jsx-a11y/media-has-caption
+          <video src={src} controls title={caption}>
+            Your browser does not support the video tag.
+          </video>


Can we use the same player for both mp4 and m3u8? I don't think there's any reason they need to be separate... Nice if they can be handled the same way.

I think vieojs supports using multiple playback "tech" https://videojs.org/guides/tech/

I have made the change you can check now please.

frontends/main/src/page-components/TiptapEditor/extensions/node/MediaEmbed/VideoJsPlayer.tsx

fix: add support for mp4 videos

c04240c

ahtesham-quraish force-pushed the ahtesham/mp4-branch branch from 8580acc to c04240c Compare February 23, 2026 09:59

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

ahtesham-quraish requested a review from ChristopherChudzicki February 23, 2026 13:44

ChristopherChudzicki requested changes Feb 23, 2026

View reviewed changes

add support for cloudfront

2ffae08

ChristopherChudzicki reviewed Feb 24, 2026

View reviewed changes

ChristopherChudzicki requested changes Feb 24, 2026

View reviewed changes

rebase with main

934fec8

ahtesham-quraish force-pushed the ahtesham/mp4-branch branch from 459aa85 to 934fec8 Compare February 26, 2026 12:44

sentry bot reviewed Feb 26, 2026

View reviewed changes

ahtesham-quraish force-pushed the ahtesham/mp4-branch branch from 3f23c0d to 2236825 Compare February 26, 2026 17:28

added package of video

d1d6977

ahtesham-quraish force-pushed the ahtesham/mp4-branch branch from 2236825 to d1d6977 Compare February 26, 2026 17:29

ChristopherChudzicki reviewed Feb 26, 2026

View reviewed changes

ChristopherChudzicki mentioned this pull request Feb 26, 2026

hardcode allowing to embed videos from ovs #2987

Merged

ChristopherChudzicki reviewed Feb 27, 2026

View reviewed changes

address review

7c29a91

ahtesham-quraish force-pushed the ahtesham/mp4-branch branch from 85c240f to 7c29a91 Compare February 27, 2026 11:06

sentry bot reviewed Feb 27, 2026

View reviewed changes

frontends/main/src/page-components/TiptapEditor/extensions/node/MediaEmbed/VideoJsPlayer.tsx Show resolved Hide resolved

Ahtesham Quraish added 2 commits February 27, 2026 18:00

fix cors issue

65e0f37

fix the track issue in case srt error

982f508

		{/* Empty track required by jsx-a11y/media-has-caption */}
		<track kind="captions" />

Conversation

ahtesham-quraish commented Feb 23, 2026

What are the relevant tickets?

Description (What does it do?)

Screenshots (if appropriate):

How can this be tested?

Additional Context

Uh oh!

ChristopherChudzicki left a comment

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!

ChristopherChudzicki left a comment

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!

sentry bot Feb 26, 2026

Choose a reason for hiding this comment

Uh oh!

ChristopherChudzicki left a comment

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!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants