support youtube clip embedding #1652

riccardobl · 2024-11-26T10:08:56Z

Description

Closes #879

This PR uses the field fetchMeta and a supporting api to fetch additional metadata needed to embed the video bypassing the lack of cors headers.

riccardobl · 2024-11-26T10:10:04Z

components/embed.js

+              allow='accelerometer; autoplay; clipboard-write; encrypted-media; gyroscope; picture-in-picture; web-share'
+              referrerPolicy='strict-origin-when-cross-origin'
+            />
+          </div>


react-youtube doesn't support clips yet

riccardobl · 2024-11-26T11:57:54Z

api/resolvers/embed.js

+
+export default {
+  Query: {
+    fetchEmbedMeta: async (parent, { provider, args }, { models, me }) => {


This needs to happen serverside because youtube doesn't set cors headers. See #879

riccardobl · 2024-11-26T11:59:09Z

lib/url.js

+            clipId: id,
+            clipt: null
+          },
+          fetchMeta: {


this new field is used to tell the code to fetch additional metadata from an external provider

riccardobl · 2024-11-26T11:59:27Z

lib/validate.js

@@ -487,6 +487,10 @@ export const pushSubscriptionSchema = object({
  auth: string().required('required').trim()
 })

+export const youtubeClipMetaFetchArgsSchema = object({
+  clipId: string().required('required').matches(/^[\w_]+$/, 'invalid clip id').max(64, 'clip id is too long')


i don't really know the length of yt clip ids, i've used 64 to be conservative

riccardobl · 2024-11-26T12:01:54Z

api/resolvers/embed.js

+        where: { hash }
+      }))?.meta
+
+      if (meta?.updatedAt && (Date.now() - meta.updatedAt.getTime()) / 1000 > EMBED_META_CACHE_DURATION_S) {


Don't cache this forever, so that the system recovers at some point if youtube does breaking changes

Copilot

Copilot reviewed 5 out of 10 changed files in this pull request and generated no suggestions.

Files not reviewed (5)

prisma/migrations/20241126113801_embed_meta/migration.sql: Language not supported
prisma/schema.prisma: Language not supported
api/typeDefs/index.js: Evaluated as low risk
components/embed.js: Evaluated as low risk
lib/constants.js: Evaluated as low risk

Comments skipped due to low confidence (1)

lib/validate.js:491

[nitpick] The error message 'invalid clip id' could be more descriptive. Consider changing it to 'Clip ID must be alphanumeric and can include underscores'.

clipId: string().required('required').matches(/^[\w_]+$/, 'invalid clip id').max(64, 'clip id is too long')

ekzyis

Is there something wrong with cachedFetcher from lib/fetch?

I thought we discussed that we want to use the same caching implementation like we do for price, blockheight, fees etc.

Or wasn't this changed yet so this isn't actually ready for review yet?

riccardobl · 2024-11-30T09:48:12Z

Is there something wrong with cachedFetcher from lib/fetch?

I thought we discussed that we want to use the same caching implementation like we do for price, blockheight, fees etc.

Or wasn't this changed yet so this isn't actually ready for review yet?

cacheFetcher uses in-memory cache, it is good for price, blockheight and fees because they all are a single values that come from a single source, but using it to store clips metadata can potentially make the cache grow too large. Using the database and its memory management capabilities makes more sense in this case, also because this cache can last for a very long time (even for ever if we want to), since these values should never change unless youtube updates its internals.

huumn · 2024-12-01T18:12:45Z

I like that you created a table for embed meta!

Rather than doing a "lazy load" of embed meta, I think it might be better to do it as job in the worker like we do with images/video, so that the client doesn't need to query for the embed meta. That method has worked well for us.

We can also add a trigger to denormalize the embed meta into its "Item" so that we don't have to join (or do an additional query) when items are queried.

(I'd prefer images/videos be normalized like your embed code and denormalize in a trigger like this, but that's another issue.)

Thoughts?

Copilot

Copilot reviewed 5 out of 15 changed files in this pull request and generated 1 suggestion.

Files not reviewed (10)

prisma/migrations/20241202100121_embedmeta/migration.sql: Language not supported
prisma/schema.prisma: Language not supported
worker/fetchEmbedMeta.js: Evaluated as low risk
api/paidAction/itemUpdate.js: Evaluated as low risk
api/typeDefs/item.js: Evaluated as low risk
components/comment.js: Evaluated as low risk
components/item-full.js: Evaluated as low risk
worker/index.js: Evaluated as low risk
api/paidAction/itemCreate.js: Evaluated as low risk
fragments/comments.js: Evaluated as low risk

components/embed.js

+          <iframe
+            title='Youtube Clip'
+            allowFullScreen
+            src={`https://www.youtube.com/embed/${videoId}?clip=${clipId}&amp;clipt=${clipt}`}


Copilot

Copilot reviewed 15 out of 15 changed files in this pull request and generated no suggestions.

riccardobl · 2024-12-02T17:53:15Z

I've moved the fetcher into a worker and added some logic to avoid refetches.
The system is extensible by registering a function of type MetaFetcher to a provider key in lib/embedFetcher/MetaFetchers.

Copilot

Copilot reviewed 5 out of 16 changed files in this pull request and generated 1 suggestion.

Files not reviewed (11)

prisma/migrations/20241202100121_embedmeta/migration.sql: Language not supported
prisma/schema.prisma: Language not supported
components/comment.js: Evaluated as low risk
components/item-full.js: Evaluated as low risk
api/paidAction/itemCreate.js: Evaluated as low risk
worker/index.js: Evaluated as low risk
api/typeDefs/item.js: Evaluated as low risk
api/paidAction/itemUpdate.js: Evaluated as low risk
fragments/items.js: Evaluated as low risk
fragments/comments.js: Evaluated as low risk
components/text.js: Evaluated as low risk

Comments skipped due to low confidence (1)

components/embed.js:182

Ensure that the meta object contains videoId, clipId, and clipt properties before constructing the iframe URL. Add a check to handle missing properties gracefully.

if (provider === 'youtube-clip') {

lib/embedFetcher.js

+
+  const content = metaProp.getAttribute('content')
+
+  const fClipId = content.match(/clip=([^&]+)/)[1]


riccardobl · 2024-12-02T18:03:29Z

worker/fetchEmbedMeta.js

+    if (!id) {
+      console.warn('[fetchEmbedMeta] embed', provider, "doesn't have an id, skipping")
+      continue
+    }


Some parsers in lib/url/parseEmbedUrl have undefined id, this is a bit counter intuitive, but they are valid for the rest of the code.

We need an id to cache the embed in the database, so instead of trying generate one by hashing stuff, i've decided to just add a warning here, so if any other dev is trying to add server side meta fetching for those providers, but forgets to add an id, he will have something to debug why it doesn't work.

riccardobl · 2024-12-02T18:05:56Z

worker/fetchEmbedMeta.js

+  const embeds = urls
+    .map(url => parseEmbedUrl(url)) // parse every unique url as a potential embed (initial url based deduplication)
+    .filter(embed => !!embed) // if it is not an embed, we can skip it
+    .reduce((acc, embed) => { // deduplicate embeds by id and provider (they might have different urls but be resolved to the same embed)


Why would someone want to embed the same thing more than once?
Idk, to abuse the system, or meme, either way, better to try to stop it here rather than firing multiple fetches

huumn

This will probably sit until after jan 3.

We haven't had anyone share a youtube clip in months, so on that level this may not be worth it for youtube clips alone, but it'll be worth merging if the logic is also useful for other stuff. Hard to tell atm, but let's keep it open until we have more time for nice-to-haves.

support youtube clip embedding

75ca334

riccardobl commented Nov 26, 2024

View reviewed changes

riccardobl marked this pull request as draft November 26, 2024 11:44

improve code, add cache

3a29a9e

riccardobl commented Nov 26, 2024

View reviewed changes

riccardobl marked this pull request as ready for review November 26, 2024 12:05

riccardobl force-pushed the clipembed branch from f936208 to 45cd454 Compare November 26, 2024 12:05

if meta are not fetchable, use an empty object

4c8a785

riccardobl force-pushed the clipembed branch from 45cd454 to 4c8a785 Compare November 26, 2024 12:05

riccardobl requested a review from Copilot November 26, 2024 12:06

Copilot AI reviewed Nov 26, 2024

View reviewed changes

riccardobl added the enhancement improvements to existing features label Nov 26, 2024

validate before args destructuring

153c504

ekzyis reviewed Nov 30, 2024

View reviewed changes

riccardobl added 6 commits December 2, 2024 12:10

fetch embed meta in worker

f5fa9b2

Merge branch 'master' into clipembed

9bc8b02

remove dead code

880842b

remove more dead code

156bfa7

fix

8fda6f5

cleanup

c184d3d

riccardobl requested a review from Copilot December 2, 2024 11:49

Copilot AI reviewed Dec 2, 2024

View reviewed changes

components/embed.js

<iframe

title='Youtube Clip'

allowFullScreen

src={`https://www.youtube.com/embed/${videoId}?clip=${clipId}&clipt=${clipt}`}

This comment was marked as off-topic.

Sign in to view

riccardobl marked this pull request as draft December 2, 2024 12:42

riccardobl added 3 commits December 2, 2024 14:06

fix url finding

b1550d6

implement clip embedding for link posts

d9067a4

optimize and improve database operations

1691775

riccardobl requested a review from Copilot December 2, 2024 15:46

Copilot AI reviewed Dec 2, 2024

View reviewed changes

riccardobl added 2 commits December 2, 2024 18:45

more optimization and extract fetch logic to lib

b30fadc

Merge branch 'master' into clipembed

5c41d53

riccardobl marked this pull request as ready for review December 2, 2024 17:51

riccardobl requested review from Copilot and huumn December 2, 2024 17:54

Copilot AI reviewed Dec 2, 2024

View reviewed changes

lib/embedFetcher.js

const content = metaProp.getAttribute('content')

const fClipId = content.match(/clip=([^&]+)/)[1]

This comment was marked as off-topic.

Sign in to view

riccardobl commented Dec 2, 2024

View reviewed changes

riccardobl added 5 commits December 2, 2024 19:10

fix if item is undefined

0b08b6d

Merge branch 'master' into clipembed

6992e34

Merge branch 'master' into clipembed

c1686df

Merge branch 'master' into clipembed

ec7b1ea

Merge branch 'master' into clipembed

f0aa58f

huumn reviewed Dec 29, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

support youtube clip embedding #1652

support youtube clip embedding #1652

riccardobl commented Nov 26, 2024 •

edited

Loading

riccardobl Nov 26, 2024

riccardobl Nov 26, 2024 •

edited

Loading

riccardobl Nov 26, 2024

riccardobl Nov 26, 2024

riccardobl Nov 26, 2024

Copilot AI left a comment

ekzyis left a comment

riccardobl commented Nov 30, 2024 •

edited

Loading

huumn commented Dec 1, 2024 •

edited

Loading

Copilot AI left a comment

This comment was marked as off-topic.

Copilot AI left a comment

riccardobl commented Dec 2, 2024 •

edited

Loading

Copilot AI left a comment

This comment was marked as off-topic.

riccardobl Dec 2, 2024

riccardobl Dec 2, 2024 •

edited

Loading

huumn left a comment


		const content = metaProp.getAttribute('content')

		const fClipId = content.match(/clip=([^&]+)/)[1]

support youtube clip embedding #1652

Are you sure you want to change the base?

support youtube clip embedding #1652

Conversation

riccardobl commented Nov 26, 2024 • edited Loading

Description

riccardobl Nov 26, 2024

Choose a reason for hiding this comment

riccardobl Nov 26, 2024 • edited Loading

Choose a reason for hiding this comment

riccardobl Nov 26, 2024

Choose a reason for hiding this comment

riccardobl Nov 26, 2024

Choose a reason for hiding this comment

riccardobl Nov 26, 2024

Choose a reason for hiding this comment

Copilot AI left a comment

Choose a reason for hiding this comment

ekzyis left a comment

Choose a reason for hiding this comment

riccardobl commented Nov 30, 2024 • edited Loading

huumn commented Dec 1, 2024 • edited Loading

Copilot AI left a comment

Choose a reason for hiding this comment

This comment was marked as off-topic.

Copilot AI left a comment

Choose a reason for hiding this comment

riccardobl commented Dec 2, 2024 • edited Loading

Copilot AI left a comment

Choose a reason for hiding this comment

This comment was marked as off-topic.

riccardobl Dec 2, 2024

Choose a reason for hiding this comment

riccardobl Dec 2, 2024 • edited Loading

Choose a reason for hiding this comment

huumn left a comment

Choose a reason for hiding this comment

riccardobl commented Nov 26, 2024 •

edited

Loading

riccardobl Nov 26, 2024 •

edited

Loading

riccardobl commented Nov 30, 2024 •

edited

Loading

huumn commented Dec 1, 2024 •

edited

Loading

riccardobl commented Dec 2, 2024 •

edited

Loading

riccardobl Dec 2, 2024 •

edited

Loading