From da708641d63a80c430c7a9951008de301c20dc54 Mon Sep 17 00:00:00 2001 From: Michael Neeley Date: Mon, 26 Oct 2026 12:05:07 -0400 Subject: [PATCH] Restore small PR reminders after a week awaiting review --- README.md | 2 +- lib/notifications.ts | 41 ++++++++++++++---------- tests/fixtures/upstream.mts | 1 + tests/notifications.test.ts | 64 +++++++++++++++++++++++++++++++++++++ 4 files changed, 90 insertions(+), 18 deletions(-) diff --git a/README.md b/README.md index b484c77..428fc5f 100644 --- a/README.md +++ b/README.md @@ -59,7 +59,7 @@ Complete Linear assignment/status histories cache for one hour per issue update Cron only runs on production deployments. Preview team metrics, open work, and projects load on demand through deployment-scoped 60-second caches when fresh snapshots are absent; their first load can take a few minutes. They never read production's Redis namespace. Fleet, apps, PTO, and regressions still require their authenticated refresh routes and fresh preview snapshots; fleet/store/blame work does not run in page requests. Notifications and Better Stack heartbeat delivery are disabled outside `VERCEL_ENV=production`. -Priority bug notifications run at 12:00 UTC. Bug lines include **Review requested** and reviewer mentions only when the linked PR and reviewer meet the same gates as the stale-work PR reminders below, including the over-24-hour wait; repeated reviewers across linked PRs are mentioned once per bug. Stale work notifications show each person's last-30-day average review decision time beside review reminders and average Linear completion time beside stale issues. They reuse the fresh metrics Cron snapshot; missing data is `n/a` and does not block reminders. Stale work notifications run at 10:00 New York and start with PRs whose review has waited over 24 hours, limited to PRs linked to Urgent/High Linear issues (by branch, title, or a "closes"/"refs" link in the body), on the default branch, without conflicts, and with passing or no checks, and include open issues assigned to engineers with more than 21 days since their last update, whether standalone or in a project. Project updates run at 14:00 New York. Manager performance outliers run on Friday at 13:00 UTC. New York schedules follow daylight saving time. Weekly leaderboard posting remains unscheduled, as it was previously. +Priority bug notifications run at 12:00 UTC. Bug lines include **Review requested** and reviewer mentions only when the linked PR and reviewer meet the same gates as the stale-work PR reminders below, including the over-24-hour wait; repeated reviewers across linked PRs are mentioned once per bug. Stale work notifications show each person's last-30-day average review decision time beside review reminders and average Linear completion time beside stale issues. They reuse the fresh metrics Cron snapshot; missing data is `n/a` and does not block reminders. Stale work notifications run at 10:00 New York and start with PRs whose review has waited over 24 hours, limited to PRs linked to Urgent/High Linear issues (by branch, title, or a "closes"/"refs" link in the body), on the default branch, without conflicts, and with passing or no checks, followed by **Other PRs**: the same review-readiness gates, but for non-Urgent/High PRs (including those without a linked Linear issue) with fewer than 200 lines added (deletions are unlimited) and a review request waiting at least seven days. Both sections group by engineer and omit empty sections. The post also includes open issues assigned to engineers with more than 21 days since their last update, whether standalone or in a project. Project updates run at 14:00 New York. Manager performance outliers run on Friday at 13:00 UTC. New York schedules follow daylight saving time. Weekly leaderboard posting remains unscheduled, as it was previously. Refresh locks prevent overlapping executions. Ordinary Cron functions have a 300-second limit and a 360-second lease. A timeout cannot block refreshes for eight hours. Failed polls retry on the next Cron tick. Durable jobs retain their eight-hour lease and Workflow retries. diff --git a/lib/notifications.ts b/lib/notifications.ts index 1b7eaed..0d0a83f 100644 --- a/lib/notifications.ts +++ b/lib/notifications.ts @@ -59,6 +59,7 @@ export async function priorityMessage() { const waiting = new Map>(); if (bugs.length) { for (const { issue, slug } of waitingReviews(await reviewQueue(true))) { + if (!issue || ![1, 2].includes(issue.priority)) continue; if (!waiting.has(issue.id)) waiting.set(issue.id, new Set()); waiting.get(issue.id)!.add(slug); } @@ -107,13 +108,10 @@ export async function priorityMessage() { ? `${text.join("\n\n")}\n\n${link(process.env.APP_URL || "", "View Bug Board")}` : null; } -// The old review reminder's gates, with Urgent/High Linear priority replacing its 200-line cap. function waitingReviews(rows: ReviewRow[], now = Date.now()) { - const waiting: { pr: PullRequest; issue: Issue; slug: string; days: number }[] = []; + const waiting: { pr: PullRequest; issue: Issue | undefined; slug: string; days: number }[] = []; for (const { pr, issue } of rows) { if ( - !issue || - ![1, 2].includes(issue.priority) || !pr.baseRefName || pr.baseRefName !== pr.repository.defaultBranchRef?.name || pr.mergeable === "CONFLICTING" || @@ -158,20 +156,29 @@ function waitingReviews(rows: ReviewRow[], now = Date.now()) { } export function reviewReminders(rows: ReviewRow[], now = Date.now(), metrics: Record = {}) { const waiting = waitingReviews(rows, now); - const sections = engineers.flatMap((slug) => { - const prs = waiting.filter((review) => review.slug === slug); - return prs.length - ? [ - `${mention(slug)} (avg review decision: ${metricDisplay("avg_review_decision_time", metrics[slug]?.avg_review_decision_time ?? null)}, last 30d):\n${prs - .sort((a, b) => b.days - a.days) - .map(({ pr, days }) => `- ${link(pr.url, pr.title)} (+${days}d)`) - .join("\n")}`, - ] - : []; + const priority = waiting.filter(({ issue }) => [1, 2].includes(issue?.priority || 0)); + const other = waiting.filter(({ issue, pr, days }) => + ![1, 2].includes(issue?.priority || 0) && days >= 7 && pr.additions < 200, + ); + const sections = [ + ["PRs - Checks Passing, Waiting for Review (+24h, Urgent/High priority)", priority], + ["Other PRs - Checks Passing, Waiting for Review (7+ days, <200 lines added)", other], + ] as const; + const text = sections.flatMap(([title, reviews]) => { + const reviewers = engineers.flatMap((slug) => { + const prs = reviews.filter((review) => review.slug === slug); + return prs.length + ? [ + `${mention(slug)} (avg review decision: ${metricDisplay("avg_review_decision_time", metrics[slug]?.avg_review_decision_time ?? null)}, last 30d):\n${prs + .sort((a, b) => b.days - a.days) + .map(({ pr, days }) => `- ${link(pr.url, pr.title)} (+${days}d)`) + .join("\n")}`, + ] + : []; + }); + return reviewers.length ? [`*${title}*\n\n${reviewers.join("\n\n")}`] : []; }); - return sections.length - ? `*PRs - Checks Passing, Waiting for Review (+24h, Urgent/High priority)*\n\n${sections.join("\n\n")}` - : null; + return text.length ? text.join("\n\n") : null; } export async function staleMessage() { // GitHub's -review:approved search also drops PRs with one approval and another pending review. diff --git a/tests/fixtures/upstream.mts b/tests/fixtures/upstream.mts index da9c5e3..0bacf08 100644 --- a/tests/fixtures/upstream.mts +++ b/tests/fixtures/upstream.mts @@ -161,6 +161,7 @@ const openPRs = [ { title: "Feed query speedup", decision: "APPROVED", ci: "SUCCESS" }, { title: "Login link expiry", decision: "APPROVED", ci: null, mergeable: "CONFLICTING" }, { title: "Bump the lint toolchain", decision: "REVIEW_REQUIRED", ci: "SUCCESS", author: "dependabot[bot]", size: 2400 }, + { title: "Remove obsolete settings", decision: "REVIEW_REQUIRED", ci: "SUCCESS", size: 199 }, ].map((o, i) => { const linked = issues[45 + i], createdAt = ago(1 + i * 2.5), url = `https://github.com/${repos[i % 2]}/pull/${2000 + i}`; if (i % 2) linked.attachments.nodes.push({ metadata: { url } }); diff --git a/tests/notifications.test.ts b/tests/notifications.test.ts index 4098db9..8960d6f 100644 --- a/tests/notifications.test.ts +++ b/tests/notifications.test.ts @@ -182,6 +182,18 @@ test("Slack digests preserve complete lines and claim delivery per part", async assert.ok(unavailable.includes("(avg completion: n/a, last 30d)")); prs = []; }); + await t.test("the daily digest puts other week-old small PRs between priority reviews and stale issues", async () => { + const requestedAt = new Date(Date.now() - 8 * 86_400_000).toISOString(); + const timelineItems = { nodes: [{ __typename: "ReviewRequestedEvent", createdAt: requestedAt, requestedReviewer: { login: "redreceipt" } }] }; + issues = [{ ...issue(10, "Priority work"), priority: 2 }, issue(11)]; + prs = [row(10, 2, { timelineItems }).pr, row(12, null, { additions: 150, deletions: 49, timelineItems }).pr]; + const message = (await staleMessage())!; + assert.ok(message.indexOf("Urgent/High priority") < message.indexOf("Other PRs")); + assert.ok(message.indexOf("Other PRs") < message.indexOf("Stale Open Issues")); + for (const pr of prs) assert.equal(message.split(pr.url).length - 1, 1); + assert.ok(message.includes("<@U01865757S9> (avg review decision: n/a, last 30d):")); + prs = []; + }); await t.test("priority bugs label and mention only reviewers eligible for PR reminders", async () => { const old = new Date(Date.now() - 3 * 86_400_000).toISOString(); const requested = (login: string, createdAt = old) => ({ @@ -351,3 +363,55 @@ test("review reminders keep the old gates with Urgent/High priority instead of s ].join("\n")); assert.equal(reviewReminders([row(3, 3)], now), null); }); + +test("other review reminders require a week per reviewer and fewer than 200 added lines", () => { + const now = Date.parse("2026-10-13T14:00:00Z"), weekAgo = now - 7 * 86_400_000; + const requested = (login: string, at: number) => ({ + __typename: "ReviewRequestedEvent", createdAt: new Date(at).toISOString(), requestedReviewer: { login }, + }); + const small = (number: number, priority: number | null = 3, overrides: Partial = {}) => row(number, priority, { + additions: 150, deletions: 49, + timelineItems: { nodes: [requested("redreceipt", weekAgo), requested("nlewis84", now - 2 * 86_400_000)] }, + ...overrides, + }); + const eligible = [ + small(101), small(102, 4), small(103, 0), small(104, null), + small(107, 3, { additions: 199, deletions: 10_000 }), + small(108, 3, { additions: 0, deletions: 50_000 }), + small(105, 3, { statusCheckRollup: null }), + small(106, 3, { timelineItems: { nodes: [requested("redreceipt", weekAgo - 2 * 86_400_000)] } }), + ]; + const excluded = [ + small(110, 3, { additions: 200, deletions: 0 }), + small(111, 3, { additions: 201, deletions: 0 }), + small(112, 3, { timelineItems: { nodes: [requested("redreceipt", weekAgo + 1)] } }), + small(113, 3, { timelineItems: { nodes: [] } }), + small(114, 3, { timelineItems: { nodes: [requested("redreceipt", weekAgo), requested("redreceipt", now - 1)] } }), + small(115, 3, { baseRefName: "stacked" }), + small(116, 3, { mergeable: "CONFLICTING" }), + small(117, 3, { statusCheckRollup: { state: "PENDING" } }), + small(118, 3, { statusCheckRollup: { state: "FAILURE" } }), + small(119, 3, { reviewDecision: "APPROVED" }), + small(120, 3, { reviews: { nodes: [{ author: { login: "redreceipt" }, state: "APPROVED", submittedAt: new Date(weekAgo + 1).toISOString() }], pageInfo: { hasNextPage: false, endCursor: null } } }), + small(121, 3, { reviewRequests: { nodes: [] } }), + small(122, 3, { reviewRequests: { nodes: [{ requestedReviewer: { login: "outsider" } }] }, timelineItems: { nodes: [requested("outsider", weekAgo)] } }), + small(123, 3, { reviewDecision: "CHANGES_REQUESTED", reviews: { nodes: [{ author: { login: "nlewis84" }, state: "CHANGES_REQUESTED", submittedAt: new Date(weekAgo).toISOString() }], pageInfo: { hasNextPage: false, endCursor: null } } }), + ]; + const priority = [small(130, 1), small(131, 2)]; + const message = reviewReminders([...eligible, ...excluded, ...priority], now)!; + const [high, other] = message.split("*Other PRs - Checks Passing, Waiting for Review (7+ days, <200 lines added)*"); + assert.ok(other); + for (const { pr } of eligible) { + assert.ok(other.includes(pr.url), pr.title); + assert.ok(!high.includes(pr.url), pr.title); + } + for (const { pr } of excluded) assert.ok(!message.includes(pr.url), pr.title); + for (const { pr } of priority) { + assert.ok(high.includes(pr.url), pr.title); + assert.ok(!other.includes(pr.url), pr.title); + } + assert.ok(!other.includes("<@U022N4RTD40>"), "Recent requests stay out even on old PRs"); + assert.ok(other.indexOf("|PR 106>") < other.indexOf("|PR 101>"), "Oldest requests first"); + assert.equal(reviewReminders(excluded, now), null); + assert.ok(reviewReminders(eligible, now)!.startsWith("*Other PRs")); +});