Fix repo diff review comment counter increment behaviour #31641
Conversation
GiteaBot
added
the
lgtm/need 2
pull-request-size
bot
added
the
size/M
|
New PRs shouldn't use jQuery https://docs.gitea.com/contributing/guidelines-frontend#framework-usage |
henrygoodman
force-pushed
the
fix-repo-diff-review-comment-counter
branch
from
cdb99cc to
3bbb210
Compare
| document.addEventListener('keydown', (e) => { | ||
| if (e.ctrlKey && e.key === 'Enter') { | ||
| const textarea = e.target; | ||
| if (textarea.tagName.toLowerCase() === 'textarea') { |
There was a problem hiding this comment.
Also, why is web_src/js/features/common_form.ts not dispatching this event already?
Fixing the reason why it isn't handled sounds like a better idea to me than duplicating the Ctrl+Enter behavior here manually.
|
|
||
| // Handle submit on click | ||
| document.addEventListener('click', (e) => { | ||
| if (e.target.name === 'pending_review') { |
There was a problem hiding this comment.
This condition seems wrong to me:
Isn't the target name button here?
Shouldn't it be hasAttribute?
There was a problem hiding this comment.
Either way, it's probably better to use
| if (e.target.name === 'pending_review') { | |
| if (e.target.matches('[name="pending_review"]') { |
| // Watch for the form's submit event. | ||
| $form.on('submit', () => { | ||
| const num = parseInt(counter.getAttribute('data-pending-comment-number')) + 1 || 1; | ||
| function handleFormSubmit(form, textarea) { |
There was a problem hiding this comment.
This architecture is a bit weird:
Why remove and re-add the handler all the time?
Typically, our behavior would rather be "initialize once on page load, and then keep it".
|
Checked if reproducible, issue seems to be fixed now (I guess externally as delvh mentioned, ctrl-Enter handling likely was fixed elsewhere). Closing EDIT: Thought it was fixed when checking on |
|
The bug is still there and needs to be fixed. Will propose a fix. |
|
-> Fix PR diff review form submit #32596 |
Thanks @wxiaoguang, yes bug was actually still present, just checked. |
Fixes #31622
Check above issue for specific behavioural issues this PR will fix
Issue 1
Currently, initializing the review comment form adds the submit handler each time the button is clicked, which means the button can end up with many handlers. When this happens, finally submitting (i.e. when the textarea is non-empty) will call all submit handler instances, incrementing the review comment counter by a lot. This is all UI so reloading the page will get the correct value from database.
Fix 1
Use a flag when attaching the handler, only attach if the flag is unset.
Issue 2
Additionally, the form is currently submitted on the button click, without validating the form on frontend (i.e. we manually call the form submit within the click handler, but the HTML will not allow the form to submit if the textarea is empty).
I'd like to point out that if we can disable the button whilst the textarea is empty, we don't have to do any validation here (as the button cannot be clicked anyway until the form can officially be submitted).
Fix 2
Prevent default behaviour in this handler if the textarea is empty.
Issue 3
We were not handling form submission on CTRL+Enter, only on button click.
Fix 3
Added explicit handler for keydown event to check CTRL+Enter and submit form to increment the counter.