feat: add inline comments on commit diffs

[yuvrajangadsingh]

fixes #4898

adds inline commenting on commit diff views, same as PR review comments but standalone, no issue/PR needed.

what this does

• new commit_comment table with migration (v326), stores repo_id, commit_sha, tree_path, line, poster, content, diff patch
• web handlers for GET (comment form), POST (create comment), POST (delete comment)
• generates diff context patch around the commented line using existing GetFileDiffCutAroundLine
• modified diff section templates (unified + split) to render existing commit comments inline between diff lines
• reuses existing JS for the add-code-comment button clicks and delete-comment flow
• separate templates from PR review comments (no review workflow, no resolve/unresolve, no pending state)

how it works

1. user clicks the + button on a commit diff line (same button as PR diffs)
2. JS fetches the comment form via GET to /{repo}/commit/{sha}/comment
3. user writes comment and submits, JS POSTs to the same URL
4. handler creates the CommitComment record, renders the conversation HTML back
5. JS replaces the form with the rendered conversation (same as PR review flow)
6. on page reload, existing comments are loaded via FindCommitCommentsForDiff and rendered inline

files changed

file	what
models/repo/commit_comment.go	CommitComment model, CRUD, FileCommitComments grouping
models/migrations/v1_26/v326.go	migration to create commit_comment table
models/migrations/migrations.go	register migration
routers/web/repo/commit_comment.go	web handlers (form, create, delete)
routers/web/repo/commit.go	load commit comments in Diff handler
routers/web/web.go	register routes
templates/repo/diff/box.tmpl	conditional comment URL for commit vs PR
templates/repo/diff/section_unified.tmpl	render commit comments inline
templates/repo/diff/section_split.tmpl	render commit comments inline
templates/repo/diff/commit_conversation.tmpl	conversation wrapper (simplified)
templates/repo/diff/commit_comments.tmpl	individual comment rendering
templates/repo/diff/commit_comment_form.tmpl	comment form
templates/repo/diff/new_commit_comment.tmpl	form wrapper for GET endpoint

this is an MVP. follow-up work could include: edit comments, reactions, email notifications, API endpoints, comment counts on commit list pages.

[yuvrajangadsingh]

@wxiaoguang you're right, the PR description was poorly worded. i've read through the full discussion on #4898, your summary comment, and the linked PR #36398 comments.

this implementation follows the minimal scope you outlined: comment only, no emoji reactions, no notifications, no attachments, no resolve/hide, no listing page, no indicator counting. just plain text comments on commit lines using the existing conversation JS.

the model is a standalone commit_comment table (not reusing the existing review/issue comment system) since you mentioned in the #36398 discussion that a unified conversation system would be ideal long term but copying existing code causes maintainability problems.

happy to adjust the approach if this direction is wrong. should i update the PR description to better reflect the scope?

[yuvrajangadsingh]

@lunny reworked. the standalone commit_comment table is now a junction table with just (repo_id, commit_sha, comment_id). actual comment data lives in the existing Comment table with a new CommentTypeCommitComment (39) type.

changes:

• migration v326 now creates a minimal junction table instead of duplicating all comment fields
• models/issues/commit_comment.go has the junction model + query methods
• router creates both a Comment entry and a CommitComment ref in a transaction
• templates use issues_model.Comment directly (DiffSide, UnsignedLine, HashTag all work)

this means commit comments get reactions, attachments, and all existing comment infrastructure for free.

[yuvrajangadsingh]

@lunny addressed both review comments in 37e0396:

• used Cols("comment_id").Table("commit_comment") for the single query approach
• removed models/repo/commit_comment.go entirely

[lunny]

For discussions, a new notification type should be added for commit comments. When someone replies to a comment on your pushed commit, you (as pusher of that commit) should receive a notification via both email and the UI. In addition, users who are mentioned or directly replied to in the comment should also receive notifications.

It would be better to include a screenshot.

[yuvrajangadsingh]

@lunny good point on notifications. that said, i think notifications deserve their own PR since the scope is quite different from the inline commenting itself (needs new notification type, email templates, mention parsing, etc). happy to pick that up as a follow-up once this one lands.

will add screenshots once CI is green and the UI flow is working end to end.

[yuvrajangadsingh]

@lunny screenshots from a local build:

inline comment rendered on commit diff:

"+" button on diff lines for adding new comments:

the existing diff JS handles both PR review comments and commit comments, so clicking the "+" button opens the comment form and submits via AJAX to the /commit/{sha}/comment endpoint. no new JS was needed.

[silverwind]

This comment was written by Claude on behalf of @silverwind

Critical Issues

1. Comment records created with IssueID=0

The new CreateCommitComment creates a Comment with no IssueID. The entire Comment infrastructure assumes comments belong to an issue — LoadIssue(), notifications, activity feeds, API listing endpoints, webhooks. Any code path that encounters these orphaned comments and calls comment.LoadIssue() or filters by IssueID will either error or produce wrong results. This is a fundamental design problem with the junction table approach.

A safer design might store RepoID and CommitSHA directly on the Comment model (adding nullable columns) rather than relying on a separate junction table, or use a completely separate model that doesn't share the comment table.

2. No route-level permission checks beyond reqSignIn

The new routes at web.go only use reqSignIn (and RepoMustNotBeArchived for POST). The existing comment routes all use unit-level middleware like RequireUnitReader(unit.TypeCode) or reqRepoIssuesOrPullsReader. Any signed-in user who can view the repo can create comments — there's no check that they actually have write/comment permission to the repository.

3. No tests

No unit tests for the model functions, no integration tests for the handlers. This is a significant new feature touching multiple layers. At minimum, needs tests for:

• CreateCommitComment / DeleteCommitComment / FindCommitCommentsForDiff
• Handler permission checks
• The junction table integrity

Important Issues

4. Orphan junction records

If a Comment gets deleted through the existing DeleteComment code path (admin action, user deletion, repo deletion cascade), the commit_comment junction record is not cleaned up. This needs either a foreign key constraint with cascade delete, or hooks into existing deletion paths.

5. Delete authorization uses ctx.Repo.IsAdmin()

DeleteCommitComment checks comment.PosterID != ctx.Doer.ID && !ctx.Repo.IsAdmin(). The existing pattern uses ctx.Repo.CanWriteIssuesOrPulls() or proper permission model checks. This is inconsistent and may not cover all appropriate roles (e.g. repo collaborators with write permission). Also returns ctx.JSONError("permission denied") instead of a proper 403 status.

6. Missing composite index

The migration creates individual indexes on repo_id and commit_sha, but the primary query filters on repo_id = ? AND commit_sha = ?. This needs a composite index (repo_id, commit_sha) for efficient lookups.

Minor Issues

7. CanComment check gates on read access — commenting should probably require write access, consistent with how issue comments work.

8. Template dict with "." key in section_split.tmpl is unusual — existing PR review templates use named keys like "root".

9. Repetitive markdown rendering in commit.go — the loop is duplicated for fcc.Left and fcc.Right, should be extracted.

[silverwind]

Need quite a bit of work it seems. If you bring the backend up to shape, I can probably assist on frontend/e2e tests.

[yuvrajangadsingh]

@silverwind thanks for the thorough review, all valid points.

the IssueID=0 thing is the biggest one. i'm going to add CommitSHA and RepoID columns directly on the Comment model instead of the junction table approach. cleaner, no orphan issues, and existing code paths that filter by IssueID won't accidentally pick up commit comments.

for the rest:

• permission checks: will add RequireUnitReader/Writer for code unit, same pattern as PR reviews
• delete auth: switch to proper permission model instead of IsAdmin()
• composite index on (repo_id, commit_sha)
• cascade cleanup via foreign key
• minor stuff (template dict keys, extract the duplicated markdown loop, CanComment gating on write)

will push the rework soon. appreciate the offer to help with frontend/e2e tests, that would be great.

[yuvrajangadsingh]

self-review after addressing @silverwind's feedback:

architecture: dropped the junction table entirely. commit comments now live directly in the Comment table with a new RepoID column (indexed, default 0). this avoids orphaned records and the IssueID=0 problem that junction tables would cause. migration is a single x.Sync(new(Comment)) call.

security: every query in commit_comment.go filters by repo_id + type = CommentTypeCommitComment, so you can't read/delete comments from a different repo. route-level middleware (reqRepoCodeWriter) gates all three endpoints. delete handler additionally checks comment.PosterID == ctx.Doer.ID || ctx.Repo.CanWrite(unit_model.TypeCode).

template changes: switched dict keys from "." to "root" in commit_conversation calls (matching the PR review template pattern). delete button visibility uses $.root.CanWriteCode so repo admins can moderate, not just the comment author.

test coverage: 6 tests covering create, get-by-id (wrong repo returns error), delete (happy path + wrong repo), find-by-sha, and find-for-diff (left/right side organization). all use unittest.PrepareTestDatabase().

patch generation: falls back gracefully. tries GetFileDiffCutAroundLine first, then GeneratePatchForUnchangedLine if the line wasn't in the diff. if both fail, comment still gets created, just without inline context.

one thing i'm not 100% sure about: the LoadIssue guard. added an early return when IssueID == 0 to prevent commit comments from trying to load a non-existent issue. seemed necessary since commit comments don't have an associated issue, but want to confirm this doesn't break any existing codepath that expects LoadIssue to always populate c.Issue.

[silverwind]

Review by Claude on behalf of @silverwind

Permission model is too restrictive

PR review comments only require reqUnitPullsReader (read access to pull requests). This PR uses reqRepoCodeWriter for commit comments, meaning only users with code write access can comment. This is a significant restriction compared to the PR review flow and inconsistent with how other Git forges work. Consider using a reader-level permission instead.

N+1 poster loading

FindCommitCommentsByCommitSHA calls c.LoadPoster(ctx) per comment in a loop. The standard Gitea pattern is batch loading via CommentList.LoadPosters() (see models/issues/comment_list.go). Should use that instead.

Delete silently succeeds for non-existent comments

DeleteCommitComment in the model layer doesn't check affected rows. The handler does call GetCommitCommentByID first (which returns 404 if not found), but there's still a TOCTOU gap. More importantly, the existing DeleteComment in Gitea also doesn't check affected rows, so this is consistent with codebase patterns — but worth noting since this is new code that could do better.

No notifications

Creating a commit comment doesn't trigger any notification or webhook. Users won't know someone commented on their commit unless they revisit the page.

[yuvrajangadsingh]

@silverwind @lunny addressed all the feedback, here's what changed:

permissions (fc9f356) - dropped reqRepoCodeWriter from the commit comment routes. they already inherit reqUnitCodeReader from the parent /{username}/{reponame} group at line 1692, so only reqSignIn is needed on top.

N+1 poster loading (fc9f356) - replaced the per-comment c.LoadPoster(ctx) loop with a single batch call via CommentList(comments).LoadPosters(ctx).

delete affected rows - keeping the current _, err pattern since it's consistent with how DeleteCommitComment and similar delete functions work throughout the codebase. happy to change if you feel strongly about it.

notifications (d37458c) - added CreateCommitCommentNotification() in models/activities/notification.go. when a commit comment is created, it now notifies the commit author (resolved via GetUserByEmail from commit.Author.Email) and any @mentioned users (parsed via references.FindAllMentionsMarkdown). uses the existing NotificationSourceCommit source, same pattern as CreateRepoTransferNotification.

template duplication - checked and this isn't an issue. commit_conversation.tmpl is the single rendering template, section_unified.tmpl and section_split.tmpl both delegate to it without repeating the markdown rendering.

[yuvrajangadsingh]

@lunny done, switched to GetUserIDsByNames with ignoreNonExistent=true for the mention lookups (88f6312).

[lunny]

 I still believe we should use a separate table for commit comments due to performance considerations.

When loading commit comments, the query will use repo_id, commit_sha, and type as a composite index. We should avoid adding an index on just repo_id and commit_sha, because most comments are associated with issues rather than commits. Adding that index to the main comments table would significantly increase index size.

Adding an extra repo_id column would also be unnecessary for most comments, since the majority of comments are not commit-related. This is especially true for instances that do not use the commit comment feature at all. In addition, the migration could take a long time on large Gitea instances.

@silverwind @yuvrajangadsingh

[yuvrajangadsingh]

@lunny @silverwind makes sense. the index bloat and migration cost on large instances is a fair concern.

thinking of going back to a separate table, but not the junction approach from before. a standalone commit_comment table with its own columns (repo_id, commit_sha, tree_path, line, poster_id, content, etc). no foreign key to the Comment table at all.

this way:

• no new columns or indexes on the comment table
• no IssueID=0 orphan problem that silverwind flagged
• no migration touching existing rows
• reactions, attachments, etc can be added later in follow-up PRs directly on this table if needed

the tradeoff is we don't get existing comment infrastructure (reactions, attachments) for free, but those weren't in scope for the MVP anyway.

does this direction work for both of you?

[lunny]

@lunny @silverwind makes sense. the index bloat and migration cost on large instances is a fair concern.

thinking of going back to a separate table, but not the junction approach from before. a standalone commit_comment table with its own columns (repo_id, commit_sha, tree_path, line, poster_id, content, etc). no foreign key to the Comment table at all.

this way:

• no new columns or indexes on the comment table
• no IssueID=0 orphan problem that silverwind flagged
• no migration touching existing rows
• reactions, attachments, etc can be added later in follow-up PRs directly on this table if needed

the tradeoff is we don't get existing comment infrastructure (reactions, attachments) for free, but those weren't in scope for the MVP anyway.

does this direction work for both of you?

If this approach can solve another problem as well, I can accept it. However, my main concern is how we would implement reactions, attachments, and pasted images if we follow this direction. Some people may think these features are unnecessary, but I don’t agree. Once the basic requirements are met, users will inevitably ask for more advanced functionality. That’s why I don’t think it’s a good idea to introduce separate tables for reactions, attachments, and other related features.

We need a complete design before starting this work; otherwise, it will make the later stages much more difficult—or even impossible—to implement.

[yuvrajangadsingh]

@lunny fair point, painting ourselves into a corner on reactions/attachments would be bad.

thinking about it more, the junction table approach actually works for both concerns:

• commit_comment table with (repo_id, commit_sha, tree_path, line, comment_id), no new columns on the comment table
• comment data stays in the Comment table with CommentTypeCommitComment type
• reactions, attachments, pasted images all work out of the box since they key off comment_id
• the IssueID=0 issue @silverwind raised gets handled with a type guard in LoadIssue(), early return when comment.Type == CommentTypeCommitComment

no index bloat on the comment table, no migration touching existing rows, and future features like reactions just work without any extra tables.

this is actually where the PR started before silverwind's review pushed it to the standalone column approach. the IssueID=0 concern is valid but fixable with targeted guards rather than a whole redesign.

does this work?