Review comments not shown in "Files changed" if a commit affects the comment's associated line

[renozao]

Consider the following situation:

1. Create a pull request
2. Insert comments at some individual lines
3. Submit review => One can see all the comments embedded within the files, right after their respective associated lines
4. Make and commit a change to the line associated with one of the comments, referencing the pull request in the commit message

Then, going to the “Files changed” tab of the commit, one can see the changes, as well as review comments, but the review comment that was associated with the line that has been modified is not shown.

Having the comment thread shown there would enable resolving the conversation straightway, since the commit would typically address the comment.

Is this an intended behaviour?

[lee-dohm]

Yes, this is the intended behavior. If the line that the comment was in relation to changed, the automatic tools have no way of knowing if the comment targeted at the old line still makes sense on the new line. You can, however, always see the full conversation on the Conversation tab. Any comments referencing lines that have been changed show with an “Outdated” flag:

You can see this example for yourself in my test repository.

I hope that helps!

[mronnblom-cint]

@lee-dohm 

This is a stupid and limiting design IMO. If I make a comment on a line, and that line is changed to fix what I commented about, I want to be able to verify that the comment is fixed.

I can’t do this in the Conversation tab view, since the fixed code isn’t shown there.

I can’t do this in the Files changed tab view (selecting the commit with the change), since the comment isn’t shown there.

Basically, this design forces me to open two different browser windows, and cross-comparing the Conversation view with the Files changed view.

Sure, the code file that the comment is in might have changed drastically, making the comment obsolete, but I rather think that the risk for that is very low compared to the chance of the file changing due to the actual comment.

[wlannistamps]

Unfortunately, whether or not the line changed isn’t as much of a big deal as having to pop open a new tab to review the conversations at the same time as being able to view the updated code. It is actually harder for me to track the conversation to the changed line than it would be if the comment were still attached to an incorrect line. We used to deal with that all the time in Swarm; when it was off by a few lines, it was ok because we knew from the comment what the original code was, and could scroll back to wherever the new relevant line was. But it was much easier to immediately review the new code to the comment than it is to have to pop open two tabs, and then manually try to line up the new code to the comment.

Note to others, you can almost kind of sort of simulate this if you expand the commit drop down (upper left of the code panel) to include more than the past one commit. Unfortunately, you may accidentally then display code from other commits that occurred in between. But in this way I was actually able to see the original comment, and see the new code. I did this by resetting the view to split mode. I’m not sure this always works though, or works as intended.

[aarontp]

@lee-dohm wrote:

Yes, this is the intended behavior. If the line that the comment was in relation to changed, the automatic tools have no way of knowing if the comment targeted at the old line still makes sense on the new line. 

I understand that the tooling will not know if the comment makes sense at the new line, but it could be attached to the same line in the left pane, and then it can be left up to the user whether to resolve it or not.  This would make it *so* much easier to review large pull requests with lots of comments.

In the worst case, I would prefer to have the old comment still show up, but attached to the potentially wrong line if things have changed substantially.  At least it would be near the correct code, and it wouldn’t require me to flip back and forth between two windows, and manually look up where in the code the comment applies.

Can we file a feature request somewhere?

[zeptonaut]

Totally agree. This makes it way more difficult than necessary to figure out if the comments that were marked “resolved” have in fact been resolved.

[taurus227]

I agree! This is the feature I would like to have: if I click the [Outdated] tag, I get a split view where on the left I see the commit I have commented on together with the comment and on the rigth side I see the newest code with all the changes that have happened since.

[brianhhq]

of course we need this feature.

[gpascale]

Adding my voice to those who argue the current behavior is problematic. It’s entirely confusing that some comments will still show in the “files changed” view while others don’t. For a while, I didn’t even realize this behavior was happening and I’ve been missing a lot of these disappearing comments as a result.

As others have said, it’s really not a big deal if a comment doesn’t “make sense” anymore due to a change in the file. I can easily resolve it or create a new comment. It IS a big deal if I lose track of and fail to address important comments in a PR because they are missing. Jumping back and forth between two tabs is not a solution. The files changed tab is the sensible place to do the work of reviewing a PR as that is where the context is.

EDIT: Even worse, I’ve just realized that the “outdated” comments that show in the Conversation tab will only show the outdated code. Thus, there’s no easy way for me to verify that the change has addressed the feedback in order to resolve the comment. AFAICT, the only way is to switch to the files changed tab and scroll around looking for the change in question, verify the comment is addressed, then go back to Conversation view and mark as resolved. This is totally impractical.

[lordzsolt]

This is one of the stupidest “intended behaviors” ever. You need to have 2 tabs open to review any change because “Github attaches comments to commit hashes” (probably because that was the easiest way to implement it). I want to see how a comment got fixed.

Just look at Bitbucket for example. Even if lines get updates, comments still show up at the correct place. EVEN if you force-push, the comment remains.

[lmussier]

Is there any issue related to that topic we can raise ?
I’m completely agree with above comments, this feature is useless like this.

[kunickiaj]

Yeah, this is one of the biggest gripes our users have that prevents us from migrating away from Gerrit.

[Januszpl]

@lee-dohm I also have similar experience like other. I would like to see on the Conversation tab also changed code, not only “outdated” to be sure it was fixed properly. For larger PR it is problematic not having easy way to resolve issues and be sure they were fixed in proper way.

[kostacasa]

Another commenter mentioned this, but BitBucket gets it much better than the current GitHub behavior. I’ve never had this level of confusion doing large PRs with BitBucket, switching to GitHub has made it really difficult keeping track of comments across multiple commits in a PR. The current behavior definitely deserves some more finessing, it’s such an important and core capability of GitHub and any git-based tool.