[GH-730] Add link preview for PR and issue. #779

Sn-Kinos · 2024-05-10T11:23:21Z

Summary

Make GitHub link preview for PR and issue. It can show private repository if OAuth connected.
Add display: flex on labels at link_tooltip

Screenshots

Denim	Onyx

Ticket Link

Fixes [Feature request] When linking to pull requests / issues on a private repository, show a preview instead of an error page #730

It had use flex-related property without flex. so I added `display: flex`

mattermost-build · 2024-05-10T11:23:24Z

Hello @Sn-Kinos,

Thanks for your pull request! A Core Committer will review your pull request soon. For code contributions, you can learn more about the review process here.

Kshitij-Katiyar · 2024-05-14T11:37:11Z

webapp/src/components/link_preview/index.jsx

@Sn-Kinos We are trying to move away from javascript to now TypeScript. Will it be possible to convert these files to typescript if possible ?

I want to convert it to TS from JS but It's hard because there are no fundamental types requiring domain knowledge (e.g. state type from Mattermost web app) and ESLint didn't works well (e.g. It can't import with relative path from baseUrl) I think It will need more best practices for contributors to convert to TS appropriately. There are only one TS file on webapp 🥲

We are working on providing a template for converting files from js to ts.
cc: @mickmister

@Sn-Kinos Can you please try to have the webapp/src/components/link_preview/link_preview.jsx file be typescript in particular?

@mickmister Yes I can. But I think I will need some templates for it. I need more informations of types such as the data from Client, state from redux, etc.

webapp/src/components/link_preview/link_preview.jsx

webapp/src/utils/github_utils.js

mickmister · 2024-05-17T16:18:32Z

@matthewbirtch I'm curious about your thoughts on this feature. It's essentially a clone of the link tooltip component in the form of a link embed preview. Do you think this duplication could be noisy or cause confusion? Note that the preview will only show for users that have their GitHub account connected.

mickmister

Excellent work @Sn-Kinos! Mostly LGTM, I have a few suggestions and some comments for discussion

webapp/src/components/link_preview/preview.css

mickmister · 2024-05-17T16:18:13Z

webapp/src/components/link_preview/link_preview.jsx

+                    res = await Client.getIssue(owner, repo, number);
+                    break;
+                case 'pull':
+                    res = await Client.getPullRequest(owner, repo, number);
+                    break;


We'll want to inject these as props instead of accessing the client directly in this component

Also I wonder what happens here if the user is connected, but doesn't have access to a private repo being linked here

It bypasses this and shows default embed preview like this.

I'm not sure I understand how that works. It shows the fallback because this component returns null in that case?

Yes, I think it caused by this

I meant the fact that the webapp handles this gracefully when the component ends up returning null. I don't think you can coordinate React components together like that

webapp/src/components/link_preview/link_preview.jsx

mickmister · 2024-05-17T16:23:27Z

webapp/src/components/link_preview/index.jsx

@Sn-Kinos Can you please try to have the webapp/src/components/link_preview/link_preview.jsx file be typescript in particular?

webapp/src/components/link_preview/preview.css

mickmister · 2024-05-17T16:26:41Z

webapp/src/components/link_tooltip/tooltip.css

@@ -86,6 +86,7 @@

 /* Labels */
 .github-tooltip .labels {
+    display: flex;


What effect does this have on the tooltip's styling?

.labels is not flex element so the properties below doesn't working. So I added it and flex works.

Before After

.labels = height: 23px; (auto) display: block;
( flex properties not working ) .labels = height: 20px; (auto) display: flex;
( flex properties working )

.labels > * = height: 20px; .labels > * = height: 20px;

webapp/src/utils/github_utils.js

webapp/src/components/link_preview/link_preview.jsx

webapp/src/utils/github_utils.js

matthewbirtch · 2024-05-17T18:11:57Z

@matthewbirtch I'm curious about your thoughts on this feature. It's essentially a clone of the link tooltip component in the form of a link embed preview. Do you think this duplication could be noisy or cause confusion? Note that the preview will only show for users that have their GitHub account connected.

@mickmister I definitely see this as redundant with the hover popover that's been added. In my mind we would need to choose one or the other. Or make it configurable which kind of preview to use in the workspace.

Sn-Kinos · 2024-05-20T01:18:41Z

@matthewbirtch I'm curious about your thoughts on this feature. It's essentially a clone of the link tooltip component in the form of a link embed preview. Do you think this duplication could be noisy or cause confusion? Note that the preview will only show for users that have their GitHub account connected.

@mickmister I definitely see this as redundant with the hover popover that's been added. In my mind we would need to choose one or the other. Or make it configurable which kind of preview to use in the workspace.

Without this feature, We can watch the preview like below on private repository. I thought it is uncomfortable, so I added this feature.

mickmister · 2024-05-21T19:24:36Z

@Sn-Kinos @matthewbirtch We can make this feature configurable per-user so they can choose for themselves. We would want to make it obvious that the user can configure these two features separately.

Also note that this feature will passively be making requests to GitHub potentially often compared to the link tooltip feature, which could have negative effects on the server in terms of performance.

On our community server, we use the autolink plugin which with our configuration of the plugin makes it so GitHub PR links are shortened and hyperlinked, which makes it so the webapp does not use the "embed" feature that this PR relies on in that case. So if I were to opt for the embed feature and not the tooltip, I wouldn't have the chance to use this feature that often on our server. Just something to point out since this could be the case for other servers as well.

mickmister

The current code LGTM, though I think we need to decide how feature is configured since it essentially clashes with the existing tooltip preview feature

webapp/src/components/link_embed_preview/link_embed_preview.jsx

mickmister · 2024-05-21T19:31:59Z

webapp/src/components/link_preview/link_preview.jsx

+                    res = await Client.getIssue(owner, repo, number);
+                    break;
+                case 'pull':
+                    res = await Client.getPullRequest(owner, repo, number);
+                    break;


I'm not sure I understand how that works. It shows the fallback because this component returns null in that case?

matthewbirtch · 2024-05-21T20:01:42Z

@Sn-Kinos @matthewbirtch We can make this feature configurable per-user so they can choose for themselves. We would want to make it obvious that the user can configure these two features separately.

@mickmister Rather than adding a user setting and increasing that complexity, I would suggest we go with a system-level setting in the admin console. That way everyone on the server sees things the same way. That can be very beneficial to ensure everyone has the same context.

prevent such as https://google.com/https://github.com/mattermost/mattermost-plugin-github/pull/768

Sn-Kinos · 2024-05-22T01:37:45Z

I'm not sure I understand how that works. It shows the fallback because this component returns null in that case?

That's what I think. I don't know deeply about the fundamentals of registerPostWillRenderEmbedComponent so I have referenced link_tooltip.

Sn-Kinos · 2024-05-22T01:44:00Z

@Sn-Kinos @matthewbirtch We can make this feature configurable per-user so they can choose for themselves. We would want to make it obvious that the user can configure these two features separately.

@mickmister Rather than adding a user setting and increasing that complexity, I would suggest we go with a system-level setting in the admin console. That way everyone on the server sees things the same way. That can be very beneficial to ensure everyone has the same context.

I think It is good to only work when EnablePrivateRepo is enabled. If It's disabled, this feature is redundant because the main issue of this PR is showing link preview of private repository.

mickmister

Great job on this @Sn-Kinos 👍

From our discussion, I have one remaining ask to add a plugin setting to toggle between this feature and the link tooltip. This will go in the plugin.json file in the root of the repository

Name: EnableLinkPreviewEmbeds
Description: When enabled, whenever a link to an issue or pull request is shared in a channel, the embed content will be loaded for the specific user viewing it. This allows users to view previews from private repositories using their own connected account. Note that enabling this also disables the "link tooltip" feature in the plugin.

Please let me know your thoughts on this @Sn-Kinos

webapp/src/components/link_embed_preview/embed_preview.scss

Sn-Kinos · 2024-05-23T01:14:03Z

Great job on this @Sn-Kinos 👍

From our discussion, I have one remaining ask to add a plugin setting to toggle between this feature and the link tooltip. This will go in the plugin.json file in the root of the repository

Name: EnableLinkPreviewEmbeds

Description: When enabled, whenever a link to an issue or pull request is shared in a channel, the embed content will be loaded for the specific user viewing it. This allows users to view previews from private repositories using their own connected account. Note that enabling this also disables the "link tooltip" feature in the plugin.

Please let me know your thoughts on this @Sn-Kinos

@mickmister @matthewbirtch I don't think it's very essential to add setting. This link_embed_preview is necessary for private repository only. If the link is public repo, It shows embed preview in a normal way.

I think we can handle it with the 'EnablePrivateRepo' setting because the reason for developing link_embed_preview is that the information of private repository is not shown in the existing embedded preview.

mickmister · 2024-05-24T15:26:14Z

@Sn-Kinos I discussed with @matthewbirtch offline, and we're thinking there should be a system console setting that allows the server to be configured for either the link tooltip or link embed. The rationale is that there is more functionality here than simply filling in the gaps for the missing open graph information for private repos, because here there's a rich display of information about the issue/PR with this feature, rather than the simple open graph preview that already shows for public repositories.

To give the system admin a choice to enable one or the other, we can use a radio setting like this one being used in the Jira plugin:
https://github.com/mattermost/mattermost-plugin-jira/blob/4b296a0c23d04f0575b4c86bc4c9f042137f52cc/plugin.json#L46

Please let me know your thoughts on this @Sn-Kinos

Sn-Kinos · 2024-05-27T02:28:30Z

@Sn-Kinos I discussed with @matthewbirtch offline, and we're thinking there should be a system console setting that allows the server to be configured for either the link tooltip or link embed. The rationale is that there is more functionality here than simply filling in the gaps for the missing open graph information for private repos, because here there's a rich display of information about the issue/PR with this feature, rather than the simple open graph preview that already shows for public repositories.

To give the system admin a choice to enable one or the other, we can use a radio setting like this one being used in the Jira plugin: https://github.com/mattermost/mattermost-plugin-jira/blob/4b296a0c23d04f0575b4c86bc4c9f042137f52cc/plugin.json#L46

Please let me know your thoughts on this @Sn-Kinos

I concerned Also note that this feature will passively be making requests to GitHub potentially often compared to the link tooltip feature, which could have negative effects on the server in terms of performance. on the comment. If It doesn't matter, It will be ok to add the settings.

mickmister · 2024-05-30T17:00:33Z

I concerned Also note that this feature will passively be making requests to GitHub potentially often compared to the link tooltip feature, which could have negative effects on the server in terms of performance. on the comment. If It doesn't matter, It will be ok to add the settings.

@Sn-Kinos The main reason we want to add the settings is the UX flow of toggling showing the issue/PR through tooltip or link embed preview. I'm not sure I understand the point you're making in your last comment here

Sn-Kinos · 2024-05-31T04:41:24Z

@Sn-Kinos The main reason we want to add the settings is the UX flow of toggling showing the issue/PR through tooltip or link embed preview. I'm not sure I understand the point you're making in your last comment here

I worried 'negative effects on the server in terms of performance' on the comment. If It doesn't matter, I'll be ok :D

mickmister · 2024-05-31T13:30:16Z

@Sn-Kinos I think the piece I'm not understanding is "If It doesn't matter". Are you saying "if the performance issues don't matter"? Or if something else doesn't matter? Sorry about this

The only blocker here is for the UX piece. I think the performance issue is probably not a real issue here

Sn-Kinos · 2024-05-31T15:39:47Z

@Sn-Kinos I think the piece I'm not understanding is "If It doesn't matter". Are you saying "if the performance issues don't matter"? Or if something else doesn't matter? Sorry about this

The only blocker here is for the UX piece. I think the performance issue is probably not a real issue here

Ahhhh You're right. I concerned about the performance issue. Now, I agree with you as you mentioned.

Sorry about the miscommunication problem. the translator that I used didn't work properly I presume.

mattermost-build · 2024-06-11T01:02:41Z

This PR has been automatically labelled "stale" because it hasn't had recent activity.
A core team member will check in on the status of the PR to help with questions.
Thank you for your contribution!

Sn-Kinos · 2024-06-11T01:31:05Z

I wonder if there's anything else I need to do 🏃‍♂️

mickmister · 2024-06-11T01:49:53Z

@Sn-Kinos Sorry for the confusing conversation here. There is one remaining requirement here to implement a plugin setting to toggle this and the link tooltip feature, as discussed here #779 (comment). Does this make sense?

Sn-Kinos · 2024-06-11T02:11:22Z

@mickmister I understand. What are the options I need to make?

mickmister · 2024-06-11T05:23:54Z

@Sn-Kinos We can have these two boolean plugin settings:

Enable Link Tooltip Default on - "When enabled, hovering over an issue or PR will show a tooltip preview of the issue/PR"
Enable Post Embed Preview Default off - "When enabled, links to issues/PRs will have a preview of the issue/PR shown within the post body."

hanzei

@ayusht2810 @Kshitij-Katiyar can you please review the PR?

mattermost-build · 2024-07-22T01:03:09Z

This PR has been automatically labelled "stale" because it hasn't had recent activity.
A core team member will check in on the status of the PR to help with questions.
Thank you for your contribution!

Sn-Kinos added 3 commits May 10, 2024 20:15

make the new file statement to new file isUrlCanPreview

96efd7d

fix css on tooltip labels

5036082

It had use flex-related property without flex. so I added `display: flex`

add link_preview component for pull and issue

df4623a

Sn-Kinos requested review from hanzei and mickmister as code owners May 10, 2024 11:23

mattermost-build added the Contributor label May 10, 2024

Sn-Kinos changed the title ~~Link preview~~ [GH-730] Add link preview for PR and issue. May 10, 2024

mickmister requested a review from ayusht2810 May 13, 2024 21:32

mickmister added 2: Dev Review Requires review by a core committer 3: QA Review Requires review by a QA tester labels May 13, 2024

Kshitij-Katiyar requested review from Kshitij-Katiyar and removed request for ayusht2810 May 14, 2024 11:34

Kshitij-Katiyar reviewed May 14, 2024

View reviewed changes

Sn-Kinos added 2 commits May 16, 2024 09:15

add array length check on statement

e04cb25

move color value to css

14c9e8b

Sn-Kinos requested a review from Kshitij-Katiyar May 17, 2024 00:08

mickmister reviewed May 17, 2024

View reviewed changes

Sn-Kinos added 4 commits May 20, 2024 10:22

link preview to link embed preview

7633e5b

make it ts and readable

802c360

early return on link_embed_preview

6e0a69f

fix eslint error

0dfcb28

Sn-Kinos requested a review from mickmister May 20, 2024 02:16

mickmister requested a review from AayushChaudhary0001 May 21, 2024 19:33

mickmister reviewed May 21, 2024

View reviewed changes

Sn-Kinos added 2 commits May 22, 2024 09:19

remove redundant comment

895ffce

fix the statement of isCanUrlPreview

f5f39bd

prevent such as https://google.com/https://github.com/mattermost/mattermost-plugin-github/pull/768

Sn-Kinos requested a review from mickmister May 22, 2024 01:48

convert css to scss

95630fd

mickmister reviewed May 22, 2024

View reviewed changes

webapp/src/components/link_embed_preview/embed_preview.scss Outdated Show resolved Hide resolved

remove var() on scss variables

dac4f1d

Sn-Kinos requested a review from mickmister May 24, 2024 00:15

mattermost-build added the Lifecycle/1:stale label Jun 11, 2024

hanzei requested a review from ayusht2810 June 27, 2024 11:05

hanzei reviewed Jun 27, 2024

View reviewed changes

hanzei removed the Lifecycle/1:stale label Jun 27, 2024

raghavaggarwal2308 requested a review from wiggin77 July 5, 2024 08:29

mickmister removed their request for review July 10, 2024 05:23

mattermost-build added the Lifecycle/1:stale label Jul 22, 2024

Before	After

`.labels = height: 23px; (auto) display: block;` ( flex properties not working )	`.labels = height: 20px; (auto) display: flex;` ( flex properties working )
`.labels > * = height: 20px;`	`.labels > * = height: 20px;`

[GH-730] Add link preview for PR and issue. #779

Are you sure you want to change the base?

[GH-730] Add link preview for PR and issue. #779

Conversation

Sn-Kinos commented May 10, 2024 • edited Loading

Summary

Screenshots

Ticket Link

mattermost-build commented May 10, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mickmister commented May 17, 2024

mickmister left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matthewbirtch commented May 17, 2024

Sn-Kinos commented May 20, 2024

mickmister commented May 21, 2024

mickmister left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matthewbirtch commented May 21, 2024

Sn-Kinos commented May 22, 2024

Sn-Kinos commented May 22, 2024 • edited Loading

mickmister left a comment

Choose a reason for hiding this comment

Sn-Kinos commented May 23, 2024

mickmister commented May 24, 2024

Sn-Kinos commented May 27, 2024

mickmister commented May 30, 2024

Sn-Kinos commented May 31, 2024

mickmister commented May 31, 2024

Sn-Kinos commented May 31, 2024

mattermost-build commented Jun 11, 2024

Sn-Kinos commented Jun 11, 2024

mickmister commented Jun 11, 2024

Sn-Kinos commented Jun 11, 2024

mickmister commented Jun 11, 2024

hanzei left a comment

Choose a reason for hiding this comment

mattermost-build commented Jul 22, 2024

Sn-Kinos commented May 10, 2024 •

edited

Loading

Sn-Kinos commented May 22, 2024 •

edited

Loading