feature: add embeddable video player#2
Conversation
| captionStyles={settings.captionStyles} | ||
| onCaptionStylesChange={(captionStyles) => update.mutate({ captionStyles })} | ||
| onVolumeChange={handleVolumeChange} | ||
| onError={player.handleError} |
There was a problem hiding this comment.
Hey, I think this should use the same recovery flow as the normal watch player. Right now the raw handleError callback remounts the player with the original startTime and autoplay values. If the viewer is already far into the video when the media fails, the embed can return to the initial t position and may stay paused. There is also no visible Retry action once automatic recovery gives up.
I think we should use usePlayerErrorResume, or an equivalent recovery state, so the current position and play intent are preserved here.
There was a problem hiding this comment.
Hey :) Position and play intent are now preserved and there's a retry button once recovery has given up
| const startTime = parseStartTime(t) * 1000; | ||
| const shouldAutoplay = autoplay === 1; | ||
|
|
||
| if (instancePending || !instance) return <EmbedLoading />; |
There was a problem hiding this comment.
If the /instance request fails, instancePending becomes false but instance stays undefined, so this condition keeps the embed on the loading screen forever.
I think a separate query-error state with a visible Retry action would be better here, so the viewer is not left on an infinite loading screen.
There was a problem hiding this comment.
Should be good now. I separate loading and error states here
| ) { | ||
| return <EmbedGuestRequired watchUrl={watchUrl} />; | ||
| } | ||
| const message = |
There was a problem hiding this comment.
A 401 or 403 does not always mean that guest access is disabled. It can also come from an allow-list restriction, an authentication problem, or a YouTube session requirement. In those cases this would show the viewer the wrong explanation.
I think we should keep the guest-required screen only when guest access is actually known to be disabled, and use the normal error state for the other cases.
There was a problem hiding this comment.
Removed that block. The prestream !guestAllowed check handles actual guest disabled mode. Stream errors are now shown as they should.
| export const Route = createFileRoute("/embed_/$videoId")({ | ||
| validateSearch: (search: Record<string, unknown>): EmbedSearch => ({ | ||
| t: typeof search.t === "string" || typeof search.t === "number" ? search.t : undefined, | ||
| autoplay: typeof search.autoplay === "number" ? search.autoplay : undefined, |
There was a problem hiding this comment.
Small compatibility thing: LibRedirect preserves the original Invidious query parameters, and Invidious accepts t, start, and time_continue for the starting position. This route currently keeps only t, so some redirected embeds would silently start from the beginning.
I think supporting those three aliases as the same start time would preserve compatibility. A small focused test for the query parsing is enough here :)
There was a problem hiding this comment.
All three params supported now. t takes proirity over start over time_continue if multiple are present.
| href={watchUrl} | ||
| target="_blank" | ||
| rel="noopener noreferrer" | ||
| className="absolute top-0 left-0 right-0 z-10 flex items-center gap-2 px-3 py-2 text-sm text-white/90 transition-opacity opacity-0 group-hover:opacity-100 pointer-events-none group-hover:pointer-events-auto" |
There was a problem hiding this comment.
Small accessibility detail: this link can receive keyboard focus while it is still invisible because the overlay is revealed only through group-hover.
I think the overlay should also react to focus-visible or group-focus-within, with a visible focus state for keyboard users.
There was a problem hiding this comment.
The overlay now appears on keyboard focus. With Firefox, it's a bit wonky when tabbing between the player, overlay and the browser chrome. Focus gets stuck sometimes. Works fine with chromium. Any idea what might be causing that?
| } | ||
|
|
||
| function EmbedLoading() { | ||
| return ( |
There was a problem hiding this comment.
There is now a new CONTRIBUTING.md asking for one React component per file. It did not exist when you started this PR, so this is absolutely not your fault ^^
If u can, I think it would be better to move the loading, error, and guest-required states into their own component files so this PR follows the new structure.
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:
