feat: Unfurl image URLs #3901
feat: Unfurl image URLs #3901
Conversation
ilmotta
added
feature
E:New Unfurling API
Jenkins BuildsClick to see older builds (27)
|
ilmotta
force-pushed
the
feat/link-previews-for-image-urls
branch
2 times, most recently
from
a83f804 to
ee9e510
Compare
|
For reviewers: I couldn't find a way to satisfy the linter when using the repo's nix shell, and because that's all I can use in my system, the PR is currently blocked because I can't run a good I tried to go back to all other revisions that changed the nixpkgs revision in git checkout <any rev in status-go>
nix-shell --pure
make generate
make lint
# => lots of errors about copyLocksI talked with @cammellos about this problem, and hopefully he will be able to solve the copyLocks errors 🙏🏼 Thanks a lot for that!
cc'ing @jakubgs as an FYI |
| func newUnfurler(logger *zap.Logger, httpClient http.Client, url *neturl.URL) Unfurler { | ||
| if isSupportedImageURL(url) { |
There was a problem hiding this comment.
Hmmm. This way we won't be able to unfurl image links that don't end with required extension, e.g. https://placekitten.com/100/200. And Discord supports such links.
I think it would be great to decide if it's an image from HTTP header Content-Type: image/jpeg. Any chance it's possible to check the header without fetching the whole image?
There was a problem hiding this comment.
I started coding with this idea in mind @igor-sirotin, but in the end I decided against it in this PR for simplicity sake. To cleanly implement this logic we would need to change the logic that decides which unfurler to create, because the HTTP request would always need to be performed first. And because the request would have been made already, the unfurling logic of all unfurlers would change as well because the response body would already be available.
This should all work fine, unless I face issues related to the user agent. For example, requests to unfurl OpenGraph URLs need to use specific headers, otherwise some websites stubbornly fail. Hopefully the same headers can be used for image URLs.
Since you commented about this limitation, I now have another reason to implement this. I can do it on Monday (I'll be off this Friday).
There was a problem hiding this comment.
Any chance it's possible to check the header without fetching the whole image?
If we knew all servers understood HEAD requests, we'd be able to get responses without a body. So I think the best we can do is always request upfront and use that whole information to decide which unfurler to instantiate and pass the response to other functions.
It should work fine because we always fallback to OpenGraph to unfurl, so always requesting upfront won't incur in additional requests, since we would request something anyway.
There was a problem hiding this comment.
No prob!
I think it makes sense to skip it for now then and add this later. It will be a status-go update with no breaking changes for desktop/mobile.
@igor-sirotin, I can't push a clean PR due to code generation issues. If I push the results of a So I think this affects the desktop integration because you won't be able to build the library pointing to this branch. This is currently preventing me from opening a good PR in status-mobile too. Let's see if tomorrow the problem can be fixed, otherwise I'll need to ask somebody else to run |
@ilmotta, |
|
Tested in desktop, works like a charm 👍 Thank you, @ilmotta |
Sweet! 💯 |
igor-sirotin
force-pushed
the
feat/link-previews-for-image-urls
branch
from
57521f3 to
113cb4f
Compare
|
Took an opportunity to rebase on latest develop to fix conflicts. |
igor-sirotin
force-pushed
the
feat/link-previews-for-image-urls
branch
from
5b83e86 to
9b6e2b5
Compare
ilmotta
force-pushed
the
feat/link-previews-for-image-urls
branch
from
655b8f2 to
9e5c38e
Compare
Closes #3761
Summary
This PR adds support for unfurling static image URLs (not GIFs, not animated WebPs), such as https://placehold.co/[email protected]. It also compresses images before returning them as data URIs to clients.
I'll run
generateafter a round of review to avoid polluting the PR.Demo
Screenshot showing the mobile app unfurling both WebP and JPEG image URLs (please, ignore UI inconsistencies).
Important
Typetocommon.message.LinkPreviewand a validation to check it exists. There's no fallback type. In the mobile client, I had to make a tiny change, which will become a PR (to be added here).user_messages.unfurled_links). This is a known pain point ever since URL unfurling (initial implementation) #3471 was opened. Given that "unfurling" is still evolving, I'd say it's wise to finally move away from the serialized column approach to a separate table. I'd be interested to work on this problem soon (and maybe finish it before the next desktop release lands).About compression
The compression strategy implemented in this PR leverages the existing function
images.CompressToFileLimits. I didn't implement a more comprehensive logic to consider the possibility of multiple image URLs being unfurled simultaneously, and the requirement that all of them fit within the target message size. For now, I believe it's good enough.About animated WebP
Quick note about animated WebP: if you try to copy raw Giphy URLs, very often you'll get animated WebP resources, and not true GIF ones, for example https://i.giphy.com/media/IaWMz9Ln8OWvf66z6k/giphy.webp. As far as I checked, we don't support animated WebP in status-go.
The requirements for GIFs are still under discussion (see #3821). Once all requirements are clear, we can come back to animated WebP URLs.