Implement single-commit PR review flow (#7155)

This implements the UI controls and information displays necessary to allow reviewing pull requests by stepping through commits individually.

Notable changes:

- Within the PR page, commit links now stay in the PR context by navigating to `{owner}/{repo}/pulls/{id}/commits/{sha}`
- When showing a single commit in the "Files changed" tab, the commit header containing commit message and metadata is displayed
  - I dropped the existing buttons, since they make less sense to me in the PR context
  - The SHA links to the separate, dedicated commit view
- "Previous"/"Next" buttons have been added to that header to allow stepping through commits
- Reviews can be submitted in "single commit" view

Talking points:

- The "Showing only changes from" banner made sense when that view was limited (e.g. review submit was disabled). Now that it's on par with the "all commits" view, and visually distinct due to the commit header, this banner could potentially be dropped.

Closes: #5670 #5126 #5671 #2281 #8084

![image](/attachments/cff441dc-a080-42f8-86ae-9b80490761bf)

## Checklist

The [contributor guide](https://forgejo.org/docs/next/contributor/) contains information that will be helpful to first time contributors. There also are a few [conditions for merging Pull Requests in Forgejo repositories](https://codeberg.org/forgejo/governance/src/branch/main/PullRequestsAgreement.md). You are also welcome to join the [Forgejo development chatroom](https://matrix.to/#/#forgejo-development:matrix.org).

### Tests

- I added test coverage for Go changes...
  - [ ] in their respective `*_test.go` for unit tests.
  - [ ] in the `tests/integration` directory if it involves interactions with a live Forgejo server.
- I added test coverage for JavaScript changes...
  - [ ] in `web_src/js/*.test.js` if it can be unit tested.
  - [x] in `tests/e2e/*.test.e2e.js` if it requires interactions with a live Forgejo server (see also the [developer guide for JavaScript testing](https://codeberg.org/forgejo/forgejo/src/branch/forgejo/tests/e2e/README.md#end-to-end-tests)).

### Documentation

- [ ] I created a pull request [to the documentation](https://codeberg.org/forgejo/docs) to explain to Forgejo users how to use this change.
- [x] I did not document these changes and I do not expect someone else to do it.

### Release notes

- [ ] I do not want this change to show in the release notes.
- [x] I want the title to show in the release notes with a link to this pull request.
- [ ] I want the content of the `release-notes/<pull request number>.md` to be be used for the release notes instead of the title.

Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/7155
Reviewed-by: Earl Warren <earl-warren@noreply.codeberg.org>
Reviewed-by: Beowulf <beowulf@beocode.eu>
Co-authored-by: Lucas Schwiderski <lucas@lschwiderski.de>
Co-committed-by: Lucas Schwiderski <lucas@lschwiderski.de>

models/fixtures/comment.yml

@@ -113,3 +113,43 @@
   review_id: 22
   assignee_id: 5
   created_unix: 946684817
+
+-
+  id: 13
+  type: 29 # push
+  poster_id: 2
+  issue_id: 19 # in repo_id 58
+  content: '{"is_force_push":false,"commit_ids":["4ca8bcaf27e28504df7bf996819665986b01c847","96cef4a7b72b3c208340ae6f0cf55a93e9077c93","c5626fc9eff57eb1bb7b796b01d4d0f2f3f792a2"]}'
+  created_unix: 1688672373
+
+-
+  id: 14
+  type: 29 # push
+  poster_id: 2
+  issue_id: 19 # in repo_id 58
+  content: '{"is_force_push":false,"commit_ids":["23576dd018294e476c06e569b6b0f170d0558705"]}'
+  created_unix: 1688672374
+
+-
+  id: 15
+  type: 29 # push
+  poster_id: 2
+  issue_id: 19 # in repo_id 58
+  content: '{"is_force_push":false,"commit_ids":["3e64625bd6eb5bcba69ac97de6c8f507402df861", "c704db5794097441aa2d9dd834d5b7e2f8f08108"]}'
+  created_unix: 1688672375
+
+-
+  id: 16
+  type: 29 # push
+  poster_id: 2
+  issue_id: 19 # in repo_id 58
+  content: '{"is_force_push":false,"commit_ids":["811d46c7e518f4f180afb862c0db5cb8c80529ce", "747ddb3506a4fa04a7747808eb56ae16f9e933dc", "837d5c8125633d7d258f93b998e867eab0145520", "1978192d98bb1b65e11c2cf37da854fbf94bffd6"]}'
+  created_unix: 1688672376
+
+-
+  id: 17
+  type: 29 # push
+  poster_id: 2
+  issue_id: 19 # in repo_id 58
+  content: '{"is_force_push":true,"commit_ids":["1978192d98bb1b65e11c2cf37da854fbf94bffd6", "9b93963cf6de4dc33f915bb67f192d099c301f43"]}'
+  created_unix: 1749734240

options/locale_next/locale_en-US.json

@@ -89,6 +89,8 @@
     "mail.actions.run_info_previous_status": "Previous Run's Status: %[1]s",
     "mail.actions.run_info_ref": "Branch: %[1]s (%[2]s)",
     "mail.actions.run_info_trigger": "Triggered because: %[1]s by: %[2]s",
+    "repo.diff.commit.next-short": "Next",
+    "repo.diff.commit.previous-short": "Prev",
     "discussion.locked": "This discussion has been locked. Commenting is limited to contributors.",
     "editor.textarea.tab_hint": "Line already indented. Press <kbd>Tab</kbd> again or <kbd>Escape</kbd> to leave the editor.",
     "editor.textarea.shift_tab_hint": "No indentation on this line. Press <kbd>Shift</kbd> + <kbd>Tab</kbd> again or <kbd>Escape</kbd> to leave the editor.",

routers/web/repo/pull.go

@@ -10,13 +10,16 @@ import (
 	"errors"
 	"fmt"
 	"html"
+	"html/template"
 	"net/http"
 	"net/url"
+	"path"
 	"strconv"
 	"strings"
 
 	"forgejo.org/models"
 	activities_model "forgejo.org/models/activities"
+	asymkey_model "forgejo.org/models/asymkey"
 	"forgejo.org/models/db"
 	git_model "forgejo.org/models/git"
 	issues_model "forgejo.org/models/issues"
@@ -28,11 +31,13 @@ import (
 	"forgejo.org/models/unit"
 	user_model "forgejo.org/models/user"
 	"forgejo.org/modules/base"
+	"forgejo.org/modules/charset"
 	"forgejo.org/modules/emoji"
 	"forgejo.org/modules/git"
 	"forgejo.org/modules/gitrepo"
 	issue_template "forgejo.org/modules/issue/template"
 	"forgejo.org/modules/log"
+	"forgejo.org/modules/markup"
 	"forgejo.org/modules/optional"
 	"forgejo.org/modules/setting"
 	"forgejo.org/modules/structs"
@@ -498,6 +503,7 @@ func PrepareMergedViewPullInfo(ctx *context.Context, issue *issues_model.Issue)
 			ctx.Data["IsPullRequestBroken"] = true
 			ctx.Data["BaseTarget"] = pull.BaseBranch
 			ctx.Data["NumCommits"] = 0
+			ctx.Data["CommitIDs"] = map[string]bool{}
 			ctx.Data["NumFiles"] = 0
 			return nil
 		}
@@ -508,6 +514,12 @@ func PrepareMergedViewPullInfo(ctx *context.Context, issue *issues_model.Issue)
 	ctx.Data["NumCommits"] = len(compareInfo.Commits)
 	ctx.Data["NumFiles"] = compareInfo.NumFiles
 
+	commitIDs := map[string]bool{}
+	for _, commit := range compareInfo.Commits {
+		commitIDs[commit.ID.String()] = true
+	}
+	ctx.Data["CommitIDs"] = commitIDs
+
 	if len(compareInfo.Commits) != 0 {
 		sha := compareInfo.Commits[0].ID.String()
 		commitStatuses, _, err := git_model.GetLatestCommitStatus(ctx, ctx.Repo.Repository.ID, sha, db.ListOptionsAll)
@@ -591,6 +603,7 @@ func PrepareViewPullInfo(ctx *context.Context, issue *issues_model.Issue) *git.C
 				ctx.Data["IsPullRequestBroken"] = true
 				ctx.Data["BaseTarget"] = pull.BaseBranch
 				ctx.Data["NumCommits"] = 0
+				ctx.Data["CommitIDs"] = map[string]bool{}
 				ctx.Data["NumFiles"] = 0
 				return nil
 			}
@@ -601,6 +614,13 @@ func PrepareViewPullInfo(ctx *context.Context, issue *issues_model.Issue) *git.C
 
 		ctx.Data["NumCommits"] = len(compareInfo.Commits)
 		ctx.Data["NumFiles"] = compareInfo.NumFiles
+
+		commitIDs := map[string]bool{}
+		for _, commit := range compareInfo.Commits {
+			commitIDs[commit.ID.String()] = true
+		}
+		ctx.Data["CommitIDs"] = commitIDs
+
 		return compareInfo
 	}
 
@@ -659,6 +679,7 @@ func PrepareViewPullInfo(ctx *context.Context, issue *issues_model.Issue) *git.C
 			}
 			ctx.Data["BaseTarget"] = pull.BaseBranch
 			ctx.Data["NumCommits"] = 0
+			ctx.Data["CommitIDs"] = map[string]bool{}
 			ctx.Data["NumFiles"] = 0
 			return nil
 		}
@@ -736,6 +757,7 @@ func PrepareViewPullInfo(ctx *context.Context, issue *issues_model.Issue) *git.C
 			ctx.Data["IsPullRequestBroken"] = true
 			ctx.Data["BaseTarget"] = pull.BaseBranch
 			ctx.Data["NumCommits"] = 0
+			ctx.Data["CommitIDs"] = map[string]bool{}
 			ctx.Data["NumFiles"] = 0
 			return nil
 		}
@@ -760,6 +782,13 @@ func PrepareViewPullInfo(ctx *context.Context, issue *issues_model.Issue) *git.C
 
 	ctx.Data["NumCommits"] = len(compareInfo.Commits)
 	ctx.Data["NumFiles"] = compareInfo.NumFiles
+
+	commitIDs := map[string]bool{}
+	for _, commit := range compareInfo.Commits {
+		commitIDs[commit.ID.String()] = true
+	}
+	ctx.Data["CommitIDs"] = commitIDs
+
 	return compareInfo
 }
 
@@ -919,7 +948,81 @@ func viewPullFiles(ctx *context.Context, specifiedStartCommit, specifiedEndCommi
 
 	ctx.Data["IsShowingOnlySingleCommit"] = willShowSpecifiedCommit
 
-	if willShowSpecifiedCommit || willShowSpecifiedCommitRange {
+	if willShowSpecifiedCommit {
+		commitID := specifiedEndCommit
+
+		ctx.Data["CommitID"] = commitID
+
+		var prevCommit, curCommit, nextCommit *git.Commit
+
+		// Iterate in reverse to properly map "previous" and "next" buttons
+		for i := len(prInfo.Commits) - 1; i >= 0; i-- {
+			commit := prInfo.Commits[i]
+
+			if curCommit != nil {
+				nextCommit = commit
+				break
+			}
+
+			if commit.ID.String() == commitID {
+				curCommit = commit
+			} else {
+				prevCommit = commit
+			}
+		}
+
+		if curCommit == nil {
+			ctx.ServerError("Repo.GitRepo.viewPullFiles", git.ErrNotExist{ID: commitID})
+			return
+		}
+
+		ctx.Data["Commit"] = curCommit
+		if prevCommit != nil {
+			ctx.Data["PrevCommitLink"] = path.Join(ctx.Repo.RepoLink, "pulls", strconv.FormatInt(issue.Index, 10), "commits", prevCommit.ID.String())
+		}
+		if nextCommit != nil {
+			ctx.Data["NextCommitLink"] = path.Join(ctx.Repo.RepoLink, "pulls", strconv.FormatInt(issue.Index, 10), "commits", nextCommit.ID.String())
+		}
+
+		statuses, _, err := git_model.GetLatestCommitStatus(ctx, ctx.Repo.Repository.ID, commitID, db.ListOptionsAll)
+		if err != nil {
+			log.Error("GetLatestCommitStatus: %v", err)
+		}
+		if !ctx.Repo.CanRead(unit.TypeActions) {
+			git_model.CommitStatusesHideActionsURL(ctx, statuses)
+		}
+
+		ctx.Data["CommitStatus"] = git_model.CalcCommitStatus(statuses)
+		ctx.Data["CommitStatuses"] = statuses
+
+		verification := asymkey_model.ParseCommitWithSignature(ctx, curCommit)
+		ctx.Data["Verification"] = verification
+		ctx.Data["Author"] = user_model.ValidateCommitWithEmail(ctx, curCommit)
+
+		note := &git.Note{}
+		err = git.GetNote(ctx, ctx.Repo.GitRepo, specifiedEndCommit, note)
+		if err == nil {
+			ctx.Data["NoteCommit"] = note.Commit
+			ctx.Data["NoteAuthor"] = user_model.ValidateCommitWithEmail(ctx, note.Commit)
+			ctx.Data["NoteRendered"], err = markup.RenderCommitMessage(&markup.RenderContext{
+				Links: markup.Links{
+					Base:       ctx.Repo.RepoLink,
+					BranchPath: path.Join("commit", util.PathEscapeSegments(commitID)),
+				},
+				Metas:   ctx.Repo.Repository.ComposeMetas(ctx),
+				GitRepo: ctx.Repo.GitRepo,
+				Ctx:     ctx,
+			}, template.HTMLEscapeString(string(charset.ToUTF8WithFallback(note.Message, charset.ConvertOpts{}))))
+			if err != nil {
+				ctx.ServerError("RenderCommitMessage", err)
+				return
+			}
+		}
+
+		endCommitID = commitID
+		startCommitID = prInfo.MergeBase
+		ctx.Data["IsShowingAllCommits"] = false
+	} else if willShowSpecifiedCommitRange {
 		if len(specifiedEndCommit) > 0 {
 			endCommitID = specifiedEndCommit
 		} else {
@@ -930,6 +1033,7 @@ func viewPullFiles(ctx *context.Context, specifiedStartCommit, specifiedEndCommi
 		} else {
 			startCommitID = prInfo.MergeBase
 		}
+
 		ctx.Data["IsShowingAllCommits"] = false
 	} else {
 		endCommitID = headCommitID
@@ -937,10 +1041,10 @@ func viewPullFiles(ctx *context.Context, specifiedStartCommit, specifiedEndCommi
 		ctx.Data["IsShowingAllCommits"] = true
 	}
 
-	ctx.Data["Username"] = ctx.Repo.Owner.Name
-	ctx.Data["Reponame"] = ctx.Repo.Repository.Name
 	ctx.Data["AfterCommitID"] = endCommitID
 	ctx.Data["BeforeCommitID"] = startCommitID
+	ctx.Data["Username"] = ctx.Repo.Owner.Name
+	ctx.Data["Reponame"] = ctx.Repo.Repository.Name
 
 	fileOnly := ctx.FormBool("file-only")
 

routers/web/web.go

@@ -1510,7 +1510,10 @@ func registerRoutes(m *web.Route) {
 			m.Group("/commits", func() {
 				m.Get("", context.RepoRef(), repo.SetWhitespaceBehavior, repo.GetPullDiffStats, repo.ViewPullCommits)
 				m.Get("/list", context.RepoRef(), repo.GetPullCommits)
-				m.Get("/{sha:[a-f0-9]{4,40}}", context.RepoRef(), repo.SetEditorconfigIfExists, repo.SetDiffViewStyle, repo.SetWhitespaceBehavior, repo.SetShowOutdatedComments, repo.ViewPullFilesForSingleCommit)
+				m.Group("/{sha:[a-f0-9]{4,40}}", func() {
+					m.Get("", context.RepoRef(), repo.SetEditorconfigIfExists, repo.SetDiffViewStyle, repo.SetWhitespaceBehavior, repo.SetShowOutdatedComments, repo.ViewPullFilesForSingleCommit)
+					m.Post("/reviews/submit", context.RepoMustNotBeArchived(), web.Bind(forms.SubmitReviewForm{}), repo.SubmitReview)
+				})
 			})
 			m.Post("/merge", context.RepoMustNotBeArchived(), web.Bind(forms.MergePullRequestForm{}), context.EnforceQuotaWeb(quota_model.LimitSubjectSizeGitAll, context.QuotaTargetRepo), repo.MergePullRequest)
 			m.Post("/cancel_auto_merge", context.RepoMustNotBeArchived(), repo.CancelAutoMergePullRequest)

templates/repo/commit_header.tmpl

@@ -14,10 +14,19 @@
 	{{end}}
 {{end}}
 <div class="ui top attached header clearing segment tw-relative commit-header {{$class}}">
-	<div class="tw-flex tw-mb-4 tw-gap-1">
+	<div class="tw-flex tw-mb-4 tw-gap-1 max-sm:tw-flex-col">
 		<h3 class="tw-mb-0 tw-flex-1"><span class="commit-summary" title="{{.Commit.Summary}}">{{RenderCommitMessage $.Context .Commit.Message ($.Repository.ComposeMetas ctx)}}</span>{{template "repo/commit_statuses" dict "Status" .CommitStatus "Statuses" .CommitStatuses}}</h3>
-		{{if not $.PageIsWiki}}
 		<div class="commit-header-buttons">
+			{{if .PageIsPullFiles}}
+				<div class="ui buttons">
+					<a class="ui small button {{if .PrevCommitLink}}" href="{{.PrevCommitLink}}"{{else}}disabled"{{end}}>
+						{{svg "octicon-chevron-left"}} {{ctx.Locale.Tr "repo.diff.commit.previous-short"}}
+					</a>
+					<a class="ui small button {{if .NextCommitLink}}" href="{{.NextCommitLink}}"{{else}}disabled"{{end}}>
+						{{ctx.Locale.Tr "repo.diff.commit.next-short"}} {{svg "octicon-chevron-right"}}
+					</a>
+				</div>
+			{{else if not $.PageIsWiki}}
 				<a class="ui primary tiny button" href="{{.SourcePath}}">
 					{{ctx.Locale.Tr "repo.diff.browse_source"}}
 				</a>
@@ -132,9 +141,9 @@
 						</div>
 					</div>
 				{{end}}
-			</div>
 			{{end}}
 		</div>
+	</div>
 	{{if IsMultilineCommitMessage .Commit.Message}}
 		<pre class="commit-body">{{RenderCommitBody $.Context .Commit.Message ($.Repository.ComposeMetas ctx)}}</pre>
 	{{end}}
@@ -185,9 +194,16 @@
 			{{end}}
 			<div class="item">
 				<span>{{ctx.Locale.Tr "repo.diff.commit"}}</span>
+				{{if .PageIsPullFiles}}
+					{{$commitShaLink := (printf "%s/commit/%s" $.RepoLink (PathEscape .CommitID))}}
+					<a href="{{$commitShaLink}}" class="ui primary sha label">
+						<span class="shortsha">{{ShortSha .CommitID}}</span>
+					</a>
+				{{else}}
 					<span class="ui primary sha label">
 						<span class="shortsha">{{ShortSha .CommitID}}</span>
 					</span>
+				{{end}}
 			</div>
 		</div>
 </div>

templates/repo/commit_load_branches_and_tags.tmpl

@@ -1,4 +1,4 @@
-{{if not .PageIsWiki}}
+{{if not (or .PageIsWiki .PageIsPullFiles)}}
 <div class="branch-and-tag-area" data-text-default-branch-tooltip="{{ctx.Locale.Tr "repo.commit.contained_in_default_branch"}}">
 	<button class="ui button ellipsis-button load-branches-and-tags tw-mt-2" aria-expanded="false"
 		data-fetch-url="{{.RepoLink}}/commit/{{.CommitID}}/load-branches-and-tags"

templates/repo/commits_list.tmpl

@@ -49,6 +49,9 @@
 								<span class="commit-summary {{if gt .ParentCount 1}} grey text{{end}}" title="{{.Summary}}">{{.Summary | RenderEmoji $.Context}}</span>
 							{{else}}
 								{{$commitLink:= printf "%s/commit/%s" $commitRepoLink (PathEscape .ID.String)}}
+								{{if $.PageIsPullCommits}}
+									{{$commitLink = (printf "%s/pulls/%d/commits/%s" $commitRepoLink $.Issue.Index (PathEscape .ID.String))}}
+								{{end}}
 								<span class="commit-summary {{if gt .ParentCount 1}} grey text{{end}}" title="{{.Summary}}">{{RenderCommitMessageLinkSubject $.Context .Message $commitLink ($.Repository.ComposeMetas ctx)}}</span>
 							{{end}}
 							</span>

templates/repo/commits_list_small.tmpl

@@ -13,6 +13,13 @@
 
 		{{$commitLink := printf "%s/commit/%s" $.comment.Issue.PullRequest.BaseRepo.Link (PathEscape .ID.String)}}
 
+		{{/* Only link those commits in the file diff view that can actually be reviewed there.
+					A force push or other rewriting of history will cause a commit to not be
+					part of the pull request anymore. */}}
+		{{if index $.root.CommitIDs .ID.String}}
+			{{$commitLink = printf "%s/pulls/%d/commits/%s" $.comment.Issue.PullRequest.BaseRepo.Link $.comment.Issue.Index (PathEscape .ID.String)}}
+		{{end}}
+
 		<span class="tw-flex-1 tw-font-mono gt-ellipsis" title="{{.Summary}}">
 			{{- RenderCommitMessageLinkSubject $.root.Context .Message $commitLink ($.comment.Issue.PullRequest.BaseRepo.ComposeMetas ctx) -}}
 		</span>

templates/repo/diff/box.tmpl

@@ -53,7 +53,7 @@
 	{{if not .DiffNotAvailable}}
 		{{if and .IsShowingOnlySingleCommit .PageIsPullFiles}}
 			<div class="ui info message">
-				<div>{{ctx.Locale.Tr "repo.pulls.showing_only_single_commit" (ShortSha .AfterCommitID)}} - <a href="{{$.Issue.Link}}/files?style={{if $.IsSplitStyle}}split{{else}}unified{{end}}&whitespace={{$.WhitespaceBehavior}}&show-outdated={{$.ShowOutdatedComments}}">{{ctx.Locale.Tr "repo.pulls.show_all_commits"}}</a></div>
+				<div>{{ctx.Locale.Tr "repo.pulls.showing_only_single_commit" (ShortSha .CommitID)}} - <a href="{{$.Issue.Link}}/files?style={{if $.IsSplitStyle}}split{{else}}unified{{end}}&whitespace={{$.WhitespaceBehavior}}&show-outdated={{$.ShowOutdatedComments}}">{{ctx.Locale.Tr "repo.pulls.show_all_commits"}}</a></div>
 			</div>
 		{{else if and (not .IsShowingAllCommits) .PageIsPullFiles}}
 			<div class="ui info message">
@@ -93,6 +93,12 @@
 				if (diffTreeVisible) document.getElementById('diff-file-tree').classList.remove('tw-hidden');
 			</script>
 		{{end}}
+		<div id="diff-content-container">
+		{{if .IsShowingOnlySingleCommit}}
+			<div id="diff-commit-header">
+			{{template "repo/commit_header" .}}
+			</div>
+		{{end}}
 		{{if .DiffNotAvailable}}
 			<h4>{{ctx.Locale.Tr "repo.diff.data_not_available"}}</h4>
 		{{else}}
@@ -231,6 +237,7 @@
 			</div>
 		{{end}}
 		</div>
+	</div>
 
 	{{if and (not $.Repository.IsArchived) (not .DiffNotAvailable)}}
 		<template id="issue-comment-editor-template">

templates/repo/diff/new_review.tmpl

@@ -1,17 +1,14 @@
 <div id="review-box">
 	<div
-		{{if not $.IsShowingAllCommits}}
-			data-tooltip-content="{{ctx.Locale.Tr "repo.pulls.review_only_possible_for_full_diff"}}"
-		{{else if .Repository.IsArchived}}
+		{{if .Repository.IsArchived}}
 			data-tooltip-content="{{ctx.Locale.Tr "repo.archive.pull.noreview"}}"
 		{{end}}>
-		<button class="ui tiny primary button tw-pr-1 tw-flex js-btn-review {{if or (not $.IsShowingAllCommits) .Repository.IsArchived}}disabled{{end}}">
+		<button class="ui tiny primary button tw-pr-1 tw-flex js-btn-review {{if .Repository.IsArchived}}disabled{{end}}">
 			{{ctx.Locale.Tr "repo.diff.review"}}
 			<span class="ui small label review-comments-counter" data-pending-comment-number="{{.PendingCodeCommentNumber}}">{{.PendingCodeCommentNumber}}</span>
 			{{svg "octicon-triangle-down" 14 "dropdown icon"}}
 		</button>
 	</div>
-	{{if $.IsShowingAllCommits}}
 	<div class="review-box-panel tippy-target">
 		<div class="ui segment">
 			<form class="ui form form-fetch-action" action="{{.Link}}/reviews/submit" method="post">
@@ -55,5 +52,4 @@
 			</form>
 		</div>
 	</div>
-	{{end}}
 </div>

tests/e2e/pr-review.test.e2e.ts

@@ -11,7 +11,7 @@ import {save_visual, test} from './utils_e2e.ts';
 
 test.use({user: 'user2'});
 
-test('PR: Finish review', async ({page}) => {
+test('PR: Create review from files', async ({page}) => {
   const response = await page.goto('/user2/repo1/pulls/5/files');
   expect(response?.status()).toBe(200);
 
@@ -22,4 +22,93 @@ test('PR: Finish review', async ({page}) => {
   await page.locator('#review-box .js-btn-review').click();
   await expect(page.locator('.tippy-box .review-box-panel')).toBeVisible();
   await save_visual(page);
+
+  await page.locator('.review-box-panel textarea#_combo_markdown_editor_0')
+    .fill('This is a review');
+  await page.locator('.review-box-panel button.btn-submit[value="approve"]').click();
+  await page.waitForURL(/.*\/user2\/repo1\/pulls\/5#issuecomment-\d+/);
+  await save_visual(page);
+});
+
+test('PR: Create review from commit', async ({page}) => {
+  const response = await page.goto('/user2/repo1/pulls/3/commits/4a357436d925b5c974181ff12a994538ddc5a269');
+  expect(response?.status()).toBe(200);
+
+  await page.locator('button.add-code-comment').click();
+  const code_comment = page.locator('.comment-code-cloud form textarea.markdown-text-editor');
+  await expect(code_comment).toBeVisible();
+
+  await code_comment.fill('This is a code comment');
+  await save_visual(page);
+
+  const start_button = page.locator('.comment-code-cloud form button.btn-start-review');
+  // Workaround for #7152, where there might already be a pending review state from previous
+  // test runs (most likely to happen when debugging tests).
+  if (await start_button.isVisible({timeout: 100})) {
+    await start_button.click();
+  } else {
+    await page.locator('.comment-code-cloud form button.btn-add-comment').click();
+  }
+
+  await expect(page.locator('.comment-list .comment-container')).toBeVisible();
+
+  // We need to wait for the review to be processed. Checking the comment counter
+  // conveniently does that.
+  await expect(page.locator('#review-box .js-btn-review > span.review-comments-counter')).toHaveText('1');
+
+  await page.locator('#review-box .js-btn-review').click();
+  await expect(page.locator('.tippy-box .review-box-panel')).toBeVisible();
+  await save_visual(page);
+
+  await page.locator('.review-box-panel textarea.markdown-text-editor')
+    .fill('This is a review');
+  await page.locator('.review-box-panel button.btn-submit[value="approve"]').click();
+  await page.waitForURL(/.*\/user2\/repo1\/pulls\/3#issuecomment-\d+/);
+  await save_visual(page);
+
+  // In addition to testing the ability to delete comments, this also
+  // performs clean up. If tests are run for multiple platforms, the data isn't reset
+  // in-between, and subsequent runs of this test would fail, because when there already is
+  // a comment, the on-hover button to start a conversation doesn't appear anymore.
+  await page.goto('/user2/repo1/pulls/3/commits/4a357436d925b5c974181ff12a994538ddc5a269');
+  await page.locator('.comment-header-right.actions a.context-menu').click();
+
+  await expect(page.locator('.comment-header-right.actions div.menu').getByText(/Copy link.*/)).toBeVisible();
+  // The button to delete a comment will prompt for confirmation using a browser alert.
+  page.on('dialog', (dialog) => dialog.accept());
+  await page.locator('.comment-header-right.actions div.menu .delete-comment').click();
+
+  await expect(page.locator('.comment-list .comment-container')).toBeHidden();
+  await save_visual(page);
+});
+
+test('PR: Navigate by single commit', async ({page}) => {
+  const response = await page.goto('/user2/repo1/pulls/3/commits');
+  expect(response?.status()).toBe(200);
+
+  await page.locator('tbody.commit-list td.message a').nth(1).click();
+  await page.waitForURL(/.*\/user2\/repo1\/pulls\/3\/commits\/4a357436d925b5c974181ff12a994538ddc5a269/);
+  await save_visual(page);
+
+  let prevButton = page.locator('.commit-header-buttons').getByText(/Prev/);
+  let nextButton = page.locator('.commit-header-buttons').getByText(/Next/);
+  await prevButton.waitFor();
+  await nextButton.waitFor();
+
+  await expect(prevButton).toHaveClass(/disabled/);
+  await expect(nextButton).not.toHaveClass(/disabled/);
+  await expect(nextButton).toHaveAttribute('href', '/user2/repo1/pulls/3/commits/5f22f7d0d95d614d25a5b68592adb345a4b5c7fd');
+  await nextButton.click();
+
+  await page.waitForURL(/.*\/user2\/repo1\/pulls\/3\/commits\/5f22f7d0d95d614d25a5b68592adb345a4b5c7fd/);
+  await save_visual(page);
+
+  prevButton = page.locator('.commit-header-buttons').getByText(/Prev/);
+  nextButton = page.locator('.commit-header-buttons').getByText(/Next/);
+  await prevButton.waitFor();
+  await nextButton.waitFor();
+
+  await expect(prevButton).not.toHaveClass(/disabled/);
+  await expect(nextButton).toHaveClass(/disabled/);
+  await expect(prevButton).toHaveAttribute('href', '/user2/repo1/pulls/3/commits/4a357436d925b5c974181ff12a994538ddc5a269');
 });

tests/gitea-repositories-meta/user2/commitsonpr.git/logs/refs/heads/branch1

@@ -1 +1,2 @@
 0000000000000000000000000000000000000000 1978192d98bb1b65e11c2cf37da854fbf94bffd6 Gitea <gitea@fake.local> 1688672383 +0200	push
+1978192d98bb1b65e11c2cf37da854fbf94bffd6 9b93963cf6de4dc33f915bb67f192d099c301f43 Forgejo <forgejo@fake.local> 1749737639 +0200	push

tests/gitea-repositories-meta/user2/commitsonpr.git/refs/heads/branch1

@@ -1 +1 @@
-1978192d98bb1b65e11c2cf37da854fbf94bffd6
+9b93963cf6de4dc33f915bb67f192d099c301f43

tests/gitea-repositories-meta/user2/commitsonpr.git/refs/pull/1/head

@@ -1 +1 @@
-1978192d98bb1b65e11c2cf37da854fbf94bffd6
+9b93963cf6de4dc33f915bb67f192d099c301f43

tests/integration/pull_commit_test.go

@@ -31,3 +31,20 @@ func TestListPullCommits(t *testing.T) {
 	}
 	assert.Equal(t, "4a357436d925b5c974181ff12a994538ddc5a269", pullCommitList.LastReviewCommitSha)
 }
+
+func TestPullCommitLinks(t *testing.T) {
+	defer tests.PrepareTestEnv(t)()
+
+	req := NewRequest(t, "GET", "/user2/repo1/pulls/3/commits")
+	resp := MakeRequest(t, req, http.StatusOK)
+
+	htmlDoc := NewHTMLParser(t, resp.Body)
+
+	commitSha := htmlDoc.Find(".commit-list td.sha a.sha.label").First()
+	commitShaHref, _ := commitSha.Attr("href")
+	assert.Equal(t, "/user2/repo1/pulls/3/commits/5f22f7d0d95d614d25a5b68592adb345a4b5c7fd", commitShaHref)
+
+	commitLink := htmlDoc.Find(".commit-list td.message a").First()
+	commitLinkHref, _ := commitLink.Attr("href")
+	assert.Equal(t, "/user2/repo1/pulls/3/commits/5f22f7d0d95d614d25a5b68592adb345a4b5c7fd", commitLinkHref)
+}

tests/integration/pull_diff_test.go

@@ -14,22 +14,22 @@ import (
 )
 
 func TestPullDiff_CompletePRDiff(t *testing.T) {
-	doTestPRDiff(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"})
+	doTestPRDiff(t, "/user2/commitsonpr/pulls/1/files", []string{"test1.txt", "test10.txt", "test2.txt", "test3.txt", "test4.txt", "test5.txt", "test6.txt", "test7.txt", "test8.txt", "test9.txt"})
 }
 
 func TestPullDiff_SingleCommitPRDiff(t *testing.T) {
-	doTestPRDiff(t, "/user2/commitsonpr/pulls/1/commits/c5626fc9eff57eb1bb7b796b01d4d0f2f3f792a2", true, []string{"test3.txt"})
+	doTestPRDiff(t, "/user2/commitsonpr/pulls/1/commits/c5626fc9eff57eb1bb7b796b01d4d0f2f3f792a2", []string{"test3.txt"})
 }
 
 func TestPullDiff_CommitRangePRDiff(t *testing.T) {
-	doTestPRDiff(t, "/user2/commitsonpr/pulls/1/files/4ca8bcaf27e28504df7bf996819665986b01c847..23576dd018294e476c06e569b6b0f170d0558705", true, []string{"test2.txt", "test3.txt", "test4.txt"})
+	doTestPRDiff(t, "/user2/commitsonpr/pulls/1/files/4ca8bcaf27e28504df7bf996819665986b01c847..23576dd018294e476c06e569b6b0f170d0558705", []string{"test2.txt", "test3.txt", "test4.txt"})
 }
 
 func TestPullDiff_StartingFromBaseToCommitPRDiff(t *testing.T) {
-	doTestPRDiff(t, "/user2/commitsonpr/pulls/1/files/c5626fc9eff57eb1bb7b796b01d4d0f2f3f792a2", true, []string{"test1.txt", "test2.txt", "test3.txt"})
+	doTestPRDiff(t, "/user2/commitsonpr/pulls/1/files/c5626fc9eff57eb1bb7b796b01d4d0f2f3f792a2", []string{"test1.txt", "test2.txt", "test3.txt"})
 }
 
-func doTestPRDiff(t *testing.T, prDiffURL string, reviewBtnDisabled bool, expectedFilenames []string) {
+func doTestPRDiff(t *testing.T, prDiffURL string, expectedFilenames []string) {
 	defer tests.PrepareTestEnv(t)()
 
 	session := loginUser(t, "user2")
@@ -52,7 +52,4 @@ func doTestPRDiff(t *testing.T, prDiffURL string, reviewBtnDisabled bool, expect
 		filename, _ := s.Attr("data-old-filename")
 		assert.Equal(t, expectedFilenames[i], filename)
 	})
-
-	// Ensure the review button is enabled for full PR reviews
-	assert.Equal(t, reviewBtnDisabled, doc.doc.Find(".js-btn-review").HasClass("disabled"))
 }

tests/integration/pull_files_test.go

@@ -0,0 +1,106 @@
+// Copyright 2025 The Forgejo Authors. All rights reserved.
+// SPDX-License-Identifier: MIT
+
+package integration
+
+import (
+	"net/http"
+	"strings"
+	"testing"
+	"time"
+
+	repo_model "forgejo.org/models/repo"
+	"forgejo.org/models/unittest"
+	"forgejo.org/modules/git"
+	"forgejo.org/tests"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+func TestPullFilesCommitHeader(t *testing.T) {
+	defer tests.PrepareTestEnv(t)()
+
+	t.Run("Verify commit info", func(t *testing.T) {
+		repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 1})
+		gitRepo, err := git.OpenRepository(git.DefaultContext, repo.RepoPath())
+		require.NoError(t, err)
+		defer gitRepo.Close()
+
+		commit, err := gitRepo.GetCommit("62fb502a7172d4453f0322a2cc85bddffa57f07a")
+		require.NoError(t, err)
+
+		req := NewRequest(t, "GET", "/user2/repo1/pulls/5/commits/62fb502a7172d4453f0322a2cc85bddffa57f07a")
+		resp := MakeRequest(t, req, http.StatusOK)
+
+		htmlDoc := NewHTMLParser(t, resp.Body)
+		header := htmlDoc.doc.Find("#diff-commit-header")
+
+		summary := header.Find(".commit-header h3")
+		assert.Equal(t, commit.Summary(), strings.TrimSpace(summary.Text()))
+
+		author := header.Find(".author strong")
+		assert.Equal(t, commit.Author.Name, author.Text())
+
+		date, _ := header.Find("#authored-time relative-time").Attr("datetime")
+		assert.Equal(t, commit.Author.When.Format(time.RFC3339), date)
+
+		sha := header.Find(".commit-header-row .sha.label")
+		shaHref, _ := sha.Attr("href")
+		assert.Equal(t, commit.ID.String()[:10], sha.Find(".shortsha").Text())
+		assert.Equal(t, "/user2/repo1/commit/62fb502a7172d4453f0322a2cc85bddffa57f07a", shaHref)
+	})
+
+	t.Run("Navigation", func(t *testing.T) {
+		t.Run("No previous on first commit", func(t *testing.T) {
+			req := NewRequest(t, "GET", "/user2/commitsonpr/pulls/1/commits/4ca8bcaf27e28504df7bf996819665986b01c847")
+			resp := MakeRequest(t, req, http.StatusOK)
+
+			htmlDoc := NewHTMLParser(t, resp.Body)
+			buttons := htmlDoc.doc.Find(".commit-header-buttons a.small.button")
+
+			assert.Equal(t, 2, buttons.Length(), "expected two buttons in commit header")
+
+			assert.True(t, buttons.First().HasClass("disabled"), "'prev' button should be disabled")
+			assert.False(t, buttons.Last().HasClass("disabled"), "'next' button should not be disabled")
+
+			href, _ := buttons.Last().Attr("href")
+			assert.Equal(t, "/user2/commitsonpr/pulls/1/commits/96cef4a7b72b3c208340ae6f0cf55a93e9077c93", href)
+		})
+
+		t.Run("No next on last commit", func(t *testing.T) {
+			req := NewRequest(t, "GET", "/user2/commitsonpr/pulls/1/commits/9b93963cf6de4dc33f915bb67f192d099c301f43")
+			resp := MakeRequest(t, req, http.StatusOK)
+
+			htmlDoc := NewHTMLParser(t, resp.Body)
+			buttons := htmlDoc.doc.Find(".commit-header-buttons a.small.button")
+
+			assert.Equal(t, 2, buttons.Length(), "expected two buttons in commit header")
+
+			assert.False(t, buttons.First().HasClass("disabled"), "'prev' button should not be disabled")
+			assert.True(t, buttons.Last().HasClass("disabled"), "'next' button should be disabled")
+
+			href, _ := buttons.First().Attr("href")
+			assert.Equal(t, "/user2/commitsonpr/pulls/1/commits/2d65d92dc800c6f448541240c18e82bf36b954bb", href)
+		})
+
+		t.Run("Both directions on middle commit", func(t *testing.T) {
+			req := NewRequest(t, "GET", "/user2/commitsonpr/pulls/1/commits/c5626fc9eff57eb1bb7b796b01d4d0f2f3f792a2")
+			resp := MakeRequest(t, req, http.StatusOK)
+
+			htmlDoc := NewHTMLParser(t, resp.Body)
+			buttons := htmlDoc.doc.Find(".commit-header-buttons a.small.button")
+
+			assert.Equal(t, 2, buttons.Length(), "expected two buttons in commit header")
+
+			assert.False(t, buttons.First().HasClass("disabled"), "'prev' button should not be disabled")
+			assert.False(t, buttons.Last().HasClass("disabled"), "'next' button should not be disabled")
+
+			href, _ := buttons.First().Attr("href")
+			assert.Equal(t, "/user2/commitsonpr/pulls/1/commits/96cef4a7b72b3c208340ae6f0cf55a93e9077c93", href)
+
+			href, _ = buttons.Last().Attr("href")
+			assert.Equal(t, "/user2/commitsonpr/pulls/1/commits/23576dd018294e476c06e569b6b0f170d0558705", href)
+		})
+	})
+}

tests/integration/pull_test.go

@@ -30,6 +30,43 @@ func TestViewPulls(t *testing.T) {
 	assert.Equal(t, "Search pulls…", placeholder)
 }
 
+func TestPullViewConversation(t *testing.T) {
+	defer tests.PrepareTestEnv(t)()
+
+	req := NewRequest(t, "GET", "/user2/commitsonpr/pulls/1")
+	resp := MakeRequest(t, req, http.StatusOK)
+
+	htmlDoc := NewHTMLParser(t, resp.Body)
+
+	t.Run("Commits", func(t *testing.T) {
+		commitLists := htmlDoc.Find(".timeline-item.commits-list")
+		assert.Equal(t, 4, commitLists.Length())
+
+		commits := commitLists.Find(".singular-commit")
+		assert.Equal(t, 10, commits.Length())
+
+		// First one has not been affected by a force push, therefore it's still part of the
+		// PR and should link to the PR-scoped review tab
+		firstCommit := commits.Eq(0)
+		firstCommitMessageHref, _ := firstCommit.Find("a.default-link").Attr("href")
+		firstCommitShaHref, _ := firstCommit.Find("a.sha.label").Attr("href")
+		assert.Equal(t, "/user2/commitsonpr/pulls/1/commits/4ca8bcaf27e28504df7bf996819665986b01c847", firstCommitMessageHref)
+		assert.Equal(t, "/user2/commitsonpr/pulls/1/commits/4ca8bcaf27e28504df7bf996819665986b01c847", firstCommitShaHref)
+
+		// The fifth commit has been overwritten by a force push.
+		// Attempting to view the old one in the review tab won't work:
+		req := NewRequest(t, "GET", "/user2/commitsonpr/pulls/1/commits/3e64625bd6eb5bcba69ac97de6c8f507402df861")
+		MakeRequest(t, req, http.StatusNotFound)
+
+		// Therefore, this commit should link to the non-PR commit view instead
+		fifthCommit := commits.Eq(4)
+		fifthCommitMessageHref, _ := fifthCommit.Find("a.default-link").Attr("href")
+		fifthCommitShaHref, _ := fifthCommit.Find("a.sha.label").Attr("href")
+		assert.Equal(t, "/user2/commitsonpr/commit/3e64625bd6eb5bcba69ac97de6c8f507402df861", fifthCommitMessageHref)
+		assert.Equal(t, "/user2/commitsonpr/commit/3e64625bd6eb5bcba69ac97de6c8f507402df861", fifthCommitShaHref)
+	})
+}
+
 func TestPullManuallyMergeWarning(t *testing.T) {
 	defer tests.PrepareTestEnv(t)()
 

web_src/css/repo.css

@@ -2468,8 +2468,21 @@ tbody.commit-list {
   display: flex;
 }
 
-#diff-file-boxes {
+#diff-content-container {
   flex: 1;
+}
+
+#diff-commit-header {
+  /* Counteract the `+2px` for width in `.segment` */
+  padding: 0 2px;
+}
+
+#diff-commit-header + h4,
+#diff-commit-header + #diff-file-boxes {
+  margin-top: 8px;
+}
+
+#diff-file-boxes {
   max-width: 100%;
   display: flex;
   flex-direction: column;