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

Reviews dock item #1995

Merged
merged 742 commits into from Apr 8, 2019

Conversation

5 participants
@smashwilson
Copy link
Member

commented Mar 1, 2019

Description of the Change

View pull request reviews and review comments in a dock item.

Screenshot/Gif

TODO

Benefits

Pull request authors will now enjoy many features for responding to code review comments from the comfort of their editor.

  • see review comments alongside the code
  • jump to code from reviews dock
  • jump to diff from reviews dock
  • reply to comments
  • resolve comments
  • see a progress bar of resolved comments, to keep track of progress
  • react with emoji to comments

Metrics

  • opening the reviews tab
    ='open-reviews-tab', {package: 'github', from: IssueishDetailController});
  • opening the review thread from the gutter decorations
    open-review-thread', {package: 'github'}
  • jumping to a file from a review comment thread
    'reviews-dock-open-file', {package: 'github'});
  • jumping to a diff from a reviews comment thread
    'reviews-dock-open-diff', {package: 'github'});
  • opening a pull request detail view from the reviews dock
    'reviews-dock-open-pr', {package: 'github', component: this.constructor.name});
  • running the command to show more context lines in a review comment thread
    'reviews-dock-show-more-context', {package: 'github'});
  • running the command to show less context lines in a review comment thread
    'reviews-dock-show-less-context', {package: 'github'});
  • resolving a comment thread
    'resolve-comment-thread', {package: 'github'});
  • unresolving a comment thread
    'unresolve-comment-thread', {package: 'github'});
  • adding a single comment
    'add-single-comment', {package: 'github'});
  • starting a pull request review review
    'start-pr-review', {package: 'github', component: (ReviewsFooterView or ReviewsView})
  • adding an emoji reaction
    'add-emoji-reaction', {package: 'github'});
    -removing an emoji reaction
    'remove-emoji-reaction', {package: 'github'});

Tests

QA checklist:

PR detail view
  • open reviews dock from footer in pull request detail view
  • start a review button from footer in pull request detail view takes you to dotcom
  • open diff from dock
    • activate changed files tab
    • scrolls to where the comment is
  • comment gutter decorations take you to the comment dock and highlight the selected comment for 1500ms
  • tab selection is preserved upon restart of application
Comment dock
  • empty state for reviews dock has image and "start a review" link
  • refresh button pulls down new comments
  • review dock header takes you back to PR detail item
  • checkout button
    • is enabled if you're not on a checked out pr branch
    • is disabled if you are on a checked out pr branch
    • checkout works as expected
  • multiple pages of comments are able to be fetched (set PAGE_SIZE to 1 to test this)
  • summaries collapse and expand
  • progress bar reflects the number of unresolved comments
  • on a comment thread:
    • thread collapses and expands
    • jump to file takes you to the correct line of that file
      • takes dirty working directory into account when calculating what line to jump to
      • takes you to the top of the file if comment position is null
      • jump to file is disabled with tooltip when PR is not checked out
    • renders emoji reactions
    • adding emoji reaction
    • removing an emoji reaction
    • resolving a comment thread
    • unresolving a comment thread
    • replying to a comment thread
    • check alllll the weird states a comment can be in and ensure we are handling them appropriately:
      • outdated
      • resolved
      • deleted
      • minimized
      • replies to deleted comments
      • pending
    • comment threads show up in the correct order
    • comments within threads show up in the right order
In-editor decorations
  • don't show up if you're not on a checked out pr branch
  • line highlighting shows up for lines with comments
  • gutter icon shows up for lines with comments
  • decoration position is adjusted to account for dirty working directory
  • clicking on gutter icon navigates to that comment in the reviews dock
Other
  • open reviews dock from GitHub tab if on a PR branch
    • no open reviews button if not checked out
  • community & safety QA review

Documentation

See 馃搱 Feature project

Release Notes

The GitHub integration now displays pull request review comments in a dock, and supports replying to comments, emoji reacting to comments, and resolving comments.

User Experience Research (Optional)

We conducted 6 user interviews while developing the prototype for this feature.

@codecov

This comment was marked as outdated.

Copy link

commented Mar 1, 2019

Codecov Report

Merging #1995 into master will decrease coverage by 0.58%.
The diff coverage is 83.84%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1995      +/-   ##
==========================================
- Coverage   92.58%   91.99%   -0.59%     
==========================================
  Files         189      201      +12     
  Lines       11244    11548     +304     
  Branches     1634     1666      +32     
==========================================
+ Hits        10410    10624     +214     
- Misses        834      924      +90
Impacted Files Coverage 螖
lib/containers/issueish-detail-container.js 100% <酶> (酶) 猬嗭笍
lib/atom/atom-text-editor.js 100% <酶> (酶) 猬嗭笍
lib/github-package.js 66.79% <0%> (-1.3%) 猬囷笍
lib/models/repository.js 89.78% <0%> (-6.32%) 猬囷笍
lib/containers/reviews-container.js 100% <100%> (酶)
lib/items/reviews-item.js 100% <100%> (酶)
lib/helpers.js 88.44% <100%> (-1.2%) 猬囷笍
lib/containers/pr-changed-files-container.js 100% <100%> (酶) 猬嗭笍
lib/views/observe-model.js 100% <100%> (酶) 猬嗭笍
lib/views/pr-review-comment-thread-view.js 100% <100%> (酶)
... and 42 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 40e469e...01d2609. Read the comment docs.

@todo

This comment has been minimized.

Copy link

commented Mar 4, 2019

Set this based on performance measurements' }</div>

<div className="github-Review-diffLine is-added">{ ' // TODO: Set this based on performance measurements' }</div>
<div className="github-Review-diffLine is-added">{ ' largeDiffThreshold: 100,' }</div>
</pre>
<main className="github-Review-comments">
<div className="github-Review-comment">
<header className="github-Review-header">


This comment was generated by todo based on a TODO comment in 4ab6105 in #1995. cc @atom.
@todo

This comment has been minimized.

Copy link

commented Mar 4, 2019

Set this based on performance measurements' }</div>

<div className="github-Review-diffLine is-added">{ ' // TODO: Set this based on performance measurements' }</div>
<div className="github-Review-diffLine is-added">{ ' largeDiffThreshold: 100,' }</div>
</pre>
<main className="github-Review-comments">
<div className="github-Review-comment">
<header className="github-Review-header">


This comment was generated by todo based on a TODO comment in da9c239 in #1995. cc @atom.
@todo

This comment has been minimized.

Copy link

commented Mar 4, 2019

Set this based on performance measurements' }</div>

<div className="github-Review-diffLine is-added">{ ' // TODO: Set this based on performance measurements' }</div>
<div className="github-Review-diffLine is-added">{ ' largeDiffThreshold: 100,' }</div>
</pre>
<main className="github-Review-comments">
<div className="github-Review-comment">
<header className="github-Review-header">


This comment was generated by todo based on a TODO comment in 3d6eaff in #1995. cc @atom.
@todo

This comment has been minimized.

Copy link

commented Mar 4, 2019

Set this based on performance measurements' }</div>

<div className="github-Review-diffLine is-added">{ ' // TODO: Set this based on performance measurements' }</div>
<div className="github-Review-diffLine is-added">{ ' largeDiffThreshold: 100,' }</div>
</pre>
<main className="github-Review-comments">
<div className="github-Review-comment">
<header className="github-Review-header">


This comment was generated by todo based on a TODO comment in 4c3211d in #1995. cc @atom.
@todo

This comment has been minimized.

Copy link

commented Mar 4, 2019

fixme

// TODO: fixme
const lineNumber = rootComment.position;
return (
<details className="github-Review" key={`review-comment-${rootComment.id}`}>
<summary className="github-Review-reference">


This comment was generated by todo based on a TODO comment in c2c08d7 in #1995. cc @atom.
@codecov

This comment has been minimized.

Copy link

commented Mar 14, 2019

Codecov Report

Merging #1995 into master will decrease coverage by 0.08%.
The diff coverage is 92.94%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1995      +/-   ##
==========================================
- Coverage   92.58%   92.49%   -0.09%     
==========================================
  Files         189      207      +18     
  Lines       11244    12010     +766     
  Branches     1634     1745     +111     
==========================================
+ Hits        10410    11109     +699     
- Misses        834      901      +67
Impacted Files Coverage 螖
lib/containers/remote-container.js 100% <酶> (酶) 猬嗭笍
lib/containers/current-pull-request-container.js 100% <酶> (+5%) 猬嗭笍
lib/views/issue-detail-view.js 100% <酶> (酶) 猬嗭笍
lib/views/github-dotcom-markdown.js 100% <酶> (酶) 猬嗭笍
lib/views/github-tab-view.js 100% <酶> (酶) 猬嗭笍
lib/controllers/remote-controller.js 100% <酶> (酶) 猬嗭笍
lib/controllers/github-tab-controller.js 100% <酶> (酶) 猬嗭笍
lib/controllers/multi-file-patch-controller.js 100% <酶> (酶) 猬嗭笍
lib/atom/atom-text-editor.js 100% <酶> (酶) 猬嗭笍
lib/containers/github-tab-container.js 100% <酶> (酶) 猬嗭笍
... and 81 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 b5e9a6f...dcd0ad6. Read the comment docs.

smashwilson and others added some commits Mar 27, 2019

annthurium
Render <BareReviewsController> instead of <ReviewsController>
Our tests were failing because we were rendering the fragment container 
instead of the direct component.
annthurium
unit test for `CommentDecorationsContainer` happy path
Co-Authored-By: Vanessa Yuen <vanessayuenn@users.noreply.github.com>

smashwilson and others added some commits Apr 5, 2019

Actually pass {bypassReadOnly: true} when setting text
Co-Authored-By: Ash Wilson <smashwilson@gmail.com>
@kuychaco
Copy link
Member

left a comment

Finished up reviewing the mutations and views, as well as all of the changes based on my review feedback. Everything looks good! Great job everyone! 馃檶

Left a couple of comments, nothing blocking.

It's time to 馃殺:shipit: 馃殌!!!!

Show resolved Hide resolved lib/mutations/add-pr-review-comment.js Outdated
}

// "forceRerender" ensures that the PullRequestReviewCommentThreadView re-renders each time that the
// MultiFilePatchView does. It doesn't re-query for reviews, but it does re-check patch visibility.

This comment has been minimized.

Copy link
@kuychaco

kuychaco Apr 6, 2019

Member

I believe this comment is simply stale and we don't need to worry about re-rendering, right @smashwilson?

This comment has been minimized.

Copy link
@smashwilson

smashwilson Apr 8, 2019

Author Member

Indeed it was just stale. Good catch 馃檱

</summary>
<nav className="github-Review-nav">
<button className={openFileClasses}
data-path={nativePath} data-line={lineNumber} data-diff={rootComment.diffHunk}

This comment has been minimized.

Copy link
@kuychaco

kuychaco Apr 6, 2019

Member

馃敟data-diff as we are not using it

smashwilson added some commits Apr 8, 2019

@smashwilson

This comment has been minimized.

Copy link
Member Author

commented Apr 8, 2019

I've filed issues for the remaining refactor-y bits of feedback so we don't lose track of them. Thanks for the super-thoughtful review as always, @kuychaco 馃憣 馃檱

super-pumped

shipping-time

@vanessayuenn
Copy link
Contributor

left a comment

AHHH this is so exciting! Let's ship it! :shipit: 馃殌

@smashwilson smashwilson merged commit 728b974 into master Apr 8, 2019

2 checks passed

codecov/patch 92.94% of diff hit (target 92.58%)
Details
codecov/project Absolute coverage decreased by -0.08% but relative coverage increased by +0.35% compared to b5e9a6f
Details

Sprint : 7 March 2019 - 3 April 2019 : v0.28.0 automation moved this from In progress to Merged Apr 8, 2019

@smashwilson smashwilson deleted the all/review-comments branch Apr 8, 2019

@smashwilson smashwilson referenced this pull request Apr 8, 2019

Closed

v0.28.0-0 QA Review #2051

3 of 3 tasks complete
@mcrocker

This comment has been minimized.

Copy link

commented Apr 15, 2019

This stuff looks amazing. Great work!

Does this work with GitLab Merge Requests, or are the plans too make it GitLab compatible, or is the a GitLab equivalent feature somewhere else?

@annthurium

This comment has been minimized.

Copy link
Contributor

commented Apr 15, 2019

@mcrocker thanks!

we have no plans to build a GitLab integration into our package.

Good news, though. Atom does have some community packages for GitLab integration. I'd try filing an issue at https://github.com/blakawk/gitlab-integration and see if you can work with the folks maintaining that package to get that feature request on their roadmap.

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