Added finish review dropdown

What does this MR do and why?

Adds a finish review dropdown to the merge request reviews bar that allows users to add a comment when submitting a review.

Screenshots or screen recordings

How to set up and validate locally

Enable mr_review_submit_comment

MR acceptance checklist

This checklist encourages us to confirm any changes have been analyzed to reduce risks in quality, performance, reliability, security, and maintainability.

• I have evaluated the MR acceptance checklist for this MR.

Closes #244044 (closed)

changed milestone to %15.1

added Category:Code Review Workflow Deliverable Technical Writing customer devopscreate documentation featureenhancement frontend groupcode review merge requests sectiondev typefeature workflowin dev labels

assigned to @iamphill

Suggested Reviewers (beta)

The individuals below may be good candidates to participate in the review based on various factors.

You can use slash commands in comments to quickly assign /assign_reviewer @user1.

Suggested Reviewers
@jivanvl, @kushalpandya, @psimyn, @mlapierre, @mikegreiling

If you do not believe these suggestions are useful, please apply the label Bad Suggested Reviewer. You can also provide feedback for this feature on this issue: https://gitlab.com/gitlab-org/gitlab/-/issues/357923.

Automatically generated by Suggested Reviewers Bot - an experimental ML-based recommendation engine created by ~"group::applied ml".

removed workflowin dev label

	2 Warnings
	This merge request is quite big (607 lines changed), please consider splitting it into multiple merge requests.
	featureaddition and featureenhancement merge requests normally have a documentation change. Consider adding a documentation update or confirming the documentation plan with the Technical Writer counterpart. For more information, see: The Handbook page on merge request types. The definition of done documentation.

	1 Message
	CHANGELOG missing: If you want to create a changelog entry for GitLab FOSS, add the Changelog trailer to the commit message you want to add to the changelog. If you want to create a changelog entry for GitLab EE, also add the EE: true trailer to your commit message. If this merge request doesn't need a CHANGELOG entry, feel free to ignore this message.

If needed, you can retry the danger-review job that generated this comment.

Reviewer roulette

Changes that require review have been detected!

Please refer to the table below for assigning reviewers and maintainers suggested by Danger in the specified category:

Category	Reviewer	Maintainer
backend	George Koltsov (@georgekoltsov) (UTC+1, same timezone as @iamphill)	Sean Arnold (@seanarnold) (UTC+12, 11 hours ahead of @iamphill)
frontend	Mireya Andres (@mgandres) (UTC+8, 7 hours ahead of @iamphill)	Michael Lunøe (@mlunoe) (UTC+2, 1 hour ahead of @iamphill)
test Quality for spec/features/*	John McDonnell (@john.mcdonnell) (UTC+12, 11 hours ahead of @iamphill)	Maintainer review is optional for test Quality for spec/features/*

To spread load more evenly across eligible reviewers, Danger has picked a candidate for each review slot, based on their timezone. Feel free to override these selections if you think someone else would be better-suited or use the GitLab Review Workload Dashboard to find other available reviewers.

To read more on how to use the reviewer roulette, please take a look at the Engineering workflow and code review guidelines. Please consider assigning a reviewer or maintainer who is a domain expert in the area of the merge request.

Once you've decided who will review this merge request, assign them as a reviewer! Danger does not automatically notify them for you.

Generated by Danger

Bundle size analysis [beta]

This compares changes in bundle size for entry points between the commits daa997b4 and ddf3a6b8

Special assets

Entrypoint / Name	Size before	Size after	Diff	Diff in percent
mainChunk	1.93 MB	1.93 MB	+6.22 KB	0.3 %
average	3.46 MB	3.46 MB	+4.48 KB	0.1 %

Significant Reduction: 5

Expand

Entrypoint / Name	Size before	Size after	Diff	Diff in percent
pages.admin.groups.new	161.66 KB	24.17 KB	-137.5 KB	-85.1 %
pages.admin.groups.edit	210.29 KB	73.19 KB	-137.1 KB	-65.2 %
pages.groups.new	216.9 KB	140.5 KB	-76.39 KB	-35.2 %
pages.dashboard.groups.index	271.79 KB	261.73 KB	-10.06 KB	-3.7 %
pages.explore.groups	273.63 KB	263.56 KB	-10.06 KB	-3.7 %

---

Note: We do not have exact data for daa997b4. So we have used data from: db83a152.
The target commit was too new, so we used the latest commit from master we have info on.
It might help to rerun the bundle-size-review job
This might mean that you have a few false positives in this report. If something unrelated to your code changes is reported, you can check this comparison in order to see if they caused this change.

Please look at the full report for more details

---

Read more about how this report works.

Generated by Danger

Allure report

allure-report-publisher generated test report!

review-qa-blocking: test report for ddf3a6b8

expand test summary

+---------------------------------------------------------------------------+
|                              suites summary                               |
+----------------------+--------+--------+---------+-------+-------+--------+
|                      | passed | failed | skipped | flaky | total | result |
+----------------------+--------+--------+---------+-------+-------+--------+
| Package              | 0      | 0      | 1       | 0     | 1     | ➖     |
| Plan                 | 48     | 0      | 1       | 48    | 49    | ❗     |
| Verify               | 12     | 0      | 1       | 12    | 13    | ❗     |
| Protect              | 2      | 0      | 0       | 2     | 2     | ❗     |
| Manage               | 36     | 0      | 2       | 37    | 38    | ❗     |
| Create               | 23     | 0      | 2       | 22    | 25    | ❗     |
| Secure               | 6      | 0      | 0       | 6     | 6     | ❗     |
| Configure            | 0      | 0      | 1       | 0     | 1     | ➖     |
| Version sanity check | 0      | 0      | 1       | 0     | 1     | ➖     |
+----------------------+--------+--------+---------+-------+-------+--------+
| Total                | 127    | 0      | 9       | 127   | 136   | ❗     |
+----------------------+--------+--------+---------+-------+-------+--------+

package-and-qa-ff-disabled: test report for ddf3a6b8

expand test summary

+---------------------------------------------------------------------------+
|                              suites summary                               |
+----------------------+--------+--------+---------+-------+-------+--------+
|                      | passed | failed | skipped | flaky | total | result |
+----------------------+--------+--------+---------+-------+-------+--------+
| Verify               | 120    | 0      | 9       | 0     | 129   | ✅     |
| Create               | 441    | 0      | 24      | 8     | 465   | ❗     |
| Fulfillment          | 6      | 0      | 33      | 0     | 39    | ✅     |
| Manage               | 303    | 0      | 12      | 1     | 315   | ❗     |
| Plan                 | 159    | 0      | 0       | 0     | 159   | ✅     |
| Secure               | 60     | 0      | 6       | 3     | 66    | ❗     |
| Configure            | 0      | 0      | 9       | 0     | 9     | ➖     |
| Release              | 18     | 0      | 0       | 0     | 18    | ✅     |
| Version sanity check | 0      | 0      | 3       | 1     | 3     | ➖     |
| Package              | 0      | 0      | 9       | 0     | 9     | ➖     |
| Product Intelligence | 6      | 0      | 0       | 0     | 6     | ✅     |
| Protect              | 6      | 0      | 0       | 0     | 6     | ✅     |
+----------------------+--------+--------+---------+-------+-------+--------+
| Total                | 1119   | 0      | 105     | 13    | 1224  | ❗     |
+----------------------+--------+--------+---------+-------+-------+--------+

package-and-qa-ff-enabled: test report for ddf3a6b8

expand test summary

+---------------------------------------------------------------------------+
|                              suites summary                               |
+----------------------+--------+--------+---------+-------+-------+--------+
|                      | passed | failed | skipped | flaky | total | result |
+----------------------+--------+--------+---------+-------+-------+--------+
| Plan                 | 159    | 0      | 0       | 0     | 159   | ✅     |
| Secure               | 60     | 0      | 6       | 2     | 66    | ❗     |
| Manage               | 303    | 0      | 12      | 1     | 315   | ❗     |
| Create               | 441    | 0      | 24      | 8     | 465   | ❗     |
| Verify               | 120    | 0      | 9       | 0     | 129   | ✅     |
| Configure            | 0      | 0      | 9       | 0     | 9     | ➖     |
| Fulfillment          | 6      | 0      | 33      | 0     | 39    | ✅     |
| Package              | 0      | 0      | 9       | 0     | 9     | ➖     |
| Version sanity check | 0      | 0      | 3       | 0     | 3     | ➖     |
| Product Intelligence | 6      | 0      | 0       | 0     | 6     | ✅     |
| Protect              | 6      | 0      | 0       | 0     | 6     | ✅     |
| Release              | 18     | 0      | 0       | 0     | 18    | ✅     |
+----------------------+--------+--------+---------+-------+-------+--------+
| Total                | 1119   | 0      | 105     | 11    | 1224  | ❗     |
+----------------------+--------+--------+---------+-------+-------+--------+

• The gitlab-qa-mirror downstream pipeline for !88937 (1edb78c0) passed.
• The gitlab-qa-mirror downstream pipeline for !88937 (1edb78c0) failed!
• The gitlab-qa-mirror downstream pipeline for !88937 (27532ddc) failed!
• The gitlab-qa-mirror downstream pipeline for !88937 (27532ddc) passed.
• The gitlab-qa-mirror downstream pipeline for !88937 (98bcacff) failed!
• The gitlab-qa-mirror downstream pipeline for !88937 (98bcacff) failed!
• The gitlab-qa-mirror downstream pipeline for !88937 (e58b8947) failed!
• The gitlab-qa-mirror downstream pipeline for !88937 (e58b8947) passed.
• The gitlab-qa-mirror downstream pipeline for !88937 (e1bb1cec) passed.
• The gitlab-qa-mirror downstream pipeline for !88937 (e1bb1cec) passed.
• The gitlab-qa-mirror downstream pipeline for !88937 (211d9bf2) passed.
• The gitlab-qa-mirror downstream pipeline for !88937 (211d9bf2) passed.
• The gitlab-qa-mirror downstream pipeline for !88937 (211d9bf2) failed!
• The gitlab-qa-mirror downstream pipeline for !88937 (211d9bf2) failed!
• The gitlab-qa-mirror downstream pipeline for !88937 (ddf3a6b8) passed.
• The gitlab-qa-mirror downstream pipeline for !88937 (ddf3a6b8) passed.
• The gitlab-qa-mirror downstream pipeline for !88937 (ddf3a6b8) passed.
• The gitlab-qa-mirror downstream pipeline for !88937 (ddf3a6b8) passed.

requested review from @annabeldunstone

added 724 commits

• 1edb78c0...234ae3d4 - 723 commits from branch master
• 27532ddc - Added finish review dropdown

Compare with previous version

changed the description

@iamphill This is looking great!!! I have a few questions/comments:

1. Will approvals be added in a separate MR?
2. Slash commands/mentioning users doesn't autocomplete (it does work on submit though)
3. On submit, user should be redirected to that comment on the Overview tab (I think we should do this in the first iteration, but I'm wondering if users would prefer to instead stay on the Changes tab and add a toast that says Review submitted I'll look into this later)
4. What are our options for making the Write/Preview buttons follow our tab design more closely? The main problem I see with the current implementation is that when you're on a tab, you don't actually have any visual indicator telling you which tab you're on

removed review request for @annabeldunstone

changed the description

added 1010 commits

• 27532ddc...3d0b9480 - 1006 commits from branch master
• 5a3a7b29 - Added finish review dropdown
• ff974b3c - Fixed styling of tabs in dropdown
• d68ba054 - Fixed size of dropdown on smaller screen sizes
• 98bcacff - Added specs to actions

Compare with previous version

requested review from @annabeldunstone

added 160 commits

• 98bcacff...29b9decb - 155 commits from branch master
• 935718e2 - Added finish review dropdown
• 0649289f - Fixed styling of tabs in dropdown
• 2c46658b - Fixed size of dropdown on smaller screen sizes
• f346b922 - Added specs to actions
• e58b8947 - Fixed failing actions spec

Compare with previous version

Can we make the "Finish review" button secondary? I just realized we're probably not meant to have so many primary buttons next to each other

One other thing I noticed- the quick actions autocomplete now but selecting an option collapses the dropdown

removed review request for @annabeldunstone

marked this merge request as ready

added 1 commit

• e1bb1cec - Scroll to note that was posted

Compare with previous version

added 1 commit

• 211d9bf2 - Added spec for drafts controller

Compare with previous version

requested review from @annabeldunstone

@iamphill Everything looks great! except for the autocomplete experience.

What are our options to get this working? Do we have any?

requested review from @garyh

@aqualls How do you want to sort the docs here? Do you want me to create some basic docs?

approved this merge request

removed review request for @annabeldunstone

@annabeldunstone, thanks for approving this merge request.

This is the first time the merge request is approved. To ensure full test coverage, a new pipeline has been started.

For more info, please refer to the following links:

• rspec minimal jobs
• jest minimal jobs
• merging a merge request.

added 1 commit

• ddf3a6b8 - Use GitLab UI components for form

Compare with previous version

approved this merge request

requested review from @kushalpandya

removed review request for @garyh

approved this merge request

unapproved this merge request

approved this merge request

Thanks @iamphill, left a minor suggestion, looks good otherwise.

removed review request for @kushalpandya

requested review from @kushalpandya

resolved all threads

enabled an automatic merge when the pipeline for 6aecd6d7 succeeds

merged

mentioned in commit e5d1d562

added workflowstaging-canary label

added workflowcanary label and removed workflowstaging-canary label

added workflowstaging label and removed workflowcanary label

added workflowproduction label and removed workflowstaging label

added releasedpublished label

@iamphill I just noticed this is causing the height of the whole dropdown to jump when switching between Write and Preview. Do we need it?

@annabeldunstone I'll get this fixed in the approve checkbox merge request

Sounds good

added releasedcandidate label and removed releasedpublished label

mentioned in merge request !93124 (closed)

mentioned in issue #368799 (closed)

mentioned in issue #368916 (closed)

mentioned in issue gitlab-org/quality/pipeline-triage#154 (closed)

removed documentation label

mentioned in merge request !93725 (merged)

mentioned in merge request gitlab-com/www-gitlab-com!109140 (merged)