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.

Sign up for GitHub

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

Jump to bottom

fix: avoid duplicating UnfurlURLs requests #12687

Merged
merged 2 commits into from
Nov 17, 2023
Merged

fix: avoid duplicating UnfurlURLs requests #12687

merged 2 commits into from
Nov 17, 2023

Conversation

igor-sirotin
Copy link
Contributor

What does the PR do

There was a bug, we were checking linkPreviewsCache for unknown URLs. But it's possible that we start editing the message before the unfurling response is received.

Now we also save the list of requested URLs and check it before making a new request.

It's possible to see from the logs that after the second setText there're no new URLs found.

DBG 2023-11-10 11:02:30.104+00:00 <<< setText                                tid=28511231 file=controller.nim:202
DBG 2023-11-10 11:02:30.105+00:00 NewBE_callPrivateRPC                       topics="rpc" tid=28511231 file=core.nim:27 rpc_method=wakuext_getTextURLs
DBG 2023-11-10 11:02:30.146+00:00 <<< newUrls                                tid=28511231 file=controller.nim:216 newUrls="@[\"https://github.com/status-im/status-go/pull/4280\", \"https://github.com/status-im/status-go/pull/4283\", \"https://github.com/status-im/status-desktop/pull/12675\", \"https://github.com/status-im/status-go/issues/4273\", \"https://github.com\"]"
DBG 2023-11-10 11:02:30.146+00:00 [threadpool task thread] initiating task   topics="task-threadpool" tid=28511416 file=threadpool.nim:54 messageType=AsyncUnfurlUrlsTaskArg:ObjectType threadid=28511416 task="{\"$type\":\"AsyncUnfurlUrlsTaskArg:ObjectType\",\"urls\":[\"https://github.com/status-im/status-go/pull/4280\",\"https://github.com/status-im/status-go/pull/4283\",\"https://github.com/status-im/status-desktop/pull/12675\",\"https://github.com/status-im/status-go/issues/4273\",\"https://github.com\"],\"vptr\":5006224608,\"slot\":\"onAsyncUnfurlUrlsFinished\",\"tptr\":4340491496}"
DBG 2023-11-10 11:02:30.151+00:00 NewBE_callPrivateRPC                       topics="rpc" tid=28511416 file=core.nim:27 rpc_method=wakuext_unfurlURLs
DBG 2023-11-10 11:02:30.421+00:00 <<< setText                                tid=28511231 file=controller.nim:202
DBG 2023-11-10 11:02:30.421+00:00 NewBE_callPrivateRPC                       topics="rpc" tid=28511231 file=core.nim:27 rpc_method=wakuext_getTextURLs
DBG 2023-11-10 11:02:30.422+00:00 <<< newUrls                                tid=28511231 file=controller.nim:216 newUrls=@[]
2023-11-10T11:02:32+0000 unset [github.com/anacrolix/torrent.(*Client).NewAnacrolixDhtServer.func2:406]: dht server on 0.0.0.0:52546 (node id d9ae7297adbf918cb7df465daaa98d2045c592c1) completed boots

@igor-sirotin igor-sirotin added the E:Desktop New Unfurling API Implementation of the new unfurling API for all links label Nov 10, 2023
@igor-sirotin igor-sirotin self-assigned this Nov 10, 2023
@status-im-auto
Copy link
Member

status-im-auto commented Nov 10, 2023 •
edited
Loading

Jenkins Builds

Click to see older builds (20)
❔ Commit #️⃣ Finished (UTC) Duration Platform Result
✔️ 2bdea1b #1 2023-11-10 11:15:11 ~6 min tests/nim 📄log
✔️ 2bdea1b #1 2023-11-10 11:15:41 ~7 min macos/aarch64 🍎dmg
✔️ 2bdea1b #1 2023-11-10 11:19:20 ~10 min tests/ui 📄log
✔️ 2bdea1b #1 2023-11-10 11:19:41 ~11 min macos/x86_64 🍎dmg
✔️ 2bdea1b #1 2023-11-10 11:25:22 ~16 min linux/x86_64 📦tgz
✔️ 2bdea1b #1 2023-11-10 11:34:50 ~26 min windows/x86_64 💿exe
✔️ 2bdea1b #1 2023-11-10 11:41:51 ~33 min tests/e2e 📄log
✔️ 9d1507b #2 2023-11-13 23:28:21 ~5 min macos/aarch64 🍎dmg
✔️ 9d1507b #2 2023-11-13 23:29:26 ~6 min tests/nim 📄log
✔️ 9d1507b #2 2023-11-13 23:32:05 ~8 min macos/x86_64 🍎dmg
✔️ 9d1507b #2 2023-11-13 23:34:19 ~11 min tests/ui 📄log
✔️ 9d1507b #2 2023-11-13 23:37:40 ~14 min linux/x86_64 📦tgz
✖️ 9d1507b #2 2023-11-13 23:55:33 ~32 min tests/e2e 📄log
✔️ 9d1507b #2 2023-11-13 23:57:30 ~34 min windows/x86_64 💿exe
✔️ 9d1507b #3 2023-11-14 12:13:25 ~7 min tests/nim 📄log
✔️ 9d1507b #3 2023-11-14 12:13:50 ~8 min macos/x86_64 🍎dmg
✔️ 9d1507b #3 2023-11-14 12:20:10 ~14 min tests/ui 📄log
✔️ 9d1507b #3 2023-11-14 12:22:54 ~17 min linux/x86_64 📦tgz
✔️ 9d1507b #3 2023-11-14 12:35:41 ~30 min windows/x86_64 💿exe
✔️ 9d1507b #3 2023-11-14 12:38:00 ~32 min tests/e2e 📄log
❔ Commit #️⃣ Finished (UTC) Duration Platform Result
✔️ 22530ae #5 2023-11-15 17:34:18 ~7 min macos/aarch64 🍎dmg
✔️ 22530ae #5 2023-11-15 17:34:24 ~7 min tests/nim 📄log
✔️ 22530ae #5 2023-11-15 17:37:47 ~10 min macos/x86_64 🍎dmg
✔️ 22530ae #5 2023-11-15 17:38:29 ~11 min tests/ui 📄log
✔️ 22530ae #5 2023-11-15 17:47:32 ~20 min linux/x86_64 📦tgz
✔️ 22530ae #5 2023-11-15 17:50:19 ~23 min windows/x86_64 💿exe
✔️ 22530ae #5 2023-11-15 18:04:44 ~37 min tests/e2e 📄log
✔️ 639c67f #6 2023-11-16 22:37:37 ~5 min tests/nim 📄log
✔️ 639c67f #6 2023-11-16 22:37:46 ~5 min macos/aarch64 🍎dmg
✔️ 639c67f #6 2023-11-16 22:41:01 ~8 min macos/x86_64 🍎dmg
✔️ 639c67f #6 2023-11-16 22:43:54 ~11 min tests/ui 📄log
✔️ 639c67f #6 2023-11-16 22:46:51 ~14 min linux/x86_64 📦tgz
✔️ 639c67f #6 2023-11-16 23:03:05 ~30 min tests/e2e 📄log
✔️ 639c67f #6 2023-11-16 23:03:13 ~30 min windows/x86_64 💿exe

@igor-sirotin igor-sirotin deleted the fix/avoid-duplicating-unfurling branch November 13, 2023 23:23
@igor-sirotin igor-sirotin restored the fix/avoid-duplicating-unfurling branch November 14, 2023 12:05
@igor-sirotin
Copy link
Contributor Author

oops, deleted the branch by accident

@igor-sirotin igor-sirotin reopened this Nov 14, 2023
@igor-sirotin igor-sirotin force-pushed the fix/avoid-duplicating-unfurling branch 2 times, most recently from 2bdea1b to 22530ae Compare November 15, 2023 17:26
@igor-sirotin igor-sirotin mentioned this pull request Nov 15, 2023
Merged
caybro
caybro approved these changes Nov 15, 2023
View reviewed changes
Copy link
Member

@caybro caybro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@igor-sirotin igor-sirotin merged commit 03d4fbc into master Nov 17, 2023
8 of 9 checks passed
@igor-sirotin igor-sirotin deleted the fix/avoid-duplicating-unfurling branch November 17, 2023 15:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@caybro caybro caybro approved these changes

@jrainville jrainville jrainville approved these changes

@alexjba alexjba Awaiting requested review from alexjba

Assignees

@igor-sirotin igor-sirotin

Labels
E:Desktop New Unfurling API Implementation of the new unfurling API for all links
Projects
Status Desktop/Mobile Board
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@igor-sirotin @status-im-auto @caybro @jrainville