From 6ba907d89cf1288878be1f3f1f4ca4ab665aa400 Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Tue, 5 May 2026 23:54:07 +0800 Subject: [PATCH] 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 --- options/locale/locale_en-US.json | 2 +- routers/web/repo/pull.go | 20 +++----- templates/repo/editor/commit_form.tmpl | 6 +-- templates/repo/issue/view_content.tmpl | 6 +-- .../repo/issue/view_content/comments.tmpl | 12 +++-- .../issue/view_content/pull_merge_box.tmpl | 2 +- .../view_content/pull_merge_instruction.tmpl | 13 +++-- .../view_content/update_branch_by_merge.tmpl | 37 ++++++-------- tests/e2e/reactions.test.ts | 2 +- tests/integration/actions_trigger_test.go | 2 +- tests/integration/issue_test.go | 4 +- web_src/css/modules/checkbox.css | 1 + web_src/css/repo.css | 32 +++--------- web_src/js/features/repo-issue-content.ts | 4 +- web_src/js/features/repo-issue-pull.ts | 49 +++---------------- web_src/js/features/repo-legacy.ts | 7 +-- 16 files changed, 73 insertions(+), 126 deletions(-) diff --git a/options/locale/locale_en-US.json b/options/locale/locale_en-US.json index 6f3141885b..47fcac7ae7 100644 --- a/options/locale/locale_en-US.json +++ b/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:", diff --git a/routers/web/repo/pull.go b/routers/web/repo/pull.go index eab4df77ad..d20bbdc36c 100644 --- a/routers/web/repo/pull.go +++ b/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.Redirect(issue.Link()) + ctx.JSONError(ctx.Tr("repo.pulls.update_not_allowed")) 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.Redirect(issue.Link()) + ctx.JSONError(flashError) 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.Redirect(issue.Link()) + ctx.JSONError(flashError) return } - ctx.Flash.Error(err.Error()) - ctx.Redirect(issue.Link()) + log.Error("Update pull request failed: %v", err) + 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 diff --git a/templates/repo/editor/commit_form.tmpl b/templates/repo/editor/commit_form.tmpl index 7fdd36b3bd..d0c47e9bdd 100644 --- a/templates/repo/editor/commit_form.tmpl +++ b/templates/repo/editor/commit_form.tmpl @@ -22,7 +22,7 @@ -
+
@@ -30,9 +30,9 @@ {{svg "octicon-git-commit"}} {{ctx.Locale.Tr "repo.editor.commit_directly_to_this_branch" .BranchName}} {{if not .CommitFormOptions.CanCommitToBranch}} -
+
{{ctx.Locale.Tr "repo.editor.no_commit_to_branch"}} -
    +
      {{if not .CommitFormOptions.UserCanPush}}
    • {{ctx.Locale.Tr "repo.editor.user_no_push_to_branch"}}
      • {{end}} {{if and .CommitFormOptions.RequireSigned (not .CommitFormOptions.WillSign)}}
    • {{ctx.Locale.Tr "repo.editor.require_signed_commit"}}
      • {{end}}
    diff --git a/templates/repo/issue/view_content.tmpl b/templates/repo/issue/view_content.tmpl index 275dd47a76..d1b327434b 100644 --- a/templates/repo/issue/view_content.tmpl +++ b/templates/repo/issue/view_content.tmpl @@ -1,8 +1,8 @@
    {{$createdStr:= DateUtils.TimeSince .Issue.CreatedUnix}} -
    -
    -
    +
    +
    +
    {{if .Issue.OriginalAuthor}} {{ctx.AvatarUtils.Avatar nil 40}} diff --git a/templates/repo/issue/view_content/comments.tmpl b/templates/repo/issue/view_content/comments.tmpl index 6a67ef754d..ad26d4da88 100644 --- a/templates/repo/issue/view_content/comments.tmpl +++ b/templates/repo/issue/view_content/comments.tmpl @@ -362,7 +362,7 @@ {{else if eq .Type 22}}
    - {{$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}} {{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 -}} {{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}} diff --git a/templates/repo/issue/view_content/pull_merge_box.tmpl b/templates/repo/issue/view_content/pull_merge_box.tmpl index 9f6330305a..d1594a52dd 100644 --- a/templates/repo/issue/view_content/pull_merge_box.tmpl +++ b/templates/repo/issue/view_content/pull_merge_box.tmpl @@ -46,7 +46,7 @@ {{end}} {{if $data.ShowUpdatePullInfo}}
    - {{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)}}
    {{end}} diff --git a/templates/repo/issue/view_content/pull_merge_instruction.tmpl b/templates/repo/issue/view_content/pull_merge_instruction.tmpl index 0f24b1b3ea..b3a36f01d6 100644 --- a/templates/repo/issue/view_content/pull_merge_instruction.tmpl +++ b/templates/repo/issue/view_content/pull_merge_instruction.tmpl @@ -1,9 +1,14 @@ {{$data := $.MergeBoxData}} {{$pull := $.PullRequest}}
    - {{ctx.Locale.Tr "repo.pulls.cmd_instruction_hint"}} -
    -

    {{ctx.Locale.Tr "repo.pulls.cmd_instruction_checkout_title"}}

    {{ctx.Locale.Tr "repo.pulls.cmd_instruction_checkout_desc"}}
    + {{/* align with other item icon & text */}} + {{ctx.Locale.Tr "repo.pulls.cmd_instruction_hint"}} + +
    +
    +

    {{ctx.Locale.Tr "repo.pulls.cmd_instruction_checkout_title"}}

    + {{ctx.Locale.Tr "repo.pulls.cmd_instruction_checkout_desc"}} +
    {{$localBranch := $pull.HeadBranch}} {{if ne $pull.HeadRepo.ID $pull.BaseRepo.ID}} {{$localBranch = print $pull.HeadRepo.OwnerName "-" $pull.HeadBranch}} @@ -19,7 +24,7 @@
    {{if $data.ShowMergeInstructions}}
    -

    {{ctx.Locale.Tr "repo.pulls.cmd_instruction_merge_title"}}

    +

    {{ctx.Locale.Tr "repo.pulls.cmd_instruction_merge_title"}}

    {{ctx.Locale.Tr "repo.pulls.cmd_instruction_merge_desc"}} {{if not $data.AutodetectManualMerge}}
    {{ctx.Locale.Tr "repo.pulls.cmd_instruction_merge_warning"}}
    diff --git a/templates/repo/issue/view_content/update_branch_by_merge.tmpl b/templates/repo/issue/view_content/update_branch_by_merge.tmpl index 0af10045d8..fcdf65b74d 100644 --- a/templates/repo/issue/view_content/update_branch_by_merge.tmpl +++ b/templates/repo/issue/view_content/update_branch_by_merge.tmpl @@ -1,33 +1,24 @@ {{$data := $.MergeBoxData}} -{{$issue := $.Issue}} +{{$issueLink := $.IssueLink}}
    {{svg "octicon-alert"}} {{ctx.Locale.Tr "repo.pulls.outdated_with_base_branch"}}
    -
    - {{if and $data.UpdateAllowed $data.UpdateByRebaseAllowed}} -
    - -
    - {{svg "octicon-triangle-down"}} -
    - {{ctx.Locale.Tr "repo.pulls.update_branch"}} - {{ctx.Locale.Tr "repo.pulls.update_branch_rebase"}} -
    -
    + {{if $data.UpdateAllowed}} +
    + + {{if $data.UpdateByRebaseAllowed}} +
    + {{svg "octicon-triangle-down"}} +
    + {{ctx.Locale.Tr "repo.pulls.update_branch"}} + {{ctx.Locale.Tr "repo.pulls.update_branch_rebase"}}
    - {{end}} - {{if and $data.UpdateAllowed (not $data.UpdateByRebaseAllowed)}} -
    - -
    +
    {{end}}
    + {{end}}
    diff --git a/tests/e2e/reactions.test.ts b/tests/e2e/reactions.test.ts index 603cae8298..2048b938cf 100644 --- a/tests/e2e/reactions.test.ts +++ b/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(); diff --git a/tests/integration/actions_trigger_test.go b/tests/integration/actions_trigger_test.go index 4d88d42f36..e7a3cd7b4f 100644 --- a/tests/integration/actions_trigger_test.go +++ b/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) }) } diff --git a/tests/integration/issue_test.go b/tests/integration/issue_test.go index 09475bad20..ba15f8962e 100644 --- a/tests/integration/issue_test.go +++ b/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) } diff --git a/web_src/css/modules/checkbox.css b/web_src/css/modules/checkbox.css index f24b91df07..a6caf3324f 100644 --- a/web_src/css/modules/checkbox.css +++ b/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; diff --git a/web_src/css/repo.css b/web_src/css/repo.css index fb85dc8e9f..8a581750ee 100644 --- a/web_src/css/repo.css +++ b/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 { - 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 { +.repository.view.issue .comment-list { 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 .comment-header-right .role-label { + .repository.view.issue .comment-list .timeline-avatar, + .repository.view.issue .comment-list .comment-header-right .role-label { display: none; } .commit-header h3 { diff --git a/web_src/js/features/repo-issue-content.ts b/web_src/js/features/repo-issue-content.ts index 865d4305e4..e385d5b1e8 100644 --- a/web_src/js/features/repo-issue-content.ts +++ b/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 = ` -
    +
    • ${i18nTextEdited}${svg('octicon-triangle-down', 14, 'dropdown icon')}
    @@ -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; diff --git a/web_src/js/features/repo-issue-pull.ts b/web_src/js/features/repo-issue-pull.ts index d16e865c70..7a72d267fc 100644 --- a/web_src/js/features/repo-issue-pull.ts +++ b/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) { - const prUpdateButtonContainer = el.querySelector('#update-pr-branch-with-base'); - if (!prUpdateButtonContainer) return; +export function initRepoPullRequestUpdate(el: HTMLElement) { + const elDropdown = el.querySelector(':scope > .ui.dropdown'); + if (!elDropdown) return; + const elButton = el.querySelector(':scope > button')!; - const prUpdateButton = prUpdateButtonContainer.querySelector(':scope > button')!; - 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 | 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({ + fomanticQuery(elDropdown).dropdown({ onChange(_text: string, _value: string, $choice: any) { const choiceEl = $choice[0]; - const url = choiceEl.getAttribute('data-do'); - if (url) { - const buttonText = prUpdateButton.querySelector('.button-text'); - if (buttonText) { - buttonText.textContent = choiceEl.textContent; - } - prUpdateButton.setAttribute('data-do', url); - } + elButton.textContent = choiceEl.textContent; + elButton.setAttribute('data-url', choiceEl.getAttribute('data-update-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'); diff --git a/web_src/js/features/repo-legacy.ts b/web_src/js/features/repo-legacy.ts index fc92242b40..db91529535 100644 --- a/web_src/js/features/repo-legacy.ts +++ b/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();