6 ms·

Gitlab has been pretty great so far, but the code review tooling is so poor I think I need to look elsewhere. The pull request pages take forever to load, they

by teen 6y ago

Gitlab has been pretty great so far, but the code review tooling is so poor I think I need to look elsewhere. The pull request pages take forever to load, they can't handle pull requests with over 800 lines well at all. It will collapse files with over 100-200 lines of changes for seemingly no reason (hiding the comments within them). You can't ignore whitespace in diffs, and the comments disappear from the diff if they push new commits, you need to look at them in the overview again.

malikolivier 6y ago
I can relate. We use GitLab at our company. I still think the merge request UI is great. Its main problems are its slowness and the fact that files with a long diff are collapsed. As the full merge request is quite slow, I will usually open the diff of each commit one by one, then put review comments as a "per commit" basis.
- andr3 6y ago
  Thanks for the comment. I've left a few replies in this thread already with more detail of work we have done, is currently in flight and work that is planned to keep improving the overall situation. As to the "per commit" review, we're working on something to make this navigation easier and easier and shipping it very soon. https://gitlab.com/gitlab-org/gitlab/-/merge_requests/28596 https://gitlab.com/gitlab-org/gitlab/-/merge_requests/28596 As to the collapsed, unfortunately if we loaded all diff lines expanded, the large changesets would make the browser unusable. We're working to lower the memory footprint and have better ways to deal with these large MRs. One example I can leave you with that we're trying to figure out a way to make this useful as an additional option, is this: * File by file mode of reviewing MRs, including the concept of "unread diffs": https://gitlab.com/groups/gitlab-org/-/epics/516 https://gitlab.com/groups/gitlab-org/-/epics/516 We're aware of several of the pain points (we use GitLab to build GitLab ;) ), but if you have further feedback, please leave a comment in the issues and epics I've left here (or create new ones) and feel free to tag me @andr3. Thanks!
jbaiter 6y ago
Yeah, this is definitely one of GitLab's weakest points at the moment and seriously impacts day-to-day work here as well. Simple features like ticking off files you've already reviewed are missing and even mid-sized MRs are a pain to review even on a beefy machine.
- erowtom 6y ago
  This feature already exists, I use it all the time.
  - jbaiter 6y ago
    In GitLab? Maybe we're using an outdated GitLab version here, but I can't find it. And it doesn't seem to be mentioned in the offical Reviewing documentation: https://docs.gitlab.com/ce/user/project/merge_requests/reviewing_and_managing_merge_requests.html\ https://docs.gitlab.com/ce/user/project/merge_requests/revie... If you mean the "collapsing" feature, this is really not equivalent, since the state does not persist between multiple review sessions.
    - buttersbrian 6y ago
      I don't have it either. I wish we could "check" things off. I also wish I had more ability to do settings "by group", rather than by project.
      - samanthalee233 6y ago
        Community Advocate at GitLab here, it'd be great if you can add this feedback to this issue: https://gitlab.com/gitlab-org/gitlab/-/issues/24629 https://gitlab.com/gitlab-org/gitlab/-/issues/24629 Thanks!
- andr3 6y ago
  Hi! I'm an Engineering Manager at GitLab in Source Code team. This is on our horizon. We have an Epic to track an overall revamp and adding an option to review file-by-file which will include developing something like what you just described. Epic: https://gitlab.com/groups/gitlab-org/-/epics/516 https://gitlab.com/groups/gitlab-org/-/epics/516 Issue for the checking of file seen (including the concept of "unread diffs"): https://gitlab.com/gitlab-org/gitlab/-/issues/24629 https://gitlab.com/gitlab-org/gitlab/-/issues/24629 If you could please add your thoughts in that issue, we could use that feedback while we develop the feature. Thanks a bunch for caring and voicing your concerns. It helps us get better.
still_grokking 6y ago
This sounds as it it would be a great improvement over Bitbucket. :-)
papaf 6y ago
In my experience, if you are reviewing 800+ line PRs you should be in an IDE. I believe the average human being can not review an 800 line diff reliably. Such large PRs would also probably benefit from live discussions over video (or in the good old days -- in person).
- sa46 6y ago
  Interesting, I’ve only went through the effort of checking out a pull request for review a couple times. What’s the workflow like? Any tips for reducing friction? I avoid checking out PRs because it’s much higher friction and because I trust CI to catch most bugs so the crappy code forge interfaces are good enough. Personally, I would kill for a one click to step review interface in IntelliJ that synced comments back to the review.
  - 3001 6y ago
    I use gitlens extension on Visual studio.Pretty great so far.
- teen 6y ago
  I've reviewed 4000 line MRs without problems using other tools. It is possible.
mcintyre1994 6y ago
Here's a little script I use occasionally, it forces the browser to download all hidden files, and to display them all: document.querySelectorAll('.js-file-title').forEach(t => t.click()); document.querySelectorAll('.diff-content').forEach(d => d.style.display="block"); document.querySelectorAll('.nothing-here-block').forEach(d => d.style.display="none");
Aeolun 6y ago
I don’t understand. This describes basically every code review tool I’ve ever used. If there’s something aside from Gitlab that I should try that still integrates, I’d love to know.
- gen220 6y ago
  For huge diffs, there’s not much that beats `git checkout <branch> && git diff HEAD^`. For leaving comments, pretty much every system I’ve used has issues. At work we use phabricator; the default settings are atrocious but they can be tuned to something workable. I sometimes wonder how hard it would be to get diff reviews to be a first (or even second) class citizen of the git ecosystem. That way you can finally get the bliss of leaving review comments in the IDE. They already have “notes” which are vaguely similar, maybe it’s something they’re considering?
  - bostik 6y ago
    > [git ecosystem] have “notes” which are vaguely similar, maybe it’s something they’re considering? If you're referring to git-notes, they are ... kinda special. They are objects in their own namespace that reference the repository objects the notes themselves are attached to. Sure enough, you can attach a note to any git-object. And you can view them either directly or from the git-log view. But the discoverability is lacking, and the UI of doing pretty much anything with git-notes is absolutely atrocious. Because they are in their own namespace, you don't automatically get them when you do a pull or fetch, which can get pretty confusing. For the curious ones, the magic invocation needed to pull in updates to notes from the remote repo is: git fetch origin +refs/notes/*:refs/notes/* Notes are not versioned, and you certainly don't get nice N-way merges between local and remote content. Semantics are different from regular git, and stuff can get lost. So from where I'm looking at things, git-notes are not suitable for plumbing in a code review flow.
    - gen220 6y ago
      You're right, I was referring to git-notes and they aren't suitable as plumbing for code review :) I was meaning to point out that we've developed plumbing for objects with "weaker" guarantees than source code, which can still be attached to refs. To me, this is indicative that the maintainers are willing to accept patches on things that are outside the "core mission" of versioning source code, and might mean that they're open to the idea of accepting a well-architected code review feature. Although, you're also right, in that there's still a lot of plumbing to make sure that authors can only edit their own comments, and that sort of thing. The more I think about it, there's a lot of work to be done and it isn't easy. But, if you made a proposal, I think git-notes is precedent for them accepting your contributions. :)
andr3 6y ago
Hi, Engineering Manager at GitLab here, Source Code team. Thanks for the comment. Indeed that is something we've had in our minds for a long time. Over the past months/years, we have been shipping out incremental improvements in the way we handle and render large MR diffs. Recently we made the loading happen in batches which resulted in a significant improvement in how fast we start showing the diffs. https://gitlab.com/groups/gitlab-org/-/epics/1816 https://gitlab.com/groups/gitlab-org/-/epics/1816 We plan to continue improving the performance of the MR page, lowering memory footprint this Q2 as well as providing other ways to enhance the experience. Some examples: * Commit navigation, to help commit by commit review: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/28596 https://gitlab.com/gitlab-org/gitlab/-/merge_requests/28596 * File by file mode of reviewing MRs, including the concept of "unread diffs": https://gitlab.com/groups/gitlab-org/-/epics/516 https://gitlab.com/groups/gitlab-org/-/epics/516 We're constantly looking to improve and learn more about the struggles of our users. If you have further thoughts, please drop a comment in one of the issues and epics above and tag me (@andr3). If some of your problems are not covered by these, feel free to open an issue (https://gitlab.com/gitlab-org/gitlab https://gitlab.com/gitlab-org/gitlab) and tag me as well. Again, thanks for the feedback.
blyry 6y ago
> It will collapse files with over 100-200 lines of changes for seemingly no reason (hiding the comments within them) OH MY GOD this so much. This one specific anti-feature has no doubt caused countless bugs to be missed during code review. I can quote the issue thread by memory at this point i've revisited it so many times the last year(s?) For goodness sake, if the trade off is between 'seeing all of the changes during the review' vs 'having the page load quickly', the only people who choose 'quickly' are those who haven't been burned by human error missing a collapsed diff. Gitlab made the wrong trade off here in good/fast/cheap triangle.

by teen 6y ago

malikolivier 6y ago

I can relate. We use GitLab at our company. I still think the merge request UI is great. Its main problems are its slowness and the fact that files with a long diff are collapsed. As the full merge request is quite slow, I will usually open the diff of each commit one by one, then put review comments as a "per commit" basis.

andr3 6y ago
Thanks for the comment. I've left a few replies in this thread already with more detail of work we have done, is currently in flight and work that is planned to keep improving the overall situation. As to the "per commit" review, we're working on something to make this navigation easier and easier and shipping it very soon. https://gitlab.com/gitlab-org/gitlab/-/merge_requests/28596 https://gitlab.com/gitlab-org/gitlab/-/merge_requests/28596 As to the collapsed, unfortunately if we loaded all diff lines expanded, the large changesets would make the browser unusable. We're working to lower the memory footprint and have better ways to deal with these large MRs. One example I can leave you with that we're trying to figure out a way to make this useful as an additional option, is this: * File by file mode of reviewing MRs, including the concept of "unread diffs": https://gitlab.com/groups/gitlab-org/-/epics/516 https://gitlab.com/groups/gitlab-org/-/epics/516 We're aware of several of the pain points (we use GitLab to build GitLab ;) ), but if you have further feedback, please leave a comment in the issues and epics I've left here (or create new ones) and feel free to tag me @andr3. Thanks!

jbaiter 6y ago

Yeah, this is definitely one of GitLab's weakest points at the moment and seriously impacts day-to-day work here as well. Simple features like ticking off files you've already reviewed are missing and even mid-sized MRs are a pain to review even on a beefy machine.

erowtom 6y ago
This feature already exists, I use it all the time.
- jbaiter 6y ago
  In GitLab? Maybe we're using an outdated GitLab version here, but I can't find it. And it doesn't seem to be mentioned in the offical Reviewing documentation: https://docs.gitlab.com/ce/user/project/merge_requests/reviewing_and_managing_merge_requests.html\ https://docs.gitlab.com/ce/user/project/merge_requests/revie... If you mean the "collapsing" feature, this is really not equivalent, since the state does not persist between multiple review sessions.
  - buttersbrian 6y ago
    I don't have it either. I wish we could "check" things off. I also wish I had more ability to do settings "by group", rather than by project.
    - samanthalee233 6y ago
      Community Advocate at GitLab here, it'd be great if you can add this feedback to this issue: https://gitlab.com/gitlab-org/gitlab/-/issues/24629 https://gitlab.com/gitlab-org/gitlab/-/issues/24629 Thanks!
andr3 6y ago
Hi! I'm an Engineering Manager at GitLab in Source Code team. This is on our horizon. We have an Epic to track an overall revamp and adding an option to review file-by-file which will include developing something like what you just described. Epic: https://gitlab.com/groups/gitlab-org/-/epics/516 https://gitlab.com/groups/gitlab-org/-/epics/516 Issue for the checking of file seen (including the concept of "unread diffs"): https://gitlab.com/gitlab-org/gitlab/-/issues/24629 https://gitlab.com/gitlab-org/gitlab/-/issues/24629 If you could please add your thoughts in that issue, we could use that feedback while we develop the feature. Thanks a bunch for caring and voicing your concerns. It helps us get better.

still_grokking 6y ago

This sounds as it it would be a great improvement over Bitbucket. :-)

papaf 6y ago

In my experience, if you are reviewing 800+ line PRs you should be in an IDE. I believe the average human being can not review an 800 line diff reliably. Such large PRs would also probably benefit from live discussions over video (or in the good old days -- in person).

sa46 6y ago
Interesting, I’ve only went through the effort of checking out a pull request for review a couple times. What’s the workflow like? Any tips for reducing friction? I avoid checking out PRs because it’s much higher friction and because I trust CI to catch most bugs so the crappy code forge interfaces are good enough. Personally, I would kill for a one click to step review interface in IntelliJ that synced comments back to the review.
- 3001 6y ago
  I use gitlens extension on Visual studio.Pretty great so far.
teen 6y ago
I've reviewed 4000 line MRs without problems using other tools. It is possible.

mcintyre1994 6y ago

Here's a little script I use occasionally, it forces the browser to download all hidden files, and to display them all: document.querySelectorAll('.js-file-title').forEach(t => t.click()); document.querySelectorAll('.diff-content').forEach(d => d.style.display="block"); document.querySelectorAll('.nothing-here-block').forEach(d => d.style.display="none");

Aeolun 6y ago

I don’t understand. This describes basically every code review tool I’ve ever used. If there’s something aside from Gitlab that I should try that still integrates, I’d love to know.

gen220 6y ago
For huge diffs, there’s not much that beats `git checkout <branch> && git diff HEAD^`. For leaving comments, pretty much every system I’ve used has issues. At work we use phabricator; the default settings are atrocious but they can be tuned to something workable. I sometimes wonder how hard it would be to get diff reviews to be a first (or even second) class citizen of the git ecosystem. That way you can finally get the bliss of leaving review comments in the IDE. They already have “notes” which are vaguely similar, maybe it’s something they’re considering?
- bostik 6y ago
  > [git ecosystem] have “notes” which are vaguely similar, maybe it’s something they’re considering? If you're referring to git-notes, they are ... kinda special. They are objects in their own namespace that reference the repository objects the notes themselves are attached to. Sure enough, you can attach a note to any git-object. And you can view them either directly or from the git-log view. But the discoverability is lacking, and the UI of doing pretty much anything with git-notes is absolutely atrocious. Because they are in their own namespace, you don't automatically get them when you do a pull or fetch, which can get pretty confusing. For the curious ones, the magic invocation needed to pull in updates to notes from the remote repo is: git fetch origin +refs/notes/*:refs/notes/* Notes are not versioned, and you certainly don't get nice N-way merges between local and remote content. Semantics are different from regular git, and stuff can get lost. So from where I'm looking at things, git-notes are not suitable for plumbing in a code review flow.
  - gen220 6y ago
    You're right, I was referring to git-notes and they aren't suitable as plumbing for code review :) I was meaning to point out that we've developed plumbing for objects with "weaker" guarantees than source code, which can still be attached to refs. To me, this is indicative that the maintainers are willing to accept patches on things that are outside the "core mission" of versioning source code, and might mean that they're open to the idea of accepting a well-architected code review feature. Although, you're also right, in that there's still a lot of plumbing to make sure that authors can only edit their own comments, and that sort of thing. The more I think about it, there's a lot of work to be done and it isn't easy. But, if you made a proposal, I think git-notes is precedent for them accepting your contributions. :)

andr3 6y ago

Hi, Engineering Manager at GitLab here, Source Code team. Thanks for the comment. Indeed that is something we've had in our minds for a long time. Over the past months/years, we have been shipping out incremental improvements in the way we handle and render large MR diffs. Recently we made the loading happen in batches which resulted in a significant improvement in how fast we start showing the diffs. https://gitlab.com/groups/gitlab-org/-/epics/1816 https://gitlab.com/groups/gitlab-org/-/epics/1816 We plan to continue improving the performance of the MR page, lowering memory footprint this Q2 as well as providing other ways to enhance the experience. Some examples: * Commit navigation, to help commit by commit review: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/28596 https://gitlab.com/gitlab-org/gitlab/-/merge_requests/28596 * File by file mode of reviewing MRs, including the concept of "unread diffs": https://gitlab.com/groups/gitlab-org/-/epics/516 https://gitlab.com/groups/gitlab-org/-/epics/516 We're constantly looking to improve and learn more about the struggles of our users. If you have further thoughts, please drop a comment in one of the issues and epics above and tag me (@andr3). If some of your problems are not covered by these, feel free to open an issue (https://gitlab.com/gitlab-org/gitlab https://gitlab.com/gitlab-org/gitlab) and tag me as well. Again, thanks for the feedback.

blyry 6y ago

> It will collapse files with over 100-200 lines of changes for seemingly no reason (hiding the comments within them) OH MY GOD this so much. This one specific anti-feature has no doubt caused countless bugs to be missed during code review. I can quote the issue thread by memory at this point i've revisited it so many times the last year(s?) For goodness sake, if the trade off is between 'seeing all of the changes during the review' vs 'having the page load quickly', the only people who choose 'quickly' are those who haven't been burned by human error missing a collapsed diff. Gitlab made the wrong trade off here in good/fast/cheap triangle.