From fa41fc94d4ef44caa116fc2f527225bd5d688cd0 Mon Sep 17 00:00:00 2001 From: Tim Date: Fri, 30 Oct 2026 21:18:13 +0100 Subject: [PATCH] feat(pulls): support per-commit review (#39326) Opening a single commit of a PR let you look, but not review. The Review button was disabled and there was no way to reach the next or previous commit. This finishes https://github.com/go-gitea/gitea/pull/37475, which only reached the head commit. - Reviews can be submitted from a single commit, a commit range stays disabled - A bar above a single-commit diff shows the commit, its position in the PR and prev/next buttons - The commit dropdown marks the commits on screen - Timeline commits open the PR's commit view, commits force-pushed out of the PR redirect to the repository commit page Assisted-by: Claude Code (Fable 5.1) Co-authored-by: bircni Co-authored-by: wxiaoguang Co-authored-by: silverwind --- options/locale/locale_en-US.json | 8 ++-- routers/web/repo/pull.go | 45 ++++++++++++++++-- routers/web/repo/pull_review.go | 2 + templates/repo/commit_statuses.tmpl | 2 +- templates/repo/commits_list_small.tmpl | 2 +- templates/repo/diff/box.tmpl | 7 ++- templates/repo/diff/comment_form.tmpl | 2 +- templates/repo/diff/new_review.tmpl | 10 ++-- templates/repo/diff/single_commit_bar.tmpl | 33 ++++++++++++++ tests/integration/pull_diff_test.go | 48 +++++++++++++++----- web_src/css/modules/button.css | 7 ++- web_src/js/components/DiffCommitSelector.vue | 24 ++++++++-- 12 files changed, 155 insertions(+), 35 deletions(-) create mode 100644 templates/repo/diff/single_commit_bar.tmpl diff --git a/options/locale/locale_en-US.json b/options/locale/locale_en-US.json index 0abaf633514..bee657b8213 100644 --- a/options/locale/locale_en-US.json +++ b/options/locale/locale_en-US.json @@ -1791,10 +1791,12 @@ "repo.pulls.filter_branch": "Filter branch", "repo.pulls.show_all_commits": "Show all commits", "repo.pulls.show_changes_since_your_last_review": "Show changes since your last review", - "repo.pulls.showing_only_single_commit": "Showing only changes of commit %[1]s", "repo.pulls.showing_specified_commit_range": "Showing only changes between %[1]s..%[2]s", "repo.pulls.select_commit_hold_shift_for_range": "Select commit. Hold Shift and click to select a range.", - "repo.pulls.review_only_possible_for_full_diff": "Review is only possible when viewing the full diff", + "repo.pulls.review_not_possible_for_commit_range": "Review is not possible when viewing a commit range", + "repo.pulls.commit_position": "Commit %[1]d of %[2]d", + "repo.pulls.previous_commit": "Previous commit", + "repo.pulls.next_commit": "Next commit", "repo.pulls.filter_changes_by_commit": "Filter by commit", "repo.pulls.nothing_to_compare": "There are no differences to show. There is no need to create a pull request.", "repo.pulls.no_common_history": "These branches do not share a common merge base. Select a different base or compare branch.", @@ -2626,7 +2628,7 @@ "repo.diff.comment.start_review": "Start review", "repo.diff.comment.reply": "Reply", "repo.diff.review": "Review", - "repo.diff.review.header": "Submit review", + "repo.diff.review.header": "Finish your review", "repo.diff.review.placeholder": "Review comment", "repo.diff.review.comment": "Comment", "repo.diff.review.approve": "Approve", diff --git a/routers/web/repo/pull.go b/routers/web/repo/pull.go index f8cdd4613c4..f6ae0148da7 100644 --- a/routers/web/repo/pull.go +++ b/routers/web/repo/pull.go @@ -6,11 +6,13 @@ package repo import ( + "bytes" "errors" "fmt" "html" "html/template" "net/http" + "slices" "strconv" "strings" "time" @@ -676,6 +678,26 @@ func indexCommit(commits []*git.Commit, commitID string) *git.Commit { return nil } +// setSingleCommitNavContext exposes the reviewed commit and its neighbors. +func setSingleCommitNavContext(ctx *context.Context, commits []*git.Commit, commit *git.Commit) { + idx := slices.IndexFunc(commits, func(c *git.Commit) bool { return bytes.Equal(c.ID.RawValue(), commit.ID.RawValue()) }) + if idx < 0 { + return + } + + ctx.Data["SingleCommit"] = commit + ctx.Data["SingleCommitIndex"] = len(commits) - idx + ctx.Data["SingleCommitCount"] = len(commits) + + // The commit list is newest-first while a reviewer walks it oldest-first, so the indexes are mirrored. + if idx+1 < len(commits) { + ctx.Data["SingleCommitPrevID"] = commits[idx+1].ID.String() + } + if idx > 0 { + ctx.Data["SingleCommitNextID"] = commits[idx-1].ID.String() + } +} + // ViewPullFiles render pull request changed files list page func viewPullFiles(ctx *context.Context, beforeCommitID, afterCommitID string) { var err error @@ -703,11 +725,17 @@ func viewPullFiles(ctx *context.Context, beforeCommitID, afterCommitID string) { } headCommitID := prCompareInfo.HeadCommitID - isSingleCommit := beforeCommitID == "" && afterCommitID != "" - // FIXME: when afterCommitID==headCommitID, isSingleCommit and isShowAllCommits can be both true, which doesn't seem right - isShowAllCommits := (beforeCommitID == "" || beforeCommitID == prCompareInfo.CompareBase) && (afterCommitID == "" || afterCommitID == headCommitID) + // show all commits when: + // * both before and after commit IDs are empty (default view) + // * before commit ID is the PR's base commit and after commit ID is the PR's head commit (default view) + // * there is only one commit in the PR and after commit ID is the PR's head commit (single-commit view) + isShowAllCommits := (beforeCommitID == "" && afterCommitID == "") || + (beforeCommitID == prCompareInfo.CompareBase && afterCommitID == headCommitID) || + (beforeCommitID == "" && afterCommitID == headCommitID && len(prCompareInfo.Commits) == 1) + isSingleCommit := beforeCommitID == "" && afterCommitID != "" && !isShowAllCommits - ctx.Data["IsShowingOnlySingleCommit"] = isSingleCommit + // it's questionable why single commit review can be submitted, but the range commits review cannot be submitted + ctx.Data["CanSubmitReview"] = isShowAllCommits || isSingleCommit ctx.Data["IsShowingAllCommits"] = isShowAllCommits // "commits list" is half-open, half-closed: (base, head] @@ -723,6 +751,10 @@ func viewPullFiles(ctx *context.Context, beforeCommitID, afterCommitID string) { return } } + if afterCommit == nil && isSingleCommit { + ctx.Redirect(ctx.Repo.RepoLink + "/commit/" + afterCommitID) // timeline commits outlive force pushes + return + } if afterCommit == nil { ctx.NotFound(nil) return @@ -756,6 +788,11 @@ func viewPullFiles(ctx *context.Context, beforeCommitID, afterCommitID string) { ctx.Data["CompareInfo"] = prCompareInfo ctx.Data["AfterCommitID"] = afterCommitID ctx.Data["BeforeCommitID"] = beforeCommitID + ctx.Data["ReviewCommitID"] = afterCommitID // the review happens on the "after commit" (the latest commit in the range) + + if isSingleCommit { + setSingleCommitNavContext(ctx, prCompareInfo.Commits, afterCommit) + } maxLines, maxFiles := setting.Git.MaxGitDiffLines, setting.Git.MaxGitDiffFiles files := ctx.FormStrings("files") diff --git a/routers/web/repo/pull_review.go b/routers/web/repo/pull_review.go index 8daa7f91b64..2fd841eb18a 100644 --- a/routers/web/repo/pull_review.go +++ b/routers/web/repo/pull_review.go @@ -56,6 +56,7 @@ func RenderNewCodeCommentForm(ctx *context.Context) { return } ctx.Data["AfterCommitID"] = pullHeadCommitID + ctx.Data["ReviewCommitID"] = pullHeadCommitID // it's questionable why it only needs the head commit ID but not correct "AfterCommitID" (single commit review) upload.AddUploadContext(ctx, "comment") ctx.HTML(http.StatusOK, tplNewComment) } @@ -209,6 +210,7 @@ func renderConversation(ctx *context.Context, comment *issues_model.Comment, ori return } ctx.Data["AfterCommitID"] = pullHeadCommitID + ctx.Data["ReviewCommitID"] = pullHeadCommitID // it's questionable why it only needs the head commit ID but not correct "AfterCommitID" (single commit review) ctx.Data["CanBlockUser"] = func(blocker, blockee *user_model.User) bool { return user_service.CanBlockUser(ctx, ctx.Doer, blocker, blockee) } diff --git a/templates/repo/commit_statuses.tmpl b/templates/repo/commit_statuses.tmpl index 80b45092533..80071dc1f6d 100644 --- a/templates/repo/commit_statuses.tmpl +++ b/templates/repo/commit_statuses.tmpl @@ -1,6 +1,6 @@ {{if .Statuses}} {{if and (eq (len .Statuses) 1) .Status.TargetURL}} - + {{template "repo/icons/commit_status" .Status}} {{else}} diff --git a/templates/repo/commits_list_small.tmpl b/templates/repo/commits_list_small.tmpl index 1312d0690fa..bf65547fcfc 100644 --- a/templates/repo/commits_list_small.tmpl +++ b/templates/repo/commits_list_small.tmpl @@ -8,7 +8,7 @@ {{svg "octicon-git-commit"}} {{ctx.RenderUtils.AvatarStack $commit.AvatarStackData}} - {{$commitBaseLink := printf "%s/commit" $.comment.Issue.PullRequest.BaseRepo.Link}} + {{$commitBaseLink := printf "%s/commits" $.comment.Issue.Link}}{{/* "/owner/repo/pulls/{idx}/commits" */}} {{$commitLink:= printf "%s/%s" $commitBaseLink (PathEscape $gitCommit.ID.String)}} diff --git a/templates/repo/diff/box.tmpl b/templates/repo/diff/box.tmpl index 7993ec021a7..9879754bef6 100644 --- a/templates/repo/diff/box.tmpl +++ b/templates/repo/diff/box.tmpl @@ -36,6 +36,7 @@ {{template "repo/diff/options_dropdown" .}} {{if .PageIsPullFiles}}
{{/* the following will be replaced by vue component, but this avoids any loading artifacts till the vue component is initialized */}} @@ -50,10 +51,8 @@
{{if not .DiffNotAvailable}} - {{if and .IsShowingOnlySingleCommit .PageIsPullFiles}} -
-
{{ctx.Locale.Tr "repo.pulls.showing_only_single_commit" (ShortSha .AfterCommitID)}} - {{ctx.Locale.Tr "repo.pulls.show_all_commits"}}
-
+ {{if and .PageIsPullFiles .SingleCommit}} + {{template "repo/diff/single_commit_bar" .}} {{else if and (not .IsShowingAllCommits) .PageIsPullFiles}}
{{ctx.Locale.Tr "repo.pulls.showing_specified_commit_range" (ShortSha .BeforeCommitID) (ShortSha .AfterCommitID)}} - {{ctx.Locale.Tr "repo.pulls.show_all_commits"}}
diff --git a/templates/repo/diff/comment_form.tmpl b/templates/repo/diff/comment_form.tmpl index 23efda9a219..19f29db1359 100644 --- a/templates/repo/diff/comment_form.tmpl +++ b/templates/repo/diff/comment_form.tmpl @@ -4,7 +4,7 @@ {{if $canAddComment}}
- + diff --git a/templates/repo/diff/new_review.tmpl b/templates/repo/diff/new_review.tmpl index 513d75cd872..cdb6ba35f1a 100644 --- a/templates/repo/diff/new_review.tmpl +++ b/templates/repo/diff/new_review.tmpl @@ -1,8 +1,8 @@ {{$disabledReviewTooltip := "" -}} {{if $.Repository.IsArchived -}} {{$disabledReviewTooltip = ctx.Locale.Tr "repo.archive.pull.nocomment" -}} -{{else if not $.IsShowingAllCommits -}} - {{$disabledReviewTooltip = ctx.Locale.Tr "repo.pulls.review_only_possible_for_full_diff" -}} +{{else if not $.CanSubmitReview -}} + {{$disabledReviewTooltip = ctx.Locale.Tr "repo.pulls.review_not_possible_for_commit_range" -}} {{end -}}
@@ -12,13 +12,13 @@ {{svg "octicon-triangle-down" 14 "dropdown icon"}}
-{{if $.IsShowingAllCommits}} +{{if $.CanSubmitReview}}
- +
-
{{ctx.Locale.Tr "repo.diff.review.header"}}
+ {{ctx.Locale.Tr "repo.diff.review.header"}} {{svg "octicon-x" 16}}
diff --git a/templates/repo/diff/single_commit_bar.tmpl b/templates/repo/diff/single_commit_bar.tmpl new file mode 100644 index 00000000000..2c4593c54a3 --- /dev/null +++ b/templates/repo/diff/single_commit_bar.tmpl @@ -0,0 +1,33 @@ +{{$queryParams := QueryBuild "?" "style" (Iif $.IsSplitStyle "split" "unified") "whitespace" $.WhitespaceBehavior "show-outdated" (Iif $.ShowOutdatedComments "true" "false")}} +{{$commit := $.SingleCommit}}{{$commitID := $commit.ID.String}} +{{$prevID := $.SingleCommitPrevID}}{{$nextID := $.SingleCommitNextID}} +{{$repoLink := $.RepoLink}} +{{$prLink := $.Issue.Link}} +
+
+
+ {{svg "octicon-git-commit"}} + {{ShortSha $commitID}} + {{$commit.MessageTitle | ctx.RenderUtils.RenderEmoji}} +
+
+
+ {{ctx.AvatarUtils.AvatarByEmail $commit.Author.Email $commit.Author.Name 16}} + {{$commit.Author.Name}} +
+ {{DateUtils.TimeSince $commit.Author.When}} +
+
+
+ {{ctx.Locale.Tr "repo.pulls.commit_position" $.SingleCommitIndex $.SingleCommitCount}} +
+ {{svg "octicon-chevron-left"}} + {{svg "octicon-chevron-right"}} +
+ {{ctx.Locale.Tr "repo.pulls.show_all_commits"}} +
+
diff --git a/tests/integration/pull_diff_test.go b/tests/integration/pull_diff_test.go index 60d06e6e043..2dc6207cbbd 100644 --- a/tests/integration/pull_diff_test.go +++ b/tests/integration/pull_diff_test.go @@ -25,27 +25,51 @@ func TestPullDiff(t *testing.T) { t.Run("CompletePRDiff", func(t *testing.T) { testPullDiffAssertPage(t, "/user2/commitsonpr/pulls/1/files", false, []string{"test1.txt", "test10.txt", "test2.txt", "test3.txt", "test4.txt", "test5.txt", "test6.txt", "test7.txt", "test8.txt", "test9.txt"}) }) - t.Run("SingleCommitPRDiff", func(t *testing.T) { - testPullDiffAssertPage(t, "/user2/commitsonpr/pulls/1/commits/c5626fc9eff57eb1bb7b796b01d4d0f2f3f792a2", true, []string{"test3.txt"}) - }) t.Run("CommitRangePRDiff", func(t *testing.T) { testPullDiffAssertPage(t, "/user2/commitsonpr/pulls/1/files/4ca8bcaf27e28504df7bf996819665986b01c847..23576dd018294e476c06e569b6b0f170d0558705", true, []string{"test2.txt", "test3.txt", "test4.txt"}) }) - t.Run("SingleHeadCommitReviewFormAction", testPullDiffSingleHeadCommitReviewFormAction) + t.Run("SingleCommit", testPullDiffSingleCommit) } -func testPullDiffSingleHeadCommitReviewFormAction(t *testing.T) { +func testPullDiffSingleCommit(t *testing.T) { session := loginUser(t, "user2") - req := NewRequest(t, "GET", "/user2/commitsonpr/pulls/1/commits/1978192d98bb1b65e11c2cf37da854fbf94bffd6") + req := NewRequest(t, "GET", "/user2/commitsonpr/pulls/1/commits/c5626fc9eff57eb1bb7b796b01d4d0f2f3f792a2") resp := session.MakeRequest(t, req, http.StatusOK) doc := NewHTMLParser(t, resp.Body) - btn := doc.Find(".review-box-button") - require.Equal(t, 1, btn.Length()) - assert.False(t, btn.Is("[disabled]")) - form := doc.Find(".review-box-panel form") - assert.Equal(t, 1, form.Length()) - assert.Equal(t, "/user2/commitsonpr/pulls/1/files/reviews/submit", form.AttrOr("action", "")) + bar := doc.Find("[data-commit-single-index]") + require.Equal(t, 1, bar.Length()) + barText := bar.Text() + assert.Contains(t, barText, "test 3") + assert.Contains(t, barText, "Sebastian Sauer") + assert.Equal(t, "3", bar.AttrOr("data-commit-single-index", "")) + assert.Equal(t, "10", bar.AttrOr("data-commit-total-count", "")) + + assert.Contains(t, bar.Find(".js-previous-commit-link").AttrOr("href", ""), "/pulls/1/commits/96cef4a7b72b3c208340ae6f0cf55a93e9077c93") + assert.Contains(t, bar.Find(".js-next-commit-link").AttrOr("href", ""), "/pulls/1/commits/23576dd018294e476c06e569b6b0f170d0558705") + + commitSelect := doc.Find("#diff-commit-select") + assert.Equal(t, "96cef4a7b72b3c208340ae6f0cf55a93e9077c93", commitSelect.AttrOr("data-before-commit", "")) + assert.Equal(t, "c5626fc9eff57eb1bb7b796b01d4d0f2f3f792a2", commitSelect.AttrOr("data-after-commit", "")) + + panel := doc.Find(".review-box-panel form") + require.Equal(t, 1, panel.Length()) + assert.Equal(t, "c5626fc9eff57eb1bb7b796b01d4d0f2f3f792a2", panel.Find(`input[name="commit_id"]`).AttrOr("value", "")) + + req = NewRequest(t, "GET", "/user2/commitsonpr/pulls/1/files/reviews/new_comment") + resp = session.MakeRequest(t, req, http.StatusOK) + assert.Equal(t, "1978192d98bb1b65e11c2cf37da854fbf94bffd6", NewHTMLParser(t, resp.Body).Find(`input[name="latest_commit_id"]`).AttrOr("value", "")) + + req = NewRequest(t, "GET", "/user2/commitsonpr/pulls/1/commits/4ca8bcaf27e28504df7bf996819665986b01c847") + resp = session.MakeRequest(t, req, http.StatusOK) + bar = NewHTMLParser(t, resp.Body).Find("[data-commit-single-index]") + assert.Equal(t, "1", bar.AttrOr("data-commit-single-index", "")) + assert.True(t, bar.Find(".js-previous-commit-link").HasClass("disabled")) + assert.False(t, bar.Find(".js-next-commit-link").HasClass("disabled")) + + req = NewRequest(t, "GET", "/user2/commitsonpr/pulls/1/commits/cbff181af4c9c7fee3cf6c106699e07d9a3f54e6") + resp = session.MakeRequest(t, req, http.StatusSeeOther) + assert.Equal(t, "/user2/commitsonpr/commit/cbff181af4c9c7fee3cf6c106699e07d9a3f54e6", test.RedirectURL(resp)) } func testPullDiffAssertPage(t *testing.T, prDiffURL string, reviewBtnDisabled bool, expectedFilenames []string) { diff --git a/web_src/css/modules/button.css b/web_src/css/modules/button.css index 811dc14f503..b9427a00808 100644 --- a/web_src/css/modules/button.css +++ b/web_src/css/modules/button.css @@ -32,7 +32,11 @@ color: var(--color-text); } -.ui.button.hover-opaque:hover { +.ui.ui.ui.buttons.hover-opaque { + background: var(--color-body) +} + +.ui.ui.ui.button.hover-opaque:hover { /* ".ui.button:hover" uses alpha channel to "highlight", it doesn't work when the background is not body: the inherited color is not right for such case, use this opaque color */ background: var(--color-hover-opaque); @@ -282,6 +286,7 @@ font-size: 0; vertical-align: baseline; margin: 0 0.25em 0 0; + border-radius: var(--border-radius); } /* btn is a plain button without any opinionated styling, it only uses flex for vertical alignment like ".ui.button" in base.css */ diff --git a/web_src/js/components/DiffCommitSelector.vue b/web_src/js/components/DiffCommitSelector.vue index 0471ad15f93..dcc26a58ec7 100644 --- a/web_src/js/components/DiffCommitSelector.vue +++ b/web_src/js/components/DiffCommitSelector.vue @@ -23,11 +23,14 @@ type CommitListResult = { const elRoot = useTemplateRef('elRoot') as Readonly>; const elExpandBtn = useTemplateRef('elExpandBtn') as Readonly>; const elShowAllChanges = useTemplateRef('elShowAllChanges') as Readonly>; +const elMenu = useTemplateRef('elMenu') as Readonly>; const elMount = document.querySelector('#diff-commit-select')!; const queryParams = elMount.getAttribute('data-queryparams'); const issueLink = elMount.getAttribute('data-issuelink'); const mergeBase = elMount.getAttribute('data-merge-base'); +const beforeCommit = elMount.getAttribute('data-before-commit'); +const afterCommit = elMount.getAttribute('data-after-commit'); const uniqueIdMenu = generateElemId('diff-commit-selector-menu-'); const uniqueIdShowAll = generateElemId('diff-commit-selector-show-all-'); @@ -37,6 +40,11 @@ const locale = shallowRef>({filter_changes_by_commit: elM const commits = ref>([]); // deep, the commit objects are mutated in place const hoverActivated = shallowRef(false); const lastReviewCommitSha = shallowRef(null); +const activeRange = shallowRef<[number, number] | null>(null); + +function isActive(idx: number) { + return activeRange.value !== null && activeRange.value[0] <= idx && idx <= activeRange.value[1]; +} const commitsSinceLastReview = computed(() => { if (lastReviewCommitSha.value) { @@ -140,6 +148,8 @@ async function toggleMenu() { nextTick(() => { if (menuVisible.value) { focusElem(elShowAllChanges.value, elExpandBtn.value); + const elActive = elMenu.value.querySelector('.item.active'); + if (elActive) elMenu.value.scrollTop = elActive.offsetTop - elMenu.value.clientHeight / 2; // scrollIntoView would also scroll the page } else { focusElem(elExpandBtn.value, elShowAllChanges.value); } @@ -160,6 +170,8 @@ async function fetchCommits() { lastReviewCommitSha.value = null; } locale.value = {...locale.value, ...results.locale}; + const end = commits.value.findIndex((x) => x.id === afterCommit); + if (end >= 0) activeRange.value = [commits.value.findIndex((x) => x.id === beforeCommit) + 1, end]; // starts at 0 when beforeCommit is outside the list } function showAllChanges() { @@ -228,7 +240,7 @@ function commitClickedShift(commit: Commit) { -
+