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

Fetch and display readonly inline comments. #1856

Merged
merged 123 commits into from Jan 11, 2019

Conversation

6 participants
@annthurium
Copy link
Contributor

annthurium commented Dec 19, 2018

Description of the Change

Another step in our quest to march ever onwards towards code review in the editor.

This pull request adds readonly comments to pull request pane, that are displayed inline with the diff.

See design here: https://github.com/atom/github/blob/master/docs/feature-requests/003-pull-request-review.md

TODO

implementation tracking project

Alternate Designs

Benefits

  • Viewing inline comments allows users to address feedback from code reviewers without context switching back and forth to GitHub dotcom.
  • Lays the groundwork for editable comments, which would allow reviewers to review code within their editor. Less context switching, and the ability to see more of the code you're reviewing, and it's easier to run the code, which should lead to easier and more thorough code reviews.

Possible Drawbacks

Applicable Issues

N/A

Metrics

  • Number of times the tab that contains the comments is viewed. This helps us understand whether users are discovering the feature.

Test Plan

  • PRs with a large # of reviews and comments should load in a reasonable amount of time
  • ensure that comment threads show up correctly when there are multiple comments on the same row
  • ensure that comment thread order is retained when viewing a pull request that has paginated comments and reviews (that is, if there are multiple comment threads, the threads show up in the right order)
  • ensure that comment order within a thread is retained when viewing a pull request that has paginated comments and reviews
  • ensure that comments are positioned on the same line of the diff as they are on github dotcom
  • single comments (that are NOT part of a review) should show up
  • resolved comments should not show up
  • outdated comments should not show up
  • replies to resolved comments should not show up
  • replies to outdated comments should not show up
  • replies to deleted comments should not show up
  • errors should be logged in the console (how should we simulate this condition? Turning off internet means you get an error loading the diff, so we won't even get to the point where we start fetching stuff.)

Release Notes

The GitHub package now shows readonly inline comments in the pull request view.

User Experience Research (Optional)

We have a user experience research script for the GitHub pane and the pull request view, that will help us understand how the information we're displaying is or is not useful to our users. We intend to resume user research in January, after the holidays.

@codecov

This comment has been minimized.

Copy link

codecov bot commented Dec 19, 2018

Codecov Report

Merging #1856 into master will decrease coverage by 0.08%.
The diff coverage is 89.83%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1856      +/-   ##
==========================================
- Coverage   91.09%   91.01%   -0.09%     
==========================================
  Files         185      189       +4     
  Lines       10719    10835     +116     
  Branches     1575     1592      +17     
==========================================
+ Hits         9765     9861      +96     
- Misses        954      974      +20
Impacted Files Coverage Δ
lib/containers/issueish-detail-container.js 100% <ø> (ø) ⬆️
lib/containers/pr-changed-files-container.js 100% <ø> (ø) ⬆️
lib/views/pr-commits-view.js 56.52% <ø> (-1.82%) ⬇️
lib/controllers/multi-file-patch-controller.js 100% <ø> (ø) ⬆️
lib/controllers/issueish-detail-controller.js 100% <ø> (ø) ⬆️
lib/views/pr-detail-view.js 100% <ø> (ø) ⬆️
lib/containers/pr-reviews-container.js 0% <0%> (ø)
lib/views/pr-review-comments-view.js 100% <100%> (ø)
lib/helpers.js 90.09% <100%> (+0.09%) ⬆️
lib/models/patch/multi-file-patch.js 100% <100%> (ø) ⬆️
... and 11 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 4786390...9f5c577. Read the comment docs.

annthurium and others added some commits Dec 19, 2018

add query to fetch reviews in the PullRequestDetailView
Co-Authored-By: Vanessa Yuen <vanessayuenn@users.noreply.github.com>
render some comments, woo
Co-Authored-By: Vanessa Yuen <vanessayuenn@users.noreply.github.com>
maybe my avatar does not need to be the giantest.
Co-Authored-By: Vanessa Yuen <vanessayuenn@users.noreply.github.com>
make comments show up at the (somewhat) correct location
Co-Authored-By: Tilde Ann Thurium <annthurium@github.com>
put in comment position also
Co-Authored-By: Tilde Ann Thurium <annthurium@github.com>
Show resolved Hide resolved lib/views/pr-comments-view.js Outdated

vanessayuenn and others added some commits Dec 21, 2018

@todo

This comment has been minimized.

Copy link

todo bot commented Dec 21, 2018

verify that this works on Windows

// TODO verify that this works on Windows
const {startBufferRow, index} = this.diffRowOffsetIndices.get(fileName);
const {offset} = index.lowerBound({diffRow}).data();
return startBufferRow + diffRow - offset;
}


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

annthurium added some commits Jan 8, 2019

don't test sorting comments by date
we don't actually sort the comments by date. They come in in order and 
then we reverse them for some reason.

@annthurium annthurium changed the title [wip] Fetch and display readonly inline comments. Fetch and display readonly inline comments. Jan 8, 2019

this.handleComments();
}

handleComments = error => {

This comment has been minimized.

@annthurium

annthurium Jan 9, 2019

Author Contributor

hmm so actually, we are logging errors and stopping fetching if the errors exist, collecting the comments, and then attempting to load more comments. So logErrors doesn't cover it all either. Not sure what a better name would be but surely we can come up with something. 🤔 anyone have suggestions?

}
}

handleError = error => {

This comment has been minimized.

@annthurium

annthurium Jan 9, 2019

Author Contributor

same as other comment above re: need a better name for this method.

state[comment.id] = [comment];
} else {
// TODO: look at this more closely...
// When comment being replied to is outdated...?? Not 100% sure...

This comment has been minimized.

@annthurium

annthurium Jan 9, 2019

Author Contributor

I tried replying to an outdated comment, and it did not show up in the pull request view. So yeah, I have no idea how we'd get into this state.

@kuychaco are there other ways you'd suggest we investigate this? I'm not sure what else to do.

This comment has been minimized.

@smashwilson

smashwilson Jan 10, 2019

Member

Maybe when replying to a deleted or hidden comment?

This comment has been minimized.

@annthurium

annthurium Jan 10, 2019

Author Contributor

okay, so I tried:

  • replying to a deleted comment
  • replying to a resolved comment
  • replying to a hidden comment

None of these triggered that condition for line 132. However, I did notice we are not respecting hidden comments. That seems like a ship blocker to me since we should not be showing users comments that have been marked as abusive or spammy.

@smashwilson smashwilson moved this from In Progress 🔧 to QA Review 🔬 in Sprint : 9 January 2019 - 12 February 2019 : v0.25.0 Jan 9, 2019

@smashwilson
Copy link
Member

smashwilson left a comment

The only blocker I see is an unresolved TODO from and for myself 😅 I'll give that a shot tonight when I'm on the Windows box and dismiss this, then we should be clear to 🚢. Nice work everyone!

commitCursor: null,
reviewCount: PAGE_SIZE,
reviewCursor: null,
commentCount: PAGE_SIZE,

This comment has been minimized.

@smashwilson

smashwilson Jan 10, 2019

Member

Do we know how this effects our rate-limit overhead? The way that GraphQL queries are rate-limited is a bit unexpected:

Individual calls cannot request more than 500,000 total nodes.

I would guess that we could drop the review and review comment page sizes to drop our computed node total without impacting the vast majority of review scenarios.

Maybe @telliott27 has some idea of the distribution of number of reviews and number of comments within a review and could help us find a lower limit that would still capture ~70% or ~80% of requests in a single page... ?

This comment has been minimized.

@telliott27

telliott27 Jan 10, 2019

I pulled some distribution stats for number of reviews and number of review comments here: https://ghe.io/telliott27/reports/tree/master/Notebooks/PR%20Review%20Comments#pr-reviews-and-comments-percentiles

The TL;DR is that it is rare for a PR to have more than 10 reviews, and for reviews to have more than 10 comments.

}

componentDidMount() {
this.handleComments();

This comment has been minimized.

@smashwilson

smashwilson Jan 10, 2019

Member

I see that we're only calling this from componentDidMount(). Is there any case in which we would want to re-collect comments again because props changed? We could make this a React.PureComponent and implement a componentDidUpdate() method as well.

This comment has been minimized.

@annthurium

annthurium Jan 10, 2019

Author Contributor

we are also calling this method in _loadMoreComments - it's the callback that gets passed to relay.loadMore. We do want to keep calling handleComments each time we receive new props, because the parent component needs to know when all the comments have been received, so it can group them by replyTo.id. Does the suggestion of making this a pure component still make sense under those conditions?

this.handleComments();
}

handleComments = error => {

This comment has been minimized.

@smashwilson

smashwilson Jan 10, 2019

Member

How about accumulateComments or acceptComments?

@@ -25,6 +25,8 @@ export default class MultiFilePatchController extends React.Component {
discardLines: PropTypes.func,
undoLastDiscard: PropTypes.func,
surface: PropTypes.func,

switchToIssueish: PropTypes.func,

This comment has been minimized.

@smashwilson

smashwilson Jan 10, 2019

Member

Ah, we need this to support hyperlinks in rendered review comment bodies. 👌

state[comment.id] = [comment];
} else {
// TODO: look at this more closely...
// When comment being replied to is outdated...?? Not 100% sure...

This comment has been minimized.

@smashwilson

smashwilson Jan 10, 2019

Member

Maybe when replying to a deleted or hidden comment?

@@ -321,6 +339,13 @@ export default class MultiFilePatch {
return false;
}

getBufferRowForDiffPosition = (fileName, diffRow) => {
// TODO verify that this works on Windows

This comment has been minimized.

@smashwilson

smashwilson Jan 10, 2019

Member

🚨 Oh right, I still need to do this. I think dotcom always gives us fileName with / separators, but we normalize them to \ before display.

This comment has been minimized.

@smashwilson

smashwilson Jan 11, 2019

Member

Spoiler alert: they were not handled correctly. It was the worst of both worlds: we were showing dotcom diffs with / separators, but also throwing an exception here when trying to load comments.

paths-not-fixed-up

I've fixed the path normalization so that the "Files" tab will (a) display paths with native path separators and (b) correctly compute comment offsets with the normalized paths.

properly-normalized

yo-dawg

image

const nativePath = toNativePathSep(rootComment.path);
const row = this.props.getBufferRowForDiffPosition(nativePath, rootComment.position);
const point = new Point(row, 0);
const range = new Range(point, point);

This comment has been minimized.

@smashwilson

smashwilson Jan 10, 2019

Member

We could probably extract a helper for "make a one-point Range at position 0 of this row". 🤔

This comment has been minimized.

@annthurium

annthurium Jan 11, 2019

Author Contributor

Hmm, this is the only place we're doing that though. So not sure we need a helper.

<img className="github-PrComment-avatar" src={author ? author.avatarUrl : ''} alt={login} />
{login} commented{' '}
<a className="github-PrComment-timeAgo" href={this.props.comment.url}>
<Timeago time={this.props.comment.createdAt} />

This comment has been minimized.

@smashwilson

smashwilson Jan 10, 2019

Member

👍 for code re-use 😄

This comment has been minimized.

@annthurium

annthurium Jan 10, 2019

Author Contributor

yeah! I did not realize Timeago existed until this pull request. We should probably be using Timeago in the PullRequestCommitView and a few other places where we're rolling our own humanizing timestamp.

wrapper.instance()._attemptToLoadMoreComments();
assert.strictEqual(relayLoadMoreStub.callCount, 0);

clock.tick(PAGINATION_WAIT_TIME_MS);

This comment has been minimized.

@smashwilson

smashwilson Jan 10, 2019

Member

Oh god setTimeout tests 🙈

hasPreviousPage: o.reviewStartCursor !== 0,
},
totalCount: o.reviewItemTotal,
};

This comment has been minimized.

@smashwilson

smashwilson Jan 10, 2019

Member

Note that we do have review and review comment builders for these 😄

annthurium and others added some commits Jan 10, 2019

move clock inside the only test that needs it
it's cleaner, and the tests were stalling when run all together (the 
individual tests worked fine)
`accumulate` is totes a better name for those functions
Co-Authored-By: Ash Wilson <smashwilson@gmail.com>
deal with minimized comments
you can mark comments as spammy, or even abusive, so I'm really glad we 
caught this before shipping.

vanessayuenn and others added some commits Jan 11, 2019

@annthurium annthurium merged commit 6a29a88 into master Jan 11, 2019

2 checks passed

codecov/patch 89.83% of diff hit (within 2% threshold of 91.09%)
Details
codecov/project 91.01% (-0.09%) compared to 4786390
Details

Sprint : 9 January 2019 - 12 February 2019 : v0.25.0 automation moved this from QA Review 🔬 to Merged ☑️ Jan 11, 2019

@annthurium annthurium deleted the 18-dec-inline-comments branch Jan 11, 2019

@annthurium

This comment has been minimized.

Copy link
Contributor Author

annthurium commented Jan 11, 2019

the last build never reported its status properly but it was green on azure pipelines I promise!

@kuychaco kuychaco referenced this pull request Feb 4, 2019

Closed

v0.25.0-0 QA Review #1936

14 of 18 tasks complete
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.