Feature/improve-line-comments by ccsert · Pull Request #6 · ccsert/opencode-review-gitea

ccsert · 2026-01-31T12:24:35Z

No description provided.

Based on Gitea API swagger documentation (CreatePullReviewComment): - new_position: line number in new file (for added/context lines) - old_position: line number in old file (for deleted lines) Changes: - gitea-pr-diff: Output [NEW:X] for new lines, [OLD:X] for deleted lines - gitea-review: Add old_line parameter, map to old_position/new_position - code-review agent: Update prompts with new line number format This enables AI to comment on both added and deleted lines accurately.

Copilot

Pull request overview

This pull request enhances the line comment functionality for the Gitea code review tool by adding support for commenting on deleted lines. Previously, the tool could only comment on added or context lines; now it supports both line (for new/context lines) and old_line (for deleted lines) parameters.

Changes:

Added old_line parameter support to enable comments on deleted lines in diffs
Updated diff output format to distinguish between [NEW:X] and [OLD:X] line markers
Enhanced documentation and examples to clarify the two-mode targeting system

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 7 comments.

Show a summary per file

File	Description
`.opencode-review/tools/gitea-review.txt`	Updated documentation with line comment types section and JSON examples showing both `line` and `old_line` usage
`.opencode-review/tools/gitea-review.ts`	Modified schema to accept optional `line` and `old_line` parameters, updated comment mapping logic to set `new_position`/`old_position`
`.opencode-review/tools/gitea-pr-diff.txt`	Updated format description to show `[NEW:X]` and `[OLD:X]` prefixes with usage instructions
`.opencode-review/tools/gitea-pr-diff.ts`	Changed formatDiffForReview to output `[NEW:X]` and `[OLD:X]` markers with line numbers for all change types
`.opencode-review/tests/gitea-pr-diff.test.ts`	Added comprehensive test file to verify diff parsing and comment mapping logic
`.opencode-review/agents/code-review.md`	Enhanced agent instructions with detailed line number format section and examples for both comment modes

Copilot · 2026-01-31T12:29:01Z

+        // Comment on deleted line (old file)
+        comment.old_position = c.old_line
+        comment.new_position = 0
+      } else if (c.line !== undefined && c.line > 0) {
+        // Comment on new/context line (new file)
+        comment.new_position = c.line
+        comment.old_position = 0


Setting positions to 0 may not be correct according to Gitea/Forgejo API semantics. When commenting on a new line, the code sets old_position = 0, and when commenting on a deleted line, it sets new_position = 0.

According to typical diff commenting APIs (including GitHub's which Gitea is based on), position fields that aren't applicable should usually be omitted (undefined) rather than explicitly set to 0. Setting them to 0 might be interpreted as "line 0" which doesn't exist, potentially causing API errors or comments to appear in unexpected locations.

Consider either:

Omitting the inapplicable position field entirely (don't set it at all)

Verifying with Gitea API documentation that setting to 0 is the correct approach

If setting to 0 is confirmed correct, add a comment explaining this is intentional per Gitea API requirements.

Suggested change

// Comment on deleted line (old file)

comment.old_position = c.old_line

comment.new_position = 0

} else if (c.line !== undefined && c.line > 0) {

// Comment on new/context line (new file)

comment.new_position = c.line

comment.old_position = 0

// Comment on deleted line (old file) - only set old_position

comment.old_position = c.old_line

} else if (c.line !== undefined && c.line > 0) {

// Comment on new/context line (new file) - only set new_position

comment.new_position = c.line

Copilot · 2026-01-31T12:29:02Z

+```json
+{
+  "owner": "org",
+  "repo": "project", 


Trailing whitespace after "project" on line 22. There's an extra space at the end of the line after the comma in the JSON example. While this doesn't affect functionality since it's in a documentation file, it's a style inconsistency.

Suggested change

"repo": "project",

"repo": "project",

Copilot · 2026-01-31T12:29:02Z

+      if (c.old_line !== undefined && c.old_line > 0) {
+        // Comment on deleted line (old file)
+        comment.old_position = c.old_line
+        comment.new_position = 0
+      } else if (c.line !== undefined && c.line > 0) {
+        // Comment on new/context line (new file)
+        comment.new_position = c.line
+        comment.old_position = 0
+      }


Missing validation for comments that have both 'line' and 'old_line' defined, or neither defined. According to the documentation, users should use one OR the other, but there's no enforcement. If both are provided, the code will use old_line (due to the if-else ordering), which may not be the user's intent. If neither is provided, a comment will be created with no position fields, which could cause API errors or unexpected behavior.

Consider adding validation to ensure exactly one of 'line' or 'old_line' is provided, and throw a descriptive error if the constraint is violated.

Copilot · 2026-01-31T12:29:02Z

+          // New line: [NEW:行号] +内容
+          parts.push(`[NEW:${change.newLine}] +${change.content}`)
        } else if (change.type === "del") {
-          parts.push(`[DEL] -${change.content}`)
+          // Deleted line: [OLD:行号] -内容 (用于 old_position 评论)
+          parts.push(`[OLD:${change.oldLine}] -${change.content}`)
        } else {
-          parts.push(`[${change.newLine}]  ${change.content}`)
+          // Context line: [NEW:行号] 内容
+          parts.push(`[NEW:${change.newLine}]  ${change.content}`)


Comments in Chinese found in production code. While the comments accurately describe the logic, mixing languages in comments is inconsistent with the English comments used throughout the rest of the codebase. For maintainability and consistency, all comments should be in English.

Consider translating these comments:

Line 135: "New line: [NEW:行号] +内容" → "New line: [NEW:line_number] +content"

Line 138: "Deleted line: [OLD:行号] -内容 (用于 old_position 评论)" → "Deleted line: [OLD:line_number] -content (for old_position comments)"

Line 141: "Context line: [NEW:行号] 内容" → "Context line: [NEW:line_number] content"

Copilot · 2026-01-31T12:29:03Z

+/**
+ * 本地验证 diff 解析逻辑
+ * 运行: cd .opencode-review && bun run gitea-pr-diff.test.ts
+ */
+
+// 模拟 parseDiff 和 formatDiffForReview 函数
+interface ParsedChange {
+  type: "add" | "del" | "normal"
+  content: string
+  oldLine?: number
+  newLine?: number
+}
+
+interface ParsedHunk {
+  oldStart: number
+  newStart: number
+  changes: ParsedChange[]
+}
+
+interface ParsedFile {
+  path: string
+  status: string
+  hunks: ParsedHunk[]
+}
+
+function parseDiff(diffText: string): ParsedFile[] {
+  const files: ParsedFile[] = []
+  const lines = diffText.split("\n")
+  let i = 0
+
+  while (i < lines.length) {
+    const line = lines[i]
+
+    if (line.startsWith("diff --git")) {
+      const pathMatch = line.match(/b\/(.+)$/)
+      const path = pathMatch ? pathMatch[1] : "unknown"
+
+      let status = "modified"
+      while (i < lines.length && !lines[i].startsWith("@@")) {
+        if (lines[i].startsWith("new file")) status = "added"
+        else if (lines[i].startsWith("deleted file")) status = "deleted"
+        else if (lines[i].startsWith("rename")) status = "renamed"
+        i++
+      }
+
+      const hunks: ParsedHunk[] = []
+
+      while (i < lines.length && !lines[i].startsWith("diff --git")) {
+        if (lines[i].startsWith("@@")) {
+          const hunkMatch = lines[i].match(/@@ -(\d+)(?:,\d+)? \+(\d+)(?:,\d+)? @@/)
+          if (hunkMatch) {
+            const oldStart = parseInt(hunkMatch[1], 10)
+            const newStart = parseInt(hunkMatch[2], 10)
+            const changes: ParsedChange[] = []
+            let oldLine = oldStart
+            let newLine = newStart
+
+            i++
+            while (i < lines.length && !lines[i].startsWith("@@") && !lines[i].startsWith("diff --git")) {
+              const content = lines[i].slice(1)
+              if (lines[i].startsWith("+")) {
+                changes.push({ type: "add", content, newLine: newLine++ })
+              } else if (lines[i].startsWith("-")) {
+                changes.push({ type: "del", content, oldLine: oldLine++ })
+              } else if (lines[i].startsWith(" ")) {
+                changes.push({ type: "normal", content, oldLine: oldLine++, newLine: newLine++ })
+              }
+              i++
+            }
+
+            hunks.push({ oldStart, newStart, changes })
+          } else {
+            i++
+          }
+        } else {
+          i++
+        }
+      }
+
+      files.push({ path, status, hunks })
+    } else {
+      i++
+    }
+  }
+
+  return files
+}
+
+function formatDiffForReview(files: ParsedFile[]): string {
+  const parts: string[] = []
+
+  for (const file of files) {
+    parts.push(`\n## ${file.path} (${file.status})\n`)
+
+    for (const hunk of file.hunks) {
+      parts.push(`@@ starting at line ${hunk.newStart} (old: ${hunk.oldStart}) @@`)
+
+      for (const change of hunk.changes) {
+        if (change.type === "add") {
+          parts.push(`[NEW:${change.newLine}] +${change.content}`)
+        } else if (change.type === "del") {
+          parts.push(`[OLD:${change.oldLine}] -${change.content}`)
+        } else {
+          parts.push(`[NEW:${change.newLine}]  ${change.content}`)
+        }
+      }
+      parts.push("")
+    }
+  }
+
+  return parts.join("\n")
+}
+
+// ============ 测试用例 ============
+
+const testDiff = `diff --git a/src/app.ts b/src/app.ts
+index 1234567..abcdefg 100644
+--- a/src/app.ts
+++ b/src/app.ts
+@@ -10,7 +10,8 @@ function main() {
+   const config = loadConfig()
+   console.log("Starting app")
+-  const oldValue = "deprecated"
+  const newValue = "updated"
+  const extraLine = "added"
+   return config
+ }
+`
+
+console.log("=== 测试 Diff 解析 ===\n")
+
+const parsed = parseDiff(testDiff)
+console.log("解析结果:")
+console.log(JSON.stringify(parsed, null, 2))
+
+console.log("\n=== 格式化输出 ===\n")
+
+const formatted = formatDiffForReview(parsed)
+console.log(formatted)
+
+console.log("\n=== 验证检查 ===\n")
+
+// 验证删除行有 oldLine
+const delChange = parsed[0]?.hunks[0]?.changes.find(c => c.type === "del")
+if (delChange && delChange.oldLine) {
+  console.log(`✅ 删除行有 oldLine: ${delChange.oldLine}`)
+} else {
+  console.log("❌ 删除行缺少 oldLine")
+}
+
+// 验证添加行有 newLine
+const addChange = parsed[0]?.hunks[0]?.changes.find(c => c.type === "add")
+if (addChange && addChange.newLine) {
+  console.log(`✅ 添加行有 newLine: ${addChange.newLine}`)
+} else {
+  console.log("❌ 添加行缺少 newLine")
+}
+
+// 验证格式化输出包含 [OLD:X] 和 [NEW:X]
+if (formatted.includes("[OLD:") && formatted.includes("[NEW:")) {
+  console.log("✅ 格式化输出包含 [OLD:X] 和 [NEW:X] 格式")
+} else {
+  console.log("❌ 格式化输出格式不正确")
+}
+
+console.log("\n=== 模拟 Review 评论映射 ===\n")
+
+// 模拟 gitea-review 的评论映射逻辑
+interface Comment {
+  path: string
+  line?: number
+  old_line?: number
+  body: string
+}
+
+function mapComment(c: Comment) {
+  const result: { path: string; body: string; new_position?: number; old_position?: number } = {
+    path: c.path,
+    body: c.body,
+  }
+
+  if (c.old_line !== undefined && c.old_line > 0) {
+    result.old_position = c.old_line
+    result.new_position = 0
+  } else if (c.line !== undefined && c.line > 0) {
+    result.new_position = c.line
+    result.old_position = 0
+  }
+
+  return result
+}
+
+// 测试评论映射
+const testComments: Comment[] = [
+  { path: "src/app.ts", line: 13, body: "Consider const here" },
+  { path: "src/app.ts", old_line: 12, body: "This was important logic" },
+]
+
+console.log("输入评论:")
+console.log(JSON.stringify(testComments, null, 2))
+
+console.log("\n映射到 Gitea API 格式:")
+const mapped = testComments.map(mapComment)
+console.log(JSON.stringify(mapped, null, 2))
+
+// 验证映射结果
+const newLineComment = mapped.find(m => m.new_position && m.new_position > 0)
+const oldLineComment = mapped.find(m => m.old_position && m.old_position > 0)
+
+if (newLineComment && newLineComment.old_position === 0) {
+  console.log("\n✅ 新行评论正确映射: new_position=" + newLineComment.new_position + ", old_position=0")
+} else {
+  console.log("\n❌ 新行评论映射错误")
+}
+
+if (oldLineComment && oldLineComment.new_position === 0) {
+  console.log("✅ 旧行评论正确映射: old_position=" + oldLineComment.old_position + ", new_position=0")
+} else {
+  console.log("❌ 旧行评论映射错误")
+}
+
+console.log("\n=== 测试完成 ===")


Comments in Chinese found in test file. The test includes Chinese comments and console output which is inconsistent with the English used throughout the codebase. For maintainability and international collaboration, all code comments and output should be in English.

Examples include:

Line 2: "本地验证 diff 解析逻辑" (Local verification of diff parsing logic)

Line 114: "测试用例" (Test cases)

Line 129: "测试 Diff 解析" (Test Diff parsing)

Line 145, 155: "删除行", "添加行" (Deleted line, Added line)

Multiple console.log messages with Chinese text

Copilot · 2026-01-31T12:29:03Z

        if (change.type === "add") {
-          parts.push(`[${change.newLine}] +${change.content}`)
+          // New line: [NEW:行号] +内容
+          parts.push(`[NEW:${change.newLine}] +${change.content}`)
        } else if (change.type === "del") {
-          parts.push(`[DEL] -${change.content}`)
+          // Deleted line: [OLD:行号] -内容 (用于 old_position 评论)
+          parts.push(`[OLD:${change.oldLine}] -${change.content}`)
        } else {
-          parts.push(`[${change.newLine}]  ${change.content}`)
+          // Context line: [NEW:行号] 内容
+          parts.push(`[NEW:${change.newLine}]  ${change.content}`)


Potential issue with undefined line numbers in formatted output. For deleted lines, if 'change.oldLine' is undefined, the output would be '[OLD:undefined] -content', which is incorrect. Similarly, for added/context lines, if 'change.newLine' is undefined, the output would be '[NEW:undefined] content'.

While the parsing logic (lines 90-101) appears to always set these values, the TypeScript interface (line 42-43) defines them as optional. If the parsing logic has any bugs or edge cases where these values aren't set, this would produce malformed output.

Consider adding defensive checks or assertions to ensure these values are always defined before formatting, or update the interface to reflect that these fields are always present in their respective contexts.

Copilot · 2026-01-31T12:29:03Z

+const testDiff = `diff --git a/src/app.ts b/src/app.ts
+index 1234567..abcdefg 100644
+--- a/src/app.ts
+++ b/src/app.ts


The test diff string has incorrect formatting. Line 119 shows +++ b/src/app.ts (the standard git diff format for the new file path), but according to the diff view, there's an extra + character at line 119, making it ++++ b/src/app.ts. This is incorrect git diff syntax and will likely cause the parseDiff function to fail or skip this file header.

The correct format should be:

--- a/src/app.ts +++ b/src/app.ts

Not:

--- a/src/app.ts ++++ b/src/app.ts

ccsert added 3 commits January 31, 2026 19:46

test: add local validation for diff parsing and comment mapping

c1401d5

fix: move test file to tests/ to prevent OpenCode loading it as tool

ade6adb

Copilot AI review requested due to automatic review settings January 31, 2026 12:24

Copilot started reviewing on behalf of ccsert January 31, 2026 12:24 View session

ccsert merged commit 9e51cf5 into main Jan 31, 2026
5 of 6 checks passed

Copilot AI reviewed Jan 31, 2026

View reviewed changes

ccsert deleted the feature/improve-line-comments branch January 31, 2026 12:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/improve-line-comments#6

Feature/improve-line-comments#6
ccsert merged 3 commits into
mainfrom
feature/improve-line-comments

ccsert commented Jan 31, 2026

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Jan 31, 2026

Uh oh!

Copilot AI Jan 31, 2026

Uh oh!

Copilot AI Jan 31, 2026

Uh oh!

Copilot AI Jan 31, 2026

Uh oh!

Copilot AI Jan 31, 2026

Uh oh!

Copilot AI Jan 31, 2026

Uh oh!

Copilot AI Jan 31, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

ccsert commented Jan 31, 2026

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Jan 31, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 31, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 31, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 31, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 31, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 31, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 31, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants