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

Support new URL unfurling APIs #10863

Closed
jrainville opened this issue May 30, 2023 · 6 comments · Fixed by #11476
Closed

Support new URL unfurling APIs #10863

jrainville opened this issue May 30, 2023 · 6 comments · Fixed by #11476
Assignees
Labels
E:Desktop New Unfurling API Implementation of the new unfurling API for all links feature
Milestone

Comments

@jrainville
Copy link
Member

jrainville commented May 30, 2023

Description

The mobile team created a new API to get the unfurling data from URLs. Instead of having the receiver fetch the data from the websites, it's the sender that fetches it, then sends it with the message. It's way better for privacy, but it also enables more websites to be unfurled.

Here is the initial work with the new APIs: status-im/status-go#3471

// GetTextURLs parses text and returns a deduplicated and (somewhat) normalized
// slice of URLs. The returned URLs can be used as cache keys by clients.
func (api *PublicAPI) GetTextURLs(text string) []string {
	return linkpreview.GetURLs(text)
}

// UnfurlURLs uses a best-effort approach to unfurl each URL. Failed URLs will
// 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) {
	return api.service.messenger.UnfurlURLs(urls)

Here is the follow up PR that adds support to more hosts: status-im/status-go#3530

For full context, here is the mobile issue: status-im/status-mobile#15469
and follow up one: status-im/status-mobile#15918

If your need help, you can ask @ilmotta , who did an amazing job on this.


Steps

Step 1

So, the first step, is that when typing the message, when we detect a link in the input (with a debounce throttle), we call the new status-go API to get the link data.

Then, when we send the message, we need to add the data to the message payload. It should also already be supported on status-go.

Step 2

Moved to a separate issue:

Out of scope

Acceptance Criteria

  • The sender is now responsible for sending the link data. What this means:
    • We get the link data when creating message in the input, with a debounce
    • When the message is sent, the new data is attached to it and propagated to other users
  • The receivers no longer fetch the data themselves
    • The link will still show as normal (or not in the special cases of images only)
    • The receiver uses the data directly from the Message and updates its model
  • The unfurled image/whatever still shows as before (some unfurls might not be pretty depending on whether or not they need the new design)
@igor-sirotin
Copy link
Contributor

@jrainville
Do we need to render the fetched previews before sending? Do you know if we have any designs on that?
Mobile does that, according to the video from status-im/status-go#3471

@jrainville
Copy link
Member Author

Do we need to render the fetched previews before sending?

Not yet, we can add that in a later PR, it's related to "opting out of sending the individual link data, ie choosing which links to send data for (this will need the updated design)"

Do you know if we have any designs on that?

Not yet

@caybro
Copy link
Member

caybro commented Jul 7, 2023

Related UI task: #10585 (@igor-sirotin wanna take it too? ;)

@igor-sirotin
Copy link
Contributor

@caybro surely I could!
But let's ask @jrainville, maybe he has some other opinion 🙂

Whoever works on this, probably better to divide the task into:

  1. implement new UI components (with storybook, ofc 🤘)
  2. use the new components in the app with proper backend

The first one can already be started to work.
The second one probably better to keep until this issue (#10863) is finished. There're some types to be reused already.

@jrainville
Copy link
Member Author

That new unfurling component (#10585) is out of the scope of this issue. It's the point I list above

New design of the unfurling component (gonna be done by the UI team)

Since it's listed as 0.15, let's keep it that way for now.

@igor-sirotin
Copy link
Contributor

Moved step 2 to a separate issue:

@github-project-automation github-project-automation bot moved this from Code review to Done in Status Desktop/Mobile Board Jul 11, 2023
@igor-sirotin igor-sirotin moved this from Done to QA in Status Desktop/Mobile Board Jul 12, 2023
@anastasiyaig anastasiyaig moved this from QA to Done in Status Desktop/Mobile Board Jul 12, 2023
@jrainville jrainville added the E:Desktop New Unfurling API Implementation of the new unfurling API for all links label Jul 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
E:Desktop New Unfurling API Implementation of the new unfurling API for all links feature
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

5 participants