Avoid stalls caused by large diffs fetched from GitHub

lib/containers/comment-decorations-container.js

@@ -126,7 +126,7 @@ export default class CommentDecorationsContainer extends React.Component {
           environment={environment}
           query={query}
           variables={variables}
-          render={queryResult => this.renderWithGraphQLData({
+          render={queryResult => this.renderWithPullRequest({
             endpoint,
             owner: variables.headOwner,
             repo: variables.headName,
@@ -137,7 +137,7 @@ export default class CommentDecorationsContainer extends React.Component {
     );
   }
 
-  renderWithGraphQLData({error, props, endpoint, owner, repo}, {repoData, token}) {
+  renderWithPullRequest({error, props, endpoint, owner, repo}, {repoData, token}) {
     if (error) {
       // eslint-disable-next-line no-console
       console.warn(`error fetching CommentDecorationsContainer data: ${error}`);
@@ -154,60 +154,70 @@ export default class CommentDecorationsContainer extends React.Component {
     }
 
     const currentPullRequest = props.repository.ref.associatedPullRequests.nodes[0];
-    const queryProps = {currentPullRequest, ...props};
+
+    return (
+      <AggregatedReviewsContainer
+        pullRequest={currentPullRequest}
+        reportRelayError={this.props.reportRelayError}>
+        {({errors, summaries, commentThreads}) => {
+          return this.renderWithReviews(
+            {errors, summaries, commentThreads},
+            {currentPullRequest, repoResult: props, endpoint, owner, repo, repoData, token},
+          );
+        }}
+      </AggregatedReviewsContainer>
+    );
+  }
+
+  renderWithReviews(
+    {errors, summaries, commentThreads},
+    {currentPullRequest, repoResult, endpoint, owner, repo, repoData, token},
+  ) {
+    if (errors && errors.length > 0) {
+      // eslint-disable-next-line no-console
+      console.warn('Errors aggregating reviews and comments for current pull request', ...errors);
+      return null;
+    }
+
+    if (commentThreads.length === 0) {
+      return null;
+    }
 
     return (
       <PullRequestPatchContainer
         owner={owner}
         repo={repo}
         number={currentPullRequest.number}
         endpoint={endpoint}
-        token={token}>
+        token={token}
+        largeDiffThreshold={Infinity}>
         {(patchError, patch) => this.renderWithPatch(
           {error: patchError, patch},
-          {queryProps, endpoint, owner, repo, repoData},
+          {summaries, commentThreads, currentPullRequest, repoResult, endpoint, owner, repo, repoData, token},
         )}
       </PullRequestPatchContainer>
     );
   }
 
-  renderWithPatch({error, patch}, {queryProps, endpoint, owner, repo, repoData}) {
+  renderWithPatch(
+    {error, patch},
+    {summaries, commentThreads, currentPullRequest, repoResult, endpoint, owner, repo, repoData, token},
+  ) {
     if (error) {
       // eslint-disable-next-line no-console
       console.warn('Error fetching patch for current pull request', error);
       return null;
     }
 
-    return (
-      <AggregatedReviewsContainer
-        pullRequest={queryProps.currentPullRequest}
-        reportRelayError={this.props.reportRelayError}>
-        {({errors, summaries, commentThreads}) => {
-          if (errors && errors.length > 0) {
-            // eslint-disable-next-line no-console
-            console.warn('Errors aggregating reviews and comments for current pull request', ...errors);
-            return null;
-          }
-
-          const aggregationResult = {summaries, commentThreads};
-          return this.renderWithResult(aggregationResult, {
-            queryProps, endpoint, owner, repo, repoData, patch,
-          });
-        }}
-      </AggregatedReviewsContainer>
-    );
-  }
-
-  renderWithResult(aggregationResult, {queryProps, endpoint, owner, repo, repoData, patch}) {
     if (!patch) {
       return null;
     }
 
     return (
       <CommentPositioningContainer
         multiFilePatch={patch}
-        commentThreads={aggregationResult.commentThreads}
-        prCommitSha={queryProps.currentPullRequest.headRefOid}
+        commentThreads={commentThreads}
+        prCommitSha={currentPullRequest.headRefOid}
         localRepository={this.props.localRepository}
         workdir={repoData.workingDirectoryPath}>
         {commentTranslations => {
@@ -223,9 +233,9 @@ export default class CommentDecorationsContainer extends React.Component {
               workspace={this.props.workspace}
               commands={this.props.commands}
               repoData={repoData}
-              commentThreads={aggregationResult.commentThreads}
+              commentThreads={commentThreads}
               commentTranslations={commentTranslations}
-              pullRequests={queryProps.repository.ref.associatedPullRequests.nodes}
+              pullRequests={repoResult.repository.ref.associatedPullRequests.nodes}
             />
           );
         }}

lib/containers/comment-positioning-container.js

@@ -121,6 +121,7 @@ class FileTranslation {
 
     this.rawPositions = new Set();
     this.diffToFilePosition = new Map();
+    this.removed = false;
     this.fileTranslations = null;
     this.digest = null;
 
@@ -147,12 +148,26 @@ class FileTranslation {
 
   update({multiFilePatch, diffs, diffPositionFn, translatePositionFn}) {
     const filePatch = multiFilePatch.getPatchForPath(this.nativeRelPath);
-    // if there's a comment on a file that used to exist in a pr but does not exist,
-    // the filePatch won't exist.  Don't even try.
+    // Comment on a file that used to exist in a PR but no longer does. Skip silently.
     if (!filePatch) {
+      this.diffToFilePosition = new Map();
+      this.removed = false;
+      this.fileTranslations = null;
+
+      return;
+    }
+
+    // This comment was left on a file that was too large to parse.
+    if (!filePatch.getRenderStatus().isVisible()) {
+      this.diffToFilePosition = new Map();
+      this.removed = true;
+      this.fileTranslations = null;
+
       return;
     }
+
     this.diffToFilePosition = diffPositionFn(this.rawPositions, filePatch.getRawContentPatch());
+    this.removed = false;
 
     let contentChangeDiff;
     if (diffs.length === 1) {

lib/containers/pr-patch-container.js

@@ -4,6 +4,7 @@ import {parse as parseDiff} from 'what-the-diff';
 
 import {toNativePathSep} from '../helpers';
 import {EndpointPropType} from '../prop-types';
+import {filter as filterDiff} from '../models/patch/filter';
 import {buildMultiFilePatch} from '../models/patch';
 
 export default class PullRequestPatchContainer extends React.Component {
@@ -17,8 +18,9 @@ export default class PullRequestPatchContainer extends React.Component {
     endpoint: EndpointPropType.isRequired,
     token: PropTypes.string.isRequired,
 
-    // Refetch diff on next component update
+    // Fetching and parsing
     refetch: PropTypes.bool,
+    largeDiffThreshold: PropTypes.number,
 
     // Render prop. Called with (error or null, multiFilePatch or null)
     children: PropTypes.func.isRequired,
@@ -27,17 +29,26 @@ export default class PullRequestPatchContainer extends React.Component {
   state = {
     multiFilePatch: null,
     error: null,
+    last: {url: null, patch: null, etag: null},
   }
 
   componentDidMount() {
     this.mounted = true;
-    this.fetchDiff();
+    this.fetchDiff(this.state.last);
   }
 
   componentDidUpdate(prevProps) {
-    if (this.props.refetch && !prevProps.refetch) {
-      this.setState({multiFilePatch: null, error: null});
-      this.fetchDiff();
+    const explicitRefetch = this.props.refetch && !prevProps.refetch;
+    const requestedURLChange = this.state.last.url !== this.getDiffURL();
+
+    if (explicitRefetch || requestedURLChange) {
+      const {last} = this.state;
+      this.setState({
+        multiFilePatch: null,
+        error: null,
+        last: {url: this.getDiffURL(), patch: null, etag: null},
+      });
+      this.fetchDiff(last);
     }
   }
 
@@ -56,7 +67,8 @@ export default class PullRequestPatchContainer extends React.Component {
   }
 
   buildPatch(rawDiff) {
-    const diffs = parseDiff(rawDiff).map(diff => {
+    const {filtered, removed} = filterDiff(rawDiff);
+    const diffs = parseDiff(filtered).map(diff => {
     // diff coming from API will have the defaul git diff prefixes a/ and b/ and use *nix-style / path separators.
     // e.g. a/dir/file1.js and b/dir/file2.js
     // see https://git-scm.com/docs/git-diff#_generating_patches_with_p
@@ -66,42 +78,76 @@ export default class PullRequestPatchContainer extends React.Component {
         oldPath: diff.oldPath ? toNativePathSep(diff.oldPath.replace(/^[a|b]\//, '')) : diff.oldPath,
       };
     });
-    return buildMultiFilePatch(diffs, {preserveOriginal: true});
+    const options = {
+      preserveOriginal: true,
+      removed,
+    };
+    if (this.props.largeDiffThreshold) {
+      options.largeDiffThreshold = this.props.largeDiffThreshold;
+    }
+    const mfp = buildMultiFilePatch(diffs, options);
+    return mfp;
   }
 
-  async fetchDiff() {
+  async fetchDiff(last) {
     const url = this.getDiffURL();
     let response;
 
     try {
-      response = await fetch(url, {
-        headers: {
-          Accept: 'application/vnd.github.v3.diff',
-          Authorization: `bearer ${this.props.token}`,
-        },
-      });
+      const headers = {
+        Accept: 'application/vnd.github.v3.diff',
+        Authorization: `bearer ${this.props.token}`,
+      };
+
+      if (url === last.url && last.etag !== null) {
+        headers['If-None-Match'] = last.etag;
+      }
+
+      response = await fetch(url, {headers});
     } catch (err) {
       return this.reportDiffError(`Network error encountered fetching the patch: ${err.message}.`, err);
     }
 
+    if (response.status === 304) {
+      // Not modified.
+      if (!this.mounted) {
+        return null;
+      }
+
+      return new Promise(resolve => this.setState({
+        multiFilePatch: last.patch,
+        error: null,
+        last,
+      }));
+    }
+
     if (!response.ok) {
       return this.reportDiffError(`Unable to fetch the diff for this pull request: ${response.statusText}.`);
     }
 
     try {
+      const etag = response.headers.get('ETag');
       const rawDiff = await response.text();
       if (!this.mounted) {
         return null;
       }
 
       const multiFilePatch = this.buildPatch(rawDiff);
-      return new Promise(resolve => this.setState({multiFilePatch}, resolve));
+      return new Promise(resolve => this.setState({
+        multiFilePatch,
+        error: null,
+        last: {url, patch: multiFilePatch, etag},
+      }, resolve));
     } catch (err) {
       return this.reportDiffError('Unable to parse the diff for this pull request.', err);
     }
   }
 
   reportDiffError(message, error) {
+    if (!this.mounted) {
+      return null;
+    }
+
     return new Promise(resolve => {
       if (error) {
         // eslint-disable-next-line no-console

lib/containers/reviews-container.js

@@ -90,7 +90,8 @@ export default class ReviewsContainer extends React.Component {
         repo={this.props.repo}
         number={this.props.number}
         endpoint={this.props.endpoint}
-        token={token}>
+        token={token}
+        largeDiffThreshold={Infinity}>
         {(error, patch) => this.renderWithPatch(error, {token, patch})}
       </PullRequestPatchContainer>
     );

lib/controllers/editor-comment-decorations-controller.js

@@ -3,8 +3,10 @@ import PropTypes from 'prop-types';
 import {Range} from 'atom';
 
 import {EndpointPropType} from '../prop-types';
+import {addEvent} from '../reporter-proxy';
 import Marker from '../atom/marker';
 import Decoration from '../atom/decoration';
+import ReviewsItem from '../items/reviews-item';
 import CommentGutterDecorationController from '../controllers/comment-gutter-decoration-controller';
 
 export default class EditorCommentDecorationsController extends React.Component {
@@ -29,6 +31,7 @@ export default class EditorCommentDecorationsController extends React.Component
       fileTranslations: PropTypes.shape({
         get: PropTypes.func.isRequired,
       }),
+      removed: PropTypes.bool.isRequired,
       digest: PropTypes.string,
     }),
   }
@@ -48,6 +51,32 @@ export default class EditorCommentDecorationsController extends React.Component
       return null;
     }
 
+    if (this.props.commentTranslationsForPath.removed && this.props.threadsForPath.length > 0) {
+      const [firstThread] = this.props.threadsForPath;
+
+      return (
+        <Marker
+          editor={this.props.editor}
+          exclusive={true}
+          invalidate="surround"
+          bufferRange={Range.fromObject([[0, 0], [0, 0]])}>
+
+          <Decoration type="block" editor={this.props.editor} className="github-EditorComment-omitted">
+            <p>
+              This file has review comments, but its patch is too large for Atom to load.
+            </p>
+            <p>
+              Review comments may still be viewed within
+              <button
+                className="btn"
+                onClick={() => this.openReviewThread(firstThread.threadID)}>the review tab</button>.
+            </p>
+          </Decoration>
+
+        </Marker>
+      );
+    }
+
     return this.props.threadsForPath.map(thread => {
       const range = this.getRangeForThread(thread);
       if (!range) {
@@ -123,6 +152,19 @@ export default class EditorCommentDecorationsController extends React.Component
     }
     return localRange;
   }
+
+  openReviewThread = async threadId => {
+    const uri = ReviewsItem.buildURI({
+      host: this.props.endpoint.getHost(),
+      owner: this.props.owner,
+      repo: this.props.repo,
+      number: this.props.number,
+      workdir: this.props.workdir,
+    });
+    const reviewsItem = await this.props.workspace.open(uri, {searchAllPanes: true});
+    reviewsItem.jumpToThread(threadId);
+    addEvent('open-review-thread', {package: 'github', from: this.constructor.name});
+  }
 }
 
 function translationDigestFrom(props) {