Improve repo commit view (#33877)

Fix #24623 Major changes: 1. Redirect `/owner/repo/blob/*` requests to `/owner/repo/src/commit/*` (like GitHub) 2. Add a "view file diff" link (see screenshot below) 3. Refactor "AssertHTMLElement" to generic, now we can accurately assert existence or number. 4. Add more tests --------- Co-authored-by: silverwind <me@silverwind.io> Co-authored-by: delvh <dev.lh@web.de>
2025-03-21 10:50:11 +03:00 · 2025-03-14 15:45:11 +08:00 · 2025-03-14 15:45:11 +08:00 · b094f9b75d
commit b094f9b75d
parent 9c673d066c
12 changed files with 116 additions and 46 deletions
--- a/options/locale/locale_en-US.ini
+++ b/options/locale/locale_en-US.ini
@ -1407,6 +1407,7 @@ commits.signed_by_untrusted_user_unmatched = Signed by untrusted user who does n
 commits.gpg_key_id = GPG Key ID
 commits.ssh_key_fingerprint = SSH Key Fingerprint
 commits.view_path=View at this point in history
+commits.view_file_diff = View changes to this file in this commit

 commit.operations = Operations
 commit.revert = Revert
--- a/routers/web/repo/view_home.go
+++ b/routers/web/repo/view_home.go
@ -429,3 +429,14 @@ func RedirectRepoTreeToSrc(ctx *context.Context) {
 	}
 	ctx.Redirect(redirect)
 }
+
+func RedirectRepoBlobToCommit(ctx *context.Context) {
+	// redirect "/owner/repo/blob/*" requests to "/owner/repo/src/commit/*"
+	// just like GitHub: browse files of a commit by "https://github/owner/repo/blob/{CommitID}"
+	// TODO: maybe we could guess more types to redirect to the related pages in the future
+	redirect := ctx.Repo.RepoLink + "/src/commit/" + ctx.PathParamRaw("*")
+	if ctx.Req.URL.RawQuery != "" {
+		redirect += "?" + ctx.Req.URL.RawQuery
+	}
+	ctx.Redirect(redirect)
+}
--- a/routers/web/web.go
+++ b/routers/web/web.go
@ -1595,7 +1595,8 @@ func registerRoutes(m *web.Router) {
 			m.Get("/commit/*", context.RepoRefByType(git.RefTypeCommit), repo.Home)
 			m.Get("/*", context.RepoRefByType(""), repo.Home) // "/*" route is deprecated, and kept for backward compatibility
 		}, repo.SetEditorconfigIfExists)
-		m.Get("/tree/*", repo.RedirectRepoTreeToSrc) // redirect "/owner/repo/tree/*" requests to "/owner/repo/src/*"
+		m.Get("/tree/*", repo.RedirectRepoTreeToSrc)    // redirect "/owner/repo/tree/*" requests to "/owner/repo/src/*"
+		m.Get("/blob/*", repo.RedirectRepoBlobToCommit) // redirect "/owner/repo/blob/*" requests to "/owner/repo/src/commit/*"

 		m.Get("/forks", context.RepoRef(), repo.Forks)
 		m.Get("/commit/{sha:([a-f0-9]{7,64})}.{ext:patch|diff}", repo.MustBeNotEmpty, repo.RawDiff)
--- a/templates/repo/commits_list.tmpl
+++ b/templates/repo/commits_list.tmpl
@ -66,11 +66,20 @@
 						<td class="text right aligned">{{DateUtils.TimeSince .Author.When}}</td>
 					{{end}}
 					<td class="text right aligned tw-py-0">
-						<button class="btn interact-bg tw-p-2" data-tooltip-content="{{ctx.Locale.Tr "copy_hash"}}" data-clipboard-text="{{.ID}}">{{svg "octicon-copy"}}</button>
-						{{if not $.PageIsWiki}}{{/* at the moment, wiki doesn't support "view at history point*/}}
+						<button class="btn interact-bg tw-p-2 copy-commit-id" data-tooltip-content="{{ctx.Locale.Tr "copy_hash"}}" data-clipboard-text="{{.ID}}">{{svg "octicon-copy"}}</button>
+						{{/* at the moment, wiki doesn't support these "view" links like "view at history point" */}}
+						{{if not $.PageIsWiki}}
+							{{/* view single file diff */}}
+							{{if $.FileName}}
+							<a class="btn interact-bg tw-p-2 view-single-diff" data-tooltip-content="{{ctx.Locale.Tr "repo.commits.view_file_diff"}}"
+								href="{{$commitRepoLink}}/commit/{{.ID.String}}?files={{$.FileName}}"
+							>{{svg "octicon-file-diff"}}</a>
+							{{end}}
+
+							{{/* view at history point */}}
 							{{$viewCommitLink := printf "%s/src/commit/%s" $commitRepoLink (PathEscape .ID.String)}}
 							{{if $.FileName}}{{$viewCommitLink = printf "%s/%s" $viewCommitLink (PathEscapeSegments $.FileName)}}{{end}}
-							<a class="btn interact-bg tw-p-2" data-tooltip-content="{{ctx.Locale.Tr "repo.commits.view_path"}}" href="{{$viewCommitLink}}">{{svg "octicon-file-code"}}</a>
+							<a class="btn interact-bg tw-p-2 view-commit-path" data-tooltip-content="{{ctx.Locale.Tr "repo.commits.view_path"}}" href="{{$viewCommitLink}}">{{svg "octicon-file-code"}}</a>
 						{{end}}
 					</td>
 				</tr>
--- a/tests/integration/html_helper.go
+++ b/tests/integration/html_helper.go
@ -42,12 +42,13 @@ func (doc *HTMLDoc) GetCSRF() string {
 	return doc.GetInputValueByName("_csrf")
 }

-// AssertElement check if element by selector exists or does not exist depending on checkExists
-func (doc *HTMLDoc) AssertElement(t testing.TB, selector string, checkExists bool) {
+// AssertHTMLElement check if element by selector exists or does not exist depending on checkExists
+func AssertHTMLElement[T int | bool](t testing.TB, doc *HTMLDoc, selector string, checkExists T) {
 	sel := doc.doc.Find(selector)
-	if checkExists {
-		assert.Equal(t, 1, sel.Length())
-	} else {
-		assert.Equal(t, 0, sel.Length())
+	switch v := any(checkExists).(type) {
+	case bool:
+		assert.Equal(t, v, sel.Length() > 0)
+	case int:
+		assert.Equal(t, v, sel.Length())
 	}
 }
--- a/tests/integration/links_test.go
+++ b/tests/integration/links_test.go
@ -55,6 +55,7 @@ func TestRedirectsNoLogin(t *testing.T) {
 		{"/user2/repo1/src/master/a%2fb.txt", "/user2/repo1/src/branch/master/a%2fb.txt"},
 		{"/user2/repo1/src/master/directory/file.txt?a=1", "/user2/repo1/src/branch/master/directory/file.txt?a=1"},
 		{"/user2/repo1/tree/a%2fb?a=1", "/user2/repo1/src/a%2fb?a=1"},
+		{"/user2/repo1/blob/123456/%20?a=1", "/user2/repo1/src/commit/123456/%20?a=1"},
 		{"/user/avatar/GhosT/-1", "/assets/img/avatar_default.png"},
 		{"/user/avatar/Gitea-ActionS/0", "/assets/img/avatar_default.png"},
 		{"/api/v1/swagger", "/api/swagger"},
--- a/tests/integration/oauth_test.go
+++ b/tests/integration/oauth_test.go
@ -77,7 +77,7 @@ func TestAuthorizeShow(t *testing.T) {
 	resp := ctx.MakeRequest(t, req, http.StatusOK)

 	htmlDoc := NewHTMLParser(t, resp.Body)
-	htmlDoc.AssertElement(t, "#authorize-app", true)
+	AssertHTMLElement(t, htmlDoc, "#authorize-app", true)
 	htmlDoc.GetCSRF()
 }

--- a/tests/integration/signin_test.go
+++ b/tests/integration/signin_test.go
@ -112,7 +112,8 @@ func TestEnablePasswordSignInFormAndEnablePasskeyAuth(t *testing.T) {

 		req := NewRequest(t, "GET", "/user/login")
 		resp := MakeRequest(t, req, http.StatusOK)
-		NewHTMLParser(t, resp.Body).AssertElement(t, "form[action='/user/login']", false)
+		doc := NewHTMLParser(t, resp.Body)
+		AssertHTMLElement(t, doc, "form[action='/user/login']", false)

 		req = NewRequest(t, "POST", "/user/login")
 		MakeRequest(t, req, http.StatusForbidden)
@ -121,7 +122,8 @@ func TestEnablePasswordSignInFormAndEnablePasskeyAuth(t *testing.T) {
 		defer web.RouteMockReset()
 		web.RouteMock(web.MockAfterMiddlewares, mockLinkAccount)
 		resp = MakeRequest(t, req, http.StatusOK)
-		NewHTMLParser(t, resp.Body).AssertElement(t, "form[action='/user/link_account_signin']", false)
+		doc = NewHTMLParser(t, resp.Body)
+		AssertHTMLElement(t, doc, "form[action='/user/link_account_signin']", false)
 	})

 	t.Run("EnablePasswordSignInForm=true", func(t *testing.T) {
@ -130,7 +132,8 @@ func TestEnablePasswordSignInFormAndEnablePasskeyAuth(t *testing.T) {

 		req := NewRequest(t, "GET", "/user/login")
 		resp := MakeRequest(t, req, http.StatusOK)
-		NewHTMLParser(t, resp.Body).AssertElement(t, "form[action='/user/login']", true)
+		doc := NewHTMLParser(t, resp.Body)
+		AssertHTMLElement(t, doc, "form[action='/user/login']", true)

 		req = NewRequest(t, "POST", "/user/login")
 		MakeRequest(t, req, http.StatusOK)
@ -139,7 +142,8 @@ func TestEnablePasswordSignInFormAndEnablePasskeyAuth(t *testing.T) {
 		defer web.RouteMockReset()
 		web.RouteMock(web.MockAfterMiddlewares, mockLinkAccount)
 		resp = MakeRequest(t, req, http.StatusOK)
-		NewHTMLParser(t, resp.Body).AssertElement(t, "form[action='/user/link_account_signin']", true)
+		doc = NewHTMLParser(t, resp.Body)
+		AssertHTMLElement(t, doc, "form[action='/user/link_account_signin']", true)
 	})

 	t.Run("EnablePasskeyAuth=false", func(t *testing.T) {
@ -148,7 +152,8 @@ func TestEnablePasswordSignInFormAndEnablePasskeyAuth(t *testing.T) {

 		req := NewRequest(t, "GET", "/user/login")
 		resp := MakeRequest(t, req, http.StatusOK)
-		NewHTMLParser(t, resp.Body).AssertElement(t, ".signin-passkey", false)
+		doc := NewHTMLParser(t, resp.Body)
+		AssertHTMLElement(t, doc, ".signin-passkey", false)
 	})

 	t.Run("EnablePasskeyAuth=true", func(t *testing.T) {
@ -157,6 +162,7 @@ func TestEnablePasswordSignInFormAndEnablePasskeyAuth(t *testing.T) {

 		req := NewRequest(t, "GET", "/user/login")
 		resp := MakeRequest(t, req, http.StatusOK)
-		NewHTMLParser(t, resp.Body).AssertElement(t, ".signin-passkey", true)
+		doc := NewHTMLParser(t, resp.Body)
+		AssertHTMLElement(t, doc, ".signin-passkey", true)
 	})
 }
--- a/tests/integration/timetracking_test.go
+++ b/tests/integration/timetracking_test.go
@ -42,8 +42,8 @@ func testViewTimetrackingControls(t *testing.T, session *TestSession, user, repo

 	htmlDoc := NewHTMLParser(t, resp.Body)

-	htmlDoc.AssertElement(t, ".issue-start-time", canTrackTime)
-	htmlDoc.AssertElement(t, ".issue-add-time", canTrackTime)
+	AssertHTMLElement(t, htmlDoc, ".issue-start-time", canTrackTime)
+	AssertHTMLElement(t, htmlDoc, ".issue-add-time", canTrackTime)

 	issueLink := path.Join(user, repo, "issues", issue)
 	req = NewRequestWithValues(t, "POST", path.Join(issueLink, "times", "stopwatch", "toggle"), map[string]string{
@ -59,8 +59,8 @@ func testViewTimetrackingControls(t *testing.T, session *TestSession, user, repo
 		events := htmlDoc.doc.Find(".event > span.text")
 		assert.Contains(t, events.Last().Text(), "started working")

-		htmlDoc.AssertElement(t, ".issue-stop-time", true)
-		htmlDoc.AssertElement(t, ".issue-cancel-time", true)
+		AssertHTMLElement(t, htmlDoc, ".issue-stop-time", true)
+		AssertHTMLElement(t, htmlDoc, ".issue-cancel-time", true)

 		// Sleep for 1 second to not get wrong order for stopping timer
 		time.Sleep(time.Second)
--- a/tests/integration/user_settings_test.go
+++ b/tests/integration/user_settings_test.go
@ -19,21 +19,21 @@ import (
 func assertNavbar(t *testing.T, doc *HTMLDoc) {
 	// Only show the account page if users can change their email notifications, delete themselves, or manage credentials
 	if setting.Admin.UserDisabledFeatures.Contains(setting.UserFeatureDeletion, setting.UserFeatureManageCredentials) && !setting.Service.EnableNotifyMail {
-		doc.AssertElement(t, ".menu a[href='/user/settings/account']", false)
+		AssertHTMLElement(t, doc, ".menu a[href='/user/settings/account']", false)
 	} else {
-		doc.AssertElement(t, ".menu a[href='/user/settings/account']", true)
+		AssertHTMLElement(t, doc, ".menu a[href='/user/settings/account']", true)
 	}

 	if setting.Admin.UserDisabledFeatures.Contains(setting.UserFeatureManageMFA, setting.UserFeatureManageCredentials) {
-		doc.AssertElement(t, ".menu a[href='/user/settings/security']", false)
+		AssertHTMLElement(t, doc, ".menu a[href='/user/settings/security']", false)
 	} else {
-		doc.AssertElement(t, ".menu a[href='/user/settings/security']", true)
+		AssertHTMLElement(t, doc, ".menu a[href='/user/settings/security']", true)
 	}

 	if setting.Admin.UserDisabledFeatures.Contains(setting.UserFeatureManageSSHKeys, setting.UserFeatureManageGPGKeys) {
-		doc.AssertElement(t, ".menu a[href='/user/settings/keys']", false)
+		AssertHTMLElement(t, doc, ".menu a[href='/user/settings/keys']", false)
 	} else {
-		doc.AssertElement(t, ".menu a[href='/user/settings/keys']", true)
+		AssertHTMLElement(t, doc, ".menu a[href='/user/settings/keys']", true)
 	}
 }

@ -64,11 +64,11 @@ func TestUserSettingsAccount(t *testing.T) {
 		doc := NewHTMLParser(t, resp.Body)

 		// account navbar should display
-		doc.AssertElement(t, ".menu a[href='/user/settings/account']", true)
+		AssertHTMLElement(t, doc, ".menu a[href='/user/settings/account']", true)

-		doc.AssertElement(t, "#password", true)
-		doc.AssertElement(t, "#email", true)
-		doc.AssertElement(t, "#delete-form", true)
+		AssertHTMLElement(t, doc, "#password", true)
+		AssertHTMLElement(t, doc, "#email", true)
+		AssertHTMLElement(t, doc, "#delete-form", true)
 	})

 	t.Run("credentials disabled", func(t *testing.T) {
@ -83,9 +83,9 @@ func TestUserSettingsAccount(t *testing.T) {

 		assertNavbar(t, doc)

-		doc.AssertElement(t, "#password", false)
-		doc.AssertElement(t, "#email", false)
-		doc.AssertElement(t, "#delete-form", true)
+		AssertHTMLElement(t, doc, "#password", false)
+		AssertHTMLElement(t, doc, "#email", false)
+		AssertHTMLElement(t, doc, "#delete-form", true)
 	})

 	t.Run("deletion disabled", func(t *testing.T) {
@ -100,9 +100,9 @@ func TestUserSettingsAccount(t *testing.T) {

 		assertNavbar(t, doc)

-		doc.AssertElement(t, "#password", true)
-		doc.AssertElement(t, "#email", true)
-		doc.AssertElement(t, "#delete-form", false)
+		AssertHTMLElement(t, doc, "#password", true)
+		AssertHTMLElement(t, doc, "#email", true)
+		AssertHTMLElement(t, doc, "#delete-form", false)
 	})

 	t.Run("deletion, credentials and email notifications are disabled", func(t *testing.T) {
@ -249,7 +249,7 @@ func TestUserSettingsSecurity(t *testing.T) {

 		assertNavbar(t, doc)

-		doc.AssertElement(t, "#register-webauthn", true)
+		AssertHTMLElement(t, doc, "#register-webauthn", true)
 	})

 	t.Run("mfa disabled", func(t *testing.T) {
@ -263,7 +263,7 @@ func TestUserSettingsSecurity(t *testing.T) {

 		assertNavbar(t, doc)

-		doc.AssertElement(t, "#register-webauthn", false)
+		AssertHTMLElement(t, doc, "#register-webauthn", false)
 	})

 	t.Run("credentials and mfa disabled", func(t *testing.T) {
@ -356,8 +356,8 @@ func TestUserSettingsKeys(t *testing.T) {

 		assertNavbar(t, doc)

-		doc.AssertElement(t, "#add-ssh-button", true)
-		doc.AssertElement(t, "#add-gpg-key-panel", true)
+		AssertHTMLElement(t, doc, "#add-ssh-button", true)
+		AssertHTMLElement(t, doc, "#add-gpg-key-panel", true)
 	})

 	t.Run("ssh keys disabled", func(t *testing.T) {
@ -372,8 +372,8 @@ func TestUserSettingsKeys(t *testing.T) {

 		assertNavbar(t, doc)

-		doc.AssertElement(t, "#add-ssh-button", false)
-		doc.AssertElement(t, "#add-gpg-key-panel", true)
+		AssertHTMLElement(t, doc, "#add-ssh-button", false)
+		AssertHTMLElement(t, doc, "#add-gpg-key-panel", true)
 	})

 	t.Run("gpg keys disabled", func(t *testing.T) {
@ -388,8 +388,8 @@ func TestUserSettingsKeys(t *testing.T) {

 		assertNavbar(t, doc)

-		doc.AssertElement(t, "#add-ssh-button", true)
-		doc.AssertElement(t, "#add-gpg-key-panel", false)
+		AssertHTMLElement(t, doc, "#add-ssh-button", true)
+		AssertHTMLElement(t, doc, "#add-gpg-key-panel", false)
 	})

 	t.Run("ssh & gpg keys disabled", func(t *testing.T) {
--- a/tests/integration/user_test.go
+++ b/tests/integration/user_test.go
@ -273,5 +273,5 @@ func TestUserLocationMapLink(t *testing.T) {
 	req = NewRequest(t, "GET", "/user2/")
 	resp := session.MakeRequest(t, req, http.StatusOK)
 	htmlDoc := NewHTMLParser(t, resp.Body)
-	htmlDoc.AssertElement(t, `a[href="https://example/foo/A%2Fb"]`, true)
+	AssertHTMLElement(t, htmlDoc, `a[href="https://example/foo/A%2Fb"]`, true)
 }
--- a/tests/integration/view_test.go
+++ b/tests/integration/view_test.go
@ -25,3 +25,43 @@ func TestRenderFileSVGIsInImgTag(t *testing.T) {
 	assert.True(t, exists, "The SVG image should be in an <img> tag so that scripts in the SVG are not run")
 	assert.Equal(t, "/user2/repo2/raw/branch/master/line.svg", src)
 }
+
+func TestCommitListActions(t *testing.T) {
+	defer tests.PrepareTestEnv(t)()
+	session := loginUser(t, "user2")
+
+	t.Run("WikiRevisionList", func(t *testing.T) {
+		defer tests.PrintCurrentTest(t)()
+
+		req := NewRequest(t, "GET", "/user2/repo1/wiki/Home?action=_revision")
+		resp := session.MakeRequest(t, req, http.StatusOK)
+		htmlDoc := NewHTMLParser(t, resp.Body)
+		AssertHTMLElement(t, htmlDoc, ".commit-list .copy-commit-id", true)
+		AssertHTMLElement(t, htmlDoc, `.commit-list .view-single-diff`, false)
+		AssertHTMLElement(t, htmlDoc, `.commit-list .view-commit-path`, false)
+	})
+
+	t.Run("RepoCommitList", func(t *testing.T) {
+		defer tests.PrintCurrentTest(t)()
+
+		req := NewRequest(t, "GET", "/user2/repo1/commits/branch/master")
+		resp := session.MakeRequest(t, req, http.StatusOK)
+		htmlDoc := NewHTMLParser(t, resp.Body)
+
+		AssertHTMLElement(t, htmlDoc, `.commit-list .copy-commit-id`, true)
+		AssertHTMLElement(t, htmlDoc, `.commit-list .view-single-diff`, false)
+		AssertHTMLElement(t, htmlDoc, `.commit-list .view-commit-path`, true)
+	})
+
+	t.Run("RepoFileHistory", func(t *testing.T) {
+		defer tests.PrintCurrentTest(t)()
+
+		req := NewRequest(t, "GET", "/user2/repo1/commits/branch/master/README.md")
+		resp := session.MakeRequest(t, req, http.StatusOK)
+		htmlDoc := NewHTMLParser(t, resp.Body)
+
+		AssertHTMLElement(t, htmlDoc, `.commit-list .copy-commit-id`, true)
+		AssertHTMLElement(t, htmlDoc, `.commit-list .view-single-diff`, true)
+		AssertHTMLElement(t, htmlDoc, `.commit-list .view-commit-path`, true)
+	})
+}