diff --git a/app/tasks/gh-design/filterReviewers.spec.ts b/app/tasks/gh-design/filterReviewers.spec.ts new file mode 100644 index 00000000..722600e2 --- /dev/null +++ b/app/tasks/gh-design/filterReviewers.spec.ts @@ -0,0 +1,51 @@ +import { describe, expect, it } from "bun:test"; +import { filterReviewers } from "./filterReviewers"; + +describe("filterReviewers", () => { + const REVIEWERS = ["PabloWiedemann", "AliceDev"]; + + it("excludes the PR author from both lists", () => { + const result = filterReviewers(REVIEWERS, "PabloWiedemann"); + expect(result.requestReviewers).toEqual(["AliceDev"]); + expect(result.newReviewers).toEqual(["AliceDev"]); + }); + + it("returns all reviewers when author is not in the list", () => { + const result = filterReviewers(REVIEWERS, "SomeoneElse"); + expect(result.requestReviewers).toEqual(["PabloWiedemann", "AliceDev"]); + expect(result.newReviewers).toEqual(["PabloWiedemann", "AliceDev"]); + }); + + it("excludes already-requested reviewers from newReviewers only", () => { + const result = filterReviewers(REVIEWERS, "SomeoneElse", ["PabloWiedemann"]); + expect(result.requestReviewers).toEqual(["PabloWiedemann", "AliceDev"]); + expect(result.newReviewers).toEqual(["AliceDev"]); + }); + + it("returns empty newReviewers when all are already requested", () => { + const result = filterReviewers(REVIEWERS, "SomeoneElse", ["PabloWiedemann", "AliceDev"]); + expect(result.requestReviewers).toEqual(["PabloWiedemann", "AliceDev"]); + expect(result.newReviewers).toEqual([]); + }); + + it("returns empty lists when author is the only reviewer", () => { + const result = filterReviewers(["PabloWiedemann"], "PabloWiedemann"); + expect(result.requestReviewers).toEqual([]); + expect(result.newReviewers).toEqual([]); + }); + + it("handles undefined alreadyRequested as no-one requested yet", () => { + const result = filterReviewers(REVIEWERS, "SomeoneElse", undefined); + expect(result.newReviewers).toEqual(["PabloWiedemann", "AliceDev"]); + }); + + it("compares usernames case-insensitively", () => { + const result = filterReviewers(REVIEWERS, "pablowiedemann"); + expect(result.requestReviewers).toEqual(["AliceDev"]); + }); + + it("matches already-requested reviewers case-insensitively", () => { + const result = filterReviewers(REVIEWERS, "SomeoneElse", ["pablowiedemann"]); + expect(result.newReviewers).toEqual(["AliceDev"]); + }); +}); diff --git a/app/tasks/gh-design/filterReviewers.ts b/app/tasks/gh-design/filterReviewers.ts new file mode 100644 index 00000000..dc0aac72 --- /dev/null +++ b/app/tasks/gh-design/filterReviewers.ts @@ -0,0 +1,17 @@ +/** + * Compute eligible reviewers for a PR. + * Returns `requestReviewers` (all reviewers minus the PR author) and + * `newReviewers` (eligible reviewers not yet requested). + * GitHub usernames are case-insensitive, so comparisons are normalized. + */ +export function filterReviewers( + allReviewers: string[], + prAuthor: string, + alreadyRequested?: string[], +): { requestReviewers: string[]; newReviewers: string[] } { + const normalizedAuthor = prAuthor.toLowerCase(); + const normalizedRequested = new Set(alreadyRequested?.map((r) => r.toLowerCase()) ?? []); + const requestReviewers = allReviewers.filter((e) => e.toLowerCase() !== normalizedAuthor); + const newReviewers = requestReviewers.filter((e) => !normalizedRequested.has(e.toLowerCase())); + return { requestReviewers, newReviewers }; +} diff --git a/app/tasks/gh-design/gh-design.ts b/app/tasks/gh-design/gh-design.ts index 97601794..1e9af828 100644 --- a/app/tasks/gh-design/gh-design.ts +++ b/app/tasks/gh-design/gh-design.ts @@ -21,6 +21,7 @@ import { planDesignCommentNotification, } from "./slackNotifications"; import { slackMessageUrlParse, slackMessageUrlStringify } from "./slackMessageUrlParse"; +import { filterReviewers } from "./filterReviewers"; const tlog = createTimeLogger(); /** @@ -254,21 +255,37 @@ export async function runGithubDesignTask() { }); if (task.state === "open") { - if ( - task.type === "pull_request" && - REQUEST_REVIEWERS.some((e) => !task.reviewers?.includes(e)) - ) { - const requestReviewers = REQUEST_REVIEWERS; - const newReviewers = requestReviewers.filter((e) => !task.reviewers?.includes(e)); - tlog(`Requesting reviewers: ${newReviewers.join(", ")}`); - if (!dryRun) { - await gh.pulls.requestReviewers({ - owner, - repo, - pull_number: issue_number, - reviewers: newReviewers, - }); - task = await saveGithubDesignTask(url, { reviewers: requestReviewers }); + if (task.type === "pull_request") { + const { requestReviewers, newReviewers } = filterReviewers( + REQUEST_REVIEWERS, + task.user, + task.reviewers, + ); + if (newReviewers.length > 0) { + tlog(`Requesting reviewers: ${newReviewers.join(", ")}`); + if (!dryRun) { + let reviewersRequested = false; + try { + await gh.pulls.requestReviewers({ + owner, + repo, + pull_number: issue_number, + reviewers: newReviewers, + }); + reviewersRequested = true; + } catch (err: unknown) { + // GitHub may return 422 when a requested reviewer cannot be added, + // such as when they are not a collaborator or cannot be requested. + // We log but don't persist, so the request will be retried on the + // next run (the reviewer may become eligible later). + const status = (err as { status?: number })?.status; + if (status !== 422) throw err; + tlog(`Reviewer request rejected (422): ${err}`); + } + if (reviewersRequested) { + task = await saveGithubDesignTask(url, { reviewers: requestReviewers }); + } + } } }