Render GitHub pull requests everywhere we render git commits#48183
Conversation
|
Wait just checking the links it generated I think they won't work lol 😮💨 Edit: fixed |
Bundle size report 📦
Look at the Statoscope report for a full comparison between the commits 81ca0c0 and e5912f4 or learn more. Open explanation
|
…t="blank" to external URL
| return match[1] | ||
| } | ||
|
|
||
| return url |
There was a problem hiding this comment.
That's the only part that seems "brittle" to me. If the URL doesn't match the pattern it doesn't necessarily mean that url is a GitHub repo URL. I guess the assumption is that the input is always a GitHub URL (and that's currently the case given how it's used) but who knows how someone else uses it in the future.
That said, I don't know if need stricter checks now (and what they would look like) but we should be aware of this.
There was a problem hiding this comment.
@fkling That's fair! Not sure if you saw this but we aoso have a check on the top though that looks at the ServiceKind exposed from the backend to detect if this resource is indeed hosted on GitHub:
const github = externalURLs ? externalURLs.find(url => url.serviceKind === ExternalServiceKind.GITHUB) : null
There was a problem hiding this comment.
I didn't pay attention to that.... in that case I'd probably implement githubRepoUrl to accept whatever type of object github is with the restriction {serviceKind: ExternalServiceKind.GITHUB} (if that's possible at all).
| rel: 'noreferrer noopener', | ||
| target: '_blank', |
There was a problem hiding this comment.
Is this not needed anymore? Why?
There was a problem hiding this comment.
Lol, that's only because I’m stupid. The nooper and target _blank should apply to the external URL only (the one that links to GitHub) but I accidentally applied it to the internal part (the one that links the Sorucegraph commit) where obviously this is not expected
There was a problem hiding this comment.
Oh and to add to the question: Yeah still needed but I moved it to the right place: https://github.com/sourcegraph/sourcegraph/pull/48183/files#diff-ac13ce26fc2f0c1e5cf927f5ffb553fe432bb5a2a6e57be8accbe4eb58352da4R60-R61
Fixed some leftover places that didn't show the git commit messages yet. Thank you for the feedback, @mrnugget.
It turns out this is evne easier because the
nodealready hadexternalURLsso I can even delete code!Test plan
Update: The screenshots are slightly outdated, the URLs are fixed now!
App preview:
Check out the client app preview documentation to learn more.