Issue with Onebox and Youtube

Contribute Bug

AI-generated summary

A user, merefield, reported an issue with Onebox and YouTube, where the thumbnail fails to show up on the Topic List for certain themes and plugins. The issue appears to be related to a recent regression in Discourse or Onebox and is not present in prior builds. merefield identified the problematic commit through a Git Bisect and Falco pointed out the specific code change that caused the issue.

The change replaced the post.post_analyzer.found_oneboxes? method with a CSS selector approach, which fails to detect oneboxes that use oembed or iframe. codinghorror and Falco expressed concerns about the fragility of this change.

nbianca explained the reasoning behind the change, which was to prevent awarding the “First Onebox” to any posted link. However, nbianca later acknowledged that the change broke the detection of oneboxes and created a PR to fix the issue.

Further investigation revealed that the issue is not solely related to the Onebox change, but also involves the TLP plugin and its overrides. merefield and nbianca discussed the possibility that the TLP plugin was relying on a changed behavior.

The issue was eventually fixed with a PR, as confirmed by nbianca.

1

e.g.

Youtube thumbnail fails to show up on Topic List for e.g. Fakebook theme, TLP plugin.

STR:

Post a youtube video, switch to Fakebook Theme, no thumbnail appears.

Workaround:

Rebuild the post and the thumbnail appears, however, given users are expecting the thumbnail to appear imminently without staff intervention, this is not a great solution.

More info:

Topic.image_url appears to be nil

This seems to have broken recently, after beginning of August … I think this worked before August the 5th (but not sure when it actually broke). I’ve noticed this aligns roughly with a new version of Onebox …

I tested this on a prior build and the issue was not present, ruling out Youtube og responses I hope. Also Fakebook theme shows the same behaviour as TLP plugin, ruling out TLP plugin, so I do suspect it’s a recent regression in Discourse or Onebox.

This is a real issue for users of Topic List Previews.

4 Likes
2

Update, I’ve done a Git Bisect, and this seems to be the problem commit:

https://github.com/discourse/discourse/commit/7c83d2eeb261ac676a8320e6a704752c56fd242e

3 Likes
3 
diff --git a/lib/cooked_post_processor.rb b/lib/cooked_post_processor.rb
index ac16eb82c16..46050a69e40 100644
--- a/lib/cooked_post_processor.rb
+++ b/lib/cooked_post_processor.rb
@@ -28,7 +28,7 @@ def initialize(post, opts = {})
     @cooking_options = @cooking_options.symbolize_keys
 
     @doc = Nokogiri::HTML::fragment(post.cook(post.raw, @cooking_options))
-    @has_oneboxes = post.post_analyzer.found_oneboxes?
+    @has_oneboxes = @doc.css("aside.onebox").count > 0
     @size_cache = {}

@nbianca why did we don’t use the post.post_analyzer.found_oneboxes? method anymore in here?

All oneboxes which use either oembed or iframe will fail the new CSS selector mode, and that also makes our Onebox detector be different across the codebase.

4 Likes
4

I agree @nbianca this looks like an incredibly fragile change, why was this method selected?

2 Likes
5

The problem was that the First Onebox was awarded for any posted link, no matter if it was or was not a Onebox. This happens because PrettyText adds class="onebox" to all links and then Oneboxer tries to parse all links with class onebox.

The change I made checks if the final document has any Oneboxes (Oneboxes are aside with class onebox). I will have another look and see what and why is it broken and come back with a change that fixes both of the issues.

7 Likes
6

I did some improvements here and created this PR.

I just tested v2.3.2 and it does not look like YouTube links set the image_url attribute of the topic. The reason is that the image is extracted from the src attribute of an img, but when a YouTube video is linked, there is a <div> with a background-image CSS attribute.

Are you sure image_url used to be set?

5 Likes
7

Thanks for looking at this. Let me check my end and revert

8

@nbianca apologies, this might be a combination of two issues

Works on and prior to this with TLP plugin:

https://github.com/discourse/discourse/commit/922c40f87cdceb8197dd61361334e0def23f13d5

After this commit, it fails with TLP.

I agree with you that for YouTube, out of the box that property is not populated.

So it seems like TLP was doing a better job with its overrides, but relying on something which is now changed.

FYI the relevant overrides are here: https://github.com/angusmcleod/discourse-topic-previews/blob/master/lib/cooked_post_processor_edits.rb

I think I will need to do some more analysis, but it looks like there’s been enough of a change to break TLP.

Let me dig into this a little more and revert again. Unfortunately I may not get chance over next couple of days.

1 Like
9

I don’t see how that commit can break the TLP plugin :thinking: Are you 100% of your bisect?

1 Like
10

Very odd how the bisect found a change to cooked_post_processor though … I will double check and revert.

Yep, confirmed:

With this commit it fails: 7c83d2eeb261ac676a8320e6a704752c56fd242e
With this prior commit it’s fine: 922c40f87cdceb8197dd61361334e0def23f13d5

1 Like
11

The problem was fixed here:
https://github.com/discourse/discourse/pull/8019

7 Likes