✨ Apply embed specific tag on subjects for video, image and external #2703

foysalit · 2024-08-09T16:54:19Z

This PR refactors the existing subject language tagger to build a more generic subject tagger that is capable or running multiple taggers that evaulates the record content from the subject and applies necessary tags.
The current implementation adds tag for embed content type along with existing language tags.

devinivy

I like the new tagging setup!

packages/ozone/src/tag-service/content-tagger.ts

devinivy · 2024-08-14T03:19:13Z

packages/ozone/src/tag-service/index.ts

+            if (tagger.isApplicable()) {
+              const newTags = await tagger.getTags()
+              if (newTags.length) tags.push(...newTags)
+            }


What is the intended behavior if you call tagger.getTags() without checking tagger.isApplicable()? It seems like isApplicable() may not need to be part of the public interface— the implementation of getTags() could alternatively check this itself and no-op by returning [].

I normally try to split these and let the consumer of the class have the option to perform necessary checks before invoking the getter. for getters like this, i often find use-cases down the line where we would wanna know "what would be the tags IF they were applicable?".
however, this isn't the case here... yet so I'm happy to make that check implicit.

That makes sense—it just seemed like the class could be prone to misuse, as it seemed implementations of getTags() were assuming it was gated by isApplicable().

I think there's a way to get the best of both worlds using the abstract class, where there's a default implementation for getTags() that performs the isApplicable() check. And then there would be a separate method for implementers getTagsImple() that is used within the default implementation of getTags().

ohh I see what you mean. refactored it a bit to get that going.

devinivy · 2024-08-14T03:22:50Z

packages/ozone/src/tag-service/embed-content-type-tagger.ts

+    return Boolean(
+      this.subjectStatus &&
+        this.subject.isRecord() &&
+        this.subject.uri.includes('app.bsky.feed.post') &&


It would be cozier if we either checked includes('/app.bsky.feed.post/'), or went ahead and parsed the URI and checked its collection.

const getCollection(uri: string) { return new AtUri(uri).collection }

devinivy · 2024-08-14T03:23:02Z

packages/ozone/src/tag-service/embed-content-type-tagger.ts

+      this.subjectStatus &&
+        this.subject.isRecord() &&
+        this.subject.uri.includes('app.bsky.feed.post') &&
+        !this.subjectStatus.tags?.find((tag) => tag.includes(this.tagPrefix)),


Just a little more specific!

Suggested change

!this.subjectStatus.tags?.find((tag) => tag.includes(this.tagPrefix)),

!this.subjectStatus.tags?.some((tag) => tag.startsWith(this.tagPrefix)),

devinivy · 2024-08-14T03:29:05Z

packages/ozone/src/tag-service/embed-content-type-tagger.ts

+      if (AppBskyEmbedImages.isView(recordValue)) {
+        tags.push(`${this.tagPrefix}image`)
+      }


If recordValue is a record then we don't have our hands on a view (isView()). I'd expect something roughly along these lines:

Suggested change

if (AppBskyEmbedImages.isView(recordValue)) {

tags.push(`${this.tagPrefix}image`)

}

if (

AppBskyFeedPost.isRecord(recordValue) &&

(AppBskyEmbedImages.isMain(recordValue.embed) ||

AppBskyEmbedRecordWithMedia.isMain(recordValue.embed))

) {

tags.push(`${this.tagPrefix}image`)

}

thanks. was gonna ask for the correct check here once we have the video stuff available on main.

devinivy · 2024-08-14T03:29:17Z

packages/ozone/src/tag-service/embed-content-type-tagger.ts

+      //     @TODO: check for video embed here
+      return tags
+    } catch (err) {
+      log.error({ subject: this.subject, err }, 'Error getting record langs')


Good call failing open 👍

devinivy · 2024-08-14T03:30:03Z

packages/ozone/src/tag-service/language-tagger.ts

+  isApplicable(): boolean {
+    return Boolean(
+      this.subjectStatus &&
+        !this.subjectStatus.tags?.find((tag) => tag.includes(this.tagPrefix)),


Just a little more specific!

Suggested change

!this.subjectStatus.tags?.find((tag) => tag.includes(this.tagPrefix)),

!this.subjectStatus.tags?.some((tag) => tag.startsWith(this.tagPrefix)),

thanks, didn't like having to change this in 2 different places so refactored the class abstract class a bit more to move this logic in one place.

…ubject-video-tag

devinivy · 2024-08-17T16:47:57Z

packages/ozone/src/tag-service/embed-content-type-tagger.ts

+      }
+      const tags: string[] = []
+      if (
+        AppBskyEmbedImages.isView(recordValue) &&


I think we want this:

Suggested change

AppBskyEmbedImages.isView(recordValue) &&

AppBskyFeedPost.isRecord(recordValue) &&

This functionality should be fairly testable, I think—maybe we should get a test going passing a post record through it.

yeah good idea, was easier to get the right checks through a test. LMK if the current checks look right. my reasoning was, we wanna check if the record is a post with media embed first. and then, apply tags based on the type of media.

devinivy

Looking good! I think just need a final pass on that one check for image tags 👍

…ubject-video-tag

devinivy · 2024-08-29T14:10:08Z

packages/ozone/src/tag-service/content-tagger.ts

+  protected abstract buildTags(): Promise<string[]>
+
+  async getTags(): Promise<string[]> {
+    console.log(this.isApplicable(), 'isApplicable')


Suggested change

console.log(this.isApplicable(), 'isApplicable')

devinivy · 2024-08-29T14:12:24Z

packages/ozone/src/tag-service/embed-tagger.ts

+        AppBskyFeedPost.isRecord(recordValue) &&
+        AppBskyEmbedRecordWithMedia.isMain(recordValue.embed)


There are six cases for recordValue.embed we should be looking for:

image embed

video embed

external embed

"record with media" embed containing image media

"record with media" embed containing video media

"record with media" embed containing external media

Those last three are handled here, think we just need to handle the top three.

oof tripping over this a little bit. hope this last implementation covers all.

devinivy · 2024-08-29T14:17:30Z

packages/ozone/tests/content-tagger.test.ts

+
+  describe('embed tagger', () => {
+    it('Adds image tag to post with image', async () => {
+      const post = sc.posts[sc.dids.carol][0]


This is a record-with-media embed. if we add a test for sc.replies[sc.dids.bob][0] I think we'll hit the direct image embed case.

devinivy

Suuuuper duper close, think we just need to add cases for those additional embed types.

✨ Refactor subject tagging to facilitate video content tagging

684691e

devinivy reviewed Aug 14, 2024

View reviewed changes

foysalit added 3 commits August 14, 2024 11:49

♻️ Refactor tag check

6ea0a6f

Merge branch 'main' of github.com:bluesky-social/atproto into ozone-s…

2e70521

…ubject-video-tag

✅ Fix tagging logic

f2620b9

devinivy reviewed Aug 17, 2024

View reviewed changes

devinivy approved these changes Aug 17, 2024

View reviewed changes

foysalit added 3 commits August 18, 2024 12:16

♻️ Refactor content tagger and fix image content type check

fa5f72e

Merge branch 'main' of github.com:bluesky-social/atproto into ozone-s…

ceeb0b1

…ubject-video-tag

✨ Add embed tag check for video and external

c46b50d

foysalit changed the title ~~🚧 Refactor subject tagging to facilitate video content tagging~~ ✨ Apply embed specific tag on subjects for video, image and external Aug 29, 2024

devinivy reviewed Aug 29, 2024

View reviewed changes

✨ Add tagging for both media and image embed

c879e1d

devinivy approved these changes Aug 29, 2024

View reviewed changes

foysalit merged commit 372ed4c into main Aug 29, 2024
10 checks passed

foysalit deleted the ozone-subject-video-tag branch August 29, 2024 16:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨ Apply embed specific tag on subjects for video, image and external #2703

✨ Apply embed specific tag on subjects for video, image and external #2703

foysalit commented Aug 9, 2024 •

edited

Loading

devinivy left a comment

devinivy Aug 14, 2024

foysalit Aug 14, 2024

devinivy Aug 17, 2024

foysalit Aug 18, 2024

devinivy Aug 14, 2024

devinivy Aug 14, 2024

devinivy Aug 14, 2024

foysalit Aug 14, 2024

devinivy Aug 14, 2024

devinivy Aug 14, 2024

foysalit Aug 14, 2024

devinivy Aug 17, 2024 •

edited

Loading

foysalit Aug 18, 2024 •

edited

Loading

devinivy left a comment

devinivy Aug 29, 2024

devinivy Aug 29, 2024 •

edited

Loading

foysalit Aug 29, 2024

devinivy Aug 29, 2024

devinivy left a comment

	!this.subjectStatus.tags?.find((tag) => tag.includes(this.tagPrefix)),
	!this.subjectStatus.tags?.some((tag) => tag.startsWith(this.tagPrefix)),

	AppBskyEmbedImages.isView(recordValue) &&
	AppBskyFeedPost.isRecord(recordValue) &&

		AppBskyFeedPost.isRecord(recordValue) &&
		AppBskyEmbedRecordWithMedia.isMain(recordValue.embed)

✨ Apply embed specific tag on subjects for video, image and external #2703

✨ Apply embed specific tag on subjects for video, image and external #2703

Conversation

foysalit commented Aug 9, 2024 • edited Loading

devinivy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devinivy Aug 17, 2024 • edited Loading

Choose a reason for hiding this comment

foysalit Aug 18, 2024 • edited Loading

Choose a reason for hiding this comment

devinivy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devinivy Aug 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devinivy left a comment

Choose a reason for hiding this comment

foysalit commented Aug 9, 2024 •

edited

Loading

devinivy Aug 17, 2024 •

edited

Loading

foysalit Aug 18, 2024 •

edited

Loading

devinivy Aug 29, 2024 •

edited

Loading