fix: handle empty line codes in ranged discussions

lua/gitlab/actions/common.lua

@@ -248,24 +248,42 @@ M.get_line_number = function(id)
   return ((is_new_sha and first_note.position.new_line or first_note.position.old_line) or 1), is_new_sha
 end
 
+---Return the number of lines between the start and end of a range on one side of the diff.
+---Line codes are preferred because they hold both line numbers, whereas the range's
+---`old_line`/`new_line` are 0 on the side a line doesn't exist on.
+---@param start_from_code? integer The start line parsed from the line code
+---@param end_from_code? integer The end line parsed from the line code
+---@param start_line? integer The start line from the range
+---@param end_line? integer The end line from the range
+---@return integer
+local function get_range_length(start_from_code, end_from_code, start_line, end_line)
+  if start_from_code and end_from_code then
+    return end_from_code - start_from_code
+  end
+  if (start_line or 0) > 0 and (end_line or 0) > 0 then
+    return end_line - start_line
+  end
+  return 0
+end
+
 ---Return start and end line numbers for the note range, and whether comment is on "NEW SHA".
----The range is calculated from the line codes but the position itself is based on
----either the `new_line` or `old_line`.
+---The range is calculated from the line codes, or from the range's line numbers if GitLab
+---returns empty line codes, but the position itself is based on either the `new_line` or `old_line`.
 ---@param old_line? integer The line number in the OLD version
 ---@param new_line? integer The line number in the NEW version
----@param start_line_code string The line code for the start of the range
----@param end_line_code string The line code for the end of the range
+---@param line_range LineRange The range of the note
 ---@return integer start_line
 ---@return integer end_line
 ---@return boolean is_new_sha True if line range refers to NEW SHA
-M.get_line_numbers_for_range = function(old_line, new_line, start_line_code, end_line_code)
-  local old_start_line, new_start_line = indicators_common.parse_line_code(start_line_code)
-  local old_end_line, new_end_line = indicators_common.parse_line_code(end_line_code)
+M.get_line_numbers_for_range = function(old_line, new_line, line_range)
+  local start, end_ = line_range.start, line_range["end"]
+  local old_start_line, new_start_line = indicators_common.parse_line_code(start.line_code)
+  local old_end_line, new_end_line = indicators_common.parse_line_code(end_.line_code)
   if old_line ~= nil and old_start_line ~= 0 then
-    local range = old_end_line - old_start_line
+    local range = get_range_length(old_start_line, old_end_line, start.old_line, end_.old_line)
     return (old_line - range), old_line, false
   elseif new_line ~= nil then
-    local range = new_end_line - new_start_line
+    local range = get_range_length(new_start_line, new_end_line, start.new_line, end_.new_line)
     -- Force start_line to be greater than 0
     -- TODO: use `math.max(new_line - range, 1)` instead
     local start_line = (new_line - range > 0) and (new_line - range) or 1
@@ -282,12 +300,8 @@ end
 ---@return boolean is_new_sha True if line number refers to NEW SHA
 M.get_line_number_from_node = function(root_node)
   if root_node.range then
-    local line_number, _, is_new_sha = M.get_line_numbers_for_range(
-      root_node.old_line,
-      root_node.new_line,
-      root_node.range.start.line_code,
-      root_node.range["end"].line_code
-    )
+    local line_number, _, is_new_sha =
+      M.get_line_numbers_for_range(root_node.old_line, root_node.new_line, root_node.range)
     return line_number, is_new_sha
   else
     return M.get_line_number(root_node.id)

lua/gitlab/indicators/common.lua

@@ -53,15 +53,14 @@ M.filter_placeable_discussions = function()
 end
 
 ---Parse old and new line from a line code like "3f454a98e586d1aa0d322e19afd5e67e08f2d3c8_10_44".
----@param line_code string A SHA hash of the file name and line numbers before and after change
----@return integer The line number before the change
----@return integer The line number after the change
+---GitLab can return an empty line code for a ranged discussion.
+---@param line_code? string A SHA hash of the file name and line numbers before and after change
+---@return integer? The line number before the change
+---@return integer? The line number after the change
 M.parse_line_code = function(line_code)
   local line_code_regex = "%w+_(%d+)_(%d+)"
-  local old_line, new_line = line_code:match(line_code_regex)
-  old_line = tonumber(old_line) --[[@as integer]]
-  new_line = tonumber(new_line) --[[@as integer]]
-  return old_line, new_line
+  local old_line, new_line = (line_code or ""):match(line_code_regex)
+  return tonumber(old_line), tonumber(new_line)
 end
 
 ---Return true if discussion/draft belongs to the old file, otherwise false.

lua/gitlab/indicators/diagnostics.lua

@@ -76,12 +76,8 @@ local create_multiline_diagnostic = function(d_or_n)
     error("Parsing multi-line comment but note does not contain line range")
   end
 
-  local start_line, end_line, _ = actions_common.get_line_numbers_for_range(
-    first_note.position.old_line,
-    first_note.position.new_line,
-    line_range.start.line_code,
-    line_range["end"].line_code
-  )
+  local start_line, end_line, _ =
+    actions_common.get_line_numbers_for_range(first_note.position.old_line, first_note.position.new_line, line_range)
 
   return create_diagnostic({
     lnum = start_line - 1,

tests/spec/actions_common_spec.lua

@@ -0,0 +1,83 @@
+local actions_common = require("gitlab.actions.common")
+
+local sha = "3f454a98e586d1aa0d322e19afd5e67e08f2d3c8"
+
+local function line_code(old_line, new_line)
+  return sha .. "_" .. old_line .. "_" .. new_line
+end
+
+local function position(type, old_line, new_line, code)
+  return { type = type, old_line = old_line, new_line = new_line, line_code = code or "" }
+end
+
+describe("gitlab/actions/common.lua", function()
+  describe("get_line_numbers_for_range", function()
+    describe("with line codes", function()
+      it("Computes a range on the new SHA", function()
+        local range = {
+          start = position("new", 0, 40, line_code(1, 40)),
+          ["end"] = position("new", 0, 44, line_code(1, 44)),
+        }
+        assert.are.same({ 40, 44, true }, { actions_common.get_line_numbers_for_range(nil, 44, range) })
+      end)
+
+      it("Computes a range on the old SHA", function()
+        local range = {
+          start = position("old", 10, 0, line_code(10, 20)),
+          ["end"] = position("old", 12, 0, line_code(12, 20)),
+        }
+        assert.are.same({ 10, 12, false }, { actions_common.get_line_numbers_for_range(12, nil, range) })
+      end)
+
+      it("Computes a range spanning deleted and added lines", function()
+        local range = {
+          start = position("old", 5, 0, line_code(5, 38)),
+          ["end"] = position("new", 0, 44, line_code(7, 44)),
+        }
+        assert.are.same({ 38, 44, true }, { actions_common.get_line_numbers_for_range(nil, 44, range) })
+      end)
+    end)
+
+    describe("without line codes", function()
+      it("Computes a range on the new SHA from the range's new lines", function()
+        local range = {
+          start = position("new", 0, 24),
+          ["end"] = position("new", 0, 26),
+        }
+        assert.are.same({ 24, 26, true }, { actions_common.get_line_numbers_for_range(nil, 26, range) })
+      end)
+
+      it("Computes a range on the old SHA from the range's old lines", function()
+        local range = {
+          start = position("old", 10, 0),
+          ["end"] = position("old", 12, 0),
+        }
+        assert.are.same({ 10, 12, false }, { actions_common.get_line_numbers_for_range(12, nil, range) })
+      end)
+
+      it("Computes a range from the range's lines when only one line code is empty", function()
+        local range = {
+          start = position("new", 0, 24, line_code(1, 24)),
+          ["end"] = position("new", 0, 26),
+        }
+        assert.are.same({ 24, 26, true }, { actions_common.get_line_numbers_for_range(nil, 26, range) })
+      end)
+
+      it("Falls back to a single line for a range spanning deleted and added lines", function()
+        local range = {
+          start = position("old", 5, 0),
+          ["end"] = position("new", 0, 44),
+        }
+        assert.are.same({ 44, 44, true }, { actions_common.get_line_numbers_for_range(nil, 44, range) })
+      end)
+
+      it("Falls back to a single line when the range's lines are missing", function()
+        local range = {
+          start = position("old", 5, nil),
+          ["end"] = position("new", nil, 44),
+        }
+        assert.are.same({ 44, 44, true }, { actions_common.get_line_numbers_for_range(nil, 44, range) })
+      end)
+    end)
+  end)
+end)

tests/spec/indicators_common_spec.lua

@@ -0,0 +1,19 @@
+local indicators_common = require("gitlab.indicators.common")
+
+local sha = "3f454a98e586d1aa0d322e19afd5e67e08f2d3c8"
+
+describe("gitlab/indicators/common.lua", function()
+  describe("parse_line_code", function()
+    it("Parses both line numbers", function()
+      assert.are.same({ 10, 44 }, { indicators_common.parse_line_code(sha .. "_10_44") })
+    end)
+
+    it("Returns nil for an empty line code", function()
+      assert.are.same({ nil, nil }, { indicators_common.parse_line_code("") })
+    end)
+
+    it("Returns nil for a missing line code", function()
+      assert.are.same({ nil, nil }, { indicators_common.parse_line_code(nil) })
+    end)
+  end)
+end)