[GH-670] Fix issue: Code previews not working for branches

raghavaggarwal2308 commented 6 months ago

Summary

Update code to show code preview for permalink with branch names

What to test?

Getting code preview for permalinks with branch names

Steps to reproduce:

Connect your GitHub account
Open a file on a GitHub repo
Click on any line number in the file
Copy the browser URL
Post the link in the channel

Expected behavior:

You should get a preview of the code near that permalink.

Ticket Link

Fixes #670

hanzei commented 6 months ago

@raghavaggarwal2308 Can you please share a example permalink that now works?

raghavaggarwal2308 commented 6 months ago

@raghavaggarwal2308 Can you please share a example permalink that now works?

@hanzei Here is an example: https://github.com/mattermost/mattermost/blob/master/server/public/Makefile#L7

raghavaggarwal2308 commented 5 months ago

The feature doesn't seem to work with branches that are not master. e.g. https://github.com/mattermost/mattermost-plugin-github/blob/playwright-shared-repo/.gitmodules#L2 doesn't create a preview.

Can you please check what is going on?

@hanzei I checked and found about the regex we are using to compare with the link and extract data is not accepting characters other than alphabets and numbers in the branch name. We can add any character in the branch on Github including /, due to which its very difficult to write a regex for this as the URL also contains forward slashes.
The only solution we could think for this is to make some trade of and no support some characters like \ here. We can mentions the same in the readme that the permalink preview will not work for branch names containing these characters. Please let me your thoughts on this

mickmister commented 4 months ago

@raghavaggarwal2308 Let's support dashes and underscores here. It's okay if we don't support slashes because of the complications. We'll want to make sure there is no UX impact when the branch has a slash though. It should be as if it's not a link to a file, rather than failing to load it

Kshitij-Katiyar commented 3 months ago

@raghavaggarwal2308 Let's support dashes and underscores here. It's okay if we don't support slashes because of the complications. We'll want to make sure there is no UX impact when the branch has a slash though. It should be as if it's not a link to a file, rather than failing to load it

@mickmister Added the support for hyphen, Hope it aligns with your suggestion.

Kshitij-Katiyar commented 3 months ago

The feature doesn't seem to work with branches that are not master. e.g. https://github.com/mattermost/mattermost-plugin-github/blob/playwright-shared-repo/.gitmodules#L2 doesn't create a preview.

Can you please check what is going on?

@hanzei @mickmister Working fine now.

mickmister commented 3 months ago

@raghavaggarwal2308 Let's support dashes and underscores here. It's okay if we don't support slashes because of the complications. We'll want to make sure there is no UX impact when the branch has a slash though. It should be as if it's not a link to a file, rather than failing to load it

@mickmister Added the support for hyphen, Hope it aligns with your suggestion.

@Kshitij-Katiyar I think underscores are commonly used in branch names too. Can we support branch names with underscores? Does it support capital letters as well? Maybe a unit test that tests using them together

Kshitij-Katiyar commented 3 months ago

@raghavaggarwal2308 Let's support dashes and underscores here. It's okay if we don't support slashes because of the complications. We'll want to make sure there is no UX impact when the branch has a slash though. It should be as if it's not a link to a file, rather than failing to load it

@mickmister Added the support for hyphen, Hope it aligns with your suggestion.

@Kshitij-Katiyar I think underscores are commonly used in branch names too. Can we support branch names with underscores? Does it support capital letters as well? Maybe a unit test that tests using them together

@raghavaggarwal2308 Let's support dashes and underscores here. It's okay if we don't support slashes because of the complications. We'll want to make sure there is no UX impact when the branch has a slash though. It should be as if it's not a link to a file, rather than failing to load it

@mickmister Added the support for hyphen, Hope it aligns with your suggestion.

@Kshitij-Katiyar I think underscores are commonly used in branch names too. Can we support branch names with underscores? Does it support capital letters as well? Maybe a unit test that tests using them together

@mickmister the regex now in use supports lowercase, uppercase, numeric, hyphen and underscore characters. Updated the existing testcase for code preview with branch name to use a branch with name including all the supported characters. Let me know if we want to add another testcase or update the existing one.(as done)

mattermost / mattermost-plugin-github