Review changes file-by-file in a merge request
Problem to solve
@joshabb writes:
...it is hard to browse the diffs in Gitlab. Gitlab makes this hard because it forces the user to review ALL files at once rather than present a simple text list of the files changed (non-bloated UI). If it could do this and then have a link to the diff (without all the UI of Gitlab so you can really focus on the change per file) then it could be better than these other tools. [Gerrit and Bitbucket]
Original description
Gitlab is a good tool. However, it lacks real review power of other review systems such as gerrit and bitbucket.
The main problem is it is hard to browse the diffs in Gitlab. Gitlab makes this hard because it forces the user to review ALL files at once rather than present a simple text list of the files changed (non-bloated UI). If it could do this and then have a link to the diff (without all the UI of Gitlab so you can really focus on the change per file) then it could be better than these other tools. As it is, Gitlab is weakest at code reviews but it is the single most important feature for our team. In the past, I have filed issues about how slow it is to review code because the changes take forever to load. All that was done to fix this is a load on demand collapse button but this was a band-aid to the problem. The real problem is forcing the user to see all the diffs at once. I realize that this may be nice for small changes or some may be used to doing reviews this way. However, when you get a large change the page is still REALLY slow (both scrolling and making comments)--I would say almost unusable for a code review tool.
Please add the feature like gerrit where it shows the list of files that were changed and the additions/deletions (number to each file). Each file then would be a link to the diff where you can comment on the code changes. This would allow the reviewer to capture the essence of the change in a snapshot and jump to the important files immediately (Ctrl+F should not have to be invoked to find changes to a specific file). An example from gerrit is the following: This kind of diff list would make reviews SO much easier to navigate and a whole lot faster.
Further details
The introduction of the file tree was an important iteration towards improving diff navigation, but it was merely the first step. As merge requests become larger navigation is still difficult. This has surfaced in feedback from customers and UX research.
- performance is poor for anything but small merge requests because so much data is loaded
- scroll-based navigation is not useful because the page is so long. Finding file boundaries using the mouse is very difficult
- keyboard-based navigation is possible by using page down, but it isn't possible to move to the top and bottom of a file easily because it is essentially a keyboard powered scroll event
- collapsed/incomplete diff to protect page performance means large files are automatically collapsed even thought these still need to be reviewed
See &516 (closed) for the full picture
Proposal
Implement an option to view merge requests diffs on a file by file basis.
- open the first file in file tree by default
- the merge request interface should only load the diff for this one file
- clicking on a different fill will open the diff for that file
- the merge request interface should load the diff for the file on demand
This will require:
- API for loading retrieving the diff for a specific file and corresponding discussions
- Can we re-use
<namespace>/<project>/merge_requests/<iid>/diff_for_path
for first iteration - Optimize and use GraphQL in later iteration?
- Can we re-use
- Support for direct linking to files
- Support for direct linking to discussions
- Support for moving between unresolved discussions across multiple files