From 17be181875c84335eedbe359c1876e075fa877e5 Mon Sep 17 00:00:00 2001 From: brymut Date: Tue, 14 Oct 2025 17:35:45 +0300 Subject: [PATCH 01/22] feat(diff): Enable commenting on expanded lines in PR diffs --- routers/web/repo/compare.go | 37 ++++++ services/gitdiff/gitdiff.go | 136 +++++++++++++++++++--- templates/repo/diff/blob_excerpt.tmpl | 46 +++++++- templates/repo/diff/section_split.tmpl | 11 +- templates/repo/diff/section_unified.tmpl | 11 +- web_src/css/index.css | 1 + web_src/css/repo/diff-hidden-comments.css | 38 ++++++ 7 files changed, 252 insertions(+), 28 deletions(-) create mode 100644 web_src/css/repo/diff-hidden-comments.css diff --git a/routers/web/repo/compare.go b/routers/web/repo/compare.go index 9262703078a6a..3104301d54959 100644 --- a/routers/web/repo/compare.go +++ b/routers/web/repo/compare.go @@ -14,6 +14,7 @@ import ( "net/http" "net/url" "path/filepath" + "sort" "strings" "code.gitea.io/gitea/models/db" @@ -43,6 +44,7 @@ import ( "code.gitea.io/gitea/services/context/upload" "code.gitea.io/gitea/services/gitdiff" pull_service "code.gitea.io/gitea/services/pull" + user_service "code.gitea.io/gitea/services/user" ) const ( @@ -947,6 +949,41 @@ func ExcerptBlob(ctx *context.Context) { section.Lines = append(section.Lines, lineSection) } } + if ctx.Doer != nil { + ctx.Data["SignedUserID"] = ctx.Doer.ID + } + isPull := ctx.FormBool("is_pull") + ctx.Data["PageIsPullFiles"] = isPull + + if isPull { + issueIndex := ctx.FormInt64("issue_index") + ctx.Data["IssueIndex"] = issueIndex + if issueIndex > 0 { + if issue, err := issues_model.GetIssueByIndex(ctx, ctx.Repo.Repository.ID, issueIndex); err == nil && issue.IsPull { + ctx.Data["Issue"] = issue + ctx.Data["CanBlockUser"] = func(blocker, blockee *user_model.User) bool { + return user_service.CanBlockUser(ctx, ctx.Doer, blocker, blockee) + } + + if allComments, err := issues_model.FetchCodeComments(ctx, issue, ctx.Doer, ctx.FormBool("show_outdated")); err == nil { + if lineComments, ok := allComments[filePath]; ok { + for _, line := range section.Lines { + if comments, ok := lineComments[int64(line.LeftIdx*-1)]; ok { + line.Comments = append(line.Comments, comments...) + } + if comments, ok := lineComments[int64(line.RightIdx)]; ok { + line.Comments = append(line.Comments, comments...) + } + sort.SliceStable(line.Comments, func(i, j int) bool { + return line.Comments[i].CreatedUnix < line.Comments[j].CreatedUnix + }) + } + } + } + } + } + } + ctx.Data["section"] = section ctx.Data["FileNameHash"] = git.HashFilePathForWebUI(filePath) ctx.Data["AfterCommitID"] = commitID diff --git a/services/gitdiff/gitdiff.go b/services/gitdiff/gitdiff.go index db0f565b526a0..52cfaf25ddffd 100644 --- a/services/gitdiff/gitdiff.go +++ b/services/gitdiff/gitdiff.go @@ -81,13 +81,15 @@ const ( // DiffLine represents a line difference in a DiffSection. type DiffLine struct { - LeftIdx int // line number, 1-based - RightIdx int // line number, 1-based - Match int // the diff matched index. -1: no match. 0: plain and no need to match. >0: for add/del, "Lines" slice index of the other side - Type DiffLineType - Content string - Comments issues_model.CommentList // related PR code comments - SectionInfo *DiffLineSectionInfo + LeftIdx int // line number, 1-based + RightIdx int // line number, 1-based + Match int // the diff matched index. -1: no match. 0: plain and no need to match. >0: for add/del, "Lines" slice index of the other side + Type DiffLineType + Content string + Comments issues_model.CommentList // related PR code comments + SectionInfo *DiffLineSectionInfo + HasHiddenComments bool // indicates if this expand button has comments in hidden lines + HiddenCommentCount int // number of hidden comments in this section } // DiffLineSectionInfo represents diff line section meta data @@ -485,6 +487,25 @@ func (diff *Diff) LoadComments(ctx context.Context, issue *issues_model.Issue, c sort.SliceStable(line.Comments, func(i, j int) bool { return line.Comments[i].CreatedUnix < line.Comments[j].CreatedUnix }) + + // Mark expand buttons that have comments in hidden lines + if line.Type == DiffLineSection && line.SectionInfo != nil { + hiddenCommentCount := 0 + // Check if there are comments in the hidden range + for commentLineNum := range lineCommits { + absLineNum := int(commentLineNum) + if commentLineNum < 0 { + absLineNum = int(-commentLineNum) + } + if absLineNum > line.SectionInfo.LastRightIdx && absLineNum < line.SectionInfo.RightIdx { + hiddenCommentCount++ + } + } + if hiddenCommentCount > 0 { + line.HasHiddenComments = true + line.HiddenCommentCount = hiddenCommentCount + } + } } } } @@ -1370,19 +1391,102 @@ outer: // CommentAsDiff returns c.Patch as *Diff func CommentAsDiff(ctx context.Context, c *issues_model.Comment) (*Diff, error) { - diff, err := ParsePatch(ctx, setting.Git.MaxGitDiffLines, - setting.Git.MaxGitDiffLineCharacters, setting.Git.MaxGitDiffFiles, strings.NewReader(c.Patch), "") + // Try to parse existing patch first + if c.Patch != "" { + diff, err := ParsePatch(ctx, setting.Git.MaxGitDiffLines, + setting.Git.MaxGitDiffLineCharacters, setting.Git.MaxGitDiffFiles, strings.NewReader(c.Patch), "") + if err == nil && len(diff.Files) > 0 && len(diff.Files[0].Sections) > 0 { + return diff, nil + } + } + + // If patch is empty or invalid, generate code context for unchanged line comments + if c.TreePath != "" && c.CommitSHA != "" && c.Line != 0 { + return generateCodeContextForComment(ctx, c) + } + + return nil, fmt.Errorf("no valid patch or context available for comment ID: %d", c.ID) +} + +func generateCodeContextForComment(ctx context.Context, c *issues_model.Comment) (*Diff, error) { + if err := c.LoadIssue(ctx); err != nil { + return nil, fmt.Errorf("LoadIssue: %w", err) + } + if err := c.Issue.LoadRepo(ctx); err != nil { + return nil, fmt.Errorf("LoadRepo: %w", err) + } + if err := c.Issue.LoadPullRequest(ctx); err != nil { + return nil, fmt.Errorf("LoadPullRequest: %w", err) + } + + pr := c.Issue.PullRequest + if err := pr.LoadBaseRepo(ctx); err != nil { + return nil, fmt.Errorf("LoadBaseRepo: %w", err) + } + + gitRepo, closer, err := gitrepo.RepositoryFromContextOrOpen(ctx, pr.BaseRepo) if err != nil { - log.Error("Unable to parse patch: %v", err) - return nil, err + return nil, fmt.Errorf("RepositoryFromContextOrOpen: %w", err) } - if len(diff.Files) == 0 { - return nil, fmt.Errorf("no file found for comment ID: %d", c.ID) + defer closer.Close() + + // Get the file content at the commit + commit, err := gitRepo.GetCommit(c.CommitSHA) + if err != nil { + return nil, fmt.Errorf("GetCommit: %w", err) + } + + entry, err := commit.GetTreeEntryByPath(c.TreePath) + if err != nil { + return nil, fmt.Errorf("GetTreeEntryByPath: %w", err) + } + + blob := entry.Blob() + dataRc, err := blob.DataAsync() + if err != nil { + return nil, fmt.Errorf("DataAsync: %w", err) + } + defer dataRc.Close() + + // Read file lines + scanner := bufio.NewScanner(dataRc) + var lines []string + for scanner.Scan() { + lines = append(lines, scanner.Text()) + } + if err := scanner.Err(); err != nil { + return nil, fmt.Errorf("scanner error: %w", err) } - secs := diff.Files[0].Sections - if len(secs) == 0 { - return nil, fmt.Errorf("no sections found for comment ID: %d", c.ID) + + // Validate comment line is within file bounds + commentLine := int(c.UnsignedLine()) + if commentLine < 1 || commentLine > len(lines) { + return nil, fmt.Errorf("comment line %d is out of file bounds (1-%d)", commentLine, len(lines)) + } + + // Calculate line range to show (commented line + lines above it, matching CutDiffAroundLine behavior) + contextLines := setting.UI.CodeCommentLines + startLine := max(commentLine-contextLines, 1) + endLine := commentLine + + var patchBuilder strings.Builder + patchBuilder.WriteString(fmt.Sprintf("diff --git a/%s b/%s\n", c.TreePath, c.TreePath)) + patchBuilder.WriteString(fmt.Sprintf("--- a/%s\n", c.TreePath)) + patchBuilder.WriteString(fmt.Sprintf("+++ b/%s\n", c.TreePath)) + patchBuilder.WriteString(fmt.Sprintf("@@ -%d,%d +%d,%d @@\n", startLine, endLine-startLine+1, startLine, endLine-startLine+1)) + + for i := startLine - 1; i < endLine; i++ { + patchBuilder.WriteString(" ") + patchBuilder.WriteString(lines[i]) + patchBuilder.WriteString("\n") } + + diff, err := ParsePatch(ctx, setting.Git.MaxGitDiffLines, + setting.Git.MaxGitDiffLineCharacters, setting.Git.MaxGitDiffFiles, strings.NewReader(patchBuilder.String()), "") + if err != nil { + return nil, fmt.Errorf("ParsePatch: %w", err) + } + return diff, nil } diff --git a/templates/repo/diff/blob_excerpt.tmpl b/templates/repo/diff/blob_excerpt.tmpl index 4089d8fb33ba8..5cc75b516c4c9 100644 --- a/templates/repo/diff/blob_excerpt.tmpl +++ b/templates/repo/diff/blob_excerpt.tmpl @@ -1,7 +1,7 @@ -{{$blobExcerptLink := print $.RepoLink (Iif $.PageIsWiki "/wiki" "") "/blob_excerpt/" (PathEscape $.AfterCommitID) (QueryBuild "?" "anchor" $.Anchor)}} +{{$blobExcerptLink := print $.RepoLink (Iif $.PageIsWiki "/wiki" "") "/blob_excerpt/" (PathEscape $.AfterCommitID) "?" (Iif $.PageIsPullFiles (print "is_pull=true&issue_index=" $.IssueIndex "&") "")}} {{if $.IsSplitStyle}} {{range $k, $line := $.section.Lines}} - + {{if eq .GetType 4}} {{$expandDirection := $line.GetExpandDirection}} @@ -33,6 +33,11 @@ {{if and $line.LeftIdx $inlineDiff.EscapeStatus.Escaped}}{{end}} {{if $line.LeftIdx}}{{end}} + {{- if and $.SignedUserID $.PageIsPullFiles $line.LeftIdx -}} + + {{- end -}} {{- if $line.LeftIdx -}} {{- template "repo/diff/section_code" dict "diff" $inlineDiff -}} {{- else -}} @@ -43,6 +48,11 @@ {{if and $line.RightIdx $inlineDiff.EscapeStatus.Escaped}}{{end}} {{if $line.RightIdx}}{{end}} + {{- if and $.SignedUserID $.PageIsPullFiles $line.RightIdx -}} + + {{- end -}} {{- if $line.RightIdx -}} {{- template "repo/diff/section_code" dict "diff" $inlineDiff -}} {{- else -}} @@ -51,10 +61,24 @@ {{end}} + {{if $line.Comments}} + + + {{if eq $line.GetCommentSide "previous"}} + {{template "repo/diff/conversation" dict "." $ "comments" $line.Comments}} + {{end}} + + + {{if eq $line.GetCommentSide "proposed"}} + {{template "repo/diff/conversation" dict "." $ "comments" $line.Comments}} + {{end}} + + + {{end}} {{end}} {{else}} {{range $k, $line := $.section.Lines}} - + {{if eq .GetType 4}} {{$expandDirection := $line.GetExpandDirection}} @@ -83,7 +107,21 @@ {{$inlineDiff := $.section.GetComputedInlineDiffFor $line ctx.Locale}} {{if $inlineDiff.EscapeStatus.Escaped}}{{end}} - {{$inlineDiff.Content}} + + {{- if and $.SignedUserID $.PageIsPullFiles -}} + + {{- end -}} + {{$inlineDiff.Content}} + + {{if $line.Comments}} + + + {{template "repo/diff/conversation" dict "." $ "comments" $line.Comments}} + + + {{end}} {{end}} {{end}} diff --git a/templates/repo/diff/section_split.tmpl b/templates/repo/diff/section_split.tmpl index 9953db5eb234c..75c7f7c059c0c 100644 --- a/templates/repo/diff/section_split.tmpl +++ b/templates/repo/diff/section_split.tmpl @@ -1,5 +1,5 @@ {{$file := .file}} -{{$blobExcerptLink := print (or ctx.RootData.CommitRepoLink ctx.RootData.RepoLink) (Iif $.root.PageIsWiki "/wiki" "") "/blob_excerpt/" (PathEscape $.root.AfterCommitID) "?"}} +{{$blobExcerptLink := print (or ctx.RootData.CommitRepoLink ctx.RootData.RepoLink) (Iif $.root.PageIsWiki "/wiki" "") "/blob_excerpt/" (PathEscape $.root.AfterCommitID) "?" (Iif $.root.PageIsPullFiles (print "is_pull=true&issue_index=" $.root.Issue.Index "&") "")}} @@ -20,18 +20,21 @@
{{if or (eq $expandDirection 3) (eq $expandDirection 5)}} - {{end}} {{if or (eq $expandDirection 3) (eq $expandDirection 4)}} - {{end}} {{if eq $expandDirection 2}} - {{end}}
diff --git a/templates/repo/diff/section_unified.tmpl b/templates/repo/diff/section_unified.tmpl index cb612bc27c4e8..3fe273738500a 100644 --- a/templates/repo/diff/section_unified.tmpl +++ b/templates/repo/diff/section_unified.tmpl @@ -1,7 +1,7 @@ {{$file := .file}} {{$repoLink := or ctx.RootData.CommitRepoLink ctx.RootData.RepoLink}} {{$afterCommitID := or $.root.AfterCommitID "no-after-commit-id"}}{{/* this tmpl is also used by the PR Conversation page, so the "AfterCommitID" may not exist */}} -{{$blobExcerptLink := print $repoLink (Iif $.root.PageIsWiki "/wiki" "") "/blob_excerpt/" (PathEscape $afterCommitID) "?"}} +{{$blobExcerptLink := print $repoLink (Iif $.root.PageIsWiki "/wiki" "") "/blob_excerpt/" (PathEscape $afterCommitID) "?" (Iif $.root.PageIsPullFiles (print "is_pull=true&issue_index=" $.root.Issue.Index "&") "")}} @@ -18,18 +18,21 @@
{{if or (eq $expandDirection 3) (eq $expandDirection 5)}} - {{end}} {{if or (eq $expandDirection 3) (eq $expandDirection 4)}} - {{end}} {{if eq $expandDirection 2}} - {{end}}
diff --git a/web_src/css/index.css b/web_src/css/index.css index 291cd04b2b95c..2b6fbd467a598 100644 --- a/web_src/css/index.css +++ b/web_src/css/index.css @@ -71,6 +71,7 @@ @import "./repo/clone.css"; @import "./repo/commit-sign.css"; @import "./repo/packages.css"; +@import "./repo/diff-hidden-comments.css"; @import "./editor/fileeditor.css"; @import "./editor/combomarkdowneditor.css"; diff --git a/web_src/css/repo/diff-hidden-comments.css b/web_src/css/repo/diff-hidden-comments.css new file mode 100644 index 0000000000000..c23f56bf55552 --- /dev/null +++ b/web_src/css/repo/diff-hidden-comments.css @@ -0,0 +1,38 @@ +/* Styling for expand buttons with hidden comments */ +.code-expander-button.has-hidden-comments { + position: relative; +} + +.code-expander-button .ui.mini.label { + position: absolute; + min-width: 16px; + height: 16px; + padding: 2px 4px; + font-size: 10px; + line-height: 12px; + background-color: var(--color-primary); + color: var(--color-primary-contrast); + border-radius: 8px; +} + +.code-expander-button:has(svg.octicon-fold-down) .ui.mini.label { + top: 50%; + left: -20px; + transform: translateY(-50%); +} + +.code-expander-button:has(svg.octicon-fold-up) .ui.mini.label { + top: 50%; + left: -20px; + transform: translateY(-50%); +} + +.code-expander-button:has(svg.octicon-fold) .ui.mini.label { + top: 50%; + left: -20px; + transform: translateY(-50%); +} + +.code-expander-button.has-hidden-comments:hover .ui.mini.label { + background-color: var(--color-primary-dark-1); +} From 1b9ac2cd3a343fa5c021dd9734182c6b9ca06eaf Mon Sep 17 00:00:00 2001 From: brymut Date: Thu, 16 Oct 2025 09:34:32 +0300 Subject: [PATCH 02/22] feat(gitdiff): Use generatePatchForUnchangedLine instead to generate context --- services/gitdiff/gitdiff.go | 102 ++++-------------------------------- services/pull/review.go | 77 +++++++++++++++++++++++++++ 2 files changed, 87 insertions(+), 92 deletions(-) diff --git a/services/gitdiff/gitdiff.go b/services/gitdiff/gitdiff.go index 52cfaf25ddffd..374c1e6a0b07b 100644 --- a/services/gitdiff/gitdiff.go +++ b/services/gitdiff/gitdiff.go @@ -1389,104 +1389,22 @@ outer: return review, nil } +// CommentAsDiff returns c.Patch as *Diff // CommentAsDiff returns c.Patch as *Diff func CommentAsDiff(ctx context.Context, c *issues_model.Comment) (*Diff, error) { - // Try to parse existing patch first - if c.Patch != "" { - diff, err := ParsePatch(ctx, setting.Git.MaxGitDiffLines, - setting.Git.MaxGitDiffLineCharacters, setting.Git.MaxGitDiffFiles, strings.NewReader(c.Patch), "") - if err == nil && len(diff.Files) > 0 && len(diff.Files[0].Sections) > 0 { - return diff, nil - } - } - - // If patch is empty or invalid, generate code context for unchanged line comments - if c.TreePath != "" && c.CommitSHA != "" && c.Line != 0 { - return generateCodeContextForComment(ctx, c) - } - - return nil, fmt.Errorf("no valid patch or context available for comment ID: %d", c.ID) -} - -func generateCodeContextForComment(ctx context.Context, c *issues_model.Comment) (*Diff, error) { - if err := c.LoadIssue(ctx); err != nil { - return nil, fmt.Errorf("LoadIssue: %w", err) - } - if err := c.Issue.LoadRepo(ctx); err != nil { - return nil, fmt.Errorf("LoadRepo: %w", err) - } - if err := c.Issue.LoadPullRequest(ctx); err != nil { - return nil, fmt.Errorf("LoadPullRequest: %w", err) - } - - pr := c.Issue.PullRequest - if err := pr.LoadBaseRepo(ctx); err != nil { - return nil, fmt.Errorf("LoadBaseRepo: %w", err) - } - - gitRepo, closer, err := gitrepo.RepositoryFromContextOrOpen(ctx, pr.BaseRepo) - if err != nil { - return nil, fmt.Errorf("RepositoryFromContextOrOpen: %w", err) - } - defer closer.Close() - - // Get the file content at the commit - commit, err := gitRepo.GetCommit(c.CommitSHA) - if err != nil { - return nil, fmt.Errorf("GetCommit: %w", err) - } - - entry, err := commit.GetTreeEntryByPath(c.TreePath) - if err != nil { - return nil, fmt.Errorf("GetTreeEntryByPath: %w", err) - } - - blob := entry.Blob() - dataRc, err := blob.DataAsync() + diff, err := ParsePatch(ctx, setting.Git.MaxGitDiffLines, + setting.Git.MaxGitDiffLineCharacters, setting.Git.MaxGitDiffFiles, strings.NewReader(c.Patch), "") if err != nil { - return nil, fmt.Errorf("DataAsync: %w", err) - } - defer dataRc.Close() - - // Read file lines - scanner := bufio.NewScanner(dataRc) - var lines []string - for scanner.Scan() { - lines = append(lines, scanner.Text()) - } - if err := scanner.Err(); err != nil { - return nil, fmt.Errorf("scanner error: %w", err) - } - - // Validate comment line is within file bounds - commentLine := int(c.UnsignedLine()) - if commentLine < 1 || commentLine > len(lines) { - return nil, fmt.Errorf("comment line %d is out of file bounds (1-%d)", commentLine, len(lines)) + log.Error("Unable to parse patch: %v", err) + return nil, err } - - // Calculate line range to show (commented line + lines above it, matching CutDiffAroundLine behavior) - contextLines := setting.UI.CodeCommentLines - startLine := max(commentLine-contextLines, 1) - endLine := commentLine - - var patchBuilder strings.Builder - patchBuilder.WriteString(fmt.Sprintf("diff --git a/%s b/%s\n", c.TreePath, c.TreePath)) - patchBuilder.WriteString(fmt.Sprintf("--- a/%s\n", c.TreePath)) - patchBuilder.WriteString(fmt.Sprintf("+++ b/%s\n", c.TreePath)) - patchBuilder.WriteString(fmt.Sprintf("@@ -%d,%d +%d,%d @@\n", startLine, endLine-startLine+1, startLine, endLine-startLine+1)) - - for i := startLine - 1; i < endLine; i++ { - patchBuilder.WriteString(" ") - patchBuilder.WriteString(lines[i]) - patchBuilder.WriteString("\n") + if len(diff.Files) == 0 { + return nil, fmt.Errorf("no file found for comment ID: %d", c.ID) } - - diff, err := ParsePatch(ctx, setting.Git.MaxGitDiffLines, - setting.Git.MaxGitDiffLineCharacters, setting.Git.MaxGitDiffFiles, strings.NewReader(patchBuilder.String()), "") - if err != nil { - return nil, fmt.Errorf("ParsePatch: %w", err) + secs := diff.Files[0].Sections + if len(secs) == 0 { + return nil, fmt.Errorf("no sections found for comment ID: %d", c.ID) } - return diff, nil } diff --git a/services/pull/review.go b/services/pull/review.go index 357a816593da0..615a01ccfdabf 100644 --- a/services/pull/review.go +++ b/services/pull/review.go @@ -5,6 +5,7 @@ package pull import ( + "bufio" "context" "errors" "fmt" @@ -197,6 +198,73 @@ func CreateCodeComment(ctx context.Context, doer *user_model.User, gitRepo *git. return comment, nil } +// generatePatchForUnchangedLine creates a patch showing code context for an unchanged line +func generatePatchForUnchangedLine(gitRepo *git.Repository, commitID, treePath string, line int64, contextLines int) (string, error) { + commit, err := gitRepo.GetCommit(commitID) + if err != nil { + return "", fmt.Errorf("GetCommit: %w", err) + } + + entry, err := commit.GetTreeEntryByPath(treePath) + if err != nil { + return "", fmt.Errorf("GetTreeEntryByPath: %w", err) + } + + blob := entry.Blob() + dataRc, err := blob.DataAsync() + if err != nil { + return "", fmt.Errorf("DataAsync: %w", err) + } + defer dataRc.Close() + + // Calculate line range (commented line + lines above it) + commentLine := int(line) + if line < 0 { + commentLine = int(-line) + } + startLine := commentLine - contextLines + if startLine < 1 { + startLine = 1 + } + endLine := commentLine + + // Read only the needed lines efficiently + scanner := bufio.NewScanner(dataRc) + currentLine := 0 + var lines []string + for scanner.Scan() { + currentLine++ + if currentLine >= startLine && currentLine <= endLine { + lines = append(lines, scanner.Text()) + } + if currentLine > endLine { + break + } + } + if err := scanner.Err(); err != nil { + return "", fmt.Errorf("scanner error: %w", err) + } + + if len(lines) == 0 { + return "", fmt.Errorf("no lines found in range %d-%d", startLine, endLine) + } + + // Generate synthetic patch + var patchBuilder strings.Builder + patchBuilder.WriteString(fmt.Sprintf("diff --git a/%s b/%s\n", treePath, treePath)) + patchBuilder.WriteString(fmt.Sprintf("--- a/%s\n", treePath)) + patchBuilder.WriteString(fmt.Sprintf("+++ b/%s\n", treePath)) + patchBuilder.WriteString(fmt.Sprintf("@@ -%d,%d +%d,%d @@\n", startLine, len(lines), startLine, len(lines))) + + for _, lineContent := range lines { + patchBuilder.WriteString(" ") + patchBuilder.WriteString(lineContent) + patchBuilder.WriteString("\n") + } + + return patchBuilder.String(), nil +} + // createCodeComment creates a plain code comment at the specified line / path func createCodeComment(ctx context.Context, doer *user_model.User, repo *repo_model.Repository, issue *issues_model.Issue, content, treePath string, line, reviewID int64, attachments []string) (*issues_model.Comment, error) { var commitID, patch string @@ -283,6 +351,15 @@ func createCodeComment(ctx context.Context, doer *user_model.User, repo *repo_mo log.Error("Error whilst generating patch: %v", err) return nil, err } + + // If patch is still empty (unchanged line), generate code context + if len(patch) == 0 && len(commitID) > 0 { + patch, err = generatePatchForUnchangedLine(gitRepo, commitID, treePath, line, setting.UI.CodeCommentLines) + if err != nil { + log.Warn("Error generating patch for unchanged line: %v", err) + // Don't fail comment creation, just leave patch empty + } + } } return issues_model.CreateComment(ctx, &issues_model.CreateCommentOptions{ Type: issues_model.CommentTypeCode, From 821600851d9a96abac339349d7144fe14f4baa03 Mon Sep 17 00:00:00 2001 From: brymut Date: Thu, 16 Oct 2025 09:38:26 +0300 Subject: [PATCH 03/22] fix(PR): Add permission check --- routers/web/repo/compare.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/routers/web/repo/compare.go b/routers/web/repo/compare.go index 3104301d54959..ea76fbe0cb4e5 100644 --- a/routers/web/repo/compare.go +++ b/routers/web/repo/compare.go @@ -956,6 +956,10 @@ func ExcerptBlob(ctx *context.Context) { ctx.Data["PageIsPullFiles"] = isPull if isPull { + if !ctx.Repo.CanRead(unit.TypePullRequests) { + ctx.NotFound(nil) + return + } issueIndex := ctx.FormInt64("issue_index") ctx.Data["IssueIndex"] = issueIndex if issueIndex > 0 { From 30e3fd19f7dce591d3b00bc34ccbfd7556ed2f92 Mon Sep 17 00:00:00 2001 From: brymut Date: Thu, 16 Oct 2025 12:09:16 +0300 Subject: [PATCH 04/22] fix(diff): update hidden comment count on code expand buttons --- routers/web/repo/compare.go | 22 ++++++++++++++++++++++ templates/repo/diff/blob_excerpt.tmpl | 18 ++++++++++++------ 2 files changed, 34 insertions(+), 6 deletions(-) diff --git a/routers/web/repo/compare.go b/routers/web/repo/compare.go index ea76fbe0cb4e5..48fb40b28af50 100644 --- a/routers/web/repo/compare.go +++ b/routers/web/repo/compare.go @@ -982,6 +982,28 @@ func ExcerptBlob(ctx *context.Context) { return line.Comments[i].CreatedUnix < line.Comments[j].CreatedUnix }) } + + // Recalculate hidden comment counts for expand buttons after loading comments + for _, line := range section.Lines { + if line.Type == gitdiff.DiffLineSection && line.SectionInfo != nil { + hiddenCommentCount := 0 + // Check if there are comments in the hidden range + for commentLineNum := range lineComments { + absLineNum := commentLineNum + if absLineNum < 0 { + absLineNum = -absLineNum + } + // Count comments that are in the hidden range + if int(absLineNum) > line.SectionInfo.LastRightIdx && int(absLineNum) < line.SectionInfo.RightIdx { + hiddenCommentCount++ + } + } + if hiddenCommentCount > 0 { + line.HasHiddenComments = true + line.HiddenCommentCount = hiddenCommentCount + } + } + } } } } diff --git a/templates/repo/diff/blob_excerpt.tmpl b/templates/repo/diff/blob_excerpt.tmpl index 5cc75b516c4c9..f824141f58ad5 100644 --- a/templates/repo/diff/blob_excerpt.tmpl +++ b/templates/repo/diff/blob_excerpt.tmpl @@ -7,18 +7,21 @@
{{if or (eq $expandDirection 3) (eq $expandDirection 5)}} - {{end}} {{if or (eq $expandDirection 3) (eq $expandDirection 4)}} - {{end}} {{if eq $expandDirection 2}} - {{end}}
@@ -84,18 +87,21 @@
{{if or (eq $expandDirection 3) (eq $expandDirection 5)}} - {{end}} {{if or (eq $expandDirection 3) (eq $expandDirection 4)}} - {{end}} {{if eq $expandDirection 2}} - {{end}}
From b234f31abf0fefc1c1f87a2a770b34c12b7aa111 Mon Sep 17 00:00:00 2001 From: brymut Date: Thu, 16 Oct 2025 17:28:47 +0300 Subject: [PATCH 05/22] feat(gitdiff): fix code context in conversation tab --- services/gitdiff/gitdiff.go | 99 ++++++++++++++++++++++++++++++++++++- services/pull/review.go | 5 +- 2 files changed, 98 insertions(+), 6 deletions(-) diff --git a/services/gitdiff/gitdiff.go b/services/gitdiff/gitdiff.go index 374c1e6a0b07b..f07ee27aa220f 100644 --- a/services/gitdiff/gitdiff.go +++ b/services/gitdiff/gitdiff.go @@ -1389,13 +1389,17 @@ outer: return review, nil } -// CommentAsDiff returns c.Patch as *Diff // CommentAsDiff returns c.Patch as *Diff func CommentAsDiff(ctx context.Context, c *issues_model.Comment) (*Diff, error) { + // If patch is empty, generate it + if c.Patch == "" && c.TreePath != "" && c.CommitSHA != "" && c.Line != 0 { + return generateCodeContextForComment(ctx, c) + } + diff, err := ParsePatch(ctx, setting.Git.MaxGitDiffLines, setting.Git.MaxGitDiffLineCharacters, setting.Git.MaxGitDiffFiles, strings.NewReader(c.Patch), "") if err != nil { - log.Error("Unable to parse patch: %v", err) + log.Error("Unable to parse patch for comment ID=%d: %v", c.ID, err) return nil, err } if len(diff.Files) == 0 { @@ -1408,6 +1412,97 @@ func CommentAsDiff(ctx context.Context, c *issues_model.Comment) (*Diff, error) return diff, nil } +// generateCodeContextForComment creates a synthetic diff showing code context around a comment +func generateCodeContextForComment(ctx context.Context, c *issues_model.Comment) (*Diff, error) { + if err := c.LoadIssue(ctx); err != nil { + return nil, fmt.Errorf("LoadIssue: %w", err) + } + if err := c.Issue.LoadRepo(ctx); err != nil { + return nil, fmt.Errorf("LoadRepo: %w", err) + } + if err := c.Issue.LoadPullRequest(ctx); err != nil { + return nil, fmt.Errorf("LoadPullRequest: %w", err) + } + + pr := c.Issue.PullRequest + if err := pr.LoadBaseRepo(ctx); err != nil { + return nil, fmt.Errorf("LoadBaseRepo: %w", err) + } + + gitRepo, closer, err := gitrepo.RepositoryFromContextOrOpen(ctx, pr.BaseRepo) + if err != nil { + return nil, fmt.Errorf("RepositoryFromContextOrOpen: %w", err) + } + defer closer.Close() + + // Get the file content at the commit + commit, err := gitRepo.GetCommit(c.CommitSHA) + if err != nil { + return nil, fmt.Errorf("GetCommit: %w", err) + } + + entry, err := commit.GetTreeEntryByPath(c.TreePath) + if err != nil { + return nil, fmt.Errorf("GetTreeEntryByPath: %w", err) + } + + blob := entry.Blob() + dataRc, err := blob.DataAsync() + if err != nil { + return nil, fmt.Errorf("DataAsync: %w", err) + } + defer dataRc.Close() + + // Calculate line range (commented line + lines above it) + commentLine := int(c.UnsignedLine()) + contextLines := setting.UI.CodeCommentLines + startLine := max(commentLine-contextLines, 1) + endLine := commentLine + + // Read only the needed lines efficiently + scanner := bufio.NewScanner(dataRc) + currentLine := 0 + var lines []string + for scanner.Scan() { + currentLine++ + if currentLine >= startLine && currentLine <= endLine { + lines = append(lines, scanner.Text()) + } + if currentLine > endLine { + break + } + } + if err := scanner.Err(); err != nil { + return nil, fmt.Errorf("scanner error: %w", err) + } + + if len(lines) == 0 { + return nil, fmt.Errorf("no lines found in range %d-%d", startLine, endLine) + } + + // Generate synthetic patch + var patchBuilder strings.Builder + patchBuilder.WriteString(fmt.Sprintf("diff --git a/%s b/%s\n", c.TreePath, c.TreePath)) + patchBuilder.WriteString(fmt.Sprintf("--- a/%s\n", c.TreePath)) + patchBuilder.WriteString(fmt.Sprintf("+++ b/%s\n", c.TreePath)) + patchBuilder.WriteString(fmt.Sprintf("@@ -%d,%d +%d,%d @@\n", startLine, len(lines), startLine, len(lines))) + + for _, lineContent := range lines { + patchBuilder.WriteString(" ") + patchBuilder.WriteString(lineContent) + patchBuilder.WriteString("\n") + } + + // Parse the synthetic patch + diff, err := ParsePatch(ctx, setting.Git.MaxGitDiffLines, + setting.Git.MaxGitDiffLineCharacters, setting.Git.MaxGitDiffFiles, strings.NewReader(patchBuilder.String()), "") + if err != nil { + return nil, fmt.Errorf("ParsePatch: %w", err) + } + + return diff, nil +} + // CommentMustAsDiff executes AsDiff and logs the error instead of returning func CommentMustAsDiff(ctx context.Context, c *issues_model.Comment) *Diff { if c == nil { diff --git a/services/pull/review.go b/services/pull/review.go index 615a01ccfdabf..3a039084b531f 100644 --- a/services/pull/review.go +++ b/services/pull/review.go @@ -222,10 +222,7 @@ func generatePatchForUnchangedLine(gitRepo *git.Repository, commitID, treePath s if line < 0 { commentLine = int(-line) } - startLine := commentLine - contextLines - if startLine < 1 { - startLine = 1 - } + startLine := max(commentLine-contextLines, 1) endLine := commentLine // Read only the needed lines efficiently From 4b85c501b1a2151a6fb79a970d07060d9991522e Mon Sep 17 00:00:00 2001 From: brymut Date: Fri, 17 Oct 2025 13:27:18 +0300 Subject: [PATCH 06/22] fix(gitdiff): remove patching logic duplication --- services/gitdiff/gitdiff.go | 68 +++++++++------------------------- services/pull/review.go | 73 +++---------------------------------- 2 files changed, 23 insertions(+), 118 deletions(-) diff --git a/services/gitdiff/gitdiff.go b/services/gitdiff/gitdiff.go index f07ee27aa220f..5c4e3e8c39f41 100644 --- a/services/gitdiff/gitdiff.go +++ b/services/gitdiff/gitdiff.go @@ -1391,15 +1391,10 @@ outer: // CommentAsDiff returns c.Patch as *Diff func CommentAsDiff(ctx context.Context, c *issues_model.Comment) (*Diff, error) { - // If patch is empty, generate it - if c.Patch == "" && c.TreePath != "" && c.CommitSHA != "" && c.Line != 0 { - return generateCodeContextForComment(ctx, c) - } - diff, err := ParsePatch(ctx, setting.Git.MaxGitDiffLines, setting.Git.MaxGitDiffLineCharacters, setting.Git.MaxGitDiffFiles, strings.NewReader(c.Patch), "") if err != nil { - log.Error("Unable to parse patch for comment ID=%d: %v", c.ID, err) + log.Error("Unable to parse patch: %v", err) return nil, err } if len(diff.Files) == 0 { @@ -1412,50 +1407,30 @@ func CommentAsDiff(ctx context.Context, c *issues_model.Comment) (*Diff, error) return diff, nil } -// generateCodeContextForComment creates a synthetic diff showing code context around a comment -func generateCodeContextForComment(ctx context.Context, c *issues_model.Comment) (*Diff, error) { - if err := c.LoadIssue(ctx); err != nil { - return nil, fmt.Errorf("LoadIssue: %w", err) - } - if err := c.Issue.LoadRepo(ctx); err != nil { - return nil, fmt.Errorf("LoadRepo: %w", err) - } - if err := c.Issue.LoadPullRequest(ctx); err != nil { - return nil, fmt.Errorf("LoadPullRequest: %w", err) - } - - pr := c.Issue.PullRequest - if err := pr.LoadBaseRepo(ctx); err != nil { - return nil, fmt.Errorf("LoadBaseRepo: %w", err) - } - - gitRepo, closer, err := gitrepo.RepositoryFromContextOrOpen(ctx, pr.BaseRepo) +// GeneratePatchForUnchangedLine creates a patch showing code context for an unchanged line +func GeneratePatchForUnchangedLine(gitRepo *git.Repository, commitID, treePath string, line int64, contextLines int) (string, error) { + commit, err := gitRepo.GetCommit(commitID) if err != nil { - return nil, fmt.Errorf("RepositoryFromContextOrOpen: %w", err) + return "", fmt.Errorf("GetCommit: %w", err) } - defer closer.Close() - // Get the file content at the commit - commit, err := gitRepo.GetCommit(c.CommitSHA) + entry, err := commit.GetTreeEntryByPath(treePath) if err != nil { - return nil, fmt.Errorf("GetCommit: %w", err) - } - - entry, err := commit.GetTreeEntryByPath(c.TreePath) - if err != nil { - return nil, fmt.Errorf("GetTreeEntryByPath: %w", err) + return "", fmt.Errorf("GetTreeEntryByPath: %w", err) } blob := entry.Blob() dataRc, err := blob.DataAsync() if err != nil { - return nil, fmt.Errorf("DataAsync: %w", err) + return "", fmt.Errorf("DataAsync: %w", err) } defer dataRc.Close() // Calculate line range (commented line + lines above it) - commentLine := int(c.UnsignedLine()) - contextLines := setting.UI.CodeCommentLines + commentLine := int(line) + if line < 0 { + commentLine = int(-line) + } startLine := max(commentLine-contextLines, 1) endLine := commentLine @@ -1473,18 +1448,18 @@ func generateCodeContextForComment(ctx context.Context, c *issues_model.Comment) } } if err := scanner.Err(); err != nil { - return nil, fmt.Errorf("scanner error: %w", err) + return "", fmt.Errorf("scanner error: %w", err) } if len(lines) == 0 { - return nil, fmt.Errorf("no lines found in range %d-%d", startLine, endLine) + return "", fmt.Errorf("no lines found in range %d-%d", startLine, endLine) } // Generate synthetic patch var patchBuilder strings.Builder - patchBuilder.WriteString(fmt.Sprintf("diff --git a/%s b/%s\n", c.TreePath, c.TreePath)) - patchBuilder.WriteString(fmt.Sprintf("--- a/%s\n", c.TreePath)) - patchBuilder.WriteString(fmt.Sprintf("+++ b/%s\n", c.TreePath)) + patchBuilder.WriteString(fmt.Sprintf("diff --git a/%s b/%s\n", treePath, treePath)) + patchBuilder.WriteString(fmt.Sprintf("--- a/%s\n", treePath)) + patchBuilder.WriteString(fmt.Sprintf("+++ b/%s\n", treePath)) patchBuilder.WriteString(fmt.Sprintf("@@ -%d,%d +%d,%d @@\n", startLine, len(lines), startLine, len(lines))) for _, lineContent := range lines { @@ -1493,14 +1468,7 @@ func generateCodeContextForComment(ctx context.Context, c *issues_model.Comment) patchBuilder.WriteString("\n") } - // Parse the synthetic patch - diff, err := ParsePatch(ctx, setting.Git.MaxGitDiffLines, - setting.Git.MaxGitDiffLineCharacters, setting.Git.MaxGitDiffFiles, strings.NewReader(patchBuilder.String()), "") - if err != nil { - return nil, fmt.Errorf("ParsePatch: %w", err) - } - - return diff, nil + return patchBuilder.String(), nil } // CommentMustAsDiff executes AsDiff and logs the error instead of returning diff --git a/services/pull/review.go b/services/pull/review.go index 3a039084b531f..808d84740f35e 100644 --- a/services/pull/review.go +++ b/services/pull/review.go @@ -5,7 +5,6 @@ package pull import ( - "bufio" "context" "errors" "fmt" @@ -23,6 +22,7 @@ import ( "code.gitea.io/gitea/modules/optional" "code.gitea.io/gitea/modules/setting" "code.gitea.io/gitea/modules/util" + "code.gitea.io/gitea/services/gitdiff" notify_service "code.gitea.io/gitea/services/notify" ) @@ -198,70 +198,6 @@ func CreateCodeComment(ctx context.Context, doer *user_model.User, gitRepo *git. return comment, nil } -// generatePatchForUnchangedLine creates a patch showing code context for an unchanged line -func generatePatchForUnchangedLine(gitRepo *git.Repository, commitID, treePath string, line int64, contextLines int) (string, error) { - commit, err := gitRepo.GetCommit(commitID) - if err != nil { - return "", fmt.Errorf("GetCommit: %w", err) - } - - entry, err := commit.GetTreeEntryByPath(treePath) - if err != nil { - return "", fmt.Errorf("GetTreeEntryByPath: %w", err) - } - - blob := entry.Blob() - dataRc, err := blob.DataAsync() - if err != nil { - return "", fmt.Errorf("DataAsync: %w", err) - } - defer dataRc.Close() - - // Calculate line range (commented line + lines above it) - commentLine := int(line) - if line < 0 { - commentLine = int(-line) - } - startLine := max(commentLine-contextLines, 1) - endLine := commentLine - - // Read only the needed lines efficiently - scanner := bufio.NewScanner(dataRc) - currentLine := 0 - var lines []string - for scanner.Scan() { - currentLine++ - if currentLine >= startLine && currentLine <= endLine { - lines = append(lines, scanner.Text()) - } - if currentLine > endLine { - break - } - } - if err := scanner.Err(); err != nil { - return "", fmt.Errorf("scanner error: %w", err) - } - - if len(lines) == 0 { - return "", fmt.Errorf("no lines found in range %d-%d", startLine, endLine) - } - - // Generate synthetic patch - var patchBuilder strings.Builder - patchBuilder.WriteString(fmt.Sprintf("diff --git a/%s b/%s\n", treePath, treePath)) - patchBuilder.WriteString(fmt.Sprintf("--- a/%s\n", treePath)) - patchBuilder.WriteString(fmt.Sprintf("+++ b/%s\n", treePath)) - patchBuilder.WriteString(fmt.Sprintf("@@ -%d,%d +%d,%d @@\n", startLine, len(lines), startLine, len(lines))) - - for _, lineContent := range lines { - patchBuilder.WriteString(" ") - patchBuilder.WriteString(lineContent) - patchBuilder.WriteString("\n") - } - - return patchBuilder.String(), nil -} - // createCodeComment creates a plain code comment at the specified line / path func createCodeComment(ctx context.Context, doer *user_model.User, repo *repo_model.Repository, issue *issues_model.Issue, content, treePath string, line, reviewID int64, attachments []string) (*issues_model.Comment, error) { var commitID, patch string @@ -351,10 +287,11 @@ func createCodeComment(ctx context.Context, doer *user_model.User, repo *repo_mo // If patch is still empty (unchanged line), generate code context if len(patch) == 0 && len(commitID) > 0 { - patch, err = generatePatchForUnchangedLine(gitRepo, commitID, treePath, line, setting.UI.CodeCommentLines) + patch, err = gitdiff.GeneratePatchForUnchangedLine(gitRepo, commitID, treePath, line, setting.UI.CodeCommentLines) if err != nil { - log.Warn("Error generating patch for unchanged line: %v", err) - // Don't fail comment creation, just leave patch empty + // Log the error but don't fail comment creation + // This can happen for deleted/renamed files or other edge cases + log.Warn("Unable to generate patch for unchanged line (file=%s, line=%d, commit=%s): %v", treePath, line, commitID, err) } } } From 2c33c7d947e68bf96302483bd0bc797d4b821bf2 Mon Sep 17 00:00:00 2001 From: brymut Date: Sat, 18 Oct 2025 06:22:44 +0300 Subject: [PATCH 07/22] feat(diff): Enhance hidden comment expansion when sharing link --- routers/web/repo/compare.go | 8 ++++++- services/gitdiff/gitdiff.go | 13 ++++++++--- templates/repo/diff/blob_excerpt.tmpl | 8 +++---- templates/repo/diff/section_unified.tmpl | 6 ++--- web_src/js/features/repo-diff.ts | 29 ++++++++++++++++++++++++ 5 files changed, 53 insertions(+), 11 deletions(-) diff --git a/routers/web/repo/compare.go b/routers/web/repo/compare.go index 48fb40b28af50..c8e2bc2cadbfc 100644 --- a/routers/web/repo/compare.go +++ b/routers/web/repo/compare.go @@ -987,8 +987,9 @@ func ExcerptBlob(ctx *context.Context) { for _, line := range section.Lines { if line.Type == gitdiff.DiffLineSection && line.SectionInfo != nil { hiddenCommentCount := 0 + var hiddenCommentIDs []int64 // Check if there are comments in the hidden range - for commentLineNum := range lineComments { + for commentLineNum, comments := range lineComments { absLineNum := commentLineNum if absLineNum < 0 { absLineNum = -absLineNum @@ -996,11 +997,16 @@ func ExcerptBlob(ctx *context.Context) { // Count comments that are in the hidden range if int(absLineNum) > line.SectionInfo.LastRightIdx && int(absLineNum) < line.SectionInfo.RightIdx { hiddenCommentCount++ + // Collect comment IDs + for _, comment := range comments { + hiddenCommentIDs = append(hiddenCommentIDs, comment.ID) + } } } if hiddenCommentCount > 0 { line.HasHiddenComments = true line.HiddenCommentCount = hiddenCommentCount + line.HiddenCommentIDs = hiddenCommentIDs } } } diff --git a/services/gitdiff/gitdiff.go b/services/gitdiff/gitdiff.go index 5c4e3e8c39f41..3e54831b2cb05 100644 --- a/services/gitdiff/gitdiff.go +++ b/services/gitdiff/gitdiff.go @@ -88,8 +88,9 @@ type DiffLine struct { Content string Comments issues_model.CommentList // related PR code comments SectionInfo *DiffLineSectionInfo - HasHiddenComments bool // indicates if this expand button has comments in hidden lines - HiddenCommentCount int // number of hidden comments in this section + HasHiddenComments bool // indicates if this expand button has comments in hidden lines + HiddenCommentCount int // number of hidden comments in this section + HiddenCommentIDs []int64 // IDs of hidden comments in this section } // DiffLineSectionInfo represents diff line section meta data @@ -491,19 +492,25 @@ func (diff *Diff) LoadComments(ctx context.Context, issue *issues_model.Issue, c // Mark expand buttons that have comments in hidden lines if line.Type == DiffLineSection && line.SectionInfo != nil { hiddenCommentCount := 0 + var hiddenCommentIDs []int64 // Check if there are comments in the hidden range - for commentLineNum := range lineCommits { + for commentLineNum, comments := range lineCommits { absLineNum := int(commentLineNum) if commentLineNum < 0 { absLineNum = int(-commentLineNum) } if absLineNum > line.SectionInfo.LastRightIdx && absLineNum < line.SectionInfo.RightIdx { hiddenCommentCount++ + // Collect comment IDs + for _, comment := range comments { + hiddenCommentIDs = append(hiddenCommentIDs, comment.ID) + } } } if hiddenCommentCount > 0 { line.HasHiddenComments = true line.HiddenCommentCount = hiddenCommentCount + line.HiddenCommentIDs = hiddenCommentIDs } } } diff --git a/templates/repo/diff/blob_excerpt.tmpl b/templates/repo/diff/blob_excerpt.tmpl index f824141f58ad5..abcc768a7e5a9 100644 --- a/templates/repo/diff/blob_excerpt.tmpl +++ b/templates/repo/diff/blob_excerpt.tmpl @@ -1,4 +1,4 @@ -{{$blobExcerptLink := print $.RepoLink (Iif $.PageIsWiki "/wiki" "") "/blob_excerpt/" (PathEscape $.AfterCommitID) "?" (Iif $.PageIsPullFiles (print "is_pull=true&issue_index=" $.IssueIndex "&") "")}} +{{$blobExcerptLink := print $.RepoLink (Iif $.PageIsWiki "/wiki" "") "/blob_excerpt/" (PathEscape $.AfterCommitID) (QueryBuild "?" "anchor" $.Anchor) (Iif $.PageIsPullFiles (print "&is_pull=true&issue_index=" $.IssueIndex) "")}} {{if $.IsSplitStyle}} {{range $k, $line := $.section.Lines}} @@ -87,19 +87,19 @@
{{if or (eq $expandDirection 3) (eq $expandDirection 5)}} - {{end}} {{if or (eq $expandDirection 3) (eq $expandDirection 4)}} - {{end}} {{if eq $expandDirection 2}} - diff --git a/templates/repo/diff/section_unified.tmpl b/templates/repo/diff/section_unified.tmpl index 3fe273738500a..f5928b7c2d41d 100644 --- a/templates/repo/diff/section_unified.tmpl +++ b/templates/repo/diff/section_unified.tmpl @@ -18,19 +18,19 @@
{{if or (eq $expandDirection 3) (eq $expandDirection 5)}} - {{end}} {{if or (eq $expandDirection 3) (eq $expandDirection 4)}} - {{end}} {{if eq $expandDirection 2}} - diff --git a/web_src/js/features/repo-diff.ts b/web_src/js/features/repo-diff.ts index bde7ec0324b5c..ea0f23aa1f355 100644 --- a/web_src/js/features/repo-diff.ts +++ b/web_src/js/features/repo-diff.ts @@ -233,6 +233,18 @@ async function loadUntilFound() { return; } + // If looking for a comment, try to expand the section that contains it + if (hashTargetSelector.startsWith('#issuecomment-')) { + const commentId = hashTargetSelector.substring('#issuecomment-'.length); + const expandButton = findExpandButtonForComment(commentId); + if (expandButton) { + expandButton.click(); + // Wait for HTMX to load the content + await new Promise((resolve) => setTimeout(resolve, 500)); + continue; // Try again to find the element + } + } + // the button will be refreshed after each "load more", so query it every time const showMoreButton = document.querySelector('#diff-show-more-files'); if (!showMoreButton) { @@ -245,6 +257,23 @@ async function loadUntilFound() { } } +// Find the expand button that contains the target comment ID +function findExpandButtonForComment(commentId: string): HTMLElement | null { + const expandButtons = document.querySelectorAll('.code-expander-button[data-hidden-comment-ids]'); + for (const button of expandButtons) { + const hiddenIds = button.getAttribute('data-hidden-comment-ids'); + if (hiddenIds) { + // Parse the comma-separated list of IDs + const ids = hiddenIds.replace(/[[\]]/g, '').split(' ').filter(Boolean); + if (ids.includes(commentId)) { + return button as HTMLElement; + } + } + } + + return null; +} + function initRepoDiffHashChangeListener() { window.addEventListener('hashchange', loadUntilFound); loadUntilFound(); From 19ed046f8ba3cb9aa788bfa0548c4601062670a3 Mon Sep 17 00:00:00 2001 From: brymut Date: Sat, 18 Oct 2025 08:34:10 +0300 Subject: [PATCH 08/22] fix: highlighting to shared hidden comments --- routers/web/repo/compare.go | 6 +----- services/gitdiff/gitdiff.go | 24 +++++++++--------------- templates/repo/diff/blob_excerpt.tmpl | 24 ++++++++++++------------ templates/repo/diff/section_split.tmpl | 12 ++++++------ templates/repo/diff/section_unified.tmpl | 12 ++++++------ web_src/js/features/repo-diff.ts | 14 +++++++++++++- 6 files changed, 47 insertions(+), 45 deletions(-) diff --git a/routers/web/repo/compare.go b/routers/web/repo/compare.go index c8e2bc2cadbfc..21e5c07af1269 100644 --- a/routers/web/repo/compare.go +++ b/routers/web/repo/compare.go @@ -986,7 +986,6 @@ func ExcerptBlob(ctx *context.Context) { // Recalculate hidden comment counts for expand buttons after loading comments for _, line := range section.Lines { if line.Type == gitdiff.DiffLineSection && line.SectionInfo != nil { - hiddenCommentCount := 0 var hiddenCommentIDs []int64 // Check if there are comments in the hidden range for commentLineNum, comments := range lineComments { @@ -996,16 +995,13 @@ func ExcerptBlob(ctx *context.Context) { } // Count comments that are in the hidden range if int(absLineNum) > line.SectionInfo.LastRightIdx && int(absLineNum) < line.SectionInfo.RightIdx { - hiddenCommentCount++ // Collect comment IDs for _, comment := range comments { hiddenCommentIDs = append(hiddenCommentIDs, comment.ID) } } } - if hiddenCommentCount > 0 { - line.HasHiddenComments = true - line.HiddenCommentCount = hiddenCommentCount + if len(hiddenCommentIDs) > 0 { line.HiddenCommentIDs = hiddenCommentIDs } } diff --git a/services/gitdiff/gitdiff.go b/services/gitdiff/gitdiff.go index 3e54831b2cb05..62ed23cc1aa2c 100644 --- a/services/gitdiff/gitdiff.go +++ b/services/gitdiff/gitdiff.go @@ -81,16 +81,14 @@ const ( // DiffLine represents a line difference in a DiffSection. type DiffLine struct { - LeftIdx int // line number, 1-based - RightIdx int // line number, 1-based - Match int // the diff matched index. -1: no match. 0: plain and no need to match. >0: for add/del, "Lines" slice index of the other side - Type DiffLineType - Content string - Comments issues_model.CommentList // related PR code comments - SectionInfo *DiffLineSectionInfo - HasHiddenComments bool // indicates if this expand button has comments in hidden lines - HiddenCommentCount int // number of hidden comments in this section - HiddenCommentIDs []int64 // IDs of hidden comments in this section + LeftIdx int // line number, 1-based + RightIdx int // line number, 1-based + Match int // the diff matched index. -1: no match. 0: plain and no need to match. >0: for add/del, "Lines" slice index of the other side + Type DiffLineType + Content string + Comments issues_model.CommentList // related PR code comments + SectionInfo *DiffLineSectionInfo + HiddenCommentIDs []int64 // IDs of hidden comments in this section } // DiffLineSectionInfo represents diff line section meta data @@ -491,7 +489,6 @@ func (diff *Diff) LoadComments(ctx context.Context, issue *issues_model.Issue, c // Mark expand buttons that have comments in hidden lines if line.Type == DiffLineSection && line.SectionInfo != nil { - hiddenCommentCount := 0 var hiddenCommentIDs []int64 // Check if there are comments in the hidden range for commentLineNum, comments := range lineCommits { @@ -500,16 +497,13 @@ func (diff *Diff) LoadComments(ctx context.Context, issue *issues_model.Issue, c absLineNum = int(-commentLineNum) } if absLineNum > line.SectionInfo.LastRightIdx && absLineNum < line.SectionInfo.RightIdx { - hiddenCommentCount++ // Collect comment IDs for _, comment := range comments { hiddenCommentIDs = append(hiddenCommentIDs, comment.ID) } } } - if hiddenCommentCount > 0 { - line.HasHiddenComments = true - line.HiddenCommentCount = hiddenCommentCount + if len(hiddenCommentIDs) > 0 { line.HiddenCommentIDs = hiddenCommentIDs } } diff --git a/templates/repo/diff/blob_excerpt.tmpl b/templates/repo/diff/blob_excerpt.tmpl index abcc768a7e5a9..23ca71e05407d 100644 --- a/templates/repo/diff/blob_excerpt.tmpl +++ b/templates/repo/diff/blob_excerpt.tmpl @@ -7,21 +7,21 @@
{{if or (eq $expandDirection 3) (eq $expandDirection 5)}} - {{end}} {{if or (eq $expandDirection 3) (eq $expandDirection 4)}} - {{end}} {{if eq $expandDirection 2}} - {{end}}
@@ -87,21 +87,21 @@
{{if or (eq $expandDirection 3) (eq $expandDirection 5)}} - {{end}} {{if or (eq $expandDirection 3) (eq $expandDirection 4)}} - {{end}} {{if eq $expandDirection 2}} - {{end}}
diff --git a/templates/repo/diff/section_split.tmpl b/templates/repo/diff/section_split.tmpl index 75c7f7c059c0c..6f30889bd7d95 100644 --- a/templates/repo/diff/section_split.tmpl +++ b/templates/repo/diff/section_split.tmpl @@ -20,21 +20,21 @@
{{if or (eq $expandDirection 3) (eq $expandDirection 5)}} - {{end}} {{if or (eq $expandDirection 3) (eq $expandDirection 4)}} - {{end}} {{if eq $expandDirection 2}} - {{end}}
diff --git a/templates/repo/diff/section_unified.tmpl b/templates/repo/diff/section_unified.tmpl index f5928b7c2d41d..290259a957e3f 100644 --- a/templates/repo/diff/section_unified.tmpl +++ b/templates/repo/diff/section_unified.tmpl @@ -18,21 +18,21 @@
{{if or (eq $expandDirection 3) (eq $expandDirection 5)}} - {{end}} {{if or (eq $expandDirection 3) (eq $expandDirection 4)}} - {{end}} {{if eq $expandDirection 2}} - {{end}}
diff --git a/web_src/js/features/repo-diff.ts b/web_src/js/features/repo-diff.ts index ea0f23aa1f355..1080e8665a99f 100644 --- a/web_src/js/features/repo-diff.ts +++ b/web_src/js/features/repo-diff.ts @@ -224,12 +224,23 @@ async function loadUntilFound() { return; } + let wasExpanded = false; + while (true) { // use getElementById to avoid querySelector throws an error when the hash is invalid // eslint-disable-next-line unicorn/prefer-query-selector const targetElement = document.getElementById(hashTargetSelector.substring(1)); if (targetElement) { - targetElement.scrollIntoView(); + // If we just expanded content, we need to re-trigger :target CSS + if (wasExpanded) { + const currentHash = window.location.hash; + window.location.hash = ''; + setTimeout(() => { + window.location.hash = currentHash; + }, 10); + } else { + targetElement.scrollIntoView(); + } return; } @@ -239,6 +250,7 @@ async function loadUntilFound() { const expandButton = findExpandButtonForComment(commentId); if (expandButton) { expandButton.click(); + wasExpanded = true; // Wait for HTMX to load the content await new Promise((resolve) => setTimeout(resolve, 500)); continue; // Try again to find the element From 3e0c892e9d6fdd5658cddd603108359868841302 Mon Sep 17 00:00:00 2001 From: brymut Date: Sat, 18 Oct 2025 12:27:55 +0300 Subject: [PATCH 09/22] test(diff): add tests for helper functions in ExcerptBlob --- routers/web/repo/compare.go | 83 +++++++++++--------- routers/web/repo/compare_test.go | 127 +++++++++++++++++++++++++++++++ 2 files changed, 175 insertions(+), 35 deletions(-) create mode 100644 routers/web/repo/compare_test.go diff --git a/routers/web/repo/compare.go b/routers/web/repo/compare.go index 21e5c07af1269..4409409ac5624 100644 --- a/routers/web/repo/compare.go +++ b/routers/web/repo/compare.go @@ -867,6 +867,52 @@ func CompareDiff(ctx *context.Context) { ctx.HTML(http.StatusOK, tplCompare) } +// attachCommentsToLines attaches comments to their corresponding diff lines +func attachCommentsToLines(section *gitdiff.DiffSection, lineComments map[int64][]*issues_model.Comment) { + for _, line := range section.Lines { + if comments, ok := lineComments[int64(line.LeftIdx*-1)]; ok { + line.Comments = append(line.Comments, comments...) + } + if comments, ok := lineComments[int64(line.RightIdx)]; ok { + line.Comments = append(line.Comments, comments...) + } + sort.SliceStable(line.Comments, func(i, j int) bool { + return line.Comments[i].CreatedUnix < line.Comments[j].CreatedUnix + }) + } +} + +// calculateHiddenCommentIDs finds comment IDs that are in the hidden range of an expand button +func calculateHiddenCommentIDs(line *gitdiff.DiffLine, lineComments map[int64][]*issues_model.Comment) []int64 { + if line.Type != gitdiff.DiffLineSection || line.SectionInfo == nil { + return nil + } + + var hiddenCommentIDs []int64 + for commentLineNum, comments := range lineComments { + absLineNum := commentLineNum + if absLineNum < 0 { + absLineNum = -absLineNum + } + // Check if comments are in the hidden range + if int(absLineNum) > line.SectionInfo.LastRightIdx && int(absLineNum) < line.SectionInfo.RightIdx { + for _, comment := range comments { + hiddenCommentIDs = append(hiddenCommentIDs, comment.ID) + } + } + } + return hiddenCommentIDs +} + +// attachHiddenCommentIDs calculates and attaches hidden comment IDs to expand buttons +func attachHiddenCommentIDs(section *gitdiff.DiffSection, lineComments map[int64][]*issues_model.Comment) { + for _, line := range section.Lines { + if hiddenIDs := calculateHiddenCommentIDs(line, lineComments); len(hiddenIDs) > 0 { + line.HiddenCommentIDs = hiddenIDs + } + } +} + // ExcerptBlob render blob excerpt contents func ExcerptBlob(ctx *context.Context) { commitID := ctx.PathParam("sha") @@ -971,41 +1017,8 @@ func ExcerptBlob(ctx *context.Context) { if allComments, err := issues_model.FetchCodeComments(ctx, issue, ctx.Doer, ctx.FormBool("show_outdated")); err == nil { if lineComments, ok := allComments[filePath]; ok { - for _, line := range section.Lines { - if comments, ok := lineComments[int64(line.LeftIdx*-1)]; ok { - line.Comments = append(line.Comments, comments...) - } - if comments, ok := lineComments[int64(line.RightIdx)]; ok { - line.Comments = append(line.Comments, comments...) - } - sort.SliceStable(line.Comments, func(i, j int) bool { - return line.Comments[i].CreatedUnix < line.Comments[j].CreatedUnix - }) - } - - // Recalculate hidden comment counts for expand buttons after loading comments - for _, line := range section.Lines { - if line.Type == gitdiff.DiffLineSection && line.SectionInfo != nil { - var hiddenCommentIDs []int64 - // Check if there are comments in the hidden range - for commentLineNum, comments := range lineComments { - absLineNum := commentLineNum - if absLineNum < 0 { - absLineNum = -absLineNum - } - // Count comments that are in the hidden range - if int(absLineNum) > line.SectionInfo.LastRightIdx && int(absLineNum) < line.SectionInfo.RightIdx { - // Collect comment IDs - for _, comment := range comments { - hiddenCommentIDs = append(hiddenCommentIDs, comment.ID) - } - } - } - if len(hiddenCommentIDs) > 0 { - line.HiddenCommentIDs = hiddenCommentIDs - } - } - } + attachCommentsToLines(section, lineComments) + attachHiddenCommentIDs(section, lineComments) } } } diff --git a/routers/web/repo/compare_test.go b/routers/web/repo/compare_test.go new file mode 100644 index 0000000000000..e4a90ddcba890 --- /dev/null +++ b/routers/web/repo/compare_test.go @@ -0,0 +1,127 @@ +// Copyright 2025 The Gitea Authors. All rights reserved. +// SPDX-License-Identifier: MIT + +package repo + +import ( + "testing" + + issues_model "code.gitea.io/gitea/models/issues" + "code.gitea.io/gitea/services/gitdiff" + + "github.com/stretchr/testify/assert" +) + +func TestCalculateHiddenCommentIDs(t *testing.T) { + tests := []struct { + name string + line *gitdiff.DiffLine + lineComments map[int64][]*issues_model.Comment + expected []int64 + }{ + { + name: "comments in hidden range", + line: &gitdiff.DiffLine{ + Type: gitdiff.DiffLineSection, + SectionInfo: &gitdiff.DiffLineSectionInfo{ + LastRightIdx: 10, + RightIdx: 20, + }, + }, + lineComments: map[int64][]*issues_model.Comment{ + 15: {{ID: 100}, {ID: 101}}, + 12: {{ID: 102}}, + }, + expected: []int64{100, 101, 102}, + }, + { + name: "comments outside hidden range", + line: &gitdiff.DiffLine{ + Type: gitdiff.DiffLineSection, + SectionInfo: &gitdiff.DiffLineSectionInfo{ + LastRightIdx: 10, + RightIdx: 20, + }, + }, + lineComments: map[int64][]*issues_model.Comment{ + 5: {{ID: 100}}, + 25: {{ID: 101}}, + }, + expected: nil, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result := calculateHiddenCommentIDs(tt.line, tt.lineComments) + if tt.expected == nil { + assert.Nil(t, result) + } else { + assert.ElementsMatch(t, tt.expected, result) + } + }) + } +} + +func TestAttachHiddenCommentIDs(t *testing.T) { + section := &gitdiff.DiffSection{ + Lines: []*gitdiff.DiffLine{ + { + Type: gitdiff.DiffLineSection, + SectionInfo: &gitdiff.DiffLineSectionInfo{ + LastRightIdx: 10, + RightIdx: 20, + }, + }, + { + Type: gitdiff.DiffLinePlain, + }, + { + Type: gitdiff.DiffLineSection, + SectionInfo: &gitdiff.DiffLineSectionInfo{ + LastRightIdx: 30, + RightIdx: 40, + }, + }, + }, + } + + lineComments := map[int64][]*issues_model.Comment{ + 15: {{ID: 100}}, // in first section's hidden range + 35: {{ID: 200}}, // in second section's hidden range + 50: {{ID: 300}}, // outside any range + } + + attachHiddenCommentIDs(section, lineComments) + + assert.Equal(t, []int64{100}, section.Lines[0].HiddenCommentIDs) + assert.Nil(t, section.Lines[1].HiddenCommentIDs) + assert.Equal(t, []int64{200}, section.Lines[2].HiddenCommentIDs) +} + +func TestAttachCommentsToLines(t *testing.T) { + section := &gitdiff.DiffSection{ + Lines: []*gitdiff.DiffLine{ + {LeftIdx: 5, RightIdx: 10}, + {LeftIdx: 6, RightIdx: 11}, + }, + } + + lineComments := map[int64][]*issues_model.Comment{ + -5: {{ID: 100, CreatedUnix: 1000}}, // left side comment + 10: {{ID: 200, CreatedUnix: 2000}}, // right side comment + 11: {{ID: 300, CreatedUnix: 1500}, {ID: 301, CreatedUnix: 2500}}, // multiple comments + } + + attachCommentsToLines(section, lineComments) + + // First line should have left and right comments + assert.Len(t, section.Lines[0].Comments, 2) + assert.Equal(t, int64(100), section.Lines[0].Comments[0].ID) + assert.Equal(t, int64(200), section.Lines[0].Comments[1].ID) + + // Second line should have two comments, sorted by creation time + assert.Len(t, section.Lines[1].Comments, 2) + assert.Equal(t, int64(300), section.Lines[1].Comments[0].ID) + assert.Equal(t, int64(301), section.Lines[1].Comments[1].ID) +} From aafa2315b6d6063bfa1426382422c07a52768913 Mon Sep 17 00:00:00 2001 From: brymut Date: Sat, 18 Oct 2025 14:37:25 +0300 Subject: [PATCH 10/22] refactor(gitdiff): Extract hidden comment calculation to shared method and testing --- routers/web/repo/compare.go | 24 +--- routers/web/repo/compare_test.go | 51 ------- services/gitdiff/gitdiff.go | 49 ++++--- services/gitdiff/gitdiff_test.go | 220 +++++++++++++++++++++++++++++++ services/pull/review.go | 1 - 5 files changed, 251 insertions(+), 94 deletions(-) diff --git a/routers/web/repo/compare.go b/routers/web/repo/compare.go index 4409409ac5624..60273609bfdbe 100644 --- a/routers/web/repo/compare.go +++ b/routers/web/repo/compare.go @@ -882,32 +882,10 @@ func attachCommentsToLines(section *gitdiff.DiffSection, lineComments map[int64] } } -// calculateHiddenCommentIDs finds comment IDs that are in the hidden range of an expand button -func calculateHiddenCommentIDs(line *gitdiff.DiffLine, lineComments map[int64][]*issues_model.Comment) []int64 { - if line.Type != gitdiff.DiffLineSection || line.SectionInfo == nil { - return nil - } - - var hiddenCommentIDs []int64 - for commentLineNum, comments := range lineComments { - absLineNum := commentLineNum - if absLineNum < 0 { - absLineNum = -absLineNum - } - // Check if comments are in the hidden range - if int(absLineNum) > line.SectionInfo.LastRightIdx && int(absLineNum) < line.SectionInfo.RightIdx { - for _, comment := range comments { - hiddenCommentIDs = append(hiddenCommentIDs, comment.ID) - } - } - } - return hiddenCommentIDs -} - // attachHiddenCommentIDs calculates and attaches hidden comment IDs to expand buttons func attachHiddenCommentIDs(section *gitdiff.DiffSection, lineComments map[int64][]*issues_model.Comment) { for _, line := range section.Lines { - if hiddenIDs := calculateHiddenCommentIDs(line, lineComments); len(hiddenIDs) > 0 { + if hiddenIDs := gitdiff.CalculateHiddenCommentIDsForLine(line, lineComments); len(hiddenIDs) > 0 { line.HiddenCommentIDs = hiddenIDs } } diff --git a/routers/web/repo/compare_test.go b/routers/web/repo/compare_test.go index e4a90ddcba890..a21d514669e66 100644 --- a/routers/web/repo/compare_test.go +++ b/routers/web/repo/compare_test.go @@ -12,57 +12,6 @@ import ( "github.com/stretchr/testify/assert" ) -func TestCalculateHiddenCommentIDs(t *testing.T) { - tests := []struct { - name string - line *gitdiff.DiffLine - lineComments map[int64][]*issues_model.Comment - expected []int64 - }{ - { - name: "comments in hidden range", - line: &gitdiff.DiffLine{ - Type: gitdiff.DiffLineSection, - SectionInfo: &gitdiff.DiffLineSectionInfo{ - LastRightIdx: 10, - RightIdx: 20, - }, - }, - lineComments: map[int64][]*issues_model.Comment{ - 15: {{ID: 100}, {ID: 101}}, - 12: {{ID: 102}}, - }, - expected: []int64{100, 101, 102}, - }, - { - name: "comments outside hidden range", - line: &gitdiff.DiffLine{ - Type: gitdiff.DiffLineSection, - SectionInfo: &gitdiff.DiffLineSectionInfo{ - LastRightIdx: 10, - RightIdx: 20, - }, - }, - lineComments: map[int64][]*issues_model.Comment{ - 5: {{ID: 100}}, - 25: {{ID: 101}}, - }, - expected: nil, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - result := calculateHiddenCommentIDs(tt.line, tt.lineComments) - if tt.expected == nil { - assert.Nil(t, result) - } else { - assert.ElementsMatch(t, tt.expected, result) - } - }) - } -} - func TestAttachHiddenCommentIDs(t *testing.T) { section := &gitdiff.DiffSection{ Lines: []*gitdiff.DiffLine{ diff --git a/services/gitdiff/gitdiff.go b/services/gitdiff/gitdiff.go index 62ed23cc1aa2c..0bed3100534e3 100644 --- a/services/gitdiff/gitdiff.go +++ b/services/gitdiff/gitdiff.go @@ -183,6 +183,28 @@ func (d *DiffLine) GetExpandDirection() DiffLineExpandDirection { return DiffLineExpandSingle } +// CalculateHiddenCommentIDsForLine finds comment IDs that are in the hidden range of an expand button +func CalculateHiddenCommentIDsForLine(line *DiffLine, lineComments map[int64][]*issues_model.Comment) []int64 { + if line.Type != DiffLineSection || line.SectionInfo == nil { + return nil + } + + var hiddenCommentIDs []int64 + for commentLineNum, comments := range lineComments { + absLineNum := int(commentLineNum) + if commentLineNum < 0 { + absLineNum = int(-commentLineNum) + } + // Check if comments are in the hidden range + if absLineNum > line.SectionInfo.LastRightIdx && absLineNum < line.SectionInfo.RightIdx { + for _, comment := range comments { + hiddenCommentIDs = append(hiddenCommentIDs, comment.ID) + } + } + } + return hiddenCommentIDs +} + func getDiffLineSectionInfo(treePath, line string, lastLeftIdx, lastRightIdx int) *DiffLineSectionInfo { leftLine, leftHunk, rightLine, rightHunk := git.ParseDiffHunkString(line) @@ -488,24 +510,8 @@ func (diff *Diff) LoadComments(ctx context.Context, issue *issues_model.Issue, c }) // Mark expand buttons that have comments in hidden lines - if line.Type == DiffLineSection && line.SectionInfo != nil { - var hiddenCommentIDs []int64 - // Check if there are comments in the hidden range - for commentLineNum, comments := range lineCommits { - absLineNum := int(commentLineNum) - if commentLineNum < 0 { - absLineNum = int(-commentLineNum) - } - if absLineNum > line.SectionInfo.LastRightIdx && absLineNum < line.SectionInfo.RightIdx { - // Collect comment IDs - for _, comment := range comments { - hiddenCommentIDs = append(hiddenCommentIDs, comment.ID) - } - } - } - if len(hiddenCommentIDs) > 0 { - line.HiddenCommentIDs = hiddenCommentIDs - } + if hiddenIDs := CalculateHiddenCommentIDsForLine(line, lineCommits); len(hiddenIDs) > 0 { + line.HiddenCommentIDs = hiddenIDs } } } @@ -1427,6 +1433,11 @@ func GeneratePatchForUnchangedLine(gitRepo *git.Repository, commitID, treePath s } defer dataRc.Close() + return generatePatchForUnchangedLineFromReader(dataRc, treePath, line, contextLines) +} + +// generatePatchForUnchangedLineFromReader is the testable core logic that generates a patch from a reader +func generatePatchForUnchangedLineFromReader(reader io.Reader, treePath string, line int64, contextLines int) (string, error) { // Calculate line range (commented line + lines above it) commentLine := int(line) if line < 0 { @@ -1436,7 +1447,7 @@ func GeneratePatchForUnchangedLine(gitRepo *git.Repository, commitID, treePath s endLine := commentLine // Read only the needed lines efficiently - scanner := bufio.NewScanner(dataRc) + scanner := bufio.NewScanner(reader) currentLine := 0 var lines []string for scanner.Scan() { diff --git a/services/gitdiff/gitdiff_test.go b/services/gitdiff/gitdiff_test.go index 7b64b6b5f8ae2..f9bfb27b92504 100644 --- a/services/gitdiff/gitdiff_test.go +++ b/services/gitdiff/gitdiff_test.go @@ -640,3 +640,223 @@ func TestNoCrashes(t *testing.T) { ParsePatch(t.Context(), setting.Git.MaxGitDiffLines, setting.Git.MaxGitDiffLineCharacters, setting.Git.MaxGitDiffFiles, strings.NewReader(testcase.gitdiff), "") } } + +func TestGeneratePatchForUnchangedLineFromReader(t *testing.T) { + tests := []struct { + name string + content string + treePath string + line int64 + contextLines int + want string + wantErr bool + }{ + { + name: "single line with context", + content: "line1\nline2\nline3\nline4\nline5\n", + treePath: "test.txt", + line: 3, + contextLines: 1, + want: `diff --git a/test.txt b/test.txt +--- a/test.txt ++++ b/test.txt +@@ -2,2 +2,2 @@ + line2 + line3 +`, + }, + { + name: "negative line number (left side)", + content: "line1\nline2\nline3\nline4\nline5\n", + treePath: "test.txt", + line: -3, + contextLines: 1, + want: `diff --git a/test.txt b/test.txt +--- a/test.txt ++++ b/test.txt +@@ -2,2 +2,2 @@ + line2 + line3 +`, + }, + { + name: "line near start of file", + content: "line1\nline2\nline3\n", + treePath: "test.txt", + line: 2, + contextLines: 5, + want: `diff --git a/test.txt b/test.txt +--- a/test.txt ++++ b/test.txt +@@ -1,2 +1,2 @@ + line1 + line2 +`, + }, + { + name: "first line with context", + content: "line1\nline2\nline3\n", + treePath: "test.txt", + line: 1, + contextLines: 3, + want: `diff --git a/test.txt b/test.txt +--- a/test.txt ++++ b/test.txt +@@ -1,1 +1,1 @@ + line1 +`, + }, + { + name: "zero context lines", + content: "line1\nline2\nline3\n", + treePath: "test.txt", + line: 2, + contextLines: 0, + want: `diff --git a/test.txt b/test.txt +--- a/test.txt ++++ b/test.txt +@@ -2,1 +2,1 @@ + line2 +`, + }, + { + name: "multi-line context", + content: "package main\n\nfunc main() {\n\tfmt.Println(\"Hello\")\n}\n", + treePath: "main.go", + line: 4, + contextLines: 2, + want: `diff --git a/main.go b/main.go +--- a/main.go ++++ b/main.go +@@ -2,3 +2,3 @@ + + func main() { + fmt.Println("Hello") +`, + }, + { + name: "empty file", + content: "", + treePath: "empty.txt", + line: 1, + contextLines: 1, + wantErr: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + reader := strings.NewReader(tt.content) + got, err := generatePatchForUnchangedLineFromReader(reader, tt.treePath, tt.line, tt.contextLines) + if tt.wantErr { + assert.Error(t, err) + } else { + require.NoError(t, err) + assert.Equal(t, tt.want, got) + } + }) + } +} + +func TestCalculateHiddenCommentIDsForLine(t *testing.T) { + tests := []struct { + name string + line *DiffLine + lineComments map[int64][]*issues_model.Comment + expected []int64 + }{ + { + name: "comments in hidden range", + line: &DiffLine{ + Type: DiffLineSection, + SectionInfo: &DiffLineSectionInfo{ + LastRightIdx: 10, + RightIdx: 20, + }, + }, + lineComments: map[int64][]*issues_model.Comment{ + 15: {{ID: 100}, {ID: 101}}, + 12: {{ID: 102}}, + }, + expected: []int64{100, 101, 102}, + }, + { + name: "comments outside hidden range", + line: &DiffLine{ + Type: DiffLineSection, + SectionInfo: &DiffLineSectionInfo{ + LastRightIdx: 10, + RightIdx: 20, + }, + }, + lineComments: map[int64][]*issues_model.Comment{ + 5: {{ID: 100}}, + 25: {{ID: 101}}, + }, + expected: nil, + }, + { + name: "negative line numbers (left side)", + line: &DiffLine{ + Type: DiffLineSection, + SectionInfo: &DiffLineSectionInfo{ + LastRightIdx: 10, + RightIdx: 20, + }, + }, + lineComments: map[int64][]*issues_model.Comment{ + -15: {{ID: 100}}, + }, + expected: []int64{100}, + }, + { + name: "boundary conditions - exclusive range", + line: &DiffLine{ + Type: DiffLineSection, + SectionInfo: &DiffLineSectionInfo{ + LastRightIdx: 10, + RightIdx: 20, + }, + }, + lineComments: map[int64][]*issues_model.Comment{ + 10: {{ID: 100}}, // at LastRightIdx, should not be included + 20: {{ID: 101}}, // at RightIdx, should not be included + 11: {{ID: 102}}, // just inside range, should be included + 19: {{ID: 103}}, // just inside range, should be included + }, + expected: []int64{102, 103}, + }, + { + name: "not a section line", + line: &DiffLine{ + Type: DiffLinePlain, + }, + lineComments: map[int64][]*issues_model.Comment{ + 15: {{ID: 100}}, + }, + expected: nil, + }, + { + name: "section line without section info", + line: &DiffLine{ + Type: DiffLineSection, + SectionInfo: nil, + }, + lineComments: map[int64][]*issues_model.Comment{ + 15: {{ID: 100}}, + }, + expected: nil, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result := CalculateHiddenCommentIDsForLine(tt.line, tt.lineComments) + if tt.expected == nil { + assert.Nil(t, result) + } else { + assert.ElementsMatch(t, tt.expected, result) + } + }) + } +} diff --git a/services/pull/review.go b/services/pull/review.go index 808d84740f35e..4c926272ea3da 100644 --- a/services/pull/review.go +++ b/services/pull/review.go @@ -290,7 +290,6 @@ func createCodeComment(ctx context.Context, doer *user_model.User, repo *repo_mo patch, err = gitdiff.GeneratePatchForUnchangedLine(gitRepo, commitID, treePath, line, setting.UI.CodeCommentLines) if err != nil { // Log the error but don't fail comment creation - // This can happen for deleted/renamed files or other edge cases log.Warn("Unable to generate patch for unchanged line (file=%s, line=%d, commit=%s): %v", treePath, line, commitID, err) } } From fd2fc613c72f474878f5727d1ae3b2e059794fc1 Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Sat, 18 Oct 2025 22:12:10 +0800 Subject: [PATCH 11/22] clean up code-expander-button --- routers/web/repo/commit.go | 23 +++-- routers/web/repo/compare.go | 19 +++- routers/web/repo/pull.go | 6 ++ services/gitdiff/gitdiff.go | 117 ++++++++++++++-------- services/gitdiff/gitdiff_test.go | 11 +- templates/repo/diff/blob_excerpt.tmpl | 50 +-------- templates/repo/diff/section_split.tmpl | 27 +---- templates/repo/diff/section_unified.tmpl | 28 +----- web_src/css/index.css | 1 - web_src/css/repo/diff-hidden-comments.css | 38 ------- web_src/css/review.css | 21 +++- 11 files changed, 140 insertions(+), 201 deletions(-) delete mode 100644 web_src/css/repo/diff-hidden-comments.css diff --git a/routers/web/repo/commit.go b/routers/web/repo/commit.go index 1a86a62fae172..0383e4ca9eea5 100644 --- a/routers/web/repo/commit.go +++ b/routers/web/repo/commit.go @@ -276,20 +276,24 @@ func Diff(ctx *context.Context) { userName := ctx.Repo.Owner.Name repoName := ctx.Repo.Repository.Name commitID := ctx.PathParam("sha") - var ( - gitRepo *git.Repository - err error - ) + + diffBlobExcerptData := &gitdiff.DiffBlobExcerptData{ + BaseLink: ctx.Repo.RepoLink + "/blob_excerpt", + DiffStyle: ctx.FormString("style"), + AfterCommitID: commitID, + } + gitRepo := ctx.Repo.GitRepo + var gitRepoStore gitrepo.Repository = ctx.Repo.Repository if ctx.Data["PageIsWiki"] != nil { - gitRepo, err = gitrepo.OpenRepository(ctx, ctx.Repo.Repository.WikiStorageRepo()) + var err error + gitRepoStore = ctx.Repo.Repository.WikiStorageRepo() + gitRepo, err = gitrepo.RepositoryFromRequestContextOrOpen(ctx, gitRepoStore) if err != nil { ctx.ServerError("Repo.GitRepo.GetCommit", err) return } - defer gitRepo.Close() - } else { - gitRepo = ctx.Repo.GitRepo + diffBlobExcerptData.BaseLink = ctx.Repo.RepoLink + "/wiki/blob_excerpt" } commit, err := gitRepo.GetCommit(commitID) @@ -324,7 +328,7 @@ func Diff(ctx *context.Context) { ctx.NotFound(err) return } - diffShortStat, err := gitdiff.GetDiffShortStat(ctx, ctx.Repo.Repository, gitRepo, "", commitID) + diffShortStat, err := gitdiff.GetDiffShortStat(ctx, gitRepoStore, gitRepo, "", commitID) if err != nil { ctx.ServerError("GetDiffShortStat", err) return @@ -360,6 +364,7 @@ func Diff(ctx *context.Context) { ctx.Data["Title"] = commit.Summary() + " · " + base.ShortSha(commitID) ctx.Data["Commit"] = commit ctx.Data["Diff"] = diff + ctx.Data["DiffBlobExcerptData"] = diffBlobExcerptData if !fileOnly { diffTree, err := gitdiff.GetDiffTree(ctx, gitRepo, false, parentCommitID, commitID) diff --git a/routers/web/repo/compare.go b/routers/web/repo/compare.go index 60273609bfdbe..8c00daee87ba8 100644 --- a/routers/web/repo/compare.go +++ b/routers/web/repo/compare.go @@ -885,9 +885,7 @@ func attachCommentsToLines(section *gitdiff.DiffSection, lineComments map[int64] // attachHiddenCommentIDs calculates and attaches hidden comment IDs to expand buttons func attachHiddenCommentIDs(section *gitdiff.DiffSection, lineComments map[int64][]*issues_model.Comment) { for _, line := range section.Lines { - if hiddenIDs := gitdiff.CalculateHiddenCommentIDsForLine(line, lineComments); len(hiddenIDs) > 0 { - line.HiddenCommentIDs = hiddenIDs - } + gitdiff.FillHiddenCommentIDsForDiffLine(line, lineComments) } } @@ -904,15 +902,23 @@ func ExcerptBlob(ctx *context.Context) { direction := ctx.FormString("direction") filePath := ctx.FormString("path") gitRepo := ctx.Repo.GitRepo + + diffBlobExcerptData := &gitdiff.DiffBlobExcerptData{ + BaseLink: ctx.Repo.RepoLink + "/blob_excerpt", + DiffStyle: ctx.FormString("style"), + AfterCommitID: commitID, + } + if ctx.Data["PageIsWiki"] == true { var err error - gitRepo, err = gitrepo.OpenRepository(ctx, ctx.Repo.Repository.WikiStorageRepo()) + gitRepo, err = gitrepo.RepositoryFromRequestContextOrOpen(ctx, ctx.Repo.Repository.WikiStorageRepo()) if err != nil { ctx.ServerError("OpenRepository", err) return } - defer gitRepo.Close() + diffBlobExcerptData.BaseLink = ctx.Repo.RepoLink + "/wiki/blob_excerpt" } + chunkSize := gitdiff.BlobExcerptChunkSize commit, err := gitRepo.GetCommit(commitID) if err != nil { @@ -985,6 +991,7 @@ func ExcerptBlob(ctx *context.Context) { return } issueIndex := ctx.FormInt64("issue_index") + diffBlobExcerptData.PullIndex = issueIndex ctx.Data["IssueIndex"] = issueIndex if issueIndex > 0 { if issue, err := issues_model.GetIssueByIndex(ctx, ctx.Repo.Repository.ID, issueIndex); err == nil && issue.IsPull { @@ -1007,6 +1014,8 @@ func ExcerptBlob(ctx *context.Context) { ctx.Data["FileNameHash"] = git.HashFilePathForWebUI(filePath) ctx.Data["AfterCommitID"] = commitID ctx.Data["Anchor"] = anchor + ctx.Data["DiffBlobExcerptData"] = diffBlobExcerptData + ctx.HTML(http.StatusOK, tplBlobExcerpt) } diff --git a/routers/web/repo/pull.go b/routers/web/repo/pull.go index ea3e700b771cd..f0d19f41af126 100644 --- a/routers/web/repo/pull.go +++ b/routers/web/repo/pull.go @@ -827,6 +827,12 @@ func viewPullFiles(ctx *context.Context, beforeCommitID, afterCommitID string) { } ctx.Data["Diff"] = diff + ctx.Data["DiffBlobExcerptData"] = &gitdiff.DiffBlobExcerptData{ + BaseLink: ctx.Repo.RepoLink + "/blob_excerpt", + PullIndex: pull.Index, + DiffStyle: ctx.FormString("style"), + AfterCommitID: afterCommitID, + } ctx.Data["DiffNotAvailable"] = diffShortStat.NumFiles == 0 if ctx.IsSigned && ctx.Doer != nil { diff --git a/services/gitdiff/gitdiff.go b/services/gitdiff/gitdiff.go index 0bed3100534e3..8aa6717c0e875 100644 --- a/services/gitdiff/gitdiff.go +++ b/services/gitdiff/gitdiff.go @@ -22,19 +22,21 @@ import ( git_model "code.gitea.io/gitea/models/git" issues_model "code.gitea.io/gitea/models/issues" pull_model "code.gitea.io/gitea/models/pull" - repo_model "code.gitea.io/gitea/models/repo" user_model "code.gitea.io/gitea/models/user" "code.gitea.io/gitea/modules/analyze" + "code.gitea.io/gitea/modules/base" "code.gitea.io/gitea/modules/charset" "code.gitea.io/gitea/modules/git" "code.gitea.io/gitea/modules/git/attribute" "code.gitea.io/gitea/modules/git/gitcmd" "code.gitea.io/gitea/modules/gitrepo" "code.gitea.io/gitea/modules/highlight" + "code.gitea.io/gitea/modules/htmlutil" "code.gitea.io/gitea/modules/lfs" "code.gitea.io/gitea/modules/log" "code.gitea.io/gitea/modules/optional" "code.gitea.io/gitea/modules/setting" + "code.gitea.io/gitea/modules/svg" "code.gitea.io/gitea/modules/translation" "code.gitea.io/gitea/modules/util" @@ -67,28 +69,15 @@ const ( DiffFileCopy ) -// DiffLineExpandDirection represents the DiffLineSection expand direction -type DiffLineExpandDirection uint8 - -// DiffLineExpandDirection possible values. -const ( - DiffLineExpandNone DiffLineExpandDirection = iota + 1 - DiffLineExpandSingle - DiffLineExpandUpDown - DiffLineExpandUp - DiffLineExpandDown -) - // DiffLine represents a line difference in a DiffSection. type DiffLine struct { - LeftIdx int // line number, 1-based - RightIdx int // line number, 1-based - Match int // the diff matched index. -1: no match. 0: plain and no need to match. >0: for add/del, "Lines" slice index of the other side - Type DiffLineType - Content string - Comments issues_model.CommentList // related PR code comments - SectionInfo *DiffLineSectionInfo - HiddenCommentIDs []int64 // IDs of hidden comments in this section + LeftIdx int // line number, 1-based + RightIdx int // line number, 1-based + Match int // the diff matched index. -1: no match. 0: plain and no need to match. >0: for add/del, "Lines" slice index of the other side + Type DiffLineType + Content string + Comments issues_model.CommentList // related PR code comments + SectionInfo *DiffLineSectionInfo } // DiffLineSectionInfo represents diff line section meta data @@ -100,6 +89,9 @@ type DiffLineSectionInfo struct { RightIdx int LeftHunkSize int RightHunkSize int + + HiddenCommentIDs []int64 // IDs of hidden comments in this section + } // DiffHTMLOperation is the HTML version of diffmatchpatch.Diff @@ -154,8 +146,7 @@ func (d *DiffLine) GetLineTypeMarker() string { return "" } -// GetBlobExcerptQuery builds query string to get blob excerpt -func (d *DiffLine) GetBlobExcerptQuery() string { +func (d *DiffLine) getBlobExcerptQuery() string { query := fmt.Sprintf( "last_left=%d&last_right=%d&"+ "left=%d&right=%d&"+ @@ -168,25 +159,70 @@ func (d *DiffLine) GetBlobExcerptQuery() string { return query } -// GetExpandDirection gets DiffLineExpandDirection -func (d *DiffLine) GetExpandDirection() DiffLineExpandDirection { +func (d *DiffLine) getExpandDirection() string { if d.Type != DiffLineSection || d.SectionInfo == nil || d.SectionInfo.LeftIdx-d.SectionInfo.LastLeftIdx <= 1 || d.SectionInfo.RightIdx-d.SectionInfo.LastRightIdx <= 1 { - return DiffLineExpandNone + return "" } if d.SectionInfo.LastLeftIdx <= 0 && d.SectionInfo.LastRightIdx <= 0 { - return DiffLineExpandUp + return "up" } else if d.SectionInfo.RightIdx-d.SectionInfo.LastRightIdx > BlobExcerptChunkSize && d.SectionInfo.RightHunkSize > 0 { - return DiffLineExpandUpDown + return "updown" } else if d.SectionInfo.LeftHunkSize <= 0 && d.SectionInfo.RightHunkSize <= 0 { - return DiffLineExpandDown + return "down" } - return DiffLineExpandSingle + return "single" } -// CalculateHiddenCommentIDsForLine finds comment IDs that are in the hidden range of an expand button -func CalculateHiddenCommentIDsForLine(line *DiffLine, lineComments map[int64][]*issues_model.Comment) []int64 { - if line.Type != DiffLineSection || line.SectionInfo == nil { - return nil +type DiffBlobExcerptData struct { + BaseLink string + IsWikiRepo bool + PullIndex int64 + DiffStyle string + AfterCommitID string +} + +func (d *DiffLine) RenderBlobExcerptButtons(fileNameHash string, data *DiffBlobExcerptData) template.HTML { + dataHiddenCommentIDs := strings.Join(base.Int64sToStrings(d.SectionInfo.HiddenCommentIDs), ",") + anchor := fmt.Sprintf("diff-%sK%d", fileNameHash, d.SectionInfo.RightIdx) + + makeButton := func(direction, svgName string) template.HTML { + style := util.IfZero(data.DiffStyle, "unified") + link := data.BaseLink + "/" + data.AfterCommitID + fmt.Sprintf("?style=%s&direction=%s&anchor=%s", url.QueryEscape(style), direction, url.QueryEscape(anchor)) + if data.PullIndex > 0 { + link += fmt.Sprintf("&is_pull=1&issue_index=%d", data.PullIndex) + } + link += "&" + d.getBlobExcerptQuery() + + svgContent := svg.RenderHTML(svgName) + return htmlutil.HTMLFormat( + ``, + link, dataHiddenCommentIDs, svgContent, + ) + } + var content template.HTML + + if len(d.SectionInfo.HiddenCommentIDs) > 0 { + tooltip := fmt.Sprintf("%d hidden comment(s)", len(d.SectionInfo.HiddenCommentIDs)) + content += htmlutil.HTMLFormat(`%d`, tooltip, len(d.SectionInfo.HiddenCommentIDs)) + } + + expandDirection := d.getExpandDirection() + if expandDirection == "up" || expandDirection == "updown" { + content += makeButton("up", "octicon-fold-up") + } + if expandDirection == "updown" || expandDirection == "down" { + content += makeButton("down", "octicon-fold-down") + } + if expandDirection == "single" { + content += makeButton("single", "octicon-fold") + } + return htmlutil.HTMLFormat(`
%s
`, expandDirection, content) +} + +// FillHiddenCommentIDsForDiffLine finds comment IDs that are in the hidden range of an expand button +func FillHiddenCommentIDsForDiffLine(line *DiffLine, lineComments map[int64][]*issues_model.Comment) { + if line.Type != DiffLineSection { + return } var hiddenCommentIDs []int64 @@ -196,13 +232,13 @@ func CalculateHiddenCommentIDsForLine(line *DiffLine, lineComments map[int64][]* absLineNum = int(-commentLineNum) } // Check if comments are in the hidden range - if absLineNum > line.SectionInfo.LastRightIdx && absLineNum < line.SectionInfo.RightIdx { + if absLineNum > line.SectionInfo.LastRightIdx && absLineNum <= line.SectionInfo.RightIdx { for _, comment := range comments { hiddenCommentIDs = append(hiddenCommentIDs, comment.ID) } } } - return hiddenCommentIDs + line.SectionInfo.HiddenCommentIDs = hiddenCommentIDs } func getDiffLineSectionInfo(treePath, line string, lastLeftIdx, lastRightIdx int) *DiffLineSectionInfo { @@ -508,11 +544,8 @@ func (diff *Diff) LoadComments(ctx context.Context, issue *issues_model.Issue, c sort.SliceStable(line.Comments, func(i, j int) bool { return line.Comments[i].CreatedUnix < line.Comments[j].CreatedUnix }) - // Mark expand buttons that have comments in hidden lines - if hiddenIDs := CalculateHiddenCommentIDsForLine(line, lineCommits); len(hiddenIDs) > 0 { - line.HiddenCommentIDs = hiddenIDs - } + FillHiddenCommentIDsForDiffLine(line, lineCommits) } } } @@ -1309,7 +1342,7 @@ type DiffShortStat struct { NumFiles, TotalAddition, TotalDeletion int } -func GetDiffShortStat(ctx context.Context, repo *repo_model.Repository, gitRepo *git.Repository, beforeCommitID, afterCommitID string) (*DiffShortStat, error) { +func GetDiffShortStat(ctx context.Context, repoStorage gitrepo.Repository, gitRepo *git.Repository, beforeCommitID, afterCommitID string) (*DiffShortStat, error) { afterCommit, err := gitRepo.GetCommit(afterCommitID) if err != nil { return nil, err @@ -1321,7 +1354,7 @@ func GetDiffShortStat(ctx context.Context, repo *repo_model.Repository, gitRepo } diff := &DiffShortStat{} - diff.NumFiles, diff.TotalAddition, diff.TotalDeletion, err = gitrepo.GetDiffShortStatByCmdArgs(ctx, repo, nil, actualBeforeCommitID.String(), afterCommitID) + diff.NumFiles, diff.TotalAddition, diff.TotalDeletion, err = gitrepo.GetDiffShortStatByCmdArgs(ctx, repoStorage, nil, actualBeforeCommitID.String(), afterCommitID) if err != nil { return nil, err } diff --git a/services/gitdiff/gitdiff_test.go b/services/gitdiff/gitdiff_test.go index f9bfb27b92504..2464c891d1c47 100644 --- a/services/gitdiff/gitdiff_test.go +++ b/services/gitdiff/gitdiff_test.go @@ -729,7 +729,7 @@ func TestGeneratePatchForUnchangedLineFromReader(t *testing.T) { --- a/main.go +++ b/main.go @@ -2,3 +2,3 @@ - + func main() { fmt.Println("Hello") `, @@ -851,12 +851,9 @@ func TestCalculateHiddenCommentIDsForLine(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - result := CalculateHiddenCommentIDsForLine(tt.line, tt.lineComments) - if tt.expected == nil { - assert.Nil(t, result) - } else { - assert.ElementsMatch(t, tt.expected, result) - } + tt.line.SectionInfo = &DiffLineSectionInfo{} + FillHiddenCommentIDsForDiffLine(tt.line, tt.lineComments) + assert.ElementsMatch(t, tt.expected, tt.line.SectionInfo.HiddenCommentIDs) }) } } diff --git a/templates/repo/diff/blob_excerpt.tmpl b/templates/repo/diff/blob_excerpt.tmpl index 23ca71e05407d..5839aced843b4 100644 --- a/templates/repo/diff/blob_excerpt.tmpl +++ b/templates/repo/diff/blob_excerpt.tmpl @@ -1,31 +1,9 @@ -{{$blobExcerptLink := print $.RepoLink (Iif $.PageIsWiki "/wiki" "") "/blob_excerpt/" (PathEscape $.AfterCommitID) (QueryBuild "?" "anchor" $.Anchor) (Iif $.PageIsPullFiles (print "&is_pull=true&issue_index=" $.IssueIndex) "")}} +{{$diffBlobExcerptData := .DiffBlobExcerptData}} {{if $.IsSplitStyle}} {{range $k, $line := $.section.Lines}} {{if eq .GetType 4}} - {{$expandDirection := $line.GetExpandDirection}} - -
- {{if or (eq $expandDirection 3) (eq $expandDirection 5)}} - - {{end}} - {{if or (eq $expandDirection 3) (eq $expandDirection 4)}} - - {{end}} - {{if eq $expandDirection 2}} - - {{end}} -
- + {{$line.RenderBlobExcerptButtons $.FileNameHash $diffBlobExcerptData}} {{- $inlineDiff := $.section.GetComputedInlineDiffFor $line ctx.Locale -}} {{- template "repo/diff/section_code" dict "diff" $inlineDiff -}} @@ -83,29 +61,7 @@ {{range $k, $line := $.section.Lines}} {{if eq .GetType 4}} - {{$expandDirection := $line.GetExpandDirection}} - -
- {{if or (eq $expandDirection 3) (eq $expandDirection 5)}} - - {{end}} - {{if or (eq $expandDirection 3) (eq $expandDirection 4)}} - - {{end}} - {{if eq $expandDirection 2}} - - {{end}} -
- + {{$line.RenderBlobExcerptButtons $.FileNameHash $diffBlobExcerptData}} {{else}} diff --git a/templates/repo/diff/section_split.tmpl b/templates/repo/diff/section_split.tmpl index 6f30889bd7d95..ab23b1b934b7b 100644 --- a/templates/repo/diff/section_split.tmpl +++ b/templates/repo/diff/section_split.tmpl @@ -1,5 +1,5 @@ {{$file := .file}} -{{$blobExcerptLink := print (or ctx.RootData.CommitRepoLink ctx.RootData.RepoLink) (Iif $.root.PageIsWiki "/wiki" "") "/blob_excerpt/" (PathEscape $.root.AfterCommitID) "?" (Iif $.root.PageIsPullFiles (print "is_pull=true&issue_index=" $.root.Issue.Index "&") "")}} +{{$diffBlobExcerptData := $.root.DiffBlobExcerptData}} @@ -16,29 +16,8 @@ {{if or (ne .GetType 2) (not $hasmatch)}} {{if eq .GetType 4}} - {{$expandDirection := $line.GetExpandDirection}} - -
- {{if or (eq $expandDirection 3) (eq $expandDirection 5)}} - - {{end}} - {{if or (eq $expandDirection 3) (eq $expandDirection 4)}} - - {{end}} - {{if eq $expandDirection 2}} - - {{end}} -
- {{$inlineDiff := $section.GetComputedInlineDiffFor $line ctx.Locale}} + {{$inlineDiff := $section.GetComputedInlineDiffFor $line ctx.Locale}} + {{$line.RenderBlobExcerptButtons $file.NameHash $diffBlobExcerptData}} {{if $inlineDiff.EscapeStatus.Escaped}}{{end}} {{template "repo/diff/section_code" dict "diff" $inlineDiff}} {{else if and (eq .GetType 3) $hasmatch}}{{/* DEL */}} diff --git a/templates/repo/diff/section_unified.tmpl b/templates/repo/diff/section_unified.tmpl index 290259a957e3f..094817d436366 100644 --- a/templates/repo/diff/section_unified.tmpl +++ b/templates/repo/diff/section_unified.tmpl @@ -1,7 +1,5 @@ {{$file := .file}} -{{$repoLink := or ctx.RootData.CommitRepoLink ctx.RootData.RepoLink}} -{{$afterCommitID := or $.root.AfterCommitID "no-after-commit-id"}}{{/* this tmpl is also used by the PR Conversation page, so the "AfterCommitID" may not exist */}} -{{$blobExcerptLink := print $repoLink (Iif $.root.PageIsWiki "/wiki" "") "/blob_excerpt/" (PathEscape $afterCommitID) "?" (Iif $.root.PageIsPullFiles (print "is_pull=true&issue_index=" $.root.Issue.Index "&") "")}} +{{$diffBlobExcerptData := $.root.DiffBlobExcerptData}} @@ -14,29 +12,7 @@ {{if eq .GetType 4}} {{if $.root.AfterCommitID}} - {{$expandDirection := $line.GetExpandDirection}} - -
- {{if or (eq $expandDirection 3) (eq $expandDirection 5)}} - - {{end}} - {{if or (eq $expandDirection 3) (eq $expandDirection 4)}} - - {{end}} - {{if eq $expandDirection 2}} - - {{end}} -
- + {{$line.RenderBlobExcerptButtons $file.NameHash $diffBlobExcerptData}} {{else}} {{/* for code file preview page or comment diffs on pull comment pages, do not show the expansion arrows */}} diff --git a/web_src/css/index.css b/web_src/css/index.css index 2b6fbd467a598..291cd04b2b95c 100644 --- a/web_src/css/index.css +++ b/web_src/css/index.css @@ -71,7 +71,6 @@ @import "./repo/clone.css"; @import "./repo/commit-sign.css"; @import "./repo/packages.css"; -@import "./repo/diff-hidden-comments.css"; @import "./editor/fileeditor.css"; @import "./editor/combomarkdowneditor.css"; diff --git a/web_src/css/repo/diff-hidden-comments.css b/web_src/css/repo/diff-hidden-comments.css deleted file mode 100644 index c23f56bf55552..0000000000000 --- a/web_src/css/repo/diff-hidden-comments.css +++ /dev/null @@ -1,38 +0,0 @@ -/* Styling for expand buttons with hidden comments */ -.code-expander-button.has-hidden-comments { - position: relative; -} - -.code-expander-button .ui.mini.label { - position: absolute; - min-width: 16px; - height: 16px; - padding: 2px 4px; - font-size: 10px; - line-height: 12px; - background-color: var(--color-primary); - color: var(--color-primary-contrast); - border-radius: 8px; -} - -.code-expander-button:has(svg.octicon-fold-down) .ui.mini.label { - top: 50%; - left: -20px; - transform: translateY(-50%); -} - -.code-expander-button:has(svg.octicon-fold-up) .ui.mini.label { - top: 50%; - left: -20px; - transform: translateY(-50%); -} - -.code-expander-button:has(svg.octicon-fold) .ui.mini.label { - top: 50%; - left: -20px; - transform: translateY(-50%); -} - -.code-expander-button.has-hidden-comments:hover .ui.mini.label { - background-color: var(--color-primary-dark-1); -} diff --git a/web_src/css/review.css b/web_src/css/review.css index 23383c051cccb..ad2762996499e 100644 --- a/web_src/css/review.css +++ b/web_src/css/review.css @@ -115,6 +115,23 @@ color: var(--color-text); } +.code-expander-buttons { + position: relative; +} + +.code-expander-buttons .code-comment-more { + position: absolute; + line-height: 12px; + padding: 2px 4px; + font-size: 10px; + background-color: var(--color-primary); + color: var(--color-primary-contrast); + border-radius: 8px !important; + left: 4px; + top: 4px; + text-align: center; +} + .code-expander-button { border: none; color: var(--color-text-light); @@ -127,8 +144,8 @@ flex: 1; } -/* expand direction 3 is both ways with two buttons */ -.code-expander-buttons[data-expand-direction="3"] .code-expander-button { +/* expand direction "updown" is both ways with two buttons */ +.code-expander-buttons[data-expand-direction="updown"] .code-expander-button { height: 18px; } From a95ebbba05ef46d7901f69181531d77fa67692f4 Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Sat, 18 Oct 2025 23:27:58 +0800 Subject: [PATCH 12/22] fix --- routers/web/repo/compare.go | 42 +++++++++++----------- routers/web/repo/compare_test.go | 6 ++-- routers/web/repo/pull.go | 8 ++--- services/gitdiff/gitdiff.go | 22 +++++------- services/pull/review.go | 4 +-- templates/repo/diff/blob_excerpt.tmpl | 2 +- web_src/css/review.css | 2 +- web_src/js/features/repo-diff.ts | 51 +++++++++++---------------- 8 files changed, 62 insertions(+), 75 deletions(-) diff --git a/routers/web/repo/compare.go b/routers/web/repo/compare.go index 8c00daee87ba8..a64c903f984d0 100644 --- a/routers/web/repo/compare.go +++ b/routers/web/repo/compare.go @@ -640,6 +640,11 @@ func PrepareCompareDiff( } ctx.Data["DiffShortStat"] = diffShortStat ctx.Data["Diff"] = diff + ctx.Data["DiffBlobExcerptData"] = &gitdiff.DiffBlobExcerptData{ + BaseLink: ctx.Repo.RepoLink + "/blob_excerpt", + DiffStyle: ctx.FormString("style"), + AfterCommitID: headCommitID, + } ctx.Data["DiffNotAvailable"] = diffShortStat.NumFiles == 0 if !fileOnly { @@ -979,32 +984,29 @@ func ExcerptBlob(ctx *context.Context) { section.Lines = append(section.Lines, lineSection) } } - if ctx.Doer != nil { - ctx.Data["SignedUserID"] = ctx.Doer.ID - } - isPull := ctx.FormBool("is_pull") - ctx.Data["PageIsPullFiles"] = isPull - if isPull { + diffBlobExcerptData.PullIssueIndex = ctx.FormInt64("pull_issue_index") + if diffBlobExcerptData.PullIssueIndex > 0 { if !ctx.Repo.CanRead(unit.TypePullRequests) { ctx.NotFound(nil) return } - issueIndex := ctx.FormInt64("issue_index") - diffBlobExcerptData.PullIndex = issueIndex - ctx.Data["IssueIndex"] = issueIndex - if issueIndex > 0 { - if issue, err := issues_model.GetIssueByIndex(ctx, ctx.Repo.Repository.ID, issueIndex); err == nil && issue.IsPull { - ctx.Data["Issue"] = issue - ctx.Data["CanBlockUser"] = func(blocker, blockee *user_model.User) bool { - return user_service.CanBlockUser(ctx, ctx.Doer, blocker, blockee) - } - if allComments, err := issues_model.FetchCodeComments(ctx, issue, ctx.Doer, ctx.FormBool("show_outdated")); err == nil { - if lineComments, ok := allComments[filePath]; ok { - attachCommentsToLines(section, lineComments) - attachHiddenCommentIDs(section, lineComments) - } + issue, err := issues_model.GetIssueByIndex(ctx, ctx.Repo.Repository.ID, diffBlobExcerptData.PullIssueIndex) + if err != nil { + log.Error("GetIssueByIndex error: %v", err) + } else if issue.IsPull { + ctx.Data["Issue"] = issue + ctx.Data["CanBlockUser"] = func(blocker, blockee *user_model.User) bool { + return user_service.CanBlockUser(ctx, ctx.Doer, blocker, blockee) + } + allComments, err := issues_model.FetchCodeComments(ctx, issue, ctx.Doer, ctx.FormBool("show_outdated")) + if err != nil { + log.Error("FetchCodeComments error: %v", err) + } else { + if lineComments, ok := allComments[filePath]; ok { + attachCommentsToLines(section, lineComments) + attachHiddenCommentIDs(section, lineComments) } } } diff --git a/routers/web/repo/compare_test.go b/routers/web/repo/compare_test.go index a21d514669e66..56b5b842a15d1 100644 --- a/routers/web/repo/compare_test.go +++ b/routers/web/repo/compare_test.go @@ -43,9 +43,9 @@ func TestAttachHiddenCommentIDs(t *testing.T) { attachHiddenCommentIDs(section, lineComments) - assert.Equal(t, []int64{100}, section.Lines[0].HiddenCommentIDs) - assert.Nil(t, section.Lines[1].HiddenCommentIDs) - assert.Equal(t, []int64{200}, section.Lines[2].HiddenCommentIDs) + assert.Equal(t, []int64{100}, section.Lines[0].SectionInfo.HiddenCommentIDs) + assert.Nil(t, section.Lines[1].SectionInfo.HiddenCommentIDs) + assert.Equal(t, []int64{200}, section.Lines[2].SectionInfo.HiddenCommentIDs) } func TestAttachCommentsToLines(t *testing.T) { diff --git a/routers/web/repo/pull.go b/routers/web/repo/pull.go index f0d19f41af126..9c99d4111627e 100644 --- a/routers/web/repo/pull.go +++ b/routers/web/repo/pull.go @@ -828,10 +828,10 @@ func viewPullFiles(ctx *context.Context, beforeCommitID, afterCommitID string) { ctx.Data["Diff"] = diff ctx.Data["DiffBlobExcerptData"] = &gitdiff.DiffBlobExcerptData{ - BaseLink: ctx.Repo.RepoLink + "/blob_excerpt", - PullIndex: pull.Index, - DiffStyle: ctx.FormString("style"), - AfterCommitID: afterCommitID, + BaseLink: ctx.Repo.RepoLink + "/blob_excerpt", + PullIssueIndex: pull.Index, + DiffStyle: ctx.FormString("style"), + AfterCommitID: afterCommitID, } ctx.Data["DiffNotAvailable"] = diffShortStat.NumFiles == 0 diff --git a/services/gitdiff/gitdiff.go b/services/gitdiff/gitdiff.go index 8aa6717c0e875..6272e659ad7ee 100644 --- a/services/gitdiff/gitdiff.go +++ b/services/gitdiff/gitdiff.go @@ -91,7 +91,6 @@ type DiffLineSectionInfo struct { RightHunkSize int HiddenCommentIDs []int64 // IDs of hidden comments in this section - } // DiffHTMLOperation is the HTML version of diffmatchpatch.Diff @@ -174,11 +173,11 @@ func (d *DiffLine) getExpandDirection() string { } type DiffBlobExcerptData struct { - BaseLink string - IsWikiRepo bool - PullIndex int64 - DiffStyle string - AfterCommitID string + BaseLink string + IsWikiRepo bool + PullIssueIndex int64 + DiffStyle string + AfterCommitID string } func (d *DiffLine) RenderBlobExcerptButtons(fileNameHash string, data *DiffBlobExcerptData) template.HTML { @@ -187,16 +186,13 @@ func (d *DiffLine) RenderBlobExcerptButtons(fileNameHash string, data *DiffBlobE makeButton := func(direction, svgName string) template.HTML { style := util.IfZero(data.DiffStyle, "unified") - link := data.BaseLink + "/" + data.AfterCommitID + fmt.Sprintf("?style=%s&direction=%s&anchor=%s", url.QueryEscape(style), direction, url.QueryEscape(anchor)) - if data.PullIndex > 0 { - link += fmt.Sprintf("&is_pull=1&issue_index=%d", data.PullIndex) + link := data.BaseLink + "/" + data.AfterCommitID + fmt.Sprintf("?style=%s&direction=%s&anchor=%s", url.QueryEscape(style), direction, url.QueryEscape(anchor)) + "&" + d.getBlobExcerptQuery() + if data.PullIssueIndex > 0 { + link += fmt.Sprintf("&pull_issue_index=%d", data.PullIssueIndex) } - link += "&" + d.getBlobExcerptQuery() - - svgContent := svg.RenderHTML(svgName) return htmlutil.HTMLFormat( ``, - link, dataHiddenCommentIDs, svgContent, + link, dataHiddenCommentIDs, svg.RenderHTML(svgName), ) } var content template.HTML diff --git a/services/pull/review.go b/services/pull/review.go index 4c926272ea3da..3977e351ca731 100644 --- a/services/pull/review.go +++ b/services/pull/review.go @@ -286,11 +286,11 @@ func createCodeComment(ctx context.Context, doer *user_model.User, repo *repo_mo } // If patch is still empty (unchanged line), generate code context - if len(patch) == 0 && len(commitID) > 0 { + if patch == "" && commitID != "" { patch, err = gitdiff.GeneratePatchForUnchangedLine(gitRepo, commitID, treePath, line, setting.UI.CodeCommentLines) if err != nil { // Log the error but don't fail comment creation - log.Warn("Unable to generate patch for unchanged line (file=%s, line=%d, commit=%s): %v", treePath, line, commitID, err) + log.Debug("Unable to generate patch for unchanged line (file=%s, line=%d, commit=%s): %v", treePath, line, commitID, err) } } } diff --git a/templates/repo/diff/blob_excerpt.tmpl b/templates/repo/diff/blob_excerpt.tmpl index 5839aced843b4..3b135f3faa25b 100644 --- a/templates/repo/diff/blob_excerpt.tmpl +++ b/templates/repo/diff/blob_excerpt.tmpl @@ -70,7 +70,7 @@ {{if $inlineDiff.EscapeStatus.Escaped}}{{end}} - {{- if and $.SignedUserID $.PageIsPullFiles -}} + {{- if and ctx.RootData.SignedUserID $diffBlobExcerptData.PullIssueIndex -}} diff --git a/web_src/css/review.css b/web_src/css/review.css index ad2762996499e..d19b1352082d3 100644 --- a/web_src/css/review.css +++ b/web_src/css/review.css @@ -128,7 +128,7 @@ color: var(--color-primary-contrast); border-radius: 8px !important; left: 4px; - top: 4px; + top: 6px; text-align: center; } diff --git a/web_src/js/features/repo-diff.ts b/web_src/js/features/repo-diff.ts index 1080e8665a99f..9f2465bc346ff 100644 --- a/web_src/js/features/repo-diff.ts +++ b/web_src/js/features/repo-diff.ts @@ -9,7 +9,7 @@ import {submitEventSubmitter, queryElemSiblings, hideElem, showElem, animateOnce import {POST, GET} from '../modules/fetch.ts'; import {createTippy} from '../modules/tippy.ts'; import {invertFileFolding} from './file-fold.ts'; -import {parseDom} from '../utils.ts'; +import {parseDom, sleep} from '../utils.ts'; import {registerGlobalSelectorFunc} from '../modules/observer.ts'; const {i18n} = window.config; @@ -219,40 +219,32 @@ function initRepoDiffShowMore() { } async function loadUntilFound() { - const hashTargetSelector = window.location.hash; - if (!hashTargetSelector.startsWith('#diff-') && !hashTargetSelector.startsWith('#issuecomment-')) { + const currentHash = window.location.hash; + if (!currentHash.startsWith('#diff-') && !currentHash.startsWith('#issuecomment-')) { return; } - let wasExpanded = false; - - while (true) { + while (window.location.hash === currentHash) { // use getElementById to avoid querySelector throws an error when the hash is invalid // eslint-disable-next-line unicorn/prefer-query-selector - const targetElement = document.getElementById(hashTargetSelector.substring(1)); + const targetElement = document.getElementById(currentHash.substring(1)); if (targetElement) { - // If we just expanded content, we need to re-trigger :target CSS - if (wasExpanded) { - const currentHash = window.location.hash; - window.location.hash = ''; - setTimeout(() => { - window.location.hash = currentHash; - }, 10); - } else { - targetElement.scrollIntoView(); - } + // re-trigger :target CSS and browser will scroll to the element + window.location.hash = ''; + setTimeout(() => { window.location.hash = currentHash }, 0); return; } - // If looking for a comment, try to expand the section that contains it - if (hashTargetSelector.startsWith('#issuecomment-')) { - const commentId = hashTargetSelector.substring('#issuecomment-'.length); + // If looking for a hidden comment, try to expand the section that contains it + if (currentHash.startsWith('#issuecomment-')) { + const commentId = currentHash.substring('#issuecomment-'.length); const expandButton = findExpandButtonForComment(commentId); if (expandButton) { + // Avoid infinite loop, do not re-click the button if already clicked + if (expandButton.hasAttribute('data-auto-load-clicked')) return; + expandButton.setAttribute('data-auto-load-clicked', 'true'); expandButton.click(); - wasExpanded = true; - // Wait for HTMX to load the content - await new Promise((resolve) => setTimeout(resolve, 500)); + await sleep(500); // Wait for HTMX to load the content. FIXME: need to drop htmx in the future continue; // Try again to find the element } } @@ -273,16 +265,13 @@ async function loadUntilFound() { function findExpandButtonForComment(commentId: string): HTMLElement | null { const expandButtons = document.querySelectorAll('.code-expander-button[data-hidden-comment-ids]'); for (const button of expandButtons) { - const hiddenIds = button.getAttribute('data-hidden-comment-ids'); - if (hiddenIds) { - // Parse the comma-separated list of IDs - const ids = hiddenIds.replace(/[[\]]/g, '').split(' ').filter(Boolean); - if (ids.includes(commentId)) { - return button as HTMLElement; - } + const hiddenIdsAttr = button.getAttribute('data-hidden-comment-ids'); + if (!hiddenIdsAttr) continue; + const hiddenIds = hiddenIdsAttr.split(',').filter(Boolean); + if (hiddenIds.includes(commentId)) { + return button as HTMLElement; } } - return null; } From 38d40a6dd380550348421fda7673c0a67a2dce52 Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Sat, 18 Oct 2025 23:53:23 +0800 Subject: [PATCH 13/22] fix tmpl var --- templates/repo/diff/blob_excerpt.tmpl | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/templates/repo/diff/blob_excerpt.tmpl b/templates/repo/diff/blob_excerpt.tmpl index 3b135f3faa25b..8caa620d0d476 100644 --- a/templates/repo/diff/blob_excerpt.tmpl +++ b/templates/repo/diff/blob_excerpt.tmpl @@ -1,4 +1,5 @@ {{$diffBlobExcerptData := .DiffBlobExcerptData}} +{{$canCreateComment := and ctx.RootData.SignedUserID $diffBlobExcerptData.PullIssueIndex}} {{if $.IsSplitStyle}} {{range $k, $line := $.section.Lines}} @@ -14,7 +15,7 @@ {{if and $line.LeftIdx $inlineDiff.EscapeStatus.Escaped}}{{end}} {{if $line.LeftIdx}}{{end}} - {{- if and $.SignedUserID $.PageIsPullFiles $line.LeftIdx -}} + {{- if and $canCreateComment $line.LeftIdx -}} @@ -29,7 +30,7 @@ {{if and $line.RightIdx $inlineDiff.EscapeStatus.Escaped}}{{end}} {{if $line.RightIdx}}{{end}} - {{- if and $.SignedUserID $.PageIsPullFiles $line.RightIdx -}} + {{- if and $canCreateComment $line.RightIdx -}} @@ -70,7 +71,7 @@ {{if $inlineDiff.EscapeStatus.Escaped}}{{end}} - {{- if and ctx.RootData.SignedUserID $diffBlobExcerptData.PullIssueIndex -}} + {{- if and $canCreateComment -}} From 6408c43c4a588bb655c0712d8e23c12d23e4344b Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Sat, 18 Oct 2025 23:57:26 +0800 Subject: [PATCH 14/22] add missing comment --- templates/repo/diff/section_unified.tmpl | 1 + 1 file changed, 1 insertion(+) diff --git a/templates/repo/diff/section_unified.tmpl b/templates/repo/diff/section_unified.tmpl index 094817d436366..908b14656e36e 100644 --- a/templates/repo/diff/section_unified.tmpl +++ b/templates/repo/diff/section_unified.tmpl @@ -1,4 +1,5 @@ {{$file := .file}} +{{/* this tmpl is also used by the PR Conversation page, so the "AfterCommitID" and "DiffBlobExcerptData" may not exist */}} {{$diffBlobExcerptData := $.root.DiffBlobExcerptData}} From bbd479948a073d8f9ce93b90e322476ed50a6f65 Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Sun, 19 Oct 2025 00:26:13 +0800 Subject: [PATCH 15/22] fine tune tests --- routers/web/repo/compare.go | 2 +- services/gitdiff/gitdiff.go | 4 ++-- services/gitdiff/gitdiff_test.go | 34 ++++++-------------------------- 3 files changed, 9 insertions(+), 31 deletions(-) diff --git a/routers/web/repo/compare.go b/routers/web/repo/compare.go index a64c903f984d0..0885f091a31c9 100644 --- a/routers/web/repo/compare.go +++ b/routers/web/repo/compare.go @@ -641,7 +641,7 @@ func PrepareCompareDiff( ctx.Data["DiffShortStat"] = diffShortStat ctx.Data["Diff"] = diff ctx.Data["DiffBlobExcerptData"] = &gitdiff.DiffBlobExcerptData{ - BaseLink: ctx.Repo.RepoLink + "/blob_excerpt", + BaseLink: ci.HeadRepo.Link() + "/blob_excerpt", DiffStyle: ctx.FormString("style"), AfterCommitID: headCommitID, } diff --git a/services/gitdiff/gitdiff.go b/services/gitdiff/gitdiff.go index 6272e659ad7ee..bec7a79012c9a 100644 --- a/services/gitdiff/gitdiff.go +++ b/services/gitdiff/gitdiff.go @@ -224,8 +224,8 @@ func FillHiddenCommentIDsForDiffLine(line *DiffLine, lineComments map[int64][]*i var hiddenCommentIDs []int64 for commentLineNum, comments := range lineComments { absLineNum := int(commentLineNum) - if commentLineNum < 0 { - absLineNum = int(-commentLineNum) + if absLineNum < 0 { + absLineNum = -absLineNum } // Check if comments are in the hidden range if absLineNum > line.SectionInfo.LastRightIdx && absLineNum <= line.SectionInfo.RightIdx { diff --git a/services/gitdiff/gitdiff_test.go b/services/gitdiff/gitdiff_test.go index 2464c891d1c47..42c966a789d58 100644 --- a/services/gitdiff/gitdiff_test.go +++ b/services/gitdiff/gitdiff_test.go @@ -721,7 +721,7 @@ func TestGeneratePatchForUnchangedLineFromReader(t *testing.T) { }, { name: "multi-line context", - content: "package main\n\nfunc main() {\n\tfmt.Println(\"Hello\")\n}\n", + content: "package main\n\nfunc main() {\n fmt.Println(\"Hello\")\n}\n", treePath: "main.go", line: 4, contextLines: 2, @@ -729,9 +729,9 @@ func TestGeneratePatchForUnchangedLineFromReader(t *testing.T) { --- a/main.go +++ b/main.go @@ -2,3 +2,3 @@ - + func main() { - fmt.Println("Hello") + fmt.Println("Hello") `, }, { @@ -752,7 +752,7 @@ func TestGeneratePatchForUnchangedLineFromReader(t *testing.T) { assert.Error(t, err) } else { require.NoError(t, err) - assert.Equal(t, tt.want, got) + assert.Equal(t, strings.ReplaceAll(tt.want, "", " "), got) } }) } @@ -820,38 +820,16 @@ func TestCalculateHiddenCommentIDsForLine(t *testing.T) { }, lineComments: map[int64][]*issues_model.Comment{ 10: {{ID: 100}}, // at LastRightIdx, should not be included - 20: {{ID: 101}}, // at RightIdx, should not be included + 20: {{ID: 101}}, // at RightIdx, should be included 11: {{ID: 102}}, // just inside range, should be included 19: {{ID: 103}}, // just inside range, should be included }, - expected: []int64{102, 103}, - }, - { - name: "not a section line", - line: &DiffLine{ - Type: DiffLinePlain, - }, - lineComments: map[int64][]*issues_model.Comment{ - 15: {{ID: 100}}, - }, - expected: nil, - }, - { - name: "section line without section info", - line: &DiffLine{ - Type: DiffLineSection, - SectionInfo: nil, - }, - lineComments: map[int64][]*issues_model.Comment{ - 15: {{ID: 100}}, - }, - expected: nil, + expected: []int64{101, 102, 103}, }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - tt.line.SectionInfo = &DiffLineSectionInfo{} FillHiddenCommentIDsForDiffLine(tt.line, tt.lineComments) assert.ElementsMatch(t, tt.expected, tt.line.SectionInfo.HiddenCommentIDs) }) From c0b8201a9613ab9ff158db39d0cab45a2dcaf730 Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Sun, 19 Oct 2025 01:05:03 +0800 Subject: [PATCH 16/22] remove unnecessary code --- routers/web/repo/compare.go | 3 --- routers/web/repo/compare_test.go | 36 -------------------------------- 2 files changed, 39 deletions(-) diff --git a/routers/web/repo/compare.go b/routers/web/repo/compare.go index 0885f091a31c9..e09d70f3a58ca 100644 --- a/routers/web/repo/compare.go +++ b/routers/web/repo/compare.go @@ -903,7 +903,6 @@ func ExcerptBlob(ctx *context.Context) { idxRight := ctx.FormInt("right") leftHunkSize := ctx.FormInt("left_hunk_size") rightHunkSize := ctx.FormInt("right_hunk_size") - anchor := ctx.FormString("anchor") direction := ctx.FormString("direction") filePath := ctx.FormString("path") gitRepo := ctx.Repo.GitRepo @@ -1014,8 +1013,6 @@ func ExcerptBlob(ctx *context.Context) { ctx.Data["section"] = section ctx.Data["FileNameHash"] = git.HashFilePathForWebUI(filePath) - ctx.Data["AfterCommitID"] = commitID - ctx.Data["Anchor"] = anchor ctx.Data["DiffBlobExcerptData"] = diffBlobExcerptData ctx.HTML(http.StatusOK, tplBlobExcerpt) diff --git a/routers/web/repo/compare_test.go b/routers/web/repo/compare_test.go index 56b5b842a15d1..61472dc71e2cc 100644 --- a/routers/web/repo/compare_test.go +++ b/routers/web/repo/compare_test.go @@ -12,42 +12,6 @@ import ( "github.com/stretchr/testify/assert" ) -func TestAttachHiddenCommentIDs(t *testing.T) { - section := &gitdiff.DiffSection{ - Lines: []*gitdiff.DiffLine{ - { - Type: gitdiff.DiffLineSection, - SectionInfo: &gitdiff.DiffLineSectionInfo{ - LastRightIdx: 10, - RightIdx: 20, - }, - }, - { - Type: gitdiff.DiffLinePlain, - }, - { - Type: gitdiff.DiffLineSection, - SectionInfo: &gitdiff.DiffLineSectionInfo{ - LastRightIdx: 30, - RightIdx: 40, - }, - }, - }, - } - - lineComments := map[int64][]*issues_model.Comment{ - 15: {{ID: 100}}, // in first section's hidden range - 35: {{ID: 200}}, // in second section's hidden range - 50: {{ID: 300}}, // outside any range - } - - attachHiddenCommentIDs(section, lineComments) - - assert.Equal(t, []int64{100}, section.Lines[0].SectionInfo.HiddenCommentIDs) - assert.Nil(t, section.Lines[1].SectionInfo.HiddenCommentIDs) - assert.Equal(t, []int64{200}, section.Lines[2].SectionInfo.HiddenCommentIDs) -} - func TestAttachCommentsToLines(t *testing.T) { section := &gitdiff.DiffSection{ Lines: []*gitdiff.DiffLine{ From 590df91615b65bdec2186323ad568c9f7f5d057c Mon Sep 17 00:00:00 2001 From: brymut Date: Sat, 18 Oct 2025 21:33:40 +0300 Subject: [PATCH 17/22] fix: handle righthunksize == 0 scenario --- routers/web/repo/compare_test.go | 44 +++++++++ services/gitdiff/gitdiff.go | 8 +- services/gitdiff/gitdiff_test.go | 159 +++++++++++++++++++++++++++++-- 3 files changed, 204 insertions(+), 7 deletions(-) diff --git a/routers/web/repo/compare_test.go b/routers/web/repo/compare_test.go index 61472dc71e2cc..d37a4a3a353f6 100644 --- a/routers/web/repo/compare_test.go +++ b/routers/web/repo/compare_test.go @@ -38,3 +38,47 @@ func TestAttachCommentsToLines(t *testing.T) { assert.Equal(t, int64(300), section.Lines[1].Comments[0].ID) assert.Equal(t, int64(301), section.Lines[1].Comments[1].ID) } + +func TestAttachHiddenCommentIDs(t *testing.T) { + section := &gitdiff.DiffSection{ + Lines: []*gitdiff.DiffLine{ + { + Type: gitdiff.DiffLineSection, + SectionInfo: &gitdiff.DiffLineSectionInfo{ + LastRightIdx: 10, + RightIdx: 20, + RightHunkSize: 5, // Normal expansion + }, + }, + { + Type: gitdiff.DiffLinePlain, + }, + { + Type: gitdiff.DiffLineSection, + SectionInfo: &gitdiff.DiffLineSectionInfo{ + LastRightIdx: 30, + RightIdx: 40, + RightHunkSize: 0, // End-of-file expansion + }, + }, + }, + } + + lineComments := map[int64][]*issues_model.Comment{ + 15: {{ID: 100}}, // in first section's hidden range + 35: {{ID: 200}}, // in second section's hidden range + 40: {{ID: 300}}, // at second section's RightIdx (end-of-file, should be included) + 50: {{ID: 400}}, // outside any range + } + + attachHiddenCommentIDs(section, lineComments) + + // First section: normal expansion, RightIdx is exclusive + assert.Equal(t, []int64{100}, section.Lines[0].SectionInfo.HiddenCommentIDs) + + // Second line is not a section + assert.Nil(t, section.Lines[1].SectionInfo) + + // Third section: end-of-file expansion, RightIdx is inclusive + assert.ElementsMatch(t, []int64{200, 300}, section.Lines[2].SectionInfo.HiddenCommentIDs) +} diff --git a/services/gitdiff/gitdiff.go b/services/gitdiff/gitdiff.go index bec7a79012c9a..ec49b776b236d 100644 --- a/services/gitdiff/gitdiff.go +++ b/services/gitdiff/gitdiff.go @@ -228,7 +228,13 @@ func FillHiddenCommentIDsForDiffLine(line *DiffLine, lineComments map[int64][]*i absLineNum = -absLineNum } // Check if comments are in the hidden range - if absLineNum > line.SectionInfo.LastRightIdx && absLineNum <= line.SectionInfo.RightIdx { + // depending on 'end-of-file' expansion might be RightHunkSize = 0 + isEndOfFileExpansion := line.SectionInfo.RightHunkSize == 0 + inRange := absLineNum > line.SectionInfo.LastRightIdx && + (isEndOfFileExpansion && absLineNum <= line.SectionInfo.RightIdx || + !isEndOfFileExpansion && absLineNum < line.SectionInfo.RightIdx) + + if inRange { for _, comment := range comments { hiddenCommentIDs = append(hiddenCommentIDs, comment.ID) } diff --git a/services/gitdiff/gitdiff_test.go b/services/gitdiff/gitdiff_test.go index 42c966a789d58..9a66016783409 100644 --- a/services/gitdiff/gitdiff_test.go +++ b/services/gitdiff/gitdiff_test.go @@ -810,21 +810,57 @@ func TestCalculateHiddenCommentIDsForLine(t *testing.T) { expected: []int64{100}, }, { - name: "boundary conditions - exclusive range", + name: "boundary conditions - normal expansion (both boundaries exclusive)", line: &DiffLine{ Type: DiffLineSection, SectionInfo: &DiffLineSectionInfo{ - LastRightIdx: 10, - RightIdx: 20, + LastRightIdx: 10, + RightIdx: 20, + RightHunkSize: 5, // Normal case: next section has content }, }, lineComments: map[int64][]*issues_model.Comment{ - 10: {{ID: 100}}, // at LastRightIdx, should not be included - 20: {{ID: 101}}, // at RightIdx, should be included + 10: {{ID: 100}}, // at LastRightIdx (visible line), should NOT be included + 20: {{ID: 101}}, // at RightIdx (visible line), should NOT be included 11: {{ID: 102}}, // just inside range, should be included 19: {{ID: 103}}, // just inside range, should be included }, - expected: []int64{101, 102, 103}, + expected: []int64{102, 103}, + }, + { + name: "boundary conditions - end of file expansion (RightIdx inclusive)", + line: &DiffLine{ + Type: DiffLineSection, + SectionInfo: &DiffLineSectionInfo{ + LastRightIdx: 54, + RightIdx: 70, + RightHunkSize: 0, // End of file: no more content after + }, + }, + lineComments: map[int64][]*issues_model.Comment{ + 54: {{ID: 54}}, // at LastRightIdx (visible line), should NOT be included + 70: {{ID: 70}}, // at RightIdx (last hidden line), SHOULD be included + 60: {{ID: 60}}, // inside range, should be included + }, + expected: []int64{60, 70}, // Lines 60 and 70 are hidden + }, + { + name: "real-world scenario - start of file with hunk", + line: &DiffLine{ + Type: DiffLineSection, + SectionInfo: &DiffLineSectionInfo{ + LastRightIdx: 0, // No previous visible section + RightIdx: 26, // Line 26 is first visible line of hunk + RightHunkSize: 9, // Normal hunk with content + }, + }, + lineComments: map[int64][]*issues_model.Comment{ + 1: {{ID: 1}}, // Line 1 is hidden + 26: {{ID: 26}}, // Line 26 is visible (hunk start) - should NOT be hidden + 10: {{ID: 10}}, // Line 10 is hidden + 15: {{ID: 15}}, // Line 15 is hidden + }, + expected: []int64{1, 10, 15}, // Lines 1, 10, 15 are hidden; line 26 is visible }, } @@ -835,3 +871,114 @@ func TestCalculateHiddenCommentIDsForLine(t *testing.T) { }) } } + +func TestDiffLine_RenderBlobExcerptButtons(t *testing.T) { + tests := []struct { + name string + line *DiffLine + fileNameHash string + data *DiffBlobExcerptData + expectContains []string + expectNotContain []string + }{ + { + name: "expand up button with hidden comments", + line: &DiffLine{ + Type: DiffLineSection, + SectionInfo: &DiffLineSectionInfo{ + LastRightIdx: 0, + RightIdx: 26, + LeftIdx: 26, + LastLeftIdx: 0, + LeftHunkSize: 0, + RightHunkSize: 0, + HiddenCommentIDs: []int64{100}, + }, + }, + fileNameHash: "abc123", + data: &DiffBlobExcerptData{ + BaseLink: "/repo/blob_excerpt", + AfterCommitID: "commit123", + DiffStyle: "unified", + }, + expectContains: []string{ + "octicon-fold-up", + "direction=up", + "code-comment-more", + "1 hidden comment(s)", + }, + }, + { + name: "expand up and down buttons with pull request", + line: &DiffLine{ + Type: DiffLineSection, + SectionInfo: &DiffLineSectionInfo{ + LastRightIdx: 10, + RightIdx: 50, + LeftIdx: 10, + LastLeftIdx: 5, + LeftHunkSize: 5, + RightHunkSize: 5, + HiddenCommentIDs: []int64{200, 201}, + }, + }, + fileNameHash: "def456", + data: &DiffBlobExcerptData{ + BaseLink: "/repo/blob_excerpt", + AfterCommitID: "commit456", + DiffStyle: "split", + PullIssueIndex: 42, + }, + expectContains: []string{ + "octicon-fold-down", + "octicon-fold-up", + "direction=down", + "direction=up", + "data-hidden-comment-ids=\"200,201\"", + "pull_issue_index=42", + "2 hidden comment(s)", + }, + }, + { + name: "no hidden comments", + line: &DiffLine{ + Type: DiffLineSection, + SectionInfo: &DiffLineSectionInfo{ + LastRightIdx: 10, + RightIdx: 20, + LeftIdx: 10, + LastLeftIdx: 5, + LeftHunkSize: 5, + RightHunkSize: 5, + HiddenCommentIDs: nil, + }, + }, + fileNameHash: "ghi789", + data: &DiffBlobExcerptData{ + BaseLink: "/repo/blob_excerpt", + AfterCommitID: "commit789", + }, + expectContains: []string{ + "code-expander-button", + }, + expectNotContain: []string{ + "code-comment-more", + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result := tt.line.RenderBlobExcerptButtons(tt.fileNameHash, tt.data) + resultStr := string(result) + + for _, expected := range tt.expectContains { + assert.Contains(t, resultStr, expected, "Expected to contain: %s", expected) + } + + for _, notExpected := range tt.expectNotContain { + assert.NotContains(t, resultStr, notExpected, "Expected NOT to contain: %s", notExpected) + } + }) + } +} From d1fc0d7684d0e8096a80b26335e495254d50be78 Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Sun, 19 Oct 2025 12:15:55 +0800 Subject: [PATCH 18/22] fine tune --- routers/web/repo/compare.go | 5 +++ services/gitdiff/gitdiff.go | 2 +- .../repo/issue/view_content/conversation.tmpl | 5 +++ web_src/css/review.css | 2 +- web_src/js/features/repo-diff.ts | 39 ++++++++++++------- 5 files changed, 36 insertions(+), 17 deletions(-) diff --git a/routers/web/repo/compare.go b/routers/web/repo/compare.go index e09d70f3a58ca..08bd0a7e74c17 100644 --- a/routers/web/repo/compare.go +++ b/routers/web/repo/compare.go @@ -995,10 +995,15 @@ func ExcerptBlob(ctx *context.Context) { if err != nil { log.Error("GetIssueByIndex error: %v", err) } else if issue.IsPull { + // FIXME: DIFF-CONVERSATION-DATA: the following data assignment is fragile ctx.Data["Issue"] = issue ctx.Data["CanBlockUser"] = func(blocker, blockee *user_model.User) bool { return user_service.CanBlockUser(ctx, ctx.Doer, blocker, blockee) } + // and "diff/comment_form.tmpl" (reply comment) needs them + ctx.Data["PageIsPullFiles"] = true + ctx.Data["AfterCommitID"] = diffBlobExcerptData.AfterCommitID + allComments, err := issues_model.FetchCodeComments(ctx, issue, ctx.Doer, ctx.FormBool("show_outdated")) if err != nil { log.Error("FetchCodeComments error: %v", err) diff --git a/services/gitdiff/gitdiff.go b/services/gitdiff/gitdiff.go index ec49b776b236d..41029f36f7dd9 100644 --- a/services/gitdiff/gitdiff.go +++ b/services/gitdiff/gitdiff.go @@ -191,7 +191,7 @@ func (d *DiffLine) RenderBlobExcerptButtons(fileNameHash string, data *DiffBlobE link += fmt.Sprintf("&pull_issue_index=%d", data.PullIssueIndex) } return htmlutil.HTMLFormat( - ``, + ``, link, dataHiddenCommentIDs, svg.RenderHTML(svgName), ) } diff --git a/templates/repo/issue/view_content/conversation.tmpl b/templates/repo/issue/view_content/conversation.tmpl index 189b9d62597d5..01261b0a56dcc 100644 --- a/templates/repo/issue/view_content/conversation.tmpl +++ b/templates/repo/issue/view_content/conversation.tmpl @@ -1,3 +1,8 @@ +{{/* FIXME: DIFF-CONVERSATION-DATA: in the future this template should be refactor to avoid called by {{... "." $}} +At the moment, two kinds of request handler call this template: +* ExcerptBlob -> blob_excerpt.tmpl -> this +* Other compare and diff pages -> ... -> {section_unified.tmpl|section_split.tmpl} -> this) +The variables in "ctx.Data" are different in each case, making this template fragile, hard to read and maintain. */}} {{if len .comments}} {{$comment := index .comments 0}} {{$invalid := $comment.Invalidated}} diff --git a/web_src/css/review.css b/web_src/css/review.css index d19b1352082d3..39916d1bd857c 100644 --- a/web_src/css/review.css +++ b/web_src/css/review.css @@ -127,7 +127,7 @@ background-color: var(--color-primary); color: var(--color-primary-contrast); border-radius: 8px !important; - left: 4px; + left: -12px; top: 6px; text-align: center; } diff --git a/web_src/js/features/repo-diff.ts b/web_src/js/features/repo-diff.ts index 9f2465bc346ff..a4cc756c350dc 100644 --- a/web_src/js/features/repo-diff.ts +++ b/web_src/js/features/repo-diff.ts @@ -218,31 +218,40 @@ function initRepoDiffShowMore() { }); } -async function loadUntilFound() { +async function onLocationHashChange() { + // try to scroll to the target element by the current hash const currentHash = window.location.hash; - if (!currentHash.startsWith('#diff-') && !currentHash.startsWith('#issuecomment-')) { - return; - } + if (!currentHash.startsWith('#diff-') && !currentHash.startsWith('#issuecomment-')) return; + + // avoid reentrance when we are changing the hash to scroll and trigger ":target" selection + const attrAutoScrollRunning = 'data-auto-scroll-running'; + if (document.body.hasAttribute(attrAutoScrollRunning)) return; - while (window.location.hash === currentHash) { + const targetElementId = currentHash.substring(1); + while (currentHash === window.location.hash) { // use getElementById to avoid querySelector throws an error when the hash is invalid // eslint-disable-next-line unicorn/prefer-query-selector - const targetElement = document.getElementById(currentHash.substring(1)); + const targetElement = document.getElementById(targetElementId); if (targetElement) { - // re-trigger :target CSS and browser will scroll to the element + // need to change hash to re-trigger ":target" CSS selector, let's manually scroll to it + targetElement.scrollIntoView(); + document.body.setAttribute(attrAutoScrollRunning, 'true'); window.location.hash = ''; - setTimeout(() => { window.location.hash = currentHash }, 0); + window.location.hash = currentHash; + setTimeout(() => document.body.removeAttribute(attrAutoScrollRunning), 0); return; } // If looking for a hidden comment, try to expand the section that contains it - if (currentHash.startsWith('#issuecomment-')) { - const commentId = currentHash.substring('#issuecomment-'.length); + const issueCommentPrefix = '#issuecomment-'; + if (currentHash.startsWith(issueCommentPrefix)) { + const commentId = currentHash.substring(issueCommentPrefix.length); const expandButton = findExpandButtonForComment(commentId); if (expandButton) { - // Avoid infinite loop, do not re-click the button if already clicked - if (expandButton.hasAttribute('data-auto-load-clicked')) return; - expandButton.setAttribute('data-auto-load-clicked', 'true'); + // avoid infinite loop, do not re-click the button if already clicked + const attrAutoLoadClicked = 'data-auto-load-clicked'; + if (expandButton.hasAttribute(attrAutoLoadClicked)) return; + expandButton.setAttribute(attrAutoLoadClicked, 'true'); expandButton.click(); await sleep(500); // Wait for HTMX to load the content. FIXME: need to drop htmx in the future continue; // Try again to find the element @@ -276,8 +285,8 @@ function findExpandButtonForComment(commentId: string): HTMLElement | null { } function initRepoDiffHashChangeListener() { - window.addEventListener('hashchange', loadUntilFound); - loadUntilFound(); + window.addEventListener('hashchange', onLocationHashChange); + onLocationHashChange(); } export function initRepoDiffView() { From 9356dbe6c7119b934151b29883584c243cd90e81 Mon Sep 17 00:00:00 2001 From: brymut Date: Sun, 19 Oct 2025 10:41:52 +0300 Subject: [PATCH 19/22] fix: remove reduntant test --- routers/web/repo/compare_test.go | 44 -------------------------------- 1 file changed, 44 deletions(-) diff --git a/routers/web/repo/compare_test.go b/routers/web/repo/compare_test.go index d37a4a3a353f6..61472dc71e2cc 100644 --- a/routers/web/repo/compare_test.go +++ b/routers/web/repo/compare_test.go @@ -38,47 +38,3 @@ func TestAttachCommentsToLines(t *testing.T) { assert.Equal(t, int64(300), section.Lines[1].Comments[0].ID) assert.Equal(t, int64(301), section.Lines[1].Comments[1].ID) } - -func TestAttachHiddenCommentIDs(t *testing.T) { - section := &gitdiff.DiffSection{ - Lines: []*gitdiff.DiffLine{ - { - Type: gitdiff.DiffLineSection, - SectionInfo: &gitdiff.DiffLineSectionInfo{ - LastRightIdx: 10, - RightIdx: 20, - RightHunkSize: 5, // Normal expansion - }, - }, - { - Type: gitdiff.DiffLinePlain, - }, - { - Type: gitdiff.DiffLineSection, - SectionInfo: &gitdiff.DiffLineSectionInfo{ - LastRightIdx: 30, - RightIdx: 40, - RightHunkSize: 0, // End-of-file expansion - }, - }, - }, - } - - lineComments := map[int64][]*issues_model.Comment{ - 15: {{ID: 100}}, // in first section's hidden range - 35: {{ID: 200}}, // in second section's hidden range - 40: {{ID: 300}}, // at second section's RightIdx (end-of-file, should be included) - 50: {{ID: 400}}, // outside any range - } - - attachHiddenCommentIDs(section, lineComments) - - // First section: normal expansion, RightIdx is exclusive - assert.Equal(t, []int64{100}, section.Lines[0].SectionInfo.HiddenCommentIDs) - - // Second line is not a section - assert.Nil(t, section.Lines[1].SectionInfo) - - // Third section: end-of-file expansion, RightIdx is inclusive - assert.ElementsMatch(t, []int64{200, 300}, section.Lines[2].SectionInfo.HiddenCommentIDs) -} From 78cd2ce5858e93ca19e398f81011dab85502518f Mon Sep 17 00:00:00 2001 From: brymut Date: Sun, 19 Oct 2025 11:55:55 +0300 Subject: [PATCH 20/22] fix(gitdiff): fix line number handling in `FillHiddenCommentIDsForDiffLine` --- services/gitdiff/gitdiff.go | 14 ++++++-------- services/gitdiff/gitdiff_test.go | 5 +++-- 2 files changed, 9 insertions(+), 10 deletions(-) diff --git a/services/gitdiff/gitdiff.go b/services/gitdiff/gitdiff.go index 41029f36f7dd9..d36fef1e4d47f 100644 --- a/services/gitdiff/gitdiff.go +++ b/services/gitdiff/gitdiff.go @@ -223,16 +223,14 @@ func FillHiddenCommentIDsForDiffLine(line *DiffLine, lineComments map[int64][]*i var hiddenCommentIDs []int64 for commentLineNum, comments := range lineComments { - absLineNum := int(commentLineNum) - if absLineNum < 0 { - absLineNum = -absLineNum + if commentLineNum < 0 { + continue // Skip left-side } - // Check if comments are in the hidden range - // depending on 'end-of-file' expansion might be RightHunkSize = 0 + lineNum := int(commentLineNum) isEndOfFileExpansion := line.SectionInfo.RightHunkSize == 0 - inRange := absLineNum > line.SectionInfo.LastRightIdx && - (isEndOfFileExpansion && absLineNum <= line.SectionInfo.RightIdx || - !isEndOfFileExpansion && absLineNum < line.SectionInfo.RightIdx) + inRange := lineNum > line.SectionInfo.LastRightIdx && + (isEndOfFileExpansion && lineNum <= line.SectionInfo.RightIdx || + !isEndOfFileExpansion && lineNum < line.SectionInfo.RightIdx) if inRange { for _, comment := range comments { diff --git a/services/gitdiff/gitdiff_test.go b/services/gitdiff/gitdiff_test.go index 9a66016783409..75093e3edc2cf 100644 --- a/services/gitdiff/gitdiff_test.go +++ b/services/gitdiff/gitdiff_test.go @@ -805,9 +805,10 @@ func TestCalculateHiddenCommentIDsForLine(t *testing.T) { }, }, lineComments: map[int64][]*issues_model.Comment{ - -15: {{ID: 100}}, + -15: {{ID: 100}}, // Left-side comment, should NOT be counted + 15: {{ID: 101}}, // Right-side comment, should be counted }, - expected: []int64{100}, + expected: []int64{101}, // Only right-side comment }, { name: "boundary conditions - normal expansion (both boundaries exclusive)", From bb714b4dd99bae603ccf36b834eca99f1f6bd7a1 Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Sun, 19 Oct 2025 17:27:42 +0800 Subject: [PATCH 21/22] fine tune --- services/gitdiff/gitdiff.go | 4 ++-- services/gitdiff/gitdiff_test.go | 2 +- web_src/js/features/repo-diff.ts | 16 +--------------- 3 files changed, 4 insertions(+), 18 deletions(-) diff --git a/services/gitdiff/gitdiff.go b/services/gitdiff/gitdiff.go index d36fef1e4d47f..fa9e0e7c66c1a 100644 --- a/services/gitdiff/gitdiff.go +++ b/services/gitdiff/gitdiff.go @@ -191,7 +191,7 @@ func (d *DiffLine) RenderBlobExcerptButtons(fileNameHash string, data *DiffBlobE link += fmt.Sprintf("&pull_issue_index=%d", data.PullIssueIndex) } return htmlutil.HTMLFormat( - ``, + ``, link, dataHiddenCommentIDs, svg.RenderHTML(svgName), ) } @@ -224,7 +224,7 @@ func FillHiddenCommentIDsForDiffLine(line *DiffLine, lineComments map[int64][]*i var hiddenCommentIDs []int64 for commentLineNum, comments := range lineComments { if commentLineNum < 0 { - continue // Skip left-side + continue // Skip left-side, unchanged lines always use "right (proposed)" side for comments } lineNum := int(commentLineNum) isEndOfFileExpansion := line.SectionInfo.RightHunkSize == 0 diff --git a/services/gitdiff/gitdiff_test.go b/services/gitdiff/gitdiff_test.go index 75093e3edc2cf..51fb9b58d60e2 100644 --- a/services/gitdiff/gitdiff_test.go +++ b/services/gitdiff/gitdiff_test.go @@ -935,7 +935,7 @@ func TestDiffLine_RenderBlobExcerptButtons(t *testing.T) { "octicon-fold-up", "direction=down", "direction=up", - "data-hidden-comment-ids=\"200,201\"", + `data-hidden-comment-ids=",200,201,"`, // use leading and trailing commas to ensure exact match by CSS selector `attr*=",id,"` "pull_issue_index=42", "2 hidden comment(s)", }, diff --git a/web_src/js/features/repo-diff.ts b/web_src/js/features/repo-diff.ts index a4cc756c350dc..24d937a252818 100644 --- a/web_src/js/features/repo-diff.ts +++ b/web_src/js/features/repo-diff.ts @@ -246,7 +246,7 @@ async function onLocationHashChange() { const issueCommentPrefix = '#issuecomment-'; if (currentHash.startsWith(issueCommentPrefix)) { const commentId = currentHash.substring(issueCommentPrefix.length); - const expandButton = findExpandButtonForComment(commentId); + const expandButton = document.querySelector(`.code-expander-button[data-hidden-comment-ids*=",${commentId},"]`); if (expandButton) { // avoid infinite loop, do not re-click the button if already clicked const attrAutoLoadClicked = 'data-auto-load-clicked'; @@ -270,20 +270,6 @@ async function onLocationHashChange() { } } -// Find the expand button that contains the target comment ID -function findExpandButtonForComment(commentId: string): HTMLElement | null { - const expandButtons = document.querySelectorAll('.code-expander-button[data-hidden-comment-ids]'); - for (const button of expandButtons) { - const hiddenIdsAttr = button.getAttribute('data-hidden-comment-ids'); - if (!hiddenIdsAttr) continue; - const hiddenIds = hiddenIdsAttr.split(',').filter(Boolean); - if (hiddenIds.includes(commentId)) { - return button as HTMLElement; - } - } - return null; -} - function initRepoDiffHashChangeListener() { window.addEventListener('hashchange', onLocationHashChange); onLocationHashChange(); From af864c2dd05bd20f239d19e14be7107ade81ad20 Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Sun, 19 Oct 2025 17:48:59 +0800 Subject: [PATCH 22/22] fix assumption --- services/gitdiff/gitdiff.go | 3 ++- templates/repo/diff/blob_excerpt.tmpl | 5 +++-- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/services/gitdiff/gitdiff.go b/services/gitdiff/gitdiff.go index fa9e0e7c66c1a..96aea8308c92a 100644 --- a/services/gitdiff/gitdiff.go +++ b/services/gitdiff/gitdiff.go @@ -224,7 +224,8 @@ func FillHiddenCommentIDsForDiffLine(line *DiffLine, lineComments map[int64][]*i var hiddenCommentIDs []int64 for commentLineNum, comments := range lineComments { if commentLineNum < 0 { - continue // Skip left-side, unchanged lines always use "right (proposed)" side for comments + // ATTENTION: BLOB-EXCERPT-COMMENT-RIGHT: skip left-side, unchanged lines always use "right (proposed)" side for comments + continue } lineNum := int(commentLineNum) isEndOfFileExpansion := line.SectionInfo.RightHunkSize == 0 diff --git a/templates/repo/diff/blob_excerpt.tmpl b/templates/repo/diff/blob_excerpt.tmpl index 8caa620d0d476..c9aac6d61d84c 100644 --- a/templates/repo/diff/blob_excerpt.tmpl +++ b/templates/repo/diff/blob_excerpt.tmpl @@ -15,8 +15,9 @@ {{if and $line.LeftIdx $inlineDiff.EscapeStatus.Escaped}}{{end}} {{if $line.LeftIdx}}{{end}} - {{- if and $canCreateComment $line.LeftIdx -}} - {{- end -}}