feat: Add internal link preview for communities #18484
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Jenkins BuildsClick to see older builds (182)
|
ibrkhalil
changed the title
ibrkhalil
commented
Jan 23, 2024
| (filter (fn [preview] | ||
| (not (:status-link-preview? preview))))) |
There was a problem hiding this comment.
Have to clear status link previews coupling here to prevent status-go failure to unfurl the URLs.
There was a problem hiding this comment.
Same suggestion from another comment of mine. You could extract the code to build :link-previews outside build-text-message. Same for the code to build status-link-previews.
I think the function build-text-message is too long with the changes being brought by this PR.
| (filter (fn [preview] | ||
| (not (:status-link-preview? preview))))) |
There was a problem hiding this comment.
Coupling removal here too.
ibrkhalil
requested review from
Parveshdhull,
smohamedjavid,
alwx and
ajayesivan
flexsurfer
reviewed
Jan 31, 2024
There was a problem hiding this comment.
thanks @ibrkhalil for the PR, i have one question why do we build and send status link from react? what's the point?
src/status_im/subs/chats.cljs
Outdated
| :chats/status-link-previews-unfurled | ||
| :<- [:chat/status-link-previews] | ||
| (fn [previews] | ||
| (get previews :unfurled))) |
| (set/rename-keys thumbnail {:data-uri :dataUri})))) | ||
| (image->rpc thumbnail)))) | ||
|
|
||
| (defn- status-link-preview->rpc |
There was a problem hiding this comment.
how is it different from ->status-link-previews-rpc ?
There was a problem hiding this comment.
Sorry forgot and duplicated it, Fixed.
| @@ -16,6 +16,30 @@ | |||
| :community-id :communityId | |||
| :clock-value :clock}))) | |||
|
|
|||
| (defn- <-status-link-previews-rpc | |||
There was a problem hiding this comment.
To convert the object of the received message from camelCase to kebab-case.
| @@ -2,6 +2,7 @@ | |||
| (:require [clojure.string :as string] | |||
| [legacy.status-im.chat.models.mentions :as mentions] | |||
| [legacy.status-im.data-store.messages :as data-store-messages] | |||
| [legacy.status-im.data-store.messages :as data-store.messages] | |||
| :status-link-preview?])) | ||
| (filter (fn [preview] | ||
| (not (:status-link-preview? preview))))) | ||
| :status-link-previews (map (fn [{{:keys [community-id color description display-name |
There was a problem hiding this comment.
why do we build status link here ?
There was a problem hiding this comment.
Because status-go expects the body of the message to send to have status-link-previews attached to it, Just like we do for normal link previews.
There was a problem hiding this comment.
but why, what's the point? for the normal link, we do it because of privacy, but why we want this for status link?
There was a problem hiding this comment.
The StatusGo side was built in a way that it expects the message body to contain status-link-preview data
Maybe @igor-sirotin can tell us why he built it this way?
There was a problem hiding this comment.
This is done :
- For sake of consistency
- To enable immediate showing the contact/community/channel preview to the recipient. Not to wait while it's being fetched from waku.
clauxx
approved these changes
Jan 31, 2024
| (let [status-link-previews (rf/sub [:chats/message-status-link-previews chat-id message-id]) | ||
| link-previews? (rf/sub [:chats/message-link-previews? chat-id message-id])] | ||
| (when (seq status-link-previews) | ||
| [rn/view {:style {:margin-top (when link-previews? 8)}} |
There was a problem hiding this comment.
why status-link-previews margin depends on link-previews?
There was a problem hiding this comment.
Because we're rendering them on top of each other
And if there was a link preview existing we need to push the community ones down a bit.
ajayesivan
approved these changes
Feb 7, 2024
alwx
approved these changes
Feb 12, 2024
ibrkhalil
force-pushed
the
add-internal-link-preview
branch
from
8615bac to
ed8b7cf
Compare
46% of end-end tests have passedFailed tests (25)Click to expandClass TestActivityMultipleDevicePRTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestCommunityMultipleDeviceMerged:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestActivityMultipleDevicePR:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Expected to fail tests (1)Click to expandClass TestCommunityOneDeviceMerged:
Passed tests (22)Click to expandClass TestCommunityOneDeviceMerged:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestActivityMultipleDevicePRTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestCommunityMultipleDeviceMerged:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestDeepLinksOneDevice:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestActivityMultipleDevicePR:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
|
ilmotta
reviewed
Feb 16, 2024
src/status_im/contexts/chat/messenger/composer/link_preview/view.cljs
Outdated
Show resolved
Hide resolved
src/status_im/contexts/chat/messenger/composer/link_preview/events.cljs
Outdated
Show resolved
Hide resolved
src/status_im/contexts/chat/messenger/composer/link_preview/events.cljs
Outdated
Show resolved
Hide resolved
| (filter (fn [preview] | ||
| (not (:status-link-preview? preview))))) |
There was a problem hiding this comment.
Same suggestion from another comment of mine. You could extract the code to build :link-previews outside build-text-message. Same for the code to build status-link-previews.
I think the function build-text-message is too long with the changes being brought by this PR.
0% of end-end tests have passedFailed tests (47)Click to expandClass TestActivityMultipleDevicePR:
Class TestCommunityOneDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestCommunityMultipleDeviceMerged:
Class TestActivityMultipleDevicePRTwo:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestDeepLinksOneDevice:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Expected to fail tests (1)Click to expandClass TestCommunityOneDeviceMerged:
|
|
ISSUE 1: messages in channel are overlapped by link preview Steps
Actual result: Messages are overlapped
Expected result: Messages looks as designed 
|
|
Current PR is not related to #17552 |
Fixed according to Mario's reply here |
ibrkhalil
force-pushed
the
add-internal-link-preview
branch
from
532f29a to
0a5cceb
Compare
ibrkhalil
changed the title
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
fixes #17954
Summary
Uses internal link preview component to show community link preview
Had to do a bit of coupling to make us of the mechanism that was already built by @ilmotta for link previews
QA NOTES:
Please also check if this issue #17552 is reproducible in this PR. If not - please close that issue.
status: ready