docs: edit reviewer checklist

[mikesposito]

Description

Each PR includes the "Pre-merge reviewer checklist", but it almost never gets used by reviewers. Also, it carries some intrinsic problems:

• The template doesn't support multiple reviewers explicitly
• Checkmarks left by a reviewer tend to become stale when the author pushes new commits, which would give a false indication since the reviewer would have to go through another review cycle. This is probably worse than leaving the checklist unchecked
• The PR author implicitly loses ownership of the main PR comment, since reviewers are supposed to edit it

With this PR I would like to propose an alternative solution, with reviewers including the Pre-merge reviewer checklist in the review feedback comment (See example). This would have these advantages:

• Multiple reviewers supported out of the box, because each reviewer writes its own checklist
• Gives the opportunity to reviewers to leave multiple feedbacks along the review cycle, with potentially different test outcomes: this makes even more sense if the checklist should also detail what exactly has been tested
• It makes easy to see the last commit tested by the reviewer and infer whether the checklist may still be valid after additional commits
• It leaves the ownership of the main PR comment to the author
• If a reviewer includes a checklist, chances are that other reviewers will do that as well

I believe this should translate in an additional explicit instruction in the PR template, but I'm not sure how to phrase it. I'd love to hear your thoughts!

Related issues

N/A

Manual testing steps

N/A

Screenshots/Recordings

N/A

Pre-merge author checklist

• [x] I've followed MetaMask Contributor Docs and MetaMask Extension Coding Standards.
• [x] I've completed the PR template to the best of my ability
• [x] I’ve included tests if applicable
• [x] I’ve documented my code using JSDoc format if applicable
• [ ] I’ve applied the right labels on the PR (see labeling guidelines). Not required for external contributors.

Pre-merge reviewer checklist

• [ ] I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed).
• [ ] I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots.

[sonarqubecloud[bot]]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

[codecov[bot]]

Codecov Report

:white_check_mark: All modified and coverable lines are covered by tests. :white_check_mark: Project coverage is 70.01%. Comparing base (56c23fc) to head (f77774c). :warning: Report is 4616 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main   #27217   +/-   ##
=======================================
  Coverage   70.01%   70.01%           
=======================================
  Files        1445     1445           
  Lines       50201    50201           
  Branches    14045    14045           
=======================================
  Hits        35147    35147           
  Misses      15054    15054           

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

:rocket: New features to boost your workflow:

• :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• :package: JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[metamaskbot]

Builds ready [f77774c]

• builds: chrome, firefox
• builds (beta): chrome
• builds (flask): chrome, firefox
• builds (MMI): chrome, firefox
• builds (test): chrome, firefox
• builds (test-flask): chrome, firefox
• build viz: Build System
• mv3: Background Module Init Stats
• mv3: UI Init Stats
• mv3: Module Load Stats
• mv3: Bundle Size Stats
• mv2: E2e Actions Stats
• code coverage: Report
• storybook: Storybook
• typescript migration: Dashboard
• all artifacts
• bundle viz:

  • background: 0, 1, 2, 3, 4, 5
  • common: 0, 1, 10, 11, 2, 3, 4, 5, 6, 7, 8, 9
  • content-script: 0
  • offscreen: 0
  • ui: 0, 1, 10, 11, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (2008 ± 133 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	303	2640	1937	464	223
		domContentLoaded	1662	2625	1978	280	135
		load	1684	2635	2008	278	133
		domInteractive	18	108	46	24	11

Bundle size diffs

• background: 0 Bytes (0.00%)
• ui: 0 Bytes (0.00%)
• common: 0 Bytes (0.00%)

[NicolasMassart]

I really like your point on this specific template part. Could not agree more on your analysis.

I see one way to enforce what you suggest: actions or an app that checks approval comment and errors if the template is not used. But that's a bit radical to me. A softer way, that would require that we unify our practices over all repos, would be to suggest to have "saved replies" (see the arrow in comment menu): We could have what you suggest in this PR and remind users to add this reply to their saved replies so it's easier to add in the comment than copy/paste.

[github-actions[bot]]

This PR has been automatically marked as stale because it has not had recent activity in the last 60 days. It will be closed in 14 days. Thank you for your contributions.

[NicolasMassart]

No one interested in this? I think it's a good idea that could also be used in mobile repo.

[mcmire]

As a suggestion on how we could improve this:

One problem that I've noticed in our repos is that when people merge a PR, they tend not to update the commit message, they just press the green button. This makes commit messages fairly unreadable on the command line, as by default we instruct GitHub to copy the entire PR description to the commit message. So what you end up with is a very high level of noise in the commit history.

I do not think we are ever going to get people to remember to update the commit message, however, I think we can simplify the PR description to reduce some of the noise.

What if instead of asking people to copy and paste a checklist, we add a GitHub workflow which automatically creates a comment including the checklist when the PR is created? The author can then check it off in a similar fashion.

[HowardBraham]

One problem that I've noticed in our repos is that when people merge a PR, they tend not to update the commit message, they just press the green button.

Before we turned on the Merge Queue, it was true that people "tended not to update the commit message." Now that we have the Merge Queue, it is actually impossible to update the commit message when merging.

[github-actions[bot]]

This PR has been automatically marked as stale because it has not had recent activity in the last 90 days. It will be closed in 7 days. Thank you for your contributions.

[github-actions[bot]]

This PR has been automatically marked as stale because it has not had recent activity in the last 90 days. It will be closed in 7 days. Thank you for your contributions.

[github-actions[bot]]

This PR was closed because there has been no follow up activity in 7 days. Thank you for your contributions.