Rumble video embed #981
Rumble video embed #981
Conversation
WalkthroughThe update introduces a refined approach for handling video links and embedding within a React application. It involves the addition of Changes
Related issues
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 2
Configuration used: CodeRabbit UI
Files selected for processing (2)
- components/text.js (3 hunks)
- middleware.js (1 hunks)
Additional comments: 2
middleware.js (1)
- 44-44: The addition of
rumble.comto theframe-srcdirective of the CSP is necessary for embedding Rumble videos. Ensure thatrumble.comis a trusted source and consider the security implications of allowing content from this source.components/text.js (1)
- 242-242: Updating the
atag handler in remarkPlugins to use the newAcomponent ensures that all links are rendered using the centralized logic. This is a good practice for maintainability and consistency.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (2)
- components/text.js (2 hunks)
- middleware.js (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- components/text.js
- middleware.js
There was a problem hiding this comment.
Thanks for the PR!
That it doesn't work for direct links is unfortunate. Did you check if there is a way to fetch the embed link from direct links? If not possible, that's okay. Maybe that's even too complicated so not worth it. I think stackers will learn fast to use embed links with rumble.
I noticed three other things though:
- The regexp is not matching for links without
startand for links wherepubcontains letters. - We also want this to work for link posts. This currently only works for links inside a post description or comment. So the rumble embed code also needs to be added to
ItemEmbedwhich you can find in components/item-full.js. That means (more) duplicate code though so if you want, you can try to refactor it. Not necessary for this PR though, I usually like create separate PRs ("stacked PRs") for stuff like this anyway.
- I think we don't need to use error-prone regexp's but can use the URL constructor. See my comment.
components/text.js
Outdated
| } | ||
|
|
||
| // if the link is to a rumble video, render the video | ||
| const rumble = href.match(/(?:https?:\/\/)?(?:www\.)?rumble\.com\/embed\/(?<id>[a-z0-9]+)\/\?pub=\d+&start=(?<start>\d+)/i) |
There was a problem hiding this comment.
I tested with this URL: https://rumble.com/embed/v17j3tt/?pub=3fid3f
As you can see, pub can be alphanumeric so you want to use [a-z0-9]+ or \w+ for it. And start should not be required to match.
I noticed that we could probably leverage the URL constructor here for parsing as I did in parseInternalLinks (see #765 (comment)). I think this would make this easier and cleaner.
The other twitter and youtube embed code you can see here is just old and bad and isn't using them yet.
If you want to refactor the existing embed code to also use the URL constructor for parsing, I recommend doing this in a separate PR.
|
Thanks for the feedback! Regex creation is definitely a skill I have not grasped yet lol I'll take a brief look at the refactor as well if we intend to add embeds for many different sites it might be worth pulling them up to be reused in multiple places
Short of scraping the data somehow when loading in from the standard link I don't think its possible. in the rumble embed tutorial video https://rumble.com/v1a59rb-rumble-basics-how-to-embed-your-video.html they talk about how embeded videos get an 80% revenue share so I imagine they don't want us to be able to claw back that 20% on a simple script
Ok I will try to get this rumble embed right first and then I can come back through with a refactor |
|
Nice iframe solution!
I don't think we can expect people to share the right url, so if we want this to be used, we'll probably want to make it work for either url:
|
Ok! yeah i think i was overthinking it, good idea! i'll give that a try! |
|
Converting to draft for now until I can get a better sense of how to fetch the embed link. |
I am confused. Maybe my English was confusing but that's what I meant with
However, I forgot that we're already scraping websites to auto-populate titles + publication date here with |
No you're fine! That's on me I just wasn't thinking |
| </div> | ||
| ) | ||
| } else { | ||
| return null |
There was a problem hiding this comment.
added these return nulls for now As i was hoping to create some logic for when the user's link does Not follow the expected embed format but at the moment no logic for that in rumble
|
I cleaned up the logc for rumble and added the embeds for link posts as well. Also in my attempts to find a solution for our embed vs non-embeded links I realized odysee handles embeds more simply. I am a little stuck with the client-side fetching, I either am unable to get the tool to work like domino or puppeteer as they seem to be designed for server-side or when trying to fetch the data directly with some basic fetch logic I am getting CORS errors. @ekzyis any thoughts on another avenue I could try? I breifly looked at the naming convention between |
|
I'd think you can just |
| const response = await fetch(ensureProtocol(url)) | ||
| const html = await response.text() | ||
| if (url.includes('peertube.tv')) { | ||
| return html.match(/"([^"]*\/embed\/[^"]*)"/)[0].substring(1, html.match(/"([^"]*\/embed\/[^"]*)"/)[0].length - 1) |
There was a problem hiding this comment.
This is the parse for the peertube html to prove to myself that this method works, it looks for the embed link and then trims the " " on either end
There was a problem hiding this comment.
From my brief investigation becauses bitcoin.tv is based on peertube it should have the exact same format so we should be able to use this exact code for parsing links from there as well
| @@ -553,6 +553,17 @@ export default { | |||
|
|
|||
| return res | |||
| }, | |||
| fetchDocument: async (parent, { url }, { models }) => { | |||
| try { | |||
| const response = await fetch(ensureProtocol(url)) | |||
There was a problem hiding this comment.
no matter how hard I tried to play with fetching and the link rumble would not let me fetch for any link with the format https://rumble.com/vabc1234-some-rumble-video.html, perhaps I am missing some additional step but this does work with peertube as expected
There was a problem hiding this comment.
The really confusing thing is that when I test a get request with this rumble link in something like postman, it works
There was a problem hiding this comment.
What kind of errors are you getting on fetch?
There was a problem hiding this comment.
Here is the error object being thrown
{
"errors": [
{
"message": "fetch failed",
"locations": [
{
"line": 2,
"column": 3
}
],
"path": [
"fetchDocument"
],
"extensions": {
"code": "INTERNAL_SERVER_ERROR",
"stacktrace": [
"Error: fetch failed",
" at Object.fetchDocument (webpack-internal:///(api)/./api/resolvers/item.js:578:23)",
" at process.processTicksAndRejections (node:internal/process/task_queues:95:5)"
]
}
}
],
"data": {
"fetchDocument": null
}
}
There was a problem hiding this comment.
Wow that's not descriptive at all. Regardless, I think we want to call this on the client.
Have you tried fetching in your browser console?
There was a problem hiding this comment.
When I fetch from the frontend or the console I get CORS errors
Cross-Origin Request Blocked: The Same Origin Policy disallows reading the remote resource at https://rumble.com/v1a59rb-rumble-basics-how-to-embed-your-video.html. (Reason: CORS header ‘Access-Control-Allow-Origin’ missing). Status code: 200.
Cross-Origin Request Blocked: The Same Origin Policy disallows reading the remote resource at https://rumble.com/v1a59rb-rumble-basics-how-to-embed-your-video.html. (Reason: CORS request did not succeed). Status code: (null).
There was a problem hiding this comment.
Ah, that makes sense. CORs is a pain.
I'd try getting a more descriptive error on the server.
There was a problem hiding this comment.
I was having a look at #879 and I was curious if rumble had a similar cors policy so I gave it a test by using this https://github.com/Rob--W/cors-anywhere as a test to see if I could grab the embed link and it works!
Note the html link but it still renders the embeded video
There was a problem hiding this comment.
didn't implement it in the sn repo yet, I just added the demo https://cors-anywhere.herokuapp.com/ server as a prefix to the url
|
Closing after #1191 was merged |
Closes #134 but if this works as we hope I am hoping to add in the embed functionality for other sites as well
Embeds rumble video embed links, unfortunately rumble is weird and what you see at the top is not the embed link you want like it is on youtube but thats the price you pay i guess for their revenue share model.
It has to do with the fact that embeded videos have a different revenue share percentage so keeping track of how to divide the money is too difficult when everyone is downloading the video from the same url (I'm guessing)
Rumble vs a Youtube embed
This standard
<iframe/>model should be able to be used on many sites so maybe once we do some quick QA on this I could try and add a more general "video embed" that parses video embeds from several of the other popular video sitesI'm going to have this PR open for now but if we want to add embedding for more sites i can put it into draft untl the logic is finished for those as well
Summary by CodeRabbit
rumble.com, alongside existing permissions forwww.youtube.comandplatform.twitter.com.