Review comments not shown in "Files changed" if a commit affects the comment's associated line #23138
-
|
Consider the following situation:
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? |
Beta Was this translation helpful? Give feedback.
Replies: 58 comments 21 replies
-
|
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! |
Beta Was this translation helpful? Give feedback.
-
|
extremely frustrating to change something in a line that's unrelated to a discussion happening in the context of that line and then have people who are reviewing the code from the code view entirely miss that context/discussion. plenty of options to fall back on if you can't "be sure" its the same line! leave it there but put a note that context might have changed! move it to the top or bottom of the file! this behavior also encourages people to use comments that aren't inline, which is awful due to the entirely separate github issue of being unable to reply to top level comments directly |
Beta Was this translation helpful? Give feedback.
-
|
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. |
Beta Was this translation helpful? Give feedback.
-
|
@ava351 Be sure to read my post about the links to other unanswered conversations. |
Beta Was this translation helpful? Give feedback.
-
|
alright |
Beta Was this translation helpful? Give feedback.
-
|
@lee-dohm Regarding
This is a solvable problem—see how Phabricator (Differential) does it. The principles are:
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! |
Beta Was this translation helpful? Give feedback.
-
|
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. |
Beta Was this translation helpful? Give feedback.
-
|
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. |
Beta Was this translation helpful? Give feedback.
-
|
What's especially frustrating is that even a one-line change causes this problem. For example:
Now, the conversation is completely absent when looking at This get totally ridiculous to deal with in reviews with many trivial change requests. |
Beta Was this translation helpful? Give feedback.
-
|
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. |
Beta Was this translation helpful? Give feedback.
-
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: |
Beta Was this translation helpful? Give feedback.
-
|
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. |
Beta Was this translation helpful? Give feedback.
-
|
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. |
Beta Was this translation helpful? Give feedback.
-
|
Bumping this response, four years later... This is exactly what I would want. |
Beta Was this translation helpful? Give feedback.
-
|
of course we need this feature. |
Beta Was this translation helpful? Give feedback.
-
|
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. |
Beta Was this translation helpful? Give feedback.
-
|
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. |
Beta Was this translation helpful? Give feedback.
-
|
Is there any issue related to that topic we can raise ? |
Beta Was this translation helpful? Give feedback.
-
|
Yeah, this is one of the biggest gripes our users have that prevents us from migrating away from Gerrit. |
Beta Was this translation helpful? Give feedback.
-
|
@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. |
Beta Was this translation helpful? Give feedback.
-
|
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. |
Beta Was this translation helpful? Give feedback.
-
|
Hi. Thanks! |
Beta Was this translation helpful? Give feedback.
-
|
+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! |
Beta Was this translation helpful? Give feedback.
-
|
Adding my voice to the choir. It is ridiculous that this issue is being ignored for so long. |
Beta Was this translation helpful? Give feedback.
-
|
Adding my voice |
Beta Was this translation helpful? Give feedback.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
-
|
Bump, this is a big UX issue |
Beta Was this translation helpful? Give feedback.
-
|
Adding my voice to this. |
Beta Was this translation helpful? Give feedback.
-
|
Googled "github make comment not outdated" only to find this thread. |
Beta Was this translation helpful? Give feedback.
-
|
Hey folks, there's an Unanswered Post out there that's complaining about practically the same thing as this one. Perhaps extra comments on that one will actually get Github's attention. |
Beta Was this translation helpful? Give feedback.
-
|
Here is another unanswered post on the same issue |
Beta Was this translation helpful? Give feedback.
-
|
Yep, discovered this "feature" today. Makes reviewing really tricky. |
Beta Was this translation helpful? Give feedback.
-
|
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. |
Beta Was this translation helpful? Give feedback.
-
|
almost 5 years later... unbelievable. |
Beta Was this translation helpful? Give feedback.
-
|
I agree that GitHub should change this and make outdated comments an optional feature. |
Beta Was this translation helpful? Give feedback.
-
|
Incredibly annoying PR UI / workflow. I found this vscode plugin today and it adds the missing feature I was looking for. |
Beta Was this translation helpful? Give feedback.
-
Beta Was this translation helpful? Give feedback.
-
|
Not able to follow this link. Please refresh or remove the comment. |
Beta Was this translation helpful? Give feedback.
-
|
This issue is now documented in at least four places with no real official response.
|
Beta Was this translation helpful? Give feedback.
-
|
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. |
Beta Was this translation helpful? Give feedback.
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!