Feature Request: Better tooling for large PRs

[DanL]

A frequent issue my teams and I run into is dealing with large PR reviews. The solution is usually to break it down into several smaller PRs, but that seems like a workaround for GitHub's PR review system not having adequate tooling for large changesets.

The closest solution I can think of would be giving authors the ability to group changes/files together, so you could easily delineate logically separate parts of a PR as belonging together. With this system, you could have a reviewer sign off on a subset of the PR, without feeling obligated to review absolutely everything.

[mpetitdant]

It feels to me that the developper should split the work in commits and this is what I logged in #5303
But I also agree the reviewer may not have the same mental model as the committer leading to frustration over the large reviews.

[iandunn]

👍🏻

IMO commits should be atomic, and only do "1 thing", in the same way that functions should only do 1 thing. ( How "1 thing" is defined is relative to the situation, of course )

I'd rather have 1 large PR than several interdependent ones, and I don't mind a larger PR if the commits are logical and organized. The problem for me is when there's a dozen "WIP" commits that can't be meaningfully viewed in isolation, but viewing all the changes files is overwhelming.

Related: #6089

[WORMSS]

We have a situation where we are migrating from cypress (due to them committing bad practices into their npm library), so migrated to playwright. This is "one piece of work"

[parhamfh]

In my experience the file overview can get a bit hard to navigate. For various reasons you can end up in a state where many files listed alphabetically might not be the ideal way to review those files, even with proper git commit discipline. (Example from Python code base would be src/ and tests/ being sorted separately while the contents of the modules are strongly correlated and perhaps mirrored)

I greatly appreciate the new tools that allow you to un/check files you've reviewed and a jump-to-file option, going further and allowing grouping would be even more great! 🤩

[nickofthyme]

I would agree this is not normal and in most cases PRs should be small. But in some cases like large refactoring (~3k file changes) it's very much needed. Moving, renaming files and paths can add up quick.

You may get away with reviewing all files and marking them as viewed and eventually GitHub will load things with maybe a few refreshes. What is harder, if not impossible, is trying to filter files on such large PRs, particularly for codeower files (~50 files of 3k). You would think this would be easier on GitHub to load less but it actually struggles more and often results in a failure to load altogether. This is true even when setting the url query params directly, not the via the UI, to trigger file filtering.

This is good example https://github.com/elastic/kibana/pull/202830/files

[tt-pkaminski]

One common use case our organization struggles with are pull requests that contain a large number of image diffs from our frontend snapshot testing library. This is especially common if we refactor a core component, or update to support a new version of iOS and have to regenerate all of our tests. These PRs are so large, viewing them on Github tends to just beachball and the tool becomes unusable.

I've always been surprised why this happens. It seems like the webpage is trying to load every single image / diff in the changeset.

Anyways, I'm looking for a workaround for this. Does the pull request plugin in VS Code offer a way to review these large changesets?

[nickofthyme]

100%, I wasn't even going to mention this because I think it's a different problem, although very related.

I've even gone so far as to create a chrome snippet to help me open and review these image diffs.

Expand to see gh-files-diff.js

This script, finds images file diffs, checks if they are open and clicks preview if needed. It batches the request in groups of 25 to not overwhelm GitHub, and ignores already open previews when running each batch.

I haven't used it in a while but I think it still works 😅

const wait = (n = 100) => new Promise((resolve) => setTimeout(resolve, n));

(async (fileElements, fileSuffix) => {
    console.log(`Opening ${fileElements.length} diffs`);
  const waitForImage = (fileElement) => {
    return new Promise(async (resolve) => {
      let loading = true;
      let iframeEl = null;

      while (loading) {
        iframeEl = fileElement.querySelector('.render-container iframe.render-viewer');
        const hasDeletedImage = fileElement.querySelector('.data.highlight.empty')?.innerText?.startsWith?.('Deleted file');

        loading = !iframeEl;
        // const loading = !!el.querySelector('.render-container .octospinner');

        if (loading) {
          await new Promise((r) => {
            setTimeout(r, 1000);
          });
        }
      }

      resolve();
    });
  };

  const loadImage = (fileElement) =>
    new Promise(async (resolve) => {
      const hasImage = !!fileElement.querySelector(`.file-info a[title$="${fileSuffix}"]`);

      if (!hasImage) {
        const imageSourceBtn = fileElement.querySelector('.js-source');
        const imagePreviewBtn = fileElement.querySelector('.js-rendered');

        if (imageSourceBtn) {
          imageSourceBtn.click();
          await wait();
        }

        if (imagePreviewBtn) {
          imagePreviewBtn.click();
          await waitForImage(fileElement);
        }
      }

      resolve();
    });

  let p = 25
  let i = 0;
  for (const element of fileElements) {
    const percent = (i++ / fileElements.length) * 100;
    if (percent > p) {
      console.log(`${p}% Complete`)
      p += 25;
    }
    await loadImage(element);
  }

  console.log(`100% Complete`)
})([...document.querySelectorAll('.js-file.open:not(:has(iframe))')].slice(0,25), '1-snap.png');

In the end I think the best solution is to have this comparison outside of GitHub but in the meantime, or for smaller diffs, it would be nice if this was improved.