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(@desktop/chat): handle invalid tenor gif link #9084

Closed
wants to merge 1 commit into from

Conversation

mprakhov
Copy link
Contributor

What does the PR do

The problem is that if you add an invalid tenor link, the tenor returns you 43 bytes gif image
Unfortunately, we can't understand if the gif provided is incorrect until we load it in Loader.

This is an ad-hoc fix which was communicated at the team meeting. In normal, verification should be done on the status-go side.

Fixes: #8560

Affected areas

Gif / Images animation

Screenshot of functionality (including design for comparison)

Screen.Recording.2023-01-12.at.16.13.16.mov

@mprakhov
Copy link
Contributor Author

mprakhov commented Jan 12, 2023

@jrainville adding you as you are in the context of the problem. That's the ad-hoc fix I was speaking about (which I do not like to be honest). Unfortunately, there is one big con - flickering. Will this work for us? In another case we need to do some verification on status-go side

@status-im-auto
Copy link
Member

status-im-auto commented Jan 12, 2023

Jenkins Builds

Commit #️⃣ Finished (UTC) Duration Platform Result
✔️ a35fc81 #2 2023-01-12 14:30:12 ~10 min macos 🍎dmg
✔️ a35fc81 #2 2023-01-12 14:33:58 ~14 min imports 📄log
✔️ a35fc81 #2 2023-01-12 14:35:35 ~15 min linux-cpp 📄log
✔️ a35fc81 #2 2023-01-12 14:37:11 ~17 min tests-nim 📄log
✔️ a35fc81 #2 2023-01-12 14:42:57 ~23 min linux 📦tgz
✔️ a35fc81 #2 2023-01-12 14:49:33 ~29 min windows 💿exe
✖️ a35fc81 #2 2023-01-12 14:56:15 ~36 min linux-e2e 📄log

@caybro
Copy link
Member

caybro commented Jan 12, 2023

Not a big fan of adding workarounds and more JS/imperative code here... :/

@caybro caybro requested a review from alexjba January 12, 2023 15:12
Copy link
Member

@jrainville jrainville left a comment

Choose a reason for hiding this comment

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

I don't mind the quick fix because it's a very low priority bug, but if people are not happy with that fix, I say let's just move the bug to the ice box. That issue doesn't deserve more time from us for now.

@@ -48,8 +48,18 @@ Column {

property bool loadingFailed: false

property bool unfurle: true
Copy link
Member

Choose a reason for hiding this comment

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

unfurle is not a valid word. Do you mean the verb "unfurl" or the adjective "unfurled"?

@alexjba
Copy link
Contributor

alexjba commented Jan 13, 2023

In normal, verification should be done on the status-go side.

Is it complicated to do it in status-go?
I'm all for fixing it as I find it annoying when scrolling.

@alexjba
Copy link
Contributor

alexjba commented Jan 13, 2023

@mprakhov Please consider we also have this task created #9101. If you have an idea on how to better process the urls in the backend and release the UI of this burden would be nice to hear it!

@mprakhov
Copy link
Contributor Author

After discussing, closing this MR, as we have a task to refactor ui/imports/shared/views/chat/LinksMessageView.qml component and move logic to status_go (#9101)

@mprakhov mprakhov closed this Jan 13, 2023
alexjba added a commit to status-im/status-go that referenced this pull request Feb 7, 2023
Motivated by: status-im/status-desktop#9084
While we don't request preview data from Tenor because the url itself will contain the GIF image, we still need to validate the url in the preview request. This is done using a HEAD request where we expect the response status code to be 200 OK.
At the same time we will add the content type to the preview response.
alexjba added a commit to status-im/status-go that referenced this pull request Feb 8, 2023
…3169)

Motivated by: status-im/status-desktop#9084
While we don't request preview data from Tenor because the url itself will contain the GIF image, we still need to validate the url in the preview request. This is done using a HEAD request where we expect the response status code to be 200 OK.
At the same time we will add the content type to the preview response.
@mprakhov mprakhov deleted the mp/fix/issue-8560 branch November 16, 2023 15:23
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.

Chat: the incorrect GIF address doesn't look good
6 participants