Skip to content

Add inline code comments on commits (#4898) #35589

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 3 commits into from
+346 −12
Closed

Add inline code comments on commits (#4898) #35589

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a706a04
Add inline code comments on commits
ktamas77 Oct 5, 2025
7ae6e20
Use separate CommentTypeCommitCode for commit inline comments
ktamas77 Oct 6, 2025
aeafe3d
Optimize FindCommitLineComments to filter in memory
ktamas77 Oct 19, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
60 changes: 54 additions & 6 deletions models/issues/comment.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,8 @@ const (
CommentTypeUnpin // 37 unpin Issue/PullRequest

CommentTypeChangeTimeEstimate // 38 Change time estimate

CommentTypeCommitCode // 39 Comment a line of code in a commit (not part of a pull request)
)

var commentStrings = []string{
Expand Down Expand Up @@ -157,6 +159,7 @@ var commentStrings = []string{
"pin",
"unpin",
"change_time_estimate",
"commit_code",
}

func (t CommentType) String() string {
Expand All @@ -174,23 +177,23 @@ func AsCommentType(typeName string) CommentType {

func (t CommentType) HasContentSupport() bool {
switch t {
case CommentTypeComment, CommentTypeCode, CommentTypeReview, CommentTypeDismissReview:
case CommentTypeComment, CommentTypeCode, CommentTypeReview, CommentTypeDismissReview, CommentTypeCommitCode:
return true
}
return false
}

func (t CommentType) HasAttachmentSupport() bool {
switch t {
case CommentTypeComment, CommentTypeCode, CommentTypeReview:
case CommentTypeComment, CommentTypeCode, CommentTypeReview, CommentTypeCommitCode:
return true
}
return false
}

func (t CommentType) HasMailReplySupport() bool {
switch t {
case CommentTypeComment, CommentTypeCode, CommentTypeReview, CommentTypeDismissReview, CommentTypeReopen, CommentTypeClose, CommentTypeMergePull, CommentTypeAssignees:
case CommentTypeComment, CommentTypeCode, CommentTypeReview, CommentTypeDismissReview, CommentTypeReopen, CommentTypeClose, CommentTypeMergePull, CommentTypeAssignees, CommentTypeCommitCode:
return true
}
return false
Expand Down Expand Up @@ -447,6 +450,9 @@ func (c *Comment) hashLink(ctx context.Context) string {
return "/files#" + c.HashTag()
}
}
if c.Type == CommentTypeCommitCode {
return "/files#" + c.HashTag()
}
return "#" + c.HashTag()
}

Expand Down Expand Up @@ -657,9 +663,9 @@ func (c *Comment) LoadAssigneeUserAndTeam(ctx context.Context) error {
return nil
}

// LoadResolveDoer if comment.Type is CommentTypeCode and ResolveDoerID not zero, then load resolveDoer
// LoadResolveDoer if comment.Type is CommentTypeCode or CommentTypeCommitCode and ResolveDoerID not zero, then load resolveDoer
func (c *Comment) LoadResolveDoer(ctx context.Context) (err error) {
if c.ResolveDoerID == 0 || c.Type != CommentTypeCode {
if c.ResolveDoerID == 0 || (c.Type != CommentTypeCode && c.Type != CommentTypeCommitCode) {
return nil
}
c.ResolveDoer, err = user_model.GetUserByID(ctx, c.ResolveDoerID)
Expand All @@ -674,7 +680,7 @@ func (c *Comment) LoadResolveDoer(ctx context.Context) (err error) {

// IsResolved check if an code comment is resolved
func (c *Comment) IsResolved() bool {
return c.ResolveDoerID != 0 && c.Type == CommentTypeCode
return c.ResolveDoerID != 0 && (c.Type == CommentTypeCode || c.Type == CommentTypeCommitCode)
}

// LoadDepIssueDetails loads Dependent Issue Details
Expand Down Expand Up @@ -862,6 +868,12 @@ func updateCommentInfos(ctx context.Context, opts *CreateCommentOptions, comment
if err = UpdateCommentAttachments(ctx, comment, opts.Attachments); err != nil {
return err
}
case CommentTypeCommitCode:
if err = UpdateCommentAttachments(ctx, comment, opts.Attachments); err != nil {
return err
}
// Commit comments don't have an associated issue, so just return here
return nil
case CommentTypeReopen, CommentTypeClose:
if err = repo_model.UpdateRepoIssueNumbers(ctx, opts.Issue.RepoID, opts.Issue.IsPull, true); err != nil {
return err
Expand Down Expand Up @@ -1076,6 +1088,42 @@ func CountComments(ctx context.Context, opts *FindCommentsOptions) (int64, error
return sess.Count(&Comment{})
}

// FindCommitComments finds all code comments for a specific commit
func FindCommitComments(ctx context.Context, repoID int64, commitSHA string) (CommentList, error) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there an index for the query?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently there's an index on type but not on commit_sha. This function would benefit from an index on commit_sha (or a composite index on (commit_sha, type)) since it queries by that field directly. Should I add one in a migration?

comments := make([]*Comment, 0, 10)
return comments, db.GetEngine(ctx).
Where("commit_sha = ?", commitSHA).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

repo_id was missed.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch! Currently, commit comments don't have a repo_id field in the table - they only store commit_sha. This means we can't filter by repository in the current schema.

We have a few options:

  1. Add a repo_id field to the comment table (requires migration)
  2. Remove the repoID parameter since it can't be used
  3. Accept that the same commit_sha across forks would show all comments

What would you prefer?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

3. Accept that the same commit_sha across forks would show all comments

I don't think this is accepted. So that I think repo_id is a MUST. Maybe we can have a new table for commit_comment which have a repo_id and a comment_id. So that we don't need to add a new column for comment table.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good - a separate commit_comment table would be cleaner. So the structure would be:

New commit_comment table:

  • id (pk)
  • repo_id (indexed)
  • comment_id (indexed, foreign key to comment.id)
  • commit_sha (indexed)
  • Composite index on (repo_id, commit_sha)

Then when querying, we'd join with this table to filter by repo. This keeps the comment table unchanged and makes the commit-specific metadata explicit.

Should I proceed with implementing this approach?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds better than before. But I don't know wether it could be accepted by other @go-gitea/maintainers .

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see - this is a deeper architectural issue. The attachment system expects bindings to issues/releases, so commit comment attachments (without issue_id) would be incorrectly deleted as orphaned.

Given the complexity here, I'll wait for maintainer consensus on the best approach before proceeding. Please tag me when there's a decision on how to handle this properly.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@delvh Dude....

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm out. Solve it for yourself

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good luck!

Copy link
Contributor

@wxiaoguang wxiaoguang Oct 20, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the misunderstanding. Maybe some messages read too like "vibe coding" and there were a lot of AI PRs from other contributors 🤣 Apologize again.

By the way I am not a native speaker, so please point out if my comment doesn't read good.

By your analysis, I think I can see a potential feasible approach now, although not completely clear yet, share some points:

  1. The repo_id is still necessary, I think only the commit comments from origin repo can be edited or replied.
  2. Adding a new commit_comment table seems the only solution . I don't think Comment.CommitSHA can be reused since it is use for "CommentTypeCommitRef" and there is no index and it's not suitable to add an index for it
  3. DeleteOrphanedAttachments should be updated to respect "comment_id"
  4. How to "reply" a comment? Maybe commit_comment.parent_comment_id?

And("type = ?", CommentTypeCommitCode).
Asc("created_unix").
Asc("id").
Find(&comments)
}

// FindCommitLineComments finds code comments for a specific file and line in a commit
func FindCommitLineComments(ctx context.Context, commitSHA, treePath string) (CommentList, error) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's better to filter by tree path and line in the memory otherwise, we need a new index for the database.

Copy link
Author

@ktamas77 ktamas77 Oct 19, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've updated FindCommitLineComments to filter by tree path in memory instead of in the database query, avoiding the need for a new index on tree_path. However, both this function and FindCommitComments would still benefit from an index on commit_sha since they query by that field.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need a new composite index repo_id and commit_sha and the issue_id should be zero. Because it maybe conflicted with a pull request's commit_sha.

Copy link
Author

@ktamas77 ktamas77 Oct 19, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently the Comment table doesn't have a repo_id field. To implement this, I would need to:

  1. Add a repo_id column to the comment table (migration)
  2. Create a composite index on (repo_id, commit_sha)
  3. Ensure issue_id = 0 for standalone commit comments
  4. Update CreateComment to populate repo_id
  5. Update FindCommitComments and FindCommitLineComments to filter by repo_id

Should I proceed with creating this migration?

// Fetch all commit code comments for this commit (filter by tree path in memory to avoid needing a new index)
allComments := make([]*Comment, 0, 10)
err := db.GetEngine(ctx).
Where("commit_sha = ?", commitSHA).
And("type = ?", CommentTypeCommitCode).
Asc("created_unix").
Asc("id").
Find(&allComments)
if err != nil {
return nil, err
}

// Filter in memory by tree path
comments := make(CommentList, 0, len(allComments))
for _, comment := range allComments {
if comment.TreePath == treePath {
comments = append(comments, comment)
}
}

return comments, nil
}

// UpdateCommentInvalidate updates comment invalidated column
func UpdateCommentInvalidate(ctx context.Context, c *Comment) error {
_, err := db.GetEngine(ctx).ID(c.ID).Cols("invalidated").Update(c)
Expand Down
154 changes: 154 additions & 0 deletions routers/web/repo/commit.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,11 @@ import (
"code.gitea.io/gitea/modules/setting"
"code.gitea.io/gitea/modules/templates"
"code.gitea.io/gitea/modules/util"
"code.gitea.io/gitea/modules/web"
asymkey_service "code.gitea.io/gitea/services/asymkey"
"code.gitea.io/gitea/services/context"
"code.gitea.io/gitea/services/context/upload"
"code.gitea.io/gitea/services/forms"
git_service "code.gitea.io/gitea/services/git"
"code.gitea.io/gitea/services/gitdiff"
repo_service "code.gitea.io/gitea/services/repository"
Expand Down Expand Up @@ -417,6 +420,25 @@ func Diff(ctx *context.Context) {
ctx.Data["MergedPRIssueNumber"] = pr.Index
}

// Load commit comments for inline display
comments, err := issues_model.FindCommitComments(ctx, ctx.Repo.Repository.ID, commitID)
if err != nil {
log.Error("FindCommitComments: %v", err)
} else {
if err := comments.LoadPosters(ctx); err != nil {
log.Error("LoadPosters: %v", err)
}
if err := comments.LoadAttachments(ctx); err != nil {
log.Error("LoadAttachments: %v", err)
}
ctx.Data["CommitComments"] = comments
}

// Mark this as a commit page to enable comment UI
ctx.Data["PageIsCommit"] = true
ctx.Data["IsAttachmentEnabled"] = setting.Attachment.Enabled
upload.AddUploadContext(ctx, "comment")

ctx.HTML(http.StatusOK, tplCommitPage)
}

Expand Down Expand Up @@ -469,3 +491,135 @@ func processGitCommits(ctx *context.Context, gitCommits []*git.Commit) ([]*git_m
}
return commits, nil
}

// RenderNewCommitCodeCommentForm renders the form for creating a new commit code comment
func RenderNewCommitCodeCommentForm(ctx *context.Context) {
ctx.Data["PageIsCommit"] = true
ctx.Data["AfterCommitID"] = ctx.PathParam("sha")
ctx.Data["IsAttachmentEnabled"] = setting.Attachment.Enabled
upload.AddUploadContext(ctx, "comment")
// Use the same template as PR new comments (defined in pull_review.go)
ctx.HTML(http.StatusOK, "repo/diff/new_comment")
}

// CreateCommitCodeComment creates an inline comment on a commit
func CreateCommitCodeComment(ctx *context.Context) {
form := web.GetForm(ctx).(*forms.CodeCommentForm)
commitSHA := ctx.PathParam("sha")

if ctx.Written() {
return
}

if ctx.HasError() {
ctx.Flash.Error(ctx.Data["ErrorMsg"].(string))
ctx.Redirect(fmt.Sprintf("%s/commit/%s", ctx.Repo.RepoLink, commitSHA))
return
}

// Convert line to signed line (negative for previous side)
signedLine := form.Line
if form.Side == "previous" {
signedLine *= -1
}

var attachments []string
if setting.Attachment.Enabled {
attachments = form.Files
}

// Create the comment using the service layer
comment, err := repo_service.CreateCommitCodeComment(
ctx,
ctx.Doer,
ctx.Repo.Repository,
ctx.Repo.GitRepo,
commitSHA,
signedLine,
form.Content,
form.TreePath,
attachments,
)
if err != nil {
ctx.ServerError("CreateCommitCodeComment", err)
return
}

log.Trace("Commit comment created: %d for commit %s in %-v", comment.ID, commitSHA, ctx.Repo.Repository)

// Render the comment
ctx.Data["Comment"] = comment
ctx.Data["IsAttachmentEnabled"] = setting.Attachment.Enabled
upload.AddUploadContext(ctx, "comment")

ctx.JSON(http.StatusOK, map[string]any{
"ok": true,
"comment": comment,
})
}

// UpdateCommitCodeComment updates an existing commit inline comment
func UpdateCommitCodeComment(ctx *context.Context) {
form := web.GetForm(ctx).(*forms.CodeCommentForm)
commentID := ctx.PathParamInt64(":id")

comment, err := issues_model.GetCommentByID(ctx, commentID)
if err != nil {
ctx.ServerError("GetCommentByID", err)
return
}

// Verify this is a commit comment
if comment.Type != issues_model.CommentTypeCommitCode || comment.CommitSHA == "" {
ctx.NotFound(errors.New("not a commit code comment"))
return
}

// Verify the comment belongs to this repository
if comment.PosterID != ctx.Doer.ID {
ctx.HTTPError(http.StatusForbidden)
return
}

var attachments []string
if setting.Attachment.Enabled {
attachments = form.Files
}

// Update the comment
if err := repo_service.UpdateCommitCodeComment(ctx, ctx.Doer, comment, form.Content, attachments); err != nil {
ctx.ServerError("UpdateCommitCodeComment", err)
return
}

ctx.JSON(http.StatusOK, map[string]any{
"ok": true,
})
}

// DeleteCommitCodeComment deletes a commit inline comment
func DeleteCommitCodeComment(ctx *context.Context) {
commentID := ctx.PathParamInt64(":id")

comment, err := issues_model.GetCommentByID(ctx, commentID)
if err != nil {
ctx.ServerError("GetCommentByID", err)
return
}

// Verify this is a commit comment
if comment.Type != issues_model.CommentTypeCommitCode || comment.CommitSHA == "" {
ctx.NotFound(errors.New("not a commit code comment"))
return
}

// Delete the comment
if err := repo_service.DeleteCommitCodeComment(ctx, ctx.Doer, comment); err != nil {
ctx.ServerError("DeleteCommitCodeComment", err)
return
}

ctx.JSON(http.StatusOK, map[string]any{
"ok": true,
})
}
10 changes: 10 additions & 0 deletions routers/web/web.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1616,6 +1616,16 @@ func registerWebRoutes(m *web.Router) {
m.Get("/commit/{sha:([a-f0-9]{7,64})$}", repo.SetEditorconfigIfExists, repo.SetDiffViewStyle, repo.SetWhitespaceBehavior, repo.Diff)
m.Get("/commit/{sha:([a-f0-9]{7,64})$}/load-branches-and-tags", repo.LoadBranchesAndTags)

// Commit inline code comments
m.Group("/commit/{sha:([a-f0-9]{7,64})$}/comments", func() {
m.Get("/new", reqSignIn, repo.RenderNewCommitCodeCommentForm)
m.Post("", web.Bind(forms.CodeCommentForm{}), reqSignIn, repo.CreateCommitCodeComment)
m.Group("/{id}", func() {
m.Post("", web.Bind(forms.CodeCommentForm{}), reqSignIn, repo.UpdateCommitCodeComment)
m.Delete("", reqSignIn, repo.DeleteCommitCodeComment)
})
})

// FIXME: this route `/cherry-pick/{sha}` doesn't seem useful or right, the new code always uses `/_cherrypick/` which could handle branch name correctly
m.Get("/cherry-pick/{sha:([a-f0-9]{7,64})$}", repo.SetEditorconfigIfExists, context.RepoRefByDefaultBranch(), repo.CherryPick)
}, repo.MustBeNotEmpty)
Expand Down
Loading