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

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

* File by file mode of reviewing MRs, including the concept of "unread diffs": 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) and tag me as well.

Again, thanks for the feedback.

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.

  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");

Such large PRs would also probably benefit from live discussions over video (or in the good old days -- in person).

Personally, I would kill for a one click to step review interface in IntelliJ that synced comments back to the review.

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.

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

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

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!

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?

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/*

So from where I'm looking at things, git-notes are not suitable for plumbing in a code review flow.

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. :)

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

Issue for the checking of file seen (including the concept of "unread diffs"): 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.

If you mean the "collapsing" feature, this is really not equivalent, since the state does not persist between multiple review sessions.