feature: Unfurl status links #4033
Conversation
Jenkins BuildsClick to see older builds (144)
|
igor-sirotin
force-pushed
the
feat/issue-3762-status-unfurler
branch
2 times, most recently
from
9942a95 to
cfdfac5
Compare
igor-sirotin
force-pushed
the
feat/issue-3762-status-unfurler
branch
from
f8bab25 to
6dba927
Compare
igor-sirotin
added
the
E:New Unfurling API
|
It's ready to review, I will fix the tests meanwhile. Probably it's linter issues only. |
|
I missed to implement the message editing, which was kindly caught by UPD: fixed. |
igor-sirotin
force-pushed
the
feat/issue-3762-status-unfurler
branch
2 times, most recently
from
2f45bdb to
196cf80
Compare
| @@ -1157,7 +1157,7 @@ func (api *PublicAPI) GetTextURLs(text string) []string { | |||
| // be removed from the response. | |||
| // | |||
| // This endpoint expects the client to send URLs normalized by GetTextURLs. | |||
| func (api *PublicAPI) UnfurlURLs(urls []string) ([]common.LinkPreview, error) { | |||
| func (api *PublicAPI) UnfurlURLs(urls []string) (protocol.UnfurlURLsResponse, error) { | |||
There was a problem hiding this comment.
Just a reminder that this is a breaking change for the mobile client. Before merging this PR it's important to create a mobile PR that is reviewed, tested, and ready to be merged before you merge your PR.
I can implement the changes in status-mobile for us since it's trivial, but it will need to go over QA, and that can take some time at the moment.
There was a problem hiding this comment.
FYI: @igor-sirotin, I have a mobile branch with the fix for the new response UnfurlURLsResponse. I'll open the mobile PR as soon as this status-go PR is finished.
| @@ -0,0 +1,2 @@ | |||
| ALTER TABLE user_messages ADD COLUMN unfurled_status_links BLOB; | |||
There was a problem hiding this comment.
Just sharing, nothing to do now, but I still fear status links have far more reasons to change than the relatively stable API of normal unfurled links, which could mean painful migrations. But since the user_messages table should never have its data migrated (according to our past discussions) then things can get tricky in the future.
A part of me still thinks this more "fragile" blob should be captured in a separate table, with explicit columns, although the join could affect the performance (or maybe not).
Don't worry, I'm just rambling here.
There was a problem hiding this comment.
I agree.
Actually there's a good news that we're going to update it on receiver side anyway. Only IDs are required to fetch the data. So we could migrate it later to some simple table, where we have IDs as separate columns, but all other data as blob 🤔
|
@ilmotta, thank you very much for your review and detailed comments, I appreciate that ❤️ I decided to refactor things on the way, therefore you'll find some new separate files. Finnaly, as discussed, I will update the unfurling logic in this PR. |
igor-sirotin
force-pushed
the
feat/issue-3762-status-unfurler
branch
from
87729a2 to
b726e65
Compare
Thank you @igor-sirotin. I'm going to review today again. |
✔️ status-go/prs/ios/PR-4033#26 🔹 ~3 min 39 sec 🔹 04468f7 🔹 📦 ios package |
|
Pushed the change:
|
There was a problem hiding this comment.
Great work in this PR @igor-sirotin. And thank you for organizing meetings to openly discuss the features.
Since the code in this PR is relatively stable now, I'll move forward with the mobile PR today (Friday) and get it checked by QAs. The mobile QA team is quite busy at the moment. I'll keep you posted when it's approved so you can merge this one.
|
@igor-sirotin hello! Could you please rebase current branch? We need it for proper testing of correspondent mobile PR status-im/status-mobile#17573 Thank you! |
igor-sirotin
force-pushed
the
feat/issue-3762-status-unfurler
branch
from
e3c47db to
1323dc4
Compare
|
@pavloburykh sure, done 👌 |
thank you @igor-sirotin! |
igor-sirotin
force-pushed
the
feat/issue-3762-status-unfurler
branch
from
3911890 to
0c50f6a
Compare
igor-sirotin
force-pushed
the
feat/issue-3762-status-unfurler
branch
from
0c50f6a to
157e2de
Compare
|
🎉 |
…n status-go (#17573) Adapt the JSON RPC response to the new shape returned by `wakuext_unfurlURLs` on v0.170.0. The changes were introduced PR status-im/status-go#4033. There are no behavioral changes in the API as far as mobile is concerned at the moment.
closes #3762
desktop PR: status-im/status-desktop#12303
This PR contains #4110. Will remove before merging.
Description
This PR introduces support for unfurling Status contact/community/channel links.
Storing the data
As we discussed, status links are unfurled in a separate database column.
So
UnfurlURLswill now return aUnfurlURLsResponsewith both link types:status-go/protocol/messenger_linkpreview.go
Lines 19 to 22 in 70ed74b
And a separate field is added to
Message:status-go/protocol/protobuf/chat_message.proto
Line 219 in 70ed74b
Contained data
The preview itself contains more data than the original encoded URL.
We add more data (e.g. icons). if it's available locally in the database, this is required for the expected designs.
We could also fetch data from Waku, if it's not available locally, but that might require more time and eventually might fail Which would be a strange behaviour, having that there's some data simply encoded in the URL. We would need some special mechanism of "supplementing the preview":
This is much more to do, but it's a not a bad idea I think.
Minor updates to shared urls
@MishkaRogachev adding you as reviewer for this part
status-go/protocol/messenger_share_urls.go
Lines 43 to 47 in 70ed74b
IsStatusSharedUrlmethodstatus-go/protocol/messenger_share_urls.go
Lines 533 to 535 in 70ed74b