diff --git a/lib/containers/comment-decorations-container.js b/lib/containers/comment-decorations-container.js index 8e62cb7e5a..81fd521b90 100644 --- a/lib/containers/comment-decorations-container.js +++ b/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,7 +154,34 @@ export default class CommentDecorationsContainer extends React.Component { } const currentPullRequest = props.repository.ref.associatedPullRequests.nodes[0]; - const queryProps = {currentPullRequest, ...props}; + + return ( + + {({errors, summaries, commentThreads}) => { + return this.renderWithReviews( + {errors, summaries, commentThreads}, + {currentPullRequest, repoResult: props, endpoint, owner, repo, repoData, token}, + ); + }} + + ); + } + + 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 ( + 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}, )} ); } - 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 ( - - {({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, - }); - }} - - ); - } - - renderWithResult(aggregationResult, {queryProps, endpoint, owner, repo, repoData, patch}) { if (!patch) { return null; } @@ -206,8 +216,8 @@ export default class CommentDecorationsContainer extends React.Component { return ( {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} /> ); }} diff --git a/lib/containers/comment-positioning-container.js b/lib/containers/comment-positioning-container.js index 20c4ed866a..e33cdccb67 100644 --- a/lib/containers/comment-positioning-container.js +++ b/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) { diff --git a/lib/containers/pr-patch-container.js b/lib/containers/pr-patch-container.js index 67f3810382..51228addc4 100644 --- a/lib/containers/pr-patch-container.js +++ b/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 diff --git a/lib/containers/reviews-container.js b/lib/containers/reviews-container.js index 90fa732da4..b473d91c43 100644 --- a/lib/containers/reviews-container.js +++ b/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})} ); diff --git a/lib/controllers/editor-comment-decorations-controller.js b/lib/controllers/editor-comment-decorations-controller.js index 917f6d7666..1cfe855a6f 100644 --- a/lib/controllers/editor-comment-decorations-controller.js +++ b/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 ( + + + +

+ This file has review comments, but its patch is too large for Atom to load. +

+

+ Review comments may still be viewed within + . +

+ + + + ); + } + 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) { diff --git a/lib/models/patch/builder.js b/lib/models/patch/builder.js index dd3daab79d..63a9473c9e 100644 --- a/lib/models/patch/builder.js +++ b/lib/models/patch/builder.js @@ -1,7 +1,7 @@ import PatchBuffer from './patch-buffer'; import Hunk from './hunk'; import File, {nullFile} from './file'; -import Patch, {TOO_LARGE, EXPANDED} from './patch'; +import Patch, {DEFERRED, EXPANDED, REMOVED} from './patch'; import {Unchanged, Addition, Deletion, NoNewline} from './region'; import FilePatch from './file-patch'; import MultiFilePatch from './multi-file-patch'; @@ -10,7 +10,7 @@ export const DEFAULT_OPTIONS = { // Number of lines after which we consider the diff "large" largeDiffThreshold: 800, - // Map of file path (relative to repository root) to Patch render status (EXPANDED, COLLAPSED, TOO_LARGE) + // Map of file path (relative to repository root) to Patch render status (EXPANDED, COLLAPSED, DEFERRED) renderStatusOverrides: {}, // Existing patch buffer to render onto @@ -18,6 +18,9 @@ export const DEFAULT_OPTIONS = { // Store off what-the-diff file patch preserveOriginal: false, + + // Paths of file patches that have been removed from the patch before parsing + removed: new Set(), }; export function buildFilePatch(diffs, options) { @@ -89,6 +92,24 @@ export function buildMultiFilePatch(diffs, options) { // Delete the final trailing newline from the last non-empty patch. patchBuffer.deleteLastNewline(); + // Append hidden patches corresponding to each removed file. + for (const removedPath of opts.removed) { + const removedFile = new File({path: removedPath}); + const removedMarker = patchBuffer.markPosition( + Patch.layerName, + patchBuffer.getBuffer().getEndPosition(), + {invalidate: 'never', exclusive: false}, + ); + filePatches.push(FilePatch.createHiddenFilePatch( + removedFile, + removedFile, + removedMarker, + REMOVED, + /* istanbul ignore next */ + () => { throw new Error(`Attempt to expand removed file patch ${removedPath}`); }, + )); + } + return new MultiFilePatch({patchBuffer, filePatches}); } @@ -124,7 +145,7 @@ function singleDiffFilePatch(diff, patchBuffer, opts) { undefined; const renderStatus = renderStatusOverride || - (isDiffLarge([diff], opts) && TOO_LARGE) || + (isDiffLarge([diff], opts) && DEFERRED) || EXPANDED; if (!renderStatus.isVisible()) { @@ -191,7 +212,7 @@ function dualDiffFilePatch(diff1, diff2, patchBuffer, opts) { const newFile = new File({path: filePath, mode: newMode, symlink: newSymlink}); const renderStatus = opts.renderStatusOverrides[filePath] || - (isDiffLarge([contentChangeDiff], opts) && TOO_LARGE) || + (isDiffLarge([contentChangeDiff], opts) && DEFERRED) || EXPANDED; if (!renderStatus.isVisible()) { diff --git a/lib/models/patch/filter.js b/lib/models/patch/filter.js new file mode 100644 index 0000000000..dfb60b8b7a --- /dev/null +++ b/lib/models/patch/filter.js @@ -0,0 +1,50 @@ +export const MAX_PATCH_CHARS = 1024 * 1024; + +export function filter(original) { + let accumulating = false; + let accumulated = ''; + let includedChars = 0; + const removed = new Set(); + const pathRx = /\n?diff --git (?:a|b)\/(\S+) (?:a|b)\/(\S+)/y; + + let index = 0; + while (index !== -1) { + let include = true; + + const result = original.indexOf('\ndiff --git ', index); + const nextIndex = result !== -1 ? result + 1 : -1; + const patchEnd = nextIndex !== -1 ? nextIndex : original.length; + + // Exclude this patch if its inclusion would cause the patch to become too large. + const patchChars = patchEnd - index + 1; + if (includedChars + patchChars > MAX_PATCH_CHARS) { + include = false; + } + + if (include) { + // Avoid copying large buffers of text around if we're including everything anyway. + if (accumulating) { + accumulated += original.slice(index, patchEnd); + } + includedChars += patchChars; + } else { + // If this is the first excluded patch, start by copying everything before this into "accumulated." + if (!accumulating) { + accumulating = true; + accumulated = original.slice(0, index); + } + + // Extract the removed filenames from the "diff --git" line. + pathRx.lastIndex = index; + const pathMatch = pathRx.exec(original); + if (pathMatch) { + removed.add(pathMatch[1]); + removed.add(pathMatch[2]); + } + } + + index = nextIndex; + } + + return {filtered: accumulating ? accumulated : original, removed}; +} diff --git a/lib/models/patch/patch.js b/lib/models/patch/patch.js index 2e75f666cd..6607a6bb91 100644 --- a/lib/models/patch/patch.js +++ b/lib/models/patch/patch.js @@ -8,6 +8,8 @@ export const EXPANDED = { toString() { return 'RenderStatus(expanded)'; }, isVisible() { return true; }, + + isExpandable() { return false; }, }; export const COLLAPSED = { @@ -15,13 +17,26 @@ export const COLLAPSED = { toString() { return 'RenderStatus(collapsed)'; }, isVisible() { return false; }, + + isExpandable() { return true; }, }; -export const TOO_LARGE = { +export const DEFERRED = { /* istanbul ignore next */ - toString() { return 'RenderStatus(too-large)'; }, + toString() { return 'RenderStatus(deferred)'; }, isVisible() { return false; }, + + isExpandable() { return true; }, +}; + +export const REMOVED = { + /* istanbul ignore next */ + toString() { return 'RenderStatus(removed)'; }, + + isVisible() { return false; }, + + isExpandable() { return false; }, }; export default class Patch { diff --git a/lib/views/multi-file-patch-view.js b/lib/views/multi-file-patch-view.js index d8798444e4..a1a82297f4 100644 --- a/lib/views/multi-file-patch-view.js +++ b/lib/views/multi-file-patch-view.js @@ -23,8 +23,6 @@ import CommentGutterDecorationController from '../controllers/comment-gutter-dec import IssueishDetailItem from '../items/issueish-detail-item'; import File from '../models/patch/file'; -import {TOO_LARGE} from '../models/patch/patch'; - const executableText = { [File.modes.NORMAL]: 'non executable', [File.modes.EXECUTABLE]: 'executable', @@ -466,6 +464,8 @@ export default class MultiFilePatchView extends React.Component { renderFilePatchDecorations = (filePatch, index) => { const isCollapsed = !filePatch.getRenderStatus().isVisible(); const isEmpty = filePatch.getMarker().getRange().isEmpty(); + const isExpandable = filePatch.getRenderStatus().isExpandable(); + const isUnavailable = isCollapsed && !isExpandable; const atEnd = filePatch.getStartRange().start.isEqual(this.props.multiFilePatch.getBuffer().getEndPosition()); const position = isEmpty && atEnd ? 'after' : 'before'; @@ -498,7 +498,8 @@ export default class MultiFilePatchView extends React.Component { - {filePatch.getPatch().getRenderStatus() === TOO_LARGE && this.renderDiffGate(filePatch, position, index)} + {isExpandable && this.renderDiffGate(filePatch, position, index)} + {isUnavailable && this.renderDiffUnavailable(filePatch, position, index)} {this.renderHunkHeaders(filePatch, index)} @@ -529,6 +530,24 @@ export default class MultiFilePatchView extends React.Component { ); } + renderDiffUnavailable(filePatch, position, orderOffset) { + return ( + + + +

+ This diff is too large to load at all. Use the command-line to view it. +

+ + + + ); + } + renderExecutableModeChangeMeta(filePatch) { if (!filePatch.didChangeExecutableMode()) { return null; diff --git a/styles/editor-comment.less b/styles/editor-comment.less index 2db98265c4..b49aba611b 100644 --- a/styles/editor-comment.less +++ b/styles/editor-comment.less @@ -36,3 +36,25 @@ atom-text-editor { vertical-align: middle; } } + +.github-EditorComment-omitted { + display: flex; + flex-direction: column; + align-items: center; + background-color: @tool-panel-background-color; + border-radius: @component-border-radius; + padding: @component-padding; + margin: @component-padding/2 0; + + p { + display: flex; + flex-direction: row; + justify-content: center; + align-items: center; + margin: @component-padding/4; + } + + button { + margin-left: @component-padding; + } +} diff --git a/test/containers/changed-file-container.test.js b/test/containers/changed-file-container.test.js index a9612146a0..adb850c960 100644 --- a/test/containers/changed-file-container.test.js +++ b/test/containers/changed-file-container.test.js @@ -5,7 +5,7 @@ import {mount} from 'enzyme'; import ChangedFileContainer from '../../lib/containers/changed-file-container'; import ChangedFileItem from '../../lib/items/changed-file-item'; -import {TOO_LARGE, EXPANDED} from '../../lib/models/patch/patch'; +import {DEFERRED, EXPANDED} from '../../lib/models/patch/patch'; import {cloneRepository, buildRepository} from '../helpers'; describe('ChangedFileContainer', function() { @@ -93,7 +93,7 @@ describe('ChangedFileContainer', function() { await assert.async.isTrue(wrapper.update().exists('ChangedFileController')); const before = wrapper.find('ChangedFileController').prop('multiFilePatch'); const fp = before.getFilePatches()[0]; - assert.strictEqual(fp.getRenderStatus(), TOO_LARGE); + assert.strictEqual(fp.getRenderStatus(), DEFERRED); before.expandFilePatch(fp); assert.strictEqual(fp.getRenderStatus(), EXPANDED); diff --git a/test/containers/comment-decorations-container.test.js b/test/containers/comment-decorations-container.test.js index 539e1895bd..6ee4eb3213 100644 --- a/test/containers/comment-decorations-container.test.js +++ b/test/containers/comment-decorations-container.test.js @@ -41,6 +41,8 @@ describe('CommentDecorationsContainer', function() { localRepository={localRepository} loginModel={loginModel} reportRelayError={() => {}} + commands={atomEnv.commands} + children={() =>
} {...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); diff --git a/test/containers/commit-preview-container.test.js b/test/containers/commit-preview-container.test.js index a7ec81d769..5a36d435b8 100644 --- a/test/containers/commit-preview-container.test.js +++ b/test/containers/commit-preview-container.test.js @@ -5,7 +5,7 @@ import path from 'path'; import CommitPreviewContainer from '../../lib/containers/commit-preview-container'; import CommitPreviewItem from '../../lib/items/commit-preview-item'; -import {TOO_LARGE, EXPANDED} from '../../lib/models/patch/patch'; +import {DEFERRED, EXPANDED} from '../../lib/models/patch/patch'; import {cloneRepository, buildRepository} from '../helpers'; describe('CommitPreviewContainer', function() { @@ -89,7 +89,7 @@ describe('CommitPreviewContainer', function() { await assert.async.isTrue(wrapper.update().exists('CommitPreviewController')); const before = wrapper.find('CommitPreviewController').prop('multiFilePatch'); - assert.strictEqual(before.getFilePatches()[0].getRenderStatus(), TOO_LARGE); + assert.strictEqual(before.getFilePatches()[0].getRenderStatus(), DEFERRED); assert.strictEqual(before.getFilePatches()[1].getRenderStatus(), EXPANDED); before.expandFilePatch(before.getFilePatches()[0]); diff --git a/test/containers/pr-patch-container.test.js b/test/containers/pr-patch-container.test.js index afb31e14cf..203be973fa 100644 --- a/test/containers/pr-patch-container.test.js +++ b/test/containers/pr-patch-container.test.js @@ -27,14 +27,33 @@ describe('PullRequestPatchContainer', function() { status: 200, statusText: 'OK', getResolver: cb => cb(), + etag: null, + callNum: null, ...options, }; - return sinon.stub(window, 'fetch').callsFake(() => { + let stub = window.fetch; + if (!stub.restore) { + stub = sinon.stub(window, 'fetch'); + } + + let call = stub; + if (opts.callNum !== null) { + call = stub.onCall(opts.callNum); + } + + call.callsFake(() => { + const headers = { + 'Content-type': 'text/plain', + }; + if (opts.etag !== null) { + headers.ETag = opts.etag; + } + const resp = new window.Response(body, { status: opts.status, statusText: opts.statusText, - headers: {'Content-type': 'text/plain'}, + headers, }); let resolveResponsePromise = null; @@ -44,22 +63,48 @@ describe('PullRequestPatchContainer', function() { opts.getResolver(() => resolveResponsePromise(resp)); return promise; }); + return stub; + } + + function createChildrenCallback() { + const calls = []; + const waitingCallbacks = []; + + const fn = function(error, mfp) { + if (waitingCallbacks.length > 0) { + waitingCallbacks.shift()({error, mfp}); + return null; + } + + calls.push({error, mfp}); + return null; + }; + + fn.nextCall = function() { + if (calls.length > 0) { + return Promise.resolve(calls.shift()); + } + + return new Promise(resolve => waitingCallbacks.push(resolve)); + }; + + return fn; } describe('while the patch is loading', function() { - it('renders its child prop with nulls', function() { + it('renders its child prop with nulls', async function() { setDiffResponse(rawDiff); - const children = sinon.spy(); + const children = createChildrenCallback(); shallow(buildApp({children})); - assert.isTrue(children.calledWith(null, null)); + assert.deepEqual(await children.nextCall(), {error: null, mfp: null}); }); }); describe('when the patch has been fetched successfully', function() { it('builds the correct request', async function() { const stub = setDiffResponse(rawDiff); - const children = sinon.spy(); + const children = createChildrenCallback(); shallow(buildApp({ owner: 'smashwilson', repo: 'pushbot', @@ -79,9 +124,9 @@ describe('PullRequestPatchContainer', function() { }, )); - await assert.async.strictEqual(children.callCount, 2); + assert.deepEqual(await children.nextCall(), {error: null, mfp: null}); - const [error, mfp] = children.lastCall.args; + const {error, mfp} = await children.nextCall(); assert.isNull(error); assert.lengthOf(mfp.getFilePatches(), 1); @@ -96,11 +141,11 @@ describe('PullRequestPatchContainer', function() { it('modifies the patch to exclude a/ and b/ prefixes on file paths', async function() { setDiffResponse(rawDiffWithPathPrefix); - const children = sinon.spy(); + const children = createChildrenCallback(); shallow(buildApp({children})); - await assert.async.strictEqual(children.callCount, 2); - const [error, mfp] = children.lastCall.args; + await children.nextCall(); + const {error, mfp} = await children.nextCall(); assert.isNull(error); assert.lengthOf(mfp.getFilePatches(), 1); @@ -112,11 +157,11 @@ describe('PullRequestPatchContainer', function() { it('excludes a/ prefix on the old file of a deletion', async function() { setDiffResponse(rawDeletionDiff); - const children = sinon.spy(); + const children = createChildrenCallback(); shallow(buildApp({children})); - await assert.async.strictEqual(children.callCount, 2); - const [error, mfp] = children.lastCall.args; + await children.nextCall(); + const {error, mfp} = await children.nextCall(); assert.isNull(error); assert.lengthOf(mfp.getFilePatches(), 1); @@ -128,11 +173,11 @@ describe('PullRequestPatchContainer', function() { it('excludes b/ prefix on the new file of an addition', async function() { setDiffResponse(rawAdditionDiff); - const children = sinon.spy(); + const children = createChildrenCallback(); shallow(buildApp({children})); - await assert.async.strictEqual(children.callCount, 2); - const [error, mfp] = children.lastCall.args; + await children.nextCall(); + const {error, mfp} = await children.nextCall(); assert.isNull(error); assert.lengthOf(mfp.getFilePatches(), 1); @@ -143,12 +188,12 @@ describe('PullRequestPatchContainer', function() { it('converts file paths to use native path separators', async function() { setDiffResponse(rawDiffWithPathPrefix); - const children = sinon.spy(); + const children = createChildrenCallback(); shallow(buildApp({children})); - await assert.async.strictEqual(children.callCount, 2); - const [error, mfp] = children.lastCall.args; + await children.nextCall(); + const {error, mfp} = await children.nextCall(); assert.isNull(error); assert.lengthOf(mfp.getFilePatches(), 1); @@ -162,12 +207,11 @@ describe('PullRequestPatchContainer', function() { setDiffResponse(rawDiff, { getResolver(cb) { resolve = cb; }, }); - const children = sinon.spy(); + const children = createChildrenCallback(); const wrapper = shallow(buildApp({children})); const fetchDiffSpy = sinon.spy(wrapper.instance(), 'fetchDiff'); wrapper.setProps({refetch: true}); - assert.isTrue(children.calledWith(null, null)); const setStateSpy = sinon.spy(wrapper.instance(), 'setState'); wrapper.unmount(); @@ -176,6 +220,24 @@ describe('PullRequestPatchContainer', function() { assert.isFalse(setStateSpy.called); }); + + it('respects a custom largeDiffThreshold', async function() { + setDiffResponse(rawDiff); + + const children = createChildrenCallback(); + shallow(buildApp({ + largeDiffThreshold: 1, + children, + })); + + await children.nextCall(); + const {error, mfp} = await children.nextCall(); + + assert.isNull(error); + assert.lengthOf(mfp.getFilePatches(), 1); + const [fp] = mfp.getFilePatches(); + assert.isFalse(fp.getRenderStatus().isVisible()); + }); }); describe('when there has been an error', function() { @@ -183,11 +245,11 @@ describe('PullRequestPatchContainer', function() { const output = sinon.stub(console, 'error'); sinon.stub(window, 'fetch').rejects(new Error('kerPOW')); - const children = sinon.spy(); + const children = createChildrenCallback(); shallow(buildApp({children})); - await assert.async.strictEqual(children.callCount, 2); - const [error, mfp] = children.lastCall.args; + await children.nextCall(); + const {error, mfp} = await children.nextCall(); assert.strictEqual(error, 'Network error encountered fetching the patch: kerPOW.'); assert.isNull(mfp); @@ -200,11 +262,11 @@ describe('PullRequestPatchContainer', function() { statusText: 'Not found', }); - const children = sinon.spy(); + const children = createChildrenCallback(); shallow(buildApp({children})); - await assert.async.strictEqual(children.callCount, 2); - const [error, mfp] = children.lastCall.args; + await children.nextCall(); + const {error, mfp} = await children.nextCall(); assert.strictEqual(error, 'Unable to fetch the diff for this pull request: Not found.'); assert.isNull(mfp); @@ -214,11 +276,11 @@ describe('PullRequestPatchContainer', function() { const output = sinon.stub(console, 'error'); setDiffResponse('bad diff no treat for you'); - const children = sinon.spy(); + const children = createChildrenCallback(); shallow(buildApp({children})); - await assert.async.strictEqual(children.callCount, 2); - const [error, mfp] = children.lastCall.args; + await children.nextCall(); + const {error, mfp} = await children.nextCall(); assert.strictEqual(error, 'Unable to parse the diff for this pull request.'); assert.isNull(mfp); @@ -227,15 +289,14 @@ describe('PullRequestPatchContainer', function() { }); describe('when a refetch is requested', function() { - it('refetches patch data on the first render', async function() { + it('refetches patch data on the next render', async function() { const fetch = setDiffResponse(rawDiff); - const children = sinon.spy(); + const children = createChildrenCallback(); const wrapper = shallow(buildApp({children})); assert.strictEqual(fetch.callCount, 1); - assert.isTrue(children.calledWith(null, null)); - - await assert.async.strictEqual(children.callCount, 2); + assert.deepEqual(await children.nextCall(), {error: null, mfp: null}); + await children.nextCall(); wrapper.setProps({refetch: true}); assert.strictEqual(fetch.callCount, 2); @@ -244,17 +305,53 @@ describe('PullRequestPatchContainer', function() { it('does not refetch data on additional renders', async function() { const fetch = setDiffResponse(rawDiff); - const children = sinon.spy(); + const children = createChildrenCallback(); const wrapper = shallow(buildApp({children, refetch: true})); assert.strictEqual(fetch.callCount, 1); - assert.strictEqual(children.callCount, 1); - await assert.async.strictEqual(children.callCount, 2); + await children.nextCall(); + await children.nextCall(); wrapper.setProps({refetch: true}); - assert.strictEqual(fetch.callCount, 1); - assert.strictEqual(children.callCount, 3); + }); + + it('does not reparse data if the diff has not been modified', async function() { + const stub = setDiffResponse(rawDiff, {callNum: 0, etag: '12345'}); + setDiffResponse(null, {callNum: 1, status: 304}); + + const children = createChildrenCallback(); + const wrapper = shallow(buildApp({ + owner: 'smashwilson', + repo: 'pushbot', + number: 12, + endpoint: getEndpoint('github.com'), + token: 'swordfish', + children, + })); + + assert.deepEqual(await children.nextCall(), {error: null, mfp: null}); + const {error: error0, mfp: mfp0} = await children.nextCall(); + assert.isNull(error0); + + wrapper.setProps({refetch: true}); + + assert.deepEqual(await children.nextCall(), {error: null, mfp: mfp0}); + assert.deepEqual(await children.nextCall(), {error: null, mfp: null}); + const {error: error1, mfp: mfp1} = await children.nextCall(); + assert.isNull(error1); + assert.strictEqual(mfp1, mfp0); + + assert.isTrue(stub.calledWith( + 'https://api.github.com/repos/smashwilson/pushbot/pulls/12', + { + headers: { + 'Accept': 'application/vnd.github.v3.diff', + 'Authorization': 'bearer swordfish', + 'If-None-Match': '12345', + }, + }, + )); }); }); }); diff --git a/test/controllers/editor-comment-decorations-controller.test.js b/test/controllers/editor-comment-decorations-controller.test.js index f9ee7b0df9..09c7666f1e 100644 --- a/test/controllers/editor-comment-decorations-controller.test.js +++ b/test/controllers/editor-comment-decorations-controller.test.js @@ -7,6 +7,7 @@ import CommentGutterDecorationController from '../../lib/controllers/comment-gut import Marker from '../../lib/atom/marker'; import Decoration from '../../lib/atom/decoration'; import {getEndpoint} from '../../lib/models/endpoint'; +import ReviewsItem from '../../lib/items/reviews-item'; describe('EditorCommentDecorationsController', function() { let atomEnv, workspace, editor, wrapper; @@ -34,6 +35,7 @@ describe('EditorCommentDecorationsController', function() { threadsForPath: [], commentTranslationsForPath: { diffToFilePosition: new Map(), + removed: false, }, ...opts, @@ -60,6 +62,7 @@ describe('EditorCommentDecorationsController', function() { [4, 7], [10, 13], ]), + removed: false, }; wrapper = shallow(buildApp({threadsForPath, commentTranslationsForPath})); @@ -85,6 +88,7 @@ describe('EditorCommentDecorationsController', function() { [4, 5], [10, 11], ]), + removed: false, }; wrapper = shallow(buildApp({threadsForPath, commentTranslationsForPath})); @@ -100,6 +104,7 @@ describe('EditorCommentDecorationsController', function() { const commentTranslationsForPath = { diffToFilePosition: new Map([[4, 4]]), + removed: false, digest: '1111', }; @@ -120,4 +125,45 @@ describe('EditorCommentDecorationsController', function() { assert.isTrue(wrapper.find(Marker).prop('bufferRange').isEqual([[5, 0], [5, 3]])); }); + + it('creates a block decoration if the diff was too large to parse', async function() { + const threadsForPath = [ + {rootCommentID: 'comment0', position: 4, threadID: 'thread0'}, + {rootCommentID: 'comment1', position: 10, threadID: 'thread1'}, + ]; + const commentTranslationsForPath = { + diffToFilePosition: new Map(), + removed: true, + }; + + wrapper = shallow(buildApp({ + threadsForPath, + commentTranslationsForPath, + endpoint: getEndpoint('github.enterprise.horse'), + owner: 'some-owner', + repo: 'a-repo', + number: 400, + workdir: __dirname, + })); + + const decorations = wrapper.find(Decoration); + assert.lengthOf(decorations.findWhere(decoration => decoration.prop('type') === 'line'), 0); + assert.lengthOf(decorations.findWhere(decoration => decoration.prop('type') === 'block'), 1); + + const reviewsItem = {jumpToThread: sinon.spy()}; + sinon.stub(workspace, 'open').resolves(reviewsItem); + + await wrapper.find('button').prop('onClick')(); + assert.isTrue(workspace.open.calledWith( + ReviewsItem.buildURI({ + host: 'github.enterprise.horse', + owner: 'some-owner', + repo: 'a-repo', + number: 400, + workdir: __dirname, + }), + {searchAllPanes: true}, + )); + assert.isTrue(reviewsItem.jumpToThread.calledWith('thread0')); + }); }); diff --git a/test/models/patch/builder.test.js b/test/models/patch/builder.test.js index 83816454c3..9e9c973053 100644 --- a/test/models/patch/builder.test.js +++ b/test/models/patch/builder.test.js @@ -1,5 +1,5 @@ import {buildFilePatch, buildMultiFilePatch} from '../../../lib/models/patch'; -import {TOO_LARGE, EXPANDED} from '../../../lib/models/patch/patch'; +import {DEFERRED, EXPANDED, REMOVED} from '../../../lib/models/patch/patch'; import {multiFilePatchBuilder} from '../../builder/patch'; import {assertInPatch, assertInFilePatch} from '../../helpers'; @@ -854,7 +854,7 @@ describe('buildFilePatch', function() { assert.lengthOf(mfp.getFilePatches(), 2); const [fp0, fp1] = mfp.getFilePatches(); - assert.strictEqual(fp0.getRenderStatus(), TOO_LARGE); + assert.strictEqual(fp0.getRenderStatus(), DEFERRED); assert.strictEqual(fp0.getOldPath(), 'first'); assert.strictEqual(fp0.getNewPath(), 'first'); assert.deepEqual(fp0.getStartRange().serialize(), [[0, 0], [0, 0]]); @@ -904,7 +904,7 @@ describe('buildFilePatch', function() { assert.lengthOf(mfp.getFilePatches(), 2); const [fp0, fp1] = mfp.getFilePatches(); - assert.strictEqual(fp0.getRenderStatus(), TOO_LARGE); + assert.strictEqual(fp0.getRenderStatus(), DEFERRED); assert.strictEqual(fp0.getOldPath(), 'big'); assert.strictEqual(fp0.getNewPath(), 'big'); assert.deepEqual(fp0.getMarker().getRange().serialize(), [[0, 0], [0, 0]]); @@ -939,7 +939,7 @@ describe('buildFilePatch', function() { assert.lengthOf(mfp.getFilePatches(), 1); const [fp] = mfp.getFilePatches(); - assert.strictEqual(fp.getRenderStatus(), TOO_LARGE); + assert.strictEqual(fp.getRenderStatus(), DEFERRED); assert.strictEqual(fp.getOldPath(), 'first'); assert.strictEqual(fp.getNewPath(), 'first'); assert.deepEqual(fp.getStartRange().serialize(), [[0, 0], [0, 0]]); @@ -980,7 +980,7 @@ describe('buildFilePatch', function() { assert.lengthOf(mfp.getFilePatches(), 1); const [fp] = mfp.getFilePatches(); - assert.strictEqual(fp.getRenderStatus(), TOO_LARGE); + assert.strictEqual(fp.getRenderStatus(), DEFERRED); assert.strictEqual(fp.getOldPath(), 'big'); assert.strictEqual(fp.getNewPath(), 'big'); assert.deepEqual(fp.getStartRange().serialize(), [[0, 0], [0, 0]]); @@ -1044,7 +1044,7 @@ describe('buildFilePatch', function() { }, ); - assert.strictEqual(fp1.getRenderStatus(), TOO_LARGE); + assert.strictEqual(fp1.getRenderStatus(), DEFERRED); assert.deepEqual(fp1.getPatch().getMarker().getRange().serialize(), [[4, 0], [4, 0]]); assertInFilePatch(fp1, mfp.getBuffer()).hunks(); @@ -1176,6 +1176,44 @@ describe('buildFilePatch', function() { }); }); + describe('with a removed diff', function() { + it('creates a HiddenPatch when the diff has been removed', function() { + const mfp = buildMultiFilePatch([ + { + oldPath: 'only', oldMode: '100644', newPath: 'only', newMode: '100755', status: 'modified', + hunks: [ + { + oldStartLine: 1, oldLineCount: 1, newStartLine: 1, newLineCount: 2, + lines: [' line-4', '+line-5'], + }, + ], + }, + ], {removed: new Set(['removed'])}); + + assert.lengthOf(mfp.getFilePatches(), 2); + const [fp0, fp1] = mfp.getFilePatches(); + + assert.strictEqual(fp0.getRenderStatus(), EXPANDED); + assert.strictEqual(fp0.getOldPath(), 'only'); + assert.strictEqual(fp0.getNewPath(), 'only'); + assert.deepEqual(fp0.getMarker().getRange().serialize(), [[0, 0], [1, 6]]); + assertInFilePatch(fp0, mfp.getBuffer()).hunks( + { + startRow: 0, endRow: 1, header: '@@ -1,1 +1,2 @@', regions: [ + {kind: 'unchanged', string: ' line-4\n', range: [[0, 0], [0, 6]]}, + {kind: 'addition', string: '+line-5', range: [[1, 0], [1, 6]]}, + ], + }, + ); + + assert.strictEqual(fp1.getRenderStatus(), REMOVED); + assert.strictEqual(fp1.getOldPath(), 'removed'); + assert.strictEqual(fp1.getNewPath(), 'removed'); + assert.deepEqual(fp1.getMarker().getRange().serialize(), [[1, 6], [1, 6]]); + assertInFilePatch(fp1, mfp.getBuffer()).hunks(); + }); + }); + it('throws an error with an unexpected number of diffs', function() { assert.throws(() => buildFilePatch([1, 2, 3]), /Unexpected number of diffs: 3/); }); diff --git a/test/models/patch/file-patch.test.js b/test/models/patch/file-patch.test.js index aa7e8eed52..562a41c370 100644 --- a/test/models/patch/file-patch.test.js +++ b/test/models/patch/file-patch.test.js @@ -2,7 +2,7 @@ import {TextBuffer} from 'atom'; import FilePatch from '../../../lib/models/patch/file-patch'; import File, {nullFile} from '../../../lib/models/patch/file'; -import Patch, {TOO_LARGE, COLLAPSED, EXPANDED} from '../../../lib/models/patch/patch'; +import Patch, {DEFERRED, COLLAPSED, EXPANDED} from '../../../lib/models/patch/patch'; import PatchBuffer from '../../../lib/models/patch/patch-buffer'; import Hunk from '../../../lib/models/patch/hunk'; import {Unchanged, Addition, Deletion, NoNewline} from '../../../lib/models/patch/region'; @@ -700,7 +700,7 @@ describe('FilePatch', function() { it('triggerCollapseIn returns false if patch is not visible', function() { const {multiFilePatch} = multiFilePatchBuilder() .addFilePatch(fp => { - fp.renderStatus(TOO_LARGE); + fp.renderStatus(DEFERRED); }).build(); const filePatch = multiFilePatch.getFilePatches()[0]; assert.isFalse(filePatch.triggerCollapseIn(new PatchBuffer(), {before: [], after: []})); diff --git a/test/models/patch/filter.test.js b/test/models/patch/filter.test.js new file mode 100644 index 0000000000..c657aabb50 --- /dev/null +++ b/test/models/patch/filter.test.js @@ -0,0 +1,60 @@ +import {filter, MAX_PATCH_CHARS} from '../../../lib/models/patch/filter'; + +describe('patch filter', function() { + it('passes through small patches as-is', function() { + const original = new PatchBuilder() + .addSmallPatch('path-a.txt') + .addSmallPatch('path-b.txt') + .toString(); + + const {filtered, removed} = filter(original); + assert.strictEqual(filtered, original); + assert.sameMembers(Array.from(removed), []); + }); + + it('removes files from the patch that exceed the size threshold', function() { + const original = new PatchBuilder() + .addLargePatch('path-a.txt') + .addSmallPatch('path-b.txt') + .toString(); + + const expected = new PatchBuilder() + .addSmallPatch('path-b.txt') + .toString(); + + const {filtered, removed} = filter(original); + assert.strictEqual(filtered, expected); + assert.sameMembers(Array.from(removed), ['path-a.txt']); + }); +}); + +class PatchBuilder { + constructor() { + this.text = ''; + } + + addSmallPatch(fileName) { + this.text += `diff --git a/${fileName} b/${fileName}\n`; + this.text += '+aaaa\n'; + this.text += '+bbbb\n'; + this.text += '+cccc\n'; + this.text += '+dddd\n'; + this.text += '+eeee\n'; + return this; + } + + addLargePatch(fileName) { + this.text += `diff --git a/${fileName} b/${fileName}\n`; + const line = '+yyyy\n'; + let totalSize = 0; + while (totalSize < MAX_PATCH_CHARS) { + this.text += line; + totalSize += line.length; + } + return this; + } + + toString() { + return this.text; + } +} diff --git a/test/models/patch/multi-file-patch.test.js b/test/models/patch/multi-file-patch.test.js index 68ca8fa58a..3a0ad00f5a 100644 --- a/test/models/patch/multi-file-patch.test.js +++ b/test/models/patch/multi-file-patch.test.js @@ -2,7 +2,7 @@ import dedent from 'dedent-js'; import {multiFilePatchBuilder, filePatchBuilder} from '../../builder/patch'; -import {TOO_LARGE, COLLAPSED, EXPANDED} from '../../../lib/models/patch/patch'; +import {DEFERRED, COLLAPSED, EXPANDED} from '../../../lib/models/patch/patch'; import MultiFilePatch from '../../../lib/models/patch/multi-file-patch'; import PatchBuffer from '../../../lib/models/patch/patch-buffer'; @@ -646,7 +646,7 @@ describe('MultiFilePatch', function() { const multiFilePatch = multiFilePatchBuilder() .addFilePatch(fp => { fp.setOldFile(f => f.path('file-0')); - fp.renderStatus(TOO_LARGE); + fp.renderStatus(DEFERRED); }) .build() .multiFilePatch; @@ -683,7 +683,7 @@ describe('MultiFilePatch', function() { }) .addFilePatch(fp => { fp.setOldFile(f => f.path('too-large-file')); - fp.renderStatus(TOO_LARGE); + fp.renderStatus(DEFERRED); }) .addFilePatch(fp => { fp.setOldFile(f => f.path('collapsed-file')); @@ -885,7 +885,7 @@ describe('MultiFilePatch', function() { fp.addHunk(h => h.unchanged('0 (1)').added('1 (2)', '2 (3)').deleted('3 (4)').unchanged('4 (5)')); fp.addHunk(h => h.unchanged('5 (7)').added('6 (8)', '7 (9)', '8 (10)').unchanged('9 (11)')); fp.addHunk(h => h.unchanged('10 (13)').deleted('11 (14)').unchanged('12 (15)')); - fp.renderStatus(TOO_LARGE); + fp.renderStatus(DEFERRED); }) .build(); assert.isTrue(multiFilePatch.isDiffRowOffsetIndexEmpty('1.txt')); diff --git a/test/views/multi-file-patch-view.test.js b/test/views/multi-file-patch-view.test.js index 927c9bf3e9..97298ad8a0 100644 --- a/test/views/multi-file-patch-view.test.js +++ b/test/views/multi-file-patch-view.test.js @@ -4,7 +4,7 @@ import {shallow, mount} from 'enzyme'; import * as reporterProxy from '../../lib/reporter-proxy'; import {cloneRepository, buildRepository} from '../helpers'; -import {EXPANDED, COLLAPSED, TOO_LARGE} from '../../lib/models/patch/patch'; +import {EXPANDED, COLLAPSED, DEFERRED, REMOVED} from '../../lib/models/patch/patch'; import MultiFilePatchView from '../../lib/views/multi-file-patch-view'; import {multiFilePatchBuilder} from '../builder/patch'; import {aggregatedReviewsBuilder} from '../builder/graphql/aggregated-reviews-builder'; @@ -1878,7 +1878,7 @@ describe('MultiFilePatchView', function() { beforeEach(function() { const {multiFilePatch} = multiFilePatchBuilder() .addFilePatch(fp => { - fp.renderStatus(TOO_LARGE); + fp.renderStatus(DEFERRED); }).build(); mfp = multiFilePatch; wrapper = mount(buildApp({multiFilePatch: mfp})); @@ -1917,4 +1917,26 @@ describe('MultiFilePatchView', function() { assert.isTrue(wrapper.find('.github-HunkHeaderView').exists()); }); }); + + describe('removed diff message', function() { + let wrapper, mfp; + + beforeEach(function() { + const {multiFilePatch} = multiFilePatchBuilder() + .addFilePatch(fp => { + fp.renderStatus(REMOVED); + }).build(); + mfp = multiFilePatch; + wrapper = mount(buildApp({multiFilePatch: mfp})); + }); + + it('displays the message when a file has been removed', function() { + assert.include( + wrapper.find('.github-FilePatchView-controlBlock .github-FilePatchView-message').first().text(), + 'This diff is too large to load at all.', + ); + assert.isFalse(wrapper.find('.github-FilePatchView-showDiffButton').exists()); + assert.isFalse(wrapper.find('.github-HunkHeaderView').exists()); + }); + }); });