search

wandersoncferreira / code-review

Code Reviews in Emacs
https://wandersoncferreira.github.io/code-review/
GNU General Public License v3.0
462 stars 49 forks source link

Github review comments are on wrong line #216

Open RagnarGrootKoerkamp opened 2 years ago

RagnarGrootKoerkamp commented 2 years ago

First, thanks for this project! It looks very promising!

Describe the bug When making comments/suggestions on a PR, sometimes they end up on the wrong line.

To Reproduce A simple reproduction I tried worked correctly, but comments on a larger PR sometimes end up on the wrong line. see below.

Expected behavior A comment/suggestion made on a line in emacs ends up on (just below) the same line in github UI.

Screenshots

Comment made just below fn str_CIGAR: image Same comment as shown on GitHub here: image After refreshing the review, that comment still shows up below the function. Here I also also added a suggestion: image After submitting the comment and refreshing, again it's shifted by 1 line. And GitHub has changed the line to be changed to the for ... instead of the str.push_str(.... image

Earlier comments made even larger jumps. E.g. this suggestion was made on line 63 at fn str_CIGAR in emacs, but ended up on line 77 in Github, which was then moved to line 22 of the previous commit since the most recent commit does not touch it.

Desktop (please complete the following information):

wandersoncferreira commented 2 years ago

I'm investigating this issue at the moment. I need to understand a bit better how magit-section works when the section is hidden. This seems the root cause of issues related to comments positioning and also some issues with inconsistent behavior when we hide sections via TAB

wandersoncferreira commented 2 years ago

@RagnarGrootKoerkamp have you made some of these PR comments from the commits tab? Back in the days I've looking at the responses from Github API to actions performed in the Commit tab I got some unexpected results back. This might be a good starting point to investigate this.

RagnarGrootKoerkamp commented 2 years ago

Hmm, no. All the comments were either made in the files changed github tab, or in emacs, as far as I remember.

The ones made on github work fine and end up on the right lines.

The issue seems to be that code-review passes a wrong line number to github when first pushing the comments.

wandersoncferreira commented 2 years ago

Got it, thanks.

The issue seems to be that code-review passes a wrong line number to github when first pushing the comments.

Yes, but the wrong line number might be computed due to mis-interpretation of either other comments fetched from Github above the line your are working on or outdated sections. The way we handle outdated comments and threads is not good in anyway right now.

Specially because we need to place it in the diff hunk section which is very complicated. I'm considering moving all the outdated comments to a dedicated section before or after the Files Changed section. wdyt?

I'll keep looking into it. Do you mind if I push some comments to the PR you linked in the issue here?

RagnarGrootKoerkamp commented 2 years ago

Got it, thanks.

The issue seems to be that code-review passes a wrong line number to github when first pushing the comments.

Yes, but the wrong line number might be computed due to mis-interpretation of either other comments fetched from Github above the line your are working on or outdated sections.

Right, makes sense.

I'll keep looking into it. Do you mind if I push some comments to the PR you linked in the issue here?

Sure, go ahead.

Specially because we need to place it in the diff hunk section which is very complicated. I'm considering moving all the outdated comments to a dedicated section before or after the Files Changed section. wdyt?

Hmm. If by outdated you mean 'relating to a line of code that was since changed', yes, that sounds fine. (If you mean 'any comment not on the most recent commit', I would disagree.)

My biggest complaint with Github reviewing is that it's somewhat hard to find old/outdated comments. Ideally (long term), it would be possible to select a range of commits and show all comments relating to the before and after state of this range. This could also include comments on other commits, as long as they relate to a line present in either the start or end state of the selected range.