Add status and notifications for your own PRs (#303)

# Why

Until now, PR Monitor was primarily useful to review incoming PRs. However, it didn't give any feedback about your own PRs, e.g. when they were approved or "rejected".

# What

This introduces the distinction in your own PRs between:
- a PR that is pending review (with or without assigned reviewers)
- a PR where changes have been requested
- a PR that has been approved

A notification is shown when your PR is approved, or when new changes are requested. Note that no notification is shown when a new comment is posted, as this isn't necessarily a call to action (and we can't easily know whether it's a positive comment that can safely be ignored or a negative comment that's equivalent to a request for change).

Additionally, the status of each PR is shown in the popup under the "My PRs" tab.

# Implementation details

The following changes were made:
- Make an extra request to get PR details in order to fetch the list of requested reviewers, which unfortunately isn't included in the /issues/search endpoint.
- Add more granular `PullRequestStatus` values to distinguish between possible states.
- Add outgoingPullRequestStatus() which computes the status based on the PR information.
- Add badges to visually represent the status of each of your own PRs.
- Add notification handling of approved and "rejected" PRs.

src/components/PullRequestStatus.tsx

@@ -22,7 +22,7 @@ const AUTHOR_REPLIED = (
 );
 
 const NEW_COMMIT = (
-  <Badge pill variant="success">
+  <Badge pill variant="primary">
     New commit
   </Badge>
 );
@@ -33,6 +33,30 @@ const DRAFT = (
   </Badge>
 );
 
+const APPROVED = (
+  <Badge pill variant="success">
+    Approved
+  </Badge>
+);
+
+const CHANGES_REQUESTED = (
+  <Badge pill variant="warning">
+    Changes requested
+  </Badge>
+);
+
+const WAITING_FOR_REVIEW = (
+  <Badge pill variant="info">
+    Waiting for review
+  </Badge>
+);
+
+const NO_REVIEWER_ASSIGNED = (
+  <Badge pill variant="light">
+    No reviewer assigned
+  </Badge>
+);
+
 export const PullRequestStatus = observer(
   ({ pullRequest }: { pullRequest: EnrichedPullRequest }) => {
     const status = renderStatus(pullRequest.status);
@@ -62,6 +86,18 @@ function renderStatus(status: Status) {
           {AUTHOR_REPLIED} {NEW_COMMIT}
         </>
       );
+    case Status.OUTGOING_APPROVED:
+      return APPROVED;
+    case Status.OUTGOING_PENDING_CHANGES:
+      return CHANGES_REQUESTED;
+    case Status.OUTGOING_PENDING_REVIEW_HAS_REVIEWERS:
+      return WAITING_FOR_REVIEW;
+    case Status.OUTGOING_PENDING_REVIEW_NO_REVIEWERS:
+      return (
+        <>
+          {WAITING_FOR_REVIEW} {NO_REVIEWER_ASSIGNED}
+        </>
+      );
     default:
       return null;
   }

src/filtering/status.ts

@@ -1,4 +1,4 @@
-import { PullRequest } from "../storage/loaded-state";
+import { PullRequest, ReviewState } from "../storage/loaded-state";
 import { userPreviouslyReviewed } from "./reviewed";
 import {
   getLastAuthorCommentTimestamp,
@@ -7,26 +7,21 @@ import {
 } from "./timestamps";
 
 /**
- * Returns whether another review is needed for a PR:
- * - either a review is explicitly requested
- * - or the PR has been updated by the author (new commit or comment) since the last review
+ * Returns the {@link PullRequestStatus} of a PR.
  */
 export function pullRequestStatus(
   pr: PullRequest,
   currentUserLogin: string
 ): PullRequestStatus {
   if (pr.author.login === currentUserLogin) {
-    return PullRequestStatus.OUTGOING;
+    return outgoingPullRequestStatus(pr, currentUserLogin);
   }
   if (!pr.reviewRequested && !userPreviouslyReviewed(pr, currentUserLogin)) {
     return PullRequestStatus.NOT_INVOLVED;
   }
   return incomingPullRequestStatus(pr, currentUserLogin);
 }
 
-/**
- * Returns whether the user, who previously wrote a review, needs to take another look.
- */
 function incomingPullRequestStatus(
   pr: PullRequest,
   currentUserLogin: string
@@ -52,14 +47,118 @@ function incomingPullRequestStatus(
   }
 }
 
+function outgoingPullRequestStatus(
+  pr: PullRequest,
+  currentUserLogin: string
+): PullRequestStatus {
+  const lastReviewOrCommentFromCurrentUserTimestamp = getLastReviewOrCommentTimestamp(
+    pr,
+    currentUserLogin
+  );
+  const lastCommitTimestamp = getLastCommitTimestamp(pr);
+  const lastActionByCurrentUserTimestamp = Math.max(
+    lastReviewOrCommentFromCurrentUserTimestamp,
+    lastCommitTimestamp
+  );
+  const statusByUser = new Map<string, ReviewState>();
+  for (const review of pr.reviews) {
+    if (review.state === "COMMENTED") {
+      // A comment doesn't necessarily override a previous request for changes
+      // or an approval. Best to just ignore it?
+      continue;
+    }
+    const submittedAt = new Date(review.submittedAt).getTime();
+    if (submittedAt < lastActionByCurrentUserTimestamp) {
+      // This review may not be relevant anymore.
+      statusByUser.set(review.authorLogin, "PENDING");
+      continue;
+    }
+    statusByUser.set(review.authorLogin, review.state);
+  }
+
+  // Requested reviewers are specifically reviewers who haven't posted a review
+  // after a request. This can also be the case when they had previously
+  // reviewed, but the author requested another review from them.
+  for (const requestedReviewer of pr.requestedReviewers || []) {
+    statusByUser.set(requestedReviewer, "PENDING");
+  }
+
+  const statuses = new Set(statusByUser.values());
+  if (statuses.has("CHANGES_REQUESTED")) {
+    return PullRequestStatus.OUTGOING_PENDING_CHANGES;
+  }
+  if (statuses.has("PENDING")) {
+    return PullRequestStatus.OUTGOING_PENDING_REVIEW_HAS_REVIEWERS;
+  }
+  if (statuses.has("APPROVED")) {
+    return PullRequestStatus.OUTGOING_APPROVED;
+  }
+  // If there are no reviewers (e.g. for a PR on an open source repo you're not
+  // a member of) then default to pending review.
+  return statuses.size > 0
+    ? PullRequestStatus.OUTGOING_PENDING_REVIEW_HAS_REVIEWERS
+    : PullRequestStatus.OUTGOING_PENDING_REVIEW_NO_REVIEWERS;
+}
+
 export enum PullRequestStatus {
+  /**
+   * A review has been requested from the user, but they haven't submitted any
+   * review or comments on the PR yet.
+   */
   INCOMING_NEW_REVIEW_REQUESTED,
+
+  /**
+   * A review or comment was previously submitted by the user, and the author
+   * has since posted a comment.
+   */
   INCOMING_REVIEWED_NEW_COMMENT_BY_AUTHOR,
+
+  /**
+   * A review or comment was previously submitted by the user, and a new commit
+   * has since been added to the PR.
+   */
   INCOMING_REVIEWED_NEW_COMMIT,
+
+  /**
+   * A review or comment was previously submitted by the user, and the author
+   * has since posted a comment. Additionally, a new commit has also been added
+   * to the PR.
+   */
   INCOMING_REVIEWED_NEW_COMMIT_AND_NEW_COMMENT_BY_AUTHOR,
+
+  /**
+   * A review or comment was submitted by the user, and the PR has not been
+   * updated since (no comment or further changes by the author).
+   */
   INCOMING_REVIEWED_PENDING_REPLY,
+
+  /**
+   * The current user is not involved in the PR. That is, they are not a
+   * reviewer and haven't posted any comments.
+   */
   NOT_INVOLVED,
-  OUTGOING
+
+  /**
+   * The current user has sent a PR and is waiting for a review from specific people.
+   */
+  OUTGOING_PENDING_REVIEW_HAS_REVIEWERS,
+
+  /**
+   * The current user has created a PR but has no specific reviewers assigned
+   * (e.g. common for open source contributions).
+   */
+  OUTGOING_PENDING_REVIEW_NO_REVIEWERS,
+
+  /**
+   * The current user has sent a PR and received review comments which need to
+   * be addressed either by responding or adding new commits.
+   */
+  OUTGOING_PENDING_CHANGES,
+
+  /**
+   * The current user has sent a PR and received approval from all reviewers.
+   */
+  OUTGOING_APPROVED
 }
 
 export function isReviewRequired(status: PullRequestStatus) {

src/github-api/api.ts

@@ -1,5 +1,6 @@
 import {
   IssuesListCommentsResponse,
+  PullsGetResponse,
   PullsListCommitsResponse,
   PullsListReviewsResponseItem as IncompletePullsListReviewsResponseItem
 } from "@octokit/rest";
@@ -19,6 +20,11 @@ export interface GitHubApi {
    */
   searchPullRequests(query: string): Promise<PullsSearchResponse>;
 
+  /**
+   * Returns the details of a pull request.
+   */
+  loadPullRequestDetails(pr: PullRequestReference): Promise<PullsGetResponse>;
+
   /**
    * Returns the full list of reviews for a pull request.
    */

src/github-api/implementation.ts

@@ -22,6 +22,14 @@ export function buildGitHubApi(token: string): GitHubApi {
         })
       );
     },
+    async loadPullRequestDetails(pr) {
+      const response = await octokit.pulls.get({
+        owner: pr.repo.owner,
+        repo: pr.repo.name,
+        pull_number: pr.number
+      });
+      return response.data;
+    },
     loadReviews(pr) {
       return octokit.paginate(
         octokit.pulls.listReviews.endpoint.merge({

src/loading/internal/pull-requests.spec.ts

@@ -1,5 +1,6 @@
 import {
   IssuesListCommentsResponse,
+  PullsGetResponse,
   PullsListCommitsResponse
 } from "@octokit/rest";
 import {
@@ -79,6 +80,11 @@ describe("refreshOpenPullRequests", () => {
         );
       }
     });
+    githubApi.loadPullRequestDetails.mockReturnValue(
+      Promise.resolve(({
+        requested_reviewers: []
+      } as unknown) as PullsGetResponse)
+    );
     githubApi.loadComments.mockReturnValue(Promise.resolve([]));
     githubApi.loadReviews.mockReturnValue(Promise.resolve([]));
     githubApi.loadCommits.mockReturnValue(Promise.resolve([]));
@@ -98,6 +104,10 @@ function mockGitHubApi() {
   return {
     loadAuthenticatedUser: jest.fn<Promise<GetAuthenticatedUserResponse>, []>(),
     searchPullRequests: jest.fn<Promise<PullsSearchResponse>, [string]>(),
+    loadPullRequestDetails: jest.fn<
+      Promise<PullsGetResponse>,
+      [PullRequestReference]
+    >(),
     loadReviews: jest.fn<
       Promise<PullsListReviewsResponse>,
       [PullRequestReference]

src/loading/internal/pull-requests.ts

@@ -1,3 +1,4 @@
+import { PullsGetResponse } from "@octokit/rest";
 import {
   GitHubApi,
   PullRequestReference,
@@ -52,7 +53,13 @@ async function updateCommentsAndReviews(
     repo,
     number: rawPullRequest.number
   };
-  const [freshReviews, freshComments, freshCommits] = await Promise.all([
+  const [
+    freshPullRequestDetails,
+    freshReviews,
+    freshComments,
+    freshCommits
+  ] = await Promise.all([
+    githubApi.loadPullRequestDetails(pr),
     githubApi.loadReviews(pr).then(reviews =>
       reviews.map(review => ({
         authorLogin: review.user.login,
@@ -75,6 +82,7 @@ async function updateCommentsAndReviews(
   ]);
   return pullRequestFromResponse(
     rawPullRequest,
+    freshPullRequestDetails,
     freshReviews,
     freshComments,
     freshCommits,
@@ -84,6 +92,7 @@ async function updateCommentsAndReviews(
 
 function pullRequestFromResponse(
   response: PullsSearchResponseItem,
+  details: PullsGetResponse,
   reviews: Review[],
   comments: Comment[],
   commits: Commit[],
@@ -104,6 +113,9 @@ function pullRequestFromResponse(
     title: response.title,
     draft: response.draft,
     reviewRequested,
+    requestedReviewers: details.requested_reviewers.map(
+      reviewer => reviewer.login
+    ),
     reviews,
     comments,
     commits

src/notifications/api.ts

@@ -2,7 +2,7 @@ import { EnrichedPullRequest } from "../filtering/enriched-pull-request";
 
 export interface Notifier {
   notify(
-    unreviewedPullRequests: EnrichedPullRequest[],
+    pullRequests: EnrichedPullRequest[],
     alreadyNotifiedPullRequestUrls: Set<string>
   ): void;
   registerClickListener(clickListener: NotifierClickListener): void;

src/notifications/implementation.ts

@@ -5,10 +5,10 @@ import { Notifier } from "./api";
 
 export function buildNotifier(chromeApi: ChromeApi): Notifier {
   return {
-    notify(unreviewedPullRequests, notifiedPullRequestUrls) {
+    notify(pullRequests, notifiedPullRequestUrls) {
       showNotificationForNewPullRequests(
         chromeApi,
-        unreviewedPullRequests,
+        pullRequests,
         notifiedPullRequestUrls
       );
     },
@@ -27,14 +27,10 @@ export function buildNotifier(chromeApi: ChromeApi): Notifier {
  */
 async function showNotificationForNewPullRequests(
   chromeApi: ChromeApi,
-  unreviewedPullRequests: EnrichedPullRequest[],
+  pullRequests: EnrichedPullRequest[],
   notifiedPullRequestUrls: Set<string>
 ) {
-  if (!unreviewedPullRequests) {
-    // Do nothing.
-    return;
-  }
-  for (const pullRequest of unreviewedPullRequests) {
+  for (const pullRequest of pullRequests) {
     if (!notifiedPullRequestUrls.has(pullRequest.htmlUrl)) {
       console.log(`Showing ${pullRequest.htmlUrl}`);
       showNotification(chromeApi, pullRequest);
@@ -77,6 +73,10 @@ function getTitle(pullRequest: EnrichedPullRequest): string {
     case PullRequestStatus.INCOMING_REVIEWED_NEW_COMMIT:
     case PullRequestStatus.INCOMING_REVIEWED_NEW_COMMIT_AND_NEW_COMMENT_BY_AUTHOR:
       return `Pull request updated`;
+    case PullRequestStatus.OUTGOING_APPROVED:
+      return `Pull request approved`;
+    case PullRequestStatus.OUTGOING_PENDING_CHANGES:
+      return `New changes requested`;
     default:
       throw new Error(`Well, this should never happen.`);
   }