fix: get correct location data for comments efficiently

README.md

@@ -47,6 +47,7 @@ vim.pack.add({
   "https://github.com/nvim-tree/nvim-web-devicons", -- Recommended but not required. Icons in discussion tree.
   "https://github.com/harrisoncramer/gitlab.nvim",
 })
+require("diffview").setup({rename_threshold = 30}) -- Recommended to match the rename threshold of Gitlab
 ---@type GitlabSettings
 local opts = {} -- Your configuration
 require("gitlab").setup(opts)
@@ -60,7 +61,10 @@ With [folke/lazy.nvim](https://github.com/folke/lazy.nvim):
   -- branch = "main", -- Uncomment to use a stable version. The default, possibly unstable, but more actively maintained branch is `develop`.
   dependencies = {
     "MunifTanjim/nui.nvim",
-    "dlyongemallo/diffview-plus.nvim", -- Maintained fork of "sindrets/diffview.nvim".
+    {
+      "dlyongemallo/diffview-plus.nvim", -- Maintained fork of "sindrets/diffview.nvim".
+      opts = { rename_threshold = 30 }, -- Recommended to match the rename threshold of Gitlab
+    },
     "stevearc/dressing.nvim", -- Recommended but not required. Better UI for pickers.
     "nvim-tree/nvim-web-devicons", -- Recommended but not required. Icons in discussion tree.
   },
@@ -77,7 +81,12 @@ And with <a href="https://github.com/lewis6991/pckr.nvim">pckr.nvim</a>:
   -- branch = "main", -- Uncomment to use a stable version. The default, possibly unstable, but more actively maintained branch is `develop`.
   requires = {
     "MunifTanjim/nui.nvim",
-    "dlyongemallo/diffview-plus.nvim", -- Maintained fork of "sindrets/diffview.nvim".
+    {
+      "dlyongemallo/diffview-plus.nvim", -- Maintained fork of "sindrets/diffview.nvim".
+      config = function()
+        require("diffview").setup({ rename_threshold = 30 }) -- Recommended to match the rename threshold of Gitlab
+      end,
+    },
     "stevearc/dressing.nvim", -- Recommended but not required. Better UI for pickers.
     "nvim-tree/nvim-web-devicons", -- Recommended but not required. Icons in discussion tree.
   },

cmd/app/comment_helpers.go

@@ -7,17 +7,27 @@ import (
 	gitlab "gitlab.com/gitlab-org/api/client-go"
 )
 
-/* LinePosition represents a position in a line range. Unlike the Gitlab struct, this does not contain LineCode with a sha1 of the filename */
-type LinePosition struct {
+/* PositionInfo represents one endpoint (start or end) of a line range, as sent by the Lua
+* plugin. Unlike the Gitlab struct, it has no LineCode - Lua can't compute a sha1, so
+* buildCommentPosition computes one below from OldLine and NewLine.
+*
+* OldLine and NewLine are always real, non-nil integers, even when Type is "old" or "new"
+* and only one side actually has a line. On the side that doesn't, the value is a position
+* marker, not a claim that a line exists there: it's wherever that side's cursor was
+* sitting when the other side's line was found. LineCode is always built from this
+* unzeroed pair; buildCommentPosition separately zeroes the inapplicable side before
+* setting it on the request's LineRange.{Start,End}.{OldLine,NewLine} - see
+* zeroInapplicableLine. */
+type PositionInfo struct {
 	Type    string `json:"type"`
 	OldLine int64  `json:"old_line"`
 	NewLine int64  `json:"new_line"`
 }
 
 /* LineRange represents the range of a note. */
 type LineRange struct {
-	StartRange *LinePosition `json:"start"`
-	EndRange   *LinePosition `json:"end"`
+	Start *PositionInfo `json:"start" validate:"required"`
+	End   *PositionInfo `json:"end" validate:"required"`
 }
 
 /* PositionData represents the position of a comment or note (relative to a file diff) */
@@ -30,7 +40,7 @@ type PositionData struct {
 	BaseCommitSHA  string     `json:"base_commit_sha"`
 	StartCommitSHA string     `json:"start_commit_sha"`
 	Type           string     `json:"type"`
-	LineRange      *LineRange `json:"line_range,omitempty"`
+	LineRange      *LineRange `json:"line_range" validate:"required_with=FileName"`
 }
 
 /* RequestWithPosition is an interface that abstracts the handling of position data for a comment or a draft comment */
@@ -59,31 +69,54 @@ func buildCommentPosition(commentWithPositionData RequestWithPosition) *gitlab.P
 		OldLine:      positionData.OldLine,
 	}
 
-	if positionData.LineRange != nil {
-		shaFormat := "%x_%d_%d"
-		startFilenameSha := fmt.Sprintf(
-			shaFormat,
-			sha1.Sum([]byte(positionData.FileName)),
-			positionData.LineRange.StartRange.OldLine,
-			positionData.LineRange.StartRange.NewLine,
-		)
-		endFilenameSha := fmt.Sprintf(
-			shaFormat,
-			sha1.Sum([]byte(positionData.FileName)),
-			positionData.LineRange.EndRange.OldLine,
-			positionData.LineRange.EndRange.NewLine,
-		)
-		opt.LineRange = &gitlab.LineRangeOptions{
-			Start: &gitlab.LinePositionOptions{
-				Type:     &positionData.LineRange.StartRange.Type,
-				LineCode: &startFilenameSha,
-			},
-			End: &gitlab.LinePositionOptions{
-				Type:     &positionData.LineRange.EndRange.Type,
-				LineCode: &endFilenameSha,
-			},
-		}
+	shaFormat := "%x_%d_%d"
+	startFilenameSha := fmt.Sprintf(
+		shaFormat,
+		sha1.Sum([]byte(positionData.FileName)),
+		positionData.LineRange.Start.OldLine,
+		positionData.LineRange.Start.NewLine,
+	)
+	endFilenameSha := fmt.Sprintf(
+		shaFormat,
+		sha1.Sum([]byte(positionData.FileName)),
+		positionData.LineRange.End.OldLine,
+		positionData.LineRange.End.NewLine,
+	)
+
+	startOldLine, startNewLine := zeroInapplicableLine(positionData.LineRange.Start)
+	endOldLine, endNewLine := zeroInapplicableLine(positionData.LineRange.End)
+
+	opt.LineRange = &gitlab.LineRangeOptions{
+		Start: &gitlab.LinePositionOptions{
+			Type:     &positionData.LineRange.Start.Type,
+			LineCode: &startFilenameSha,
+			OldLine:  &startOldLine,
+			NewLine:  &startNewLine,
+		},
+		End: &gitlab.LinePositionOptions{
+			Type:     &positionData.LineRange.End.Type,
+			LineCode: &endFilenameSha,
+			OldLine:  &endOldLine,
+			NewLine:  &endNewLine,
+		},
 	}
 
 	return opt
 }
+
+/* zeroInapplicableLine returns a line_range endpoint's OldLine/NewLine with the side
+* that its Type doesn't apply to zeroed out: NewLine for a deleted ("old") line, OldLine
+* for an added ("new") line. Both stay real for an unmodified ("") or "expanded" line.
+* The unzeroed pair is still what the LineCode hash above is computed from - Gitlab
+* expects LineCode to encode the real old/new correspondence even when the displayed
+* OldLine or NewLine is zeroed. */
+func zeroInapplicableLine(position *PositionInfo) (oldLine int64, newLine int64) {
+	oldLine, newLine = position.OldLine, position.NewLine
+	switch position.Type {
+	case "old":
+		newLine = 0
+	case "new":
+		oldLine = 0
+	}
+	return oldLine, newLine
+}

cmd/app/comment_helpers_test.go

@@ -0,0 +1,65 @@
+package app
+
+import (
+	"testing"
+)
+
+func TestBuildCommentPosition(t *testing.T) {
+	makePositionData := func(startType string, startOld, startNew int64, endType string, endOld, endNew int64) PositionData {
+		return PositionData{
+			FileName:       "file.txt",
+			HeadCommitSHA:  "head-sha",
+			BaseCommitSHA:  "base-sha",
+			StartCommitSHA: "start-sha",
+			Type:           "text",
+			LineRange: &LineRange{
+				Start: &PositionInfo{Type: startType, OldLine: startOld, NewLine: startNew},
+				End:   &PositionInfo{Type: endType, OldLine: endOld, NewLine: endNew},
+			},
+		}
+	}
+
+	t.Run("zeroes NewLine for a deleted (\"old\") line, keeping LineCode's real pair", func(t *testing.T) {
+		positionData := makePositionData("", 4, 4, "old", 5, 5)
+		opt := buildCommentPosition(CommentWithPosition{PositionData: positionData})
+
+		assert(t, *opt.LineRange.End.OldLine, int64(5))
+		assert(t, *opt.LineRange.End.NewLine, int64(0))
+		assert(t, *opt.LineRange.End.LineCode, "5436437fa01a7d3e41d46741da54b451446774ca_5_5")
+	})
+
+	t.Run("zeroes OldLine for an added (\"new\") line, keeping LineCode's real pair", func(t *testing.T) {
+		positionData := makePositionData("", 4, 4, "new", 5, 5)
+		opt := buildCommentPosition(CommentWithPosition{PositionData: positionData})
+
+		assert(t, *opt.LineRange.End.OldLine, int64(0))
+		assert(t, *opt.LineRange.End.NewLine, int64(5))
+		assert(t, *opt.LineRange.End.LineCode, "5436437fa01a7d3e41d46741da54b451446774ca_5_5")
+	})
+
+	t.Run("keeps both lines real for an unmodified (\"\") line", func(t *testing.T) {
+		positionData := makePositionData("", 4, 4, "", 5, 6)
+		opt := buildCommentPosition(CommentWithPosition{PositionData: positionData})
+
+		assert(t, *opt.LineRange.End.OldLine, int64(5))
+		assert(t, *opt.LineRange.End.NewLine, int64(6))
+	})
+
+	t.Run("keeps both lines real for an expanded line", func(t *testing.T) {
+		positionData := makePositionData("", 4, 4, "expanded", 59, 61)
+		opt := buildCommentPosition(CommentWithPosition{PositionData: positionData})
+
+		assert(t, *opt.LineRange.End.OldLine, int64(59))
+		assert(t, *opt.LineRange.End.NewLine, int64(61))
+	})
+
+	t.Run("zeroes the start and end independently", func(t *testing.T) {
+		positionData := makePositionData("new", 0, 50, "", 60, 62)
+		opt := buildCommentPosition(CommentWithPosition{PositionData: positionData})
+
+		assert(t, *opt.LineRange.Start.OldLine, int64(0))
+		assert(t, *opt.LineRange.Start.NewLine, int64(50))
+		assert(t, *opt.LineRange.End.OldLine, int64(60))
+		assert(t, *opt.LineRange.End.NewLine, int64(62))
+	})
+}

cmd/app/comment_test.go

@@ -59,6 +59,10 @@ func TestPostComment(t *testing.T) {
 			Comment: "Some comment",
 			PositionData: PositionData{
 				FileName: "file.txt",
+				LineRange: &LineRange{
+					Start: &PositionInfo{Type: "", OldLine: 4, NewLine: 4},
+					End:   &PositionInfo{Type: "", OldLine: 4, NewLine: 4},
+				},
 			},
 		}
 		request := makeRequest(t, http.MethodPost, "/mr/comment", testCommentCreationData)

cmd/app/middleware_test.go

@@ -111,4 +111,39 @@ func TestValidatorMiddleware(t *testing.T) {
 		), request)
 		assert(t, data.Message, "Some message")
 	})
+	t.Run("Should reject a line_range with a missing endpoint instead of panicking", func(t *testing.T) {
+		payload := PostCommentRequest{
+			Comment: "Some comment",
+			PositionData: PositionData{
+				FileName:  "file.txt",
+				LineRange: &LineRange{}, // Start and End left nil
+			},
+		}
+		request := makeRequest(t, http.MethodPost, "/mr/comment", payload)
+		svc := middleware(
+			commentService{testProjectData, fakeCommentClient{}},
+			withMr(testProjectData, fakeMergeRequestLister{}),
+			withPayloadValidation(methodToPayload{http.MethodPost: newPayload[PostCommentRequest]}),
+			withMethodCheck(http.MethodPost),
+		)
+		data, status := getFailData(t, svc, request)
+		assert(t, data.Message, "Invalid payload")
+		assert(t, data.Details, "Start is required; End is required")
+		assert(t, status, http.StatusBadRequest)
+	})
+	t.Run("Should allow a missing line_range when there is no FileName (unlinked comment)", func(t *testing.T) {
+		payload := PostCommentRequest{
+			Comment: "Some comment",
+			// PositionData is left zero-valued: no FileName, no LineRange.
+		}
+		request := makeRequest(t, http.MethodPost, "/mr/comment", payload)
+		svc := middleware(
+			commentService{testProjectData, fakeCommentClient{}},
+			withMr(testProjectData, fakeMergeRequestLister{}),
+			withPayloadValidation(methodToPayload{http.MethodPost: newPayload[PostCommentRequest]}),
+			withMethodCheck(http.MethodPost),
+		)
+		data := getSuccessData(t, svc, request)
+		assert(t, data.Message, "Comment created successfully")
+	})
 }

doc/gitlab.nvim.txt

@@ -85,6 +85,7 @@ With |vim.pack| (the built-in plugin manager on Neovim 0.12 and newer):
     "https://github.com/nvim-tree/nvim-web-devicons", -- Recommended but not required. Icons in discussion tree.
     "https://github.com/harrisoncramer/gitlab.nvim",
   })
+  require("diffview").setup({rename_threshold = 30}) -- Recommended to match the rename threshold of Gitlab
   ---@type GitlabSettings
   local opts = {} -- Your configuration
   require("gitlab").setup(opts)
@@ -97,7 +98,10 @@ With folke/lazy.nvim:
     -- branch = "main", -- Uncomment to use a stable version. The default, possibly unstable, but more actively maintained branch is `develop`.
     dependencies = {
       "MunifTanjim/nui.nvim",
-      "dlyongemallo/diffview-plus.nvim", -- Maintained fork of "sindrets/diffview.nvim".
+      {
+        "dlyongemallo/diffview-plus.nvim", -- Maintained fork of "sindrets/diffview.nvim".
+        opts = { rename_threshold = 30 }, -- Recommended to match the rename threshold of Gitlab
+      },
       "stevearc/dressing.nvim", -- Recommended but not required. Better UI for pickers.
       "nvim-tree/nvim-web-devicons", -- Recommended but not required. Icons in discussion tree.
     },
@@ -112,7 +116,12 @@ And with pckr.nvim:
     -- branch = "main", -- Uncomment to use a stable version. The default, possibly unstable, but more actively maintained branch is `develop`.
     requires = {
       "MunifTanjim/nui.nvim",
-      "dlyongemallo/diffview-plus.nvim", -- Maintained fork of "sindrets/diffview.nvim".
+      {
+        "dlyongemallo/diffview-plus.nvim", -- Maintained fork of "sindrets/diffview.nvim".
+        config = function()
+          require("diffview").setup({ rename_threshold = 30 }) -- Recommended to match the rename threshold of Gitlab
+        end,
+      },
       "stevearc/dressing.nvim", -- Recommended but not required. Better UI for pickers.
       "nvim-tree/nvim-web-devicons", -- Recommended but not required. Icons in discussion tree.
     },