Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Avoid stalls caused by large diffs fetched from GitHub #2195

Merged
merged 20 commits into from Jul 16, 2019
Merged
Changes from 1 commit
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
0076e92
Use the ETag and If-None-Match headers to avoid reparsing the same diff
smashwilson Jul 4, 2019
b9ceab8
Green PullRequestPatchContainer tests
smashwilson Jul 5, 2019
770a3b7
Intelligently refetch when the diff URL changes
smashwilson Jul 5, 2019
7dbbfb3
Only fetch the PR patch when reviews have arrived with at least one t鈥
smashwilson Jul 5, 2019
0d35246
Function to filter out large and irrelevant patches
smashwilson Jul 5, 2019
4d33f23
Simplify filter
smashwilson Jul 5, 2019
e5ce163
Filter excessively large diffs before attempting to parse
smashwilson Jul 5, 2019
50daaa5
Remove unused argument and increase patch size limit
smashwilson Jul 5, 2019
f537082
Turns out all i have to update here is the comment
smashwilson Jul 5, 2019
145102c
Teach PullRequestPatchContainer to pass a largeDiffThreshold
smashwilson Jul 11, 2019
ef8db73
Suppress large patch hiding in components that won't render the full 鈥
smashwilson Jul 11, 2019
f0da9c5
Skip hidden patches
smashwilson Jul 11, 2019
98de9af
Pass paths of removed files to the MFP builder
smashwilson Jul 16, 2019
c0805e0
Rename TOO_LARGE to DEFERRED
smashwilson Jul 16, 2019
6d775a6
Render a "diff unavailable" message when the diff couldn't be parsed 鈥
smashwilson Jul 16, 2019
483b3e1
Construct hidden patches for removed files
smashwilson Jul 16, 2019
76958da
Set .removed on a FileTranslation for removed patches
smashwilson Jul 16, 2019
e30036d
Block decoration at the beginning of an editor with a removed patch
smashwilson Jul 16, 2019
85b1a7d
Style the "patch too large" message
smashwilson Jul 16, 2019
4712435
Filter at 1MB, not 10MB
smashwilson Jul 16, 2019
File filter...
Filter file types
Jump to鈥
Jump to file or symbol
Failed to load files and symbols.

Always

Just for now

Only fetch the PR patch when reviews have arrived with at least one t鈥

鈥read
  • Loading branch information...
smashwilson committed Jul 5, 2019
commit 7dbbfb3e67af664114e42161487c23143ae73c62
@@ -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,7 +154,34 @@ 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
@@ -165,49 +192,31 @@ export default class CommentDecorationsContainer extends React.Component {
token={token}>
{(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 +232,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}
/>
);
}}
@@ -41,6 +41,8 @@ describe('CommentDecorationsContainer', function() {
localRepository={localRepository}
loginModel={loginModel}
reportRelayError={() => {}}
commands={atomEnv.commands}
children={() => <div />}
{...overrideProps}
/>
);
@@ -217,7 +219,7 @@ describe('CommentDecorationsContainer', function() {
assert.isTrue(resultWrapper.isEmptyRender());
});

it('renders the PullRequestPatchContainer if result includes repository and ref', function() {
it('renders the AggregatedReviewsContainerContainer if result includes repository and ref', function() {
const props = queryBuilder(rootQuery)
.repository(r => {
r.ref(r0 => {
@@ -233,10 +235,10 @@ describe('CommentDecorationsContainer', function() {
const resultWrapper = tokenWrapper.find(QueryRenderer).renderProp('render')({
error: null, props, retry: () => {},
});
assert.lengthOf(resultWrapper.find(PullRequestPatchContainer), 1);
assert.lengthOf(resultWrapper.find(AggregatedReviewsContainer), 1);
});

it('renders nothing if patch cannot be fetched', function() {
it("renders nothing if there's an error aggregating reviews", function() {
const props = queryBuilder(rootQuery)
.repository(r => {
r.ref(r0 => {
@@ -252,13 +254,16 @@ describe('CommentDecorationsContainer', function() {
const resultWrapper = tokenWrapper.find(QueryRenderer).renderProp('render')({
error: null, props, retry: () => {},
});
const patchWrapper = resultWrapper.find(PullRequestPatchContainer).renderProp('children')(
new Error('oops'), null,
);
assert.isTrue(patchWrapper.isEmptyRender());
const reviewsWrapper = resultWrapper.find(AggregatedReviewsContainer).renderProp('children')({
errors: [new Error('ahhhh')],
summaries: [],
commentThreads: [],
});

assert.isTrue(reviewsWrapper.isEmptyRender());
});

it('aggregates reviews while the patch is loading', function() {
it('renders nothing if there are no review comment threads', function() {
const props = queryBuilder(rootQuery)
.repository(r => {
r.ref(r0 => {
@@ -274,10 +279,7 @@ describe('CommentDecorationsContainer', function() {
const resultWrapper = tokenWrapper.find(QueryRenderer).renderProp('render')({
error: null, props, retry: () => {},
});
const patchWrapper = resultWrapper.find(PullRequestPatchContainer).renderProp('children')(
null, null,
);
const reviewsWrapper = patchWrapper.find(AggregatedReviewsContainer).renderProp('children')({
const reviewsWrapper = resultWrapper.find(AggregatedReviewsContainer).renderProp('children')({
errors: [],
summaries: [],
commentThreads: [],
@@ -286,7 +288,7 @@ describe('CommentDecorationsContainer', function() {
assert.isTrue(reviewsWrapper.isEmptyRender());
});

it("renders nothing if there's an error aggregating reviews", function() {
it('loads the patch once there is at least one loaded review comment thread', function() {
const props = queryBuilder(rootQuery)
.repository(r => {
r.ref(r0 => {
@@ -297,20 +299,52 @@ describe('CommentDecorationsContainer', function() {
});
})
.build();
const patch = multiFilePatchBuilder().build();

const tokenWrapper = localRepoWrapper.find(ObserveModel).renderProp('children')('1234');
const resultWrapper = tokenWrapper.find(QueryRenderer).renderProp('render')({
error: null, props, retry: () => {},
});
const patchWrapper = resultWrapper.find(PullRequestPatchContainer).renderProp('children')(null, patch);
const reviewsWrapper = patchWrapper.find(AggregatedReviewsContainer).renderProp('children')({
errors: [new Error('ahhhh')],
const reviewsWrapper = resultWrapper.find(AggregatedReviewsContainer).renderProp('children')({
errors: [],
summaries: [],
commentThreads: [],
commentThreads: [
{thread: {id: 'thread0'}, comments: [{id: 'comment0', path: 'a.txt'}, {id: 'comment1', path: 'a.txt'}]},
],
});
const patchWrapper = reviewsWrapper.find(PullRequestPatchContainer).renderProp('children')(
null, null,
);

assert.isTrue(reviewsWrapper.isEmptyRender());
assert.isTrue(patchWrapper.isEmptyRender());
});

it('renders nothing if patch cannot be fetched', function() {
const props = queryBuilder(rootQuery)
.repository(r => {
r.ref(r0 => {
r0.associatedPullRequests(conn => {
conn.totalCount(1);
conn.addNode();
});
});
})
.build();

const tokenWrapper = localRepoWrapper.find(ObserveModel).renderProp('children')('1234');
const resultWrapper = tokenWrapper.find(QueryRenderer).renderProp('render')({
error: null, props, retry: () => {},
});
const reviewsWrapper = resultWrapper.find(AggregatedReviewsContainer).renderProp('children')({
errors: [],
summaries: [],
commentThreads: [
{thread: {id: 'thread0'}, comments: [{id: 'comment0', path: 'a.txt'}, {id: 'comment1', path: 'a.txt'}]},
],
});
const patchWrapper = reviewsWrapper.find(PullRequestPatchContainer).renderProp('children')(
new Error('oops'), null,
);
assert.isTrue(patchWrapper.isEmptyRender());
});

it('renders a CommentPositioningContainer when the patch and reviews arrive', function() {
@@ -330,14 +364,16 @@ describe('CommentDecorationsContainer', function() {
const resultWrapper = tokenWrapper.find(QueryRenderer).renderProp('render')({
error: null, props, retry: () => {},
});
const patchWrapper = resultWrapper.find(PullRequestPatchContainer).renderProp('children')(null, patch);
const reviewsWrapper = patchWrapper.find(AggregatedReviewsContainer).renderProp('children')({
const reviewsWrapper = resultWrapper.find(AggregatedReviewsContainer).renderProp('children')({
errors: [],
summaries: [],
commentThreads: [],
commentThreads: [
{thread: {id: 'thread0'}, comments: [{id: 'comment0', path: 'a.txt'}, {id: 'comment1', path: 'a.txt'}]},
],
});
const patchWrapper = reviewsWrapper.find(PullRequestPatchContainer).renderProp('children')(null, patch);

assert.isTrue(reviewsWrapper.find(CommentPositioningContainer).exists());
assert.isTrue(patchWrapper.find(CommentPositioningContainer).exists());
});

it('renders nothing while the comment positions are being calculated', function() {
@@ -357,14 +393,16 @@ describe('CommentDecorationsContainer', function() {
const resultWrapper = tokenWrapper.find(QueryRenderer).renderProp('render')({
error: null, props, retry: () => {},
});
const patchWrapper = resultWrapper.find(PullRequestPatchContainer).renderProp('children')(null, patch);
const reviewsWrapper = patchWrapper.find(AggregatedReviewsContainer).renderProp('children')({
const reviewsWrapper = resultWrapper.find(AggregatedReviewsContainer).renderProp('children')({
errors: [],
summaries: [],
commentThreads: [],
commentThreads: [
{thread: {id: 'thread0'}, comments: [{id: 'comment0', path: 'a.txt'}, {id: 'comment1', path: 'a.txt'}]},
],
});
const patchWrapper = reviewsWrapper.find(PullRequestPatchContainer).renderProp('children')(null, patch);

const positionedWrapper = reviewsWrapper.find(CommentPositioningContainer).renderProp('children')(null);
const positionedWrapper = patchWrapper.find(CommentPositioningContainer).renderProp('children')(null);
assert.isTrue(positionedWrapper.isEmptyRender());
});

@@ -385,15 +423,17 @@ describe('CommentDecorationsContainer', function() {
const resultWrapper = tokenWrapper.find(QueryRenderer).renderProp('render')({
error: null, props, retry: () => {},
});
const patchWrapper = resultWrapper.find(PullRequestPatchContainer).renderProp('children')(null, patch);
const reviewsWrapper = patchWrapper.find(AggregatedReviewsContainer).renderProp('children')({
const reviewsWrapper = resultWrapper.find(AggregatedReviewsContainer).renderProp('children')({
errors: [],
summaries: [],
commentThreads: [],
commentThreads: [
{thread: {id: 'thread0'}, comments: [{id: 'comment0', path: 'a.txt'}, {id: 'comment1', path: 'a.txt'}]},
],
});
const patchWrapper = reviewsWrapper.find(PullRequestPatchContainer).renderProp('children')(null, patch);

const translations = new Map();
const positionedWrapper = reviewsWrapper.find(CommentPositioningContainer).renderProp('children')(translations);
const positionedWrapper = patchWrapper.find(CommentPositioningContainer).renderProp('children')(translations);

const controller = positionedWrapper.find(CommentDecorationsController);
assert.strictEqual(controller.prop('commentTranslations'), translations);
ProTip! Use n and p to navigate between commits in a pull request.
You can鈥檛 perform that action at this time.