Updates to reflect best practices for PR review

docs/contributing/pull-requests/branching.md

@@ -1,4 +1,5 @@
 ---
+sidebar_position: 1
 sidebar_custom_props:
   access: bitwarden
 ---

docs/contributing/pull-requests/chromatic.md

@@ -1,4 +1,5 @@
 ---
+sidebar_position: 3
 sidebar_custom_props:
   access: bitwarden
 ---

docs/contributing/pull-requests/code-review.md

@@ -1,10 +1,18 @@
+---
+sidebar_position: 2
+---
+
 # Code Review Guidelines
 
 At Bitwarden, we encourage everyone to participate in code reviews. A team will focus primarily on
 their own code reviews, but if you see something interesting, feel free to jump in and discuss.
 
-To have efficient code reviews there are a few things to keep in mind (from
-[Best Practices for Code Review | SmartBear](https://smartbear.com/learn/code-review/best-practices-for-peer-code-review/)):
+:::tip Want to read more?
+
+You can find more tips for PR review here:
+[Best Practices for Code Review | SmartBear](https://smartbear.com/learn/code-review/best-practices-for-peer-code-review/)
+
+:::
 
 - Pull requests should be manageable. If the PR is too large -- significantly above a few hundred
   lines -- ask the contributor if it can be split it up into multiple PRs before reviewing.
@@ -15,13 +23,6 @@ To have efficient code reviews there are a few things to keep in mind (from
 Don’t feel bad for taking your time when doing code reviews! They often take longer than you think,
 and we should be spending as much time as needed.
 
-:::tip
-
-Bugs or defects found early in the development cycle have a much smaller cost associated with fixing
-them.
-
-:::
-
 ## Reviewing
 
 If you feel that someone else has good knowledge of the code you are reviewing, please feel free to
@@ -33,6 +34,11 @@ always welcome! For example, it’s okay to leave some general comments or feedb
 that you don’t have enough knowledge to approve the changes. The author can ask for another review
 from someone else, and there’s nothing wrong in having two reviewers on a PR.
 
+While we mostly use an asynchronous review process, please don't hesitate to schedule a meeting with
+the author to discuss the changes. While asynchronous communication can be useful, it incurs a time
+penalty which can drag out the review process. Sometimes setting up a short call to discuss the
+changes can potentially save a lot of time.
+
 :::info Note
 
 <a id="assumptions-note"></a> When reviewing code, remember that all software is built to conform to
@@ -46,20 +52,36 @@ requirements, which may not necessarily be in line with the previous solution.
 
 Please use the review statuses appropriately.
 
+:::tip Think about your audience
+
+When providing comments or requesting changes, keep in mind the experience level of the author.
+
+If the engineer is new to the company and the codebase or less experienced overall, they would
+probably welcome more explicit background on your concerns and more concrete suggestions, whereas a
+more experienced engineer would prefer general guidance so that they can solve the problem
+themselves.
+
+:::
+
 #### Comment
 
 Comment is a great way to discuss things without explicitly approving or requesting changes.
 
+We use [conventional comments](https://conventionalcomments.org) to help convey the intent and
+expected action taken from each comment on the PR.
+
 #### Request changes
 
 Request changes should be used when you believe something **needs** to change prior to the PR
 getting merged, as it will prevent someone else from approving the PR before your concerns have been
 tackled.
 
-We shouldn’t hesitate to use this status, however we should give clear feedback on what needs to
-change for the PR to get approved. Likewise a PR author should not be discouraged by a _request for
-changes_, it's simply an indication that changes should be made prior to the PR being merged. This
-is common.
+We shouldn’t hesitate to use this status. However, it does come with obligations for the reviewer.
+By blocking the PR from progressing, they have taken an ownership stake in it and should make
+themselves available to answer clarifying questions. They should also give clear feedback on what
+needs to change for the PR to get approved. Likewise, a PR author should not be discouraged by a
+request for changes, it's simply an indication that changes should be made prior to the PR being
+merged. This is common.
 
 :::warning Discarding reviews
 
@@ -76,12 +98,14 @@ scenarios where discarding the reviews are generally seen as accepted.
 
 #### Approve
 
-Approving a PR means that you have confidence in that the code works, and that it does what the PR
+Approving a PR means that you have confidence in that the code works and that it does what the PR
 claims. This can be based on testing the change or on previous domain knowledge.
 
 - The PR targets the [correct branch](branching/#which-branching-model-to-choose).
 - You have verified that the linked Jira issue description matches the changes made in the PR.
 - You have read and understood the full impact of the changes suggested by the PR.
+- You have verified that all possible changes have been
+  [unit tested](./../testing/unit/naming-conventions.mdx).
 - You attest that the changes
   - Solve the intended problem,
   - [solve the requirements in the best way](#assumptions-note),
@@ -113,6 +137,7 @@ a time.
   - Does the PR change the areas you expect to be changed?
     - Are any missing?
     - Are any present you didn't expect?
+  - Are there unit tests present?
 - Micro View - Focus on individual files.
   - Is the code style adhered?
   - Is the code readable?

docs/contributing/pull-requests/index.md

@@ -97,33 +97,75 @@ changes” to a PR forcing the reviewer to start over again.
 
 :::
 
-## Creating a Pull Request
+## Creating a pull request
 
 The Bitwarden repositories have a _Pull Request template_ which should be followed. This will ensure
-the PR review goes smoothly since it will provide context to the reviewer. <community> Once a
-community PRs has been created, they will be automatically be linked to an internal Jira ticket. The
-internal ticket is used for prioritization and tracking purposes.</community><bitwarden>Include a
-Jira ticket reference so the reviewer can gain all context on the work.</bitwarden> Tag
-`@dept-design` as a reviewer for any UI changes.
-
-## Review process
+the PR review goes smoothly since it will provide context to the reviewer.
 
 <community>
+Once a community PRs has been created, they will be automatically be linked to an internal Jira ticket. The
+internal ticket is used for prioritization and tracking purposes.
+</community>
 
-Once a Community PR has been created a Bitwarden developer will perform a code review, while we try
-to this in a reasonable time-frame, please understand that we have internal roadmaps and priorities
-that may delay this process.
+<bitwarden>
+When creating the PR, include a Jira ticket reference so the reviewer can gain all context on the work, as well as links to any associated PRs in other repositories.
 
-</community>
+### Tagging reviewers
+
+We use `CODEOWNERS` in each repository to assign the reviewing teams based on the files in the PR.
+These reviews are required for the changes to be merged to `master`.
+
+You can tag additional teams or individuals for review as you see fit, including `@dept-design` for
+any design changes.
+
+### Opening the PR for review
+
+As its name implies, marking a PR as "Ready for Review" indicates that you are ready for all
+assigned teams to review it. If the changes are still in progress, leave the PR in `Draft` status.
+Doing this ensures that reviewers can act on the "Ready for Review" as their signal to begin the
+review process without further notification.
+
+</bitwarden>
+
+## Reviewing the pull request
 
 <bitwarden>
 
-While we mostly use an async review process, please don't hesitate to schedule a meeting with the
-reviewer/contributor to discuss the changes. While async communication can be useful it incurs a
-time penalty which can drag out the review process. And sometimes setting up a short call to discuss
-the changes can potentially save a lot of time.
+At Bitwarden, we believe that the act of reviewing PRs is a critically important part of each
+engineer's job. It is as important, if not more important, than the act of writing code.
+
+To ensure that teams within the organization operate on same set of assumptions for performing
+reviews, we have agreed to a baseline set of expectations.
+
+When a PR author opens a PR for review, they should have the expectation that:
+
+- The act of opening the PR for review is the **only** notification required. Teams are responsible
+  for properly configuring notifications so that team members are aware of their obligations.
+- The reviewing team(s) will respond within **24 hours** to either:
+  - Provide a review, or
+  - Inform the author when such a review will be provided.
+
+:::info Follow-up notification
+
+If there is no response to a request for review in 24 hours, the author should reach out to the
+team(s) - or to individual engineers if assigned - via Slack to follow up.
+
+This should wait for 24 hours to allow the default process to take place and not overwhelm the team
+with notifications on multiple platforms.
+
+:::
+
+###
 
 </bitwarden>
 
+<community>
+
+Once a Community PR has been created, a Bitwarden developer will perform a code review. While we try
+to this in a reasonable time frame, please understand that we have internal roadmaps and priorities
+that may delay this process.
+
+</community>
+
 We've written up some [guidelines](./code-review.md) for reviewing code, which we recommend reading
 before performing your first code review.