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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix bug expanding large-diff-gated patches with comments #2007

merged 4 commits into from Mar 6, 2019


2 participants
Copy link

commented Mar 6, 2019

Description of the Change

Once upon a time, there was a bug where clicking "expand" on a file patch that exceeded the large diff gate size, that had comments, would throw a stacktrace. Sadness!

@vanessayuenn and I determined that the root cause of this bug was that because large patches are initially collapsed, we were never populating the red and black tree used to keep track of the diff offset indices for comment placement.

Solution was to extract a function to populate the diff offset indices, and then call that function when expanding file patches.



Alternate Designs

None considered.


Users can now expand large-diff-gated file patches that contain comments without errors and sadness. (Well, I can't guarantee there will be no sadness.)

Possible Drawbacks

Can't think of any drawbacks but if you can, I'm open to discussion.

Applicable Issues




  • Wrote unit test to ensure that:

    • we are only populating the diffOffsetIndices if it's empty
    • after expanding an initially-collapsed too large pass, calling getBufferRowForDiffPosition in fact gives us the correct buffer row.
  • Manually testing expanding the file patch on kuychaco/test-repo#7


Added one code comment like a champ.

Release Notes

This bug is for a feature that hasn't been released yet so I'm ok leaving it out of the release notes.

User Experience Research (Optional)


annthurium and others added some commits Mar 6, 2019

extract method for populating multiFilePatch.diffRowOffsetIndices
File patches that exceed the large diff threshold are initially collapsed.  We were not actually populating the diffRowOffsetIndex for these collapsed patches.  When expanding a collapsed patch with comments, we'd try to use the diffRowOffsetIndex which was empty, and throw an error.

Co-Authored-By: Vanessa Yuen <>
perf optimization to only populateDiffRowOffsetIndices if we need to.
Co-Authored-By: Vanessa Yuen <>
add unit tests for collapsed patch bug
Co-Authored-By: Vanessa Yuen <>

This comment has been minimized.

Copy link

commented Mar 6, 2019

Codecov Report

Merging #2007 into master will decrease coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2007      +/-   ##
- Coverage   92.58%   92.58%   -0.01%     
  Files         189      189              
  Lines       11237    11244       +7     
  Branches     1633     1634       +1     
+ Hits        10404    10410       +6     
- Misses        833      834       +1
Impacted Files Coverage Δ
lib/models/patch/multi-file-patch.js 100% <100%> (ø) ⬆️
lib/github-package.js 68.09% <0%> (-0.39%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 14c238c...41136a6. Read the comment docs.

@annthurium annthurium requested a review from atom/github-package Mar 6, 2019

Copy link

left a comment

Thanks for putting in this fix so quickly! It looks good 👍

Just left a couple of comments -- one suggestion and one question about a potential edge case. But I think neither of them are necessarily ship-blocking.

const diffRowOffsetIndex = this.diffRowOffsetIndices.get(filePatch.getPath());
// if the patch was initially collapsed, we need to calculate
// the diffRowOffsetIndices to calculate comment position.
if (diffRowOffsetIndex.index.size === 0) {

This comment has been minimized.

Copy link

kuychaco Mar 6, 2019


🎨minor readability suggestion -- I noticed I had to look at this a couple times, as well as assert.strictEqual(multiFilePatch.diffRowOffsetIndices.get('1.txt').index.size, 0) in the tests.

Perhaps we could add a method diffRowOffsetIndexUnpopulated that does this check and reads more declaratively. And we can also use the method to improve readability in our test assertions.

This comment has been minimized.

Copy link

kuychaco Mar 6, 2019


Hmm is there an edge case here for symlink and file mode change diffs that would have empty indexes? I guess we'd just unnecessarily call populateDiffRowOffsetIndices and create a new empty index, which isn't a big deal. It's probably not worth the extra effort to distinguish between empty indexes from large diffs versus symlink or file mode changes, huh?

This comment has been minimized.

Copy link

annthurium Mar 6, 2019

Author Contributor

yeah, I'd take readability and simplicity over dealing with symlink and file mode changes here. Creating a new empty index shouldn't impact performance very much.

I'll work on the other changes though, thanks for the suggestion.

@annthurium annthurium requested a review from atom/github-package Mar 6, 2019

@annthurium annthurium merged commit c0f6679 into master Mar 6, 2019

2 checks passed

codecov/patch 100% of diff hit (target 92.58%)
codecov/project Absolute coverage decreased by -<.01% but relative coverage increased by +7.41% compared to 14c238c

Sprint : 13 February 2019 - 5 March 2019 : v0.27.0 automation moved this from In progress to Merged Mar 6, 2019

@annthurium annthurium deleted the tt-19-mar-destructure-bug branch Mar 6, 2019

@vanessayuenn vanessayuenn referenced this pull request Mar 7, 2019


v0.27.0-0 QA Review #2005

7 of 9 tasks complete

@smashwilson smashwilson referenced this pull request Apr 8, 2019


v0.28.0-0 QA Review #2051

3 of 3 tasks complete

@smashwilson smashwilson added this to In progress in Sprint : 7 March 2019 - 3 April 2019 : v0.28.0 via automation Apr 8, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.