Draft: Feat: Add suggestions functionality #504
Conversation
| vim.api.nvim_cmd({ cmd = "tabnew", args = { original_buf_name } }, {}) | ||
| local original_buf = vim.api.nvim_get_current_buf() | ||
| local original_winid = vim.api.nvim_get_current_win() | ||
| vim.api.nvim_buf_set_lines(original_buf, 0, -1, false, original_lines) | ||
| vim.bo.bufhidden = "wipe" | ||
| vim.bo.buflisted = false | ||
| vim.bo.buftype = "nofile" | ||
| vim.bo.modifiable = false | ||
| vim.cmd.filetype("detect") | ||
| local buf_filetype = vim.api.nvim_get_option_value("filetype", { buf = 0 }) | ||
|
|
||
| local imply_local = determine_imply_local(opts) | ||
|
|
||
| -- Create the suggestion buffer and show a diff with the original version | ||
| local split_cmd = vim.o.columns > 240 and "vsplit" or "split" | ||
| if imply_local then | ||
| vim.api.nvim_cmd({ cmd = split_cmd, args = { opts.new_file_name } }, {}) | ||
| else | ||
| local sug_buf_name = get_temp_file_name("SUGGESTION", opts.note_node_id or "NEW_COMMENT", commented_file_name) | ||
| vim.api.nvim_cmd({ cmd = split_cmd, args = { sug_buf_name } }, {}) | ||
| vim.bo.bufhidden = "wipe" | ||
| vim.bo.buflisted = false | ||
| vim.bo.buftype = "nofile" | ||
| vim.bo.filetype = buf_filetype | ||
| end | ||
| local suggestion_buf = vim.api.nvim_get_current_buf() | ||
| local suggestion_winid = vim.api.nvim_get_current_win() | ||
| set_buffer_lines(suggestion_buf, suggestions[1].full_text, imply_local) | ||
| vim.cmd("1,2windo diffthis") | ||
|
|
||
| -- Backup the suggestion buffer winbar to reset it when suggestion preview is closed. Despite the | ||
| -- option being "window-local", it's carried over to the buffer even after closing the preview. | ||
| -- See https://github.com/neovim/neovim/issues/11525 | ||
| local suggestion_winbar = vim.api.nvim_get_option_value("winbar", { scope = "local", win = suggestion_winid }) | ||
|
|
||
| -- Create the note window | ||
| local note_buf = vim.api.nvim_create_buf(false, false) | ||
| local note_winid = vim.fn.win_getid(3) | ||
| local note_bufname = vim.fn.tempname() | ||
| vim.api.nvim_buf_set_name(note_buf, note_bufname) | ||
| vim.api.nvim_cmd({ cmd = "vnew", mods = { split = "botright" }, args = { note_bufname } }, {}) | ||
| vim.api.nvim_buf_set_lines(note_buf, 0, -1, false, note_lines) | ||
| vim.bo.bufhidden = "wipe" | ||
| vim.bo.buflisted = false | ||
| vim.bo.filetype = "markdown" | ||
| vim.bo.modified = false |
There was a problem hiding this comment.
This part is pretty ugly and the buffer/tab/split creation should probably be handled in a cleaner and simpler way. Possibly by using something like Nui.Split.
| disable_all = false, -- Disable all default mappings for the reviewer windows | ||
| create_comment = "c", -- Create a comment for the lines that the following {motion} moves over. Repeat the key(s) for creating comment for the current line | ||
| create_suggestion = "s", -- Create a suggestion for the lines that the following {motion} moves over. Repeat the key(s) for creating comment for the current line | ||
| create_suggestion_with_preview = "S", -- In a new tab create a suggestion with a diff preview for the lines that the following {motion} moves over. Repeat the key(s) for creating comment for the current line |
There was a problem hiding this comment.
This shouldn't be just "S" since that's a native Vim binding and won't trigger right away.
There was a problem hiding this comment.
Hi Harrison, thanks for looking at this! I'm using exactly this mapping without a problem. The builtin "S" is remapped the same was as "s" for "create_suggestion". The builtin behaviour is "delete lines", so it does trigger right away, what may be happening is that this mapping conflicts with another plugin (Folke's Flash suggests to remap "S", so maybe that). This can be fixed with the create_suggestion_with_preview_nowait = true, setting in gitlab.nvim config.
But of course I can choose a different default and remap it in my personal config if you prefer that.
| ---Open a new tab with a suggestion preview. | ||
| ---@param tree NuiTree The current discussion tree instance. | ||
| ---@param action "reply"|"edit"|"apply" Reply to the current thread, edit the current comment or apply the suggestion to local file. | ||
| M.suggestion_preview = function(tree, action) |
There was a problem hiding this comment.
This isn't working for me, the suggestion is hidden, and slowly expands each time I press a character
funky-scroll.behavior.mov
There was a problem hiding this comment.
I remember that I had the behaviour you describe before but then I added a fix for that, so maybe you need to pull the latest feat-add-suggestions-functionality branch.
I'm still getting a similar behaviour when I leave the suggestion intact and only modify the comment above the" ```suggestion:-0+0" line. But as soon as I make a modification in the suggestion body itself, the diff is shown correctly. It would be best to fix this as well, I've experimented with different folding normal mode commands instead of zX, but haven't found a solution yet.
There was a problem hiding this comment.
I've just pushed a fix that gets rid of the behaviour completely.
| apply_suggestion = "sa", -- Apply the suggestion to the local file with a preview in a new tab | ||
| }, | ||
| suggestion_preview = { | ||
| apply_changes = "ZZ", -- Close suggestion preview tab, and post suggestion comment to Gitlab (and discard changes to local file) or "apply" changes to local file |
There was a problem hiding this comment.
Was getting these errors sometimes on submit:
gitlab.nvim: Could not create discussion: POST https://gitlab.com/api/v4/projects/45056705/merge_requests/49/discussions: 400 {message: 400 Bad request - Note {:line_code=>["can't be blank", "must be a valid line code"]}}
There was a problem hiding this comment.
This is probably not related to the changes introduced in this MR but is a general problem of how the range data are gathered. That's why I reopened the #386 some time ago.
| if action == "apply" and not is_new_sha then | ||
| local range = end_line - start_line | ||
| start_line = common.get_new_line(root_node) | ||
|
|
||
| if start_line == nil then | ||
| u.notify("Couldn't get position in new version. Can't create suggestion preview", vim.log.levels.ERROR) | ||
| return | ||
| end | ||
|
|
||
| end_line = start_line + range | ||
| is_new_sha = true | ||
| end | ||
|
|
||
| -- Get values for preview depending on whether comment is on OLD or NEW version | ||
| local revision | ||
| if is_new_sha then | ||
| revision = common.commented_line_has_changed(tree, root_node) and root_node.head_sha or "HEAD" | ||
| else | ||
| revision = root_node.base_sha | ||
| end | ||
|
|
||
| ---@type ShowPreviewOpts | ||
| local opts = { | ||
| old_file_name = root_node.old_file_name, | ||
| new_file_name = root_node.file_name, | ||
| start_line = start_line, | ||
| end_line = end_line, | ||
| is_new_sha = is_new_sha, | ||
| revision = revision, | ||
| note_header = note_node.text, | ||
| comment_type = is_draft and "draft" or action, | ||
| note_lines = action ~= "reply" and common.get_note_lines(tree) or nil, | ||
| root_node_id = root_node.id, | ||
| note_node_id = note_node_id, | ||
| } | ||
| require("gitlab.actions.suggestions").show_preview(opts) | ||
| end |
There was a problem hiding this comment.
This is confusing to me. Does "apply" not actually apply the suggestion? All it's doing for me locally is opening the preview. I'd expect the behavior would be to apply the suggestion to the MR, as it does in Gitlab, and like close the issue as well. Is that not possible?
There was a problem hiding this comment.
Yeah, I understand it's not ideal. The suggestion is "applied" in the sense that it is shown in the local file and when closing the suggestion tab with "ZZ" the local file is not restored but keeps the suggested changes. The main purpose of the whole suggestions functionality in this PR is basically making it easier to see what the suggestion looks like in the actual code base (rather than in markdown) and possibly get LSP diagnostics on it, to run tests with the suggestion applied, etc. and then easily discard it if it's not OK or keep it if it is.
Since the "apply suggestion" is only performed on the local file if the file doesn't contain any other modifications, I guess it would be safe to also git add the file and possibly close the thread. But that would still not be the same as applying a suggestion online, where a new commit is created with the suggestion applied. If we wanted to do this locally, we'd have to control there are no other staged changes. But that should not block the "apply suggestion" functionality if the user just wanted to apply the suggestion locally and not create a commit.
Ideally, we'd want to allow users to create suggestion batches and create a single commit with all of them applied, but that would be even more involved.
I've been using this branch for some time already and it works quite reliably and is already useful, but I'll think about how to make the "apply suggestion" less confusing while not making the code too complicated.
| local get_default_suggestion = function(original_lines, opts) | ||
| local backticks = "```" | ||
| local selected_lines = { unpack(original_lines, opts.start_line, opts.end_line) } | ||
| for _, line in ipairs(selected_lines) do | ||
| local match = string.match(line, "^%s*(`+)%s*$") | ||
| if match and #match >= #backticks then | ||
| backticks = match .. "`" | ||
| end | ||
| end | ||
| local suggestion_lines = { backticks .. "suggestion:-" .. (opts.end_line - opts.start_line) .. "+0" } | ||
| vim.list_extend(suggestion_lines, selected_lines) | ||
| table.insert(suggestion_lines, backticks) | ||
| return suggestion_lines | ||
| end |
There was a problem hiding this comment.
This partially duplicates the build_suggestion function in the comment.lua module and it could certainly be refactored to serve the purpose in both cases, however, I didn't want to bloat this PR even more with changes in the comment.lua module. I'd prefer to refactor it later in a follow-up PR. It would perhaps also allow us to add a keybinding to the comment popup to paste the default suggestion like I do in suggestions.lua
| if is_new_sha then | ||
| revision = common.commented_line_has_changed(tree, root_node) and root_node.head_sha or "HEAD" | ||
| else | ||
| revision = root_node.base_sha | ||
| end |
There was a problem hiding this comment.
If the comment is on the "NEW SHA", we can use HEAD as the original text of the suggestion and show it on the local file. Othewise we use the base_sha (i.e. the version in the target branch) and show it in a temp file. This reduces the usefulness of the suggestion preview as it doesn't allow for using all of the LSP goodies etc.
For this reason, I'd like to reverse the logic of showing comments on unmodified lines in the old file and show them in the new file whenever possible. What do you think?
This prevents the folding in the suggestion buffer to get off when the suggestion doesn't actually modify the original and the user types some text outside of the tripple-quoted suggestion segment.
jakubbortlik
force-pushed
the
feat-add-suggestions-functionality
branch
from
a91be08 to
efdf5b7
Compare
Hi @harrisoncramer, in this rather large PR, I would like to introduce suggestions functionality in the discussion tree and in the reviewer:
The suggestion preview has some useful features:
I've tried to make it so that the suggestion is previewed on the local file in as many cases as possible so the user can make use of LSP, running tests with the suggestion applied, etc.
Documentation in
doc/gitlab.nvim.txtis largely missing yet (that's why the PR is in Draft mode), but the code is fairly well documented.I'd appreciate feedback, both on the code as well as on the functionality. I've been using this for some time and find it useful and worth the large amount of changes.
I believe some refactoring could be applied to the existing comment creation which would ultimately reduce the amount of code, like I've written a separate function for creating a suggestion, which is a little more universal than the existing build_suggestion.