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:

Screen Shot 2019-03-29 at 7.25.59 AM.png1954×1080 223 KB

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

I hope that helps!

[ava351]

No, this is likely not intended behavior. Review comments should ideally persist in the "Files changed" tab after a commit that addresses them. This helps maintain conversation context. It might be a bug in the specific platform you're using.

[jchesshirAspire]

@ava351 Be sure to read my post about the links to other unanswered conversations.

[ava351]

alright

[bdaz]

@lee-dohm Regarding

"the automatic tools have no way of knowing if the comment targeted at the old line still makes sense on the new line."

This is a solvable problem—see how Phabricator (Differential) does it. The principles are:

1. The changeset we want to view is from the time the review was done → to the latest changes. When viewing that, review comments show up on the left when in split mode.
2. If you can't perfectly map from the context of a review comment to the latest changes, then either: (a) make a best guess, or failing that, (b) put the unmappable change at the top, with no corresponding code on the right side.

Most of the complexity of fixing this huge UX issue with GitHub is in implementing 2(a). But an implementation that only did 2(b) would be a massive upgrade!

[serhii-surazhskyi-gl]

The problem is still there.

It is frustrating and absolutely makes no sense in the normal review system to hide comments, regardless of their status.
Of course, if developers were using their review tool that would make obvious to them (yup, passive aggressive based on shithub quality)
Kindly check Gerriit implementation, how such situation is handled, even there are proposals from other developers in the thread.

"the automatic tools have no way of knowing if the comment targeted at the old line still makes sense on the new line." - that is not responsibility of the review system. It shall provide convenient way of viewing changes, commenting them and tacking review comments, it it is clear if they are fixed etc.

Why would so many people created so many comments about the issue if that was not important?

[ghost]

@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.

[Wim-Veen]

Exactly that. If I write a comment in a PR, I want to see how that comment is answered (either by reply, or by code change). If I have to look through all code changes to see what possibly has been done, it takes me ages to see if I can set the comment to resolved.

[jwosty]

What's especially frustrating is that even a one-line change causes this problem. For example:

1. Reviewer recommends a trivial change on line 100
2. Developer makes some simple one-line change on line 100 to address it

Now, the conversation is completely absent when looking at Files Changed > Changes from all commits. If the reviewer tries to use Files Changed > Conversations to locate the conversation, the comment shows up, but without the new code... Hence, the only way to verify if that newest one line change addresses the feedback, is to open two browser windows and line them up manually.

This get totally ridiculous to deal with in reviews with many trivial change requests.

[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? :slight_smile:

[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.

[Caitlin-Syntax]

Bumping this response, four years later... This is exactly what I would want.

[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.

[MichalLaczynskiAmartus]

Hi.
What is the status of the issue?
I am also in favour of having all comments in the files changed window.
Now I am forced to llok for other tool than GitHub to deal with my Pull Requests.
And that is such a simple feature to do.
There can always be a flag to trigger “show outdated”

Thanks!

[codingjeremy]

+1 to all this. I use gerrit now and it does this so it took me a long time of troubleshooting to figure out the issue wasn’t with me, but the feature isn’t present. Please fix!

[berniedurfee-renaissance]

Yep, discovered this "feature" today. Makes reviewing really tricky.

[jchesshirAspire]

I recommend commenting on one of those other two. This one might not get seen since the discussion has not been closed.

Admins, I'd like to request further comments to this post be suspended. It would also be good for one of the above linked posts be selected as the primary one to implement and the other marked as a duplicate. Which one better describes the problem is up to you. I think they both do a good job, and both highlight at least one extra problem that the other one doesn't.

[Tommuh]

almost 5 years later... unbelievable.

[IRHayfield]

I agree that GitHub should change this and make outdated comments an optional feature.
As a workaround I created outdated-parrot an action which adds all open outdated comment threads as annotations so they are displayed in the files changed view.
This is not an perfect solution, but does allow users to review comments against the correct line with the addition of some links that users may find useful.

[zebholt]

Incredibly annoying PR UI / workflow.

I found this vscode plugin today and it adds the missing feature I was looking for.
Right clicking on the "outdated" comment gives you an option to Diff comment with HEAD showing old and new code in one place.

https://marketplace.visualstudio.com/items?itemName=GitHub.vscode-pull-request-github

[taurus227]

I can't find this Diff comment with HEAD option, could you provide screenshot?

[Injamul2k24]

https://github.com/orgs/Hoqueinja/discussions/1

[jchesshirAspire]

Not able to follow this link. Please refresh or remove the comment.

[Caitlin-Syntax]

This issue is now documented in at least four places with no real official response.

• https://github.com/orgs/community/discussions/9956
• https://github.com/orgs/community/discussions/17481
• https://github.com/orgs/community/discussions/23138 (this thread)
• https://github.com/orgs/community/discussions/110796 (feature request)

[CoolGoose]

I guess we need to bump all a bit more, it's the most annoying thing coming over gitlab, initial context of seeing how the new change 'fixes it' it's important.

[serhii-surazhskyi-gl]

Up

[dmbursty]

This 'intended behaviour' doesn't make sense when using Split View, where the left side shows the same file in the exact state the comments were made. There is no reason the comments can't be attached to the appropriate lines.

[Caitlin-Syntax]

New year, new bump. This issue, which has been getting upvotes and comments across at least 4 threads since March 2019, still has no official response. My previous comment links to the threads documenting this issue and requesting UX changes. Please, please fix this, GitHub!

[elseifthen]

@lee-dohm I kindly ask you to unmark the "Answer" (or whoever marked it as an answer @renozao?) as it just brushes off what many people request and expect to be natural behavior and functionality (e.g. from gerrit) in order to do efficient code review. This is an issue that needs visibility and absolutely needs to be addressed.

[Caitlin-Syntax]

They technically did answer the question (Is this intended? Yes). It's inexplicably bad intended behavior, but it's intended. We might have better luck trying to boost this feature request: https://github.com/orgs/community/discussions/110796

[Reston]

devs should go to bitbucket see how they do this thing and copy... because there the comments are kept and you can review properly.

[layla-retalk]

This feature is also present in azure devops

[Caitlin-Syntax]

Another six months, another new bump. Still awaiting any functional response to any of the (at least) four discussions about this issue over six years. This is still making my PR review process profoundly annoying.

[LionetChen]

Bump. Azure DevOps has the "View Original Diff", which is handy. It's sorely missed from GitHub.

[b3nThomas]

So I just opened Files Changed on a PR with the new_files_changed beta active, and my outdated comments are THERE! Granted they're at the top instead of inline, but I almost jumped for joy out of my chair. Huge step in the right direction. Already reaping the benefits on the very first PR with the feature. Finally 🥲

[kielpins-nearmap]

I'm trying to find out how to activate the new_files_changed beta, but I'm not able to find it in the repo settings. Google is no help. Could you provide a screenshot? Thanks!

[stefan-schweiger]

my assumption would be to add it to the query parameters as new_files_changed=true when using the new experience, but not seeing anything different..