gitlab.nvim
gitlab.nvim copied to clipboard
harrisoncramer
Bug: Comment Locations Become "Out of Date" When SHA Changes
Prerequsities
- [x] The "Troubleshooting" section of the README did not help
- [x] I've installed the required dependencies
- [x] I'm on the latest version of the plugin
Bug Description
In a MR that I'm currently working on, some notes are not shown in the correct place. The problem is like this:
- In the original file there was some text on line 28.
- Some later revisions moved that text to line 27.
- One colleague made some changes on that line so it showed up in the MR review.
- In a later revision, the line moved to line 32.
- Another colleague didn't like that line and made a comment on it
- In yet another revision the line was deleted completely.
- Now,
gitlab.nvimshows the diagnostic with the discussion still on line 27 in the "new" part of the reviewer, where there is no trace of the original text from line 27. And it shows the discussion sign (💬) on line 32 in the "new" part of the reviewer. - Now, it's rather difficult to tell what the correct place for the diagnostic and discussion sign should be, since the reviewer in
gitlab.nvimonly shows the target of the merge and the latest revision. So on the "old" side in the reviewer, the text is shown on line 28, and on the "new" side the text is missing. But the comment was made on line 27 - I've pasted a snippet from the log at the bottom which shows the relevant line numbers.
I guess this diagnostic should only be shown if gitlab.nvim was able to show the relevant revision which really had the text on line 27. This brings me to the idea whether gitlab.nvim could be set up to show an arbitrary revision, e.g., just what has changed between the last two commits. @harrisoncramer do you think this would be possible?
Screenshots
Other Details
This is the relevant part of my gitlab.nvim.log:
{
"id": "a4263882de942bf21dfec69a4ad2e96be2089e4a",
"individual_note": false,
"notes": [
{
"id": 89132,
"type": "DiffNote",
"body": "This does not sound right to me...",
"attachment": null,
"author": {...},
"created_at": "2024-01-09T11:19:03.419Z",
"updated_at": "2024-01-11T09:23:33.928Z",
"system": false,
"noteable_id": 6948,
"noteable_type": "MergeRequest",
"project_id": 341,
"commit_id": null,
"position": {
"base_sha": "56e4b05af9d933233c4c931a0f709141c4f125e7",
"start_sha": "38f3e2845c5ecbbe268fd0065ae982d7e24531e2",
"head_sha": "a82704c051e3fe779c40da933929f1a1359a1308",
"old_path": "doc/README.md.tmpl",
"new_path": "doc/README.md.tmpl",
"position_type": "text",
"old_line": null,
"new_line": 32,
"line_range": {
"start": {
"line_code": "f99baa27fa71c7c920fbeb33a18474a49df2ce03_29_27",
"type": "new",
"old_line": null,
"new_line": 27,
},
"end": {
"line_code": "f99baa27fa71c7c920fbeb33a18474a49df2ce03_29_27",
"type": "new",
"old_line": null,
"new_line": 27,
},
},
},
"resolvable": true,
"resolved": true,
"resolved_by": {...},
"resolved_at": "2024-01-11T09:23:33.928Z",
"confidential": false,
"internal": false,
"noteable_iid": 86,
"commands_changes": {},
},
],
},
Hey thanks for filing this issue, it's a tricky one.
I'm wondering if we can make Gitlab do most of the work for us here. Gitlab already detects these types of problematic comments, for instance comments where the line is moved, and then appends a comment: "changed this line [version 3 of the diff]".
This link (and perhaps also the metadata of the node, I'm not sure) also have the original line number and hash of the MR baked in. I wonder if we could take advantage of that. For instance, we might be able to:
- Prior to placing any diagnostic signs, check to see if there is a reply in the discussion tree with this reply structure
- If there is, then do not place the diagnostic sign
- Additionally, for the "jump to line" functionality, we could instead open up a new tab and open the file at the specific commit hash where the comment was originally added to, and just jump to the line, so that you could see what the original content of the line was.
Hi @harrisoncramer, thanks for looking at this. It's indeed tricky. I've come up with another example where the changed line numbers cause a problem -- if the original line number does not exist in the file anymore (the original comment appeared say on line 158, but now the file is only 157 lines long), then I get this error:
E5108: Error executing lua: ...e/nvim/lazy/gitlab.nvim/lua/gitlab/reviewer/diffview.lua:89: Cursor position outside buffer
stack traceback:
[C]: in function 'nvim_win_set_cursor'
...e/nvim/lazy/gitlab.nvim/lua/gitlab/reviewer/diffview.lua:89: in function 'jump'
...lazy/gitlab.nvim/lua/gitlab/actions/discussions/init.lua:344: in function 'jump_to_reviewer'
...lazy/gitlab.nvim/lua/gitlab/actions/discussions/init.lua:571: in function <...lazy/gitlab.nvim/lua/gitlab/actions/discussions/init.lua:569>
I like your suggested solution. But I'm also thinking about cases when the comment was made on a specific change - then it would be helpful to see what the change was so instead of just opening the file at a specific commit, it might be practical to open another diffview which would show the relevant diff (the target of the merge vs the revision that was commented on) and the comment.
On the other hand, in some cases in our MRs a reviewer suggests some changes to the code under review, the programmer makes some changes to the code and then it would be helpful to easily see what changes were actually done, so the the new diffview should show A. the version that was commented on and B. the version that changed it (I believe that's Gitlab's "[version 17 of the diff](...)" that you mention).
Opening a new diffview would require some changes in gitlab.nvim though, because of a bug I'm just about to report :)
For the beginning just showing the note in the correct place albeit in a single-file (i.e., non-diff) view would be helpful.
Hi @harrisoncramer,
I've got another example of the misaligned diagnostics. The Discussion sign "💬" is shown correctly on line 109, whereas the range and the diagnostic is shown on lines 101-105 which were correct in the previous revision, now correct would be 105-109:
This is an excerpt from the log:
{
"id": "e15fd68f3b1ee14cbc1a1eafc54f354007276e62",
"individual_note": false,
"notes": [
{
"id": 90758,
"type": "DiffNote",
"body": "...",
"attachment": null,
"author": {"..."},
"created_at": "2024-01-17T18:37:35.956Z",
"updated_at": "2024-01-17T18:38:51.746Z",
"system": false,
"noteable_id": 7064,
"noteable_type": "MergeRequest",
"project_id": 42,
"commit_id": null,
"position": {
"base_sha": "9e772112d56512ab8a9f87a7a8fd85e37cbb73f1",
"start_sha": "9e772112d56512ab8a9f87a7a8fd85e37cbb73f1",
"head_sha": "db7559499880de943a0012db1feaa740475c3dac",
"old_path": "...",
"new_path": "...",
"position_type": "text",
"old_line": 68,
"new_line": 109,
"line_range": {
"start": {
"line_code": "8974ff47cec3c6dd8260df581dc6df7040db82fd_67_101",
"type": "new",
"old_line": null,
"new_line": 101,
},
"end": {
"line_code": "8974ff47cec3c6dd8260df581dc6df7040db82fd_68_105",
"type": null,
"old_line": 68,
"new_line": 105,
},
},
},
"resolvable": true,
"resolved": false,
"resolved_by": null,
"resolved_at": null,
"confidential": false,
"internal": false,
"noteable_iid": 283,
"commands_changes": {},
},
],
}
It shows that the line range should actually be something like (position.new_line - (position.line_range.end.new_line - position.line_range.start.new_line)) -> position.new_line, i.e., you'd only use position.line_range values to calculate the range length, but use position.new_line to determine the actual position. I don't know if this would be a universal solution for this kind of misalignments, however, this patch fixes the position of the diagnostic for the example in this post (the range indicator in the sign column is still off though, as it's managed elsewhere):
diff --git a/lua/gitlab/actions/discussions/signs.lua b/lua/gitlab/actions/discussions/signs.lua
index 142a5f9..d678228 100644
--- a/lua/gitlab/actions/discussions/signs.lua
+++ b/lua/gitlab/actions/discussions/signs.lua
@@ -167,8 +167,8 @@ M.parse_diagnostics_from_discussions = function(discussions)
}
else
new_diagnostic = {
- lnum = end_new_line - 1,
- end_lnum = start_new_line - 1,
+ lnum = first_note.position.new_line - 1,
+ end_lnum = first_note.position.new_line - 1 - (end_new_line - start_new_line),
}
end
new_diagnostic = vim.tbl_deep_extend("force", new_diagnostic, diagnostic)
I'm wondering whether DiffviewFileHistory could be used to show notes on relevant older commits. Is it actually possible to get the commit hashes from the information Gitlab sends? Possibly any of these: "base_sha", "start_sha", "head_sha"?
Yes, we have access to all of those values, they are stored in lua=require("gitlab.state").INFO.diff_refs