Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix og:image requests when html in a web page is over 1.megabyte #30362

Merged
merged 6 commits into from
May 19, 2024

Conversation

phocks
Copy link
Contributor

@phocks phocks commented May 19, 2024

When truncating we were adding the chunk and then checking if it was over 1MB. This meant that it was always over 1MB with that included chunk. You need to check before adding the chunk, otherwise it will always fail. Sites like the New Yorker and Wired often have 1MB + html in their articles now (not good but what can you do).

This seems to fix a bunch of issues dealing with preview images not loading which were originally thought to be webp or avif errors: #18762 #14983 #27370 (but not all of them ie. The Guardian images seem to be having issues with a certain CDN key or something. Anyway, this should fix bunch of stuff because it won't raise the error and stop the return.)

ps. I haven't written much Ruby before so feel free to make any changes etc.

@phocks
Copy link
Contributor Author

phocks commented May 19, 2024

Hmm I'm just trying to fix the tests, but it seems they require it to reject the request? We don't want that do we?

@renchap
Copy link
Sponsor Member

renchap commented May 19, 2024

This fix does not seem correct at all. I think changing line 59 of app/services/fetch_link_card_service.rb to res.truncated_body should make it work without other changes.

@phocks
Copy link
Contributor Author

phocks commented May 19, 2024

Ah yes. Thank you @renchap. I was going down a wrong rabbit hole it seems! But was kind of on the right track. I was about to suggest res.body_with_limit(2.megabytes) fixes it too, but res.truncated_body does it better. Thanks again. Will be good to have preview cards working with these sites that return more than 1MB in html.

Anyway feel free to either merge this latest commit, or just close the PR and change that line in another one. I just wanna see it fixed. It's been driving me mad.

Cheers.

@Gargron Gargron added this pull request to the merge queue May 19, 2024
Merged via the queue into mastodon:main with commit 6282b6d May 19, 2024
30 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants