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

collapse large diffs && collapse/expand any diff within multi file patch view #1913

Merged
merged 264 commits into from Feb 28, 2019

Conversation

Projects
7 participants
@annthurium
Copy link
Contributor

annthurium commented Jan 16, 2019

Description of the Change

This pull request adds code to detect large diffs, and automatically collapse them. It also adds the ability for users to collapse and expand any diff within a multi file patch view. There's a lot of shared logic between both so it made sense to tackle them at the same time.

TODO

https://github.com/atom/github/projects/18

Screenshot/Gif

TBD

Alternate Designs

Benefits

  • The pull request view, commit preview, and commit pane are more useful now.
  • No more endless scrolling to get past your package-lock.json changes that you probably don't need to review in such detail!

Possible Drawbacks

TBD

Applicable Issues

#1860

QA Checklist

  • file patches that exceed large diff threshold are initially collapsed
  • file patches that do not exceed large diff threshold are not initially collapsed
  • collapsing by clicking "collapse" icon collapses the file patch and all decorations:
    • comments
    • hunk headers
    • file mode headers
    • symlink headers
  • collapsing by clicking in the diff collapses the file patch and all decorations:
    • comments (only exist in the pull request files changed tab)
    • hunk headers
    • file mode headers
    • symlink headers
  • expanding a previously collapsed patch re-expands the patch and all decorations are present
  • when collapsing and expanding, patches stay in the correct order
  • test collapsing / expanding if only one patch exists
  • staging view should update when new files are added
  • no visual regressions in the component trees that use multi file patch:
    • pull request changed files tab
    • commit detail
    • commit preview
    • changed files

Metrics

  • Count of how many files are manually collapsed / expanded
  • Performance of rendering large diffs

Tests

TBD

Documentation

TBD

Release Notes

The GitHub package now supports collapsing and expanding any files within multi file diff views (such as commit preview and the pull request files view), and also automatically collapses large files.

User Experience Research (Optional)

The UI is simple enough for this that I would argue it does not warrant UXR.

smashwilson and others added some commits Jan 16, 2019

@todo

This comment has been minimized.

Copy link

todo bot commented Jan 16, 2019

Set this based on performance measurements

// TODO: Set this based on performance measurements
largeDiffThreshold: 10,
// Map of file path (relative to repository root) to Patch render status (EXPANDED, COLLAPSED, TOO_LARGE)
renderStatusOverrides: {},
};


This comment was generated by todo based on a TODO comment in 7eab119 in #1913. cc @atom.
@todo

This comment has been minimized.

Copy link

todo bot commented Jan 16, 2019

do something with large diff too

// TODO: do something with large diff too
}
const [hunks, patchMarker] = buildHunks(contentChangeDiff, layeredBuffer, layeredBuffer.buffer.getLastRow());
const patch = new Patch({status, hunks, marker: patchMarker, buffer: layeredBuffer.buffer});


This comment was generated by todo based on a TODO comment in 7eab119 in #1913. cc @atom.
@todo

This comment has been minimized.

Copy link

todo bot commented Jan 16, 2019

it prob shouldn't extend NullPatch

// TODO: it prob shouldn't extend NullPatch
class DelayedPatch extends NullPatch {
constructor(position, renderStatus, parseFn) {
super();
this.position = position;
this.renderStatus = renderStatus;


This comment was generated by todo based on a TODO comment in 7eab119 in #1913. cc @atom.
@codecov

This comment has been minimized.

Copy link

codecov bot commented Jan 16, 2019

Codecov Report

Merging #1913 into master will increase coverage by 0.45%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1913      +/-   ##
==========================================
+ Coverage   92.12%   92.57%   +0.45%     
==========================================
  Files         188      189       +1     
  Lines       10809    11237     +428     
  Branches     1582     1633      +51     
==========================================
+ Hits         9958    10403     +445     
+ Misses        851      834      -17
Impacted Files Coverage Δ
lib/controllers/pr-reviews-controller.js 100% <ø> (ø) ⬆️
lib/models/patch/patch.js 100% <100%> (ø) ⬆️
lib/models/patch/region.js 100% <100%> (ø) ⬆️
lib/views/pr-review-comments-view.js 100% <100%> (ø) ⬆️
lib/models/patch/hunk.js 100% <100%> (ø) ⬆️
lib/containers/changed-file-container.js 100% <100%> (ø) ⬆️
lib/views/file-patch-header-view.js 100% <100%> (ø) ⬆️
lib/models/patch/multi-file-patch.js 100% <100%> (ø) ⬆️
lib/models/patch/file-patch.js 100% <100%> (ø) ⬆️
lib/views/multi-file-patch-view.js 100% <100%> (ø) ⬆️
... and 12 more

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 d42b8e8...3b65cfe. Read the comment docs.

@annthurium annthurium changed the title Vy/diff gate collapse large diffs && collapse/expand any diff within multi file patch view Jan 16, 2019

smashwilson and others added some commits Jan 16, 2019

deal with null patches
...not sure how we would get into this situation, but it seems like an 
important case to cover.
clean up pr comments builder
unnecessary console output during tests is a bummer.

smashwilson and others added some commits Feb 26, 2019

@smashwilson
Copy link
Member

smashwilson left a comment

I found a few minor tweaks and renames. Almost there 🎉

> > > Render a sequence of git-generated file patches within a TextEditor, using decorations to include contextually
> > > relevant controls.
> > > Render a sequence of git-generated file patches within a TextEditor, using decorations to include contextually relevant controls.
> > > See [`MultiFilePatchView` atlas](#multifilepatchview-atlas) below for a more detailed breakdown.

This comment has been minimized.

@smashwilson

smashwilson Feb 28, 2019

Member

What I really want is a tool that parses our source and outputs this atlas as an .svg diagram or something 🤔

Show resolved Hide resolved lib/atom/atom-text-editor.js
Show resolved Hide resolved lib/containers/pr-changed-files-container.js
Show resolved Hide resolved lib/models/patch/builder.js Outdated
Show resolved Hide resolved lib/models/patch/multi-file-patch.js Outdated
Show resolved Hide resolved test/models/patch/file-patch.test.js Outdated
});
});

it('is deterministic regardless of the order in which collapse and expand operations are performed', function() {

This comment has been minimized.

@smashwilson

smashwilson Feb 28, 2019

Member

I'm still proud of this test case 😆

Show resolved Hide resolved test/models/patch/patch-buffer.test.js Outdated
Show resolved Hide resolved test/models/patch/patch.test.js Outdated
Show resolved Hide resolved test/models/patch/patch.test.js Outdated

smashwilson added some commits Feb 28, 2019

@smashwilson smashwilson merged commit 96d77c5 into master Feb 28, 2019

2 checks passed

codecov/patch 100% of diff hit (target 92.12%)
Details
codecov/project 92.57% (+0.45%) compared to d42b8e8
Details

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

@smashwilson smashwilson deleted the vy/diff-gate branch Feb 28, 2019

@vanessayuenn vanessayuenn referenced this pull request Mar 6, 2019

Closed

v0.27.0-0 QA Review #2005

7 of 9 tasks complete
@@ -74,8 +74,8 @@ This is a high-level overview of the structure of the React component tree that
> > > [`<MultiFilePatchController>`](/lib/controllers/multi-file-patch-controller.js)
> > > [`<MultiFilePatchView>`](/lib/views/multi-file-patch-view.js)
> > >
> > > Render a sequence of git-generated file patches within a TextEditor, using decorations to include contextually
> > > relevant controls.

This comment has been minimized.

@kuychaco

kuychaco Mar 12, 2019

Member

for testing: comment on deleted line



## `MultiFilePatchView` Atlas

This comment has been minimized.

@kuychaco

kuychaco Mar 13, 2019

Member

another test comment

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.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.