From b2eea502d0ae10ee4a21d5f9a66348094829bfef Mon Sep 17 00:00:00 2001
From: claw <claw@weiker.me>
Date: Tue, 12 May 2026 13:57:44 -0700
Subject: [PATCH] refactor(gitea): address review feedback on PR #90

- position.go: Replace O(n) maxPosition scan with O(1) lookup by
  tracking max position during map construction. Also eliminates
  shadowing of the builtin max identifier (Go 1.21+).
- position.go: Add comment clarifying +++ prefix ordering intent.
- adapter.go: Document diff-fetch tradeoff in PostReview.
- adapter_test.go: Remove extra blank line between test functions.
---
 gitea/adapter.go      |  6 +++++-
 gitea/adapter_test.go |  1 -
 gitea/position.go     | 27 +++++++++++++++++----------
 3 files changed, 22 insertions(+), 12 deletions(-)

diff --git a/gitea/adapter.go b/gitea/adapter.go
index b3d8122..d6134fb 100644
--- a/gitea/adapter.go
+++ b/gitea/adapter.go
@@ -150,7 +150,11 @@ func (a *Adapter) PostReview(ctx context.Context, owner, repo string, number int
 
 	var giteaComments []ReviewComment
 	if len(req.Comments) > 0 {
-		// Fetch diff to build position → line number map
+		// Fetch diff to build position → line number map.
+		// The diff is fetched unconditionally when comments exist. This adds latency
+		// for reviews with inline comments but keeps the implementation simple — caching
+		// the diff across calls would add complexity for minimal gain since PostReview
+		// is called at most once per review cycle.
 		diff, err := a.client.GetPullRequestDiff(ctx, owner, repo, number)
 		if err != nil {
 			return nil, fmt.Errorf("fetch diff for position translation: %w", err)
diff --git a/gitea/adapter_test.go b/gitea/adapter_test.go
index f272116..0071699 100644
--- a/gitea/adapter_test.go
+++ b/gitea/adapter_test.go
@@ -354,7 +354,6 @@ func TestAdapter_ListContents(t *testing.T) {
 	}
 }
 
-
 func TestAdapter_GetFileContent_RefRouting(t *testing.T) {
 	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 		// When ref is provided, the URL should contain ?ref=
diff --git a/gitea/position.go b/gitea/position.go
index 4be26b4..e1aa4be 100644
--- a/gitea/position.go
+++ b/gitea/position.go
@@ -12,6 +12,9 @@ type PositionMap struct {
 	// files maps filename → (position → new-file line number).
 	// Deletion lines are mapped to -1 (no new-file line).
 	files map[string]map[int]int
+	// maxPositions caches the highest position number per file,
+	// tracked during construction to avoid O(n) scans at translate time.
+	maxPositions map[string]int
 }
 
 // Translate converts a GitHub diff-position to a new-file line number for a given file.
@@ -53,15 +56,9 @@ func (pm *PositionMap) Translate(file string, position int) (int, error) {
 }
 
 // maxPosition returns the highest position number for a file.
-// O(n) per call — acceptable since deletion-line fallback is rare and n is small (typical hunk size).
+// O(1) — the maximum is tracked during map construction.
 func (pm *PositionMap) maxPosition(file string) int {
-	max := 0
-	for pos := range pm.files[file] {
-		if pos > max {
-			max = pos
-		}
-	}
-	return max
+	return pm.maxPositions[file]
 }
 
 // BuildPositionToLineMap parses a unified diff and builds a PositionMap
@@ -74,7 +71,10 @@ func (pm *PositionMap) maxPosition(file string) int {
 //   - Position maps to the new file line number for additions and context lines
 //   - Deletion lines have a position but no new-file line number (stored as -1)
 func BuildPositionToLineMap(diff string) *PositionMap {
-	pm := &PositionMap{files: make(map[string]map[int]int)}
+	pm := &PositionMap{
+		files:        make(map[string]map[int]int),
+		maxPositions: make(map[string]int),
+	}
 
 	lines := strings.Split(diff, "\n")
 	var currentFile string
@@ -82,7 +82,10 @@ func BuildPositionToLineMap(diff string) *PositionMap {
 	var newLine int
 
 	for _, line := range lines {
-		// Detect new file in diff
+		// Detect new file in diff.
+		// "+++ b/" is checked before "+++ /dev/null" — the two prefixes are
+		// non-overlapping ("+++ /dev/null" does not start with "+++ b/"), so
+		// ordering is independent. Checking the common case first for clarity.
 		if strings.HasPrefix(line, "+++ b/") {
 			currentFile = strings.TrimPrefix(line, "+++ b/")
 			position = 0
@@ -123,6 +126,7 @@ func BuildPositionToLineMap(diff string) *PositionMap {
 		// Parse hunk headers
 		if strings.HasPrefix(line, "@@") && currentFile != "" {
 			position++
+			pm.maxPositions[currentFile] = position
 			newLine = parseHunkStart(line)
 			continue
 		}
@@ -141,15 +145,18 @@ func BuildPositionToLineMap(diff string) *PositionMap {
 			// Addition: has a new-file line number
 			position++
 			pm.files[currentFile][position] = newLine
+			pm.maxPositions[currentFile] = position
 			newLine++
 		} else if strings.HasPrefix(line, "-") {
 			// Deletion: has a position but no new-file line number
 			position++
 			pm.files[currentFile][position] = -1
+			pm.maxPositions[currentFile] = position
 		} else if strings.HasPrefix(line, " ") {
 			// Context line
 			position++
 			pm.files[currentFile][position] = newLine
+			pm.maxPositions[currentFile] = position
 			newLine++
 		}
 	}