Leaving Comments Does Not Work

[harrisoncramer]

Describe the bug
When you try to leave a comment on an unchanged/undeleted/unadded line, you'll get a nil pointer dereference:

http: panic serving 127.0.0.1:60819: runtime error: invalid memory address or nil pointer dereference

Leaving comments is definitely the hardest part of this plugin. Right now we bulk attempt to send comments to three different APIs since it's not clear whether you're leaving a comment on a deleted, changed, or added line. There's probably a better way of doing this that would also potentially address this issue.

Expected behavior
We should handle this more gracefully.

[mnbroatch]

For me, the problem is not unchanged lines, rather more specifically, lines in unchanged files.

[levouh]

This is not a nil pointer error, but it looks like the same issue of not being able to comment on unchanged lines, perhaps with the improvements from #42?

Full acknowledgement that the way the Gitlab API has you post comments is horrible.

---

Edit: This also happens when trying to comment on a changed line.

[harrisoncramer]

Yes, thanks for pointing this out.

Fundamentally, the issue here is that Gitlab requires you to submit a different payload for a new discussion thread depending on whether or not the line you're commenting on has been deleted, updated, or unchanged.

Right now this plugin has no built-in mechanism for viewing changes in an MR, which makes it impossible to know what type of line you're leaving a comment on. Has the line been deleted? Modified? Added? In the near future I'm going to make delta, a pager for Git, a dependency of this project, so that we can control the view when someone is leaving comments. It'd look something like this in Neovim:

Delta shows all of the relevant changed lines in an easy-to-see view along with line numbers in the affected files. If we use this output as the "review" window we could then use the data to send the correct payload.

This is going to require a relatively large refactor of the plugin and will change some fundamental behavior, but hopefully will give us both a scrollable view of the changes and fix these API issues.

I'll drop a comment on this thread when this refactor is done, it's definitely going to be a breaking change.

[levouh]

I don't know a ton about it, but I'm curious why something like delta (mostly because it is an external tool). If it is just being used to get line numbers, that makes sense, but if it is a forced addition to a workflow it feels a bit weird when things like diffview and gitsigns, etc. already exist (not that people should be forced to use them either) that utilize mostly native solutions. Again, not saying people should be forced to use those either, but they utilize things like :h diff-mode or :h signs.

[harrisoncramer]

Yes, I'm basically leaning toward using Delta because it provides a single unified diff view (which Diffview does not provide, see: sindrets/diffview.nvim#39) and line numbers within that view with very little customization required. The maintainer of DiffView specifically recommends this tool.

Frankly this is a bit of a time/effort decision for me... Delta provides basically all of the functionality that we'll want in a "review" view out of the box, including the ability to show completely added and removed files and all of their content, and to show line numbers in a unified diff format (like Gitlab does) for every change in every modified file.

It's probably possible to do this with the built in diffing provided by Neovim, but it's certainly not my area of expertise would require a lot of trial and error and research. At the moment not something I'm willing to sink many hours of time into figuring out.

I'm aware that this will mean this plugin picks up an external dependency, but it's a well-known tool and I'm hopeful that it's quick and painless (brew install delta, e.g.) to install, and wouldn't require any additional configuration.

[harrisoncramer]

That's all to say: You're not wrong and a native approach would be possible, and probably preferable, but I don't have the bandwidth for it right now.

If someone wants to try and tackle this in the future though I'm more than happy to work with them to merge it.

It sucks because this is 100 percent due to Gitlab's rough comments API.

[harrisoncramer]

@levouh and @mnbroatch if you would be so kind as to try the feature branch mentioned above and let me know if it works for you, that'd be great. Please note that the setup function is slightly different now (breaking change).

It changes the plugin to show you the diff of an MR, and you can comment on the changed/updated lines. Please let me know if it solves your issues, I think it gives us a better overall experience too since you can now see all of the MR changes in a single review buffer, which we can center the plugin around moving forward.

[levouh]

A few notes @harrisoncramer:

• If there is a well defined "contract" for what the implementation around a "review" needs to be implemented, it might be easy to allow users to implement their own, or add different types to the "core" here. I'd be happy to submit one that utilizes more "native" methods, as everyone has their own preference about how to view diffs, or might want a language server running against the buffer to check for issues, etc.
  • In the changes you've pushed (and this is nothing against the changes by the way - the fact you've blessed us with anything at all is phenomenal), it might be easier to "abstract" things into:
    • Starting a review can call some initialization around a Review "class"
    • During this phase, the implementation can setup key bindings or run delta or whatever they need to do
    • When a user wants to leave a comment, the action from the "core" is to grab the current Review instance and ask it for the information that needs to be submitted to Gitlab. What file is this for? What line? Is it changed or unchanged or removed?
  • I have little insight into what else is required as I have only started digesting your PR here, but this does not seem like a ton of overhead and would make other implementations far easier to add.
• I am not sure what the right command is, but running git diff --cached %s with the target branch can produce a massive diff. compared to what is shown in Gitlab. The source branch I'm using is intentionally out of date versus the merge base to see how this behaves. To get something more similar to what Gitlab shows, I believe you want to use git diff %s...HEAD (see here more information on the ... syntax).
• Doing require("gitlab").review() and trying to leave a comment on the first added line (in the "review" buffer, so the delta pager) for a file shows:

• The same happens for an unchanged line, I did not try removed lines as the MR I'm testing with does not have any. I will update it to have some though. Obviously this is weird to debug (didn't see anything useful in the logs either), but here is a preview of what I'm looking at:

(Note I did change the git diff --cached %s to the alternative ... syntax along with my configuration to be compliant with the new format, but otherwise this is fresh off this branch).

---

Appreciate the (quick) work, and will update here if I find anything else.

[harrisoncramer]

I really like the idea of having separate reviewers, I think that's a great idea. Also, thank you so much for the catch on the delta options -- I've added your changes to the MR.

I'm game to refactor this MR to be more abstract. I think getting everything working with Delta first (with perhaps an abstraction around the reviewer like you mentioned, I'm happy to take a crack at it if you'd like) is perhaps the best plan of attack, and then you can follow up with a subsequent MR working on a native approach using the same abstraction?

To answer your questions: "When a user wants to leave a comment, the action from the "core" is to grab the current Review instance and ask it for the information that needs to be submitted to Gitlab. What file is this for? What line? Is it changed or unchanged or removed?"

In init.lua all of the actions are wrapped in functions with the ensure prefix. These functions call APIs to "ensure" that the state is set that those other actions require to run. For instance:

M.add_reviewer           = M.ensureState(M.ensureProjectMembers(assignees_and_reviewers.add_reviewer))

This will ensure that state is set that's required for those actions. In this case, the ensure functions call two different endpoints, the "/info" and "/members" endpoints, to set state on the require("gitlab').state table.

These are the actions that would have to be tied to a "reviewer" abstraction:

M.review                 = M.ensureState(review.open)
M.create_comment         = M.ensureState(M.ensureRevisions(comment.create_comment))
M.edit_comment           = M.ensureState(comment.edit_comment)
M.delete_comment         = M.ensureState(comment.delete_comment)

Your idea of breaking these into a separate Lua review module is a great idea. For instance, refactored they might look like this:

M.review                 = M.ensureState(reviewer.open)
M.create_comment         = M.ensureState(M.ensureRevisions(reviewer.create_comment))
M.edit_comment           = M.ensureState(reviewer.edit_comment)
M.delete_comment         = M.ensureState(reviewer.delete_comment)

The reviewer.open() function would likely be responsible for initializing the reviewer, kind of like a class constructor. The other functions could basically be methods on that module, which pass state back to the core module that performs the actual API calls, without regard for which reviewer is performing the action.

For instance right now the confirm_create_comment() function (which is in lua/gitlab/comment.lua is closely coupled to Delta because the function fetches information from the Delta buffer:

-- This function (settings.popup.perform_action) will send the comment to the Go server
M.confirm_create_comment = function(text)
  local line_num = u.get_current_line_number()
  local content = u.get_line_content(state.REVIEW_BUF, line_num)
  local current_line_changes = discussions.get_change_nums(content)
  local new_line = u.get_line_content(state.REVIEW_BUF, line_num + 1)
  local next_line_changes = discussions.get_change_nums(new_line)

...

  local file_name = discussions.get_file_from_review_buffer(line_num)

...

  local revision = state.MR_REVISIONS[1]
  local jsonTable = {
    comment = text,
    file_name = file_name,
    old_line = current_line_changes.old_line,
    new_line = current_line_changes.new_line,
    base_commit_sha = revision.base_commit_sha,
    start_commit_sha = revision.start_commit_sha,
    head_commit_sha = revision.head_commit_sha,
  }

  local json = vim.json.encode(jsonTable)

  job.run_job("comment", "POST", json, function(data)
    vim.notify("Comment created")
    discussions.refresh_tree()
  end)

If the reviewer module was abstracted, it could be reused, something like this:

M.confirm_create_comment = function(text)
  local reviewer_data, reviewer_data_valid, err = reviewer.data()
  if err then vim.notify(err.error(), vim.log.levels.ERROR) end
  local revision = state.MR_REVISIONS[1]
  local jsonTable = {
    comment = text,
    file_name = reviewer_data.file_name,
    old_line = reviewer_data.current_line_changes.old_line,
    new_line = reviewer_data.current_line_changes.new_line,
    base_commit_sha = revision.base_commit_sha,
    start_commit_sha = revision.start_commit_sha,
    head_commit_sha = revision.head_commit_sha,
  }

  local json = vim.json.encode(jsonTable)

  job.run_job("comment", "POST", json, function(data)
    vim.notify("Comment created")
    discussions.refresh_tree()
  end)
end

** Regarding the errors that you're seeing right now -- have you pushed up your changes? I've noticed that the POST will fail if you have committed changes locally that don't match your remote repository. Additionally, if you make changes, commit, push, and then try to leave a comment without leaving Neovim then you'll probably see that error. In both cases, it's because the SHAs in your state.MR_REVISIONS will be out of date.

This feels like a separate bug. Please let me know if this is your problem or if it's something else...

[mnbroatch]

Hmm, I'm seeing some issues:

• listing discussions and hitting o on a comment now gives me

E5108: Error executing lua: ...l/share/nvim/lazy/gitlab.nvim/lua/gitlab/discussions.lua:128: Expected Lua number
stack traceback:
        [C]: in function 'nvim_buf_get_lines'
                ...l/share/nvim/lazy/gitlab.nvim/lua/gitlab/discussions.lua:128: in function 'get_review_buffer_range'
                        ...l/share/nvim/lazy/gitlab.nvim/lua/gitlab/discussions.lua:72: in function 'jump_to_location'
                                ...l/share/nvim/lazy/gitlab.nvim/lua/gitlab/discussions.lua:166: in function <...l/share/nvim/lazy/gitlab.nvim/lua/gitlab/discussions.lua:165>

I think it expects me to have opened the review view instead of discussion list. If that's required it should be enforced

• If I do have the review mode open, lines in the diff aren't wrapping correctly. There is horizontal scroll on the diff in my full screen terminal window.

  • It looks like the diff is rendering, then the comments split is popping in and just shoving everything to the right. I think we need to make the split then generate the diff

• opening a review without pulling the target branch gives an error. Maybe it always did?

[mnbroatch]

yea, actually commenting isn't working at all for me. I highlight a line in the side-by-side diff (I set side-by-side mode for delta in .gitconfig) and glc gives me

E5108: Error executing lua: ...al/share/nvim/lazy/gitlab.nvim/lua/gitlab/utils/init.lua:254: attempt to index local 's' (a nil value)
stack traceback:
        ...al/share/nvim/lazy/gitlab.nvim/lua/gitlab/utils/init.lua:254: in function 'trim'
                ...l/share/nvim/lazy/gitlab.nvim/lua/gitlab/discussions.lua:119: in function 'get_change_nums'
                        ...local/share/nvim/lazy/gitlab.nvim/lua/gitlab/comment.lua:30: in function 'action'
                                ...ocal/share/nvim/lazy/gitlab.nvim/lua/gitlab/settings.lua:12: in function <...ocal/share/nvim/lazy/gitlab.nvim/lua/gitlab/settings.lua:9>

[mnbroatch]

In the old diffview workflow, diffview gave me a browsable tree of changed files. Is that possible with the new workflow?

edit: Oh I see all changed files are there in the diff one after the other. That's not quite as nice, huh

[harrisoncramer]

1. You can't use a side-by-side mode, the whole point of using Delta was to get the unified diff numbers in a single buffer. As Levouh mentioned having a side-by-side diff might be possible in the future if someone wants to put in an MR, once I've abstracted the reviewer module (I'll do that in this MR).
2. You cannot have the discussion tree open apart outside of the diff view. I'll try to enforce that, thanks for catching it.
3. Trying to review something that you do not have locally (the target branch) will throw an error since you do not have the target branch on your machine. That has always been the case, but it's an issue and we could probably fix it in the future with a watcher on the .git repository or something.

I'll post something in this thread once I've made some progress on abstracting the reviewer module.

[levouh]

A few messages out of sync now so pardon me - replying to this message

---

I think getting everything working with Delta first [...] is perhaps the best plan of attack

Absolutely agree. It will help define what the abstract version actually needs to implement to function, with a functional baseline using delta 👍.

and then you can follow up with a subsequent MR working on a native approach using the same abstraction

Can certainly try.

To answer your questions: [...]

These were more poking at what the abstract class might need to implement as "questions" asked of it by the "core engine", but either way, your notes certainly help provide some insight into what the code is doing.

If the reviewer module was abstracted, it could be reused, something like this: [...]

This looks exactly like what I imagined and makes implementing different "reviewers" far simpler than the tight coupling that exists right now 👍.

Regarding the errors that you're seeing right now -- have you pushed up your changes? I've noticed that the POST will fail if you have committed changes locally that don't match your remote repository. Additionally, if you make changes, commit, push, and then try to leave a comment without leaving Neovim then you'll probably see that error. In both cases, it's because the SHAs in your state.MR_REVISIONS will be out of date.

My local state was actually just git fetch && git checkout origin/branch to make sure I was in sync, so no local changes.

This feels like a separate bug. Please let me know if this is your problem or if it's something else...

Fully agree about the notion of "state mismatch" being a bug. I wonder if having a defined "pipeline" or some ensureRevision that checks the "validity" of the current state before dependent actions would help. E.g. if the HEAD changed underneath our feet, then just tell the user the state is mismatched and they will have to re-do the MR "pipeline" by re-running require("gitlab").review(), or similar. I think getting the primary workflows ironed out makes more sense to start out though, and then ironing out the common bugs/issues as things mature a bit more.

[levouh]

And now replying to this message -

---

Trying to review something that you do not have locally (the target branch) will of course throw an error since you do not have the target branch on your machine. That has always been the case

Obviously turning this into a "git library" is not the goal, but some basic helpers might go a long way. E.g. the branch can be compared against a remote, say git diff origin/main...origin/feature and the user never has to pull the branch down. As it stands right now it seems the "entrypoint" for the whole pipeline is to assume the current branch is the MR we want to look at, but obviously in the future that could be something to add - e.g. select a branch first, etc.

Just food-for-thought though - I think getting the core workflow nailed down a bit first makes infinitely more sense, then seeing how the more QOL features fit in after the fact. Obviously depending on how the early development goes, it might be easier/harder to implement features down the road, but foresight into "all possible features" is also impossible.

[harrisoncramer]

Okay, pretty major refactor is complete, but the Delta reviewer code is now separated from the rest of the application.

Additionally there was a major cleanup of the entire code-base, I think you'll find it much easier to read through. This is a breaking change so I'll make sure to make a note of that after merging it in, some of the settings have changed (and delta has been added as a requirement until we build another solution).

Given the size of this MR it makes sense to do follow-up bug tracking/squashing in separate follow-up MRs. Please feel free to open up new, specific issues for anything you find. Please note that the setup function arguments have changed.