Fix Multi Line Issues (Large Refactor) (#197)

Fix: Multi-line discussions. The calculation of a range for a multiline comment has been consolidated and moved into the location.lua file. This does not attempt to fix diagnostics.
Refactor: It refactors the discussions code to split hunk parsing and management into a separate module
Fix: Don't allow comments on modified buffers #194 by preventing comments on the reviewer when using --imply-local and when the working tree is dirty entirely.
Refactor: It introduces a new List class for data aggregation, filtering, etc.
Fix: It removes redundant API calls and refreshes from the discussion pane

Author: harrisoncramer

.github/CONTRIBUTING.md

@@ -49,6 +49,6 @@ $ luacheck --globals vim busted --no-max-line-length -- .
 
 Please provide a description of the feature, and links to any relevant issues. 
 
-That's it! I'll try to respond to any incoming merge request in a few days. Once we've reviewed it, it will be merged into the develop branc, it will be merged into the develop branch. 
+That's it! I'll try to respond to any incoming merge request in a few days. Once we've reviewed it, it will be merged into the develop branch. 
 
 After some time, if the develop branch is found to be stable, that branch will be merged into `main` and released. When merged into `main` the pipeline will detect whether we're merging in a patch, minor, or major change, and create a new tag (e.g. 1.0.12) and release.

.github/workflows/go.yaml

@@ -3,6 +3,7 @@ on:
   pull_request:
     branches:
       - main
+      - develop
 jobs:
   go_lint:
     name: Lint Go 💅 

.github/workflows/lua.yaml

@@ -3,6 +3,7 @@ on:
   pull_request:
     branches:
       - main
+      - develop
 jobs:
   lua_lint:
     name: Lint Lua 💅

lua/gitlab/actions/assignees_and_reviewers.lua

@@ -2,6 +2,7 @@
 -- and assignees in Gitlab, those who must review an MR.
 local u = require("gitlab.utils")
 local job = require("gitlab.job")
+local List = require("gitlab.utils.list")
 local state = require("gitlab.state")
 local M = {}
 
@@ -67,13 +68,11 @@ end
 
 M.filter_eligible = function(current, to_remove)
   local ids = u.extract(to_remove, "id")
-  local res = {}
-  for _, member in ipairs(current) do
+  return List.new(current):filter(function(member)
     if not u.contains(ids, member.id) then
-      table.insert(res, member)
+      return true
     end
-  end
-  return res
+  end)
 end
 
 return M

lua/gitlab/actions/comment.lua

@@ -5,9 +5,11 @@ local Popup = require("nui.popup")
 local state = require("gitlab.state")
 local job = require("gitlab.job")
 local u = require("gitlab.utils")
+local git = require("gitlab.git")
 local discussions = require("gitlab.actions.discussions")
 local miscellaneous = require("gitlab.actions.miscellaneous")
 local reviewer = require("gitlab.reviewer")
+local Location = require("gitlab.reviewer.location")
 local M = {}
 
 -- Popup creation is wrapped in a function so that it is performed *after* user
@@ -20,6 +22,14 @@ end
 -- This function will open a comment popup in order to create a comment on the changed/updated
 -- line in the current MR
 M.create_comment = function()
+  local has_clean_tree = git.has_clean_tree()
+  if state.settings.reviewer_settings.diffview.imply_local and not has_clean_tree then
+    u.notify(
+      "Cannot leave comments on a dirty tree. \n Please stash all local changes or push them up.",
+      vim.log.levels.WARN
+    )
+    return
+  end
   local comment_popup = create_comment_popup()
   comment_popup:mount()
   state.set_popup_keymaps(comment_popup, function(text)
@@ -95,30 +105,11 @@ M.create_note = function()
   end, miscellaneous.attach_file)
 end
 
----@class LineRange
----@field start_line integer
----@field end_line integer
-
----@class ReviewerLineInfo
----@field old_line integer
----@field new_line integer
----@field type string either "new" or "old"
-
----@class ReviewerRangeInfo
----@field start ReviewerLineInfo
----@field end ReviewerLineInfo
-
----@class ReviewerInfo
----@field file_name string
----@field old_line integer | nil
----@field new_line integer | nil
----@field range_info ReviewerRangeInfo|nil
-
 ---This function (settings.popup.perform_action) will send the comment to the Go server
 ---@param text string comment text
----@param range LineRange | nil range of visuel selection or nil
+---@param visual_range LineRange | nil range of visual selection or nil
 ---@param unlinked boolean | nil if true, the comment is not linked to a line
-M.confirm_create_comment = function(text, range, unlinked)
+M.confirm_create_comment = function(text, visual_range, unlinked)
   if text == nil then
     u.notify("Reviewer did not provide text of change", vim.log.levels.ERROR)
     return
@@ -129,33 +120,42 @@ M.confirm_create_comment = function(text, range, unlinked)
     job.run_job("/mr/comment", "POST", body, function(data)
       u.notify("Note created!", vim.log.levels.INFO)
       discussions.add_discussion({ data = data, unlinked = true })
-      discussions.refresh_discussion_data()
+      discussions.refresh()
     end)
     return
   end
 
-  local reviewer_info = reviewer.get_location(range)
-  if not reviewer_info then
+  local reviewer_data = reviewer.get_reviewer_data()
+  if reviewer_data == nil then
+    u.notify("Error getting reviewer data", vim.log.levels.ERROR)
+    return
+  end
+
+  local location = Location:new(reviewer_data, visual_range)
+  location:build_location_data()
+  local location_data = location.location_data
+  if location_data == nil then
+    u.notify("Error getting location information", vim.log.levels.ERROR)
     return
   end
 
   local revision = state.MR_REVISIONS[1]
   local body = {
+    type = "text",
     comment = text,
-    file_name = reviewer_info.file_name,
-    old_line = reviewer_info.old_line,
-    new_line = reviewer_info.new_line,
+    file_name = reviewer_data.file_name,
     base_commit_sha = revision.base_commit_sha,
     start_commit_sha = revision.start_commit_sha,
     head_commit_sha = revision.head_commit_sha,
-    type = "text",
-    line_range = reviewer_info.range_info,
+    old_line = location_data.old_line,
+    new_line = location_data.new_line,
+    line_range = location_data.line_range,
   }
 
   job.run_job("/mr/comment", "POST", body, function(data)
     u.notify("Comment created!", vim.log.levels.INFO)
     discussions.add_discussion({ data = data, unlinked = false })
-    discussions.refresh_discussion_data()
+    discussions.refresh()
   end)
 end
 

lua/gitlab/actions/create_mr.lua

@@ -5,6 +5,7 @@ local Input = require("nui.input")
 local Popup = require("nui.popup")
 local job = require("gitlab.job")
 local u = require("gitlab.utils")
+local git = require("gitlab.git")
 local state = require("gitlab.state")
 local miscellaneous = require("gitlab.actions.miscellaneous")
 
@@ -124,7 +125,7 @@ M.pick_target = function(args)
 end
 
 local function make_template_path(t)
-  local base_dir = vim.fn.trim(vim.fn.system({ "git", "rev-parse", "--show-toplevel" }))
+  local base_dir = git.base_dir()
   return base_dir
     .. state.settings.file_separator
     .. ".gitlab"

lua/gitlab/actions/discussions/annotations.lua

@@ -101,3 +101,22 @@
 ---@field user_data table
 ---@field source string
 ---@field code string?
+
+---@class LineRange
+---@field start_line integer
+---@field end_line integer
+
+---@class DiffviewInfo
+---@field modification_type string
+---@field file_name string
+---@field current_bufnr integer
+---@field new_sha_win_id integer
+---@field old_sha_win_id integer
+---@field opposite_bufnr integer
+---@field new_line_from_buf integer
+---@field old_line_from_buf integer
+
+---@class LocationData
+---@field old_line integer | nil
+---@field new_line integer | nil
+---@field line_range ReviewerRangeInfo|nil

lua/gitlab/actions/discussions/init.lua

@@ -9,8 +9,10 @@ local job = require("gitlab.job")
 local u = require("gitlab.utils")
 local state = require("gitlab.state")
 local reviewer = require("gitlab.reviewer")
+local List = require("gitlab.utils.list")
 local miscellaneous = require("gitlab.actions.miscellaneous")
 local discussions_tree = require("gitlab.actions.discussions.tree")
+local diffview_lib = require("diffview.lib")
 local signs = require("gitlab.actions.discussions.signs")
 local winbar = require("gitlab.actions.discussions.winbar")
 local help = require("gitlab.actions.help")
@@ -53,28 +55,55 @@ end
 ---Initialize everything for discussions like setup of signs, callbacks for reviewer, etc.
 M.initialize_discussions = function()
   signs.setup_signs()
-  -- Setup callback to refresh discussion data, discussion signs and diagnostics whenever the reviewed file changes.
-  reviewer.set_callback_for_file_changed(M.refresh_discussion_data)
-  -- Setup callback to clear signs and diagnostics whenever reviewer is left.
-  reviewer.set_callback_for_reviewer_leave(signs.clear_signs_and_diagnostics)
+  reviewer.set_callback_for_file_changed(function()
+    M.refresh_view()
+    M.modifiable(false)
+  end)
+  reviewer.set_callback_for_reviewer_enter(function()
+    M.modifiable(false)
+  end)
+  reviewer.set_callback_for_reviewer_leave(function()
+    signs.clear_signs_and_diagnostics()
+    M.modifiable(true)
+  end)
+end
+
+--- Ensures that the both buffers in the reviewer are/not modifiable. Relevant if the user is using
+--- the --imply-local setting
+M.modifiable = function(bool)
+  local view = diffview_lib.get_current_view()
+  local a = view.cur_layout.a.file.bufnr
+  local b = view.cur_layout.b.file.bufnr
+  if vim.api.nvim_buf_is_loaded(a) then
+    vim.api.nvim_buf_set_option(a, "modifiable", bool)
+  end
+  if vim.api.nvim_buf_is_loaded(b) then
+    vim.api.nvim_buf_set_option(b, "modifiable", bool)
+  end
 end
 
 ---Refresh discussion data, signs, diagnostics, and winbar with new data from API
-M.refresh_discussion_data = function()
+--- and rebuild the entire view
+M.refresh = function()
   M.load_discussions(function()
-    if state.settings.discussion_sign.enabled then
-      signs.refresh_signs(M.discussions)
-    end
-    if state.settings.discussion_diagnostic.enabled then
-      signs.refresh_diagnostics(M.discussions)
-    end
-    if M.split_visible then
-      local linked_is_focused = M.linked_bufnr == M.focused_bufnr
-      winbar.update_winbar(M.discussions, M.unlinked_discussions, linked_is_focused and "Discussions" or "Notes")
-    end
+    M.refresh_view()
   end)
 end
 
+--- Take existing data and refresh the diagnostics, the winbar, and the signs
+M.refresh_view = function()
+  if state.settings.discussion_sign.enabled then
+    signs.refresh_signs(M.discussions)
+  end
+  if state.settings.discussion_diagnostic.enabled then
+    signs.refresh_diagnostics(M.discussions)
+  end
+  if M.split_visible then
+    local linked_is_focused = M.linked_bufnr == M.focused_bufnr
+    winbar.update_winbar(M.discussions, M.unlinked_discussions, linked_is_focused and "Discussions" or "Notes")
+  end
+end
+
 ---Toggle Discussions tree type between "simple" and "by_file_name"
 ---@param unlinked boolean True if selected view type is Notes (unlinked discussions)
 M.toggle_tree_type = function(unlinked)
@@ -148,6 +177,7 @@ M.toggle = function(callback)
   end)
 end
 
+-- Change between views in the discussion panel, either notes or discussions
 local switch_view_type = function()
   local change_to_unlinked = M.linked_bufnr == M.focused_bufnr
   local new_bufnr = change_to_unlinked and M.unlinked_bufnr or M.linked_bufnr
@@ -254,36 +284,23 @@ end
 
 -- This function will actually send the deletion to Gitlab
 -- when you make a selection, and re-render the tree
-M.send_deletion = function(tree, unlinked)
+M.send_deletion = function(tree)
   local current_node = tree:get_node()
 
   local note_node = M.get_note_node(tree, current_node)
   local root_node = M.get_root_node(tree, current_node)
   local note_id = note_node.is_root and root_node.root_note_id or note_node.id
-
   local body = { discussion_id = root_node.id, note_id = tonumber(note_id) }
-
   job.run_job("/mr/comment", "DELETE", body, function(data)
     u.notify(data.message, vim.log.levels.INFO)
-    if not note_node.is_root then
-      tree:remove_node("-" .. note_id) -- Note is not a discussion root, safe to remove
-      tree:render()
+    if note_node.is_root then
+      -- Replace root node w/ current node's contents...
+      tree:remove_node("-" .. root_node.id)
     else
-      if unlinked then
-        M.unlinked_discussions = u.remove_first_value(M.unlinked_discussions)
-        M.rebuild_unlinked_discussion_tree()
-      else
-        M.discussions = u.remove_first_value(M.discussions)
-        M.rebuild_discussion_tree()
-      end
-      M.add_empty_titles({
-        { M.linked_bufnr, M.discussions, "No Discussions for this MR" },
-        { M.unlinked_bufnr, M.unlinked_discussions, "No Notes (Unlinked Discussions) for this MR" },
-      })
-      M.switch_can_edit_bufs(false)
+      tree:remove_node("-" .. note_id)
     end
-
-    M.refresh_discussion_data()
+    tree:render()
+    M.refresh()
   end)
 end
 
@@ -293,18 +310,22 @@ M.edit_comment = function(tree, unlinked)
   local current_node = tree:get_node()
   local note_node = M.get_note_node(tree, current_node)
   local root_node = M.get_root_node(tree, current_node)
+  if note_node == nil or root_node == nil then
+    u.notify("Could not get root or note node", vim.log.levels.ERROR)
+    return
+  end
 
   edit_popup:mount()
 
-  local lines = {} -- Gather all lines from immediate children that aren't note nodes
-  local children_ids = note_node:get_child_ids()
-  for _, child_id in ipairs(children_ids) do
+  -- Gather all lines from immediate children that aren't note nodes
+  local lines = List.new(note_node:get_child_ids()):reduce(function(agg, child_id)
     local child_node = tree:get_node(child_id)
     if not child_node:has_children() then
       local line = tree:get_node(child_id).text
-      table.insert(lines, line)
+      table.insert(agg, line)
     end
-  end
+    return agg
+  end, {})
 
   local currentBuffer = vim.api.nvim_get_current_buf()
   vim.api.nvim_buf_set_lines(currentBuffer, 0, -1, false, lines)
@@ -362,7 +383,7 @@ M.toggle_discussion_resolved = function(tree)
   job.run_job("/mr/discussions/resolve", "PUT", body, function(data)
     u.notify(data.message, vim.log.levels.INFO)
     M.redraw_resolved_status(tree, note, not note.resolved)
-    M.refresh_discussion_data()
+    M.refresh()
   end)
 end
 
@@ -373,7 +394,17 @@ M.jump_to_reviewer = function(tree)
     u.notify(error, vim.log.levels.ERROR)
     return
   end
-  reviewer.jump(file_name, new_line, old_line, { is_undefined_type = is_undefined_type })
+
+  local new_line_int = tonumber(new_line)
+  local old_line_int = tonumber(old_line)
+
+  if new_line_int == nil and old_line_int == nil then
+    u.notify("Could not get new or old line", vim.log.levels.ERROR)
+    return
+  end
+
+  reviewer.jump(file_name, new_line_int, old_line_int, { is_undefined_type = is_undefined_type })
+  M.refresh_view()
 end
 
 -- This function (settings.discussion_tree.jump_to_file) will jump to the file changed in a new tab