feat(api,ui): add multiline comment on pullrequest (#12582)
Closes https://codeberg.org/forgejo/forgejo/issues/6093 This PR adds support for **multi-line review comments** on pull requests, allowing reviewers to select a range of lines in diffs instead of only a single line — similar to GitHub's implementation. ### Tests for Go changes - I added test coverage for Go changes... - [X] in their respective `*_test.go` for unit tests. - [X] `make pr-go` before pushing Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/12582 Reviewed-by: Mathieu Fenniak <mfenniak@noreply.codeberg.org>
This commit is contained in:
committed by
Mathieu Fenniak
parent
f576a1a21e
commit
6a27eb051d
@@ -1285,6 +1285,9 @@ LEVEL = Info
|
||||
;; Number of line of codes shown for a code comment
|
||||
;CODE_COMMENT_LINES = 4
|
||||
;;
|
||||
;; Maximum number of lines a single (multi-line) code comment may span. 0 means no limit.
|
||||
;MAX_CODE_COMMENT_LINES = 50
|
||||
;;
|
||||
;; Max size of files to be displayed (default is 8MiB)
|
||||
;MAX_DISPLAY_FILE_SIZE = 8388608
|
||||
;;
|
||||
|
||||
@@ -0,0 +1,25 @@
|
||||
// Copyright 2026 The Forgejo Authors. All rights reserved.
|
||||
// SPDX-License-Identifier: GPL-3.0-or-later
|
||||
|
||||
package forgejo_migrations
|
||||
|
||||
import "code.forgejo.org/xorm/xorm"
|
||||
|
||||
func init() {
|
||||
registerMigration(&Migration{
|
||||
Description: "add extra_lines_count to comment for multi-line review comments",
|
||||
Upgrade: addCommentExtraLinesCount,
|
||||
})
|
||||
}
|
||||
|
||||
func addCommentExtraLinesCount(x *xorm.Engine) error {
|
||||
type Comment struct {
|
||||
ExtraLinesCount int64 `xorm:"NOT NULL DEFAULT 0"`
|
||||
}
|
||||
|
||||
_, err := x.SyncWithOptions(
|
||||
xorm.SyncOptions{IgnoreDropIndices: true},
|
||||
new(Comment),
|
||||
)
|
||||
return err
|
||||
}
|
||||
+124
-37
@@ -283,6 +283,7 @@ type Comment struct {
|
||||
|
||||
CommitID int64
|
||||
Line int64 // - previous line / + proposed line
|
||||
ExtraLinesCount int64 `xorm:"NOT NULL DEFAULT 0"` // number of additional lines after Line (0 = single line)
|
||||
TreePath string
|
||||
Content string `xorm:"LONGTEXT"`
|
||||
ContentVersion int `xorm:"NOT NULL DEFAULT 0"`
|
||||
@@ -751,6 +752,58 @@ func (c *Comment) UnsignedLine() uint64 {
|
||||
return uint64(c.Line)
|
||||
}
|
||||
|
||||
// DisplayLine returns the signed line number where the comment should be displayed
|
||||
// in the diff view. For multi-line comments, this is the last line of the range.
|
||||
func (c *Comment) DisplayLine() int64 {
|
||||
if c.Line < 0 {
|
||||
return c.Line - c.ExtraLinesCount
|
||||
}
|
||||
return c.Line + c.ExtraLinesCount
|
||||
}
|
||||
|
||||
// UnsignedDisplayLine returns the unsigned (absolute) display line number.
|
||||
// For multi-line comments, this is the last line of the range (UnsignedLine + ExtraLinesCount).
|
||||
func (c *Comment) UnsignedDisplayLine() uint64 {
|
||||
return c.UnsignedLine() + uint64(c.ExtraLinesCount)
|
||||
}
|
||||
|
||||
// resolveLineAtHead checks whether a specific line is still present at the given head commit.
|
||||
// For positive lines (proposed side), uses git blame --reverse.
|
||||
// For negative lines (previous side), uses diff + FindAdjustedLineNumber.
|
||||
func (c *Comment) resolveLineAtHead(gitRepo *git.Repository, lineNum uint64, currentHead string) (*git.ReverseLineBlame, error) {
|
||||
if c.Line > 0 {
|
||||
blame, err := gitRepo.ReverseLineBlame(c.CommitSHA, c.TreePath, lineNum, currentHead)
|
||||
if err != nil {
|
||||
return nil, fmt.Errorf("ReverseLineBlame for line %d: %w", lineNum, err)
|
||||
}
|
||||
return blame, nil
|
||||
}
|
||||
|
||||
// For comments on removed lines, diff the commit the line was known to exist in against the head
|
||||
// being viewed, then locate the line in that diff.
|
||||
var buffer bytes.Buffer
|
||||
if err := git.GetRepoRawDiffForFile(gitRepo, c.CommitSHA, currentHead, git.RawDiffNormal, c.TreePath, &buffer); err != nil {
|
||||
return nil, fmt.Errorf("failed to get diff: %w", err)
|
||||
}
|
||||
diff := buffer.String()
|
||||
|
||||
adjustedLine, err := git.FindAdjustedLineNumber(c.Patch, int64(lineNum), strings.NewReader(diff))
|
||||
if err != nil && errors.Is(err, git.ErrLineNotFound) {
|
||||
return &git.ReverseLineBlame{
|
||||
CommitID: "", // not currentHead — indicates the line is outdated
|
||||
LineNumber: lineNum,
|
||||
FilePath: c.TreePath,
|
||||
}, nil
|
||||
} else if err != nil {
|
||||
return nil, fmt.Errorf("FindAdjustedLineNumber for line %d: %w", lineNum, err)
|
||||
}
|
||||
return &git.ReverseLineBlame{
|
||||
CommitID: currentHead,
|
||||
LineNumber: uint64(adjustedLine.Left),
|
||||
FilePath: c.TreePath,
|
||||
}, nil
|
||||
}
|
||||
|
||||
func (c *Comment) ResolveCurrentLine(ctx context.Context, repo *repo_model.Repository, currentHead string) (*git.ReverseLineBlame, error) {
|
||||
if c.reverseLineBlame != nil {
|
||||
return c.reverseLineBlame, nil
|
||||
@@ -774,43 +827,15 @@ func (c *Comment) ResolveCurrentLine(ctx context.Context, repo *repo_model.Repos
|
||||
}
|
||||
defer closer.Close()
|
||||
|
||||
var reverseBlame *git.ReverseLineBlame
|
||||
if c.Line > 0 {
|
||||
var err error
|
||||
reverseBlame, err = gitRepo.ReverseLineBlame(c.CommitSHA, c.TreePath, c.UnsignedLine(), currentHead)
|
||||
if err != nil {
|
||||
return "", fmt.Errorf("failed to perform `git blame --reverse` to resolve current line for comment (id=%d): %w", c.ID, err)
|
||||
}
|
||||
} else {
|
||||
// For comments on removed lines, perform a `git diff` between the last commit that the line of code was
|
||||
// known to exist (which is recorded as CommitSHA) and the requested head. Then inspect the diff to verify
|
||||
// that the removed line of code is present in the diff.
|
||||
buffer := bytes.Buffer{}
|
||||
err := git.GetRepoRawDiffForFile(gitRepo, c.CommitSHA, currentHead, git.RawDiffNormal, c.TreePath, &buffer)
|
||||
if err != nil {
|
||||
return "", fmt.Errorf("failed to get diff: %w", err)
|
||||
}
|
||||
|
||||
diff := buffer.String()
|
||||
adjustedLine, err := git.FindAdjustedLineNumber(c.Patch, int64(c.UnsignedLine()), strings.NewReader(diff))
|
||||
if err != nil && errors.Is(err, git.ErrLineNotFound) {
|
||||
// Line not found in the diff. Don't treat this as an error, because that would break the caching --
|
||||
// instead, return a blame where CommitID != headCommitID, which will be an indicator to callers (for
|
||||
// both resolution methods) that the line of code is outdated in the diff.
|
||||
reverseBlame = &git.ReverseLineBlame{
|
||||
CommitID: "", // not currentHead
|
||||
LineNumber: c.UnsignedLine(),
|
||||
FilePath: c.TreePath,
|
||||
}
|
||||
} else if err != nil {
|
||||
return "", fmt.Errorf("failed in finding adjusted line number: %w", err)
|
||||
} else {
|
||||
reverseBlame = &git.ReverseLineBlame{
|
||||
CommitID: currentHead,
|
||||
LineNumber: uint64(adjustedLine.Left),
|
||||
FilePath: c.TreePath,
|
||||
}
|
||||
}
|
||||
// On the previous side the patch is cut around the last line of the range, so resolve that line
|
||||
// (for single-line comments and the proposed side it equals UnsignedLine).
|
||||
resolveLine := c.UnsignedLine()
|
||||
if c.Line < 0 {
|
||||
resolveLine = c.UnsignedDisplayLine()
|
||||
}
|
||||
reverseBlame, err := c.resolveLineAtHead(gitRepo, resolveLine, currentHead)
|
||||
if err != nil {
|
||||
return "", err
|
||||
}
|
||||
|
||||
data, err := json.Marshal(reverseBlame)
|
||||
@@ -834,6 +859,66 @@ func (c *Comment) ResolveCurrentLine(ctx context.Context, repo *repo_model.Repos
|
||||
return c.reverseLineBlame, nil
|
||||
}
|
||||
|
||||
// CheckLineRangeValid reports whether a multi-line comment range can still be placed at the given head.
|
||||
// Previous (negative) side: only the last line can be located in the cut patch, so only it is checked.
|
||||
// Proposed (positive) side: modified lines are tolerated; only a line landing at an unexpected offset
|
||||
// (lines inserted/removed inside the range) invalidates it. Uses the same caching pattern as ResolveCurrentLine.
|
||||
func (c *Comment) CheckLineRangeValid(ctx context.Context, repo *repo_model.Repository, currentHead string) (bool, error) {
|
||||
if c.ExtraLinesCount <= 0 {
|
||||
return true, nil
|
||||
}
|
||||
|
||||
resultJSON, err := cache.GetString(fmt.Sprintf("comment.ResolveRange;ID=%d;HEAD=%s", c.ID, currentHead), func() (string, error) {
|
||||
gitRepo, closer, err := gitrepo.RepositoryFromContextOrOpen(ctx, repo)
|
||||
if err != nil {
|
||||
return "", fmt.Errorf("failed to open repo: %w", err)
|
||||
}
|
||||
defer closer.Close()
|
||||
|
||||
// Previous side: only the last line of the range can be located in the cut patch, so validate it.
|
||||
if c.Line < 0 {
|
||||
blame, err := c.resolveLineAtHead(gitRepo, c.UnsignedDisplayLine(), currentHead)
|
||||
if err != nil {
|
||||
return "", err
|
||||
}
|
||||
if blame.CommitID != currentHead {
|
||||
return "invalid", nil
|
||||
}
|
||||
return "valid", nil
|
||||
}
|
||||
|
||||
// Proposed side: resolve the first line; the others are expected to follow it consecutively.
|
||||
anchorBlame, err := c.resolveLineAtHead(gitRepo, c.UnsignedLine(), currentHead)
|
||||
if err != nil {
|
||||
return "", err
|
||||
}
|
||||
if anchorBlame.CommitID != currentHead {
|
||||
return "invalid", nil
|
||||
}
|
||||
anchorResolvedLine := anchorBlame.LineNumber
|
||||
|
||||
// A line that no longer resolves is treated as "modified but present" and tolerated; only a
|
||||
// resolved line landing at an unexpected offset (lines inserted/removed inside the range) is a break.
|
||||
startLine := c.UnsignedLine()
|
||||
for i := int64(1); i <= c.ExtraLinesCount; i++ {
|
||||
blame, err := c.resolveLineAtHead(gitRepo, startLine+uint64(i), currentHead)
|
||||
if err != nil {
|
||||
return "", err
|
||||
}
|
||||
if blame.CommitID == currentHead && blame.LineNumber != anchorResolvedLine+uint64(i) {
|
||||
return "invalid", nil
|
||||
}
|
||||
}
|
||||
|
||||
return "valid", nil
|
||||
})
|
||||
if err != nil {
|
||||
return false, err
|
||||
}
|
||||
|
||||
return resultJSON == "valid", nil
|
||||
}
|
||||
|
||||
// CodeCommentLink returns the url to a comment in code
|
||||
func (c *Comment) CodeCommentLink(ctx context.Context) string {
|
||||
err := c.LoadIssue(ctx)
|
||||
@@ -914,6 +999,7 @@ func CreateComment(ctx context.Context, opts *CreateCommentOptions) (_ *Comment,
|
||||
CommitID: opts.CommitID,
|
||||
CommitSHA: opts.CommitSHA,
|
||||
Line: opts.LineNum,
|
||||
ExtraLinesCount: opts.ExtraLinesCount,
|
||||
Content: opts.Content,
|
||||
OldTitle: opts.OldTitle,
|
||||
NewTitle: opts.NewTitle,
|
||||
@@ -1090,6 +1176,7 @@ type CreateCommentOptions struct {
|
||||
CommitSHA string
|
||||
Patch string
|
||||
LineNum int64
|
||||
ExtraLinesCount int64
|
||||
TreePath string
|
||||
ReviewID int64
|
||||
Content string
|
||||
|
||||
@@ -46,6 +46,19 @@ func newCodeConversationsAtLineAndTreePath(ctx context.Context, comments []*Comm
|
||||
// when the user views a different commit in the PR, and it will always appear on the "Conversations" tab.
|
||||
continue
|
||||
}
|
||||
|
||||
// For multi-line comments, verify that the full line range is still valid and contiguous at head.
|
||||
// If lines were inserted/removed/reordered within the range, the comment would be displayed
|
||||
// at wrong lines — skip it in this view (it remains visible on the "Conversations" tab).
|
||||
if comment.ExtraLinesCount > 0 && blame != nil {
|
||||
valid, err := comment.CheckLineRangeValid(ctx, repo, headCommitID)
|
||||
if err != nil {
|
||||
log.Warn("CheckLineRangeValid failed for comment %d: %s", comment.ID, err.Error())
|
||||
} else if !valid {
|
||||
continue
|
||||
}
|
||||
}
|
||||
|
||||
tree.insertComment(comment, blame)
|
||||
}
|
||||
return tree, nil
|
||||
@@ -53,12 +66,17 @@ func newCodeConversationsAtLineAndTreePath(ctx context.Context, comments []*Comm
|
||||
|
||||
func (tree CodeConversationsAtLineAndTreePath) insertComment(comment *Comment, blame *git.ReverseLineBlame) {
|
||||
treePath := comment.TreePath
|
||||
line := comment.Line
|
||||
line := comment.DisplayLine()
|
||||
if blame != nil {
|
||||
treePath = blame.FilePath
|
||||
line = int64(blame.LineNumber)
|
||||
if comment.Line < 0 {
|
||||
line *= -1
|
||||
// On the previous side, ResolveCurrentLine resolves the last line of the range (the display
|
||||
// line) directly, so blame.LineNumber already is the signed display line.
|
||||
line = int64(blame.LineNumber) * -1
|
||||
} else {
|
||||
// On the proposed side, blame resolves the first line; the display line is that line shifted
|
||||
// down by the number of extra lines.
|
||||
line = int64(blame.LineNumber) + comment.ExtraLinesCount
|
||||
}
|
||||
}
|
||||
|
||||
@@ -117,7 +135,8 @@ func fetchCodeCommentsByReview(ctx context.Context, issue *Issue, doer *user_mod
|
||||
if pathToLineToComment[comment.TreePath] == nil {
|
||||
pathToLineToComment[comment.TreePath] = make(map[int64][]*Comment)
|
||||
}
|
||||
pathToLineToComment[comment.TreePath][comment.Line] = append(pathToLineToComment[comment.TreePath][comment.Line], comment)
|
||||
displayLine := comment.DisplayLine()
|
||||
pathToLineToComment[comment.TreePath][displayLine] = append(pathToLineToComment[comment.TreePath][displayLine], comment)
|
||||
}
|
||||
return pathToLineToComment, nil
|
||||
}
|
||||
|
||||
@@ -151,3 +151,59 @@ func Test_UpdateIssueNumComments(t *testing.T) {
|
||||
issue2 = unittest.AssertExistsAndLoadBean(t, &issues_model.Issue{ID: 2})
|
||||
assert.Equal(t, 1, issue2.NumComments)
|
||||
}
|
||||
|
||||
func TestDisplayLine(t *testing.T) {
|
||||
tests := []struct {
|
||||
name string
|
||||
line int64
|
||||
count int64
|
||||
expected int64
|
||||
}{
|
||||
{"positive single line", 10, 0, 10},
|
||||
{"positive multi-line", 7, 2, 9},
|
||||
{"positive large range", 1, 49, 50},
|
||||
{"negative single line", -10, 0, -10},
|
||||
{"negative multi-line", -7, 2, -9},
|
||||
{"negative large range", -1, 49, -50},
|
||||
}
|
||||
for _, tc := range tests {
|
||||
t.Run(tc.name, func(t *testing.T) {
|
||||
c := &issues_model.Comment{Line: tc.line, ExtraLinesCount: tc.count}
|
||||
assert.Equal(t, tc.expected, c.DisplayLine())
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
func TestUnsignedDisplayLine(t *testing.T) {
|
||||
tests := []struct {
|
||||
name string
|
||||
line int64
|
||||
count int64
|
||||
expected uint64
|
||||
}{
|
||||
{"positive single line", 10, 0, 10},
|
||||
{"positive multi-line", 7, 2, 9},
|
||||
{"negative single line", -10, 0, 10},
|
||||
{"negative multi-line", -7, 2, 9},
|
||||
}
|
||||
for _, tc := range tests {
|
||||
t.Run(tc.name, func(t *testing.T) {
|
||||
c := &issues_model.Comment{Line: tc.line, ExtraLinesCount: tc.count}
|
||||
assert.Equal(t, tc.expected, c.UnsignedDisplayLine())
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
func TestCheckLineRangeValid_SingleLine(t *testing.T) {
|
||||
// ExtraLinesCount=0 should return true immediately without any git operations
|
||||
c := &issues_model.Comment{Line: 10, ExtraLinesCount: 0}
|
||||
valid, err := c.CheckLineRangeValid(t.Context(), nil, "any-commit-id")
|
||||
require.NoError(t, err)
|
||||
assert.True(t, valid)
|
||||
|
||||
// Negative line, ExtraLinesCount=0
|
||||
c2 := &issues_model.Comment{Line: -5, ExtraLinesCount: 0}
|
||||
valid2, err := c2.CheckLineRangeValid(t.Context(), nil, "any-commit-id")
|
||||
require.NoError(t, err)
|
||||
assert.True(t, valid2)
|
||||
}
|
||||
|
||||
+8
-2
@@ -319,6 +319,14 @@ func FindAdjustedLineNumber(cutDiff string, originalLine int64, fullDiff io.Read
|
||||
rightLine = beginRight
|
||||
inHunk = true
|
||||
} else if inHunk {
|
||||
// Added ('+') lines exist only on the right side of the diff and have no left-side line
|
||||
// number. Skip them before testing the target, otherwise a target line that immediately
|
||||
// follows a removed line would be matched against the inserted line's text and wrongly
|
||||
// reported as changed.
|
||||
if len(lineText) > 0 && lineText[0] == '+' {
|
||||
rightLine++
|
||||
continue
|
||||
}
|
||||
if leftLine == originalLine {
|
||||
if lineText != endOfCutDiff {
|
||||
return LinePlacement{}, fmt.Errorf(
|
||||
@@ -328,8 +336,6 @@ func FindAdjustedLineNumber(cutDiff string, originalLine int64, fullDiff io.Read
|
||||
return LinePlacement{Left: leftLine, Right: rightLine}, nil
|
||||
}
|
||||
switch lineText[0] {
|
||||
case '+':
|
||||
rightLine++
|
||||
case '-':
|
||||
leftLine++
|
||||
case '\\':
|
||||
|
||||
@@ -154,6 +154,34 @@ func TestCutDiffAroundLine(t *testing.T) {
|
||||
assert.Equal(t, expected, minusDiff)
|
||||
}
|
||||
|
||||
func TestCutDiffAroundLineMultiRange(t *testing.T) {
|
||||
// Simulate multi-line comment: lines 2-4 on new side (
|
||||
// displayLine=4 -> last line of the comment
|
||||
// context=3+2=5 -> 3 lines of context + 2 lines of the comment that are above the display line
|
||||
// )
|
||||
// This mirrors how services/pull/review.go computes the patch for multi-line comments:
|
||||
// displayLine = UnsignedDisplayLine() (last line of range)
|
||||
// contextLines = setting.UI.CodeCommentLines + extraLinesCount
|
||||
result, err := CutDiffAroundLine(strings.NewReader(breakingDiff), 4, false, 5)
|
||||
require.NoError(t, err)
|
||||
|
||||
// Should include lines 1-4 of the new side (--some comment, --some comment 2, -- some comment 3, create or replace...)
|
||||
assert.Contains(t, result, "--some comment 2")
|
||||
assert.Contains(t, result, "-- some comment 3")
|
||||
assert.Contains(t, result, "create or replace procedure")
|
||||
|
||||
// Simulate single line (extraLinesCount=0): displayLine=2, context=3
|
||||
singleResult, err := CutDiffAroundLine(strings.NewReader(breakingDiff), 2, false, 3)
|
||||
require.NoError(t, err)
|
||||
assert.Contains(t, singleResult, "--some comment 2")
|
||||
|
||||
// Multi-line on exampleDiff: lines 3-5 new side (displayLine=5, context=3+2=5)
|
||||
multiResult, err := CutDiffAroundLine(strings.NewReader(exampleDiff), 5, false, 5)
|
||||
require.NoError(t, err)
|
||||
assert.Contains(t, multiResult, "Build Status")
|
||||
assert.Contains(t, multiResult, "Docker Pulls")
|
||||
}
|
||||
|
||||
func BenchmarkCutDiffAroundLine(b *testing.B) {
|
||||
for n := 0; n < b.N; n++ {
|
||||
CutDiffAroundLine(strings.NewReader(exampleDiff), 3, true, 3)
|
||||
@@ -291,6 +319,35 @@ index 2d203fb..d0cb63f 100644
|
||||
assert.Equal(t, LinePlacement{Left: 50, Right: 52}, lineNumber)
|
||||
})
|
||||
|
||||
t.Run("target line is an unchanged line right after a modification", func(t *testing.T) {
|
||||
// The commented line (49) is a context line immediately following a modified line (48).
|
||||
// The added ('+') line for the modification must be skipped while locating the old-side line,
|
||||
// otherwise it would be matched against line 49 and wrongly reported as changed.
|
||||
cutDiff := `diff --git a/file1.md b/file1.md
|
||||
--- a/file1.md
|
||||
+++ b/file1.md
|
||||
@@ -46,4 +46,4 @@ Line 45
|
||||
Line 46
|
||||
Line 47
|
||||
-Line 48
|
||||
+Line 48--modified
|
||||
Line 49`
|
||||
diff := `diff --git a/file1.md b/file1.md
|
||||
index 2d203fb..b21df3f 100644
|
||||
--- a/file1.md
|
||||
+++ b/file1.md
|
||||
@@ -46,4 +46,4 @@ Line 45
|
||||
Line 46
|
||||
Line 47
|
||||
-Line 48
|
||||
+Line 48--modified
|
||||
Line 49
|
||||
Line 50`
|
||||
lineNumber, err := FindAdjustedLineNumber(cutDiff, 49, strings.NewReader(diff))
|
||||
require.NoError(t, err)
|
||||
assert.Equal(t, LinePlacement{Left: 49, Right: 49}, lineNumber)
|
||||
})
|
||||
|
||||
t.Run("changes above in the same hunk", func(t *testing.T) {
|
||||
diff := `diff --git a/file1.md b/file1.md
|
||||
index 2d203fb..f35a466 100644
|
||||
|
||||
@@ -21,6 +21,7 @@ var UI = struct {
|
||||
PackagesPagingNum int
|
||||
GraphMaxCommitNum int
|
||||
CodeCommentLines int
|
||||
MaxCodeCommentLines int
|
||||
ReactionMaxUserNum int
|
||||
MaxDisplayFileSize int64
|
||||
ShowUserEmail bool
|
||||
@@ -81,6 +82,7 @@ var UI = struct {
|
||||
PackagesPagingNum: 20,
|
||||
GraphMaxCommitNum: 100,
|
||||
CodeCommentLines: 4,
|
||||
MaxCodeCommentLines: 50,
|
||||
ReactionMaxUserNum: 10,
|
||||
MaxDisplayFileSize: 8388608,
|
||||
DefaultTheme: `forgejo-auto`,
|
||||
|
||||
@@ -65,6 +65,8 @@ type PullReviewComment struct {
|
||||
DiffHunk string `json:"diff_hunk"`
|
||||
LineNum uint64 `json:"position"`
|
||||
OldLineNum uint64 `json:"original_position"`
|
||||
// number of additional lines after the commented line (0 = single line comment)
|
||||
ExtraLinesCount int64 `json:"extra_lines_count"`
|
||||
|
||||
HTMLURL string `json:"html_url"`
|
||||
HTMLPullURL string `json:"pull_request_url"`
|
||||
@@ -87,6 +89,8 @@ type CreatePullReviewComment struct {
|
||||
OldLineNum int64 `json:"old_position"`
|
||||
// if comment to new file line or 0
|
||||
NewLineNum int64 `json:"new_position"`
|
||||
// number of additional lines after the commented line (0 = single line comment)
|
||||
ExtraLinesCount int64 `json:"extra_lines_count"`
|
||||
}
|
||||
|
||||
type CreatePullReviewCommentOptions CreatePullReviewComment
|
||||
|
||||
@@ -331,6 +331,11 @@ func CreatePullReviewComment(ctx *context.APIContext) {
|
||||
line = opts.OldLineNum * -1
|
||||
}
|
||||
|
||||
if err := pull_service.ValidateCodeCommentLineRange(opts.ExtraLinesCount); err != nil {
|
||||
ctx.Error(http.StatusUnprocessableEntity, "invalid extra_lines_count", err)
|
||||
return
|
||||
}
|
||||
|
||||
comment, err := pull_service.CreateCodeCommentKnownReviewID(ctx,
|
||||
ctx.Doer,
|
||||
pr.Issue.Repo,
|
||||
@@ -340,6 +345,7 @@ func CreatePullReviewComment(ctx *context.APIContext) {
|
||||
pr.MergeBase,
|
||||
review.CommitID,
|
||||
line,
|
||||
opts.ExtraLinesCount,
|
||||
review.ID,
|
||||
nil,
|
||||
)
|
||||
@@ -496,6 +502,11 @@ func CreatePullReview(ctx *context.APIContext) {
|
||||
|
||||
// create review comments
|
||||
for _, c := range opts.Comments {
|
||||
if err := pull_service.ValidateCodeCommentLineRange(c.ExtraLinesCount); err != nil {
|
||||
ctx.Error(http.StatusUnprocessableEntity, "invalid extra_lines_count", err)
|
||||
return
|
||||
}
|
||||
|
||||
line := c.NewLineNum
|
||||
if c.OldLineNum > 0 {
|
||||
line = c.OldLineNum * -1
|
||||
@@ -506,6 +517,7 @@ func CreatePullReview(ctx *context.APIContext) {
|
||||
ctx.Repo.GitRepo,
|
||||
pr.Issue,
|
||||
line,
|
||||
c.ExtraLinesCount,
|
||||
c.Body,
|
||||
c.Path,
|
||||
true, // pending review
|
||||
|
||||
@@ -89,6 +89,11 @@ func CreateCodeComment(ctx *context.Context) {
|
||||
signedLine *= -1
|
||||
}
|
||||
|
||||
if err := pull_service.ValidateCodeCommentLineRange(form.ExtraLinesCount); err != nil {
|
||||
ctx.Error(http.StatusBadRequest, err.Error())
|
||||
return
|
||||
}
|
||||
|
||||
var attachments []string
|
||||
if setting.Attachment.Enabled {
|
||||
attachments = form.Files
|
||||
@@ -117,6 +122,7 @@ func CreateCodeComment(ctx *context.Context) {
|
||||
ctx.Repo.GitRepo,
|
||||
issue,
|
||||
signedLine,
|
||||
form.ExtraLinesCount,
|
||||
form.Content,
|
||||
form.TreePath,
|
||||
pendingReview,
|
||||
|
||||
@@ -6,15 +6,20 @@ package repo
|
||||
import (
|
||||
"net/http"
|
||||
"net/http/httptest"
|
||||
"strconv"
|
||||
"testing"
|
||||
|
||||
"forgejo.org/models/db"
|
||||
issues_model "forgejo.org/models/issues"
|
||||
"forgejo.org/models/unittest"
|
||||
"forgejo.org/modules/git"
|
||||
"forgejo.org/modules/setting"
|
||||
"forgejo.org/modules/templates"
|
||||
"forgejo.org/modules/test"
|
||||
"forgejo.org/modules/web"
|
||||
"forgejo.org/services/context"
|
||||
"forgejo.org/services/contexttest"
|
||||
"forgejo.org/services/forms"
|
||||
"forgejo.org/services/pull"
|
||||
|
||||
"github.com/stretchr/testify/assert"
|
||||
@@ -51,7 +56,7 @@ func TestRenderConversation(t *testing.T) {
|
||||
var preparedComment *issues_model.Comment
|
||||
run("prepare", func(t *testing.T, ctx *context.Context, resp *httptest.ResponseRecorder) {
|
||||
comment, err := pull.CreateCodeComment(ctx, pr.Issue.Poster, ctx.Repo.GitRepo, pr.Issue,
|
||||
1, "content", "", false, 0, pr.MergeBase,
|
||||
1, 0, "content", "", false, 0, pr.MergeBase,
|
||||
prHeadCommitID, nil)
|
||||
require.NoError(t, err)
|
||||
|
||||
@@ -111,4 +116,92 @@ func TestRenderConversation(t *testing.T) {
|
||||
assert.Equal(t, http.StatusOK, resp.Code)
|
||||
assert.NotContains(t, resp.Body.String(), `status-page-500`)
|
||||
})
|
||||
|
||||
// Test multi-line comment rendering
|
||||
var multiLineComment *issues_model.Comment
|
||||
run("prepare multi-line comment", func(t *testing.T, ctx *context.Context, resp *httptest.ResponseRecorder) {
|
||||
comment, err := pull.CreateCodeComment(ctx, pr.Issue.Poster, ctx.Repo.GitRepo, pr.Issue,
|
||||
1, 2, "multi-line content", "", false, 0, pr.MergeBase,
|
||||
prHeadCommitID, nil)
|
||||
require.NoError(t, err)
|
||||
assert.EqualValues(t, 2, comment.ExtraLinesCount)
|
||||
multiLineComment = comment
|
||||
})
|
||||
if !assert.NotNil(t, multiLineComment) {
|
||||
return
|
||||
}
|
||||
run("timeline multi-line comment renders", func(t *testing.T, ctx *context.Context, resp *httptest.ResponseRecorder) {
|
||||
renderConversation(ctx, multiLineComment, "timeline")
|
||||
body := resp.Body.String()
|
||||
assert.Contains(t, body, `<div id="code-comments-`)
|
||||
assert.Contains(t, body, "multi-line content")
|
||||
// Verify the "Lines X-Y" label is rendered in the conversation header
|
||||
assert.Contains(t, body, "Lines ")
|
||||
})
|
||||
run("diff multi-line comment renders", func(t *testing.T, ctx *context.Context, resp *httptest.ResponseRecorder) {
|
||||
ctx.Data["ShowOutdatedComments"] = true
|
||||
renderConversation(ctx, multiLineComment, "diff")
|
||||
body := resp.Body.String()
|
||||
assert.Contains(t, body, `<div class="content comment-container"`)
|
||||
// Verify the conversation-holder has data-extra-lines-count attribute
|
||||
assert.Contains(t, body, `data-extra-lines-count="2"`)
|
||||
})
|
||||
}
|
||||
|
||||
// TestCreateCodeCommentRejectsNegativeExtraLinesCount checks that the CreateCodeComment handler
|
||||
// rejects a negative extra_lines_count with a 400 response, before reaching any comment creation.
|
||||
func TestCreateCodeCommentRejectsNegativeExtraLinesCount(t *testing.T) {
|
||||
unittest.PrepareTestEnv(t)
|
||||
|
||||
pr, err := issues_model.GetPullRequestByID(db.DefaultContext, 2)
|
||||
require.NoError(t, err)
|
||||
require.NoError(t, pr.LoadIssue(db.DefaultContext))
|
||||
|
||||
ctx, resp := contexttest.MockContext(t, "/")
|
||||
contexttest.LoadUser(t, ctx, pr.Issue.PosterID)
|
||||
contexttest.LoadRepo(t, ctx, pr.BaseRepoID)
|
||||
ctx.SetParams(":index", strconv.FormatInt(pr.Issue.Index, 10))
|
||||
|
||||
web.SetForm(ctx, &forms.CodeCommentForm{
|
||||
Origin: "diff",
|
||||
Content: "a comment",
|
||||
Side: "proposed",
|
||||
Line: 1,
|
||||
TreePath: "README.md",
|
||||
ExtraLinesCount: -1,
|
||||
})
|
||||
|
||||
CreateCodeComment(ctx)
|
||||
|
||||
assert.Equal(t, http.StatusBadRequest, resp.Code)
|
||||
}
|
||||
|
||||
// TestCreateCodeCommentRejectsExceedingMaxLines checks that the CreateCodeComment handler rejects a
|
||||
// multi-line comment spanning more than setting.UI.MaxCodeCommentLines lines with a 400 response.
|
||||
func TestCreateCodeCommentRejectsExceedingMaxLines(t *testing.T) {
|
||||
unittest.PrepareTestEnv(t)
|
||||
|
||||
defer test.MockVariableValue(&setting.UI.MaxCodeCommentLines, 50)()
|
||||
|
||||
pr, err := issues_model.GetPullRequestByID(db.DefaultContext, 2)
|
||||
require.NoError(t, err)
|
||||
require.NoError(t, pr.LoadIssue(db.DefaultContext))
|
||||
|
||||
ctx, resp := contexttest.MockContext(t, "/")
|
||||
contexttest.LoadUser(t, ctx, pr.Issue.PosterID)
|
||||
contexttest.LoadRepo(t, ctx, pr.BaseRepoID)
|
||||
ctx.SetParams(":index", strconv.FormatInt(pr.Issue.Index, 10))
|
||||
|
||||
web.SetForm(ctx, &forms.CodeCommentForm{
|
||||
Origin: "diff",
|
||||
Content: "a comment",
|
||||
Side: "proposed",
|
||||
Line: 1,
|
||||
TreePath: "README.md",
|
||||
ExtraLinesCount: 50, // range spans 51 lines > 50
|
||||
})
|
||||
|
||||
CreateCodeComment(ctx)
|
||||
|
||||
assert.Equal(t, http.StatusBadRequest, resp.Code)
|
||||
}
|
||||
|
||||
@@ -81,19 +81,20 @@ func ToPullReviewList(ctx context.Context, rl []*issues_model.Review, doer *user
|
||||
// ToPullReviewCommentList convert the CodeComments of an review to it's api format
|
||||
func ToPullReviewComment(ctx context.Context, review *issues_model.Review, comment *issues_model.Comment, doer *user_model.User) (*api.PullReviewComment, error) {
|
||||
apiComment := &api.PullReviewComment{
|
||||
ID: comment.ID,
|
||||
Body: comment.Content,
|
||||
Poster: ToUser(ctx, comment.Poster, doer),
|
||||
Resolver: ToUser(ctx, comment.ResolveDoer, doer),
|
||||
ReviewID: review.ID,
|
||||
Created: comment.CreatedUnix.AsTime(),
|
||||
Updated: comment.UpdatedUnix.AsTime(),
|
||||
Path: comment.TreePath,
|
||||
CommitID: comment.CommitSHA,
|
||||
OrigCommitID: comment.OldRef,
|
||||
DiffHunk: patch2diff(comment.Patch),
|
||||
HTMLURL: comment.HTMLURL(ctx),
|
||||
HTMLPullURL: review.Issue.HTMLURL(),
|
||||
ID: comment.ID,
|
||||
Body: comment.Content,
|
||||
Poster: ToUser(ctx, comment.Poster, doer),
|
||||
Resolver: ToUser(ctx, comment.ResolveDoer, doer),
|
||||
ReviewID: review.ID,
|
||||
Created: comment.CreatedUnix.AsTime(),
|
||||
Updated: comment.UpdatedUnix.AsTime(),
|
||||
Path: comment.TreePath,
|
||||
CommitID: comment.CommitSHA,
|
||||
OrigCommitID: comment.OldRef,
|
||||
DiffHunk: patch2diff(comment.Patch),
|
||||
ExtraLinesCount: comment.ExtraLinesCount,
|
||||
HTMLURL: comment.HTMLURL(ctx),
|
||||
HTMLPullURL: review.Issue.HTMLURL(),
|
||||
}
|
||||
|
||||
if comment.Line < 0 {
|
||||
|
||||
+11
-10
@@ -465,16 +465,17 @@ func (f *MergePullRequestForm) Validate(req *http.Request, errs binding.Errors)
|
||||
|
||||
// CodeCommentForm form for adding code comments for PRs
|
||||
type CodeCommentForm struct {
|
||||
Origin string `binding:"Required;In(timeline,diff)"`
|
||||
Content string `binding:"Required"`
|
||||
Side string `binding:"Required;In(previous,proposed)"`
|
||||
Line int64
|
||||
TreePath string `form:"path" binding:"Required"`
|
||||
SingleReview bool `form:"single_review"`
|
||||
Reply int64 `form:"reply"`
|
||||
BeforeCommitID string
|
||||
LatestCommitID string
|
||||
Files []string
|
||||
Origin string `binding:"Required;In(timeline,diff)"`
|
||||
Content string `binding:"Required"`
|
||||
Side string `binding:"Required;In(previous,proposed)"`
|
||||
Line int64
|
||||
ExtraLinesCount int64 `form:"extra_lines_count"`
|
||||
TreePath string `form:"path" binding:"Required"`
|
||||
SingleReview bool `form:"single_review"`
|
||||
Reply int64 `form:"reply"`
|
||||
BeforeCommitID string
|
||||
LatestCommitID string
|
||||
Files []string
|
||||
}
|
||||
|
||||
// Validate validates the fields
|
||||
|
||||
@@ -148,6 +148,7 @@ func (h *ReplyHandler) Handle(ctx context.Context, content *MailContent, doer *u
|
||||
nil,
|
||||
issue,
|
||||
comment.Line,
|
||||
0, // extraLinesCount: a reply to a comment inherit single line
|
||||
content.Content,
|
||||
comment.TreePath,
|
||||
false, // not pending review but a single review
|
||||
|
||||
+83
-23
@@ -46,13 +46,38 @@ func checkInvalidation(ctx context.Context, c *issues_model.Comment, repo *repo_
|
||||
reverseBlame, err := c.ResolveCurrentLine(ctx, repo, newCommitID)
|
||||
if err != nil {
|
||||
log.Warn("ResolveCurrentLine failed: %s", err.Error())
|
||||
} else if reverseBlame.CommitID != newCommitID {
|
||||
return nil
|
||||
}
|
||||
if reverseBlame.CommitID != newCommitID {
|
||||
c.Invalidated = true
|
||||
return issues_model.UpdateCommentInvalidate(ctx, c)
|
||||
}
|
||||
|
||||
// For multi-line comments, check additional lines in the range
|
||||
if c.ExtraLinesCount > 0 {
|
||||
invalidated, err := checkMultiLineInvalidation(ctx, c, repo, newCommitID)
|
||||
if err != nil {
|
||||
log.Warn("checkMultiLineInvalidation failed: %s", err.Error())
|
||||
} else if invalidated {
|
||||
c.Invalidated = true
|
||||
return issues_model.UpdateCommentInvalidate(ctx, c)
|
||||
}
|
||||
}
|
||||
|
||||
return nil
|
||||
}
|
||||
|
||||
// checkMultiLineInvalidation checks if any additional line in a multi-line comment range
|
||||
// has been changed. Returns true if the comment should be invalidated.
|
||||
// Uses cached results via Comment.CheckLineRangeValid.
|
||||
func checkMultiLineInvalidation(ctx context.Context, c *issues_model.Comment, repo *repo_model.Repository, newCommitID string) (bool, error) {
|
||||
valid, err := c.CheckLineRangeValid(ctx, repo, newCommitID)
|
||||
if err != nil {
|
||||
return false, err
|
||||
}
|
||||
return !valid, nil
|
||||
}
|
||||
|
||||
// InvalidateCodeComments will lookup the prs for code comments which got invalidated by change
|
||||
func InvalidateCodeComments(ctx context.Context, prs issues_model.PullRequestList, doer *user_model.User, repo *repo_model.Repository, newCommitID string) error {
|
||||
if len(prs) == 0 {
|
||||
@@ -77,9 +102,22 @@ func InvalidateCodeComments(ctx context.Context, prs issues_model.PullRequestLis
|
||||
return nil
|
||||
}
|
||||
|
||||
// ValidateCodeCommentLineRange validates the extra_lines_count of a (multi-line) code comment: it must
|
||||
// not be negative and the resulting range must not span more than setting.UI.MaxCodeCommentLines lines
|
||||
// (0 = no limit). It is the single source of truth shared by the web and API creation handlers.
|
||||
func ValidateCodeCommentLineRange(extraLinesCount int64) error {
|
||||
if extraLinesCount < 0 {
|
||||
return fmt.Errorf("extra_lines_count must be >= 0")
|
||||
}
|
||||
if setting.UI.MaxCodeCommentLines > 0 && extraLinesCount+1 > int64(setting.UI.MaxCodeCommentLines) {
|
||||
return fmt.Errorf("a code comment may span at most %d lines", setting.UI.MaxCodeCommentLines)
|
||||
}
|
||||
return nil
|
||||
}
|
||||
|
||||
// CreateCodeComment creates a comment on the code line
|
||||
func CreateCodeComment(ctx context.Context, doer *user_model.User, gitRepo *git.Repository,
|
||||
issue *issues_model.Issue, line int64, content, treePath string, pendingReview bool,
|
||||
issue *issues_model.Issue, line, extraLinesCount int64, content, treePath string, pendingReview bool,
|
||||
replyReviewID int64, beforeCommitID, latestCommitID string, attachments []string,
|
||||
) (*issues_model.Comment, error) {
|
||||
var (
|
||||
@@ -115,6 +153,7 @@ func CreateCodeComment(ctx context.Context, doer *user_model.User, gitRepo *git.
|
||||
beforeCommitID,
|
||||
latestCommitID,
|
||||
line,
|
||||
extraLinesCount,
|
||||
replyReviewID,
|
||||
attachments,
|
||||
)
|
||||
@@ -158,6 +197,7 @@ func CreateCodeComment(ctx context.Context, doer *user_model.User, gitRepo *git.
|
||||
beforeCommitID,
|
||||
latestCommitID,
|
||||
line,
|
||||
extraLinesCount,
|
||||
review.ID,
|
||||
attachments,
|
||||
)
|
||||
@@ -180,7 +220,7 @@ func CreateCodeComment(ctx context.Context, doer *user_model.User, gitRepo *git.
|
||||
// CreateCodeCommentKnownReviewID creates a plain code comment at the specified line / path
|
||||
func CreateCodeCommentKnownReviewID(ctx context.Context, doer *user_model.User, repo *repo_model.Repository,
|
||||
issue *issues_model.Issue, content, treePath, beforeCommitID, afterCommitID string,
|
||||
line, reviewID int64, attachments []string,
|
||||
line, extraLinesCount, reviewID int64, attachments []string,
|
||||
) (*issues_model.Comment, error) {
|
||||
var commitID, blamedCommitID, patch string
|
||||
blamedLine := line
|
||||
@@ -248,12 +288,23 @@ func CreateCodeCommentKnownReviewID(ctx context.Context, doer *user_model.User,
|
||||
blame, err := gitRepo.ReverseLineBlame(beforeCommitID, treePath, uint64(-1*line), afterCommitID)
|
||||
if err != nil {
|
||||
return nil, fmt.Errorf("ReverseLineBlame[%s, %s, %d, %s]: %w", beforeCommitID, treePath, -1*line, afterCommitID, err)
|
||||
} else if blame.CommitID == afterCommitID {
|
||||
// Although this is a comment on the "previous" side of the diff, the reverse blame indicates that the line
|
||||
// of code still exists in the commit being viewed (eg. it was a comment on a white line in the left-side of
|
||||
// the diff, not a red removed line). In order to record the right information for where to place this
|
||||
// commit, we'll convert this into a right-hand comment -- using the present line number that the reverse
|
||||
// blame gave us:
|
||||
}
|
||||
|
||||
// Convert to a right-hand (proposed) comment only when EVERY line of the range still exists at head
|
||||
// (whole selection unchanged); if any line was removed or modified, keep it on the previous side.
|
||||
rangeStillExists := blame.CommitID == afterCommitID
|
||||
for i := int64(1); rangeStillExists && i <= extraLinesCount; i++ {
|
||||
lineBlame, err := gitRepo.ReverseLineBlame(beforeCommitID, treePath, uint64(-1*line)+uint64(i), afterCommitID)
|
||||
if err != nil {
|
||||
return nil, fmt.Errorf("ReverseLineBlame[%s, %s, %d, %s]: %w", beforeCommitID, treePath, -1*line+i, afterCommitID, err)
|
||||
}
|
||||
if lineBlame.CommitID != afterCommitID {
|
||||
rangeStillExists = false
|
||||
}
|
||||
}
|
||||
|
||||
switch {
|
||||
case rangeStillExists:
|
||||
commit, lineres, err := gitRepo.LineBlame(afterCommitID, treePath, blame.LineNumber)
|
||||
if err == nil {
|
||||
blamedCommitID = commit.ID.String()
|
||||
@@ -261,7 +312,12 @@ func CreateCodeCommentKnownReviewID(ctx context.Context, doer *user_model.User,
|
||||
} else if !errors.Is(err, git.ErrBlameFileDoesNotExist) && !errors.Is(err, git.ErrBlameFileNotEnoughLines) {
|
||||
return nil, fmt.Errorf("LineBlame[%s, %s, %s, %d]: %w", pr.GetGitRefName(), gitRepo.Path, treePath, line, err)
|
||||
}
|
||||
} else {
|
||||
case blame.CommitID == afterCommitID:
|
||||
// First line still exists but a later one changed
|
||||
blamedCommitID = beforeCommitID
|
||||
// retain negative line numbering to identify we're commenting on the "previous" side of the diff
|
||||
blamedLine = line
|
||||
default:
|
||||
blamedCommitID = blame.CommitID
|
||||
// retain negative line numbering to identify we're commenting on the "previous" side of the diff
|
||||
blamedLine = -1 * int64(blame.LineNumber)
|
||||
@@ -292,25 +348,29 @@ func CreateCodeCommentKnownReviewID(ctx context.Context, doer *user_model.User,
|
||||
_ = writer.Close()
|
||||
}()
|
||||
|
||||
patch, err = git.CutDiffAroundLine(reader, int64((&issues_model.Comment{Line: line}).UnsignedLine()), line < 0, setting.UI.CodeCommentLines)
|
||||
// For multi-line comments, center the patch on the last line and expand context to include the full range
|
||||
displayLine := int64((&issues_model.Comment{Line: line, ExtraLinesCount: extraLinesCount}).UnsignedDisplayLine())
|
||||
contextLines := setting.UI.CodeCommentLines + int(extraLinesCount)
|
||||
patch, err = git.CutDiffAroundLine(reader, displayLine, line < 0, contextLines)
|
||||
if err != nil {
|
||||
log.Error("Error whilst generating patch: %v", err)
|
||||
return nil, err
|
||||
}
|
||||
}
|
||||
return issues_model.CreateComment(ctx, &issues_model.CreateCommentOptions{
|
||||
Type: issues_model.CommentTypeCode,
|
||||
Doer: doer,
|
||||
Repo: repo,
|
||||
Issue: issue,
|
||||
Content: content,
|
||||
LineNum: blamedLine,
|
||||
TreePath: treePath,
|
||||
CommitSHA: blamedCommitID,
|
||||
ReviewID: reviewID,
|
||||
Patch: patch,
|
||||
Invalidated: invalidated,
|
||||
Attachments: attachments,
|
||||
Type: issues_model.CommentTypeCode,
|
||||
Doer: doer,
|
||||
Repo: repo,
|
||||
Issue: issue,
|
||||
Content: content,
|
||||
LineNum: blamedLine,
|
||||
ExtraLinesCount: extraLinesCount,
|
||||
TreePath: treePath,
|
||||
CommitSHA: blamedCommitID,
|
||||
ReviewID: reviewID,
|
||||
Patch: patch,
|
||||
Invalidated: invalidated,
|
||||
Attachments: attachments,
|
||||
})
|
||||
}
|
||||
|
||||
|
||||
@@ -5,6 +5,7 @@
|
||||
<input type="hidden" name="latest_commit_id" value="{{$.root.AfterCommitID}}">
|
||||
<input type="hidden" name="side" value="{{if $.Side}}{{$.Side}}{{end}}">
|
||||
<input type="hidden" name="line" value="{{if $.Line}}{{$.Line}}{{end}}">
|
||||
<input type="hidden" name="extra_lines_count" value="{{if $.ExtraLinesCount}}{{$.ExtraLinesCount}}{{else}}0{{end}}">
|
||||
<input type="hidden" name="path" value="{{if $.File}}{{$.File}}{{end}}">
|
||||
<input type="hidden" name="diff_start_cid">
|
||||
<input type="hidden" name="diff_end_cid">
|
||||
|
||||
@@ -31,6 +31,9 @@
|
||||
{{end}}
|
||||
</div>
|
||||
<div class="comment-header-right actions tw-flex tw-items-center">
|
||||
{{if gt .ExtraLinesCount 0}}
|
||||
<span class="ui label">Lines {{.UnsignedLine}}-{{.UnsignedDisplayLine}}</span>
|
||||
{{end}}
|
||||
{{if .Invalidated}}
|
||||
{{$referenceUrl := printf "%s#%s" $.root.Issue.Link .HashTag}}
|
||||
<a href="{{AppSubUrl}}{{$referenceUrl}}" class="ui label" data-tooltip-content="{{ctx.Locale.Tr "repo.issues.review.outdated_description"}}">
|
||||
|
||||
@@ -3,7 +3,7 @@
|
||||
{{$resolveDoer := (index .comments 0).ResolveDoer}}
|
||||
{{$isNotPending := (not (eq (index .comments 0).Review.Type 0))}}
|
||||
{{$referenceUrl := printf "%s#%s" $.Issue.Link (index .comments 0).HashTag}}
|
||||
<div class="conversation-holder" data-path="{{(index .comments 0).TreePath}}" data-side="{{if lt (index .comments 0).Line 0}}left{{else}}right{{end}}" data-idx="{{(index .comments 0).UnsignedLine}}">
|
||||
<div class="conversation-holder" data-path="{{(index .comments 0).TreePath}}" data-side="{{if lt (index .comments 0).Line 0}}left{{else}}right{{end}}" data-idx="{{(index .comments 0).UnsignedLine}}" data-extra-lines-count="{{(index .comments 0).ExtraLinesCount}}">
|
||||
{{if $resolved}}
|
||||
<div class="ui attached header resolved-placeholder tw-flex tw-items-center tw-justify-between">
|
||||
<div class="ui grey text tw-flex tw-items-center tw-flex-wrap tw-gap-1">
|
||||
|
||||
@@ -12,7 +12,10 @@
|
||||
</span>
|
||||
{{end}}
|
||||
</div>
|
||||
<div>
|
||||
<div class="tw-flex tw-items-center tw-gap-2">
|
||||
{{if gt (index .comments 0).ExtraLinesCount 0}}
|
||||
<span class="ui label">Lines {{(index .comments 0).UnsignedLine}}-{{(index .comments 0).UnsignedDisplayLine}}</span>
|
||||
{{end}}
|
||||
{{if or $invalid $resolved}}
|
||||
<button id="show-outdated-{{(index .comments 0).ID}}" data-comment="{{(index .comments 0).ID}}" class="{{if not $resolved}}tw-hidden {{end}}show-outdated">
|
||||
{{svg "octicon-unfold" 16 "tw-mr-2"}}
|
||||
|
||||
Generated
+12
@@ -25052,6 +25052,12 @@
|
||||
"type": "string",
|
||||
"x-go-name": "Body"
|
||||
},
|
||||
"extra_lines_count": {
|
||||
"description": "number of additional lines after the commented line (0 = single line comment)",
|
||||
"type": "integer",
|
||||
"format": "int64",
|
||||
"x-go-name": "ExtraLinesCount"
|
||||
},
|
||||
"new_position": {
|
||||
"description": "if comment to new file line or 0",
|
||||
"type": "integer",
|
||||
@@ -28954,6 +28960,12 @@
|
||||
"type": "string",
|
||||
"x-go-name": "DiffHunk"
|
||||
},
|
||||
"extra_lines_count": {
|
||||
"description": "number of additional lines after the commented line (0 = single line comment)",
|
||||
"type": "integer",
|
||||
"format": "int64",
|
||||
"x-go-name": "ExtraLinesCount"
|
||||
},
|
||||
"html_url": {
|
||||
"type": "string",
|
||||
"x-go-name": "HTMLURL"
|
||||
|
||||
@@ -16,6 +16,7 @@ import (
|
||||
"forgejo.org/models/unittest"
|
||||
user_model "forgejo.org/models/user"
|
||||
"forgejo.org/modules/json"
|
||||
"forgejo.org/modules/setting"
|
||||
api "forgejo.org/modules/structs"
|
||||
issue_service "forgejo.org/services/issue"
|
||||
"forgejo.org/tests"
|
||||
@@ -151,6 +152,109 @@ func TestAPIPullReviewCreateDeleteComment(t *testing.T) {
|
||||
}
|
||||
}
|
||||
|
||||
func TestAPIPullReviewMultiLineComment(t *testing.T) {
|
||||
defer tests.PrepareTestEnv(t)()
|
||||
pullIssue := unittest.AssertExistsAndLoadBean(t, &issues_model.Issue{ID: 3})
|
||||
require.NoError(t, pullIssue.LoadAttributes(db.DefaultContext))
|
||||
repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: pullIssue.RepoID})
|
||||
|
||||
session := loginUser(t, "user2")
|
||||
token := getTokenForLoggedInUser(t, session, auth_model.AccessTokenScopeWriteRepository)
|
||||
|
||||
// Create a review with a multi-line comment (extra_lines_count > 0)
|
||||
var review api.PullReview
|
||||
req := NewRequestWithJSON(t, http.MethodPost, fmt.Sprintf("/api/v1/repos/%s/%s/pulls/%d/reviews", repo.OwnerName, repo.Name, pullIssue.Index), &api.CreatePullReviewOptions{
|
||||
Body: "multi-line review",
|
||||
Event: "COMMENT",
|
||||
Comments: []api.CreatePullReviewComment{
|
||||
{
|
||||
Path: "README.md",
|
||||
Body: "multi-line comment on lines 1-3",
|
||||
NewLineNum: 1,
|
||||
ExtraLinesCount: 2,
|
||||
},
|
||||
},
|
||||
}).AddTokenAuth(token)
|
||||
resp := MakeRequest(t, req, http.StatusOK)
|
||||
DecodeJSON(t, resp, &review)
|
||||
assert.EqualValues(t, "COMMENT", review.State)
|
||||
assert.Equal(t, 1, review.CodeCommentsCount)
|
||||
|
||||
// Get the review comments and verify extra_lines_count is returned
|
||||
req = NewRequestf(t, http.MethodGet, "/api/v1/repos/%s/%s/pulls/%d/reviews/%d/comments", repo.OwnerName, repo.Name, pullIssue.Index, review.ID).
|
||||
AddTokenAuth(token)
|
||||
resp = MakeRequest(t, req, http.StatusOK)
|
||||
var reviewComments []*api.PullReviewComment
|
||||
DecodeJSON(t, resp, &reviewComments)
|
||||
require.Len(t, reviewComments, 1)
|
||||
assert.EqualValues(t, 2, reviewComments[0].ExtraLinesCount)
|
||||
assert.EqualValues(t, 1, reviewComments[0].LineNum)
|
||||
assert.Equal(t, "multi-line comment on lines 1-3", reviewComments[0].Body)
|
||||
|
||||
// Create a review with a single-line comment (extra_lines_count = 0, backward compat)
|
||||
var review2 api.PullReview
|
||||
req = NewRequestWithJSON(t, http.MethodPost, fmt.Sprintf("/api/v1/repos/%s/%s/pulls/%d/reviews", repo.OwnerName, repo.Name, pullIssue.Index), &api.CreatePullReviewOptions{
|
||||
Body: "single-line review",
|
||||
Event: "COMMENT",
|
||||
Comments: []api.CreatePullReviewComment{
|
||||
{
|
||||
Path: "README.md",
|
||||
Body: "single-line comment",
|
||||
NewLineNum: 1,
|
||||
},
|
||||
},
|
||||
}).AddTokenAuth(token)
|
||||
resp = MakeRequest(t, req, http.StatusOK)
|
||||
DecodeJSON(t, resp, &review2)
|
||||
|
||||
// Get the review comments and verify extra_lines_count is 0
|
||||
req = NewRequestf(t, http.MethodGet, "/api/v1/repos/%s/%s/pulls/%d/reviews/%d/comments", repo.OwnerName, repo.Name, pullIssue.Index, review2.ID).
|
||||
AddTokenAuth(token)
|
||||
resp = MakeRequest(t, req, http.StatusOK)
|
||||
var singleComments []*api.PullReviewComment
|
||||
DecodeJSON(t, resp, &singleComments)
|
||||
require.Len(t, singleComments, 1)
|
||||
assert.EqualValues(t, 0, singleComments[0].ExtraLinesCount)
|
||||
|
||||
// Create a review with negative extra_lines_count → should be rejected
|
||||
req = NewRequestWithJSON(t, http.MethodPost, fmt.Sprintf("/api/v1/repos/%s/%s/pulls/%d/reviews", repo.OwnerName, repo.Name, pullIssue.Index), &api.CreatePullReviewOptions{
|
||||
Body: "negative review",
|
||||
Event: "COMMENT",
|
||||
Comments: []api.CreatePullReviewComment{
|
||||
{
|
||||
Path: "README.md",
|
||||
Body: "negative extra_lines_count",
|
||||
NewLineNum: 1,
|
||||
ExtraLinesCount: -1,
|
||||
},
|
||||
},
|
||||
}).AddTokenAuth(token)
|
||||
MakeRequest(t, req, http.StatusUnprocessableEntity)
|
||||
|
||||
// Create a review with a range exceeding setting.UI.MaxCodeCommentLines → should be rejected
|
||||
req = NewRequestWithJSON(t, http.MethodPost, fmt.Sprintf("/api/v1/repos/%s/%s/pulls/%d/reviews", repo.OwnerName, repo.Name, pullIssue.Index), &api.CreatePullReviewOptions{
|
||||
Body: "too long review",
|
||||
Event: "COMMENT",
|
||||
Comments: []api.CreatePullReviewComment{
|
||||
{
|
||||
Path: "README.md",
|
||||
Body: "extra_lines_count over the limit",
|
||||
NewLineNum: 1,
|
||||
ExtraLinesCount: int64(setting.UI.MaxCodeCommentLines), // range spans MaxCodeCommentLines+1 lines
|
||||
},
|
||||
},
|
||||
}).AddTokenAuth(token)
|
||||
MakeRequest(t, req, http.StatusUnprocessableEntity)
|
||||
|
||||
// Cleanup
|
||||
req = NewRequestf(t, http.MethodDelete, "/api/v1/repos/%s/%s/pulls/%d/reviews/%d", repo.OwnerName, repo.Name, pullIssue.Index, review.ID).
|
||||
AddTokenAuth(token)
|
||||
MakeRequest(t, req, http.StatusNoContent)
|
||||
req = NewRequestf(t, http.MethodDelete, "/api/v1/repos/%s/%s/pulls/%d/reviews/%d", repo.OwnerName, repo.Name, pullIssue.Index, review2.ID).
|
||||
AddTokenAuth(token)
|
||||
MakeRequest(t, req, http.StatusNoContent)
|
||||
}
|
||||
|
||||
func TestAPIPullReview(t *testing.T) {
|
||||
defer tests.PrepareTestEnv(t)()
|
||||
pullIssue := unittest.AssertExistsAndLoadBean(t, &issues_model.Issue{ID: 3})
|
||||
|
||||
@@ -1783,6 +1783,261 @@ func TestPullRequestCommentPlacement(t *testing.T) {
|
||||
}
|
||||
tester.assertFilesChangedDiff(diff3, "checking overall contents in full PR diff")
|
||||
})
|
||||
|
||||
t.Run("multi-line comment stores ExtraLinesCount correctly", func(t *testing.T) {
|
||||
defer tests.PrintCurrentTest(t)()
|
||||
tester := newPullRequestCommentPlacementTester(t)
|
||||
|
||||
// Modify lines 48-50
|
||||
content := tester.fileContent
|
||||
content = strings.Replace(content, "Line 48\n", "Line 48--modified\n", 1)
|
||||
content = strings.Replace(content, "Line 49\n", "Line 49--modified\n", 1)
|
||||
content = strings.Replace(content, "Line 50\n", "Line 50--modified\n", 1)
|
||||
tester.changeFile("file1.md", content)
|
||||
tester.createPR()
|
||||
|
||||
// Create a multi-line comment on lines 48-50 (line=48, extraLinesCount=2)
|
||||
comment := tester.multiLineCommentFromFilesChanged("file1.md", 48, 2)
|
||||
assert.EqualValues(t, 48, comment.Line)
|
||||
assert.EqualValues(t, 2, comment.ExtraLinesCount)
|
||||
assert.EqualValues(t, 50, comment.UnsignedDisplayLine())
|
||||
assert.EqualValues(t, 50, comment.DisplayLine())
|
||||
assert.False(t, comment.Invalidated)
|
||||
})
|
||||
|
||||
t.Run("multi-line proposed comment over unmodified-then-modified lines stays visible", func(t *testing.T) {
|
||||
defer tests.PrintCurrentTest(t)()
|
||||
tester := newPullRequestCommentPlacementTester(t)
|
||||
|
||||
// Modify line 49 only; line 48 is unmodified. A proposed comment on 48-49 thus spans an
|
||||
// unmodified (context) line and a modified (added) line. The whole-range validation must NOT
|
||||
// hide it just because line 49 can't be reverse-blamed from line 48's (pre-PR) commit.
|
||||
content := strings.Replace(tester.fileContent, "Line 49\n", "Line 49--modified\n", 1)
|
||||
commit := tester.changeFile("file1.md", content)
|
||||
tester.createPR()
|
||||
|
||||
comment := tester.multiLineCommentFromFilesChanged("file1.md", 48, 1)
|
||||
assert.Equal(t, "proposed", comment.DiffSide())
|
||||
assert.EqualValues(t, 48, comment.Line)
|
||||
assert.EqualValues(t, 1, comment.ExtraLinesCount)
|
||||
|
||||
diff := []diffTableRow{
|
||||
{rowType: RowHasCode, code: "Line 48"},
|
||||
{rowType: RowDelCode, code: "Line 49"},
|
||||
{rowType: RowAddCode, code: "Line 49--modified"},
|
||||
{rowType: RowComment, commentID: comment.ID},
|
||||
{rowType: RowHasCode, code: "Line 50"},
|
||||
}
|
||||
tester.assertFilesChangedDiff(diff)
|
||||
tester.assertCommitDiff(commit, diff)
|
||||
})
|
||||
|
||||
t.Run("multi-line previous comment over only modified lines stays visible", func(t *testing.T) {
|
||||
defer tests.PrintCurrentTest(t)()
|
||||
tester := newPullRequestCommentPlacementTester(t)
|
||||
|
||||
// Modify lines 48-50; on the previous (left) side they appear as removed lines. A previous
|
||||
// comment on 48-50 must stay on the left and remain visible (gauche-3).
|
||||
content := tester.fileContent
|
||||
content = strings.Replace(content, "Line 48\n", "Line 48--modified\n", 1)
|
||||
content = strings.Replace(content, "Line 49\n", "Line 49--modified\n", 1)
|
||||
content = strings.Replace(content, "Line 50\n", "Line 50--modified\n", 1)
|
||||
commit := tester.changeFile("file1.md", content)
|
||||
tester.createPR()
|
||||
|
||||
comment := tester.multiLineCommentOnPreviousFromFilesChanged("file1.md", 48, 2)
|
||||
assert.Equal(t, "previous", comment.DiffSide())
|
||||
assert.EqualValues(t, -48, comment.Line)
|
||||
assert.EqualValues(t, 2, comment.ExtraLinesCount)
|
||||
|
||||
diff := []diffTableRow{
|
||||
{rowType: RowHasCode, code: "Line 47"},
|
||||
{rowType: RowDelCode, code: "Line 48"},
|
||||
{rowType: RowDelCode, code: "Line 49"},
|
||||
{rowType: RowDelCode, code: "Line 50"},
|
||||
{rowType: RowComment, commentID: comment.ID},
|
||||
{rowType: RowAddCode, code: "Line 48--modified"},
|
||||
{rowType: RowAddCode, code: "Line 49--modified"},
|
||||
{rowType: RowAddCode, code: "Line 50--modified"},
|
||||
{rowType: RowHasCode, code: "Line 51"},
|
||||
}
|
||||
tester.assertFilesChangedDiff(diff)
|
||||
tester.assertCommitDiff(commit, diff)
|
||||
})
|
||||
|
||||
t.Run("multi-line previous comment ending on an unmodified line stays visible", func(t *testing.T) {
|
||||
defer tests.PrintCurrentTest(t)()
|
||||
tester := newPullRequestCommentPlacementTester(t)
|
||||
|
||||
// Modify line 48; line 49 is unmodified. Previous comment on 48-49: first line removed (so it
|
||||
// stays on the left), last line unmodified — the display anchor is the unmodified line (gauche-4).
|
||||
content := strings.Replace(tester.fileContent, "Line 48\n", "Line 48--modified\n", 1)
|
||||
commit := tester.changeFile("file1.md", content)
|
||||
tester.createPR()
|
||||
|
||||
comment := tester.multiLineCommentOnPreviousFromFilesChanged("file1.md", 48, 1)
|
||||
assert.Equal(t, "previous", comment.DiffSide())
|
||||
assert.EqualValues(t, -48, comment.Line)
|
||||
assert.EqualValues(t, 1, comment.ExtraLinesCount)
|
||||
|
||||
diff := []diffTableRow{
|
||||
{rowType: RowHasCode, code: "Line 47"},
|
||||
{rowType: RowDelCode, code: "Line 48"},
|
||||
{rowType: RowAddCode, code: "Line 48--modified"},
|
||||
{rowType: RowHasCode, code: "Line 49"},
|
||||
{rowType: RowComment, commentID: comment.ID},
|
||||
{rowType: RowHasCode, code: "Line 50"},
|
||||
}
|
||||
tester.assertFilesChangedDiff(diff)
|
||||
tester.assertCommitDiff(commit, diff)
|
||||
})
|
||||
|
||||
t.Run("multi-line previous comment starting on an unmodified line stays previous", func(t *testing.T) {
|
||||
defer tests.PrintCurrentTest(t)()
|
||||
tester := newPullRequestCommentPlacementTester(t)
|
||||
|
||||
// Modify line 49 only; line 48 is unmodified. A previous comment on 48-49 starts on an
|
||||
// unmodified line but spans a modified one. It must NOT be converted to a proposed comment
|
||||
// just because its first line still exists — it stays on the previous side (gauche-2).
|
||||
content := strings.Replace(tester.fileContent, "Line 49\n", "Line 49--modified\n", 1)
|
||||
commit := tester.changeFile("file1.md", content)
|
||||
tester.createPR()
|
||||
|
||||
comment := tester.multiLineCommentOnPreviousFromFilesChanged("file1.md", 48, 1)
|
||||
assert.Equal(t, "previous", comment.DiffSide())
|
||||
assert.EqualValues(t, -48, comment.Line)
|
||||
assert.EqualValues(t, 1, comment.ExtraLinesCount)
|
||||
|
||||
diff := []diffTableRow{
|
||||
{rowType: RowHasCode, code: "Line 47"},
|
||||
{rowType: RowHasCode, code: "Line 48"},
|
||||
{rowType: RowDelCode, code: "Line 49"},
|
||||
{rowType: RowComment, commentID: comment.ID},
|
||||
{rowType: RowAddCode, code: "Line 49--modified"},
|
||||
{rowType: RowHasCode, code: "Line 50"},
|
||||
}
|
||||
tester.assertFilesChangedDiff(diff)
|
||||
tester.assertCommitDiff(commit, diff)
|
||||
})
|
||||
|
||||
t.Run("multi-line previous comment over only unmodified lines becomes proposed", func(t *testing.T) {
|
||||
defer tests.PrintCurrentTest(t)()
|
||||
tester := newPullRequestCommentPlacementTester(t)
|
||||
|
||||
// Modify line 51 (just after the range) so lines 48-50 stay unmodified but appear as context.
|
||||
// A previous comment over a fully unmodified range is recorded as a proposed-side comment
|
||||
// (matching upstream #12092), since every line still exists at head (gauche-1a).
|
||||
content := strings.Replace(tester.fileContent, "Line 51\n", "Line 51--modified\n", 1)
|
||||
tester.changeFile("file1.md", content)
|
||||
tester.createPR()
|
||||
|
||||
comment := tester.multiLineCommentOnPreviousFromFilesChanged("file1.md", 48, 2)
|
||||
assert.Equal(t, "proposed", comment.DiffSide())
|
||||
assert.Positive(t, comment.Line)
|
||||
assert.EqualValues(t, 2, comment.ExtraLinesCount)
|
||||
})
|
||||
|
||||
t.Run("multi-line comment invalidated by DB update", func(t *testing.T) {
|
||||
defer tests.PrintCurrentTest(t)()
|
||||
tester := newPullRequestCommentPlacementTester(t)
|
||||
|
||||
content := tester.fileContent
|
||||
content = strings.Replace(content, "Line 48\n", "Line 48--modified\n", 1)
|
||||
content = strings.Replace(content, "Line 49\n", "Line 49--modified\n", 1)
|
||||
content = strings.Replace(content, "Line 50\n", "Line 50--modified\n", 1)
|
||||
tester.changeFile("file1.md", content)
|
||||
tester.createPR()
|
||||
|
||||
comment := tester.multiLineCommentFromFilesChanged("file1.md", 48, 2)
|
||||
assert.False(t, comment.Invalidated)
|
||||
|
||||
// Manually mark as invalidated (same pattern as existing invalidation tests)
|
||||
require.NoError(t, issues_model.UpdateCommentInvalidate(t.Context(), &issues_model.Comment{
|
||||
ID: comment.ID,
|
||||
Invalidated: true,
|
||||
}))
|
||||
|
||||
// Reload and verify
|
||||
updatedComment := unittest.AssertExistsAndLoadBean(t, &issues_model.Comment{ID: comment.ID})
|
||||
assert.True(t, updatedComment.Invalidated)
|
||||
assert.EqualValues(t, 2, updatedComment.ExtraLinesCount)
|
||||
})
|
||||
|
||||
t.Run("multi-line comment invalidated when line inserted in range by second commit", func(t *testing.T) {
|
||||
defer tests.PrintCurrentTest(t)()
|
||||
tester := newPullRequestCommentPlacementTester(t)
|
||||
|
||||
// Modify lines 48-50 in the first commit
|
||||
content := tester.fileContent
|
||||
content = strings.Replace(content, "Line 48\n", "Line 48--modified\n", 1)
|
||||
content = strings.Replace(content, "Line 49\n", "Line 49--modified\n", 1)
|
||||
content = strings.Replace(content, "Line 50\n", "Line 50--modified\n", 1)
|
||||
tester.changeFile("file1.md", content)
|
||||
tester.createPR()
|
||||
|
||||
// Create a multi-line comment on lines 48-50 (line=48, extraLinesCount=2)
|
||||
comment := tester.multiLineCommentFromFilesChanged("file1.md", 48, 2)
|
||||
assert.EqualValues(t, 48, comment.Line)
|
||||
assert.EqualValues(t, 2, comment.ExtraLinesCount)
|
||||
assert.False(t, comment.Invalidated)
|
||||
|
||||
// Push a second commit that inserts a new line between 48 and 49,
|
||||
// breaking the contiguity of the range.
|
||||
content = strings.Replace(content, "Line 48--modified\n", "Line 48--modified\nInserted line\n", 1)
|
||||
tester.changeFile("file1.md", content)
|
||||
|
||||
// The invalidation is triggered asynchronously when the branch is pushed.
|
||||
// The comment should eventually be invalidated because the range [48, 49, 50]
|
||||
// resolves to non-contiguous lines at HEAD (48, 50, 51 — with a gap at 49).
|
||||
assert.EventuallyWithT(t, func(t *assert.CollectT) {
|
||||
commentReloaded := unittest.AssertExistsAndLoadBean(t, &issues_model.Comment{ID: comment.ID})
|
||||
assert.True(t, commentReloaded.Invalidated)
|
||||
}, 5*time.Second, 50*time.Millisecond)
|
||||
})
|
||||
|
||||
t.Run("multi-line comment not invalidated when range stays contiguous", func(t *testing.T) {
|
||||
defer tests.PrintCurrentTest(t)()
|
||||
tester := newPullRequestCommentPlacementTester(t)
|
||||
|
||||
// Modify lines 48-50 in the first commit
|
||||
content := tester.fileContent
|
||||
content = strings.Replace(content, "Line 48\n", "Line 48--modified\n", 1)
|
||||
content = strings.Replace(content, "Line 49\n", "Line 49--modified\n", 1)
|
||||
content = strings.Replace(content, "Line 50\n", "Line 50--modified\n", 1)
|
||||
tester.changeFile("file1.md", content)
|
||||
tester.createPR()
|
||||
|
||||
// Create a multi-line comment on lines 48-50 (line=48, extraLinesCount=2)
|
||||
comment := tester.multiLineCommentFromFilesChanged("file1.md", 48, 2)
|
||||
assert.EqualValues(t, 48, comment.Line)
|
||||
assert.EqualValues(t, 2, comment.ExtraLinesCount)
|
||||
assert.False(t, comment.Invalidated)
|
||||
|
||||
// Push a second commit that changes lines BEFORE the range (removing lines 1-10),
|
||||
// which shifts the range but keeps it contiguous.
|
||||
content = strings.Replace(content, "Line 1\nLine 2\nLine 3\nLine 4\nLine 5\nLine 6\nLine 7\nLine 8\nLine 9\nLine 10\n", "", 1)
|
||||
tester.changeFile("file1.md", content)
|
||||
|
||||
// Wait a bit for async invalidation to run, then check the comment is still valid.
|
||||
time.Sleep(2 * time.Second)
|
||||
commentReloaded := unittest.AssertExistsAndLoadBean(t, &issues_model.Comment{ID: comment.ID})
|
||||
assert.False(t, commentReloaded.Invalidated)
|
||||
|
||||
// Verify the comment is visible in the files changed diff (not skipped).
|
||||
// The full PR diff (mergebase→HEAD) groups deletions then additions for modified lines.
|
||||
diff := []diffTableRow{
|
||||
{rowType: RowHasCode, code: "Line 47"},
|
||||
{rowType: RowDelCode, code: "Line 48"},
|
||||
{rowType: RowDelCode, code: "Line 49"},
|
||||
{rowType: RowDelCode, code: "Line 50"},
|
||||
{rowType: RowAddCode, code: "Line 48--modified"},
|
||||
{rowType: RowAddCode, code: "Line 49--modified"},
|
||||
{rowType: RowAddCode, code: "Line 50--modified"},
|
||||
{rowType: RowComment, commentID: comment.ID},
|
||||
{rowType: RowHasCode, code: "Line 51"},
|
||||
}
|
||||
tester.assertFilesChangedDiff(diff)
|
||||
})
|
||||
})
|
||||
}
|
||||
|
||||
@@ -1908,6 +2163,13 @@ func (tester *PullRequestCommentPlacementTester) commentOnPreviousFromFilesChang
|
||||
return tester.commentFromNewCommentForm(resp, filename, line, "previous")
|
||||
}
|
||||
|
||||
func (tester *PullRequestCommentPlacementTester) multiLineCommentOnPreviousFromFilesChanged(filename string, line, extraLinesCount int) *issues_model.Comment {
|
||||
req := NewRequest(tester.t, "GET",
|
||||
fmt.Sprintf("/%s/%s/pulls/%d/files/reviews/new_comment", tester.repo.OwnerName, tester.repo.Name, tester.pr.Index))
|
||||
resp := tester.session.MakeRequest(tester.t, req, http.StatusOK)
|
||||
return tester.commentFromNewCommentFormWithExtraLinesCount(resp, filename, line, extraLinesCount, "previous")
|
||||
}
|
||||
|
||||
func (tester *PullRequestCommentPlacementTester) getCommitParent(commitID string) string {
|
||||
repo, err := gitrepo.OpenRepository(tester.t.Context(), tester.repo)
|
||||
require.NoError(tester.t, err)
|
||||
@@ -1936,6 +2198,10 @@ func (tester *PullRequestCommentPlacementTester) commentOnPreviousFromSpecificCo
|
||||
}
|
||||
|
||||
func (tester *PullRequestCommentPlacementTester) commentFromNewCommentForm(resp *httptest.ResponseRecorder, filename string, line int, side string) *issues_model.Comment {
|
||||
return tester.commentFromNewCommentFormWithExtraLinesCount(resp, filename, line, 0, side)
|
||||
}
|
||||
|
||||
func (tester *PullRequestCommentPlacementTester) commentFromNewCommentFormWithExtraLinesCount(resp *httptest.ResponseRecorder, filename string, line, extraLinesCount int, side string) *issues_model.Comment {
|
||||
commentContent := uuid.New().String()
|
||||
doc := NewHTMLParser(tester.t, resp.Body)
|
||||
tester.t.Logf("doc.before = %q", doc.GetInputValueByName("before_commit_id"))
|
||||
@@ -1943,17 +2209,18 @@ func (tester *PullRequestCommentPlacementTester) commentFromNewCommentForm(resp
|
||||
req := NewRequestWithValues(tester.t, "POST",
|
||||
fmt.Sprintf("/%s/%s/pulls/%d/files/reviews/comments", tester.repo.OwnerName, tester.repo.Name, tester.pr.Index),
|
||||
map[string]string{
|
||||
"origin": doc.GetInputValueByName("origin"),
|
||||
"before_commit_id": doc.GetInputValueByName("before_commit_id"),
|
||||
"latest_commit_id": doc.GetInputValueByName("latest_commit_id"),
|
||||
"side": side, // "proposed" (RHS) or "previous" (LHS)
|
||||
"line": strconv.Itoa(line),
|
||||
"path": filename,
|
||||
"diff_start_cid": doc.GetInputValueByName("diff_start_cid"),
|
||||
"diff_end_cid": doc.GetInputValueByName("diff_end_cid"),
|
||||
"diff_base_cid": doc.GetInputValueByName("diff_base_cid"),
|
||||
"content": commentContent,
|
||||
"single_review": "true",
|
||||
"origin": doc.GetInputValueByName("origin"),
|
||||
"before_commit_id": doc.GetInputValueByName("before_commit_id"),
|
||||
"latest_commit_id": doc.GetInputValueByName("latest_commit_id"),
|
||||
"side": side,
|
||||
"line": strconv.Itoa(line),
|
||||
"extra_lines_count": strconv.Itoa(extraLinesCount),
|
||||
"path": filename,
|
||||
"diff_start_cid": doc.GetInputValueByName("diff_start_cid"),
|
||||
"diff_end_cid": doc.GetInputValueByName("diff_end_cid"),
|
||||
"diff_base_cid": doc.GetInputValueByName("diff_base_cid"),
|
||||
"content": commentContent,
|
||||
"single_review": "true",
|
||||
})
|
||||
tester.session.MakeRequest(tester.t, req, http.StatusOK)
|
||||
|
||||
@@ -1961,6 +2228,13 @@ func (tester *PullRequestCommentPlacementTester) commentFromNewCommentForm(resp
|
||||
return comment
|
||||
}
|
||||
|
||||
func (tester *PullRequestCommentPlacementTester) multiLineCommentFromFilesChanged(filename string, line, extraLinesCount int) *issues_model.Comment {
|
||||
req := NewRequest(tester.t, "GET",
|
||||
fmt.Sprintf("/%s/%s/pulls/%d/files/reviews/new_comment", tester.repo.OwnerName, tester.repo.Name, tester.pr.Index))
|
||||
resp := tester.session.MakeRequest(tester.t, req, http.StatusOK)
|
||||
return tester.commentFromNewCommentFormWithExtraLinesCount(resp, filename, line, extraLinesCount, "proposed")
|
||||
}
|
||||
|
||||
func (tester *PullRequestCommentPlacementTester) withBranchCheckout(action func(string)) {
|
||||
dstPath := tester.t.TempDir()
|
||||
cloneURL, _ := url.Parse(tester.repo.CloneLink().HTTPS)
|
||||
|
||||
@@ -282,3 +282,16 @@
|
||||
width: 100%;
|
||||
height: 8px;
|
||||
}
|
||||
|
||||
/* Multi-line comment selection highlight (shift+drag in progress) */
|
||||
/* Unified view highlights the whole row; split view highlights only the commented side's cells */
|
||||
tr.diff-line-selected > td,
|
||||
td.diff-line-selected {
|
||||
background-color: var(--color-primary-alpha-20) !important;
|
||||
}
|
||||
|
||||
/* Existing multi-line comment range highlight */
|
||||
tr.diff-line-commented-range > td,
|
||||
td.diff-line-commented-range {
|
||||
background-color: var(--color-yellow-badge-bg) !important;
|
||||
}
|
||||
|
||||
@@ -10,6 +10,7 @@ import {initImageDiff} from './imagediff.js';
|
||||
import {showErrorToast} from '../modules/toast.js';
|
||||
import {submitEventSubmitter, queryElemSiblings, hideElem, showElem} from '../utils/dom.js';
|
||||
import {POST, GET} from '../modules/fetch.js';
|
||||
import {clearMultiLineSelection, refreshMultiLineCommentHighlights} from './repo-issue.js';
|
||||
|
||||
const {pageData, i18n} = window.config;
|
||||
|
||||
@@ -80,6 +81,33 @@ function initRepoDiffConversationForm() {
|
||||
const {path, side, idx} = $newConversationHolder.data();
|
||||
|
||||
$form.closest('.conversation-holder').replaceWith($newConversationHolder);
|
||||
// The backend may store the comment on the other side than it was drawn (e.g. a "previous" selection over
|
||||
// unchanged lines becomes a "proposed" comment). Move the rendered box into its final side's cell.
|
||||
const holderEl = $newConversationHolder[0];
|
||||
const currentTd = holderEl?.nodeType === 1 ? holderEl.closest('td') : null;
|
||||
const addCommentRow = currentTd?.closest('tr.add-comment');
|
||||
let movedSides = false;
|
||||
if (currentTd && addCommentRow && side) {
|
||||
const wantClass = side === 'left' ? 'add-comment-left' : 'add-comment-right';
|
||||
if (!currentTd.classList.contains(wantClass)) {
|
||||
const targetTd = addCommentRow.querySelector(`.${wantClass}`);
|
||||
if (targetTd && targetTd !== currentTd) {
|
||||
targetTd.append(holderEl);
|
||||
movedSides = true;
|
||||
}
|
||||
}
|
||||
}
|
||||
clearMultiLineSelection(); // Clear line selection
|
||||
refreshMultiLineCommentHighlights(); // refresh highlighted on all multilines comments without a page reload
|
||||
// When the comment moved to the other side, hide the add-code-comment button at the range's display
|
||||
// line (last line) on the final side too, so no leftover '+' button sits next to the moved box.
|
||||
if (movedSides) {
|
||||
const extra = parseInt(holderEl.getAttribute('data-extra-lines-count')) || 0;
|
||||
const displayIdx = parseInt(idx) + extra;
|
||||
for (const el of document.querySelectorAll(`[data-path="${path}"] .add-code-comment[data-side="${side}"][data-idx="${displayIdx}"]`)) {
|
||||
el.classList.add('tw-invisible');
|
||||
}
|
||||
}
|
||||
let selector;
|
||||
if ($form.closest('tr').data('line-type') === 'same') {
|
||||
selector = `[data-path="${path}"] .add-code-comment[data-idx="${idx}"]`;
|
||||
|
||||
@@ -153,6 +153,9 @@ export function initRepoIssueCommentDelete() {
|
||||
conversationHolder.remove();
|
||||
}
|
||||
|
||||
// Recompute multi-line comment highlights so a removed comment no longer leaves a stale range highlight.
|
||||
refreshMultiLineCommentHighlights();
|
||||
|
||||
// Check if there is no review content, move the time avatar upward to avoid overlapping the content below.
|
||||
if (!parentTimelineGroup?.querySelector('.timeline-item.comment') && !parentTimelineItem?.querySelector('.conversation-holder')) {
|
||||
const timelineAvatar = parentTimelineGroup?.querySelector('.timeline-avatar');
|
||||
@@ -188,6 +191,8 @@ export function initRepoIssueCodeCommentCancel() {
|
||||
document.addEventListener('click', (e) => {
|
||||
if (!e.target.matches('.cancel-code-comment')) return;
|
||||
|
||||
clearMultiLineSelection();
|
||||
|
||||
const form = e.target.closest('form');
|
||||
if (form?.classList.contains('comment-form')) {
|
||||
hideElem(form);
|
||||
@@ -376,7 +381,84 @@ export async function handleReply($el) {
|
||||
return editor;
|
||||
}
|
||||
|
||||
// Multi-line comment selection helpers
|
||||
|
||||
export function clearMultiLineSelection() {
|
||||
for (const el of document.querySelectorAll('.diff-line-selected')) {
|
||||
el.classList.remove('diff-line-selected');
|
||||
}
|
||||
}
|
||||
|
||||
function findDiffLineRow(path, side, lineNum) {
|
||||
const fileBox = document.querySelector(`[data-path="${path}"]`);
|
||||
if (!fileBox) return null;
|
||||
|
||||
const numClass = side === 'left' ? 'lines-num-old' : 'lines-num-new';
|
||||
const cells = fileBox.querySelectorAll(`.${numClass}[data-line-num="${lineNum}"]`);
|
||||
for (const cell of cells) {
|
||||
const row = cell.closest('tr');
|
||||
if (row) return row;
|
||||
}
|
||||
return null;
|
||||
}
|
||||
|
||||
// addLineHighlight applies a highlight class to a single diff line.
|
||||
// In split view a row holds both sides, so only the cells of `side` are highlighted to avoid
|
||||
// lighting up the opposite column. In unified view there is a single code column, so the whole
|
||||
// row is highlighted.
|
||||
function addLineHighlight(row, side, className) {
|
||||
if (!row.closest('.code-diff-split')) {
|
||||
row.classList.add(className);
|
||||
return;
|
||||
}
|
||||
const suffix = side === 'left' ? 'old' : 'new';
|
||||
for (const cell of row.querySelectorAll(`.lines-num-${suffix}, .lines-escape-${suffix}, .lines-type-marker-${suffix}, .lines-code-${suffix}`)) {
|
||||
cell.classList.add(className);
|
||||
}
|
||||
}
|
||||
|
||||
function highlightLineRange(path, side, startLine, endLine) {
|
||||
for (let i = startLine; i <= endLine; i++) {
|
||||
const row = findDiffLineRow(path, side, i);
|
||||
if (row) {
|
||||
addLineHighlight(row, side, 'diff-line-selected');
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
// refreshMultiLineCommentHighlights recomputes the highlight of every existing multi-line
|
||||
// comment range from the current DOM. It is idempotent: it first clears all previous range
|
||||
// highlights so that comments removed since the last call no longer leave stale highlighting,
|
||||
// then re-applies the highlight for every conversation holder still present.
|
||||
// Call it on page load and after any dynamic create/delete of a code comment.
|
||||
export function refreshMultiLineCommentHighlights() {
|
||||
for (const el of document.querySelectorAll('.diff-line-commented-range')) {
|
||||
el.classList.remove('diff-line-commented-range');
|
||||
}
|
||||
|
||||
for (const holder of document.querySelectorAll('.conversation-holder[data-extra-lines-count]')) {
|
||||
const extraLinesCount = parseInt(holder.getAttribute('data-extra-lines-count'));
|
||||
if (!extraLinesCount) continue;
|
||||
|
||||
const side = holder.getAttribute('data-side');
|
||||
const idx = parseInt(holder.getAttribute('data-idx'));
|
||||
const path = holder.getAttribute('data-path');
|
||||
|
||||
// idx is UnsignedLine (first line), the comment displays at idx + extraLinesCount (last line)
|
||||
// Highlight from idx to idx + extraLinesCount
|
||||
for (let i = idx; i <= idx + extraLinesCount; i++) {
|
||||
const row = findDiffLineRow(path, side, i);
|
||||
if (row) {
|
||||
addLineHighlight(row, side, 'diff-line-commented-range');
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
export function initRepoPullRequestReview() {
|
||||
// Highlight existing multi-line comment ranges
|
||||
refreshMultiLineCommentHighlights();
|
||||
|
||||
if (window.location.hash && window.location.hash.startsWith('#issuecomment-')) {
|
||||
// set scrollRestoration to 'manual' when there is a hash in url, so that the scroll position will not be remembered after refreshing
|
||||
if (window.history.scrollRestoration !== 'manual') {
|
||||
@@ -467,8 +549,124 @@ export function initRepoPullRequestReview() {
|
||||
});
|
||||
}
|
||||
|
||||
// Multi-line comment selection state (drag-based like GitHub)
|
||||
let multiLineDrag = null; // {side, startIdx, path, isSplit, newCommentUrl}
|
||||
|
||||
// Mousedown on + button with Shift held: start multi-line drag
|
||||
$(document).on('mousedown', '.add-code-comment', function (e) {
|
||||
if (e.target.classList.contains('btn-add-single')) return;
|
||||
if (!e.shiftKey) return;
|
||||
if (e.button !== 0) return; // left click only
|
||||
|
||||
e.preventDefault();
|
||||
e.stopPropagation();
|
||||
|
||||
const isSplit = this.closest('.code-diff')?.classList.contains('code-diff-split');
|
||||
const side = this.getAttribute('data-side');
|
||||
const idx = parseInt(this.getAttribute('data-idx'));
|
||||
const path = this.closest('[data-path]')?.getAttribute('data-path');
|
||||
const newCommentUrl = this.closest('[data-new-comment-url]')?.getAttribute('data-new-comment-url');
|
||||
|
||||
clearMultiLineSelection();
|
||||
multiLineDrag = {side, startIdx: idx, currentIdx: idx, path, isSplit, newCommentUrl};
|
||||
|
||||
// Highlight the initial line
|
||||
const row = findDiffLineRow(path, side, idx);
|
||||
if (row) addLineHighlight(row, side, 'diff-line-selected');
|
||||
|
||||
// Prevent text selection during drag
|
||||
document.body.style.userSelect = 'none';
|
||||
});
|
||||
|
||||
// Mouseover on diff table rows during drag: extend selection
|
||||
$(document).on('mouseover', 'tr[data-line-type]', function () {
|
||||
if (!multiLineDrag) return;
|
||||
|
||||
// Find the line number from the appropriate side's cell
|
||||
const numClass = multiLineDrag.side === 'left' ? 'lines-num-old' : 'lines-num-new';
|
||||
const numCell = this.querySelector(`.${numClass}[data-line-num]`);
|
||||
if (!numCell) return;
|
||||
|
||||
const lineNum = parseInt(numCell.getAttribute('data-line-num'));
|
||||
if (!lineNum || Number.isNaN(lineNum)) return;
|
||||
|
||||
// Check same file
|
||||
const filePath = this.closest('[data-path]')?.getAttribute('data-path');
|
||||
if (filePath !== multiLineDrag.path) return;
|
||||
|
||||
// Only extend downward (end >= start)
|
||||
if (lineNum < multiLineDrag.startIdx) return;
|
||||
|
||||
multiLineDrag.currentIdx = lineNum;
|
||||
|
||||
// Update highlight
|
||||
clearMultiLineSelection();
|
||||
highlightLineRange(multiLineDrag.path, multiLineDrag.side, multiLineDrag.startIdx, multiLineDrag.currentIdx);
|
||||
});
|
||||
|
||||
// Mouseup: finalize multi-line selection
|
||||
$(document).on('mouseup', async () => {
|
||||
if (!multiLineDrag) return;
|
||||
|
||||
document.body.style.userSelect = '';
|
||||
|
||||
const {side, startIdx, currentIdx, path, isSplit, newCommentUrl} = multiLineDrag;
|
||||
const lineStart = Math.min(startIdx, currentIdx);
|
||||
const lineEnd = Math.max(startIdx, currentIdx);
|
||||
const extraLinesCount = lineEnd - lineStart;
|
||||
multiLineDrag = null;
|
||||
|
||||
if (extraLinesCount === 0) {
|
||||
// Single line: clear highlight, no multi-line form
|
||||
clearMultiLineSelection();
|
||||
return;
|
||||
}
|
||||
|
||||
// Open comment form below the last line of the range
|
||||
const endTr = findDiffLineRow(path, side, lineEnd);
|
||||
if (!endTr) return;
|
||||
|
||||
const lineType = endTr.getAttribute('data-line-type') || 'same';
|
||||
const ntr = endTr.nextElementSibling;
|
||||
let $ntr = $(ntr);
|
||||
if (!ntr?.classList.contains('add-comment')) {
|
||||
$ntr = $(`
|
||||
<tr class="add-comment" data-line-type="${lineType}">
|
||||
${isSplit ? `
|
||||
<td class="add-comment-left" colspan="4"></td>
|
||||
<td class="add-comment-right" colspan="4"></td>
|
||||
` : `
|
||||
<td class="add-comment-left add-comment-right" colspan="5"></td>
|
||||
`}
|
||||
</tr>`);
|
||||
$(endTr).after($ntr);
|
||||
}
|
||||
|
||||
const $td = $ntr.find(`.add-comment-${side}`);
|
||||
const $commentCloud = $td.find('.comment-code-cloud');
|
||||
if (!$commentCloud.length && !$ntr.find('button[name="pending_review"]').length) {
|
||||
try {
|
||||
const response = await GET(newCommentUrl);
|
||||
const html = await response.text();
|
||||
$td.html(html);
|
||||
$td.find("input[name='line']").val(lineStart);
|
||||
$td.find("input[name='side']").val(side === 'left' ? 'previous' : 'proposed');
|
||||
$td.find("input[name='path']").val(path);
|
||||
$td.find("input[name='extra_lines_count']").val(extraLinesCount);
|
||||
|
||||
await initDropzone($td.find('.dropzone')[0]);
|
||||
const editor = await initComboMarkdownEditor($td.find('.combo-markdown-editor'));
|
||||
editor.focus();
|
||||
} catch (error) {
|
||||
console.error(error);
|
||||
}
|
||||
}
|
||||
});
|
||||
|
||||
// Normal click (no shift): single-line comment (existing behavior)
|
||||
$(document).on('click', '.add-code-comment', async function (e) {
|
||||
if (e.target.classList.contains('btn-add-single')) return; // https://github.com/go-gitea/gitea/issues/4745
|
||||
if (e.shiftKey) return; // handled by mousedown/mouseup above
|
||||
e.preventDefault();
|
||||
|
||||
const isSplit = this.closest('.code-diff')?.classList.contains('code-diff-split');
|
||||
@@ -512,6 +710,19 @@ export function initRepoPullRequestReview() {
|
||||
}
|
||||
}
|
||||
});
|
||||
|
||||
// Clear multi-line selection when pressing Escape or cancelling
|
||||
$(document).on('click', '.cancel-code-comment', () => {
|
||||
clearMultiLineSelection();
|
||||
multiLineDrag = null;
|
||||
});
|
||||
$(document).on('keydown', (e) => {
|
||||
if (e.key === 'Escape' && multiLineDrag) {
|
||||
document.body.style.userSelect = '';
|
||||
clearMultiLineSelection();
|
||||
multiLineDrag = null;
|
||||
}
|
||||
});
|
||||
}
|
||||
|
||||
export function initRepoIssueReferenceIssue() {
|
||||
|
||||
Reference in New Issue
Block a user