Fix various problems (#37547)

1. Fix ugly commit form "warning" message
2. Use JSONError for "Update PR Branch" response 
3. Remove useless "timeline" class
4. Make timeline review default to "comment" to avoid icon missing
5. Align PR's "command line instructions" UI
6. Simply "Update PR branch" button logic

And then some TODOs are fixed.

---------

Signed-off-by: wxiaoguang <wxiaoguang@gmail.com>

options/locale/locale_en-US.json

@@ -1306,7 +1306,7 @@
   "repo.editor.upload_file_is_locked": "File \"%s\" is locked by %s.",
   "repo.editor.upload_files_to_dir": "Upload files to \"%s\"",
   "repo.editor.cannot_commit_to_protected_branch": "Cannot commit to protected branch \"%s\".",
-  "repo.editor.no_commit_to_branch": "Unable to commit directly to branch because:",
+  "repo.editor.no_commit_to_branch": "Not allowed to commit directly to branch because:",
   "repo.editor.user_no_push_to_branch": "User cannot push to branch",
   "repo.editor.require_signed_commit": "Branch requires a signed commit",
   "repo.editor.cherry_pick": "Cherry-pick %s onto:",

routers/web/repo/pull.go

@@ -974,17 +974,15 @@ func UpdatePullRequest(ctx *context.Context) {
 		return
 	}
 
-	// ToDo: add check if maintainers are allowed to change branch ... (need migration & co)
 	if (!allowedUpdateByMerge && !rebase) || (rebase && !allowedUpdateByRebase) {
-		ctx.Flash.Error(ctx.Tr("repo.pulls.update_not_allowed"))
+		ctx.JSONError(ctx.Tr("repo.pulls.update_not_allowed"))
-		ctx.Redirect(issue.Link())
 		return
 	}
 
 	// default merge commit message
 	message := fmt.Sprintf("Merge branch '%s' into %s", issue.PullRequest.BaseBranch, issue.PullRequest.HeadBranch)
 
-	// The update process should not be cancelled by the user
+	// The update process should not be canceled by the user
 	// so we set the context to be a background context
 	if err = pull_service.Update(graceful.GetManager().ShutdownContext(), issue.PullRequest, ctx.Doer, message, rebase); err != nil {
 		if pull_service.IsErrMergeConflicts(err) {
@@ -998,8 +996,7 @@ func UpdatePullRequest(ctx *context.Context) {
 				ctx.ServerError("UpdatePullRequest.HTMLString", err)
 				return
 			}
-			ctx.Flash.Error(flashError)
+			ctx.JSONError(flashError)
-			ctx.Redirect(issue.Link())
 			return
 		} else if pull_service.IsErrRebaseConflicts(err) {
 			conflictError := err.(pull_service.ErrRebaseConflicts)
@@ -1012,19 +1009,18 @@ func UpdatePullRequest(ctx *context.Context) {
 				ctx.ServerError("UpdatePullRequest.HTMLString", err)
 				return
 			}
-			ctx.Flash.Error(flashError)
+			ctx.JSONError(flashError)
-			ctx.Redirect(issue.Link())
 			return
 		}
-		ctx.Flash.Error(err.Error())
+		log.Error("Update pull request failed: %v", err)
-		ctx.Redirect(issue.Link())
+		ctx.JSONError("Unable to update pull request")
 		return
 	}
 
-	time.Sleep(1 * time.Second)
+	time.Sleep(100 * time.Millisecond) // TODO: it is really questionable whether the Sleep is useful here, need to figure out
 
 	ctx.Flash.Success(ctx.Tr("repo.pulls.update_branch_success"))
-	ctx.Redirect(issue.Link())
+	ctx.JSONRedirect(issue.Link())
 }
 
 // MergePullRequest response for merging pull request

templates/repo/editor/commit_form.tmpl

@@ -22,7 +22,7 @@
 				<label>{{ctx.Locale.Tr "repo.editor.signoff_desc"}}</label>
 			</div>
 		</div>
-		<div class="quick-pull-choice js-quick-pull-choice">
+		<div class="field quick-pull-choice js-quick-pull-choice">
 			<div class="field">
 				<div class="ui radio checkbox {{if not .CommitFormOptions.CanCommitToBranch}}disabled{{end}}">
 					<input type="radio" class="js-quick-pull-choice-option" name="commit_choice" value="direct" data-button-text="{{ctx.Locale.Tr "repo.editor.commit_changes"}}" {{if eq .commit_choice "direct"}}checked{{end}}>
@@ -30,9 +30,9 @@
 						{{svg "octicon-git-commit"}}
 						{{ctx.Locale.Tr "repo.editor.commit_directly_to_this_branch" .BranchName}}
 						{{if not .CommitFormOptions.CanCommitToBranch}}
-						<div class="ui visible small warning message">
+						<div class="tw-mt-2">
 							{{ctx.Locale.Tr "repo.editor.no_commit_to_branch"}}
-							<ul>
+							<ul class="tw-mb-0">
 								{{if not .CommitFormOptions.UserCanPush}}<li>{{ctx.Locale.Tr "repo.editor.user_no_push_to_branch"}}</li>{{end}}
 								{{if and .CommitFormOptions.RequireSigned (not .CommitFormOptions.WillSign)}}<li>{{ctx.Locale.Tr "repo.editor.require_signed_commit"}}</li>{{end}}
 							</ul>

templates/repo/issue/view_content.tmpl

@@ -1,8 +1,8 @@
 <div class="issue-content">
 	{{$createdStr:= DateUtils.TimeSince .Issue.CreatedUnix}}
-	<div class="issue-content-left comment-list prevent-before-timeline">
+	<div class="issue-content-left">
-		<div class="ui timeline">
+		<div class="comment-list">
-			<div id="{{.Issue.HashTag}}" class="timeline-item comment first">
+			<div id="{{.Issue.HashTag}}" class="timeline-item comment issue-content-comment">
 				{{if .Issue.OriginalAuthor}}
 				<span class="timeline-avatar">
 					{{ctx.AvatarUtils.Avatar nil 40}}

templates/repo/issue/view_content/comments.tmpl

@@ -362,7 +362,7 @@
 		{{else if eq .Type 22}}
 			<div class="timeline-item-group" id="{{.HashTag}}">
 				<div class="timeline-item event">
-					{{$reviewType := -1}}
+					{{$reviewType := 2}}{{/* default to "comment" type if the review record is missing */}}
 					{{if .Review}}{{$reviewType = .Review.Type}}{{end}}
 					{{if not .OriginalAuthor}}
 					{{/* Some timeline avatars need a offset to correctly align with their speech bubble.
@@ -372,15 +372,17 @@
 						{{ctx.AvatarUtils.Avatar .Poster 40}}
 					</a>
 					{{end}}
-					<span class="badge tw-text-white{{if eq $reviewType 1}}{{if .Review.Official}} tw-bg-green {{else}} tw-bg-grey{{end}}{{else if eq $reviewType 3}} tw-bg-red{{end}}">
+					<span class="badge tw-text-white {{if eq $reviewType 1}}{{Iif .Review.Official "tw-bg-green" "tw-bg-grey"}}{{else if eq $reviewType 3}}tw-bg-red{{end}}">
-						{{if .Review}}{{svg (printf "octicon-%s" .Review.Type.Icon)}}{{end}}
+						{{- if .Review -}}
+							{{- svg (printf "octicon-%s" .Review.Type.Icon) -}}
+						{{- else -}}
+							{{- svg "octicon-comment" -}}
+						{{- end -}}
 					</span>
 					<span class="comment-text-line">
 						{{template "repo/issue/view_content/comments_authorlink" dict "ctxData" $ "comment" .}}
 						{{if eq $reviewType 1}}
 							{{ctx.Locale.Tr "repo.issues.review.approve" $createdStr}}
-						{{else if eq $reviewType 2}}
-							{{ctx.Locale.Tr "repo.issues.review.comment" $createdStr}}
 						{{else if eq $reviewType 3}}
 							{{ctx.Locale.Tr "repo.issues.review.reject" $createdStr}}
 						{{else}}

templates/repo/issue/view_content/pull_merge_box.tmpl

@@ -46,7 +46,7 @@
 			{{end}}
 			{{if $data.ShowUpdatePullInfo}}
 				<div class="item">
-					{{template "repo/issue/view_content/update_branch_by_merge" (dict "MergeBoxData" $data "Issue" $.Issue)}}
+					{{template "repo/issue/view_content/update_branch_by_merge" (dict "MergeBoxData" $data "IssueLink" $.Issue.Link)}}
 				</div>
 			{{end}}
 

templates/repo/issue/view_content/pull_merge_instruction.tmpl

@@ -1,9 +1,14 @@
 {{$data := $.MergeBoxData}}
 {{$pull := $.PullRequest}}
 <details>
-	<summary>{{ctx.Locale.Tr "repo.pulls.cmd_instruction_hint"}}</summary>
+	<summary class="tw-pl-1">{{/* align with other item icon & text */}}
-	<div class="tw-mt-2">
+		<span class="tw-pl-2">{{ctx.Locale.Tr "repo.pulls.cmd_instruction_hint"}}</span>
-		<div><h3>{{ctx.Locale.Tr "repo.pulls.cmd_instruction_checkout_title"}}</h3>{{ctx.Locale.Tr "repo.pulls.cmd_instruction_checkout_desc"}}</div>
+	</summary>
+	<div class="tw-my-[5px]">
+		<div>
+			<h3 class="tw-m-0">{{ctx.Locale.Tr "repo.pulls.cmd_instruction_checkout_title"}}</h3>
+			{{ctx.Locale.Tr "repo.pulls.cmd_instruction_checkout_desc"}}
+		</div>
 		{{$localBranch := $pull.HeadBranch}}
 		{{if ne $pull.HeadRepo.ID $pull.BaseRepo.ID}}
 			{{$localBranch = print $pull.HeadRepo.OwnerName "-" $pull.HeadBranch}}
@@ -19,7 +24,7 @@
 		</div>
 		{{if $data.ShowMergeInstructions}}
 		<div>
-			<h3>{{ctx.Locale.Tr "repo.pulls.cmd_instruction_merge_title"}}</h3>
+			<h3 class="tw-m-0">{{ctx.Locale.Tr "repo.pulls.cmd_instruction_merge_title"}}</h3>
 			{{ctx.Locale.Tr "repo.pulls.cmd_instruction_merge_desc"}}
 			{{if not $data.AutodetectManualMerge}}
 				<div>{{ctx.Locale.Tr "repo.pulls.cmd_instruction_merge_warning"}}</div>

templates/repo/issue/view_content/update_branch_by_merge.tmpl

@@ -1,33 +1,24 @@
 {{$data := $.MergeBoxData}}
-{{$issue := $.Issue}}
+{{$issueLink := $.IssueLink}}
 <div class="tw-w-full flex-left-right">
 	<div class="flex-text-block">
 		{{svg "octicon-alert"}}
 		{{ctx.Locale.Tr "repo.pulls.outdated_with_base_branch"}}
 	</div>
-	<div>
+	{{if $data.UpdateAllowed}}
-		{{if and $data.UpdateAllowed $data.UpdateByRebaseAllowed}}
+	<div class="ui buttons" data-global-init="initRepoPullRequestUpdate">
-			<div id="update-pr-branch-with-base" class="ui buttons">
+		<button class="ui button link-action" data-url="{{$issueLink}}/update">
-				<button class="ui button" data-do="{{$issue.Link}}/update">
+			{{ctx.Locale.Tr "repo.pulls.update_branch"}}
-					<span class="button-text">
+		</button>
-						{{ctx.Locale.Tr "repo.pulls.update_branch"}}
+		{{if $data.UpdateByRebaseAllowed}}
-					</span>
+		<div class="ui dropdown icon button">
-				</button>
+			{{svg "octicon-triangle-down"}}
-				<div class="ui dropdown icon button">
+			<div class="menu">
-					{{svg "octicon-triangle-down"}}
+				<a class="item selected" data-update-url="{{$issueLink}}/update">{{ctx.Locale.Tr "repo.pulls.update_branch"}}</a>
-					<div class="menu">
+				<a class="item" data-update-url="{{$issueLink}}/update?style=rebase">{{ctx.Locale.Tr "repo.pulls.update_branch_rebase"}}</a>
-						<a class="item active selected" data-do="{{$issue.Link}}/update">{{ctx.Locale.Tr "repo.pulls.update_branch"}}</a>
-						<a class="item" data-do="{{$issue.Link}}/update?style=rebase">{{ctx.Locale.Tr "repo.pulls.update_branch_rebase"}}</a>
-					</div>
-				</div>
 			</div>
-		{{end}}
+		</div>
-		{{if and $data.UpdateAllowed (not $data.UpdateByRebaseAllowed)}}
-			<form action="{{$issue.Link}}/update" method="post">
-				<button class="ui compact button">
-					<span class="ui text">{{ctx.Locale.Tr "repo.pulls.update_branch"}}</span>
-				</button>
-			</form>
 		{{end}}
 	</div>
+	{{end}}
 </div>

tests/e2e/reactions.test.ts

@@ -12,7 +12,7 @@ test('toggle issue reactions', async ({page, request}) => {
   ]);
   await page.goto(`/${owner}/${repoName}/issues/1`);
 
-  const issueComment = page.locator('.timeline-item.comment.first');
+  const issueComment = page.locator('.timeline-item.comment.issue-content-comment');
 
   const reactionPicker = issueComment.locator('.select-reaction');
   await reactionPicker.click();

tests/integration/actions_trigger_test.go

@@ -1801,7 +1801,7 @@ jobs:
 		testEditFile(t, session, "user2", repoName, repo.DefaultBranch, "dir1/dir1.txt", "11")
 		// update by rebase
 		req := NewRequest(t, "POST", fmt.Sprintf("/%s/%s/pulls/%d/update?style=rebase", "user2", repoName, apiPull.Index))
-		session.MakeRequest(t, req, http.StatusSeeOther)
+		session.MakeRequest(t, req, http.StatusOK)
 		runner.fetchNoTask(t)
 	})
 }

tests/integration/issue_test.go

@@ -692,9 +692,9 @@ func TestIssueReferenceURL(t *testing.T) {
 	htmlDoc := NewHTMLParser(t, resp.Body)
 
 	// the "reference" uses relative URLs, then JS code will convert them to absolute URLs for current origin, in case users are using multiple domains
-	ref, _ := htmlDoc.Find(`.timeline-item.comment.first .reference-issue`).Attr("data-reference")
+	ref, _ := htmlDoc.Find(`.timeline-item.comment.issue-content-comment .reference-issue`).Attr("data-reference")
 	assert.Equal(t, "/user2/repo1/issues/1#issue-1", ref)
 
-	ref, _ = htmlDoc.Find(`.timeline-item.comment:not(.first) .reference-issue`).Attr("data-reference")
+	ref, _ = htmlDoc.Find(`.timeline-item.comment:not(.issue-content-comment) .reference-issue`).Attr("data-reference")
 	assert.Equal(t, "/user2/repo1/issues/1#issuecomment-2", ref)
 }

web_src/css/modules/checkbox.css

@@ -105,6 +105,7 @@ input[type="checkbox"]:indeterminate::before {
   vertical-align: middle;
 }
 
+.ui.disabled.checkbox input,
 .ui.disabled.checkbox label,
 .ui.checkbox input[disabled] ~ label {
   cursor: default !important;

web_src/css/repo.css

@@ -423,28 +423,14 @@ td .commit-summary {
   margin-right: 5px;
 }
 
-.repository.view.issue .comment-list:not(.prevent-before-timeline)::before {
+.repository.view.issue .comment-list {
-  display: block;
-  content: "";
-  position: absolute;
-  margin-top: 12px;
-  margin-bottom: 14px;
-  top: 0;
-  bottom: 0;
-  left: 96px;
-  width: 2px;
-  background-color: var(--color-timeline);
-  z-index: -1;
-}
-
-.repository.view.issue .comment-list .timeline {
   position: relative;
   display: block;
   margin-left: 40px;
   padding-left: 16px;
 }
 
-.repository.view.issue .comment-list .timeline::before { /* ciara */
+.repository.view.issue .comment-list::before {
   display: block;
   content: "";
   position: absolute;
@@ -1318,6 +1304,7 @@ td .commit-summary {
   display: flex;
   align-items: center;
   gap: 6px;
+  flex-wrap: wrap;
 }
 
 .comment-header-right {
@@ -1844,26 +1831,23 @@ tbody.commit-list {
 }
 
 @media (max-width: 767.98px) {
-  .repository.view.issue .comment-list .timeline,
+  .repository.view.issue .comment-list,
   .repository.view.issue .comment-list .timeline-item {
     margin-left: 0;
   }
-  .repository.view.issue .comment-list .timeline::before {
+  .repository.view.issue .comment-list::before {
     left: 14px;
   }
-  .repository.view.issue .comment-list .timeline .inline-timeline-avatar {
+  .repository.view.issue .comment-list .inline-timeline-avatar {
     display: flex;
     margin-bottom: auto;
     margin-left: 6px;
     margin-right: 2px;
   }
-  .repository.view.issue .comment-list .timeline .comment-header {
-    padding-left: 4px;
-  }
   /* Don't show the general avatar, we show the inline avatar on mobile.
    * And don't show the role labels, there's no place for that. */
-  .repository.view.issue .comment-list .timeline .timeline-avatar,
+  .repository.view.issue .comment-list .timeline-avatar,
-  .repository.view.issue .comment-list .timeline .comment-header-right .role-label {
+  .repository.view.issue .comment-list .comment-header-right .role-label {
     display: none;
   }
   .commit-header h3 {

web_src/js/features/repo-issue-content.ts

@@ -94,7 +94,7 @@ function showContentHistoryDetail(issueBaseUrl: string, commentId: string, histo
 function showContentHistoryMenu(issueBaseUrl: string, elCommentItem: Element, commentId: string) {
   const elHeaderLeft = elCommentItem.querySelector('.comment-header-left')!;
   const menuHtml = `
-  <div class="ui dropdown interact-fg content-history-menu" data-comment-id="${commentId}">
+  <div class="ui dropdown interact-fg content-history-menu tw-flex-shrink-0" data-comment-id="${commentId}">
     &bull; ${i18nTextEdited}${svg('octicon-triangle-down', 14, 'dropdown icon')}
     <div class="menu">
     </div>
@@ -127,7 +127,7 @@ export async function initRepoIssueContentHistory() {
   const issuePageInfo = parseIssuePageInfo();
   if (!issuePageInfo.issueNumber) return;
 
-  const elIssueDescription = document.querySelector('.repository.issue .timeline-item.comment.first'); // issue(PR) main content
+  const elIssueDescription = document.querySelector('.repository.issue .timeline-item.comment.issue-content-comment'); // issue(PR) main content
   const elComments = document.querySelectorAll('.repository.issue .comment-list .comment'); // includes: issue(PR) comments, review comments, code comments
   if (!elIssueDescription && !elComments.length) return;
 

web_src/js/features/repo-issue-pull.ts

@@ -1,51 +1,19 @@
 import {createApp} from 'vue';
-import {GET, POST} from '../modules/fetch.ts';
+import {GET} from '../modules/fetch.ts';
 import {fomanticQuery} from '../modules/fomantic/base.ts';
 import {createElementFromHTML} from '../utils/dom.ts';
 import {registerGlobalEventFunc} from '../modules/observer.ts';
 
-function initRepoPullRequestUpdate(el: HTMLElement) {
+export function initRepoPullRequestUpdate(el: HTMLElement) {
-  const prUpdateButtonContainer = el.querySelector('#update-pr-branch-with-base');
+  const elDropdown = el.querySelector(':scope > .ui.dropdown');
-  if (!prUpdateButtonContainer) return;
+  if (!elDropdown) return;
+  const elButton = el.querySelector<HTMLButtonElement>(':scope > button')!;
 
-  const prUpdateButton = prUpdateButtonContainer.querySelector<HTMLButtonElement>(':scope > button')!;
+  fomanticQuery(elDropdown).dropdown({
-  const prUpdateDropdown = prUpdateButtonContainer.querySelector(':scope > .ui.dropdown')!;
-  prUpdateButton.addEventListener('click', async function (e) {
-    e.preventDefault();
-    this.classList.add('is-loading');
-    let response: Response | undefined;
-    try {
-      response = await POST(this.getAttribute('data-do')!);
-    } catch (error) {
-      console.error(error);
-    } finally {
-      this.classList.remove('is-loading');
-    }
-    let data: Record<string, any> | undefined;
-    try {
-      // TODO: the response is indeed not JSON, need to fix (see backend UpdatePullRequest)
-      data = await response?.json(); // the response is probably not a JSON
-    } catch (error) {
-      console.error(error);
-    }
-    if (data?.redirect) {
-      window.location.href = data.redirect;
-    } else {
-      window.location.reload();
-    }
-  });
-
-  fomanticQuery(prUpdateDropdown).dropdown({
     onChange(_text: string, _value: string, $choice: any) {
       const choiceEl = $choice[0];
-      const url = choiceEl.getAttribute('data-do');
+      elButton.textContent = choiceEl.textContent;
-      if (url) {
+      elButton.setAttribute('data-url', choiceEl.getAttribute('data-update-url'));
-        const buttonText = prUpdateButton.querySelector('.button-text');
-        if (buttonText) {
-          buttonText.textContent = choiceEl.textContent;
-        }
-        prUpdateButton.setAttribute('data-do', url);
-      }
     },
   });
 }
@@ -69,7 +37,6 @@ async function initRepoPullRequestMergeForm(box: HTMLElement) {
 
 export function initRepoPullMergeBox(el: HTMLElement) {
   registerGlobalEventFunc('click', 'onCommitStatusChecksToggle', onCommitStatusChecksToggle);
-  initRepoPullRequestUpdate(el);
   initRepoPullRequestMergeForm(el);
 
   const reloadingIntervalValue = el.getAttribute('data-pull-merge-box-reloading-interval');

web_src/js/features/repo-legacy.ts

@@ -17,7 +17,7 @@ import {initRepoMilestone} from './repo-milestone.ts';
 import {initRepoNew} from './repo-new.ts';
 import {createApp} from 'vue';
 import RepoBranchTagSelector from '../components/RepoBranchTagSelector.vue';
-import {initRepoPullMergeBox} from './repo-issue-pull.ts';
+import {initRepoPullMergeBox, initRepoPullRequestUpdate} from './repo-issue-pull.ts';
 
 function initRepoBranchTagSelector() {
   registerGlobalInitFunc('initRepoBranchTagSelector', async (elRoot: HTMLInputElement) => {
@@ -38,6 +38,9 @@ export function initBranchSelectorTabs() {
 }
 
 export function initRepository() {
+  registerGlobalInitFunc('initRepoPullMergeBox', initRepoPullMergeBox);
+  registerGlobalInitFunc('initRepoPullRequestUpdate', initRepoPullRequestUpdate);
+
   const pageContent = document.querySelector('.page-content.repository');
   if (!pageContent) return;
 
@@ -68,8 +71,6 @@ export function initRepository() {
     initRepoIssueCommentDelete();
     initRepoIssueCodeCommentCancel();
     initCompReactionSelector();
-
-    registerGlobalInitFunc('initRepoPullMergeBox', initRepoPullMergeBox);
   }
 
   initUnicodeEscapeButton();