[BUG] split code conversations in diff tab (go-gitea#2306)

Follow-up of go-gitea#2282 and go-gitea#2296 (which tried to address go-gitea#2278)

One of the issue with the previous PR is that when a conversation on the Files tab was marked as "resolved", it would fetch all the comments for that line (even the outdated ones, which should not be shown on this page - except when explicitly activated).

To properly fix this, I have changed `FetchCodeCommentsByLine` to `FetchCodeConversation`. Its role is to fetch all comments related to a given (review, path, line) and reverted my changes in the template (which were based on a misunderstanding).

Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/2306
Reviewed-by: Earl Warren <[email protected]>
Reviewed-by: Gusted <[email protected]>
Co-authored-by: oliverpool <[email protected]>
Co-committed-by: oliverpool <[email protected]>

Author: oliverpool

models/issues/comment_code.go

@@ -14,15 +14,58 @@ import (
 	"xorm.io/builder"
 )
 
-// CodeComments represents comments on code by using this structure: FILENAME -> LINE (+ == proposed; - == previous) -> COMMENTS
-type CodeComments map[string]map[int64][]*Comment
+// CodeConversation contains the comment of a given review
+type CodeConversation []*Comment
+
+// CodeConversationsAtLine contains the conversations for a given line
+type CodeConversationsAtLine map[int64][]CodeConversation
+
+// CodeConversationsAtLineAndTreePath contains the conversations for a given TreePath and line
+type CodeConversationsAtLineAndTreePath map[string]CodeConversationsAtLine
+
+func newCodeConversationsAtLineAndTreePath(comments []*Comment) CodeConversationsAtLineAndTreePath {
+	tree := make(CodeConversationsAtLineAndTreePath)
+	for _, comment := range comments {
+		tree.insertComment(comment)
+	}
+	return tree
+}
+
+func (tree CodeConversationsAtLineAndTreePath) insertComment(comment *Comment) {
+	// attempt to append comment to existing conversations (i.e. list of comments belonging to the same review)
+	for i, conversation := range tree[comment.TreePath][comment.Line] {
+		if conversation[0].ReviewID == comment.ReviewID {
+			tree[comment.TreePath][comment.Line][i] = append(conversation, comment)
+			return
+		}
+	}
+
+	// no previous conversation was found at this line, create it
+	if tree[comment.TreePath] == nil {
+		tree[comment.TreePath] = make(map[int64][]CodeConversation)
+	}
 
-// FetchCodeComments will return a 2d-map: ["Path"]["Line"] = Comments at line
-func FetchCodeComments(ctx context.Context, issue *Issue, currentUser *user_model.User, showOutdatedComments bool) (CodeComments, error) {
-	return fetchCodeCommentsByReview(ctx, issue, currentUser, nil, showOutdatedComments)
+	tree[comment.TreePath][comment.Line] = append(tree[comment.TreePath][comment.Line], CodeConversation{comment})
 }
 
-func fetchCodeCommentsByReview(ctx context.Context, issue *Issue, currentUser *user_model.User, review *Review, showOutdatedComments bool) (CodeComments, error) {
+// FetchCodeConversations will return a 2d-map: ["Path"]["Line"] = List of CodeConversation (one per review) for this line
+func FetchCodeConversations(ctx context.Context, issue *Issue, doer *user_model.User, showOutdatedComments bool) (CodeConversationsAtLineAndTreePath, error) {
+	opts := FindCommentsOptions{
+		Type:    CommentTypeCode,
+		IssueID: issue.ID,
+	}
+	comments, err := findCodeComments(ctx, opts, issue, doer, nil, showOutdatedComments)
+	if err != nil {
+		return nil, err
+	}
+
+	return newCodeConversationsAtLineAndTreePath(comments), nil
+}
+
+// CodeComments represents comments on code by using this structure: FILENAME -> LINE (+ == proposed; - == previous) -> COMMENTS
+type CodeComments map[string]map[int64][]*Comment
+
+func fetchCodeCommentsByReview(ctx context.Context, issue *Issue, doer *user_model.User, review *Review, showOutdatedComments bool) (CodeComments, error) {
 	pathToLineToComment := make(CodeComments)
 	if review == nil {
 		review = &Review{ID: 0}
@@ -33,7 +76,7 @@ func fetchCodeCommentsByReview(ctx context.Context, issue *Issue, currentUser *u
 		ReviewID: review.ID,
 	}
 
-	comments, err := findCodeComments(ctx, opts, issue, currentUser, review, showOutdatedComments)
+	comments, err := findCodeComments(ctx, opts, issue, doer, review, showOutdatedComments)
 	if err != nil {
 		return nil, err
 	}
@@ -47,7 +90,7 @@ func fetchCodeCommentsByReview(ctx context.Context, issue *Issue, currentUser *u
 	return pathToLineToComment, nil
 }
 
-func findCodeComments(ctx context.Context, opts FindCommentsOptions, issue *Issue, currentUser *user_model.User, review *Review, showOutdatedComments bool) ([]*Comment, error) {
+func findCodeComments(ctx context.Context, opts FindCommentsOptions, issue *Issue, doer *user_model.User, review *Review, showOutdatedComments bool) ([]*Comment, error) {
 	var comments CommentList
 	if review == nil {
 		review = &Review{ID: 0}
@@ -91,7 +134,7 @@ func findCodeComments(ctx context.Context, opts FindCommentsOptions, issue *Issu
 		if re, ok := reviews[comment.ReviewID]; ok && re != nil {
 			// If the review is pending only the author can see the comments (except if the review is set)
 			if review.ID == 0 && re.Type == ReviewTypePending &&
-				(currentUser == nil || currentUser.ID != re.ReviewerID) {
+				(doer == nil || doer.ID != re.ReviewerID) {
 				continue
 			}
 			comment.Review = re
@@ -121,13 +164,14 @@ func findCodeComments(ctx context.Context, opts FindCommentsOptions, issue *Issu
 	return comments[:n], nil
 }
 
-// FetchCodeCommentsByLine fetches the code comments for a given treePath and line number
-func FetchCodeCommentsByLine(ctx context.Context, issue *Issue, currentUser *user_model.User, treePath string, line int64, showOutdatedComments bool) ([]*Comment, error) {
+// FetchCodeConversation fetches the code conversation of a given comment (same review, treePath and line number)
+func FetchCodeConversation(ctx context.Context, comment *Comment, doer *user_model.User) ([]*Comment, error) {
 	opts := FindCommentsOptions{
 		Type:     CommentTypeCode,
-		IssueID:  issue.ID,
-		TreePath: treePath,
-		Line:     line,
+		IssueID:  comment.IssueID,
+		ReviewID: comment.ReviewID,
+		TreePath: comment.TreePath,
+		Line:     comment.Line,
 	}
-	return findCodeComments(ctx, opts, issue, currentUser, nil, showOutdatedComments)
+	return findCodeComments(ctx, opts, comment.Issue, doer, nil, true)
 }

models/issues/comment_test.go

@@ -46,20 +46,20 @@ func TestCreateComment(t *testing.T) {
 	unittest.AssertInt64InRange(t, now, then, int64(updatedIssue.UpdatedUnix))
 }
 
-func TestFetchCodeComments(t *testing.T) {
+func TestFetchCodeConversations(t *testing.T) {
 	assert.NoError(t, unittest.PrepareTestDatabase())
 
 	issue := unittest.AssertExistsAndLoadBean(t, &issues_model.Issue{ID: 2})
 	user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 1})
-	res, err := issues_model.FetchCodeComments(db.DefaultContext, issue, user, false)
+	res, err := issues_model.FetchCodeConversations(db.DefaultContext, issue, user, false)
 	assert.NoError(t, err)
 	assert.Contains(t, res, "README.md")
 	assert.Contains(t, res["README.md"], int64(4))
 	assert.Len(t, res["README.md"][4], 1)
-	assert.Equal(t, int64(4), res["README.md"][4][0].ID)
+	assert.Equal(t, int64(4), res["README.md"][4][0][0].ID)
 
 	user2 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2})
-	res, err = issues_model.FetchCodeComments(db.DefaultContext, issue, user2, false)
+	res, err = issues_model.FetchCodeConversations(db.DefaultContext, issue, user2, false)
 	assert.NoError(t, err)
 	assert.Len(t, res, 1)
 }

routers/web/repo/pull_review.go

@@ -153,7 +153,7 @@ func UpdateResolveConversation(ctx *context.Context) {
 }
 
 func renderConversation(ctx *context.Context, comment *issues_model.Comment, origin string) {
-	comments, err := issues_model.FetchCodeCommentsByLine(ctx, comment.Issue, ctx.Doer, comment.TreePath, comment.Line, true)
+	comments, err := issues_model.FetchCodeConversation(ctx, comment, ctx.Doer)
 	if err != nil {
 		ctx.ServerError("FetchCodeCommentsByLine", err)
 		return

services/gitdiff/gitdiff.go

@@ -13,7 +13,6 @@ import (
 	"html/template"
 	"io"
 	"net/url"
-	"sort"
 	"strings"
 	"time"
 
@@ -75,13 +74,13 @@ const (
 
 // DiffLine represents a line difference in a DiffSection.
 type DiffLine struct {
-	LeftIdx     int
-	RightIdx    int
-	Match       int
-	Type        DiffLineType
-	Content     string
-	Comments    []*issues_model.Comment
-	SectionInfo *DiffLineSectionInfo
+	LeftIdx       int
+	RightIdx      int
+	Match         int
+	Type          DiffLineType
+	Content       string
+	Conversations []issues_model.CodeConversation
+	SectionInfo   *DiffLineSectionInfo
 }
 
 // DiffLineSectionInfo represents diff line section meta data
@@ -118,15 +117,15 @@ func (d *DiffLine) GetHTMLDiffLineType() string {
 
 // CanComment returns whether a line can get commented
 func (d *DiffLine) CanComment() bool {
-	return len(d.Comments) == 0 && d.Type != DiffLineSection
+	return len(d.Conversations) == 0 && d.Type != DiffLineSection
 }
 
 // GetCommentSide returns the comment side of the first comment, if not set returns empty string
 func (d *DiffLine) GetCommentSide() string {
-	if len(d.Comments) == 0 {
+	if len(d.Conversations) == 0 || len(d.Conversations[0]) == 0 {
 		return ""
 	}
-	return d.Comments[0].DiffSide()
+	return d.Conversations[0][0].DiffSide()
 }
 
 // GetLineTypeMarker returns the line type marker
@@ -467,23 +466,20 @@ type Diff struct {
 
 // LoadComments loads comments into each line
 func (diff *Diff) LoadComments(ctx context.Context, issue *issues_model.Issue, currentUser *user_model.User, showOutdatedComments bool) error {
-	allComments, err := issues_model.FetchCodeComments(ctx, issue, currentUser, showOutdatedComments)
+	allConversations, err := issues_model.FetchCodeConversations(ctx, issue, currentUser, showOutdatedComments)
 	if err != nil {
 		return err
 	}
 	for _, file := range diff.Files {
-		if lineCommits, ok := allComments[file.Name]; ok {
+		if lineCommits, ok := allConversations[file.Name]; ok {
 			for _, section := range file.Sections {
 				for _, line := range section.Lines {
-					if comments, ok := lineCommits[int64(line.LeftIdx*-1)]; ok {
-						line.Comments = append(line.Comments, comments...)
+					if conversations, ok := lineCommits[int64(line.LeftIdx*-1)]; ok {
+						line.Conversations = append(line.Conversations, conversations...)
 					}
 					if comments, ok := lineCommits[int64(line.RightIdx)]; ok {
-						line.Comments = append(line.Comments, comments...)
+						line.Conversations = append(line.Conversations, comments...)
 					}
-					sort.SliceStable(line.Comments, func(i, j int) bool {
-						return line.Comments[i].CreatedUnix < line.Comments[j].CreatedUnix
-					})
 				}
 			}
 		}

services/gitdiff/gitdiff_test.go

@@ -601,7 +601,7 @@ func TestDiff_LoadCommentsNoOutdated(t *testing.T) {
 	user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 1})
 	diff := setupDefaultDiff()
 	assert.NoError(t, diff.LoadComments(db.DefaultContext, issue, user, false))
-	assert.Len(t, diff.Files[0].Sections[0].Lines[0].Comments, 2)
+	assert.Len(t, diff.Files[0].Sections[0].Lines[0].Conversations, 2)
 }
 
 func TestDiff_LoadCommentsWithOutdated(t *testing.T) {
@@ -611,20 +611,22 @@ func TestDiff_LoadCommentsWithOutdated(t *testing.T) {
 	user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 1})
 	diff := setupDefaultDiff()
 	assert.NoError(t, diff.LoadComments(db.DefaultContext, issue, user, true))
-	assert.Len(t, diff.Files[0].Sections[0].Lines[0].Comments, 3)
+	assert.Len(t, diff.Files[0].Sections[0].Lines[0].Conversations, 2)
+	assert.Len(t, diff.Files[0].Sections[0].Lines[0].Conversations[0], 2)
+	assert.Len(t, diff.Files[0].Sections[0].Lines[0].Conversations[1], 1)
 }
 
 func TestDiffLine_CanComment(t *testing.T) {
 	assert.False(t, (&DiffLine{Type: DiffLineSection}).CanComment())
-	assert.False(t, (&DiffLine{Type: DiffLineAdd, Comments: []*issues_model.Comment{{Content: "bla"}}}).CanComment())
+	assert.False(t, (&DiffLine{Type: DiffLineAdd, Conversations: []issues_model.CodeConversation{{{Content: "bla"}}}}).CanComment())
 	assert.True(t, (&DiffLine{Type: DiffLineAdd}).CanComment())
 	assert.True(t, (&DiffLine{Type: DiffLineDel}).CanComment())
 	assert.True(t, (&DiffLine{Type: DiffLinePlain}).CanComment())
 }
 
 func TestDiffLine_GetCommentSide(t *testing.T) {
-	assert.Equal(t, "previous", (&DiffLine{Comments: []*issues_model.Comment{{Line: -3}}}).GetCommentSide())
-	assert.Equal(t, "proposed", (&DiffLine{Comments: []*issues_model.Comment{{Line: 3}}}).GetCommentSide())
+	assert.Equal(t, "previous", (&DiffLine{Conversations: []issues_model.CodeConversation{{{Line: -3}}}}).GetCommentSide())
+	assert.Equal(t, "proposed", (&DiffLine{Conversations: []issues_model.CodeConversation{{{Line: 3}}}}).GetCommentSide())
 }
 
 func TestGetDiffRangeWithWhitespaceBehavior(t *testing.T) {

services/repository/files/diff_test.go

@@ -53,11 +53,11 @@ func TestGetDiffPreview(t *testing.T) {
 						Name:     "",
 						Lines: []*gitdiff.DiffLine{
 							{
-								LeftIdx:  0,
-								RightIdx: 0,
-								Type:     4,
-								Content:  "@@ -1,3 +1,4 @@",
-								Comments: nil,
+								LeftIdx:       0,
+								RightIdx:      0,
+								Type:          4,
+								Content:       "@@ -1,3 +1,4 @@",
+								Conversations: nil,
 								SectionInfo: &gitdiff.DiffLineSectionInfo{
 									Path:          "README.md",
 									LastLeftIdx:   0,
@@ -69,42 +69,42 @@ func TestGetDiffPreview(t *testing.T) {
 								},
 							},
 							{
-								LeftIdx:  1,
-								RightIdx: 1,
-								Type:     1,
-								Content:  " # repo1",
-								Comments: nil,
+								LeftIdx:       1,
+								RightIdx:      1,
+								Type:          1,
+								Content:       " # repo1",
+								Conversations: nil,
 							},
 							{
-								LeftIdx:  2,
-								RightIdx: 2,
-								Type:     1,
-								Content:  " ",
-								Comments: nil,
+								LeftIdx:       2,
+								RightIdx:      2,
+								Type:          1,
+								Content:       " ",
+								Conversations: nil,
 							},
 							{
-								LeftIdx:  3,
-								RightIdx: 0,
-								Match:    4,
-								Type:     3,
-								Content:  "-Description for repo1",
-								Comments: nil,
+								LeftIdx:       3,
+								RightIdx:      0,
+								Match:         4,
+								Type:          3,
+								Content:       "-Description for repo1",
+								Conversations: nil,
 							},
 							{
-								LeftIdx:  0,
-								RightIdx: 3,
-								Match:    3,
-								Type:     2,
-								Content:  "+Description for repo1",
-								Comments: nil,
+								LeftIdx:       0,
+								RightIdx:      3,
+								Match:         3,
+								Type:          2,
+								Content:       "+Description for repo1",
+								Conversations: nil,
 							},
 							{
-								LeftIdx:  0,
-								RightIdx: 4,
-								Match:    -1,
-								Type:     2,
-								Content:  "+this is a new line",
-								Comments: nil,
+								LeftIdx:       0,
+								RightIdx:      4,
+								Match:         -1,
+								Type:          2,
+								Content:       "+this is a new line",
+								Conversations: nil,
 							},
 						},
 					},

templates/repo/diff/conversations.tmpl

@@ -0,0 +1,3 @@
+{{range .conversations}}
+	{{template "repo/diff/conversation" dict "." $ "comments" .}}
+{{end}}