sturdy-dev / codereview.gpt

Reviews your Pull/Merge Requests using ChatGPT
MIT License
529 stars 63 forks source link

Adding support for Self Hosted GitLab and remove an unnecessary http request as we run scripts on the active tab for detection purposes already #24

Closed nickveenhof closed 1 year ago

nickveenhof commented 1 year ago

Fixes #15

  1. Removes the fetch of the current tab and replaces this with script injection to fetch the required elements.
    1. For GitLab this is
      • a metatag to detect whether the website is GitLab (SaaS or Self-Hosted)
      • the description or the MR.
    2. For GitHub this is the description of the PR.
  2. Adds permissions for the chrome extension to run on all websites, as we do not have a list of self-hosted GitLab instances
  3. Fixes error message that did not appear when ran on websites that were not compatible.
krlvi commented 1 year ago

Looks good. I kind of wanted to keep the permissions as narrow as possible though. Maybe there's a better way of solving this? For the host_permissions I was thinking we could use this optional_host_permissions

Are like regular host_permissions, but are granted by the extension's user at runtime, rather than in advance.

https://developer.chrome.com/docs/extensions/mv3/declare_permissions/

(for a nicer UX we could pick a the host from the URL and ask for confirmation)

The scripting part, I guess there might not be a better way of doing this

nickveenhof commented 1 year ago

Looks good. I kind of wanted to keep the permissions as narrow as possible though. Maybe there's a better way of solving this? For the host_permissions I was thinking we could use this optional_host_permissions

Agree. Let's change it so that github.com & gitlab.com are added by default, and the rest as optional.

nickveenhof commented 1 year ago

Updated the permissions. Validated it still works as expected

The scripting part, I guess there might not be a better way of doing this

Probably there is a cleaner way to order the code, but that would make is less readable. I am not the best javascript programmer, so hopefully someone in the community can help us clean this code. 🤞