Fix PR diff review form submit (#32596)

Fix #31622, there is a longstanding bug in #19612, 
it doesn't handle submit event, correctly.

Author: wxiaoguang

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<HTMLFormElement>(document, 'submit', '.conversation-holder form', async (form, e) => {
     e.preventDefault();
+    const textArea = form.querySelector<HTMLTextAreaElement>('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();

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(`
         <tr class="add-comment" data-line-type="${lineType}">
           ${isSplit ? `
             <td class="add-comment-left" colspan="4"></td>
@@ -466,24 +464,18 @@ export function initRepoPullRequestReview() {
             <td class="add-comment-left add-comment-right" colspan="5"></td>
           `}
         </tr>`);
-      $(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<HTMLInputElement>("input[name='line']").value = idx;
+      td.querySelector<HTMLInputElement>("input[name='side']").value = (side === 'left' ? 'previous' : 'proposed');
+      td.querySelector<HTMLInputElement>("input[name='path']").value = path;
+      const editor = await initComboMarkdownEditor(td.querySelector<HTMLElement>('.combo-markdown-editor'));
+      editor.focus();
     }
   });
 }

web_src/js/utils/dom.test.ts

@@ -2,6 +2,7 @@ import {createElementFromAttrs, createElementFromHTML, querySingleVisibleElem} f
 
 test('createElementFromHTML', () => {
   expect(createElementFromHTML('<a>foo<span>bar</span></a>').outerHTML).toEqual('<a>foo<span>bar</span></a>');
+  expect(createElementFromHTML('<tr data-x="1"><td>foo</td></tr>').outerHTML).toEqual('<tr data-x="1"><td>foo</td></tr>');
 });
 
 test('createElementFromAttrs', () => {

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<T extends HTMLElement>(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 container = document.createElement('table');
+    container.innerHTML = htmlString;
+    return container.querySelector<T>('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<string, any>, ...children: (Node|string)[]): HTMLElement {
@@ -340,3 +347,11 @@ export function querySingleVisibleElem<T extends HTMLElement>(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<T extends HTMLElement>(parent: Node, type: string, selector: string, listener: (elem: T, e: Event) => void | Promise<any>, 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);
+}