feature: add embeddable video player - #2
Conversation
Priveetee
left a comment
There was a problem hiding this comment.
Hey @tam1m,
Thank u again for moving this PR to the new frontend repo and for taking the time to do it! :)
First of all, I'm really sorry again about the timing. I reorganized TypeType just after your first PR, and now I also added a CONTRIBUTING.md after you had already done most of the work 😭
So if some parts do not follow the new contribution rules yet, no worries. That is on me, not you. You could not follow rules that did not exist when you started ^^
I finally had the time to review and test the complete PR properly. Honestly, the feature is really cool and I definitely want it in TypeType! The embed is separated from the normal watch page, the video ID is validated, the guest-disabled state is there, and visually the player looks really clean :)
I found a few things that should be fixed before I test it on beta. I added the exact details directly on the relevant lines of code, so I will not repeat a three-kilometer technical explanation here ^^
The main one is playback recovery. Right now, if the media fails, the embed can restart from the original t position instead of where the viewer actually was. It can also return paused after someone started the video manually, and there is no visible Retry action when automatic recovery gives up.
The other inline comments cover the permanent loading state if /instance fails, the 401/403 message, Invidious query parameters, keyboard focus, and the new component-per-file rule. I can take care of the smaller cleanup myself if needed, no worries :)
For the tests, don't worry, I'm not asking you to build a huge test suite 😅 One small focused test around the embed route, its query parameters, and its permanent error state is enough here. I can improve the deeper playback recovery coverage myself afterward.
I tested the current version locally and everything currently passes:
bun install --frozen-lockfile;bun run check;bun run testwith 59 passing tests;bun run knip;bun run sherif;bun run build;git diff --checkand the merge simulation.
The PR now correctly targets dev. It originally targeted main because I had not changed the default branch of the new repository yet. Again, that was my fault, not yours. I will also change the repository default branch to dev.
Once the remaining changes are ready, I'll merge the PR into dev, let it deploy to beta, and test the embed in real conditions. I will not merge it directly into main. If everything works correctly on beta, I'll prepare the final merge afterward.
Also, there are two completely valid ways to continue. If you want to make the remaining changes yourself, I'll be happy to review and test them with you. If you prefer, I can take over the remaining fixes from here without touching your original commits, authorship, or contribution. No problem either way :)
I know this review looks a little long, but I prefer giving you everything now instead of coming back three times with new problems.
Thank u again for contributing to TypeType, I really appreciate it! And I'm honestly sorry again for the super unlucky timing around the migration and the new contribution rules 🥰
|
Alright.. First of all, thank you for the very thourough review. That was actually way more than I expected on feedback. Loved that! I'm really glad you liked the idea of the embed feature. I really wanted to use TypeType for that and thats pretty much the last thing I needed to fully replace Invidious. Regarding your feedback. I tried to mirror the normal player and behaviour as much as possible. Also updated the error displays to match the the ones from the watchpage/player. So UI changed a bit to be more cohrent with the rest of TypeType. Let me know if there is anything else that need attention.
Honestly, I prefer this over three rounds of of back and forth comments. So thanks again for your time and thank you for TypeType in general. Keep it up :) edit: Probem is, the button is hardcoded in the |
|
Hey @tam1m, Yes, I think I took over the remaining cleanup like I proposed. I kept all your original commits and authorship untouched and only added 3 small commits on top. All six review threads are handled now. About your Firefox question: the overlay link isn't actually trapping the focus. Firefox moves focus between the page, Vidstack's internal controls and sometimes the browser chrome differently from Chromium. When focus leaves the document, the previous element can still look focused, which explains the wonky behavior you noticed. I don't think adding a focus trap or forcing the tab order would be a good fix here because it could make keyboard navigation worse. Your While testing everything, I also found a few edge cases around cross-origin sessions, guest mode, autoplay recovery and the initial Everything passes locally with 96 tests, and I tested the player in Chromium and Firefox on both desktop and mobile. So there is nothing else needed from you on this PR now :) My next step is to merge this into I still have quite a lot of work before the next stable release, so it won't reach Thank u again for the feature and for all the work you put into it! I honestly really like it and I'm happy to finally have proper embeds in TypeType :) |
Migrated from TypeType-Video/TypeType#159 because apparently timing isn't my strong suit :D
This adds an
/embed/$videoIdendpoint for embedding the TypeType player.My main use case is replacing embedded YT videos via the LibRedirect extension, but can be used for whatever of course. It's fully compatible with Libredirects using Invidious as frontend setting.
Example Urls (not working on this instance for now, of course):
YouTube: https://watch.typetype.video/embed/dQw4w9WgXcQ
NicoNico: https://watch.typetype.video/embed/sm37617029?t=30
BillBill: https://watch.typetype.video/embed/BV1UT42167xb?t=1m30s&autoplay=1
Since the auth token and settings live in localStorage, embedded same-origin and cross-origin work a bit differenlty.
For same-origin (opening /embed/videoid directly) works like the normal player that just fills the whole browsertab. With account and settings all being used.
For cross-origin TypeTypes localStorage isnt readable and therefor, it just uses some default settings. The server MUST have guest mode enabled for this to work, otherwise users will be redirected to the main TypeType watch page. See image below..
It also supports
?t=30and?autoplay=1params and reuses the upstreamVideoPlayer, so all the the normal player features (SABR, chapters, subtitles, quality switching etc) are working.I actually built this over the last weekend on the pre-1.0 codebase, just to realise today, the entire architecture had been rewritten. Had to rewrite the whole thing, but the changes made it so much simpler now.
Guest Mode Off:
