fix: Comments on unchanged, expanded lines (#146)

This MR fixes an issue with refreshes of the diagnostics and signs when users leave comments on unchanged lines.

lua/gitlab/actions/discussions/annotations.lua

@@ -71,3 +71,19 @@
 ---@field resolved_discussions number
 ---@field resolvable_notes number
 ---@field resolved_notes number
+---
+---@class SignTable
+---@field name string
+---@field group string
+---@field priority number
+---@field id number
+---@field lnum number
+---@field buffer number?
+---
+---@class DiagnosticTable
+---@field message string
+---@field col number
+---@field severity number
+---@field user_data table
+---@field source string
+---@field code string?

lua/gitlab/actions/discussions/init.lua

@@ -336,17 +336,17 @@ end
 
 -- This function (settings.discussion_tree.jump_to_reviewer) will jump the cursor to the reviewer's location associated with the note. The implementation depends on the reviewer
 M.jump_to_reviewer = function(tree)
-  local file_name, new_line, old_line, error = M.get_note_location(tree)
+  local file_name, new_line, old_line, is_undefined_type, error = M.get_note_location(tree)
   if error ~= nil then
     u.notify(error, vim.log.levels.ERROR)
     return
   end
-  reviewer.jump(file_name, new_line, old_line)
+  reviewer.jump(file_name, new_line, old_line, { is_undefined_type = is_undefined_type })
 end
 
 -- This function (settings.discussion_tree.jump_to_file) will jump to the file changed in a new tab
 M.jump_to_file = function(tree)
-  local file_name, new_line, old_line, error = M.get_note_location(tree)
+  local file_name, new_line, old_line, _, error = M.get_note_location(tree)
   if error ~= nil then
     u.notify(error, vim.log.levels.ERROR)
     return
@@ -666,16 +666,21 @@ end
 
 ---Get note location
 ---@param tree NuiTree
+---@return string, string, string, boolean, string?
 M.get_note_location = function(tree)
   local node = tree:get_node()
   if node == nil then
-    return nil, nil, nil, "Could not get node"
+    return "", "", "", false, "Could not get node"
   end
   local discussion_node = M.get_root_node(tree, node)
   if discussion_node == nil then
-    return nil, nil, nil, "Could not get discussion node"
+    return "", "", "", false, "Could not get discussion node"
   end
-  return discussion_node.file_name, discussion_node.new_line, discussion_node.old_line
+  return discussion_node.file_name,
+    discussion_node.new_line,
+    discussion_node.old_line,
+    discussion_node.undefined_type or false,
+    nil
 end
 
 return M

lua/gitlab/actions/discussions/signs.lua

@@ -10,19 +10,64 @@ local diagnostics_namespace = vim.api.nvim_create_namespace(discussion_sign_name
 local M = {}
 M.diagnostics_namespace = diagnostics_namespace
 
----Parse line code and return old and new line numbers
+---Clear all signs and diagnostics
----@param line_code string gitlab line code -> 588440f66559714280628a4f9799f0c4eb880a4a_10_10
+M.clear_signs_and_discussions = function()
----@return number?
+  vim.fn.sign_unplace(discussion_sign_name)
----@return number?
+  vim.diagnostic.reset(diagnostics_namespace)
-local function _parse_line_code(line_code)
+end
-  local line_code_regex = "%w+_(%d+)_(%d+)"
+
-  local old_line, new_line = line_code:match(line_code_regex)
+---Refresh the discussion signs for currently loaded file in reviewer For convinience we use same
-  return tonumber(old_line), tonumber(new_line)
+---string for sign name and sign group ( currently there is only one sign needed)
+---@param discussions Discussion[]
+M.refresh_signs = function(discussions)
+  local diagnostics = M.filter_discussions_for_signs_and_diagnostics(discussions)
+  if diagnostics == nil then
+    vim.diagnostic.reset(diagnostics_namespace)
+    return
+  end
+
+  local new_signs, old_signs, error = M.parse_signs_from_discussions(discussions)
+  if error ~= nil then
+    vim.notify(error, vim.log.levels.ERROR)
+    return
+  end
+
+  vim.fn.sign_unplace(discussion_sign_name)
+  reviewer.place_sign(old_signs, "old")
+  reviewer.place_sign(new_signs, "new")
+end
+
+---Refresh the diagnostics for the currently reviewed file
+---@param discussions Discussion[]
+M.refresh_diagnostics = function(discussions)
+  -- Keep in mind that diagnostic line numbers use 0-based indexing while line numbers use
+  -- 1-based indexing
+  local diagnostics = M.filter_discussions_for_signs_and_diagnostics(discussions)
+  if diagnostics == nil then
+    vim.diagnostic.reset(diagnostics_namespace)
+    return
+  end
+
+  local new_diagnostics, old_diagnostics = M.parse_diagnostics_from_discussions(discussions)
+
+  vim.diagnostic.reset(diagnostics_namespace)
+  reviewer.set_diagnostics(
+    diagnostics_namespace,
+    new_diagnostics,
+    "new",
+    state.settings.discussion_diagnostic.display_opts
+  )
+  reviewer.set_diagnostics(
+    diagnostics_namespace,
+    old_diagnostics,
+    "old",
+    state.settings.discussion_diagnostic.display_opts
+  )
 end
 
 ---Filter all discussions which are relevant for currently visible signs and diagnostscs.
 ---@return Discussion[]?
-local filter_discussions_for_signs_and_diagnostics = function(all_discussions)
+M.filter_discussions_for_signs_and_diagnostics = function(all_discussions)
   if type(all_discussions) ~= "table" then
     return
   end
@@ -58,13 +103,6 @@ local filter_discussions_for_signs_and_diagnostics = function(all_discussions)
   return discussions
 end
 
----Build note header from note.
----@param note Note
----@return string
-local build_note_header = function(note)
-  return "@" .. note.author.username .. " " .. u.time_since(note.created_at)
-end
-
 ---Define signs for discussions if not already defined
 M.setup_signs = function()
   local discussion_sign = state.settings.discussion_sign
@@ -87,34 +125,126 @@ M.setup_signs = function()
   end
 end
 
----Refresh the discussion signs for currently loaded file in reviewer For convinience we use same
+---Iterates over each discussion and returns a list of tables with sign
----string for sign name and sign group ( currently there is only one sign needed)
+---data, for instance group, priority, line number etc.
-M.refresh_signs = function(discussions)
+---@param discussions Discussion[]
-  local diagnostics = filter_discussions_for_signs_and_diagnostics(discussions)
+---@return DiagnosticTable[], DiagnosticTable[], string?
-  if diagnostics == nil then
+M.parse_diagnostics_from_discussions = function(discussions)
-    vim.diagnostic.reset(diagnostics_namespace)
+  local new_diagnostics = {}
-    return
+  local old_diagnostics = {}
+  for _, discussion in ipairs(discussions) do
+    local first_note = discussion.notes[1]
+    local message = ""
+    for _, note in ipairs(discussion.notes) do
+      message = message .. M.build_note_header(note) .. "\n" .. note.body .. "\n"
+    end
+
+    local diagnostic = {
+      message = message,
+      col = 0,
+      severity = state.settings.discussion_diagnostic.severity,
+      user_data = { discussion_id = discussion.id, header = M.build_note_header(discussion.notes[1]) },
+      source = "gitlab",
+      code = state.settings.discussion_diagnostic.code,
+    }
+
+    -- Diagnostics for line range discussions are tricky - you need to set lnum to be the
+    -- line number equal to note.position.new_line or note.position.old_line because that is the
+    -- only line where you can trigger the diagnostic to show. This also needs to be in sync
+    -- with the sign placement.
+    local line_range = first_note.position.line_range
+    if line_range ~= nil then
+      local start_old_line, start_new_line = M.parse_line_code(line_range.start.line_code)
+      local end_old_line, end_new_line = M.parse_line_code(line_range["end"].line_code)
+
+      local start_type = line_range.start.type
+      if start_type == "new" then
+        local new_diagnostic
+        if first_note.position.new_line == start_new_line then
+          new_diagnostic = {
+            lnum = start_new_line - 1,
+            end_lnum = end_new_line - 1,
+          }
+        else
+          new_diagnostic = {
+            lnum = end_new_line - 1,
+            end_lnum = start_new_line - 1,
+          }
+        end
+        new_diagnostic = vim.tbl_deep_extend("force", new_diagnostic, diagnostic)
+        table.insert(new_diagnostics, new_diagnostic)
+      elseif start_type == "old" or start_type == "expanded" or start_type == "" then
+        local old_diagnostic
+        if first_note.position.old_line == start_old_line then
+          old_diagnostic = {
+            lnum = start_old_line - 1,
+            end_lnum = end_old_line - 1,
+          }
+        else
+          old_diagnostic = {
+            lnum = end_old_line - 1,
+            end_lnum = start_old_line - 1,
+          }
+        end
+        old_diagnostic = vim.tbl_deep_extend("force", old_diagnostic, diagnostic)
+        table.insert(old_diagnostics, old_diagnostic)
+      else -- Comments on expanded, non-changed lines
+        return {}, {}, string.format("Unsupported line range type found for discussion %s", discussion.id)
+      end
+    else -- Diagnostics for single line discussions.
+      if first_note.position.new_line ~= nil then
+        local new_diagnostic = {
+          lnum = first_note.position.new_line - 1,
+        }
+        new_diagnostic = vim.tbl_deep_extend("force", new_diagnostic, diagnostic)
+        table.insert(new_diagnostics, new_diagnostic)
+      end
+      if first_note.position.old_line ~= nil then
+        local old_diagnostic = {
+          lnum = first_note.position.old_line - 1,
+        }
+        old_diagnostic = vim.tbl_deep_extend("force", old_diagnostic, diagnostic)
+        table.insert(old_diagnostics, old_diagnostic)
+      end
+    end
   end
 
+  return new_diagnostics, old_diagnostics
+end
+
+local base_sign = {
+  name = discussion_sign_name,
+  group = discussion_sign_name,
+  priority = state.settings.discussion_sign.priority,
+  buffer = nil,
+}
+local base_helper_sign = {
+  name = discussion_sign_name,
+  group = discussion_sign_name,
+  priority = state.settings.discussion_sign.priority - 1,
+  buffer = nil,
+}
+
+---Iterates over each discussion and returns a list of tables with sign
+---data, for instance group, priority, line number etc.
+---@param discussions Discussion[]
+---@return SignTable[], SignTable[], string?
+M.parse_signs_from_discussions = function(discussions)
   local new_signs = {}
   local old_signs = {}
-  for _, discussion in ipairs(diagnostics) do
+  for _, discussion in ipairs(discussions) do
     local first_note = discussion.notes[1]
-    local base_sign = {
+    local line_range = first_note.position.line_range
-      name = discussion_sign_name,
+
-      group = discussion_sign_name,
+    -- We have a line range which means we either have a multi-line comment or a comment
-      priority = state.settings.discussion_sign.priority,
+    -- on a line in an "expanded" part of a file
-    }
+    if line_range ~= nil then
-    local base_helper_sign = {
+      local start_old_line, start_new_line = M.parse_line_code(line_range.start.line_code)
-      name = discussion_sign_name,
+      local end_old_line, end_new_line = M.parse_line_code(line_range["end"].line_code)
-      group = discussion_sign_name,
-      priority = state.settings.discussion_sign.priority - 1,
-    }
-    if first_note.position.line_range ~= nil then
-      local start_old_line, start_new_line = _parse_line_code(first_note.position.line_range.start.line_code)
-      local end_old_line, end_new_line = _parse_line_code(first_note.position.line_range["end"].line_code)
       local discussion_line, start_line, end_line
-      if first_note.position.line_range.start.type == "new" then
+
+      local start_type = line_range.start.type
+      if start_type == "new" then
         table.insert(
           new_signs,
           vim.tbl_deep_extend("force", {
@@ -125,7 +255,7 @@ M.refresh_signs = function(discussions)
         discussion_line = first_note.position.new_line
         start_line = start_new_line
         end_line = end_new_line
-      elseif first_note.position.line_range.start.type == "old" then
+      elseif start_type == "old" or start_type == "expanded" or start_type == "" then
         table.insert(
           old_signs,
           vim.tbl_deep_extend("force", {
@@ -136,7 +266,11 @@ M.refresh_signs = function(discussions)
         discussion_line = first_note.position.old_line
         start_line = start_old_line
         end_line = end_old_line
+      else
+        vim.print(start_type == "")
+        return {}, {}, string.format("Unsupported line range type found for discussion %s", discussion.id)
       end
+
       -- Helper signs does not have specific ids currently.
       if state.settings.discussion_sign.helper_signs.enabled then
         local helper_signs = {}
@@ -162,13 +296,13 @@ M.refresh_signs = function(discussions)
             )
           end
         end
-        if first_note.position.line_range.start.type == "new" then
+        if start_type == "new" then
           vim.list_extend(new_signs, helper_signs)
-        elseif first_note.position.line_range.start.type == "old" then
+        elseif start_type == "old" or start_type == "expanded" or start_type == "" then
           vim.list_extend(old_signs, helper_signs)
         end
       end
-    else
+    else -- The note is a normal comment, not a range comment
       local sign = vim.tbl_deep_extend("force", {
         id = first_note.id,
       }, base_sign)
@@ -180,114 +314,24 @@ M.refresh_signs = function(discussions)
       end
     end
   end
-  vim.fn.sign_unplace(discussion_sign_name)
+
-  reviewer.place_sign(old_signs, "old")
+  return new_signs, old_signs, nil
-  reviewer.place_sign(new_signs, "new")
 end
 
----Refresh the diagnostics for the currently reviewed file
+---Parse line code and return old and new line numbers
-M.refresh_diagnostics = function(discussions)
+---@param line_code string gitlab line code -> 588440f66559714280628a4f9799f0c4eb880a4a_10_10
-  -- Keep in mind that diagnostic line numbers use 0-based indexing while line numbers use
+---@return number?
-  -- 1-based indexing
+M.parse_line_code = function(line_code)
-  local diagnostics = filter_discussions_for_signs_and_diagnostics(discussions)
+  local line_code_regex = "%w+_(%d+)_(%d+)"
-  if diagnostics == nil then
+  local old_line, new_line = line_code:match(line_code_regex)
-    vim.diagnostic.reset(diagnostics_namespace)
+  return tonumber(old_line), tonumber(new_line)
-    return
-  end
-
-  local new_diagnostics = {}
-  local old_diagnostics = {}
-  for _, discussion in ipairs(diagnostics) do
-    local first_note = discussion.notes[1]
-    local message = ""
-    for _, note in ipairs(discussion.notes) do
-      message = message .. build_note_header(note) .. "\n" .. note.body .. "\n"
-    end
-
-    local diagnostic = {
-      message = message,
-      col = 0,
-      severity = state.settings.discussion_diagnostic.severity,
-      user_data = { discussion_id = discussion.id, header = build_note_header(discussion.notes[1]) },
-      source = "gitlab",
-      code = state.settings.discussion_diagnostic.code,
-    }
-    if first_note.position.line_range ~= nil then
-      -- Diagnostics for line range discussions are tricky - you need to set lnum to
-      -- line number equal to note.position.new_line or note.position.old_line because that is
-      -- only line where you can trigger the diagnostic show. This also need to be in sinc
-      -- with the sign placement.
-      local start_old_line, start_new_line = _parse_line_code(first_note.position.line_range.start.line_code)
-      local end_old_line, end_new_line = _parse_line_code(first_note.position.line_range["end"].line_code)
-      if first_note.position.line_range.start.type == "new" then
-        local new_diagnostic
-        if first_note.position.new_line == start_new_line then
-          new_diagnostic = {
-            lnum = start_new_line - 1,
-            end_lnum = end_new_line - 1,
-          }
-        else
-          new_diagnostic = {
-            lnum = end_new_line - 1,
-            end_lnum = start_new_line - 1,
-          }
-        end
-        new_diagnostic = vim.tbl_deep_extend("force", new_diagnostic, diagnostic)
-        table.insert(new_diagnostics, new_diagnostic)
-      elseif first_note.position.line_range.start.type == "old" then
-        local old_diagnostic
-        if first_note.position.old_line == start_old_line then
-          old_diagnostic = {
-            lnum = start_old_line - 1,
-            end_lnum = end_old_line - 1,
-          }
-        else
-          old_diagnostic = {
-            lnum = end_old_line - 1,
-            end_lnum = start_old_line - 1,
-          }
-        end
-        old_diagnostic = vim.tbl_deep_extend("force", old_diagnostic, diagnostic)
-        table.insert(old_diagnostics, old_diagnostic)
-      end
-    else
-      -- Diagnostics for single line discussions.
-      if first_note.position.new_line ~= nil then
-        local new_diagnostic = {
-          lnum = first_note.position.new_line - 1,
-        }
-        new_diagnostic = vim.tbl_deep_extend("force", new_diagnostic, diagnostic)
-        table.insert(new_diagnostics, new_diagnostic)
-      end
-      if first_note.position.old_line ~= nil then
-        local old_diagnostic = {
-          lnum = first_note.position.old_line - 1,
-        }
-        old_diagnostic = vim.tbl_deep_extend("force", old_diagnostic, diagnostic)
-        table.insert(old_diagnostics, old_diagnostic)
-      end
-    end
-  end
-
-  vim.diagnostic.reset(diagnostics_namespace)
-  reviewer.set_diagnostics(
-    diagnostics_namespace,
-    new_diagnostics,
-    "new",
-    state.settings.discussion_diagnostic.display_opts
-  )
-  reviewer.set_diagnostics(
-    diagnostics_namespace,
-    old_diagnostics,
-    "old",
-    state.settings.discussion_diagnostic.display_opts
-  )
 end
 
----Clear all signs and diagnostics
+---Build note header from note.
-M.clear_signs_and_discussions = function()
+---@param note Note
-  vim.fn.sign_unplace(discussion_sign_name)
+---@return string
-  vim.diagnostic.reset(diagnostics_namespace)
+M.build_note_header = function(note)
+  return "@" .. note.author.username .. " " .. u.time_since(note.created_at)
 end
 
 return M

lua/gitlab/actions/discussions/tree.lua

@@ -158,6 +158,7 @@ M.add_discussions_to_table = function(items, unlinked)
     local root_text_nodes = {}
     local resolvable = false
     local resolved = false
+    local undefined_type = false
     local root_new_line = nil
     local root_old_line = nil
 
@@ -172,6 +173,16 @@ M.add_discussions_to_table = function(items, unlinked)
         root_note_id = tostring(note.id)
         resolvable = note.resolvable
         resolved = note.resolved
+
+        -- This appears to be a Gitlab 🐛 where the "type" is returned as an empty string in some cases
+        -- We link these comments to the old file by default
+        if
+          type(note.position) == "table"
+          and note.position.line_range ~= nil
+          and note.position.line_range.start.type == ""
+        then
+          undefined_type = true
+        end
       else -- Otherwise insert it as a child node...
         local note_node = M.build_note(note)
         table.insert(discussion_children, note_node)
@@ -191,6 +202,7 @@ M.add_discussions_to_table = function(items, unlinked)
       old_line = root_old_line,
       resolvable = resolvable,
       resolved = resolved,
+      undefined_type = undefined_type,
     }, body)
 
     table.insert(t, root_node)

lua/gitlab/reviewer/diffview.lua

@@ -53,7 +53,7 @@ M.close = function()
   discussions.close()
 end
 
-M.jump = function(file_name, new_line, old_line)
+M.jump = function(file_name, new_line, old_line, opts)
   if M.tabnr == nil then
     u.notify("Can't jump to Diffvew. Is it open?", vim.log.levels.ERROR)
     return
@@ -74,7 +74,12 @@ M.jump = function(file_name, new_line, old_line)
         return
       end
       async.await(view:set_file(file))
-      if new_line ~= nil then
+      -- TODO: Ranged comments on unchanged lines will have both a
+      -- new line and a old line. We need to distinguish them somehow from
+      -- range comments (which also have this) so that we can know
+      -- which buffer to jump to. Right now, we jump to the wrong
+      -- buffer for ranged comments on unchanged lines.
+      if new_line ~= nil and not opts.is_undefined_type then
         layout.b:focus()
         vim.api.nvim_win_set_cursor(0, { tonumber(new_line), 0 })
       elseif old_line ~= nil then
@@ -200,7 +205,7 @@ end
 
 ---Place a sign in currently reviewed file. Use new line for identifing lines after changes, old
 ---line for identifing lines before changes and both if line was not changed.
----@param signs table table of signs. See :h sign_placelist
+---@param signs SignTable[] table of signs. See :h sign_placelist
 ---@param type string "new" if diagnostic should be in file after changes else "old"
 M.place_sign = function(signs, type)
   local view = diffview_lib.get_current_view()

lua/gitlab/utils/init.lua

@@ -234,6 +234,13 @@ M.map = function(tbl, f)
   return t
 end
 
+M.reduce = function(tbl, agg, f)
+  for _, v in pairs(tbl) do
+    agg = f(agg, v)
+  end
+  return agg
+end
+
 M.notify = function(msg, lvl)
   vim.notify("gitlab.nvim: " .. msg, lvl)
 end