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

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

Open
wants to merge 13 commits into
base: master
Choose a base branch
from

Conversation

Sn-Kinos
Copy link

@Sn-Kinos Sn-Kinos commented May 10, 2024

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
image image

Ticket Link

@mattermost-build
Copy link
Contributor

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.

@Sn-Kinos Sn-Kinos changed the title Link preview [GH-730] Add link preview for PR and issue. May 10, 2024
@mickmister 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 Kshitij-Katiyar requested review from Kshitij-Katiyar and removed request for ayusht2810 May 14, 2024 11:34
Copy link
Contributor

Choose a reason for hiding this comment

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

@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 ?

Copy link
Author

Choose a reason for hiding this comment

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

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 🥲

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

@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 Outdated Show resolved Hide resolved
webapp/src/components/link_preview/link_preview.jsx Outdated Show resolved Hide resolved
webapp/src/utils/github_utils.js Outdated Show resolved Hide resolved
@mickmister
Copy link
Contributor

@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.

Copy link
Contributor

@mickmister mickmister left a comment

Choose a reason for hiding this comment

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

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

webapp/src/components/link_preview/preview.css Outdated Show resolved Hide resolved
Comment on lines 23 to 27
res = await Client.getIssue(owner, repo, number);
break;
case 'pull':
res = await Client.getPullRequest(owner, repo, number);
break;
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

It bypasses this and shows default embed preview like this.
image

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

Yes, I think it caused by this

Copy link
Contributor

Choose a reason for hiding this comment

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

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 Outdated Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

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

@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 Outdated Show resolved Hide resolved
webapp/src/components/link_preview/preview.css Outdated Show resolved Hide resolved
@@ -86,6 +86,7 @@

/* Labels */
.github-tooltip .labels {
display: flex;
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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

Before After
image image
.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 Outdated Show resolved Hide resolved
webapp/src/components/link_preview/link_preview.jsx Outdated Show resolved Hide resolved
webapp/src/utils/github_utils.js Outdated Show resolved Hide resolved
@matthewbirtch
Copy link

@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
Copy link
Author

@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.
image

@Sn-Kinos Sn-Kinos requested a review from mickmister May 20, 2024 02:16
@mickmister
Copy link
Contributor

@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.

Copy link
Contributor

@mickmister mickmister left a comment

Choose a reason for hiding this comment

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

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

Comment on lines 23 to 27
res = await Client.getIssue(owner, repo, number);
break;
case 'pull':
res = await Client.getPullRequest(owner, repo, number);
break;
Copy link
Contributor

Choose a reason for hiding this comment

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

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

@matthewbirtch
Copy link

@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.

@Sn-Kinos
Copy link
Author

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
Copy link
Author

Sn-Kinos commented May 22, 2024

@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.

@Sn-Kinos Sn-Kinos requested a review from mickmister May 22, 2024 01:48
Copy link
Contributor

@mickmister mickmister left a comment

Choose a reason for hiding this comment

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

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

@Sn-Kinos
Copy link
Author

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.
image

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.

@Sn-Kinos Sn-Kinos requested a review from mickmister May 24, 2024 00:15
@mickmister
Copy link
Contributor

@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
Copy link
Author

@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
Copy link
Contributor

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
Copy link
Author

@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
Copy link
Contributor

@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
Copy link
Author

@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
Copy link
Contributor

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
Copy link
Author

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

@mickmister
Copy link
Contributor

@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
Copy link
Author

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

@mickmister
Copy link
Contributor

@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 hanzei requested a review from ayusht2810 June 27, 2024 11:05
Copy link
Contributor

@hanzei hanzei left a comment

Choose a reason for hiding this comment

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

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

@mattermost-build
Copy link
Contributor

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2: Dev Review Requires review by a core committer 3: QA Review Requires review by a QA tester Contributor Lifecycle/1:stale
Projects
None yet
6 participants