"Start review" with an empty comment breaks GUI

[cvennel]

• Gitea version (or commit ref): Verified on 1.14.2 and 1.15.0+dev-377-g894742278
• Git version: 2.24.1
• Operating system: Centos
• Database (use [x]):
  • PostgreSQL
  • MySQL
  • MSSQL
  • SQLite
• Can you reproduce the bug at https://try.gitea.io:
  • Yes (provide example URL): https://try.gitea.io/cvennel/Test_gui_bug/pulls/1/files
  • No
• Log gist:

Description

Clicking "Start review" with an empty comment seems like it should refresh the screen and display an error message, however the new screen is being shown in a smaller window where the new review comment should be. See screen capture below:

Screenshots

1.-.Update.README.md.-.Test_gui_bug.-.Gitea_.Git.with.a.cup.of.tea.-.Google.Chrome.2021-06-02.08-49-49.mp4

[lunny]

Hm, should we allow empty comment when click start review?

[cvennel]

Hm, should we allow empty comment when click start review?

Not sure if you're asking for an outside opinion or not but personally I don't see the value of being able to start a review with an empty comment. I would think ideally the start review button and likely also the single comment button would be disabled until there is content in the comment box. This would be similar to how the new pull request button is grayed out when not signed in.

Alternatively, I think the existing refresh and show the error message at the top would be more acceptable if it actually refreshed the entire page. The down-side to this is if you are reviewing a large PR and accidentally click the start button it's annoying to have to scroll back to where you were last reviewing.

[jannaahs]

I would like to work on it. Actually, I would implement it as @cvennel suggested.

[jannaahs]

My first idea was to use a vue component and wrap it around the editor and the button. Unfortunately, the vue component is not rendered, because the comment_form.tmpl will be loaded after vue was initialised.

Does anyone has a different idea?
Otherwise there is a lot more to do like a refactoring of how the html is loaded.

[cvennel]

@jannaahs I haven't looked too far into this, but it seems like right around here the javascript handles graying out the commit button on file edits if the commit message is empty or there are no file changes. Could that sort of thing be expanded for this situation or does that fall under where you were saying "there's a lot more to do."

[jannaahs]

@cvennel Yeah, I think it is not that easy.

While doing a code review you might have open more then one form. Committing files is a bit different, because there is only one form open at the same time.

In case you have two forms open, the buttons should only be disabled if the corresponding input is empty. Currently there is no mechanism in place to have the editor and the buttons in their own scope.

Implementing a own scope for each form is what I meant by "there's a lot more to do". At the moment I am not sure what would be a good solution to add a scope. Like I said a vue component would provide a scope, but is not possible due to the template being loaded after vue was initilized.

Maybe I didn't understood the initEditor you mentioned, but I don't see a quick and easy solution.

[Gusted]

Same issue as #18151, fixed by #18156