From e90753a7120d116829fe71b03b074e8bfedce5d9 Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Thu, 21 Nov 2024 22:09:16 +0800 Subject: [PATCH] Fix PR diff review form submit (#32596) Fix #31622, there is a longstanding bug in #19612, it doesn't handle submit event, correctly. --- web_src/js/features/repo-diff.ts | 79 ++++++++++++++++--------------- web_src/js/features/repo-issue.ts | 48 ++++++++----------- web_src/js/utils/dom.test.ts | 1 + web_src/js/utils/dom.ts | 21 ++++++-- 4 files changed, 79 insertions(+), 70 deletions(-) diff --git a/web_src/js/features/repo-diff.ts b/web_src/js/features/repo-diff.ts index a0bd9955fe468..0d489665a2307 100644 --- a/web_src/js/features/repo-diff.ts +++ b/web_src/js/features/repo-diff.ts @@ -7,30 +7,20 @@ import {validateTextareaNonEmpty} from './comp/ComboMarkdownEditor.ts'; import {initViewedCheckboxListenerFor, countAndUpdateViewedFiles, initExpandAndCollapseFilesButton} from './pull-view-file.ts'; import {initImageDiff} from './imagediff.ts'; import {showErrorToast} from '../modules/toast.ts'; -import {submitEventSubmitter, queryElemSiblings, hideElem, showElem, animateOnce} from '../utils/dom.ts'; +import { + submitEventSubmitter, + queryElemSiblings, + hideElem, + showElem, + animateOnce, + addDelegatedEventListener, + createElementFromHTML, +} from '../utils/dom.ts'; import {POST, GET} from '../modules/fetch.ts'; +import {fomanticQuery} from '../modules/fomantic/base.ts'; const {pageData, i18n} = window.config; -function initRepoDiffReviewButton() { - const reviewBox = document.querySelector('#review-box'); - if (!reviewBox) return; - - const counter = reviewBox.querySelector('.review-comments-counter'); - if (!counter) return; - - $(document).on('click', 'button[name="pending_review"]', (e) => { - const $form = $(e.target).closest('form'); - // Watch for the form's submit event. - $form.on('submit', () => { - const num = parseInt(counter.getAttribute('data-pending-comment-number')) + 1 || 1; - counter.setAttribute('data-pending-comment-number', num); - counter.textContent = num; - animateOnce(reviewBox, 'pulse-1p5-200'); - }); - }); -} - function initRepoDiffFileViewToggle() { $('.file-view-toggle').on('click', function () { for (const el of queryElemSiblings(this)) { @@ -47,19 +37,15 @@ function initRepoDiffFileViewToggle() { } function initRepoDiffConversationForm() { - $(document).on('submit', '.conversation-holder form', async (e) => { + addDelegatedEventListener(document, 'submit', '.conversation-holder form', async (form, e) => { e.preventDefault(); + const textArea = form.querySelector('textarea'); + if (!validateTextareaNonEmpty(textArea)) return; + if (form.classList.contains('is-loading')) return; - const $form = $(e.target); - const textArea = e.target.querySelector('textarea'); - if (!validateTextareaNonEmpty(textArea)) { - return; - } - - if (e.target.classList.contains('is-loading')) return; try { - e.target.classList.add('is-loading'); - const formData = new FormData($form[0]); + form.classList.add('is-loading'); + const formData = new FormData(form); // if the form is submitted by a button, append the button's name and value to the form data const submitter = submitEventSubmitter(e); @@ -68,26 +54,42 @@ function initRepoDiffConversationForm() { formData.append(submitter.name, submitter.value); } - const response = await POST(e.target.getAttribute('action'), {data: formData}); - const $newConversationHolder = $(await response.text()); - const {path, side, idx} = $newConversationHolder.data(); + const trLineType = form.closest('tr').getAttribute('data-line-type'); + const response = await POST(form.getAttribute('action'), {data: formData}); + const newConversationHolder = createElementFromHTML(await response.text()); + const path = newConversationHolder.getAttribute('data-path'); + const side = newConversationHolder.getAttribute('data-side'); + const idx = newConversationHolder.getAttribute('data-idx'); + + form.closest('.conversation-holder').replaceWith(newConversationHolder); + form = null; // prevent further usage of the form because it should have been replaced - $form.closest('.conversation-holder').replaceWith($newConversationHolder); let selector; - if ($form.closest('tr').data('line-type') === 'same') { + if (trLineType === 'same') { selector = `[data-path="${path}"] .add-code-comment[data-idx="${idx}"]`; } else { selector = `[data-path="${path}"] .add-code-comment[data-side="${side}"][data-idx="${idx}"]`; } for (const el of document.querySelectorAll(selector)) { - el.classList.add('tw-invisible'); + el.classList.add('tw-invisible'); // TODO need to figure out why + } + fomanticQuery(newConversationHolder.querySelectorAll('.ui.dropdown')).dropdown(); + + // the default behavior is to add a pending review, so if no submitter, it also means "pending_review" + if (!submitter || submitter?.matches('button[name="pending_review"]')) { + const reviewBox = document.querySelector('#review-box'); + const counter = reviewBox?.querySelector('.review-comments-counter'); + if (!counter) return; + const num = parseInt(counter.getAttribute('data-pending-comment-number')) + 1 || 1; + counter.setAttribute('data-pending-comment-number', String(num)); + counter.textContent = String(num); + animateOnce(reviewBox, 'pulse-1p5-200'); } - $newConversationHolder.find('.dropdown').dropdown(); } catch (error) { console.error('Error:', error); showErrorToast(i18n.network_error); } finally { - e.target.classList.remove('is-loading'); + form?.classList.remove('is-loading'); } }); @@ -219,7 +221,6 @@ export function initRepoDiffView() { initDiffFileList(); initDiffCommitSelect(); initRepoDiffShowMore(); - initRepoDiffReviewButton(); initRepoDiffFileViewToggle(); initViewedCheckboxListenerFor(); initExpandAndCollapseFilesButton(); diff --git a/web_src/js/features/repo-issue.ts b/web_src/js/features/repo-issue.ts index 7457531ece47d..9cc478712bcee 100644 --- a/web_src/js/features/repo-issue.ts +++ b/web_src/js/features/repo-issue.ts @@ -1,7 +1,7 @@ import $ from 'jquery'; import {htmlEscape} from 'escape-goat'; import {createTippy, showTemporaryTooltip} from '../modules/tippy.ts'; -import {hideElem, showElem, toggleElem} from '../utils/dom.ts'; +import {addDelegatedEventListener, createElementFromHTML, hideElem, showElem, toggleElem} from '../utils/dom.ts'; import {setFileFolding} from './file-fold.ts'; import {ComboMarkdownEditor, getComboMarkdownEditor, initComboMarkdownEditor} from './comp/ComboMarkdownEditor.ts'; import {parseIssuePageInfo, toAbsoluteUrl} from '../utils.ts'; @@ -443,21 +443,19 @@ export function initRepoPullRequestReview() { }); } - $(document).on('click', '.add-code-comment', async function (e) { - if (e.target.classList.contains('btn-add-single')) return; // https://github.com/go-gitea/gitea/issues/4745 + addDelegatedEventListener(document, 'click', '.add-code-comment', async (el, e) => { e.preventDefault(); - const isSplit = this.closest('.code-diff')?.classList.contains('code-diff-split'); - const side = this.getAttribute('data-side'); - const idx = this.getAttribute('data-idx'); - const path = this.closest('[data-path]')?.getAttribute('data-path'); - const tr = this.closest('tr'); + const isSplit = el.closest('.code-diff')?.classList.contains('code-diff-split'); + const side = el.getAttribute('data-side'); + const idx = el.getAttribute('data-idx'); + const path = el.closest('[data-path]')?.getAttribute('data-path'); + const tr = el.closest('tr'); const lineType = tr.getAttribute('data-line-type'); - const ntr = tr.nextElementSibling; - let $ntr = $(ntr); + let ntr = tr.nextElementSibling; if (!ntr?.classList.contains('add-comment')) { - $ntr = $(` + ntr = createElementFromHTML(` ${isSplit ? ` @@ -466,24 +464,18 @@ export function initRepoPullRequestReview() { `} `); - $(tr).after($ntr); + tr.after(ntr); } - - const $td = $ntr.find(`.add-comment-${side}`); - const $commentCloud = $td.find('.comment-code-cloud'); - if (!$commentCloud.length && !$ntr.find('button[name="pending_review"]').length) { - try { - const response = await GET(this.closest('[data-new-comment-url]')?.getAttribute('data-new-comment-url')); - const html = await response.text(); - $td.html(html); - $td.find("input[name='line']").val(idx); - $td.find("input[name='side']").val(side === 'left' ? 'previous' : 'proposed'); - $td.find("input[name='path']").val(path); - const editor = await initComboMarkdownEditor($td[0].querySelector('.combo-markdown-editor')); - editor.focus(); - } catch (error) { - console.error(error); - } + const td = ntr.querySelector(`.add-comment-${side}`); + const commentCloud = td.querySelector('.comment-code-cloud'); + if (!commentCloud && !ntr.querySelector('button[name="pending_review"]')) { + const response = await GET(el.closest('[data-new-comment-url]')?.getAttribute('data-new-comment-url')); + td.innerHTML = await response.text(); + td.querySelector("input[name='line']").value = idx; + td.querySelector("input[name='side']").value = (side === 'left' ? 'previous' : 'proposed'); + td.querySelector("input[name='path']").value = path; + const editor = await initComboMarkdownEditor(td.querySelector('.combo-markdown-editor')); + editor.focus(); } }); } diff --git a/web_src/js/utils/dom.test.ts b/web_src/js/utils/dom.test.ts index d7e3a4939e23b..cb99a85511b06 100644 --- a/web_src/js/utils/dom.test.ts +++ b/web_src/js/utils/dom.test.ts @@ -2,6 +2,7 @@ import {createElementFromAttrs, createElementFromHTML, querySingleVisibleElem} f test('createElementFromHTML', () => { expect(createElementFromHTML('foobar').outerHTML).toEqual('foobar'); + expect(createElementFromHTML('foo').outerHTML).toEqual('foo'); }); test('createElementFromAttrs', () => { diff --git a/web_src/js/utils/dom.ts b/web_src/js/utils/dom.ts index 29b34dd1e3f42..4bbb0c414aca0 100644 --- a/web_src/js/utils/dom.ts +++ b/web_src/js/utils/dom.ts @@ -301,10 +301,17 @@ export function replaceTextareaSelection(textarea: HTMLTextAreaElement, text: st } // Warning: Do not enter any unsanitized variables here -export function createElementFromHTML(htmlString: string): HTMLElement { +export function createElementFromHTML(htmlString: string): T { + htmlString = htmlString.trim(); + // some tags like "tr" are special, it must use a correct parent container to create + if (htmlString.startsWith('('tr'); + } const div = document.createElement('div'); - div.innerHTML = htmlString.trim(); - return div.firstChild as HTMLElement; + div.innerHTML = htmlString; + return div.firstChild as T; } export function createElementFromAttrs(tagName: string, attrs: Record, ...children: (Node|string)[]): HTMLElement { @@ -340,3 +347,11 @@ export function querySingleVisibleElem(parent: Element, s if (candidates.length > 1) throw new Error(`Expected exactly one visible element matching selector "${selector}", but found ${candidates.length}`); return candidates.length ? candidates[0] as T : null; } + +export function addDelegatedEventListener(parent: Node, type: string, selector: string, listener: (elem: T, e: Event) => void | Promise, options?: boolean | AddEventListenerOptions) { + parent.addEventListener(type, (e: Event) => { + const elem = (e.target as HTMLElement).closest(selector); + if (!elem) return; + listener(elem as T, e); + }, options); +}